fix(frontend/ux): show per-rec details in pre-purchase approval modal (closes #374) - #375
Merged
cristim merged 4 commits intoMay 14, 2026
Conversation
…closes #374) Pre-fix, the confirm dialog shown when a user clicks Approve only carried a static sentence and (on the deep-link path) the execution UUID. Approving an RI was effectively "approve <UUID>" - not informed consent for a financial commitment. This change introduces a new `approval-details.ts` module that builds a rich HTMLElement body for confirmDialog: - Aggregate header: total upfront, total monthly + annual savings, commitment count, distinct providers, distinct accounts touched. - Per-rec table (12 columns): account (Name + external ID), provider, service, resource type, engine, region, count, term, payment, upfront, monthly savings, effective savings %. Account resolution uses the existing /api/accounts listing to turn the rec's internal cloud_account_id UUID into "Production AWS (123456789012)" - AWS account number, Azure subscription, GCP project all flow through the existing external_id field. Recs without an account (ambient-credentials direct-execute path) render as "(ambient)" so the user can still tell what they're signing for. Effective savings % uses the same denominator policy as the recommendations table: prefer rec.on_demand_cost when the provider returned a baseline, fall back to monthly_cost + savings reconstruction, render "-" when neither is usable. Both call sites use the new body: - `frontend/src/history.ts` Approve button on a pending row. - `frontend/src/purchases-deeplink.ts` email-link approve flow. The Cancel-purchase deep link keeps the legacy text body - cancelling a pending approval doesn't commit cloud spend, so the cost-of-error asymmetry doesn't justify the extra fetch. `buildApprovalDetailsBody(executionId)` is the network wrapper; it fetches the purchase details + accounts list in parallel and falls back to the legacy sentence if either GET fails so the Approve button keeps working on stale caches or 403s. `renderApprovalDetailsBody` is the pure builder, exported separately so the 20 unit tests in `approval-details.test.ts` exercise rendering without mocking fetch. CSS additions in `components.css` widen `.modal-confirm` to `min(1100px, 95vw)` only when it contains an `.approval-details` element (via `:has()`), so the other confirmDialog callers keep their 480px layout. The 12-col table is sticky-header + horizontally scrollable for narrow viewports. `PurchaseDetails` (frontend/src/api/types.ts) was expanded to match the actual shape from backend `buildPurchaseDetailsResponse` - the recommendations[] array and total_upfront_cost / estimated_savings were already on the wire, the TypeScript type was just stale.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a detailed pre-purchase approval modal: updates ChangesApproval-Details Modal Implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant buildApprovalDetailsBody
participant API
participant renderApprovalDetailsBody
Caller->>buildApprovalDetailsBody: executionId
buildApprovalDetailsBody->>API: GET /api/purchases/{id} & GET /api/accounts (parallel)
API-->>buildApprovalDetailsBody: PurchaseDetails + CloudAccount[]
buildApprovalDetailsBody->>renderApprovalDetailsBody: details, accountsById
renderApprovalDetailsBody->>renderApprovalDetailsBody: compute aggregates (providers/accounts, totals)
renderApprovalDetailsBody->>renderApprovalDetailsBody: for each recommendation: formatAccountLabel, computeEffectiveSavingsPct, escape values, build row
renderApprovalDetailsBody-->>buildApprovalDetailsBody: HTMLElement
buildApprovalDetailsBody-->>Caller: HTMLElement (or legacy fallback on error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/approval-details.test.ts (1)
1-8: ⚡ Quick winAdd one wrapper-level fallback test for
buildApprovalDetailsBody.Current tests are strong for pure rendering, but a targeted test for
buildApprovalDetailsBodywhen/api/accountsor/api/purchases/{id}fails would lock the fallback contract and catch regressions in the approval flow.Also applies to: 62-231
🤖 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/__tests__/approval-details.test.ts` around lines 1 - 8, Add a wrapper-level test that exercises buildApprovalDetailsBody (not just renderApprovalDetailsBody) to verify the fallback UI when the network helpers fail: stub the network calls used by buildApprovalDetailsBody (the accounts fetch and the purchases/{id} fetch) to return errors or non-200 responses, call buildApprovalDetailsBody with a test purchase id, and assert that the returned DOM shows the intended fallback messaging/structure (same contract the pure renderer expects). Use the existing test harness patterns for stubbing/faking fetch and DOM assertions so the test mirrors real wrapper behavior and locks the fallback contract.
🤖 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/approval-details.ts`:
- Around line 241-244: The code currently swallows errors from
api.listAccounts() by converting failures to an empty array, preventing the
legacy-fallback path when accounts fetch fails; remove the .catch(() => [] as
CloudAccount[]) wrapper (or instead wrap the entire Promise.all in a try/catch)
so that api.listAccounts() errors propagate and trigger the existing fallback
logic used when api.getPurchaseDetails(executionId) fails; update the block that
awaits Promise.all([ api.getPurchaseDetails(executionId), api.listAccounts() ])
to let failures surface (or catch them at a higher level) so the approval modal
falls back to the legacy sentence when either fetch fails.
---
Nitpick comments:
In `@frontend/src/__tests__/approval-details.test.ts`:
- Around line 1-8: Add a wrapper-level test that exercises
buildApprovalDetailsBody (not just renderApprovalDetailsBody) to verify the
fallback UI when the network helpers fail: stub the network calls used by
buildApprovalDetailsBody (the accounts fetch and the purchases/{id} fetch) to
return errors or non-200 responses, call buildApprovalDetailsBody with a test
purchase id, and assert that the returned DOM shows the intended fallback
messaging/structure (same contract the pure renderer expects). Use the existing
test harness patterns for stubbing/faking fetch and DOM assertions so the test
mirrors real wrapper behavior and locks the fallback contract.
🪄 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: 471de831-8872-4f53-8667-919bceee1ca1
📒 Files selected for processing (6)
frontend/src/__tests__/approval-details.test.tsfrontend/src/api/types.tsfrontend/src/approval-details.tsfrontend/src/history.tsfrontend/src/purchases-deeplink.tsfrontend/src/styles/components.css
Comment on lines
+241
to
+244
| const [details, accounts] = await Promise.all([ | ||
| api.getPurchaseDetails(executionId), | ||
| api.listAccounts().catch(() => [] as CloudAccount[]), | ||
| ]); |
Contributor
There was a problem hiding this comment.
Do not swallow /api/accounts failures in the fetch wrapper.
At Line 243, listAccounts() errors are converted to [], so an accounts-fetch failure won’t trigger the legacy fallback. That leaves a partially-resolved approval modal in a flow that should degrade to the legacy sentence when either fetch fails.
Suggested fix
const [details, accounts] = await Promise.all([
api.getPurchaseDetails(executionId),
- api.listAccounts().catch(() => [] as CloudAccount[]),
+ api.listAccounts(),
]);🤖 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 241 - 244, The code currently
swallows errors from api.listAccounts() by converting failures to an empty
array, preventing the legacy-fallback path when accounts fetch fails; remove the
.catch(() => [] as CloudAccount[]) wrapper (or instead wrap the entire
Promise.all in a try/catch) so that api.listAccounts() errors propagate and
trigger the existing fallback logic used when
api.getPurchaseDetails(executionId) fails; update the block that awaits
Promise.all([ api.getPurchaseDetails(executionId), api.listAccounts() ]) to let
failures surface (or catch them at a higher level) so the approval modal falls
back to the legacy sentence when either fetch fails.
CR pass on #375: CodeRabbit's Docstring Coverage check flagged the new frontend/src/approval-details.ts module (38% vs 80% threshold). The exports (renderApprovalDetailsBody, formatAccountLabel, computeEffectiveSavingsPct, buildApprovalDetailsBody) already carry docstrings; the internal helpers do not. Adds JSDoc for the four internal rendering functions: - renderApprovalDetailsHeader (the stat-row above the table) - headerStat (one label/value cell) - renderApprovalDetailsTable (the wrapper + sticky thead container) - renderRecRow (one tbody row from a Recommendation) Each docstring names what the function returns and any non-obvious contract (accountsById lookup, escapeHtml safety, baseline-preferred effective savings %) so the module is self-documenting end-to-end.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/approval-details.ts (1)
276-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet
/api/accountsfailures trigger the legacy fallback.Line 278 still converts
listAccounts()failures into[], so this flow can render partial approval details instead of falling back when either request fails. That breaks the resilience contract for this modal.Suggested fix
const [details, accounts] = await Promise.all([ api.getPurchaseDetails(executionId), - api.listAccounts().catch(() => [] as CloudAccount[]), + api.listAccounts(), ]);🤖 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 276 - 279, The current Promise.all call swallows failures from api.listAccounts by catching and returning [] (variables details/accounts and functions api.getPurchaseDetails/api.listAccounts), allowing partial rendering instead of triggering the legacy fallback; remove the .catch(() => [] as CloudAccount[]) wrapper so api.listAccounts() errors propagate (i.e., use Promise.all([api.getPurchaseDetails(executionId), api.listAccounts()])) and ensure the surrounding error handling that triggers the legacy fallback will run when either request fails.
🤖 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/approval-details.ts`:
- Around line 93-96: The loop over recs currently only adds rec.cloud_account_id
into the accounts Set, which causes an all-ambient result to show 0 accounts;
inside the same loop (for (const rec of recs) { ... }) add an else branch that
inserts a sentinel value (e.g. 'ambient' or 'unknown') into accounts when
rec.cloud_account_id is falsy/undefined (or when an ambient flag on rec is set),
so ambient recommendations are counted as their own bucket; keep using
accounts.size for display and ensure any UI text treats that sentinel as
"Ambient" or "Unknown".
---
Duplicate comments:
In `@frontend/src/approval-details.ts`:
- Around line 276-279: The current Promise.all call swallows failures from
api.listAccounts by catching and returning [] (variables details/accounts and
functions api.getPurchaseDetails/api.listAccounts), allowing partial rendering
instead of triggering the legacy fallback; remove the .catch(() => [] as
CloudAccount[]) wrapper so api.listAccounts() errors propagate (i.e., use
Promise.all([api.getPurchaseDetails(executionId), api.listAccounts()])) and
ensure the surrounding error handling that triggers the legacy fallback will run
when either request fails.
🪄 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: 9a328f9a-d818-42ae-9611-07179d28cdcc
📒 Files selected for processing (1)
frontend/src/approval-details.ts
Comment on lines
+93
to
+96
| for (const rec of recs) { | ||
| if (rec.provider) providers.add(rec.provider); | ||
| if (rec.cloud_account_id) accounts.add(rec.cloud_account_id); | ||
| totalMonthly += rec.savings; |
Contributor
There was a problem hiding this comment.
Do not report 0 accounts when recommendations are ambient.
This aggregate only counts rows with cloud_account_id, so an all-ambient approval can show Accounts: 0 even though the table is about to commit spend against the ambient execution context. Count ambient as its own bucket or render an explicit unknown/ambient value here.
🤖 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 93 - 96, The loop over recs
currently only adds rec.cloud_account_id into the accounts Set, which causes an
all-ambient result to show 0 accounts; inside the same loop (for (const rec of
recs) { ... }) add an else branch that inserts a sentinel value (e.g. 'ambient'
or 'unknown') into accounts when rec.cloud_account_id is falsy/undefined (or
when an ambient flag on rec is set), so ambient recommendations are counted as
their own bucket; keep using accounts.size for display and ensure any UI text
treats that sentinel as "Ambient" or "Unknown".
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…fallback + warn on accounts failure Self-review pass on #375 — three issues caught that would be CR actionables on a careful read: 1. Floating-point strict-equality bug in the annual-savings tooltip. `totalAnnual !== monthly * 12` triggered on rounding noise as small as 1e-17 (Number((0.1 + 0.2) * 12) !== 0.3 * 12 is true), so the "from per-row sum" tooltip would fire on conceptually-equal totals. Switched to `Math.abs(...) > 0.005` — well below the sub-cent precision formatCurrency renders. Two new tests pin both the no-tooltip-on-rounding-noise and the tooltip-on-genuine-mismatch shape. 2. Silent accounts-API failure. The inner `.catch()` on listAccounts recovered to an empty array but produced no signal — every rec would fall back to "acct xxxxxxxx…" labels without telling the debugger why. Added a console.warn so a network blip is traceable in the browser console, separate from the outer error path that handles a full details failure. 3. Duplicated fallback HTML. The legacy approval sentence lived in two places (empty-recs branch + fetch-failure branch) and could drift. Extracted to a shared `buildApprovalDetailsFallback` helper. Plus a small defensive `?? 0` on `rec.savings` in the totalMonthly accumulation: the wire type is wider than the TS type and a single NaN from a legacy row would poison the entire header total. Also simplified `rec.engine && rec.engine !== ''` to `rec.engine` — for a string both halves test the same thing. Frontend tests: 1667/1667 pass (22 in approval-details.test.ts), `tsc --noEmit` clean.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/approval-details.ts (1)
292-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet
/api/accountsfailures hit the existing fallback path.This still leaves a partial modal on 403/network errors from
listAccounts(), even though the surrounding wrapper is documented to fall back when either fetch fails. Let this rejection escapePromise.all()so Lines 302-304 handle both requests consistently.Suggested fix
const [details, accounts] = await Promise.all([ api.getPurchaseDetails(executionId), - api.listAccounts().catch((err) => { - console.warn('Failed to load accounts for approval modal — falling back to UUID-prefixed labels:', err); - return [] as CloudAccount[]; - }), + api.listAccounts(), ]);🤖 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 292 - 297, The Promise.all currently swallows errors from api.listAccounts() via .catch, preventing the outer fallback for failed fetches from running; remove the inline .catch on api.listAccounts() so that Promise.all([api.getPurchaseDetails(executionId), api.listAccounts()]) will reject on listAccounts errors and allow the existing post-Promise.all handling (the code around details/accounts handling at lines ~302-304) to execute the shared fallback path; locate the Promise.all call that assigns const [details, accounts] and remove the catch on listAccounts while keeping the warning/fallback logic that runs when Promise.all rejects.
🤖 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/approval-details.ts`:
- Around line 265-275: computeEffectiveSavingsPct can produce NaN when
rec.savings is null/undefined; guard the numerator before computing and return
null if rec.savings is nullish. In the on_demand branch (inside
computeEffectiveSavingsPct) check that rec.savings is not null/undefined before
performing (rec.savings / rec.on_demand_cost) * 100 (allow zero savings), and
likewise in the reconstructed branch check rec.savings is not null/undefined
before computing with reconstructed; if rec.savings is nullish return null.
---
Duplicate comments:
In `@frontend/src/approval-details.ts`:
- Around line 292-297: The Promise.all currently swallows errors from
api.listAccounts() via .catch, preventing the outer fallback for failed fetches
from running; remove the inline .catch on api.listAccounts() so that
Promise.all([api.getPurchaseDetails(executionId), api.listAccounts()]) will
reject on listAccounts errors and allow the existing post-Promise.all handling
(the code around details/accounts handling at lines ~302-304) to execute the
shared fallback path; locate the Promise.all call that assigns const [details,
accounts] and remove the catch on listAccounts while keeping the
warning/fallback logic that runs when Promise.all rejects.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 389e59aa-00be-4fb2-bb5b-be57bd91d173
📒 Files selected for processing (2)
frontend/src/__tests__/approval-details.test.tsfrontend/src/approval-details.ts
Comment on lines
+265
to
+275
| export function computeEffectiveSavingsPct(rec: Recommendation): number | null { | ||
| if (rec.on_demand_cost !== undefined && rec.on_demand_cost !== null && rec.on_demand_cost > 0) { | ||
| return (rec.savings / rec.on_demand_cost) * 100; | ||
| } | ||
| // Fallback: reconstruct on-demand from monthly + savings. Only valid | ||
| // when monthly_cost is present and positive — for all-upfront recs | ||
| // with monthly_cost = 0 the reconstruction collapses (see #274). | ||
| if (rec.monthly_cost !== null && rec.monthly_cost !== undefined && rec.monthly_cost > 0) { | ||
| const reconstructed = rec.monthly_cost + rec.savings; | ||
| if (reconstructed > 0) return (rec.savings / reconstructed) * 100; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard nullish rec.savings before computing the percentage.
The header code already acknowledges that legacy rows can carry null savings. Here that same payload becomes NaN, which bubbles to Line 226 as NaN% instead of —. Return null when the numerator is missing.
Suggested fix
export function computeEffectiveSavingsPct(rec: Recommendation): number | null {
- if (rec.on_demand_cost !== undefined && rec.on_demand_cost !== null && rec.on_demand_cost > 0) {
- return (rec.savings / rec.on_demand_cost) * 100;
+ const savings = rec.savings;
+ if (savings === undefined || savings === null) return null;
+
+ if (rec.on_demand_cost !== undefined && rec.on_demand_cost !== null && rec.on_demand_cost > 0) {
+ return (savings / rec.on_demand_cost) * 100;
}
// Fallback: reconstruct on-demand from monthly + savings. Only valid
// when monthly_cost is present and positive — for all-upfront recs
// with monthly_cost = 0 the reconstruction collapses (see `#274`).
if (rec.monthly_cost !== null && rec.monthly_cost !== undefined && rec.monthly_cost > 0) {
- const reconstructed = rec.monthly_cost + rec.savings;
- if (reconstructed > 0) return (rec.savings / reconstructed) * 100;
+ const reconstructed = rec.monthly_cost + savings;
+ if (reconstructed > 0) return (savings / reconstructed) * 100;
}
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function computeEffectiveSavingsPct(rec: Recommendation): number | null { | |
| if (rec.on_demand_cost !== undefined && rec.on_demand_cost !== null && rec.on_demand_cost > 0) { | |
| return (rec.savings / rec.on_demand_cost) * 100; | |
| } | |
| // Fallback: reconstruct on-demand from monthly + savings. Only valid | |
| // when monthly_cost is present and positive — for all-upfront recs | |
| // with monthly_cost = 0 the reconstruction collapses (see #274). | |
| if (rec.monthly_cost !== null && rec.monthly_cost !== undefined && rec.monthly_cost > 0) { | |
| const reconstructed = rec.monthly_cost + rec.savings; | |
| if (reconstructed > 0) return (rec.savings / reconstructed) * 100; | |
| } | |
| export function computeEffectiveSavingsPct(rec: Recommendation): number | null { | |
| const savings = rec.savings; | |
| if (savings === undefined || savings === null) return null; | |
| if (rec.on_demand_cost !== undefined && rec.on_demand_cost !== null && rec.on_demand_cost > 0) { | |
| return (savings / rec.on_demand_cost) * 100; | |
| } | |
| // Fallback: reconstruct on-demand from monthly + savings. Only valid | |
| // when monthly_cost is present and positive — for all-upfront recs | |
| // with monthly_cost = 0 the reconstruction collapses (see `#274`). | |
| if (rec.monthly_cost !== null && rec.monthly_cost !== undefined && rec.monthly_cost > 0) { | |
| const reconstructed = rec.monthly_cost + savings; | |
| if (reconstructed > 0) return (savings / reconstructed) * 100; | |
| } | |
| return null; | |
| } |
🤖 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 265 - 275,
computeEffectiveSavingsPct can produce NaN when rec.savings is null/undefined;
guard the numerator before computing and return null if rec.savings is nullish.
In the on_demand branch (inside computeEffectiveSavingsPct) check that
rec.savings is not null/undefined before performing (rec.savings /
rec.on_demand_cost) * 100 (allow zero savings), and likewise in the
reconstructed branch check rec.savings is not null/undefined before computing
with reconstructed; if rec.savings is nullish return null.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
3 tasks
5 tasks
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
Closes #374. Pre-fix, the confirmation modal shown when a user clicks Approve only carried a static sentence and the execution UUID — not informed consent for a financial commitment.
This PR introduces a new
frontend/src/approval-details.tsmodule that builds a rich modal body:Per-rec field sources
/api/accountslookup keyed offrec.cloud_account_id→Name (external_id)recrec.engine, present on RDS / Aurora / OpenSearch / ElastiCache; "—" otherwiserecvia existingformatTermrecvia existingformatCurrencyrec.savings / rec.on_demand_costwhen baseline present, falls back tosavings / (monthly + savings)reconstruction, "—" when neither is usableThe external-ID column does triple duty: AWS account number, Azure subscription ID, GCP project — all flow through the existing
CloudAccount.external_idfield. Recs without an account (ambient-credentials direct-execute path) render as(ambient).Call sites wired
frontend/src/history.ts— dashboard Approve button on pending rows.frontend/src/purchases-deeplink.ts— email-link approve flow.The Cancel-purchase deep link keeps the legacy text body — cancelling a pending approval doesn't commit cloud spend, so the cost-of-error asymmetry doesn't justify the extra fetch.
Resilience
buildApprovalDetailsBody(executionId)fetches details + accounts list in parallel and falls back to the legacy sentence if either GET fails. The Approve button keeps working on stale caches, 403s, or transient network failures — just without the details.CSS
components.csswidens.modal-confirmtomin(1100px, 95vw)only when it contains an.approval-detailselement (via:has()), so the other confirmDialog callers (Cancel, Delete, etc.) keep their 480px layout. The 12-col table is sticky-header + horizontally scrollable for narrow viewports, capped at 50vh.Tests
frontend/src/__tests__/approval-details.test.ts— 20 unit tests onrenderApprovalDetailsBody+ the pure helpers. Covers:Name (external_id), unknown →acct <prefix>…, missing →(ambient)formatAccountLabelandcomputeEffectiveSavingsPctedge cases (zero baseline, missing fields)Test plan
npx jest— 1666 tests pass (47 suites)npx tsc --noEmit— cleanBackend changes
None. The backend's
buildPurchaseDetailsResponsealready returns the fullrecommendations[]array andtotal_upfront_cost/estimated_savings. Only the TypeScriptPurchaseDetailstype was stale — it has been brought into sync with the actual response shape.Summary by CodeRabbit
New Features
Style
Tests