fix: preserve payment recovery rollback behavior - #14746
Conversation
Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019fc867-33d7-7463-b4d0-ab1c1cb297de
dosubot
Bot
added
the
size:L
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
🎭 Playwright: 🕵🏻 0 passed, 0 failed📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle Size
⚡ Performance
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPayment recovery now uses abortable billing requests, session validation, and stale-response protection. Recovery banners and dialogs react to feature-flag and permission changes. Queue controls restore their previous mode after recovery locks end. ChangesPayment recovery lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CloudRunButtonWrapper
participant useWorkspaceBilling
participant workspaceApi
User->>CloudRunButtonWrapper: Start payment recovery
CloudRunButtonWrapper->>useWorkspaceBilling: manageSubscription(signal)
useWorkspaceBilling->>workspaceApi: getPaymentPortalUrl(signal)
workspaceApi-->>useWorkspaceBilling: Portal response or abort
useWorkspaceBilling-->>CloudRunButtonWrapper: Open only current portal
CloudRunButtonWrapper-->>User: Update recovery dialog
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/dialogs/settings/BillingStatusBanner.test.ts`:
- Around line 338-352: Add a colocated Vitest test covering the unmount abort
path in the BillingStatusBanner test suite: start an unresolved
manageSubscription request, capture its AbortSignal, unmount the rendered
banner, and assert the signal is aborted. Keep the existing flag-rollback test
unchanged and target the component’s unmount cleanup behavior.
In `@src/platform/workspace/composables/deriveBillingBanner.test.ts`:
- Around line 57-78: Add a test in the deriveBillingBanner suite covering
billingControlEnabled: false with v1PaymentRecovery: true, using the existing
paymentFailed and paused fixtures to verify recovery banners remain visible
under the recovery-only rollout state. Keep the assertions aligned with the
expected banner values and existing test style.
In `@src/platform/workspace/composables/useBillingBanner.ts`:
- Around line 103-126: Update useBillingBanner to import and register
onScopeDispose, aborting paymentRefreshController and clearing it when the
shared composable scope is disposed. Preserve the existing flag-watcher cleanup
and ensure both fetchStatus/fetchBalance requests are canceled on flag changes
and final consumer unmount.
In `@src/platform/workspace/composables/useWorkspaceBilling.ts`:
- Around line 87-99: Update clearLoadingOnAbort and the surrounding billing
operation loading flow to track active operations centrally with loading tokens
or equivalent ownership state. Ensure aborting one request only releases that
request’s token and sets isLoading to false after the final pending operation
completes or aborts, including concurrent subscribe(), topup(), and refresh
operations. Add a regression test covering a pending billing operation alongside
an aborted refresh.
🪄 Autofix
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: 25f46090-7d6d-4f75-9a2d-7a1cd2b74714
📒 Files selected for processing (16)
src/components/actionbar/ComfyRunButton/CloudRunButtonWrapper.test.tssrc/components/actionbar/ComfyRunButton/CloudRunButtonWrapper.vuesrc/components/actionbar/ComfyRunButton/ComfyQueueButton.test.tssrc/components/actionbar/ComfyRunButton/ComfyQueueButton.vuesrc/composables/billing/types.tssrc/composables/billing/useBillingContext.tssrc/locales/en/main.jsonsrc/platform/workspace/api/workspaceApi.test.tssrc/platform/workspace/api/workspaceApi.tssrc/platform/workspace/components/dialogs/settings/BillingStatusBanner.test.tssrc/platform/workspace/components/dialogs/settings/BillingStatusBanner.vuesrc/platform/workspace/composables/deriveBillingBanner.test.tssrc/platform/workspace/composables/useBillingBanner.test.tssrc/platform/workspace/composables/useBillingBanner.tssrc/platform/workspace/composables/useWorkspaceBilling.test.tssrc/platform/workspace/composables/useWorkspaceBilling.ts
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #14746 +/- ##
==========================================
+ Coverage 79.51% 79.72% +0.20%
==========================================
Files 1795 1804 +9
Lines 116642 117464 +822
Branches 34027 33648 -379
==========================================
+ Hits 92751 93645 +894
+ Misses 23336 23261 -75
- Partials 555 558 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 56 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Amp-Thread-ID: https://ampcode.com/threads/T-019fc867-33d7-7463-b4d0-ab1c1cb297de Co-authored-by: Amp <amp@ampcode.com>
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/composables/useWorkspaceBilling.ts (1)
354-357: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winForward the portal abort signal to the API.
manageSubscription()passes the signal only to local cleanup and stale-response checks.workspaceApi.getPaymentPortalUrl(returnUrl)still makes the HTTP request without cancellation, so aborting recovery or stopping the scope leaves that request running. UpdategetPaymentPortalUrl()to accept an optionalsignal, pass it toworkspaceApiClient.post(...), and add a regression test verifying the API request receives and honors it.🤖 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/composables/useWorkspaceBilling.ts` around lines 354 - 357, Update manageSubscription to pass its AbortSignal into workspaceApi.getPaymentPortalUrl, extend getPaymentPortalUrl to accept an optional signal and forward it to workspaceApiClient.post, and add a regression test confirming the request receives and honors cancellation.
🤖 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/composables/useWorkspaceBilling.ts`:
- Around line 193-197: Update fetchBalance and fetchStatus in
useWorkspaceBilling to capture the active workspace ID when each request starts,
then verify it still matches before publishing either success data or errors,
alongside the existing request/abort guards. Add a regression test that switches
workspaces while a billing read is pending and confirms the old workspace cannot
update the new workspace’s balance, status, or error state.
---
Outside diff comments:
In `@src/platform/workspace/composables/useWorkspaceBilling.ts`:
- Around line 354-357: Update manageSubscription to pass its AbortSignal into
workspaceApi.getPaymentPortalUrl, extend getPaymentPortalUrl to accept an
optional signal and forward it to workspaceApiClient.post, and add a regression
test confirming the request receives and honors cancellation.
🪄 Autofix
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: c542749c-ac46-4f90-b5af-e94096b95f52
📒 Files selected for processing (6)
src/platform/workspace/components/dialogs/settings/BillingStatusBanner.test.tssrc/platform/workspace/composables/deriveBillingBanner.test.tssrc/platform/workspace/composables/useBillingBanner.test.tssrc/platform/workspace/composables/useBillingBanner.tssrc/platform/workspace/composables/useWorkspaceBilling.test.tssrc/platform/workspace/composables/useWorkspaceBilling.ts
Amp-Thread-ID: https://ampcode.com/threads/T-019fc867-33d7-7463-b4d0-ab1c1cb297de Co-authored-by: Amp <amp@ampcode.com>
|
Addressed the outside-diff portal cancellation finding in f052aa7: |
dante01yoon
added
needs-backport
Amp-Thread-ID: https://ampcode.com/threads/T-019fc867-33d7-7463-b4d0-ab1c1cb297de Co-authored-by: Amp <amp@ampcode.com>
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)
src/platform/workspace/components/dialogs/settings/BillingStatusBanner.vue (1)
143-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide and block recovery payment updates for non-managers.
When
kind.valueispaymentFailed, Line 155 always exposesupdatePayment. A member can then click the button aftercanManage.valuebecomes false, and Line 196 startsmanageSubscription()anyway. The watcher only aborts an existing request.Gate the recovery warning body and action on
canManage.value. Preserve the legacy behavior whenv1PaymentRecoveryis disabled. Add a regression test for permission loss in thepaymentFailedstate.Also applies to: 187-200
🤖 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/dialogs/settings/BillingStatusBanner.vue` around lines 143 - 155, Update the paymentFailed branch in BillingStatusBanner.vue so the recovery warning body and action are only exposed when canManage.value is true, while keeping the existing legacy v1PaymentRecovery-disabled behavior unchanged. Adjust the action returned from the state logic that feeds the updatePayment flow and ensure manageSubscription() is not reachable after permission is lost; also add a regression test covering permission loss while kind.value remains paymentFailed.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.
Outside diff comments:
In `@src/platform/workspace/components/dialogs/settings/BillingStatusBanner.vue`:
- Around line 143-155: Update the paymentFailed branch in
BillingStatusBanner.vue so the recovery warning body and action are only exposed
when canManage.value is true, while keeping the existing legacy
v1PaymentRecovery-disabled behavior unchanged. Adjust the action returned from
the state logic that feeds the updatePayment flow and ensure
manageSubscription() is not reachable after permission is lost; also add a
regression test covering permission loss while kind.value remains paymentFailed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fcfe877-ad5d-4878-8ce0-b0d4580717b9
📒 Files selected for processing (4)
src/components/actionbar/ComfyRunButton/CloudRunButtonWrapper.test.tssrc/composables/billing/useBillingContext.test.tssrc/platform/workspace/components/dialogs/settings/BillingStatusBanner.test.tssrc/platform/workspace/components/dialogs/settings/BillingStatusBanner.vue
Amp-Thread-ID: https://ampcode.com/threads/T-019fc867-33d7-7463-b4d0-ab1c1cb297de Co-authored-by: Amp <amp@ampcode.com>
|
Addressed the latest permission-loss finding in |
|
Current-head visual evidence for Route-mocked cloud app. The red plans-loading toast is capture-fixture noise from the billing-plans mock, not PR behavior.
|
huang47
left a comment
huang47
left a comment
There was a problem hiding this comment.
LGTM. Current-head rollback comparison: #14746 (comment)
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
There are failing tests and merge conflicts.
3 participants
Summary
Follow-up to #14701 that makes
v1_payment_recoverya complete runtime rollback boundary: flag OFF restores the pre-feature billing/payment experience, while flag ON keeps payment recovery unchanged.Root cause and why this is needed
The original gate controlled which recovery UI was rendered, but not every feature-owned side effect. Legacy paused/payment-failed banners remained hidden behind the older
billing_control_enabledderivation, and requests/callbacks started while the new flag was ON could finish after rollback. That allowed stale billing commits, dialog updates, queue mode, focus refreshes, or Stripe portal actions to survive atrue → falsetransition. A status-first portal-return refresh could also cancel its slower balance refresh.How this changes
billing_control_enabledwhenv1_payment_recoveryis OFF.manageSubscription()behavior when recovery is already OFF.Changes
true → false,true → false → true, workspace transition, and unmount paths.Review Focus
billing_control_enabledbanners and actions.Screenshots
These are the real cloud-app captures from #14701. This follow-up intentionally adds no new visual state: OFF returns to the original experience and ON retains the recovery experience.
AS IS / rollback target — original paused experience
TO BE when flag ON — recovery experience remains unchanged
Validation
true → false → true), switching workspaces, and unmounting within the same component lifetime. The cloud browser suite cannot reliably control those in-flight boundaries; colocated Vitest tests exercise each exact abort and stale-response boundary instead, while the existing cloud E2E suite remains green.pnpm typecheckoxlint --type-awareoxfmt --checkpnpm knip(pre-push hook)git diff --checkRollout / rollback