fix(router-core): guard against evicted matches during in-flight preloads#7762
fix(router-core): guard against evicted matches during in-flight preloads#7762thribhuvan003 wants to merge 2 commits into
Conversation
|
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)
📝 WalkthroughWalkthroughThis PR guards several ChangesPreload eviction guard fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant preloadRoute
participant loadMatches
participant routerStore
preloadRoute->>loadMatches: start in-flight preload
loadMatches->>routerStore: getMatch(matchId)
routerStore-->>loadMatches: undefined after eviction
loadMatches->>loadMatches: fall back to inner.matches[index] or return early
loadMatches-->>preloadRoute: settle promise without _nonReactive crash
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
373-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse curly braces for one-line
ifbodies.Lines 375 and 399 use
if (!match) returnwithout braces. The project's coding guidelines require curly braces for all control statements.As per coding guidelines: "Always use curly braces for
if,else, loops, and similar control statements. Never write one-line bodies likeif (foo) x = 1."♻️ Proposed fix
const then = () => { const match = inner.router.getMatch(matchId) // in case the match was evicted while awaiting the previous beforeLoad - if (!match) return + if (!match) { + return + }const executeBeforeLoad = ( inner: InnerLoadContext, matchId: string, index: number, route: AnyRoute, ): void | Promise<void> => { const match = inner.router.getMatch(matchId) // in case the match was evicted while `preBeforeLoadSetup` was awaiting the // previous beforeLoad - if (!match) return + if (!match) { + return + }Also applies to: 396-399
🤖 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/src/load-matches.ts` around lines 373 - 375, The control-flow guards in load-matches.ts use one-line if bodies without braces, which violates the project style guide. Update the match eviction checks around the getMatch lookup in the load-matches flow to use curly braces for the if statements, including the related guard near the later match handling path referenced by the same logic. Keep the behavior unchanged while rewriting those conditional early returns in the functions that handle match loading.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/router-core/src/load-matches.ts`:
- Around line 373-375: The control-flow guards in load-matches.ts use one-line
if bodies without braces, which violates the project style guide. Update the
match eviction checks around the getMatch lookup in the load-matches flow to use
curly braces for the if statements, including the related guard near the later
match handling path referenced by the same logic. Keep the behavior unchanged
while rewriting those conditional early returns in the functions that handle
match loading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ae4f2a0-ca21-44e2-8e6f-edbddf77bfa1
📒 Files selected for processing (3)
.changeset/fix-preload-evicted-match-guards.mdpackages/router-core/src/load-matches.tspackages/router-core/tests/load.test.ts
when a preload's cached match gets evicted while its async work is still in flight (cache cleared, gc'd, or the user actually navigates mid-preload), the load pipeline re-looks the match up after each
awaitwith a non-null assertion, so the next._nonReactiveread throwsTypeError: Cannot read properties of undefined (reading '_nonReactive')as an unhandled rejection.this guards those re-lookups the same way
executeHeadandrunLoader's catch already do. the post-load cleanup falls back to the matched object instead of bailing —_nonReactivekeeps its identity across store updates, so resolving the promises on it means a second load awaiting the first one'sloaderPromisestill settles instead of hanging forever (that hang is real: the second regression test times out without the fix).added two deterministic tests (evict mid-loader, and a second preload joining the in-flight load). both fail without the fix, and router-core (1186) + react-router (912) suites pass with it.
fixes #7759
Summary by CodeRabbit
Bug Fixes
Tests