From 355e35b5f2502f9cb37a896bf037ff81cc88a236 Mon Sep 17 00:00:00 2001 From: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com> Date: Mon, 5 Oct 2026 23:20:52 +0000 Subject: [PATCH] perf(tab-bar): scrolling the tab strip no longer re-renders every tab (#24240) * perf(tab-bar): move scroll thumb position out of TabBar state The thumb moved every scroll and resize frame, re-rendering every tab with it. Fixes #24012 * perf(tab-bar): reuse the strip's resize observers for the scroll thumb The thumb had its own observer on every tab, on top of the tab bar's. * test(e2e): run the tab strip scroll spec without waiting on frames CI's hidden window draws about one frame a second, so real wheel input timed out. Also drops Reflect.get for the anti-slop audit. * test(tab-bar): pin that a re-render leaves the scroll thumb's geometry alone The thumb's width and transform live outside React, so the element must survive a hover re-render with its geometry intact. * test(e2e): set the tab strip scroll start after the strip stops retitling Opening the tabs leaves the strip pinned to its end, and the scroll event that releases that pin only lands with the next frame - about a second in CI's hidden window. A tab retitling inside that window re-pinned the strip to the end, so the wheel sweep would start at an edge and every step would flip the edge flags. --------- Co-authored-by: Neil --- .../tab-bar/TabStripScrollIndicator.test.tsx | 113 +++++--- .../tab-bar/TabStripScrollIndicator.tsx | 74 +++--- .../components/tab-bar/tab-bar-surface.tsx | 12 +- .../tab-bar/tab-strip-growth-scroll.test.tsx | 33 ++- .../tab-bar/tab-strip-overflow-navigation.ts | 42 ++- .../tab-bar/tab-strip-scroll-metrics.test.ts | 46 +--- .../tab-bar/tab-strip-scroll-metrics.ts | 43 ++- .../tab-strip-scroll-render-isolation.spec.ts | 250 ++++++++++++++++++ 8 files changed, 459 insertions(+), 154 deletions(-) create mode 100644 tests/e2e/tab-strip-scroll-render-isolation.spec.ts diff --git a/src/renderer/src/components/tab-bar/TabStripScrollIndicator.test.tsx b/src/renderer/src/components/tab-bar/TabStripScrollIndicator.test.tsx index 6aae0441237..c89c8a21087 100644 --- a/src/renderer/src/components/tab-bar/TabStripScrollIndicator.test.tsx +++ b/src/renderer/src/components/tab-bar/TabStripScrollIndicator.test.tsx @@ -4,37 +4,20 @@ import React, { createRef } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' import { cleanup, fireEvent, render } from '@testing-library/react' import { TabStripScrollIndicator } from './TabStripScrollIndicator' -import type { TabStripScrollMetrics } from './tab-strip-scroll-metrics' afterEach(() => { cleanup() vi.restoreAllMocks() }) -const OVERFLOW_METRICS: TabStripScrollMetrics = { - hasOverflow: true, - canScrollStart: false, - canScrollEnd: true, - thumbSizeFraction: 0.4, - thumbOffsetFraction: 0 -} - -const NO_OVERFLOW_METRICS: TabStripScrollMetrics = { - hasOverflow: false, - canScrollStart: false, - canScrollEnd: false, - thumbSizeFraction: 1, - thumbOffsetFraction: 0 -} - describe('TabStripScrollIndicator', () => { it('renders null when there is no overflow', () => { - const { container } = render() + const { container } = render() expect(container.firstChild).toBeNull() }) it('renders under the tabs with bottom-0 and idle 3px height', () => { - const { getByTestId } = render() + const { getByTestId } = render() const indicator = getByTestId('tab-strip-scroll-indicator') expect(indicator).toBeTruthy() expect(indicator.className).toContain('bottom-0') @@ -48,7 +31,7 @@ describe('TabStripScrollIndicator', () => { }) it('expands to 4px and becomes opaque on pointer hover, restores on leave', () => { - const { getByTestId } = render() + const { getByTestId } = render() const indicator = getByTestId('tab-strip-scroll-indicator') expect(indicator.className).toContain('h-[3px]') expect(indicator.className).toContain('opacity-0') @@ -70,7 +53,7 @@ describe('TabStripScrollIndicator', () => { ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer const { getByTestId } = render( - + ) const indicator = getByTestId('tab-strip-scroll-indicator') Object.defineProperty(indicator, 'clientWidth', { value: 400, configurable: true }) @@ -100,18 +83,14 @@ describe('TabStripScrollIndicator', () => { }) it('applies pointer-events-none when disabled', () => { - const { getByTestId } = render( - - ) + const { getByTestId } = render() const indicator = getByTestId('tab-strip-scroll-indicator') expect(indicator.className).toContain('pointer-events-none') expect(indicator.className).not.toContain('group-hover/tab-strip:pointer-events-auto') }) it('stays hidden and unexpanded on hover when disabled', () => { - const { getByTestId } = render( - - ) + const { getByTestId } = render() const indicator = getByTestId('tab-strip-scroll-indicator') fireEvent.pointerEnter(indicator) expect(indicator.className).toContain('opacity-0') @@ -127,7 +106,7 @@ describe('TabStripScrollIndicator', () => { const { getByTestId } = render( @@ -144,7 +123,7 @@ describe('TabStripScrollIndicator', () => { ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer const { getByTestId } = render( - + ) const indicator = getByTestId('tab-strip-scroll-indicator') fireEvent.wheel(indicator, { deltaX: 40, deltaY: 0 }) @@ -164,7 +143,7 @@ describe('TabStripScrollIndicator', () => { ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer const { getByTestId } = render( - + ) const indicator = getByTestId('tab-strip-scroll-indicator') Object.defineProperty(indicator, 'clientWidth', { value: 400, configurable: true }) @@ -200,13 +179,7 @@ describe('TabStripScrollIndicator', () => { ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer const { getByTestId } = render( - + ) const indicator = getByTestId('tab-strip-scroll-indicator') Object.defineProperty(indicator, 'clientWidth', { value: 400, configurable: true }) @@ -216,9 +189,9 @@ describe('TabStripScrollIndicator', () => { fireEvent.pointerDown(thumb, { button: 0, clientX: 50 }) expect(indicator.className).toContain('h-[4px]') - // Move pointer by 60px + // 60px of a 240px thumb travel scrolls 60/240 of the 600px scroll range fireEvent(window, new MouseEvent('pointermove', { clientX: 110 })) - expect(scrollContainer.scrollLeft).toBeGreaterThan(0) + expect(scrollContainer.scrollLeft).toBe(150) // Release drag fireEvent(window, new MouseEvent('pointerup')) @@ -235,7 +208,7 @@ describe('TabStripScrollIndicator', () => { ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer const { getByTestId, rerender } = render( - + ) const indicator = getByTestId('tab-strip-scroll-indicator') Object.defineProperty(indicator, 'clientWidth', { value: 400, configurable: true }) @@ -245,7 +218,7 @@ describe('TabStripScrollIndicator', () => { rerender( @@ -257,4 +230,62 @@ describe('TabStripScrollIndicator', () => { fireEvent(window, new MouseEvent('pointermove', { clientX: 300 })) expect(scrollContainer.scrollLeft).toBe(0) }) + + it('sizes the thumb when overflow appears and keeps it in step with scrolling and tab changes', () => { + const scrollContainer = document.createElement('div') + Object.defineProperty(scrollContainer, 'scrollWidth', { value: 1000, configurable: true }) + Object.defineProperty(scrollContainer, 'clientWidth', { value: 400, configurable: true }) + const scrollContainerRef = createRef() + ;(scrollContainerRef as React.MutableRefObject).current = scrollContainer + const clientWidth = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'clientWidth')! + vi.spyOn(HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(function ( + this: HTMLElement + ) { + return this.dataset.testid === 'tab-strip-scroll-indicator' + ? 400 + : clientWidth.get!.call(this) + }) + + const stripResizeListeners = new Set<() => void>() + const subscribeToStripResize = (listener: () => void): (() => void) => { + stripResizeListeners.add(listener) + return () => stripResizeListeners.delete(listener) + } + + const { getByTestId, rerender, unmount } = render( + + ) + rerender( + + ) + const thumb = getByTestId('tab-strip-scroll-thumb') + expect(thumb.style.width).toBe('160px') + expect(thumb.style.transform).toBe('translateX(0px)') + + scrollContainer.scrollLeft = 300 + fireEvent.scroll(scrollContainer) + expect(thumb.style.transform).toBe('translateX(120px)') + + // The thumb's geometry is not a React prop, so a re-render for hover must not reset it. + fireEvent.pointerEnter(getByTestId('tab-strip-scroll-indicator')) + expect(getByTestId('tab-strip-scroll-thumb')).toBe(thumb) + expect(thumb.style.width).toBe('160px') + expect(thumb.style.transform).toBe('translateX(120px)') + + // A tab opening grows the strip without a scroll event. + Object.defineProperty(scrollContainer, 'scrollWidth', { value: 2000, configurable: true }) + stripResizeListeners.forEach((listener) => listener()) + expect(thumb.style.width).toBe('80px') + + unmount() + expect(stripResizeListeners.size).toBe(0) + }) }) diff --git a/src/renderer/src/components/tab-bar/TabStripScrollIndicator.tsx b/src/renderer/src/components/tab-bar/TabStripScrollIndicator.tsx index d3b1c4672fd..4e586159a7b 100644 --- a/src/renderer/src/components/tab-bar/TabStripScrollIndicator.tsx +++ b/src/renderer/src/components/tab-bar/TabStripScrollIndicator.tsx @@ -1,52 +1,52 @@ -import React, { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react' -import { - computeTabStripThumbLayout, - type TabStripScrollMetrics, - type TabStripThumbLayout -} from './tab-strip-scroll-metrics' - -const EMPTY_THUMB_LAYOUT: TabStripThumbLayout = { widthPx: 0, leftPx: 0 } +import React, { useEffect, useLayoutEffect, useRef, useState } from 'react' +import { computeTabStripThumbLayout } from './tab-strip-scroll-metrics' export type TabStripScrollIndicatorProps = { - metrics: TabStripScrollMetrics + hasOverflow: boolean scrollContainerRef?: React.RefObject + /** Called when tabs change the strip's size without a scroll event; returns an unsubscribe. */ + subscribeToStripResize?: (listener: () => void) => () => void disabled?: boolean } export function TabStripScrollIndicator({ - metrics, + hasOverflow, scrollContainerRef, + subscribeToStripResize, disabled = false }: TabStripScrollIndicatorProps): React.JSX.Element | null { const trackRef = useRef(null) + const thumbRef = useRef(null) const cleanupDragRef = useRef<(() => void) | null>(null) - const [thumbLayout, setThumbLayout] = useState(EMPTY_THUMB_LAYOUT) const [isHovered, setIsHovered] = useState(false) const [isDragging, setIsDragging] = useState(false) const [isScrolling, setIsScrolling] = useState(false) const scrollTimeoutRef = useRef | null>(null) - const remeasureThumb = useCallback((): void => { - const track = trackRef.current - if (!track) { - return - } - setThumbLayout(computeTabStripThumbLayout(track.clientWidth, metrics)) - }, [metrics]) - - useLayoutEffect(() => { - remeasureThumb() - }, [remeasureThumb]) - + // Why write the thumb's style directly: it moves every scroll frame, and React state would re-render per frame. useLayoutEffect(() => { const track = trackRef.current - if (!track) { + const thumb = thumbRef.current + const scrollContainer = scrollContainerRef?.current + if (!track || !thumb || !scrollContainer) { return } - const resizeObserver = new ResizeObserver(remeasureThumb) - resizeObserver.observe(track) - return () => resizeObserver.disconnect() - }, [remeasureThumb]) + const syncThumb = (): void => { + const layout = computeTabStripThumbLayout(track.clientWidth, scrollContainer) + thumb.style.width = `${layout.widthPx}px` + thumb.style.transform = `translateX(${layout.leftPx}px)` + } + syncThumb() + scrollContainer.addEventListener('scroll', syncThumb, { passive: true }) + const trackResizeObserver = new ResizeObserver(syncThumb) + trackResizeObserver.observe(track) + const unsubscribeStripResize = subscribeToStripResize?.(syncThumb) + return () => { + scrollContainer.removeEventListener('scroll', syncThumb) + trackResizeObserver.disconnect() + unsubscribeStripResize?.() + } + }, [hasOverflow, scrollContainerRef, subscribeToStripResize]) useEffect(() => { const scrollContainer = scrollContainerRef?.current @@ -79,10 +79,10 @@ export function TabStripScrollIndicator({ // Why: hiding the indicator mid-drag would otherwise leave window listeners and body cursor/user-select stuck. useEffect(() => { - if (disabled || !metrics.hasOverflow) { + if (disabled || !hasOverflow) { cleanupDragRef.current?.() } - }, [disabled, metrics.hasOverflow]) + }, [disabled, hasOverflow]) const handleThumbPointerDown = (e: React.PointerEvent): void => { if (e.button !== 0 || disabled) { @@ -100,7 +100,8 @@ export function TabStripScrollIndicator({ const startX = e.clientX const startScrollLeft = scrollContainer.scrollLeft const trackWidth = track.clientWidth - const maxLeft = Math.max(1, trackWidth - thumbLayout.widthPx) + const thumbWidth = computeTabStripThumbLayout(trackWidth, scrollContainer).widthPx + const maxLeft = Math.max(1, trackWidth - thumbWidth) const maxScrollLeft = Math.max(0, scrollContainer.scrollWidth - scrollContainer.clientWidth) if (maxScrollLeft <= 0 || maxLeft <= 0) { @@ -157,7 +158,7 @@ export function TabStripScrollIndicator({ const trackRect = track.getBoundingClientRect() const clickX = e.clientX - trackRect.left const trackWidth = track.clientWidth - const thumbWidth = thumbLayout.widthPx + const thumbWidth = computeTabStripThumbLayout(trackWidth, scrollContainer).widthPx const maxLeft = Math.max(1, trackWidth - thumbWidth) const maxScrollLeft = Math.max(0, scrollContainer.scrollWidth - scrollContainer.clientWidth) @@ -184,7 +185,7 @@ export function TabStripScrollIndicator({ scrollContainer.scrollLeft += delta } - if (!metrics.hasOverflow) { + if (!hasOverflow) { return null } @@ -214,18 +215,15 @@ export function TabStripScrollIndicator({ aria-hidden >
diff --git a/src/renderer/src/components/tab-bar/tab-bar-surface.tsx b/src/renderer/src/components/tab-bar/tab-bar-surface.tsx index 8a6ad5be964..693691a5b80 100644 --- a/src/renderer/src/components/tab-bar/tab-bar-surface.tsx +++ b/src/renderer/src/components/tab-bar/tab-bar-surface.tsx @@ -88,8 +88,13 @@ export function renderTabBarSurface({ } = createMenu const { orderedItems, sortableIds, dropIndicatorByVisibleId } = itemProjection const clientHostedBrowserRows = props.clientHostedBrowserRows ?? EMPTY_CLIENT_HOSTED_ROWS - const { tabStripRef, tabStripOverflowState, activeTabDockSide, scrollTabStrip } = - tabStripNavigation + const { + tabStripRef, + tabStripOverflowState, + activeTabDockSide, + scrollTabStrip, + subscribeToStripResize + } = tabStripNavigation const includeTopTabBorder = tabStripChrome !== 'floating-panel' const renderedItems = renderTabBarItems({ items: orderedItems, @@ -166,8 +171,9 @@ export function renderTabBarSurface({ ) : null} diff --git a/src/renderer/src/components/tab-bar/tab-strip-growth-scroll.test.tsx b/src/renderer/src/components/tab-bar/tab-strip-growth-scroll.test.tsx index c6d80d4d2f1..dee09fec386 100644 --- a/src/renderer/src/components/tab-bar/tab-strip-growth-scroll.test.tsx +++ b/src/renderer/src/components/tab-bar/tab-strip-growth-scroll.test.tsx @@ -1,6 +1,6 @@ // @vitest-environment happy-dom import { act, cleanup, render } from '@testing-library/react' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { useTabStripOverflowNavigation } from './tab-strip-overflow-navigation' const TAB_WIDTH = 100 @@ -80,6 +80,8 @@ function restoreStripLayout(): void { } const NO_HOSTED_ROWS: string[] = [] +let stripRenderCount = 0 +let subscribeToStripResize: ((listener: () => void) => () => void) | null = null /** `hostedRows` render like client-hosted browser rows: a strip slot with no `data-tab-id`. */ function Strip({ @@ -93,12 +95,14 @@ function Strip({ hostedRows?: string[] activeHostedRow?: string | null }): React.JSX.Element { + stripRenderCount++ const navigation = useTabStripOverflowNavigation({ activeVisibleTabId: active, activeDockSlotId: activeHostedRow ?? active, layoutKey: [...tabs, ...hostedRows].join(','), worktreeId: 'wt-1' }) + subscribeToStripResize = navigation.subscribeToStripResize return (
{ expect(tabX(strip, 'N')).toBe(100) }) }) + +describe('tab strip while scrolling', () => { + beforeEach(installStripLayout) + afterEach(() => { + cleanup() + restoreStripLayout() + }) + + it('does not re-render the strip for a scroll that stays between the edges', () => { + const { strip } = mountScrolled('E', 300) + const rendersBefore = stripRenderCount + act(() => { + strip.scrollLeft = 400 + strip.dispatchEvent(new Event('scroll')) + }) + expect(stripRenderCount).toBe(rendersBefore) + }) + + it('tells resize subscribers when tabs change the strip without a scroll', async () => { + const { rerender } = mountScrolled('J', 700) + const listener = vi.fn() + subscribeToStripResize!(listener) + rerender() + await Promise.resolve() + expect(listener).toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/tab-bar/tab-strip-overflow-navigation.ts b/src/renderer/src/components/tab-bar/tab-strip-overflow-navigation.ts index 29508efb3e6..df1f292bfb4 100644 --- a/src/renderer/src/components/tab-bar/tab-strip-overflow-navigation.ts +++ b/src/renderer/src/components/tab-bar/tab-strip-overflow-navigation.ts @@ -1,9 +1,9 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState, type RefObject } from 'react' import { bindTabStripContentResizeObservers } from './tab-strip-content-resize-observers' import { - computeTabStripScrollMetrics, - sameTabStripScrollMetrics, - type TabStripScrollMetrics + computeTabStripOverflowState, + sameTabStripOverflowState, + type TabStripOverflowState } from './tab-strip-scroll-metrics' import { isTabStripPointerGestureActive } from './tab-strip-pointer-gesture' import { @@ -44,12 +44,10 @@ function isTabStripScrolledToEnd(el: HTMLElement): boolean { return el.scrollLeft >= max - 2 } -const EMPTY_TAB_STRIP_OVERFLOW_STATE: TabStripScrollMetrics = { +const EMPTY_TAB_STRIP_OVERFLOW_STATE: TabStripOverflowState = { hasOverflow: false, canScrollStart: false, - canScrollEnd: false, - thumbSizeFraction: 1, - thumbOffsetFraction: 0 + canScrollEnd: false } export function useTabStripOverflowNavigation({ @@ -65,11 +63,13 @@ export function useTabStripOverflowNavigation({ worktreeId: string }): { tabStripRef: RefObject - tabStripOverflowState: TabStripScrollMetrics + tabStripOverflowState: TabStripOverflowState activeTabDockSide: ActiveTabDockSide | null scrollTabStrip: (direction: 'start' | 'end', behavior?: ScrollBehavior) => void + subscribeToStripResize: (listener: () => void) => () => void } { const tabStripRef = useRef(null) + const stripResizeListenersRef = useRef void>>(new Set()) const prevStripRef = useRef<{ worktreeId: string; tabIds: ReadonlySet } | null>(null) const stickToEndRef = useRef(false) const tabClosedThisCommitRef = useRef(false) @@ -79,7 +79,8 @@ export function useTabStripOverflowNavigation({ activeTabId: string | null anchor: TabStripScrollAnchor | null } | null>(null) - const [tabStripOverflowState, setTabStripOverflowState] = useState( + // Why no thumb position here: it changes every scroll frame, and every tab would re-render with it. + const [tabStripOverflowState, setTabStripOverflowState] = useState( EMPTY_TAB_STRIP_OVERFLOW_STATE ) const [activeTabDockSide, setActiveTabDockSide] = useState(null) @@ -88,9 +89,9 @@ export function useTabStripOverflowNavigation({ if (!el) { return } - const next = computeTabStripScrollMetrics(el) + const next = computeTabStripOverflowState(el) setTabStripOverflowState((previous) => - sameTabStripScrollMetrics(previous, next) ? previous : next + sameTabStripOverflowState(previous, next) ? previous : next ) setActiveTabDockSide(getActiveTabDockSide(el)) }, []) @@ -151,6 +152,9 @@ export function useTabStripOverflowNavigation({ el.scrollLeft = Math.max(0, el.scrollWidth - el.clientWidth) } recordScrollAnchor() + for (const listener of stripResizeListenersRef.current) { + listener() + } } const disconnectResizeObservers = bindTabStripContentResizeObservers(el, handleStripResize) @@ -300,5 +304,19 @@ export function useTabStripOverflowNavigation({ tabClosedThisCommitRef.current = false }) - return { tabStripRef, tabStripOverflowState, activeTabDockSide, scrollTabStrip } + // Why share these observers: each one watches every tab, so a second set doubles that work. + const subscribeToStripResize = useCallback((listener: () => void): (() => void) => { + stripResizeListenersRef.current.add(listener) + return () => { + stripResizeListenersRef.current.delete(listener) + } + }, []) + + return { + tabStripRef, + tabStripOverflowState, + activeTabDockSide, + scrollTabStrip, + subscribeToStripResize + } } diff --git a/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.test.ts b/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.test.ts index cbaa69603a5..8ede8fdd39e 100644 --- a/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.test.ts +++ b/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.test.ts @@ -1,14 +1,14 @@ import { describe, expect, it } from 'vitest' import { - computeTabStripScrollMetrics, + computeTabStripOverflowState, computeTabStripThumbLayout, getTabStripScrollMaskClassName } from './tab-strip-scroll-metrics' -describe('computeTabStripScrollMetrics', () => { +describe('computeTabStripOverflowState', () => { it('reports no overflow when all tabs fit', () => { expect( - computeTabStripScrollMetrics({ + computeTabStripOverflowState({ scrollWidth: 400, clientWidth: 400, scrollLeft: 0 @@ -16,38 +16,20 @@ describe('computeTabStripScrollMetrics', () => { ).toEqual({ hasOverflow: false, canScrollStart: false, - canScrollEnd: false, - thumbSizeFraction: 1, - thumbOffsetFraction: 0 - }) - }) - - it('tracks thumb size and offset while scrolled', () => { - expect( - computeTabStripScrollMetrics({ - scrollWidth: 800, - clientWidth: 400, - scrollLeft: 200 - }) - ).toEqual({ - hasOverflow: true, - canScrollStart: true, - canScrollEnd: true, - thumbSizeFraction: 0.5, - thumbOffsetFraction: 0.5 + canScrollEnd: false }) }) it('marks the start and end scroll edges', () => { expect( - computeTabStripScrollMetrics({ + computeTabStripOverflowState({ scrollWidth: 800, clientWidth: 400, scrollLeft: 0 }).canScrollStart ).toBe(false) expect( - computeTabStripScrollMetrics({ + computeTabStripOverflowState({ scrollWidth: 800, clientWidth: 400, scrollLeft: 0 @@ -55,14 +37,14 @@ describe('computeTabStripScrollMetrics', () => { ).toBe(true) expect( - computeTabStripScrollMetrics({ + computeTabStripOverflowState({ scrollWidth: 800, clientWidth: 400, scrollLeft: 400 }).canScrollStart ).toBe(true) expect( - computeTabStripScrollMetrics({ + computeTabStripOverflowState({ scrollWidth: 800, clientWidth: 400, scrollLeft: 400 @@ -74,22 +56,16 @@ describe('computeTabStripScrollMetrics', () => { describe('computeTabStripThumbLayout', () => { it('clamps thumb width and keeps the thumb inside the track', () => { expect( - computeTabStripThumbLayout(200, { - thumbSizeFraction: 0.04, - thumbOffsetFraction: 1 - }) + computeTabStripThumbLayout(200, { scrollWidth: 10_000, clientWidth: 400, scrollLeft: 9_600 }) ).toEqual({ widthPx: 18, leftPx: 182 }) }) - it('uses the raw width when it is already above the minimum', () => { + it('sizes and offsets the thumb from the strip scroll position', () => { expect( - computeTabStripThumbLayout(400, { - thumbSizeFraction: 0.5, - thumbOffsetFraction: 0.25 - }) + computeTabStripThumbLayout(400, { scrollWidth: 800, clientWidth: 400, scrollLeft: 100 }) ).toEqual({ widthPx: 200, leftPx: 50 diff --git a/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.ts b/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.ts index 39489e21a74..9f467c06d3c 100644 --- a/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.ts +++ b/src/renderer/src/components/tab-bar/tab-strip-scroll-metrics.ts @@ -1,13 +1,9 @@ import type { ActiveTabDockSide } from './tab-strip-slot-geometry' -export type TabStripScrollMetrics = { +export type TabStripOverflowState = { hasOverflow: boolean canScrollStart: boolean canScrollEnd: boolean - /** Portion of total tab width currently visible in the strip viewport. */ - thumbSizeFraction: number - /** 0 = scrolled to start, 1 = scrolled to end. */ - thumbOffsetFraction: number } export type TabStripThumbLayout = { @@ -15,48 +11,49 @@ export type TabStripThumbLayout = { leftPx: number } +type TabStripScrollBox = Pick + export const TAB_STRIP_THUMB_MIN_WIDTH_PX = 18 const OVERFLOW_EPSILON_PX = 1 -export function computeTabStripScrollMetrics( - el: Pick -): TabStripScrollMetrics { +export function computeTabStripOverflowState(el: TabStripScrollBox): TabStripOverflowState { const maxScrollLeft = Math.max(0, el.scrollWidth - el.clientWidth) const hasOverflow = maxScrollLeft > OVERFLOW_EPSILON_PX - const thumbSizeFraction = el.scrollWidth > 0 ? Math.min(1, el.clientWidth / el.scrollWidth) : 1 - const thumbOffsetFraction = hasOverflow && maxScrollLeft > 0 ? el.scrollLeft / maxScrollLeft : 0 return { hasOverflow, canScrollStart: hasOverflow && el.scrollLeft > OVERFLOW_EPSILON_PX, - canScrollEnd: hasOverflow && el.scrollLeft < maxScrollLeft - OVERFLOW_EPSILON_PX, - thumbSizeFraction, - thumbOffsetFraction + canScrollEnd: hasOverflow && el.scrollLeft < maxScrollLeft - OVERFLOW_EPSILON_PX } } export function computeTabStripThumbLayout( trackWidthPx: number, - metrics: Pick + el: TabStripScrollBox ): TabStripThumbLayout { if (trackWidthPx <= 0) { return { widthPx: 0, leftPx: 0 } } - const rawWidthPx = metrics.thumbSizeFraction * trackWidthPx - const widthPx = Math.min(trackWidthPx, Math.max(TAB_STRIP_THUMB_MIN_WIDTH_PX, rawWidthPx)) + const maxScrollLeft = Math.max(0, el.scrollWidth - el.clientWidth) + const sizeFraction = el.scrollWidth > 0 ? Math.min(1, el.clientWidth / el.scrollWidth) : 1 + const offsetFraction = maxScrollLeft > OVERFLOW_EPSILON_PX ? el.scrollLeft / maxScrollLeft : 0 + const widthPx = Math.min( + trackWidthPx, + Math.max(TAB_STRIP_THUMB_MIN_WIDTH_PX, sizeFraction * trackWidthPx) + ) const maxLeftPx = Math.max(0, trackWidthPx - widthPx) return { widthPx, - leftPx: metrics.thumbOffsetFraction * maxLeftPx + leftPx: offsetFraction * maxLeftPx } } /** `dockedSide` skips that edge's fade, which would otherwise wash out the active tab docked there. */ export function getTabStripScrollMaskClassName( - metrics: Pick, + metrics: TabStripOverflowState, dockedSide: ActiveTabDockSide | null = null ): string { if (!metrics.hasOverflow) { @@ -73,15 +70,13 @@ export function getTabStripScrollMaskClassName( return classes.join(' ') } -export function sameTabStripScrollMetrics( - left: TabStripScrollMetrics, - right: TabStripScrollMetrics +export function sameTabStripOverflowState( + left: TabStripOverflowState, + right: TabStripOverflowState ): boolean { return ( left.hasOverflow === right.hasOverflow && left.canScrollStart === right.canScrollStart && - left.canScrollEnd === right.canScrollEnd && - Math.abs(left.thumbSizeFraction - right.thumbSizeFraction) < 0.002 && - Math.abs(left.thumbOffsetFraction - right.thumbOffsetFraction) < 0.002 + left.canScrollEnd === right.canScrollEnd ) } diff --git a/tests/e2e/tab-strip-scroll-render-isolation.spec.ts b/tests/e2e/tab-strip-scroll-render-isolation.spec.ts new file mode 100644 index 00000000000..ca1336203d5 --- /dev/null +++ b/tests/e2e/tab-strip-scroll-render-isolation.spec.ts @@ -0,0 +1,250 @@ +/** + * E2E test for scrolling an overflowing tab strip: the scroll must not re-render the tabs, and the + * scroll thumb must still track the strip. + * + * Why E2E: only real Chromium lays out the strip and delivers the scroll and resize callbacks the + * thumb follows, and only the whole app shows every React commit a scroll step causes. + */ + +import type { Locator, Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { waitForSessionReady, waitForActiveWorktree, ensureTerminalVisible } from './helpers/store' + +const STRIP = '.terminal-tab-strip' +const START_SCROLL_LEFT_PX = 400 +const WHEEL_STEPS = 20 +const WHEEL_DELTA_PX = 50 +const MIN_SCROLL_RANGE_PX = 2_000 + +type CommitFiber = { + tag: number + flags: number + child: CommitFiber | null + sibling: CommitFiber | null + stateNode: unknown +} + +declare global { + // oxlint-disable-next-line typescript-eslint/consistent-type-definitions -- declaration merging requires interface + interface Window { + __REACT_DEVTOOLS_GLOBAL_HOOK__?: { + onCommitFiberRoot?: ( + rendererId: unknown, + root: { current: CommitFiber }, + ...rest: unknown[] + ) => unknown + } + __tabsRenderedPerCommit?: number[] + } +} + +async function addBackgroundTerminalTabs( + page: Page, + worktreeId: string, + count: number +): Promise { + return page.evaluate( + ({ wId, tabCount }) => + Array.from( + { length: tabCount }, + () => + window.__store!.getState().createTab(wId, undefined, undefined, { activate: false }).id + ), + { wId: worktreeId, tabCount: count } + ) +} + +/** + * Records how many tabs each React commit re-renders, the way React DevTools highlights updates: + * through the commit hook the renderer always installs. Commits that render no tab (the sidebar, + * terminals starting up) are left out. + */ +async function startRecordingTabRenders(page: Page): Promise { + await page.evaluate(() => { + // Function, class, forwardRef and memo components; bit 1 is React's PerformedWork flag. + const componentTags = new Set([0, 1, 11, 14, 15]) + const performedWork = 1 + const hook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__! + const original = hook.onCommitFiberRoot + // A subtree React skipped keeps last commit's fiber objects, whose flags are stale. + let previousFibers = new Set() + window.__tabsRenderedPerCommit = [] + hook.onCommitFiberRoot = function (rendererId, root, ...rest) { + const fibers = new Set() + const renderedTabIds = new Set() + const stack: CommitFiber[] = [root.current] + while (stack.length > 0) { + const fiber = stack.pop()! + fibers.add(fiber) + if ( + componentTags.has(fiber.tag) && + (fiber.flags & performedWork) === performedWork && + !previousFibers.has(fiber) + ) { + let host: CommitFiber | null = fiber + while (host && host.tag !== 5) { + host = host.child + } + const element = host?.stateNode + const tabId = + element instanceof Element + ? element.closest('[data-tab-strip-slot]')?.getAttribute('data-tab-strip-slot') + : undefined + if (tabId) { + renderedTabIds.add(tabId) + } + } + if (fiber.sibling) { + stack.push(fiber.sibling) + } + if (fiber.child) { + stack.push(fiber.child) + } + } + previousFibers = fibers + if (renderedTabIds.size > 0) { + window.__tabsRenderedPerCommit?.push(renderedTabIds.size) + } + return original?.call(this, rendererId, root, ...rest) + } + }) +} + +async function takeTabRenders(page: Page): Promise { + return page.evaluate(() => { + const recorded = window.__tabsRenderedPerCommit ?? [] + window.__tabsRenderedPerCommit = [] + return recorded + }) +} + +/** Thumb geometry next to where the strip's scroll position and size say it should be. */ +async function readThumb(strip: Locator) { + return strip.evaluate((el) => { + const track = el.parentElement!.querySelector( + '[data-testid="tab-strip-scroll-indicator"]' + )! + const trackRect = track.getBoundingClientRect() + const thumbRect = track + .querySelector('[data-testid="tab-strip-scroll-thumb"]')! + .getBoundingClientRect() + const maxScrollLeft = el.scrollWidth - el.clientWidth + const expectedWidth = Math.max(18, (el.clientWidth / el.scrollWidth) * trackRect.width) + return { + width: thumbRect.width, + expectedWidth, + left: thumbRect.left - trackRect.left, + expectedLeft: (el.scrollLeft / maxScrollLeft) * (trackRect.width - expectedWidth) + } + }) +} + +/** Polls rather than waiting frames: CI's hidden window can draw as little as one frame a second. */ +async function expectThumbToTrackStrip(strip: Locator): Promise { + await expect + .poll(async () => { + const thumb = await readThumb(strip) + return Math.max( + Math.abs(thumb.width - thumb.expectedWidth), + Math.abs(thumb.left - thumb.expectedLeft) + ) + }) + .toBeLessThanOrEqual(1) +} + +test.describe('Tab strip scroll render isolation', () => { + test.beforeEach(async ({ orcaPage }) => { + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + }) + + test('scrolls a long strip without re-rendering its tabs, and the thumb keeps up', async ({ + orcaPage + }) => { + const worktreeId = await waitForActiveWorktree(orcaPage) + const strip = orcaPage.locator(STRIP).first() + await expect(strip).toBeVisible() + const tabIds: string[] = [] + for (let i = 0; i < 12; i++) { + const range = await strip.evaluate((el) => el.scrollWidth - el.clientWidth) + if (range >= MIN_SCROLL_RANGE_PX) { + break + } + tabIds.push(...(await addBackgroundTerminalTabs(orcaPage, worktreeId, 10))) + await expect + .poll(() => strip.evaluate((el) => el.querySelectorAll('[data-tab-strip-slot]').length)) + .toBeGreaterThan(tabIds.length) + } + + await startRecordingTabRenders(orcaPage) + // New terminals retitle their tabs after opening, longer on a loaded machine; wait for quiet. + await expect + .poll( + async () => { + await takeTabRenders(orcaPage) + await orcaPage.waitForTimeout(1_500) + return (await takeTabRenders(orcaPage)).length + }, + { timeout: 45_000 } + ) + .toBe(0) + expect(await strip.evaluate((el) => el.scrollWidth - el.clientWidth)).toBeGreaterThanOrEqual( + MIN_SCROLL_RANGE_PX + ) + + // Start and end mid-strip so neither scroll edge flips the arrows, fades or dock. Why set it + // here and hold until it sticks: opening the tabs left the strip pinned to its end, and the + // scroll event that releases that pin only lands with the next frame — about a second in CI's + // hidden window. A tab retitling inside that window re-pins the strip to the end. + await expect + .poll( + async () => { + await strip.evaluate((el, left) => { + el.scrollLeft = left + }, START_SCROLL_LEFT_PX) + await orcaPage.waitForTimeout(1_500) + return strip.evaluate((el) => el.scrollLeft) + }, + { timeout: 30_000 } + ) + .toBe(START_SCROLL_LEFT_PX) + // That scroll flipped the end-edge flags; the commit it caused is not the scroll under test. + await takeTabRenders(orcaPage) + // Why wheel events dispatched in the page: real input waits for a frame per step, and CI's hidden + // window draws about one a second. The strip's own wheel handler still does the scrolling. + await strip.evaluate( + async (el, { steps, delta }) => { + for (let i = 0; i < steps; i++) { + el.dispatchEvent(new WheelEvent('wheel', { deltaY: delta, cancelable: true })) + await new Promise((resolve) => setTimeout(resolve, 20)) + } + // The scroll events arrive with the next frame; let React commit what they cause. + await new Promise(requestAnimationFrame) + await new Promise((resolve) => setTimeout(resolve, 20)) + }, + { steps: WHEEL_STEPS, delta: WHEEL_DELTA_PX } + ) + expect(await strip.evaluate((el) => el.scrollLeft)).toBe( + START_SCROLL_LEFT_PX + WHEEL_STEPS * WHEEL_DELTA_PX + ) + // Why "many tabs": before the fix every scroll step re-rendered all of them in one commit; + // a single tab can still update itself (a late retitle). + expect((await takeTabRenders(orcaPage)).filter((tabs) => tabs >= 3)).toEqual([]) + await expectThumbToTrackStrip(strip) + + // Control: switching tabs re-renders at least the tab it activates, so the recorder sees tabs. + await orcaPage.evaluate((tabId) => window.__store!.getState().setActiveTab(tabId), tabIds[0]) + await expect.poll(async () => (await takeTabRenders(orcaPage)).length).toBeGreaterThan(0) + + // Tabs opening grow the strip without a scroll event. + await addBackgroundTerminalTabs(orcaPage, worktreeId, 10) + await expectThumbToTrackStrip(strip) + + // A narrower pane shrinks the strip and the track without a scroll event. + await strip.evaluate((el) => { + el.closest('[data-native-file-drop-target]')!.style.maxWidth = '700px' + }) + await expectThumbToTrackStrip(strip) + }) +})