ui-next: page registration#1159
Conversation
WalkthroughThis PR introduces a page registration system that replaces the static Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui-next/src/registry/page.tsx (1)
18-21: ⚡ Quick winReconsider the props type signature.
The wrapper component is typed as accepting
React.PropsWithChildren<P>, but:
- It's called without children in
app.tsx(line 31:<Comp />)- Spreading
{...props}toPageCompincludes achildrenprop thatPageCompdoesn't expectThe wrapper generates its own children (
<PageComp />), so it shouldn't accept children from its caller.♻️ Proposed fix: Remove children from props type
- default(props: React.PropsWithChildren<P>) { + default(props: P) { return ( <LayoutComp> <PageComp {...props} /> </LayoutComp> ); },Alternatively, if pages should support receiving children, destructure them:
- default(props: React.PropsWithChildren<P>) { + default({ children, ...pageProps }: React.PropsWithChildren<P>) { return ( <LayoutComp> - <PageComp {...props} /> + <PageComp {...pageProps} /> </LayoutComp> ); },🤖 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/ui-next/src/registry/page.tsx` around lines 18 - 21, The wrapper component currently types its parameter as React.PropsWithChildren<P>, causing an unexpected children prop to be forwarded to PageComp; change the component signature from default(props: React.PropsWithChildren<P>) to default(props: P) so the wrapper does not accept or forward children, and keep using <LayoutComp><PageComp {...props} /></LayoutComp> (or alternatively explicitly destructure and drop children: const { children, ...rest } = props and pass rest to PageComp) to ensure PageComp only receives the props it expects.
🤖 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.
Inline comments:
In `@packages/ui-next/index.ts`:
- Around line 52-55: The generated plugin entries use raw interpolation and
place name before the plugin spread so plugin${i}.name can overwrite it and
unescaped addon names can break JS; change the construction to ensure the
addon-derived name is safely JSON-escaped (e.g. use JSON.stringify(addonName)
when inserting the literal) and make the name property come after the plugin
spread so it cannot be overridden (e.g. produce an entry like `{ ...plugin${i},
name: <escaped-addonName> }` or use Object.assign({}, plugin${i}, { name:
<escaped-addonName> }) ); apply the same fix to the other occurrence around
lines 128-131.
---
Nitpick comments:
In `@packages/ui-next/src/registry/page.tsx`:
- Around line 18-21: The wrapper component currently types its parameter as
React.PropsWithChildren<P>, causing an unexpected children prop to be forwarded
to PageComp; change the component signature from default(props:
React.PropsWithChildren<P>) to default(props: P) so the wrapper does not accept
or forward children, and keep using <LayoutComp><PageComp {...props}
/></LayoutComp> (or alternatively explicitly destructure and drop children:
const { children, ...rest } = props and pass rest to PageComp) to ensure
PageComp only receives the props it expects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6818855e-0d95-4d73-a525-6a5c46c9398f
📒 Files selected for processing (13)
packages/ui-next/index.tspackages/ui-next/src/app.tsxpackages/ui-next/src/components/layout.tsxpackages/ui-next/src/globals.tspackages/ui-next/src/main.tsxpackages/ui-next/src/pages/homepage.tsxpackages/ui-next/src/pages/index.tspackages/ui-next/src/pages/problem_main.tsxpackages/ui-next/src/registry/index.tspackages/ui-next/src/registry/page.tsxpackages/ui-next/src/registry/plugin.tspackages/ui-next/src/registry/store.tspackages/ui-next/src/registry/types.ts
💤 Files with no reviewable changes (1)
- packages/ui-next/src/globals.ts
| const exports = `export default [${entries.map((e, i) => { | ||
| const addonName = path.basename(path.resolve(e, '..', '..')); | ||
| return `{ name: '${addonName}', ...plugin${i} }`; | ||
| }).join(', ')}];`; |
There was a problem hiding this comment.
Make generated plugin name deterministic and safely escaped.
On Lines 52-55 and Lines 128-131, { name: '...', ...plugin${i} } lets plugin${i}.name overwrite the addon-derived name, and raw string interpolation can break generated JS for quoted names.
💡 Suggested patch
-const exports = `export default [${entries.map((e, i) => {
- const addonName = path.basename(path.resolve(e, '..', '..'));
- return `{ name: '${addonName}', ...plugin${i} }`;
-}).join(', ')}];`;
+const exports = `export default [${entries.map((e, i) => {
+ const addonName = JSON.stringify(path.basename(path.resolve(e, '..', '..')));
+ return `{ ...plugin${i}, name: ${addonName} }`;
+}).join(', ')}];`;-`window.__hydroPlugins = [${entries.map((e, i) => {
- const addonName = path.basename(path.resolve(e, '..', '..'));
- return `{ name: '${addonName}', ...plugin${i} }`;
-}).join(', ')}];`,
+`window.__hydroPlugins = [${entries.map((e, i) => {
+ const addonName = JSON.stringify(path.basename(path.resolve(e, '..', '..')));
+ return `{ ...plugin${i}, name: ${addonName} }`;
+}).join(', ')}];`,Also applies to: 128-131
🤖 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/ui-next/index.ts` around lines 52 - 55, The generated plugin entries
use raw interpolation and place name before the plugin spread so plugin${i}.name
can overwrite it and unescaped addon names can break JS; change the construction
to ensure the addon-derived name is safely JSON-escaped (e.g. use
JSON.stringify(addonName) when inserting the literal) and make the name property
come after the plugin spread so it cannot be overridden (e.g. produce an entry
like `{ ...plugin${i}, name: <escaped-addonName> }` or use Object.assign({},
plugin${i}, { name: <escaped-addonName> }) ); apply the same fix to the other
occurrence around lines 128-131.
There was a problem hiding this comment.
This expected behaviour.
There was a problem hiding this comment.
@renbaoshuo, understood — the placement of name before the plugin spread is intentional so that individual plugins can override the addon-derived name. I'll note this for future reviews.
( `∪｡∪｡) ~~
✏️ Learnings added
Learnt from: renbaoshuo
Repo: hydro-dev/Hydro PR: 1159
File: packages/ui-next/index.ts:52-55
Timestamp: 2026-05-10T15:27:48.125Z
Learning: In `packages/ui-next/index.ts`, the virtual plugin module generates entries as `{ name: '<addonName>', ...plugin${i} }` (name first, spread second) on purpose: this allows individual plugin modules to override the addon-derived `name` property by exporting their own `name`. The same pattern applies to the `buildPlugins()` esbuild bundle (`window.__hydroPlugins`). Do not flag this ordering as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary by CodeRabbit