Repository navigation
Conversation
Cmd-click resolves a clicked token to a file by probing it for existence. Text like `src/state.js:87` (compiler output, grep -n, stack traces) never matches a real file, so the token does not resolve and the link is dead. Assert that `resolveQuicklookPath` treats a trailing `:line` / `:line:col` locator as pointing at the file itself. Red until the resolver learns to probe the locator-stripped spelling.
Cmd-click resolved a clicked token to a file by probing it for existence, so `src/state.js:87` and `src/state.js:87:12` (compiler output, grep -n, stack traces) never matched a file and the link was dead. Teach the path resolver to also probe the locator-stripped spelling, after every literal candidate so a real file whose name ends in a colon-number still wins. Parse the trailing `:line` / `:line:col` off the clicked token and forward it to the preferred editor as the `path:line[:col]` locator Zed, VS Code, and Sublime accept; the capture seam and the system-default fallback still receive the bare path, and the in-cmux preview route already opens the stripped path. No user-facing strings added, so no localization changes.
|
@liflovs is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughTrailing ChangesTerminal locator forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GhosttyNSView
participant TerminalFilePathLineLocator
participant PreferredEditorService
participant EditorCommand
GhosttyNSView->>TerminalFilePathLineLocator: derive line and column from clicked token
TerminalFilePathLineLocator-->>GhosttyNSView: return optional position
GhosttyNSView->>PreferredEditorService: open resolved path with position
PreferredEditorService->>EditorCommand: pass path:line[:column]
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 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 |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@liflovs I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 17 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryThis PR fixes cmd-click on
Confidence Score: 4/5The core path-resolution and editor-open changes are correct and well-tested; the two findings are confined to a new helper type's shape and an untested disambiguation edge case involving files literally named with colon-number suffixes. The candidate-stripping logic in pathResolutionCandidates() and the shell-argument construction in PreferredEditorService are sound. The main concerns are that TerminalFilePathLineLocator uses a caseless-enum namespace (against codebase conventions) and that position disambiguation produces a wrong line/column when a colon-named file like weird:87 is clicked as weird:87:12 — an edge case not covered by the test suite. Neither finding breaks the primary happy path or any existing behavior. TerminalFileLinePosition.swift — both the namespace shape of TerminalFilePathLineLocator and the position comparison logic that can misclassify column locators on colon-named files. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant TPR as TerminalPathResolver
participant TFLL as TerminalFilePathLineLocator
participant PES as PreferredEditorService
User->>GTV: cmd-click on "src/state.js:87:12"
GTV->>TPR: resolveQuicklookPath("src/state.js:87:12", cwd)
Note over TPR: pathResolutionCandidates() tries<br/>"src/state.js:87:12" (miss),<br/>then stripped "src/state.js" (hit)
TPR-->>GTV: "resolvedPath = "/repo/src/state.js""
GTV->>TFLL: position(rawToken:"src/state.js:87:12", resolvedPath:"/repo/src/state.js")
Note over TFLL: splittingTrailingLineColumn() →<br/>{line:87, column:12}
TFLL-->>GTV: TerminalFileLinePosition(line:87, column:12)
GTV->>PES: open(URL("/repo/src/state.js"), line:87, column:12)
Note over PES: argument = "/repo/src/state.js:87:12"<br/>shell: sh -c "editor '/repo/src/state.js:87:12'"
PES-->>User: Editor opens at line 87, col 12
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant TPR as TerminalPathResolver
participant TFLL as TerminalFilePathLineLocator
participant PES as PreferredEditorService
User->>GTV: cmd-click on "src/state.js:87:12"
GTV->>TPR: resolveQuicklookPath("src/state.js:87:12", cwd)
Note over TPR: pathResolutionCandidates() tries<br/>"src/state.js:87:12" (miss),<br/>then stripped "src/state.js" (hit)
TPR-->>GTV: "resolvedPath = "/repo/src/state.js""
GTV->>TFLL: position(rawToken:"src/state.js:87:12", resolvedPath:"/repo/src/state.js")
Note over TFLL: splittingTrailingLineColumn() →<br/>{line:87, column:12}
TFLL-->>GTV: TerminalFileLinePosition(line:87, column:12)
GTV->>PES: open(URL("/repo/src/state.js"), line:87, column:12)
Note over PES: argument = "/repo/src/state.js:87:12"<br/>shell: sh -c "editor '/repo/src/state.js:87:12'"
PES-->>User: Editor opens at line 87, col 12
Reviews (1): Last reviewed commit: "Open path:line[:col] cmd-click links at ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public enum TerminalFilePathLineLocator { | ||
| /// The position to open `resolvedPath` at, given the `rawToken` the user | ||
| /// clicked. | ||
| /// | ||
| /// Returns the token's trailing locator, unless the resolved file itself | ||
| /// carries the same numeric suffix (a real filename ending in `:line`), in | ||
| /// which case the digits are part of the name and no position is forwarded. | ||
| public static func position( | ||
| rawToken: String, | ||
| resolvedPath: String | ||
| ) -> TerminalFileLinePosition? { | ||
| guard let (_, position) = rawToken.splittingTrailingLineColumn() else { return nil } | ||
| if let (_, resolvedPosition) = resolvedPath.splittingTrailingLineColumn(), | ||
| resolvedPosition == position { | ||
| return nil | ||
| } | ||
| return position | ||
| } |
There was a problem hiding this comment.
TerminalFilePathLineLocator has no cases and a single static method, making it a pure namespace type. This pattern is explicitly prohibited by the cmux no-ambient-global-state rule, which flags caseless enums and empty structs used purely as static func/static let namespaces. The preferred shape is a static method on the type that owns the data — here TerminalFileLinePosition already exists and is the natural home for this factory logic (e.g., TerminalFileLinePosition.extracting(rawToken:resolvedPath:)).
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| public static func position( | ||
| rawToken: String, | ||
| resolvedPath: String | ||
| ) -> TerminalFileLinePosition? { | ||
| guard let (_, position) = rawToken.splittingTrailingLineColumn() else { return nil } | ||
| if let (_, resolvedPosition) = resolvedPath.splittingTrailingLineColumn(), | ||
| resolvedPosition == position { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When a file is literally named weird:87 and the user clicks weird:87:12, the resolver correctly returns weird:87 as resolvedPath. splittingTrailingLineColumn on the rawToken weird:87:12 yields position {line: 87, column: 12}, while the resolved path yields {line: 87, column: nil}. Because these two positions differ, the guard falls through and the editor receives {line: 87, column: 12} — but the correct locator for that resolved file is {line: 12, column: nil}. The current comparison strategy cannot distinguish "the column extends a locator that was already the filename" from "column is a new locator on top of the stripped path".
Greptile SummaryThis PR fixes cmd-click on terminal path tokens that carry a trailing line/column locator (e.g.
Confidence Score: 4/5The change is well-scoped to the pure path-resolution and editor-service layers, with solid unit coverage for the new parsing and disambiguation logic. The single structural concern is a new caseless enum used as a static-function namespace in production source. The disambiguation guard, candidate-stripping loop, and path:line[:col] argument construction are all correct. The only flag is TerminalFilePathLineLocator being a caseless enum whose sole member is a static function — the method's natural home is a static extension on TerminalFileLinePosition, and the caseless-enum shape is the style the repo explicitly discourages. Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/PathResolution/TerminalFileLinePosition.swift — the caseless enum namespace. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant Resolver as TerminalPathResolver
participant Locator as TerminalFilePathLineLocator
participant Editor as PreferredEditorService
User->>GTV: cmd-click "src/state.js:87:12"
GTV->>Resolver: resolveQuicklookPath("src/state.js:87:12", cwd:)
Note over Resolver: candidates: ["src/state.js:87:12", "src/state.js"] (stripped)
Resolver-->>GTV: "resolvedPath = "/repo/src/state.js""
GTV->>Locator: position(rawToken: "src/state.js:87:12", resolvedPath: "/repo/src/state.js")
Note over Locator: resolvedPath has no :N suffix → forward position
Locator-->>GTV: TerminalFileLinePosition(line: 87, column: 12)
GTV->>Editor: open(url, line: 87, column: 12)
Note over Editor: capture seam receives bare path
Editor->>Editor: "argument = "/repo/src/state.js:87:12""
Editor->>Editor: /bin/sh -c "code '/repo/src/state.js:87:12'"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant Resolver as TerminalPathResolver
participant Locator as TerminalFilePathLineLocator
participant Editor as PreferredEditorService
User->>GTV: cmd-click "src/state.js:87:12"
GTV->>Resolver: resolveQuicklookPath("src/state.js:87:12", cwd:)
Note over Resolver: candidates: ["src/state.js:87:12", "src/state.js"] (stripped)
Resolver-->>GTV: "resolvedPath = "/repo/src/state.js""
GTV->>Locator: position(rawToken: "src/state.js:87:12", resolvedPath: "/repo/src/state.js")
Note over Locator: resolvedPath has no :N suffix → forward position
Locator-->>GTV: TerminalFileLinePosition(line: 87, column: 12)
GTV->>Editor: open(url, line: 87, column: 12)
Note over Editor: capture seam receives bare path
Editor->>Editor: "argument = "/repo/src/state.js:87:12""
Editor->>Editor: /bin/sh -c "code '/repo/src/state.js:87:12'"
Reviews (2): Last reviewed commit: "Open path:line[:col] cmd-click links at ..." | Re-trigger Greptile |
| public enum TerminalFilePathLineLocator { | ||
| /// The position to open `resolvedPath` at, given the `rawToken` the user | ||
| /// clicked. | ||
| /// | ||
| /// Returns the token's trailing locator, unless the resolved file itself | ||
| /// carries the same numeric suffix (a real filename ending in `:line`), in | ||
| /// which case the digits are part of the name and no position is forwarded. | ||
| public static func position( | ||
| rawToken: String, | ||
| resolvedPath: String | ||
| ) -> TerminalFileLinePosition? { | ||
| guard let (_, position) = rawToken.splittingTrailingLineColumn() else { return nil } | ||
| if let (_, resolvedPosition) = resolvedPath.splittingTrailingLineColumn(), | ||
| resolvedPosition == position { | ||
| return nil | ||
| } | ||
| return position | ||
| } | ||
| } |
There was a problem hiding this comment.
TerminalFilePathLineLocator is a caseless public enum whose entire API is a single static func. This is exactly the ambient-global-state anti-pattern flagged in the repo's review rules: a caseless enum acting as a namespace for a static function. The method's natural home is a static func on TerminalFileLinePosition itself (the type it produces), or — because the function is only used in GhosttyTerminalView — a fileprivate free function or a private/internal static extension kept close to its call site. A caseless enum that is never constructed and holds no data carries no semantic value over a properly owned method.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR fixes cmd-click on compiler-style
Confidence Score: 4/5Safe to merge; the fix is well-contained in the pure path-resolution and editor-service layers, and all new logic is exercised by unit tests including a red/green regression pair. The implementation is correct and the test coverage is thorough. The one flagged issue is TerminalFilePathLineLocator being shaped as a caseless enum namespace rather than the idiomatic free-function or String-extension form the codebase's style rules prescribe — it is a style concern, not a correctness defect. TerminalFileLinePosition.swift — the TerminalFilePathLineLocator enum declaration should be reviewed against the no-ambient-global-state rule before merging. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant Resolver as TerminalPathResolver
participant Locator as TerminalFilePathLineLocator
participant Editor as PreferredEditorService
User->>GTV: cmd-click on "src/state.js:87:12"
GTV->>Resolver: resolveQuicklookPath("src/state.js:87:12", cwd)
Note over Resolver: candidates: ["src/state.js:87:12", "src/state.js"] (stripped added)
Resolver-->>GTV: WordPathResolution(path: "/repo/src/state.js", rawToken: "src/state.js:87:12")
GTV->>Locator: position(rawToken: "src/state.js:87:12", resolvedPath: "/repo/src/state.js")
Note over Locator: splittingTrailingLineColumn → {line:87, col:12}
Note over Locator: resolvedPath has no numeric suffix → forward position
Locator-->>GTV: TerminalFileLinePosition(line: 87, column: 12)
GTV->>Editor: open(URL("/repo/src/state.js"), line: 87, column: 12)
Note over Editor: argument = "/repo/src/state.js:87:12"
Editor->>Editor: /bin/sh -c "editor '/repo/src/state.js:87:12'"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User as User (cmd-click)
participant GTV as GhosttyNSView
participant Resolver as TerminalPathResolver
participant Locator as TerminalFilePathLineLocator
participant Editor as PreferredEditorService
User->>GTV: cmd-click on "src/state.js:87:12"
GTV->>Resolver: resolveQuicklookPath("src/state.js:87:12", cwd)
Note over Resolver: candidates: ["src/state.js:87:12", "src/state.js"] (stripped added)
Resolver-->>GTV: WordPathResolution(path: "/repo/src/state.js", rawToken: "src/state.js:87:12")
GTV->>Locator: position(rawToken: "src/state.js:87:12", resolvedPath: "/repo/src/state.js")
Note over Locator: splittingTrailingLineColumn → {line:87, col:12}
Note over Locator: resolvedPath has no numeric suffix → forward position
Locator-->>GTV: TerminalFileLinePosition(line: 87, column: 12)
GTV->>Editor: open(URL("/repo/src/state.js"), line: 87, column: 12)
Note over Editor: argument = "/repo/src/state.js:87:12"
Editor->>Editor: /bin/sh -c "editor '/repo/src/state.js:87:12'"
Reviews (3): Last reviewed commit: "Open path:line[:col] cmd-click links at ..." | Re-trigger Greptile |
| public enum TerminalFilePathLineLocator { | ||
| /// The position to open `resolvedPath` at, given the `rawToken` the user | ||
| /// clicked. | ||
| /// | ||
| /// Returns the token's trailing locator, unless the resolved file itself | ||
| /// carries the same numeric suffix (a real filename ending in `:line`), in | ||
| /// which case the digits are part of the name and no position is forwarded. | ||
| public static func position( | ||
| rawToken: String, | ||
| resolvedPath: String | ||
| ) -> TerminalFileLinePosition? { | ||
| guard let (_, position) = rawToken.splittingTrailingLineColumn() else { return nil } | ||
| if let (_, resolvedPosition) = resolvedPath.splittingTrailingLineColumn(), | ||
| resolvedPosition == position { | ||
| return nil | ||
| } | ||
| return position | ||
| } | ||
| } |
There was a problem hiding this comment.
TerminalFilePathLineLocator has no cases and exposes a single public static func. The cmux-no-ambient-global-state rule flags this pattern — a caseless enum or empty struct used purely as a static func/static let namespace should instead be a method on the type that owns the data or a nonisolated free function. Here the computation is a pure function of its two String inputs, so a free function (or an extension String helper alongside splittingTrailingLineColumn) would be the idiomatic fit without introducing a namespace type at all.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch has not been deployed
2 participants
Summary
src/state.js:87orsrc/state.js:87:12, the form printed by compilers,grep -n, and stack traces — now resolves to the file and opens the editor at that line/column. Previously such a token did not linkify at all (no pointing-hand cursor, dead click).src/state.js:87is never a real file, so resolution returnedniland the link silently no-op'd. The bare pathsrc/state.jsworked; the:87broke detection.The fix has two parts, both in the shared, well-tested path layer:
TerminalPathResolvernow probes the locator-stripped spelling as an additional candidate — appended after every literal candidate, so a real file whose name genuinely ends in:<number>still wins first. Locator segments must be positive integers and leave a non-empty token, and the existence probe is the real guard, so ordinary paths andpath:(no number) are untouched.:line[:col]is parsed off the clicked token and forwarded to the preferred editor as thepath:line[:col]argument that Zed, VS Code (code), and Sublime accept. The UI-test capture seam and the system-default fallback still receive the bare path (non-editor handlers can't take a position), and the in-cmux preview route already opens the stripped path.No user-facing strings were added, so no localization changes.
Testing
Behavior lives entirely in the pure
String/TerminalPathResolver/PreferredEditorServicelayer. Added a two-commit red/green regression (commit 1 = failing test, commit 2 = fix) plus unit coverage:TerminalPathResolverTests— locator resolution (:line,:line:col), literal-file-with-colon precedence,splittingTrailingLineColumnparsing (rejectspath:,path:noline,:0, empty token), and theTerminalFilePathLineLocatorrawToken-vs-resolved-path guard.PreferredEditorServiceTests— the configured editor command receivespath:87:12andpath:87, while capture/fallback keep the bare path.Verified by extracting the pure sources + their real tests into a standalone SwiftPM package and running
swift test(the full app build needs the GhosttyKit xcframework + submodules):Not verified locally: the full macOS app/Xcode build of
GhosttyTerminalView.swift(requires the GhosttyKit binary +bonsplitsubmodule not present in a shallow fork clone). The app-target edit is two call sites routed through one new private helper using the publicTerminalFilePathLineLocatorAPI.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes cmd-click on terminal paths with trailing line/column (e.g., src/state.js:87:12) so the file opens at the correct position in the preferred editor. Previously these tokens didn’t linkify; now they resolve and pass
path:line[:col]to editors like Zed,code, and Sublime.TerminalPathResolvernow also probes a locator-stripped candidate after literal candidates, preserving real filenames that end with:<number>.:line[:col]and a guard to avoid treating literal colon-number filenames as positions; position is forwarded only when appropriate.PreferredEditorService.openaccepts optional line/column and sendspath:line[:col]to the editor command; capture and system-open still receive the bare path.GhosttyTerminalViewnow forwards the parsed line/column when opening in the preferred editor.Written for commit 431a2de. Summary will update on new commits.
Summary by CodeRabbit
New Features
:lineor:line:columnlocators now opens the file at the specified position in the preferred editor.Bug Fixes