Lead the shared usage card with spend - #3704
Chipagosfinest wants to merge 9 commits into
Conversation
…al disclaimer The shared snapshot set the token count at 104pt and the spend figure at 32pt, so the least meaningful number was the hero and the reason the card exists was secondary. The two hero columns were also spaced independently, leaving their labels on different baselines. Spend now leads, tokens support it, and both columns run the same label / value / detail rhythm. The card said it was local and aggregate four separate times; the LOCAL SNAPSHOT badge and the aggregate sentence are gone and the footer keeps the claim. The rankings block no longer reserves a fixed 286pt, so a sparse snapshot centres instead of leaving a void above the footer, and the empty model column reads as a row rather than an apology. Also fixes a plural bug: the most common card there is rendered "1 subscriptions".
|
🦞👀 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: needs changes before merge. Reviewed September 21, 2026, 7:08 PM ET / 23:08 UTC (Revision 10). ClawSweeper reviewWhat this changesThe PR makes estimated spend the shared card’s headline, simplifies supporting text, and adds currency-formatting coverage and native layout renders. Merge readiness⛔ Needs changes before merge - 2 items remain The presentation proposal remains distinct from the merged caption fix. The new production-render images resolve the earlier visual-proof gap; one minor documentation defect remains. Priority: P3 Review scores
Verification
How this fits togetherCodexBar turns aggregate provider usage and spend into a shareable image. The changed card view consumes that prepared data and feeds the native PNG renderer used by image export and clipboard copying. flowchart LR
A[Provider usage and spend] --> B[Aggregate sharing data]
B --> C[Currency captions]
B --> D[Shared card layout]
C --> D
D --> E[Native PNG renderer]
E --> F[Save image or copy image]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the presentation change focused on the existing card and renderer, with working links to its inspected native output. Do we have a high-confidence way to reproduce the issue? Yes, for the remaining documentation defect: resolving the three relative image URLs against the document directory points to nonexistent files. The layout proposal itself is not a reported runtime bug. Is this the best way to solve the issue? Yes: reusing the existing card and native exporter is a focused approach, and the inspected renders support the layout; only the proof-document paths need correction. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b99a91694d78. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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 (9 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Dropping the fixed rankings reservation removed the only guarantee that a dense card fits 630pt. SwiftUI does not clip an oversize frame by default, so five provider rows plus three model rows could have pushed the footer out of the image. The reservation is back; the void this PR set out to fix came from content hugging the top of that block, not from the block itself, so it now centres inside it. The 104pt slot previously held a compact token count (never more than six characters, so it never scaled). It now holds a currency string: 654pt of available width cannot fit "~$123,456.78" at 104pt, which would have silently scaled the headline down. 76pt fits twelve characters unscaled and still reads as dominant over the 42pt token count. The coverage line is pinned to one line now that it carries explicit secondary-currency totals.
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Correct the one-subscription caption while preserving the existing card layout, currency overflow text, totals and reporting date. Reuse the contributor's pluralization helper and cover zero, one and multiple subscription counts. Related to #3704. Validated with 58 sharing/architecture tests, synthetic before/after production renders, make check and clean P2 review. Co-authored-by: Alec Gutman <44984861+Chipagosfinest@users.noreply.github.com>
|
The singular subscription caption correction has landed in #3706, with 58 focused tests, green macOS/Linux CI, and inspected before/after production renders. The card now says “1 subscription” for a single included subscription. This PR remains open for the larger presentation proposal. Thanks for contributing the formatter and identifying the caption issue. |
steipete extracted the plural fix from this PR and shipped it as steipete#3706, so the helper and its test now come from main; our duplicates are dropped. What remains here is the layout work: spend leads the hero, the local/aggregate disclaimer is stated once, the rankings block centres inside its reservation, and secondary currency totals stay explicit via spendCoverage.
…d badge Rendering the card at 1200x630 across sparse, dense and multi-currency payloads showed two problems with earlier revisions of this branch. Centring the rankings inside its reservation split the empty space on a sparse card and left a gap directly under the divider, reading as a layout fault rather than as absent content. Top alignment keeps one contiguous region below the content. Replacing the LOCAL SNAPSHOT badge with the period label put "30 DAYS" directly above "EST. 30-DAY SPEND". The badge is removed instead, since the footer already carries the local/aggregate claim and the spend label already carries the period. periodLabel is now unused and removed with it. Renders and method are recorded in docs/research/share-card-layout.md.
|
Cross-link: #3747 keeps retained top-model families on the shared card when a sibling USD provider is unpriced (the empty TOP MODELS column on current 0.61.0). Independent of this spend-first layout. Verified against installed 0.61.0. @clawsweeper this is a related ranking fix, not a substitute for the layout proof here. |
… path ClawSweeper flagged the existing layout proof (docs/research/share-card-layout.md) as a standalone ImageRenderer harness, not the app's actual exporter. Add ShareStatsLayoutProductionRenderTests, which renders the same sparse/dense/multi payloads through ShareStatsRenderer.pngData — the literal NSHostingView path ShareStatsExporter.saveImage/copyImage use in production — and commit the resulting PNGs.
|
Added production-path proof: @clawsweeper re-review — this should close the outstanding proof gap. |
|
🦞👀 Re-review progress:
|
ClawSweeper caught it: the links used assets/production-*.png, which resolves relative to docs/research/ (where the doc lives), but the committed images are at docs/research/share-card-layout/assets/.
|
Fixed the broken image links — @clawsweeper re-review |
Scope note: the singular-caption fix that started here landed separately in #3706, so this PR is now
purely the presentation change.
Before / after
Sparse card — 1 subscription, no model history:
Dense card — 5 subscriptions, 3 models,
~$123,456.78:Multi-currency — 3 currencies:
What changed
HStack, same label / value / detail rhythmLOCAL SNAPSHOT(duplicated the footer)ShareStatsFormatting.spendCoverageCard size stays
1200x630— it is an OG image and the ratio is load-bearing.What the renders settled
Dense fits. The
286ptrankings reservation is load-bearing. An earlier revision of this branchremoved it, and since SwiftUI does not clip an oversize frame, five provider rows plus three model
rows would have pushed the footer out of the image. Restored, alignment only.
The hero no longer scales.
~$123,456.78renders unscaled at 76pt. At the original 104pt itneeded roughly 660-780pt against 654pt of available width (
1200 - 104 padding - 52 spacing - 390 right column), so the headline would have silently shrunk on realistic values. The 104pt slotpreviously held
compactCountoutput, at most six characters, so it never exercisedminimumScaleFactor.Two of my own earlier choices were wrong, and the renders caught both. Centring the rankings
split the sparse card's empty space and put a gap under the divider that reads as a fault; top
alignment leaves one contiguous region that reads as absent content. And replacing
LOCAL SNAPSHOTwith the period label put
30 DAYSdirectly aboveEST. 30-DAY SPEND— one redundancy traded foranother, so the badge is gone entirely.
Method, and its limit
ShareStatsCardViewis a pure function ofShareStatsPayload, so these were produced by compilingthe view verbatim alongside the payload types and
ShareStatsFormattingfrom each revision andrendering through
ImageRendererat the card's own size. OnlyUsageFormatter.currencyStringandSpendDashboardSource.scanDaysare stubbed, neither of which affects layout. Method and payloads arerecorded in
docs/research/share-card-layout.md.These are not production-bundle renders — the app was not built for them. The view code is
identical to this branch so layout and typography are faithful, but if you want output from a signed
build before merging, say so and I will produce it.
Tests
spend coverage preserves secondary currency when subscriptions overflow— single, dual andtriple-currency cases pinning the exact coverage string.
ProviderArchitectureGatekeeperTestsline anchors re-verified: zero stale.