fix(chat): flush batched updates at top of onExit on rc (#1022) by chr1syy · Pull Request #1052 · RunMaestro/Maestro

chr1syy · 2026-05-28T20:05:21Z

rc companion to #1023. Same race, different file after the post-decompose layout on rc.

Closes the same gap reported in #1022 for users on the RC channel (the reporter is on `0.16.18-RC`).

Root cause (recap)

Stdout chunks from the agent are accumulated in `useBatchedSessionUpdates` and flushed on a 200ms timer. When the agent exits, `onExit` synchronously calls `setSessions` to append a new `user` log entry for the next queued message. If the batched flush hasn't fired yet, trailing chunks of the just-finished response land in `tab.logs` after the new user entry. `TerminalOutput`'s `collapsedLogs` memo groups consecutive non-user/tool/thinking entries into one bubble, so the late chunks of response 1 get coalesced with response 2 — visually merging them.

Why a separate PR

After commit `e1b8eed29 refactor(agent): decompose useAgentListeners (1,977 -> 104 LOC, -95%)` on rc, the `onExit` handler now lives in `src/renderer/hooks/agent/internal/useAgentExitListener.ts` and does not receive `batchedUpdater`. PR #1023 targets `main` (where `useAgentListeners.ts` is still monolithic), so its diff cannot apply cleanly to rc. When rc next syncs from main, the structural conflict means the fix would most likely silently drop unless re-applied — this PR does that.

Fix

Add `flushNow: () => void` to the local `BatchedUpdater` subset interface in `src/renderer/hooks/agent/internal/types.ts` (matches the real `useBatchedSessionUpdates.ts:93` surface).
Plumb `batchedUpdater` through `UseAgentExitListenerDeps` and the `useAgentListeners` coordinator.
Call `deps.batchedUpdater.flushNow()` at the top of the `onExit` async callback (after the `-terminal-` early-return guard).

Test plan

Added regression test `'flushes batched updates at the top of onExit'` in `useAgentExitListener.test.tsx` asserting `flushNow` is called on AI exit.
Updated `makeBatched` factories in the 4 sibling listener tests (`useAgentDataListener`, `useAgentSessionIdListener`, `useAgentStderrListener`, `useAgentUsageListener`) to include `flushNow: vi.fn()` so they conform to the extended interface.
`npx vitest run` on all 5 internal listener test files — 27/27 pass.
`npx prettier --check` on all 8 changed files — clean.
`npx tsc --noEmit` — no new errors in changed files (8 pre-existing TS6133 errors in `FileExplorerPanel` components are unrelated).
Manual: send two prompts in quick succession to Claude Code on rc; confirm the two responses render as two separate dialog boxes with the queued user message between them.

Summary by CodeRabbit

Bug Fixes
- Resolved output ordering so trailing buffered log/output chunks are flushed before finalizing agent sessions, preventing out-of-sequence logs when a process exits.
Tests
- Added regression coverage to ensure buffered output is flushed in the correct order on agent exit.

) rc companion to RunMaestro#1023. After commit e1b8eed split useAgentListeners into per-channel sub-listeners, the queued-message dispatch race lives in useAgentExitListener. Without a pre-mutation flush, trailing batched stdout chunks from the just-finished response land in tab.logs after the new user log entry, merging with the next response's bubble in TerminalOutput's collapsedLogs grouping. - Plumb batchedUpdater through UseAgentExitListenerDeps and the useAgentListeners coordinator. - Call deps.batchedUpdater.flushNow() at the top of onExit, after the -terminal- guard. - Extend the local BatchedUpdater interface in hooks/agent/internal/types.ts with flushNow (matches the real useBatchedSessionUpdates surface). - Add flushNow: vi.fn() to the makeBatched factory in the 4 sibling listener tests and to the new makeDeps in useAgentExitListener.test. - Add regression test asserting flushNow is called on AI exit. Targeted tests: 27/27 pass across the 5 internal listener test files.

coderabbitai · 2026-05-28T20:05:40Z

📝 Walkthrough

Walkthrough

This pull request adds a flushNow() method to the BatchedUpdater interface, updates useAgentExitListener to call it on process exit, and ensures all test mocks include the new method. The flush is called before state mutations to guarantee trailing buffered stdout/stderr chunks are flushed in correct sequence order.

Changes

BatchedUpdater flushNow implementation

Layer / File(s)	Summary
BatchedUpdater interface contract `src/renderer/hooks/agent/internal/types.ts`	The `BatchedUpdater` interface now exports a `flushNow: () => void` method.
Exit listener flushNow integration `src/renderer/hooks/agent/internal/useAgentExitListener.ts`, `src/renderer/hooks/agent/useAgentListeners.ts`	`UseAgentExitListenerDeps` adds a required `batchedUpdater` dependency. On process exit, `flushNow()` is invoked before logger and state mutations with inline documentation explaining that this ordering prevents user-log entries from being appended ahead of trailing response chunks. The dependency is wired through from `useAgentListeners`.
Test mock updates `src/__tests__/renderer/hooks/agent/internal/useAgentDataListener.test.tsx`, `src/__tests__/renderer/hooks/agent/internal/useAgentExitListener.test.tsx`, `src/__tests__/renderer/hooks/agent/internal/useAgentSessionIdListener.test.tsx`, `src/__tests__/renderer/hooks/agent/internal/useAgentStderrListener.test.tsx`, `src/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx`, `src/__tests__/renderer/hooks/useAgentListeners.test.ts`	All test suites update their `makeBatched()` mock to include `flushNow: vi.fn()`. The `useAgentExitListener` test additionally adds a regression test asserting that `flushNow` is called at the start of the exit handling flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

approved, ready to merge

Poem

A rabbit hops with joy today,
New flushNow() lights the way!
Buffers drain in perfect time,
Trailing chunks now come in line,
Exit flows sing tidy rhyme. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(chat): flush batched updates at top of onExit on rc (`#1022`)' clearly and specifically describes the main change—flushing batched updates in the onExit handler to fix a race condition on the RC branch.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

greptile-apps · 2026-05-28T20:08:17Z

Greptile Summary

This PR ports the batched-update flush fix from main (#1023) to the rc branch, where the post-decompose refactor split onExit into its own hook (useAgentExitListener). The race condition — trailing stdout chunks landing after the user log entry for the next queued message, causing them to merge visually in collapsedLogs — is correctly addressed by calling flushNow() synchronously at the top of the onExit callback.

flushNow: () => void is added to the BatchedUpdater interface in types.ts, batchedUpdater is plumbed through UseAgentExitListenerDeps and the coordinator, and deps.batchedUpdater.flushNow() is called immediately after the -terminal- early-return guard.
A regression test is added to useAgentExitListener.test.tsx asserting that flushNow is called on AI exit; the four sibling listener test files are updated to include flushNow: vi.fn() in their makeBatched() factories.

Confidence Score: 4/5

Safe to merge; the fix is a minimal, targeted flush call that closes the described visual race without touching the broader exit-handling logic.

The core change is correct and well-tested. Two minor observations: deps.batchedUpdater is not in the useEffect dependency array despite being used directly in the closure, and flushNow() fires for -batch- session IDs before their early-return guard. Neither causes a current defect in practice — batchedUpdater is stable in the app and batch sessions don't accumulate AI stdout chunks — but both are small inconsistencies worth tidying.

src/renderer/hooks/agent/internal/useAgentExitListener.ts — the useEffect dependency array and the position of the flushNow() call relative to the -batch- guard.

Important Files Changed

Filename	Overview
src/renderer/hooks/agent/internal/useAgentExitListener.ts	Core fix: `deps.batchedUpdater.flushNow()` added after the `-terminal-` guard. Correct placement, but `batchedUpdater` is not listed in the `useEffect` dependency array, and the flush also fires for `-batch-` sessions before their early-return.
src/renderer/hooks/agent/internal/types.ts	Added `flushNow: () => void` to `BatchedUpdater` interface, matching the real `useBatchedSessionUpdates` surface. Clean, minimal change.
src/renderer/hooks/agent/useAgentListeners.ts	Plumbs `batchedUpdater` into `useAgentExitListener` call. One-line change, correctly wired from `deps.batchedUpdater`.
src/tests/renderer/hooks/agent/internal/useAgentExitListener.test.tsx	Adds `makeBatched()` factory with `flushNow: vi.fn()`, wires it into `makeDeps()`, and adds a regression test asserting `flushNow` is called on AI exit.

Sequence Diagram

sequenceDiagram
    participant Agent as AI Agent Process
    participant IPC as window.maestro.process.onExit
    participant Flush as batchedUpdater.flushNow()
    participant Store as useSessionStore (setSessions)
    participant UI as TerminalOutput (collapsedLogs)

    Agent->>IPC: exit event (sessionId, code)
    Note over IPC: guard: skip -terminal- tab IDs
    IPC->>Flush: flushNow() ← NEW: drains 200ms timer buffer
    Flush->>Store: pending stdout chunks committed atomically
    IPC->>Store: setSessions → append user log for queued message
    Store->>UI: re-render with correct log order
    Note over UI: response chunks appear BEFORE user entry ✓

Comments Outside Diff (2)

src/renderer/hooks/agent/internal/useAgentExitListener.ts, line 59-92 (link)

flushNow() fires unconditionally for all sessions that pass the first guard, including -batch- session IDs that return early a few lines later (line 91). Batch sessions don't produce AI batched updates, so this is a no-op in practice, but logically the flush belongs only to AI or terminal exits. Moving the call inside the if (isFromAi) block, or at minimum after the batch early-return, would more precisely scope it to the sessions that actually accumulate batched chunks.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
src/renderer/hooks/agent/internal/useAgentExitListener.ts, line 636-643 (link)

deps.batchedUpdater is captured in the useEffect closure and called directly with flushNow(), but it is not listed in the dependency array. If batchedUpdater ever changes identity, the handler would hold a stale reference and call the old flushNow. Adding it to the array keeps the closure in sync with the live value.

_{Reviews (1): Last reviewed commit: "fix(chat): flush batched updates at top ..." | Re-trigger Greptile}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/renderer/hooks/agent/internal/useAgentExitListener.ts`:
- Around line 62-70: The useEffect in useAgentExitListener registers
window.maestro.process.onExit and calls deps.batchedUpdater.flushNow(), but the
dependency array omits deps.batchedUpdater causing a stale reference; update the
useEffect dependency array to include deps.batchedUpdater (and any stable
identity for deps if needed) so flushNow() is always the current function,
keeping the effect and the window.maestro.process.onExit handler in sync with
changes to deps.batchedUpdater.

🪄 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: 77cdfc7e-22cb-4a85-ae45-094c80a41068

📥 Commits

Reviewing files that changed from the base of the PR and between ebe65a1 and 3c0d1f2.

📒 Files selected for processing (8)

src/__tests__/renderer/hooks/agent/internal/useAgentDataListener.test.tsx
src/__tests__/renderer/hooks/agent/internal/useAgentExitListener.test.tsx
src/__tests__/renderer/hooks/agent/internal/useAgentSessionIdListener.test.tsx
src/__tests__/renderer/hooks/agent/internal/useAgentStderrListener.test.tsx
src/__tests__/renderer/hooks/agent/internal/useAgentUsageListener.test.tsx
src/renderer/hooks/agent/internal/types.ts
src/renderer/hooks/agent/internal/useAgentExitListener.ts
src/renderer/hooks/agent/useAgentListeners.ts

pedramamini · 2026-05-28T21:54:58Z

Thanks for porting this over, @chr1syy — appreciate you catching the post-decompose layout difference on rc and adding a focused regression test. The fix itself is correct: flushing at the top of onExit is the right place to close the #1022 race, and it mirrors the companion #1023 cleanly.

One blocking item before this can land:

🔴 CI `test` job is red — 15 failures

The new deps.batchedUpdater.flushNow() call at useAgentExitListener.ts:70 throws TypeError: deps.batchedUpdater.flushNow is not a function in the coordinator suite src/__tests__/renderer/hooks/useAgentListeners.test.ts. Its mock factory createMockBatchedUpdater() (line ~134) wasn't updated with the new method:

function createMockBatchedUpdater(): BatchedUpdater {
	return {
		appendLog: vi.fn(),
		markDelivered: vi.fn(),
		markUnread: vi.fn(),
		updateUsage: vi.fn(),
		updateContextUsage: vi.fn(),
		updateCycleBytes: vi.fn(),
		updateCycleTokens: vi.fn(),
		flushNow: vi.fn(), // <-- add this
	};
}

Your test plan ran the 5 internal/ listener suites (which you correctly updated), but this coordinator-level suite keeps its own batchedUpdater mock and was missed. Adding flushNow: vi.fn() there should turn CI green.

Optional nits (non-blocking, your call) — from Greptile

deps.batchedUpdater is used in the onExit closure but isn't listed in the useEffect dependency array (lines ~636–643), even though every other deps.* is. Small consistency gap; harmless in practice since the updater is stable.
flushNow() fires before the -batch- early-return, so it runs (as a no-op) for batch sessions too. Moving it after that guard would scope it more precisely, though behavior is unchanged.

Once CI is green I'll take another pass. Thanks again for the contribution!

Addresses CI failure and review feedback from PR RunMaestro#1052: 1. `createMockBatchedUpdater` in `src/__tests__/renderer/hooks/useAgentListeners.test.ts` was missing `flushNow: vi.fn()`, causing 15 failures in the coordinator suite (TypeError: flushNow is not a function). 2. Move `flushNow()` past a new `-batch-` early-return guard at the top of onExit so it doesn't fire as a no-op for batch sessions (Greptile/pedramamini nit). The existing `-batch-` branch in the detection chain at line ~89 is now dead but left in place defensively — happy to remove if preferred. 3. Add `deps.batchedUpdater` to the `useEffect` dependency array for consistency with every other `deps.*` (CodeRabbit nit). Targeted tests: 94/94 pass across the coordinator + 5 internal listener suites.

chr1syy · 2026-05-29T05:20:23Z

Pushed acf7df53e addressing all three:

CI blocker (the actual one!): Added flushNow: vi.fn() to createMockBatchedUpdater in the coordinator suite src/__tests__/renderer/hooks/useAgentListeners.test.ts. My test plan missed this one because it lives outside internal/ — apologies for the noise.
-batch- scope: Added if (sessionId.includes('-batch-')) return; immediately after the -terminal- guard, before flushNow(). The existing else if (sessionId.includes('-batch-')) { return; } branch at line ~89 in the detection chain is now unreachable — I left it in place defensively to keep this change minimal, but happy to delete it in this PR or a follow-up if you'd prefer no corpses.
dep array: Added deps.batchedUpdater between deps.addHistoryEntryRef and deps.getBatchStateRef to match the existing pattern.

Local verification:

npx vitest run on the coordinator + 5 internal listener suites → 94/94 pass.
npx prettier --check → clean.

CI should turn green on the next run.

coderabbitai

🧹 Nitpick comments (1)

src/renderer/hooks/agent/internal/useAgentExitListener.ts (1)
61-61: 💤 Low value

Optional: Consolidate duplicate -batch- checks for clarity.

The -batch- session guard appears twice: at line 61 (before flushNow) and again at lines 91-92 (in the sessionId parsing logic). Per the commit message, the early guard was added to prevent flushNow from running for batch sessions, and the later branch was left defensively. While intentional, future maintainers might find the duplication confusing.

Also applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/hooks/agent/internal/useAgentExitListener.ts` at line 61,
Consolidate the duplicate "-batch-" guards in useAgentExitListener by
introducing a small helper (e.g., isBatchSession(sessionId)) and using it both
before calling flushNow and in the sessionId parsing/branching logic instead of
repeating sessionId.includes('-batch-'); update the early guard (the check that
short-circuits flushNow) and remove the duplicate literal check in the later
parsing branch so both sites call isBatchSession(sessionId) for clarity and
maintainability.

🤖 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.

Nitpick comments:
In `@src/renderer/hooks/agent/internal/useAgentExitListener.ts`:
- Line 61: Consolidate the duplicate "-batch-" guards in useAgentExitListener by
introducing a small helper (e.g., isBatchSession(sessionId)) and using it both
before calling flushNow and in the sessionId parsing/branching logic instead of
repeating sessionId.includes('-batch-'); update the early guard (the check that
short-circuits flushNow) and remove the duplicate literal check in the later
parsing branch so both sites call isBatchSession(sessionId) for clarity and
maintainability.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 75e62d17-fe11-4724-96c4-c4debba1a1a6

📥 Commits

Reviewing files that changed from the base of the PR and between 3c0d1f2 and acf7df5.

📒 Files selected for processing (2)

src/__tests__/renderer/hooks/useAgentListeners.test.ts
src/renderer/hooks/agent/internal/useAgentExitListener.ts

coderabbitai Bot reviewed May 28, 2026

View reviewed changes

Comment thread src/renderer/hooks/agent/internal/useAgentExitListener.ts

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

chr1syy added the ready to merge This PR is ready to merge label May 29, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(chat): flush batched updates at top of onExit on rc (#1022)#1052

fix(chat): flush batched updates at top of onExit on rc (#1022)#1052
chr1syy wants to merge 2 commits into
RunMaestro:rcfrom
chr1syy:fix/1022-queued-message-flush-rc

chr1syy commented May 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 28, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

greptile-apps Bot commented May 28, 2026 •

edited

Loading

Comments Outside Diff (2)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

pedramamini commented May 28, 2026

Uh oh!

chr1syy commented May 29, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

chr1syy commented May 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause (recap)

Why a separate PR

Fix

Test plan

Related

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

greptile-apps Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (2)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pedramamini commented May 28, 2026

🔴 CI test job is red — 15 failures

Optional nits (non-blocking, your call) — from Greptile

Uh oh!

chr1syy commented May 29, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chr1syy commented May 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

greptile-apps Bot commented May 28, 2026 •

edited

Loading

🔴 CI `test` job is red — 15 failures