ui-next: layout refactor & dynamic template#1182
Conversation
|
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 introduces template-aware layout routing in the Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
framework/framework/base.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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
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/src/app.tsx (1)
13-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStore reads inside
useMemobecome stale when the store updates.The
useMemoat lines 13-33 reads fromstore.getDefault()multiple times, but its dependencies are[name, template, isError]. When a slot is registered late (e.g., via lazy plugin loading):
useSyncExternalStoretriggers a re-render because the store version changeduseMemoreturns the cached[slotName, entry]because its deps are unchangedentryremains stale (e.g., stillundefined), causing "Page not found" even though the page is now registeredConsider restructuring to separate deterministic slot-name computation from store reads:
Suggested approach
- const [slotName, entry] = useMemo(() => { - if (isError) { - return ['page:error', store.getDefault('page:error')] as const; - } - const templateName = typeof template === 'string' ? template.replace(/\.html$/, '') : null; - if (templateName) { - const templateSlot = `page:${templateName}` as `page:${string}`; - const templateEntry = store.getDefault(templateSlot); - if (templateEntry) { - if (import.meta.env.DEV) { - console.log(`[ui-next] using template "${templateName}" for page "${name}"`); - } - return [templateSlot, templateEntry] as const; - } - } - const slot = `page:${name}` as `page:${string}`; - if (import.meta.env.DEV) { - console.log(`[ui-next] using page slot "${slot}"`); - } - return [slot, store.getDefault(slot)] as const; - }, [name, template, isError]); - - const [subscribe, getSnapshot] = useMemo(() => [ - (cb: () => void) => store.subscribe(slotName, cb), - () => store.getVersion(slotName), - ], [slotName]); - - useSyncExternalStore(subscribe, getSnapshot); + // Step 1: Compute potential slot names (deterministic) + const candidateSlots = useMemo(() => { + if (isError) return ['page:error'] as const; + const templateName = typeof template === 'string' ? template.replace(/\.html$/, '') : null; + if (templateName) return [`page:${templateName}`, `page:${name}`] as const; + return [`page:${name}`] as const; + }, [name, template, isError]); + + // Step 2: Subscribe to all candidate slots + const [subscribe, getSnapshot] = useMemo(() => [ + (cb: () => void) => { + const unsubs = candidateSlots.map((s) => store.subscribe(s, cb)); + return () => unsubs.forEach((u) => u()); + }, + () => candidateSlots.map((s) => store.getVersion(s)).join(','), + ], [candidateSlots]); + + useSyncExternalStore(subscribe, getSnapshot); + + // Step 3: Resolve entry fresh on each render (after subscription ensures re-render on changes) + const [slotName, entry] = (() => { + for (const slot of candidateSlots) { + const e = store.getDefault(slot); + if (e) return [slot, e] as const; + } + return [candidateSlots[candidateSlots.length - 1], undefined] as const; + })();🤖 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/app.tsx` around lines 13 - 40, The useMemo hook is caching store reads from store.getDefault() based only on [name, template, isError] dependencies, which causes stale entry values when the store is updated asynchronously (e.g., via lazy plugin loading). Refactor by separating the deterministic slot-name computation from the store reads: keep the slotName calculation in useMemo with its current dependencies, but move the store.getDefault(slotName) calls outside of useMemo so they re-execute whenever slotName changes and when useSyncExternalStore detects store updates. This ensures entry is always in sync with the current store state for the computed slot.
🧹 Nitpick comments (1)
packages/ui-next/src/components/layout.tsx (1)
3-5: ⚡ Quick winAvoid unconditional logging in the default layout render path.
This runs on every render and will add noisy console output in production.
♻️ Proposed change
const Layout = defineSlot('layout:default', ({ children }: React.PropsWithChildren) => { - console.log('[ui-next] using default layout'); return <>{children}</>; });🤖 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/components/layout.tsx` around lines 3 - 5, The Layout component defined through defineSlot with the key 'layout:default' contains an unconditional console.log statement that executes on every render, creating unnecessary console noise in production. Remove the console.log statement entirely from the component's render path, or if the logging is needed for debugging purposes, wrap it with a development environment check (such as checking process.env.NODE_ENV) so it only logs during development and not in production builds.
🤖 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`:
- Line 194: The injected template values at line 194 and line 228 in the file
are inconsistent with how navigation normalizes templates in framework/base.ts.
Both instances currently inject context.handler.response.template verbatim, but
the navigation header contract strips the .html extension from the template
name. Normalize both injections by applying the same .html stripping logic that
the navigation code uses, ensuring PageData.template values remain consistent
between first render and client navigation to prevent resolving different page
layouts for the same route.
---
Outside diff comments:
In `@packages/ui-next/src/app.tsx`:
- Around line 13-40: The useMemo hook is caching store reads from
store.getDefault() based only on [name, template, isError] dependencies, which
causes stale entry values when the store is updated asynchronously (e.g., via
lazy plugin loading). Refactor by separating the deterministic slot-name
computation from the store reads: keep the slotName calculation in useMemo with
its current dependencies, but move the store.getDefault(slotName) calls outside
of useMemo so they re-execute whenever slotName changes and when
useSyncExternalStore detects store updates. This ensures entry is always in sync
with the current store state for the computed slot.
---
Nitpick comments:
In `@packages/ui-next/src/components/layout.tsx`:
- Around line 3-5: The Layout component defined through defineSlot with the key
'layout:default' contains an unconditional console.log statement that executes
on every render, creating unnecessary console noise in production. Remove the
console.log statement entirely from the component's render path, or if the
logging is needed for debugging purposes, wrap it with a development environment
check (such as checking process.env.NODE_ENV) so it only logs during development
and not in production builds.
🪄 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: 7d89c169-6a3f-4a1b-b955-aded36273d22
📒 Files selected for processing (15)
framework/framework/base.tspackages/ui-next/index.tspackages/ui-next/src/app.tsxpackages/ui-next/src/components/layout.tsxpackages/ui-next/src/context/page-data.tsxpackages/ui-next/src/context/router.tsxpackages/ui-next/src/globals.tspackages/ui-next/src/pages/homepage.tsxpackages/ui-next/src/registry/index.tspackages/ui-next/src/registry/layout.tspackages/ui-next/src/registry/page.tsxpackages/ui-next/src/registry/plugin.tspackages/ui-next/src/registry/slot.tsxpackages/ui-next/src/registry/store.tspackages/ui-next/src/registry/types.ts
💤 Files with no reviewable changes (1)
- packages/ui-next/src/pages/homepage.tsx
| const serialized = JSON.stringify({ | ||
| HYDRO_INJECTED: true, | ||
| name: context.handler.context._matchedRouteName, | ||
| template: context.handler.response.template || '', |
There was a problem hiding this comment.
Normalize injected template to match navigation header contract.
Line 194 and Line 228 inject context.handler.response.template verbatim, but navigation reads x-hydro-template from framework/framework/base.ts Line 87 where .html is stripped. This creates inconsistent PageData.template values between first render and client navigation, which can resolve different page:* entries/layouts for the same route.
Proposed fix
- template: context.handler.response.template || '',
+ template: (context.handler.response.template || '').replace(/\.html$/, ''),
...
- template: context.handler.response.template || '',
+ template: (context.handler.response.template || '').replace(/\.html$/, ''),Also applies to: 228-228
🤖 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 194, The injected template values at line
194 and line 228 in the file are inconsistent with how navigation normalizes
templates in framework/base.ts. Both instances currently inject
context.handler.response.template verbatim, but the navigation header contract
strips the .html extension from the template name. Normalize both injections by
applying the same .html stripping logic that the navigation code uses, ensuring
PageData.template values remain consistent between first render and client
navigation to prevent resolving different page layouts for the same route.
2 participants
Summary by CodeRabbit