Repository navigation
Suppress cmd-hover path highlighting while terminal selection is active - #2579
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSuppresses Quick Look word-hover when Command is held while a terminal selection exists; refactors modifier extraction to flags-based handling; adds internal quicklook path-resolution helpers with a test-only wrapper; introduces UI-test capture utilities and a DEBUG-gated terminal cmd-click UI-test harness plus new UI tests and Xcode project entries. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant App as cmux App (DEBUG harness)
participant Socket as Control Socket
participant FS as Filesystem
TestRunner->>App: Launch with env (socket, capture files, manifest)
App->>Socket: Bind/listen control socket
TestRunner->>Socket: Connect & poll for ready manifest
TestRunner->>Socket: Send action command (e.g., cmd_click_token)
App->>App: Read terminal snapshot, compute token geometry
App->>App: Simulate input (CGEvents) / selection as requested
App->>App: On Cmd-down + selection -> evaluate shouldSuppressCommandPathHover
App->>App: Resolve candidate path via quicklook helpers
App->>FS: If open-capture configured, append resolved path to capture log
TestRunner->>FS: Read capture log / hover diagnostics JSON
TestRunner-->>App: Assert expected suppression and opened-path entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR suppresses the cmd-hover path-highlight and custom pointer-hand cursor while a terminal text selection is active. It adds two focused helpers — Confidence Score: 5/5Safe to merge — the change is tightly scoped, all event paths clean up cursor state correctly, and no P0/P1 issues were found. The suppression logic is applied consistently across all three entry points (flagsChanged, mouseMoved, mouseEntered). The shouldSuppressCommandPathHover guard is short-circuited when Cmd is not held, so there is no extra cost on non-Cmd moves. The wordPathHoverActive cursor stack is always balanced: mouseMoved after drag completion, flagsChanged on Cmd release, mouseExited, and viewDidMoveToWindow all pop it when needed. No P0 or P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cmd key pressed / mouse moved / mouse entered] --> B{shouldSuppressCommandPathHover}
B --> C{Cmd held?}
C -- No --> D[suppressPathHover = false]
C -- Yes --> E{ghostty_surface_has_selection?}
E -- No --> D
E -- Yes --> F[suppressPathHover = true]
D --> G[hoverModsFromFlags: full modifiers]
F --> H[hoverModsFromFlags: strip .command]
G --> I[ghostty_surface_mouse_pos with full mods]
H --> J[ghostty_surface_mouse_pos without .command]
I --> K[updateWordPathHover cmdHeld suppressPathHover=false]
J --> L[updateWordPathHover cmdHeld suppressPathHover=true]
K --> M{cmdHeld?}
M -- No --> N[Pop cursor if active]
M -- Yes --> O{resolveWordUnderCursorAsPath?}
O -- Found --> P[Push pointingHand cursor]
O -- Not found --> N
L --> N
Reviews (1): Last reviewed commit: "Suppress cmd hover path highlight during..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 6535-6548: The cmd-click fallback path activation code needs to
respect active text selection like the hover logic does: update the cmd-click
detection branch (the block that detects "cmd-click fallback path" where
hoverModsFromFlags is used) to first check selection state using
shouldSuppressCommandPathHover(_:) or directly via
ghostty_surface_has_selection(surface) and skip path activation when selection
is present; i.e., if shouldSuppressCommandPathHover(flags) is true, do not run
the cmd-click fallback path detection/activation and proceed with the regular
selection-aware behavior.
🪄 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: 0ff8bbdf-77dc-4f39-a098-ad8cba16a86d
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:6527">
P2: Command-hover suppression is computed but not applied to `ghostty_surface_mouse_pos`, so Cmd-modified hover can still be activated while a selection is active.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
6779-6785:⚠️ Potential issue | 🟠 MajorApply selection suppression before mouse button dispatch to avoid cmd-click leakage.
On Line 6792, release is sent before suppression is computed; if Ghostty clears selection during that event, the later check can miss prior active selection and still run fallback open logic. Also, mouse press/release still use full Command mods, so suppression is not consistently enforced in click paths.
🔧 Suggested patch
override func mouseDown(with event: NSEvent) { @@ - if event.clickCount == 1 { - ghostty_surface_mouse_pos(surface, point.x, bounds.height - point.y, modsFromEvent(event)) - } - _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, modsFromEvent(event)) + let suppressCommandPathHover = shouldSuppressCommandPathHover(for: event.modifierFlags) + let mouseMods = hoverModsFromFlags( + event.modifierFlags, + suppressCommandPathHover: suppressCommandPathHover + ) + if event.clickCount == 1 { + ghostty_surface_mouse_pos(surface, point.x, bounds.height - point.y, mouseMods) + } + _ = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_LEFT, mouseMods) } override func mouseUp(with event: NSEvent) { @@ - let consumed = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_RELEASE, GHOSTTY_MOUSE_LEFT, modsFromEvent(event)) + let suppressCommandPathHover = shouldSuppressCommandPathHover(for: event.modifierFlags) + let mouseMods = hoverModsFromFlags( + event.modifierFlags, + suppressCommandPathHover: suppressCommandPathHover + ) + let consumed = ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_RELEASE, GHOSTTY_MOUSE_LEFT, mouseMods) @@ - let suppressCommandPathHover = shouldSuppressCommandPathHover(for: event.modifierFlags) if !consumed, event.modifierFlags.contains(.command), !suppressCommandPathHover { @@ - ghostty_surface_mouse_pos(surface, point.x, bounds.height - point.y, modsFromEvent(event)) + ghostty_surface_mouse_pos(surface, point.x, bounds.height - point.y, mouseMods) tryOpenWordAsPath() } }Also applies to: 6792-6805
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6779 - 6785, Compute and apply the selection-suppression-modifiers before calling any ghostty_surface_mouse_button/ghostty_surface_mouse_pos so the suppressed modifier set is used for both press and release and the selection state check sees the correct suppressed mods; specifically, derive a suppressedMods = suppressedModsFromEvent(event) (or call the existing suppression helper) immediately after computing modsFromEvent(event), use suppressedMods for the ghostty_surface_mouse_pos and both ghostty_surface_mouse_button calls (GHOSTTY_MOUSE_PRESS/GHOSTTY_MOUSE_LEFT and the corresponding release), and ensure suppression logic runs before dispatching the release so clearing selection cannot race with your fallback open logic (update the click-handling paths that call ghostty_surface_mouse_button and modsFromEvent(event) accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6779-6785: Compute and apply the selection-suppression-modifiers
before calling any ghostty_surface_mouse_button/ghostty_surface_mouse_pos so the
suppressed modifier set is used for both press and release and the selection
state check sees the correct suppressed mods; specifically, derive a
suppressedMods = suppressedModsFromEvent(event) (or call the existing
suppression helper) immediately after computing modsFromEvent(event), use
suppressedMods for the ghostty_surface_mouse_pos and both
ghostty_surface_mouse_button calls (GHOSTTY_MOUSE_PRESS/GHOSTTY_MOUSE_LEFT and
the corresponding release), and ensure suppression logic runs before dispatching
the release so clearing selection cannot race with your fallback open logic
(update the click-handling paths that call ghostty_surface_mouse_button and
modsFromEvent(event) accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bf1637c-60d6-409d-915b-78e312ebbe98
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:6666">
P3: This nested debug condition is unreachable: `effectiveFlags` has already had `.command` removed, so `effectiveFlags.contains(.command)` is always false.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/TerminalCmdClickUITests.swift`:
- Around line 159-166: The XCTFail message currently calls socketJSON(method:
"surface.send_text", ...) again, which re-sends the mutation; instead call
socketJSON once and store its raw response (e.g., responseEnvelope or
responseRaw) before the guard, use that stored value to derive 'envelope' and
'ok', and interpolate the stored response in XCTFail—this ensures socketJSON is
invoked only once (refer to socketJSON, envelope, XCTFail, and the
"surface.send_text" call).
- Around line 320-331: The code mixes AppKit and Quartz coordinates by using
NSEvent.mouseLocation (AppKit Y-down) with quartzEnd (Quartz Y-up) when
interpolating drag points; convert the currentLocation to Quartz coordinates
before the interpolation (e.g., use the existing
quartzPoint(fromAccessibilityPoint:) or add a helper to transform
NSEvent.mouseLocation into Quartz space) so the loop that computes point for
postMouseEvent(type:.leftMouseDragged, at:point, ...) uses both endpoints in the
same coordinate system; keep the same clampedSteps and dragDuration logic but
perform the coordinate conversion once before the for loop.
- Around line 509-513: The current send loop treats any non-negative writeResult
as success and can leave part of outbound unwritten; change the logic around
outbound.withUnsafeBytes { ... send(fd, ...) } so that you loop until all bytes
are sent or an error occurs: keep track of bytesSent and bytesRemaining, call
send repeatedly using bytes.baseAddress.advanced(by: bytesSent) and
bytesRemaining as the length, update bytesSent by the positive return value, and
handle negative returns (errno) by returning nil; update the code that uses
outbound, send, writeResult and fd to implement this retrying write loop to
ensure the full request is written.
In `@Sources/cmuxApp.swift`:
- Around line 3909-3925: appendLine currently falls back to
payload.write(to:options:) inside the catch, which overwrites the whole file;
change the catch in appendLine(_ line:String, to url:URL) so it preserves
existing contents by reading existing data (if any) and writing existingData +
payload atomically, or by retrying an append via FileHandle seeking to end
before writing, instead of replacing the file. Ensure you still call
ensureParentDirectory(for:) and keep the original append path
(FileHandle(forWritingTo:)) as primary and only use the combined-data atomic
write as a safe fallback to maintain append-only semantics.
🪄 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: c4e96001-9347-462f-a76e-17c3f6edb82f
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/GhosttyTerminalView.swiftSources/cmuxApp.swiftcmuxUITests/TerminalCmdClickUITests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmuxUITests/TerminalCmdClickUITests.swift (1)
42-47: Decouple hover-test readiness from cmd-click fixture setup.
testHoldingCommandAfterSelectionSuppresssCommandHoverDispatchonly needsready == "1", butwaitForReadySetup()also blocks onexpectedPath. That means a cmd-click fixture/setup regression can fail the hover test before it ever reachesselect_token_and_hold_command.♻️ Suggested refactor
func testHoldingCommandAfterSelectionSuppresssCommandHoverDispatch() throws { let app = launchApp(captureOpenPaths: false, captureHoverDiagnostics: true) defer { app.terminate() } - _ = try waitForReadySetup() + try waitForHarnessReady() let result = try runCommand(action: "select_token_and_hold_command") @@ + private func waitForHarnessReady(timeout: TimeInterval = 15.0) throws { + let matched = waitForCondition(timeout: timeout) { + self.loadSetupData()?["ready"] as? String == "1" + } + + guard matched else { + throw NSError(domain: "TerminalCmdClickUITests", code: 1, userInfo: [ + NSLocalizedDescriptionKey: "Expected terminal cmd-click setup data. payload=\(loadSetupData() ?? [:])" + ]) + } + } + private func waitForReadySetup(timeout: TimeInterval = 15.0) throws -> SetupData { var setup: SetupData? let matched = waitForCondition(timeout: timeout) { guard let payload = self.loadSetupData(), payload["ready"] as? String == "1",Also applies to: 168-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/TerminalCmdClickUITests.swift` around lines 42 - 47, The test testHoldingCommandAfterSelectionSuppresssCommandHoverDispatch should not call waitForReadySetup() because that blocks on expectedPath; instead wait only for hover readiness (ready == "1") before running the fixture action. Replace the call to waitForReadySetup() in testHoldingCommandAfterSelectionSuppresssCommandHoverDispatch (and the other affected test at lines ~168-176) with a call to a readiness-only helper (e.g., waitForReady(ready: "1") or a new waitForReadyOnly function) so the test waits just for ready == "1" and then proceeds to runCommand(action: "select_token_and_hold_command"); keep references to waitForReadySetup, runCommand, and the test method names to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/TerminalCmdClickUITests.swift`:
- Around line 117-135: The launch path can return before the UI is actually
foregrounded; update launchAndEnsureForeground to use its timeout parameter,
treat only .runningForeground as success (not .runningBackground), and block
until the app's state is .runningForeground (or until key window/activation
evidence is present) before returning; then in launchApp call
launchAndEnsureForeground and only call/return after that confirmation so
app.activate() (if still needed) happens after the foreground check—refer to the
functions launchAndEnsureForeground and launchApp when making these changes.
In `@Sources/AppDelegate.swift`:
- Around line 3376-3379: The current logic only sets tokenPointPayload once when
hasRenderedToken is true, so if that payload contains tokenLayoutMatch = "0" we
never recompute and never detect the later "1"; modify the condition around
tokenPointPayload assignment in AppDelegate (the hasRenderedToken /
tokenPointPayload block) to recompute tokenPointPayload by calling
tokenPoints(in:terminalPanel, visibleText:visibleText) whenever hasRenderedToken
is true AND (tokenPointPayload is nil OR (tokenPointPayload?["tokenLayoutMatch"]
as? String) != "1"), or alternatively clear tokenPointPayload when
tokenLayoutMatch == "0" so subsequent polls invoke tokenPoints again; reference
tokenPointPayload, hasRenderedToken, tokenPoints(in:visibleText:), and
tokenLayoutMatch in your change.
- Around line 3302-3309: The test captures currentTabManager but later code
(evaluate()/timeout path) uses self.tabManager and looks up the first cmux.main*
window, causing a race; update the terminal snapshot and window lookup to use
the captured currentTabManager (and its selectedWorkspace/focusedTerminalPanel)
throughout the evaluate()/timeout paths and in writeState calls so the same
pinned manager/window pair is used for both reading textSnapshot and for the
simulated selection; specifically, replace references that call self.tabManager
or NSApp.windows.first... with lookups bound to currentTabManager and a window
resolved once from that captured context before passing into
writeState/evaluate/timeout routines.
---
Nitpick comments:
In `@cmuxUITests/TerminalCmdClickUITests.swift`:
- Around line 42-47: The test
testHoldingCommandAfterSelectionSuppresssCommandHoverDispatch should not call
waitForReadySetup() because that blocks on expectedPath; instead wait only for
hover readiness (ready == "1") before running the fixture action. Replace the
call to waitForReadySetup() in
testHoldingCommandAfterSelectionSuppresssCommandHoverDispatch (and the other
affected test at lines ~168-176) with a call to a readiness-only helper (e.g.,
waitForReady(ready: "1") or a new waitForReadyOnly function) so the test waits
just for ready == "1" and then proceeds to runCommand(action:
"select_token_and_hold_command"); keep references to waitForReadySetup,
runCommand, and the test method names to locate the changes.
🪄 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: 7bccfa5f-fa80-424d-b9b6-5354bf45150f
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftcmuxUITests/TerminalCmdClickUITests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
This was referenced Apr 13, 2026
This was referenced May 5, 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
2 participants
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
Testing
./scripts/reload.sh --tag cmd-hold-highlight --launch./scripts/reload.sh --tag cmd-hold-highlightFixes #2577
Summary by cubic
Suppresses Cmd-hover link/path highlighting and the hand cursor while a text selection is active; behavior without a selection is unchanged (fixes #2577). Improves Cmd-click path opening by resolving escaped/quoted/raw-space filenames with unquoting/unescaping, tilde/CWD expansion, and preferring the click-point snapshot path when quicklook or viewport offsets disagree.
TerminalCmdClickUITestsand a harness to simulate selection and cmd-hover/click, and to capture opened paths and hover diagnostics.Written for commit 641f115. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests