smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #8733: fix(tui): consolidate live tool spinner timers into one shared ticker (@roboomp)

Browse Source
This commit is contained in:
can1357
2026-08-19 01:36:04 +02:00
parent 79de0acd44 49f22987d5
commit b0bfe7e457
3 changed files with 99 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -31,6 +31,9 @@
### Fixed
- Fixed `omp --smoke-test` recursively deleting unrelated directories in `os.tmpdir()` (tmux/ssh sockets, editor state, build trees). The smoke broker now keeps its runtime dir under a private parent, and the dead-scope reclaim refuses any root that is not the `daemons` container and only prunes entries named like a 16-hex daemon scope key ([#8721](https://github.com/can1357/oh-my-pi/issues/8721)).
### Fixed
- Fixed high CPU during multi-subagent / workflowz / orchestrate sessions: each live tool block (streaming args, a running partial tool, or a `task` subagent) armed its own 80ms spinner `setInterval` driving `requestComponentRender`, so N concurrent live blocks created N unsynchronized repaint timers that kept the render scheduler awake near-continuously. The per-block timers are now consolidated into a single shared spinner ticker that repaints every live block in one coalesced frame per glyph step, independent of block count ([#8731](https://github.com/can1357/oh-my-pi/issues/8731)).
## [17.3.5] - 2026-08-16
packages/coding-agent/src/modes/components/tool-execution.ts
+58 -22
View File
@@ -269,6 +269,39 @@ export function sharedSpinnerFrame(frameCount: number, now: number = performance
return frameCount > 0 ? Math.floor(now / SPINNER_GLYPH_ADVANCE_MS) % frameCount : 0;
}
/** Live tool blocks currently driving a spinner. A single shared ticker (below)
* advances and repaints every registered block per glyph step, so N concurrent
* live/streaming blocks — e.g. parallel `task` subagents — cost one 80ms timer
* and one coalesced render frame per tick instead of N unsynchronized timers
* each independently waking the render scheduler (issue #8731). */
const liveSpinnerBlocks = new Set<ToolExecutionComponent>();
let sharedSpinnerTimer: NodeJS.Timeout | undefined;
/** Arm the shared spinner ticker if it is not already running. */
function ensureSharedSpinnerTicker(): void {
if (sharedSpinnerTimer) return;
sharedSpinnerTimer = setInterval(() => {
const frame = sharedSpinnerFrame(theme.spinnerFrames.length);
// Deleting the current block mid-iteration (freeze path) is safe on a Set.
for (const block of liveSpinnerBlocks) block.tickSpinner(frame);
}, SPINNER_RENDER_INTERVAL_MS);
}
/** Register a live block with the shared ticker, starting it on first use. */
function registerSpinnerBlock(block: ToolExecutionComponent): void {
liveSpinnerBlocks.add(block);
ensureSharedSpinnerTicker();
}
/** Drop a block; stop the ticker once no live block remains. */
function unregisterSpinnerBlock(block: ToolExecutionComponent): void {
if (!liveSpinnerBlocks.delete(block)) return;
if (liveSpinnerBlocks.size === 0 && sharedSpinnerTimer) {
clearInterval(sharedSpinnerTimer);
sharedSpinnerTimer = undefined;
}
}
// 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).
let toolExecutionInstanceSeq = 0;
@@ -337,7 +370,7 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
#convertedImages: Map<number, { data: string; mimeType: string }> = new Map();
// Spinner animation for partial task results
#spinnerFrame?: number;
#spinnerInterval?: NodeJS.Timeout;
#spinnerActive = false;
// Todo write completion strikethrough reveal animation
#todoStrikeInterval?: NodeJS.Timeout;
// Track if args are still being streamed (for edit/write spinner)
@@ -697,27 +730,16 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
!isBackgroundAsyncRunning &&
(pendingCallConsumesSpinner || partialResultConsumesSpinner);
const needsSpinner = isStreamingArgs || isLivePartialTool || this.#displaceableByToolName === "hub";
if (needsSpinner && !this.#spinnerInterval) {
if (needsSpinner && !this.#spinnerActive) {
const frameCount = theme.spinnerFrames.length;
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;
this.#spinnerFrame = sharedSpinnerFrame(frameCount, now);
this.#renderState.spinnerFrame = this.#spinnerFrame;
// Component-scoped: a spinner tick only changes this tool block, so
// the TUI reuses every other root subtree instead of walking the
// whole tree (issue #4377).
this.#ui.requestComponentRender(this);
}, SPINNER_RENDER_INTERVAL_MS);
} else if (!needsSpinner && this.#spinnerInterval) {
clearInterval(this.#spinnerInterval);
this.#spinnerInterval = undefined;
this.#spinnerActive = true;
registerSpinnerBlock(this);
} else if (!needsSpinner && this.#spinnerActive) {
this.#spinnerActive = false;
unregisterSpinnerBlock(this);
// Clear the last drawn frame so a non-live renderCall (e.g. a write whose
// args just completed) stops showing a frozen spinner glyph. Skip when a
// todo strike owns the frame — it sets its own value right after this.
@@ -728,6 +750,20 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
}
}
/**
* Advance to the shared spinner glyph and repaint just this block. Driven by
* the single shared spinner ticker (see `registerSpinnerBlock`); the tick is
* component-scoped so the TUI reuses every other root subtree (issue #4377).
*/
tickSpinner(frame: number): void {
// A detached task block that scrolled into native scrollback stops the
// instant it leaves the repaintable region.
if (this.#maybeFreezeBackgroundTask()) return;
this.#spinnerFrame = frame;
this.#renderState.spinnerFrame = frame;
this.#ui.requestComponentRender(this);
}
/**
* Freeze a detached (`async.state === "running"`) task block once its rows
* become native-scrollback history: the block left the transcript's live
@@ -790,7 +826,7 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
clearInterval(this.#todoStrikeInterval);
this.#todoStrikeInterval = undefined;
}
if (!this.#spinnerInterval) {
if (!this.#spinnerActive) {
this.#spinnerFrame = undefined;
this.#renderState.spinnerFrame = undefined;
}
@@ -886,9 +922,9 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
* Stop spinner animation and cleanup resources.
*/
stopAnimation(): void {
if (this.#spinnerInterval) {
clearInterval(this.#spinnerInterval);
this.#spinnerInterval = undefined;
if (this.#spinnerActive) {
this.#spinnerActive = false;
unregisterSpinnerBlock(this);
this.#spinnerFrame = undefined;
this.#renderState.spinnerFrame = undefined;
}
packages/coding-agent/test/modes/components/tool-execution-spinner.test.ts
+38 -1
View File
@@ -1,6 +1,9 @@
import { afterEach, beforeAll, describe, expect, it, vi } from "bun:test";
import { stripVTControlCharacters } from "node:util";
import { ToolExecutionComponent } from "@oh-my-pi/pi-coding-agent/modes/components/tool-execution";
import {
SPINNER_RENDER_INTERVAL_MS,
ToolExecutionComponent,
} from "@oh-my-pi/pi-coding-agent/modes/components/tool-execution";
import { TranscriptContainer } from "@oh-my-pi/pi-coding-agent/modes/components/transcript-container";
import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme";
import type { TUI } from "@oh-my-pi/pi-tui";
@@ -222,4 +225,38 @@ describe("ToolExecutionComponent live preview spinners", () => {
component.stopAnimation();
}
});
// Regression (issue #8731): concurrent live tool blocks — e.g. parallel task
// subagents — must share ONE spinner timer, not one per block, or active-work
// CPU scales with block count.
it("drives every concurrent live block from a single shared spinner timer", () => {
vi.useFakeTimers();
const setIntervalSpy = vi.spyOn(globalThis, "setInterval");
const renders = [vi.fn(), vi.fn(), vi.fn()];
const components = renders.map(
requestComponentRender =>
new ToolExecutionComponent(
"eval",
{ language: "py", code: "import time\ntime.sleep(10)" },
{},
undefined,
{ requestRender: vi.fn(), requestComponentRender } as unknown as TUI,
process.cwd(),
),
);
try {
const spinnerTimers = setIntervalSpy.mock.calls.filter(([, ms]) => ms === SPINNER_RENDER_INTERVAL_MS).length;
// One shared ticker for all three live blocks, not three.
expect(spinnerTimers).toBe(1);
// A single tick repaints every registered block in lockstep.
vi.advanceTimersByTime(SPINNER_RENDER_INTERVAL_MS);
for (const requestComponentRender of renders) {
expect(requestComponentRender).toHaveBeenCalledTimes(1);
}
} finally {
for (const component of components) component.stopAnimation();
}
});
});