feat(billing): top-up dialog confirm step and outcome states - #14526
Draft
comfydesigner wants to merge 10 commits into
Draft
feat(billing): top-up dialog confirm step and outcome states#14526comfydesigner wants to merge 10 commits into
comfydesigner wants to merge 10 commits into
Conversation
…pport Branch rebuilt on main past #14490 (top-up 3DS verification). The verification CTA integrates into the step machine per the confirm-flow sequencing: it renders grouped above the primary action on the amount and confirm steps whenever a pending operation carries an action_url, demoting that action to tertiary; success/declined steps are unaffected. Ports the #14490 verification tests and adds coverage that verification stays reachable on the confirm step.
Reopening the dialog mid-verification previously landed on the amount step
with the verification stack attached, inviting a second charge while one was
in flight. With a pending action_url (and top-up permission) the dialog now
opens onto a dedicated verifying step — DES-592's settled copy ('Verify your
payment' / bank-verification line) with Complete verification as the only
action. The remounted dialog cannot know the pending charge's amount, so the
step shows none rather than the steppers' defaults.
A pending charge that later succeeded or failed during polling left the dialog stranded on the confirm step with only the store's toast. handleBuy now awaits the operation's terminal state: succeeded refreshes balances and lands on the success step; failed lands on the declined step carrying the operation's error message. Timeout keeps the current behavior (the pending op persists inside the authentication window).
Declined is the flow's one non-terminal outcome — no charge happened — so it gets the retry path back to the confirm step (amount and method intact), clearing the stale decline reason. Success and verifying stay back-less: one is terminal, the other is blocked on the pending charge. Declined title joins the success/verifying type scale.
savedMethods becomes a nullable prop (the BE-contract seam, mirroring the subscription confirm's savedMethodsForConfirm): null keeps the confirm step unreachable, so production behavior is unchanged until the parent supplies the list. The URL/keyboard demo hooks and mock methods are gone; tests arm the flow through the prop.
Contributor
📝 WalkthroughWalkthroughThe workspace top-up dialog now supports confirmation, saved payment methods, verification, success and declined states, billing refreshes, and expanded localization. Tests cover the updated payment and operation flows. ChangesWorkspace top-up flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceUser
participant TopUpCreditsDialogContentWorkspace
participant BillingContext
participant SubscriptionManagement
WorkspaceUser->>TopUpCreditsDialogContentWorkspace: Select amount and payment method
TopUpCreditsDialogContentWorkspace->>BillingContext: Start top-up operation
BillingContext-->>TopUpCreditsDialogContentWorkspace: Return operation status
TopUpCreditsDialogContentWorkspace->>BillingContext: Poll pending operation
BillingContext-->>TopUpCreditsDialogContentWorkspace: Return success or failure
WorkspaceUser->>SubscriptionManagement: Open payment-method management
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Backing out of the confirm step during a pending verification landed on the amount step with live steppers — the eventual 3DS completion charges the original amount, not whatever the steppers now show (Hunter's repro). With a pending action_url, back now routes to the verifying step, making the amount step unreachable mid-verification by both reopen and back.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue (1)
546-551: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep topup tracking clearing consistent on the success step.
The top-right X calls
handleClose()withclearTracking = true, while the success “Close” button and billing link callhandleClose(false). Make clearing deterministic after purchase succeeds, for example by skippingclearTopupTracking()whenstep.value === 'success'.🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue` around lines 546 - 551, Update handleClose so clearTopupTracking is skipped whenever step.value is 'success', including calls that pass clearTracking as false or true. Preserve tracking cleanup for non-success steps and keep dialogStore.closeDialog unchanged.
🤖 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 `@src/locales/en/main.json`:
- Line 2759: Update the translation value for monthlyCreditsRollover to use the
verb phrase “roll over” instead of “rollover,” matching the wording already used
by monthlyCreditsInfo.
- Around line 2770-2773: Update the locale entries partnerNodesBalance,
partnerNodesCredits, and partnerNodesPricingTable to use the singular,
capitalized compound adjective “Partner Node” before the following noun, while
preserving “Partner Nodes” only when it stands alone as a noun.
In
`@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts`:
- Around line 433-449: Add a declined-payment test alongside the existing
rejected-charge test that clicks the declined-step “Back” button, verifies the
dialog returns to the confirmation step, and confirms the displayed decline
reason is cleared. Reuse the existing dialog setup and payment-decline flow
helpers such as mockTopup, renderDialog, clickAddCredits, and the relevant
accessible button names.
In `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 463-468: Wrap the manageSubscription() calls in both
handleChangePaymentMethod and the watch(selectedMethodId, ...) handler with the
same try/catch/finally error-handling pattern used by handleBuy. Ensure rejected
promises are routed through the existing handled error state and any required
cleanup runs in finally, while preserving the current payment-method selection
behavior.
- Around line 400-405: Export the SavedPaymentMethod interface in
TopUpCreditsDialogContentWorkspace.vue. In
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts
lines 176-182, import SavedPaymentMethod and define the VISA and MASTERCARD
fixtures using satisfies SavedPaymentMethod instead of as const.
- Around line 130-137: Replace the raw billing <button> in the billing template
with the shared Button component, matching the variant="link" and size="lg"
configuration used by handleChangePaymentMethod. Preserve the existing
handleViewBilling click behavior and translated billingAndInvoices label.
- Around line 581-602: Extract the shared success-finalization block from the
completed branch and the pending-to-succeeded branch into a local helper near
the surrounding top-up flow. Have the helper refresh balance and status, assign
successSummary, and set step.value to 'success', then replace both duplicated
blocks with calls to that helper while preserving existing control flow.
---
Outside diff comments:
In `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 546-551: Update handleClose so clearTopupTracking is skipped
whenever step.value is 'success', including calls that pass clearTracking as
false or true. Preserve tracking cleanup for non-success steps and keep
dialogStore.closeDialog unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9114d1df-3041-49dc-bdb3-70d30ee333a7
📒 Files selected for processing (4)
src/components/ui/stepper/FormattedNumberStepper.vuesrc/locales/en/main.jsonsrc/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.tssrc/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue
| "monthlyCreditsInfo": "These credits refresh monthly and don't roll over", | ||
| "monthlyCreditsLabel": "Monthly credits", | ||
| "monthlyCreditsPerMemberLabel": "Monthly credits / member", | ||
| "monthlyCreditsRollover": "These credits will rollover to the next month", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Fix verb form of "roll over".
monthlyCreditsRollover reads "These credits will rollover to the next month". "Rollover" is a noun/adjective. Use the verb phrase "roll over", as monthlyCreditsInfo does three lines above.
✏️ Proposed wording fix
- "monthlyCreditsRollover": "These credits will rollover to the next month",
+ "monthlyCreditsRollover": "These credits will roll over to the next month",📝 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
| "monthlyCreditsRollover": "These credits will rollover to the next month", | |
| "monthlyCreditsRollover": "These credits will roll over to the next month", |
🤖 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 `@src/locales/en/main.json` at line 2759, Update the translation value for
monthlyCreditsRollover to use the verb phrase “roll over” instead of “rollover,”
matching the wording already used by monthlyCreditsInfo.
Comment on lines
+2770
to
+2773
| "partnerNodesBalance": "\"Partner Nodes\" Credit Balance", | ||
| "partnerNodesCredits": "Partner nodes pricing", | ||
| "partnerNodesDescription": "For running commercial/proprietary models", | ||
| "partnerNodesPricingTable": "Partner Nodes pricing table", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Fix inconsistent "Partner Node(s)" wording.
partnerNodesCredits reads "Partner nodes pricing" and partnerNodesPricingTable reads "Partner Nodes pricing table". Both use "Partner Node(s)" as a compound adjective before "pricing". Keep it singular and capitalized: "Partner Node pricing" and "Partner Node pricing table". Apply the same singular form to partnerNodesBalance, since "Partner Nodes" there modifies "Credit Balance".
Based on learnings, "Partner Node(s)" as a compound adjective modifying another noun (e.g. "Partner Node pricing badge") stays singular; use "Partner Nodes" only as a standalone noun referring to the node set.
✏️ Proposed wording fix
- "partnerNodesBalance": "\"Partner Nodes\" Credit Balance",
- "partnerNodesCredits": "Partner nodes pricing",
+ "partnerNodesBalance": "\"Partner Node\" Credit Balance",
+ "partnerNodesCredits": "Partner Node pricing",
"partnerNodesDescription": "For running commercial/proprietary models",
- "partnerNodesPricingTable": "Partner Nodes pricing table",
+ "partnerNodesPricingTable": "Partner Node pricing table",📝 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
| "partnerNodesBalance": "\"Partner Nodes\" Credit Balance", | |
| "partnerNodesCredits": "Partner nodes pricing", | |
| "partnerNodesDescription": "For running commercial/proprietary models", | |
| "partnerNodesPricingTable": "Partner Nodes pricing table", | |
| "partnerNodesBalance": "\"Partner Node\" Credit Balance", | |
| "partnerNodesCredits": "Partner Node pricing", | |
| "partnerNodesDescription": "For running commercial/proprietary models", | |
| "partnerNodesPricingTable": "Partner Node pricing table", |
🤖 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 `@src/locales/en/main.json` around lines 2770 - 2773, Update the locale entries
partnerNodesBalance, partnerNodesCredits, and partnerNodesPricingTable to use
the singular, capitalized compound adjective “Partner Node” before the following
noun, while preserving “Partner Nodes” only when it stands alone as a noun.
Source: Learnings
Comment on lines
+433
to
+449
| it('shows the declined step with the failure reason after a rejected charge', async () => { | ||
| mockTopup.mockRejectedValue(new Error('Insufficient funds')) | ||
|
|
||
| renderDialog({ savedMethods: [VISA] }) | ||
| await clickAddCredits() | ||
| const user = userEvent.setup() | ||
| await user.click(screen.getByRole('button', { name: 'Pay $50.00' })) | ||
|
|
||
| expect(screen.getByText('Payment declined')).toBeInTheDocument() | ||
| expect(screen.getByText('Insufficient funds')).toBeInTheDocument() | ||
| expect(mockToastAdd).not.toHaveBeenCalled() | ||
|
|
||
| await user.click( | ||
| screen.getByRole('button', { name: 'Update payment method' }) | ||
| ) | ||
| expect(mockManageSubscription).toHaveBeenCalledOnce() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a test for the declined-step retry flow.
The PR adds "Declined-step retry navigation back to confirmation," but no test in this diff clicks "Back" from the declined step and verifies the dialog returns to confirm with declineReason cleared. Add a test alongside these declined-payment tests.
Do you want me to generate this test?
As per path instructions, "Ensure behavioral coverage for every new flow: amount, confirmation, saved-method selection/null handling, verification reopening, polling, success, decline, retry, balance updates, and payment-method management."
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts`
around lines 433 - 449, Add a declined-payment test alongside the existing
rejected-charge test that clicks the declined-step “Back” button, verifies the
dialog returns to the confirmation step, and confirms the displayed decline
reason is cleared. Reuse the existing dialog setup and payment-decline flow
helpers such as mockTopup, renderDialog, clickAddCredits, and the relevant
accessible button names.
Source: Path instructions
Comment on lines
+130
to
+137
| <template #billing> | ||
| <button | ||
| class="cursor-pointer border-none bg-transparent p-0 font-inter text-sm text-base-foreground underline-offset-2 hover:underline" | ||
| @click="handleViewBilling" | ||
| > | ||
| {{ $t('subscription.billingAndInvoices') }} | ||
| </button> | ||
| </template> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the shared Button component for the billing link.
The success step renders a raw <button> for the "Billing & invoices" link, while handleChangePaymentMethod's "Change" control uses <Button variant="link" size="lg"> at line 67 in the same file. Use the same shared component for consistency.
♻️ Proposed fix
- <button
- class="cursor-pointer border-none bg-transparent p-0 font-inter text-sm text-base-foreground underline-offset-2 hover:underline"
- `@click`="handleViewBilling"
- >
- {{ $t('subscription.billingAndInvoices') }}
- </button>
+ <Button
+ variant="link"
+ size="sm"
+ class="h-auto p-0 text-sm"
+ `@click`="handleViewBilling"
+ >
+ {{ $t('subscription.billingAndInvoices') }}
+ </Button>📝 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
| <template #billing> | |
| <button | |
| class="cursor-pointer border-none bg-transparent p-0 font-inter text-sm text-base-foreground underline-offset-2 hover:underline" | |
| @click="handleViewBilling" | |
| > | |
| {{ $t('subscription.billingAndInvoices') }} | |
| </button> | |
| </template> | |
| <template `#billing`> | |
| <Button | |
| variant="link" | |
| size="sm" | |
| class="h-auto p-0 text-sm" | |
| `@click`="handleViewBilling" | |
| > | |
| {{ $t('subscription.billingAndInvoices') }} | |
| </Button> | |
| </template> |
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 130 - 137, Replace the raw billing <button> in the billing template
with the shared Button component, matching the variant="link" and size="lg"
configuration used by handleChangePaymentMethod. Preserve the existing
handleViewBilling click behavior and translated billingAndInvoices label.
Comment on lines
+400
to
+405
| interface SavedPaymentMethod { | ||
| id: string | ||
| type: 'card' | 'alipay' | 'bank' | ||
| brand?: string | ||
| last4?: string | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Export SavedPaymentMethod so the test file can type its fixtures. The SavedPaymentMethod interface is declared but not exported, so the test file cannot import it and instead relies on as const for its fixtures, losing shape validation against the real interface.
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue#L400-L405: Addexportto theSavedPaymentMethodinterface declaration.src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts#L176-L182: ImportSavedPaymentMethodand defineVISA/MASTERCARDwithsatisfies SavedPaymentMethodinstead ofas const.
As per path instructions, "Use explicit TypeScript types for saved payment methods... and test fixtures explicitly."
📍 Affects 2 files
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue#L400-L405(this comment)src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts#L176-L182
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 400 - 405, Export the SavedPaymentMethod interface in
TopUpCreditsDialogContentWorkspace.vue. In
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts
lines 176-182, import SavedPaymentMethod and define the VISA and MASTERCARD
fixtures using satisfies SavedPaymentMethod instead of as const.
Source: Path instructions
Comment on lines
+463
to
+468
| watch(selectedMethodId, async (id, previous) => { | ||
| if (id === 'add-new') { | ||
| selectedMethodId.value = previous | ||
| await manageSubscription() | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add error handling around manageSubscription() calls.
handleChangePaymentMethod and the watch(selectedMethodId, ...) handler both await manageSubscription() with no try/catch. handleBuy wraps its async work in try/catch/finally; these two call sites do not, so a rejection surfaces as an unhandled promise rejection instead of a handled error state.
🛡️ Proposed fix
async function handleChangePaymentMethod() {
- await manageSubscription()
+ try {
+ await manageSubscription()
+ } catch (error) {
+ console.error('Failed to open billing portal:', error)
+ }
}Also applies to: 542-544
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 463 - 468, Wrap the manageSubscription() calls in both
handleChangePaymentMethod and the watch(selectedMethodId, ...) handler with the
same try/catch/finally error-handling pattern used by handleBuy. Ensure rejected
promises are routed through the existing handled error state and any required
cleanup runs in finally, while preserving the current payment-method selection
behavior.
Source: Coding guidelines
Comment on lines
581
to
+602
| await Promise.allSettled([fetchBalance(), fetchStatus()]) | ||
| handleClose(false) | ||
| settingsDialog.show('workspace') | ||
| successSummary.value = { | ||
| previous: previousCredits, | ||
| added: creditsModel.value | ||
| } | ||
| step.value = 'success' | ||
| } else if (response.status === 'pending') { | ||
| billingOperationStore.startOperation(response.billing_op_id, 'topup') | ||
| const operation = await billingOperationStore.startOperation( | ||
| response.billing_op_id, | ||
| 'topup' | ||
| ) | ||
| if (operation.status === 'succeeded') { | ||
| await Promise.allSettled([fetchBalance(), fetchStatus()]) | ||
| successSummary.value = { | ||
| previous: previousCredits, | ||
| added: creditsModel.value | ||
| } | ||
| step.value = 'success' | ||
| } else if (operation.status === 'failed') { | ||
| declineReason.value = operation.errorMessage | ||
| step.value = 'declined' | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated success-finalization logic.
The completed branch and the pending -> succeeded branch both refresh balance/status, set successSummary, and set step.value = 'success' with identical code. Extract a helper to avoid the duplication and reduce drift risk if the success flow changes later.
♻️ Proposed refactor
+async function finalizeSuccess(previousCredits: number) {
+ await Promise.allSettled([fetchBalance(), fetchStatus()])
+ successSummary.value = {
+ previous: previousCredits,
+ added: creditsModel.value
+ }
+ step.value = 'success'
+}
+
if (response.status === 'completed') {
telemetry?.trackBillingEvent({ ... })
- await Promise.allSettled([fetchBalance(), fetchStatus()])
- successSummary.value = {
- previous: previousCredits,
- added: creditsModel.value
- }
- step.value = 'success'
+ await finalizeSuccess(previousCredits)
} else if (response.status === 'pending') {
const operation = await billingOperationStore.startOperation(
response.billing_op_id,
'topup'
)
if (operation.status === 'succeeded') {
- await Promise.allSettled([fetchBalance(), fetchStatus()])
- successSummary.value = {
- previous: previousCredits,
- added: creditsModel.value
- }
- step.value = 'success'
+ await finalizeSuccess(previousCredits)
} else if (operation.status === 'failed') {
declineReason.value = operation.errorMessage
step.value = 'declined'
}
}📝 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
| await Promise.allSettled([fetchBalance(), fetchStatus()]) | |
| handleClose(false) | |
| settingsDialog.show('workspace') | |
| successSummary.value = { | |
| previous: previousCredits, | |
| added: creditsModel.value | |
| } | |
| step.value = 'success' | |
| } else if (response.status === 'pending') { | |
| billingOperationStore.startOperation(response.billing_op_id, 'topup') | |
| const operation = await billingOperationStore.startOperation( | |
| response.billing_op_id, | |
| 'topup' | |
| ) | |
| if (operation.status === 'succeeded') { | |
| await Promise.allSettled([fetchBalance(), fetchStatus()]) | |
| successSummary.value = { | |
| previous: previousCredits, | |
| added: creditsModel.value | |
| } | |
| step.value = 'success' | |
| } else if (operation.status === 'failed') { | |
| declineReason.value = operation.errorMessage | |
| step.value = 'declined' | |
| } | |
| async function finalizeSuccess(previousCredits: number) { | |
| await Promise.allSettled([fetchBalance(), fetchStatus()]) | |
| successSummary.value = { | |
| previous: previousCredits, | |
| added: creditsModel.value | |
| } | |
| step.value = 'success' | |
| } | |
| await finalizeSuccess(previousCredits) | |
| } else if (response.status === 'pending') { | |
| const operation = await billingOperationStore.startOperation( | |
| response.billing_op_id, | |
| 'topup' | |
| ) | |
| if (operation.status === 'succeeded') { | |
| await finalizeSuccess(previousCredits) | |
| } else if (operation.status === 'failed') { | |
| declineReason.value = operation.errorMessage | |
| step.value = 'declined' | |
| } |
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 581 - 602, Extract the shared success-finalization block from the
completed branch and the pending-to-succeeded branch into a local helper near
the surrounding top-up flow. Have the helper refresh balance and status, assign
successSummary, and set step.value to 'success', then replace both duplicated
blocks with calls to that helper while preserving existing control flow.
Contributor
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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 539-542: Update the Back-navigation branch that checks
topupActionUrl.value to also require permissions.value.canTopUp before setting
step.value to 'verifying'; otherwise fall through to the amount step. Add a
regression test covering permission revocation after the action URL appears and
verify that Back returns to the amount step rather than reopening verification.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 232ee8b4-77e4-464f-b33c-eb07754d84a0
📒 Files selected for processing (2)
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.tssrc/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue
Comment on lines
+539
to
+542
| if (topupActionUrl.value) { | ||
| step.value = 'verifying' | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Apply the top-up permission check before reopening verification.
The initial state requires permissions.value.canTopUp. Line 539 drops that requirement. If permission is revoked while confirmation is open, Back opens the verification step and exposes the pending charge action.
Require permissions.value.canTopUp in this branch. Add a regression test that revokes permission after the action URL appears and verifies Back returns to the amount step.
Proposed fix
- if (topupActionUrl.value) {
+ if (topupActionUrl.value && permissions.value.canTopUp) {
step.value = 'verifying'
return
}📝 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
| if (topupActionUrl.value) { | |
| step.value = 'verifying' | |
| return | |
| } | |
| if (topupActionUrl.value && permissions.value.canTopUp) { | |
| step.value = 'verifying' | |
| return | |
| } |
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 539 - 542, Update the Back-navigation branch that checks
topupActionUrl.value to also require permissions.value.canTopUp before setting
step.value to 'verifying'; otherwise fall through to the amount step. Add a
regression test covering permission revocation after the action URL appears and
verify that Back returns to the amount step rather than reopening verification.
Back routed to the amount step during the window before the bank link arrives, and a user already sitting on the amount step never reached the verifying state at all. Back now routes to verifying whenever a top-up is polling; the verification CTA spins until the action_url arrives; and the amount step jumps to verifying the moment the link lands.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue (1)
597-612: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnter
verifyingimmediately for pending top-ups.A pending operation currently leaves the dialog on
confirm. The test encodes that incorrect manual-Back flow.
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue#L597-L612: setstep.value = 'verifying'whenstartOperation()returnspending.src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts#L356-L373: assert the verification view directly after the pending payment action. Do not require Back navigation.🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue` around lines 597 - 612, Update the pending branch in TopUpCreditsDialogContentWorkspace.vue around startOperation to set step.value to 'verifying' immediately when a pending operation is returned, while preserving the existing succeeded and failed handling. In src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts lines 356-373, assert the verification view directly after the pending payment action and remove the manual-Back navigation requirement.Source: Path instructions
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 450-452: Gate every transition into the verifying step in the
top-up flow on permissions.value.canTopUp, including the topupActionUrl watcher
and the polling transition around the existing verification logic. Reuse one
permission-gated computed condition for URL arrival and back navigation,
preserve the amount step when permission is denied, and add tests covering
revoked canTopUp permissions.
---
Outside diff comments:
In `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 597-612: Update the pending branch in
TopUpCreditsDialogContentWorkspace.vue around startOperation to set step.value
to 'verifying' immediately when a pending operation is returned, while
preserving the existing succeeded and failed handling. In
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.ts
lines 356-373, assert the verification view directly after the pending payment
action and remove the manual-Back navigation requirement.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbdac6fe-1112-4bd6-b866-beac553cf523
📒 Files selected for processing (2)
src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.test.tssrc/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue
Comment on lines
+450
to
+452
| watch(topupActionUrl, (url) => { | ||
| if (url && step.value === 'amount') step.value = 'verifying' | ||
| }) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require canTopUp before every verification transition.
Line 450 enters verifying when an action URL arrives after permission revocation. Lines 545-547 also enter verifying while polling without checking permission. Keep the initial-state permission rule consistent for URL arrival and back navigation.
Use one permission-gated computed condition for these transitions. Preserve the amount step when permissions.value.canTopUp is false.
As per path instructions, tests must cover permissions in the top-up flow.
Also applies to: 545-547
🤖 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 `@src/platform/workspace/components/TopUpCreditsDialogContentWorkspace.vue`
around lines 450 - 452, Gate every transition into the verifying step in the
top-up flow on permissions.value.canTopUp, including the topupActionUrl watcher
and the polling transition around the existing verification logic. Reuse one
permission-gated computed condition for URL arrival and back navigation,
preserve the amount step when permission is denied, and add tests covering
revoked canTopUp permissions.
Source: Path instructions
…light Matches the checkout dialog's polling-disabled back button. Back can't honestly return to the amount step mid-charge, and routing it to the verifying screen read as a surprise; the arrow now dims and locks until the operation resolves. The verifying-on-back routing stays underneath as a safety net.
After Complete verification opens the bank tab, the demoted pay spinner tile becomes the status row — spinner plus 'Waiting for your bank…' — and the verifying step's body swaps to 'Complete the verification in the new tab — this updates automatically.' The verification CTA stays enabled for reopening lost tabs; the flag only sets when the tab actually opened, so popup-blocked clicks keep the original copy.
This was referenced Aug 3, 2026
dante01yoon
added a commit
that referenced
this pull request
Aug 7, 2026
## Summary Adds deterministic browser coverage for the current-main partner-node, top-up, and responsive billing-cycle workflows. This PR is stacked on the isolated maximum-warning prerequisite #14815, which is itself stacked on #14713 so the Cloud Team recovery-banner slice remains independently reviewable. ## Why this is needed These journeys span stateful API polling, exact currency payloads, workspace-scoped balances, partner-node recovery, permission boundaries, and responsive pricing interactions. Main and #14713 did not deterministically exercise them, leaving regressions dependent on live billing state and timing. The maximum top-up test also exposed a current frontend bug: manually entering an amount at or above $10,000 clamps the input correctly, but the enterprise guidance was unreachable because the warning only appeared for values strictly greater than the maximum. ## Changes - Adds a typed stateful billing API fixture for deterministic status, balance, plans, preview, top-up, subscription, cancellation, and operation polling responses. - Adds partner-node coverage for price presentation, prompt execution request, sufficient-credit balance deduction/refresh, insufficient-credit recovery after a failed billing refresh, and member privacy/permissions. - Adds top-up coverage for exact-cent personal preset/custom payloads, active Team workspace scoping, pending completion, HTTP failure/retry, failed operation feedback, and the current $10,000/enterprise contract. - Adds monthly-to-yearly Creator pricing interaction through the direct supported pricing route on Cloud desktop and mobile, guarded by nonempty interactive-plan assertions. - Depends on #14815, which isolates the `showCeilingWarning` production prerequisite. ## Root cause and fix ### AS IS `showCeilingWarning` used `value > MAX_AMOUNT`. Because the input is clamped to `MAX_AMOUNT`, the observable value could never remain above the cap, hiding the warning and Contact us link at $10,000. [AS IS screenshot](https://ampcode.com/user-content/artifacts/6f5bcdc4d255307bdaed598273b88977160cebe70cc89f89baa56c5330e79c0f-file.png) ### TO BE The warning uses `value >= MAX_AMOUNT`, preserving the cap while exposing enterprise billing guidance at the reachable maximum. [TO BE screenshot](https://ampcode.com/user-content/artifacts/4ef8548ce8b54dee264330d27bcef202cc5633667fecacd04c46d170a3a4b44b-file.png) ## Review focus - Stateful billing operation polling and mocked balance refresh/deduction behavior. - Team member permission/privacy expectations. - The one-line maximum-warning prerequisite is isolated in #14815. - #14526 is an active draft for a future saved-payment-method UI contract with component tests; these browser tests remain scoped to current-main behavior and do not mock provider metering. - Workspace Activity API wiring and browser coverage are isolated in #14811 and #14813 rather than duplicated here. ## Validation - `pnpm typecheck` and `pnpm typecheck:browser` - Focused Cloud partner-node/top-up/pricing Playwright - Pricing mobile-chrome repeated 3 times and Cloud once after the responsive route fix - ESLint, oxfmt, oxlint, knip, and `git diff --check` ## Stacking - Base: `dante/topup-max-enterprise-warning` (#14815) - #14815 is based on `dante01yoon/test-billing-e2e-matrix` (#14713) - Workspace Activity prerequisite/tests: #14811 → #14813 --------- Co-authored-by: Amp <amp@ampcode.com>
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
There are significant merge conflicts
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
3 participants
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.
With a saved payment method the top-up charges immediately — the Stripe hand-off that acted as the implicit confirmation is gone. The dialog gains a step machine:
amount → confirm → success | declined, plus averifyingre-entry state for pending 3DS charges.Figma: New Top Up Flow (9 states, current tokens).
Live now (no BE dependency):
verifying(DES-592's copy) is the landing for reopen/back during any pending charge — the amount step is unreachable while money is in flight, so a stale amount can never be toggled against a pending 3DS charge (Hunter's repro)Ships dark (behind the saved-methods BE contract):
Pay {amount}.savedMethodsis a nullable prop — null (today's reality) keeps the dialog behaving exactly as on main[{ type, brand?, last4?, id, isDefault }]+ chosen method id on the charge; structured decline reason; op amount in status (verifying with amount, success on late resolution); challenge-completed polling granularity (window itself is BE-5770); cancel API for pending 3DS (abandon currently parks on Verifying until resolution — pre-existing lockout, now visible)22 component tests. Draft until the contract lands and the confirm step is reachable outside tests.
🤖 Generated with Claude Code