Repository navigation
Conversation
…sponses
The Home per-service and "Savings over time" charts and the Purchases
savings-history chart were destroyed and recreated on every filter,
range, period or unit change, so Chart.js replayed its grow-from-zero
entry animation each time. A new upsertChart helper
(modules/chart-utils.ts) swaps the data and options into the existing
instance and redraws with update('none'); it only creates a chart on
first render or when the canvas was replaced, and the empty states
still destroy the chart.
loadSavingsTrendChart had no request-generation guard, so clicking one
range and then another could let the older, slower response paint last
and show its data under the newer range button. It now takes a
generation number per call and ignores responses (and errors) from
superseded calls, the same pattern riexchange.ts uses.
The ladder timeline item from the issue was already fixed in LeanerCloud#823, which
updates that chart in place.
Refs LeanerCloud#747
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Conflicts: - frontend/src/dashboard.ts (import): main (LeanerCloud#1323) added formatTrendTooltipTitle to the chart-utils import, this branch added upsertChart. Import both. - frontend/src/dashboard.ts (renderSavingsByService): main (LeanerCloud#1335) dropped the per-service bgColors/darken/upside shade block in favour of the fixed SAVINGS_LAYER_COLORS; this branch replaced new Chart(...) with upsertChart(...). Took main's layer colours and kept upsertChart, so the per-service chart still updates in place. - frontend/src/modules/chart-utils.ts: both sides appended a helper after formatTrendAxisTick (main's formatTrendTooltipTitle, this branch's upsertChart). Kept both, main's first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After merging main, webpack's ts-loader rejected trends.ts (new on main) with TS2322 assigning Chart<'bar', (number | null)[], string> to the default Chart type. tsc --noEmit still passed: Chart.js's generic variance check depends on the order files are checked in, and adding upsertChart (which takes the default ChartConfiguration) to chart-utils changed that order for ts-loader. Declaring trendsChart with the exact type it holds removes the cross-type assignment. Refs LeanerCloud#747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - frontend/src/modules/chart-utils.ts: import block only; kept this branch's chart.js import (for upsertChart) and main's formatCurrency import (LeanerCloud#1376). - frontend/src/dashboard.ts: chart-utils import line only; import both main's formatSparseTrendMessage/MIN_TREND_POINTS (LeanerCloud#1334) and this branch's upsertChart. The sparse-data early return from main sits before the upsertChart call and destroys the chart like the empty state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No conflicts (picks up LeanerCloud#1387, which main gained after the previous merge). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's LeanerCloud#1334 (issue LeanerCloud#188) shows a sparse-data message instead of a chart when the trend has fewer than MIN_TREND_POINTS (2) points. The stale-response and update-in-place tests used one point, so no chart was drawn. Use two points so they still exercise the chart path. Refs LeanerCloud#747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
The Home "Savings over time" and per-service charts and the Purchases savings-history chart were destroyed and recreated on every range, filter, period or unit change, so the grow-from-zero animation replayed each time. The Home trend loader also had no request-generation guard: clicking 7d then 30d could let the slower 7d response paint last, under the highlighted 30d button. This PR updates those charts in place and ignores superseded trend responses.
Closes #747
What changed
frontend/src/modules/chart-utils.ts: newupsertChart(existing, canvas, config). If the existing instance still draws on the same canvas, it setsdata/optionsand callsupdate('none'); otherwise it destroys any stale instance and creates a new one.frontend/src/dashboard.ts: both Home charts useupsertChart(the per-service chart now destroys only in its empty state).loadSavingsTrendCharttakes a generation number per call and returns early after theawait(and in thecatch) when a newer call has started.frontend/src/modules/savings-history.ts: the Purchases chart usesupsertChart.6c4be959(fix(ladder): show cumulative commitment as a stepped stacked chart #823), which updates that chart in place (frontend/src/ladder-timeline.ts:97).__tests__/chart-upsert.test.tsand 4 new cases in__tests__/dashboard.test.ts.Why this approach
Updating in place is what the issue suggests and keeps one Chart instance per canvas; the canvas-identity check keeps it safe if a view re-renders its DOM.
update('none')skips the transition entirely; a short tween from old to new values would also stop the replay but isn't what the issue asks for. The generation counter copies the existingriexchange.tspattern rather than addingAbortControllerplumbing to the API client.Screenshots
Before is main (
a14dc8f2), after is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API.Verification
Run on this branch's head
a18ba161on top of maina14dc8f2:chart-upsert.test.ts(3: first render creates; same canvas updates in place with'none'and no destroy; replaced canvas recreates) and 4 indashboard.test.ts(an older trend response landing last is ignored; an older request failing last doesn't show the error state; a second trend load updates in place; a per-service re-render updates in place while the empty state still destroys). New tests pass with--randomize.e2e-local.sh: 157 passed. No new spec: the race and the animation are asserted in jest with deferred promises and a Chart mock, and the screenshots reproduce both in the real build.scripts/test-git-secrets-allowlist.sh: 137 pass, 0 fail.Notes for review
update('none'), range and filter changes now redraw instantly with no animation at all; the first render still animates.Overlaps
dashboard.tsandsavings-history.ts. Expect small conflicts; after combining, the table/label code should stay beforeupsertChart.loadDashboard/renderDashboardSummary.Those PRs will be opened alongside this one and merged in sequence, with conflicts resolved on the later branch.
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