Skip to content

fix(router-core): commit memory-history navigations in Node - #8560

Open
00200200 wants to merge 1 commit into
TanStack:mainfrom
00200200:fix/memory-history-navigate-node
Open

00200200 wants to merge 1 commit into
TanStack:mainfrom
00200200:fix/memory-history-navigate-node

Conversation

@00200200

@00200200 00200200 commented Sep 29, 2026 •

Copy link
Copy Markdown

🎯 Changes

Fixes #8479.

After #8354, navigate() / commitLocation() treated every Node process as a server render. A router created with createMemoryHistory() then silently did nothing, including in node scripts and test runners that do not set NODE_ENV=test.

The no-op is now limited to:

  • isServer: true
  • createServerHistory() (what the SSR request handler already uses)
  • a router with no history yet

Memory and browser history still commit, including in Node. isServer: true with memory history is unchanged.

✅ Checklist

  • I have followed the steps in the Contributing guide.
  • I have tested code changes locally with the relevant test commands, or tests do not apply to this pull request.
  • I fully understand the code in this pull request, including any code generated with AI assistance.

🚀 Release Impact

  • This change affects published code, and I have generated a changeset.
  • This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

  • Bug Fixes
    • Navigation using memory history now commits location changes when running in Node, keeping router state and history in sync.
    • Navigation remains a no-op for server-request histories and routers explicitly configured for server-side operation.
  • Documentation
    • Clarified when navigation commits are skipped during server-side rendering; request redirects should still use the redirect API.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: TanStack/router/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: dac6bd44-07b5-4b2b-b68c-002ab7ec8803

📥 Commits

Reviewing files that changed from the base of the PR and between 41ebd28 and 5b94b70.

📒 Files selected for processing (6)
  • .changeset/soft-memory-navigate.md
  • docs/router/guide/ssr.md
  • packages/history/src/index.ts
  • packages/history/tests/createServerHistory.test.ts
  • packages/router-core/src/router.ts
  • packages/router-core/tests/server-history.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Router histories identify server history with an optional marker. Router location commit methods now no-op when the router is server-side, has no history, or uses server history. Tests cover memory-history commits and server-history no-ops.

Changes

Navigation commit behavior

Layer / File(s) Summary
Identify server history
packages/history/src/index.ts, packages/history/tests/createServerHistory.test.ts
RouterHistory adds an optional isServerHistory property, and ServerHistory sets it to true. Tests check the marker on server and memory histories.
Apply the shared commit guard
packages/router-core/src/router.ts, packages/router-core/tests/server-history.test.ts, docs/router/guide/ssr.md, .changeset/soft-memory-navigate.md
commitLocation, buildAndCommitLocation, and navigate use a shared no-op check. Tests cover memory-history commits and server-history no-ops. The SSR guide and changeset describe the no-op conditions.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: sheraff

Merge Risk: ⚪ Minimal · up to 5b94b

Memory-history navigation in Node can commit while server-history navigation remains a no-op. No actionable merge-blocking risk is established.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 5b94b

Built-in server histories remain protected, but memory and custom histories can now navigate in Node. Applications relying on server-side navigation guards or request-scoped no-op behavior may need to account for the changed behavior. No specific exploit has been established.

Retained concerns

  • Medium · security · inferred: Server request isolation now depends on an explicit server option or history marker. An unmarked custom server history can pass the commit guard and run another server load; whether any production consumer does so is unknown.
  • Low · security · observed: Newly enabled Node memory-history commits do not invoke registered push/replace blockers when document is absent. This matters if an application treats a blocker as a navigation control; no such security use is established here.
  • Low · reliability · inferred: A newly reachable server-mode memory commit can leave its navigation promise pending if server loading throws a non-redirect error before publication. This limits failure containment for callers awaiting navigation.
Security review details

Security Blast Radius

  • inferred — The newly reachable work is scoped to routers configured with an unmarked history in Node. A wider request or tenant exposure would depend on how an application constructs, shares, and invokes those routers; that exposure is not established.

Security Findings and Attack Paths

  • inferred — If an application lets an untrusted input drive navigation on an unmarked server-side router, navigation can now reach server route execution. The reviewed evidence identifies neither such an input path nor a sensitive application loader.

Trust Boundaries and Controls

  • observed — The commit guard trusts router configuration and the history object's marker, not the router's inferred server mode. Built-in server history supplies the marker, and explicit isServer remains a separate control.

Resilience and Maintainability Implications

  • observed — Successful history pushes notify subscribers, and commits without subscribers initiate loading. The changed no-op checks run before pending-location and commit-promise mutation.

Hardening Proposals

  • proposed — Define the server-side contract for custom histories and Node blockers explicitly, and settle commit promises when a newly permitted server load fails.
🚥 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the repository template. It explains the cause, scope, behavior, tests, checklist status, and changeset impact.
Title check ✅ Passed The title clearly and concisely describes the primary change: memory-history navigations now commit in Node.
Linked Issues check ✅ Passed The change meets the coding requirements in [#8479]. shouldNoOpLocationCommit skips commits only when isServer === true, history is absent, or history.isServerHistory === true. Memory history th…
Out of Scope Changes check ✅ Passed The changes stay within [#8479]. The server-history marker, router commit guard, focused tests, SSR documentation update, and release changeset all support the Node memory-history fix or preserve serv…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

This branch has not been deployed

No deployments
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.

router.commitLocation is a no-op on 1.170.36 / router-core 1.171.30, so router.navigate() silently does nothing

1 participant