Frontend refactoring#1616
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request implements a comprehensive design system overhaul across the entire UI, transitioning from a prior design language to a GCP-inspired visual style. Changes include: rewriting the TabBar component to use custom divs instead of Radix primitives, completely reworking the global CSS with new color tokens and component classes, updating dozens of UI components to align with the new design system, simplifying focus ring styling throughout, constraining page widths with max-width utilities, adjusting hover states and spacing, redesigning the layout and navigation sidebar, changing the default theme from dark to light, and restructuring the audit logs page to use native HTML tables. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This pull request involves substantial, heterogeneous changes across 50+ files spanning multiple layers of the codebase: a complete design system overhaul (global.css), complete TabBar rewrite, dozens of component styling updates, layout restructuring, and page-wide standardization. While many changes follow consistent patterns (focus ring simplifications, max-width constraints, hover state updates), they are applied across disparate file types (CSS, React components, pages) with varying contexts and require verification that the new design system is consistently applied and that public API changes (TabBar export, Tabs value prop removal, Dialog exports) don't break existing consumers. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
ui/src/features/dags/components/dag-list/DAGTable.tsx (2)
333-377:⚠️ Potential issue | 🟡 MinorRestore a subtle focus-visible indicator for the expand toggle controls.
These are keyboard-focusable buttons; removing focus cues makes focus hard to track.
💡 Suggested tweak
- className="flex items-center justify-center text-muted-foreground cursor-pointer h-6 w-6 focus:outline-none rounded" + className="flex items-center justify-center text-muted-foreground cursor-pointer h-6 w-6 rounded focus-visible:outline focus-visible:outline-2 focus-visible:outline-primary/60 focus-visible:outline-offset-2"- className="flex items-center justify-center min-h-[2.5rem] text-muted-foreground cursor-pointer focus:outline-none rounded" + className="flex items-center justify-center min-h-[2.5rem] text-muted-foreground cursor-pointer rounded focus-visible:outline focus-visible:outline-2 focus-visible:outline-primary/60 focus-visible:outline-offset-2"As per coding guidelines, "Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility".
442-462:⚠️ Potential issue | 🟡 MinorAdd a non-intrusive focus-visible style to tag badges.
These badges are keyboard-focusable and should expose focus state for accessibility.
💡 Suggested tweak
- className="text-xs px-1 py-0 h-3.5 rounded-sm border-primary/30 bg-primary/10 text-primary hover:bg-primary/15 hover:text-primary transition-colors duration-200 cursor-pointer font-normal whitespace-normal break-words focus-visible:outline-none" + className="text-xs px-1 py-0 h-3.5 rounded-sm border-primary/30 bg-primary/10 text-primary hover:bg-primary/15 hover:text-primary transition-colors duration-200 cursor-pointer font-normal whitespace-normal break-words focus-visible:outline focus-visible:outline-2 focus-visible:outline-primary/60 focus-visible:outline-offset-2"As per coding guidelines, "Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility".
ui/src/components/ui/toast.tsx (1)
56-66:⚠️ Potential issue | 🟡 MinorMissing focus indicator on ToastClose button.
Removing
focus:ring-2leaves the close button without a visible focus indicator for keyboard users. Whilefocus:opacity-100makes the button visible, there's no visual distinction between "hovered state" and "focused state," which can confuse keyboard navigators.Consider adding a subtle focus indicator, such as
focus:ring-1 focus:ring-currentorfocus:text-foreground, to maintain accessibility without being intrusive.🛠️ Suggested fix
- 'absolute right-2 top-2 rounded-md p-1 text-foreground/50 opacity-0 transition-opacity hover:text-foreground focus:opacity-100 focus:outline-none group-hover:opacity-100', + 'absolute right-2 top-2 rounded-md p-1 text-foreground/50 opacity-0 transition-opacity hover:text-foreground focus:opacity-100 focus:outline-none focus:ring-1 focus:ring-current group-hover:opacity-100',As per coding guidelines: "Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility."
ui/src/features/dag-runs/components/dag-run-details/DAGRunHeader.tsx (1)
59-95:⚠️ Potential issue | 🟡 MinorMissing
useCallbackforhandleRefreshcauses effect to re-run on every render.The
handleRefreshfunction is in theuseEffectdependency array but is recreated on every render since it's not wrapped inuseCallback. This causes the event listener to be removed and re-added on every render, which is inefficient.🔧 Proposed fix
+ const handleRefresh = React.useCallback(() => { + setIsRefreshing(true); + refreshFn(); + setTimeout(() => setIsRefreshing(false), 600); + }, [refreshFn]); - const handleRefresh = () => { - setIsRefreshing(true); - refreshFn(); - setTimeout(() => setIsRefreshing(false), 600); - }; // Add keyboard shortcut for refresh useEffect(() => { // ... handler code - }, [handleRefresh]); + }, [handleRefresh]);ui/src/features/dags/components/chat-history/ChatHistoryTab.tsx (1)
78-83:⚠️ Potential issue | 🟡 MinorRestore a visible focus state for the step selector.
focus:outline-noneremoves the default outline without a replacement, so keyboard users lose a focus indicator.Proposed fix
- className="h-6 px-2 text-xs border rounded bg-card focus:outline-none" + className="h-6 px-2 text-xs border rounded bg-card focus:outline-none focus-visible:border-ring"As per coding guidelines: Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility.
ui/src/components/ui/switch.css (1)
16-18:⚠️ Potential issue | 🟠 MajorRestore a visible focus indicator for the switch.
:focus-visibleremoves the outline but doesn’t add a replacement, so keyboard users can’t see focus. Please add a visible focus style.🔧 Suggested fix
.switch-root:focus-visible { - outline: none; + outline: 2px solid var(--ring); + outline-offset: 2px; }ui/src/pages/api-keys/index.tsx (1)
140-205:⚠️ Potential issue | 🟡 MinorAdd long-text wrapping to key fields to prevent layout breaks.
Key names/descriptions and prefixes can be long; adding
whitespace-normal break-wordsto those cells will keep the table stable.🔧 Suggested fix
- <TableCell className="font-medium"> + <TableCell className="font-medium whitespace-normal break-words"> @@ - <span className="text-xs text-muted-foreground ml-5"> + <span className="text-xs text-muted-foreground ml-5 whitespace-normal break-words"> @@ - <TableCell> + <TableCell className="whitespace-normal break-words"> <code className="text-xs bg-muted px-1.5 py-0.5 rounded"> {key.keyPrefix}... </code> </TableCell>As per coding guidelines, Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout breaks.ui/src/components/ui/table.tsx (1)
80-88:⚠️ Potential issue | 🟡 MinorEnsure table cells wrap long content by default.
The shared TableCell class still lacks
whitespace-normal break-words, so long values can overflow the layout. Please add them to the base class so all tables inherit safe wrapping.Proposed fix
- className={cn('py-2 px-3 text-xs', className)} + className={cn('py-2 px-3 text-xs whitespace-normal break-words', className)}As per coding guidelines: Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout breaks.ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (2)
196-216:⚠️ Potential issue | 🟡 MinorMissing focus indicators on sort buttons reduces keyboard accessibility.
The sort buttons have
focus:outline-nonebut no replacement focus indicator. This makes it difficult for keyboard users to identify which element is focused.As per coding guidelines: "Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility."
🛡️ Proposed fix to add subtle focus indicators
<button type="button" onClick={() => handleSort('name')} - className="flex items-center gap-1 cursor-pointer select-none focus:outline-none rounded" + className="flex items-center gap-1 cursor-pointer select-none focus:outline-none focus-visible:ring-1 focus-visible:ring-ring rounded" aria-label="Sort by key" >Apply the same pattern to the "Sort by value" button on line 210.
231-254:⚠️ Potential issue | 🟡 MinorCopy button missing focus indicator; table row styling looks good.
The hover state (
hover:bg-muted/50) and text handling (whitespace-normal break-words) are well implemented. However, the copy button at line 243 hasfocus:outline-nonewithout a replacement focus indicator.🛡️ Proposed fix for copy button focus state
<button type="button" onClick={() => handleCopy(key, value)} - className="p-1 hover:bg-accent rounded focus:outline-none" + className="p-1 hover:bg-accent rounded focus:outline-none focus-visible:ring-1 focus-visible:ring-ring" title="Copy value" aria-label={`Copy value for ${key}`} >ui/src/components/ui/dialog.tsx (1)
56-61:⚠️ Potential issue | 🟡 MinorDialogClose button missing focus indicator.
The close button has
focus:outline-nonewith no replacement focus indicator. For a modal close button, a visible focus state is important for keyboard accessibility.As per coding guidelines: "Support keyboard navigation (arrows, enter, escape) and proper focus management in all modal components" and "Provide clear focus indicators (but not intrusive ones)."
🛡️ Proposed fix to add focus indicator
- <DialogPrimitive.Close className="absolute right-3 top-3 p-1.5 rounded-md opacity-70 transition-opacity hover:opacity-100 hover:bg-muted focus:outline-none disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground"> + <DialogPrimitive.Close className="absolute right-3 top-3 p-1.5 rounded-md opacity-70 transition-opacity hover:opacity-100 hover:bg-muted focus:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">ui/src/components/ui/tabs.tsx (1)
32-56:⚠️ Potential issue | 🟡 MinorTab component missing visible focus indicator for inactive tabs.
The Tab has
focus-visible:outline-nonewithout a replacement focus indicator. While active tabs haveborder-primaryas a visual indicator, inactive tabs withborder-transparenthave no visible focus state for keyboard users.As per coding guidelines: "Provide clear focus indicators (but not intrusive ones)."
🛡️ Proposed fix to add focus ring for tabs
const classes = cn( 'inline-flex items-center justify-center whitespace-nowrap h-12 px-4 text-sm font-medium relative', - 'transition-all duration-150 ease-in-out focus-visible:outline-none', + 'transition-all duration-150 ease-in-out focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-inset', 'disabled:pointer-events-none disabled:opacity-50', 'border-b-2',ui/src/menu.tsx (2)
252-258:⚠️ Potential issue | 🟡 MinorToggle button missing focus indicator.
The collapse sidebar button has hover states but lacks a visible focus indicator.
🛡️ Proposed fix
<button onClick={onToggle} - className="p-1.5 text-sidebar-foreground hover:text-foreground hover:bg-sidebar-hover rounded-md transition-all" + className="p-1.5 text-sidebar-foreground hover:text-foreground hover:bg-sidebar-hover focus-visible:ring-1 focus-visible:ring-ring rounded-md transition-all" aria-label="Collapse sidebar" >
261-281:⚠️ Potential issue | 🟡 MinorExpand sidebar button missing focus indicator.
Similar to the collapse button, the expand button lacks a visible focus state for keyboard users.
🛡️ Proposed fix
<button onClick={onToggle} className={cn( - 'w-9 h-9 flex items-center justify-center rounded-lg transition-all', + 'w-9 h-9 flex items-center justify-center rounded-lg transition-all focus-visible:ring-1 focus-visible:ring-ring', !customColor && 'hover:bg-sidebar-hover' )} aria-label="Expand sidebar" >ui/src/pages/search/index.tsx (1)
118-120:⚠️ Potential issue | 🟡 MinorIncorrect useEffect dependency on
ref.current.
ref.currentis a mutable value that doesn't trigger re-renders and shouldn't be used as a dependency. The effect runs once on mount anyway since refs are stable, but this is incorrect usage that could cause linting warnings.🛠️ Proposed fix
useEffect(() => { ref.current?.focus(); - }, [ref.current]); + }, []);ui/src/pages/git-sync/index.tsx (1)
376-381:⚠️ Potential issue | 🟡 MinorAdd focus indicator for filter tab buttons.
The filter tab buttons currently lack visible focus indicators. The
focus:outline-noneis present without a compensating focus-visible style, which impacts keyboard accessibility. As per coding guidelines, UI components should provide clear focus indicators.🔧 Proposed fix to add focus indicator
className={cn( - 'px-3 py-1.5 border-b-2 -mb-px transition-colors focus:outline-none', + 'px-3 py-1.5 border-b-2 -mb-px transition-colors focus:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-1', filter === f ? 'border-foreground text-foreground' : 'border-transparent text-muted-foreground hover:text-foreground' )}
🤖 Fix all issues with AI agents
In `@ui/src/components/TabBar.tsx`:
- Around line 29-37: The handleKeyDown handler currently only handles
Enter/Space and Delete/Backspace; update handleKeyDown to also handle ArrowRight
and ArrowLeft (optionally Home/End) to move focus and activate the next/previous
tab: on ArrowRight find the next tab element, focus it and call
setActiveTab(nextId), on ArrowLeft do the same for the previous tab. To enable
locating DOM nodes, add data-tab-id={tab.id} to each tab div in the TabBar
render so the handler can query the current element via
document.querySelector(`[data-tab-id="${tabId}"]`) and then compute
next/previous elements, set focus, and call setActiveTab; keep existing
preventDefault behavior and still support closeTab on Delete/Backspace.
In `@ui/src/components/ui/input.tsx`:
- Around line 12-17: Update the sizing classes on the Input component: replace
'h-9' with 'h-7' and change the vertical padding from 'py-2' to a compact value
like 'py-0.5' (or 'py-1') in the class list used by the Input component so the
control uses the compact form control sizing per UI guidelines; locate the class
string array in the Input component (input.tsx) and adjust those specific tokens
only.
In `@ui/src/features/system-status/components/PathsCard.tsx`:
- Around line 45-48: The PathsCard component's root button/container currently
strips focus styling via `focus:outline-none`, removing keyboard focus feedback;
update the className usage in PathsCard to replace `focus:outline-none` with a
non-intrusive keyboard-only indicator such as `focus-visible:ring-2
focus-visible:ring-offset-2 focus-visible:ring-accent/60
focus-visible:rounded-lg` (or a subtle `focus-visible:outline` alternative) so
keyboard users see a clear focus state while mouse interactions remain
unchanged.
In `@ui/src/menu.tsx`:
- Around line 121-147: Add an accessible focus indicator to the SidebarButton by
updating the root <button> className in function SidebarButton to include
focus-visible classes (e.g., focus:outline-none focus-visible:ring
focus-visible:ring-offset-2 focus-visible:ring-[color-or-token]) so keyboard
users see a clear ring on focus; keep the existing hover/bg and conditional
justify-center logic and ensure any color token (like ring-foreground or a
custom focus color) matches the design system.
In `@ui/src/pages/users/index.tsx`:
- Around line 182-183: The scroll container div currently using className
"card-obsidian overflow-auto" needs the min-h-0 utility to behave correctly in
the new flex-column layout; update the JSX that renders the div with className
"card-obsidian overflow-auto" (the container wrapping <Table />) to include
"min-h-0" so the card can shrink and allow internal scrolling when layout space
is constrained.
In `@ui/src/styles/global.css`:
- Around line 655-667: Remove the global "*:focus { outline: none; }" rule and
instead explicitly handle focus styles with :focus-visible for interactive
controls; update selectors like "input:focus-visible, textarea:focus-visible,
select:focus-visible" to also include "button:focus-visible, a:focus-visible,
[role='button']:focus-visible, [tabindex]:focus-visible" (or scope the removal
to purely decorative/non-interactive elements only) so that buttons, links and
custom controls retain a visible, non-intrusive focus indicator while keeping
form inputs styled as intended.
In `@ui/src/ui/ActionButton.tsx`:
- Around line 16-18: The baseStyles constant in ActionButton removes the default
outline ("outline-none") which can hide focus state; update the ActionButton
styles to preserve a visible, non-intrusive focus indicator instead of removing
it — remove or replace "outline-none" in baseStyles and add appropriate
focus-visible classes (e.g., a subtle ring and offset) so keyboard focus is
clearly visible; ensure the ActionButton component and its variants use those
focus-visible styles and that contrast remains sufficient at small text sizes.
In `@ui/src/ui/CustomDialog.tsx`:
- Around line 27-30: The Close button (DialogPrimitive.Close) currently removes
focus styling by using focus:outline-none only, so add accessible focus-visible
ring styles to restore keyboard focus indication: update the className on
DialogPrimitive.Close to include focus-visible or focus classes such as
focus-visible:ring-2, focus-visible:ring-ring and focus-visible:ring-offset-2
(or the project's equivalent tokens) while keeping focus:outline-none if
desired; ensure the visually-hidden label (<span
className="sr-only">Close</span>) remains and verify the X icon (component X)
still renders correctly.
🧹 Nitpick comments (7)
ui/src/features/queues/components/QueueCard.tsx (1)
181-185: Selected state may be too subtle without a border/background.With the ring removed, the selected card relies only on a soft shadow, which can be hard to distinguish in light and dark themes. Consider adding a subtle border or background tint for clearer selection contrast, or at least verify visibility in both themes.
✅ Optional tweak for clearer selection state
- isSelected && - 'shadow-[0_0_20px_rgba(var(--primary-rgb),0.1)]' + isSelected && + 'shadow-[0_0_20px_rgba(var(--primary-rgb),0.1)] border border-primary/20 dark:border-primary/30'As per coding guidelines: Maintain sufficient color contrast in both light and dark modes for accessibility.
ui/src/pages/audit-logs/index.tsx (1)
549-574: Consider allowing text wrapping in the Details column.The Details column uses
truncate(line 567) which clips long text. While thetitleattribute provides hover access to full content, this may not be accessible to keyboard users or touch devices.As per coding guidelines: "Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout breaks."♻️ Optional: Allow wrapping for better accessibility
- <td className="px-4 py-3 text-sm text-muted-foreground truncate" title={entry.details}> + <td className="px-4 py-3 text-sm text-muted-foreground whitespace-normal break-words max-w-[300px]" title={entry.details}> {formatDetails(entry)} </td>Alternatively, keep
truncatebut add a max-width to prevent excessive column growth, which maintains the current behavior while being more explicit about constraints.ui/src/menu.tsx (1)
60-79: Redundant conditional logic in style helper functions.
getActiveLinkStyleandgetIconWrapperStylereturn the same value regardless of thecustomColorparameter:// Lines 67-71: Both branches return 'bg-sidebar-active' function getActiveLinkStyle(customColor: boolean): string { return customColor ? 'bg-sidebar-active' : 'bg-sidebar-active'; } // Lines 77-78: Both branches return 'text-sidebar-foreground' function getIconWrapperStyle(customColor: boolean): string { return customColor ? 'text-sidebar-foreground' : 'text-sidebar-foreground'; }This appears to be either incomplete implementation or dead code from refactoring.
♻️ Simplify or differentiate the functions
If the styles should be the same regardless of
customColor, simplify:function getActiveLinkStyle(customColor: boolean): string { - return customColor - ? 'bg-sidebar-active' - : 'bg-sidebar-active'; + return 'bg-sidebar-active'; } function getIconWrapperStyle(customColor: boolean): string { - return customColor ? 'text-sidebar-foreground' : 'text-sidebar-foreground'; + return 'text-sidebar-foreground'; }Or, if they should differ for
customColor, implement the actual differentiation.ui/src/styles/global.css (1)
773-779: Duplicate utility class definitions.
text-balanceandtracking-tightare already defined at lines 415-420. These duplicate definitions are redundant.♻️ Proposed fix: Remove duplicate definitions
-/* Utility Classes */ -.text-balance { - text-wrap: balance; -} - -.tracking-tight { - letter-spacing: -0.02em; -} - .tracking-wide { letter-spacing: 0.5px; }ui/src/pages/dag-runs/dag-run/index.tsx (1)
131-131: Width constraint applied, but error display has inconsistent styling.The main content container now uses
max-w-7xl px-4, but theErrorDisplaycomponent at line 35 still usesw-full px-4. This creates visual inconsistency between error and success states.♻️ Suggested fix for consistency
function ErrorDisplay({ error, name, dagRunId }: ErrorDisplayProps) { // ... return ( - <div className="w-full px-4"> + <div className="max-w-7xl px-4"> <div className={`${containerClass} rounded-lg p-6 m-4`}>ui/src/layouts/Layout.tsx (2)
143-149: Add focus indicator to mobile menu button.The mobile menu button has hover styling but lacks a visible focus indicator for keyboard navigation. As per coding guidelines, buttons should provide clear focus indicators.
🔧 Proposed fix
<button - className="p-2 rounded-md hover:bg-muted transition-colors" + className="p-2 rounded-md hover:bg-muted transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" onClick={() => setIsMobileSidebarOpen(true)} aria-label="Open menu" >
191-196: Add focus indicator to mobile close button.Similar to the menu button, the close button in the mobile overlay lacks focus-visible styling.
🔧 Proposed fix
<button onClick={() => setIsMobileSidebarOpen(false)} - className="p-1.5 hover:bg-sidebar-hover rounded-md transition-colors" + className="p-1.5 hover:bg-sidebar-hover rounded-md transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" >
Summary by CodeRabbit
Release Notes
New Features
Style
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.