Skip to content

fix(next): menus opened inside a native popup keep the popup open (bookmark bubble's Folder picker) - #28704

Merged
teamleaderleo merged 7 commits into
feat-cmux-nextfrom
fix/popup-host-keeps-child-menus
Nov 1, 2026
Merged

teamleaderleo merged 7 commits into
feat-cmux-nextfrom
fix/popup-host-keeps-child-menus

Conversation

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Bug

The bookmark edit bubble ("Bookmark added" / "Edit bookmark", cmux.bookmarks.edit) closed as soon as its Folder picker opened.

The bubble is a NativePopupHost panel. The Folder picker is a ThemedPicker, a pop-up button whose menu is AppKit's own NSMenu, which runs in a window of its own. When that menu opened it took the keys from the bubble, and the panel's resignKey() called the host's onLostKeys, which treated the key loss as a dismiss. The host's click and Escape monitors had the same gap: they only knew the panel itself and the toggle control, so a menu opened from inside the popup counted as "outside".

Fix

NativePopupHost now treats a menu opened from inside its popup, and a window parented under its panel, as part of the popup:

  • While the popup is shown it watches NSMenu.didBeginTrackingNotification and didEndTrackingNotification. A menu counts as the popup's own when a view in the panel owns it (a pop-up button's menu, a context menu) or when the event that opened it was in the popup. The menu bar's menus never count.
  • While one of those menus is tracking, or a window parented under the panel is key, losing the keys does not dismiss the popup, a click outside does not dismiss it, and Escape closes only the menu.
  • When the last such menu ends, the popup takes the keys back. An ActiveAppKeyPanel still only takes them while the app is active.

So a pick, a click elsewhere or Escape closes only the inner menu, and the bubble stays.

Unchanged:

  • Opening. Menus still open on the click with no fade-in, and the close fade is untouched.
  • The other NativePopupHost callers (Page Info) close themselves before opening children, so they never hit the new path.
  • With no menu of its own open, a popup that loses the keys to another window or to an unrelated menu still closes, as before.

The menu observers are added in watch and removed in unwatch, together with the existing monitors, so they live only while the popup is shown.

Tests

CmuxNextDesignTests/NativePopupHostChildMenuTests builds a visible window with a bubble-shaped keyed popup that holds a ThemedPopUpButton, the control ThemedPicker makes:

  • thePickersMenuTakingTheKeysLeavesTheBubbleOpen: the picker's menu begins tracking and the panel resigns key, then a pick ends the menu. The bubble stays open and onDismiss never fires. It fails before the fix.
  • losingTheKeysToSomethingElseStillClosesTheBubble: an unrelated menu begins tracking and the panel resigns key. The bubble closes once. It passes before and after.

The first commit adds only the tests, so it shows the red; the second commit is the fix.

Risk: low. The change is contained in the shared native popup host, and nothing changes for popups that open no child menu.

🤖 Generated with Claude Code

Agent session: agent session

teamleaderleo and others added 2 commits October 30, 2026 18:09
…(red before the fix)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-f2206f5448805d85f14b
The bookmark edit bubble closed when its Folder picker opened: the picker's
NSMenu took the keys and NativePopupHost read the key loss as a dismiss. The
host now treats menus opened from inside the popup (and windows parented under
its panel) as part of it: key loss, outside clicks and Escape while such a menu
tracks close only the menu, and the popup takes the keys back when it ends.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-f2206f5448805d85f14b
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 5a432fd: 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=pr28704 -f ref=fix/popup-host-keeps-child-menus -f platform=macos -f backend_mode=local

web and size checks: green

Shipped in:

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41255987285 at 97d2420 failed: cmux-next fleet Release compile.
Those jobs last passed at 914d05f. This pull request is one of 5 merged since: #28752, #28728, #28723, #28704, #28691.
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 31, 2026
@blacksmith-sh

blacksmith-sh Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
test_a_picked_signing_label_has_a_rerun_alternative (main.RunsOnFallbackTests.test_
a_picked_signing_label_has_a_rerun_alternative)/
test_a_picked_signing_label_has_a_rerun_alternative (main.RunsOnFallbackTests.test_
a_picked_signing_label_has_a_rerun_alternative)
View Logs
test_no_runs_on_chain_ends_on_the_reserved_side_label (main.RunsOnFallbackTests.tes
t_no_runs_on_chain_ends_on_the_reserved_side_label)/
test_no_runs_on_chain_ends_on_the_reserved_side_label (main.RunsOnFallbackTests.tes
t_no_runs_on_chain_ends_on_the_reserved_side_label)
View Logs

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

Review: the early return for a key loss during a press in the popup kept it open
whenever the press handed the keys to an unrelated window. The host now
re-checks once the press is handled and closes the popup unless a child menu or
window took the keys. Menu tracking is observed synchronously, so the order of
resignKey and didBeginTracking no longer matters. Tests cover both orders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-f2206f5448805d85f14b
NativePopupHost takes the NotificationCenter it observes (default .default, so
the app is unchanged). The child-menu tests hand it a fresh center and post the
menu tracking notices there, so no other observer in the test process gets them
(check-test-notification-centers.sh).

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

cursor Bot commented Oct 31, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

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

SHIP at cb464cd
Visual verdict: no images; Leo tries the build

Static review found no blocking defect. AppKit tests could not run in this Linux box, which has no Swift toolchain.

  1. [P2] [follow-up] Packages/macOS/CmuxNext/Tests/CmuxNextDesignTests/NativePopupHostChildMenuTests.swift:32: The fixture uses show’s default makeKey: false, and tests never assert key restoration. Removing menuEnded’s makeKey() would leave these tests passing. Add coverage starting with a key panel, transferring focus, and verifying that focus returns after the menu closes.

  2. [P2] [follow-up] Packages/macOS/CmuxNext/Sources/CmuxNextDesign/Overlay/NativePopupHost.swift:238: No screenshots were supplied. Validate the native Folder picker interaction on macOS, including selection, outside click, and Escape.

Follow-ups:

  • Add coverage for actual key transfer and restoration.
  • Capture and visually verify the Folder picker interaction on macOS.

@teamleaderleo teamleaderleo removed the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
cmux-bot[bot]
cmux-bot Bot previously approved these changes Oct 31, 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 cb464cd: 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.

@github-actions github-actions Bot added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent October 31, 2026 20:19 Inactive
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent October 31, 2026 20:19 Inactive
@cmux-bot
cmux-bot Bot dismissed their stale review October 31, 2026 20:25

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

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (next-review patch-id; carried): SHIP at edd2696

The independent SHIP at cb464cd carries to this head: the PR's own change (git diff $(git merge-base feat-cmux-next HEAD) HEAD) has the same git patch-id --verbatim (58d87826928ab394) at both heads, and no merge between them was resolved by hand. Nothing new to review. A newer BLOCK, or any change to the PR's own diff, ends the carry.

@teamleaderleo teamleaderleo removed the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent October 31, 2026 22:35 Inactive
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent October 31, 2026 22:35 Inactive
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent October 31, 2026 22:39 Inactive
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (next-review patch-id; carried): SHIP at aeeea58

The independent SHIP at cb464cd carries to this head: the PR's own change (git diff $(git merge-base feat-cmux-next HEAD) HEAD) has the same git patch-id --verbatim (58d87826928ab394) at both heads, and no merge between them was resolved by hand. Nothing new to review. A newer BLOCK, or any change to the PR's own diff, ends the carry.

@teamleaderleo
teamleaderleo merged commit a8502be into feat-cmux-next Nov 1, 2026
158 of 166 checks passed
@teamleaderleo
teamleaderleo deleted the fix/popup-host-keeps-child-menus branch November 1, 2026 00:15
teamleaderleo added a commit that referenced this pull request Nov 1, 2026
…o assumeIsolated as nonisolated(unsafe)

#28704 (a8502be) added NSMenu didBegin/didEndTracking observers that pass
the notification's NSMenu into MainActor.assumeIsolated. Swift 6 refuses it:
NativePopupHost.swift:235/239 'sending menu risks causing data races'. Compile
check app fails at base db63437 in CmuxNextDesign, so nothing downstream
compiles. The menu is read only on its own main-thread delivery, so it is
bound nonisolated(unsafe) as MainActorObservation does for its note.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-53d81bfaf9dd62794c6b
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…without a sending error (a8502be; #28704)

a8502be (#28704) made CmuxNextDesign fail to compile under Swift 6
region checks: "sending 'menu' risks causing data races" at
NativePopupHost.swift:235 and :239 (fleet compile-check, aws-m4pro-8).
The menu is now read from the (Sendable) notification inside the
main-actor closure, after the same main-thread guard, so no
non-Sendable value crosses into it. No behavior change; no new
assumeIsolated or unchecked escape.
lawrencecchen pushed a commit that referenced this pull request Nov 1, 2026
…o assumeIsolated as nonisolated(unsafe)

#28704 (a8502be) added NSMenu didBegin/didEndTracking observers that pass
the notification's NSMenu into MainActor.assumeIsolated. Swift 6 refuses it:
NativePopupHost.swift:235/239 'sending menu risks causing data races'. Compile
check app fails at base db63437 in CmuxNextDesign, so nothing downstream
compiles. The menu is read only on its own main-thread delivery, so it is
bound nonisolated(unsafe) as MainActorObservation does for its note.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-53d81bfaf9dd62794c6b
teamleaderleo added a commit that referenced this pull request Nov 1, 2026
…isolated(unsafe) before assumeIsolated (#29871)

* base red: NativePopupHost's menu-tracking observers send the menu into assumeIsolated as nonisolated(unsafe)

#28704 (a8502be) added NSMenu didBegin/didEndTracking observers that pass
the notification's NSMenu into MainActor.assumeIsolated. Swift 6 refuses it:
NativePopupHost.swift:235/239 'sending menu risks causing data races'. Compile
check app fails at base db63437 in CmuxNextDesign, so nothing downstream
compiles. The menu is read only on its own main-thread delivery, so it is
bound nonisolated(unsafe) as MainActorObservation does for its note.

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

* base red fix: main-proof on the assumeIsolated lines; record the two crash-allow waivers

crash_ratchet reads a main-proof comment only on its own line or the one above.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…without a sending error (a8502be; #28704)

a8502be (#28704) made CmuxNextDesign fail to compile under Swift 6
region checks: "sending 'menu' risks causing data races" at
NativePopupHost.swift:235 and :239 (fleet compile-check on
aws-m4pro-8). The observers must stay queue-less (delivered inside the
press, as the child-menu tests post them), so the menu is handed to
MainActor.assumeIsolated through a reviewed nonisolated(unsafe) local:
NSMenu is a main-thread object and the block returns off the main
thread before it. No behavior change. Two waived unchecked hits are
recorded (crash-safety-baseline.json waived swift.unchecked
CmuxNextDesign 3 -> 5).
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…apter (MenuTracking), so the popup host compiles and adds no assumeIsolated or observer (#29856)

a8502be (#28704, "menus opened inside a native popup keep the popup
open") broke the tip twice:
- compile: NativePopupHost.swift:235:46 and :239:46 "sending 'menu' risks
  causing data races" (the NSMenu read from the notification was sent into
  MainActor.assumeIsolated);
- arch: assume_isolated_outside_adapters +3 and observer_sites +2 (two raw
  NSMenu did{Begin,End}Tracking observers with assumeIsolated in the popup
  host, and a DispatchQueue.main.async + assumeIsolated).

Root fix (PATTERNS.md P2/P3, no escape added):
- MenuTracking (new, CmuxNextCompat) is the one seam for the two NSMenu
  tracking notices: it observes both through MainActorObservation and calls
  its subscribers on the main actor with `.began(menu)` / `.ended(menu)`.
- MainActorObservation gains `atOnce:` (queue nil): the handler runs in the
  poster's own call, as #28704 needs (a press that opens a menu sees it
  before its key-loss check), and only for a post on the main thread. Its
  one reviewed main-delivery proof covers both modes; the NSMenu is read on
  the main actor inside that proof, never sent across isolation.
- NativePopupHost keeps a MenuTracking while it watches (dropped in
  unwatch, as the raw observers were); its deferred dismiss check is a
  `Task { @mainactor }` instead of DispatchQueue.main.async + assumeIsolated.
- The two other NSMenu tracking observers use it too, so those notices are
  observed in one place: ChromiumWarmup's wait for a menu to end (a raw
  observer with assumeIsolated) and TabMenuTracking's end of the strip's
  menu (it filters the ended menu in the subscriber instead of by `object:`).
  Both now hear the end in AppKit's post instead of right after it.
  CmuxNextChromiumWarmup declares its CmuxNextCompat dependency (core).

arch-ratchet against the merge base: assume_isolated_outside_adapters -5,
observer_sites -2, no rise. defaults_guard: exit 0.

Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>

This branch was previously deployed

1 inactive deployment
ci-routing-agent — aeeea587 Deployed Oct 31, 2026 by cmux-bot[bot] via wait for same-key tree publisher #91844
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