fix(home/filter): wire Account chip to dashboard chart (QA 2.3) - #747
Merged
Merged
Conversation
The Home page Savings-over-time chart showed "No purchase history yet" for accounts that DID have purchases when selected via the topbar Account filter. Root cause: three issues compounded: 1. Backend SQL (fixed by PR #741): QueryHistory matched only the legacy account_id VARCHAR(20) column, not cloud_account_id UUID FK. The topbar chip sends cloud_accounts.id (a UUID), so all rows returned 0 and the chart fell into the empty-state branch. 2. Missing coalescing: setupDashboardHandlers subscribed directly with () => void loadDashboard(), causing two concurrent loadDashboard() calls on provider change (topbar fires both account-cleared and provider-changed synchronously). Added queueMicrotask coalescing matching the scheduleReload pattern in modules/savings-history.ts. 3. Missing home-tab guard: subscriptions fired even when the user was on a different tab, wasting API calls. Added isHomeTabActive() guard matching isPurchasesTabActive() in modules/savings-history.ts. 4. Non-informative empty state: when the API returned 0 data points due to an active filter, the chart showed the generic "No purchase history yet" message rather than indicating the filter caused the empty result. Added buildTrendFilterDesc() and filter-aware empty.textContent, mirroring showEmptyState() in savings-history.ts. Tests: 7 new tests covering subscription wiring, active-tab guard, coalescing, account_id forwarding, and filter-aware empty-state copy. All 63 frontend test suites pass; all Go tests pass. Mirrors PR #741 fix for the Purchases page (same filter-not-wired pattern). Verified by selecting each account in turn and confirming the chart reflects only that account's history. Refs QA row 384, step 2.3.
Contributor
|
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 (2)
📝 WalkthroughWalkthroughDashboard reload behavior now coalesces provider/account filter changes into a single queued call via ChangesDashboard reload coalescing and empty-state filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
10 tasks
cristim
added a commit
that referenced
this pull request
May 27, 2026
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.
This was referenced May 27, 2026
cristim
added a commit
that referenced
this pull request
May 27, 2026
* fix(home/chart): x-axis spans selected timeframe window (QA 3.1) 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. * fixup: remove stray closing brace left after rebase 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. * fix(home/chart): align savings-trend-empty assertions + lift All-range 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. * fix(home/chart): drop provider name from empty state + omit start on 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.
This was referenced May 28, 2026
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
…#881) * fix(inventory): Active Commitments + Coverage honor Main Header chips Mirrors PR #741 (Purchases) and PR #747 (Home): wireChipSubscriptions ties provider + account chip changes to a queueMicrotask-coalesced reload, guarded by isInventoryTabActive(). Active Commitments and Coverage handlers in internal/api/handler_inventory.go accept provider and account_id query params and filter on dual-column semantics matching analytics_postgres.go. Filter-aware empty state: "No active commitments for <filter>" when chips exclude all data, instead of the generic "No active commitments" stub. Test setup fixes: chip-subscription describe block now explicitly resets module-scoped currentSubSection via switchInventorySubSection, and the inactive-tab test removes the .active class after fixing the sub-section. Closes #866. * refactor(api/inventory): extract getCoverageBreakdown helper to fit gocyclo budget Pull the recommendation-aggregation loop into aggregateOnDemandByKey so that getCoverageBreakdown stays within the cyclomatic-10 limit enforced by the pre-commit gocyclo hook (was 12, now 9). Pure extraction: no behaviour change. * fix(api/inventory): scope recommendations to account chip in getCoverageBreakdown Before this change `getCoverageBreakdown` fetched recs via `ListRecommendations(ctx, RecommendationFilter{})` — i.e. across every account — even when the Main Header `account_id` chip was active. The covered side honoured the chip (via `fetchCommitmentRecords` → `GetPurchaseHistory(acc)`), so coverage% mixed account-scoped commitments with cross-account on-demand gaps and over-reported the remaining gap for the selected account. Plumb `params["account_id"]` into `RecommendationFilter.AccountIDs` via a tiny `buildCoverageRecFilter` helper. Pulling the construction into a helper keeps `getCoverageBreakdown` at gocyclo=9 — the budget headroom the PR #881 extraction already reserved is preserved. Adds `TestHandler_getCoverageBreakdown_ProviderAndAccountChip` which seeds multi-account, multi-provider commitments + recs, calls the handler with both chips set, and asserts the mock scheduler is invoked with `AccountIDs: []string{"acc-1"}` (the filter being passed is the exact regression signal) plus the covered/on-demand sums match only the selected-account+aws rows. Locks down both the F1 account-scope consistency and the F2 provider+account chip intersection through `getCoverageBreakdown`. CR follow-up on PR #881; refs issue #866.
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 2.3 (Home page Global filter, "Select one Account at a time") flagged that the Savings-over-time chart shows "No purchase history yet" even when the selected account DOES have purchases.
Root cause (compound)
QueryHistoryonly matchedaccount_id(legacy VARCHAR), notcloud_account_id(UUID FK). The topbar chip sends a UUID, so the historic query returned 0 rows for any account.setupDashboardHandlerssubscribed directly with() => void loadDashboard(), causing two concurrentloadDashboard()calls on provider change (the topbar fires account-cleared + provider-changed synchronously). AddedqueueMicrotaskcoalescing to dedupe within a tick.isHomeTabActive()(mirrorsisPurchasesTabActive()fromsavings-history.ts).buildTrendFilterDesc()for a filter-aware empty-state message (same pattern as PR fix(purchases): wire topbar filter chips to all three Purchases consumers (refs #701) #741's Purchases chart fix).Files changed
frontend/src/dashboard.tsfrontend/src/__tests__/dashboard.test.tsTest plan
Refs QA row 384, step 2.3.
Summary by CodeRabbit