fix(ux/recommendations): rename 'Execute Purchase' to 'Send for Approval' (#288)#294
Conversation
…val' (closes #288) The purchase modal's primary action was labeled `Execute Purchase` but clicking it does NOT execute the purchase — it sends an approval email to the configured approver(s) and the row enters `pending_approval`. The actual upfront charge fires only when an approver clicks the link in that email. Users were re-clicking the button thinking nothing had happened, then panicking when an email arrived later. Changes: * `frontend/src/index.html:812` — button label flipped to `Send for Approval`. * `frontend/src/app.ts` — single-purchase + fan-out paths: - confirmDialog title `Execute N purchase(s)?` → `Send N purchase(s) for approval?` - confirmDialog body rewritten: the old "This will spend real money" framing was the OPPOSITE of what happens; new copy explains the email-approval contract explicitly. `destructive: false` since the immediate action is reversible (no money is moved). - confirmLabel `Execute purchases` / `Execute all` → `Send for approval` / `Send all for approval`. - Loading state `Executing...` / `Executing 0/N…` → `Sending...` / `Sending 0/N…`. - Button reset text mirrors the new label. * `frontend/src/recommendations.ts::openPurchaseModal` — added an inline `<p class="approval-required-note">` after the upfront-cost line in the Purchase Summary, so the user sees the contract BEFORE they click the primary action, not just inside the confirmDialog. Comment explicitly notes that #289 will introduce a session-permission branch where `execute-any:purchases` holders bypass the approval email; this PR's wording assumes the always-approval-required default. Tests: * New `describe('approval-required messaging (issue #288)')` block in `frontend/src/__tests__/recommendations.test.ts` with 3 cases: - Primary button reads `Send for Approval`, not `Execute Purchase`. - Modal body contains the explanatory note (`will email an approval request`). - The `.approval-required-note` element exists in the rendered modal. Belt-and-braces: also asserts no element on the rendered modal still carries the pre-#288 `Execute Purchase` text. * Test fixture in the file's shared `beforeEach` extended to include the `#execute-purchase-btn` button — production index.html includes it, the fixture had been omitting it. No other test in this file asserts against its absence; full suite (1464 tests) passes. Verification: - `npm test` clean (1464 / 1464 pass). - `npm run build` clean. - `npx tsc --noEmit` clean. Out of scope (filed as siblings): - #289 — permission-gated direct execute (the alternate label path this PR's TODO comment references). - #287 — pretty email links (the recipient-side polish). - #291 — post-execution notification email + cancel-link.
|
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:
📝 WalkthroughWalkthroughThis PR changes the purchase flow language and UX to reflect an approval-request workflow: modal primary button now reads "Send for Approval", modal body includes an approval-required note, confirmation dialogs and toasts were reworded to reference sending approval requests, and API responses now carry resolved ChangesPurchase Approval-request Flow (single cohesive change DAG)
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend (Browser)
participant Backend as Backend API
participant Email as Email Service
User->>Frontend: Click "Send for Approval"
Frontend->>Backend: POST /executePurchase (purchase data)
Backend->>Backend: validate & prepare approval email
Backend->>Email: sendPurchaseApprovalEmail(to)
Email-->>Backend: (sent / failed) + resolved recipient
Backend-->>Frontend: 200 { email_sent, email_reason?, approval_recipient? }
Frontend->>Frontend: show toast (include approval_recipient if present)
Frontend-->>User: UI feedback (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
cristim
added
priority/p2
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/app.ts`:
- Around line 285-288: The success toasts after the approval execute response
are still generic; update the code that handles the execute/send response (the
handler that currently builds the success toasts after calling execute/send for
localRecs) to read the recipient identity from the execute response (e.g.,
response.recipient or response.approverEmail) and include that identity in both
the single-record and multi-record (fan-out) success messages; modify the
success-toast construction in the same handler (the branches emitting the single
success toast and the loop/fan-out success toast) and repeat the same change for
the other identical success block (the other success-path around the fan-out
code) so both places show "Approval request sent to <recipient>" (with
pluralized text for multiple records).
🪄 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: ab9784e0-0e54-4e00-823b-2ff2dfea56dc
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/app.tsfrontend/src/index.htmlfrontend/src/recommendations.ts
…#294) Addresses CodeRabbit's actionable on PR #294 (`frontend/src/app.ts:285-288, 376-379`): the post-#288 success toasts still emit generic "check your email" copy, so the user can't confirm WHO received the approval request. The original issue called out naming the approver as a stated objective; this commit closes that gap. Backend (`internal/api/handler_purchases.go`): - Extend `sendPurchaseApprovalEmail` to also return the resolved To address (the recipient) so the response can surface it. Previously returned `(bool, string)` for `(emailSent, emailReason)`; now `(bool, string, string)` for `(emailSent, emailReason, recipient)`. Returns "" for recipient when resolution itself failed (no approvers configured) or when the email-notifier isn't wired; non-empty even on send failures AFTER resolution succeeded so the response can still tell the user who would have been notified. - Both response builders (executePurchase + retryPurchase) now include `approval_recipient` in the JSON response when non-empty. Frontend (`frontend/src/api/`, `frontend/src/app.ts`): - `PurchaseResult` (api/types.ts) and `RetryPurchaseResult` (api/purchases.ts) gain an optional `approval_recipient?: string` field. Optional so older deploys without the new field fall through to the legacy generic toast. - Single-purchase success toast (app.ts:347-362) reads `result.approval_recipient`; when present, "Approval request sent to <addr>." Otherwise the legacy line. - Fan-out success toast (app.ts:432-456) collects the unique set of recipients across the fulfilled bucket responses, dedupes, and renders one of three shapes: - 0 recipients (older deploy / all responses missing the field): legacy "submitted — check your email" message. - 1 recipient: "N purchase(s) sent for approval to <addr>." - 2+ recipients (multi-bucket purchase routing to distinct contact emails): "N purchase(s) sent for approval to K recipients (<sorted, comma-joined list>)." Verification: - `go test ./internal/api/...` clean (1055 pass). - `npm test` clean (1464 pass). - `npm run build` + `npx tsc --noEmit` clean. - The new `approval_recipient` field is optional on both wire shapes — pre-existing call sites that don't read it are unaffected, and the legacy toast still renders when the backend response omits it. Closes the actionable from CodeRabbit's review at #294 (comment)... (the inline comment around `app.ts:288`).
|
Addressed CodeRabbit's actionable:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@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/app.ts (1)
426-468:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat business-level email failures as fan-out failures.
A fulfilled
executePurchase()promise only means the POST returned 200. The backend can still returnemail_sent === false/status === 'failed', and this branch currently toasts “sent for approval” even when no approval email went out. Reclassify those responses before building the success toast.Suggested fix
- const succeeded = results.filter((r) => r.status === 'fulfilled').length; - const failed = results.length - succeeded; + const fulfilled = results.filter( + (r): r is PromiseFulfilledResult<api.PurchaseResult> => r.status === 'fulfilled', + ); + const submissionFailures = fulfilled.filter( + (r) => r.value.email_sent === false || r.value.status === 'failed', + ); + const succeeded = fulfilled.length - submissionFailures.length; + const failed = results.length - succeeded; closePurchaseModal(); clearFanOutBuckets(); clearPurchaseModalRecommendations(); if (failed === 0) { @@ } else { - const failureMsgs = results - .filter((r): r is PromiseRejectedResult => r.status === 'rejected') - .map((r) => (r.reason instanceof Error ? r.reason.message : String(r.reason))) + const failureMsgs = [ + ...results + .filter((r): r is PromiseRejectedResult => r.status === 'rejected') + .map((r) => (r.reason instanceof Error ? r.reason.message : String(r.reason))), + ...submissionFailures.map((r) => r.value.email_reason || 'approval email did not send'), + ] .slice(0, 3) .join('; '); showToast({ message: `${succeeded} of ${results.length} submitted · ${failed} failed: ${failureMsgs}${failed > 3 ? ' (…)' : ''}`, kind: failed === results.length ? 'error' : 'warning',🤖 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/app.ts` around lines 426 - 468, The success branch currently treats any PromiseFulfilledResult as a true success even when the backend indicates email_sent === false or value.status === 'failed'; update the post-processing of results (the loop using results, the succeeded/failed counters, and recipient set) to reclassify fulfilled responses whose value.email_sent === false or value.status === 'failed' as failures: decrement succeeded/increment failed for those entries, include their messages in failureMsgs (use r.value.error or a descriptive string), and only add r.value.approval_recipient into recipients when r.status === 'fulfilled' AND r.value.email_sent === true AND r.value.status !== 'failed'; finally ensure showToast uses the adjusted counts and recipients when building the success or failure toast.
🤖 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/app.ts`:
- Around line 426-468: The success branch currently treats any
PromiseFulfilledResult as a true success even when the backend indicates
email_sent === false or value.status === 'failed'; update the post-processing of
results (the loop using results, the succeeded/failed counters, and recipient
set) to reclassify fulfilled responses whose value.email_sent === false or
value.status === 'failed' as failures: decrement succeeded/increment failed for
those entries, include their messages in failureMsgs (use r.value.error or a
descriptive string), and only add r.value.approval_recipient into recipients
when r.status === 'fulfilled' AND r.value.email_sent === true AND r.value.status
!== 'failed'; finally ensure showToast uses the adjusted counts and recipients
when building the success or failure toast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f246897-78c1-4ac4-88cb-ecd2f1f501e7
📒 Files selected for processing (4)
frontend/src/api/purchases.tsfrontend/src/api/types.tsfrontend/src/app.tsinternal/api/handler_purchases.go
|
@coderabbitai — the outside-diff finding on The reclassification logic landed in commit 9bf6caf18 and is identical to your suggested diff:
The inline comment at line 426-428 even says It looks like the review at 15:22:51Z (Run ID Mirroring the global CR-loop exit criterion: the most recent review's only finding is satisfied. PR is loop-clean unless a new review surfaces something else. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
…ntity in success toast + reclassify email-send failures Finding 1 (single-record): the recipient-in-toast was already addressed in the prior commit (9bf6caf). Confirmed still present; no change needed. Finding 2 (fan-out): a fulfilled executePurchase POST returning email_sent === false or status === 'failed' was previously counted as a success even though no approval email was delivered. Fix: - Separate fulfilled results into truly-succeeded vs submission failures before computing the succeeded/failed counters. - Only add approval_recipient to the success toast's recipient set when email_sent !== false AND status !== 'failed'. - Include email_reason from submission failures in the failure toast message (alongside HTTP-rejection messages) via merged spread. Tests: 8 new tests in purchase-execution-toast.test.ts covering: - Single-record with recipient → named in toast - Single-record without recipient → generic fallback copy - Single-record email_sent:false → warning toast - Fan-out email_sent:false counts as failure, reason in toast, recipient excluded - Fan-out status:failed counts as failure, reason in toast, recipient excluded - Fan-out all-success → success toast lists unique recipients - Fan-out deduplication of same recipient across buckets - Fan-out all-email-fail → error toast with reason
CR pass on PR #294 — both Major findings addressedFinding 1 — Recipient identity in success toastThe single-record path ( No change was needed here — confirmed present and correct. Finding 2 — Business-level email failures reclassified as failures
Fix in
New tests (
|
| Test | Covers |
|---|---|
Finding 1 with approval_recipient |
Toast contains recipient name |
Finding 1 without approval_recipient |
Generic fallback copy |
email_sent: false single-record |
Warning toast |
Fan-out email_sent: false → failure counter |
Finding 2 — reason in toast, recipient excluded |
Fan-out status: 'failed' → failure counter |
Finding 2 |
| Fan-out all-success | Recipients listed in success toast |
| Fan-out duplicate recipient | Deduplication |
| Fan-out all-email-fail | Error toast |
All 1472 tests pass; tsc --noEmit clean; all pre-commit hooks passed.
@coderabbitai review
|
Tip For best results, initiate chat on the files or code changes.
The 8-test matrix in Triggering a fresh review against the latest commits now. [review] |
1 participant
Closes #288.
Why
The purchase modal's primary action was labeled
Execute Purchasebut clicking it does NOT execute the purchase — it sends an approval email to the configured approver(s); the actual upfront charge fires only when an approver clicks the link in that email. Users frequently re-click the button thinking nothing had happened, then panic when an email arrives later.What changed
frontend/src/index.html:812):Execute Purchase→Send for Approval.frontend/src/app.ts(single-purchase + fan-out paths):destructive: falsesince the immediate action is reversible (no money is moved at confirm-time).Execute N purchase(s)?→Send N purchase(s) for approval?(and the fan-out symmetric).Executing...→Sending....frontend/src/recommendations.ts::openPurchaseModal): added a<p class="approval-required-note">after the upfront-cost line, so the user sees the contract BEFORE they click the primary action.recommendations.ts:2745-2752notes that feat(api,recs): permission-gated direct purchase execute (execute-{any,own}) bypassing approval email, with cost warning + confirm gate #289 will introduce a session-permission branch whereexecute-any:purchasesholders opt into direct execution; until that lands, every user is on this path and the new wording is correct unconditionally.Test plan
npm test— 1464 / 1464 pass.npm run build— clean.npx tsc --noEmit— clean.describe('approval-required messaging (issue #288)')with 3 tests pinning the button label, the modal body, and the.approval-required-noteelement — plus a belt-and-braces "noExecute Purchasetext anywhere on the rendered modal" assertion to guard against future regressions.Out of scope
Notes for reviewers
beforeEachwas extended to include#execute-purchase-btn(it's in productionindex.htmlbut the test fixture had been omitting it). No other test in the file asserted against its absence; full suite green.destructive: falseis intentional — argue either way, but the priortruewas paired with the misleading "spend real money" body. With the corrected body, the dialog isn't communicating a destructive operation any more (it's a "send an email" operation).Summary by CodeRabbit
New Features
Tests