From 4c65b42ee274cd293efd965363e4b06d07bc9f4e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:03:58 -0700 Subject: [PATCH] fix(sidebar): move project header grab cursor to title surface only (#11435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sidebar): move project header grab cursor to title surface only Prevent grab cursor appearing over action buttons (…, +, chevron) which should show cursor-pointer, not the reorder hand. - Grab cursor scoped to icon + label surface only - Row retains data-repo-header-drag-handle for indent/padding drag targets - Actions excluded via [data-repo-header-actions] selector - Add lockstep test to keep action selectors synchronized * fix(sidebar): share project header action selector across drag contracts Address Greptile feedback: drop the format-sensitive regex lockstep parse and import one shared REPO_HEADER_ACTION_SELECTOR for repo and group headers. --- .../sidebar/ProjectHeaderActions.test.tsx | 4 + .../sidebar/ProjectHeaderActions.tsx | 5 +- .../src/components/sidebar/WorktreeList.tsx | 82 ++++++----- .../sidebar/project-group-header-dom.test.ts | 25 ++++ .../project-group-header-drag-contract.ts | 8 +- .../project-group-header-drag-start.test.ts | 135 ++++++++++++++++++ .../sidebar/project-group-header-drag.test.ts | 54 +++++++ ...ct-header-action-selector-lockstep.test.ts | 25 ++++ .../sidebar/project-header-drag-contract.ts | 5 +- .../sidebar/project-header-drag-start.test.ts | 68 +++++++++ .../sidebar/project-header-drag.test.ts | 13 ++ 11 files changed, 380 insertions(+), 44 deletions(-) create mode 100644 src/renderer/src/components/sidebar/project-group-header-drag.test.ts create mode 100644 src/renderer/src/components/sidebar/project-header-action-selector-lockstep.test.ts diff --git a/src/renderer/src/components/sidebar/ProjectHeaderActions.test.tsx b/src/renderer/src/components/sidebar/ProjectHeaderActions.test.tsx index dc41dfef2..eab571774 100644 --- a/src/renderer/src/components/sidebar/ProjectHeaderActions.test.tsx +++ b/src/renderer/src/components/sidebar/ProjectHeaderActions.test.tsx @@ -35,5 +35,9 @@ describe('ProjectHeaderActions', () => { expect(actions?.className).toContain('group-hover:pointer-events-auto') expect(actions?.className).toContain('has-[:focus-visible]:pointer-events-auto') expect(actions?.className).toContain('has-[button[data-state=open]]:pointer-events-auto') + // Why: title drag surface is cursor-grab; actions must override so hovering + // … / + never shows the reorder hand. + expect(actions?.className).toContain('cursor-pointer') + expect(actions?.className).toContain('self-stretch') }) }) diff --git a/src/renderer/src/components/sidebar/ProjectHeaderActions.tsx b/src/renderer/src/components/sidebar/ProjectHeaderActions.tsx index 681f3cb21..158d0e1c2 100644 --- a/src/renderer/src/components/sidebar/ProjectHeaderActions.tsx +++ b/src/renderer/src/components/sidebar/ProjectHeaderActions.tsx @@ -3,8 +3,11 @@ import { cn } from '@/lib/utils' // Why: desktop hover actions should not permanently reserve project-title width; // touch devices keep them in normal flow because there is no hover reveal. +// Why: title surface uses cursor-grab; force pointer here so … / + / chevron +// (and gaps) never show the reorder hand. self-stretch fills the row height in +// flow so the gutter beside buttons is still an action hit target, not a drag arm. export const PROJECT_HEADER_ACTIONS_CLASS_NAME = cn( - 'flex shrink-0 items-center gap-0.5', + 'flex shrink-0 cursor-pointer items-center gap-0.5 self-stretch', 'can-hover:absolute can-hover:right-1 can-hover:top-1/2 can-hover:z-10 can-hover:-translate-y-1/2', 'can-hover:rounded-md can-hover:bg-worktree-sidebar can-hover:pl-1', 'can-hover:pointer-events-none can-hover:opacity-0 can-hover:transition-opacity', diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index f0db0c6fb..2abe607a1 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -4254,6 +4254,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp ? repoHeaderSectionEndByRepoId.get(projectIdForHeader) : undefined } + // Why: row keeps handle attrs so indent/padding still arms drag; grab + // cursor lives only on the title surface so … / + never inherit it. data-repo-header-drag-handle={isDraggableRepoHeader ? '' : undefined} data-project-group-header-id={projectGroupIdForHeader} data-project-group-header-index={projectGroupHeaderIndex} @@ -4270,10 +4272,10 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp data-workspace-status={headerWorkspaceStatus ?? undefined} data-workspace-pin-drop-target={isPinnedHeader ? '' : undefined} className={cn( + // Why: no row-level grab — only the title surface below shows the hand; + // actions use cursor-pointer so … / + never look reorderable. 'group relative flex h-7 w-full items-center gap-1.5 pr-2 text-left transition-all', - isDraggableRepoHeader || isDraggableProjectGroupHeader - ? 'cursor-grab active:cursor-grabbing' - : 'cursor-pointer', + !(isDraggableRepoHeader || isDraggableProjectGroupHeader) && 'cursor-pointer', highlightedRevealRowKey === row.key && 'rounded-md bg-worktree-sidebar-accent ring-1 ring-worktree-sidebar-ring/50', (isDraggingThis || isDraggingThisProjectGroup) && @@ -4330,39 +4332,48 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }} > - {row.icon ? ( -
- {row.repo ? ( - - ) : ( - - )} -
- ) : null} - -
-
-
- {row.label} + {/* Why: grab cursor on icon+title only. Row still has handle attrs so + indent/padding can arm drag; actions are excluded via data-repo-header-actions. + self-stretch fills h-7 so grab matches the full title column height. */} +
+ {row.icon ? ( +
+ {row.repo ? ( + + ) : ( + + )} +
+ ) : null} + +
+
+
+ {row.label} +
+ +
- -
@@ -4705,6 +4716,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp ) } onKeyDown={stopRepoHeaderKeyboardToggle} + onPointerDown={handleRepoHeaderActionPointerDown} onClick={(event) => { event.preventDefault() event.stopPropagation() diff --git a/src/renderer/src/components/sidebar/project-group-header-dom.test.ts b/src/renderer/src/components/sidebar/project-group-header-dom.test.ts index 266c85c48..5f7d294ec 100644 --- a/src/renderer/src/components/sidebar/project-group-header-dom.test.ts +++ b/src/renderer/src/components/sidebar/project-group-header-dom.test.ts @@ -22,4 +22,29 @@ describe('Project Group header drag DOM source', () => { expect(source).toContain('const updateProjectGroup = useAppStore((s) => s.updateProjectGroup)') expect(source).toContain('void updateProjectGroup(groupId, { tabOrder })') }) + + it('keeps grab cursor on the title surface and dual handle attrs on row + surface', () => { + // Why: lock the cursor/hit-test split so a cleanup does not put grab back on the whole row + // or drop row-level handle attrs that arm drag from indent/padding. + const source = readWorktreeListSource() + const headerBlockStart = source.indexOf('data-repo-header-id={projectIdForHeader}') + const headerBlockEnd = source.indexOf('', headerBlockStart) + expect(headerBlockStart).toBeGreaterThan(-1) + expect(headerBlockEnd).toBeGreaterThan(headerBlockStart) + const headerBlock = source.slice(headerBlockStart, headerBlockEnd) + + // Dual handle placement: row (indent/padding) + title surface. + expect(headerBlock.match(/data-repo-header-drag-handle=/g)?.length).toBe(2) + expect(headerBlock.match(/data-project-group-header-drag-handle=/g)?.length).toBe(2) + // Surface owns grab; outer row only gets cursor-pointer when not draggable. + expect(headerBlock).toContain( + "!(isDraggableRepoHeader || isDraggableProjectGroupHeader) && 'cursor-pointer'" + ) + expect(headerBlock).toContain("'flex min-w-0 flex-1 items-center gap-1.5 self-stretch'") + expect(headerBlock).toContain("'cursor-grab active:cursor-grabbing'") + // Row-level ternary grab must stay gone. + expect(headerBlock).not.toMatch( + /isDraggableRepoHeader \|\| isDraggableProjectGroupHeader\s*\?\s*'cursor-grab/ + ) + }) }) diff --git a/src/renderer/src/components/sidebar/project-group-header-drag-contract.ts b/src/renderer/src/components/sidebar/project-group-header-drag-contract.ts index 4435605b6..bedcefcfc 100644 --- a/src/renderer/src/components/sidebar/project-group-header-drag-contract.ts +++ b/src/renderer/src/components/sidebar/project-group-header-drag-contract.ts @@ -4,6 +4,7 @@ import type { ProjectGroupHeaderDragBucketKey, ProjectGroupHeaderDragRect } from './project-group-header-drop' +import { REPO_HEADER_ACTION_SELECTOR } from './project-header-drag-contract' import type { ProjectGroup } from '../../../../shared/types' export type ProjectGroupDragState = { @@ -50,9 +51,6 @@ export const PROJECT_GROUP_HEADER_DRAG_THRESHOLD_PX = 4 const PROJECT_GROUP_HEADER_DRAG_HANDLE_SELECTOR = '[data-project-group-header-drag-handle]' -const PROJECT_GROUP_HEADER_ACTION_SELECTOR = - '[data-repo-header-action], [data-repo-header-collapse-affordance], button, a, input, textarea, select, [contenteditable=""], [contenteditable="true"]' - export function isProjectGroupHeaderDragHandleTarget( target: EventTarget | null, currentTarget: HTMLElement @@ -76,7 +74,5 @@ export function isProjectGroupHeaderActionTarget( if (!(target instanceof Element) || target === currentTarget) { return false } - return ( - currentTarget.contains(target) && target.closest(PROJECT_GROUP_HEADER_ACTION_SELECTOR) !== null - ) + return currentTarget.contains(target) && target.closest(REPO_HEADER_ACTION_SELECTOR) !== null } diff --git a/src/renderer/src/components/sidebar/project-group-header-drag-start.test.ts b/src/renderer/src/components/sidebar/project-group-header-drag-start.test.ts index 4755cfaf1..ef53dc541 100644 --- a/src/renderer/src/components/sidebar/project-group-header-drag-start.test.ts +++ b/src/renderer/src/components/sidebar/project-group-header-drag-start.test.ts @@ -87,4 +87,139 @@ describe('createProjectGroupHeaderDragSession', () => { expect(session).toBeNull() }) + + it('arms from the group icon svg (SVGElement target)', () => { + const header = document.createElement('div') + header.setAttribute('data-project-group-header-drag-handle', '') + const iconSvg = document.createElementNS('http://www.w3.org/2000/svg', 'svg') + header.append(iconSvg) + const scrollContainer = document.createElement('div') + document.body.append(scrollContainer, header) + + const projectGroupById = new Map([ + ['group-a', group('group-a')], + ['group-b', group('group-b')] + ]) + const sidebarProjectGroupHeaderIdsByBucket = new Map([['root', ['group-a', 'group-b']]]) + + const session = createProjectGroupHeaderDragSession({ + event: { + button: 0, + pointerId: 1, + clientX: 10, + clientY: 20, + target: iconSvg, + currentTarget: header + } as unknown as React.PointerEvent, + groupId: 'group-a', + projectGroupById, + sidebarProjectGroupHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + + expect(session?.groupId).toBe('group-a') + }) + + it('does not arm when pressing an svg icon inside an action button', () => { + // Why: row stays the handle for hit-testing; action targets must be filtered. + const header = document.createElement('div') + header.setAttribute('data-project-group-header-drag-handle', '') + const actionButton = document.createElement('button') + actionButton.setAttribute('data-repo-header-action', '') + const actionIcon = document.createElementNS('http://www.w3.org/2000/svg', 'svg') + actionButton.append(actionIcon) + header.append(actionButton) + const scrollContainer = document.createElement('div') + document.body.append(scrollContainer, header) + + const projectGroupById = new Map([ + ['group-a', group('group-a')], + ['group-b', group('group-b')] + ]) + const sidebarProjectGroupHeaderIdsByBucket = new Map([['root', ['group-a', 'group-b']]]) + + const session = createProjectGroupHeaderDragSession({ + event: { + button: 0, + pointerId: 1, + clientX: 10, + clientY: 20, + target: actionIcon, + currentTarget: header + } as unknown as React.PointerEvent, + groupId: 'group-a', + projectGroupById, + sidebarProjectGroupHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + + expect(session).toBeNull() + }) + + it('does not arm from the actions overlay even if the row is the drag handle', () => { + // Why: row keeps the handle attr for indent/padding; overlay gaps must still be excluded. + const header = document.createElement('div') + header.setAttribute('data-project-group-header-drag-handle', '') + const dragSurface = document.createElement('div') + const label = document.createElement('span') + dragSurface.append(label) + const actions = document.createElement('div') + actions.setAttribute('data-repo-header-actions', '') + header.append(dragSurface, actions) + const scrollContainer = document.createElement('div') + document.body.append(scrollContainer, header) + + const projectGroupById = new Map([ + ['group-a', group('group-a')], + ['group-b', group('group-b')] + ]) + const sidebarProjectGroupHeaderIdsByBucket = new Map([['root', ['group-a', 'group-b']]]) + + const sessionFromActions = createProjectGroupHeaderDragSession({ + event: { + button: 0, + pointerId: 1, + clientX: 10, + clientY: 20, + target: actions, + currentTarget: header + } as unknown as React.PointerEvent, + groupId: 'group-a', + projectGroupById, + sidebarProjectGroupHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + const sessionFromLabel = createProjectGroupHeaderDragSession({ + event: { + button: 0, + pointerId: 2, + clientX: 10, + clientY: 20, + target: label, + currentTarget: header + } as unknown as React.PointerEvent, + groupId: 'group-a', + projectGroupById, + sidebarProjectGroupHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + const sessionFromRowPadding = createProjectGroupHeaderDragSession({ + event: { + button: 0, + pointerId: 3, + clientX: 10, + clientY: 20, + target: header, + currentTarget: header + } as unknown as React.PointerEvent, + groupId: 'group-a', + projectGroupById, + sidebarProjectGroupHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + + expect(sessionFromActions).toBeNull() + expect(sessionFromLabel?.groupId).toBe('group-a') + expect(sessionFromRowPadding?.groupId).toBe('group-a') + }) }) diff --git a/src/renderer/src/components/sidebar/project-group-header-drag.test.ts b/src/renderer/src/components/sidebar/project-group-header-drag.test.ts new file mode 100644 index 000000000..a306662c4 --- /dev/null +++ b/src/renderer/src/components/sidebar/project-group-header-drag.test.ts @@ -0,0 +1,54 @@ +// @vitest-environment happy-dom +import { describe, expect, it } from 'vitest' + +import { isProjectGroupHeaderActionTarget } from './project-group-header-drag' + +function createHeader(markup: string): HTMLElement { + const header = document.createElement('div') + header.setAttribute('data-project-group-header-id', 'group-1') + header.innerHTML = markup + document.body.appendChild(header) + return header +} + +describe('project group header action targets', () => { + it('ignores explicit project action wrappers', () => { + const header = createHeader(` + + + + `) + + expect(isProjectGroupHeaderActionTarget(header.querySelector('#icon'), header)).toBe(true) + }) + + it('ignores the project header actions overlay (including gaps between icons)', () => { + const header = createHeader(` +
+ +
+ `) + + expect( + isProjectGroupHeaderActionTarget(header.querySelector('[data-repo-header-actions]'), header) + ).toBe(true) + expect(isProjectGroupHeaderActionTarget(header.querySelector('#icon'), header)).toBe(true) + }) + + it('ignores the hover collapse affordance', () => { + const header = createHeader(` +
+ +
+ `) + + expect(isProjectGroupHeaderActionTarget(header.querySelector('#chevron'), header)).toBe(true) + }) + + it('does not ignore plain header text or the header itself', () => { + const header = createHeader('Group') + + expect(isProjectGroupHeaderActionTarget(header.querySelector('#label'), header)).toBe(false) + expect(isProjectGroupHeaderActionTarget(header, header)).toBe(false) + }) +}) diff --git a/src/renderer/src/components/sidebar/project-header-action-selector-lockstep.test.ts b/src/renderer/src/components/sidebar/project-header-action-selector-lockstep.test.ts new file mode 100644 index 000000000..cfba96f41 --- /dev/null +++ b/src/renderer/src/components/sidebar/project-header-action-selector-lockstep.test.ts @@ -0,0 +1,25 @@ +// @vitest-environment happy-dom +import { describe, expect, it } from 'vitest' + +import { isProjectGroupHeaderActionTarget } from './project-group-header-drag-contract' +import { + isRepoHeaderActionTarget, + REPO_HEADER_ACTION_SELECTOR +} from './project-header-drag-contract' + +describe('project header action selectors', () => { + it('includes the actions overlay for both repo and group helpers', () => { + // Why: both helpers must share REPO_HEADER_ACTION_SELECTOR so overlay gaps + // never arm drag/toggle on either header kind. + expect(REPO_HEADER_ACTION_SELECTOR).toContain('[data-repo-header-actions]') + + const header = document.createElement('div') + const actions = document.createElement('div') + actions.setAttribute('data-repo-header-actions', '') + header.append(actions) + document.body.append(header) + + expect(isRepoHeaderActionTarget(actions, header)).toBe(true) + expect(isProjectGroupHeaderActionTarget(actions, header)).toBe(true) + }) +}) diff --git a/src/renderer/src/components/sidebar/project-header-drag-contract.ts b/src/renderer/src/components/sidebar/project-header-drag-contract.ts index cc5f49127..9923c8c09 100644 --- a/src/renderer/src/components/sidebar/project-header-drag-contract.ts +++ b/src/renderer/src/components/sidebar/project-header-drag-contract.ts @@ -47,8 +47,9 @@ export const PROJECT_HEADER_DRAG_THRESHOLD_PX = 4 const REPO_HEADER_DRAG_HANDLE_SELECTOR = '[data-repo-header-drag-handle]' -const REPO_HEADER_ACTION_SELECTOR = - '[data-repo-header-action], [data-repo-header-collapse-affordance], button, a, input, textarea, select, [contenteditable=""], [contenteditable="true"]' +// Shared with project-group headers: both reuse ProjectHeaderActions markup. +export const REPO_HEADER_ACTION_SELECTOR = + '[data-repo-header-actions], [data-repo-header-action], [data-repo-header-collapse-affordance], button, a, input, textarea, select, [contenteditable=""], [contenteditable="true"]' export function isProjectHeaderDragHandleTarget( target: EventTarget | null, diff --git a/src/renderer/src/components/sidebar/project-header-drag-start.test.ts b/src/renderer/src/components/sidebar/project-header-drag-start.test.ts index 6ac031cac..6fa80295e 100644 --- a/src/renderer/src/components/sidebar/project-header-drag-start.test.ts +++ b/src/renderer/src/components/sidebar/project-header-drag-start.test.ts @@ -107,6 +107,8 @@ describe('createProjectHeaderDragSession', () => { }) it('does not arm a drag session when pressing an svg icon inside an action button', () => { + // Why: the row is still the drag handle for hit-testing; action targets must + // be filtered even though closest(drag-handle) would match the row. const header = document.createElement('div') header.setAttribute('data-repo-header-drag-handle', '') const actionButton = document.createElement('button') @@ -137,4 +139,70 @@ describe('createProjectHeaderDragSession', () => { expect(session).toBeNull() }) + + it('does not arm a drag session when pressing the actions overlay even if the row is the drag handle', () => { + // Why: the row keeps data-repo-header-drag-handle so indent/padding can arm + // drag; actions must still be excluded via data-repo-header-actions. + const header = document.createElement('div') + header.setAttribute('data-repo-header-drag-handle', '') + const dragSurface = document.createElement('div') + const label = document.createElement('span') + dragSurface.append(label) + const actions = document.createElement('div') + actions.setAttribute('data-repo-header-actions', '') + header.append(dragSurface, actions) + const scrollContainer = document.createElement('div') + document.body.append(scrollContainer, header) + + const repoById = new Map([['repo-a', createRepo('repo-a')]]) + const sidebarRepoHeaderIdsByBucket = new Map([['ungrouped', ['repo-a', 'repo-b']]]) + + const sessionFromActions = createProjectHeaderDragSession({ + event: { + button: 0, + pointerId: 1, + clientX: 10, + clientY: 20, + target: actions, + currentTarget: header + } as unknown as React.PointerEvent, + repoId: 'repo-a', + repoById, + sidebarRepoHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + const sessionFromLabel = createProjectHeaderDragSession({ + event: { + button: 0, + pointerId: 2, + clientX: 10, + clientY: 20, + target: label, + currentTarget: header + } as unknown as React.PointerEvent, + repoId: 'repo-a', + repoById, + sidebarRepoHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + const sessionFromRowPadding = createProjectHeaderDragSession({ + event: { + button: 0, + pointerId: 3, + clientX: 10, + clientY: 20, + target: header, + currentTarget: header + } as unknown as React.PointerEvent, + repoId: 'repo-a', + repoById, + sidebarRepoHeaderIdsByBucket, + getScrollContainer: () => scrollContainer + }) + + expect(sessionFromActions).toBeNull() + expect(sessionFromLabel?.repoId).toBe('repo-a') + // Why: target === currentTarget is treated as the empty row (not an action). + expect(sessionFromRowPadding?.repoId).toBe('repo-a') + }) }) diff --git a/src/renderer/src/components/sidebar/project-header-drag.test.ts b/src/renderer/src/components/sidebar/project-header-drag.test.ts index 2ef27043f..008f5f1dc 100644 --- a/src/renderer/src/components/sidebar/project-header-drag.test.ts +++ b/src/renderer/src/components/sidebar/project-header-drag.test.ts @@ -44,4 +44,17 @@ describe('repo header action targets', () => { expect(isRepoHeaderActionTarget(header.querySelector('#chevron'), header)).toBe(true) }) + + it('ignores the project header actions overlay (including gaps between icons)', () => { + const header = createHeader(` +
+ +
+ `) + + expect( + isRepoHeaderActionTarget(header.querySelector('[data-repo-header-actions]'), header) + ).toBe(true) + expect(isRepoHeaderActionTarget(header.querySelector('#icon'), header)).toBe(true) + }) })