Skip to content

Add opt-in NSPopover menu with a persistent SwiftUI hierarchy#1428

Open
Jassy930 wants to merge 33 commits into
steipete:mainfrom
Jassy930:popover-optin
Open

Add opt-in NSPopover menu with a persistent SwiftUI hierarchy#1428
Jassy930 wants to merge 33 commits into
steipete:mainfrom
Jassy930:popover-optin

Conversation

@Jassy930

@Jassy930 Jassy930 commented Jun 11, 2026

Copy link
Copy Markdown

Summary

This PR adds an opt-in NSPopover-based menu that replaces the per-open NSMenu rebuild cycle with a single persistent SwiftUI hierarchy driven by @Observable state. It directly targets the architectural root cause behind the long tail of merged-menu hang reports (#1321 and the follow-up perf PRs): every menu open / provider switch currently rebuilds NSMenuItems, instantiates fresh NSHostingViews, and synchronously measures them on the main thread (layoutSubtreeIfNeeded + fittingSize). The existing caches (width/height/switcher-content) only memoize measurements — the SwiftUI view construction itself is re-done on every interaction.

With the popover implementation, switching providers mutates one @Observable property and SwiftUI diffs the persistent tree.

Off by default. Nothing changes unless the user opts in:

defaults write com.steipete.codexbar usePopoverMenu -bool true

The NSMenu path is untouched when the flag is off (verified by the full test suite). A follow-up branch that removes the legacy NSMenu implementation once this has soaked is ready (popover-cleanup, −15k lines) — happy to open it as a stacked PR if you want to go that way.

What the popover implements (aligned with the NSMenu UI)

  • Provider switcher: vertical icon+title tabs, accent-filled selection, per-provider quota indicator bars, Overview tab, multi-row wrapping, switcherShowsIcons honored
  • Full card flows: Codex stacked accounts (incl. workspace groups + health fallback), token-account stacking, Kilo scopes, storage section, Buy Credits
  • Sectioned card rendering matching addMenuCardSections (usage/credits/extra-usage/compact cost rows with per-section chart drill-downs)
  • Chart drill-downs as cascading popovers anchored to their rows, hover-to-open with NSMenu-style grace-period dismissal (180ms open / 350ms linger), click still works
  • Account switchers (Codex visible accounts / provider token accounts) as segmented SwiftUI controls wired to the existing selection handlers
  • Overview mode reusing OverviewMenuCardRowView, row tap switches provider, chevron opens the per-provider chart
  • Bottom action sections from the same MenuDescriptor.build data source, dispatched through the existing selector(for:) mapping with the active panel's provider context written to lastMenuProvider first; in-flight login disabling via switchAccountSubtitle/codexAddAccountSubtitle
  • Keyboard shortcuts preserved while the panel is open: ⌘R / ⌘, / ⌘Q, ←/→ cycling (incl. Overview), ⌘1–9 provider selection, Esc to close
  • Selection persistence (selectedMenuProvider / mergedMenuLastSelectedWasOverview), open-time stale-provider refresh, storage-footprint refresh, icon animation freeze while the panel is visible
  • Hover highlighting through the same menuItemHighlighted environment the section views already consume; manual accessibility tree (labels/hints incl. shortcut descriptions)
  • Split (non-merged) icon mode: one popover per provider status item with mutual exclusion; menu actions dismiss every active popover

Review follow-ups addressed

Both ClawSweeper findings were real and are fixed in the branch:

  • [P1] Provider context before legacy action dispatchonAction/onBuyCredits now write lastMenuProvider from the active panel's selection (same resolution that feeds MenuDescriptor.build, so menu content and action targets always agree) before invoking the legacy selectors. Dashboard/status/changelog from a Claude panel now route to Claude.
  • [P2] Split-popover dismissalperformMenuAction now closes every popover (merged + per-provider) for non-quit actions, matching NSMenu's dismiss-on-action behavior.
  • Packaging script changes removed from this PR — the two local-dev switches in Scripts/package_app.sh were dropped from the branch; Scripts/ is now untouched (git diff main -- Scripts/ is empty).

Proof (terminal output, flag on, built from this branch)

Scripted run against the freshly built bundle: AppleScript opens the popover, cycles providers with →, and closes with Esc, 4 rounds, while sample profiles the process.

$ defaults read com.steipete.codexbar usePopoverMenu
1

$ defaults read CodexBar.app/Contents/Info.plist CodexGitCommit   # built from PR branch
f3f3b496

# sampling PID 20371 for 14s while AppleScript opens/closes the popover and cycles providers

$ grep -cE "populateMenu|rebuildMenuContent|makeMenuCardItem|measuredHeight|hostedSubviewFittingHeight|updateMenuContentPreservingSwitcher" sample.txt
0

$ grep -oE "(PopoverRootView|PopoverMenuController|_NSPopoverWindow|NSHostingView)[A-Za-z.()]*" sample.txt | sort | uniq -c | sort -rn | head -6
  20 PopoverRootView.swift
  18 NSHostingView.layout()
   4 PopoverRootView.body.getter
   4 PopoverMenuController.swift
   4 NSHostingView.windowDidLayout()
   4 NSHostingView.updateConstraints()

# app still alive after the loop:
PID 20371

Zero NSMenu rebuild/measure frames on the main thread during open/switch loops; the popover code path is what's executing. I can additionally post redacted screenshots (with hidePersonalInfo enabled) on request.

Before / after benchmark (same machine, same build, instrumented)

To quantify the win I temporarily lowered slowMenuOperationThreshold to 0 and printed every logMenuOperationDurationIfSlow sample, then drove the app with AppleScript and sample (instrumentation was on a throwaway branch, not in this PR).

Metric NSMenu (flag off) Popover (flag on)
Main-thread sync cost to build/rebuild menu content (populateMenu) 33.1 ms 0 — no rebuild path exists
populateMenu/makeMenuCardItem/measuredHeight/hostedSubviewFittingHeight frames on the main thread during open + provider-switch loops (sample) present every time content changes 0
Steady-state reopen with no content change 0.1 ms (existing caches already make this fast) lightweight SwiftUI
Resident memory ~107 MB ~105 MB

Honest framing: the NSMenu path is not slow on every open — the existing caches make a steady-state reopen ~0.1 ms. The cost lands on every content change: first open, provider switch, post-refresh redraw all synchronously rebuild NSMenuItems, instantiate fresh NSHostingViews, and measure them on the main thread (≈33 ms each on an idle machine; this scales up under memory pressure or with deep content such as the Zai hourly chart, which is where the recurring "switching is laggy" reports come from). The popover removes the rebuild step entirely — the view tree is built once and switching providers mutates one @Observable property — so that per-change cost drops to 0, as the profiling above shows.

Commands run

  • swift build / swift test — 3493 tests, 404 suites, all passing (flag off and on)
  • make check (SwiftFormat + SwiftLint --strict) — 0 violations
  • Scripts/compile_and_run.sh — bundle-level validation, app stays running
  • Scripted sample profiling (output above)

Notes for review

  • Verified side-by-side against the NSMenu implementation on live data (two instances); the layouts match section-for-section.
  • The commit history is intentionally granular (flag → view model → controller → content parity → charts → split mode → polish → review fixes); each commit builds and passes the suite.
  • New popover code lives in Sources/CodexBar/Popover/ plus StatusItemController+Popover.swift; shared card/chart/descriptor code is reused, not duplicated.
  • Refs can someone fix the performance? #1321.

🤖 Generated with Claude Code

Jassy930 and others added 12 commits June 9, 2026 21:17
Add usePopoverMenu: Bool feature flag to SettingsStore following the
exact usageBarsShowUsed pattern (three-point: state struct, loadDefaultsState,
computed property). Defaults to false; UserDefaults key "usePopoverMenu".
Includes TDD tests in PopoverMenuFeatureFlagTests.swift.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
新增 Sources/CodexBar/Popover/MenuViewModel.swift,作为 popover 重构的
单一可观察状态源;新增配套测试 4 个(全部通过)。不接 UI,不改现有行为。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctive)

阶段 0 骨架:NSPopover(.transient) 承载持久 SwiftUI 根视图;
show/close 同步 MenuViewModel.isVisible;PopoverRootView 占位(310pt 宽 Color.clear)。
测试走真实路径(NSStatusBarButton),未采用 GUI 降级预案。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, inactive)

Adds menuViewModel, popoverMenuController?, and usePopoverMenu computed property
to StatusItemController as gated holding points. No existing logic is touched;
the feature flag defaults to false so behavior is byte-for-byte identical until
Task 1.1 wires the popover activation path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s on (merged mode)

- attachMenus(): when usePopoverMenu is true, set statusItem.menu = nil, lazily
  init PopoverMenuController, and wire button target/action to handleStatusItemClick;
  original NSMenu path is unchanged when flag is off
- openMenuFromShortcut(): merged-mode branch now calls popoverMenuController?.show()
  when usePopoverMenu is on; falls back to performClick when off (identical to before)
- New @objc handleStatusItemClick(_:): guards on usePopoverMenu, refreshes providers,
  calls popoverMenuController?.toggle(relativeTo: button)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PopoverRootView 重写为完整阶段 1 实现:持久视图内嵌 SwiftUI provider
切换器(纯文字按钮,>1 provider 时显示)+ UsageMenuCardView 卡片;
切换 provider 只改 viewModel.select(...),SwiftUI 增量更新不重建视图。
store 注入保证 @observable 观察链,数据变化自动重渲。
attachMenus 构造点用局部常量 + [weak self] 捕获,无强引用环。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Task 1.3:给 PopoverMenuController 加 NSEvent local key monitor,Esc(keyCode 53)
触发 close();show() 时装载 monitor,close() 时卸载,deinit 兜底防泄漏。
暴露 handleKeyDownForTesting 测试接缝,新增 escapeKeyClosesPopover /
nonEscapeKeyNotHandled 两个测试,全套 3411 测试通过。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- MenuViewModel: add selectNext/selectPrevious/selectProvider(atIndex:) navigation helpers with cycleSelection(by:) wrapping through Overview + all providers
- StatusItemMenuProviderNavigationDirection: add Equatable conformance for test assertions
- PopoverMenuController: replace handleKeyDown with unified handle(characters:keyCode:modifiers:) that dispatches character-level shortcuts (Cmd+R/,/Q, Cmd+1-9) and keyCode-level keys (Esc, ← →) via injected callbacks onRefresh/onSettings/onQuit/onNavigate/onSelectIndex
- StatusItemController.attachMenus: wire callbacks on first controller creation with [weak self] to prevent retain cycles
- Tests: 7 MenuViewModelTests + 9 PopoverMenuControllerTests all pass (3420 total, 0 failures)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…set (review fixes)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Task{@mainactor} wrapper deferred handleDidClose() to a later runloop,
so suppressNextToggleOpen was set AFTER the button.action toggle fired —
defeating the double-trigger guard and leaving the popover un-closable by
clicking the status item. NSPopoverDelegate fires on the main thread, so
assumeIsolated runs the cleanup synchronously before the action.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Apply swiftformat to the Phase 0/1 popover files.
- Extract popover attach/wiring from StatusItemController into a new
  StatusItemController+Popover.swift extension so the main class body
  stays under swiftlint's type_body_length limit (and groups popover
  code for upcoming phases).
- Annotate SettingsStore.loadDefaultsState with swiftlint:disable:next
  function_body_length (one-line-over a large flat defaults loader).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed June 13, 2026, 7:25 AM ET / 11:25 UTC.

Summary
The branch adds a default-off usePopoverMenu setting that routes CodexBar menus through a persistent SwiftUI NSPopover stack with provider switching, cards, charts, actions, tests, and migration docs.

Reproducibility: yes. for the review finding from source: current main attaches makeMenu(for: nil) for the no-provider fallback, while the PR constructs a .codex single-provider popover for the same fallback item. I did not run live AppKit validation because the repo policy prefers stable seams unless the AppKit wiring itself must be tested.

Review metrics: 3 noteworthy metrics.

  • Changed files: 30 files, +5215/-8. The PR is a broad menu architecture addition, so file count and line volume are maintainer-relevant beyond CI status.
  • New popover source: 9 added files under Sources/CodexBar/Popover/ plus StatusItemController+Popover.swift. Most merge risk is concentrated in the new parallel menu stack and its controller integration.
  • New focused tests: 9 added popover/menu test files. The branch adds useful seam-level coverage, but it does not cover the no-provider fallback path identified in review.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Fix the no-provider fallback popover path and add focused coverage for that split-mode case.
  • [P2] Add redacted visual proof for provider switching, chart drill-downs, and account switchers if maintainers want higher UI parity confidence.

Mantis proof suggestion
A short desktop recording would materially help maintainers judge UI parity and provider/action behavior in the new popover path. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify the opt-in CodexBar popover opens, switches providers, shows a chart drill-down, and keeps provider-specific actions routed to the visible provider with redacted data.

Risk before merge

  • [P2] In split mode with no enabled provider, opt-in popover users would get a Codex-scoped panel instead of the existing provider-less fallback menu.
  • [P1] The default-off popover is a parallel provider/account/action UI path; provider-routing and UI-parity mistakes can affect opt-in users even though the legacy NSMenu remains the default.
  • [P1] VISION.md treats broad architecture and meaningful maintenance complexity as sign-off items, so maintainers need to decide whether carrying a second menu stack is worth the performance soak.

Maintainer options:

  1. Fix fallback menu parity (recommended)
    Make the popover split-mode fallback status item render provider-less menu content and actions when no provider is enabled, matching makeMenu(for: nil).
  2. Accept a hidden soak after repair
    Maintainers can merge with usePopoverMenu off by default and treat remaining provider/account parity risk as an opt-in soak before any legacy-menu cleanup.
  3. Pause the second menu stack
    Maintainers can pause this PR if the added architecture is more maintenance complexity than the current NSMenu performance fixes justify.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Fix the popover split-mode fallback path so fallbackProvider creates a provider-less panel/menu descriptor matching makeMenu(for: nil), add focused tests for no enabled providers, and do not change the default NSMenu path.

Next step before merge

  • [P2] There is a narrow mechanical repair for the no-provider fallback regression; maintainer architecture sign-off will still be needed after the repair.

Security
Cleared: No concrete security or supply-chain issue found; the diff adds local UI, settings, tests, and docs without dependency, release-script, package, or secret-storage changes.

Review findings

  • [P2] Preserve provider-less fallback menus in popover mode — Sources/CodexBar/StatusItemController+Popover.swift:158
Review details

Best possible solution:

Fix the fallback semantics, then land only after maintainer acceptance of the hidden opt-in soak path while keeping the legacy NSMenu default until provider/account parity is proven enough for a later cleanup PR.

Do we have a high-confidence way to reproduce the issue?

Yes for the review finding from source: current main attaches makeMenu(for: nil) for the no-provider fallback, while the PR constructs a .codex single-provider popover for the same fallback item. I did not run live AppKit validation because the repo policy prefers stable seams unless the AppKit wiring itself must be tested.

Is this the best way to solve the issue?

No, not as-is: the popover path should preserve provider-less fallback semantics before merge. After that narrow repair, the default-off soak strategy remains a maintainer product/maintenance decision.

Full review comments:

  • [P2] Preserve provider-less fallback menus in popover mode — Sources/CodexBar/StatusItemController+Popover.swift:158
    When no provider is enabled, current split NSMenu mode still shows the Codex status item but attaches makeMenu(for: nil), so the user sees the provider-less No usage configured. fallback and no Codex-scoped content. This branch routes the same fallback item through ensureProviderPopover(for: .codex), so makeCardPlan, makeSections, and open refresh all act as if Codex is selected even though it is disabled. Please keep a provider-less fallback popover path for the fallback item.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 69831cad6cac.

Label changes

Label changes:

  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes copied terminal output from a flagged-on bundle showing the popover path executing during scripted open/switch/close loops and the app staying alive.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: This is a normal-priority opt-in performance and UX architecture improvement with limited default blast radius.
  • merge-risk: 🚨 auth-provider: The diff adds a parallel provider/account action path that must preserve provider-specific routing and provider-less fallback behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes copied terminal output from a flagged-on bundle showing the popover path executing during scripted open/switch/close loops and the app staying alive.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied terminal output from a flagged-on bundle showing the popover path executing during scripted open/switch/close loops and the app staying alive.
Evidence reviewed

Acceptance criteria:

  • [P1] swift test --filter PopoverPerProviderTests.
  • [P1] swift test --filter PopoverActionDispatchTests.
  • [P1] make check.

What I checked:

  • Repository policy read and applied: AGENTS.md was read fully; the menu/UI validation, focused-test preference, provider-siloing, and no-Keychain-prompt guidance affected this review. (AGENTS.md:1, 69831cad6cac)
  • Vision sign-off context: VISION.md makes performance improvements merge-by-default unless they add too much complexity, while broad architecture and meaningful maintenance complexity need sign-off. (VISION.md:5, 69831cad6cac)
  • Current main fallback behavior: When no provider can work, current main sets fallbackProvider to Codex for visibility but attaches makeMenu(for: nil), so the menu is provider-less rather than Codex-scoped. (Sources/CodexBar/StatusItemController.swift:772, 69831cad6cac)
  • Provider-less fallback descriptor: The provider-less descriptor path renders No usage configured. when no provider is enabled, which is the fallback menu semantics the popover path should preserve. (Sources/CodexBar/MenuDescriptor.swift:116, 69831cad6cac)
  • PR fallback regression source: The PR's split popover attachment calls ensureProviderPopover(for: provider) for both enabled and fallback items, and that creates a MenuViewModel.singleProvider(provider) instead of a provider-less fallback panel. (Sources/CodexBar/StatusItemController+Popover.swift:158, f3f3b496636f)
  • Previous provider-context finding addressed: The updated head writes lastMenuProvider from the active popover view model before legacy action dispatch and closes merged plus per-provider popovers after non-quit actions. (Sources/CodexBar/StatusItemController+Popover.swift:216, f3f3b496636f)

Likely related people:

  • steipete: Recent current-main history and blame show repeated ownership of the status controller and menu code, including native merged menu positioning and status controller factoring. (role: current main owner; confidence: high; commits: 1dfd7d90f3f1, 2ced0e620ff6, 87e6e26270b2; files: Sources/CodexBar/StatusItemController.swift, Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+Actions.swift)
  • Yuxin-Qiao: Recently worked on merged provider menu positioning and terminal-launch/provider action adjacency in the same menu/status-item surface. (role: recent adjacent owner; confidence: medium; commits: 67a9f43f58bf, 2e42427db242; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+Actions.swift)
  • hhh2210: Recently changed manual refresh feedback in the open menu path, adjacent to this PR's popover refresh and persistent action behavior. (role: recent area contributor; confidence: medium; commits: d1c96c3ac4a6; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+Actions.swift, Sources/CodexBar/StatusItemController.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2007863359

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +283 to +287
private func performLegacyMenuAction(_ action: MenuDescriptor.MenuAction) {
let (sel, payload) = self.selector(for: action)
let item = NSMenuItem()
item.representedObject = payload
_ = self.perform(sel, with: item)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Set the selected provider before dispatching actions

When the popover is showing a provider different from lastMenuProvider (or in split-icon mode where lastMenuProvider may still be nil), provider-dependent actions like Usage Dashboard, Status Page, and Changelog are dispatched without updating that legacy context. MenuDescriptor.MenuAction.dashboard/statusPage/changelog carry no provider payload and the target selectors resolve through lastMenuProvider, so clicking these rows from a Claude/OpenAI/etc. popover can open Codex or the previously opened provider instead of the visible provider. Set lastMenuProvider from the current popover selection/per-provider view model before calling the legacy selector.

Useful? React with 👍 / 👎.

Comment on lines +273 to +275
if action != .quit {
self.popoverMenuController?.close()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Close the active split popover after menu actions

In split-icon popover mode, onAction uses this same performMenuAction, but this only closes the merged popoverMenuController. Since per-provider popovers live in providerPopoverControllers, clicking actions such as Settings, Dashboard, Switch Account, or Refresh from a split provider popover leaves that popover visible instead of dismissing like the NSMenu path; close the active provider popover as well (or call the all-popovers close helper) for non-quit actions.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 11, 2026
Jassy930 and others added 12 commits June 11, 2026 21:05
- 新增 PopoverActionSectionsView(SwiftUI),将 MenuDescriptor.Section 列表
  渲染为 popover 底部动作区,与 NSMenu 路径共用同一数据源。
- 在 StatusItemController+Popover 添加 performMenuAction(_:) 与
  performLegacyMenuAction(_:),通过 selector(for:) 复用 NSMenu 全部分发逻辑;
  有 payload 时构造临时 NSMenuItem 传递 representedObject,无 payload 时直接
  perform(sel),quit 动作不关闭 popover。
- attachMergedPopover() 补充 makeSections/onAction 注入,PopoverRootView 在
  content 区之后渲染动作区并底部 padding 6pt。
- 新增测试 PopoverActionDispatchTests:shortcutLabel 映射 + metaSection 数据源契约。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
新增 PopoverCardPlan 纯数据结构,StatusItemController+Popover 实现
popoverCardPlan(for:) 完整复刻 addMenuCards 分流逻辑(Codex workspace
stacked / Token stacked / Kilo multi-scope / 单卡),含 storageText
与 showBuyCredits;PopoverRootView 改为消费 makeCardPlan 注入渲染计划,
并新增 onBuyCredits 回调。新增 PopoverCardPlanTests 8 个测试。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Task 2.3: 新增 PopoverAccountSwitcherView(纯渲染 SwiftUI Segment 切换器);
在 StatusItemController+Popover 中加入 PopoverAccountSwitcherModel 构造逻辑,
Codex 路径接入 handleCodexVisibleAccountSelectionFromPopover(对应 NSMenu 的
handleCodexVisibleAccountSelection),Token 路径接入
handleTokenAccountSelectionFromPopover(对应 makeTokenAccountSwitcherItem
中的 onSelect 闭包);PopoverRootView 注入 makeAccountSwitcher 参数并在
单 provider 内容分支的卡片上方插入切换器 + Divider。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
实现 Task 2.4:popover 的 .overview 分支从占位升级为完整 Overview 模式。
- StatusItemController+Popover: 新增 PopoverOverviewRow 结构体(id = provider.rawValue)、
  popoverOverviewRows()(与 addOverviewRows 同源:reconcileSelected→compactMap model→过滤 isOverviewErrorOnly→附 storageText)、
  popoverOverviewEmptyText()(两档空态文案与 NSMenu 一致),以及更新 makeSections 闭包(overview 时 includeContextualActions: false,provider 取可用优先逻辑对齐 populateMenu)。
- PopoverRootView: 注入 makeOverviewRows/overviewEmptyText 闭包,实现 overviewContent:
  空态 Text + 非空时 ForEach OverviewMenuCardRowView + 行间 Divider + 点击切 provider。
- Tests/PopoverOverviewTests: 9 个测试(id 稳定性 3、overview↔provider 切换 6),全绿。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- MenuViewModel: add includesOverview flag; navigationStops now
  conditionally includes .overview based on includesOverview
- PopoverRootView: inject switcherIcon closure; render Overview tab
  (square.grid.2x2) when includesOverview; show brand icon + label
  for providers when switcherIcon returns non-nil
- StatusItemController+Popover: add refreshPopoverViewModelInputs()
  to sync providers/includesOverview and correct .overview selection
  when overview is absent; add popoverSwitcherIcon(for:) via
  ProviderBrandIcon (avoids private switcherIcon in +Menu.swift)
- StatusItemController+Actions: use refreshPopoverViewModelInputs()
  in handleStatusItemClick and openMenuFromShortcut
- Tests: update MenuViewModelTests and PopoverOverviewTests to set
  includesOverview=true where navigation through overview is expected;
  add two new tests for includesOverview=false behavior

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add onSelectionChanged callback to MenuViewModel (fires only on actual
change), wire it in attachMergedPopover to write back
mergedMenuLastSelectedWasOverview / selectedMenuProvider to settings,
and update refreshPopoverViewModelInputs to restore the last selection
from settings on every popover open (mirrors resolvedSwitcherSelection
logic from the NSMenu path).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With >4 tabs the single-row HStack overflowed the 310pt popover. Switch to
LazyVGrid(.adaptive(minimum: 64)) so tabs wrap into multiple rows, matching
the NSView switcher's width-driven multi-row behavior. Few tabs keep the
compact single-row layout.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iew fixes)

- C1: stacked 路径(Codex/Token/Kilo)空 cards 时 fallback 到单卡片,通过私有 helper applyStackedFallback 消除重复,与 NSMenu addStackedMenuCards/addStackedCodexMenuCards 语义对齐
- C2: stacked 路径强制 showBuyCredits=false,仅单卡片路径保留 popoverCanShowBuyCredits 计算,与 NSMenu stacked 路径 return false 早退行为一致
- I1: refreshPopoverViewModelInputs 中 includesOverview 增加 enabledProviders.count > 1 条件,单 provider 时不显示切换器
- I3: onSelectionChanged 注册移至 refreshPopoverViewModelInputs 调用之前,确保首次 restore 写回 settings 不丢失
- m2: performLegacyMenuAction 统一构造 NSMenuItem 并 perform(_:with:),消除两路分支与 addCodexAccount ABI 歧义
- m1: PopoverOverviewTests 第一个测试从重言式改为实际构造 PopoverOverviewRow 并断言 row.id == provider.rawValue

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
新增 PopoverChartKind 枚举(6 类图表:usageBreakdown/creditsHistory/costHistory/usageHistory/storageBreakdown/zaiHourly),
popoverChartEntries(for:) 入口清单方法(顺序/去重对齐 NSMenu),popoverOverviewChart(for:model:) overview 图表入口,
以及 popoverChartView(for:width:) 懒构造工厂(数据获取与 +HostedSubmenus.swift append* 方法完全对齐)。
新增 PopoverChartTests:19 项测试全绿,全量 3483 项通过,lint 0 violations。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Task 3.2:在 PopoverRootView 接入六类图表二级 popover 下钻入口。
- 注入 makeChartEntries/makeOverviewChart/makeChartView 三闭包
- @State presentedChart 驱动 .popover(item:) 侧边浮层(整体 bounds 锚点)
- 单 provider 视图:卡片区之后渲染下钻入口行列表(带 chevron.right)
- Overview 行:有下钻时在行尾附加独立 chevron 按钮,行主体 Button 保持切 provider
- onChange selection/isVisible 自动收起子 popover
- attachMergedPopover 补三个 [weak self] 闭包注入

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…der mode

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- attachProviderPopovers mirrors attachMenus(fallback:) traversal: each
  enabled/fallback provider gets its own MenuViewModel.singleProvider +
  PopoverMenuController (no switcher, no overview).
- Extract makePopoverController factory shared by merged & per-provider
  modes (vm parameterized so closures capture their own view model).
- Mutual exclusion: opening one provider's popover closes all others.
- Keyboard shortcut open (openMenuFromShortcut) toggles per-provider
  popover; Cmd+R/,/Q wired per controller; arrows/digits intentionally
  no-op in single-provider panels.
- clearPopoverButtonActions cleans residual button actions when switching
  back to NSMenu mode (both merged and split paths).
- Buy Credits now closes whichever popover is frontmost via
  closeAllProviderPopovers.
- Plan docs for phases 2-4.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Jassy930 and others added 9 commits June 11, 2026 21:05
…led actions, zai details

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add MenuRowHoverHighlight ViewModifier (accent bg + white text, RoundedRect r4, h-padding 5)
- Extract ActionRowButton, OverviewRowView, ChartEntryRowView, BuyCreditsRowView as
  independent Views to hold @State isHovered; disabled rows skip hover
- subtitle/shortcut labels adopt white.opacity(0.75) on hover for legibility
- a11y: root VStack .accessibilityElement(children:.contain) + label "CodexBar menu"
- a11y: switcher container .accessibilityElement(children:.contain)
- a11y: overviewTab .accessibilityLabel("Overview"); providerTab .accessibilityLabel(provider.rawValue)
- a11y: overview row Button label "\(provider) overview" + hint "Show \(provider) details";
  chevron Button label = chart.title
- a11y: PopoverActionSectionsView adds shortcutHint() mapping (refresh/settings/quit)
  applied via .accessibilityHint on ActionRowButton
- a11y: PopoverAccountSwitcherView segment button .accessibilityLabel(title) +
  .accessibilityAddTraits(.isSelected) when selected

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

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…filter, focus ring)

- PopoverActionSectionsView: apply actionable section filter identical to
  NSMenu addActionableSections (only render sections containing action/submenu
  entries, suppressing pure-text usage/account info sections)
- PopoverRootView switcher: rewrite provider tabs to vertical stacked layout
  (icon 18pt above, caption2 title below) with solid accentColor capsule for
  selected state and transparent+secondary for unselected, matching NSView
  ProviderSwitcherView stackedIcons mode
- Overview tab: vertical layout with square.grid.2x2 icon + "Overview" label
- Provider titles: use ProviderDescriptorRegistry.descriptor.metadata.displayName
  (same source as NSView switcherTitle(for:)) instead of rawValue
- Switcher indicator bars: inject switcherIndicator closure in PopoverRootView;
  StatusItemController wires it via switcherWeeklyRemaining + branding.color
  (identical data path to NSView quotaIndicator); 3pt rounded bar, gray track +
  brand-color fill, hidden when tab is selected
- Fixed 3-column LazyVGrid for >3 tabs (vs previous adaptive grid)
- focusEffectDisabled() on all interactive rows to eliminate blue focus rings
  in popover (ActionRowButton, ChartEntryRowView, BuyCreditsRowView,
  OverviewRowView buttons, ProviderSwitcherTabView)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
对齐原版 NSMenu addMenuCardSections 拆段形态:
- PopoverCardPlan 新增 SectionedCard struct,携带各段存在标志与段 chevron 图表字段
- StatusItemController+Popover buildSectionedCard:按 NSMenu openAIWebContext/hasOpenAIAPIUsageSubmenu
  同判定决定是否拆段,各 chart 字段完全照抄 makeUsageSubmenu/addMenuCardSections 条件
- PopoverRootView 新增拆段渲染路径 sectionedCard(_:):Header+Usage/Storage/Credits/ExtraUsage/Cost
  五段顺序与 Divider 规律逐行对齐 Menu.swift:1283-1341
- ChartSectionContainer 可复用段容器:content wrap + 可选 chevron(topTrailing) + hover 高亮 + 点击触发 popover
- PopoverCostCompactRow:title "Cost" + subtitle 详情行,对齐 NSMenuItem title+subtitle 观感
- popoverChartEntries 收缩为仅返回 usageHistory/zaiHourly(原版仅有的两个独立入口行)
- 拆段时 plan.showBuyCredits 置 false,由 sectioned.showBuyCredits 接管避免重复渲染
- 测试:SectionedCard 纯逻辑测试(空 store 无拆段、拆段时顶层 showBuyCredits=false、stacked 不拆段)
  + chartEntries 收缩验证(entries 只含 usageHistory/zaiHourly)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Same mechanism as NSMenu's MenuCardHighlighting: section views consume
@Environment(\.menuItemHighlighted) and switch to highlight colors.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the single global @State presentedChart + root-level .popover(item:)
with per-row @State isPresentingChart + .popover(isPresented:, arrowEdge: .trailing)
on each trigger view (ChartSectionContainer, ChartEntryRowView, OverviewRowView).
Each view now receives makeChartView: (PopoverChartKind, CGFloat) -> AnyView?
injected from PopoverRootView; the two .onChange(of: viewModel.selection/isVisible)
that were manually clearing the global state are removed — SwiftUI diff destroys
local @State automatically when rows are replaced on provider switch, and child
popovers disappear with the host NSPopover on panel close.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
实现 HoverChartPopover ViewModifier,复刻 NSMenu 子菜单 hover 级联行为:
悬停 180ms 自动打开图表 popover,离开后 350ms 宽限(穿越间隙不闪关),
点击立即打开保留。改造 ChartSectionContainer、ChartEntryRowView、
OverviewRowView 三视图移除内联 .popover,统一挂 modifier。

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

Address review findings on steipete#1428:
- Write lastMenuProvider from the active panel's selection before
  dispatching legacy menu actions (dashboard/statusPage/changelog and
  Buy Credits resolve their target provider through that field, exactly
  like NSMenu's menuWillOpen used to set it). The same resolution helper
  now also feeds MenuDescriptor.build so content and action targets agree.
- performMenuAction now closes every popover (merged + per-provider),
  matching NSMenu's dismiss-on-action behavior in split icon mode.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Jassy930

Copy link
Copy Markdown
Author

Thanks for the thorough review — both findings were real bugs, fixed and pushed:

  • [P1] Provider context (a73c7099 → now f3f3b496 after rebase): onAction/onBuyCredits write lastMenuProvider from the active panel's selection before dispatching legacy selectors. The same resolution helper feeds MenuDescriptor.build, so menu content and action targets always agree — dashboard/status/changelog from a Claude panel now route to Claude in both merged and split mode.
  • [P2] Split-popover dismissal: performMenuAction now closes every popover (merged + all per-provider) for non-quit actions.
  • Packaging scripts: the two local-dev switches in Scripts/package_app.sh were dropped from the branch entirely — git diff main -- Scripts/ is now empty.

Real-behavior proof (flag on, scripted open/provider-cycle/close loops, sample profiling, built from this branch's HEAD) is now in the PR body — zero NSMenu rebuild/measure frames on the main thread, popover code path visibly executing, app stays alive. Redacted screenshots available on request.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant