From 224a33db2ea735e5720ffa1342ecd93723193002 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Wed, 5 Nov 2025 16:20:04 -0800 Subject: [PATCH] Improve tracking of animated components. (#12618) --- .../cli/src/ui/components/CliSpinner.test.tsx | 24 ++++++ packages/cli/src/ui/components/CliSpinner.tsx | 8 +- .../src/ui/components/DebugProfiler.test.tsx | 19 +++++ .../cli/src/ui/components/DebugProfiler.tsx | 4 +- packages/cli/src/ui/debug.ts | 11 +++ .../ui/hooks/useAnimatedScrollbar.test.tsx | 73 +++++++++++++++++++ .../cli/src/ui/hooks/useAnimatedScrollbar.ts | 13 +++- 7 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 packages/cli/src/ui/components/CliSpinner.test.tsx create mode 100644 packages/cli/src/ui/debug.ts create mode 100644 packages/cli/src/ui/hooks/useAnimatedScrollbar.test.tsx diff --git a/packages/cli/src/ui/components/CliSpinner.test.tsx b/packages/cli/src/ui/components/CliSpinner.test.tsx new file mode 100644 index 0000000000..bbea23ab5d --- /dev/null +++ b/packages/cli/src/ui/components/CliSpinner.test.tsx @@ -0,0 +1,24 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from '../../test-utils/render.js'; +import { CliSpinner } from './CliSpinner.js'; +import { debugState } from '../debug.js'; +import { describe, it, expect, beforeEach } from 'vitest'; + +describe('', () => { + beforeEach(() => { + debugState.debugNumAnimatedComponents = 0; + }); + + it('should increment debugNumAnimatedComponents on mount and decrement on unmount', () => { + expect(debugState.debugNumAnimatedComponents).toBe(0); + const { unmount } = render(); + expect(debugState.debugNumAnimatedComponents).toBe(1); + unmount(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); +}); diff --git a/packages/cli/src/ui/components/CliSpinner.tsx b/packages/cli/src/ui/components/CliSpinner.tsx index b194a6c468..6795bf2670 100644 --- a/packages/cli/src/ui/components/CliSpinner.tsx +++ b/packages/cli/src/ui/components/CliSpinner.tsx @@ -6,17 +6,15 @@ import Spinner from 'ink-spinner'; import { type ComponentProps, useEffect } from 'react'; - -// A top-level field to track the total number of active spinners. -export let debugNumSpinners = 0; +import { debugState } from '../debug.js'; export type SpinnerProps = ComponentProps; export const CliSpinner = (props: SpinnerProps) => { useEffect(() => { - debugNumSpinners++; + debugState.debugNumAnimatedComponents++; return () => { - debugNumSpinners--; + debugState.debugNumAnimatedComponents--; }; }, []); diff --git a/packages/cli/src/ui/components/DebugProfiler.test.tsx b/packages/cli/src/ui/components/DebugProfiler.test.tsx index c7e63f0b26..604e54c5fd 100644 --- a/packages/cli/src/ui/components/DebugProfiler.test.tsx +++ b/packages/cli/src/ui/components/DebugProfiler.test.tsx @@ -12,6 +12,7 @@ import { FRAME_TIMESTAMP_CAPACITY, } from './DebugProfiler.js'; import { FixedDeque } from 'mnemonist'; +import { debugState } from '../debug.js'; describe('DebugProfiler', () => { beforeEach(() => { @@ -29,12 +30,14 @@ describe('DebugProfiler', () => { Array, ACTION_TIMESTAMP_CAPACITY, ); + debugState.debugNumAnimatedComponents = 0; }); afterEach(() => { vi.restoreAllMocks(); profiler.actionTimestamps.clear(); profiler.possiblyIdleFrameTimestamps.clear(); + debugState.debugNumAnimatedComponents = 0; }); it('should not exceed action timestamp capacity', () => { @@ -193,4 +196,20 @@ describe('DebugProfiler', () => { expect(profiler.totalIdleFrames).toBe(0); }); + + it('should not report frames as idle if debugNumAnimatedComponents > 0', async () => { + const startTime = Date.now(); + vi.setSystemTime(startTime); + debugState.debugNumAnimatedComponents = 1; + + for (let i = 0; i < 5; i++) { + profiler.reportFrameRendered(); + vi.advanceTimersByTime(20); + } + + vi.advanceTimersByTime(1000); + profiler.checkForIdleFrames(); + + expect(profiler.totalIdleFrames).toBe(0); + }); }); diff --git a/packages/cli/src/ui/components/DebugProfiler.tsx b/packages/cli/src/ui/components/DebugProfiler.tsx index 5b46e33251..cbcdbe5f24 100644 --- a/packages/cli/src/ui/components/DebugProfiler.tsx +++ b/packages/cli/src/ui/components/DebugProfiler.tsx @@ -9,7 +9,7 @@ import { useEffect, useState } from 'react'; import { FixedDeque } from 'mnemonist'; import { theme } from '../semantic-colors.js'; import { useUIState } from '../contexts/UIStateContext.js'; -import { debugNumSpinners } from './CliSpinner.js'; +import { debugState } from '../debug.js'; import { appEvents, AppEvent } from '../../utils/events.js'; // Frames that render at least this far before or after an action are considered @@ -52,7 +52,7 @@ export const profiler = { if (now - this.lastFrameStartTime > 16) { this.lastFrameStartTime = now; this.numFrames++; - if (debugNumSpinners === 0) { + if (debugState.debugNumAnimatedComponents === 0) { if (this.possiblyIdleFrameTimestamps.size >= FRAME_TIMESTAMP_CAPACITY) { this.possiblyIdleFrameTimestamps.shift(); } diff --git a/packages/cli/src/ui/debug.ts b/packages/cli/src/ui/debug.ts new file mode 100644 index 0000000000..833dcc8b81 --- /dev/null +++ b/packages/cli/src/ui/debug.ts @@ -0,0 +1,11 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +// A top-level field to track the total number of active animated components. +// This is used for testing to ensure we wait for animations to finish. +export const debugState = { + debugNumAnimatedComponents: 0, +}; diff --git a/packages/cli/src/ui/hooks/useAnimatedScrollbar.test.tsx b/packages/cli/src/ui/hooks/useAnimatedScrollbar.test.tsx new file mode 100644 index 0000000000..3fd84ad7a5 --- /dev/null +++ b/packages/cli/src/ui/hooks/useAnimatedScrollbar.test.tsx @@ -0,0 +1,73 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { act } from 'react'; +import { render } from '../../test-utils/render.js'; +import { useAnimatedScrollbar } from './useAnimatedScrollbar.js'; +import { debugState } from '../debug.js'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +const TestComponent = ({ isFocused = false }: { isFocused?: boolean }) => { + useAnimatedScrollbar(isFocused, () => {}); + return null; +}; + +describe('useAnimatedScrollbar', () => { + beforeEach(() => { + debugState.debugNumAnimatedComponents = 0; + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('should not increment debugNumAnimatedComponents when not focused', () => { + render(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); + + it('should not increment debugNumAnimatedComponents on initial mount even if focused', () => { + render(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); + + it('should increment debugNumAnimatedComponents when becoming focused', () => { + const { rerender } = render(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + rerender(); + expect(debugState.debugNumAnimatedComponents).toBe(1); + }); + + it('should decrement debugNumAnimatedComponents when becoming unfocused', () => { + const { rerender } = render(); + rerender(); + expect(debugState.debugNumAnimatedComponents).toBe(1); + rerender(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); + + it('should decrement debugNumAnimatedComponents on unmount', () => { + const { rerender, unmount } = render(); + rerender(); + expect(debugState.debugNumAnimatedComponents).toBe(1); + unmount(); + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); + + it('should decrement debugNumAnimatedComponents after animation finishes', async () => { + const { rerender } = render(); + rerender(); + expect(debugState.debugNumAnimatedComponents).toBe(1); + + // Advance timers by enough time for animation to complete (200 + 1000 + 300 + buffer) + await act(async () => { + await vi.advanceTimersByTimeAsync(2000); + }); + + expect(debugState.debugNumAnimatedComponents).toBe(0); + }); +}); diff --git a/packages/cli/src/ui/hooks/useAnimatedScrollbar.ts b/packages/cli/src/ui/hooks/useAnimatedScrollbar.ts index fa290f5b54..aeb1d79041 100644 --- a/packages/cli/src/ui/hooks/useAnimatedScrollbar.ts +++ b/packages/cli/src/ui/hooks/useAnimatedScrollbar.ts @@ -7,6 +7,7 @@ import { useState, useEffect, useRef, useCallback } from 'react'; import { theme } from '../semantic-colors.js'; import { interpolateColor } from '../themes/color-utils.js'; +import { debugState } from '../debug.js'; export function useAnimatedScrollbar( isFocused: boolean, @@ -18,8 +19,13 @@ export function useAnimatedScrollbar( const animationFrame = useRef(null); const timeout = useRef(null); + const isAnimatingRef = useRef(false); const cleanup = useCallback(() => { + if (isAnimatingRef.current) { + debugState.debugNumAnimatedComponents--; + isAnimatingRef.current = false; + } if (animationFrame.current) { clearInterval(animationFrame.current); animationFrame.current = null; @@ -32,6 +38,8 @@ export function useAnimatedScrollbar( const flashScrollbar = useCallback(() => { cleanup(); + debugState.debugNumAnimatedComponents++; + isAnimatingRef.current = true; const fadeInDuration = 200; const visibleDuration = 1000; @@ -67,10 +75,7 @@ export function useAnimatedScrollbar( ); if (progress === 1) { - if (animationFrame.current) { - clearInterval(animationFrame.current); - animationFrame.current = null; - } + cleanup(); } };