Skip to content

fix(frontend): plot one savings series per axis with shared formatters - #1385

Merged
IAmJSD merged 7 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-749
Oct 24, 2026
Merged

IAmJSD merged 7 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-749

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

The Purchases savings chart drew period savings (left axis) and cumulative savings (right axis) in one plot with different scales, so line crossings meant nothing, and its ticks read $1500.00. savings-history.ts also had a private formatCurrency that shadowed the shared one, so its stat cards ($12.84K) did not match the rest of the page. This PR plots one series on one compact-currency axis, with a selector to switch to the cumulative view, and removes the local formatter.

Closes #749
Closes #171

What changed

  • frontend/src/utils.ts: new formatCompactCurrency(value, digits = 2) on Intl.NumberFormat compact notation ($1.5K, $12.35K, $1.23M), pinned to en-US. Absent values return --, the marker formatCurrency used at the main commit this branch was last merged with (003170d).
  • frontend/src/modules/savings-history.ts (43d1d5f):
    • The chart plots one series on one y-axis. "Savings per period" (default) is green bars; "Cumulative savings" is a filled blue line.
    • Y ticks use formatCompactCurrency; tooltips use formatCurrency with 2 decimals. The legend is hidden since there is one series and the axis title names it. The x-axis keeps its period-start anchor with offset: false.
    • The local formatCurrency is deleted. The Period / Avg / Peak stat cards use the shared formatCurrency(v, '$', 2) ($12,840.00, $428.00/mo).
  • frontend/src/index.html: a new "Show:" select (#savings-series) picks the series and reloads, with the same no-duplicate-listener pattern as the unit select.
  • Tests: utils.test.ts and savings-history.test.ts updated and extended. Main's fix(frontend): savings-analytics summary keys read by the client never exist on the wire #276 wire-key stats test now expects the shared formatter's $1,800.00 and $2,000.00/mo instead of the deleted compact formatter's $1.80K and $2.00K/mo; tests-e2e/savings-history-average.spec.ts expects full figures and switches to Cumulative and back in the narrow-screen test.

Why this approach

The issue suggests "bars on one compact-currency axis; cumulative as a separate small sparkline or a toggle". A toggle reuses the existing chart, needs no new layout and works on narrow screens, so it was chosen over a second sparkline canvas. Stat cards use full figures with cents (not compact) to match the Purchase History cards on the same page (the mismatch reported in #739); the compact form is used for axis ticks only. Deleting the local helper, rather than renaming it as #171 suggests, removes the shadowing and the duplicate formatter at the same time.

Screenshots

Screenshots were taken before this branch was merged with main; before = main at a14dc8f. After is this branch at that time. Both are headless Chromium at 1440x900 against the repo's mocked e2e API, with a 30-day fixture of varied daily savings.

Before After
chart-period before chart-period after
(no equivalent on main) chart-cumulative after
  • Period view: on main, period savings (left axis, $1500.00 ticks) and cumulative (right axis) share one plot, and the stat cards read $12.84K / $1.50K/mo. On this branch, period savings are bars on one compact axis ($1.5K), there is a new Show selector, and the stat cards read $12,840.00 / $1,500.00/mo.
  • Cumulative view (new): with Show = Cumulative savings, the running total is alone on its own single axis ($15K).

Verification

Run on this branch's head 7927c74c, merged with main 6122fa24:

  • npm run lint: 0 errors. Typecheck and build pass.
  • Jest: 4099 passed, 1 skipped, 0 failed. savings-history + utils: 188 passed.
  • New jest tests: formatCompactCurrency (abbreviation, distinct fractional ticks, absent marker); savings-history "one series on one axis (issue fix(frontend): purchases savings chart uses two y-axes and shadows formatCurrency #749)" (period view is one bar dataset on scales x,y only; cumulative view is one line dataset with the running total and no rate suffix; changing the series reloads once after a double init). The three y1 tests are removed and the tick, tooltip and QA 2.4/2.5 tests now target the single axis.
  • Playwright via e2e-local.sh: 231 passed, 0 failed.
  • Secrets scan clean.
  • Merged with main at 6122fa2; conflicts in savings-history.test.ts resolved, see the merge commits.

Notes for review

  • Design choices to weigh in on: a toggle instead of a sparkline for the cumulative series; the "Show:" label and the option text "Savings per period" / "Cumulative savings"; stat cards changing from compact ($12.84K) to full with cents ($12,840.00); the hidden legend.
  • The first and last bars are half-clipped at the axis edges, because the x-axis stays anchored to the exact period start and end (QA 2.2).
  • Not in scope: the Unit select still does not convert the cumulative series, which is the data defect tracked in chore(frontend/core): medium/low findings from the 2026-07-28 full review #134. The cumulative view drops the rate suffix so it does not claim a unit.
  • The chart canvas itself has no pixel test; it is checked in the screenshots.

Overlaps

Note for the UI owner

This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.

🤖 Generated with Claude Code

IAmJSD and others added 2 commits October 23, 2026 23:05
Chart axes need short labels ("$1.5K", "$12.35K", "$1.23M"). The
savings chart built them by hand with `$${(v / 1000).toFixed(1)}K`.
Add one shared helper on Intl.NumberFormat's compact notation, pinned
to en-US like formatCurrency and with the same absent-value marker, so
the chart can drop its own formatting (issue LeanerCloud#749).

Refs LeanerCloud#749

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ters

The Purchases savings chart drew period savings (left axis) and
cumulative savings (right axis) in one plot, so the points where the
lines crossed meant nothing. Its ticks read "$1500.00", and the module
had its own formatCurrency ("$1.50K") with the same name as the shared
helper in utils.ts, so the stat cards formatted money differently from
the rest of the Purchases page.

- Period savings are bars on a single y-axis with compact ticks
  (formatCompactCurrency: "$1.5K"). The running total is a separate view,
  chosen with a new Show: Savings per period / Cumulative savings select
  (#savings-series), on the same single axis.
- Tooltips and the Period / Avg / Peak stat cards use the shared
  formatCurrency with cents ("$12,840.00", "$428.00/mo"); the local
  formatCurrency is removed.
- The x-axis keeps its period-start anchor (QA 2.2) with the bar chart's
  default offset turned off.

Refs LeanerCloud#749

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-quarter Within the quarter impact/many Affects most users effort/s Hours type/bug Defect labels Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 6 billable files and costs up to $1.50.

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 049c617 and 55e4e29.

Files selected for processing: 6
  • frontend/src/__tests__/savings-history.test.ts
  • frontend/src/__tests__/utils.test.ts
  • frontend/src/index.html
  • frontend/src/modules/savings-history.ts
  • frontend/src/utils.ts
  • frontend/tests-e2e/savings-history-average.spec.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

IAmJSD and others added 2 commits October 24, 2026 17:46
Conflicts:
- frontend/src/__tests__/savings-history.test.ts: main (LeanerCloud#1341) replaced the
  "renders savings stats from summary" test, which fed the never-sent
  legacy summary keys, with a wire-key test and an "ignores the legacy keys"
  test. Took main's tests. Main's new wire-key test expected the deleted
  local compact formatter ('$1.80K', '$2.00K/mo'); its expectations now use
  this branch's shared formatCurrency output ('$1,800.00', '$2,000.00/mo').
  The rest of the file (this branch's single-series/axis tests and main's
  wire-key fixtures) merged cleanly.

savings-history.ts merged cleanly: main's server-total-first stats logic
now feeds this branch's shared formatCurrency(v, '$', 2) tiles.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No textual conflicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE 7927c74cc2517423600083e11684cf79b7694353

Blocking:

  • none
    Non-blocking:
  • frontend/src/modules/savings-history.ts:546: switching series re-fetches /history/analytics even though the data has not changed; it could re-render from the last response. There is no stale-series race, because getSelectedSeries() is read after the await.
  • frontend/src/tests/utils.test.ts:644: the new formatCompactCurrency describe is inserted between the "Regression tests for finding 11-N2" banner (line 642) and the describe it labels (line 664), so the banner now heads the wrong block.
  • frontend/src/modules/savings-history.ts:468: this chart's y-axis now uses compact ticks ($1.5K), while the Home charts use full formatCurrency ticks (dashboard.ts:1021,1170). That is two axis conventions in the app; fine for now.
    Attacked: Diffed origin/main...HEAD. Main still has no compact or other currency formatter besides utils.formatCurrency (CURRENCY_DEFAULT_DIGITS=0, so the explicit '$', 2 is needed). The local formatter is fully removed, and no other spec expects $x.xxK. Merge b8aa479: main's fix(frontend): read the savings-analytics summary keys the API sends #1341 wire-key tests are kept and only their two compact expectations changed, to $1,800.00 and $2,000.00/mo. A test-name diff against main shows only the intended y1 removals and renames. fix(frontend): read the savings-analytics summary keys the API sends #1341's server-total-first stats auto-merged and feed the shared formatter. 7927c74 merged cleanly, newest main f2ab673 touches nothing here, and merge-tree is clean. Checked the listener dedup on #savings-series, the null guards (?? 0) and the tooltip suffix for cumulative. Re-ran savings-history and utils jest: 188/188.

IAmJSD and others added 3 commits October 24, 2026 20:59
…urrency

Main's LeanerCloud#1376 (Refs LeanerCloud#739) changed formatCurrency's absent marker to
NO_VALUE ("—"); formatCompactCurrency still hardcoded "--". Delegate
null/NaN/Infinity to formatCurrency so the two cannot drift again.

Refs LeanerCloud#749

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/modules/savings-history.ts (imports): kept this branch's
  shared formatCompactCurrency/formatCurrency import and main's
  lib/chart-a11y import (LeanerCloud#1381).
- frontend/src/modules/savings-history.ts (renderSavingsChart): kept this
  branch's single-series dataset (period bars or cumulative line) and
  added main's LeanerCloud#744 summary label and "View as table" disclosure after
  it. Combined deliberately:
  * main built the table from periodSavingsData/cumulativeSavingsData,
    which this branch replaced with one `points` array, so the rows are
    now built from dataPoints directly (period converted with
    convertFromMonthly, cumulative as-is). The table keeps both columns
    in either view so the series toggle hides no values.
  * the period column header uses unitSuffix(unit), not the branch's
    view-dependent `suffix` (which is '' in the cumulative view).
  * main used the deleted local formatter; values now use the shared
    formatCurrency(v, '$', 2), giving the same "$25.00" main's test expects.
  * the summary names the drawn series ("Bar chart of savings per
    period" / "Line chart of cumulative savings") instead of main's
    fixed "Line chart of savings over time"; count and latest
    cumulative value are unchanged.
  formatCompactCurrency still delegates absent values to formatCurrency.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE 55e4e29d7d83a21cbad5cc787021bdeee78ea74d

Blocking:

@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 55e4e29d merged onto current main (e59d1438; merge-tree 9d663abe; Linux, Node, headless Chromium):

  • This branch's own checks passed at this head: lint, typecheck, build, the full jest suite, the full Playwright e2e suite, and the git-secrets checks (see Verification in the PR body).
  • The combined tree of every UI PR in this merge wave (tree 22a093f5, main plus 5 PRs) passes npm ci, lint, typecheck, build, jest (1 skipped, 4374 passed, 4375 total), and the full Playwright suite (259 passed).
  • CI is green on 55e4e29d (CI Success, Run pre-commit hooks).
  • The independent review above approves this exact head.
  • Nothing was applied to any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 24, 2026 23:00
@IAmJSD
IAmJSD merged commit effe0ba into LeanerCloud:main Oct 24, 2026
29 checks passed
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at 55e4e29d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

1 participant