Conversation
Before That constructor only set this.array but never populated the ArrayList superclass
|
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:
📝 WalkthroughWalkthroughDeleted iOS SwiftPM lock entries; bumped native Stripe versions; added card-funding filtering models and plumbing; added connectedAccountId support for Financial Connections; migrated Android image/icon flows to coroutine-based async base64 conversion and lifecycle/UI tweaks; added Dart enum/field updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/Stripe Sdk/ConnectAccountOnboarding/ConnectAccountOnboardingView.swift (1)
47-74:⚠️ Potential issue | 🟠 MajorGuard against stale completion revealing after a fast dismiss.
If
visibleflips to false while the presentation animation is in-flight, the completion still runs and can unhide the view or attach React content after dismissal. That yields a visible view whenvisible == false. Add a state guard (or token) before un-hiding.✅ Suggested guard to prevent stale completion
presenter.present(navigationController!, animated: true) { [weak self] in - guard let self = self else { return } - // Add React content after presentation animation finishes - self.viewController?.setReactContentView(self) - self.isHidden = false // Show the view now that modal is presented + guard let self = self, self.visible, self.wasVisible else { return } + // Add React content after presentation animation finishes + self.viewController?.setReactContentView(self) + self.isHidden = false // Show the view now that modal is presented }packages/stripe_android/android/src/main/kotlin/com/reactnativestripesdk/NavigationBarView.kt (1)
61-78:⚠️ Potential issue | 🟠 MajorAdd a contentDescription to the close button for accessibility.
Line 61 creates an icon-only button without an accessible label, which blocks screen-reader users. Add a localized content description.
🔧 Suggested fix
ImageButton(context).apply { + contentDescription = context.getString(R.string.stripe_close_button) setImageDrawable( context.resources.getDrawable( android.R.drawable.ic_menu_close_clear_cancel, null, ), )
🤖 Fix all issues with AI agents
In
`@packages/stripe_android/android/src/main/kotlin/com/reactnativestripesdk/PaymentSheetManager.kt`:
- Around line 585-628: Detekt flags the empty bodies in the Drawable.Callback
implementation inside waitForDrawableToLoad; either annotate the anonymous
object with a suppression (e.g., add `@Suppress`("Detekt.EmptyFunctionBlock") to
the callback object) or add explicit no-op bodies/comments to scheduleDrawable
and unscheduleDrawable (e.g., include a comment like // no-op or a trivial
return) so detekt no longer reports EmptyFunctionBlock while leaving
invalidateDrawable behavior unchanged.
In
`@packages/stripe_android/android/src/main/kotlin/com/reactnativestripesdk/StripeSdkModule.kt`:
- Around line 1077-1088: The current assignment for accountId uses
getValOr(params, "connectedAccountId", null) which allows an empty string to
override stripeAccountId; change it to treat blank strings as absent by
normalizing the retrieved value to null when it's null or blank (e.g., call a
helper or inline check like treating the result of getValOr(...) as null if
isNullOrBlank()), then fall back to stripeAccountId; apply the same
normalization for the other occurrence around lines 1105-1116 so
FinancialConnectionsSheetManager receives either a real account id or the
fallback stripeAccountId, not an empty string.
In `@packages/stripe_ios/ios/stripe_ios/Sources/stripe_ios/Stripe`
Sdk/StripeSdkImpl.swift:
- Around line 1140-1158: The code temporarily mutates the global
STPAPIClient.shared.stripeAccount in collectBankAccountToken and
collectFinancialConnectionsAccounts, causing race conditions; fix by serializing
the entire override->call->restore sequence on a dedicated serial DispatchQueue
(e.g., create a private static let stripeAccountQueue = DispatchQueue(label:
"com.app.stripeAccountQueue")) and perform the block that saves
originalStripeAccount, sets STPAPIClient.shared.stripeAccount, calls
FinancialConnections.presentForToken (or the corresponding FinancialConnections
call), and relies on the wrappedResolve to restore the originalStripeAccount all
on that queue so no two flows can interleave; apply the same queue-serialization
pattern to both collectBankAccountToken and collectFinancialConnectionsAccounts
(referencing STPAPIClient.shared.stripeAccount, wrappedResolve, and
FinancialConnections.presentForToken) or alternatively replace use of the global
by instantiating and using a per-instance STPAPIClient if supported.
🧹 Nitpick comments (3)
packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReactContextBaseJavaModule.java (1)
44-47: Consider cleaning up formatting and adding documentation.The lifecycle hook pattern is appropriate here, enabling
StripeSdkModuleto properly callsuper.invalidate()during cleanup. However, the formatting has unnecessary blank lines inside the method body.✨ Suggested cleanup
- - public void invalidate() { - - } - + /** + * Called when the module is being invalidated. Subclasses should override + * to perform cleanup (e.g., releasing resources, clearing managers). + */ + public void invalidate() { + }packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableArray.java (1)
47-53: Optional:toArrayList()could now leverage the populated ArrayList.Since both constructors now populate the inherited ArrayList, this method could be simplified to avoid iterating over the JSONArray separately, ensuring consistency and reducing code duplication.
♻️ Suggested simplification
`@NotNull` public ArrayList<Object> toArrayList() { - ArrayList<Object> arrayList = new ArrayList<>(); - for (int i = 0; i < array.length(); i++) { - arrayList.add(array.opt(i)); - } - return arrayList; + return new ArrayList<>(this); }packages/stripe_ios/ios/stripe_ios/Package.swift (1)
15-15: Align SPM constraint with CocoaPods and repository patterns using a floating version range.The CocoaPods constraint uses
~> 25.6.0(floating patch range), while SPM is pinned to exact25.6.0. Changing to.upToNextMinor(from: "25.6.0")maintains consistency across dependency managers and aligns with the repository's pattern of using floating ranges for Stripe dependencies.♻️ Suggested change
- .package(url: "https://github.com/stripe/stripe-ios-spm", exact: "25.6.0") + .package(url: "https://github.com/stripe/stripe-ios-spm", .upToNextMinor(from: "25.6.0"))
| // Use connectedAccountId from params if provided, otherwise fall back to global stripeAccountId | ||
| val accountId = getValOr(params, "connectedAccountId", null) ?: stripeAccountId | ||
|
|
||
| unregisterStripeUIManager(financialConnectionsSheetManager) | ||
| financialConnectionsSheetManager = | ||
| FinancialConnectionsSheetManager( | ||
| reactApplicationContext, | ||
| clientSecret, | ||
| FinancialConnectionsSheetManager.Mode.ForToken, | ||
| publishableKey, | ||
| stripeAccountId, | ||
| accountId, | ||
| ).also { |
There was a problem hiding this comment.
Treat blank connectedAccountId as absent to avoid invalid overrides.
An empty string will currently bypass the fallback and be passed to the SDK. Consider normalizing blanks to null so it falls back to stripeAccountId.
💡 Suggested tweak
- val accountId = getValOr(params, "connectedAccountId", null) ?: stripeAccountId
+ val accountId =
+ getValOr(params, "connectedAccountId", null)
+ ?.takeIf { it.isNotBlank() }
+ ?: stripeAccountIdAlso applies to: 1105-1116
🤖 Prompt for AI Agents
In
`@packages/stripe_android/android/src/main/kotlin/com/reactnativestripesdk/StripeSdkModule.kt`
around lines 1077 - 1088, The current assignment for accountId uses
getValOr(params, "connectedAccountId", null) which allows an empty string to
override stripeAccountId; change it to treat blank strings as absent by
normalizing the retrieved value to null when it's null or blank (e.g., call a
helper or inline check like treating the result of getValOr(...) as null if
isNullOrBlank()), then fall back to stripeAccountId; apply the same
normalization for the other occurrence around lines 1105-1116 so
FinancialConnectionsSheetManager receives either a real account id or the
fallback stripeAccountId, not an empty string.
…o delegate to JSONArray, matching the size() override
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/facebook/react/bridge/ReadableArray.java`:
- Around line 51-59: The ReadableArray overrides size() and get(int) to proxy to
the underlying JSONArray but still uses JSONArray.opt() which returns null for
OOB (violating List.get() contract) and leaves ArrayList's iterator/toArray
using the empty backing store; update get(int) to throw
IndexOutOfBoundsException when index<0 or index>=array.length(), and implement
iterator() and both toArray() overloads in the ReadableArray class to iterate
over and materialize elements from the JSONArray (use array.get(index) or
equivalent safe access to build an Iterator<Object> and Object[]/T[] results) so
all List behaviors (iteration, toArray, get, size) consistently reflect the
JSONArray contents.
| @Override | ||
| public int size() { | ||
| return super.size(); | ||
| return array.length(); | ||
| } | ||
|
|
||
| @Override | ||
| public Object get(int index) { | ||
| return array.opt(index); | ||
| } |
There was a problem hiding this comment.
Correct bug-fix, but opt() silently swallows out-of-bounds and ArrayList.iterator() still bypasses these overrides.
The previous state was broken: super.size() always returned 0 (the ArrayList backing store is never populated), and super.get(i) always threw IndexOutOfBoundsException for the same reason. Both overrides fix that correctly.
Two residual concerns:
-
opt()vs theListcontract —JSONArray.opt(index)returnsnullfor out-of-bounds instead of throwingIndexOutOfBoundsException, whichList.get()requires. Callers treatingReadableArrayas aList<Object>could silently receivenullfor a bad index.getType()handles this gracefully (→ ReadableType.Null), but any external consumer relying on the exception for bounds checking would be misled. -
ArrayList.iterator()bypasses these overrides (pre-existing design gap, now more visible) —ArrayList's privateItraccesseselementData[]directly; it does not callget()or consultsize(). As a result,for (Object o : readableArray),.iterator(), and.toArray()still iterate over the empty ArrayList backing store even thoughsize()now returns the JSONArray length. Any code path that iterates via standard Java iteration will silently produce zero elements.
🔧 Optional: override iterator() and toArray() to stay consistent
+ `@Override`
+ `@NotNull`
+ public java.util.Iterator<Object> iterator() {
+ return toArrayList().iterator();
+ }
+
+ `@Override`
+ public Object[] toArray() {
+ return toArrayList().toArray();
+ }🤖 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/facebook/react/bridge/ReadableArray.java`
around lines 51 - 59, The ReadableArray overrides size() and get(int) to proxy
to the underlying JSONArray but still uses JSONArray.opt() which returns null
for OOB (violating List.get() contract) and leaves ArrayList's iterator/toArray
using the empty backing store; update get(int) to throw
IndexOutOfBoundsException when index<0 or index>=array.length(), and implement
iterator() and both toArray() overloads in the ReadableArray class to iterate
over and materialize elements from the JSONArray (use array.get(index) or
equivalent safe access to build an Iterator<Object> and Object[]/T[] results) so
all List behaviors (iteration, toArray, get, size) consistently reflect the
JSONArray contents.
ReadableMap extends Dynamic which extends HashMap, but stores all data in a private JSONObject field. The inherited HashMap.isEmpty() and HashMap.size() always returned true/0 respectively, since no data is ever put into the HashMap itself. This caused GooglePay config params to be incorrectly treated as empty.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableMap.java (1)
44-47: Remove the now-deadisEmpty()extension function inExtensions.kt.In Kotlin, member functions always take precedence over extension functions with the same signature. Adding
isEmpty()as a member here silently shadows the existing extension:// Extensions.kt:67 — now unreachable for ReadableMap receivers fun ReadableMap.isEmpty() = !this.keySetIterator().hasNextKey()Any call site that previously resolved to the Kotlin extension will now silently dispatch to this Java member method. The two implementations are semantically equivalent, but the extension function is now dead code and should be deleted.
🧹 Proposed cleanup in
Extensions.kt-fun ReadableMap.isEmpty() = !this.keySetIterator().hasNextKey()🤖 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/facebook/react/bridge/ReadableMap.java` around lines 44 - 47, The Java class ReadableMap now defines a member method isEmpty(), which takes precedence over the Kotlin extension fun ReadableMap.isEmpty() in Extensions.kt and makes that extension dead; remove the extension declaration fun ReadableMap.isEmpty() from Extensions.kt (or replace its usage if intended) so there is no silently unreachable duplicate — keep the Java ReadableMap.isEmpty() as the single implementation and delete the extension function to clean up dead code.
🤖 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_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableMap.java`:
- Around line 44-47: The Java class ReadableMap now defines a member method
isEmpty(), which takes precedence over the Kotlin extension fun
ReadableMap.isEmpty() in Extensions.kt and makes that extension dead; remove the
extension declaration fun ReadableMap.isEmpty() from Extensions.kt (or replace
its usage if intended) so there is no silently unreachable duplicate — keep the
Java ReadableMap.isEmpty() as the single implementation and delete the extension
function to clean up dead code.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes