From cd24efec615cc0124e8be310679a6cc6cbc81780 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 21 Jul 2026 19:18:06 -0500 Subject: [PATCH] perf(ui-tui): shimmer loaders share one clock and stop after a bounded period MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #20379, finding 5 (Perf). Every ShimmerRows mounted its own 90 ms setInterval — the session panel can show lazy skills AND lazy tools at once, and a lazy watch session stays lazy indefinitely, so an otherwise- idle TUI ran ~22 React state updates per second forever. All shimmer compositions now subscribe to a single module-level clock: one interval regardless of how many skeletons are on screen, updates delivered in one timer callback so React batches them into a single render pass, and the interval is torn down with the last subscriber. Each mount's animation is also bounded (SHIMMER_ANIMATE_MS, 30 s): after the budget the skeleton freezes in place — it still reads as "loading" — and stops costing renders entirely. Tests: fake-timer coverage that N subscribers share one timer in lockstep, the interval stops with the last unsubscribe, and a late subscriber restarts the clock cleanly. --- ui-tui/src/__tests__/loaders.test.ts | 65 +++++++++++++++++++++++- ui-tui/src/components/loaders.tsx | 74 +++++++++++++++++++++++++--- 2 files changed, 130 insertions(+), 9 deletions(-) diff --git a/ui-tui/src/__tests__/loaders.test.ts b/ui-tui/src/__tests__/loaders.test.ts index cab44b8d2b57..3e1d007a2d82 100644 --- a/ui-tui/src/__tests__/loaders.test.ts +++ b/ui-tui/src/__tests__/loaders.test.ts @@ -1,8 +1,8 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { renderToScreen } from '../../packages/hermes-ink/src/ink/render-to-screen.js' import { cellAtIndex } from '../../packages/hermes-ink/src/ink/screen.js' -import { ShimmerRows, shimmerSegments } from '../components/loaders.js' +import { ShimmerRows, shimmerSegments, subscribeShimmerClock } from '../components/loaders.js' describe('ShimmerRows leniency (agent-authored calls)', () => { it('accepts a bare row COUNT and derives widths — the generated-code shape', async () => { @@ -50,3 +50,64 @@ describe('shimmerSegments', () => { expect(pre + band + post).toBe(10) }) }) + +// Review on #20379 (finding 5): independent 90 ms intervals per shimmer +// composition meant an idle TUI with two lazy sections repainted ~22x/sec +// forever. All compositions now share ONE clock, and the interval exists +// only while subscribers do. +describe('subscribeShimmerClock', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('drives any number of subscribers from a single interval', () => { + const a: number[] = [] + const b: number[] = [] + + const timersBefore = vi.getTimerCount() + const unsubA = subscribeShimmerClock(p => a.push(p)) + const unsubB = subscribeShimmerClock(p => b.push(p)) + + // Two subscribers, ONE new timer. + expect(vi.getTimerCount()).toBe(timersBefore + 1) + + vi.advanceTimersByTime(300) + + // Same shared phases, in lockstep. + expect(a.length).toBeGreaterThan(0) + expect(a).toEqual(b) + + unsubA() + unsubB() + }) + + it('stops the interval with the last unsubscribe', () => { + const unsubA = subscribeShimmerClock(() => {}) + const unsubB = subscribeShimmerClock(() => {}) + const timersRunning = vi.getTimerCount() + + unsubA() + // Still one subscriber — clock keeps running. + expect(vi.getTimerCount()).toBe(timersRunning) + + unsubB() + expect(vi.getTimerCount()).toBe(timersRunning - 1) + }) + + it('a late subscriber restarts the clock cleanly', () => { + const unsubA = subscribeShimmerClock(() => {}) + + unsubA() + + const seen: number[] = [] + const unsubB = subscribeShimmerClock(p => seen.push(p)) + + vi.advanceTimersByTime(200) + expect(seen.length).toBeGreaterThan(0) + unsubB() + }) +}) diff --git a/ui-tui/src/components/loaders.tsx b/ui-tui/src/components/loaders.tsx index ebe0bb8ce822..8ce6ed7f9591 100644 --- a/ui-tui/src/components/loaders.tsx +++ b/ui-tui/src/components/loaders.tsx @@ -50,17 +50,77 @@ export function Shimmer({ ) } -/** Self-ticking phase for shimmer compositions. */ -export function useShimmerPhase(tickMs = 90): number { - const [phase, setPhase] = useState(0) +// ── Shared shimmer clock ───────────────────────────────────────────── +// +// ONE interval drives every mounted shimmer composition (review on #20379, +// finding 5: the session panel could mount independent 90 ms intervals for +// lazy skills AND lazy tools — ~22 state updates/sec on an otherwise-idle +// TUI). Subscribers share the tick; the interval exists only while +// subscribers do, and all updates land in one timer callback so React +// batches them into a single render pass. + +const TICK_MS = 90 + +/** Animation budget per mount. A lazy watch session can stay lazy + * indefinitely — after the budget the skeleton freezes in place (still + * reads as "loading") instead of repainting forever. */ +export const SHIMMER_ANIMATE_MS = 30_000 + +const clockListeners = new Set<(phase: number) => void>() +let clockId: NodeJS.Timeout | null = null +let clockPhase = 0 + +/** Subscribe to the shared clock (exported for tests). Returns unsubscribe; + * the interval stops with the last subscriber. */ +export function subscribeShimmerClock(fn: (phase: number) => void): () => void { + clockListeners.add(fn) + + if (!clockId) { + clockId = setInterval(() => { + clockPhase += 1 + + for (const listener of clockListeners) { + listener(clockPhase) + } + }, TICK_MS) + + clockId.unref?.() + } + + return () => { + clockListeners.delete(fn) + + if (!clockListeners.size && clockId) { + clearInterval(clockId) + clockId = null + } + } +} + +/** Phase from the shared clock, bounded: stops advancing (and stops costing + * renders) after `animateMs`. */ +export function useShimmerPhase(animateMs = SHIMMER_ANIMATE_MS): number { + const [phase, setPhase] = useState(clockPhase) useEffect(() => { - const id = setInterval(() => setPhase(p => p + 1), tickMs) + const startedAt = Date.now() - id.unref?.() + let unsubscribe: (() => void) | null = subscribeShimmerClock(next => { + if (Date.now() - startedAt >= animateMs) { + unsubscribe?.() + unsubscribe = null - return () => clearInterval(id) - }, [tickMs]) + return + } + + setPhase(next) + }) + + return () => { + unsubscribe?.() + unsubscribe = null + } + }, [animateMs]) return phase }