Open
Conversation
Prevent local Claude Code permission settings from being committed to the public repository. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-participant autorun parsing/tracking and timeouts, streams raw process stdout as per-participant live output, introduces stop-all and autorun-complete IPC paths and emitters, and wires renderer stores, hooks, UI, and a registry to start/cancel/report autorun batch runs. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Renderer (App / UI)
participant S as Main (group-chat-router / IPC)
participant P as ProcessManager / Worker
participant Store as Renderer Store
R->>S: send moderator message (may include !autorun ...)
S->>S: extractAutoRunDirectives -> emit autoRunTriggered(groupChatId, participant, filename?)
S-->>R: autoRunTriggered event
R->>R: registerGroupChatAutoRun(sessionId, groupChatId, participant)
R->>P: start batch run (spawn processes)
P->>S: emit raw-stdout(sessionId, chunk)
S->>S: resolve participant from chunk -> emit participantLiveOutput(groupChatId, participant, chunk)
S-->>R: participantLiveOutput event
R->>Store: appendParticipantLiveOutput(participant, chunk)
P-->>R: batch completes
R->>S: reportAutoRunComplete(groupChatId, participant, summary)
S->>S: routeAgentResponse / markParticipantResponded / spawnModeratorSynthesis?
S-->>R: autoRunBatchComplete(groupChatId, participant)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR substantially extends the group chat feature with autorun orchestration (moderator-driven Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Renderer
participant MainProcess as Main Process (IPC)
participant Router as group-chat-router
participant BatchProc as useBatchProcessor
User->>Renderer: Send message
Renderer->>MainProcess: groupChat:sendToModerator
MainProcess->>Router: routeUserMessage → spawns moderator process
Router-->>Renderer: emitStateChange("agent-working")
Note over Router: Moderator responds with @mentions / !autorun
alt Regular @mention participant
Router->>Router: spawns GROUP_CHAT_PREFIX process
Router->>Router: setParticipantResponseTimeout (10 min)
Router-->>Renderer: emitParticipantState("working")
Note over Router: Process exits → exit-listener fires
Router->>Router: routeAgentResponse → appendToLog
Router->>Router: markParticipantResponded
Router->>Router: [if last] spawnModeratorSynthesis
else !autorun @AgentName directive
Router-->>Renderer: emitAutoRunTriggered(groupChatId, name, file?)
Router->>Router: setParticipantResponseTimeout (10 min)
Renderer->>BatchProc: startBatchRun(sessionId, config)
BatchProc-->>Renderer: onComplete(info)
Renderer->>MainProcess: groupChat:reportAutoRunComplete(summary)
MainProcess->>Router: routeAgentResponse → appendToLog
MainProcess->>Router: markParticipantResponded (cancels timeout)
Router-->>Renderer: emitAutoRunBatchComplete
Router->>Router: [if last] spawnModeratorSynthesis
end
Router-->>Renderer: Synthesis result → emitMessage (moderator)
Router-->>Renderer: emitStateChange("idle")
Last reviewed commit: ae7950d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/__tests__/main/ipc/handlers/groupChat.test.ts (1)
172-173: Add behavior tests for the two new IPC channels.This only proves the handler names were registered. It won't catch
groupChat:stopAllorgroupChat:reportAutoRunCompletecalling the wrong dependency or sending the wrong payload. Please add focused tests for those handlers alongside the existing moderator/participant cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/groupChat.test.ts` around lines 172 - 173, Add focused behavior tests that invoke the registered IPC handlers 'groupChat:stopAll' and 'groupChat:reportAutoRunComplete' (not just assert registration): mock/spy the concrete dependencies those handlers call (the service/method that stops chats and the reporter/method that logs auto-run completion), trigger the handler via the same test harness used for moderator/participant cases, and assert the correct dependency was called with the expected payload and that the handler returns or sends the expected result; mirror the structure of the existing moderator/participant tests so you cover both happy paths and at least one error/edge case for each new handler.src/__tests__/renderer/components/GroupChatHeader.test.tsx (1)
27-31: LGTM on the mock addition.The StopCircle mock follows the established pattern for icon mocks in this file.
However, consider adding tests for the new
stateandonStopAllprops that were added toGroupChatHeader. The currentdefaultProps(lines 48-57) doesn't include these required props, which may cause test failures. Tests should verify:
- Stop All button renders when
state !== 'idle'- Stop All button is hidden when
state === 'idle'onStopAllcallback is invoked on click🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/GroupChatHeader.test.tsx` around lines 27 - 31, Add tests for the new GroupChatHeader props: update the test suite around GroupChatHeader (use the existing defaultProps constant in the test file) to include the required state and onStopAll props; add three tests: (1) render with state !== 'idle' (e.g., 'running') and assert the "Stop All" button is present, (2) render with state === 'idle' and assert the "Stop All" button is not present, and (3) render with a mock onStopAll function, click the "Stop All" button and assert the mock was called. Ensure the tests reference the GroupChatHeader component and the defaultProps pattern used in this test file so props are supplied consistently.src/main/ipc/handlers/groupChat.ts (1)
515-516: Consider hoisting the dynamic import to a top-level import.The dynamic
import()inside the handler works but is inconsistent with the rest of the file whererouteUserMessageandclearPendingParticipantsare imported at the top level. This could cause subtle timing issues and makes the code harder to follow.♻️ Proposed refactor
// Group chat router imports -import { routeUserMessage, clearPendingParticipants } from '../../group-chat/group-chat-router'; +import { + routeUserMessage, + clearPendingParticipants, + routeAgentResponse, + markParticipantResponded, + spawnModeratorSynthesis, +} from '../../group-chat/group-chat-router';Then update the handler:
- const { routeAgentResponse, markParticipantResponded, spawnModeratorSynthesis } = - await import('../../group-chat/group-chat-router'); - // Log the autorun summary as the participant's response await routeAgentResponse(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/groupChat.ts` around lines 515 - 516, Hoist the dynamic import by adding a top-level import for routeAgentResponse, markParticipantResponded, and spawnModeratorSynthesis from '../../group-chat/group-chat-router' (instead of using await import inside the handler), then remove the await import call in the handler and use the imported symbols directly; ensure you update any existing local references in the handler that used the dynamically-loaded names so they reference the top-level imports (routeAgentResponse, markParticipantResponded, spawnModeratorSynthesis) to keep imports consistent with routeUserMessage and clearPendingParticipants.src/renderer/components/ParticipantCard.tsx (1)
259-272: Consider addingaria-expandedfor accessibility.The peek button toggles visibility of the live output panel. Adding
aria-expandedandaria-controlswould improve screen reader support.♿ Proposed accessibility improvement
<button onClick={() => setPeekOpen((v) => !v)} className="flex items-center gap-0.5 text-[10px] px-1.5 py-0.5 rounded shrink-0 hover:opacity-80 transition-opacity cursor-pointer" style={{ backgroundColor: peekOpen ? `${theme.colors.accent}25` : `${theme.colors.accent}10`, color: peekOpen ? theme.colors.accent : theme.colors.textDim, border: `1px solid ${peekOpen ? theme.colors.accent + '60' : theme.colors.border}`, }} title="Peek at live output" + aria-expanded={peekOpen} + aria-controls={`live-output-${participant.name}`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ParticipantCard.tsx` around lines 259 - 272, The Peek button in ParticipantCard (the button that calls setPeekOpen and reads peekOpen) should expose its state to assistive tech by adding aria-expanded={peekOpen} and aria-controls referencing the live output panel's id; update the live output panel element to have a matching id (e.g., "peek-panel-<uniqueParticipantId>" or another stable identifier) so the button's aria-controls points to it, leaving the existing onClick and visual logic unchanged.src/main/group-chat/group-chat-router.ts (1)
170-172: Consider logging the error in the catch block.The empty catch block silently swallows errors when appending the timeout message to the log. While non-critical, logging these errors aids debugging.
🔧 Suggested improvement
} catch { - // Non-critical — synthesize anyway + // Non-critical — synthesize anyway, but log for debugging + console.warn(`[GroupChat:Debug] Failed to append timeout log for ${participantName}:`, err); }Note: You'll need to capture the error:
} catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 170 - 172, The silent catch that swallows errors when appending the timeout message should capture and log the error instead: change the empty catch to catch (err) and call the appropriate logger (e.g., processLogger.warn or console.warn) with a clear message like "Failed to append timeout message to log" plus the error details; keep the non-critical flow (do not rethrow) so behavior remains unchanged. Target the catch block around the timeout append logic in group-chat-router (the block that currently reads "} catch { // Non-critical — synthesize anyway }") and ensure the logged output includes the caught error object for debugging.
🤖 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/main/process-listeners/exit-listener.ts`:
- Around line 246-252: Replace the user-facing message that currently
interpolates String(err) in groupChatEmitters.emitMessage with a generic retry
notice (e.g., "⚠️ Synthesis failed. You can send another message to continue.")
and do not expose internal error details in the chat transcript; instead, log
the full error and report it to Sentry by calling processLogger.error(...) with
the error and context and invoking Sentry.captureException(err) (or your
project's Sentry helper) so the detailed exception is recorded for debugging
while the chat only shows the generic message; keep the existing emitStateChange
call and use the same err variable when logging/reporting.
In `@src/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 430-434: Wrap the best-effort this.emitter.emit('raw-stdout',
sessionId, output) call in its own try/catch so any synchronous listener
exception cannot prevent this.stdoutHandler.handleData(sessionId, output) from
running; on catch, log the error and report it via the project's Sentry
utilities (e.g., Sentry.captureException or the repo's equivalent) with context
(sessionId, 'raw-stdout'), but do not rethrow so stdoutHandler still processes
the chunk. Ensure the change is applied in the ChildProcessSpawner stdout data
handler where emitter.emit and stdoutHandler.handleData are called.
In `@src/prompts/group-chat-participant-request.md`:
- Line 19: The prompt template uses the {{WORKTREE_BASE_PATH}} placeholder but
group-chat-router.ts currently replaces it with an empty string; find the
replacement in src/main/group-chat/group-chat-router.ts and stop hardcoding
''—instead pass the real worktree base path from the request/context into the
template renderer (e.g., use the existing worktreeBasePath or config variable
used elsewhere), ensuring the function that constructs participant prompts (the
handler that currently does the replace) forwards that value into the template
so participant prompts receive the correct worktree-path context.
In `@src/renderer/components/GroupChatPanel.tsx`:
- Line 33: The prop onStopAll on GroupChatPanel is currently optional and
defaulted to a no-op, which masks missing wiring and renders the stop action
nonfunctional; update GroupChatPanel to either require onStopAll (make its type
non-optional) or stop providing the default (() => {}) so the prop value can be
undefined and GroupChatHeader can hide the action when absent—locate the
onStopAll declaration in GroupChatPanel and remove the default no-op (or change
its type to mandatory), and ensure any parent components wiring this prop pass
the actual handler or omit it intentionally.
In `@src/renderer/components/GroupChatRightPanel.tsx`:
- Around line 84-85: participantLiveOutput is being keyed only by
participant.name which can collide across chats; update the lookup and any
set/write paths to use a chat-scoped key (for example groupChatId or
participant.sessionId) instead of plain participant.name. Locate uses of
participantLiveOutput.get(...) and the code that writes into
participantLiveOutput in GroupChatRightPanel (and related handlers) and change
them to compute a composite key (e.g.,
`${groupChatId}:${participant.sessionId}`) or directly use
participant.sessionId/groupChatId as the map key so the panel only shows live
output for the current chat's participant.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 72-73: The async handler handleStopAll currently returns
Promise<void> but is invoked synchronously by GroupChatHeader.tsx via onStopAll,
risking unhandled promise rejections; wrap the entire implementation of
handleStopAll in a try/catch, catch any thrown/rejected errors, log or report
them (using the existing logger or console.error), and do not rethrow so the
function resolves to void when called without awaiting; keep the exported name
handleStopAll unchanged so consumers (onStopAll) can call it without causing
unhandled rejections.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/groupChat.test.ts`:
- Around line 172-173: Add focused behavior tests that invoke the registered IPC
handlers 'groupChat:stopAll' and 'groupChat:reportAutoRunComplete' (not just
assert registration): mock/spy the concrete dependencies those handlers call
(the service/method that stops chats and the reporter/method that logs auto-run
completion), trigger the handler via the same test harness used for
moderator/participant cases, and assert the correct dependency was called with
the expected payload and that the handler returns or sends the expected result;
mirror the structure of the existing moderator/participant tests so you cover
both happy paths and at least one error/edge case for each new handler.
In `@src/__tests__/renderer/components/GroupChatHeader.test.tsx`:
- Around line 27-31: Add tests for the new GroupChatHeader props: update the
test suite around GroupChatHeader (use the existing defaultProps constant in the
test file) to include the required state and onStopAll props; add three tests:
(1) render with state !== 'idle' (e.g., 'running') and assert the "Stop All"
button is present, (2) render with state === 'idle' and assert the "Stop All"
button is not present, and (3) render with a mock onStopAll function, click the
"Stop All" button and assert the mock was called. Ensure the tests reference the
GroupChatHeader component and the defaultProps pattern used in this test file so
props are supplied consistently.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 170-172: The silent catch that swallows errors when appending the
timeout message should capture and log the error instead: change the empty catch
to catch (err) and call the appropriate logger (e.g., processLogger.warn or
console.warn) with a clear message like "Failed to append timeout message to
log" plus the error details; keep the non-critical flow (do not rethrow) so
behavior remains unchanged. Target the catch block around the timeout append
logic in group-chat-router (the block that currently reads "} catch { //
Non-critical — synthesize anyway }") and ensure the logged output includes the
caught error object for debugging.
In `@src/main/ipc/handlers/groupChat.ts`:
- Around line 515-516: Hoist the dynamic import by adding a top-level import for
routeAgentResponse, markParticipantResponded, and spawnModeratorSynthesis from
'../../group-chat/group-chat-router' (instead of using await import inside the
handler), then remove the await import call in the handler and use the imported
symbols directly; ensure you update any existing local references in the handler
that used the dynamically-loaded names so they reference the top-level imports
(routeAgentResponse, markParticipantResponded, spawnModeratorSynthesis) to keep
imports consistent with routeUserMessage and clearPendingParticipants.
In `@src/renderer/components/ParticipantCard.tsx`:
- Around line 259-272: The Peek button in ParticipantCard (the button that calls
setPeekOpen and reads peekOpen) should expose its state to assistive tech by
adding aria-expanded={peekOpen} and aria-controls referencing the live output
panel's id; update the live output panel element to have a matching id (e.g.,
"peek-panel-<uniqueParticipantId>" or another stable identifier) so the button's
aria-controls points to it, leaving the existing onClick and visual logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b763724-0e04-4124-b99e-08526d690db4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.gitignoresrc/__tests__/main/agents/session-storage.test.tssrc/__tests__/main/ipc/handlers/groupChat.test.tssrc/__tests__/renderer/components/GroupChatHeader.test.tsxsrc/main/group-chat/group-chat-router.tssrc/main/index.tssrc/main/ipc/handlers/groupChat.tssrc/main/preload/groupChat.tssrc/main/process-listeners/data-listener.tssrc/main/process-listeners/exit-listener.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/prompts/group-chat-moderator-system.mdsrc/prompts/group-chat-participant-request.mdsrc/renderer/App.tsxsrc/renderer/components/GroupChatHeader.tsxsrc/renderer/components/GroupChatList.tsxsrc/renderer/components/GroupChatPanel.tsxsrc/renderer/components/GroupChatParticipants.tsxsrc/renderer/components/GroupChatRightPanel.tsxsrc/renderer/components/ParticipantCard.tsxsrc/renderer/components/Settings/SettingsModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/batch/useBatchHandlers.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/groupChatStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/utils/groupChatAutoRunRegistry.ts
Author
|
I'll work the greptile callouts. For context, this PR is from a branch I've been daily driving for a bit. I'm obviously heavily focused on Group Chat and having the moderator coordinate participant activity. I layered in an ability for each participant to work through Maestro's excellent autorun doc facilities as their SOP. This works well for my typical workflow. No hard feelings if you want to reject this - but it's been super useful for me so wanted to share if there's interest. |
Author
|
@greptileai review latest changes |
- Protect raw-stdout emit from breaking stdout handling (try/catch) - Sanitize error messages in chat transcript (Sentry instead of String(err)) - Scope live-output lookup by groupChatId:participantName composite key - Remove dead WORKTREE_BASE_PATH template variable - Hoist dynamic imports to top-level in groupChat IPC handler - Deduplicate extractAutoRunDirectives call in router - Make onStopAll required prop, add async error handling - stopAll now cancels in-flight autorun batch runs via registry - Report failure instead of silent fallback when autorun target file missing - Add behavior tests for stopAll and reportAutoRunComplete IPC handlers - Add tests for GroupChatHeader state/stop props Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/groupChat.test.ts`:
- Around line 1068-1071: The test mocks for
groupChatRouter.markParticipantResponded are returning an object but the real
API returns a boolean; change the mocks to return boolean values
(mockReturnValue(false) and mockReturnValue(true) where applicable) for
groupChatRouter.markParticipantResponded, update the two places that currently
return an object to return plain booleans, and then adjust the assertions to
validate spawn behavior based on the boolean (i.e., assert that synthesis is not
spawned when false and is spawned when true).
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 1239-1248: The finalization branch currently checks only
mentionsToSpawn.length and autoRunParticipants.length, which can be non-zero
despite no actionable work; change the condition to use the actual actionable
set (participantsToRespond) so cleanup runs when there is no work: replace the
check `mentionsToSpawn.length === 0 && autoRunParticipants.length === 0` with
`mentionsToSpawn.length === 0 && participantsToRespond.size === 0` (and apply
the same change to the other similar block later), ensuring you still call
groupChatEmitters.emitStateChange?.(groupChatId, 'idle') and
powerManager.removeBlockReason(`groupchat:${groupChatId}`) when idle.
- Around line 177-179: The timeout path is unconditionally calling
groupChatEmitters.emitAutoRunBatchComplete which causes auto-run completion to
fire for non-autorun participants; change the call site so
emitAutoRunBatchComplete is only invoked for participants that are tracked as
autorun runs (e.g., check the autorun-tracking structure or a participant flag
such as participant.meta?.isAutoRun or
autorunTrackedParticipants.has(participantName) before calling it). After
emitting, clear the participant from the autorun tracking so it won't fire
again. Keep the existing emitParticipantState call unconditional.
- Around line 170-172: The empty catch in group-chat-router.ts that swallows
failures when writing timeout markers should be replaced with a proper error
handler: catch the error as "err", call processLogger.error with a descriptive
message and relevant context (e.g., chatId, userId, traceId or request id), call
Sentry.captureException(err) (or your app's telemetry/reporting helper) to
ensure it is reported, and then rethrow the error so it bubbles up; update the
catch block around the timeout-marker write to use these calls and include the
unique symbols/processLogger and Sentry.captureException in the change.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 237-245: The handler registered with
window.maestro.groupChat.onAutoRunBatchComplete currently resolves the session
via participantName (useSessionStore.getState().sessions.find(...)) which can
collide across chats and ignores the provided _groupChatId; instead look up the
sessionId from a group-chat-scoped autorun registry keyed by _groupChatId (or
maintain a map from groupChatId -> sessionId created when autoruns start) and
dispatch the COMPLETE_BATCH using that sessionId via
useBatchStore.getState().dispatchBatch; update the onAutoRunBatchComplete
callback to use the group-scoped registry (not global name lookup) and fall back
to participantName only if the registry entry is missing.
- Around line 620-622: The catch in handleStopAll currently only console.errors
the failure and swallows it; replace that with a recoverable UI failure and
proper error reporting: when catching the error from the IPC stop call in
handleStopAll, call the app's toast/notification helper (e.g., showToast or
notify) with a user-friendly message like "Failed to stop all conversations" and
include a brief action if available, and also pass the error to the centralized
error reporter (e.g., reportError or captureException) with context { location:
'handleStopAll', ipc: 'stopAll' } or similar; remove the bare console.error and
ensure the catch handles known error codes explicitly if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c171e9c6-8a2c-4872-8747-761ff4b15eac
📒 Files selected for processing (11)
src/__tests__/main/ipc/handlers/groupChat.test.tssrc/__tests__/renderer/components/GroupChatHeader.test.tsxsrc/main/group-chat/group-chat-router.tssrc/main/ipc/handlers/groupChat.tssrc/main/process-listeners/exit-listener.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/renderer/App.tsxsrc/renderer/components/GroupChatPanel.tsxsrc/renderer/components/GroupChatRightPanel.tsxsrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/utils/groupChatAutoRunRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/utils/groupChatAutoRunRegistry.ts
- src/main/process-manager/spawners/ChildProcessSpawner.ts
- src/tests/renderer/components/GroupChatHeader.test.tsx
Contributor
|
@ets please run the following on your machine for this PR so it can move toward acceptance. Here is the prompt you should run: |
- Fix mock return type for markParticipantResponded (returns boolean, not object) - Add logging and Sentry reporting to timeout-path catch block - Gate emitAutoRunBatchComplete to autorun-tracked participants only - Use participantsToRespond.size for finalization check instead of raw directive count - Scope batch completion resolution to group chat via autorun registry - Surface handleStopAll failures to user via toast notification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/group-chat/group-chat-router.ts (2)
257-286:⚠️ Potential issue | 🟠 MajorClear autorun tracker state on normal completion and reset.
autoRunParticipantTrackeris only pruned in the timeout path. After a successfulreportAutoRunComplete()orstopAll(), the stale entry survives, so a later ordinary timeout for the same participant/chat can still emit an unrelatedemitAutoRunBatchComplete().Suggested fix
export function clearPendingParticipants(groupChatId: string): void { // Cancel all timeouts for this chat before clearing const pending = pendingParticipantResponses.get(groupChatId); if (pending) { for (const name of pending) { clearParticipantResponseTimeout(groupChatId, name); } } pendingParticipantResponses.delete(groupChatId); + autoRunParticipantTracker.delete(groupChatId); } @@ export function markParticipantResponded(groupChatId: string, participantName: string): boolean { clearParticipantResponseTimeout(groupChatId, participantName); + + const autoRunSet = autoRunParticipantTracker.get(groupChatId); + if (autoRunSet?.delete(participantName) && autoRunSet.size === 0) { + autoRunParticipantTracker.delete(groupChatId); + } const pending = pendingParticipantResponses.get(groupChatId); if (!pending) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 257 - 286, autoRunParticipantTracker entries are only removed on timeout, leaving stale entries after normal completion paths; update reportAutoRunComplete() and stopAll() to remove the corresponding key from autoRunParticipantTracker when a run finishes or is reset so later timeouts don't emit spurious emitAutoRunBatchComplete(). Locate autoRunParticipantTracker usage and add a delete for the groupChatId (or participant-specific key used) in both reportAutoRunComplete() and stopAll() (and any other normal-completion code paths), ensuring consistency with how timeouts currently prune the tracker.
1520-1608:⚠️ Potential issue | 🟠 MajorModerator synthesis still bypasses SSH wrapping.
routeUserMessage()already routes moderator spawns throughwrapSpawnWithSsh(), but this synthesis path still goes straight toprocessManager.spawn(). Any chat withmoderatorConfig.sshRemoteConfigwill execute the follow-up synthesis locally instead of on the configured remote.As per coding guidelines, "When implementing features that spawn agent processes (context grooming, group chat, batch operations), you MUST support SSH remote execution."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 1520 - 1608, Synthesis spawn bypasses SSH wrapping by calling processManager.spawn() directly; update the synthesis path to use the same SSH wrapping as routeUserMessage() by calling wrapSpawnWithSsh(...) with the original command, finalArgs, winConfig (shell/runInShell/sendPromptViaStdin flags) and chat.moderatorConfig?.sshRemoteConfig, then pass the wrapped command/args into processManager.spawn instead of the unwrapped values; ensure you preserve prompt, readOnlyMode, customEnvVars, promptArgs, contextWindow and other spawn options when replacing the direct call so synthesis runs on the configured remote.
♻️ Duplicate comments (2)
src/renderer/hooks/groupChat/useGroupChatHandlers.ts (1)
633-640:⚠️ Potential issue | 🟠 MajorReport stop-all failures explicitly.
The toast is good, but this catch still turns every failure into
console.error+ UI feedback and drops the error from telemetry. Please capture the exception withactiveGroupChatId/groupChat:stopAllcontext, and only special-case known recoverable error codes here.As per coding guidelines, "For expected/recoverable errors, catch them explicitly, check the error code, and handle gracefully." and "Use Sentry utilities for explicit reporting."
🤖 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 633 - 640, In the catch block inside useGroupChatHandlers.ts for the stop-all flow, replace the blanket console.error + toast by explicitly reporting the exception to Sentry (use your project Sentry utility, e.g., Sentry.captureException or the app's report function) with context/tags { operation: 'groupChat:stopAll', activeGroupChatId } and include the original error; then special-case known recoverable errors by checking error.code (handle those without reporting or with a lower-severity log and keep the UI toast), and only send telemetry for unexpected failures while still showing the same user-facing toast on error.src/__tests__/main/ipc/handlers/groupChat.test.ts (1)
1065-1094:⚠️ Potential issue | 🟡 MinorAssert the synthesis branch, not just the mock return type.
These cases still pass if
reportAutoRunCompletealways skips synthesis or always triggers it. Add explicitspawnModeratorSynthesisexpectations for thefalseandtruebranches so the boolean contract is actually covered.Suggested test assertions
expect(groupChatRouter.routeAgentResponse).toHaveBeenCalledWith( 'gc-autorun', 'Worker 1', 'Task completed successfully', mockProcessManager ); + expect(groupChatRouter.spawnModeratorSynthesis).not.toHaveBeenCalled(); @@ expect(groupChatRouter.markParticipantResponded).toHaveBeenCalledWith( 'gc-autorun-done', 'Worker 1' ); + expect(groupChatRouter.spawnModeratorSynthesis).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/groupChat.test.ts` around lines 1065 - 1094, The tests for the "groupChat:reportAutoRunComplete" handler currently assert routeAgentResponse and markParticipantResponded but don't assert the synthesis branch; update the two cases to explicitly assert spawnModeratorSynthesis behavior: in the first test (when markParticipantResponded returns false) add an expectation that groupChatRouter.spawnModeratorSynthesis was not called, and in the second test (when markParticipantResponded returns true) add an expectation that groupChatRouter.spawnModeratorSynthesis was called (and optionally called with the expected gc id and mockProcessManager) so both boolean branches of markParticipantResponded are actually validated; locate the handler via handlers.get('groupChat:reportAutoRunComplete') and the router mocks groupChatRouter.routeAgentResponse, groupChatRouter.markParticipantResponded and groupChatRouter.spawnModeratorSynthesis to modify the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 204-209: The console-only catch for spawnModeratorSynthesis should
instead report the error to Sentry and perform the same cleanup the successful
path would do so the power manager lock is cleared; replace the current .catch
block on spawnModeratorSynthesis(groupChatId, processManager, agentDetector) to
call the project's Sentry reporting util (e.g., reportExceptionToSentry or
Sentry.captureException) with the error, then call
groupChatEmitters.emitStateChange?.(groupChatId, 'idle') and explicitly clear
any power/process lock for this chat (use processManager or powerManager helper
to release/unblock the key `groupchat:${groupChatId}` or call the existing
cleanup helper used by spawnModeratorSynthesis), ensuring no errors are
swallowed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 237-259: The callback currently completes every autorun session
from getAutoRunSessionsForGroupChat(groupChatId) when any participant emits
completion; instead, resolve a single session by matching participantName to a
session id inside that registry: when autoRunSessionIds.length > 0, lookup
useSessionStore.getState().sessions to find the session whose id is in
autoRunSessionIds and whose name === participantName and call
useBatchStore.getState().dispatchBatch({ type: 'COMPLETE_BATCH', sessionId })
only for that session (if found); keep the existing participantName-only lookup
(using useSessionStore.sessions.find(s => s.name === participantName)) as the
fallback branch when autoRunSessionIds is empty.
---
Outside diff comments:
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 257-286: autoRunParticipantTracker entries are only removed on
timeout, leaving stale entries after normal completion paths; update
reportAutoRunComplete() and stopAll() to remove the corresponding key from
autoRunParticipantTracker when a run finishes or is reset so later timeouts
don't emit spurious emitAutoRunBatchComplete(). Locate autoRunParticipantTracker
usage and add a delete for the groupChatId (or participant-specific key used) in
both reportAutoRunComplete() and stopAll() (and any other normal-completion code
paths), ensuring consistency with how timeouts currently prune the tracker.
- Around line 1520-1608: Synthesis spawn bypasses SSH wrapping by calling
processManager.spawn() directly; update the synthesis path to use the same SSH
wrapping as routeUserMessage() by calling wrapSpawnWithSsh(...) with the
original command, finalArgs, winConfig (shell/runInShell/sendPromptViaStdin
flags) and chat.moderatorConfig?.sshRemoteConfig, then pass the wrapped
command/args into processManager.spawn instead of the unwrapped values; ensure
you preserve prompt, readOnlyMode, customEnvVars, promptArgs, contextWindow and
other spawn options when replacing the direct call so synthesis runs on the
configured remote.
---
Duplicate comments:
In `@src/__tests__/main/ipc/handlers/groupChat.test.ts`:
- Around line 1065-1094: The tests for the "groupChat:reportAutoRunComplete"
handler currently assert routeAgentResponse and markParticipantResponded but
don't assert the synthesis branch; update the two cases to explicitly assert
spawnModeratorSynthesis behavior: in the first test (when
markParticipantResponded returns false) add an expectation that
groupChatRouter.spawnModeratorSynthesis was not called, and in the second test
(when markParticipantResponded returns true) add an expectation that
groupChatRouter.spawnModeratorSynthesis was called (and optionally called with
the expected gc id and mockProcessManager) so both boolean branches of
markParticipantResponded are actually validated; locate the handler via
handlers.get('groupChat:reportAutoRunComplete') and the router mocks
groupChatRouter.routeAgentResponse, groupChatRouter.markParticipantResponded and
groupChatRouter.spawnModeratorSynthesis to modify the assertions.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 633-640: In the catch block inside useGroupChatHandlers.ts for the
stop-all flow, replace the blanket console.error + toast by explicitly reporting
the exception to Sentry (use your project Sentry utility, e.g.,
Sentry.captureException or the app's report function) with context/tags {
operation: 'groupChat:stopAll', activeGroupChatId } and include the original
error; then special-case known recoverable errors by checking error.code (handle
those without reporting or with a lower-severity log and keep the UI toast), and
only send telemetry for unexpected failures while still showing the same
user-facing toast on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e5fd41b-1a81-41b6-974c-f6cf42fa20e2
📒 Files selected for processing (3)
src/__tests__/main/ipc/handlers/groupChat.test.tssrc/main/group-chat/group-chat-router.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.ts
- Add Sentry reporting and powerManager cleanup to timeout synthesis catch - Scope batch completion to matching participant session, not all sessions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/renderer/hooks/groupChat/useGroupChatHandlers.ts (2)
621-643:⚠️ Potential issue | 🟠 Major
handleStopAllstill swallows unexpected errors and misses Sentry reporting.This catch handles every failure identically, logs to console, and never escalates unexpected errors.
Suggested fix aligned with error-handling guidelines
+import { captureException } from '../../utils/sentry'; ... const handleStopAll = useCallback(async () => { const { activeGroupChatId } = useGroupChatStore.getState(); if (!activeGroupChatId) return; try { // Cancel any in-flight autorun batch runs for this group chat. // These run in the agent's own Maestro session (not group-chat-prefixed), // so the main process's clearAllParticipantSessions won't reach them. const autoRunSessionIds = getAutoRunSessionsForGroupChat(activeGroupChatId); for (const sessionId of autoRunSessionIds) { useBatchStore.getState().dispatchBatch({ type: 'COMPLETE_BATCH', sessionId, }); } await window.maestro.groupChat.stopAll(activeGroupChatId); } catch (error) { - console.error('[GroupChat] Failed to stop all:', error); - notifyToast({ - type: 'error', - title: 'Stop Failed', - message: 'Failed to stop all group chat conversations. Please try again.', - }); + const errorCode = + typeof error === 'object' && error !== null && 'code' in error + ? String((error as { code?: unknown }).code) + : ''; + + if (errorCode === 'NETWORK_ERROR') { + notifyToast({ + type: 'error', + title: 'Stop Failed', + message: 'You appear to be offline. Please reconnect and try again.', + }); + return; + } + + captureException(error, { + operation: 'groupChat:stopAll', + groupChatId: activeGroupChatId, + }); + throw error; } }, []);As per coding guidelines, "For expected/recoverable errors, catch them explicitly, check the error code, and handle gracefully" and "For unexpected errors, rethrow to let Sentry capture them."
🤖 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 621 - 643, The catch block in handleStopAll swallows all errors; change it to handle expected/recoverable errors explicitly (e.g., inspect error.code or instance for known Maestro/group-chat errors) and only show the notifyToast and suppress the error for those cases; for any unexpected errors, rethrow the error so Sentry can capture it. Keep the existing cleanup of autoRunSessionIds via useBatchStore.dispatchBatch and the call to window.maestro.groupChat.stopAll, but replace the generic catch to branch: if error matches known/recoverable cases then log and notifyToast as now, otherwise console.error and throw error (or rethrow) so the error propagates to global handlers/Sentry.
241-257:⚠️ Potential issue | 🟠 MajorAvoid global name-only fallback while group-scoped autorun entries still exist.
When
autoRunSessionIdsis non-empty but no participant match is found, the current fallback can still complete an unrelated same-named session from another chat.Suggested minimal fix
const autoRunSessionIds = getAutoRunSessionsForGroupChat(groupChatId); if (autoRunSessionIds.length > 0) { const sessions = useSessionStore.getState().sessions; const matchingSessionId = autoRunSessionIds.find((sid) => sessions.some((s) => s.id === sid && s.name === participantName) ); if (matchingSessionId) { useBatchStore.getState().dispatchBatch({ type: 'COMPLETE_BATCH', sessionId: matchingSessionId, }); return; } + // Prevent cross-chat collisions from global name-only fallback. + return; } // Fallback: resolve by participant name if registry entry was already consumed const session = useSessionStore.getState().sessions.find((s) => s.name === participantName);🤖 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 241 - 257, The code currently falls back to resolving a session by name even when group-scoped autoRun entries exist but none matched, which can complete a same-named session from another chat; update the logic in useGroupChatHandlers by using getAutoRunSessionsForGroupChat(groupChatId) and after computing matchingSessionId (using useSessionStore.getState().sessions) if autoRunSessionIds.length > 0 and no matchingSessionId was found then return early (do not fall back), so only perform the name-only lookup (useSessionStore.getState().sessions.find by participantName) when autoRunSessionIds is empty.
🤖 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/main/group-chat/group-chat-router.ts`:
- Around line 1049-1054: The autoRunParticipantTracker map is populated for
autorun participants (autoRunParticipantTracker.set/get with groupChatId and
participant.name) but never cleared on successful completion, leaking entries
and causing future emitAutoRunBatchComplete to misfire; fix by removing the
participant from autoRunParticipantTracker in the non-timeout success path (the
code path that calls emitBatchComplete / handles successful batch processing for
participant) — call
autoRunParticipantTracker.get(groupChatId)!.delete(participant.name) and if the
Set becomes empty, remove the groupChatId key
(autoRunParticipantTracker.delete(groupChatId)); apply the same cleanup wherever
successful completion is handled so timeout is the only path that triggers
emitAutoRunBatchComplete for autorun entries.
- Around line 860-883: The code currently appends/emits/creates history for a
moderator message even when displayMessage is empty after stripping !autorun
directives; update the block around
appendToLog/groupChatEmitters.emitMessage/addGroupChatHistoryEntry to first
check whether displayMessage (after trimming) is non-empty and only proceed if
it has content—otherwise skip appending, emitting, and history creation (or
return early) to avoid empty chat bubbles and history rows; reference symbols:
displayMessage, appendToLog, groupChatEmitters.emitMessage,
addGroupChatHistoryEntry, extractFirstSentence, and groupChatId.
---
Duplicate comments:
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 621-643: The catch block in handleStopAll swallows all errors;
change it to handle expected/recoverable errors explicitly (e.g., inspect
error.code or instance for known Maestro/group-chat errors) and only show the
notifyToast and suppress the error for those cases; for any unexpected errors,
rethrow the error so Sentry can capture it. Keep the existing cleanup of
autoRunSessionIds via useBatchStore.dispatchBatch and the call to
window.maestro.groupChat.stopAll, but replace the generic catch to branch: if
error matches known/recoverable cases then log and notifyToast as now, otherwise
console.error and throw error (or rethrow) so the error propagates to global
handlers/Sentry.
- Around line 241-257: The code currently falls back to resolving a session by
name even when group-scoped autoRun entries exist but none matched, which can
complete a same-named session from another chat; update the logic in
useGroupChatHandlers by using getAutoRunSessionsForGroupChat(groupChatId) and
after computing matchingSessionId (using useSessionStore.getState().sessions) if
autoRunSessionIds.length > 0 and no matchingSessionId was found then return
early (do not fall back), so only perform the name-only lookup
(useSessionStore.getState().sessions.find by participantName) when
autoRunSessionIds is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab44283f-5590-46fe-8c29-5324a7bc609b
📒 Files selected for processing (2)
src/main/group-chat/group-chat-router.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.ts
- Skip log/emit/history for empty moderator messages after stripping !autorun - Clean up autoRunParticipantTracker in markParticipantResponded and clearPendingParticipants Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@jeffscottward my model confirms completion. |
…ion feedback Root cause: AI moderators often output markdown-formatted mentions like **@name** or _@name_. The mention regex captured trailing formatting chars (e.g. "controlplane**"), which failed participant matching silently — no agents were spawned, no visual indicators changed, and no synthesis triggered. Changes: - Add stripMarkdownFormatting() to clean *, _, `, ~ from mention names - Apply to extractMentions, extractAllMentions, and extractAutoRunDirectives - Emit system message when moderator mentions resolve to zero actionable work - Add 12 tests covering markdown-formatted mention extraction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause: pendingParticipantResponses was only populated AFTER the spawn
loop completed (deferred to post-loop block). If a participant process
exited before that registration, the exit listener's markParticipantResponded
call found no pending entry, returned false, and synthesis never triggered.
This also meant emitStateChange('agent-working') fired late, so the sidebar
and chat indicators didn't reflect participant activity during the window
between spawn and post-loop registration.
Fix: Register each participant in pendingParticipantResponses immediately
after spawn (both @mention and !autorun paths). Install per-participant
timeouts and emit 'agent-working' state on first spawn, not after loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This is a great contribution to group chats, and I would love to use it in main 🚀 |
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
3 participants
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
Changes
Core group chat improvements:
src/main/group-chat/group-chat-router.ts— Major enhancements to routing logic (+321 lines)src/main/ipc/handlers/groupChat.ts— Extended IPC handlers for group chat operationssrc/main/preload/groupChat.ts— New preload bridge for group chat IPCsrc/main/process-listeners/data-listener.ts/exit-listener.ts— Process lifecycle improvementsRenderer/UI:
src/renderer/App.tsx— Group chat integration into main app coordinatorsrc/renderer/components/GroupChat*.tsx— Updated header, list, panel, participants, and right panel componentssrc/renderer/components/ParticipantCard.tsx— Enhanced participant card with richer displaysrc/renderer/components/Settings/SettingsModal.tsx— New group chat settingsState management & utilities:
src/renderer/stores/groupChatStore.ts— New Zustand store for group chat statesrc/renderer/hooks/groupChat/useGroupChatHandlers.ts— Group chat event handlerssrc/renderer/utils/groupChatAutoRunRegistry.ts— Auto-run registry for group chat workflowsPrompts:
src/prompts/group-chat-moderator-system.md— New moderator system promptsrc/prompts/group-chat-participant-request.md— Updated participant request promptPR Checklist
npm run lint && npm run lint:eslint)npm test) — 22,296 passedTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests