feat: Shift+click to open links in system browser#2909
Conversation
Cmd+Shift+click on terminal links and browser panel links opens them in the system default browser instead of the embedded browser. Terminal: strips Shift from mods sent to Ghostty so it still treats Cmd+Shift as a link click, then checks NSEvent.modifierFlags for Shift in the OPEN_URL handler to force external open. This is a workaround until Ghostty exposes a link-query API (see PR description). Browser panel + popup: checks navigationAction.modifierFlags for Shift on link-activated navigations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@ClementRouquier is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements Shift+click behavior to open links in the system default browser across terminal and browser panel components, replacing embedded-browser selection logic with direct Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Shift+click support to open links in the system browser: terminal links via Cmd+Shift+click (by stripping Shift from mods sent to Ghostty and checking Confidence Score: 4/5Safe to merge after addressing the One P1 finding: Sources/GhosttyTerminalView.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant macOS
participant GhosttyTerminalView
participant Ghostty
participant BrowserPanel
participant SystemBrowser
Note over User,SystemBrowser: Cmd+Shift+click on terminal link
User->>macOS: Press Cmd+Shift, click link
macOS->>GhosttyTerminalView: mouseDown(event)
GhosttyTerminalView->>GhosttyTerminalView: linkModsFromEvent() strips Shift
GhosttyTerminalView->>Ghostty: mouse_pos(mods=Cmd only)
GhosttyTerminalView->>Ghostty: mouse_button(PRESS, mods=Cmd only)
macOS->>GhosttyTerminalView: mouseUp(event)
GhosttyTerminalView->>Ghostty: mouse_button(RELEASE, mods=Cmd only)
Ghostty->>GhosttyTerminalView: OPEN_URL callback
GhosttyTerminalView->>macOS: NSEvent.modifierFlags.contains(.shift)?
macOS-->>GhosttyTerminalView: true
GhosttyTerminalView->>SystemBrowser: NSWorkspace.open(url)
Note over User,SystemBrowser: Shift+click on browser panel link
User->>macOS: Press Shift, click link
macOS->>BrowserPanel: decidePolicyFor(navigationAction)
BrowserPanel->>BrowserPanel: modifierFlags.contains(.shift) and linkActivated?
BrowserPanel-->>BrowserPanel: true
BrowserPanel->>SystemBrowser: NSWorkspace.open(url)
BrowserPanel->>BrowserPanel: decisionHandler(.cancel)
Reviews (1): Last reviewed commit: "feat: Shift+click to open links in syste..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| keyEvent.action = action | ||
| keyEvent.keycode = UInt32(event.keyCode) | ||
| keyEvent.mods = modsFromEvent(event) | ||
| keyEvent.mods = linkModsFromEvent(event) |
There was a problem hiding this comment.
linkModsFromEvent misapplied to keyboard modifier-tracking events
flagsChanged sends Ghostty a modifier-key event to keep its internal modifier state in sync. When Shift is pressed while Cmd is held, linkModsFromEvent strips Shift from the mods field, so Ghostty receives a PRESS event for the Shift keycode but with a modifier state that doesn't include Shift — a self-contradictory event. Ghostty may use this field when generating modifier-based escape sequences for subsequent keyboard input, risking wrong output.
The stripping is only necessary for the mouse paths (ghostty_surface_mouse_pos / ghostty_surface_mouse_button) so Ghostty treats Cmd+Shift as a plain Cmd link click. For this keyboard modifier-tracking event, modsFromEvent(event) (the prior behavior) is correct. Consider reverting just this line to modsFromEvent(event).
| ghostty_surface_mouse_pos(surface, eventPoint.x, bounds.height - eventPoint.y, linkModsFromEvent(event)) | ||
| } | ||
| _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, modsFromEvent(event)) | ||
| _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, linkModsFromEvent(event)) |
There was a problem hiding this comment.
linkModsFromEvent called twice in mouseDown
linkModsFromEvent(event) is a pure function but is called twice, once for mouse_pos and once for mouse_button. Caching the result in a local variable avoids the redundant call.
| ghostty_surface_mouse_pos(surface, eventPoint.x, bounds.height - eventPoint.y, linkModsFromEvent(event)) | |
| } | |
| _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, modsFromEvent(event)) | |
| _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, linkModsFromEvent(event)) | |
| let mouseMods = linkModsFromEvent(event) | |
| if event.clickCount == 1 { | |
| ghostty_surface_mouse_pos(surface, eventPoint.x, bounds.height - eventPoint.y, mouseMods) | |
| } | |
| _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, mouseMods) |
| // Shift+click: bypass embedded browser and open in system default browser. | ||
| if navigationAction.modifierFlags.contains(.shift), | ||
| navigationAction.navigationType == .linkActivated { | ||
| #if DEBUG | ||
| dlog("popup.nav.shiftClickExternal url=\(url.absoluteString)") | ||
| #endif | ||
| NSWorkspace.shared.open(url) | ||
| decisionHandler(.cancel) | ||
| return | ||
| } |
There was a problem hiding this comment.
The Shift+click check here fires before the insecure HTTP guard (line 583), so Shift+clicking an http:// link in a popup opens it directly in the system browser without the security alert. The main BrowserPanel places the insecure HTTP check before the Shift+click check, so those two paths are inconsistent. Since the user's intent with Shift+click is explicitly to delegate to the system browser (which has its own HTTP warnings), this is arguably harmless — but worth aligning for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3260-3267: The callback is reading NSEvent.modifierFlags (global
state) instead of the latched Shift from the original click; modify the input
path so the Shift bit is captured at mouseDown/mouseUp and carried to the
Ghostty callback (or stored on GhosttyTerminalView) and then read in the
link-opening code instead of NSEvent.modifierFlags. Concretely: add a boolean
(e.g., shiftLatched) to the data passed through ghostty_action_s or as a
temporary property on GhosttyTerminalView set during mouseDown/mouseUp, set it
from event.modifierFlags.contains(.shift) at the original event, propagate it
into handleAction, and replace uses of NSEvent.modifierFlags.contains(.shift)
with that latched boolean (update any relevant functions that construct or
consume the ghostty_action_s).
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6174-6184: The shift-click external-open path currently
unconditionally calls NSWorkspace.shared.open(url) then
decisionHandler(.cancel); update the logic in the BrowserPanel navigation
handler so you capture the Bool result from NSWorkspace.shared.open(url) and
only call decisionHandler(.cancel) when it returns true, otherwise call
decisionHandler(.allow) (and optionally log the failure via dlog in DEBUG).
Locate the shift-click branch handling navigationAction, the
NSWorkspace.shared.open call and the decisionHandler invocation to implement
this conditional fallback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 577a33cb-df40-44b3-a632-8934d51c864d
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swift
| // Shift+click: bypass embedded browser, open in system default browser. | ||
| if NSEvent.modifierFlags.contains(.shift) { | ||
| #if DEBUG | ||
| dlog("link.openURL shift=true, forcing system browser url=\(target.url)") | ||
| #endif | ||
| return performOnMain { | ||
| NSWorkspace.shared.open(target.url) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In AppKit, does NSEvent.modifierFlags return the current global modifier state, or the modifier flags from the event that triggered a later callback?
💡 Result:
In AppKit, NSEvent.modifierFlags (the instance property on an NSEvent object) returns the modifier flags from the specific event that triggered the callback, not the current global modifier state.
Citations:
- 1: https://developer.apple.com/documentation/appkit/nsevent
- 2: https://stackoverflow.com/questions/6084266/check-modifierflags-of-nsevent-if-a-certain-modifier-was-pressed-but-no-other
- 3: https://stackoverflow.com/questions/3660432/nsevent-modifier-flags-not-updating-until-mouse-moves
- 4: https://home.gnustep.org/resources/OpenStepSpec/ApplicationKit/Classes/NSEvent.html
- 5: https://stackoverflow.com/questions/9602595/nsevent-global-event-monitoring-of-modifier-flags-returns-mixed-results
🏁 Script executed:
cd Sources && sed -n '3240,3280p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 2054
🏁 Script executed:
cd Sources && sed -n '3200,3280p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 4608
🏁 Script executed:
cd Sources && sed -n '3050,3150p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 5577
🏁 Script executed:
cd Sources && sed -n '2900,2950p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 2400
🏁 Script executed:
cd Sources && grep -n "func handleAction" GhosttyTerminalView.swift | head -5Repository: manaflow-ai/cmux
Length of output: 156
🏁 Script executed:
cd Sources && sed -n '2857,2900p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 2723
🏁 Script executed:
cd Sources && rg -n "GHOSTTY_ACTION_OPEN_URL" --type swift | head -20Repository: manaflow-ai/cmux
Length of output: 128
🏁 Script executed:
cd Sources && rg -n "handleAction\(" GhosttyTerminalView.swift | head -10Repository: manaflow-ai/cmux
Length of output: 243
🏁 Script executed:
cd Sources && sed -n '1660,1680p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1256
🏁 Script executed:
cd Sources && sed -n '7400,7430p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1915
🏁 Script executed:
cd Sources && sed -n '7500,7600p' GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 4667
🏁 Script executed:
cd Sources && rg "NSEvent\.modifierFlags" GhosttyTerminalView.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 330
🏁 Script executed:
cd Sources && rg "ghostty_action_s" GhosttyTerminalView.swift | head -20Repository: manaflow-ai/cmux
Length of output: 482
🏁 Script executed:
cd Sources && rg "class var modifierFlags\|static var modifierFlags" --type swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cd Sources && python3 << 'EOF'
# Check if NSEvent.modifierFlags is being called as class property (which returns current state)
# or instance property (which returns event's flags)
import subprocess
result = subprocess.run(['rg', 'NSEvent\\.modifierFlags', '-A', '5', '-B', '5'],
capture_output=True, text=True)
print("Search results for NSEvent.modifierFlags:")
print(result.stdout)
EOFRepository: manaflow-ai/cmux
Length of output: 102
🏁 Script executed:
cd Sources && rg "ghostty_action_s\|ghostty_action_open_url" --type swift -B 2 -A 10 | head -50Repository: manaflow-ai/cmux
Length of output: 42
Latch the click's Shift bit instead of reading global modifier state.
Line 3262 uses NSEvent.modifierFlags (the class property), which returns the current global modifier state, not the modifiers from the original click event. The handleAction callback receives only a ghostty_action_s struct—the original NSEvent is unavailable. If Shift is released between mouseUp and this callback, the same Cmd+Shift+click can fall back to the embedded browser instead of opening externally.
Capture the Shift state from the event during mouseDown/mouseUp and propagate it through the Ghostty callback chain (or store it temporarily) so this check reads the latched value instead of global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 3260 - 3267, The callback is
reading NSEvent.modifierFlags (global state) instead of the latched Shift from
the original click; modify the input path so the Shift bit is captured at
mouseDown/mouseUp and carried to the Ghostty callback (or stored on
GhosttyTerminalView) and then read in the link-opening code instead of
NSEvent.modifierFlags. Concretely: add a boolean (e.g., shiftLatched) to the
data passed through ghostty_action_s or as a temporary property on
GhosttyTerminalView set during mouseDown/mouseUp, set it from
event.modifierFlags.contains(.shift) at the original event, propagate it into
handleAction, and replace uses of NSEvent.modifierFlags.contains(.shift) with
that latched boolean (update any relevant functions that construct or consume
the ghostty_action_s).
| // Shift+click: bypass embedded browser and open in system default browser. | ||
| if navigationAction.modifierFlags.contains(.shift), | ||
| navigationAction.navigationType == .linkActivated, | ||
| let url = navigationAction.request.url { | ||
| #if DEBUG | ||
| dlog("browser.nav.decidePolicy.action kind=shiftClickExternal url=\(url.absoluteString)") | ||
| #endif | ||
| NSWorkspace.shared.open(url) | ||
| decisionHandler(.cancel) | ||
| return | ||
| } |
There was a problem hiding this comment.
Handle failed external-open gracefully before canceling navigation.
If NSWorkspace.shared.open(url) fails, this path still cancels the navigation, so the click does nothing. Please only cancel on successful handoff (or fall back to .allow).
💡 Suggested fix
if navigationAction.modifierFlags.contains(.shift),
navigationAction.navigationType == .linkActivated,
let url = navigationAction.request.url {
`#if` DEBUG
dlog("browser.nav.decidePolicy.action kind=shiftClickExternal url=\(url.absoluteString)")
`#endif`
- NSWorkspace.shared.open(url)
- decisionHandler(.cancel)
- return
+ let opened = NSWorkspace.shared.open(url)
+ if opened {
+ decisionHandler(.cancel)
+ return
+ }
+ decisionHandler(.allow)
+ return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 6174 - 6184, The shift-click
external-open path currently unconditionally calls NSWorkspace.shared.open(url)
then decisionHandler(.cancel); update the logic in the BrowserPanel navigation
handler so you capture the Bool result from NSWorkspace.shared.open(url) and
only call decisionHandler(.cancel) when it returns true, otherwise call
decisionHandler(.allow) (and optionally log the failure via dlog in DEBUG).
Locate the shift-click branch handling navigationAction, the
NSWorkspace.shared.open call and the decisionHandler invocation to implement
this conditional fallback.
|
Closing as duplicate of #1470 which takes the cleaner approach of modifying the Ghostty submodule directly. |
1 participant
Summary
Approach
Ghostty only fires
OPEN_URLwhen modifiers exactly match its link-mods config (Cmd on macOS). Adding Shift breaks both hover underline and the action. As a workaround, this PR strips Shift from the mods sent to Ghostty when Cmd is held — inhoverModsFromFlags,flagsChanged,mouseDown, andmouseUp— so Ghostty still sees plain Cmd. The real Shift state is then read viaNSEvent.modifierFlagsin theOPEN_URLhandler to force system browser.Known concern: This silently hides Shift from Ghostty for all mouse events when Cmd is held. A cleaner long-term approach would be a Ghostty C API like
ghostty_surface_link_url_at_pos()to query the link under the cursor without triggering the action — then no modifier stripping is needed. Happy to rework if Ghostty exposes this.Test plan
🤖 Generated with Claude Code
Summary by cubic
Shift+click opens links in the system default browser: Cmd+Shift+click for terminal links, and Shift+click for links in the embedded browser. Cmd+click behavior is unchanged.
Written for commit bf290a5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes