Skip to content

Register animated footer popovers for exclusivity - #13582

Closed
austinywang wants to merge 5 commits into
mainfrom
13308-footer-popover-animated-registration
Closed

austinywang wants to merge 5 commits into
mainfrom
13308-footer-popover-animated-registration

Conversation

@austinywang

@austinywang austinywang commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow up the merged sidebar footer popover exclusivity fix.

The ? help popover opens with AppKit animation. While that animation is in flight, NSPopover.isShown can still be false, so the animated root did not register with the shared footer CmuxPopoverGroup. The account/profile menu and help menu could therefore remain visible together.

The coordinator now registers a popover once immediately after show, regardless of transient isShown state, so account/profile, nested team picker, and help roots all participate in the same exclusive dismissal lifecycle.

Testing

  • Test-only commit fe717cfc49c adds coverage for an immediately opened account root followed by an animated help root.
  • Fix commit 8f9762e8a06 registers animated popovers before the opening transition finishes.
  • Hosted focused CmuxPopoverGroupTests for the final SHA are running.
  • Local non-build checks: Swift file-length budget and git diff --check.

Demo Video

  • Video URL or attachment: pending explicitly authorized tagged build.

Review Trigger

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

Checklist

  • I tested the change locally (build deferred by task instructions)
  • I added or updated tests for behavior changes
  • I requested bot reviews after the latest commit
  • All code review bot comments are resolved
  • All human review comments are resolved

Summary by CodeRabbit

  • New Features

    • Sidebar account, help, and team-picker popovers now share coordinated dismissal behavior.
    • Opening one footer popover automatically closes other open footer popovers.
    • Popovers are registered for dismissal immediately, including while their opening animation is in progress.
  • Bug Fixes

    • Prevented multiple root-level popovers from remaining open simultaneously.
    • Preserved independent popover behavior across separate windows.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@cursor

cursor Bot commented Sep 22, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 83c45052-ce91-49de-b62a-791e55f84c31

📥 Commits

Reviewing files that changed from the base of the PR and between 8f9762e and a8ba06f.

📒 Files selected for processing (5)
  • Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift
  • Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift
  • Sources/ContentView.swift
  • Sources/SidebarAccountTeamPopover.swift
  • cmuxTests/CmuxPopoverGroupTests.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 54c98563-c6ef-478e-89cb-2bd46e592f45

📥 Commits

Reviewing files that changed from the base of the PR and between ef42fc4 and 8f9762e.

📒 Files selected for processing (5)
  • Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift
  • Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift
  • Sources/ContentView.swift
  • Sources/SidebarAccountTeamPopover.swift
  • cmuxTests/CmuxPopoverGroupTests.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The sidebar account and help popovers now share one CmuxPopoverGroup. New root popovers dismiss existing group members. Animated popovers register before their opening transition completes. Tests cover dismissal order, replacement, animation, and group isolation.

Changes

Popover coordination

Layer / File(s) Summary
Root registration dismissal
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift
New root-level registrations dismiss existing group members before registration. Child registrations keep their existing behavior.
Footer popover integration
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift, Sources/ContentView.swift, Sources/SidebarAccountTeamPopover.swift
Account and help popovers receive a shared group. Their actions dismiss grouped popovers before presenting. Animated popovers register immediately after showing.
Popover coordination validation
cmuxTests/CmuxPopoverGroupTests.swift
Tests cover animated registration, dismissal order, root replacement, repeated dismissal, and independent groups.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SidebarButton
  participant CmuxPopoverGroup
  participant ArrowlessPopoverAnchor
  participant AppKitPopover
  SidebarButton->>CmuxPopoverGroup: dismissAll()
  SidebarButton->>ArrowlessPopoverAnchor: present popover
  ArrowlessPopoverAnchor->>AppKitPopover: show
  ArrowlessPopoverAnchor->>CmuxPopoverGroup: register root member
Loading

Suggested reviewers: azooz2003-bit

Merge Risk: ⚪ Minimal · up to 8f976

Account and help footer popovers now coordinate dismissal, including during animated opening, so only the intended menu remains visible. The change is ready to merge.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
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 Cloud Persistent Session And Early Input ✅ Passed PASS: The PR changes only AppKit popover grouping, sidebar footer state, and related tests. The authoritative diff contains no Cloud terminal creation, cmux-tui transport, manual renderer, PTY readine…
Cmux Swift Actor Isolation ✅ Passed PASS — The production diff does not introduce a listed Swift actor-isolation mistake. CmuxPopoverGroup was already @MainActor, and ArrowlessPopoverAnchor.Coordinator was already @MainActor; th…
Cmux Swift Blocking Runtime ✅ Passed PASS: The production diff adds no blocking or timing primitives covered by the rule. It changes popover registration to occur after show based on groupMemberID, and adds group dismissal/state rout…
Cmux Browser Automation Off-Main ✅ Passed PASS. The pull request changes only popover UI/grouping code and popover tests. It does not modify Sources/TerminalController.swift, ControlCommandExecutionPolicy.swift, or any browser socket auto…
Cmux Expensive Synchronous Load ✅ Passed PASS. The production diff only changes popover registration, dismissal, shared group state, and presentation animation. Added-line inspection found no agent-history loader, file read, JSON/JSONL parse…
Cmux Cache Substitution Correctness ✅ Passed The changed production code only updates transient popover UI state and grouped dismissal. It registers animated popovers immediately, shares a CmuxPopoverGroup, and dismisses existing UI members be…
Cmux No Hacky Sleeps ✅ Passed PASS — The authoritative diff changes five .swift files only. The rule applies to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. The patch adds no covered non-Swift sleeps, time…
Cmux Algorithmic Complexity ✅ Passed The production changes operate on CmuxPopoverGroup.members, which is a small fixed menu structure: the shared footer group has account, help, and one nested team-picker popover. The new root registr…
Cmux Swift Concurrency ✅ Passed The diff adds synchronous popover registration and dismissal only. It introduces no DispatchQueue, DispatchGroup, Combine state, completion-handler API, or new fire-and-forget Task. The existing…
Cmux Swift @Concurrent ✅ Passed PASS. The reviewed Swift diff adds no async, nonisolated async, or @concurrent declarations and adds no CPU-, file-, parsing-, or network-heavy async helper call. The changed .task body only p…
Cmux Swift Package Boundaries ✅ Passed The production diff does not violate the package-boundary rule. The popover behavior remains in the existing CmuxAppKitSupportUI SwiftPM target (ArrowlessPopoverAnchor and CmuxPopoverGroup), whe…
Cmux Swiftpm Lockfiles ✅ Passed The authoritative PR diff changes only five Swift source/test files. It contains no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project package-reference changes. Therefore, no Swi…
Cmux Swift Logging ✅ Passed PASS: The pull request adds no production logging. The changed production lines only update popover registration, group state, and dismissal behavior. A targeted diff scan found no added or materially…
Cmux User-Facing Error Privacy ✅ Passed PASS: The production diff changes popover registration and dismissal behavior only. It adds no user-facing errors, alerts, command output, API error bodies, or recovery copy. The new AppKit comment is…
Cmux Full Internationalization ✅ Passed PASS: The production diff changes popover registration, dismissal, and shared group state only. It adds no user-facing text, localization key, catalog entry, web message, metadata, or locale configura…
Cmux Swiftui State Layout ✅ Passed PASS: The diff does not introduce a listed SwiftUI state-layout violation. It adds @State private var footerPopoverGroup = CmuxPopoverGroup(), but CmuxPopoverGroup is an AppKit-owned @MainActor …
Cmux Architecture Rethink ✅ Passed The diff is a small lifecycle correctness fix with clear ownership. ArrowlessPopoverAnchor.Coordinator registers immediately after show, with groupMemberID preventing duplicate registration. `Cm…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The PR changes NSPopover grouping and SwiftUI popover views only. It does not add or materially change a standalone NSWindow, NSPanel, NSWindowController, Window, or WindowGroup. The…
Cmux Source Artifacts ✅ Passed The authoritative diff changes only five existing Swift source/test files: four product source files and cmuxTests/CmuxPopoverGroupTests.swift. The patch contains hand-written popover logic and dura…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The production diff adds popover lifecycle behavior only: immediate group registration, root dismissal, and shared footer-group wiring. It adds no #if DEBUG or test-build guard, no debug…/`……
Title check ✅ Passed The title clearly identifies the main change: registering animated footer popovers to enforce exclusivity.
Description check ✅ Passed The description explains what changed, why it changed, and how the behavior was tested. It includes the required sections and checklist, but the demo video is still pending and some template checklist…
Full details: Docstring Coverage

Explanation

Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 too large.)

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@cursor

cursor Bot commented Sep 23, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@teamleaderleo teamleaderleo added the full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks. label Sep 23, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator

None of the red app-host shards on run 35854660149 (head a8ba06f, full-ci) trace to this diff.

Heads-up: I re-ran this run (attempt 2) and then cancelled it. That run's workflow predates #13923, so it paired the macOS 26 runner with the macOS 15 Xcode pin, and its results aren't meaningful for this PR. Once #14006 greens main's suite, a fresh push or re-label here gets a clean read.

— Camera g1 🛠️

@github-actions

Copy link
Copy Markdown
Contributor

Automatic catch-up: main is green again and this branch needed it.

I tried to catch this branch up with main (e0e635e38fbb), but these files need a person:

  • Sources/SidebarAccountTeamPopover.swift: not a generated file; needs a person

Nothing was pushed. Merge main locally, fix those, and push; /catch-up is there again whenever you want it.

Automatic catch-up will not try this head again; a new push or /catch-up does.
Label the pull request no-auto-catch-up to opt out.

Catch-up run

@github-actions

Copy link
Copy Markdown
Contributor

CI failure attribution

CI failed on a8ba06fb92 (run 35854660149 attempt 3): 1 code, 2 unknown.

Job Verdict Why
macos / swift-package-tests unknown no known signature; failed step: Select helper Xcode
macos / app-host unit tests (3/7) unknown no known signature; failed step: Download compiled app-host test product
macos / app-host unit tests (7/7) code a test failed
Matched log lines
macos / app-host unit tests (7/7): ✘ Test browserEditingShortcutCompletesActiveGlobalSearchChord() recorded an issue at GlobalSearchInputOwnershipTests.swift:295:9: Expectation failed: (GlobalSearchCoordinator.shared → cmux_DEV.GlobalSearchCoordinator).isPaletteVisible()

Not re-run automatically: macos / swift-package-tests, macos / app-host unit tests (3/7), macos / app-host unit tests (7/7) are not machine failures.

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

@teamleaderleo teamleaderleo added area: sidebar The workspace sidebar: list, groups, status, reordering S3: minor Wrong behavior with a workaround difficulty:2 Focused: one package or feature boundary closing-soon Conflicting or red with no activity for 7+ days; closes 2026-10-06 unless the label is removed labels Sep 30, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator

Closing; reopen if you still want it.

auto-merge was automatically disabled October 6, 2026 21:00

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sidebar The workspace sidebar: list, groups, status, reordering closing-soon Conflicting or red with no activity for 7+ days; closes 2026-10-06 unless the label is removed difficulty:2 Focused: one package or feature boundary full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks. S3: minor Wrong behavior with a workaround

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants