fix(review): harden kanban virtualization interaction edges

Cancel deferred card mount on close, re-apply marquee preview after remounts, query drag styles from live DOM, drop re-exports/casts, and cover edge cases.
2026-10-20 08:03:28 +00:00 · 2026-07-28 20:55:38 -07:00
parent 6d63bcac04
commit 951b3eb0ca
10 changed files with 236 additions and 85 deletions
@@ -652,10 +652,18 @@ export default function WorkspaceKanbanDrawer({
      setRenderCards(false)
      return
    }
+    let cancelled = false
    const frameId = window.requestAnimationFrame(() => {
-      startTransition(() => setRenderCards(true))
+      startTransition(() => {
+        if (!cancelled) {
+          setRenderCards(true)
+        }
+      })
    })
-    return () => window.cancelAnimationFrame(frameId)
+    return () => {
+      cancelled = true
+      window.cancelAnimationFrame(frameId)
+    }
  }, [open])

  useWorkspaceKanbanShiftWheelScroll(boardRef, laneScrollerRef, open, isPointerDragActiveRef)
@@ -18,20 +18,24 @@ const WINDOW_END = 25
 const TOTAL_SIZE = 21_992

 vi.mock('@tanstack/react-virtual', () => ({
-  useVirtualizer: ({ count }: { count: number }) => ({
-    getTotalSize: () => TOTAL_SIZE,
-    measurementsCache: Array.from({ length: count }, (_, index) => ({
-      index,
-      start: index * 44,
-      end: index * 44 + 36
-    })),
-    getVirtualItems: () =>
-      Array.from({ length: WINDOW_END - WINDOW_START + 1 }, (_, offset) => {
-        const index = WINDOW_START + offset
-        return { index, key: `w${index}`, start: index * 44 }
-      }),
-    measureElement: () => {}
-  })
+  useVirtualizer: ({ count }: { count: number }) => {
+    const start = count === 0 ? 0 : Math.min(WINDOW_START, count - 1)
+    const end = count === 0 ? -1 : Math.min(WINDOW_END, count - 1)
+    return {
+      getTotalSize: () => (count === 0 ? 0 : count * 44 - 8),
+      measurementsCache: Array.from({ length: count }, (_, index) => ({
+        index,
+        start: index * 44,
+        end: index * 44 + 36
+      })),
+      getVirtualItems: () =>
+        Array.from({ length: Math.max(0, end - start + 1) }, (_, offset) => {
+          const index = start + offset
+          return { index, key: `w${index}`, start: index * 44 }
+        }),
+      measureElement: () => {}
+    }
+  }
 }))

 vi.mock('./WorkspaceKanbanCard', () => ({
@@ -102,4 +106,14 @@ describe('WorkspaceKanbanLaneCardList', () => {
    )?.parentElement
    expect(first?.style.transform).toBe(`translateY(${WINDOW_START * 44}px)`)
  })
+
+  it('renders nothing for an empty lane and a single card at index 0', () => {
+    const empty = renderLane(0)
+    expect(empty.querySelectorAll('[data-workspace-board-card-id]')).toHaveLength(0)
+
+    const single = renderLane(1)
+    const card = single.querySelector('[data-workspace-board-card-id]')
+    expect(card?.getAttribute('data-workspace-board-card-id')).toBe('w0')
+    expect(card?.getAttribute('data-workspace-board-card-index')).toBe('0')
+  })
 })
@@ -1,4 +1,4 @@
-import React, { useCallback, useLayoutEffect, useRef } from 'react'
+import React, { useCallback, useLayoutEffect, useMemo, useRef } from 'react'
 import { useVirtualizer } from '@tanstack/react-virtual'
 import type { Repo, WorkspaceStatus, Worktree } from '../../../../shared/types'
 import WorkspaceKanbanCard from './WorkspaceKanbanCard'
@@ -11,6 +11,10 @@ const WORKSPACE_BOARD_CARD_ESTIMATED_HEIGHT = 36
 const WORKSPACE_BOARD_CARD_GAP = 8
 const WORKSPACE_BOARD_CARD_OVERSCAN = 6

+function estimateWorkspaceBoardCardSize(): number {
+  return WORKSPACE_BOARD_CARD_ESTIMATED_HEIGHT
+}
+
 type WorkspaceKanbanLaneCardListProps = {
  items: readonly Worktree[]
  repoMap: Map<string, Repo>
@@ -42,12 +46,13 @@ function WorkspaceKanbanLaneCardList({
  onAssignWorkspaceStatus
 }: WorkspaceKanbanLaneCardListProps): React.JSX.Element {
  const spacerRef = useRef<HTMLDivElement | null>(null)
-  const itemIdsRef = useRef<readonly string[]>([])
-  itemIdsRef.current = items.map((item) => item.id)
+  const itemIds = useMemo(() => items.map((item) => item.id), [items])
+  const itemIdsRef = useRef(itemIds)
+  itemIdsRef.current = itemIds
  const virtualizer = useVirtualizer({
    count: items.length,
    getScrollElement: () => scrollRef.current,
-    estimateSize: () => WORKSPACE_BOARD_CARD_ESTIMATED_HEIGHT,
+    estimateSize: estimateWorkspaceBoardCardSize,
    getItemKey: useCallback((index: number) => items[index]?.id ?? index, [items]),
    overscan: WORKSPACE_BOARD_CARD_OVERSCAN,
    gap: WORKSPACE_BOARD_CARD_GAP,
@@ -1,18 +1,20 @@
 /* eslint-disable max-lines -- Why: marquee selection coordinates pointer capture, lane-scroll refresh, auto-scroll, preview cleanup, and final commit against one drag state. Splitting those phases would make the interaction easier to desynchronize. */
 import React, { useCallback, useEffect, useRef } from 'react'
+import {
+  getAreaSelectionCardRects,
+  type AreaSelectionCardRect
+} from './workspace-kanban-area-selection-card-rects'
 import {
  clearPreviewSelection,
  getAreaSelectionAutoScrollDelta,
  getAreaSelectionCardIds,
-  getAreaSelectionCardRects,
  getAreaSelectionRect,
  getAreaSelectionScrollContainer,
  getAreaSelectionScrollStartContentYByElement,
  isScrollbarPointerDown,
  setOverlayRect,
  shouldIgnoreAreaSelectionStart,
-  updatePreviewSelection,
-  type AreaSelectionCardRect
+  updatePreviewSelection
 } from './workspace-kanban-area-selection-dom'

 type AreaSelectionDragState = {
@@ -110,6 +112,20 @@ export function useWorkspaceKanbanAreaSelection({

    state.started = true

+    // Why: deferred card mount can start a marquee against empty rects; pick up
+    // virtual layout once lanes register without waiting for a scroll event.
+    if (state.cardRects.length === 0) {
+      const board = boardRef.current
+      if (board) {
+        state.boardRect = board.getBoundingClientRect()
+        state.cardRects = getAreaSelectionCardRects(board)
+        state.scrollStartContentYByElement = getAreaSelectionScrollStartContentYByElement(
+          board,
+          state.startY
+        )
+      }
+    }
+
    const viewportRect = getAreaSelectionRect(
      state.startX,
      state.startY,
@@ -153,7 +169,7 @@ export function useWorkspaceKanbanAreaSelection({
      state.additive,
      areaIds
    )
-  }, [overlayRef])
+  }, [boardRef, overlayRef])

  const refreshAreaSelectionMeasurements = useCallback(() => {
    const state = dragRef.current
@@ -11,7 +11,6 @@ import {
 } from './workspace-kanban-card-pointer-drag-dom'
 import {
  createDragPreview,
-  getDraggedCards,
  setDragDocumentStyles,
  setDraggedCardsDragging,
  updateDragPreviewPosition
@@ -33,7 +32,6 @@ type DragState = {
  currentY: number
  worktreeIds: string[]
  sourceCard: HTMLElement
-  draggedCards: HTMLElement[]
  preview: HTMLElement | null
  previewOffsetX: number
  previewOffsetY: number
@@ -117,7 +115,11 @@ export function useWorkspaceKanbanCardPointerDrag({
      if (state.frameId !== null) {
        window.cancelAnimationFrame(state.frameId)
      }
-      setDraggedCardsDragging(state.draggedCards, false)
+      setDraggedCardsDragging({
+        board: boardRef.current,
+        worktreeIds: state.worktreeIds,
+        enabled: false
+      })
      removeCardDropIndicator()
      state.preview?.remove()
      setDragDocumentStyles(false)
@@ -146,13 +148,20 @@ export function useWorkspaceKanbanCardPointerDrag({
    [boardRef, clearDragTarget]
  )

-  const startPointerDrag = useCallback((state: DragState) => {
-    state.started = true
-    isPointerDragActiveRef.current = true
-    setDraggedCardsDragging(state.draggedCards, true)
-    state.preview = createDragPreview(state)
-    setDragDocumentStyles(true)
-  }, [])
+  const startPointerDrag = useCallback(
+    (state: DragState) => {
+      state.started = true
+      isPointerDragActiveRef.current = true
+      setDraggedCardsDragging({
+        board: boardRef.current,
+        worktreeIds: state.worktreeIds,
+        enabled: true
+      })
+      state.preview = createDragPreview(state)
+      setDragDocumentStyles(true)
+    },
+    [boardRef]
+  )

  const updatePointerDragTarget = useCallback(
    (state: DragState) => {
@@ -303,7 +312,6 @@ export function useWorkspaceKanbanCardPointerDrag({
        currentY: event.clientY,
        worktreeIds,
        sourceCard: card,
-        draggedCards: getDraggedCards(board, worktreeIds, card),
        preview: null,
        previewOffsetX: 0,
        previewOffsetY: 0,
@@ -7,10 +7,17 @@ type AreaSelectionCardContentRect = {
  scrollTop: number
 }

+export type AreaSelectionViewportRect = {
+  left: number
+  top: number
+  right: number
+  bottom: number
+}
+
 export type AreaSelectionCardRect = {
  id: string
  element: HTMLElement | null
-  rect: DOMRect
+  rect: AreaSelectionViewportRect
  scrollContainer: HTMLElement | null
  contentRect: AreaSelectionCardContentRect | null
 }
@@ -35,7 +42,12 @@ export function getAreaSelectionCardRects(board: HTMLElement): AreaSelectionCard
      cardRects.set(virtualRect.id, {
        id: virtualRect.id,
        element: null,
-        rect: makeDomRect(virtualRect),
+        rect: {
+          left: virtualRect.left,
+          top: virtualRect.top,
+          right: virtualRect.right,
+          bottom: virtualRect.bottom
+        },
        scrollContainer,
        contentRect: {
          top: virtualRect.contentTop,
@@ -66,7 +78,12 @@ export function getAreaSelectionCardRects(board: HTMLElement): AreaSelectionCard
    cardRects.set(id, {
      id,
      element: card,
-      rect,
+      rect: {
+        left: rect.left,
+        top: rect.top,
+        right: rect.right,
+        bottom: rect.bottom
+      },
      scrollContainer,
      contentRect: metrics
        ? {
@@ -80,17 +97,3 @@ export function getAreaSelectionCardRects(board: HTMLElement): AreaSelectionCard
  }
  return Array.from(cardRects.values())
 }
-
-function makeDomRect(rect: { left: number; top: number; right: number; bottom: number }): DOMRect {
-  return {
-    x: rect.left,
-    y: rect.top,
-    left: rect.left,
-    top: rect.top,
-    right: rect.right,
-    bottom: rect.bottom,
-    width: rect.right - rect.left,
-    height: rect.bottom - rect.top,
-    toJSON: () => ({})
-  } as DOMRect
-}
@@ -3,12 +3,6 @@ import {
  type AreaSelectionCardRect
 } from './workspace-kanban-area-selection-card-rects'

-export {
-  AREA_SELECTION_SCROLL_CONTAINER_SELECTOR,
-  getAreaSelectionCardRects,
-  type AreaSelectionCardRect
-} from './workspace-kanban-area-selection-card-rects'
-
 export type AreaSelectionRect = {
  left: number
  top: number
@@ -212,7 +206,8 @@ export function clearPreviewSelection(
  previewIds: Set<string>
 ): void {
  for (const card of cardRects) {
-    if (card.element && previewIds.has(card.id)) {
+    // Why: virtual remounts replace the element; clear via live connected node when present.
+    if (card.element?.isConnected && previewIds.has(card.id)) {
      card.element.removeAttribute(AREA_SELECTED_ATTR)
    }
  }
@@ -232,19 +227,29 @@ export function updatePreviewSelection(
  }

  for (const card of cardRects) {
-    if (!card.element) {
+    const element = card.element?.isConnected ? card.element : null
+    if (!element) {
+      if (!nextIds.has(card.id)) {
+        previewIds.delete(card.id)
+      }
      continue
    }
    const shouldPreview = nextIds.has(card.id)
-    const isPreviewed = previewIds.has(card.id)
+    // Why: virtualization can remount the same card id; trust the live attr, not previewIds alone.
+    const isPreviewed = element.getAttribute(AREA_SELECTED_ATTR) === 'true'
    if (shouldPreview === isPreviewed) {
+      if (shouldPreview) {
+        previewIds.add(card.id)
+      } else {
+        previewIds.delete(card.id)
+      }
      continue
    }
    if (shouldPreview) {
-      card.element.setAttribute(AREA_SELECTED_ATTR, 'true')
+      element.setAttribute(AREA_SELECTED_ATTR, 'true')
      previewIds.add(card.id)
    } else {
-      card.element.removeAttribute(AREA_SELECTED_ATTR)
+      element.removeAttribute(AREA_SELECTED_ATTR)
      previewIds.delete(card.id)
    }
  }
@@ -1,8 +1,10 @@
+// @vitest-environment happy-dom
 import { describe, expect, it } from 'vitest'
+import type { AreaSelectionCardRect } from './workspace-kanban-area-selection-card-rects'
 import {
  getAreaSelectionAutoScrollDelta,
  getAreaSelectionCardIds,
-  type AreaSelectionCardRect
+  updatePreviewSelection
 } from './workspace-kanban-area-selection-dom'
 import { shouldCommitWorkspaceKanbanAreaSelection } from './use-workspace-kanban-area-selection'

@@ -166,16 +168,52 @@ function makeRect({
  top: number
  right: number
  bottom: number
-}): DOMRect {
-  return {
-    left,
-    top,
-    right,
-    bottom,
-    width: right - left,
-    height: bottom - top,
-    x: left,
-    y: top,
-    toJSON: () => ({})
-  } as DOMRect
+}): AreaSelectionCardRect['rect'] {
+  return { left, top, right, bottom }
 }
+
+describe('workspace kanban area selection preview remount', () => {
+  it('re-applies the preview attribute when a card remounts under the same id', () => {
+    const first = document.createElement('div')
+    first.setAttribute('data-workspace-board-card-id', 'card-a')
+    Object.defineProperty(first, 'isConnected', { value: true })
+    const previewIds = new Set<string>()
+    updatePreviewSelection(
+      [
+        {
+          id: 'card-a',
+          element: first,
+          rect: makeRect({ left: 0, top: 0, right: 100, bottom: 40 }),
+          scrollContainer: null,
+          contentRect: null
+        }
+      ],
+      previewIds,
+      new Set(),
+      false,
+      ['card-a']
+    )
+    expect(first.getAttribute('data-workspace-board-card-area-selected')).toBe('true')
+    expect(previewIds.has('card-a')).toBe(true)
+
+    const remounted = document.createElement('div')
+    remounted.setAttribute('data-workspace-board-card-id', 'card-a')
+    Object.defineProperty(remounted, 'isConnected', { value: true })
+    updatePreviewSelection(
+      [
+        {
+          id: 'card-a',
+          element: remounted,
+          rect: makeRect({ left: 0, top: 0, right: 100, bottom: 40 }),
+          scrollContainer: null,
+          contentRect: null
+        }
+      ],
+      previewIds,
+      new Set(),
+      false,
+      ['card-a']
+    )
+    expect(remounted.getAttribute('data-workspace-board-card-area-selected')).toBe('true')
+  })
+})
@@ -37,12 +37,27 @@ export function getDraggedCards(
  return cards.length > 0 ? cards : [fallbackCard]
 }

-export function setDraggedCardsDragging(cards: readonly HTMLElement[], enabled: boolean): void {
-  for (const card of cards) {
-    if (enabled) {
-      card.setAttribute(POINTER_CARD_DRAGGING_ATTR, 'true')
-    } else {
+export function setDraggedCardsDragging(args: {
+  board: HTMLElement | null
+  worktreeIds: readonly string[]
+  enabled: boolean
+}): void {
+  const { board, worktreeIds, enabled } = args
+  if (!board) {
+    return
+  }
+  // Why: virtual lanes remount cards during scroll; query live nodes by id
+  // instead of holding HTMLElement references across unmounts.
+  if (!enabled) {
+    board.querySelectorAll<HTMLElement>(`[${POINTER_CARD_DRAGGING_ATTR}]`).forEach((card) => {
      card.removeAttribute(POINTER_CARD_DRAGGING_ATTR)
+    })
+    return
+  }
+  const ids = new Set(worktreeIds)
+  for (const card of board.querySelectorAll<HTMLElement>(CARD_SELECTOR)) {
+    if (ids.has(card.dataset.workspaceBoardCardId ?? '')) {
+      card.setAttribute(POINTER_CARD_DRAGGING_ATTR, 'true')
    }
  }
 }
@@ -1,8 +1,6 @@
 import { describe, expect, it } from 'vitest'
-import {
-  getAreaSelectionCardIds,
-  getAreaSelectionCardRects
-} from './workspace-kanban-area-selection-dom'
+import { getAreaSelectionCardRects } from './workspace-kanban-area-selection-card-rects'
+import { getAreaSelectionCardIds } from './workspace-kanban-area-selection-dom'
 import {
  getWorkspaceKanbanVirtualLaneItemRects,
  registerWorkspaceKanbanVirtualLaneLayout,
@@ -131,4 +129,45 @@ describe('workspace kanban virtual lane layout', () => {
    unregisterSecond()
    expect(getWorkspaceKanbanVirtualLaneItemRects(scrollElement)).toBeNull()
  })
+
+  it('places empty-lane and boundary drop indicators from the spacer', () => {
+    const { scrollElement, spacerElement } = createLayoutElements()
+    const unregisterEmpty = registerWorkspaceKanbanVirtualLaneLayout({
+      scrollElement,
+      spacerElement,
+      getItemIds: () => [],
+      getMeasurements: () => []
+    })
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndex(scrollElement, 200)).toBe(0)
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndicatorY(scrollElement, 0)).toBe(114)
+    unregisterEmpty()
+
+    const unregister = registerLayout(scrollElement, spacerElement)
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndex(scrollElement, 90)).toBe(0)
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndicatorY(scrollElement, 0)).toBe(103)
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndicatorY(scrollElement, 1)).toBe(148)
+    unregister()
+  })
+
+  it('returns null when measurements are incomplete or mismatched', () => {
+    const { scrollElement, spacerElement } = createLayoutElements()
+    const unregisterShort = registerWorkspaceKanbanVirtualLaneLayout({
+      scrollElement,
+      spacerElement,
+      getItemIds: () => ['a', 'b'],
+      getMeasurements: () => [{ index: 0, start: 0, end: ITEM_HEIGHT }]
+    })
+    expect(getWorkspaceKanbanVirtualLaneItemRects(scrollElement)).toBeNull()
+    expect(resolveWorkspaceKanbanVirtualLaneDropIndex(scrollElement, 120)).toBeNull()
+    unregisterShort()
+
+    const unregisterBadIndex = registerWorkspaceKanbanVirtualLaneLayout({
+      scrollElement,
+      spacerElement,
+      getItemIds: () => ['a'],
+      getMeasurements: () => [{ index: 3, start: 0, end: ITEM_HEIGHT }]
+    })
+    expect(getWorkspaceKanbanVirtualLaneItemRects(scrollElement)).toBeNull()
+    unregisterBadIndex()
+  })
 })