Repository navigation
Add browser focus mode - #4573
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughImplements a browser focus mode that routes keyboard events to web pages with a two-Escape activation/exit sequence, adds UI controls (toolbar, context menu, View menu, command-palette), updates AppDelegate and CmuxWebView keyboard routing, exposes TabManager facades, includes localization, and adds unit and UI tests. ChangesBrowser Focus Mode Feature
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant BrowserPanel
participant CmuxWebView
User->>AppDelegate: trigger command-palette / menu / context-menu / shortcut
AppDelegate->>BrowserPanel: toggle/set/clear focus mode request
BrowserPanel->>CmuxWebView: focus web view and route key events
User->>CmuxWebView: key equivalents (Cmd+F/P/S, Escape)
CmuxWebView->>AppDelegate: consult handleBrowserFocusModeKeyEvent
AppDelegate->>CmuxWebView: forward or consume per decision
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (13 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 |
Greptile SummaryThis PR implements Browser Focus Mode for cmux: while active, the focused
Confidence Score: 5/5Safe to merge. The keyboard routing changes are well-scoped: the focus-mode bypass only fires when the web view holds first responder, previously-flagged issues are addressed in the current code, and all locale catalog entries cover every supported locale. The global shortcut monitor bypass and the window performKeyEquivalent focus-mode block both gate on isWebViewFocused, preventing app shortcuts from being swallowed after focus moves away. The double-Escape state machine guards event.isARepeat before the fingerprint check. The escape arming uses event-timestamp comparison (not a timer or sleep). The @observable WindowScopedShortcutHintModifierMonitor is stored correctly as @State. All 13 new catalog keys are fully translated across all 20 locales. No new blocking primitives, actor isolation mistakes, or algorithmic regressions were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant GlobalMonitor as Global Shortcut Monitor
participant Window as NSWindow.cmux_performKeyEquivalent
participant WebView as CmuxWebView
participant Panel as BrowserPanel
User->>GlobalMonitor: Key event (any key)
alt Focus mode active + web view focused
GlobalMonitor-->>User: return false (bypass, let AppKit dispatch)
User->>Window: AppKit dispatches to window
Window->>WebView: performKeyEquivalent(event)
WebView->>Panel: handleBrowserFocusModeKeyEvent()
alt Plain Escape (first)
Panel-->>WebView: .forwardToWebView (arm exit)
WebView->>WebView: super.keyDown(Escape)
WebView-->>Window: true
else Plain Escape (second, within 1.6s)
Panel-->>WebView: .consume (clearBrowserFocusMode)
WebView-->>Window: true
else Command key
Panel-->>WebView: .forwardToWebView
WebView->>WebView: super.performKeyEquivalent
WebView-->>Window: true (page gets it, menu never sees it)
end
else Focus mode off
GlobalMonitor->>GlobalMonitor: normal cmux shortcut dispatch
alt toggleBrowserFocusMode shortcut pressed
GlobalMonitor->>Panel: toggleBrowserFocusMode()
Panel-->>User: "isBrowserFocusModeActive = true"
end
end
Reviews (23): Last reviewed commit: "Resolve focus-mode bypass from the respo..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 10665-10766: The new localization keys (browser.focusMode.*,
command.browserFocusMode.*, menu.view.*) only include en and ja entries but must
include every existing catalog locale; add a localization block for each locale
already present in Resources/Localizable.xcstrings for all these new keys, using
the repository fallback convention (copy the English "value" into each missing
locale and set "state": "translated"), and ensure keys like
"browser.focusMode.active", "browser.focusMode.armed",
"browser.focusMode.context.enter", "browser.focusMode.context.exit",
"browser.focusMode.enter.help", "browser.focusMode.exit.help" (and the
corresponding command.browserFocusMode.* and menu.view.* keys referenced in the
comment ranges) receive the complete set of locale entries.
In `@Sources/AppDelegate.swift`:
- Around line 15869-15876: The current branch in AppDelegate is discarding the
Bool from firstResponderWebView.performKeyEquivalent(with: event) and
unconditionally returning true when isBrowserFocusModeActive(for:) is true;
change it to capture the Bool result (e.g., let handled =
firstResponderWebView.performKeyEquivalent(with: event)) and return that value
so CmuxWebView.performKeyEquivalent can veto handling (it returns false for
specific keyCodes like 36/76). Keep the debug log but log the actual result if
desired, then return the captured Bool instead of returning true.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6313-6318: The canEnterBrowserFocusMode getter incorrectly treats
a webView attached to the offscreen backgroundPreloadWindow as eligible; update
the boolean to also verify the webView's window is not the hidden preload host
by adding a check like webView.window !== backgroundPreloadWindow (or an
equivalent isAttachedToPreloadWindow predicate) alongside the existing
webView.window != nil and webView.isHiddenOrHasHiddenAncestor checks so focus
mode only enables for an interactive, non-preload host.
- Around line 6391-6410: handleBrowserFocusModeKeyEvent should first verify the
focus mode is still valid and self-clear it if the interactive web host/webView
has been detached by paths like switchToProfile, reattachToWorkspace, or
replaceWebViewPreservingState; change the initial guard to check the actual
webView/interactive host presence (or other indicator used when entering focus
mode) and, if missing, call clearBrowserFocusMode(reason: "stale") (and
clearBrowserFocusModeExitArm if needed) and return .inactive before proceeding
to use isBrowserFocusModeExitArmed, clearBrowserFocusModeExitArm, or returning
.forwardToWebView so shortcut routing cannot remain stuck in browser focus mode
after the web view was swapped/detached.
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 1566-1583: Duplicate focus-mode items are created because
appendBrowserFocusModeContextMenuItem always appends a new separator and item;
instead detect and reuse an existing item by a stable identifier. In
appendBrowserFocusModeContextMenuItem, search menu.items for an NSMenuItem whose
identifier.rawValue equals a chosen stable id (e.g.
"browserFocusModeContextItem"); if found, update that item's title, state,
target and action (use the existing selector
contextMenuToggleBrowserFocusMode(_:)); if not found, insert a separator only
when needed and create a new NSMenuItem setting its identifier to the stable id,
then add it to the menu. Ensure you don't append additional separators when
reusing the item.
🪄 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: e04f9ce3-70ff-4bba-bf21-380398721431
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/CmuxWebView.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxUITests/MenuKeyEquivalentRoutingUITests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/ContentView.swift
There was a problem hiding this comment.
3 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
cda42fe to
60769d9
Compare
60769d9 to
591919c
Compare
591919c to
1444bfd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Resources/Localizable.xcstrings (1)
10665-10766:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing locale entries for all new focus-mode keys.
All new
browser.focusMode.*,command.browserFocusMode.*, andmenu.view.*keys only includeenandjalocalizations but must include translated entries for every locale already supported by this catalog. As per coding guidelines, "UpdatedResources/*.xcstringskeys must include translated entries for every locale already supported by that string catalog." Based on learnings, use the repository's established fallback convention (English-language fallback values withstate: "translated") for lower-confidence locales.Also applies to: 17580-17613, 50299-50332
🤖 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 `@Resources/Localizable.xcstrings` around lines 10665 - 10766, Several new keys (e.g., "browser.focusMode.active", "browser.focusMode.armed", "browser.focusMode.context.enter", "browser.focusMode.context.exit", "browser.focusMode.enter.help", "browser.focusMode.exit.help" plus the related "command.browserFocusMode.*" and "menu.view.*" keys) only include "en" and "ja" entries; add missing locale entries for every locale already present in this xcstrings catalog, using the repository fallback convention: copy the English value into each missing locale's "value" and set "state": "translated" for those locales so every supported locale has an entry for each of these keys.
🤖 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.
Duplicate comments:
In `@Resources/Localizable.xcstrings`:
- Around line 10665-10766: Several new keys (e.g., "browser.focusMode.active",
"browser.focusMode.armed", "browser.focusMode.context.enter",
"browser.focusMode.context.exit", "browser.focusMode.enter.help",
"browser.focusMode.exit.help" plus the related "command.browserFocusMode.*" and
"menu.view.*" keys) only include "en" and "ja" entries; add missing locale
entries for every locale already present in this xcstrings catalog, using the
repository fallback convention: copy the English value into each missing
locale's "value" and set "state": "translated" for those locales so every
supported locale has an entry for each of these keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9644a668-e6b3-466c-bb76-d44ba5863f85
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/CmuxWebView.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxUITests/MenuKeyEquivalentRoutingUITests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/ContentView.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanel.swift (1)
6427-6447:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTimeout only the exit arm leaves entry armed indefinitely.
After the first plain Escape outside focus mode,
isBrowserFocusModeEnterArmedstays set until a non-Escape key arrives. That means a later unrelated Escape can still activate focus mode, while exit now requires the second Escape within1.6s. This makes the new behavior asymmetric and can unexpectedly steal shortcut routing long after the original key press.Suggested fix
- private static let browserFocusModeExitArmTimeout: TimeInterval = 1.6 - private var browserFocusModeExitArmTimeoutWorkItem: DispatchWorkItem? + private static let browserFocusModeEscapeArmTimeout: TimeInterval = 1.6 + private var browserFocusModeEscapeArmTimeoutWorkItem: DispatchWorkItem? @@ - browserFocusModeExitArmTimeoutWorkItem?.cancel() - browserFocusModeExitArmTimeoutWorkItem = nil + browserFocusModeEscapeArmTimeoutWorkItem?.cancel() + browserFocusModeEscapeArmTimeoutWorkItem = nil @@ - browserFocusModeExitArmTimeoutWorkItem?.cancel() - browserFocusModeExitArmTimeoutWorkItem = nil + browserFocusModeEscapeArmTimeoutWorkItem?.cancel() + browserFocusModeEscapeArmTimeoutWorkItem = nil @@ func clearBrowserFocusModeEnterArm(reason: String) { guard isBrowserFocusModeEnterArmed else { return } + browserFocusModeEscapeArmTimeoutWorkItem?.cancel() + browserFocusModeEscapeArmTimeoutWorkItem = nil isBrowserFocusModeEnterArmed = false `#if` DEBUG cmuxDebugLog("browser.focusMode.escape.enterDisarm panel=\(id.uuidString.prefix(5)) reason=\(reason)") `#endif` } - private func scheduleBrowserFocusModeExitArmTimeout(reason: String) { - browserFocusModeExitArmTimeoutWorkItem?.cancel() + private func scheduleBrowserFocusModeArmTimeout( + reason: String, + clear: `@escaping` `@MainActor` () -> Void + ) { + browserFocusModeEscapeArmTimeoutWorkItem?.cancel() let workItem = DispatchWorkItem { [weak self] in guard let self else { return } - guard self.isBrowserFocusModeExitArmed else { return } - self.clearBrowserFocusModeExitArm(reason: "\(reason).timeout") + clear() } - browserFocusModeExitArmTimeoutWorkItem = workItem + browserFocusModeEscapeArmTimeoutWorkItem = workItem DispatchQueue.main.asyncAfter( - deadline: .now() + Self.browserFocusModeExitArmTimeout, + deadline: .now() + Self.browserFocusModeEscapeArmTimeout, execute: workItem ) } @@ isBrowserFocusModeEnterArmed = true + scheduleBrowserFocusModeArmTimeout(reason: "\(reason).enterArm") { [weak self] in + guard let self, self.isBrowserFocusModeEnterArmed else { return } + self.clearBrowserFocusModeEnterArm(reason: "\(reason).timeout") + } `#if` DEBUG cmuxDebugLog("browser.focusMode.escape.enterArm panel=\(id.uuidString.prefix(5)) reason=\(reason)") `#endif` @@ isBrowserFocusModeExitArmed = true - scheduleBrowserFocusModeExitArmTimeout(reason: reason) + scheduleBrowserFocusModeArmTimeout(reason: "\(reason).exitArm") { [weak self] in + guard let self, self.isBrowserFocusModeExitArmed else { return } + self.clearBrowserFocusModeExitArm(reason: "\(reason).timeout") + } `#if` DEBUG cmuxDebugLog("browser.focusMode.escape.arm panel=\(id.uuidString.prefix(5)) reason=\(reason)") `#endif`Also applies to: 6490-6504
🤖 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/BrowserPanel.swift` around lines 6427 - 6447, Enter arm is never scheduled to time out, so isBrowserFocusModeEnterArmed can remain true indefinitely; implement a symmetric timeout like the exit arm. Add a browserFocusModeEnterArmTimeoutWorkItem property and a private scheduleBrowserFocusModeEnterArmTimeout(reason: String) that mirrors scheduleBrowserFocusModeExitArmTimeout (cancel previous work item, create DispatchWorkItem that calls clearBrowserFocusModeEnterArm(reason: "\(reason).timeout") with [weak self], store it, and dispatch asyncAfter using Self.browserFocusModeEnterArmTimeout), and call this scheduler wherever code sets isBrowserFocusModeEnterArmed = true so clearBrowserFocusModeEnterArm runs automatically after the timeout.
🤖 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.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6427-6447: Enter arm is never scheduled to time out, so
isBrowserFocusModeEnterArmed can remain true indefinitely; implement a symmetric
timeout like the exit arm. Add a browserFocusModeEnterArmTimeoutWorkItem
property and a private scheduleBrowserFocusModeEnterArmTimeout(reason: String)
that mirrors scheduleBrowserFocusModeExitArmTimeout (cancel previous work item,
create DispatchWorkItem that calls clearBrowserFocusModeEnterArm(reason:
"\(reason).timeout") with [weak self], store it, and dispatch asyncAfter using
Self.browserFocusModeEnterArmTimeout), and call this scheduler wherever code
sets isBrowserFocusModeEnterArmed = true so clearBrowserFocusModeEnterArm runs
automatically after the timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3afbc74-7426-4c4b-a27b-f78077f8ca7c
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/CmuxWebView.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
… focus palette.browserFocusMode focused the web view synchronously, then the palette dismiss ran makeFirstResponder(nil) and cleared it, leaving focus mode active but shortcuts not routed to the page. Add it to the dismiss-before-run path so the handler runs after dismissal and its web-view focus sticks (the sibling address-bar command is unaffected because it focuses via an async notification). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
browserFocusModePanelForShortcutEvent bypassed all cmux shortcut handling whenever the selected pane was a focus-mode browser, even after keyboard focus moved elsewhere (e.g. the right sidebar) without changing the pane. In that state cmux shortcuts were swallowed while the web-view double-Escape exit path couldn't run, leaving routing stuck until the user exited via the UI. Require the focus-mode web view to actually own the responder chain (isWebViewFocused) before bypassing, so shortcuts route normally once focus leaves the page. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit gated the focus-mode shortcut bypass on isWebViewFocused, but that helper lived inside a #if DEBUG block (it supported the gotoSplit UI test recorder), so the Release build failed with 'cannot find isWebViewFocused in scope' while the local DEBUG build compiled cleanly. Move the helper out of the DEBUG block so it is defined in all configurations; the DEBUG UI-test callers still reach it via same-file same-type access. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 b8109c5. Configure here.
Switch the default from Ctrl+Cmd+Return to Option+Cmd+Return: Ctrl+Cmd+Return collides with a global hotkey some screen recorders (e.g. Screen Studio) bind, and Option+Cmd is still a modifier tier web pages rarely use. Remains fully customizable via Settings and shortcuts.bindings.toggleBrowserFocusMode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Settings UI (CmuxSettingsUI) builds its rows and conflict checks from the packaged CmuxSettings ShortcutAction enum, so adding toggleBrowserFocusMode only to the app-target KeyboardShortcutSettings.Action left it invisible/uneditable in Settings and excluded from conflict detection. Add the case, Browser group, display name, and the Option+Cmd+Return default stroke to the packaged model so the bridge (by rawValue) shows and edits it correctly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed pane browserFocusModePanelForShortcutEvent resolved the panel via the selected pane (focusedBrowserPanel), but entrypoints like the web-view context menu focus a WKWebView without changing focusedPanelId. The bypass then looked at the wrong panel and kept cmux intercepting Cmd+F/Cmd+N instead of letting the page own them. Resolve via shortcutEventBrowserPanel (the responder-aware resolver every other browser shortcut uses) and keep the isWebViewFocused gate so the bypass still stops when focus leaves the page. Removes the now-unused pane resolver. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Verification
Local xcodebuild tests were not run per repo policy; focused tests will run through GitHub Actions.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches global shortcut routing, first-responder chains, and command-palette focus; mistakes could swallow app shortcuts or leave focus mode stuck, but scope is browser UI with tests and clear exit paths.
Overview
Adds Browser Focus Mode so the focused web page gets first claim on keyboard shortcuts while cmux and app menus step aside.
Enter/exit: Default ⌥⌘↩ (
toggleBrowserFocusModein settings,cmux.json, and docs). Exit uses double Esc (first Esc forwards to the page and arms exit; second exits within a timeout). Toolbar button, View menu, command palette, and web context menu toggle the mode; the palette dismisses before enter so web focus is not cleared.Routing:
BrowserPaneltracks active/armed state and handles Esc sequencing.CmuxWebViewandAppDelegateroute keys to the web view first; the global shortcut monitor bypasses cmux when focus mode is on and the web view is first responder. Opening the command palette or focusing address/find clears the mode.Localization & tests: New
browser.focusMode.*strings; unit/UI tests cover Esc arming, stale arms, and Cmd shortcuts reaching the page vs app menu after exit.Reviewed by Cursor Bugbot for commit aa0bbf1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds Browser Focus Mode so the focused web page gets first claim on ⌘ shortcuts; press Esc twice to exit. The enter shortcut defaults to ⌥⌘↩, is editable in Settings via
CmuxSettingsandcmux.json, with quick toggles in the toolbar, View menu, command palette, and page context menu; fully localized across 20 languages.New Features
CmuxSettings;cmux.schema.jsonvalidatestoggleBrowserFocusMode.browserFocusModeActiveandbrowserFocusModeExitArmed.Bug Fixes
isWebViewFocusedmoved out of DEBUG.Written for commit aa0bbf1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests