fix(purchases): wire topbar filter chips to all three Purchases consumers (refs #701) - #741
Merged
Merged
Conversation
…mers (#701) Three sub-flows of the global filter on the Purchases page still failed after PR #716: 1. Provider filter (4.2): selecting a provider did nothing to Purchase History or Approval Queue because setupHistoryHandlers() was exported from history.ts but never called in app.ts. 2. Account filter (4.4): the Savings History chart disappeared when an account UUID was selected because the analytics SQL only matched the legacy account_id VARCHAR(20) column, not cloud_account_id UUID FK. The topbar chip always sends cloud_accounts.id (a UUID), so the WHERE clause returned 0 rows and the chart fell into the empty-state branch. 3. Empty-state UX (4.4/4.5): showEmptyState() always showed "No savings history data available yet." regardless of whether a filter caused the empty result, giving no signal to the user. Changes: - frontend/src/app.ts: import and call setupHistoryHandlers() after initSavingsHistory() so provider/account chip changes reload Purchase History and Approval Queue. - internal/api/analytics_postgres.go: add OR cloud_account_id::text = $3 to the WHERE clause in QueryHistory and QueryBreakdown so UUID-based account filtering includes rows written by current purchase flows. - frontend/src/modules/savings-history.ts: add buildFilterDesc() helper and update showEmptyState() to show "No savings data for the selected filter (AWS)" when a filter is active vs the original message when not. - Tests: new Go tests for UUID account filter path; new TS tests for setupHistoryHandlers subscriptions and filter-aware empty-state copy; update history mocks in index/sidebar-anchors/purchase-execution-toast tests to include setupHistoryHandlers.
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBackend analytics queries now accept UUID account IDs by matching either legacy ChangesIssue
Sequence Diagram(s)sequenceDiagram
participant App as App Initialization
participant Setup as setupHistoryHandlers
participant State as State Manager
participant Loader as loadHistory
participant API as api.getHistory
App->>Setup: call setupHistoryHandlers()
Setup->>State: state.subscribeProvider(callback)
Setup->>State: state.subscribeAccount(callback)
State-->>Setup: callbacks registered
State->>Loader: trigger callback on filter change
Loader->>API: api.getHistory()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/modules/savings-history.ts`:
- Around line 121-125: The function buildFilterDesc currently treats provider
=== 'all' as an active filter; update buildFilterDesc to treat the "all"
sentinel as unfiltered by skipping adding provider to parts when provider is
falsy or equals 'all' (case-insensitive), e.g. check provider &&
provider.toLowerCase() !== 'all' before pushing provider.toUpperCase(); keep the
existing accountIDs behavior (push accountIDs[0] if present) so empty-state
copy/rendering becomes correct.
- Around line 64-67: The catch block currently calls showEmptyState, which
misleads users on API failures; implement a new showErrorState(chartContainer,
emptyEl, statsEl, message) that hides chartContainer and statsEl, shows emptyEl
with an explicit "Failed to load savings history." heading and help text
containing the provided message, and destroys the global savingsChart if
present; then replace the catch path to call showErrorState(chartContainer,
emptyEl, statsEl, error instanceof Error ? error.message : String(error))
instead of showEmptyState to surface real fetch failures.
In `@internal/api/analytics_postgres_test.go`:
- Around line 182-184: The test's mock.ExpectQuery currently only matches the
SELECT prefix so it won't catch removal of the account filter; update the
ExpectQuery regex to include the WHERE fragment that asserts the dual-column
account filter (e.g., the condition that checks cloud_account_id::text and
cloud_account_external_id::text equals $3) so the mock fails if that filter is
removed; modify the ExpectQuery call (the one chained with
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid).WillReturnRows(rows)) to
assert the WHERE clause explicitly in the regex.
🪄 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: 2a96a6fd-3e49-4727-bec4-3e617d42fc0b
📒 Files selected for processing (9)
frontend/src/__tests__/history.test.tsfrontend/src/__tests__/index.test.tsfrontend/src/__tests__/purchase-execution-toast.test.tsfrontend/src/__tests__/savings-history.test.tsfrontend/src/__tests__/sidebar-anchors.test.tsfrontend/src/app.tsfrontend/src/modules/savings-history.tsinternal/api/analytics_postgres.gointernal/api/analytics_postgres_test.go
Three CodeRabbit findings addressed:
1. (Major, line 67) Use explicit error state on fetch failure: add
showErrorState() that sets distinct heading/help copy ("Failed to
load savings history. / Please retry.") instead of the no-data
empty state, so users can tell a filter-scope empty from a real
outage. Update the catch path to call showErrorState with the
extracted error message.
2. (Minor, line 125) Treat the 'all' provider sentinel as unfiltered
in buildFilterDesc: skip pushing provider.toUpperCase() when the
value is 'all' (case-insensitive), so the empty-state heading reads
"No savings history data available yet." rather than "No savings
data for the selected filter (ALL)." when no real provider chip is
selected.
3. (Minor, line 184) Tighten SQL expectations in
TestQueryHistory_CloudAccountIDFilter and
TestQueryBreakdown_CloudAccountIDFilter: prefix regexes with (?s)
and extend to include the dual-column account filter WHERE fragment,
so removing that clause would now cause the mock expectations to
fail instead of passing silently.
Tests: all 66 savings-history frontend tests pass; Go analytics tests
pass (including the two tightened CloudAccountID tests).
Member
Author
|
Addressed all three CR findings from the 2026-05-27 09:48Z review (commit 2083142): F1 (Major, savings-history.ts:67) - Error state on fetch failure F2 (Minor, savings-history.ts:125) - 'all' sentinel treated as unfiltered F3 (Minor, analytics_postgres_test.go:184) - Tighter SQL expectations Tests: 66 frontend savings-history tests pass; all Go @coderabbitai review |
Contributor
|
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
3 tasks
cristim
added a commit
that referenced
this pull request
May 27, 2026
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.
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
Fixes #701 follow-up (PR #716 left several consumers unwired; QA rows 305-307 still failed).
Root causes and fixes
setupHistoryHandlers()was exported fromhistory.tsbut never called inapp.tsapp.ts: import + callsetupHistoryHandlers()afterinitSavingsHistory()QueryHistory/QueryBreakdownSQL matchedaccount_id(VARCHAR 12-digit ARN tail) only; topbar chip sendscloud_accounts.id(UUID)analytics_postgres.go: addedOR cloud_account_id::text = $3to WHERE in both queriesshowEmptyState()always showed generic copysavings-history.ts:buildFilterDesc()helper + filter-aware copy ("No savings data for the selected filter (AWS)")Test plan
internal/api: 1275 pass, incl. 2 new UUID filter testssetupHistoryHandlerssubscription testsFiles changed (9, +261/-12):
frontend/src/app.ts,internal/api/analytics_postgres.go,frontend/src/modules/savings-history.ts, plus tests.Summary by CodeRabbit
New Features
Bug Fixes