Skip to content

Add browser focus mode - #4573

Merged
lawrencecchen merged 32 commits into
mainfrom
feat-browser-focus-mode
Jun 2, 2026
Merged

lawrencecchen merged 32 commits into
mainfrom
feat-browser-focus-mode

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add shared BrowserPanel/TabManager browser focus-mode action with toolbar, context menu, View menu, and command palette entry points
  • route active focus-mode key equivalents to WKWebView before cmux/menu fallback, including double-Escape exit
  • add localization plus unit/UI coverage for focus-mode shortcut routing and exit behavior

Verification

  • ./scripts/setup.sh
  • git diff --check
  • ruby -rjson -e 'JSON.parse(File.read("Resources/Localizable.xcstrings")); puts "Localizable JSON OK"'
  • ./scripts/reload.sh --tag brfocus

Local xcodebuild tests were not run per repo policy; focused tests will run through GitHub Actions.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with 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 ⌥⌘↩ (toggleBrowserFocusMode in 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: BrowserPanel tracks active/armed state and handles Esc sequencing. CmuxWebView and AppDelegate route 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 CmuxSettings and cmux.json, with quick toggles in the toolbar, View menu, command palette, and page context menu; fully localized across 20 languages.

  • New Features

    • Keyboard routing: while active, page gets first pass on shortcuts; unhandled ones don’t hit the app menu. Esc once arms exit, second Esc within timeout exits; repeats are consumed; any non‑Esc disarms.
    • Entry points: toolbar button with “Esc Esc” hint and a tooltip showing the configured enter shortcut; View menu, command palette (runs after dismiss so focus sticks), and web context menu. Shortcut appears in Keyboard Settings via CmuxSettings; cmux.schema.json validates toggleBrowserFocusMode.
    • State/UI: mode auto‑clears on command‑palette open/visible, address/find focus, panel unfocus, and webview reset/discard/removal; command‑palette context and menus update live; diagnostics include browserFocusModeActive and browserFocusModeExitArmed.
  • Bug Fixes

    • Bypass correctness: only applies when the responder is the focus‑mode web view, and resolution now uses the responder web view (not the selected pane). Prevents swallowed app shortcuts and gives context‑menu‑focused pages priority; isWebViewFocused moved out of DEBUG.
    • Routing/escapes: restored modifier‑flag normalization; fixed ⌘↩ not falling through to the app menu; tightened event‑driven Esc arming/timing; fixed the page context‑menu toggle build.
    • Tests: foreground the app so keystrokes reach cmux; added unit/UI tests for routing, the ⌥⌘↩ toggle, and double‑Esc exit.

Written for commit aa0bbf1. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Browser Focus Mode: toolbar button with dynamic shortcut hint, View menu item, command‑palette command, context‑menu toggle, and double‑Escape enter/exit (armed‑state hints); while active, page shortcuts are routed to web content and some keys are consumed.
  • Documentation

    • Added localized strings (English, Japanese, and other locales) and contextual help text for entering/exiting focus mode and shortcut hints.
  • Tests

    • Added unit and UI tests covering keyboard routing, double‑Escape behavior, enter/exit arming, and state transitions.

Review Change Stack

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@vercel

vercel Bot commented May 22, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jun 2, 2026 11:39am
cmux-staging Building Building Preview, Comment Jun 2, 2026 11:39am

@coderabbitai

coderabbitai Bot commented May 22, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Implements 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.

Changes

Browser Focus Mode Feature

Layer / File(s) Summary
BrowserPanel state and control
Sources/Panels/BrowserPanel.swift
Core isBrowserFocusModeActive/armed flags, gating (canEnterBrowserFocusMode/canToggleBrowserFocusMode), toggle/set/clear/disarm methods, two-plain-Escape activation/exit handling, de-dup fingerprinting, and clearing on search/webview discard/unfocus/workspace reset/find/address bar interactions.
Keyboard routing & AppDelegate hooks
Sources/AppDelegate.swift
AppDelegate helpers to query focus-mode state, delegate key-event decisions, deactivate focus mode for command-palette lifecycle, record focus-mode diagnostics, and bypass cmux/menu shortcut fallback when a focus-mode panel is the target.
CmuxWebView routing & context menu
Sources/Panels/CmuxWebView.swift
BrowserFocusModeKeyDecision enum; performKeyEquivalent/keyDown consult AppDelegate decision to consume/forward/default; context menu inserts Enter/Exit toggle item and routes toggle action to AppDelegate.
Command palette & ContentView
Sources/ContentView.swift
Adds panelBrowserFocusModeActive context key, snapshots it for focused browser panels, contributes palette.browserFocusMode command that toggles the focused browser via TabManager.
UI toolbar & View menu
Sources/Panels/BrowserPanelView.swift, Sources/cmuxApp.swift
Toolbar button inserted into address-bar accessory with contextual help/shortcut hint and unfocus clearing; cmuxApp View menu includes Enter/Exit Browser Focus Mode item, disabled when no focused browser can toggle; menu listens for state-change notifications to refresh.
TabManager facade
Sources/TabManager.swift
Exposes canToggleBrowserFocusModeForFocusedBrowser, toggleBrowserFocusModeForFocusedBrowser(reason:), and setFocusedBrowserFocusModeActive(_:reason:), delegating to the focused BrowserPanel and posting Notification.Name.browserFocusModeStateDidChange.
Tests and harness
cmuxTests/AppDelegateShortcutRoutingTests.swift, cmuxUITests/MenuKeyEquivalentRoutingUITests.swift
Unit tests for Escape sequencing and command-equivalent routing; UI test that injects a focus-mode page, verifies Cmd+F/P/S routing while active and double-Escape exit behavior; helpers to create harness and data-URL test page; test key-event helpers updated to accept explicit timestamps.
Localization
Resources/Localizable.xcstrings
Added English and Japanese strings for focus mode active/armed labels, enter/exit context/help texts, command titles (Enter/Exit), and View menu entries.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • manaflow-ai/cmux-dev-artifacts#1752: Overlaps keyboard-focus-mode key routing and escape-state logic touched by AppDelegate/CmuxWebView/BrowserPanel changes.
  • manaflow-ai/cmux-dev-artifacts#1768: Reports a failing UI test for the exact test added here (testBrowserFocusModeRoutesPageShortcutsAndDoubleEscapeExits).
  • manaflow-ai/cmux-dev-artifacts#1754: CI test run for the new MenuKeyEquivalentRoutingUITests exercising browser focus-mode routing and double-Escape behavior.

Possibly related PRs

  • manaflow-ai/cmux#4479: Modifies CmuxWebView context-menu/menu-building code paths that overlap with the new context-menu toggle insertion.

"I’m a rabbit in the code, ears tuned to Esc,
Two taps to enter, two to leave — a nimble press,
Pages keep their shortcuts while menus step aside,
A tiny hop of joy — focus held with pride,
🐇🥕"


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Cmux Swift Blocking Runtime ❌ Error DispatchQueue.main.asyncAfter(1.6s) in BrowserPanel.scheduleBrowserFocusModeExitArmTimeout violates swift-blocking-runtime rule—timeout-based dispatch is flagged as failure by default per rule. Replace 1.6s delayed dispatch with notification or state-based signal to clear exit-armed state, avoiding time-based polling.
Cmux Swiftui State Layout ❌ Error PR adds 3 new @Published properties to existing BrowserPanel ObservableObject class; should use @Observable pattern per modern SwiftUI state layout rules. Convert BrowserPanel from ObservableObject + @Published to @Observable per codebase modern pattern (TaskManagerWindowController, CommandPaletteOverlay, ContentView).
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description is comprehensive and covers the key aspects of the changes, including what was added (Browser Focus Mode), how it works, testing verification, and entry points. However, it lacks some required sections from the template. Add a 'Testing' section detailing manual verification steps and test coverage; include a 'Demo Video' section or explicitly state if one is not applicable; complete the 'Checklist' section to confirm all pre-submission items were completed.
✅ Passed checks (13 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add browser focus mode' directly summarizes the main feature being introduced throughout the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed @Published properties on @MainActor BrowserPanel, methods on @MainActor AppDelegate/TabManager, BrowserFocusModeKeyDecision is simple enum, no shared mutable Sendable types, UI code properly isolated.
Cmux No Hacky Sleeps ✅ Passed PR contains only Swift code and localization strings. The rule explicitly excludes Swift, which is covered by a separate swift-blocking-runtime rule.
Cmux Swift Concurrency ✅ Passed No legacy async patterns introduced. Uses DispatchQueue.main.asyncAfter for timeout (existing pattern) and follows established NotificationCenter.publisher usage. All focus mode APIs are synchronous.
Cmux Swift @Concurrent ✅ Passed PR adds only synchronous browser focus mode functions with no async boundaries, no @concurrent annotations, and no violations of the Swift concurrent annotation rule.
Cmux Swift File And Package Boundaries ✅ Passed Max +216 lines to BrowserPanel (under 250-line threshold). Responsibilities align with existing file purposes. Browser focus mode appropriately in app target due to NSEvent/AppKit/WKWebView coupling.
Cmux Swift Logging ✅ Passed No production logging violations found. All debug logging uses cmuxDebugLog guarded by #if DEBUG, which is compliant with swift-logging.md rules.
Cmux User-Facing Error Privacy ✅ Passed PR adds generic browser focus mode UI text without exposing vendor names, credentials, environment variables, database details, or sensitive data. Failures use silent beep feedback only.
Cmux Full Internationalization ✅ Passed All user-facing browser focus mode Swift text uses String(localized:defaultValue:) with matching translated entries in Resources/Localizable.xcstrings for all 20 supported locales including Khmer.
Cmux Architecture Rethink ✅ Passed Single state owner (BrowserPanel @Published), UI entry points converge to shared actions, synchronous clearing, timeout is intentional design not symptom patch.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds browser focus mode without creating new standalone windows; only modifies existing panels with state, routing, and UI controls.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-browser-focus-mode

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread Sources/AppDelegate.swift Outdated
Comment thread Sources/Panels/CmuxWebView.swift
Comment thread Sources/Panels/BrowserPanel.swift
@greptile-apps

greptile-apps Bot commented May 22, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements Browser Focus Mode for cmux: while active, the focused WKWebView gets first claim on keyboard shortcuts (including Command equivalents) before cmux or the app menu, with a double-Escape exit sequence. Entry points span toolbar, View menu, command palette, and web context menu; exit is handled via a timestamp-based escape-arming state machine.

  • Keyboard routing: A bypass is added to both the global shortcut monitor (returns false so the event falls through to AppKit) and NSWindow.cmux_performKeyEquivalent (routes command equivalents to CmuxWebView first, consuming them for the page); CmuxWebView.performKeyEquivalent and keyDown both call handleBrowserFocusModeKeyEvent for Escape sequencing.
  • State lifecycle: BrowserPanel gains isBrowserFocusModeActive, isBrowserFocusModeExitArmed, and two private timing fields; focus mode is auto-cleared on address-bar focus, find field, panel unfocus, web view discard, and context reset.
  • Localization: All 13 new string keys cover all 20 app locales in Localizable.xcstrings; the web/data/cmux-shortcuts.ts entry follows the existing {en, ja}-only LocalizedText type used by every other shortcut.

Confidence Score: 5/5

Safe 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

Filename Overview
Sources/Panels/BrowserPanel.swift Adds ~200 lines of focus-mode state with lifecycle cleanup in ~8 existing hooks. The double-Escape state machine correctly guards key-repeats before the fingerprint check, and all clear paths are idempotent.
Sources/Panels/CmuxWebView.swift Both performKeyEquivalent and keyDown are wired into handleBrowserFocusModeKeyEvent; the focus-mode block returns result
Sources/AppDelegate.swift Global shortcut monitor bypass and NSWindow.cmux_performKeyEquivalent focus-mode block both correctly return handled (not unconditionally true); focus mode is cleared when the command palette opens via two idempotent call-sites.
Sources/Panels/BrowserPanelView.swift Toolbar button and shortcut-hint pill added; uses @State correctly with @observable WindowScopedShortcutHintModifierMonitor (Swift 5.9 pattern); start/stop lifecycle tied to onAppear/onDisappear.
Sources/cmuxApp.swift View-menu label and disabled state driven by browserFocusModeMenuRevision incremented on .browserFocusModeStateDidChange notifications, correctly propagating nested ObservableObject changes that SwiftUI cannot observe directly.
Sources/ContentView.swift palette.browserFocusMode correctly added to dismissesBeforeExecution set so web-view focus is not cleared by palette teardown before the toggle executes.
Resources/Localizable.xcstrings All 13 new focus-mode string keys cover all 20 app locales; two keys use compact single-line JSON format but locale coverage is complete.
Sources/TabManager.swift Adds three thin delegation methods and the .browserFocusModeStateDidChange notification name.
Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift Adds toggleBrowserFocusMode case; defaultName returns a hardcoded English string consistent with all other existing cases in the file.

Sequence Diagram

sequenceDiagram
    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
Loading

Reviews (23): Last reviewed commit: "Resolve focus-mode bypass from the respo..." | Re-trigger Greptile

Comment thread Sources/cmuxApp.swift Outdated
coderabbitai[bot]
coderabbitai Bot previously requested changes May 22, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 16bcb32 and cda42fe.

📒 Files selected for processing (10)
  • Resources/Localizable.xcstrings
  • Sources/AppDelegate.swift
  • Sources/ContentView.swift
  • Sources/Panels/BrowserPanel.swift
  • Sources/Panels/BrowserPanelView.swift
  • Sources/Panels/CmuxWebView.swift
  • Sources/TabManager.swift
  • Sources/cmuxApp.swift
  • cmuxTests/AppDelegateShortcutRoutingTests.swift
  • cmuxUITests/MenuKeyEquivalentRoutingUITests.swift
👮 Files not reviewed due to content moderation or server errors (1)
  • Sources/ContentView.swift

Comment thread Resources/Localizable.xcstrings Outdated
Comment thread Sources/AppDelegate.swift
Comment thread Sources/Panels/BrowserPanel.swift
Comment thread Sources/Panels/BrowserPanel.swift
Comment thread Sources/Panels/CmuxWebView.swift

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 10 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread Sources/AppDelegate.swift Outdated
Comment thread Sources/Panels/CmuxWebView.swift Outdated
Comment thread Sources/Panels/BrowserPanel.swift
@lawrencecchen
lawrencecchen force-pushed the feat-browser-focus-mode branch from cda42fe to 60769d9 Compare May 22, 2026 10:45
Comment thread Sources/AppDelegate.swift
Comment thread Sources/AppDelegate.swift Outdated
Comment thread Sources/AppDelegate.swift
@lawrencecchen
lawrencecchen force-pushed the feat-browser-focus-mode branch from 591919c to 1444bfd Compare May 22, 2026 12:21
Comment thread Sources/Panels/BrowserPanel.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
Resources/Localizable.xcstrings (1)

10665-10766: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing locale entries for all new focus-mode keys.

All new browser.focusMode.*, command.browserFocusMode.*, and menu.view.* keys only include en and ja localizations but must include translated entries for every locale already supported by this catalog. As per coding guidelines, "Updated Resources/*.xcstrings keys 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 with state: "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

📥 Commits

Reviewing files that changed from the base of the PR and between 591919c and 1444bfd.

📒 Files selected for processing (10)
  • Resources/Localizable.xcstrings
  • Sources/AppDelegate.swift
  • Sources/ContentView.swift
  • Sources/Panels/BrowserPanel.swift
  • Sources/Panels/BrowserPanelView.swift
  • Sources/Panels/CmuxWebView.swift
  • Sources/TabManager.swift
  • Sources/cmuxApp.swift
  • cmuxTests/AppDelegateShortcutRoutingTests.swift
  • cmuxUITests/MenuKeyEquivalentRoutingUITests.swift
👮 Files not reviewed due to content moderation or server errors (1)
  • Sources/ContentView.swift

Comment thread Sources/Panels/BrowserPanel.swift
Comment thread Sources/Panels/CmuxWebView.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Timeout only the exit arm leaves entry armed indefinitely.

After the first plain Escape outside focus mode, isBrowserFocusModeEnterArmed stays 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 within 1.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

📥 Commits

Reviewing files that changed from the base of the PR and between 1444bfd and c8f3cd5.

📒 Files selected for processing (8)
  • Resources/Localizable.xcstrings
  • Sources/AppDelegate.swift
  • Sources/Panels/BrowserPanel.swift
  • Sources/Panels/BrowserPanelView.swift
  • Sources/Panels/CmuxWebView.swift
  • Sources/TabManager.swift
  • Sources/cmuxApp.swift
  • cmuxTests/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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread Sources/Panels/CmuxWebView.swift
lawrencecchen and others added 2 commits June 2, 2026 03:53
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>
@lawrencecchen
lawrencecchen merged commit 7d4b8e4 into main Jun 2, 2026
21 checks passed
@lawrencecchen
lawrencecchen deleted the feat-browser-focus-mode branch June 2, 2026 11:41
@austinywang austinywang mentioned this pull request Jun 4, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — aa0bbf13 Deployed Jun 2, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant