diff --git a/src/renderer/src/components/sidebar/WorkspaceKanbanStatusLane.test.tsx b/src/renderer/src/components/sidebar/WorkspaceKanbanStatusLane.test.tsx index 989f7eb991e..523e59c6974 100644 --- a/src/renderer/src/components/sidebar/WorkspaceKanbanStatusLane.test.tsx +++ b/src/renderer/src/components/sidebar/WorkspaceKanbanStatusLane.test.tsx @@ -4,6 +4,7 @@ import React, { act } from 'react' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Repo, Worktree } from '../../../../shared/types' +import { serializeWorkspaceLaneFullIds } from './workspace-kanban-filtered-drop-index' import WorkspaceKanbanStatusLane from './WorkspaceKanbanStatusLane' ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true @@ -127,7 +128,7 @@ describe('WorkspaceKanbanStatusLane', () => { fullWorktreeIds: ['a', 'b', 'c'] }) - expect(lane().dataset.workspaceLaneFullIds).toBe('a\nb\nc') + expect(lane().dataset.workspaceLaneFullIds).toBe(serializeWorkspaceLaneFullIds(['a', 'b', 'c'])) expect(container.querySelectorAll('[data-workspace-board-card-id]')).toHaveLength(1) }) diff --git a/src/renderer/src/components/sidebar/use-workspace-kanban-selection.test.tsx b/src/renderer/src/components/sidebar/use-workspace-kanban-selection.test.tsx index c885df7888f..9098a35b8bf 100644 --- a/src/renderer/src/components/sidebar/use-workspace-kanban-selection.test.tsx +++ b/src/renderer/src/components/sidebar/use-workspace-kanban-selection.test.tsx @@ -53,6 +53,19 @@ function click(worktreeId: string, shiftKey = false): void { }) } +function toggleClick(worktreeId: string): void { + act(() => { + selection.updateSelectionForGesture( + { + metaKey: navigator.userAgent.includes('Mac'), + ctrlKey: !navigator.userAgent.includes('Mac'), + shiftKey: false + } as React.MouseEvent, + worktreeId + ) + }) +} + function selectedIds(): string[] { return [...selection.selectedWorktreeIds].sort() } @@ -102,6 +115,47 @@ describe('useWorkspaceKanbanSelection', () => { expect(selectedIds()).toEqual(['alpha', 'beta']) }) + it('extends the range from a visible card when a search hides the anchor', () => { + // Anchor lands on delta, then a query hides only delta. + renderSelection() + click(alpha.id) + toggleClick(beta.id) + toggleClick(delta.id) + expect(selectedIds()).toEqual(['alpha', 'beta', 'delta']) + + renderSelection([alpha, beta, gamma]) + click(gamma.id, true) + + // Without a rendered anchor this collapsed to just gamma, dropping the + // still-visible alpha and beta along with the hidden delta. + expect(selectedIds()).toEqual(['alpha', 'beta', 'delta', 'gamma']) + }) + + it('leaves a hidden selection intact when a range replaces the visible one', () => { + // Anchor stays on the rendered alpha, so this isolates the range replace + // from the re-anchoring above. A plain click still clears everything. + renderSelection() + click(delta.id) + toggleClick(alpha.id) + expect(selectedIds()).toEqual(['alpha', 'delta']) + + renderSelection([alpha, beta, gamma]) + click(gamma.id, true) + + expect(selectedIds()).toEqual(['alpha', 'beta', 'delta', 'gamma']) + }) + + it('lets a plain click clear a selection the search is hiding', () => { + renderSelection() + click(alpha.id) + toggleClick(delta.id) + + renderSelection([alpha, beta, gamma]) + click(beta.id) + + expect(selectedIds()).toEqual(['beta']) + }) + it('still prunes ids that leave the board entirely', () => { renderSelection() click(alpha.id) diff --git a/src/renderer/src/components/sidebar/use-workspace-kanban-selection.ts b/src/renderer/src/components/sidebar/use-workspace-kanban-selection.ts index 4db4f132a4c..7cb004bd39d 100644 --- a/src/renderer/src/components/sidebar/use-workspace-kanban-selection.ts +++ b/src/renderer/src/components/sidebar/use-workspace-kanban-selection.ts @@ -8,6 +8,33 @@ import { updateWorktreeSelection } from './worktree-multi-selection' +/** Returns the first still-rendered selected id, or `null` if the anchor is fine. */ +function resolveRenderedAnchorId( + renderedWorktreeIds: readonly string[], + selectedWorktreeIds: ReadonlySet, + anchorId: string +): string | null { + if (renderedWorktreeIds.includes(anchorId)) { + return null + } + return renderedWorktreeIds.find((id) => selectedWorktreeIds.has(id)) ?? null +} + +function withHiddenSelections( + nextSelectedIds: ReadonlySet, + previousSelectedIds: ReadonlySet, + renderedWorktreeIds: readonly string[] +): Set { + const rendered = new Set(renderedWorktreeIds) + const merged = new Set(nextSelectedIds) + for (const id of previousSelectedIds) { + if (!rendered.has(id)) { + merged.add(id) + } + } + return merged +} + // Why: board search hides cards without dropping them from the board, so range // and area gestures index the rendered subset while pruning still spans the // whole board — otherwise a query would silently discard hidden selections. @@ -53,14 +80,30 @@ export function useWorkspaceKanbanSelection( const updateSelectionForGesture = useCallback( (event: React.MouseEvent, worktreeId: string): boolean => { const intent = getWorktreeSelectionIntent(event, navigator.userAgent.includes('Mac')) + // Why: a search can hide the anchor while leaving the rest of the + // selection on screen. updateWorktreeSelection reads an anchor missing + // from visibleIds as "no anchor" and collapses the range to the click, + // so re-anchor onto the first still-rendered selected card instead. + const anchorId = + intent === 'range' && selectionAnchorId !== null + ? (resolveRenderedAnchorId(renderedWorktreeIds, selectedWorktreeIds, selectionAnchorId) ?? + selectionAnchorId) + : selectionAnchorId const result = updateWorktreeSelection({ visibleIds: renderedWorktreeIds, previousSelectedIds: selectedWorktreeIds, - previousAnchorId: selectionAnchorId, + previousAnchorId: anchorId, targetId: worktreeId, intent }) - setSelectedWorktreeIds(result.selectedIds) + // Why: a range replaces the selection with what it spans, which spans only + // rendered cards. Selections the query is hiding are not part of the + // gesture and must survive it. + setSelectedWorktreeIds( + intent === 'range' + ? withHiddenSelections(result.selectedIds, selectedWorktreeIds, renderedWorktreeIds) + : result.selectedIds + ) setSelectionAnchorId(result.anchorId) return intent !== 'replace' }, diff --git a/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.test.ts b/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.test.ts index 4c68f93aad4..63987713b7b 100644 --- a/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.test.ts +++ b/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.test.ts @@ -50,23 +50,31 @@ describe('resolveFullLaneDropIndex', () => { ).toBe(4) }) - it('handles a lane filtered to zero cards', () => { + it('appends into a lane whose cards are all filtered away', () => { + // Why: an empty rendered lane reports drop index 0 for every pointer + // position, so honouring it would always prepend. The document-drop path + // appends for the same gesture, and these must not disagree. expect( resolveFullLaneDropIndex({ fullLaneIds: FULL, renderedIds: [], filteredDropIndex: 0 }) - ).toBe(0) + ).toBe(FULL.length) expect( resolveFullLaneDropIndex({ fullLaneIds: FULL, renderedIds: [], filteredDropIndex: 3 }) ).toBe(FULL.length) + expect( + resolveFullLaneDropIndex({ fullLaneIds: [], renderedIds: [], filteredDropIndex: 0 }) + ).toBe(0) }) - it('falls back to the lane end when a rendered id is missing from the full lane', () => { + it('falls back toward the end of the lane the branch was aiming at', () => { + // A head drop resolves to the head, not the tail — the opposite fallback + // would land a card at the bottom of a lane the user dropped it on top of. expect( resolveFullLaneDropIndex({ fullLaneIds: FULL, renderedIds: ['stale', 'e'], filteredDropIndex: 0 }) - ).toBe(FULL.length) + ).toBe(0) expect( resolveFullLaneDropIndex({ fullLaneIds: FULL, @@ -74,6 +82,13 @@ describe('resolveFullLaneDropIndex', () => { filteredDropIndex: 2 }) ).toBe(FULL.length) + expect( + resolveFullLaneDropIndex({ + fullLaneIds: FULL, + renderedIds: ['b', 'stale', 'g'], + filteredDropIndex: 1 + }) + ).toBe(FULL.length) }) it('still translates when a stale lane has the same length but different members', () => { @@ -121,9 +136,13 @@ describe('workspace lane full-id channel', () => { expect(serializeWorkspaceLaneFullIds([])).toBe('') }) - it('refuses to publish a lane whose ids would split on the delimiter', () => { - // Why: a newline is legal in a POSIX path. Publishing would invent phantom - // lane members; declining makes the reader fall back to scanning cards. - expect(serializeWorkspaceLaneFullIds(['repo-a::/Users/dev/we\nird', 'repo-a::/ok'])).toBeNull() + it('survives ids holding every character a path can legally contain', () => { + // Why: a POSIX path may hold any byte but NUL and '/', so a newline, comma + // or colon delimiter would split one id into phantom lane members. Dropping + // the channel is not an escape hatch either — under a query the reader + // would fall back to the DOM and see only the matched cards. + const ids = ['repo-a::/Users/dev/we\nird, one: two', 'repo-b::C:\\src\\atlas'] + + expect(parseWorkspaceLaneFullIds(serializeWorkspaceLaneFullIds(ids) ?? undefined)).toEqual(ids) }) }) diff --git a/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.ts b/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.ts index 742efc2bcc9..6321e9b6231 100644 --- a/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.ts +++ b/src/renderer/src/components/sidebar/workspace-kanban-filtered-drop-index.ts @@ -1,12 +1,15 @@ -// Why: worktree ids embed repo paths, so commas and colons are unusable as a -// separator in the `data-workspace-lane-full-ids` channel; newlines are not. -export const WORKSPACE_LANE_FULL_IDS_DELIMITER = '\n' +// Why: worktree ids embed repo paths, so commas, colons and newlines are all +// unusable as a separator in the `data-workspace-lane-full-ids` channel — a +// POSIX path may contain any byte but NUL and '/', and a Windows path carries +// a drive colon. NUL is the one character no path can hold, so it cannot split +// an id into phantom lane members. +export const WORKSPACE_LANE_FULL_IDS_DELIMITER = '\0' /** - * Returns `null` when the lane cannot be represented on this channel, so the - * caller omits the attribute and the reader falls back to scanning cards. - * Why: a newline is legal in a POSIX path, and joining over one would silently - * split a single id into two phantom lane members. + * Returns `null` when the lane cannot be represented on this channel. Defence + * only: no real worktree id can contain the NUL delimiter. Dropping the channel + * is the wrong fallback under an active query — the reader would then scan the + * DOM and see only the matched cards — so this must stay unreachable. */ export function serializeWorkspaceLaneFullIds(worktreeIds: readonly string[]): string | null { if (worktreeIds.some((worktreeId) => worktreeId.includes(WORKSPACE_LANE_FULL_IDS_DELIMITER))) { @@ -43,18 +46,23 @@ export function resolveFullLaneDropIndex(args: { if (isSameLane(fullLaneIds, renderedIds)) { return filteredDropIndex } + // Why: a lane filtered down to nothing reports drop index 0 for every pointer + // position, so honouring it would silently prepend. Append instead, matching + // dropWorktreesAtEndOfStatus for the same gesture on the document-drop path. if (renderedIds.length === 0) { - return filteredDropIndex <= 0 ? 0 : fullLaneIds.length + return fullLaneIds.length } if (filteredDropIndex <= 0) { - return indexInFullLane(fullLaneIds, renderedIds[0]!) + // Why: the head branch means "above the first match", so an unresolvable id + // falls back to the lane head. Using the tail would invert the gesture. + return indexInFullLane(fullLaneIds, renderedIds[0]!, 0) } if (filteredDropIndex >= renderedIds.length) { - const lastIndex = indexInFullLane(fullLaneIds, renderedIds.at(-1)!) + const lastIndex = indexInFullLane(fullLaneIds, renderedIds.at(-1)!, fullLaneIds.length - 1) return Math.min(fullLaneIds.length, lastIndex + 1) } - return indexInFullLane(fullLaneIds, renderedIds[filteredDropIndex]!) + return indexInFullLane(fullLaneIds, renderedIds[filteredDropIndex]!, fullLaneIds.length) } function isSameLane(fullLaneIds: readonly string[], renderedIds: readonly string[]): boolean { @@ -64,9 +72,14 @@ function isSameLane(fullLaneIds: readonly string[], renderedIds: readonly string ) } -// Why: a rendered id missing from the full lane is a stale-DOM race. Returning -// -1 would clamp to 0 downstream and teleport the card to the top of the lane. -function indexInFullLane(fullLaneIds: readonly string[], worktreeId: string): number { +// Why: a rendered id missing from the full lane is a stale-DOM race, so the +// caller supplies the end of the lane its branch was aiming at — a raw -1 would +// clamp to 0 downstream and teleport a tail drop to the top. +function indexInFullLane( + fullLaneIds: readonly string[], + worktreeId: string, + fallbackIndex: number +): number { const index = fullLaneIds.indexOf(worktreeId) - return index === -1 ? fullLaneIds.length : index + return index === -1 ? fallbackIndex : index } 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 67fdd0f67e3..3feefdd62da 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 @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { WorkspaceStatusDefinition, Worktree } from '../../../../shared/types' +import { serializeWorkspaceLaneFullIds } from './workspace-kanban-filtered-drop-index' import { buildWorkspaceKanbanSidebarDropUpdates, clearWorkspaceKanbanSidebarDropTargetVisual, @@ -250,7 +251,8 @@ describe('workspace kanban sidebar drop DOM bridge', () => { it('prefers the published full lane membership over the rendered card scan', () => { const { lane } = appendBoard() - lane.dataset.workspaceLaneFullIds = ['doing-x', 'doing-a', 'doing-y', 'doing-b'].join('\n') + lane.dataset.workspaceLaneFullIds = + serializeWorkspaceLaneFullIds(['doing-x', 'doing-a', 'doing-y', 'doing-b']) ?? '' setElementFromPoint(lane) expect(getWorkspaceKanbanSidebarDropGroups()).toEqual([ @@ -260,7 +262,8 @@ describe('workspace kanban sidebar drop DOM bridge', () => { it('keeps the tracked drop target in the rendered index space of the indicator', () => { const { lane } = appendBoard() - lane.dataset.workspaceLaneFullIds = ['doing-x', 'doing-a', 'doing-y', 'doing-b'].join('\n') + lane.dataset.workspaceLaneFullIds = + serializeWorkspaceLaneFullIds(['doing-x', 'doing-a', 'doing-y', 'doing-b']) ?? '' setElementFromPoint(lane) expect(getWorkspaceKanbanSidebarDropTarget(24, 60)).toMatchObject({ @@ -271,7 +274,8 @@ describe('workspace kanban sidebar drop DOM bridge', () => { it('translates a rendered drop index onto the full lane at the commit boundary', () => { const { lane } = appendBoard() - lane.dataset.workspaceLaneFullIds = ['doing-x', 'doing-a', 'doing-y', 'doing-b'].join('\n') + lane.dataset.workspaceLaneFullIds = + serializeWorkspaceLaneFullIds(['doing-x', 'doing-a', 'doing-y', 'doing-b']) ?? '' setElementFromPoint(lane) // Rendered index 1 means "before doing-b", which is index 3 in the full lane.