refactor: decompose SettingsModal and SessionList components#502
Merged
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.
📝 WalkthroughWalkthroughAdds a Settings modal refactor: five new tab components, a new EnvVarsEditor (with exports), new SessionList UI components, many test suites, and several non-functional formatting edits across renderer hooks and docs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SettingsUI
participant EncoreTab
participant AgentAPI
participant SettingsStore
User->>SettingsUI: Open Settings, select Encore tab
SettingsUI->>EncoreTab: mount(isOpen=true)
EncoreTab->>AgentAPI: detectAgents()
AgentAPI-->>EncoreTab: agent list
User->>EncoreTab: Click "Customize" for provider X
EncoreTab->>AgentAPI: getConfig(provider X)
AgentAPI-->>EncoreTab: provider config
EncoreTab->>AgentAPI: getModels(provider X) (if supported)
AgentAPI-->>EncoreTab: model list
User->>EncoreTab: change customPath/args/envVars
EncoreTab->>SettingsStore: setDirectorNotesSettings(updated config)
SettingsStore-->>EncoreTab: persisted
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
+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
+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.
Resolves conflict in SettingsModal.tsx: kept decomposed <GeneralTab> component (from our refactoring) and ported the "Conductor Profile" heading rename from PR #500 into GeneralTab.tsx. Auto-merged changes from main (PRs #499, #500, #503): - Historical agent error modal support (effectiveAgentError) - OpenCode parser improvements (filePath tool detail) - TerminalOutput error details callback signature update - Director's Notes lookback default initialization
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
src/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx (1)
818-830:⚠️ Potential issue | 🟡 MinorPlatform-specific assertion is brittle here.
At Line 829, asserting
'Ctrl + Enter'will fail on macOS where this can be'⌘ + Enter'.Suggested fix
-expect(toggleButton?.textContent).toBe('Ctrl + Enter'); +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 "should show Ctrl + Enter label when enterToSendAI is false" is brittle on macOS because it expects the literal string 'Ctrl + Enter'; update the assertion in the test (GeneralTab.test.tsx, the it block with mockUseSettingsOverrides = { enterToSendAI: false } that queries aiModeLabel and toggleButton) to accept both platform variants by either asserting a regex that matches either 'Ctrl' or the Mac '⌘' (e.g. /(Ctrl|⌘)\s*\+\s*Enter/) or by splitting the button text and checking it contains 'Enter' and (contains 'Ctrl' or contains '⌘'), so the test passes on all OSes.src/renderer/components/Settings/tabs/GeneralTab.tsx (4)
1344-1368:⚠️ Potential issue | 🟠 MajorSync migration handlers need explicit rejection handling.
Both async handlers use
try/finallywithoutcatch; rejected promises fromselectSyncFolder/setCustomPathcan bubble and skip user-visible error state updates.Suggested fix
try { const result = await window.maestro.sync.setCustomPath(folder); // existing success/error handling +} 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 handling +} 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 onClick async handler for selecting and setting a custom sync folder uses try/finally but no catch, so rejections from window.maestro.sync.selectSyncFolder or window.maestro.sync.setCustomPath can escape and leave UI state inconsistent; update the handler(s) (the onClick that calls window.maestro.sync.selectSyncFolder and window.maestro.sync.setCustomPath, and the similar handler at the other occurrence) to include a catch block that captures any thrown error, calls setSyncError with a user-friendly message (e.g., error.message or String(error)), and ensures setSyncMigrating(false) still runs in finally; keep existing success/error-result handling inside try and do not remove the finally cleanup.
536-545:⚠️ Potential issue | 🟡 MinorFocusable tooltip trigger is missing explicit focus handlers.
The wrapper is keyboard-focusable (
tabIndex={0}) but has noonFocus/onBlurhandlers at Line 536-545.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 keyboard-focusable tooltip wrapper div (the element with className "group relative flex-shrink-0 mt-0.5 outline-none" in GeneralTab.tsx) needs explicit focus handlers: add onFocus and onBlur that toggle the tooltip visibility the same way hover does (e.g., set a local piece of state like isTooltipOpen or call the existing hover logic) so keyboard users see/hide the tooltip when the wrapper gains/loses focus; ensure the title/help icon (HelpCircle) remains accessible and that the handlers mirror the group-hover/group-focus-visible behavior.
786-799:⚠️ Potential issue | 🟠 MajorSwitch controls are missing accessible names.
Each
role="switch"button here hasaria-checkedbut noaria-label/aria-labelledby, so screen readers can encounter unnamed controls.Suggested fix (example pattern)
-<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 in GeneralTab.tsx (the button using preventSleepEnabled and setPreventSleepEnabled with role="switch") lack accessible names; update this button and every other button in GeneralTab.tsx that has role="switch" to include an aria-label or aria-labelledby that describes the control and its state (for example include the label text plus on/off state), e.g. add aria-label derived from the same label shown in the UI or a localized string and reflect the current state (preventSleepEnabled) so screen readers get a meaningful name and state.
256-274:⚠️ Potential issue | 🟠 MajorShell detection can get stuck after an empty detect result.
At Line 257 and Line 271, a successful detect that returns
[]still setsshellsLoaded=true, which makes “Detect other available shells...” non-functional on later clicks.Suggested fix
-const loadShells = async () => { - if (shellsLoaded) return; +const loadShells = async (force = false) => { + if (shellsLoading || (shellsLoaded && !force)) return; setShellsLoading(true); try { const detected = await window.maestro.shells.detect(); setShells(detected); - setShellsLoaded(true); + setShellsLoaded(detected.length > 0); } catch (error) { console.error('Failed to load shells:', error); } finally { setShellsLoading(false); } }; @@ -<button - onClick={handleShellInteraction} +<button + onClick={() => loadShells(true)}🤖 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 256 - 274, The bug is that loadShells sets shellsLoaded to true even when detected is an empty array, preventing subsequent detections; change loadShells so after const detected = await window.maestro.shells.detect() you set setShells(detected) but only call setShellsLoaded(true) when detected && detected.length > 0 (leave shellsLoaded false when no shells found so handleShellInteraction can trigger detection again); ensure you still setShells([]) on empty results and keep the existing error/finally behavior.
🧹 Nitpick comments (1)
src/renderer/components/SessionList/SessionTooltipContent.tsx (1)
160-163: Consider renaming “Session Cost” to agent-facing copy for consistency.This is user-facing UI; “Agent Cost” would align better with the terminology used elsewhere in this PR.
As per coding guidelines:
src/**/*.{ts,tsx}: Use the Session interface in code to represent agents (due to historical naming), but use 'agent' in user-facing language and 'session' for provider-level conversation contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList/SessionTooltipContent.tsx` around lines 160 - 163, The label "Session Cost" in SessionTooltipContent is agent-facing copy and should be renamed to "Agent Cost" for consistency with the project's user-facing terminology; update the displayed string in the JSX where SessionTooltipContent renders the label (the span showing "Session Cost" near session.usageStats.totalCostUsd) to "Agent Cost" while leaving the underlying Session interface/variable names (e.g., session) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/releases.md`:
- Line 86: Update the compound adjectives in the release notes by hyphenating
them: replace the phrase "cross context memory" with "cross-context memory" (as
used in the history panel sentence) and replace "built in updater" with
"built-in updater" wherever it appears (e.g., the line referenced as the
built-in updater mention); ensure the hyphenated forms are used consistently in
docs/releases.md.
- Line 147: The document uses both "pre-release" and "prerelease"; pick one
spelling and make it consistent by replacing all occurrences of the other form
in this file (e.g., change every "pre-release" to "prerelease" or vice versa),
including the instances shown in the diff snippet ("Added pre-release channel
support for beta/RC updates 🧪") and the other occurrence ("prerelease"); update
any headings, bullet points, and release notes in docs/releases.md so the chosen
form is used everywhere.
- Line 197: Replace the incorrect trademark spelling "Github" with the official
"GitHub" in the release note text — specifically update the phrase "Github
Worktree support was added" (and any other occurrences of "Github" in this
paragraph) to "GitHub Worktree support was added" so the documentation uses the
correct capital "H" trademark form.
- Line 61: Update the release note line that currently reads "AI responses can
now be saved to markdown on disk 💾" to use the proper noun capitalization:
change "markdown" to "Markdown" so the line reads "AI responses can now be saved
to Markdown on disk 💾".
- Line 20: In the release notes paragraph starting with "🎶 Maestro Symphony -
Contribute to open source with AI assistance!" replace the unhyphenated phrase
"open source projects and features" with the compound-adjective form
"open-source projects and features" (locate the exact occurrence of "open
source" in that sentence and add the hyphen) to correct the compound adjective
hyphenation.
In `@src/renderer/components/SessionList/SessionContextMenu.tsx`:
- Around line 61-111: The submenu hide timeout can persist past unmount and
multiple leave calls; update handleMoveToGroupLeave to clear any existing
timeout before setting a new one (use submenuTimeoutRef.current and
clearTimeout, then set it), and add a cleanup effect that runs on unmount to
clearTimeout(submenuTimeoutRef.current) and set submenuTimeoutRef.current =
null; ensure you also set submenuTimeoutRef.current = null after clearing in
handleMoveToGroupHover/handleMoveToGroupLeave. This touches submenuTimeoutRef,
handleMoveToGroupHover, handleMoveToGroupLeave and the component's effects for
cleanup.
- Around line 176-193: The "Move to Group" submenu is only opened on mouse
hover; update the submenu trigger in SessionContextMenu (the element using
moveToGroupRef and the Move to Group button) to be keyboard-accessible by adding
tabIndex={0}, onFocus and onBlur handlers that call the existing
handleMoveToGroupHover and handleMoveToGroupLeave (or new focus equivalents),
and add an onKeyDown handler that opens the submenu on Enter/Space and closes it
on Escape; ensure the focus/keyboard handlers mirror the hover behavior so
keyboard users can open, navigate, and dismiss the submenu.
- Around line 266-330: The menu currently renders a divider even when no
actionable buttons will appear; update the conditional guards so the divider and
its buttons only render when at least one visible action exists: for the top
section change the condition to require ((hasWorktreeChildren ||
session.isGitRepo) && !session.parentSessionId && ((onQuickCreateWorktree &&
session.worktreeConfig) || onConfigureWorktrees)), and for the worktree-child
section require (session.parentSessionId && session.worktreeBranch &&
(onCreatePR || onDeleteWorktree)); modify the JSX around those blocks
(references: session, hasWorktreeChildren, session.isGitRepo,
session.parentSessionId, session.worktreeConfig, onQuickCreateWorktree,
onConfigureWorktrees, onCreatePR, onDeleteWorktree, worktreeBranch) so the
divider is only shown when a button will follow.
In `@src/renderer/components/SessionList/SessionTooltipContent.tsx`:
- Around line 129-145: Clamp session.contextUsage into the 0–100 range before
using it for the label and progress-bar width: compute a clamped value (e.g.,
via Math.max(0, Math.min(100, session.contextUsage)) or a small clamp helper)
inside SessionTooltipContent and use that clamped variable for the displayed
text and the width string (width: `${clamped}%`), and pass the clamped value to
getContextColor/contextWarning thresholds so the bar renders correctly even if
session.contextUsage is out of range.
---
Duplicate comments:
In `@src/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx`:
- Around line 818-830: The test "should show Ctrl + Enter label when
enterToSendAI is false" is brittle on macOS because it expects the literal
string 'Ctrl + Enter'; update the assertion in the test (GeneralTab.test.tsx,
the it block with mockUseSettingsOverrides = { enterToSendAI: false } that
queries aiModeLabel and toggleButton) to accept both platform variants by either
asserting a regex that matches either 'Ctrl' or the Mac '⌘' (e.g.
/(Ctrl|⌘)\s*\+\s*Enter/) or by splitting the button text and checking it
contains 'Enter' and (contains 'Ctrl' or contains '⌘'), so the test passes on
all OSes.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 1344-1368: The onClick async handler for selecting and setting a
custom sync folder uses try/finally but no catch, so rejections from
window.maestro.sync.selectSyncFolder or window.maestro.sync.setCustomPath can
escape and leave UI state inconsistent; update the handler(s) (the onClick that
calls window.maestro.sync.selectSyncFolder and
window.maestro.sync.setCustomPath, and the similar handler at the other
occurrence) to include a catch block that captures any thrown error, calls
setSyncError with a user-friendly message (e.g., error.message or
String(error)), and ensures setSyncMigrating(false) still runs in finally; keep
existing success/error-result handling inside try and do not remove the finally
cleanup.
- Around line 536-545: The keyboard-focusable tooltip wrapper div (the element
with className "group relative flex-shrink-0 mt-0.5 outline-none" in
GeneralTab.tsx) needs explicit focus handlers: add onFocus and onBlur that
toggle the tooltip visibility the same way hover does (e.g., set a local piece
of state like isTooltipOpen or call the existing hover logic) so keyboard users
see/hide the tooltip when the wrapper gains/loses focus; ensure the title/help
icon (HelpCircle) remains accessible and that the handlers mirror the
group-hover/group-focus-visible behavior.
- Around line 786-799: The switch buttons in GeneralTab.tsx (the button using
preventSleepEnabled and setPreventSleepEnabled with role="switch") lack
accessible names; update this button and every other button in GeneralTab.tsx
that has role="switch" to include an aria-label or aria-labelledby that
describes the control and its state (for example include the label text plus
on/off state), e.g. add aria-label derived from the same label shown in the UI
or a localized string and reflect the current state (preventSleepEnabled) so
screen readers get a meaningful name and state.
- Around line 256-274: The bug is that loadShells sets shellsLoaded to true even
when detected is an empty array, preventing subsequent detections; change
loadShells so after const detected = await window.maestro.shells.detect() you
set setShells(detected) but only call setShellsLoaded(true) when detected &&
detected.length > 0 (leave shellsLoaded false when no shells found so
handleShellInteraction can trigger detection again); ensure you still
setShells([]) on empty results and keep the existing error/finally behavior.
---
Nitpick comments:
In `@src/renderer/components/SessionList/SessionTooltipContent.tsx`:
- Around line 160-163: The label "Session Cost" in SessionTooltipContent is
agent-facing copy and should be renamed to "Agent Cost" for consistency with the
project's user-facing terminology; update the displayed string in the JSX where
SessionTooltipContent renders the label (the span showing "Session Cost" near
session.usageStats.totalCostUsd) to "Agent Cost" while leaving the underlying
Session interface/variable names (e.g., session) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/releases.mdsrc/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsxsrc/__tests__/renderer/hooks/useAgentSessionManagement.test.tssrc/renderer/components/SessionList/HamburgerMenuContent.tsxsrc/renderer/components/SessionList/SessionContextMenu.tsxsrc/renderer/components/SessionList/SessionTooltipContent.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/hooks/agent/useAgentSessionManagement.tssrc/renderer/hooks/modal/useModalHandlers.ts
✅ Files skipped from review due to trivial changes (2)
- src/renderer/hooks/modal/useModalHandlers.ts
- src/tests/renderer/hooks/useAgentSessionManagement.test.ts
Comment on lines
+61
to
+111
| const submenuTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| const [showMoveSubmenu, setShowMoveSubmenu] = useState(false); | ||
| const [submenuPosition, setSubmenuPosition] = useState<{ | ||
| vertical: 'below' | 'above'; | ||
| horizontal: 'right' | 'left'; | ||
| }>({ vertical: 'below', horizontal: 'right' }); | ||
|
|
||
| const onDismissRef = useRef(onDismiss); | ||
| onDismissRef.current = onDismiss; | ||
|
|
||
| useClickOutside(menuRef, onDismiss); | ||
|
|
||
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { | ||
| onDismissRef.current(); | ||
| } | ||
| }; | ||
| document.addEventListener('keydown', handleKeyDown); | ||
| return () => document.removeEventListener('keydown', handleKeyDown); | ||
| }, []); | ||
|
|
||
| const { left, top, ready } = useContextMenuPosition(menuRef, x, y); | ||
|
|
||
| const handleMoveToGroupHover = () => { | ||
| if (submenuTimeoutRef.current) { | ||
| clearTimeout(submenuTimeoutRef.current); | ||
| submenuTimeoutRef.current = null; | ||
| } | ||
| setShowMoveSubmenu(true); | ||
|
|
||
| if (moveToGroupRef.current) { | ||
| const rect = moveToGroupRef.current.getBoundingClientRect(); | ||
| const itemHeight = 28; | ||
| const submenuHeight = (groups.length + 1) * itemHeight + 16 + (groups.length > 0 ? 8 : 0); | ||
| const submenuWidth = 160; | ||
| const spaceBelow = window.innerHeight - rect.top; | ||
| const spaceRight = window.innerWidth - rect.right; | ||
|
|
||
| const vertical = spaceBelow < submenuHeight && rect.top > submenuHeight ? 'above' : 'below'; | ||
| const horizontal = spaceRight < submenuWidth && rect.left > submenuWidth ? 'left' : 'right'; | ||
|
|
||
| setSubmenuPosition({ vertical, horizontal }); | ||
| } | ||
| }; | ||
|
|
||
| const handleMoveToGroupLeave = () => { | ||
| submenuTimeoutRef.current = setTimeout(() => { | ||
| setShowMoveSubmenu(false); | ||
| }, 300); | ||
| }; |
There was a problem hiding this comment.
Clear pending submenu timeout on unmount.
A pending hide timeout can outlive the menu lifecycle. Add cleanup and clear any existing timeout before creating a new one.
💡 Proposed fix
const handleMoveToGroupLeave = () => {
+ if (submenuTimeoutRef.current) {
+ clearTimeout(submenuTimeoutRef.current);
+ }
submenuTimeoutRef.current = setTimeout(() => {
setShowMoveSubmenu(false);
}, 300);
};
+
+ useEffect(() => {
+ return () => {
+ if (submenuTimeoutRef.current) {
+ clearTimeout(submenuTimeoutRef.current);
+ }
+ };
+ }, []);📝 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 submenuTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const [showMoveSubmenu, setShowMoveSubmenu] = useState(false); | |
| const [submenuPosition, setSubmenuPosition] = useState<{ | |
| vertical: 'below' | 'above'; | |
| horizontal: 'right' | 'left'; | |
| }>({ vertical: 'below', horizontal: 'right' }); | |
| const onDismissRef = useRef(onDismiss); | |
| onDismissRef.current = onDismiss; | |
| useClickOutside(menuRef, onDismiss); | |
| useEffect(() => { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| if (e.key === 'Escape') { | |
| onDismissRef.current(); | |
| } | |
| }; | |
| document.addEventListener('keydown', handleKeyDown); | |
| return () => document.removeEventListener('keydown', handleKeyDown); | |
| }, []); | |
| const { left, top, ready } = useContextMenuPosition(menuRef, x, y); | |
| const handleMoveToGroupHover = () => { | |
| if (submenuTimeoutRef.current) { | |
| clearTimeout(submenuTimeoutRef.current); | |
| submenuTimeoutRef.current = null; | |
| } | |
| setShowMoveSubmenu(true); | |
| if (moveToGroupRef.current) { | |
| const rect = moveToGroupRef.current.getBoundingClientRect(); | |
| const itemHeight = 28; | |
| const submenuHeight = (groups.length + 1) * itemHeight + 16 + (groups.length > 0 ? 8 : 0); | |
| const submenuWidth = 160; | |
| const spaceBelow = window.innerHeight - rect.top; | |
| const spaceRight = window.innerWidth - rect.right; | |
| const vertical = spaceBelow < submenuHeight && rect.top > submenuHeight ? 'above' : 'below'; | |
| const horizontal = spaceRight < submenuWidth && rect.left > submenuWidth ? 'left' : 'right'; | |
| setSubmenuPosition({ vertical, horizontal }); | |
| } | |
| }; | |
| const handleMoveToGroupLeave = () => { | |
| submenuTimeoutRef.current = setTimeout(() => { | |
| setShowMoveSubmenu(false); | |
| }, 300); | |
| }; | |
| const submenuTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const [showMoveSubmenu, setShowMoveSubmenu] = useState(false); | |
| const [submenuPosition, setSubmenuPosition] = useState<{ | |
| vertical: 'below' | 'above'; | |
| horizontal: 'right' | 'left'; | |
| }>({ vertical: 'below', horizontal: 'right' }); | |
| const onDismissRef = useRef(onDismiss); | |
| onDismissRef.current = onDismiss; | |
| useClickOutside(menuRef, onDismiss); | |
| useEffect(() => { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| if (e.key === 'Escape') { | |
| onDismissRef.current(); | |
| } | |
| }; | |
| document.addEventListener('keydown', handleKeyDown); | |
| return () => document.removeEventListener('keydown', handleKeyDown); | |
| }, []); | |
| const { left, top, ready } = useContextMenuPosition(menuRef, x, y); | |
| const handleMoveToGroupHover = () => { | |
| if (submenuTimeoutRef.current) { | |
| clearTimeout(submenuTimeoutRef.current); | |
| submenuTimeoutRef.current = null; | |
| } | |
| setShowMoveSubmenu(true); | |
| if (moveToGroupRef.current) { | |
| const rect = moveToGroupRef.current.getBoundingClientRect(); | |
| const itemHeight = 28; | |
| const submenuHeight = (groups.length + 1) * itemHeight + 16 + (groups.length > 0 ? 8 : 0); | |
| const submenuWidth = 160; | |
| const spaceBelow = window.innerHeight - rect.top; | |
| const spaceRight = window.innerWidth - rect.right; | |
| const vertical = spaceBelow < submenuHeight && rect.top > submenuHeight ? 'above' : 'below'; | |
| const horizontal = spaceRight < submenuWidth && rect.left > submenuWidth ? 'left' : 'right'; | |
| setSubmenuPosition({ vertical, horizontal }); | |
| } | |
| }; | |
| const handleMoveToGroupLeave = () => { | |
| if (submenuTimeoutRef.current) { | |
| clearTimeout(submenuTimeoutRef.current); | |
| } | |
| submenuTimeoutRef.current = setTimeout(() => { | |
| setShowMoveSubmenu(false); | |
| }, 300); | |
| }; | |
| useEffect(() => { | |
| return () => { | |
| if (submenuTimeoutRef.current) { | |
| clearTimeout(submenuTimeoutRef.current); | |
| } | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SessionList/SessionContextMenu.tsx` around lines 61 -
111, The submenu hide timeout can persist past unmount and multiple leave calls;
update handleMoveToGroupLeave to clear any existing timeout before setting a new
one (use submenuTimeoutRef.current and clearTimeout, then set it), and add a
cleanup effect that runs on unmount to clearTimeout(submenuTimeoutRef.current)
and set submenuTimeoutRef.current = null; ensure you also set
submenuTimeoutRef.current = null after clearing in
handleMoveToGroupHover/handleMoveToGroupLeave. This touches submenuTimeoutRef,
handleMoveToGroupHover, handleMoveToGroupLeave and the component's effects for
cleanup.
docs/releases.md: - Hyphenate compound adjectives (cross-context, built-in, open-source) - Consistent "prerelease" spelling throughout - Fix "Github" → "GitHub" trademark capitalization - Capitalize "Markdown" as proper noun SessionContextMenu.tsx: - Add unmount cleanup for submenu hide timeout - Clear existing timeout in leave handler before setting new one - Add keyboard accessibility (tabIndex, focus/blur, Enter/Space/Escape) - Guard divider rendering so it only appears when buttons follow SessionTooltipContent.tsx: - Clamp contextUsage to 0–100 range for display and progress bar - Rename "Session Cost" to "Agent Cost" per project terminology GeneralTab.tsx: - Add catch block to sync folder handler for error resilience - Add aria-label to all 6 role="switch" toggle buttons - Only set shellsLoaded=true when shells are actually detected GeneralTab.test.tsx: - Make Ctrl+Enter assertion platform-agnostic via regex
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/renderer/components/Settings/tabs/GeneralTab.tsx (2)
538-547:⚠️ Potential issue | 🟡 MinorAdd explicit focus handlers for the keyboard-focusable help trigger.
This element is focusable (
tabIndex={0}) but has noonFocus/onBlurhandlers, which is required for keyboard-focus UI affordances here.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 538 - 547, The focusable help trigger (the outer div with tabIndex={0} that contains HelpCircle and the tooltip div using classes "group" and "absolute ... group-hover:block group-focus-visible:block") lacks onFocus/onBlur handlers needed for keyboard users; add explicit onFocus and onBlur callbacks (or controlled boolean state like isHelpFocused) on that div to set/unset a focused flag, toggle the tooltip's visibility based on that flag (in addition to existing hover/focus CSS), and add appropriate accessibility attributes such as aria-describedby linking the HelpCircle to the tooltip and role="tooltip" on the tooltip div; optionally handle onKeyDown to close the tooltip on Escape within the same component (GeneralTab / the help-trigger div).
1352-1415:⚠️ Potential issue | 🟠 MajorHandle rejected sync API calls in both storage-location actions.
selectSyncFolder()(Line 1353) and the reset-path flow (Line 1397-1415) can reject without acatch, causing unhandled promise rejections and skippingsetSyncError(...)updates.Suggested fix
- onClick={async () => { - const folder = await window.maestro.sync.selectSyncFolder(); - if (folder) { - setSyncMigrating(true); - setSyncError(null); - setSyncMigratedCount(null); - try { + onClick={async () => { + try { + const folder = await window.maestro.sync.selectSyncFolder(); + if (!folder) return; + setSyncMigrating(true); + setSyncError(null); + setSyncMigratedCount(null); + try { const result = await window.maestro.sync.setCustomPath(folder); ... - } catch (error) { - setSyncError(error instanceof Error ? error.message : String(error)); - } finally { + } catch (error) { + setSyncError(error instanceof Error ? error.message : 'Failed to change storage location'); + } finally { setSyncMigrating(false); } - } + } catch (error) { + setSyncError(error instanceof Error ? error.message : 'Failed to select folder'); + } }}try { const result = await window.maestro.sync.setCustomPath(null); ... +} catch (error) { + setSyncError(error instanceof Error ? error.message : 'Failed to reset storage location'); } finally { setSyncMigrating(false); }🤖 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 1352 - 1415, selectSyncFolder() and the reset-path call to window.maestro.sync.setCustomPath(null) can reject and currently lack catch handlers; wrap the initial selectSyncFolder invocation in a try/catch (so any rejection from selectSyncFolder is caught) and ensure both button handlers wrap their await window.maestro.sync.setCustomPath(...) calls in try/catch/finally blocks that setSyncMigrating(false) in finally and call setSyncError(...) in catch (use error instanceof Error ? error.message : String(error)). Keep existing success-path updates (setCustomSyncPath, setCurrentStoragePath, setSyncRestartRequired, setSyncMigratedCount) inside try; also clear/setSyncMigratedCount and setSyncError(null) as before before the try.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/releases.md`:
- Line 97: Update the sentence in docs/releases.md that currently reads "Launch
from file preview or from the FIle tree panel." to use correct capitalization by
changing "FIle" to "File" so it reads "Launch from file preview or from the File
tree panel."; locate the sentence in the "Document Graphs" section and make the
single-word capitalization correction.
- Line 103: Replace the incorrect release heading text "# Smaller Changes in
014.x" with the consistent format "# Smaller Changes in 0.14.x"; locate the
heading string "Smaller Changes in 014.x" in the document and update it to
"Smaller Changes in 0.14.x" so it matches the other version headings.
In `@src/renderer/components/SessionList/SessionContextMenu.tsx`:
- Around line 149-377: The menu's <button> elements in SessionContextMenu.tsx
(e.g., the buttons invoking onRename/onDismiss, onEdit/onDismiss,
onDuplicate/onDismiss, the Move to Group button and submenu buttons that call
onMoveToGroup, onCreateGroup, onQuickCreateWorktree, onConfigureWorktrees,
onCreatePR, onDeleteWorktree, onDelete, onToggleBookmark, etc.) need explicit
type="button" attributes to avoid accidental form submission when nested in a
form; update every <button> in this component (including those inside the
moveToGroupRef submenu and conditional sections controlled by showMoveSubmenu,
showWorktreeParentSection, and showWorktreeChildSection) to include
type="button".
---
Duplicate comments:
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 538-547: The focusable help trigger (the outer div with
tabIndex={0} that contains HelpCircle and the tooltip div using classes "group"
and "absolute ... group-hover:block group-focus-visible:block") lacks
onFocus/onBlur handlers needed for keyboard users; add explicit onFocus and
onBlur callbacks (or controlled boolean state like isHelpFocused) on that div to
set/unset a focused flag, toggle the tooltip's visibility based on that flag (in
addition to existing hover/focus CSS), and add appropriate accessibility
attributes such as aria-describedby linking the HelpCircle to the tooltip and
role="tooltip" on the tooltip div; optionally handle onKeyDown to close the
tooltip on Escape within the same component (GeneralTab / the help-trigger div).
- Around line 1352-1415: selectSyncFolder() and the reset-path call to
window.maestro.sync.setCustomPath(null) can reject and currently lack catch
handlers; wrap the initial selectSyncFolder invocation in a try/catch (so any
rejection from selectSyncFolder is caught) and ensure both button handlers wrap
their await window.maestro.sync.setCustomPath(...) calls in try/catch/finally
blocks that setSyncMigrating(false) in finally and call setSyncError(...) in
catch (use error instanceof Error ? error.message : String(error)). Keep
existing success-path updates (setCustomSyncPath, setCurrentStoragePath,
setSyncRestartRequired, setSyncMigratedCount) inside try; also
clear/setSyncMigratedCount and setSyncError(null) as before before the try.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/releases.mdsrc/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsxsrc/renderer/components/SessionList/SessionContextMenu.tsxsrc/renderer/components/SessionList/SessionTooltipContent.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/renderer/components/Settings/tabs/GeneralTab.test.tsx
|
|
||
| The major contributions to 0.14.x remain: | ||
|
|
||
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. |
There was a problem hiding this comment.
Fix typo in “FIle tree panel.”
“FIle” should be “File” in this sentence.
📝 Proposed fix
-🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs.
+🗄️ Document Graphs. Launch from file preview or from the File tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs.📝 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
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. | |
| 🗄️ Document Graphs. Launch from file preview or from the File tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 97, Update the sentence in docs/releases.md that
currently reads "Launch from file preview or from the FIle tree panel." to use
correct capitalization by changing "FIle" to "File" so it reads "Launch from
file preview or from the File tree panel."; locate the sentence in the "Document
Graphs" section and make the single-word capitalization correction.
|
|
||
| 🧙♂️ Added an in-tab wizard for generating Auto Run Playbooks via `/wizard` or a new button in the Auto Run panel. | ||
|
|
||
| # Smaller Changes in 014.x |
There was a problem hiding this comment.
Correct version heading format for consistency.
The heading says 014.x; this should be 0.14.x to match the rest of the document’s version format.
📝 Proposed fix
-# Smaller Changes in 014.x
+# Smaller Changes in 0.14.x📝 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
| # Smaller Changes in 014.x | |
| # Smaller Changes in 0.14.x |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 103, Replace the incorrect release heading text "#
Smaller Changes in 014.x" with the consistent format "# Smaller Changes in
0.14.x"; locate the heading string "Smaller Changes in 014.x" in the document
and update it to "Smaller Changes in 0.14.x" so it matches the other version
headings.
Comment on lines
+149
to
+377
| <button | ||
| onClick={() => { | ||
| onRename(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.textMain }} | ||
| > | ||
| <Edit3 className="w-3.5 h-3.5" /> | ||
| Rename | ||
| </button> | ||
|
|
||
| <button | ||
| onClick={() => { | ||
| onEdit(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.textMain }} | ||
| > | ||
| <Settings className="w-3.5 h-3.5" /> | ||
| Edit Agent... | ||
| </button> | ||
|
|
||
| <button | ||
| onClick={() => { | ||
| onDuplicate(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.textMain }} | ||
| > | ||
| <Copy className="w-3.5 h-3.5" /> | ||
| Duplicate... | ||
| </button> | ||
|
|
||
| {!session.parentSessionId && ( | ||
| <button | ||
| onClick={() => { | ||
| onToggleBookmark(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.textMain }} | ||
| > | ||
| <Bookmark className="w-3.5 h-3.5" fill={session.bookmarked ? 'currentColor' : 'none'} /> | ||
| {session.bookmarked ? 'Remove Bookmark' : 'Add Bookmark'} | ||
| </button> | ||
| )} | ||
|
|
||
| {!session.parentSessionId && ( | ||
| <div | ||
| ref={moveToGroupRef} | ||
| className="relative" | ||
| tabIndex={0} | ||
| onMouseEnter={handleMoveToGroupHover} | ||
| onMouseLeave={handleMoveToGroupLeave} | ||
| onFocus={handleMoveToGroupHover} | ||
| onBlur={handleMoveToGroupLeave} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| handleMoveToGroupHover(); | ||
| } else if (e.key === 'Escape' && showMoveSubmenu) { | ||
| e.stopPropagation(); | ||
| setShowMoveSubmenu(false); | ||
| } | ||
| }} | ||
| > | ||
| <button | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center justify-between" | ||
| style={{ color: theme.colors.textMain }} | ||
| > | ||
| <span className="flex items-center gap-2"> | ||
| <FolderInput className="w-3.5 h-3.5" /> | ||
| Move to Group | ||
| </span> | ||
| <ChevronRight className="w-3 h-3" /> | ||
| </button> | ||
|
|
||
| {showMoveSubmenu && ( | ||
| <div | ||
| className="absolute py-1 rounded-md shadow-xl border" | ||
| style={{ | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| minWidth: '140px', | ||
| ...(submenuPosition.vertical === 'above' ? { bottom: 0 } : { top: 0 }), | ||
| ...(submenuPosition.horizontal === 'left' | ||
| ? { right: '100%', marginRight: 4 } | ||
| : { left: '100%', marginLeft: 4 }), | ||
| }} | ||
| > | ||
| <button | ||
| onClick={() => { | ||
| onMoveToGroup(''); | ||
| onDismiss(); | ||
| }} | ||
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${!session.groupId ? 'opacity-50' : ''}`} | ||
| style={{ color: theme.colors.textMain }} | ||
| disabled={!session.groupId} | ||
| > | ||
| <Folder className="w-3.5 h-3.5" /> | ||
| Ungrouped | ||
| {!session.groupId && <span className="text-[10px] opacity-50">(current)</span>} | ||
| </button> | ||
|
|
||
| {groups.length > 0 && ( | ||
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | ||
| )} | ||
|
|
||
| {groups.map((group) => ( | ||
| <button | ||
| key={group.id} | ||
| onClick={() => { | ||
| onMoveToGroup(group.id); | ||
| onDismiss(); | ||
| }} | ||
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${session.groupId === group.id ? 'opacity-50' : ''}`} | ||
| style={{ color: theme.colors.textMain }} | ||
| disabled={session.groupId === group.id} | ||
| > | ||
| <span>{group.emoji}</span> | ||
| <span className="truncate">{group.name}</span> | ||
| {session.groupId === group.id && ( | ||
| <span className="text-[10px] opacity-50">(current)</span> | ||
| )} | ||
| </button> | ||
| ))} | ||
|
|
||
| {onCreateGroup && ( | ||
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | ||
| )} | ||
|
|
||
| {onCreateGroup && ( | ||
| <button | ||
| onClick={() => { | ||
| onCreateGroup(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <FolderPlus className="w-3.5 h-3.5" /> | ||
| Create New Group | ||
| </button> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {showWorktreeParentSection && ( | ||
| <> | ||
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | ||
| {onQuickCreateWorktree && session.worktreeConfig && ( | ||
| <button | ||
| onClick={() => { | ||
| onQuickCreateWorktree(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <GitBranch className="w-3.5 h-3.5" /> | ||
| Create Worktree | ||
| </button> | ||
| )} | ||
| {onConfigureWorktrees && ( | ||
| <button | ||
| onClick={() => { | ||
| onConfigureWorktrees(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <Settings className="w-3.5 h-3.5" /> | ||
| Configure Worktrees | ||
| </button> | ||
| )} | ||
| </> | ||
| )} | ||
|
|
||
| {showWorktreeChildSection && ( | ||
| <> | ||
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | ||
| {onCreatePR && ( | ||
| <button | ||
| onClick={() => { | ||
| onCreatePR(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <GitPullRequest className="w-3.5 h-3.5" /> | ||
| Create Pull Request | ||
| </button> | ||
| )} | ||
| {onDeleteWorktree && ( | ||
| <button | ||
| onClick={() => { | ||
| onDeleteWorktree(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.error }} | ||
| > | ||
| <Trash2 className="w-3.5 h-3.5" /> | ||
| Remove Worktree | ||
| </button> | ||
| )} | ||
| </> | ||
| )} | ||
|
|
||
| {!session.parentSessionId && ( | ||
| <> | ||
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | ||
| <button | ||
| onClick={() => { | ||
| onDelete(); | ||
| onDismiss(); | ||
| }} | ||
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | ||
| style={{ color: theme.colors.error }} | ||
| > | ||
| <Trash2 className="w-3.5 h-3.5" /> | ||
| Remove Agent |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -nP '<button(?![^>]*\btype=)' src/renderer/components/SessionList/SessionContextMenu.tsxRepository: RunMaestro/Maestro
Length of output: 287
🏁 Script executed:
wc -l src/renderer/components/SessionList/SessionContextMenu.tsxRepository: RunMaestro/Maestro
Length of output: 124
Add explicit type="button" to all menu buttons.
These buttons currently rely on the HTML default type; if this menu is ever nested in a form, actions can unintentionally submit it.
Proposed patch
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"
@@
- <button
+ <button type="button"📝 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
| <button | |
| onClick={() => { | |
| onRename(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Edit3 className="w-3.5 h-3.5" /> | |
| Rename | |
| </button> | |
| <button | |
| onClick={() => { | |
| onEdit(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Settings className="w-3.5 h-3.5" /> | |
| Edit Agent... | |
| </button> | |
| <button | |
| onClick={() => { | |
| onDuplicate(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Copy className="w-3.5 h-3.5" /> | |
| Duplicate... | |
| </button> | |
| {!session.parentSessionId && ( | |
| <button | |
| onClick={() => { | |
| onToggleBookmark(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Bookmark className="w-3.5 h-3.5" fill={session.bookmarked ? 'currentColor' : 'none'} /> | |
| {session.bookmarked ? 'Remove Bookmark' : 'Add Bookmark'} | |
| </button> | |
| )} | |
| {!session.parentSessionId && ( | |
| <div | |
| ref={moveToGroupRef} | |
| className="relative" | |
| tabIndex={0} | |
| onMouseEnter={handleMoveToGroupHover} | |
| onMouseLeave={handleMoveToGroupLeave} | |
| onFocus={handleMoveToGroupHover} | |
| onBlur={handleMoveToGroupLeave} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| handleMoveToGroupHover(); | |
| } else if (e.key === 'Escape' && showMoveSubmenu) { | |
| e.stopPropagation(); | |
| setShowMoveSubmenu(false); | |
| } | |
| }} | |
| > | |
| <button | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center justify-between" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <span className="flex items-center gap-2"> | |
| <FolderInput className="w-3.5 h-3.5" /> | |
| Move to Group | |
| </span> | |
| <ChevronRight className="w-3 h-3" /> | |
| </button> | |
| {showMoveSubmenu && ( | |
| <div | |
| className="absolute py-1 rounded-md shadow-xl border" | |
| style={{ | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| minWidth: '140px', | |
| ...(submenuPosition.vertical === 'above' ? { bottom: 0 } : { top: 0 }), | |
| ...(submenuPosition.horizontal === 'left' | |
| ? { right: '100%', marginRight: 4 } | |
| : { left: '100%', marginLeft: 4 }), | |
| }} | |
| > | |
| <button | |
| onClick={() => { | |
| onMoveToGroup(''); | |
| onDismiss(); | |
| }} | |
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${!session.groupId ? 'opacity-50' : ''}`} | |
| style={{ color: theme.colors.textMain }} | |
| disabled={!session.groupId} | |
| > | |
| <Folder className="w-3.5 h-3.5" /> | |
| Ungrouped | |
| {!session.groupId && <span className="text-[10px] opacity-50">(current)</span>} | |
| </button> | |
| {groups.length > 0 && ( | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| )} | |
| {groups.map((group) => ( | |
| <button | |
| key={group.id} | |
| onClick={() => { | |
| onMoveToGroup(group.id); | |
| onDismiss(); | |
| }} | |
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${session.groupId === group.id ? 'opacity-50' : ''}`} | |
| style={{ color: theme.colors.textMain }} | |
| disabled={session.groupId === group.id} | |
| > | |
| <span>{group.emoji}</span> | |
| <span className="truncate">{group.name}</span> | |
| {session.groupId === group.id && ( | |
| <span className="text-[10px] opacity-50">(current)</span> | |
| )} | |
| </button> | |
| ))} | |
| {onCreateGroup && ( | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| )} | |
| {onCreateGroup && ( | |
| <button | |
| onClick={() => { | |
| onCreateGroup(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <FolderPlus className="w-3.5 h-3.5" /> | |
| Create New Group | |
| </button> | |
| )} | |
| </div> | |
| )} | |
| </div> | |
| )} | |
| {showWorktreeParentSection && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| {onQuickCreateWorktree && session.worktreeConfig && ( | |
| <button | |
| onClick={() => { | |
| onQuickCreateWorktree(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <GitBranch className="w-3.5 h-3.5" /> | |
| Create Worktree | |
| </button> | |
| )} | |
| {onConfigureWorktrees && ( | |
| <button | |
| onClick={() => { | |
| onConfigureWorktrees(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <Settings className="w-3.5 h-3.5" /> | |
| Configure Worktrees | |
| </button> | |
| )} | |
| </> | |
| )} | |
| {showWorktreeChildSection && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| {onCreatePR && ( | |
| <button | |
| onClick={() => { | |
| onCreatePR(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <GitPullRequest className="w-3.5 h-3.5" /> | |
| Create Pull Request | |
| </button> | |
| )} | |
| {onDeleteWorktree && ( | |
| <button | |
| onClick={() => { | |
| onDeleteWorktree(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.error }} | |
| > | |
| <Trash2 className="w-3.5 h-3.5" /> | |
| Remove Worktree | |
| </button> | |
| )} | |
| </> | |
| )} | |
| {!session.parentSessionId && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| <button | |
| onClick={() => { | |
| onDelete(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.error }} | |
| > | |
| <Trash2 className="w-3.5 h-3.5" /> | |
| Remove Agent | |
| <button type="button" | |
| onClick={() => { | |
| onRename(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Edit3 className="w-3.5 h-3.5" /> | |
| Rename | |
| </button> | |
| <button type="button" | |
| onClick={() => { | |
| onEdit(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Settings className="w-3.5 h-3.5" /> | |
| Edit Agent... | |
| </button> | |
| <button type="button" | |
| onClick={() => { | |
| onDuplicate(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Copy className="w-3.5 h-3.5" /> | |
| Duplicate... | |
| </button> | |
| {!session.parentSessionId && ( | |
| <button type="button" | |
| onClick={() => { | |
| onToggleBookmark(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <Bookmark className="w-3.5 h-3.5" fill={session.bookmarked ? 'currentColor' : 'none'} /> | |
| {session.bookmarked ? 'Remove Bookmark' : 'Add Bookmark'} | |
| </button> | |
| )} | |
| {!session.parentSessionId && ( | |
| <div | |
| ref={moveToGroupRef} | |
| className="relative" | |
| tabIndex={0} | |
| onMouseEnter={handleMoveToGroupHover} | |
| onMouseLeave={handleMoveToGroupLeave} | |
| onFocus={handleMoveToGroupHover} | |
| onBlur={handleMoveToGroupLeave} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| handleMoveToGroupHover(); | |
| } else if (e.key === 'Escape' && showMoveSubmenu) { | |
| e.stopPropagation(); | |
| setShowMoveSubmenu(false); | |
| } | |
| }} | |
| > | |
| <button type="button" | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center justify-between" | |
| style={{ color: theme.colors.textMain }} | |
| > | |
| <span className="flex items-center gap-2"> | |
| <FolderInput className="w-3.5 h-3.5" /> | |
| Move to Group | |
| </span> | |
| <ChevronRight className="w-3 h-3" /> | |
| </button> | |
| {showMoveSubmenu && ( | |
| <div | |
| className="absolute py-1 rounded-md shadow-xl border" | |
| style={{ | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| minWidth: '140px', | |
| ...(submenuPosition.vertical === 'above' ? { bottom: 0 } : { top: 0 }), | |
| ...(submenuPosition.horizontal === 'left' | |
| ? { right: '100%', marginRight: 4 } | |
| : { left: '100%', marginLeft: 4 }), | |
| }} | |
| > | |
| <button type="button" | |
| onClick={() => { | |
| onMoveToGroup(''); | |
| onDismiss(); | |
| }} | |
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${!session.groupId ? 'opacity-50' : ''}`} | |
| style={{ color: theme.colors.textMain }} | |
| disabled={!session.groupId} | |
| > | |
| <Folder className="w-3.5 h-3.5" /> | |
| Ungrouped | |
| {!session.groupId && <span className="text-[10px] opacity-50">(current)</span>} | |
| </button> | |
| {groups.length > 0 && ( | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| )} | |
| {groups.map((group) => ( | |
| <button type="button" | |
| key={group.id} | |
| onClick={() => { | |
| onMoveToGroup(group.id); | |
| onDismiss(); | |
| }} | |
| className={`w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 ${session.groupId === group.id ? 'opacity-50' : ''}`} | |
| style={{ color: theme.colors.textMain }} | |
| disabled={session.groupId === group.id} | |
| > | |
| <span>{group.emoji}</span> | |
| <span className="truncate">{group.name}</span> | |
| {session.groupId === group.id && ( | |
| <span className="text-[10px] opacity-50">(current)</span> | |
| )} | |
| </button> | |
| ))} | |
| {onCreateGroup && ( | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| )} | |
| {onCreateGroup && ( | |
| <button type="button" | |
| onClick={() => { | |
| onCreateGroup(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <FolderPlus className="w-3.5 h-3.5" /> | |
| Create New Group | |
| </button> | |
| )} | |
| </div> | |
| )} | |
| </div> | |
| )} | |
| {showWorktreeParentSection && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| {onQuickCreateWorktree && session.worktreeConfig && ( | |
| <button type="button" | |
| onClick={() => { | |
| onQuickCreateWorktree(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <GitBranch className="w-3.5 h-3.5" /> | |
| Create Worktree | |
| </button> | |
| )} | |
| {onConfigureWorktrees && ( | |
| <button type="button" | |
| onClick={() => { | |
| onConfigureWorktrees(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <Settings className="w-3.5 h-3.5" /> | |
| Configure Worktrees | |
| </button> | |
| )} | |
| </> | |
| )} | |
| {showWorktreeChildSection && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| {onCreatePR && ( | |
| <button type="button" | |
| onClick={() => { | |
| onCreatePR(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <GitPullRequest className="w-3.5 h-3.5" /> | |
| Create Pull Request | |
| </button> | |
| )} | |
| {onDeleteWorktree && ( | |
| <button type="button" | |
| onClick={() => { | |
| onDeleteWorktree(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.error }} | |
| > | |
| <Trash2 className="w-3.5 h-3.5" /> | |
| Remove Worktree | |
| </button> | |
| )} | |
| </> | |
| )} | |
| {!session.parentSessionId && ( | |
| <> | |
| <div className="my-1 border-t" style={{ borderColor: theme.colors.border }} /> | |
| <button type="button" | |
| onClick={() => { | |
| onDelete(); | |
| onDismiss(); | |
| }} | |
| className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" | |
| style={{ color: theme.colors.error }} | |
| > | |
| <Trash2 className="w-3.5 h-3.5" /> | |
| Remove Agent |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SessionList/SessionContextMenu.tsx` around lines 149
- 377, The menu's <button> elements in SessionContextMenu.tsx (e.g., the buttons
invoking onRename/onDismiss, onEdit/onDismiss, onDuplicate/onDismiss, the Move
to Group button and submenu buttons that call onMoveToGroup, onCreateGroup,
onQuickCreateWorktree, onConfigureWorktrees, onCreatePR, onDeleteWorktree,
onDelete, onToggleBookmark, etc.) need explicit type="button" attributes to
avoid accidental form submission when nested in a form; update every <button> in
this component (including those inside the moveToGroupRef submenu and
conditional sections controlled by showMoveSubmenu, showWorktreeParentSection,
and showWorktreeChildSection) to include type="button".
reachrazamair
changed the title
4 tasks
This was referenced Mar 4, 2026
8 tasks
This was referenced Apr 11, 2026
Closed
4 tasks
This was referenced Jun 24, 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
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
New Features
Tests