fix(router-generator): make buildRouteTree route ordering deterministic#7751
fix(router-generator): make buildRouteTree route ordering deterministic#77510x80 wants to merge 6 commits into
Conversation
The final sort tiebreaker in buildRouteTree compared whole route-node objects ((d) => d). Two objects both coerce to "[object Object]", so the comparator returns -1 for both (a, b) and (b, a) — an inconsistent comparator. Routes tying on segment-count and index-token were then left in an engine- and input-order-dependent order, producing non-deterministic routeTree.gen.ts diffs across machines and Node versions. Compare routePath instead, mirroring the pre-sort's final accessor, for a stable total order.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe route generator now uses a shared deterministic route-node sorter with ChangesDeterministic route ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes non-deterministic route ordering during route-tree generation in @tanstack/router-generator by ensuring buildRouteTree uses a consistent, total ordering when sorting route nodes. This prevents semantically-identical routeTree.gen.ts churn across different Node/V8 versions and machines.
Changes:
- Replace the final
multiSortBytiebreaker inbuildRouteTreefrom comparing whole route-node objects to comparingroutePath. - Ensure sibling routes that tie on earlier sort keys still sort deterministically by a stable string key.
- Add a patch changeset for
@tanstack/router-generatordocumenting the determinism fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/router-generator/src/generator.ts | Fixes buildRouteTree sorting by using routePath as the final tiebreaker for deterministic generation. |
| .changeset/deterministic-route-tree-sort.md | Adds a patch changeset describing the deterministic route ordering fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
can you please add unit tests that prove stable sorting? |
…apshots Extract the buildRouteTree tiebreaker sort into an exported pure `sortRouteNodes` helper and cover it with unit tests. The key test feeds a route set that ties on every key before `routePath` through all input permutations and asserts identical output, proving the sort is order- and engine-independent. Regenerate the 38 generator snapshots whose ordering changes under the deterministic tiebreaker. The diffs are pure emission-order reordering (every route's id/path/parentRoute is unchanged) -- the same semantically-identical churn the fix eliminates going forward.
|
@schiller-manuel added — pushed in 85b7e95. To make the sort testable I pulled the Heads-up: the deterministic order also changes route emission order, so I had to regenerate the 38 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/router-generator/tests/utils.test.ts`:
- Around line 982-984: The one-line `if` in `permutations<T>` violates the
project’s brace-required control-flow style. Update the `if (arr.length <= 1)`
branch in `permutations` to use curly braces around its body, keeping the same
return behavior while matching the coding guidelines for all `if`/`else`
statements.
🪄 Autofix (Beta)
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: ceac5fba-5751-4ad3-8b46-5c6144b193f8
📒 Files selected for processing (42)
packages/router-generator/src/generator.tspackages/router-generator/src/index.tspackages/router-generator/src/utils.tspackages/router-generator/tests/generator/add-extensions-custom/routeTree.snapshot.tspackages/router-generator/tests/generator/custom-scaffolding/routeTree.snapshot.tspackages/router-generator/tests/generator/custom-tokens/routeTree.snapshot.tspackages/router-generator/tests/generator/dot-escaped/routeTree.snapshot.tspackages/router-generator/tests/generator/escaped-custom-tokens/routeTree.snapshot.tspackages/router-generator/tests/generator/escaped-special-strings/routeTree.snapshot.tspackages/router-generator/tests/generator/export-variations/routeTree.snapshot.tspackages/router-generator/tests/generator/file-modification/routeTree.snapshot.tspackages/router-generator/tests/generator/flat-route-group/routeTree.snapshot.tspackages/router-generator/tests/generator/flat/routeTree.snapshot.tspackages/router-generator/tests/generator/invalid-param-names/routeTree.snapshot.tspackages/router-generator/tests/generator/nested-layouts/routeTree.snapshot.tspackages/router-generator/tests/generator/nested-route-groups-with-layouts-before-physical/routeTree.snapshot.tspackages/router-generator/tests/generator/nested/routeTree.snapshot.tspackages/router-generator/tests/generator/numbers-in-path/routeTree.snapshot.tspackages/router-generator/tests/generator/physical-pathless-layout-escaped-underscore/routeTree.snapshot.tspackages/router-generator/tests/generator/prefix-suffix/routeTree.snapshot.tspackages/router-generator/tests/generator/regex-tokens-inline/routeTree.snapshot.tspackages/router-generator/tests/generator/regex-tokens-json/routeTree.snapshot.tspackages/router-generator/tests/generator/regex-tokens-plus-json/routeTree.snapshot.tspackages/router-generator/tests/generator/root-export-specifier/routeTree.snapshot.tspackages/router-generator/tests/generator/route-groups/routeTree.snapshot.tspackages/router-generator/tests/generator/routeFileIgnore/routeTree.snapshot.tspackages/router-generator/tests/generator/routeFilePrefix/routeTree.snapshot.tspackages/router-generator/tests/generator/single-level/routeTree.snapshot.tspackages/router-generator/tests/generator/types-disabled/routeTree.snapshot.jspackages/router-generator/tests/generator/virtual-config-file-default-export/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-config-file-named-export/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-config-file-tsconfig-paths/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-inside-nested/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-inside-with-escaped-underscore/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-nested-layouts-with-virtual-route/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-pathless-layout-escaped-underscore/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-physical-empty-path-merge/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-physical-layout-and-index/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-physical-no-prefix/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual-with-escaped-underscore/routeTree.snapshot.tspackages/router-generator/tests/generator/virtual/routeTree.snapshot.tspackages/router-generator/tests/utils.test.ts
✅ Files skipped from review due to trivial changes (34)
- packages/router-generator/tests/generator/escaped-custom-tokens/routeTree.snapshot.ts
- packages/router-generator/tests/generator/single-level/routeTree.snapshot.ts
- packages/router-generator/tests/generator/regex-tokens-inline/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-physical-empty-path-merge/routeTree.snapshot.ts
- packages/router-generator/tests/generator/add-extensions-custom/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-with-escaped-underscore/routeTree.snapshot.ts
- packages/router-generator/tests/generator/types-disabled/routeTree.snapshot.js
- packages/router-generator/tests/generator/export-variations/routeTree.snapshot.ts
- packages/router-generator/tests/generator/regex-tokens-json/routeTree.snapshot.ts
- packages/router-generator/tests/generator/custom-scaffolding/routeTree.snapshot.ts
- packages/router-generator/tests/generator/invalid-param-names/routeTree.snapshot.ts
- packages/router-generator/tests/generator/regex-tokens-plus-json/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-physical-layout-and-index/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-nested-layouts-with-virtual-route/routeTree.snapshot.ts
- packages/router-generator/tests/generator/routeFileIgnore/routeTree.snapshot.ts
- packages/router-generator/tests/generator/numbers-in-path/routeTree.snapshot.ts
- packages/router-generator/tests/generator/file-modification/routeTree.snapshot.ts
- packages/router-generator/tests/generator/flat-route-group/routeTree.snapshot.ts
- packages/router-generator/tests/generator/root-export-specifier/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-inside-with-escaped-underscore/routeTree.snapshot.ts
- packages/router-generator/tests/generator/nested-route-groups-with-layouts-before-physical/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-config-file-default-export/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual/routeTree.snapshot.ts
- packages/router-generator/tests/generator/escaped-special-strings/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-config-file-named-export/routeTree.snapshot.ts
- packages/router-generator/tests/generator/dot-escaped/routeTree.snapshot.ts
- packages/router-generator/tests/generator/flat/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-pathless-layout-escaped-underscore/routeTree.snapshot.ts
- packages/router-generator/tests/generator/virtual-config-file-tsconfig-paths/routeTree.snapshot.ts
- packages/router-generator/tests/generator/custom-tokens/routeTree.snapshot.ts
- packages/router-generator/tests/generator/nested/routeTree.snapshot.ts
- packages/router-generator/tests/generator/nested-layouts/routeTree.snapshot.ts
- packages/router-generator/tests/generator/physical-pathless-layout-escaped-underscore/routeTree.snapshot.ts
- packages/router-generator/tests/generator/route-groups/routeTree.snapshot.ts
Follow the AGENTS.md control-statement convention (no one-line if bodies).
| removeUnderscores, | ||
| resetRegex, | ||
| multiSortBy, | ||
| sortRouteNodes, |
There was a problem hiding this comment.
Did you intend to make this a public API? Tests already import it from utils so if extraction is only for unit testing, can we keep it internal and omit this re-export?
There was a problem hiding this comment.
No, that was unintentional — the extraction was only for unit testing, and both the test and generator.ts import it directly from ./utils. Dropped the re-export in a07c93c so it stays internal.
… re-export Tests import sortRouteNodes directly from ../src/utils and the only runtime consumer (generator.ts) imports it from ./utils, so the public re-export from the package entry point served no purpose. Removing it keeps the extraction internal to unit testing as intended.
LadyBluenotes
left a comment
LadyBluenotes
left a comment
There was a problem hiding this comment.
LGTM. Going to wait for @schiller-manuel to give the final sign off :)
|
View your CI Pipeline Execution ↗ for commit 9817319
☁️ Nx Cloud last updated this comment at |
Merging this PR will improve performance by 51.08%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | client-route-tree-scale navigation loop (react) |
74.2 ms | 77.2 ms | -3.87% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
832.9 KB | 306 KB | ×2.7 |
| ⚡ | Memory | mem server error-paths not-found (solid) |
792.7 KB | 414.9 KB | +91.04% |
| ⚡ | Memory | mem server server-fn-churn (solid) |
273.8 KB | 262.8 KB | +4.22% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing 0x80:fix/deterministic-route-tree-sort (09e70aa) with main (58c005f)1
Footnotes
Note on the failing checksAll three red checks are environmental artifacts of this PR coming from a fork, not defects in the change. The code checks —
CodSpeed Performance Analysis flags a single regressed benchmark, |
Fixes #7750.
Problem
buildRouteTreesorts route nodes withmultiSortBy, whose final accessor was(d) => d— comparing whole route-node objects:multiSortBy's comparator ends withreturn ao > bo ? 1 : -1. For two distinct route-node objects, both coerce to"[object Object]", soao === boisfalseandao > boisfalse→ it returns-1for both(a, b)and(b, a). That's an inconsistent comparator, soArray.prototype.sortleaves routes that tie on the earlier keys (same segment-count, same index-token status — e.g./account/beta-featuresvs/account/busy-guard) in an order that depends on the engine's sort implementation, the input order, and the array size. The result:routeTree.gen.tsreorders non-deterministically across machines and Node versions, dirtying committed route trees with semantically-identical churn.Fix
Change the final tiebreaker from
(d) => dto(d) => d.routePath, mirroring the pre-sort earlier in the same file (which already ends with(d) => d.routePath).routePathis unique per node, so this gives a total order and makes generation byte-identical regardless of Node version or route-set composition.A changeset is included (
@tanstack/router-generator, patch).Tests
The
buildRouteTreetiebreaker sort is extracted into an exported puresortRouteNodes(routeNodes, indexTokenSegmentRegex)helper (behavior-identical to the previous inlinemultiSortBycall) so it can be unit-tested directly. New tests inrouter-generator/tests/utils.test.ts:routePath(e.g./account/beta-featuresvs/account/busy-guard,/aboutvs/posts) through every input permutation and asserts identical output, proving the sort is independent of input order, engine sort implementation, and array size. This test fails against the old(d) => dtiebreaker and passes against(d) => d.routePath.routePath, and that root / segment-count / index-token precedence still apply ahead of the tiebreaker.Regenerated snapshots
The deterministic tiebreaker changes the order in which route nodes are emitted, so the 38 committed
routeTree.snapshot.*files undertests/generator/are regenerated. This is expected: the previous snapshots captured the arbitrary order the inconsistent comparator happened to produce on the machine that generated them. The buggy sort was stable run-to-run for a fixed input order and engine, so CI kept passing — the non-determinism only appeared across machines/Node versions (issue #7750).The snapshot diffs are pure emission-order reordering: every route's
id,path, andparentRouteis unchanged, and the route-tree structure (built fromacc.routeTree, not the sorted list) is untouched — only the order of imports and route declarations shifts. In other words, the regenerated snapshots are exactly the semantically-identical churn this fix eliminates going forward, applied once.Summary by CodeRabbit