fix: complete theme token system with WCAG-compliant color tokens#370
Open
openasocket wants to merge 26 commits intoRunMaestro:mainfrom
Open
fix: complete theme token system with WCAG-compliant color tokens#370openasocket wants to merge 26 commits intoRunMaestro:mainfrom
openasocket wants to merge 26 commits intoRunMaestro:mainfrom
Conversation
Replace hardcoded #fff, #000, and 'white' color values with theme tokens (accentForeground, bgMain) across 15 component files. This ensures proper contrast ratios on accent-colored backgrounds for all themes, including high-contrast and vibe themes like dre-synth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add info, status foregrounds (successForeground, warningForeground, errorForeground), dimmed status backgrounds (successDim, warningDim, errorDim, infoDim), git diff colors (diffAddition, diffAdditionBg, diffDeletion, diffDeletionBg), overlay/interactive states (overlay, overlayHeavy, hoverBg, activeBg), and shadow tokens to ThemeColors. All 17 theme definitions populated with palette-appropriate values. Light themes use black hover overlays and lighter shadows; dark/vibe themes use white hover overlays with heavier shadows. CSS custom property mappings, fallback themes, and tests updated accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g components - CustomThemeBuilder now displays all 30 tokens organized in 8 semantic sections - Fix remaining hardcoded colors in AgentSessionsModal chat bubbles, PlaygroundPanel button, and AgentSelectionScreen detection indicators - Update tests to assert against theme tokens instead of hardcoded color values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dre Synth theme has bright teal border color (#00d4aa) which made textDim (#60e0d0) invisible when used as disabled button text on border background. Switch disabled Send button from border to bgActivity background to ensure text contrast across all themes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Separate success/warning from accent/error colors so each status is visually distinct: success (#33ff99 green), warning (#ff9944 orange), error (#ff2a6d pink). Previously success shared accent teal and warning shared error pink. Also slightly brighten bgActivity for better contrast. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pedramamini
reviewed
Feb 15, 2026
Collaborator
pedramamini
left a comment
pedramamini
left a comment
There was a problem hiding this comment.
Theme Token System Review
Solid direction — eliminating hardcoded colors in favor of semantic tokens is the right call. The new token taxonomy is well-organized and the section grouping in CustomThemeBuilder is a nice touch. A few things I noticed while going through it:
1. Missing Custom Theme Migration (Critical)
Users with saved custom themes have 13 tokens stored in electron-store. After this PR, ThemeColors requires 30. When useSettings.ts loads the saved theme, it directly assigns the incomplete object. Every component referencing the new tokens (info, successDim, overlay, etc.) will get undefined.
Fix in useSettings.ts where savedCustomThemeColors is loaded — merge with defaults:
if (savedCustomThemeColors !== undefined) {
setCustomThemeColorsState({
...DEFAULT_CUSTOM_THEME_COLORS,
...(savedCustomThemeColors as Partial<ThemeColors>),
});
}2. accentForeground Used on Non-Accent Backgrounds
Several replacements use accentForeground (text ON accent bg) on success and error backgrounds:
LeaderboardRegistrationModal.tsx:1256— Delete button:errorbg +accentForeground→ should beerrorForegroundAICommandsPanel.tsx:320,368— Save buttons:successbg +accentForeground→ should besuccessForegroundSpecKitCommandsPanel.tsx:270— Same pattern → should besuccessForeground
The whole point of adding successForeground/errorForeground is to use them on their respective backgrounds.
3. TerminalOutput.tsx — bgMain as Foreground Color
color: theme.colors.bgMain, // search highlight textUsing bgMain as text color on a warning background is semantically wrong — it only works if bgMain happens to contrast with warning. Should be warningForeground.
4. themes.test.ts REQUIRED_COLORS Not Updated
The REQUIRED_COLORS array in src/__tests__/renderer/constants/themes.test.ts:14 still only lists the original 13 tokens. Won't catch themes missing the new 17 tokens.
5. Remaining Hardcoded Colors (~40+ instances)
The PR addresses ~17 components but there are still hardcoded colors in AutoRun.tsx (4), DeleteAgentConfirmModal.tsx (2), DeleteWorktreeModal.tsx (3), ProcessMonitor.tsx (3), QueuedItemsList.tsx (1), InlineWizard/*.tsx (4), MergeProgressModal.tsx (2), NotificationsPanel.tsx (1), GroupChatInput.tsx:456 (1), SessionList.tsx (3), MainPanel.tsx (1), LogViewer.tsx (2), MarketplaceModal.tsx (3), SettingsModal.tsx (4) — mostly the same color: 'white' / color: '#fff' on accent/status backgrounds pattern.
6. New Tokens Not Yet Consumed
overlay, overlayHeavy, hoverBg, activeBg, shadow, diffAddition/Deletion/Bg, info, infoDim are defined but not used anywhere in this PR. Not blocking, just noting.
That said — don't stress about addressing all of this. I'm going to pull this into a local branch and give it a polish pass myself. If you've got the patience and want to work through the items above, that's hugely appreciated, but no pressure at all. The foundation here is good. 👍
The border token (#00d4aa) was a bright teal identical to the accent color, causing invisible text on 100+ components that use border as a disabled button/badge background with textDim text. Changed to #1a3a4a (dark muted teal) which maintains the synth aesthetic while functioning correctly as a border/surface color. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sume Add configurable auto-scroll behavior for AI mode output that automatically scrolls to bottom when new content arrives, with smart pause when user scrolls up to read content and automatic resume when they scroll back to bottom. - Add autoScrollAiMode setting persisted via electron-store (default: off) - Add inline floating toggle button (bottom-left) with visual state feedback - Add keyboard shortcut (Alt+Cmd+S) registered as system utility shortcut - Add Settings Modal checkbox in General tab - Thread setting through App → MainPanel → TerminalOutput via props - Use instant scroll (behavior: 'auto') to prevent jitter during streaming - Pause auto-scroll when user scrolls up, resume on scroll-to-bottom - Toggle button reflects paused state (dim icon when paused) - New message indicator shown when paused (user scrolled away from bottom) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pass autoScrollAiMode/setAutoScrollAiMode as props from App to SettingsModal so toggling takes immediate effect (was using independent useSettings() instance that didn't propagate to App's render tree) - Disable browser native scroll anchoring (overflow-anchor: none) on the AI output scroll container when auto-scroll is off or paused, preventing the browser from pinning viewport to bottom on new content - Add programmaticScrollRef guard and autoScrollPaused state to properly distinguish user scrolls from effect-driven scrolls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When loading saved custom themes from electron-store, merge with DEFAULT_CUSTOM_THEME_COLORS so that themes saved with the old 13-token structure get sensible defaults for the 17 new tokens (info, successDim, overlay, hoverBg, shadow, diff*, etc.) instead of undefined values. Follows the same spread-defaults pattern already used for globalStats, autoRunStats, onboardingStats, and other settings in loadSettings(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace accentForeground with the correct semantic foreground token when paired with success, error, or warning backgrounds: - LeaderboardRegistrationModal: error bg → errorForeground - AICommandsPanel: success bg → successForeground (2 locations) - SpecKitCommandsPanel: success bg → successForeground - AutoRunDocumentSelector: success bg → successForeground (3 locations) - GroupChatInput: warning bg (busy state) → warningForeground Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace theme.colors.bgMain with theme.colors.warningForeground for search highlight text in TerminalOutput.tsx. bgMain as a text color on warning backgrounds only works by accident when bgMain happens to contrast with warning — breaks on themes with different luminance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s tokens Previously only checked 13 of 30 tokens, meaning themes missing the new 17 tokens (status foregrounds, dim variants, diff colors, overlays, etc.) would pass tests silently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ss 15 components
Replace hardcoded color values ('white', '#fff', '#ffffff', '#000', hex colors)
with semantic theme tokens (errorForeground, successForeground, warningForeground,
accentForeground, overlay, info, infoDim, successDim, warningDim, errorDim, etc.)
in AutoRun, DeleteAgentConfirmModal, DeleteWorktreeModal, GroupChatInput, LogViewer,
MainPanel, MarketplaceModal, MergeProgressModal, NotificationsPanel, ProcessMonitor,
QueuedItemsList, SessionList, SettingsModal, ConversationScreen, and DocumentEditor.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…verBg, activeBg) across 37 components Replace hardcoded rgba/hex colors with semantic theme tokens: - overlay/overlayHeavy: 15 modal backdrops (SymphonyModal, MarketplaceModal, MaestroWizard, etc.) - shadow: 11 boxShadow/textShadow instances (AchievementCard, DocumentsPanel, App.tsx, etc.) - diffAddition/diffDeletion/diffAdditionBg/diffDeletionBg: diff viewers (markdownConfig, ImageDiffViewer, GitDiffViewer, GitLogViewer) - info/infoDim: LogViewer level colors, DocumentNode large file indicator - hoverBg/activeBg: TabBar hover computation, TabSwitcherModal selected states - errorDim/warningDim: ContextWarningSash status backgrounds Updated 7 test files to assert theme token values instead of hardcoded colors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…theme tokens across 11 components Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
All review items addressed + additional fixesThanks for the thorough review! Every item has been addressed across 6 commits: Review Item #1 — Custom Theme Migration (Critical) ✅Commit: Saved custom themes (13 tokens) are now merged with if (savedCustomThemeColors !== undefined) {
setCustomThemeColorsState({
...DEFAULT_CUSTOM_THEME_COLORS,
...(savedCustomThemeColors as Partial<ThemeColors>),
});
}Review Item #2 —
|
| File | Old | New | Context |
|---|---|---|---|
HistoryPanel.tsx |
#ffffff |
successForeground |
Validated task icon |
HistoryDetailModal.tsx |
#ffffff |
successForeground |
Validated task icon |
OpenSpecCommandsPanel.tsx |
#000000 |
successForeground |
Save button on success bg |
QuitConfirmModal.tsx |
#ffffff |
errorForeground |
"Quit Anyway" button |
TerminalOutput.tsx |
#fff (×2) |
errorForeground |
STDERR label + delete confirm |
MergeProgressOverlay.tsx |
#fff |
errorForeground |
Cancel button |
SummarizeProgressOverlay.tsx |
#fff |
errorForeground |
"Yes" button |
SummarizeProgressModal.tsx |
#fff |
successForeground |
Check icon on success bg |
TransferProgressModal.tsx |
#fff (×2) |
errorForeground + successForeground |
Cancel button + check icon |
PlaygroundPanel.tsx |
#fff (×2) |
successForeground |
Copy-success buttons |
FilePreview.tsx |
accentForeground ?? '#000' |
accentForeground |
Removed unnecessary fallback |
All 19,184 tests pass, lint and build clean.
Remove unused programmaticScrollRef declaration that was left over from an earlier iteration. It was declared but never read or written. Addresses PR RunMaestro#390 review feedback (Task 1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace inline IIFE that computed isActive within JSX with a pre-computed isAutoScrollActive variable before the return statement. Eliminates unnecessary function creation per render and improves readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…new content When auto-scroll is paused and the user clicks resume, immediately scroll to the bottom using behavior: 'auto' (instant snap) rather than leaving the viewport at the old scroll position until new content arrives. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ffect deps filteredLogs is derived from shellLogs via activeLogs → collapsedLogs → filteredLogs, so having both in the dependency array caused the effect to double-fire on every new log message. Keep only filteredLogs.length which correctly covers both AI and terminal modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…button rendering, and props threading Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…and palette Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # src/renderer/components/TerminalOutput.tsx # src/renderer/hooks/settings/useSettings.ts
Resolve merge conflicts in useSettings.ts (accept zustand store refactor), HistoryPanel.tsx (use imported HistoryEntryItem from History/), and TabSwitcherModal.tsx (keep theme-aware activeBg color). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Branch synced with latest main (8708e73)Merged
All conflicts resolved cleanly. Lint and 19,697 tests pass. |
Collaborator
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds many new theme color tokens and applies them across the UI; replaces hard-coded colors with theme tokens; introduces persisted autoScrollAiMode setting, keyboard shortcut, Settings/QuickActions UI, prop threading, and TerminalOutput AI auto-scroll (MutationObserver, pause/resume, UI toggle); updates tests to match theme-driven colors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keyboard
participant App as App/Store
participant Settings as SettingsStore
participant Terminal as TerminalOutput
participant QuickActions
rect rgba(120, 170, 255, 0.5)
User->>Keyboard: press Alt+Meta+S
Keyboard->>App: dispatch shortcut "toggleAutoScroll"
App->>Settings: setAutoScrollAiMode(!current)
Settings-->>App: persisted new value
App->>Terminal: forward updated autoScrollAiMode prop
Terminal->>Terminal: enable/disable observer and update UI
end
rect rgba(120, 255, 170, 0.5)
User->>QuickActions: open modal and click "Toggle Auto-scroll"
QuickActions->>App: call setAutoScrollAiMode(value)
App->>Settings: persist change
Settings-->>App: updated value
App->>Terminal: propagate change
Terminal->>Terminal: update auto-scroll behavior (pause/resume, scroll)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/ContextWarningSash.tsx (1)
128-131:⚠️ Potential issue | 🟡 MinorHardcoded
#000contradicts the PR's theming goal.The button text uses a hardcoded black color, which won't adapt to themes and contradicts this PR's objective of replacing hardcoded colors with theme tokens. Consider using a semantic foreground token (e.g.,
theme.colors.bgMainor a dedicated button-text token) to ensure proper contrast across all themes.Suggested fix
style={{ backgroundColor: buttonBgColor, - color: '#000', + color: theme.colors.bgMain, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` around lines 128 - 131, The inline style in the ContextWarningSash component hardcodes color: '#000'; replace this with a semantic theme token instead (e.g., use the injected theme.colors.buttonText or theme.colors.text via the component's theme prop or a useTheme() hook) and remove the literal '#000'; update the style object that references buttonBgColor so the text color derives from the theme (or a dedicated button-text token) to ensure correct contrast across themes.src/renderer/components/FilePreview.tsx (1)
1313-1331:⚠️ Potential issue | 🟡 MinorAdd
accentForegroundto the effect dependencies.
The effect now readstheme.colors.accentForeground, but it isn’t in the dependency array, so theme changes that only affect the foreground won’t refresh highlights.🔧 Suggested fix
- }, [searchQuery, file?.content, isMarkdown, isImage, isCsv, theme.colors.accent]); + }, [ + searchQuery, + file?.content, + isMarkdown, + isImage, + isCsv, + theme.colors.accent, + theme.colors.accentForeground, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` around lines 1313 - 1331, The useEffect in FilePreview that highlights search matches depends on theme.colors.accentForeground but the dependency array only includes theme.colors.accent, so changes to accentForeground won't retrigger the effect; update the dependency array (the effect surrounding the match highlighting and cleanup where matchElementsRef is used) to include theme.colors.accentForeground alongside theme.colors.accent so theme foreground changes correctly refresh highlights.
🧹 Nitpick comments (8)
src/renderer/components/ContextWarningSash.tsx (3)
123-127: Missingoutline-noneclass on focusable buttons.The buttons have
tabIndex={0}but are missing theoutline-noneclass. As per coding guidelines, focusable elements should include both to ensure focus works correctly.Suggested fix
<button onClick={onSummarizeClick} onKeyDown={(e) => e.key === 'Enter' && onSummarizeClick()} tabIndex={0} - className="px-1.5 py-0.5 text-[10px] font-medium rounded transition-colors hover:opacity-90" + className="px-1.5 py-0.5 text-[10px] font-medium rounded transition-colors hover:opacity-90 outline-none"<button onClick={handleDismiss} onKeyDown={(e) => e.key === 'Enter' && handleDismiss()} tabIndex={0} - className="p-0.5 rounded hover:bg-white/10 transition-colors" + className="p-0.5 rounded hover:bg-white/10 transition-colors outline-none"Also applies to: 137-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` around lines 123 - 127, The focusable button elements in the ContextWarningSash component (the button using onSummarizeClick and the other button around lines 137-141) are missing the required outline-none Tailwind class; update those button JSX elements to include "outline-none" in their className strings so they have tabIndex={0} and the outline-none utility applied for consistent focus styling.
151-161: UnusedslideDownanimation is dead code.The
slideDownkeyframe animation is defined but never applied to any element in this component. Consider removing it or applying it to the root div if the slide-down effect is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` around lines 151 - 161, The CSS keyframes named slideDown in the ContextWarningSash component are defined but never used; either remove the unused `@keyframes` slideDown block from the styled string in ContextWarningSash.tsx, or apply it to the component root (e.g., add animation: slideDown 200ms ease-out; to the root div/class used in ContextWarningSash) so the animation is actually applied; update the styled string where `@keyframes` slideDown is declared and adjust the root element styling (in the React component render for ContextWarningSash) accordingly.
141-141: Hardcoded hover color doesn't respect theme.
hover:bg-white/10is a hardcoded color that won't adapt to themes. Consider using the theme'shoverBgtoken that was added in this PR.Suggested approach
-className="p-0.5 rounded hover:bg-white/10 transition-colors" +className="p-0.5 rounded transition-colors" style={{ color: textColor }}Then add hover style via inline or use
theme.colors.hoverBgin a hover state handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` at line 141, The hover background is hardcoded as "hover:bg-white/10" in the ContextWarningSash component; replace it with the theme token (e.g. theme.colors.hoverBg) so the hover color follows the active theme — update the className to remove "hover:bg-white/10" and instead apply the hover style using the theme token (either via inline style using useTheme() or via your existing hover state handler that reads theme.colors.hoverBg) in the ContextWarningSash render path so the hover background adapts to light/dark themes.src/renderer/components/MergeProgressModal.tsx (1)
149-159: Consider usinghoverBgtoken for hover states.The hardcoded
hover:bg-white/5won't adapt to light themes where a white overlay would be invisible. Since this PR introduces anhoverBgtoken, consider using it for consistency.Same applies to line 310 (
hover:bg-white/10).♻️ Suggested change
<button type="button" onClick={onCancel} - className="px-3 py-1.5 rounded text-xs border hover:bg-white/5 transition-colors" + className="px-3 py-1.5 rounded text-xs border transition-colors" style={{ borderColor: theme.colors.border, color: theme.colors.textMain, }} + onMouseEnter={(e) => e.currentTarget.style.backgroundColor = theme.colors.hoverBg} + onMouseLeave={(e) => e.currentTarget.style.backgroundColor = 'transparent'} >Alternatively, if keeping Tailwind hover classes is preferred for simplicity, this can be deferred to a future cleanup pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MergeProgressModal.tsx` around lines 149 - 159, Replace the hardcoded Tailwind hover classes on the "Continue Merge" button (the JSX button using onClick={onCancel} in MergeProgressModal.tsx) with the new hoverBg token so hover styles adapt to light/dark themes; specifically, remove or replace the "hover:bg-white/5" (and similarly the "hover:bg-white/10" usage noted elsewhere) and apply the component/theme hoverBg token via inline style or class composition that references theme.colors.hoverBg to ensure consistent hover appearance across themes.src/renderer/components/PlaygroundPanel.tsx (1)
636-648: Consider usingaccentForegroundfor remaining selected-state buttons.This PR updated several buttons to use
theme.colors.accentForegroundfor text on accent backgrounds (lines 761, 832, 1184, 1358), but similar patterns still use hardcoded#fff:
- Line 641: Badge level buttons
- Line 873: Origin grid buttons
- Line 997: Shape toggle buttons
- Line 1482: Easing option buttons
For consistency and full WCAG compliance across all themes, consider updating these as well.
♻️ Example fix for badge level buttons
style={{ backgroundColor: mockAutoRunStats.currentBadgeLevel === level ? theme.colors.accent : theme.colors.bgMain, color: mockAutoRunStats.currentBadgeLevel === level - ? '#fff' + ? theme.colors.accentForeground : theme.colors.textMain, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/PlaygroundPanel.tsx` around lines 636 - 648, Selected-state button text uses a hardcoded '#fff' in several places (e.g., badge level buttons using mockAutoRunStats.currentBadgeLevel) which breaks consistency with the newer theme usage; replace those hardcoded values with theme.colors.accentForeground so selected buttons use the theme-provided foreground color. Update the badge level buttons (where mockAutoRunStats.currentBadgeLevel is compared to level) and the other similar button groups mentioned (origin grid buttons, shape toggle buttons, easing option buttons) to use theme.colors.accentForeground for the color when in the selected state instead of '#fff'. Ensure each conditional that sets color for the selected state mirrors the pattern used elsewhere (using theme.colors.accentForeground).src/renderer/components/TerminalOutput.tsx (1)
1747-1772: Add tabIndex for keyboard accessibility on auto-scroll button.The auto-scroll toggle button is missing
tabIndexattribute. Per coding guidelines, interactive elements should have explicit tabIndex to ensure focus works correctly.♿ Proposed fix for accessibility
{session.inputMode === 'ai' && setAutoScrollAiMode && ( <button onClick={() => { if (autoScrollPaused) { // Resume: clear pause and snap to bottom immediately setAutoScrollPaused(false); if (scrollContainerRef.current) { scrollContainerRef.current.scrollTo({ top: scrollContainerRef.current.scrollHeight, behavior: 'auto', }); } } else { setAutoScrollAiMode(!autoScrollAiMode); } }} className="absolute bottom-4 left-4 p-2 rounded-full shadow-lg transition-all hover:scale-105 z-20" + tabIndex={0} style={{As per coding guidelines: "Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalOutput.tsx` around lines 1747 - 1772, The auto-scroll toggle <button> lacks an explicit tabIndex which breaks keyboard focus; update the button (the element that uses setAutoScrollAiMode, autoScrollPaused, autoScrollAiMode, isAutoScrollActive, and scrollContainerRef) to include tabIndex={0} and add the 'outline-none' CSS class (or the project-equivalent focus style) to its className so it is keyboard-focusable and matches the coding guideline for interactive elements.src/renderer/components/AutoRunExpandedModal.tsx (1)
366-381: Consider usingtheme.colors.errorForegroundfor the Stop button text.The Stop button (line 369) still uses hardcoded
'white'for text color when not stopping, while other components in this PR usetheme.colors.errorForegroundontheme.colors.errorbackgrounds for consistency. This may be intentional for visual distinction, but consider aligning with the semantic token pattern.♻️ Suggested change
style={{ backgroundColor: isStopping ? theme.colors.warning : theme.colors.error, - color: isStopping ? theme.colors.bgMain : 'white', + color: isStopping ? theme.colors.warningForeground : theme.colors.errorForeground, border: `1px solid ${isStopping ? theme.colors.warning : theme.colors.error}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRunExpandedModal.tsx` around lines 366 - 381, The Stop button in AutoRunExpandedModal still uses hardcoded 'white' for its text color; change the inline style on the button (the object where color is set) to use theme.colors.errorForeground when not stopping (i.e., color: isStopping ? theme.colors.bgMain : theme.colors.errorForeground) so the text uses the semantic foreground token consistently with theme.colors.error; update the same ternary usage near the title/pointerEvents/Loader render context if any related hardcoded color is present.src/renderer/components/QueuedItemsList.tsx (1)
234-273: Theme token updates look good, but modal should register with layer stack.The theme changes are correct:
theme.colors.overlayfor the backdroptheme.colors.errorForegroundpaired withtheme.colors.errorfor the Remove buttonHowever, this confirmation modal handles Escape locally (lines 74-77) instead of registering with the layer stack. Based on learnings: "Register modals with layer stack instead of handling Escape locally. Check that modal priority is set correctly in constants/modalPriorities.ts."
,
♻️ Suggested refactor to use layer stack
+import { useLayerStack } from '../contexts/LayerStackContext'; +import { MODAL_PRIORITIES } from '../constants/modalPriorities'; export const QueuedItemsList = memo( ({ executionQueue, theme, onRemoveQueuedItem, onReorderItems, activeTabId, }: QueuedItemsListProps) => { + const { registerLayer, unregisterLayer } = useLayerStack(); + const layerIdRef = useRef<string>(); + + // Register with layer stack when confirmation modal is open + useEffect(() => { + if (queueRemoveConfirmId) { + const id = registerLayer({ + type: 'modal', + priority: MODAL_PRIORITIES.CONFIRMATION, // Add to modalPriorities.ts if needed + blocksLowerLayers: true, + capturesFocus: true, + onEscape: () => setQueueRemoveConfirmId(null), + }); + layerIdRef.current = id; + return () => { + if (layerIdRef.current) { + unregisterLayer(layerIdRef.current); + } + }; + } + }, [queueRemoveConfirmId, registerLayer, unregisterLayer]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/QueuedItemsList.tsx` around lines 234 - 273, The modal currently handles Escape locally via handleModalKeyDown and uses queueRemoveConfirmId/state to close itself; instead register this confirmation modal with the app's layer stack so Escape and stacking are managed centrally. Replace the local onKeyDown handling/logic by calling the layer stack registration API (use the same pattern as other components that register modals), provide the correct priority from constants/modalPriorities.ts, and ensure unregistering calls setQueueRemoveConfirmId(null) (or invokes handleConfirmRemove when appropriate) on layer pop; keep existing visual changes (theme.colors.overlay, theme.colors.errorForeground) but remove local Escape handling in QueuedItemsList and rely on the layer stack callbacks to close the modal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/auto-scroll.test.tsx`:
- Around line 67-86: defaultTheme used in the tests is missing the new color
tokens referenced by TerminalOutput (warningForeground and errorForeground),
causing undefined styles; update the defaultTheme constant in the test to
include warningForeground and errorForeground within the colors object (matching
existing token naming like success/warning/error) so tests that render
TerminalOutput have those values available.
---
Outside diff comments:
In `@src/renderer/components/ContextWarningSash.tsx`:
- Around line 128-131: The inline style in the ContextWarningSash component
hardcodes color: '#000'; replace this with a semantic theme token instead (e.g.,
use the injected theme.colors.buttonText or theme.colors.text via the
component's theme prop or a useTheme() hook) and remove the literal '#000';
update the style object that references buttonBgColor so the text color derives
from the theme (or a dedicated button-text token) to ensure correct contrast
across themes.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 1313-1331: The useEffect in FilePreview that highlights search
matches depends on theme.colors.accentForeground but the dependency array only
includes theme.colors.accent, so changes to accentForeground won't retrigger the
effect; update the dependency array (the effect surrounding the match
highlighting and cleanup where matchElementsRef is used) to include
theme.colors.accentForeground alongside theme.colors.accent so theme foreground
changes correctly refresh highlights.
---
Nitpick comments:
In `@src/renderer/components/AutoRunExpandedModal.tsx`:
- Around line 366-381: The Stop button in AutoRunExpandedModal still uses
hardcoded 'white' for its text color; change the inline style on the button (the
object where color is set) to use theme.colors.errorForeground when not stopping
(i.e., color: isStopping ? theme.colors.bgMain : theme.colors.errorForeground)
so the text uses the semantic foreground token consistently with
theme.colors.error; update the same ternary usage near the
title/pointerEvents/Loader render context if any related hardcoded color is
present.
In `@src/renderer/components/ContextWarningSash.tsx`:
- Around line 123-127: The focusable button elements in the ContextWarningSash
component (the button using onSummarizeClick and the other button around lines
137-141) are missing the required outline-none Tailwind class; update those
button JSX elements to include "outline-none" in their className strings so they
have tabIndex={0} and the outline-none utility applied for consistent focus
styling.
- Around line 151-161: The CSS keyframes named slideDown in the
ContextWarningSash component are defined but never used; either remove the
unused `@keyframes` slideDown block from the styled string in
ContextWarningSash.tsx, or apply it to the component root (e.g., add animation:
slideDown 200ms ease-out; to the root div/class used in ContextWarningSash) so
the animation is actually applied; update the styled string where `@keyframes`
slideDown is declared and adjust the root element styling (in the React
component render for ContextWarningSash) accordingly.
- Line 141: The hover background is hardcoded as "hover:bg-white/10" in the
ContextWarningSash component; replace it with the theme token (e.g.
theme.colors.hoverBg) so the hover color follows the active theme — update the
className to remove "hover:bg-white/10" and instead apply the hover style using
the theme token (either via inline style using useTheme() or via your existing
hover state handler that reads theme.colors.hoverBg) in the ContextWarningSash
render path so the hover background adapts to light/dark themes.
In `@src/renderer/components/MergeProgressModal.tsx`:
- Around line 149-159: Replace the hardcoded Tailwind hover classes on the
"Continue Merge" button (the JSX button using onClick={onCancel} in
MergeProgressModal.tsx) with the new hoverBg token so hover styles adapt to
light/dark themes; specifically, remove or replace the "hover:bg-white/5" (and
similarly the "hover:bg-white/10" usage noted elsewhere) and apply the
component/theme hoverBg token via inline style or class composition that
references theme.colors.hoverBg to ensure consistent hover appearance across
themes.
In `@src/renderer/components/PlaygroundPanel.tsx`:
- Around line 636-648: Selected-state button text uses a hardcoded '#fff' in
several places (e.g., badge level buttons using
mockAutoRunStats.currentBadgeLevel) which breaks consistency with the newer
theme usage; replace those hardcoded values with theme.colors.accentForeground
so selected buttons use the theme-provided foreground color. Update the badge
level buttons (where mockAutoRunStats.currentBadgeLevel is compared to level)
and the other similar button groups mentioned (origin grid buttons, shape toggle
buttons, easing option buttons) to use theme.colors.accentForeground for the
color when in the selected state instead of '#fff'. Ensure each conditional that
sets color for the selected state mirrors the pattern used elsewhere (using
theme.colors.accentForeground).
In `@src/renderer/components/QueuedItemsList.tsx`:
- Around line 234-273: The modal currently handles Escape locally via
handleModalKeyDown and uses queueRemoveConfirmId/state to close itself; instead
register this confirmation modal with the app's layer stack so Escape and
stacking are managed centrally. Replace the local onKeyDown handling/logic by
calling the layer stack registration API (use the same pattern as other
components that register modals), provide the correct priority from
constants/modalPriorities.ts, and ensure unregistering calls
setQueueRemoveConfirmId(null) (or invokes handleConfirmRemove when appropriate)
on layer pop; keep existing visual changes (theme.colors.overlay,
theme.colors.errorForeground) but remove local Escape handling in
QueuedItemsList and rely on the layer stack callbacks to close the modal.
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1747-1772: The auto-scroll toggle <button> lacks an explicit
tabIndex which breaks keyboard focus; update the button (the element that uses
setAutoScrollAiMode, autoScrollPaused, autoScrollAiMode, isAutoScrollActive, and
scrollContainerRef) to include tabIndex={0} and add the 'outline-none' CSS class
(or the project-equivalent focus style) to its className so it is
keyboard-focusable and matches the coding guideline for interactive elements.
…eground/errorForeground tokens onto main's refactored version Reset TerminalOutput.tsx to main's latest version (which includes hook extraction refactors from PR RunMaestro#470), then re-applied the 4 theme token replacements from fix/theme-integration: 1. Search highlight (JSX): theme.mode ternary → theme.colors.warningForeground 2. Search highlight (HTML string): same pattern in innerHTML mark tag 3. STDERR badge: hardcoded '#fff' → theme.colors.errorForeground 4. Delete confirm button: hardcoded '#fff' → theme.colors.errorForeground Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/TerminalOutput.tsx (1)
1803-1860: Consider adding a visible focus indicator for keyboard accessibility.The button is natively focusable, but lacks explicit focus styling. Consider adding a focus ring (e.g.,
focus:ring-2 focus:ring-offset-2) to improve keyboard navigation visibility.♿ Suggested focus styling
- className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20`} + className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20 focus:outline-none focus:ring-2 focus:ring-offset-2`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalOutput.tsx` around lines 1803 - 1860, The auto-scroll toggle button rendered inside TerminalOutput (the button that calls setAutoScrollAiMode, setAutoScrollPaused, setHasNewMessages and uses ArrowDown/newMessageCount) lacks an explicit keyboard focus style; add visible focus styling to the button (for example Tailwind classes like focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-[color] or use focus-visible variants) so keyboard users see focus when tabbing to the control, ensuring the focus color contrasts with theme.colors (match accent or accentForeground) and keep existing hover/transition classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1803-1860: The auto-scroll toggle button rendered inside
TerminalOutput (the button that calls setAutoScrollAiMode, setAutoScrollPaused,
setHasNewMessages and uses ArrowDown/newMessageCount) lacks an explicit keyboard
focus style; add visible focus styling to the button (for example Tailwind
classes like focus:outline-none focus:ring-2 focus:ring-offset-2
focus:ring-[color] or use focus-visible variants) so keyboard users see focus
when tabbing to the control, ensuring the focus color contrasts with
theme.colors (match accent or accentForeground) and keep existing
hover/transition classes.
…igration spread Reset settingsStore.ts and useSettings.ts to main's versions to pick up EncoreFeatureFlags, userMessageAlignment, useNativeTitleBar, and autoHideMenuBar features added since the branch diverged. Added EncoreFeatureFlags type to renderer types. Key change: in loadAllSettings(), custom theme colors are now merged with DEFAULT_CUSTOM_THEME_COLORS via spread so saved themes with 13 tokens automatically gain the 17 new WCAG-compliant tokens on load. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/stores/settingsStore.ts`:
- Around line 1781-1787: getSettingsActions is missing the
setUserMessageAlignment setter from SettingsStoreActions; add
setUserMessageAlignment to the object returned by getSettingsActions so
external/non-React code can call it. Locate the getSettingsActions() return
(where setters like setAutoScrollAiMode, setEncoreFeatures,
setDirectorNotesSettings, setWakatimeApiKey, setWakatimeEnabled,
setUseNativeTitleBar, setAutoHideMenuBar are listed) and include
setUserMessageAlignment: state.setUserMessageAlignment alongside them.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/hooks/settings/useSettings.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.ts
Comment on lines
+1781
to
+1787
| setAutoScrollAiMode: state.setAutoScrollAiMode, | ||
| setEncoreFeatures: state.setEncoreFeatures, | ||
| setDirectorNotesSettings: state.setDirectorNotesSettings, | ||
| setWakatimeApiKey: state.setWakatimeApiKey, | ||
| setWakatimeEnabled: state.setWakatimeEnabled, | ||
| setUseNativeTitleBar: state.setUseNativeTitleBar, | ||
| setAutoHideMenuBar: state.setAutoHideMenuBar, |
There was a problem hiding this comment.
Missing setUserMessageAlignment in getSettingsActions.
The setUserMessageAlignment setter is defined in SettingsStoreActions (line 312) and implemented (lines 755-758), but it's not exposed in getSettingsActions(). This will cause issues if any non-React code needs to access this setter.
Proposed fix
setAutoScrollAiMode: state.setAutoScrollAiMode,
setEncoreFeatures: state.setEncoreFeatures,
+ setUserMessageAlignment: state.setUserMessageAlignment,
setDirectorNotesSettings: state.setDirectorNotesSettings,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setAutoScrollAiMode: state.setAutoScrollAiMode, | |
| setEncoreFeatures: state.setEncoreFeatures, | |
| setDirectorNotesSettings: state.setDirectorNotesSettings, | |
| setWakatimeApiKey: state.setWakatimeApiKey, | |
| setWakatimeEnabled: state.setWakatimeEnabled, | |
| setUseNativeTitleBar: state.setUseNativeTitleBar, | |
| setAutoHideMenuBar: state.setAutoHideMenuBar, | |
| setAutoScrollAiMode: state.setAutoScrollAiMode, | |
| setEncoreFeatures: state.setEncoreFeatures, | |
| setUserMessageAlignment: state.setUserMessageAlignment, | |
| setDirectorNotesSettings: state.setDirectorNotesSettings, | |
| setWakatimeApiKey: state.setWakatimeApiKey, | |
| setWakatimeEnabled: state.setWakatimeEnabled, | |
| setUseNativeTitleBar: state.setUseNativeTitleBar, | |
| setAutoHideMenuBar: state.setAutoHideMenuBar, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/stores/settingsStore.ts` around lines 1781 - 1787,
getSettingsActions is missing the setUserMessageAlignment setter from
SettingsStoreActions; add setUserMessageAlignment to the object returned by
getSettingsActions so external/non-React code can call it. Locate the
getSettingsActions() return (where setters like setAutoScrollAiMode,
setEncoreFeatures, setDirectorNotesSettings, setWakatimeApiKey,
setWakatimeEnabled, setUseNativeTitleBar, setAutoHideMenuBar are listed) and
include setUserMessageAlignment: state.setUserMessageAlignment alongside them.
…th semantic theme tokens
- ContextWarningSash: replace hardcoded #000 with errorForeground/warningForeground
- AutoRunExpandedModal: replace hardcoded 'white' with theme.colors.errorForeground
- MergeProgressModal: replace hover:bg-white/5 and hover:bg-white/10 with theme.colors.hoverBg
- FilePreview: add theme.colors.accentForeground to useEffect dependency array
- TerminalOutput: add tabIndex={0} and outline-none to auto-scroll toggle for a11y
- auto-scroll.test.tsx: expand test theme from 13 to 30 color tokens; fix stale
button title expectations (enable/disable → pin/unpin terminology)
All changes ensure light themes render correctly by using semantic tokens instead
of hardcoded dark-theme-only colors. Tests: 19,697 pass, lint clean.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Review Feedback Addressed (commit 67479d9)Addressed all remaining CodeRabbit review items from the initial review pass. Changes ensure light themes render correctly by replacing hardcoded dark-theme-only color values with semantic theme tokens. ChangesHardcoded color → semantic token replacements:
React correctness:
Accessibility:
Test maintenance:
Verification
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/FilePreview.tsx (1)
1316-1339:⚠️ Potential issue | 🟡 MinorMarkdown current-search highlight still hardcodes foreground color
Great move to
theme.colors.accentForegroundfor code highlights, but markdown current highlight still usescolor:#fff`` at Line 1791. This can break contrast on themes where accent foreground is not white.🎯 Proposed fix
::highlight(search-current) { background-color: ${theme.colors.accent}; - color: `#fff`; + color: ${theme.colors.accentForeground}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` around lines 1316 - 1339, In FilePreview.tsx locate the markdown "current-search" highlight implementation (the CSS or inline style that sets color: '#fff' for markdown highlights around the current match) and replace the hardcoded '#fff' with the theme value (theme.colors.accentForeground) so the foreground respects the active theme; update any inline style usage or CSS-in-JS to reference theme.colors.accentForeground and ensure the component that manages highlights (the code that sets matchElements[0].style.color and the cleanup logic in the same effect) uses that theme value consistently.
🧹 Nitpick comments (5)
src/renderer/components/AutoRunExpandedModal.tsx (1)
367-370: Prefer semantic warning foreground in stopping state.Line 369 still uses
theme.colors.bgMainontheme.colors.warning. Considertheme.colors.warningForegroundfor consistent status token pairing across themes.Suggested change
- color: isStopping ? theme.colors.bgMain : theme.colors.errorForeground, + color: isStopping ? theme.colors.warningForeground : theme.colors.errorForeground,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRunExpandedModal.tsx` around lines 367 - 370, The inline style for the stop button uses theme.colors.bgMain as the text color when isStopping is true; update the color expression in AutoRunExpandedModal's style block (the property currently using isStopping ? theme.colors.bgMain : theme.colors.errorForeground) to use theme.colors.warningForeground when isStopping is true so the warning token pairs semantically (reference: isStopping, theme.colors.warning, theme.colors.warningForeground).src/renderer/components/ContextWarningSash.tsx (2)
152-162: UnusedslideDownanimation can be removed.The
slideDownkeyframes are defined but the animation is never applied to any element. Consider removing this dead CSS to reduce bundle size.🧹 Suggested cleanup
<style>{` - `@keyframes` slideDown { - from { - opacity: 0; - transform: translateY(-100%); - } - to { - opacity: 1; - transform: translateY(0); - } - } - `@keyframes` pulse { 0%, 100% { opacity: 1; } 50% { opacity: 0.7; } } .warning-icon-pulse { animation: pulse 2s infinite; } `}</style>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` around lines 152 - 162, The CSS keyframes named slideDown in ContextWarningSash.tsx are dead code because nothing applies that animation; remove the `@keyframes` slideDown block from the style string (or alternatively apply it where appropriate) to eliminate unused CSS and reduce bundle size—search for the slideDown identifier in ContextWarningSash.tsx and delete the entire `@keyframes` slideDown { ... } declaration if you do not intend to animate any element with that name.
142-142: Hardcoded hover color inconsistent with PR goals.The dismiss button uses
hover:bg-white/10, which is a hardcoded color. Given this PR's objective to replace hardcoded colors with theme tokens, and sincehoverBgis now available in the theme system, consider using the theme token for consistency.♻️ Suggested fix
<button onClick={handleDismiss} onKeyDown={(e) => e.key === 'Enter' && handleDismiss()} tabIndex={0} - className="p-0.5 rounded hover:bg-white/10 transition-colors" + className="p-0.5 rounded transition-colors" - style={{ color: textColor }} + style={{ color: textColor, '--hover-bg': theme.colors.hoverBg } as React.CSSProperties} title="Dismiss" aria-label="Dismiss warning" >Alternatively, apply the hover background inline or via a CSS-in-JS approach that uses
theme.colors.hoverBg.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ContextWarningSash.tsx` at line 142, The dismiss button in ContextWarningSash uses a hardcoded hover class "hover:bg-white/10"; replace this with the theme token (e.g., theme.colors.hoverBg) so hover styling follows the theme system: locate the dismiss button render in ContextWarningSash (the element with className containing "p-0.5 rounded hover:bg-white/10 transition-colors") and swap the hardcoded hover class for a theme-driven style (either by adding a class that maps to the hoverBg token or applying an inline/CSS-in-JS hover style using theme.colors.hoverBg) to maintain consistency with the new theme tokens.src/renderer/components/MergeProgressModal.tsx (2)
316-323: Same accessibility gap: add focus handlers.Same issue as the "Continue Merge" button—keyboard users won't see focus feedback. Apply the same
onFocus/onBlurpattern here.♻️ Suggested fix
<button type="button" onClick={onCancel} className="p-1 rounded transition-colors" style={{ color: theme.colors.textDim }} onMouseEnter={(e) => { e.currentTarget.style.backgroundColor = theme.colors.hoverBg; }} onMouseLeave={(e) => { e.currentTarget.style.backgroundColor = ''; }} + onFocus={(e) => { + e.currentTarget.style.backgroundColor = theme.colors.hoverBg; + }} + onBlur={(e) => { + e.currentTarget.style.backgroundColor = ''; + }} aria-label="Close modal" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MergeProgressModal.tsx` around lines 316 - 323, The interactive element in MergeProgressModal (the element with className "p-1 rounded transition-colors" that currently uses onMouseEnter/onMouseLeave) lacks keyboard focus feedback; add matching onFocus and onBlur handlers to set and clear the same backgroundColor (use theme.colors.hoverBg) so keyboard users receive the same visual cue as mouse users, updating the element alongside the existing onMouseEnter/onMouseLeave handlers in the MergeProgressModal component.
152-162: Add focus state handling for keyboard accessibility.The
onMouseEnter/onMouseLeavepattern provides hover feedback for mouse users but doesn't address keyboard navigation. When a user tabs to this button, there's no visual indication of focus.Consider adding
onFocus/onBlurhandlers alongside the mouse handlers, or use CSS:focus-visiblefor the highlight.♻️ Suggested fix
<button type="button" onClick={onCancel} className="px-3 py-1.5 rounded text-xs border transition-colors" style={{ borderColor: theme.colors.border, color: theme.colors.textMain, }} onMouseEnter={(e) => { e.currentTarget.style.backgroundColor = theme.colors.hoverBg; }} onMouseLeave={(e) => { e.currentTarget.style.backgroundColor = ''; }} + onFocus={(e) => { + e.currentTarget.style.backgroundColor = theme.colors.hoverBg; + }} + onBlur={(e) => { + e.currentTarget.style.backgroundColor = ''; + }} >As per coding guidelines: "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MergeProgressModal.tsx` around lines 152 - 162, In MergeProgressModal, the interactive element that currently uses onMouseEnter/onMouseLeave for hover (the element with className "px-3 py-1.5 rounded text-xs border transition-colors") needs keyboard focus support: add tabIndex={0} and mirror the hover behavior with onFocus and onBlur handlers (set e.currentTarget.style.backgroundColor = theme.colors.hoverBg on focus and clear it on blur) or alternatively apply a CSS :focus-visible rule that matches the hover highlight; ensure the same border/color styling logic (using theme.colors.border and theme.colors.textMain) is preserved so keyboard users get the same visual feedback as mouse users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1827-1828: The button-like element in TerminalOutput.tsx currently
has tabIndex={0} and the class contains "outline-none", leaving no visible focus
indicator; remove "outline-none" and add visible focus styles (e.g. Tailwind
focus-visible:ring, focus-visible:ring-offset, and focus-visible:ring-{color} or
focus:ring-2/focus:ring-offset-2) to the className so keyboard users see focus,
and also add onFocus and onBlur handlers (or reuse existing focus handlers) on
that same element to manage any focus state if required.
---
Outside diff comments:
In `@src/renderer/components/FilePreview.tsx`:
- Around line 1316-1339: In FilePreview.tsx locate the markdown "current-search"
highlight implementation (the CSS or inline style that sets color: '#fff' for
markdown highlights around the current match) and replace the hardcoded '#fff'
with the theme value (theme.colors.accentForeground) so the foreground respects
the active theme; update any inline style usage or CSS-in-JS to reference
theme.colors.accentForeground and ensure the component that manages highlights
(the code that sets matchElements[0].style.color and the cleanup logic in the
same effect) uses that theme value consistently.
---
Nitpick comments:
In `@src/renderer/components/AutoRunExpandedModal.tsx`:
- Around line 367-370: The inline style for the stop button uses
theme.colors.bgMain as the text color when isStopping is true; update the color
expression in AutoRunExpandedModal's style block (the property currently using
isStopping ? theme.colors.bgMain : theme.colors.errorForeground) to use
theme.colors.warningForeground when isStopping is true so the warning token
pairs semantically (reference: isStopping, theme.colors.warning,
theme.colors.warningForeground).
In `@src/renderer/components/ContextWarningSash.tsx`:
- Around line 152-162: The CSS keyframes named slideDown in
ContextWarningSash.tsx are dead code because nothing applies that animation;
remove the `@keyframes` slideDown block from the style string (or alternatively
apply it where appropriate) to eliminate unused CSS and reduce bundle
size—search for the slideDown identifier in ContextWarningSash.tsx and delete
the entire `@keyframes` slideDown { ... } declaration if you do not intend to
animate any element with that name.
- Line 142: The dismiss button in ContextWarningSash uses a hardcoded hover
class "hover:bg-white/10"; replace this with the theme token (e.g.,
theme.colors.hoverBg) so hover styling follows the theme system: locate the
dismiss button render in ContextWarningSash (the element with className
containing "p-0.5 rounded hover:bg-white/10 transition-colors") and swap the
hardcoded hover class for a theme-driven style (either by adding a class that
maps to the hoverBg token or applying an inline/CSS-in-JS hover style using
theme.colors.hoverBg) to maintain consistency with the new theme tokens.
In `@src/renderer/components/MergeProgressModal.tsx`:
- Around line 316-323: The interactive element in MergeProgressModal (the
element with className "p-1 rounded transition-colors" that currently uses
onMouseEnter/onMouseLeave) lacks keyboard focus feedback; add matching onFocus
and onBlur handlers to set and clear the same backgroundColor (use
theme.colors.hoverBg) so keyboard users receive the same visual cue as mouse
users, updating the element alongside the existing onMouseEnter/onMouseLeave
handlers in the MergeProgressModal component.
- Around line 152-162: In MergeProgressModal, the interactive element that
currently uses onMouseEnter/onMouseLeave for hover (the element with className
"px-3 py-1.5 rounded text-xs border transition-colors") needs keyboard focus
support: add tabIndex={0} and mirror the hover behavior with onFocus and onBlur
handlers (set e.currentTarget.style.backgroundColor = theme.colors.hoverBg on
focus and clear it on blur) or alternatively apply a CSS :focus-visible rule
that matches the hover highlight; ensure the same border/color styling logic
(using theme.colors.border and theme.colors.textMain) is preserved so keyboard
users get the same visual feedback as mouse users.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/renderer/components/auto-scroll.test.tsxsrc/renderer/components/AutoRunExpandedModal.tsxsrc/renderer/components/ContextWarningSash.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/MergeProgressModal.tsxsrc/renderer/components/TerminalOutput.tsx
Comment on lines
+1827
to
+1828
| tabIndex={0} | ||
| className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20 outline-none`} |
There was a problem hiding this comment.
Add visible focus state for keyboard accessibility.
The button has tabIndex={0} and outline-none, but no visible focus indicator. Keyboard users won't be able to see when the button is focused. Consider adding a focus ring style.
♿ Proposed fix to add focus visibility
- className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20 outline-none`}
+ className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20 outline-none focus-visible:ring-2 focus-visible:ring-offset-2`}
+ style={{
+ ...existingStyles,
+ '--tw-ring-color': theme.colors.accent,
+ }}Alternatively, use inline focus styles consistent with theme:
className={`absolute bottom-4 ${userMessageAlignment === 'right' ? 'left-6' : 'right-6'} flex items-center gap-2 px-3 py-2 rounded-full shadow-lg transition-all hover:scale-105 z-20`}(Remove outline-none to allow default browser focus ring)
As per coding guidelines: "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TerminalOutput.tsx` around lines 1827 - 1828, The
button-like element in TerminalOutput.tsx currently has tabIndex={0} and the
class contains "outline-none", leaving no visible focus indicator; remove
"outline-none" and add visible focus styles (e.g. Tailwind focus-visible:ring,
focus-visible:ring-offset, and focus-visible:ring-{color} or
focus:ring-2/focus:ring-offset-2) to the className so keyboard users see focus,
and also add onFocus and onBlur handlers (or reuse existing focus handlers) on
that same element to manage any focus state if required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New tokens added to
ThemeColors:successForeground,warningForeground,errorForegroundsuccessDim,warningDim,errorDim,infoDiminfodiffAddition,diffAdditionBg,diffDeletion,diffDeletionBgoverlay,overlayHeavyhoverBg,activeBgshadowDre Synth color fixes:
bgActivity#150530#1a0838success#00ffcc(same as accent)#33ff99(green)warning#ff2a6d(same as error)#ff9944(orange)Files changed: 27 files, 5 commits
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests