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>
📝 WalkthroughWalkthroughThis PR introduces auto-run support for group chats, enabling moderators to trigger batch document execution via !autorun directives. It adds participant live output streaming, stop-all controls, moderator settings integration (standing instructions and conductor profile), and per-participant timeout handling with synthesis fallback. Changes
Sequence DiagramsequenceDiagram
participant Moderator as Moderator (UI)
participant Router as Group Chat Router
participant Renderer as Renderer Auto-run<br/>Bridge
participant BatchRunner as Batch Runner
participant GroupChat as GroupChat IPC
Moderator->>Router: Send message with !autorun directive
Router->>Router: Extract & validate !autorun directives
Router->>Renderer: Emit onAutoRunTriggered event
Renderer->>Renderer: Validate session & auto-run folder
Renderer->>Renderer: Register auto-run context in<br/>groupChatAutoRunRegistry
Renderer->>BatchRunner: Start batch run with document config
BatchRunner->>BatchRunner: Execute documents
BatchRunner->>GroupChat: Report completion via<br/>reportAutoRunComplete
GroupChat->>Router: Route auto-run completion response
Router->>Moderator: Update group chat state & emit<br/>synthesis trigger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
Comment on lines
+1068
to
+1071
| vi.mocked(groupChatRouter.markParticipantResponded).mockReturnValue({ | ||
| allResponded: false, | ||
| isLastParticipant: false, | ||
| } as any); |
There was a problem hiding this comment.
Mock return type for markParticipantResponded is incorrect.
The real router API returns boolean, but tests mock an object. That weakens branch validation and can hide regressions in synthesis-trigger logic. Mock false/true directly and assert spawn behavior accordingly.
Also applies to: 1086-1089
🤖 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 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).
Comment on lines
+170
to
+172
| } catch { | ||
| // Non-critical — synthesize anyway | ||
| } |
There was a problem hiding this comment.
Silent catch drops timeout-path failures from telemetry.
catch {} here suppresses failures when writing timeout markers, so production issues disappear silently. Please at least log + report with context.
As per coding guidelines, "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production."
🤖 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 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.
Comment on lines
+177
to
+179
| groupChatEmitters.emitParticipantState?.(groupChatId, participantName, 'idle'); | ||
| groupChatEmitters.emitAutoRunBatchComplete?.(groupChatId, participantName); | ||
|
|
There was a problem hiding this comment.
emitAutoRunBatchComplete is firing for all participant timeouts, not just autorun.
This timeout path is shared by normal participant spawns too. Emitting auto-run completion unconditionally can force-complete unrelated batch state in the renderer for same-named sessions. Gate this emit to autorun-tracked participants only.
🤖 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 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.
Comment on lines
+1239
to
1248
| } else if (mentionsToSpawn.length === 0 && autoRunParticipants.length === 0) { | ||
| console.log( | ||
| `[GroupChat:Debug] No participant @mentions or autorun directives found - moderator response is final` | ||
| ); | ||
| // Set state back to idle since no agents are being spawned | ||
| groupChatEmitters.emitStateChange?.(groupChatId, 'idle'); | ||
| console.log(`[GroupChat:Debug] Emitted state change: idle`); | ||
| // Remove power block reason since round is complete | ||
| powerManager.removeBlockReason(`groupchat:${groupChatId}`); | ||
| } |
There was a problem hiding this comment.
Finalization check is based on raw directives, not actionable work.
If !autorun directives are present but all are invalid/skipped, autoRunParticipants.length > 0 prevents the idle/power-block cleanup branch from running, while participantsToRespond.size may still be 0. That can leak powerManager block reasons and leave lifecycle inconsistent.
Suggested fix
- } else if (mentionsToSpawn.length === 0 && autoRunParticipants.length === 0) {
+ }
+
+ // No actionable participant work was started (all mentions/directives resolved to no-op)
+ if (participantsToRespond.size === 0) {
groupChatEmitters.emitStateChange?.(groupChatId, 'idle');
powerManager.removeBlockReason(`groupchat:${groupChatId}`);
}Also applies to: 1251-1268
🤖 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 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.
Comment on lines
+237
to
+245
| const unsubBatchComplete = window.maestro.groupChat.onAutoRunBatchComplete?.( | ||
| (_groupChatId, participantName) => { | ||
| const session = useSessionStore.getState().sessions.find((s) => s.name === participantName); | ||
| if (!session) return; | ||
| // Dispatch COMPLETE_BATCH — no-op if already completed, cleans up if stuck. | ||
| useBatchStore.getState().dispatchBatch({ | ||
| type: 'COMPLETE_BATCH', | ||
| sessionId: session.id, | ||
| }); |
There was a problem hiding this comment.
Batch completion resolution is ambiguous (participant name only).
On Line 239, selecting session by participantName can complete the wrong batch when names collide across sessions/chats; _groupChatId is ignored too. Resolve batch session IDs from a group-chat-scoped autorun registry mapping, not global name search.
🤖 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 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.
Comment on lines
+620
to
+622
| } catch (error) { | ||
| console.error('[GroupChat] Failed to stop all:', error); | ||
| } |
There was a problem hiding this comment.
handleStopAll failure is swallowed with console-only logging.
If IPC stop fails, users get no feedback and the error is not explicitly reported. Handle this as recoverable UI failure (toast) and report exception context.
As per coding guidelines, "For expected/recoverable errors, catch them explicitly, check the error code, and handle gracefully (e.g., show offline message)."
🤖 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 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.
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
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
Release Notes
New Features
Bug Fixes
Documentation
Tests