refactor: App.tsx Tier 1-3 extractions (4,602 → 3,208 lines, −30%)#478
Merged
refactor: App.tsx Tier 1-3 extractions (4,602 → 3,208 lines, −30%)#478
Conversation
Extract domain logic into dedicated hooks with 385 tests: - useSymphonyContribution (52 tests) — symphony session creation + batch start - useAutoRunDocumentLoader (43 tests) — document loading, task counting, file watching - useQueueProcessing (55 tests) — queue item processing + startup recovery - usePromptComposerHandlers (47 tests) — 6 prompt composer modal handlers - useQuickActionsHandlers (42 tests) — 7 quick actions modal handlers - useCycleSession (41 tests) — session/group chat cycling in visual sidebar order - useAutoRunAchievements (37 tests) — progress tracking + badge unlocking effects - useTabExportHandlers (34 tests) — copy context, export HTML, publish gist - useQueueHandlers (21 tests) — remove/switch/reorder queue items - useTourActions (15 tests) — tour:action custom event listener effect 947 lines removed from App.tsx, all 21,382 tests pass, 0 TS errors.
… lines) Extract remaining inline functions into 2 new hooks + extend 3 existing hooks: - useInputMode (12 tests) — toggle AI/terminal mode with file tab save/restore - useLiveMode (14 tests) — global web interface toggle + server restart - useModalHandlers +handleViewGitDiff +handleDirectorNotesResumeSession (8 tests) - useWizardHandlers +handleWizardResume/StartFresh/Close (8 tests) - useSummarizeAndContinue +handleSummarizeAndContinue (8 tests) 238 lines removed from App.tsx, 50 new tests, all 21,432 tests pass, 0 TS errors.
…ines) Migrate 3 components to read directly from Zustand stores instead of receiving data as props from App.tsx: - AppOverlays: self-sources overlay data from modalStore and settings from settingsStore (5 props removed) - SettingsModal: self-sources ~60 settings from useSettings() hook, interface reduced from ~68 props to 8 props - AppModals: self-sources sessions/groups from sessionStore, groupChats from groupChatStore, and 29 modal booleans from modalStore (~37 props removed) App.tsx cleanup: removed 20 unused modal variables from useModalActions() destructuring and 50 unused settings from useSettings() destructuring. Tests: 149 new/updated tests (8 AppOverlays, 22 AppModals, 113 SettingsModal updated to use module-level mock setters). Full suite: 21,462 tests pass, 0 failures.
Tailwind Preflight sets cursor:default on buttons. The settings tab bar buttons were missing cursor-pointer, making them appear unresponsive on hover.
📝 WalkthroughWalkthroughRefactors App.tsx to extract many responsibilities into dedicated hooks; moves AppModals, AppOverlays, and SettingsModal to read state from Zustand stores instead of props; adds many new hooks (session, agent, batch, modal, remote, ui, symphony, wizard, input) and comprehensive unit tests covering components and hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Component
participant SymphonyHook as useSymphonyContribution
participant MaestroIPC as Maestro IPC
participant SessionStore as sessionStore
participant ModalStore as modalStore
participant BatchRun as Batch Runner
App->>SymphonyHook: handleStartContribution()
SymphonyHook->>MaestroIPC: validateNewSession(...)
MaestroIPC-->>SymphonyHook: validation result
SymphonyHook->>MaestroIPC: isRepo / getGitBranch/getTags
MaestroIPC-->>SymphonyHook: git metadata
SymphonyHook->>SessionStore: addSession(newSession)
SymphonyHook->>SessionStore: setActiveSessionId(newSessionId)
SymphonyHook->>ModalStore: setSymphonyModalOpen(false)
SymphonyHook->>MaestroIPC: registerActive(payload)
SymphonyHook->>BatchRun: startBatchRun(sessionId, config)
BatchRun-->>App: batch run initiated
sequenceDiagram
participant UI as UI Component
participant QueueHook as useQueueProcessing
participant SessionStore as sessionStore
participant AgentStore as agentStore
participant TabState as Tab State
UI->>QueueHook: initialize (deps)
QueueHook->>SessionStore: read sessions after sessionsLoaded
SessionStore-->>QueueHook: sessions with leftover queues
Note over QueueHook: Detect startup leftover queued items
QueueHook->>TabState: mark target tab busy
QueueHook->>AgentStore: processQueuedItem(sessionId, item, refs)
AgentStore-->>QueueHook: success/failure
alt success
QueueHook->>SessionStore: remove item from queue
QueueHook->>TabState: update thinking state
else failure
QueueHook->>SessionStore: re-queue item and reset busy flags
end
UI->>QueueHook: processQueuedItem(sessionId, item)
QueueHook->>AgentStore: delegate processing call
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryMethodical refactoring reducing App.tsx from 4,602 to 3,208 lines (−30%) through three extraction tiers executed across four commits. No logic changes—purely architectural improvements. Tier 2 (10 Hook Extractions, commit
Tier 3 (Inline Function Extractions, commit
Tier 1 (Component Self-Sourcing, commit
Bug Fix (commit
Test Coverage: 21,462 tests pass (12,668 new/updated lines of tests). TypeScript compiles with 0 errors. All hooks follow consistent patterns with dependency injection and return type interfaces. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TB
subgraph "Before: App.tsx (4,602 lines)"
A[App.tsx] --> A1[All Logic]
A --> A2[Props Drilling]
A --> A3[Inline Functions]
end
subgraph "After: App.tsx (3,208 lines, -30%)"
B[App.tsx Core]
end
subgraph "Tier 2: 10 Hook Extractions (947 lines)"
H1[useSymphonyContribution]
H2[useAutoRunDocumentLoader]
H3[useQueueProcessing]
H4[usePromptComposerHandlers]
H5[useQuickActionsHandlers]
H6[useCycleSession]
H7[useAutoRunAchievements]
H8[useTabExportHandlers]
H9[useQueueHandlers]
H10[useTourActions]
end
subgraph "Tier 3: Inline Functions (238 lines)"
H11[useInputMode]
H12[useLiveMode]
H13[useModalHandlers<br/>extended]
end
subgraph "Tier 1: Self-Sourcing (209 lines removed)"
C1[AppModals<br/>→ stores]
C2[AppOverlays<br/>→ stores]
C3[SettingsModal<br/>→ useSettings]
end
A --> B
A --> H1
A --> H2
A --> H3
A --> H4
A --> H5
A --> H6
A --> H7
A --> H8
A --> H9
A --> H10
A --> H11
A --> H12
A --> H13
A --> C1
A --> C2
A --> C3
B --> |uses| H1
B --> |uses| H2
B --> |uses| H3
B --> |uses| H11
B --> |uses| H12
style A fill:#ff6b6b
style B fill:#51cf66
style H1 fill:#4dabf7
style H2 fill:#4dabf7
style H3 fill:#4dabf7
style H4 fill:#4dabf7
style H5 fill:#4dabf7
style H6 fill:#4dabf7
style H7 fill:#4dabf7
style H8 fill:#4dabf7
style H9 fill:#4dabf7
style H10 fill:#4dabf7
style H11 fill:#748ffc
style H12 fill:#748ffc
style H13 fill:#748ffc
style C1 fill:#ffd43b
style C2 fill:#ffd43b
style C3 fill:#ffd43b
Last reviewed commit: ca5b5aa |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/SettingsModal.tsx (2)
1058-1071:⚠️ Potential issue | 🟡 MinorGuard against
activeThemeIdnot found before cycling themes.If
findIndexreturns-1, Shift+Tab can compute an invalid index and passundefinedintosetActiveThemeId. Please normalize the index before computing next/prev.🔧 Suggested fix
-const currentIndex = allThemeIds.findIndex((id: string) => id === activeThemeId); +const currentIndex = allThemeIds.findIndex((id: string) => id === activeThemeId); +const safeIndex = currentIndex >= 0 ? currentIndex : 0; let newThemeId: string; if (e.shiftKey) { - const prevIndex = currentIndex === 0 ? allThemeIds.length - 1 : currentIndex - 1; + const prevIndex = (safeIndex - 1 + allThemeIds.length) % allThemeIds.length; newThemeId = allThemeIds[prevIndex]; } else { - const nextIndex = (currentIndex + 1) % allThemeIds.length; + const nextIndex = (safeIndex + 1) % allThemeIds.length; newThemeId = allThemeIds[nextIndex]; } setActiveThemeId(newThemeId as ThemeId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 1058 - 1071, currentIndex may be -1 when activeThemeId is not found, causing invalid indexing; before computing prev/next normalize currentIndex (e.g., if currentIndex === -1 set currentIndex to 0 or to allThemeIds.length - 1 depending on desired wrap behavior) and then compute prevIndex/nextIndex and call setActiveThemeId(newThemeId as ThemeId); update the logic around allThemeIds, activeThemeId, currentIndex, prevIndex/nextIndex and setActiveThemeId to ensure newThemeId is never undefined.
1171-1258:⚠️ Potential issue | 🟠 MajorSettings tab controls remain keyboard-inaccessible (
tabIndex={-1}).Line 1174+ and Line 1257 keep interactive controls out of normal Tab navigation. This blocks keyboard users from reaching top-level settings tabs/close via standard focus flow.
As per coding guidelines "src/renderer/components/**/*.{ts,tsx}: Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".♿ Suggested direction
-<div className="flex border-b" style={{ borderColor: theme.colors.border }}> +<div + className="flex border-b" + style={{ borderColor: theme.colors.border }} + role="tablist" + aria-label="Settings sections" +> <button + role="tab" + aria-selected={activeTab === 'general'} onClick={() => setActiveTab('general')} - tabIndex={-1} + onFocus={() => setActiveTab('general')} + tabIndex={activeTab === 'general' ? 0 : -1} ... >Apply the same pattern across all tab buttons, and keep the close button keyboard-reachable (
tabIndex={0}or omittabIndex).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 1171 - 1258, The tab buttons in SettingsModal are keyboard-inaccessible because they all set tabIndex={-1}; update the interactive elements (the tab buttons that call setActiveTab and the close button that calls onClose/X) to be keyboard-focusable (remove tabIndex or use tabIndex={0}) and add keyboard handlers (onKeyDown) that trigger the same actions on Enter/Space so keyboard users can activate tabs and close the modal; ensure this change is applied to all buttons referencing setActiveTab (e.g., 'general','display','llm','shortcuts','theme','notifications','aicommands','ssh','encore') and the close button that calls onClose/X, and keep existing visual focus styles tied to activeTab.
🧹 Nitpick comments (13)
src/renderer/hooks/input/useInputMode.ts (1)
42-75: Consider reading UI store state before the session update.The current pattern calls
useUIStore.getState()inside thesetSessionscallback. While this works, it creates a dependency on external store state during the session update. ReadingpreTerminalFileTabIdbefore thesetSessionscall would be cleaner and avoid potential timing issues.♻️ Suggested refactor
const toggleInputMode = useCallback(() => { const { setSessions, activeSessionId } = useSessionStore.getState(); + const { preTerminalFileTabId, setPreTerminalFileTabId } = useUIStore.getState(); setSessions((prev) => prev.map((s) => { if (s.id !== activeSessionId) return s; const newMode = s.inputMode === 'ai' ? 'terminal' : 'ai'; if (newMode === 'terminal') { // Switching to terminal mode: save current file tab (if any) and clear it - useUIStore.getState().setPreTerminalFileTabId(s.activeFileTabId); + setPreTerminalFileTabId(s.activeFileTabId); return { ...s, inputMode: newMode, activeFileTabId: null, }; } else { // Switching to AI mode: restore previous file tab if it still exists - const savedFileTabId = useUIStore.getState().preTerminalFileTabId; const fileTabStillExists = - savedFileTabId && s.filePreviewTabs?.some((t) => t.id === savedFileTabId); - useUIStore.getState().setPreTerminalFileTabId(null); + preTerminalFileTabId && s.filePreviewTabs?.some((t) => t.id === preTerminalFileTabId); + setPreTerminalFileTabId(null); return { ...s, inputMode: newMode, - ...(fileTabStillExists && { activeFileTabId: savedFileTabId }), + ...(fileTabStillExists && { activeFileTabId: preTerminalFileTabId }), }; } }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/input/useInputMode.ts` around lines 42 - 75, Read preTerminalFileTabId from useUIStore once at the start of toggleInputMode and capture it in a local variable, then use that variable inside the setSessions callback instead of calling useUIStore.getState() there; keep the existing useUIStore.getState().setPreTerminalFileTabId(s.activeFileTabId) call when switching to terminal mode (or move it just before setSessions if you prefer), and null out the UI store's preTerminalFileTabId after mapping when switching to AI mode by calling setPreTerminalFileTabId(null) outside or using the captured variable to decide restoration—update the code points toggleInputMode, setSessions, useUIStore.getState(), setPreTerminalFileTabId, and preTerminalFileTabId accordingly.src/__tests__/renderer/hooks/useLiveMode.test.ts (1)
152-175: Assert the state invariants in the disableAll error path.The test comment says state should remain unchanged, but it currently only checks logging. Add explicit state assertions to lock in that behavior.
Suggested test hardening
it('toggleGlobalLive handles disableAll error when turning off', async () => { const { result } = renderHook(() => useLiveMode()); @@ await act(async () => { await result.current.toggleGlobalLive(); }); // State should NOT have changed because the error was caught before setState + expect(result.current.isLiveMode).toBe(true); + expect(result.current.webInterfaceUrl).toBe('http://localhost:3000'); expect(consoleSpy).toHaveBeenCalledWith('[toggleGlobalLive] Error:', error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useLiveMode.test.ts` around lines 152 - 175, The test currently only verifies the console error when mockLive.disableAll rejects but doesn't assert the component state; update the test for toggleGlobalLive to also assert that result.current.isLiveMode remains true after the failed disableAll call (and optionally that any related state toggles haven't occurred), referencing the useLiveMode hook's toggleGlobalLive method and isLiveMode property and the mockLive.disableAll rejection and console.error spy so the state invariant is explicitly checked in the error path.src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts (1)
149-151: Optional teardown hardening for global spies.Adding
vi.restoreAllMocks()inafterEachwill ensuresetInterval/clearIntervalspies are always restored cleanly between tests.Optional teardown update
afterEach(() => { vi.useRealTimers(); + vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts` around lines 149 - 151, In the afterEach teardown that currently calls vi.useRealTimers(), also call vi.restoreAllMocks() to ensure any global spies (like those on setInterval/clearInterval) are reset between tests; update the afterEach block (the one containing vi.useRealTimers()) to invoke vi.restoreAllMocks() before or after useRealTimers() so spies are consistently restored.src/__tests__/renderer/components/AppOverlays.test.tsx (1)
41-47: Shortcut propagation test should assert actual shortcut data.The current test validates render presence only; it does not prove
shortcutsis passed through.Suggested assertion upgrade
vi.mock('../../../renderer/components/KeyboardMasteryCelebration', () => ({ KeyboardMasteryCelebration: (props: Record<string, unknown>) => ( <div data-testid="keyboard-mastery-celebration" data-level={props.level} + data-shortcut-count={String(Object.keys((props.shortcuts as Record<string, unknown>) ?? {}).length)} data-disable-confetti={String(props.disableConfetti)} > KeyboardMasteryCelebration </div> ), })); @@ it('reads shortcuts from settingsStore and passes to KeyboardMasteryCelebration', () => { @@ - expect(screen.getByTestId('keyboard-mastery-celebration')).toBeInTheDocument(); + expect(screen.getByTestId('keyboard-mastery-celebration')).toHaveAttribute( + 'data-shortcut-count', + '1' + ); });Also applies to: 161-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AppOverlays.test.tsx` around lines 41 - 47, The test currently only checks that the mocked KeyboardMasteryCelebration rendered; update the assertions to verify the actual shortcuts data is propagated by asserting the mocked component received the correct shortcuts prop (e.g., inspect props.shortcuts or a specific key/command expected) when rendering AppOverlays; locate the mock for KeyboardMasteryCelebration and the relevant tests in AppOverlays.test.tsx (both the earlier block and the repeated case around lines 161-168) and replace or augment the render presence checks with assertions that compare props.shortcuts to the expected shortcuts object/array for that test scenario.src/renderer/hooks/session/useCycleSession.ts (1)
149-158: Inconsistent sorting: group chats uselocaleComparewhile sessions usecompareNamesIgnoringEmojis.Sessions and groups are sorted using
compareNamesIgnoringEmojis(lines 124, 129, 134, 143), but group chats use directlocaleCompare(line 150). If group chat names include emojis, they'll sort differently than sessions.♻️ Consistent sorting for group chats
// Group Chats section (if expanded and has group chats) if (groupChatsExpanded && groupChats.length > 0) { const sortedGroupChats = [...groupChats].sort((a, b) => - a.name.toLowerCase().localeCompare(b.name.toLowerCase()) + compareNamesIgnoringEmojis(a.name, b.name) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useCycleSession.ts` around lines 149 - 158, Group chat sorting is inconsistent: sortedGroupChats currently uses a.name.toLowerCase().localeCompare(b.name.toLowerCase()) while sessions use compareNamesIgnoringEmojis, causing different ordering when names contain emojis. Change the sort for groupChats to use compareNamesIgnoringEmojis(a.name, b.name) (or the same comparator used for sessions) so sortedGroupChats and session arrays share identical comparison logic before pushing into visualOrder; keep the mapping to type:'groupChat', id:gc.id, name:gc.name unchanged.src/renderer/hooks/agent/useQueueHandlers.ts (1)
52-65: Consider adding boundary checks for reorder indices.If
fromIndexis out of bounds,splice(fromIndex, 1)returns an empty array, makingremovedundefined. Insertingundefinedinto the queue could corrupt state. While the UI likely prevents invalid indices, defensive checks would make this more robust.🛡️ Optional defensive guard
const handleReorderQueueItems = useCallback( (sessionId: string, fromIndex: number, toIndex: number) => { setSessions((prev) => prev.map((s) => { if (s.id !== sessionId) return s; const queue = [...s.executionQueue]; + if (fromIndex < 0 || fromIndex >= queue.length || toIndex < 0 || toIndex > queue.length) { + return s; + } const [removed] = queue.splice(fromIndex, 1); queue.splice(toIndex, 0, removed); return { ...s, executionQueue: queue }; }) ); }, [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useQueueHandlers.ts` around lines 52 - 65, The handleReorderQueueItems callback should guard against invalid indices to avoid inserting undefined into executionQueue; inside the setSessions updater for the matching session (the block that reads s.executionQueue and uses splice), validate and/or clamp fromIndex and toIndex against 0 and queue.length-1 (or no-op and return s if fromIndex is out of bounds or equal to toIndex), ensure that the splice removing an item yields a defined removed value before inserting, and only update the session when a valid item was actually removed.src/__tests__/renderer/hooks/useAutoRunDocumentLoader.test.ts (1)
759-762: Emptyact(async () => {})pattern for settling effects.While this works, it relies on microtask timing. Consider using
waitForwith a condition instead for more explicit waiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useAutoRunDocumentLoader.test.ts` around lines 759 - 762, The test uses an empty act(async () => {}) to let effects settle; replace that fragile pattern with React Testing Library's waitFor to explicitly wait for the desired condition. In this test (referencing mockWatchFolder and the assertion expect(mockWatchFolder).not.toHaveBeenCalled()), remove the empty act and instead await waitFor with a callback that asserts mockWatchFolder has not been called (or waits for mockWatchFolder.mock.calls.length to be 0) so the test reliably waits for side effects to complete.src/__tests__/renderer/components/AppModals-selfSourced.test.tsx (2)
418-592: Self-sourcing tests need observable assertions, not just mount/unmount success.These cases currently pass even if store reads regress, as long as render doesn’t throw. Please assert at least one observable output (e.g., mocked child receives expected store-derived props before/after store updates).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AppModals-selfSourced.test.tsx` around lines 418 - 592, Tests in AppModals self-sourcing suites only mount/unmount which hides regressions; update each test that currently only renders to include at least one observable assertion: after setting useSessionStore/useGroupChatStore/useModalStore state (or calling useModalStore.getState().openModal) assert a DOM change or prop passed to a mocked child of AppModals (e.g., verify text rendered, a button visible, or the mocked child received expected props) and after act() updates assert the UI/props changed accordingly; target symbols: AppModals, useSessionStore, useGroupChatStore, useModalStore, createDefaultProps, and openModal when adding expect() checks so tests fail if self-sourcing regresses.
595-693: Prop interface checks are validating the fixture object more than component behavior.
expect(props).not.toHaveProperty(...)verifiescreateDefaultProps, not AppModals behavior. Consider replacing these with assertions tied to rendered behavior (or type-level contract tests).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AppModals-selfSourced.test.tsx` around lines 595 - 693, The tests are currently asserting properties on the fixture object (createDefaultProps) instead of verifying AppModals behavior; update the tests that use createDefaultProps and removedBooleanProps to render <AppModals {...props}/> and assert rendered behavior (e.g., use queryByText/queryByTestId or ensure modal DOM nodes are not present/open by default) rather than expect(props).not.toHaveProperty(...). Specifically, for tests referencing createDefaultProps and removedBooleanProps, replace the props-property assertions with assertions that the corresponding modal elements are absent or closed when rendered (use AppModals rendering and DOM queries), and keep the two positive tests (createGroupModalOpen and showDeleteGroupChatModal) by asserting rendered effects instead of fixture shape.src/renderer/components/AppModals.tsx (1)
2108-2119: Consider consolidating session store selectors for consistency.Lines 2109-2113 use five separate
useSessionStorecalls, while the modal states (lines 2152-2184) use a single consolidated selector withuseShallow. For consistency and potentially fewer subscriptions, you could consolidate the session store selectors similarly:♻️ Optional consolidation
- const sessions = useSessionStore((s) => s.sessions); - const activeSessionId = useSessionStore((s) => s.activeSessionId); - const groups = useSessionStore((s) => s.groups); - const setSessions = useSessionStore((s) => s.setSessions); - const setGroups = useSessionStore((s) => s.setGroups); + const { sessions, activeSessionId, groups, setSessions, setGroups } = useSessionStore( + useShallow((s) => ({ + sessions: s.sessions, + activeSessionId: s.activeSessionId, + groups: s.groups, + setSessions: s.setSessions, + setGroups: s.setGroups, + })) + );The current approach is functionally correct—individual selectors for primitives/stable references work fine. This is purely a stylistic suggestion for consistency with the modal store pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AppModals.tsx` around lines 2108 - 2119, The code calls useSessionStore five times (sessions, activeSessionId, groups, setSessions, setGroups) which is inconsistent with the modal pattern that uses a single consolidated selector with useShallow; replace the multiple selectors with one combined selector like useSessionStore((s) => ({ sessions: s.sessions, activeSessionId: s.activeSessionId, groups: s.groups, setSessions: s.setSessions, setGroups: s.setGroups }), useShallow) and then derive activeSession from the consolidated sessions/activeSessionId (or inside useMemo using the combined values) so subscriptions are consistent and fewer; update references to sessions, activeSessionId, groups, setSessions, setGroups accordingly and ensure useShallow is imported and used.src/renderer/hooks/agent/useSummarizeAndContinue.ts (1)
382-383: Avoid parsingreductionPercentfrom a display string.Line 382 extracts numeric data from
systemLogEntry.textvia regex. This is brittle against message wording changes. Prefer returning structuredreductionPercentfromstartSummarizeand using it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 382 - 383, The code extracts reductionPercent by regexing result.systemLogEntry.text (reductionPercent = result.systemLogEntry.text.match(/(\d+)%/)?.[1] ?? '0'), which is brittle; instead change startSummarize to return a structured reductionPercent value (e.g., add reductionPercent to the result object) and use that field directly when calling notifyToast and elsewhere; update callers in useSummarizeAndContinue to read result.reductionPercent (falling back to 0 only if missing) rather than parsing result.systemLogEntry.text, and remove the regex parsing from this file.src/__tests__/renderer/hooks/useSummarizeHandler.test.ts (2)
227-246: Strengthen the success assertion to verify actual session mutation.
expect(updatedSessions.length).toBeGreaterThan(0)is too weak and can pass even if the update logic regresses. Assert Line 244 state for new tab insertion and active-tab switch.Suggested stronger assertions
// Session should have been updated in the store via setSessions const updatedSessions = useSessionStore.getState().sessions; - expect(updatedSessions.length).toBeGreaterThan(0); + const updated = updatedSessions.find((s) => s.id === session.id); + expect(updated).toBeDefined(); + expect(updated?.activeTabId).toBe('new-tab-1'); + expect(updated?.aiTabs.some((tab) => tab.id === 'new-tab-1')).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSummarizeHandler.test.ts` around lines 227 - 246, Replace the weak length-only assertion with checks that the session was actually mutated: after calling useSummarizeAndContinue().handleSummarizeAndContinue() and verifying createTabAtPosition was called, read useSessionStore.getState() and assert that (1) the sessions array contains a new tab object associated with the original session id (or a tab with expected properties produced by the summarization) and (2) useSessionStore.getState().activeSessionId equals the id of the newly created/activated tab (or the expected active id); reference useSummarizeAndContinue, handleSummarizeAndContinue, createTabAtPosition, and useSessionStore/session.id to locate the relevant state and assertions to update.
156-319: Add coverage for summarize failure after validation passes.There’s no test that simulates
summarizeContextfailing (orstartSummarizeresolvingnull) aftercanSummarize === true. Add one to lock down failure feedback behavior and prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSummarizeHandler.test.ts` around lines 156 - 319, Add a new test in the describe('handleSummarizeAndContinue (Tier 3E)') block that sets contextSummarizationService.canSummarize to true but then simulates startSummarize/summarizeContext failing (either by mocking summarizeContext to reject or making startSummarize resolve to null), call result.current.handleSummarizeAndContinue(), await the async chain with vi.waitFor, and assert that failure feedback is emitted (e.g., notifyToast called with a warning/error about compacting failing) and that success side effects do NOT occur (createTabAtPosition not called, session store not updated, and tab summarize state remains intact); reference useSummarizeAndContinue, handleSummarizeAndContinue, contextSummarizationService.summarizeContext (or startSummarize), notifyToast, createTabAtPosition, and useSessionStore/useOperationStore when adding assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 359-395: startSummarize’s promise currently ignores the
non-success and rejection paths; add handling so when startSummarize returns
null/undefined or throws you display a failure toast and clean up state.
Specifically, update the block around startSummarize(targetTabId).then(...) to
include an else branch that calls notifyToast with an error type/message
(include sessionId: sourceSessionId and tabId: targetTabId) and invokes
clearTabState(targetTabId) if needed, and add a .catch(...) to log the error and
call notifyToast for unexpected failures; reference startSummarize, notifyToast,
clearTabState and setSessions in your changes.
- Around line 359-379: The two separate setSessions calls can race and clobber
concurrent updates; instead perform a single atomic update using the functional
updater: inside the then((result) => ...) call, call setSessions(prev =>
prev.map(s => { if (s.id !== sourceSessionId) return s; const merged = { ...s,
...result.updatedSession }; return { ...merged, aiTabs: merged.aiTabs.map(tab =>
tab.id === targetTabId ? { ...tab, logs: [...tab.logs, result.systemLogEntry] }
: tab) }; })); this uses the current store state, merges result.updatedSession
into the existing session (preserving any concurrent changes), and appends
result.systemLogEntry to the target tab in one atomic update.
In `@src/renderer/hooks/remote/useLiveMode.ts`:
- Around line 38-59: The toggleGlobalLive handler can leave UI state
inconsistent if maestro.tunnel.stop() succeeds but maestro.live.disableAll()
throws; update toggleGlobalLive to update state immediately after each
successful step or perform compensating actions on partial failure: when turning
off, call (window as any).maestro.tunnel.stop() and if that resolves set
setIsLiveMode(false) and setWebInterfaceUrl(null) before calling (window as
any).maestro.live.disableAll(); if disableAll() then fails, log the error and
attempt a compensating action (e.g., try to restart the tunnel or call a cleanup
API) or mark a separate error state so the UI reflects the partial failure;
likewise, when enabling, ensure setIsLiveMode(true) and
setWebInterfaceUrl(result.url) are only set after startServer() succeeds and
roll back them if any follow-up step fails—adjust the logic in the
toggleGlobalLive useCallback to perform these stepwise state updates and
compensations (references: toggleGlobalLive, isLiveMode, setIsLiveMode,
setWebInterfaceUrl, maestro.tunnel.stop, maestro.live.disableAll,
maestro.live.startServer).
In `@src/renderer/hooks/tabs/useTabExportHandlers.ts`:
- Around line 94-95: The call in handleExportHtml passes themeRef.current with a
non-null assertion which can crash if it's null; modify handleExportHtml to
guard themeRef.current (the themeRef used in downloadTabExport) by checking if
themeRef.current is present before calling downloadTabExport and either return
early with a safe error/notification or provide a sensible fallback theme
object; ensure the check references themeRef.current and the call to
downloadTabExport so the function never receives a null value.
---
Outside diff comments:
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 1058-1071: currentIndex may be -1 when activeThemeId is not found,
causing invalid indexing; before computing prev/next normalize currentIndex
(e.g., if currentIndex === -1 set currentIndex to 0 or to allThemeIds.length - 1
depending on desired wrap behavior) and then compute prevIndex/nextIndex and
call setActiveThemeId(newThemeId as ThemeId); update the logic around
allThemeIds, activeThemeId, currentIndex, prevIndex/nextIndex and
setActiveThemeId to ensure newThemeId is never undefined.
- Around line 1171-1258: The tab buttons in SettingsModal are
keyboard-inaccessible because they all set tabIndex={-1}; update the interactive
elements (the tab buttons that call setActiveTab and the close button that calls
onClose/X) to be keyboard-focusable (remove tabIndex or use tabIndex={0}) and
add keyboard handlers (onKeyDown) that trigger the same actions on Enter/Space
so keyboard users can activate tabs and close the modal; ensure this change is
applied to all buttons referencing setActiveTab (e.g.,
'general','display','llm','shortcuts','theme','notifications','aicommands','ssh','encore')
and the close button that calls onClose/X, and keep existing visual focus styles
tied to activeTab.
---
Nitpick comments:
In `@src/__tests__/renderer/components/AppModals-selfSourced.test.tsx`:
- Around line 418-592: Tests in AppModals self-sourcing suites only
mount/unmount which hides regressions; update each test that currently only
renders to include at least one observable assertion: after setting
useSessionStore/useGroupChatStore/useModalStore state (or calling
useModalStore.getState().openModal) assert a DOM change or prop passed to a
mocked child of AppModals (e.g., verify text rendered, a button visible, or the
mocked child received expected props) and after act() updates assert the
UI/props changed accordingly; target symbols: AppModals, useSessionStore,
useGroupChatStore, useModalStore, createDefaultProps, and openModal when adding
expect() checks so tests fail if self-sourcing regresses.
- Around line 595-693: The tests are currently asserting properties on the
fixture object (createDefaultProps) instead of verifying AppModals behavior;
update the tests that use createDefaultProps and removedBooleanProps to render
<AppModals {...props}/> and assert rendered behavior (e.g., use
queryByText/queryByTestId or ensure modal DOM nodes are not present/open by
default) rather than expect(props).not.toHaveProperty(...). Specifically, for
tests referencing createDefaultProps and removedBooleanProps, replace the
props-property assertions with assertions that the corresponding modal elements
are absent or closed when rendered (use AppModals rendering and DOM queries),
and keep the two positive tests (createGroupModalOpen and
showDeleteGroupChatModal) by asserting rendered effects instead of fixture
shape.
In `@src/__tests__/renderer/components/AppOverlays.test.tsx`:
- Around line 41-47: The test currently only checks that the mocked
KeyboardMasteryCelebration rendered; update the assertions to verify the actual
shortcuts data is propagated by asserting the mocked component received the
correct shortcuts prop (e.g., inspect props.shortcuts or a specific key/command
expected) when rendering AppOverlays; locate the mock for
KeyboardMasteryCelebration and the relevant tests in AppOverlays.test.tsx (both
the earlier block and the repeated case around lines 161-168) and replace or
augment the render presence checks with assertions that compare props.shortcuts
to the expected shortcuts object/array for that test scenario.
In `@src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts`:
- Around line 149-151: In the afterEach teardown that currently calls
vi.useRealTimers(), also call vi.restoreAllMocks() to ensure any global spies
(like those on setInterval/clearInterval) are reset between tests; update the
afterEach block (the one containing vi.useRealTimers()) to invoke
vi.restoreAllMocks() before or after useRealTimers() so spies are consistently
restored.
In `@src/__tests__/renderer/hooks/useAutoRunDocumentLoader.test.ts`:
- Around line 759-762: The test uses an empty act(async () => {}) to let effects
settle; replace that fragile pattern with React Testing Library's waitFor to
explicitly wait for the desired condition. In this test (referencing
mockWatchFolder and the assertion
expect(mockWatchFolder).not.toHaveBeenCalled()), remove the empty act and
instead await waitFor with a callback that asserts mockWatchFolder has not been
called (or waits for mockWatchFolder.mock.calls.length to be 0) so the test
reliably waits for side effects to complete.
In `@src/__tests__/renderer/hooks/useLiveMode.test.ts`:
- Around line 152-175: The test currently only verifies the console error when
mockLive.disableAll rejects but doesn't assert the component state; update the
test for toggleGlobalLive to also assert that result.current.isLiveMode remains
true after the failed disableAll call (and optionally that any related state
toggles haven't occurred), referencing the useLiveMode hook's toggleGlobalLive
method and isLiveMode property and the mockLive.disableAll rejection and
console.error spy so the state invariant is explicitly checked in the error
path.
In `@src/__tests__/renderer/hooks/useSummarizeHandler.test.ts`:
- Around line 227-246: Replace the weak length-only assertion with checks that
the session was actually mutated: after calling
useSummarizeAndContinue().handleSummarizeAndContinue() and verifying
createTabAtPosition was called, read useSessionStore.getState() and assert that
(1) the sessions array contains a new tab object associated with the original
session id (or a tab with expected properties produced by the summarization) and
(2) useSessionStore.getState().activeSessionId equals the id of the newly
created/activated tab (or the expected active id); reference
useSummarizeAndContinue, handleSummarizeAndContinue, createTabAtPosition, and
useSessionStore/session.id to locate the relevant state and assertions to
update.
- Around line 156-319: Add a new test in the
describe('handleSummarizeAndContinue (Tier 3E)') block that sets
contextSummarizationService.canSummarize to true but then simulates
startSummarize/summarizeContext failing (either by mocking summarizeContext to
reject or making startSummarize resolve to null), call
result.current.handleSummarizeAndContinue(), await the async chain with
vi.waitFor, and assert that failure feedback is emitted (e.g., notifyToast
called with a warning/error about compacting failing) and that success side
effects do NOT occur (createTabAtPosition not called, session store not updated,
and tab summarize state remains intact); reference useSummarizeAndContinue,
handleSummarizeAndContinue, contextSummarizationService.summarizeContext (or
startSummarize), notifyToast, createTabAtPosition, and
useSessionStore/useOperationStore when adding assertions.
In `@src/renderer/components/AppModals.tsx`:
- Around line 2108-2119: The code calls useSessionStore five times (sessions,
activeSessionId, groups, setSessions, setGroups) which is inconsistent with the
modal pattern that uses a single consolidated selector with useShallow; replace
the multiple selectors with one combined selector like useSessionStore((s) => ({
sessions: s.sessions, activeSessionId: s.activeSessionId, groups: s.groups,
setSessions: s.setSessions, setGroups: s.setGroups }), useShallow) and then
derive activeSession from the consolidated sessions/activeSessionId (or inside
useMemo using the combined values) so subscriptions are consistent and fewer;
update references to sessions, activeSessionId, groups, setSessions, setGroups
accordingly and ensure useShallow is imported and used.
In `@src/renderer/hooks/agent/useQueueHandlers.ts`:
- Around line 52-65: The handleReorderQueueItems callback should guard against
invalid indices to avoid inserting undefined into executionQueue; inside the
setSessions updater for the matching session (the block that reads
s.executionQueue and uses splice), validate and/or clamp fromIndex and toIndex
against 0 and queue.length-1 (or no-op and return s if fromIndex is out of
bounds or equal to toIndex), ensure that the splice removing an item yields a
defined removed value before inserting, and only update the session when a valid
item was actually removed.
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 382-383: The code extracts reductionPercent by regexing
result.systemLogEntry.text (reductionPercent =
result.systemLogEntry.text.match(/(\d+)%/)?.[1] ?? '0'), which is brittle;
instead change startSummarize to return a structured reductionPercent value
(e.g., add reductionPercent to the result object) and use that field directly
when calling notifyToast and elsewhere; update callers in
useSummarizeAndContinue to read result.reductionPercent (falling back to 0 only
if missing) rather than parsing result.systemLogEntry.text, and remove the regex
parsing from this file.
In `@src/renderer/hooks/input/useInputMode.ts`:
- Around line 42-75: Read preTerminalFileTabId from useUIStore once at the start
of toggleInputMode and capture it in a local variable, then use that variable
inside the setSessions callback instead of calling useUIStore.getState() there;
keep the existing
useUIStore.getState().setPreTerminalFileTabId(s.activeFileTabId) call when
switching to terminal mode (or move it just before setSessions if you prefer),
and null out the UI store's preTerminalFileTabId after mapping when switching to
AI mode by calling setPreTerminalFileTabId(null) outside or using the captured
variable to decide restoration—update the code points toggleInputMode,
setSessions, useUIStore.getState(), setPreTerminalFileTabId, and
preTerminalFileTabId accordingly.
In `@src/renderer/hooks/session/useCycleSession.ts`:
- Around line 149-158: Group chat sorting is inconsistent: sortedGroupChats
currently uses a.name.toLowerCase().localeCompare(b.name.toLowerCase()) while
sessions use compareNamesIgnoringEmojis, causing different ordering when names
contain emojis. Change the sort for groupChats to use
compareNamesIgnoringEmojis(a.name, b.name) (or the same comparator used for
sessions) so sortedGroupChats and session arrays share identical comparison
logic before pushing into visualOrder; keep the mapping to type:'groupChat',
id:gc.id, name:gc.name unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
src/__tests__/renderer/components/AppModals-selfSourced.test.tsxsrc/__tests__/renderer/components/AppOverlays.test.tsxsrc/__tests__/renderer/components/SettingsModal.test.tsxsrc/__tests__/renderer/hooks/useAutoRunAchievements.test.tssrc/__tests__/renderer/hooks/useAutoRunDocumentLoader.test.tssrc/__tests__/renderer/hooks/useCycleSession.test.tssrc/__tests__/renderer/hooks/useInputMode.test.tssrc/__tests__/renderer/hooks/useLiveMode.test.tssrc/__tests__/renderer/hooks/useModalHandlers.test.tssrc/__tests__/renderer/hooks/usePromptComposerHandlers.test.tssrc/__tests__/renderer/hooks/useQueueHandlers.test.tssrc/__tests__/renderer/hooks/useQueueProcessing.test.tssrc/__tests__/renderer/hooks/useQuickActionsHandlers.test.tssrc/__tests__/renderer/hooks/useSummarizeHandler.test.tssrc/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/__tests__/renderer/hooks/useTabExportHandlers.test.tssrc/__tests__/renderer/hooks/useTourActions.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/renderer/App.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/AppOverlays.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/hooks/agent/index.tssrc/renderer/hooks/agent/useQueueHandlers.tssrc/renderer/hooks/agent/useQueueProcessing.tssrc/renderer/hooks/agent/useSummarizeAndContinue.tssrc/renderer/hooks/batch/index.tssrc/renderer/hooks/batch/useAutoRunAchievements.tssrc/renderer/hooks/batch/useAutoRunDocumentLoader.tssrc/renderer/hooks/input/index.tssrc/renderer/hooks/input/useInputMode.tssrc/renderer/hooks/modal/index.tssrc/renderer/hooks/modal/useModalHandlers.tssrc/renderer/hooks/modal/usePromptComposerHandlers.tssrc/renderer/hooks/modal/useQuickActionsHandlers.tssrc/renderer/hooks/remote/index.tssrc/renderer/hooks/remote/useLiveMode.tssrc/renderer/hooks/session/index.tssrc/renderer/hooks/session/useCycleSession.tssrc/renderer/hooks/symphony/index.tssrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/hooks/tabs/index.tssrc/renderer/hooks/tabs/useTabExportHandlers.tssrc/renderer/hooks/ui/index.tssrc/renderer/hooks/ui/useTourActions.tssrc/renderer/hooks/wizard/useWizardHandlers.ts
Code fixes:
- useSummarizeAndContinue: add error/null handling for startSummarize
promise, merge two setSessions calls into single atomic update
- useLiveMode: update state after tunnel.stop() before disableAll()
so UI stays consistent on partial failure
- useTabExportHandlers: guard themeRef.current null before export
- SettingsModal: guard currentIndex=-1 in theme picker keyboard nav,
remove tabIndex={-1} from tab buttons for keyboard accessibility
- useQueueHandlers: validate fromIndex/toIndex bounds in reorder
- useCycleSession: use compareNamesIgnoringEmojis for group chat sort
Test improvements:
- AppOverlays: assert shortcuts prop passed to KeyboardMasteryCelebration
- useAutoRunAchievements: add vi.restoreAllMocks() in afterEach
- useLiveMode: assert isLiveMode state after disableAll failure
- useSummarizeHandler: add failure test, strengthen success assertions
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/renderer/hooks/agent/useSummarizeAndContinue.ts (1)
363-374:⚠️ Potential issue | 🟠 MajorAvoid merging stale async session snapshots into live store state.
At Line 366,
{ ...s, ...result.updatedSession }can still clobber concurrent updates made while summarization was running. Build from currentsand apply only deterministic deltas (new tab insertion, active tab switch, system log append).Suggested fix
- setSessions((prev) => - prev.map((s) => { - if (s.id !== sourceSessionId) return s; - const merged = { ...s, ...result.updatedSession }; - return { - ...merged, - aiTabs: merged.aiTabs.map((tab) => - tab.id === targetTabId - ? { ...tab, logs: [...tab.logs, result.systemLogEntry] } - : tab - ), - }; - }) - ); + setSessions((prev) => + prev.map((s) => { + if (s.id !== sourceSessionId) return s; + + const newTab = result.updatedSession.aiTabs.find((tab) => tab.id === result.newTabId); + if (!newTab) return s; + + const sourceIndex = s.aiTabs.findIndex((tab) => tab.id === targetTabId); + const hasNewTab = s.aiTabs.some((tab) => tab.id === newTab.id); + const nextTabs = hasNewTab + ? s.aiTabs + : sourceIndex >= 0 + ? [...s.aiTabs.slice(0, sourceIndex + 1), newTab, ...s.aiTabs.slice(sourceIndex + 1)] + : [...s.aiTabs, newTab]; + + return { + ...s, + activeTabId: result.newTabId, + aiTabs: nextTabs.map((tab) => + tab.id === targetTabId + ? { ...tab, logs: [...tab.logs, result.systemLogEntry] } + : tab + ), + }; + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 363 - 374, The current setSessions updater merges a stale snapshot by doing { ...s, ...result.updatedSession } which can overwrite concurrent changes; instead, inside the setSessions((prev) => prev.map(...)) callback for the session matching sourceSessionId, build the new session from the live session object s and apply only deterministic deltas from result (e.g., insert a new aiTab if missing into s.aiTabs, switch s.activeTabId only if result.updatedSession.activeTabId is present, and append result.systemLogEntry to the matching tab's logs for targetTabId) rather than spreading result.updatedSession over s; use the unique symbols setSessions, sourceSessionId, result.updatedSession, result.systemLogEntry, targetTabId, aiTabs, and activeTabId to locate and implement the changes.
🧹 Nitpick comments (6)
src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts (1)
366-377: Add a positive-path test forisNewRecord: truepropagation.Current coverage only asserts the
falsebranch. Add one test whereupdateAutoRunProgressreturns{ isNewRecord: true }and verifysetStandingOvationDatareceivesisNewRecord: true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts` around lines 366 - 377, Add a symmetric positive-path test to verify true propagation: copy the existing test for isNewRecord:false in the useAutoRunAchievements test file, change mockUpdateAutoRunProgress.mockReturnValue to return { newBadgeLevel: 1, isNewRecord: true }, render the hook via renderHook(() => useAutoRunAchievements({ activeBatchSessionIds: ['session-1'] })), advance timers the same way (vi.advanceTimersByTime(60000)), then assert that mockSetStandingOvationData.mock.calls[0][0].isNewRecord is true; keep the same act wrapping and call inspection as the existing test so it mirrors the false-case check.src/renderer/components/SettingsModal.tsx (1)
2579-2598: Consider removing unnecessary guard.The
setUserMessageAlignment &&check on line 2579 appears to be vestigial from the prop-based approach. SinceuseSettingsalways provides this setter, the conditional wrapper could be removed for cleaner code.♻️ Proposed simplification
- {setUserMessageAlignment && ( - <div> + <div> <div className="block text-xs font-bold opacity-70 uppercase mb-2"> User Message Alignment </div> ... </div> - )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 2579 - 2598, Remove the unnecessary guard around the user alignment UI: delete the `setUserMessageAlignment && (` conditional wrapper so the div block rendering `User Message Alignment` (containing `ToggleButtonGroup` which uses `userMessageAlignment ?? 'right'` and `onChange={setUserMessageAlignment}`) is always rendered; keep the existing `ToggleButtonGroup`, `userMessageAlignment` fallback, and `theme` props unchanged and ensure parentheses/braces are cleaned up so the JSX is valid after removing the conditional.src/__tests__/renderer/hooks/useLiveMode.test.ts (1)
244-269: Add state assertions for failed restart afterstopServersucceeds.On this path, the test only checks
null+ logging. Please also assertisLiveModeandwebInterfaceUrlto lock expected post-failure state and prevent stale-UI regressions.✅ Suggested assertion additions
it('restartWebServer returns null on startServer failure (success: false)', async () => { const { result } = renderHook(() => useLiveMode()); // Turn on await act(async () => { await result.current.toggleGlobalLive(); }); // Make restart fail mockLive.startServer.mockResolvedValue({ success: false, error: 'Restart failed' }); const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); let returnValue: string | null = 'not-null'; await act(async () => { returnValue = await result.current.restartWebServer(); }); expect(returnValue).toBeNull(); + expect(result.current.isLiveMode).toBe(false); + expect(result.current.webInterfaceUrl).toBeNull(); expect(consoleSpy).toHaveBeenCalledWith( '[restartWebServer] Failed to restart server:', 'Restart failed' ); consoleSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useLiveMode.test.ts` around lines 244 - 269, Add assertions after the failed restart path in the test for restartWebServer: after mocking mockLive.startServer to return { success: false } and calling result.current.restartWebServer(), assert the hook state fields result.current.isLiveMode is false (or the expected value when stopServer previously succeeded) and result.current.webInterfaceUrl equals null (or the expected cleared URL). Reference the hook methods/fields restartWebServer, toggleGlobalLive, isLiveMode, and webInterfaceUrl to locate where to add these assertions in the test so the post-failure UI state is locked down and prevents stale-UI regressions.src/renderer/hooks/tabs/useTabExportHandlers.ts (1)
54-57: Extract repeated active-session/tab lookup into a helper.The same lookup/validation block is duplicated in all three handlers. A small local helper would reduce drift and keep guard behavior consistent.
♻️ Refactor sketch
export function useTabExportHandlers(deps: UseTabExportHandlersDeps): UseTabExportHandlersReturn { const { sessionsRef, activeSessionIdRef, themeRef, setGistPublishModalOpen } = deps; + + const getActiveTabWithLogs = useCallback((tabId: string) => { + const currentSession = sessionsRef.current?.find((s) => s.id === activeSessionIdRef.current); + if (!currentSession) return null; + const tab = currentSession.aiTabs.find((t) => t.id === tabId); + if (!tab || !tab.logs || tab.logs.length === 0) return null; + return { currentSession, tab }; + }, []); const handleCopyContext = useCallback((tabId: string) => { - const currentSession = sessionsRef.current?.find((s) => s.id === activeSessionIdRef.current); - if (!currentSession) return; - const tab = currentSession.aiTabs.find((t) => t.id === tabId); - if (!tab || !tab.logs || tab.logs.length === 0) return; + const resolved = getActiveTabWithLogs(tabId); + if (!resolved) return; + const { tab } = resolved; @@ const handleExportHtml = useCallback(async (tabId: string) => { - const currentSession = sessionsRef.current?.find((s) => s.id === activeSessionIdRef.current); - if (!currentSession) return; - const tab = currentSession.aiTabs.find((t) => t.id === tabId); - if (!tab || !tab.logs || tab.logs.length === 0) return; + const resolved = getActiveTabWithLogs(tabId); + if (!resolved) return; + const { currentSession, tab } = resolved; @@ const handlePublishTabGist = useCallback((tabId: string) => { - const currentSession = sessionsRef.current?.find((s) => s.id === activeSessionIdRef.current); - if (!currentSession) return; - const tab = currentSession.aiTabs.find((t) => t.id === tabId); - if (!tab || !tab.logs || tab.logs.length === 0) return; + const resolved = getActiveTabWithLogs(tabId); + if (!resolved) return; + const { tab } = resolved;Also applies to: 80-83, 114-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/tabs/useTabExportHandlers.ts` around lines 54 - 57, Extract the repeated active-session/tab lookup into a single helper (e.g., resolveActiveSessionTab or getActiveSessionAndTab) inside useTabExportHandlers.ts that reads sessionsRef.current and activeSessionIdRef.current, finds the currentSession and the tab by tabId (using currentSession.aiTabs.find), and returns null or a tuple/object when missing; then replace the duplicated blocks in the three handlers (the blocks that reference sessionsRef, activeSessionIdRef, currentSession, and tab/logs checks) with calls to this helper so all handlers use the same validation and retrieval logic.src/renderer/hooks/session/useCycleSession.ts (2)
59-61: Consider movinggetState()calls inside the callback.Calling
getState()at render time (outsideuseCallback) runs on every render. While the action functions are stable references making this functionally correct, callinggetState()inside the callback would be more idiomatic and avoid the extra destructuring each render.♻️ Optional refactor
- // --- Store actions (stable via getState) --- - const { setActiveSessionIdInternal, setCyclePosition } = useSessionStore.getState(); - const { setActiveGroupChatId } = useGroupChatStore.getState(); - // --- Settings --- const ungroupedCollapsed = useSettingsStore((s) => s.ungroupedCollapsed); const cycleSession = useCallback( (dir: 'next' | 'prev') => { + const { setActiveSessionIdInternal, setCyclePosition } = useSessionStore.getState(); + const { setActiveGroupChatId } = useGroupChatStore.getState(); + // Build the visual order of items as they appear in the sidebar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useCycleSession.ts` around lines 59 - 61, Move the useSessionStore.getState() and useGroupChatStore.getState() calls out of render-time destructuring and into the callback so getState() is invoked only when the callback runs; specifically, remove the top-level destructuring of setActiveSessionIdInternal, setCyclePosition, and setActiveGroupChatId and instead call useSessionStore.getState() / useGroupChatStore.getState() inside the relevant useCallback so you obtain those action functions there (e.g., inside the callback that currently relies on setActiveSessionIdInternal, setCyclePosition, setActiveGroupChatId) to avoid calling getState() on every render.
174-175: Minor edge case: empty string IDs would cause inconsistent behavior.If
activeGroupChatIdwere an empty string""(falsy), line 174 would fall through toactiveSessionId, but line 175 would still setcurrentIsGroupChat = truesince it only checks fornull.If IDs in this codebase are always non-empty strings or
null, this is fine. Otherwise, consider explicit null checks.♻️ Optional fix for consistency
- const currentActiveId = activeGroupChatId || activeSessionId; - const currentIsGroupChat = activeGroupChatId !== null; + const currentIsGroupChat = activeGroupChatId !== null && activeGroupChatId !== ''; + const currentActiveId = currentIsGroupChat ? activeGroupChatId : activeSessionId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useCycleSession.ts` around lines 174 - 175, The code uses activeGroupChatId || activeSessionId to choose currentActiveId but checks currentIsGroupChat with activeGroupChatId !== null, causing inconsistent behavior when activeGroupChatId is an empty string; change both selections to treat empty-string IDs as absent—e.g., compute a boolean like const hasGroupId = activeGroupChatId != null && activeGroupChatId !== '' and then set currentActiveId = hasGroupId ? activeGroupChatId : activeSessionId and currentIsGroupChat = hasGroupId; update references to activeGroupChatId, activeSessionId, currentActiveId, and currentIsGroupChat accordingly.
🤖 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/hooks/useAutoRunAchievements.test.ts`:
- Around line 44-47: Replace the untyped fixtures and selector mocks with the
Session type (or Partial<Session> for overrides): change mockSessions from any[]
to Session[] or Partial<Session>[], and update the vi.mock for useSessionStore
to accept a selector typed as (s: { sessions: Session[] }) => any (or
corresponding Partial types); specifically update the symbols mockSessions and
the mocked useSessionStore factory in useAutoRunAchievements.test.ts and apply
the same replacement to the other occurrences you noted (lines around the other
mocks) so tests enforce the Session shape rather than using any.
In `@src/renderer/hooks/remote/useLiveMode.ts`:
- Around line 65-80: restartWebServer can leave isLiveMode true and
webInterfaceUrl stale if stopServer succeeds but startServer fails or throws;
modify restartWebServer so that whenever startServer returns a failing result or
an exception is caught after stopServer has run, you update state to reflect the
stopped server (call setIsLiveMode(false) and clear webInterfaceUrl via
setWebInterfaceUrl(null) or ''), keeping those updates in both the else branch
where result.success is false and inside the catch block; reference
restartWebServer, stopServer, startServer, isLiveMode, setIsLiveMode and
setWebInterfaceUrl when locating where to add these state changes.
In `@src/renderer/hooks/tabs/useTabExportHandlers.ts`:
- Around line 59-61: formatLogsForClipboard can return an empty string and the
code currently proceeds to navigator.clipboard.writeText(text) (and the
gist-publish flow) which silently copies/publishes nothing; before calling
navigator.clipboard.writeText(text) and before publishing a gist, check if text
=== '' (or text.trim() === '') and if so abort the operation, surface an
error/notification to the user (or reject the promise) instead of proceeding;
update the handlers in useTabExportHandlers (the clipboard handler and the
gist/publish handler) to perform this guard and return early with a clear
error/notification when formatted output is empty.
---
Duplicate comments:
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 363-374: The current setSessions updater merges a stale snapshot
by doing { ...s, ...result.updatedSession } which can overwrite concurrent
changes; instead, inside the setSessions((prev) => prev.map(...)) callback for
the session matching sourceSessionId, build the new session from the live
session object s and apply only deterministic deltas from result (e.g., insert a
new aiTab if missing into s.aiTabs, switch s.activeTabId only if
result.updatedSession.activeTabId is present, and append result.systemLogEntry
to the matching tab's logs for targetTabId) rather than spreading
result.updatedSession over s; use the unique symbols setSessions,
sourceSessionId, result.updatedSession, result.systemLogEntry, targetTabId,
aiTabs, and activeTabId to locate and implement the changes.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts`:
- Around line 366-377: Add a symmetric positive-path test to verify true
propagation: copy the existing test for isNewRecord:false in the
useAutoRunAchievements test file, change
mockUpdateAutoRunProgress.mockReturnValue to return { newBadgeLevel: 1,
isNewRecord: true }, render the hook via renderHook(() =>
useAutoRunAchievements({ activeBatchSessionIds: ['session-1'] })), advance
timers the same way (vi.advanceTimersByTime(60000)), then assert that
mockSetStandingOvationData.mock.calls[0][0].isNewRecord is true; keep the same
act wrapping and call inspection as the existing test so it mirrors the
false-case check.
In `@src/__tests__/renderer/hooks/useLiveMode.test.ts`:
- Around line 244-269: Add assertions after the failed restart path in the test
for restartWebServer: after mocking mockLive.startServer to return { success:
false } and calling result.current.restartWebServer(), assert the hook state
fields result.current.isLiveMode is false (or the expected value when stopServer
previously succeeded) and result.current.webInterfaceUrl equals null (or the
expected cleared URL). Reference the hook methods/fields restartWebServer,
toggleGlobalLive, isLiveMode, and webInterfaceUrl to locate where to add these
assertions in the test so the post-failure UI state is locked down and prevents
stale-UI regressions.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 2579-2598: Remove the unnecessary guard around the user alignment
UI: delete the `setUserMessageAlignment && (` conditional wrapper so the div
block rendering `User Message Alignment` (containing `ToggleButtonGroup` which
uses `userMessageAlignment ?? 'right'` and `onChange={setUserMessageAlignment}`)
is always rendered; keep the existing `ToggleButtonGroup`,
`userMessageAlignment` fallback, and `theme` props unchanged and ensure
parentheses/braces are cleaned up so the JSX is valid after removing the
conditional.
In `@src/renderer/hooks/session/useCycleSession.ts`:
- Around line 59-61: Move the useSessionStore.getState() and
useGroupChatStore.getState() calls out of render-time destructuring and into the
callback so getState() is invoked only when the callback runs; specifically,
remove the top-level destructuring of setActiveSessionIdInternal,
setCyclePosition, and setActiveGroupChatId and instead call
useSessionStore.getState() / useGroupChatStore.getState() inside the relevant
useCallback so you obtain those action functions there (e.g., inside the
callback that currently relies on setActiveSessionIdInternal, setCyclePosition,
setActiveGroupChatId) to avoid calling getState() on every render.
- Around line 174-175: The code uses activeGroupChatId || activeSessionId to
choose currentActiveId but checks currentIsGroupChat with activeGroupChatId !==
null, causing inconsistent behavior when activeGroupChatId is an empty string;
change both selections to treat empty-string IDs as absent—e.g., compute a
boolean like const hasGroupId = activeGroupChatId != null && activeGroupChatId
!== '' and then set currentActiveId = hasGroupId ? activeGroupChatId :
activeSessionId and currentIsGroupChat = hasGroupId; update references to
activeGroupChatId, activeSessionId, currentActiveId, and currentIsGroupChat
accordingly.
In `@src/renderer/hooks/tabs/useTabExportHandlers.ts`:
- Around line 54-57: Extract the repeated active-session/tab lookup into a
single helper (e.g., resolveActiveSessionTab or getActiveSessionAndTab) inside
useTabExportHandlers.ts that reads sessionsRef.current and
activeSessionIdRef.current, finds the currentSession and the tab by tabId (using
currentSession.aiTabs.find), and returns null or a tuple/object when missing;
then replace the duplicated blocks in the three handlers (the blocks that
reference sessionsRef, activeSessionIdRef, currentSession, and tab/logs checks)
with calls to this helper so all handlers use the same validation and retrieval
logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/__tests__/renderer/components/AppOverlays.test.tsxsrc/__tests__/renderer/hooks/useAutoRunAchievements.test.tssrc/__tests__/renderer/hooks/useLiveMode.test.tssrc/__tests__/renderer/hooks/useSummarizeHandler.test.tssrc/renderer/components/SettingsModal.tsxsrc/renderer/hooks/agent/useQueueHandlers.tssrc/renderer/hooks/agent/useSummarizeAndContinue.tssrc/renderer/hooks/remote/useLiveMode.tssrc/renderer/hooks/session/useCycleSession.tssrc/renderer/hooks/tabs/useTabExportHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/hooks/agent/useQueueHandlers.ts
- src/tests/renderer/components/AppOverlays.test.tsx
Comment on lines
+44
to
+47
| const mockSessions: any[] = []; | ||
|
|
||
| vi.mock('../../../renderer/stores/sessionStore', () => ({ | ||
| useSessionStore: vi.fn((selector: (s: any) => any) => selector({ sessions: mockSessions })), |
There was a problem hiding this comment.
Replace any-typed session fixtures/mocks with Session-typed data.
Using any here weakens the tests’ ability to catch contract drift in agent/session shape. Please type these fixtures/selectors with Session (or Partial<Session> for overrides) instead of any.
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.
Also applies to: 98-99, 136-143, 597-599, 712-722
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/hooks/useAutoRunAchievements.test.ts` around lines 44
- 47, Replace the untyped fixtures and selector mocks with the Session type (or
Partial<Session> for overrides): change mockSessions from any[] to Session[] or
Partial<Session>[], and update the vi.mock for useSessionStore to accept a
selector typed as (s: { sessions: Session[] }) => any (or corresponding Partial
types); specifically update the symbols mockSessions and the mocked
useSessionStore factory in useAutoRunAchievements.test.ts and apply the same
replacement to the other occurrences you noted (lines around the other mocks) so
tests enforce the Session shape rather than using any.
…pty content guards - Fix useLiveMode restartWebServer leaving stale isLiveMode/webInterfaceUrl when startServer fails after stopServer succeeds - Fix useSummarizeAndContinue stale snapshot merge replacing concurrent session changes; now applies only deterministic deltas (new tab insertion, activeTabId, system log entry) - Fix useTabExportHandlers silently copying/publishing empty content when formatLogsForClipboard returns empty; extract shared resolveSessionAndTab helper - Remove unnecessary setUserMessageAlignment guard in SettingsModal (always truthy) - Add isNewRecord symmetry test in useAutoRunAchievements - Add post-failure state assertions in useLiveMode restart tests
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/renderer/hooks/agent/useSummarizeAndContinue.ts (1)
393-397: Avoid parsing reduction percent from display text.Deriving
%via regex fromsystemLogEntry.textis brittle. Prefer returningreductionPercentexplicitly fromstartSummarizeand using that typed field in the toast message.♻️ Suggested typed payload for reduction percent
startSummarize: (sourceTabId: string) => Promise<{ newTabId: string; updatedSession: Session; systemLogEntry: LogEntry; + reductionPercent: number; } | null>; @@ - ): Promise<{ newTabId: string; updatedSession: Session; systemLogEntry: LogEntry } | null> => { + ): Promise<{ + newTabId: string; + updatedSession: Session; + systemLogEntry: LogEntry; + reductionPercent: number; + } | null> => { @@ return { newTabId: tabResult.tab.id, updatedSession: { ...tabResult.session, activeTabId: tabResult.tab.id, // Switch to the new tab }, systemLogEntry, + reductionPercent: finalResult.reductionPercent, }; @@ - const reductionPercent = result.systemLogEntry.text.match(/(\d+)%/)?.[1] ?? '0'; notifyToast({ type: 'success', title: 'Context Compacted', - message: `Reduced context by ${reductionPercent}%. Click to view the new tab.`, + message: `Reduced context by ${result.reductionPercent}%. Click to view the new tab.`, sessionId: sourceSessionId, tabId: result.newTabId, project: sourceSessionName, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 393 - 397, The code is brittle because it parses reduction percent from systemLogEntry.text; modify the startSummarize implementation to return an explicit typed reductionPercent (e.g., number or string) on the result object, update callers in useSummarizeAndContinue.ts to read result.reductionPercent (falling back to '0' if undefined) instead of regexing systemLogEntry.text, and pass that value into notifyToast's message; reference the startSummarize function and the result variable/structure and replace the regex extraction used around systemLogEntry with the new typed field.src/__tests__/renderer/hooks/useLiveMode.test.ts (1)
90-93: Consider extracting a shared “enable live mode” test helper.The repeated turn-on setup appears in many cases; a small helper would reduce noise and make intent clearer.
Also applies to: 156-158, 204-206, 228-230, 248-250, 278-280, 310-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useLiveMode.test.ts` around lines 90 - 93, Extract a small test helper (e.g., enableLiveMode or enableGlobalLive) to centralize the repeated "Turn on first" setup that calls result.current.toggleGlobalLive inside act; update tests in useLiveMode.test.ts that currently call await act(async () => { await result.current.toggleGlobalLive(); }) (occurrences around toggleGlobalLive at lines indicated) to call the new helper instead, ensuring the helper accepts the renderResult (or result) object and performs the act wrapper and await, keeping existing async behavior and error handling.
🤖 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/hooks/useLiveMode.test.ts`:
- Around line 87-107: The test should assert call order, not just counts: after
toggling off via result.current.toggleGlobalLive(), keep the existing assertions
for isLiveMode and webInterfaceUrl but replace or augment the call-count checks
by asserting the sequence using Vitest’s order matcher — call
result.current.toggleGlobalLive() as you do now and then assert
expect(mockTunnel.stop).toHaveBeenCalledBefore(mockLive.disableAll) (while you
may also keep expect(mockTunnel.stop).toHaveBeenCalledOnce() and
expect(mockLive.disableAll).toHaveBeenCalledOnce()) so the test fails if stop is
not called prior to disableAll.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 1249-1251: The close button in SettingsModal.tsx (the <button
onClick={onClose}> that renders the X icon) is icon-only and lacks an accessible
name; add an explicit accessible label by providing aria-label="Close" (or
aria-label tied to i18n) or include visually-hidden text inside the button
(e.g., a span with sr-only text) so screen readers can announce it; ensure the
label text is descriptive and, if your app uses localization, pull the localized
"Close" string rather than hard-coding.
- Line 2979: The placeholder currently uses an OpenRouter-style string for
Anthropic; update the JSX in the SettingsModal component that builds the
placeholder (where llmProvider is inspected) to return provider-specific
placeholders: keep "llama3:latest" for llmProvider === 'ollama', use
"claude-3.5-sonnet" (no "anthropic/" prefix) for llmProvider === 'anthropic',
and fall back to the existing OpenRouter-style string only for the OpenRouter
branch; adjust the ternary/conditional that references llmProvider to include an
explicit anthropic check so the Anthropic input guidance is valid.
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 359-414: startSummarize can return null for non-error conditions
(e.g. "already summarizing"), so don’t unconditionally show the failure toast;
instead query the operation state and only show the error toast when an actual
error is recorded. Update the null branch after startSummarize in
useSummarizeAndContinue.ts to inspect the operation store (e.g. check
operationStore.getOperation(targetTabId) or operationStore.get(...).status /
.error) and only call notifyToast({type: 'error', ...}) when that operation
indicates an error; otherwise skip the toast (or show a different info toast for
"already summarizing"). Ensure you reference startSummarize, targetTabId,
notifyToast and the operationStore/getOperation API in your changes.
In `@src/renderer/hooks/tabs/useTabExportHandlers.ts`:
- Around line 79-95: The clipboard call lacks a runtime guard and can throw if
navigator.clipboard is undefined; in useTabExportHandlers.ts (the clipboard
write in the export handler that calls navigator.clipboard.writeText and
notifyToast), add a guard like checking navigator.clipboard and
navigator.clipboard.writeText (e.g., if (!navigator.clipboard ||
!navigator.clipboard.writeText) { notifyToast error } else {
navigator.clipboard.writeText(...).then(...).catch(...) }) so you never call
writeText on undefined and still surface a friendly notifyToast error when the
Clipboard API is unavailable.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useLiveMode.test.ts`:
- Around line 90-93: Extract a small test helper (e.g., enableLiveMode or
enableGlobalLive) to centralize the repeated "Turn on first" setup that calls
result.current.toggleGlobalLive inside act; update tests in useLiveMode.test.ts
that currently call await act(async () => { await
result.current.toggleGlobalLive(); }) (occurrences around toggleGlobalLive at
lines indicated) to call the new helper instead, ensuring the helper accepts the
renderResult (or result) object and performs the act wrapper and await, keeping
existing async behavior and error handling.
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 393-397: The code is brittle because it parses reduction percent
from systemLogEntry.text; modify the startSummarize implementation to return an
explicit typed reductionPercent (e.g., number or string) on the result object,
update callers in useSummarizeAndContinue.ts to read result.reductionPercent
(falling back to '0' if undefined) instead of regexing systemLogEntry.text, and
pass that value into notifyToast's message; reference the startSummarize
function and the result variable/structure and replace the regex extraction used
around systemLogEntry with the new typed field.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/renderer/hooks/useAutoRunAchievements.test.tssrc/__tests__/renderer/hooks/useLiveMode.test.tssrc/renderer/components/SettingsModal.tsxsrc/renderer/hooks/agent/useSummarizeAndContinue.tssrc/renderer/hooks/remote/useLiveMode.tssrc/renderer/hooks/tabs/useTabExportHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/hooks/remote/useLiveMode.ts
Comment on lines
+87
to
+107
| it('toggleGlobalLive turns off: calls tunnel.stop then disableAll, clears state', async () => { | ||
| const { result } = renderHook(() => useLiveMode()); | ||
|
|
||
| // Turn on first | ||
| await act(async () => { | ||
| await result.current.toggleGlobalLive(); | ||
| }); | ||
|
|
||
| expect(result.current.isLiveMode).toBe(true); | ||
| expect(result.current.webInterfaceUrl).toBe('http://localhost:3000'); | ||
|
|
||
| // Now turn off | ||
| await act(async () => { | ||
| await result.current.toggleGlobalLive(); | ||
| }); | ||
|
|
||
| expect(mockTunnel.stop).toHaveBeenCalledOnce(); | ||
| expect(mockLive.disableAll).toHaveBeenCalledOnce(); | ||
| expect(result.current.isLiveMode).toBe(false); | ||
| expect(result.current.webInterfaceUrl).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/__tests__/renderer/hooks && head -n 120 useLiveMode.test.ts | tail -n 50Repository: RunMaestro/Maestro
Length of output: 1818
🌐 Web query:
Vitest mock invocationCallOrder verification methods
💡 Result:
Vitest gives you two main ways to verify mock/spies call order:
1) Use built-in matchers (Vitest 3.0.0+)
expect(mockA).toHaveBeenCalledBefore(mockB[, failIfNoFirstInvocation])expect(mockA).toHaveBeenCalledAfter(mockB[, failIfNoFirstInvocation])[1]
import { expect, vi, test } from 'vitest'
test('order', () => {
const a = vi.fn()
const b = vi.fn()
a()
b()
expect(a).toHaveBeenCalledBefore(b)
expect(b).toHaveBeenCalledAfter(a)
})2) Assert using mock.invocationCallOrder (low-level)
Each mock has mock.invocationCallOrder: number[], and the numbers come from a single shared increasing sequence across all mocks—so you can compare them directly. [2]
import { expect, vi, test } from 'vitest'
test('order via invocationCallOrder', () => {
const a = vi.fn()
const b = vi.fn()
a() // order 1
b() // order 2
a() // order 3
expect(a.mock.invocationCallOrder).toEqual([1, 3])
expect(b.mock.invocationCallOrder).toEqual([2])
// “a happened before b at least once”:
expect(Math.min(...a.mock.invocationCallOrder))
.toBeLessThan(Math.min(...b.mock.invocationCallOrder))
})Sources: [1] Vitest expect API (toHaveBeenCalledBefore/After), [2] Vitest mock.invocationCallOrder docs.
Assert the stop→disable order explicitly.
This test name promises sequencing ("calls tunnel.stop then disableAll"), but current assertions only check call counts. Use Vitest's built-in order matcher to ensure it fails if execution order regresses.
Suggested test hardening
expect(mockTunnel.stop).toHaveBeenCalledOnce();
expect(mockLive.disableAll).toHaveBeenCalledOnce();
+ expect(mockTunnel.stop).toHaveBeenCalledBefore(mockLive.disableAll);
expect(result.current.isLiveMode).toBe(false);
expect(result.current.webInterfaceUrl).toBeNull();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('toggleGlobalLive turns off: calls tunnel.stop then disableAll, clears state', async () => { | |
| const { result } = renderHook(() => useLiveMode()); | |
| // Turn on first | |
| await act(async () => { | |
| await result.current.toggleGlobalLive(); | |
| }); | |
| expect(result.current.isLiveMode).toBe(true); | |
| expect(result.current.webInterfaceUrl).toBe('http://localhost:3000'); | |
| // Now turn off | |
| await act(async () => { | |
| await result.current.toggleGlobalLive(); | |
| }); | |
| expect(mockTunnel.stop).toHaveBeenCalledOnce(); | |
| expect(mockLive.disableAll).toHaveBeenCalledOnce(); | |
| expect(result.current.isLiveMode).toBe(false); | |
| expect(result.current.webInterfaceUrl).toBeNull(); | |
| }); | |
| it('toggleGlobalLive turns off: calls tunnel.stop then disableAll, clears state', async () => { | |
| const { result } = renderHook(() => useLiveMode()); | |
| // Turn on first | |
| await act(async () => { | |
| await result.current.toggleGlobalLive(); | |
| }); | |
| expect(result.current.isLiveMode).toBe(true); | |
| expect(result.current.webInterfaceUrl).toBe('http://localhost:3000'); | |
| // Now turn off | |
| await act(async () => { | |
| await result.current.toggleGlobalLive(); | |
| }); | |
| expect(mockTunnel.stop).toHaveBeenCalledOnce(); | |
| expect(mockLive.disableAll).toHaveBeenCalledOnce(); | |
| expect(mockTunnel.stop.mock.invocationCallOrder[0]).toBeLessThan(mockLive.disableAll.mock.invocationCallOrder[0]); | |
| expect(result.current.isLiveMode).toBe(false); | |
| expect(result.current.webInterfaceUrl).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/hooks/useLiveMode.test.ts` around lines 87 - 107, The
test should assert call order, not just counts: after toggling off via
result.current.toggleGlobalLive(), keep the existing assertions for isLiveMode
and webInterfaceUrl but replace or augment the call-count checks by asserting
the sequence using Vitest’s order matcher — call
result.current.toggleGlobalLive() as you do now and then assert
expect(mockTunnel.stop).toHaveBeenCalledBefore(mockLive.disableAll) (while you
may also keep expect(mockTunnel.stop).toHaveBeenCalledOnce() and
expect(mockLive.disableAll).toHaveBeenCalledOnce()) so the test fails if stop is
not called prior to disableAll.
Comment on lines
+1249
to
1251
| <button onClick={onClose} className="cursor-pointer"> | ||
| <X className="w-5 h-5 opacity-50 hover:opacity-100" /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the icon-only close button.
Line 1249 uses an icon-only <button> without an explicit accessible label, which can be ambiguous for assistive tech.
Suggested fix
- <button onClick={onClose} className="cursor-pointer">
+ <button onClick={onClose} className="cursor-pointer" aria-label="Close settings" title="Close settings">
<X className="w-5 h-5 opacity-50 hover:opacity-100" />
</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={onClose} className="cursor-pointer"> | |
| <X className="w-5 h-5 opacity-50 hover:opacity-100" /> | |
| </button> | |
| <button onClick={onClose} className="cursor-pointer" aria-label="Close settings" title="Close settings"> | |
| <X className="w-5 h-5 opacity-50 hover:opacity-100" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` around lines 1249 - 1251, The
close button in SettingsModal.tsx (the <button onClick={onClose}> that renders
the X icon) is icon-only and lacks an accessible name; add an explicit
accessible label by providing aria-label="Close" (or aria-label tied to i18n) or
include visually-hidden text inside the button (e.g., a span with sr-only text)
so screen readers can announce it; ensure the label text is descriptive and, if
your app uses localization, pull the localized "Close" string rather than
hard-coding.
| style={{ borderColor: theme.colors.border }} | ||
| placeholder={ | ||
| props.llmProvider === 'ollama' ? 'llama3:latest' : 'anthropic/claude-3.5-sonnet' | ||
| llmProvider === 'ollama' ? 'llama3:latest' : 'anthropic/claude-3.5-sonnet' |
There was a problem hiding this comment.
Use provider-specific model placeholder text.
Line 2979 currently shows an OpenRouter-style placeholder for Anthropic as well, which can lead to invalid input guidance.
Suggested fix
- placeholder={
- llmProvider === 'ollama' ? 'llama3:latest' : 'anthropic/claude-3.5-sonnet'
- }
+ placeholder={
+ llmProvider === 'ollama'
+ ? 'llama3:latest'
+ : llmProvider === 'anthropic'
+ ? 'claude-3-5-sonnet-20241022'
+ : 'anthropic/claude-3.5-sonnet'
+ }📝 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
| llmProvider === 'ollama' ? 'llama3:latest' : 'anthropic/claude-3.5-sonnet' | |
| placeholder={ | |
| llmProvider === 'ollama' | |
| ? 'llama3:latest' | |
| : llmProvider === 'anthropic' | |
| ? 'claude-3-5-sonnet-20241022' | |
| : 'anthropic/claude-3.5-sonnet' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` at line 2979, The placeholder
currently uses an OpenRouter-style string for Anthropic; update the JSX in the
SettingsModal component that builds the placeholder (where llmProvider is
inspected) to return provider-specific placeholders: keep "llama3:latest" for
llmProvider === 'ollama', use "claude-3.5-sonnet" (no "anthropic/" prefix) for
llmProvider === 'anthropic', and fall back to the existing OpenRouter-style
string only for the OpenRouter branch; adjust the ternary/conditional that
references llmProvider to include an explicit anthropic check so the Anthropic
input guidance is valid.
Comment on lines
+359
to
+414
| startSummarize(targetTabId) | ||
| .then((result) => { | ||
| if (result) { | ||
| // Apply only deterministic deltas to the live session (avoid stale snapshot spread) | ||
| setSessions((prev) => | ||
| prev.map((s) => { | ||
| if (s.id !== sourceSessionId) return s; | ||
| // Insert the new tab if not already present | ||
| const newTab = result.updatedSession.aiTabs.find((t) => t.id === result.newTabId); | ||
| const hasNewTab = s.aiTabs.some((t) => t.id === result.newTabId); | ||
| // Find insertion point: right after the source tab | ||
| let updatedTabs = s.aiTabs; | ||
| if (newTab && !hasNewTab) { | ||
| const sourceIdx = s.aiTabs.findIndex((t) => t.id === targetTabId); | ||
| const insertIdx = sourceIdx >= 0 ? sourceIdx + 1 : s.aiTabs.length; | ||
| updatedTabs = [ | ||
| ...s.aiTabs.slice(0, insertIdx), | ||
| newTab, | ||
| ...s.aiTabs.slice(insertIdx), | ||
| ]; | ||
| } | ||
| return { | ||
| ...s, | ||
| activeTabId: result.newTabId, | ||
| aiTabs: updatedTabs.map((tab) => | ||
| tab.id === targetTabId | ||
| ? { ...tab, logs: [...tab.logs, result.systemLogEntry] } | ||
| : tab | ||
| ), | ||
| }; | ||
| }) | ||
| ); | ||
|
|
||
| // Show success notification with click-to-navigate | ||
| const reductionPercent = result.systemLogEntry.text.match(/(\d+)%/)?.[1] ?? '0'; | ||
| notifyToast({ | ||
| type: 'success', | ||
| title: 'Context Compacted', | ||
| message: `Reduced context by ${reductionPercent}%. Click to view the new tab.`, | ||
| sessionId: sourceSessionId, | ||
| tabId: result.newTabId, | ||
| project: sourceSessionName, | ||
| }); | ||
|
|
||
| // Clear the summarization state for this tab | ||
| clearTabState(targetTabId); | ||
| } else { | ||
| // startSummarize returned null (error already set in operationStore) | ||
| notifyToast({ | ||
| type: 'error', | ||
| title: 'Compaction Failed', | ||
| message: 'Failed to compact context. Check the tab for details.', | ||
| sessionId: sourceSessionId, | ||
| tabId: targetTabId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Differentiate startSummarize null outcomes before showing failure toast.
This branch treats every null as a failure, but startSummarize can also return null for non-error paths (notably “already summarizing”). That can produce false “Compaction Failed” toasts.
💡 Suggested guard to avoid false failure toasts
const targetTabId = tabId || session.activeTabId;
const targetTab = session.aiTabs.find((t) => t.id === targetTabId);
+ const existingState = useOperationStore.getState().summarizeStates.get(targetTabId);
+
+ if (existingState?.state === 'summarizing') {
+ notifyToast({
+ type: 'info',
+ title: 'Compaction in Progress',
+ message: 'This tab is already being compacted.',
+ sessionId: session.id,
+ tabId: targetTabId,
+ });
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 359 - 414,
startSummarize can return null for non-error conditions (e.g. "already
summarizing"), so don’t unconditionally show the failure toast; instead query
the operation state and only show the error toast when an actual error is
recorded. Update the null branch after startSummarize in
useSummarizeAndContinue.ts to inspect the operation store (e.g. check
operationStore.getOperation(targetTabId) or operationStore.get(...).status /
.error) and only call notifyToast({type: 'error', ...}) when that operation
indicates an error; otherwise skip the toast (or show a different info toast for
"already summarizing"). Ensure you reference startSummarize, targetTabId,
notifyToast and the operationStore/getOperation API in your changes.
Comment on lines
+79
to
+95
| navigator.clipboard | ||
| .writeText(text) | ||
| .then(() => { | ||
| notifyToast({ | ||
| type: 'success', | ||
| title: 'Context Copied', | ||
| message: 'Conversation copied to clipboard.', | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to copy context:', err); | ||
| notifyToast({ | ||
| type: 'error', | ||
| title: 'Copy Failed', | ||
| message: 'Failed to copy context to clipboard.', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
navigator.clipboard Electron renderer process availability requirements
💡 Result:
In an Electron renderer, navigator.clipboard (Async Clipboard API) is available/usable only when Chromium’s secure-context + permission requirements are met:
-
Must be a secure context (otherwise
navigator.clipboardmay be missing/blocked). The Clipboard API is restricted to secure contexts. [1]- In Electron, if you’re loading
file://or a custom scheme, make it a secure/standard scheme (e.g.,app://) viaprotocol.registerSchemesAsPrivileged(... privileges: { standard: true, secure: true }). [2]
- In Electron, if you’re loading
-
Permissions may be required (especially for reading) and are controlled by Electron’s session permission handlers:
- Electron’s permission system includes
clipboard-readandclipboard-sanitized-write. [3] - Handle them with
ses.setPermissionCheckHandler(...)/ses.setPermissionRequestHandler(...)to allow/deny. [3]
- Electron’s permission system includes
If you don’t want to deal with these web-API requirements, use Electron’s built-in clipboard module instead (works in Main and Renderer (non-sandboxed); for contextIsolation, call it in preload and expose via contextBridge). [4]
Sources:
[1] MDN Clipboard API (secure context requirement) (developer.mozilla.org)
[2] Electron protocol.registerSchemesAsPrivileged (secure: true) (electronjs.org)
[3] Electron session permission names include clipboard-read / clipboard-sanitized-write (electronjs.org)
[4] Electron clipboard module availability notes (electronjs.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 2: https://www.electronjs.org/docs/latest/api/protocol?utm_source=openai
- 3: https://www.electronjs.org/docs/latest/api/session?utm_source=openai
- 4: https://www.electronjs.org/docs/latest/api/clipboard?utm_source=openai
🌐 Web query:
Clipboard API navigator.clipboard undefined throw error Chromium secure context
💡 Result:
In Chromium-based browsers, navigator.clipboard is intentionally undefined when the page is not in a secure context (and in a few other restricted contexts). The “secure context” rule is the most common cause. (developer.mozilla.org)
Why it happens (most common causes)
-
Not a secure context
- The Async Clipboard API is exposed only on secure origins (typically
https://, and also treated-secure cases likehttp://localhost). If you’re onhttp://(non-localhost) orfile://, Chromium may hide the API, sonavigator.clipboardbecomesundefined. (developer.mozilla.org)
- The Async Clipboard API is exposed only on secure origins (typically
-
Running inside an iframe without permission
- In an
<iframe>, clipboard access can be blocked unless the frame is granted permission via Permissions Policy, e.g.allow="clipboard-read; clipboard-write". (codestudy.net)
- In an
-
Enterprise / extension restrictions
- Some managed Chromium setups or security/privacy extensions can restrict clipboard features (often you’ll see permission errors, but restrictions can vary). (codestudy.net)
Quick checks
console.log("isSecureContext =", window.isSecureContext);
console.log("navigator.clipboard =", navigator.clipboard);If window.isSecureContext is false, expect navigator.clipboard to be missing. (stackoverflow.com)
Fixes
- Serve the page over HTTPS (recommended for anything not on localhost). (developer.mozilla.org)
- For local dev, use
http://localhost(treated as secure by browsers) rather than an IP/remote HTTP host. (codestudy.net) - If in an iframe, add:
(codestudy.net)
<iframe src="..." allow="clipboard-read; clipboard-write"></iframe>
If you paste the exact URL you’re running on (scheme + host) and whether it’s in an iframe/extension, I can pinpoint which case you’re hitting.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 3: https://www.codestudy.net/blog/navigator-clipboard-is-undefined/?utm_source=openai
- 4: https://www.codestudy.net/blog/navigator-clipboard-is-undefined/?utm_source=openai
- 5: https://stackoverflow.com/questions/71873824/copy-text-to-clipboard-cannot-read-properties-of-undefined-reading-writetext?utm_source=openai
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 7: https://www.codestudy.net/blog/navigator-clipboard-is-undefined/?utm_source=openai
- 8: https://www.codestudy.net/blog/navigator-clipboard-is-undefined/?utm_source=openai
🏁 Script executed:
# First, let's verify this is an Electron app and check the context
git ls-files | head -20Repository: RunMaestro/Maestro
Length of output: 428
🏁 Script executed:
# Check for Electron configuration files
fd -e json | grep -E "(electron|package)" | head -10Repository: RunMaestro/Maestro
Length of output: 92
🏁 Script executed:
# Check if this is an Electron app
head -100 package.jsonRepository: RunMaestro/Maestro
Length of output: 4148
🏁 Script executed:
# Search for imports of 'electron' clipboard module to see if native clipboard is already used
rg "from\s+['\"]electron['\"]|require\(['\"]electron" -A 2 | head -40Repository: RunMaestro/Maestro
Length of output: 1527
🏁 Script executed:
# Search for other uses of navigator.clipboard in the codebase
rg "navigator\.clipboard" --type ts --type tsxRepository: RunMaestro/Maestro
Length of output: 89
🏁 Script executed:
# Search for uses of navigator.clipboard (corrected rg command)
rg "navigator\.clipboard" -t tsRepository: RunMaestro/Maestro
Length of output: 6299
🏁 Script executed:
# Also search in tsx files with a broader pattern
fd -e ts -e tsx | xargs grep -l "navigator\.clipboard"Repository: RunMaestro/Maestro
Length of output: 1982
🏁 Script executed:
# Check if electron clipboard module is being used anywhere
rg "from\s+['\"]electron['\"]" | grep clipboardRepository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check the preload scripts to see if clipboard is exposed via contextBridge
fd preload | head -10Repository: RunMaestro/Maestro
Length of output: 133
🏁 Script executed:
# Check the clipboard utility that's already in the codebase
cat -n src/renderer/utils/clipboard.tsRepository: RunMaestro/Maestro
Length of output: 1034
🏁 Script executed:
# Check how SessionStatusBanner handles the clipboard guard (it has one)
sed -n '1,100p' src/web/mobile/SessionStatusBanner.tsx | grep -A 5 -B 5 "navigator.clipboard"Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check if there's a main/preload setup that exposes clipboard safely
ls -la src/main/preload/Repository: RunMaestro/Maestro
Length of output: 1735
🏁 Script executed:
# Check vite config to see secure context setup
fd vite.config | xargs catRepository: RunMaestro/Maestro
Length of output: 9444
Add a Clipboard API availability guard before writeText.
navigator.clipboard can be undefined in certain Electron configurations (non-secure contexts). Without the guard, attempting to call .writeText() on undefined will throw synchronously and bypass your .catch() handler. This pattern is already used elsewhere in the codebase (e.g., SessionStatusBanner.tsx).
Suggested fix
+ if (!navigator.clipboard?.writeText) {
+ notifyToast({
+ type: 'error',
+ title: 'Copy Failed',
+ message: 'Clipboard is not available in this environment.',
+ });
+ return;
+ }
+
navigator.clipboard
.writeText(text)
.then(() => {📝 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
| navigator.clipboard | |
| .writeText(text) | |
| .then(() => { | |
| notifyToast({ | |
| type: 'success', | |
| title: 'Context Copied', | |
| message: 'Conversation copied to clipboard.', | |
| }); | |
| }) | |
| .catch((err) => { | |
| console.error('Failed to copy context:', err); | |
| notifyToast({ | |
| type: 'error', | |
| title: 'Copy Failed', | |
| message: 'Failed to copy context to clipboard.', | |
| }); | |
| }); | |
| if (!navigator.clipboard?.writeText) { | |
| notifyToast({ | |
| type: 'error', | |
| title: 'Copy Failed', | |
| message: 'Clipboard is not available in this environment.', | |
| }); | |
| return; | |
| } | |
| navigator.clipboard | |
| .writeText(text) | |
| .then(() => { | |
| notifyToast({ | |
| type: 'success', | |
| title: 'Context Copied', | |
| message: 'Conversation copied to clipboard.', | |
| }); | |
| }) | |
| .catch((err) => { | |
| console.error('Failed to copy context:', err); | |
| notifyToast({ | |
| type: 'error', | |
| title: 'Copy Failed', | |
| message: 'Failed to copy context to clipboard.', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/tabs/useTabExportHandlers.ts` around lines 79 - 95, The
clipboard call lacks a runtime guard and can throw if navigator.clipboard is
undefined; in useTabExportHandlers.ts (the clipboard write in the export handler
that calls navigator.clipboard.writeText and notifyToast), add a guard like
checking navigator.clipboard and navigator.clipboard.writeText (e.g., if
(!navigator.clipboard || !navigator.clipboard.writeText) { notifyToast error }
else { navigator.clipboard.writeText(...).then(...).catch(...) }) so you never
call writeText on undefined and still surface a friendly notifyToast error when
the Clipboard API is unavailable.
This was referenced Feb 28, 2026
This was referenced Mar 1, 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.
Summary
Continues the App.tsx refactoring with three tiers of extraction, reducing the file from 4,602 to 3,208 lines (−1,394 lines, −30%).
10 New Hook Extractions (
a9098967)useSymphonyContribution,useAutoRunDocumentLoader,useQueueProcessing,usePromptComposerHandlers,useQuickActionsHandlers,useCycleSession,useAutoRunAchievements,useTabExportHandlers,useQueueHandlers,useTourActionsInline Function Extractions (
14343676)useInputMode(AI/terminal toggle),useLiveMode(web interface toggle)useModalHandlers,useWizardHandlers,useSummarizeAndContinueComponent Self-Sourcing (
9d8067e7)AppOverlays→ reads frommodalStore+settingsStoreAppModals→ reads fromsessionStore+groupChatStore+modalStore(29 booleans viauseShallow)SettingsModal→ reads ~60 settings fromuseSettings(), interface reduced from ~68 to 8 propsBug Fix (
ca5b5aa3)cursor-pointerto all SettingsModal tab buttons (Tailwind Preflight resets buttons tocursor: default)Test plan
npx tsc --noEmit— 0 errorsnpm run test— 21,462 tests pass, 0 failures, 504 test filesSummary by CodeRabbit
Tests
New Features
Refactor