[cristim]

Summary

These are frontend fixes for how the UI reports a refused purchase (#1694, items 1, 2, 4, 5 and 6). Item 3, the duplicate-purchase warning, is in #1712, so this PR is Part of #1694.

• Toast height (item 1). .toast-message gets max-height: 8rem; overflow-y: auto, so a long failure message can no longer cover the viewport.
• Status cell height (item 6). The Purchase History status description gets a new class .history-status-desc, clamped to 4 lines. Its full text is in a title.
• Review modal (item 2). After a failed execute that the server recorded (the error details carry execution_id), the modal closes and its selection and mode are cleared, so a new attempt needs a new Purchase click. The error toast still shows. Errors with no recorded execution, such as network failures, keep the modal open so the user can retry. Fan-out behaviour is unchanged.
• Savings on rows that bought nothing (item 4). Failed, cancelled and expired rows show a muted "—" in Monthly Savings instead of a green figure.
• Total Purchases (item 5). The card shows summary.total_completed, which the API already returns, instead of total_purchases, which counted every row. For an older API it falls back to total_purchases. The subline now reads "M pending".

Tests

• New jest tests fail on main and pass here (8 cases): the toast cap, status clamp, full text in title, savings muted on failed/cancelled/expired rows, completed-only count, and the modal closing on a recorded failure.
• The CSS caps are asserted from the stylesheet text, since jsdom has no layout.
• Each change was reverted one at a time, and each revert makes a test fail.
• Local, macOS: npm test passes (184 suites, 4723 passed). npm run lint reports 0 errors and npm run build compiles. The full Playwright suite passes (299 tests).

Part of #1694

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 34 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/refused-purchase-ui
• File: .coderabbit.yaml
• Commit: 406cfbe
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7c6218b8-a18e-43bb-8c36-f1c43a6e5591

Commits

Reviewing files that changed from the base of the PR and between b3a6b30 and 406cfbe.

Files selected for processing: 11

• frontend/src/__tests__/app-fanout-retry.test.ts
• frontend/src/__tests__/app-submit-toast.test.ts
• frontend/src/__tests__/history-refused-purchase.test.ts
• frontend/src/__tests__/purchase-execution-toast.test.ts
• frontend/src/__tests__/refused-purchase-css.test.ts
• frontend/src/__tests__/sidebar-anchors.test.ts
• frontend/src/app.ts
• frontend/src/history.ts
• frontend/src/recommendations.ts
• frontend/src/styles/components.css
• frontend/tests-e2e/purchase-failure-closes-modal.spec.ts

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

Cold review: claude-qa-ui-3, verdict APPROVE at 3bda834

I didn't author this PR. I checked it against #1694 items 1, 2, 4, 5 and 6, and the backend summary code.

Verified

• Totals.
  • The card now shows summary.total_completed.
  • The backend excludes pending/notified/scheduled, approved/running/paused, failed, expired, canceled (both spellings) and revoked rows from TotalCompleted and from every dollar total (internal/api/handler_history.go addHistoryRowToSummary / addNonCompletedRowToSummary). So a failed or refused execution no longer counts as a purchase, and the savings and upfront cards already excluded it.
  • total_completed has no omitempty (types.go:1051), so the ?? total fallback only matters for an older API.
• Home. The KPIs come from purchase_history rows of active commitments (handler_dashboard.go calculateCommitmentMetrics, isActiveCommitment). A failed or refused execution writes no purchase_history row, so it can't show savings or count there.
• History rows. noSavingsCell mutes Monthly Savings for failed, cancelled/canceled and expired, and for the empty case. Failed and refused rows no longer show a green figure.
• Full message reachable.
  • The toast is max-height: 8rem; overflow-y: auto: capped, and the full text scrolls.
  • The status cell is line-clamped to 4 lines, with the full escaped text in title (escapeHtmlAttr).
• Modal close. It only happens when the server recorded the execution (details.execution_id). Network-style errors keep the modal open for a retry, and the fan-out path is unchanged. finally then recomputes the button state from the cleared selection, so it can't re-arm.
• Tests fail on main. I ran the 3 new jest files against main's app.ts, history.ts and components.css. Exactly the 8 new cases fail: toast cap, clamp, title, failed, cancelled and expired savings, completed-only count, and the modal close. The other 34 pass.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 184 suites, 4723 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.
  • Playwright bulk-approval-confirm, execute-now-warning, fanout-partial-failure, history-queue-link, history-filter-popover-scope and history-local-dates: 11 passed (CI=1, under the review and port-4173 locks).

Non-blocking notes

1. Modal-close race (pre-existing pattern). Nothing blocks closing the modal while a submit is in flight. If the user closes it and opens a new purchase modal before the old request fails, the failure path's closePurchaseModal() / clearPurchaseModalRecommendations() closes and clears the new modal's selection. The success path on main already behaves the same way, so this PR extends an existing race rather than adding one. A submission generation token checked before the close would fix both paths.
2. Rows still shown green. Rows the summary excludes still show green estimated savings in the list: pending, notified, scheduled, approved, running and paused rows, and revoked rows (revoked_at set, status still completed). These aren't failures or refusals, so they're outside this PR, but a follow-up could make the list match the totals.
3. Accessibility of the status text. The full text is reachable only through title, which keyboard and touch users can't open, and the scrollable toast isn't focusable. An expand control would make it reachable for everyone.

Not merged.

[cristim]

Cold review verdict: CHANGES REQUESTED at 3bda834

Reviewer: claude-ui-fix-2. I did not author this PR and had no author transcript, and I treated the PR body as a claim.

Local verification (macOS, at 3bda834, under heavy-review.lock with the pw-4173 port lock)

• npm ci && npm test: 184 suites, 4723 passed, 1 skipped
• npm run lint: 0 errors (141 warnings, already on main)
• npm run build: OK
• Full Playwright on the production bundle (CI=1): 299 passed
• The new tests fail on main. With the three new test files against main's app.ts, history.ts and components.css: 8 failed, 34 passed, as the PR says.

Findings

F1 (medium): an audit-loss failure, where the purchase ran but its final status could not be saved, still leaves the review modal open with Execute re-armed.
The modal closes only when the error details carry execution_id (frontend/src/app.ts:615). The execute failure path in internal/api/handler_purchases.go (executeFailureError, :523 and :527) answers two recorded outcomes without details:

• ErrAuditLoss: a 500, "execution … ran but its final status could not be saved". The purchase may have been bought.
• ErrStaleAzurePricing: a 409, sent after the row was claimed.

Both take the "keep the modal open for retry" branch, so a single click can start a second execution. The audit-loss case is the riskiest one item 2 of #1694 is about. Possible fixes:

• add execution_id (and status) to those two errors server-side, so the existing frontend check catches them; or
• close the modal on any error the server returned with a status (anything but a network or parse failure), and keep it open only when no response arrived.

F2 (low-medium, accessibility): the full status text is reachable only by mouse hover.
.history-status-desc clamps the text to 4 lines and puts the full text in title (frontend/src/history.ts:1421). A title tooltip can't be reached by keyboard, and touch has no hover. That includes the phone card layout, where this cell is clamped too. So the full sanitized message is cut off for those users. A disclosure ("Show details") or a focusable control that reveals the text would make it reachable. The toast is fine: it scrolls inside 8rem, so its full text can be reached.

F3 (low): revoked rows still show a green Monthly Savings figure.
noSavingsCell checks status only (frontend/src/history.ts:102). A row with revoked_at set keeps its "completed" status and its green savings, but the API leaves revoked rows out of total_completed and every savings KPI (internal/api/handler_history.go, addHistoryRowToSummary). A row that no longer saves anything therefore looks like a saving, which is the same mismatch this PR fixes for failed rows. Adding || p.revoked_at to the check would close it.

F4 (low): the Monthly Savings filter and sort still use the raw estimate.
frontend/src/history.ts:1231 returns p.estimated_savings for the savings column, so a filter such as > 0 still matches failed, cancelled and expired rows that now show "—".

F5 (low, existing pattern now also on the failure path): closing and reopening the modal mid-request.
Closing the purchase modal is not blocked while a request is in flight. If a user closes it and opens a new purchase before the first response lands, the new failure branch closes and clears the newer modal's selection. The success path at :548 already does the same, but tying both branches to the modal session that sent the request would avoid acting on the wrong one.

Checked and fine

• Failed, cancelled and expired rows show a muted "—" in both the table and the phone card layout (data-label cells).
• The approval queue lists pending rows only, so its savings column is unaffected.
• Total Purchases now matches the API's total_completed, which already leaves out failed, cancelled, expired, in-progress and revoked rows. The fallback to total_purchases applies only to an older API.
• The toast cap is max-height: 8rem; overflow-y: auto, so the text stays scrollable.
• After a recorded failure, the finally block re-enables Execute from the now-empty selection, so it stays disabled.
• Fan-out behaviour is unchanged.

Not checked: a production refused purchase. Home's YTD figures come from the backend and are not touched by this PR.

[cristim]

Fix round for the review at 3bda8341. New head: 7d21af5b. The two new commits are a fast-forward on top of the reviewed ones.

• F1. The review modal now also closes on any 5xx (including the audit-loss 500) and on a 409 (stale Azure pricing after the claim), not only when execution_id is present. Transport failures (status 0) and other 4xx refusals keep it open, so the user can fix the input (attemptMayHaveRun, app.ts).
• F5. The close and clear act only on the modal session that sent the request: the selection array must be the same one. A late failure no longer clears a newer purchase.
• F2. A status description longer than the clamp also renders a <details class="history-status-full"> with a "Full message" summary. It can be reached by keyboard and touch, and its body scrolls inside 12rem. The title stays for mouse hover.
• F3. Rows with revoked_at show no savings (boughtNothing).
• F4. The Monthly Savings sort and filter value is 0 for rows that bought nothing, including revoked rows.

Tests. The new tests failed before these commits (3 modal, 4 history) and pass now:

• the audit-loss 500 and the 409 close the modal;
• a 400 keeps it open;
• a late failure leaves a newer modal alone;
• a long description has a disclosure and a short one has none;
• a revoked row is not styled as a saving;
• the savings sort and filter value is 0 for failed and revoked rows.

Local, macOS: npm test passes (184 suites, 4732 passed). npm run lint reports 0 errors, npm run build compiles, and the full Playwright suite passes (299).

[cristim]

Cold re-review: claude-qa-ui-3, verdict CHANGES REQUESTED at 7d21af5

This reviews the delta 3bda8341..7d21af5b (7e40f07 and 7d21af5).

Blocking finding

High: frontend/src/app.ts:616. The F5 session guard is never true in production, so the modal never closes on a failure.

• handleExecutePurchase captures const localRecs = getPurchaseModalRecommendations(); (app.ts:454) and later closes only if getPurchaseModalRecommendations() === localRecs.
• The real getter returns a new array on every call: [...currentPurchaseRecommendations] before the modal state is initialised, currentPurchaseRecommendations.filter(...) after (recommendations.ts:250-256). Two calls are never ===.
• Probe: a temporary jest test importing the real getPurchaseModalRecommendations and comparing two consecutive calls fails with Expected: true, Received: false.
• In purchase-execution-toast.test.ts, getPurchaseModalRecommendations is a jest.fn().mockReturnValue(sameArray), so the identity holds only in the tests.

Net effect at this head: after a recorded failure (execution_id), a 409 or any 5xx, the Review Purchase modal stays open with Execute re-armed. That is #1694 item 2, which the first commit (3bda834) fixed and this delta undoes.

Suggested fix: compare a modal session token instead of the array. For example, bump a counter or store a token in openPurchaseModal and in clearPurchaseModalRecommendations, expose getPurchaseModalSessionId(), capture it before await api.executePurchase(...), and compare after. Add one test that uses the real recommendations module, or at least a mock that returns a new array per call, so this can't pass by reference identity again. The success path has the same open-after-reopen race on main; the same token would fix it too.

F1: closing on 409 or any 5xx (logic is correct once the guard works)

attemptMayHaveRun returns true for details.execution_id, status >= 500 or 409.

• Closing on an ambiguous 5xx can't hide a definite success: the error toast still shows the server's message (for example, the audit-loss 500 says the execution "ran but its final status could not be saved"). Closing only removes the one-more-click path to a second purchase.
• A 5xx where nothing ran costs the user a reselect, which is the safe direction.
• 4xx validation refusals and transport errors (no status) keep the modal open.

Verified (fine)

• F2: statusFullTextHtml adds <details><summary>Full message</summary>…</details> for text over 160 characters. A native <summary> is keyboard-focusable (Tab, then Enter or Space) and toggles on touch, and its body scrolls inside max-height: 12rem.
  • Non-blocking: the 160-character threshold is a heuristic for the 4-line, 18rem clamp. On narrow or stacked layouts, shorter text can be clamped with no disclosure, and long unbreakable tokens behave differently. A ResizeObserver or scrollHeight > clientHeight check would be exact. Screen readers also read the text twice (clamped div plus disclosure).
• F3: boughtNothing treats revoked_at rows as no-savings.
• F4: the savings sort and filter value (purchaseHistoryNumericCellValue) is 0 for those rows, consistent with the cell.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 184 suites, 4732 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.
  • Playwright execute-now-warning, fanout-partial-failure, history-queue-link and history-local-dates: 8 passed (CI=1, under the review and port-4173 locks). These don't exercise the single-path failure close, which is why the regression doesn't show.

Not merged.

[cristim]

Delta cold review verdict: CHANGES REQUESTED at 7d21af5

Reviewer: claude-as-qa-2 (cold; took over from claude-ui-fix-2, whose handover flagged the same suspected blocker). Delta 3bda834..7d21af5. Everything was mocked with page.route, and no real purchase endpoint was called.

Verified

• Scratch worktree at the head, review lock plus pw-4173.lock: npm ci, full jest (184 suites; 4732 passed, 1 skipped), npm run lint (0 errors), npm run typecheck (clean), npm run build (ok), full Playwright with CI=1 (299 passed). node_modules removed afterwards.
• The delta tests fail on 3bda834. With 3bda834's app.ts, history.ts and components.css, 3 tests fail; all 49 pass at the head.

Blocker: F1/F5 regression, the review modal never closes after a failed purchase

handleExecutePurchase now closes only if attemptMayHaveRun(err) && getPurchaseModalRecommendations() === localRecs (frontend/src/app.ts:616). But getPurchaseModalRecommendations() builds a new array on every call: [...currentPurchaseRecommendations], or .filter(...) once the modal is open (frontend/src/recommendations.ts:250-255). In the real app the identity check is therefore always false, so the close branch never runs, including the execution_id case that 3bda834 already closed. The new jest tests can't see this, because purchase-execution-toast.test.ts mocks ../recommendations, and its mockReturnValue hands back the same array object each time.

A review-only Playwright probe against the real bundle confirms it. It selects one recommendation, opens Review Purchase, executes, and the mocked /api/purchases/** POST answers with the failure; exactly one POST was made in each case:

Path	Response	Review dialog after	Execute
Execute now	500 audit-loss, no details	still open	enabled
Execute now	409 stale pricing, no details	still open	enabled
Execute now	403 with execution_id	still open	enabled
Execute now	400 validation (control)	open (expected)	enabled
Send for approval	502 with execution_id	still open	n/a
Send for approval	500, no details	still open	n/a

So after a purchase that may have run, one more click can start a second one: the exact risk F1 is about. Suggested fix: tie the modal session to a token, not array identity. For example, increment a purchaseModalSession counter when the modal opens, capture it with localRecs, and compare that number. Then add a test that uses the real recommendations module (or a Playwright case like the table above), so a getter that returns copies can't hide this again.

Other findings from the last round

• F2: closed. Descriptions over STATUS_CLAMP_CHARS (160) render <details class="history-status-full"><summary>Full message</summary>…, with the text escaped. In the probe, the summary took focus, Enter opened it, and the body held the full text (505 of 506 characters, trimmed). Nit: 160 characters approximates the 4-line clamp at desktop width. On a phone card the clamp can cut shorter text, which then has no disclosure.
• F3: closed. boughtNothing treats revoked_at rows as no saving.
• F4: closed. The savings sort/filter value is 0 when boughtNothing(p).

Not merging.

[cristim]

Fix round for qa-ui-3's and as-qa-2's findings at 7d21af5b. New head: 406cfbe58aa66b8cf5f990e7f40da20719c9023b, a fast-forward of 2 commits.

• Blocker (modal never closed). The selection array was compared with the one captured when the request was sent, but getPurchaseModalRecommendations() returns a new array on every call, so the check was never true in a real browser. The close now compares the purchase modal's open token (getPurchaseModalSession(), the existing purchaseModalOpenToken, bumped on every open). The jest mock that hid this now uses the token too.
• Regression test on the real module. The new Playwright spec tests-e2e/purchase-failure-closes-modal.spec.ts drives Opportunities, Purchase, Execute now and confirm against the real bundle. The modal closes on 502 with execution_id, 403 with execution_id, 500 without it and 409. A 400 keeps it open. The close cases failed at 7d21af5b (modal still visible) and pass now.
• F2 at phone width (as-qa-2). The disclosure now appears above 80 characters, which the four-line clamp cuts in the phone card layout. A new jest case for ~100 characters failed before and passes now.

Local, macOS: npm test passes (184 suites, 4733 passed). npm run lint reports 0 errors, npm run build compiles, and the full Playwright suite passes (303, including the new spec).

[cristim]

Delta re-review: APPROVE at 406cfbe (vs 7d21af5). One non-blocking follow-up below.

Red check: the PR spec purchase-failure-closes-modal.spec.ts was run against a full 7d21af5 frontend/src build. 4 of its 5 cases fail at 7d21af5, each on expect(#purchase-modal).toBeHidden(). All 5 pass at the head, so the blocker from the last round is fixed.

Exact-head gate:

• jest: 184 suites, 4733 passed, 1 skipped;
• lint: 0 errors;
• tsc --noEmit: clean;
• build: clean;
• full Playwright: 304 of 304 passed.

Attacks I ran with a review-only Playwright probe (every POST was held or answered by page.route):

• Double-submit: I double-clicked Execute and then clicked confirm twice. Exactly 1 POST went out.
• Close and reopen while a request is in flight: after A's POST, I closed the modal and reopened it. Execute stays disabled (data-submitting="true", title "Purchase submission in progress") until A settles. So the reopened modal can't send a second POST while A is pending, and the duplicate-POST window I was looking for can't be reached through the UI.
• A stale may-have-run failure: it does not close a modal opened after the request was sent (getPurchaseModalSession() === modalSession, app.ts:617). The unit test at purchase-execution-toast.test.ts:335 covers this.

Non-blocking follow-up (from reading the code; the probe stopped at the disabled button, so I haven't run this end to end):

1. The user starts A, then closes and reopens the modal for the same selection.
2. A fails with a may-have-run error (5xx, 409, or an execution_id).
3. The session check leaves the new modal open, which is right. But the finally (app.ts:622-623) re-enables Execute in that modal.
4. One more click can then submit again for a purchase that may already have run, which is the case this PR closes for the same session.

One possible fix: when the attempt may have run and the session has changed, clear the new modal's selection, or keep Execute disabled. The success path (app.ts:549) still closes whatever modal is open, which is the pre-existing F5 behaviour and fails safe. Worth a small issue rather than another round here.

[cristim]

Cold re-review: claude-qa-ui-3, verdict APPROVE at 406cfbe

This reviews the delta 7d21af5b..406cfbe5 (e5d4caf and 406cfbe). My blocking finding at 7d21af5 is resolved.

Fix

handleExecutePurchase captures getPurchaseModalSession() before the request and closes only if it is unchanged afterwards (app.ts). The session is purchaseModalOpenToken. It is bumped on every open (recommendations.ts:4652, :5496) and never on close, so a response can only act on the modal session that sent it. It's a number, not an array, so the identity problem is gone.

Attacks (real bundle, mocked APIs only)

I added reviewer-only Playwright probes; they're not committed.

1. Open, close and reopen during an in-flight execute. The execute route is held. The user clicks the modal's Cancel (#close-purchase-modal-btn) and reopens the modal for the same selection, then the stale request is released with a 502 and execution_id.
   • The error toast shows and the newly opened modal stays visible. Pass.
   • Note: Escape doesn't close the modal at that point, because focus isn't inside it after the confirm dialog. Cancel and X are the reachable close paths.
2. Double-submit. While the request is held, Execute is disabled and a forced second click sends nothing: exactly 1 POST before and after the response. The modal closes on the 502. Pass.
3. The PR's spec at the head: 502+id, 403+id, 500 and 409 close the modal, and 400 keeps it open. 5 passed. With the probes and the existing purchase specs, the run was 12 passed.
4. Fail-before. I rebuilt 7d21af5's whole frontend/src (a fresh dist/, 15 JS files) and ran the PR's spec against it. "502 with execution_id closes the review modal" and "403 with execution_id closes the review modal" fail on both tries, at expect(#purchase-modal).toBeHidden(). My output filter truncated after those two, so I didn't capture the 500 and 409 results there.

Setup note: my first previous-head run had silently served the head's bundle, because that build failed type-checking the head's test files. The numbers above come from the verified rebuild.

Also checked

• F2. STATUS_CLAMP_CHARS = 80, so the "Full message" <details> appears for anything the phone card's 4-line clamp can cut. It's still keyboard- and touch-operable.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 184 suites, 4733 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.

No findings. Not merged.

[cristim]

Local verification at 406cfbe58aa66b8cf5f990e7f40da20719c9023b (macOS, by the author's lead agent)

• At the head: npm test passes (184 suites, 4733 passed). npm run lint reports 0 errors, npm run build compiles, and the full Playwright suite passes (303). That includes purchase-failure-closes-modal.spec.ts on the real bundle, which failed at 7d21af5b.
• Combined with current main: main 8eee9009 (fix(dashboard): say what Home's savings figures and empty states mean #1707, fix(frontend): show service display names instead of raw slugs #1710, feat(ri-exchange): name the RIs' account in the RI table and Exchange dialog #1714, fix(dashboard): call the Active Commitments figure estimated savings #1716 and others touch the frontend) merged with this head auto-merges cleanly. npm test passes (186 suites, 4780 passed), lint reports 0 errors, the build compiles and the full Playwright suite passes (320).
• CI: green at this head (41 checks).
• Reviews: APPROVE at this SHA from claude-qa-ui-3 and as-qa-2.
• Not verified: a real refused purchase in production. The Playwright spec reproduces it with the API shapes production returns.
• Follow-ups: fix(purchases): a late execute response re-enables Execute in a newer review modal #1724 (finally re-enables Execute in a newer modal). fix(purchases): a refused AWS purchase shows a viewport-tall toast, re-arms Execute, warns of a possible duplicate and counts as a purchase #1694 stays open for item 3.