Repository navigation
feat(menu): open a provider's full card in a hover flyout - #4902
Chipagosfinest wants to merge 3 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: c254d8b459
ℹ️ 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 func hostedSubviewIdentity(for menu: NSMenu) | ||
| -> HostedSubviewIdentity? { | ||
| // Its rows are built with the flyout, and "Show details" carries a row ID rather than a chart ID. | ||
| if menu is OverviewProviderDetailMenu, menu.items.first?.view != nil { return nil } |
There was a problem hiding this comment.
If a previously opened flyout is hovered again after a background refresh adds a quota window or changes provider details while Overview remains open, this early return leaves refreshHostedSubviewMenu doing only a height refresh. MenuCardRefreshMonitor.model cannot supply the new data because its hasCompatibleTrackedLayout check rejects those changes, and the root menu deliberately defers rebuilding during tracking. Consequently, the newly reopened full card still shows its original snapshot until the root menu is rebuilt. Retain the provider identity and refresh/rebuild the detail content on reopen, as the existing hosted chart submenus do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ccadfbc: the flyout menu keeps its provider, so hostedSubviewIdentity still resolves after hydration. Its render signature covers the current card model, so a reopen rebuilds only when the data changed, like the hosted chart submenus. Covered by a reopened flyout rebuilds only when its provider's data changed, which fails without the fix and also asserts no rebuild when nothing changed.
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: blocked before merge. What this changesThis PR replaces Compact Overview’s chart hover submenu with a full provider card containing a nested chart and Show details action. Example: Hover Claude in Compact Overview with Session 65% and Weekly 78% remaining.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 2 items remain Keep open: the earlier findings are resolved, but storage-only updates can leave the flyout stale, and the new hover interaction needs an owner decision. Priority: P3 Decision needed
Before merge
Findings
Agent review detailsHow this fits togetherThe menu controller turns cached provider models and storage footprints into Overview rows, hosted flyouts, charts, and provider-tab navigation. flowchart LR
A[Provider snapshots] --> C[Menu controller]
B[Storage footprints] --> C
C --> D[Compact Overview row]
D --> E[Full card flyout]
E --> F[Nested chart]
E --> G[Provider tab]
Technical reviewBest possible solution: Reuse the existing hosted-menu lifecycle, include separately rendered storage content in invalidation, and adopt the hover change only with owner agreement. Do we have a high-confidence way to reproduce the issue? Source shows that a storage-only update leaves the usage-model signature unchanged and skips rebuilding the captured storage text; this review did not execute target code. Is this the best way to solve the issue? Reusing full-card rendering and hosted submenus is appropriately scoped, but the reuse signature must cover all content captured by the flyout. Full review comments:
Overall correctness: patch is incorrect 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, 12:55 AM ET / 05:55 UTC (Revision 4). |
Keep the provider on the flyout menu so reopening compares the current card model against what was rendered and rebuilds a stale card, the same way hosted chart submenus do. 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
clawsweeper:intake:feature
clawsweeper
Bot
added
the
rating: 🦪 silver shellfish
A real click cleared the native highlight, which resumed a flyout rebuild queued when the nested chart closed. That rebuild detached the clicked item before AppKit sent its action, so the selector saw no menu and returned. Mouse-up no longer resumes the deferred rebuild, and the action falls back to the merged menu if another rebuild detached its item. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:feature
Part of a four-PR Overview redesign, each standalone on
main: #4899 rows, #4900 footer, #4901 chart, #4902 flyout. Rebase needed after #4899: see below.Hovering a row in compact Overview opens the provider's full card beside the menu, with its chart one level further in. A click still opens the provider's tab, and the parent menu does not move, so detail can't push the footer away. Detailed keeps its chart submenu.
Packaged-app interaction proof (2026-11-02 UTC)
Actual-app run · screenshots, recording, checks, saved defaults and hashes. Product
0ff1e04c1043ebb549dd0df28f34884141fd3b90, unmodified release package; installed binary matches the package. Real CGEvent pointer input. Synthetic fixture data; saved Compact with three selected providers on 1024×768. No real accounts or Keychain prompts. 18 of 18 checks pass. This is the standalone head, before #4899's one-line rows.Show details, the same click before and after the fix:
ccadfbc33, run)0ff1e04c1)mergedMenuLastSelectedWasOverviewtruefalse,selectedMenuProvider=claudeThe after-run passes both routes: straight to the item, and after visiting the nested chart. Recording · checks.json.
The rest still holds on this head:
Root cause, from a lifecycle log on a temporary instrumented build (run), the same in 3 of 3 clicks. Passing over Plan Usage opens and closes its chart, which queues a flyout rebuild that waits on the native highlight. Mouse-up clears the highlight, so the rebuild ran and
removeAllItems()detached the clicked item. AppKit then still sent the action, withsender.menu == nil, andselectOverviewProvider(_:)returned without selecting.Fix: mouse-up no longer resumes the deferred rebuild, since the menu is closing and the next open rebuilds anyway. The action also falls back to the merged menu, for a data refresh landing in the same gap, and still selects through
selectOverviewProvider.The same log shows the flyout rebuilding on every open with the fixture's reset countdowns, even without new provider data. I think the countdown text changes its signature; I did not confirm that. It is unchanged here and did not affect these results.
Screenshots retain the native full-SHA/synthetic banner. No post-#4899 integration claim is made. The rebase stays pending until #4899 merges.
Earlier appearance references (test-menu / optional local integration)
main): chartEach 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. The flyout was opened by warping the cursor onto the row and posting a mouse-moved event, and its window was then present beside the menu.Combined with the other three PRs (local merge, not on any branch), on the one-line rows:
Optional comparison outside this PR head: combined local-integration capture.
Known limits
main's full card. It fits this display but is taller than the redesign's target of about 485pt, which needs a separate denser-card change that is not in this series.After #4899 merges
This wires the flyout into
main's row loop. #4899 adds a new row path, so one line must move there, and row tests must go through the flyout's "Show details". The exact change was verified on the local merge: 565 tests in 43 suites, 1 failure (the same flake below).Tests
OverviewProviderFlyoutTests: the flyout carries the detail, reopening never rebuilds it as the root menu (the type marks it hosted), the parent keeps its items, "Show details" opens the tab through the parent, Detailed keeps its chart.sendActionfrom the item), the other sends the action from an item a rebuild already detached. Both fail without the fix and pass with it (6 of 6 in the suite locally; the full suite runs in CI).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