Repository navigation
Add iOS account deletion and legal links - #7645
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThis PR adds account deletion handling across the auth runtime, iOS settings UI, backend DELETE route, and deletion-aware billing/auth guards, with tests and localized strings covering the new flow. ChangesAccount Deletion Flow
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR introduces permanent account deletion from iOS Settings backed by a new
Confidence Score: 3/5The deletion route is architecturally sound but carries real risk from confirmed gaps: ambiguous-completion cases (timeout, unknown) leave the user signed in to what may be a deleted account, the sheet can be dismissed mid-deletion causing the sign-out callback to silently drop, and several billing-path edge cases can leave Stripe state partially mutated on failure — all on an irreversible operation with no rollback. Multiple confirmed defects in the iOS sign-out sequencing and billing error paths exist in the current diff. The tombstone/advisory-lock skeleton is solid, VM teardown now collects failures instead of aborting, and localization is complete — but the known gaps in sign-out coverage for ambiguous outcomes and the billing snapshot race are real behavioral bugs on the changed paths, not speculative concerns.
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant iOS as iOS App
participant AC as AuthCoordinator
participant ADC as AccountDeletionClient
participant API as DELETE /api/account
participant DB as Postgres
participant Stack as Stack Auth
participant Stripe as Stripe
participant S3 as Vault S3
iOS->>AC: deleteAccount()
AC->>Stack: currentTokens()
Stack-->>AC: accessToken refreshToken
AC->>ADC: deleteAccount(tokens)
ADC->>API: DELETE /api/account
API->>DB: advisory lock + upsert tombstone pending
API->>Stack: getUser listTeams
API->>Stack: user.update cmuxAccountDeleting true
API->>Stripe: cancel subs or transfer ownership
API->>DB: revoke identity leases batched
API->>API: destroyVm x N failures collected
API->>S3: deleteObject batches
API->>API: deletePersonalSubrouterTenant
API->>DB: deleteCmuxOwnedAccountRows transaction
API->>DB: tombstone stack_delete_pending
API->>Stack: stackUser.delete()
alt Stack delete succeeds
API->>S3: finishPostStackAccountCleanup vault re-sweep
API->>DB: tombstone completed
API-->>ADC: 200 ok
iOS->>iOS: signOut and dismiss
else Stack delete fails
API->>DB: tombstone failed
API-->>ADC: 500 account_delete_retryable
iOS->>iOS: show retry alert no sign-out
else Post-stack cleanup fails
API->>DB: tombstone cleanup_incomplete
API-->>ADC: 202 cleanupIncomplete
iOS->>iOS: show alert sign out on acknowledge
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant iOS as iOS App
participant AC as AuthCoordinator
participant ADC as AccountDeletionClient
participant API as DELETE /api/account
participant DB as Postgres
participant Stack as Stack Auth
participant Stripe as Stripe
participant S3 as Vault S3
iOS->>AC: deleteAccount()
AC->>Stack: currentTokens()
Stack-->>AC: accessToken refreshToken
AC->>ADC: deleteAccount(tokens)
ADC->>API: DELETE /api/account
API->>DB: advisory lock + upsert tombstone pending
API->>Stack: getUser listTeams
API->>Stack: user.update cmuxAccountDeleting true
API->>Stripe: cancel subs or transfer ownership
API->>DB: revoke identity leases batched
API->>API: destroyVm x N failures collected
API->>S3: deleteObject batches
API->>API: deletePersonalSubrouterTenant
API->>DB: deleteCmuxOwnedAccountRows transaction
API->>DB: tombstone stack_delete_pending
API->>Stack: stackUser.delete()
alt Stack delete succeeds
API->>S3: finishPostStackAccountCleanup vault re-sweep
API->>DB: tombstone completed
API-->>ADC: 200 ok
iOS->>iOS: signOut and dismiss
else Stack delete fails
API->>DB: tombstone failed
API-->>ADC: 500 account_delete_retryable
iOS->>iOS: show retry alert no sign-out
else Post-stack cleanup fails
API->>DB: tombstone cleanup_incomplete
API-->>ADC: 202 cleanupIncomplete
iOS->>iOS: show alert sign out on acknowledge
end
Reviews (37): Last reviewed commit: "Clear stale deletion auth lockouts" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| console.error("account.delete.failed", error); | ||
| return jsonResponse({ error: "account_delete_failed" }, 500); | ||
| } | ||
| } | ||
|
|
||
| async function currentDeletableStackUser(request: Request): Promise<DeletableStackUser | null> { | ||
| if (!isStackConfigured()) return null; | ||
|
|
||
| const authHeader = request.headers.get("authorization"); |
There was a problem hiding this comment.
stackUser.delete() fails after DB rows are gone
deleteCmuxOwnedAccountRows runs inside a committed transaction; if it succeeds but stackUser.delete() then throws, the single catch returns { error: "account_delete_failed" } with HTTP 500. The iOS client shows "Couldn't Delete Account" — but the user's entire cmux dataset is already permanently erased. If the user interprets the error as "nothing changed" and walks away, they are left with a valid Stack auth account pointing at no cmux data: they can still sign in on another device and find an empty app, with no obvious path to complete the deletion.
A retry does eventually work (the idempotent DB deletes succeed with 0 rows, stackUser.delete() is called again), but the gap between the first 500 and a retry is a real broken-state window. Consider separating the Stack deletion failure into its own error path (e.g. a 202 with a background retry job or a distinct 500 body) so the client can communicate "data deleted, auth cleanup pending" rather than implying the whole operation rolled back.
| } | ||
|
|
||
| async function destroyPersonalCloudVms(userId: string): Promise<number> { | ||
| const vms = await runVmWorkflow(listUserVms(userId)); | ||
| for (const vm of vms) { |
There was a problem hiding this comment.
VMs are destroyed one-at-a-time; if destroyVm throws mid-loop (e.g. VM 2 of 3 returns an error), some VMs are already destroyed with no compensation possible. The outer catch returns 500, so the client sees "Couldn't Delete Account". On retry listUserVms may or may not return the already-destroyed VMs, and if destroyVm is not idempotent for an already-terminated VM, the retry can also fail. Consider either (a) collecting errors and continuing so the loop always attempts every VM before propagating failure, or (b) confirming that destroyVm is idempotent for already-terminated VMs and documenting that assumption.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Around line 388-390: No code change is needed for privacyPolicyURL,
termsOfServiceURL, or supportURL in MobileSettingsView; the literals are valid
and the privacy-policy target is confirmed live. If you touch these later, keep
the static URL constants in sync with the marketing site and support email, but
leave the current URL definitions unchanged.
In `@web/app/api/account/route.ts`:
- Around line 46-48: The error handling in account.delete.failed is logging the
raw error object, which can expose sensitive details. Update the catch block in
the account route to log only a sanitized summary using the error’s message or a
redacted string, rather than passing error directly to console.error. Keep the
existing jsonResponse behavior unchanged and make the logging change in the
route handler around the account.delete.failed message.
- Around line 34-39: `DELETE` in `web/app/api/account/route.ts` is parsing Stack
auth twice by calling both `verifyRequest` and `currentDeletableStackUser`,
which duplicates the bearer/refresh-token logic and adds a second `getUser(...)`
round-trip. Update `verifyRequest`/`AuthedUser` (or extract a shared helper) so
the initial auth check can also return the raw Stack user needed for
`.delete()`, then remove the separate `currentDeletableStackUser` lookup and
reuse the same parsed result in `DELETE`.
🪄 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
Run ID: 85c43860-a859-430e-b6f2-2c581ece1f43
📒 Files selected for processing (7)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+AccountDeletion.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftios/cmux/Resources/Localizable.xcstringsweb/app/api/account/route.tsweb/tests/account-route.test.ts
| private static let privacyPolicyURL = URL(string: "https://cmux.com/privacy-policy")! | ||
| private static let termsOfServiceURL = URL(string: "https://cmux.com/terms-of-service")! | ||
| private static let supportURL = URL(string: "mailto:feedback@manaflow.com?subject=cmux%20iOS%20support")! |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Static legal/support URLs.
privacyPolicyURL/termsOfServiceURL/supportURL are force-unwrapped literals; the privacy-policy URL was confirmed live and matches cmux's content. No action needed here, just flagging for completeness that hardcoded URLs like these should be kept in sync if the marketing site restructures its routes.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`
around lines 388 - 390, No code change is needed for privacyPolicyURL,
termsOfServiceURL, or supportURL in MobileSettingsView; the literals are valid
and the privacy-policy target is confirmed live. If you touch these later, keep
the static URL constants in sync with the marketing site and support email, but
leave the current URL definitions unchanged.
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)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift (1)
404-426: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUntracked fire-and-forget
Taskfor a multi-second destructive operation.The deletion
Task { ... }is not stored, cancelled, or tied to the view's lifecycle. If the sheet is dismissed mid-flight, there's no way to track/cancel or verify completion of this operation from outside the closure.Consider storing the task handle (e.g.
@State private var deleteAccountTask: Task<Void, Never>?) so its lifecycle is explicit and testable, even if cancellation itself isn't desired for a destructive action.Based on learnings and coding guidelines, "Flag fire-and-forget
Task { ... }work with meaningful lifecycle that is not stored, cancelled, or tied to a caller-owned operation."🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift` around lines 404 - 426, The deleteAccount() flow launches an untracked fire-and-forget Task, which makes this destructive operation’s lifecycle implicit and hard to verify. Update MobileSettingsView to keep a stored task handle (for example, a `@State` property like deleteAccountTask) and assign the Task to it from deleteAccount(), then clear it when the work finishes. Keep the existing authManager.deleteAccount(), signOut(), dismiss(), and failure-state handling, but make the task ownership explicit so the operation is tied to the view lifecycle and testable.Source: Coding guidelines
♻️ Duplicate comments (1)
web/app/api/account/route.ts (1)
34-39: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDuplicate Stack-auth parsing between
verifyRequestandcurrentDeletableStackUser(unresolved from prior review).
currentDeletableStackUserre-parses the bearer/refresh-token headers and makes a secondgetStackServerApp().getUser(...)call independently ofverifyRequest, doubling the round-trip to Stack per deletion request and creating two auth-parsing paths that must stay in sync. This was flagged in a prior review and doesn't appear to have a corresponding fix commit (unlike the siblingconsole.errorredaction comment, which is marked addressed).Also applies to: 65-81
🤖 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 `@web/app/api/account/route.ts` around lines 34 - 39, The DELETE flow is re-parsing Stack auth twice by calling both verifyRequest and currentDeletableStackUser, which duplicates the getStackServerApp().getUser lookup and creates two auth-parsing paths to maintain. Refactor the account route so DELETE reuses the user/auth data already obtained from verifyRequest, and update currentDeletableStackUser (or replace it with a helper used by verifyRequest) to avoid a second Stack round-trip while preserving the existing authorization check against stackUser.id.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Around line 404-426: The deleteAccount() flow launches an untracked
fire-and-forget Task, which makes this destructive operation’s lifecycle
implicit and hard to verify. Update MobileSettingsView to keep a stored task
handle (for example, a `@State` property like deleteAccountTask) and assign the
Task to it from deleteAccount(), then clear it when the work finishes. Keep the
existing authManager.deleteAccount(), signOut(), dismiss(), and failure-state
handling, but make the task ownership explicit so the operation is tied to the
view lifecycle and testable.
---
Duplicate comments:
In `@web/app/api/account/route.ts`:
- Around line 34-39: The DELETE flow is re-parsing Stack auth twice by calling
both verifyRequest and currentDeletableStackUser, which duplicates the
getStackServerApp().getUser lookup and creates two auth-parsing paths to
maintain. Refactor the account route so DELETE reuses the user/auth data already
obtained from verifyRequest, and update currentDeletableStackUser (or replace it
with a helper used by verifyRequest) to avoid a second Stack round-trip while
preserving the existing authorization check against stackUser.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 152c8686-7515-4c7b-8a6f-ed6b8774225e
📒 Files selected for processing (6)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+AccountDeletion.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftios/cmux/Resources/Localizable.xcstringsweb/app/api/account/route.tsweb/tests/account-route.test.ts
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)
Packages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swift (1)
31-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-retryable
.rejected(statusCode:)fallback.Tests cover unauthorized, both retryable-error JSON codes, and transport timeout, but not the default
.rejected(statusCode:)path (e.g. a 500 without a recognizederrorcode). This is the branch that distinguishes "safe to retry" from "hard failure" for a destructive account-deletion flow, so it's worth locking down explicitly.Suggested additional test
`@Test` func deleteAccountMapsUnrecognizedErrorToRejected() async { let client = AccountDeletionClient(apiBaseURL: "https://cmux.test") { request in ( Data(#"{"error":"some_other_error"}"#.utf8), HTTPURLResponse( url: request.url!, statusCode: 500, httpVersion: nil, headerFields: nil )! ) } await `#expect`(throws: AccountDeletionRequestError.rejected(statusCode: 500)) { try await client.deleteAccount(accessToken: "access-token", refreshToken: "refresh-token") } }🤖 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 `@Packages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swift` around lines 31 - 83, Add a test in AccountDeletionClientTests to cover the non-retryable fallback path in AccountDeletionClient.deleteAccount(accessToken:refreshToken:): when the server returns a 500 with an unrecognized JSON error code, the client should throw AccountDeletionRequestError.rejected(statusCode:). Reuse the existing mocking pattern from the other deleteAccountMaps… tests, but return a 500 response with an unknown error string, and assert the rejected(statusCode: 500) case so this hard-failure branch is locked down.
🤖 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 `@web/services/billing/purchase.ts`:
- Around line 183-204: In recordCheckoutCompletion handling inside purchase.ts,
do not use mappedStackUserId matching stackUserId as proof that a
stripeSubscriptions row already exists. The current hasLocalBillingRecord check
can skip or fail updates when customer.subscription.* or invoice.payment_failed
arrives before the checkout-created row. Update this branch to base the decision
on the actual subscription row existence (via userStripeSubscriptionExists or
equivalent) and, if missing, upsert/create the user-scoped subscription before
calling updateExistingStripeSubscription.
---
Outside diff comments:
In
`@Packages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swift`:
- Around line 31-83: Add a test in AccountDeletionClientTests to cover the
non-retryable fallback path in
AccountDeletionClient.deleteAccount(accessToken:refreshToken:): when the server
returns a 500 with an unrecognized JSON error code, the client should throw
AccountDeletionRequestError.rejected(statusCode:). Reuse the existing mocking
pattern from the other deleteAccountMaps… tests, but return a 500 response with
an unknown error string, and assert the rejected(statusCode: 500) case so this
hard-failure branch is locked down.
🪄 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
Run ID: 58325d4c-5aa1-4f6e-ad92-025b37a71703
📒 Files selected for processing (13)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+AccountDeletion.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhase.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AccountDeletionClientTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsAccountSection.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsLegalSupportSection.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftios/cmux/Resources/Localizable.xcstringsweb/app/api/account/route.tsweb/services/billing/purchase.tsweb/services/vms/auth.tsweb/tests/account-route.test.tsweb/tests/billing-purchase.test.tsweb/tests/vm-route-auth.test.ts
| readonly beforeExternalRequest?: () => void; | ||
| readonly afterExternalMutation?: () => Promise<void>; | ||
| } = {}, | ||
| ): Promise<void> { | ||
| const db = cloudDb(); | ||
| const deletionTeamIds = uniqueNonEmptyStrings([userId, ...accountTeamIds]); | ||
| const retainedOwnerByTeam = new Map( | ||
| retainedTeamBillingOwners.map((owner) => [owner.stackTeamId, owner.stackUserId] as const), | ||
| ); | ||
| const subscriptionRows = await db | ||
| .select({ | ||
| id: stripeSubscriptions.id, | ||
| stackTeamId: stripeSubscriptions.stackTeamId, | ||
| scope: stripeSubscriptions.scope, | ||
| status: stripeSubscriptions.status, | ||
| }) | ||
| .from(stripeSubscriptions) | ||
| .where(or( | ||
| eq(stripeSubscriptions.stackUserId, userId), | ||
| inArray(stripeSubscriptions.stackTeamId, deletionTeamIds), | ||
| )); | ||
| const activeSubscriptions = subscriptionRows.filter((subscription) => | ||
| stripeSubscriptionBelongsToDeletingAccount(subscription, deletionTeamIds) && |
There was a problem hiding this comment.
beforeExternalRequest fires before non-destructive Stripe transfers, triggering the wrong error path
The beforeExternalRequest callback — which sets both restoreBillingEntitlementsOnFailure = false and destructiveCleanupStarted = true — fires before the very first Stripe API call in resolveUserBillingForAccountDeletion. When retainedTeamCustomers is non-empty, that first call is client.customers.update(...) (an ownership transfer, not a deletion). If it fails transiently (Stripe 503, network timeout), the outer catch block sees destructiveCleanupStarted = true and cmuxOwnedRowsDeleted = false, so it returns { error: "account_delete_retryable" } with HTTP 500 — the same code used when real data destruction has partially completed.
The iOS client maps account_delete_retryable to stackDeleteIncomplete, which shows: "Your cmux data was deleted, but account sign-in cleanup did not finish." This is factually wrong — no cmux data was deleted. The user also cannot use any cmux API until they successfully retry (the metadata cmuxAccountDeleting: true stays set because restoreStackMetadataAfterAccountDeletionFailure is skipped).
Consider splitting the callback into beforeDestructiveRequest (fires only before subscriptions.cancel / customers.del) so that a failed ownership-transfer returns account_delete_failed (metadata restored, no lockout) rather than account_delete_retryable. Alternatively, use a distinct server error code that the iOS client maps to a neutral "retry" message without implying data loss.
…tion-app-review # Conflicts: # web/app/api/billing/checkout/route.ts
| }); | ||
|
|
||
| if ( | ||
| activeSubscriptions.length === 0 && | ||
| customers.length === 0 && | ||
| retainedTeamCustomers.length === 0 && | ||
| retainedTeamSubscriptions.length === 0 | ||
| ) return; | ||
| if (!isStripeBillingConfigured()) { | ||
| throw new Error("Stripe billing cleanup is not configured"); | ||
| } | ||
|
|
||
| const client = stripe(); | ||
| for (const customer of retainedTeamCustomers) { | ||
| const retainedOwnerId = retainedOwnerByTeam.get(customer.stackTeamId ?? ""); | ||
| if (!retainedOwnerId) throw new Error(`retained team billing owner missing for ${customer.stackTeamId}`); |
There was a problem hiding this comment.
afterExternalMutation fires on 404, skipping metadata restore on subsequent failure
When client.revokeTenant returns 404 the catch block still calls options.afterExternalMutation?.(), which at the call site sets destructiveCleanupStarted = true. If no prior destructive work has occurred (empty billing rows, no ASC, no VMs, no vault objects) and all subrouter tenants 404, then destructiveCleanupStarted becomes true before any real mutation has taken place. A transient failure of the subsequent deleteCmuxOwnedAccountRows then hits the destructiveCleanupStarted branch in the outer catch, which skips restoreStackMetadataAfterAccountDeletionFailure and returns { error: "account_delete_retryable" }. The user's Stack metadata is left with cmuxAccountDeleting: true and no cmuxPlan, blocking all non-deletion API calls, while the iOS client displays "Your cmux data was deleted" — factually wrong since nothing was deleted. The tombstone is marked failed (non-blocking), so a retry proceeds correctly, but the stuck metadata window is real. A 404 from the subrouter means the tenant was already absent; the callback should not be invoked in that branch.
| const accountScope = await accountDeletionScopeForUser(stackUser); | ||
| await finishPostStackAccountCleanup(userId, accountScope.teamIds); | ||
| await markAccountDeletionTombstoneCompleted(userId); | ||
| return jsonResponse({ ok: true, destroyedVms: 0 }, 200); |
There was a problem hiding this comment.
Cleanup resume needs deleted Stack user
Medium Severity
When a tombstone is cleanup_incomplete, a retry runs post-Stack cleanup only after currentDeletableStackUser succeeds and accountDeletionScopeForUser calls Stack team APIs. That state is recorded only after stackUser.delete() succeeds, so a normal client retry is usually unauthenticated and cannot finish orphaned cmux cleanup.
Reviewed by Cursor Bugbot for commit 47f7ea5. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e957eae. Configure here.
| } | ||
| try { | ||
| await finishPostStackAccountCleanup(userId, accountScope.teamIds); | ||
| await markAccountDeletionTombstoneCompleted(userId); |
There was a problem hiding this comment.
Stuck stack_delete_pending tombstone
Medium Severity
If the process dies after stackUser.delete() but before post-Stack cleanup updates the tombstone to cleanup_incomplete, the row can remain stack_delete_pending. Later DELETE calls cannot authenticate a deleted Stack user, and markAccountDeletionTombstonePending never resumes post-Stack cleanup for that status—only for cleanup_incomplete.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e957eae. Configure here.
The legacy-billing rebase over #7645 dropped the App Store distribution gate that hid the Stripe /api/billing/portal link for Pro users (Apple 3.1.1 bans external purchase links in App Store builds). Restore the !appStorePaymentGated gate and cover the Pro + App Store combination, which no test exercised.
* Remove legacy Stack billing entirely (checkout + recognition) cmux migrated from Stack Auth-hosted subscription products to direct Stripe billing. This rips out the legacy path completely: - Delete legacyStackCheckout and the /api/billing/confirm return route. Checkout with Stripe unconfigured now redirects to /pricing?billing=unavailable instead of the Stack hosted flow. - resolveProPlanStatus: Pro is now true iff there is an active row in stripe_subscriptions (user or team scope). Stack product subscriptions (customer.listProducts) no longer grant Pro; the VM entitlement check follows the same rule. - BillingManagementKind drops "external"; the "managed by our previous billing system, contact support" message and its keys are removed from services, the plan API, every billing UI, and all locale catalogs (en/ja/km/th). Existing legacy-only subscribers become Free (intentional; no migration). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * app-pricing: keep App Store gate on the Pro billing-portal link The legacy-billing rebase over #7645 dropped the App Store distribution gate that hid the Stripe /api/billing/portal link for Pro users (Apple 3.1.1 bans external purchase links in App Store builds). Restore the !appStorePaymentGated gate and cover the Pro + App Store combination, which no test exercised. * billing: restore Stripe pending banner state and gate availability on Stripe config Two regressions from removing legacy Stack billing: - /api/billing/complete (async payment) and /billing/success (subscription not yet active due to webhook race) still redirect to /pricing?welcome=pending, but the pending branch was dropped from ProWelcomeBanner, leaving paid users on a blank page. Restore the welcomePending message + a Check again link that reloads /pricing to re-check status (the deleted /api/billing/confirm route is not resurrected). Re-add welcomePending/welcomePendingAction to en + ja. - Checkout now requires Stripe, but /api/billing/plan still reported billingAvailable:true whenever Stack was configured, so Stripe-less environments advertised an upgrade flow that dead-ends at billing=unavailable. Derive billingAvailable from isStripeBillingConfigured() for both the anonymous and authenticated responses. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Bootstrap oRPC + OpenAPI + type-safe Swift client pipeline Adds a framework-agnostic oRPC server (portable router/procedures with no Next imports), mounted two ways in Next: the RPC handler at /api/rpc for the web client and an OpenAPI (REST) handler at /api/v1 that serves the generated spec. The first procedure is account.me (GET, authenticated) returning the signed-in user's id, email, and resolved billing plan via resolveProPlanStatus. Web consumes it through @orpc/tanstack-query (AccountPlanBadge on the billing page). Swift consumes the checked-in OpenAPI spec via swift-openapi-generator in a new standalone CmuxAPIClient package (accountMe()). Account deletion is intentionally left to the merged PR #7645 (REST + macOS + iOS + tests); this PR does not duplicate it. Delete-over-oRPC can be added later by delegating to a shared extraction of that route. - oRPC foundation: web/orpc/server/{base,router,openapi}.ts, client.ts, query.ts - account.me procedure + behavioral tests (auth gate, plan mapping, spec) - OpenAPI spec checked in (web/openapi + CmuxAPIClient copy, identical) - 20-locale strings for the plan badge; removed unused dangerZone keys * Fix CI: self-contained next-intl mock in billing test + drop banned namespace enum The billing-page test rendered AccountPlanBadge (a client component using next-intl useTranslations) without mocking the client next-intl module, so it only passed when another file's mock.module("next-intl") leaked in. CI runs tests in sorted order where dashboard-billing runs before vault-sessions (the leak source), so useTranslations threw and web-typecheck failed. Mock next-intl in the test itself, mirroring its existing next-intl/server mock. CmuxAPIClientBootstrap was an all-static public enum, which lint-ios-package- conventions bans as a namespace type. It was unused; move its apiServerPath constant onto the CmuxAPIClient struct (which has instance members). * account badge: hide on fetch error; guard OpenAPI spec drift AccountPlanBadge treated any non-pending state as resolved, so a 401/500/network failure (isPending false, data undefined) fell through to the "Free" label, presenting a backend failure as an authoritative downgrade. Return null on error instead; the page's server-rendered plan sections still show the real state. Add a test asserting both checked-in openapi.json copies (web/openapi and the Swift package) are byte-identical to the freshly generated document, so a router change that skips spec regeneration fails in CI instead of at Swift decode time. * CmuxAPIClient: one type per file + DocC on public API Match the repo's Swift package discipline (see sibling CmuxClientConfig): split the three public types into CmuxAccountPlan.swift, CmuxAPIError.swift, and CmuxAPIClient.swift, and document every public symbol including initializer parameters and thrown errors. * billing test: export full next-intl client surface from mock The mock only exported useTranslations, so in CI's sorted test order it shadowed next-intl for files loaded before vault-sessions and broke their static `import { useLocale }`. Export NextIntlClientProvider, useLocale, and useTranslations (every client symbol the app imports) so this global, persistent bun mock never removes a binding a later file's module evaluation needs. * orpc: type the context user by inference at the resolution seam Removes the two TypeScript smells in the account.me path: - me.ts no longer does `await import("...billing/pro")` inside the handler (a dynamic import with no lazy-load justification) — it's a normal top-level import. The one remaining dynamic import, in base.ts, is the deliberate Stack-coupling seam (app/lib/stack validates env + builds the Stack app at module load), documented as such. - Drops the `user as unknown as Parameters<typeof resolveProPlanStatus>[0]` cast. AuthedUser is now inferred once at the root from the exact getUser() call resolveStackUser makes, so it's the same Stack server-user type the billing helpers already accept and procedures pass it on uncast. userId maps from user.id (always a string) with no dead `?? ""`. Test uses a realistic user with an id; with no DATABASE_URL the Stripe lookup catches missing-config and returns false, so the Stack product list decides the plan. typecheck clean, full web suite green, swift build compiles. * orpc: keep account.me unit test database-free The prior commit gave the test fake a user id, which made resolveProPlanStatus run the real Stripe-subscription DB query. That query is non-deterministic under the suite's process-wide db/client mocks (a leaked partial mock lacks .limit), so it passed locally but failed on CI. Revert to the id-less fake: with no id the Stripe lookup short-circuits to false and never touches a database, so the Stack product list alone drives the plan. userId maps via user.id ?? "" to stay total for that fake (a real Stack user always has an id). Full TS cleanups from the previous commit (static import, root-inferred AuthedUser, no as-unknown-as) are unchanged. * account badge: scope label to personal plan The account.me-backed badge only knows the user's personal Pro status (resolveProPlanStatus), but the billing page renders an active Team plan section for team-only subscribers. Labeling the badge "Your plan: Free" contradicted that section on the same page. Rename the heading to "Your personal plan" across all 20 catalogs so the free/pro value reads as the individual plan, not the account-wide entitlement. No schema change.
This branch was successfully deployed
1 participant
Summary
Verification
Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Touches irreversible account/data deletion, auth, billing/Stripe, and VM teardown with partial-failure retry semantics—errors could strand users or leave inconsistent state.
Overview
Adds end-to-end permanent account deletion: native clients call
DELETE /api/account(Bearer +X-Stack-Refresh-Token), and iOS Settings gets a Delete Account flow wired throughAuthCoordinator.deleteAccount()with confirmation, progress, mapped failure alerts, and sign-out on success or after certain acknowledgements.Backend introduces a large staged teardown in
web/app/api/account/route.ts: advisory locks andaccount_deletion_tombstones(15‑minute lease) for idempotent retries; billing/Stripe/TestFlight/subrouter/vault/VM destroy and cmux row cleanup before Stack user delete when possible; distinct responses for pending, retryable partial failure (account_delete_retryable), and post-Stack incomplete cleanup (cleanupIncomplete). Guards block auth (deleting users), device/token registration, VM create, and checkout/webhook billing updates while deletion is in progress.iOS refactors account UI into
MobileSettingsAccountSection, addsMobileSettingsLegalSupportSection(privacy, terms, support), plus en/ja strings.CmuxAuthRuntimeaddsAccountDeletionClientand rich error/result mapping with unit tests.Reviewed by Cursor Bugbot for commit 260fb26. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds in‑app iOS account deletion backed by native‑auth
DELETE /api/account, plus a Legal & Support section in Settings. The backend performs a staged, idempotent teardown guarded by tombstones/advisory locks with a 15‑minute lease, blocks mutations during deletion, and returns clear 204/202/retry outcomes with safe retries.New Features
DELETE /api/account: staged teardown with tombstones/advisory locks and a 15‑min lease; returns 204, retryable{ error: "account_delete_retryable" }, or 202{ cleanupIncomplete: true }; treats post‑Stack cleanup as terminal and resumes safely across retries.AuthCoordinator.deleteAccount()andAccountDeletionClientwith confirmation, progress, and mapped alerts (including timeout/unknown completion); signs out on success or after acknowledging incomplete cleanup.Bug Fixes
account_deletion_in_progress; revoke SSH identities in bounded batches; skip providerless VMs.Written for commit 260fb26. Summary will update on new commits.
Summary by CodeRabbit