test: add regression coverage for existing router behavior#7812
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds six regression test suites covering React Router pending and redirect flows, invalidation transition errors, route head execution after ChangesReact router navigation regressions
Invalidation transition regression
Router core load regressions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 09f0bf9
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
Bundle Size Benchmarks
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/react-router/tests/issue-7457-redirect-chain-first-load.test.tsx (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
anyfor the search updater parameter.
(prev: any) => ({ ...prev, flag: undefined })bypasses type checking on the previous search shape. Sinceflagis typed as{ flag?: boolean }viavalidateSearch, this can be typed explicitly (or left for inference) instead ofany.♻️ Proposed fix
- search: (prev: any) => ({ ...prev, flag: undefined }), + search: (prev) => ({ ...prev, flag: undefined }),As per coding guidelines, "Use TypeScript strict mode with extensive type safety" for
**/*.{ts,tsx}files.🤖 Prompt for 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. In `@packages/react-router/tests/issue-7457-redirect-chain-first-load.test.tsx` around lines 41 - 45, Remove the explicit any annotation from the search updater in the redirect call, allowing the parameter to be inferred or explicitly typed as the validated search shape containing optional flag. Preserve the existing spread and flag-clearing behavior.Source: Coding guidelines
packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve strict typing across these regression tests.
Both tests bypass router API contracts with
any, reducing their ability to detect type and API regressions.
packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts#L19-L21: type the head callback context.packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts#L60-L67: type thebuildLocationoptions.packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts#L74-L75: type thecommitLocationpayload.🤖 Prompt for 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. In `@packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts` around lines 19 - 21, Replace the any-based types in the regression tests with the appropriate router API types: type the failingHead callback context in packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts (lines 19-21), the buildLocation options in packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts (lines 60-67), and the commitLocation payload there (lines 74-75). Preserve the existing test behavior while ensuring these callbacks and method arguments are checked against their actual contracts.Source: Coding guidelines
🤖 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-core/tests/issue-4684-head-on-beforeload-error.test.ts`:
- Around line 76-84: Extend the client-lane assertions in the test around
rootMatch and failingMatch to verify rootMatch.links contains the retained
stylesheet link, matching the existing server-side coverage while preserving the
current metadata and error-status assertions.
In
`@packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts`:
- Around line 82-84: Replace the two waitForMacrotask calls in the aborted-load
test with a direct await of the aborted load’s rejection or an equivalent router
settlement signal, then assert the intermediate state only after that operation
completes; remove the timing-dependent event-loop sleeps while preserving the
existing assertions.
---
Nitpick comments:
In `@packages/react-router/tests/issue-7457-redirect-chain-first-load.test.tsx`:
- Around line 41-45: Remove the explicit any annotation from the search updater
in the redirect call, allowing the parameter to be inferred or explicitly typed
as the validated search shape containing optional flag. Preserve the existing
spread and flag-clearing behavior.
In `@packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts`:
- Around line 19-21: Replace the any-based types in the regression tests with
the appropriate router API types: type the failingHead callback context in
packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts (lines
19-21), the buildLocation options in
packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts
(lines 60-67), and the commitLocation payload there (lines 74-75). Preserve the
existing test behavior while ensuring these callbacks and method arguments are
checked against their actual contracts.
🪄 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: 1ae21b96-7907-4832-a3d3-14140a1b1342
📒 Files selected for processing (6)
packages/react-router/tests/issue-2182-root-pending.test.tsxpackages/react-router/tests/issue-7367-pending-min-redirect.test.tsxpackages/react-router/tests/issue-7457-redirect-chain-first-load.test.tsxpackages/react-router/tests/issue-7638-invalidate-transition-error.test.tsxpackages/router-core/tests/issue-4684-head-on-beforeload-error.test.tspackages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts
Comment on lines
+76
to
+84
| const rootMatch = router.state.matches.find( | ||
| (match) => match.routeId === rootRoute.id, | ||
| ) | ||
| const failingMatch = router.state.matches.find( | ||
| (match) => match.routeId === failingRoute.id, | ||
| ) | ||
| expect(rootMatch?.meta).toEqual([{ title: 'Root title' }]) | ||
| expect(failingMatch?.status).toBe('error') | ||
| expect(failingMatch?.meta).toEqual([{ title: 'Error title' }]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced test and nearby related tests.
git ls-files 'packages/router-core/tests/*issue-4684*' 'packages/router-core/tests/*4684*' | sed 's#^`#FILE` #'
# Show the relevant portion of the client test with line numbers.
test_file='packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts'
if [ -f "$test_file" ]; then
echo '--- CLIENT TEST ---'
cat -n "$test_file" | sed -n '1,220p'
fi
# Search for the server-side links assertion in related tests.
echo '--- SEARCH links/meta in issue-4684 tests ---'
rg -n "links|meta|Root title|Error title|beforeload" packages/router-core/tests -SRepository: TanStack/router
Length of output: 8928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file='packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts'
echo '--- EXISTS ---'
ls -l "$test_file"
echo '--- CLIENT TEST SNIPPET ---'
cat -n "$test_file" | sed -n '1,220p'
echo '--- SEARCH IN TEST DIR ---'
rg -n "links|meta|Root title|Error title|beforeload" packages/router-core/tests -SRepository: TanStack/router
Length of output: 8976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file='packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts'
echo '--- CLIENT TEST ---'
cat -n "$test_file" | sed -n '1,220p'
echo '--- RELATED ASSERTIONS ---'
rg -n "rootMatch\.links|failingMatch\.links|Root title|Error title|beforeload" packages/router-core/tests -SRepository: TanStack/router
Length of output: 4676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the code path that assembles head output into match state.
rg -n "links.*meta|meta.*links|head\\(|routeId.*links|statusCode|head content|links:" packages/router-core/src packages/router-core/tests -S
# Map likely files first.
for f in \
packages/router-core/src/index.ts \
packages/router-core/src/router.ts \
packages/router-core/src/load.ts \
packages/router-core/src/route.ts \
packages/router-core/src/utils.ts
do
if [ -f "$f" ]; then
echo "--- OUTLINE $f ---"
ast-grep outline "$f" --view expanded || true
fi
doneRepository: TanStack/router
Length of output: 29701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- router.ts around match initialization ---'
sed -n '1628,1660p' packages/router-core/src/router.ts | cat -n
echo '--- load-matches.ts around head/link fields ---'
sed -n '540,620p' packages/router-core/src/load-matches.ts | cat -n
echo '--- ssr-client.ts around head execution ---'
sed -n '150,210p' packages/router-core/src/ssr/ssr-client.ts | cat -nRepository: TanStack/router
Length of output: 6756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- find assignments to match.links / head output application ---'
rg -n "match\.links|links =|headScripts|executeHead\(|meta = headFnContent|styles =" packages/router-core/src -S
echo '--- load-matches.ts around where head results are consumed ---'
sed -n '620,760p' packages/router-core/src/load-matches.ts | cat -n
echo '--- maybe client-specific execution branches ---'
rg -n "isServer|isClient|ssr-client|executeHead" packages/router-core/src/load-matches.ts packages/router-core/src/ssr/ssr-client.ts -SRepository: TanStack/router
Length of output: 7680
Assert rootMatch.links in the client lane too. The client path also hydrates head().links onto the match, so this test should cover the retained stylesheet like the server case.
🤖 Prompt for 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.
In `@packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts`
around lines 76 - 84, Extend the client-lane assertions in the test around
rootMatch and failingMatch to verify rootMatch.links contains the retained
stylesheet link, matching the existing server-side coverage while preserving the
current metadata and error-status assertions.
Comment on lines
+82
to
+84
| // Give the aborted rejection time to (incorrectly) commit as an error. | ||
| await waitForMacrotask() | ||
| await waitForMacrotask() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
printf '\n== outline ==\n'
ast-grep outline "$file" --view expanded || true
printf '\n== relevant lines ==\n'
nl -ba "$file" | sed -n '1,220p'Repository: TanStack/router
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
echo "== file exists =="
test -f "$file" && echo yes
echo "== lines 1-220 =="
nl -ba "$file" | sed -n '1,220p'Repository: TanStack/router
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
nl -ba "$file" | sed -n '1,220p'Repository: TanStack/router
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
if [ -f "$file" ]; then
echo "FOUND"
sed -n '1,220p' "$file" | nl -ba | sed -n '1,220p'
else
echo "MISSING"
fiRepository: TanStack/router
Length of output: 199
🏁 Script executed:
echo hiRepository: TanStack/router
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
echo "== file slice =="
sed -n '1,220p' "$file" | cat -nRepository: TanStack/router
Length of output: 5229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts"
sed -n '1,220p' "$file" | cat -nRepository: TanStack/router
Length of output: 5212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "waitForMacrotask|router.*settle|settlement|waitFor.*route|waitFor.*router|commitLocation" packages/router-core/tests packages/router-core/src -g '!**/dist/**'Repository: TanStack/router
Length of output: 1555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/src/router.ts"
sed -n '2140,2660p' "$file" | cat -n | sed -n '1,260p'Repository: TanStack/router
Length of output: 9851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router-core/src/router.ts"
sed -n '2580,2650p' "$file" | cat -nRepository: TanStack/router
Length of output: 2636
Replace the macrotask sleeps with a direct await
Two setTimeout(0) turns are timing-dependent here; await the aborted load’s rejection or another router settlement signal instead of relying on event-loop timing before asserting the intermediate state.
🤖 Prompt for 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.
In
`@packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts`
around lines 82 - 84, Replace the two waitForMacrotask calls in the aborted-load
test with a direct await of the aborted load’s rejection or an equivalent router
settlement signal, then assert the intermediate state only after that operation
completes; remove the timing-dependent event-loop sleeps while preserving the
existing assertions.
Merging this PR will improve performance by 12.86%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem server error-paths unmatched (react) |
848.5 KB | 477.6 KB | +77.68% |
| ⚡ | Memory | mem server error-paths redirect (solid) |
918.1 KB | 604 KB | +52.01% |
| ⚡ | Simulation | ssr streaming deferred (react) |
73.1 ms | 71 ms | +3.03% |
| 👁 | Simulation | ssr global-mw document (vue) |
221.1 ms | 233.7 ms | -5.38% |
| 👁 | Simulation | ssr head (solid) |
190.6 ms | 196.9 ms | -3.22% |
| 👁 | Simulation | client-loaders navigation loop (react) |
52.8 ms | 55.5 ms | -4.94% |
| 👁 | Memory | mem server error-paths not-found (react) |
266.3 KB | 276.5 KB | -3.69% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/already-fixed-router-issues (09f0bf9) with main (a41f265)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
headafterbeforeLoaderrors, and canonical search normalization abortsrouter.invalidate()inside React transitionsIssues covered
Refs #2182
Refs #2905
Refs #4684
Refs #6371
Refs #7367
Refs #7457
Refs #7638
The direct #7120 regression already exists on
main, so this PR does not duplicate it.Test plan
pnpm test:eslintpnpm test:typespnpm test:unitSummary by CodeRabbit