From 68952f452534c9145503df0b6b24470e9ddf3585 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 3 Aug 2026 00:18:55 +0000 Subject: [PATCH 1/2] fix: pause frame metrics while hidden --- .changeset/frame-lifecycle.md | 5 ++ .../collector-manager.browser.test.ts | 8 ++- .../frame-timing-collector.browser.test.ts | 66 ++++++++++++++----- .../collectors/frame-timing-collector.ts | 43 ++++++++++-- 4 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 .changeset/frame-lifecycle.md diff --git a/.changeset/frame-lifecycle.md b/.changeset/frame-lifecycle.md new file mode 100644 index 0000000..38db9f0 --- /dev/null +++ b/.changeset/frame-lifecycle.md @@ -0,0 +1,5 @@ +--- +'@github-ui/storybook-addon-performance-panel': patch +--- + +Avoid synthetic first-frame samples and pause frame collection while the preview document is hidden. \ No newline at end of file diff --git a/packages/storybook-addon-performance-panel/collectors/__tests__/collector-manager.browser.test.ts b/packages/storybook-addon-performance-panel/collectors/__tests__/collector-manager.browser.test.ts index 206a7f0..1248fda 100644 --- a/packages/storybook-addon-performance-panel/collectors/__tests__/collector-manager.browser.test.ts +++ b/packages/storybook-addon-performance-panel/collectors/__tests__/collector-manager.browser.test.ts @@ -251,9 +251,11 @@ describe('CollectorManager', () => { it('returns updated metrics after collecting', () => { manager.start() - // Simulate a frame - vi.spyOn(performance, 'now').mockReturnValue(16.67) - rafCallback?.(16.67) + // Seed the frame baseline, then simulate a measured frame. + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + vi.spyOn(performance, 'now').mockReturnValue(26.67) + rafCallback?.(26.67) const metrics = manager.getFrameMetrics() expect(metrics.frameTimes.length).toBeGreaterThan(0) diff --git a/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts b/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts index 512076c..8a85c4f 100644 --- a/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts +++ b/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts @@ -6,6 +6,7 @@ describe('FrameTimingCollector', () => { let collector: FrameTimingCollector let rafCallback: FrameRequestCallback | null = null let rafId = 0 + let hidden = false beforeEach(() => { // Mock requestAnimationFrame @@ -17,6 +18,7 @@ describe('FrameTimingCollector', () => { rafCallback = null }) vi.spyOn(performance, 'now').mockReturnValue(0) + vi.spyOn(document, 'hidden', 'get').mockImplementation(() => hidden) collector = new FrameTimingCollector() }) @@ -46,10 +48,10 @@ describe('FrameTimingCollector', () => { collector.start() // Simulate some frames - vi.spyOn(performance, 'now').mockReturnValue(16.67) - rafCallback?.(16.67) - vi.spyOn(performance, 'now').mockReturnValue(33.34) - rafCallback?.(33.34) + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + vi.spyOn(performance, 'now').mockReturnValue(26.67) + rafCallback?.(26.67) collector.reset() @@ -74,22 +76,26 @@ describe('FrameTimingCollector', () => { it('tracks frame times', () => { collector.start() - // Simulate 60fps frame (16.67ms) - vi.spyOn(performance, 'now').mockReturnValue(16.67) - rafCallback?.(16.67) + // The first callback establishes a baseline; the second records a frame. + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + expect(collector.getMetrics().frameTimes).toEqual([]) + + vi.spyOn(performance, 'now').mockReturnValue(26.67) + rafCallback?.(26.67) const metrics = collector.getMetrics() - // At least one frame should be tracked (start() may record initial frame too) - expect(metrics.frameTimes.length).toBeGreaterThanOrEqual(1) - // The last frame should be close to 16.67ms - expect(metrics.frameTimes[metrics.frameTimes.length - 1]).toBeCloseTo(16.67, 1) + expect(metrics.frameTimes).toHaveLength(1) + expect(metrics.frameTimes[0]).toBeCloseTo(16.67, 1) }) it('tracks max frame time', () => { collector.start() - vi.spyOn(performance, 'now').mockReturnValue(50) - rafCallback?.(50) + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + vi.spyOn(performance, 'now').mockReturnValue(60) + rafCallback?.(60) const metrics = collector.getMetrics() expect(metrics.maxFrameTime).toBe(50) @@ -99,12 +105,36 @@ describe('FrameTimingCollector', () => { collector.start() // Frame time of 50ms = should count as 2 dropped frames (50/16.67 - 1 ≈ 2) - vi.spyOn(performance, 'now').mockReturnValue(50) - rafCallback?.(50) + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + vi.spyOn(performance, 'now').mockReturnValue(60) + rafCallback?.(60) const metrics = collector.getMetrics() expect(metrics.droppedFrames).toBeGreaterThan(0) }) + + it('starts a fresh baseline after the document becomes visible', () => { + collector.start() + + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + + hidden = true + document.dispatchEvent(new Event('visibilitychange')) + hidden = false + document.dispatchEvent(new Event('visibilitychange')) + + vi.spyOn(performance, 'now').mockReturnValue(1_000) + rafCallback?.(1_000) + expect(collector.getMetrics().frameTimes).toEqual([]) + + vi.spyOn(performance, 'now').mockReturnValue(1_016.67) + rafCallback?.(1_016.67) + + expect(collector.getMetrics().frameTimes[0]).toBeCloseTo(16.67, 1) + expect(collector.getMetrics().droppedFrames).toBe(0) + }) }) describe('onFrame callback', () => { @@ -113,8 +143,10 @@ describe('FrameTimingCollector', () => { collector = new FrameTimingCollector(onFrame) collector.start() - vi.spyOn(performance, 'now').mockReturnValue(16.67) - rafCallback?.(16.67) + vi.spyOn(performance, 'now').mockReturnValue(10) + rafCallback?.(10) + vi.spyOn(performance, 'now').mockReturnValue(26.67) + rafCallback?.(26.67) expect(onFrame).toHaveBeenCalledWith(expect.closeTo(16.67, 1)) }) diff --git a/packages/storybook-addon-performance-panel/collectors/frame-timing-collector.ts b/packages/storybook-addon-performance-panel/collectors/frame-timing-collector.ts index a3cfb9b..7ae8189 100644 --- a/packages/storybook-addon-performance-panel/collectors/frame-timing-collector.ts +++ b/packages/storybook-addon-performance-panel/collectors/frame-timing-collector.ts @@ -43,21 +43,33 @@ export class FrameTimingCollector implements MetricCollector #lastTime = 0 #animationId: number | null = null #onFrame?: (delta: number) => void + #running = false constructor(onFrame?: (delta: number) => void) { this.#onFrame = onFrame } start(): void { - this.#lastTime = performance.now() - this.#measure() + if (this.#running) return + + this.#running = true + this.#lastTime = 0 + document.addEventListener('visibilitychange', this.#handleVisibilityChange) + if (!document.hidden) { + this.#animationId = requestAnimationFrame(this.#measure) + } } stop(): void { + if (!this.#running) return + + this.#running = false + document.removeEventListener('visibilitychange', this.#handleVisibilityChange) if (this.#animationId !== null) { cancelAnimationFrame(this.#animationId) this.#animationId = null } + this.#lastTime = 0 } reset(): void { @@ -65,6 +77,7 @@ export class FrameTimingCollector implements MetricCollector this.#maxFrameTime = 0 this.#droppedFrames = 0 this.#frameJitter = 0 + this.#lastTime = 0 } getMetrics(): FrameTimingMetrics { @@ -78,16 +91,34 @@ export class FrameTimingCollector implements MetricCollector } #measure = (): void => { + this.#animationId = null + if (!this.#running || document.hidden) return + const now = performance.now() - const delta = now - this.#lastTime - this.#lastTime = now + if (this.#lastTime > 0) { + const delta = now - this.#lastTime - this.#processFrame(delta) - this.#onFrame?.(delta) + this.#processFrame(delta) + this.#onFrame?.(delta) + } + this.#lastTime = now this.#animationId = requestAnimationFrame(this.#measure) } + #handleVisibilityChange = (): void => { + this.#lastTime = 0 + + if (document.hidden) { + if (this.#animationId !== null) { + cancelAnimationFrame(this.#animationId) + this.#animationId = null + } + } else if (this.#running && this.#animationId === null) { + this.#animationId = requestAnimationFrame(this.#measure) + } + } + #processFrame(delta: number): void { // Add to rolling window addToWindow(this.#frameTimes, delta, FRAME_TIMES_WINDOW) From a6becbd09893587568491f6b117a653c3186e97e Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 3 Aug 2026 02:14:37 +0000 Subject: [PATCH 2/2] test: cover idempotent frame lifecycle --- .../frame-timing-collector.browser.test.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts b/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts index 8a85c4f..e7dfa6d 100644 --- a/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts +++ b/packages/storybook-addon-performance-panel/collectors/__tests__/frame-timing-collector.browser.test.ts @@ -33,6 +33,17 @@ describe('FrameTimingCollector', () => { collector.start() expect(window.requestAnimationFrame).toHaveBeenCalled() }) + + it('does not schedule duplicate work when started repeatedly', () => { + const addEventListener = vi.spyOn(document, 'addEventListener') + + collector.start() + collector.start() + + const visibilityListeners = addEventListener.mock.calls.filter(([eventName]) => eventName === 'visibilitychange') + expect(visibilityListeners).toHaveLength(1) + expect(window.requestAnimationFrame).toHaveBeenCalledOnce() + }) }) describe('stop', () => { @@ -41,6 +52,20 @@ describe('FrameTimingCollector', () => { collector.stop() expect(window.cancelAnimationFrame).toHaveBeenCalled() }) + + it('does not clean up more than once when stopped repeatedly', () => { + const removeEventListener = vi.spyOn(document, 'removeEventListener') + + collector.start() + collector.stop() + collector.stop() + + const visibilityListeners = removeEventListener.mock.calls.filter( + ([eventName]) => eventName === 'visibilitychange', + ) + expect(visibilityListeners).toHaveLength(1) + expect(window.cancelAnimationFrame).toHaveBeenCalledOnce() + }) }) describe('reset', () => {