feat(useIAP): add reconnect method for manual connection retry#3174
Conversation
When the initial auto-connect fails (e.g., Play Store not ready at mount time on Android), there was no way to retry the connection through the hook — connected would permanently stay false. This adds a reconnect() method that re-runs initConnection, updates the connected state, and re-registers event listeners if they were cleaned up during a previous failure. Related to hyochan/expo-iap#328
hyochan
added
🐛 bug
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 introduces a crucial enhancement to the 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. 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. Footnotes
|
📝 WalkthroughWalkthroughIntroduces Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useIAP Hook
participant IAP as Native IAP Module
participant Listeners as Listener Registry
participant ErrorHandler as onError
Hook->>IAP: buildAndroidConfig()\ncall initConnection(config)
alt initConnection resolves true
IAP-->>Hook: true
Hook->>Hook: set connected = true (if mounted)
Hook->>Listeners: registerListeners() (idempotent)
Hook-->>Hook: return true
else initConnection resolves false
IAP-->>Hook: false
Hook->>Hook: set connected = false
Hook-->>Hook: return false
else initConnection rejects
IAP-->>Hook: error
Hook->>ErrorHandler: invokeOnError(error)
Hook->>Listeners: cleanupListeners()
Hook->>Hook: set connected = false (if mounted)
Hook-->>Hook: return false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3174 +/- ##
==========================================
+ Coverage 68.90% 69.64% +0.74%
==========================================
Files 9 9
Lines 1791 1825 +34
Branches 584 596 +12
==========================================
+ Hits 1234 1271 +37
+ Misses 552 549 -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 new reconnect function to the useIAP hook, allowing manual retry of the store connection and re-registration of various listeners. The feedback indicates that the reconnect function is missing the re-registration of the userChoiceBillingAndroid listener, which could lead to missed events. Additionally, there is a suggestion to refactor duplicated logic between reconnect and initIapWithSubscriptions into helper functions to improve maintainability.
![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
🧹 Nitpick comments (1)
src/hooks/useIAP.ts (1)
486-505: Consider extracting config building logic to reduce duplication.The Android billing config construction is duplicated between
initIapWithSubscriptions(lines 369-389) andreconnect(lines 486-505). Extracting this into a shared helper would improve maintainability.♻️ Example helper extraction
const buildAndroidBillingConfig = useCallback(() => { if (Platform.OS !== 'android') return undefined; if (optionsRef.current?.enableBillingProgramAndroid) { return { enableBillingProgramAndroid: optionsRef.current.enableBillingProgramAndroid, }; } if (optionsRef.current?.alternativeBillingModeAndroid) { return { alternativeBillingModeAndroid: optionsRef.current.alternativeBillingModeAndroid, }; } return undefined; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useIAP.ts` around lines 486 - 505, The Android-specific billing config construction is duplicated in initIapWithSubscriptions and reconnect; extract it into a shared helper (e.g., buildAndroidBillingConfig) that reads optionsRef.current and returns either { enableBillingProgramAndroid: ... } or { alternativeBillingModeAndroid: ... } or undefined for non-Android, then replace the duplicated blocks in initIapWithSubscriptions and reconnect to call buildAndroidBillingConfig() to get the config.
🤖 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/hooks/useIAP.ts`:
- Around line 507-509: The reconnect function calls await initConnection(config)
and then proceeds to setConnected and register listeners without checking
component mount state; add a guard after the await (e.g., if
(!isMountedRef.current) return) in reconnect so it exits early if unmounted,
mirroring the behavior in initIapWithSubscriptions, preventing listener
registration/leaks; ensure the check is applied before any calls to setConnected
or listener setup that follow initConnection.
- Around line 546-556: The reconnect logic is missing re-registration of the
Android userChoiceBilling listener, so when listeners were cleaned up the
onUserChoiceBillingAndroid callback stops working; update the reconnect function
to attach subscriptionsRef.current.userChoiceBillingAndroid exactly like
initIapWithSubscriptions does by calling the same listener registration
(userChoiceBillingAndroid(...)) when
optionsRef.current?.onUserChoiceBillingAndroid is present and store the returned
subscription on subscriptionsRef.current.userChoiceBillingAndroid so it can be
cleaned up later, mirroring how purchaseUpdate, purchaseError, and
promotedProductIOS are handled.
---
Nitpick comments:
In `@src/hooks/useIAP.ts`:
- Around line 486-505: The Android-specific billing config construction is
duplicated in initIapWithSubscriptions and reconnect; extract it into a shared
helper (e.g., buildAndroidBillingConfig) that reads optionsRef.current and
returns either { enableBillingProgramAndroid: ... } or {
alternativeBillingModeAndroid: ... } or undefined for non-Android, then replace
the duplicated blocks in initIapWithSubscriptions and reconnect to call
buildAndroidBillingConfig() to get the config.
- Add isMountedRef check after async initConnection in reconnect - Add userChoiceBillingAndroid listener re-registration in reconnect - Add reconnect success/failure tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/__tests__/hooks/useIAP.test.ts (2)
378-401: Consider starting from a disconnected state for a more robust reconnect success test.The test asserts
api.connectedistrueafterreconnect(), but the component was already connected from the initial mount (thebeforeEachmock returnstrue). This means the assertion at line 400 doesn't actually verify thatreconnectupdates the state—it was alreadytrue.A more robust test would simulate an initial connection failure so
connectedstarts asfalse, then verifyreconnect()changes it totrue:💡 Suggestion for more robust test
it('reconnects and returns true on success', async () => { + // Start with a failed initial connection + jest.spyOn(IAP, 'initConnection').mockRejectedValueOnce(new Error('Initial failure')); + let api: any; const Harness = () => { api = useIAP(); return null; }; await act(async () => { TestRenderer.create(React.createElement(Harness)); }); await act(async () => {}); - // Reset mock to track reconnect call - (IAP.initConnection as jest.Mock).mockClear(); + // Verify initially disconnected, then setup successful reconnect + expect(api.connected).toBe(false); jest.spyOn(IAP, 'initConnection').mockResolvedValue(true as any); let result: boolean | undefined; await act(async () => { result = await api.reconnect(); }); expect(result).toBe(true); expect(api.connected).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/hooks/useIAP.test.ts` around lines 378 - 401, The test currently starts with a connected state because the beforeEach mock returns true, so change the IAP.initConnection mock behavior for the initial mount to resolve false (so useIAP's api.connected is false after mounting), then reset/spy and make IAP.initConnection resolve true for the reconnect call; specifically, when rendering the Harness (which calls useIAP) ensure IAP.initConnection is mocked to return false first, then later mockResolvedValue(true) before calling api.reconnect(), and assert that api.connected changes from false to true and result is true.
403-427: Consider adding an assertion forconnectedstate after failure.The failure test verifies
onErroris called andresultisfalse, but doesn't assert the expectedconnectedstate. Adding this would document the expected behavior (state should remain unchanged on reconnect failure).💡 Optional enhancement
expect(result).toBe(false); expect(onError).toHaveBeenCalledWith(reconnectError); + // connected state should remain unchanged (still true from initial connection) + expect(api.connected).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/hooks/useIAP.test.ts` around lines 403 - 427, Capture the prior connected state from the hook (e.g., const prevConnected = api.connected) before calling api.reconnect(), then after the reconnect failure assertions add an assertion that the connected state did not change (e.g., expect(api.connected).toBe(prevConnected)); locate this logic around the test's Harness/useIAP setup and the api.reconnect() call to ensure it verifies the hook's connected state remains unchanged on failure.
🤖 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/__tests__/hooks/useIAP.test.ts`:
- Around line 378-401: The test currently starts with a connected state because
the beforeEach mock returns true, so change the IAP.initConnection mock behavior
for the initial mount to resolve false (so useIAP's api.connected is false after
mounting), then reset/spy and make IAP.initConnection resolve true for the
reconnect call; specifically, when rendering the Harness (which calls useIAP)
ensure IAP.initConnection is mocked to return false first, then later
mockResolvedValue(true) before calling api.reconnect(), and assert that
api.connected changes from false to true and result is true.
- Around line 403-427: Capture the prior connected state from the hook (e.g.,
const prevConnected = api.connected) before calling api.reconnect(), then after
the reconnect failure assertions add an assertion that the connected state did
not change (e.g., expect(api.connected).toBe(prevConnected)); locate this logic
around the test's Harness/useIAP setup and the api.reconnect() call to ensure it
verifies the hook's connected state remains unchanged on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e21231c1-9a96-4545-81ae-4471738d8983
📒 Files selected for processing (2)
src/__tests__/hooks/useIAP.test.tssrc/hooks/useIAP.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useIAP.ts
…ation - Extract buildAndroidConfig, registerListeners, cleanupListeners from initIapWithSubscriptions and reconnect into shared useCallback helpers - Add isMountedRef check in reconnect after async initConnection - Add userChoiceBillingAndroid listener re-registration - Add tests for reconnect success/failure, Android billing config, and userChoiceBillingAndroid listener registration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/hooks/useIAP.ts (1)
366-390: Reuse the canonicalinitConnectionconfig type.
buildAndroidConfigis copying the connection config shape inline here. Please move this to, or reuse, the canonical type insrc/types.tsso new Android billing flags only need to be added in one place.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/hooks/useIAP.ts` around lines 366 - 390, Replace the ad-hoc inline Android config type in buildAndroidConfig with the canonical initConnection config type exported from src/types.ts: import that type (e.g., InitConnectionConfig or the exact exported name) and use a Narrowed/Partial/Pick of it for the Android-only fields (for example Partial<InitConnectionConfig> or Pick<InitConnectionConfig, 'enableBillingProgramAndroid' | 'alternativeBillingModeAndroid'>) so future Android billing flags are defined in one place; update the config variable and the function return type to use this imported type and remove the inline object shape.
🤖 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/hooks/useIAP.ts`:
- Around line 451-461: The effect cleanup is manually removing native listeners
but not clearing the idempotency refs, causing registerListeners to skip
re-registration; change all teardown paths (including the cleanup returned by
the effect and any early-exit cleanup in registerListeners) to call the shared
cleanup helper cleanupListeners() instead of calling
subscriptionsRef.current.*.remove() directly so the native listeners are removed
and the subscriptionsRef.current.* fields are set to undefined; update
references inside registerListeners, the effect cleanup, and any other
listener-teardown locations to invoke cleanupListeners() to ensure consistent,
idempotent listener teardown and re-registration.
- Around line 463-486: The hook sets connected via setConnected immediately
after initConnection completes, but registerListeners or subsequent steps may
throw leaving connected true while listeners are incomplete; change
initIapWithSubscriptions (and the similar reconnect flow) to defer calling
setConnected until after registerListeners succeeds (i.e., call
registerListeners() first, then setConnected(true)), and ensure that if
registerListeners throws you call cleanupListeners() and do not flip connected;
update error handling in initIapWithSubscriptions and reconnect to only set
connected on successful listener registration and to perform cleanup on failure.
---
Nitpick comments:
In `@src/hooks/useIAP.ts`:
- Around line 366-390: Replace the ad-hoc inline Android config type in
buildAndroidConfig with the canonical initConnection config type exported from
src/types.ts: import that type (e.g., InitConnectionConfig or the exact exported
name) and use a Narrowed/Partial/Pick of it for the Android-only fields (for
example Partial<InitConnectionConfig> or Pick<InitConnectionConfig,
'enableBillingProgramAndroid' | 'alternativeBillingModeAndroid'>) so future
Android billing flags are defined in one place; update the config variable and
the function return type to use this imported type and remove the inline object
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a2b3796-456e-47a1-9d14-af46b8288f7f
📒 Files selected for processing (3)
src/__tests__/hooks/useIAP.android.test.tssrc/__tests__/hooks/useIAP.test.tssrc/hooks/useIAP.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/hooks/useIAP.test.ts
…ected - Use shared cleanupListeners() in useEffect cleanup to reset refs - Defer setConnected(true) until after registerListeners() succeeds - Add cleanupListeners in reconnect catch to prevent partial registrations - Guard setConnected with isMountedRef in catch blocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useIAP.ts (1)
367-389: Use the canonicalinitConnectionconfig type here.This helper re-declares the
initConnectionpayload inline, so it can drift the next time a billing flag is added. Please import the canonical config type fromsrc/types.tsas a type-only import and annotatebuildAndroidConfigwith it instead of maintaining a local shape.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/hooks/useIAP.ts` around lines 367 - 389, Replace the ad-hoc inline payload in buildAndroidConfig with the canonical initConnection config type from src/types.ts: add a type-only import (e.g., import type { InitConnectionConfig } from 'src/types') and change the local declaration so config is typed as InitConnectionConfig | undefined; keep the same runtime logic inside buildAndroidConfig but ensure the returned object fields match the InitConnectionConfig shape and types (referencing the existing option fields like enableBillingProgramAndroid and alternativeBillingModeAndroid) so the function signature and returned value use the imported canonical type instead of the inline declaration.
🤖 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/hooks/useIAP.ts`:
- Around line 367-389: Replace the ad-hoc inline payload in buildAndroidConfig
with the canonical initConnection config type from src/types.ts: add a type-only
import (e.g., import type { InitConnectionConfig } from 'src/types') and change
the local declaration so config is typed as InitConnectionConfig | undefined;
keep the same runtime logic inside buildAndroidConfig but ensure the returned
object fields match the InitConnectionConfig shape and types (referencing the
existing option fields like enableBillingProgramAndroid and
alternativeBillingModeAndroid) so the function signature and returned value use
the imported canonical type instead of the inline declaration.
1 participant
When the initial auto-connect fails (e.g., Play Store not ready at mount time on Android), there was no way to retry the connection through the hook — connected would permanently stay false. This adds a reconnect() method that re-runs initConnection, updates the connected state, and re-registers event listeners if they were cleaned up during a previous failure.
Related to hyochan/expo-iap#328
Summary by CodeRabbit
New Features
Tests