From 7627ccff890226c1a806dadbfae4a484ee0b3a00 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:58:23 -0700 Subject: [PATCH] [P1] fix(sidebar): drop onto the indicated board position in virtualized lanes The sidebar->board drop translation still read the mounted DOM cards, while getCardDropTarget now indexes the lane's virtual layout, so a searched and scrolled lane resolved the drop to the wrong full-lane position. Co-authored-by: Orca --- .../workspace-kanban-sidebar-drop.test.ts | 153 ++++++++++++++++++ .../sidebar/workspace-kanban-sidebar-drop.ts | 71 ++++---- .../workspace-kanban-virtual-lane-layout.ts | 10 ++ 3 files changed, 204 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.test.ts b/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.test.ts index 08f627cc0b4..5a4a3d62b4c 100644 --- a/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.test.ts +++ b/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.test.ts @@ -210,6 +210,91 @@ function appendBoard(): { board: HTMLElement; lane: HTMLElement; firstCard: HTML return { board, lane, firstCard } } +const VIRTUAL_CARD_PITCH = 44 +const VIRTUAL_CARD_HEIGHT = 36 +// Scrolled past the head of the lane, so the mounted window starts mid-list. +const VIRTUAL_SPACER_TOP = -350 + +// A virtualized lane: only `windowRange` of the view is mounted, and the layout +// registration carries the whole view — exactly what the virtualizer publishes. +function appendVirtualBoard(args: { + fullLaneIds: readonly string[] + viewIds: readonly string[] + windowRange: readonly [number, number] + publishFullIds: boolean +}): { lane: HTMLElement; unregister: () => void } { + const board = document.createElement('div') + board.setAttribute('data-workspace-board-selection-surface', '') + setRect(board, { left: 0, top: 0, right: 320, bottom: 240, width: 320, height: 240 }) + + const lane = document.createElement('section') + lane.setAttribute('data-workspace-status-drop-target', '') + lane.dataset.workspaceStatus = 'doing' + setRect(lane, { left: 0, top: 0, right: 200, bottom: 220, width: 200, height: 220 }) + if (args.publishFullIds) { + lane.dataset.workspaceLaneFullIds = serializeWorkspaceLaneFullIds([...args.fullLaneIds]) ?? '' + } + + const laneScroll = document.createElement('div') + laneScroll.setAttribute('data-workspace-board-lane-scroll', '') + setRect(laneScroll, { left: 0, top: 0, right: 200, bottom: 220, width: 200, height: 220 }) + + const totalSize = args.viewIds.length * VIRTUAL_CARD_PITCH + const spacer = document.createElement('div') + setRect(spacer, { + left: 8, + top: VIRTUAL_SPACER_TOP, + right: 192, + bottom: VIRTUAL_SPACER_TOP + totalSize, + width: 184, + height: totalSize + }) + + for (let index = args.windowRange[0]; index <= args.windowRange[1]; index++) { + const worktreeId = args.viewIds[index]! + const card = document.createElement('div') + card.setAttribute('data-workspace-board-card-id', worktreeId) + card.dataset.workspaceBoardCardId = worktreeId + card.dataset.workspaceBoardCardIndex = String(index) + const top = VIRTUAL_SPACER_TOP + index * VIRTUAL_CARD_PITCH + setRect(card, { + left: 8, + top, + right: 192, + bottom: top + VIRTUAL_CARD_HEIGHT, + width: 184, + height: VIRTUAL_CARD_HEIGHT + }) + setVisible(card) + spacer.append(card) + } + + laneScroll.append(spacer) + lane.append(laneScroll) + board.append(lane) + document.body.append(board) + setElementFromPoint(lane) + + const unregister = registerWorkspaceKanbanVirtualLaneLayout({ + scrollElement: laneScroll, + spacerElement: spacer, + getItemIds: () => args.viewIds, + getMeasurements: () => + args.viewIds.map((_, index) => ({ + index, + start: index * VIRTUAL_CARD_PITCH, + end: index * VIRTUAL_CARD_PITCH + VIRTUAL_CARD_HEIGHT + })) + }) + return { lane, unregister } +} + +// 40-member lane under a search that matches every other card. +function searchedVirtualLaneIds(): { fullLaneIds: string[]; viewIds: string[] } { + const fullLaneIds = Array.from({ length: 40 }, (_, index) => `doing-${index}`) + return { fullLaneIds, viewIds: fullLaneIds.filter((_, index) => index % 2 === 0) } +} + function worktree(args: { id: string workspaceStatus: string @@ -356,6 +441,74 @@ describe('workspace kanban sidebar drop DOM bridge', () => { expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 2)).toBe(4) }) + it('lands a virtualized searched lane drop where the indicator pointed', () => { + const { fullLaneIds, viewIds } = searchedVirtualLaneIds() + const { unregister } = appendVirtualBoard({ + fullLaneIds, + viewIds, + windowRange: [8, 17], + publishFullIds: true + }) + + const target = getWorkspaceKanbanSidebarDropTarget(24, 210) + // The indicator sits between the mounted cards for view items 12 and 13, + // whose full-lane neighbours are doing-24 and doing-26. + expect(target).toMatchObject({ status: 'doing', dropIndex: 13, dropIndicatorY: 218 }) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', target.dropIndex)).toBe(26) + + unregister() + }) + + it('translates virtualized searched lane drops above and below the mounted window', () => { + const { fullLaneIds, viewIds } = searchedVirtualLaneIds() + const { unregister } = appendVirtualBoard({ + fullLaneIds, + viewIds, + windowRange: [8, 17], + publishFullIds: true + }) + + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 0)).toBe(0) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 3)).toBe(6) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 19)).toBe(38) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 20)).toBe(39) + + unregister() + }) + + it('passes a virtualized unfiltered lane drop index through untranslated', () => { + const { fullLaneIds } = searchedVirtualLaneIds() + const { unregister } = appendVirtualBoard({ + fullLaneIds, + viewIds: fullLaneIds, + windowRange: [8, 17], + publishFullIds: false + }) + + expect(getWorkspaceKanbanSidebarDropGroups()).toEqual([ + { key: 'doing', worktreeIds: fullLaneIds } + ]) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 13)).toBe(13) + + unregister() + }) + + it('still counts unlaid-out cards as lane members without a virtual layout', () => { + const { lane } = appendBoard() + const secondCard = lane.querySelectorAll('[data-workspace-board-card-id]')[1]! + secondCard.remove() + const hiddenCard = document.createElement('div') + hiddenCard.setAttribute('data-workspace-board-card-id', 'doing-hidden') + hiddenCard.dataset.workspaceBoardCardId = 'doing-hidden' + lane.append(hiddenCard, secondCard) + setElementFromPoint(lane) + + expect(getWorkspaceKanbanSidebarDropGroups()).toEqual([ + { key: 'doing', worktreeIds: ['doing-a', 'doing-hidden', 'doing-b'] } + ]) + expect(resolveWorkspaceKanbanSidebarFullLaneDropIndex('doing', 1)).toBe(2) + }) + it('passes the drop index through for a lane it cannot find', () => { appendBoard() diff --git a/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.ts b/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.ts index 363038bd529..8475d91cdd8 100644 --- a/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.ts +++ b/src/renderer/src/components/sidebar/workspace-kanban-sidebar-drop.ts @@ -23,9 +23,11 @@ import { updateCardDropIndicator, type WorkspaceKanbanCardDropTarget } from './workspace-kanban-card-pointer-drag-dom' +import { getWorkspaceKanbanVirtualLaneItemIds } from './workspace-kanban-virtual-lane-layout' const BOARD_SELECTOR = '[data-workspace-board-selection-surface]' const BOARD_SHEET_SELECTOR = '[data-workspace-board-sheet]' +const LANE_SCROLL_SELECTOR = '[data-workspace-board-lane-scroll]' const EXTERNAL_DRAG_TARGET_ATTR = 'data-workspace-board-external-drag-target' let externalDragTargetElement: HTMLElement | null = null @@ -53,24 +55,33 @@ function getLaneCardIds(lane: HTMLElement): HTMLElement[] { return Array.from(lane.querySelectorAll(CARD_SELECTOR)) } -// Mirrors getCardDropTarget's card scan so both sides share one index space. -// The offsetParent read forces layout, so callers pass the card list they -// already collected rather than re-querying. -function toRenderedCardIds(cards: readonly HTMLElement[]): string[] { - return cards - .filter((card) => card.offsetParent !== null) - .flatMap((card) => card.dataset.workspaceBoardCardId ?? []) -} - -// Why: board search hides non-matching cards, so the rendered card scan is a -// filtered lane. Lanes publish their full membership for exactly this reader. -// The fallback is the unfiltered card list — a lane member the browser is not -// laying out is still a member for manual-order purposes. -function toLaneFullWorktreeIds(lane: HTMLElement, cards: readonly HTMLElement[]): string[] { - return ( - parseWorkspaceLaneFullIds(lane.dataset.workspaceLaneFullIds) ?? - cards.flatMap((card) => card.dataset.workspaceBoardCardId ?? []) - ) +/** + * `viewIds` is the lane in the index space `getCardDropTarget` reports, and + * `fullLaneIds` is the lane's whole membership. Board search hides non-matching + * cards, so lanes publish their full membership for exactly this reader. + * + * Why the virtual layout first: lanes virtualize, so the mounted cards are a + * window rather than the lane view, and `getCardDropTarget` indexes the same + * virtual layout. The DOM scan is only the fallback for a lane the virtualizer + * never claimed — there the unfiltered card list is the full lane, since a card + * the browser is not laying out is still a member for manual-order purposes. + */ +function toLaneDropIds(lane: HTMLElement): { fullLaneIds: string[]; viewIds: string[] } { + const publishedFullLaneIds = parseWorkspaceLaneFullIds(lane.dataset.workspaceLaneFullIds) + const laneScroll = lane.querySelector(LANE_SCROLL_SELECTOR) + const virtualItemIds = laneScroll ? getWorkspaceKanbanVirtualLaneItemIds(laneScroll) : null + if (virtualItemIds) { + const viewIds = [...virtualItemIds] + return { fullLaneIds: publishedFullLaneIds ?? viewIds, viewIds } + } + const cards = getLaneCardIds(lane) + return { + fullLaneIds: + publishedFullLaneIds ?? cards.flatMap((card) => card.dataset.workspaceBoardCardId ?? []), + viewIds: cards + .filter((card) => card.offsetParent !== null) + .flatMap((card) => card.dataset.workspaceBoardCardId ?? []) + } } function getStatusDropTargetElement( @@ -128,7 +139,7 @@ export function getWorkspaceKanbanSidebarDropGroups(): WorktreeDragGroup[] { if (!status) { return [] } - return [{ key: status, worktreeIds: toLaneFullWorktreeIds(lane, getLaneCardIds(lane)) }] + return [{ key: status, worktreeIds: toLaneDropIds(lane).fullLaneIds }] }) } @@ -144,26 +155,26 @@ export function getWorkspaceKanbanSidebarDropTarget( } /** - * Translates a tracked drop index — which counts *rendered* cards, matching the - * drop indicator — onto the full lane that `getWorkspaceKanbanSidebarDropGroups` - * reports. Call this once, at the commit boundary: a tracked target can be - * committed after the pointer has left the lane, so translating earlier would - * miss that path. + * Translates a tracked drop index — which counts the lane's *view* items, + * matching the drop indicator — onto the full lane that + * `getWorkspaceKanbanSidebarDropGroups` reports. Call this once, at the commit + * boundary: a tracked target can be committed after the pointer has left the + * lane, so translating earlier would miss that path. */ export function resolveWorkspaceKanbanSidebarFullLaneDropIndex( status: WorkspaceStatus, - renderedDropIndex: number + viewDropIndex: number ): number { const board = getWorkspaceKanbanBoardElement() const lane = board ? getStatusDropTargetElement(board, status) : null if (!lane) { - return renderedDropIndex + return viewDropIndex } - const cards = getLaneCardIds(lane) + const { fullLaneIds, viewIds } = toLaneDropIds(lane) return resolveFullLaneDropIndex({ - fullLaneIds: toLaneFullWorktreeIds(lane, cards), - renderedIds: toRenderedCardIds(cards), - filteredDropIndex: renderedDropIndex + fullLaneIds, + renderedIds: viewIds, + filteredDropIndex: viewDropIndex }) } diff --git a/src/renderer/src/components/sidebar/workspace-kanban-virtual-lane-layout.ts b/src/renderer/src/components/sidebar/workspace-kanban-virtual-lane-layout.ts index e142b3617d6..4d7b2897fcd 100644 --- a/src/renderer/src/components/sidebar/workspace-kanban-virtual-lane-layout.ts +++ b/src/renderer/src/components/sidebar/workspace-kanban-virtual-lane-layout.ts @@ -44,6 +44,16 @@ export function registerWorkspaceKanbanVirtualLaneLayout(args: { } } +/** + * The lane's item ids in the index space `resolveWorkspaceKanbanVirtualLaneDropIndex` + * reports — every item in the lane view, not just the mounted virtual window. + */ +export function getWorkspaceKanbanVirtualLaneItemIds( + scrollElement: HTMLElement +): readonly string[] | null { + return virtualLaneLayouts.get(scrollElement)?.getItemIds() ?? null +} + export function getWorkspaceKanbanVirtualLaneItemRects( scrollElement: HTMLElement ): WorkspaceKanbanVirtualLaneItemRect[] | null {