refactor: extract 4 domain hooks from App.tsx (−3,645 lines, 223 tests)#404
refactor: extract 4 domain hooks from App.tsx (−3,645 lines, 223 tests)#404
Conversation
…se 2A) Move all tab management callbacks and derived state into a dedicated hook that reads directly from Zustand stores. App.tsx reduced from ~12,863 to ~11,553 lines. Also consolidate duplicate MaestroSettings type definitions in persistence.ts to import from canonical stores/types.ts. - 54 tests covering AI/file tab ops, navigation, close, properties, scroll - Full suite passes (19,724 tests)
Phase 2B: Extract useGroupChatHandlers hook from App.tsx (~448 lines removed). The hook encapsulates all group chat lifecycle handlers, IPC event listeners, execution queue processor, and error recovery. Reads from groupChatStore, modalStore, sessionStore, and uiStore. 681 lines, 68 tests. Phase 2A tests: Expand useTabHandlers test coverage from 54 to 74 tests. Added tests for file tab reload/auto-refresh, wizard tab close confirmation, show thinking toggle, delete log edge cases, close tabs with file tabs, file tab adjacent insertion, and new agent session settings defaults. App.tsx: 11,528 → 11,080 lines. Full test suite: 19,812 tests, 0 failures.
…se 2C) Extract 50+ modal lifecycle handlers into useModalHandlers hook: - Simple close handlers (10), session-related close handlers (5) - Quit confirmation, celebration, leaderboard handlers - Agent error recovery handlers with useAgentErrorRecovery composition - Lightbox open/navigate/delete/close handlers - Quick actions modal openers (7) - LogViewer focus-restore and shortcuts search reset effects - Migrate deleteAgent state from local useState to modalStore - 81 new tests, 19,893 total (0 failures)
setDeleteAgentSession(session) already calls openModal('deleteAgent', { session }).
The subsequent setDeleteAgentModalOpen(true) called openModal('deleteAgent') without
data, overwriting the session with undefined.
…, Phase 2D) Extract all worktree-related handlers, effects, refs, and memoized values into useWorktreeHandlers hook. Deduplicate Session construction (repeated 5x) into buildWorktreeSession helper. Includes 40 tests covering all 14 handlers and effects.
…alOutputRef fallback Move standing ovation startup/return-to-app checks, keyboard mastery startup check, and handleLogViewerShortcutUsed from App.tsx into useModalHandlers. Add terminalOutputRef param for LogViewer close focus fallback (from main's auto-scroll PR). 11 new tests (92 total).
📝 WalkthroughWalkthroughAdds four new renderer hooks (groupChat, modal, tabs, worktree) with full implementations and extensive tests; updates hooks barrel exports; re-exports/shared-type adjustments in persistence/types; and extends modal store with delete-agent modal support. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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 |
Code Review — PR #404: Extract 4 domain hooks from App.tsxThis is a well-structured, high-value refactor. The extraction goals are clear, the test coverage is extensive (223 new tests), and the `getState()` pattern for stable `[]` dependency arrays is the correct approach for this Zustand architecture. The overall direction is right. Below are specific issues worth addressing before merge. Bugs / Correctness1. Consecutive In // handleCreateWorktreeFromConfig (lines ~433–440) and handleCreateWorktree (lines ~513–532)
useSessionStore.getState().setSessions((prev) => [...prev, worktreeSession]);
// Expand parent's worktrees
useSessionStore
.getState()
.setSessions((prev) =>
prev.map((s) => (s.id === activeSession.id ? { ...s, worktreesExpanded: true } : s))
);
// handleCreateWorktree also has a third for worktreeConfig (lines ~524–532)
useSessionStore.getState().setSessions((prev) => prev.map(...));Each useSessionStore.getState().setSessions((prev) => [
...prev.map((s) =>
s.id === parentId
? { ...s, worktreesExpanded: true, worktreeConfig: config }
: s
),
worktreeSession,
]);2. setGroupChatState((_prev) => {
const savedState = groupChatStates.get(id);
return savedState ?? 'idle';
});If Code Quality3. Debug
console.log(`[GroupChat:UI] Updated allGroupChatParticipantStates for ${id}: ...`);
console.log(`[GroupChat:UI] Updating participantStates for active chat: ...`);
// etc.
4. Unused computed value:
5.
6. const { recoveryActions: groupChatRecoveryActions } = useAgentErrorRecovery({
agentId: 'claude-code', // Group chat moderator is always claude-code for nowThe comment acknowledges this is a known limitation. If the group chat system already supports arbitrary moderator agent IDs (as evidenced by Type Safety7. ipcMain.handle('sessions:setAll', async (_, sessions: any[]) => { ... }
ipcMain.handle('groups:setAll', async (_, groups: any[]) => { ... }
console.log(`... store path: ${(sessionsStore as any).path}`);The Performance8. const sessions = useSessionStore((s) => s.sessions);This subscribes the hook to every sessions array change. With large session counts, every session state update (typing, agent output, etc.) re-runs derived computations in this hook. The Testing9. Test for The 10. Worktree consecutive Tests for Minor
Summary
The reduction of App.tsx by ~3,600 lines is a meaningful win and the 223 tests give confidence in behavioral parity. The issues above are mostly cleanup-level — the intermediate |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/groupChat/useGroupChatHandlers.ts`:
- Around line 281-294: The handler handleProcessMonitorNavigateToGroupChat
currently only sets IDs/states/participants and leaves stale groupChatMessages;
update it to reuse the existing open flow by invoking
handleOpenGroupChat(groupChatId) (or explicitly call the store setter that
resets/loads messages, e.g., setGroupChatMessages) before closing the modal so
the target chat's messages are cleared/loaded; ensure you still
setActiveGroupChatId, setGroupChatState and setParticipantStates as needed and
then call closeModal('processMonitor').
In `@src/renderer/hooks/modal/useModalHandlers.ts`:
- Around line 452-461: The modal flags are being set before the modal is opened,
so updateModalData no-ops and isGroupChat/allowDelete remain default; in
handleSetLightboxImage (and using getModalActions and
useGroupChatStore.getState()), call actions.setLightboxImage(image) and
actions.setLightboxSource(source) first to open the lightbox, then call
actions.setLightboxIsGroupChat(activeGroupChatId !== null),
actions.setLightboxAllowDelete(source === 'staged'), and finally
actions.setLightboxImages(contextImages || []), ensuring the open happens before
flag updates.
In `@src/renderer/hooks/tabs/useTabHandlers.ts`:
- Around line 521-553: The handler handleReloadFileTab currently reads the
active session at completion time which can apply updates to the wrong session
if the user switches tabs; capture the target session id at the start (e.g.,
const targetSessionId = useSessionStore.getState().activeSessionId) and then use
that captured value inside the Promise resolution and setSessions mapper instead
of re-reading useSessionStore.getState().activeSessionId; ensure the same change
is applied to the other handler referenced (lines ~578-599) and update the
setSessions mapping for filePreviewTabs by matching s.id === targetSessionId and
tab.id === tabId so updates always apply to the intended session/tab.
- Around line 1243-1273: The file-preview navigation handlers
(back/forward/index in useTabHandlers: code paths that read historyEntry and
call setSessions to update filePreviewTabs) update path/name/content but forget
to recompute the file extension, leaving extension stale and desyncing
icons/syntax; when constructing the new tab object inside setSessions for the
matching tab id (in the block that currently assigns path: historyEntry.path,
name: historyEntry.name, content, scrollTop, navigationIndex), compute the
extension from historyEntry.path (e.g., derive the substring after the last dot
or use the existing utility for extension extraction) and set extension:
computedExtension on the returned tab; apply the same change to the
corresponding handlers at the other locations mentioned (around the blocks for
lines noted in the review).
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 547-567: When deleting a worktree session without removing files,
call useSessionStore.getState().addRemovedWorktreePath(deleteWtSession.cwd)
inside handleConfirmDeleteWorktree to mark the path as intentionally removed;
also add the same check in discovery flows so removed paths are skipped: update
scanWorktreeConfigsOnStartup and the worktree-config save/handler (the function
that processes discovered worktree configs) to consult
useSessionStore.getState().removedWorktreePaths (e.g., check
removedWorktreePaths.has(path) and skip adding/re-discovering those paths) so
removed worktrees do not reappear after restart or config save.
| const handleProcessMonitorNavigateToGroupChat = useCallback((groupChatId: string) => { | ||
| const { | ||
| setActiveGroupChatId, | ||
| setGroupChatState, | ||
| setParticipantStates, | ||
| groupChatStates, | ||
| allGroupChatParticipantStates, | ||
| } = useGroupChatStore.getState(); | ||
| const { closeModal } = useModalStore.getState(); | ||
| setActiveGroupChatId(groupChatId); | ||
| setGroupChatState(groupChatStates.get(groupChatId) ?? 'idle'); | ||
| setParticipantStates(allGroupChatParticipantStates.get(groupChatId) ?? new Map()); | ||
| closeModal('processMonitor'); | ||
| }, []); |
There was a problem hiding this comment.
Process monitor navigation doesn’t load/clear messages for the target chat.
This path only swaps IDs/state/participants, so groupChatMessages can remain from the previous chat until new messages arrive. Consider reusing handleOpenGroupChat (or explicitly clearing/reloading messages) before closing the modal.
Suggested fix (reuse existing open flow)
-const handleProcessMonitorNavigateToGroupChat = useCallback((groupChatId: string) => {
- const {
- setActiveGroupChatId,
- setGroupChatState,
- setParticipantStates,
- groupChatStates,
- allGroupChatParticipantStates,
- } = useGroupChatStore.getState();
- const { closeModal } = useModalStore.getState();
- setActiveGroupChatId(groupChatId);
- setGroupChatState(groupChatStates.get(groupChatId) ?? 'idle');
- setParticipantStates(allGroupChatParticipantStates.get(groupChatId) ?? new Map());
- closeModal('processMonitor');
-}, []);
+const handleProcessMonitorNavigateToGroupChat = useCallback((groupChatId: string) => {
+ void handleOpenGroupChat(groupChatId);
+ useModalStore.getState().closeModal('processMonitor');
+}, [handleOpenGroupChat]);📝 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.
| const handleProcessMonitorNavigateToGroupChat = useCallback((groupChatId: string) => { | |
| const { | |
| setActiveGroupChatId, | |
| setGroupChatState, | |
| setParticipantStates, | |
| groupChatStates, | |
| allGroupChatParticipantStates, | |
| } = useGroupChatStore.getState(); | |
| const { closeModal } = useModalStore.getState(); | |
| setActiveGroupChatId(groupChatId); | |
| setGroupChatState(groupChatStates.get(groupChatId) ?? 'idle'); | |
| setParticipantStates(allGroupChatParticipantStates.get(groupChatId) ?? new Map()); | |
| closeModal('processMonitor'); | |
| }, []); | |
| const handleProcessMonitorNavigateToGroupChat = useCallback((groupChatId: string) => { | |
| void handleOpenGroupChat(groupChatId); | |
| useModalStore.getState().closeModal('processMonitor'); | |
| }, [handleOpenGroupChat]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 281 - 294,
The handler handleProcessMonitorNavigateToGroupChat currently only sets
IDs/states/participants and leaves stale groupChatMessages; update it to reuse
the existing open flow by invoking handleOpenGroupChat(groupChatId) (or
explicitly call the store setter that resets/loads messages, e.g.,
setGroupChatMessages) before closing the modal so the target chat's messages are
cleared/loaded; ensure you still setActiveGroupChatId, setGroupChatState and
setParticipantStates as needed and then call closeModal('processMonitor').
| const handleReloadFileTab = useCallback(async (tabId: string) => { | ||
| const { sessions, activeSessionId } = useSessionStore.getState(); | ||
| const currentSession = sessions.find((s) => s.id === activeSessionId); | ||
| if (!currentSession) return; | ||
|
|
||
| const fileTab = currentSession.filePreviewTabs.find((tab) => tab.id === tabId); | ||
| if (!fileTab) return; | ||
|
|
||
| try { | ||
| const [content, stat] = await Promise.all([ | ||
| window.maestro.fs.readFile(fileTab.path, fileTab.sshRemoteId), | ||
| window.maestro.fs.stat(fileTab.path, fileTab.sshRemoteId), | ||
| ]); | ||
| const newMtime = stat?.modifiedAt ? new Date(stat.modifiedAt).getTime() : Date.now(); | ||
|
|
||
| useSessionStore.getState().setSessions((prev: Session[]) => | ||
| prev.map((s) => { | ||
| if (s.id !== useSessionStore.getState().activeSessionId) return s; | ||
| return { | ||
| ...s, | ||
| filePreviewTabs: s.filePreviewTabs.map((tab) => | ||
| tab.id === tabId | ||
| ? { | ||
| ...tab, | ||
| content, | ||
| lastModified: newMtime, | ||
| editContent: undefined, | ||
| } | ||
| : tab | ||
| ), | ||
| }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Avoid dropping reload/auto-refresh updates when the active session changes.
Both async flows apply updates to the current active session at completion time. If the user switches sessions mid-flight, the original tab stays stale. Capture the target session ID once and use it consistently when applying updates.
🛠️ Suggested fix (capture targetSessionId)
const handleReloadFileTab = useCallback(async (tabId: string) => {
- const { sessions, activeSessionId } = useSessionStore.getState();
+ const { sessions, activeSessionId } = useSessionStore.getState();
+ const targetSessionId = activeSessionId;
const currentSession = sessions.find((s) => s.id === activeSessionId);
if (!currentSession) return;
@@
useSessionStore.getState().setSessions((prev: Session[]) =>
prev.map((s) => {
- if (s.id !== useSessionStore.getState().activeSessionId) return s;
+ if (s.id !== targetSessionId) return s;
return {
...s,
filePreviewTabs: s.filePreviewTabs.map((tab) =>
@@
const handleSelectFileTab = useCallback(async (tabId: string) => {
- const { sessions, activeSessionId, setSessions } = useSessionStore.getState();
+ const { sessions, activeSessionId, setSessions } = useSessionStore.getState();
+ const targetSessionId = activeSessionId;
const currentSession = sessions.find((s) => s.id === activeSessionId);
if (!currentSession) return;
@@
setSessions((prev: Session[]) =>
prev.map((s) => {
- if (s.id !== activeSessionId) return s;
+ if (s.id !== targetSessionId) return s;
return { ...s, activeFileTabId: tabId };
})
);
@@
useSessionStore.getState().setSessions((prev: Session[]) =>
prev.map((s) => {
- if (s.id !== useSessionStore.getState().activeSessionId) return s;
+ if (s.id !== targetSessionId) return s;
return {
...s,
filePreviewTabs: s.filePreviewTabs.map((tab) =>Also applies to: 578-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/tabs/useTabHandlers.ts` around lines 521 - 553, The
handler handleReloadFileTab currently reads the active session at completion
time which can apply updates to the wrong session if the user switches tabs;
capture the target session id at the start (e.g., const targetSessionId =
useSessionStore.getState().activeSessionId) and then use that captured value
inside the Promise resolution and setSessions mapper instead of re-reading
useSessionStore.getState().activeSessionId; ensure the same change is applied to
the other handler referenced (lines ~578-599) and update the setSessions mapping
for filePreviewTabs by matching s.id === targetSessionId and tab.id === tabId so
updates always apply to the intended session/tab.
| const history = currentTab.navigationHistory ?? []; | ||
| const currentIndex = currentTab.navigationIndex ?? history.length - 1; | ||
|
|
||
| if (currentIndex > 0) { | ||
| const newIndex = currentIndex - 1; | ||
| const historyEntry = history[newIndex]; | ||
|
|
||
| try { | ||
| const sshRemoteId = currentTab.sshRemoteId; | ||
| const content = await window.maestro.fs.readFile(historyEntry.path, sshRemoteId); | ||
| if (!content) return; | ||
|
|
||
| setSessions((prev: Session[]) => | ||
| prev.map((s) => { | ||
| if (s.id !== currentSession.id) return s; | ||
| return { | ||
| ...s, | ||
| filePreviewTabs: s.filePreviewTabs.map((tab) => | ||
| tab.id === currentTab.id | ||
| ? { | ||
| ...tab, | ||
| path: historyEntry.path, | ||
| name: historyEntry.name, | ||
| content, | ||
| scrollTop: historyEntry.scrollTop ?? 0, | ||
| navigationIndex: newIndex, | ||
| } | ||
| : tab | ||
| ), | ||
| }; | ||
| }) |
There was a problem hiding this comment.
Update the file extension when navigating file-tab history.
Navigation changes path/name but leaves extension from the previous file, which can desync icons/syntax handling. Compute the extension from the history path in back/forward/index handlers.
🛠️ Suggested fix (apply to back/forward/index handlers)
-const historyEntry = history[newIndex];
+const historyEntry = history[newIndex];
+const fileName = historyEntry.path.split('/').pop() ?? historyEntry.name;
+const extension = fileName.includes('.') ? `.${fileName.split('.').pop()}` : '';
+const nameWithoutExtension = extension ? fileName.slice(0, -extension.length) : fileName;
@@
- name: historyEntry.name,
+ name: nameWithoutExtension,
+ extension,Also applies to: 1291-1322, 1339-1367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/tabs/useTabHandlers.ts` around lines 1243 - 1273, The
file-preview navigation handlers (back/forward/index in useTabHandlers: code
paths that read historyEntry and call setSessions to update filePreviewTabs)
update path/name/content but forget to recompute the file extension, leaving
extension stale and desyncing icons/syntax; when constructing the new tab object
inside setSessions for the matching tab id (in the block that currently assigns
path: historyEntry.path, name: historyEntry.name, content, scrollTop,
navigationIndex), compute the extension from historyEntry.path (e.g., derive the
substring after the last dot or use the existing utility for extension
extraction) and set extension: computedExtension on the returned tab; apply the
same change to the corresponding handlers at the other locations mentioned
(around the blocks for lines noted in the review).
| const handleConfirmDeleteWorktree = useCallback(() => { | ||
| const deleteWtSession = useModalStore.getState().getData('deleteWorktree')?.session ?? null; | ||
| if (!deleteWtSession) return; | ||
| // Remove the session but keep the worktree on disk | ||
| useSessionStore | ||
| .getState() | ||
| .setSessions((prev) => prev.filter((s) => s.id !== deleteWtSession.id)); | ||
| }, []); | ||
|
|
||
| const handleConfirmAndDeleteWorktreeOnDisk = useCallback(async () => { | ||
| const deleteWtSession = useModalStore.getState().getData('deleteWorktree')?.session ?? null; | ||
| if (!deleteWtSession) return; | ||
| // Remove the session AND delete the worktree from disk | ||
| const result = await window.maestro.git.removeWorktree(deleteWtSession.cwd, true); | ||
| if (!result.success) { | ||
| throw new Error(result.error || 'Failed to remove worktree'); | ||
| } | ||
| useSessionStore | ||
| .getState() | ||
| .setSessions((prev) => prev.filter((s) => s.id !== deleteWtSession.id)); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for removed-worktree tracking mechanisms
rg -n "removedWorktreePaths|removedWorktree|markWorktree|removeWorktreePath" -g '*.ts' -g '*.tsx'
# Search for handleConfirmDeleteWorktree usage and related delete flows
rg -n "handleConfirmDeleteWorktree|deleteWorktreeSession|deleteWorktree" -g '*.ts' -g '*.tsx' | head -50Repository: RunMaestro/Maestro
Length of output: 6557
🏁 Script executed:
# Check the file structure around session store and worktree management
fd "session" -g "*.ts" -g "*.tsx" | grep -i store | head -20Repository: RunMaestro/Maestro
Length of output: 205
🏁 Script executed:
# Look for file watcher or discovery/scanner related code
rg -n "fileWatcher|legacyScanner|discoverWorktree|scanWorktree" -g '*.ts' -g '*.tsx' | head -30Repository: RunMaestro/Maestro
Length of output: 3096
🏁 Script executed:
# View the handleConfirmDeleteWorktree function and surrounding code
sed -n '545,575p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 1304
🏁 Script executed:
# View the context around line 811 where removedWorktreePaths is used
sed -n '800,825p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 1044
🏁 Script executed:
# Examine sessionStore.ts to see the removeWorktreePath method
rg -A 10 "removeWorktreePath|markWorktreeAsRemoved" src/renderer/stores/sessionStore.tsRepository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check if scanWorktreeDirectory or the discovery process filters out removedWorktreePaths
rg -B 5 -A 15 "scanWorktreeDirectory\(" src/renderer/hooks/worktree/useWorktreeHandlers.ts | head -80Repository: RunMaestro/Maestro
Length of output: 2533
🏁 Script executed:
# Look for methods in sessionStore to add/remove from removedWorktreePaths
rg -B 5 -A 10 "addRemovedWorktreePath|addRemovedWorktree|removeWorktree" src/renderer/stores/sessionStore.tsRepository: RunMaestro/Maestro
Length of output: 1453
🏁 Script executed:
# Check the discovery logic at line 283
sed -n '270,300p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 1193
🏁 Script executed:
# Check the discovery logic at line 596
sed -n '578,620p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 1774
🏁 Script executed:
# Check what methods exist on useSessionStore for removedWorktreePaths
rg -B 2 -A 8 "removedWorktreePaths.*=" src/renderer/stores/sessionStore.ts | head -50Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Verify that the discovery at lines 283 and 596 do NOT check removedWorktreePaths
sed -n '270,310p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 1514
🏁 Script executed:
# Also check the full scanWorktreeConfigsOnStartup to see if it checks removedWorktreePaths
sed -n '578,650p' src/renderer/hooks/worktree/useWorktreeHandlers.tsRepository: RunMaestro/Maestro
Length of output: 2589
Add removed worktree path to prevent re-discovery.
handleConfirmDeleteWorktree removes the session but does not call addRemovedWorktreePath(). The removedWorktreePaths tracking mechanism exists and is checked by scanWorktreeParents, but the startup scanner (scanWorktreeConfigsOnStartup) and the worktree config handler do not check it. When users remove a worktree, it can reappear after app restart or when the worktree config is saved. Call useSessionStore.getState().addRemovedWorktreePath(deleteWtSession.cwd) in handleConfirmDeleteWorktree to mark the path as removed. Also update the other discovery flows (lines ~596 and ~283) to check removedWorktreePaths for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 547 - 567,
When deleting a worktree session without removing files, call
useSessionStore.getState().addRemovedWorktreePath(deleteWtSession.cwd) inside
handleConfirmDeleteWorktree to mark the path as intentionally removed; also add
the same check in discovery flows so removed paths are skipped: update
scanWorktreeConfigsOnStartup and the worktree-config save/handler (the function
that processes discovered worktree configs) to consult
useSessionStore.getState().removedWorktreePaths (e.g., check
removedWorktreePaths.has(path) and skip adding/re-discovering those paths) so
removed worktrees do not reappear after restart or config save.
…ype safety - Batch consecutive setSessions calls in useWorktreeHandlers (avoid transient state and extra IPC writes) - Remove 9 debug console.log calls from useGroupChatHandlers and useWorktreeHandlers - Extract resetGroupChatUI() helper to deduplicate close logic - Simplify setGroupChatState to direct value form - Replace any[] with StoredSession[]/Group[] in persistence.ts - Replace console.log with logger.debug in persistence.ts - Add clarifying comments: worktreeConfigKey purpose, sessions subscription breadth, getModalActions() legacy shim, hardcoded agentId TODO
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 (1)
src/main/ipc/handlers/persistence.ts (1)
177-185:⚠️ Potential issue | 🟠 MajorDon’t swallow unexpected persistence errors; report to Sentry and rethrow.
Both
sessions:setAllandgroups:setAllcatch all errors, log, and returnfalse. Per guidelines, only expected/recoverable errors (explicit codes) should be handled; unexpected failures must be reported to Sentry and allowed to bubble. Please gate on specific error codes and usecaptureExceptionfor the rest (and consider aligningsettings:setsimilarly).🔧 Suggested fix (explicit codes + Sentry)
+import { captureException } from '../../utils/sentry'; @@ } catch (err) { // ENOSPC, ENFILE, or JSON serialization failures are recoverable — // the next debounced write will succeed when conditions improve. // Log but don't throw so the renderer doesn't see an unhandled rejection. const code = (err as NodeJS.ErrnoException).code; - logger.warn(`Failed to persist sessions: ${code || (err as Error).message}`, 'Sessions'); - return false; + if (code === 'ENOSPC' || code === 'ENFILE') { + logger.warn(`Failed to persist sessions: ${code}`, 'Sessions'); + return false; + } + captureException(err, { + tags: { area: 'Sessions', op: 'sessions:setAll' }, + extra: { code }, + }); + throw err; } @@ } catch (err) { const code = (err as NodeJS.ErrnoException).code; - logger.warn(`Failed to persist groups: ${code || (err as Error).message}`, 'Groups'); - return false; + if (code === 'ENOSPC' || code === 'ENFILE') { + logger.warn(`Failed to persist groups: ${code}`, 'Groups'); + return false; + } + captureException(err, { + tags: { area: 'Groups', op: 'groups:setAll' }, + extra: { code }, + }); + throw err; }As per coding guidelines "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking... Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
Also applies to: 196-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/persistence.ts` around lines 177 - 185, The handlers for sessions:setAll (around sessionsStore.set in persistence.ts) currently catch all errors and swallow them; change the try/catch to only treat known recoverable errors (e.g., errno codes ENOSPC, ENFILE, and JSON serialization failures detected via instanceof SyntaxError or error.message) as recoverable (log and return false), but for any other errors call captureException from src/utils/sentry with contextual info (e.g., handler name "sessions:setAll" or "groups:setAll") and rethrow the error so it bubbles; apply the same pattern to the groups:setAll block (and align settings:set) so unexpected exceptions are reported to Sentry and not silently suppressed.
🤖 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/groupChat/useGroupChatHandlers.ts`:
- Around line 252-275: The queue processor must only process items intended for
the current active chat: before dequeuing, inspect groupChatExecutionQueue for
an item whose tabId === activeGroupChatId (referencing groupChatExecutionQueue,
activeGroupChatId, and nextItem.tabId); if the head item isn’t for the active
tab, scan for the first matching item, remove only that item from the queue via
setGroupChatExecutionQueue, and use that item's tabId when updating
setGroupChatState/setGroupChatStates and when calling
window.maestro.groupChat.sendToModerator so state changes and sends target the
correct chat (keep useEffect, setGCState/setGCStates, and sendToModerator logic
but operate on the matched queue item instead of unconditionally using the
head).
- Around line 352-357: The call to window.maestro.settings.set(...) inside
handleGroupChatRightTabChange is returning a Promise<boolean> that is currently
ignored; update handleGroupChatRightTabChange (referencing the function name and
setGroupChatRightTab from useGroupChatStore.getState) to handle that Promise
explicitly—either make the callback async and await the call inside a try/catch
and report errors via the existing Sentry utilities (or processLogger) or append
a .catch(...) to window.maestro.settings.set(...) that logs or reports the
error—ensuring no unhandled rejection occurs.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 344-346: In useWorktreeHandlers.ts (inside the useWorktreeHandlers
hook) replace console.error in the catch blocks (e.g., the catch around the
worktree scan, and the other catches at the ranges you flagged) with
Sentry.captureException(err) and then either rethrow the error or implement
explicit handling for known/expected error cases; update the catch bodies in the
relevant functions (the scan-for-worktrees handler and the other handlers at the
flagged ranges) to call captureException(err) and then either: a) throw err to
let it bubble to global error tracking, or b) inspect the error code/message and
handle that case explicitly (return a controlled result or user-friendly error)
before optionally rethrowing unknown errors. Ensure you import captureException
from Sentry and remove console.error calls.
- Around line 67-75: The fetchGitInfo handler currently swallows all errors by
returning {} in the catch; instead, catch only known/recoverable error cases
(inspect error codes/messages from gitService.getBranches/getTags) and handle
them explicitly, but for unexpected errors call Sentry's
captureException/captureMessage (import from src/utils/sentry) with context
(path, sshRemoteId) and then rethrow the error so it can bubble up; update the
catch block in fetchGitInfo to implement this behavior and reference
gitService.getBranches, gitService.getTags, and the Sentry utilities.
---
Outside diff comments:
In `@src/main/ipc/handlers/persistence.ts`:
- Around line 177-185: The handlers for sessions:setAll (around
sessionsStore.set in persistence.ts) currently catch all errors and swallow
them; change the try/catch to only treat known recoverable errors (e.g., errno
codes ENOSPC, ENFILE, and JSON serialization failures detected via instanceof
SyntaxError or error.message) as recoverable (log and return false), but for any
other errors call captureException from src/utils/sentry with contextual info
(e.g., handler name "sessions:setAll" or "groups:setAll") and rethrow the error
so it bubbles; apply the same pattern to the groups:setAll block (and align
settings:set) so unexpected exceptions are reported to Sentry and not silently
suppressed.
---
Duplicate comments:
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 282-295: handleProcessMonitorNavigateToGroupChat currently swaps
active IDs/states/participants but doesn't touch the message cache, leaving
stale messages visible; update this handler to clear or load the correct
messages by calling the store's message setter (e.g., setGroupChatMessages) for
the target groupChatId: first clear existing messages (or set to
groupChatMessages.get(groupChatId) ?? []) or otherwise load the persisted
messages for groupChatId, then proceed to setActiveGroupChatId,
setGroupChatState, setParticipantStates and closeModal('processMonitor'),
ensuring you reference handleProcessMonitorNavigateToGroupChat,
useGroupChatStore.getState(), and the groupChatMessages setter so the UI shows
the correct messages after navigation.
In `@src/renderer/hooks/modal/useModalHandlers.ts`:
- Around line 456-517: The issue is that handleSetLightboxImage sets flags
(setLightboxIsGroupChat, setLightboxAllowDelete) before opening the lightbox,
which can be lost if updateModalData is a no-op prior to open; fix by reordering
operations in handleSetLightboxImage so you first call
actions.setLightboxImage(image), actions.setLightboxImages(contextImages || []),
and actions.setLightboxSource(source) (and any open/show modal action if
present) and only after those succeed call
actions.setLightboxIsGroupChat(activeGroupChatId !== null) and
actions.setLightboxAllowDelete(source === 'staged') to ensure flags are applied
after the modal data is initialized.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 541-548: The delete handler currently removes the session but
doesn't mark its filesystem path as ignored, so worktrees reappear on restart;
update handleConfirmDeleteWorktree to obtain the deleted worktree's path from
the session (useModalStore.getState().getData('deleteWorktree')?.session) and
add that path to removedWorktreePaths (via whatever setter/store holds
removedWorktreePaths) before removing the session, and apply the same change to
the other delete handler referenced around the 596-611 block so both handlers
persist the path into removedWorktreePaths used by the startup/rescan logic.
| useEffect(() => { | ||
| if (groupChatState === 'idle' && groupChatExecutionQueue.length > 0 && activeGroupChatId) { | ||
| const { | ||
| setGroupChatExecutionQueue, | ||
| setGroupChatState: setGCState, | ||
| setGroupChatStates: setGCStates, | ||
| } = useGroupChatStore.getState(); | ||
|
|
||
| const [nextItem, ...remainingQueue] = groupChatExecutionQueue; | ||
| setGroupChatExecutionQueue(remainingQueue); | ||
|
|
||
| setGCState('moderator-thinking'); | ||
| setGCStates((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(activeGroupChatId, 'moderator-thinking'); | ||
| return next; | ||
| }); | ||
| window.maestro.groupChat.sendToModerator( | ||
| activeGroupChatId, | ||
| nextItem.text || '', | ||
| nextItem.images, | ||
| nextItem.readOnlyMode | ||
| ); | ||
| } |
There was a problem hiding this comment.
Queue processor ignores queued item tabId, risking wrong-target sends.
If the user switches chats while the queue still has items, the next item can be sent to the current active chat instead of the item’s intended tabId, and state updates will be applied to the wrong chat.
🛠️ Suggested fix (process only items for the active chat)
- if (groupChatState === 'idle' && groupChatExecutionQueue.length > 0 && activeGroupChatId) {
+ if (groupChatState === 'idle' && activeGroupChatId) {
const {
setGroupChatExecutionQueue,
setGroupChatState: setGCState,
setGroupChatStates: setGCStates,
} = useGroupChatStore.getState();
- const [nextItem, ...remainingQueue] = groupChatExecutionQueue;
- setGroupChatExecutionQueue(remainingQueue);
+ const nextIndex = groupChatExecutionQueue.findIndex(
+ (item) => item.tabId === activeGroupChatId
+ );
+ if (nextIndex === -1) return;
+ const nextItem = groupChatExecutionQueue[nextIndex];
+ const remainingQueue = groupChatExecutionQueue.filter((_, index) => index !== nextIndex);
+ setGroupChatExecutionQueue(remainingQueue);
setGCState('moderator-thinking');
setGCStates((prev) => {
const next = new Map(prev);
next.set(activeGroupChatId, 'moderator-thinking');
return next;
});
- window.maestro.groupChat.sendToModerator(
- activeGroupChatId,
+ window.maestro.groupChat.sendToModerator(
+ nextItem.tabId,
nextItem.text || '',
nextItem.images,
nextItem.readOnlyMode
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 252 - 275,
The queue processor must only process items intended for the current active
chat: before dequeuing, inspect groupChatExecutionQueue for an item whose tabId
=== activeGroupChatId (referencing groupChatExecutionQueue, activeGroupChatId,
and nextItem.tabId); if the head item isn’t for the active tab, scan for the
first matching item, remove only that item from the queue via
setGroupChatExecutionQueue, and use that item's tabId when updating
setGroupChatState/setGroupChatStates and when calling
window.maestro.groupChat.sendToModerator so state changes and sends target the
correct chat (keep useEffect, setGCState/setGCStates, and sendToModerator logic
but operate on the matched queue item instead of unconditionally using the
head).
| const handleGroupChatRightTabChange = useCallback((tab: GroupChatRightTab) => { | ||
| const { setGroupChatRightTab, activeGroupChatId } = useGroupChatStore.getState(); | ||
| setGroupChatRightTab(tab); | ||
| if (activeGroupChatId) { | ||
| window.maestro.settings.set(`groupChatRightTab:${activeGroupChatId}`, tab); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for maestro.settings.set definition and type information
rg "settings\.set" -g'*.ts' -g'*.tsx' -g'*.js' --max-count=20Repository: RunMaestro/Maestro
Length of output: 12574
🏁 Script executed:
# Search for window.maestro definition and setup
rg "window\.maestro\s*=" -g'*.ts' -g'*.tsx' -g'*.js'Repository: RunMaestro/Maestro
Length of output: 1842
🏁 Script executed:
# Look for preload or IPC definitions
fd -e ts -e tsx -e js | grep -i preload | head -20Repository: RunMaestro/Maestro
Length of output: 624
🏁 Script executed:
# Search for maestro settings interface/type definitions
rg "settings\s*:\s*\{" -A 10 -g'*.ts' -g'*.tsx'Repository: RunMaestro/Maestro
Length of output: 24530
🏁 Script executed:
# Check the test for useGroupChatHandlers to see if there's guidance on how to handle this
cat -n src/__tests__/renderer/hooks/useGroupChatHandlers.test.ts | head -200Repository: RunMaestro/Maestro
Length of output: 9615
🏁 Script executed:
# Search for the specific test for handleGroupChatRightTabChange
rg -n "handleGroupChatRightTabChange" src/__tests__/renderer/hooks/useGroupChatHandlers.test.ts -A 15Repository: RunMaestro/Maestro
Length of output: 1936
Handle Promise from window.maestro.settings.set() to avoid unhandled rejections.
window.maestro.settings.set() returns Promise<boolean> (defined in src/renderer/global.d.ts). The current code ignores this Promise without error handling. If the operation fails, the rejection will go unhandled. Either await the call and handle errors with Sentry utilities, or explicitly attach a .catch() handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 352 - 357,
The call to window.maestro.settings.set(...) inside
handleGroupChatRightTabChange is returning a Promise<boolean> that is currently
ignored; update handleGroupChatRightTabChange (referencing the function name and
setGroupChatRightTab from useGroupChatStore.getState) to handle that Promise
explicitly—either make the callback async and await the call inside a try/catch
and report errors via the existing Sentry utilities (or processLogger) or append
a .catch(...) to window.maestro.settings.set(...) that logs or reports the
error—ensuring no unhandled rejection occurs.
| try { | ||
| const [gitBranches, gitTags] = await Promise.all([ | ||
| gitService.getBranches(path, sshRemoteId), | ||
| gitService.getTags(path, sshRemoteId), | ||
| ]); | ||
| return { gitBranches, gitTags, gitRefsCacheTime: Date.now() }; | ||
| } catch { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
Don’t swallow git info failures in fetchGitInfo.
Returning {} on any failure hides real errors and violates the “bubble to Sentry / handle expected error codes” rule. Prefer capturing and rethrowing, or explicitly handle known recoverable error codes.
🔧 Proposed fix
-import { generateId } from '../../utils/ids';
+import { generateId } from '../../utils/ids';
+import { captureException } from 'src/utils/sentry';
async function fetchGitInfo(
path: string,
sshRemoteId?: string
): Promise<{
gitBranches?: string[];
gitTags?: string[];
gitRefsCacheTime?: number;
}> {
try {
const [gitBranches, gitTags] = await Promise.all([
gitService.getBranches(path, sshRemoteId),
gitService.getTags(path, sshRemoteId),
]);
return { gitBranches, gitTags, gitRefsCacheTime: Date.now() };
- } catch {
- return {};
+ } catch (error) {
+ captureException(error, {
+ tags: { area: 'worktree', op: 'fetchGitInfo' },
+ extra: { path, sshRemoteId },
+ });
+ throw error;
}
}As per coding guidelines: "Handle only expected/recoverable errors explicitly with specific error codes. Let unexpected errors bubble up to Sentry. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 67 - 75, The
fetchGitInfo handler currently swallows all errors by returning {} in the catch;
instead, catch only known/recoverable error cases (inspect error codes/messages
from gitService.getBranches/getTags) and handle them explicitly, but for
unexpected errors call Sentry's captureException/captureMessage (import from
src/utils/sentry) with context (path, sshRemoteId) and then rethrow the error so
it can bubble up; update the catch block in fetchGitInfo to implement this
behavior and reference gitService.getBranches, gitService.getTags, and the
Sentry utilities.
| } catch (err) { | ||
| console.error('Failed to scan for worktrees:', err); | ||
| } |
There was a problem hiding this comment.
Replace console.error catch blocks with Sentry + explicit handling.
These catches log and swallow errors (or log before rethrow), which the guidelines prohibit. Use captureException and either rethrow or handle known error codes explicitly.
🔧 Example adjustment (apply similarly to other catch blocks)
-import { generateId } from '../../utils/ids';
+import { generateId } from '../../utils/ids';
+import { captureException } from 'src/utils/sentry';
} catch (err) {
- console.error('[WorktreeConfig] Failed to create worktree:', err);
+ captureException(err, {
+ tags: { area: 'worktree', op: 'worktreeSetup' },
+ extra: { branchName, worktreePath },
+ });
notifyToast({
type: 'error',
title: 'Failed to Create Worktree',
message: err instanceof Error ? err.message : String(err),
});
throw err; // Re-throw so the modal can show the error
}As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking."
Also applies to: 451-456, 635-639, 845-847
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 344 - 346,
In useWorktreeHandlers.ts (inside the useWorktreeHandlers hook) replace
console.error in the catch blocks (e.g., the catch around the worktree scan, and
the other catches at the ranges you flagged) with Sentry.captureException(err)
and then either rethrow the error or implement explicit handling for
known/expected error cases; update the catch bodies in the relevant functions
(the scan-for-worktrees handler and the other handlers at the flagged ranges) to
call captureException(err) and then either: a) throw err to let it bubble to
global error tracking, or b) inspect the error code/message and handle that case
explicitly (return a controlled result or user-friendly error) before optionally
rethrowing unknown errors. Ensure you import captureException from Sentry and
remove console.error calls.
Code Review — Extract 4 Domain Hooks from App.tsxOverall this is solid, well-scoped refactoring. The Bugs / Correctness1. // useModalHandlers.ts ~line 11111
const handleCloseDeleteAgentModal = useCallback(() => {
getModalActions().setDeleteAgentModalOpen(false); // → closeModal('deleteAgent')
getModalActions().setDeleteAgentSession(null); // → closeModal('deleteAgent') again
}, []);
2. // App.tsx ~line 9418
setDeleteAgentSession(session); // opens modal with data
// setDeleteAgentModalOpen(true) — removedThis works because 3. IPC listener re-registration scope in useEffect(() => {
// Registers 6 listeners: onMessage, onStateChange, onParticipantsChanged,
// onModeratorUsage, onParticipantState, onModeratorSessionIdChanged
...
}, [activeGroupChatId]);Three of the six listeners ( Style / Conventions (per CLAUDE.md)4. // useWorktreeHandlers.ts
}, [sessions.some((s) => s.worktreeParentPath), defaultSaveToHistory]);While technically valid (evaluates to a boolean primitive), React's rules-of-hooks ESLint plugin will flag this as a non-stable expression in a dep array. More importantly, it's non-obvious to future readers. Extract to a named boolean: const hasWorktreeParentSessions = useMemo(
() => sessions.some((s) => s.worktreeParentPath),
[sessions]
);
// ...
}, [hasWorktreeParentSessions, defaultSaveToHistory]);5. Several 6. const worktreeConfigKey = useMemo(
() => sessions.map((s) => `${s.id}:${s.worktreeConfig?.basePath}:${s.worktreeConfig?.watchEnabled}`).join(','),
[sessions]
);This correctly prevents spurious file-watcher re-registrations. One edge case: if a session's What's Done Well
Summary
The double-close bug (#1) should be fixed. Issues #2 and #3 are worth addressing before this lands given the size of the refactor — both are correctness-adjacent patterns that will be copied by future contributors. #4 is a blocking ESLint concern if the linter config includes |
…ep array - Remove redundant setDeleteAgentModalOpen/setEditAgentModalOpen calls in close handlers (session setter already calls closeModal) - Split group chat IPC listeners into two effects: global (mount-once) for session-agnostic listeners, active-chat (dep on activeGroupChatId) for onMessage and onModeratorUsage — eliminates unnecessary listener churn - Extract inline sessions.some() from dep array to named useMemo (hasLegacyWorktreeSessions) for ESLint rules-of-hooks compliance - Switch worktreeConfigKey delimiter from : to | to avoid potential collisions - Update IPC listener tests to match two-effect split
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 293-305: The process monitor navigation handler
handleProcessMonitorNavigateToGroupChat sets IDs/state/participants but leaves
groupChatMessages from the previous chat, causing stale messages; update this
handler to reuse the existing handleOpenGroupChat flow (or explicitly
clear/reload messages) before calling closeModal('processMonitor') — e.g., call
handleOpenGroupChat(groupChatId) or invoke
useGroupChatStore.getState().setGroupChatMessages(new
Map()/loadMessagesFor(groupChatId)) within
handleProcessMonitorNavigateToGroupChat so groupChatMessages are correctly
initialized for the new group chat.
- Around line 263-285: The queue processor currently always dequeues the head
and sends to activeGroupChatId which can misroute messages if the user switched
tabs; instead, in the useEffect handling groupChatExecutionQueue find and remove
the first queue item whose tabId matches the current activeGroupChatId, call
setGroupChatExecutionQueue with the filtered queue (only removing that matched
item), update setGroupChatState and setGroupChatStates using that matched item's
tabId (not the global activeGroupChatId), and call
window.maestro.groupChat.sendToModerator with the matched item's tabId and its
text/images/readOnlyMode; use useGroupChatStore.getState() helpers
(setGroupChatExecutionQueue, setGroupChatState, setGroupChatStates) to apply
these changes atomically.
- Around line 363-368: The settings Promise from window.maestro.settings.set in
handleGroupChatRightTabChange is unhandled; update handleGroupChatRightTabChange
to handle rejections by awaiting the Promise in a try/catch or appending .catch
and reporting the error via the Sentry utilities (import
captureException/captureMessage from src/utils/sentry), passing contextual info
like { activeGroupChatId, tab } when logging; ensure the original behavior
(calling setGroupChatRightTab) remains synchronous and only the persistence call
is error-handled so no unhandled Promise rejections occur.
In `@src/renderer/hooks/modal/useModalHandlers.ts`:
- Around line 458-466: In handleSetLightboxImage, open the lightbox modal before
applying flags so updateModalData runs against an active modal: after obtaining
actions = getModalActions(), invoke the modal-open action (e.g.
actions.openLightbox or actions.openModal) immediately, then call
actions.setLightboxIsGroupChat(activeGroupChatId !== null) and
actions.setLightboxAllowDelete(source === 'staged'), followed by
actions.setLightboxImage, actions.setLightboxImages and
actions.setLightboxSource; this ensures the flag updates are applied on first
open.
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 58-75: fetchGitInfo currently swallows all errors and returns {},
hiding failures; update fetchGitInfo to catch errors, call the Sentry utilities
(captureException and/or captureMessage from src/utils/sentry) with contextual
info (path and sshRemoteId and which call failed —
gitService.getBranches/getTags), and then rethrow the error so unexpected
failures bubble up; if you must swallow only expected/recoverable cases,
explicitly check error codes and handle those cases only, logging them to Sentry
with context and returning an appropriate partial result.
- Around line 548-555: handleConfirmDeleteWorktree currently only removes the
session (via useSessionStore.setSessions) but does not record the worktree path
as removed, so deleted worktrees can be rediscovered; update
handleConfirmDeleteWorktree to also obtain the session's path from
useModalStore.getState().getData('deleteWorktree')?.session and add that path to
the persistent removedWorktreePaths set (e.g. via the config/store API you use
for removedWorktreePaths), then persist the change; additionally ensure
discovery/startup routines consult removedWorktreePaths and skip any paths
present there so deleted worktrees are not re-added during discovery.
- Around line 351-353: Replace the console.error-only catch blocks with Sentry
reporting and explicit handling: import and call captureException(err, { extra:
{ context: '<describe action, e.g. scanForWorktrees>' } }) in place of
console.error, then either rethrow the error or handle known cases explicitly
(e.g. return a controlled value or show a user-facing error) instead of
swallowing; specifically update the catch that logs "Failed to scan for
worktrees:" to call captureException with that context and then rethrow or
handle, and apply the same pattern to the other similar catch blocks (the other
three catches referenced) so all uses log to Sentry and perform explicit error
handling.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/hooks/modal/useModalHandlers.ts (1)
616-749: Clear scheduled timeouts on cleanup.
Several effects schedule delayed actions; clearing them prevents late focus/celebration updates after unmount.🧹 Cleanup pattern (apply similarly to other setTimeouts)
useEffect(() => { if (!logViewerOpen) { - setTimeout(() => { + const focusTimeout = window.setTimeout(() => { if (inputRef.current) { inputRef.current.focus(); } else if (terminalOutputRef.current) { terminalOutputRef.current.focus(); } else { (document.activeElement as HTMLElement)?.blur(); document.body.focus(); } - }, 50); + }, 50); + return () => window.clearTimeout(focusTimeout); } }, [logViewerOpen, inputRef, terminalOutputRef]); useEffect(() => { + let badgeTimeout: number | undefined; if (settingsLoaded && sessionsLoaded) { const { getUnacknowledgedBadgeLevel, autoRunStats } = useSettingsStore.getState(); const unacknowledgedLevel = getUnacknowledgedBadgeLevel(); if (unacknowledgedLevel !== null) { const badge = CONDUCTOR_BADGES.find((b) => b.level === unacknowledgedLevel); if (badge) { - setTimeout(() => { + badgeTimeout = window.setTimeout(() => { getModalActions().setStandingOvationData({ badge, isNewRecord: false, recordTimeMs: autoRunStats.longestRunMs, }); - }, 1000); + }, 1000); } } } + return () => { + if (badgeTimeout) window.clearTimeout(badgeTimeout); + }; }, [settingsLoaded, sessionsLoaded]); useEffect(() => { if (!settingsLoaded || !sessionsLoaded) return; // Debounce to avoid showing multiple times let checkPending = false; + let checkTimeout: number | undefined; const checkForUnacknowledgedBadge = () => { // Don't show if there's already an ovation displayed const currentOvation = useModalStore.getState().getData('standingOvation'); if (currentOvation) return; if (checkPending) return; const { getUnacknowledgedBadgeLevel, autoRunStats } = useSettingsStore.getState(); const unacknowledgedLevel = getUnacknowledgedBadgeLevel(); if (unacknowledgedLevel !== null) { const badge = CONDUCTOR_BADGES.find((b) => b.level === unacknowledgedLevel); if (badge) { checkPending = true; // Small delay to let the UI stabilize - setTimeout(() => { + checkTimeout = window.setTimeout(() => { // Double-check in case it was acknowledged in the meantime if (!useModalStore.getState().getData('standingOvation')) { getModalActions().setStandingOvationData({ badge, isNewRecord: false, recordTimeMs: autoRunStats.longestRunMs, }); } checkPending = false; - }, 500); + }, 500); } } }; // ... event listeners ... return () => { + if (checkTimeout) window.clearTimeout(checkTimeout); document.removeEventListener('visibilitychange', handleVisibilityChange); window.removeEventListener('focus', handleWindowFocus); document.removeEventListener('mousemove', handleMouseMove); }; }, [settingsLoaded, sessionsLoaded]); useEffect(() => { + let masteryTimeout: number | undefined; if (settingsLoaded && sessionsLoaded) { const unacknowledgedLevel = useSettingsStore .getState() .getUnacknowledgedKeyboardMasteryLevel(); if (unacknowledgedLevel !== null) { // Show the keyboard mastery level-up celebration after a short delay - setTimeout(() => { + masteryTimeout = window.setTimeout(() => { getModalActions().setPendingKeyboardMasteryLevel(unacknowledgedLevel); - }, 1200); + }, 1200); } } + return () => { + if (masteryTimeout) window.clearTimeout(masteryTimeout); + }; }, [settingsLoaded, sessionsLoaded]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/modal/useModalHandlers.ts` around lines 616 - 749, Multiple effects schedule setTimeouts but do not clear them on unmount; capture each timer ID and clear it in the effect cleanup: in the useEffect handling logViewerOpen (refs inputRef/terminalOutputRef) store the 50ms timer ID and clearTimeout on return; in the startup badge effect (calls getModalActions().setStandingOvationData with 1000ms) store and clear that timer; in the "user returns to the app" effect wrap the inner 500ms delayed action in a variable (e.g., pendingTimeout) and clearTimeout(pendingTimeout) in the effect cleanup (also ensure checkPending is reset appropriately); and in the keyboard mastery startup effect (setPendingKeyboardMasteryLevel with 1200ms) store and clear that timer—use window.setTimeout to get numeric IDs if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/hooks/modal/useModalHandlers.ts`:
- Around line 616-749: Multiple effects schedule setTimeouts but do not clear
them on unmount; capture each timer ID and clear it in the effect cleanup: in
the useEffect handling logViewerOpen (refs inputRef/terminalOutputRef) store the
50ms timer ID and clearTimeout on return; in the startup badge effect (calls
getModalActions().setStandingOvationData with 1000ms) store and clear that
timer; in the "user returns to the app" effect wrap the inner 500ms delayed
action in a variable (e.g., pendingTimeout) and clearTimeout(pendingTimeout) in
the effect cleanup (also ensure checkPending is reset appropriately); and in the
keyboard mastery startup effect (setPendingKeyboardMasteryLevel with 1200ms)
store and clear that timer—use window.setTimeout to get numeric IDs if needed.
1 participant
Summary
useTabHandlers— tab management callbacks + derived state (1,433 lines, 54 tests)useGroupChatHandlers— group chat IPC listeners, refs, open/close/send handlers (704 lines, 37 tests)useModalHandlers— modal open/close/toggle, celebration startup effects, LogViewer focus chain (843 lines, 92 tests)useWorktreeHandlers— worktree config/CRUD/expand, git branch info polling, SSH detection (917 lines, 40 tests)App.tsx reduced from ~12,863 → ~9,556 lines. All hooks read from Zustand stores via
getState()with stable[]dependency arrays — no prop threading needed. Zero behavioral changes; pure code movement.17 files changed, +9,937 / −3,645
Test plan
npx tsc --noEmit --prettycompiles cleannpm run test— 19,978 tests pass, 0 failuresSummary by CodeRabbit
Tests
New Features
Chores