Repository navigation
Add configurable link-click modifiers (#797) - #4022
austinywang wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ 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 |
Greptile SummaryThis PR adds configurable modifier-key routing for terminal link clicks, letting users choose separate modifiers for opening links in the system default browser vs. the cmux browser pane. The feature is wired end-to-end: new
Confidence Score: 3/5Safe to merge after fixing the parseBrowserSection early-return bug; the side-channel timing concern is low risk given current Ghostty behavior but worth a follow-up. The guard/return defect in parseBrowserSection is a real config-parsing regression: any user who writes an invalid modifier string loses their host whitelist, intercept, and external-patterns settings silently for that config file load. The modifier logic, parser, UI, and tests are otherwise correct and well-covered. Sources/KeyboardShortcutSettingsFileStore.swift (guard/return in parseBrowserSection) and Sources/GhosttyTerminalView.swift (currentTerminalOpenURLModifierFlags side-channel timing contract). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[mouseDown / mouseUp\nGhosttyNSView] -->|set currentTerminalOpenURLModifierFlags| B[ghostty_surface_mouse_button\nC call]
B -->|synchronous link callback| C[GhosttyApp link handler]
C --> D[resolveTerminalOpenURLRoute\ntarget + modifierFlags]
D --> E{modifier\nmatches?}
E -->|defaultBrowser modifier| F[.external - NSWorkspace.open]
E -->|cmuxBrowser modifier\nbypasses host whitelist| G[.embeddedBrowser - open in pane]
E -->|no match - automatic| H{openTerminalLinksInCmuxBrowser\n+ host whitelist check}
H -->|pass| G
H -->|fail| F
B -->|defer clears flag| I[currentTerminalOpenURLModifierFlags = nil]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let raw = jsonString(section["terminalLinkDefaultBrowserModifier"]) { | ||
| guard let binding = TerminalLinkClickModifierBinding.parse(raw) else { | ||
| logInvalid("browser.terminalLinkDefaultBrowserModifier", sourcePath: sourcePath) | ||
| return | ||
| } | ||
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkDefaultBrowserModifierKey] = .string( | ||
| binding.configString | ||
| ) | ||
| } | ||
| if let raw = jsonString(section["terminalLinkCmuxBrowserModifier"]) { | ||
| guard let binding = TerminalLinkClickModifierBinding.parse(raw) else { | ||
| logInvalid("browser.terminalLinkCmuxBrowserModifier", sourcePath: sourcePath) | ||
| return | ||
| } | ||
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkCmuxBrowserModifierKey] = .string( | ||
| binding.configString | ||
| ) | ||
| } |
There was a problem hiding this comment.
return in guard exits parseBrowserSection entirely on bad modifier values
Both new guard blocks call return on parse failure, which exits parseBrowserSection altogether — not just the current if block. If a user writes "terminalLinkDefaultBrowserModifier": "typo" in their cmux.json, every browser setting that appears after this block (interceptTerminalOpenCommandInCmuxBrowser, hostsToOpenInEmbeddedBrowser, urlsToAlwaysOpenExternally, etc.) is silently dropped for that file load. The same PR's top-level link.click.* parsing correctly uses if/else and avoids early exit — apply the same pattern here.
| if let raw = jsonString(section["terminalLinkDefaultBrowserModifier"]) { | |
| guard let binding = TerminalLinkClickModifierBinding.parse(raw) else { | |
| logInvalid("browser.terminalLinkDefaultBrowserModifier", sourcePath: sourcePath) | |
| return | |
| } | |
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkDefaultBrowserModifierKey] = .string( | |
| binding.configString | |
| ) | |
| } | |
| if let raw = jsonString(section["terminalLinkCmuxBrowserModifier"]) { | |
| guard let binding = TerminalLinkClickModifierBinding.parse(raw) else { | |
| logInvalid("browser.terminalLinkCmuxBrowserModifier", sourcePath: sourcePath) | |
| return | |
| } | |
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkCmuxBrowserModifierKey] = .string( | |
| binding.configString | |
| ) | |
| } | |
| if let raw = jsonString(section["terminalLinkDefaultBrowserModifier"]) { | |
| if let binding = TerminalLinkClickModifierBinding.parse(raw) { | |
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkDefaultBrowserModifierKey] = .string( | |
| binding.configString | |
| ) | |
| } else { | |
| logInvalid("browser.terminalLinkDefaultBrowserModifier", sourcePath: sourcePath) | |
| } | |
| } | |
| if let raw = jsonString(section["terminalLinkCmuxBrowserModifier"]) { | |
| if let binding = TerminalLinkClickModifierBinding.parse(raw) { | |
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkCmuxBrowserModifierKey] = .string( | |
| binding.configString | |
| ) | |
| } else { | |
| logInvalid("browser.terminalLinkCmuxBrowserModifier", sourcePath: sourcePath) | |
| } | |
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 52bb500. Configure here.
| snapshot.managedUserDefaults[BrowserLinkOpenSettings.terminalLinkCmuxBrowserModifierKey] = .string( | ||
| binding.configString | ||
| ) | ||
| } |
There was a problem hiding this comment.
Invalid modifier string aborts entire browser section parsing
Medium Severity
The guard ... else { return } blocks for terminalLinkDefaultBrowserModifier and terminalLinkCmuxBrowserModifier in parseBrowserSection exit the entire function when the freeform string fails to parse. This silently drops all subsequent browser settings (interceptTerminalOpenCommandInCmuxBrowser, hostsToOpenInEmbeddedBrowser, urlsToAlwaysOpenExternally, insecureHttpHostsAllowedInEmbeddedBrowser, etc.). The top-level link.click.* aliases for the same settings correctly use if-else to log and continue parsing. The browser-section path is inconsistent with that approach.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 52bb500. Configure here.
|
cmux-reconcile: useful Usefulness verdict: Use as the configurable-link candidate, adapted to the current shared link coordinator. The inspected diff adds modifier parsing/settings, per-destination preferences and tests. The current current terminal-link coordinator is now a dedicated shared routing path, whereas the patch introduces routing helpers around older GhosttyTerminalView/BrowserPanel code. Preserve the capability, rebase it onto that current path, and reconcile #1470’s link hit-testing rather than merging two overlapping policies. Reviewed patch head: Older issue/PR tracking index — remaining scope and competing implementations are recorded there. |
|
Mac fleet instructions for head JOB_JSON=$(~/.local/bin/cmux-ci submit --kind cmux --command 'CMUX_FLEET_BUILD_TAG=pr-4022-52bb5007 /Users/Shared/cmux-build-fleet/recipes/cmux.sh https://github.com/manaflow-ai/cmux.git 52bb5007f00e267c8997a8dd5c096704549c2078' --artifact artifacts/cmux.app.zip --workspace https://github.com/manaflow-ai/cmux/pull/4022 --source-digest 52bb5007f00e267c8997a8dd5c096704549c2078 --cache-key cmux:pr-4022 --min-free-bytes 268435456000 --label cmux --label ram48)
JOB_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["id"])' <<<"$JOB_JSON")
~/.local/bin/cmux-ci wait "$JOB_ID" --receipt artifacts/fleet/$JOB_ID.json
~/.local/bin/cmux-ci publish-hq "$JOB_ID"Use an existing campaign job ID if one is already posted; do not submit a duplicate. A wait timeout leaves the remote job running. Published results will include an exact-head artifact link and timing/disk receipt. This recipe validates the macOS app only, not iOS or tests. Never use maclease or put credentials in a PR comment. |
|
Reviewed. This has gone stale against main: terminal link routing moved into There's also an unaddressed bug that Greptile and Bugbot both flagged: And a product question: an ⌥-click default collides with |
teamleaderleo
added
area: browser
This branch was successfully deployed
3 participants
Closes #797
Summary
Testing
Note
Medium Risk
Changes how terminal URL clicks are routed (embedded vs external) and how modifier keys are interpreted, which can affect core navigation behavior and input handling. Scope is contained to browser/link settings with added tests and schema/docs updates.
Overview
Adds configurable terminal link-click modifiers so specific modifier+click chords can force opening URLs in either the system default browser or a cmux browser pane, with defaults of
cmd+click(external) andopt+click(cmux).Refactors terminal URL opening to resolve a new
TerminalOpenURLRoutethat applies external URL patterns, per-modifier overrides, the existing terminal-link toggle, and host-whitelist logic (explicit cmux-modifier clicks bypass the host whitelist but not external patterns). Modifier handling is updated so Ghostty receives the correct link trigger while cmux captures the actual modifier flags for routing.Wires the new settings through Settings UI,
UserDefaults,cmux.jsonparsing (includinglink.click.*aliases), settings search index/aliases, localization strings, and updates the web schema/docs; adds unit tests covering parsing, routing, search anchors, and settings-file ingestion.Reviewed by Cursor Bugbot for commit 52bb500. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds configurable link-click modifiers for terminal URLs so users can choose which modifier opens links in the system browser vs the built‑in cmux browser. Updates settings, schema, routing, and tests to support explicit routing and clearer defaults. Closes #797.
New Features
browser.terminalLinkDefaultBrowserModifierandbrowser.terminalLinkCmuxBrowserModifier(defaults:cmd+click→ system browser,opt+click→ cmux pane).link.click.defaultBrowserandlink.click.builtInBrowser.openTerminalLinksInCmuxBrowsernow applies to unmatched clicks; copy updated in UI/docs.Migration
cmd+clickopens in the system browser;opt+clickopens in the cmux browser pane.Written for commit 52bb500. Summary will update on new commits.