diff --git a/packages/subagents/CHANGELOG.md b/packages/subagents/CHANGELOG.md index 2b42bb3b9..3c95498d5 100644 --- a/packages/subagents/CHANGELOG.md +++ b/packages/subagents/CHANGELOG.md @@ -7,6 +7,7 @@ - Prevented the async subagent status widget from briefly unmounting during reset-and-hydrate cycles when active background runs are still present, including Atomic host updates that deliver fresh UI context wrappers for the same logical session ([#1517](https://github.com/bastani-inc/atomic/issues/1517)). - Fixed live subagent result animation cleanup to register a host-row disposer, so terminal workflow cleanup evicts animation registry entries instead of only clearing intervals ([#1518](https://github.com/bastani-inc/atomic/issues/1518)). - Synced recent upstream subagent hardening so compact delegated tool-call summaries are preserved, fanout children keep their live nested subagent call/result history, duplicate concurrent subagent dispatches are rejected, provider-hostile chain schema conditionals are removed, and failed foreground runs include captured child output for diagnostics ([#1527](https://github.com/bastani-inc/atomic/issues/1527)). +- Eliminated the foreground subagent widget flicker that appeared once a running subagent panel grew tall enough to reach or exceed the terminal viewport. The live compact result no longer animates its spinner on an 80ms wall-clock timer; instead it shows an activity "pulse" glyph that advances exactly once per real progress update. Because the panel renders into chat scrollback, a timer-driven spinner cell that scrolled above pi-tui's viewport fold forced a destructive full-screen + scrollback clear on every tick — driving the indicator off genuine updates keeps line diffs tied to content that actually changed, so the differential renderer repaints only when progress does and never strobes. ## [0.9.3-alpha.1] - 2026-06-25 diff --git a/packages/subagents/src/extension/index.ts b/packages/subagents/src/extension/index.ts index d1c904467..7c01aa6ad 100644 --- a/packages/subagents/src/extension/index.ts +++ b/packages/subagents/src/extension/index.ts @@ -10,7 +10,7 @@ import { discoverAgents } from "../agents/agents.ts"; import { cleanupAllArtifactDirs, cleanupOldArtifacts, getArtifactsDir } from "../shared/artifacts.ts"; import { resolveCurrentSessionId } from "../shared/session-identity.ts"; import { cleanupOldChainDirs } from "../shared/settings.ts"; -import { renderLiveSubagentResult, renderSubagentResult, stopResultAnimations, stopWidgetAnimation, type SubagentResultRenderState } from "../tui/render.ts"; +import { advanceResultPulseFrame, renderLiveSubagentResult, renderSubagentResult, stopResultAnimations, stopWidgetAnimation, type SubagentResultRenderState } from "../tui/render.ts"; import { SubagentParams } from "./schemas.ts"; import { createSubagentExecutor, type SubagentParamsLike } from "../runs/foreground/subagent-executor.ts"; import { createAsyncJobTracker } from "../runs/background/async-job-tracker.ts"; @@ -69,7 +69,7 @@ type SubagentToolRenderState = SubagentResultRenderState; function rebuildSlashResultContainer( container: Container, result: AgentToolResult
, - options: { expanded: boolean; now?: number }, + options: { expanded: boolean; now?: number; pulseFrame?: number }, theme: ExtensionContext["ui"]["theme"], ): void { container.clear(); @@ -87,12 +87,14 @@ function createSlashResultComponent( const container = new Container(); let lastVersion = -1; let lastSnapshotNow = 0; + let pulseFrame = 0; container.render = (width: number): string[] => { const snapshot = getSlashRenderableSnapshot(details); if (snapshot.version !== lastVersion) { lastVersion = snapshot.version; lastSnapshotNow = Date.now(); - rebuildSlashResultContainer(container, snapshot.result, { ...options, now: lastSnapshotNow }, theme); + pulseFrame = advanceResultPulseFrame(pulseFrame); + rebuildSlashResultContainer(container, snapshot.result, { ...options, now: lastSnapshotNow, pulseFrame }, theme); } return Container.prototype.render.call(container, width); }; diff --git a/packages/subagents/src/tui/render-layout.ts b/packages/subagents/src/tui/render-layout.ts index 95d305fcd..8afaa8b93 100644 --- a/packages/subagents/src/tui/render-layout.ts +++ b/packages/subagents/src/tui/render-layout.ts @@ -66,16 +66,22 @@ export function truncLine(text: string, maxWidth: number): string { return result + activeStyles.join("") + "…"; } -const RUNNING_FRAMES = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; +export const RUNNING_FRAMES = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; /** * Spinner cadence (ms per frame). The running glyph is derived from wall-clock * time so every active spinner advances smoothly and in lockstep, independent * of how often (or how irregularly) progress data updates arrive. The animation * timers below only schedule re-renders; the displayed frame always comes from - * the clock. This fixes the frozen/stuttering spinner from issue #1084 while - * keeping per-frame diffs to a single glyph cell so the differential renderer - * never needs a full-clear (no flicker). + * the clock. This fixes the frozen/stuttering spinner from issue #1084. + * + * IMPORTANT: a wall-clock spinner only stays flicker-free for widgets pinned to + * the bottom of the buffer (e.g. the below-editor async widget), where every + * tick stays inside the viewport. Content rendered into chat scrollback (live + * foreground subagent results) can scroll above the viewport fold; there, even + * a single-cell spinner diff forces pi-tui into a destructive full-screen + + * scrollback clear on every tick. Such surfaces must NOT animate on a timer — + * see pulseGlyph(), which is advanced once per real progress update instead. */ export const RUNNING_ANIMATION_MS = 80; @@ -106,6 +112,23 @@ export function runningGlyph(seed?: number, now?: number): string { return RUNNING_FRAMES[Math.abs(animatedSeed) % RUNNING_FRAMES.length]!; } +export const PULSE_FRAMES = ["·", "•", "●", "•"]; + +/** + * Activity "heartbeat" glyph for live foreground subagent results. Unlike + * runningGlyph(), the frame is NOT derived from wall-clock time: the caller + * advances `frame` exactly once per real progress update (see + * renderLiveSubagentResult). With no animation timer, the only line diffs this + * produces coincide with progress data that genuinely changed, so the pulse can + * live in chat scrollback (above or below the fold) without ever triggering + * pi-tui's full-screen/scrollback clear. Returns a steady breathing dot that + * grows and settles as the subagent reports activity. + */ +export function pulseGlyph(frame?: number): string { + const index = Number.isFinite(frame) ? Math.abs(Math.trunc(frame as number)) : 0; + return PULSE_FRAMES[index % PULSE_FRAMES.length]!; +} + export function progressRunningSeed(progress: ProgressSeedSource | undefined): number | undefined { if (!progress) return undefined; return runningSeed( diff --git a/packages/subagents/src/tui/render-result-animation.ts b/packages/subagents/src/tui/render-result-animation.ts index 1f135db19..b7d438c90 100644 --- a/packages/subagents/src/tui/render-result-animation.ts +++ b/packages/subagents/src/tui/render-result-animation.ts @@ -1,5 +1,3 @@ -import { RUNNING_ANIMATION_MS } from "./render-layout.ts"; - type ResultAnimationTimer = ReturnType; export interface SubagentResultRenderState { @@ -8,8 +6,8 @@ export interface SubagentResultRenderState { subagentResultSnapshotKey?: string; /** Stable semantic/content timestamp used for durations and activity text. */ subagentResultSnapshotNow?: number; - /** Timer-driven timestamp used only for spinner glyph frames. */ - subagentResultSpinnerFrameNow?: number; + /** Monotonic pulse frame, advanced once per progress update (no timer). */ + subagentResultPulseFrame?: number; } export type ResultAnimationContext = { @@ -23,45 +21,27 @@ type LegacyResultAnimationContext = { }; }; -const activeResultAnimationTimers = new Map(); - +/** + * Legacy safety net for render state objects created by earlier timer-driven + * foreground result rendering. New code never schedules result timers, but + * clearing the field prevents a stale interval from surviving across upgrades. + */ export function clearResultAnimationTimer(context: LegacyResultAnimationContext): void { const timer = context.state.subagentResultAnimationTimer; - if (timer) { - clearInterval(timer); - activeResultAnimationTimers.delete(timer); - } + if (timer) clearInterval(timer); context.state.subagentResultAnimationTimer = undefined; context.state.subagentResultAnimationCleanup = undefined; } -export function clearLegacyResultAnimationTimer(context: LegacyResultAnimationContext): void { - clearResultAnimationTimer(context); +export function advanceResultPulseFrame(frame: number | undefined): number { + return (frame ?? 0) + 1; } -export function ensureResultAnimation(context: ResultAnimationContext): void { - if (context.state.subagentResultAnimationTimer) return; - const timer = setInterval(() => { - context.state.subagentResultSpinnerFrameNow = Date.now(); - try { - context.invalidate(); - } catch { - clearResultAnimationTimer(context); - } - }, RUNNING_ANIMATION_MS); - timer.unref?.(); - context.state.subagentResultAnimationTimer = timer; - context.state.subagentResultAnimationCleanup = () => clearResultAnimationTimer(context); - activeResultAnimationTimers.set(timer, context.state); +export function clearLegacyResultAnimationTimer(context: LegacyResultAnimationContext): void { + clearResultAnimationTimer(context); } export function stopResultAnimations(): void { - for (const [timer, state] of activeResultAnimationTimers) { - clearInterval(timer); - if (state.subagentResultAnimationTimer === timer) { - state.subagentResultAnimationTimer = undefined; - state.subagentResultAnimationCleanup = undefined; - } - } - activeResultAnimationTimers.clear(); + // Retained for extension teardown compatibility; result rendering no longer + // registers global animation timers. } diff --git a/packages/subagents/src/tui/render-result-compact.ts b/packages/subagents/src/tui/render-result-compact.ts index df0d0b81a..c7d33aa57 100644 --- a/packages/subagents/src/tui/render-result-compact.ts +++ b/packages/subagents/src/tui/render-result-compact.ts @@ -2,7 +2,7 @@ import { Container, Text, type Component } from "@earendil-works/pi-tui"; import type { AgentProgress, AsyncJobStep, Details } from "../shared/types.ts"; import { shortenPath } from "../shared/formatters.ts"; import { getSingleResultOutput } from "../shared/utils.ts"; -import { getTermWidth, progressRunningSeed, runningGlyph, runningSeed, truncLine, type Theme } from "./render-layout.ts"; +import { getTermWidth, pulseGlyph, truncLine, type Theme } from "./render-layout.ts"; import { buildLiveStatusLine, compactCurrentActivity, @@ -25,7 +25,7 @@ import { } from "./render-chain-graph.ts"; import { modelThinkingBadge, widgetStepGlyph, widgetStepStatus } from "./render-event-formatting.ts"; -export function renderSingleCompact(d: Details, r: Details["results"][number], theme: Theme, now?: number, spinnerNow?: number): Component { +export function renderSingleCompact(d: Details, r: Details["results"][number], theme: Theme, now?: number, pulseFrame?: number): Component { const output = r.truncation?.text || getSingleResultOutput(r); const progress = r.progress || r.progressSummary; const isRunning = r.progress?.status === "running"; @@ -37,7 +37,7 @@ export function renderSingleCompact(d: Details, r: Details["results"][number], t const c = new Container(); const width = getTermWidth() - 4; const modelDisplay = modelThinkingBadge(theme, r.model, undefined, r.fastMode); - c.addChild(new Text(truncLine(`${resultGlyph(r, output, theme, isRunning, progressRunningSeed(r.progress ?? r.progressSummary), spinnerNow ?? now)} ${theme.fg("toolTitle", theme.bold(r.agent))}${modelDisplay}${contextBadge}${stats ? ` ${theme.fg("dim", "·")} ${stats}` : ""}`, width), 0, 0)); + c.addChild(new Text(truncLine(`${resultGlyph(r, output, theme, isRunning, pulseFrame)} ${theme.fg("toolTitle", theme.bold(r.agent))}${modelDisplay}${contextBadge}${stats ? ` ${theme.fg("dim", "·")} ${stats}` : ""}`, width), 0, 0)); if (isRunning && r.progress) { const progressSnapshotNow = snapshotNowForProgress(r.progress, now); @@ -61,7 +61,7 @@ export function renderSingleCompact(d: Details, r: Details["results"][number], t return c; } -export function renderMultiCompact(d: Details, theme: Theme, now?: number, spinnerNow?: number): Component { +export function renderMultiCompact(d: Details, theme: Theme, now?: number, pulseFrame?: number): Component { const hasRunning = d.progress?.some((p) => p.status === "running") || d.results.some((r) => r.progress?.status === "running") || workflowGraphHasStatus(d, ["running"]); @@ -87,7 +87,7 @@ export function renderMultiCompact(d: Details, theme: Theme, now?: number, spinn const itemTitle = multiLabel.itemTitle; const stats = statJoin(theme, [multiLabel.headerLabel, formatProgressStats(theme, totalSummary, true, now)]); const glyph = hasRunning - ? theme.fg("accent", runningGlyph(runningSeed(progressRunningSeed(totalSummary), d.currentStepIndex), spinnerNow ?? now)) + ? theme.fg("accent", pulseGlyph(pulseFrame)) : failed ? theme.fg("error", "✗") : paused @@ -133,7 +133,7 @@ export function renderMultiCompact(d: Details, theme: Theme, now?: number, spinn const rPending = rProg && "status" in rProg && rProg.status === "pending"; const stepNumber = r.progress?.index !== undefined ? r.progress.index + 1 : progressFromArray?.index !== undefined ? progressFromArray.index + 1 : i + 1; const stepStats = formatProgressStats(theme, rProg, true, now); - const glyph = rPending ? theme.fg("dim", "◦") : resultGlyph(r, output, theme, rRunning, progressRunningSeed(rProg), spinnerNow ?? now); + const glyph = rPending ? theme.fg("dim", "◦") : resultGlyph(r, output, theme, rRunning, pulseFrame); const pendingLabel = rPending ? ` ${theme.fg("dim", "· pending")}` : ""; const stepLabel = resultRowLabel(d, multiLabel, i, stepNumber); const line = `${glyph} ${stepLabel}: ${themeBold(theme, agentName)}${stepStats ? ` ${theme.fg("dim", "·")} ${stepStats}` : ""}${pendingLabel}`; diff --git a/packages/subagents/src/tui/render-result.ts b/packages/subagents/src/tui/render-result.ts index 0c42a03a8..f41b7b18f 100644 --- a/packages/subagents/src/tui/render-result.ts +++ b/packages/subagents/src/tui/render-result.ts @@ -5,10 +5,10 @@ import type { AgentProgress, AsyncJobStep, Details } from "../shared/types.ts"; import { formatDuration, formatTokens, formatUsage, shortenPath } from "../shared/formatters.ts"; import { getSingleResultOutput } from "../shared/utils.ts"; import { getTermWidth, truncLine, type Theme } from "./render-layout.ts"; -import { clearResultAnimationTimer, ensureResultAnimation, type ResultAnimationContext } from "./render-result-animation.ts"; +import { advanceResultPulseFrame, clearResultAnimationTimer, type ResultAnimationContext } from "./render-result-animation.ts"; import { renderMultiCompact, renderSingleCompact } from "./render-result-compact.ts"; import { buildChainRenderEntries, buildMultiProgressLabel, resultRowLabel, workflowGraphHasStatus, type ChainRenderEntry } from "./render-chain-graph.ts"; -import { isRunningSubagentResult, subagentResultRenderKey } from "./render-stable-output.ts"; +import { subagentResultRenderKey } from "./render-stable-output.ts"; import { modelThinkingBadge, widgetStepStatus } from "./render-event-formatting.ts"; import { buildLiveStatusLine, @@ -28,26 +28,27 @@ export function renderLiveSubagentResult( ): Component { const nextKey = subagentResultRenderKey(result, options); if (context.state.subagentResultSnapshotKey !== nextKey) { - const frameNow = Date.now(); context.state.subagentResultSnapshotKey = nextKey; - context.state.subagentResultSnapshotNow = frameNow; - context.state.subagentResultSpinnerFrameNow = frameNow; + context.state.subagentResultSnapshotNow = Date.now(); + // Advance the activity pulse exactly once per real progress update. + // Foreground subagent results render into chat scrollback, which can sit + // above the viewport fold. Animating on a timer there forces pi-tui into a + // destructive full-screen/scrollback clear on every tick (the flicker that + // scaled with widget height). Driving the pulse off genuine updates keeps + // the only line diffs tied to content that actually changed, so the + // differential renderer repaints exactly as it would for any progress + // update — no extra above-fold churn between updates. + context.state.subagentResultPulseFrame = advanceResultPulseFrame(context.state.subagentResultPulseFrame); } context.state.subagentResultSnapshotNow ??= Date.now(); - context.state.subagentResultSpinnerFrameNow ??= context.state.subagentResultSnapshotNow; - // Foreground subagent results render inside chat scrollback. Keep semantic - // content time stable between tool/progress updates, but let the spinner tick - // independently. That limits timer-driven diffs to spinner glyph cells instead - // of updating elapsed/tool/activity text and causing broad chatbox churn. - if (options.isPartial && isRunningSubagentResult(result)) { - ensureResultAnimation(context); - } else { - clearResultAnimationTimer(context); - } + context.state.subagentResultPulseFrame ??= 0; + // Never schedule timer-driven re-renders for scrollback content; clear any + // stale timer a previous version may have installed for this render slot. + clearResultAnimationTimer(context); return renderSubagentResult(result, { ...options, now: context.state.subagentResultSnapshotNow, - spinnerNow: context.state.subagentResultSpinnerFrameNow, + pulseFrame: context.state.subagentResultPulseFrame, }, theme); } @@ -56,7 +57,7 @@ export function renderLiveSubagentResult( */ export function renderSubagentResult( result: AgentToolResult
, - options: { expanded: boolean; now?: number; spinnerNow?: number }, + options: { expanded: boolean; now?: number; pulseFrame?: number }, theme: Theme, ): Component { const d = result.details; @@ -72,7 +73,7 @@ export function renderSubagentResult( if (d.mode === "single" && d.results.length === 1) { const r = d.results[0]; - if (!expanded) return renderSingleCompact(d, r, theme, options.now, options.spinnerNow); + if (!expanded) return renderSingleCompact(d, r, theme, options.now, options.pulseFrame); const isRunning = r.progress?.status === "running"; const icon = isRunning ? theme.fg("warning", "running") @@ -166,7 +167,7 @@ export function renderSubagentResult( return c; } - if (!expanded) return renderMultiCompact(d, theme, options.now, options.spinnerNow); + if (!expanded) return renderMultiCompact(d, theme, options.now, options.pulseFrame); const hasRunning = d.progress?.some((p) => p.status === "running") || d.results.some((r) => r.progress?.status === "running") diff --git a/packages/subagents/src/tui/render-status-progress.ts b/packages/subagents/src/tui/render-status-progress.ts index 8017ae0af..187f55d1d 100644 --- a/packages/subagents/src/tui/render-status-progress.ts +++ b/packages/subagents/src/tui/render-status-progress.ts @@ -2,7 +2,7 @@ import type { AgentProgress, Details } from "../shared/types.ts"; import { formatDuration, formatTokens, formatToolCall } from "../shared/formatters.ts"; import { getDisplayItems } from "../shared/utils.ts"; import { formatActivityLabel } from "../shared/status-format.ts"; -import { getTermWidth, progressRunningSeed, runningGlyph, type Theme } from "./render-layout.ts"; +import { getTermWidth, pulseGlyph, type Theme } from "./render-layout.ts"; export function extractOutputTarget(task: string): string | undefined { const writeToMatch = task.match(/\[Write to:\s*([^\]\n]+)\]/i); @@ -118,8 +118,8 @@ export function resultStatusLine(result: Details["results"][number], output: str return "Done"; } -export function resultGlyph(result: Details["results"][number], output: string, theme: Theme, running = result.progress?.status === "running", seed = progressRunningSeed(result.progress ?? result.progressSummary), now?: number): string { - if (running) return theme.fg("accent", runningGlyph(seed, now)); +export function resultGlyph(result: Details["results"][number], output: string, theme: Theme, running = result.progress?.status === "running", pulseFrame?: number): string { + if (running) return theme.fg("accent", pulseGlyph(pulseFrame)); if (result.detached) return theme.fg("warning", "■"); if (result.interrupted) return theme.fg("warning", "■"); if (result.exitCode !== 0) return theme.fg("error", "✗"); diff --git a/packages/subagents/src/tui/render.ts b/packages/subagents/src/tui/render.ts index c03dfad90..e160a1add 100644 --- a/packages/subagents/src/tui/render.ts +++ b/packages/subagents/src/tui/render.ts @@ -5,11 +5,11 @@ * rendering responsibility across sibling modules. */ -export { RUNNING_ANIMATION_MS, currentRunningFrame } from "./render-layout.ts"; +export { PULSE_FRAMES, RUNNING_ANIMATION_MS, RUNNING_FRAMES, currentRunningFrame, pulseGlyph } from "./render-layout.ts"; export { + advanceResultPulseFrame, clearLegacyResultAnimationTimer, clearResultAnimationTimer, - ensureResultAnimation, stopResultAnimations, } from "./render-result-animation.ts"; export type { SubagentResultRenderState } from "./render-result-animation.ts"; diff --git a/test/integration/overlay-resume-regressions.test.ts b/test/integration/overlay-resume-regressions.test.ts index c1d611aed..9e133a3a0 100644 --- a/test/integration/overlay-resume-regressions.test.ts +++ b/test/integration/overlay-resume-regressions.test.ts @@ -207,11 +207,12 @@ describe("/workflow resume — durable regression coverage", () => { }); test("combined picker resolves live selection before dispose", async () => { - const liveRunId = `live-select-${Date.now()}`; - singletonStore.recordRunStart({ id: liveRunId, name: "live-select-wf", inputs: {}, status: "running", stages: [], startedAt: Date.now() }); - singletonStore.recordRunPaused(liveRunId); + const now = Date.now(); + const liveRunId = `live-select-${now}`; + singletonStore.recordRunStart({ id: liveRunId, name: "live-select-wf", inputs: {}, status: "running", stages: [], startedAt: now }); + singletonStore.recordRunPaused(liveRunId, now + 2); const backend = new InMemoryDurableBackend(); - backend.registerWorkflow({ workflowId: "durable-select-alongside", name: "durable-select", inputs: {}, createdAt: Date.now(), status: "paused", completedCheckpoints: 1 }); + backend.registerWorkflow({ workflowId: "durable-select-alongside", name: "durable-select", inputs: {}, createdAt: now, updatedAt: now + 1, status: "paused", completedCheckpoints: 1 }); setDurableBackend(backend); const { pi, commands } = buildMockPi(); factory(pi); @@ -227,8 +228,9 @@ describe("/workflow resume — durable regression coverage", () => { }); test("combined picker resumes failed live runs through continuation path", async () => { - const failedRunId = `failed-live-${Date.now()}`; - singletonStore.recordRunStart({ id: failedRunId, name: "missing-continuation-wf", inputs: {}, status: "running", stages: [], startedAt: Date.now() }); + const now = Date.now(); + const failedRunId = `failed-live-${now}`; + singletonStore.recordRunStart({ id: failedRunId, name: "missing-continuation-wf", inputs: {}, status: "running", stages: [], startedAt: now }); singletonStore.recordRunEnd(failedRunId, "failed", undefined, "recoverable", { failureRecoverability: "recoverable", failureDisposition: "terminal_failed", @@ -236,7 +238,7 @@ describe("/workflow resume — durable regression coverage", () => { resumable: true, }); const backend = new InMemoryDurableBackend(); - backend.registerWorkflow({ workflowId: "durable-with-failed-live", name: "durable-select", inputs: {}, createdAt: Date.now(), status: "paused", completedCheckpoints: 1 }); + backend.registerWorkflow({ workflowId: "durable-with-failed-live", name: "durable-select", inputs: {}, createdAt: now - 2, updatedAt: now - 1, status: "paused", completedCheckpoints: 1 }); setDurableBackend(backend); const { pi, commands } = buildMockPi(); factory(pi); @@ -249,7 +251,7 @@ describe("/workflow resume — durable regression coverage", () => { await handlerPromise; const joined = messages.join("\n"); - assert.match(joined, /Workflow definition not found|Cannot resume failed run|missing-continuation-wf/); + assert.match(joined, /missing-continuation-wf/); assert.doesNotMatch(joined, /Snapshot available/); }); }); diff --git a/test/unit/stage-chat-view-15.test.ts b/test/unit/stage-chat-view-15.test.ts index 67487abb3..e964978fe 100644 --- a/test/unit/stage-chat-view-15.test.ts +++ b/test/unit/stage-chat-view-15.test.ts @@ -159,19 +159,17 @@ describe("StageChatView terminal subagent cleanup regressions", () => { view.dispose(); }); - test("terminal cleanup stops the rendered subagent result animation interval", () => { + test("rendering a running subagent installs no animation interval (update-driven pulse)", () => { stopResultAnimations(); const originalSetInterval = globalThis.setInterval; const originalClearInterval = globalThis.clearInterval; const activeIntervals = new Set[0]>(); - let clearIntervalCalls = 0; globalThis.setInterval = ((handler: TimerHandler, timeout?: number, ...args: unknown[]) => { const timer = originalSetInterval(handler, timeout, ...args); activeIntervals.add(timer as Parameters[0]); return timer; }) as typeof setInterval; globalThis.clearInterval = ((timer?: Parameters[0]) => { - clearIntervalCalls++; activeIntervals.delete(timer); return originalClearInterval(timer); }) as typeof clearInterval; @@ -193,8 +191,11 @@ describe("StageChatView terminal subagent cleanup regressions", () => { emitRunningSubagent(emit); renderText(view); - assert.equal(activeIntervals.size, 1); - assert.equal(clearIntervalCalls, 0); + // The foreground subagent renderer must NOT spin up a wall-clock + // animation timer: its activity pulse is advanced once per progress + // update instead. A timer here would tick above pi-tui's viewport + // fold and force a destructive full-screen/scrollback clear (flicker). + assert.equal(activeIntervals.size, 0); const runningStage = store.runs()[0]!.stages[0]!; store.recordStageEnd("run-1", { @@ -204,10 +205,12 @@ describe("StageChatView terminal subagent cleanup regressions", () => { durationMs: 1, }); + // Terminating the stage and re-rendering must likewise leak no + // interval, and the host carries no animation tick of its own. + renderText(view); assert.equal(activeIntervals.size, 0); - assert.equal(clearIntervalCalls, 1); stopResultAnimations(); - assert.equal(clearIntervalCalls, 1); + assert.equal(activeIntervals.size, 0); assert.equal(view._hasAnimationTick, false); view.dispose(); } finally { diff --git a/test/unit/subagents-render-stability-helpers.ts b/test/unit/subagents-render-stability-helpers.ts index 45c87ef12..67cb67a97 100644 --- a/test/unit/subagents-render-stability-helpers.ts +++ b/test/unit/subagents-render-stability-helpers.ts @@ -2,7 +2,7 @@ import type { AgentToolResult } from "@earendil-works/pi-agent-core"; import type { Component } from "@earendil-works/pi-tui"; import type { ExtensionContext } from "@bastani/atomic"; import type { AsyncJobState, Details } from "../../packages/subagents/src/shared/types.js"; -import { renderSubagentResult } from "../../packages/subagents/src/tui/render.js"; +import { PULSE_FRAMES, renderSubagentResult, RUNNING_FRAMES } from "../../packages/subagents/src/tui/render.js"; export type { AgentToolResult, AsyncJobState, Component, Details, ExtensionContext }; @@ -14,8 +14,7 @@ export const theme = { bold: (value: string) => value, } as unknown as RenderTheme; -// Braille spinner frames used by the running glyph. Kept in sync with render.ts. -export const RUNNING_FRAMES = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; +export { PULSE_FRAMES, RUNNING_FRAMES }; const SPINNER_CHARS = new Set(RUNNING_FRAMES); export function withMockedNow(now: number, run: () => T): T { @@ -37,6 +36,13 @@ export function firstSpinnerChar(text: string): string | undefined { return undefined; } +const PULSE_CHARS = new Set(PULSE_FRAMES); + +export function firstPulseChar(text: string): string | undefined { + for (const char of text) if (PULSE_CHARS.has(char)) return char; + return undefined; +} + export function runningSingleResult(): AgentToolResult
{ return { content: [{ type: "text", text: "running" }], diff --git a/test/unit/subagents-render-stability-running-spinner.ts b/test/unit/subagents-render-stability-running-spinner.ts index fbc646b25..72cb6e89f 100644 --- a/test/unit/subagents-render-stability-running-spinner.ts +++ b/test/unit/subagents-render-stability-running-spinner.ts @@ -1,142 +1,85 @@ import { afterEach, describe, test } from "bun:test"; import assert from "node:assert/strict"; -import { renderLiveSubagentResult, renderSubagentResult, RUNNING_ANIMATION_MS, stopResultAnimations } from "../../packages/subagents/src/tui/render.js"; -import { type AgentToolResult, type Details, RUNNING_FRAMES, firstSpinnerChar, runningSingleResult, stripSpinnerChars, theme, withMockedNow } from "./subagents-render-stability-helpers.js"; -describe("subagent running spinner animation (issue #1084)", () => { - afterEach(() => { - stopResultAnimations(); - }); +import { + PULSE_FRAMES, + renderLiveSubagentResult, + renderSubagentResult, + pulseGlyph, + RUNNING_ANIMATION_MS, + stopResultAnimations, +} from "../../packages/subagents/src/tui/render.js"; +import { + type AgentToolResult, + type Details, + firstPulseChar, + runningSingleResult, + theme, + withMockedNow, +} from "./subagents-render-stability-helpers.js"; - test("running glyph advances with wall clock (no longer frozen)", () => { - const result = runningSingleResult(); +type LiveContext = Parameters[3]; - // Two renders exactly one animation frame apart must differ: the spinner - // is driven by wall-clock time, not by progress data changes. - const first = withMockedNow(10_000, () => - renderSubagentResult(result, { expanded: false }, theme) - .render(120) - .join("\n"), - ); - const second = withMockedNow(10_000 + RUNNING_ANIMATION_MS, () => - renderSubagentResult(result, { expanded: false }, theme) - .render(120) - .join("\n"), - ); +function freshContext(): LiveContext { + return { state: {}, invalidate: () => {} } as LiveContext; +} - assert.notEqual( - second, - first, - "running spinner should advance after one animation interval", - ); - }); +/** Simulate a genuine progress update (a tool use plus elapsed time). */ +function bumpProgress(result: AgentToolResult
): AgentToolResult
{ + return { + ...result, + details: { + ...result.details!, + results: result.details!.results.map((entry) => ({ + ...entry, + progress: entry.progress + ? { + ...entry.progress, + durationMs: entry.progress.durationMs + 1_000, + toolCount: entry.progress.toolCount + 1, + } + : entry.progress, + })), + }, + }; +} - // NOTE: this invariant assumes the render path only consults Date.now() for - // time (which the tests mock). If elapsed-time labels ever start reading - // performance.now()/process.uptime(), this assertion would start to drift. - test("renders within the same animation frame are identical (deterministic, no churn)", () => { - const result = runningSingleResult(); - const frameStart = 10_000; - - const a = withMockedNow(frameStart, () => - renderSubagentResult(result, { expanded: false }, theme) - .render(120) - .join("\n"), - ); - const b = withMockedNow(frameStart + RUNNING_ANIMATION_MS - 1, () => - renderSubagentResult(result, { expanded: false }, theme) - .render(120) - .join("\n"), - ); - - assert.equal( - b, - a, - "renders inside the same animation frame must be byte-identical", - ); +// Foreground subagent results render into chat scrollback, which can scroll +// above pi-tui's viewport fold. A wall-clock animation timer there forces a +// destructive full-screen/scrollback clear on every tick (the flicker that +// grew with widget height). The fix replaces the timer-driven spinner with a +// pulse that advances once per real progress update — so the only line diffs +// coincide with content that genuinely changed. +describe("subagent running pulse (foreground flicker fix)", () => { + afterEach(() => { + stopResultAnimations(); }); - test("foreground tool result timer changes only spinner glyphs", async () => { - const result = runningSingleResult(); - let invalidates = 0; - const context = { - state: {}, - invalidate: () => { - invalidates++; - }, - } as Parameters[3]; - - const firstLines = withMockedNow(10_000, () => + test("foreground running rows never install an animation timer", () => { + const context = freshContext(); + withMockedNow(10_000, () => renderLiveSubagentResult( - result, + runningSingleResult(), { expanded: false, isPartial: true }, theme, context, ).render(120), ); - assert.ok( - context.state.subagentResultAnimationTimer, - "running foreground rows should install a spinner-only timer", - ); - assert.equal(context.state.subagentResultSnapshotNow, 10_000); - assert.equal(context.state.subagentResultSpinnerFrameNow, 10_000); - - await new Promise((resolve) => - setTimeout(resolve, RUNNING_ANIMATION_MS + 40), - ); - assert.ok( - invalidates > 0, - "foreground spinner timer should invalidate for smooth glyph updates", - ); assert.equal( - context.state.subagentResultSnapshotNow, - 10_000, - "timer must not advance semantic/content time", - ); - assert.notEqual( - context.state.subagentResultSpinnerFrameNow, - 10_000, - "timer should advance spinner-only time", + context.state.subagentResultAnimationTimer, + undefined, + "foreground subagent rows must not run a wall-clock timer (above-fold ticks flicker)", ); - - // Pin the next spinner frame deterministically; the real timer assertion - // above proves the timer updates only spinnerFrameNow, while this render - // assertion proves such an update only changes spinner glyph cells. - context.state.subagentResultSpinnerFrameNow = - 10_000 + RUNNING_ANIMATION_MS; - const secondLines = renderLiveSubagentResult( - result, - { expanded: false, isPartial: true }, - theme, - context, - ).render(120); assert.equal( - secondLines.length, - firstLines.length, - "spinner tick must preserve row height", - ); - let changed = 0; - for (let i = 0; i < firstLines.length; i++) { - if (firstLines[i] === secondLines[i]) continue; - changed++; - assert.equal( - stripSpinnerChars(firstLines[i]!), - stripSpinnerChars(secondLines[i]!), - `line ${i} changed in non-spinner content between foreground spinner frames`, - ); - } - assert.ok( - changed > 0, - "expected spinner-only timer to advance at least one glyph", + context.state.subagentResultPulseFrame, + 1, + "the first render seeds the pulse at frame 1", ); + assert.equal(context.state.subagentResultSnapshotNow, 10_000); }); - test("foreground tool result captures a fresh frame on semantic progress updates", () => { + test("renders are byte-identical across wall-clock advances without a progress update", () => { const result = runningSingleResult(); - const context = { - state: {}, - invalidate: () => {}, - } as Parameters[3]; - + const context = freshContext(); const first = withMockedNow(10_000, () => renderLiveSubagentResult( result, @@ -147,27 +90,9 @@ describe("subagent running spinner animation (issue #1084)", () => { .render(120) .join("\n"), ); - assert.equal(context.state.subagentResultSnapshotNow, 10_000); - - const updated: AgentToolResult
= { - ...result, - details: { - ...result.details!, - results: result.details!.results.map((entry) => ({ - ...entry, - progress: entry.progress - ? { - ...entry.progress, - durationMs: entry.progress.durationMs + 1_000, - toolCount: entry.progress.toolCount + 1, - } - : entry.progress, - })), - }, - }; - const second = withMockedNow(10_000 + RUNNING_ANIMATION_MS, () => + const second = withMockedNow(10_000 + 8 * RUNNING_ANIMATION_MS, () => renderLiveSubagentResult( - updated, + result, { expanded: false, isPartial: true }, theme, context, @@ -175,34 +100,22 @@ describe("subagent running spinner animation (issue #1084)", () => { .render(120) .join("\n"), ); - - assert.equal( - context.state.subagentResultSnapshotNow, - 10_000 + RUNNING_ANIMATION_MS, - ); assert.equal( - context.state.subagentResultSpinnerFrameNow, - 10_000 + RUNNING_ANIMATION_MS, - ); - assert.notEqual( second, first, - "semantic progress updates should still refresh the foreground row", + "with no progress update the row must not change across host re-renders / time", ); - assert.ok( - context.state.subagentResultAnimationTimer, - "running semantic updates should keep the spinner-only timer installed", + assert.equal( + context.state.subagentResultPulseFrame, + 1, + "the pulse must not advance without a progress update", ); }); - test("foreground tool result reuses captured now across unrelated renderer calls", () => { - const result = runningSingleResult(); - const context = { - state: {}, - invalidate: () => {}, - } as Parameters[3]; - - const first = withMockedNow(10_000, () => + test("the pulse advances exactly once per progress update and refreshes the row", () => { + let result = runningSingleResult(); + const context = freshContext(); + const before = withMockedNow(10_000, () => renderLiveSubagentResult( result, { expanded: false, isPartial: true }, @@ -212,7 +125,10 @@ describe("subagent running spinner animation (issue #1084)", () => { .render(120) .join("\n"), ); - const second = withMockedNow(10_000 + RUNNING_ANIMATION_MS, () => + assert.equal(context.state.subagentResultPulseFrame, 1); + + result = bumpProgress(result); + const after = withMockedNow(10_000, () => renderLiveSubagentResult( result, { expanded: false, isPartial: true }, @@ -222,62 +138,78 @@ describe("subagent running spinner animation (issue #1084)", () => { .render(120) .join("\n"), ); - assert.equal( - second, - first, - "same foreground snapshot should stay stable until a semantic update advances now", + context.state.subagentResultPulseFrame, + 2, + "a progress update advances the pulse by exactly one", ); + assert.notEqual(after, before, "a progress update refreshes the foreground row"); }); - test("honours captured now so chatbox result rows do not tick on host re-renders", () => { - const result = runningSingleResult(); - const first = renderSubagentResult( - result, - { expanded: false, now: 10_000 }, - theme, - ) - .render(120) - .join("\n"); - const second = renderSubagentResult( - result, - { expanded: false, now: 10_000 + RUNNING_ANIMATION_MS }, - theme, - ) - .render(120) - .join("\n"); - assert.notEqual( - second, - first, - "sanity: running subagent result glyphs should still be sensitive to opts.now", - ); + test("the running glyph is a pulse frame that visibly changes on every update", () => { + let result = runningSingleResult(); + const context = freshContext(); + let previous: string | undefined; + for (let frame = 1; frame <= PULSE_FRAMES.length + 2; frame++) { + const out = withMockedNow(10_000, () => + renderLiveSubagentResult( + result, + { expanded: false, isPartial: true }, + theme, + context, + ) + .render(120) + .join("\n"), + ); + assert.equal(context.state.subagentResultPulseFrame, frame); + const glyph = firstPulseChar(out); + assert.ok(glyph, `expected a pulse glyph on update ${frame}`); + assert.equal( + glyph, + pulseGlyph(frame), + `glyph must match pulseGlyph(${frame})`, + ); + if (previous !== undefined) { + assert.notEqual( + glyph, + previous, + `update ${frame} must visibly change the pulse glyph`, + ); + } + previous = glyph; + result = bumpProgress(result); + } + }); - const stableA = withMockedNow(20_000, () => + test("the running glyph is decoupled from wall-clock time", () => { + const result = runningSingleResult(); + const a = withMockedNow(10_000, () => renderSubagentResult( result, - { expanded: false, now: 10_000 }, + { expanded: false, now: 10_000, pulseFrame: 2 }, theme, ) .render(120) .join("\n"), ); - const stableB = withMockedNow(30_000, () => + const b = withMockedNow(10_000 + 9 * RUNNING_ANIMATION_MS, () => renderSubagentResult( result, - { expanded: false, now: 10_000 }, + { expanded: false, now: 10_000, pulseFrame: 2 }, theme, ) .render(120) .join("\n"), ); assert.equal( - stableB, - stableA, - "a captured opts.now should keep chatbox rows byte-stable across host re-renders", + a, + b, + "same pulse frame + same captured now must render byte-identically regardless of wall-clock", ); + assert.equal(firstPulseChar(a), pulseGlyph(2)); }); - test("honours captured now for multi-agent compact chatbox rows", () => { + test("multi-agent compact rows stay stable until a progress update", () => { const base = runningSingleResult().details!.results[0]!; const parallel: AgentToolResult
= { content: [{ type: "text", text: "running parallel" }], @@ -289,11 +221,7 @@ describe("subagent running spinner animation (issue #1084)", () => { ...base, agent: "reviewer", task: "review", - progress: { - ...base.progress!, - agent: "reviewer", - index: 1, - }, + progress: { ...base.progress!, agent: "reviewer", index: 1 }, }, ], progress: [ @@ -303,117 +231,31 @@ describe("subagent running spinner animation (issue #1084)", () => { totalSteps: 2, }, }; - - const first = renderSubagentResult( - parallel, - { expanded: false, now: 10_000 }, - theme, - ) - .render(120) - .join("\n"); - const second = renderSubagentResult( - parallel, - { expanded: false, now: 10_000 + RUNNING_ANIMATION_MS }, - theme, - ) - .render(120) - .join("\n"); - assert.notEqual( - second, - first, - "sanity: multi-agent running glyphs should be sensitive to opts.now", - ); - - const stableA = withMockedNow(20_000, () => - renderSubagentResult( + const context = freshContext(); + const first = withMockedNow(10_000, () => + renderLiveSubagentResult( parallel, - { expanded: false, now: 10_000 }, + { expanded: false, isPartial: true }, theme, + context, ) .render(120) .join("\n"), ); - const stableB = withMockedNow(30_000, () => - renderSubagentResult( + const second = withMockedNow(10_000 + 5 * RUNNING_ANIMATION_MS, () => + renderLiveSubagentResult( parallel, - { expanded: false, now: 10_000 }, + { expanded: false, isPartial: true }, theme, + context, ) .render(120) .join("\n"), ); assert.equal( - stableB, - stableA, - "captured opts.now should keep multi-agent chatbox rows byte-stable", - ); - }); - - test("consecutive frames differ only in spinner glyph cells (minimal diff = no flicker)", () => { - const result = runningSingleResult(); - - const firstLines = withMockedNow(10_000, () => - renderSubagentResult(result, { expanded: false }, theme).render( - 120, - ), - ); - const secondLines = withMockedNow(10_000 + RUNNING_ANIMATION_MS, () => - renderSubagentResult(result, { expanded: false }, theme).render( - 120, - ), - ); - - assert.equal( - firstLines.length, - secondLines.length, - "line count must stay stable across animation frames", - ); - - let changedLines = 0; - for (let i = 0; i < firstLines.length; i++) { - if (firstLines[i] === secondLines[i]) continue; - changedLines++; - // The only thing that may change between frames is the spinner glyph. - assert.equal( - stripSpinnerChars(firstLines[i]!), - stripSpinnerChars(secondLines[i]!), - `line ${i} changed in non-spinner content between animation frames`, - ); - } - assert.ok( - changedLines > 0, - "expected at least one spinner line to animate", - ); - }); - - test("running glyph cycles through frames in order over a full period", () => { - const result = runningSingleResult(); - const sequence: string[] = []; - for (let frame = 0; frame <= RUNNING_FRAMES.length; frame++) { - const out = withMockedNow(frame * RUNNING_ANIMATION_MS, () => - renderSubagentResult(result, { expanded: false }, theme) - .render(120) - .join("\n"), - ); - const glyph = firstSpinnerChar(out); - assert.ok(glyph, `expected a spinner glyph at frame ${frame}`); - sequence.push(glyph!); - } - // Every distinct frame is visited... - assert.equal( - new Set(sequence).size, - RUNNING_FRAMES.length, - "spinner should visit every frame", + second, + first, + "multi-agent compact rows must be byte-stable across host re-renders without updates", ); - // ...and each step advances to the cyclic successor in RUNNING_FRAMES order. - for (let i = 1; i < sequence.length; i++) { - const prev = RUNNING_FRAMES.indexOf(sequence[i - 1]!); - const cur = RUNNING_FRAMES.indexOf(sequence[i]!); - assert.equal( - cur, - (prev + 1) % RUNNING_FRAMES.length, - `frame ${i} did not advance by exactly one step`, - ); - } }); });