Repository navigation
Open terminal file links in editor - #3977
Closed
austinywang wants to merge 19 commits into
Closed
austinywang wants to merge 19 commits into
austinywang wants to merge 19 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes terminal file/path opening: shell-aware tokenization, preferred-editor open with fragment parsing, a route-kind router for command-clicks, terminal-local file resolution with cwd/fileExists injection, and an async open_url pipeline that defers CMUX opens with revalidation. ChangesFile path opening from terminal interactions
Sequence DiagramssequenceDiagram
participant User
participant Terminal
participant Resolver
participant Router
participant Workspace
User->>Terminal: Cmd+click file path
Terminal->>Resolver: resolveTerminalOpenURLTarget(raw, cwd, fileExists)
Resolver-->>Terminal: TerminalOpenURLTarget (local file)
Terminal->>Router: deferredOpenFileInCmux(workspace, panelId, path)
Router->>Router: Task.detached -> compute routeKind(path)
Router->>Workspace: openOrFocus*Split based on routeKind
Workspace-->>Router: success/failure
Router-->>Terminal: opened (or fallback invoked)
sequenceDiagram
participant Terminal
participant ActionHandler
participant Resolver
participant EditorSettings
Terminal->>ActionHandler: GHOSTTY_ACTION_OPEN_URL(urlString)
ActionHandler->>Resolver: resolve off-main (cwd, fileExists)
Resolver-->>ActionHandler: TerminalOpenURLTarget
ActionHandler->>EditorSettings: open(resolvedURL) or deferred CMUX open
EditorSettings->>EditorSettings: parse fragment and tokenize command
EditorSettings->>EditorSettings: build shell command and exec
EditorSettings-->>ActionHandler: process result (success/fallback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR routes terminal file references (path:line[:column], relative and absolute paths, extensionless files like
Confidence Score: 4/5Safe to merge; the async dispatch chain correctly moves file I/O off the main actor and all previously identified blocking-path concerns are resolved. The async three-phase design is sound and all previously flagged synchronous-main-thread issues are resolved. The only delta from clean is a concurrency-style choice (Task.detached wrapping a pure synchronous routeKind call) and a behavioral note about CmuxShellWords semantics vs the old splitShellWords. Neither affects runtime correctness for any realistic input. No files require special attention; the most active changes in GhosttyTerminalView.swift and cmuxApp.swift both read cleanly. Important Files Changed
Sequence DiagramsequenceDiagram
participant Ghostty as Ghostty Callback
participant MA as MainActor
participant DT as Detached Task
participant FS as FileManager
participant Editor as PreferredEditorSettings
participant CMux as deferredOpenFileInCmux
Ghostty->>MA: GHOSTTY_ACTION_OPEN_URL (urlString)
MA->>MA: capture TerminalOpenURLActionContext
MA-->>Ghostty: return true
MA->>DT: Task.detached resolveOpenURLActionTarget(context)
DT->>FS: fileExists(path)
FS-->>DT: Bool
DT->>DT: resolveTerminalOpenURLTarget
DT->>MA: MainActor.run handleResolvedOpenURLTarget
alt local file, no fragment
MA->>CMux: deferredOpenFileInCmux
CMux->>MA: openInCmux or PreferredEditorSettings.open
else local file with fragment
MA->>Editor: PreferredEditorSettings.open (line/col from fragment)
else embeddedBrowser
MA->>MA: openEmbeddedBrowserLink
else external
MA->>MA: NSWorkspace.shared.open
end
Reviews (12): Last reviewed commit: "fix: preserve line jump editor routing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…file-to-ide # Conflicts: # Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 5142-5146: The test currently mutates the process-global CWD using
FileManager.default.changeCurrentDirectoryPath (saving originalDirectory and
deferring restore), which risks cross-test flakiness; instead, stop changing the
global CWD and call resolveTerminalOpenURLTarget with an explicit cwd parameter
pointing to root.path (e.g. replace the changeCurrentDirectoryPath/defer block
and pass cwd: root.path into resolveTerminalOpenURLTarget when resolving
"Sources/App.swift:1"). Ensure any reference to originalDirectory and the
deferred restore are removed and the test still unwraps the returned target via
try XCTUnwrap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 025751f3-bcbf-403d-9f3f-6baf7564fbb1
📒 Files selected for processing (6)
Sources/CommandClickFileOpenRouter.swiftSources/GhosttyTerminalView.swiftSources/RestorableAgentSession.swiftSources/cmuxApp.swiftcmuxTests/PreferredEditorSettingsTests.swiftcmuxTests/TerminalAndGhosttyTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/GhosttyTerminalView.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CommandClickFileOpenRouter.swift`:
- Around line 100-107: The code currently falls back to the captured workspace
reference after an async hop (using
AppDelegate.shared?.workspaceContainingPanel(... )?.workspace ?? workspace)
which can be stale; change this to re-resolve the live Workspace instance by
ID/ownership (use AppDelegate.shared?.workspaceContainingPanel(panelId:
surfaceId, preferredWorkspaceId: preferredWorkspaceId)?.workspace and if that
lookup returns nil call the provided fallback closure instead of using the
captured workspace variable), ensuring you do not mutate or reuse the captured
workspace; reference the symbols workspaceContainingPanel, AppDelegate.shared,
preferredWorkspaceId, surfaceId and fallback when making the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9406002e-feda-4241-8c08-14cb62b54710
📒 Files selected for processing (3)
Sources/CommandClickFileOpenRouter.swiftSources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CommandClickFileOpenRouter.swift`:
- Around line 100-107: The code currently falls back to the captured workspace
reference after an async hop (using
AppDelegate.shared?.workspaceContainingPanel(... )?.workspace ?? workspace)
which can be stale; change this to re-resolve the live Workspace instance by
ID/ownership (use AppDelegate.shared?.workspaceContainingPanel(panelId:
surfaceId, preferredWorkspaceId: preferredWorkspaceId)?.workspace and if that
lookup returns nil call the provided fallback closure instead of using the
captured workspace variable), ensuring you do not mutate or reuse the captured
workspace; reference the symbols workspaceContainingPanel, AppDelegate.shared,
preferredWorkspaceId, surfaceId and fallback when making the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9406002e-feda-4241-8c08-14cb62b54710
📒 Files selected for processing (3)
Sources/CommandClickFileOpenRouter.swiftSources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/GhosttyTerminalView.swift
🛑 Comments failed to post (1)
Sources/CommandClickFileOpenRouter.swift (1)
100-107:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t fall back to the captured
workspaceafter the async hop.On Lines 104-107,
workspaceContainingPanel(... )?.workspace ?? workspacecan reopen on a staleWorkspaceinstance if the panel moved or the tab list was rebuilt before this task runs. That makes the deferred open target the wrong workspace or a detached object. Re-resolve the live workspace by ID/ownership here and callfallbackwhen that lookup fails instead of mutating the captured fallback object.Based on learnings, workspace mutations must re-resolve the live
Workspaceinstance becauserestoreSessionSnapshot()can replacetabswith fresh instances, making captured references stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/CommandClickFileOpenRouter.swift` around lines 100 - 107, The code currently falls back to the captured workspace reference after an async hop (using AppDelegate.shared?.workspaceContainingPanel(... )?.workspace ?? workspace) which can be stale; change this to re-resolve the live Workspace instance by ID/ownership (use AppDelegate.shared?.workspaceContainingPanel(panelId: surfaceId, preferredWorkspaceId: preferredWorkspaceId)?.workspace and if that lookup returns nil call the provided fallback closure instead of using the captured workspace variable), ensuring you do not mutate or reuse the captured workspace; reference the symbols workspaceContainingPanel, AppDelegate.shared, preferredWorkspaceId, surfaceId and fallback when making the fix.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 5200-5206: The test currently allows both .external and
.embeddedBrowser for the evaluated target, which masks regressions; change the
assertion to require the concrete .external case for the evaluated target
variable (use a guard/case let .external(url) else { XCTFail(...); return } or
an XCTAssertTrue case comparison), then assert url.isFileURL is false and
url.absoluteString == "config:8080" to ensure only the expected external route
is accepted.
- Around line 5183-5185: The test is asserting the full URL string which can
fail due to harmless normalization; instead in the .embeddedBrowser(url) branch
inspect URL components: assert url.scheme == "http", url.host == "localhost",
and assert url.port == 3000 (or if port is nil assert default 80 is not
expected), and optionally assert that url.path is "/" or startsWith the expected
path rather than using url.absoluteString; update the
XCTAssertEqual(url.absoluteString, ...) to these component checks in the
.embeddedBrowser(url) case and leave the .external(url) case unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40ddfc37-1fed-4082-ae72-aa4b322941f7
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
3 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 22509e3. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/cmuxApp.swift (2)
5254-5475: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract shell/editor routing logic out of
cmuxApp.swift.
CmuxShellWords+PreferredEditorSettingsare core, testable logic that do not depend on SwiftUI app lifecycle, but they’re being expanded in an already oversized root app file. Move them into a focused module/file (and package target if reused) to keep boundaries and ownership clear.As per coding guidelines: “Flag features implemented directly in the app target/module's root Sources path when core logic is independent of cmux app lifecycle...” and “Flag Swift production files that exceed 400 lines without a clear single responsibility...”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/cmuxApp.swift` around lines 5254 - 5475, Extract CmuxShellWords and PreferredEditorSettings (including their helper types and functions like OpenTarget, commandInvocationComponents, lineColumn, commandNeedsGotoFlag, normalizedCommandName, firstShellWord, shellQuote) into a new focused Swift source file and, if reused by other targets, into a new module/package target; update visibility (make types/functions internal/public as needed for tests), add necessary imports (Foundation, AppKit) at the top of the new file, remove the duplicated definitions from cmuxApp.swift, and update any unit tests or references to import the new module or file so existing callers (e.g., PreferredEditorSettings.open and PreferredEditorSettings.resolvedCommand) continue to work.
5367-5372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid blocking a global worker thread per editor launch.
process.waitUntilExit()insideDispatchQueue.global().asynccan strand a worker thread for the full editor lifetime (e.g.,vim), and repeated cmd-click opens can accumulate blocked threads. Preferprocess.terminationHandlerto observe exit without blocking a pooled thread.Suggested change
do { try process.run() - // Check exit status on a background thread; fall back on failure - // (e.g. command not found exits 127 but /bin/sh itself succeeds) - DispatchQueue.global(qos: .userInitiated).async { - process.waitUntilExit() - if process.terminationStatus != 0 { - DispatchQueue.main.async { NSWorkspace.shared.open(target.fallbackURL) } - } - } + process.terminationHandler = { proc in + guard proc.terminationStatus != 0 else { return } + DispatchQueue.main.async { + NSWorkspace.shared.open(target.fallbackURL) + } + } } catch { NSWorkspace.shared.open(target.fallbackURL) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/cmuxApp.swift` around lines 5367 - 5372, The current code spawns a pooled thread with DispatchQueue.global(qos: .userInitiated).async and calls process.waitUntilExit(), which can block a worker thread for the lifetime of the editor; replace this pattern by assigning process.terminationHandler to a closure that checks process.terminationStatus and, if non‑zero, calls DispatchQueue.main.async { NSWorkspace.shared.open(target.fallbackURL) } so no global worker thread is blocked. Locate the block using process.waitUntilExit(), DispatchQueue.global(...).async and NSWorkspace.shared.open(target.fallbackURL) and convert it to use process.terminationHandler instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 5254-5475: Extract CmuxShellWords and PreferredEditorSettings
(including their helper types and functions like OpenTarget,
commandInvocationComponents, lineColumn, commandNeedsGotoFlag,
normalizedCommandName, firstShellWord, shellQuote) into a new focused Swift
source file and, if reused by other targets, into a new module/package target;
update visibility (make types/functions internal/public as needed for tests),
add necessary imports (Foundation, AppKit) at the top of the new file, remove
the duplicated definitions from cmuxApp.swift, and update any unit tests or
references to import the new module or file so existing callers (e.g.,
PreferredEditorSettings.open and PreferredEditorSettings.resolvedCommand)
continue to work.
- Around line 5367-5372: The current code spawns a pooled thread with
DispatchQueue.global(qos: .userInitiated).async and calls
process.waitUntilExit(), which can block a worker thread for the lifetime of the
editor; replace this pattern by assigning process.terminationHandler to a
closure that checks process.terminationStatus and, if non‑zero, calls
DispatchQueue.main.async { NSWorkspace.shared.open(target.fallbackURL) } so no
global worker thread is blocked. Locate the block using process.waitUntilExit(),
DispatchQueue.global(...).async and NSWorkspace.shared.open(target.fallbackURL)
and convert it to use process.terminationHandler instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a765e50c-2b83-4cad-85ea-694142457e84
📒 Files selected for processing (6)
Sources/CommandClickFileOpenRouter.swiftSources/GhosttyTerminalView.swiftSources/cmuxApp.swiftcmuxTests/PreferredEditorSettingsTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/WindowAndDragTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/GhosttyTerminalView.swift
austinywang
dismissed
coderabbitai[bot]’s stale review
June 4, 2026 13:53
Stale CodeRabbit review on an older commit; both actionable threads were fixed in follow-up commits, all review threads are resolved, and CodeRabbit re-reviewed the latest head without requesting changes.
Eridanus117
added a commit
to Eridanus117/cmux
that referenced
this pull request
Jul 1, 2026
Resolve terminal file links of the form `path:line[:column]` — the convention Claude Code and Codex print for click-to-open — to the referenced file and open it at that line in the configured editor: - `String.splitTerminalPathLineSuffix()` peels a trailing `:line[:column]`. - `TerminalPathResolver.resolveOpenURLFileReference(_:cwd:)` resolves a token to an existing file plus optional line/column, probing the literal path first (so a file that really ends in a colon-number wins) and the line-stripped path second; web URLs are never treated as file paths. - `TerminalFileReference.fileURL` carries line/column across the package boundary as a `#L<line>[:<column>]` fragment. - `PreferredEditorService.open` decodes that fragment and hands `path:line:column` (plus `-g`) to VS Code-family editors, falling back to the fragment-free file URL for other editors and the system opener. - The terminal open-URL handler routes any line reference to the editor, which also fixes relative `dir/file.swift:line` links that previously fell through to the browser omnibar. Verified with package-level swift test on CmuxTerminalCore and CmuxWorkspaces (red without the fix, green with it). No new user-facing strings. Supersedes manaflow-ai#3977 by @austinywang, which implemented the same user-facing behavior before manaflow-ai#5894 extracted the resolver into CmuxTerminalCore / CmuxWorkspaces; the `:line` split and editor `-g` dispatch are adapted from that PR onto the new package seams. Co-authored-by: austinpower1258 <austinwang115@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1544
Testing
Note
Medium Risk
Touches terminal link classification, async open-url handling, and editor invocation paths where misclassification could open wrong targets or skip in-app routing; scope is large but covered by new tests.
Overview
Terminal file links (including
path:line[:column], relative paths, extensionless names likeMakefile, andfile:URLs with#L…fragments) are resolved against the terminal CWD and opened through the preferred editor, with line/column passed via URL fragments and editor-specific flags (-g/path:line:colfor VS Code–family commands). HTTP(S) and oddscheme:porttokens stay on the browser/external path; unverified absolute paths are not opened on remote terminals.In-app routing gains an explicit
CommandClickFileRouteKind, off-main route checks in deferred opens, and a markdown → file-preview fallback when a markdown split cannot be created. Ghosttyopen_urlhandling is refactored to resolve off the main thread and apply opens on the main actor, usingPreferredEditorSettings(and cmux splits only for fragment-free local files when routing applies).Shared
CmuxShellWordsreplaces ad-hoc shell splitting for agent resume and editor command parsing.Reviewed by Cursor Bugbot for commit a80502d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Open terminal file links in the preferred editor with correct line/column jumps, while keeping web URLs in the browser and routing Markdown/supported files in‑app. Resolution runs off‑main with a clear action context, preserves
file:fragments, and resolves relative paths against the terminal CWD (closes #1544).New Features
path:line[:col],file:) over browser URLs; resolve against terminal CWD; support extensionless files (e.g.,Makefile); emitfile://…#L….PreferredEditorSettings; parse#L…fragments; add--goto/-gforcode/cursor/windsurf; sharedCmuxShellWords; explicitCommandClickFileRouteKindwith markdown→preview fallback.Bug Fixes
http/httpsandscheme:porttokens (e.g.,config:8080) external; avoid host:port false positives.file:line fragments; don't routefile#L…into in‑app preview; resolve off‑main and defer UI on main; preserve source workspace/panel; skip absolute‑path fallbacks for remote terminals; fallback toNSWorkspacewhen editor routing fails.Written for commit a80502d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests