From 200390ca333d8600a20bdc85112701b3e20b4874 Mon Sep 17 00:00:00 2001 From: Devansh Jain <31609257+devanshrj@users.noreply.github.com> Date: Tue, 3 Mar 2026 12:36:55 -0800 Subject: [PATCH] fix(tui): show background subagent notifications immediately when idle (#1242) --- src/cli/App.tsx | 34 ++-- src/cli/helpers/taskNotifications.ts | 41 +++++ src/tests/cli/task-notification-flush.test.ts | 174 ++++++++++++++++++ 3 files changed, 232 insertions(+), 17 deletions(-) create mode 100644 src/tests/cli/task-notification-flush.test.ts diff --git a/src/cli/App.tsx b/src/cli/App.tsx index e7b5551..b0e1f2a 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -276,7 +276,10 @@ import { flushEligibleLinesBeforeReentry, shouldClearCompletedSubagentsOnTurnStart, } from "./helpers/subagentTurnStart"; -import { extractTaskNotificationsForDisplay } from "./helpers/taskNotifications"; +import { + appendTaskNotificationEventsToBuffer, + extractTaskNotificationsForDisplay, +} from "./helpers/taskNotifications"; import { getRandomPastTenseVerb, getRandomThinkingVerb, @@ -1894,23 +1897,19 @@ export default function App({ ); }, [isExecutingTool]); + // Ref indirection: refreshDerived is declared later in the component but + // appendTaskNotificationEvents needs to call it. Using a ref avoids a + // forward-declaration error while keeping the deps array empty. + const refreshDerivedRef = useRef<(() => void) | null>(null); + const appendTaskNotificationEvents = useCallback( - (summaries: string[]): boolean => { - if (summaries.length === 0) return false; - for (const summary of summaries) { - const eventId = uid("event"); - buffersRef.current.byId.set(eventId, { - kind: "event", - id: eventId, - eventType: "task_notification", - eventData: {}, - phase: "finished", - summary, - }); - buffersRef.current.order.push(eventId); - } - return true; - }, + (summaries: string[]): boolean => + appendTaskNotificationEventsToBuffer( + summaries, + buffersRef.current, + () => uid("event"), + () => refreshDerivedRef.current?.(), + ), [], ); @@ -2702,6 +2701,7 @@ export default function App({ setLines(newLines); commitEligibleLines(b); }, [commitEligibleLines]); + refreshDerivedRef.current = refreshDerived; const recordCommandReminder = useCallback((event: CommandFinishedEvent) => { const input = event.input.trim(); diff --git a/src/cli/helpers/taskNotifications.ts b/src/cli/helpers/taskNotifications.ts index b1dd0b8..c6c441d 100644 --- a/src/cli/helpers/taskNotifications.ts +++ b/src/cli/helpers/taskNotifications.ts @@ -106,6 +106,47 @@ export function extractTaskNotificationsForDisplay(message: string): { return { notifications, cleanedText }; } +// ============================================================================ +// Buffer Helpers +// ============================================================================ + +/** + * Append task-notification events to a transcript buffer and flush. + * + * This is the pure-function core of App.tsx's `appendTaskNotificationEvents` + * useCallback. Extracting it here makes the behavioral contract testable: + * buffer writes MUST be followed by a flush so that notifications from + * background subagent onComplete callbacks (which run outside React's render + * cycle) appear immediately instead of waiting for the next unrelated render. + */ +export type NotificationBuffer = Pick< + import("./accumulator").Buffers, + "byId" | "order" +>; + +export function appendTaskNotificationEventsToBuffer( + summaries: string[], + buffer: NotificationBuffer, + generateId: () => string, + flush?: () => void, +): boolean { + if (summaries.length === 0) return false; + for (const summary of summaries) { + const eventId = generateId(); + buffer.byId.set(eventId, { + kind: "event", + id: eventId, + eventType: "task_notification", + eventData: {}, + phase: "finished", + summary, + }); + buffer.order.push(eventId); + } + flush?.(); + return true; +} + /** * Format multiple notifications as XML string. * @deprecated Use formatTaskNotification and queue individually diff --git a/src/tests/cli/task-notification-flush.test.ts b/src/tests/cli/task-notification-flush.test.ts new file mode 100644 index 0000000..f462f0f --- /dev/null +++ b/src/tests/cli/task-notification-flush.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, mock, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import type { NotificationBuffer } from "../../cli/helpers/taskNotifications"; +import { appendTaskNotificationEventsToBuffer } from "../../cli/helpers/taskNotifications"; + +// --------------------------------------------------------------------------- +// Helper-level behavioral tests +// --------------------------------------------------------------------------- + +describe("appendTaskNotificationEventsToBuffer", () => { + const makeBuffer = (): NotificationBuffer => ({ + byId: new Map(), + order: [], + }); + + let idCounter = 0; + const generateId = () => `event_${++idCounter}`; + + test("writes events to buffer and calls flush", () => { + const buffer = makeBuffer(); + const flush = mock(() => {}); + + const result = appendTaskNotificationEventsToBuffer( + ["Agent completed", "Reflection done"], + buffer, + generateId, + flush, + ); + + expect(result).toBe(true); + expect(buffer.order).toHaveLength(2); + expect(buffer.byId.size).toBe(2); + expect(flush).toHaveBeenCalledTimes(1); + + // Verify event shape + const firstId = buffer.order[0]; + expect(firstId).toBeDefined(); + const first = buffer.byId.get(firstId as string) as Record; + expect(first.kind).toBe("event"); + expect(first.eventType).toBe("task_notification"); + expect(first.phase).toBe("finished"); + expect(first.summary).toBe("Agent completed"); + }); + + test("flush is called exactly once even with multiple summaries", () => { + const buffer = makeBuffer(); + const flush = mock(() => {}); + + appendTaskNotificationEventsToBuffer( + ["one", "two", "three"], + buffer, + generateId, + flush, + ); + + expect(buffer.order).toHaveLength(3); + expect(flush).toHaveBeenCalledTimes(1); + }); + + test("returns false and skips flush for empty summaries", () => { + const buffer = makeBuffer(); + const flush = mock(() => {}); + + const result = appendTaskNotificationEventsToBuffer( + [], + buffer, + generateId, + flush, + ); + + expect(result).toBe(false); + expect(buffer.order).toHaveLength(0); + expect(flush).not.toHaveBeenCalled(); + }); + + test("works without flush callback (non-background caller)", () => { + const buffer = makeBuffer(); + + const result = appendTaskNotificationEventsToBuffer( + ["Agent completed"], + buffer, + generateId, + ); + + expect(result).toBe(true); + expect(buffer.order).toHaveLength(1); + }); +}); + +// --------------------------------------------------------------------------- +// Integration wiring: onComplete callbacks → appendTaskNotificationEvents +// +// These verify that the actual background subagent completion sites in App.tsx +// route through the flush-equipped appendTaskNotificationEvents, and that the +// ref indirection connecting it to refreshDerived is wired correctly. +// --------------------------------------------------------------------------- + +describe("background onComplete → flush wiring in App.tsx", () => { + const readSource = () => + readFileSync( + fileURLToPath(new URL("../../cli/App.tsx", import.meta.url)), + "utf-8", + ); + + test("appendTaskNotificationEvents delegates to appendTaskNotificationEventsToBuffer with a flush arg", () => { + const source = readSource(); + + // The useCallback must call the extracted helper + expect(source).toContain("appendTaskNotificationEventsToBuffer("); + + // It must pass refreshDerivedRef as the flush callback (4th arg). + // Match the delegation pattern: the flush lambda references refreshDerivedRef. + expect(source).toContain("refreshDerivedRef.current?."); + }); + + test("refreshDerivedRef is assigned after refreshDerived is defined", () => { + const source = readSource(); + + const refDecl = source.indexOf("const refreshDerivedRef = useRef"); + const derivedDecl = source.indexOf("const refreshDerived = useCallback"); + const refAssign = source.indexOf( + "refreshDerivedRef.current = refreshDerived", + ); + + expect(refDecl).toBeGreaterThan(-1); + expect(derivedDecl).toBeGreaterThan(-1); + expect(refAssign).toBeGreaterThan(-1); + + // Declaration order: ref declared before refreshDerived, assignment after + expect(refDecl).toBeLessThan(derivedDecl); + expect(refAssign).toBeGreaterThan(derivedDecl); + }); + + test("/init onComplete calls appendTaskNotificationEvents", () => { + const source = readSource(); + + // Find the /init silentCompletion block and verify its onComplete + // calls appendTaskNotificationEvents (not addToMessageQueue or inline buffer writes) + const initBlock = source.indexOf('subagentType: "init"'); + expect(initBlock).toBeGreaterThan(-1); + + const onCompleteIdx = source.indexOf("onComplete:", initBlock); + expect(onCompleteIdx).toBeGreaterThan(-1); + + // The appendTaskNotificationEvents call must be within the onComplete body + // (before the next closing of spawnBackgroundSubagentTask) + const callIdx = source.indexOf( + "appendTaskNotificationEvents(", + onCompleteIdx, + ); + const blockEnd = source.indexOf("});", onCompleteIdx); + expect(callIdx).toBeGreaterThan(onCompleteIdx); + expect(callIdx).toBeLessThan(blockEnd); + }); + + test("reflection onComplete calls appendTaskNotificationEvents", () => { + const source = readSource(); + + const reflectionBlock = source.indexOf('subagentType: "reflection"'); + expect(reflectionBlock).toBeGreaterThan(-1); + + const onCompleteIdx = source.indexOf("onComplete:", reflectionBlock); + expect(onCompleteIdx).toBeGreaterThan(-1); + + const callIdx = source.indexOf( + "appendTaskNotificationEvents(", + onCompleteIdx, + ); + const blockEnd = source.indexOf("});", onCompleteIdx); + expect(callIdx).toBeGreaterThan(onCompleteIdx); + expect(callIdx).toBeLessThan(blockEnd); + }); +});