Fix: #2382 confirm token handler on payment sheet#2400
Conversation
📝 WalkthroughWalkthroughDart now precomputes and mutates the serialized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 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 |
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/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swift (1)
202-209:⚠️ Potential issue | 🔴 CriticalMatch the confirmation-token completion payload key used by Dart.
MethodChannelStripe.confirmationTokenCreationCallbacksendsparams, but iOS only readsresult, so after_confirmTokenHandleris invoked the completion callback is rejected asinvalidParamson iOS.🐛 Proposed fix
case "confirmationTokenCreationCallback": - guard let arguments = call.arguments as? FlutterMap, - let res = arguments["result"] as? NSDictionary else { + guard let arguments = call.arguments as? FlutterMap else { + result(FlutterError.invalidParams) + return + } + guard let res = (arguments["result"] as? NSDictionary) + ?? (arguments["params"] as? NSDictionary) else { result(FlutterError.invalidParams) return }🤖 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/StripePlugin.swift` around lines 202 - 209, The iOS handler for the "confirmationTokenCreationCallback" case is reading the wrong payload key ("result") causing invalidParams; update the guard in StripePlugin.swift (inside the switch case for "confirmationTokenCreationCallback") to read let res = arguments["params"] as? NSDictionary (keeping the existing call.arguments as? FlutterMap guard) and pass that res into confirmationTokenCreationCallback(result: res) so the iOS key matches the Dart MethodChannelStripe.confirmationTokenCreationCallback payload.
🤖 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/StripePlugin.swift`:
- Around line 456-458: The code incorrectly synthesizes
mutable["confirmHandler"] = true when mutable["confirmationTokenConfirmHandler"]
is nil, which can mark a confirm handler as present even when Dart didn't
provide one; update the logic in StripePlugin.swift where mutable is constructed
(look for keys "confirmationTokenConfirmHandler" and "confirmHandler" and the
downstream onConfirmHandlerCallback usage) so that you only set "confirmHandler"
when an explicit Dart flag for it exists (or when
confirmationTokenConfirmHandler is truthy), otherwise leave it unset/null to
allow native “missing handler” validation to catch absent callbacks.
---
Outside diff comments:
In `@packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swift`:
- Around line 202-209: The iOS handler for the
"confirmationTokenCreationCallback" case is reading the wrong payload key
("result") causing invalidParams; update the guard in StripePlugin.swift (inside
the switch case for "confirmationTokenCreationCallback") to read let res =
arguments["params"] as? NSDictionary (keeping the existing call.arguments as?
FlutterMap guard) and pass that res into
confirmationTokenCreationCallback(result: res) so the iOS key matches the Dart
MethodChannelStripe.confirmationTokenCreationCallback payload.
🪄 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: 89f71c4d-1382-4854-a74d-2866df89e85d
📒 Files selected for processing (2)
packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swiftpackages/stripe_platform_interface/lib/src/method_channel_stripe.dart
c744634 to
a50fd50
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stripe_platform_interface/lib/src/method_channel_stripe.dart (1)
246-258: Move handler-presence flag serialization intoIntentConfiguration.toJson().The native side checks for key presence (not boolean values)—absent keys are treated as
false. SinceIntentConfigurationalready knows whether each handler is non-null, derivingconfirmHandlerandconfirmationTokenConfirmHandlerflags inside its owntoJson()method would keep serialization logic self-contained and eliminate the post-mutation pattern andis Map<String, dynamic>guard here.This requires a custom
toJson()override since the handlers are marked@JsonKey(includeToJson: false).🤖 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 246 - 258, The post-serialization mutation that sets 'confirmHandler'/'confirmationTokenConfirmHandler' on paramsJson (using params.toJson(), intentConfig, and the is Map guard) should be removed and instead implemented inside IntentConfiguration.toJson(): update IntentConfiguration.toJson() to return a Map that includes 'confirmHandler' when its confirmHandler field is non-null and 'confirmationTokenConfirmHandler' when confirmTokenHandler is non-null (these handlers are currently annotated `@JsonKey`(includeToJson: false)), then remove the extra mutation logic around paramsJson/intentConfig in method_channel_stripe.dart and keep the invokeMethod call as-is so the native side sees the presence (key existence) of those flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/stripe_platform_interface/lib/src/method_channel_stripe.dart`:
- Around line 246-258: The post-serialization mutation that sets
'confirmHandler'/'confirmationTokenConfirmHandler' on paramsJson (using
params.toJson(), intentConfig, and the is Map guard) should be removed and
instead implemented inside IntentConfiguration.toJson(): update
IntentConfiguration.toJson() to return a Map that includes 'confirmHandler' when
its confirmHandler field is non-null and 'confirmationTokenConfirmHandler' when
confirmTokenHandler is non-null (these handlers are currently annotated
`@JsonKey`(includeToJson: false)), then remove the extra mutation logic around
paramsJson/intentConfig in method_channel_stripe.dart and keep the invokeMethod
call as-is so the native side sees the presence (key existence) of those flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c80e9856-88ba-4e8b-9435-8a2c09309d9c
📒 Files selected for processing (2)
packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swiftpackages/stripe_platform_interface/lib/src/method_channel_stripe.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/StripePlugin.swift
| result(FlutterError.invalidParams) | ||
| return | ||
| } | ||
| if (params.object(forKey: "intentConfiguration") != nil && params.object(forKey: "intentConfiguration") is NSDictionary) { |
There was a problem hiding this comment.
this is not needed anymore: Now that Dart explicitly sends exactly the right flag (confirmHandler or confirmationTokenConfirmHandler), the iOS layer should just pass the params through unchanged.
I'm finding that this feedback is valid and needs to be applied, as InvalidParams is otherwise always thrown. |
Fixes #2382
Summary by CodeRabbit