Improve group chat autorun resilience and large-session performance #1037
Open
ets wants to merge 14 commits into
Open
Improve group chat autorun resilience and large-session performance #1037ets wants to merge 14 commits into
ets wants to merge 14 commits into
Conversation
|
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 canonical Auto Run parsing and propagation to group-chat messages (backend resolution and renderer hydration), virtualizes the group chat message list with memoized subcomponents and index-based scrolling, tracks agent-run errors and batch registry consumption, updates prompts and release-note formatting, and updates related tests. ChangesAuto Run References Feature
Message List Virtualization
Agent Execution and Batch Handling
Tests and Release Notes
Sequence DiagramsequenceDiagram
participant P
participant R
participant X
participant F
participant S
participant U
P->>R: message with !autorun directives
R->>X: extractStructuredAutoRunPaths(content)
X-->>R: relative paths
R->>F: resolve local .md candidates
R->>S: readDirRemote when SSH configured
F-->>R: matched documents
S-->>R: matched remote documents
R->>X: extractCanonicalAutoRunRefs(enrichedContent)
X-->>R: canonical AUTO_RUN_PATH/AUTO_RUN_TRIGGER blocks
R->>U: emit message with autoRunRefs and enriched content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ets
changed the title
Greptile SummaryThis PR improves group chat reliability and performance through two main tracks: (1) virtualizing the message list with
Confidence Score: 5/5Safe to merge. The cross-chat session collision in onAutoRunBatchComplete is properly fixed and all three completion paths (normal, orphan, timeout) are covered by targeted tests. All three paths through the Auto Run completion flow (normal batch finish, orphaned batch, and participant timeout) are now scoped by groupChatId. The previous unscoped name-only fallback is removed. Both the registry lookup and the pendingCompletions map enforce groupChatId isolation, and the new test suite explicitly verifies that same-named participants across different group chats produce independent COMPLETE_BATCH dispatches without interference. No files require special attention. The new filesystem-traversal code in group-chat-router.ts handles error paths gracefully and the error test coverage in group-chat-router.test.ts is thorough. Important Files Changed
Sequence DiagramsequenceDiagram
participant Router as group-chat-router.ts
participant Registry as groupChatAutoRunRegistry
participant BatchHandlers as useBatchHandlers
participant AppTsx as App.tsx
participant GCHandlers as useGroupChatHandlers
participant BatchStore as batchStore
Router->>Router: "setParticipantResponseTimeout(isAutoRun=true, 2h)"
Router->>AppTsx: emitAutoRunRequest(groupChatId, participantName, files)
AppTsx->>Registry: registerGroupChatAutoRun(sessionId, groupChatId, participantName)
AppTsx->>AppTsx: startBatchRun(sessionId, config)
alt Batch completes normally
BatchHandlers->>Registry: consumeGroupChatAutoRunForCompletion(sessionId)
Note over Registry: moves to pendingCompletions[groupChatId:participant]
BatchHandlers->>Router: reportAutoRunComplete(groupChatId, participantName, summary)
Router->>Router: clearParticipantTimeout
Router->>GCHandlers: emitAutoRunBatchComplete(groupChatId, participantName)
GCHandlers->>Registry: consumeCompletedGroupChatAutoRun(groupChatId, participantName)
GCHandlers->>BatchStore: COMPLETE_BATCH(sessionId)
else Orphaned batch (0 tasks)
AppTsx->>Registry: consumeGroupChatAutoRunForCompletion(sessionId)
Note over Registry: moves to pendingCompletions[groupChatId:participant]
AppTsx->>Router: reportAutoRunComplete(groupChatId, participantName, failureMsg)
Router->>Router: clearParticipantTimeout
Router->>GCHandlers: emitAutoRunBatchComplete(groupChatId, participantName)
GCHandlers->>Registry: consumeCompletedGroupChatAutoRun(groupChatId, participantName)
GCHandlers->>BatchStore: COMPLETE_BATCH(sessionId)
else Participant timeout (2h)
Router->>GCHandlers: emitAutoRunBatchComplete(groupChatId, participantName)
GCHandlers->>Registry: getAutoRunSessionForGroupChatParticipant(groupChatId, participantName)
Note over Registry: live registry still has entry
GCHandlers->>BatchStore: COMPLETE_BATCH(sessionId)
end
Reviews (3): Last reviewed commit: "MAESTRO: Fix agent execution error order..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+666
to
679
| // Debounce draft persistence to avoid triggering store updates (and full re-renders) | ||
| // on every keystroke. The input component keeps its own local state for immediate | ||
| // responsiveness; this only persists for cross-session draft recovery. | ||
| const draftTimerRef = useRef<ReturnType<typeof setTimeout>>(); | ||
| const handleGroupChatDraftChange = useCallback((draft: string) => { | ||
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | ||
| if (!activeGroupChatId) return; | ||
| setGroupChats((prev) => | ||
| prev.map((c) => (c.id === activeGroupChatId ? { ...c, draftMessage: draft } : c)) | ||
| ); | ||
| if (draftTimerRef.current) clearTimeout(draftTimerRef.current); | ||
| draftTimerRef.current = setTimeout(() => { | ||
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | ||
| if (!activeGroupChatId) return; | ||
| setGroupChats((prev) => | ||
| prev.map((c) => (c.id === activeGroupChatId ? { ...c, draftMessage: draft } : c)) | ||
| ); | ||
| }, 300); | ||
| }, []); |
There was a problem hiding this comment.
setGroupChats on the Zustand store with a stale chat ID. Adding an unmount cleanup keeps this consistent with React best practices and safe if the hook's lifecycle ever changes.
Suggested change
| // Debounce draft persistence to avoid triggering store updates (and full re-renders) | |
| // on every keystroke. The input component keeps its own local state for immediate | |
| // responsiveness; this only persists for cross-session draft recovery. | |
| const draftTimerRef = useRef<ReturnType<typeof setTimeout>>(); | |
| const handleGroupChatDraftChange = useCallback((draft: string) => { | |
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | |
| if (!activeGroupChatId) return; | |
| setGroupChats((prev) => | |
| prev.map((c) => (c.id === activeGroupChatId ? { ...c, draftMessage: draft } : c)) | |
| ); | |
| if (draftTimerRef.current) clearTimeout(draftTimerRef.current); | |
| draftTimerRef.current = setTimeout(() => { | |
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | |
| if (!activeGroupChatId) return; | |
| setGroupChats((prev) => | |
| prev.map((c) => (c.id === activeGroupChatId ? { ...c, draftMessage: draft } : c)) | |
| ); | |
| }, 300); | |
| }, []); | |
| // Debounce draft persistence to avoid triggering store updates (and full re-renders) | |
| // on every keystroke. The input component keeps its own local state for immediate | |
| // responsiveness; this only persists for cross-session draft recovery. | |
| const draftTimerRef = useRef<ReturnType<typeof setTimeout>>(); | |
| useEffect(() => { | |
| return () => { | |
| if (draftTimerRef.current) clearTimeout(draftTimerRef.current); | |
| }; | |
| }, []); | |
| const handleGroupChatDraftChange = useCallback((draft: string) => { | |
| if (draftTimerRef.current) clearTimeout(draftTimerRef.current); | |
| draftTimerRef.current = setTimeout(() => { | |
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | |
| if (!activeGroupChatId) return; | |
| setGroupChats((prev) => | |
| prev.map((c) => (c.id === activeGroupChatId ? { ...c, draftMessage: draft } : c)) | |
| ); | |
| }, 300); | |
| }, []); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/__tests__/renderer/hooks/useAgentExecution.test.ts (1)
71-71: ⚡ Quick winExpose and assert the
onAgentErrorpath in the test harness.At Line [103]-Line [105], the mock registers
onAgentErrorbut does not retain the callback, so the new failure path can’t be exercised. Please store the handler and add one assertion thatspawnAgentForSessionresolves withsuccess: falsewhen that handler fires.Suggested patch
let onUsageHandler: ((sid: string, usage: UsageStats) => void) | undefined; +let onAgentErrorHandler: ((sid: string) => void) | undefined; let onExitHandler: ((sid: string) => void) | undefined; ... - mockProcess.onAgentError.mockImplementation(() => { + mockProcess.onAgentError.mockImplementation((handler: (sid: string) => void) => { + onAgentErrorHandler = handler; return () => {}; });it('returns success=false when agent error is emitted for the batch session', async () => { const session = createMockSession(); const sessionsRef = { current: [session] }; const { result } = renderHook(() => useAgentExecution({ activeSession: session, sessionsRef, setSessions: vi.fn(), processQueuedItemRef: { current: null }, setFlashNotification: vi.fn(), setSuccessFlashNotification: vi.fn(), }) ); const spawnPromise = result.current.spawnAgentForSession(session.id, 'Test prompt'); await waitFor(() => expect(mockProcess.spawn).toHaveBeenCalledTimes(1)); const targetSessionId = mockProcess.spawn.mock.calls[0][0].sessionId as string; act(() => { onAgentErrorHandler?.(targetSessionId); onExitHandler?.(targetSessionId); }); await expect(spawnPromise).resolves.toMatchObject({ success: false }); });Also applies to: 103-105
🤖 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/useAgentExecution.test.ts` at line 71, The test currently registers onAgentError as vi.fn() without saving the callback, so the error path in useAgentExecution.spawnAgentForSession can't be exercised; update the mock registration of onAgentError to capture and store the handler (e.g., onAgentErrorHandler) when mockProcess.on or similar is called, then in the new test invoke that stored onAgentErrorHandler with the target sessionId after spawn is triggered (alongside onExitHandler if required) and add an assertion that awaiting result.current.spawnAgentForSession(session.id, 'Test prompt') resolves to an object with success: false; reference mockProcess.spawn to obtain the targetSessionId and the spawnAgentForSession function in useAgentExecution to locate where to trigger and assert this behavior.
🤖 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 `@docs/releases.md`:
- Line 20: Several release-section headings (for example the heading "Major
0.15.x Additions" and other similar section titles) use top-level/incorrect
heading depth and should be demoted to maintain consistent nesting under their
version sections; update those headings to use "###" (or "####" where deeper
nesting is intended) so they are properly nested under the surrounding "##
vX.Y.x" version headings and restore a consistent TOC/hierarchy across the
document.
- Line 174: Replace the incorrect branding "Github" with the official casing
"GitHub" in the release note string that currently reads "Github Worktree
support was added." (search for the exact phrase "Github Worktree support" in
the doc and update it to "GitHub Worktree support").
- Line 80: Update the release notes header text that currently reads "# Smaller
Changes in 014.x" to the correct semantic version series "# Smaller Changes in
0.14.x" in docs/releases.md so the heading accurately reflects the 0.14.x
release series.
- Line 74: Update the typo in the release note sentence that currently reads
“Launch from file preview or from the FIle tree panel” by changing “FIle” to
“File” so it reads “File tree panel”; edit the string in docs/releases.md (the
sentence containing “Launch from file preview or from the FIle tree panel”) to
correct the capitalization.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 295-298: The current code silently returns [] when
readDirRemote(folderPath, sshRemoteConfig) fails or returns no data; update the
handling around readDirRemote so you inspect result.error and
result.success/result.data, classify expected vs unexpected failures, and do not
swallow unexpected errors: for expected/recoverable cases return an explicit
empty array or a clear error object (with context including folderPath and
sshRemoteConfig host), but for unexpected failures rethrow the original error
(or wrap it with context) so Sentry can capture it; ensure you reference the
readDirRemote call and use result.success, result.data, and result.error when
implementing this change in group-chat-router.ts.
- Around line 1704-1706: Wrap the call to
resolveParticipantAutoRunRefs(participantName, message) inside a try/catch in
routeAgentResponse so any I/O/remote error does not abort the flow; on success
use appendCanonicalAutoRunRefs(message, autoRunRefs), on failure log/report the
error with Sentry utilities (include context like participantName and message
metadata) and continue using the original message so append/emit/history still
run. Ensure the catch only treats this as recoverable (do not rethrow), and add
a brief error-level log via processLogger or Sentry captureException with
contextual tags to aid debugging.
In `@src/renderer/App.tsx`:
- Around line 1341-1360: startBatchRunRef.current(...) can throw synchronously
and its rejection path isn’t reporting to Sentry; wrap the call so synchronous
throws are caught and reported with Sentry utilities and keep the existing chat
failure/reporting behavior. Specifically, call startBatchRunRef.current(...)
inside a try/catch (or use Promise.resolve(...) to normalize sync throws), and
in the catch call captureException(err) and/or captureMessage with a contextual
message before rethrowing or reporting to chat; also add captureException calls
in the existing promise rejection handler (the (err) => { ... }) so both sync
throws and async rejections send errors to Sentry while still invoking
consumeGroupChatAutoRun(session.id) and reportFailure.
In `@src/renderer/global.d.ts`:
- Around line 1742-1747: The groupChat message type is inconsistent: getMessages
advertises autoRunRefs but onMessage's callback type omits it; update the
onMessage callback signature to include the same optional autoRunRefs property
(Array<{ participantName: string; relativePath: string; triggerCommand: string;
}> ) so the groupChat message shape matches getMessages. Locate the
groupChat-related types and the onMessage callback declaration (symbols:
groupChat, getMessages, onMessage, autoRunRefs) and add autoRunRefs?: Array<{
participantName: string; relativePath: string; triggerCommand: string }>; to the
callback parameter type.
In `@src/renderer/hooks/agent/useAgentExecution.ts`:
- Around line 237-245: The code may report success=true if onExit cleanup
removes listeners before a late onAgentError flips didError and also fails to
report agent errors to Sentry; modify the onExit handler (and cleanup flow
around cleanupFns) to delay finalizing/resolving success until a short grace
window or until you deterministically know no further onAgentError can arrive
(e.g., wait for a few milliseconds or an "confirmedExit" flag), ensure the
onAgentError listener (window.maestro.process.onAgentError) sets didError=true
and calls Sentry.captureException/captureMessage with context about
targetSessionId and the error, and only remove the onAgentError listener from
cleanupFns after this window/confirmation so that lines resolving success (the
resolution logic that reads didError) always sees a correct error state.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 670-678: The debounce currently reads
useGroupChatStore.getState().activeGroupChatId inside the setTimeout, which can
change before the timer fires and cause the draft to be saved to the wrong chat;
modify handleGroupChatDraftChange to capture the current activeGroupChatId (and
setGroupChats getter if you prefer) immediately when the callback runs and then
use that captured id inside draftTimerRef's setTimeout closure so the draft is
applied to the chat that was active at keystroke time (reference symbols:
handleGroupChatDraftChange, draftTimerRef, activeGroupChatId, setGroupChats).
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useAgentExecution.test.ts`:
- Line 71: The test currently registers onAgentError as vi.fn() without saving
the callback, so the error path in useAgentExecution.spawnAgentForSession can't
be exercised; update the mock registration of onAgentError to capture and store
the handler (e.g., onAgentErrorHandler) when mockProcess.on or similar is
called, then in the new test invoke that stored onAgentErrorHandler with the
target sessionId after spawn is triggered (alongside onExitHandler if required)
and add an assertion that awaiting
result.current.spawnAgentForSession(session.id, 'Test prompt') resolves to an
object with success: false; reference mockProcess.spawn to obtain the
targetSessionId and the spawnAgentForSession function in useAgentExecution to
locate where to trigger and assert this behavior.
🪄 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: c2284ee6-7036-4298-b9b4-46d484e1ddfd
📒 Files selected for processing (15)
docs/releases.mdsrc/__tests__/renderer/hooks/useAgentExecution.test.tssrc/__tests__/renderer/hooks/useGroupChatHandlers.test.tssrc/__tests__/shared/group-chat-types.test.tssrc/main/group-chat/group-chat-router.tssrc/main/preload/groupChat.tssrc/prompts/group-chat-moderator-system.mdsrc/prompts/group-chat-participant-request.mdsrc/renderer/App.tsxsrc/renderer/components/GroupChatMessages.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentExecution.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/types/index.tssrc/shared/group-chat-types.ts
ets
force-pushed
the
ets/group-chat-enhancements
branch
from
279b995 to
575efd0
Compare
…ncements # Conflicts: # src/__tests__/renderer/hooks/useGroupChatHandlers.test.ts # src/renderer/components/GroupChatMessages.tsx
Contributor
Author
|
@greptile review |
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
Improves group chat behavior for long-running conversations by virtualizing the message list and tightening state updates that previously made the chat composer sluggish as message volume grew. Also strengthens group chat Auto Run handling so participant execution, queueing, and completion reporting are more robust in large collaborative sessions.
Why
Long group chats can accumulate enough rendered message content to degrade the responsiveness of the Maestro window, including visibly slower typing in the group chat input. These changes reduce rendering pressure for large sessions and make group chat Auto Run flows safer for users coordinating multiple agents over longer conversations.
What changed
Testing
Notes:
npm run lint:eslintreports one existing warning insrc/main/web-server/web-server-factory.ts:80for an unusedevariable. Full test suite passed with 22,512 tests and 107 skipped.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests