From e958f193545abc90f5781e59ccdc02297d7aaf38 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 28 Jul 2026 21:07:50 -0700 Subject: [PATCH] fix(kanban-search): stop a hidden anchor from collapsing a shift-click A query can hide the selection anchor while leaving the rest of the selection on screen. updateWorktreeSelection reads an anchor missing from visibleIds as "no anchor" and replaces the selection with the clicked card, so shift-clicking dropped the still-visible cards too. Re-anchor onto the first still-rendered selected card, and carry hidden selections through a range so the query cannot silently discard them. A plain click still clears everything. Also, in the drop-index translation: - a lane filtered down to nothing now appends rather than always prepending (an empty rendered lane reports index 0 for every pointer position, so the old branch could only prepend, disagreeing with the document-drop path) - an unresolvable rendered id falls back toward the end of the lane its branch was aiming at, instead of sending every head drop to the bottom - the full-id channel uses NUL, the one character no path can contain, so serialization can no longer be defeated by a newline in a repo path. Dropping the channel was the wrong fallback: under a query the reader would scan the DOM and see only the matched cards. Tests now build the channel through its own serializer rather than hardcoding the delimiter. Co-authored-by: Orca --- .../WorkspaceKanbanStatusLane.test.tsx | 3 +- .../use-workspace-kanban-selection.test.tsx | 54 +++++++++++++++++++ .../sidebar/use-workspace-kanban-selection.ts | 47 +++++++++++++++- ...rkspace-kanban-filtered-drop-index.test.ts | 35 +++++++++--- .../workspace-kanban-filtered-drop-index.ts | 43 +++++++++------ .../workspace-kanban-sidebar-drop.test.ts | 10 ++-- 6 files changed, 163 insertions(+), 29 deletions(-) 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.