-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5540
Versions: Review Loop 1.0.0 · Component Audit Rubric 1.10
Verdict: approve
#5540 fix(docsite): stop registering the preview theme's icons globally — it fails hydration on every other page by imdreamrunner (bucket: internal collaborator; repository write access)
cdafa9b4e7bfcede9547c3ffffa43a19f249eec4
LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10
A visitor can receive server HTML containing neutral-theme Lucide semantic icons while a fresh client resolves the same names to built-ins, so React reports hydration error #418 and regenerates the page. On the parent production build I reproduced one #418 on / and /docs/getting-started in both light and dark after the component route had entered the build/server process; /components/Banner remained clean. I did not reproduce #418 on /blog or /templates in that local production build, so the exact four-route prevalence from the body remains unverified even though the shared failure mechanism is reproduced.
VERDICT: clear — the user-facing hydration failure and its cause are established; only the exact route prevalence varied locally
A component preview asks its nearest theme for semantic icons. Removing the docsite's process-wide override lets the neutral preview keep Lucide icons while the surrounding Astryx page keeps built-ins, so server and client resolve the same icon for each theme boundary.
SOLUTION (1 decision · 4 runtime lines removed; the rest is regression coverage and explanation)
- Remove the component-route module's
registerIcons(neutralTheme.icons)side effect and guard the docsite against reintroducing any direct call. [the fix]
The decision maps directly to the stated problem. It is the true fix at the offending ownership boundary: the core contract already resolves Icon through useThemeName() → getIcon(name, themeName), and the removed global write was the only production registerIcons() call in the docsite.
VERDICT: clear
OWNER: <Theme> owns theme identity; Icon/getIcon owns semantic icon resolution.
TIER 1: theming — reuses Theme, registerTheme, useThemeName, and getIcon.
TIER 2: semantic icons — neutral's icon map is selected by the nearest theme name.
SEAMS: root Astryx theme, nested neutral preview themes, built-theme imports, SSR/client boundary, Next route cache and client transitions.
BEHAVIOR UNIT: existing pure lookup (getIcon) plus existing Theme provider lifecycle — no new unit needed.
| Seam driven in Chromium | Result at PR head |
|---|---|
| Root Astryx page | registry icons are built-ins |
| Nested neutral previews | 24/24 registry icons are Lucide on /components/Banner
|
| Component → home → components → Banner | no hydration warning; Astryx icons stay built-in and neutral icons stay Lucide |
/themes with Astryx, neutral, stone, gothic, matcha, y2k, butter scopes |
coexists without a global icon override |
| Light ↔ dark mode toggle | ownership counts unchanged; no hydration warning |
VERDICT: clear
Visitors no longer pay a full React client regeneration where the mismatch occurred. Component-detail chrome intentionally returns from Lucide to the root Astryx theme's built-ins; the nested preview islands remain on neutral's Lucide set. Builders and published packages are unaffected.
VERDICT: clear
No public or internal API is added, removed, or retyped. The diff deletes a call to the existing public registerIcons() fallback; no surface ossifies.
VERDICT: clear
The change follows the documented Icon guidance: theme overrides belong in defineTheme({icons}) and resolve through the nearest <Theme>, while registerIcons() is process-wide app bootstrap only. At the head, /components/Banner renders 36/36 root astryx registry icons as built-ins and 24/24 nested neutral registry icons as Lucide in both color modes; /themes contains all seven theme scopes without cross-theme pollution. A repository search found no production registerIcons() calls in the docsite after this diff.
VERDICT: clear
- API: no public surface changes.
- Visual: yes, intentionally — component-page chrome switches from Lucide to built-in icons; nested preview icons do not change.
- Theme: no target, token, override, or theme object changes; the existing per-theme contract is restored.
- Behavior: hydration failure is removed. No state/default/interaction path is added; light/dark and client route transitions remain functional.
VERDICT: clear
Effects: 0 added, moved, or removed. No listeners, observers, layout reads, dependencies, or shipped bundle work are added; the runtime change deletes one process-global registry merge at module evaluation. The added guard is test-only and the full 26-file docsite Vitest project completed in 1.95s (404 tests).
VERDICT: clear
- Parent
/components/Banner:before/before-final__component-cold.png - Head
/components/Banner:after/after-verified__component-cold.png - Head
/themes:after/after-verified__themes-full-load.png - Head route transitions:
after/after-verified__component-to-home-client-transition.png,after/after-verified__home-to-components-client-transition.png,after/after-verified__components-to-banner-client-transition.png
The 1280×5269 component-page pair differs in 545 pixels (0.0081%), all within y=13–1024; DOM classification identifies the intended change as the 36 root-theme registry icons moving Lucide → built-in while all 24 neutral-preview registry icons remain Lucide. No layout, geometry, text, or theme-token code changed.
VERDICT: clear — the only observed delta matches the stated icon-ownership correction
No role, ARIA, focus, keyboard, or user-facing string changed. In real Chromium after the fix, all 36 root-theme and all 24 neutral-preview registry-icon wrappers on /components/Banner retain aria-hidden="true"; none gains or loses role="img" or an accessible label. CI did not run pr-a11y/pr-rtl because this path does not trigger those jobs.
VERDICT: clear
| Slot | Verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL EVIDENCE | clear |
| A11Y & I18N | clear |
GOAL: met — parent production Chromium produced React #418; the head produced zero hydration errors across 10 fresh route/mode loads and both transition sequences. All five non-component SSR responses were byte-identical before and after warming /components/Banner.
DISPOSITION: the long implementation comment at ComponentPreviewTheme.tsx:19-29 is a non-blocking cleanup already requested in the author's own inline review; accept for this internal-author review and keep the existing note as the action.
ADVICE: proven remedy — delete ComponentPreviewTheme.tsx:19-29; the regression test and repository docs preserve the reason.
WORST OUTCOME: none found at the reviewed head. The strongest change is “component-detail chrome intentionally returns from Lucide to the root Astryx theme's built-ins” → note-level, documented, and scoped.
PRIOR REVIEW: imdreamrunner left one self-review inline asking to remove the implementation comment; there is no other reviewer conclusion to reconcile.
JUDGEMENT NEEDED: none — defect fix against the existing theme-scoped icon contract.
Verdict: APPROVE with one non-blocking existing nit.
Thanks, this fixes the hydration failure and keeps neutral icons scoped to preview themes. Agree with your existing note to remove the implementation comment.
None — the author's existing inline already identifies the only nit.
- On the parent production build,
/blogand/templatesdid not reproduce #418; build-worker ordering makes the route prevalence nondeterministic, while/and/docs/getting-startedreproduced in both modes. - Local production emits two expected 404s per page for Vercel Analytics and Speed Insights endpoints; neither is a React/hydration warning and both occur before and after.
- The existing caller docs still contain one pre-existing contradictory phrase saying semantic names use the “global icon registry”; this PR does not touch docs and its runtime behavior follows the newer theme-scoped guidance.