ux(frontend/approval): label orphan executions as "Account deleted" instead of "(ambient)" (closes #608) - #611
Conversation
…nstead of "(ambient)" (closes #608) Extends formatAccountLabel with provider and hostAWSAccountID params to distinguish three cases: resolved account, legitimate AWS ambient host, and deleted account. GET /api/info now returns deployment_aws_account_id (from STS GetCallerIdentity) so the frontend can tell the difference.
|
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)
📝 WalkthroughWalkthroughBackend adds optional ChangesDeployment AWS Account ID Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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.
|
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)
frontend/src/approval-details.ts (1)
52-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude host-account rows in the header account count.
When AWS recs resolve to
CUDly host (...), the header still counts onlycloud_account_id, so “Accounts” can show0while a concrete host account is listed in the table.💡 Proposed fix
-export function renderApprovalDetailsBody(details: PurchaseDetails, accountsById: AccountsById, hostAWSAccountID?: string): HTMLElement { +export function renderApprovalDetailsBody(details: PurchaseDetails, accountsById: AccountsById, hostAWSAccountID?: string): HTMLElement { @@ - root.appendChild(renderApprovalDetailsHeader(details, recs)); + root.appendChild(renderApprovalDetailsHeader(details, recs, hostAWSAccountID)); root.appendChild(renderApprovalDetailsTable(recs, accountsById, hostAWSAccountID)); return root; } @@ -function renderApprovalDetailsHeader(details: PurchaseDetails, recs: Recommendation[]): HTMLElement { +function renderApprovalDetailsHeader(details: PurchaseDetails, recs: Recommendation[], hostAWSAccountID?: string): HTMLElement { @@ - if (rec.cloud_account_id) accounts.add(rec.cloud_account_id); + if (rec.cloud_account_id) { + accounts.add(rec.cloud_account_id); + } else if ((rec.provider ?? '').toLowerCase() === 'aws' && hostAWSAccountID) { + accounts.add(hostAWSAccountID); + }🤖 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/approval-details.ts` around lines 52 - 69, renderApprovalDetailsBody currently passes recs to renderApprovalDetailsHeader but the header only counts cloud_account_id and therefore misses host-account rows (so "Accounts" can display 0 despite host rows in the table). Update renderApprovalDetailsHeader (or the counting logic it uses) to include host AWS account IDs present on recommendations: when iterating recs include both recommendation.cloud_account_id and any host account identifier (e.g., rec.host_aws_account_id or the hostAWSAccountID-derived id used by renderApprovalDetailsTable) in the unique account-id set before computing the "Accounts" count; ensure renderApprovalDetailsBody still supplies the needed hostAWSAccountID or rec fields so the header can compute the combined unique set.
🤖 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 `@frontend/src/approval-details.ts`:
- Around line 52-69: renderApprovalDetailsBody currently passes recs to
renderApprovalDetailsHeader but the header only counts cloud_account_id and
therefore misses host-account rows (so "Accounts" can display 0 despite host
rows in the table). Update renderApprovalDetailsHeader (or the counting logic it
uses) to include host AWS account IDs present on recommendations: when iterating
recs include both recommendation.cloud_account_id and any host account
identifier (e.g., rec.host_aws_account_id or the hostAWSAccountID-derived id
used by renderApprovalDetailsTable) in the unique account-id set before
computing the "Accounts" count; ensure renderApprovalDetailsBody still supplies
the needed hostAWSAccountID or rec fields so the header can compute the combined
unique set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9b72363-fa0b-4028-a2f6-163720feea91
📒 Files selected for processing (6)
frontend/src/__tests__/approval-details.test.tsfrontend/src/api/types.tsfrontend/src/approval-details.tsinternal/api/handler_dashboard.gointernal/api/openapi.yamlinternal/api/types.go
…ount (CR pass-1) Host-account rows (AWS recs with nil cloud_account_id rendered via hostAWSAccountID) were displayed correctly in the table but not counted in the "Accounts" header stat, so all-host-account purchases showed "Accounts: 0". Pass hostAWSAccountID into renderApprovalDetailsHeader and compute effectiveAccountID per rec using the same cloud_account_id ?? host-fallback logic as the table, filtering out orphan "Account deleted" rows so they don't inflate the count. Adds 3 regression tests: all-host (asserts 1), mixed host+named (asserts 2), orphan Azure row (asserts 0).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
formatAccountLabelwith two new params (provider,hostAWSAccountID) to distinguish three cases: resolved account, legitimate AWS ambient host, and deleted/orphan account.GET /api/infonow returnsdeployment_aws_account_id(populated via STSGetCallerIdentity, best-effort) so the frontend can tell genuine ambient from orphan rows.Test plan
npm testinfrontend/- all 1906 tests pass (26 in approval-details, 0 regressions elsewhere)go build ./internal/api/...compiles cleanlySummary by CodeRabbit
New Features
Bug Fixes
Tests