Skip to content

fix(router-core): review follow-ups for Link stack - #8410

Merged
schiller-manuel merged 6 commits into
schiller-manuel-lazy-current-match-layerfrom
schiller-manuel-link-review-fixes-layer
Sep 13, 2026
Merged

schiller-manuel merged 6 commits into
schiller-manuel-lazy-current-match-layerfrom
schiller-manuel-link-review-fixes-layer

Conversation

@schiller-manuel

@schiller-manuel schiller-manuel commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Follow-ups from review on #8327, #8318, #8319 and #8325, stacked on top of #8406 (stack #8346).

  • Remove the TSR_LINK_PERF opt-in flag from the router-core Link/path benchmarks and the react-link-performance project; rely on standard vitest -t name filtering instead. These benchmarks were never part of the CodSpeed matrix or the client-nav/SSR aggregate projects.
  • Treat 0 and false as provided _splat values when interpolating paths. Only undefined, null, and '' omit a splat segment, matching how $id: 0 already interpolates to /0. Includes a changeset.
  • Use path-derived route ids in the original-param-names matcher fixtures, matching the convention in new-process-route-tree.test.ts.
  • Solid Link: spread cond && { value } for the conditional class/style props (perf(router): optimize Link state props across frameworks #8318 review). Measured on solid-router.full: gzip −7, raw −12, brotli −33. Covered by the existing six-adults-open changeset.
  • Tests: call parseSegments, interpolatePath, getParamNames and hasMissingPathParams directly instead of the interpolateTestPath compatibility shim, and drop the interpolatePath (server: …) axis that no longer read its argument.

Review notes for threads that needed no code change here:

Summary by CodeRabbit

  • Bug Fixes

    • Path interpolation now preserves wildcard values of 0 and false.
    • Wildcard parameters are considered missing only when they are null, undefined, or empty strings.
  • Documentation

    • Updated benchmarking instructions to reflect direct execution through dedicated link-performance commands.
  • Performance

    • Benchmark suites now run without requiring an environment-variable opt-in, simplifying performance testing and reporting.

schiller-manuel and others added 3 commits September 13, 2026 12:39
The router-core Link/path benchmarks and the react-link-performance
project are never part of the CodSpeed matrix or the client-nav/SSR
aggregate projects, so the env-var gate only added friction. Rely on
standard vitest name filtering (-t) to narrow runs instead.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Splat interpolation used a falsy check, so _splat: 0 was omitted like a
missing param while $id: 0 interpolated to '/0'. Only undefined, null,
and '' now omit a splat segment, matching how other path params are
stringified.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The matcher only uses route ids as routesById keys and for the pathless
'_' check, so unrelated ids worked but never occur in a real router.
Align the fixtures with the sibling test file's convention where each
id mirrors its fullPath.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3a80a6b7-43b1-4cb7-99ad-037c3acb6355

📥 Commits

Reviewing files that changed from the base of the PR and between f2fa990 and 0b85fd8.

📒 Files selected for processing (7)
  • packages/router-core/tests/optional-path-params-clean.test.ts
  • packages/router-core/tests/optional-path-params.test.ts
  • packages/router-core/tests/path-interpolation.bench.ts
  • packages/router-core/tests/path.test.ts
  • packages/router-core/tests/route-interpolation.test.ts
  • packages/router-core/tests/routerTestUtils.ts
  • packages/solid-router/src/link.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The change preserves 0 and false splat values during path interpolation. It removes TSR_LINK_PERF=1 gates from benchmark configuration, runners, documentation, and router-core benchmark suites. It also updates interpolation tests and simplifies link prop spreads.

Changes

Path interpolation behavior

Layer / File(s) Summary
Splat value handling
packages/router-core/src/path.ts, packages/router-core/tests/path.test.ts, packages/router-core/tests/route-interpolation.test.ts, .changeset/quiet-zeros-stay.md
Splat checks now treat only null, undefined, and empty strings as missing. Tests cover 0, false, and null.
Interpolation test migration
packages/router-core/tests/path.test.ts, packages/router-core/tests/route-interpolation.test.ts, packages/router-core/tests/optional-path-params*.test.ts, packages/router-core/tests/path-interpolation.bench.ts, packages/router-core/tests/routerTestUtils.ts
Tests now parse segments explicitly, call production interpolation helpers, and remove the shared interpolation test helper.
Route tree test identifiers
packages/router-core/tests/original-param-names.test.ts
Route-tree tests now use canonical route identifiers for sibling, terminal, alias, and optional-language cases.

Benchmark execution defaults

Layer / File(s) Summary
Benchmark configuration and runner
benchmarks/client-nav/link-performance/config.ts, benchmarks/client-nav/link-performance/config.test.ts, benchmarks/client-nav/link-performance/stable-runner.ts
Link-performance benchmarks are always discovered, and the stable runner no longer exits when TSR_LINK_PERF is unset.
Benchmark invocation documentation
benchmarks/client-nav/README.md, benchmarks/ssr/README.md
Documented benchmark commands no longer require the environment-variable prefix.
Router-core benchmark execution
packages/router-core/tests/*bench.ts
Lightweight-location, matching-interpolation, path-decoder, and route-tree-construction benchmarks now run unconditionally.

Link props cleanup

Layer / File(s) Summary
Link prop spread expressions
packages/solid-router/src/link.tsx
Conditional style and class spreads now use short-circuit expressions. The resulting props are unchanged.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0b85f

The interpolation behavior and benchmark defaults are consistently implemented and covered, with no identified issue that should block merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies router-core follow-up fixes for the Link stack and matches the main changes.
Description check ✅ Passed The description gives a detailed and relevant summary of the changes, motivation, tests, and review context. It does not use the template headings or include the checklist and release-impact sections,…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch schiller-manuel-link-review-fixes-layer

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.

@nx-cloud

nx-cloud Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit 0b85fd8

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 10m 8s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 2m 6s View ↗

☁️ Nx Cloud last updated this comment at 2026-09-13 11:38:51 UTC

@schiller-manuel
schiller-manuel added this pull request to stack #8346 September 13, 2026 10:43
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Changeset Version Preview

7 package(s) bumped directly, 22 bumped as dependents.

🟩 Patch bumps

Package Version Reason
@tanstack/history 1.162.3 → 1.162.4 Changeset
@tanstack/react-router 1.170.35 → 1.170.36 Changeset
@tanstack/router-core 1.171.29 → 1.171.30 Changeset
@tanstack/router-devtools-core 1.168.1 → 1.168.2 Changeset
@tanstack/solid-router 1.170.33 → 1.170.34 Changeset
@tanstack/start-server-core 1.169.34 → 1.169.35 Changeset
@tanstack/vue-router 1.170.32 → 1.170.33 Changeset
@tanstack/react-router-devtools 1.167.1 → 1.167.2 Dependent
@tanstack/react-start 1.168.52 → 1.168.53 Dependent
@tanstack/react-start-client 1.168.33 → 1.168.34 Dependent
@tanstack/react-start-rsc 0.1.51 → 0.1.52 Dependent
@tanstack/react-start-server 1.167.40 → 1.167.41 Dependent
@tanstack/router-cli 1.167.35 → 1.167.36 Dependent
@tanstack/router-devtools 1.167.1 → 1.167.2 Dependent
@tanstack/router-generator 1.167.35 → 1.167.36 Dependent
@tanstack/router-plugin 1.168.37 → 1.168.38 Dependent
@tanstack/router-vite-plugin 1.167.37 → 1.167.38 Dependent
@tanstack/solid-router-devtools 1.167.1 → 1.167.2 Dependent
@tanstack/solid-start 1.168.50 → 1.168.51 Dependent
@tanstack/solid-start-client 1.168.32 → 1.168.33 Dependent
@tanstack/solid-start-server 1.167.39 → 1.167.40 Dependent
@tanstack/start-client-core 1.170.29 → 1.170.30 Dependent
@tanstack/start-plugin-core 1.171.42 → 1.171.43 Dependent
@tanstack/start-static-server-functions 1.167.34 → 1.167.35 Dependent
@tanstack/start-storage-context 1.167.31 → 1.167.32 Dependent
@tanstack/vue-router-devtools 1.167.1 → 1.167.2 Dependent
@tanstack/vue-start 1.168.49 → 1.168.50 Dependent
@tanstack/vue-start-client 1.167.35 → 1.167.36 Dependent
@tanstack/vue-start-server 1.167.39 → 1.167.40 Dependent

@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Benchmarks

  • Commit: b2869b2755b8
  • Measured at: 2026-09-13T11:30:00.461Z
  • Baseline source: history:ae6853592904
  • Dashboard: bundle-size history

The following scenarios have bundle-size changes compared with the baseline:

Scenario Current (gzip) Initial (gzip) Raw Brotli Trend
react-router.minimal 84.0 KiB
+35 B
83.9 KiB
+36 B
261.7 KiB
-609 B
73.2 KiB
+14 B
█████▁▁▁▁▁▁▃
react-router.full 87.5 KiB
+39 B
87.4 KiB
+38 B
273.4 KiB
-585 B
76.3 KiB
+73 B
█████▂▂▂▂▂▁▃
solid-router.minimal 33.4 KiB
-15 B
33.3 KiB
-14 B
96.0 KiB
-812 B
30.3 KiB
+9 B
▅█████████▆▁
solid-router.full 38.3 KiB
0 B
38.2 KiB
+2 B
110.6 KiB
-812 B
34.5 KiB
+62 B
▂█████████▁
vue-router.minimal 49.5 KiB
-62 B
49.4 KiB
-59 B
137.1 KiB
-1.2 KiB
44.8 KiB
-79 B
█████▂▂▂▂▂▂▁
vue-router.full 55.1 KiB
-74 B
55.0 KiB
-72 B
155.3 KiB
-1.2 KiB
49.7 KiB
-71 B
█████▂▂▂▂▂▂▁
react-start.minimal 97.0 KiB
+72 B
96.8 KiB
+72 B
304.0 KiB
-629 B
84.1 KiB
+39 B
█████▁▁▁▁▁▁▅
react-start.query-integration 104.3 KiB
+66 B
104.2 KiB
+65 B
330.5 KiB
-633 B
90.5 KiB
+134 B
█████▁▁▁▁▁▁▅
react-start.deferred-hydration 97.7 KiB
+71 B
96.8 KiB
+70 B
305.3 KiB
-629 B
84.8 KiB
+57 B
█████▁▁▁▁▁▁▅
react-start.full 100.1 KiB
+60 B
100.0 KiB
+60 B
313.8 KiB
-573 B
86.8 KiB
+83 B
█████▁▁▁▁▁▁▄
react-start.rsbuild.minimal 100.2 KiB
+106 B
100.1 KiB
+106 B
314.4 KiB
-444 B
86.6 KiB
+89 B
█████▁▁▁▁▁▁▇
react-start.rsbuild.minimal-iife 100.6 KiB
+110 B
100.5 KiB
+110 B
315.4 KiB
-427 B
86.9 KiB
+98 B
█████▁▁▁▁▁▁▇
react-start.rsbuild.full 103.5 KiB
+92 B
103.3 KiB
+92 B
324.5 KiB
-405 B
89.3 KiB
+155 B
█████▁▁▁▁▁▁▆
solid-start.minimal 46.4 KiB
+38 B
46.3 KiB
+40 B
137.1 KiB
-812 B
41.3 KiB
+124 B
▃▂▂▂▂▂▂▂▂▂▁█
solid-start.deferred-hydration 49.4 KiB
+24 B
46.3 KiB
+26 B
144.5 KiB
-813 B
44.0 KiB
+44 B
▁▂▂▂▂▂▂▂▂▂▃█
solid-start.full 51.4 KiB
+49 B
51.3 KiB
+46 B
152.5 KiB
-807 B
45.6 KiB
+52 B
▂▂▂▂▂▂▂▂▂▂▁█
vue-start.minimal 65.6 KiB
-84 B
65.5 KiB
-81 B
188.0 KiB
-1.2 KiB
58.6 KiB
+100 B
█████▃▃▃▃▃▃▁
vue-start.full 69.5 KiB
-55 B
69.4 KiB
-52 B
200.3 KiB
-1.2 KiB
61.9 KiB
+118 B
█████▂▂▂▂▂▂▁

Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better.

@pkg-pr-new

pkg-pr-new Bot commented Sep 13, 2026

Copy link
Copy Markdown
More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/@tanstack/arktype-adapter@8410

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/@tanstack/eslint-plugin-router@8410

@tanstack/eslint-plugin-start

npm i https://pkg.pr.new/@tanstack/eslint-plugin-start@8410

@tanstack/history

npm i https://pkg.pr.new/@tanstack/history@8410

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/@tanstack/nitro-v2-vite-plugin@8410

@tanstack/react-router

npm i https://pkg.pr.new/@tanstack/react-router@8410

@tanstack/react-router-devtools

npm i https://pkg.pr.new/@tanstack/react-router-devtools@8410

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/@tanstack/react-router-ssr-query@8410

@tanstack/react-start

npm i https://pkg.pr.new/@tanstack/react-start@8410

@tanstack/react-start-client

npm i https://pkg.pr.new/@tanstack/react-start-client@8410

@tanstack/react-start-rsc

npm i https://pkg.pr.new/@tanstack/react-start-rsc@8410

@tanstack/react-start-server

npm i https://pkg.pr.new/@tanstack/react-start-server@8410

@tanstack/router-cli

npm i https://pkg.pr.new/@tanstack/router-cli@8410

@tanstack/router-core

npm i https://pkg.pr.new/@tanstack/router-core@8410

@tanstack/router-devtools

npm i https://pkg.pr.new/@tanstack/router-devtools@8410

@tanstack/router-devtools-core

npm i https://pkg.pr.new/@tanstack/router-devtools-core@8410

@tanstack/router-generator

npm i https://pkg.pr.new/@tanstack/router-generator@8410

@tanstack/router-plugin

npm i https://pkg.pr.new/@tanstack/router-plugin@8410

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/@tanstack/router-ssr-query-core@8410

@tanstack/router-utils

npm i https://pkg.pr.new/@tanstack/router-utils@8410

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/@tanstack/router-vite-plugin@8410

@tanstack/solid-router

npm i https://pkg.pr.new/@tanstack/solid-router@8410

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/@tanstack/solid-router-devtools@8410

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/@tanstack/solid-router-ssr-query@8410

@tanstack/solid-start

npm i https://pkg.pr.new/@tanstack/solid-start@8410

@tanstack/solid-start-client

npm i https://pkg.pr.new/@tanstack/solid-start-client@8410

@tanstack/solid-start-server

npm i https://pkg.pr.new/@tanstack/solid-start-server@8410

@tanstack/start-client-core

npm i https://pkg.pr.new/@tanstack/start-client-core@8410

@tanstack/start-fn-stubs

npm i https://pkg.pr.new/@tanstack/start-fn-stubs@8410

@tanstack/start-plugin-core

npm i https://pkg.pr.new/@tanstack/start-plugin-core@8410

@tanstack/start-server-core

npm i https://pkg.pr.new/@tanstack/start-server-core@8410

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/@tanstack/start-static-server-functions@8410

@tanstack/start-storage-context

npm i https://pkg.pr.new/@tanstack/start-storage-context@8410

@tanstack/valibot-adapter

npm i https://pkg.pr.new/@tanstack/valibot-adapter@8410

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/@tanstack/virtual-file-routes@8410

@tanstack/vue-router

npm i https://pkg.pr.new/@tanstack/vue-router@8410

@tanstack/vue-router-devtools

npm i https://pkg.pr.new/@tanstack/vue-router-devtools@8410

@tanstack/vue-router-ssr-query

npm i https://pkg.pr.new/@tanstack/vue-router-ssr-query@8410

@tanstack/vue-start

npm i https://pkg.pr.new/@tanstack/vue-start@8410

@tanstack/vue-start-client

npm i https://pkg.pr.new/@tanstack/vue-start-client@8410

@tanstack/vue-start-server

npm i https://pkg.pr.new/@tanstack/vue-start-server@8410

@tanstack/zod-adapter

npm i https://pkg.pr.new/@tanstack/zod-adapter@8410

commit: 0b85fd8

@codspeed

codspeed Bot commented Sep 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 180 untouched benchmarks


Comparing schiller-manuel-link-review-fixes-layer (0b85fd8) with schiller-manuel-lazy-current-match-layer (1f197ad)

Open in CodSpeed

schiller-manuel and others added 3 commits September 13, 2026 13:11
Spread `cond && { value }` instead of `cond ? { value } : undefined`,
matching the neighbouring `active && STATIC_ACTIVE_ATTRIBUTES` spread.
Falsy operands spread to nothing, so the emitted props are unchanged.

Measured with the bundle-size benchmark on solid-router.full:
gzip 39205 -> 39198 (-7), raw -12, brotli -33.

Covered by the existing `six-adults-open` changeset.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ests

The `interpolatePath (server: $server)` block never read its `server`
argument since interpolation stopped taking a server flag, so it ran every
case twice under a misleading label. The remaining diff is re-indentation;
review with `git show -w`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`interpolateTestPath` only survived as a compatibility layer from the
segment-reuse refactor: it emulated the removed
`(path, params, decoder, usedParams, keys, metadata)` signature by parsing
the template, re-implementing the param-name loop and folding
`hasMissingPathParams` into a metadata flag.

Call the real functions instead. Tests now parse templates with
`parseSegments(false, { fullPath }, 0)` exactly like `buildLocation` does
for templates without a processed route, pass the segments to
`interpolatePath`, and read names and availability from `getParamNames`
and `hasMissingPathParams`. The prepared-route tests compare route-owned
segments against that standalone parse directly, and the interpolation
bench derives its reference values the same way.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@schiller-manuel
schiller-manuel merged commit bc57fa3 into main Sep 13, 2026
26 checks passed
@schiller-manuel
schiller-manuel deleted the schiller-manuel-link-review-fixes-layer branch September 13, 2026 11:55
Sheraff pushed a commit that referenced this pull request Sep 14, 2026
* chore(benchmarks): remove TSR_LINK_PERF opt-in flag

The router-core Link/path benchmarks and the react-link-performance
project are never part of the CodSpeed matrix or the client-nav/SSR
aggregate projects, so the env-var gate only added friction. Rely on
standard vitest name filtering (-t) to narrow runs instead.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* fix(router-core): treat 0 and false as provided _splat values

Splat interpolation used a falsy check, so _splat: 0 was omitted like a
missing param while $id: 0 interpolated to '/0'. Only undefined, null,
and '' now omit a splat segment, matching how other path params are
stringified.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* test(router-core): use path-derived ids in original-param-names fixtures

The matcher only uses route ids as routesById keys and for the pathless
'_' check, so unrelated ids worked but never occur in a real router.
Align the fixtures with the sibling test file's convention where each
id mirrors its fullPath.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* perf(solid-router): shorten conditional Link class and style spreads

Spread `cond && { value }` instead of `cond ? { value } : undefined`,
matching the neighbouring `active && STATIC_ACTIVE_ATTRIBUTES` spread.
Falsy operands spread to nothing, so the emitted props are unchanged.

Measured with the bundle-size benchmark on solid-router.full:
gzip 39205 -> 39198 (-7), raw -12, brotli -33.

Covered by the existing `six-adults-open` changeset.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* test(router-core): drop the unused server axis from interpolatePath tests

The `interpolatePath (server: $server)` block never read its `server`
argument since interpolation stopped taking a server flag, so it ran every
case twice under a misleading label. The remaining diff is re-indentation;
review with `git show -w`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* test(router-core): interpolate paths through the production API in tests

`interpolateTestPath` only survived as a compatibility layer from the
segment-reuse refactor: it emulated the removed
`(path, params, decoder, usedParams, keys, metadata)` signature by parsing
the template, re-implementing the param-name loop and folding
`hasMissingPathParams` into a metadata flag.

Call the real functions instead. Tests now parse templates with
`parseSegments(false, { fullPath }, 0)` exactly like `buildLocation` does
for templates without a processed route, pass the segments to
`interpolatePath`, and read names and availability from `getParamNames`
and `hasMissingPathParams`. The prepared-route tests compare route-owned
segments against that standalone parse directly, and the interpolation
bench derives its reference values the same way.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants