From 549816c9869084b70d6767ea720bc437a5021bcf Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:59:25 -0700 Subject: [PATCH] perf(tabs): take split-divider drag off the store (STA-3328) (#12392) * perf(tabs): take split-divider drag off the store (STA-3328) Every pointermove committed a global store write (60-120 publications/s against every subscriber) plus a forced reflow from per-move getBoundingClientRect. The drag now writes the two panes' flex styles directly (identical visuals) and commits setTabGroupSplitRatio once on release/unmount; the action bails without minting state when the ratio is unchanged. * fix(tabs): keep deferred divider commits coherent * fix(tabs): preserve divider pointer ownership --- .../TabGroupSplitLayout.drag.test.tsx | 247 ++++++++++++++++++ .../tab-group/TabGroupSplitLayout.tsx | 50 +++- src/renderer/src/store/slices/tabs.test.ts | 31 +++ src/renderer/src/store/slices/tabs.ts | 14 +- 4 files changed, 331 insertions(+), 11 deletions(-) create mode 100644 src/renderer/src/components/tab-group/TabGroupSplitLayout.drag.test.tsx diff --git a/src/renderer/src/components/tab-group/TabGroupSplitLayout.drag.test.tsx b/src/renderer/src/components/tab-group/TabGroupSplitLayout.drag.test.tsx new file mode 100644 index 000000000..8cd7374b8 --- /dev/null +++ b/src/renderer/src/components/tab-group/TabGroupSplitLayout.drag.test.tsx @@ -0,0 +1,247 @@ +// @vitest-environment happy-dom + +// Why: the sibling TabGroupSplitLayout.test.ts calls components as plain +// functions and cannot exercise the drag gesture; these tests real-render the +// handle to pin the STA-3328 contract — pointermove writes pane styles +// directly and the store is committed exactly once, on release. +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +globalThis.IS_REACT_ACT_ENVIRONMENT = true + +const setTabGroupSplitRatioMock = vi.fn() +const recordFeatureInteractionMock = vi.fn() + +vi.mock('../../store', () => ({ + useAppStore: (selector: (state: Record) => unknown) => + selector({ + recordFeatureInteraction: recordFeatureInteractionMock, + setTabGroupSplitRatio: setTabGroupSplitRatioMock + }) +})) + +vi.mock('./TabGroupPanel', () => ({ + default: ({ groupId }: { groupId: string }) =>
+})) + +vi.mock('./useTabDragSplit', () => ({ + useTabDragSplit: () => ({ + activeDrag: null, + collisionDetection: vi.fn(), + hoveredDropTarget: null, + hoveredTabInsertion: null, + isTabDragActiveRef: { current: false }, + onDragCancel: vi.fn(), + onDragEnd: vi.fn(), + onDragMove: vi.fn(), + onDragOver: vi.fn(), + onDragStart: vi.fn(), + sensors: [], + setDragRootNode: vi.fn() + }) +})) + +import TabGroupSplitLayout from './TabGroupSplitLayout' + +function pointerEvent(type: string, init: { pointerId?: number; clientX?: number }): Event { + const event = new Event(type, { bubbles: true, cancelable: true }) + Object.defineProperty(event, 'pointerId', { value: init.pointerId ?? 1 }) + Object.defineProperty(event, 'clientX', { value: init.clientX ?? 0 }) + Object.defineProperty(event, 'clientY', { value: 0 }) + return event +} + +describe('TabGroupSplitLayout divider drag', () => { + let container: HTMLDivElement + let root: Root | null + let handle: HTMLElement + let firstPane: HTMLElement + let secondPane: HTMLElement + let containerRect: DOMRect + let resizeObserverCallback: ResizeObserverCallback | null + const resizeObserverDisconnectMock = vi.fn() + + beforeEach(async () => { + setTabGroupSplitRatioMock.mockClear() + recordFeatureInteractionMock.mockClear() + resizeObserverDisconnectMock.mockClear() + resizeObserverCallback = null + vi.stubGlobal( + 'ResizeObserver', + class { + private readonly callback: ResizeObserverCallback + private tracksSplitContainer = false + + constructor(callback: ResizeObserverCallback) { + this.callback = callback + } + + observe(target: Element): void { + if (target === handle?.parentElement) { + this.tracksSplitContainer = true + resizeObserverCallback = this.callback + } + } + unobserve(): void {} + disconnect(): void { + if (this.tracksSplitContainer) { + resizeObserverDisconnectMock() + } + } + } + ) + container = document.createElement('div') + document.body.appendChild(container) + const mountedRoot = createRoot(container) + root = mountedRoot + await act(async () => { + mountedRoot.render( + + ) + }) + handle = container.querySelector('.tab-group-split-resize-handle') as HTMLElement + expect(handle).not.toBeNull() + firstPane = handle.previousElementSibling as HTMLElement + secondPane = handle.nextElementSibling as HTMLElement + const capturedPointers = new Set() + Object.assign(handle, { + setPointerCapture: (pointerId: number) => { + capturedPointers.add(pointerId) + }, + releasePointerCapture: (pointerId: number) => { + capturedPointers.delete(pointerId) + }, + hasPointerCapture: (pointerId: number) => capturedPointers.has(pointerId) + }) + const splitContainer = handle.parentElement as HTMLElement + containerRect = { + left: 0, + top: 0, + width: 1000, + height: 500, + right: 1000, + bottom: 500 + } as DOMRect + splitContainer.getBoundingClientRect = vi.fn(() => containerRect) + }) + + afterEach(() => { + act(() => root?.unmount()) + container.remove() + vi.unstubAllGlobals() + }) + + it('writes pane styles per move and commits the store once on release', async () => { + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + }) + expect(recordFeatureInteractionMock).toHaveBeenCalledWith('terminal-panes') + + await act(async () => { + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 300 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 320 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 340 })) + }) + + // Why: the drag must never publish store updates per move (STA-3328) — + // the panes track the pointer via direct style writes instead. + expect(setTabGroupSplitRatioMock).not.toHaveBeenCalled() + expect(firstPane.style.flex).toBe('0.34 1 0%') + expect(secondPane.style.flex).toBe('0.66 1 0%') + + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 1 })) + }) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledTimes(1) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.34) + }) + + it('clamps the committed ratio and skips the commit when nothing moved', async () => { + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 1 })) + }) + expect(setTabGroupSplitRatioMock).not.toHaveBeenCalled() + + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 20 })) + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 1 })) + }) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledTimes(1) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.15) + }) + + it('refreshes drag geometry only when the split container resizes', async () => { + const getRectMock = handle.parentElement?.getBoundingClientRect as ReturnType + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 250 })) + }) + expect(firstPane.style.flex).toBe('0.25 1 0%') + expect(getRectMock).toHaveBeenCalledOnce() + + containerRect = { ...containerRect, width: 500, right: 500 } as DOMRect + resizeObserverCallback?.([], {} as ResizeObserver) + await act(async () => { + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 250 })) + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 1 })) + }) + + expect(firstPane.style.flex).toBe('0.5 1 0%') + expect(getRectMock).toHaveBeenCalledTimes(2) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.5) + expect(resizeObserverDisconnectMock).toHaveBeenCalledOnce() + }) + + it('ignores events from pointers that do not own the drag', async () => { + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 350 })) + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 2 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 2, clientX: 300 })) + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 2 })) + }) + expect(firstPane.style.flex).toBe('0.35 1 0%') + expect(setTabGroupSplitRatioMock).not.toHaveBeenCalled() + + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerup', { pointerId: 1 })) + }) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.35) + }) + + it.each(['pointercancel', 'lostpointercapture'])('commits on %s', async (eventType) => { + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 400 })) + handle.dispatchEvent(pointerEvent(eventType, { pointerId: 1 })) + }) + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.4) + expect(resizeObserverDisconnectMock).toHaveBeenCalledOnce() + }) + + it('commits and releases drag resources on unmount', async () => { + await act(async () => { + handle.dispatchEvent(pointerEvent('pointerdown', { pointerId: 1 })) + handle.dispatchEvent(pointerEvent('pointermove', { pointerId: 1, clientX: 450 })) + }) + act(() => root?.unmount()) + root = null + + expect(setTabGroupSplitRatioMock).toHaveBeenCalledWith('wt-1', '', 0.45) + expect(resizeObserverDisconnectMock).toHaveBeenCalledOnce() + }) +}) diff --git a/src/renderer/src/components/tab-group/TabGroupSplitLayout.tsx b/src/renderer/src/components/tab-group/TabGroupSplitLayout.tsx index 04cc65362..b467e5822 100644 --- a/src/renderer/src/components/tab-group/TabGroupSplitLayout.tsx +++ b/src/renderer/src/components/tab-group/TabGroupSplitLayout.tsx @@ -34,25 +34,45 @@ function ResizeHandle({ const onPointerDown = useCallback( (event: React.PointerEvent) => { event.preventDefault() + // Why: a second pointer must not steal or finalize the active gesture. + if (activeResizeCleanupRef.current) { + return + } const handle = event.currentTarget const container = handle.parentElement if (!container) { return } - activeResizeCleanupRef.current?.() + const firstPane = handle.previousElementSibling as HTMLElement | null + const secondPane = handle.nextElementSibling as HTMLElement | null + if (!firstPane || !secondPane) { + return + } onResizeStart() setDragging(true) handle.setPointerCapture(event.pointerId) + // Why: measure outside pointermove so pane writes never force a readback. + let rect = container.getBoundingClientRect() + const resizeObserver = new ResizeObserver(() => { + rect = container.getBoundingClientRect() + }) + resizeObserver.observe(container) + let draggedRatio: number | null = null const onPointerMove = (moveEvent: PointerEvent): void => { - if (!handle.hasPointerCapture(event.pointerId)) { + if (moveEvent.pointerId !== event.pointerId || !handle.hasPointerCapture(event.pointerId)) { return } - const rect = container.getBoundingClientRect() const ratio = isHorizontal ? (moveEvent.clientX - rect.left) / rect.width : (moveEvent.clientY - rect.top) / rect.height - onRatioChange(Math.min(MAX_RATIO, Math.max(MIN_RATIO, ratio))) + const clamped = Math.min(MAX_RATIO, Math.max(MIN_RATIO, ratio)) + draggedRatio = clamped + // Why: direct style writes keep the drag off the store — a commit per + // pointermove published 60-120 global store updates/s against every + // subscriber (STA-3328). React re-applies identical flex on commit. + firstPane.style.flex = `${clamped} 1 0%` + secondPane.style.flex = `${1 - clamped} 1 0%` } let cleaned = false @@ -61,6 +81,10 @@ function ResizeHandle({ return } cleaned = true + resizeObserver.disconnect() + if (draggedRatio !== null) { + onRatioChange(draggedRatio) + } if (updateDragging) { setDragging(false) } @@ -80,16 +104,22 @@ function ResizeHandle({ } } - const onPointerUp = (): void => { - cleanup() + const onPointerUp = (upEvent: PointerEvent): void => { + if (upEvent.pointerId === event.pointerId) { + cleanup() + } } - const onPointerCancel = (): void => { - cleanup() + const onPointerCancel = (cancelEvent: PointerEvent): void => { + if (cancelEvent.pointerId === event.pointerId) { + cleanup() + } } - const onLostPointerCapture = (): void => { - cleanup() + const onLostPointerCapture = (lostEvent: PointerEvent): void => { + if (lostEvent.pointerId === event.pointerId) { + cleanup() + } } handle.addEventListener('pointermove', onPointerMove) diff --git a/src/renderer/src/store/slices/tabs.test.ts b/src/renderer/src/store/slices/tabs.test.ts index 30bcdb249..c7bab81ff 100644 --- a/src/renderer/src/store/slices/tabs.test.ts +++ b/src/renderer/src/store/slices/tabs.test.ts @@ -949,6 +949,37 @@ describe('TabsSlice', () => { expect(layout.ratio).toBe(0.5) expect(layout.second.ratio).toBe(0.7) }) + + it('keeps state identity when the ratio is unchanged', () => { + store.setState({ + layoutByWorktree: { + [WT]: { + type: 'split', + direction: 'horizontal', + ratio: 0.5, + first: { type: 'leaf', groupId: 'g-1' }, + second: { type: 'leaf', groupId: 'g-2' } + } + } + }) + const beforeState = store.getState() + const beforeLayout = beforeState.layoutByWorktree + const subscriber = vi.fn() + const unsubscribe = store.subscribe(subscriber) + + // Why: an unchanged commit must not mint fresh root state — every store + // subscriber wakes on the new reference (STA-3328). + store.getState().setTabGroupSplitRatio(WT, '', 0.5) + expect(store.getState()).toBe(beforeState) + expect(store.getState().layoutByWorktree).toBe(beforeLayout) + expect(subscriber).not.toHaveBeenCalled() + + store.getState().setTabGroupSplitRatio(WT, '', 0.5004) + expect(store.getState().layoutByWorktree).not.toBe(beforeLayout) + expect(subscriber).toHaveBeenCalledOnce() + expect((store.getState().layoutByWorktree[WT] as { ratio: number }).ratio).toBe(0.5004) + unsubscribe() + }) }) describe('move/copy/merge group operations', () => { diff --git a/src/renderer/src/store/slices/tabs.ts b/src/renderer/src/store/slices/tabs.ts index 5a2456c24..7ce9dec3d 100644 --- a/src/renderer/src/store/slices/tabs.ts +++ b/src/renderer/src/store/slices/tabs.ts @@ -1999,7 +1999,19 @@ export const createTabsSlice: StateCreator = (set, set((state) => { const currentLayout = state.layoutByWorktree[worktreeId] if (!currentLayout) { - return {} + return state + } + // Why: an unchanged ratio must not mint fresh root state — every store + // subscriber wakes on the new reference (STA-3328). + let targetNode: TabGroupLayoutNode | undefined = currentLayout + for (const segment of nodePath.length > 0 ? nodePath.split('.') : []) { + targetNode = + targetNode && targetNode.type === 'split' && (segment === 'first' || segment === 'second') + ? targetNode[segment] + : undefined + } + if (!targetNode || targetNode.type !== 'split' || (targetNode.ratio ?? 0.5) === ratio) { + return state } return { layoutByWorktree: {