Skip to content

feat: per-activity CFP submission reopen on the activity form - #1042

Merged
smarcet merged 17 commits into
masterfrom
feature/per-activity-cfp-reopen
Aug 12, 2026
Merged

feat: per-activity CFP submission reopen on the activity form#1042
smarcet merged 17 commits into
masterfrom
feature/per-activity-cfp-reopen

Conversation

@caseylocker

@caseylocker caseylocker commented Aug 11, 2026

Copy link
Copy Markdown

ref: https://app.clickup.com/t/86bba82y3

Third of three PRs for per-activity CFP reopen (SDS sds/per-activity-cfp-reopen.md §6, plus the dated ## Amendments entry of 2026-08-10). summit-api #581 is merged and deployed to dev; call-for-presentations #90 is open. This is the exposure point: nothing a user can see changes until this merges.

What it does

An admin can grant a time boxed CFP reopen window on a single presentation from the activity form: pick 24, 48 or 72 hours or a custom value, confirm, and the block flips to show the deadline, who granted it, a Close now action, and a copyable deep link to the speaker's CFP editor.

  • Two thunks in the existing event-actions.js (reopenSubmissionPeriod, closeSubmissionPeriod) call the two admin endpoints
  • Two dedicated action types rather than reusing EVENT_UPDATED, whose reducer case replaces the entity wholesale and would null out selection_plan_id when handed the narrow expand
  • submission_reopened_by appended to all four copies of the shared expand string, so attribution survives an unrelated save
  • One new block in event-form.js, gated to presentations on an enabled selection plan whose submission window has ended

Verified end to end against dev: grant returns 201 and renders the reopened state with attribution, the deep link resolves, Close now returns 204 and restores the offer state, and an over ceiling value surfaces the server's own "hours must be between 1 and 168." in the snackbar. That last path was verified before the client side cap landed, and it is still the behaviour where CFP_MAX_REOPEN_HOURS is unset.

Notes for the reviewer

1. Two pre-existing instances of a defect this PR fixes, elsewhere in the app. One commit here fixes a bug where the new controls were completely inert: the page destructures its actions off props, the two new thunks were missing from that block, so the JSX resolved the module import instead of the dispatch bound prop and calling it returned an un-dispatched thunk. No request, no error. The same defect exists on master, untouched by this PR, at:

  • src/pages/tickets/registration-invitations-list-page.js:255 calls selectInvitation / unSelectInvitation directly rather than via props, so row selection and select all dispatch nothing and bulk email cannot be used
  • src/pages/speakers/edit-submission-invitation.js:58 calls getSentEmailsByTemplatesAndEmail directly inside the getInvitation(...).then(...) callback, while the lines around it correctly use this.props, so Email Activity never refreshes after navigating between invitations

Not fixed here, to keep this scoped. Flagging because that makes four known instances, which suggests a lint rule rather than case by case fixes.

2. The reopen ceiling is mirrored as a deploy time env value, per @smarcet's review. MAX_REOPEN_HOURS is exposed by no serializer, controller or route, so the client originally relied on the server's 412 to communicate the limit, which meant the admin only learned about it after confirming the dialog. The input is now capped from a new CFP_MAX_REOPEN_HOURS, following the .env.example plus window.* pattern this repo already uses for CFP_APP_BASE_URL. Named to match the server's own CFP_MAX_REOPEN_HOURS rather than the shorter form, so the two copies are greppable together. Three things worth knowing:

  • The clamp lives in getSelectedReopenHours(), which drives the button's disabled state, so it covers the presets as well as the custom entry: a ceiling configured below 72 cannot offer a preset the server would refuse. The max attribute alone enforces nothing, since a number input still accepts a typed over max value.
  • Unset means uncapped. A deployment that never sets it behaves exactly as it does today and the server's 412 stays the authoritative backstop, so drift in either direction is benign: too high is today's behaviour, too low blocks a value the server would have taken. Neither produces an invalid grant.
  • This is still a second copy of the server's cfp.max_reopen_hours, and it is worth being precise about what that buys. It is not a runtime config flip. dotenv-webpack substitutes process.env.* at build time, and summit-admin-docker copies .env.$APP_ENV and runs yarn build inside the image build, so changing the value means an image rebuild and redeploy, in a different repo from the server's Doppler and argocd config. What it buys over a hardcoded constant is that changing it needs no code change or PR here. If the ceiling is ever served, on the summit payload or via the config values endpoint, the env var is replaced by the served value and the second copy goes away.

3. We gate the offer as well as the display. The API reopens a submission only when the plan is enabled, its window has actually ended, and a grant is live. Keying the UI on the grant alone offered a button the server could only answer 412 for, and announced a deadline that was no longer operative once an admin extended the plan's submission_end_date past the grant. The whole block is now hidden when the reopen is not applicable. Hiding it was a deliberate choice over rendering it disabled with an explanation; say the word if you would rather see it greyed out. Thanks to @smarcet, whose review of the same invariant on call-for-presentations #90 is where this came from.

4. Stale response guard uses the entity id, not a per thunk sequence. The playbook prescribes a sequenced() guard. That instrument supersedes repeated calls to the same thunk, but the failure mode here was a single in flight response landing after the admin navigated to a different activity, which a per thunk sequence would not catch. Both reducer cases drop a response whose id does not match the loaded entity, and return the same state object so a discarded response cannot trigger a re-render. Reproduced in a browser by holding the request and navigating mid flight: without the guard the other activity flips to the reopened state.

5. Two pre-existing races left alone. summit-event-reducer.js:190 (EVENT_UPDATED) and :183 (UNPUBLISHED_EVENT) have the same unguarded shape, and EVENT_UPDATED is worse: it replaces the entity wholesale, so a late saveEvent response overwrites the newly loaded activity entirely rather than merging three fields. Out of scope here, but worth its own change.

6. Accepted limitation: an expired grant does not re-render on its own. There is no timer, so a page left completely untouched past the deadline keeps showing the reopened state until any interaction re-renders it. This matches the passive expiry design, and the server rejects anything attempted after expiry regardless.

7. Commit granularity. Three commits carry more than one concern (3e6fcc5f, 69e57fbd, 0f439b6d); their subjects need a comma or an "and", which is the tell. They were written before the atomic commits convention was adopted mid branch, and splitting after the fact needs interactive rebase, which is not available in this environment. Later commits are one concern each.

8. Error feedback pairing changed. Success now reports through snackbarSuccessHandler rather than showSuccessMessage. The feature already used the MUI confirm dialog and the MUI error snackbar, so a SweetAlert success left it mixing two feedback systems. The one pre-existing showSuccessMessage call in this file pairs Swal with authErrorHandler, which is consistent in the legacy direction; this makes ours consistent in the MUI direction.

Testing

167 suites, 1515 tests, lint clean on the changed files, production build compiles.

New coverage: thunk URLs and payloads, the expand string, the dedicated action types, the offer and granted states, the applicability invariants, custom hours validation, the configured ceiling and its uncapped fallback, the stale response guard, and a page level test that the actions are forwarded as dispatch bound props rather than raw imports. Every added test was mutation checked: an injected defect fails at least one of them.

Manual verification ran against api.dev.fnopen.com on a presentation with a closed selection plan, covering grant, attribution, deep link, revoke, and the over ceiling error path.

Summary by CodeRabbit

  • New Features
    • Added controls to reopen or close presentation-event submission periods.
    • Supports 24-, 48-, and 72-hour presets, plus custom durations with configurable limits.
    • Displays reopening status, deadline, administrator details, and optional speaker links.
    • Added confirmation dialogs, validation, and success/error notifications.
  • Bug Fixes
    • Event details now refresh correctly after submission periods are reopened or closed.
    • Prevented outdated event responses from overwriting current event information.
  • Tests
    • Added coverage for reopening, closing, validation, confirmations, and event-state updates.

caseylocker and others added 14 commits August 11, 2026 09:11
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Final review fix wave for the per-activity CFP reopen feature:

- Gate the reopen block on entity.selection_plan_id > 0 (matches the
  existing > 0 convention at the ProgressFlags gate a few lines down),
  not just isPresentation()/!isNew(). Without a selection plan the
  server 412s the reopen call, so the button previously offered an
  action that could only fail; this also removes the only path that
  could produce a /all-plans/null/ deep link.
- Disable the Reopen button when no valid hours value is selected
  (e.g. Custom left blank/non-numeric), so it doesn't silently no-op.
- Make isSubmissionReopened() null-safe with optional chaining, since
  epochToMomentTimeZone can return undefined (not null) when
  time_zone_id is missing.
- Drop the unreferenced edit_event.reopen_deep_link_unavailable i18n
  key.

Updated the event-form test fixtures (selection_plan_id + a matching
selectionPlansOpts entry) so existing reopen-flow tests still clear
the new gate, and added focused tests for the no-selection-plan and
disabled-button cases.

Co-Authored-By: Claude <noreply@anthropic.com>
parseInt let "-1" through as a truthy negative, so the Reopen button
stayed enabled, the confirm dialog named a deadline in the past, and
the request went out only for the server to 412 it.

Guard at the getter rather than at each call site, so the existing
truthiness checks in handleReopenSubmission and the button's disabled
prop both become correct without changing either.

Co-Authored-By: Claude <noreply@anthropic.com>
An API error rejects the returned promise and the form fire-and-forgets
it, which is the same shape as saveEvent and every other write thunk in
this file. Matching the repo convention was a deliberate choice, so
record it where the next reader will look rather than in a PR comment.

Co-Authored-By: Claude <noreply@anthropic.com>
The page destructured every other action from props but not these two, so
the JSX referenced the module import instead of the dispatch-bound prop.
Calling it returned an un-dispatched thunk: the Reopen and Close now
buttons issued no request and raised no error.

Unit tests could not catch this. EventForm is presentational and its
tests inject onReopenSubmission directly, so the page wiring is never
exercised; the full suite was green with the buttons inert. Found by
driving the real app against the dev API.

Co-Authored-By: Claude <noreply@anthropic.com>
The no-client-cap design makes an over-ceiling hours value an expected
outcome, not a fault: the server answers 412 and snackbarErrorHandler
puts its message ("hours must be between 1 and 168.") in front of the
admin, which is the only way the ceiling is discoverable. uicore's
response handler rejects after running that handler, so every such 412
also escaped as an unhandled rejection, reaching Sentry as a fault and
raising a full-screen error overlay in development.

Catch at the two call sites rather than in the thunks, keeping them the
same shape as saveEvent and the other write thunks.

Co-Authored-By: Claude <noreply@anthropic.com>
parseInt read "1.5" and "1e3" as 1, so both enabled the button and would
have silently granted a one hour window instead of what the admin typed.
"1e3" also never reached the server's 412 for 1000 hours, which is the
only way the ceiling is discoverable.

Verified in Chrome rather than assumed: a number input preserves the raw
"1e3" and "1.5" in .value, so this is reachable, not a jsdom artifact.
The test sets the value instead of typing it because jsdom normalises a
typed "1e3" to "1000", which would pass against the old code.

Co-Authored-By: Claude <noreply@anthropic.com>
Covers the regression fixed in 4e0d009, where the page forwarded the
module import instead of the dispatch-bound prop and the reopen controls
issued no request while the suite stayed green.

Asserts on store.dispatch rather than on the action creator. Once the
actions module is mocked the raw import and the connect-bound prop are
the same jest.fn, so asserting the creator was called passes even with
the bug present; only the dispatch assertion fails. Verified by
reintroducing the defect: both tests fail, and the creator assertion is
not the one that catches it.

Co-Authored-By: Claude <noreply@anthropic.com>
Both responses merged into the single current entity with no check of
which activity they belonged to. Granting or revoking on activity A and
navigating to B before the request finished left B showing A's grant,
with A's deadline and attribution and a deep link for a window that was
never opened on B.

The close action already carried its eventId and the reducer simply
never read it; the reopen action now carries one too, and both cases
drop a response whose id does not match the loaded entity. The guard
returns the same state object, so a discarded response cannot trigger a
re-render.

Reproduced in the browser by holding the request and navigating mid
flight: without the guard the other activity flips to the reopened
state when the response lands, with it the activity is untouched.

Co-Authored-By: Claude <noreply@anthropic.com>
Convention pass against the show-admin playbooks, including the async
lifecycle rules proposed in ftn-docsnsklz PR #64.

- Dispatch startLoading() before the token await. A refresh can take
  seconds, and anything dispatched after it leaves a window where the UI
  is neither blocked nor marked in-flight.
- Move stopLoading() into .finally(). snackbarErrorHandler happens to
  clear it on the error paths today, so this fixes no live defect, but
  .then() alone is one library change away from a stuck overlay.
- Give the custom-hours input an accessible name. It had only a
  placeholder, which is not a name, while the select beside it already
  had a label. The test now selects it by label rather than placeholder.
- Report success through snackbarSuccessHandler. The feature already
  used the MUI confirm dialog and the MUI error snackbar, so a
  SweetAlert success left it mixing two feedback systems. The one
  pre-existing showSuccessMessage call in this file pairs Swal with
  authErrorHandler, which is consistent in the legacy direction; this
  makes ours consistent in the MUI direction.

Co-Authored-By: Claude <noreply@anthropic.com>
The API reopens a submission only when three things hold: the selection
plan is enabled, its submission window has actually ended, and a grant
is live. The UI keyed on the grant alone, so it offered a Reopen button
the server could only answer 412 for, and it announced a deadline that
was no longer the operative one.

The case that matters: an admin grants a reopen, then extends the plan's
submission_end_date past it. The speaker is editing under normal
open-window rules again, but the panel still said "Reopened until
<grant expiry>". Gating the whole block covers both symptoms, since the
granted display lives inside it.

Found by smarcet's review of the same invariant on the
call-for-presentations side (fntechgit/call-for-presentations#90).

Co-Authored-By: Claude <noreply@anthropic.com>
@caseylocker caseylocker self-assigned this Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@caseylocker, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b13aad8-b12f-4735-aa2b-449f67bfea79

📥 Commits

Reviewing files that changed from the base of the PR and between d4111e5 and 66cfb80.

📒 Files selected for processing (2)
  • src/components/forms/__tests__/event-form.test.js
  • src/components/forms/event-form.js
📝 Walkthrough

Walkthrough

The PR adds controls to reopen and close presentation-event submission periods. It adds API thunks, reducer state, validation and confirmation logic, localized UI text, page wiring, configuration, speaker links, and test coverage.

Changes

Submission period management

Layer / File(s) Summary
Submission-period actions
src/actions/event-actions.js, src/actions/__tests__/event-actions.test.js
Event thunks call the reopen and close endpoints, dispatch event-scoped actions, show snackbar feedback, and expand submission_reopened_by data.
Event reopening state
src/reducers/events/summit-event-reducer.js, src/reducers/events/__tests__/summit-event-reducer.test.js
The reducer stores reopening deadlines and administrator data, applies matching responses, ignores stale events, and clears grants on close.
Reopening controls and validation
src/components/forms/event-form.js, src/components/forms/__tests__/event-form.test.js, src/utils/constants.js, .env.example, src/app.js, src/i18n/en.json
EventForm validates durations, confirms reopen and close actions, displays grant details, and renders optional speaker links. Constants, configuration, and English translations support the controls.
Edit-page action wiring
src/pages/events/edit-summit-event-page.js, src/pages/events/__tests__/edit-summit-event-page.test.js
The edit page connects the new action creators and passes their callbacks to EventForm. Tests verify both dispatch paths.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EventForm
  participant EditSummitEventPage
  participant eventActions
  participant API
  participant summitEventReducer
  EventForm->>EditSummitEventPage: submit reopen or close action
  EditSummitEventPage->>eventActions: dispatch thunk
  eventActions->>API: call submission-period endpoint
  API-->>eventActions: return event reopening response
  eventActions->>summitEventReducer: dispatch event-scoped result
  summitEventReducer-->>EventForm: provide updated grant state
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: per-activity CFP submission reopening in the activity form.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/per-activity-cfp-reopen

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/actions/__tests__/event-actions.test.js (1)

53-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the three identical request mocks into one factory.

getRequest, putRequest, and deleteRequest now share the same 24-line mock body. Only getRequest additionally records capturedParamsHistory. A single factory keeps them in sync when the capture logic changes.

♻️ Proposed refactor
+    const makeRequestMock = ({ track = false } = {}) =>
+      (requestActionCreator, receiveActionCreator) =>
+        (params = {}) =>
+        (dispatch) => {
+          capturedParams = params;
+          if (track) capturedParamsHistory.push(params);
+
+          if (typeof requestActionCreator === "function") {
+            dispatch(requestActionCreator({}));
+          }
+
+          return new Promise((resolve) => {
+            if (typeof receiveActionCreator === "function") {
+              dispatch(receiveActionCreator({ response: {} }));
+            } else {
+              dispatch(receiveActionCreator);
+            }
+            resolve({ response: {} });
+          });
+        };
+
+    getRequest.mockImplementation(makeRequestMock({ track: true }));
+    putRequest.mockImplementation(makeRequestMock());
+    deleteRequest.mockImplementation(makeRequestMock());
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/actions/__tests__/event-actions.test.js` around lines 53 - 127, Extract
the shared mock implementation from getRequest, putRequest, and deleteRequest
into one reusable request-mock factory, then configure each mock with it.
Preserve the existing request/receive dispatch and promise behavior, while
retaining capturedParamsHistory recording only for getRequest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/components/forms/event-form.js`:
- Around line 753-777: Update isReopenApplicable(), getReopenDeadline(),
handleReopenSubmission(), and handleCloseSubmission() to read entity from
this.state, matching the render gate and unsaved selection-plan changes. Use the
state entity consistently for plan eligibility, reopen deadline, and submitted
event ID, while preserving the existing behavior otherwise.

---

Nitpick comments:
In `@src/actions/__tests__/event-actions.test.js`:
- Around line 53-127: Extract the shared mock implementation from getRequest,
putRequest, and deleteRequest into one reusable request-mock factory, then
configure each mock with it. Preserve the existing request/receive dispatch and
promise behavior, while retaining capturedParamsHistory recording only for
getRequest.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5de7d5b8-180d-4a80-b074-742bd736d21b

📥 Commits

Reviewing files that changed from the base of the PR and between 26e2aa7 and 93867d8.

📒 Files selected for processing (10)
  • src/actions/__tests__/event-actions.test.js
  • src/actions/event-actions.js
  • src/components/forms/__tests__/event-form.test.js
  • src/components/forms/event-form.js
  • src/i18n/en.json
  • src/pages/events/__tests__/edit-summit-event-page.test.js
  • src/pages/events/edit-summit-event-page.js
  • src/reducers/events/__tests__/summit-event-reducer.test.js
  • src/reducers/events/summit-event-reducer.js
  • src/utils/constants.js

Comment thread src/components/forms/event-form.js
@caseylocker
caseylocker requested review from smarcet and a balanced review from Copilot August 11, 2026 20:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a per-activity CFP submission reopen control to the activity (event) form, the client-facing exposure point of a three-PR effort (summit-api #581, call-for-presentations #90). Admins can grant a time-boxed reopen window (24/48/72h or custom) on a single presentation; the block then flips to show the deadline, the granting admin, a "Close now" action, and a copyable deep link to the speaker's CFP editor. Two new thunks call dedicated admin endpoints, two dedicated reducer cases apply/clear the grant (avoiding EVENT_UPDATED's wholesale entity replacement), and a stale-response guard keyed on the entity id prevents a late response from bleeding onto a different activity.

Changes:

  • New reopenSubmissionPeriod/closeSubmissionPeriod thunks plus SUBMISSION_PERIOD_REOPENED/SUBMISSION_PERIOD_CLOSED action types and reducer cases (with an entity-id stale-response guard); submission_reopened_by appended to all four expand strings.
  • New reopen/close UI block in event-form.js gated to non-new presentations on an enabled plan whose submission window has ended, with confirm dialogs, custom-hours validation, and a speaker deep link.
  • Prop wiring in edit-summit-event-page.js, new i18n strings, new reopen-hours constants, and comprehensive unit tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/constants.js Adds DEFAULT_REOPEN_HOURS/REOPEN_PRESET_HOURS_48/_72 for the duration presets.
src/actions/event-actions.js Adds reopen/close thunks and action types; appends submission_reopened_by to the four expand strings; routes errors to snackbarErrorHandler and success to snackbarSuccessHandler.
src/reducers/events/summit-event-reducer.js Adds reducer cases that merge/clear grant fields with an entity-id stale-response guard, and grant defaults on DEFAULT_ENTITY.
src/components/forms/event-form.js Adds the reopen/close UI block, applicability/deadline helpers, and confirm-dialog handlers.
src/pages/events/edit-summit-event-page.js Destructures and connects the two new thunks and passes them to EventForm.
src/i18n/en.json Adds the reopen/close labels, confirm copy, and success messages.
src/reducers/events/tests/summit-event-reducer.test.js Tests reopen/close application, stale-response guard, and null coercion.
src/components/forms/tests/event-form.test.js Tests applicability gating, custom-hours validation, grant/offer states, and the deep link.
src/pages/events/tests/edit-summit-event-page.test.js Asserts the page forwards dispatch-bound thunks rather than raw imports.
src/actions/tests/event-actions.test.js Tests thunk URLs/payloads, action types, error routing, and the expand string.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/components/forms/event-form.js Outdated
The render gate reads entity from state while isReopenApplicable() and
getReopenDeadline() read it from props, and the two diverge:
handleChangeSelectionPlan writes selection_plan_id into state without
saving, and componentDidUpdate only syncs props into state, never back.

So after an admin picks a different plan in the dropdown, the gate reads
the new plan's id from state while eligibility is judged against the
persisted plan from props. The control could stay visible for a plan
whose window is still open, or hide for one that is eligible. Nothing
incorrect reached the server, which judges against the persisted plan,
but the control's visibility reflected a plan other than the one shown.

handleReopenSubmission and handleCloseSubmission move too, so the id
they submit comes from one source.

Not unit tested: every existing test passes one entity object as a prop
which the component copies into state, so both sources are identical and
no test can reproduce the divergence. Reproducing it needs the Selection
Plan dropdown driven for real; left to manual verification.

Reported by CodeRabbit on PR #1042.

Co-Authored-By: Claude <noreply@anthropic.com>
{T.translate("edit_event.reopen_custom_hours")}
</label>
<input
id="reopen_custom_hours"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caseylocker The custom hours input accepts any positive integer with no client-side ceiling, so the only guard against an over-limit value is the server's 412 after the admin has already confirmed the dialog — SDS §6 (sds/per-activity-cfp-reopen.md) specifies the custom entry "capped at MAX_REOPEN_HOURS".

The PR description's reason for not capping is fair — hardcoding 168 in a second place drifts silently the day the server config changes — but there's a middle path that doesn't require a backend change: make the ceiling an env value in this repo, following the existing env/config pattern (.env.example + window.* mapping in src/app.js, same as CFP_APP_BASE_URL):

  • .env.example: MAX_REOPEN_HOURS=168
  • src/app.js: window.MAX_REOPEN_HOURS = process.env.MAX_REOPEN_HOURS;
  • Here: set max={window.MAX_REOPEN_HOURS} on the input, include the value in getSelectedReopenHours()'s validity check so the button disables, and interpolate the real range into the label/i18n copy.

This is still a second copy of the server's cfp.max_reopen_hours, but it lives in per-environment deploy config next to the server's own config (both are ops-managed values, changed in the same place at the same time) rather than baked into the bundle, and it keeps the server's 412 as the authoritative backstop if they ever drift. If the env var is unset, fall back to the current uncapped behavior so nothing regresses.

This also closes the SDS §6 gap without waiting on the "expose the ceiling via serializer" backend question — and if that endpoint ever materializes, the env var is trivially replaced by the served value.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I was trying to avoid having it in 2 places but until we change the api to make that available this can be a good solution. Making the change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d4111e5e. Followed your recipe with three deviations, all small:

Named CFP_MAX_REOPEN_HOURS, not MAX_REOPEN_HOURS. The server's var is CFP_MAX_REOPEN_HOURS (config/cfp.php:22, .env.example:160 on summit-api main). Since the whole point is that the two copies move together, they should carry the same name and grep as a pair. It also matches the CFP_APP_BASE_URL prefix already here.

The clamp lives in getSelectedReopenHours(), not on the max attribute. max on a number input only sets :invalid and bounds the spinner, so a typed 500 still reaches value. The attribute is set for the browser's own affordance, but the enforcement is the button's disabled state, as you asked. Putting it in that method rather than on the custom branch means it covers the presets too: a ceiling configured below 72 now disables the 72h preset instead of silently offering something the server would refuse.

Coerced to Number once. dotenv values arrive as strings, so window.CFP_MAX_REOPEN_HOURS is "168". Comparison operators would have coerced it anyway, but anything later reaching for Math.min or Number.isInteger would not have.

Unset means uncapped exactly as you specified, so nothing regresses and the 412 stays authoritative. The custom hours label interpolates the real range now (Hours (1-168)), with the plain string kept for the uncapped case.

One correction on the rationale, because it changes what to expect at rollout rather than whether to do this. The value is still baked into the bundle. This repo uses dotenv-webpack (webpack.common.js:17), which substitutes process.env.* at build time, and summit-admin-docker copies .env.$APP_ENV then runs yarn build inside the image build (Dockerfile:62,66 into deployment.sh). So changing this is an image rebuild and redeploy, in a different repo from the server's Doppler and argocd config, where CFP_MAX_REOPEN_HOURS is a restart. Not the same place and not the same effort. What the env var genuinely buys over a hardcoded 168 is that changing it needs no code change or PR here, which is still worth having, just less than "ops flips a config". Worth knowing so nobody plans a live change on it.

Drift stays benign in both directions: FE too high is today's behaviour, FE too low blocks a value the server would accept. Neither produces an invalid grant.

Tests: 167 suites, 1515 tests green. Three new ones cover an over ceiling custom value, a value on the ceiling, and an over ceiling preset, plus one for the uncapped fallback. All mutation checked. PR body note 2 rewritten to describe the cap rather than argue against it.

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caseylocker please review

The custom hours entry accepted any positive integer, so an over ceiling
value was only refused by the server's 412, after the admin had already
confirmed the dialog. SDS section 6 specifies the custom entry capped at
MAX_REOPEN_HOURS.

The ceiling is server side config exposed by no serializer or route, so
mirror it as a deploy time env value rather than hardcoding it: the same
.env.example plus window.* mapping the repo already uses for
CFP_APP_BASE_URL. Named CFP_MAX_REOPEN_HOURS to match the server's own
var so the two twins are greppable together.

Applied in getSelectedReopenHours(), which drives the button's disabled
state, so it covers the presets as well as the custom entry: a ceiling
configured below 72 cannot offer a preset the server would refuse. The
max attribute alone would not enforce anything, since a number input
still accepts a typed over max value.

Unset means uncapped, so a deployment that never sets it behaves exactly
as before and the server's 412 stays the authoritative backstop. Values
arrive from dotenv as strings, hence the Number coercion.

Suggested by smarcet on PR #1042.

Co-Authored-By: Claude <noreply@anthropic.com>
@caseylocker
caseylocker requested a review from smarcet August 12, 2026 15:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/components/forms/event-form.js`:
- Around line 801-806: Update getSelectedReopenHours to require
Number.isSafeInteger(hours) alongside the existing positive digit validation
before applying the maximum-hours ceiling, returning 0 for oversized or
non-finite values even when CFP_MAX_REOPEN_HOURS is unset. Add a regression test
covering an oversized digit-string custom value.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 70c00617-65c2-4e1b-ba72-798248288f91

📥 Commits

Reviewing files that changed from the base of the PR and between 93867d8 and d4111e5.

📒 Files selected for processing (5)
  • .env.example
  • src/app.js
  • src/components/forms/__tests__/event-form.test.js
  • src/components/forms/event-form.js
  • src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/i18n/en.json

Comment thread src/components/forms/event-form.js
getSelectedReopenHours() accepted any digit-only value, and with no
ceiling configured nothing bounded it. Past roughly 2.4e9 hours
moment().add() yields an invalid date, unix() gives NaN, and uicore's
epochToMomentTimeZone returns NaN unwrapped rather than a moment, so the
confirm dialog's .format() throws.

The method is async and wired straight to onClick, so it surfaced as an
unhandled rejection: the button did nothing, no dialog, no message to the
admin, and a TypeError to Sentry. 9999999999 is enough to trigger it.

Guard on the deadline being representable rather than on the magnitude of
the input. A safe-integer check is not the boundary, since that value
passes one and still overflows, and every unsafe integer is far past the
range anyway, so one condition covers both.

Only reachable where CFP_MAX_REOPEN_HOURS is unset, since any configured
ceiling rejects these first.

Reported by CodeRabbit on PR #1042.

Co-Authored-By: Claude <noreply@anthropic.com>

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smarcet
smarcet merged commit 599ca9f into master Aug 12, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants