You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Line anchors verified at cee658a3 (the head of feat/preserve-scroll-opt-out, PR #1446). The defect predates that branch and is unrelated to it; it was found while threading a new option through the same call.
Problem
navigate(url, { replace: true }) is a documented public API that does not replace anything, and its actual behaviour is worse than a no-op.
packages/core/src/router-client/navigator.js:200 passes the option straight into the SECOND positional parameter of performNavigation, which is isPopState, not a replace flag:
performNavigation(href, isPopState, frameId, opts) is declared at navigator.js:439. So { replace: true } tells the router "this is a back/forward traverse". Two things follow.
1. The URL never changes.recordHistory is computed as !isPopState && !refresh (navigator.js:660), so it is false. That flag gates BOTH history.pushState sites, at packages/core/src/router-client/fetch-apply.js:211 and :320 (the latter through the recordHistoryNow closure). There is no history.replaceState call anywhere in packages/core/src (grep -rn 'replaceState' packages/core/src returns nothing). So no entry is pushed, none is replaced, and the address bar keeps pointing at the page the reader just left while the document shows a different one. Press Back and you land somewhere unrelated to what is on screen.
2. It takes the back-button restore path.navigator.js:531 branches on isPopState and calls snapshotGet(href) (snapshot-cache.js:69). On a cache hit a forward navigation is served from the snapshot cache AND gets the traverse treatment: the recorded height reservation and the scroll-offset replay that #1428 built for real traverses. A forward navigate() can therefore land at a scroll offset recorded during some earlier visit.
The option is typed in both declaration files (packages/core/index.d.ts:91, packages/core/src/router-client.d.ts:5) and documented on three surfaces, one of which states the contract explicitly:
website/app/docs/routing/page.ts:596 , "history entry by default; pass { replace: true } to replace"
Nothing in the repo calls navigate with replace, so only an app following the docs hits it. That is also why it went unnoticed: test/types/route-types.test-d.ts:137 exercises the option, but it is a TYPE test and only asserts the call compiles.
Design / approach
Give performNavigation an explicit replace input rather than overloading isPopState, and make the history commit honour it.
The two concepts must stay separate, and conflating them is the whole bug. isPopState means "the browser already moved the history pointer, so write no entry and consider the snapshot cache". replace means "write an entry, but overwrite the current one instead of pushing". A replace navigation is a FORWARD navigation in every other respect: it should apply the optimistic loading skeleton, scroll to top by default, and never consult the traverse restore path.
The recommended shape:
Carry replace on the existing opts bag that performNavigation already takes (navigator.js:439), beside refresh and preserveScroll. Do not add a fifth positional.
Leave isPopStatefalse for a navigate() call, always. That alone restores the optimistic skeleton (navigator.js:522) and keeps the snapshot branch (navigator.js:531) out of the path.
Keep recordHistory TRUE for a replace, and thread the replace choice down to the commit site so fetch-apply.js calls history.replaceState(null, '', finalUrl) instead of pushState. Both commit sites need it: the 204/205 short-circuit at fetch-apply.js:211 and the recordHistoryNow closure at :320. The closure is the one that matters for ordering, and dogfood: record history before the swap so iOS back-swipe is not blank #1406 fixed that ordering deliberately (history is committed BEFORE the DOM mutation), so change WHICH history method is called and not WHEN it is called.
Prior art, worth reading before settling the shape: Next's router.replace and Remix's navigate(to, { replace: true }) both keep replace on the forward path and differ only in the history method. Neither routes a replace through its popstate handling. Clones are at ~/Documents/Projects/frameworks/next.js and ~/Documents/Projects/frameworks/remix.
Decide and record one open question: whether replace should also suppress the outgoing SNAPSHOT write (snapshotCurrent), since the entry it would be keyed against is about to be overwritten. Look at what the snapshot is keyed by before deciding; a stale snapshot under a replaced URL is the failure mode to avoid.
Implementation notes (for the implementing agent)
Where to edit
packages/core/src/router-client/navigator.js:192-203 , navigate(). The one-line defect.
packages/core/src/router-client/navigator.js:439 , performNavigation(href, isPopState, frameId, opts). Widen the opts JSDoc (its @param block is just above) and unpack replace beside const refresh = ... and const preserveScroll = ....
packages/core/src/router-client/navigator.js:660 , the foreground fetchAndApply call, which computes recordHistory and passes the opts bag.
packages/core/src/router-client/fetch-apply.js:65 , fetchAndApply(...), whose tenth argument is now an opts bag carrying refresh / noPrefetch / preserveScroll (PR feat: add data-preserve-scroll to opt out of the forward-nav scroll #1446 moved those off the positional list). Add replace there.
packages/core/src/router-client/fetch-apply.js:211 and :320 , the two history.pushState sites.
packages/core/index.d.ts:91 and packages/core/src/router-client.d.ts:5 , both already declare replace?: boolean, so they need no change. Verify rather than assume.
Landmines
Do not "fix" this by making isPopState mean two things. The parameter is read in five places (navigator.js:452, :522, :531, :660, plus the JSDoc at :419), and each one wants the traverse meaning.
History ordering is load-bearing and was fixed deliberately.dogfood: record history before the swap so iOS back-swipe is not blank #1406 moved the history commit to BEFORE the DOM mutation, and packages/core/src/router-client/swap.js:52 plus constants.js:46 document the commit order. recordHistoryNow exists to make that ordering explicit and idempotent (historyRecorded guard). Change the METHOD, not the position.
The scroll block keys off recordHistory.fetch-apply.js scrolls inside if (recordHistory && !frameId). Making recordHistory true for a replace (correctly) means a replace navigation will now scroll to top where today it silently does not. That is the correct behaviour and matches Next, but it IS an observable change, so state it in the PR body rather than letting it look like a regression. data-preserve-scroll / navigate(url, { scroll: false }) (dogfood: no per-link opt-out from the forward-nav scroll-to-top #1436) is the opt-out.
packages/core/dist is BUILT. e2e and Bun resolve the built bundle, so a src-only edit is invisible to them until npm --workspace @webjsdev/core run build:dist runs.
Invariants to respect
packages/ is plain .js with JSDoc. Never add a .ts file there (AGENTS.md).
Progressive enhancement: navigate() is a programmatic client-only API, so nothing about a page's correctness may depend on it.
WebJs has no users yet, so prefer a clean fix over a back-compat shim.
Tests
Browser (the headline layer), packages/core/test/routing/browser/. linkedom implements no history stack, so the assertion that matters (the URL advanced, and the entry count did not grow) belongs here. Model the fixture on nav-preserve-scroll.test.js or frame-swap-scroll.test.js: both install installNavGuard, keep the page's own query string on every href (a link that drops it takes the whole web-test-runner session down while every test still passes), and repeat the live boundary KEY so the router swaps rather than degrading to a full load. Assert: navigate(url) pushes (URL advances, history.length grows by one); navigate(url, { replace: true }) replaces (URL advances, history.length unchanged); and after a replace, one Back leaves the page that preceded the replaced entry. Run all three engines with npm run test:browser.
Counterfactual, mandatory: revert the replaceState call to pushState and confirm the replace case reds. Record the toggle and the commit it was proven at in the test file header, per the repo's counterfactual-decay rule.
Unit, packages/core/test/routing/router-client.test.js, for the option plumbing. Note its installNavigationMocks stubs history as { pushState: () => {}, replaceState: () => {} } (around :1561), so it can record WHICH method was called even though it models no real stack. That is the cheap assertion that catches the option name.
e2e / Bun: not applicable. This is browser-only client-router code and is on none of AGENTS.md's runtime-sensitive surfaces, so no test/bun/** file is needed.
Docs
website/app/docs/routing/page.ts:596 and website/app/docs/client-router/page.ts:270 , the two docs-site statements of the contract.
.agents/skills/webjs/references/client-router-and-streaming.md:69 , the agent-facing copy.
Note the scroll consequence wherever the replace contract is stated, since a replace will now scroll to top like any forward navigation.
Invoke the webjs-doc-sync skill: this changes the behaviour of an already-documented API, which is exactly its trigger.
Acceptance criteria
navigate(url, { replace: true }) advances the URL to url
It replaces the current history entry rather than pushing, so the entry count does not grow
After a replace, one Back goes to the page that preceded the replaced entry
navigate(url) with no options still pushes, unchanged
A replace navigation no longer consults the snapshot cache or the traverse restore path
A replace navigation applies the optimistic loading skeleton like any forward navigation
isPopState is never set true by a navigate() call
The scroll consequence is deliberate, documented, and opt-out-able via navigate(url, { scroll: false })
Problem
navigate(url, { replace: true })is a documented public API that does not replace anything, and its actual behaviour is worse than a no-op.packages/core/src/router-client/navigator.js:200passes the option straight into the SECOND positional parameter ofperformNavigation, which isisPopState, not a replace flag:performNavigation(href, isPopState, frameId, opts)is declared atnavigator.js:439. So{ replace: true }tells the router "this is a back/forward traverse". Two things follow.1. The URL never changes.
recordHistoryis computed as!isPopState && !refresh(navigator.js:660), so it isfalse. That flag gates BOTHhistory.pushStatesites, atpackages/core/src/router-client/fetch-apply.js:211and:320(the latter through therecordHistoryNowclosure). There is nohistory.replaceStatecall anywhere inpackages/core/src(grep -rn 'replaceState' packages/core/srcreturns nothing). So no entry is pushed, none is replaced, and the address bar keeps pointing at the page the reader just left while the document shows a different one. Press Back and you land somewhere unrelated to what is on screen.2. It takes the back-button restore path.
navigator.js:531branches onisPopStateand callssnapshotGet(href)(snapshot-cache.js:69). On a cache hit a forward navigation is served from the snapshot cache AND gets the traverse treatment: the recorded height reservation and the scroll-offset replay that #1428 built for real traverses. A forwardnavigate()can therefore land at a scroll offset recorded during some earlier visit.The option is typed in both declaration files (
packages/core/index.d.ts:91,packages/core/src/router-client.d.ts:5) and documented on three surfaces, one of which states the contract explicitly:website/app/docs/routing/page.ts:596, "history entry by default; pass{ replace: true }to replace"website/app/docs/client-router/page.ts:270.agents/skills/webjs/references/client-router-and-streaming.md:69, "replace history"Nothing in the repo calls
navigatewithreplace, so only an app following the docs hits it. That is also why it went unnoticed:test/types/route-types.test-d.ts:137exercises the option, but it is a TYPE test and only asserts the call compiles.Design / approach
Give
performNavigationan explicitreplaceinput rather than overloadingisPopState, and make the history commit honour it.The two concepts must stay separate, and conflating them is the whole bug.
isPopStatemeans "the browser already moved the history pointer, so write no entry and consider the snapshot cache".replacemeans "write an entry, but overwrite the current one instead of pushing". A replace navigation is a FORWARD navigation in every other respect: it should apply the optimistic loading skeleton, scroll to top by default, and never consult the traverse restore path.The recommended shape:
replaceon the existingoptsbag thatperformNavigationalready takes (navigator.js:439), besiderefreshandpreserveScroll. Do not add a fifth positional.isPopStatefalsefor anavigate()call, always. That alone restores the optimistic skeleton (navigator.js:522) and keeps the snapshot branch (navigator.js:531) out of the path.recordHistoryTRUE for a replace, and thread the replace choice down to the commit site sofetch-apply.jscallshistory.replaceState(null, '', finalUrl)instead ofpushState. Both commit sites need it: the 204/205 short-circuit atfetch-apply.js:211and therecordHistoryNowclosure at:320. The closure is the one that matters for ordering, and dogfood: record history before the swap so iOS back-swipe is not blank #1406 fixed that ordering deliberately (history is committed BEFORE the DOM mutation), so change WHICH history method is called and not WHEN it is called.Prior art, worth reading before settling the shape: Next's
router.replaceand Remix'snavigate(to, { replace: true })both keep replace on the forward path and differ only in the history method. Neither routes a replace through its popstate handling. Clones are at~/Documents/Projects/frameworks/next.jsand~/Documents/Projects/frameworks/remix.Decide and record one open question: whether
replaceshould also suppress the outgoing SNAPSHOT write (snapshotCurrent), since the entry it would be keyed against is about to be overwritten. Look at what the snapshot is keyed by before deciding; a stale snapshot under a replaced URL is the failure mode to avoid.Implementation notes (for the implementing agent)
Where to edit
packages/core/src/router-client/navigator.js:192-203,navigate(). The one-line defect.packages/core/src/router-client/navigator.js:439,performNavigation(href, isPopState, frameId, opts). Widen theoptsJSDoc (its@paramblock is just above) and unpackreplacebesideconst refresh = ...andconst preserveScroll = ....packages/core/src/router-client/navigator.js:660, the foregroundfetchAndApplycall, which computesrecordHistoryand passes the opts bag.packages/core/src/router-client/fetch-apply.js:65,fetchAndApply(...), whose tenth argument is now anoptsbag carryingrefresh/noPrefetch/preserveScroll(PR feat: add data-preserve-scroll to opt out of the forward-nav scroll #1446 moved those off the positional list). Addreplacethere.packages/core/src/router-client/fetch-apply.js:211and:320, the twohistory.pushStatesites.packages/core/index.d.ts:91andpackages/core/src/router-client.d.ts:5, both already declarereplace?: boolean, so they need no change. Verify rather than assume.Landmines
isPopStatemean two things. The parameter is read in five places (navigator.js:452,:522,:531,:660, plus the JSDoc at:419), and each one wants the traverse meaning.packages/core/src/router-client/swap.js:52plusconstants.js:46document the commit order.recordHistoryNowexists to make that ordering explicit and idempotent (historyRecordedguard). Change the METHOD, not the position.recordHistory.fetch-apply.jsscrolls insideif (recordHistory && !frameId). MakingrecordHistorytrue for a replace (correctly) means a replace navigation will now scroll to top where today it silently does not. That is the correct behaviour and matches Next, but it IS an observable change, so state it in the PR body rather than letting it look like a regression.data-preserve-scroll/navigate(url, { scroll: false })(dogfood: no per-link opt-out from the forward-nav scroll-to-top #1436) is the opt-out.packages/core/distis BUILT. e2e and Bun resolve the built bundle, so asrc-only edit is invisible to them untilnpm --workspace @webjsdev/core run build:distruns.Invariants to respect
packages/is plain.jswith JSDoc. Never add a.tsfile there (AGENTS.md).navigate()is a programmatic client-only API, so nothing about a page's correctness may depend on it.Tests
packages/core/test/routing/browser/. linkedom implements no history stack, so the assertion that matters (the URL advanced, and the entry count did not grow) belongs here. Model the fixture onnav-preserve-scroll.test.jsorframe-swap-scroll.test.js: both installinstallNavGuard, keep the page's own query string on every href (a link that drops it takes the whole web-test-runner session down while every test still passes), and repeat the live boundary KEY so the router swaps rather than degrading to a full load. Assert:navigate(url)pushes (URL advances,history.lengthgrows by one);navigate(url, { replace: true })replaces (URL advances,history.lengthunchanged); and after a replace, one Back leaves the page that preceded the replaced entry. Run all three engines withnpm run test:browser.replaceStatecall topushStateand confirm the replace case reds. Record the toggle and the commit it was proven at in the test file header, per the repo's counterfactual-decay rule.packages/core/test/routing/router-client.test.js, for the option plumbing. Note itsinstallNavigationMocksstubshistoryas{ pushState: () => {}, replaceState: () => {} }(around:1561), so it can record WHICH method was called even though it models no real stack. That is the cheap assertion that catches the option name.test/bun/**file is needed.Docs
website/app/docs/routing/page.ts:596andwebsite/app/docs/client-router/page.ts:270, the two docs-site statements of the contract..agents/skills/webjs/references/client-router-and-streaming.md:69, the agent-facing copy.webjs-doc-syncskill: this changes the behaviour of an already-documented API, which is exactly its trigger.Acceptance criteria
navigate(url, { replace: true })advances the URL tourlnavigate(url)with no options still pushes, unchangedisPopStateis never set true by anavigate()callnavigate(url, { scroll: false })packages/core/distrebuilt before e2e