Repository navigation
Route web and macOS cancellations through the mobile cancel flow (SCA-2171) - #25273
Git-on-my-level wants to merge 14 commits into
Conversation
…-2171) Web and macOS now ask why, then show the existing 50%-off retention offer, then confirm, reusing the mobile DELETE /v1/payments/subscription payload, the retention-offer endpoints and the five existing cancel events with a platform property. The added steps sit behind cancel_flow_parity (CANCEL_FLOW_PARITY_ENABLED), off until David's render/copy approval; with it off both platforms keep today's plain confirm / Manage-only paths.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b0f5f55c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trackCancelConfirmed(reason, Boolean(trimmed)); | ||
| try { | ||
| const result = await cancelSubscription({ reason, reasonDetails: trimmed }); | ||
| if (result?.status === 'success' || result?.cancel_at_period_end) { |
There was a problem hiding this comment.
With the parity flag enabled, a successful cancellation never reaches the done screen: both success branches of backend/routers/payment.py::cancel_subscription_endpoint return { "status": "ok", "message": "Subscription scheduled for cancellation." }, without cancel_at_period_end. This condition therefore treats the successful response as an error, displays its message as an error, and skips onSubscriptionUpdate(), leaving the surrounding billing UI stale even though Stripe has scheduled cancellation. Accept the endpoint's ok status and cover that actual response shape in the flow test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@/Volumes/scratch/tmp/opencode/reply-codex.md
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9856449707
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .sheet(isPresented: $showCancelSubscriptionFlow) { | ||
| CancelSubscriptionFlowSheet( | ||
| periodEnd: userSubscription?.subscription.currentPeriodEnd, | ||
| onClose: { showCancelSubscriptionFlow = false }, | ||
| onSubscriptionChanged: { loadSubscriptionInfo() }) |
There was a problem hiding this comment.
On macOS, a successful cancellation calls onSubscriptionChanged(), which refreshes userSubscription through loadSubscriptionInfo(). The cancellation endpoint persists cancel_at_period_end = true, so that refresh makes the enclosing cancelAtPeriodEnd != true condition false and removes the button hosting this .sheet. This dismisses the flow's success screen as soon as billing finishes refreshing, rather than leaving the confirmation visible until the user selects Done. Attach the sheet to a stable ancestor outside the conditional cancellation entry.
Useful? React with 👍 / 👎.
Resolves the feature-flag registry conflict by keeping both sides: main's CONVERSION_BUNDLE_ATTRIBUTION_ENABLED and PUSHER_RECORD_SEAL_PENDING_RETRY_BOUND_ENABLED rows plus this branch's cancel_flow_parity row. No source file this PR touches was changed on main, so the approved render/copy packet is unaffected.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
…-2171) The macOS cancel sheet was attached to the Cancel Subscription button inside `cancelSubscriptionEntry`. A successful cancellation refreshes the subscription, which sets `cancelAtPeriodEnd` and removes that row — so the presenter disappeared and SwiftUI dismissed the sheet before the user could read the done screen. Present the sheet from `planUsageSection` instead, through a small `presentsCancelSubscriptionFlow` modifier, so the done screen stays until the user explicitly dismisses it (Done, Keep, or close) and then shows the refreshed billing state. Entry visibility moves to `showsCancelEntry` with the same flag/cancelAtPeriodEnd rules, so the default-off path is unchanged. Tests: CancelSheetLifecycleTests pins the entry-visibility transition, the presentation placement, and the done-screen hold. Renders are byte-identical to the committed evidence packet. Failure-Class: FC-presenter-removed-with-its-conditional-row
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
Resolves the feature-flag registry conflict by keeping both sides: main's new CONVERSATION_NOTES_SCALED_DETAIL_ENABLED, EPISODE_COMPOSER_*, RECORD_FRESH_MERGE_ENABLED, RECORD_NOTES_EAGER_ENABLED, RECORD_FOLLOWUP_WAKE_ENABLED and RECORD_SEAL_WAKE_PRESERVED_ENABLED rows plus this branch's cancel_flow_parity row. No source file this PR touches was changed on main, so the approved render/copy packet and the SCA-2364 stable-presenter lifecycle fix are unaffected.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
Brings in the upstream guard repair (#25372) that unblocks this PR's backend unit run, plus the episode default-context and pusher codec work. Resolves the feature-flag registry conflict by keeping both sides: main's new EPISODE_COMPOSER_DEFAULT_CONTEXT_ENABLED row plus this branch's cancel_flow_parity row. No source file this PR touches was changed on main, so the approved render/copy packet and the SCA-2364 stable-presenter lifecycle fix are unaffected.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
Resolves the feature-flag registry conflict by keeping both sides: main's new HOME_FEED_ENABLED row plus this branch's cancel_flow_parity row. No source file this PR touches was changed on main, so the approved render/copy packet and the SCA-2364 stable-presenter lifecycle fix are unaffected.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b12ee62474
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cancelSubscription: ((String, String?) async throws -> Void)? = nil | ||
| ) { | ||
| self.periodEnd = periodEnd | ||
| self.loadOffer = loadOffer ?? { try? await APIClient.shared.getRetentionOffer() } |
There was a problem hiding this comment.
When the macOS retention-offer request fails, try? silently converts the error to nil, and loadOfferIfNeeded() presents the same no-offer state used for genuinely ineligible subscriptions. Consequently, an endpoint or connectivity failure can suppress retention offers without any fallback telemetry distinguishing it from normal eligibility decisions. Keep cancellation reachable, but catch the lookup failure and call DesktopDiagnosticsManager.recordFallback with bounded fields and a degraded outcome before returning nil, as required by the macOS guide.
AGENTS.md reference: desktop/macos/AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
What
Web and macOS cancellations now run the mobile cancel flow: reason → optional feedback → consequences with the existing 50%-off-3-months stay offer → confirm. The reason and details are posted to the existing
DELETE /v1/payments/subscription; the offer uses the existingGET/POST /v1/payments/retention-offer. No new endpoint, event, price, plan, ceiling or payer state.PlansSheetandSettingsPagecancel buttons open the sharedCancelSubscriptionFlow; the old plainConfirmDialogremains as the flag-off fallback.Cancel Subscriptionrow under the current-plan card opensCancelSubscriptionFlowSheet;Managestill opens the Stripe portal unchanged.platform: web/platform: macos. Mobile is untouched (it keepsapp_platform).cancel_flow_parity(CANCEL_FLOW_PARITY_ENABLED), read from the Omi flag store on both clients. Missing/unlisted/failed reads keep today's plain confirm and Manage-only paths. The flag never gates the cancel entry point. Registered as aconfig_switchinconfig/feature-flags.yamlwithfail: closedand no production flip here.Checklist
Exact copy (shipped mobile English, string for string)
Step 1: “Why are you canceling?” / “Can you tell us why you're leaving?” / Too expensive · Not using it enough · Missing features · Audio/transcription quality · Battery drain concerns · Found an alternative · Other / Continue.
Step 2: per-reason titles and subtitles from
app_localizations_en.dart(e.g. too_expensive → “What price would work for you?” / “Your feedback helps us find the right balance.”), 300-character field, Continue / Skip for Now.Step 3: “Just a moment, please” / “We highly recommend exploring your other options instead of canceling.” / “Stay and pay {price} a month for the next {months} months.” / “Your plan will remain active until {date}. After that, you'll be moved to the free version with limited features.” / “If you cancel:” + the six mobile consequence lines / “Stay for {percent}% off” or “Keep Subscription” / “Cancel Subscription” / success: “Your subscription is set to cancel at the end of the period.” or “Done. {percent}% off your next {months} months.”
Tests
cd web/app && bun run check— typecheck + 1080 vitest tests pass, including the new flow, kill-switch fallback, error-retry and offer paths.cd desktop/macos/Desktop && swift test --filter 'CancelSheetLifecycleTests|CancelSubscriptionFlowTests'— 12 tests pass (1 render test skipped unlessOMI_CANCEL_FLOW_RENDER_DIRis set).CancelSheetLifecycleTestspins the sheet-presentation lifetime: the cancel row disappears when a successful cancellation setscancelAtPeriodEnd, and the done screen survives that refresh until the user dismisses it..agent-artifacts/ui-evidence/sca-2171-cancel-flow-parity/(web via hermetic Playwright, macOS via the shipped SwiftUI sheet).UI evidence
Failure class (fixes)
Failure-Class: new
Failure-class transition narrative
This PR adds one class,
FC-presenter-removed-with-its-conditional-row: a sheet presented by a row the flow itself hides is torn down with that row, so the flow's own result never reaches the user. The macOS cancel sheet hung off the Cancel Subscription row; a successful cancellation refreshes the subscription, setscancelAtPeriodEnd, and removes that row, which dismissed the done screen mid-flow. The fix moves the presenter to the plan-usage section (a stable ancestor) and drives it from that section's state, and the guard artifact isdesktop/macos/Desktop/Tests/CancelSheetLifecycleTests.swift, which pins the row's disappearance, the section-level presentation, and the done screen holding until explicit dismissal. No other failure-class definition is changed.Product invariants affected
APIClient.swiftchange adds an optional JSON body to the existing authenticated DELETE helper; auth policy, owner binding, refresh and session invalidation are untouched.Notes
main(SCA-1808 is blocked), and placing it is SCA-1808's decision. Parity here is the retention offer the mobile flow actually shows.PlansSheet.tsx/SettingsPage.tsx/subscriptionFunnel.tsin checkout code; this PR touches only the cancel paths and imports. PR Preselect yearly on the mobile settings plan picker, macOS and web (SCA-1753) #25222's plan-picker files are untouched.Line-Count-Exception: desktop/macos/Desktop/Sources/AnalyticsManager.swift | 1704 -> 1758 | the five existing mobile cancel events belong beside the other subscription-funnel emitters; splitting the analytics manager is a separate refactor.