test: add more regression coverage for existing router behavior - #7822
Merged
Conversation
Contributor
|
View your CI Pipeline Execution ↗ for commit 0fd4483
☁️ 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. |
Contributor
📝 WalkthroughWalkthroughAdds regression tests covering SSR and hydration boundaries, router lifecycle ordering, context propagation, loading and preload states, and transition invalidation. The React e2e fixture adds a hydration counter used to verify direct nested not-found hydration. ChangesRouter regression coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/router-core/tests/issue-5106-hydrated-notfound-boundary.test.ts (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unsafe global cast.
Lines 19 and 23 bypass strict typing with
(global as any). Install and clean up the test-onlywindowproperty throughglobalThisAPIs instead.Proposed fix
describe('hydrated child-owned notFound boundary coverage', () => { let mockWindow: { $_TSR?: TsrSsrGlobal } + let originalWindowDescriptor: PropertyDescriptor | undefined beforeEach(() => { mockWindow = {} - ;(global as any).window = mockWindow + originalWindowDescriptor = Object.getOwnPropertyDescriptor( + globalThis, + 'window', + ) + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: mockWindow, + }) }) afterEach(() => { - delete (global as any).window + if (originalWindowDescriptor) { + Object.defineProperty(globalThis, 'window', originalWindowDescriptor) + } else { + Reflect.deleteProperty(globalThis, 'window') + } vi.restoreAllMocks() })🤖 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-5106-hydrated-notfound-boundary.test.ts` around lines 19 - 23, Update the test setup and cleanup around mockWindow to use globalThis APIs for installing and removing the temporary window property, eliminating both `(global as any)` casts while preserving the existing beforeEach/afterEach behavior.Source: Coding guidelines
packages/react-router/tests/issue-5778-router-provider-context-preload.test.tsx (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a valid initial router context value.
null!bypasses the declared{ foo: string }contract while passingnullat runtime. Initializefoowith a string so this test remains type-safe and cannot mask fallback-context behavior.Proposed fix
- context: { foo: null! }, + context: { foo: 'foo' },As per coding guidelines,
**/*.{ts,tsx}must “Use TypeScript strict mode with extensive type safety.”🤖 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-5778-router-provider-context-preload.test.tsx` at line 67, Replace the null-forced value in the router provider test’s initial context with a valid string for the declared `{ foo: string }` shape. Update the `context` initializer while preserving the test’s intended preload and fallback-context behavior.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.
Nitpick comments:
In
`@packages/react-router/tests/issue-5778-router-provider-context-preload.test.tsx`:
- Line 67: Replace the null-forced value in the router provider test’s initial
context with a valid string for the declared `{ foo: string }` shape. Update the
`context` initializer while preserving the test’s intended preload and
fallback-context behavior.
In `@packages/router-core/tests/issue-5106-hydrated-notfound-boundary.test.ts`:
- Around line 19-23: Update the test setup and cleanup around mockWindow to use
globalThis APIs for installing and removing the temporary window property,
eliminating both `(global as any)` casts while preserving the existing
beforeEach/afterEach behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3739812-d970-43ac-bf77-b85d004feea8
📒 Files selected for processing (11)
e2e/react-start/basic/src/routes/posts.tsxe2e/react-start/basic/tests/not-found.spec.tspackages/react-router/tests/errorComponent.test.tsxpackages/react-router/tests/issue-2905-root-beforeload-pending.test.tsxpackages/react-router/tests/issue-5778-router-provider-context-preload.test.tsxpackages/react-router/tests/issue-7638-invalidate-transition-error.test.tsxpackages/react-router/tests/loaders.test.tsxpackages/router-core/tests/issue-3293-on-enter-after-loader.test.tspackages/router-core/tests/issue-4078-loader-notfound-root-boundary.test.tspackages/router-core/tests/issue-4696-parent-context-search-normalization.test.tspackages/router-core/tests/issue-5106-hydrated-notfound-boundary.test.ts
Merging this PR will improve performance by 6.87%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (solid) |
423.3 KB | 747.5 KB | -43.37% |
| ❌ | Memory | mem server error-paths not-found (vue) |
380.7 KB | 612.7 KB | -37.86% |
| ❌ | Simulation | client-preload interaction loop (react) |
57.3 ms | 60.6 ms | -5.46% |
| ❌ | Memory | mem server server-fn-churn (react) |
271.4 KB | 281.9 KB | -3.71% |
| ⚡ | Memory | mem server peak-large-page (react) |
2,080.9 KB | 955.2 KB | ×2.2 |
| ⚡ | Memory | mem server error-paths unmatched (react) |
641.2 KB | 416 KB | +54.14% |
| ⚡ | Memory | mem server serialization-payload (react) |
5.3 MB | 3.4 MB | +53.24% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
304.3 KB | 294.8 KB | +3.2% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/passing-router-issue-tests (0fd4483) with main (11a6a0d)
This was referenced Jul 23, 2026
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
headis not executed whenbeforeLoadthrows an Error #4684, Context is missing from parent loader when child's validateSearch fails #4696, Throwing 404 in loader causes parent route to "freeze" #5106, and [Stackblitz reproduction] Authenticated route, beforeLoad has expiredcontexton preload (intent) #5778Not included
mainvia test: add regression coverage for existing router behavior #7812, so this branch has no test delta to addAbortError: signal is aborted without reasonis bubbling up when using loader with basepath #4517: no branch-only test that passes onmainwas found; the related rewritten AbortError tests primarily cover fix(router-core): don't resolve aborted match to success without loaderData #7673 and fail againstmainTest plan
@tanstack/router-core:test:types@tanstack/react-router:test:typestest:eslintfor Router Core and React RouterSummary by CodeRabbit
Bug Fixes
Tests