fix(inventory): Active Commitments + Coverage honor Main Header chips#881
Conversation
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.
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPropagates topbar provider/account chips into Inventory and Coverage: adds a shared API filter contract, backend provider/account scoping, frontend scoped loading and scope-aware empty states, and chip subscription wiring with visibility checks and microtask coalescing. ChangesProvider/Account Filter Propagation
Sequence DiagramsequenceDiagram
participant TopbarChips
participant wireChipSubscriptions
participant isInventoryTabActive
participant queueMicrotask
participant loadActiveCommitments
participant loadCoverageBreakdown
TopbarChips->>wireChipSubscriptions: provider/account change
wireChipSubscriptions->>isInventoryTabActive: isInventoryTabActive()
alt tab active
wireChipSubscriptions->>queueMicrotask: schedule callback
queueMicrotask->>loadActiveCommitments: refetch if active sub-tab = commitments
queueMicrotask->>loadCoverageBreakdown: refetch if active sub-tab = coverage
else tab inactive
wireChipSubscriptions-->>wireChipSubscriptions: skip refetch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ocyclo 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler_inventory.go (1)
157-197:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply
account_idchip scoping to on-demand (recommendations) coverage
Ininternal/api/handler_inventory.go,getCoverageBreakdownfetches recommendations withh.scheduler.ListRecommendations(ctx, config.RecommendationFilter{})(noAccountIDs/account_id), then only narrows by allowed-accounts and aggregates by provider—so when the account chip is active, coverage combines account-scoped covered spend with unscoped on-demand gaps, producing misleading per-service coverage (issue#866).Scope recommendations the same way as commitments: pass
params["account_id"]intoRecommendationFilter.AccountIDs(usingRecommendationRecord.CloudAccountID) or filter the returned recs to the selected account before callingaggregateOnDemandByKey. Add/extend a test to cover theaccount_idchip intersection (not just allowed-accounts).🤖 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 `@internal/api/handler_inventory.go` around lines 157 - 197, The recommendations query is not scoped to the selected account chip, mixing account-scoped commitments with unscoped on-demand gaps; when calling h.scheduler.ListRecommendations use a config.RecommendationFilter populated with AccountIDs from params["account_id"] (or, if easier, after ListRecommendations filter the returned recs by RecommendationRecord.CloudAccountID == params["account_id"]) before calling aggregateOnDemandByKey; update tests to assert the account_id chip intersection (coverage combines only recommendations for the selected account) in addition to existing allowed-accounts checks.
🧹 Nitpick comments (1)
internal/api/handler_inventory_test.go (1)
171-214: ⚡ Quick winThe new provider-filter test for
listActiveCommitmentsis correct and well-scoped.Consider adding a sibling test through
getCoverageBreakdownthat passesprovider=(andaccount_id=) so theaggregateOnDemandByKeyprovider filtering — and the covered/on-demand account-scope consistency raised onhandler_inventory.go— are locked down by a test rather than only validated indirectly.🤖 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 `@internal/api/handler_inventory_test.go` around lines 171 - 214, Add a sibling unit test for getCoverageBreakdown that explicitly passes the provider= and account_id= query parameters to exercise the provider-filtering path; set up MockConfigStore purchase history and cloud accounts similar to TestHandler_listActiveCommitments_ProviderFilter, call handler.getCoverageBreakdown with those params, and assert that aggregateOnDemandByKey applies the provider filter and that covered vs on-demand results are consistent at the account scope (use getCoverageBreakdown, aggregateOnDemandByKey, and Handler methods/structs from handler_inventory.go to locate the logic to verify).
🤖 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.
Outside diff comments:
In `@internal/api/handler_inventory.go`:
- Around line 157-197: The recommendations query is not scoped to the selected
account chip, mixing account-scoped commitments with unscoped on-demand gaps;
when calling h.scheduler.ListRecommendations use a config.RecommendationFilter
populated with AccountIDs from params["account_id"] (or, if easier, after
ListRecommendations filter the returned recs by
RecommendationRecord.CloudAccountID == params["account_id"]) before calling
aggregateOnDemandByKey; update tests to assert the account_id chip intersection
(coverage combines only recommendations for the selected account) in addition to
existing allowed-accounts checks.
---
Nitpick comments:
In `@internal/api/handler_inventory_test.go`:
- Around line 171-214: Add a sibling unit test for getCoverageBreakdown that
explicitly passes the provider= and account_id= query parameters to exercise the
provider-filtering path; set up MockConfigStore purchase history and cloud
accounts similar to TestHandler_listActiveCommitments_ProviderFilter, call
handler.getCoverageBreakdown with those params, and assert that
aggregateOnDemandByKey applies the provider filter and that covered vs on-demand
results are consistent at the account scope (use getCoverageBreakdown,
aggregateOnDemandByKey, and Handler methods/structs from handler_inventory.go to
locate the logic to verify).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b48140c-0139-4a88-92ae-d5e99383c836
📒 Files selected for processing (6)
frontend/src/__tests__/api-inventory.test.tsfrontend/src/__tests__/inventory.test.tsfrontend/src/api/inventory.tsfrontend/src/inventory.tsinternal/api/handler_inventory.gointernal/api/handler_inventory_test.go
…ageBreakdown
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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…871) (#906) RI Exchange was the last Inventory sub-tab that ignored the Main Header Provider/Account chips; Active Commitments and Coverage already honor them (PR #866/#881). QA 2.9. Frontend (riexchange.ts): - loadRIExchange is now provider-aware. AWS shows convertible RIs + reshape recs + history; Azure shows exchangeable VM reservations + history with a provider-aware "reshape not available" note; GCP shows a "RI Exchange isn't available for GCP" empty state across all sections and calls no list endpoint. - The convertible-RI list is scoped to the single-selected account chip and renders scoped, provider-aware empty states (named account/subscription). - An in-progress exchange modal is closed synchronously on any filter change so the user can't act on a now-hidden RI (mutating workflow). - New Azure table built via DOM/textContent (no innerHTML). Backend (handler_ri_exchange.go): - listConvertibleRIs accepts ?account_id= and returns an empty list when the chip selects an account other than the running ambient AWS account; a real STS failure fails closed. Azure already scopes by subscription_id; GCP has no endpoint (frontend short-circuits to empty). Tests: frontend chip-scoping, GCP not-available state, Azure path, and selection-clear on filter change; backend account-scope mismatch returns empty and resolve-error fails closed.
Summary
QA Main Header 2.7+2.14, 2.8+2.15: The Main Header global Provider/Account chips did not propagate into the Inventory & Coverage subpages. Active Commitments and Coverage stayed unfiltered regardless of chip selection.
Fix
Mirrors the chip-subscription pattern from PR #741 (Purchases) and PR #747 (Home):
wireChipSubscriptions()infrontend/src/inventory.tsties provider + account chip changes to aqueueMicrotask-coalesced reload.isInventoryTabActive()guard prevents fires when the user is on another top-level tab.Backend:
internal/api/handler_inventory.goActive Commitments + Coverage handlers acceptprovider=andaccount_id=query params and filter on the dual-column semantics matchinganalytics_postgres.go(after PR #741).Filter-aware empty state: "No active commitments for
<filter>" when chips exclude all data, instead of the generic stub.Files changed
frontend/src/inventory.tsfrontend/src/api/inventory.tsfrontend/src/__tests__/inventory.test.tsfrontend/src/__tests__/api-inventory.test.tsinternal/api/handler_inventory.gointernal/api/handler_inventory_test.goTest plan
Note on test setup
The chip-subscription describe block explicitly resets module-scoped
currentSubSectionviaswitchInventorySubSection('active-commitments')afterloadInventory()to avoid bleed from earlier tests that switch tocoverage. The inactive-tab test removes the.activeclass after fixing the sub-section.Closes #866.
Summary by CodeRabbit
New Features
Improvements
Tests