Styling improvements #2731
Styling improvements #2731
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughAdds viewport-based gating for the hero promo, widespread UI/className and spacing tweaks across toolbar and mockups, introduces flex-only guards for display controls, refactors device selector/tooltips and button sizing, loads brand colors from theme with a file watcher, and adds loading skeletons for subscription UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant W as Window
participant Hero as HeroComponent
Note over Hero: viewport height gating
U->>W: load / resize
W->>Hero: read window.innerHeight
Hero->>Hero: set isShortScreen = h < 700
alt isShortScreen
Hero-->>U: render header without promo
else
Hero-->>U: render header with promo
end
sequenceDiagram
autonumber
participant User
participant TypeInput as DisplayTypeInput
participant Style as editorEngine.style
Note over TypeInput: selecting display type
User->>TypeInput: select value
alt value == "flex"
TypeInput->>Style: updateMultiple({display:flex,flex-direction:row,align-items:flex-start,justify-content:flex-start})
else
TypeInput->>Style: update(display:value)
TypeInput->>Style: updateMultiple({flex-direction:initial,align-items:initial,justify-content:initial})
end
Note over Align/Dir: Align/Direction inputs render only when display is flex/inline-flex
sequenceDiagram
autonumber
participant BT as BrandTab
participant Theme as ThemeConfig
participant Bus as fileEventBus
Note over BT: load & sync brand colors
BT->>Theme: scan colorGroups/defaults
Theme-->>BT: brandColors
BT->>BT: setState(brandColors)
Bus->>BT: file change (tailwind config or globals.css)
BT->>Theme: re-scan
Theme-->>BT: updated brandColors
BT->>BT: setState(updated)
sequenceDiagram
autonumber
participant DS as DeviceSelector
participant UI as Tooltip
participant User
User->>DS: open dropdown
DS->>DS: set isOpen = true
DS-->>UI: disable tooltip
User->>DS: close dropdown
DS->>DS: set isOpen = false
DS-->>UI: enable tooltip("Device")
sequenceDiagram
autonumber
participant Plans as PlansPanel
participant API as BillingAPI
participant UI as Skeletons
Plans->>API: fetch subscription & usage
Plans-->>UI: show skeletons
API-->>Plans: return data
Plans-->>UI: render product/price/usage, hide skeletons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-image.tsx (1)
104-109: Dropdown can’t close via outside click (onOpenChange ignores false)handleOpenChange only forwards true, so outside clicks or ESC won’t close the menu. Forward the actual state.
- const handleOpenChange = (open: boolean) => { - if (open) { - onOpenChange(true); - } - }; + const handleOpenChange = (open: boolean) => { + onOpenChange(open); + };After this, confirm the dropdown closes on outside click and ESC.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/direction.tsx (1)
37-43: Don’t clobber inline-flex on change; preserve the current display mode.When the user is currently on inline-flex, changing direction forces display to flex, altering layout. Preserve the existing mode.
Apply:
onChange={(newValue) => { setValue(newValue); - editorEngine.style.updateMultiple({ - 'flex-direction': newValue, - display: 'flex', - }); + const currentDisplay = editorEngine.style.selectedStyle?.styles.computed.display; + editorEngine.style.updateMultiple({ + 'flex-direction': newValue, + display: currentDisplay === 'inline-flex' ? 'inline-flex' : 'flex', + }); }}If the display is already flex/inline-flex, you could also omit setting
displayentirely; the above is the least disruptive change.
🧹 Nitpick comments (28)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/color-panel/index.tsx (1)
80-90: Back button needs an accessible name; consider sticky header to avoid stacking issues
- The icon-only button has no accessible name. Add an aria-label/title or sr-only text so screen readers announce its purpose.
- Optional: switching from fixed to sticky reduces z-index/overlay risks in nested scroll containers.
Apply this diff to add accessibility and (optionally) switch to sticky:
- <div className="border-border bg-background fixed top-0 right-0 left-0 z-10 flex items-center justify-start border-b py-1.5 pr-2.5 pl-3 gap-2"> + <div className="border-border bg-background sticky top-0 z-10 flex items-center justify-start border-b py-1.5 pr-2.5 pl-3 gap-2"> <Button variant="ghost" size="icon" className="hover:bg-background-secondary h-7 w-7 rounded-md" onClick={handleClose} + aria-label="Back to Brand Tab" + title="Back" > - <Icons.ArrowLeft className="h-4 w-4" /> + <Icons.ArrowLeft className="h-4 w-4" aria-hidden /> </Button> <h2 className="text-foreground text-sm font-normal">Brand Colors</h2> </div>apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (5)
16-21: Make color tiles non-interactive for screen readers and avoid misleading pointer cursorThe square itself isn’t clickable (parent handles onClick). Mark it decorative and avoid pointer on the child to reduce confusion.
-const ColorSquare = ({ color }: ColorSquareProps) => ( - <div - className="w-full aspect-square cursor-pointer" - style={{ backgroundColor: color }} - /> -); +const ColorSquare = ({ color }: ColorSquareProps) => ( + <div + className="w-full aspect-square" + aria-hidden + role="presentation" + style={{ backgroundColor: color }} + /> +);
27-33: Split concerns and avoid re-running theme scan due to broad dependenciesThis effect mixes font and theme preloading and depends on
editorEngine.theme, which can retrigger scans unnecessarily.
- Keep the font scan keyed to
fontConfigPath.- Run
theme.scanConfig()once on mount (or let the colors effect handle it).- Verify that
editorEngine.themeobject identity is stable; otherwise this effect may rerun more than intended.Proposed change within this effect:
- // Pre-load theme configuration to avoid delay when opening color panel - editorEngine.theme.scanConfig(); -}, [editorEngine.font.fontConfigPath, editorEngine.theme]); +}, [editorEngine.font.fontConfigPath]);And add a separate mount-only effect (outside this range):
useEffect(() => { editorEngine.theme.scanConfig(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []);
35-69: Deduplicate color extraction and avoid redundant scan; cap to visible swatch count
- You call
scanConfig()here after already preloading. If preloading remains, drop the extra scan to reduce I/O.- The extraction logic appears twice in the file; centralize it to a helper to keep behavior in one place.
- The grid renders only a single 12-column row; slice to 12 here to match the UI and avoid unnecessary DOM work.
- Deduplicate colors to avoid repeated swatches when 500/default collide across groups/defaults.
Apply these focused edits:
- await editorEngine.theme.scanConfig(); const { colorGroups, colorDefaults } = editorEngine.theme; @@ - setBrandColors(projectColors); + // unique + cap to the 12 tiles we actually render + setBrandColors(Array.from(new Set(projectColors)).slice(0, 12));And add a small helper (outside this range) to reuse in both effects:
function extractProjectColors(theme: any): string[] { const out: string[] = []; const { colorGroups, colorDefaults } = theme; const pick = (group: any[]) => { group.forEach((color) => { if (color?.name === '500' || color?.name?.toLowerCase?.() === 'default') { out.push(color.lightColor); } }); }; Object.values(colorGroups).forEach(pick); Object.values(colorDefaults).forEach(pick); return Array.from(new Set(out)); // unique }
71-112: Debounce file-change handling and clear pending timers on unmount
- Using
setTimeoutwithout clearing on unmount can set state after unmount.- Rapid successive change events (common on save) will enqueue multiple scans.
Minimal changes to introduce a debounced refresh:
- Import
useRef:-import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react';
- Add a ref above the effect:
const refreshTimerRef = useRef<number | null>(null);
- Replace the inner
setTimeoutblock:- // Rescan for new color groups - setTimeout(async () => { + // Debounced rescan for new color groups + if (refreshTimerRef.current) { + window.clearTimeout(refreshTimerRef.current); + } + refreshTimerRef.current = window.setTimeout(async () => { try { - await editorEngine.theme.scanConfig(); - - const { colorGroups, colorDefaults } = editorEngine.theme; - - // Extract color-500 variants from project colors - const projectColors: string[] = []; - - // Add colors from custom color groups (user-defined in Tailwind config) - Object.values(colorGroups).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each custom color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - // Add colors from default color groups (standard Tailwind colors) - Object.values(colorDefaults).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each default color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - setBrandColors(projectColors); + await editorEngine.theme.scanConfig(); + setBrandColors(extractProjectColors(editorEngine.theme).slice(0, 12)); } catch (error) { console.warn('Theme scanning failed:', error); } - }, 100); // Small delay to ensure file is fully written + }, 150); // small debounce to ensure file is fully written
- Clear any pending timers in cleanup:
return () => { unsubscribe(); + if (refreshTimerRef.current) { + window.clearTimeout(refreshTimerRef.current); + } };Also verify that
extractProjectColors(proposed earlier) is in scope.Finally, confirm that
animate-shimmeris defined in your Tailwind config; otherwise skeletons won’t animate.
149-165: Align rendering to a single-row summary tile and improve keyboard/a11y behavior
- The container is fixed to 40px height; rendering more than 12 colors creates hidden extra rows. Slice to 12.
- Make the clickable grid keyboard-accessible with role/tabIndex and an onKeyDown handler.
- Mark skeleton cells as aria-hidden.
- <div - className="grid grid-cols-12 gap-0 rounded-lg overflow-hidden h-[40px] max-h-[40px] bg-background-onlook border-[0.5px] border-white/50 hover:border-[0.5px] hover:border-white cursor-pointer hover:border-transparent transition-all duration-200" - onClick={() => (editorEngine.state.brandTab = BrandTabValue.COLORS)} - > - {brandColors.length > 0 ? ( - brandColors.map((color, index) => ( + <div + className="grid grid-cols-12 gap-0 rounded-lg overflow-hidden h-[40px] max-h-[40px] bg-background-onlook border-[0.5px] border-white/50 hover:border-[0.5px] hover:border-white cursor-pointer hover:border-transparent transition-all duration-200" + role="button" + tabIndex={0} + aria-label="Open Brand Colors" + onKeyDown={(e) => (e.key === 'Enter' || e.key === ' ') && (editorEngine.state.brandTab = BrandTabValue.COLORS)} + onClick={() => (editorEngine.state.brandTab = BrandTabValue.COLORS)} + > + {brandColors.length > 0 ? ( + brandColors.slice(0, 12).map((color, index) => ( <ColorSquare key={`brand-color-${index}`} color={color} /> )) ) : ( Array.from({ length: 12 }, (_, index) => ( <div key={`loading-color-${index}`} - className="w-full h-full bg-gradient-to-r from-gray-300 via-gray-400 to-gray-300 bg-[length:200%_100%] animate-shimmer" + className="w-full h-full bg-gradient-to-r from-gray-300 via-gray-400 to-gray-300 bg-[length:200%_100%] animate-shimmer" + aria-hidden /> )) )} </div>apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/padding.tsx (2)
126-133: Align tooltip behavior with other triggers to avoid showing while openOther triggers disable HoverOnlyTooltip when the dropdown is open and hide the arrow. Mirror that here for consistency and to prevent tooltip flicker.
Apply:
- <HoverOnlyTooltip content="Padding" side="bottom"> + <HoverOnlyTooltip content="Padding" side="bottom" className="mt-1" hideArrow disabled={isOpen}>
129-133: Consistent width token across toolbarMost triggers use min-w-9; this matches the pattern and looks fine. If you end up standardizing, consider a ToolbarButton size prop (e.g., size="sm") to encode this width centrally.
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-image.tsx (1)
144-147: Prefer min-w-9 over fixed w-9 for consistency and layout resilienceOther triggers use min-w-9. Using min width avoids potential jitter if the spinner or icon variants slightly vary. Not critical, but keeps things uniform.
- className="flex w-9 flex-col items-center justify-center gap-0.5 relative" + className="flex min-w-9 flex-col items-center justify-center gap-0.5 relative"apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/font/font-size.tsx (1)
103-112: Add basic a11y and input constraints to the number field.Small wins that don’t change behavior but improve UX/a11y:
- Provide an accessible name so SR users hear “Font size”.
- Hint numeric keypad on mobile via inputMode.
- Add min/step to match your runtime validation.
You can apply this outside the changed hunk:
<input ref={inputRef} type="number" min={1} step={1} inputMode="numeric" aria-label="Font size" value={inputValue} onChange={handleInputChange} onKeyDown={handleInputKeyDown} onBlur={handleInputBlur} onClick={(e) => e.stopPropagation()} className="w-full bg-transparent text-center text-sm focus:outline-none [appearance:textfield] [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none" />Optionally, consider adding
tabular-numsto the className to avoid digit-width jitter.apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/window-actions-group.tsx (1)
43-53: Shrinking to w-9 reduces hit area; add aria-labels and verify target-size.
- w-9 (≈36px) is under the 44px tap target recommended by Apple/Material. It still satisfies WCAG 2.2 AA (24px), but please verify comfort on touch devices.
- Buttons rely on icons only; add aria-labels for SR users.
Two minimal changes that keep the new width:
<ToolbarButton - className="flex items-center w-9" + className="flex items-center w-9" + aria-label="Duplicate window" onClick={duplicateWindow} disabled={isDuplicating} > ... <ToolbarButton - className="flex items-center w-9" + className="flex items-center w-9" + aria-label="Delete window" disabled={!editorEngine.frames.canDelete() || isDeleting} onClick={deleteWindow} >If you want to preserve the slimmer visuals but meet a 44px hit target, consider adding a larger invisible hitbox via utility classes (e.g.,
relative after:absolute after:-inset-1 after:content-['']) on the ToolbarButton.Also applies to: 57-69
apps/web/client/src/app/_components/hero/create.tsx (1)
259-265: Spacing changes LGTM; make the Card width responsive to narrow screens.Adding p-4 and tighter gaps reads better. Since this component is referenced in home/hero contexts, guard the fixed 600px width to avoid overflow on small devices.
Apply:
- 'w-[600px] overflow-hidden gap-1.5 backdrop-blur-md bg-background/20 p-4', + 'w-[600px] max-w-full overflow-hidden gap-1.5 backdrop-blur-md bg-background/20 p-4',Alternatively:
'w-full sm:w-[600px]'if you want it to expand fluidly below sm.apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/direction.tsx (1)
22-29: Good guard: only render when flex is active.Conditionalizing on computed display prevents irrelevant controls from showing. Minor simplification:
-const displayValue = editorEngine.style.selectedStyle?.styles.computed.display; -const isFlexboxActive = displayValue === 'flex' || displayValue === 'inline-flex'; +const displayValue = editorEngine.style.selectedStyle?.styles.computed.display; +const isFlexboxActive = (displayValue ?? '').includes('flex');This also future-proofs for any other flex-like tokens.
apps/web/client/src/app/_components/shared/mockups/tailwind-color-editor.tsx (1)
211-223: Mock toolbar width trims are fine; mirror a11y where practical.Reducing to min-w-9 is consistent with the editor bar updates. Since this mock mirrors production styling, consider also adding aria-labels to icon-only buttons (e.g., “Align left”, “Advanced typography”, “More options”) to keep examples aligned with best practices.
Example:
<ToolbarButton className="flex items-center justify-center min-w-9" aria-label="Align left"> {React.createElement(Icons.TextAlignLeft as any, { className: 'h-4 w-4' } as any)} </ToolbarButton>apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/theme-group.tsx (2)
37-43: Fix hover text-color override on active theme buttons and add accessible names.ToolbarButton’s base classes include hover:text-white, which will override your active text-foreground-primary on hover. Add hover:text-foreground-primary for the active branch to avoid the flash to white. Also, these are icon-only buttons; tooltips don’t provide an accessible name. Add aria-labels.
- <ToolbarButton - className={`w-9 ${theme === SystemTheme.SYSTEM ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} + <ToolbarButton + aria-label="System theme" + className={`w-9 ${theme === SystemTheme.SYSTEM ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary hover:text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} onClick={() => changeTheme(SystemTheme.SYSTEM)} > ... - <ToolbarButton - className={`w-9 ${theme === SystemTheme.DARK ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} + <ToolbarButton + aria-label="Dark theme" + className={`w-9 ${theme === SystemTheme.DARK ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary hover:text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} onClick={() => changeTheme(SystemTheme.DARK)} > ... - <ToolbarButton - className={`w-9 ${theme === SystemTheme.LIGHT ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} + <ToolbarButton + aria-label="Light theme" + className={`w-9 ${theme === SystemTheme.LIGHT ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary hover:text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`} onClick={() => changeTheme(SystemTheme.LIGHT)} >Also applies to: 45-51, 53-59
37-59: Reduce duplication by extracting a helper for button classes.The three className templates differ only by the active check. Consider extracting a small helper to improve maintainability.
Example outside the changed lines:
const themeBtnClass = (active: boolean) => `w-9 ${active ? 'bg-background-tertiary/50 hover:bg-background-tertiary/50 text-foreground-primary hover:text-foreground-primary' : 'hover:bg-background-tertiary/50 text-foreground-onlook'}`;Usage:
<ToolbarButton aria-label="System theme" className={themeBtnClass(theme === SystemTheme.SYSTEM)} ... />apps/web/client/src/app/_components/hero/index.tsx (1)
18-30: Prevent initial flash by deriving isShortScreen in the state initializer.On short screens, the promo renders once then disappears after the effect runs. Initialize from window.innerHeight to avoid the flicker.
- const [isShortScreen, setIsShortScreen] = useState(false); + const [isShortScreen, setIsShortScreen] = useState( + () => typeof window !== 'undefined' && window.innerHeight < 700 + );Optional: throttle the resize handler (e.g., requestAnimationFrame or lodash/throttle) if resize performance becomes a concern.
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/rotate-group.tsx (1)
10-19: Add an accessible name to the icon-only rotate button.Improves screen reader support; tooltips don’t supply accessible names.
- <ToolbarButton + <ToolbarButton + aria-label="Rotate device" className="w-9" onClick={() => { const { width, height } = frameData.frame.dimension; frameData.frame.dimension.width = height; frameData.frame.dimension.height = width; }} >apps/web/client/src/components/ui/avatar-dropdown/plans.tsx (1)
31-31: Clamp usagePercent to 100 to avoid overflow.When usageCount exceeds limitCount, the progress bar can exceed bounds.
- const usagePercent = usage && usage.limitCount > 0 ? usage.usageCount / usage.limitCount * 100 : 0; + const usagePercent = + usage && usage.limitCount > 0 + ? Math.min(100, (usage.usageCount / usage.limitCount) * 100) + : 0;apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/gap.tsx (1)
26-38: Avoid writing invalid CSS when the input is empty or non-numeric.Currently, clearing the input could write values like "px" or "em". Guard updates to only commit valid numeric values.
<InputIcon value={numValue} unit={unitValue} onChange={(newValue) => { setNumValue(newValue); - editorEngine.style.update('gap', `${newValue}${unitValue}`); + if (newValue !== '' && !Number.isNaN(Number(newValue))) { + editorEngine.style.update('gap', `${newValue}${unitValue}`); + } }} onUnitChange={(newUnit) => { setUnitValue(newUnit); - editorEngine.style.update('gap', `${numValue}${newUnit}`); + if (numValue !== '' && !Number.isNaN(Number(numValue))) { + editorEngine.style.update('gap', `${numValue}${newUnit}`); + } }} />Optional: Clamp negative values to 0 since CSS gap cannot be negative.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/align.tsx (1)
72-75: Label width consistency verified; optional axis-aware labels
All Display dropdown labels (Direction,Gap,Vertical,Horizontal) uniformly usew-20, so there’s no width mismatch or panel jiggle.Affected files/locations:
- apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/direction.tsx (line 33)
- apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/gap.tsx (line 24)
- apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/align.tsx (lines 74, 109)
Optional refactor: switch static “Vertical”/“Horizontal” labels to reflect the current
flexDirection. For example:const isColumn = editorEngine.style.selectedStyle?.styles.computed.flexDirection === 'column'; <span className="text-sm text-muted-foreground w-20"> {isColumn ? 'Horizontal' : 'Vertical'} </span>apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/device-selector.tsx (2)
11-24: DRY icon sizing; centralize the base class.Both
DeviceIconandCustomIconcompute identical size classes. Define a singlebaseIconClassand reuse it to keep sizes consistent and simpler to tweak in the future.Apply this diff:
+const baseIconClass = 'h-3.5 w-3.5 min-h-3.5 min-w-3.5'; -const DeviceIcon = ({ deviceType, orientation, className }: { deviceType: string, orientation: Orientation, className?: string }) => { - const iconClassName = `h-3.5 w-3.5 min-h-3.5 min-w-3.5 ${className || ''}`; +const DeviceIcon = ({ deviceType, orientation, className }: { deviceType: string, orientation: Orientation, className?: string }) => { + const iconClassName = `${baseIconClass} ${className || ''}`; @@ - default: - return <CustomIcon orientation={orientation} className={className} />; + default: + return <CustomIcon orientation={orientation} className={className} />; } }; -const CustomIcon = ({ orientation, className }: { orientation: Orientation, className?: string }) => { - const iconClassName = `h-3.5 w-3.5 min-h-3.5 min-w-3.5 ${className || ''}`; +const CustomIcon = ({ orientation, className }: { orientation: Orientation, className?: string }) => { + const iconClassName = `${baseIconClass} ${className || ''}`;Note: Passing
classNametoCustomIconis fine because it recomputes with the same base size.Also applies to: 27-34
110-118: Minor cleanup: reduce repeated selection checks in item rendering.The selection condition is repeated three times (item class, icon color, micro-dimension color). Consider computing
const isSelected = device === \${category}:${name}`` in the map callback and reusing it to simplify classNames and avoid drift.If you prefer a minimal change without refactoring the map:
- className={`text-xs flex items-center cursor-pointer ${ - device === `${category}:${name}` - ? 'bg-background-tertiary/50 text-foreground-primary' - : '' - }`} + className={`text-xs flex items-center cursor-pointer ${device === `${category}:${name}` ? 'bg-background-tertiary/50 text-foreground-primary' : ''}`}Optional: adopt a
cn()util to compose classes more readably across the file.apps/web/client/src/app/project/[id]/_components/editor-bar/index.tsx (1)
100-100: Padding tweak LGTM; consider avoiding half-pixel borders for crisper rendering.
border-[0.5px]can render blurry on some displays/browsers. A 1px border with reduced opacity typically looks sharper while preserving the light-weight look.Apply this diff:
- className={cn( - 'flex flex-col border-[0.5px] border-border p-1 px-1 bg-background rounded-xl backdrop-blur drop-shadow-xl z-50 overflow-hidden', + className={cn( + 'flex flex-col border border-border/50 p-1 px-1 bg-background rounded-xl backdrop-blur drop-shadow-xl z-50 overflow-hidden',apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/opacity.tsx (1)
82-82: Stray Tailwind token “hover” in Input className.
hoverwithout a utility is a no-op and can trigger unnecessary class churn in JIT. Suggest removing it.- className="px-1 w-8 text-left data-[state=open]:text-white text-small focus:text-foreground-primary focus-visible:ring-0 focus-visible:ring-offset-0 focus-visible:outline-none !bg-transparent border-none group-hover:text-foreground-primary focus:ring-0 focus:outline-none text-muted-foreground !hide-spin-buttons no-focus-ring [appearance:textfield] group-hover:text-foreground-primary transition-colors duration-150 hover" + className="px-1 w-8 text-left data-[state=open]:text-white text-small focus:text-foreground-primary focus-visible:ring-0 focus-visible:ring-offset-0 focus-visible:outline-none !bg-transparent border-none group-hover:text-foreground-primary focus:ring-0 focus:outline-none text-muted-foreground !hide-spin-buttons no-focus-ring [appearance:textfield] group-hover:text-foreground-primary transition-colors duration-150"apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/index.tsx (1)
58-59: Guard against small-view overflows in the dropdown.Bumping min width to 250px improves content fit, but can overflow on narrow viewports. Add a responsive min width and an upper bound.
- <DropdownMenuContent align="start" className="min-w-[250px] mt-2 p-1.5 rounded-lg"> + <DropdownMenuContent align="start" className="min-w-[220px] sm:min-w-[250px] max-w-[90vw] mt-2 p-1.5 rounded-lg">apps/web/client/src/app/project/[id]/_components/editor-bar/overflow-menu.tsx (1)
29-35: Add ARIA state to improve screen-reader context.Expose popup semantics and open state on the trigger.
- <DropdownMenuTrigger asChild> + <DropdownMenuTrigger asChild> <ToolbarButton isOpen={isOpen} - className="w-9 flex items-center justify-center" + className="w-9 flex items-center justify-center" aria-label="Show more toolbar controls" + aria-haspopup="menu" + aria-expanded={isOpen} >apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx (1)
24-39: Re-evaluate default/resets for flex properties to avoid unexpected layout shifts.
- Switching to flex: setting
align-items: flex-startdiverges from the CSS initial ofstretch. This can change cross-axis sizing unexpectedly when users simply toggle “Flex”.- Switching away from flex: writing
initialvalues persists explicit CSS that may override theme/cascade later. Often, not writing the property at all is preferable to “reset”.Option A — Minimal, preserve CSS defaults:
- if (newValue === 'flex') { - // When switching to flex, set default flex properties - editorEngine.style.updateMultiple({ - display: 'flex', - 'flex-direction': 'row', - 'align-items': 'flex-start', - 'justify-content': 'flex-start', - }); - } else { - // When switching away from flex, clear flex properties - editorEngine.style.update('display', newValue); - // Clear flex-specific properties by setting them to initial values - editorEngine.style.update('flex-direction', 'initial'); - editorEngine.style.update('align-items', 'initial'); - editorEngine.style.update('justify-content', 'initial'); - } + if (newValue === 'flex') { + // Let CSS defaults apply: flex-direction=row, align-items=stretch, justify-content=flex-start + editorEngine.style.update('display', 'flex'); + } else { + // Only change the display; avoid persisting explicit resets. + editorEngine.style.update('display', newValue); + }Option B — If explicit values are required by the engine:
- For “to flex”, set
align-items: stretch(matches CSS default) and omitjustify-contentsince default is alreadyflex-start.- For “from flex”, prefer removing properties (if supported by the API) instead of writing
initial. For example,editorEngine.style.updateMultiple({ 'flex-direction': undefined, 'align-items': undefined, 'justify-content': undefined })or a dedicatedclearAPI.Please confirm how
editorEngine.styletreats absent vs.'initial'values so we can finalize the safest variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
apps/web/client/src/app/_components/hero/create.tsx(1 hunks)apps/web/client/src/app/_components/hero/index.tsx(2 hunks)apps/web/client/src/app/_components/shared/mockups/tailwind-color-editor.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border-color.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/align.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/direction.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/gap.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/margin.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/opacity.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/padding.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/radius.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-image.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/overflow-menu.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/advanced-typography.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/font/font-size.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/text-align.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/device-selector.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/rotate-group.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/theme-group.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/window-actions-group.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/color-panel/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx(2 hunks)apps/web/client/src/components/ui/avatar-dropdown/plans.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
apps/web/client/src/components/ui/avatar-dropdown/plans.tsx (2)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)packages/stripe/src/constants.ts (1)
FREE_PRODUCT_CONFIG(46-52)
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx (2)
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-radio.tsx (1)
InputRadio(23-52)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/index.tsx (1)
layoutTypeOptions(23-27)
apps/web/client/src/app/project/[id]/_components/editor-bar/overflow-menu.tsx (2)
apps/web/client/src/app/project/[id]/_components/editor-bar/hover-tooltip.tsx (1)
HoverOnlyTooltip(14-35)apps/web/client/src/app/project/[id]/_components/editor-bar/toolbar-button.tsx (1)
ToolbarButton(13-67)
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/index.tsx (1)
packages/ui/src/components/dropdown-menu.tsx (1)
DropdownMenuContent(216-216)
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/gap.tsx (1)
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx (1)
InputIcon(31-75)
apps/web/client/src/app/_components/shared/mockups/tailwind-color-editor.tsx (2)
apps/web/client/src/app/project/[id]/_components/editor-bar/toolbar-button.tsx (1)
ToolbarButton(13-67)apps/web/client/src/app/project/[id]/_components/editor-bar/separator.tsx (1)
InputSeparator(1-3)
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/theme-group.tsx (2)
apps/web/client/src/app/project/[id]/_components/editor-bar/toolbar-button.tsx (1)
ToolbarButton(13-67)apps/web/client/src/app/project/[id]/_components/editor-bar/hover-tooltip.tsx (1)
HoverOnlyTooltip(14-35)
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/rotate-group.tsx (3)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
FrameData(11-15)apps/web/client/src/app/project/[id]/_components/editor-bar/hover-tooltip.tsx (1)
HoverOnlyTooltip(14-35)apps/web/client/src/app/project/[id]/_components/editor-bar/toolbar-button.tsx (1)
ToolbarButton(13-67)
apps/web/client/src/app/_components/hero/index.tsx (2)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)apps/web/client/src/app/_components/hero/unicorn-background.tsx (1)
UnicornBackground(73-145)
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/device-selector.tsx (2)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(9-13)apps/web/client/src/app/project/[id]/_components/editor-bar/hover-tooltip.tsx (1)
HoverOnlyTooltip(14-35)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (4)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(9-13)apps/web/client/src/components/store/editor/theme/index.ts (2)
colorGroups(618-620)colorDefaults(622-624)apps/web/client/src/components/store/editor/sandbox/index.ts (2)
handleFileChange(414-483)index(77-131)apps/web/client/src/components/store/editor/sandbox/file-event-bus.ts (1)
unsubscribe(34-36)
🪛 Biome (2.1.2)
apps/web/client/src/app/_components/hero/index.tsx
[error] 45-45: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🔇 Additional comments (16)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (1)
114-128: Confirm subscribe() returns an unsubscribe function; otherwise switch to explicit unsubscribe APIThe cleanup calls
unsubscribe()assuming the bus returns a disposer. Ifsubscribedoesn’t return one, use the bus’sunsubscribe(eventType, callback)API instead.If needed, keep a reference to the callback and swap cleanup:
const cb = (event: FileEvent) => { /* ... */ }; editorEngine.sandbox.fileEventBus.subscribe('*', cb); return () => { editorEngine.sandbox.fileEventBus.unsubscribe('*', cb); };apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border-color.tsx (1)
46-51: Width tweak looks safe; verify hit area on touch devicesReducing to min-w-9 keeps visual consistency with the rest of the bar; just double-check that 36px min width still meets your target tap area on smaller touch screens.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/margin.tsx (1)
154-160: LGTM on min-width reductionThe trigger remains legible with icon + value while aligning with the toolbar’s tighter spacing.
apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/advanced-typography.tsx (1)
56-60: LGTM on min-width reductionIcon-only trigger with min-w-9 and px-2 remains balanced and consistent with adjacent controls.
apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/font/font-size.tsx (1)
98-104: Width tweak makes sense; confirm 3-digit values fit without truncation.Moving to w-11 (≈44px) with min-w-[40px] gives the input enough room for typical 2–3 digit sizes and aligns closely with the 48px dropdown. Looks good.
Please sanity-check that values like 100 and 128 don’t visually clip in your default font and scaling.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/direction.tsx (1)
32-34: Nit: consistent label width and spacing look good.Moving to gap-0 and w-20 aligns with the other display controls. No issues spotted.
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/rotate-group.tsx (1)
12-16: Confirm state reactivity on direct dimension mutation.If frameData.frame.dimension isn’t observable or doesn’t trigger a render/update, the UI may not reflect the swap. Ensure these assignments notify the relevant store/engine, or route through an action that does so.
Would you like me to scan for the frame/engine mutation helpers and propose a small wrapper (e.g., rotateFrame(frameData))?
apps/web/client/src/components/ui/avatar-dropdown/plans.tsx (2)
58-68: Nice loading UX with skeletons.The loading placeholders for product/price and usage are clear and consistent with the rest of the UI.
Also applies to: 71-81
37-44: Verify scheduledChangeAt is a Date.toLocaleDateString will throw if scheduledChangeAt is a string or undefined. If the API returns an ISO string, coerce via new Date(...) or adjust the tRPC transformer to hydrate Dates.
If needed, I can add a safe formatter util (handles undefined and string/Date input) and wire it here.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx (1)
79-79: Width tweak LGTM; just watch content truncation.The drop from
min-w-10tomin-w-9looks good and aligns with the PR’s tightening. Since this trigger can also show a numeric value, keep an eye on wrapping/truncation at smaller zoom levels.Quick check: open Border with a 3-digit value (e.g., 128) and confirm it doesn’t wrap in common app widths.
apps/web/client/src/app/project/[id]/_components/editor-bar/text-inputs/text-align.tsx (1)
36-36: LGTM on min-width reduction.The switch to
min-w-9keeps the trigger compact without affecting icon hit area.apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/radius.tsx (1)
109-109: LGTM; consistent with other toolbar triggers.
min-w-9aligns with adjacent controls. The “Full” label still reads clearly at this width.apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/opacity.tsx (1)
68-71: Good cleanup: remove fixed height to rely on ToolbarButton’s h-9 baseline.This removes a potential height conflict and keeps trigger sizing consistent across the toolbar.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/index.tsx (1)
49-49: Consistent trigger width.Using
min-w-9aligns this trigger with other toolbar buttons and helps prevent layout jitter.apps/web/client/src/app/project/[id]/_components/editor-bar/overflow-menu.tsx (1)
27-37: Nice UX polish: tooltip on closed state only.Wrapping the trigger with HoverOnlyTooltip and disabling it when open avoids overlapping popovers and keeps hints discoverable.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx (1)
17-19: Layout spacing/readability improvement LGTM.Reducing to
gap-0and tightening the label tow-20aligns with the rest of the display controls.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/align.tsx
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/device-selector.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| <Icons.ArrowRight className="w-4 h-4" /> | ||
| </a> | ||
| <a | ||
| <a |
There was a problem hiding this comment.
There's a duplicate opening <a> tag on line 45 that creates invalid HTML. This will cause rendering issues and potential accessibility problems. The duplicate tag should be removed, keeping only the properly formed anchor element that follows it.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/editor-bar/window-selected/device-selector.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/app/_components/hero/index.tsx (1)
44-49: External link now includes rel="noopener noreferrer" — resolved.This addresses the earlier security/lint warning for target="_blank".
🧹 Nitpick comments (2)
apps/web/client/src/app/_components/hero/index.tsx (2)
21-30: Prefer matchMedia over resize listener to reduce work and avoid unnecessary rerenders.The resize handler fires continuously while the user resizes, which can cause many state updates. A media query listener is cheaper and semantically matches the intent. Also consider extracting the 700 value into a constant to avoid a magic number.
Apply this diff inside the effect:
- useEffect(() => { - const checkScreenHeight = () => { - setIsShortScreen(window.innerHeight < 700); - }; - - checkScreenHeight(); - window.addEventListener('resize', checkScreenHeight); - - return () => window.removeEventListener('resize', checkScreenHeight); - }, []); + useEffect(() => { + const mql = window.matchMedia('(max-height: 700px)'); + // Initialize from current state + setIsShortScreen(mql.matches); + const handleChange = (e: MediaQueryListEvent) => setIsShortScreen(e.matches); + if ('addEventListener' in mql) { + mql.addEventListener('change', handleChange); + return () => mql.removeEventListener('change', handleChange); + } else { + // Safari < 14 fallback + // @ts-ignore older API + mql.addListener(handleChange); + // @ts-ignore older API + return () => mql.removeListener(handleChange); + } + }, []);Optionally, define a constant near the component:
const SHORT_SCREEN_MAX_HEIGHT = 700; // pxAnd reference it in the query string.
52-53: Optional a11y: ensure decorative icon is hidden from screen readers.If
Icons.ArrowRightisn’t alreadyaria-hiddeninternally, addaria-hidden="true"on the icon or provide an explicitaria-labelon the link. The link text is clear, so hiding the icon is preferred.- <Icons.ArrowRight className="w-4 h-4" /> + <Icons.ArrowRight className="w-4 h-4" aria-hidden="true" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/client/src/app/_components/hero/index.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/client/src/app/_components/hero/index.tsx (1)
apps/web/client/src/app/_components/hero/unicorn-background.tsx (1)
UnicornBackground(73-145)
🪛 Biome (2.1.2)
apps/web/client/src/app/_components/hero/index.tsx
[error] 46-46: expected , but instead found href
Remove href
(parse)
[error] 46-46: expected , but instead found =
Remove =
(parse)
[error] 47-47: expected , but instead found target
Remove target
(parse)
[error] 47-47: expected , but instead found =
Remove =
(parse)
[error] 48-48: expected , but instead found rel
Remove rel
(parse)
[error] 48-48: expected , but instead found =
Remove =
(parse)
[error] 49-49: expected , but instead found className
Remove className
(parse)
[error] 49-49: expected , but instead found =
Remove =
(parse)
[error] 51-51: unterminated string literal
The closing quote must be on the same line.
(parse)
[error] 52-52: expected > but instead found <
Remove <
(parse)
🔇 Additional comments (3)
apps/web/client/src/app/_components/hero/index.tsx (3)
6-6: Import useEffect alongside useState — looks good.Correctly scoped to a client component and only used inside effects.
18-18: Adds isShortScreen state — OK.Clear naming, boolean, and local to this component.
36-36: Minor spacing tweak — OK.The added pt-8 aligns with the surrounding layout and motion timings.
| value={value} | ||
| onChange={(newValue) => { | ||
| setValue(newValue); | ||
| editorEngine.style.update('display', newValue); |
There was a problem hiding this comment.
Not sure we want to do this. If we want to switch back and forth don't we want to retain the existing flex setup?
| setTimeout(async () => { | ||
| try { | ||
| await editorEngine.theme.scanConfig(); | ||
|
|
||
| const { colorGroups, colorDefaults } = editorEngine.theme; | ||
|
|
||
| // Extract color-500 variants from project colors | ||
| const projectColors: string[] = []; | ||
|
|
||
| // Add colors from custom color groups (user-defined in Tailwind config) | ||
| Object.values(colorGroups).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each custom color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| // Add colors from default color groups (standard Tailwind colors) | ||
| Object.values(colorDefaults).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each default color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| setBrandColors(projectColors); | ||
| } catch (error) { | ||
| console.warn('Theme scanning failed:', error); | ||
| } | ||
| }, 100); // Small delay to ensure file is fully written |
There was a problem hiding this comment.
Potential memory leak in file change handler
The async operations inside setTimeout could lead to state updates after component unmount. If the component unmounts while the theme scanning is in progress, setBrandColors(projectColors) will attempt to update state on an unmounted component.
Consider adding a mount status check:
const isMounted = useRef(true);
useEffect(() => {
return () => {
isMounted.current = false;
};
}, []);
// Then in the setTimeout callback:
setTimeout(async () => {
try {
await editorEngine.theme.scanConfig();
// ...processing logic...
if (isMounted.current) {
setBrandColors(projectColors);
}
} catch (error) {
console.warn('Theme scanning failed:', error);
}
}, 100);This pattern ensures state updates only occur when the component is still mounted, preventing memory leaks and React warnings about updates on unmounted components.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| const loadBrandColors = async () => { | ||
| await editorEngine.theme.scanConfig(); | ||
| const { colorGroups, colorDefaults } = editorEngine.theme; | ||
|
|
||
| // Extract color-500 variants from project colors | ||
| const projectColors: string[] = []; | ||
|
|
||
| // Add colors from custom color groups (user-defined in Tailwind config) | ||
| Object.values(colorGroups).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each custom color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| // Add colors from default color groups (standard Tailwind colors) | ||
| Object.values(colorDefaults).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each default color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| setBrandColors(projectColors); | ||
| }; | ||
|
|
||
| loadBrandColors(); | ||
| }, [editorEngine.theme]); |
There was a problem hiding this comment.
Potential Duplicate Colors in Brand Panel
The code processes both colorGroups and colorDefaults with identical logic, extracting colors with the same name criteria ('500', 'default', or 'DEFAULT'). This approach could lead to duplicate colors in the projectColors array if the same color appears in both collections.
Consider adding a deduplication step before setting the state:
// Remove duplicates before setting state
const uniqueColors = [...new Set(projectColors)];
setBrandColors(uniqueColors);Alternatively, if maintaining the order is important, a more explicit deduplication approach could be used to ensure the brand color palette displays each color only once.
| const loadBrandColors = async () => { | |
| await editorEngine.theme.scanConfig(); | |
| const { colorGroups, colorDefaults } = editorEngine.theme; | |
| // Extract color-500 variants from project colors | |
| const projectColors: string[] = []; | |
| // Add colors from custom color groups (user-defined in Tailwind config) | |
| Object.values(colorGroups).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each custom color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Add colors from default color groups (standard Tailwind colors) | |
| Object.values(colorDefaults).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each default color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| setBrandColors(projectColors); | |
| }; | |
| loadBrandColors(); | |
| }, [editorEngine.theme]); | |
| const loadBrandColors = async () => { | |
| await editorEngine.theme.scanConfig(); | |
| const { colorGroups, colorDefaults } = editorEngine.theme; | |
| // Extract color-500 variants from project colors | |
| const projectColors: string[] = []; | |
| // Add colors from custom color groups (user-defined in Tailwind config) | |
| Object.values(colorGroups).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each custom color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Add colors from default color groups (standard Tailwind colors) | |
| Object.values(colorDefaults).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each default color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Remove duplicates before setting state | |
| const uniqueColors = [...new Set(projectColors)]; | |
| setBrandColors(uniqueColors); | |
| }; | |
| loadBrandColors(); | |
| }, [editorEngine.theme]); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| // Extract color-500 variants from project colors | ||
| const projectColors: string[] = []; | ||
|
|
||
| // Add colors from custom color groups (user-defined in Tailwind config) | ||
| Object.values(colorGroups).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each custom color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| // Add colors from default color groups (standard Tailwind colors) | ||
| Object.values(colorDefaults).forEach(group => { | ||
| group.forEach(color => { | ||
| // Get the default/500 color from each default color group | ||
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | ||
| projectColors.push(color.lightColor); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Code Duplication in Color Extraction Logic
The color extraction logic is duplicated between the loadBrandColors function and the file change handler. Both sections contain identical code for processing colorGroups and colorDefaults with the same conditions.
Consider extracting this logic into a separate function:
function extractBrandColors(colorGroups, colorDefaults) {
const projectColors = [];
// Process custom color groups
Object.values(colorGroups).forEach(group => {
group.forEach(color => {
if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') {
projectColors.push(color.lightColor);
}
});
});
// Process default color groups
Object.values(colorDefaults).forEach(group => {
group.forEach(color => {
if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') {
projectColors.push(color.lightColor);
}
});
});
return projectColors;
}This would improve maintainability and ensure consistent behavior across both code paths.
| // Extract color-500 variants from project colors | |
| const projectColors: string[] = []; | |
| // Add colors from custom color groups (user-defined in Tailwind config) | |
| Object.values(colorGroups).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each custom color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Add colors from default color groups (standard Tailwind colors) | |
| Object.values(colorDefaults).forEach(group => { | |
| group.forEach(color => { | |
| // Get the default/500 color from each default color group | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| projectColors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Extract color-500 variants from project colors | |
| const projectColors = extractBrandColors(colorGroups, colorDefaults); | |
| /** | |
| * Extracts brand colors from color groups and defaults | |
| * @param colorGroups - Custom color groups from Tailwind config | |
| * @param colorDefaults - Default Tailwind color groups | |
| * @returns Array of extracted brand colors | |
| */ | |
| function extractBrandColors(colorGroups, colorDefaults) { | |
| const colors = []; | |
| // Process custom color groups | |
| Object.values(colorGroups).forEach(group => { | |
| group.forEach(color => { | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| colors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| // Process default color groups | |
| Object.values(colorDefaults).forEach(group => { | |
| group.forEach(color => { | |
| if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { | |
| colors.push(color.lightColor); | |
| } | |
| }); | |
| }); | |
| return colors; | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (2)
70-112: Debounce and add retry/backoff for file-change rescans (race-proof the 100ms timer).A fixed 100ms delay is fragile when editors write files in bursts. Debounce changes and add bounded retries with backoff to avoid scanning partial files.
- const handleFileChange = async (filePath: string) => { + let debounceId: ReturnType<typeof setTimeout> | null = null; + const rescanWithRetry = (retries = 3) => { + const attempt = async () => { + try { + await editorEngine.theme.scanConfig(); + setBrandColors(extractBrandColors(editorEngine.theme)); + } catch (error) { + if (retries > 0) { + setTimeout(() => rescanWithRetry(retries - 1), 500); + } else { + console.error('Theme scanning failed after maximum retries:', error); + } + } + }; + void attempt(); + }; + + const handleFileChange = async (filePath: string) => { // Check if the changed file is the Tailwind config if (filePath.includes('tailwind.config') || filePath.includes('globals.css')) { - // Rescan for new color groups - setTimeout(async () => { - try { - await editorEngine.theme.scanConfig(); - - const { colorGroups, colorDefaults } = editorEngine.theme; - - // Extract color-500 variants from project colors - const projectColors: string[] = []; - - // Add colors from custom color groups (user-defined in Tailwind config) - Object.values(colorGroups).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each custom color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - // Add colors from default color groups (standard Tailwind colors) - Object.values(colorDefaults).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each default color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - setBrandColors(projectColors); - } catch (error) { - console.warn('Theme scanning failed:', error); - } - }, 100); // Small delay to ensure file is fully written + // Debounce to coalesce rapid successive writes and then retry/backoff. + if (debounceId) clearTimeout(debounceId); + debounceId = setTimeout(() => rescanWithRetry(3), 500); } };Also clear debounceId in the effect cleanup (see next comment).
114-128: Subscription cleanup: don’t assume subscribe() returns a function; unsubscribe explicitly and guard cleanup.The event bus exposes unsubscribe(eventType, callback). Prefer explicit unsubscribe to avoid runtime errors if subscribe returns void, and clear any pending debounce timer.
- // Listen for file changes in the sandbox - const unsubscribe = editorEngine.sandbox.fileEventBus.subscribe('*', (event) => { + // Listen for file changes in the sandbox + const onEvent = (event: any) => { // Check if any of the changed files are Tailwind config files const isTailwindConfigChange = event.paths.some(path => path.includes('tailwind.config') || path.includes('globals.css') ); if (isTailwindConfigChange && event.paths[0]) { handleFileChange(event.paths[0]); } - }); + }; + editorEngine.sandbox.fileEventBus.subscribe('*', onEvent); return () => { - unsubscribe(); + try { + // Clear pending debounced rescan if present + // (debounceId is declared in the effect above) + // @ts-ignore - exists when the debounced variant is adopted + if (typeof debounceId !== 'undefined' && debounceId) { + clearTimeout(debounceId); + } + editorEngine.sandbox.fileEventBus.unsubscribe('*', onEvent); + } catch { + // no-op + } };If you prefer the “subscribe returns unsubscribe” pattern, at least null-check before invoking as suggested in the previous review.
🧹 Nitpick comments (6)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (6)
18-18: Nit: Cursor pointer on child without handler.The grid container handles onClick, so the cursor style on ColorSquare is redundant and slightly misleading if the square is used elsewhere. Consider dropping it or wiring the click at the square level.
- className="w-full aspect-square cursor-pointer" + className="w-full aspect-square"
25-25: State addition looks good; consider distinguishing loading vs. empty.Right now an empty array renders the shimmer placeholders indefinitely (also when no colors exist). Add a separate isLoading flag so “no colors found” can be represented distinctly.
- const [brandColors, setBrandColors] = useState<string[]>([]); + const [brandColors, setBrandColors] = useState<string[]>([]); + const [isLoadingColors, setIsLoadingColors] = useState<boolean>(true);And set isLoadingColors=false after successful/failed loads; use it in the render condition.
31-33: Avoid unhandled promise rejections in pre-load scan.scanConfig likely returns a Promise. Wrap with a catch to prevent unhandled rejections and to aid debugging. Also confirm this pre-load isn’t redundant with the subsequent loadBrandColors effect that scans again.
- editorEngine.theme.scanConfig(); + void editorEngine.theme.scanConfig().catch((e) => + console.warn('Preload theme scan failed:', e) + );
37-65: Deduplicate color extraction logic and harden against malformed groups.This block is repeated below in the file-change handler, and it assumes array shapes and present lightColor. Extract a helper, dedupe values, slice to 12 (since UI shows 12 columns), and add error handling. Also set a loading state when starting/finishing.
- const loadBrandColors = async () => { - await editorEngine.theme.scanConfig(); - const { colorGroups, colorDefaults } = editorEngine.theme; - - // Extract color-500 variants from project colors - const projectColors: string[] = []; - - // Add colors from custom color groups (user-defined in Tailwind config) - Object.values(colorGroups).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each custom color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - // Add colors from default color groups (standard Tailwind colors) - Object.values(colorDefaults).forEach(group => { - group.forEach(color => { - // Get the default/500 color from each default color group - if (color.name === '500' || color.name === 'default' || color.name === 'DEFAULT') { - projectColors.push(color.lightColor); - } - }); - }); - - setBrandColors(projectColors); - }; + const loadBrandColors = async () => { + try { + // optional: setIsLoadingColors?.(true); + await editorEngine.theme.scanConfig(); + setBrandColors(extractBrandColors(editorEngine.theme)); + } catch (e) { + console.warn('Failed to load brand colors:', e); + setBrandColors([]); + } finally { + // optional: setIsLoadingColors?.(false); + } + };Additional helper (place at module scope):
function extractBrandColors(theme: any): string[] { const pick = (group: any): string[] => { if (!Array.isArray(group)) return []; const out: string[] = []; for (const color of group) { const name = String(color?.name ?? '').toLowerCase(); if (name === '500' || name === 'default') { const v = color?.lightColor; if (typeof v === 'string' && v.trim()) out.push(v); } } return out; }; const values: string[] = []; Object.values(theme.colorGroups || {}).forEach((g: any) => values.push(...pick(g))); Object.values(theme.colorDefaults || {}).forEach((g: any) => values.push(...pick(g))); return Array.from(new Set(values)).slice(0, 12); }
149-151: Tailwind class conflict on hover border color.Both hover:border-white and hover:border-transparent are present; the latter wins due to order, making the former dead code. Remove the conflicting class and keep the intended hover behavior.
- className="grid grid-cols-12 gap-0 rounded-lg overflow-hidden h-[40px] max-h-[40px] bg-background-onlook border-[0.5px] border-white/50 hover:border-[0.5px] hover:border-white cursor-pointer hover:border-transparent transition-all duration-200" + className="grid grid-cols-12 gap-0 rounded-lg overflow-hidden h-[40px] max-h-[40px] bg-background-onlook border-[0.5px] border-white/50 hover:border-[0.5px] hover:border-transparent cursor-pointer transition-all duration-200"If the intent was to emphasize the border on hover, keep hover:border-white and drop hover:border-transparent instead.
153-164: Constrain preview to 12 swatches and prefer stable keys; handle “no colors” vs. “loading”.
- Use slice(0, 12) to avoid clipped overflow when more colors are available.
- Prefer color-derived keys when possible; fallback to index to avoid duplicates.
- Render placeholders only when loading (see isLoadingColors suggestion).
- {brandColors.length > 0 ? ( - brandColors.map((color, index) => ( - <ColorSquare key={`brand-color-${index}`} color={color} /> - )) - ) : ( - Array.from({ length: 12 }, (_, index) => ( + {brandColors.length > 0 ? ( + brandColors.slice(0, 12).map((color, index) => ( + <ColorSquare key={`brand-color-${color}-${index}`} color={color} /> + )) + ) : ( + Array.from({ length: 12 }, (_, index) => ( <div key={`loading-color-${index}`} className="w-full h-full bg-gradient-to-r from-gray-300 via-gray-400 to-gray-300 bg-[length:200%_100%] animate-shimmer" /> )) )}Also consider showing a small “No brand colors detected” message instead of shimmer when not loading and brandColors.length === 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/client/src/app/_components/hero/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/display/type.tsx
- apps/web/client/src/app/_components/hero/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (4)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(9-13)apps/web/client/src/components/store/editor/theme/index.ts (2)
colorGroups(618-620)colorDefaults(622-624)apps/web/client/src/components/store/editor/sandbox/index.ts (2)
handleFileChange(414-483)index(77-131)apps/web/client/src/components/store/editor/sandbox/file-event-bus.ts (1)
unsubscribe(34-36)
🔇 Additional comments (2)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx (2)
5-5: LGTM on hooks import.Importing useEffect/useState is appropriate for the added reactive behavior.
159-163: Verify animate-shimmer utility exists in Tailwind config.The skeleton relies on a custom animate-shimmer class. If this utility isn’t defined, the effect won’t show. Confirm it exists in your Tailwind plugin setup.
Description
Related Issues
Type of Change
Important
Styling improvements include brand panel color loading, hiring promo visibility adjustments, and UI component refinements for better user experience.
index.tsx.color-panel/index.tsx.plans.tsx.device-selector.tsx.align.tsx,direction.tsx.gap.tsx.create.tsx,index.tsx.color-panel/index.tsx.This description was created by
for b807ae3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Style