fix(dashboard): apply provider filter to savings-history chart (closes #498) - #1122
Conversation
…498) The Savings History / savings-over-time chart and YTD KPI sparkline are backed by GET /history/analytics. The handler read and applied the account filter but never read params["provider"], and QueryHistory had no provider predicate, so the series showed identical all-provider data regardless of the selected AWS/Azure/GCP chip (QA row 2.3). The Home dashboard trend chart also deliberately dropped the provider param when calling the endpoint. Thread the provider through end to end, mirroring the working /history and trends (QueryByService) paths: - handler getHistoryAnalytics: read provider, normalise the "all" sentinel to "" (no filter) and validateProvider at the boundary, then pass it to QueryHistory. - PostgresAnalyticsClient.QueryHistory + AnalyticsClientInterface: add a provider param and an optional parameter-bound `AND provider = $N` WHERE fragment ("" = all providers), positioned after the dual-column account binds. - dashboard.ts loadSavingsTrendChart: forward the provider chip (and name it in the empty-state via buildFilterDesc, now that the query is actually provider-scoped). modules/savings-history.ts already forwarded provider; correct its stale "no-op until #502" comment. Regression tests fail pre-fix and pass post-fix: - TestQueryHistory_ProviderFilter / _ProviderFilterWithAccount assert the provider bind and its position. - TestHandler_getHistoryAnalytics_ProviderFilter / _ProviderAllIsNoFilter / _InvalidProvider cover threading, the "all" sentinel, and boundary rejection. - dashboard.test.ts asserts the provider chip is forwarded to the API and omitted when no provider is selected. Also reset the topbar-filter mocks in the savings-trend describe block so tests stay isolated now that the chart reads the provider chip.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughProvider filter from the dashboard is now forwarded through the analytics API to the backend, where it is validated and applied as a SQL WHERE clause in history analytics queries. Frontend empty-state copy reflects the active provider filter. ChangesProvider filtering in savings analytics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Summary
The dashboard Savings History / savings-over-time chart (and the YTD KPI sparkline) ignored the global provider filter: selecting AWS / Azure / GCP showed the same data regardless of which provider was selected. Account filtering (fixed in #956) worked; provider filtering on this chart did not.
Fixes the NEW bug reported under issue #498, QA row 2.3.
Root cause
The chart is backed by
GET /history/analytics. The handlergetHistoryAnalyticsread and applied theaccount_idfilter but never readparams["provider"], andPostgresAnalyticsClient.QueryHistoryhad no provider WHERE predicate at all. The frontendmodules/savings-history.tsforwardedprovider(commented as a "no-op until #502 lands"), while the Homedashboard.tstrend chart deliberately dropped it. Net effect: the savings-history query always aggregated all providers.By contrast, the working charts (
/history, and the trendsQueryByService) readprovider, normalise the"all"sentinel to"", validate it, and applyAND provider = $N.Fix (mirrors the existing provider-filter pattern)
handler_analytics.gogetHistoryAnalytics: readprovider, normalise"all"->""(no filter),validateProviderat the boundary, pass it toQueryHistory.analytics_postgres.goQueryHistory+AnalyticsClientInterface: add aproviderparam and an optional parameter-boundAND provider = $Nfragment (""= all providers), positioned after the dual-column account binds.dashboard.tsloadSavingsTrendChart: forward the provider chip and name it in the empty-state (now accurate, since the query is provider-scoped).modules/savings-history.ts: corrected the stale comment (already forwarded the param)."all"provider preserves the existing "all providers" semantics intentionally; an unknown provider returns a 400.Repro (QA row 2.3)
Tests (fail pre-fix, pass post-fix)
TestQueryHistory_ProviderFilter,TestQueryHistory_ProviderFilterWithAccount— assert theAND provider = $Nbind and its position.TestHandler_getHistoryAnalytics_ProviderFilter,_ProviderAllIsNoFilter,_InvalidProvider— threading, the"all"sentinel, boundary rejection.dashboard.test.ts— asserts the provider chip is forwarded to the API (and omitted when none selected); empty-state names the provider.Verified pre-fix failure / post-fix pass for the core regressions.
go build ./...,go test ./internal/api/... ./internal/analytics/..., frontendtsc+ the affected jest suites all green.Closes #498
Summary by CodeRabbit
New Features
Tests