refactor: decompose SettingsModal into per-tab components#502
Open
refactor: decompose SettingsModal into per-tab components#502
Conversation
Extract 6 components from the monolithic SettingsModal.tsx (3,649 → 601 lines, −84%): - EnvVarsEditor: environment variable editing sub-component (219 lines) - GeneralTab: shell, input, toggles, WakaTime, sync/stats (1,530 lines) - DisplayTab: fonts, terminal width, log buffer, alignment (558 lines) - EncoreTab: feature flags, Director's Notes agent config (509 lines) - ShortcutsTab: shortcut recording, filtering, escape coordination (215 lines) - ThemeTab: theme selection, grouping, keyboard navigation (162 lines) Each tab self-sources settings via useSettings() and owns its local state. SettingsModal.tsx retains modal chrome, tab bar, layer stack, and tab routing. 308 new tests across 6 test files; 416 total Settings tests passing.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Settings modal refactoring with five specialized tab components (GeneralTab, DisplayTab, EncoreTab, ShortcutsTab, ThemeTab) for managing application settings, along with a new EnvVarsEditor component for environment variable management. Extensive test suites and index exports are included for all new functionality. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR successfully decomposes a 3,649-line monolithic Key findings:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
SM["SettingsModal.tsx\n(modal chrome + tab bar)"]
SM -->|activeTab = general| GT["GeneralTab\n(shell, input, WakaTime, sync, stats)"]
SM -->|activeTab = display| DT["DisplayTab\n(fonts, width, buffer, alignment)"]
SM -->|activeTab = shortcuts| ST["ShortcutsTab\n(recording, filtering, escape coord)"]
SM -->|activeTab = theme| TT["ThemeTab\n(grouping, keyboard nav)"]
SM -->|activeTab = encore| ET["EncoreTab\n(feature flags, Director's Notes)"]
SM -->|activeTab = notifications| NP["NotificationsPanel\n(existing component)"]
SM -->|activeTab = aicommands| ACP["AICommandsPanel\n(existing component)"]
SM -->|activeTab = ssh| SSH["SshRemotesSection\n(existing component)"]
GT --> EVE["EnvVarsEditor\n(env var add/edit/remove)"]
GT & DT & ST & TT & ET -->|self-sources| US["useSettings()"]
SM -->|retains for LLM + notifications + ssh| US
US --> SS["settingsStore"]
Last reviewed commit: 7baef09 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| defaultShell === shell.id ? theme.colors.accentDim : theme.colors.bgMain, | ||
| '--tw-ring-color': theme.colors.accent, | ||
| color: theme.colors.textMain, | ||
| } as React.CSSProperties |
There was a problem hiding this comment.
React not imported but used as a namespace
React.CSSProperties is referenced here (and again at line 411), but GeneralTab.tsx only imports named exports from 'react' — it never imports React as a default:
import { useState, useEffect, useCallback } from 'react';Every other tab in this PR that uses the same pattern (ShortcutsTab.tsx, ThemeTab.tsx) imports React explicitly. This will fail TypeScript compilation with an error such as 'React' refers to a UMD global, but the current file is a module.
Suggested change
| } as React.CSSProperties | |
| } as import('react').CSSProperties |
Or, more consistently with the rest of the codebase, add React to the import at the top of the file:
import React, { useState, useEffect, useCallback } from 'react';The same fix is needed at line 411.
Comment on lines
+88
to
+110
| useEffect(() => { | ||
| const parentEntries = Object.entries(envVars); | ||
| // Only reset if the keys/values actually differ | ||
| const currentKeys = entries | ||
| .filter((e) => e.key.trim()) | ||
| .map((e) => `${e.key}=${e.value}`) | ||
| .sort() | ||
| .join(','); | ||
| const parentKeys = parentEntries | ||
| .map(([k, v]) => `${k}=${v}`) | ||
| .sort() | ||
| .join(','); | ||
| if (currentKeys !== parentKeys) { | ||
| setEntries( | ||
| parentEntries.map(([key, value], index) => ({ | ||
| id: index, | ||
| key, | ||
| value, | ||
| })) | ||
| ); | ||
| setNextId(parentEntries.length); | ||
| } | ||
| }, [envVars]); |
There was a problem hiding this comment.
Stale closure: entries used inside useEffect but omitted from dependencies
The effect reads entries to build currentKeys, but entries is not listed in the dependency array — only envVars is. When envVars changes (e.g. the parent re-opens the modal), React will re-run this closure with a potentially stale snapshot of entries, making the currentKeys !== parentKeys guard unreliable. This can lead to either:
- Missed resets: if the stale
entriesaccidentally produces a matching string, the UI won't sync to the newenvVars. - Spurious resets: if the stale value differs from the already-correct current state, in-progress edits are wiped.
The safest fix is to use the functional form of setEntries so that entries is never captured stale:
useEffect(() => {
const parentEntries = Object.entries(envVars);
const parentKeys = parentEntries
.map(([k, v]) => `${k}=${v}`)
.sort()
.join(',');
setEntries((current) => {
const currentKeys = current
.filter((e) => e.key.trim())
.map((e) => `${e.key}=${e.value}`)
.sort()
.join(',');
if (currentKeys === parentKeys) return current; // no change
setNextId(parentEntries.length);
return parentEntries.map(([key, value], index) => ({ id: index, key, value }));
});
}, [envVars]);Note: setNextId inside setEntries is not ideal (two separate state updates), but it avoids the stale closure. Alternatively, merging entries and nextId into a single state object would be cleaner.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
src/renderer/components/Settings/EnvVarsEditor.tsx (1)
159-164: Remove the no-op conditional class fragment.This ternary always resolves to an empty string, so it doesn’t affect rendering and adds noise.
🧹 Proposed cleanup
- className={`flex-1 p-2 rounded border bg-transparent outline-none text-xs font-mono ${ - entry.key.trim() && - !validateEntry({ id: entry.id, key: entry.key, value: entry.value }) - ? '' - : '' - }`} + className="flex-1 p-2 rounded border bg-transparent outline-none text-xs font-mono"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/EnvVarsEditor.tsx` around lines 159 - 164, The className string contains a no-op ternary that always returns an empty string; remove the conditional fragment and simplify the className for the input element in EnvVarsEditor so it no longer includes the useless expression referencing entry.key and validateEntry({ id: entry.id, key: entry.key, value: entry.value }); update the JSX for that element (the input/textarea rendering within EnvVarsEditor) to use a single static or meaningful dynamic class list without the empty ternary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/Settings/EnvVarsEditor.test.tsx`:
- Around line 139-142: The test currently can pass without exercising the
invalid-key assertion because it only checks mockSetEnvVars.mock.calls length
conditionally; change the assertion to require that mockSetEnvVars was called
and then inspect its last call. Specifically, assert that mockSetEnvVars was
called (e.g., toHaveBeenCalled or toHaveBeenCalledTimes) and then read the last
call from mockSetEnvVars.mock.calls[...] to assert that ['MY-VAR'] is undefined;
keep the reference to mockSetEnvVars and the lastCall extraction logic
(mockSetEnvVars.mock.calls and lastCall) but make the call presence check
deterministic instead of conditional so the test fails if setEnvVars is never
invoked.
In `@src/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx`:
- Around line 818-830: The test hardcodes the platform-specific label "Ctrl +
Enter" causing brittleness; instead, change the expectation in the GeneralTab
test to use the same formatter or a platform-safe assertion: call or import
formatEnterToSend(false) and assert toggleButton?.textContent equals that value
(or assert it contains "Enter" and either "Ctrl" or "⌘"), referencing
formatEnterToSend, GeneralTab, and the enterToSendAI setting so the test passes
on macOS and other platforms.
In `@src/__tests__/renderer/components/Settings/tabs/ThemeTab.test.tsx`:
- Around line 322-339: The test currently only asserts CustomThemeBuilder is
rendered but doesn't verify the import callbacks are forwarded; update the test
to find the CustomThemeBuilder (screen.getByTestId('custom-theme-builder')) and
call its forwarded props for import error and success to assert
onThemeImportError and onThemeImportSuccess (the vi.fn() mocks) are invoked;
specifically, after rendering ThemeTab, access the component instance or
element, read its props-like handlers (e.g., getByTestId(...).props or use
ReactTestUtils/act to invoke the child callbacks) and add
expect(onError).toHaveBeenCalledWith(...) and
expect(onSuccess).toHaveBeenCalledWith(...) assertions to confirm wiring in
ThemeTab -> CustomThemeBuilder.
- Around line 240-243: The assertion is scoped to the wrong DOM node because
darkSection is set via screen.getByText('dark Mode').closest('div'), which
targets the label wrapper and can miss the theme buttons; update the query so
darkButtons come from the actual theme-group container (e.g., locate the
section/fieldset that wraps the dark-mode themes using a more specific selector
or accessible query such as getByRole/getByTestId/getByLabelText, or use
within() on that container) then recompute darkThemeIds and assert it does not
contain 'custom'; adjust references to darkSection, darkButtons, and
darkThemeIds accordingly.
In `@src/__tests__/renderer/components/SettingsModal.test.tsx`:
- Around line 1671-1675: The test currently calls capturedEscapeHandler?.()
which can mask missing registration; update the test in SettingsModal.test.tsx
to assert capturedEscapeHandler is defined before invoking it (e.g.,
expect(capturedEscapeHandler).toBeDefined() or toBeTruthy()), then call
capturedEscapeHandler(); ensure you reference the capturedEscapeHandler variable
used in the escape simulation and keep the existing comments about ShortcutsTab
handling escape intact.
In `@src/renderer/components/Settings/EnvVarsEditor.tsx`:
- Around line 76-81: The loop that builds newEnvVars from newEntries silently
overwrites earlier values when two entries share the same entry.key; change the
logic in the block that iterates newEntries so it tracks seen keys (e.g., a Set)
and when encountering a duplicate key it does NOT assign into newEnvVars and
instead adds/sets a validation error for that entry (use the existing errors map
keyed by entry.id), leaving the first-occurrence value intact; update the
handling around newEntries/newEnvVars/errors to prevent clobbering and surface
duplicates to the user.
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx`:
- Around line 72-75: Wrap all calls to window.maestro.agents.getConfig and
agents.setConfig in proper async error handling (either use try/catch in an
async function or append .catch) so transient backend failures don't produce
unhandled rejections or leave stale UI; on error log the error (e.g., via
console.error or the existing logger), setDnAgentConfig to a safe fallback ({}),
update dnAgentConfigRef.current to the same fallback, and surface a non-blocking
user-facing error state if available; apply the same pattern around the other
usages of getConfig/setConfig in this file (referencing
window.maestro.agents.getConfig, setDnAgentConfig, dnAgentConfigRef, and
agents.setConfig) so every external call is guarded and state is kept consistent
on failure.
- Around line 60-67: The effect that syncs local DN state (useEffect) currently
has an empty dependency array so it only runs on mount; change its dependency
array to include directorNotesSettings (and any prop indicating the tab is
active, if present) so setDnCustomPath, setDnCustomArgs, setDnCustomEnvVars, and
setDnIsConfigExpanded are updated whenever directorNotesSettings changes (or the
tab is reopened) — locate the useEffect block referencing those setters and
replace [] with [directorNotesSettings] (or [directorNotesSettings, isActiveTab]
if an is-active-tab prop/state exists).
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 786-799: The switch buttons with role="switch" are missing
accessible names; update each switch element (e.g., the one toggling
preventSleepEnabled via setPreventSleepEnabled in GeneralTab.tsx) to include an
explicit accessible label using aria-label or aria-labelledby that describes the
control (e.g., "Prevent computer from sleeping") and preserve aria-checked
binding; apply the same fix to the other switch buttons referenced (the switches
around lines 854-867, 902-913, 986-998, 1164-1172, 1199-1209) so every
role="switch" has a meaningful aria-label or aria-labelledby matching the state
it toggles.
- Around line 1344-1368: The click handler currently awaits
window.maestro.sync.selectSyncFolder and window.maestro.sync.setCustomPath
inside a try/finally but has no catch, so promise rejections can become
unhandled and skip setting user-visible error state; wrap the awaits in a
try/catch/finally (or add an inner catch) for both onClick handlers that call
selectSyncFolder and setCustomPath, and in the catch setSyncError with the
caught error message (or a fallback string) and ensure setSyncMigrating(false)
still runs in finally; update references in the handler to the symbols
setSyncMigrating, setSyncError, setSyncMigratedCount, setSyncRestartRequired,
setCustomSyncPath, and setCurrentStoragePath so error cases properly update UI
state.
- Around line 256-274: The detect button becomes inert when detection returns an
empty array because shellsLoaded is set true and handleShellInteraction prevents
further calls; update the logic so an empty detection does not permanently mark
shells as loaded — either (A) in loadShells only call setShellsLoaded(true) when
detected.length > 0 (and still setShells([]) / shellsLoading false on empty), or
(B) change handleShellInteraction to call loadShells when shellsLoaded is true
but shells.length === 0; apply the same fix to the other occurrence that uses
the same pattern (the other handleShellInteraction-like block referenced in the
file).
- Around line 536-545: The focusable help-icon wrapper (the div with className
"group relative flex-shrink-0 mt-0.5 outline-none" that contains <HelpCircle />
and uses tabIndex={0}) is missing explicit keyboard focus handlers; add onFocus
and onBlur handlers to that wrapper (or corresponding focus-visible handlers) to
toggle the same visibility state used by hover (so the tooltip div currently
shown via "group-hover:block group-focus-visible:block" is reliably shown/hidden
for keyboard users), and ensure the handlers update any local state or call the
same CSS-visible class logic so keyboard focus triggers the tooltip just like
mouse hover.
In `@src/renderer/components/Settings/tabs/ShortcutsTab.tsx`:
- Around line 27-31: The current useEffect in ShortcutsTab calls
onRecordingChange when recordingId changes but has no cleanup on unmount; modify
the useEffect (the one referencing useEffect, onRecordingChange, and
recordingId) to return a cleanup function that calls onRecordingChange?.(false)
so the parent is notified if the tab unmounts during a recording, ensuring no
stale "recording" state remains; keep onRecordingChange and recordingId in the
dependency array.
In `@src/renderer/components/Settings/tabs/ThemeTab.tsx`:
- Around line 55-89: The Tab handling in handleThemePickerKeyDown currently
prevents Tab at the root which blocks focus inside nested controls (e.g.,
CustomThemeBuilder); change the logic to only intercept and call
e.preventDefault()/e.stopPropagation() when the Tab originated on a theme item
inside the picker (use themePickerRef.current.contains(e.target as Node) AND
check the target or its closest ancestor has a data-theme-id or matches your
theme button selector), otherwise let the event propagate so nested
inputs/buttons keep normal focus behavior; apply the same targeted-check
approach to the other Tab handler block referenced (lines 93–99) that also
prevents Tab.
---
Nitpick comments:
In `@src/renderer/components/Settings/EnvVarsEditor.tsx`:
- Around line 159-164: The className string contains a no-op ternary that always
returns an empty string; remove the conditional fragment and simplify the
className for the input element in EnvVarsEditor so it no longer includes the
useless expression referencing entry.key and validateEntry({ id: entry.id, key:
entry.key, value: entry.value }); update the JSX for that element (the
input/textarea rendering within EnvVarsEditor) to use a single static or
meaningful dynamic class list without the empty ternary.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/__tests__/renderer/components/Settings/EnvVarsEditor.test.tsxsrc/__tests__/renderer/components/Settings/tabs/DisplayTab.test.tsxsrc/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsxsrc/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsxsrc/__tests__/renderer/components/Settings/tabs/ShortcutsTab.test.tsxsrc/__tests__/renderer/components/Settings/tabs/ThemeTab.test.tsxsrc/__tests__/renderer/components/SettingsModal.test.tsxsrc/renderer/components/Settings/EnvVarsEditor.tsxsrc/renderer/components/Settings/index.tssrc/renderer/components/Settings/tabs/DisplayTab.tsxsrc/renderer/components/Settings/tabs/EncoreTab.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/components/Settings/tabs/ShortcutsTab.tsxsrc/renderer/components/Settings/tabs/ThemeTab.tsxsrc/renderer/components/Settings/tabs/index.tssrc/renderer/components/SettingsModal.tsx
Comment on lines
+139
to
+142
| if (mockSetEnvVars.mock.calls.length > 0) { | ||
| const lastCall = mockSetEnvVars.mock.calls[mockSetEnvVars.mock.calls.length - 1][0]; | ||
| expect(lastCall['MY-VAR']).toBeUndefined(); | ||
| } |
There was a problem hiding this comment.
This assertion can skip validation entirely.
If setEnvVars is never called, the test still passes without checking the invalid-key invariant.
Suggested deterministic assertion
-// The setEnvVars should NOT have been called with this invalid entry
-if (mockSetEnvVars.mock.calls.length > 0) {
- const lastCall = mockSetEnvVars.mock.calls[mockSetEnvVars.mock.calls.length - 1][0];
- expect(lastCall['MY-VAR']).toBeUndefined();
-}
+// Ensure no emitted payload ever contains the invalid key
+expect(
+ mockSetEnvVars.mock.calls.every(([vars]) => vars['MY-VAR'] === undefined)
+).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/Settings/EnvVarsEditor.test.tsx` around
lines 139 - 142, The test currently can pass without exercising the invalid-key
assertion because it only checks mockSetEnvVars.mock.calls length conditionally;
change the assertion to require that mockSetEnvVars was called and then inspect
its last call. Specifically, assert that mockSetEnvVars was called (e.g.,
toHaveBeenCalled or toHaveBeenCalledTimes) and then read the last call from
mockSetEnvVars.mock.calls[...] to assert that ['MY-VAR'] is undefined; keep the
reference to mockSetEnvVars and the lastCall extraction logic
(mockSetEnvVars.mock.calls and lastCall) but make the call presence check
deterministic instead of conditional so the test fails if setEnvVars is never
invoked.
Comment on lines
+818
to
+830
| it('should show Ctrl + Enter label when enterToSendAI is false', async () => { | ||
| mockUseSettingsOverrides = { enterToSendAI: false }; | ||
| render(<GeneralTab theme={mockTheme} isOpen={true} />); | ||
|
|
||
| await act(async () => { | ||
| await vi.advanceTimersByTimeAsync(100); | ||
| }); | ||
|
|
||
| const aiModeLabel = screen.getByText('AI Interaction Mode'); | ||
| const aiModeSection = aiModeLabel.closest('.p-3'); | ||
| const toggleButton = aiModeSection?.querySelector('button'); | ||
| expect(toggleButton?.textContent).toBe('Ctrl + Enter'); | ||
| }); |
There was a problem hiding this comment.
Platform-specific expectation is hardcoded.
formatEnterToSend(false) can produce ⌘ + Enter on macOS, so asserting only Ctrl + Enter is brittle.
Suggested platform-safe assertion
-expect(toggleButton?.textContent).toBe('Ctrl + Enter');
+expect(toggleButton?.textContent).toMatch(/^(Ctrl \+ Enter|⌘ \+ Enter)$/);📝 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
| it('should show Ctrl + Enter label when enterToSendAI is false', async () => { | |
| mockUseSettingsOverrides = { enterToSendAI: false }; | |
| render(<GeneralTab theme={mockTheme} isOpen={true} />); | |
| await act(async () => { | |
| await vi.advanceTimersByTimeAsync(100); | |
| }); | |
| const aiModeLabel = screen.getByText('AI Interaction Mode'); | |
| const aiModeSection = aiModeLabel.closest('.p-3'); | |
| const toggleButton = aiModeSection?.querySelector('button'); | |
| expect(toggleButton?.textContent).toBe('Ctrl + Enter'); | |
| }); | |
| it('should show Ctrl + Enter label when enterToSendAI is false', async () => { | |
| mockUseSettingsOverrides = { enterToSendAI: false }; | |
| render(<GeneralTab theme={mockTheme} isOpen={true} />); | |
| await act(async () => { | |
| await vi.advanceTimersByTimeAsync(100); | |
| }); | |
| const aiModeLabel = screen.getByText('AI Interaction Mode'); | |
| const aiModeSection = aiModeLabel.closest('.p-3'); | |
| const toggleButton = aiModeSection?.querySelector('button'); | |
| expect(toggleButton?.textContent).toMatch(/^(Ctrl \+ Enter|⌘ \+ Enter)$/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx` around
lines 818 - 830, The test hardcodes the platform-specific label "Ctrl + Enter"
causing brittleness; instead, change the expectation in the GeneralTab test to
use the same formatter or a platform-safe assertion: call or import
formatEnterToSend(false) and assert toggleButton?.textContent equals that value
(or assert it contains "Enter" and either "Ctrl" or "⌘"), referencing
formatEnterToSend, GeneralTab, and the enterToSendAI setting so the test passes
on macOS and other platforms.
Comment on lines
+240
to
+243
| const darkSection = screen.getByText('dark Mode').closest('div'); | ||
| const darkButtons = darkSection?.querySelectorAll('button[data-theme-id]') || []; | ||
| const darkThemeIds = Array.from(darkButtons).map((b) => b.getAttribute('data-theme-id')); | ||
| expect(darkThemeIds).not.toContain('custom'); |
There was a problem hiding this comment.
exclude custom theme assertion is scoped to the wrong DOM node.
screen.getByText('dark Mode').closest('div') targets the mode label wrapper, so button[data-theme-id] lookup can be empty even when grouping is wrong.
Suggested test hardening
-const darkSection = screen.getByText('dark Mode').closest('div');
-const darkButtons = darkSection?.querySelectorAll('button[data-theme-id]') || [];
-const darkThemeIds = Array.from(darkButtons).map((b) => b.getAttribute('data-theme-id'));
-expect(darkThemeIds).not.toContain('custom');
+const customButtonsInRegularGrids = document.querySelectorAll('.grid [data-theme-id="custom"]');
+expect(customButtonsInRegularGrids).toHaveLength(0);📝 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 darkSection = screen.getByText('dark Mode').closest('div'); | |
| const darkButtons = darkSection?.querySelectorAll('button[data-theme-id]') || []; | |
| const darkThemeIds = Array.from(darkButtons).map((b) => b.getAttribute('data-theme-id')); | |
| expect(darkThemeIds).not.toContain('custom'); | |
| const customButtonsInRegularGrids = document.querySelectorAll('.grid [data-theme-id="custom"]'); | |
| expect(customButtonsInRegularGrids).toHaveLength(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/Settings/tabs/ThemeTab.test.tsx` around
lines 240 - 243, The assertion is scoped to the wrong DOM node because
darkSection is set via screen.getByText('dark Mode').closest('div'), which
targets the label wrapper and can miss the theme buttons; update the query so
darkButtons come from the actual theme-group container (e.g., locate the
section/fieldset that wraps the dark-mode themes using a more specific selector
or accessible query such as getByRole/getByTestId/getByLabelText, or use
within() on that container) then recompute darkThemeIds and assert it does not
contain 'custom'; adjust references to darkSection, darkButtons, and
darkThemeIds accordingly.
Comment on lines
+322
to
+339
| it('should pass import callbacks to CustomThemeBuilder', async () => { | ||
| const onError = vi.fn(); | ||
| const onSuccess = vi.fn(); | ||
|
|
||
| render( | ||
| <ThemeTab | ||
| theme={mockTheme} | ||
| themes={mockThemes} | ||
| onThemeImportError={onError} | ||
| onThemeImportSuccess={onSuccess} | ||
| /> | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await vi.advanceTimersByTimeAsync(100); | ||
| }); | ||
|
|
||
| expect(screen.getByTestId('custom-theme-builder')).toBeInTheDocument(); |
There was a problem hiding this comment.
import callbacks test does not verify callback wiring.
The current assertion only checks render presence, so it won’t fail if onThemeImportError/onThemeImportSuccess stop being forwarded.
Suggested assertion upgrade
+const mockCustomThemeBuilderProps = vi.fn();
vi.mock('../../../../../renderer/components/CustomThemeBuilder', () => ({
- CustomThemeBuilder: ({ isSelected, onSelect }: { isSelected: boolean; onSelect: () => void }) => (
- <div data-testid="custom-theme-builder">
- <button onClick={onSelect} data-theme-id="custom" className={isSelected ? 'ring-2' : ''}>
- Custom Theme
- </button>
- </div>
- ),
+ CustomThemeBuilder: (props: {
+ isSelected: boolean;
+ onSelect: () => void;
+ onImportError?: (message: string) => void;
+ onImportSuccess?: (themeName: string) => void;
+ }) => {
+ mockCustomThemeBuilderProps(props);
+ const { isSelected, onSelect } = props;
+ return (
+ <div data-testid="custom-theme-builder">
+ <button onClick={onSelect} data-theme-id="custom" className={isSelected ? 'ring-2' : ''}>
+ Custom Theme
+ </button>
+ </div>
+ );
+ },
}));
// ...
expect(screen.getByTestId('custom-theme-builder')).toBeInTheDocument();
+expect(mockCustomThemeBuilderProps).toHaveBeenCalledWith(
+ expect.objectContaining({
+ onImportError: onError,
+ onImportSuccess: onSuccess,
+ })
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/Settings/tabs/ThemeTab.test.tsx` around
lines 322 - 339, The test currently only asserts CustomThemeBuilder is rendered
but doesn't verify the import callbacks are forwarded; update the test to find
the CustomThemeBuilder (screen.getByTestId('custom-theme-builder')) and call its
forwarded props for import error and success to assert onThemeImportError and
onThemeImportSuccess (the vi.fn() mocks) are invoked; specifically, after
rendering ThemeTab, access the component instance or element, read its
props-like handlers (e.g., getByTestId(...).props or use ReactTestUtils/act to
invoke the child callbacks) and add expect(onError).toHaveBeenCalledWith(...)
and expect(onSuccess).toHaveBeenCalledWith(...) assertions to confirm wiring in
ThemeTab -> CustomThemeBuilder.
Comment on lines
1671
to
1675
| // Call the escape handler (simulating layer stack escape) | ||
| // ShortcutsTab handles its own escape via onKeyDownCapture, so | ||
| // the shell just ignores the escape when recording is active | ||
| capturedEscapeHandler?.(); | ||
|
|
There was a problem hiding this comment.
Avoid a false-positive in this Escape regression test.
capturedEscapeHandler?.() on Line 1674 makes the test pass even if no escape handler was registered. Assert the handler exists before invoking it.
🔧 Proposed fix
// Call the escape handler (simulating layer stack escape)
// ShortcutsTab handles its own escape via onKeyDownCapture, so
// the shell just ignores the escape when recording is active
- capturedEscapeHandler?.();
+ expect(capturedEscapeHandler).toBeTypeOf('function');
+ capturedEscapeHandler!();📝 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
| // Call the escape handler (simulating layer stack escape) | |
| // ShortcutsTab handles its own escape via onKeyDownCapture, so | |
| // the shell just ignores the escape when recording is active | |
| capturedEscapeHandler?.(); | |
| // Call the escape handler (simulating layer stack escape) | |
| // ShortcutsTab handles its own escape via onKeyDownCapture, so | |
| // the shell just ignores the escape when recording is active | |
| expect(capturedEscapeHandler).toBeTypeOf('function'); | |
| capturedEscapeHandler!(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/SettingsModal.test.tsx` around lines 1671 -
1675, The test currently calls capturedEscapeHandler?.() which can mask missing
registration; update the test in SettingsModal.test.tsx to assert
capturedEscapeHandler is defined before invoking it (e.g.,
expect(capturedEscapeHandler).toBeDefined() or toBeTruthy()), then call
capturedEscapeHandler(); ensure you reference the capturedEscapeHandler variable
used in the escape simulation and keep the existing comments about ShortcutsTab
handling escape intact.
Comment on lines
+536
to
+545
| <div | ||
| className="group relative flex-shrink-0 mt-0.5 outline-none" | ||
| tabIndex={0} | ||
| title="Environment variables configured here are available to all terminal sessions, all AI agent processes (Claude, OpenCode, etc.), and any spawned child processes. Agent-specific settings can override these values." | ||
| > | ||
| <HelpCircle | ||
| className="w-4 h-4 cursor-help" | ||
| style={{ color: theme.colors.textDim }} | ||
| /> | ||
| <div className="absolute hidden group-hover:block group-focus-visible:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal"> |
There was a problem hiding this comment.
Focusable help affordance is missing explicit focus handlers.
The help icon wrapper is keyboard-focusable (tabIndex={0}) but has no onFocus/onBlur handling for keyboard interaction state.
As per coding guidelines "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx` around lines 536 - 545,
The focusable help-icon wrapper (the div with className "group relative
flex-shrink-0 mt-0.5 outline-none" that contains <HelpCircle /> and uses
tabIndex={0}) is missing explicit keyboard focus handlers; add onFocus and
onBlur handlers to that wrapper (or corresponding focus-visible handlers) to
toggle the same visibility state used by hover (so the tooltip div currently
shown via "group-hover:block group-focus-visible:block" is reliably shown/hidden
for keyboard users), and ensure the handlers update any local state or call the
same CSS-visible class logic so keyboard focus triggers the tooltip just like
mouse hover.
Comment on lines
+786
to
+799
| <button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setPreventSleepEnabled(!preventSleepEnabled); | ||
| }} | ||
| className="relative w-10 h-5 rounded-full transition-colors flex-shrink-0" | ||
| style={{ | ||
| backgroundColor: preventSleepEnabled | ||
| ? theme.colors.accent | ||
| : theme.colors.bgActivity, | ||
| }} | ||
| role="switch" | ||
| aria-checked={preventSleepEnabled} | ||
| > |
There was a problem hiding this comment.
Standalone switch buttons need accessible names.
These role="switch" buttons are icon/shape-only and currently have no accessible label, so screen readers will announce unnamed controls.
Suggested fix (add explicit labels)
-<button ... role="switch" aria-checked={preventSleepEnabled}>
+<button ... role="switch" aria-checked={preventSleepEnabled} aria-label="Prevent sleep while working">
-<button ... role="switch" aria-checked={disableGpuAcceleration}>
+<button ... role="switch" aria-checked={disableGpuAcceleration} aria-label="Disable GPU acceleration">
-<button ... role="switch" aria-checked={disableConfetti}>
+<button ... role="switch" aria-checked={disableConfetti} aria-label="Disable confetti animations">
-<button ... role="switch" aria-checked={statsCollectionEnabled}>
+<button ... role="switch" aria-checked={statsCollectionEnabled} aria-label="Enable stats collection">
-<button ... role="switch" aria-checked={wakatimeEnabled}>
+<button ... role="switch" aria-checked={wakatimeEnabled} aria-label="Enable WakaTime tracking">
-<button ... role="switch" aria-checked={wakatimeDetailedTracking}>
+<button ... role="switch" aria-checked={wakatimeDetailedTracking} aria-label="Enable detailed file tracking">Also applies to: 854-867, 902-913, 986-998, 1164-1172, 1199-1209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx` around lines 786 - 799,
The switch buttons with role="switch" are missing accessible names; update each
switch element (e.g., the one toggling preventSleepEnabled via
setPreventSleepEnabled in GeneralTab.tsx) to include an explicit accessible
label using aria-label or aria-labelledby that describes the control (e.g.,
"Prevent computer from sleeping") and preserve aria-checked binding; apply the
same fix to the other switch buttons referenced (the switches around lines
854-867, 902-913, 986-998, 1164-1172, 1199-1209) so every role="switch" has a
meaningful aria-label or aria-labelledby matching the state it toggles.
Comment on lines
+1344
to
+1368
| onClick={async () => { | ||
| const folder = await window.maestro.sync.selectSyncFolder(); | ||
| if (folder) { | ||
| setSyncMigrating(true); | ||
| setSyncError(null); | ||
| setSyncMigratedCount(null); | ||
| try { | ||
| const result = await window.maestro.sync.setCustomPath(folder); | ||
| if (result.success) { | ||
| setCustomSyncPath(folder); | ||
| setCurrentStoragePath(folder); | ||
| setSyncRestartRequired(true); | ||
| if (result.migrated !== undefined) { | ||
| setSyncMigratedCount(result.migrated); | ||
| } | ||
| } else { | ||
| setSyncError(result.error || 'Failed to change storage location'); | ||
| } | ||
| if (result.errors && result.errors.length > 0) { | ||
| setSyncError(result.errors.join(', ')); | ||
| } | ||
| } finally { | ||
| setSyncMigrating(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle rejected sync API promises explicitly.
Both storage migration click handlers use try/finally without catch. Rejections from selectSyncFolder/setCustomPath can bubble as unhandled errors and skip user-facing error state updates.
Suggested fix (add explicit error handling)
try {
const result = await window.maestro.sync.setCustomPath(folder);
// existing success/error result handling
-} finally {
+} catch (err) {
+ setSyncError(err instanceof Error ? err.message : 'Failed to change storage location');
+} finally {
setSyncMigrating(false);
}
@@
try {
const result = await window.maestro.sync.setCustomPath(null);
// existing success/error result handling
-} finally {
+} catch (err) {
+ setSyncError(err instanceof Error ? err.message : 'Failed to reset storage location');
+} finally {
setSyncMigrating(false);
}Also applies to: 1387-1405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx` around lines 1344 -
1368, The click handler currently awaits window.maestro.sync.selectSyncFolder
and window.maestro.sync.setCustomPath inside a try/finally but has no catch, so
promise rejections can become unhandled and skip setting user-visible error
state; wrap the awaits in a try/catch/finally (or add an inner catch) for both
onClick handlers that call selectSyncFolder and setCustomPath, and in the catch
setSyncError with the caught error message (or a fallback string) and ensure
setSyncMigrating(false) still runs in finally; update references in the handler
to the symbols setSyncMigrating, setSyncError, setSyncMigratedCount,
setSyncRestartRequired, setCustomSyncPath, and setCurrentStoragePath so error
cases properly update UI state.
Comment on lines
+27
to
+31
| // Notify parent of recording state changes (for escape handler coordination) | ||
| useEffect(() => { | ||
| onRecordingChange?.(!!recordingId); | ||
| }, [recordingId, onRecordingChange]); | ||
|
|
There was a problem hiding this comment.
Reset onRecordingChange on unmount to prevent stale modal state.
The effect reports changes while mounted, but there’s no cleanup on unmount. If this tab unmounts during recording, parent escape coordination can stay stuck in recording mode.
🔧 Proposed fix
// Notify parent of recording state changes (for escape handler coordination)
useEffect(() => {
onRecordingChange?.(!!recordingId);
}, [recordingId, onRecordingChange]);
+
+ // Ensure parent state is reset if tab unmounts mid-recording
+ useEffect(() => {
+ return () => {
+ onRecordingChange?.(false);
+ };
+ }, [onRecordingChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/ShortcutsTab.tsx` around lines 27 - 31,
The current useEffect in ShortcutsTab calls onRecordingChange when recordingId
changes but has no cleanup on unmount; modify the useEffect (the one referencing
useEffect, onRecordingChange, and recordingId) to return a cleanup function that
calls onRecordingChange?.(false) so the parent is notified if the tab unmounts
during a recording, ensuring no stale "recording" state remains; keep
onRecordingChange and recordingId in the dependency array.
Comment on lines
+55
to
+89
| const handleThemePickerKeyDown = (e: React.KeyboardEvent) => { | ||
| if (e.key === 'Tab') { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| // Create ordered array: dark themes first, then light, then vibe, then custom (cycling back to dark) | ||
| const allThemes = [ | ||
| ...(groupedThemes['dark'] || []), | ||
| ...(groupedThemes['light'] || []), | ||
| ...(groupedThemes['vibe'] || []), | ||
| ]; | ||
| // Add 'custom' as the last item in the cycle | ||
| const allThemeIds = [...allThemes.map((t) => t.id), 'custom']; | ||
| let currentIndex = allThemeIds.findIndex((id: string) => id === activeThemeId); | ||
| if (currentIndex === -1) currentIndex = 0; | ||
|
|
||
| let newThemeId: string; | ||
| if (e.shiftKey) { | ||
| // Shift+Tab: go backwards | ||
| const prevIndex = currentIndex === 0 ? allThemeIds.length - 1 : currentIndex - 1; | ||
| newThemeId = allThemeIds[prevIndex]; | ||
| } else { | ||
| // Tab: go forward | ||
| const nextIndex = (currentIndex + 1) % allThemeIds.length; | ||
| newThemeId = allThemeIds[nextIndex]; | ||
| } | ||
| setActiveThemeId(newThemeId as ThemeId); | ||
|
|
||
| // Scroll the newly selected theme button into view | ||
| setTimeout(() => { | ||
| const themeButton = themePickerRef.current?.querySelector( | ||
| `[data-theme-id="${newThemeId}"]` | ||
| ); | ||
| themeButton?.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
Scope Tab interception so nested controls keep normal focus behavior.
On Line 55, Tab is prevented at the root container level, so Tab presses originating from descendants (including CustomThemeBuilder inputs/buttons) are also blocked. This can break keyboard navigation inside the custom theme editor.
🔧 Proposed fix
const handleThemePickerKeyDown = (e: React.KeyboardEvent) => {
- if (e.key === 'Tab') {
- e.preventDefault();
- e.stopPropagation();
+ if (e.key === 'Tab') {
+ // Only hijack Tab when the picker container itself has focus.
+ // Let nested controls (e.g. custom theme inputs) use normal Tab behavior.
+ if (e.target !== themePickerRef.current) return;
+ e.preventDefault();
+ e.stopPropagation();
// Create ordered array: dark themes first, then light, then vibe, then custom (cycling back to dark)
const allThemes = [
...(groupedThemes['dark'] || []),
...(groupedThemes['light'] || []),
...(groupedThemes['vibe'] || []),
];Also applies to: 93-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/ThemeTab.tsx` around lines 55 - 89, The
Tab handling in handleThemePickerKeyDown currently prevents Tab at the root
which blocks focus inside nested controls (e.g., CustomThemeBuilder); change the
logic to only intercept and call e.preventDefault()/e.stopPropagation() when the
Tab originated on a theme item inside the picker (use
themePickerRef.current.contains(e.target as Node) AND check the target or its
closest ancestor has a data-theme-id or matches your theme button selector),
otherwise let the event propagate so nested inputs/buttons keep normal focus
behavior; apply the same targeted-check approach to the other Tab handler block
referenced (lines 93–99) that also prevents Tab.
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
1 participant
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.
Extract 6 components from the monolithic SettingsModal.tsx (3,649 → 601 lines, −84%):
Each tab self-sources settings via useSettings() and owns its local state. SettingsModal.tsx retains modal chrome, tab bar, layer stack, and tab routing.
308 new tests across 6 test files; 416 total Settings tests passing.
Summary by CodeRabbit