fix stripe_web: store card element div globally to prevent mounting errors#2346
fix stripe_web: store card element div globally to prevent mounting errors#2346hifiaz wants to merge 2 commits intoflutter-stripe:mainfrom
Conversation
The card element div was being recreated on each mount, causing issues with Stripe's element mounting. Store it globally and clear on dispose to ensure consistent mounting and unmounting.
📝 WalkthroughWalkthroughCached the web card container element, wired platform view creation to initialize a per-instance MethodChannel, added handlers for focus/card-change events that update state and emit Flutter callbacks, and updated mounting/dispose logic. Also introduced Android-side short-circuit handling for those events and several React-bridge conversions and gradle updates. Changes
Sequence Diagram(s)sequenceDiagram
participant FlutterWidget as Flutter CardField
participant PlatformView as HtmlElementView (stripe_card)
participant DOM as Card Container (div)
participant StripeJS as Stripe Card Element
participant MethodChannel as MethodChannel (per-view)
participant FlutterHandlers as Flutter event handlers
FlutterWidget->>PlatformView: create view (onPlatformViewCreated)
PlatformView->>DOM: create/store div element
PlatformView->>MethodChannel: init per-view channel
StripeJS->>DOM: mount card into container
StripeJS->>MethodChannel: emit topCardChange / topFocusChange events
MethodChannel->>FlutterHandlers: invoke handlers (_handleCardChanged/_handlePlatformFocusChanged)
FlutterHandlers->>FlutterWidget: update state / emit onCardChanged/onFocus
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🤖 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_web/lib/src/widgets/card_field.dart`:
- Line 14: The top-level variable _cardElementDiv is shared across
WebStripeCardState instances and races between the HtmlElementView factory,
initStripe's post-frame callback, and dispose; change _cardElementDiv from a
file-scope variable into an instance field on WebStripeCardState so each state
owns its own HTMLDivElement, update the HtmlElementView factory to use a unique
per-instance viewType key that the factory closure captures (so it writes the
correct instance field), and adjust initStripe (the post-frame callback) and
dispose to reference the instance field instead of the top-level one to avoid
cross-instance clobbering or nulling.
- Around line 90-93: When the early-return checks _cardElementDiv for null (in
the CardField widget where _cardElementDiv is referenced), add a diagnostic log
using the existing dart:developer import instead of silently returning; update
the conditional in the method that contains the snippet (search for
_cardElementDiv and the surrounding code in CardField/State) to call log() (or
log with a level/message) describing that the container is null and why this
might occur before returning, so callers/debuggers see a clear diagnostic when
the element is missing.
|
|
||
| import '../../flutter_stripe_web.dart'; | ||
|
|
||
| web.HTMLDivElement? _cardElementDiv; |
There was a problem hiding this comment.
Global _cardElementDiv is unsafe during concurrent instances or interleaved navigation transitions
_cardElementDiv is a top-level file-scope variable shared by every WebStripeCardState instance. Two code paths race on it:
| Event | Effect on _cardElementDiv |
|---|---|
Platform view factory (per HtmlElementView render) |
writes the newly-created div |
Inner addPostFrameCallback in initStripe |
reads it to mount the Stripe element |
dispose() |
nulls it |
If a second WebCardField is alive at the same time (e.g., the old and new screen overlap on the navigation stack before dispose fires), the factory for the second instance overwrites the global, and the first instance's deferred callback mounts to the wrong container. Conversely, if dispose on the departing widget runs after the new widget's factory has already written the reference, _cardElementDiv = null at Line 197 wipes the freshly valid reference, causing the new instance's inner callback to silently return without mounting (the null-guard at Line 91 swallows the failure).
The pre-existing #card-element CSS selector carried the same single-instance constraint (duplicate IDs), but the global-variable approach is more fragile because the null-out and the factory write are not co-located and can interleave across lifecycle calls.
Recommended fix – promote _cardElementDiv to an instance field and use a per-instance view type key so the factory closure can capture the correct state:
♻️ Proposed refactor
// Remove the top-level variable:
- web.HTMLDivElement? _cardElementDiv;In WebStripeCardState:
class WebStripeCardState extends State<WebCardField> with CardFieldContext {
CardEditController get controller => widget.controller;
+ web.HTMLDivElement? _cardElementDiv; // instance-scoped
+ // Unique per-instance viewType avoids factory-collision between instances.
+ String get _viewType => 'stripe_card_${hashCode}';
`@override`
void initState() {
ui.platformViewRegistry.registerViewFactory(
- 'stripe_card',
- (int viewId) {
- final element = web.HTMLDivElement()..style.border = 'none';
- _cardElementDiv = element;
- return element;
- },
+ _viewType,
+ (int viewId) {
+ final element = web.HTMLDivElement()..style.border = 'none';
+ _cardElementDiv = element;
+ return element;
+ },
);- child: const HtmlElementView(viewType: 'stripe_card'),
+ child: HtmlElementView(viewType: _viewType), `@override`
void dispose() {
detachController(controller);
element?.unmount();
- _cardElementDiv = null;
+ _cardElementDiv = null; // clears instance field only
super.dispose();
}Also applies to: 61-65, 90-93, 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stripe_web/lib/src/widgets/card_field.dart` at line 14, The
top-level variable _cardElementDiv is shared across WebStripeCardState instances
and races between the HtmlElementView factory, initStripe's post-frame callback,
and dispose; change _cardElementDiv from a file-scope variable into an instance
field on WebStripeCardState so each state owns its own HTMLDivElement, update
the HtmlElementView factory to use a unique per-instance viewType key that the
factory closure captures (so it writes the correct instance field), and adjust
initStripe (the post-frame callback) and dispose to reference the instance field
instead of the top-level one to avoid cross-instance clobbering or nulling.
| final container = _cardElementDiv; | ||
| if (container == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silent failure on null container — add a diagnostic log
When _cardElementDiv is null (e.g., due to the race described above, or a frame-ordering edge case), the early return leaves the card field blank with no indication of failure. The file already imports dart:developer for exactly this purpose.
final container = _cardElementDiv;
if (container == null) {
+ dev.log(
+ 'WebCardField: card container element is null; Stripe card element not mounted.',
+ name: 'stripe_web',
+ level: 900, // Level.WARNING
+ );
return;
}📝 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.
| final container = _cardElementDiv; | |
| if (container == null) { | |
| return; | |
| } | |
| final container = _cardElementDiv; | |
| if (container == null) { | |
| dev.log( | |
| 'WebCardField: card container element is null; Stripe card element not mounted.', | |
| name: 'stripe_web', | |
| level: 900, // Level.WARNING | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stripe_web/lib/src/widgets/card_field.dart` around lines 90 - 93,
When the early-return checks _cardElementDiv for null (in the CardField widget
where _cardElementDiv is referenced), add a diagnostic log using the existing
dart:developer import instead of silently returning; update the conditional in
the method that contains the snippet (search for _cardElementDiv and the
surrounding code in CardField/State) to call log() (or log with a level/message)
describing that the container is null and why this might occur before returning,
so callers/debuggers see a clear diagnostic when the element is missing.
Update Android Gradle plugin and Gradle wrapper to latest versions. Add handling for 'topFocusChange' and 'topCardChange' events in platform views to prevent crashes when event data is null. Fix ReadableMap and ReadableArray wrapper classes to properly handle empty JSON objects/arrays. Implement deep conversion for event data in RCTEventEmitter to ensure proper serialization. Add platform view method channel handling for web card field to process focus and card change events.
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 (1)
packages/stripe_web/lib/src/widgets/card_field.dart (1)
219-225:⚠️ Potential issue | 🔴 CriticalBug: Attaching
oldWidget.controllerinstead ofwidget.controllerafter detach.Line 224 attaches
oldWidget.controllerinstead of the newwidget.controller, which means after a controller change, the old controller remains attached rather than the new one.🐛 Proposed fix
if (widget.controller != oldWidget.controller) { assert(!controller.hasCardField, 'CardEditController is already attached to a CardView'); detachController(oldWidget.controller); - attachController(oldWidget.controller); + attachController(widget.controller); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe_web/lib/src/widgets/card_field.dart` around lines 219 - 225, In didUpdateWidget (WebCardField) the code detaches oldWidget.controller but then incorrectly re-attaches oldWidget.controller; change the logic to detach oldWidget.controller and attach the new widget.controller instead—update the calls to detachController(oldWidget.controller) followed by attachController(widget.controller) and ensure the assert still checks controller.hasCardField on the new controller reference (CardEditController/controller) to prevent double attachment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/android/settings.gradle`:
- Around line 19-23: The plugins block declares an AGP version
("com.android.application" version "8.9.1") that is incompatible with the Kotlin
plugin ("org.jetbrains.kotlin.android" version "2.1.0"); update the Kotlin or
AGP version to a compatible pair by either (A) bumping the Kotlin plugin used in
the plugins block to a version explicitly compatible with AGP 8.9.1 (replace
"org.jetbrains.kotlin.android" "2.1.0" with a supported Kotlin release), or (B)
downgrading the AGP declaration ("com.android.application" "8.9.1") to a
supported AGP (e.g., 8.7.2) that matches Kotlin 2.1.0; ensure the change is
applied in the same plugins block so both id "com.android.application" and id
"org.jetbrains.kotlin.android" versions stay compatible.
In
`@packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableArray.java`:
- Around line 51-68: The WritableArray class is losing data because mutator
methods like pushString(...) call the inherited add() which only updates the
ArrayList and never the internal JSONArray used by size(), get(), and
toArrayList(); fix by overriding add(Object) (and any other mutators) in
WritableArray to append to the internal JSONArray as well as the ArrayList (or
alternatively stop extending ArrayList and implement a backing structure that
keeps JSONArray and list in sync), ensuring methods referenced
here—WritableArray.add, pushString, pushBoolean, pushInt (all push*),
ReadableArray.size, ReadableArray.get, and toArrayList—operate on the same
synchronized backing data so toArrayList() reflects pushed values.
In
`@packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableMap.java`:
- Around line 44-58: The isEmpty() and size() implementations incorrectly
fallback to HashMap state; instead make them reflect the JSONObject field only
by returning map.length() == 0 for isEmpty() and map.length() for size() (i.e.,
remove calls to super.isEmpty() and super.size()); update the ReadableMap
methods isEmpty() and size() so they use the JSONObject map as the sole source
of truth.
---
Outside diff comments:
In `@packages/stripe_web/lib/src/widgets/card_field.dart`:
- Around line 219-225: In didUpdateWidget (WebCardField) the code detaches
oldWidget.controller but then incorrectly re-attaches oldWidget.controller;
change the logic to detach oldWidget.controller and attach the new
widget.controller instead—update the calls to
detachController(oldWidget.controller) followed by
attachController(widget.controller) and ensure the assert still checks
controller.hasCardField on the new controller reference
(CardEditController/controller) to prevent double attachment.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
example/android/gradle/wrapper/gradle-wrapper.propertiesexample/android/settings.gradlepackages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableArray.javapackages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableMap.javapackages/stripe_android/android/src/main/kotlin/com/facebook/react/uimanager/events/RCTEventEmitter.ktpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripeSdkCardFormPlatformView.ktpackages/stripe_android/android/src/main/kotlin/com/flutter/stripe/StripeSdkCardPlatformView.ktpackages/stripe_web/lib/src/widgets/card_field.dart
| plugins { | ||
| id "dev.flutter.flutter-plugin-loader" version "1.0.0" | ||
| id "com.android.application" version "8.7.3" apply false | ||
| id "com.android.application" version "8.9.1" apply false | ||
| id "org.jetbrains.kotlin.android" version "2.1.0" apply false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check AGP versions available on Google Maven
curl -s "https://dl.google.com/dl/android/maven2/com/android/tools/build/gradle/maven-metadata.xml" | grep -o '<version>[^<]*</version>' | sed 's/<[^>]*>//g' | tail -20Repository: flutter-stripe/flutter_stripe
Length of output: 320
🌐 Web query:
AGP 8.9.1 Kotlin 2.1.0 compatibility requirements
💡 Result:
For Android Gradle Plugin (AGP) 8.9.1 to build with Kotlin 2.1.0 (KGP 2.1.0), the practical compatibility constraints come from three places:
-
AGP 8.9.x environment requirements
- Gradle: 8.11.1 (minimum/default for AGP 8.9) [1]
- JDK: 17 [1]
-
Minimum AGP/R8 needed for Kotlin 2.1 classfiles
- Minimum AGP: 8.6 (implies you must be on AGP ≥ 8.6; AGP 8.9.1 satisfies this) [2]
-
Kotlin 2.1.0 (KGP) minimums and “fully supported/tested” range
- Minimum supported AGP (by Kotlin): 7.3.1 [3]
- Minimum supported Gradle (by Kotlin): 7.6.3 [3]
- But: Kotlin’s own compatibility table lists KGP 2.1.0–2.1.10 as fully supported only up to AGP 8.7.2, so AGP 8.9.1 + KGP 2.1.0 is outside Kotlin’s “fully supported” range (may still work, but not guaranteed/tested by that table). [4]
Bottom line: AGP 8.9.1 meets the Android toolchain minimum for Kotlin 2.1.0 (AGP ≥ 8.6) [2], but Kotlin 2.1.0 specifically is not listed as “fully supported” with AGP as new as 8.9.x, so if you want a configuration that’s within Kotlin’s tested matrix, you’d typically upgrade KGP (or use an AGP version ≤ 8.7.2). [4]
Sources: [1] [2] [3] [4]
🏁 Script executed:
#!/bin/bash
# Search for all AGP 8.x versions specifically
curl -s "https://dl.google.com/dl/android/maven2/com/android/tools/build/gradle/maven-metadata.xml" | grep -o '<version>8\.[^<]*</version>' | sed 's/<[^>]*>//g' | sort -VRepository: flutter-stripe/flutter_stripe
Length of output: 3060
🌐 Web query:
AGP Android Gradle Plugin 8.9.1 release notes
💡 Result:
AGP 8.9.1 release notes (as listed under “Fixed issues” on the AGP 8.9.x page) include the following fixes: [1]
- Dexer (D8):
- Issue #401489623 — Backporting of
android.os.Build.VERSION_CODES_FULLincorrect for Baklava.
- Issue #401489623 — Backporting of
- Shrinker (R8):
- Issue #401546693 — Unable to generate a signed APK after updating to AGP 8.9.0.
- Issue #397737234 — Error analyzing “written-before-read” property at allocation sites when generating classfile.
- Issue #389737060 — Java SPI shrinking issue affecting R8 versions 8.6.*, 8.7.*, 8.8.0.
Source: Android Developers — Android Gradle Plugin 8.9.0 (March 2025) (contains the 8.9.1 fixed-issues list). [1]
[1] (developer.android.com)
Citations:
- 1: https://developer.android.com/build/releases/past-releases/agp-8-9-0-release-notes?utm_source=openai
Kotlin 2.1.0 is incompatible with AGP 8.9.1 — upgrade Kotlin or downgrade AGP.
AGP 8.9.1 is a stable release. However, Kotlin plugin 2.1.0 is only fully supported up to AGP 8.7.2. This configuration falls outside Kotlin's tested compatibility matrix and may cause build failures. Either upgrade Kotlin to a version compatible with AGP 8.9.1 (e.g., Kotlin 2.0.x or later 2.1.x versions if available), or downgrade AGP to 8.7.2 or earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/android/settings.gradle` around lines 19 - 23, The plugins block
declares an AGP version ("com.android.application" version "8.9.1") that is
incompatible with the Kotlin plugin ("org.jetbrains.kotlin.android" version
"2.1.0"); update the Kotlin or AGP version to a compatible pair by either (A)
bumping the Kotlin plugin used in the plugins block to a version explicitly
compatible with AGP 8.9.1 (replace "org.jetbrains.kotlin.android" "2.1.0" with a
supported Kotlin release), or (B) downgrading the AGP declaration
("com.android.application" "8.9.1") to a supported AGP (e.g., 8.7.2) that
matches Kotlin 2.1.0; ensure the change is applied in the same plugins block so
both id "com.android.application" and id "org.jetbrains.kotlin.android" versions
stay compatible.
| @Override | ||
| public int size() { | ||
| return array.length(); | ||
| if (array.length() > 0) { | ||
| return array.length(); | ||
| } | ||
| return super.size(); | ||
| } | ||
|
|
||
| @Override | ||
| public Object get(int index) { | ||
| if (index < 0 || index >= array.length()) { | ||
| throw new IndexOutOfBoundsException("Index " + index + " out of bounds for length " + array.length()); | ||
| if (array.length() > 0) { | ||
| if (index < 0 || index >= array.length()) { | ||
| throw new IndexOutOfBoundsException("Index " + index + " out of bounds for length " + array.length()); | ||
| } | ||
| return array.opt(index); | ||
| } | ||
| return array.opt(index); | ||
| return super.get(index); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ReadableArray.java" -o -name "ReadableArray.kt" 2>/dev/null | head -20Repository: flutter-stripe/flutter_stripe
Length of output: 167
🏁 Script executed:
git ls-files | grep -i readablearrayRepository: flutter-stripe/flutter_stripe
Length of output: 165
🏁 Script executed:
# Search for ReadableArray in the codebase
rg "class ReadableArray" --type java -A 20Repository: flutter-stripe/flutter_stripe
Length of output: 2473
🏁 Script executed:
cat -n packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableArray.javaRepository: flutter-stripe/flutter_stripe
Length of output: 3283
🏁 Script executed:
# Check for usage patterns - any calls to add/set/addAll on ReadableArray
rg "ReadableArray" --type java -B2 -A5 | rg -P '\.(add|addAll|set|remove)\(' | head -20Repository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Search for any place where ReadableArray methods are called
rg "\.size\(\)|\.get\(" packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ --type java -B2 -A2Repository: flutter-stripe/flutter_stripe
Length of output: 4938
🏁 Script executed:
# Check if ReadableArray is instantiated and used anywhere
rg "new ReadableArray" --type java -B2 -A5Repository: flutter-stripe/flutter_stripe
Length of output: 1066
🏁 Script executed:
# Check if any of the ArrayList inherited methods (add, remove, etc) are called on ReadableArray instances
rg "ReadableArray" --type java -B5 -A10 | rg -E "\.(add|remove|addAll|removeAll|set|clear)\(" | head -20Repository: flutter-stripe/flutter_stripe
Length of output: 469
🏁 Script executed:
# Also check how ReadableArray is used in the codebase
rg "ReadableArray" packages/stripe_android/ --type java -B2 -A5 | grep -v "class ReadableArray" | head -50Repository: flutter-stripe/flutter_stripe
Length of output: 6429
🏁 Script executed:
# Check if there are any subclasses or other manipulations of ReadableArray
rg "extends ReadableArray|instanceof ReadableArray" --type javaRepository: flutter-stripe/flutter_stripe
Length of output: 216
🏁 Script executed:
# Verify ReadableArray constructor calls and how JSONArray is created/modified
rg "new JSONArray" packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ --type java -B2 -A3Repository: flutter-stripe/flutter_stripe
Length of output: 1420
🏁 Script executed:
# Examine WritableArray to understand the subclass better
cat -n packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/WritableArray.javaRepository: flutter-stripe/flutter_stripe
Length of output: 1313
🏁 Script executed:
# Check if WritableArray's push methods add to JSONArray or ArrayList
rg "push" packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/WritableArray.java -A3Repository: flutter-stripe/flutter_stripe
Length of output: 595
🏁 Script executed:
# Check if add() method is overridden in WritableArray
rg "public.*add\(" packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/WritableArray.javaRepository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Check how WritableArray instances are used in the codebase - particularly push operations
rg "WritableArray\(\)" packages/stripe_android/ --type java -B2 -A8 | head -40Repository: flutter-stripe/flutter_stripe
Length of output: 1842
🏁 Script executed:
# Check if toArrayList is ever called on WritableArray
rg "toArrayList\(\)" packages/stripe_android/ --type java -B3 -A2Repository: flutter-stripe/flutter_stripe
Length of output: 803
Critical bug in WritableArray: push methods don't update JSONArray, causing data loss in serialization.
WritableArray extends ReadableArray and uses its inherited add() method (lines 21-47 of WritableArray.java) in all push*() methods. However, add() is not overridden, so it updates the inherited ArrayList storage only, never the JSONArray. This creates a critical data integrity issue:
- New
WritableArray()→new JSONArray()(empty, length 0) pushString("x")→add("x")adds to ArrayList, JSONArray remains emptysize()returns 0 (falls back tosuper.size()sincearray.length() == 0)get(0)returns from parent ArrayList (works by accident via fallback)toArrayList()iterates only over JSONArray, so pushed items are lost
The fallback logic in size() and get() masks this broken design but fails in toArrayList(). The root cause is that WritableArray's add() method must be overridden to update the JSONArray field.
Instead of just removing the fallback as suggested, WritableArray needs to properly implement add() and related mutator methods to keep JSONArray and ArrayList in sync, or WritableArray should not extend ArrayList at all.
🤖 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 - 68, The WritableArray class is losing data because mutator
methods like pushString(...) call the inherited add() which only updates the
ArrayList and never the internal JSONArray used by size(), get(), and
toArrayList(); fix by overriding add(Object) (and any other mutators) in
WritableArray to append to the internal JSONArray as well as the ArrayList (or
alternatively stop extending ArrayList and implement a backing structure that
keeps JSONArray and list in sync), ensuring methods referenced
here—WritableArray.add, pushString, pushBoolean, pushInt (all push*),
ReadableArray.size, ReadableArray.get, and toArrayList—operate on the same
synchronized backing data so toArrayList() reflects pushed values.
| @Override | ||
| public boolean isEmpty() { | ||
| return map.length() == 0; | ||
| if (map.length() > 0) { | ||
| return false; | ||
| } | ||
| return super.isEmpty(); | ||
| } | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return map.length(); | ||
| if (map.length() > 0) { | ||
| return map.length(); | ||
| } | ||
| return super.size(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the ReadableMap.java file
find . -name "ReadableMap.java" -type fRepository: flutter-stripe/flutter_stripe
Length of output: 165
🏁 Script executed:
# Read the ReadableMap.java file to understand the context
cat -n ./packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/ReadableMap.javaRepository: flutter-stripe/flutter_stripe
Length of output: 6309
🏁 Script executed:
# Find the Dynamic class
find . -name "Dynamic.java" -type fRepository: flutter-stripe/flutter_stripe
Length of output: 161
🏁 Script executed:
# Read the Dynamic.java file
cat -n ./packages/stripe_android/android/src/main/kotlin/com/facebook/react/bridge/Dynamic.javaRepository: flutter-stripe/flutter_stripe
Length of output: 517
🏁 Script executed:
# Search for usages of ReadableMap to understand how it's used
rg "new ReadableMap" -A 3 -B 3Repository: flutter-stripe/flutter_stripe
Length of output: 3391
🏁 Script executed:
# Search for tests of ReadableMap
find . -path "*/test*" -name "*ReadableMap*" -type fRepository: flutter-stripe/flutter_stripe
Length of output: 55
🏁 Script executed:
# Search for tests that might test isEmpty or size methods
rg "isEmpty\(\)|\.size\(\)" packages/stripe_android --type java -B 2 -A 2Repository: flutter-stripe/flutter_stripe
Length of output: 5218
Fallback to HashMap state creates inconsistency; isEmpty() and size() should directly reflect the JSONObject's state.
ReadableMap stores data exclusively in its JSONObject map field—all public methods (getString, getInt, getArray, keySetIterator, etc.) operate only on this field. However, when map.length() == 0, the isEmpty() and size() methods delegate to super.isEmpty() and super.size(), which check the inherited HashMap state instead. This creates semantic inconsistency: a map could report as non-empty or have a non-zero size based on entries in the HashMap that are invisible to all other methods.
Since the JSONObject is the sole source of truth for data operations, these methods should return true and 0 respectively when the JSONObject is empty:
♻️ Proposed fix
`@Override`
public boolean isEmpty() {
if (map.length() > 0) {
return false;
}
- return super.isEmpty();
+ return true;
}
`@Override`
public int size() {
if (map.length() > 0) {
return map.length();
}
- return super.size();
+ return 0;
}📝 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.
| @Override | |
| public boolean isEmpty() { | |
| return map.length() == 0; | |
| if (map.length() > 0) { | |
| return false; | |
| } | |
| return super.isEmpty(); | |
| } | |
| @Override | |
| public int size() { | |
| return map.length(); | |
| if (map.length() > 0) { | |
| return map.length(); | |
| } | |
| return super.size(); | |
| } | |
| `@Override` | |
| public boolean isEmpty() { | |
| if (map.length() > 0) { | |
| return false; | |
| } | |
| return true; | |
| } | |
| `@Override` | |
| public int size() { | |
| if (map.length() > 0) { | |
| return map.length(); | |
| } | |
| return 0; | |
| } |
🤖 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 - 58, The isEmpty() and size() implementations incorrectly
fallback to HashMap state; instead make them reflect the JSONObject field only
by returning map.length() == 0 for isEmpty() and map.length() for size() (i.e.,
remove calls to super.isEmpty() and super.size()); update the ReadableMap
methods isEmpty() and size() so they use the JSONObject map as the sole source
of truth.
|
@hifiaz can ou make sure to address the comments and fix the conflices so we can integrate it? |
|
closed due to inactivity feel free to reopen it with updated comments |
2 participants
The card element div was being recreated on each mount, causing issues with Stripe's element mounting. Store it globally and clear on dispose to ensure consistent mounting and unmounting.
Summary by CodeRabbit
New Features
Bug Fixes
Platform