Reduce merged icon observation churn#1297
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 5, 2026, 2:52 PM ET / 18:52 UTC. Summary Reproducibility: yes. the current-main source still shows broad provider snapshot stringification, and the PR body gives a concrete macOS 26.5 packaged-app sampling path plus focused contract tests. I did not rerun that live sampling in this read-only pass. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused signature narrowing if maintainers accept the availability tradeoff, and keep the broader remaining menu-lag work tracked through #1274 and #1314. Do we have a high-confidence way to reproduce the issue? Yes: the current-main source still shows broad provider snapshot stringification, and the PR body gives a concrete macOS 26.5 packaged-app sampling path plus focused contract tests. I did not rerun that live sampling in this read-only pass. Is this the best way to solve the issue? Yes: the branch is a narrow performance repair that aligns the observation signature with the existing render inputs and keeps the remaining menu rebuild lag separate. The safer alternative would be to add another contract test only if maintainers identify a missing visible input. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 65e39f4dcb3a. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat 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
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors menu bar icon observation signature generation and expands test coverage to ensure the signature only changes when user-visible icon state changes (not when non-visual snapshot metadata churns).
Changes:
- Extracted
storeIconObservationSignature()into a dedicatedStatusItemController+IconObservation.swiftextension and updated the signature contents. - Added several tests to validate that the signature ignores non-visual churn (e.g., account email / updatedAt) while still changing for visual deltas (percent bars, status, credit fallback).
- Introduced shared helpers (
menuBarCreditsRemainingForIcon, widened access for existing helpers) to keep icon rendering + signature logic aligned.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusItemIconObservationSignatureTests.swift | Adds tests covering signature stability vs. visual changes and merged/non-merged behaviors. |
| Sources/CodexBar/StatusItemController.swift | Removes the inlined observation signature methods (moved to a new extension file). |
| Sources/CodexBar/StatusItemController+IconObservation.swift | New implementation of the icon observation signature with merged/non-merged handling and reduced churn sensitivity. |
| Sources/CodexBar/StatusItemController+Animation.swift | Reuses a new credits helper in icon rendering; loosens access control for helpers used by the new extension; removes a SwiftLint suppression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func menuBarCreditsRemainingForIcon(provider: UsageProvider, snapshot: UsageSnapshot?) -> Double? { | ||
| guard provider == .codex, | ||
| let creditsRemaining = self.store.credits?.remaining, | ||
| creditsRemaining > 0 | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| let rateWindows = [snapshot?.primary, snapshot?.secondary].compactMap(\.self) | ||
| guard rateWindows.isEmpty || rateWindows.contains(where: { $0.remainingPercent <= 0 }) | ||
| else { | ||
| return nil | ||
| } | ||
| return creditsRemaining | ||
| } |
|
|
||
| @discardableResult | ||
| func applyIcon(phase: Double?) -> Bool { // swiftlint:disable:this function_body_length | ||
| func applyIcon(phase: Double?) -> Bool { |
| static func iconSignatureValue(_ value: Double?) -> String { | ||
| guard let value else { return "nil" } | ||
| return String(format: "%.3f", value) | ||
| } |
| let visibleProviders = self.store.enabledProvidersForDisplay().map(\.rawValue).sorted().joined(separator: ",") | ||
| let providerSignatures: String | ||
| let primaryProvider: UsageProvider? | ||
| if mergeIcons { | ||
| let primary = self.primaryProviderForUnifiedIcon() | ||
| primaryProvider = primary | ||
| providerSignatures = [ | ||
| self.providerStoreIconObservationSignature(for: primary, showBrandPercent: showBrandPercent), | ||
| "mergedStatus=\(self.mergedIconStatusIndicator().rawValue)", | ||
| ].joined(separator: "||") | ||
| } else { | ||
| primaryProvider = nil | ||
| providerSignatures = UsageProvider.allCases | ||
| .filter { self.isVisible($0) } | ||
| .map { self.providerStoreIconObservationSignature(for: $0, showBrandPercent: showBrandPercent) } | ||
| .joined(separator: "||") | ||
| } |
| } | ||
|
|
||
| private func primaryProviderForUnifiedIcon() -> UsageProvider { | ||
| func primaryProviderForUnifiedIcon() -> UsageProvider { |
| } | ||
|
|
||
| private func shouldAnimate(provider: UsageProvider, mergeIcons: Bool? = nil) -> Bool { | ||
| func shouldAnimate(provider: UsageProvider, mergeIcons: Bool? = nil) -> Bool { |
clawsweeper
Bot
added
rating: 🧂 unranked krab
A main-thread sample of the packaged build (Merge Icons on, macOS 26.5) showed providerStoreIconObservationSignature still bottoming out in String(describing:) -> _adHocPrint_unlocked reflection, from two String(describing:) calls over the payload-free IconStyle enum. Make IconStyle String-raw-represented (rawValue == case name, so the signature string is byte-identical) and replace both String(describing:) calls with .rawValue. The icon-observation leaf is now reflection-free; a re-sample shows providerStoreIconObservationSignature and _adHocPrint_unlocked gone from the path entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@clawsweeper re-review Added the icon-observation behavior proof to the PR body: the 8 signature contract tests (ignore-churn + react-to-visible-change, both directions) on macOS 26.5, plus a packaged-app main-thread Also pushed a follow-up ( |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review CI is green on the current head Additional runtime proof from #1274 is also positive for this PR scope: @giuseppebisemi retested packaged #1297 on macOS 26.5 with Merge Icons on, and the targeted icon-observation reflection path is gone ( |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
…lled predicate menuBarCreditsRemainingForIcon (used by both the rendered menu-bar icon and the icon observation signature) reimplemented the menu-bar credits fallback with its own rate-window predicate over snapshot.primary/secondary. That is a second source of truth for a decision the Codex projection already owns (codexConsumerProjection -> menuBarFallback == .creditsBalance): equivalent today, but free to drift from the rendered/menu fallback semantics as the projection evolves. Delegate to store.codexMenuBarCreditsRemaining instead, so render, signature, and the menu-bar fallback all read one projection. The projection is pure value composition over already-loaded snapshot/credits state (no IO), so the icon/signature path stays cheap. Behavior is unchanged in the covered cases (8 icon observation signature tests still pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
clawsweeper
Bot
added
rating: 🦞 diamond lobster
clawsweeper
Bot
added
the
rating: 🦀 challenger crab
clawsweeper
Bot
added
rating: 🦞 diamond lobster
clawsweeper
Bot
added
rating: 🦀 challenger crab
Refs #1274.
Summary
StatusItemController+IconObservation.swift.UsageSnapshotstringification (String(describing: snapshot)) from the icon observation signature.IconStyleString-raw-represented and sign it via.rawValue, removing the lastString(describing:)reflection (_adHocPrint_unlocked) from the icon-observation leaf.Why
@giuseppebisemisampled the residual lag after #1286 and the hot path wasobserveStoreIconChanges -> storeIconObservationSignature -> providerStoreIconObservationSignature. The old signature subscribed to every provider snapshot and usedString(describing: snapshot), so account/email/timestamp and non-rendered provider churn could retrigger icon work even when the visible merged icon would not change.Behavior proof
Live runtime proof on the exact reported platform — macOS 26.5 (build 25F71), Apple Swift 6.3.2 — against this branch head.
1) Signature contract tests (both directions: ignore churn / react to visible change)
The four
ignores …tests cover the stale-content risk (non-rendered snapshot / account / timestamp / unused-credit churn no longer moves the signature); the fourchanges when …tests prove visible percent, Codex credits fallback, primary-vs-aggregate status, and status-indicator changes still move it. So the narrowed signature does not suppress required icon refreshes.2) Packaged-app main-thread
sample(Merge Icons on), before vs after the reflection-removal commitSampled
sample <pid>over the merged status item while driving repeated store refresh churn (open/dismiss the menu).Before — the icon-observation leaf still bottomed out in reflective string printing:
After —
providerStoreIconObservationSignatureand_adHocPrint_unlockedare gone from the path; it now bottoms out at cheap value reads:Whole-sample greps after the fix:
_adHocPrint_unlocked= 0,providerStoreIconObservationSignature= 0 (it was present before). The only residualswift_getObjectTypeframes are in unrelated FoundationURLbridging /__JSONEncodercode, not the icon path.Scope / honesty: this is deterministic contract proof plus a packaged-app main-thread sample on macOS 26.5, not a video. It demonstrates the icon-observation signature no longer does full-snapshot or enum reflection on the main thread, and that visible icon updates are preserved. The original field repro is
@giuseppebisemi's post-#1286 sample in #1274.Notes
This is the icon-observation half of the residual Merge Icons lag (#1286 covered the menu close/rebuild half). It does not change rendered icon output — only what retriggers the icon work.
Validation
swift buildswift build --build-testsxcrun xctest -XCTest CodexBarTests.StatusItemIconObservationSignatureTests .build/arm64-apple-macosx/debug/CodexBarPackageTests.xctest(8 Swift Testing tests passed on macOS 26.5)make check(0 lint violations)samplebefore/after (above)Residual merged-menu close freeze → separate next slice (#1274)
This PR is the icon-observation half of the residual Merge Icons lag (#1286 covered the menu close/rebuild half). The remaining menu-rebuild close freeze
@giuseppebisemiprofiled after254412f0(#1314) is a separate slice, not in scope here, captured for clarity:Root cause (verified in current main):
menuDidCloseonly defers the merged menu when it is already stale at close time, butobserveStoreChangesinvalidates withallowStaleContentDuringDataRefresh: trueand still falls through toprepareAttachedClosedMenusIfNeeded()— so a merged menu that closed fresh is rebuilt while closed on the next background tick.The fix is self-contained: in merged mode, skip the closed merged-menu rebuild entirely (defer-until-next-open unconditionally in
prepareAttachedClosedMenusIfNeeded;menuWillOpenalready repopulates synchronously before display, so no stale flash or width jump). That takes the 388-sample path to 0. It will be its own measured slice on #1274; this PR lands independently of it.