From 4d0ace87ecb3024aa9606df587abc6313429b343 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 09:23:29 +0000 Subject: [PATCH 01/10] release: v1.4.133-rc.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index ad112158be..3a31196cbe 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.132", + "version": "1.4.133-rc.0", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", From 26224196f593d0f86043672d7da7ecda8fdec3b3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 10 Jul 2026 02:31:47 -0700 Subject: [PATCH 02/10] perf(worktrees): avoid auto-maintenance in create fetches (#8039) * perf(worktrees): avoid auto-maintenance in create fetches Git's opportunistic maintenance can keep an already-complete exact-base fetch open for seconds. Disable it per command for create-base refreshes only, leaving ordinary fetch maintenance and exact-ref freshness unchanged. * docs(worktrees): explain create fetch maintenance scope * test(worktrees): account for fetch config prefixes * fix(worktrees): cover Git 2.29 auto maintenance --- src/main/ipc/worktree-remote.ts | 6 +- src/main/ipc/worktrees.test.ts | 9 ++- src/main/providers/ssh-git-provider.test.ts | 6 +- src/main/providers/ssh-git-provider.ts | 6 +- src/main/runtime/fetch-remote-cache.test.ts | 71 +++++++++++++-------- src/main/runtime/orca-runtime.test.ts | 36 +++++++++-- src/main/runtime/orca-runtime.ts | 10 ++- src/relay/git-handler.test.ts | 16 ++++- src/relay/git-handler.ts | 16 ++++- src/shared/git-fetch-auto-maintenance.ts | 10 +++ 10 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 src/shared/git-fetch-auto-maintenance.ts diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index e0a2bbf5db..43ef357eed 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -477,7 +477,11 @@ async function refreshRemoteTrackingBaseForWorktreeCreate( return getOrStartSshWorktreeCreateFetch( getSshWorktreeCreateBaseFetchKey(repo, base), getSshWorktreeCreateRemoteQueueKey(repo, base.remote), - () => provider.fetchRemoteTrackingRef(repo.path, base.remote, base.branch, base.ref) + () => + // Why: the exact-base refresh gates create; unrelated repo housekeeping must not extend it. + provider.fetchRemoteTrackingRef(repo.path, base.remote, base.branch, base.ref, { + skipAutoMaintenance: true + }) ) } diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 43c9432578..7b4f699cd6 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -3733,7 +3733,8 @@ describe('registerWorktreeHandlers', () => { '/remote/repo', 'origin', 'master', - 'refs/remotes/origin/master' + 'refs/remotes/origin/master', + { skipAutoMaintenance: true } ) }) @@ -3800,7 +3801,8 @@ describe('registerWorktreeHandlers', () => { '/remote/repo', 'origin', 'main', - 'refs/remotes/origin/main' + 'refs/remotes/origin/main', + { skipAutoMaintenance: true } ) expect(provider.addWorktree).toHaveBeenCalledWith( '/remote/repo', @@ -4032,7 +4034,8 @@ describe('registerWorktreeHandlers', () => { '/remote/repo', 'origin', 'main', - 'refs/remotes/origin/main' + 'refs/remotes/origin/main', + { skipAutoMaintenance: true } ) expect(provider.addWorktree).toHaveBeenCalledTimes(2) }) diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index acfaefaed1..0974367ef8 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -850,14 +850,16 @@ describe('SshGitProvider', () => { '/home/user/repo', 'origin', 'main', - 'refs/remotes/origin/main' + 'refs/remotes/origin/main', + { skipAutoMaintenance: true } ) expect(mux.request).toHaveBeenCalledWith('git.fetchRemoteTrackingRef', { worktreePath: '/home/user/repo', remote: 'origin', branch: 'main', - ref: 'refs/remotes/origin/main' + ref: 'refs/remotes/origin/main', + skipAutoMaintenance: true }) }) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 5ba6a29e38..0cde62d534 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -553,14 +553,16 @@ export class SshGitProvider implements IGitProvider { worktreePath: string, remote: string, branch: string, - ref: string + ref: string, + options?: { skipAutoMaintenance?: boolean } ): Promise { await this.runWithDiffDedupeClear(async () => { await this.mux.request('git.fetchRemoteTrackingRef', { worktreePath, remote, branch, - ref + ref, + ...(options?.skipAutoMaintenance ? { skipAutoMaintenance: true } : {}) }) }) } diff --git a/src/main/runtime/fetch-remote-cache.test.ts b/src/main/runtime/fetch-remote-cache.test.ts index fce2a5e6d5..14c1811563 100644 --- a/src/main/runtime/fetch-remote-cache.test.ts +++ b/src/main/runtime/fetch-remote-cache.test.ts @@ -22,10 +22,19 @@ vi.mock('../git/runner', async (importOriginal) => { // normally — none of them trigger IO until a runtime method is called. import { OrcaRuntimeService } from './orca-runtime' +function isFetchArgs(argv: unknown): argv is string[] { + if (!Array.isArray(argv)) { + return false + } + let commandIndex = 0 + while (argv[commandIndex] === '-c' && typeof argv[commandIndex + 1] === 'string') { + commandIndex += 2 + } + return argv[commandIndex] === 'fetch' +} + function fetchCallCount(): number { - return gitExecFileAsyncMock.mock.calls.filter( - ([argv]) => Array.isArray(argv) && argv[0] === 'fetch' - ).length + return gitExecFileAsyncMock.mock.calls.filter(([argv]) => isFetchArgs(argv)).length } function exactBaseRefreshOptions(cwd: string): { @@ -36,6 +45,21 @@ function exactBaseRefreshOptions(cwd: string): { return { cwd, timeout: 60_000, useConfiguredSshCommandForNetwork: true } } +function exactBaseRefreshArgs(branch = 'main'): string[] { + return [ + '-c', + 'maintenance.auto=false', + '-c', + 'maintenance.commit-graph.auto=0', + '-c', + 'gc.auto=0', + 'fetch', + '--no-tags', + 'origin', + `+refs/heads/${branch}:refs/remotes/origin/${branch}` + ] +} + // Why (STA-1292): the broad create-path fetch must carry a timeout so a Windows // credential-manager GUI hang can't wedge worktree creation forever. function fullRemoteFetchOptions(cwd: string): { cwd: string; timeout: number } { @@ -188,11 +212,23 @@ describe('OrcaRuntimeService.fetchRemoteWithCache', () => { }) expect(gitExecFileAsyncMock).toHaveBeenCalledWith( - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], + exactBaseRefreshArgs(), exactBaseRefreshOptions('/repo/f') ) }) + it('keeps automatic maintenance enabled for ordinary full remote fetches', async () => { + mockFetchResults([{ stdout: '', stderr: '' }]) + const runtime = new OrcaRuntimeService(null) + + await runtime.getOrStartRemoteFetch('/repo/full-maintenance', 'origin') + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['fetch', 'origin'], + fullRemoteFetchOptions('/repo/full-maintenance') + ) + }) + it('shares an in-flight remote-tracking base refresh and reuses exact-base freshness', async () => { let resolveFetch!: () => void const pending = new Promise<{ stdout: string; stderr: string }>((resolve) => { @@ -297,14 +333,9 @@ describe('OrcaRuntimeService.fetchRemoteWithCache', () => { { ok: true }, { ok: true } ]) - const fetchCalls = gitExecFileAsyncMock.mock.calls.filter( - ([argv]) => Array.isArray(argv) && argv[0] === 'fetch' - ) + const fetchCalls = gitExecFileAsyncMock.mock.calls.filter(([argv]) => isFetchArgs(argv)) expect(fetchCalls).toEqual([ - [ - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], - exactBaseRefreshOptions('/repo/h') - ], + [exactBaseRefreshArgs(), exactBaseRefreshOptions('/repo/h')], [['fetch', 'origin'], fullRemoteFetchOptions('/repo/h')] ]) }) @@ -343,15 +374,10 @@ describe('OrcaRuntimeService.fetchRemoteWithCache', () => { { ok: true }, { ok: true } ]) - const fetchCalls = gitExecFileAsyncMock.mock.calls.filter( - ([argv]) => Array.isArray(argv) && argv[0] === 'fetch' - ) + const fetchCalls = gitExecFileAsyncMock.mock.calls.filter(([argv]) => isFetchArgs(argv)) expect(fetchCalls).toEqual([ [['fetch', 'origin'], fullRemoteFetchOptions('/repo/i')], - [ - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], - exactBaseRefreshOptions('/repo/i') - ] + [exactBaseRefreshArgs(), exactBaseRefreshOptions('/repo/i')] ]) }) @@ -389,15 +415,10 @@ describe('OrcaRuntimeService.fetchRemoteWithCache', () => { { ok: false, errorKind: 'git_error' }, { ok: true } ]) - const fetchCalls = gitExecFileAsyncMock.mock.calls.filter( - ([argv]) => Array.isArray(argv) && argv[0] === 'fetch' - ) + const fetchCalls = gitExecFileAsyncMock.mock.calls.filter(([argv]) => isFetchArgs(argv)) expect(fetchCalls).toEqual([ [['fetch', 'origin'], fullRemoteFetchOptions('/repo/i-fail')], - [ - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], - exactBaseRefreshOptions('/repo/i-fail') - ] + [exactBaseRefreshArgs(), exactBaseRefreshOptions('/repo/i-fail')] ]) }) }) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index ef14013273..b5fb46e500 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -2312,7 +2312,7 @@ describe('OrcaRuntimeService', () => { if (args[0] === 'rev-parse' && args[1] === '--verify') { return { stdout: 'base-sha\n', stderr: '' } } - if (args[0] === 'fetch') { + if (args.includes('fetch')) { return refresh.promise } return { stdout: '', stderr: '' } @@ -2325,7 +2325,18 @@ describe('OrcaRuntimeService', () => { await vi.waitFor(() => { expect(gitSpy).toHaveBeenCalledWith( - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], + [ + '-c', + 'maintenance.auto=false', + '-c', + 'maintenance.commit-graph.auto=0', + '-c', + 'gc.auto=0', + 'fetch', + '--no-tags', + 'origin', + '+refs/heads/main:refs/remotes/origin/main' + ], { cwd: TEST_REPO_PATH, useConfiguredSshCommandForNetwork: true, @@ -2443,7 +2454,7 @@ describe('OrcaRuntimeService', () => { if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main^{commit}')) { return { stdout: 'base-sha\n', stderr: '' } } - if (args[0] === 'fetch') { + if (args.includes('fetch')) { throw new Error('network unavailable') } return { stdout: '', stderr: '' } @@ -2505,7 +2516,7 @@ describe('OrcaRuntimeService', () => { if (args[0] === 'rev-parse' && args.includes('refs/heads/develop^{commit}')) { return { stdout: 'develop-sha\n', stderr: '' } } - if (args[0] === 'fetch') { + if (args.includes('fetch')) { throw new Error('network unavailable') } return { stdout: '', stderr: '' } @@ -2559,7 +2570,7 @@ describe('OrcaRuntimeService', () => { if (args[0] === 'rev-parse' && args.includes('refs/heads/team/feature^{commit}')) { return { stdout: 'team-feature-sha\n', stderr: '' } } - if (args[0] === 'fetch') { + if (args.includes('fetch')) { throw new Error('network unavailable') } return { stdout: '', stderr: '' } @@ -2580,7 +2591,18 @@ describe('OrcaRuntimeService', () => { false ) expect(gitSpy).not.toHaveBeenCalledWith( - ['fetch', '--no-tags', 'team', '+refs/heads/feature:refs/remotes/team/feature'], + [ + '-c', + 'maintenance.auto=false', + '-c', + 'maintenance.commit-graph.auto=0', + '-c', + 'gc.auto=0', + 'fetch', + '--no-tags', + 'team', + '+refs/heads/feature:refs/remotes/team/feature' + ], expect.any(Object) ) } finally { @@ -2604,7 +2626,7 @@ describe('OrcaRuntimeService', () => { if (args[0] === 'rev-parse' && args[1] === '--verify') { throw new Error('missing ref') } - if (args[0] === 'fetch') { + if (args.includes('fetch')) { throw new Error('network unavailable') } return { stdout: '', stderr: '' } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index a16fcca9f8..f420ebd003 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -49,6 +49,7 @@ import { getClonePathComparisonKey } from '../git/repo-clone-path' import { getGitCloneFailureMessage } from '../../shared/git-clone-failure-message' +import { GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS } from '../../shared/git-fetch-auto-maintenance' import { createHash, randomUUID } from 'node:crypto' import { homedir } from 'node:os' import { isAbsolute, join, resolve } from 'node:path' @@ -14620,8 +14621,15 @@ export class OrcaRuntimeService { if (this.getFreshFetchCompletedAt(key) !== null) { return { ok: true } } + // Why: this exact refresh gates worktree create; ordinary fetches still own maintenance. return gitExecFileAsync( - ['fetch', '--no-tags', base.remote, `+refs/heads/${base.branch}:${base.ref}`], + [ + ...GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS, + 'fetch', + '--no-tags', + base.remote, + `+refs/heads/${base.branch}:${base.ref}` + ], { cwd: repoPath, ...gitOptions, diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 07b84db43f..ba8f631d81 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -1222,7 +1222,8 @@ describe('GitHandler', () => { worktreePath: tmpDir, remote: 'origin', branch: 'main', - ref: 'refs/remotes/origin/main' + ref: 'refs/remotes/origin/main', + skipAutoMaintenance: true }) const second = dispatcher.callRequest('git.diff', { @@ -1238,7 +1239,18 @@ describe('GitHandler', () => { expect(gitBufferSpy).toHaveBeenCalledTimes(2) expect(gitSpy).toHaveBeenCalledWith( - ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], + [ + '-c', + 'maintenance.auto=false', + '-c', + 'maintenance.commit-graph.auto=0', + '-c', + 'gc.auto=0', + 'fetch', + '--no-tags', + 'origin', + '+refs/heads/main:refs/remotes/origin/main' + ], tmpDir ) }) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index a5ae6d3cb7..c878dfd563 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -60,6 +60,7 @@ import { import { getGitCloneFailureMessage } from '../shared/git-clone-failure-message' import { syncForkDefaultBranch, validateGitForkSyncExpectedUpstream } from '../shared/git-fork-sync' import { InFlightPromiseDedupe, stableInFlightKey } from '../shared/in-flight-promise-dedupe' +import { GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS } from '../shared/git-fetch-auto-maintenance' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -844,10 +845,14 @@ export class GitHandler { const remote = params.remote const branch = params.branch const ref = params.ref + const skipAutoMaintenance = params.skipAutoMaintenance try { if (typeof remote !== 'string' || typeof branch !== 'string' || typeof ref !== 'string') { throw new Error('Invalid remote-tracking fetch request.') } + if (skipAutoMaintenance !== undefined && typeof skipAutoMaintenance !== 'boolean') { + throw new Error('Invalid remote-tracking fetch maintenance option.') + } if (remote.startsWith('-') || branch.startsWith('-')) { throw new Error('Remote-tracking fetch inputs must not start with "-".') } @@ -866,7 +871,16 @@ export class GitHandler { } await this.git(['check-ref-format', `refs/heads/${branch}`], worktreePath) await this.git(['check-ref-format', ref], worktreePath) - await this.git(['fetch', '--no-tags', remote, `+refs/heads/${branch}:${ref}`], worktreePath) + await this.git( + [ + ...(skipAutoMaintenance ? GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS : []), + 'fetch', + '--no-tags', + remote, + `+refs/heads/${branch}:${ref}` + ], + worktreePath + ) } catch (error) { // Why: create-worktree needs a write-capable fetch, but generic git.exec // intentionally rejects fetch. This narrow RPC keeps the relay allowlist diff --git a/src/shared/git-fetch-auto-maintenance.ts b/src/shared/git-fetch-auto-maintenance.ts new file mode 100644 index 0000000000..3723298ab3 --- /dev/null +++ b/src/shared/git-fetch-auto-maintenance.ts @@ -0,0 +1,10 @@ +// Why: Git 2.29 can auto-run commit-graph work before maintenance.auto became a gate. +// The other keys cover modern maintenance and legacy auto-gc without changing user config. +export const GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS = [ + '-c', + 'maintenance.auto=false', + '-c', + 'maintenance.commit-graph.auto=0', + '-c', + 'gc.auto=0' +] as const From 8c50aaddf6b2f829fc60d906ebababe59125c0ff Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 10 Jul 2026 02:47:23 -0700 Subject: [PATCH 03/10] Keep speech model dropdown open during downloads (#8087) --- .../settings/VoiceSpeechModelSection.test.tsx | 30 +++++++++++++++++-- .../settings/VoiceSpeechModelSection.tsx | 4 ++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx b/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx index b6e061994a..4825397fba 100644 --- a/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx +++ b/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx @@ -8,6 +8,7 @@ import type { SpeechModelManifest, SpeechModelState } from '../../../../shared/s import { getDefaultVoiceSettings } from '../../../../shared/constants' const toastErrorMock = vi.hoisted(() => vi.fn()) +const menuDismissMock = vi.hoisted(() => vi.fn()) vi.mock('sonner', () => ({ toast: { @@ -32,7 +33,7 @@ vi.mock('../ui/dropdown-menu', () => ({ }: { children: ReactNode disabled?: boolean - onSelect?: () => void + onSelect?: (event: Event) => void className?: string }) => (
({ role="option" onClick={() => { if (!disabled) { - onSelect?.() + const selectEvent = new Event('select', { cancelable: true }) + onSelect?.(selectEvent) + if (!selectEvent.defaultPrevented) { + menuDismissMock() + } } }} > @@ -73,6 +78,7 @@ const secondLocalModel: SpeechModelManifest = { function renderSection(args: { deleteModel: (modelId: string) => Promise + downloadModel?: (modelId: string) => Promise catalog?: SpeechModelManifest[] modelStates?: SpeechModelState[] refreshModelStates?: () => void @@ -81,7 +87,7 @@ function renderSection(args: { api: { speech: { deleteModel: vi.fn(args.deleteModel), - downloadModel: vi.fn() + downloadModel: vi.fn(args.downloadModel ?? (() => Promise.resolve())) } } }) @@ -111,6 +117,7 @@ function renderSection(args: { describe('VoiceSpeechModelSection', () => { beforeEach(() => { toastErrorMock.mockReset() + menuDismissMock.mockReset() }) afterEach(() => { @@ -218,4 +225,21 @@ describe('VoiceSpeechModelSection', () => { expect(refreshModelStates).not.toHaveBeenCalled() root.unmount() }) + + it('keeps the model menu open when starting a local model download', async () => { + const { container, root } = renderSection({ + deleteModel: () => Promise.resolve(), + modelStates: [{ id: localModel.id, status: 'not-downloaded' }] + }) + const modelOption = container.querySelector('[role="option"]') + + await act(async () => { + modelOption!.dispatchEvent(new MouseEvent('click', { bubbles: true })) + await Promise.resolve() + }) + + expect(window.api.speech.downloadModel).toHaveBeenCalledWith(localModel.id) + expect(menuDismissMock).not.toHaveBeenCalled() + root.unmount() + }) }) diff --git a/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx b/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx index 0380bbc0ce..ea96c68fd3 100644 --- a/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx +++ b/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx @@ -82,12 +82,14 @@ export function VoiceSpeechModelSection({ { + onSelect={(event) => { if (isReady) { onUpdateVoiceSettings({ sttModel: manifest.id }) } else if (isCloud) { onOpenOpenAiDialog(manifest.id) } else if (!isDownloading) { + // Why: download progress appears in this menu, so starting one should not dismiss it. + event.preventDefault() void window.api.speech .downloadModel(manifest.id) .catch(() => From c17d7cdae82c94e68280940dd8dcabe0bb32796c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 09:48:30 +0000 Subject: [PATCH 04/10] release: v1.4.133 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 3a31196cbe..757b3d7e22 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.133-rc.0", + "version": "1.4.133", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", From db9bae81bd060397547c23e558bcfad627bbb73b Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:00:21 -0700 Subject: [PATCH 05/10] Revert sidebar virtualization boundary changes (#8036) Reverts #8036 to remove the sidebar jumpiness regression. --- .../src/components/sidebar/WorktreeList.tsx | 40 ++++++++++++++++ .../worktree-list-scroll-adjustment.test.ts | 46 ++++++++++++------- .../worktree-scroll-to-current-button.test.ts | 4 +- .../sidebar/worktree-sidebar-reveal.ts | 32 ++++++++++++- 4 files changed, 101 insertions(+), 21 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 84f5ab7783..a1542323e6 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' @@ -1320,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) @@ -1969,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, @@ -2120,10 +2129,30 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } } 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 @@ -2199,6 +2228,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, setRenamingWorktreeId, + centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -2282,6 +2312,14 @@ 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) : null @@ -2307,6 +2345,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }, [ pendingRevealSidebarRow, + pendingRevealWorktree, repoMap, projectGroups, projectGrouping, @@ -2318,6 +2357,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, clearPendingRevealSidebarRow, + centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) 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 aa71d47944..f9a5a5f62c 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,12 +1,12 @@ import { describe, expect, it, vi } from 'vitest' import { countRecordKeysByReference, + getCenteringScrollPadding, getScrollTopToRevealBounds, resolvePendingSidebarReveal, WORKTREE_SIDEBAR_REVEAL_TOP_INSET, shouldAdjustWorktreeSidebarMeasuredRowScroll } from './WorktreeList' -import { revealElementInScrollContainer } from './worktree-sidebar-reveal' import { extractWorktreeVirtualRowIndexes, estimateRenderRowSize, @@ -125,6 +125,21 @@ describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => { }) describe('getScrollTopToRevealBounds', () => { + it('requests leading space when the first target would be clamped above center', () => { + const container = makeScrollContainer(0, 400) + + expect( + getCenteringScrollPadding( + container, + { + start: 34, + end: 74 + }, + WORKTREE_SIDEBAR_REVEAL_TOP_INSET + ) + ).toEqual({ start: 163, end: 0 }) + }) + it('centers a fully visible target', () => { const container = makeScrollContainer(100, 400) @@ -155,22 +170,19 @@ describe('getScrollTopToRevealBounds', () => { ).toBe(136) }) - it('clamps a boundary target to the list edge instead of padding the list', () => { - // Regression (#8019 follow-up): centering first/last rows via temporary - // paddingStart/paddingEnd left a permanent phantom gap in the sidebar once - // the reveal finished, since nothing cleared the padding afterwards. - const container = { - scrollTop: 100, - clientHeight: 200, - contains: () => true, - getBoundingClientRect: () => ({ top: 0, bottom: 200 }) - } as unknown as HTMLElement - const firstRow = { - getBoundingClientRect: () => ({ top: -40, bottom: 20 }) - } as unknown as Element - - expect(revealElementInScrollContainer(container, firstRow)).toBe(true) - expect(container.scrollTop).toBe(0) + 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: 470 + }, + WORKTREE_SIDEBAR_REVEAL_TOP_INSET + ) + ).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 e2ec2cfaff..850fc45704 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 @@ -9,8 +9,8 @@ describe('getScrollTopToRevealBounds', () => { }) as HTMLElement it('centers a mounted current workspace card that starts above the viewport', () => { - // Raw result may be negative; revealElementInScrollContainer clamps it to - // the list edge so boundary reveals never pad the list. + // 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) }) diff --git a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts index 56586682a5..765efc2eab 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() @@ -31,6 +36,30 @@ export function getScrollTopToRevealBounds( return targetCenter - viewportCenterOffset } +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, + 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 @@ -41,8 +70,7 @@ export function revealElementInScrollContainer(container: HTMLElement, element: WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) // Why: sidebar reveal is a focus handoff, so reposition immediately instead - // of making the user track an animated list. Boundary rows clamp to the list - // edge — padding the list so they can center leaves a phantom gap behind. + // of making the user track an animated list. container.scrollTop = Math.max(0, nextScrollTop) return true } From 70ff47ea90f14dd8d51fff9a61a562522c866feb Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:00:51 -0700 Subject: [PATCH 06/10] Revert centered sidebar jump behavior (#8019) Reverts #8019 to restore the sidebar reveal behavior from before the jumpiness regression. --- docs/new-worktree-sidebar-reveal.md | 79 +++++++++++++++++ .../src/components/WorktreeJumpPalette.tsx | 2 +- .../components/sidebar/NonGitFolderDialog.tsx | 5 +- .../sidebar/ProjectAddedDialog.test.tsx | 4 +- .../components/sidebar/ProjectAddedDialog.tsx | 2 +- .../components/sidebar/WorktreeCardAgents.tsx | 4 +- .../src/components/sidebar/WorktreeList.tsx | 85 +++++++------------ .../useCreateRepo.default-checkout.test.ts | 4 +- .../src/components/sidebar/useCreateRepo.ts | 2 +- .../worktree-list-scroll-adjustment.test.ts | 50 +++++------ .../worktree-scroll-to-current-button.test.ts | 14 ++- .../sidebar/worktree-sidebar-reveal.ts | 60 ++++++------- .../terminal-agent-session-fork.test.ts | 8 +- .../terminal-agent-session-fork.ts | 4 +- src/renderer/src/hooks/useComposerState.ts | 1 + src/renderer/src/hooks/useIpcEvents.test.ts | 1 + src/renderer/src/hooks/useIpcEvents.ts | 3 + .../src/lib/launch-work-item-direct.ts | 2 + .../worktree-activation-created-agent.test.ts | 4 +- src/renderer/src/lib/worktree-activation.ts | 15 +++- .../src/lib/worktree-creation-flow.ts | 1 + .../repos-onboarding-folder-startup.test.ts | 3 +- src/renderer/src/store/slices/repos.ts | 5 +- src/renderer/src/store/slices/ui.test.ts | 3 + src/renderer/src/store/slices/ui.ts | 8 +- .../src/store/slices/worktrees.test.ts | 6 +- src/renderer/src/store/slices/worktrees.ts | 2 +- tests/e2e/worktree-scroll-to-current.spec.ts | 8 +- .../worktree-switch-responsiveness.spec.ts | 4 +- 29 files changed, 238 insertions(+), 151 deletions(-) create mode 100644 docs/new-worktree-sidebar-reveal.md diff --git a/docs/new-worktree-sidebar-reveal.md b/docs/new-worktree-sidebar-reveal.md new file mode 100644 index 0000000000..75abb7bd64 --- /dev/null +++ b/docs/new-worktree-sidebar-reveal.md @@ -0,0 +1,79 @@ +# 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 1cfe10cf9b..0b18f5f3f0 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, { highlight: true }) + revealSidebarRow(result.rowKey, { behavior: 'smooth', 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 33de6b6eb1..5ec3354ec2 100644 --- a/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx +++ b/src/renderer/src/components/sidebar/NonGitFolderDialog.tsx @@ -82,7 +82,10 @@ const NonGitFolderDialog = React.memo(function NonGitFolderDialog() { onboarding, hadProjectBeforeAdd ) - activateAndRevealWorktree(folderWorktree.id, startup ? { startup } : undefined) + activateAndRevealWorktree(folderWorktree.id, { + sidebarRevealBehavior: 'auto', + ...(startup ? { startup } : {}) + }) } } 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 6640952677..f358047243 100644 --- a/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx +++ b/src/renderer/src/components/sidebar/ProjectAddedDialog.test.tsx @@ -145,7 +145,9 @@ describe('ProjectAddedDialog', () => { expect(markup).toBe('') expect(mocks.state.fetchWorktrees).toHaveBeenCalledWith('repo-1') - expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::folder') + expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::folder', { + sidebarRevealBehavior: 'auto' + }) 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 34f53ffd6a..6532a39cae 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) + activateAndRevealWorktree(folderWorktree.id, { sidebarRevealBehavior: 'auto' }) } closeModal() })() diff --git a/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx b/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx index 033cb286df..7bfda38d23 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) + revealElementInScrollContainer(sidebarElement, worktreeOptionElement, 'auto') } 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 - // instant reveal still lands before the height transition finishes. + // ScrollBehavior 'auto' 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 a1542323e6..e9403b41b1 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -100,7 +100,6 @@ import { type RenderRow } from './worktree-list-virtual-rows' import { - getElementCenteringScrollPadding, revealElementInScrollContainer, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -288,7 +287,6 @@ import { import { getFolderWorkspaceCardPrDisplay } from './folder-workspace-card-pr-display' export { - getCenteringScrollPadding, getScrollTopToRevealBounds, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -472,15 +470,31 @@ 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 + rowKey: string, + behavior: ScrollBehavior ): HTMLElement | null { const element = document.getElementById(getWorktreeOptionId(rowKey)) if (!element || !container.contains(element)) { return null } - return revealElementInScrollContainer(container, element) ? element : null + return revealElementInScrollContainer(container, element, behavior) ? element : null } function getRenderRowSidebarKey(row: RenderRow): string | null { @@ -1322,11 +1336,6 @@ 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) @@ -1976,8 +1985,6 @@ 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, @@ -2128,35 +2135,14 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp clearPendingRevealWorktreeId() } } - 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 + const revealedOption = container + ? revealMountedWorktreeElement( + container, + pendingRevealWorktree.worktreeId, + pendingRevealWorktree.behavior, + getRenderRowOptionId(targetRow, pendingRevealWorktree.worktreeId) + ) + : null if (revealedOption) { if (pendingRevealWorktree.highlight) { const revealedRowKey = @@ -2228,7 +2214,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, setRenamingWorktreeId, - centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -2312,16 +2297,12 @@ 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) + ? revealMountedSidebarRowElement( + container, + pendingRevealSidebarRow.rowKey, + pendingRevealSidebarRow.behavior + ) : null if (revealedElement) { if (pendingRevealSidebarRow.highlight) { @@ -2345,7 +2326,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }, [ pendingRevealSidebarRow, - pendingRevealWorktree, repoMap, projectGroups, projectGrouping, @@ -2357,7 +2337,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, clearPendingRevealSidebarRow, - centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -6681,6 +6660,7 @@ const WorktreeList = React.memo(function WorktreeList({ { target: { type: 'sidebar-row' } } > revealSidebarRow(detail.target.rowKey, { + behavior: 'smooth', highlight: sidebarDetail.highlight !== false }) return @@ -6702,6 +6682,7 @@ 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 83caaaa6d1..51a5ad755b 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts @@ -213,7 +213,9 @@ describe('useCreateRepo default-checkout handoff', () => { kind: 'git' }) expect(mocks.fetchWorktrees).toHaveBeenCalledWith(repo.id) - expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith(worktree.id) + expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith(worktree.id, { + sidebarRevealBehavior: 'auto' + }) 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 eb42f347e6..abf35db7e6 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) + activateAndRevealWorktree(folderWorktree.id, { sidebarRevealBehavior: 'auto' }) } 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 f9a5a5f62c..924a4edc88 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,7 +1,6 @@ import { describe, expect, it, vi } from 'vitest' import { countRecordKeysByReference, - getCenteringScrollPadding, getScrollTopToRevealBounds, resolvePendingSidebarReveal, WORKTREE_SIDEBAR_REVEAL_TOP_INSET, @@ -44,11 +43,8 @@ const makeImportedCardRow = (): Extract ({ scrollTop, clientHeight, scrollHeight }) as HTMLElement +const makeScrollContainer = (scrollTop: number, clientHeight: number): HTMLElement => + ({ scrollTop, clientHeight }) as HTMLElement describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => { it('counts record keys once per object reference', () => { @@ -125,64 +121,64 @@ describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => { }) describe('getScrollTopToRevealBounds', () => { - it('requests leading space when the first target would be clamped above center', () => { - const container = makeScrollContainer(0, 400) + it('treats the sticky header as occluding the viewport top', () => { + const container = makeScrollContainer(100, 400) expect( - getCenteringScrollPadding( + getScrollTopToRevealBounds( container, { - start: 34, - end: 74 + start: 100, + end: 216 }, - WORKTREE_SIDEBAR_REVEAL_TOP_INSET + GROUP_HEADER_ROW_HEIGHT ) - ).toEqual({ start: 163, end: 0 }) + ).toBe(72) }) - it('centers a fully visible target', () => { + it('includes extra reveal clearance for the highlight ring', () => { const container = makeScrollContainer(100, 400) expect( getScrollTopToRevealBounds( container, { - start: 300, - end: 400 + start: 100, + end: 216 }, WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - ).toBe(133) + ).toBe(66) }) - it('centers within the area below the sticky header', () => { + it('does not scroll when the bounds are below the sticky header', () => { const container = makeScrollContainer(100, 400) expect( getScrollTopToRevealBounds( container, { - start: 300, - end: 400 + start: 128, + end: 244 }, GROUP_HEADER_ROW_HEIGHT ) - ).toBe(136) + ).toBeNull() }) - it('requests trailing space when the last target would be clamped below center', () => { - const container = makeScrollContainer(100, 400, 500) + it('keeps the viewport bottom independent of the sticky header inset', () => { + const container = makeScrollContainer(100, 400) expect( - getCenteringScrollPadding( + getScrollTopToRevealBounds( container, { start: 430, - end: 470 + end: 520 }, - WORKTREE_SIDEBAR_REVEAL_TOP_INSET + GROUP_HEADER_ROW_HEIGHT ) - ).toEqual({ start: 0, end: 133 }) + ).toBe(120) }) }) 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 850fc45704..933261f20a 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,17 +8,15 @@ describe('getScrollTopToRevealBounds', () => { clientHeight }) as HTMLElement - 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 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 below the viewport', () => { - expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 250, end: 340 })).toBe(195) + 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('recenters a card that is already fully visible', () => { - expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 125, end: 260 })).toBe(92.5) + it('does not scroll when the current workspace card is already fully visible', () => { + expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 125, end: 260 })).toBeNull() }) }) diff --git a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts index 765efc2eab..38b158532a 100644 --- a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts +++ b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts @@ -9,11 +9,6 @@ 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() @@ -27,40 +22,24 @@ export function getScrollTopToRevealBounds( container: HTMLElement, bounds: SidebarRevealBounds, topInset = 0 -): number { +): number | null { const viewportTopInset = Math.max(0, Math.min(container.clientHeight, topInset)) - // 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 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)) + 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 } -export function getElementCenteringScrollPadding( +export function revealElementInScrollContainer( container: HTMLElement, element: Element, - 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 { + behavior: ScrollBehavior +): boolean { if (!container.contains(element)) { return false } @@ -69,8 +48,17 @@ export function revealElementInScrollContainer(container: HTMLElement, element: getElementScrollBounds(container, element), WORKTREE_SIDEBAR_REVEAL_TOP_INSET ) - // 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) + 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 }) 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 35686a4abe..8cd870aec1 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,7 +153,9 @@ describe('forkAgentSessionFromPane', () => { launchSource: 'terminal_context_menu' }) ) - expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork') + expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork', { + sidebarRevealBehavior: 'auto' + }) expect(mockToast.success).toHaveBeenCalledWith( 'Top-level session fork opened in a new workspace' ) @@ -344,7 +346,9 @@ describe('forkAgentSessionFromPane', () => { undefined ) expect(mockLaunchAgentInNewTab).not.toHaveBeenCalled() - expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork') + expect(mockActivateAndRevealWorktree).toHaveBeenCalledWith('wt-fork', { + sidebarRevealBehavior: 'auto' + }) 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 7f2d62b478..6b36a63874 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) + activateAndRevealWorktree(forkWorktreeId, { sidebarRevealBehavior: 'auto' }) return copyAgentSessionForkContext(fork) } await preflightForkAgentTrust({ @@ -287,7 +287,7 @@ export async function startAgentSessionFork(fork: PreparedAgentSessionFork): Pro launchSource: 'terminal_context_menu', ...(launchPlatform ? { launchPlatform } : {}) }) - activateAndRevealWorktree(forkWorktreeId) + activateAndRevealWorktree(forkWorktreeId, { sidebarRevealBehavior: 'auto' }) if (!result) { return copyAgentSessionForkContext(fork) diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index 4f4b6ea8b1..d1316b9867 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -3687,6 +3687,7 @@ 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 f8d46e38b8..bef5560289 100644 --- a/src/renderer/src/hooks/useIpcEvents.test.ts +++ b/src/renderer/src/hooks/useIpcEvents.test.ts @@ -4025,6 +4025,7 @@ 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 80e5c2e30c..fab0f38e01 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -931,10 +931,12 @@ 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 @@ -944,6 +946,7 @@ 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 7d88276cc5..61f49274ed 100644 --- a/src/renderer/src/lib/launch-work-item-direct.ts +++ b/src/renderer/src/lib/launch-work-item-direct.ts @@ -216,6 +216,7 @@ export async function launchWorkItemDirect(args: LaunchWorkItemDirectArgs): Prom !isTuiAgentEnabled(agentOverride, latestStore.settings?.disabledTuiAgents) ) { activateAndRevealWorktree(worktreeId, { + sidebarRevealBehavior: 'auto', setup: result.setup }) toast.error(unavailableAgentErrorMessage()) @@ -281,6 +282,7 @@ 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 184eb7dc7c..174c1fab32 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) + const result = activateAndRevealWorktree(worktree.id, { sidebarRevealBehavior: 'auto' }) expect(result).toEqual({ primaryTabId: expect.any(String) }) - expect(revealWorktreeInSidebar).toHaveBeenCalledWith(worktree.id) + expect(revealWorktreeInSidebar).toHaveBeenCalledWith(worktree.id, { behavior: 'auto' }) }) 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 4f735de5c1..1d94786a34 100644 --- a/src/renderer/src/lib/worktree-activation.ts +++ b/src/renderer/src/lib/worktree-activation.ts @@ -25,6 +25,7 @@ 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, @@ -182,6 +183,7 @@ function ensureFolderWorkspaceInitialTerminal( export function activateAndRevealFolderWorkspace( folderWorkspaceId: string, opts?: { + sidebarRevealBehavior?: PendingSidebarWorktreeReveal['behavior'] startup?: WorktreeStartupPayload runtimeEnvironmentId?: string | null } @@ -225,7 +227,11 @@ export function activateAndRevealFolderWorkspace( resumeSleepingAgentSessionsForWorktree(workspaceKey) const primaryTabId = ensureFolderWorkspaceInitialTerminal(folderWorkspace, opts?.startup) - state.revealWorktreeInSidebar(workspaceKey) + if (opts?.sidebarRevealBehavior) { + state.revealWorktreeInSidebar(workspaceKey, { behavior: opts.sidebarRevealBehavior }) + } else { + state.revealWorktreeInSidebar(workspaceKey) + } return { primaryTabId } } @@ -283,6 +289,7 @@ export function activateAndRevealWorktree( setup?: WorktreeSetupLaunch defaultTabs?: WorktreeDefaultTabsLaunch issueCommand?: IssueCommandLaunch + sidebarRevealBehavior?: PendingSidebarWorktreeReveal['behavior'] notifyHostRuntime?: boolean revealInSidebar?: boolean } @@ -382,7 +389,11 @@ export function activateAndRevealWorktree( // 6. Reveal in sidebar if (opts?.revealInSidebar !== false) { - state.revealWorktreeInSidebar(worktreeId) + if (opts?.sidebarRevealBehavior) { + state.revealWorktreeInSidebar(worktreeId, { behavior: opts.sidebarRevealBehavior }) + } else { + 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 3e37be6d18..9af1dc61eb 100644 --- a/src/renderer/src/lib/worktree-creation-flow.ts +++ b/src/renderer/src/lib/worktree-creation-flow.ts @@ -222,6 +222,7 @@ 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 71e071f91f..a18a2733dc 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,6 +56,7 @@ describe('repo slice skipped-onboarding folder startup', () => { 1, 'folder-1::/folder', { + sidebarRevealBehavior: 'auto', startup: { command: "codex '--dangerously-bypass-approvals-and-sandbox'", env: {}, @@ -76,7 +77,7 @@ describe('repo slice skipped-onboarding folder startup', () => { expect(worktreeActivation.activateAndRevealWorktree).toHaveBeenNthCalledWith( 2, 'folder-2::/folder', - undefined + { sidebarRevealBehavior: 'auto' } ) }) }) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 4f535acbce..6fc31768e2 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -2586,7 +2586,10 @@ export const createRepoSlice: StateCreator = (set, onboarding, hadProjectBeforeAdd ) - activateAndRevealWorktree(folderWorktree.id, startup ? { startup } : undefined) + activateAndRevealWorktree(folderWorktree.id, { + sidebarRevealBehavior: 'auto', + ...(startup ? { startup } : {}) + }) } 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 6c47388db0..eb1732f1a2 100644 --- a/src/renderer/src/store/slices/ui.test.ts +++ b/src/renderer/src/store/slices/ui.test.ts @@ -286,6 +286,7 @@ describe('createUISlice agent send target mode', () => { }) expect(store.getState().pendingRevealWorktree).toMatchObject({ worktreeId, + behavior: 'auto', highlight: true }) }) @@ -1097,12 +1098,14 @@ 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 00f40ee2f3..321f9ce09a 100644 --- a/src/renderer/src/store/slices/ui.ts +++ b/src/renderer/src/store/slices/ui.ts @@ -113,12 +113,14 @@ 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 } @@ -896,6 +898,7 @@ export type UISlice = { revealWorktreeInSidebar: ( worktreeId: string, options?: { + behavior?: PendingSidebarWorktreeReveal['behavior'] highlight?: boolean beginRename?: boolean } @@ -903,6 +906,7 @@ export type UISlice = { revealSidebarRow: ( rowKey: string, options?: { + behavior?: PendingSidebarRowReveal['behavior'] highlight?: boolean } ) => void @@ -986,7 +990,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, { highlight: true }) + get().revealWorktreeInSidebar(args.worktreeId, { behavior: 'auto', highlight: true }) } }, closeAgentSendPopoverTargetMode: (id, instanceId) => @@ -2202,6 +2206,7 @@ export const createUISlice: StateCreator = (set, get) set({ pendingRevealWorktree: { worktreeId, + behavior: options?.behavior ?? 'smooth', ...(options?.highlight ? { highlight: true } : {}), ...(options?.beginRename ? { beginRename: true } : {}) } @@ -2210,6 +2215,7 @@ 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 8e0cb6e3a3..8929f57594 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, { highlight: true }) + expect(reveal).toHaveBeenCalledWith(wt.id, { behavior: 'smooth', 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, { highlight: true }) + expect(reveal).toHaveBeenCalledWith(wt.id, { behavior: 'smooth', 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, { highlight: true }) + expect(reveal).toHaveBeenCalledWith(first.id, { behavior: 'smooth', 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 9c350fd765..d6fb11a315 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, { highlight: true }) + get().revealWorktreeInSidebar(revealWorktreeId, { behavior: 'smooth', 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 a3bef0d095..19c51005cf 100644 --- a/tests/e2e/worktree-scroll-to-current.spec.ts +++ b/tests/e2e/worktree-scroll-to-current.spec.ts @@ -23,9 +23,11 @@ async function prepareSidebarForScrollTest(page: Page): Promise { test.describe('Reveal active workspace button', () => { test.beforeEach(async ({ orcaPage }) => { - // 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. + // 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' }) 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 11b4dea8c9..45a6e6e6b2 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) - state.revealWorktreeInSidebar(second.id) + state.revealWorktreeInSidebar(first.id, { behavior: 'auto' }) + state.revealWorktreeInSidebar(second.id, { behavior: 'auto' }) state.setActiveWorktree(first.id) return [first.id, second.id] }) From 506f6ffbb9b970293af11895afdf278152b5a9e0 Mon Sep 17 00:00:00 2001 From: mehmet turac Date: Fri, 10 Jul 2026 13:02:34 +0300 Subject: [PATCH 07/10] Fix remote host branch diff routing (#6747) * fix(editor): route remote diff reads through ssh * Address folder workspace diff routing review * test(editor): regression-lock combined diff section SSH routing Extract getCombinedDiffSectionConnectionId so the per-section diff-read host resolution (joinPath + getConnectionIdForFile) is unit-covered, including the Windows separator composition for mixed folder workspaces (#6688). Co-authored-by: Orca --------- Co-authored-by: Orca --- .../components/editor/CombinedDiffViewer.tsx | 9 +- .../combined-diff-section-connection.test.ts | 128 ++++++++++++++++++ .../combined-diff-section-connection.ts | 17 +++ .../useEditorPanelContentState.test.tsx | 58 +++++++- .../editor/useEditorPanelContentState.ts | 8 +- .../src/lib/connection-context.test.ts | 40 ++++++ 6 files changed, 251 insertions(+), 9 deletions(-) create mode 100644 src/renderer/src/components/editor/combined-diff-section-connection.test.ts create mode 100644 src/renderer/src/components/editor/combined-diff-section-connection.ts diff --git a/src/renderer/src/components/editor/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/CombinedDiffViewer.tsx index a206fa8c8f..8f9eee91a8 100644 --- a/src/renderer/src/components/editor/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/CombinedDiffViewer.tsx @@ -18,7 +18,8 @@ import { createProgrammaticScrollMarks } from '@/hooks/programmatic-scroll-marks import { joinPath } from '@/lib/path' import { detectLanguage } from '@/lib/language-detect' import { setWithLRU } from '@/lib/scroll-cache' -import { getConnectionId, getConnectionIdForFile } from '@/lib/connection-context' +import { getConnectionIdForFile } from '@/lib/connection-context' +import { getCombinedDiffSectionConnectionId } from './combined-diff-section-connection' import { findWorktreeById } from '@/store/slices/worktree-helpers' import { writeRuntimeFile } from '@/runtime/runtime-file-client' import { settingsForRuntimeOwner } from '@/runtime/runtime-rpc-client' @@ -622,7 +623,11 @@ export default function CombinedDiffViewer({ let result: GitDiffResult let error: string | undefined try { - const connectionId = getConnectionId(file.worktreeId) ?? undefined + const connectionId = getCombinedDiffSectionConnectionId( + file.worktreeId, + file.filePath, + entry.path + ) const state = useAppStore.getState() const fileSettings = settingsForRuntimeOwner(state.settings, file.runtimeEnvironmentId) if ((isBranchMode || (isAllMode && !('area' in entry))) && branchCompare) { diff --git a/src/renderer/src/components/editor/combined-diff-section-connection.test.ts b/src/renderer/src/components/editor/combined-diff-section-connection.test.ts new file mode 100644 index 0000000000..894f6df783 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-section-connection.test.ts @@ -0,0 +1,128 @@ +import { afterEach, describe, expect, it } from 'vitest' +import type { FolderWorkspace, ProjectGroup, Repo } from '../../../../shared/types' +import { useAppStore } from '@/store' +import { folderWorkspaceKey } from '../../../../shared/workspace-scope' +import { getCombinedDiffSectionConnectionId } from './combined-diff-section-connection' + +const initialState = useAppStore.getInitialState() + +function makeRepo(overrides: Partial & { id: string }): Repo { + return { + path: '/home/neil/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + ...overrides + } +} + +function makeFolderWorkspace(folderPath: string): FolderWorkspace { + return { + id: 'folder-workspace-1', + projectGroupId: 'group-1', + name: 'Platform workspace', + folderPath, + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 1, + lastActivityAt: 0, + createdAt: 1, + updatedAt: 1 + } +} + +function makeGroup(parentPath: string): ProjectGroup { + return { + id: 'group-1', + name: 'Platform', + parentPath, + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } +} + +describe('getCombinedDiffSectionConnectionId', () => { + afterEach(() => { + useAppStore.setState(initialState, true) + }) + + it('routes a section to its SSH child repo in a mixed folder workspace (#6688)', () => { + const workspaceKey = folderWorkspaceKey('folder-workspace-1') + useAppStore.setState({ + folderWorkspaces: [makeFolderWorkspace('/home/neil/platform')], + projectGroups: [makeGroup('/home/neil/platform')], + repos: [ + makeRepo({ id: 'repo-local', path: '/home/neil/platform/web', projectGroupId: 'group-1' }), + makeRepo({ + id: 'repo-ssh', + path: '/home/neil/platform/api', + projectGroupId: 'group-1', + connectionId: 'ssh-1' + }) + ], + worktreesByRepo: {} + }) + + // Section paths are repo-tree-relative; joined onto the workspace root they + // must resolve to the owning child repo, not the ambiguous workspace. + expect( + getCombinedDiffSectionConnectionId(workspaceKey, '/home/neil/platform', 'api/src/index.ts') + ).toBe('ssh-1') + // Local child repo -> no connection (local read). + expect( + getCombinedDiffSectionConnectionId(workspaceKey, '/home/neil/platform', 'web/src/index.ts') + ).toBeUndefined() + // A path owned by no single child repo stays ambiguous. + expect( + getCombinedDiffSectionConnectionId(workspaceKey, '/home/neil/platform', 'README.md') + ).toBeUndefined() + }) + + it('composes Windows section paths with the workspace root separator', () => { + const workspaceKey = folderWorkspaceKey('folder-workspace-1') + // A mixed local+SSH workspace makes the whole-workspace owner ambiguous, so + // resolution must fall through to composing the section path — the behavior + // this test guards. A single-SSH workspace would resolve before joinPath. + useAppStore.setState({ + folderWorkspaces: [makeFolderWorkspace('C:\\Users\\neil\\platform')], + projectGroups: [makeGroup('C:\\Users\\neil\\platform')], + repos: [ + makeRepo({ + id: 'repo-local', + path: 'C:\\Users\\neil\\platform\\web', + projectGroupId: 'group-1' + }), + makeRepo({ + id: 'repo-ssh', + path: 'C:\\Users\\neil\\platform\\api', + projectGroupId: 'group-1', + connectionId: 'ssh-1' + }) + ], + worktreesByRepo: {} + }) + + expect( + getCombinedDiffSectionConnectionId( + workspaceKey, + 'C:\\Users\\neil\\platform', + 'api/src/index.ts' + ) + ).toBe('ssh-1') + expect( + getCombinedDiffSectionConnectionId( + workspaceKey, + 'C:\\Users\\neil\\platform', + 'web/src/index.ts' + ) + ).toBeUndefined() + }) +}) diff --git a/src/renderer/src/components/editor/combined-diff-section-connection.ts b/src/renderer/src/components/editor/combined-diff-section-connection.ts new file mode 100644 index 0000000000..b330ba4c13 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-section-connection.ts @@ -0,0 +1,17 @@ +import { getConnectionIdForFile } from '@/lib/connection-context' +import { joinPath } from '@/lib/path' + +/** + * Resolve the SSH connectionId that owns a combined-diff section's file. + * + * Why: folder workspaces mix local and SSH child repos, so the workspace + * worktree is an ambiguous owner; the section must resolve its host by the + * concrete child-repo path or a remote read is misrouted locally (#6688). + */ +export function getCombinedDiffSectionConnectionId( + worktreeId: string | null, + worktreeRootPath: string, + sectionPath: string +): string | undefined { + return getConnectionIdForFile(worktreeId, joinPath(worktreeRootPath, sectionPath)) ?? undefined +} diff --git a/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx b/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx index 15cd29adb4..90807e66cc 100644 --- a/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx +++ b/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx @@ -10,6 +10,7 @@ import type { DiffContent, FileContent } from './editor-panel-content-types' const mocks = vi.hoisted(() => ({ readRuntimeFileContent: vi.fn(), getRuntimeGitDiff: vi.fn(), + getRuntimeGitBranchDiff: vi.fn(), getConnectionId: vi.fn(), getConnectionIdForFile: vi.fn(), isWorktreeConnectionResolved: vi.fn(() => true), @@ -28,7 +29,7 @@ vi.mock('@/runtime/runtime-file-client', () => ({ })) vi.mock('@/runtime/runtime-git-client', () => ({ - getRuntimeGitBranchDiff: vi.fn(), + getRuntimeGitBranchDiff: mocks.getRuntimeGitBranchDiff, getRuntimeGitCommitDiff: vi.fn(), getRuntimeGitDiff: mocks.getRuntimeGitDiff, getRuntimeGitScope: vi.fn(() => null) @@ -131,6 +132,7 @@ describe('useEditorPanelContentState', () => { latestDiffContents = {} mocks.readRuntimeFileContent.mockReset() mocks.getRuntimeGitDiff.mockReset() + mocks.getRuntimeGitBranchDiff.mockReset() mocks.getConnectionId.mockReset() mocks.getConnectionId.mockReturnValue(undefined) mocks.getConnectionIdForFile.mockReset() @@ -188,6 +190,60 @@ describe('useEditorPanelContentState', () => { ) }) + it('loads folder workspace branch diffs through the path-specific SSH connection', async () => { + const activeFile = createOpenFile({ + id: 'branch-diff', + filePath: '/home/neil/platform/api/src/file.ts', + relativePath: 'api/src/file.ts', + worktreeId: 'folder:folder-workspace-1', + mode: 'diff', + diffSource: 'branch', + branchCompare: { + baseRef: 'main', + compareRef: 'feature', + compareVersion: 'feature', + baseOid: 'base', + headOid: 'head', + mergeBase: 'merge-base' + } + }) + mocks.getConnectionIdForFile.mockReturnValue('ssh-1') + mocks.getRuntimeGitBranchDiff.mockResolvedValue({ + kind: 'text', + originalContent: 'old', + modifiedContent: 'remote branch diff', + originalIsBinary: false, + modifiedIsBinary: false + }) + + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + + await act(async () => { + root?.render() + }) + + await vi.waitFor(() => + expect(latestDiffContents[activeFile.id]?.modifiedContent).toBe('remote branch diff') + ) + expect(mocks.getConnectionIdForFile).toHaveBeenCalledWith( + 'folder:folder-workspace-1', + '/home/neil/platform/api/src/file.ts' + ) + expect(mocks.getRuntimeGitBranchDiff).toHaveBeenCalledWith( + expect.objectContaining({ + worktreeId: 'folder:folder-workspace-1', + worktreePath: '/home/neil/platform', + connectionId: 'ssh-1' + }), + expect.objectContaining({ + compare: expect.objectContaining({ mergeBase: 'merge-base' }), + filePath: 'api/src/file.ts' + }) + ) + }) + it('does not read locally while a remote host worktree owner is still hydrating (#6648)', async () => { const activeFile = createOpenFile({ filePath: '/home/user/project/src/index.ts', diff --git a/src/renderer/src/components/editor/useEditorPanelContentState.ts b/src/renderer/src/components/editor/useEditorPanelContentState.ts index ae05de9547..f4f668a5a4 100644 --- a/src/renderer/src/components/editor/useEditorPanelContentState.ts +++ b/src/renderer/src/components/editor/useEditorPanelContentState.ts @@ -3,11 +3,7 @@ make the hook coordination harder to audit. */ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import type { OpenFile } from '@/store/slices/editor' -import { - getConnectionId, - getConnectionIdForFile, - isWorktreeConnectionResolved -} from '@/lib/connection-context' +import { getConnectionIdForFile, isWorktreeConnectionResolved } from '@/lib/connection-context' import { joinPath } from '@/lib/path' import { useAppStore } from '@/store' import { getDiskBaselineSignature } from './diff-content-signature' @@ -224,7 +220,7 @@ export function useEditorPanelContentState({ ? file.branchCompare : null const commitCompare = file.commitCompare?.commitOid ? file.commitCompare : null - const connectionId = getConnectionId(file.worktreeId) ?? undefined + const connectionId = getConnectionIdForFile(file.worktreeId, file.filePath) ?? undefined const activeSettings = useAppStore.getState().settings const fileSettings = settingsForRuntimeOwner(activeSettings, file.runtimeEnvironmentId) const gitScope = getRuntimeGitScope(fileSettings, connectionId) diff --git a/src/renderer/src/lib/connection-context.test.ts b/src/renderer/src/lib/connection-context.test.ts index bef67faf05..48d27ecf1e 100644 --- a/src/renderer/src/lib/connection-context.test.ts +++ b/src/renderer/src/lib/connection-context.test.ts @@ -256,6 +256,46 @@ describe('getConnectionId', () => { expect(getConnectionIdForFile(workspaceKey, '/home/neil/platform/README.md')).toBeUndefined() }) + it('resolves folder workspace combined diff sections by child repo path', () => { + const workspaceKey = folderWorkspaceKey('folder-workspace-1') + useAppStore.setState({ + folderWorkspaces: [makeFolderWorkspace()], + projectGroups: [ + { + id: 'group-1', + name: 'Platform', + parentPath: '/home/neil/platform', + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } + ], + repos: [ + makeRepo({ + id: 'repo-local', + path: '/home/neil/platform/web', + projectGroupId: 'group-1' + }), + makeRepo({ + id: 'repo-ssh', + path: '/home/neil/platform/api', + projectGroupId: 'group-1', + connectionId: 'ssh-1' + }) + ], + worktreesByRepo: {} + }) + + expect(getConnectionIdForFile(workspaceKey, '/home/neil/platform')).toBeUndefined() + expect(getConnectionIdForFile(workspaceKey, '/home/neil/platform/api/src/index.ts')).toBe( + 'ssh-1' + ) + }) + it('keeps explicit folder workspace provenance isolated from unrelated same-path SSH repos', () => { useAppStore.setState({ folderWorkspaces: [ From c5669cceb5c3d0a086dba06df9328bf46d35f434 Mon Sep 17 00:00:00 2001 From: BingZ Date: Fri, 10 Jul 2026 18:06:44 +0800 Subject: [PATCH 08/10] fix(quick-open): prune generated dirs in git fallback (#7842) * fix(quick-open): prune generated dirs in git fallback * fix(source-control): narrow manual review provider switch * fix(quick-open): collapse git fallback directories Co-authored-by: Orca * fix(quick-open): harden collapsed git directory expansion Co-authored-by: Orca * fix(quick-open): close directory expansion races Co-authored-by: Orca * perf(quick-open): collapse placeholders efficiently Co-authored-by: Orca * fix(quick-open): preserve root and cancellation semantics Co-authored-by: Orca --------- Co-authored-by: Jinwoo-H Co-authored-by: Orca --- ...list-files-git-directory-expansion.test.ts | 103 +++++++ .../ipc/filesystem-list-files-git-fallback.ts | 71 ++++- src/main/ipc/filesystem-list-files.test.ts | 3 + src/main/ipc/filesystem-list-files.ts | 5 +- src/main/ipc/filesystem.ts | 5 +- src/relay/fs-handler-git-fallback.ts | 19 +- .../fs-handler-list-files-ignored.test.ts | 22 +- src/shared/quick-open-directory-validation.ts | 7 + src/shared/quick-open-expansion-paths.test.ts | 26 ++ src/shared/quick-open-expansion-paths.ts | 37 +++ src/shared/quick-open-filter.test.ts | 31 +- src/shared/quick-open-filter.ts | 18 +- .../quick-open-git-directory-collapse.test.ts | 58 ++++ src/shared/quick-open-readdir-walk.test.ts | 269 +++++++++++++++++- src/shared/quick-open-readdir-walk.ts | 206 ++++++++++---- 15 files changed, 778 insertions(+), 102 deletions(-) create mode 100644 src/main/ipc/filesystem-list-files-git-directory-expansion.test.ts create mode 100644 src/shared/quick-open-directory-validation.ts create mode 100644 src/shared/quick-open-expansion-paths.test.ts create mode 100644 src/shared/quick-open-expansion-paths.ts create mode 100644 src/shared/quick-open-git-directory-collapse.test.ts diff --git a/src/main/ipc/filesystem-list-files-git-directory-expansion.test.ts b/src/main/ipc/filesystem-list-files-git-directory-expansion.test.ts new file mode 100644 index 0000000000..5d5cf16374 --- /dev/null +++ b/src/main/ipc/filesystem-list-files-git-directory-expansion.test.ts @@ -0,0 +1,103 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const { gitSpawnMock } = vi.hoisted(() => ({ + gitSpawnMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ + gitSpawn: gitSpawnMock +})) + +import { listFilesWithGit } from './filesystem-list-files-git-fallback' +import { isFileListingCancellation } from '../../shared/file-listing-cancellation' + +const tempDirs: string[] = [] +const SHA1 = '0123456789abcdef0123456789abcdef01234567' + +function createMockProcess(): ChildProcess { + const process = new EventEmitter() as unknown as ChildProcess + ;(process as unknown as Record).stdout = new EventEmitter() + ;( + (process as unknown as Record).stdout as EventEmitter & { + setEncoding: () => void + } + ).setEncoding = vi.fn() + ;(process as unknown as Record).stderr = new EventEmitter() + ;(process as unknown as Record).kill = vi.fn() + ;(process as unknown as Record).exitCode = null + ;(process as unknown as Record).signalCode = null + return process +} + +async function writeRel(root: string, relPath: string): Promise { + const absPath = join(root, ...relPath.split('/')) + await mkdir(dirname(absPath), { recursive: true }) + await writeFile(absPath, 'x') +} + +afterEach(async () => { + vi.clearAllMocks() + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +describe('main Quick Open git directory expansion', () => { + it('expands placeholders emitted by both directory-collapsing passes', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-main-git-ignored-dir-')) + tempDirs.push(root) + await writeRel(root, 'dist/generated.js') + await writeRel(root, 'scratch/notes.txt') + + const revParse = createMockProcess() + const primary = createMockProcess() + const ignored = createMockProcess() + gitSpawnMock + .mockReturnValueOnce(revParse) + .mockReturnValueOnce(primary) + .mockReturnValueOnce(ignored) + + const promise = listFilesWithGit(root, [], {}) + revParse.emit('close', 0, null) + await vi.waitFor(() => expect(gitSpawnMock).toHaveBeenCalledTimes(3)) + ;(primary.stdout as unknown as EventEmitter).emit( + 'data', + `100644 ${SHA1} 0\tsrc/index.ts\0scratch/\0` + ) + primary.emit('close', 0, null) + ;(ignored.stdout as unknown as EventEmitter).emit('data', 'dist/\0') + ignored.emit('close', 0, null) + + await expect(promise).resolves.toEqual([ + 'dist/generated.js', + 'scratch/notes.txt', + 'src/index.ts' + ]) + expect(gitSpawnMock.mock.calls[2][0]).toContain('--directory') + }) + + it('cancels both local Git passes when Quick Open abandons the request', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-main-git-cancel-')) + tempDirs.push(root) + const revParse = createMockProcess() + const primary = createMockProcess() + const ignored = createMockProcess() + gitSpawnMock + .mockReturnValueOnce(revParse) + .mockReturnValueOnce(primary) + .mockReturnValueOnce(ignored) + + const controller = new AbortController() + const promise = listFilesWithGit(root, [], {}, controller.signal) + revParse.emit('close', 0, null) + await vi.waitFor(() => expect(gitSpawnMock).toHaveBeenCalledTimes(3)) + controller.abort() + + await expect(promise).rejects.toSatisfy(isFileListingCancellation) + expect(primary.kill).toHaveBeenCalled() + expect(ignored.kill).toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/filesystem-list-files-git-fallback.ts b/src/main/ipc/filesystem-list-files-git-fallback.ts index 3b77ac2813..8b5bb526f8 100644 --- a/src/main/ipc/filesystem-list-files-git-fallback.ts +++ b/src/main/ipc/filesystem-list-files-git-fallback.ts @@ -3,9 +3,10 @@ import { gitSpawn } from '../git/runner' import { buildGitLsFilesArgsForQuickOpen } from '../../shared/quick-open-filter' import { createQuickOpenReaddirBudget, - expandQuickOpenGitFilesWithNestedRepos, + expandQuickOpenGitFileListing, listQuickOpenFilesWithReaddir } from '../../shared/quick-open-readdir-walk' +import { fileListingCancellationError } from '../../shared/file-listing-cancellation' /** * Fallback file lister using git ls-files. Used when rg is not available. @@ -16,9 +17,13 @@ import { */ async function isInsideGitWorkTree( rootPath: string, - localGitOptions: { wslDistro?: string } + localGitOptions: { wslDistro?: string }, + signal?: AbortSignal ): Promise { - return new Promise((resolve) => { + if (signal?.aborted) { + throw fileListingCancellationError(signal) + } + return new Promise((resolve, reject) => { const child = gitSpawn(['rev-parse', '--is-inside-work-tree'], { cwd: rootPath, ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), @@ -26,22 +31,37 @@ async function isInsideGitWorkTree( }) let done = false let timer: ReturnType + const cleanup = (): void => { + clearTimeout(timer) + child.off('error', handleError) + child.off('close', handleClose) + signal?.removeEventListener('abort', handleAbort) + } const finish = (isGitRepo: boolean): void => { if (done) { return } done = true - clearTimeout(timer) - child.off('error', handleError) - child.off('close', handleClose) + cleanup() resolve(isGitRepo) } + const cancel = (): void => { + if (done) { + return + } + done = true + child.kill() + cleanup() + reject(fileListingCancellationError(signal)) + } const handleError = (): void => finish(false) const handleClose = (code: number | null, signal: NodeJS.Signals | null): void => finish(code === 0 && signal === null) + const handleAbort = (): void => cancel() child.once('error', handleError) child.once('close', handleClose) + signal?.addEventListener('abort', handleAbort, { once: true }) timer = setTimeout(() => { child.kill() finish(false) @@ -52,16 +72,23 @@ async function isInsideGitWorkTree( export async function listFilesWithGit( rootPath: string, excludePathPrefixes: readonly string[], - localGitOptions: { wslDistro?: string } + localGitOptions: { wslDistro?: string }, + signal?: AbortSignal ): Promise { - if (!(await isInsideGitWorkTree(rootPath, localGitOptions))) { + const isGitWorkTree = await isInsideGitWorkTree(rootPath, localGitOptions, signal) + if (signal?.aborted) { + throw fileListingCancellationError(signal) + } + if (!isGitWorkTree) { return listQuickOpenFilesWithReaddir(rootPath, { excludePathPrefixes, - budget: createQuickOpenReaddirBudget() + budget: createQuickOpenReaddirBudget(), + signal }) } const gitPaths = new Set() + const directoryPaths = new Set() const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen(excludePathPrefixes) const children: { child: ChildProcess @@ -78,7 +105,11 @@ export async function listFilesWithGit( if (!path) { return } - gitPaths.add(path) + if (path.endsWith('/')) { + directoryPaths.add(path) + } else { + gitPaths.add(path) + } } // Why: git ls-files outputs paths relative to cwd, so we set cwd to @@ -168,7 +199,7 @@ export async function listFilesWithGit( }) } - const killSurvivors = (): void => { + const killSurvivors = (reason = 'git ls-files canceled after sibling failure'): void => { // Why: Promise.all rejects on the first failed pass; cancel the sibling so // a stuck git process cannot keep scanning after Quick Open has failed. for (const entry of children) { @@ -178,20 +209,32 @@ export async function listFilesWithGit( if (entry.child.exitCode === null && entry.child.signalCode === null) { entry.child.kill() } - entry.reject(new Error('git ls-files canceled after sibling failure')) + entry.reject(new Error(reason)) } } + const onAbort = (): void => killSurvivors('git ls-files cancelled') + signal?.addEventListener('abort', onAbort, { once: true }) try { await Promise.all([runGitLsFiles(primary), runGitLsFiles(ignoredPass)]) } catch (err) { killSurvivors() + if (signal?.aborted) { + throw fileListingCancellationError(signal) + } throw err + } finally { + signal?.removeEventListener('abort', onAbort) } - return expandQuickOpenGitFilesWithNestedRepos({ + const files = await expandQuickOpenGitFileListing({ rootPath, gitPaths, - excludePathPrefixes + directoryPaths, + excludePathPrefixes, + signal }) + // Why: directory placeholders are expanded after Git exits; restore Git's + // path order so empty queries and fuzzy-score ties remain stable. + return files.sort() } diff --git a/src/main/ipc/filesystem-list-files.test.ts b/src/main/ipc/filesystem-list-files.test.ts index 638775a481..a3a556b234 100644 --- a/src/main/ipc/filesystem-list-files.test.ts +++ b/src/main/ipc/filesystem-list-files.test.ts @@ -365,6 +365,9 @@ describe('filesystem-list-files', () => { expect(gitCalls.length).toBe(2) expect(gitCalls[0][1]).toContain('ls-files') expect(gitCalls[0][1]).toContain('-s') + expect(gitCalls[0][1]).toContain('--directory') + expect(gitCalls[1][1]).toContain('--directory') + expect(gitCalls[1][1]).toContain('--no-empty-directory') // Should include valid files and filter node_modules expect(result).toContain('src/index.ts') diff --git a/src/main/ipc/filesystem-list-files.ts b/src/main/ipc/filesystem-list-files.ts index 3637622730..9dcd2c4503 100644 --- a/src/main/ipc/filesystem-list-files.ts +++ b/src/main/ipc/filesystem-list-files.ts @@ -19,7 +19,8 @@ import { listFilesWithGit } from './filesystem-list-files-git-fallback' export async function listQuickOpenFiles( rootPath: string, store: Store, - excludePaths?: string[] + excludePaths?: string[], + signal?: AbortSignal ): Promise { const authorizedRootPath = await resolveAuthorizedPath(rootPath, store) const localGitOptions = getLocalGitOptionsForRegisteredWorktree( @@ -40,7 +41,7 @@ export async function listQuickOpenFiles( // can run. const rgAvailable = await checkRgAvailable(authorizedRootPath, localGitOptions.wslDistro) if (!rgAvailable) { - return listFilesWithGit(authorizedRootPath, excludePathPrefixes, localGitOptions) + return listFilesWithGit(authorizedRootPath, excludePathPrefixes, localGitOptions, signal) } const files = new Set() diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 7e7006a375..100245dca8 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -1063,7 +1063,7 @@ export function registerFilesystemHandlers( signal: controller?.signal }) } - return await listQuickOpenFiles(args.rootPath, store, args.excludePaths) + return await listQuickOpenFiles(args.rootPath, store, args.excludePaths, controller?.signal) } finally { if (args.requestToken) { listFilesCancellations.delete(args.requestToken) @@ -1073,8 +1073,7 @@ export function registerFilesystemHandlers( ) ipcMain.handle('fs:cancelListFiles', (_event, args: { requestToken: string }): void => { - // Why: best-effort — the entry is gone once the listing settles, and - // local scans are fast enough to simply let them finish. + // Why: best-effort — the entry is gone once the listing settles. listFilesCancellations.get(args.requestToken)?.abort() }) diff --git a/src/relay/fs-handler-git-fallback.ts b/src/relay/fs-handler-git-fallback.ts index 9e69339bfc..6cceb5de7a 100644 --- a/src/relay/fs-handler-git-fallback.ts +++ b/src/relay/fs-handler-git-fallback.ts @@ -10,7 +10,7 @@ import { spawn } from 'node:child_process' import { fileListingCancellationError } from '../shared/file-listing-cancellation' import type { SearchOptions, SearchResult } from './fs-handler-utils' import { buildGitLsFilesArgsForQuickOpen } from '../shared/quick-open-filter' -import { expandQuickOpenGitFilesWithNestedRepos } from '../shared/quick-open-readdir-walk' +import { expandQuickOpenGitFileListing } from '../shared/quick-open-readdir-walk' import { buildGitGrepArgs, buildSubmatchRegex, @@ -40,6 +40,7 @@ export function listFilesWithGit( return Promise.reject(fileListingCancellationError(signal)) } const gitPaths = new Set() + const directoryPaths = new Set() const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen(excludePathPrefixes) const children: { child: ReturnType @@ -56,7 +57,11 @@ export function listFilesWithGit( if (!path) { return } - gitPaths.add(path) + if (path.endsWith('/')) { + directoryPaths.add(path) + } else { + gitPaths.add(path) + } } const child = spawn('git', ['ls-files', ...args], { @@ -172,14 +177,18 @@ export function listFilesWithGit( signal?.addEventListener('abort', onAbort, { once: true }) return Promise.all([runGitLsFiles(primary), runGitLsFiles(ignoredPass)]) - .then(() => - expandQuickOpenGitFilesWithNestedRepos({ + .then(async () => { + const files = await expandQuickOpenGitFileListing({ rootPath, gitPaths, + directoryPaths, excludePathPrefixes, signal }) - ) + // Why: directory placeholders are expanded after Git exits; restore + // Git's path order for empty queries and fuzzy-score ties over SSH. + return files.sort() + }) .catch((err) => { killSurvivors('git ls-files canceled after sibling failure') if (signal?.aborted) { diff --git a/src/relay/fs-handler-list-files-ignored.test.ts b/src/relay/fs-handler-list-files-ignored.test.ts index 753ce02fdd..5a1804437b 100644 --- a/src/relay/fs-handler-list-files-ignored.test.ts +++ b/src/relay/fs-handler-list-files-ignored.test.ts @@ -98,6 +98,8 @@ describe('relay quick open ignored file listing', () => { }) it('git fallback ignored pass includes ignored non-env files', async () => { + const root = await makeTempRoot() + await writeRel(root, 'dist/generated.js') const primaryProc = createMockProcess() const ignoredProc = createMockProcess() let callIndex = 0 @@ -107,7 +109,7 @@ describe('relay quick open ignored file listing', () => { return callIndex === 1 ? primaryProc : ignoredProc }) - const promise = listFilesWithGit('/remote/root', ['packages/other']) + const promise = listFilesWithGit(root, ['packages/other']) setTimeout(() => { ;(primaryProc.stdout as unknown as EventEmitter).emit( @@ -120,26 +122,28 @@ describe('relay quick open ignored file listing', () => { ) primaryProc.emit('close', 0, null) - ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'dist/generated.js\0') + ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'dist/\0') ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'packages/other/src/x.ts\0') ignoredProc.emit('close', 0, null) }, 10) - await expect(promise).resolves.toEqual(['src/index.ts', 'tab\tfile.txt', 'dist/generated.js']) + await expect(promise).resolves.toEqual(['dist/generated.js', 'src/index.ts', 'tab\tfile.txt']) const ignoredArgs = spawnMock.mock.calls[1][1] as string[] - expect(ignoredArgs).toEqual([ + expect(ignoredArgs.slice(0, 6)).toEqual([ 'ls-files', '-z', '-s', '--others', '--ignored', - '--exclude-standard', - '--', - '.', - ':(exclude,glob)packages/other', - ':(exclude,glob)packages/other/**' + '--exclude-standard' ]) + expect(ignoredArgs).toContain('--') + expect(ignoredArgs).toContain('.') + expect(ignoredArgs).toContain('--directory') + expect(ignoredArgs).toContain('--no-empty-directory') + expect(ignoredArgs).toContain(':(exclude,glob)packages/other') + expect(ignoredArgs).toContain(':(exclude,glob)packages/other/**') }) it('git fallback fills nested git repos returned as root-relative placeholders', async () => { diff --git a/src/shared/quick-open-directory-validation.ts b/src/shared/quick-open-directory-validation.ts new file mode 100644 index 0000000000..501791bb05 --- /dev/null +++ b/src/shared/quick-open-directory-validation.ts @@ -0,0 +1,7 @@ +import type { Stats } from 'node:fs' + +export function isQuickOpenReadableDirectory(stat: Stats, allowSymlinkedRoot = false): boolean { + // Why: an explicitly selected workspace root may be a symlink, while nested + // traversal must never follow a symlink outside that authorized root. + return stat.isDirectory() || Boolean(allowSymlinkedRoot && stat.isSymbolicLink()) +} diff --git a/src/shared/quick-open-expansion-paths.test.ts b/src/shared/quick-open-expansion-paths.test.ts new file mode 100644 index 0000000000..e5e91e9b40 --- /dev/null +++ b/src/shared/quick-open-expansion-paths.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest' +import { collapseQuickOpenExpansionPaths } from './quick-open-expansion-paths' + +describe('collapseQuickOpenExpansionPaths', () => { + it('collapses descendants without confusing shared-prefix siblings', () => { + const paths = new Map([ + ['foo/bar/deep', false], + ['foo-sibling', false], + ['foo', false], + ['foo/bar', true] + ]) + + expect(collapseQuickOpenExpansionPaths(paths)).toEqual([ + ['foo', true], + ['foo-sibling', false] + ]) + }) + + it('keeps many sibling placeholders distinct', () => { + const paths = new Map( + Array.from({ length: 10_001 }, (_, index) => [`generated-${index}`, true] as const) + ) + + expect(collapseQuickOpenExpansionPaths(paths)).toHaveLength(paths.size) + }) +}) diff --git a/src/shared/quick-open-expansion-paths.ts b/src/shared/quick-open-expansion-paths.ts new file mode 100644 index 0000000000..43f094edd3 --- /dev/null +++ b/src/shared/quick-open-expansion-paths.ts @@ -0,0 +1,37 @@ +/** + * Remove descendant placeholders already covered by an ancestor. Sorting puts + * ancestors first; prefix lookups avoid quadratic scans across sibling paths. + */ +export function collapseQuickOpenExpansionPaths( + expansionPaths: ReadonlyMap +): [string, boolean][] { + const sortedPaths = Array.from(expansionPaths).sort(([left], [right]) => + left < right ? -1 : left > right ? 1 : 0 + ) + const collapsedPaths = new Map() + + for (const [relPath, includeSymlinks] of sortedPaths) { + let ancestorPath: string | undefined + let slashIndex = relPath.indexOf('/') + while (slashIndex !== -1) { + const candidate = relPath.substring(0, slashIndex) + if (collapsedPaths.has(candidate)) { + ancestorPath = candidate + break + } + slashIndex = relPath.indexOf('/', slashIndex + 1) + } + + if (ancestorPath) { + // Why: primary and ignored passes can overlap; the ancestor walk covers + // the descendant, but must preserve either pass's symlink-leaf contract. + if (includeSymlinks) { + collapsedPaths.set(ancestorPath, true) + } + continue + } + collapsedPaths.set(relPath, includeSymlinks) + } + + return Array.from(collapsedPaths) +} diff --git a/src/shared/quick-open-filter.test.ts b/src/shared/quick-open-filter.test.ts index 2b079f6b7d..1d96bc1744 100644 --- a/src/shared/quick-open-filter.test.ts +++ b/src/shared/quick-open-filter.test.ts @@ -85,6 +85,9 @@ describe('buildExcludePathPrefixes', () => { expect(buildExcludePathPrefixes('C:\\repo', ['C:\\repo\\packages\\app'])).toEqual([ 'packages/app' ]) + expect( + buildExcludePathPrefixes('//Server/Share/Repo', ['//server/share/repo/packages/app']) + ).toEqual(['packages/app']) }) it('strips trailing slashes', () => { @@ -239,16 +242,40 @@ describe('normalizeQuickOpenRgLine', () => { describe('buildGitLsFilesArgsForQuickOpen', () => { it('primary pass is --cached --others --exclude-standard', () => { const { primary } = buildGitLsFilesArgsForQuickOpen() - expect(primary).toEqual(['-z', '-s', '--cached', '--others', '--exclude-standard']) + expect(primary).toEqual([ + '-z', + '-s', + '--cached', + '--others', + '--exclude-standard', + '--directory', + '--no-empty-directory' + ]) }) it('ignored pass surfaces ignored files without .env* pathspec whitelist', () => { const { ignoredPass } = buildGitLsFilesArgsForQuickOpen() - expect(ignoredPass).toEqual(['-z', '-s', '--others', '--ignored', '--exclude-standard']) + expect(ignoredPass).toEqual([ + '-z', + '-s', + '--others', + '--ignored', + '--exclude-standard', + '--directory', + '--no-empty-directory' + ]) expect(ignoredPass).not.toContain('.env*') expect(ignoredPass).not.toContain(':(glob)**/.env*') }) + it('collapses untracked directories in both passes without generated pathspec churn', () => { + const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen() + expect(primary).toContain('--directory') + expect(ignoredPass).toContain('--directory') + expect(ignoredPass).toContain('--no-empty-directory') + expect([...primary, ...ignoredPass]).not.toContain(':(exclude,glob)**/node_modules/**') + }) + it('exclude prefixes prepend positive "." pathspec', () => { const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen(['packages/app']) const dashDashIdx = primary.indexOf('--') diff --git a/src/shared/quick-open-filter.ts b/src/shared/quick-open-filter.ts index 980c43351a..899d13aeaa 100644 --- a/src/shared/quick-open-filter.ts +++ b/src/shared/quick-open-filter.ts @@ -104,8 +104,8 @@ function pathFlavor(rootPath: string): typeof posix | typeof win32 { if (/^[a-zA-Z]:[\\/]/.test(rootPath)) { return win32 } - // UNC \\server\share - if (rootPath.startsWith('\\\\')) { + // UNC \\server\share or //server/share + if (rootPath.startsWith('\\\\') || rootPath.startsWith('//')) { return win32 } return posix @@ -376,16 +376,28 @@ export function buildGitLsFilesArgsForQuickOpen( excludeSpecs.push(`:(exclude,glob)${escapeGlobPath(prefix)}/**`) } const trailingPathspecs = excludeSpecs.length > 0 ? ['--', '.', ...excludeSpecs] : [] + // Why: collapse untracked trees before Git traverses them; callers expand + // only allowed directory placeholders with the shared bounded walker. + const directoryCollapseArgs = ['--directory', '--no-empty-directory'] // Why: NUL preserves real Git paths; stage mode identifies gitlinks without // lstat probes for ordinary tracked files. - const primary = ['-z', '-s', '--cached', '--others', '--exclude-standard', ...trailingPathspecs] + const primary = [ + '-z', + '-s', + '--cached', + '--others', + '--exclude-standard', + ...directoryCollapseArgs, + ...trailingPathspecs + ] const ignoredPass = [ '-z', '-s', '--others', '--ignored', '--exclude-standard', + ...directoryCollapseArgs, ...trailingPathspecs ] return { primary, ignoredPass } diff --git a/src/shared/quick-open-git-directory-collapse.test.ts b/src/shared/quick-open-git-directory-collapse.test.ts new file mode 100644 index 0000000000..8e197a6d60 --- /dev/null +++ b/src/shared/quick-open-git-directory-collapse.test.ts @@ -0,0 +1,58 @@ +import { execFile } from 'node:child_process' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { promisify } from 'node:util' +import { afterEach, describe, expect, it } from 'vitest' +import { buildGitLsFilesArgsForQuickOpen } from './quick-open-filter' + +const execFileAsync = promisify(execFile) +const tempDirs: string[] = [] + +async function writeRel(root: string, relPath: string): Promise { + const absPath = join(root, ...relPath.split('/')) + await mkdir(dirname(absPath), { recursive: true }) + await writeFile(absPath, 'x') +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +describe('Quick Open git directory collapse', () => { + it('collapses unignored and ignored trees while preserving individual ignored files', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-quick-open-git-collapse-')) + tempDirs.push(root) + await execFileAsync('git', ['init', '--quiet'], { cwd: root }) + await writeFile( + join(root, '.gitignore'), + ['.cache/', '.local/share/', 'dist/', '*.log'].join('\n') + ) + await Promise.all([ + ...Array.from({ length: 200 }, (_, index) => + writeRel(root, `node_modules/pkg/file-${index}.js`) + ), + writeRel(root, '.cache/state.json'), + writeRel(root, '.local/share/state.json'), + writeRel(root, 'dist/generated.js'), + writeRel(root, 'debug.log') + ]) + + const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen() + const [primaryResult, ignoredResult] = await Promise.all( + [primary, ignoredPass].map((args) => + execFileAsync('git', ['-c', 'core.excludesFile=', 'ls-files', ...args], { + cwd: root, + encoding: 'buffer' + }) + ) + ) + const primaryPaths = primaryResult.stdout.toString().split('\0').filter(Boolean) + const ignoredPaths = ignoredResult.stdout.toString().split('\0').filter(Boolean) + + expect(primaryPaths).toEqual(['.gitignore', 'node_modules/']) + expect(primaryPaths.some((path) => path.startsWith('node_modules/pkg/'))).toBe(false) + expect(ignoredPaths).toEqual(['.cache/', '.local/', 'debug.log', 'dist/']) + expect(ignoredPaths.some((path) => path.startsWith('.local/share/'))).toBe(false) + }) +}) diff --git a/src/shared/quick-open-readdir-walk.test.ts b/src/shared/quick-open-readdir-walk.test.ts index cb73572bc3..2331bc3b1c 100644 --- a/src/shared/quick-open-readdir-walk.test.ts +++ b/src/shared/quick-open-readdir-walk.test.ts @@ -1,28 +1,32 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -const { lstatMock } = vi.hoisted(() => ({ - lstatMock: vi.fn() +const { lstatMock, readdirMock } = vi.hoisted(() => ({ + lstatMock: vi.fn(), + readdirMock: vi.fn() })) vi.mock('fs/promises', async () => { const actual = await vi.importActual('fs/promises') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() lstatMock.mockImplementation(actual.lstat) + readdirMock.mockImplementation(actual.readdir) return { ...actual, - lstat: lstatMock + lstat: lstatMock, + readdir: readdirMock } }) -import { mkdtemp, mkdir, rm, symlink, writeFile } from 'node:fs/promises' +import { mkdtemp, mkdir, rename, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { classifyQuickOpenGitEntry, createQuickOpenReaddirBudget, - expandQuickOpenGitFilesWithNestedRepos, + expandQuickOpenGitFileListing, isQuickOpenReaddirBudgetError, listQuickOpenFilesWithReaddir, - parseQuickOpenGitLsFilesEntry + parseQuickOpenGitLsFilesEntry, + QUICK_OPEN_READDIR_MAX_FILES } from './quick-open-readdir-walk' import { isFileListingCancellation } from './file-listing-cancellation' @@ -94,7 +98,7 @@ describe('quick-open readdir walk', () => { it('keeps ordinary git entries without lstat calls', async () => { await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: '/unused/root', gitPaths: [ staged('100644', 'README.md'), @@ -152,7 +156,7 @@ describe('quick-open readdir walk', () => { await writeRel(root, 'packages/lib/src/lib.ts') await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: root, gitPaths: [ staged('100644', 'README.md'), @@ -180,7 +184,7 @@ describe('quick-open readdir walk', () => { await writeRel(root, 'packages/lib/c.ts') await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: root, gitPaths: [staged('160000', 'packages/app'), staged('160000', 'packages/lib')], budget: createQuickOpenReaddirBudget({ maxFiles: 2 }) @@ -199,7 +203,7 @@ describe('quick-open readdir walk', () => { } await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: root, gitPaths: [staged('160000', 'packages/app')], excludePathPrefixes: ['packages/app/excluded'], @@ -208,6 +212,209 @@ describe('quick-open readdir walk', () => { ).resolves.toEqual(['packages/app/keep.ts']) }) + it('expands allowed ignored directories without walking blocked or excluded directories', async () => { + const root = await makeTempRoot() + await writeRel(root, 'dist/generated.js') + await writeRel(root, 'node_modules/pkg/index.js') + await writeRel(root, '.cache/state.json') + await writeRel(root, '.local/share/state.json') + await writeRel(root, '.local/config.toml') + await writeRel(root, 'excluded/other.js') + + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: [ + 'dist/', + '.local/', + 'node_modules/', + '.cache/', + '.local/share/', + 'excluded/' + ], + excludePathPrefixes: ['excluded'], + budget: createQuickOpenReaddirBudget({ maxFiles: 2 }) + }) + ).resolves.toEqual(['.local/config.toml', 'dist/generated.js']) + + const walkedPaths = readdirMock.mock.calls.map(([path]) => path) + expect(walkedPaths).toContain(join(root, 'dist')) + expect(walkedPaths).toContain(join(root, '.local')) + expect(walkedPaths).not.toContain(join(root, '.local', 'share')) + }) + + it('batches many allowed directory placeholders with bounded concurrency', async () => { + const root = await makeTempRoot() + const directoryPaths = Array.from({ length: 40 }, (_, index) => `generated-${index}/`) + await Promise.all( + directoryPaths.map((directoryPath, index) => + writeRel(root, `${directoryPath}file-${index}.ts`) + ) + ) + + const actual = await vi.importActual('node:fs/promises') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() + let activeReads = 0 + let maxActiveReads = 0 + readdirMock.mockImplementation(async (...args: Parameters) => { + activeReads++ + maxActiveReads = Math.max(maxActiveReads, activeReads) + await new Promise((resolve) => setTimeout(resolve, 5)) + try { + return await actual.readdir(...args) + } finally { + activeReads-- + } + }) + + try { + const files = await expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths + }) + expect(files).toHaveLength(directoryPaths.length) + expect(maxActiveReads).toBeGreaterThan(1) + expect(maxActiveReads).toBeLessThanOrEqual(32) + } finally { + readdirMock.mockImplementation(actual.readdir) + } + }) + + it('preserves symlink leaves from collapsed Git directories without following them', async () => { + const root = await makeTempRoot() + await mkdirRel(root, 'scratch') + await writeRel(root, 'target/file.ts') + + try { + await symlink(join(root, 'target', 'file.ts'), join(root, 'scratch', 'link.ts')) + await symlink(join(root, 'target'), join(root, 'scratch', 'linked-dir'), 'dir') + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EPERM') { + return + } + throw err + } + + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['scratch/'] + }) + ).resolves.toEqual(['scratch/link.ts', 'scratch/linked-dir']) + }) + + it('does not follow a collapsed directory replaced by a symlink', async () => { + const root = await makeTempRoot() + const outsideRoot = await makeTempRoot() + await writeRel(outsideRoot, 'secret.ts') + + try { + await symlink(outsideRoot, join(root, 'dist'), 'dir') + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EPERM') { + return + } + throw err + } + + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['dist/'] + }) + ).resolves.toEqual([]) + }) + + it('discards entries when a collapsed directory changes during readdir', async () => { + const root = await makeTempRoot() + const outsideRoot = await makeTempRoot() + await mkdirRel(root, 'dist') + await writeRel(outsideRoot, 'secret.ts') + const actual = await vi.importActual('node:fs/promises') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() + const distPath = join(root, 'dist') + let swapped = false + readdirMock.mockImplementation(async (...args: Parameters) => { + if (!swapped && args[0] === distPath) { + swapped = true + await rename(distPath, join(root, 'old-dist')) + await symlink(outsideRoot, distPath, 'dir') + } + return actual.readdir(...args) + }) + + try { + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['dist/'] + }) + ).resolves.toEqual([]) + } finally { + readdirMock.mockImplementation(actual.readdir) + } + }) + + it('walks overlapping primary and ignored placeholders only once', async () => { + const root = await makeTempRoot() + await writeRel(root, 'foo/a.ts') + await writeRel(root, 'foo/bar/b.ts') + + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['foo/', 'foo/bar/'], + budget: createQuickOpenReaddirBudget({ maxFiles: 2 }) + }) + ).resolves.toEqual(['foo/a.ts', 'foo/bar/b.ts']) + + expect( + readdirMock.mock.calls.filter(([path]) => path === join(root, 'foo', 'bar')) + ).toHaveLength(1) + }) + + it('rejects instead of returning a partial ignored-directory expansion', async () => { + const root = await makeTempRoot() + await writeRel(root, 'dist/a.js') + await writeRel(root, 'dist/b.js') + + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['dist/'], + budget: createQuickOpenReaddirBudget({ maxFiles: 1 }) + }) + ).rejects.toThrow('File listing exceeded') + }) + + it('keeps the default safety cap for a very large collapsed directory', async () => { + const root = await makeTempRoot() + await mkdirRel(root, 'dist') + readdirMock.mockResolvedValueOnce( + Array.from({ length: QUICK_OPEN_READDIR_MAX_FILES + 1 }, (_, index) => ({ + name: `file-${index}.ts`, + isDirectory: () => false, + isFile: () => true, + isSymbolicLink: () => false + })) + ) + + // Why: directory collapse prevents generated trees from flooding the relay; + // the Git fallback must reject rather than silently return a partial list. + await expect( + expandQuickOpenGitFileListing({ + rootPath: root, + gitPaths: [], + directoryPaths: ['dist/'] + }) + ).rejects.toThrow('File listing exceeded 10000 files') + }) + it('identifies budget errors so callers can translate only those to install-rg guidance', () => { expect(isQuickOpenReaddirBudgetError(new Error('File listing timed out'))).toBe(true) expect(isQuickOpenReaddirBudgetError(new Error('File listing exceeded 10000 files'))).toBe(true) @@ -248,13 +455,31 @@ describe('quick-open readdir walk', () => { expect(files).not.toContain('linked-dir/file.ts') }) + it('walks an explicitly selected symlinked workspace root', async () => { + const targetRoot = await makeTempRoot() + const linkContainer = await makeTempRoot() + await writeRel(targetRoot, 'src/index.ts') + const linkedRoot = join(linkContainer, 'linked-workspace') + + try { + await symlink(targetRoot, linkedRoot, 'dir') + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EPERM') { + return + } + throw err + } + + await expect(listQuickOpenFilesWithReaddir(linkedRoot)).resolves.toEqual(['src/index.ts']) + }) + it('fills nested repo paths containing spaces and glob metacharacters', async () => { const root = await makeTempRoot() await makeNestedRepo(root, 'packages/app [one] space') await writeRel(root, 'packages/app [one] space/src/main.ts') await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: root, gitPaths: ['packages/app [one] space/'] }) @@ -276,7 +501,7 @@ describe('quick-open readdir walk', () => { await rejection.catch((err) => expect(isQuickOpenReaddirBudgetError(err)).toBe(false)) }) - it('stops nested-repo expansion when the signal aborts (#7721)', async () => { + it('stops ignored-directory expansion when the signal aborts (#7721)', async () => { const root = await makeTempRoot() await writeRel(root, 'src/kept.ts') @@ -284,11 +509,27 @@ describe('quick-open readdir walk', () => { controller.abort() await expect( - expandQuickOpenGitFilesWithNestedRepos({ + expandQuickOpenGitFileListing({ rootPath: root, - gitPaths: ['src/kept.ts'], + gitPaths: [], + directoryPaths: ['src/'], signal: controller.signal }) ).rejects.toSatisfy(isFileListingCancellation) }) + + it('rejects when cancellation lands during an empty readdir batch', async () => { + const root = await makeTempRoot() + const controller = new AbortController() + const actual = await vi.importActual('node:fs/promises') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() + readdirMock.mockImplementationOnce(async (...args: Parameters) => { + const entries = await actual.readdir(...args) + controller.abort() + return entries + }) + + await expect( + listQuickOpenFilesWithReaddir(root, { signal: controller.signal }) + ).rejects.toSatisfy(isFileListingCancellation) + }) }) diff --git a/src/shared/quick-open-readdir-walk.ts b/src/shared/quick-open-readdir-walk.ts index 90ce3ea9fd..ada9442cd2 100644 --- a/src/shared/quick-open-readdir-walk.ts +++ b/src/shared/quick-open-readdir-walk.ts @@ -1,6 +1,8 @@ import { lstat, readdir } from 'node:fs/promises' import { join, relative } from 'node:path' import { throwIfFileListingCancelled } from './file-listing-cancellation' +import { isQuickOpenReadableDirectory } from './quick-open-directory-validation' +import { collapseQuickOpenExpansionPaths } from './quick-open-expansion-paths' import { HIDDEN_DIR_BLOCKLIST, shouldExcludeQuickOpenRelPath, @@ -9,6 +11,7 @@ import { export const QUICK_OPEN_READDIR_MAX_FILES = 10_000 export const QUICK_OPEN_READDIR_TIMEOUT_MS = 10_000 +const QUICK_OPEN_READDIR_CONCURRENCY = 32 export type QuickOpenReaddirBudget = { remainingFiles: number @@ -96,13 +99,13 @@ function normalizeGitEntry(entry: string): string { } // Translate workspace-root-relative exclude prefixes into prefixes relative to -// a nested repo at `nestedRelPath`, so the nested walk can prune them during -// traversal. Prefixes outside the nested repo are dropped (they cannot match). -function rebaseExcludePrefixesForNestedRepo( +// one expanded subtree, so its walk prunes them during traversal. Prefixes +// outside that subtree are dropped because they cannot match. +function rebaseExcludePrefixesForSubtree( excludePathPrefixes: readonly string[], - nestedRelPath: string + subtreeRelPath: string ): string[] { - const base = `${nestedRelPath}/` + const base = `${subtreeRelPath}/` const rebased: string[] = [] for (const prefix of excludePathPrefixes) { if (prefix.startsWith(base)) { @@ -157,59 +160,132 @@ export async function listQuickOpenFilesWithReaddir( rootPath: string, opts: { excludePathPrefixes?: readonly string[] + workspaceRelPathPrefix?: string budget?: QuickOpenReaddirBudget signal?: AbortSignal } = {} ): Promise { - const files: string[] = [] - const budget = opts.budget ?? createQuickOpenReaddirBudget() - const excludePathPrefixes = opts.excludePathPrefixes ?? [] - - async function walk(dir: string): Promise { - // Why: an abandoned scan (workspace switch) must stop consuming IO and - // event-loop time on the single-threaded relay, not just run to budget. - throwIfFileListingCancelled(opts.signal) - assertWithinDeadline(budget) + return listQuickOpenFilesFromRoots( + [ + { + rootPath, + excludePathPrefixes: opts.excludePathPrefixes ?? [], + workspaceRelPathPrefix: opts.workspaceRelPathPrefix, + allowRootSymlink: true + } + ], + opts.budget ?? createQuickOpenReaddirBudget(), + opts.signal + ) +} - let entries - try { - entries = await readdir(dir, { withFileTypes: true }) - } catch { - // Why: permission denied on an individual subtree is common for broad - // roots. Skipping that subtree preserves the existing relay fallback. - return - } +type QuickOpenReaddirRoot = { + rootPath: string + excludePathPrefixes: readonly string[] + workspaceRelPathPrefix?: string + outputPathPrefix?: string + includeSymlinks?: boolean + allowRootSymlink?: boolean +} - for (const entry of entries) { - throwIfFileListingCancelled(opts.signal) +async function listQuickOpenFilesFromRoots( + roots: readonly QuickOpenReaddirRoot[], + budget: QuickOpenReaddirBudget, + signal?: AbortSignal +): Promise { + const files: string[] = [] + let pendingDirectories = roots.map((root) => ({ + root, + absPath: root.rootPath, + isRoot: true + })) + + while (pendingDirectories.length > 0) { + const nextDirectories: typeof pendingDirectories = [] + for ( + let offset = 0; + offset < pendingDirectories.length; + offset += QUICK_OPEN_READDIR_CONCURRENCY + ) { + // Why: batch only the filesystem calls. Result processing stays serial, + // so the shared cap remains exact while shallow placeholder-heavy repos + // do not pay one relay event-loop turn per directory. + throwIfFileListingCancelled(signal) + assertWithinDeadline(budget) + const batch = pendingDirectories.slice(offset, offset + QUICK_OPEN_READDIR_CONCURRENCY) + const entryGroups = await Promise.all( + batch.map(async (pending) => { + try { + // Why: Git's placeholder may have been replaced with a symlink + // before expansion. Never let readdir follow it outside the root. + const stat = await lstat(pending.absPath) + const allowSymlinkedRoot = pending.isRoot && pending.root.allowRootSymlink + if (!isQuickOpenReadableDirectory(stat, allowSymlinkedRoot)) { + return { pending, entries: [] } + } + const entries = await readdir(pending.absPath, { withFileTypes: true }) + // Why: close the ordinary check/use race. If the directory became + // a symlink while readdir was pending, discard everything read. + const statAfterRead = await lstat(pending.absPath) + if (!isQuickOpenReadableDirectory(statAfterRead, allowSymlinkedRoot)) { + return { pending, entries: [] } + } + return { pending, entries } + } catch { + // Why: permission denied on one subtree is common for broad roots. + return { pending, entries: [] } + } + }) + ) + // Why: an empty directory has no per-entry checkpoint below. Cancellation + // or timeout that lands during readdir must still reject, never resolve []. + throwIfFileListingCancelled(signal) assertWithinDeadline(budget) - const name = entry.name - const absPath = join(dir, name) - const relPath = toRelPath(rootPath, absPath) - if (shouldExcludeQuickOpenRelPath(relPath, excludePathPrefixes)) { - continue - } - if (entry.isDirectory()) { - if (shouldDescend(name)) { - await walk(absPath) + for (const { pending, entries } of entryGroups) { + for (const entry of entries) { + throwIfFileListingCancelled(signal) + assertWithinDeadline(budget) + + const name = entry.name + const absPath = join(pending.absPath, name) + const relPath = toRelPath(pending.root.rootPath, absPath) + const workspaceRelPath = pending.root.workspaceRelPathPrefix + ? `${pending.root.workspaceRelPathPrefix}/${relPath}` + : relPath + if (shouldExcludeQuickOpenRelPath(relPath, pending.root.excludePathPrefixes)) { + continue + } + if (entry.isDirectory()) { + if (shouldDescend(name) && shouldIncludeQuickOpenPath(workspaceRelPath)) { + nextDirectories.push({ root: pending.root, absPath, isRoot: false }) + } + continue + } + if ( + (entry.isFile() || (pending.root.includeSymlinks && entry.isSymbolicLink())) && + shouldIncludeQuickOpenPath(workspaceRelPath) + ) { + consumeFileBudget(budget) + files.push( + pending.root.outputPathPrefix + ? `${pending.root.outputPathPrefix}/${relPath}` + : relPath + ) + } } - continue - } - if (entry.isFile()) { - consumeFileBudget(budget) - files.push(relPath) } } + pendingDirectories = nextDirectories } - await walk(rootPath) return files } -export async function expandQuickOpenGitFilesWithNestedRepos(opts: { +export async function expandQuickOpenGitFileListing(opts: { rootPath: string gitPaths: Iterable + directoryPaths?: Iterable excludePathPrefixes?: readonly string[] budget?: QuickOpenReaddirBudget signal?: AbortSignal @@ -217,6 +293,7 @@ export async function expandQuickOpenGitFilesWithNestedRepos(opts: { const files = new Set() const excludePathPrefixes = opts.excludePathPrefixes ?? [] const budget = opts.budget ?? createQuickOpenReaddirBudget() + const expansionPaths = new Map() const addFinalPath = (relPath: string): void => { if (!relPath) { @@ -243,17 +320,46 @@ export async function expandQuickOpenGitFilesWithNestedRepos(opts: { continue } - const nestedFiles = await listQuickOpenFilesWithReaddir(joinRootRel(opts.rootPath, relPath), { - // Why: exclude prefixes are workspace-root-relative; rebase them onto the - // nested repo so the walk prunes excluded subtrees during traversal - // instead of burning the shared budget and filtering them at the end. - excludePathPrefixes: rebaseExcludePrefixesForNestedRepo(excludePathPrefixes, relPath), - budget, - signal: opts.signal - }) - for (const nestedFile of nestedFiles) { - addFinalPath(`${relPath}/${nestedFile}`) + expansionPaths.set(relPath, expansionPaths.get(relPath) ?? false) + } + + for (const rawPath of opts.directoryPaths ?? []) { + throwIfFileListingCancelled(opts.signal) + assertWithinDeadline(budget) + + const relPath = normalizeGitEntry(rawPath) + // Why: Git intentionally leaves collapsed directories unexpanded; reject + // blocked and nested-worktree placeholders before any filesystem IO. + if ( + !relPath || + shouldExcludeQuickOpenRelPath(relPath, excludePathPrefixes) || + !shouldIncludeQuickOpenPath(relPath) + ) { + continue } + + // Why: before directory collapse, Git returned untracked symlink entries + // without following them. Preserve those paths when expanding placeholders. + expansionPaths.set(relPath, true) + } + + const expandedFiles = await listQuickOpenFilesFromRoots( + collapseQuickOpenExpansionPaths(expansionPaths).map(([relPath, includeSymlinks]) => ({ + rootPath: joinRootRel(opts.rootPath, relPath), + // Why: exclude prefixes are workspace-root-relative; rebase them onto + // each expanded subtree so blocked work is pruned before consuming cap. + excludePathPrefixes: rebaseExcludePrefixesForSubtree(excludePathPrefixes, relPath), + // Why: Git can collapse `.local/share/` to `.local/`; keep workspace + // context so the walker still prunes the multi-segment blocklist. + workspaceRelPathPrefix: relPath, + outputPathPrefix: relPath, + includeSymlinks + })), + budget, + opts.signal + ) + for (const expandedFile of expandedFiles) { + addFinalPath(expandedFile) } return Array.from(files) From 0f0e32952e7c33adbc8d03325c8ea9d241df1bb8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:56:55 -0700 Subject: [PATCH 09/10] Fix layout-aware find in the source editor (#8088) * fix(editor): make find shortcut layout-aware * fix(editor): cover layout-aware find surfaces --- docs/editor-find-layout-aware-shortcut.md | 92 ++++++++ .../src/components/editor/DiffSectionItem.tsx | 11 +- .../src/components/editor/DiffViewer.tsx | 10 +- .../src/components/editor/IpynbViewer.tsx | 12 +- .../src/components/editor/MonacoEditor.tsx | 17 +- .../editor/editor-shortcuts.test.ts | 198 ++++++++++++++++++ .../src/components/editor/editor-shortcuts.ts | 28 +++ 7 files changed, 350 insertions(+), 18 deletions(-) create mode 100644 docs/editor-find-layout-aware-shortcut.md create mode 100644 src/renderer/src/components/editor/editor-shortcuts.test.ts diff --git a/docs/editor-find-layout-aware-shortcut.md b/docs/editor-find-layout-aware-shortcut.md new file mode 100644 index 0000000000..9f75390c17 --- /dev/null +++ b/docs/editor-find-layout-aware-shortcut.md @@ -0,0 +1,92 @@ +# Layout-aware find in editable Monaco editors + +## Problem + +Issue [#7953](https://github.com/stablyai/orca/issues/7953) reports that `Cmd+F` can type `f` into a TypeScript file instead of opening find on macOS. + +Orca's editable Monaco surfaces currently install its layout-aware save shortcut through `editor-shortcuts.ts` (`src/renderer/src/components/editor/editor-shortcuts.ts:18`), but leave find entirely to Monaco's internal keycode dispatch. Orca already declares `editor.find` as `Mod+F` (`src/shared/keybindings.ts:784`) and its matcher resolves logical keys before physical-code fallback (`src/shared/keybindings.ts:2041`). + +A real Electron repro sends a macOS event with logical key `f`, physical code `KeyU`, and virtual key code `U`, as produced by a non-QWERTY layout. Monaco leaves its find widget closed. The equivalent QWERTY `KeyF` event opens it. + +## Root cause + +Monaco's built-in find keybinding follows the physical/virtual keycode delivered by Chromium. Orca's shortcut system is layout-aware, but its editable Monaco integrations do not use it for find. On layouts where the key that produces `f` is not physical `KeyF`, Monaco misses the chord and Native Edit Context remains in editing mode. + +## Non-goals + +- Reimplementing Monaco's find widget, match navigation, or search state. +- Changing find behavior in markdown preview, rich markdown, PDF, browser, terminal, or file search. +- Changing find behavior in read-only diff surfaces. +- Reworking all Monaco keybindings or making Monaco defaults fully obey shortcut unbinding in this patch. +- Adding telemetry for a local keyboard action. + +## Design + +1. Add a focused editor find installer beside the existing save installer in `editor-shortcuts.ts`. It matches `editor.find` through `editorShortcutMatches`, consumes every matched event before it reaches Native Edit Context or Monaco, and invokes a supplied callback only for the initial, non-repeat keydown. Repeat events must still be prevented and propagation-stopped so Monaco's QWERTY binding cannot reopen/reset the widget. +2. Install that handler on every editable Monaco container in the source editor, editable diff views, and notebook code cells. Run that editor's existing `actions.find` action and dispose the bridge from its existing teardown callback. +3. Add unit coverage at the DOM-listener seam for logical `f` with a non-`KeyF` physical code, QWERTY/default behavior, repeat suppression, unrelated typing, and cleanup. +4. Preserve an Electron regression loop that drives both QWERTY and layout-aware raw key events against a real `.ts` editor and verifies the existing Monaco find widget becomes visible without dirtying the file. + +## Data flow + +- macOS/Linux/Windows keydown reaches the focused editable Monaco container. +- `editorShortcutMatches('editor.find', event)` resolves the active platform, user bindings, modifiers, and logical key. +- On match, Orca consumes the DOM event and calls Monaco's existing `actions.find` action. +- Monaco owns the visible find widget and focus exactly as before. + +## Edge cases + +- Auto-repeat must be consumed without invoking find again; returning early before prevention would let Monaco handle a repeated QWERTY `KeyF` event. +- Ordinary unmodified `f` typing and unrelated shortcuts must continue to Monaco unchanged. +- A removed/disposed source editor, diff pane, or notebook cell must not retain the listener. +- QWERTY `Cmd/Ctrl+F` must still open the same Monaco widget once, not twice. +- User-configured bindings accepted by Orca's `editor.find` matcher should open find; Monaco's own default bindings remain outside the scope of this patch. +- The behavior is renderer-local and does not read files or execute commands, so local, SSH, and Remote Orca files share the same path. + +## Test plan + +- Unit: `editor-shortcuts.test.ts` dispatches keyboard events through a real element and parameterizes the shared bridge across macOS (`metaKey`) and Linux/Windows (`ctrlKey`) using logical `f` with physical `KeyU`. It also asserts QWERTY/default handling, matched-repeat prevention without a second callback, unrelated typing, and disposal behavior used by every editable Monaco integration. +- Electron: open a disposable `.ts` file and drive `Cmd+F`/`Ctrl+F` using the platform modifier; assert `.find-widget.visible`, focused find input, unchanged source text, and clean editor state. +- Electron layout regression: on macOS, dispatch logical `f` with non-QWERTY physical/virtual key identity; assert the same find state and unchanged source text. +- Adjacent smoke: dismiss find, type a normal character, and verify it edits the file rather than reopening find. +- Static: run focused Vitest, `pnpm typecheck`, and `pnpm lint`. + +## UI quality bar + +No new UI. The existing Monaco find widget must appear in its current position and styling, focus its input, and leave the editor content unchanged. No overlap, clipping, duplicate widget, or focus flicker is acceptable. + +## Review screenshots + +1. QWERTY/default shortcut with the existing Monaco find widget visible in a `.ts` editor. +2. Non-QWERTY logical-`f` regression path with the same find widget visible and source text unchanged. +3. Adjacent ordinary typing state after find is dismissed, showing the source editor still accepts text normally. + +## Rollout + +1. Add failing unit coverage for the layout-aware find installer contract. +2. Implement the installer in `editor-shortcuts.ts`. +3. Wire it to each editable Monaco surface's existing find action and lifecycle cleanup. +4. Run focused tests, typecheck, lint, and the Electron QWERTY/layout/typing scenarios. + +## Lightweight Eng Review + +- Scope: Kept to one shared shortcut installer and the existing mount/teardown seams for source editors, editable diff panes, and notebook code cells; no new find implementation or global shortcut interception. +- Architecture/data flow: The renderer-local Monaco container owns the keydown. Orca's canonical matcher resolves the logical key, while Monaco continues to own widget state and rendering. No main/preload/IPC, persistence, network, SSH, or provider boundary changes. +- Failure modes covered: + - Non-QWERTY logical key differs from physical/virtual keycode. + - QWERTY double handling. + - Auto-repeat escaping to Monaco's native QWERTY handler. + - Listener surviving editor disposal. + - Ordinary typing being consumed. + - Custom `editor.find` chord accepted by Orca but not Monaco. +- Test coverage required: + - DOM-listener unit tests in `src/renderer/src/components/editor/editor-shortcuts.test.ts`, parameterized for Darwin/Meta, Linux/Ctrl, and Windows/Ctrl. + - Electron-visible QWERTY and layout-aware `.ts` scenarios. + - Adjacent ordinary typing smoke test. +- Performance/blast radius: One capture listener per mounted editable Monaco editor, doing a constant-time keybinding comparison only for events within that editor. Multiple mounted diff sections do not fan out because each listener is scoped to its own container. Listeners are removed with Monaco disposal; no polling, scans, IPC, storage, or render-loop work. +- UI quality bar: Existing Monaco find widget only; verify focus, unchanged source text, no duplicate opening, and unchanged styling against `docs/STYLEGUIDE.md`. +- Required review screenshots: + 1. Default QWERTY find-open state. + 2. Non-QWERTY logical-key find-open state. + 3. Find-dismissed ordinary typing state. +- Residual risks: Monaco's internal default bindings remain active when a user explicitly rebinds `editor.find`; fully suppressing/remapping Monaco's native keybinding table is a separate, larger change. diff --git a/src/renderer/src/components/editor/DiffSectionItem.tsx b/src/renderer/src/components/editor/DiffSectionItem.tsx index 71fe21cf98..ffb1aa36b9 100644 --- a/src/renderer/src/components/editor/DiffSectionItem.tsx +++ b/src/renderer/src/components/editor/DiffSectionItem.tsx @@ -27,7 +27,7 @@ import { DiffSectionHeader } from './DiffSectionHeader' import type { DiffSection } from './diff-section-types' import type { DiffComment } from '../../../../shared/types' import { isDiffComment } from '@/lib/diff-comment-compat' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { DiffSectionBody } from './DiffSectionBody' import { useDiffSectionLayoutMetrics } from './useDiffSectionLayoutMetrics' import { disposeUnattachedMonacoModelPaths } from './diff-monaco-model-disposal' @@ -343,9 +343,12 @@ export function DiffSectionItem({ } modifiedEditorsRef.current.set(index, modified) + const original = editor.getOriginalEditor() const cleanupSaveShortcut = installEditorSaveShortcut(modified.getContainerDomNode(), () => handleSectionSaveRef.current(index) ) + const cleanupOriginalFindShortcut = installMonacoEditorFindShortcut(original) + const cleanupModifiedFindShortcut = installMonacoEditorFindShortcut(modified) const modelContentSub = modified.onDidChangeModelContent(() => { const current = modified.getValue() setSections((prev) => { @@ -380,9 +383,11 @@ export function DiffSectionItem({ }) }) modified.onDidDispose(() => { - // Why: editable diff sections own both the save shortcut and model-change - // subscription for this Monaco editor instance. + // Why: editable diff sections own both panes' shortcut bridges and the + // model subscription for the lifetime of this Monaco diff instance. cleanupSaveShortcut() + cleanupOriginalFindShortcut() + cleanupModifiedFindShortcut() modelContentSub.dispose() }) } diff --git a/src/renderer/src/components/editor/DiffViewer.tsx b/src/renderer/src/components/editor/DiffViewer.tsx index 7f58688754..9aae498c1c 100644 --- a/src/renderer/src/components/editor/DiffViewer.tsx +++ b/src/renderer/src/components/editor/DiffViewer.tsx @@ -16,7 +16,7 @@ import { import { applyDiffEditorLineNumberOptions } from './diff-editor-line-number-options' import type { DiffComment } from '../../../../shared/types' import { isDiffComment } from '@/lib/diff-comment-compat' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { diffEditorScrollbarOptions } from './diff-editor-scrollbar-options' import { LargeDiffFallback } from './LargeDiffFallback' import { getLargeDiffRenderLimit } from './large-diff-render-limit' @@ -336,15 +336,19 @@ export default function DiffViewer({ onSaveRef.current?.(modifiedEditor.getValue()) } ) + const cleanupOriginalFindShortcut = installMonacoEditorFindShortcut(originalEditor) + const cleanupModifiedFindShortcut = installMonacoEditorFindShortcut(modifiedEditor) // Track changes const modelContentSub = modifiedEditor.onDidChangeModelContent(() => { onContentChangeRef.current?.(modifiedEditor.getValue()) }) modifiedEditor.onDidDispose(() => { - // Why: editable diff views own both the save shortcut and - // model-change subscription for this Monaco editor instance. + // Why: editable diff views own both panes' shortcut bridges and the + // model subscription for the lifetime of this Monaco diff instance. cleanupSaveShortcut() + cleanupOriginalFindShortcut() + cleanupModifiedFindShortcut() modelContentSub.dispose() }) diff --git a/src/renderer/src/components/editor/IpynbViewer.tsx b/src/renderer/src/components/editor/IpynbViewer.tsx index d813e3ff90..fc19ca0417 100644 --- a/src/renderer/src/components/editor/IpynbViewer.tsx +++ b/src/renderer/src/components/editor/IpynbViewer.tsx @@ -51,7 +51,11 @@ import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip import { ShortcutKeyCombo } from '@/components/ShortcutKeyCombo' import { useShortcutKeyDetails, type ShortcutKeyComboDetails } from '@/hooks/useShortcutLabel' import { registerPendingEditorFlush } from './editor-pending-flush' -import { editorShortcutMatches, installEditorSaveShortcut } from './editor-shortcuts' +import { + editorShortcutMatches, + installEditorSaveShortcut, + installMonacoEditorFindShortcut +} from './editor-shortcuts' import { getIpynbCodeCellEditorHeight, getIpynbCodeCellPreviewLines } from './ipynb-code-cell-lines' import MonacoCodeExcerpt from './MonacoCodeExcerpt' import { @@ -349,13 +353,15 @@ function CodeCell({ void onSaveRequestRef.current() } ) + const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance) const blurSub = editorInstance.onDidBlurEditorWidget(() => { onDeactivateRef.current() }) editorInstance.onDidDispose(() => { - // Why: the inline source editor owns both the save shortcut and blur - // subscription for this Monaco editor instance. + // Why: the inline source editor owns its shortcut bridges and blur + // subscription for the lifetime of this Monaco editor instance. cleanupSaveShortcut() + cleanupFindShortcut() blurSub.dispose() }) editorInstance.addCommand(monacoInstance.KeyCode.Escape, () => { diff --git a/src/renderer/src/components/editor/MonacoEditor.tsx b/src/renderer/src/components/editor/MonacoEditor.tsx index bf90188314..d99af690af 100644 --- a/src/renderer/src/components/editor/MonacoEditor.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.tsx @@ -44,7 +44,7 @@ import { getDiffCommentPopoverTop } from '../diff-comments/diff-comment-popover-position' import { isLinuxUserAgent } from '../terminal-pane/pane-helpers' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { Plus } from 'lucide-react' import { getMonacoMarkdownSelectionAnnotationTarget, @@ -390,13 +390,12 @@ export default function MonacoEditor({ return model.getValueInRange(selection) }) - const cleanupSaveShortcut = installEditorSaveShortcut( - editorInstance.getContainerDomNode(), - () => { - const value = editorInstance.getValue() - propsRef.current.onSave(value) - } - ) + const editorDomNode = editorInstance.getContainerDomNode() + const cleanupSaveShortcut = installEditorSaveShortcut(editorDomNode, () => { + const value = editorInstance.getValue() + propsRef.current.onSave(value) + }) + const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance) const searchInFilesAction = editorInstance.addAction({ id: 'orca.searchInFiles', label: translate('auto.components.editor.MonacoEditor.fd68ae03b3', 'Search in Files'), @@ -442,7 +441,6 @@ export default function MonacoEditor({ } }) } - const editorDomNode = editorInstance.getContainerDomNode() editorDomNode.addEventListener('paste', onLargeTextPaste, { capture: true }) // Track cursor line for "copy path to line" feature @@ -496,6 +494,7 @@ export default function MonacoEditor({ scrollStateSub.dispose() gutterMouseDownSub.dispose() cleanupSaveShortcut() + cleanupFindShortcut() editorDomNode.removeEventListener('paste', onLargeTextPaste, { capture: true }) searchInFilesAction.dispose() autoHeightSub?.dispose() diff --git a/src/renderer/src/components/editor/editor-shortcuts.test.ts b/src/renderer/src/components/editor/editor-shortcuts.test.ts new file mode 100644 index 0000000000..daacea7f64 --- /dev/null +++ b/src/renderer/src/components/editor/editor-shortcuts.test.ts @@ -0,0 +1,198 @@ +// @vitest-environment happy-dom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const shortcutState = vi.hoisted(() => ({ + keybindings: {} as Record, + platform: 'darwin' as NodeJS.Platform +})) + +vi.mock('@/lib/shortcut-platform', () => ({ + getShortcutPlatform: () => shortcutState.platform +})) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => ({ keybindings: shortcutState.keybindings }) + } +})) + +import { installEditorFindShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' + +type ShortcutFixture = { + container: HTMLDivElement + dispose: () => void + input: HTMLTextAreaElement + onDownstreamKeyDown: ReturnType + onFind: ReturnType +} + +function createShortcutFixture(): ShortcutFixture { + const container = document.createElement('div') + const input = document.createElement('textarea') + const onDownstreamKeyDown = vi.fn() + const onFind = vi.fn() + + container.appendChild(input) + document.body.appendChild(container) + input.addEventListener('keydown', onDownstreamKeyDown) + + return { + container, + dispose: installEditorFindShortcut(container, onFind), + input, + onDownstreamKeyDown, + onFind + } +} + +function dispatchKeyDown(target: HTMLElement, init: KeyboardEventInit): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + ...init, + bubbles: true, + cancelable: true + }) + target.dispatchEvent(event) + return event +} + +beforeEach(() => { + shortcutState.keybindings = {} + shortcutState.platform = 'darwin' +}) + +afterEach(() => { + document.body.replaceChildren() +}) + +describe('installEditorFindShortcut', () => { + it.each([ + { label: 'macOS', platform: 'darwin' as const, modifier: { metaKey: true } }, + { label: 'Linux', platform: 'linux' as const, modifier: { ctrlKey: true } }, + { label: 'Windows', platform: 'win32' as const, modifier: { ctrlKey: true } } + ])('matches logical f on physical KeyU for $label', ({ platform, modifier }) => { + shortcutState.platform = platform + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyU', + ...modifier + }) + + expect(event.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it('consumes the QWERTY shortcut before Monaco can handle it again', () => { + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + + expect(event.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it('consumes matched repeats without invoking find again', () => { + const fixture = createShortcutFixture() + + const initialEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + const repeatEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true, + repeat: true + }) + + expect(initialEvent.defaultPrevented).toBe(true) + expect(repeatEvent.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it.each([ + { label: 'ordinary f typing', init: { key: 'f', code: 'KeyU' } }, + { + label: 'an unrelated shortcut', + init: { key: 'g', code: 'KeyG', metaKey: true } + } + ])('leaves $label untouched', ({ init }) => { + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, init) + + expect(event.defaultPrevented).toBe(false) + expect(fixture.onFind).not.toHaveBeenCalled() + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + fixture.dispose() + }) + + it('honors a custom editor.find binding', () => { + shortcutState.keybindings = { 'editor.find': ['Mod+G'] } + const fixture = createShortcutFixture() + + const defaultEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + const customEvent = dispatchKeyDown(fixture.input, { + key: 'g', + code: 'KeyU', + metaKey: true + }) + + expect(defaultEvent.defaultPrevented).toBe(false) + expect(customEvent.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + fixture.dispose() + }) + + it('removes the listener when disposed', () => { + const fixture = createShortcutFixture() + fixture.dispose() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyU', + metaKey: true + }) + + expect(event.defaultPrevented).toBe(false) + expect(fixture.onFind).not.toHaveBeenCalled() + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + }) + + it('runs Monaco existing find action through the shared bridge', () => { + const container = document.createElement('div') + const input = document.createElement('textarea') + const run = vi.fn() + const getAction = vi.fn((_id: string) => ({ run })) + container.appendChild(input) + document.body.appendChild(container) + const dispose = installMonacoEditorFindShortcut({ + getAction, + getContainerDomNode: () => container + }) + + dispatchKeyDown(input, { key: 'f', code: 'KeyU', metaKey: true }) + + expect(getAction).toHaveBeenCalledWith('actions.find') + expect(run).toHaveBeenCalledTimes(1) + dispose() + }) +}) diff --git a/src/renderer/src/components/editor/editor-shortcuts.ts b/src/renderer/src/components/editor/editor-shortcuts.ts index 66f30c493b..86a7e72122 100644 --- a/src/renderer/src/components/editor/editor-shortcuts.ts +++ b/src/renderer/src/components/editor/editor-shortcuts.ts @@ -28,3 +28,31 @@ export function installEditorSaveShortcut(target: HTMLElement, onSave: () => voi target.addEventListener('keydown', handleKeyDown, true) return () => target.removeEventListener('keydown', handleKeyDown, true) } + +export function installEditorFindShortcut(target: HTMLElement, onFind: () => void): () => void { + const handleKeyDown = (event: KeyboardEvent): void => { + if (!editorShortcutMatches('editor.find', event)) { + return + } + event.preventDefault() + event.stopPropagation() + // Why: matched repeats must stay consumed so Monaco cannot reopen or reset find. + if (!event.repeat) { + onFind() + } + } + + target.addEventListener('keydown', handleKeyDown, true) + return () => target.removeEventListener('keydown', handleKeyDown, true) +} + +type MonacoFindShortcutEditor = { + getAction: (id: string) => { run: () => void | Promise } | null + getContainerDomNode: () => HTMLElement +} + +export function installMonacoEditorFindShortcut(editor: MonacoFindShortcutEditor): () => void { + return installEditorFindShortcut(editor.getContainerDomNode(), () => { + void editor.getAction('actions.find')?.run() + }) +} From f9c6c26018703c9333d64ac3b321fdcb004146d9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:00:41 +0000 Subject: [PATCH 10/10] Update README downloads badge --- docs/assets/readme-downloads.svg | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/assets/readme-downloads.svg b/docs/assets/readme-downloads.svg index 1333939252..bb7f03ef09 100644 --- a/docs/assets/readme-downloads.svg +++ b/docs/assets/readme-downloads.svg @@ -1,5 +1,5 @@ - - downloads: 4.2m + + downloads: 4.3m @@ -15,7 +15,7 @@ downloads downloads - 4.2m - 4.2m + 4.3m + 4.3m