From 8dc26e152f4cf442def11e7985e823e11c5adab3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:35:28 -0700 Subject: [PATCH 01/27] perf(editor): stop re-rendering every code block on each keystroke (#17008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(editor): stop re-rendering every code block on each keystroke Profiling a 305 KB document in a packaged build showed typing was dominated by two things that had nothing to do with the text being typed. Tiptap re-renders a React node view whenever its document *position* changes, even when the node and its decorations are untouched (@tiptap/react 3.22.5, ReactNodeView.update). Typing shifts the position of every node after the caret, so one keystroke in a document with 533 code blocks cost 533 React renders. That re-render only exists so a component can observe a fresh getPos(); RichMarkdownCodeBlock never reads it, so it now opts out via an explicit `update`. getPos() stays correct for later callers — Tiptap updates its position bookkeeping before calling `update`, and passes getPos as a live function rather than a captured value. The language only paints its selected option, so until the + // user reaches for the list one entry renders the same visible label. + + )} {/* If the document has a language not in our list, show it as-is */} - {language && !LANGUAGES.some((l) => l.value === language) ? ( - + {languageListMounted && language && !isKnownCodeBlockLanguage(language) ? ( + ) : null} ) @@ -534,6 +563,13 @@ describe('WorktreeJumpPalette interleaved primary sections', () => { btn.textContent?.includes('See more') ) expect(seeMoreBtn).toBeDefined() + const initialItemIds = Array.from(testContainer.querySelectorAll('[cmdk-item]')).map((item) => + item.getAttribute('data-value') + ) + const seeMoreIndex = initialItemIds.indexOf('__hint_worktree_overflow__') + expect(seeMoreIndex).toBeGreaterThan(0) + const input = testContainer.querySelector('[data-command-input="true"]') + input?.focus() await act(async () => { seeMoreBtn?.click() @@ -541,9 +577,20 @@ describe('WorktreeJumpPalette interleaved primary sections', () => { await flushEffects() // After expanding by 20: 30 worktrees are rendered, 5 more - const renderedItems = testContainer.querySelectorAll('[data-command-item]') + const renderedItems = testContainer.querySelectorAll('[data-command-item^="worktree:"]') expect(renderedItems).toHaveLength(30) expect(testContainer.textContent).toContain('5 more') + const firstRevealedItemId = Array.from(testContainer.querySelectorAll('[cmdk-item]'))[ + seeMoreIndex + ]?.getAttribute('data-value') + expect(firstRevealedItemId).toMatch(/^worktree:/) + expect(firstRevealedItemId).not.toBe(initialItemIds[0]) + expect( + testContainer + .querySelector('[data-command-dialog="true"]') + ?.getAttribute('data-command-value') + ).toBe(firstRevealedItemId) + expect(document.activeElement).toBe(input) // Click again: 30 + 20 = 50 (all 35 fit), hint disappears const seeMoreBtn2 = Array.from(testContainer.querySelectorAll('button')).find((btn) => @@ -554,7 +601,7 @@ describe('WorktreeJumpPalette interleaved primary sections', () => { }) await flushEffects() - const renderedItemsAll = testContainer.querySelectorAll('[data-command-item]') + const renderedItemsAll = testContainer.querySelectorAll('[data-command-item^="worktree:"]') expect(renderedItemsAll).toHaveLength(35) expect(testContainer.textContent).not.toContain('more') }) diff --git a/src/renderer/src/lib/worktree-palette-create-action.test.ts b/src/renderer/src/lib/worktree-palette-create-action.test.ts index 408d6227c45..8f4667a043e 100644 --- a/src/renderer/src/lib/worktree-palette-create-action.test.ts +++ b/src/renderer/src/lib/worktree-palette-create-action.test.ts @@ -182,7 +182,7 @@ describe('worktree-palette-create-action', () => { }) }) - it('derives selection ids from rendered entries while skipping headers and hints', () => { + it('derives selection ids from rendered entries while skipping headers', () => { expect( getWorktreePaletteSelectionItemIds([ { id: '__header_worktrees__', type: 'section-header' }, @@ -198,6 +198,7 @@ describe('worktree-palette-create-action', () => { ).toEqual([ 'worktree:one', CREATE_WORKTREE_ITEM_ID, + '__hint_worktree_cap__', 'settings:ai-provider-accounts', 'quick-action:new-terminal', 'browser-page:one' diff --git a/src/renderer/src/lib/worktree-palette-create-action.ts b/src/renderer/src/lib/worktree-palette-create-action.ts index f054b5d0f52..392bcddc33f 100644 --- a/src/renderer/src/lib/worktree-palette-create-action.ts +++ b/src/renderer/src/lib/worktree-palette-create-action.ts @@ -66,7 +66,8 @@ const SELECTABLE_ENTRY_TYPES = [ 'browser-page', 'workspace-tab', 'simulator-tab', - 'project-target' + 'project-target', + 'hint' ] as const type WorktreePaletteSelectableEntryType = (typeof SELECTABLE_ENTRY_TYPES)[number] @@ -85,7 +86,7 @@ export function getWorktreePaletteSelectionItemIds< T extends WorktreePaletteSelectionCandidateEntry >(entries: readonly T[], renderKeys: readonly string[] = []): string[] { // Why: keyboard focus should mirror rendered order, including synthetic - // action rows, while skipping headers and explanatory hint rows. + // action rows, while skipping only section headers. // Why renderKeys wins: rows render under de-duplicated keys, so naming the bare // id here would leave a duplicate row absent from the list the `includes` check // above consults — arrowing onto it would snap the highlight back to the top. From aa95bdb11ad7b1b6604334399c00210041530900 Mon Sep 17 00:00:00 2001 From: JahyunBaek <60498711+JahyunBaek@users.noreply.github.com> Date: Sun, 30 Aug 2026 06:23:22 +0900 Subject: [PATCH 22/27] test(shared): stop two suites asserting POSIX separators on Windows (#16511) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both files describe paths with POSIX literals while their subjects compose paths through `node:path`, so the assertions only hold where the separator happens to be `/`. `node-markdown-document-discovery` keys its fake tree at `/repo/docs` and `/repo/one`, but `discoverMarkdownRelativePaths` descends with `join(absoluteDirectoryPath, entry.name)` — `\repo\docs` on win32. The child lookup misses, `readDirectory` yields nothing, and the walk stops at the root: `docs/guide.mdx` disappears and the depth-limit case never reaches its limit, so it resolves `[]` instead of rejecting. Keying the children with `join` walks the tree the subject actually walks. `git-fetch-head-lock` expects `cwd: '/tmp/repo'` from a subject that returns `path.resolve(cwd, 'repo')`, which is `C:\tmp\repo` on win32. Asserting through `path.resolve` pins the behaviour — that `-C` and `--git-dir` are resolved against the cwd — rather than the separator of whichever machine runs the suite. Verified on Windows 11: the two files go from 3 failed / 12 passed to 14 passed / 1 skipped, and the wider `src/shared` run shows no regression. Co-authored-by: Claude Opus 5 (1M context) --- src/shared/git-fetch-head-lock.test.ts | 6 ++++-- src/shared/node-markdown-document-discovery.test.ts | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/shared/git-fetch-head-lock.test.ts b/src/shared/git-fetch-head-lock.test.ts index bcf1e13a7fc..4133d8d373e 100644 --- a/src/shared/git-fetch-head-lock.test.ts +++ b/src/shared/git-fetch-head-lock.test.ts @@ -26,13 +26,15 @@ describe('runWithGitFetchHeadLock', () => { expect(resolveGitFetchHeadCommand(args, '/repo').needsLock).toBe(expected) }) + // Why resolve() rather than a literal: the subject resolves these through path.resolve, so a + // POSIX literal asserts the separator of the machine running the suite instead of the behaviour. it('resolves -C and --git-dir before deriving the lock identity', () => { expect(resolveGitFetchHeadCommand(['-C', 'repo', 'fetch'], '/tmp')).toMatchObject({ - cwd: '/tmp/repo', + cwd: path.resolve('/tmp', 'repo'), needsLock: true }) expect(resolveGitFetchHeadCommand(['--git-dir=/repo/.git', 'fetch'], '/tmp')).toMatchObject({ - gitDir: '/repo/.git', + gitDir: path.resolve('/tmp', '/repo/.git'), needsLock: true }) }) diff --git a/src/shared/node-markdown-document-discovery.test.ts b/src/shared/node-markdown-document-discovery.test.ts index e151f25ad90..9c5e1bdfd14 100644 --- a/src/shared/node-markdown-document-discovery.test.ts +++ b/src/shared/node-markdown-document-discovery.test.ts @@ -1,4 +1,5 @@ import type { Dirent } from 'node:fs' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { MarkdownDocumentListingCapacityError } from './markdown-document-listing-limits' import { discoverMarkdownRelativePaths } from './node-markdown-document-discovery' @@ -22,6 +23,8 @@ function reader(entriesByPath: Record) { describe('bounded Markdown document discovery', () => { it('preserves depth-first discovery and skips excluded and symlinked directories', async () => { + // Why join() for the child key: the subject descends with path.join, so a '/repo/docs' + // literal never matches on Windows and the walk silently stops at the root. const result = await discoverMarkdownRelativePaths('/repo', { readDirectory: reader({ '/repo': [ @@ -30,7 +33,7 @@ describe('bounded Markdown document discovery', () => { entry('docs', 'directory'), entry('linked', 'symlink') ], - '/repo/docs': [entry('guide.mdx'), entry('app.ts')] + [join('/repo', 'docs')]: [entry('guide.mdx'), entry('app.ts')] }), shouldDescend: (_relativePath, name) => name !== '.git' }) @@ -65,7 +68,7 @@ describe('bounded Markdown document discovery', () => { limits: { maxDepth: 1 }, readDirectory: reader({ '/repo': [entry('one', 'directory')], - '/repo/one': [entry('two', 'directory')] + [join('/repo', 'one')]: [entry('two', 'directory')] }), shouldDescend: () => true }) From 7655d20f04ffc11a6221c504652fb435c8e0d88d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 29 Aug 2026 14:34:50 -0700 Subject: [PATCH 23/27] fix(terminal): complete OMP stale cwd recovery (#17154) --- config/reliability-gates.jsonc | 12 +- .../daemon-bash-rcfile.txt | 11 +- .../daemon-zsh-zshenv.txt | 11 +- .../local-bash-rcfile.txt | 11 +- .../local-zsh-zshenv.txt | 11 +- .../relay-bash-rcfile.txt | 11 +- .../relay-zsh-zshenv.txt | 11 +- .../pty/omp-shell-wrapper.node-pty.test.ts | 111 +++++++++++++----- src/main/pty/omp-shell-wrapper.ts | 11 +- 9 files changed, 141 insertions(+), 59 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 10143d9019c..4dea7b1e3e2 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -6345,8 +6345,8 @@ "https://github.com/stablyai/orca/issues/16457", "https://github.com/stablyai/orca/pull/17128" ], - "invariant": "A fresh local renderer terminal spawn may recover from a saved startup cwd whose directory no longer exists only by spawning at the selected workspace root and printing a generic in-terminal notice; existing directories — including ones outside the worktree (#7685) — spawn as requested, and reattach, SSH, remote-runtime, runtime/API, and mobile callers keep exact cwd semantics. A long-lived POSIX shell whose cwd inode was deleted and replaced may run an extension-enabled OMP launch from the live path named by its logical PWD only in a subshell; the parent shell cwd, OMP argv, status extension, and exit status remain unchanged, while a genuinely unavailable path fails visibly without invoking OMP. A usable current directory remains authoritative when PWD is unset instead of being remapped to a workspace fallback.", - "oracle": "The shared resolver falls back to the workspace root only when the injected existence probe reports the resolved cwd missing and the workspace root present, and never probes floating terminals or a cwd equal to the root. The renderer sends cwdFallback only for fresh local IPC spawns, main honors it only when connectionId and sessionId are absent, WSL UNC paths never engage the probe-based fallback, main returns fallback metadata only after an actual fallback, the IPC transport preserves that metadata, and the connection layer writes a generic notice that omits the missing path. In real interactive Bash and Zsh PTYs, unset PWD in a usable nested directory and require OMP to stay there rather than remap to ORCA_WORKTREE_PATH; then delete and recreate the active project path, require OMP to observe the replacement inode with byte-exact extension argv and its nonzero status preserved, require the parent shell to remain on the stale inode, and finally delete the replacement and require an actionable failure before the fake OMP binary runs again.", + "invariant": "A fresh local renderer terminal spawn may recover from a saved startup cwd whose directory no longer exists only by spawning at the selected workspace root and printing a generic in-terminal notice; existing directories — including ones outside the worktree (#7685) — spawn as requested, and reattach, SSH, remote-runtime, runtime/API, and mobile callers keep exact cwd semantics. A long-lived POSIX shell whose cwd inode was deleted and replaced may run a wrapped OMP command from the live path named by its logical PWD only in a subshell; the parent shell cwd, OMP argv, status extension, and exit status remain unchanged, while a genuinely unavailable path fails visibly without invoking OMP. A usable physical cwd remains authoritative when PWD is unset or names a removed logical symlink instead of being remapped to a workspace fallback.", + "oracle": "The shared resolver falls back to the workspace root only when the injected existence probe reports the resolved cwd missing and the workspace root present, and never probes floating terminals or a cwd equal to the root. The renderer sends cwdFallback only for fresh local IPC spawns, main honors it only when connectionId and sessionId are absent, WSL UNC paths never engage the probe-based fallback, main returns fallback metadata only after an actual fallback, the IPC transport preserves that metadata, and the connection layer writes a generic notice that omits the missing path. In real interactive Bash and Zsh PTYs, unset PWD and remove a logical symlink while the physical cwd stays usable, requiring OMP to remain there; then delete and recreate the active project path, require extension-enabled and extension-skipping invocations to observe the replacement inode with byte-exact argv and nonzero status preserved, require the parent shell to remain on the stale inode, unset PWD and require recovery through ORCA_WORKTREE_PATH, clear every logical fallback and require an actionable failure without invoking OMP, and finally delete the replacement and require the same for the missing PWD path.", "commands": [ "pnpm exec vitest run --config config/vitest.config.ts src/shared/terminal-startup-cwd.test.ts", "pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/pty-spawn-cwd-fallback.test.ts src/main/ipc/pty-wsl-cwd-validation.test.ts", @@ -6410,6 +6410,10 @@ "assertions": [ "real Bash and Zsh PTYs rebind OMP to a recreated cwd inode without changing the parent shell", "an unset PWD with a usable cwd stays unset and does not remap OMP to ORCA_WORKTREE_PATH", + "a removed logical symlink does not invalidate its still-usable physical cwd", + "extension-skipping commands such as omp --version still recover the cwd without gaining extension argv", + "an unset PWD on a stale inode recovers through ORCA_WORKTREE_PATH", + "an unset PWD on a stale inode without a logical fallback fails visibly before invoking OMP", "extension argv and a nonzero OMP exit status survive the recovery subshell", "a subsequently missing logical cwd prints an actionable error and does not invoke OMP again" ] @@ -6465,7 +6469,7 @@ "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/pty/omp-shell-wrapper.node-pty.test.ts src/main/pty/omp-shell-wrapper-alias-safety.test.ts src/main/shell-wrapper-generated-file-snapshot.test.ts", "result": "passed", "durationSeconds": 3, - "summary": "3 test files passed; real Bash and Zsh PTYs covered unset-PWD authority, stale-inode recovery, parent-shell isolation, extension argv, exit status, missing-path visibility, alias safety, and generated local/daemon/relay parity." + "summary": "3 test files passed; real Bash and Zsh PTYs covered usable physical cwd authority, stale-inode recovery with set and unset PWD, extension-enabled and extension-skipping argv, parent-shell isolation, exit status, missing-path visibility, alias safety, and generated local/daemon/relay parity." } ], "runtimeBudget": { @@ -6482,7 +6486,7 @@ }, "performanceBudget": { "required": true, - "evidence": "The spawn recovery adds at most two statSync probes on the fresh-local spawn path (the provider already stats the same paths during validation) and one bounded terminal write only when fallback actually occurs. Each extension-enabled OMP launch adds a constant number of shell filesystem predicates; only a detected stale/unusable cwd creates one short recovery subshell. No polling, retry, provider listing, hidden-pane work, startup awaits, extra OMP subprocesses, or render-loop work was added." + "evidence": "The spawn recovery adds at most two statSync probes on the fresh-local spawn path (the provider already stats the same paths during validation) and one bounded terminal write only when fallback actually occurs. Each wrapped OMP launch adds a constant number of shell filesystem predicates; only a detected stale/unusable cwd creates one short recovery subshell. No polling, retry, provider listing, hidden-pane work, startup awaits, extra OMP subprocesses, or render-loop work was added." }, "promotionCriteria": [ "Attach CI evidence for all declared test files.", diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt index b91f1065f84..54837bf4d65 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt @@ -45,11 +45,14 @@ __orca_omp_should_skip_extension() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -69,7 +72,7 @@ __orca_omp_invoke() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txt b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txt index bce09f52ff0..3d3403ad099 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txt @@ -83,11 +83,14 @@ __orca_deferred_init() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -107,7 +110,7 @@ __orca_deferred_init() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txt b/src/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txt index ee9cb7975ec..d4bb98997c2 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txt @@ -48,11 +48,14 @@ __orca_omp_should_skip_extension() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -72,7 +75,7 @@ __orca_omp_invoke() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txt b/src/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txt index 222cab93896..ee18600acb1 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txt @@ -83,11 +83,14 @@ __orca_deferred_init() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -107,7 +110,7 @@ __orca_deferred_init() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txt b/src/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txt index 9d90929085d..de9c8f95248 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txt @@ -37,11 +37,14 @@ __orca_omp_should_skip_extension() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -61,7 +64,7 @@ __orca_omp_invoke() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txt b/src/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txt index 335b54c60c3..394bc4a6d10 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txt @@ -57,11 +57,14 @@ __orca_deferred_init() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "${PWD:-}" ]]; then - [[ -d "${PWD}" && "${PWD}" -ef . ]] + if [[ -n "${PWD:-}" && -d "${PWD:-}" ]]; then + [[ "${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -81,7 +84,7 @@ __orca_deferred_init() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="${PWD:-${ORCA_WORKTREE_PATH:-${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( diff --git a/src/main/pty/omp-shell-wrapper.node-pty.test.ts b/src/main/pty/omp-shell-wrapper.node-pty.test.ts index 6ab30dfdbae..15bc98bdb3b 100644 --- a/src/main/pty/omp-shell-wrapper.node-pty.test.ts +++ b/src/main/pty/omp-shell-wrapper.node-pty.test.ts @@ -1,11 +1,13 @@ import { spawnSync } from 'node:child_process' import { chmodSync, + existsSync, mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, + symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' @@ -334,18 +336,82 @@ exit 0 const homeDir = join(tempDir, 'home') const binDir = join(tempDir, 'bin') const extensionDir = join(tempDir, 'extensions') + const logicalProjectLink = join(tempDir, 'logical-project') mkdirSync(projectDir, { recursive: true }) mkdirSync(homeDir) mkdirSync(binDir) mkdirSync(extensionDir) + symlinkSync(projectDir, logicalProjectLink) const expectedProjectDir = realpathSync(projectDir) + const expectedWorkspaceDir = realpathSync(workspaceDir) const statusExtension = join(extensionDir, 'orca-agent-status.ts') writeFileSync(statusExtension, 'export default {}') writeFakeOmp(binDir) const unsetPwdCaptureFile = join(tempDir, 'unset-pwd-capture') + const deletedLinkCaptureFile = join(tempDir, 'deleted-link-capture') const staleCaptureFile = join(tempDir, 'stale-cwd-capture') + const skipCaptureFile = join(tempDir, 'skip-capture') + const staleUnsetCaptureFile = join(tempDir, 'stale-unset-capture') + const noLogicalCaptureFile = join(tempDir, 'no-logical-capture') const resultFile = join(tempDir, 'stale-cwd-result') + const scenarioFile = join(tempDir, 'stale-cwd-scenario') + writeFileSync( + scenarioFile, + `ORCA_CAPTURE_FILE="$ORCA_UNSET_PWD_CAPTURE_FILE" +unset PWD +omp +__orca_test_unset_status=$? +if [[ -z "\${PWD+x}" ]]; then + __orca_test_pwd_state=unset +else + __orca_test_pwd_state=set +fi +builtin cd -- "$ORCA_LOGICAL_PROJECT_LINK" +/bin/rm -- "$ORCA_LOGICAL_PROJECT_LINK" +ORCA_CAPTURE_FILE="$ORCA_DELETED_LINK_CAPTURE_FILE" +omp +__orca_test_deleted_link_status=$? +builtin cd -P -- "$ORCA_STALE_PROJECT_DIR" +ORCA_CAPTURE_FILE="$ORCA_STALE_CAPTURE_FILE" +/bin/rm -rf -- "$ORCA_STALE_PROJECT_DIR" +/bin/mkdir -p -- "$ORCA_STALE_PROJECT_DIR" +omp +__orca_test_first_status=$? +ORCA_CAPTURE_FILE="$ORCA_SKIP_CAPTURE_FILE" +omp --version +__orca_test_skip_status=$? +if [[ "$PWD" -ef . ]]; then + __orca_test_parent_state=live +else + __orca_test_parent_state=stale +fi +unset PWD +ORCA_CAPTURE_FILE="$ORCA_STALE_UNSET_CAPTURE_FILE" +omp +__orca_test_stale_unset_status=$? +unset ORCA_WORKTREE_PATH ORCA_ROOT_PATH +ORCA_CAPTURE_FILE="$ORCA_NO_LOGICAL_CAPTURE_FILE" +omp +__orca_test_no_logical_status=$? +PWD="$ORCA_STALE_PROJECT_DIR" +/bin/rm -rf -- "$ORCA_STALE_PROJECT_DIR" +omp +__orca_test_missing_status=$? +{ + echo "UNSET=$__orca_test_unset_status" + echo "PWD=$__orca_test_pwd_state" + echo "LINK=$__orca_test_deleted_link_status" + echo "FIRST=$__orca_test_first_status" + echo "SKIP=$__orca_test_skip_status" + echo "PARENT=$__orca_test_parent_state" + echo "STALE_UNSET=$__orca_test_stale_unset_status" + echo "NO_LOGICAL=$__orca_test_no_logical_status" + echo "MISSING=$__orca_test_missing_status" +} > "$ORCA_RESULT_FILE" +exit 0 +` + ) const output = await runInteractivePosixPty({ shell, cwd: projectDir, @@ -355,47 +421,30 @@ ${getPosixOmpShellWrapper()}`, INPUTRC: '/dev/null', PROMPT_COMMAND: '', ORCA_STALE_PROJECT_DIR: projectDir, + ORCA_LOGICAL_PROJECT_LINK: logicalProjectLink, ORCA_UNSET_PWD_CAPTURE_FILE: unsetPwdCaptureFile, + ORCA_DELETED_LINK_CAPTURE_FILE: deletedLinkCaptureFile, ORCA_STALE_CAPTURE_FILE: staleCaptureFile, + ORCA_SKIP_CAPTURE_FILE: skipCaptureFile, + ORCA_STALE_UNSET_CAPTURE_FILE: staleUnsetCaptureFile, + ORCA_NO_LOGICAL_CAPTURE_FILE: noLogicalCaptureFile, ORCA_WORKTREE_PATH: workspaceDir, HOME: homeDir, PATH: `${binDir}:/usr/bin:/bin:/usr/sbin:/sbin`, ORCA_OMP_STATUS_EXTENSION: statusExtension, ORCA_CAPTURE_FILE: staleCaptureFile, ORCA_RESULT_FILE: resultFile, + ORCA_SCENARIO_FILE: scenarioFile, ORCA_TEST_FAKE_OMP_EXIT_CODE: '23', TERM: 'xterm-256color' }, - input: `ORCA_CAPTURE_FILE="$ORCA_UNSET_PWD_CAPTURE_FILE" -unset PWD -omp -__orca_test_unset_status=$? -if [[ -z "\${PWD+x}" ]]; then - __orca_test_pwd_state=unset -else - __orca_test_pwd_state=set -fi -builtin cd -P -- "$ORCA_STALE_PROJECT_DIR" -ORCA_CAPTURE_FILE="$ORCA_STALE_CAPTURE_FILE" -/bin/rm -rf -- "$ORCA_STALE_PROJECT_DIR" -/bin/mkdir -p -- "$ORCA_STALE_PROJECT_DIR" -omp -__orca_test_first_status=$? -if [[ "$PWD" -ef . ]]; then - __orca_test_parent_state=live -else - __orca_test_parent_state=stale -fi -/bin/rm -rf -- "$ORCA_STALE_PROJECT_DIR" -omp -__orca_test_missing_status=$? -printf 'UNSET=%s\nPWD=%s\nFIRST=%s\nPARENT=%s\nMISSING=%s\n' "$__orca_test_unset_status" "$__orca_test_pwd_state" "$__orca_test_first_status" "$__orca_test_parent_state" "$__orca_test_missing_status" > "$ORCA_RESULT_FILE" -exit 0 -` + input: 'source "$ORCA_SCENARIO_FILE"\n' }) const unsetPwdCapture = readFileSync(unsetPwdCaptureFile, 'utf8') expect(unsetPwdCapture).toContain(`CWD=${expectedProjectDir}`) + const deletedLinkCapture = readFileSync(deletedLinkCaptureFile, 'utf8') + expect(deletedLinkCapture).toContain(`CWD=${expectedProjectDir}`) const staleCapture = readFileSync(staleCaptureFile, 'utf8') expect(staleCapture).toContain(`CWD=${expectedProjectDir}`) expect(staleCapture.split('\n').filter((line) => line.startsWith('ARG'))).toEqual([ @@ -403,9 +452,17 @@ exit 0 `ARG2=${statusExtension}` ]) expect(staleCapture).not.toContain('--cwd') + const skipCapture = readFileSync(skipCaptureFile, 'utf8') + expect(skipCapture).toContain(`CWD=${expectedProjectDir}`) + expect(skipCapture).toContain('ARG1=--version') + expect(skipCapture).not.toContain('--extension') + const staleUnsetCapture = readFileSync(staleUnsetCaptureFile, 'utf8') + expect(staleUnsetCapture).toContain(`CWD=${expectedWorkspaceDir}`) + expect(existsSync(noLogicalCaptureFile)).toBe(false) expect(readFileSync(resultFile, 'utf8')).toBe( - 'UNSET=23\nPWD=unset\nFIRST=23\nPARENT=stale\nMISSING=1\n' + 'UNSET=23\nPWD=unset\nLINK=23\nFIRST=23\nSKIP=23\nPARENT=stale\nSTALE_UNSET=23\nNO_LOGICAL=1\nMISSING=1\n' ) + expect(output).toContain('no terminal working directory is available') expect(output).toContain('Orca: OMP cannot access the terminal working directory') } diff --git a/src/main/pty/omp-shell-wrapper.ts b/src/main/pty/omp-shell-wrapper.ts index fb6d274dc57..f5bc25421bb 100644 --- a/src/main/pty/omp-shell-wrapper.ts +++ b/src/main/pty/omp-shell-wrapper.ts @@ -52,11 +52,14 @@ __orca_omp_should_skip_extension() { return 1 } __orca_omp_cwd_is_usable() { + local __orca_physical_cwd [[ -x . ]] || return 1 - if [[ -n "\${PWD:-}" ]]; then - [[ -d "\${PWD}" && "\${PWD}" -ef . ]] + if [[ -n "\${PWD:-}" && -d "\${PWD:-}" ]]; then + [[ "\${PWD}" -ef . ]] else - builtin pwd -P >/dev/null 2>&1 + # Why compare the path: shell builtins can print a cached path for a deleted cwd. + __orca_physical_cwd="$(builtin pwd -P 2>/dev/null)" || return 1 + [[ -d "$__orca_physical_cwd" && "$__orca_physical_cwd" -ef . ]] fi } __orca_omp_invoke() { @@ -76,7 +79,7 @@ __orca_omp_invoke() { __orca_omp() { local __orca_use_extension=1 __orca_omp_should_skip_extension "\${1:-}" && __orca_use_extension=0 - if [[ $__orca_use_extension -eq 1 ]] && ! __orca_omp_cwd_is_usable; then + if ! __orca_omp_cwd_is_usable; then local __orca_logical_cwd="\${PWD:-\${ORCA_WORKTREE_PATH:-\${ORCA_ROOT_PATH:-}}}" # Why: a restored shell can retain the deleted directory inode after its path is recreated. ( From e3757a1d30224e5686974fd491c82aab0cc64395 Mon Sep 17 00:00:00 2001 From: JahyunBaek <60498711+JahyunBaek@users.noreply.github.com> Date: Sun, 30 Aug 2026 06:36:24 +0900 Subject: [PATCH 24/27] fix(worktree): ignore orphan remote-tracking refs in branch conflicts (#16699) Refs #16646 Unify native, WSL, and direct SSH conflict checks behind the execution host, remove the duplicated SSH classifier, and cover orphan/configured remote behavior across both paths. --- src/main/git/repo-base-ref-search.ts | 23 ++++- src/main/git/repo-branch-conflict.ts | 63 +++++++----- src/main/git/repo.test.ts | 37 +++++++ ...orktree-remote-ssh-branch-conflict.test.ts | 75 +++++++++++++++ src/main/ipc/worktree-remote.ts | 96 ++----------------- 5 files changed, 181 insertions(+), 113 deletions(-) create mode 100644 src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts diff --git a/src/main/git/repo-base-ref-search.ts b/src/main/git/repo-base-ref-search.ts index 5ea01a7ec1c..213868fe5a7 100644 --- a/src/main/git/repo-base-ref-search.ts +++ b/src/main/git/repo-base-ref-search.ts @@ -218,6 +218,21 @@ export function parseAndFilterSearchRefDetails( .slice(0, Math.max(0, limit)) } +export function resolveConfiguredRemoteBranchName( + fullRef: string, + longestFirstRemoteNames: readonly string[] +): string | null { + const remoteRefPrefix = 'refs/remotes/' + if (!fullRef.startsWith(remoteRefPrefix)) { + return null + } + const remoteAndBranch = fullRef.slice(remoteRefPrefix.length) + const remote = longestFirstRemoteNames.find((candidate) => + remoteAndBranch.startsWith(`${candidate}/`) + ) + return remote ? remoteAndBranch.slice(remote.length + 1) || null : null +} + export function resolveLocalBranchName( fullRef: string, shortRef: string, @@ -227,11 +242,11 @@ export function resolveLocalBranchName( if (!fullRef.startsWith(remoteRefPrefix)) { return shortRef } - const remoteAndBranch = fullRef.slice(remoteRefPrefix.length) - const remote = remotes.find((candidate) => remoteAndBranch.startsWith(`${candidate}/`)) - if (remote) { - return remoteAndBranch.slice(remote.length + 1) + const configuredBranch = resolveConfiguredRemoteBranchName(fullRef, remotes) + if (configuredBranch) { + return configuredBranch } + const remoteAndBranch = fullRef.slice(remoteRefPrefix.length) return remoteAndBranch.split('/').slice(1).join('/') || shortRef } diff --git a/src/main/git/repo-branch-conflict.ts b/src/main/git/repo-branch-conflict.ts index dbadf254d11..fcb6d8dc101 100644 --- a/src/main/git/repo-branch-conflict.ts +++ b/src/main/git/repo-branch-conflict.ts @@ -1,45 +1,50 @@ -import { listRemoteNames, resolveLocalBranchName } from './repo-base-ref-search' -import { gitExecOptions, type LocalGitExecOptions } from './repo-default-base-ref' +import { resolveConfiguredRemoteBranchName } from './repo-base-ref-search' +import { gitExecOptions, type GitExec, type LocalGitExecOptions } from './repo-default-base-ref' import { gitExecFileAsync } from './runner' export type BranchConflictKind = 'local' | 'remote' -async function hasGitRefAsync( - path: string, - ref: string, - options: LocalGitExecOptions = {} -): Promise { +async function hasGitRefAsync(exec: GitExec, ref: string): Promise { try { - await gitExecFileAsync(['rev-parse', '--verify', ref], gitExecOptions(path, options)) - return true + const { stdout } = await exec(['rev-parse', '--verify', ref]) + return stdout.trim().length > 0 } catch { return false } } -export async function getBranchConflictKind( - path: string, +async function listRemoteNamesViaExec(exec: GitExec): Promise { + try { + const { stdout } = await exec(['remote']) + return stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + .sort((a, b) => b.length - a.length) + } catch { + return [] + } +} + +/** Run branch-conflict policy through the host that owns Git execution. */ +export async function getBranchConflictKindViaExec( + exec: GitExec, branchName: string, - allowedBaseRef?: string, - options: LocalGitExecOptions = {} + allowedBaseRef?: string ): Promise { - if (await hasGitRefAsync(path, `refs/heads/${branchName}`, options)) { + if (await hasGitRefAsync(exec, `refs/heads/${branchName}`)) { return 'local' } try { - const remoteNames = (await listRemoteNames(path, options)).sort((a, b) => b.length - a.length) - const { stdout } = await gitExecFileAsync( - ['for-each-ref', '--format=%(refname)', 'refs/remotes'], - gitExecOptions(path, options) - ) - const hasRemoteConflict = stdout.split('\n').some((ref) => { + const remoteNames = await listRemoteNamesViaExec(exec) + const { stdout } = await exec(['for-each-ref', '--format=%(refname)', 'refs/remotes']) + const hasRemoteConflict = stdout.split(/\r?\n/).some((ref) => { const trimmed = ref.trim() if (isAllowedRemoteBaseRef(trimmed, allowedBaseRef)) { return false } - const shortRef = trimmed.replace(/^refs\/remotes\//, '') - return resolveLocalBranchName(trimmed, shortRef, remoteNames) === branchName + return resolveConfiguredRemoteBranchName(trimmed, remoteNames) === branchName }) return hasRemoteConflict ? 'remote' : null @@ -48,6 +53,20 @@ export async function getBranchConflictKind( } } +export function getBranchConflictKind( + path: string, + branchName: string, + allowedBaseRef?: string, + options: LocalGitExecOptions = {} +): Promise { + const execOptions = gitExecOptions(path, options) + return getBranchConflictKindViaExec( + (argv) => gitExecFileAsync(argv, execOptions), + branchName, + allowedBaseRef + ) +} + function isAllowedRemoteBaseRef(refName: string, allowedBaseRef: string | undefined): boolean { if (!allowedBaseRef) { return false diff --git a/src/main/git/repo.test.ts b/src/main/git/repo.test.ts index 90199d1c69b..7500735d72f 100644 --- a/src/main/git/repo.test.ts +++ b/src/main/git/repo.test.ts @@ -215,6 +215,7 @@ describe('searchBaseRefs (widened glob)', () => { it('allows creating a local branch from the selected matching remote base ref', async () => { const sha = getHeadSha(tmpDir) + git(tmpDir, ['remote', 'add', 'origin', 'https://example.invalid/repo.git']) createRemoteRef(tmpDir, 'origin/feature/something', sha) const result = await getBranchConflictKind( @@ -228,6 +229,10 @@ describe('searchBaseRefs (widened glob)', () => { it('still reports a remote conflict for a different tracking ref with the same branch name', async () => { const sha = getHeadSha(tmpDir) + // Why register both: a ref only tracks a branch when a remote actually owns its + // prefix, and this case is about a second *tracking* ref, not an orphan. + git(tmpDir, ['remote', 'add', 'origin', 'https://example.invalid/repo.git']) + git(tmpDir, ['remote', 'add', 'upstream', 'https://example.invalid/upstream.git']) createRemoteRef(tmpDir, 'origin/feature/something', sha) createRemoteRef(tmpDir, 'upstream/feature/something', sha) @@ -236,6 +241,38 @@ describe('searchBaseRefs (widened glob)', () => { ).toBe('remote') }) + it('ignores a ref whose prefix matches no configured remote', async () => { + const sha = getHeadSha(tmpDir) + git(tmpDir, ['remote', 'add', 'origin', 'https://example.invalid/repo.git']) + createRemoteRef(tmpDir, 'mimic-fork/my-task', sha) + + expect(await getBranchConflictKind(tmpDir, 'my-task', 'origin/main')).toBeNull() + }) + + it('stops treating leftover refs from a removed remote as conflicts', async () => { + const sha = getHeadSha(tmpDir) + git(tmpDir, ['remote', 'add', 'origin', 'https://example.invalid/repo.git']) + git(tmpDir, ['remote', 'add', 'fork', 'https://example.invalid/fork.git']) + createRemoteRef(tmpDir, 'fork/my-task', sha) + expect(await getBranchConflictKind(tmpDir, 'my-task', 'origin/main')).toBe('remote') + + // Why not `remote remove`: that prunes the tracking refs too, which would pass + // with or without the ownership guard. Dropping only the config leaves the + // orphan ref behind, which is the state this guard exists for. + git(tmpDir, ['config', '--remove-section', 'remote.fork']) + + expect(await getBranchConflictKind(tmpDir, 'my-task', 'origin/main')).toBeNull() + }) + + it('still reports a local conflict when an unrelated ref shares the name', async () => { + const sha = getHeadSha(tmpDir) + git(tmpDir, ['remote', 'add', 'origin', 'https://example.invalid/repo.git']) + createRemoteRef(tmpDir, 'mimic-fork/my-task', sha) + git(tmpDir, ['branch', 'my-task']) + + expect(await getBranchConflictKind(tmpDir, 'my-task', 'origin/main')).toBe('local') + }) + it('reports remote conflicts when the remote name contains a slash', async () => { const sha = getHeadSha(tmpDir) git(tmpDir, ['remote', 'add', 'foo/bar', 'https://example.invalid/repo.git']) diff --git a/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts b/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts new file mode 100644 index 00000000000..7ced18eb173 --- /dev/null +++ b/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it, vi } from 'vitest' + +import { getSshBranchConflictKind } from './worktree-remote' + +type ConflictProvider = Parameters[0] + +function providerAnswering(answers: { + remotes: string[] + localBranchHead?: string + remoteRefs: string[] +}): ConflictProvider { + const exec: ConflictProvider['exec'] = vi.fn(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: `${answers.remotes.join('\n')}\n`, stderr: '' } + } + if (args[0] === 'rev-parse') { + if (!answers.localBranchHead) { + throw new Error('missing ref') + } + return { stdout: `${answers.localBranchHead}\n`, stderr: '' } + } + if (args[0] === 'for-each-ref') { + return { stdout: `${answers.remoteRefs.join('\n')}\n`, stderr: '' } + } + throw new Error(`unexpected git ${args.join(' ')}`) + }) + return { exec } +} + +describe('getSshBranchConflictKind remote ownership', () => { + it('reports a local branch before scanning remote refs', async () => { + const provider = providerAnswering({ + localBranchHead: 'abc123', + remotes: ['origin'], + remoteRefs: [] + }) + + await expect( + getSshBranchConflictKind(provider, '/srv/repo', 'my-task', 'origin/main') + ).resolves.toBe('local') + }) + + it('ignores a ref whose prefix matches no configured remote', async () => { + const provider = providerAnswering({ + remotes: ['origin'], + remoteRefs: ['refs/remotes/origin/main', 'refs/remotes/deleted-fork/my-task'] + }) + + await expect( + getSshBranchConflictKind(provider, '/srv/repo', 'my-task', 'origin/main') + ).resolves.toBeNull() + }) + + it('still reports a ref owned by a configured remote', async () => { + const provider = providerAnswering({ + remotes: ['origin'], + remoteRefs: ['refs/remotes/origin/main', 'refs/remotes/origin/my-task'] + }) + + await expect( + getSshBranchConflictKind(provider, '/srv/repo', 'my-task', 'origin/main') + ).resolves.toBe('remote') + }) + + it('prefers the longest configured remote name', async () => { + const provider = providerAnswering({ + remotes: ['foo', 'foo/bar'], + remoteRefs: ['refs/remotes/foo/bar/my-task'] + }) + + await expect( + getSshBranchConflictKind(provider, '/srv/repo', 'my-task', 'origin/main') + ).resolves.toBe('remote') + }) +}) diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 581872540df..d9908b7c05c 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -34,6 +34,7 @@ import { resolveDefaultBaseRefViaExec, resolveDefaultBaseRefWithLocalGit } from '../git/repo' +import { getBranchConflictKindViaExec } from '../git/repo-branch-conflict' import { resolveLocalGitUsername, getSshGitUsername } from '../git/git-username' import { hasCommitObjectViaGitExec } from '../git/commit-object-ref' import { probeWorktreeBaseRefPresence } from '../git/worktree-base-ref-probe' @@ -839,98 +840,19 @@ async function canCheckoutExistingLocalBranchSsh( return !worktrees.some((worktree) => normalizeLocalBranchName(worktree.branch) === branchName) } -async function listSshRemoteNames(provider: SshGitProvider, repoPath: string): Promise { - try { - const { stdout } = await provider.exec(['remote'], repoPath) - return stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - .sort((a, b) => b.length - a.length) - } catch { - return [] - } -} +type SshGitExecutor = Pick -function isAllowedSshRemoteBaseRef(refName: string, allowedBaseRef: string): boolean { - if (!allowedBaseRef) { - return false - } - const normalizedAllowedRef = allowedBaseRef.startsWith('refs/remotes/') - ? allowedBaseRef - : `refs/remotes/${allowedBaseRef}` - return refName === normalizedAllowedRef -} - -function resolveSshRemoteBranchName(refName: string, remoteNames: string[]): string { - const remotePrefix = 'refs/remotes/' - if (!refName.startsWith(remotePrefix)) { - return refName - } - const remoteAndBranch = refName.slice(remotePrefix.length) - const remote = remoteNames.find((candidate) => remoteAndBranch.startsWith(`${candidate}/`)) - if (remote) { - return remoteAndBranch.slice(remote.length + 1) - } - return remoteAndBranch.split('/').slice(1).join('/') || remoteAndBranch -} - -async function hasSshRemoteBranchConflict( - provider: SshGitProvider, - repoPath: string, - branchName: string, - allowedBaseRef: string -): Promise { - const remoteNames = await listSshRemoteNames(provider, repoPath) - try { - const { stdout } = await provider.exec( - ['for-each-ref', '--format=%(refname)', 'refs/remotes'], - repoPath - ) - return stdout.split(/\r?\n/).some((line) => { - const refName = line.trim() - if (!refName || /^refs\/remotes\/.+\/HEAD$/.test(refName)) { - return false - } - if (isAllowedSshRemoteBaseRef(refName, allowedBaseRef)) { - return false - } - // Why: `git branch --all --list feature/x` doesn't match `remotes/origin/feature/x`; parse remote refs directly. - return resolveSshRemoteBranchName(refName, remoteNames) === branchName - }) - } catch { - return false - } -} - -async function hasSshLocalBranchConflict( - provider: SshGitProvider, - repoPath: string, - branchName: string -): Promise { - try { - const { stdout } = await provider.exec( - ['rev-parse', '--verify', '--quiet', `refs/heads/${branchName}^{commit}`], - repoPath - ) - return stdout.trim().length > 0 - } catch { - return false - } -} - -async function getSshBranchConflictKind( - provider: SshGitProvider, +export function getSshBranchConflictKind( + provider: SshGitExecutor, repoPath: string, branchName: string, allowedBaseRef: string ): Promise<'local' | 'remote' | null> { - if (await hasSshLocalBranchConflict(provider, repoPath, branchName)) { - return 'local' - } - return (await hasSshRemoteBranchConflict(provider, repoPath, branchName, allowedBaseRef)) - ? 'remote' - : null + return getBranchConflictKindViaExec( + (argv) => provider.exec(argv, repoPath), + branchName, + allowedBaseRef + ) } type SelectedReviewBranchInput = Pick< From b826b3fa5b7b3a3d28babd86adc5dba2568bf2f8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 29 Aug 2026 15:03:36 -0700 Subject: [PATCH 25/27] feat(editor): raise rich Markdown size limit to 600 KB (#17288) --- .../editor/markdown-rich-size-limit.test.ts | 4 ++++ src/shared/constants.ts | 17 +++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/editor/markdown-rich-size-limit.test.ts b/src/renderer/src/components/editor/markdown-rich-size-limit.test.ts index 616e298b175..3539ffc565f 100644 --- a/src/renderer/src/components/editor/markdown-rich-size-limit.test.ts +++ b/src/renderer/src/components/editor/markdown-rich-size-limit.test.ts @@ -3,6 +3,10 @@ import { RICH_MARKDOWN_MAX_SIZE_BYTES } from '../../../../shared/constants' import { exceedsMarkdownRichModeSizeLimit } from './markdown-rich-size-limit' describe('exceedsMarkdownRichModeSizeLimit', () => { + it('uses a 600 KB default rich-mode ceiling', () => { + expect(RICH_MARKDOWN_MAX_SIZE_BYTES).toBe(600 * 1024) + }) + it('allows markdown at the rich-mode byte limit', () => { expect(exceedsMarkdownRichModeSizeLimit('a'.repeat(RICH_MARKDOWN_MAX_SIZE_BYTES))).toBe(false) }) diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 15c47bdd968..96c7901cc08 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -83,16 +83,13 @@ export const getDefaultTerminalRightClickToPaste = ( platform = typeof process !== 'undefined' ? process.platform : '' ): boolean => platform === 'win32' -/** Why: ProseMirror renders the whole document — no virtualization — so opening - * one blocks the main thread for as long as it takes to build the DOM. Measured - * in a packaged build (M-series) on a doc with a code block every ~570 bytes, - * blocking mount / median keystroke: 300 KB 1.7 s / 59 ms · 600 KB 4.2 s / 201 ms - * · 900 KB 8.8 s / 431 ms. The same 300 KB as pure prose is 0.8 s / 16 ms, so - * node-view count drives the cost far more than byte size — but bytes are the - * only thing cheap enough to test before parsing. The blocking mount is what - * pins this ceiling; past it, fall back to source mode (Monaco) with a per-file - * "Open anyway" escape hatch. Real headroom needs #7056. */ -export const RICH_MARKDOWN_MAX_SIZE_BYTES = 300 * 1024 +/** Why: ProseMirror renders the whole document without virtualization. After the + * parser/highlighter work in #17134/#17147/#17158, M-series Electron measurements + * on distinct code blocks every ~600 bytes put visible mount / longest task at + * 300 KB 0.70 / 0.66 s · 450 KB 1.09 / 1.02 s · 600 KB 1.49 / 1.41 s, with + * 600 KB typing at 38 ms median / 39 ms p95. Bytes remain the only cheap + * pre-parse guard; larger files use source mode with an "Open anyway" escape hatch. */ +export const RICH_MARKDOWN_MAX_SIZE_BYTES = 600 * 1024 export const DEFAULT_EDITOR_AUTO_SAVE_DELAY_MS = 1000 export const MIN_EDITOR_AUTO_SAVE_DELAY_MS = 250 From 8b01cf1a48d91cf28a87451cb5412d5c7626e46c Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 29 Aug 2026 15:07:38 -0700 Subject: [PATCH 26/27] Clarify automation history unavailability and improve recovery UX (#17284) * Clarify automation history unavailability and improve recovery UX - Improve error message to explain run history is unavailable due to host version requirements, not automation failure - Hide misleading "0 runs" count badge when history is unavailable - Deep-link "update server" recovery actions to specific runtime environment in settings instead of pane root - Add test coverage for run count hiding and recovery targeting * Watch for deep-linked settings targets that render asynchronously Some settings panes (such as Remote Orca Servers) fetch and render their rows asynchronously. Deep links can name targets that don't exist yet, but the scroll effect has no way to know when they finally mount. Add a MutationObserver-based watcher to detect when async rows appear and trigger scrolling. --- .../automations/AutomationRunHistory.test.tsx | 3 +- .../automations/AutomationRunHistory.tsx | 2 +- .../AutomationsDetailPane.run-count.test.tsx | 88 ++++++++++++++ .../automations/AutomationsDetailPane.tsx | 4 +- .../AutomationsPage.notice-recovery.test.tsx | 13 ++- .../automation-captured-owner.test.ts | 6 + .../automations/automation-captured-owner.ts | 2 +- .../automation-host-recovery.test.ts | 14 ++- .../automations/automation-host-recovery.ts | 8 +- .../use-automation-host-catalog.ts | 2 +- .../src/components/settings/Settings.tsx | 22 ++++ .../settings-deep-link-target-watcher.test.ts | 108 ++++++++++++++++++ .../settings-deep-link-target-watcher.ts | 54 +++++++++ src/renderer/src/i18n/locales/en.json | 4 +- 14 files changed, 315 insertions(+), 15 deletions(-) create mode 100644 src/renderer/src/components/automations/AutomationsDetailPane.run-count.test.tsx create mode 100644 src/renderer/src/components/settings/settings-deep-link-target-watcher.test.ts create mode 100644 src/renderer/src/components/settings/settings-deep-link-target-watcher.ts diff --git a/src/renderer/src/components/automations/AutomationRunHistory.test.tsx b/src/renderer/src/components/automations/AutomationRunHistory.test.tsx index 7742f74b26a..41d839e615a 100644 --- a/src/renderer/src/components/automations/AutomationRunHistory.test.tsx +++ b/src/renderer/src/components/automations/AutomationRunHistory.test.tsx @@ -113,7 +113,8 @@ describe('AutomationRunHistory unanswered history', () => { expect(container.textContent).not.toContain('No runs yet.') // "0 runs" is a count of something nobody managed to read. expect(container.textContent).not.toContain('0 runs') - expect(container.textContent).toContain('Run history could not be loaded') + expect(container.textContent).toContain('Run history is unavailable from this host') + expect(container.textContent).toContain('does not mean the automation failed or has no runs') expect(container.textContent).toContain('web-01 is not connected') }) diff --git a/src/renderer/src/components/automations/AutomationRunHistory.tsx b/src/renderer/src/components/automations/AutomationRunHistory.tsx index cd2ece0d7dc..13395dae6ad 100644 --- a/src/renderer/src/components/automations/AutomationRunHistory.tsx +++ b/src/renderer/src/components/automations/AutomationRunHistory.tsx @@ -167,7 +167,7 @@ export function AutomationRunHistory({

{translate( 'auto.components.automations.AutomationRunHistory.historyUnavailable', - 'Run history could not be loaded for this automation.' + 'Run history is unavailable from this host. This does not mean the automation failed or has no runs.' )}

diff --git a/src/renderer/src/components/automations/AutomationsDetailPane.run-count.test.tsx b/src/renderer/src/components/automations/AutomationsDetailPane.run-count.test.tsx new file mode 100644 index 00000000000..740f1130405 --- /dev/null +++ b/src/renderer/src/components/automations/AutomationsDetailPane.run-count.test.tsx @@ -0,0 +1,88 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { AutomationsDetailPane } from './AutomationsDetailPane' +import { makeAutomation } from './automations-page-fixtures' + +let root: Root | null = null + +beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true +}) + +afterEach(async () => { + await act(async () => root?.unmount()) + root = null + document.body.innerHTML = '' +}) + +async function renderRunsTab(historyUnavailable: boolean): Promise { + const container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + await act(async () => { + root?.render( + + ) + }) + const runsTab = [...container.querySelectorAll('[role="tab"]')].find((tab) => + tab.textContent?.includes('Runs') + ) + if (!runsTab) { + throw new Error('Runs tab was not rendered') + } + return runsTab +} + +describe('AutomationsDetailPane run count', () => { + it('does not report zero when run history is unavailable', async () => { + expect((await renderRunsTab(true)).textContent?.trim()).toBe('Runs') + }) + + it('reports zero when the host answered with an empty history', async () => { + expect((await renderRunsTab(false)).textContent).toContain('0') + }) +}) diff --git a/src/renderer/src/components/automations/AutomationsDetailPane.tsx b/src/renderer/src/components/automations/AutomationsDetailPane.tsx index 7eb2ff13c1d..0affe311f2c 100644 --- a/src/renderer/src/components/automations/AutomationsDetailPane.tsx +++ b/src/renderer/src/components/automations/AutomationsDetailPane.tsx @@ -213,7 +213,9 @@ export function AutomationsDetailPane({ {translate('auto.components.automations.AutomationsPage.0e110a3469', 'Runs')}{' '} - {selectedRuns.length} + {selectedRunsNotice ? null : ( + {selectedRuns.length} + )} diff --git a/src/renderer/src/components/automations/AutomationsPage.notice-recovery.test.tsx b/src/renderer/src/components/automations/AutomationsPage.notice-recovery.test.tsx index 503d0877ed5..d99cfb91dac 100644 --- a/src/renderer/src/components/automations/AutomationsPage.notice-recovery.test.tsx +++ b/src/renderer/src/components/automations/AutomationsPage.notice-recovery.test.tsx @@ -19,6 +19,7 @@ import { mocks, renderPage, runtimeHost, + RUNTIME_ID, RUNTIME_REPO_ID, RUNTIME_SELF_FILTER, RUNTIME_WORKSPACE_ID, @@ -88,7 +89,11 @@ describe('AutomationsPage notice recovery', () => { }) expect(settings.open).toHaveBeenCalled() - expect(settings.target).toHaveBeenCalledWith({ pane: 'servers', repoId: null }) + expect(settings.target).toHaveBeenCalledWith({ + pane: 'servers', + repoId: null, + sectionId: RUNTIME_ID + }) }) it('sends a row Update server to the row own host, not to the unfiltered list', async () => { @@ -116,6 +121,10 @@ describe('AutomationsPage notice recovery', () => { // All hosts selects no host, so only the row's captured owner names one. expect(settings.open).toHaveBeenCalled() - expect(settings.target).toHaveBeenCalledWith({ pane: 'servers', repoId: null }) + expect(settings.target).toHaveBeenCalledWith({ + pane: 'servers', + repoId: null, + sectionId: RUNTIME_ID + }) }) }) diff --git a/src/renderer/src/components/automations/automation-captured-owner.test.ts b/src/renderer/src/components/automations/automation-captured-owner.test.ts index ffc998a5b36..ca22b0286e0 100644 --- a/src/renderer/src/components/automations/automation-captured-owner.test.ts +++ b/src/renderer/src/components/automations/automation-captured-owner.test.ts @@ -92,6 +92,12 @@ describe('captured owner availability', () => { const availability = automationActionAvailability(legacy, 'delete') expect(availability.kind).toBe('blocked') expect(availability.kind === 'blocked' && availability.block.recovery).toBe('update-server') + expect(availability.kind === 'blocked' && availability.block.message).toContain( + 'needs an update' + ) + expect(availability.kind === 'blocked' && availability.block.message).toContain( + 'Scheduled runs may still continue' + ) }) it('reports a row with no metadata as uncaptured rather than blocked', () => { diff --git a/src/renderer/src/components/automations/automation-captured-owner.ts b/src/renderer/src/components/automations/automation-captured-owner.ts index 761dd787e78..7a95b517fe5 100644 --- a/src/renderer/src/components/automations/automation-captured-owner.ts +++ b/src/renderer/src/components/automations/automation-captured-owner.ts @@ -71,7 +71,7 @@ function unfencedBlock(): AutomationActionBlock { reason: 'unfenced', message: translate( 'auto.components.automations.capturedOwner.unfenced', - 'This host cannot confirm which automation it is changing, so this automation is read-only here.' + 'Orca on this host needs an update before you can view run history or manage this automation. Scheduled runs may still continue on the host.' ), recovery: 'update-server' } diff --git a/src/renderer/src/components/automations/automation-host-recovery.test.ts b/src/renderer/src/components/automations/automation-host-recovery.test.ts index 8d6419fb128..871f9343114 100644 --- a/src/renderer/src/components/automations/automation-host-recovery.test.ts +++ b/src/renderer/src/components/automations/automation-host-recovery.test.ts @@ -35,9 +35,10 @@ const DESKTOP_SSH = entry({ stableRef: { authority: { kind: 'desktop' }, selector: { kind: 'ssh', targetId: 't1' } }, kind: 'ssh' }) +const RUNTIME_ENVIRONMENT_ID = 'gpu' const RUNTIME_SSH = entry({ stableRef: { - authority: { kind: 'runtime', environmentId: 'gpu' }, + authority: { kind: 'runtime', environmentId: RUNTIME_ENVIRONMENT_ID }, selector: { kind: 'ssh', targetId: 't1' } }, kind: 'ssh' @@ -65,7 +66,7 @@ describe('automation host recovery', () => { { ...RUNTIME_SSH, authorityHealth: 'unavailable' }, target ) - expect(target.connectRuntimeEnvironment).toHaveBeenCalledWith('gpu') + expect(target.connectRuntimeEnvironment).toHaveBeenCalledWith(RUNTIME_ENVIRONMENT_ID) expect(target.connectSshTarget).not.toHaveBeenCalled() }) @@ -78,10 +79,15 @@ describe('automation host recovery', () => { expect(target.retry).toHaveBeenCalledWith(desktopSelf) }) - it('sends a runtime to the servers pane, since nothing here updates a host', () => { + it('deep-links a runtime to its update row in Remote Orca Servers settings', () => { const target = deps() runAutomationHostRecovery('update-server', RUNTIME_SSH, target) - expect(target.openSettings).toHaveBeenCalledWith({ pane: 'servers', repoId: null }) + expect(target.openSettings).toHaveBeenCalledWith({ + pane: 'servers', + repoId: null, + // Runtime environment IDs are nested scroll anchors within the pane. + sectionId: RUNTIME_ENVIRONMENT_ID + }) }) it('sends a stale desktop SSH registration to the SSH pane instead', () => { diff --git a/src/renderer/src/components/automations/automation-host-recovery.ts b/src/renderer/src/components/automations/automation-host-recovery.ts index bc71f08ff18..d443d485b1d 100644 --- a/src/renderer/src/components/automations/automation-host-recovery.ts +++ b/src/renderer/src/components/automations/automation-host-recovery.ts @@ -20,10 +20,14 @@ export type AutomationHostRecoveryDeps = { openSettings: (target: SettingsNavigationTarget) => void } -/** Where a host's version is managed; the app cannot update a host on its own. */ +/** The Remote Orca Servers pane owns each runtime's version status and update action. */ function versionSettingsTarget(entry: AutomationHostCatalogEntry): SettingsNavigationTarget { if (entry.stableRef.authority.kind === 'runtime') { - return { pane: 'servers', repoId: null } + return { + pane: 'servers', + repoId: null, + sectionId: entry.stableRef.authority.environmentId + } } // A desktop SSH host with no registration generation is a stale registration, // repaired by re-adding the target rather than by updating anything. diff --git a/src/renderer/src/components/automations/use-automation-host-catalog.ts b/src/renderer/src/components/automations/use-automation-host-catalog.ts index bd7cd8dbb67..e40a4393e20 100644 --- a/src/renderer/src/components/automations/use-automation-host-catalog.ts +++ b/src/renderer/src/components/automations/use-automation-host-catalog.ts @@ -248,8 +248,8 @@ export function useAutomationHostCatalog( void window.api.runtimeEnvironments.connect({ selector: environmentId }) }, openSettings: (target) => { - openSettingsPage() openSettingsTarget(target) + openSettingsPage() } }), [controller, openSettingsPage, openSettingsTarget, pairingRevision] diff --git a/src/renderer/src/components/settings/Settings.tsx b/src/renderer/src/components/settings/Settings.tsx index fb928aa2ceb..f51a7fe22f5 100644 --- a/src/renderer/src/components/settings/Settings.tsx +++ b/src/renderer/src/components/settings/Settings.tsx @@ -120,6 +120,10 @@ import { getLinearAgentSkillNavInstallStatus } from '@/lib/agent-skill-nav-install-status' import { deriveNeededSectionIds, getInitialMountedSectionIds } from './settings-load-performance' +import { + watchForSettingsDeepLinkTarget, + type SettingsDeepLinkTargetWatch +} from './settings-deep-link-target-watcher' import { translate } from '@/i18n/i18n' import { getProjectHostSetupProjectionFromState } from '../../store/selectors' import { getRepoHostIdentity } from '../../store/slices/repo-host-identity' @@ -277,6 +281,13 @@ function cancelPendingSettingsSubsectionScrollFrame( } } +function cancelPendingSettingsDeepLinkTargetWatch( + watchRef: MutableRefObject +): void { + watchRef.current?.cancel() + watchRef.current = null +} + function isEditableTarget(target: EventTarget | null): boolean { if (!(target instanceof HTMLElement)) { return false @@ -399,6 +410,7 @@ function Settings(): React.JSX.Element { const pendingNavSectionRef = useRef(null) const pendingScrollTargetRef = useRef(null) const pendingSubsectionScrollFrameRef = useRef(null) + const pendingScrollTargetWatchRef = useRef(null) const repoHooksRequestSeqRef = useRef(0) const shortcutsEscapeConfirmUntilRef = useRef(0) const sourceControlAiWriteQueueRef = useRef>(Promise.resolve()) @@ -446,6 +458,7 @@ function Settings(): React.JSX.Element { } // Why: cancel pending subsection jumps with the scroll container so a stale deep-link frame can't run after close. cancelPendingSettingsSubsectionScrollFrame(pendingSubsectionScrollFrameRef) + cancelPendingSettingsDeepLinkTargetWatch(pendingScrollTargetWatchRef) }, []) useEffect(() => { @@ -1043,6 +1056,8 @@ function Settings(): React.JSX.Element { useEffect(() => { const scrollTargetId = pendingScrollTargetRef.current const pendingNavSectionId = pendingNavSectionRef.current + // Why: this pass re-decides whether to wait for the target, so drop any watch armed by the previous one. + cancelPendingSettingsDeepLinkTargetWatch(pendingScrollTargetWatchRef) // Why: subsection deep links clear a stale filter that could hide the target row; pane-level links keep it to force-open the matching section. if ( @@ -1069,6 +1084,13 @@ function Settings(): React.JSX.Element { if (scrollTargetId !== pendingNavSectionId) { // Why: target can arrive before the lazy section mounts; keep pending refs until it does. if (!getSettingsScrollTarget(scrollTargetId, container)) { + // Why: async panes (the server list, say) mount rows after this pass, and nothing else re-runs the effect. + pendingScrollTargetWatchRef.current = watchForSettingsDeepLinkTarget({ + root: container, + isTargetPresent: () => + getSettingsScrollTarget(scrollTargetId, contentScrollRef.current) !== null, + onTargetPresent: () => setPendingNavRequestTick((tick) => tick + 1) + }) return } const scrollToSubsection = (): void => { diff --git a/src/renderer/src/components/settings/settings-deep-link-target-watcher.test.ts b/src/renderer/src/components/settings/settings-deep-link-target-watcher.test.ts new file mode 100644 index 00000000000..f3d87ee07c6 --- /dev/null +++ b/src/renderer/src/components/settings/settings-deep-link-target-watcher.test.ts @@ -0,0 +1,108 @@ +// @vitest-environment happy-dom + +import { afterEach, describe, expect, it, vi } from 'vitest' +import { watchForSettingsDeepLinkTarget } from './settings-deep-link-target-watcher' + +function flushMutations(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)) +} + +afterEach(() => { + document.body.innerHTML = '' +}) + +describe('watchForSettingsDeepLinkTarget', () => { + it('reports the target once the async rows render', async () => { + const container = document.createElement('div') + document.body.append(container) + const onTargetPresent = vi.fn() + watchForSettingsDeepLinkTarget({ + root: container, + isTargetPresent: () => container.querySelector('[data-settings-section="env-1"]') !== null, + onTargetPresent + }) + + const row = document.createElement('div') + row.dataset.settingsSection = 'env-1' + container.append(row) + await flushMutations() + + expect(onTargetPresent).toHaveBeenCalledTimes(1) + }) + + it('keeps waiting while unrelated rows render', async () => { + const container = document.createElement('div') + document.body.append(container) + const onTargetPresent = vi.fn() + watchForSettingsDeepLinkTarget({ + root: container, + isTargetPresent: () => container.querySelector('[data-settings-section="env-1"]') !== null, + onTargetPresent + }) + + const other = document.createElement('div') + other.dataset.settingsSection = 'env-2' + container.append(other) + await flushMutations() + expect(onTargetPresent).not.toHaveBeenCalled() + + const row = document.createElement('div') + row.dataset.settingsSection = 'env-1' + container.append(row) + await flushMutations() + expect(onTargetPresent).toHaveBeenCalledTimes(1) + }) + + it('reports at most once even as more rows arrive', async () => { + const container = document.createElement('div') + document.body.append(container) + const onTargetPresent = vi.fn() + watchForSettingsDeepLinkTarget({ + root: container, + isTargetPresent: () => container.querySelector('[data-settings-section="env-1"]') !== null, + onTargetPresent + }) + + const row = document.createElement('div') + row.dataset.settingsSection = 'env-1' + container.append(row) + await flushMutations() + container.append(document.createElement('div')) + await flushMutations() + + expect(onTargetPresent).toHaveBeenCalledTimes(1) + }) + + it('stops reporting after cancel', async () => { + const container = document.createElement('div') + document.body.append(container) + const onTargetPresent = vi.fn() + const watch = watchForSettingsDeepLinkTarget({ + root: container, + isTargetPresent: () => true, + onTargetPresent + }) + watch.cancel() + + container.append(document.createElement('div')) + await flushMutations() + + expect(onTargetPresent).not.toHaveBeenCalled() + }) + + it('falls back to the document body when the pane has no scroll container yet', async () => { + const onTargetPresent = vi.fn() + watchForSettingsDeepLinkTarget({ + root: null, + isTargetPresent: () => document.querySelector('[data-settings-section="env-1"]') !== null, + onTargetPresent + }) + + const row = document.createElement('div') + row.dataset.settingsSection = 'env-1' + document.body.append(row) + await flushMutations() + + expect(onTargetPresent).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/components/settings/settings-deep-link-target-watcher.ts b/src/renderer/src/components/settings/settings-deep-link-target-watcher.ts new file mode 100644 index 00000000000..30d7ee1b103 --- /dev/null +++ b/src/renderer/src/components/settings/settings-deep-link-target-watcher.ts @@ -0,0 +1,54 @@ +/** + * A deep link can name a row that does not exist yet: panes such as Remote Orca + * Servers render their rows only after an async fetch resolves. The scroll + * effect parks its target in a ref, but nothing re-runs it when those rows + * finally mount, so this watcher reports the moment they do. + */ + +/** Give up eventually; a target that never mounts must not leave an observer alive. */ +const TARGET_WAIT_TIMEOUT_MS = 5000 + +export type SettingsDeepLinkTargetWatch = { + cancel: () => void +} + +export function watchForSettingsDeepLinkTarget(args: { + /** Subtree to observe; falls back to the document body when the pane has no scroll container yet. */ + root: HTMLElement | null + isTargetPresent: () => boolean + onTargetPresent: () => void +}): SettingsDeepLinkTargetWatch { + const root = args.root ?? (typeof document === 'undefined' ? null : document.body) + if (!root || typeof MutationObserver === 'undefined') { + return { cancel: () => {} } + } + + let settled = false + let observer: MutationObserver | null = null + let timeout: number | null = null + + const finish = (): void => { + if (settled) { + return + } + settled = true + observer?.disconnect() + observer = null + if (timeout !== null) { + window.clearTimeout(timeout) + timeout = null + } + } + + observer = new MutationObserver(() => { + if (settled || !args.isTargetPresent()) { + return + } + finish() + args.onTargetPresent() + }) + observer.observe(root, { subtree: true, childList: true }) + timeout = window.setTimeout(finish, TARGET_WAIT_TIMEOUT_MS) + + return { cancel: finish } +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 23f03820b50..6660bb6c200 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -15405,7 +15405,7 @@ "53fc5f07ab": "Run history", "a00e38d1a3": "n/a", "fdb3caa8fb": "known", - "historyUnavailable": "Run history could not be loaded for this automation." + "historyUnavailable": "Run history is unavailable from this host. This does not mean the automation failed or has no runs." }, "AutomationRunPageFrame": { "40a511bed4": "Run context", @@ -15727,7 +15727,7 @@ }, "capturedOwner": { "orphan": "This automation has no host to run on. Delete it, or re-add the host it belongs to.", - "unfenced": "This host cannot confirm which automation it is changing, so this automation is read-only here." + "unfenced": "Orca on this host needs an update before you can view run history or manage this automation. Scheduled runs may still continue on the host." }, "hostStatus": { "authority": { From 2214d29f1513fae7631d6a2b774e285f497bbf03 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 29 Aug 2026 15:43:36 -0700 Subject: [PATCH 27/27] fix(browser): close guest-owned split tab (#17281) * fix(browser): close guest-owned split tab * fix: check sourceId before toggling floating panel on close The empty-panel toggle is the ambient fallback only. Guest-initiated closes (with sourceId) target the main workspace and should not toggle the panel. * test(browser-split-shortcuts): remove terminal-mirrors close test and un Removes test case that verified Cmd+W closes guest-owned browser splits when active-tab mirrors point to a terminal, along with the helper function and unused fixture properties that only that test required. --- .../browser-guest-shortcut-dispatch.ts | 4 +- .../browser-guest-shortcut-forwarding.test.ts | 8 +- .../browser-manager-guest-shortcuts.test.ts | 4 +- src/preload/api/ui-command-event-api.ts | 4 +- ...close-active-tab-payload-admission.test.ts | 22 +++++ .../close-active-tab-payload-admission.ts | 25 ++++++ src/preload/index.ts | 13 ++- .../ipc-events-close-routing-test-harness.ts | 2 +- .../ipc-events/tab-lifecycle-ipc-bridge.ts | 25 ++++-- ...s-close-routing-active-browser-tab.test.ts | 73 +++++++++++++++++ .../browser-workspace-source-resolution.ts | 27 +++++++ .../floating-workspace-terminal-actions.ts | 16 ++-- ...pec.ts => browser-split-shortcuts.spec.ts} | 81 ++++++++++++++----- 13 files changed, 258 insertions(+), 46 deletions(-) create mode 100644 src/preload/close-active-tab-payload-admission.test.ts create mode 100644 src/preload/close-active-tab-payload-admission.ts create mode 100644 src/renderer/src/lib/browser-workspace-source-resolution.ts rename tests/e2e/{browser-split-find-shortcut.spec.ts => browser-split-shortcuts.spec.ts} (83%) diff --git a/src/main/browser/browser-guest-shortcut-dispatch.ts b/src/main/browser/browser-guest-shortcut-dispatch.ts index fa4bd966994..5d24dce43ea 100644 --- a/src/main/browser/browser-guest-shortcut-dispatch.ts +++ b/src/main/browser/browser-guest-shortcut-dispatch.ts @@ -160,7 +160,9 @@ export function forwardGuestShortcutInput( if (isFloatingGuest) { renderer.send('ui:closeFloatingItem', { sourceId: browserTabId }) } else { - renderer.send('ui:closeActiveTab') + // Why: carry the guest's page id — activeTabType/activeBrowserTabId can be stale in split + // layouts (guest focus doesn't reach the group's focus-capture), silently dropping the close. + renderer.send('ui:closeActiveTab', { sourceId: browserTabId }) } } else if (keybindingMatchesAction('tab.nextSameType', input, process.platform, keybindings)) { renderer.send('ui:switchTab', 1) diff --git a/src/main/browser/browser-guest-shortcut-forwarding.test.ts b/src/main/browser/browser-guest-shortcut-forwarding.test.ts index 19bf03bd2f8..f7f35917fa9 100644 --- a/src/main/browser/browser-guest-shortcut-forwarding.test.ts +++ b/src/main/browser/browser-guest-shortcut-forwarding.test.ts @@ -872,7 +872,9 @@ describe('setupGuestShortcutForwarding', () => { expect(rendererSendMock).toHaveBeenCalledWith('ui:closeFloatingItem', { sourceId: browserTabId }) - expect(rendererSendMock).not.toHaveBeenCalledWith('ui:closeActiveTab') + expect(rendererSendMock.mock.calls.some(([channel]) => channel === 'ui:closeActiveTab')).toBe( + false + ) }) it('routes workspace/tab index chords to ui:selectFloatingIndex for a floating guest', () => { @@ -903,7 +905,7 @@ describe('setupGuestShortcutForwarding', () => { triggerBeforeInput(closeInput) triggerBeforeInput(workspaceIndexInput) - expect(rendererSendMock).toHaveBeenCalledWith('ui:closeActiveTab') + expect(rendererSendMock).toHaveBeenCalledWith('ui:closeActiveTab', { sourceId: browserTabId }) expect(rendererSendMock).toHaveBeenCalledWith('ui:jumpToWorktreeIndex', 0) expect(rendererSendMock).not.toHaveBeenCalledWith('ui:closeFloatingItem', expect.anything()) expect(rendererSendMock).not.toHaveBeenCalledWith('ui:selectFloatingIndex', expect.anything()) @@ -918,7 +920,7 @@ describe('setupGuestShortcutForwarding', () => { triggerBeforeInput(closeInput) - expect(rendererSendMock).toHaveBeenCalledWith('ui:closeActiveTab') + expect(rendererSendMock).toHaveBeenCalledWith('ui:closeActiveTab', { sourceId: browserTabId }) expect(rendererSendMock).not.toHaveBeenCalledWith('ui:closeFloatingItem', expect.anything()) }) diff --git a/src/main/browser/browser-manager-guest-shortcuts.test.ts b/src/main/browser/browser-manager-guest-shortcuts.test.ts index cc9af9b6329..fa84fd99d50 100644 --- a/src/main/browser/browser-manager-guest-shortcuts.test.ts +++ b/src/main/browser/browser-manager-guest-shortcuts.test.ts @@ -297,7 +297,9 @@ describe('browserManager', () => { expect(rendererSendMock).toHaveBeenNthCalledWith(1, 'ui:newBrowserTab') expect(rendererSendMock).toHaveBeenNthCalledWith(2, 'ui:newTerminalTab') - expect(rendererSendMock).toHaveBeenNthCalledWith(3, 'ui:closeActiveTab') + expect(rendererSendMock).toHaveBeenNthCalledWith(3, 'ui:closeActiveTab', { + sourceId: 'browser-1' + }) expect(rendererSendMock).toHaveBeenNthCalledWith(4, 'ui:switchTabAcrossAllTypes', 1) expect(rendererSendMock).toHaveBeenNthCalledWith(5, 'ui:switchTab', 1) expect(rendererSendMock).toHaveBeenNthCalledWith(6, 'ui:switchTerminalTab', 1) diff --git a/src/preload/api/ui-command-event-api.ts b/src/preload/api/ui-command-event-api.ts index c3eefb02115..f287d747aee 100644 --- a/src/preload/api/ui-command-event-api.ts +++ b/src/preload/api/ui-command-event-api.ts @@ -32,6 +32,8 @@ import type { SessionTabCloseResponse } from '../../shared/session-tab-close' +export type CloseActiveTabPayload = { sourceId: string } + export type UiCommandEventApi = { get: () => Promise set: (args: Partial) => Promise @@ -105,7 +107,7 @@ export type UiCommandEventApi = { onBrowserHistoryNavigate: (callback: (direction: 'back' | 'forward') => void) => () => void onZoomBrowserPage: (callback: (direction: 'in' | 'out' | 'reset') => void) => () => void onHardReloadBrowserPage: (callback: () => void) => () => void - onCloseActiveTab: (callback: () => void) => () => void + onCloseActiveTab: (callback: (payload?: CloseActiveTabPayload) => void) => () => void onCloseFloatingItem: (callback: (payload: { sourceId: string }) => void) => () => void onSelectFloatingIndex: (callback: (payload: { index: number }) => void) => () => void onSwitchTab: (callback: (direction: 1 | -1) => void) => () => void diff --git a/src/preload/close-active-tab-payload-admission.test.ts b/src/preload/close-active-tab-payload-admission.test.ts new file mode 100644 index 00000000000..76f0270c34c --- /dev/null +++ b/src/preload/close-active-tab-payload-admission.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { admitCloseActiveTabPayload } from './close-active-tab-payload-admission' + +describe('admitCloseActiveTabPayload', () => { + it('preserves the legacy omitted payload', () => { + expect(admitCloseActiveTabPayload(undefined)).toEqual({ kind: 'legacy' }) + }) + + it('admits a nonempty source id', () => { + expect(admitCloseActiveTabPayload({ sourceId: 'page-1' })).toEqual({ + kind: 'source', + payload: { sourceId: 'page-1' } + }) + }) + + it.each([null, {}, [], { sourceId: '' }, { sourceId: 1 }, 'page-1'])( + 'rejects malformed payload %#', + (payload) => { + expect(admitCloseActiveTabPayload(payload)).toEqual({ kind: 'invalid' }) + } + ) +}) diff --git a/src/preload/close-active-tab-payload-admission.ts b/src/preload/close-active-tab-payload-admission.ts new file mode 100644 index 00000000000..17428cb6a7a --- /dev/null +++ b/src/preload/close-active-tab-payload-admission.ts @@ -0,0 +1,25 @@ +import type { CloseActiveTabPayload } from './api/ui-command-event-api' + +export type AdmittedCloseActiveTabPayload = + | { kind: 'legacy' } + | { kind: 'source'; payload: CloseActiveTabPayload } + | { kind: 'invalid' } + +export function admitCloseActiveTabPayload(value: unknown): AdmittedCloseActiveTabPayload { + if (value === undefined) { + return { kind: 'legacy' } + } + if ( + typeof value === 'object' && + value !== null && + !Array.isArray(value) && + typeof (value as Record).sourceId === 'string' && + (value as Record).sourceId !== '' + ) { + return { + kind: 'source', + payload: { sourceId: (value as Record).sourceId as string } + } + } + return { kind: 'invalid' } +} diff --git a/src/preload/index.ts b/src/preload/index.ts index a474eb7560e..c13e7a30a7d 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -3,6 +3,8 @@ import { contextBridge, ipcRenderer, webFrame, webUtils } from 'electron' import { electronAPI } from '@electron-toolkit/preload' import { preloadE2EConfig } from './e2e-config' import { glApi } from './gitlab' +import { admitCloseActiveTabPayload } from './close-active-tab-payload-admission' +import type { CloseActiveTabPayload } from './api/ui-command-event-api' import type { SkillDeletePlan, SkillDeleteRequest, @@ -4050,8 +4052,15 @@ const api = { ipcRenderer.on('ui:hardReloadBrowserPage', listener) return () => ipcRenderer.removeListener('ui:hardReloadBrowserPage', listener) }, - onCloseActiveTab: (callback: () => void): (() => void) => { - const listener = (_event: Electron.IpcRendererEvent) => callback() + onCloseActiveTab: (callback: (payload?: CloseActiveTabPayload) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, payload?: unknown): void => { + const admitted = admitCloseActiveTabPayload(payload) + if (admitted.kind === 'legacy') { + callback() + } else if (admitted.kind === 'source') { + callback(admitted.payload) + } + } ipcRenderer.on('ui:closeActiveTab', listener) return () => ipcRenderer.removeListener('ui:closeActiveTab', listener) }, diff --git a/src/renderer/src/hooks/ipc-events-close-routing-test-harness.ts b/src/renderer/src/hooks/ipc-events-close-routing-test-harness.ts index eede0d6fa8e..d3424f8fc32 100644 --- a/src/renderer/src/hooks/ipc-events-close-routing-test-harness.ts +++ b/src/renderer/src/hooks/ipc-events-close-routing-test-harness.ts @@ -6,7 +6,7 @@ export type RequestTabCloseListener = (data: { tabId: string | null worktreeId?: string }) => void -export type CloseActiveTabListener = () => void +export type CloseActiveTabListener = (payload?: { sourceId: string }) => void export type CloseFloatingItemListener = (payload: { sourceId: string }) => void export type SelectFloatingIndexListener = (payload: { index: number }) => void export type CloseTerminalListener = (data: { tabId: string; paneRuntimeId?: number | null }) => void diff --git a/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts index b079dc9ad29..bf60e9dc0a1 100644 --- a/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts @@ -25,6 +25,7 @@ import { } from '@/lib/floating-workspace-guest-bridge' import { useAppStore } from '../../store' +import { resolveBrowserWorkspaceOwner } from '../../lib/browser-workspace-source-resolution' import { handleSwitchRecentTab, handleSwitchTab, @@ -86,15 +87,29 @@ export function registerTabLifecycleIpcBridge(unsubs: (() => void)[]): void { ) unsubs.push( - window.api.ui.onCloseActiveTab(() => { - if (isEmptyFloatingWorkspacePanelVisible()) { + window.api.ui.onCloseActiveTab((payload) => { + // Why: the empty-panel toggle is the ambient fallback only. A guest-originated close names a + // main-workspace target, so an open-but-empty floating panel must not swallow it. + if (!payload?.sourceId && isEmptyFloatingWorkspacePanelVisible()) { window.dispatchEvent(new Event(TOGGLE_FLOATING_TERMINAL_EVENT)) return } const store = useAppStore.getState() - if (store.activeTabType === 'browser' && store.activeBrowserTabId) { - const tabId = store.activeBrowserTabId - const worktreeId = store.activeWorktreeId + // Why: a guest-originated close names its own page; the activeTabType mirror goes stale in + // split layouts (guest focus never reaches the group's focus-capture), so trust the source id. + const explicitTarget = payload?.sourceId + ? resolveBrowserWorkspaceOwner(store, payload.sourceId) + : null + if (payload?.sourceId && !explicitTarget) { + // Stale id (guest closed between keydown and IPC) = no-op, never the ambient fallback. + return + } + if (explicitTarget || (store.activeTabType === 'browser' && store.activeBrowserTabId)) { + const tabId = explicitTarget?.workspaceId ?? store.activeBrowserTabId + const worktreeId = explicitTarget?.worktreeId ?? store.activeWorktreeId + if (!tabId) { + return + } const closeActiveBrowserTab = (): void => { const currentStore = useAppStore.getState() if (!worktreeId) { diff --git a/src/renderer/src/hooks/useIpcEvents-close-routing-active-browser-tab.test.ts b/src/renderer/src/hooks/useIpcEvents-close-routing-active-browser-tab.test.ts index e9a4097fcea..4225e309427 100644 --- a/src/renderer/src/hooks/useIpcEvents-close-routing-active-browser-tab.test.ts +++ b/src/renderer/src/hooks/useIpcEvents-close-routing-active-browser-tab.test.ts @@ -184,4 +184,77 @@ describe('useIpcEvents Close Tab on the active browser tab', () => { expect(closeUnifiedTab).not.toHaveBeenCalled() expect(closeBrowserTab).toHaveBeenCalledWith('workspace-1', undefined) }) + + // Why: in split layouts the activeTabType mirror can say 'terminal' while a browser guest holds + // focus (guest focus never reaches the group's focus-capture); the guest-forwarded source id must + // close the guest's own workspace anyway. + it('closes the source workspace when the active-tab mirror points at a terminal', async () => { + const listenerRef: { current: CloseActiveTabListener | null } = { current: null } + await useIpcEventsForCloseRouting({ + closeActiveTabListenerRef: listenerRef, + getState: () => ({ + ...activeBrowserWorkspace({}), + activeTabType: 'terminal', + activeBrowserTabId: null, + browserPagesByWorkspace: { + // Ownership comes from the canonical maps, not denormalized page fields. + 'workspace-1': [{ id: 'page-1', workspaceId: 'stale-workspace', worktreeId: 'stale-wt' }] + } + }) + }) + + requireListener(listenerRef)({ sourceId: 'page-1' }) + + expect(closeBrowserTab).toHaveBeenCalledWith('workspace-1', undefined) + expect(destroyWorkspaceWebviews).toHaveBeenCalled() + }) + + // Why: an open-but-empty floating panel is the ambient close fallback only; a guest-forwarded + // source id names a main-workspace target and must not be swallowed by the panel toggle. + it('closes the source workspace even while an empty floating panel is visible', async () => { + vi.stubGlobal('document', { querySelector: () => ({}) }) + const listenerRef: { current: CloseActiveTabListener | null } = { current: null } + await useIpcEventsForCloseRouting({ + closeActiveTabListenerRef: listenerRef, + getState: () => activeBrowserWorkspace({}) + }) + + requireListener(listenerRef)({ sourceId: 'page-1' }) + + expect(closeBrowserTab).toHaveBeenCalledWith('workspace-1', undefined) + expect(destroyWorkspaceWebviews).toHaveBeenCalled() + }) + + it('no-ops on a stale source id instead of closing the ambient active tab', async () => { + const listenerRef: { current: CloseActiveTabListener | null } = { current: null } + await useIpcEventsForCloseRouting({ + closeActiveTabListenerRef: listenerRef, + getState: () => activeBrowserWorkspace({}) + }) + + requireListener(listenerRef)({ sourceId: 'page-that-no-longer-exists' }) + + expect(closeBrowserTab).not.toHaveBeenCalled() + expect(closeUnifiedTab).not.toHaveBeenCalled() + expect(closeWebRuntimeSessionTab).not.toHaveBeenCalled() + }) + + it('no-ops when a source page belongs to no live workspace', async () => { + const listenerRef: { current: CloseActiveTabListener | null } = { current: null } + await useIpcEventsForCloseRouting({ + closeActiveTabListenerRef: listenerRef, + getState: () => ({ + ...activeBrowserWorkspace({}), + browserPagesByWorkspace: { + orphan: [{ id: 'orphan-page', workspaceId: 'workspace-1', worktreeId: 'wt-1' }] + } + }) + }) + + requireListener(listenerRef)({ sourceId: 'orphan-page' }) + + expect(closeBrowserTab).not.toHaveBeenCalled() + expect(closeUnifiedTab).not.toHaveBeenCalled() + expect(closeWebRuntimeSessionTab).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/lib/browser-workspace-source-resolution.ts b/src/renderer/src/lib/browser-workspace-source-resolution.ts new file mode 100644 index 00000000000..9e10c6f7043 --- /dev/null +++ b/src/renderer/src/lib/browser-workspace-source-resolution.ts @@ -0,0 +1,27 @@ +import type { AppState } from '../store/types' + +export type BrowserWorkspaceOwner = { + worktreeId: string + workspaceId: string +} + +export function resolveBrowserWorkspaceOwner( + state: Pick, + sourceId: string, + requiredWorktreeId?: string +): BrowserWorkspaceOwner | null { + for (const [worktreeId, workspaces] of Object.entries(state.browserTabsByWorktree)) { + if (requiredWorktreeId && worktreeId !== requiredWorktreeId) { + continue + } + for (const workspace of workspaces) { + if ( + workspace.id === sourceId || + (state.browserPagesByWorkspace[workspace.id] ?? []).some((page) => page.id === sourceId) + ) { + return { worktreeId, workspaceId: workspace.id } + } + } + } + return null +} diff --git a/src/renderer/src/lib/floating-workspace-terminal-actions.ts b/src/renderer/src/lib/floating-workspace-terminal-actions.ts index 484143160d9..d8c81add055 100644 --- a/src/renderer/src/lib/floating-workspace-terminal-actions.ts +++ b/src/renderer/src/lib/floating-workspace-terminal-actions.ts @@ -9,6 +9,7 @@ import { type TypeCyclableTab } from '@/components/terminal/tab-type-cycle' import type { AppState } from '@/store/types' +import { resolveBrowserWorkspaceOwner } from './browser-workspace-source-resolution' import { TOGGLE_FLOATING_TERMINAL_EVENT } from './floating-terminal' import { focusTerminalTabSurface } from './focus-terminal-tab-surface' import { keybindingMatchesAction, type KeybindingOverrides } from '../../../shared/keybindings' @@ -143,17 +144,10 @@ export function resolveFloatingWorkspaceBrowserWorkspaceId( store: Pick, sourceId: string ): string | null { - const workspaces = store.browserTabsByWorktree[FLOATING_TERMINAL_WORKTREE_ID] ?? [] - const pagesByWorkspace = store.browserPagesByWorkspace ?? {} - for (const workspace of workspaces) { - if (workspace.id === sourceId) { - return workspace.id - } - if ((pagesByWorkspace[workspace.id] ?? []).some((page) => page.id === sourceId)) { - return workspace.id - } - } - return null + return ( + resolveBrowserWorkspaceOwner(store, sourceId, FLOATING_TERMINAL_WORKTREE_ID)?.workspaceId ?? + null + ) } function activateFloatingWorkspaceCyclableTab( diff --git a/tests/e2e/browser-split-find-shortcut.spec.ts b/tests/e2e/browser-split-shortcuts.spec.ts similarity index 83% rename from tests/e2e/browser-split-find-shortcut.spec.ts rename to tests/e2e/browser-split-shortcuts.spec.ts index a7563fb66f2..4d9c824bbac 100644 --- a/tests/e2e/browser-split-find-shortcut.spec.ts +++ b/tests/e2e/browser-split-shortcuts.spec.ts @@ -4,8 +4,9 @@ import { focusActiveTerminalInput } from './helpers/terminal' import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' const modifier = process.platform === 'darwin' ? 'Meta' : 'Control' +const guestModifier: 'meta' | 'control' = process.platform === 'darwin' ? 'meta' : 'control' -type SplitFindFixture = { +type TerminalBrowserSplitFixture = { browserGroupId: string browserTabId: string terminalGroupId: string @@ -17,7 +18,7 @@ type BrowserSplitFixture = { secondBrowserTabId: string } -async function createTerminalBrowserSplit(page: Page): Promise { +async function createTerminalBrowserSplit(page: Page): Promise { return page.evaluate(() => { const store = window.__store if (!store) { @@ -28,6 +29,13 @@ async function createTerminalBrowserSplit(page: Page): Promise if (!worktreeId) { throw new Error('Active worktree unavailable') } + const terminalTabId = state.activeTabIdByWorktree[worktreeId] ?? state.activeTabId ?? undefined + if ( + !terminalTabId || + !(state.tabsByWorktree[worktreeId] ?? []).some((tab) => tab.id === terminalTabId) + ) { + throw new Error('Active terminal tab unavailable') + } const terminalGroupId = state.ensureWorktreeRootGroup(worktreeId) const browserGroupId = state.createEmptySplitGroup(worktreeId, terminalGroupId, 'right') if (!browserGroupId) { @@ -38,7 +46,15 @@ async function createTerminalBrowserSplit(page: Page): Promise focusAddressBar: false, targetGroupId: browserGroupId }) - return { browserGroupId, browserTabId: browserTab.id, terminalGroupId } + const browserPageId = browserTab.activePageId + if (!browserPageId) { + throw new Error('Active browser page unavailable') + } + return { + browserGroupId, + browserTabId: browserTab.id, + terminalGroupId + } }) } @@ -76,8 +92,12 @@ async function createBrowserSplit(page: Page): Promise { focusAddressBar: false, targetGroupId: secondBrowserGroupId }) + const firstBrowserPageId = firstBrowserTab.activePageId + if (!firstBrowserPageId) { + throw new Error('First active browser page unavailable') + } return { - firstBrowserPageId: firstBrowserTab.activePageId, + firstBrowserPageId, firstBrowserTabId: firstBrowserTab.id, secondBrowserTabId: secondBrowserTab.id } @@ -97,7 +117,7 @@ async function focusBrowserAddressBar(page: Page, browserTabId: string): Promise 'form:has(> [data-orca-browser-address-bar="true"])' ) await expect(addressBarForm).toBeVisible() - await addressBarForm.click() + await addressBar.focus() await expect(addressBar).toBeFocused() } @@ -115,7 +135,7 @@ function browserSplitFindInput(page: Page, browserTabId: string) { .getByPlaceholder('Find in page...') } -async function pressFindInBrowserGuest( +async function waitForBrowserGuestRegistration( page: Page, browserTabId: string, browserPageId: string @@ -123,7 +143,7 @@ async function pressFindInBrowserGuest( await expect .poll(() => page.evaluate( - async ({ targetBrowserPageId, targetBrowserTabId, inputModifier }) => { + async ({ targetBrowserPageId, targetBrowserTabId }) => { const overlay = document.querySelector( `[data-browser-overlay-tab-id="${targetBrowserTabId}"]` ) @@ -140,17 +160,6 @@ async function pressFindInBrowserGuest( if (!registered) { return false } - webview.focus() - await webview.sendInputEvent({ - type: 'keyDown', - keyCode: 'F', - modifiers: [inputModifier] - }) - await webview.sendInputEvent({ - type: 'keyUp', - keyCode: 'F', - modifiers: [inputModifier] - }) return true } catch { return false @@ -158,14 +167,44 @@ async function pressFindInBrowserGuest( }, { targetBrowserPageId: browserPageId, - targetBrowserTabId: browserTabId, - inputModifier: modifier.toLowerCase() + targetBrowserTabId: browserTabId } ) ) .toBe(true) } +async function pressFindInBrowserGuest( + page: Page, + browserTabId: string, + browserPageId: string +): Promise { + await waitForBrowserGuestRegistration(page, browserTabId, browserPageId) + await page.evaluate( + async ({ targetBrowserTabId, inputModifier }) => { + const overlay = document.querySelector( + `[data-browser-overlay-tab-id="${targetBrowserTabId}"]` + ) + const webview = overlay?.querySelector('webview') as Electron.WebviewTag | null + if (!webview) { + throw new Error('Registered browser guest unavailable') + } + webview.focus() + await webview.sendInputEvent({ + type: 'keyDown', + keyCode: 'F', + modifiers: [inputModifier] + }) + await webview.sendInputEvent({ + type: 'keyUp', + keyCode: 'F', + modifiers: [inputModifier] + }) + }, + { targetBrowserTabId: browserTabId, inputModifier: guestModifier } + ) +} + function terminalFindInput(page: Page) { return page.locator('[data-terminal-search-root] input:visible') } @@ -199,7 +238,7 @@ async function focusBrowserGroup(page: Page, groupId: string): Promise { await waitForFocusedGroup(page, groupId) } -test.describe('browser split Find shortcut', () => { +test.describe('browser split shortcuts', () => { test.beforeEach(async ({ orcaPage }) => { await waitForSessionReady(orcaPage) await waitForActiveWorktree(orcaPage)