Conversation
📝 WalkthroughWalkthroughRefactored router internals: replaced several Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit caf3888
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
2476-2503: Please lock the hook identity choice in with coverage.This dispatch is now driven by
routeIdsets, while cache identity is stillmatch.id. For transitions where the match instance changes but therouteIddoes not, the expected outcome is nowonStay. A focused regression test here would make that contract explicit and protect future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 2476 - 2503, Add a focused regression test that locks the hook identity choice to routeId (not match.id) by asserting that when a navigation changes the match instance but keeps the same routeId the router invokes onStay (via looseRoutesById[...] .options.onStay) instead of onLeave+onEnter; specifically, create a test that produces two distinct Match objects with the same routeId (altering match.id or params), trigger the transition that sets pendingRouteIds/activeRouteIds as in the dispatch logic using currentMatches and pendingMatches, and verify onStay was called once and onLeave/onEnter were not called, referencing looseRoutesById, currentMatches, pendingMatches, pendingRouteIds and activeRouteIds in the test to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/src/router.ts`:
- Around line 2476-2503: Add a focused regression test that locks the hook
identity choice to routeId (not match.id) by asserting that when a navigation
changes the match instance but keeps the same routeId the router invokes onStay
(via looseRoutesById[...] .options.onStay) instead of onLeave+onEnter;
specifically, create a test that produces two distinct Match objects with the
same routeId (altering match.id or params), trigger the transition that sets
pendingRouteIds/activeRouteIds as in the dispatch logic using currentMatches and
pendingMatches, and verify onStay was called once and onLeave/onEnter were not
called, referencing looseRoutesById, currentMatches, pendingMatches,
pendingRouteIds and activeRouteIds in the test to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18316082-ea7c-4978-8d7d-2c2e3f8f2941
📒 Files selected for processing (1)
packages/router-core/src/router.ts
schiller-manuel
force-pushed
the
chore-perf-bundlesize
branch
from
14ed429 to
e10a3f1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/router.ts (1)
2528-2534:⚠️ Potential issue | 🟠 MajorDon’t classify caught navigation failures via active-match status.
hasErrorMatch()only looks at committed active matches. If something in thetryblock throws before a match is markedstatus: 'error'—for example a lifecycle hook or another non-match failure—this branch falls back to200even though navigation failed. Non-redirect/non-notFound failures should default to500here, with abort/cancel handled explicitly if you want those to stay neutral.🛠️ Suggested fix
const nextStatusCode = redirect ? redirect.status : notFound ? 404 - : this.hasErrorMatch() - ? 500 - : 200 + : 500Also applies to: 2933-2941
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 2528 - 2534, The status selection for nextStatusCode incorrectly uses hasErrorMatch() (which checks only committed active matches) and can return 200 when a non-match failure occurred; change the logic in the nextStatusCode calculation (the redirect / notFound / hasErrorMatch() branch) to treat any non-redirect and non-notFound failure as 500 by default, removing reliance on hasErrorMatch(); explicitly handle abort/cancel cases if they should be neutral (e.g., check the specific navigation failure type before setting a neutral status), and update the code paths around nextStatusCode, redirect, notFound, and hasErrorMatch() so that thrown errors during the try block lead to 500 rather than 200.
🧹 Nitpick comments (1)
packages/router-core/tests/callbacks.test.ts (1)
221-225: Avoid a fixed 10-tick poll in this test.This depends on the pending match showing up within 10
Promise.resolve()turns. If store propagation slips by another tick,pendingFooMatchstays empty and the test becomes timing-dependent. Waiting on an explicit pending-match signal would make this deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/tests/callbacks.test.ts` around lines 221 - 225, The test currently polls pendingMatches via a fixed 10-tick loop (reading router.stores.pendingMatchesSnapshot.state) which is timing-dependent; replace that loop with an explicit await on the snapshot update by subscribing to router.stores.pendingMatchesSnapshot and returning a Promise that resolves when the snapshot's state becomes non-empty (unsubscribe immediately after resolving). Target the pendingMatches variable and router.stores.pendingMatchesSnapshot.subscribe/unsubscribe logic so the test deterministically waits for the pending match instead of relying on Promise.resolve() ticks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-core/src/router.ts`:
- Around line 2528-2534: The status selection for nextStatusCode incorrectly
uses hasErrorMatch() (which checks only committed active matches) and can return
200 when a non-match failure occurred; change the logic in the nextStatusCode
calculation (the redirect / notFound / hasErrorMatch() branch) to treat any
non-redirect and non-notFound failure as 500 by default, removing reliance on
hasErrorMatch(); explicitly handle abort/cancel cases if they should be neutral
(e.g., check the specific navigation failure type before setting a neutral
status), and update the code paths around nextStatusCode, redirect, notFound,
and hasErrorMatch() so that thrown errors during the try block lead to 500
rather than 200.
---
Nitpick comments:
In `@packages/router-core/tests/callbacks.test.ts`:
- Around line 221-225: The test currently polls pendingMatches via a fixed
10-tick loop (reading router.stores.pendingMatchesSnapshot.state) which is
timing-dependent; replace that loop with an explicit await on the snapshot
update by subscribing to router.stores.pendingMatchesSnapshot and returning a
Promise that resolves when the snapshot's state becomes non-empty (unsubscribe
immediately after resolving). Target the pendingMatches variable and
router.stores.pendingMatchesSnapshot.subscribe/unsubscribe logic so the test
deterministically waits for the pending match instead of relying on
Promise.resolve() ticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2aa5b93-44fd-4bb4-89ac-763d4bca7652
📒 Files selected for processing (2)
packages/router-core/src/router.tspackages/router-core/tests/callbacks.test.ts
| } else { | ||
| exitingMatches = null | ||
| } |
There was a problem hiding this comment.
else not necessary, exitingMatches is already null
| } else { | |
| exitingMatches = null | |
| } | |
| } |
| const nextPendingRouteIds = | ||
| pendingRouteIds as Set<string> | null | ||
| const nextActiveRouteIds = | ||
| activeRouteIds as Set<string> | null |
There was a problem hiding this comment.
This doesn't seem necessary, as casts to the same type those variables already have
2 participants
Summary by CodeRabbit
Refactor
New Features
Tests