fix: App Router discovery for nested parallel-slot sub-routes#458
fix: App Router discovery for nested parallel-slot sub-routes#458JaredStowell wants to merge 2 commits intocloudflare:mainfrom
Conversation
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4070516a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review |
There was a problem hiding this comment.
Review
The net code change here is a small defensive fix: before this PR, discoverSlotSubRoutes would create synthetic routes with pagePath: null (set from childrenDefault) when the parent directory had no default.tsx. This would produce routes that fail at render time with a "Page has no default export" error. The fix correctly gates synthetic sub-route creation on the existence of childrenDefault.
The change is correct and the test is well-structured. Two items worth noting:
PR description is misleading
The PR description says:
This updates slot discovery to keep slots that only contain descendant
pagefiles
This described the first commit (a407051), but the second commit (a1d4c02) reverted that change entirely. The actual net effect of this PR is the opposite of what the description says: it adds a guard to skip synthetic sub-route creation when there is no childrenDefault, rather than expanding slot discovery. The discoverParallelSlots filter at line 650 is unchanged.
Please update the PR title/description to match the actual change — something like "fix: skip synthetic slot sub-routes when parent has no default.tsx fallback".
The original problem from the PR description remains unsolved
The scenario described in the PR body:
app/inbox/page.tsx
app/inbox/@modal/profile/page.tsx
(where @modal has no root page.tsx or default.tsx) still does not produce a /inbox/profile route after this PR. The discoverParallelSlots filter at line 650 still excludes @modal from parallelSlots, so discoverSlotSubRoutes never sees its nested pages. This PR only fixes the downstream symptom (creating broken synthetic routes with null pagePath), not the discovery gap.
If the intent is to also support the "slot with only nested descendants" case, that would need a separate follow-up to relax the discoverParallelSlots filter — which was the original first commit that got reverted. That's fine as a separate PR, but worth tracking.
|
|
||
| // Find the default.tsx for the children slot at the parent directory | ||
| const childrenDefault = findFile(parentPageDir, "default", matcher); | ||
| if (subPathMap.size === 0 || !childrenDefault) continue; |
There was a problem hiding this comment.
This is the right fix — previously, if childrenDefault was null, synthetic routes were created with pagePath: null, which would 500 at render time. Combining the two guards into one continue is clean.
Minor: the original code had the subPathMap.size === 0 check first (cheap) before the findFile call (does filesystem I/O). The new code always calls findFile even when subPathMap is empty. Consider preserving the short-circuit:
| if (subPathMap.size === 0 || !childrenDefault) continue; | |
| if (subPathMap.size === 0) continue; | |
| // Find the default.tsx for the children slot at the parent directory | |
| const childrenDefault = findFile(parentPageDir, "default", matcher); | |
| if (!childrenDefault) continue; |
This avoids a pointless findFile call when there are no sub-paths. Not a big deal since fs.existsSync is fast, but it's a free optimization and arguably clearer about the two independent exit conditions.
| }); | ||
| }); | ||
|
|
||
| it("does not create nested @slot sub-routes without a children default fallback", async () => { |
There was a problem hiding this comment.
Good test. The name accurately describes what it asserts.
Note that this test gives @modal a default.tsx (line 484), so the slot passes the discoverParallelSlots filter. What's being tested is purely the childrenDefault guard in discoverSlotSubRoutes.
It might be worth adding a companion test for the other case: @modal with no root page.tsx/default.tsx and only @modal/profile/page.tsx — which tests that discoverParallelSlots itself excludes such a slot. That would document the current behavior and guard against future regressions if someone relaxes the filter.
|
Review submitted. Here's a summary of the feedback: The code change is correct — it fixes a real bug where synthetic slot sub-routes were created with Key feedback points:
|
Fix App Router route discovery for nested parallel-slot pages when the slot root has no own
page,default, or intercepting route.Before this change, a structure like:
failed to produce the synthetic
/inbox/profileroute because@modalwas filtered out too early during slot discovery.This updates slot discovery to keep slots that only contain descendant
pagefiles, so the existing synthetic sub-route generation can create and match routes like/inbox/profile.Changes
pagefiles as enough for a parallel slot to participate in routingpage,default, or intercepting routes/inbox/profilematchAppRoute('/inbox/profile', routes)Testing
pnpm test tests/routing.test.tspnpm test tests/app-router.test.tspnpm run fmtpnpm run typecheck