Skip to content

fix: navigate(url, { replace: true }) never replaces, and takes the traverse path #1449

Description

@vivek7405

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:

await performNavigation(target.href, opts?.replace ?? false, null, {
  preserveScroll: opts?.scroll === false,
});

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"
  • 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 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:

  1. Carry replace on the existing opts bag that performNavigation already takes (navigator.js:439), beside refresh and preserveScroll. Do not add a fifth positional.
  2. Leave isPopState false 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.
  3. 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

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 })
  • Back/Forward restore is untouched, and the dogfood: back-button scroll restores ~763px too low on pages that grow after swap #1310 / fix: back-button restore survives late layout growth #1313 / dogfood: iOS back-swipe still blank after #1410; A/B the snapshot timing on-device #1428 guards stay green
  • A counterfactual proves the new test actually fires, dated to the commit it was proven at
  • Tests at every layer the change touches, green on Chromium, Firefox and WebKit
  • packages/core/dist rebuilt before e2e
  • Every doc surface stating the replace contract is updated

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions