Repository navigation
Fix Slack composer Cmd+C in browser panes - #4126
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 (4)
✨ 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 |
![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 367cf01. Configure here.
| if result { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Document editing preflight may replay event into WebKit twice
Low Severity
When firstResponderWebView.performKeyEquivalent(with: event) returns false (both WebKit and the main menu declined), the window-level code falls through to cmux_performKeyEquivalent, which walks the view hierarchy and calls CmuxWebView.performKeyEquivalent again — triggering a second super.performKeyEquivalent into WebKit. The find command path at line 14846 explicitly prevents this by always returning true with a comment explaining that WebKit must not observe the same key equivalent twice. The new document editing path lacks this guard, so WebKit could see the event twice when neither it nor the menu claims it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 367cf01. Configure here.
There was a problem hiding this comment.
Fixed by suppressing fallthrough after the window-level browser editing preflight. I also added a menu-miss regression test that proves WebKit only receives Cmd+C once when both WebKit and the menu decline.
— Claude Code
Greptile SummaryRoutes Cmd+C, Cmd+X, and Cmd+A through WebKit before cmux falls back to the AppKit main menu, fixing Slack's contentEditable composer not updating the pasteboard when those keys were pressed in a focused browser child responder.
Confidence Score: 4/5The fix correctly routes document editing commands through WebKit before the main menu; the happy path is well-covered and produces no regression. The only rough edge is in the window-level fallthrough when neither WebKit nor the menu handles the command. The core routing change in CmuxWebView.swift is correct and the three new tests cover the primary scenarios. The AppDelegate window preflight departs from the unconditional-return-true pattern that the Find preflight uses to prevent double dispatch; when both WebKit and the menu decline from within the window preflight, cmux_performKeyEquivalent can re-invoke the same CmuxWebView, sending the event to WebKit and the menu a second time. This only surfaces in the nothing-to-copy edge case and produces no wrong clipboard state, but it violates an invariant explicitly documented in the adjacent Find-preflight block. Sources/AppDelegate.swift — the new editing command preflight block around line 14811 should be examined against the unconditional-return-true pattern used by the Find preflight directly below it. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Cmd+C)
participant W as NSWindow (swizzled)
participant CV as CmuxWebView
participant WK as WebKit (super)
participant M as NSApp.mainMenu
U->>W: performKeyEquivalent(Cmd+C)
Note over W: firstResponderWebView found
W->>CV: performKeyEquivalent(Cmd+C) [window preflight]
CV->>WK: super.performKeyEquivalent(Cmd+C)
alt WebKit handles (contentEditable focused)
WK-->>CV: true
CV-->>W: true
W-->>U: handled
else WebKit declines
WK-->>CV: false
CV->>M: mainMenu.performKeyEquivalent(Cmd+C)
alt Menu handles
M-->>CV: true
CV-->>W: true
W-->>U: handled
else Menu also declines
M-->>CV: false
CV-->>W: false
Note over W: Falls through to cmux_performKeyEquivalent
W->>CV: performKeyEquivalent(Cmd+C) [2nd invoke]
CV->>WK: super.performKeyEquivalent (2nd time)
CV->>M: mainMenu.performKeyEquivalent (2nd time)
end
end
Reviews (1): Last reviewed commit: "fix: route browser copy through web cont..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let firstResponderWebView, | ||
| shouldRouteBrowserDocumentEditingCommandEquivalentThroughWebContentFirst( | ||
| event, | ||
| responder: self.firstResponder | ||
| ) { | ||
| let result = firstResponderWebView.performKeyEquivalent(with: event) | ||
| #if DEBUG | ||
| cmuxDebugLog( | ||
| " → browser document editing command preflight " + | ||
| (result ? "resolved before window menu path" : "left unclaimed; continuing") | ||
| ) | ||
| #endif | ||
| if result { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
cmux_performKeyEquivalent, unlike the Find path
When firstResponderWebView.performKeyEquivalent returns false here (WebKit declined and the menu inside CmuxWebView also declined), the code falls through to cmux_performKeyEquivalent at line 14895. The original NSWindow.performKeyEquivalent will then send the event to the first responder again — and when that first responder is CmuxWebView itself, CmuxWebView.performKeyEquivalent is invoked a second time, causing WebKit and the main menu each to receive the same event twice.
The Find-command preflight right below this block explicitly avoids this by returning true unconditionally, with the comment: "Do not fall through into the original NSWindow.performKeyEquivalent path, or WebKit can observe the same key equivalent a second time." The same invariant applies to document editing commands, but this block only returns true on success, leaving the fallthrough path open. In practice the double dispatch produces the same false results both times (nothing to copy means neither invocation succeeds), but the second pass calls both super.performKeyEquivalent and NSApp.mainMenu.performKeyEquivalent redundantly.
There was a problem hiding this comment.
Fixed by making the window-level document-editing preflight terminal, matching the existing Find preflight invariant. Once the browser web view has seen Cmd+C/Cmd+X/Cmd+A, the window path returns true to suppress a second WebKit/menu replay.
— Claude Code
| XCTAssertTrue(webView.performKeyEquivalent(with: event!)) | ||
| XCTAssertEqual(forwardedEvents.count, 1) | ||
| XCTAssertEqual(forwardedEvents.first?.keyCode, 8) | ||
| XCTAssertTrue(spy.invoked) | ||
| } | ||
|
|
||
| @MainActor | ||
| func testWindowCmdCCopyPreflightsFocusedBrowserChildIntoWebContentBeforeMainMenu() { | ||
| _ = NSApplication.shared | ||
| AppDelegate.installWindowResponderSwizzlesForTesting() | ||
| installCmuxUnitTestWKWebViewPerformKeyEquivalentOverride() | ||
|
|
||
| let spy = ActionSpy() | ||
| installMenu(spy: spy, key: "c", modifiers: [.command]) | ||
|
|
||
| let window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), | ||
| styleMask: [.titled, .closable], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| let container = NSView(frame: window.contentRect(forFrameRect: window.frame)) | ||
| window.contentView = container | ||
|
|
||
| let webView = CmuxWebView(frame: container.bounds, configuration: WKWebViewConfiguration()) | ||
| webView.autoresizingMask = [.width, .height] | ||
| container.addSubview(webView) | ||
|
|
||
| let responder = FirstResponderView(frame: NSRect(x: 0, y: 0, width: 32, height: 20)) | ||
| webView.addSubview(responder) | ||
|
|
||
| var forwardedEvents: [NSEvent] = [] | ||
| cmuxUnitTestWKWebViewPerformKeyEquivalentHook = { currentWebView, event in | ||
| guard currentWebView === webView else { return nil } | ||
| forwardedEvents.append(event) | ||
| return true | ||
| } | ||
|
|
||
| window.makeKeyAndOrderFront(nil) | ||
| defer { | ||
| cmuxUnitTestWKWebViewPerformKeyEquivalentHook = nil | ||
| window.orderOut(nil) | ||
| } | ||
|
|
||
| XCTAssertTrue(window.makeFirstResponder(responder)) | ||
| guard let event = makeKeyDownEvent( | ||
| key: "c", | ||
| modifiers: [.command], | ||
| keyCode: 8, | ||
| windowNumber: window.windowNumber | ||
| ) else { | ||
| XCTFail("Failed to construct Cmd+C event") | ||
| return | ||
| } | ||
|
|
||
| XCTAssertTrue(window.performKeyEquivalent(with: event)) | ||
| XCTAssertEqual(forwardedEvents.count, 1) | ||
| XCTAssertEqual(forwardedEvents.first?.keyCode, 8) | ||
| XCTAssertFalse(spy.invoked) | ||
| } | ||
|
|
||
| func testReturnDoesNotRouteToMainMenuWhenWebViewIsFirstResponder() { | ||
| let spy = ActionSpy() | ||
| installMenu(spy: spy, key: "\r", modifiers: []) |
There was a problem hiding this comment.
The three new tests cover: (a) CmuxWebView as first responder + WebKit claims the event, (b) CmuxWebView as first responder + WebKit declines + menu claims, and (c) child view as first responder + WebKit claims. There is no case for (d): child view as first responder + WebKit declines + the menu spy should fire. That case exercises the full window-level fallthrough path added in AppDelegate.swift and would also expose the double-dispatch concern noted on the window block above if the menu did NOT claim the event.
There was a problem hiding this comment.
Added window-path coverage for focused browser child responders where WebKit declines and the AppKit menu fallback handles Cmd+C. The test asserts WebKit is invoked exactly once and the menu spy fires.
— Claude Code
This branch was successfully deployed
1 participant
Summary
Root cause
Slack's selected message text copied in cmux, but selected text inside Slack's contentEditable composer did not update the pasteboard. A JS copy probe saw copy events for message selections, but no copy event fired when Cmd+C was pressed in the focused composer. That ruled out Slack-specific clipboard permission/configuration as the primary failure and pointed at cmux's command-equivalent routing.
CmuxWebView.performKeyEquivalentrouted command equivalents toNSApp.mainMenu.performKeyEquivalentbefore WebKit for non-find commands. With Slack's composer focused through a child responder, AppKit's Edit > Copy path could consume Cmd+C before WebKit/page handlers saw the command, so Slack's contentEditable copy handler never ran and the pasteboard stayed unchanged.Tests
Closes #4123
Note
Medium Risk
Changes key-equivalent routing for browser panes at the window and
WKWebViewlevel, which could subtly affect other command shortcuts or menu handling across responder chains.Overview
Improves browser-pane keyboard routing so document editing command equivalents (Cmd+C, Cmd+X, Cmd+A) are first offered to WebKit/page handlers (e.g.,
contentEditable) before cmux falls back to the AppKit main-menu Edit actions.Adds shared shortcut detection (
shouldRouteBrowserDocumentEditingCommandEquivalentThroughWebContentFirst) and integrates it into bothNSWindow.performKeyEquivalentandCmuxWebView.performKeyEquivalent, including logic to avoid double-replaying the same shortcut. Adds regression tests ensuring Cmd+C preflights into WebKit (including when a focused child view is first responder) and still falls back to the main menu when unhandled.Reviewed by Cursor Bugbot for commit 367cf01. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes copying from Slack’s composer in browser panes by sending Cmd+C/Cmd+X/Cmd+A to WebKit first, then falling back to the Edit menu. Ensures contentEditable handlers run so the pasteboard updates correctly.
Written for commit 367cf01. Summary will update on new commits.