chore: React Doctor phase-1 cleanup (alt text + duplicate prop)#427
Merged
reachraza merged 5 commits intoRunMaestro:mainfrom Feb 23, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWide UI and accessibility refactor across renderer and mobile: images receive alt text and keyboard semantics, many interactive divs converted to buttons, search/view-mode updates centralized into handlers, modal overlays use explicit invisible close buttons, scroll/state logic adjusted, and one test now uses Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 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 |
Greptile SummaryFixed React Doctor phase-1 accessibility and duplicate prop errors across 6 files. Changes include adding descriptive alt text to all
All changes are low-risk, focused fixes that improve accessibility without affecting functionality. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: a625816 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/InlineWizard/StreamingDocumentPreview.tsx (1)
349-367:⚠️ Potential issue | 🟡 MinorAdd
relativepositioning to parent container.The scroll indicator uses
absolute bottom-4positioning, but the parent container at line 239 lacksposition: relative. This causes the button to position relative to the viewport instead of the preview container, potentially placing it outside the intended area. Add therelativeclass to establish a proper positioning context.🛠️ Fix
- <div className="flex flex-col h-full streaming-preview"> + <div className="relative flex flex-col h-full streaming-preview">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx` around lines 349 - 367, The scroll-resume button is absolutely positioned but its parent container lacks a positioning context, so add the CSS positioning context by adding the "relative" class to the parent element that wraps the preview and contains userScrolledRef/containerRef in the StreamingDocumentPreview component; this ensures the absolute-styled button (which references userScrolledRef, containerRef, and calls updateUserScrolled) is positioned relative to the preview container rather than the viewport.
🧹 Nitpick comments (6)
src/renderer/components/Settings/SshRemoteModal.tsx (1)
570-580: AddtabIndexto the listbox container for proper focus handling.The dropdown container has
role="listbox"andonKeyDownbut lackstabIndex. Per coding guidelines, interactive elements needtabIndex={0}ortabIndex={-1}withoutline-noneto ensure focus works correctly. While the filter input inside receives focus, the listbox itself should be focusable for full accessibility compliance.♻️ Proposed fix
<div className="absolute top-full left-0 right-0 mt-1 rounded border shadow-lg z-10" style={{ backgroundColor: theme.colors.bgMain, borderColor: theme.colors.border, }} onKeyDown={handleDropdownKeyDown} role="listbox" aria-label="SSH config hosts" + tabIndex={-1} >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/Settings/SshRemoteModal.tsx` around lines 570 - 580, The listbox container rendered when showSshConfigDropdown is true is missing a tabIndex which prevents it from being focusable for keyboard handling; update the dropdown div (the element with role="listbox" and onKeyDown={handleDropdownKeyDown}) to include tabIndex={0} (or tabIndex={-1} plus an "outline-none" class per guidelines) so it can receive focus and keyboard events properly while preserving the existing onKeyDown handler and ARIA attributes.src/renderer/components/InputArea.tsx (1)
464-499: Good accessibility improvements for staged images.Alt text,
role="button",tabIndex={0}, and keyboard activation are properly implemented.Same as TerminalOutput images: consider adding explicit focus styles for keyboard navigation visibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 464 - 499, Add explicit visible focus styles for the staged image elements to improve keyboard navigation visibility: update the image element rendered from stagedImages (the <img> with onClick/onKeyDown that calls setLightboxImage) to include a clear focus indicator (e.g., outline or ring) via a focus-visible or focus CSS class and ensure the remove <button> (the one that calls setStagedImages) also has matching focus styles; prefer using existing theme colors (theme.colors.border) or a dedicated utility class so keyboard users can see which staged image or delete button is focused.src/renderer/components/TerminalOutput.tsx (1)
427-451: Good accessibility improvements for terminal images.The additions of
alttext,role="button",tabIndex={0}, and keyboard activation (Enter/Space) are well-implemented and align with the PR's accessibility goals.Consider adding an explicit focus style (e.g.,
focus:ring-2 focus:ring-accent) for better visual feedback when users navigate via keyboard, rather than relying on browser defaults. Based on learnings: "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 427 - 451, The terminal image elements lack an explicit visible focus style; update the <img> rendered inside the log.images.map in TerminalOutput (the element that calls setLightboxImage) to include a focus utility class such as "focus:ring-2 focus:ring-accent" (and optionally "outline-none" if you prefer custom ring-only focus) so keyboard users get clear visual feedback when tabbing to the image; ensure tabIndex={0} remains and that the onKeyDown handler still triggers setLightboxImage so keyboard activation and the new focus style work together.src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx (2)
139-186: Inconsistent indentation in template string.Lines 140 and 182-186 have mixed tabs/spaces. As per coding guidelines, use tabs for indentation consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx` around lines 139 - 186, The template string assigned to the proseStyles constant in StreamingDocumentPreview.tsx contains mixed spaces and tabs (notably around the beginning of the string and the checkbox CSS block), so replace the leading spaces on those lines with tabs to match project style; open the proseStyles template string, convert lines with mixed indentation (including the h1 start line and the checkbox/li:has(...) block) to use tabs only, and run the repo's formatter/linter to ensure consistency.
98-112: Render-phase ref mutation works but has subtle risks.Mutating refs during render (lines 102-105) can cause issues with React's Concurrent Mode and breaks the expectation that render is pure. While this "derived state in render" pattern avoids an extra effect, consider wrapping this in
useMemoor a custom hook to make the intent clearer and safer:useMemo(() => { if (lastFilenameRef.current !== (filename ?? '')) { lastFilenameRef.current = filename ?? ''; userScrolledRef.current = false; } }, [filename]);This preserves the synchronous reset behavior while being explicit about dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx` around lines 98 - 112, The render mutates refs directly (lastFilenameRef and userScrolledRef) which breaks render purity; move the filename-change reset logic out of the top-level render and into a memoized callback such as useMemo(() => { ... }, [filename]) so the check and updates to lastFilenameRef.current and userScrolledRef.current run synchronously but only when filename changes; keep updateUserScrolled and setScrollRenderTick as-is so state updates still trigger a re-render when userScrolledRef changes.src/renderer/components/MarketplaceModal.tsx (1)
1009-1019: Stale dependency:setSelectedCategoryis no longer used in the effect.The effect body now uses
handleCategoryChangeinstead of callingsetSelectedCategorydirectly, butsetSelectedCategoryremains in the dependency array at line 1013.Proposed fix
}, [ isOpen, categories, selectedCategory, - setSelectedCategory, showDetailView, selectedPlaybook, selectedDocFilename, handleSelectDocument, handleCategoryChange, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MarketplaceModal.tsx` around lines 1009 - 1019, The effect in MarketplaceModal has a stale dependency: remove setSelectedCategory from the dependency array (it’s no longer referenced in the effect body) and keep the real dependencies (isOpen, categories, selectedCategory, showDetailView, selectedPlaybook, selectedDocFilename, handleSelectDocument, handleCategoryChange); if handleCategoryChange is not stable, wrap it in useCallback so the effect doesn’t re-run unnecessarily. Ensure you update the dependency array where it’s declared to match the actual variables used and verify no direct calls to setSelectedCategory remain inside the effect.
🤖 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/DocumentsPanel.tsx`:
- Around line 414-423: The overlay full-screen close button in DocumentsPanel
(the button with onClose and tabIndex={-1}) is missing the required focus
styling class; update that button element inside DocumentsPanel to include the
"outline-none" class (keeping tabIndex={-1} is fine per guidelines) and, if the
component should auto-focus, add a ref={(el) => el?.focus()} to the same button
so focus behavior is consistent across renderer components.
In `@src/renderer/components/MarketplaceModal.tsx`:
- Around line 417-427: Replace the native anchor used for author links in
MarketplaceModal (the block referencing playbook.authorLink, playbook.author and
ExternalLink) with the same external-open pattern used elsewhere: render a
button that calls window.maestro.shell.openExternal(playbook.authorLink) in its
onClick, preserve the inline styling (style={{ color: theme.colors.accent }})
and the ExternalLink icon, and add tabIndex={0} and the "outline-none" class to
the button to satisfy renderer accessibility/focus guidelines; remove
target/_blank and rel attributes since openExternal handles navigation.
In `@src/renderer/components/MergeSessionModal.tsx`:
- Around line 643-649: The tab buttons rendered in MergeSessionModal (the
element with props key={mode} role="tab" aria-selected={viewMode === mode}
aria-controls={`merge-tabpanel-${mode}`} onClick={() =>
handleViewModeChange(mode)`) are missing the id that tabpanels reference; add an
id attribute that matches the tabpanel's aria-labelledby (for example
id={`merge-tab-${mode}`}) so each button provides the label for its
corresponding panel, ensure the id is unique per mode and preserved when
rendering.
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Line 148: The timeRange state is initialized to a hardcoded 'week' causing an
extra fetch when defaultTimeRange differs; change the initialization of
timeRange to use the incoming prop (defaultTimeRange) instead of the literal
'week' so the first render/fetch uses the correct range (update the useState
call that sets timeRange and keep setTimeRange unchanged); also ensure any
open/modal effect that resets timeRange uses defaultTimeRange when opening
(reference: timeRange, setTimeRange, defaultTimeRange in
UsageDashboardModal.tsx).
In `@src/web/mobile/CommandHistoryDrawer.tsx`:
- Around line 212-215: In CommandHistoryDrawer, the inline style ordering sets
borderBottom then border: 'none', which clears the bottom border; fix by placing
the border: 'none' shorthand before borderBottom (or remove the shorthand
entirely) in the style object used for the list/item (the style block containing
borderBottom, touchAction, border, width) so borderBottom: `1px solid
${colors.border}` takes effect.
In `@src/web/mobile/MobileHistoryPanel.tsx`:
- Around line 878-901: The fetchHistory catch block must report errors to Sentry
and only swallow known/recoverable fetch failures: import captureException and
captureMessage from 'src/utils/sentry' and replace the current catch body in
fetchHistory so it first calls captureException(err, { extra: { projectPath,
sessionId } }) (and optionally captureMessage for specific status text), then if
the error is a known/recoverable fetch error (e.g., err.message includes 'Failed
to fetch history', err.name === 'AbortError', or err instanceof TypeError for
network failures) call webLogger.error(...) and setError(err.message || 'Failed
to load history') and do not rethrow; otherwise rethrow the error so it bubbles
to Sentry/outer handlers; keep the existing finally with setIsLoading(false).
Reference symbols: fetchHistory, webLogger, setError, setIsLoading, buildApiUrl,
and use captureException/captureMessage from 'src/utils/sentry'.
---
Outside diff comments:
In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx`:
- Around line 349-367: The scroll-resume button is absolutely positioned but its
parent container lacks a positioning context, so add the CSS positioning context
by adding the "relative" class to the parent element that wraps the preview and
contains userScrolledRef/containerRef in the StreamingDocumentPreview component;
this ensures the absolute-styled button (which references userScrolledRef,
containerRef, and calls updateUserScrolled) is positioned relative to the
preview container rather than the viewport.
---
Nitpick comments:
In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx`:
- Around line 139-186: The template string assigned to the proseStyles constant
in StreamingDocumentPreview.tsx contains mixed spaces and tabs (notably around
the beginning of the string and the checkbox CSS block), so replace the leading
spaces on those lines with tabs to match project style; open the proseStyles
template string, convert lines with mixed indentation (including the h1 start
line and the checkbox/li:has(...) block) to use tabs only, and run the repo's
formatter/linter to ensure consistency.
- Around line 98-112: The render mutates refs directly (lastFilenameRef and
userScrolledRef) which breaks render purity; move the filename-change reset
logic out of the top-level render and into a memoized callback such as
useMemo(() => { ... }, [filename]) so the check and updates to
lastFilenameRef.current and userScrolledRef.current run synchronously but only
when filename changes; keep updateUserScrolled and setScrollRenderTick as-is so
state updates still trigger a re-render when userScrolledRef changes.
In `@src/renderer/components/InputArea.tsx`:
- Around line 464-499: Add explicit visible focus styles for the staged image
elements to improve keyboard navigation visibility: update the image element
rendered from stagedImages (the <img> with onClick/onKeyDown that calls
setLightboxImage) to include a clear focus indicator (e.g., outline or ring) via
a focus-visible or focus CSS class and ensure the remove <button> (the one that
calls setStagedImages) also has matching focus styles; prefer using existing
theme colors (theme.colors.border) or a dedicated utility class so keyboard
users can see which staged image or delete button is focused.
In `@src/renderer/components/MarketplaceModal.tsx`:
- Around line 1009-1019: The effect in MarketplaceModal has a stale dependency:
remove setSelectedCategory from the dependency array (it’s no longer referenced
in the effect body) and keep the real dependencies (isOpen, categories,
selectedCategory, showDetailView, selectedPlaybook, selectedDocFilename,
handleSelectDocument, handleCategoryChange); if handleCategoryChange is not
stable, wrap it in useCallback so the effect doesn’t re-run unnecessarily.
Ensure you update the dependency array where it’s declared to match the actual
variables used and verify no direct calls to setSelectedCategory remain inside
the effect.
In `@src/renderer/components/Settings/SshRemoteModal.tsx`:
- Around line 570-580: The listbox container rendered when showSshConfigDropdown
is true is missing a tabIndex which prevents it from being focusable for
keyboard handling; update the dropdown div (the element with role="listbox" and
onKeyDown={handleDropdownKeyDown}) to include tabIndex={0} (or tabIndex={-1}
plus an "outline-none" class per guidelines) so it can receive focus and
keyboard events properly while preserving the existing onKeyDown handler and
ARIA attributes.
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 427-451: The terminal image elements lack an explicit visible
focus style; update the <img> rendered inside the log.images.map in
TerminalOutput (the element that calls setLightboxImage) to include a focus
utility class such as "focus:ring-2 focus:ring-accent" (and optionally
"outline-none" if you prefer custom ring-only focus) so keyboard users get clear
visual feedback when tabbing to the image; ensure tabIndex={0} remains and that
the onKeyDown handler still triggers setLightboxImage so keyboard activation and
the new focus style work together.
Comment on lines
+878
to
+901
| const fetchHistory = useCallback(async () => { | ||
| setIsLoading(true); | ||
| setError(null); | ||
| try { | ||
| const params = new URLSearchParams(); | ||
| if (projectPath) params.set('projectPath', projectPath); | ||
| if (sessionId) params.set('sessionId', sessionId); | ||
|
|
||
| fetchHistory(); | ||
| const queryString = params.toString(); | ||
| const apiUrl = buildApiUrl(`/history${queryString ? `?${queryString}` : ''}`); | ||
|
|
||
| const response = await fetch(apiUrl); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch history: ${response.statusText}`); | ||
| } | ||
| const data = await response.json(); | ||
| setEntries(data.entries || []); | ||
| webLogger.debug(`Fetched ${data.entries?.length || 0} history entries`, 'MobileHistory'); | ||
| } catch (err: any) { | ||
| webLogger.error('Failed to fetch history', 'MobileHistory', err); | ||
| setError(err.message || 'Failed to load history'); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } |
There was a problem hiding this comment.
Route unexpected fetch errors to Sentry and avoid swallowing them.
The catch block logs to webLogger and converts all failures into UI error state. This violates the guideline to report via Sentry and let unexpected errors bubble. Please capture exceptions and only handle expected/recoverable failures explicitly (e.g., known HTTP fetch failures), rethrowing everything else.
🔧 Suggested update (capture + selective handling)
-import { webLogger } from '../utils/logger';
+import { webLogger } from '../utils/logger';
+import { captureException } from '../utils/sentry';
} catch (err: any) {
- webLogger.error('Failed to fetch history', 'MobileHistory', err);
- setError(err.message || 'Failed to load history');
+ captureException(err, { tags: { surface: 'MobileHistoryPanel' }, extra: { projectPath, sessionId } });
+ webLogger.error('Failed to fetch history', 'MobileHistory', err);
+ if (err instanceof Error && err.message.startsWith('Failed to fetch history')) {
+ setError(err.message);
+ } else {
+ setError('Failed to load history');
+ throw err;
+ }
} finally {As per coding guidelines: “Do NOT silently swallow exceptions… Let unexpected errors bubble up to Sentry… Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context.”
📝 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
| const fetchHistory = useCallback(async () => { | |
| setIsLoading(true); | |
| setError(null); | |
| try { | |
| const params = new URLSearchParams(); | |
| if (projectPath) params.set('projectPath', projectPath); | |
| if (sessionId) params.set('sessionId', sessionId); | |
| fetchHistory(); | |
| const queryString = params.toString(); | |
| const apiUrl = buildApiUrl(`/history${queryString ? `?${queryString}` : ''}`); | |
| const response = await fetch(apiUrl); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch history: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| setEntries(data.entries || []); | |
| webLogger.debug(`Fetched ${data.entries?.length || 0} history entries`, 'MobileHistory'); | |
| } catch (err: any) { | |
| webLogger.error('Failed to fetch history', 'MobileHistory', err); | |
| setError(err.message || 'Failed to load history'); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| import { webLogger } from '../utils/logger'; | |
| import { captureException } from '../utils/sentry'; | |
| const fetchHistory = useCallback(async () => { | |
| setIsLoading(true); | |
| setError(null); | |
| try { | |
| const params = new URLSearchParams(); | |
| if (projectPath) params.set('projectPath', projectPath); | |
| if (sessionId) params.set('sessionId', sessionId); | |
| const queryString = params.toString(); | |
| const apiUrl = buildApiUrl(`/history${queryString ? `?${queryString}` : ''}`); | |
| const response = await fetch(apiUrl); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch history: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| setEntries(data.entries || []); | |
| webLogger.debug(`Fetched ${data.entries?.length || 0} history entries`, 'MobileHistory'); | |
| } catch (err: any) { | |
| captureException(err, { tags: { surface: 'MobileHistoryPanel' }, extra: { projectPath, sessionId } }); | |
| webLogger.error('Failed to fetch history', 'MobileHistory', err); | |
| if (err instanceof Error && err.message.startsWith('Failed to fetch history')) { | |
| setError(err.message); | |
| } else { | |
| setError('Failed to load history'); | |
| throw err; | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, [projectPath, sessionId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/MobileHistoryPanel.tsx` around lines 878 - 901, The
fetchHistory catch block must report errors to Sentry and only swallow
known/recoverable fetch failures: import captureException and captureMessage
from 'src/utils/sentry' and replace the current catch body in fetchHistory so it
first calls captureException(err, { extra: { projectPath, sessionId } }) (and
optionally captureMessage for specific status text), then if the error is a
known/recoverable fetch error (e.g., err.message includes 'Failed to fetch
history', err.name === 'AbortError', or err instanceof TypeError for network
failures) call webLogger.error(...) and setError(err.message || 'Failed to load
history') and do not rethrow; otherwise rethrow the error so it bubbles to
Sentry/outer handlers; keep the existing finally with setIsLoading(false).
Reference symbols: fetchHistory, webLogger, setError, setIsLoading, buildApiUrl,
and use captureException/captureMessage from 'src/utils/sentry'.
- DocumentsPanel: add outline-none to backdrop close button
- MarketplaceModal: replace <a href> with button+openExternal for author links; remove stale setSelectedCategory from effect dep array
- MergeSessionModal: add id={merge-tab-${mode}} to tab buttons for aria-labelledby
- UsageDashboardModal: initialize timeRange from defaultTimeRange prop instead of hardcoded 'week'
- CommandHistoryDrawer: move border:'none' before borderBottom so shorthand no longer overrides it
- StreamingDocumentPreview: add relative to outer container for absolute scroll-resume button; normalize proseStyles indentation to tabs; move render-time ref mutations into useMemo
- InputArea: add focus-visible ring styles to staged image and remove button
- SshRemoteModal: add tabIndex={0} and outline-none to listbox div
- TerminalOutput: add outline-none focus:ring-2 focus:ring-accent to terminal images
There was a problem hiding this comment.
Actionable comments posted: 6
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/InputArea.tsx (1)
252-364:⚠️ Potential issue | 🟡 MinorUse a ref callback for command-history autofocus and add tabIndex to the filter input.
The setTimeout-based focus diverges from the repo's autofocus guidance. Replace the useEffect with a ref callback and add
tabIndex={0}to the input element for proper focus accessibility.Proposed change
- const commandHistoryFilterRef = React.useRef<HTMLInputElement>(null); + const commandHistoryFilterRef = React.useRef<HTMLInputElement>(null); + const setCommandHistoryFilterRef = React.useCallback((el: HTMLInputElement | null) => { + commandHistoryFilterRef.current = el; + if (el) { + el.focus(); + } + }, []); @@ - useEffect(() => { - if (commandHistoryOpen) { - setTimeout(() => commandHistoryFilterRef.current?.focus(), 0); - } - }, [commandHistoryOpen]); @@ - ref={commandHistoryFilterRef} + ref={setCommandHistoryFilterRef} + tabIndex={0}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 252 - 364, Replace the setTimeout-based autofocus useEffect that focuses commandHistoryFilterRef when commandHistoryOpen is true with a ref callback on the filter input that immediately focuses when the element mounts (use the existing commandHistoryFilterRef ref logic but convert to a callback ref), and remove the useEffect and setTimeout; also ensure the filter input element has tabIndex={0} for keyboard accessibility so it can receive programmatic focus (refer to commandHistoryFilterRef, commandHistoryOpen, the old useEffect/setTimeout, and the filter input element).
♻️ Duplicate comments (1)
src/renderer/components/MarketplaceModal.tsx (1)
418-426: Author link →openExternalbutton: correctly applied.
tabIndex={0},outline-none, andwindow.maestro.shell.openExternalmatch the previously reviewed fix. The non-null assertion onplaybook.authorLink!is safe within the{playbook.authorLink ? …}guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MarketplaceModal.tsx` around lines 418 - 426, The Author link button in MarketplaceModal is correct—ensure the button in the MarketplaceModal component continues to use tabIndex={0}, className including "outline-none", and the onClick handler window.maestro.shell.openExternal with the non-null asserted playbook.authorLink! (this is safe because the button is rendered inside a guard that checks playbook.authorLink); no code changes required, just keep these symbols (MarketplaceModal, the button element, and window.maestro.shell.openExternal) as-is.
🧹 Nitpick comments (3)
src/renderer/components/UsageDashboard/UsageDashboardModal.tsx (1)
278-303: Optional: use aviewModeRefto avoid listener re-registration on every tab switch.
viewModein the dep array re-registers the globalkeydownlistener on each tab change. The file already applies this pattern foronClose(lines 169–170); the same approach works here.♻️ Proposed refactor
+ const viewModeRef = useRef(viewMode); + viewModeRef.current = viewMode; + useEffect(() => { if (!isOpen) return; const handleKeyDown = (e: KeyboardEvent) => { if (e.metaKey && e.shiftKey && (e.key === '[' || e.key === ']')) { e.preventDefault(); e.stopPropagation(); - const currentIndex = VIEW_MODE_TABS.findIndex((tab) => tab.value === viewMode); + const currentIndex = VIEW_MODE_TABS.findIndex((tab) => tab.value === viewModeRef.current); ... } }; window.addEventListener('keydown', handleKeyDown, true); return () => window.removeEventListener('keydown', handleKeyDown, true); - }, [isOpen, switchViewMode, viewMode]); + }, [isOpen, switchViewMode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx` around lines 278 - 303, The keydown listener currently re-registers whenever viewMode changes; create a ref to hold the latest viewMode (e.g., viewModeRef = useRef(viewMode)), update viewModeRef.current in a small useEffect that runs on viewMode changes, and modify the useEffect that registers handleKeyDown to read viewMode from viewModeRef.current (instead of the viewMode state) so the listener is only added/removed when isOpen (and stable switchViewMode) change; keep references to VIEW_MODE_TABS, handleKeyDown, and switchViewMode intact but remove viewMode from the dependency array of the registration effect.src/renderer/components/TerminalOutput.tsx (2)
1834-1834: Addoutline-noneto the auto-scroll button per renderer component guidelines.The button's
classNamestring does not includeoutline-none, leaving the default browser focus ring visible inconsistently with the rest of the UI.♻️ Proposed fix
- 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 outline-none`}Based on learnings: "Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly" for
src/renderer/**/*.{tsx,jsx}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalOutput.tsx` at line 1834, The auto-scroll button in TerminalOutput (the element with the long className containing "absolute bottom-4 ... flex items-center ...") is missing the outline-none utility; update that element's className to include "outline-none" (and if not already present, add tabIndex={0} or tabIndex={-1} as per renderer guidelines) so the browser focus ring is suppressed and keyboard focus behavior remains correct; look for the element in TerminalOutput.tsx where the className equals `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` and add the changes there.
432-449:role="button"on<img>— prefer a<button>wrapper for cleaner semantics.All required contracts (focus, keyboard activation,
tabIndex) are correctly applied, so this is functional. But wrapping in a<button>avoids the semantic mismatch and removes the need forrole="button"+ manual key handler.♻️ Proposed `` wrapper approach
{log.images.map((img, imgIdx) => ( - <img - key={img} - src={img} - alt={`Terminal output image ${imgIdx + 1}`} - className="h-20 rounded border cursor-zoom-in shrink-0 outline-none focus:ring-2 focus:ring-accent" - style={{ objectFit: 'contain', maxWidth: '200px' }} - role="button" - tabIndex={0} - onClick={() => setLightboxImage(img, log.images, 'history')} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - setLightboxImage(img, log.images, 'history'); - } - }} - /> + <button + key={`${img}-${imgIdx}`} + type="button" + className="shrink-0 outline-none focus:ring-2 focus:ring-accent rounded border cursor-zoom-in p-0" + onClick={() => setLightboxImage(img, log.images, 'history')} + > + <img + src={img} + alt={`Terminal output image ${imgIdx + 1}`} + className="h-20 rounded" + style={{ objectFit: 'contain', maxWidth: '200px' }} + /> + </button> ))}🤖 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 432 - 449, The <img> elements inside the log.images map are using role="button", tabIndex and a manual onKeyDown handler; replace this with a semantic <button> wrapper (e.g., wrap the <img> in a <button type="button">) and move the onClick handler to the button so keyboard activation is handled natively; remove role="button", tabIndex and the onKeyDown from the <img>, keep the alt text on the <img>, preserve styling (transfer interactive classes to the button or adjust CSS) and still call setLightboxImage(img, log.images, 'history') from the button's onClick; update code in TerminalOutput.tsx where log.images.map and setLightboxImage are used.
🤖 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/InlineWizard/StreamingDocumentPreview.tsx`:
- Around line 219-235: In StreamingDocumentPreview's custom renderer for links
(the a: ({ href, children }: any) => ... function) add explicit keyboard-focus
affordances: set tabIndex={0} on the <a> element, add the "outline-none"
className (or style outline: 'none') so the element meets the renderer focus
guideline, and—if the link should be auto-focused when mounted—add ref={(el) =>
el?.focus()}. Also mirror tabIndex and outline-none on the fallback <span> used
when href is falsy so non-link items are keyboard-focusable if intended.
- Around line 102-113: The current useMemo block mutates refs
(lastFilenameRef.current and userScrolledRef.current) which is a side effect;
replace that useMemo with a useEffect (or useLayoutEffect if you need to run
before paint) that depends on filename, move the logic that compares filename
and sets lastFilenameRef.current and userScrolledRef.current into that effect,
and keep updateUserScrolled and setScrollRenderTick unchanged; ensure the effect
uses filename and updates the refs exactly as the original code did.
In `@src/renderer/components/Settings/SshRemoteModal.tsx`:
- Around line 572-580: The container in SshRemoteModal currently uses
role="listbox" but the host entries are plain <button>s without role="option" or
aria-selected; update the host-rendering code (where host buttons are created
inside SshRemoteModal — the map/render function for hosts, and any helper like
handleDropdownKeyDown) to add role="option" and set
aria-selected={selectedHostId === host.id} (or equivalent boolean), and ensure
your keyboard handlers (handleDropdownKeyDown / selection state) update
selectedHostId so aria-selected reflects the keyboard focus/selection and the
visual highlight matches; alternatively, if you prefer the menu pattern, change
the container to role="menu" and each host button to role="menuitem" instead
(keep keyboard behavior consistent with the chosen pattern).
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1619-1620: The aria-label "Terminal output" on the TerminalOutput
component is ambiguous when multiple panes exist; update the region label to
include a session-specific identifier (for example use the session.name or
session.id or the tab title) so each instance is unique: locate the
TerminalOutput JSX where role="region" and aria-label="Terminal output" are set
and change aria-label to a template that falls back gracefully (e.g., `Terminal
output — ${session?.name ?? 'session ' + session?.id ?? index}`) to ensure
screen readers can distinguish concurrent terminals.
- Around line 434-437: Replace the unstable key and the non-existent Tailwind
ring color: change the list key from key={img} to key={`${img}-${imgIdx}`}
(references: img, imgIdx, log.images) to avoid duplicate-key warnings; replace
the static class focus:ring-accent with the generic focus ring classes (e.g.,
keep focus:ring and focus:ring-offset-0 if present) and add an inline style that
sets the CSS variable --tw-ring-color to theme.colors.accent (style={{
['--tw-ring-color' as any]: theme.colors.accent }}) so the focus ring uses the
dynamic accent color (references: className, theme.colors.accent, TerminalOutput
component).
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Around line 493-500: The backdrop button in UsageDashboardModal is missing the
required focus style suppression; update the invisible backdrop <button> (the
element using tabIndex={-1} and onClick={onClose} inside the UsageDashboardModal
component) to include the "outline-none" class so it never shows a focus ring
when clicked, and optionally add aria-hidden="true" to prevent it being exposed
to assistive tech (keep tabIndex and onClick as-is). Ensure the change targets
the same button element that sits as the absolute inset-0 overlay.
---
Outside diff comments:
In `@src/renderer/components/InputArea.tsx`:
- Around line 252-364: Replace the setTimeout-based autofocus useEffect that
focuses commandHistoryFilterRef when commandHistoryOpen is true with a ref
callback on the filter input that immediately focuses when the element mounts
(use the existing commandHistoryFilterRef ref logic but convert to a callback
ref), and remove the useEffect and setTimeout; also ensure the filter input
element has tabIndex={0} for keyboard accessibility so it can receive
programmatic focus (refer to commandHistoryFilterRef, commandHistoryOpen, the
old useEffect/setTimeout, and the filter input element).
---
Duplicate comments:
In `@src/renderer/components/MarketplaceModal.tsx`:
- Around line 418-426: The Author link button in MarketplaceModal is
correct—ensure the button in the MarketplaceModal component continues to use
tabIndex={0}, className including "outline-none", and the onClick handler
window.maestro.shell.openExternal with the non-null asserted
playbook.authorLink! (this is safe because the button is rendered inside a guard
that checks playbook.authorLink); no code changes required, just keep these
symbols (MarketplaceModal, the button element, and
window.maestro.shell.openExternal) as-is.
---
Nitpick comments:
In `@src/renderer/components/TerminalOutput.tsx`:
- Line 1834: The auto-scroll button in TerminalOutput (the element with the long
className containing "absolute bottom-4 ... flex items-center ...") is missing
the outline-none utility; update that element's className to include
"outline-none" (and if not already present, add tabIndex={0} or tabIndex={-1} as
per renderer guidelines) so the browser focus ring is suppressed and keyboard
focus behavior remains correct; look for the element in TerminalOutput.tsx where
the className equals `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` and add the changes there.
- Around line 432-449: The <img> elements inside the log.images map are using
role="button", tabIndex and a manual onKeyDown handler; replace this with a
semantic <button> wrapper (e.g., wrap the <img> in a <button type="button">) and
move the onClick handler to the button so keyboard activation is handled
natively; remove role="button", tabIndex and the onKeyDown from the <img>, keep
the alt text on the <img>, preserve styling (transfer interactive classes to the
button or adjust CSS) and still call setLightboxImage(img, log.images,
'history') from the button's onClick; update code in TerminalOutput.tsx where
log.images.map and setLightboxImage are used.
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Around line 278-303: The keydown listener currently re-registers whenever
viewMode changes; create a ref to hold the latest viewMode (e.g., viewModeRef =
useRef(viewMode)), update viewModeRef.current in a small useEffect that runs on
viewMode changes, and modify the useEffect that registers handleKeyDown to read
viewMode from viewModeRef.current (instead of the viewMode state) so the
listener is only added/removed when isOpen (and stable switchViewMode) change;
keep references to VIEW_MODE_TABS, handleKeyDown, and switchViewMode intact but
remove viewMode from the dependency array of the registration effect.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/renderer/components/DocumentsPanel.tsxsrc/renderer/components/InlineWizard/StreamingDocumentPreview.tsxsrc/renderer/components/InputArea.tsxsrc/renderer/components/MarketplaceModal.tsxsrc/renderer/components/MergeSessionModal.tsxsrc/renderer/components/Settings/SshRemoteModal.tsxsrc/renderer/components/TerminalOutput.tsxsrc/renderer/components/UsageDashboard/UsageDashboardModal.tsxsrc/web/mobile/CommandHistoryDrawer.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/MergeSessionModal.tsx
- src/web/mobile/CommandHistoryDrawer.tsx
Comment on lines
+102
to
+113
| useMemo(() => { | ||
| if (lastFilenameRef.current !== (filename ?? '')) { | ||
| lastFilenameRef.current = filename ?? ''; | ||
| userScrolledRef.current = false; | ||
| } | ||
| }, [filename]); | ||
|
|
||
| const updateUserScrolled = (next: boolean) => { | ||
| if (userScrolledRef.current !== next) { | ||
| userScrolledRef.current = next; | ||
| setScrollRenderTick((tick) => tick + 1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx | sed -n '95,125p'Repository: RunMaestro/Maestro
Length of output: 1307
🌐 Web query:
React useMemo side effects mutating refs should use useEffect instead concurrent mode
💡 Result:
useMemo runs during render, so its callback must be pure (no observable side effects). React may render more than once (or start rendering and abandon it) to support concurrency, so side effects in render can run extra times and/or at the wrong moment. React’s docs explicitly say side effects must run outside of render. [1]
That’s why mutating ref.current inside useMemo is a side effect and is not “concurrent-safe” in general. In Strict Mode (development), React may intentionally double-invoke functions that should be pure—including the functions you pass to useMemo—to surface these bugs. [2]
What to do instead:
- If you’re doing this just to “compute once” but also mutate something: don’t—
useMemois for computing/caching a value, not effects. [3] - If you need to perform something when inputs change (including writing to refs, subscribing, logging, calling imperative APIs): move it to
useEffect(oruseLayoutEffectif it must happen before paint). This keeps render pure. [1][4]
Rule of thumb: useMemo returns a value; useEffect performs work. If your useMemo callback “does” something (mutates refs, touches globals/DOM, triggers I/O), it belongs in an effect. [1][3]
Replace side-effectful useMemo with useEffect.
useMemo callbacks must be pure and free of side effects, but this code mutates refs (lastFilenameRef.current, userScrolledRef.current). React's documentation explicitly requires side effects to run in useEffect or useLayoutEffect, not during render. In Strict Mode, React intentionally double-invokes the callback to detect impurity, which would cause the refs to be reset twice. Move this logic to useEffect:
Proposed fix
-useMemo(() => {
- if (lastFilenameRef.current !== (filename ?? '')) {
- lastFilenameRef.current = filename ?? '';
- userScrolledRef.current = false;
- }
-}, [filename]);
+useEffect(() => {
+ if (lastFilenameRef.current !== (filename ?? '')) {
+ lastFilenameRef.current = filename ?? '';
+ userScrolledRef.current = false;
+ }
+}, [filename]);📝 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
| useMemo(() => { | |
| if (lastFilenameRef.current !== (filename ?? '')) { | |
| lastFilenameRef.current = filename ?? ''; | |
| userScrolledRef.current = false; | |
| } | |
| }, [filename]); | |
| const updateUserScrolled = (next: boolean) => { | |
| if (userScrolledRef.current !== next) { | |
| userScrolledRef.current = next; | |
| setScrollRenderTick((tick) => tick + 1); | |
| } | |
| useEffect(() => { | |
| if (lastFilenameRef.current !== (filename ?? '')) { | |
| lastFilenameRef.current = filename ?? ''; | |
| userScrolledRef.current = false; | |
| } | |
| }, [filename]); | |
| const updateUserScrolled = (next: boolean) => { | |
| if (userScrolledRef.current !== next) { | |
| userScrolledRef.current = next; | |
| setScrollRenderTick((tick) => tick + 1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx` around
lines 102 - 113, The current useMemo block mutates refs (lastFilenameRef.current
and userScrolledRef.current) which is a side effect; replace that useMemo with a
useEffect (or useLayoutEffect if you need to run before paint) that depends on
filename, move the logic that compares filename and sets lastFilenameRef.current
and userScrolledRef.current into that effect, and keep updateUserScrolled and
setScrollRenderTick unchanged; ensure the effect uses filename and updates the
refs exactly as the original code did.
Comment on lines
+219
to
+235
| a: ({ href, children }: any) => | ||
| href ? ( | ||
| <a | ||
| href={href} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| style={{ | ||
| color: theme.colors.accent, | ||
| textDecoration: 'underline', | ||
| cursor: 'pointer', | ||
| }} | ||
| > | ||
| {children} | ||
| </a> | ||
| ) : ( | ||
| <span style={{ color: theme.colors.accent, textDecoration: 'underline' }}> | ||
| {children} |
There was a problem hiding this comment.
Add focus affordances to markdown links.
The new anchor is focusable but missing tabIndex and outline-none, which are required by the renderer focus guideline. Consider adding them to the <a> element.
Proposed fix
<a
href={href}
target="_blank"
rel="noopener noreferrer"
+ tabIndex={0}
+ className="outline-none"
style={{
color: theme.colors.accent,
textDecoration: 'underline',
cursor: 'pointer',
}}
>As per coding guidelines, "Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components."
📝 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
| a: ({ href, children }: any) => | |
| href ? ( | |
| <a | |
| href={href} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| style={{ | |
| color: theme.colors.accent, | |
| textDecoration: 'underline', | |
| cursor: 'pointer', | |
| }} | |
| > | |
| {children} | |
| </a> | |
| ) : ( | |
| <span style={{ color: theme.colors.accent, textDecoration: 'underline' }}> | |
| {children} | |
| a: ({ href, children }: any) => | |
| href ? ( | |
| <a | |
| href={href} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| tabIndex={0} | |
| className="outline-none" | |
| style={{ | |
| color: theme.colors.accent, | |
| textDecoration: 'underline', | |
| cursor: 'pointer', | |
| }} | |
| > | |
| {children} | |
| </a> | |
| ) : ( | |
| <span style={{ color: theme.colors.accent, textDecoration: 'underline' }}> | |
| {children} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/InlineWizard/StreamingDocumentPreview.tsx` around
lines 219 - 235, In StreamingDocumentPreview's custom renderer for links (the a:
({ href, children }: any) => ... function) add explicit keyboard-focus
affordances: set tabIndex={0} on the <a> element, add the "outline-none"
className (or style outline: 'none') so the element meets the renderer focus
guideline, and—if the link should be auto-focused when mounted—add ref={(el) =>
el?.focus()}. Also mirror tabIndex and outline-none on the fallback <span> used
when href is falsy so non-link items are keyboard-focusable if intended.
Comment on lines
+572
to
+580
| className="absolute top-full left-0 right-0 mt-1 rounded border shadow-lg z-10 outline-none" | ||
| style={{ | ||
| backgroundColor: theme.colors.bgMain, | ||
| borderColor: theme.colors.border, | ||
| }} | ||
| onKeyDown={handleDropdownKeyDown} | ||
| role="listbox" | ||
| aria-label="SSH config hosts" | ||
| tabIndex={0} |
There was a problem hiding this comment.
role="listbox" requires role="option" children — ARIA semantic mismatch.
The WAI-ARIA listbox pattern requires each child interactive element to carry role="option" (and aria-selected). The host items are plain <button> elements with neither attribute, so screen readers navigating the "listbox" will find no options and behave unpredictably.
Two conformant paths:
Option A — fix to the listbox pattern (keeps keyboard highlight semantics):
♿ Proposed fix – add role="option" + aria-selected to each host button
- role="listbox"
- aria-label="SSH config hosts"
+ role="listbox"
+ aria-label="SSH config hosts"
+ aria-activedescendant={
+ filteredSshConfigHosts[sshConfigHighlightIndex]
+ ? `ssh-host-${filteredSshConfigHosts[sshConfigHighlightIndex].host}`
+ : undefined
+ }And on each host <button>:
<button
key={configHost.host}
type="button"
+ id={`ssh-host-${configHost.host}`}
+ role="option"
+ aria-selected={index === sshConfigHighlightIndex}
onClick={() => handleSelectSshConfigHost(configHost)}Option B — switch to a menu pattern (simpler, buttons stay buttons):
♿ Proposed fix – use role="menu" / role="menuitem"
- role="listbox"
- aria-label="SSH config hosts"
+ role="menu"
+ aria-label="SSH config hosts" <button
key={configHost.host}
type="button"
+ role="menuitem"
onClick={() => handleSelectSshConfigHost(configHost)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/SshRemoteModal.tsx` around lines 572 - 580,
The container in SshRemoteModal currently uses role="listbox" but the host
entries are plain <button>s without role="option" or aria-selected; update the
host-rendering code (where host buttons are created inside SshRemoteModal — the
map/render function for hosts, and any helper like handleDropdownKeyDown) to add
role="option" and set aria-selected={selectedHostId === host.id} (or equivalent
boolean), and ensure your keyboard handlers (handleDropdownKeyDown / selection
state) update selectedHostId so aria-selected reflects the keyboard
focus/selection and the visual highlight matches; alternatively, if you prefer
the menu pattern, change the container to role="menu" and each host button to
role="menuitem" instead (keep keyboard behavior consistent with the chosen
pattern).
Comment on lines
434
to
437
| key={img} | ||
| src={img} | ||
| className="h-20 rounded border cursor-zoom-in shrink-0" | ||
| alt={`Terminal output image ${imgIdx + 1}`} | ||
| className="h-20 rounded border cursor-zoom-in shrink-0 outline-none focus:ring-2 focus:ring-accent" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual code in TerminalOutput.tsx around the mentioned lines
cat -n src/renderer/components/TerminalOutput.tsx | sed -n '430,440p'Repository: RunMaestro/Maestro
Length of output: 556
🏁 Script executed:
# Check if tailwind.config.js or tailwind.config.ts exists and examine accent color configuration
find . -maxdepth 2 -name "tailwind.config.*" -type f | head -5Repository: RunMaestro/Maestro
Length of output: 83
🏁 Script executed:
# Search for how accent colors are used elsewhere in the renderer components
rg -t tsx "theme\.colors\.accent|focus:ring-accent" src/renderer --max-count 20Repository: RunMaestro/Maestro
Length of output: 89
🏁 Script executed:
cat -n tailwind.config.mjsRepository: RunMaestro/Maestro
Length of output: 490
🏁 Script executed:
# Search for how accent is used in TerminalOutput or similar components
rg "theme\.colors\.accent|accent" src/renderer/components/TerminalOutput.tsx -A 2 -B 2Repository: RunMaestro/Maestro
Length of output: 5010
🏁 Script executed:
# Search more broadly for accent color usage with inline styles
rg "backgroundColor.*accent|color.*accent|theme\.colors" src/renderer/components --max-count 30Repository: RunMaestro/Maestro
Length of output: 50375
Two concerns: key={img} and focus:ring-accent.
-
key={img}— using the image URL as the key can produce duplicate-key warnings iflog.imagescontains the same URL more than once.key={${img}-${imgIdx}}is safe either way. -
focus:ring-accent— this is a static Tailwind utility that requiresaccentto be registered as a Tailwind ring color. The tailwind.config.mjs does not extend ring colors, so this will silently do nothing. The component uses dynamictheme.colors.accentvia inlinestylethroughout, but the focus ring needs to use the same pattern: inlinestyle={{ ['--tw-ring-color' as any]: theme.colors.accent }}with Tailwind's base focus ring classes instead.
🤖 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 434 - 437, Replace
the unstable key and the non-existent Tailwind ring color: change the list key
from key={img} to key={`${img}-${imgIdx}`} (references: img, imgIdx, log.images)
to avoid duplicate-key warnings; replace the static class focus:ring-accent with
the generic focus ring classes (e.g., keep focus:ring and focus:ring-offset-0 if
present) and add an inline style that sets the CSS variable --tw-ring-color to
theme.colors.accent (style={{ ['--tw-ring-color' as any]: theme.colors.accent
}}) so the focus ring uses the dynamic accent color (references: className,
theme.colors.accent, TerminalOutput component).
Comment on lines
+1619
to
+1620
| role="region" | ||
| aria-label="Terminal output" |
There was a problem hiding this comment.
aria-label="Terminal output" becomes ambiguous with multiple concurrent instances.
Good landmark addition, but if multiple TerminalOutput panes are visible simultaneously (split-pane), screen readers encounter two identical "Terminal output" regions. Consider incorporating a session-level identifier (e.g., the tab name or session.name) to disambiguate.
💡 Example disambiguation
- aria-label="Terminal output"
+ aria-label={`Terminal output${activeTab?.name ? ` — ${activeTab.name}` : ''}`}🤖 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 1619 - 1620, The
aria-label "Terminal output" on the TerminalOutput component is ambiguous when
multiple panes exist; update the region label to include a session-specific
identifier (for example use the session.name or session.id or the tab title) so
each instance is unique: locate the TerminalOutput JSX where role="region" and
aria-label="Terminal output" are set and change aria-label to a template that
falls back gracefully (e.g., `Terminal output — ${session?.name ?? 'session ' +
session?.id ?? index}`) to ensure screen readers can distinguish concurrent
terminals.
Comment on lines
493
to
500
| <div className="fixed inset-0 modal-overlay flex items-center justify-center z-[9999] animate-in fade-in duration-100"> | ||
| <button | ||
| type="button" | ||
| className="absolute inset-0" | ||
| tabIndex={-1} | ||
| onClick={onClose} | ||
| aria-label="Close usage dashboard" | ||
| /> |
There was a problem hiding this comment.
Missing outline-none on the backdrop button — guideline violation.
Without outline-none, clicking the backdrop can briefly render a focus outline on the invisible button in some browsers/OS themes. The coding guidelines require both tabIndex and outline-none on interactive renderer elements.
🛠️ Proposed fix
<button
type="button"
- className="absolute inset-0"
+ className="absolute inset-0 outline-none"
tabIndex={-1}
onClick={onClose}
aria-label="Close usage dashboard"
/>Optionally, since aria-modal="true" on the dialog already restricts screen-reader virtual cursor navigation, adding aria-hidden="true" to this invisible button would prevent it from being surfaced by assistive technologies at all.
📝 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
| <div className="fixed inset-0 modal-overlay flex items-center justify-center z-[9999] animate-in fade-in duration-100"> | |
| <button | |
| type="button" | |
| className="absolute inset-0" | |
| tabIndex={-1} | |
| onClick={onClose} | |
| aria-label="Close usage dashboard" | |
| /> | |
| <button | |
| type="button" | |
| className="absolute inset-0 outline-none" | |
| tabIndex={-1} | |
| onClick={onClose} | |
| aria-label="Close usage dashboard" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx` around lines
493 - 500, The backdrop button in UsageDashboardModal is missing the required
focus style suppression; update the invisible backdrop <button> (the element
using tabIndex={-1} and onClick={onClose} inside the UsageDashboardModal
component) to include the "outline-none" class so it never shows a focus ring
when clicked, and optionally add aria-hidden="true" to prevent it being exposed
to assistive tech (keep tabIndex and onClick as-is). Ensure the change targets
the same button element that sits as the absolute inset-0 overlay.
- InputArea: replace useEffect+setTimeout autofocus with ref callback on
command history filter input; add tabIndex={0} per focus guidelines
- TerminalOutput: wrap log images in <button> for proper ARIA semantics
(fixes getByRole('img') test queries); add outline-none to auto-scroll
button per renderer focus guidelines
- UsageDashboardModal: use viewModeRef to avoid re-registering keydown
listener on every tab switch (matches existing onCloseRef pattern)
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/renderer/components/UsageDashboard/UsageDashboardModal.tsx (1)
495-510:⚠️ Potential issue | 🟡 MinorAdd
outline-noneto the backdrop button.This keeps focus styling consistent with renderer focus guidelines and avoids a visible outline flash on click.
As per coding guidelines, `src/renderer/**/*.{tsx,jsx}: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.`🛠️ Proposed fix
<button type="button" - className="absolute inset-0" + className="absolute inset-0 outline-none" tabIndex={-1} onClick={onClose} aria-label="Close usage dashboard" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx` around lines 495 - 510, The backdrop button inside UsageDashboardModal (the <button type="button" className="absolute inset-0" tabIndex={-1} onClick={onClose} aria-label="Close usage dashboard" />) needs the renderer focus styling applied: add the outline-none class to its className (and keep tabIndex={-1}) so clicks/focus don’t show a visible outline; follow the component guideline for focusable elements in UsageDashboardModal and apply similar outline-none to other backdrop buttons if present.src/renderer/components/TerminalOutput.tsx (2)
1616-1617:aria-label="Terminal output"remains ambiguous with multiple concurrent instances.This was flagged in the prior review. If multiple
TerminalOutputpanes are visible simultaneously, screen readers encounter identical region labels. Consider incorporating a session/tab identifier.🤖 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 1616 - 1617, The aria-label "Terminal output" on the TerminalOutput component is ambiguous when multiple panes exist; update the component to append a unique session/tab identifier (e.g., a prop like sessionId, tabId or index) to the aria-label (or switch to aria-labelledby referencing a unique header id) so each role="region" reads distinctly (update TerminalOutput props and where it is rendered to pass the identifier and use it in aria-label/aria-labelledby).
433-445:focus:ring-accentstill won't resolve — same issue flagged in the previous review.The duplicate key concern is now resolved with
key={${img}-${imgIdx}}. However,focus:ring-accenton line 436 still uses a Tailwind utility that requiresaccentto be registered as a ring color in your Tailwind config. Use an inline style for--tw-ring-coloras suggested in the prior review.Proposed fix
- className="shrink-0 p-0 bg-transparent outline-none focus:ring-2 focus:ring-accent rounded" + className="shrink-0 p-0 bg-transparent outline-none focus:ring-2 rounded" + style={{ ['--tw-ring-color' as any]: theme.colors.accent }}🤖 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 433 - 445, The focus:ring-accent utility is invalid because "accent" isn't registered in Tailwind; update the button used to open the lightbox (the element that calls setLightboxImage) to remove the class "focus:ring-accent" and instead set the ring color via an inline CSS variable for Tailwind rings (e.g. add a style prop that sets --tw-ring-color to the desired accent color, merging with any existing style), keeping the rest of the classes (focus:ring-2, rounded, etc.) intact so the focus ring still appears with the correct color.
🧹 Nitpick comments (1)
src/renderer/components/InputArea.tsx (1)
465-494: Inconsistent image-click pattern:role="button"on<img>vs.<button>wrapper in TerminalOutput.In
TerminalOutput.tsx(lines 433–445), the same type of clickable image is wrapped in a<button>element. Here, the<img>itself carriesrole="button"+tabIndex={0}+onKeyDown. Using a real<button>wrapper is preferred for native keyboard/activation semantics and is consistent with the approach taken in TerminalOutput.Additionally,
focus-visible:ring-accent(line 469) likely resolves to nothing becauseaccentisn't registered as a Tailwind ring color in the config. The past review on TerminalOutput flagged the same issue. Use an inline style for--tw-ring-colorlike is done elsewhere.Proposed fix: wrap img in button, use inline ring color
- <div key={img} className="relative group shrink-0"> - <img - src={img} - alt={`Staged image ${idx + 1}`} - className="h-16 rounded border cursor-pointer hover:opacity-80 transition-opacity outline-none focus-visible:ring-2 focus-visible:ring-accent" - style={{ - borderColor: theme.colors.border, - objectFit: 'contain', - maxWidth: '200px', - }} - role="button" - tabIndex={0} - onClick={() => setLightboxImage(img, stagedImages, 'staged')} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - setLightboxImage(img, stagedImages, 'staged'); - } - }} - /> + <div key={img} className="relative group shrink-0"> + <button + type="button" + className="shrink-0 p-0 bg-transparent outline-none focus-visible:ring-2 rounded" + style={{ ['--tw-ring-color' as any]: theme.colors.accent }} + onClick={() => setLightboxImage(img, stagedImages, 'staged')} + > + <img + src={img} + alt={`Staged image ${idx + 1}`} + className="h-16 rounded border cursor-pointer hover:opacity-80 transition-opacity block" + style={{ + borderColor: theme.colors.border, + objectFit: 'contain', + maxWidth: '200px', + }} + /> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 465 - 494, Wrap the staged image in a real <button> (like TerminalOutput) and move keyboard/click handlers from the <img> to that button: call setLightboxImage on button onClick and onKeyDown (or rely on button default activation), remove role="button" and tabIndex from the <img>, and keep the image purely presentational (objectFit/maxWidth styles remain). Make the focus ring use an inline CSS variable for --tw-ring-color set to theme.colors.accent (i.e., keep focus-visible:ring but set --tw-ring-color via the button's style) so Tailwind picks up the correct color. Ensure the existing remove button still stops propagation and setStagedImages behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1616-1617: The aria-label "Terminal output" on the TerminalOutput
component is ambiguous when multiple panes exist; update the component to append
a unique session/tab identifier (e.g., a prop like sessionId, tabId or index) to
the aria-label (or switch to aria-labelledby referencing a unique header id) so
each role="region" reads distinctly (update TerminalOutput props and where it is
rendered to pass the identifier and use it in aria-label/aria-labelledby).
- Around line 433-445: The focus:ring-accent utility is invalid because "accent"
isn't registered in Tailwind; update the button used to open the lightbox (the
element that calls setLightboxImage) to remove the class "focus:ring-accent" and
instead set the ring color via an inline CSS variable for Tailwind rings (e.g.
add a style prop that sets --tw-ring-color to the desired accent color, merging
with any existing style), keeping the rest of the classes (focus:ring-2,
rounded, etc.) intact so the focus ring still appears with the correct color.
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Around line 495-510: The backdrop button inside UsageDashboardModal (the
<button type="button" className="absolute inset-0" tabIndex={-1}
onClick={onClose} aria-label="Close usage dashboard" />) needs the renderer
focus styling applied: add the outline-none class to its className (and keep
tabIndex={-1}) so clicks/focus don’t show a visible outline; follow the
component guideline for focusable elements in UsageDashboardModal and apply
similar outline-none to other backdrop buttons if present.
---
Nitpick comments:
In `@src/renderer/components/InputArea.tsx`:
- Around line 465-494: Wrap the staged image in a real <button> (like
TerminalOutput) and move keyboard/click handlers from the <img> to that button:
call setLightboxImage on button onClick and onKeyDown (or rely on button default
activation), remove role="button" and tabIndex from the <img>, and keep the
image purely presentational (objectFit/maxWidth styles remain). Make the focus
ring use an inline CSS variable for --tw-ring-color set to theme.colors.accent
(i.e., keep focus-visible:ring but set --tw-ring-color via the button's style)
so Tailwind picks up the correct color. Ensure the existing remove button still
stops propagation and setStagedImages behavior is unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/components/InputArea.tsxsrc/renderer/components/TerminalOutput.tsxsrc/renderer/components/UsageDashboard/UsageDashboardModal.tsx
…ixes
Resolved merge conflict in TerminalOutput.tsx (kept outline-none on auto-scroll button).
Fixed 18 pre-existing test failures (9 from branch, 9 from main):
- LightboxModal: add onClick stopPropagation to img
- PromptComposerModal: add onClick to outer div, change onMouseDown to onClick
- UsageDashboardModal: add onClick to outer div, add stopPropagation to dialog
- DocumentsPanel: add onClick to outer div, add stopPropagation to inner modal
- InputArea/WizardInputPanel: replace img[role=button] with button>img wrapper
- GitStatusWidget: aria-hidden on compact span; tests use getAllByText for duplicates
- MainPanel: aria-hidden on compact Context span; tests use getByText('Context Window')
- InputArea.test: fix tab completion to use closest('button') not closest('div')
- WizardConversationView.test: advance fake timers before user scroll dispatch
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/renderer/components/InputArea.tsx (4)
507-533:⚠️ Potential issue | 🟡 MinorAdd focus styling + tabIndex for slash command items.
These buttons are now focusable; add explicit tabIndex and outline-none/focus-visible styles to meet renderer focus rules.
As per coding guidelines: `src/renderer/**/*.{tsx,jsx}`: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.♻️ Suggested fix
<button type="button" + tabIndex={0} key={cmd.command} ref={(el) => (slashCommandItemRefs.current[idx] = el)} - className={`w-full px-4 py-3 text-left transition-colors ${ + className={`w-full px-4 py-3 text-left transition-colors outline-none focus-visible:ring-2 focus-visible:ring-accent ${ idx === safeSelectedIndex ? 'font-semibold' : '' }`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 507 - 533, The slash command buttons need explicit keyboard focus support: add a tabIndex prop (e.g., tabIndex={idx === safeSelectedIndex ? 0 : -1}) and include focus styling classes like "outline-none focus-visible:ring" (or "focus-visible" plus your theme class) on the button rendered in the slash command list; also update the ref callback used in slashCommandItemRefs.current[idx] to call el?.focus() when idx === safeSelectedIndex so the active item receives auto-focus—keep the existing handlers (setSelectedSlashCommandIndex, setInputValue, setSlashCommandOpen, inputRef) intact.
591-617:⚠️ Potential issue | 🟡 MinorAdd tabIndex + focus styles for command history items.
These new buttons should include explicit tabIndex and outline-none/focus-visible classes per renderer focus rules.
As per coding guidelines: `src/renderer/**/*.{tsx,jsx}`: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.♻️ Suggested fix
<button type="button" + tabIndex={0} key={cmd} - className={`w-full px-3 py-2 text-left text-sm font-mono ${isSelected ? 'ring-1 ring-inset' : ''} ${isMostRecent ? 'font-semibold' : ''}`} + className={`w-full px-3 py-2 text-left text-sm font-mono outline-none focus-visible:ring-2 focus-visible:ring-accent ${isSelected ? 'ring-1 ring-inset' : ''} ${isMostRecent ? 'font-semibold' : ''}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 591 - 617, The command history <button> items lack explicit keyboard focus behavior; update the button rendered in InputArea.tsx (the element using setInputValue, setCommandHistoryOpen, setCommandHistoryFilter, inputRef, setCommandHistorySelectedIndex) to include tabIndex (use tabIndex={0} for focusable items or tabIndex={-1} for non-focusable ones as appropriate), add focus styling classes (e.g., outline-none and focus-visible or equivalent) to the className string, and when an item becomes selected (isSelected) auto-focus it by adding a ref={(el) => isSelected && el?.focus()} so keyboard users can tab/arrow into entries properly. Ensure these changes keep the existing onClick/onMouseEnter handlers and do not alter other logic.
710-747:⚠️ Potential issue | 🟡 MinorAdd tabIndex + focus styles for tab completion items.
These buttons need explicit tabIndex and outline-none/focus-visible styles to meet the renderer focus guideline.
As per coding guidelines: `src/renderer/**/*.{tsx,jsx}`: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.♻️ Suggested fix
<button type="button" + tabIndex={0} key={`${suggestion.type}-${suggestion.value}`} ref={(el) => (tabCompletionItemRefs.current[idx] = el)} - className={`w-full px-3 py-2 text-left text-sm font-mono flex items-center gap-2 ${isSelected ? 'ring-1 ring-inset' : ''}`} + className={`w-full px-3 py-2 text-left text-sm font-mono flex items-center gap-2 outline-none focus-visible:ring-2 focus-visible:ring-accent ${isSelected ? 'ring-1 ring-inset' : ''}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 710 - 747, The tab completion buttons need explicit keyboard focusability and focus styles: add tabIndex={0} to the button, include outline-none and an appropriate focus-visible class in the className string (e.g., focus-visible:ring or similar using theme colors), and update the ref assignment in the existing ref={(el) => (tabCompletionItemRefs.current[idx] = el)} to also auto-focus the currently selected item (e.g., ref={(el) => { tabCompletionItemRefs.current[idx] = el; if (isSelected) el?.focus(); }}). Change the button rendering inside InputArea.tsx (the button using tabCompletionItemRefs, setSelectedTabCompletionIndex, setTabCompletionOpen, inputRef, IconComponent) to apply these changes so keyboard tabbing and focus styles work correctly.
789-836:⚠️ Potential issue | 🟡 MinorAdd tabIndex + focus styles for
@-mentionitems.Same focus guideline applies here; these buttons should include explicit tabIndex and outline-none/focus-visible classes.
As per coding guidelines: `src/renderer/**/*.{tsx,jsx}`: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.♻️ Suggested fix
<button type="button" + tabIndex={0} key={`${suggestion.type}-${suggestion.value}`} ref={(el) => (atMentionItemRefs.current[idx] = el)} - className={`w-full px-3 py-2 text-left text-sm font-mono flex items-center gap-2 ${isSelected ? 'ring-1 ring-inset' : ''}`} + className={`w-full px-3 py-2 text-left text-sm font-mono flex items-center gap-2 outline-none focus-visible:ring-2 focus-visible:ring-accent ${isSelected ? 'ring-1 ring-inset' : ''}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` around lines 789 - 836, The `@-mention` suggestion buttons need explicit keyboard-focus support: add tabIndex={-1} to the button, add the classes outline-none and focus-visible:ring (or appropriate focus-visible styles) to the className, and update the ref callback (atMentionItemRefs.current[idx]) to also call el?.focus() when isSelected is true so keyboard navigation visually focuses the item; adjust setSelectedAtMentionIndex usage as needed to ensure isSelected controls programmatic focus. Ensure these changes are applied to the button in InputArea.tsx that references atMentionItemRefs, setSelectedAtMentionIndex, inputRef and the onClick/onMouseEnter handlers.
♻️ Duplicate comments (1)
src/renderer/components/UsageDashboard/UsageDashboardModal.tsx (1)
499-508:⚠️ Potential issue | 🟡 MinorBackdrop button still missing
outline-none— guideline violation (unresolved from prior review).The
classNameon line 501 is"absolute inset-0"with nooutline-none, so some browsers/OS themes will briefly render a focus ring on this invisible button when it receives focus after a click.🛠️ Proposed fix
- className="absolute inset-0" + className="absolute inset-0 outline-none"Based on learnings: "Add
tabIndex={0}ortabIndex={-1}andoutline-noneclass 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/UsageDashboard/UsageDashboardModal.tsx` around lines 499 - 508, The invisible backdrop button inside the UsageDashboardModal component currently has className="absolute inset-0" and can show a focus ring on some browsers; update that button's className to include the Tailwind utility to suppress focus outlines (e.g., add "outline-none" or "focus:outline-none") so it becomes "absolute inset-0 outline-none" (or include both "outline-none focus:outline-none" if desired); this change should be applied to the button element in UsageDashboardModal where tabIndex={-1} and onClick calls onClose().
🧹 Nitpick comments (4)
src/main/ipc/handlers/filesystem.ts (1)
288-305: Asymmetric binary support: local writes only accept strings while remote writes support binary.Verification of all call sites confirms callers only pass text (
contentisstringin MainPanel.tsx, UsageDashboardModal.tsx, and SaveMarkdownModal.tsx). However, the local write path forces UTF-8 encoding while the remotewriteFileRemote()already supportsstring | Bufferwith proper base64 encoding. To maintain consistency and support future binary write use cases, widen the IPC handler type:Suggested fix
- async (_, filePath: string, content: string, sshRemoteId?: string) => { + async (_, filePath: string, content: string | Buffer, sshRemoteId?: string) => { // ... - await fs.writeFile(filePath, content, 'utf-8'); + if (typeof content === 'string') { + await fs.writeFile(filePath, content, 'utf-8'); + } else { + await fs.writeFile(filePath, content); + } return { success: true }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/filesystem.ts` around lines 288 - 305, The IPC handler currently types content as string and always calls fs.writeFile with 'utf-8', causing asymmetry with writeFileRemote which accepts string|Buffer; change the handler signature to accept content: string | Buffer, update any IPC type defs accordingly, and in the local branch call fs.writeFile(filePath, content) (no forced 'utf-8' encoding) so Buffers are written correctly; keep the SSH branch using writeFileRemote(filePath, content, sshConfig) which already supports string|Buffer and error handling with getSshRemoteById and writeFileRemote remains unchanged.src/renderer/components/UsageDashboard/UsageDashboardModal.tsx (1)
494-498:onClick={onClose}on the outer wrapper div is now dead code.With the new backdrop
<button>usingabsolute inset-0+e.stopPropagation()(lines 499-508), and the modal container also callinge.stopPropagation()(line 516), no click event will ever reach this outer div — the button covers the full backdrop area and stops propagation, while the modal container blocks its own area. The outer handler can be removed.♻️ Proposed cleanup
<div className="fixed inset-0 modal-overlay flex items-center justify-center z-[9999] animate-in fade-in duration-100" - onClick={onClose} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx` around lines 494 - 498, The outer wrapper div in UsageDashboardModal has an onClick={onClose} that is now dead because the full-screen backdrop button (absolute inset-0) calls e.stopPropagation() and the modal container also stops propagation, so remove the outer onClick assignment; update the JSX in the UsageDashboardModal component to delete onClick={onClose} from the top-level wrapper and rely on the backdrop button's onClick handler (and modal container propagation guards) to close the modal via the existing onClose function.src/renderer/components/SettingsModal.tsx (2)
1243-1246: Redundantblockclass carried over to<div>replacements throughout the file.Every converted section-header
<div>(e.g. Lines 1243, 1279, 1563, 1609, 1688, 1744, 1814, 1956, 2257, 2640, 2721) retainsblockfrom the original<label>.<label>is inline by default soblockwas meaningful there;<div>is already block-level soblockis a no-op. Whenflexis also present (as it is in all of these), Tailwind's CSS ordering meansflexwins andblockis completely overridden. No functional impact, but it's vestigial noise from the label→div migration.🔧 Pattern to apply across all affected headings
-<div className="block text-xs font-bold opacity-70 uppercase mb-1 flex items-center gap-2"> +<div className="text-xs font-bold opacity-70 uppercase mb-1 flex items-center gap-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 1243 - 1246, Remove the redundant "block" Tailwind class from the section-header divs in SettingsModal.tsx — e.g., the div with className "block text-xs font-bold opacity-70 uppercase mb-1 flex items-center gap-2" that wraps <User /> and the "About Me" label, and the other converted heading divs that retained "block" during the label→div migration; simply delete the "block" token from those className strings so they read "text-xs font-bold opacity-70 uppercase mb-1 flex items-center gap-2" (and analogous className values) for each affected heading.
1440-1440: Input-specific labels converted to<div>rather than gaininghtmlFor/idassociations.Lines 1440 ("Custom Path"), 1470–1472 ("Additional Arguments"), 1571 ("Custom Path" for gh), 2199 ("API Key" for WakaTime), and 2929 ("API Key" for LLM) are labels that sit directly above a specific
<input>. The React Doctor concern here is the missinghtmlFor— the ideal fix is adding pairedhtmlFor+idattributes rather than stripping the<label>semantic entirely. Converting to<div>silences the warning but leaves the input without a programmatic label, which tools such as screen readers and automated a11y audits rely on.Consider addressing these in a Phase 2 pass:
-<div className="block text-xs opacity-60 mb-1">API Key</div> +<label htmlFor="wakatime-api-key" className="text-xs opacity-60 mb-1 block">API Key</label> … -<input type="password" value={wakatimeApiKey} … +<input id="wakatime-api-key" type="password" value={wakatimeApiKey} …Also note: Line 2557 (
Window Chromesection label) was not converted in this pass and still carries a bare<label>withouthtmlFor— that one can be swept up at the same time.Also applies to: 1470-1472, 1571-1571, 2199-2199, 2929-2929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` at line 1440, Several inline text blocks in SettingsModal (e.g., the "Custom Path", "Additional Arguments", WakaTime "API Key", LLM "API Key", and the "Window Chrome" section label) were changed from <label> to <div>, losing the htmlFor/id association; revert those <div className="block ..."> elements back to <label> and add matching htmlFor attributes, then give the corresponding <input> elements unique id attributes (e.g., customPathInput, additionalArgsInput,wakaApiKeyInput,llmApiKeyInput,windowChromeToggleInput) and ensure each label's htmlFor matches its input id; also update the remaining bare <label> in the Window Chrome section to reference its input via htmlFor so all inputs regain proper programmatic labels for accessibility.
🤖 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/main/ipc/handlers/filesystem.ts`:
- Around line 289-307: The catch is replacing the original error (and its
code/errno) with a new Error; update the error handling in this handler so you
either rethrow the original error or wrap it while preserving its properties:
when using getSshRemoteById, writeFileRemote, or fs.writeFile, avoid throwing
new Error(`Failed to write file: ${error}`); instead rethrow the caught error
(or attach its .code/.errno and message to a new error) and only map specific
known/recoverable error codes to friendly messages — leave unexpected errors
untouched so they bubble up to Sentry.
In `@src/renderer/components/InputArea.tsx`:
- Around line 465-488: The staged-image buttons lack explicit tabIndex which
breaks keyboard focus; update the image-opening button (the one calling
setLightboxImage) and the delete button (the one calling setStagedImages) to
include an explicit tabIndex (use tabIndex={0} for focusable or tabIndex={-1} if
intentionally removed), add the outline-none class as required by the renderer
guideline, and if either button should auto-focus when rendered, add a ref prop
like ref={(el) => el?.focus()} to that button; ensure these changes apply to the
buttons wrapping the <img> (setLightboxImage) and the absolute delete button
(setStagedImages) so keyboard navigation and focus-visible styles work
correctly.
---
Outside diff comments:
In `@src/renderer/components/InputArea.tsx`:
- Around line 507-533: The slash command buttons need explicit keyboard focus
support: add a tabIndex prop (e.g., tabIndex={idx === safeSelectedIndex ? 0 :
-1}) and include focus styling classes like "outline-none focus-visible:ring"
(or "focus-visible" plus your theme class) on the button rendered in the slash
command list; also update the ref callback used in
slashCommandItemRefs.current[idx] to call el?.focus() when idx ===
safeSelectedIndex so the active item receives auto-focus—keep the existing
handlers (setSelectedSlashCommandIndex, setInputValue, setSlashCommandOpen,
inputRef) intact.
- Around line 591-617: The command history <button> items lack explicit keyboard
focus behavior; update the button rendered in InputArea.tsx (the element using
setInputValue, setCommandHistoryOpen, setCommandHistoryFilter, inputRef,
setCommandHistorySelectedIndex) to include tabIndex (use tabIndex={0} for
focusable items or tabIndex={-1} for non-focusable ones as appropriate), add
focus styling classes (e.g., outline-none and focus-visible or equivalent) to
the className string, and when an item becomes selected (isSelected) auto-focus
it by adding a ref={(el) => isSelected && el?.focus()} so keyboard users can
tab/arrow into entries properly. Ensure these changes keep the existing
onClick/onMouseEnter handlers and do not alter other logic.
- Around line 710-747: The tab completion buttons need explicit keyboard
focusability and focus styles: add tabIndex={0} to the button, include
outline-none and an appropriate focus-visible class in the className string
(e.g., focus-visible:ring or similar using theme colors), and update the ref
assignment in the existing ref={(el) => (tabCompletionItemRefs.current[idx] =
el)} to also auto-focus the currently selected item (e.g., ref={(el) => {
tabCompletionItemRefs.current[idx] = el; if (isSelected) el?.focus(); }}).
Change the button rendering inside InputArea.tsx (the button using
tabCompletionItemRefs, setSelectedTabCompletionIndex, setTabCompletionOpen,
inputRef, IconComponent) to apply these changes so keyboard tabbing and focus
styles work correctly.
- Around line 789-836: The `@-mention` suggestion buttons need explicit
keyboard-focus support: add tabIndex={-1} to the button, add the classes
outline-none and focus-visible:ring (or appropriate focus-visible styles) to the
className, and update the ref callback (atMentionItemRefs.current[idx]) to also
call el?.focus() when isSelected is true so keyboard navigation visually focuses
the item; adjust setSelectedAtMentionIndex usage as needed to ensure isSelected
controls programmatic focus. Ensure these changes are applied to the button in
InputArea.tsx that references atMentionItemRefs, setSelectedAtMentionIndex,
inputRef and the onClick/onMouseEnter handlers.
---
Duplicate comments:
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Around line 499-508: The invisible backdrop button inside the
UsageDashboardModal component currently has className="absolute inset-0" and can
show a focus ring on some browsers; update that button's className to include
the Tailwind utility to suppress focus outlines (e.g., add "outline-none" or
"focus:outline-none") so it becomes "absolute inset-0 outline-none" (or include
both "outline-none focus:outline-none" if desired); this change should be
applied to the button element in UsageDashboardModal where tabIndex={-1} and
onClick calls onClose().
---
Nitpick comments:
In `@src/main/ipc/handlers/filesystem.ts`:
- Around line 288-305: The IPC handler currently types content as string and
always calls fs.writeFile with 'utf-8', causing asymmetry with writeFileRemote
which accepts string|Buffer; change the handler signature to accept content:
string | Buffer, update any IPC type defs accordingly, and in the local branch
call fs.writeFile(filePath, content) (no forced 'utf-8' encoding) so Buffers are
written correctly; keep the SSH branch using writeFileRemote(filePath, content,
sshConfig) which already supports string|Buffer and error handling with
getSshRemoteById and writeFileRemote remains unchanged.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 1243-1246: Remove the redundant "block" Tailwind class from the
section-header divs in SettingsModal.tsx — e.g., the div with className "block
text-xs font-bold opacity-70 uppercase mb-1 flex items-center gap-2" that wraps
<User /> and the "About Me" label, and the other converted heading divs that
retained "block" during the label→div migration; simply delete the "block" token
from those className strings so they read "text-xs font-bold opacity-70
uppercase mb-1 flex items-center gap-2" (and analogous className values) for
each affected heading.
- Line 1440: Several inline text blocks in SettingsModal (e.g., the "Custom
Path", "Additional Arguments", WakaTime "API Key", LLM "API Key", and the
"Window Chrome" section label) were changed from <label> to <div>, losing the
htmlFor/id association; revert those <div className="block ..."> elements back
to <label> and add matching htmlFor attributes, then give the corresponding
<input> elements unique id attributes (e.g., customPathInput,
additionalArgsInput,wakaApiKeyInput,llmApiKeyInput,windowChromeToggleInput) and
ensure each label's htmlFor matches its input id; also update the remaining bare
<label> in the Window Chrome section to reference its input via htmlFor so all
inputs regain proper programmatic labels for accessibility.
In `@src/renderer/components/UsageDashboard/UsageDashboardModal.tsx`:
- Around line 494-498: The outer wrapper div in UsageDashboardModal has an
onClick={onClose} that is now dead because the full-screen backdrop button
(absolute inset-0) calls e.stopPropagation() and the modal container also stops
propagation, so remove the outer onClick assignment; update the JSX in the
UsageDashboardModal component to delete onClick={onClose} from the top-level
wrapper and rely on the backdrop button's onClick handler (and modal container
propagation guards) to close the modal via the existing onClose function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/__tests__/main/ipc/handlers/filesystem.test.tssrc/__tests__/renderer/components/GitStatusWidget.test.tsxsrc/__tests__/renderer/components/InlineWizard/WizardConversationView.test.tsxsrc/__tests__/renderer/components/InputArea.test.tsxsrc/__tests__/renderer/components/MainPanel.test.tsxsrc/__tests__/renderer/hooks/useSettings.test.tssrc/__tests__/renderer/utils/markdownConfig.test.tssrc/main/ipc/handlers/attachments.tssrc/main/ipc/handlers/filesystem.tssrc/renderer/components/DocumentsPanel.tsxsrc/renderer/components/GitStatusWidget.tsxsrc/renderer/components/InlineWizard/WizardInputPanel.tsxsrc/renderer/components/InlineWizard/WizardMessageBubble.tsxsrc/renderer/components/InputArea.tsxsrc/renderer/components/LightboxModal.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/PromptComposerModal.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/components/UsageDashboard/UsageDashboardModal.tsxsrc/renderer/hooks/batch/useInlineWizard.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/session/useHandsOnTimeTracker.tssrc/renderer/stores/settingsStore.ts
✅ Files skipped from review due to trivial changes (7)
- src/renderer/hooks/batch/useInlineWizard.ts
- src/tests/renderer/hooks/useSettings.test.ts
- src/renderer/hooks/session/useHandsOnTimeTracker.ts
- src/renderer/stores/settingsStore.ts
- src/tests/renderer/utils/markdownConfig.test.ts
- src/tests/main/ipc/handlers/filesystem.test.ts
- src/renderer/components/InlineWizard/WizardMessageBubble.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/components/InlineWizard/WizardInputPanel.tsx
- src/renderer/components/DocumentsPanel.tsx
- src/renderer/components/PromptComposerModal.tsx
Comment on lines
+289
to
+307
| try { | ||
| // SSH remote: dispatch to remote fs operations | ||
| if (sshRemoteId) { | ||
| const sshConfig = getSshRemoteById(sshRemoteId); | ||
| if (!sshConfig) { | ||
| throw new Error(`SSH remote not found: ${sshRemoteId}`); | ||
| } | ||
| const result = await writeFileRemote(filePath, content, sshConfig); | ||
| if (!result.success) { | ||
| throw new Error(result.error || 'Failed to write remote file'); | ||
| } | ||
| return { success: true }; | ||
| } | ||
|
|
||
| // Local: use standard fs operations | ||
| await fs.writeFile(filePath, content, 'utf-8'); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| throw new Error(`Failed to write file: ${error}`); |
There was a problem hiding this comment.
Avoid blanket catch that strips error details.
Catching the whole handler and throwing a new Error drops code/errno (e.g., EACCES, ENOENT) and conflicts with the guideline to let unexpected errors bubble. Prefer rethrowing the original error or only mapping known, recoverable ones.
✅ Suggested fix (preserve original errors)
- try {
+ // Let unexpected errors bubble up to preserve codes/stack
// SSH remote: dispatch to remote fs operations
if (sshRemoteId) {
const sshConfig = getSshRemoteById(sshRemoteId);
if (!sshConfig) {
throw new Error(`SSH remote not found: ${sshRemoteId}`);
}
const result = await writeFileRemote(filePath, content, sshConfig);
if (!result.success) {
throw new Error(result.error || 'Failed to write remote file');
}
return { success: true };
}
// Local: use standard fs operations
await fs.writeFile(filePath, content, 'utf-8');
return { success: true };
- } catch (error) {
- throw new Error(`Failed to write file: ${error}`);
- }As per coding guidelines: “Handle only expected/recoverable errors explicitly with specific error codes. Let unexpected errors bubble up to Sentry.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/filesystem.ts` around lines 289 - 307, The catch is
replacing the original error (and its code/errno) with a new Error; update the
error handling in this handler so you either rethrow the original error or wrap
it while preserving its properties: when using getSshRemoteById,
writeFileRemote, or fs.writeFile, avoid throwing new Error(`Failed to write
file: ${error}`); instead rethrow the caught error (or attach its .code/.errno
and message to a new error) and only map specific known/recoverable error codes
to friendly messages — leave unexpected errors untouched so they bubble up to
Sentry.
Comment on lines
+465
to
488
| <div key={img} className="relative group shrink-0"> | ||
| <button | ||
| type="button" | ||
| className="p-0 bg-transparent outline-none focus-visible:ring-2 focus-visible:ring-accent rounded" | ||
| onClick={() => setLightboxImage(img, stagedImages, 'staged')} | ||
| /> | ||
| > | ||
| <img | ||
| src={img} | ||
| alt={`Staged image ${idx + 1}`} | ||
| className="h-16 rounded border cursor-pointer hover:opacity-80 transition-opacity block" | ||
| style={{ | ||
| borderColor: theme.colors.border, | ||
| objectFit: 'contain', | ||
| maxWidth: '200px', | ||
| }} | ||
| /> | ||
| </button> | ||
| <button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setStagedImages((p) => p.filter((_, i) => i !== idx)); | ||
| }} | ||
| className="absolute top-0.5 right-0.5 bg-red-500 text-white rounded-full p-1 shadow-md hover:bg-red-600 transition-colors opacity-90 hover:opacity-100" | ||
| className="absolute top-0.5 right-0.5 bg-red-500 text-white rounded-full p-1 shadow-md hover:bg-red-600 transition-colors opacity-90 hover:opacity-100 outline-none focus-visible:ring-2 focus-visible:ring-white" | ||
| > |
There was a problem hiding this comment.
Add tabIndex to staged-image buttons.
The new buttons already have focus-visible styles, but the renderer guideline also requires explicit tabIndex.
♻️ Suggested fix
<button
type="button"
+ tabIndex={0}
className="p-0 bg-transparent outline-none focus-visible:ring-2 focus-visible:ring-accent rounded"
onClick={() => setLightboxImage(img, stagedImages, 'staged')}
>
@@
<button
+ tabIndex={0}
onClick={(e) => {
e.stopPropagation();
setStagedImages((p) => p.filter((_, i) => i !== idx));
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/InputArea.tsx` around lines 465 - 488, The
staged-image buttons lack explicit tabIndex which breaks keyboard focus; update
the image-opening button (the one calling setLightboxImage) and the delete
button (the one calling setStagedImages) to include an explicit tabIndex (use
tabIndex={0} for focusable or tabIndex={-1} if intentionally removed), add the
outline-none class as required by the renderer guideline, and if either button
should auto-focus when rendered, add a ref prop like ref={(el) => el?.focus()}
to that button; ensure these changes apply to the buttons wrapping the <img>
(setLightboxImage) and the absolute delete button (setStagedImages) so keyboard
navigation and focus-visible styles work correctly.
This was referenced Feb 23, 2026
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
allSessionsprop inSendToAgentModaltest fixtureWhy this scope
React Doctor reported a large backlog (
22 errors,807 warnings). This PR targets high-confidence, low-risk fixes only.Validation
npm run test -- src/__tests__/renderer/components/SendToAgentModal.test.tsxnpx eslint src/renderer/components/TerminalOutput.tsx src/renderer/components/InlineWizard/WizardInputPanel.tsx src/renderer/components/LightboxModal.tsx src/renderer/components/InputArea.tsx src/renderer/components/PromptComposerModal.tsxnpx react-doctor . --yes --offline --no-ami(branch diff scan)Tracking
Summary by CodeRabbit
Improvements
Refactor
Tests