fix(dashboard): fix Home savings 'All' range and label chart tooltip date - #1257
Conversation
…date QA 3.1: The 'All' timeframe sent '1970-01-01T00:00:00Z' as the start param, triggering HTTP 400 "date range too large" from the analytics handler (which caps at 366 days). The catch block rendered all errors as "Savings history is not available yet." Replace the epoch sentinel with a rolling ~365-day window (now - 365d) that fits the backend cap. The existing axisMinMs anchor-to-earliest-point logic is preserved. QA 3.2: The tooltip had no title callback, so Chart.js rendered the raw 13-digit millisecond x value as the tooltip header. Add a title callback that formats the x timestamp via the existing formatTrendAxisTick helper. Closes #1248
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…test mock Adversarial-review follow-ups to PR #1257: - frontend/src/dashboard.ts: the loadSavingsTrendChart docstring still described the pre-QA-2.3 "empty windows render labelled axes" behaviour; the shipped code (lines 1083-1098) shows a filter-aware empty-state banner instead. Reconciled the docstring to describe the actual three branches (data / empty / fetch-error). - frontend/src/__tests__/dashboard.test.ts: the new "All range sends ~365d start" test set mockResolvedValue twice with a mockClear() in between. mockClear() resets call history but NOT mock return values, so the first set was dead code. Collapsed to a single mockResolvedValue + mockClear with a clarifying comment. 97 dashboard tests pass, tsc clean. No behaviour change.
Adversarial-review passRan the playbook against the QA 3.1 / QA 3.2 fixes plus the explicit risk surfaces (All semantics, stale docstring memory hit, tooltip date format, empty/zero state, timezone, range cross-product, tests). In-scope fixes pushed (8008bc0)
Confirmed clean
Out-of-scope follow-ups filed
Risk surfaces from the brief, checked
Re-pinging CR. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Closes #1248
Symptoms
Root cause
QA 3.1:
loadSavingsTrendChartsentstart=1970-01-01T00:00:00Z(an epoch sentinel) when the "All" range was selected. The backend analytics handler (internal/api/handler_analytics.go) caps the date range at 366 days and returns HTTP 400 "date range too large" for a 1970 start. Thecatchblock renders the generic empty-state message for all errors, so the 400 surfaced as "Savings history is not available yet."QA 3.2: The chart x-axis is
type: 'linear'with data points{x: timestampMs, y: value}. The tooltip had alabelcallback but notitlecallback, so Chart.js rendered the raw x value (milliseconds) as the tooltip header.Fix
now - 365 * 86400_000) that fits inside the backend's 366-day cap. The existingaxisMinMsanchor-to-earliest-point logic is preserved so sparse data still fills the chart width correctly. The backend 366-day cap (a deliberate DoS guard) is unchanged.titlecallback to the tooltip that formats the x timestamp using the existingformatTrendAxisTickhelper (already used for x-axis tick labels).Testing
startwithin ~365 days of now (not 1970), and that the chart renders rather than showing the empty-state when the API returns data.titlecallback returns a formatted date string (not a bare 13-digit number) for a given x timestamp.npx jest src/__tests__/dashboard.test.ts: 97 pass, 0 fail, 1 skipped.npx tsc --noEmit: no errors.Stacked on #1254 (pre-commit repair); retarget to main when #1254 merges.
Summary by CodeRabbit