diff --git a/desktop/src/components/layout/TabBar.test.tsx b/desktop/src/components/layout/TabBar.test.tsx index 732d83d7..21c662e9 100644 --- a/desktop/src/components/layout/TabBar.test.tsx +++ b/desktop/src/components/layout/TabBar.test.tsx @@ -23,6 +23,36 @@ const sessionsApiMock = vi.hoisted(() => ({ delete: vi.fn(() => Promise.resolve()), })) +// The strip re-reveals a clipped active tab from its ResizeObserver, so the +// tests have to be able to fire one. jsdom lays nothing out, so the geometry +// the guard reads has to be stubbed alongside it. +const resizeObserverCallbacks = new Set() + +function stubRect(element: Element, left: number, right: number) { + Object.defineProperty(element, 'getBoundingClientRect', { + configurable: true, + value: () => ({ + left, + right, + width: right - left, + top: 0, + bottom: 46, + height: 46, + x: left, + y: 0, + toJSON: () => ({}), + }), + }) +} + +function fireStripResize() { + act(() => { + for (const callback of [...resizeObserverCallbacks]) { + callback([], {} as ResizeObserver) + } + }) +} + function makeChatSession(chatState: ChatState): PerSessionState { return { messages: [], @@ -158,12 +188,19 @@ describe('TabBar', () => { } beforeEach(() => { + resizeObserverCallbacks.clear() + class ResizeObserverMock { - constructor(_callback: ResizeObserverCallback) {} + constructor(private readonly callback: ResizeObserverCallback) { + resizeObserverCallbacks.add(callback) + } observe(_target: Element) {} - disconnect() {} + disconnect() { + resizeObserverCallbacks.delete(this.callback) + } + unobserve() {} } @@ -880,6 +917,131 @@ describe('TabBar', () => { }) }) + describe('keeping the active tab whole while the strip resizes', () => { + // The chevrons are `w-7` siblings of the scroll region, so the moment the + // strip is found to overflow they take 28px each out of it — after the + // activation scroll has already landed on a scrollLeft computed without + // them. Measured on a 1280px window with seven tabs: the scroll stopped at + // 108 when the reachable end had moved to 164, and the last tab lost + // exactly those 56px, taking the close button past the strip edge, where + // elementFromPoint returned the toolbar's terminal button instead. So the + // tab could not be closed at all, not merely not seen. + async function renderOverflowingStrip() { + const { TabBar } = await import('./TabBar') + const { useTabStore } = await import('../../stores/tabStore') + const { useChatStore } = await import('../../stores/chatStore') + + useTabStore.setState({ + tabs: [ + { sessionId: 'tab-1', title: 'First Session', type: 'session', status: 'idle' }, + { sessionId: 'tab-2', title: 'Second Session', type: 'session', status: 'idle' }, + { sessionId: 'tab-3', title: 'Third Session', type: 'session', status: 'idle' }, + { sessionId: 'tab-4', title: 'Fourth Session', type: 'session', status: 'idle' }, + { sessionId: 'tab-5', title: 'Last Session', type: 'session', status: 'idle' }, + ], + activeTabId: 'tab-5', + }) + useChatStore.setState({ + sessions: {}, + disconnectSession: vi.fn(), + } as Partial>) + + await act(async () => { + render() + }) + + const strip = screen.getByTestId('tab-bar-scroll-region') + const activeTab = strip.querySelector('[data-active="true"]') as HTMLElement + expect(activeTab).toBeInTheDocument() + + // The strip runs 0–840 once both chevrons are in. Everything below moves + // only the active tab's own rect against that. + stubRect(strip, 0, 840) + Object.defineProperty(strip, 'clientWidth', { configurable: true, get: () => 840 }) + Object.defineProperty(strip, 'scrollWidth', { configurable: true, get: () => 1004 }) + Object.defineProperty(strip, 'scrollLeft', { configurable: true, get: () => 108 }) + Object.defineProperty(strip, 'scrollBy', { configurable: true, value: vi.fn() }) + + return { strip, activeTab, useTabStore } + } + + it('re-reveals the active tab when a resize clips it', async () => { + const { activeTab } = await renderOverflowingStrip() + // 56px past the strip's right edge — the close button's slot. + stubRect(activeTab, 640, 896) + scrollIntoViewMock.mockClear() + + fireStripResize() + + expect(scrollIntoViewMock).toHaveBeenCalledWith({ + block: 'nearest', + inline: 'nearest', + behavior: 'smooth', + }) + }) + + it('leaves an already whole active tab where it is', async () => { + const { activeTab } = await renderOverflowingStrip() + stubRect(activeTab, 640, 840) + scrollIntoViewMock.mockClear() + + fireStripResize() + + // Nothing is clipped, so a resize must not scroll. Without the tolerance + // in the visibility test, subpixel edges would land here and re-scroll on + // every single resize. + expect(scrollIntoViewMock).not.toHaveBeenCalled() + }) + + it('leaves the strip alone once the user has driven it with a chevron', async () => { + const { activeTab } = await renderOverflowingStrip() + stubRect(activeTab, 640, 896) + + const leftChevron = await waitFor(() => { + const button = screen.getByText('chevron_left').closest('button') + expect(button).toBeInTheDocument() + return button as HTMLButtonElement + }) + fireEvent.click(leftChevron) + scrollIntoViewMock.mockClear() + + // A chevron press is itself a resize source: reaching an end retires one + // chevron and leaving an end brings the other back, so a plain user + // scroll fires this observer mid-flight. Realigning there snapped the + // view straight back and made the left end unreachable. + fireStripResize() + + expect(scrollIntoViewMock).not.toHaveBeenCalled() + }) + + it('takes the strip back over when the user switches tabs', async () => { + const { activeTab, strip, useTabStore } = await renderOverflowingStrip() + stubRect(activeTab, 640, 896) + + const leftChevron = await waitFor(() => { + const button = screen.getByText('chevron_left').closest('button') + expect(button).toBeInTheDocument() + return button as HTMLButtonElement + }) + fireEvent.click(leftChevron) + + await act(async () => { + useTabStore.getState().setActiveTab('tab-4') + }) + const nextActiveTab = strip.querySelector('[data-active="true"]') as HTMLElement + stubRect(nextActiveTab, 640, 896) + scrollIntoViewMock.mockClear() + + fireStripResize() + + expect(scrollIntoViewMock).toHaveBeenCalledWith({ + block: 'nearest', + inline: 'nearest', + behavior: 'smooth', + }) + }) + }) + it('keeps the overflow button flush against window controls on Windows', async () => { const { TabBar } = await import('./TabBar') const { useTabStore } = await import('../../stores/tabStore') diff --git a/desktop/src/components/layout/TabBar.tsx b/desktop/src/components/layout/TabBar.tsx index d100c90b..f287c462 100644 --- a/desktop/src/components/layout/TabBar.tsx +++ b/desktop/src/components/layout/TabBar.tsx @@ -41,6 +41,17 @@ const DRAG_START_THRESHOLD = 4 // undershoot a row of long ones; leaving a quarter behind keeps the tab you // were looking at on screen as an anchor. const SCROLL_STEP_RATIO = 0.75 +// `nearest` on both axes: bring the tab fully on screen with the smallest +// possible move, and leave a tab that is already whole exactly where it is. +const REVEAL_ACTIVE_TAB: ScrollIntoViewOptions = { + block: 'nearest', + inline: 'nearest', + behavior: 'smooth', +} +// Subpixel slack for the "is the active tab whole?" test. Without it a strip +// whose edges land on fractional pixels reports the tab as clipped on every +// single resize and re-scrolls forever. +const TAB_VISIBILITY_TOLERANCE = 1 // One glyph per *non-chat* tab kind: the glyph says "this tab is not a // conversation". Chat tabs deliberately have none — a bubble on every tab in a // strip that is mostly chats is pure noise, and the slot it occupied is worth @@ -170,6 +181,9 @@ export function TabBar() { const moveTab = useTabStore((s) => s.moveTab) const scrollRef = useRef(null) + // Set the moment the user drives the strip themselves, cleared when they + // switch tabs. See `realignActiveTab`. + const userScrolledRef = useRef(false) const [canScrollLeft, setCanScrollLeft] = useState(false) const [canScrollRight, setCanScrollRight] = useState(false) const [contextMenu, setContextMenu] = useState<{ sessionId: string; x: number; y: number } | null>(null) @@ -201,29 +215,76 @@ export function TabBar() { setCanScrollRight(el.scrollLeft + el.clientWidth < el.scrollWidth - 1) }, []) + // Keeping the active tab whole is an invariant the strip has to re-establish + // after its own width changes, not something a single scroll on activation + // can settle. The chevrons are why: they are `w-7` siblings of this region, + // so the moment `updateScrollState` decides the strip overflows they take + // 28px each out of it — *after* the activation scroll has already landed on + // a scrollLeft computed without them. Measured on a 1280px window with seven + // tabs: the scroll stopped at 108 when the reachable end had moved to 164, + // and the last tab lost exactly those 56px off its right edge, taking the + // close button with it. Not merely hidden — the button's centre sat past the + // strip, so `elementFromPoint` there returned the toolbar's terminal button + // and the tab could not be closed at all. Window resizes, sidebar drags and + // the toolbar's own conditional buttons narrow the region the same way. + const realignActiveTab = useCallback(() => { + // Once the user has driven the strip with a chevron, where it sits is what + // they asked for, and the active tab being half off the edge is an + // ordinary consequence of scrolling a row. Only reinstate the invariant + // when the position is still ours to choose. Width alone cannot stand in + // for this: a chevron retires when its end is reached and rejoins when it + // is left, so a plain user scroll narrows the strip mid-flight and looks + // exactly like the layout event this guards against — measured, the view + // snapped straight back and the left end became unreachable. + if (userScrolledRef.current) return + + const el = scrollRef.current + if (!el) return + const currentActiveTabId = useTabStore.getState().activeTabId + if (!currentActiveTabId) return + const activeTabEl = tabRefs.current.get(currentActiveTabId) + if (!activeTabEl) return + + const strip = el.getBoundingClientRect() + const tab = activeTabEl.getBoundingClientRect() + + // Already whole. The tolerance is for subpixel layout, which would + // otherwise report a clip on every resize and scroll forever. + if ( + tab.left >= strip.left - TAB_VISIBILITY_TOLERANCE && + tab.right <= strip.right + TAB_VISIBILITY_TOLERANCE + ) return + + activeTabEl.scrollIntoView(REVEAL_ACTIVE_TAB) + }, []) + useEffect(() => { updateScrollState() const el = scrollRef.current if (!el) return el.addEventListener('scroll', updateScrollState) - const ro = new ResizeObserver(updateScrollState) + const ro = new ResizeObserver(() => { + updateScrollState() + realignActiveTab() + }) ro.observe(el) return () => { el.removeEventListener('scroll', updateScrollState) ro.disconnect() } - }, [updateScrollState, tabs.length]) + }, [realignActiveTab, updateScrollState, tabs.length]) useEffect(() => { if (!activeTabId) return const activeTabEl = tabRefs.current.get(activeTabId) if (!activeTabEl) return - activeTabEl.scrollIntoView({ - block: 'nearest', - inline: 'nearest', - behavior: 'smooth', - }) + // Switching tabs hands the position back to the strip: wherever the user + // had scrolled to, they have now named a tab they want to see. + userScrolledRef.current = false + // Unconditional, unlike the resize path: a tab that has just been activated + // has to come on screen even from completely outside the strip. + activeTabEl.scrollIntoView(REVEAL_ACTIVE_TAB) const frame = window.requestAnimationFrame(updateScrollState) return () => window.cancelAnimationFrame(frame) @@ -244,6 +305,10 @@ export function TabBar() { const el = scrollRef.current if (!el) return const step = el.clientWidth * SCROLL_STEP_RATIO + // The chevrons are the only way to drive the strip by hand — it is + // `overflow-x-hidden`, so wheel and trackpad do not reach it — which makes + // this the one place that has to hand the position over to the user. + userScrolledRef.current = true el.scrollBy({ left: direction === 'left' ? -step : step, behavior: 'smooth' }) }