Skip to content

Fix notification jump focus for nested tabs - #6416

Merged
lawrencecchen merged 3 commits into
mainfrom
task-notification-focus-tab
Jun 19, 2026
Merged

lawrencecchen merged 3 commits into
mainfrom
task-notification-focus-tab

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add a regression test for notification focus when the notification target is a Bonsplit nested tab UUID.
  • Normalize notification focus targets in TabManager so panel UUIDs and Bonsplit tab UUIDs both resolve to the intended panel before focusing.

Testing

  • RED before fix: xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-notiftab-test -only-testing:cmuxTests/TabManagerNotificationFocusTests/testFocusTabFromNotificationAcceptsBonsplitSurfaceIdForNestedTabNotification\n- PASS after fix: same focused test\n- PASS: xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-notiftab-test -only-testing:cmuxTests/TabManagerNotificationFocusTests\n- PASS: ./scripts/reload-cloud.sh --tag nftab\n- PASS preflight: launched tagged app, created a workspace with three nested terminal tabs, queued a third-tab notification, jumped from another workspace, verified selectedTabId and selected surface are the third tab, captured screenshot /var/folders/rr/vmfx6xh12dz2tlvgtmyvjmf80000gn/T/cmux-screenshots/nftab-preflight_2026-06-19T02-06-44Z_2A401BF6.png\n\nLocalization\n- No user-facing strings changed.

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


Note

Low Risk
Small, targeted change to notification focus resolution aligned with existing focusTab behavior; test-only addition otherwise.

Overview
Fixes notification jump-to-unread when the target is a nested Bonsplit tab whose UUID is a surface/tab id, not a panel key in tab.panels.

focusTabFromNotification now resolves the optional surfaceId through panelId(forSurfaceOrPanelId:in:) (same normalization as focusTab), treats a provided id as missing only when that lookup fails, and focuses using the resolved panel id instead of passing the raw surface UUID through.

Adds TabManagerNotificationFocusRegressionTests to assert that focusing by a third nested tab’s surface UUID selects the correct panel and Bonsplit selected tab.

Reviewed by Cursor Bugbot for commit 0d35171. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Fixes notification jump focus when a notification targets a nested Bonsplit tab. Surface/tab IDs now resolve to the owning panel before focusing, so the right panel is selected consistently.

  • Bug Fixes
    • Normalize notification focus targets in TabManager by resolving surface IDs to panel IDs before focus.
    • Add a Swift Testing regression test (TabManagerNotificationFocusRegressionTests) to ensure Bonsplit surface tab UUIDs select the correct panel.

Written for commit 0d35171. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved tab focus handling triggered from notifications, enhancing workspace panel identification and navigation.
  • Tests

    • Added regression test coverage for notification-triggered tab focus behavior.

@vercel

vercel Bot commented Jun 19, 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 19, 2026 4:39am

@coderabbitai

coderabbitai Bot commented Jun 19, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

TabManager.focusTabFromNotification is updated to resolve an optional surfaceId to an internal panelId via panelId(forSurfaceOrPanelId:in:), returning false when the mapping fails. A new regression test (TabManagerNotificationFocusRegressionTests) exercises the nested bonsplit surface case and is wired into the Xcode project.

Surface ID → Panel ID Resolution

Layer / File(s) Summary
Surface-to-panel resolution logic
Sources/TabManager.swift
focusTabFromNotification now calls panelId(forSurfaceOrPanelId:in:) to map surfaceId to requestedPanelId, returns false with a missingPanel debug log if mapping fails, and sets desiredPanelId from the resolved id or the tab's existing focusedPanelId.
Regression test and Xcode project wiring
cmuxTests/TabManagerNotificationFocusRegressionTests.swift, cmux.xcodeproj/project.pbxproj
Adds a serialized @MainActor test that creates terminal surfaces in a nested bonsplit pane, drains the main queue, and asserts the focused panel and selected tab match the expected surface UUID. The test file is registered in all four relevant sections of the .pbxproj.

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly Related PRs

  • manaflow-ai/cmux#6222: Introduces the workspace surface-tree mapping (panelId(forSurfaceId:)) that the updated focusTabFromNotification now calls for surface-to-panel resolution.
  • manaflow-ai/cmux#5108: Changes how .ghosttyDidFocusSurface notifications are emitted from Workspace.applyTabSelectionNow, directly coupling to the surfaceId payload that focusTabFromNotification now resolves.
  • manaflow-ai/cmux#4371: Updates focusTab/NotificationDismissalContext in TabManager.swift with related surfaceId-to-panelId mapping logic in the notification-driven focus/dismiss flow.

Poem

🐇 A surface once lost in the panel maze,
now mapped to its home through the right ID haze.
The bonsplit pane holds its surfaces tight,
requestedPanelId brings them to light.
No more missing panels in the fog —
the rabbit resolves them and updates the log! 🗺️

🚥 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 directly and clearly describes the main change: fixing notification focus behavior for nested tabs, which aligns with the primary objective of resolving a bug in focusTabFromNotification to handle Bonsplit surface IDs.
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 Production changes to TabManager.focusTabFromNotification are purely internal implementation updates within the @MainActor class, with no new types or isolation violations. Test file is properly ma...
Cmux Swift Blocking Runtime ✅ Passed Production code introduces no blocking patterns; only uses synchronous function calls to resolve surface IDs. Test code uses allowed deterministic async/await scaffolding (withCheckedContinuation)...
Cmux Expensive Synchronous Load ✅ Passed The change adds lightweight dictionary lookups via panelId(forSurfaceOrPanelId:in:) to normalize notification targets, not expensive file I/O or sysctl operations like RestorableAgentSessionIndex.l...
Cmux Cache Substitution Correctness ✅ Passed No cache substitution correctness issue found. focusTabFromNotification uses panelId(forSurfaceOrPanelId:) which: 1) first checks workspace.panels (fresh read), 2) falls back to cache if needed. Th...
Cmux No Hacky Sleeps ✅ Passed All PR changes are Swift code (TabManager.swift, test file) or project configuration, which are explicitly excluded from this check's scope. The rule covers TypeScript, JavaScript, shell, and non-S...
Cmux Algorithmic Complexity ✅ Passed PR uses O(1) dictionary lookups instead of collection scans; function is called individually, not in loops; aligns with existing focusTab pattern.
Cmux Swift Concurrency ✅ Passed Production code uses modern patterns (synchronous, no legacy async). Test-only DispatchQueue.main.async synchronization is explicitly allowed per concurrency rules for AppKit boundaries.
Cmux Swift @Concurrent ✅ Passed The PR changes comply with swift-concurrent-annotation.md: focusTabFromNotification is synchronous with no @concurrent misuse; the new test's async helpers intentionally inherit @MainActor and coor...
Cmux Swift File And Package Boundaries ✅ Passed This PR satisfies the allowed case for "focused bug fixes that add a small amount of code to a large file": only +4/-6 lines to TabManager.swift (within 6,168 budget), uses existing panelId normali...
Cmux Swiftpm Lockfiles ✅ Passed PR makes no Package.resolved, Package.swift, .gitignore, workflow, or SwiftPM package-reference changes. Only test file addition and logic fix; check not applicable.
Cmux Swift Logging ✅ Passed Production code uses only DEBUG-guarded cmuxDebugLog calls, which comply with swift-logging.md rules. No print/debugPrint/dump/NSLog found in production code or exposed secrets.
Cmux User-Facing Error Privacy ✅ Passed Production changes contain only DEBUG-only log messages with UUID prefixes for debugging; no user-facing errors, alerts, or sensitive information exposed. Test and project file changes are allowed...
Cmux Full Internationalization ✅ Passed PR contains no user-facing text changes: production code changes are debug-only logs wrapped in #if DEBUG guards, test file is exempt from i18n requirements, and no localization keys were modified.
Cmux Swiftui State Layout ✅ Passed PR involves no SwiftUI state changes. TabManager method fix is logic-only, test file is pure Swift unit test with no SwiftUI state patterns, existing ObservableObject touched incidentally only.
Cmux Architecture Rethink ✅ Passed Small correctness fix normalizing notification focus targets through existing panelId(forSurfaceOrPanelId:) method; test-only drainMainQueue() uses allowed synchronization pattern.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR changes don't introduce or modify user-visible NSWindow/NSPanel/NSWindowController or SwiftUI Window/WindowGroup. TabManager method normalizes panel resolution; test file is a test-only fixture...
Cmux Source Artifacts ✅ Passed All three changed files are intentional hand-written source code, configuration, and test files. No local artifacts, generated logs, screenshots, caches, or temp folders detected in any changed paths.
Description check ✅ Passed The pull request description comprehensively covers all required template sections with specific details about changes, testing methodology, and verification results.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 task-notification-focus-tab

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.

@greptile-apps

greptile-apps Bot commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes notification jump-to-unread focus when the notification target is a Bonsplit nested tab surface UUID rather than a panel UUID, by routing focusTabFromNotification through the same panelId(forSurfaceOrPanelId:in:) normalization already used by focusTab.

  • TabManager.swift: Replaces the direct tab.panels[surfaceId] lookup with panelId(forSurfaceOrPanelId:in:), so both panel IDs and Bonsplit surface/tab IDs resolve to the correct panel before focus, split-zoom clear, and notification dismissal.
  • TabManagerNotificationFocusRegressionTests.swift: Adds a regression test that creates three nested terminal tabs, verifies focus starts on the first panel, then asserts focusTabFromNotification with the third tab's surface UUID correctly moves focus to that panel.

Confidence Score: 5/5

Safe to merge — the change is a single-line normalization that replaces a direct dictionary lookup with an already-proven helper used identically in focusTab.

The production diff is four lines, all within focusTabFromNotification. The new resolution path mirrors focusTab's existing call to panelId(forSurfaceOrPanelId:in:), which already handles both panel IDs and Bonsplit surface IDs correctly. Nil-surfaceId fallback and notification dismissal logic are structurally unchanged. The regression test directly covers the previously-broken code path.

No files require special attention.

Important Files Changed

Filename Overview
Sources/TabManager.swift Minimal one-line fix: replaces direct panel-dict lookup with panelId(forSurfaceOrPanelId:in:) normalization, matching the pattern already used in focusTab. Logic for nil-surfaceId fallback and dismissal is unchanged and correct.
cmuxTests/TabManagerNotificationFocusRegressionTests.swift New regression test covering the fixed path; uses @mainactor, deterministic main-queue draining, and the Swift Testing framework consistently with the rest of the test suite.
cmux.xcodeproj/project.pbxproj Correctly adds the new test file reference and build-phase entry to the cmux-unit target; no unrelated changes.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant N as Notification
    participant TM as TabManager
    participant PM as panelId(forSurfaceOrPanelId:in:)
    participant W as Workspace

    N->>TM: focusTabFromNotification(tabId, surfaceId)
    TM->>PM: resolve surfaceId → panelId
    PM-->>TM: requestedPanelId (panel UUID or nil)
    alt surfaceId provided but unresolvable
        TM-->>N: return false (log missingPanel)
    else resolved or no surfaceId
        TM->>W: clearSplitZoom()
        TM->>TM: focusTab(tabId, surfaceId: desiredPanelId)
        TM->>W: dismissNotificationOnDirectInteraction
        TM-->>N: return true
    end
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 N as Notification
    participant TM as TabManager
    participant PM as panelId(forSurfaceOrPanelId:in:)
    participant W as Workspace

    N->>TM: focusTabFromNotification(tabId, surfaceId)
    TM->>PM: resolve surfaceId → panelId
    PM-->>TM: requestedPanelId (panel UUID or nil)
    alt surfaceId provided but unresolvable
        TM-->>N: return false (log missingPanel)
    else resolved or no surfaceId
        TM->>W: clearSplitZoom()
        TM->>TM: focusTab(tabId, surfaceId: desiredPanelId)
        TM->>W: dismissNotificationOnDirectInteraction
        TM-->>N: return true
    end
Loading

Reviews (2): Last reviewed commit: "test: move notification focus regression..." | Re-trigger Greptile

@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 0ec0fae. Configure here.

Comment thread Sources/TabManager.swift
@lawrencecchen
lawrencecchen merged commit df96363 into main Jun 19, 2026
21 checks passed
@lawrencecchen
lawrencecchen deleted the task-notification-focus-tab branch June 19, 2026 04:48
hhsw2015 pushed a commit to hhsw2015/cmux that referenced this pull request Jun 21, 2026
…ettingsCore + wire 110 fork files

Key upstream commits:
- SSH ControlMaster PTY-resize fix (manaflow-ai#6432)
- profiling capture action (manaflow-ai#6433)
- hookless agent forking revert (manaflow-ai#6434)
- notification jump-focus fix for nested tabs (manaflow-ai#6416)
- ~100% CPU re-render loop fix
- right-sidebar custom sidebar tabs (manaflow-ai#6430) — adds SurfaceKind.customSidebar
- terminal scroll-speed multiplier (manaflow-ai#6422)
- Settings recover from offscreen frames (manaflow-ai#5770)

Strategy:
- Default 3-way merge (no -X theirs) — only 15 conflicts vs Frankenstein corruption
  in P58's -X theirs attempt.
- Took fork's TerminalController.swift wholesale (huge fork v2 handler surface).
- Took upstream's pbxproj wholesale — re-added CMUXSessionDaemon + CMUXSettingsCore
  packages to BOTH cmux + cmux-cli targets (P58 only added cmux-cli, leaving
  CmuxSettingsRegistry symbols undefined at link time for the app).
- Wired 110 fork-only Swift files (Sources/Herdr*, StableLayout/, etc.) lost
  when taking upstream's pbxproj.
- Removed dup TerminalController+CustomSidebarCommands.swift (fork TC already
  has all v2CustomSidebar* handlers).
- Removed dup titlebarShortcutHintShouldShow (now in RightSidebarChromeStyle.swift).
- Added SurfaceKind.customSidebar + SessionBlueprintEncoder case for it.
- Made Workspace.isProgrammaticSplit non-private (consumed by Workspace+CustomSidebarPane).

Manual conflict resolutions:
- AppDelegate.shortcut routing: kept fork's selectNext/PreviousTopLevelTab
  but composed with upstream's preferredMainWindowContextForShortcutRouting.
- Workspace.swift session restore: combined fork's Claude restorability filter
  with upstream's Self.resumeBindingForSessionRestore helper.
- WorkspaceContentView canvas mode: nested fork's shouldBypassTopBar branch
  inside the non-canvas arm.

Build green: cmux app + cmuxTests both compile clean.

Co-Authored-By: Claude <noreply@anthropic.com>
@lawrencecchen
lawrencecchen restored the task-notification-focus-tab branch July 18, 2026 10:25

This branch was successfully deployed

1 active deployment
Preview – cmux — 0d351715 Deployed Jun 19, 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