Skip to content

refactor(shell): the local-open guard takes an owner, not settings - #27144

Merged
OrcaWin merged 1 commit into
mainfrom
OrcaWin/d4-d4-s8a
Oct 10, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
OrcaWin/d4-d4-s8a

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​122 $\color{#cf222e}{\Huge{\mathbf{−}}}$​23 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​99
Prod 12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​113 $\color{#cf222e}{\Huge{\mathbf{−}}}$​59 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​54

ELI5

Orca refuses to open a remote file with this computer's own apps, because the path names a file on another machine. That check used to look at which paired server you last focused. Now it looks at the machine that actually owns the file.

What Changed

Before (as a user):

  • With a paired server focused, Shift/Cmd-clicking a file link in a local Markdown document was refused with "Opening remote paths in the local OS is not available."
  • In a Markdown document owned by a paired server whose tab carried no owner stamp, the same click with no server focused opened a same-named file on this computer instead.

After: the click opens the file when the document lives on this computer, and is refused when the document lives on an SSH host or a paired server, whatever server is focused. Reveal in Finder / File Explorer, "Open in" menus, the tab-bar absolute-path entry and the source-control ignore suggestion behave exactly as before.

Mechanism: isLocalPathOpenBlocked no longer takes settings. It takes the path's owning host (ExecutionHostId, an ExecutionHostOwnerMatch, or 'unresolved') and allows only local. Removing the parameter made the compiler list every caller, and each now names its owner:

  • Markdown preview, rich Markdown editor and the store link action use the new getLinkSourceLocalOpenOwner. A document that reports no SSH connection is checked against its workspace row in the catalog, because a paired-server workspace without a nested SSH target also reports no connection. Before, when the tab carried no owner, the focused server decided.
  • Callers whose context was already pinned to the owner (tab-bar entry, source-control status) convert it with getLocalPathOpenOwnerForRoute.
  • isRevealInFileManagerBlocked keeps the focused-server check explicitly, because the main process still refuses every reveal while a server is focused. The "Open in" menu now goes through it, so those menu items look the same as before. A follow-up PR moves that refusal in the main process to the file's owner and then removes the check.

Why

The focused server says nothing about where a file lives. Some callers had worked around that by rewriting settings.activeRuntimeEnvironmentId to the owner before calling the guard. When no owner was known, those callers fell back to whatever server was focused. Changing the signature removes that workaround, and the compiler finds every caller. Leaving settings as an optional extra would have kept the fallback alive. Keeping the reveal check where the main process enforces it means this PR does not enable menu items that would then fail on click.

Linked Issue

None. This is part of the work that routes actions by the host that owns them. The follow-up that moves the main-process refusal to the file's owner closes #16539.

Visual Proof

N/A. No UI changes. The only behaviour change is whether a modifier-click on a Markdown file link opens or shows the existing toast. Automated tests cover it.

Testing

  • pnpm tc, oxlint and pnpm run check:code-quality:changed pass.

  • Focused vitest run over src/renderer/src/{lib,components/editor,store/slices,components/right-sidebar,components/tab-bar,components/sidebar,components/terminal-pane}: 24107 passed.

  • New acceptance test rich-markdown-editor-click-routing.test.ts › "refuses a runtime-owned document even when the path exists locally" fails on origin/main (the file opens through the OS) and passes here. Its partner test: a local document with a server focused opens.

  • local-path-open-guard.test.ts and link-source-local-open-owner.test.ts cover local, SSH, paired-server, missing/ambiguous owners and the unresolved sentinel.

  • SSH: SSH-owned documents stay refused (owner ssh:<target>). Folder workspaces resolve through the same catalog lookup. If the catalog cannot place a document, the click is refused rather than treated as local.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Renderer only. No IPC or wire change, and nothing persisted. Paths are not parsed here, so Windows and WSL are unaffected.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

isLocalPathOpenBlocked now takes the path's owning host (ExecutionHostId,
an ExecutionHostOwnerMatch, or 'unresolved') and allows only 'local'. The
settings parameter is gone, so every caller names its owner. Markdown link
callers resolve the document's host from the catalog instead of falling back
to the focused server; reveal keeps the focus check explicitly because the
main process still refuses reveals while a server is focused.
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d63cea84-e64f-466a-8b65-38ecc563e1ad

📥 Commits

Reviewing files that changed from the base of the PR and between 8452fc3 and f241ddc.


📒 Files selected for processing (16)
  • src/renderer/src/components/editor/markdown-preview-link-actions.ts
  • src/renderer/src/components/editor/markdown-preview-system-link-action.ts
  • src/renderer/src/components/editor/rich-markdown-editor-click-routing.test.ts
  • src/renderer/src/components/editor/rich-markdown-editor-click-routing.ts
  • src/renderer/src/components/editor/rich-markdown-editor-config.ts
  • src/renderer/src/components/editor/use-markdown-preview-components.tsx
  • src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.ts
  • src/renderer/src/components/sidebar/WorktreeOpenInMenu.tsx
  • src/renderer/src/components/tab-bar/tab-create-entry-local-path.ts
  • src/renderer/src/lib/link-source-local-open-owner.test.ts
  • src/renderer/src/lib/link-source-local-open-owner.ts
  • src/renderer/src/lib/local-path-open-guard.test.ts
  • src/renderer/src/lib/local-path-open-guard.ts
  • src/renderer/src/lib/reveal-in-file-manager.ts
  • src/renderer/src/lib/worktree-row-open-in-owner.test.ts
  • src/renderer/src/store/slices/editor/actions/markdown-link-action.ts

💤 Files with no reviewable changes (2)
  • src/renderer/src/components/editor/rich-markdown-editor-config.ts
  • src/renderer/src/components/editor/use-markdown-preview-components.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.



📝 Walkthrough

Walkthrough

Local-path blocking now uses resolved execution-host ownership rather than settings and connection inputs. Markdown link handling and route-based actions resolve ownership before checking access. File-manager actions use a reveal-specific guard. Tests cover local, remote, and unresolved ownership cases, including local worktree links while a runtime is active.


Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to f241d

The ownership change has no established merge-blocking issue. Normal validation can proceed.

Pre-merge checks | Passed 3 | Failed 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check Warning Issue #16539 requires the Reveal in Finder action to use the worktree's local hostId and allow an existing local path. The current isRevealInFileManagerBlocked still blocks whenever `activeRuntime… Update the reveal guard to use the selected worktree's owner. Allow a local owner to reach openInFileManager when the local path is valid, regardless of the focused server. Keep remote and unresolved owners blocked. Add a regression test …
Docstring Coverage Warning Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: the local-open guard now uses an owner instead of settings.
Description check Passed The description is detailed and covers the user impact, mechanism, rationale, testing, scope, compatibility considerations, and checklist. It does not provide a standard issue link in the Fixes # fi…
Out of Scope Changes check Passed The changed owner resolver, shared local-path guard, caller updates, and tests support the stated host-ownership routing refactor. The Markdown, tab-bar, source-control, and menu updates use the same …

Full details: Linked Issues check

Explanation

Issue #16539 requires the Reveal in Finder action to use the worktree's local hostId and allow an existing local path. The current isRevealInFileManagerBlocked still blocks whenever activeRuntimeEnvironmentId is non-empty, even when the route owner is local. The PR description also states that this reveal behavior remains for a follow-up.

Resolution

Update the reveal guard to use the selected worktree's owner. Allow a local owner to reach openInFileManager when the local path is valid, regardless of the focused server. Keep remote and unresolved owners blocked. Add a regression test for a local worktree while a remote server is focused.


  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@OrcaWin
OrcaWin merged commit c0f18c2 into main Oct 10, 2026
69 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: 'Reveal in Finder' misidentifies local worktree as remote project after switching worktrees

1 participant