[PC-1482] 미처리 구매 내역 검증 및 재화 지급 처리 #182
Conversation
| viewModel.handlePurchaseEvent(product) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이부분 purchaseProduct 를 앱 전체에서 수집해야하는걸로 이해했는데, 매칭 route 안의 LaunchedEffect 안에 두면 매칭라우트를 벗어날 때 코루틴이 종료되지않나요?
There was a problem hiding this comment.
공통 로직으로 통합할 경우 스토어와의 관찰 우선순위에 따라 사이드 이펙트가 발생할 수 있어, 현재는 화면별로 로직을 분리했습니다.
또한 유저의 퍼즐 수를 표시하는 곳은 매칭 메인과 스토어 둘 뿐이기 때문에 해당 화면에 진입 시 하면 될 거라고 생각했습니다.
| viewModel.billingHelper.purchaseProduct.collect { product -> | ||
| viewModel.handlePurchaseEvent(product) | ||
| } | ||
| } |
There was a problem hiding this comment.
여기도 비슷한 의문입니도
(추가) 뷰모델의 purchaseProduct collect를 왜 screen으로 빼셨는지 ??
There was a problem hiding this comment.
매칭 로직 먼저 짜고 그대로 가져온 거 같네요, 이건 뷰모델로 넣어도 될 거 같습니다
| viewModelScope.launch { | ||
| val job1 = launch { getUserPuzzle() } | ||
| val job2 = launch { getAvailableProducts() } | ||
| val job3 = launch { getUserInfo() } |
There was a problem hiding this comment.
사소하지만 job Naming을 좀 더 알아보기 좋게만들면 좋을듯합니다
| billingHelper.verifyPurchaseProduct(product) | ||
| }.onSuccess { | ||
| initPuzzleCount() | ||
| }.onFailure{ } |
There was a problem hiding this comment.
요 Fail 처리는 구현이 안된건가요? 아니면 todo 인가요?
There was a problem hiding this comment.
유저한테는 백그라운드 기능이라 .onFailure는 지우겠습니다
|
고생하셨습니다 ~! |
|
Tip For best results, initiate chat on the files or code changes. @comst19: Hello! How can I assist you with this PR? I can help with:
Please let me know what you'd like me to do! 😊 |
|
@comst19 네, PR을 리뷰하겠습니다. 미처리 구매 내역 검증 및 재화 지급 처리 관련 변경사항들을 확인해보겠습니다. ✅ Actions performedReview triggered.
|
WalkthroughReplaces StateFlow-based purchase signaling with a Channel-backed Flow of typed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Store / Matching)
participant VM as ViewModel
participant BH as BillingHelper (interface)
participant BI as BillingHelperImpl
participant BC as BillingClient
participant Repo as PaymentRepository
UI->>VM: OnPurchaseClick(product, userId)
VM->>BH: purchaseProduct(activity, product, userId)
BH->>BI: start purchase (setObfuscatedAccountId = userId)
BI->>BC: launchBillingFlow(...)
BC-->>BI: onPurchasesUpdated(resultCode, purchases)
alt OK && purchases != null
BI->>BI: filter PURCHASED purchases
loop per purchased item
BI->>Repo: verifyPurchaseProduct(purchase)
alt verification success
BI-->>BH: emit PurchaseResult.Success(reward)
else verification failure
BI-->>BH: emit PurchaseResult.Failure(error)
end
end
BH-->>VM: purchaseResult Flow emits results
VM->>VM: refresh puzzle count / UI on Success
else non-cancel error
BI-->>BH: emit PurchaseResult.Failure(error)
BH-->>VM: purchaseResult Flow emits Failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@comst19 네, PR 리뷰를 진행하겠습니다. 미처리 구매 내역 검증 및 재화 지급 처리 관련 변경사항들을 자세히 검토해보겠습니다. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (1)
64-69: Consider moving purchase collection to ViewModel.Past review noted that the purchaseProduct collection could be moved to the ViewModel. While the current implementation works, handling purchase flows in the ViewModel would better separate UI and business logic concerns.
feature/store/src/main/java/com/puzzle/store/contract/StoreSideEffect.kt (1)
6-6: userId type inconsistency across store contracts.This defines
userId: String, but StoreState usesuserId: Int. Refer to the comment on StoreState.kt for details on this type mismatch.
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
259-265: Consider adding the dispatcher for consistency.Other coroutine launches in this ViewModel (e.g.,
initPuzzleCount,initMatchInfo) useviewModelScope.launch(dispatcher), but this function usesviewModelScope.launchwithout the dispatcher. For consistency, consider adding the dispatcher parameter.🔎 Suggested fix
- internal fun handlePurchaseEvent(product: PurchaseProduct) = viewModelScope.launch { + internal fun handlePurchaseEvent(product: PurchaseProduct) = viewModelScope.launch(dispatcher) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(7 hunks)core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt(1 hunks)core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt(1 hunks)core/data/src/main/java/com/puzzle/data/di/DataModule.kt(2 hunks)feature/matching/build.gradle.kts(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt(5 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(5 hunks)feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.kt(3 hunks)feature/store/src/main/java/com/puzzle/store/StoreScreen.kt(2 hunks)feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt(4 hunks)feature/store/src/main/java/com/puzzle/store/contract/StoreSideEffect.kt(1 hunks)feature/store/src/main/java/com/puzzle/store/contract/StoreState.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)
🪛 detekt (1.23.8)
core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt
[warning] 37-37: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 43-43: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (19)
feature/matching/build.gradle.kts (1)
27-27: LGTM!The billing module dependency addition correctly enables purchase verification and billing functionality in the matching feature.
feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (1)
39-39: LGTM!The import enables coroutine usage for handling side effects.
feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.kt (1)
3-3: LGTM!The test correctly integrates FakeBillingHelper to support the new billing dependency in MatchingViewModel.
Also applies to: 52-52
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
78-84: LGTM!The lifecycle-aware purchase event handling is correctly implemented. The author's explanation that screen-level collection avoids priority conflicts between matching and store screens is reasonable given that only these two screens display puzzle counts.
core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt (2)
18-26: LGTM!The SharedFlow setup with
extraBufferCapacityandBufferOverflow.DROP_OLDESTis appropriate for test scenarios. TheemitPurchaseEventhelper enables deterministic testing of purchase flows.
33-43: Empty method bodies are intentional for this test fake.The static analysis warnings about empty function blocks are false positives. Test doubles commonly use no-op implementations to provide minimal, predictable behavior for unit tests.
gradle/libs.versions.toml (1)
32-32: Version 8.2.1 ofcom.android.billingclient:billing-ktxis verified and has no known security advisories.This is an officially supported version with no published vulnerabilities.
feature/store/src/main/java/com/puzzle/store/contract/StoreState.kt (1)
11-11: Type mismatch: userId is Int in StoreState but String in StoreSideEffect.PurchaseProduct.However, this is already handled—when creating the PurchaseProduct side effect in StoreViewModel.kt line 67, the userId is explicitly converted using
.toString(). No action required.Likely an incorrect or invalid review comment.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
50-54: LGTM on dependency injection pattern.The
billingHelperfollows the sameinternal valvisibility pattern aseventHelperandnavigationHelper, maintaining consistency for testability purposes.
111-112: LGTM on unprocessed purchase check placement.Calling
checkUnprocessedPurchases()after user info validation ensures purchases are only processed for authenticated, non-banned users.core/data/src/main/java/com/puzzle/data/di/DataModule.kt (2)
85-89: LGTM on PaymentRepository binding.The binding follows the established pattern for repository bindings in this module.
104-113: LGTM on BillingModule scope change to Singleton.Moving
BillingHelperto singleton scope is appropriate as it manages theBillingClientconnection lifecycle and needs to be shared across ViewModels (Matching and Store) to handle unprocessed purchases consistently.core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt (1)
11-17: Good API evolution for event-based purchase flow.The changes are well-designed:
SharedFlow<PurchaseProduct>(non-nullable) is more appropriate thanStateFlow<PurchaseProduct?>for discrete purchase events- Adding
userIdtopurchaseProduct()enables purchase-to-user association viasetObfuscatedAccountIdcheckUnprocessedPurchases()addresses the PR objective of handling pending purchasesOne consideration: the removal of
release()relies onBillingClient'senableAutoServiceReconnection()for lifecycle management. Verify this is sufficient for your use case.feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (3)
73-84: LGTM on purchase collection update.The switch from
collectLatesttocollectis appropriate since theSharedFlownow emits non-nullPurchaseProductevents. Error handling is properly maintained.
114-122: LGTM on getUserInfo implementation.The function follows the established pattern in this ViewModel and properly handles errors via
errorHelper.
66-67: Invalid userId handling for purchase operations.The
userIdis a non-nullableIntwith a default value of-1. IfgetUserInfo()fails, the purchase will proceed with-1as the user ID, which is invalid and will cause billing issues. Ensure the purchase is blocked or deferred until a valid userId is available.Likely an incorrect or invalid review comment.
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
46-51: LGTM on SharedFlow configuration.The
MutableSharedFlowwithreplay = 0,extraBufferCapacity = 1, andBufferOverflow.DROP_OLDESTis appropriate for discrete purchase events where only the latest emission matters if the collector is slow.
53-74: LGTM on PurchasesUpdatedListener implementation.Good handling of purchase states with explicit branches for
PURCHASED,PENDING, and other states. The TODO for pending state handling is noted.
117-137: LGTM on userId integration in purchase flow.Using
setObfuscatedAccountId(userId)properly associates the purchase with the user for tracking and unprocessed purchase recovery.
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
66-68: Address TODO: Implement PENDING purchase state handling.The
PENDINGstate occurs when payment requires additional authentication (e.g., cash payments in some regions). Users should be notified that their payment is being processed.Would you like me to generate an implementation for pending state handling, or should I open an issue to track this work?
142-154: Consider distinguishing between error and empty results.
queryPurchases()returns an empty list both when there are no purchases and when the query fails. This makes it impossible for callers to distinguish between these cases, potentially masking billing connection issues.🔎 Option 1: Return Result wrapper
- private suspend fun queryPurchases(): List<Purchase> = suspendCancellableCoroutine { continuation -> + private suspend fun queryPurchases(): Result<List<Purchase>> = suspendCancellableCoroutine { continuation -> val params = QueryPurchasesParams.newBuilder() .setProductType(BillingClient.ProductType.INAPP) .build() billingClient.queryPurchasesAsync(params) { result, purchases -> if (result.responseCode == BillingClient.BillingResponseCode.OK) { - continuation.resume(purchases) + continuation.resume(Result.success(purchases)) } else { - continuation.resume(emptyList()) + continuation.resume(Result.failure(Exception("Query failed: ${result.debugMessage}"))) } } }Then update
checkUnprocessedPurchases()to handle the Result.🔎 Option 2: Log errors before returning empty list
billingClient.queryPurchasesAsync(params) { result, purchases -> if (result.responseCode == BillingClient.BillingResponseCode.OK) { continuation.resume(purchases) } else { + Log.e("Billing", "Query purchases failed: ${result.debugMessage}") continuation.resume(emptyList()) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(7 hunks)
⏰ 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: build
🔇 Additional comments (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (1)
117-117: LGTM: Good practice to set obfuscated account ID.Setting the obfuscated account ID helps Google Play associate purchases with user accounts for fraud detection and better purchase tracking.
Also applies to: 129-129
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
Outdated
Show resolved
Hide resolved
| private val purchasesUpdatedListener = PurchasesUpdatedListener { result, purchases -> | ||
| Log.d("purchasesUpdateListener", "$result $purchases") | ||
|
|
||
| if (result.responseCode == BillingClient.BillingResponseCode.OK && !purchases.isNullOrEmpty()) { | ||
| purchases.forEach { purchase -> | ||
| val token = purchase.purchaseToken | ||
| val uuid = purchase.products.firstOrNull() ?: return@forEach | ||
| _purchaseProduct.value = PurchaseProduct(uuid = uuid, credential = token) | ||
| if (result.responseCode != BillingClient.BillingResponseCode.OK || purchases.isNullOrEmpty()) { | ||
| return@PurchasesUpdatedListener | ||
| } | ||
|
|
||
| purchases.forEach { purchase -> | ||
| when (purchase.purchaseState) { | ||
| Purchase.PurchaseState.PURCHASED -> { | ||
| val productId = purchase.products.firstOrNull() ?: return@forEach | ||
| val token = purchase.purchaseToken | ||
| _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) | ||
| } | ||
| Purchase.PurchaseState.PENDING -> { | ||
| // TODO: 결제 대기 처리 | ||
| } | ||
| else -> { | ||
| // 기타 상태 처리 | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Purchases must be acknowledged after processing.
When a purchase reaches PURCHASED state, it must be acknowledged via billingClient.acknowledgePurchase() or consumed via billingClient.consumePurchase() within 3 days, or Google Play will automatically refund it. Currently, purchases are emitted but never acknowledged.
🔎 Recommended acknowledgment flow
After emitting the purchase, you should acknowledge it (or consume it if it's a consumable product):
Purchase.PurchaseState.PURCHASED -> {
val productId = purchase.products.firstOrNull() ?: return@forEach
val token = purchase.purchaseToken
_purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token))
// Acknowledge after successful emission
if (!purchase.isAcknowledged) {
val ackParams = AcknowledgePurchaseParams.newBuilder()
.setPurchaseToken(token)
.build()
billingClient.acknowledgePurchase(ackParams) { result ->
if (result.responseCode != BillingClient.BillingResponseCode.OK) {
Log.e("Billing", "Acknowledgment failed: ${result.debugMessage}")
}
}
}
}Note: Acknowledgment typically happens after backend verification succeeds. Consider moving this to post-verification in your architecture.
🤖 Prompt for AI Agents
In core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
around lines 53-74, purchased items are emitted but never acknowledged or
consumed which will cause automatic refunds; update the PURCHASED handling to
acknowledge (or consume for consumables) the purchase after successful
processing: check purchase.isAcknowledged, build AcknowledgePurchaseParams with
purchase.purchaseToken and call billingClient.acknowledgePurchase(...) and
handle the callback error logging; if the product is consumable use
billingClient.consumeAsync(...) similarly; ideally perform acknowledgment after
any backend verification succeeds — move or trigger the ack from the
verification completion path if applicable.
| suspendRunCatching { | ||
| billingHelper.verifyPurchaseProduct(product) | ||
| }.onSuccess { | ||
| initPuzzleCount() |
There was a problem hiding this comment.
init을 하면 어떻게 되나요?
구매 성공시 재호출인 것 같은데 fetch라는 네이밍이 더 어올릴지도??
There was a problem hiding this comment.
현재 유저의 퍼즐 개수를 가져오는 함수입니다.
여기가 아니더라도 매칭 화면에 들어올 때마다 쓰는 거라서 init 보다는 fetch가 좋겠네요! 수정하겠습니다.
| suspendRunCatching { | ||
| billingHelper.verifyPurchaseProduct(product) | ||
| }.onSuccess { verifyPurchaseProduct -> | ||
| getUserPuzzle() | ||
| setState { copy(purchasedPuzzleCount = verifyPurchaseProduct.rewardPuzzleCount) } | ||
| }.onFailure { | ||
| errorHelper.sendError(it) | ||
| } |
There was a problem hiding this comment.
Collect 내부에서 runCatching을 하는 이유가 있나요?
Flow.catch 알아보시면 좋을 것 같아요!
| getUserPuzzle() | ||
| setState { copy(purchasedPuzzleCount = verifyPurchaseProduct.rewardPuzzleCount) } |
There was a problem hiding this comment.
getUserPuzzle() 을 하는 이유는 뭔가요??
Ui재랜더링이라면 fetch가 더 적절할 것 같아요.
| private val _purchaseProduct = MutableSharedFlow<PurchaseProduct>( | ||
| replay = 0, | ||
| extraBufferCapacity = 1, | ||
| onBufferOverflow = BufferOverflow.DROP_OLDEST | ||
| ) |
There was a problem hiding this comment.
질문 :
스토어 화면에서 구매 진행된 뒤, collect 후 바로 다른 화면으로 전환되어 코루틴 Job이 취소되면 어떻게 되나요?
There was a problem hiding this comment.
collect 했더라도 검증 API를 호출하기 전에 화면이 전환되어 Job이 취소되면, 해당 구매 건은 Consume되지 않은 미처리 상태로 남게 됩니다. => 검증 API를 쏘면 서버에서 확인 후 Consume 처리합니다.
이번 pr에서 매칭화면에 진입하여 유저 프로필을 받아올 수 있으면 미처리 상태의 재화를 조회하여 있다면 검증 api를 쏘는 로직을 추가했습니다.
BillingHelper의 queryPurchases(), checkUnprocessedPurchases() 입니다.
| if (result.responseCode != BillingClient.BillingResponseCode.OK || purchases.isNullOrEmpty()) { | ||
| return@PurchasesUpdatedListener | ||
| } |
There was a problem hiding this comment.
이 경우는 구매 실패인 것으로 보이는데, 구매 실패했을 때 사용자에게 UI적으로 무언갈 보여줄 필요가 없나요??
There was a problem hiding this comment.
UI 상으로는 구매를 실패했거나 구매를 취소했을 때의 처리는 없습니다.
#183 pr에서 스피너 관련해서 추가 수정만 할 거 같습니다.
| else -> { | ||
| // 기타 상태 처리 | ||
| } |
There was a problem hiding this comment.
#183 pr에서 스피너를 넣으면서 사용할 거 같아서 분기처리를 했는데 사용하지 않을 거 같습니다.
public @interface PurchaseState {
int UNSPECIFIED_STATE = 0;
int PURCHASED = 1;
int PENDING = 2;
}
|
|
||
| val billingFlowParams = BillingFlowParams.newBuilder() | ||
| .setProductDetailsParamsList(productDetailsParamsList) | ||
| .setObfuscatedAccountId(userId) |
There was a problem hiding this comment.
결제 영수증에 인자 값(구매자 ID)를 포함시키는 메서드 입니다.
설정한 이유는 결제한 사람과 재화 받는 사람이 달라지는 사고를 막으려고 넣었습니다.
=> 아직 서버에서는 해당 로직을 하지 않은 것으로 알고 있습니다.
case:
- 계정 A로 결제 후 서버 검증 전 앱 종료
- 계정 B로 다시 로그인
- 앱 진입 시 미처리 재화 조회 로직 작동
- 서버에 검증 요청 시, 계정 B로 재화가 지급
공식 문서: https://developer.android.com/reference/com/android/billingclient/api/BillingFlowParams.Builder
| } else { | ||
| continuation.resume(emptyList()) | ||
| } |
There was a problem hiding this comment.
이 경우는 emptyList가 아니라 resumeWithException 으로 처리해서 스낵바 같은 UI를 뛰워줘야 할 것 같아요.
데이터를 불러오는 데 성공했는데 빈리스트와, 에러는 다르다고 생각해요.
There was a problem hiding this comment.
앗 그러네요, 미처리 상품이 없을 때라고 생각한 거 같네요 수정하겠습니다!
=> 1a15f87
| val productId = purchase.products.firstOrNull() ?: return@forEach | ||
| val token = purchase.purchaseToken | ||
|
|
||
| _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) |
There was a problem hiding this comment.
이번 pr에서 매칭화면에 진입하여 유저 프로필을 받아올 수 있으면 미처리 상태의 재화를 조회하여 있다면 검증 api를 쏘는 로직을 추가했습니다.
BillingHelper의 queryPurchases(), checkUnprocessedPurchases() 입니다.
<추가>
빌링 모듈에서 결제 검증까지 한 결과(Channel)을 내려주도록 수정했습니다.
=> b562926
|
|
||
| interface BillingHelper { | ||
| val purchaseProduct: StateFlow<PurchaseProduct?> | ||
| val purchaseProduct: SharedFlow<PurchaseProduct> |
There was a problem hiding this comment.
SharedFlow인 이유가 궁금해요~
또한, 싱글톤 객체인데 발행될 때 collect 되지 않으면 유실될 것 같은데,
Channel 같은 것으로 버퍼를 주면 어떨까 싶어요.
이유가 있을까요?!
There was a problem hiding this comment.
There was a problem hiding this comment.
이번엔 괜찮은데 다음 pr이랑 뭔가 겹치는 부분이 많은 것 같아요.
Cherrypick이랑 rebase 활용하시면 리뷰어가 더 편할 것 같아욥~~!
근형님 pr에서도 이전에 민수님 pr내용이랑 동일한 변경사항을 봤었던 것 같아서 근형님한테도 공유주시면 좋을 것 같네요.
이 부분은 다음 pr에서 이어나갈게요~
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
46-51: Buffer capacity of 1 risks losing purchase events.With
extraBufferCapacity = 1andDROP_OLDEST, only one purchase can be buffered. If multiple purchases occur rapidly before collection (e.g., user makes multiple purchases or checkUnprocessedPurchases finds multiple unprocessed items), earlier purchases will be dropped, leading to lost revenue and angry users.🔎 Recommended fix
private val _purchaseProduct = MutableSharedFlow<PurchaseProduct>( replay = 0, - extraBufferCapacity = 1, - onBufferOverflow = BufferOverflow.DROP_OLDEST + extraBufferCapacity = 64, + onBufferOverflow = BufferOverflow.SUSPEND )This ensures purchases are queued rather than dropped.
53-74: Critical: Purchases must be acknowledged to prevent refunds and data loss.This implementation has several critical issues:
Missing acknowledgment: Purchases are never acknowledged via
acknowledgePurchase()or consumed viaconsumeAsync(). Google Play will automatically refund un-acknowledged purchases after 3 days. Acknowledgment should occur after successful backend verification (inverifyPurchaseProductsuccess path).Silent emit failures:
tryEmit()at line 64 returnsfalseif the buffer is full, but the return value is not checked. Failed emits mean lost purchases with no error or retry mechanism.Duplicate emissions: Without filtering by
isAcknowledged, the same purchase will be emitted repeatedly on every collection until acknowledged, causing duplicate verification requests and potential duplicate puzzle grants.🔎 Recommended comprehensive fix
purchases.forEach { purchase -> + if (purchase.isAcknowledged) return@forEach + when (purchase.purchaseState) { Purchase.PurchaseState.PURCHASED -> { val productId = purchase.products.firstOrNull() ?: return@forEach val token = purchase.purchaseToken - _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) + val emitted = _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) + if (!emitted) { + Log.e("Billing", "Failed to emit purchase: $productId") + } } Purchase.PurchaseState.PENDING -> { - // TODO: 결제 대기 처리 + // Pending purchases will be delivered when completed + Log.d("Billing", "Purchase pending: ${purchase.products}") } else -> { - // 기타 상태 처리 + Log.w("Billing", "Unexpected purchase state: ${purchase.purchaseState}") } } }Then, add acknowledgment after successful verification in the verification success handler (likely in the caller or after
paymentRepository.verifyPurchaseProductsucceeds):// After verification succeeds if (!purchase.isAcknowledged) { val ackParams = AcknowledgePurchaseParams.newBuilder() .setPurchaseToken(purchaseToken) .build() billingClient.acknowledgePurchase(ackParams) { result -> if (result.responseCode != BillingClient.BillingResponseCode.OK) { Log.e("Billing", "Acknowledgment failed: ${result.debugMessage}") } } }
156-168: Same critical issues as purchasesUpdatedListener apply here.This function has the same problems identified in lines 53-74:
- No
isAcknowledgedfilter: Will re-emit the same purchases on every call until they're acknowledged, causing duplicates.- Silent emit failures:
tryEmit()at line 166 can fail silently if the buffer is full.- No acknowledgment: These purchases must eventually be acknowledged after verification.
🔎 Recommended fix
override suspend fun checkUnprocessedPurchases() { connectGooglePlayIfNeeded() val purchases = queryPurchases() - purchases.filter { it.purchaseState == Purchase.PurchaseState.PURCHASED } + purchases.filter { + it.purchaseState == Purchase.PurchaseState.PURCHASED && !it.isAcknowledged + } .forEach { purchase -> val productId = purchase.products.firstOrNull() ?: return@forEach val token = purchase.purchaseToken - _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) + val emitted = _purchaseProduct.tryEmit(PurchaseProduct(uuid = productId, credential = token)) + if (!emitted) { + Log.e("Billing", "Failed to emit unprocessed purchase: $productId") + } } }
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
265-271: Consider adding error handling for purchase verification.The function handles successful verification but doesn't handle failure cases. Since this processes user-initiated purchases, consider adding
.onFailureto provide feedback if verification fails.🔎 Suggested enhancement
internal fun handlePurchaseEvent(product: PurchaseProduct) = viewModelScope.launch { suspendRunCatching { billingHelper.verifyPurchaseProduct(product) }.onSuccess { initPuzzleCount() + }.onFailure { + errorHelper.sendError(it) } }Optional: Consider renaming
initPuzzleCounttofetchPuzzleCount.Based on a past review comment, since this function is used both for initialization and refresh (e.g., after purchases),
fetch*might be a clearer name thaninit*.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(7 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(5 hunks)
⏰ 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: build
🔇 Additional comments (4)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
50-50: LGTM!Clean dependency injection of BillingHelper with appropriate internal visibility for testing.
111-118: Background purchase check looks good.The unprocessed purchase check runs independently after successful user info retrieval, with appropriate error handling for a background operation.
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
117-117: LGTM! Good security enhancement.Adding userId to the purchase flow and setting it via
setObfuscatedAccountIdhelps prevent account mismatch scenarios where one user's purchase is credited to another user.Also applies to: 129-129
142-154: Proper suspend implementation for querying purchases.The use of
suspendCancellableCoroutinecorrectly wraps the async callback, ensuring the caller suspends until results are available.One minor concern: verify that
purchasesin the callback cannot be null whenresponseCode == OK. If it can, you'd need null handling:billingClient.queryPurchasesAsync(params) { result, purchases -> if (result.responseCode == BillingClient.BillingResponseCode.OK) { continuation.resume(purchases ?: emptyList()) } else { continuation.resumeWithException(RuntimeException("구매 조회 실패: ${result.responseCode}")) } }Based on learnings from past comments, the developer indicated this handles unprocessed purchases for cases where the app is closed before verification.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (1)
57-70: Fix scope lifecycle and add purchase acknowledgment.Three issues require attention:
Critical: Missing purchase acknowledgment. Google Play requires all purchases to be acknowledged within 3 days via
billingClient.acknowledgePurchase()(for non-consumables) orbillingClient.consumeAsync()(for consumables), or they will be automatically refunded. Currently, purchases are verified but never acknowledged.Major: Potential memory leak. The
CoroutineScopewithSupervisorJobis never cancelled. Consider implementing a cleanup method or tying the scope to a lifecycle.Major: Duplicate processing risk. Both
purchasesUpdatedListenerandcheckUnprocessedPurchases()will process the same unacknowledged purchases repeatedly. Filter purchases by!purchase.isAcknowledgedbefore processing.🔎 Proposed fixes
1. Add acknowledgment after successful verification:
private suspend fun handlePurchaseVerification(purchase: Purchase) { val productIds = purchase.products if (productIds.isEmpty()) return try { val productId = purchase.products.firstOrNull() ?: return val token = purchase.purchaseToken val verifyResult = paymentRepository.verifyPurchaseProduct( PurchaseProduct( uuid = productId, credential = token ) ) _purchaseResult.send(PurchaseResult.Success(verifyResult.rewardPuzzleCount)) // Acknowledge after successful verification if (!purchase.isAcknowledged) { val consumeParams = ConsumeParams.newBuilder() .setPurchaseToken(token) .build() withContext(Dispatchers.IO) { billingClient.consumeAsync(consumeParams) { result, _ -> if (result.responseCode != BillingClient.BillingResponseCode.OK) { Log.e("Billing", "Consume failed: ${result.debugMessage}") } } } } } catch (e: Exception) { _purchaseResult.send(PurchaseResult.Failure(e)) } }2. Filter by acknowledgment status:
private val purchasesUpdatedListener = PurchasesUpdatedListener { result, purchases -> if (result.responseCode == BillingClient.BillingResponseCode.OK && purchases != null) { purchases.forEach { purchase -> - if (purchase.purchaseState == Purchase.PurchaseState.PURCHASED) { + if (purchase.purchaseState == Purchase.PurchaseState.PURCHASED && !purchase.isAcknowledged) { scope.launch { handlePurchaseVerification(purchase) } } } } else if (result.responseCode != BillingClient.BillingResponseCode.USER_CANCELED) { _purchaseResult.trySend(PurchaseResult.Failure(Exception("Billing Error: ${result.debugMessage}"))) } }3. Add cleanup method:
Add a public cleanup method and cancel the scope:
fun cleanup() { scope.cancel() billingClient.endConnection() }
🧹 Nitpick comments (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (1)
113-133: Consider emitting failure when billing flow launch fails.The
userIdparameter andsetObfuscatedAccountIdcorrectly prevent purchase-user mismatch scenarios. However, whenlaunchBillingFlowfails, only a log is generated. Consider emittingPurchaseResult.Failureso the UI can notify the user.🔎 Suggested improvement
val billingResult = billingClient.launchBillingFlow(activity, billingFlowParams) if (billingResult.responseCode != BillingClient.BillingResponseCode.OK) { Log.e("Billing", "Purchase failed: ${billingResult.debugMessage}") + _purchaseResult.trySend( + PurchaseResult.Failure(Exception("Failed to launch billing flow: ${billingResult.debugMessage}")) + ) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(6 hunks)
⏰ 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: build
🔇 Additional comments (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
53-55: LGTM: Channel-based result emission.The migration from
MutableSharedFlowtoChannel<PurchaseResult>withChannel.BUFFERED(default capacity 64) properly addresses the previous buffer overflow concerns and provides clearer success/failure semantics.
159-171: LGTM: Proper suspend wrapper for async callback.The
suspendCancellableCoroutinecorrectly wraps thequeryPurchasesAsynccallback, ensuring the caller suspends until results are available. Error handling is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
277-287: Error suppression approach is reasonable.Silently ignoring
VERIFICATION_FAILEDandRECEIPT_ALREADY_USEDwhile forwarding other errors aligns with the background processing nature of this feature. This is a good refinement from completely removing failure handling.
🧹 Nitpick comments (1)
core/domain/src/main/java/com/puzzle/domain/model/error/HttpResponseException.kt (1)
153-153: Extra blank line.There's an extra blank line here before the
// Unknowncomment. Consider removing it for consistency with the rest of the file formatting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/domain/src/main/java/com/puzzle/domain/model/error/HttpResponseException.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(5 hunks)
🔇 Additional comments (5)
core/domain/src/main/java/com/puzzle/domain/model/error/HttpResponseException.kt (1)
146-151: New InAppPayment error codes added correctly.The additions follow the existing pattern and are well-formatted. However, three of the four error codes (
RECEIPT_ALREADY_USED,DUPLICATE_PAYMENT,PAYMENT_ALREADY_COMPLETED) appear semantically related and may overlap in usage scenarios. Consider documenting the specific scenarios where each should be used to prevent confusion and ensure consistent error handling throughout the billing flow.Can you clarify the distinction between these three error codes? For example:
- When should
RECEIPT_ALREADY_USEDbe thrown vs.DUPLICATE_PAYMENT?- What differentiates
DUPLICATE_PAYMENTfromPAYMENT_ALREADY_COMPLETED?feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
46-55: LGTM!The
billingHelperdependency is properly injected via constructor following the same pattern as other dependencies.
79-93: LGTM!The method renames from
init*tofetch*improve clarity as suggested in the past review. The naming now accurately reflects that these methods fetch data rather than initialize state.
112-118: LGTM!The background check for unprocessed purchases is appropriately placed within the user info success flow. The fire-and-forget pattern with error forwarding to
errorHelperis reasonable for this background task.
266-290: No action required —collectPurchaseResult()is already properly invoked from the Matching screen Composable.The function is correctly called from
MatchingScreen.ktwithin aLaunchedEffectwithrepeatOnLifecycle(Lifecycle.State.RESUMED), which provides proper lifecycle-aware coroutine scoping and automatic cancellation when the UI is not visible.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
186-198: Fix suspend semantics: removescope.launchand callhandlePurchaseVerificationdirectly.This
suspendfunction launches coroutines viascope.launchand returns immediately, violating the caller's expectation that a suspend function completes its work before returning. The launches at line 194 are fire-and-forget, meaningcheckUnprocessedPurchasescompletes before verification finishes.🔎 Recommended fix to await verification completion
Option 1 (Sequential processing):
override suspend fun checkUnprocessedPurchases() { connectGooglePlayIfNeeded() val purchases = queryPurchases() purchases.filter { it.purchaseState == Purchase.PurchaseState.PURCHASED && !it.isAcknowledged }.forEach { purchase -> - scope.launch { - handlePurchaseVerification(purchase) - } + handlePurchaseVerification(purchase) } }Option 2 (Parallel processing with proper suspension):
+ import kotlinx.coroutines.async + import kotlinx.coroutines.awaitAll + import kotlinx.coroutines.coroutineScope override suspend fun checkUnprocessedPurchases() { connectGooglePlayIfNeeded() val purchases = queryPurchases() - purchases.filter { - it.purchaseState == Purchase.PurchaseState.PURCHASED && !it.isAcknowledged - }.forEach { purchase -> - scope.launch { - handlePurchaseVerification(purchase) - } - } + coroutineScope { + purchases.filter { + it.purchaseState == Purchase.PurchaseState.PURCHASED && !it.isAcknowledged + }.map { purchase -> + async { handlePurchaseVerification(purchase) } + }.awaitAll() + } }Option 1 is simpler and processes purchases sequentially. Option 2 processes in parallel but still suspends until all complete.
151-169: Critical: Purchases must be acknowledged after server verification succeeds.After emitting
PurchaseResult.Success, the purchase must be acknowledged viabillingClient.acknowledgePurchase()(or consumed if it's a consumable). Google Play automatically refunds unacknowledged purchases after 3 days. Currently, no acknowledgment occurs anywhere in this file.🔎 Recommended fix to acknowledge after verification
try { val productId = purchase.products.firstOrNull() ?: return val token = purchase.purchaseToken val verifyResult = paymentRepository.verifyPurchaseProduct( PurchaseProduct( uuid = productId, credential = token ) ) _purchaseResult.send(PurchaseResult.Success(verifyResult.rewardPuzzleCount)) + + // Acknowledge purchase after successful verification + if (!purchase.isAcknowledged) { + val ackParams = AcknowledgePurchaseParams.newBuilder() + .setPurchaseToken(token) + .build() + withContext(Dispatchers.IO) { + billingClient.acknowledgePurchase(ackParams) { result -> + if (result.responseCode != BillingClient.BillingResponseCode.OK) { + Log.e("Billing", "Acknowledgment failed: ${result.debugMessage}") + } + } + } + } } catch (e: Exception) { _purchaseResult.send(PurchaseResult.Failure(e)) }Note: Add
import com.android.billingclient.api.AcknowledgePurchaseParamsto imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(6 hunks)
⏰ 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: build
🔇 Additional comments (3)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
53-55: LGTM: Channel-based purchase result signaling.The use of
Channel.BUFFERED(default capacity 64) withreceiveAsFlow()provides robust buffering for purchase results and addresses the previous buffer capacity concern.
126-146: LGTM: User ID propagation prevents account-switching fraud.Setting
obfuscatedAccountIdensures the purchase is tied to the correct user, preventing scenarios where User A purchases but User B (after re-login) receives the rewards.
172-184: LGTM: Proper suspend semantics for queryPurchases.The use of
suspendCancellableCoroutinecorrectly wraps the async callback, ensuring the function suspends until the query completes and handles errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
157-172: Consider consolidating duplicate verification logic.The verification logic in
handlePurchaseVerificationduplicates what's already infetchPurchaseReward(lines 71-83). Both extractproductId, callpaymentRepository.verifyPurchaseProduct, and handle exceptions.🔎 Proposed refactor to reuse fetchPurchaseReward
private suspend fun handlePurchaseVerification(purchase: Purchase) { - val productId = purchase.products.firstOrNull() ?: return try { - val verifyResult = withContext(Dispatchers.IO) { - paymentRepository.verifyPurchaseProduct( - PurchaseProduct( - uuid = productId, - credential = purchase.purchaseToken - ) - ) - } - _purchaseResult.send(PurchaseResult.Success(verifyResult.rewardPuzzleCount)) + val reward = fetchPurchaseReward(purchase) + _purchaseResult.send(PurchaseResult.Success(reward)) } catch (e: Exception) { _purchaseResult.send(PurchaseResult.Failure(e)) } }This reduces duplication and ensures consistent verification behavior across both purchase flows.
188-208: Consider emitting an event when all verifications fail.If all unprocessed purchases fail verification (each returns 0 via
getOrDefault(0)),totalRewardwill be 0 and noPurchaseResultevent will be emitted. This could silently hide verification failures from the UI.💡 Alternative: emit Failure when all verifications fail
val totalReward = supervisorScope { - purchases.map { purchase -> + val results = purchases.map { purchase -> async { runCatching { fetchPurchaseReward(purchase) }.getOrDefault(0) } - }.awaitAll().sum() + }.awaitAll() + + if (results.all { it == 0 }) { + throw Exception("All purchase verifications failed") + } + results.sum() } if (totalReward > 0) { _purchaseResult.send(PurchaseResult.Success(totalReward)) }This ensures the user is notified when batch verification completely fails, though it may be intentional to silently skip failed purchases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(6 hunks)core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt(1 hunks)core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-41)
🪛 detekt (1.23.8)
core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt
[warning] 29-29: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 31-31: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
⏰ 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: build
🔇 Additional comments (8)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (3)
51-51: LGTM!The BillingHelper injection enables unprocessed purchase checking and purchase result observation. The integration looks clean.
112-118: LGTM!The concurrent launch of
checkUnprocessedPurchases()is appropriate for background verification. Error handling viaerrorHelperensures failures are surfaced without blocking the main user info flow.
266-280: Code properly handles infinite Flow collection with lifecycle awareness.The function is correctly called from
LaunchedEffectin MatchingScreen.kt wrapped withlifecycleOwner.repeatOnLifecycle(Lifecycle.State.RESUMED), which prevents coroutine leaks and ensures proper lifecycle management.core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt (1)
14-32: LGTM!The test fake correctly implements the
BillingHelperinterface with deterministic behavior suitable for testing. TheChannel.BUFFEREDcapacity is appropriate for test scenarios.Note: The empty function blocks flagged by static analysis are intentional for a test double and can be safely ignored.
core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt (1)
10-16: LGTM!The interface changes provide a clean API surface:
Flow<PurchaseResult>is more appropriate thanStateFlowfor event signalinguserIdparameter inpurchaseProducthelps prevent wrong-user reward scenarioscheckUnprocessedPurchases()enables recovery from interrupted purchase flowscore/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
49-51: LGTM!The
Channel.BUFFEREDapproach prevents event loss compared to the previousSharedFlowwith limited capacity. The Flow exposure viareceiveAsFlow()provides clean one-way event signaling.
55-69: LGTM!The
PurchasesUpdatedListenercorrectly handles purchase callbacks by launching coroutines for each purchase. Usingscope.launchhere is appropriate for fire-and-forget processing, and non-cancellation errors are properly emitted asPurchaseResult.Failure.
135-155: LGTM!The addition of
userIdviasetObfuscatedAccountIdhelps prevent wrong-user reward scenarios by embedding the user context in the purchase receipt.Based on learnings, this addresses the case where a purchase is made on one account but rewards could be granted to a different account after app restart.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt(6 hunks)
⏰ 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: build
🔇 Additional comments (4)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (4)
49-51: LGTM! Channel-based signaling with adequate buffering.The switch to
Channel.BUFFERED(64 capacity) with suspend-on-overflow properly addresses the previous buffer capacity concern. Purchase events will now queue rather than drop.
53-69: LGTM! Proper coroutine setup and purchase listener.The
SupervisorJobwithDispatchers.Main.immediatecorrectly handles billing callbacks. The removal of nestedscope.launchinside the listener (from past reviews) ensures proper error propagation. Usingsend()instead oftrySend()guarantees delivery.
157-169: LGTM! Proper suspend wrapper for async callback.The use of
suspendCancellableCoroutinecorrectly wraps the callback-basedqueryPurchasesAsync, ensuring the function suspends until completion and properly propagates errors.
135-147: LGTM! User ID prevents account-switching exploits.The addition of
userIdparameter and its use withsetObfuscatedAccountId(line 147) prevents the scenario where User A makes a purchase, User B logs in before verification, and User B receives the rewards. Good security practice.Based on past review comments, the backend should validate this obfuscated account ID during verification.
| private suspend fun handleSinglePurchase(purchase: Purchase) { | ||
| try { | ||
| val reward = fetchPurchaseReward(purchase) | ||
| _purchaseResult.send(PurchaseResult.Success(reward)) | ||
| } catch (e: Exception) { | ||
| _purchaseResult.send(PurchaseResult.Failure(e)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Purchases are never acknowledged.
After successful verification (line 87), the purchase must be acknowledged via billingClient.acknowledgePurchase() or consumed via billingClient.consumePurchase(). Without acknowledgment within 3 days, Google Play automatically refunds the purchase, resulting in revenue loss and users receiving both rewards and refunds.
The past review flagged this as critical and was marked "addressed," but no acknowledgment logic is visible in the current code.
🔎 Recommended fix to acknowledge after verification
private suspend fun handleSinglePurchase(purchase: Purchase) {
try {
val reward = fetchPurchaseReward(purchase)
+
+ // Acknowledge after successful verification
+ if (!purchase.isAcknowledged) {
+ val ackParams = AcknowledgePurchaseParams.newBuilder()
+ .setPurchaseToken(purchase.purchaseToken)
+ .build()
+ billingClient.acknowledgePurchase(ackParams) { result ->
+ if (result.responseCode != BillingClient.BillingResponseCode.OK) {
+ Log.e("Billing", "Acknowledgment failed: ${result.debugMessage}")
+ }
+ }
+ }
+
_purchaseResult.send(PurchaseResult.Success(reward))
} catch (e: Exception) {
_purchaseResult.send(PurchaseResult.Failure(e))
}
}Add import:
import com.android.billingclient.api.AcknowledgePurchaseParamsApply the same acknowledgment logic in checkUnprocessedPurchases after successful verification (around line 183).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
around lines 85 to 92, the code verifies purchases but never acknowledges or
consumes them; add logic to call billingClient.acknowledgePurchase() (or
billingClient.consumePurchase() for consumables) immediately after successful
verification: build AcknowledgePurchaseParams with the purchase token, invoke
billingClient.acknowledgePurchase with a listener or suspend wrapper, check the
response code and handle errors, and only then send PurchaseResult.Success; also
apply the same acknowledgment flow in checkUnprocessedPurchases (around line
183) so previously verified-but-unacknowledged purchases are acknowledged too.
|
일단 pr이 무거워서 어프로브 눌러놓겠습니닷 |
tgyuuAn
added
머지 해도될듯염🌟
* [PC-1487] store 진입 시 로딩전에 바로 스켈레톤 나오도록 수정 * [PC-1487] store 화면에서 결제 진행 시 spinner show/hide 로직 * [PC-1487] store 화면 리팩토링 * [PC-1487] 로그 제거 * [PC-1487] 스피너 표출 시 뒤로가기 이벤트 무시 로직 * [PC-1487] 결제 진행 상태 관리 로직 개선 * [PC-1487] 컨플릭트 수정 * [PC-1487] 컨플릭트 수정 * [PC-1487] 결제 로딩 상태(isPurchaseProcessing)를 제거하고 뷰모델 내부 로직으로 통합 * [PC-1487] purchaseProduct를 suspend로 변경 * [PC-1487] 미사용 import 정리 * [PC-1487] 스토어 로딩 처리 로직 위치 수정 * [PC-1487] 성공만 처리하도록 변경 * [PC-1487] when 분기처리 -> filterIsInstance * [PC-1487] else -> PurchaseResult.Cancelled * [PC-1487] isShowSpinner -> isPurchase * [PC-1487] Loading을 compat, expanded 내부에서 분기 처리 * [PC-1487] 결제 상태 별 에러 메시지 추가 및 수정 * [PC-1487] purchasesUpdatedListener에서 각 상품 결제는 순차 처리 하도록 수정 * [PC-1487] DEVELOPER_ERROR 에러 메시지 수정
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)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (1)
227-244: Fix return type mismatch in early return.Line 229 calls
continuation.resume(Result.success(Unit)), but the function signature issuspend fun connectGooglePlayIfNeeded()which returnsUnit, notResult<Unit>. This will cause a compilation error or type mismatch.🔎 Proposed fix
private suspend fun connectGooglePlayIfNeeded() = suspendCancellableCoroutine { continuation -> if (billingClient.isReady) { - continuation.resume(Result.success(Unit)) + continuation.resume(Unit) return@suspendCancellableCoroutine }
♻️ Duplicate comments (2)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
205-225: Major: Silent failure suppression loses error visibility.Using
runCatching { ... }.getOrDefault(0)at line 217 silently suppresses individual purchase verification failures. If a user has 3 unprocessed purchases and 2 fail verification, they receive only 1/3 of the expected reward with no indication of the failures.Consider:
- Logging failures at minimum
- Optionally emitting individual results per purchase
- Or including failure count/details in the aggregated result
Additionally, this method still lacks purchase acknowledgment logic (see separate critical comment on lines 116-123).
🔎 Recommended improvements
override suspend fun checkUnprocessedPurchases() { connectGooglePlayIfNeeded() val purchases = queryPurchases().filter { it.purchaseState == Purchase.PurchaseState.PURCHASED && !it.isAcknowledged } if (purchases.isEmpty()) return - val totalReward = supervisorScope { + val results = supervisorScope { purchases.map { purchase -> async { - runCatching { fetchPurchaseReward(purchase) }.getOrDefault(0) + runCatching { + fetchPurchaseReward(purchase) + }.onFailure { + Log.e("Billing", "Purchase verification failed: ${purchase.orderId}", it) + } } - }.awaitAll().sum() + }.awaitAll() } + + val totalReward = results.mapNotNull { it.getOrNull() }.sum() + val failedCount = results.count { it.isFailure } + + if (failedCount > 0) { + Log.w("Billing", "$failedCount purchases failed verification") + } if (totalReward > 0) { _purchaseResult.send(PurchaseResult.Success(totalReward)) } }Add import:
import android.util.Log
116-123: Critical: Purchases must be acknowledged after successful verification.After successful server verification (line 118), the purchase must be acknowledged via
billingClient.acknowledgePurchase()or consumed. Without acknowledgment within 3 days, Google Play automatically refunds the purchase, causing revenue loss and allowing users to receive both rewards and refunds.This issue was flagged as critical in multiple past reviews and marked "addressed," but no acknowledgment logic is present in the current code.
🔎 Recommended fix to acknowledge after verification
+import com.android.billingclient.api.AcknowledgePurchaseParams + private suspend fun handleSinglePurchase(purchase: Purchase) { try { val reward = fetchPurchaseReward(purchase) + + // Acknowledge purchase after successful verification + if (!purchase.isAcknowledged) { + val ackParams = AcknowledgePurchaseParams.newBuilder() + .setPurchaseToken(purchase.purchaseToken) + .build() + + withContext(Dispatchers.IO) { + suspendCancellableCoroutine<Unit> { continuation -> + billingClient.acknowledgePurchase(ackParams) { result -> + if (result.responseCode == BillingClient.BillingResponseCode.OK) { + continuation.resume(Unit) + } else { + continuation.resumeWithException( + Exception("Acknowledgment failed: ${result.debugMessage}") + ) + } + } + } + } + } + _purchaseResult.send(PurchaseResult.Success(reward)) } catch (e: Exception) { _purchaseResult.send(PurchaseResult.Failure(e)) } }Apply the same acknowledgment logic in
checkUnprocessedPurchases(around line 217) afterfetchPurchaseRewardsucceeds.
🧹 Nitpick comments (3)
feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (1)
70-80: Validate userId before initiating purchase.The purchase is initiated with
currentState.userId.toString()(line 77), but there's no explicit check thatuserId != -1. If the user somehow triggers a purchase beforegetUserInfocompletes, the purchase would proceed with"-1"as the userId.Although the loading state should prevent interaction until initialization completes, adding a defensive check would improve robustness:
🔎 Proposed guard
is StoreIntent.OnPurchaseClick -> { + if (currentState.userId == -1) { + errorHelper.sendError(IllegalStateException("User ID not loaded")) + return + } setState { copy(isPurchase = true) } _sideEffects.send( StoreSideEffect.PurchaseProduct( intent.purchaseProduct, currentState.userId.toString() ) ) }core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
52-52: Consider adding lifecycle management for the coroutine scope.The
CoroutineScopeis never cancelled, which can lead to resource leaks if theBillingHelperis released. Although the module is singleton-scoped, consider either:
- Adding a cleanup method (e.g.,
close()orrelease()) that cancels the scope- Using a lifecycle-aware scope if available in your DI framework
🔎 Proposed cleanup approach
+ fun release() { + scope.cancel() + }Then ensure this is called when the billing module is no longer needed.
166-189: Minor style: Simplify early-return pattern.The
runblock at lines 168-171 is unnecessary since you're returning immediately after sending the failure. Consider simplifying for clarity.🔎 Optional simplification
- val offerToken = purchaseProduct.detail.oneTimePurchaseOfferDetails?.offerToken - ?: run { - _purchaseResult.send(PurchaseResult.Failure(IllegalStateException("Product has no offer token"))) - return - } + val offerToken = purchaseProduct.detail.oneTimePurchaseOfferDetails?.offerToken + ?: return _purchaseResult.send(PurchaseResult.Failure(IllegalStateException("Product has no offer token")))Or even clearer:
+ val offerToken = purchaseProduct.detail.oneTimePurchaseOfferDetails?.offerToken + if (offerToken == null) { + _purchaseResult.send(PurchaseResult.Failure(IllegalStateException("Product has no offer token"))) + return + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktcore/billing/src/main/java/com/puzzle/billing/model/PurchaseResult.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktfeature/store/src/main/java/com/puzzle/store/StoreScreen.ktfeature/store/src/main/java/com/puzzle/store/StoreViewModel.ktfeature/store/src/main/java/com/puzzle/store/contract/StoreState.ktfeature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreLoadingScreen.ktfeature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.ktfeature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreLoadingScreen.ktfeature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.kt (4)
feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreLoadingScreen.kt (1)
CompactStoreLoadingScreen(27-61)feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.kt (1)
PromotionProductList(17-39)feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.kt (1)
NormalProductList(16-40)feature/store/src/main/java/com/puzzle/store/ui/components/StoreDescription.kt (1)
StoreDescription(25-80)
feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt (5)
feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreLoadingScreen.kt (1)
ExpandedStoreLoadingScreen(31-80)feature/store/src/main/java/com/puzzle/store/ui/components/StoreTopBar.kt (1)
StoreTopBar(26-49)feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.kt (1)
PromotionProductList(17-39)feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.kt (1)
NormalProductList(16-40)feature/store/src/main/java/com/puzzle/store/ui/components/StoreDescription.kt (1)
StoreDescription(25-80)
feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (4)
feature/store/src/main/java/com/puzzle/store/ui/dialog/PurchaseSuccessDialog.kt (1)
PurchaseSuccessDialog(20-62)feature/store/src/main/java/com/puzzle/store/ui/AdaptiveLayout.kt (1)
AdaptiveLayout(7-20)feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.kt (1)
CompactStoreScreen(19-59)feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt (1)
ExpandedStoreScreen(23-73)
feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (7)
core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
getUserInfo(88-89)core/data/src/main/java/com/puzzle/data/repository/UserRepositoryImpl.kt (1)
getUserInfo(37-57)core/domain/src/main/java/com/puzzle/domain/repository/UserRepository.kt (1)
getUserInfo(16-16)core/network/src/main/java/com/puzzle/network/source/user/UserDataSource.kt (1)
getUserInfo(12-12)core/network/src/main/java/com/puzzle/network/source/user/UserDataSourceImpl.kt (1)
getUserInfo(22-22)core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-41)core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)
⏰ 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: build
🔇 Additional comments (21)
feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (5)
40-40: Good UX: Back navigation blocked during purchase.Preventing navigation during the purchase flow ensures users don't accidentally interrupt the transaction.
46-50: Correctly placed in UI layer due to Activity requirement.The side-effect collection and
purchaseProductcall are properly located in the UI layer because:
- The
billingHelper.purchaseProduct()call requires anActivityparameter- ViewModels should not hold references to Android framework components like Activities
Adding
userIdto the purchase flow aligns with the PR's objective to verify and process purchases with proper user identification.Regarding the past review comment: while moving logic to the ViewModel is generally preferred, this specific case requires the Activity context, so the current implementation is correct.
56-74: Well-structured layering of UI states.The Box-based layout cleanly separates three concerns:
- Main store content (always rendered)
- Purchase progress overlay (
isPurchasestate)- Success dialog (
isShowPurchaseDialogstate)This approach maintains proper z-ordering and makes state-driven visibility clear.
77-101: Clean delegation to adaptive layouts.The extracted
StoreScreencomposable properly delegates to compact/expanded layouts while managing shared concerns like blur effects and intent forwarding.
103-115: Effective interaction blocking during purchase.The
pointerInput(Unit) {}modifier correctly consumes all pointer events, preventing user interaction while the purchase is processing. Combined with theBackHandler, this provides solid UX to prevent purchase flow interruption.Error handling is properly implemented: when purchase fails,
isPurchaseis reset tofalsevia thecollectPurchaseResult()flow, allowing users to retry or navigate away.feature/store/src/main/java/com/puzzle/store/contract/StoreState.kt (1)
11-12: LGTM!The new state fields
isPurchaseanduserIdare well-integrated into the purchase flow. The default values (falseand-1) are appropriate sentinel values.feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreLoadingScreen.kt (1)
3-3: LGTM! Animation timing improvement.The reduced duration (5000ms → 1200ms) and switch to
FastOutSlowInEasingcreate a more responsive and natural loading animation.Also applies to: 71-71
feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.kt (1)
27-58: LGTM! Clean loading state handling.The conditional rendering pattern appropriately delegates to
CompactStoreLoadingScreenduring the loading phase, with back navigation preserved.feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreLoadingScreen.kt (2)
89-89: LGTM! Faster loading animations.The reduced animation durations (1800ms → 1000ms for big card, 2400ms → 1200ms for list) improve perceived performance.
Also applies to: 104-104
111-111: Verify fixed delayStep is intentional.The
delayStepchanged from computed1f/itemCountto a fixed0.15f. While this works well with the currentitemCount = 4, if the item count changes in the future, the stagger timing will not adapt accordingly. Confirm this fixed value is intentional.feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt (1)
31-72: LGTM! Consistent loading pattern.The loading state handling mirrors
CompactStoreScreen, maintaining consistency across screen variants.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
80-86: Good naming improvement: init → fetch.**The renamed functions (
fetchPuzzleCount,fetchFreeMatch,fetchMatchInfo) better convey that they retrieve and update data rather than just initialize.Also applies to: 88-94, 96-96
113-119: LGTM! Non-blocking unprocessed purchase check.The
checkUnprocessedPurchasescall is correctly launched in a separate coroutine with proper error handling. This ensures the main user info flow isn't blocked while background purchase verification occurs.core/billing/src/main/java/com/puzzle/billing/model/PurchaseResult.kt (1)
1-8: LGTM! Well-designed domain model.The
PurchaseResultsealed class provides clear variants for all purchase outcomes. IncludingrewardPuzzleinSuccessenables immediate UI feedback without an additional query.feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (3)
44-56: LGTM! Efficient parallel initialization.The concurrent fetching of user puzzle count, available products, and user info with
joinAlloptimizes startup time. The separatecollectPurchaseResultlaunch ensures purchase results are observed throughout the ViewModel's lifecycle.
141-149: LGTM! User info fetch for purchase flow.The new
getUserInfofunction correctly retrieves and stores theuserIdneeded for the purchase flow.
85-111: Comprehensive purchase result handling.The handling of all
PurchaseResultvariants is well-structured:
- Success: Refreshes puzzle count and shows reward dialog
- Failure: Delegates to error handler
- Pending: Informs user via snackbar about async reward delivery
- Cancelled: Silently ignored (appropriate UX)
The
billingHelper.purchaseResultuses aChannel-basedFlow(Channel.receiveAsFlow()), which correctly prevents replay of stale results. Each purchase result is consumed exactly once, making this pattern appropriate for one-time events.core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (4)
48-50: LGTM: Channel-based signaling properly configured.
Channel.BUFFERED(capacity 64 withSUSPENDoverflow) addresses the previous concern about purchase event loss. The Flow API viareceiveAsFlow()provides clean consumer-side semantics.
54-100: LGTM: Comprehensive purchase event handling.The listener properly handles all purchase states and billing response codes with user-friendly error messages. The use of
scope.launchallows async verification while maintaining proper error propagation through the channel.
191-203: LGTM: Proper suspend wrapper for async billing call.The
suspendCancellableCoroutinewrapper correctly handles the callback-basedqueryPurchasesAsyncAPI, ensuring the caller awaits completion. Error handling is appropriate.
246-274: LGTM: Product conversion logic is clean.The helper properly handles both product types and safely returns
nullfor products without offers.
| internal suspend fun collectPurchaseResult() { | ||
| billingHelper.purchaseResult | ||
| .filterIsInstance<PurchaseResult.Success>() | ||
| .collect { | ||
|
|
||
| eventHelper.sendEvent( | ||
| PieceEvent.ShowSnackBar( | ||
| SnackBarState.Info("퍼즐이 추가로 지급되었습니다.") | ||
| ) | ||
| ) | ||
|
|
||
| fetchPuzzleCount() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for collectPurchaseResult invocation
rg "collectPurchaseResult" --type=kt -A5 -B5Repository: Piece-Puzzly/Piece-Android
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for collectPurchaseResult without specifying file type
rg "collectPurchaseResult" -A5 -B5Repository: Piece-Puzzly/Piece-Android
Length of output: 4536
🏁 Script executed:
#!/bin/bash
# Get more context on StoreViewModel's collectPurchaseResult to see Failure handling
rg "collectPurchaseResult" --type=kt -A20 feature/store/src/main/java/com/puzzle/store/StoreViewModel.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Get more context on StoreViewModel's collectPurchaseResult to see Failure handling
rg "collectPurchaseResult" -A20 feature/store/src/main/java/com/puzzle/store/StoreViewModel.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 1558
🏁 Script executed:
#!/bin/bash
# Find PurchaseResult definition to confirm it has Success, Failure, and Pending cases
rg "sealed.*PurchaseResult|class PurchaseResult|object PurchaseResult" -A10Repository: Piece-Puzzly/Piece-Android
Length of output: 742
🏁 Script executed:
#!/bin/bash
# Verify the exact line numbers and confirm what the function does with the filtered result
sed -n '267,280p' feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 480
Handle all PurchaseResult states in MatchingViewModel.
Currently, collectPurchaseResult() filters only PurchaseResult.Success via filterIsInstance<>(), silently ignoring Failure, Pending, and Cancelled results. Compare with StoreViewModel.collectPurchaseResult(), which explicitly handles each case: Success updates UI, Failure reports the error via errorHelper, and Pending shows an in-progress message. Implement similar comprehensive handling for consistency and to ensure purchase failures are properly reported to the user.
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
around lines 267-280, replace the current
filterIsInstance<PurchaseResult.Success>() collector with a full collector that
handles every PurchaseResult variant using a when expression: on Success send
the existing PieceEvent.ShowSnackBar(SnackBarState.Info("퍼즐이 추가로 지급되었습니다.")) and
call fetchPuzzleCount(); on Failure call errorHelper.reportError(error) (and
optionally show a user-facing SnackBar with the error message); on Pending send
a SnackBar indicating the purchase is in progress; on Cancelled send a SnackBar
indicating the purchase was cancelled. Ensure you stop using filterIsInstance
and directly collect purchaseResult to cover all states.
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.