refactor(agent): decompose useAgentListeners#975
Merged
Conversation
Split the IPC-listener god-hook per real seam (one hook per window.maestro.process.onXxx channel) and lift the shared activeHiddenToolRef into the coordinator before splitting consumers. Pure logic from onExit (queue dequeue, git refresh, synopsis spawn, eligibility predicate) is extracted as named helpers in internal/helpers/exit*.ts. Each per-channel hook owns its own subscribe/unsubscribe, its own state-touching surface, and a single unit-testable concern. The shell preserves the original public API and listener registration order (Data -> Exit -> SessionId -> SlashCommands -> Stderr -> CommandExit -> Usage -> AgentError -> Thinking -> SshRemote -> ToolExecution) so React mounts the per-channel useEffects in the same sequence; cross-listener event timing the existing test suite depends on is unchanged. App.tsx, services/promptInit.ts, and hooks/agent/ index.ts call sites are untouched. Perf wins shipped in passing: - Orphan early-return in hot listeners (Slash, Stderr, CommandExit, Usage, SshRemote, ToolExecution) avoids no-op prev.map allocations and Zustand subscriber notifications when the target session has been deleted (common during session-delete races). - Coordinator clears the shared activeHiddenToolRef Map on unmount so any orphan tool entries are released for GC. - Each per-channel useEffect captures only the deps it needs, not the union (smaller closures, better V8 inlining). Tests: existing 67-test integration suite stays green via the shell re-export. Added 110 new unit tests across 18 files (177 total for useAgentListeners and its internals). Full project suite: 27,720 passing / 108 skipped / 0 failing (was 27,693). Zero type errors, zero lint errors.
No behaviour change. Captures the lint-staged formatting pass that ran after the previous commit.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors the monolithic useAgentListeners into 11 dedicated per-IPC hooks, extracts dependency-light shared types and pure/async helper modules, hoists shared activeHiddenToolRef to the coordinator, and adds comprehensive Vitest tests for helpers and all new hooks. ChangesShared Types & Contracts
Core & Async Helper Modules
Listener Hooks
Coordinator & Re-exports
Test Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR decomposes the 1,977-line
Confidence Score: 4/5Safe to merge — this is a structural refactor with no public API changes, all 27,720 tests pass, and the observable behaviour of each IPC channel is preserved verbatim. All issues found are style and documentation concerns carried over from the original code or introduced by the decomposition. The stale coordinator comment about activeHiddenToolRef being written by onToolExecution could mislead future developers, and the pre-computed chooseNextQueuedItem / setSessions split makes a pre-existing post-await race window more structurally visible. Neither changes observable runtime behaviour given the existing test coverage. useAgentExitListener.ts is the densest file (603 LOC, retains the full setSessions reducer inline) and is the most likely source of future regressions; useAgentListeners.ts coordinator comment about activeHiddenToolRef warrants a correction before the next developer touches hidden-tool-progress tracking. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useAgentListeners coordinator] --> B[activeHiddenToolRef shared Map]
A --> C[useAgentDataListener onData]
A --> D[useAgentExitListener onExit]
A --> E[useAgentSessionIdListener onSessionId]
A --> F[useAgentSlashCommandsListener onSlashCommands]
A --> G[useAgentStderrListener onStderr]
A --> H[useAgentCommandExitListener onCommandExit]
A --> I[useAgentUsageListener onUsage]
A --> J[useAgentErrorListener onAgentError]
A --> K[useAgentThinkingListener onThinkingChunk]
A --> L[useAgentSshRemoteListener onSshRemote]
A --> M[useAgentToolExecutionListener onToolExecution]
B -.->|delete on data| C
B -.->|delete on exit| D
B -.->|delete on error| J
D --> N[exitDequeue chooseNextQueuedItem]
D --> O[exitTabCleanup cleanupExitedTabLogs]
D --> P[exitGitRefresh refreshGitRefsAfterTerminalExit]
D --> Q[exitSynopsis runExitSynopsis]
C --> R[exitTabCleanup removeHiddenProgressLog]
K --> S[RAF Buffer thinkingChunkBufferRef]
Reviews (1): Last reviewed commit: "style(agent): apply prettier formatting ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+53
to
59
| // Shared ref — written by `onToolExecution`, deleted by `onData` and | ||
| // `onAgentError`. Hoisted to the coordinator so per-channel hooks operate | ||
| // on the same Map. Inner listeners that don't read this ref shouldn't | ||
| // receive it. | ||
| const activeHiddenToolRef = useRef< | ||
| Map<string, { toolName: string; toolState?: ToolProgressState }> | ||
| >(new Map()); |
There was a problem hiding this comment.
activeHiddenToolRef is never written by onToolExecution
The coordinator comment states the Map is "written by onToolExecution", but useAgentToolExecutionListener is called with no arguments and never receives or writes to this ref. The Map is initialised empty and only .delete() / .clear() are ever called on it — all of which are no-ops on an empty Map. As a result, the cleanup on unmount (activeHiddenTools.clear()) and all three delete calls in onData, onExit, and onAgentError are dead code. removeHiddenProgressLog uses buildHiddenProgressLogId(tabId) for its lookup and doesn't consult the ref at all, so behaviour is functionally unchanged — but the comment actively misleads anyone trying to understand the hidden-tool-progress lifecycle here.
Comment on lines
+129
to
+138
| if (isFromAi) { | ||
| const currentSession = getSessions().find((s) => s.id === actualSessionId); | ||
| if (currentSession) { | ||
| const queueDecision = chooseNextQueuedItem(currentSession, tabIdFromSession); | ||
| if (queueDecision.action === 'dequeue' && queueDecision.item) { | ||
| queuedItemToProcess = { | ||
| sessionId: actualSessionId, | ||
| item: queueDecision.item, | ||
| }; | ||
| } |
There was a problem hiding this comment.
chooseNextQueuedItem result and setSessions reducer can diverge after await
queuedItemToProcess is pre-computed from currentSession (line 130) after the await getActiveProcesses() at line 93. The setSessions reducer then independently re-evaluates the same queue condition against the latest committed state (prev). If a concurrent IPC event mutates the queue between the two evaluations (for example, another onExit fires during the process-status check), queuedItemToProcess could be set while the reducer takes the wait path, or vice versa: the reducer dequeues and transitions state, but processQueuedItemRef.current is never called because queuedItemToProcess is null — leaving the item permanently stuck. This race existed in the original monolithic handler too, but the explicit side-effect computation is now more visible as a separate step that can get out of sync with the state update.
Comment on lines
+44
to
+50
| if (!session) return prev; | ||
|
|
||
| window.maestro.agentSessions | ||
| .registerSessionOrigin(session.projectRoot, agentSessionId, 'user') | ||
| .catch((err) => | ||
| logger.error('[onSessionId] Failed to register session origin:', undefined, err) | ||
| ); |
There was a problem hiding this comment.
setSessions reducer
window.maestro.agentSessions.registerSessionOrigin(...) is called from inside the setSessions updater function (lines 44-48). In Zustand, the updater is expected to be a pure function; in React 18 concurrent mode, state updater functions can be invoked more than once, which would fire the IPC call multiple times for a single onSessionId event. The resumeFailureDetected flag mutation on line 117 shares the same anti-pattern. Both patterns pre-exist this refactor, but they are now more visible as isolated behaviour in this hook.
![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
🧹 Nitpick comments (8)
src/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.ts (1)
20-20: ⚡ Quick winPrefer
?? ''over the non-null assertion onresult.content.The
!operator bypasses TypeScript's nullability check. Ifresult.successistruebutresult.contentisundefinedat runtime,cachedAutorunSynopsisPromptbecomesundefined(notnull), so the=== nullguard ingetAutorunSynopsisPromptwon't catch it and the function returnsundefinedtyped asstring— silently breaking the''-fallback contract.🛡️ Proposed fix
- cachedAutorunSynopsisPrompt = result.content!; + cachedAutorunSynopsisPrompt = result.content ?? '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.ts` at line 20, Replace the non-null assertion on result.content when assigning cachedAutorunSynopsisPrompt with a safe nullish coalescing default (use result.content ?? '') so cachedAutorunSynopsisPrompt is always a string; update the assignment in autorunSynopsisPrompt helper (where cachedAutorunSynopsisPrompt is set from result.content) and ensure getAutorunSynopsisPrompt logic relying on a string/'' fallback remains correct.src/renderer/hooks/agent/internal/useAgentSshRemoteListener.ts (1)
55-86: ⚡ Quick win
try/catcharoundgitServicecalls is dead code and silently swallows unexpected errors from Sentry.Per the established codebase pattern,
gitService.isRepo,gitService.getBranches, andgitService.getTagsare all implemented viacreateIpcMethodwith adefaultValueand withoutrethrow: true. This means IPC failures are swallowed internally and already reported to Sentry bycreateIpcMethod— thecatchblock at line 79 will never fire for IPC errors. For an unexpected runtime error (e.g., a programming bug inside the async IIFE), thelogger.errorcall silently consumes it instead of letting the unhandled rejection propagate to Sentry, directly violating the guideline: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."♻️ Proposed fix — remove the try/catch
- void (async () => { - try { - const isGitRepo = await gitService.isRepo(remoteCwd, sshRemote.id); - if (isGitRepo) { - const [gitBranches, gitTags] = await Promise.all([ - gitService.getBranches(remoteCwd, sshRemote.id), - gitService.getTags(remoteCwd, sshRemote.id), - ]); - const gitRefsCacheTime = Date.now(); - - setSessions((prev) => - prev.map((s) => { - if (s.id !== actualSessionId) return s; - if (s.isGitRepo) return s; - return { - ...s, - isGitRepo: true, - gitBranches, - gitTags, - gitRefsCacheTime, - }; - }) - ); - } - } catch (err) { - logger.error( - `[SSH] Failed to check git repo status for ${actualSessionId}:`, - undefined, - err - ); - } - })(); + void (async () => { + const isGitRepo = await gitService.isRepo(remoteCwd, sshRemote.id); + if (isGitRepo) { + const [gitBranches, gitTags] = await Promise.all([ + gitService.getBranches(remoteCwd, sshRemote.id), + gitService.getTags(remoteCwd, sshRemote.id), + ]); + const gitRefsCacheTime = Date.now(); + + setSessions((prev) => + prev.map((s) => { + if (s.id !== actualSessionId) return s; + if (s.isGitRepo) return s; + return { + ...s, + isGitRepo: true, + gitBranches, + gitTags, + gitRefsCacheTime, + }; + }) + ); + } + })();Based on learnings:
gitServicemethods (isRepo,getBranches,getTags) are implemented viacreateIpcMethodwithdefaultValueand withoutrethrow: true, so wrapping call sites intry/catchproduces dead code for IPC failures and suppresses unexpected runtime errors that should reach Sentry. As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/agent/internal/useAgentSshRemoteListener.ts` around lines 55 - 86, Remove the try/catch wrapper inside the async IIFE in useAgentSshRemoteListener so unexpected runtime errors are not silently swallowed; call gitService.isRepo, gitService.getBranches and gitService.getTags directly (they already handle IPC failures) and then update setSessions for actualSessionId as before, but do not catch/log errors here—let them propagate to Sentry per project guidelines.src/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx (1)
44-63: 💤 Low valueOptional: assert the routing arguments to
updateUsage.
expect(batched.updateUsage).toHaveBeenCalled()passes regardless of which(sessionId, tabId, usage)triple was forwarded. Asserting at least one of the two expected calls (per-tab and per-session) would catch routing regressions where the hook keeps calling but with wrong ids.🧪 Suggested tightening
- expect(batched.updateUsage).toHaveBeenCalled(); + expect(batched.updateUsage).toHaveBeenCalledWith( + 'sess-1', + null, + expect.objectContaining({ outputTokens: 50 }) + ); expect(batched.updateCycleTokens).toHaveBeenCalledWith('sess-1', 50);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx` around lines 44 - 63, The test currently only checks that batched.updateUsage was called but not what arguments were routed; update the test for useAgentUsageListener to assert the specific routing by verifying batched.updateUsage was called with the expected (sessionId, tabId, usage) tuples after invoking handler (use the created session id 'sess-1' and the tab id present on createMockSession or the default tab id), and also assert the per-session/per-tab distinction (e.g., one call for the tab and one for the session) alongside the existing expect(batched.updateCycleTokens) assertion so routing regressions are caught; reference the test-level handler variable, the useAgentUsageListener hook invocation, and batched.updateUsage when adding these assertions.src/renderer/hooks/agent/internal/useAgentThinkingListener.ts (1)
89-141: 💤 Low valueOptional: collapse the replace/append branches.
The
if (lastLog?.source === 'thinking')block has two near-identicalupdatedTabs.map(...)arms that differ only in the chosentextvalue. Computing the new text once and sharing the map call would shrink the surface area and make future log-shape changes a one-line edit.♻️ Sketch
- if (lastLog?.source === 'thinking') { - const combinedText = lastLog.text + bufferedContent; - if (isLikelyConcatenatedToolNames(combinedText)) { - logger.warn( - '[App] Detected malformed thinking content, replacing instead of appending' - ); - updatedTabs = updatedTabs.map((tab) => - tab.id === chunkTabId - ? { - ...tab, - logs: [ - ...tab.logs.slice(0, -1), - { - ...lastLog, - text: bufferedContent, - }, - ], - } - : tab - ); - } else { - updatedTabs = updatedTabs.map((tab) => - tab.id === chunkTabId - ? { - ...tab, - logs: [ - ...tab.logs.slice(0, -1), - { - ...lastLog, - text: combinedText, - }, - ], - } - : tab - ); - } + if (lastLog?.source === 'thinking') { + const combinedText = lastLog.text + bufferedContent; + const malformed = isLikelyConcatenatedToolNames(combinedText); + if (malformed) { + logger.warn( + '[App] Detected malformed thinking content, replacing instead of appending' + ); + } + const nextText = malformed ? bufferedContent : combinedText; + updatedTabs = updatedTabs.map((tab) => + tab.id === chunkTabId + ? { + ...tab, + logs: [...tab.logs.slice(0, -1), { ...lastLog, text: nextText }], + } + : tab + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/agent/internal/useAgentThinkingListener.ts` around lines 89 - 141, The map branches that update updatedTabs in useAgentThinkingListener.ts duplicate logic; compute the replacement text once (e.g., determine newText by checking lastLog?.source === 'thinking' and using bufferedContent or lastLog.text + bufferedContent unless isLikelyConcatenatedToolNames(combinedText) dictates using bufferedContent), then perform a single updatedTabs = updatedTabs.map(tab => tab.id === chunkTabId ? { ...tab, logs: lastLog?.source === 'thinking' ? [...tab.logs.slice(0, -1), { ...lastLog, text: newText }] : [...tab.logs, { id: generateId(), timestamp: Date.now(), source: 'thinking', text: newText }] : tab); this collapses the replace/append branches while still using lastLog, isLikelyConcatenatedToolNames, bufferedContent, chunkTabId, and generateId.src/renderer/hooks/agent/internal/useAgentUsageListener.ts (1)
60-62: 💤 Low valueOptional: name the
-5warning gap.
yellowThreshold - 5is the cap that keeps the fallback estimate from triggering the yellow warning prematurely. A named constant (e.g.,YELLOW_WARNING_GAP_PCT) would make the intent self-documenting and ease tuning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/agent/internal/useAgentUsageListener.ts` around lines 60 - 62, The magic number "-5" used when computing maxEstimate in useAgentUsageListener is unclear; introduce a named constant (e.g., YELLOW_WARNING_GAP_PCT) and replace the literal with yellowThreshold - YELLOW_WARNING_GAP_PCT so the intent is explicit and easy to tune; update the calculation that sets maxEstimate and keep the call to deps.batchedUpdater.updateContextUsage(actualSessionId, Math.min(estimated, maxEstimate)) unchanged otherwise.src/__tests__/renderer/hooks/agent/internal/helpers/exitSynopsis.test.ts (1)
234-246: 💤 Low valueConsider a lowercase-prefix variant.
This test only locks in the uppercase shape (
'AB12CD34'). If the regex inexitSynopsis.tsis broadened to be case-insensitive (see prior comment), an additional case for'ab12cd34'would prevent regressions in the case handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/renderer/hooks/agent/internal/helpers/exitSynopsis.test.ts` around lines 234 - 246, The test only asserts behavior for an uppercase UUID-like tabName; add a lowercase variant to prevent regressions if the regex becomes case-insensitive: update the test in exitSynopsis.test.ts (the it block calling runExitSynopsis with makeSynopsisData and asserting window.maestro.claude.updateSessionName not called) to also call runExitSynopsis with tabName 'ab12cd34' (or add a second it) so both 'AB12CD34' and 'ab12cd34' are covered.src/renderer/hooks/agent/internal/useAgentSessionIdListener.ts (1)
42-151: ⚡ Quick winSide effect and closure mutation inside
setSessionsreducer.The reducer body fires the
registerSessionOriginIPC call (lines 46–50) and mutates the outerresumeFailureDetectedflag from insideprev.map(line 113). This works today because zustand's setter invokes the function exactly once, but it couples correctness to that contract and makes the data flow harder to follow. Hoisting the IPC call and the resume-failure decision out of the reducer keeps the state update pure and the side effects explicit.♻️ Suggested restructure
- const unsubscribe = window.maestro.process.onSessionId( - async (sessionId: string, agentSessionId: string) => { - if (isBatchSession(sessionId)) return; - - const parsed = parseSessionId(sessionId); - const actualSessionId = parsed.actualSessionId; - const tabId = parsed.tabId ?? undefined; - - let resumeFailureDetected = false; - - setSessions((prev) => { - const session = prev.find((s) => s.id === actualSessionId); - if (!session) return prev; - - window.maestro.agentSessions - .registerSessionOrigin(session.projectRoot, agentSessionId, 'user') - .catch((err) => - logger.error('[onSessionId] Failed to register session origin:', undefined, err) - ); - - return prev.map((s) => { + const unsubscribe = window.maestro.process.onSessionId( + async (sessionId: string, agentSessionId: string) => { + if (isBatchSession(sessionId)) return; + + const parsed = parseSessionId(sessionId); + const actualSessionId = parsed.actualSessionId; + const tabId = parsed.tabId ?? undefined; + + const sessionBefore = useSessionStore + .getState() + .sessions.find((s) => s.id === actualSessionId); + if (!sessionBefore) return; + + window.maestro.agentSessions + .registerSessionOrigin(sessionBefore.projectRoot, agentSessionId, 'user') + .catch((err) => + logger.error('[onSessionId] Failed to register session origin:', undefined, err) + ); + + let resumeFailureDetected = false; + setSessions((prev) => + prev.map((s) => {(also remove the now-unneeded outer
prev.find/early-return inside the reducer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/agent/internal/useAgentSessionIdListener.ts` around lines 42 - 151, The reducer passed to setSessions currently performs side effects and mutates outer state: it calls window.maestro.agentSessions.registerSessionOrigin and sets resumeFailureDetected from inside prev.map; move those out so the reducer remains pure. Specifically, before calling setSessions, compute the session (using the same lookup logic that finds session by actualSessionId and tab by tabId/getActiveTab), call window.maestro.agentSessions.registerSessionOrigin(session.projectRoot, agentSessionId, 'user') and catch/log errors there, and determine whether resumeFailureDetected should be set (based on comparing existing targetTab.agentSessionId to agentSessionId) and set that flag outside the reducer; then call setSessions with a pure updater that only maps and returns the new session/aiTabs (using the same transformation logic in the current prev.map) without performing IPC calls or mutating resumeFailureDetected. Ensure you reference setSessions, registerSessionOrigin, resumeFailureDetected, getActiveTab, actualSessionId, tabId and agentSessionId so you update the correct code paths.src/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsx (1)
95-103: ⚡ Quick winMake the RAF cleanup test prove the callback cannot flush after unmount.
This currently only proves
cancelAnimationFramewas invoked. Because the mock never unschedulesscheduledcallbacks and the test never runsflushRaf()after unmount, a leaked post-unmount flush would still pass here. Making the mock cancellable and asserting that a post-unmountflushRaf()leaves logs unchanged would turn this into a real regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsx` around lines 95 - 103, The test for useAgentThinkingListener should be changed to ensure a scheduled RAF callback truly cannot run after unmount: update the test to use a cancellable RAF mock (so global.requestAnimationFrame returns an id and global.cancelAnimationFrame removes that id from the scheduled set) and then after calling handler!('sess-1-ai-tab-1','data') and unmount(), call the test helper flushRaf() and assert that no additional logs/side-effects occurred; locate the test using symbols useAgentThinkingListener, handler, flushRaf, and cancelAnimationFrame and modify the mock RAF behaviour and assertions so a post-unmount flushRaf() does not invoke the previously scheduled callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/__tests__/renderer/hooks/agent/internal/useAgentSshRemoteListener.test.tsx`:
- Around line 55-69: The test "does NOT re-fetch when same remote already
attached" is missing assertions and never sets session.isGitRepo, so it passes
vacuously; update the test that creates the session via createMockSession and
sets useSessionStore state, then set session.isGitRepo = true (or
createMockSession with isGitRepo: true) before rendering
useAgentSshRemoteListener(), spy or mock the git probe function used by the hook
(the probe invoked by useAgentSshRemoteListener/handler), call handler('sess-1',
{ id: 'r-1', ... }) and await the microtask, and finally add an expect asserting
the probe was not called (e.g., expect(probeMock).not.toHaveBeenCalled()) to
verify no re-fetch occurs when the same remote is already attached.
In `@src/renderer/hooks/agent/internal/helpers/exitSynopsis.ts`:
- Around line 172-196: persistTabNameAfterSynopsis uses a regex that only
matches uppercase hex when detecting UUID-prefixes; change the detection to be
case-insensitive so lowercase prefixes don't slip through. In the
persistTabNameAfterSynopsis function, update the UUID-prefix check (the
isUuidPrefix test against synopsisData.tabName) to use a case-insensitive match
(or normalize the string) so both lowercase and uppercase hex characters are
treated the same, leaving the rest of the logic (persistAgentId,
persistProjectRoot, persistSessionId and the window.maestro.* calls) unchanged.
In `@src/renderer/hooks/agent/internal/useAgentErrorListener.ts`:
- Around line 128-130: The listener currently uses parsed.baseSessionId which
strips important context; update the code in useAgentErrorListener to use
parsed.actualSessionId (the full session id returned by parseSessionId) when
assigning actualSessionId for error tracking and related updates, matching the
behavior in useAgentSessionIdListener; keep tabIdFromSession as parsed.tabId ??
undefined and ensure any downstream references that used actualSessionId
continue to receive the full parsed.actualSessionId value instead of
baseSessionId.
In `@src/renderer/hooks/agent/internal/useAgentExitListener.ts`:
- Around line 86-104: The hidden-tool cleanup
(deps.activeHiddenToolRef.current?.delete(`${actualSessionId}:${tabIdFromSession}`))
must be moved to after the SAFETY CHECK that calls
window.maestro.process.getActiveProcesses() so we only delete when verification
confirms the process is gone; in the block where processStillRunning is true,
abort the exit handler without deleting the hidden-tool entry (keep existing
logger.warn) and do not proceed to state transitions, and in the catch path
after logger.error re-throw the error (rather than continuing) so unexpected
failures bubble up to Sentry; update the on-exit flow that references isFromAi,
tabIdFromSession, actualSessionId, sessionId, deps.activeHiddenToolRef, and
window.maestro.process.getActiveProcesses accordingly.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/agent/internal/helpers/exitSynopsis.test.ts`:
- Around line 234-246: The test only asserts behavior for an uppercase UUID-like
tabName; add a lowercase variant to prevent regressions if the regex becomes
case-insensitive: update the test in exitSynopsis.test.ts (the it block calling
runExitSynopsis with makeSynopsisData and asserting
window.maestro.claude.updateSessionName not called) to also call runExitSynopsis
with tabName 'ab12cd34' (or add a second it) so both 'AB12CD34' and 'ab12cd34'
are covered.
In
`@src/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsx`:
- Around line 95-103: The test for useAgentThinkingListener should be changed to
ensure a scheduled RAF callback truly cannot run after unmount: update the test
to use a cancellable RAF mock (so global.requestAnimationFrame returns an id and
global.cancelAnimationFrame removes that id from the scheduled set) and then
after calling handler!('sess-1-ai-tab-1','data') and unmount(), call the test
helper flushRaf() and assert that no additional logs/side-effects occurred;
locate the test using symbols useAgentThinkingListener, handler, flushRaf, and
cancelAnimationFrame and modify the mock RAF behaviour and assertions so a
post-unmount flushRaf() does not invoke the previously scheduled callback.
In `@src/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx`:
- Around line 44-63: The test currently only checks that batched.updateUsage was
called but not what arguments were routed; update the test for
useAgentUsageListener to assert the specific routing by verifying
batched.updateUsage was called with the expected (sessionId, tabId, usage)
tuples after invoking handler (use the created session id 'sess-1' and the tab
id present on createMockSession or the default tab id), and also assert the
per-session/per-tab distinction (e.g., one call for the tab and one for the
session) alongside the existing expect(batched.updateCycleTokens) assertion so
routing regressions are caught; reference the test-level handler variable, the
useAgentUsageListener hook invocation, and batched.updateUsage when adding these
assertions.
In `@src/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.ts`:
- Line 20: Replace the non-null assertion on result.content when assigning
cachedAutorunSynopsisPrompt with a safe nullish coalescing default (use
result.content ?? '') so cachedAutorunSynopsisPrompt is always a string; update
the assignment in autorunSynopsisPrompt helper (where
cachedAutorunSynopsisPrompt is set from result.content) and ensure
getAutorunSynopsisPrompt logic relying on a string/'' fallback remains correct.
In `@src/renderer/hooks/agent/internal/useAgentSessionIdListener.ts`:
- Around line 42-151: The reducer passed to setSessions currently performs side
effects and mutates outer state: it calls
window.maestro.agentSessions.registerSessionOrigin and sets
resumeFailureDetected from inside prev.map; move those out so the reducer
remains pure. Specifically, before calling setSessions, compute the session
(using the same lookup logic that finds session by actualSessionId and tab by
tabId/getActiveTab), call
window.maestro.agentSessions.registerSessionOrigin(session.projectRoot,
agentSessionId, 'user') and catch/log errors there, and determine whether
resumeFailureDetected should be set (based on comparing existing
targetTab.agentSessionId to agentSessionId) and set that flag outside the
reducer; then call setSessions with a pure updater that only maps and returns
the new session/aiTabs (using the same transformation logic in the current
prev.map) without performing IPC calls or mutating resumeFailureDetected. Ensure
you reference setSessions, registerSessionOrigin, resumeFailureDetected,
getActiveTab, actualSessionId, tabId and agentSessionId so you update the
correct code paths.
In `@src/renderer/hooks/agent/internal/useAgentSshRemoteListener.ts`:
- Around line 55-86: Remove the try/catch wrapper inside the async IIFE in
useAgentSshRemoteListener so unexpected runtime errors are not silently
swallowed; call gitService.isRepo, gitService.getBranches and gitService.getTags
directly (they already handle IPC failures) and then update setSessions for
actualSessionId as before, but do not catch/log errors here—let them propagate
to Sentry per project guidelines.
In `@src/renderer/hooks/agent/internal/useAgentThinkingListener.ts`:
- Around line 89-141: The map branches that update updatedTabs in
useAgentThinkingListener.ts duplicate logic; compute the replacement text once
(e.g., determine newText by checking lastLog?.source === 'thinking' and using
bufferedContent or lastLog.text + bufferedContent unless
isLikelyConcatenatedToolNames(combinedText) dictates using bufferedContent),
then perform a single updatedTabs = updatedTabs.map(tab => tab.id === chunkTabId
? { ...tab, logs: lastLog?.source === 'thinking' ? [...tab.logs.slice(0, -1), {
...lastLog, text: newText }] : [...tab.logs, { id: generateId(), timestamp:
Date.now(), source: 'thinking', text: newText }] : tab); this collapses the
replace/append branches while still using lastLog,
isLikelyConcatenatedToolNames, bufferedContent, chunkTabId, and generateId.
In `@src/renderer/hooks/agent/internal/useAgentUsageListener.ts`:
- Around line 60-62: The magic number "-5" used when computing maxEstimate in
useAgentUsageListener is unclear; introduce a named constant (e.g.,
YELLOW_WARNING_GAP_PCT) and replace the literal with yellowThreshold -
YELLOW_WARNING_GAP_PCT so the intent is explicit and easy to tune; update the
calculation that sets maxEstimate and keep the call to
deps.batchedUpdater.updateContextUsage(actualSessionId, Math.min(estimated,
maxEstimate)) unchanged otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6eecfe4-048a-4cf3-b3c1-eff9e4ab3e7e
📒 Files selected for processing (37)
src/__tests__/renderer/hooks/agent/internal/helpers/agentErrorLogMatch.test.tssrc/__tests__/renderer/hooks/agent/internal/helpers/errorTitles.test.tssrc/__tests__/renderer/hooks/agent/internal/helpers/exitDequeue.test.tssrc/__tests__/renderer/hooks/agent/internal/helpers/exitGitRefresh.test.tssrc/__tests__/renderer/hooks/agent/internal/helpers/exitSynopsis.test.tssrc/__tests__/renderer/hooks/agent/internal/helpers/exitTabCleanup.test.tssrc/__tests__/renderer/hooks/agent/internal/useAgentCommandExitListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentDataListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentErrorListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentExitListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentSessionIdListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentSlashCommandsListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentSshRemoteListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentStderrListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentToolExecutionListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsxsrc/renderer/hooks/agent/internal/helpers/agentErrorLogMatch.tssrc/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.tssrc/renderer/hooks/agent/internal/helpers/errorTitles.tssrc/renderer/hooks/agent/internal/helpers/exitDequeue.tssrc/renderer/hooks/agent/internal/helpers/exitGitRefresh.tssrc/renderer/hooks/agent/internal/helpers/exitSynopsis.tssrc/renderer/hooks/agent/internal/helpers/exitTabCleanup.tssrc/renderer/hooks/agent/internal/types.tssrc/renderer/hooks/agent/internal/useAgentCommandExitListener.tssrc/renderer/hooks/agent/internal/useAgentDataListener.tssrc/renderer/hooks/agent/internal/useAgentErrorListener.tssrc/renderer/hooks/agent/internal/useAgentExitListener.tssrc/renderer/hooks/agent/internal/useAgentSessionIdListener.tssrc/renderer/hooks/agent/internal/useAgentSlashCommandsListener.tssrc/renderer/hooks/agent/internal/useAgentSshRemoteListener.tssrc/renderer/hooks/agent/internal/useAgentStderrListener.tssrc/renderer/hooks/agent/internal/useAgentThinkingListener.tssrc/renderer/hooks/agent/internal/useAgentToolExecutionListener.tssrc/renderer/hooks/agent/internal/useAgentUsageListener.tssrc/renderer/hooks/agent/useAgentListeners.ts
Comment on lines
+172
to
+196
| function persistTabNameAfterSynopsis(synopsisData: SynopsisData): void { | ||
| const persistName = synopsisData.tabName; | ||
| if (!persistName) return; | ||
| const isUuidPrefix = /^[0-9A-F]{8}$/.test(persistName); | ||
| if (isUuidPrefix) return; | ||
| if (!synopsisData.agentSessionId || !synopsisData.projectRoot) return; | ||
|
|
||
| const persistAgentId = synopsisData.toolType || 'claude-code'; | ||
| const persistProjectRoot = synopsisData.projectRoot; | ||
| const persistSessionId = synopsisData.agentSessionId; | ||
|
|
||
| if (persistAgentId === 'claude-code') { | ||
| window.maestro.claude | ||
| .updateSessionName(persistProjectRoot, persistSessionId, persistName) | ||
| .catch((err) => | ||
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | ||
| ); | ||
| } else { | ||
| window.maestro.agentSessions | ||
| .setSessionName(persistAgentId, persistProjectRoot, persistSessionId, persistName) | ||
| .catch((err) => | ||
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the UUID-prefix fallback tab name is generated to confirm casing.
rg -nP --type=ts -C3 'substring\(\s*0\s*,\s*8\s*\)' -g '!**/__tests__/**'
echo '---'
rg -nP --type=ts -C2 '\bgenerateId\b|crypto\.randomUUID|toUpperCase\(\).*substring|substring.*toUpperCase' -g '!**/__tests__/**'Repository: RunMaestro/Maestro
Length of output: 50374
UUID-prefix detection should be case-insensitive to prevent fragile assumptions.
The regex /^[0-9A-F]{8}$/ only matches uppercase hex, but relies on every code path that generates the fallback UUID-prefix to apply .toUpperCase(). While current code does this consistently, the regex itself doesn't enforce it—if a code path forgets .toUpperCase(), the lowercase prefix silently persists as a user-facing name. A case-insensitive regex is a low-effort safeguard:
- const isUuidPrefix = /^[0-9A-F]{8}$/.test(persistName);
+ const isUuidPrefix = /^[0-9a-fA-F]{8}$/.test(persistName);📝 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
| function persistTabNameAfterSynopsis(synopsisData: SynopsisData): void { | |
| const persistName = synopsisData.tabName; | |
| if (!persistName) return; | |
| const isUuidPrefix = /^[0-9A-F]{8}$/.test(persistName); | |
| if (isUuidPrefix) return; | |
| if (!synopsisData.agentSessionId || !synopsisData.projectRoot) return; | |
| const persistAgentId = synopsisData.toolType || 'claude-code'; | |
| const persistProjectRoot = synopsisData.projectRoot; | |
| const persistSessionId = synopsisData.agentSessionId; | |
| if (persistAgentId === 'claude-code') { | |
| window.maestro.claude | |
| .updateSessionName(persistProjectRoot, persistSessionId, persistName) | |
| .catch((err) => | |
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | |
| ); | |
| } else { | |
| window.maestro.agentSessions | |
| .setSessionName(persistAgentId, persistProjectRoot, persistSessionId, persistName) | |
| .catch((err) => | |
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | |
| ); | |
| } | |
| } | |
| function persistTabNameAfterSynopsis(synopsisData: SynopsisData): void { | |
| const persistName = synopsisData.tabName; | |
| if (!persistName) return; | |
| const isUuidPrefix = /^[0-9a-fA-F]{8}$/.test(persistName); | |
| if (isUuidPrefix) return; | |
| if (!synopsisData.agentSessionId || !synopsisData.projectRoot) return; | |
| const persistAgentId = synopsisData.toolType || 'claude-code'; | |
| const persistProjectRoot = synopsisData.projectRoot; | |
| const persistSessionId = synopsisData.agentSessionId; | |
| if (persistAgentId === 'claude-code') { | |
| window.maestro.claude | |
| .updateSessionName(persistProjectRoot, persistSessionId, persistName) | |
| .catch((err) => | |
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | |
| ); | |
| } else { | |
| window.maestro.agentSessions | |
| .setSessionName(persistAgentId, persistProjectRoot, persistSessionId, persistName) | |
| .catch((err) => | |
| logger.warn('[onProcessExit] Failed to persist synopsis tab name', undefined, err) | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/agent/internal/helpers/exitSynopsis.ts` around lines 172 -
196, persistTabNameAfterSynopsis uses a regex that only matches uppercase hex
when detecting UUID-prefixes; change the detection to be case-insensitive so
lowercase prefixes don't slip through. In the persistTabNameAfterSynopsis
function, update the UUID-prefix check (the isUuidPrefix test against
synopsisData.tabName) to use a case-insensitive match (or normalize the string)
so both lowercase and uppercase hex characters are treated the same, leaving the
rest of the logic (persistAgentId, persistProjectRoot, persistSessionId and the
window.maestro.* calls) unchanged.
Comment on lines
+128
to
+130
| const parsed = parseSessionId(sessionId); | ||
| const actualSessionId = parsed.baseSessionId; | ||
| const tabIdFromSession = parsed.tabId ?? undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect parseSessionId's return shape and any parseSessionId result-field usages across the new internal listeners.
fd -t f 'sessionIdParser' --exec cat {}
echo '---'
rg -nP --type=ts -C2 'parsed\.(baseSessionId|actualSessionId|tabId)\b'Repository: RunMaestro/Maestro
Length of output: 17537
Use parsed.actualSessionId instead of parsed.baseSessionId for error tracking.
The parseSessionId() function returns both actualSessionId (the full session ID to use for updates) and baseSessionId (the parent session for synopsis/batch). Using baseSessionId strips important context for synopsis and batch sessions—the actual session IDs would be truncated from 'session-123-synopsis-timestamp' to just 'session-123'.
Compare with useAgentSessionIdListener.ts line 37, which correctly uses parsed.actualSessionId. Align this listener to use the same field for consistency and to preserve full session ID context in error tracking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/agent/internal/useAgentErrorListener.ts` around lines 128
- 130, The listener currently uses parsed.baseSessionId which strips important
context; update the code in useAgentErrorListener to use parsed.actualSessionId
(the full session id returned by parseSessionId) when assigning actualSessionId
for error tracking and related updates, matching the behavior in
useAgentSessionIdListener; keep tabIdFromSession as parsed.tabId ?? undefined
and ensure any downstream references that used actualSessionId continue to
receive the full parsed.actualSessionId value instead of baseSessionId.
Applies the still-valid issues from review of the §4.4 decomposition and skips the rest with rationale below. Behavior fixes: - useAgentExitListener: move activeHiddenToolRef.delete() to AFTER the getActiveProcesses safety check so a false-positive exit event no longer drops bookkeeping for a process that's still alive. Code-quality cleanups: - useAgentSshRemoteListener: drop the redundant try/catch around the git probe IIFE. gitService.isRepo / getBranches / getTags route through createIpcMethod which already swallows IPC errors, reports to Sentry, and returns the configured default value, so the wrapper was dead code that risked masking genuine programmer errors. - useAgentThinkingListener: collapse three near-identical updatedTabs.map() branches into a single map by computing the replacement logs once per chunk (append vs. clean-continuation vs. malformed-continuation). Behavior unchanged; ~30 lines lighter. - useAgentUsageListener: replace the magic "-5" buffer with a named constant ESTIMATED_USAGE_YELLOW_GAP_PCT and document why we keep estimates below the user's yellow threshold. - autorunSynopsisPrompt: replace `result.content!` with `result.content ?? ''` so cachedAutorunSynopsisPrompt always satisfies the `string | null` type contract even if the IPC layer ever violates the success-implies-content invariant. Test improvements: - useAgentSshRemoteListener.test: the "same remote already attached" case had zero assertions (vacuous pass). Now sets isGitRepo=true and asserts the git probe functions were not called. - useAgentThinkingListener.test: the RAF cleanup test only checked cancelAnimationFrame was invoked. The mock is now cancellable (id -> callback Map; cancelAnimationFrame removes from the queue), and the test additionally calls flushRaf() AFTER unmount and asserts no logs were appended — proves the cancelled callback truly cannot fire. - useAgentUsageListener.test: the "routes usage updates" test only checked that updateUsage was called. Now uses toHaveBeenNthCalledWith for both the per-tab and per-session calls; added a second test that asserts ai-tab-format ids carry the tabId on call #1 and null on call #2. - exitSynopsis.test: the UUID-prefix test only covered uppercase. New positive test pins the case-sensitive contract by asserting that lowercase 8-hex tab names ARE persisted (the auto-generated fallback is always uppercase, so lowercase can only have been user-typed). Skipped (with reason): - "Make UUID-prefix regex case-insensitive" — the only fallback path uses `agentSessionId.substring(0, 8).toUpperCase()`, so lowercase 8-hex names can only have come from the user typing them. Making the regex case-insensitive would silently drop those custom names. - "Use parsed.actualSessionId instead of parsed.baseSessionId in useAgentErrorListener" — the original (pre-decomposition) code used baseSessionId intentionally to attribute batch-suffix errors to the user-visible session. Changing would break that attribution. The "match useAgentSessionIdListener" claim is misleading because that listener bails on isBatchSession() before parsing. - "Re-throw on getActiveProcesses safety-check failure" — pre-refactor code swallowed too; the current logger.error path is not silent (forwards to main with full context). Re-throwing would leave the agent's tab in a permanently busy state on transient IPC failures for very limited Sentry value. - "Move IPC + flag-mutation outside useAgentSessionIdListener reducer" — the targetTab/isClaudeCode lookup IS the bulk of the reducer body, so hoisting would force a duplicate pass. The IPC is idempotent (no harm if double-fired in StrictMode dev). Out of minimal-change scope. Validation: 179 / 179 listener tests passing (was 177); 27,722 / 108 skipped / 0 failing across the full suite (+2 from the new lowercase UUID and ai-tab routing tests). Zero type errors, zero lint errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/hooks/agent/internal/useAgentSshRemoteListener.ts`:
- Around line 50-85: The git probe runs unconditionally when sshRemote?.id
exists, causing redundant IPC calls even if the remote didn't change; fix by
gating the probe on an actual remote change: inside useAgentSshRemoteListener
where you locate the session (getSessions().find(s => s.id ===
actualSessionId)), capture the session's existing remote id
(session.sessionSshRemoteConfig?.id or similar) and only start the async probe
(the void async IIFE that calls gitService.isRepo / getBranches / getTags and
later calls setSessions) if that existingRemoteId !== sshRemote.id (or if
existingRemoteId is undefined and sshRemote.id is defined), otherwise skip the
probe; keep the existing setSessions shortcircuit (currentRemoteId ===
newRemoteId) as-is and reference actualSessionId, sshRemote.id, getSessions,
gitService.isRepo, gitService.getBranches, gitService.getTags, and setSessions
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 208956d8-f463-4e9d-9dbf-8529fc537f9f
📒 Files selected for processing (9)
src/__tests__/renderer/hooks/agent/internal/helpers/exitSynopsis.test.tssrc/__tests__/renderer/hooks/agent/internal/useAgentSshRemoteListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsxsrc/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsxsrc/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.tssrc/renderer/hooks/agent/internal/useAgentExitListener.tssrc/renderer/hooks/agent/internal/useAgentSshRemoteListener.tssrc/renderer/hooks/agent/internal/useAgentThinkingListener.tssrc/renderer/hooks/agent/internal/useAgentUsageListener.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/tests/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx
- src/renderer/hooks/agent/internal/useAgentUsageListener.ts
- src/renderer/hooks/agent/internal/helpers/autorunSynopsisPrompt.ts
- src/tests/renderer/hooks/agent/internal/useAgentSshRemoteListener.test.tsx
- src/renderer/hooks/agent/internal/useAgentThinkingListener.ts
- src/tests/renderer/hooks/agent/internal/helpers/exitSynopsis.test.ts
- src/tests/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsx
- src/renderer/hooks/agent/internal/useAgentExitListener.ts
The probe block in useAgentSshRemoteListener fired on every onSshRemote
event whose `sshRemote.id` was set and whose target session wasn't yet
flagged as a git repo. Reconnects and duplicate IPC events for the same
remote therefore burned 3 IPC roundtrips (isRepo + getBranches + getTags)
for nothing — and could even spawn concurrent probes if the first hadn't
flipped `isGitRepo` to true yet.
Fix: snapshot the previously-attached remote id from the store BEFORE
the setSessions call (after, Zustand has synchronously written the new
value, so we'd have no way to tell a fresh attach from a duplicate
event), and add `previousSshRemoteId !== sshRemote.id` to the probe gate.
Test added: "does NOT re-probe on a duplicate event for the SAME remote
(e.g. reconnect/replay)" — seeds a session with `sshRemote: { id: 'r-1' }`
and `isGitRepo: false`, dispatches the same `r-1` remote again, and
asserts all three gitService.* probes were skipped.
180 / 180 listener tests passing (+1); 27,723 / 108 skipped / 0 failing
across the full suite.
No behaviour change. Captures the lint-staged pass that ran after the previous commit.
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
Splits the IPC-listener god-hook in
src/renderer/hooks/agent/useAgentListeners.tsper real seam — one hook perwindow.maestro.process.onXxxchannel — and lifts the sharedactiveHiddenToolRefinto the coordinator before splitting consumers. Pure logic fromonExit(queue dequeue, git refresh, synopsis spawn, eligibility predicate) is extracted as named helpers ininternal/helpers/exit*.ts.This is the last open P0 in the Tier 4 backlog and follows the precedent set by §4.3 useBatchProcessor (2,281 → 328, −86%) and §4.2 ProcessMonitor (2,031 → 280, −86%).
useAgentListeners.ts1,977 → 104 LOC (−95%).useAgentListenersand its internals.useAgentListeners,loadAgentListenersPrompts,getAutorunSynopsisPrompt,getErrorTitleForType,BatchedUpdater,UseAgentListenersDepsall re-exported from the shell.App.tsx,services/promptInit.ts,hooks/agent/index.tscall sites are untouched.Final shape
Key decisions
onCommandExit + onUsageinto a "status" hook would couple a shell-exit-code listener to a token-counter listener with zero shared logic; groupingonData + onStderrwould force two unrelated guard tables into one file. Per-channel = one file = one cleanup path = one test file. Mirrors the §4.3 deviation from the speculative shape.activeHiddenToolRefto the coordinator before splitting consumers (3 readers/writers acrossonData,onExit,onAgentError). Same atomic-claim hoisting pattern that landed in §4.3 (batchFlushState.ts) — centralise the subtle invariant first, then split the consumers around it. Thinking RAF refs (thinkingChunkBufferRef,thinkingChunkRafIdRef) stay encapsulated inuseAgentThinkingListenersince only that listener uses them.onExitreducer kept inline. The 280-linesetSessions(prev => prev.map(s => { ... }))body is too entangled with closure variables to extract safely; the 67-test integration suite already exercises it deeply. Extracted the surrounding side-effects (queue dequeue, git refresh, synopsis spawn, eligibility predicate) as pure helpers instead — same SRP win, much lower regression risk.useEffects in declaration order; this preserves any cross-listener event timing the existing test suite depends on.loadAgentListenersPrompts+getAutorunSynopsisPromptmoved intohelpers/autorunSynopsisPrompt.ts. The shell re-exports both soservices/promptInit.tsis untouched.Performance wins (in passing)
onSlashCommands,onStderrskip-by-format,onCommandExit,onUsage,onSshRemote,onToolExecution) — skips the no-opprev.map()allocation and Zustand subscriber notifications when the target session has been deleted (common during session-delete races).activeHiddenToolRefMap on unmount so any orphan tool entries are released for GC.Skipped (not real wins): wholesale
setSessions → updateSessionWithmigration (sugar over the sameprev.map), pause-on-hidden (would drop incoming agent output, unsafe).Tests
useAgentListeners.test.tsintegration suiteuseAgentListeners+ internals coverageNew unit tests cover each per-channel hook in isolation (subscribe/unsubscribe, primary handler path, edge cases, orphan early-return) plus 8 helper modules. The biggest single addition is
helpers/exitSynopsis.test.ts(15 tests covering eligibility quadrants, prompt building, history recording, name persistence for both claude-code and non-claude-code IPC paths).Test plan
npm run test— full suite green (27,720 / 0 failing)npx tsc --noEmit --pretty— zero type errorsnpx eslint 'src/renderer/hooks/agent/**/*.{ts,tsx}'— zero lint errorsSummary by CodeRabbit
Tests
New Features
Refactor