Fix visual settings refresh#1996
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 11:25 PM ET / 03:25 UTC. Summary Reproducibility: yes. Source inspection on current main shows visual settings are observed by UsageStore background work and the on-change path calls refreshForSettingsChange; I avoided live provider validation because AGENTS.md warns against provider and Keychain prompting checks. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. 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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the refresh-boundary split once redacted real-behavior proof and normal validation show visual-only settings repaint cached UI while provider, auth, account, refresh, alert, history, and optional-usage settings still refresh data. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows visual settings are observed by UsageStore background work and the on-change path calls refreshForSettingsChange; I avoided live provider validation because AGENTS.md warns against provider and Keychain prompting checks. Is this the best way to solve the issue? Yes. The revision-counter split is a narrow maintainable fix, and the latest head treats optional usage as data-affecting because it changes ProviderFetchContext.includeOptionalUsage; the remaining blocker is proof, not a code finding. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against aa401f1d8b74. 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
Review history (5 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Landed as 5c6e6f9. Thanks @Zihao-Qi! Maintainer verification:
Review note: |
Address the remaining P2 review points on steipete#2001: - Quota hooks no longer depend on notification preferences. Quota-warning and session-depletion detection now runs when either notifications OR a matching hook rule is enabled, and only the notification post is gated on the notification settings. Gating is keyed on hasQuotaHookRule(event:provider:), so behavior is byte-identical for users without a configured quota hook. - Editing hook rules no longer triggers a provider refresh. Hook config writes go through a dedicated path that persists and bumps a separate hooksRevision for the settings pane, without bumping configRevision (which drives the background refresh introduced by steipete#1996). Bundles the per-refresh quota-warning constants into QuotaWarningTransitionContext to keep the transition helper within the parameter-count limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Fixes #1994 by separating visual/display settings changes from background provider refresh settings.
Why
Changing visual settings such as usage bar display mode, icon layout, loading pattern, or other menu presentation options should update cached UI only. Previously, those changes were observed by the background refresh path and could trigger provider quota probes, which risks hitting provider rate limits while the user is only adjusting display preferences.
This keeps quota refreshes scoped to settings that actually affect provider data, credentials, background checks, alerts, cost/history processing, or account layout behavior.
Behavior Proof
I avoided live provider probes and real Keychain reads, per the repository guidance against validation that can display credential or Keychain prompts. The proof below uses redacted/no-credential test seams that would record provider refreshes if the settings observer triggered them.
Display-only settings do not refresh providers:
UsageStoreCoverageTests.display only settings do not invoke provider refresh while background work is activeinstalls_test_providerRefreshOverrideto record every provider refresh request.backgroundWorkSettingsObservationTokenwith the same callback shape used by the background settings watcher: on change, callrefreshForSettingsChange().usageBarsShowUsed,mergeIcons,randomBlinkEnabled, anddebugLoadingPattern.refreshForSettingsChange()and confirms.codexis recorded, proving the override would catch a provider refresh if one occurred.Provider-affecting settings still refresh:
UsageStoreCoverageTests.background work settings observation ignores display only settings churnfirst confirms the same display-only setting changes do not fire the background observer.statusChecksEnabledand observes the background-work token firing.multiAccountMenuLayout, then confirms the token fires again.Cached UI/widget state updates without unrelated config churn:
StatusItemIconObservationSignatureTests.display settings persist cached widget snapshottogglesusageBarsShowUsed, waits forwidgetSnapshotPersistTask, and confirms the cached widget snapshot recordsusageBarsShowUsed == truewith a Codex entry.StatusItemIconObservationSignatureTests.config only settings do not persist cached widget snapshotmutateszaiAPITokenand confirms no widget snapshot is persisted for that unrelated config-only change.Validation
swift test --filter UsageStoreCoverageTestsswift test --filter StatusItemIconObservationSignatureTestsswift test --filter UsageStoreWidgetSnapshotTestsmake checkmake test