Skip to content

Conversation

@yottahmd
Copy link
Collaborator

@yottahmd yottahmd commented Jan 17, 2026

Summary by CodeRabbit

  • New Features

    • Added dark mode support with theme toggle in navigation
    • Implemented confirmation modals for critical toggle actions (webhooks, live switch)
    • Added single-click node interactions in graph visualization
  • Improvements

    • Redesigned UI components (cards, dialogs, tabs, buttons) with modern styling
    • Enhanced toast notifications appearance
    • Improved graph and timeline visualizations with theme awareness
    • Terminal styling now adapts to selected theme
    • Migrated color system to CSS variable-based tokens
  • Documentation

    • Expanded dialog component exports for better usability

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 17, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

Introduces comprehensive dark mode theming by adding a theme field to user preferences, implementing CSS variable-based styling system replacing Tailwind config, and updating numerous UI components across the application to support light and dark mode rendering. Additionally refactors layout responsiveness, adds confirmation flows for toggle operations, and updates component APIs.

Changes

Cohort / File(s) Summary
Theme & Styling Infrastructure
ui/index.html, ui/src/styles/global.css, ui/tailwind.config.js, ui/src/contexts/UserPreference.tsx, ui/src/App.tsx
Complete theming system overhaul: moved color tokens from Tailwind config to CSS variables, added dark mode variant, introduced theme field to UserPreferences, added theme initialization and document root styling in App component, and updated HTML root with dark class attribute.
Core UI Component Library
ui/src/components/ui/{dialog, tabs, switch, toggle-group, table, simple-toast}.tsx, ui/src/components/ui/{switch, TabBar}.css
Updated UI component styling and APIs: expanded dialog exports, added asChild prop to Tab, redesigned TabBar with flatter appearance, reworked Switch styling via CSS, unified simple-toast color scheme, updated toggle-group appearance by removing position-based logic.
Layout & Navigation
ui/src/layouts/Layout.tsx, ui/src/menu.tsx, ui/src/components/SplitLayout.tsx
Restructured responsive layout with sidebar/main content separation and mobile overlay, reorganized menu navigation with NavItem component abstraction and theme toggle, enhanced SplitLayout divider with improved dragging visuals and z-index management.
Status & Display Components
ui/src/ui/{StatusChip.tsx}, ui/src/features/dags/components/common/{NodeStatusChip, StatusDot}.tsx, ui/src/consts.ts
Consolidated status color mapping from individual variables to CSS variable tokens and status-class scheme, refactored StatusDot to use helper function, simplified NodeStatusChip styling logic.
DAG Visualization & Interactions
ui/src/features/dags/components/visualization/{Graph, TimelineChart}.tsx, ui/src/ui/Mermaid.tsx
Added theme-aware color rendering for graph and timeline components, introduced onClick handler for single-click node interactions in Mermaid with double-click/right-click cancellation logic.
DAG Details & Metadata
ui/src/features/dags/components/dag-details/{DAGHeader, NodeStatusTableRow, WebhookTab}.tsx, ui/src/features/dag-runs/components/dag-run-details/{DAGRunDetailsModal, DAGRunHeader}.tsx
Added confirmation modal flow for webhook toggle, updated styling to use CSS variables, reformatted conditionals and data selection logic, enhanced border styling with color tokens.
DAG Navigation & Lists
ui/src/features/dags/components/dag-list/DAGTable.tsx, ui/src/features/dags/components/common/{LinkTab, ModalLinkTab, LiveSwitch}.tsx, ui/src/pages/dags/index.tsx
Added asChild prop usage to Tab components in LinkTab/ModalLinkTab, introduced confirmation flow for LiveSwitch toggle, updated DAGTable styling to card-obsidian, added left panel padding adjustments.
DAG Editor & Schema
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/{PropertyTypeDisplay, SchemaPathBreadcrumb}.tsx
Replaced hardcoded color mapping with status-class tokens for type badges and array indices, streamlined styling logic.
Chat History
ui/src/features/dags/components/chat-history/{ChatHistoryTab, StepMessagesTable}.tsx
Reorganized imports, updated styling from bg-white to bg-card with primary focus rings, minor text formatting adjustments.
Queue Management
ui/src/features/queues/components/{QueueCard, QueueList}.tsx
Updated container styling to card-obsidian with hover effects, removed auto-selection of first queue item, added bounds checking for selected index.
Admin Pages: API Keys, Audit Logs, Users
ui/src/pages/{api-keys, audit-logs, users}/index.tsx
Added dropdown menus for row actions, integrated ConfirmModal/FormModal components, refactored API calls for clarity, updated container styling to card-obsidian, introduced explicit TableCell wrappers.
Advanced Admin Pages: Audit Logs, Webhooks
ui/src/pages/{audit-logs, webhooks}/index.tsx
Refactored audit-logs with date helper functions and timezone-aware formatting, added webhook toggle confirmation flow with dynamic messaging, extended import sets for context and utilities.
Other Pages
ui/src/pages/{system-status, terminal, workers}/index.tsx, ui/src/features/{system-status, dashboard}/components/{PathsCard.tsx, DashboardTimechart.tsx}
Updated system-status chart colors to unified hex value, restructured terminal with theme-aware xterm configuration, refactored dashboard timeline with explicit options object and time-axis management, converted PathsDialog to internal-state PathsCard using Dialog component.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~90 minutes

Possibly related PRs

  • feat(core): waiting status #1554: Both PRs modify the same UI status-related components (StatusChip, NodeStatusChip, dialog.tsx) and update status color/styling logic with CSS variable tokens and refactored classification schemes.
  • feat: system status page #1572: Both PRs update App.tsx routing structure, menu navigation exports, and wire System Status feature into the main application layout and navigation.
  • feat(ui): add spec tab to status details #1553: Both PRs extend ui/src/ui/Mermaid.tsx with single-click node handling (onClick prop) alongside existing double-click and right-click handlers.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add darkmode ui again' accurately reflects the primary objective of the PR, which implements dark mode UI styling across multiple components and adds theme support throughout the application.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
ui/src/pages/audit-logs/index.tsx (1)

537-556: Loading state hides content instead of showing stale data.

As per coding guidelines, avoid hiding content during loading—show stale data while fetching updates instead. Currently, when isLoading is true, all entries are replaced with "Loading audit logs..." message.

Consider showing the previous entries with a subtle loading indicator (e.g., reduced opacity or a small spinner in the header).

Suggested approach
 <TableBody>
-  {isLoading ? (
-    <TableRow>
-      <TableCell
-        colSpan={6}
-        className="text-center text-muted-foreground py-8"
-      >
-        Loading audit logs...
-      </TableCell>
-    </TableRow>
-  ) : entries.length === 0 ? (
+  {entries.length === 0 && !isLoading ? (
     <TableRow>
       <TableCell
         colSpan={6}
         className="text-center text-muted-foreground py-8"
       >
         <ScrollText className="h-8 w-8 mx-auto mb-2 opacity-50" />
         No audit log entries found
       </TableCell>
     </TableRow>
   ) : (
-    entries.map((entry) => (
+    entries.map((entry) => (
-      <TableRow key={entry.id}>
+      <TableRow key={entry.id} className={isLoading ? 'opacity-50' : ''}>

Also add a loading indicator in the header or refresh button:

 <Button
   onClick={() => fetchAuditLogs()}
   size="sm"
   variant="outline"
-  className="h-8"
+  className="h-8"
+  disabled={isLoading}
 >
-  <RefreshCw className="h-4 w-4" />
+  <RefreshCw className={`h-4 w-4 ${isLoading ? 'animate-spin' : ''}`} />
 </Button>
ui/src/pages/workers/index.tsx (1)

221-230: Avoid adding a new history entry when closing the modal.
Line 229 uses pushState, which appends a new history entry; the back button can then land on a URL with query params while the modal remains closed. Prefer replaceState for cleanup.

🩹 Suggested fix
-            window.history.pushState({}, '', window.location.pathname);
+            window.history.replaceState({}, '', window.location.pathname);
ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (1)

182-209: Hardcoded colors in InlineLogViewer are inconsistent with dark mode theming.

The InlineLogViewer uses hardcoded Tailwind slate colors (bg-slate-800, border-slate-700, text-slate-100, text-slate-400, text-slate-500) which won't adapt to the CSS variable-based theming system used elsewhere in this PR. Consider using semantic tokens for consistency.

♻️ Suggested fix using semantic tokens
-    <div className="bg-slate-800 rounded overflow-hidden border border-slate-700">
+    <div className="bg-card rounded overflow-hidden border border-border">
       {isLoading && !data ? (
-        <div className="text-slate-400 text-xs py-4 px-3">Loading logs...</div>
+        <div className="text-muted-foreground text-xs py-4 px-3">Loading logs...</div>
       ) : lines.length === 0 ? (
-        <div className="text-slate-400 text-xs py-4 px-3">
+        <div className="text-muted-foreground text-xs py-4 px-3">
           &lt;No log output&gt;
         </div>
       ) : (
         <div className="overflow-x-auto max-h-[400px] overflow-y-auto">
-          <pre className="font-mono text-[11px] text-slate-100 p-2">
+          <pre className="font-mono text-[11px] text-foreground p-2">
             {lines.map((line, index) => {
               const lineNumber = totalLines - lineCount + index + 1;
               return (
                 <div key={index} className="flex px-1 py-0.5">
-                  <span className="text-slate-500 mr-3 select-none w-12 text-right flex-shrink-0">
+                  <span className="text-muted-foreground mr-3 select-none w-12 text-right flex-shrink-0">
                     {lineNumber}
                   </span>
ui/src/pages/terminal/index.tsx (1)

92-222: Terminal recreation on theme change may disrupt active sessions.

Adding theme to the effect's dependency array (line 222) causes the entire terminal and WebSocket connection to be torn down and recreated whenever the user toggles the theme. This will disconnect any active shell session, losing command history and state.

Consider applying theme changes to the existing terminal instance instead:

Proposed approach: separate theme update effect
+  // Update terminal theme without recreating
+  useEffect(() => {
+    if (terminalRef.current) {
+      terminalRef.current.options.theme = {
+        background: theme === 'dark' ? '#0f1129' : '#ffffff',
+        foreground: theme === 'dark' ? '#f1f5f9' : '#020617',
+        cursor: theme === 'dark' ? '#f1f5f9' : '#020617',
+        cursorAccent: theme === 'dark' ? '#0f1129' : '#ffffff',
+        selectionBackground: theme === 'dark' ? '#1c224d' : '#add6ff',
+      };
+    }
+  }, [theme]);

   useEffect(() => {
     if (!termRef.current || !isAdmin || !config.terminalEnabled) return;
     // ... terminal initialization ...
-  }, [config.basePath, config.terminalEnabled, isAdmin, theme]);
+  }, [config.basePath, config.terminalEnabled, isAdmin]);
ui/src/features/dashboard/components/DashboardTimechart.tsx (1)

249-252: initialViewRef is used before it's declared.

The ref is assigned at line 249 inside the first useEffect, but the declaration occurs at line 402. While React hooks are called in order during render (before effects run), placing the declaration after its usage in effects makes the code harder to follow and maintain.

Move the declaration before the first useEffect that uses it:

♻️ Suggested reordering
  const [isModalOpen, setIsModalOpen] = useState(false);
+ const initialViewRef = useRef<{ start: Date; end: Date } | null>(null);

  // Helper function to ensure we have a valid IANA timezone

And remove the declaration at line 402:

  const handleCloseModal = () => {
    setIsModalOpen(false);
  };

- const initialViewRef = useRef<{ start: Date; end: Date } | null>(null);

  const handleZoomIn = () => {

Also applies to: 402-402

🤖 Fix all issues with AI agents
In `@ui/src/App.tsx`:
- Around line 87-91: The useEffect that checks remoteNodes vs selectedRemoteNode
calls handleSelectRemoteNode but doesn't include it in the dependency array;
update the effect to either inline the selection logic or add
handleSelectRemoteNode to deps by ensuring handleSelectRemoteNode is stable
(wrap it with useCallback) and then include it in the dependency array of the
useEffect that references remoteNodes and selectedRemoteNode so React's
exhaustive-deps rule is satisfied.

In `@ui/src/components/ui/simple-toast.tsx`:
- Around line 78-86: The success checkmark uses the theme CSS variable
(--success / `#10b981`) which fails WCAG contrast in light mode; update
simple-toast.tsx (the SimpleToast component and the checkmark SVG/icon) to use a
theme-aware color that meets 4.5:1 in light mode—either adjust the --success
variable to a darker accessible value for light theme or override the icon color
in light mode to a darker success (or text-foreground) value via a CSS variable
or conditional class (keep bg-popover and other theme vars unchanged). Ensure
the change targets the checkmark/icon element and the theme variable name
(--success) so both light and dark modes remain visually consistent and
accessible.

In `@ui/src/components/ui/tabs.tsx`:
- Around line 40-46: When asChild is true the component returns a bare <span>
which lacks interactive semantics and won't receive keyboard focus or disabled
styling; update the asChild branch in the Tabs component (the block that returns
the span using classes, props, children) to either render an element with proper
semantics (e.g., role="button" and tabIndex={0}) and forward the disabled state
via aria-disabled or class, or document/require that asChild is only used to
wrap inherently interactive children like React Router <Link>; ensure you update
the returned element to include role="button", tabIndex={0}, and aria-disabled
when appropriate so keyboard users can interact, and keep props and children
forwarded unchanged.

In `@ui/src/components/ui/toggle-group.tsx`:
- Around line 104-105: The hover background 'hover:bg-white/10' in the
conditional class string inside the ToggleGroup component produces low contrast
in light mode; update the ternary branch that currently contains
"'text-muted-foreground hover:bg-white/10 hover:text-foreground
active:scale-95'" to use a theme-aware hover background (e.g., a light-mode
hover token and a dark-mode hover token) so the hover state remains visible in
both modes, keeping the rest of the class sequence (hover text, active scale)
intact; look for the class string in the ToggleGroup render/return where that
ternary is defined and replace only the hover:bg value with a combined
light/dark-friendly token.

In `@ui/src/features/dags/components/common/NodeStatusChip.tsx`:
- Around line 46-85: The NodeStatusChip component switch on NodeStatus (cases
NotStarted, Skipped → status-info; PartialSuccess, Waiting, Aborted →
status-warning; default → status-muted) references three missing CSS classes;
add CSS definitions for .status-info, .status-warning, and .status-muted in the
global stylesheet so the chip has appropriate background/border/text colors and
contrast consistent with existing .status-success, .status-failed, and
.status-running styles (match naming and visual weight used by those classes to
ensure consistent UI across NodeStatusChip).

In
`@ui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsx`:
- Around line 9-20: The project references CSS classes used by typeColors in
PropertyTypeDisplay and by StatusChip/NodeStatusChip (status-info,
status-warning, status-muted) but they are not defined; add matching class
definitions to global.css following the pattern used for
status-success/status-failed so the Tailwind tokens (--info, --warning, --muted)
are applied. Create .status-info, .status-warning, and .status-muted entries
using the same structure as the existing status-success class (bg-<token>/10,
text-<token>, border border-<token>/20) so components like PropertyTypeDisplay,
NodeStatusChip and StatusChip render correctly.

In `@ui/src/features/dashboard/components/DashboardTimechart.tsx`:
- Around line 647-652: The box-shadow RGB variable --primary-rgb is only defined
in the .dark theme, so in light mode the shadows on .vis-item.vis-selected and
.vis-current-time won’t render; fix this by adding a fallback RGB definition to
the global scope (add --primary-rgb to the :root block in global.css) or modify
the CSS here to use a fallback (e.g. use var(--primary-rgb, <fallback-rgb>)) so
the box-shadow declarations referencing --primary-rgb always have a value.

In `@ui/src/features/queues/components/QueueCard.tsx`:
- Around line 181-185: The inline hover utility classes inside the QueueCard
component's className (the cn(...) expression that includes
'hover:bg-white/[0.05] hover:border-white/10' alongside 'card-obsidian') are
overriding the global card-obsidian styles and break light/dark behavior; remove
those hover utilities or change them to dark-prefixed variants
(dark:hover:bg-white/[0.05] dark:hover:border-white/10) so the card-obsidian CSS
in global.css controls default behavior, and update the className expression in
QueueCard accordingly.

In `@ui/src/menu.tsx`:
- Around line 52-55: NavItem currently sets isActive with
location.pathname.startsWith(to) which produces false positives (e.g., "/dags"
matches "/dag-runs"); update NavItem's active-check logic (inside the NavItem
component where useLocation() and isActive are computed) to use exact matching
for leaf routes and prefix matching only for parent routes — for example, treat
a route as active if location.pathname === to or location.pathname.startsWith(to
+ '/') for routes that are parents, or add a boolean prop (e.g., hasChildren) to
decide which check to use so leaf routes use strict equality and parent routes
use the prefix-with-separator check.
- Around line 64-67: The light theme is missing the --primary-rgb CSS variable
used unconditionally by the NavItem shadow in ui/src/menu.tsx; add the variable
to the :root theme block in ui/src/styles/global.css by defining --primary-rgb:
234 88 12; so rgba(var(--primary-rgb),...) works in light mode (this complements
the existing .dark --primary-rgb: 99 102 241).

In `@ui/src/styles/global.css`:
- Around line 100-105: The :root block is missing the --primary-rgb variable
used by components in light mode; add a --primary-rgb entry inside :root
(alongside --bg, --text, --sidebar-bg, --sidebar-text) and set it to the
light-mode primary RGB value (matching the RGB used in .dark's --primary-rgb or
the project's light primary color) so rgba(var(--primary-rgb), ...) calls work
in light theme (look for :root and .dark and the --primary-rgb variable).

In `@ui/src/ui/Mermaid.tsx`:
- Around line 144-148: The clickTimeouts Map is created inside render() so
pending timeouts can be lost on re-renders; move timeout management into a
persistent ref (e.g., const clickTimeoutsRef = useRef(new Map<string,
ReturnType<typeof setTimeout>>())) used by the click handling logic in the
Mermaid component, ensure you always read/write clickTimeoutsRef.current, clear
any existing timeout before setting a new one, and add cleanup on unmount (or
when def changes) to clear and delete all timeouts from clickTimeoutsRef.current
to avoid leaks.
🧹 Nitpick comments (23)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsx (1)

35-38: Keep breadcrumb segment styling consistent and unobtrusive.

status-warning likely adds emphasis (color/background) that can break uniform metadata styling and make navigation too prominent. Consider keeping the same muted background for array indices and differentiate with font/weight only.

♻️ Proposed adjustment
-              segment.isArrayIndex ? 'font-mono status-warning' : 'bg-muted'
+              segment.isArrayIndex ? 'font-mono bg-muted' : 'bg-muted'

Based on learnings, ...

ui/src/features/dags/components/chat-history/StepMessagesTable.tsx (1)

201-202: Minor readability: Consider adding space after colons for consistency.

The token display formatting removes spacing after colons (in: and out:). While functional, adding a space (in: and out: ) would improve readability and follow common formatting conventions.

✨ Optional formatting improvement
-                          in:{msg.metadata.promptTokens} out:
-                          {msg.metadata.completionTokens}
+                          in: {msg.metadata.promptTokens} out: {msg.metadata.completionTokens}
ui/src/pages/audit-logs/index.tsx (1)

155-170: Consider validating the parsed date before conversion.

If dateString is malformed, dayjs(dateWithSeconds) returns an invalid date object, and toISOString() will return "Invalid Date" which may cause API errors or unexpected behavior.

Suggested validation
 const formatDateForApi = useCallback(
   (dateString: string | undefined): string | undefined => {
     if (!dateString) return undefined;
     // Add seconds if missing
     const dateWithSeconds =
       dateString.split(':').length < 3 ? `${dateString}:00` : dateString;
+    const parsed = dayjs(dateWithSeconds);
+    if (!parsed.isValid()) return undefined;
     // Apply timezone offset and convert to ISO string
     if (config.tzOffsetInSec !== undefined) {
-      return dayjs(dateWithSeconds)
+      return parsed
         .utcOffset(config.tzOffsetInSec / 60, true)
         .toISOString();
     }
-    return dayjs(dateWithSeconds).toISOString();
+    return parsed.toISOString();
   },
   [config.tzOffsetInSec]
 );
ui/src/pages/workers/index.tsx (1)

26-39: Confirm the 1s polling cadence is acceptable at scale.
Line 36’s refreshInterval: 1000 may be costly under many concurrent clients; please confirm expected load or consider adaptive polling (e.g., visibility-aware or longer intervals) if this endpoint is heavy.

ui/src/components/ui/switch.css (1)

24-42: Consider using CSS variables instead of hardcoded hex colors.

The unchecked background colors use hardcoded hex values (#cbd5e1, #334155) while the checked state uses var(--primary). For consistency with the theme-driven approach established in global.css, consider using CSS variables for all colors.

♻️ Suggested refactor
 /* Light mode - unchecked */
 .switch-root {
-  background-color: `#cbd5e1`; /* slate-300 */
+  background-color: var(--muted);
 }

 /* Dark mode - unchecked */
 .dark .switch-root {
-  background-color: `#334155`; /* slate-700 */
+  background-color: var(--muted);
 }
ui/src/features/queues/components/QueueCard.tsx (1)

314-318: Spinning trash icon may confuse users.

Changing the animation from animate-pulse to animate-spin on the Trash2 icon during clearing is unconventional. A spinning trash can may suggest "refreshing" rather than "deleting in progress." Consider keeping animate-pulse or replacing the icon with a dedicated loading spinner during the operation.

ui/src/features/dags/components/visualization/TimelineChart.tsx (1)

114-116: Fallback color uses hardcoded hex instead of CSS variable.

The statusColors map correctly uses CSS variables, but the fallback in getStatusColor still uses a hardcoded hex value #6b7280. For consistency with the theming system, consider using a CSS variable.

♻️ Suggested fix
 function getStatusColor(status: NodeStatus): { bg: string; border: string } {
-  return statusColors[status] || { bg: '#6b7280', border: '#6b7280' };
+  return statusColors[status] || { bg: 'var(--muted)', border: 'var(--muted-foreground)' };
 }
ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsx (1)

198-198: Consider adding a semi-transparent backdrop for better modal visibility.

The backdrop div has no visual styling, making it invisible. Users may not realize they can click outside to close the modal. Consider adding a semi-transparent background:

-      <div className="fixed inset-0 h-screen w-screen z-40" onClick={onClose} />
+      <div className="fixed inset-0 h-screen w-screen z-40 bg-black/20" onClick={onClose} />
ui/src/pages/users/index.tsx (1)

273-288: Consider adding confirmation for the disable action.

The enable/disable toggle directly calls handleToggleDisabled without confirmation. While enabling is generally safe, disabling a user is a potentially disruptive action. The PR summary mentions "confirmation flows for toggle operations" — consider adding a confirmation modal for the disable action specifically.

💡 Suggested approach
+ const [togglingUser, setTogglingUser] = useState<User | null>(null);

  // In the dropdown menu:
  <DropdownMenuItem
-   onClick={() => handleToggleDisabled(user)}
+   onClick={() => user.isDisabled ? handleToggleDisabled(user) : setTogglingUser(user)}
  >

  // Add confirmation modal:
+ <ConfirmModal
+   title={`Disable User`}
+   buttonText="Disable"
+   visible={!!togglingUser}
+   dismissModal={() => setTogglingUser(null)}
+   onSubmit={() => {
+     if (togglingUser) handleToggleDisabled(togglingUser);
+     setTogglingUser(null);
+   }}
+ >
+   <p>Are you sure you want to disable user &quot;{togglingUser?.username}&quot;?</p>
+ </ConfirmModal>
ui/src/components/ui/toggle-group.tsx (1)

85-85: Remove unused position prop.

The position prop is declared in ToggleButtonProps but is no longer used in the component implementation. Per the AI summary, the position-based border radius logic was removed.

🧹 Suggested cleanup
 type ToggleButtonProps = {
   value: string;
   groupValue?: string;
   onClick?: () => void;
   children: React.ReactNode;
   className?: string;
   'aria-label'?: string;
-  position?: 'first' | 'middle' | 'last' | 'single';
 };
ui/src/components/TabBar.css (1)

51-58: Consider using a CSS variable for the shadow.

The box-shadow uses a hardcoded rgba(0, 0, 0, 0.1) value. For consistency with the CSS variable-based theming system, consider using a shadow variable that can adapt to light/dark modes.

-.tab-trigger[data-state='active'] {
-  box-shadow: 0 -4px 12px -4px rgba(0, 0, 0, 0.1);
+.tab-trigger[data-state='active'] {
+  box-shadow: 0 -4px 12px -4px var(--shadow-color, rgba(0, 0, 0, 0.1));
 }
ui/src/pages/system-status/index.tsx (1)

217-242: Consider per-metric chart tokens for faster scanning.

Using the same color for all metrics makes charts harder to distinguish quickly. Consider CSS tokens (e.g., var(--chart-cpu), var(--chart-memory)) or subtle hue/brightness shifts per metric.

ui/src/contexts/UserPreference.tsx (1)

5-42: Sanitize theme values loaded from storage.

LocalStorage can be tampered with or contain stale values. Consider validating theme to 'light' | 'dark' before applying it.

♻️ Suggested guard
-      return saved ? { ...defaultPrefs, ...JSON.parse(saved) } : defaultPrefs;
+      const parsed = saved ? JSON.parse(saved) : {};
+      const merged = { ...defaultPrefs, ...parsed };
+      return merged.theme === 'light' || merged.theme === 'dark'
+        ? merged
+        : { ...merged, theme: 'dark' };
ui/src/consts.ts (1)

4-7: Stale comment: update to reflect CSS variables.

The comment mentions "Using Tailwind color palette for consistency" but the implementation now uses CSS custom properties (var(--muted), var(--success), etc.). Consider updating to reflect the current approach.

📝 Suggested comment update
 /**
- * Status color mappings - Professional gray theme
- * Using Tailwind color palette for consistency
+ * Status color mappings
+ * Using CSS custom properties for theme support
  */
ui/src/components/SplitLayout.tsx (1)

139-151: Enhanced divider with theme-aware styling.

The draggable divider now uses CSS variable-based colors and shadow effects for both hover and active states. The z-30 ensures proper layering.

One minor consideration: transition-all on lines 139 and 145 may trigger more properties than needed. For a more performant animation, consider transitioning only the specific properties that change.

🎨 Optional: Scope transitions to specific properties
-        className={`hidden md:flex items-center justify-center w-[1px] h-full bg-border/20 hover:bg-primary/50 hover:shadow-[0_0_10px_var(--color-primary)] cursor-col-resize flex-shrink-0 transition-all duration-300 group z-30 ${
+        className={`hidden md:flex items-center justify-center w-[1px] h-full bg-border/20 hover:bg-primary/50 hover:shadow-[0_0_10px_var(--color-primary)] cursor-col-resize flex-shrink-0 transition-[background-color,box-shadow] duration-300 group z-30 ${
ui/src/pages/dags/index.tsx (1)

369-376: Complex height calculation with negative margins.

The negative margin pattern (-my-4 md:-my-6 lg:-my-8) combined with calc(100%+Xrem) heights is a workaround for inherited padding. While functional, this creates a maintenance burden if parent padding changes.

Consider documenting why this pattern is necessary, or refactoring the parent container to avoid the need for compensation.

ui/src/features/dags/components/visualization/Graph.tsx (1)

248-280: Theme-aware node styling with hardcoded stroke colors.

The node fills and text colors correctly adapt to isDarkMode, but stroke colors remain hardcoded hex values (e.g., #10b981 for success, #ef4444 for error). For full theme consistency, consider extracting these to CSS variables as well. Based on learnings, consistent styling with uniform color sources is preferred.

ui/src/layouts/Layout.tsx (2)

10-56: Consider memoizing contrast color computation.

The getContrastColor function creates and removes DOM elements to compute colors from non-hex values. While this works, it could cause minor layout thrashing if called frequently. Consider memoizing the result or computing it only when navbarColor changes.

💡 Optional: Memoize contrast color
+const contrastColorCache = new Map<string, string>();
+
 function getContrastColor(input?: string): string {
   if (!input) return '#000';
+  
+  if (contrastColorCache.has(input)) {
+    return contrastColorCache.get(input)!;
+  }

   let hex = input.trim();
   // ... rest of function
+  
+  contrastColorCache.set(input, result);
+  return result;
 }

187-195: Potential overflow issue in mobile sidebar.

The container uses h-full but also has pt-2 padding. Combined with the header above, this could cause the navigation to overflow. Consider using flex-1 overflow-y-auto on the nav container to handle long navigation lists.

♻️ Suggested fix
-            <div className="flex flex-col h-full pt-2">
-              <nav className="flex-1 px-3">
+            <div className="flex flex-col flex-1 min-h-0 pt-2">
+              <nav className="flex-1 overflow-y-auto px-3">
ui/src/ui/StatusChip.tsx (1)

53-58: Minor: Redundant font-bold declaration.

Both the container (line 53) and the inner span (line 58) have font-bold. Consider removing one for clarity.

🧹 Optional cleanup
-      <span className="font-bold break-keep text-nowrap whitespace-nowrap">
+      <span className="break-keep text-nowrap whitespace-nowrap">
ui/src/App.tsx (1)

93-103: Consider using CSS variables instead of hardcoded background colors.

The inline backgroundColor style uses hardcoded hex values that may drift from your CSS theme definitions. Since you're already adding/removing the dark class, let CSS handle the background via variables.

Proposed simplification
   React.useEffect(() => {
     const root = document.documentElement;
     if (theme === 'dark') {
       root.classList.add('dark');
-      root.style.backgroundColor = '#020617';
     } else {
       root.classList.remove('dark');
-      root.style.backgroundColor = '#ffffff';
     }
   }, [theme]);

Then ensure your global CSS defines:

:root { background-color: `#ffffff`; }
.dark { background-color: `#020617`; }
ui/src/components/ui/dialog.tsx (1)

49-51: Dialog border uses hardcoded indigo accent.

The border-indigo-500/30 and shadow-indigo-500/10 are fixed colors that won't adapt if the theme's accent color changes. This is acceptable if indigo is the intentional brand accent, but consider using CSS variables like border-primary/30 if the accent should be themeable.

ui/src/styles/global.css (1)

205-214: Hardcoded shadow color doesn't match light mode primary.

The box-shadow uses rgba(99, 102, 241, ...) which is the dark mode indigo primary. In light mode, the primary is orange (#ea580c), so the shadow glow color will be inconsistent with the button's gradient.

Consider using CSS variables or defining separate light/dark shadows if the glow should match the primary color in each theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants