fix!: return sealed CollectBankAccountResult from collectBankAccount / verifyMicrodeposits#2395
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe ACH-related methods Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stripe_platform_interface/lib/src/method_channel_stripe.dart (1)
587-603: Add regression coverage for thesetupIntentbranch.Both public methods now rely on the same private key-selection logic, so this bug can come back quietly. Please add tests that exercise
collectBankAccount(..., isPaymentIntent: false)andverifyPaymentIntentWithMicrodeposits(..., isPaymentIntent: false)with a native result shaped like{ "setupIntent": ... }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe_platform_interface/lib/src/method_channel_stripe.dart` around lines 587 - 603, Add regression tests that exercise the setupIntent branch by invoking collectBankAccount (or the public method that calls it) and verifyPaymentIntentWithMicrodeposits with isPaymentIntent: false and mocking the platform/method channel to return a map whose top-level key is "setupIntent" (e.g., {"setupIntent": {...}}). Specifically, in your unit tests for MethodChannelStripe (or the test file that covers _methodChannel interactions), mock _methodChannel.invokeMapMethod to return a Map<String,dynamic> containing "setupIntent" and assert that _parseIntentResult (via the public APIs collectBankAccount and verifyPaymentIntentWithMicrodeposits) correctly handles the setupIntent payload instead of expecting paymentIntent. Ensure both public entry points are covered so the private key-selection logic that looks for "paymentIntent" vs "setupIntent" is exercised when isPaymentIntent is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stripe_platform_interface/lib/src/method_channel_stripe.dart`:
- Around line 622-669: The current branch fabricates a PaymentIntent from
setupIntentJson by filling missing fields with defaults and forcing
CaptureMethod.Automatic; instead parse the native payload using
SetupIntent.fromJson (or validate required fields) and convert it to a
PaymentIntent only if parsing succeeds, otherwise fail fast; replace the manual
construction that uses setupIntentJson and CaptureMethod.Automatic with code
that calls SetupIntent.fromJson(setupIntentJson) (or validates
id/clientSecret/etc.) and map the captureMethod to CaptureMethod.Unknown for a
synthetic wrapper when appropriate, ensuring any missing required values cause
an error/early return rather than silently using ''/0/false.
---
Nitpick comments:
In `@packages/stripe_platform_interface/lib/src/method_channel_stripe.dart`:
- Around line 587-603: Add regression tests that exercise the setupIntent branch
by invoking collectBankAccount (or the public method that calls it) and
verifyPaymentIntentWithMicrodeposits with isPaymentIntent: false and mocking the
platform/method channel to return a map whose top-level key is "setupIntent"
(e.g., {"setupIntent": {...}}). Specifically, in your unit tests for
MethodChannelStripe (or the test file that covers _methodChannel interactions),
mock _methodChannel.invokeMapMethod to return a Map<String,dynamic> containing
"setupIntent" and assert that _parseIntentResult (via the public APIs
collectBankAccount and verifyPaymentIntentWithMicrodeposits) correctly handles
the setupIntent payload instead of expecting paymentIntent. Ensure both public
entry points are covered so the private key-selection logic that looks for
"paymentIntent" vs "setupIntent" is exercised when isPaymentIntent is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9a0b525-2e61-4427-8e19-8f430fee8dad
📒 Files selected for processing (1)
packages/stripe_platform_interface/lib/src/method_channel_stripe.dart
remonh87
left a comment
There was a problem hiding this comment.
I do not think this is correct if the key is different we should handle this but not create our own parsing ourselves
|
|
||
| final setupIntentJson = result['setupIntent'] as Map<String, dynamic>?; | ||
| if (setupIntentJson != null) { | ||
| return PaymentIntent( |
There was a problem hiding this comment.
I do not like this this we should always rely on automatic parsing of the map through freezed + jsonserializable like we do above.
| ).parse(result: result, successResultKey: 'paymentIntent'); | ||
| } | ||
|
|
||
| static PaymentIntentsStatus _parseStatus(String value) { |
There was a problem hiding this comment.
this should not be needed when we parse it using freezed + json serializable
|
Thanks, that makes sense. I added the manual parsing because the native response is setup-intent-shaped and doesn’t fully match PaymentIntent.fromJson, but I agree this isn’t the right layer for it. I’ll refactor this to rely on the existing freezed + json_serializable parsing and align with the current pattern. For the setup intent case, I may introduce a sealed result type to keep both flows consistent. Let me know if you’d prefer a different approach. I can also update the feat/web PR (#2396) in the same way if that makes sense. |
d0a96cf to
da389bf
Compare
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/stripe/lib/src/stripe.dart (1)
635-663:⚠️ Potential issue | 🟠 MajorThis is a breaking API change that requires version bumping and CHANGELOG documentation.
The return type change from
Future<PaymentIntent>toFuture<CollectBankAccountResult>for bothcollectBankAccount()andverifyPaymentIntentWithMicrodeposits()breaks existing code that expectsPaymentIntent. However, the version remains at 12.6.0 and no breaking change entry appears in the CHANGELOG.Semantic versioning requires a major version bump to 13.0.0 and explicit documentation of this breaking change. Add an entry to both
packages/stripe/CHANGELOG.mdandpackages/stripe_platform_interface/CHANGELOG.mdnoting the sealed union return type, then bump versions accordingly before release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe/lib/src/stripe.dart` around lines 635 - 663, This change introduces a breaking API: the return type for collectBankAccount and verifyPaymentIntentWithMicrodeposits changed from Future<PaymentIntent> to Future<CollectBankAccountResult>, so update package versions and docs to reflect that breaking change: add clear entries to both CHANGELOG.md files describing the sealed-union return type and migration note for collectBankAccount and verifyPaymentIntentWithMicrodeposits, and bump the package versions (pubspecs) from 12.6.0 to 13.0.0 for both the stripe and stripe_platform_interface packages before releasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/stripe/lib/src/stripe.dart`:
- Around line 635-663: This change introduces a breaking API: the return type
for collectBankAccount and verifyPaymentIntentWithMicrodeposits changed from
Future<PaymentIntent> to Future<CollectBankAccountResult>, so update package
versions and docs to reflect that breaking change: add clear entries to both
CHANGELOG.md files describing the sealed-union return type and migration note
for collectBankAccount and verifyPaymentIntentWithMicrodeposits, and bump the
package versions (pubspecs) from 12.6.0 to 13.0.0 for both the stripe and
stripe_platform_interface packages before releasing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5099a4b9-ecaf-47e5-a7c4-cec96b437736
📒 Files selected for processing (8)
example/lib/screens/regional_payment_methods/us_bank_account_direct_debit_screen.dartpackages/stripe/lib/src/stripe.dartpackages/stripe_platform_interface/lib/src/method_channel_stripe.dartpackages/stripe_platform_interface/lib/src/models/collect_bank_account_result.dartpackages/stripe_platform_interface/lib/src/models/collect_bank_account_result.freezed.dartpackages/stripe_platform_interface/lib/src/stripe_platform_interface.dartpackages/stripe_platform_interface/lib/stripe_platform_interface.dartpackages/stripe_web/lib/src/web_stripe.dart
✅ Files skipped from review due to trivial changes (2)
- packages/stripe_platform_interface/lib/stripe_platform_interface.dart
- packages/stripe_platform_interface/lib/src/models/collect_bank_account_result.freezed.dart
4def7c8 to
a8d4c55
Compare
|
Pushed the refactor. |
remonh87
left a comment
There was a problem hiding this comment.
Almost there! If you can revert the changelog and versioning we can ship this. You can think about the jsonserialzable instead of the resultparser but this is not required.
| @@ -1,3 +1,9 @@ | |||
| ## 13.0.0 | |||
There was a problem hiding this comment.
Can you revert this change? We will update the changelogs when we actually release.
| @@ -1,3 +1,10 @@ | |||
| ## 8.0.0 | |||
There was a problem hiding this comment.
Can you revert this change? We will update the changelogs when we actually release.
| name: flutter_stripe | ||
| description: Flutter library for Stripe. Supports PaymentSheets, Apple & Google Pay, SCA, PSD2 and much more. | ||
| version: 12.6.0 | ||
| version: 13.0.0 |
| name: flutter_stripe_web | ||
| description: Stripe sdk bindings for the Flutter web platform. This package contains the implementation of the platform interface for web. | ||
| version: 7.6.0 | ||
| version: 8.0.0 |
Native SDKs return either a payment intent or a setup intent depending on the client secret passed to collectBankAccount / verifyPaymentIntentWithMicrodeposits, but the platform interface only typed the result as PaymentIntent. The setup intent branch therefore failed to parse against PaymentIntent.fromJson, which requires fields (amount, currency, captureMethod, confirmationMethod) that are not present on a setup intent payload. Introduce a sealed CollectBankAccountResult union so both variants can be parsed by their own freezed + json_serializable generated factories without any hand-written field extraction or status mapping.
a8d4c55 to
073d14f
Compare
|
Pushed.
I didn't put @JsonSerializable directly on the union because our native payload uses the key name itself as the discriminator ({"paymentIntent": ...} vs {"setupIntent": ...}) rather than a separate type field, so freezed's auto generated union dispatch doesn't fit cleanly. The manual fromJson is ~10 lines and still delegates all the field work to json_serializable. Happy to restructure if you'd prefer the annotation. |
Summary
collectBankAccountandverifyPaymentIntentWithMicrodepositswhen called withisPaymentIntent: false. The native SDKs return a setup-intent-shaped payload that cannot be parsed byPaymentIntent.fromJson(missingamount,currency,captureMethod,confirmationMethod).CollectBankAccountResultunion with.paymentIntent(PaymentIntent)/.setupIntent(SetupIntent)variants.MethodChannelStripemethods now delegate toResultParser<T>+ the generatedfromJsonfor both branches, removing all hand-written field extraction and status mapping.Breaking change
Stripe.instance.collectBankAccountandStripe.instance.verifyPaymentIntentWithMicrodepositsnow returnFuture<CollectBankAccountResult>instead ofFuture<PaymentIntent>.Callers must now destructure the union:
Note: The setup-intent branch was broken before this change, so no working code depends on the previous shape.
Affected packages
stripe_platform_interfacestripestripe_webSummary by CodeRabbit
collectBankAccount()andverifyPaymentIntentWithMicrodeposits()has changed from a generic intent to a sealed union result type that explicitly indicates whether a payment or setup intent was obtained. Code calling these methods requires updates to properly handle the distinct result types and their associated data.