diff --git a/docs/new-worktree-sidebar-reveal.md b/docs/new-worktree-sidebar-reveal.md deleted file mode 100644 index 75abb7bd6..000000000 --- a/docs/new-worktree-sidebar-reveal.md +++ /dev/null @@ -1,79 +0,0 @@ -# New Worktree Sidebar Reveal - -## Problem - -Issue https://github.com/stablyai/orca-internal/issues/350 asks that newly created worktrees jump into view in the left sidebar list with no scroll animation. - -Current behavior: - -- `activateAndRevealWorktree(...)` always calls `state.revealWorktreeInSidebar(worktreeId)` with no options. -- `revealWorktreeInSidebar` defaults `behavior` to `'smooth'` (`ui.ts`). -- `WorktreeList` forwards that behavior to `virtualizer.scrollToIndex(..., { behavior })`. - -Result: off-screen targets animate by default, including freshly created worktrees. - -## Root Cause - -`activateAndRevealWorktree` conflates two intents: - -1. activate existing worktree navigation (smooth reveal is fine); -2. activate a just-created worktree (must jump immediately). - -Created-worktree callers cannot currently express reveal intent, so they inherit `'smooth'`. - -## Scope and Non-goals - -- Add an opt-in reveal behavior at activation call sites. -- Apply `'auto'` only where the worktree is newly created/added in that flow. -- Preserve existing behavior for normal worktree navigation (clicks, keyboard, history nav, palette selection of existing worktrees, port/status-driven activation) unless a caller opts in. -- Do not change direct raw reveal paths in IPC handlers that intentionally call `store.revealWorktreeInSidebar(...)` outside `activateAndRevealWorktree` (terminal/editor/mobile focus paths remain smooth). -- Do not change sorting/grouping/filter UI, list virtualization strategy, or sidebar styling. - -## Design - -1. Extend `activateAndRevealWorktree` options: - - `sidebarRevealBehavior?: PendingSidebarWorktreeReveal['behavior']`. -2. In `activateAndRevealWorktree`, call: - - `state.revealWorktreeInSidebar(worktreeId, { behavior: opts.sidebarRevealBehavior })` when provided; - - otherwise keep `state.revealWorktreeInSidebar(worktreeId)` so default behavior stays unchanged. -3. Pass `sidebarRevealBehavior: 'auto'` only from created/added-worktree flows: - - `useComposerState` full-create path; - - `useComposerState` quick-create path; - - `useIpcEvents` `onActivateWorktree` only when the event corresponds to a newly created worktree; - - `launch-work-item-direct`; - - folder add/create flows that activate a newly-added synthetic folder worktree (`AddRepoCreateStep` folder branch, `NonGitFolderDialog`, and `repos` slice `addNonGitFolder` path). -4. Keep existing navigation activations smooth, including: - - `AddRepoDialog` / `ProjectAddedDialog` “open primary worktree” actions (these can target pre-existing worktrees, not guaranteed newly created); - - all existing `activateAndRevealWorktree(...)` callers that do not opt in. -5. Keep `WorktreeList` reveal effect unchanged; it already honors `pendingRevealWorktree.behavior`. - -## Correctness Notes and Edge Cases - -- Repo-filter clearing remains unchanged: `activateAndRevealWorktree` only clears `filterRepoIds` when target repo is excluded. -- Other visibility constraints are not auto-cleared. If the target exists but is hidden by other sidebar state, `resolvePendingSidebarReveal(...)` keeps the reveal pending. -- The reveal effect uncollapses lineage/group containers before scroll; behavior changes only animation mode, not visibility resolution. -- Pending reveal is a single store slot (`pendingRevealWorktree`). Concurrent reveal requests are last-writer-wins; this change should not alter that behavior. -- If activation cannot resolve the worktree (`getKnownWorktreeById` miss), behavior remains unchanged (`false`, no reveal queued). -- `ui:activateWorktree` is an overloaded IPC used by both creation and non-creation activation paths. The renderer must choose `'auto'` only for create cases (for example, worktree absent before fetch and present after fetch), and keep default smooth reveal for existing-worktree activations. -- Multi-window consistency remains per renderer window store; each window applies its own reveal behavior locally. -- This change is renderer-only; it does not add main-process coordination and does not make reveal ordering transactional across concurrent async creators. - -## Tests - -Add/adjust focused tests in `worktree-activation` coverage: - -- explicit `sidebarRevealBehavior: 'auto'` is forwarded to `revealWorktreeInSidebar(worktreeId, { behavior: 'auto' })`; -- no option still calls `revealWorktreeInSidebar(worktreeId)` (store default remains smooth). - -Add call-site regression tests (recommended, small): - -- one composer create path passes `'auto'`; -- one non-created navigation path stays default (no behavior option). - -## Rollout - -1. Add `sidebarRevealBehavior` option in `activateAndRevealWorktree`. -2. Update created-worktree callers to pass `'auto'`. -3. Add tests above. -4. Run targeted Vitest tests, then `pnpm typecheck` and `pnpm lint`. -5. Validate in Electron: with sidebar overflow, create a worktree and verify the list jumps to it without smooth animation. diff --git a/src/renderer/src/components/WorktreeJumpPalette.tsx b/src/renderer/src/components/WorktreeJumpPalette.tsx index 0b18f5f3f..1cfe10cf9 100644 --- a/src/renderer/src/components/WorktreeJumpPalette.tsx +++ b/src/renderer/src/components/WorktreeJumpPalette.tsx @@ -1409,7 +1409,7 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { skipRestoreFocusRef.current = true // Why: selecting a project or repo group is a sidebar navigation action; // it should reveal the grouping row without activating an arbitrary workspace. - revealSidebarRow(result.rowKey, { behavior: 'smooth', highlight: true }) + revealSidebarRow(result.rowKey, { highlight: true }) recordFeatureInteraction('cmd-j') closeModal() setSelectedItemId('') diff --git a/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx b/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx index 5ec3354ec..33de6b6eb 100644 --- a/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx +++ b/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx @@ -82,10 +82,7 @@ const NonGitFolderDialog = React.memo(function NonGitFolderDialog() { onboarding, hadProjectBeforeAdd ) - activateAndRevealWorktree(folderWorktree.id, { - sidebarRevealBehavior: 'auto', - ...(startup ? { startup } : {}) - }) + activateAndRevealWorktree(folderWorktree.id, startup ? { startup } : undefined) } } catch (err) { // This code path calls addRemote directly (not through the store), diff --git a/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx b/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx index f35804724..664095267 100644 --- a/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx +++ b/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx @@ -145,9 +145,7 @@ describe('ProjectAddedDialog', () => { expect(markup).toBe('') expect(mocks.state.fetchWorktrees).toHaveBeenCalledWith('repo-1') - expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::folder', { - sidebarRevealBehavior: 'auto' - }) + expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::folder') expect(mocks.state.closeModal).toHaveBeenCalledTimes(1) expect(mocks.finishProjectAddWithDefaultCheckout).not.toHaveBeenCalled() }) diff --git a/src/renderer/src/components/sidebar/ProjectAddedDialog.tsx b/src/renderer/src/components/sidebar/ProjectAddedDialog.tsx index 6532a39ca..34f53ffd6 100644 --- a/src/renderer/src/components/sidebar/ProjectAddedDialog.tsx +++ b/src/renderer/src/components/sidebar/ProjectAddedDialog.tsx @@ -81,7 +81,7 @@ export default function ProjectAddedDialog(): null { } const folderWorktree = useAppStore.getState().worktreesByRepo[repoId]?.[0] if (folderWorktree) { - activateAndRevealWorktree(folderWorktree.id, { sidebarRevealBehavior: 'auto' }) + activateAndRevealWorktree(folderWorktree.id) } closeModal() })() diff --git a/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx b/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx index 7bfda38d2..033cb286d 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx @@ -36,7 +36,7 @@ function revealCompactAgentCard(agentListRoot: HTMLElement | null): void { if (!(sidebarElement instanceof HTMLElement) || !worktreeOptionElement) { return } - revealElementInScrollContainer(sidebarElement, worktreeOptionElement, 'auto') + revealElementInScrollContainer(sidebarElement, worktreeOptionElement) } type Props = { @@ -238,7 +238,7 @@ const WorktreeCardAgentsBody = React.memo(function WorktreeCardAgentsBody({ // Why: defer the reveal scroll out of the expand commit. Running it inline // forces a synchronous sidebar layout that blocks the animation's opening // frames (a visible jump); next-frame keeps the open smooth and the - // ScrollBehavior 'auto' still lands before the height transition finishes. + // instant reveal still lands before the height transition finishes. const handle = requestAnimationFrame(() => { revealCompactAgentCard(compactAgentListRootRef.current) }) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 7b8aaab26..34725365a 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -100,6 +100,7 @@ import { type RenderRow } from './worktree-list-virtual-rows' import { + getElementCenteringScrollPadding, revealElementInScrollContainer, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -287,6 +288,7 @@ import { import { getFolderWorkspaceCardPrDisplay } from './folder-workspace-card-pr-display' export { + getCenteringScrollPadding, getScrollTopToRevealBounds, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -470,31 +472,15 @@ function markSidebarWorktreeActiveImmediately(worktreeId: string, primaryRowKey? } } -function revealMountedWorktreeElement( - container: HTMLElement, - worktreeId: string, - behavior: ScrollBehavior, - optionId?: string -): HTMLElement | null { - const element = optionId - ? document.getElementById(optionId) - : getMountedWorktreeOptions(worktreeId, container)[0] - if (!element || !container.contains(element)) { - return null - } - return revealElementInScrollContainer(container, element, behavior) ? element : null -} - function revealMountedSidebarRowElement( container: HTMLElement, - rowKey: string, - behavior: ScrollBehavior + rowKey: string ): HTMLElement | null { const element = document.getElementById(getWorktreeOptionId(rowKey)) if (!element || !container.contains(element)) { return null } - return revealElementInScrollContainer(container, element, behavior) ? element : null + return revealElementInScrollContainer(container, element) ? element : null } function getRenderRowSidebarKey(row: RenderRow): string | null { @@ -1336,6 +1322,11 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp const [worktreeDragState, setWorktreeDragState] = useState( WORKTREE_ROW_DRAG_INITIAL_STATE ) + const [centeringScrollPadding, setCenteringScrollPadding] = useState<{ + targetKey: string + start: number + end: number + } | null>(null) const [pendingRevealRetryTick, setPendingRevealRetryTick] = useState(0) const [documentVisibilityRevision, setDocumentVisibilityRevision] = useState(0) const [highlightedRevealRowKey, setHighlightedRevealRowKey] = useState(null) @@ -1985,6 +1976,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp ), overscan: 10, gap: 6, + paddingStart: centeringScrollPadding?.start ?? 0, + paddingEnd: centeringScrollPadding?.end ?? 0, // Why: the active sticky group header is rendered inside the virtual list, // so TanStack's scroll math needs the same top inset as the exact DOM reveal. scrollPaddingStart: WORKTREE_SIDEBAR_REVEAL_TOP_INSET, @@ -2132,14 +2125,35 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp clearPendingRevealWorktreeId() } } - const revealedOption = container - ? revealMountedWorktreeElement( - container, - pendingRevealWorktree.worktreeId, - pendingRevealWorktree.behavior, - getRenderRowOptionId(targetRow, pendingRevealWorktree.worktreeId) - ) - : null + const optionId = getRenderRowOptionId(targetRow, pendingRevealWorktree.worktreeId) + const centeringTargetKey = `worktree:${optionId ?? pendingRevealWorktree.worktreeId}` + if (centeringScrollPadding && centeringScrollPadding.targetKey !== centeringTargetKey) { + setCenteringScrollPadding(null) + retryExactRevealOnNextFrame() + return + } + const option = optionId + ? document.getElementById(optionId) + : getMountedWorktreeOptions(pendingRevealWorktree.worktreeId, container)[0] + const mountedOption = container && option && container.contains(option) ? option : null + if (container && mountedOption) { + const additionalPadding = getElementCenteringScrollPadding(container, mountedOption) + if (additionalPadding && (additionalPadding.start > 0 || additionalPadding.end > 0)) { + // Why: the first and last virtual rows need temporary boundary space + // or the browser clamps their centered position to the list edge. + setCenteringScrollPadding({ + targetKey: centeringTargetKey, + start: (centeringScrollPadding?.start ?? 0) + additionalPadding.start, + end: (centeringScrollPadding?.end ?? 0) + additionalPadding.end + }) + retryExactRevealOnNextFrame() + return + } + } + const revealedOption = + container && mountedOption && revealElementInScrollContainer(container, mountedOption) + ? mountedOption + : null if (revealedOption) { if (pendingRevealWorktree.highlight) { const revealedRowKey = @@ -2211,6 +2225,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, setRenamingWorktreeId, + centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -2294,12 +2309,16 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } const container = scrollRef.current + // Why: only clear stale boundary padding here. While a worktree reveal is + // still pending, clearing would wipe the padding it just accumulated and + // its boundary target could exhaust retries without ever centering. + if (centeringScrollPadding && !pendingRevealWorktree) { + setCenteringScrollPadding(null) + retryExactRevealOnNextFrame() + return + } const revealedElement = container - ? revealMountedSidebarRowElement( - container, - pendingRevealSidebarRow.rowKey, - pendingRevealSidebarRow.behavior - ) + ? revealMountedSidebarRowElement(container, pendingRevealSidebarRow.rowKey) : null if (revealedElement) { if (pendingRevealSidebarRow.highlight) { @@ -2323,6 +2342,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }, [ pendingRevealSidebarRow, + pendingRevealWorktree, repoMap, projectGroups, projectGrouping, @@ -2334,6 +2354,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, clearPendingRevealSidebarRow, + centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -6657,7 +6678,6 @@ const WorktreeList = React.memo(function WorktreeList({ { target: { type: 'sidebar-row' } } > revealSidebarRow(detail.target.rowKey, { - behavior: 'smooth', highlight: sidebarDetail.highlight !== false }) return @@ -6679,7 +6699,6 @@ const WorktreeList = React.memo(function WorktreeList({ clearFilters() } revealWorktreeInSidebar(currentSidebarWorktreeId, { - behavior: 'smooth', highlight: true, beginRename: (detail as { beginRename?: boolean } | undefined)?.beginRename === true }) diff --git a/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts b/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts index 51a5ad755..83caaaa6d 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts @@ -213,9 +213,7 @@ describe('useCreateRepo default-checkout handoff', () => { kind: 'git' }) expect(mocks.fetchWorktrees).toHaveBeenCalledWith(repo.id) - expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith(worktree.id, { - sidebarRevealBehavior: 'auto' - }) + expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith(worktree.id) expect(mocks.markOnboardingProjectAdded).toHaveBeenCalledWith('addedFolder') expect(closeModal).toHaveBeenCalled() expect(mocks.onGitRepoReady).not.toHaveBeenCalled() diff --git a/src/renderer/src/components/sidebar/useCreateRepo.ts b/src/renderer/src/components/sidebar/useCreateRepo.ts index abf35db7e..eb42f347e 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.ts @@ -190,7 +190,7 @@ export function useCreateRepo( } const folderWorktree = useAppStore.getState().worktreesByRepo[repo.id]?.[0] if (folderWorktree) { - activateAndRevealWorktree(folderWorktree.id, { sidebarRevealBehavior: 'auto' }) + activateAndRevealWorktree(folderWorktree.id) } await markOnboardingProjectAdded('addedFolder') closeModal() diff --git a/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts b/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts index 924a4edc8..f9a5a5f62 100644 --- a/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts +++ b/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { countRecordKeysByReference, + getCenteringScrollPadding, getScrollTopToRevealBounds, resolvePendingSidebarReveal, WORKTREE_SIDEBAR_REVEAL_TOP_INSET, @@ -43,8 +44,11 @@ const makeImportedCardRow = (): Extract - ({ scrollTop, clientHeight }) as HTMLElement +const makeScrollContainer = ( + scrollTop: number, + clientHeight: number, + scrollHeight = 1_000 +): HTMLElement => ({ scrollTop, clientHeight, scrollHeight }) as HTMLElement describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => { it('counts record keys once per object reference', () => { @@ -121,64 +125,64 @@ describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => { }) describe('getScrollTopToRevealBounds', () => { - it('treats the sticky header as occluding the viewport top', () => { - const container = makeScrollContainer(100, 400) + it('requests leading space when the first target would be clamped above center', () => { + const container = makeScrollContainer(0, 400) expect( - getScrollTopToRevealBounds( + getCenteringScrollPadding( container, { - start: 100, - end: 216 - }, - GROUP_HEADER_ROW_HEIGHT - ) - ).toBe(72) - }) - - it('includes extra reveal clearance for the highlight ring', () => { - const container = makeScrollContainer(100, 400) - - expect( - getScrollTopToRevealBounds( - container, - { - start: 100, - end: 216 + start: 34, + end: 74 }, WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - ).toBe(66) + ).toEqual({ start: 163, end: 0 }) }) - it('does not scroll when the bounds are below the sticky header', () => { + it('centers a fully visible target', () => { const container = makeScrollContainer(100, 400) expect( getScrollTopToRevealBounds( container, { - start: 128, - end: 244 + start: 300, + end: 400 }, - GROUP_HEADER_ROW_HEIGHT + WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - ).toBeNull() + ).toBe(133) }) - it('keeps the viewport bottom independent of the sticky header inset', () => { + it('centers within the area below the sticky header', () => { const container = makeScrollContainer(100, 400) expect( getScrollTopToRevealBounds( + container, + { + start: 300, + end: 400 + }, + GROUP_HEADER_ROW_HEIGHT + ) + ).toBe(136) + }) + + it('requests trailing space when the last target would be clamped below center', () => { + const container = makeScrollContainer(100, 400, 500) + + expect( + getCenteringScrollPadding( container, { start: 430, - end: 520 + end: 470 }, - GROUP_HEADER_ROW_HEIGHT + WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - ).toBe(120) + ).toEqual({ start: 0, end: 133 }) }) }) diff --git a/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts b/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts index 933261f20..850fc4570 100644 --- a/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts +++ b/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts @@ -8,15 +8,17 @@ describe('getScrollTopToRevealBounds', () => { clientHeight }) as HTMLElement - it('scrolls upward to reveal a mounted current workspace card above the viewport', () => { - expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 60, end: 120 })).toBe(60) + it('centers a mounted current workspace card that starts above the viewport', () => { + // Raw result may be negative; boundary reveals convert the deficit into + // temporary paddingStart instead of clamping (see getCenteringScrollPadding). + expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 60, end: 120 })).toBe(-10) }) - it('scrolls downward to reveal a mounted current workspace card below the viewport', () => { - expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 250, end: 340 })).toBe(140) + it('centers a mounted current workspace card that starts below the viewport', () => { + expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 250, end: 340 })).toBe(195) }) - it('does not scroll when the current workspace card is already fully visible', () => { - expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 125, end: 260 })).toBeNull() + it('recenters a card that is already fully visible', () => { + expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 125, end: 260 })).toBe(92.5) }) }) diff --git a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts index 38b158532..765efc2ea 100644 --- a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts +++ b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts @@ -9,6 +9,11 @@ type SidebarRevealBounds = { end: number } +type SidebarCenteringScrollPadding = { + start: number + end: number +} + function getElementScrollBounds(container: HTMLElement, element: Element): SidebarRevealBounds { const containerRect = container.getBoundingClientRect() const elementRect = element.getBoundingClientRect() @@ -22,24 +27,40 @@ export function getScrollTopToRevealBounds( container: HTMLElement, bounds: SidebarRevealBounds, topInset = 0 -): number | null { +): number { const viewportTopInset = Math.max(0, Math.min(container.clientHeight, topInset)) - const viewportTop = container.scrollTop + viewportTopInset - const viewportBottom = container.scrollTop + container.clientHeight - if (bounds.start < viewportTop) { - return bounds.start - viewportTopInset - } - if (bounds.end > viewportBottom) { - return bounds.end - container.clientHeight - } - return null + // Why: the sticky header reduces the usable viewport, so center within the + // visible area below it instead of behind it. + const targetCenter = bounds.start + (bounds.end - bounds.start) / 2 + const viewportCenterOffset = (viewportTopInset + container.clientHeight) / 2 + return targetCenter - viewportCenterOffset } -export function revealElementInScrollContainer( +export function getCenteringScrollPadding( + container: HTMLElement, + bounds: SidebarRevealBounds, + topInset = 0 +): SidebarCenteringScrollPadding { + const desiredScrollTop = getScrollTopToRevealBounds(container, bounds, topInset) + const maxScrollTop = Math.max(0, container.scrollHeight - container.clientHeight) + return { + start: Math.ceil(Math.max(0, -desiredScrollTop)), + end: Math.ceil(Math.max(0, desiredScrollTop - maxScrollTop)) + } +} + +export function getElementCenteringScrollPadding( container: HTMLElement, element: Element, - behavior: ScrollBehavior -): boolean { + topInset = WORKTREE_SIDEBAR_REVEAL_TOP_INSET +): SidebarCenteringScrollPadding | null { + if (!container.contains(element)) { + return null + } + return getCenteringScrollPadding(container, getElementScrollBounds(container, element), topInset) +} + +export function revealElementInScrollContainer(container: HTMLElement, element: Element): boolean { if (!container.contains(element)) { return false } @@ -48,17 +69,8 @@ export function revealElementInScrollContainer( getElementScrollBounds(container, element), WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - if (nextScrollTop === null) { - return true - } - // Why: honor the user's reduced-motion preference by jumping instantly instead of - // animating a smooth scroll (also makes the reveal deterministic in headless - // environments that never tick the smooth-scroll animation). - const prefersReducedMotion = - typeof window !== 'undefined' && - window.matchMedia?.('(prefers-reduced-motion: reduce)').matches === true - const resolvedBehavior: ScrollBehavior = - behavior === 'smooth' && prefersReducedMotion ? 'auto' : behavior - container.scrollTo({ top: Math.max(0, nextScrollTop), behavior: resolvedBehavior }) + // Why: sidebar reveal is a focus handoff, so reposition immediately instead + // of making the user track an animated list. + container.scrollTop = Math.max(0, nextScrollTop) return true } diff --git a/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.test.ts b/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.test.ts index 8cd870aec..35686a4ab 100644 --- a/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.test.ts @@ -153,9 +153,7 @@ describe('forkAgentSessionFromPane', () => { launchSource: 'terminal_context_menu' }) ) - expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork', { - sidebarRevealBehavior: 'auto' - }) + expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork') expect(mockToast.success).toHaveBeenCalledWith( 'Top-level session fork opened in a new workspace' ) @@ -346,9 +344,7 @@ describe('forkAgentSessionFromPane', () => { undefined ) expect(mockLaunchAgentInNewTab).not.toHaveBeenCalled() - expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork', { - sidebarRevealBehavior: 'auto' - }) + expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork') expect(mockWriteClipboardText).toHaveBeenCalledWith( expect.stringContaining('Assistant: here is the current plan') ) diff --git a/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.ts b/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.ts index 6b36a6387..7f2d62b47 100644 --- a/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.ts +++ b/src/renderer/src/components/terminal-pane/terminal-agent-session-fork.ts @@ -266,7 +266,7 @@ export async function startAgentSessionFork(fork: PreparedAgentSessionFork): Pro const forkWorktreeId = created.worktree.id if (!fork.agent) { - activateAndRevealWorktree(forkWorktreeId, { sidebarRevealBehavior: 'auto' }) + activateAndRevealWorktree(forkWorktreeId) return copyAgentSessionForkContext(fork) } await preflightForkAgentTrust({ @@ -287,7 +287,7 @@ export async function startAgentSessionFork(fork: PreparedAgentSessionFork): Pro launchSource: 'terminal_context_menu', ...(launchPlatform ? { launchPlatform } : {}) }) - activateAndRevealWorktree(forkWorktreeId, { sidebarRevealBehavior: 'auto' }) + activateAndRevealWorktree(forkWorktreeId) if (!result) { return copyAgentSessionForkContext(fork) diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index d1316b986..4f4b6ea8b 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -3687,7 +3687,6 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS startupPlan.launchToken = createBrowserUuid() } const activation = activateAndRevealWorktree(worktree.id, { - sidebarRevealBehavior: 'auto', setup: result.setup, defaultTabs: result.defaultTabs, issueCommand, diff --git a/src/renderer/src/hooks/useIpcEvents.test.ts b/src/renderer/src/hooks/useIpcEvents.test.ts index c4c51907c..3a7683073 100644 --- a/src/renderer/src/hooks/useIpcEvents.test.ts +++ b/src/renderer/src/hooks/useIpcEvents.test.ts @@ -4025,7 +4025,6 @@ describe('useIpcEvents CLI-created worktree activation', () => { expect(activateAndRevealWorktree).toHaveBeenCalledTimes(1) expect(activateAndRevealWorktree).toHaveBeenCalledWith('wt-new', { setup, - sidebarRevealBehavior: 'auto', notifyHostRuntime: false }) diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index f918744e6..5657d69a5 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -935,12 +935,10 @@ export function useIpcEvents(): void { // stream and is allowed through this helper separately. return } - const existedBeforeFetch = Boolean(useAppStore.getState().getKnownWorktreeById(worktreeId)) // Why: fetch worktrees first so the activation helper can resolve // the CLI-created worktree via findWorktreeById — it arrived from // the main process and is not yet in the renderer state. await useAppStore.getState().fetchWorktrees(repoId) - const existsAfterFetch = Boolean(useAppStore.getState().getKnownWorktreeById(worktreeId)) // Why: route through activateAndRevealWorktree so CLI-created // worktrees share the canonical activation path with UI-created // ones. This records the visit in the back/forward history stack @@ -950,7 +948,6 @@ export function useIpcEvents(): void { ...(setup ? { setup } : {}), ...(startup ? { startup } : {}), ...(defaultTabs ? { defaultTabs } : {}), - ...(!existedBeforeFetch && existsAfterFetch ? { sidebarRevealBehavior: 'auto' } : {}), // Why: this activation already came from the host runtime event stream. // Echoing it back as worktree.activate can create a selection loop. notifyHostRuntime: false diff --git a/src/renderer/src/lib/launch-work-item-direct.ts b/src/renderer/src/lib/launch-work-item-direct.ts index 61f49274e..7d88276cc 100644 --- a/src/renderer/src/lib/launch-work-item-direct.ts +++ b/src/renderer/src/lib/launch-work-item-direct.ts @@ -216,7 +216,6 @@ export async function launchWorkItemDirect(args: LaunchWorkItemDirectArgs): Prom !isTuiAgentEnabled(agentOverride, latestStore.settings?.disabledTuiAgents) ) { activateAndRevealWorktree(worktreeId, { - sidebarRevealBehavior: 'auto', setup: result.setup }) toast.error(unavailableAgentErrorMessage()) @@ -282,7 +281,6 @@ export async function launchWorkItemDirect(args: LaunchWorkItemDirectArgs): Prom })) const activation = activateAndRevealWorktree(worktreeId, { - sidebarRevealBehavior: 'auto', setup: result.setup, defaultTabs: result.defaultTabs, ...buildDirectWorkItemStartupOpts(effectiveAgent, startupPlan, launchSource) diff --git a/src/renderer/src/lib/worktree-activation-created-agent.test.ts b/src/renderer/src/lib/worktree-activation-created-agent.test.ts index 174c1fab3..184eb7dc7 100644 --- a/src/renderer/src/lib/worktree-activation-created-agent.test.ts +++ b/src/renderer/src/lib/worktree-activation-created-agent.test.ts @@ -299,10 +299,10 @@ describe('activateAndRevealWorktree created agent reopen', () => { revealWorktreeInSidebar }) - const result = activateAndRevealWorktree(worktree.id, { sidebarRevealBehavior: 'auto' }) + const result = activateAndRevealWorktree(worktree.id) expect(result).toEqual({ primaryTabId: expect.any(String) }) - expect(revealWorktreeInSidebar).toHaveBeenCalledWith(worktree.id, { behavior: 'auto' }) + expect(revealWorktreeInSidebar).toHaveBeenCalledWith(worktree.id) }) it('asks the host runtime to activate the worktree in the paired web client', async () => { diff --git a/src/renderer/src/lib/worktree-activation.ts b/src/renderer/src/lib/worktree-activation.ts index 1d94786a3..4f735de5c 100644 --- a/src/renderer/src/lib/worktree-activation.ts +++ b/src/renderer/src/lib/worktree-activation.ts @@ -25,7 +25,6 @@ import { CLIENT_PLATFORM } from './new-workspace' import { tuiAgentToAgentKind } from './telemetry' import { agentKindToTuiAgent } from '../../../shared/agent-kind' import { useAppStore } from '@/store' -import type { PendingSidebarWorktreeReveal } from '@/store/slices/ui' import { tabHasLivePty } from '@/lib/tab-has-live-pty' import { activateWebRuntimeSessionWorktree, @@ -183,7 +182,6 @@ function ensureFolderWorkspaceInitialTerminal( export function activateAndRevealFolderWorkspace( folderWorkspaceId: string, opts?: { - sidebarRevealBehavior?: PendingSidebarWorktreeReveal['behavior'] startup?: WorktreeStartupPayload runtimeEnvironmentId?: string | null } @@ -227,11 +225,7 @@ export function activateAndRevealFolderWorkspace( resumeSleepingAgentSessionsForWorktree(workspaceKey) const primaryTabId = ensureFolderWorkspaceInitialTerminal(folderWorkspace, opts?.startup) - if (opts?.sidebarRevealBehavior) { - state.revealWorktreeInSidebar(workspaceKey, { behavior: opts.sidebarRevealBehavior }) - } else { - state.revealWorktreeInSidebar(workspaceKey) - } + state.revealWorktreeInSidebar(workspaceKey) return { primaryTabId } } @@ -289,7 +283,6 @@ export function activateAndRevealWorktree( setup?: WorktreeSetupLaunch defaultTabs?: WorktreeDefaultTabsLaunch issueCommand?: IssueCommandLaunch - sidebarRevealBehavior?: PendingSidebarWorktreeReveal['behavior'] notifyHostRuntime?: boolean revealInSidebar?: boolean } @@ -389,11 +382,7 @@ export function activateAndRevealWorktree( // 6. Reveal in sidebar if (opts?.revealInSidebar !== false) { - if (opts?.sidebarRevealBehavior) { - state.revealWorktreeInSidebar(worktreeId, { behavior: opts.sidebarRevealBehavior }) - } else { - state.revealWorktreeInSidebar(worktreeId) - } + state.revealWorktreeInSidebar(worktreeId) } if (opts?.notifyHostRuntime !== false) { diff --git a/src/renderer/src/lib/worktree-creation-flow.ts b/src/renderer/src/lib/worktree-creation-flow.ts index 9af1dc61e..3e37be6d1 100644 --- a/src/renderer/src/lib/worktree-creation-flow.ts +++ b/src/renderer/src/lib/worktree-creation-flow.ts @@ -222,7 +222,6 @@ async function executeWorktreeCreation( let primaryTabId: string | null if (stillActive) { activation = activateAndRevealWorktree(worktree.id, { - sidebarRevealBehavior: 'auto', ...(result.setup ? { setup: result.setup } : {}), ...(result.defaultTabs ? { defaultTabs: result.defaultTabs } : {}), ...(startupOpt ? { startup: startupOpt } : {}), diff --git a/src/renderer/src/store/slices/repos-onboarding-folder-startup.test.ts b/src/renderer/src/store/slices/repos-onboarding-folder-startup.test.ts index a18a2733d..71e071f91 100644 --- a/src/renderer/src/store/slices/repos-onboarding-folder-startup.test.ts +++ b/src/renderer/src/store/slices/repos-onboarding-folder-startup.test.ts @@ -56,7 +56,6 @@ describe('repo slice skipped-onboarding folder startup', () => { 1, 'folder-1::/folder', { - sidebarRevealBehavior: 'auto', startup: { command: "codex '--dangerously-bypass-approvals-and-sandbox'", env: {}, @@ -77,7 +76,7 @@ describe('repo slice skipped-onboarding folder startup', () => { expect(worktreeActivation.activateAndRevealWorktree).toHaveBeenNthCalledWith( 2, 'folder-2::/folder', - { sidebarRevealBehavior: 'auto' } + undefined ) }) }) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 6fc31768e..4f535acbc 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -2586,10 +2586,7 @@ export const createRepoSlice: StateCreator = (set, onboarding, hadProjectBeforeAdd ) - activateAndRevealWorktree(folderWorktree.id, { - sidebarRevealBehavior: 'auto', - ...(startup ? { startup } : {}) - }) + activateAndRevealWorktree(folderWorktree.id, startup ? { startup } : undefined) } return repo } catch (err) { diff --git a/src/renderer/src/store/slices/ui.test.ts b/src/renderer/src/store/slices/ui.test.ts index 6b4e88cbc..202adfe5e 100644 --- a/src/renderer/src/store/slices/ui.test.ts +++ b/src/renderer/src/store/slices/ui.test.ts @@ -286,7 +286,6 @@ describe('createUISlice agent send target mode', () => { }) expect(store.getState().pendingRevealWorktree).toMatchObject({ worktreeId, - behavior: 'auto', highlight: true }) }) @@ -1098,14 +1097,12 @@ describe('createUISlice hydratePersistedUI', () => { const store = createUIStore() store.getState().revealWorktreeInSidebar('repo1::/feature', { - behavior: 'smooth', highlight: true, beginRename: true }) expect(store.getState().pendingRevealWorktree).toEqual({ worktreeId: 'repo1::/feature', - behavior: 'smooth', highlight: true, beginRename: true }) diff --git a/src/renderer/src/store/slices/ui.ts b/src/renderer/src/store/slices/ui.ts index 04748058e..7b280b7b1 100644 --- a/src/renderer/src/store/slices/ui.ts +++ b/src/renderer/src/store/slices/ui.ts @@ -113,14 +113,12 @@ import { translate } from '@/i18n/i18n' export type PendingSidebarWorktreeReveal = { worktreeId: string - behavior: 'auto' | 'smooth' highlight?: boolean beginRename?: boolean } export type PendingSidebarRowReveal = { rowKey: string - behavior: 'auto' | 'smooth' highlight?: boolean } @@ -897,7 +895,6 @@ export type UISlice = { revealWorktreeInSidebar: ( worktreeId: string, options?: { - behavior?: PendingSidebarWorktreeReveal['behavior'] highlight?: boolean beginRename?: boolean } @@ -905,7 +902,6 @@ export type UISlice = { revealSidebarRow: ( rowKey: string, options?: { - behavior?: PendingSidebarRowReveal['behavior'] highlight?: boolean } ) => void @@ -989,7 +985,7 @@ export const createUISlice: StateCreator = (set, get) targets.some((target) => target.status === 'eligible') && (previousMode?.id !== args.id || previousMode.worktreeId !== args.worktreeId) ) { - get().revealWorktreeInSidebar(args.worktreeId, { behavior: 'auto', highlight: true }) + get().revealWorktreeInSidebar(args.worktreeId, { highlight: true }) } }, closeAgentSendPopoverTargetMode: (id, instanceId) => @@ -2205,7 +2201,6 @@ export const createUISlice: StateCreator = (set, get) set({ pendingRevealWorktree: { worktreeId, - behavior: options?.behavior ?? 'smooth', ...(options?.highlight ? { highlight: true } : {}), ...(options?.beginRename ? { beginRename: true } : {}) } @@ -2214,7 +2209,6 @@ export const createUISlice: StateCreator = (set, get) set({ pendingRevealSidebarRow: { rowKey, - behavior: options?.behavior ?? 'smooth', ...(options?.highlight === false ? {} : { highlight: true }) } }), diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index 8929f5759..8e0cb6e3a 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -6412,7 +6412,7 @@ describe('setWorktreesPinnedAndReveal', () => { store.getState().setWorktreesPinnedAndReveal([wt.id], true) expect(store.getState().worktreesByRepo.repo1[0].isPinned).toBe(true) - expect(reveal).toHaveBeenCalledWith(wt.id, { behavior: 'smooth', highlight: true }) + expect(reveal).toHaveBeenCalledWith(wt.id, { highlight: true }) }) it('reveals on unpin so the viewport follows the row back to its status group', () => { @@ -6427,7 +6427,7 @@ describe('setWorktreesPinnedAndReveal', () => { store.getState().setWorktreesPinnedAndReveal([wt.id], false) expect(store.getState().worktreesByRepo.repo1[0].isPinned).toBe(false) - expect(reveal).toHaveBeenCalledWith(wt.id, { behavior: 'smooth', highlight: true }) + expect(reveal).toHaveBeenCalledWith(wt.id, { highlight: true }) }) it('skips a no-op toggle without requesting a reveal', () => { @@ -6492,7 +6492,7 @@ describe('setWorktreesPinnedAndReveal', () => { store.getState().setWorktreesPinnedAndReveal([alreadyPinned.id, first.id, second.id], true) expect(reveal).toHaveBeenCalledTimes(1) - expect(reveal).toHaveBeenCalledWith(first.id, { behavior: 'smooth', highlight: true }) + expect(reveal).toHaveBeenCalledWith(first.id, { highlight: true }) // Every targeted row is pinned, not just the revealed one, and the // already-pinned row is left untouched. expect(store.getState().worktreesByRepo.repo1[0].isPinned).toBe(true) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index d6fb11a31..9c350fd76 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -3959,7 +3959,7 @@ export const createWorktreeSlice: StateCreator // persistence is async), so the reveal below resolves against a render // where the shortcut row already exists. void get().updateWorktreesMeta(updates) - get().revealWorktreeInSidebar(revealWorktreeId, { behavior: 'smooth', highlight: true }) + get().revealWorktreeInSidebar(revealWorktreeId, { highlight: true }) }, markWorktreeUnread: (worktreeId) => { diff --git a/tests/e2e/worktree-scroll-to-current.spec.ts b/tests/e2e/worktree-scroll-to-current.spec.ts index 19c51005c..a3bef0d09 100644 --- a/tests/e2e/worktree-scroll-to-current.spec.ts +++ b/tests/e2e/worktree-scroll-to-current.spec.ts @@ -23,11 +23,9 @@ async function prepareSidebarForScrollTest(page: Page): Promise { test.describe('Reveal active workspace button', () => { test.beforeEach(async ({ orcaPage }) => { - // Why: headless Electron under xvfb never ticks a smooth-scroll animation, - // so the reveal's `scrollTo({ behavior: 'smooth' })` would never reach its - // target. Reduced-motion makes the reveal jump instantly (see - // worktree-sidebar-reveal.ts) so the geometry assertions are deterministic. - await orcaPage.emulateMedia({ reducedMotion: 'reduce' }) + // Sidebar reveal now always jumps instantly (worktree-sidebar-reveal.ts + // assigns scrollTop directly), so no reduced-motion emulation is needed + // for the geometry assertions to be deterministic. await waitForSessionReady(orcaPage) await waitForActiveWorktree(orcaPage) }) diff --git a/tests/e2e/worktree-switch-responsiveness.spec.ts b/tests/e2e/worktree-switch-responsiveness.spec.ts index 45a6e6e6b..11b4dea8c 100644 --- a/tests/e2e/worktree-switch-responsiveness.spec.ts +++ b/tests/e2e/worktree-switch-responsiveness.spec.ts @@ -33,8 +33,8 @@ async function prepareSidebarForSwitchTest(page: Page): Promise<[string, string] if ((state.tabsByWorktree[second.id] ?? []).length === 0) { state.createTab(second.id, undefined, undefined, { pendingActivationSpawn: true }) } - state.revealWorktreeInSidebar(first.id, { behavior: 'auto' }) - state.revealWorktreeInSidebar(second.id, { behavior: 'auto' }) + state.revealWorktreeInSidebar(first.id) + state.revealWorktreeInSidebar(second.id) state.setActiveWorktree(first.id) return [first.id, second.id] })