Repository navigation
Fix Cmd+Shift+Enter pane zoom for browser panes - #3520
Merged
Merged
Conversation
The browser pane regression needs an executable guard before changing the shared shortcut routing path. The new test focuses a browser panel, sends Cmd+Shift+Return through the WKWebView key-equivalent surface, and expects the existing split zoom action to run. Constraint: Regression policy requires a failing test-only commit before the fix Confidence: high Scope-risk: narrow Tested: ./scripts/test-unit.sh test -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdShiftReturnFocusedBrowserTogglesSplitZoom (fails: WebView returns false and split zoom remains off)
Browser Return key equivalents were classified inside CmuxWebView before the configured shortcut router could claim Cmd+Shift+Return. The default Return shortcut was also misclassified as unbound because the shortcut model trimmed newline characters. Treat only an empty key as unbound, let Return-bearing browser key equivalents ask the central AppDelegate shortcut handler before falling through to WebKit, and route the zoom action through the event-window manager. Constraint: Plain Return and unclaimed Cmd+Return must still reach WebKit keyDown for forms and editors Rejected: Hardcode Cmd+Shift+Return in CmuxWebView | would duplicate shortcut configuration and miss remapped pane-zoom bindings Confidence: high Scope-risk: moderate Directive: Keep Return-bearing browser shortcuts routed through AppDelegate first; do not add one-off Return guards in WebView Tested: ./scripts/test-unit.sh test -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdShiftReturnFocusedBrowserTogglesSplitZoom Not-tested: XCUITests; per project policy these run in CI
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR fixes a regression where Cmd+Shift+Enter no longer toggles pane zoom when a browser pane is focused. It routes the toggleSplitZoom shortcut action through the correct window context instead of always using the cached tabManager, adjusts unbound shortcut detection to treat only empty (not whitespace-only) keys as unbound, enables browser Return/Enter key equivalents to route through the app's shortcut handler, and adds a test to verify the fix. ChangesSplit-Zoom Shortcut Routing & Browser Key Handling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 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 SummaryFixes Cmd+Shift+Enter pane zoom when a browser pane is focused by addressing two independent root causes: Confidence Score: 5/5Safe to merge — the two root causes are independently addressed and the new test exercises both the monitor path and the All three changes are small and tightly scoped: the No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CmuxWebView
participant AppDelegate
participant handleCustomShortcut
participant WebKit
User->>CmuxWebView: Cmd+Shift+Return (keyCode 36/76)
Note over CmuxWebView: Previously: return false unconditionally
CmuxWebView->>AppDelegate: handleBrowserSurfaceKeyEquivalent(event)
AppDelegate->>handleCustomShortcut: handleCustomShortcut(event)
alt shortcut matches (e.g. toggleSplitZoom)
handleCustomShortcut-->>AppDelegate: true
AppDelegate-->>CmuxWebView: true
CmuxWebView-->>User: event consumed, zoom toggled
else no shortcut match (plain Return, form submit)
handleCustomShortcut-->>AppDelegate: false
AppDelegate-->>CmuxWebView: false
CmuxWebView->>WebKit: keyDown (form submission path)
WebKit-->>User: form submitted
end
Reviews (3): Last reviewed commit: "Keep browser shortcut timing accurate" | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI blocker: file length budget exceeded — trim 7 lines before merging.
The pipeline reports
actual=2498, budget=2491. The changed block inperformKeyEquivalentgrew from 3 lines (if … { return false }) to 12 lines (comment + nested if +#if DEBUG+return false), a net +9 that pushes the file 7 lines over budget.The quickest path to compliance is to extract one of the self-contained top-level types — for example,
BrowserImageCopyPasteboardPayload+BrowserImageCopyPasteboardBuilder(lines 26–103, ≈78 lines) — into a newSources/Panels/BrowserImageCopyPasteboardBuilder.swiftfile, which removes the budget pressure here and in any future change to this file.Based on learnings: "In this repo's CI enforces a Swift file-length budget for large view files… When adding helper views/small components, avoid bloating the existing file: extract the subview into a dedicated Swift file under Sources… and keep it under the CI length threshold."
🤖 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` at line 1, This file exceeds the CI file-length budget because the new performKeyEquivalent block expanded the file; fix by extracting the top-level types BrowserImageCopyPasteboardPayload and BrowserImageCopyPasteboardBuilder into a new Swift file (e.g., Sources/Panels/BrowserImageCopyPasteboardBuilder.swift). Move the entire definitions of BrowserImageCopyPasteboardPayload and BrowserImageCopyPasteboardBuilder out of CmuxWebView.swift, ensure they keep the same access levels and any required imports (e.g., AppKit/Foundation), update any references in CmuxWebView (e.g., where BrowserImageCopyPasteboardBuilder is instantiated or BrowserImageCopyPasteboardPayload is used) to the new file, and run the build to confirm no missing symbols remain.
🤖 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/AppDelegateShortcutRoutingTests.swift`:
- Around line 2750-2820: The test
testCmdShiftReturnFocusedBrowserTogglesSplitZoom is making the file exceed the
length budget; refactor by extracting the repeated setup/assertion scaffolding
into shared helpers and shortening the test to a single high-level flow. Create
helpers (e.g., makeMainWindowAndManager(),
openFocusedBrowserPanel(in:manager:preferSplitRight:),
attachWebViewToWindow(browserPanel:window:), and
makeCmdShiftReturnEvent(window:)) and move the window creation,
workspace/browserPanel lookup, webView attach/remove, and event construction
into those helpers; then reduce the test body to withTemporaryShortcut { guard
let appDelegate = AppDelegate.shared else XCTFail...; let (windowId, manager,
browserPanel, event) =
makeMainWindowAndManager()/openFocusedBrowserPanel()/makeCmdShiftReturnEvent();
perform the minimal assertions and invoke
appDelegate.debugHandleShortcutMonitorEvent and
browserPanel.webView.performKeyEquivalent. Also consider removing or
consolidating duplicate DEBUG-only assertions to further shrink the test size.
- Around line 2773-2778: The test currently always removes browserPanel.webView
in defer which can detach a pre-existing view; add a local Bool (e.g.
didAttachForTest) initialized false, set it to true inside the if branch that
adds browserPanel.webView to window.contentView, and change the defer to only
call browserPanel.webView.removeFromSuperview() when didAttachForTest is true;
reference browserPanel.webView, window.contentView?.addSubview(...), and the new
didAttachForTest flag so the cleanup only runs if this test attached the view.
In `@Sources/AppDelegate.swift`:
- Around line 11246-11247: Collapse the two-line sequence that assigns
routedManager and calls toggleFocusedSplitZoom into a single expression to save
a line: inline the optional chaining so you call toggleFocusedSplitZoom() on
either preferredMainWindowContextForShortcutRouting(event: event)?.tabManager or
fallback tabManager in one statement (referencing
preferredMainWindowContextForShortcutRouting(event:), tabManager, and
toggleFocusedSplitZoom()). Ensure the result is discarded as before (using _ =
...) and preserve the same optional semantics.
---
Outside diff comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Line 1: This file exceeds the CI file-length budget because the new
performKeyEquivalent block expanded the file; fix by extracting the top-level
types BrowserImageCopyPasteboardPayload and BrowserImageCopyPasteboardBuilder
into a new Swift file (e.g.,
Sources/Panels/BrowserImageCopyPasteboardBuilder.swift). Move the entire
definitions of BrowserImageCopyPasteboardPayload and
BrowserImageCopyPasteboardBuilder out of CmuxWebView.swift, ensure they keep the
same access levels and any required imports (e.g., AppKit/Foundation), update
any references in CmuxWebView (e.g., where BrowserImageCopyPasteboardBuilder is
instantiated or BrowserImageCopyPasteboardPayload is used) to the new file, and
run the build to confirm no missing symbols remain.
🪄 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: 5c7d8152-0b32-40a3-b70a-4f3a8731dc0a
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftSources/Panels/CmuxWebView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
The PR branch needed the latest main changes before resolving review feedback so CI validates against the current shortcut model and file-length budgets. Constraint: PR target main advanced after the original branch commits Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag issue-3514-cmd-shift-enter-zoom Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv
Review feedback exposed two cleanup issues after the browser pane-zoom fix: Swift file-length budgets were over limit and DEBUG goto-split recording still read the fallback tab manager after routed zooms. The test now lives in the smaller equalize-splits shortcut suite, the browser Return key-equivalent path stays compact, and the debug recorder snapshots the routed manager's workspace. Constraint: Swift file-length budget is enforced by workflow-guard-tests Rejected: Extract BrowserImageCopyPasteboardBuilder | unnecessary after trimming the Return path and moving the regression test Rejected: Inline toggleFocusedSplitZoom without routedManager | would lose the manager needed for correct DEBUG recording Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag issue-3514-cmd-shift-enter-zoom Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check
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 0a0c650. Configure here.
Cursor Bugbot caught that the compact Return/Enter shortcut path could report handled=0 in DEBUG timing logs even when it consumed a browser pane shortcut. Centralizing performKeyEquivalent returns through a local finish helper keeps the timing flag accurate and reduces repeated DEBUG bookkeeping. Constraint: CmuxWebView.swift is governed by the Swift file-length budget Rejected: Re-add per-branch DEBUG blocks | more repetitive and weaker against the file-length budget Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check
This was referenced May 11, 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
1 participant
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
Fixes browser-focused Cmd+Shift+Enter pane zoom by routing Return-bearing browser key equivalents through the central shortcut handler before falling back to WebKit keyDown handling.
Offending commit: 7d59e55
Root cause: commit 7d59e55 widened CmuxWebView Return/Enter routing to unconditionally bypass app/menu key-equivalent handling so WebKit form submission kept working. That bypass meant browser-focused Cmd+Shift+Enter never reached the app shortcut path, and the stored Return binding was also treated as unbound because the shortcut model trimmed newline characters. The fix keeps plain/unclaimed Return events on WebKit keyDown, but lets configured cmux Return shortcuts claim the event first and routes pane zoom through the event-window tab manager.
Tests added:
Verification:
./scripts/test-unit.sh test -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdShiftReturnFocusedBrowserTogglesSplitZoom./scripts/test-unit.sh test -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdShiftReturnFocusedBrowserTogglesSplitZoomCloses #3514
Note
Medium Risk
Changes keyboard shortcut routing for Return/Enter inside
CmuxWebViewand routes split-zoom to a window-specificTabManager, which could affect other browser key-equivalent behaviors and shortcut dispatch across windows.Overview
Fixes pane zoom (
toggleSplitZoom) when a browser pane is focused by routing Return/Enter key equivalents fromCmuxWebView.performKeyEquivalentthrough the central shortcut handler instead of unconditionally bypassing app/menu handling.Updates shortcut dispatch to use the event window’s routed
TabManager(and passes that through DEBUG goto-split recording) so split-zoom toggles the correct workspace. Also correctsStoredShortcut.isUnboundto treat only an empty key as unbound (preserving"\r"Return bindings) and adds a unit test covering Cmd+Shift+Return split zoom from a focused browser web view.Reviewed by Cursor Bugbot for commit a66b9f6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes Cmd+Shift+Enter pane zoom when a browser pane is focused. Return/Enter shortcuts now go through the central handler before falling back to WebKit, and DEBUG shortcut timing stays accurate.
StoredShortcut.isUnbound.CmuxWebView.performKeyEquivalentby centralizing the handled state. Add unit testtestCmdShiftReturnFocusedBrowserTogglesSplitZoom(Equalize Splits suite).Written for commit a66b9f6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests