fix(router): ensure layout and page loaders are merged correctly#67
Merged
Conversation
📝 WalkthroughWalkthroughThis PR fixes wrapLayout's keyed k:page slot handling in the router package so that composed loader props (from root layouts, nested layouts, and pages) merge correctly across mount, hydration, and SSR paths without losing page-only keys. Extensive tests for composeLoaders behavior are added, and ChangesRouter Loader Composition Fix
Router version bump propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/router/src/index.ts (2)
370-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary
?? {}fallback in spread.Same as line 335 —
...(props ?? {})is equivalent to...propssince spreadingundefinedis a no-op. Flagged byunicorn(no-useless-fallback-in-spread).♻️ Proposed fix
const slotProps = - merged && typeof merged === "object" ? { ...merged, ...(props ?? {}) } : props; + merged && typeof merged === "object" ? { ...merged, ...props } : props;🤖 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/src/index.ts` at line 370, The spread fallback in the merge logic is unnecessary because spreading undefined is already a no-op. Update the object merge expression in the relevant router merge path (the one using merged and props) to spread props directly instead of using the nullish-coalescing fallback, keeping the same behavior while satisfying unicorn(no-useless-fallback-in-spread).Source: Linters/SAST tools
335-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary
?? {}fallback in spread.Spreading
undefinedin an object literal is a no-op, so...(partial ?? {})is equivalent to...partial. The linter flagged this viaunicorn(no-useless-fallback-in-spread).♻️ Proposed fix
- const shellChild = ((partial?: Record<string, unknown>) => - rawKeyed({ ...props, ...(partial ?? {}) })) as unknown as Island<any, any>; + const shellChild = ((partial?: Record<string, unknown>) => + rawKeyed({ ...props, ...partial })) as unknown as Island<any, any>;🤖 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/src/index.ts` at line 335, The spread in the router Island creation path uses an unnecessary fallback: in the expression inside the `rawKeyed`/`Island` construction, replace `...(partial ?? {})` with a direct spread of `partial` since `undefined` is already a no-op in object spreads. Keep the change localized to the `rawKeyed` call so the `Island<any, any>` cast and surrounding props merging stay unchanged.Source: Linters/SAST tools
packages/router/src/index.test.ts (2)
1397-1397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
inputparameter in test island.The
Pageisland destructuresinputbut doesn't use it — this test validatesrunLoaderoutput, not rendered HTML. Eslint flagged this (no-unused-vars).♻️ Proposed fix
- const Page = ilha.render(({ input }: any) => `<p>ok</p>`); + const Page = ilha.render(() => `<p>ok</p>`);🤖 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/src/index.test.ts` at line 1397, The test island in the Page render callback destructures an input parameter that is never used, triggering no-unused-vars. Update the ilha.render callback in index.test.ts to remove the unused destructured input from the Page definition, keeping the test focused on runLoader output rather than rendered HTML. Use the Page and ilha.render symbols to locate the callback and ensure the render signature only includes parameters that are actually referenced.Source: Linters/SAST tools
1552-1553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
.input<...>()calls — copy-paste artifacts.Multiple test islands call
.input<...>()two or three times redundantly. Each call returns a freshIlhaBuilder, so only the last one takes effect. Since all calls use the same type parameter, the duplicates are no-ops but make the tests harder to read.♻️ Example fix for lines 1552-1553
const Page = ilha - .input<{ root: boolean; nested: boolean; page: boolean }>() - .input<{ root: boolean; nested: boolean; page: boolean }>() .input<{ root: boolean; nested: boolean; page: boolean }>() .render(({ input }) => `<p>${String(input.page)}</p>`);Also applies to: 2755-2756, 2783-2784, 2791-2792
🤖 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/src/index.test.ts` around lines 1552 - 1553, The test islands are calling ilha.input<...>() multiple times in a row, but each call creates a new IlhaBuilder so only the last one is used. Remove the redundant duplicate .input<...>() calls in the affected test setups (for example where Page is defined) and keep a single input declaration per island so the intent is clear and the tests remain unchanged.
🤖 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/src/index.test.ts`:
- Line 1397: The test island in the Page render callback destructures an input
parameter that is never used, triggering no-unused-vars. Update the ilha.render
callback in index.test.ts to remove the unused destructured input from the Page
definition, keeping the test focused on runLoader output rather than rendered
HTML. Use the Page and ilha.render symbols to locate the callback and ensure the
render signature only includes parameters that are actually referenced.
- Around line 1552-1553: The test islands are calling ilha.input<...>() multiple
times in a row, but each call creates a new IlhaBuilder so only the last one is
used. Remove the redundant duplicate .input<...>() calls in the affected test
setups (for example where Page is defined) and keep a single input declaration
per island so the intent is clear and the tests remain unchanged.
In `@packages/router/src/index.ts`:
- Line 370: The spread fallback in the merge logic is unnecessary because
spreading undefined is already a no-op. Update the object merge expression in
the relevant router merge path (the one using merged and props) to spread props
directly instead of using the nullish-coalescing fallback, keeping the same
behavior while satisfying unicorn(no-useless-fallback-in-spread).
- Line 335: The spread in the router Island creation path uses an unnecessary
fallback: in the expression inside the `rawKeyed`/`Island` construction, replace
`...(partial ?? {})` with a direct spread of `partial` since `undefined` is
already a no-op in object spreads. Keep the change localized to the `rawKeyed`
call so the `Island<any, any>` cast and surrounding props merging stay
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f47f42c5-61d0-4136-9e08-b49fef9a4e49
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/website/package.jsonpackages/router/package.jsonpackages/router/src/codegen.test.tspackages/router/src/index.test.tspackages/router/src/index.tstemplates/elysia/package.jsontemplates/hono/package.jsontemplates/nitro/package.jsontemplates/vite/package.json
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 by CodeRabbit
New Features
Bug Fixes
Chores