Repository navigation
feat(menu): show Overview as one urgency-sorted line per provider - #4899
Chipagosfinest wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 61b1a59de3
ℹ️ 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".
| self.id = id | ||
| self.provider = model.provider | ||
| self.name = model.providerName | ||
| let metrics = model.metrics.filter { $0.statusText == nil } |
There was a problem hiding this comment.
For Mistral accounts without an included quota window, the balance is represented by the mistral-balance metric's statusText, not creditsText or providerCost. This filter removes the only metric, and fallbackValue never consults status text, so the default Overview now shows only the provider name instead of its balance. Keep excluding these metrics from percentage comparisons, but use their textual value when no percentage metric is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 292ba68: the compact row falls back to the metric's statusText when no percentage metric exists, so a balance-only Mistral account shows its balance. Status-only metrics stay out of percentage comparisons. Covered by a status only limit keeps its text when no percentage exists.
| detailRightText: String?, | ||
| pacePercent: Double?, | ||
| detailIsPaceDerived: Bool = false, | ||
| exhaustionETASeconds: TimeInterval? = nil, |
There was a problem hiding this comment.
For a Claude primary session forecast to exhaust within an hour while more than 10% remains, this new argument stays at nil: PrimaryMetricPresentation and apply(_:to:) never carry PaceDetail.exhaustionETASeconds, and primaryMetric does not pass it to Metric. Consequently, OverviewProviderLine cannot mark that session critical or prefer it over another window, even though sessionPaceDetail calculated the warning. Thread the ETA through the primary presentation/build path, as done for secondary and Codex metrics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 292ba68: PrimaryMetricPresentation now carries exhaustionETASeconds through apply(_:to:) into primaryMetric. Covered by the primary metric carries the forecast only while pace is visible.
| detailRightText: paceDetail?.rightLabel, | ||
| pacePercent: paceDetail?.pacePercent, | ||
| detailIsPaceDerived: paceDetail?.isPaceDerived ?? false, | ||
| exhaustionETASeconds: paceDetail?.exhaustionETASeconds, |
There was a problem hiding this comment.
When paceVisible is false but a Codex limit has more than 10% left and an exhaustion forecast under an hour, paceGatedMetrics clears the forecast text and pace stripe but leaves the ETA copied here. OverviewProviderLine reads that surviving value to recreate the → 0% forecast, mark the row critical, and promote it in the sort. Thus disabling pace no longer suppresses pace-derived information in the default Overview; the new ETA also needs to be cleared by the pace gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 292ba68: paceGatedMetrics clears the ETA, and so does the blocked-window path. Covered by the same pace-visibility test, which asserts every metric's ETA is nil when pace is hidden.
| let leftETA = left.exhaustionETASeconds ?? .infinity | ||
| let rightETA = right.exhaustionETASeconds ?? .infinity | ||
| if leftETA != rightETA { return leftETA < rightETA } |
There was a problem hiding this comment.
When one provider has 0% remaining and another is forecast to exhaust in 20 minutes, this comparison puts the future forecast first. Exhausted windows normally have no ETA because both sessionPace and weeklyPaceDetail suppress forecasts when remaining usage is zero, so their sort key becomes infinity. This places providers that have already stopped below providers that can still run, contrary to the urgency ordering. Treat zero remaining as an effective exhaustion time of zero before comparing forecast ETAs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 292ba68: a row with zero remaining now sorts with an effective ETA of 0, ahead of any forecast. Covered by a provider already at zero sorts ahead of one forecast to stop.
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: blocked before merge. What this changesThis PR replaces Compact Overview cards with urgency-sorted provider lines, makes Compact the default, and raises the shared provider limit to twelve. Example: Eight enabled providers with no saved Overview layout or selection.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 1 item remains Keep this PR open for a maintainer product decision; the earlier attention-row and keyboard blockers are fixed, with no remaining concrete code defect found. Priority: P2 Decision needed
Before merge
FindingsNone. Agent review detailsHow this fits togetherCached provider and account models enter the merged-menu renderer, which combines saved visibility preferences with quota and forecast data to produce navigable Overview rows. flowchart TD
A[Cached provider snapshots] --> B[Usage card models]
C[Saved Overview preferences] --> D[Overview row selection]
B --> D
D --> E[Urgency sorted compact lines]
D --> F[Detailed cards]
E --> G[Provider tab navigation]
F --> G
Technical reviewBest possible solution: Approve the compact row redesign with an explicit choice about existing users' default layout and the shared provider cap. Do we have a high-confidence way to reproduce the issue? The supplied released-app captures demonstrate the tall Overview, and current-main source retains the corresponding Detailed default and stacked rendering path; no target code was executed during this review. Is this the best way to solve the issue? Reusing existing card models, menu tracking, and provider navigation is a coherent implementation, while changing defaults and the shared cap needs an explicit owner decision. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 897bff31731b. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (3 earlier review cycles)
Reviewed November 2, 2026, 1:21 AM ET / 06:21 UTC (Revision 4). |
Carry exhaustion ETAs through the primary metric, clear them with the pace gate and on blocked windows, sort exhausted providers first, and keep status-only values such as Mistral balances in the compact row. Update the plugin budget test for the 12-provider limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:feature
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
the
clawsweeper:intake:feature
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Compact now renders its attention row when every selected provider has failed instead of the empty state; Detailed keeps the empty state. A single attention row now carries a provider identifier and the native action, so Return and accessibility press reach selectOverviewProvider(_:). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AppKit never sends the native action of a highlighted item that hosts a view: Return is dropped and an accessibility press only closes the menu. Deliver Return from the menu tracking monitor and forward the item's accessibility press to its target, so wired rows really do activate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Part of a four-PR Overview redesign, each standalone on
main: #4899 rows, #4900 footer, #4901 chart, #4902 flyout. Merge this one first.Overview today does not fit. With 8 providers connected, the default menu has 2548pt of content, is clipped to 975pt with a scroll arrow, shows 6 providers, and puts Quit out of sight. This makes Overview one line per provider, so all 8 fit in 577pt with the footer on screen.
Attention rows: before and after in the packaged app (2026-11-02 UTC)
Actual-app run · screenshots, checks, findings, driver log, kit and hashes. The same driver ran both unmodified release-config packages on a hosted macOS 26 Mac: before
292ba68c892dbf9d51bfc95e5cd824db983bb9eb, afterf3f90c608f522cdce60e1404323c49a1645f34f0. Provider replies are synthetic, from a local TLS fixture. No real accounts or Keychain prompts.More after-state captures: flyout, only selected provider failed, accessibility press, Detailed unchanged, all after checks, before findings.
Why target and action were not enough: the first fix commit,
3589b98a0, assigned them and the accessibility tree reported the action, yet Return and accessibility press still did nothing in the packaged app. A small probe app (log) showed that AppKit sends neither for an item hosting a view, and that an item-levelaccessibilityPerformPressoverride is honored. So Return now goes through the existing menu-tracking monitor, andMenuCardMenuItemforwards the accessibility press to its target. Plain Compact rows with no flyout had the same failure and are fixed too (DeepSeek above); plain Detailed rows share the wiring but were not driven.Regression tests on the old head: the final
OverviewProviderLineTests, run against the sources of292ba68c8, fail 6 of 23 tests with 8 issues; atf3f90c608all 23 pass. These ran in this Mac's local test harness.Not exercised: VoiceOver itself (the press is a real
AXPressfrom a helper process); Return on a row that has a flyout, which stays with AppKit and has a unit test only; Return on a plain Detailed row. The upgrade and refresh captures below are from the previous head and were not rerun.Packaged-app upgrade and interaction proof, previous head
292ba68c8(2026-11-02 UTC)Actual-app run · original screenshots, saved defaults, checks, findings and hashes. Product
292ba68c892dbf9d51bfc95e5cd824db983bb9eb, unmodified release-config package; controlleree1cb9489. A genuine checksum-verified, Developer ID signed v0.83.0 release was replaced by this ad-hoc signed build in/Applications, using the same defaults domain. Data is synthetic; preferences were seeded using the release's own keys, not clicked in Settings. No real accounts or Keychain prompts.Compatibility risk remains explicit: raw saved preferences are unchanged in all three cases; this does not mean unchanged effective behavior.
The default change and global selection-cap increase 6 → 12 (also affecting Detailed) remain maintainer decisions below. The shared app-group defaults were not separately asserted. Moonshot/DeepSeek name-only rows are visible; Mistral status-only balance is not exercised.
Actual pointer activation selects the provider; a normal Refresh reaches the fixture in that phase, and reopening displays Claude 3% left re-sorted above Codex 8% and z.ai 25%: i5-menu-after-refresh-still-open-screen.png → i6-reopened-after-normal-refresh-screen.png. These are inspected actual screen captures with a native full-SHA/synthetic banner.
The attention-row and keyboard failures this run recorded are fixed and re-proven in the section above. No movie-based claim or absolute network-containment claim is made. Merge this PR first; #4902's rebase remains after its merge.
Earlier appearance references (test-menu / optional local integration)
main)Each shot is a real
NSMenupopped on macOS by productionpopulateMenu, captured by window ID from a local test target (kit). Data is synthetic. This is not a packaged-app run: this Mac has no Xcode, so the app bundle can't be built here. Raw numbers: before, after.Changes
maindrops error-only providers from Overview. They now get one red row. Several collapse into one.Decisions for the maintainer
These change behavior for existing users, and each is a small revert:
SettingsStore). Detailed stays in Settings, and anyone who chose a layout keeps it.Tests
OverviewProviderLineTests: tightest-window selection in both percent styles, forecast and exhausted precedence, stale handling, sort stability, and height budgets on real menus (8 providers at most 500pt, the worst-case 16-provider roster at most 620pt, without spend data).CompactOverviewTests,SettingsStoreTests,StatusMenuOverviewSpendTests,StatusMenuTests,MergedOverviewAccountsTests,PreferencesPaneSmokeTests.StatusMenuScopedCodexRefreshTests"scoped refresh publishes compatible quota" fails under load in this harness, on untouchedmaintoo, and passes alone.make check: format, lint and locale checks pass, then it stops attest_packaged_app_launch.py, which fails identically onmainon this Mac. XCTest suites can't run here (no Xcode), so those rely on CI.🤖 Generated with Claude Code