fix(subagents): drive live result indicator off progress, not a timer

[flora131]

Summary

Eliminates foreground subagent widget flicker that appeared once a running subagent panel grew tall enough to reach or exceed the terminal viewport, by replacing the 80ms wall-clock spinner with a progress-driven pulse glyph.

Problem

The live compact result animated its spinner on an 80ms wall-clock timer (ensureResultAnimation). 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 — strobing the indicator even when nothing about the run had changed. The flicker scaled with widget height: the taller the subagent panel, the more pronounced the strobing.

Root Cause

ensureResultAnimation() scheduled a recurring setInterval(…, 80) and stored the result in subagentResultSpinnerFrameNow / subagentResultAnimationTimer. Because the animated glyph lived in chat scrollback (not the pinned-to-bottom async widget), pi-tui had no way to do a partial repaint — it had to do a full-screen + scrollback clear on every 80ms tick to update a single glyph cell above the viewport fold.

Fix

Replace the wall-clock spinnerNow path with a pulseFrame counter that advances once per real progress update (driven by snapshot.version). Because the only line diffs now coincide with content that genuinely changed, the differential renderer repaints exactly as it would for any progress update — no extra above-fold churn between updates.

Key Changes

• render-layout.ts: Add pulseGlyph(frame?) — a heartbeat glyph (·, •, ●, •) whose frame is a monotonic counter, not a timestamp. Export RUNNING_FRAMES and PULSE_FRAMES for test assertions. Add inline doc explaining when to use pulseGlyph vs. runningGlyph (scrollback vs. pinned-to-bottom).
• render-result-animation.ts: Remove ensureResultAnimation(), activeResultAnimationTimers, and the subagentResultSpinnerFrameNow timer-driven field; replace with subagentResultPulseFrame (monotonic counter, no timer) and advanceResultPulseFrame(). stopResultAnimations() is now a no-op stub kept for extension teardown compatibility.
• render-result.ts: Advance subagentResultPulseFrame once per key change (i.e., per progress update) instead of scheduling interval re-renders. Always call clearResultAnimationTimer to evict any stale timer a prior version may have installed.
• render-result-compact.ts: Replace spinnerNow parameter with pulseFrame; use pulseGlyph(pulseFrame) for both single and multi-agent running glyphs.
• render-status-progress.ts: Update resultGlyph() signature — drop seed/now in favour of pulseFrame, delegate to pulseGlyph.
• extension/index.ts: Thread pulseFrame: advanceResultPulseFrame(pulseFrame) into rebuildSlashResultContainer (advanced once per snapshot.version change) so the slash-result component also drives off progress, not a timer.
• Tests: Replace timer-sensitivity assertions with progress-driven equivalents — verify the pulse glyph advances once per update, stays byte-stable across host re-renders without updates, and is decoupled from wall-clock time. Verify zero setInterval calls are made for a running foreground subagent.

Testing

• bun run lint ✅
• bun run check:file-length ✅
• bun run test:unit ✅ (all pass via pre-commit hook)

Notes

CHANGELOG entry added under @bastani/subagents [Unreleased] → Fixed.

[claude]

Review — fix(subagents): drive live result indicator off progress, not a timer

Solid, well-scoped fix. Net -138 lines, the root-cause writeup is excellent, and the key design distinction is correct: bottom-pinned widget rows (always in-viewport) keep the wall-clock runningGlyph, while scrollback content (foreground subagent results, which can scroll above pi-tui viewport fold) now uses the update-driven pulseGlyph. The pulseFrame?: number thread is consistent across every call site (extension/index.ts -> renderSubagentResult -> renderSingle/MultiCompact -> resultGlyph), and the tests were genuinely rewritten to assert the new invariant (no timer installed, pulse advances once per progress update, byte-stable across wall-clock advances with no update) rather than just deleted. File-length gate and CHANGELOG conventions are respected.

I was unable to run bun/git in this environment (sandbox denied), so the below is static review; the PR reports lint / check:file-length / test:unit green.

Worth confirming (UX tradeoff)

• The indicator now advances ONLY on a real progress update. For a subagent sitting in a single long-running, quiet tool call (no progress events for, say, 20-30s), the foreground row will be visually static. Previously the wall-clock spinner reassured the user during those gaps; a frozen glyph can read as hung. This is the deliberate tradeoff the PR makes to kill the flicker, and it is the right call for above-fold scrollback — just flagging it for an explicit human sign-off since it changes the liveness signal, not just the rendering mechanism.

Cleanup suggestions (non-blocking)

1. Now-dead timer registry. With ensureResultAnimation removed, nothing ever populates activeResultAnimationTimers anymore, so stopResultAnimations() (still called at extension/index.ts:209,465) is a permanent no-op, and clearResultAnimationTimer / clearLegacyResultAnimationTimer only clear a subagentResultAnimationTimer field that nothing sets. Keeping them as a defensive guard against legacy state objects is defensible, but consider either a follow-up to prune the map + stopResultAnimations, or a one-line comment noting they are retained purely as a legacy safety net — otherwise the next reader will assume they are load-bearing.

2. Two sources of pulseFrame. createSlashResultComponent passes pulseFrame: snapshot.version, while renderLiveSubagentResult maintains its own incrementing subagentResultPulseFrame counter. Both are correct mod 4, but it is the same advance-once-per-update idea expressed two ways — minor duplication worth centralizing if these paths ever diverge.

3. PULSE_FRAMES duplicated in tests. The test helper re-declares PULSE_FRAMES with a keep-in-sync-with-pulseGlyph() comment because the const is not exported from render-layout.ts. RUNNING_FRAMES is already exported and consumed by tests; exporting PULSE_FRAMES the same way would give the test a single source of truth and remove the drift risk.

Nits

• The changelog entry does not link a tracking issue, while the three sibling Fixed entries do (1517/1518/1527). If an issue exists, add it per the CLAUDE.md attribution rule.
• pulseGlyph uses frame-as-number inside the Number.isFinite(frame) guard — harmless, but the cast hints the param could be typed more tightly.

Overall: clean, correct, and the test rewrite gives me good confidence. Recommend addressing the dead-timer-registry note (at least with a comment) and confirming the quiet-period liveness tradeoff is acceptable before merge.

[claude]

Review — fix(subagents): drive live result indicator off progress, not a timer

Nice, well-scoped fix with a clear root-cause analysis. The core insight is sound: content rendered into chat scrollback can scroll above pi-tui's viewport fold, where any per-tick diff (even a single glyph cell) forces a destructive full-screen + scrollback clear, so an 80ms wall-clock spinner on that surface strobes. Driving the indicator off snapshot.version so line diffs only coincide with genuine content changes is the right call. I especially like that the wall-clock runningGlyph/RUNNING_ANIMATION_MS path is retained for the bottom-pinned async widget (render-widget.ts, render-event-formatting.ts), where every tick stays in the viewport — the two surfaces have genuinely different constraints and the comments now document that distinction well.

I verified the changes are internally consistent (no dangling references to ensureResultAnimation / subagentResultSpinnerFrameNow / spinnerNow remain in src), and that file lengths stay well under the 500-line gate. I was not able to run bun run typecheck / bun test in this environment (sandbox restrictions), so please confirm CI is green — the PR notes it passes.

Observations / suggestions (mostly minor)

1. UX tradeoff worth a sanity check (intentional, but flag it). The pulse now advances only on real progress updates. A foreground subagent that runs for a stretch without emitting any progress (e.g. a long single model turn with no tool calls / token deltas) will show a frozen pulse — the pre-TUI flickers and spinner freezes in the subagent component while subagents are running #1084 "looks hung" behavior, scoped to this one surface. The flicker is clearly worse than a frozen dot, so the trade is defensible; just worth confirming progress updates arrive frequently enough in practice (token/tool/elapsed deltas) that a running agent never looks stalled. If elapsed-time text is part of the render key it'll tick regularly; if not, consider whether a low-frequency heartbeat is warranted.

2. stopResultAnimations() is now a no-op but still exported and called (extension/index.ts:211,467). Fine for teardown compatibility, but the old global-sweep semantics are gone: the activeResultAnimationTimers registry was removed, so a stale timer on a render slot never re-rendered after an in-process upgrade would no longer be swept (only renderLiveSubagentResult re-renders clear it via clearResultAnimationTimer). Purely theoretical since new code never creates result timers, but a one-line comment on the call sites (or removing the dead calls) would avoid future confusion.

3. Dead test helpers. With the spinner test rewritten, firstSpinnerChar, stripSpinnerChars, and the SPINNER_CHARS set in test/unit/subagents-render-stability-helpers.ts no longer appear referenced by any test. They survive noUnusedLocals because they're exported, but they're now dead — consider removing them (firstPulseChar is a clean replacement).

4. PULSE_FRAMES[0] (·) collides with the dim separator glyph. Compact rows use theme.fg("dim", "·") as a stat separator, and frame 0 of the pulse is also ·. In practice disambiguated by color (accent vs dim) and by position (the glyph is always first on the line, which firstPulseChar relies on), so cosmetic only — but if a future refactor moves the glyph off the line start, firstPulseChar would start matching separators. A brief comment that it assumes leading-glyph position would harden the helper.

5. CHANGELOG entry has no issue/PR link unlike its neighboring Fixed entries (Enhancement: remove flicker from the subagent UI #1517, Bug: subagent spinner gets stuck in workflow chat sessions #1518, GitHub Copilot Claude can reject replay after repeated subagent runs due to orphaned tool_result #1527). If there's a tracking issue for this flicker, per CLAUDE.md's attribution convention it'd be good to link it.

Test coverage

Good coverage of the new contract: no timer installed on running rows, byte-stable across wall-clock advances without a progress update, pulse advances exactly once per update, glyph visibly changes each update, and decoupling from wall-clock. The stage-chat-view-15 assertion flip (activeIntervals.size === 0) directly pins the regression. Removing the old runningGlyph cycle/frame-order tests is appropriate since that path no longer drives this surface (and stays covered for the widget surface).

Overall: looks good to merge once CI is confirmed green; the items above are non-blocking polish.

[claude]

Code Review — fix(subagents): drive live result indicator off progress, not a timer

Reviewed the full diff plus surrounding code (render-result*.ts, render-layout.ts, the host tool-execution.ts disposal path, and the rewritten tests). This is a well-scoped, well-documented fix with a convincing root-cause story. The mechanical refactor is consistent and I found no correctness bugs. Notes below, mostly minor / discussion.

Strengths

• Clean teardown of the timer machinery. ensureResultAnimation/activeResultAnimationTimers/subagentResultSpinnerFrameNow are fully removed with no dangling references (verified by grep). All resultGlyph/renderSingleCompact/renderMultiCompact callers were updated in lockstep, so the signature changes are internally consistent.
• Good upgrade safety. renderLiveSubagentResult now unconditionally calls clearResultAnimationTimer, and the host tool-execution.ts still defensively clears subagentResultAnimationTimer/subagentResultAnimationCleanup from rendererState. So a stale interval installed by a pre-upgrade render state can't survive — nice.
• The new invariant is the right thing to test. Asserting setInterval is never called for a running foreground row (stage-chat-view-15.test.ts) directly encodes the property the fix is about, rather than a proxy. The rewritten stability tests (pulse advances exactly once per progress update, byte-stable across wall-clock advances) are clear.
• Excellent inline docs in render-layout.ts distinguishing pulseGlyph (scrollback) vs runningGlyph (pinned-to-bottom). That comment will save the next person from "fixing" it back into a timer.

Discussion / potential concerns

1. UX trade-off: the indicator freezes during quiet periods. Because the pulse only advances on a real snapshot.version / render-key change, a subagent that is genuinely running but emits no progress for a while (e.g. a single long tool call with no intermediate updates) will show a static glyph. Previously the wall-clock spinner gave continuous "still alive" feedback. This is an intentional flicker-vs-liveness trade and probably the right call for scrollback content, but worth confirming it's acceptable for long-running, low-chatter agents.

2. Multi-agent rows now pulse in lockstep. renderMultiCompact previously derived a per-row seed (progressRunningSeed(rProg)), so each running row could be at a different phase. Now every running row shares the single pulseFrame, so they all show the identical glyph and advance together. Cosmetic, almost certainly fine — just flagging the visible behavior change.

3. PULSE_FRAMES = ["·", "•", "●", "•"] repeats •. The "visibly changes on every update" test only passes because the two • entries are non-adjacent in the cycle (separated by ●). That's a deliberate grow/settle breathing pattern (·→•→●→•→·), but it's subtle — a one-line comment on the constant would stop a future refactor from "deduping" it down to 3 frames and breaking the consecutive-difference property.

4. Out-of-scope test changes. test/integration/overlay-resume-regressions.test.ts gains explicit now/updatedAt timestamps and narrows an assertion from /Workflow definition not found|Cannot resume failed run|missing-continuation-wf/ down to just /missing-continuation-wf/. These look like determinism fixes unrelated to the flicker work and aren't mentioned in the PR description. Could you confirm they're needed here (vs. a separate PR), and that the narrowed assertion isn't masking a regression in the other two branches it used to accept?

5. Dead test helpers. After this PR, firstSpinnerChar, stripSpinnerChars, and SPINNER_CHARS in subagents-render-stability-helpers.ts have no remaining consumers (only RUNNING_FRAMES/PULSE_FRAMES/firstPulseChar are used). They survive noUnusedLocals only because they're exports. Consider removing them to avoid bit-rot.

6. Nit — changelog. The new [Unreleased] → Fixed entry has no ([#…]) link, unlike its siblings. Per CLAUDE.md convention, link the issue/PR if one exists.

Verification

• Confirmed no lingering references to the removed symbols across packages/ and test/.
• Confirmed all resultGlyph call sites updated; no unused imports introduced in render-result-compact.ts.
• I was unable to run bun run typecheck / bun test in this environment (sandbox approval), so I could not execute the suite — the PR reports lint, check:file-length, and test:unit green via pre-commit. Static review found nothing that should fail typecheck.

Overall: solid fix, good docs and tests. Main asks are confirming the unrelated integration-test changes (#4) and the two small cleanups (#3, #5).