Add a daily spend trend to the shared usage card - #3722
Open
Chipagosfinest wants to merge 10 commits into
Open
Chipagosfinest wants to merge 10 commits into
Chipagosfinest wants to merge 10 commits into
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. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 21, 2026, 3:21 AM ET / 07:21 UTC (Revision 4). ClawSweeper reviewWhat this changesAdds a daily spending sparkline to shared usage images, alongside the stacked spend-first layout, currency captions, tests, and comparison images. Merge readiness⛔ Blocked before merge - 5 items remain Keep open: the trend remains useful work absent from main. One earlier finding is fixed, but two coverage defects remain, and the supplied renders do not yet establish production behavior. Priority: P2 Review scores
Verification
How this fits togetherCodexBar builds shared usage cards from local provider spending summaries. The sharing builder supplies aggregate data to a SwiftUI preview and the PNG copy/save renderer. flowchart LR
A[Local provider history] --> B[Spending dashboard model]
B --> C[Coverage and daily totals]
C --> D[Shared card payload]
D --> E[Card preview]
D --> F[PNG copy or save]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Build the trend only from complete, covered daily summaries, preserving genuine zero days while suppressing unknown or partially priced series. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: production partial-cost summaries pass the new guard, and disjoint provider coverage reaches the zero-filling fallback. These are PR-head defects; no runtime reproduction was executed. Is this the best way to solve the issue? The existing dashboard-to-card pipeline is the right integration point, but the extraction must honor its partial-cost and coverage semantics instead of treating non-nil totals or missing points as complete observations. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ec12a0243d2a. LabelsLabel 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 (3 earlier review cycles)
|
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
5 tasks
Contributor
Author
|
Cross-link: #3747 restores retained top-model rows on the OG card when the USD group is incomplete. Independent of the sparkline; this branch can stay stacked on the spend-first layout. Live-checked on Homebrew 0.61.0. @clawsweeper related ranking fix on current main — not a replacement for this trend PR. |
…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".
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.
…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.
The card showed a total and a ranking but no movement. Every metric-card pattern worth copying pairs a headline value with its trend, and the data was already available: CurrencyGroup.dailyPoints carries per-provider cost per day, it just never reached the payload. ShareStatsPayload gains dailySpend: one value per covered day for the headline currency, summed across providers, oldest first. It is aggregate by construction, so it exposes nothing the rows do not already show. The sparkline sits in the hero right column, which was using only ~96pt of its 140pt, so the vertical budget is unchanged and the dense card still fits 630pt. It is labelled DAILY SPEND because it plots spend while sitting beneath the token count, and it is suppressed entirely for a flat or single-point series rather than drawing a shape the data does not support.
main added a required totalTokens field to SpendDashboardModel.DailySummary after this branch forked; rebase surfaced the missing argument at all six call sites in this file.
Chipagosfinest
force-pushed
the
feat/share-card-spend-trend
branch
from
September 21, 2026 07:17
6207ce8 to
d7c6a95
Compare
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
1 participant
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.
Stacked on #3704, which restructured the hero this builds on. Merge that first.
Why
The card showed a total and a ranking but no movement. Reviewing how metric cards are built
elsewhere, pairing a headline value with its trend is close to universal — PatternFly's dashboard
card taxonomy defines a "trend card" as a current value plus a sparkline of its change over time,
and the component libraries that implement this pattern (Neon
MetricCard, AuraDataCard, theshadcn stats blocks) all take a value plus an optional series. A total with no shape is the one
card type none of them ship.
The data was already there.
SpendDashboardModel.CurrencyGroup.dailyPointscarries per-providercost per day; it just never reached the payload.
Before / after
What it adds
ShareStatsPayload.dailySpend: [Double]— one value per covered day for the headline currency,summed across every provider on that day, oldest first. Aggregate by construction: it exposes
nothing the subscription rows do not already show, and carries no provider, model or account
identity, so the card's
LOCAL · AGGREGATE ONLYclaim still holds.The sparkline is a plain
Path— no charting dependency — with a filled gradient and a 2pt strokein the existing accent. No axes, no gridlines, no labels on the series itself: the hero already
states the period and the total.
Two deliberate constraints
The vertical budget does not move. The hero's right column was using roughly 96pt of its 140pt,
so the label and the 30pt sparkline fit in existing slack. The dense render above confirms the card
still fits 630pt with the footer clear — which matters because SwiftUI does not clip an oversize
frame.
It is suppressed rather than faked.
spendTrendreturns empty for a single point or a flatseries, so no sparkline is drawn when there is no shape in the data. Inventing one would imply
movement that is not there.
One correction worth recording
The first revision put the sparkline directly beneath
TRACKED TOKENSwith no label, which read asa token trend when it plots spend. It now carries a
DAILY SPENDeyebrow. The render is whatcaught it.
Tests
daily spend trend aggregates every provider per day in chronological orderbuilds aCurrencyGroupwhosedailyPointsare deliberately out of order, with two providers sharing themiddle day, and asserts
payload.dailySpend == [1, 5, 5]throughShareStatsBuilder.make— thereal builder, not a reimplementation of its logic in the test.
All 398
ProviderArchitectureGatekeeperTestsanchors re-verified; theShareStatsPayload.swiftMistral anchor shifts 198 → 203 because the new field sits above it, and is updated here.