fix: validate the landing selection plan id before building URLs from it - #91
Conversation
SP_LANDING is stored globally and can outlive the summit it was set on,
so SummitLayout could redirect to /all-plans/{id}/profile for a plan the
user cannot submit to. SelectionPlanLayout's guard then bounced back to
/all-plans, which redirected again -> infinite loop.
Validate the id against summit.selection_plans and fall back to the
global /all-plans/profile route when it is not submittable. Allowed plans
keep going through SelectionPlanLayout, so per-plan marketing settings
still load.
NavMenu read SP_LANDING directly, so a landing plan the user cannot submit to produced My Bio / Tracks Guide / Selection Process links that the allowed-plan guard bounced back to /all-plans. Reuse getAllowedLandingSelectionPlanId so those fall back to the global routes, and stop filtering the summit documents by an unreachable plan.
jest was present in node_modules but absent from package.json, and the jest/babel configs were untracked, so `yarn test` did not exist and the suite could not run in CI or on a fresh clone. Pin jest, add the test script, and track both configs. babel.config.js is scoped to env.test, so the webpack build (which configures babel-loader with its own inline presets) is unaffected.
getSubmissionsPath read SP_LANDING directly, so the redirects after
saving a profile or finishing a presentation could point at
/all-plans/{id} for a plan the user cannot submit to, landing on the
allowed-plan guard. Take the summit and reuse
getAllowedLandingSelectionPlanId, falling back to the global path.
Mirrors the jest workflow already used in summit-admin, so the unit tests added alongside the SP_LANDING fixes actually gate changes instead of only running on a dev machine. Node 22 matches .nvmrc.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR validates stored landing selection plans against the current summit, updates summit-aware submission redirects and navigation, removes an unused import, and adds Jest configuration with GitHub Actions test execution. ChangesSubmission routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR validates stored landing selections before building URLs and adds test tooling; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/jest.yml (1)
14-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse an immutable dependency install.
If
package.jsonandyarn.lockdrift, plainyarn installcan resolve or write dependency versions instead of failing. Use the lockfile enforcement flag for the declared Yarn version, such as--frozen-lockfilefor Yarn Classic or--immutablefor Yarn Berry.🤖 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 @.github/workflows/jest.yml at line 14, Update the dependency installation step in the Jest workflow from plain yarn install to the lockfile-enforcing option supported by the repository’s declared Yarn version, using --frozen-lockfile for Yarn Classic or --immutable for Yarn Berry.jest.config.js (1)
1-4: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd browser-level tests for redirect and navigation behavior.
testEnvironment: 'node'is sufficient for the only matched test file. That file mocksglobal.localStorageand uses no other browser globals. The application contains redirect and navigation paths, but no tests cover them.🤖 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 `@jest.config.js` around lines 1 - 4, Add browser-level tests covering the application’s redirect and navigation behavior, while preserving the existing Jest configuration and its node test environment. Add the tests under the existing src test-match pattern and mock only the browser globals required by those paths, including localStorage where needed.
🤖 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 @.github/workflows/jest.yml:
- Around line 3-6: Add explicit read-only workflow permissions using contents:
read at the workflow or build job level near the existing on and jobs
configuration. Ensure the workflow does not inherit broader GITHUB_TOKEN
permissions.
- Around line 8-10: Update the actions/checkout step in the Jest workflow to set
persist-credentials to false, preserving fetch-depth: 0 and avoiding credential
persistence unless a later workflow step explicitly requires Git authentication.
- Around line 8-13: Update the actions/checkout and actions/setup-node uses in
the Jest workflow from v3 to the repository-approved supported major versions,
preserving their existing configuration; then rerun actionlint to verify the
workflow.
In `@src/utils/methods.js`:
- Around line 283-284: Update the selection plan ID parsing near the storedId
handling in src/utils/methods.js: replace parseInt validation with Number and
require a positive safe integer, or equivalent canonical positive-integer
validation, before matching a plan. Add tests in src/utils/methods.test.js lines
46-52 covering “123abc” and “1.5”, both expecting null.
---
Nitpick comments:
In @.github/workflows/jest.yml:
- Line 14: Update the dependency installation step in the Jest workflow from
plain yarn install to the lockfile-enforcing option supported by the
repository’s declared Yarn version, using --frozen-lockfile for Yarn Classic or
--immutable for Yarn Berry.
In `@jest.config.js`:
- Around line 1-4: Add browser-level tests covering the application’s redirect
and navigation behavior, while preserving the existing Jest configuration and
its node test environment. Add the tests under the existing src test-match
pattern and mock only the browser globals required by those paths, including
localStorage where needed.
🪄 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
Run ID: af1a8a25-3ad1-4d57-9323-f7501b9e42d9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/jest.ymlbabel.config.jsjest.config.jspackage.jsonsrc/actions/speaker-actions.jssrc/components/nav-menu/index.jssrc/layouts/summit-layout.jssrc/pages/preview-presentation-page.jssrc/pages/profile-page.jssrc/pages/thankyou-presentation-page.jssrc/utils/methods.jssrc/utils/methods.test.js
💤 Files with no reviewable changes (1)
- src/pages/profile-page.js
There was a problem hiding this comment.
Pull request overview
This PR fixes an infinite redirect loop caused by using a globally-stored landing selection plan ID (SP_LANDING) without validating it against the currently-loaded summit’s allowed/submittable selection plans, and it also makes the repo’s unit test setup runnable in CI.
Changes:
- Introduces
getAllowedLandingSelectionPlanId(summit)and updates all URL-building consumers to fall back to safe “global” routes when the stored ID is stale/invalid. - Updates
getSubmissionsPathto validate against the current summit before returning a plan-specific path, and adjusts call sites accordingly. - Adds Jest + Babel/Jest configs, a small unit test suite, and a GitHub Actions workflow to run tests.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/methods.js |
Adds validation helper and makes getSubmissionsPath summit-aware to avoid stale-ID redirects. |
src/utils/methods.test.js |
Adds unit tests for the new helper and the updated submissions path behavior. |
src/layouts/summit-layout.js |
Uses validated landing plan ID to prevent redirect loops for users without a profile. |
src/components/nav-menu/index.js |
Uses validated landing plan ID so menu links don’t send users into guarded routes that bounce back. |
src/actions/speaker-actions.js |
Updates post-save redirect to use summit-aware getSubmissionsPath. |
src/pages/thankyou-presentation-page.js |
Updates navigation to use summit-aware getSubmissionsPath. |
src/pages/preview-presentation-page.js |
Updates navigation to use summit-aware getSubmissionsPath. |
src/pages/profile-page.js |
Removes now-unused getSubmissionsPath import. |
package.json |
Adds jest and yarn test script. |
jest.config.js |
Adds Jest configuration for test discovery and environment. |
babel.config.js |
Adds Babel config scoped to test environment for Jest transforms. |
.github/workflows/jest.yml |
Adds CI workflow to run unit tests on pushes/PRs. |
yarn.lock |
Locks new/updated dependencies for Jest and related tooling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
Fix is at the right layer and the mechanism holds up. Verified independently:
- The predicate matches
SelectionPlanLayout's guard exactly (selection-plan-layout.js:30-32), which is why the loop closes. - Load ordering is safe.
summit.selection_plansis replaced wholesale by/selection-plans/me(base-reducer.js:98), andSummitLayoutreturns null untildataLoaded(summit-layout.js:37), set only aftergetAllowedSelectionPlansresolves. NavMenu renders only underall-plans-layout, inside that gate. No consumer sees the pre-melist. - Not clearing an invalid
SP_LANDINGlooks like an omission but is right:summitis undefined early, so remove on invalid would wipe a valid landing plan before the allowed list arrives. - Suite runs green at
a14e934, 9/9, on a clean checkout withjest@30.4.2plusbabel-jestinstalled.
Four things outside the inline comments:
yarn.lock picked up an npm resolved tree. The branch adds 202 registry.npmjs.org entries against a lockfile that is otherwise 1001 registry.yarnpkg.com entries, and @babel/core, parser, traverse and types are each now resolved twice (7.29.0/7.29.1 and 7.29.7/7.29.8) instead of deduping onto versions that already satisfy jest's ^7.27.4. That is install and CI time for nothing. npx yarn-deduplicate && yarn install should collapse it.
nav-menu/index.js:38 still wraps parseInt(currentSP || landingSP). landingSP is a number now, so the wrapper is dead. Cosmetic.
Doc filtering in NavMenu changes as a side effect. With a stale landing id and no currentSP, otherDocs (nav-menu/index.js:34-53) used to filter to the stale plan's docs, likely showing none, and now falls through to showing every selection plan doc. Reads as an improvement, but it is the one user visible change nobody has looked at. Worth folding into the browser pass already flagged for the redirect itself.
Two workflow nits on top of what CodeRabbit and Copilot already flagged: cache: yarn on setup-node, and testMatch in jest.config.js will not pick up *.test.jsx when component tests arrive.
Its three callers were all converted to getAllowedLandingSelectionPlanId in this branch; grep across src/ finds no remaining reference. Leaving the unvalidated reader exported next to the validated one invites reintroducing the redirect-loop bug this PR fixes.
The docstring said 'not submittable' / summit-layout's redirect comment said
'closed plan', but the check only verifies membership in
summit.selection_plans, which is not filtered by submission window
(summit-api OAuth2SummitSelectionPlansApiController::getMySelectionPlans
filters on summit_id, is_enabled and allowed_member_email only). A closed
plan the member is still allowed on correctly stays in the list -- speakers
need /all-plans/{id} to see what they submitted after the window shuts.
Reword to 'not among the member's allowed plans for this summit', and note
the predicate mirrors selection-plan-layout.js's guard on purpose: that
identity is what closes the redirect loop.
setLanding assigned global.localStorage and never restored it. Harmless with a single test file today, but a second file assuming a clean global would break depending on run order.
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
Thanks, the four items are addressed. One leftover on the reword: the same "cannot submit" framing is still in three other places, and 2e5d1d0's commit message says summit-layout's comment was reworded but that diff only touches src/utils/methods.js.
The contract is membership in summit.selection_plans, not submittability. /selection-plans/me filters on summit_id, is_enabled and allowed_member_email only, so a plan whose submission window has closed is still in that array and the helper deliberately keeps it. These three read the other way:
src/utils/methods.test.js:39
// stale SP_LANDING: id kept from another summit or a closed plan.The second half names a case the helper accepts. Suggest: // stale SP_LANDING: id from another summit, or a plan no longer allowed for this member.
src/layouts/summit-layout.js:42
// only keep the landing plan in the URL if the user can actually submit to it, otherwiseSuggest: // only keep the landing plan in the URL if it is still one of the user's allowed plans, otherwise
src/components/nav-menu/index.js:27
// a landing plan the user cannot submit to would build menu links that the allowed-planSuggest: // a landing plan outside the user's allowed set would build menu links that the allowed-plan
Comments only, no behavior change. Rest of the PR looks good to me.
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
Once those outstanding "cannot submit" items are cleared up then it's all good.
Ref.: https://app.clickup.com/t/86b959jmg
Alternative to #87, fixing the same redirect loop at its cause instead of at the routing layer.
Root cause
SP_LANDINGis stored in localStorage globally, not per summit (src/app.js:113-117), so it can hold a selection plan id belonging to another summit or one the member is no longer allowed on. Three places built URLs from that value without validating it, and every one of them fed a route thatSelectionPlanLayout's allowed-plan guard bounces back — which is what produced the infinite loop on/all-plans/{id}/profile.Fix
One helper,
getAllowedLandingSelectionPlanId(summit), validates the stored id againstsummit.selection_plansand returnsnullwhen the id is not among the member's allowed plans for this summit. The three consumers reuse it:SummitLayout's no-speaker-profile redirect — falls back to/all-plans/profileNavMenulinks (My Bio / Tracks Guide / Selection Process) — fall back to the global routesgetSubmissionsPath()— falls back toall-plansWhy not #87's approach
That PR adds a dedicated route that bypasses
SelectionPlanLayoutentirely. That also skipsgetSelectionPlanSettings, which is the only thing populatingselectionPlansSettingsfor a plan, so a show customizingCFP_SPEAKERS_SINGULAR_LABELsilently falls back to "Speaker" on a direct load. It also leaves two divergent route definitions for the same URL, and leaves the stale id driving the other menu links. Here allowed plans keep going throughSelectionPlanLayout, so per-plan settings still load.Also included
jestwas innode_modulesbut not inpackage.json, and the jest/babel configs were untracked, so the suite could not run outside a dev machine. Pinned it, addedyarn test, tracked both configs, and added ajestworkflow mirroring summit-admin's.Verification
9 unit tests on the helper and
getSubmissionsPath, verified red-then-green.yarn build-devcompiles. Fresh-cloneyarn install+yarn testboth exit 0.Not verified in a browser — that needs a summit with a user without a speaker profile and a stale
SP_LANDING.Summary by CodeRabbit
Bug Fixes
Tests
Chores