chore(ui/history): improve Retry toast wording - #708
Conversation
cristim
added
triaged
|
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)
📝 WalkthroughWalkthroughThe Retry click handler now inspects the ChangesApproval Workflow Toast Update
Sequence Diagram(s)sequenceDiagram
participant HistoryUI as History UI
participant API as api.retryPurchase
participant Toast as Toast system
HistoryUI->>API: POST retryPurchase({ force? })
API-->>HistoryUI: { status, email_sent, ... } (retryResult)
HistoryUI->>Toast: show success if email_sent===true or status in [pending, notified]
HistoryUI->>Toast: show warning otherwise
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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.
|
![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
🧹 Nitpick comments (1)
frontend/src/history.ts (1)
783-783: ⚡ Quick winUpdate the confirm dialog to match the new toast message.
The confirmation dialog states "This will create a new purchase execution from the same recommendations," but the success toast now says "Purchase request sent for approval." If retries create approval-required purchases (as the PR rationale states), this dialog message is inconsistent and should be updated.
📝 Suggested update for consistency
- ? 'This will create a new purchase execution from the same recommendations. The original failed row will be linked to the new attempt.' + ? 'This will create a new purchase request from the same recommendations, which will be sent for approval. The original failed row will be linked to the new attempt.'🤖 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/history.ts` at line 783, Update the confirm dialog text that currently reads "This will create a new purchase execution from the same recommendations. The original failed row will be linked to the new attempt." to match the new success toast ("Purchase request sent for approval") and the PR rationale: make the confirmation explicitly state that retrying will create a purchase request requiring approval (e.g. mention "a purchase request will be created and sent for approval" and that the failed row will be linked). Locate the string in frontend/src/history.ts (the exact literal above) and replace it with the clarified confirmation message used by the confirm dialog function/call so wording is consistent with the toast and approval flow.
🤖 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/history.ts`:
- Line 815: The success toast is shown unconditionally after calling the
retryPurchase flow; change the logic in frontend/src/history.ts around the retry
handler so that you inspect the API response (the object that contains
ApprovalToken, email_sent and status) and only call showToast({message:
'Purchase request sent for approval', ...}) when email_sent is true (or status
=== 'pending'); when email_sent is false (or status === 'failed') show an
appropriate failure toast (e.g., 'Failed to send approval email') or handle the
finalized failed execution path. Locate the retry call and the existing
showToast invocation and gate/branch based on response.email_sent and/or
response.status to match the backend behavior.
---
Nitpick comments:
In `@frontend/src/history.ts`:
- Line 783: Update the confirm dialog text that currently reads "This will
create a new purchase execution from the same recommendations. The original
failed row will be linked to the new attempt." to match the new success toast
("Purchase request sent for approval") and the PR rationale: make the
confirmation explicitly state that retrying will create a purchase request
requiring approval (e.g. mention "a purchase request will be created and sent
for approval" and that the failed row will be linked). Locate the string in
frontend/src/history.ts (the exact literal above) and replace it with the
clarified confirmation message used by the confirm dialog function/call so
wording is consistent with the toast and approval flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…onse The "Purchase request sent for approval" toast was shown unconditionally after a successful retry POST. The backend's RetryPurchaseResult already carries email_sent (bool) and status (string); when the approval email fails, the execution is created with status "failed" and email_sent false, but the toast previously still claimed approval was requested. Gate the toast: show success when email_sent===true or status==='pending'/'notified'; show a warning toast when email_sent===false or status indicates failure. Adds three new test cases covering the email-sent, status-only, and email-failed branches. Resolves CR finding on PR #708 (discussion_r3299766964).
CR finding addressed: gate retry toast on email_sent / statusCommit: 8c63d50 Change summary: Captured the
The Tests added: 3 new cases in @coderabbitai review |
|
(∪・ω・∪) ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/history.ts`:
- Around line 817-824: The condition computing emailOk in the retry handling
must give explicit email_sent === false precedence over status fallbacks; update
the logic in the block around the emailOk variable (where retryResult.email_sent
and retryResult.status are checked) so that if retryResult.email_sent === false
you treat emailOk as false (and show the warning toast), otherwise treat emailOk
as true when retryResult.email_sent === true or when email_sent is not
explicitly false and retryResult.status is 'pending' or 'notified'—this ensures
explicit failures override status-based success.
🪄 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: e293abcf-2779-4ed4-b277-5273f2e8c207
📒 Files selected for processing (2)
frontend/src/__tests__/history-retry-button.test.tsfrontend/src/history.ts
…in retry toast gating Guard the status-based emailOk fallback with !emailExplicitlyFailed so that a backend payload carrying email_sent===false always shows the warning toast, even when status is 'pending' or 'notified'. Add test case #14 covering the conflicting-payload scenario CR round 2 identified.
|
Round 2 CR finding addressed in 25ab8a1. Finding: Fix ( const emailExplicitlyFailed = retryResult.email_sent === false;
const emailOk = !emailExplicitlyFailed && (
retryResult.email_sent === true
|| retryResult.status === 'pending'
|| retryResult.status === 'notified'
);New test (case #14 in 1934 tests pass, TypeScript clean. @coderabbitai review |
|
The additional test case (case (ᵔᴥᵔ)ﾉ ✅ Actions performedReview triggered.
|
1 participant
Closes #707. Changes the toast string in frontend/src/history.ts:815 from 'Retry execution created' to 'Purchase request sent for approval' — friendlier and more accurate (the Retry creates an approval-required execution, not an execution itself). All 1932 frontend tests pass.
Summary by CodeRabbit
Bug Fixes
Tests