Skip to content

fix(next palette): the focus overlay goes on the window the palette opens over - #26951

Merged
teamleaderleo merged 3 commits into
feat-cmux-nextfrom
palette-focus
Oct 30, 2026
Merged

teamleaderleo merged 3 commits into
feat-cmux-nextfrom
palette-focus

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #4303.

In cmux NIGHTLY, pressing Cmd+P or Cmd+Shift+P opens (or appears to invoke) the surface picker / command palette, but the palette text field never claims keyboard focus. All subsequent keystrokes go to the underlying focused terminal surface — typing the search query writes directly into the shell instead of filtering the palette.

What was wrong

#24814 fixed the responder handoff inside the panel (onBecomeKey → focusField). The remaining hole is one level up: the window under the panel is not reliably told the palette is open.

The palette panel is a child of the window it opens over (PaletteController.present, which resolves relativeTo: ?? NSApp.keyWindow ?? NSApp.mainWindow, then walks up to the top-level parent). Its focus overlay, though, went on windows.active — a different resolution: the window an input event came from, else the key window's owner, else the last active one.

Those disagree whenever the palette opens over a window that is not the active one:

  • an explicit relativeTo: that is not windows.active;
  • a press in one window while another holds the keys — windows.active is the event's window, present parents to the key window;
  • a key window no controller owns, so owner(of:) is nil and active falls back to lastActive.

When they disagree the overlay lands on a window the palette is not over, and the window under the panel keeps an empty overlay stack. That window's FocusCoordinator therefore goes on treating its terminal as the keyboard target and FocusEffectApplier keeps re-seating the terminal surface as its first responder — the "keystrokes leak to the terminal" shape of this bug. It breaks three stated invariants in plans/cmux-next/input-spec.md: W5 ("panel has the keys but the window's overlay stack is empty"), W6, and F1 ("while an overlay is open it is the keyboard target").

W6's check only counts overlays (palettes.count != 1), so an overlay on the wrong window passed it; only W5 catches it, and only in a verification build, and it reports rather than repairs.

The fix

PaletteController.onVisibilityChange now reports the window the panel actually parented to, and AppServices.observePaletteForFocus opens the overlay on that window's controller instead of guessing from windows.active. parentWindow is assigned before the callback so the reported window and the panel's parent cannot drift. The close path is unchanged — it already closes the overlay on whichever window holds it.

Test

PaletteOverlayWindowTests.theOverlayGoesOnTheWindowThePaletteOpensOver opens two windows, points keyWindowSource at the second so windows.active is the one the palette is not over, presents over the first, and asserts the overlay is on the first and not the second. It fails on the parent commit and passes here. observePaletteForFocus had no test at all before this.

Deliberately out of scope

An independent read of the whole key path also found that ActiveAppKeyPanel.canBecomeKey == isAppActive() means a palette opened while the app is inactive (a CLI/MCP open, a CMUX_NEXT_NO_ACTIVATE=1 run) draws without taking the keys. That is the panel's documented contract — it exists so a non-activating panel never captures what the user is typing into their frontmost app — so changing it is a product decision, not a bug fix, and it is not touched here. The Cmd+P path cannot hit it: a key event reaching the app means the app is active.

No UI change, so no before/after capture applies.

Risk

Low and contained: one callback signature with a single consumer, and one window lookup. The only behaviour change is which window's overlay stack records an open palette, and only in the cases where the two resolutions disagreed.

Agent session: agent session

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes the command palette focus leak: when the palette opened over a window that wasn't windows.active, its focus overlay went on the wrong window, leaving the window under the panel with an empty overlay stack so keystrokes went to the terminal instead of the palette.

PaletteController now reports the window the panel actually parents to, and observePaletteForFocus opens the overlay on that window's controller instead of guessing from windows.active. Adds a test (PaletteOverlayWindowTests) that fails on the old behavior and passes here.

No UI change; only the overlay placement logic is affected.

Written for commit 6c26512. Summary will update on new commits.

View guided diff Turn on auto-fix

No capture: the change only moves the palette's focus overlay onto the window the palette opens over; nothing drawn changes.

…pens over

The palette panel is a child of the window it opens over, but its focus
overlay went on `windows.active`: the window an input event came from, else
the key window's, else the last active one. Those disagree with the panel's
parent whenever the palette opens over a window that is not the active one
(an explicit `relativeTo:`, or a press in one window while another holds the
keys). The overlay then landed on a window the palette was not over, and the
window under the panel kept an empty overlay stack, so it went on treating
its terminal as the keyboard target (input-spec.md W5, W6, F1).

The palette now reports the window it parented to, and the app opens the
overlay on that window's controller.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-c8241b2aa98359e57bc6
@teamleaderleo

teamleaderleo commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Owned by an agent session · session

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Bead cx-zwxhpn created for this PR by the cmux coordinator.

@teamleaderleo

teamleaderleo commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Independent review at this exact head: SHIP

Two independent Codex gpt-6-astra reviewers at model_reasoning_effort=high in read-only sandboxes reviewed 3a7653b054ed0407e8fb0b62ad7e79b5229643c8..ec6d8cec5406c36252e439f782a8910b7a25f658, which is this PR's head. Both returned SHIP; the second reported no actionable findings. The first was given the full diff plus the unchanged surrounding code and seven adversarial questions, and checked the claims against the tree rather than taking them from the prompt.

SHIP. Its findings:

  1. The divergence is reachable. WindowManager.active prefers the event-origin controller (WindowManager.swift:130-144) while PaletteController.present defaults to NSApp.keyWindow (PaletteController.swift:250-264). It found a concrete caller I had not named: CmuxPicker.open(_:over:) (CmuxPicker.swift:81-85) can retain window A while window B becomes active, so the old code overlaid B while the panel was parented to A. Activation races reach the same state.
  2. The reordering is safe. parentWindow is available before the synchronous callback (PaletteController.swift:274-278) and the callback still precedes makeKeyAndOrderFront (:280-286), so B7 still holds. The focus observer is the only consumer.
  3. No new leak or double-close. hide clears isVisible before the callback or the re-key (:330-345), so a reentrant resign cannot close twice; the close sweep covers every registered controller, and a window already removed by WindowManager.windowWillClose (WindowManager.swift:415-423) has no live coordinator to leak.
  4. The owner(of:) nil fallback is the right conservative choice. An unowned settings/debug window has no FocusCoordinator (CrossWindowInputGuard.swift:10-12), so there is no correct stack; returning no overlay could let fallback key handling reach a terminal, and the close sweep clears the conservative marker. It suggests a dedicated unowned-window test as a follow-up.
  5. The new test genuinely fails at base and passes at head. At base the injected keyWindowSource makes windows.active === elsewhere, so the old callback marks elsewhere and the first assertion fails; at head the explicit overWindow bypasses NSApp.keyWindow and owner(of:) resolves over. It confirmed the test does not depend on NSApp.keyWindow being nil in a headless run, and that ordersWindowsIn = false plus the injected closure make the precondition deterministic.
  6. No Swift 6 concurrency problem. UI targets use default MainActor isolation (Package.swift:140-148); the closure is neither @Sendable nor crossing an actor boundary, so NSWindow in the signature is fine.
  7. The out-of-scope reasoning holds. ActiveAppKeyPanel.canBecomeKey intentionally requires an active app (ActiveAppKeyPanel.swift:15-35), and a real Cmd+P reaching CmuxApplication implies the app is active, so the inactive-CLI case is defined non-key behaviour, not this bug.

Checks at this head

  • Linux Swift compile check: passed, compile check CmuxNext: passed (295 s).
  • scripts/cmux-next/prepush-gate.py: no new violations (3.2 s). Two app-size warnings only (warn, never refuse): CmuxNextApp grows one source line.

The only follow-up raised is item 4's unowned-window test, which covers pre-existing fallback behaviour this PR preserves unchanged, so it is not a blocker here.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review ec6d8ce: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr26951 -f ref=palette-focus -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 13531e3 to compare; do not merge until green; see lane-rules

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41004307929 at 8e7537c failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 7f5968e. This pull request is one of 3 merged since: #24366, #27772, #26951.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions github-actions Bot added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 28, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6-astra high, next-review): SHIP at ec6d8ce

No blocking findings.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Orphaned: no activity for 14 h, and no lane holds a lease on it. Adopted by the orphan sweep: merged feat-cmux-next in at 978a05e. A later pass queues it for the merge hub if it is reviewed, small and red only on base reds.

@blacksmith-sh

blacksmith-sh Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Found 1 test failure on Blacksmith runners:

Failure

Test View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6-astra high, next-review): SHIP at 978a05e

No blocking findings.

cmux-bot[bot]
cmux-bot Bot previously approved these changes Oct 29, 2026

@cmux-bot cmux-bot 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.

cmux-bot approves 978a05e: an independent review passed this exact head (next-review receipt by teamleaderleo). A push needs a new review; a later BLOCK at this head dismisses this approval.

@cmux-bot
cmux-bot Bot dismissed their stale review October 29, 2026 23:00

A newer verdict blocks 6c26512 (no independent SHIP at the head).

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6-astra high, next-review): SHIP at 6c26512

No blocking findings. The callback reports the resolved parent before key-window changes, and the regression test checks overlay placement and cleanup.

Reviewed the supplied diff and files; tests were not run.

@cmux-bot cmux-bot 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.

cmux-bot approves 6c26512: an independent review passed this exact head (next-review receipt by teamleaderleo). A push needs a new review; a later BLOCK at this head dismisses this approval.

@teamleaderleo
teamleaderleo merged commit 8e7537c into feat-cmux-next Oct 30, 2026
170 of 182 checks passed
@teamleaderleo
teamleaderleo deleted the palette-focus branch October 30, 2026 04:41
@github-actions

Copy link
Copy Markdown
Contributor

Shipped in:

@github-actions

Copy link
Copy Markdown
Contributor

Shipped in:

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

lawrencecchen added a commit that referenced this pull request Oct 30, 2026
The slice head 2b2fc8e (= 43037147a0b's tree: slice + trunk sync c09bb74) merges the feat tip, 130 new
commits. 58 files conflicted:
- 0 reuse the previous round's resolutions: this round's base is the previous feat tip a14725a, so no hunk had
  all three sides equal to 5f39f9e's inputs.
- Regenerated, not hand-merged: the 4 CmuxMessagesLab patches (Fixture, Layout, RowDrawing, WindowView) from the
  merged vendored sources against the pinned upstream reconstructed from the side whose patch was consistent (feat's
  for 3, perf's for WindowView; feat 65de604 edited WindowView.swift without rewriting its patch).
- 54 resolved by porting feat's change into perf's shape:
  - Package.swift (CmuxNext: Floating tests get CmuxNextDesign with perf's Wakeups deps; CmuxMobileKit: Model gets
    feat's Resources with perf's CmuxSafeConcurrency).
  - Lineage parents #24136: SidebarSessionTreeFeed (recordedParents and per-host links on perf's ServiceObservation),
    RemoteAcpmuxWire (host/linksAlias notes posted on perf's mainWork; test-only inits take them too),
    AgentTabRemoteHost, AgentTabs+Wiring, AgentSessionWorkspace (the record is one completion task on main), tests.
  - Artifact sandbox #26216: perf's page-host PageArtifactFrame already serves the sandboxed endpoint; feat's
    navigation tests ported to PageNavigation, frame-ancestors expectation added, artifactFrameURL keeps only the
    page host, webviews tests on perf's URLs.
  - pane.commandAvailability (#25405) and chief.face (#27914) beside perf's keyToPaint and Wiring init.
  - State files off main #27749: AgentUnsentDrafts (perf's ObservedMap) and ScrollAnchors (blockingDecode) on
    AgentPaneStateFile.
  - AgentPaneTransport.close: perf's close reason plus feat's host-request failure and trust reset.
  - CloudService: perf's file, plus #27104 signedOutCreation (scope, policy, stop, signOut in +Machines) and #26740
    autosize/sizePick; listIsCurrent moved to +AppServer as feat did.
  - debug.object_menu into perf's DebugDriverControl; Home fixture presence/faces beside perf's field.
  - Palette overlay window #26951 into perf's paletteBuilt.
  - Per-window Home #27880 in perf's dockTab helper; active team (1bb1b96) on perf's cloudAuth.
  - Busy refusal (a930474, 2a9b813) in perf's LineTransport split (+State, +Route, framed write).
  - Launch snapshot sidebar doc (dc38061): DaemonLaunchSnapshot.sidebarDoc, shown once settled
    (whenSettled), its connect follow a ServiceObservation; the Rust test moved to launch_snapshot_tests.rs.
  - Hover card corners (bc66475) with perf's interactive flag; Chief pet state (#25671) on perf's
    ServiceObservation and AppearanceSignals; Chief picture crop (#27933) on perf's lanes and optional store;
    Home presence (#25578) as a FrameObservation; sidebar tab look (#27530) via SidebarLookFollower; settleOpenings
    (#24374) with perf's snap; Omnibar imports.
  - acpmux: inbox digest senders without slicing; withdraw_queued async with from_agents_only; cmux-tui-core and
    platform: connection leases beside perf's modules and InlineSocket (bounded_outbound.rs takes back the
    `platform::transport` import perf had dropped, which feat's claim_watched_connection names).
  - webviews: App/NewTab/NewTabScreen (retry, slash commands, at), settings keys moved to cardRows.ts.
- Feat's tests that call perf-changed inits fixed: TabConcentricCornerTests (input:), HomeChatPresenceFacesTests
  (deadlines:, frame delivery), RemoteChildSessionsTests (main run loop for posted notes).

Generators: cargo metadata in cmux-tui, cmux-tui/fuzz, Native/DiffSidecar and optchat-chief changed no lock;
gen-features.py --check, gen-chromium-host-wire.py --check and --check-ops, sync-resource-api.py --check pass.
webviews: bun run check passes. schemas/settings/settings-schema.json stays perf's (generated; the autofix
regenerates it): until then feat's chiefLook.test.tsx misses the home.bubbleColors rows.

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 6c265126 Deployed Oct 29, 2026 by teamleaderleo via wait for same-key tree publisher #78774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant