fix: retain flicker-probe sessions so release builds actually record - #2709
Conversation
ProbeSession(...).begin() ran as an unretained temporary whose only other reference was the driving timer's [weak self], so release-mode builds deallocated the session at creation (compiler: "weak reference will always be nil") and the probe recorded nothing. Sessions are now strongly owned in MenuSwitchFlickerProbe.activeSessions until finish() unregisters them, and ProbeSession gained a Configuration seam (timings + menu opener) so a test can run a full session against a synthetic merged menu and prove it records switch activity and frame samples.
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix: retain flicker-probe sessions so release builds actually record This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
![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: 976eb2e4a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| account: fetcher.loadAccountInfo(), | ||
| updater: DisabledUpdaterController(), | ||
| preferencesSelection: PreferencesSelection(), | ||
| statusBar: .system) |
There was a problem hiding this comment.
When this macOS test constructs a StatusItemController against .system, the initializer registers a real NSStatusItem, but this new test never calls controller.releaseStatusItemsForTesting() like the neighboring status-menu tests do. That leaves the menu-bar item behind after the serialized test finishes, which can pollute later headless AppKit/status-menu tests; add a defer to release it or use a non-system status-bar seam.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
1 participant
Summary
The Tahoe menu-flicker self-probe (
MenuSwitchFlickerProbe) was silently dead in release builds: eachProbeSession(...).begin()ran as an unretained temporary whose only other reference was the driving timer's[weak self]. The release optimizer deallocated the session at creation — surfacing as the compiler warning "weak reference will always be nil" at the two creation sites — so the timer never ticked, the menu was never located, and no frames or logs were recorded.Changes
MenuSwitchFlickerProbe.activeSessionsfrom creation untilfinish()unregisters them (beginRetainedSession/endSession). This covers both the normal end-of-schedule path and the give-up path, which both funnel throughfinish().ProbeSessiongained aConfigurationseam: switch schedule / session-end timings (previously file statics) and anopenMenuclosure standing in for the blockingopenMenuFromShortcut()tracking loop.MenuSwitchFlickerProbeTestsruns a full session against a realStatusItemControllerandProviderSwitcherViewmenu on a ~400ms schedule, asserting the session stays retained while running, is released after finishing, and recordsmenu located,handled=trueswitches, and a positive frame-sample count.Proof
swift build -c releasereproduces the warning at both creation sites; fixed code compiles warning-free (only the pre-existingCGWindowListCreateImagedeprecation remains).swift test --filter MenuSwitchFlickerProbeTestspasses (0.8s).make checkclean (0 violations, 1810 files).