Repository navigation
Open cmd-clicked Markdown paths in Markdown viewer - #4864
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR centralizes terminal-open-URL resolution (trims input, rejects URL schemes), flips Cmd-click Markdown routing default to enabled, extends DEBUG UI test display configuration, and adds unit/UI tests plus localization and schema/docs updates validating trailing-dot trimming and viewer routing. ChangesTerminal-Open-URL Resolution and Default Markdown Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 3 inconclusive)
✅ Passed checks (13 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryCmd-clicking Markdown paths (relative or absolute, with or without trailing punctuation) now opens the cmux Markdown viewer by default, and the terminal link handler normalizes its fallback URL string to the clean resolved path so external openers receive a well-formed file URL instead of the raw token.
Confidence Score: 5/5Safe to merge; all nine locales are updated, the new path-resolution function is a clean extraction of existing logic, and the fallback normalizedOpenURLString hand-off to resolveTerminalOpenURLTarget is correct because that function already handles bare absolute paths via URL(fileURLWithPath:). The default-value flip and path-routing changes are the highest-impact parts of this PR. The default flip is intentional and well-documented. The cmuxResolveTerminalOpenURLFilePath extraction correctly removes the now-unnecessary absolute-path exclusion. The normalizedOpenURLString fallback feeds a clean absolute path into resolveTerminalOpenURLTarget, which already handles absolute paths as file:// URLs — no gap there. All nine locales are updated in Localizable.xcstrings, resolving the previous internationalization concern. Unit and UI tests cover the key new paths. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Ghostty cmd-click / terminal link\nurlString"] --> B["Trim whitespace\n→ trimmedUrlString"]
B --> C{"trimmedUrlString\nempty?"}
C -- yes --> Z["return false"]
C -- no --> D["performOnMain:\ncmuxResolveTerminalOpenURLFilePath\ntrimmedUrlString, cwd"]
D --> E{"URL has\nscheme?"}
E -- yes --> F["return nil → (false, nil)"]
E -- no --> G["cmuxResolveQuicklookPath\n(strips trailing punctuation,\nquotes, resolves relative/absolute)"]
G --> H{"file\nexists?"}
H -- no --> F
H -- yes --> I{"shouldRouteInCmux\n(Markdown viewer enabled\n& readable .md file)?"}
I -- yes --> J["Open in cmux\nMarkdown viewer\n→ (true, resolvedPath)"]
I -- no --> K["→ (false, resolvedPath)\nnormalizedOpenURLString = resolvedPath"]
J --> L["return true ✓"]
K --> M["resolveTerminalOpenURLTarget\nnormalizedOpenURLString"]
F --> N["resolveTerminalOpenURLTarget\noriginal urlString"]
M --> O{"absolute\npath?"}
N --> O
O -- yes --> P["URL(fileURLWithPath:)\n→ .external(fileURL)"]
O -- no --> Q["parse URL scheme\n→ browser / external"]
P --> R["NSWorkspace.open"]
Q --> R
Reviews (5): Last reviewed commit: "Align Markdown routing settings subtitle" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
4697-4733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the normalized file path for the fallback open path.
When
cmuxResolveTerminalOpenURLFilePath(...)succeeds butCommandClickFileOpenRouter.shouldRouteInCmux(...)is false, this dropsresolvedPathand later reopens the raw token. For the new trailing-punctuation cases, opting out of markdown-in-cmux or hitting an unsupported file type falls through tofoo.md.//tmp/foo.md.instead of the real file, so the click stops opening the file at all.💡 Proposed fix
- if !trimmedUrlString.isEmpty { - let filePathRouted: Bool = performOnMain { + var resolvedOpenURLFilePath: String? + if !trimmedUrlString.isEmpty { + let filePathRouted: Bool = performOnMain { guard let termSurface = surfaceView.terminalSurface, let workspace = termSurface.owningWorkspace(), !workspace.isRemoteTerminalSurface(termSurface.id) else { return false } let cwd = CommandClickFileOpenRouter.resolveWorkingDirectory( workspace: workspace, surfaceId: termSurface.id ) - guard let resolvedPath = cmuxResolveTerminalOpenURLFilePath(trimmedUrlString, cwd: cwd), - CommandClickFileOpenRouter.shouldRouteInCmux(path: resolvedPath) else { + guard let resolvedPath = cmuxResolveTerminalOpenURLFilePath(trimmedUrlString, cwd: cwd) else { return false } + resolvedOpenURLFilePath = resolvedPath + guard CommandClickFileOpenRouter.shouldRouteInCmux(path: resolvedPath) else { + return false + } `#if` DEBUG cmuxDebugLog("link.openURL resolvedAsFilePath=\(resolvedPath)") `#endif` let fileURL = URL(fileURLWithPath: resolvedPath) CommandClickFileOpenRouter.deferredOpenFileInCmux( @@ if filePathRouted { return true } } - guard let target = resolveTerminalOpenURLTarget(urlString) else { + let normalizedURLString = resolvedOpenURLFilePath ?? urlString + guard let target = resolveTerminalOpenURLTarget(normalizedURLString) else {🤖 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/GhosttyTerminalView.swift` around lines 4697 - 4733, The code currently discards the normalized resolvedPath when cmuxResolveTerminalOpenURLFilePath(...) returns a path but CommandClickFileOpenRouter.shouldRouteInCmux(...) is false, which later causes the fallback to reopen the raw token (e.g., "foo.md.") instead of the normalized file path; change the logic in the performOnMain block around cmuxResolveTerminalOpenURLFilePath and shouldRouteInCmux so that if cmuxResolveTerminalOpenURLFilePath returns a non-nil resolvedPath you preserve that value (e.g., assign to a new local fallbackResolvedPath or replace urlString) for use by the later fallback open logic, while still skipping CommandClickFileOpenRouter.deferredOpenFileInCmux when shouldRouteInCmux is false; reference cmuxResolveTerminalOpenURLFilePath, CommandClickFileOpenRouter.shouldRouteInCmux, resolvedPath, and the performOnMain closure to locate the code to update.
🤖 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/AppDelegate.swift`:
- Around line 1819-1823: Add a setEnabled(_:) helper to
CmdClickMarkdownRouteSettings (mirroring
CmdClickSupportedFileRouteSettings.setEnabled) that writes the boolean to
UserDefaults and posts the same change notification; then replace the direct
UserDefaults write in AppDelegate (the block checking
CMUX_UI_TEST_OPEN_MARKDOWN_IN_CMUX_VIEWER) to call
CmdClickMarkdownRouteSettings.setEnabled(rawOpenMarkdown == "1"). Ensure the
method name matches CmdClickMarkdownRouteSettings and that the notification
key/payload matches the pattern used by CmdClickSupportedFileRouteSettings.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4697-4733: The code currently discards the normalized resolvedPath
when cmuxResolveTerminalOpenURLFilePath(...) returns a path but
CommandClickFileOpenRouter.shouldRouteInCmux(...) is false, which later causes
the fallback to reopen the raw token (e.g., "foo.md.") instead of the normalized
file path; change the logic in the performOnMain block around
cmuxResolveTerminalOpenURLFilePath and shouldRouteInCmux so that if
cmuxResolveTerminalOpenURLFilePath returns a non-nil resolvedPath you preserve
that value (e.g., assign to a new local fallbackResolvedPath or replace
urlString) for use by the later fallback open logic, while still skipping
CommandClickFileOpenRouter.deferredOpenFileInCmux when shouldRouteInCmux is
false; reference cmuxResolveTerminalOpenURLFilePath,
CommandClickFileOpenRouter.shouldRouteInCmux, resolvedPath, and the
performOnMain closure to locate the code to update.
🪄 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: 7bb62230-b0b7-40e1-8c31-85684bd99924
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/cmuxApp.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/WindowAndDragTests.swiftcmuxUITests/TerminalCmdClickUITests.swiftskills/cmux-settings/references/all-keys.mdweb/data/cmux.schema.json
lawrencecchen
dismissed
coderabbitai[bot]’s stale review
May 28, 2026 05:08
Addressed in fa27877 by preserving the normalized fallback path and adding the settings helper.
This was referenced May 31, 2026
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
1 participant
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
skills/marketing/data/lawrencecchen-tweets.md.as the shape.Testing
jq empty Resources/Localizable.xcstrings web/data/cmux.schema.jsongit diff --check HEAD~2..HEADIssues
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes default app behavior (Markdown opens in cmux viewer unless disabled) and expands terminal link/file-path resolution for absolute paths and punctuation edge cases.
Overview
Cmd-click and terminal link handling now resolve Markdown paths more reliably—including absolute and quoted paths with trailing punctuation—and route readable
.mdfiles into the cmux Markdown viewer when routing applies, instead of only handling relative paths.app.openMarkdownInCmuxVieweris on by default and no longer described as requiring supported-file routing; settings useCmdClickMarkdownRouteSettings.setEnabled, a change notification, and updated schema/docs/localized subtitles. UI-test fixtures support nested paths, display suffixes, and the Markdown toggle env var.Reviewed by Cursor Bugbot for commit cd1f05d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Cmd-clicking Markdown files now opens the rendered cmux Markdown viewer by default. Cmd-click and terminal link handling resolve relative, absolute, and quoted Markdown paths, trim trailing punctuation, and normalize fallback paths; schema/docs updated and the settings subtitle aligned across locales.
Bug Fixes
Migration
app.openMarkdownInCmuxViewertofalsein Settings > App.Written for commit cd1f05d. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests