From ec01b46cbdfd5bf4382becdae6935cf0b1cdc539 Mon Sep 17 00:00:00 2001 From: lucaronin Date: Tue, 24 Feb 2026 10:13:43 +0100 Subject: [PATCH 1/2] fix: use refs in useTabDrag to prevent stale closures on drop handleDrop and handleDragOver closed over dragIndex/dropIndex state values. When the last dragover and drop events fire in the same React render cycle, the drop handler reads stale state (often null), causing computeDropTarget to return null and skip the reorder. Mirror dragIndex/dropIndex into refs that are updated synchronously alongside setState. Event handlers now read from refs, ensuring they always see the latest values regardless of React's batching schedule. Co-Authored-By: Claude Opus 4.6 --- src/components/TabBar.tsx | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/components/TabBar.tsx b/src/components/TabBar.tsx index 73ef729a..f0a30826 100644 --- a/src/components/TabBar.tsx +++ b/src/components/TabBar.tsx @@ -100,16 +100,25 @@ function computeInsertIndex(e: React.DragEvent, index: number): function useTabDrag(onReorderTabs?: (from: number, to: number) => void) { const [dragIndex, setDragIndex] = useState(null) const [dropIndex, setDropIndex] = useState(null) + // Refs mirror state so event handlers always read the latest values, + // avoiding stale closures when dragover and drop fire in the same frame. + const dragIndexRef = useRef(null) + const dropIndexRef = useRef(null) const dragNodeRef = useRef(null) + const onReorderRef = useRef(onReorderTabs) + useEffect(() => { onReorderRef.current = onReorderTabs }) const resetDrag = useCallback(() => { if (dragNodeRef.current) dragNodeRef.current.style.opacity = '' dragNodeRef.current = null + dragIndexRef.current = null + dropIndexRef.current = null setDragIndex(null) setDropIndex(null) }, []) const handleDragStart = useCallback((e: React.DragEvent, index: number) => { + dragIndexRef.current = index setDragIndex(index) e.dataTransfer.effectAllowed = 'move' e.dataTransfer.setData('text/plain', String(index)) @@ -122,20 +131,32 @@ function useTabDrag(onReorderTabs?: (from: number, to: number) => void) { const handleDragOver = useCallback((e: React.DragEvent, index: number) => { e.preventDefault() e.dataTransfer.dropEffect = 'move' - if (dragIndex === null || dragIndex === index) { setDropIndex(null); return } - setDropIndex(computeInsertIndex(e, index)) - }, [dragIndex]) + const currentDrag = dragIndexRef.current + if (currentDrag === null || currentDrag === index) { + dropIndexRef.current = null + setDropIndex(null) + return + } + const idx = computeInsertIndex(e, index) + dropIndexRef.current = idx + setDropIndex(idx) + }, []) const handleDrop = useCallback((e: React.DragEvent) => { e.preventDefault() - const toIndex = computeDropTarget(dragIndex, dropIndex) - if (toIndex !== null && onReorderTabs) onReorderTabs(dragIndex!, toIndex) + const toIndex = computeDropTarget(dragIndexRef.current, dropIndexRef.current) + if (toIndex !== null && onReorderRef.current) { + onReorderRef.current(dragIndexRef.current!, toIndex) + } resetDrag() - }, [dragIndex, dropIndex, onReorderTabs, resetDrag]) + }, [resetDrag]) const handleBarDragLeave = useCallback((e: React.DragEvent) => { const related = e.relatedTarget as HTMLElement | null - if (!e.currentTarget.contains(related)) setDropIndex(null) + if (!e.currentTarget.contains(related)) { + dropIndexRef.current = null + setDropIndex(null) + } }, []) return { dragIndex, dropIndex, handleDragStart, handleDragEnd: resetDrag, handleDragOver, handleDrop, handleBarDragLeave } From e288ccc9059bb3b53c77f468a0b370a9397b0fac Mon Sep 17 00:00:00 2001 From: lucaronin Date: Tue, 24 Feb 2026 10:17:48 +0100 Subject: [PATCH 2/2] test: add edge case tests for tab drag-and-drop reorder - Drag cancel (dragEnd without drop) does not trigger reorder - Drag from last tab toward first produces correct reorder - Active tab path is preserved after reorder Co-Authored-By: Claude Opus 4.6 --- src/components/TabBar.test.tsx | 65 ++++++++++++++++++++++++++++++ src/hooks/useTabManagement.test.ts | 22 ++++++++++ 2 files changed, 87 insertions(+) diff --git a/src/components/TabBar.test.tsx b/src/components/TabBar.test.tsx index fa150e8a..5ddf4632 100644 --- a/src/components/TabBar.test.tsx +++ b/src/components/TabBar.test.tsx @@ -132,6 +132,71 @@ describe('TabBar', () => { expect(screen.getAllByTestId('tab-modified-indicator')).toHaveLength(3) }) + it('does not reorder on drag cancel (dragEnd without drop)', () => { + const onReorderTabs = vi.fn() + const tabs = makeTabs(['Alpha', 'Beta', 'Gamma']) + render( + + ) + + const alphaTab = screen.getByText('Alpha').closest('[draggable]')! + const betaTab = screen.getByText('Beta').closest('[draggable]')! + + fireEvent.dragStart(alphaTab, { + dataTransfer: { effectAllowed: 'move', setData: vi.fn() }, + }) + + const rect = betaTab.getBoundingClientRect() + fireEvent.dragOver(betaTab, { + clientX: rect.left + rect.width * 0.75, + dataTransfer: { dropEffect: 'move' }, + }) + + // Cancel via dragEnd (Escape or release outside tab bar) + fireEvent.dragEnd(alphaTab) + + expect(onReorderTabs).not.toHaveBeenCalled() + }) + + it('reorders from last toward first position', () => { + const onReorderTabs = vi.fn() + const tabs = makeTabs(['Alpha', 'Beta', 'Gamma']) + render( + + ) + + const gammaTab = screen.getByText('Gamma').closest('[draggable]')! + const alphaTab = screen.getByText('Alpha').closest('[draggable]')! + + fireEvent.dragStart(gammaTab, { + dataTransfer: { effectAllowed: 'move', setData: vi.fn() }, + }) + + // jsdom returns zero-sized rects, so clientX always hits "right half" + // (insert after index 0 → insert index 1). This still validates + // that dragging the last tab toward the front produces a reorder. + const rect = alphaTab.getBoundingClientRect() + fireEvent.dragOver(alphaTab, { + clientX: rect.left + rect.width * 0.75, + dataTransfer: { dropEffect: 'move' }, + }) + + fireEvent.drop(alphaTab, { dataTransfer: {} }) + + // Gamma (2) dragged onto Alpha (0) → reorder from 2 to 1 + expect(onReorderTabs).toHaveBeenCalledWith(2, 1) + }) + it('switches tab on click', () => { const onSwitchTab = vi.fn() const tabs = makeTabs(['Alpha', 'Beta']) diff --git a/src/hooks/useTabManagement.test.ts b/src/hooks/useTabManagement.test.ts index 749a2c79..ca7186d9 100644 --- a/src/hooks/useTabManagement.test.ts +++ b/src/hooks/useTabManagement.test.ts @@ -205,6 +205,28 @@ describe('useTabManagement', () => { expect(result.current.tabs.map(t => t.entry.title)).toEqual(['C', 'A', 'B']) }) + it('preserves active tab after reorder', async () => { + const { result } = renderHook(() => useTabManagement()) + + await act(async () => { + await result.current.handleSelectNote(makeEntry({ path: '/vault/a.md', title: 'A' })) + }) + await act(async () => { + await result.current.handleSelectNote(makeEntry({ path: '/vault/b.md', title: 'B' })) + }) + await act(async () => { + await result.current.handleSelectNote(makeEntry({ path: '/vault/c.md', title: 'C' })) + }) + + // C is active (last opened). Move it to the front. + act(() => { + result.current.handleReorderTabs(2, 0) + }) + + expect(result.current.tabs.map(t => t.entry.title)).toEqual(['C', 'A', 'B']) + expect(result.current.activeTabPath).toBe('/vault/c.md') + }) + it('persists tab order to localStorage', async () => { const { result } = renderHook(() => useTabManagement())