Expose usage snapshot sharing from the menu overview - #3677
Conversation
|
🦞👀 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: dac96d99bf
ℹ️ 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".
| extension StatusItemController { | ||
| func makeOverviewShareStatsMenuItem(model: SpendDashboardModel) -> NSMenuItem? { | ||
| guard ShareStatsPayloadFactory.make(model: model, store: self.store) != nil else { return nil } | ||
| let title = L("Share Usage Snapshot…") |
There was a problem hiding this comment.
For every non-English locale, this menu item remains in English because Share Usage Snapshot… is absent from all Localizable.strings catalogs, so L can only fall back to the key; the failure alert at lines 185–187 also uses raw English literals. Add the new strings to the localization catalogs and localize the alert title/message so this newly exposed flow follows the selected app language.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. Reviewed September 18, 2026, 10:52 PM ET / September 19, 2026, 02:52 UTC (Revision 13). ClawSweeper reviewWhat this changesAdds usage snapshot sharing to Overview through the existing local preview, shares payload construction with Settings, and adds localized commands, source filtering, and clipboard feedback. Merge readiness✅ Ready for maintainer review The menu entry point remains a useful addition beyond current main. No actionable introduced defect was found, the earlier localization concern is resolved, and the supplied native proof supports the feature. Priority: P3 Review scores
Verification
How this fits togetherCodexBar collects provider usage into a shared Usage & Spend model. Overview filters that model and passes its aggregate data to a local preview that can copy or save an image. flowchart TD
A[Provider usage snapshots] --> B[Shared spend model]
C[Visible sources and reporting preferences] --> B
B --> D[Overview summary]
D --> E[Share command]
E --> F[Local snapshot preview]
F --> G[Clipboard or saved image]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep Overview and Settings on one local export path that honors source visibility, account ownership, reporting preferences, and truthful copy feedback. Do we have a high-confidence way to reproduce the issue? Not applicable as a feature request; current main lacks the Overview command, and supplied native screenshots demonstrate the resulting filtered preview. Is this the best way to solve the issue? Yes. Reusing the existing presenter and shared spend model avoids a separate export implementation while preserving current reporting preferences. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6e603f578647. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (12 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
clawsweeper
Bot
added
proof: sufficient
Localization catalogs conflicted because both sides appended new keys in the same region; all 23 keep both sets. The gatekeeper allowlist entry main added for PreferencesSpendDashboardPane.swift:519 is dropped: that construct moved into ShareStatsPayload.swift on this branch and already has its own entry, whose anchor shifts 361 -> 372 under the merged file.
…r task A failed image copy left "Could not copy image" on the button and on its accessibility label until the window was closed and reopened: only the success path scheduled a reset, and the preceding cancel() had killed any pending one. Both outcomes now share makeCopyFeedbackResetTask(). The presentation task's defer cleared whichever handle was stored rather than its own, so a task resuming from its 50ms sleep after being superseded nil'd its successor's handle and let a second presentation start concurrently. A generation token means a task only clears the handle it installed.
… value StatusItemController was already at exactly the 800-line type_body_length ceiling, so adding the generation counter as a second stored property tipped it to 801. Both now live in an OverviewSharePresentation value declared outside the class body, which restores the count to 800 and keeps the two fields together where their invariant lives.
|
Green and ready whenever you have a slot — ClawSweeper has this at "Ready for maintainer review" with no findings, and CI is 9/9 after merging current main. One note on ordering, since the diff looks larger than it is: of the 44 files, 7 are source. The rest are 23 localization catalogs (one key each), 11 proof artifacts under #3678 is stacked on this one, so its diff currently carries this PR's changes too — that's most of why it reads as 74 files. Landing this first shrinks #3678 to just the widget entry point, which should make it a much smaller read. |
Keep Overview and Usage & Spend exports on the same source-owned payload; prevent account history from borrowing another account plan label. Co-authored-by: Gabriel Pineda <Chipagosfinest@users.noreply.github.com>
Contributor attribution for Overview sharing: Alec Gutman, Chip; this supersedes the name recorded in the preceding integration commit. Co-authored-by: Alec Gutman, Chip <44984861+Chipagosfinest@users.noreply.github.com>
2 participants
Overview now offers Share Usage Snapshot…, using the same filtered Usage & Spend sources as its summary. Hidden accounts and providers stay excluded, including during initial loading, and an account's saved history cannot borrow a different account's subscription label.
The existing local preview is shared with Settings. Image copying verifies PNG/TIFF publication, reports success or failure truthfully, and respects Reduce Motion. Export stays local until the user chooses where to share it. Thanks @Chipagosfinest for the original contribution.
Validation:
make checkand the final full suite passed: 1,219 selections in 112 groups, with no retries or timeouts. All eight CI jobs passed at40acb895b1979bca5e6afbd5a114b43e863b3bee.Native preview, English:
Native preview, German: