fix(home/chart): x-axis spans selected timeframe window (QA 3.1) - #746
Merged
Merged
Conversation
cristim
added
triaged
Contributor
|
Warning Review limit reached
More reviews will be available in 52 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds exported formatTrendAxisTick() and refactors loadSavingsTrendChart() to compute explicit time bounds, map analytics into timestamped {x,y} points, use a linear x-axis with formatted ticks, hide the canvas on empty/error responses, and include tests validating formatting, mapping, axis windowing, empty/error UI, and account_ids forwarding. ChangesSavings-trend chart axis formatting and data positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
The Savings-over-time chart on the Home page snapped the x-axis to the single purchase date regardless of the selected timeframe (7d/30d/ 90d/All). Now the x-axis always spans [now - window, now], purchases are positioned by their actual date within that window, and empty windows render labelled axes instead of a "No purchase history" stub. Refs QA row 405, step 3.1.
The rebase onto origin/feat/multicloud-web-frontend (which merged the buildTrendFilterDesc removal in PR #747) left an orphaned closing brace at frontend/src/dashboard.ts:624 that TypeScript rejected. Drop the brace so the file compiles cleanly on the rebased base.
cristim
force-pushed
the
fix/qa-savings-chart-axis
branch
from
May 27, 2026 14:53
31b8c4a to
406f837
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/dashboard.test.ts`:
- Around line 771-783: The test suite contains conflicting expectations about
how the savings-trend UI should behave on empty data: the 'renders chart with
visible canvas even when there are no data points (QA 3.1)' test (uses
loadSavingsTrendChart and mocks api.getSavingsAnalytics to return { data_points:
[] }) expects the canvas '`#savings-trend-chart`' to be visible and the empty stub
'`#savings-trend-empty`' to be hidden; update the other test(s) that assert the
opposite (the assertions around '`#savings-trend-empty`' and
'`#savings-trend-chart`' later in the file) to match this single behavior so both
tests assert the canvas is visible, the empty stub is hidden, and Chart is
instantiated (expect(Chart).toHaveBeenCalled()) when api.getSavingsAnalytics
returns no data points.
In `@frontend/src/dashboard.ts`:
- Around line 642-656: The code forces 'all' to 365 days by setting days when
savingsTrendRange === 'all', so the start sent to api.getSavingsAnalytics (new
Date(windowStartMs).toISOString()) never goes earlier than ~365 days; change the
logic so when savingsTrendRange === 'all' you compute windowStartMs from the
true earliest history instead of using days: omit the days assignment for 'all',
fetch or derive an earliestPurchaseMs (e.g., from state or an API call using the
same accountIDs from state.getCurrentAccountIDs()), set windowStartMs =
earliestPurchaseMs (with a fallback to nowMs - 365d), keep the interval
selection for non-'all' ranges, and ensure the start passed to
api.getSavingsAnalytics uses this computed windowStartMs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb2b921c-4593-4298-b7f2-ecf3b585d719
📒 Files selected for processing (2)
frontend/src/__tests__/dashboard.test.tsfrontend/src/dashboard.ts
…e 365d cap Finding 1 (conflicting empty-state assertions): the QA 3.1 test at line 779 asserted that a successful-but-empty response hides #savings-trend-empty (show blank axes), while the QA 2.3 tests at lines 981-995 asserted the opposite (show a friendly empty-state message). QA 2.3 is the more-recent, intentional policy: show the empty-state with account context when a filter is active, and a generic "No purchase history yet" message when no filter is active. Production code updated to implement this policy with an early-return on empty data_points; QA 3.1 test updated and annotated to match. Finding 2 (All-range 365d cap): the 'all' range was hardcoded to 365 days both in the days computation and in the API start param, silently truncating history older than a year. Changed to 3650 days (10-year sentinel that parseDateRange accepts as a valid RFC3339 start), which lets the backend return the full available history. The x-axis interval for 'all' is now 'weekly' (appropriate for a multi-year span). Finding 3 (chart/KPI filter parity): loadSavingsTrendChart previously passed account_ids only when exactly one account was selected, causing the chart to show all-accounts data while the KPI tiles above it filtered to the selected subset. Changed the guard from `=== 1` to `> 0` so any non-empty selection is forwarded. Added a regression test asserting that multi-account filters are passed through. Provider filtering is not yet forwarded (backend analytics handler has no provider param); this is documented in a comment.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/dashboard.ts (1)
642-650:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win"All" range still caps history to 10 years via the
startparameter.The
days = 3650sentinel improves on the previous 365-day limit, but Line 662 still sendsstart: new Date(windowStartMs).toISOString()to the API, which means purchase history older than 10 years will never be fetched. The axis-override logic at Lines 680–686 only adjusts the chart display, not the data query.To support truly unbounded history for "All", consider omitting the
startparameter entirely whenisAllRangeis true, so the backend can return the full available history.💡 Suggested fix
const accountIDs = state.getCurrentAccountIDs(); const data = await api.getSavingsAnalytics({ - start: new Date(windowStartMs).toISOString(), + ...(isAllRange ? {} : { start: new Date(windowStartMs).toISOString() }), end: now.toISOString(), interval, ...(accountIDs.length > 0 ? { account_ids: accountIDs } : {}), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/dashboard.ts` around lines 642 - 650, The "All" range still limits data because we always send start: new Date(windowStartMs).toISOString() — update the request-building logic that uses savingsTrendRange/isAllRange (and variables days/windowStartMs) so that when isAllRange is true you omit the start parameter entirely from the API query (instead of sending the 3650-day sentinel); keep the axis override that adjusts windowStartMs for display but ensure parseDateRange/ request creation skips adding start for true "all" to let the backend return full history.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/dashboard.ts`:
- Around line 705-712: The empty-state text incorrectly implies results were
filtered by provider because provider (from state.getCurrentProvider()) is not
passed to api.getSavingsAnalytics; update the logic around provider usage in the
block that sets empty.textContent so it does not claim "No savings history for
${provider}." Instead either remove the provider-specific message and fall back
to a generic "No savings history yet." or change it to a clarifying message such
as "No savings history yet (provider filter not applied)." Ensure you update the
code that reads provider (state.getCurrentProvider()) and the branch that sets
empty.textContent to use the chosen generic/clarifying string.
---
Duplicate comments:
In `@frontend/src/dashboard.ts`:
- Around line 642-650: The "All" range still limits data because we always send
start: new Date(windowStartMs).toISOString() — update the request-building logic
that uses savingsTrendRange/isAllRange (and variables days/windowStartMs) so
that when isAllRange is true you omit the start parameter entirely from the API
query (instead of sending the 3650-day sentinel); keep the axis override that
adjusts windowStartMs for display but ensure parseDateRange/ request creation
skips adding start for true "all" to let the backend return full history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d203f758-505e-4c6f-8889-9276e905a10a
📒 Files selected for processing (2)
frontend/src/__tests__/dashboard.test.tsfrontend/src/dashboard.ts
…All range Finding 1: the empty-state message showed "No savings history for <provider>" when a provider filter was active, but the analytics endpoint does not accept a provider param yet (tracked in #764), so the query always returns all-provider data. Mentioning the provider implied a scope that was never applied. Drop the provider branch; show "No purchase history yet." until #764 lands. Add inline comment and regression test. Finding 2: the All range capped history at 10 years by computing now - 3650d as the start param. The backend parseDateRange defaults a missing start to (end - 7d), so we cannot omit it — instead send the Unix epoch sentinel "1970-01-01T00:00:00Z" to get truly unbounded history. Add regression test asserting the epoch constant reaches the API.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
… y-axis ticks
QA 2.2 -- x-axis starts at period start, not first data point:
Convert datasets from scalar arrays to {x: timestamp_ms, y: value}
objects and switch the x-axis to type:'linear' with min/max anchored
to the selected period window. Mirrors the Home dashboard approach
from PR #746. Uses the shared formatTrendAxisTick helper from
chart-utils.ts for tick labels.
QA 2.3 -- Period Savings tooltip precision matches Cumulative:
Change toFixed(4) to toFixed(2) for the Period Savings tooltip label
so both series and the KPI box above the chart show 2 decimal places.
QA 2.4/2.5 -- y-axis ticks stable when toggling series:
Add maxTicksLimit:6 to both y and y1 axis tick configs to cap
re-autoscaling on legend toggle. Fix the y1 formatter to emit 2
decimal places for non-integer float ticks, preventing distinct float
values from collapsing to the same integer label string.
Regression tests added in savings-history.test.ts:
- QA 2.2: x-axis type:linear with numeric min equal to period start
- QA 2.3: Period Savings tooltip has exactly 2 decimal places
- QA 2.4: both y-axes have maxTicksLimit
- QA 2.5: y1 formatter does not collapse distinct floats to same label
Closes #1252
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
… y-axis ticks
QA 2.2 -- x-axis starts at period start, not first data point:
Convert datasets from scalar arrays to {x: timestamp_ms, y: value}
objects and switch the x-axis to type:'linear' with min/max anchored
to the selected period window. Mirrors the Home dashboard approach
from PR #746. Uses the shared formatTrendAxisTick helper from
chart-utils.ts for tick labels.
QA 2.3 -- Period Savings tooltip precision matches Cumulative:
Change toFixed(4) to toFixed(2) for the Period Savings tooltip label
so both series and the KPI box above the chart show 2 decimal places.
QA 2.4/2.5 -- y-axis ticks stable when toggling series:
Add maxTicksLimit:6 to both y and y1 axis tick configs to cap
re-autoscaling on legend toggle. Fix the y1 formatter to emit 2
decimal places for non-integer float ticks, preventing distinct float
values from collapsing to the same integer label string.
Regression tests added in savings-history.test.ts:
- QA 2.2: x-axis type:linear with numeric min equal to period start
- QA 2.3: Period Savings tooltip has exactly 2 decimal places
- QA 2.4: both y-axes have maxTicksLimit
- QA 2.5: y1 formatter does not collapse distinct floats to same label
Closes #1252
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… y-axis ticks
QA 2.2 -- x-axis starts at period start, not first data point:
Convert datasets from scalar arrays to {x: timestamp_ms, y: value}
objects and switch the x-axis to type:'linear' with min/max anchored
to the selected period window. Mirrors the Home dashboard approach
from PR #746. Uses the shared formatTrendAxisTick helper from
chart-utils.ts for tick labels.
QA 2.3 -- Period Savings tooltip precision matches Cumulative:
Change toFixed(4) to toFixed(2) for the Period Savings tooltip label
so both series and the KPI box above the chart show 2 decimal places.
QA 2.4/2.5 -- y-axis ticks stable when toggling series:
Add maxTicksLimit:6 to both y and y1 axis tick configs to cap
re-autoscaling on legend toggle. Fix the y1 formatter to emit 2
decimal places for non-integer float ticks, preventing distinct float
values from collapsing to the same integer label string.
Regression tests added in savings-history.test.ts:
- QA 2.2: x-axis type:linear with numeric min equal to period start
- QA 2.3: Period Savings tooltip has exactly 2 decimal places
- QA 2.4: both y-axes have maxTicksLimit
- QA 2.5: y1 formatter does not collapse distinct floats to same label
Closes #1252
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… y-axis ticks
QA 2.2 -- x-axis starts at period start, not first data point:
Convert datasets from scalar arrays to {x: timestamp_ms, y: value}
objects and switch the x-axis to type:'linear' with min/max anchored
to the selected period window. Mirrors the Home dashboard approach
from PR #746. Uses the shared formatTrendAxisTick helper from
chart-utils.ts for tick labels.
QA 2.3 -- Period Savings tooltip precision matches Cumulative:
Change toFixed(4) to toFixed(2) for the Period Savings tooltip label
so both series and the KPI box above the chart show 2 decimal places.
QA 2.4/2.5 -- y-axis ticks stable when toggling series:
Add maxTicksLimit:6 to both y and y1 axis tick configs to cap
re-autoscaling on legend toggle. Fix the y1 formatter to emit 2
decimal places for non-integer float ticks, preventing distinct float
values from collapsing to the same integer label string.
Regression tests added in savings-history.test.ts:
- QA 2.2: x-axis type:linear with numeric min equal to period start
- QA 2.3: Period Savings tooltip has exactly 2 decimal places
- QA 2.4: both y-axes have maxTicksLimit
- QA 2.5: y1 formatter does not collapse distinct floats to same label
Closes #1252
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… y-axis ticks
QA 2.2 -- x-axis starts at period start, not first data point:
Convert datasets from scalar arrays to {x: timestamp_ms, y: value}
objects and switch the x-axis to type:'linear' with min/max anchored
to the selected period window. Mirrors the Home dashboard approach
from PR #746. Uses the shared formatTrendAxisTick helper from
chart-utils.ts for tick labels.
QA 2.3 -- Period Savings tooltip precision matches Cumulative:
Change toFixed(4) to toFixed(2) for the Period Savings tooltip label
so both series and the KPI box above the chart show 2 decimal places.
QA 2.4/2.5 -- y-axis ticks stable when toggling series:
Add maxTicksLimit:6 to both y and y1 axis tick configs to cap
re-autoscaling on legend toggle. Fix the y1 formatter to emit 2
decimal places for non-integer float ticks, preventing distinct float
values from collapsing to the same integer label string.
Regression tests added in savings-history.test.ts:
- QA 2.2: x-axis type:linear with numeric min equal to period start
- QA 2.3: Period Savings tooltip has exactly 2 decimal places
- QA 2.4: both y-axes have maxTicksLimit
- QA 2.5: y1 formatter does not collapse distinct floats to same label
Closes #1252
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… y-axis ticks (#1253) * refactor(chart): extract formatTrendAxisTick to shared chart-utils module Move formatTrendAxisTick from dashboard.ts into frontend/src/modules/chart-utils.ts so it can be reused by the Purchases Savings History chart. Re-export from dashboard.ts for backward compatibility with existing tests and callers. * fix(purchases): correct Savings History chart axis, tooltip decimals, y-axis ticks QA 2.2 -- x-axis starts at period start, not first data point: Convert datasets from scalar arrays to {x: timestamp_ms, y: value} objects and switch the x-axis to type:'linear' with min/max anchored to the selected period window. Mirrors the Home dashboard approach from PR #746. Uses the shared formatTrendAxisTick helper from chart-utils.ts for tick labels. QA 2.3 -- Period Savings tooltip precision matches Cumulative: Change toFixed(4) to toFixed(2) for the Period Savings tooltip label so both series and the KPI box above the chart show 2 decimal places. QA 2.4/2.5 -- y-axis ticks stable when toggling series: Add maxTicksLimit:6 to both y and y1 axis tick configs to cap re-autoscaling on legend toggle. Fix the y1 formatter to emit 2 decimal places for non-integer float ticks, preventing distinct float values from collapsing to the same integer label string. Regression tests added in savings-history.test.ts: - QA 2.2: x-axis type:linear with numeric min equal to period start - QA 2.3: Period Savings tooltip has exactly 2 decimal places - QA 2.4: both y-axes have maxTicksLimit - QA 2.5: y1 formatter does not collapse distinct floats to same label Closes #1252 * refactor(purchases): rename savingsData to periodSavingsData for symmetry Minor readability cleanup: the Period Savings dataset array is now named periodSavingsData, matching its sibling cumulativeSavingsData. No behavior change.
5 tasks done
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.
Summary
QA verification step 3.1 (Home page Savings-over-time chart) flagged that the x-axis snaps to the single purchase date regardless of the selected timeframe (7d / 30d / 90d / All). Suggested in QA: "have the x-axis show the selected date range and leave the chart empty (no dots where there are no purchases)... currently, the purchase is shown at the start of the x-axis, no matter which date range is selected."
Fix
type: 'linear'timestamp axis with explicitmin/maxcovering the selected window:[now - days * 86400_000, now](days = 7 / 30 / 90, or 365 for "All").axisMinMsis overridden to the earliest purchase timestamp.{x: timestamp_ms, y: cumulative_savings}so each purchase is positioned by its real date inside the window, not snapped to either end.formatTrendAxisTickhelper for tick formatting.No new chart-library dependencies (no
chartjs-adapter-date-fnsneeded because the linear axis uses raw ms timestamps).Test plan
Refs QA row 405, step 3.1.
Summary by CodeRabbit
Bug Fixes
Tests