fix(react-native-iap): avoid iOS listener cleanup crash after unmount - #262
Conversation
|
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:
📝 WalkthroughWalkthroughThe pull request preserves purchase success delivery after ChangesListener lifecycle safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PurchaseEvent
participant useIAP
participant SubscriptionRefresh
participant onPurchaseSuccess
PurchaseEvent->>useIAP: deliver purchase update
useIAP->>SubscriptionRefresh: refresh active subscriptions
PurchaseEvent->>useIAP: unmount hook
SubscriptionRefresh-->>useIAP: return refreshed subscriptions
useIAP->>useIAP: guard internal state updates
useIAP-->>onPurchaseSuccess: deliver purchase success
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libraries/react-native-iap/src/hooks/useIAP.ts (1)
588-600: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-check mount state after the asynchronous refreshes.
The guard at Line 588 runs before two
awaitcalls. If cleanup runs while either call is pending, execution resumes and invokesonPurchaseSuccessat Lines 598-600 after unmount. Add a second mount check after the awaits. Add a regression test that unmounts while the refresh is pending.As per coding guidelines, follow React Native best practices for component structure and lifecycle management.
Proposed fix
await getActiveSubscriptionsInternal(); await getAvailablePurchasesInternal(); + if (!isMountedRef.current) { + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libraries/react-native-iap/src/hooks/useIAP.ts` around lines 588 - 600, In the post-purchase refresh flow around getActiveSubscriptionsInternal and getAvailablePurchasesInternal, re-check isMountedRef.current after both awaited refreshes and before invoking optionsRef.current?.onPurchaseSuccess, returning when the hook has unmounted. Add a regression test that unmounts the hook while refresh is pending and verifies the success callback is not called.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libraries/react-native-iap/src/hooks/useIAP.ts`:
- Around line 588-600: In the post-purchase refresh flow around
getActiveSubscriptionsInternal and getAvailablePurchasesInternal, re-check
isMountedRef.current after both awaited refreshes and before invoking
optionsRef.current?.onPurchaseSuccess, returning when the hook has unmounted.
Add a regression test that unmounts the hook while refresh is pending and
verifies the success callback is not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d016f82e-901a-413e-bef6-3f08b1b1d8a4
📒 Files selected for processing (2)
libraries/react-native-iap/src/hooks/useIAP.tslibraries/react-native-iap/src/index.ts
6dd403f to
b813f62
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libraries/react-native-iap/src/__tests__/hooks/useIAP.test.ts`:
- Around line 155-164: Update the test setup around the captured purchase
listener and active-subscriptions resolver to assert both handles are
initialized before calling renderer.unmount(). After those assertions, invoke
capturedPurchaseListener and resolveActiveSubscriptions directly without
optional chaining, preserving the existing await sequence so the test fails when
either event path is not initialized.
🪄 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 Plus
Run ID: 86b4f5d1-a080-470d-949b-6e88bdc1f316
📒 Files selected for processing (3)
libraries/react-native-iap/src/__tests__/hooks/useIAP.test.tslibraries/react-native-iap/src/hooks/useIAP.tslibraries/react-native-iap/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libraries/react-native-iap/src/index.ts
- libraries/react-native-iap/src/hooks/useIAP.ts
b813f62 to
8ee6518
Compare
8ee6518 to
7a5f95e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libraries/react-native-iap/src/__tests__/index.test.ts (1)
232-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert removal of the non-deduping JavaScript callback.
Lines [240] and [243] only verify that native removal does not occur. A stale duplicate callback would still pass this test. Capture the duplicate callback and native handler, dispatch a valid purchase before and after
duplicateSub.remove(), and assert that the callback is not invoked after removal.As per coding guidelines, cover both callback removal and native listener retention for this IAP operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libraries/react-native-iap/src/__tests__/index.test.ts` around lines 232 - 243, Strengthen the test around IAP.purchaseUpdatedListener by capturing the duplicate callback and native purchase-update handler, dispatching a valid purchase before and after duplicateSub.remove(), and asserting the duplicate callback runs only before removal. Retain the existing assertions that native removal is not called, including after defaultSub.remove(), to cover both JavaScript callback removal and native listener retention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libraries/react-native-iap/src/__tests__/index.test.ts`:
- Around line 232-243: Strengthen the test around IAP.purchaseUpdatedListener by
capturing the duplicate callback and native purchase-update handler, dispatching
a valid purchase before and after duplicateSub.remove(), and asserting the
duplicate callback runs only before removal. Retain the existing assertions that
native removal is not called, including after defaultSub.remove(), to cover both
JavaScript callback removal and native listener retention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d80d6a8a-51c7-40a6-bc1c-e356b192f601
📒 Files selected for processing (4)
libraries/react-native-iap/src/__tests__/hooks/useIAP.test.tslibraries/react-native-iap/src/__tests__/index.test.tslibraries/react-native-iap/src/hooks/useIAP.tslibraries/react-native-iap/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libraries/react-native-iap/src/tests/hooks/useIAP.test.ts
- libraries/react-native-iap/src/index.ts
- libraries/react-native-iap/src/hooks/useIAP.ts
hyochan
left a comment
hyochan
left a comment
There was a problem hiding this comment.
Thanks for the thorough fix and the clear PR description. I did a deep pass over the listener lifecycle, the hook semantics, tests, and cross-SDK impact. Overall the approach is sound, and it actually converges with how expo-iap already works: there, native OpenIAP listeners attach once at module creation and are only disposed at module destroy, while JS remove() detaches only the JS callback. So keeping the iOS singleton bridge attached for the session moves react-native-iap toward the sibling SDK's architecture, and it matches StoreKit 2 guidance to keep a Transaction.updates observer alive for the app's lifetime.
Verified sound (no action needed):
- Native listener count stays bounded:
purchaseUpdateNativeAttached/token stay set after the iOS early return, so re-subscribes never double-attach, and delivery to new JS callbacks keeps working through the retained handler. endConnectionstill cleans everything up (resetListenerState()mirrors the nativedetachConnectionState), so nothing outlives the connection.- Zero-JS-listener dispatch is a safe no-op, and events arriving with zero listeners were dropped natively before this PR too — no new event-loss class.
- Android and the other platforms keep the exact pre-PR removal path; docs never promised native disposal on
remove(), so no docs contradiction.
Findings are in the inline comments. The one I'd like resolved before merge is the silent onPurchaseSuccess drop (major); the rest are test-coverage gaps and one clarification on the crash mechanism.
One parity note for a follow-up (not this PR): expo-iap's useIAP has the same post-unmount onPurchaseSuccess bug this PR fixes — its purchase-update callback awaits refreshSubscriptionStatus(...) and then calls optionsRef.current.onPurchaseSuccess(purchase) with no mount guard (libraries/expo-iap/src/useIAP.ts:653-667, and the re-subscribe path at 777-789). Worth a mirrored fix there once the semantics are settled here.
Deliver onPurchaseSuccess even when the hook unmounts while the post-purchase refresh is pending: it is where apps call finishTransaction, and a dropped event has no in-session redelivery (Android auto-refunds unacknowledged purchases). Drop the sync-path mount guards, which were unreachable: unmount cleanup removes the JS callback from the fan-out set synchronously, so callbacks only enter while mounted. Internal setState stays mount-guarded in the refresh helpers. Extend the iOS keep-attached policy to purchaseErrorListener.remove() so the same modal-dismiss teardown cannot release the stored native callback while a dispatch snapshot is in flight; endConnection still owns native disposal. Tests: pin delivery-after-unmount, restore Android token-removal coverage, pin the iOS re-subscribe-after-removal invariant, and reset capturedPurchaseListener between tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@MadeinFrance Thanks again for this fix — the core insight (keep the iOS singleton native listener attached for the session; the JS callback set is the real subscription surface) is right, and it converges with how expo-iap already works. To speed things up I pushed b922850 using maintainer edits, addressing everything from my review in one pass:
The PR description's line about re-checking mounted state before onPurchaseSuccess no longer matches the code — could you update it when you get a chance? Happy to revert any part of this if you disagree with the direction. |
|
@MadeinFrance The review cycle on this branch is complete: CI is green, the full jest suite (411 tests) passes, and an independent verification round over b922850 came back clean — including mutation checks confirming each new test genuinely pins its behavior (delivery-after-unmount, Android token removal, iOS re-subscribe reuse, iOS error-listener retention). Since you have the only real reproduction environment (the consumer app you validated with patch-package), could you re-test the updated branch against the original scenario before we merge?
And if your crash log points specifically at |
|
@hyochan Thanks for the edits, the new semantics make sense to me. Keeping I also agree with extending the iOS keep-attached policy to I updated the PR description. |
|
@hyochan Tested in the Abandoned World app release build. Everything works correctly with the updated branch:
Looks good from the consumer app side. |
|
@MadeinFrance Deployment is complete — thank you again for the fix and for validating the final behavior in the consumer app release build.
No further action is required for this PR. |
2 participants
Summary
This keeps the iOS StoreKit-backed singleton native purchase and purchase-error listeners attached for the app session while JavaScript subscriptions are still removed normally.
useIAPkeeps internal state updates mount-guarded during post-purchase refresh, but still deliversonPurchaseSuccessfor a purchase event that already entered before unmount so apps can callfinishTransaction.Why
In a React Native app using
useIAPfrom a modal-like plans screen, dismissing the screen can unmount the hook while StoreKit/Nitro purchase listener work is still in flight. The JS subscription cleanup removes the JS callback, but when it is also the last callback it previously disposed the native iOS listener immediately.On iOS release builds this can abort during screen dismissal. Keeping the singleton native listener attached for the app session avoids that teardown race while still removing the JS callback, so unmounted screens no longer receive new purchase, purchase-error, promoted-product, or billing-issue callbacks.
For a purchase event that already entered before unmount,
onPurchaseSuccessremains deliverable after the pending refresh completes. That callback is the documentedfinishTransactionsite, and dropping it would risk leaving the purchase unfinished.Android behavior is unchanged: native listener removal still happens when the last JS listener is removed.
Validation
yarn typecheck:libinlibraries/react-native-iapyarn lintinlibraries/react-native-iapyarn lint:tscinlibraries/react-native-iapyarn test:ci --watchman=false --silentinlibraries/react-native-iapgit diff --checknpm run typecheckSummary by CodeRabbit