Skip to content

fix: open terminal links on cmd-click even when mouse reporting is active #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 2 commits into from
Jun 5, 2026
Merged

fix: open terminal links on cmd-click even when mouse reporting is active #71

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c7613c
fix: open terminal links on cmd-click even when mouse reporting is ac…
doronpr Jun 3, 2026
55d154a
fix: gate link refresh on effective mouse-reporting state
doronpr Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 70 additions & 12 deletions src/Surface.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2785,13 +2785,11 @@ pub fn keyCallback(
// Update our modifiers, this will update mouse mods too
self.modsChanged(event.mods);

// We only refresh links if
// 1. mouse reporting is off
// OR
// 2. mouse reporting is on and we are not reporting shift to the terminal
if (self.io.terminal.flags.mouse_event == .none or
(self.mouse.mods.shift and !self.mouseShiftCapture(false)))
{
// We refresh links when local link handling is allowed for the
// current mouse-reporting state and mods (see mouseLinkRefreshAllowed):
// mouse reporting off, shift releasing capture, or the ctrl/super link
// modifier held so Cmd-click opens links inside a mouse-grabbing TUI.
if (self.mouseLinkRefreshAllowed()) {
// Refresh our link state
const pos = self.rt_surface.getCursorPos() catch break :mouse_mods;
self.renderer_state.mutex.lock();
Expand Down Expand Up @@ -3813,6 +3811,35 @@ fn mouseShiftCapture(self: *const Surface, lock: bool) bool {
};
}

/// Returns true if link hover/highlight state should be evaluated locally
/// instead of handing the mouse event to the running program.
///
/// We always evaluate links when mouse reporting is off. When a program has
/// mouse reporting enabled we still evaluate them if the user is holding
/// shift to release the mouse from capture, or is holding the ctrl/super
/// link-activation modifier. The latter lets Cmd-click (macOS) or Ctrl-click
/// open a link even while a fullscreen/alternate-screen TUI has grabbed the
/// mouse, matching iTerm2 and macOS Terminal.
fn mouseLinkRefreshAllowed(self: *const Surface) bool {
return mouseLinkRefreshAllowedState(
self.isMouseReporting(),
self.mouseShiftCapture(false),
self.mouse.mods,
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

/// Pure decision logic for `mouseLinkRefreshAllowed`, split out so it can be
/// unit tested without constructing a `Surface`.
fn mouseLinkRefreshAllowedState(
mouse_reporting: bool,
shift_capture: bool,
mods: input.Mods,
) bool {
if (!mouse_reporting) return true;
if (mods.shift and !shift_capture) return true;
return mods.equal(input.ctrlOrSuper(.{}));

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Bypass condition doesn't cover custom link modifiers

mouseLinkRefreshAllowedState bypasses mouse reporting only for the exact ctrlOrSuper(.{}) chord. For OSC 8 links that's the right check (it mirrors linkAtPos line 4413), but for regex links the activation mod comes from Config.link.highlight.hover_mods, which users can configure to something else (e.g. ctrl+alt). A user who configured a non-default hover_mods will see the bypass condition silently fail under mouse reporting because the exact-match against the default chord won't fire for their custom mod.

This is a design limitation that could be addressed by threading the configured hover_mods into this check, but it's out of scope for the current fix and the PR description acknowledges the default modifier scope.

}

/// Returns true if the mouse is currently captured by the terminal
/// (i.e. reporting events).
pub fn mouseCaptured(self: *Surface) bool {
Expand Down Expand Up @@ -4683,14 +4710,12 @@ pub fn cursorPosCallback(
// 2. the cursor position has changed (either we have no previous state, or the state has
// changed)
// AND
// 1. mouse reporting is off
// OR
// 2. mouse reporting is on and we are not reporting shift to the terminal
// local link handling is allowed for the current mouse-reporting state
// and mods (see mouseLinkRefreshAllowed)
if ((over_link or
self.mouse.link_point == null or
(self.mouse.link_point != null and !self.mouse.link_point.?.eql(pos_vp))) and
(self.io.terminal.flags.mouse_event == .none or
(self.mouse.mods.shift and !self.mouseShiftCapture(false))))
self.mouseLinkRefreshAllowed())
{
// If we were previously over a link, we always update. We do this so that if the text
// changed underneath us, even if the mouse didn't move, we update the URL hints and state
Expand Down Expand Up @@ -6531,6 +6556,39 @@ pub fn getProcessInfo(self: *Surface, comptime info: ProcessInfo) ?ProcessInfo.T
return self.io.getProcessInfo(info);
}

test "Surface: mouseLinkRefreshAllowedState honors ctrl/super under mouse reporting" {
const ctrl_or_super = input.ctrlOrSuper(.{});

// Mouse reporting off: links are always evaluated, regardless of mods.
try std.testing.expect(mouseLinkRefreshAllowedState(false, false, .{}));
try std.testing.expect(mouseLinkRefreshAllowedState(false, false, ctrl_or_super));

// Mouse reporting on, no relevant mods: the event is reported to the app,
// links are not evaluated locally.
try std.testing.expect(!mouseLinkRefreshAllowedState(true, false, .{}));

// Mouse reporting on, ctrl/super link modifier held: links are evaluated
// so Cmd-click (macOS) / Ctrl-click opens a link even while a
// fullscreen/alternate-screen TUI has grabbed the mouse. This is the
// behavior that was missing in cmux issue #5128.
try std.testing.expect(mouseLinkRefreshAllowedState(true, false, ctrl_or_super));

// Same as above but with shift-capture enabled: the ctrl/super link path
// must not be gated on shift_capture, since shift is not part of the chord.
try std.testing.expect(mouseLinkRefreshAllowedState(true, true, ctrl_or_super));

// Mouse reporting on, shift held and shift-capture disallowed: evaluated
// (pre-existing shift-release-from-capture behavior, unchanged).
try std.testing.expect(mouseLinkRefreshAllowedState(true, false, .{ .shift = true }));

// Mouse reporting on, shift held but shift-capture allowed: reported.
try std.testing.expect(!mouseLinkRefreshAllowedState(true, true, .{ .shift = true }));

// Mouse reporting on, ctrl/super plus a non-shift modifier: not an exact
// link-activation chord, so the event is reported to the app.
try std.testing.expect(!mouseLinkRefreshAllowedState(true, false, input.ctrlOrSuper(.{ .alt = true })));
Comment on lines +6562 to +6589

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test matrix is missing one cell

The truth table covers (mouse_event_active=true, shift_capture=false, ctrl_or_super) but not (true, shift_capture=true, ctrl_or_super). When shift capture is enabled and ctrl/super is the only mod, the function should still return true because mods.shift is false and we fall through to the mods.equal(ctrlOrSuper) check. Adding that assertion would prevent a future refactor from inadvertently gating the ctrl/super path on shift_capture.

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!

}

test "Surface: selection logic" {
// We disable format to make these easier to
// read by pairing sets of coordinates per line.
Expand Down
Loading