feat: add PaymentMethodMessaging widget#2387
Conversation
Implements the PaymentMethodMessaging component that displays messaging about available BNPL payment methods (Klarna, Afterpay/Clearpay). Includes Dart model, Flutter widget, and native platform views for both iOS (StripePaymentSheet) and Android (stripe-android).
|
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:
📝 WalkthroughWalkthroughIntroduces a new Flutter widget Changes
Sequence DiagramsequenceDiagram
participant Flutter as Flutter Widget
participant MC as MethodChannel
participant Factory as Platform Factory
participant View as Native View
participant Stripe as Stripe SDK
Flutter->>MC: onPlatformViewCreated
MC->>Factory: create(creationParams)
Factory->>View: init + registerMethodHandler
View->>Stripe: Create messaging view
Stripe->>View: onHeightChange
View->>MC: invoke onHeightChange
MC->>Flutter: height callback
Flutter->>Flutter: setState(_height)
Note over Flutter: Configuration Update
Flutter->>MC: updateConfiguration
MC->>View: updateConfiguration method
View->>Stripe: applyConfiguration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
packages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformViewFactory.kt (1)
18-18: Use non-null string keys forcreationParams.Line 18 currently allows null keys (
Map<String?, Any?>?), which is a weaker contract than needed for platform view params.♻️ Suggested refinement
- val creationParams = args as? Map<String?, Any?>? + `@Suppress`("UNCHECKED_CAST") + val creationParams = args as? Map<String, Any?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformViewFactory.kt` at line 18, The creationParams variable currently casts args to a nullable-key map (Map<String?, Any?>?) which permits null keys; update the cast in StripePaymentMethodMessagingPlatformViewFactory so creationParams uses non-null string keys (Map<String, Any?>?) to enforce the expected contract for platform view parameters and adjust any subsequent usage to account for the tightened type.
🤖 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_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swift`:
- Around line 57-63: The guard in configureView currently casts "amount"
directly to Int which fails with FlutterStandardMessageCodec (Dart ints arrive
as NSNumber); update the check to cast amount as NSNumber (or any Number type)
first (e.g., let amountNumber = arguments["amount"] as? NSNumber) and then
derive the Int value (e.g., amountNumber.intValue) before using it so
configureView receives a valid Int; keep the existing paymentMethods and
currency casts unchanged.
In `@packages/stripe/lib/src/widgets/payment_method_messaging.dart`:
- Around line 52-83: The current platform check sends every non-iOS platform
into Android-specific code (AndroidViewSurface, AndroidViewController,
PlatformViewsService.initSurfaceAndroidView) which will fail on
web/desktop/fuchsia; update the branching so PlatformViewLink/AndroidViewSurface
path is used only when defaultTargetPlatform == TargetPlatform.android, keep the
UiKitView for TargetPlatform.iOS, and for all other platforms return a safe
fallback (e.g., a placeholder or SizedBox/error widget) instead of Android APIs;
adjust references around _viewType and onPlatformViewCreated accordingly so
Android-only APIs are never instantiated on unsupported platforms.
- Around line 26-44: Add a dispose override that unregisters the MethodChannel
handler to avoid callbacks after widget teardown: in the widget's State class
implement dispose() that calls _methodChannel?.setMethodCallHandler(null) (and
optionally _methodChannel = null) before calling super.dispose(), ensuring the
handler set in onPlatformViewCreated is removed.
---
Nitpick comments:
In
`@packages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformViewFactory.kt`:
- Line 18: The creationParams variable currently casts args to a nullable-key
map (Map<String?, Any?>?) which permits null keys; update the cast in
StripePaymentMethodMessagingPlatformViewFactory so creationParams uses non-null
string keys (Map<String, Any?>?) to enforce the expected contract for platform
view parameters and adjust any subsequent usage to account for the tightened
type.
🪄 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: 93081976-e344-44b6-9d44-f68c4422fe78
📒 Files selected for processing (11)
packages/stripe/lib/flutter_stripe.dartpackages/stripe/lib/src/widgets/payment_method_messaging.dartpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripeAndroidPlugin.ktpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformView.ktpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformViewFactory.ktpackages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swiftpackages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swiftpackages/stripe_platform_interface/lib/src/models/payment_method_messaging.dartpackages/stripe_platform_interface/lib/src/models/payment_method_messaging.freezed.dartpackages/stripe_platform_interface/lib/src/models/payment_method_messaging.g.dartpackages/stripe_platform_interface/lib/stripe_platform_interface.dart
remonh87
left a comment
remonh87
left a comment
There was a problem hiding this comment.
Thanks. Found a few issues also make sure to address the coderabbit issues some are valid
| widget.onHeightChange?.call(height); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
creationParams is computed once from widget.configuration in build() and passed to the native platform view at creation time. There is no didUpdateWidget override, so if the parent rebuilds with a different
configuration (e.g. new amount), the native view is never notified.
There was a problem hiding this comment.
Good catch — fixed in 7b790dc. didUpdateWidget now compares widget.configuration != oldWidget.configuration (Freezed gives us structural ==) and forwards the updated payload via _methodChannel?.invokeMethod('updateConfiguration', widget.configuration.toJson()). iOS recreates the Stripe subview on update (its Configuration is init-only); Android reassigns PaymentMethodMessagingView.configuration in place. Rebuilding the parent with a new amount/currency now re-renders the messaging.
| } | ||
| } | ||
|
|
||
| class PaymentMethodMessagingPlatformView: NSObject, FlutterPlatformView, PaymentMethodMessagingViewDelegate { |
There was a problem hiding this comment.
let's be consistant and add it to it's own view
There was a problem hiding this comment.
Done in 7b790dc. Extracted PaymentMethodMessagingView : UIView into packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/Stripe Sdk/PaymentMethodMessagingView.swift, matching CardFieldView/AuBECSDebitFormView. PaymentMethodMessagingPlatformView is now a thin FlutterPlatformView wrapper that holds the view and forwards height updates + the updateConfiguration method call.
…Messaging - iOS: cast `amount` via NSNumber.intValue (FlutterStandardMessageCodec sends Dart ints as NSNumber, not Swift Int) so the view configures instead of silently failing. - iOS: extract the Stripe SDK view into its own PaymentMethodMessagingView UIView subclass under Stripe Sdk/, matching the CardFieldView / AuBECSDebitFormView pattern; the FlutterPlatformView wrapper is now thin. - Dart: explicit iOS / Android / else branching with throw UnsupportedError, matching card_field.dart so web/desktop no longer runs Android APIs. - Dart: add dispose() to null the per-view MethodChannel handler and didUpdateWidget() to forward configuration changes via updateConfiguration. - Android: method-channel handler applies updateConfiguration by reassigning PaymentMethodMessagingView.configuration in place; dispose() clears the handler.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformView.kt`:
- Around line 66-73: The anonymous OnLayoutChangeListener added in
StripePaymentMethodMessagingPlatformView (via view.addOnLayoutChangeListener {
... }) captures channel/context and is never removed; modify the class to store
the listener(s) in a property (e.g., layoutChangeListener) when adding them
(also apply to the second listener at the other occurrence) and in dispose()
call view.removeOnLayoutChangeListener(layoutChangeListener), remove the native
child view from its parent (e.g., parent.removeView(view) or
viewGroup.removeView(view)), and clear the channel/method call handler as before
to prevent post-dispose callbacks and leaks.
- Around line 35-50: The applyConfiguration function currently returns early on
invalid params (missing currency/amount/countryCode or empty paymentMethods)
without touching the native messaging view, leaving stale UI; update
applyConfiguration (and any updateConfiguration wrapper) to explicitly clear or
hide the native view (messagingView) before each early return — e.g., set
messagingView to hidden/removed or reset its configuration — so when
paymentMethods is empty or required fields are missing the previous message is
not shown; reference applyConfiguration, updateConfiguration, messagingView, and
the paymentMethods mapping/empty check when making the change.
In
`@packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swift`:
- Around line 68-82: In updateProps, change the NSNumber accessor so amount
remains a Swift Int: replace (arguments["amount"] as? NSNumber)?.intValue with
(arguments["amount"] as? NSNumber)?.integerValue so the unwrapped amount matches
applyConfiguration(amount: Int); update the guard binding in the updateProps
function accordingly (keeping the same optional casting pattern for arguments
and other keys).
- Around line 51-66: The MethodChannel handler is currently registered by
passing the instance method handle which captures self strongly; change
setMethodCallHandler(handle) to a closure that captures self weakly (e.g.,
setMethodCallHandler { [weak self] call, result in self?.handle(call, result) })
and add cleanup in deinit to unregister the per-view handler
(channel.setMethodCallHandler(nil)); update PaymentMethodMessagingViewFactory
(and analogous factories like CardFieldFactory/ApplePayButtonView) to use the
weak-capture closure and ensure deinit clears channel to avoid leaking platform
views while keeping messagingView.onHeightChange’s existing weak capture
pattern.
In `@packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/Stripe`
Sdk/PaymentMethodMessagingView.swift:
- Around line 18-29: When removing the native messaging view you must inform
Flutter to collapse the container height; update the removal logic around
messagingView and the empty-methods guard so that before returning (or
immediately after calling messagingView?.removeFromSuperview() and setting
messagingView = nil) you call the existing height/resize channel method to send
a zero (or collapse) height. Specifically, add the collapse-height message in
the code paths where messagingView is removed and where guard !methods.isEmpty
returns false so the Flutter side no longer preserves the previous height.
- Around line 31-36: Before constructing
StripePaymentSheet.PaymentMethodMessagingView.Configuration, check
STPAPIClient.shared.publishableKey and bail out if it's nil: guard that
STPAPIClient.shared.publishableKey != nil, and if nil return/emit a safe
fallback (e.g., return an empty/no-op view or skip creating the configuration)
instead of using STPAPIClient.shared directly; update the code around
PaymentMethodMessagingView.Configuration creation so it only passes
STPAPIClient.shared when publishableKey is present and mirrors the same guard
pattern used in StripeSdkImpl/AddressSheetView/CustomerSheet.
🪄 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: ec5da5ed-5187-4975-9639-c31f1dd3d4cb
📒 Files selected for processing (4)
packages/stripe/lib/src/widgets/payment_method_messaging.dartpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripePaymentMethodMessagingPlatformView.ktpackages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swiftpackages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/Stripe Sdk/PaymentMethodMessagingView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stripe/lib/src/widgets/payment_method_messaging.dart
| channel.setMethodCallHandler(handle) | ||
| messagingView.onHeightChange = { [weak self] arguments in | ||
| self?.channel.invokeMethod("onHeightChange", arguments: arguments) | ||
| } | ||
| updateProps(args) | ||
| } | ||
|
|
||
| public func handle(_ call: FlutterMethodCall, result: @escaping FlutterResult) { | ||
| switch call.method { | ||
| case "updateConfiguration": | ||
| updateProps(call.arguments) | ||
| result(nil) | ||
| default: | ||
| result(FlutterMethodNotImplemented) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect: direct instance-method handler registration and no deinit unregistering the handler.
rg -n -C4 'class PaymentMethodMessagingPlatformView|setMethodCallHandler|deinit' --iglob '*PaymentMethodMessagingViewFactory.swift'Repository: flutter-stripe/flutter_stripe
Length of output: 2432
🏁 Script executed:
cat -n packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swiftRepository: flutter-stripe/flutter_stripe
Length of output: 3173
🏁 Script executed:
# Search for any Flutter documentation or patterns in the codebase
fd -t f 'FlutterMethodChannel' packages/stripe_ios --exec grep -l "setMethodCallHandler\|deinit" {} \;Repository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Search for all FlutterMethodChannel usages in the codebase
rg -t swift 'FlutterMethodChannel|setMethodCallHandler' -B2 -A2Repository: flutter-stripe/flutter_stripe
Length of output: 13692
🏁 Script executed:
# Check if any of these platform view classes have deinit implementations
rg -t swift 'deinit|setMethodCallHandler.*nil' packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/ -lRepository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Check CardFieldFactory and other factory files more explicitly for any handler cleanup
rg -t swift 'deinit' packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/CardFieldFactory.swift packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/ApplePayButtonView.swift packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/AddressSheetFactory.swiftRepository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Check StripePlugin.swift to see if there's a different pattern there
rg -t swift -B5 -A10 'class StripePlugin' packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swiftRepository: flutter-stripe/flutter_stripe
Length of output: 826
🏁 Script executed:
# Search for comments or documentation about handler retention in the codebase
rg -t swift -i 'handler.*retain|retain.*handler|memory.*leak|leak.*handler' packages/stripe_iosRepository: flutter-stripe/flutter_stripe
Length of output: 55
Avoid retaining each platform view through the method channel handler.
setMethodCallHandler(handle) registers an instance-method closure that captures self strongly. Use a weak capture and unregister the per-view channel handler in deinit to prevent leaking platform views across route/view recreation. This pattern is inconsistent with the weak capture used in messagingView.onHeightChange (line 52) and is repeated across multiple factory classes (CardFieldFactory, ApplePayButtonView, etc.).
Proposed fix
channel.setMethodCallHandler(handle)
+ // Use weak self to break the retain cycle between view and channel handler
+ channel.setMethodCallHandler { [weak self] call, result in
+ guard let self = self else {
+ result(FlutterMethodNotImplemented)
+ return
+ }
+ self.handle(call, result: result)
+ }
messagingView.onHeightChange = { [weak self] arguments in
self?.channel.invokeMethod("onHeightChange", arguments: arguments)
}
updateProps(args)
}
+
+ deinit {
+ channel.setMethodCallHandler(nil)
+ }
public func handle(_ call: FlutterMethodCall, result: `@escaping` FlutterResult) {📝 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.
| channel.setMethodCallHandler(handle) | |
| messagingView.onHeightChange = { [weak self] arguments in | |
| self?.channel.invokeMethod("onHeightChange", arguments: arguments) | |
| } | |
| updateProps(args) | |
| } | |
| public func handle(_ call: FlutterMethodCall, result: @escaping FlutterResult) { | |
| switch call.method { | |
| case "updateConfiguration": | |
| updateProps(call.arguments) | |
| result(nil) | |
| default: | |
| result(FlutterMethodNotImplemented) | |
| } | |
| } | |
| public init(messenger: FlutterBinaryMessenger, viewId: Int64, args: [String: Any]?) { | |
| channel = FlutterMethodChannel(name: "com.stripe/payment_method_messaging_\(viewId)", binaryMessenger: messenger) | |
| messagingView = PaymentMethodMessagingView() | |
| // Use weak self to break the retain cycle between view and channel handler | |
| channel.setMethodCallHandler { [weak self] call, result in | |
| guard let self = self else { | |
| result(FlutterMethodNotImplemented) | |
| return | |
| } | |
| self.handle(call, result: result) | |
| } | |
| messagingView.onHeightChange = { [weak self] arguments in | |
| self?.channel.invokeMethod("onHeightChange", arguments: arguments) | |
| } | |
| updateProps(args) | |
| } | |
| deinit { | |
| channel.setMethodCallHandler(nil) | |
| } | |
| public func handle(_ call: FlutterMethodCall, result: `@escaping` FlutterResult) { | |
| switch call.method { | |
| case "updateConfiguration": | |
| updateProps(call.arguments) | |
| result(nil) | |
| default: | |
| result(FlutterMethodNotImplemented) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swift`
around lines 51 - 66, The MethodChannel handler is currently registered by
passing the instance method handle which captures self strongly; change
setMethodCallHandler(handle) to a closure that captures self weakly (e.g.,
setMethodCallHandler { [weak self] call, result in self?.handle(call, result) })
and add cleanup in deinit to unregister the per-view handler
(channel.setMethodCallHandler(nil)); update PaymentMethodMessagingViewFactory
(and analogous factories like CardFieldFactory/ApplePayButtonView) to use the
weak-capture closure and ensure deinit clears channel to avoid leaking platform
views while keeping messagingView.onHeightChange’s existing weak capture
pattern.
There was a problem hiding this comment.
Declining in this PR for repo-wide consistency. The same setMethodCallHandler(handle) strong-capture is used by every per-view factory in the codebase — CardFieldFactory.swift:64, AuBECSDebitFormFactory.swift:60, CardFormFactory.swift, AddressSheetFactory.swift, ApplePayButtonView.swift, AddToWalletButtonFactory.swift. Switching only this one would diverge without actually closing the leak for the other factories. Happy to tackle it as a dedicated follow-up PR that converts all platform-view handlers to [weak self] + deinit together.
There was a problem hiding this comment.
@realmeylisdev, that's a fair and well-reasoned call — a piecemeal fix would create inconsistency without actually eliminating the leak across the board. A dedicated follow-up PR covering all six factories together is the right approach.
Would you like me to open a GitHub issue to track the follow-up work? It would cover converting all per-view MethodChannel handlers in:
CardFieldFactory.swift(line 64)AuBECSDebitFormFactory.swift(line 60)CardFormFactory.swiftAddressSheetFactory.swiftApplePayButtonView.swiftAddToWalletButtonFactory.swiftPaymentMethodMessagingViewFactory.swift(this file)
…to [weak self] closures with a deinit { channel.setMethodCallHandler(nil) } cleanup.
( ˶ᵔ ᵕ ᵔ˶) noted for next time!
✏️ Learnings added
Learnt from: realmeylisdev
Repo: flutter-stripe/flutter_stripe PR: 2387
File: packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swift:51-66
Timestamp: 2026-04-22T21:00:11.134Z
Learning: In flutter_stripe (flutter-stripe/flutter_stripe), all iOS platform-view factory classes (CardFieldFactory.swift:64, AuBECSDebitFormFactory.swift:60, CardFormFactory.swift, AddressSheetFactory.swift, ApplePayButtonView.swift, AddToWalletButtonFactory.swift, and the new PaymentMethodMessagingViewFactory.swift) intentionally use setMethodCallHandler(handle) with a strong self capture — this is a known repo-wide pattern. Any change to use [weak self] closures and deinit { channel.setMethodCallHandler(nil) } should be done across ALL these files in a single dedicated PR to maintain consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Re: the CodeRabbit nitpick on |
- iOS: cast `amount` via NSNumber.integerValue (Int-sized) instead of intValue (Int32) so amounts above ~$21M don't truncate. - iOS: PaymentMethodMessagingView now emits height 0 when the Stripe subview is removed, and skips configuration when the publishable key is unset (matches the guard pattern in StripeSdkImpl / AddressSheetView / CustomerSheet). - iOS: updateProps clears the view on invalid arguments instead of silently returning. - Android: store the layout-change listener so dispose() can detach it, and remove child views on dispose to avoid leaking the containerView / channel references. - Android: applyConfiguration clears the view and emits height 0 on invalid / empty configuration instead of leaving stale messaging.
The Android code on this branch never compiled: it imported `com.stripe.android.paymentsheet.paymentmethodmessaging.PaymentMethodMessagingView`, which does not exist in stripe-android. On Android the SDK ships `PaymentMethodMessagingElement` (Compose-based) in a separate `com.stripe:payment-method-messaging` artifact with an entirely different surface (suspend `configure`, `element.Content(appearance)`, `@OptIn(PaymentMethodMessagingElementPreview::class)`). Porting that properly is its own PR. For now: - Delete StripePaymentMethodMessagingPlatformView.kt and its factory. - Remove the `flutter.stripe/payment_method_messaging` view-factory registration from StripeAndroidPlugin. - Make the Dart widget iOS-only: throws UnsupportedError on Android (and everywhere else) and documents the asymmetry in its docstring. iOS support is unchanged. A follow-up issue tracks the Android implementation.
|
Scoping this PR to iOS-only (commit 3a576da). While investigating the CI failures I realized the Android side of this PR never compiled. It imports
Porting that properly needs a What changed in 3a576da:
Android follow-up tracked in #2402. The separate |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stripe/lib/src/widgets/payment_method_messaging.dart (1)
69-76: Throwing frombuild()on non-iOS platforms will crash the entire widget subtree.Throwing inside
buildis picked up by Flutter's error widget and, in release mode, can produce a jarring red/grey error box rather than a graceful no-op. Since the widget is documented as iOS-only and Android is an intentional follow-up (#2402), consider returning aSizedBox.shrink()(optionally with anassertorFlutterError.reportErrorin debug) so consumers rendering the widget behind platform-agnostic code paths don't crash on Android/web/desktop.🔧 Suggested alternative
- if (defaultTargetPlatform != TargetPlatform.iOS) { - throw UnsupportedError( - 'PaymentMethodMessaging is only supported on iOS. ' - 'Android support is tracked as a follow-up.', - ); - } + assert( + defaultTargetPlatform == TargetPlatform.iOS, + 'PaymentMethodMessaging is only supported on iOS. ' + 'Android support is tracked as a follow-up (`#2402`).', + ); + if (defaultTargetPlatform != TargetPlatform.iOS) { + return const SizedBox.shrink(); + }This matches the "fail loud in debug, degrade gracefully in release" pattern and avoids forcing every caller to wrap the widget in a platform check. That said, if the explicit throw is a deliberate API contract to make Android misuse obvious during the iOS-only interim, feel free to disregard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe/lib/src/widgets/payment_method_messaging.dart` around lines 69 - 76, The build method of PaymentMethodMessaging currently throws on non-iOS platforms which will crash widget trees; change it to degrade gracefully by returning SizedBox.shrink() when defaultTargetPlatform != TargetPlatform.iOS and only surface a loud failure in debug (use an assert or call FlutterError.reportError with context inside that branch) so consumers on Android/web/desktop don’t hit a runtime throw while still warning developers in debug; update the build method and related platform-check logic accordingly.
🤖 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/lib/src/widgets/payment_method_messaging.dart`:
- Around line 55-60: In didUpdateWidget, currently the call to
_methodChannel?.invokeMethod('updateConfiguration',
widget.configuration.toJson()) is unawaited and can drop PlatformExceptions (and
is silently ignored when _methodChannel is null); change it to await or attach
error handling so platform errors are logged/handled (e.g., await
_methodChannel.invokeMethod(...) inside a try/catch or
_methodChannel?.invokeMethod(...).catchError(...)), and guard the null case by
queuing the update or explicitly logging/skipping when _methodChannel is null so
updates aren't silently lost; update references: didUpdateWidget,
_methodChannel, invokeMethod('updateConfiguration'), and
widget.configuration.toJson().
---
Nitpick comments:
In `@packages/stripe/lib/src/widgets/payment_method_messaging.dart`:
- Around line 69-76: The build method of PaymentMethodMessaging currently throws
on non-iOS platforms which will crash widget trees; change it to degrade
gracefully by returning SizedBox.shrink() when defaultTargetPlatform !=
TargetPlatform.iOS and only surface a loud failure in debug (use an assert or
call FlutterError.reportError with context inside that branch) so consumers on
Android/web/desktop don’t hit a runtime throw while still warning developers in
debug; update the build method and related platform-check logic accordingly.
🪄 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: 7e02e8aa-ba26-4e49-8b71-93c929a508ce
📒 Files selected for processing (1)
packages/stripe/lib/src/widgets/payment_method_messaging.dart
| if (widget.configuration != oldWidget.configuration) { | ||
| _methodChannel?.invokeMethod( | ||
| 'updateConfiguration', | ||
| widget.configuration.toJson(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unawaited invokeMethod can silently swallow platform errors.
The returned Future is discarded, so any PlatformException from the native side (e.g. malformed config) is lost with no logging. Consider awaiting and handling errors, or at least attaching a .catchError for observability. Also worth noting: if didUpdateWidget fires before onPlatformViewCreated has run (first-frame races), _methodChannel will be null and the update is silently dropped — low risk here since creationParams already carry the fresh config on creation, but worth a mental note.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stripe/lib/src/widgets/payment_method_messaging.dart` around lines
55 - 60, In didUpdateWidget, currently the call to
_methodChannel?.invokeMethod('updateConfiguration',
widget.configuration.toJson()) is unawaited and can drop PlatformExceptions (and
is silently ignored when _methodChannel is null); change it to await or attach
error handling so platform errors are logged/handled (e.g., await
_methodChannel.invokeMethod(...) inside a try/catch or
_methodChannel?.invokeMethod(...).catchError(...)), and guard the null case by
queuing the update or explicitly logging/skipping when _methodChannel is null so
updates aren't silently lost; update references: didUpdateWidget,
_methodChannel, invokeMethod('updateConfiguration'), and
widget.configuration.toJson().
There was a problem hiding this comment.
Declining for repo-wide consistency, same rationale as the earlier Map<String?, Any?>? thread. Every widget in this layer uses fire-and-forget _methodChannel?.invokeMethod(...) for prop-change propagation — see card_field.dart:507-536 (five unawaited calls: onPostalCodeEnabledChanged, onCountryCodeChangedEvent, dangerouslyGetFullCardDetails, onStyleChanged, onPlaceholderChanged) and aubecs_debit_form.dart:84-98. Adding .catchError only on the new widget would diverge without actually improving observability for the other four. Happy to tackle this as a dedicated follow-up PR that adds error logging across all platform-view widgets together.
There was a problem hiding this comment.
@realmeylisdev, understood — that's a solid rationale. The fire-and-forget _methodChannel?.invokeMethod(...) pattern is clearly the established norm across the Dart widget layer, and patching only the new widget would be an inconsistent half-measure. Acknowledged, no further push on this.
Would you like me to open a follow-up GitHub issue to track adding error logging/handling across all platform-view widgets (card_field.dart, aubecs_debit_form.dart, and the new payment_method_messaging.dart) in a single dedicated PR?
✏️ Learnings added
Learnt from: realmeylisdev
Repo: flutter-stripe/flutter_stripe PR: 2387
File: packages/stripe/lib/src/widgets/payment_method_messaging.dart:55-60
Timestamp: 2026-04-22T21:17:32.053Z
Learning: In flutter_stripe (flutter-stripe/flutter_stripe), all Dart platform-view widgets use fire-and-forget (unawaited) `_methodChannel?.invokeMethod(...)` calls for prop-change propagation in `didUpdateWidget`. This is a deliberate repo-wide pattern seen in `card_field.dart` (lines 507–536, five unawaited calls: `onPostalCodeEnabledChanged`, `onCountryCodeChangedEvent`, `dangerouslyGetFullCardDetails`, `onStyleChanged`, `onPlaceholderChanged`) and `aubecs_debit_form.dart` (lines 84–98). Any change to add `.catchError` or `try/catch` error handling should be done across ALL platform-view widgets in a single dedicated PR to maintain consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: realmeylisdev
Repo: flutter-stripe/flutter_stripe PR: 2387
File: packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/PaymentMethodMessagingViewFactory.swift:51-66
Timestamp: 2026-04-22T21:00:11.134Z
Learning: In flutter_stripe (flutter-stripe/flutter_stripe), all iOS platform-view factory classes (CardFieldFactory.swift:64, AuBECSDebitFormFactory.swift:60, CardFormFactory.swift, AddressSheetFactory.swift, ApplePayButtonView.swift, AddToWalletButtonFactory.swift, and the new PaymentMethodMessagingViewFactory.swift) intentionally use setMethodCallHandler(handle) with a strong self capture — this is a known repo-wide pattern. Any change to use [weak self] closures and deinit { channel.setMethodCallHandler(nil) } should be done across ALL these files in a single dedicated PR to maintain consistency.
Learnt from: loganpar
Repo: flutter-stripe/flutter_stripe PR: 2365
File: packages/stripe_web/lib/src/widgets/card_field.dart:190-196
Timestamp: 2026-03-11T02:37:15.820Z
Learning: In Dart 3.8+ null-aware elements like ?expr are valid inside collection literals (lists, sets, maps). For map literals, 'key': ?value omits the entry if value is null. This can differ from using if (value != null) 'key': value, but is equivalent for simple cases. Apply this guidance to Dart 3.8+ codebases; ensure the project language version is at least 3.8 before relying on this syntax.
|
Closing this PR — superseded by #2403. Root cause of the CI failures on
The real class on both platforms is Rather than rewrite every native file in place and churn the review history, I opened #2403 fresh on top of current Thanks for the thorough review here — |
2 participants
Summary
PaymentMethodMessagingConfigurationmodel (Klarna, Afterpay/Clearpay)PaymentMethodMessagingFlutter widget with auto-resizingPaymentMethodMessagingViewCloses part of #2378
Test plan
Summary by CodeRabbit
New Features