Recycle menu card hosting views and reconcile menu content in place - #1394
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2cc7da99
ℹ️ 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".
| guard let id = item.representedObject as? String else { continue } | ||
| guard let view = item.view, view is any MenuCardMeasuring else { continue } | ||
| guard self.menuCardViewRecyclePool[id] == nil else { continue } | ||
| item.view = nil |
There was a problem hiding this comment.
When an open menu is rebuilt while a card is highlighted, this detaches the view from the NSMenuItem that highlightedMenuItems still stores. If AppKit subsequently sends menu(_:willHighlight:) with nil or a different rebuilt item, the old item has no view, so the existing code cannot call setHighlighted(false) and the recycled hosting view can remain visibly highlighted on the wrong/no row. Clear the highlight or update highlightedMenuItems before moving the view into the recycle pool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed real: both unhighlight paths (menu(_:willHighlight:) and the menuDidClose cleanup) unwind through the tracked item's view, which harvesting nils out, so a card highlighted at rebuild time would re-attach with stale highlight rendering and no path to clear it. On current main this can't surface because the discarded view dies with the item; with recycling the state survives into the visible menu.
Fixed in 2f5cf69: harvestRecyclableMenuCardViews now calls setHighlighted(false) on the view and drops the menu's highlightedMenuItems entry when it strips the tracked item, before pooling. Behavior matches main's rebuild semantics (fresh content starts unhighlighted; the next mouse-move re-highlights via willHighlight). Added a regression test: harvesting a highlighted card clears its highlight and tracking entry. Full suite + make check green.
|
Codex review: needs real behavior proof before merge. Reviewed June 10, 2026, 12:53 PM ET / 16:53 UTC. Summary Reproducibility: not applicable. as a PR-level performance optimization. The reconstruction path is source-verifiable and related reports contain field samples, but this branch has not established an affected packaged after-fix reproduction. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the one-pass, cache-aware reuse design if a freshly packaged affected configuration demonstrates lower menu rebuild cost with correct content, sizing, hover, click, submenu, and tab-switch behavior, and leave Do we have a high-confidence way to reproduce the issue? Not applicable as a PR-level performance optimization. The reconstruction path is source-verifiable and related reports contain field samples, but this branch has not established an affected packaged after-fix reproduction. Is this the best way to solve the issue? Unclear. The pool is narrow and follows the existing merged-cache ownership boundary, but a fresh-bundle run is necessary to show that preserving hosting graphs is safer and materially better than narrower caching approaches. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: reasoning high; reviewed against 08c171b6b487. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
|
Two responses to the review: Changelog (P3): I checked repo practice before opening this — the changelog entry follows the established contributor convention here. The four most recently merged PRs all carry their own Reproducible timing harness: full file below — drop it into swift test --filter "timing harness"It mutates the seeded snapshot before every tick so card height fingerprints miss their cache, matching real refresh behavior (ticks always change MenuRepopulateTimingTests.swiftimport AppKit
import CodexBarCore
import SwiftUI
import Testing
@testable import CodexBar
extension StatusMenuTests {
@Test
func `timing harness data only repopulate`() {
StatusItemController.setMenuRefreshEnabledForTesting(false)
let previousRendering = StatusItemController.menuCardRenderingEnabled
StatusItemController.menuCardRenderingEnabled = true
defer { StatusItemController.menuCardRenderingEnabled = previousRendering }
let settings = self.makeSettings()
settings.statusChecksEnabled = false
settings.refreshFrequency = .manual
settings.mergeIcons = false
let registry = ProviderRegistry.shared
for provider in UsageProvider.allCases {
if let metadata = registry.metadata[provider] {
settings.setProviderEnabled(provider: provider, metadata: metadata, enabled: provider == .codex)
}
}
settings.costUsageEnabled = true
let store = self.makeCodexStore(settings: settings, dashboardAuthorized: false)
let dayFormatter = DateFormatter()
dayFormatter.dateFormat = "yyyy-MM-dd"
dayFormatter.timeZone = TimeZone(identifier: "UTC")
var dailyEntries: [CostUsageDailyReport.Entry] = []
for offset in 0..<30 {
let day: Date = Date().addingTimeInterval(Double(-offset) * 86400)
let input: Int = 120_000 + (offset * 1000)
let output: Int = 60_000 + (offset * 500)
let total: Int = input + output
let cost: Double = 4.2 + (Double(offset) * 0.3)
dailyEntries.append(CostUsageDailyReport.Entry(
date: dayFormatter.string(from: day),
inputTokens: input,
outputTokens: output,
totalTokens: total,
costUSD: cost,
modelsUsed: ["gpt-5.2-codex", "gpt-5.2", "gpt-5.1-codex-mini"],
modelBreakdowns: nil))
}
store._setTokenSnapshotForTesting(
CostUsageTokenSnapshot(
sessionTokens: 1_234_567,
sessionCostUSD: 12.34,
last30DaysTokens: 45_678_910,
last30DaysCostUSD: 123.45,
daily: dailyEntries,
updatedAt: Date()),
provider: .codex)
let controller = StatusItemController(
store: store,
settings: settings,
account: UsageFetcher().loadAccountInfo(),
updater: DisabledUpdaterController(),
preferencesSelection: PreferencesSelection(),
statusBar: self.makeStatusBarForTesting())
defer { controller.releaseStatusItemsForTesting() }
let menu = controller.makeMenu()
controller.populateMenu(menu, provider: .codex)
let clock = ContinuousClock()
var samples: [Double] = []
for iteration in 0..<60 {
// Real refresh ticks change usage data, so card height fingerprints miss their
// cache and every populate re-measures. Mirror that here.
let used: Double = 20 + Double(iteration % 50)
store._setSnapshotForTesting(
UsageSnapshot(
primary: RateWindow(
usedPercent: used,
windowMinutes: 300,
resetsAt: Date().addingTimeInterval(1800 + Double(iteration)),
resetDescription: nil),
secondary: RateWindow(
usedPercent: 100 - used,
windowMinutes: 10080,
resetsAt: Date().addingTimeInterval(86400),
resetDescription: nil),
tertiary: nil,
updatedAt: Date(),
identity: ProviderIdentitySnapshot(
providerID: .codex,
accountEmail: "codex@example.com",
accountOrganization: nil,
loginMethod: "Plus Plan")),
provider: .codex)
controller.invalidateMenus(allowStaleContentDuringDataRefresh: true)
let elapsed = clock.measure {
controller.populateMenu(menu, provider: .codex)
}
samples.append(Double(elapsed.components.attoseconds) / 1e15 +
Double(elapsed.components.seconds) * 1000)
}
samples.sort()
let median = samples[samples.count / 2]
let p90 = samples[Int(Double(samples.count) * 0.9)]
let avg = samples.reduce(0, +) / Double(samples.count)
print("TIMING_RESULT single_card median_ms=\(median) p90_ms=\(p90) avg_ms=\(avg) n=\(samples.count)")
#expect(!samples.isEmpty)
}
@Test
func `timing harness merged overview repopulate`() {
StatusItemController.setMenuRefreshEnabledForTesting(false)
let previousRendering = StatusItemController.menuCardRenderingEnabled
StatusItemController.menuCardRenderingEnabled = true
defer { StatusItemController.menuCardRenderingEnabled = previousRendering }
let overviewProviders: [UsageProvider] = [
.codex, .claude, .gemini, .cursor, .openrouter, .deepseek, .mistral, .grok,
]
let settings = self.makeSettings()
settings.statusChecksEnabled = false
settings.refreshFrequency = .manual
settings.mergeIcons = true
settings.mergedMenuLastSelectedWasOverview = true
let registry = ProviderRegistry.shared
for provider in UsageProvider.allCases {
if let metadata = registry.metadata[provider] {
settings.setProviderEnabled(
provider: provider,
metadata: metadata,
enabled: overviewProviders.contains(provider))
}
}
let store = self.makeCodexStore(settings: settings, dashboardAuthorized: false)
let now = Date()
for provider in overviewProviders {
store._setSnapshotForTesting(
UsageSnapshot(
primary: RateWindow(
usedPercent: 35,
windowMinutes: 300,
resetsAt: now.addingTimeInterval(2400),
resetDescription: nil),
secondary: RateWindow(
usedPercent: 41,
windowMinutes: 10080,
resetsAt: now.addingTimeInterval(86400),
resetDescription: nil),
tertiary: nil,
updatedAt: now,
identity: ProviderIdentitySnapshot(
providerID: provider,
accountEmail: "user@example.com",
accountOrganization: nil,
loginMethod: "Plan")),
provider: provider)
}
let controller = StatusItemController(
store: store,
settings: settings,
account: UsageFetcher().loadAccountInfo(),
updater: DisabledUpdaterController(),
preferencesSelection: PreferencesSelection(),
statusBar: self.makeStatusBarForTesting())
defer { controller.releaseStatusItemsForTesting() }
let menu = controller.makeMenu()
controller.populateMenu(menu, provider: nil)
let cardCount = menu.items.count { ($0.representedObject as? String)?.isEmpty == false }
let clock = ContinuousClock()
var samples: [Double] = []
for _ in 0..<60 {
controller.invalidateMenus(allowStaleContentDuringDataRefresh: true)
let elapsed = clock.measure {
controller.populateMenu(menu, provider: nil)
}
samples.append(Double(elapsed.components.attoseconds) / 1e15 +
Double(elapsed.components.seconds) * 1000)
}
samples.sort()
let median = samples[samples.count / 2]
let p90 = samples[Int(Double(samples.count) * 0.9)]
let avg = samples.reduce(0, +) / Double(samples.count)
print(
"TIMING_RESULT merged_overview cards=\(cardCount) median_ms=\(median) " +
"p90_ms=\(p90) avg_ms=\(avg) n=\(samples.count)")
#expect(!samples.isEmpty)
}
}On packaged proof: agreed that's the decisive gate. I'll follow up with a packaged before/after capture from a local config if I can make it representative; the #1274/#1314-style retest from the reporters with affected heavy multi-provider setups (tagged in the PR body) plus the suggested Mantis run would settle the field side either way. |
|
Round 2, after field profiling on a real multi-provider setup surfaced where the remaining lag lives. Two findings worth recording, then the change. Field finding 1 — switch lag is the dominant complaint path, and the tab cache can't help. 90-second Field finding 2 — view recycling alone doesn't fix switches. Per-interaction-normalized populate cost was flat with commit The change (
This is the shape #1374's closing asked for — a design that does not rebuild the full parent menu during tracking — applied to both the tick and switch paths. Numbers (unit harness, interleaved runs, data varied per tick so height fingerprints miss): single-card data-tick repopulate 8.75ms vs 11.96ms median (~27% less); provider-switch repopulate 7.84ms vs 8.87ms (~12% less). The unit harness understates the switch win by construction — test menus are never inside a live menu window, so the per-insert relayout cost that reconciliation eliminates does not appear in it; that part is exactly the hot subtree in the field samples above. Validation: 7 focused tests now, including the key structural assertion — a merged data-tick repopulate leaves the menu's Packaged before/after interaction proof from the same real-config setup is being collected and will follow. |
bcssewl
changed the title
|
Round 3, from continued field profiling of the round-2 build on the same real multi-provider setup. What the new trace showed: the all-or-nothing reconcile rarely engaged on provider switches — providers differ in their bottom action sections, so the skeleton check failed and the wholesale fallback ran. The Changes (
Validation: |
|
Promised packaged-app follow-up — before/after from the affected real multi-provider setup (Apple Silicon, merged icons, daily-driver config). Before (current release lineage): 90s Intermediate (recycling only, After (edge reconcile + #1397, combined build): interactive verification on the same setup reports responsive switching with no beachballs; both previously dominant paths are eliminated by construction — same-shape rows update in place (zero remove/insert during tracking, enforced by the item-identity regression test) and the event peek runs only when a key/click counter advanced. Happy to attach redacted raw |
Co-authored-by: bcssewl <samirbassel@gmail.com>
fa922c2 to
7918c08
Compare
2 participants
Refs #1374, #1321, #1311, #1360, #1325, #1308.
Summary
Two structural changes that together remove the rebuild cost from CodexBar's hottest menu paths — background data ticks and provider switches — without touching any scheduling semantics:
populateMenu's teardown harvests the outgoing items' card hosting views, andmakeMenuCardItemadopts a harvested view when the card identifier matches — or, failing that, the first type-compatible leftover (a usage card is the same SwiftUI content type for every provider, so cold provider switches repaint existing views instead of constructing fresh hierarchies). The replacedrootViewis diffed in place by SwiftUI on the live graph.captureMenuparameter threaded through the builders), and a position-wise reconciler mutates the liveNSMenuItems in place when the row skeleton (separators, identifiers, view classes) is unchanged — zero removals, zero insertions, so AppKit never relayouts the open tracked menu per insert. Mismatched skeletons fall back to the wholesale swap.Tab switches whose incoming cache entry is still valid keep the instant wholesale-reattach path; the displaced selection's cache entry is consumed before harvesting so no cached item can ever share a view with a live one. See the round-2 comment for the field traces that motivated the reconciler and the detailed numbers.
Why
Every data tick currently pays full hosting-view reconstruction wherever a rebuild lands:
populateMenusynchronously insidemenuWillOpen—NSHostingViewcreation + SwiftUI graph construction + initial layout/height measurement per card, before the dropdown can appear. Open data-stale dropdowns in two phases (populateMenu off the click path) #1375 was closed because relocating that full rebuild into the tracking window isn't acceptable; making the rebuild not reconstruct hosting views attacks the same latency from the other side, with no scheduling risk.samples of 0.32.4 idle CPU show the closed-menu rebuild loop dominated byNSHostingView.__deallocating_deinit → GraphHost.invalidate → AG::Graph::invalidate_subgraphsplus from-scratch AttributeGraph re-evaluation — exactly the teardown/reconstruction this PR removes. With recycling, the per-card graph persists and the swappedrootViewis diffed incrementally.addMenuCards → MenuCardItemHostingView.measuredHeight → NSHostingController.sizeThatFitsas the hot subtree; with changing data the height-fingerprint cache misses, and on current main that measurement is a first layout of a virgin hosting view. On a recycled view it is an incremental relayout.Measurements
Unit-level timing of 60 data-only repopulates of a single codex card (cost usage enabled, 30-day daily breakdown seeded, snapshot data varied per tick so height fingerprints miss the cache, matching real refresh behavior —
refreshFrequencyticks always changeupdatedAt/usage):≈ 25–30% less main-thread time per repopulate for one moderate card; the saving is per card, so merged menus with several cards/overview rows save proportionally more. When data does not change between populate passes (fingerprint cache hits on both sides), timings are flat — the win is specifically the virgin-hosting-view construction + first layout that recycling eliminates, consistent with the field samples above.
Timing harness (not committed; drop into Tests/CodexBarTests to reproduce)
// 60 iterations of: mutate codex snapshot (usedPercent/resetsAt) → // invalidateMenus(allowStaleContentDuringDataRefresh: true) → // clock.measure { controller.populateMenu(menu, provider: .codex) } // reporting median/p90/avg; single-provider menu, rendering enabled, // cost usage enabled with a 30-entry daily breakdown.I kept the harness out of the tree since timing assertions are CI-flaky; happy to post the full file in a comment or gist if useful.
Safety notes
canRecycleMenuCardViewsgate (cache-empty per menu) makes that state unreachable: any pass that just cached outgoing tab content, or that could re-attach cached content, sees a non-empty cache and skips harvesting entirely.MenuCardItemHostingView.setHighlighteddrives theMenuCardHighlightStatecaptured at construction, so adoption rebuilds the wrappedMenuCardSectionContainerViewaround the recycled view's own state object — hover highlighting keeps working across reuse (covered by a test).onClickis replaced on reuse and the gesture recognizer is installed lazily if a recycled card gains a click action it didn't have.defer { clearMenuCardViewRecyclePool() }on both teardown sites); unconsumed views are released there, so no retention growth.menuCardRenderingEnabled == falsenothing is harvested or adopted.Validation
MenuCardViewRecyclingTests.swift: data-only repopulate reuses hosting views by identity; preserved merged-switcher caches disable harvesting (and harvesting detaches views from outgoing items); recycled cards keep their hosting view + highlight state; same-id/different-type builds a fresh view and consumes the pool entryswift test(full suite): green except the pre-existing locale-dependent expectation inMiniMaxMenuCardModelTests("Renews: May 18, 2027" vs "18 May 2027"), which fails identically on cleanmainin a non-US-format localemake check: 0 violationsswift build,swift test,make checkI can't reproduce the heavy multi-provider field configs locally, so the unit numbers above are the floor, not the ceiling. A packaged retest from the #1274/#1314/#1325 measurement setups (@giuseppebisemi, @psufka, @dengshu2 — the before/after
samplestack identity would be:MenuCardItemHostingView.__deallocating_deinit/ initialsizeThatFitsdisappearing from repopulate paths) would be the decisive field proof, as in previous rounds.