ui-next: page registration#1159
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui-next/index.ts (1)
152-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the entries count in the log message.
The
entriesvariable is aRecord<string, string>object, which does not have a.lengthproperty. This will logundefinedfor the entry count.📊 Proposed fix
- logger.success('Plugins built in %dms (%d entries, %s)', Date.now() - start, entries.length, size(totalSize)); + logger.success('Plugins built in %dms (%d entries, %s)', Date.now() - start, Object.keys(entries).length, size(totalSize));🤖 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` at line 152, The log uses entries.length but entries is a Record<string,string>, so replace the length access with a proper key count (e.g., compute Object.keys(entries).length or Object.entries(entries).length) before calling logger.success in the same location where entries and totalSize are used; update the logger.success call to pass that computed count (refer to the variables entries, totalSize and the logger.success invocation).
🤖 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 194-198: The non-dev renderer is using context.handler.user while
the dev renderer and the RendererContext interface expose the user as
context.UserContext; update the non-dev renderer to pass the same property by
replacing usages of context.handler.user with context.UserContext (and keep
UiContext as context.handler.UiContext) so both renderers consistently use
context.UserContext for the UserContext argument.
---
Outside diff comments:
In `@packages/ui-next/index.ts`:
- Line 152: The log uses entries.length but entries is a Record<string,string>,
so replace the length access with a proper key count (e.g., compute
Object.keys(entries).length or Object.entries(entries).length) before calling
logger.success in the same location where entries and totalSize are used; update
the logger.success call to pass that computed count (refer to the variables
entries, totalSize and the logger.success invocation).
🪄 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: 06b4b7a9-1bf2-4186-a4ad-6175bd864f80
📒 Files selected for processing (8)
build/prepare.jspackages/ui-next-plugin-sample/package.jsonpackages/ui-next-plugin-sample/ui/before.tsxpackages/ui-next-plugin-sample/ui/index.tspackages/ui-next-plugin-sample/ui/index.tsxpackages/ui-next/index.tspackages/ui-next/src/context/page-data.tsxpackages/ui-next/vite.config.ts
💤 Files with no reviewable changes (3)
- packages/ui-next-plugin-sample/ui/before.tsx
- packages/ui-next-plugin-sample/ui/index.ts
- build/prepare.js
✅ Files skipped from review due to trivial changes (2)
- packages/ui-next-plugin-sample/package.json
- packages/ui-next-plugin-sample/ui/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/hooks/use-build-url.ts`:
- Around line 14-19: The getPrefix callback in use-build-url assumes domain.host
and window are always present and may produce "/d/undefined"; fix it by guarding
access: treat domain.host as optional (use empty array or fallback) and ensure
window is available (typeof window !== 'undefined') before reading
window.location.host; also normalize the id at the top (if id is falsy use
domainId, and if still falsy return '' immediately) so you never return
`/d/undefined`; update the logic inside getPrefix (references: getPrefix,
domainId, domain.host, window.location.host) to use these guards and safe
fallbacks.
🪄 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: 61a76aaa-79aa-4cab-b163-fbc69c3f2306
📒 Files selected for processing (5)
packages/ui-next/src/api.tspackages/ui-next/src/components/link.tsxpackages/ui-next/src/context/page-data.tsxpackages/ui-next/src/hooks/use-build-url.tspackages/ui-next/src/hooks/use-url.ts
💤 Files with no reviewable changes (1)
- packages/ui-next/src/hooks/use-url.ts
| const getPrefix = useCallback((id?: string) => { | ||
| id ||= domainId; | ||
| const domainHost = Array.isArray(domain.host) ? domain.host : [domain.host]; | ||
| const currentHost = window.location.host; | ||
| return id === (domainHost && domainHost.includes(currentHost) ? domainId : 'system') ? '' : `/d/${id}`; | ||
| }, [domainId, domain]); |
There was a problem hiding this comment.
Guard prefix resolution for SSR and partial UiContext.
Line 16 assumes domain.host exists and Line 17 assumes window exists. This can crash URL building in SSR/injected contexts, and can generate malformed /d/undefined when ids are missing.
Proposed fix
const getPrefix = useCallback((id?: string) => {
- id ||= domainId;
- const domainHost = Array.isArray(domain.host) ? domain.host : [domain.host];
- const currentHost = window.location.host;
- return id === (domainHost && domainHost.includes(currentHost) ? domainId : 'system') ? '' : `/d/${id}`;
- }, [domainId, domain]);
+ const resolvedId = id ?? domainId ?? 'system';
+ const domainHost = Array.isArray(domain?.host)
+ ? domain.host
+ : (domain?.host ? [domain.host] : []);
+ const currentHost = typeof window !== 'undefined' ? window.location.host : '';
+ const defaultId = currentHost && domainHost.includes(currentHost) ? (domainId ?? 'system') : 'system';
+ return resolvedId === defaultId ? '' : `/d/${resolvedId}`;
+ }, [domainId, domain]);🤖 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/hooks/use-build-url.ts` around lines 14 - 19, The
getPrefix callback in use-build-url assumes domain.host and window are always
present and may produce "/d/undefined"; fix it by guarding access: treat
domain.host as optional (use empty array or fallback) and ensure window is
available (typeof window !== 'undefined') before reading window.location.host;
also normalize the id at the top (if id is falsy use domainId, and if still
falsy return '' immediately) so you never return `/d/undefined`; update the
logic inside getPrefix (references: getPrefix, domainId, domain.host,
window.location.host) to use these guards and safe fallbacks.
There was a problem hiding this comment.
This is a CSR app.
There was a problem hiding this comment.
@renbaoshuo, understood — since this is a pure CSR app, window is always available and that guard is unnecessary. I'll withdraw that part of the suggestion.
The narrower remaining concern is whether domainId or domain.host could ever be absent in UiContext (e.g. on a route that doesn't inject full context), which could produce /d/undefined. If UiContext is guaranteed to always include valid domainId and domain.host values before this hook is called, the original code is fine as-is.
✏️ Learnings added
Learnt from: renbaoshuo
Repo: hydro-dev/Hydro PR: 1159
File: packages/ui-next/src/hooks/use-build-url.ts:14-19
Timestamp: 2026-05-17T02:28:42.594Z
Learning: `packages/ui-next` is a pure Client-Side Rendering (CSR) app. Guards like `typeof window !== 'undefined'` are unnecessary as `window` is always available. SSR-related concerns do not apply to this package.
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.
2 participants
Summary by CodeRabbit
New Features
Chores