Repository navigation
Fix browser context menu opening the wrong link - #5780
Conversation
Right-click on a link, then "Open Link in Default Browser": the link that opens must be the contextmenu event target, not whatever a main-frame elementFromPoint hit test finds at the raw AppKit event coordinates. The two diverge under page zoom and inside iframes, opening the wrong link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"Open Link in Default Browser" / "Open Link in New Tab" resolved the link by re-running a main-frame document.elementFromPoint hit test at the raw AppKit event coordinates when the menu item was clicked. That re-resolution disagrees with the link the user right-clicked whenever CSS coordinates diverge from view points (pageZoom != 1) or the link lives in an iframe the main frame cannot hit-test, so the action opened the wrong link. Capture the link at contextmenu time instead: a document-start user script, injected into every frame in an isolated content world (same fingerprinting-safety pattern as the media-playback hook), reports the contextmenu event target's closest anchor to a private message handler. The menu actions prefer that captured link when it belongs to the same right-click as the open menu, falling back to the hit test otherwise. Also scale the remaining elementFromPoint fallbacks (link, image, debug inspect) by pageZoom so coordinate-based resolution is correct on zoomed pages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds an isolated-world document-start script to capture right-clicked link hrefs, timestamps captures and menu-open uptime, prefers recent captures when resolving menu actions, normalizes hit-testing to CSS viewport coordinates, provides robust JS fallbacks, integrates into CmuxWebView initializers, and adds tests and project wiring. ChangesContext-menu link capture and resolution
Sequence DiagramsequenceDiagram
participant Page as WebPage (document)
participant WK as CmuxWebView (WKContentWorld)
participant Handler as ContextMenuLinkCaptureMessageHandler
participant View as CmuxWebView (main actor)
participant AppKit as AppKit Menu Flow
participant Finder as DefaultBrowserOpener
Page->>WK: contextmenu (capture) with target element
WK->>Handler: postMessage({href, trusted})
Handler->>View: noteContextMenuCapturedLink(URL, uptime)
AppKit->>View: rightMouseDown -> willOpenMenu (record uptime)
View->>View: resolveContextMenuLinkURL(at:point)
alt recent captured link within max-age
View->>Finder: openContextMenuLinkInDefaultBrowser(captured URL)
else fallback
View->>WK: evaluate findLinkURLAtPoint(cssPoint)
View->>Finder: openContextMenuLinkInDefaultBrowser(found URL)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b796c19dd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| window.addEventListener("contextmenu", (event) => { | ||
| post(linkForEvent(event)); |
There was a problem hiding this comment.
Because this capture listener posts for every contextmenu event without checking event.isTrusted, page JavaScript can still forge link reports by dispatching synthetic MouseEvent('contextmenu') events on an arbitrary anchor; the new native menu action then prefers that captured URL for up to 2 seconds. On pages that dispatch such events shortly before or while the user opens the context menu, “Open Link in Default Browser” / “Open Link in New Tab” can open a page-chosen URL instead of the actual right-click target, despite the isolated message handler. Please only accept trusted user contextmenu events (or otherwise correlate the report to the native event).
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes "Open Link in Default Browser" (and related menu actions) opening the wrong URL by capturing the
Confidence Score: 5/5Safe to merge; the capture-first approach correctly fixes the wrong-link bug and the cssViewportPoint double-flip is properly addressed. The core fix captures the contextmenu event target in an isolated content world and pairs it with the AppKit menu via timestamp guards; four regression tests demonstrate it end-to-end. The rightMouseDown clearing and isTrusted gate close the mispairing and decoy-link vectors. The one new observation — internal var mutation surface on the three stored properties — is a style trade-off of the file-split, not a defect in the live code path. Sources/Panels/CmuxWebView.swift — the three capture-state stored properties are internal vars; direct assignment in mouseDown/rightMouseDown bypasses the method-level lifecycle defined in CmuxWebView+ContextMenuLinkCapture.swift. Important Files Changed
Reviews (6): Last reviewed commit: "Move ContextMenuCapturedLink into the ca..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// be while still describing the same right-click. | ||
| private static let contextMenuLinkCaptureMaxAge: TimeInterval = 2.0 | ||
|
|
||
| func noteContextMenuCapturedLink(_ url: URL?) { |
There was a problem hiding this comment.
noteContextMenuCapturedLink access level should be private
The method is only called from the Task { @MainActor } closure inside ContextMenuLinkCaptureMessageHandler, which is a private nested type declared inside CmuxWebView. In Swift, nested types share their enclosing type's lexical scope, so private members of CmuxWebView are fully accessible from within the nested class. Leaving the method internal (implicit) widens the access surface unnecessarily and suggests a future call site outside the class hierarchy could invoke it.
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!
| private func capturedContextMenuLinkURLForCurrentMenu() -> URL? { | ||
| guard let captured = contextMenuCapturedLink, | ||
| let menuOpenUptime = lastContextMenuOpenUptime, | ||
| abs(captured.uptime - menuOpenUptime) <= Self.contextMenuLinkCaptureMaxAge | ||
| else { return nil } | ||
| return captured.url | ||
| } |
There was a problem hiding this comment.
capturedContextMenuLinkURLForCurrentMenu() loses the distinction between "no valid capture" and "DOM confirmed no link"
The function returns nil for two meaningfully different states: (1) no capture arrived or the uptime delta exceeded the 2 s window; and (2) the capture arrived but href was empty (the contextmenu target was not a link). The call site treats both as identical and falls through to findLinkURLAtPoint. In theory, a capture that says "no link here" is authoritative evidence that the hit-test fallback should also produce nil — but because the fallback still runs, a zoomed-page or cross-frame scenario where the DOM event fired on a non-link element yet the AppKit hit-test lands on a nearby link could still resolve the wrong target. Representing this as URL?? (or a small enum) would make the three-state contract explicit and let the caller skip the fallback authoritatively when the capture confirmed no link.
| /// Link reported by the contextmenu capture hook for the most recent | ||
| /// right-click (`url` is nil when the click was not on a link). | ||
| private struct ContextMenuCapturedLink { | ||
| let url: URL? | ||
| let uptime: TimeInterval | ||
| } | ||
| private var contextMenuCapturedLink: ContextMenuCapturedLink? | ||
| /// Uptime at which the current context menu opened, used to pair the menu | ||
| /// with the contextmenu capture report from the same right-click. | ||
| private var lastContextMenuOpenUptime: TimeInterval? | ||
| /// How far apart the DOM contextmenu capture and the AppKit menu open may | ||
| /// be while still describing the same right-click. | ||
| private static let contextMenuLinkCaptureMaxAge: TimeInterval = 2.0 | ||
|
|
||
| func noteContextMenuCapturedLink(_ url: URL?) { | ||
| contextMenuCapturedLink = ContextMenuCapturedLink( | ||
| url: url, | ||
| uptime: ProcessInfo.processInfo.systemUptime | ||
| ) | ||
| } | ||
|
|
||
| private func capturedContextMenuLinkURLForCurrentMenu() -> URL? { | ||
| guard let captured = contextMenuCapturedLink, | ||
| let menuOpenUptime = lastContextMenuOpenUptime, | ||
| abs(captured.uptime - menuOpenUptime) <= Self.contextMenuLinkCaptureMaxAge | ||
| else { return nil } | ||
| return captured.url | ||
| } | ||
| /// Saved native WebKit action for "Download Image". |
There was a problem hiding this comment.
The two new instance variables contextMenuCapturedLink and lastContextMenuOpenUptime correlate events from two independent pipelines (DOM → WKScriptMessage → MainActor, and AppKit willOpenMenu) using a 2-second wall-clock window. Per the architectural-rethink rule, timing-based side channels should be flagged when bad state remains representable: if the message-delivery hop from the isolated content world to the main actor takes longer than 2 s (e.g. under abnormal system load or debugger attachment), the capture silently expires and resolveContextMenuLinkURL falls back to the hit-test path that this PR is trying to retire. The two mutable side-channel variables are also never cleared after the menu action fires, so stale state lingers until the next right-click. A per-event identifier (e.g. a monotonic counter incremented in willOpenMenu and echoed back through the message handler) would make the pairing unambiguous and remove the time-window heuristic entirely. There is no public WKWebView API to do this today, so this is a pragmatic limitation, but the current design should be documented as such.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
![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/Panels/CmuxWebView.swift (1)
1759-1771:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the captured-link resolver for every link-targeted context-menu action.
resolveContextMenuLinkURLfixes iframe/zoom skew for the open-link actions, butcontextMenuDownloadLinkedFilestill starts fromfindLinkURLAtPoint. Right-clicking a link inside an iframe will still re-hit-test the main frame and can download the wrong target or fall back unnecessarily.🐛 Proposed fix
- findLinkURLAtPoint(point) { [weak self] url in + resolveContextMenuLinkURL(at: point) { [weak self] url in guard let self else { return } self.debugContextDownload( "browser.ctxdl.resolve trace=\(traceID) kind=linked linkURL=\(url?.absoluteString ?? "nil")" )Based on learnings, "When a behavior is exposed through multiple entrypoints ... implement one shared action/model path and verify every entrypoint that should invoke it."
🤖 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/Panels/CmuxWebView.swift` around lines 1759 - 1771, contextMenuDownloadLinkedFile currently re-tests the page with findLinkURLAtPoint and can hit the wrong element in iframes/zoom; change it to use the shared resolver by calling resolveContextMenuLinkURL(at:completion:) (which already prefers capturedContextMenuLinkURLForCurrentMenu()) instead of directly invoking findLinkURLAtPoint so all link-targeted context-menu actions use the same captured-link logic and return the correct URL.Source: Coding guidelines
🤖 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/Panels/CmuxWebView.swift`:
- Around line 179-210: The injected context-menu listener
(contextMenuLinkCaptureBootstrapScriptSource) posts hrefs for every contextmenu
event and is vulnerable to synthetic events; update the injected script to only
post when event.isTrusted === true and the discovered href is non-empty to
prevent page JS steering URLs, and adjust the native pairing logic in
capturedContextMenuLinkURLForCurrentMenu() to require recent trusted captures
(e.g., ignore captures older than the existing 2.0s window or mark captures as
trusted) so only trusted, timely captures are resolved for “Open Link …”
actions.
---
Outside diff comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 1759-1771: contextMenuDownloadLinkedFile currently re-tests the
page with findLinkURLAtPoint and can hit the wrong element in iframes/zoom;
change it to use the shared resolver by calling
resolveContextMenuLinkURL(at:completion:) (which already prefers
capturedContextMenuLinkURLForCurrentMenu()) instead of directly invoking
findLinkURLAtPoint so all link-targeted context-menu actions use the same
captured-link logic and return the correct URL.
🪄 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: 0146ca00-09ba-4357-ad4e-44af6d4e4598
📒 Files selected for processing (2)
Sources/Panels/CmuxWebView.swiftcmuxTests/BrowserConfigTests.swift
| private static let contextMenuLinkCaptureBootstrapScriptSource = """ | ||
| (() => { | ||
| try { | ||
| const post = (href) => { | ||
| try { | ||
| window.webkit.messageHandlers["\(contextMenuLinkCaptureMessageHandlerName)"].postMessage({ | ||
| href: typeof href === "string" ? href : "" | ||
| }); | ||
| } catch (_) {} | ||
| }; | ||
| const linkForEvent = (event) => { | ||
| try { | ||
| const path = typeof event.composedPath === "function" ? event.composedPath() : []; | ||
| for (const node of path) { | ||
| if (!node || node.nodeType !== 1) continue; | ||
| const tag = node.tagName; | ||
| if ((tag === "A" || tag === "AREA") && node.href) return String(node.href); | ||
| } | ||
| const target = event.target; | ||
| if (target && target.closest) { | ||
| const link = target.closest("a[href],area[href]"); | ||
| if (link && link.href) return String(link.href); | ||
| } | ||
| } catch (_) {} | ||
| return ""; | ||
| }; | ||
| window.addEventListener("contextmenu", (event) => { | ||
| post(linkForEvent(event)); | ||
| }, true); | ||
| } catch (_) {} | ||
| })(); | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/Panels/CmuxWebView.swift"
test -f "$file" || { echo "Missing $file"; exit 1; }
echo "== Locate injected contextmenu bootstrap + handler name =="
rg -n "contextMenuLinkCaptureBootstrapScriptSource|addEventListener\\(\"contextmenu\"|postMessage\\(\\{\\s*href:|contextMenuLinkCaptureMessageHandlerName" "$file"
echo "== Show injected JS around listener =="
python3 - <<'PY'
import re
path="Sources/Panels/CmuxWebView.swift"
with open(path,'r',encoding='utf-8') as f:
s=f.read()
m=re.search(r'contextMenuLinkCaptureBootstrapScriptSource\s*=\s*"""([\s\S]*?)"""', s)
if not m:
print("No bootstrap script found")
raise SystemExit(0)
js=m.group(1)
start=js.find('window.addEventListener("contextmenu"')
print("JS snippet starting at contextmenu listener:\n")
print(js[start:start+700])
PY
echo "== Check for isTrusted anywhere in file (esp injected JS + native pairing) =="
rg -n "isTrusted" "$file" || true
echo "== Inspect native pairing / uptime matching for captured contextmenu link =="
rg -n "2\\.0 seconds|uptime|mach_absolute_time|Date\\(|timeInterval|contextmenu|menu open|resolveContextMenuLinkURL|captured" "$file"
echo "== Find resolveContextMenuLinkURL and its callers =="
rg -n "resolveContextMenuLinkURL" "$file"
rg -n "findLinkURLAtPoint" "$file"
rg -n "contextMenuDownloadLinkedFile" "$file"
echo "== Dump relevant surrounding regions for the specific functions =="
# Use sed to print around matches but keep output small
for pat in "resolveContextMenuLinkURL" "contextMenuDownloadLinkedFile" "findLinkURLAtPoint" "contextMenuLinkCapture" "capturedLink"; do
echo "--- Around: $pat ---"
line=$(rg -n "$pat" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-80))
end=$((line+120))
sed -n "${start},${end}p" "$file" | nl -ba | sed 's/^/ /'
fi
doneRepository: manaflow-ai/cmux
Length of output: 3793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/Panels/CmuxWebView.swift"
test -f "$file" || { echo "Missing $file"; exit 1; }
# Helper to print with line numbers without `nl`
pwl() { # args: start end
start="$1"; end="$2"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d: %s\n", NR, $0}' "$file"
}
echo "== Check injected contextmenu listener for any isTrusted guard =="
pwl 170 235
echo "== Check capturedContextMenuLinkURLForCurrentMenu() pairing (around 1059-1086) =="
pwl 1035 1095
echo "== Check resolveContextMenuLinkURL() (around 1759-1775) =="
pwl 1748 1795
echo "== Find who uses resolveContextMenuLinkURL vs findLinkURLAtPoint =="
echo "-- findLinkURLAtPoint callers (top matches) --"
rg -n "findLinkURLAtPoint\\(" "$file" | head -n 30
echo "-- resolveContextMenuLinkURL callers (top matches) --"
rg -n "resolveContextMenuLinkURL\\(" "$file" | head -n 30
echo "== Inspect contextMenuDownloadLinkedFile() to see if it uses captured link or point hit-testing =="
line=$(rg -n "contextMenuDownloadLinkedFile" "$file" | head -n1 | cut -d: -f1)
echo "contextMenuDownloadLinkedFile line: $line"
if [ -n "${line:-}" ]; then
pwl $((line-120)) $((line+160))
fiRepository: manaflow-ai/cmux
Length of output: 23186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/Panels/CmuxWebView.swift"
pwl(){ s="$1"; e="$2"; awk -v s="$s" -v e="$e" 'NR>=s && NR<=e {printf "%6d: %s\n", NR, $0}' "$file"; }
echo "== Find where lastContextMenuOpenUptime is set =="
rg -n "lastContextMenuOpenUptime" "$file"
line=$(rg -n "lastContextMenuOpenUptime\\s*=" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
pwl $((line-80)) $((line+80))
fi
echo "== Inspect resolveContextMenuLinkURL call sites (~2452, ~2460) =="
rg -n "resolveContextMenuLinkURL\\(at: point\\)" "$file"
for l in $(rg -n "resolveContextMenuLinkURL\\(at: point\\)" "$file" | cut -d: -f1 | head -n 5); do
pwl $((l-60)) $((l+80))
done
echo "== Inspect contextMenuDownloadLinkedFile(_:) body (~2679) =="
line=$(rg -n "`@objc` private func contextMenuDownloadLinkedFile" "$file" | head -n1 | cut -d: -f1)
pwl $((line-80)) $((line+220))Repository: manaflow-ai/cmux
Length of output: 36585
Harden context-menu link capture against synthetic contextmenu events
The injected contextmenu listener posts a captured href for every event (no event.isTrusted check), and the native code pairs it within a 2.0 second window when resolving “Open Link …” actions—allowing page JS to steer the wrong URL. Also consider the pairing logic at capturedContextMenuLinkURLForCurrentMenu() (~1080–1086).
🐛 Proposed hardening
window.addEventListener("contextmenu", (event) => {
+ if (!event || event.isTrusted !== true) return;
post(linkForEvent(event));
}, true);🤖 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/Panels/CmuxWebView.swift` around lines 179 - 210, The injected
context-menu listener (contextMenuLinkCaptureBootstrapScriptSource) posts hrefs
for every contextmenu event and is vulnerable to synthetic events; update the
injected script to only post when event.isTrusted === true and the discovered
href is non-empty to prevent page JS steering URLs, and adjust the native
pairing logic in capturedContextMenuLinkURLForCurrentMenu() to require recent
trusted captures (e.g., ignore captures older than the existing 2.0s window or
mark captures as trusted) so only trusted, timely captures are resolved for
“Open Link …” actions.
…e.swift Keeps CmuxWebView.swift and BrowserConfigTests.swift inside the Swift file length budget: the capture hook, link resolution helpers, and the zoom-aware hit-test coordinate conversion move to a dedicated extension file, and the regression test gets its own wired test file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Panels/CmuxWebView+ContextMenuLinkCapture.swift (1)
60-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore synthetic
contextmenuevents in the capture hook.This listener records page-scripted
contextmenuevents too. Because the native side later accepts any capture within the 2s pairing window, page JS can overwrite the stored href and steer the “Open Link …” actions to a different URL.🔒 Minimal hardening
window.addEventListener("contextmenu", (event) => { + if (!event || event.isTrusted !== true) return; post(linkForEvent(event)); }, true);🤖 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/Panels/CmuxWebView`+ContextMenuLinkCapture.swift around lines 60 - 62, The contextmenu listener currently captures synthetic page‑scripted events; update the handler registered in window.addEventListener("contextmenu", ...) to ignore non-user events by checking event.isTrusted (only call post(linkForEvent(event)) when event.isTrusted is true) so that synthetic/contextmenu events from page JS cannot overwrite the stored href; keep the existing linkForEvent and post usages but gate them behind this trust check.
🤖 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/Panels/CmuxWebView`+ContextMenuLinkCapture.swift:
- Around line 218-221: The current subtree-wide fallback in collectChain(start)
uses el.querySelector('a[href],area[href]') which can pick up unrelated
descendant links when walking up to wrapper ancestors; remove that querySelector
fallback and instead only accept links that belong to the current element itself
(e.g. test el.matches('a[href],area[href]') or check immediate children only)
before calling normalize, so collectChain only captures links that are directly
on the node being inspected rather than anywhere in its subtree.
---
Duplicate comments:
In `@Sources/Panels/CmuxWebView`+ContextMenuLinkCapture.swift:
- Around line 60-62: The contextmenu listener currently captures synthetic
page‑scripted events; update the handler registered in
window.addEventListener("contextmenu", ...) to ignore non-user events by
checking event.isTrusted (only call post(linkForEvent(event)) when
event.isTrusted is true) so that synthetic/contextmenu events from page JS
cannot overwrite the stored href; keep the existing linkForEvent and post usages
but gate them behind this trust check.
🪄 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: 7c90bb08-fe81-43ca-b9de-55e931fd245a
📒 Files selected for processing (4)
Sources/Panels/CmuxWebView+ContextMenuLinkCapture.swiftSources/Panels/CmuxWebView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxWebViewContextMenuLinkCaptureTests.swift
| if (el.querySelector) { | ||
| const nestedLink = el.querySelector('a[href],area[href]'); | ||
| if (nestedLink && nestedLink.href) return normalize(nestedLink.href); | ||
| } |
There was a problem hiding this comment.
Drop the subtree-wide fallback here.
collectChain(start) eventually reaches wrapper ancestors like cards or body, and el.querySelector('a[href],area[href]') then returns the first descendant link anywhere in that subtree. When capture is unavailable, that can still resolve the wrong link.
💡 Safer fallback
- if (el.querySelector) {
- const nestedLink = el.querySelector('a[href],area[href]');
- if (nestedLink && nestedLink.href) return normalize(nestedLink.href);
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (el.querySelector) { | |
| const nestedLink = el.querySelector('a[href],area[href]'); | |
| if (nestedLink && nestedLink.href) return normalize(nestedLink.href); | |
| } |
🤖 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/Panels/CmuxWebView`+ContextMenuLinkCapture.swift around lines 218 -
221, The current subtree-wide fallback in collectChain(start) uses
el.querySelector('a[href],area[href]') which can pick up unrelated descendant
links when walking up to wrapper ancestors; remove that querySelector fallback
and instead only accept links that belong to the current element itself (e.g.
test el.matches('a[href],area[href]') or check immediate children only) before
calling normalize, so collectChain only captures links that are directly on the
node being inspected rather than anywhere in its subtree.
…contextmenu decoy The dogfood probe log (tag ctxlk2) showed the coordinate fallback resolving the mirrored link on every right-click: WKWebView is a flipped view on macOS, so subtracting from bounds.height double-flips Y. It also showed synthetic contextmenu events are recorded just like real ones, so page JavaScript can plant a decoy link (flagged by review bots on the PR). Tests only; the untrusted-events test seam is declared but not yet enforced, so both new tests fail at this commit.
…scope capture to one click Three fixes from the ctxlk2 dogfood probe log: - cssViewportPoint no longer flips Y: WKWebView is a flipped view on macOS, so view-local points are already top-left-origin. The old subtraction mirrored every fallback hit test vertically (probe log: clicking the top link resolved the bottom link, an HN title resolved the row's upvote URL). This was the pre-capture root cause of 'open in default browser opens the wrong link sometimes' and also affected image download/copy resolution, which share the helper. - The capture message handler ignores synthetic contextmenu events (isTrusted == false), so page JavaScript cannot plant a decoy link (review-bot finding). Unit tests opt back in via the test seam. - rightMouseDown / ctrl-mouseDown clear the previous capture, so a context menu can only pair with the link captured by the click that opened it; the 2s pairing window now only bounds menu paths that never saw a mouse event. Probe evidence (6/6 right-clicks): capture stored 10-24ms before willOpenMenu and always matched the clicked link, while the coordinate fallback disagreed every time until the Y-flip fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2f359056
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let trusted = body?["trusted"] as? Bool ?? false | ||
| let href = body?["href"] as? String ?? "" | ||
| let url = href.isEmpty ? nil : URL(string: href) | ||
| Task { @MainActor [weak webView] in |
There was a problem hiding this comment.
When WebKit delivers the contextmenu script message during the same right-click that opens the native menu, wrapping the state update in a MainActor task lets super.rightMouseDown continue into menu tracking before contextMenuCapturedLink is set. If the user then chooses “Open Link in Default Browser” / “Open Link in New Tab” on a zoomed page or iframe, resolveContextMenuLinkURL can still see no captured link and fall back to the coordinate hit test, reproducing the wrong-link behavior this change is meant to fix. Record the trusted report synchronously in the script-message callback instead of enqueueing it.
Useful? React with 👍 / 👎.
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/Panels/CmuxWebView.swift (1)
2431-2456:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the captured-link resolver for "Download Linked File".
This action still begins with
findLinkURLAtPoint(point), so right-clicks on zoomed pages or iframe content can still download the wrong target even thoughresolveContextMenuLinkURL(at:completion:)now has the captured href for this exact menu open.💡 Minimal fix
- findLinkURLAtPoint(point) { [weak self] url in + resolveContextMenuLinkURL(at: point) { [weak self] url in guard let self else { return } self.debugContextDownload( "browser.ctxdl.resolve trace=\(traceID) kind=linked linkURL=\(url?.absoluteString ?? "nil")" )🤖 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/Panels/CmuxWebView.swift` around lines 2431 - 2456, In contextMenuDownloadLinkedFile, stop using findLinkURLAtPoint(point) and instead call the captured-link resolver resolveContextMenuLinkURL(at:completion:) to obtain the exact href captured when the context menu opened; then proceed with the same normalization (normalizedLinkedDownloadURL) and isDownloadSupportedScheme checks and call startContextMenuDownload as before. Update the async completion closure references (currently using findLinkURLAtPoint's callback) to use resolveContextMenuLinkURL(at:completion:) and keep the existing trace/logging and fallback handling unchanged.
🤖 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 `@cmuxTests/CmuxWebViewContextMenuLinkCaptureTests.swift`:
- Line 155: Replace the locale-dependent title match on menu.items with a
selector-based match: instead of searching menu.items.first { $0.title == "Open
Link in Default Browser" }, find the item by comparing $0.action to the selector
that performs the "open link in default browser" behavior (e.g.,
`#selector`(yourTarget.openLinkInDefaultBrowser:)) so the test uses menu.items and
the resulting item variable but is independent of localized UI text.
---
Outside diff comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 2431-2456: In contextMenuDownloadLinkedFile, stop using
findLinkURLAtPoint(point) and instead call the captured-link resolver
resolveContextMenuLinkURL(at:completion:) to obtain the exact href captured when
the context menu opened; then proceed with the same normalization
(normalizedLinkedDownloadURL) and isDownloadSupportedScheme checks and call
startContextMenuDownload as before. Update the async completion closure
references (currently using findLinkURLAtPoint's callback) to use
resolveContextMenuLinkURL(at:completion:) and keep the existing trace/logging
and fallback handling unchanged.
🪄 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: 0ea80cad-ccf9-4c50-83b3-fc14d72932c5
📒 Files selected for processing (3)
Sources/Panels/CmuxWebView+ContextMenuLinkCapture.swiftSources/Panels/CmuxWebView.swiftcmuxTests/CmuxWebViewContextMenuLinkCaptureTests.swift
| ) | ||
| webView.willOpenMenu(menu, with: rightMouseDown) | ||
|
|
||
| let item = try XCTUnwrap(menu.items.first { $0.title == "Open Link in Default Browser" }) |
There was a problem hiding this comment.
Avoid English-title matching for a localized menu item in tests.
Line 155 finds the action item by "Open Link in Default Browser", which makes this regression test locale-dependent. Match by selector instead so the test validates behavior independent of UI language.
Suggested fix
- let item = try XCTUnwrap(menu.items.first { $0.title == "Open Link in Default Browser" })
+ let openInDefaultBrowser = Selector(("contextMenuOpenLinkInDefaultBrowser:"))
+ let item = try XCTUnwrap(menu.items.first { $0.action == openInDefaultBrowser })As per coding guidelines, user-facing text is localized across supported locales, so tests should avoid hard-coding English UI labels when validating behavior.
🤖 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 `@cmuxTests/CmuxWebViewContextMenuLinkCaptureTests.swift` at line 155, Replace
the locale-dependent title match on menu.items with a selector-based match:
instead of searching menu.items.first { $0.title == "Open Link in Default
Browser" }, find the item by comparing $0.action to the selector that performs
the "open link in default browser" behavior (e.g.,
`#selector`(yourTarget.openLinkInDefaultBrowser:)) so the test uses menu.items and
the resulting item variable but is independent of localized UI text.
Source: Coding guidelines
…andler
Autoreview finding: the Task { @mainactor } hop unordered the capture store
relative to rightMouseDown clearing and willOpenMenu, so a deferred report
from the previous click could repopulate the capture after the clear and pair
with the new menu. WebKit delivers script messages on the main thread; apply
synchronously via MainActor.assumeIsolated, same pattern and reasoning as
BrowserMediaPlaybackMessageHandler.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d0e8d36. Configure here.
…e IPC hop Drops isTrusted == false events inside the capture hook itself instead of in the native handler: the page cannot plant a decoy link and a synthetic dispatch loop cannot flood the script-message bridge. The test seam is baked into the script at install time; the production bridge path never consults it.
Repo test policy: new non-UI tests use Swift Testing. Same three regression tests, now a @mainactor @suite(.serialized) struct following the BrowserWebContentProcessTests pattern.
…tale captures by menu-event time Two autoreview findings: - contextMenuDownloadLinkedFile resolved with the raw coordinate hit test; it now goes through resolveContextMenuLinkURL like the Open Link actions, so the captured contextmenu target wins and coordinates are only a fallback. - Pairing now requires the capture to be newer than the NSEvent that opened the menu (same uptime clock, and the DOM contextmenu event always follows the AppKit event for the same interaction). A keyboard-opened menu can no longer reuse a capture left over from an earlier right-click; mouse-down clearing remains as defense in depth.
Aziz file-organization finding: the type, its writer, and its pairing logic now live together in CmuxWebView+ContextMenuLinkCapture.swift; only the stored property stays in the class body (extensions cannot add stored properties).
This branch was successfully deployed
1 participant
Summary
document.elementFromPointhit test at the raw AppKit event coordinates, which disagrees with the real click target whenever CSS coordinates diverge from view points (page zoom != 100%) or the link lives inside an iframe the main frame cannot hit-test.contextmenutime. A document-start user script, injected into every frame in an isolated content world (same fingerprinting-safety pattern as the media-playback hook inBrowserPanel+MediaPlayback.swift), reports the event target's closest anchor to a private message handler. The menu actions prefer that captured link when it belongs to the same right-click as the open menu (paired by uptime, 2s window) and fall back to the old hit test otherwise.elementFromPointfallbacks (link, image, debug inspect) bypageZoomso coordinate-based resolution is correct on zoomed pages.Sources/Panels/CmuxWebView+ContextMenuLinkCapture.swiftsoCmuxWebView.swiftandBrowserConfigTests.swiftstay inside the Swift file length budget.Testing
-only-testingon the AWS M4 Pro fleet Mac (aws-m4pro-2) because the CItestsjob's app-host suite currently times out mid-suite and does not reliably reach this class:testOpenLinkInDefaultBrowserOpensTheLinkUnderTheRightClickfails withXCTAssertEqual failed: ("https://example.test/decoy") is not equal to ("https://example.test/clicked"), the exact wrong-link symptom.CmuxWebViewContextMenuLinkCaptureTestsand all 4 pre-existingCmuxWebViewContextMenuTestspass.contextmenuevent on one link while the AppKit menu event points at the other, and asserts the right-clicked link is what opens.ctxlink(cloud builder) launched locally; browser pane preflighted via the debug socket: page loads, contextmenu dispatch runs with no JS errors, and the private capture handler is not visible to page JavaScript (isolated content world confirmed).Issues
Note
Medium Risk
Touches injected WebKit scripts and context-menu navigation paths across all browser panes; pairing and spoofing guards reduce but do not eliminate edge cases on non-mouse menu opens.
Overview
Fixes browser context menu actions (Open Link in Default Browser, Open Link in New Tab, linked-file download) opening the wrong URL when link resolution disagreed with the actual right-click target (page zoom, iframes, and a vertical mirror bug in coordinate fallbacks).
A new
CmuxWebView+ContextMenuLinkCaptureextension injects a document-start script in an isolatedWKContentWorldthat records thecontextmenuevent’s anchor and pairs it with the open menu via uptime/timestamp guards.resolveContextMenuLinkURLprefers that capture and only then falls back toelementFromPoint.cssViewportPointrespects flipped WKWebView coordinates andpageZoomfor image/link/debug hit tests. Captures are cleared on right- and ctrl-click; untrusted syntheticcontextmenuevents are ignored (with a test-only opt-in). Unit tests cover capture vs skew, stale pairing, spoofing, and coordinate math.Reviewed by Cursor Bugbot for commit c1bbdb3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Round 2: live probe evidence and three more fixes
A probe build (tag
ctxlk2) instrumented capture arrival, pairing, and both resolution paths during a live dogfood session. Six real right-clicks showed the capture stored 10-24ms beforewillOpenMenuand always matched the clicked link, while the coordinate fallback resolved the wrong link on every single click, including a flat page at 100% zoom (top link resolved as the bottom link; an HN title resolved as the row's upvote URL).Root cause of the fallback wrongness: WKWebView is a flipped NSView on macOS, so view-local points are already top-left-origin;
bounds.height - point.ydouble-flipped and mirrored every hit test vertically. This predates this PR (four copies of the subtraction on main) and is the original root cause of the wrong-link reports. It also affected the image download/copy fallbacks, which sharecssViewportPoint.Fixes added (red/green: tests at the first commit fail, pass after the second):
cssViewportPointrespectsisFlippedinstead of always re-flipping.contextmenuevents (isTrusted == false), closing the decoy-link spoof the review bots flagged. Unit tests opt back in throughcontextMenuLinkCaptureAcceptsUntrustedEventsForTesting.rightMouseDown/ ctrl-mouseDownclear the previous capture so a menu can only pair with the link captured by the click that opened it; the 2s window now only bounds non-mouse menu paths.Verified on
aws-m4pro-2: at the test-only committestCssViewportPointDoesNotReflipFlippedViewCoordinatesandtestSyntheticContextMenuEventCannotPlantDecoyLinkfail and the original capture test passes; at head all 3 pass.