From b85f59991726c33347152aa4fe8875e8dffc86ff Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:38:37 +0900 Subject: [PATCH] fix: phase-lock tool-call spinners across parallel calls Each live ToolExecutionComponent advanced its spinner glyph from its own per-instance start time, so concurrent tool spinners showed different frames and read as janky. Derive the glyph from a single shared monotonic clock (sharedSpinnerFrame) so every live block animates in lockstep; the per-instance interval still drives requestRender. The unused #lastSpinnerAdvanceAt anchor is removed; the todo-strike counter is untouched. --- packages/coding-agent/CHANGELOG.md | 4 +++ .../src/modes/components/tool-execution.ts | 35 +++++++------------ .../test/theme-spinner-frames.test.ts | 16 +++++++++ 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 7fc96cb5e..3d464d239 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed tool-call spinners animating out of phase across parallel tool calls — each live tool block advanced its glyph from its own per-instance start time, so concurrent spinners showed different frames. Glyphs now derive from a single shared monotonic clock (`sharedSpinnerFrame`), keeping every live block in lockstep. + ## [15.12.5] - 2026-06-13 ### Changed diff --git a/packages/coding-agent/src/modes/components/tool-execution.ts b/packages/coding-agent/src/modes/components/tool-execution.ts index 8d659cb1d..d053a524c 100644 --- a/packages/coding-agent/src/modes/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/components/tool-execution.ts @@ -148,10 +148,16 @@ export interface ToolExecutionHandle { /** Drive pending-tool redraws at 30fps for live tool headers and displaceable * poll blocks. The TUI throttles at the same cadence, and static frames diff to * a no-op redraw at ~zero cost. */ -const SPINNER_RENDER_INTERVAL_MS = 1000 / 30; +export const SPINNER_RENDER_INTERVAL_MS = 1000 / 30; /** Advance the spinner glyph at its classic ~12.5fps step, decoupled from the * render cadence (mirrors `Loader`). */ -const SPINNER_GLYPH_ADVANCE_MS = 80; +export const SPINNER_GLYPH_ADVANCE_MS = 80; + +/** Phase-locked spinner glyph index shared by every live tool block so parallel + * spinners advance in lockstep instead of each tracking its own start time. */ +export function sharedSpinnerFrame(frameCount: number, now: number = performance.now()): number { + return frameCount > 0 ? Math.floor(now / SPINNER_GLYPH_ADVANCE_MS) % frameCount : 0; +} // Stable per-instance counter so each tool execution's inline images get a // graphics id that survives child re-creation (the image budget keys off it). @@ -196,7 +202,6 @@ export class ToolExecutionComponent extends Container { // Spinner animation for partial task results #spinnerFrame?: number; #spinnerInterval?: NodeJS.Timeout; - #lastSpinnerAdvanceAt = 0; // Todo write completion strikethrough reveal animation #todoStrikeInterval?: NodeJS.Timeout; // Track if args are still being streamed (for edit/write spinner) @@ -470,32 +475,18 @@ export class ToolExecutionComponent extends Container { // once the block leaves the live region. const needsSpinner = isStreamingArgs || isPartialTask || this.isDisplaceableBlock(); if (needsSpinner && !this.#spinnerInterval) { - const now = performance.now(); const frameCount = theme.spinnerFrames.length; - this.#lastSpinnerAdvanceAt = now; - if (frameCount > 0 && this.#spinnerFrame === undefined) { - this.#spinnerFrame = 0; - this.#renderState.spinnerFrame = 0; - } + const frame = sharedSpinnerFrame(frameCount); + this.#spinnerFrame = frame; + this.#renderState.spinnerFrame = frame; this.#spinnerInterval = setInterval(() => { // If a detached task interval from an older render path is still live, // stop it the instant the block leaves the repaintable region. if (this.#maybeFreezeBackgroundTask()) return; const now = performance.now(); const frameCount = theme.spinnerFrames.length; - // Redraw at 30fps, but keep the spinner glyph phase-locked to its - // classic ~12.5fps cadence. Advancing the anchor by elapsed frames - // instead of resetting to `now` avoids the 30fps timer quantizing the - // glyph down to one step every three ticks. - if (frameCount > 0) { - const elapsed = now - this.#lastSpinnerAdvanceAt; - if (elapsed >= SPINNER_GLYPH_ADVANCE_MS) { - const steps = Math.floor(elapsed / SPINNER_GLYPH_ADVANCE_MS); - this.#spinnerFrame = ((this.#spinnerFrame ?? 0) + steps) % frameCount; - this.#renderState.spinnerFrame = this.#spinnerFrame; - this.#lastSpinnerAdvanceAt += steps * SPINNER_GLYPH_ADVANCE_MS; - } - } + this.#spinnerFrame = sharedSpinnerFrame(frameCount, now); + this.#renderState.spinnerFrame = this.#spinnerFrame; this.#ui.requestRender(); }, SPINNER_RENDER_INTERVAL_MS); } else if (!needsSpinner && this.#spinnerInterval) { diff --git a/packages/coding-agent/test/theme-spinner-frames.test.ts b/packages/coding-agent/test/theme-spinner-frames.test.ts index 07a9857dd..dd421ec27 100644 --- a/packages/coding-agent/test/theme-spinner-frames.test.ts +++ b/packages/coding-agent/test/theme-spinner-frames.test.ts @@ -2,6 +2,10 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import { + SPINNER_GLYPH_ADVANCE_MS, + sharedSpinnerFrame, +} from "@oh-my-pi/pi-coding-agent/modes/components/tool-execution"; import { getThemeByName } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import { getConfigRootDir, getCustomThemesDir, setAgentDir } from "@oh-my-pi/pi-utils"; @@ -87,4 +91,16 @@ describe("theme symbols.spinnerFrames", () => { expect(status.length).toBeGreaterThan(1); expect(status).not.toContain("A"); }); + + it("derives live tool spinner frames from a shared clock", () => { + const frameCount = 4; + const now = SPINNER_GLYPH_ADVANCE_MS * 3 + 12; + + expect(sharedSpinnerFrame(frameCount, now)).toBe(sharedSpinnerFrame(frameCount, now)); + expect(sharedSpinnerFrame(frameCount, now + SPINNER_GLYPH_ADVANCE_MS)).toBe( + (sharedSpinnerFrame(frameCount, now) + 1) % frameCount, + ); + expect(sharedSpinnerFrame(frameCount, SPINNER_GLYPH_ADVANCE_MS * frameCount)).toBe(0); + expect(sharedSpinnerFrame(0, now)).toBe(0); + }); });