Skip to content

Fix workspace number shortcut rebinding - #5616

Merged
austinywang merged 25 commits into
mainfrom
issue-5588-select-workspace-by-number
Jun 30, 2026
Merged

austinywang merged 25 commits into
mainfrom
issue-5588-select-workspace-by-number

Conversation

@austinywang

@austinywang austinywang commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #5588

Summary

  • Add a regression for selectWorkspaceByNumber when workspace digits are rebound to Option+1...9.
  • Let explicit numbered workspace/surface shortcuts run before the printable Option-text bypass.
  • Route numbered workspace/surface actions through the event-scoped main-window TabManager instead of only the cached app-level manager.

Two-commit red to green structure

  1. a0215cf26 adds the failing regression test only. On the pre-fix dispatcher, Option+2 is consumed by the printable Option-text bypass before selectWorkspaceByNumber can run.
  2. fce2dfa60 adds the fix, so the regression should go green.

Repro notes

On current origin/main (1c2083178) on macOS 26.4.1, I could not reproduce the default Cmd+2 failure from the issue: both the DEBUG shortcut injector and a real System Events Cmd+2 switched from workspace:1 to workspace:2, and the terminal did not receive 2. The existing testCmdDigitRoutesToEventWindowWhenActiveManagerIsStale also already covers the default numbered route.

I did reproduce the related route from the issue body:

  • Build/open tagged app with /Users/austinwang/manaflow/cmuxterm-hq/scripts/reload-cloud.sh --tag issue-5588-select-workspace-by-number and http://127.0.0.1:17320/issue-5588-select-workspace-by-number.
  • Create/restore two workspaces, select workspace:1.
  • set_shortcut workspace_digits opt+1, then simulate_shortcut opt+2.
  • Before the fix: socket returned OK, current-workspace stayed workspace:1, and no workspaceDigit action log appeared.
  • After the fix: current-workspace becomes workspace:2 and the debug log shows shortcut.action name=workspaceDigit digit=2 followed by ws.switch.begin.

Verification

  • /Users/austinwang/manaflow/cmuxterm-hq/scripts/reload-cloud.sh --tag issue-5588-select-workspace-by-number
  • Tag opener: http://127.0.0.1:17320/issue-5588-select-workspace-by-number
  • Verified default Cmd+2 switches workspace:1 -> workspace:2 on the tagged socket.
  • Verified rebound Option+2 switches workspace:1 -> workspace:2 on the tagged socket.
  • Did not run the XCUITest suite locally; CI owns that run.

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Fixes workspace and surface number shortcut rebinding so Option+1…9 selects the right target in the focused window instead of typing symbols. Handles Option+digit before the terminal fallback, honors "when" clauses, keeps the WebKit keyDown reentry guard, and avoids work for non-digit keys (closes #5588).

  • Bug Fixes

    • Only bypass printable Option text when no active selectWorkspaceByNumber/selectSurfaceByNumber binding matches; evaluate "when" first.
    • Pre-handle Option+digit in NSWindow.performKeyEquivalent via AppDelegate.handleRoutableNumberedShortcutKeyEquivalent; route through the event’s main-window TabManager (9 = last).
    • Add Swift tests for Option+digit routing, terminal key-equivalent path, and forwarding when the "when" clause is inactive; isolate shortcut settings per test.
  • Refactors

    • Centralize the Option-text bypass in shouldBypassPrintableOptionTextForShortcutRouting; add tabManagerForNumberedShortcut.
    • Add a cheap preflight (eventCouldMatchNumberedShortcutDigit + routableNumberedConfiguredShortcutDigit) to skip settings lookups/dispatcher for non-digits.
    • Remove .claude/scheduled_tasks.lock and ignore it; update the Swift file-length budget.

Written for commit 11cfc2b. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed keyboard shortcut routing for Option+digit workspace and surface selection to prevent interference from printable Option text bypass behavior. Digit-based shortcuts now route correctly regardless of text editing mode.
  • Tests

    • Added comprehensive unit tests validating Option+digit keyboard shortcut routing behavior and precedence across various application states.

@vercel

vercel Bot commented Jun 8, 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 30, 2026 6:26am
cmux-staging Building Building Preview, Comment Jun 30, 2026 6:26am

@coderabbitai

coderabbitai Bot commented Jun 8, 2026 •

Copy link
Copy Markdown

Review Change Stack

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

Printable-Option-text bypass logic in AppDelegate is refactored: two new fileprivate helpers gate the bypass when the event matches a configured digit shortcut and centralize TabManager resolution. Digit-based workspace and surface selection routes through the new helper. Three new integration tests covering routing precedence scenarios are added.

Changes

Option+digit shortcut bypass and routing fix

Layer / File(s) Summary
New bypass and TabManager resolution helpers
Sources/AppDelegate.swift
Adds shouldBypassPrintableOptionTextForShortcutRouting(event:) (wraps prior bypass logic, forces it off when the event matches a digit shortcut) and tabManagerForNumberedShortcut(event:) (resolves TabManager from preferred window context with app-level fallback).
Bypass and digit selection integration
Sources/AppDelegate.swift
handleCustomShortcut delegates to the new bypass helper; workspace and surface digit selection routes through tabManagerForNumberedShortcut(event:) with optional chaining and digit == 9 last-surface behavior preserved; cmux_performKeyEquivalent also prefers the new helper.
Option+digit routing test suite
cmuxTests/AppDelegateOptionDigitShortcutRoutingTests.swift, cmux.xcodeproj/project.pbxproj
Serialized @MainActor test suite with three routing tests (explicit binding wins over bypass; terminal path routes before printable text; inactive when-guarded binding forwards text to focused view). Includes state isolation, NSEvent synthesis, and window management helpers. Xcode project updated to compile the file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • manaflow-ai/cmux#3981: Adds the original printable Option bypass helpers and wires them into handleCustomShortcut/cmux_performKeyEquivalent — this PR directly extends that bypass path with the digit-shortcut exemption.
  • manaflow-ai/cmux#4406: Also modifies the handleCustomShortcut/key-equivalent dispatch path in AppDelegate, overlapping with the same core shortcut-routing code changed here.
  • manaflow-ai/cmux#4442: Adds AppDelegate.debugResetShortcutRoutingStateForTesting() and recorder reset hooks that the new AppDelegateOptionDigitShortcutRoutingTests relies on for state isolation.

Suggested reviewers

  • lawrencecchen

🐇 Hop, hop — the digits now leap,
Past Option-text traps that stole their sleep,
Workspace two answers with a click,
No more silent swallows, no vanishing trick.
The bypass bows when digits are near,
The rabbit routes shortcuts — the path is clear! 🎯

🚥 Pre-merge checks | ✅ 21 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
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.
✅ Passed checks (21 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing the workspace number shortcut rebinding issue when digits are rebound to Option+1...9.
Linked Issues check ✅ Passed The PR successfully addresses issue #5588 by fixing workspace digit shortcut routing, routing through event-scoped TabManager, and conditionally bypassing printable Option text only when no active binding matches.
Out of Scope Changes check ✅ Passed All changes are focused on fixing workspace number shortcut rebinding. Updates to .gitignore and project file are minor supporting changes; the Swift files directly address the routing regression.
Cmux Swift Actor Isolation ✅ Passed Production code changes in AppDelegate.swift add three new helper functions (handleRoutableNumberedShortcutKeyEquivalent, shouldBypassPrintableOptionTextForShortcutRouting, tabManagerForNumberedSho...
Cmux Swift Blocking Runtime ✅ Passed PR introduces no blocking or timing-based synchronization in production Swift code. Three new helpers (shouldBypassPrintableOptionTextForShortcutRouting, tabManagerForNumberedShortcut, handleRoutab...
Cmux Expensive Synchronous Load ✅ Passed PR adds Option+digit shortcut routing via fast UI operations (selectTab/selectSurface), not expensive loaders like RestorableAgentSessionIndex.load(). No synchronous disk/syscall operations on main...
Cmux Cache Substitution Correctness ✅ Passed PR implements "cold-cache fallback plus event-driven cache" pattern: tabManagerForNumberedShortcut tries fresh preferredMainWindowContextForShortcutRouting first, then falls back to self.tabManager...
Cmux No Hacky Sleeps ✅ Passed PR contains only Swift source/test code and configuration files. The custom check targets non-Swift runtime code (TypeScript, JavaScript, shell scripts), which are not modified in this PR.
Cmux Algorithmic Complexity ✅ Passed Production code uses O(1) lookups for numbered shortcuts (2 specific action checks, window context retrieval). Test code iterates over fixed ~181-case enum in setup/teardown only—allowed for test s...
Cmux Swift Concurrency ✅ Passed PR introduces no legacy async patterns. New helper methods use synchronous logic only; test uses RunLoop.main for test synchronization only (allowed per guidelines).
Cmux Swift @Concurrent ✅ Passed All Swift changes in the PR comply with the concurrent annotation rules. The three new synchronous methods (handleRoutableNumberedShortcutKeyEquivalent, shouldBypassPrintableOptionTextForShortcutRo...
Cmux Swift File And Package Boundaries ✅ Passed PR adds only 26 lines to existing 17644-line AppDelegate.swift and introduces a 283-line test file, both within limits. Changes are focused bug fixes (two small helpers) with no new responsibilitie...
Cmux Swiftpm Lockfiles ✅ Passed PR complies with SwiftPM Package.resolved policy: .gitignore does not ignore Package.resolved, root cmux.xcodeproj Package.resolved is committed with 123 additions, and package-local Package.resolv...
Cmux Swift Logging ✅ Passed PR adds no logging violations: new production functions use no print/debugPrint/dump/NSLog; debug-only logs use cmuxDebugLog; test code contains no logging.
Cmux User-Facing Error Privacy ✅ Passed PR contains no user-facing error messages, alerts, or sensitive information exposure; changes are internal shortcut routing logic with test code marked DEBUG-only.
Cmux Full Internationalization ✅ Passed PR adds internal keyboard shortcut routing logic with no user-facing text, DEBUG-guarded test file (allowed per rules), and .gitignore update. No i18n violations.
Cmux Swiftui State Layout ✅ Passed PR contains no SwiftUI state changes; modifications are AppKit/AppDelegate routing helpers and a test suite with NSView helper (no SwiftUI views or state annotations).
Cmux Architecture Rethink ✅ Passed Small correctness fix with clear ownership; uses event-scoped TabManager resolution, respects 'when' clauses, encapsulates bypass logic cleanly, and introduces no timing patterns, mutable state, or...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR contains no production window creation/modification changes. New test file is properly marked DEBUG-only with deferred cleanup of all created test windows.
Cmux Source Artifacts ✅ Passed All changed files are intentional source code, tests, configuration, or proper artifact cleanup: Swift source files, test code, Xcode project config, and .gitignore update to ignore lock files.
Description check ✅ Passed The PR description covers the summary, rationale, and verification steps; only optional template items like demo video, checklist, and review trigger are missing.
✨ 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 issue-5588-select-workspace-by-number

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.

@greptile-apps

greptile-apps Bot commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes Option+digit workspace and surface shortcut rebinding (issue #5588) with a clean two-commit red-to-green structure: first a failing regression test, then the fix that makes it pass.

  • Core fix (shouldBypassPrintableOptionTextForShortcutRouting): wraps the existing shortcutRoutingShouldBypassForPrintableOptionText gate to carve out active selectWorkspaceByNumber / selectSurfaceByNumber bindings, so explicitly bound Option+digit shortcuts are never swallowed by the printable Option-text bypass.
  • NSWindow routing (handleRoutableNumberedShortcutKeyEquivalent): adds a pre-Ghostty entry point in the terminal's performKeyEquivalent branch so Option+digit shortcuts run before the terminal receives the raw key, with a cheap eventCouldMatchNumberedShortcutDigit preflight to avoid settings lookups on non-digit events.
  • Test coverage: three focused tests in cmuxTests/AppDelegateOptionDigitShortcutRoutingTests.swift validate the bypass priority, the NSWindow terminal path, and the inactive-when-clause forwarding case.

Confidence Score: 5/5

Safe to merge; the change is surgical, well-tested, and isolated to the Option+digit shortcut routing path.

Both changed code paths (the local-monitor bypass and the NSWindow terminal key-equivalent path) are narrow and guarded by a cheap digit preflight. The When-clause evaluation is correctly centralised in routableNumberedConfiguredShortcutDigit. Three new regression tests exercise the priority ordering, the NSWindow path, and the inactive-when-clause forwarding case. The .claude/scheduled_tasks.lock artifact is removed and gitignored. No correctness issues were found.

No files require special attention.

Important Files Changed

Filename Overview
Sources/AppDelegate.swift Adds shouldBypassPrintableOptionTextForShortcutRouting, routableNumberedConfiguredShortcutDigit, tabManagerForNumberedShortcut, eventCouldMatchNumberedShortcutDigit, and handleRoutableNumberedShortcutKeyEquivalent; refactors the selectWorkspaceByNumber / selectSurfaceByNumber dispatch and the NSWindow key-equivalent path to honour rebound Option+digit shortcuts before the terminal fast path. Logic looks correct.
cmuxTests/AppDelegateOptionDigitShortcutRoutingTests.swift New test suite with three cases covering the regression (Option+digit beats bypass), the NSWindow terminal path, and the inactive-when-clause forwarding path. All test methods are correctly gated under #if DEBUG and live in the test target, not Sources/.
.claude/scheduled_tasks.lock Local tool artifact removed from source control and added to .gitignore — correct housekeeping.
.gitignore Adds .claude/scheduled_tasks.lock to ignore list to prevent future accidental commits of the lock file.
.github/swift-file-length-budget.tsv Bumps AppDelegate.swift budget by 56 lines to account for the new helpers; minor reordering of other entries to reflect current line counts.
cmux.xcodeproj/project.pbxproj Registers AppDelegateOptionDigitShortcutRoutingTests.swift in both file references and Sources build phases; fixes pre-existing indentation inconsistencies in the file list.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant App as AppDelegate (local monitor)
    participant Win as NSWindow performKeyEquivalent
    participant AD as AppDelegate shortcut dispatch
    participant Term as Ghostty terminal view

    Note over App,Term: Option+digit with selectWorkspaceByNumber rebound to opt+1...9

    App->>AD: handleCustomShortcut(event)
    AD->>AD: shouldBypassPrintableOptionTextForShortcutRouting(event)
    AD->>AD: shortcutRoutingShouldBypassForPrintableOptionText → true
    AD->>AD: "routableNumberedConfiguredShortcutDigit(.selectWorkspaceByNumber) → digit != nil"
    AD-->>AD: "bypass = false (carve-out fires)"
    AD->>AD: routableNumberedConfiguredShortcutDigit → digit confirmed
    AD->>AD: tabManagerForNumberedShortcut → event-window TabManager
    AD->>AD: selectTab(at: digit-1)
    AD-->>App: true (consumed)

    Note over Win,Term: Terminal has first responder — NSWindow path

    Win->>AD: shouldBypassPrintableOptionTextForShortcutRouting → false (same carve-out)
    Win->>Term: (bypass skipped, terminal block entered)
    Win->>AD: handleRoutableNumberedShortcutKeyEquivalent(event)
    AD->>AD: eventCouldMatchNumberedShortcutDigit → true
    AD->>AD: "routableNumberedConfiguredShortcutDigit preflight → != nil"
    AD->>AD: handleCustomShortcut(event) → workspace switched
    AD-->>Win: true
    Win-->>Term: (event not forwarded to terminal)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant App as AppDelegate (local monitor)
    participant Win as NSWindow performKeyEquivalent
    participant AD as AppDelegate shortcut dispatch
    participant Term as Ghostty terminal view

    Note over App,Term: Option+digit with selectWorkspaceByNumber rebound to opt+1...9

    App->>AD: handleCustomShortcut(event)
    AD->>AD: shouldBypassPrintableOptionTextForShortcutRouting(event)
    AD->>AD: shortcutRoutingShouldBypassForPrintableOptionText → true
    AD->>AD: "routableNumberedConfiguredShortcutDigit(.selectWorkspaceByNumber) → digit != nil"
    AD-->>AD: "bypass = false (carve-out fires)"
    AD->>AD: routableNumberedConfiguredShortcutDigit → digit confirmed
    AD->>AD: tabManagerForNumberedShortcut → event-window TabManager
    AD->>AD: selectTab(at: digit-1)
    AD-->>App: true (consumed)

    Note over Win,Term: Terminal has first responder — NSWindow path

    Win->>AD: shouldBypassPrintableOptionTextForShortcutRouting → false (same carve-out)
    Win->>Term: (bypass skipped, terminal block entered)
    Win->>AD: handleRoutableNumberedShortcutKeyEquivalent(event)
    AD->>AD: eventCouldMatchNumberedShortcutDigit → true
    AD->>AD: "routableNumberedConfiguredShortcutDigit preflight → != nil"
    AD->>AD: handleCustomShortcut(event) → workspace switched
    AD-->>Win: true
    Win-->>Term: (event not forwarded to terminal)
Loading

Reviews (23): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread Sources/AppDelegate.swift Outdated
@techyogi

techyogi commented Jun 9, 2026

Copy link
Copy Markdown

I built this and can confirm it works

austinywang and others added 2 commits June 9, 2026 22:55
…-5588-select-workspace-by-number

# Conflicts:
#	Sources/AppDelegate.swift
Resolve AppDelegate.swift conflict in NSWindow.cmux_performKeyEquivalent:
combine this PR's conditional Option-text bypass
(shouldBypassPrintableOptionTextForShortcutRouting, which lets configured
Option+digit workspace/surface shortcuts reach selectWorkspaceByNumber)
with main's browserWebKitKeyDownReentry guard. Keep the reentry local at
outer scope since later branches reference it.

Regenerate .github/swift-file-length-budget.tsv for the merged tree;
swift-warning-budget.tsv already matches main. Take main's ghostty
submodule pointer (branch did not modify ghostty).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second sync after main advanced (1df5237 -> baec525). Sources/AppDelegate.swift auto-merged with this PR's Option+digit shortcut routing intact. Regenerate .github/swift-file-length-budget.tsv for the merged tree; warning budget and ghostty pointer match main.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Regenerate .github/swift-file-length-budget.tsv via swift_file_length_budget.py (budget respected)
- Untrack .claude/scheduled_tasks.lock (local Claude session lock file) and add to .gitignore;
  resolves CodeRabbit source-control-artifacts finding and removes a recurring merge-churn source

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kspace-by-number

# Conflicts:
#	.github/swift-file-length-budget.tsv
#	Sources/AppDelegate.swift
#	cmux.xcodeproj/project.pbxproj
…kspace-by-number

# Conflicts:
#	.github/swift-file-length-budget.tsv
@austinywang
austinywang merged commit 63248b9 into main Jun 30, 2026
30 checks passed
azooz2003-bit added a commit that referenced this pull request Jul 3, 2026
…empty-workspace-group entrypoints (menu/shortcut/handler) reverted by refactor's AppDelegate/cmuxApp winning the merge

This branch was successfully deployed

1 active deployment
Preview – cmux — 11cfc2bd Deployed Jun 30, 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.

selectWorkspaceByNumber: default cmd+1..9 silently doesn't switch workspaces (0.64.13 and 0.64.14)

2 participants