Make merged-Overview provider limit user-configurable - #2339
Closed
Vit129 wants to merge 6 commits into
Closed
Conversation
Was hardcoded to 3 (docs/ui.md, issue steipete#2107). Now backed by UserDefaults (static var, since it's used as a default-parameter value elsewhere and Swift disallows self in those) with a Settings stepper (1-12, default 3 for no behavior change).
![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: 3017da8f53
ℹ️ 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".
| private static let maxOverviewProviders = SettingsStore.mergedOverviewProviderLimit | ||
| private static var maxOverviewProviders: Int { SettingsStore.mergedOverviewProviderLimit } | ||
|
|
||
| @AppStorage("mergedOverviewProviderLimit") private var overviewProviderLimitSetting = 6 |
There was a problem hiding this comment.
When this setting changes after the merged Overview menu has already been rendered, only UserDefaults.standard is updated. StatusItemController invalidates/rebuilds menu content via observation of settings.menuObservationToken, and that token never reads this new key, so menuNeedsUpdate can consider the existing menu fresh and keep showing the old number of Overview rows until an unrelated observed setting/store change or app restart. Please route the limit through SettingsStore/defaultsState and include it in menu observation, or otherwise invalidate the merged menu when the value changes.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 11:52 AM ET / 15:52 UTC. ClawSweeper reviewWhat this changesAdds a persisted Settings stepper that lets users choose one to twelve providers for the merged Overview tab, while retaining six as the default. Merge readiness⛔ Blocked until real behavior proof is added - 5 items remain Keep this PR open for a maintainer product-direction choice and fresh real-behavior proof. The code is a focused, plausible implementation, but the related Overview work now has competing per-provider-list and unified-card directions. Likely related people
Priority: P3 Review scores
Verification
How this fits togetherThe merged menu reads enabled providers and the user’s Overview selection, resolves a capped list, then renders it as Overview rows. This PR replaces the fixed cap with a setting stored in the app’s configured defaults store and observed by already-open menus. flowchart LR
A[Enabled providers] --> C[Overview selection resolver]
B[Menu Bar settings stepper] --> D[Persisted provider limit]
D --> C
C --> E[Merged menu controller]
E --> F[Overview provider rows]
D --> G[Menu refresh observation]
G --> E
Decision needed
Why: The requested setting is technically bounded, but it controls a UI model that the linked work is actively reconsidering; code review cannot determine which long-lived user-facing configuration should exist. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Choose the intended Overview layout first; if the per-provider list remains supported, land this narrow preference only after fresh-bundle proof shows the stepper updates both newly opened and already-open Overview menus. Do we have a high-confidence way to reproduce the issue? No. The PR body claims a debug build but leaves its manual verification unchecked, and it contains no screenshot, recording, runtime output, or redacted log showing the stepper changes actual Overview rows on a fresh bundle. Is this the best way to solve the issue? Unclear. The implementation is a narrow way to configure the current list, but #2107 and #2578 leave the durable Overview layout and configuration direction unresolved. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2bd6a6c37eff. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
clawsweeper
Bot
added
the
merge-risk: 🚨 other
…er changes Addresses clawsweeper P2 finding on steipete#2339: the Overview-limit Stepper wrote straight to UserDefaults via @AppStorage, bypassing SettingsStore entirely, so menuObservationToken (which drives cached-menu invalidation) never changed and an already-open Overview menu kept its old row count. Adds mergedOverviewProviderLimitRevision, an @Observable-tracked instance counter bumped by a new instance accessor for the setting, and touches it in menuObservationToken. The Stepper now binds through settings.mergedOverviewProviderLimit instead of a raw @AppStorage.
…rDefaults Addresses clawsweeper P2 finding on steipete#2339: mergedOverviewProviderLimit read and wrote UserDefaults.standard directly instead of the store's own configured userDefaults, so isolated test stores (and any future non-standard suite) couldn't observe or reset it independently of the real app. Replaces the static get/set with a plain mergedOverviewProviderLimitDefault constant (used only where Swift requires a self-independent default parameter value) and routes every real read/write through the existing mergedOverviewProviderLimit instance accessor, backed by self.userDefaults like the rest of SettingsStore's persisted settings.
clawsweeper
Bot
removed
the
merge-risk: 🚨 other
CI lint gate flagged spaceAroundOperators/wrapMultilineStatementBraces on the overview-limit Stepper introduced in this PR.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
The stepper's title/subtitle were raw English strings while every adjacent Menu Bar settings row uses L(...) (ClawSweeper finding on PR steipete#2339).
Vit129
added a commit
to Vit129/CodexBar
that referenced
this pull request
Aug 2, 2026
…er changes Addresses clawsweeper P2 finding on steipete#2339: the Overview-limit Stepper wrote straight to UserDefaults via @AppStorage, bypassing SettingsStore entirely, so menuObservationToken (which drives cached-menu invalidation) never changed and an already-open Overview menu kept its old row count. Adds mergedOverviewProviderLimitRevision, an @Observable-tracked instance counter bumped by a new instance accessor for the setting, and touches it in menuObservationToken. The Stepper now binds through settings.mergedOverviewProviderLimit instead of a raw @AppStorage.
Vit129
added a commit
to Vit129/CodexBar
that referenced
this pull request
Aug 2, 2026
…rDefaults Addresses clawsweeper P2 finding on steipete#2339: mergedOverviewProviderLimit read and wrote UserDefaults.standard directly instead of the store's own configured userDefaults, so isolated test stores (and any future non-standard suite) couldn't observe or reset it independently of the real app. Replaces the static get/set with a plain mergedOverviewProviderLimitDefault constant (used only where Swift requires a self-independent default parameter value) and routes every real read/write through the existing mergedOverviewProviderLimit instance accessor, backed by self.userDefaults like the rest of SettingsStore's persisted settings.
Vit129
added a commit
to Vit129/CodexBar
that referenced
this pull request
Aug 2, 2026
The stepper's title/subtitle were raw English strings while every adjacent Menu Bar settings row uses L(...) (ClawSweeper finding on PR steipete#2339).
This was referenced Aug 2, 2026
Manual testing found the stepper completely unresponsive. Root cause: mergedOverviewProviderLimit's getter read straight from self.userDefaults, which is @ObservationIgnored — so SwiftUI recorded zero dependencies for that read and never re-rendered the Stepper/Text after a click updated the stored value. Reading the already-tracked mergedOverviewProviderLimitRevision first gives the getter an observable dependency, same technique already used in menuObservationToken. Also moves the stepper's numeric readout out of the Stepper's own label into a LabeledContent accessory (matching CostHistoryDaysEditor's existing pattern) so the +/- hit target isn't sharing space with a three-line label.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Owner
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs/ui.md, issue Allow more than 3 providers in the Overview tab (compact density mode) #2107). This makes it user-configurable via a Settings stepper (1-12), backed byUserDefaults.main, which already raised the hardcoded default from 3 to 6 — kept that as the new configurable default so existing users see no behavior change.Test plan
swift build --product CodexBar -c debugsucceeds