fix(listeners): use singleton native listener to prevent iOS removeAll() bug#3164
Conversation
…l() bug When multiple useIAP instances were active (or a single instance unmounted/remounted during navigation), iOS native removePurchaseUpdatedListener called removeAll() instead of removing only the specific listener. This silently wiped ALL registered listeners, causing onPurchaseSuccess to never fire — users paid but the app never acknowledged the purchase. The fix replaces per-listener native registration with a singleton native handler per event type and JS-level Set-based fan-out. remove() now only deletes from the JS Set, so other listeners remain intact regardless of iOS native behavior. Applied to all 5 listener types: purchaseUpdated, purchaseError, promotedProduct, userChoiceBilling, developerProvidedBilling. Closes #3150 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 (1)
📝 WalkthroughWalkthroughRefactors listener management to use a single native callback per event type that fans out to multiple JS listeners via Sets; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,200,255,0.5)
participant JS_App as JS App
participant ListenerMgr as Listener Manager (src/index.ts)
participant Native as Native Layer
end
JS_App->>ListenerMgr: addPurchaseUpdatedListener(cb1)
JS_App->>ListenerMgr: addPurchaseUpdatedListener(cb2)
ListenerMgr->>Native: registerSingleNativePurchaseListener() %% on first JS listener
Native->>ListenerMgr: nativePurchaseEvent(purchase)
ListenerMgr->>JS_App: invoke cb1(purchase)
ListenerMgr->>JS_App: invoke cb2(purchase)
JS_App->>ListenerMgr: removePurchaseUpdatedListener(cb1) %% only JS Set updated
JS_App->>ListenerMgr: endConnection()
ListenerMgr->>ListenerMgr: resetListenerState()
JS_App->>ListenerMgr: initConnection() -> native listener re-registered on next add
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue in the iOS implementation of react-native-iap where removing a purchase update listener would inadvertently remove all listeners, causing purchase events to be missed. The solution replaces the previous per-listener native registration with a singleton native handler and JS-level Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3164 +/- ##
==========================================
+ Coverage 68.84% 68.90% +0.05%
==========================================
Files 9 9
Lines 1778 1791 +13
Branches 587 584 -3
==========================================
+ Hits 1224 1234 +10
- Misses 549 552 +3
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a critical fix for an iOS bug where removing one IAP listener would inadvertently remove all listeners, by refactoring the event listener logic to use a singleton native listener for each event type with a JavaScript-level Set to manage individual listeners. While this is a significant improvement for reliability, a security issue was identified where sensitive purchase data and PII are logged to the system console in error conditions, which needs to be addressed by sanitizing the logged objects. Additionally, consider improving the robustness of the endConnection logic by ensuring the listener state is only reset after the native connection is successfully closed.
| resetListenerState(); | ||
| return await IAP.instance.endConnection(); |
There was a problem hiding this comment.
To improve robustness, it's better to reset the listener state after the native endConnection call succeeds. If IAP.instance.endConnection() fails (by throwing or returning false), the current implementation would leave the JS state reset while the native state might not be, causing an inconsistency. Moving resetListenerState() to after the await and checking for success ensures that the JS state is only cleared if the native operation is successful.
For example:
export const endConnection: MutationField<'endConnection'> = async () => {
try {
if (!iapRef) return true;
const success = await IAP.instance.endConnection();
if (success) {
resetListenerState();
}
return success;
} catch (error) {
// ...
}
};There was a problem hiding this comment.
Fixed in 60f2092 — resetListenerState() now runs after endConnection() succeeds, so JS state is preserved if the native call fails.
| RnIapConsole.error( | ||
| 'Invalid purchase data received from native:', | ||
| nitroPurchase, | ||
| ); |
There was a problem hiding this comment.
The library logs the entire nitroPurchase object to console.error when validation fails in purchaseUpdateNativeHandler. This object can contain sensitive information such as purchaseToken, appAccountToken (PII), and other transaction details. Since RnIapConsole.error is always enabled, this information is written to the system logs in production environments. An attacker with access to system logs could potentially exfiltrate sensitive purchase tokens or PII.
Recommendation: Sanitize the nitroPurchase object before logging it. Only log non-sensitive fields like productId and transactionId to help with debugging.
There was a problem hiding this comment.
Fixed in 60f2092 — error logs now only include productId/id instead of the full purchase object.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/index.ts (1)
396-412: Use canonical listener payload types instead ofany/ local interfaces.These callback payloads are part of the public TS API, but one is still
anyand the other is declared ad hoc insrc/index.ts. Please move them intosrc/types.tsand import them here withimport typeso the public surface has one canonical source.As per coding guidelines, "When declaring API params/results in JS/TS modules, import canonical types from
src/types.tsrather than creating ad-hoc interfaces."Also applies to: 481-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 396 - 412, Replace the ad-hoc/any listener payload types with canonical exported types from src/types.ts: define (or locate) a UserChoiceBillingPayload (and any other payload used around lines ~481-503) in src/types.ts, export it, then in this file import type { UserChoiceBillingPayload, NitroUserChoiceBillingListener } from 'src/types'; update the type of NitroUserChoiceBillingListener and the listener callbacks (userChoiceBillingJsListeners, userChoiceBillingNativeHandler, userChoiceBillingListenerAndroid) to use those imported types instead of any or locally-declared interfaces so the public API types are centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 260-272: resetListenerState currently clears all JS listener
bookkeeping immediately, which can lose references if the native endConnection()
fails; change the flow so resetListenerState is only invoked after
endConnection() has completed successfully (i.e., wait for endConnection() to
resolve before calling resetListenerState), or make resetListenerState async and
have endConnection() await its success before clearing; specifically ensure the
flags purchaseUpdateNativeAttached, purchaseErrorNativeAttached,
promotedProductNativeAttached, userChoiceBillingNativeAttached,
developerProvidedBillingNativeAttached and the listener maps
purchaseUpdateJsListeners, purchaseErrorJsListeners, promotedProductJsListeners,
userChoiceBillingJsListeners, developerProvidedBillingJsListeners are cleared
only after a confirmed native teardown from endConnection() (apply same change
at the other occurrence around the lines referenced 1175-1179).
- Around line 227-235: The purchaseErrorNativeHandler is discarding the native
productId by hardcoding productId to undefined; update
purchaseErrorNativeHandler so the normalized PurchaseError preserves the native
productId (e.g., productId: error.productId or falling back to undefined) while
still normalizing the code via normalizeErrorCodeFromNative, then emit that
normalized object to purchaseErrorJsListeners; ensure the property name matches
the PurchaseError type.
- Around line 279-292: The listener attachment blocks (e.g., the purchaseUpdated
listener using purchaseUpdateNativeAttached and
IAP.instance.addPurchaseUpdatedListener with purchaseUpdateNativeHandler)
swallow the listener when Nitro isn't ready and never replay it; modify the
module to queue pending native listener attachments when toErrorMessage(e)
indicates "Nitro runtime not installed" (store the listener identity and type),
and then, inside initConnection() (or wherever IAP.instance becomes available),
iterate that pending queue to call the corresponding IAP.instance.add*Listener
methods and flip the associated *Attached flags (purchaseUpdateNativeAttached
and the analogous flags for other blocks at lines ~307, ~350, ~422, ~513).
Ensure you only attach each queued listener once and clear the queue after
replay.
- Around line 211-216: The native handler (purchaseUpdateNativeHandler)
currently calls each function in purchaseUpdateJsListeners directly so a thrown
exception from one listener aborts the loop; change it to iterate over a shallow
snapshot of purchaseUpdateJsListeners (e.g., Array.from(...) or
[...purchaseUpdateJsListeners]) and invoke each listener inside its own
try/catch so failures are caught and logged per-listener (include identifying
context like the listener reference and the convertedPurchase), leaving other
listeners to run; apply the same pattern to the equivalent native handlers /
listener arrays referenced elsewhere in this file (the other purchase/event
handler blocks noted in the review).
---
Nitpick comments:
In `@src/index.ts`:
- Around line 396-412: Replace the ad-hoc/any listener payload types with
canonical exported types from src/types.ts: define (or locate) a
UserChoiceBillingPayload (and any other payload used around lines ~481-503) in
src/types.ts, export it, then in this file import type {
UserChoiceBillingPayload, NitroUserChoiceBillingListener } from 'src/types';
update the type of NitroUserChoiceBillingListener and the listener callbacks
(userChoiceBillingJsListeners, userChoiceBillingNativeHandler,
userChoiceBillingListenerAndroid) to use those imported types instead of any or
locally-declared interfaces so the public API types are centralized and
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7538016-66c0-4e4e-8e25-e24dc4a8cb70
📒 Files selected for processing (2)
src/__tests__/index.test.tssrc/index.ts
| const purchaseErrorNativeHandler: NitroPurchaseErrorListener = (error) => { | ||
| const normalized: PurchaseError = { | ||
| code: normalizeErrorCodeFromNative(error.code), | ||
| message: error.message, | ||
| productId: undefined, | ||
| }; | ||
| for (const listener of purchaseErrorJsListeners) { | ||
| listener(normalized); | ||
| } |
There was a problem hiding this comment.
Preserve productId when normalizing purchase errors.
Line 231 hardcodes productId to undefined, so SKU-specific failures lose the identifier apps use to correlate the error back to the request. The native side still carries productId, so this looks like a regression.
🔧 Minimal fix
const purchaseErrorNativeHandler: NitroPurchaseErrorListener = (error) => {
const normalized: PurchaseError = {
code: normalizeErrorCodeFromNative(error.code),
message: error.message,
- productId: undefined,
+ productId: error.productId,
};📝 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.
| const purchaseErrorNativeHandler: NitroPurchaseErrorListener = (error) => { | |
| const normalized: PurchaseError = { | |
| code: normalizeErrorCodeFromNative(error.code), | |
| message: error.message, | |
| productId: undefined, | |
| }; | |
| for (const listener of purchaseErrorJsListeners) { | |
| listener(normalized); | |
| } | |
| const purchaseErrorNativeHandler: NitroPurchaseErrorListener = (error) => { | |
| const normalized: PurchaseError = { | |
| code: normalizeErrorCodeFromNative(error.code), | |
| message: error.message, | |
| productId: error.productId, | |
| }; | |
| for (const listener of purchaseErrorJsListeners) { | |
| listener(normalized); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 227 - 235, The purchaseErrorNativeHandler is
discarding the native productId by hardcoding productId to undefined; update
purchaseErrorNativeHandler so the normalized PurchaseError preserves the native
productId (e.g., productId: error.productId or falling back to undefined) while
still normalizing the code via normalizeErrorCodeFromNative, then emit that
normalized object to purchaseErrorJsListeners; ensure the property name matches
the PurchaseError type.
There was a problem hiding this comment.
Investigated — NitroPurchaseResult (the native error type) does not carry a productId field, only responseCode/debugMessage/code/message/purchaseToken. So undefined is correct here; there's no native productId to preserve.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the NitroPurchaseErrorListener / NitroPurchaseResult type in the Nitro spec
rg -n "NitroPurchaseResult\|PurchaseErrorListener\|purchaseError\|NitroPurchaseError" --type=ts -C3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the Nitro spec file and check the error-related type definitions
fd "RnIap.nitro" --type f
Length of output: 24
🏁 Script executed:
#!/bin/bash
# Check the error listener and related types in the Nitro spec
grep -n "purchaseError\|PurchaseError\|NitroPurchase\|productId\|responseCode\|debugMessage\|purchaseToken" src/specs/RnIap.nitro.ts | head -60
Length of output: 2410
🏁 Script executed:
#!/bin/bash
# Show the full NitroPurchaseResult interface definition
sed -n '329,345p' src/specs/RnIap.nitro.ts
Length of output: 437
@hyochan, confirmed — NitroPurchaseResult (the type flowing into addPurchaseErrorListener) is defined as:
export interface NitroPurchaseResult {
responseCode: number;
debugMessage?: string;
code: string;
message: string;
purchaseToken?: string;
}No productId field exists on that interface, so productId: undefined is the correct and only option. My original suggestion was wrong — error.productId would be a TypeScript type error. Thanks for digging into this!
🐇
sniffs the spec file
productId? Never heard of her.
✏️ Learnings added
Learnt from: hyochan
Repo: hyochan/react-native-iap PR: 3164
File: src/index.ts:231-243
Timestamp: 2026-03-10T18:42:37.865Z
Learning: In `src/index.ts`, the `purchaseErrorNativeHandler` correctly sets `productId: undefined` in the normalized `PurchaseError` because the native error type `NitroPurchaseResult` (defined in `src/specs/RnIap.nitro.ts`) does not carry a `productId` field — it only has `responseCode`, `debugMessage`, `code`, `message`, and `purchaseToken`. Suggesting `error.productId` would be a TypeScript type error.
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: hyochan
Repo: hyochan/react-native-iap PR: 3157
File: ios/HybridRnIap.swift:870-872
Timestamp: 2026-03-04T06:38:51.213Z
Learning: In `ios/HybridRnIap.swift`, the `removePromotedProductListenerIOS`, `removePurchaseUpdatedListener`, and `removePurchaseErrorListener` methods intentionally call `removeAll()` rather than removing a single listener by identity. This is because Swift closures passed through the Nitro bridge cannot be identity-compared — each bridge call wraps the closure in a new object. Per-listener removal is instead handled by the JS layer's WeakMap-based listener management. An ID/token-based approach would require changes to the Nitro interface and is deferred as a separate enhancement.
Learnt from: CR
Repo: hyochan/react-native-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-18T15:04:06.079Z
Learning: Applies to example/**/*.{ts,tsx} : For `requestPurchase` in the `useIAP` hook, use the `onPurchaseSuccess` callback to receive purchase results, NOT the return value.
Learnt from: CR
Repo: hyochan/react-native-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-18T15:04:06.079Z
Learning: Applies to src/specs/RnIap.nitro.ts : The Nitro interface definition in `src/specs/RnIap.nitro.ts` contains the contract between TypeScript and native implementations. Include ALL native method declarations here.
Learnt from: CR
Repo: hyochan/react-native-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-18T15:04:06.079Z
Learning: Applies to example/**/*.{ts,tsx} : Inside the `useIAP` hook, most methods return `Promise<void>` and update internal state. Do not design examples that expect returned data from methods like `fetchProducts`, `requestPurchase`, `getAvailablePurchases`. Consume state from the hook instead.
Learnt from: CR
Repo: hyochan/react-native-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-18T15:04:06.079Z
Learning: Applies to example/**/*.{ts,tsx} : Always use `useIAP` hook for error handling - errors are automatically normalized to `ErrorCode` enum. Use helper functions `isUserCancelledError()` and `getUserFriendlyErrorMessage()` for common error checks.
Learnt from: hyochan
Repo: hyochan/react-native-iap PR: 2999
File: android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt:644-660
Timestamp: 2025-09-13T01:07:18.841Z
Learning: In Android IAP error handling: purchaseToken and productId are distinct properties - purchaseToken identifies a completed purchase transaction (should be null in error cases), while productId is the product SKU for context
Learnt from: hyochan
Repo: hyochan/react-native-iap PR: 3002
File: docs/docs/getting-started/setup-ios.md:55-60
Timestamp: 2025-09-14T00:13:04.055Z
Learning: The validateReceipt function from useIAP hook in react-native-iap expects a transaction identifier as parameter, which is accessed via purchase.id (not purchase.productId). This is confirmed by the maintainer hyochan and aligns with the library's migration from purchase.transactionId to purchase.id.
Learnt from: hyochan
Repo: hyochan/react-native-iap PR: 3162
File: android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt:696-697
Timestamp: 2026-03-09T18:42:11.197Z
Learning: In `android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt`, `finishTransaction` returns `Variant_Boolean_NitroPurchaseResult.First(true)` when `params.android` is null. This is intentional cross-platform behavior: iOS calls `finishTransaction` with iOS-only params, so missing Android params is a valid no-op success case for the iOS stub path on Android. Platform detection to distinguish iOS stub calls from genuine Android caller errors is a future improvement.
…nnection Address PR review feedback: - Wrap each listener callback in try/catch so one throwing subscriber cannot prevent other listeners from receiving the event - Move resetListenerState() after native endConnection() succeeds, so JS listener state is preserved if endConnection fails - Sanitize error logs to avoid leaking full purchase objects Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.ts (1)
412-430: Consider using a strongly-typed interface instead ofany.
userChoiceBillingJsListenersand thelistenerparameter useanyfor the details type. For type safety and API consistency, consider defining or importing a canonical type (e.g.,UserChoiceBillingDetailsAndroid) fromsrc/types.ts, similar to howDeveloperProvidedBillingDetailsAndroidis defined at line 518.As per coding guidelines: "When declaring API params/results in JS/TS modules, import canonical types from
src/types.tsrather than creating ad-hoc interfaces."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 412 - 430, Replace the ad-hoc any types with the canonical type from src/types.ts: import and use UserChoiceBillingDetailsAndroid (or the appropriate exported name) for the Set, for userChoiceBillingNativeHandler's details parameter, and for the listener parameter in userChoiceBillingListenerAndroid; also ensure NitroUserChoiceBillingListener (referenced by userChoiceBillingNativeHandler) matches or is updated to accept UserChoiceBillingDetailsAndroid so the types are consistent across userChoiceBillingJsListeners, userChoiceBillingNativeHandler, and userChoiceBillingListenerAndroid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.ts`:
- Around line 412-430: Replace the ad-hoc any types with the canonical type from
src/types.ts: import and use UserChoiceBillingDetailsAndroid (or the appropriate
exported name) for the Set, for userChoiceBillingNativeHandler's details
parameter, and for the listener parameter in userChoiceBillingListenerAndroid;
also ensure NitroUserChoiceBillingListener (referenced by
userChoiceBillingNativeHandler) matches or is updated to accept
UserChoiceBillingDetailsAndroid so the types are consistent across
userChoiceBillingJsListeners, userChoiceBillingNativeHandler, and
userChoiceBillingListenerAndroid.
Without -X POST, gh api defaults to GET on the replies endpoint which returns 404. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
removePurchaseUpdatedListenercalledremoveAll()instead of removing only the specific listener, silently wiping ALL registered listenersSetfan-out for all 5 listener typesremove()now only deletes from the JS Set — other listeners remain intact regardless of iOS native behaviorRoot Cause
When multiple
useIAPhooks were active (e.g., a persistent top-level component + a screen-level component), unmounting one component calledremovePurchaseUpdatedListeneron iOS which triggeredpurchaseUpdatedListeners.removeAll(). This wiped all listeners including the ones from the still-mounted component, causingonPurchaseSuccessto silently stop firing.Android was unaffected because it correctly used
.remove(listener)instead of.removeAll().Before (broken)
After (fixed)
Changes
src/index.tsWeakMapper-listener tracking with module-levelSetper event typeremove()only deletes from JS Set (never calls native remove)resetListenerState()called inendConnection()for clean re-registrationpurchaseUpdated,purchaseError,promotedProduct,userChoiceBilling,developerProvidedBillingsrc/__tests__/index.test.tsremove()Test plan
yarn typecheckpassesyarn lintpassesyarn jest— 251 tests passCloses #3150
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation