fix: serial head execution #6093
Conversation
WalkthroughRefactored head metadata execution to defer until after all loaders settle using Promise.allSettled, enabling all loaders to complete regardless of failures before head processing, with improved early handling for redirect and notFound errors. Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant LoadMatches
participant Loaders
participant HeadExecution
participant ErrorHandler
Client->>LoadMatches: initiate match loading
LoadMatches->>Loaders: execute all loaders in parallel
par Loader Execution
Loaders->>Loaders: loader 1 (success/failure)
Loaders->>Loaders: loader 2 (success/failure)
Loaders->>Loaders: loader N (success/failure)
end
rect rgb(100, 200, 100, 0.1)
note over LoadMatches: AllSettled Phase
Loaders-->>LoadMatches: all promises settle (regardless of failures)
end
rect rgb(200, 100, 100, 0.1)
note over ErrorHandler: Early Redirect Check
LoadMatches->>ErrorHandler: check for redirect errors
alt Redirect found
ErrorHandler-->>Client: throw immediately (skip heads)
end
end
rect rgb(100, 150, 200, 0.1)
note over HeadExecution: Deferred Head Execution
LoadMatches->>HeadExecution: iterate matches, executeHead serially
par Head Processing
HeadExecution->>HeadExecution: head 1 (try/catch)
HeadExecution->>HeadExecution: head 2 (try/catch)
HeadExecution->>HeadExecution: head N (try/catch)
end
HeadExecution-->>LoadMatches: apply head metadata to matches
end
LoadMatches->>ErrorHandler: check for notFound errors
alt NotFound found
ErrorHandler-->>Client: throw notFound error
else All processed
ErrorHandler-->>Client: return settled matches
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 8m 57s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 44s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-13 13:32:18 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (2)
917-933: Consider limiting head execution to successfully loaded matches.The loop iterates over all
inner.matches, but loaders only execute for matches up toinner.firstBadMatchIndex(line 896-898). Matches beyond this index may have incomplete state (no loader data), which could cause unexpected behavior in head functions that depend onloaderData.Consider aligning the head execution range with the loader execution range:
- for (const match of inner.matches) { + const headMax = inner.firstBadMatchIndex ?? inner.matches.length + for (let i = 0; i < headMax; i++) { + const match = inner.matches[i]! const { id: matchId, routeId } = matchAlternatively, if executing heads for all matches is intentional (even failed ones), please add a comment clarifying this design choice.
916-916: Address TODO: decide on head execution failure behavior.The current implementation continues executing other head functions when one fails, which is resilient but may mask issues in development. Consider documenting the chosen behavior or removing the TODO if the current approach is final.
Would you like me to open an issue to track this decision, or is the current continue-on-failure behavior the intended final design?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
🧬 Code graph analysis (1)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/index.ts (2)
isRedirect(368-368)isNotFound(374-374)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/src/load-matches.ts (3)
900-905: LGTM -Promise.allSettledusage is appropriate here.Using
Promise.allSettledensures all loaders complete before processing results, which aligns with the goal of deferring head execution until all loaders settle. The failure extraction logic is correct.
907-912: Redirect early-exit logic is correct.Throwing on the first redirect before head execution is the right approach - there's no need to compute head metadata when redirecting.
935-940: NotFound handling after head execution is appropriate.The asymmetry with redirect handling (redirect throws before head, notFound throws after head) is intentional and makes sense - a notFound page can still benefit from head metadata, while a redirect doesn't need any rendering.
| // After head execution, check for notFound errors and throw the first one | ||
| for (const err of failures) { | ||
| if (isNotFound(err)) { | ||
| throw err | ||
| } | ||
| } |
There was a problem hiding this comment.
I find it a little curious that we call executeHead for children of a match that would have thrown a notFound
If we have /a/b/c and /b throws a notFound it its loader, should we really still execute the head of /c? It feels "leaky"
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.