fix(ui): make appearance editor more cohesive#2897
Conversation
Fake-User
commented
Apr 27, 2026
Fake-User
commented
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAppearance and theme UI files are refactored to use Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/app/packages/theme/components/ThemeEditorBasic.tsx (1)
264-300: 🧹 Nitpick | 🔵 TrivialGuard
resizeObserverin cleanup against early-unmount.
resizeObserveris typed as non-nullable but only assigned insideonMount. If the component is disposed beforesetupWebGLcompletes (e.g., a thrown WebGL2-not-supported error before line 278),onCleanupwill dereference an undefined and throw, masking the original error.♻️ Suggested change
- let resizeObserver: ResizeObserver; + let resizeObserver: ResizeObserver | undefined;onCleanup(() => { - resizeObserver.disconnect(); + resizeObserver?.disconnect(); document.removeEventListener('pointermove', handlePointerMove); document.removeEventListener('pointerup', handlePointerUp); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/theme/components/ThemeEditorBasic.tsx` around lines 264 - 300, The cleanup currently assumes resizeObserver is always assigned in onMount and calls resizeObserver.disconnect(), which can throw if setupWebGL fails and the component unmounts early; make resizeObserver nullable (or initialize to null) and in onCleanup guard the disconnect call (e.g., if (resizeObserver) resizeObserver.disconnect() or use optional chaining) so disconnect is only invoked when it was actually created; keep references like resizeObserver, onMount, setupWebGL and onCleanup when making this change.js/app/packages/theme/components/ThemeList.tsx (1)
70-78: 🧹 Nitpick | 🔵 Trivial
/* temp remove when long */hack — capture as a follow-up.The bottom border + negative margin only works for the short-list case; once the theme list grows (or scrolls), this will produce a stray underline above the scroll thumb. Worth tracking before this lands so it doesn't become permanent.
Want me to open a follow-up issue describing the long-list case (and proposing replacing the negative-margin compensation with a sticky header that owns its border), or sketch a stickier header implementation here?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/theme/components/ThemeList.tsx` around lines 70 - 78, In ThemeList.tsx locate the inline style block on the outer grid container that adds "border-bottom: 1px solid ...; margin-bottom: -1px;" (the short-list hack) and remove those two properties; instead give the header row (or the element rendered as the list's top bar inside ThemeList) an explicit border-bottom and set it to position: sticky with top: 0 and appropriate z-index so the header owns the dividing line for long/scrolling lists. If you don't implement the sticky header now, replace the hack with a clear TODO/follow-up comment referencing ThemeList and the long-list case so we don't accidentally keep the negative-margin workaround.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/settings/Appearance.tsx`:
- Around line 27-30: The outer container div and the inner grid use different
gap sources (an inline gap: '20px' vs Tailwind gap-5 on the nested grid), so
pick one source-of-truth and apply it consistently: either replace the outer
inline gap with the Tailwind utility gap-5 (preferred) on the container div that
wraps Panel/ThemeList/ThemeEditorAdvanced, or change the nested grid to use the
same inline style; update the class on the container div or the nested div
accordingly so both grids use the same spacing unit (keep existing responsive
classes like @[700px]:grid-cols-2 intact).
In `@js/app/packages/theme/components/ThemeEditorBasic.tsx`:
- Around line 423-424: The input handlers currently wrap the event in an
unnecessary arrow (e.g. onInput={(e) => { handleChromaChange(e); }}); replace
these with direct function references (onInput={handleChromaChange}) for both
handleChromaChange and handleSaturationChange (the same pattern used by
handleContrastChange), ensuring the handler signatures remain compatible with
the input event type.
In `@js/app/packages/theme/components/ThemeTools.tsx`:
- Line 40: Remove the stray leading space in the grid-template-columns value in
ThemeTools.tsx: update the inline style where 'grid-template-columns' is set
(the template using columnCount()) so the template string starts with
"min-content" rather than " min-content" (i.e., adjust the template literal
around columnCount() in the component to trim the leading whitespace).
- Around line 72-79: The contentEditable name field in ThemeTools (the element
using ref themeName and contentEditable={true}) removes the native focus
indicator via "outline: 'none'"; restore accessible focus by removing that style
and either (a) delete the outline: 'none' entry so the native focus ring
remains, or (b) replace it with a class that applies an explicit :focus-visible
ring in CSS and add an aria-label (e.g., aria-label="Theme name") to the element
to ensure keyboard users can detect focus before typing/Enter; update the inline
style block or className on the element and add the aria-label accordingly.
---
Outside diff comments:
In `@js/app/packages/theme/components/ThemeEditorBasic.tsx`:
- Around line 264-300: The cleanup currently assumes resizeObserver is always
assigned in onMount and calls resizeObserver.disconnect(), which can throw if
setupWebGL fails and the component unmounts early; make resizeObserver nullable
(or initialize to null) and in onCleanup guard the disconnect call (e.g., if
(resizeObserver) resizeObserver.disconnect() or use optional chaining) so
disconnect is only invoked when it was actually created; keep references like
resizeObserver, onMount, setupWebGL and onCleanup when making this change.
In `@js/app/packages/theme/components/ThemeList.tsx`:
- Around line 70-78: In ThemeList.tsx locate the inline style block on the outer
grid container that adds "border-bottom: 1px solid ...; margin-bottom: -1px;"
(the short-list hack) and remove those two properties; instead give the header
row (or the element rendered as the list's top bar inside ThemeList) an explicit
border-bottom and set it to position: sticky with top: 0 and appropriate z-index
so the header owns the dividing line for long/scrolling lists. If you don't
implement the sticky header now, replace the hack with a clear TODO/follow-up
comment referencing ThemeList and the long-list case so we don't accidentally
keep the negative-margin workaround.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a509da49-bcac-47bd-9564-8697db19c48d
📒 Files selected for processing (6)
js/app/packages/app/component/settings/Appearance.tsxjs/app/packages/theme/components/ThemeEditorAdvanced.tsxjs/app/packages/theme/components/ThemeEditorBasic.tsxjs/app/packages/theme/components/ThemeList.tsxjs/app/packages/theme/components/ThemeTools.tsxjs/app/packages/ui/components/Panel.tsx
| <div class="@container grid grid-cols-1 @[700px]:grid-cols-2 gap-5 overflow-hidden min-h-0"> | ||
| <Panel><ThemeList /></Panel> | ||
| <Panel><ThemeEditorAdvanced /></Panel> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Are @containerand arbitrary container-query variants like@[700px]:grid-cols-2 stable in Tailwind CSS v4.2?
💡 Result:
Yes, both @container and arbitrary container query variants like @[700px]:grid-cols-2 (more precisely @min-[700px]:grid-cols-2) are stable in Tailwind CSS v4.2. Container queries have been a core feature since v4.0, with no breaking changes or deprecations noted in v4.2 release notes or changelogs. The official documentation explicitly supports arbitrary values using @min-[value] and @max-[value] variants, as shown in examples like @min-[475px]:flex-row and @[17.5rem]:underline. v4.2 changelogs (v4.2.0 to v4.2.3) focus on fixes and additions unrelated to container queries, confirming their stability. Multiple sources, including Tailwind docs and third-party tutorials, demonstrate usage without caveats for production.
Citations:
- 1: https://tailwindcss.com/blog/tailwindcss-v4
- 2: https://tailwindcss.com/docs/responsive-design
- 3: https://tailkits.com/blog/tailwind-container-queries/
- 4: https://staticmania.com/blog/tailwind-4-container-queries
- 5: https://www.hremonen.com/blog/are-tailwind-container-queries-supreme
- 6: https://github.com/tailwindlabs/tailwindcss-container-queries/
- 7: https://github.com/tailwindlabs/tailwindcss/blob/51aa9d79/CHANGELOG.md
🏁 Script executed:
cat -n js/app/packages/app/component/settings/Appearance.tsx | head -50Repository: macro-inc/macro
Length of output: 1185
Consider matching the gap unit source across nested grids for consistency.
Inner grid uses gap-5 (Tailwind utility, 1.25rem = 20px) while the outer container uses inline gap: '20px'. Although both currently resolve to the same value, using a single source-of-truth (either consistently Tailwind utilities or consistently inline CSS values) prevents unintended drift if the spacing scale changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/settings/Appearance.tsx` around lines 27 - 30,
The outer container div and the inner grid use different gap sources (an inline
gap: '20px' vs Tailwind gap-5 on the nested grid), so pick one source-of-truth
and apply it consistently: either replace the outer inline gap with the Tailwind
utility gap-5 (preferred) on the container div that wraps
Panel/ThemeList/ThemeEditorAdvanced, or change the nested grid to use the same
inline style; update the class on the container div or the nested div
accordingly so both grids use the same spacing unit (keep existing responsive
classes like @[700px]:grid-cols-2 intact).
| <input | ||
| onInput={(e) => { handleChromaChange(e);}} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: drop redundant arrow wrappers around handleChromaChange/handleSaturationChange.
These arrows just forward e to the named handler; the handleContrastChange slider directly uses the same pattern correctly inline. Pass the function reference instead.
- onInput={(e) => { handleChromaChange(e);}}
+ onInput={handleChromaChange}- onInput={(e) => {handleSaturationChange(e);}}
+ onInput={handleSaturationChange}Also applies to: 514-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/theme/components/ThemeEditorBasic.tsx` around lines 423 -
424, The input handlers currently wrap the event in an unnecessary arrow (e.g.
onInput={(e) => { handleChromaChange(e); }}); replace these with direct function
references (onInput={handleChromaChange}) for both handleChromaChange and
handleSaturationChange (the same pattern used by handleContrastChange), ensuring
the handler signatures remain compatible with the input event type.
| <div | ||
| style={{ | ||
| 'grid-template-columns': `min-content min-content 1fr repeat(${columnCount()}, min-content)`, | ||
| 'grid-template-columns': ` min-content 1fr repeat(${columnCount()}, min-content)`, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: stray leading whitespace in grid-template-columns.
Harmless — CSS tolerates the leading space — but worth trimming for hygiene.
- 'grid-template-columns': ` min-content 1fr repeat(${columnCount()}, min-content)`,
+ 'grid-template-columns': `min-content 1fr repeat(${columnCount()}, min-content)`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'grid-template-columns': ` min-content 1fr repeat(${columnCount()}, min-content)`, | |
| 'grid-template-columns': `min-content 1fr repeat(${columnCount()}, min-content)`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/theme/components/ThemeTools.tsx` at line 40, Remove the stray
leading space in the grid-template-columns value in ThemeTools.tsx: update the
inline style where 'grid-template-columns' is set (the template using
columnCount()) so the template string starts with "min-content" rather than "
min-content" (i.e., adjust the template literal around columnCount() in the
component to trim the leading whitespace).
| style={{ | ||
| 'white-space': 'nowrap', | ||
| 'font-size': '0.875rem', | ||
| 'font-weight': '600', | ||
| 'outline': 'none', | ||
| }} | ||
| contentEditable={true} | ||
| ref={themeName} |
There was a problem hiding this comment.
Don't strip the focus outline from the contentEditable name field.
outline: 'none' on this contentEditable removes the only visible focus indicator for keyboard users — they won't be able to tell when the theme name is focused before typing/Enter. The element also has no aria-label. Either drop outline: none and keep the native ring, or provide an explicit :focus-visible style via a class.
♿ Suggested fix
style={{
'white-space': 'nowrap',
'font-size': '0.875rem',
'font-weight': '600',
- 'outline': 'none',
}}
contentEditable={true}
+ aria-label="Theme name"
ref={themeName}If the native outline is visually undesirable, replace it with a custom :focus-visible ring in a stylesheet rather than removing it outright.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| style={{ | |
| 'white-space': 'nowrap', | |
| 'font-size': '0.875rem', | |
| 'font-weight': '600', | |
| 'outline': 'none', | |
| }} | |
| contentEditable={true} | |
| ref={themeName} | |
| style={{ | |
| 'white-space': 'nowrap', | |
| 'font-size': '0.875rem', | |
| 'font-weight': '600', | |
| }} | |
| contentEditable={true} | |
| aria-label="Theme name" | |
| ref={themeName} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/theme/components/ThemeTools.tsx` around lines 72 - 79, The
contentEditable name field in ThemeTools (the element using ref themeName and
contentEditable={true}) removes the native focus indicator via "outline:
'none'"; restore accessible focus by removing that style and either (a) delete
the outline: 'none' entry so the native focus ring remains, or (b) replace it
with a class that applies an explicit :focus-visible ring in CSS and add an
aria-label (e.g., aria-label="Theme name") to the element to ensure keyboard
users can detect focus before typing/Enter; update the inline style block or
className on the element and add the aria-label accordingly.
1 participant
