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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/ui-next/src/globals.tsOops! Something went wrong! :( ESLint: 9.39.4 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'eslint-plugin-react-hooks' imported from /eslint.config.mjs Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
1 participant
Summary by CodeRabbit
New Features
Chores