fix(router): preserve basePath in Pages Router events - #2888
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
/bigbonk reviewReviewed the full diff for Verification performed:
Findings1. [minor / docs] Stale comment describes the removed redirect mechanism — The comment above the
That is no longer how it works. Non-issues considered
No blocking issues. Recommend fixing the stale comment (can be a follow-up). |
|
Review posted to PR #2888. SummaryI reviewed the full diff ( Assessment: Solid, well-tested change. The core refactor — replacing the fire-and-forget Verified specifically:
One finding (minor/docs): The comment at No blocking issues. |
|
/bigbonk review for issues |
/bigbonk reviewRe-reviewed the full diff after the latest commits ( Verification performed
What I confirmed is correct1. Double-slash redirect security (defense-in-depth holds). For a validated same-origin absolute redirect whose app pathname starts with 2. 3. Redirect throw/unwind mechanism. 4. Prior finding — resolvedThe stale comment at router.ts:2342-2349 flagged in the earlier review (describing the removed "increment navigation id supersedes" mechanism) has already been corrected by commit Non-issues considered
No blocking issues. |
|
Review posted to PR #2888. SummaryI re-reviewed the full diff ( Assessment: no blocking issues. This is a well-constructed change that correctly achieves its goal of preserving basePath-visible URLs across all Pages Router events while aligning cancellation, redirect recursion, and event ordering with Next.js. What I verified as correct:
Prior finding resolved: the stale comment at router.ts:2342-2349 flagged in the earlier bonk review was already fixed by commit No local |
|
Independent targeted Next.js E2E verification on immutable head
The checkout remained clean and the local/remote PR head SHA matched after the run. |
|
/bigbonk review for issues |
/bigbonk reviewRe-reviewed the full diff ( Verification performed
What I confirmed is correct1. New popstate-redirect path ( 2. 3. 4. Redirect throw/unwind mechanism. 5. Option propagation on recursion. Push/replace passes 6. Prior finding resolved. The stale comment at router.ts:2342-2349 flagged in the earlier review was corrected by Non-issues considered
No blocking issues. This is a well-constructed, thoroughly tested change with correct dev/prod parity (all logic is client-only in the |
|
Review posted to PR #2888. SummaryI reviewed the full diff ( Assessment: no blocking issues. This is a well-constructed, thoroughly tested change with correct dev/prod parity (all router-event logic is client-only in the Key things I verified:
Two narrow non-issues considered and dismissed (redirect recursion depth cap, locale-prefix bypass for |
|
Fresh independent review at exact head Reviewed the full two-file diff with specific focus on the new popstate-redirect commit. Verified:
Independent validation from a clean detached checkout:
Local and remote head SHAs still match. PR merge state is currently UNSTABLE due to CI state, not a review finding. |
|
/bigbonk review for issues |
|
Fresh full deploy-suite run 31470591156 exposed three candidate-only Head |
/bigbonk reviewReviewed the full two-file diff ( Verification performed
What I confirmed is correct1. New eager-commit path ( 2. Double-slash redirect defense holds across all three entry points. For a validated same-origin absolute redirect whose app pathname begins with 3. 4. Popstate redirect recursion. Correctly threads Non-issues considered
Coverage note (non-blocking)The eager-commit path's supersession-during-render case (a second navigation arriving after the eager No blocking issues. This is a well-constructed, thoroughly tested change with correct dev/prod parity (all router-event logic is client-only in the |
|
Review posted to PR #2888. SummaryI reviewed the full two-file diff ( Assessment: no blocking issues. Well-constructed, thoroughly tested change with correct dev/prod parity (all router-event logic is client-only in the What I verified as correct:
One non-blocking coverage note: the eager-commit "superseded during render" edge case isn't directly asserted (low-risk given the gating). Two non-issues considered and dismissed (gSSP I couldn't find a real bug — the code was already reviewed multiple times and the security-critical paths are well-defended and thoroughly tested. |
…uter-events-31439707085 # Conflicts: # packages/vinext/src/shims/router.ts
Summary
Failure mapping
Fixes all four non-deferred failures from Actions run 31439707085, job 93624401572 in
test/e2e/basepath/router-events.test.ts:should use urls with basepath in router eventsshould use urls with basepath in router events for hash changesshould use urls with basepath in router events for cancelled routesshould use urls with basepath in router events for failed route changeValidation
REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" VINEXT_BUILD=0 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/basepath/router-events.test.tsvp test run tests/shims.test.ts: 1,293/1,293 passedvp check: passedvp run vinext#build: passed