perf: skip storage footprint republish when scan is unchanged - #1416
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 3:40 PM ET / 19:40 UTC. Summary Reproducibility: yes. Current main unconditionally republishes the timestamp-bearing footprint dictionary, and the focused real-directory Observation regression directly verifies the second identical scan is a no-op after the patch. 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. Next step before merge
Security Review detailsBest possible solution: Merge the current exact head after its refreshed CI completes, retaining the dedicated content comparison and unconditional refresh-bookkeeping updates. Do we have a high-confidence way to reproduce the issue? Yes. Current main unconditionally republishes the timestamp-bearing footprint dictionary, and the focused real-directory Observation regression directly verifies the second identical scan is a no-op after the patch. Is this the best way to solve the issue? Yes. A dedicated content comparator avoids changing the type's global Equatable contract, preserves all user-visible storage fields, and continues advancing scan signatures and timestamps. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5cc8a248a457. Label changesLabel 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
|
c7c7ad6 to
e8634e6
Compare
e8634e6 to
fc6386a
Compare
bdf5b3d to
5463ff8
Compare
5463ff8 to
6412dec
Compare
|
Rebased onto current Validation:
Peekaboo/runtime proof with provider storage display temporarily enabled:
The focused regression is the direct proof for the optimization boundary: an unchanged second scan does not republish @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
162912e to
a262b29
Compare
|
Rebased onto current Proof:
Waiting for exact-head CI before merge. |
|
Exact-head maintainer proof for
|
|
Additional exact-head proof for
|
Co-authored-by: Soohan Park <725psh@gmail.com>
a262b29 to
ce5b595
Compare
|
Landed in Proof on the rebased exact head
Thanks @soohanpark! |
|
Validated rebased exact head
|
What
UsageStore.applyStorageFootprintsreassigned the observableproviderStorageFootprintson every scan, even when the scanned bytes were identical. Storage scans run on every menu open (refreshStorageFootprintsForOverview) and roughly every 5 minutes, so each no-op scan woke themenuObservationTokenobservation →invalidateMenus, which clears the merged-menu content caches (clearMergedSwitcherContentCaches) and schedules avoidable rebuild work — pure churn when nothing the user sees changed.The reason a guard wasn't already effective:
ProviderStorageFootprintisEquatable, but it carriesupdatedAt: Date, stamped fresh on each scan, so two scans of identical on-disk data never compare equal.Change
ProviderStorageFootprint.hasSameContents(as:)— value equality over everything exceptupdatedAt(which is never surfaced to the UI).applyStorageFootprints, reuse the existing footprint when only the timestamp would change, then guard the observable write with!=. An unchanged scan is now a true no-op and fires no observation.Test
Adds
repeated identical storage refresh does not republish observable footprints— asserts (viawithObservationTracking) that a second scan over identical on-disk data does not republishproviderStorageFootprints. Fails before the change, passes after.Full
swift test: green (the only failure on my machine is the pre-existing locale-dependent MiniMax date assertion atMenuCardModelTests.swift:735, unrelated to this change — it hardcodes an en_US date string).Manual verification (release build)
Built and ran a release build (
Scripts/compile_and_run.sh) and exercised the merged Overview menu — repeated open/close, Claude↔Codex tab switching, and chart-row hovers — while streaming the app's own os_log (subsystem == "com.steipete.codexbar"). BothlogMenuOperationDurationIfSlow(≥150 ms) andlogChartRenderDurationIfSlow(≥50 ms) log via the same.warningpath, which persists to the log store, so a slow op would have been captured.Over the session:
slow menu operationwarnings (open/close/tab-switch blocking the main thread ≥150 ms): 0slow chart render: 1 —55.1 mson the first hydrate ofusageHistoryChart(first hover only; subsequent hovers are skipped by the existing render-signature guard).So the open/close freeze does not reproduce on this branch, and the one remaining sub-threshold hitch is the inherent first-build cost of a SwiftUI chart (not addressed here, and not cacheable on first render).
Scope / notes
This is a small, low-risk churn-reduction. It is not the main menu open/close freeze fix — recent
mainalready addresses that via hosted-chart render signatures (#1384), card hosting-view harvest/recycling, height caching, and deferred rebuilds (#1376). This just removes one remaining source of needless menu invalidation that those caches then have to recover from.🤖 Generated with Claude Code
Maintainer proof
Rebased head
6412dec8passes 16 focused storage tests,make check,git diff --check, and clean autoreview. The focused Observation regression performs two identical scans over a real temporary Codex directory and verifies that the second scan does not republish observable storage state.A freshly packaged release bundle rendered the storage row in Overview and completed three Peekaboo menu close/open cycles without crash, hang,
slow menu operation, orslow chart renderdiagnostics. The temporary storage-display preference was restored afterward.