fix(example): prevent product loading race condition on direct nav#35
Conversation
…tion Replace DisposableEffect with LaunchedEffect and add initialization state tracking to ensure products load before UI renders. Update all flow screens to properly wait for initConnection() and fetchProducts() to complete.
|
Warning Rate limit exceeded@hyochan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds per-screen coroutine-backed initialization state ( Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.25)
participant Screen as Composable Screen
participant Launch as LaunchedEffect(Unit)
participant Store as iapStore
participant Dispose as DisposableEffect
end
Screen->>Launch: on composition -> isInitializing = true
Launch->>Store: initConnection()
alt init succeeds
Store-->>Launch: success
Launch->>Store: optional getAvailablePurchases() / setActivity()
Store-->>Launch: purchases / activity set
else init fails
Store-->>Launch: error/exception
Launch->>Screen: set initError / post status
end
Launch->>Screen: isInitializing = false
Note right of Screen: UI gates Loading, Empty, actions on isInitializing/status
Screen->>Dispose: on dispose
Dispose->>Store: endConnection() & clear() via cleanupScope
Dispose->>Screen: cancel cleanupScope
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt (1)
99-107: Cleanup scope may be cancelled before execution.Using
uiScope.launch(fromrememberCoroutineScope) inonDisposeis unsafe because the scope is cancelled when the composition leaves, butonDisposeruns afterward. The cleanup coroutines may never execute.Perform cleanup synchronously or use a scope that's guaranteed to be active:
DisposableEffect(Unit) { onDispose { - // End connection and clear listeners when this screen leaves (per-screen lifecycle) - uiScope.launch { - runCatching { iapStore.endConnection() } - runCatching { iapStore.clear() } - } + // End connection and clear listeners when this screen leaves (per-screen lifecycle) + runCatching { iapStore.endConnection() } + runCatching { iapStore.clear() } } }Note: If
endConnection()andclear()are suspending functions that must run in a coroutine, consider usingMainScope().launchinstead, though that scope should eventually be cancelled separately.packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (1)
129-192: Initialization now runs twiceThis new
LaunchedEffectkicks offinitConnection(),setActivity(), andfetchProducts()immediately, but the existingDisposableEffectat Lines 275‑307 still launches the very same sequence viastartupScope. On first composition both coroutines run concurrently, soOpenIapStore.initConnection()and subsequent fetches race each other, reintroducing the initialization bug this PR set out to eliminate. It also means we attempt to tear the connection down twice on dispose. Please consolidate to a single effect (e.g., remove the redundantstartupScopeblock and keep one cleanup path) so initialization happens exactly once.- val startupScope = rememberCoroutineScope() - DisposableEffect(Unit) { - startupScope.launch { - ... - } - - onDispose { - startupScope.launch { - runCatching { iapStore.endConnection() } - runCatching { iapStore.clear() } - } - } - }
🧹 Nitpick comments (2)
packages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt (1)
32-47: Consider case-insensitive comparison for robustness.The conditional text logic is correct and provides clear platform-specific messaging. As an optional improvement, consider using case-insensitive comparison to make the build configuration more resilient:
- val isHorizonBuild = BuildConfig.OPENIAP_STORE == "horizon" + val isHorizonBuild = BuildConfig.OPENIAP_STORE.equals("horizon", ignoreCase = true)This would prevent issues if the build config value uses different casing (e.g., "Horizon", "HORIZON").
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt (1)
51-51: Consider consolidating coroutine scopes.Multiple
rememberCoroutineScope()calls exist at lines 51, 85, and 114. While not incorrect, using a single scope for all operations would be more efficient.Apply this diff to consolidate the scopes:
- val uiScope = rememberCoroutineScope() + val scope = rememberCoroutineScope() // Initialize and connect on first composition (spec-aligned names) LaunchedEffect(Unit) {Then at line 68, update the DisposableEffect:
DisposableEffect(Unit) { onDispose { - uiScope.launch { + scope.launch {And remove the local scope declarations at lines 85 and 114 to use the consolidated
scopeinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt(6 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt(3 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt(6 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-18T05:46:51.596Z
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 17
File: packages/google/openiap/src/main/java/dev/hyo/openiap/store/OpenIapStore.kt:620-629
Timestamp: 2025-10-18T05:46:51.596Z
Learning: In packages/google/openiap/src/main/java/**/*.kt (Android-only package): DO NOT add Android suffix to function names, even for Android-specific APIs. Exception: Only use Android suffix for cross-platform API types (e.g., ProductAndroid, PurchaseAndroid) that contrast with iOS types. Examples of correct naming: isHorizonEnvironment(context: Context), buildModule(context: Context), acknowledgePurchase(), consumePurchase().
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : In Android-only package, do not add Android suffix to function names (e.g., acknowledgePurchase, not acknowledgePurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : Only use Android suffix for types that are part of a cross-platform API (e.g., ProductAndroid, PurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/Types.kt : Do not edit generated file openiap/src/main/Types.kt in Android package
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/java/dev/hyo/openiap/utils/**/*.kt : Place reusable Kotlin helpers in openiap/src/main/java/dev/hyo/openiap/utils/
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt
🧬 Code graph analysis (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt (1)
packages/gql/src/generated/types.ts (1)
ProductRequest(361-364)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Android
🔇 Additional comments (10)
packages/google/Example/src/main/java/dev/hyo/martie/screens/HomeScreen.kt (3)
21-21: LGTM - BuildConfig import added correctly.The import is properly scoped to the example app package.
100-100: LGTM - Dynamic text correctly applied.The conditional text variables are properly used in the UI, ensuring the displayed messaging accurately reflects the compiled billing integration.
Also applies to: 208-208
28-30: BuildConfig.OPENIAP_STORE field is properly configured across all build variants.The verification confirms that the field is correctly defined with exact values "play" and "horizon" in both the Example and openiap modules' build gradle files. The case-sensitive string comparison at line 30 is appropriate and matches the compile-time constant values. No configuration issues were found.
packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt (5)
78-79: LGTM: Initialization state tracking added.The
isInitializingstate correctly distinguishes the initial setup phase from ongoing loading operations, which helps prevent race conditions as stated in the PR objectives.
133-133: LGTM: Refresh button correctly gated on initialization state.The button is properly disabled during initialization, preventing premature refresh actions.
203-203: LGTM: Loading state includes initialization phase.Correctly displays loading feedback during both initialization and subsequent loading operations.
293-293: LGTM: Empty state correctly suppressed during initialization.Prevents showing "No products available" while initialization is in progress, avoiding misleading UI states.
333-333: LGTM: Action buttons correctly disabled during initialization.Both Restore and Refresh buttons are properly gated on the initialization state, maintaining consistency and preventing race conditions.
Also applies to: 353-353
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt (2)
103-103: LGTM! Proper initialization gating.The button enabling logic and loading state correctly incorporate
isInitializingto prevent user interactions and show loading indicators during the initialization phase.Also applies to: 173-173
334-334: LGTM! Consistent initialization checks across UI.The empty state condition and action button enabling logic correctly prevent premature empty state display and user actions during initialization. This is consistent with the pattern established throughout the screen.
Also applies to: 406-406, 431-431
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (1)
293-326: Remove duplicate initialization code.Lines 293-326 contain the OLD initialization pattern using
DisposableEffectandrememberCoroutineScope(). This code conflicts with the NEW initialization pattern at lines 135-210 and will cause:
- Double initialization: Both effects will run, calling
initConnection()andfetchProducts()twice- Race condition reintroduced: The cleanup at lines 319-325 uses
startupScope(composition-tied), reintroducing the race condition that lines 212-220 were meant to fix- Code confusion: Having two different initialization patterns in the same screen is error-prone
Remove this entire block.
Apply this diff:
- // Initialize and connect on first composition (spec-aligned names) - val startupScope = rememberCoroutineScope() - DisposableEffect(Unit) { - startupScope.launch { - try { - println("SubscriptionFlow: Calling initConnection...") - val connected = iapStore.initConnection() - println("SubscriptionFlow: initConnection returned: $connected") - if (connected) { - iapStore.setActivity(activity) - println("SubscriptionFlow: Loading subscription products: $subscriptionSkus") - val request = ProductRequest( - skus = subscriptionSkus, - type = ProductQueryType.Subs - ) - iapStore.fetchProducts(request) - iapStore.getAvailablePurchases(null) - } else { - println("SubscriptionFlow: Failed to connect to billing service") - } - } catch (e: Exception) { - println("SubscriptionFlow: Exception during initConnection: ${e.message}") - e.printStackTrace() - } - } - - onDispose { - // End connection and clear listeners when this screen leaves (per-screen lifecycle) - startupScope.launch { - runCatching { iapStore.endConnection() } - runCatching { iapStore.clear() } - } - } - } -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt(7 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt(7 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/google/Example/src/main/java/dev/hyo/martie/screens/PurchaseFlowScreen.kt
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T05:46:51.596Z
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 17
File: packages/google/openiap/src/main/java/dev/hyo/openiap/store/OpenIapStore.kt:620-629
Timestamp: 2025-10-18T05:46:51.596Z
Learning: In packages/google/openiap/src/main/java/**/*.kt (Android-only package): DO NOT add Android suffix to function names, even for Android-specific APIs. Exception: Only use Android suffix for cross-platform API types (e.g., ProductAndroid, PurchaseAndroid) that contrast with iOS types. Examples of correct naming: isHorizonEnvironment(context: Context), buildModule(context: Context), acknowledgePurchase(), consumePurchase().
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : In Android-only package, do not add Android suffix to function names (e.g., acknowledgePurchase, not acknowledgePurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : Only use Android suffix for types that are part of a cross-platform API (e.g., ProductAndroid, PurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/java/dev/hyo/openiap/utils/**/*.kt : Place reusable Kotlin helpers in openiap/src/main/java/dev/hyo/openiap/utils/
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Android
🔇 Additional comments (11)
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt (6)
29-31: LGTM: Necessary imports for the new cleanup pattern.The added imports support the dedicated cleanup scope pattern, which resolves the race condition identified in previous reviews.
52-53: LGTM: Proper initialization state tracking.The initialization state and error tracking enable proper UI gating and error display, addressing the race condition mentioned in the PR objectives.
55-56: LGTM: Dedicated cleanup scope addresses race condition.The cleanup scope with
SupervisorJobensures that cleanup operations inonDisposecomplete reliably, addressing the critical race condition flagged in previous reviews.
59-72: LGTM: Comprehensive error handling for initialization.The initialization logic properly addresses the past review comment by capturing connection failures and exceptions with user-friendly error messages, and ensuring
isInitializingis always updated in thefinallyblock.
74-82: LGTM: Cleanup race condition resolved.The dedicated
cleanupScopepattern directly addresses the critical race condition from previous reviews. The cleanup operations can now complete reliably without racing with composition-tied scope cancellation.
112-112: LGTM: Comprehensive UI gating with initialization state.All UI elements properly respect the initialization state:
- Actions disabled during initialization (lines 112, 446, 471)
- Loading indicator shown during initialization (line 182)
- Error display for initialization failures (lines 188-217)
- Empty state deferred until initialization completes (line 374)
This ensures the UI accurately reflects the initialization lifecycle.
Also applies to: 182-182, 188-217, 374-374, 446-446, 471-471
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (5)
129-132: LGTM: Initialization state and cleanup scope properly implemented.The initialization state tracking and dedicated cleanup scope follow the same correct pattern as
AvailablePurchasesScreen.kt.
135-210: LGTM: Comprehensive initialization with error handling.The initialization logic properly:
- Connects to the billing service
- Sets the activity context on success
- Fetches products with detailed logging
- Posts error status messages on failure
- Ensures
isInitializingis set to false infinallyThis follows the pattern established in
AvailablePurchasesScreen.kt.
212-220: LGTM: Cleanup pattern addresses race condition.The dedicated
cleanupScopeensures cleanup operations complete reliably, consistent with the pattern inAvailablePurchasesScreen.kt.
432-432: LGTM: Loading state properly gated with initialization.The loading indicator correctly considers both
isInitializingandstatus.isLoading.
1103-1103: LGTM: Empty state properly gated with initialization.The empty state is correctly deferred until initialization completes, consistent with the pattern in
AvailablePurchasesScreen.kt.
…ubscriptionFlowScreen.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (1)
293-325: Critical: Duplicate initialization creates race condition.This
DisposableEffectblock (lines 293-325) duplicates the initialization logic from theLaunchedEffectat lines 134-209. Both blocks call:
iapStore.initConnection()iapStore.setActivity(activity)iapStore.fetchProducts(request)iapStore.getAvailablePurchases(null)This creates a race condition where initialization happens twice concurrently, which is the exact problem this PR claims to fix. Additionally, the cleanup logic is duplicated between lines 211-219 and lines 318-324.
Remove this entire
DisposableEffectblock since initialization is already handled by theLaunchedEffectat lines 134-209 and cleanup is handled by theDisposableEffectat lines 211-219:- // Initialize and connect on first composition (spec-aligned names) - val startupScope = rememberCoroutineScope() - DisposableEffect(Unit) { - startupScope.launch { - try { - println("SubscriptionFlow: Calling initConnection...") - val connected = iapStore.initConnection() - println("SubscriptionFlow: initConnection returned: $connected") - if (connected) { - iapStore.setActivity(activity) - println("SubscriptionFlow: Loading subscription products: $subscriptionSkus") - val request = ProductRequest( - skus = subscriptionSkus, - type = ProductQueryType.Subs - ) - iapStore.fetchProducts(request) - iapStore.getAvailablePurchases(null) - } else { - println("SubscriptionFlow: Failed to connect to billing service") - } - } catch (e: Exception) { - println("SubscriptionFlow: Exception during initConnection: ${e.message}") - e.printStackTrace() - } - } - - onDispose { - // End connection and clear listeners when this screen leaves (per-screen lifecycle) - startupScope.launch { - runCatching { iapStore.endConnection() } - runCatching { iapStore.clear() } - } - } - } -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-18T05:46:51.596Z
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 17
File: packages/google/openiap/src/main/java/dev/hyo/openiap/store/OpenIapStore.kt:620-629
Timestamp: 2025-10-18T05:46:51.596Z
Learning: In packages/google/openiap/src/main/java/**/*.kt (Android-only package): DO NOT add Android suffix to function names, even for Android-specific APIs. Exception: Only use Android suffix for cross-platform API types (e.g., ProductAndroid, PurchaseAndroid) that contrast with iOS types. Examples of correct naming: isHorizonEnvironment(context: Context), buildModule(context: Context), acknowledgePurchase(), consumePurchase().
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : In Android-only package, do not add Android suffix to function names (e.g., acknowledgePurchase, not acknowledgePurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : Only use Android suffix for types that are part of a cross-platform API (e.g., ProductAndroid, PurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Android
- GitHub Check: Test iOS
🔇 Additional comments (3)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (3)
44-46: The duplicate import issue from the previous review appears to be resolved.The imports for
CoroutineScope,Dispatchers, andSupervisorJobare needed for the newcleanupScopefunctionality.
134-209: Verify if arbitrary delays are still necessary.The
delay(500)calls at lines 156 and 175 suggest the code is working around timing issues rather than properly coordinating async operations. With the new initialization state tracking, these delays may no longer be necessary.Consider removing these delays and relying on proper async flow coordination instead. If they're required for external API timing constraints, add comments explaining why.
431-431: LGTM: Proper initialization state checks.The loading condition at line 431 and empty state condition at line 1102 correctly incorporate
isInitializingto prevent UI rendering before initialization completes. This ensures products load before the UI renders them.Also applies to: 1102-1102
…ubscriptionFlowScreen.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (3)
298-331: Critical: Remove duplicate initialization logic.This
DisposableEffect(Unit)block (lines 300-331) duplicates the initialization already performed by theLaunchedEffect(Unit)at lines 140-215. Both will run on first composition, causing:
- Double initialization of the billing connection
- Products fetched twice
- Potential race conditions
Since initialization is now handled by the
LaunchedEffectwith properisInitializingstate tracking, and cleanup is handled by theDisposableEffectat lines 217-225, this entire block should be removed.Apply this diff to remove the duplicate initialization:
- // Initialize and connect on first composition (spec-aligned names) - val startupScope = rememberCoroutineScope() - DisposableEffect(Unit) { - startupScope.launch { - try { - println("SubscriptionFlow: Calling initConnection...") - val connected = iapStore.initConnection() - println("SubscriptionFlow: initConnection returned: $connected") - if (connected) { - iapStore.setActivity(activity) - println("SubscriptionFlow: Loading subscription products: $subscriptionSkus") - val request = ProductRequest( - skus = subscriptionSkus, - type = ProductQueryType.Subs - ) - iapStore.fetchProducts(request) - iapStore.getAvailablePurchases(null) - } else { - println("SubscriptionFlow: Failed to connect to billing service") - } - } catch (e: Exception) { - println("SubscriptionFlow: Exception during initConnection: ${e.message}") - e.printStackTrace() - } - } - - onDispose { - // End connection and clear listeners when this screen leaves (per-screen lifecycle) - startupScope.launch { - runCatching { iapStore.endConnection() } - runCatching { iapStore.clear() } - } - } - } -
357-357: Add initialization check to refresh button.The refresh button should also check
!isInitializingto prevent refreshing during initialization.Apply this diff:
IconButton( onClick = { scope.launch { try { iapStore.setActivity(activity) val request = ProductRequest( skus = subscriptionSkus, type = ProductQueryType.Subs ) iapStore.fetchProducts(request) } catch (_: Exception) { } } }, - enabled = !status.isLoading + enabled = !isInitializing && !status.isLoading ) {
706-706: Add initialization check to upgrade buttons.Both upgrade/switch buttons should check
!isInitializingin addition to!status.isPurchasing()to prevent purchase attempts during initialization.Apply this diff to both buttons (lines 706 and 961):
- enabled = !status.isPurchasing(IapConstants.PREMIUM_PRODUCT_ID) + enabled = !isInitializing && !status.isPurchasing(IapConstants.PREMIUM_PRODUCT_ID)- enabled = !status.isPurchasing(PREMIUM_SUBSCRIPTION_PRODUCT_ID) + enabled = !isInitializing && !status.isPurchasing(PREMIUM_SUBSCRIPTION_PRODUCT_ID)Also applies to: 961-961
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T05:46:51.596Z
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 17
File: packages/google/openiap/src/main/java/dev/hyo/openiap/store/OpenIapStore.kt:620-629
Timestamp: 2025-10-18T05:46:51.596Z
Learning: In packages/google/openiap/src/main/java/**/*.kt (Android-only package): DO NOT add Android suffix to function names, even for Android-specific APIs. Exception: Only use Android suffix for cross-platform API types (e.g., ProductAndroid, PurchaseAndroid) that contrast with iOS types. Examples of correct naming: isHorizonEnvironment(context: Context), buildModule(context: Context), acknowledgePurchase(), consumePurchase().
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : Only use Android suffix for types that are part of a cross-platform API (e.g., ProductAndroid, PurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Android
🔇 Additional comments (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (1)
44-46: LGTM!The coroutine imports are correctly added to support the cleanupScope pattern.
- Add missing launch import for cleanupScope - Remove duplicate DisposableEffect blocks - Fix AppColors references in error UI
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (1)
131-132: Memory leak:cleanupScopeis never cancelled.The
cleanupScopewithSupervisorJob()is created but never cancelled, causing a memory leak when the composable leaves composition. Past reviews indicated this should be fixed by cancelling the scope in aDisposableEffect.Apply this fix to cancel the scope:
// Use a dedicated scope for cleanup that won't be cancelled with composition val cleanupScope = remember { CoroutineScope(Dispatchers.Main + SupervisorJob()) } + +DisposableEffect(cleanupScope) { + onDispose { + cleanupScope.cancel() + } +}Add the required import:
import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt(7 hunks)packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T05:46:51.596Z
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 17
File: packages/google/openiap/src/main/java/dev/hyo/openiap/store/OpenIapStore.kt:620-629
Timestamp: 2025-10-18T05:46:51.596Z
Learning: In packages/google/openiap/src/main/java/**/*.kt (Android-only package): DO NOT add Android suffix to function names, even for Android-specific APIs. Exception: Only use Android suffix for cross-platform API types (e.g., ProductAndroid, PurchaseAndroid) that contrast with iOS types. Examples of correct naming: isHorizonEnvironment(context: Context), buildModule(context: Context), acknowledgePurchase(), consumePurchase().
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : Only use Android suffix for types that are part of a cross-platform API (e.g., ProductAndroid, PurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/java/dev/hyo/openiap/utils/**/*.kt : Place reusable Kotlin helpers in openiap/src/main/java/dev/hyo/openiap/utils/
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
📚 Learning: 2025-10-18T05:54:54.802Z
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T05:54:54.802Z
Learning: Applies to packages/google/openiap/src/main/**/*.kt : In Android-only package, do not add Android suffix to function names (e.g., acknowledgePurchase, not acknowledgePurchaseAndroid)
Applied to files:
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.ktpackages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Android
🔇 Additional comments (10)
packages/google/Example/src/main/java/dev/hyo/martie/screens/AvailablePurchasesScreen.kt (6)
52-53: LGTM!Initialization state tracking is properly implemented. The
isInitializingflag starts astrueandinitErroris nullable, which correctly gates UI rendering during startup.
59-72: LGTM!Initialization logic is well-structured with proper error handling. The try/catch/finally ensures
isInitializingis cleared regardless of success or failure, and error messages are captured ininitErrorfor user feedback.
74-82: Cleanup pattern improved but depends on scope lifecycle fix.Using
cleanupScopeinstead of a composition-tied scope avoids the immediate cancellation race described in past reviews. However, this cleanup will only work correctly if the scope lifecycle issue at lines 55-56 is addressed (the scope must be cancelled when the composable disposes).
112-112: LGTM!Button states are correctly gated on both
isInitializingandstatus.isLoading, preventing user actions during initialization and ongoing operations.Also applies to: 446-446, 471-471
182-182: LGTM!Loading and empty state logic correctly incorporates
isInitializingto prevent premature empty state display during startup.Also applies to: 374-374
188-217: LGTM!Initialization error UI is well-implemented with clear visual feedback using the danger color scheme and appropriate iconography.
packages/google/Example/src/main/java/dev/hyo/martie/screens/SubscriptionFlowScreen.kt (4)
129-129: LGTM!Initialization state tracking is properly initialized and follows the same pattern as other screens in this PR.
135-211: LGTM!Initialization logic is comprehensive with proper error handling. Using status messages for error feedback is consistent with the existing pattern in this screen. The try/catch/finally ensures
isInitializingis cleared even on failure, preventing stuck loading states.
213-221: Cleanup pattern improved but depends on scope lifecycle fix.Using
cleanupScopeavoids the cancellation race with composition-tied scopes. However, this cleanup depends on the scope lifecycle issue at lines 131-132 being addressed (the scope must be cancelled when the composable disposes).
398-398: LGTM!Loading and empty state logic properly gates UI on
isInitializingto prevent premature empty state display and ensure loading feedback during startup.Also applies to: 1069-1069
- Add DisposableEffect to cancel cleanupScope and prevent memory leaks - Remove duplicate initialization code in SubscriptionFlowScreen - Fix missing imports and AppColors references
1 participant
Replace DisposableEffect with LaunchedEffect and add initialization state tracking to ensure products load before UI renders. Update all flow screens to properly wait for initConnection() and fetchProducts() to complete.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements