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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user