[PC-1614] Billing 모듈 의존성 리팩토링 #201
Conversation
- domain 모듈 의존성 제거 - analytics 사용하는 부분 제거
📝 WalkthroughWalkthroughMoves payment types and mappers into a billing module, removes analytics from BillingHelper, introduces BillingModule DI, updates network/domain mappings, and adds notification count plumbing (API → data source → repository → matching ViewModel/UI) plus pull-to-refresh and related UI/state changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Screen as MatchingScreen (UI)
participant VM as MatchingViewModel
participant Repo as NotificationRepository
participant DS as NotificationDataSourceImpl
participant API as PieceApi
Screen->>VM: onRouteInit / refreshFlow
VM->>Repo: fetchNotificationsCount()
Repo->>DS: getNotificationCount()
DS->>API: GET /api/notifications/count
API-->>DS: ApiResponse<Int>
DS-->>Repo: unwrapData() -> Int
Repo-->>VM: hasUnreadNotifications (Bool)
VM-->>Screen: update state.hasUnreadNotifications
Screen->>Screen: choose alarm icon (unread vs read)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
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)
100-113: Revenue tracking has been removed - consider alternative observability solution.The removal of
AnalyticsHelper.trackRevenueEvent()eliminates revenue tracking for successful purchases. This impacts business metrics, attribution, and monitoring of payment flows.While removing the dependency aligns with the module independence goal, consider:
- Implementing an event callback interface that the Billing module can expose
- Moving analytics tracking to a higher layer that depends on both Billing and Analytics modules
- Using a publish-subscribe pattern to decouple tracking from the core billing logic
Would you like me to suggest an implementation pattern for decoupled event tracking?
🤖 Fix all issues with AI agents
In
@core/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.kt:
- Around line 35-38: The mapping uses UNKNOWN_INT as a sentinel for missing
reward (VerifyPurchaseProductResponse.toDomain -> rewardPuzzleCount =
rewardPuzzleCount ?: UNKNOWN_INT) but downstream code treats that value
inconsistently; fix this by normalizing the sentinel before returning final
results: update fetchPurchaseReward() to treat UNKNOWN_INT (and any non-positive
value) as "no reward" and either return 0 or null (so callers can handle it
consistently), or alternatively modify handleSinglePurchase() to check the
reward returned and if reward <= 0 (including UNKNOWN_INT) convert it to the
same behavior as batch purchases (e.g., do not return
PurchaseResult.Success(-1); return Success(0) or the appropriate non-reward
path). Ensure you update the code paths that construct PurchaseResult.Success to
never be passed UNKNOWN_INT and reference
VerifyPurchaseProductResponse.toDomain, fetchPurchaseReward(),
handleSinglePurchase(), and PurchaseResult.Success when making the change.
In @core/network/src/main/java/com/puzzle/network/model/ApiResponse.kt:
- Around line 25-27: The unwrapData function performs an unsafe cast (data ?:
Unit as T) that can throw ClassCastException; change its contract to safely
propagate nulls by making the function return a nullable T (fun <T>
ApiResponse<T>.unwrapData(): T? = data) so callers handle absence explicitly, or
alternatively provide an explicit throwing variant (e.g., unwrapDataOrThrow()
that throws a clear IllegalStateException when data is null) and update any
callers of ApiResponse.unwrapData accordingly; locate the function
ApiResponse.unwrapData and replace the unsafe cast with a safe nullable return
or add the explicit throwing helper.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
core/billing/build.gradle.ktscore/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktcore/billing/src/main/java/com/puzzle/billing/data/PaymentRepositoryImpl.ktcore/billing/src/main/java/com/puzzle/billing/di/BillingModule.ktcore/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.ktcore/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.ktcore/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.ktcore/billing/src/main/java/com/puzzle/billing/model/billing/PieceProduct.ktcore/billing/src/main/java/com/puzzle/billing/model/billing/PurchaseResult.ktcore/billing/src/main/java/com/puzzle/billing/model/payment/BasicCashProduct.ktcore/billing/src/main/java/com/puzzle/billing/model/payment/PurchaseProduct.ktcore/billing/src/main/java/com/puzzle/billing/model/payment/VerifyPurchaseProduct.ktcore/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSource.ktcore/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSourceImpl.ktcore/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.ktcore/data/build.gradle.ktscore/data/src/main/java/com/puzzle/data/di/DataModule.ktcore/domain/src/main/java/com/puzzle/domain/model/payment/PurchaseProduct.ktcore/network/src/main/java/com/puzzle/network/di/NetworkModule.ktcore/network/src/main/java/com/puzzle/network/model/ApiResponse.ktcore/network/src/main/java/com/puzzle/network/model/payment/GetAvailableProductResponse.ktcore/network/src/main/java/com/puzzle/network/model/payment/VerifyPurchaseProductRequest.ktcore/network/src/main/java/com/puzzle/network/model/payment/VerifyPurchaseProductResponse.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktfeature/store/src/main/java/com/puzzle/store/StoreViewModel.ktfeature/store/src/main/java/com/puzzle/store/contract/StoreIntent.ktfeature/store/src/main/java/com/puzzle/store/contract/StoreSideEffect.ktfeature/store/src/main/java/com/puzzle/store/contract/StoreState.ktfeature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.ktfeature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.ktfeature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.ktfeature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt
💤 Files with no reviewable changes (6)
- core/data/build.gradle.kts
- core/network/src/main/java/com/puzzle/network/di/NetworkModule.kt
- core/network/src/main/java/com/puzzle/network/model/payment/VerifyPurchaseProductRequest.kt
- core/billing/build.gradle.kts
- core/data/src/main/java/com/puzzle/data/di/DataModule.kt
- core/domain/src/main/java/com/puzzle/domain/model/payment/PurchaseProduct.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: build
🔇 Additional comments (27)
core/billing/src/main/java/com/puzzle/billing/model/payment/BasicCashProduct.kt (1)
1-1: LGTM! Clean package relocation.The package has been correctly updated to align with the billing module restructuring.
core/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSourceImpl.kt (2)
1-3: Package and import updates look correct.The package relocation and updated PurchaseProduct import align with the billing module refactoring.
15-16: Code is correct —toDto()exists as a member function onPurchaseProduct.
PurchaseProductincore/billing/src/main/java/com/puzzle/billing/model/payment/PurchaseProduct.ktdefinestoDto()as a member function (lines 10-14), which returnsVerifyPurchaseProductRequest. The call at line 16 is valid.core/billing/src/main/java/com/puzzle/billing/model/billing/PurchaseResult.kt (1)
1-1: LGTM! Package reorganization aligns with module structure.Moving PurchaseResult to the
billingsubpackage improves organization within the billing module.core/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSource.kt (1)
1-3: LGTM! Interface relocated to billing module.The package and import updates correctly move
PaymentDataSourceinto the billing module's network layer.feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (1)
5-7: LGTM! Import paths updated correctly.The import paths have been correctly updated to reflect the new billing module package structure (
com.puzzle.billing.model.billing). No logic changes required.core/network/src/main/java/com/puzzle/network/model/payment/VerifyPurchaseProductResponse.kt (1)
6-10: LGTM! Domain mapping correctly moved to billing mapper.The removal of
toDomain()from this network DTO aligns with the refactoring goal of separating concerns. The mapping logic has been relocated to the billing module's mapper, which improves modularity and keeps network models focused on serialization.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
5-5: LGTM! Import path updated correctly.The import path has been correctly updated to
com.puzzle.billing.model.billing.PurchaseResult, aligning with the billing module package restructuring.feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.kt (1)
11-12: LGTM! Import paths updated correctly.The import paths for
NormalProductandPieceProducthave been correctly updated tocom.puzzle.billing.model.billing, consistent with the billing module restructuring.core/billing/src/main/java/com/puzzle/billing/model/payment/PurchaseProduct.kt (1)
5-15: Confirm the store name format matches backend API expectations.The
toDto()method usesstore.namewhich produces uppercase strings ("PLAY_STORE", "APP_STORE"). Verify with your backend API documentation or contract that it expects this exact uppercase enum format, as REST APIs typically use lowercase (e.g., "play_store") or other conventions.feature/store/src/main/java/com/puzzle/store/ui/page/ExpandedStoreScreen.kt (1)
15-15: LGTM!Import path correctly updated to reflect the new billing model package structure.
feature/store/src/main/java/com/puzzle/store/contract/StoreState.kt (1)
3-3: LGTM!Import paths correctly updated for NormalProduct and PromotionProduct to reflect the new billing model package structure.
Also applies to: 5-5
feature/store/src/main/java/com/puzzle/store/ui/page/CompactStoreScreen.kt (1)
11-11: LGTM!Import path correctly updated to reflect the new billing model package structure.
feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.kt (1)
12-13: LGTM!Import paths correctly updated for PieceProduct and PromotionProduct to reflect the new billing model package structure.
feature/store/src/main/java/com/puzzle/store/contract/StoreIntent.kt (1)
3-3: Import paths successfully updated across the codebase.All billing model imports have been correctly migrated to the new
com.puzzle.billing.model.billingpackage. Verification confirms no old import paths remain in the codebase and all 21 references across feature/store, feature/matching, and core/billing modules use the correct new paths.core/billing/src/main/java/com/puzzle/billing/model/billing/PieceProduct.kt (1)
1-1: LGTM: Clean package relocation.The move to
com.puzzle.billing.model.billingcreates better organization for billing-specific models and aligns with the PR's goal of modularizing the billing component.core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt (1)
4-6: LGTM: Import updates reflect new package structure.The updated import paths correctly reference the relocated model classes while maintaining the existing interface contract.
core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt (1)
5-7: LGTM: Test fake updated consistently.The import updates maintain compatibility with the
BillingHelperinterface while reflecting the new package structure.feature/store/src/main/java/com/puzzle/store/contract/StoreSideEffect.kt (1)
3-3: LGTM: Cross-module import updated correctly.The feature/store module correctly references the relocated
PieceProductfrom the newbilling.model.billingpackage.core/billing/src/main/java/com/puzzle/billing/model/payment/VerifyPurchaseProduct.kt (1)
1-1: LGTM: Model correctly relocated to billing module with all imports updated.Moving
VerifyPurchaseProductfrom the domain module tocom.puzzle.billing.model.paymentaligns with the PR's decoupling objective. All imports have been properly updated across the codebase:
ProductMapper.ktimports and uses it for mappingVerifyPurchaseProductResponsePaymentRepository.ktuses it in theverifyPurchaseProductmethod signatureNo stale imports from the old domain package exist.
core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt (1)
3-5: LGTM! Import path migration aligns with module decoupling.The imports have been correctly updated to reference the billing module's own model package, supporting the goal of removing domain module dependencies.
core/network/src/main/java/com/puzzle/network/model/payment/GetAvailableProductResponse.kt (1)
9-26: Excellent separation of concerns!Removing
toDomain()mappings from network DTOs is a solid architectural improvement. The network layer now focuses purely on serialization, while domain mapping responsibility has been properly relocated to the billing module's mapper layer.core/billing/src/main/java/com/puzzle/billing/data/PaymentRepositoryImpl.kt (1)
4-17: Clean repository implementation with proper mapper integration.The implementation correctly uses the new mapper layer and updated import paths. The repository pattern is well-structured, delegating to the data source and applying domain mapping via
toDomain()extensions.core/billing/src/main/java/com/puzzle/billing/di/BillingModule.kt (1)
15-35: DI module structure is correct with all implementations properly configured.All three implementation classes have
@Injectconstructors as required:
PaymentDataSourceImplinjectsPieceApiPaymentRepositoryImplinjectsPaymentDataSourceBillingHelperImplinjectsPaymentRepositoryand@ApplicationContext ContextThe bindings follow standard patterns with appropriate singleton scoping.
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
42-45: LGTM! Constructor simplified correctly.The removal of the
AnalyticsHelperdependency aligns with the PR objective to make the Billing module independent. The constructor signature is cleaner and correctly maintains the required dependencies.
42-277: Overall refactoring structure is well-executed.The billing implementation maintains correct concurrency handling, proper error management, and clean integration with Google Play Billing. The refactoring successfully decouples the module from external dependencies while preserving the core functionality.
18-24: The new package structure is correctly in place and all imports are valid.All seven imported classes (
NormalProduct,PieceProduct,PromotionProduct,PurchaseResult,CashProduct,Product,PurchaseProduct) are properly defined in their respective packages:
com.puzzle.billing.model.billing: ContainsPieceProduct,NormalProduct,PromotionProduct, andPurchaseResultcom.puzzle.billing.model.payment: ContainsCashProduct,Product, andPurchaseProduct
| internal fun VerifyPurchaseProductResponse.toDomain(): VerifyPurchaseProduct = | ||
| VerifyPurchaseProduct( | ||
| rewardPuzzleCount = rewardPuzzleCount ?: UNKNOWN_INT | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how UNKNOWN_INT is used across the codebase and verify it's a valid sentinel
echo "=== UNKNOWN_INT definition ==="
rg -n "const val UNKNOWN_INT" --type=kt
echo -e "\n=== VerifyPurchaseProduct usage (check if UNKNOWN_INT is handled) ==="
rg -n "rewardPuzzleCount" --type=kt -A 2 -B 2 -g '!**/test/**'Repository: Piece-Puzzly/Piece-Android
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Check how UNKNOWN_INT is used across the codebase and verify it's a valid sentinel
echo "=== UNKNOWN_INT definition ==="
rg -n "const val UNKNOWN_INT" --glob="*.kt"
echo -e "\n=== UNKNOWN_INT all usages ==="
rg -n "UNKNOWN_INT" --glob="*.kt" -B 1 -A 1
echo -e "\n=== VerifyPurchaseProduct class definition ==="
rg -n "class VerifyPurchaseProduct" --glob="*.kt" -A 5
echo -e "\n=== rewardPuzzleCount property usage ==="
rg -n "rewardPuzzleCount" --glob="*.kt" -B 2 -A 2Repository: Piece-Puzzly/Piece-Android
Length of output: 22812
🏁 Script executed:
#!/bin/bash
echo "=== BillingHelperImpl full context around rewardPuzzleCount ==="
rg -n "rewardPuzzleCount" --glob="*.kt" -B 10 -A 5 core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
echo -e "\n=== BillingHelperImpl interface/contract ==="
rg -n "interface|fun.*rewardPuzzleCount" --glob="*.kt" core/billing/ -B 2 -A 3
echo -e "\n=== How verifyResult is used after return ==="
rg -n "verifyResult" --glob="*.kt" core/billing/Repository: Piece-Puzzly/Piece-Android
Length of output: 4439
🏁 Script executed:
#!/bin/bash
echo "=== handleSinglePurchase full implementation ==="
rg -n "handleSinglePurchase" --glob="*.kt" -A 20 core/billing/
echo -e "\n=== Reward usage in context ==="
rg -n "val reward|fetchPurchaseReward" --glob="*.kt" -B 2 -A 8 core/billing/Repository: Piece-Puzzly/Piece-Android
Length of output: 11213
Address inconsistent handling of UNKNOWN_INT sentinel in purchase reward flow.
The mapper correctly uses UNKNOWN_INT as a fallback, but downstream handling is inconsistent. While batch purchases filter out non-positive rewards (line 221: if (totalReward > 0)), single purchases directly return the reward value without validation (line 118). This means a null rewardPuzzleCount would result in PurchaseResult.Success(-1), which is likely unintended.
Either validate the reward in fetchPurchaseReward() (e.g., filter UNKNOWN_INT before returning) or ensure handleSinglePurchase() handles negative values consistently with batch processing.
🤖 Prompt for AI Agents
In
@core/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.kt
around lines 35 - 38, The mapping uses UNKNOWN_INT as a sentinel for missing
reward (VerifyPurchaseProductResponse.toDomain -> rewardPuzzleCount =
rewardPuzzleCount ?: UNKNOWN_INT) but downstream code treats that value
inconsistently; fix this by normalizing the sentinel before returning final
results: update fetchPurchaseReward() to treat UNKNOWN_INT (and any non-positive
value) as "no reward" and either return 0 or null (so callers can handle it
consistently), or alternatively modify handleSinglePurchase() to check the
reward returned and if reward <= 0 (including UNKNOWN_INT) convert it to the
same behavior as batch purchases (e.g., do not return
PurchaseResult.Success(-1); return Success(0) or the appropriate non-reward
path). Ensure you update the code paths that construct PurchaseResult.Success to
never be passed UNKNOWN_INT and reference
VerifyPurchaseProductResponse.toDomain, fetchPurchaseReward(),
handleSinglePurchase(), and PurchaseResult.Success when making the change.
| fun <T> ApiResponse<T>.unwrapData(): T { | ||
| return data ?: Unit as T | ||
| } |
There was a problem hiding this comment.
Critical: Unsafe cast will cause runtime crashes.
Line 26 contains an unsafe cast: data ?: Unit as T. When data is null and T is not Unit, this will compile but throw a ClassCastException at runtime.
Example failure:
val response = ApiResponse<String>(status = "ok", message = null, data = null)
val result = response.unwrapData() // Returns Unit cast as String → ClassCastExceptionNow that this function is public (accessible from the billing module), the risk of misuse increases.
🔒 Proposed fix
-fun <T> ApiResponse<T>.unwrapData(): T {
- return data ?: Unit as T
+fun <T> ApiResponse<T>.unwrapData(): T {
+ return data ?: throw IllegalStateException("ApiResponse data is null")
}Alternative: Return a nullable type or use a Result wrapper to handle the null case safely.
🤖 Prompt for AI Agents
In @core/network/src/main/java/com/puzzle/network/model/ApiResponse.kt around
lines 25 - 27, The unwrapData function performs an unsafe cast (data ?: Unit as
T) that can throw ClassCastException; change its contract to safely propagate
nulls by making the function return a nullable T (fun <T>
ApiResponse<T>.unwrapData(): T? = data) so callers handle absence explicitly, or
alternatively provide an explicit throwing variant (e.g., unwrapDataOrThrow()
that throws a clear IllegalStateException when data is null) and update any
callers of ApiResponse.unwrapData accordingly; locate the function
ApiResponse.unwrapData and replace the unsafe cast with a safe nullable return
or add the explicit throwing helper.
kkh725
added
리뷰 원해요🔥
| ) | ||
|
|
||
| internal fun VerifyPurchaseProductResponse.toDomain(): VerifyPurchaseProduct = | ||
| VerifyPurchaseProduct( |
There was a problem hiding this comment.
최근에 정한 대로 단일 값이면 VerifyPurchaseProduct 만들지 말고 Int로 처리해 주시면 좋을 거 같아요.
| } | ||
|
|
||
| if (verifyResult.rewardPuzzleCount > 0) { | ||
| analyticsHelper.trackRevenueEvent( |
There was a problem hiding this comment.
IOS 는 추가를 일단안한거같기도하고, 정확한 price 가 아니고 퍼즐값이라 일단 제거했어요
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)
42-45: Analytics tracking for purchase events is missing.The removal of
AnalyticsHelperfrom the constructor eliminated revenue tracking at the billing layer. WhileAnalyticsHelper.trackRevenueEvent()is still available in the codebase, it is never called when purchases succeed. ThePurchaseResult.Successis collected inStoreViewModelandMatchingViewModel, but neither location triggers any revenue or purchase analytics.This results in lost visibility into purchase completions and revenue events, which impacts business intelligence. Either add analytics tracking to the ViewModels that handle
PurchaseResult.Success, or restoreAnalyticsHelpertoBillingHelperImplto track revenue at the source.
🤖 Fix all issues with AI agents
In @core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt:
- Line 8: Change the repository method signature and mapper to use the domain
model VerifyPurchaseProduct instead of Int: update the suspend fun
verifyPurchaseProduct(purchaseProduct: PurchaseProduct) declaration in
PaymentRepository (and any implementing classes) to return
VerifyPurchaseProduct, then modify the mapping in ProductMapper (the conversion
from VerifyPurchaseProductResponse) to produce a VerifyPurchaseProduct instance
rather than an Int, and adjust callers to consume the domain object; ensure all
compile-time types and imports are updated accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktcore/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.ktcore/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.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: build
🔇 Additional comments (3)
core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt (1)
3-4: LGTM! Import refactoring aligns with module independence goal.The migration of
CashProductandPurchaseProductfrom the domain module to the billing module successfully decouples the Billing module from external domain dependencies.core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (2)
18-24: LGTM! Clean package reorganization.The import updates reflect the billing module's new independent structure. The namespace changes are consistent with the PR's modularization objectives.
100-113: LGTM! Improved variable naming.The rename from
verifyResulttorewardPuzzleCountmakes the variable's purpose clearer. The simplified return logic is also cleaner.
| interface PaymentRepository { | ||
| suspend fun getAvailableProduct(): CashProduct | ||
| suspend fun verifyPurchaseProduct(purchaseProduct: PurchaseProduct): VerifyPurchaseProduct | ||
| suspend fun verifyPurchaseProduct(purchaseProduct: PurchaseProduct): Int |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if VerifyPurchaseProduct exists in the billing module
echo "=== Searching for VerifyPurchaseProduct in billing module ==="
find core/billing -type f -name "*.kt" | xargs grep -l "VerifyPurchaseProduct" 2>/dev/null || echo "Not found in files"
# Search for definition of VerifyPurchaseProduct
echo -e "\n=== Searching for VerifyPurchaseProduct definition ==="
rg "class|data class|enum|sealed class|object|typealias.*VerifyPurchaseProduct" --type=kotlin
# Check the PaymentRepository file
echo -e "\n=== Content of PaymentRepository.kt ==="
find core/billing -name "PaymentRepository.kt" -type f -exec cat {} \;
# Search for usages of verifyPurchaseProduct method
echo -e "\n=== Finding callers of verifyPurchaseProduct ==="
rg -n "verifyPurchaseProduct" --type=kotlin -B2 -A2Repository: Piece-Puzzly/Piece-Android
Length of output: 50382
🏁 Script executed:
#!/bin/bash
# Get the PaymentRepository.kt file content
echo "=== PaymentRepository.kt content ==="
cat core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt
echo -e "\n=== VerifyPurchaseProduct.kt content ==="
cat core/billing/src/main/java/com/puzzle/billing/model/payment/VerifyPurchaseProduct.kt
echo -e "\n=== Usages of verifyPurchaseProduct method ==="
rg "verifyPurchaseProduct" --type=kotlin -B3 -A3Repository: Piece-Puzzly/Piece-Android
Length of output: 4815
🏁 Script executed:
#!/bin/bash
# Get PaymentRepositoryImpl to see what toDomain() actually does
echo "=== PaymentRepositoryImpl.kt content ==="
cat core/billing/src/main/java/com/puzzle/billing/data/PaymentRepositoryImpl.kt
# Check for toDomain extension functions
echo -e "\n=== Searching for toDomain extension on VerifyPurchaseProductResponse ==="
rg "fun.*VerifyPurchaseProductResponse.*toDomain" --type=kotlin -A5
# Check ProductMapper which likely has the mapping logic
echo -e "\n=== ProductMapper.kt content ==="
cat core/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 2542
Return type should be VerifyPurchaseProduct instead of Int—this is an architectural inconsistency.
The domain model VerifyPurchaseProduct exists but is not being used. The repository currently returns a raw Int value, which:
- Loses type safety: Primitive
Intlacks semantic meaning; callers cannot distinguish this from other integer values. - Violates clean architecture: Domain repositories should return domain models, not primitives.
- Breaks consistency: Other repository methods (e.g.,
getAvailableProduct()) correctly return domain models (CashProduct).
The current mapper in ProductMapper.kt explicitly converts VerifyPurchaseProductResponse to Int. Update it to return VerifyPurchaseProduct instead, and change the repository interface return type accordingly.
🤖 Prompt for AI Agents
In @core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt at
line 8, Change the repository method signature and mapper to use the domain
model VerifyPurchaseProduct instead of Int: update the suspend fun
verifyPurchaseProduct(purchaseProduct: PurchaseProduct) declaration in
PaymentRepository (and any implementing classes) to return
VerifyPurchaseProduct, then modify the mapping in ProductMapper (the conversion
from VerifyPurchaseProductResponse) to produce a VerifyPurchaseProduct instance
rather than an Int, and adjust callers to consume the domain object; ensure all
compile-time types and imports are updated accordingly.
| import com.puzzle.billing.model.billing.NormalProduct | ||
| import com.puzzle.billing.model.billing.PieceProduct | ||
| import com.puzzle.billing.model.billing.PromotionProduct | ||
| import com.puzzle.billing.model.billing.PurchaseResult | ||
| import com.puzzle.billing.model.payment.CashProduct | ||
| import com.puzzle.billing.model.payment.Product | ||
| import com.puzzle.billing.model.payment.PurchaseProduct |
There was a problem hiding this comment.
billing/model/billing 구조가 조금 이상한 것 같아요. billing 패키지를 더 좋은 네이밍이 없을까 고민해봐도 좋을 것 같아요.
| internal fun GetAvailableProductResponse.toDomain(): CashProduct = | ||
| CashProduct( | ||
| products = promotionCashProducts.map(PromotionProductResponse::toDomain) + | ||
| basicCashProducts.map(BasicCashProductResponse::toDomain), | ||
| ) | ||
|
|
||
| internal fun BasicCashProductResponse.toDomain(): Product.BasicCashProduct = | ||
| Product.BasicCashProduct( | ||
| uuid = uuid, | ||
| name = name, | ||
| rewardPuzzleCount = rewardPuzzleCount, | ||
| originalAmount = BigDecimal(originalAmount), | ||
| discountRate = discountRate, | ||
| discountedAmount = BigDecimal(discountedAmount), | ||
| ) | ||
|
|
||
| internal fun PromotionProductResponse.toDomain(): Product.PromotionProduct = | ||
| Product.PromotionProduct( | ||
| uuid = uuid, | ||
| imageUrl = cardImageUrl, | ||
| ) | ||
|
|
||
| internal fun VerifyPurchaseProductResponse.toDomain(): VerifyPurchaseProduct = | ||
| VerifyPurchaseProduct( |
There was a problem hiding this comment.
얘는 Network 모듈이 billing 모듈을 굳이 알게 하기 싫어서 이렇게 한 거군요.
타당 한 것 같아요~!
사실 다른 부분도 Mapper로 다 마이그레이션하면 data 하위 모듈 ex) database, datastore, network 가 domain을 직접 바라보는 것들도 다 끊을 수 있을 것 같긴한데, 피쳐보다 우선순위가 높은 건 아니라서 나중에 시간 나실 때 챙겨봐도 좋을 것 같네요,
tgyuuAn
added
머지 해도될듯염🌟
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/billing/src/main/java/com/puzzle/billing/model/PurchaseProduct.kt:
- Line 3: The PurchaseProduct data class currently imports
com.puzzle.network.model.payment.VerifyPurchaseProductRequest and exposes a
toDto() conversion, creating an unwanted network-module dependency; remove that
import and any toDto() method from the PurchaseProduct class so the domain model
is dependency-free, and instead add a new mapper file (e.g.,
PurchaseProductMapper in core.billing.mapper.payment) that defines an extension
function fun PurchaseProduct.toDto(): VerifyPurchaseProductRequest mapping uuid
-> productUUID, credential -> purchaseCredential and store.name -> store,
keeping the network import only inside the mapper.
🧹 Nitpick comments (2)
core/billing/src/main/java/com/puzzle/billing/model/BasicCashProduct.kt (1)
1-8: Model migration looks good. Consider elaborating the comment.The package migration from
domain.model.paymenttobilling.modelaligns with the PR's goal of establishing independent billing models. The// apicomment could be more descriptive—consider clarifying what makes this an "api" type (e.g., "// Public API exposed to data layer" or "// External API contract").core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt (1)
8-8: Return type change correctly implemented, but consider using a value class for improved type safety.The return type change from
VerifyPurchaseProductResponsetoIntis properly implemented across all layers. TheIntrepresentsrewardPuzzleCount(extracted viaVerifyPurchaseProductResponse.toDomain()), and all call sites inPaymentRepositoryImplandBillingHelperImplcorrectly handle the new return type.However, to improve type safety and self-document the domain semantics, consider wrapping the
Intin a value class:@JvmInline value class RewardPuzzleCount(val value: Int) suspend fun verifyPurchaseProduct(purchaseProduct: PurchaseProduct): RewardPuzzleCountThis would eliminate the ambiguity of a raw
Intand make the intent explicit throughout the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktcore/billing/src/main/java/com/puzzle/billing/data/PaymentRepositoryImpl.ktcore/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.ktcore/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.ktcore/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.ktcore/billing/src/main/java/com/puzzle/billing/model/BasicCashProduct.ktcore/billing/src/main/java/com/puzzle/billing/model/PieceProduct.ktcore/billing/src/main/java/com/puzzle/billing/model/PurchaseProduct.ktcore/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSource.ktcore/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSourceImpl.ktcore/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt
- core/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSourceImpl.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: build
🔇 Additional comments (13)
core/billing/src/main/java/com/puzzle/billing/model/PieceProduct.kt (1)
5-9: LGTM! Clear interface contract improvement.Explicitly declaring
pricein the sealed interface improves the contract clarity, even though implementations already provided it. The// for uicomment marks this as a UI-facing type.core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt (1)
6-6: LGTM! Import path updated consistently.The import change reflects the model migration from domain to billing package. No functional changes to the interface.
core/billing/src/main/java/com/puzzle/billing/model/PurchaseProduct.kt (1)
6-20: Data model structure is clean (pending network dependency fix).The
PurchaseProductdata class andStoreenum are well-structured for the billing domain. The default value ofStore.PLAY_STOREis reasonable for the Android context. Once the network dependency issue is resolved, this model will properly serve as a clean domain representation.core/billing/src/main/java/com/puzzle/billing/domain/PaymentRepository.kt (1)
3-4: LGTM! Imports updated consistently with model migration.The import changes align with moving
CashProductandPurchaseProductfrom the domain module to the billing module.core/billing/src/main/java/com/puzzle/billing/network/source/payment/PaymentDataSource.kt (1)
1-10: LGTM! Clean package refactoring.The interface has been successfully moved into the billing module's namespace, and the import paths are correctly updated to reference the new billing model types. This aligns well with the PR objective of making the billing module more independent.
core/billing/src/main/java/com/puzzle/billing/data/PaymentRepositoryImpl.kt (1)
1-18: LGTM! Repository properly refactored to use billing module types.The implementation correctly uses the new mapper functions and model types from the billing module. The repository maintains its clean separation of concerns, delegating response transformation to the mapper layer.
core/billing/src/main/java/com/puzzle/billing/mapper/payment/ProductMapper.kt (4)
12-16: LGTM! Clean list concatenation.The mapper correctly aggregates promotion and basic cash products into a single CashProduct domain model.
28-32: LGTM! Straightforward promotion product mapping.Simple and correct field mapping from response to domain model.
34-34: Verify UNKNOWN_INT fallback behavior.The mapper returns
UNKNOWN_INTwhenrewardPuzzleCountis null. Please confirm this is the intended behavior - should the system proceed with an unknown reward count, or should this case throw an exception since purchase verification without a reward count might indicate a backend error?If
nullindicates a failed verification, consider:internal fun VerifyPurchaseProductResponse.toDomain(): Int = this.rewardPuzzleCount ?: throw IllegalStateException("Purchase verification returned no reward count")Otherwise, ensure downstream code properly handles
UNKNOWN_INT.
18-26: No changes needed. The code is safe.The review assumes
originalAmountanddiscountedAmountare strings being converted toBigDecimal, which could throwNumberFormatException. However, they are declared asLonginBasicCashProductResponse. The mapper uses theBigDecimal(Long)constructor, which is safe and cannot throw exceptions. Numeric validation happens during JSON deserialization upstream; the mapper receives already-validatedLongvalues.Likely an incorrect or invalid review comment.
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
42-44: Confirm analytics tracking removal is acceptable.The
AnalyticsHelperdependency has been removed from the constructor as part of the module independence refactoring. This means purchase verification events are no longer tracked.Please confirm:
- Is the loss of purchase verification analytics tracking acceptable?
- If analytics are still needed, where should they be tracked instead (e.g., at a higher layer that depends on both billing and analytics modules)?
Based on the PR objectives, this change appears intentional to remove the Analytics module dependency from Billing.
100-113: LGTM! Cleaner variable naming.The variable rename from
verifyResulttorewardPuzzleCountis more descriptive and accurately reflects the returned value. The function logic remains correct.
22-24: LGTM! Import paths correctly updated.Imports properly reference the new billing module model types, maintaining consistency with the overall refactoring.
| @@ -0,0 +1,20 @@ | |||
| package com.puzzle.billing.model | |||
|
|
|||
| import com.puzzle.network.model.payment.VerifyPurchaseProductRequest | |||
There was a problem hiding this comment.
Critical: Network layer dependency contradicts module independence goal.
The import of com.puzzle.network.model.payment.VerifyPurchaseProductRequest creates a direct dependency from the billing module to the network layer. This contradicts the PR's stated objective of making the Billing module independent by removing external dependencies.
Impact:
- Billing module now depends on network module
- Cannot use/test
PurchaseProductwithout network module present - Violates separation of concerns between domain models and network DTOs
Recommendation:
Move the toDto() conversion logic to a dedicated mapper (e.g., PurchaseProductMapper) within the billing module's mapper layer, which can handle the dependency on network types. The domain model should remain dependency-free.
♻️ Suggested refactor to remove network dependency
Remove the network import and toDto() method from this file:
package com.puzzle.billing.model
-import com.puzzle.network.model.payment.VerifyPurchaseProductRequest
-
// api
data class PurchaseProduct(
val uuid: String,
val credential: String,
val store: Store = Store.PLAY_STORE,
-) {
- fun toDto() = VerifyPurchaseProductRequest(
- productUUID = uuid,
- purchaseCredential = credential,
- store = store.name,
- )
-}
+)
enum class Store {
PLAY_STORE, APP_STORE
}Then create a mapper in core/billing/mapper/payment/PurchaseProductMapper.kt:
package com.puzzle.billing.mapper.payment
import com.puzzle.billing.model.PurchaseProduct
import com.puzzle.network.model.payment.VerifyPurchaseProductRequest
fun PurchaseProduct.toDto() = VerifyPurchaseProductRequest(
productUUID = uuid,
purchaseCredential = credential,
store = store.name,
)🤖 Prompt for AI Agents
In @core/billing/src/main/java/com/puzzle/billing/model/PurchaseProduct.kt at
line 3, The PurchaseProduct data class currently imports
com.puzzle.network.model.payment.VerifyPurchaseProductRequest and exposes a
toDto() conversion, creating an unwanted network-module dependency; remove that
import and any toDto() method from the PurchaseProduct class so the domain model
is dependency-free, and instead add a new mapper file (e.g.,
PurchaseProductMapper in core.billing.mapper.payment) that defines an extension
function fun PurchaseProduct.toDto(): VerifyPurchaseProductRequest mapping uuid
-> productUUID, credential -> purchaseCredential and store.name -> store,
keeping the network import only inside the mapper.
* [PC-1639] 읽지 않은 notification 개수 확인 api 추가 * [PC-1639] matchingScreen 과 안읽은 알람 연동 완료 * [PC-1639] 24시간 이후인지 판단하는 string 확장함수 추가 * [PC-1639] UserInfo에 isApprovalExpired 파라미터 추가. -승인 후 24시간 여부 * [PC-1639] isApprovalExpired 관련 로직 추가 * [PC-1639] 매칭 풀 부족(기본) 다이얼로그 리팩토링 * [PC-1639] 즉시매칭 에러타입 추가 및 매칭풀부족 팝업 이슈 해결 * [PC-1639] unknown_error sideEffect 추가 * [PC-1639] 결제 성공 시 상품리스트 다시 풀러오는 로직 추가 * [PC-1639] 서버가 제공하는 순서로 상품을 넘겨주도록 수정 * [PC-1639] 기본 매칭 풀 부족 다이얼로그 표출 로직 수정 * [PC-1639] 알람 아이콘 res 수정 * [PC-1639] 기존 로직 변경에 따른 테스트 코드 수정 * [PC-1639] 매칭 screen 레이아웃 수정 * [PC-1639] 기존 로직 변경에 따른 테스트 코드 수정 * [PC-1639] pending, waiting 화면에 PiecePullRefreshBox 추가 * [PC-1639] 사용하지 않는 ic_alarm_black_unread.xml 제거 * [PC-1639] isOver24Hours 함수 crash 에러 방지 로직 추가 * [PC-1639] pending 인 경우 24시간 승인 여부 false로 수정 * [PC-1639] hasUnreadNotifications 비즈니스 모델 repository에서 계산하게 변경 --------- Co-authored-by: comst19 <[email protected]>
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/network/src/main/java/com/puzzle/network/model/user/GetUserInfoResponse.kt (1)
21-26: Potential runtime exception ifapprovedAtcontains malformed data.
formatIsoToDate()throwsDateTimeParseExceptionif the input isn't valid ISO format. WhileisOver24Hours()has internal error handling, it won't help here since the exception occurs before that function is called.If the server returns unexpected data in
approvedAt, this will crash the app during domain mapping.🛠️ Proposed fix with error handling
fun toDomain() = UserInfo( userId = userId ?: UNKNOWN_INT, userRole = UserRole.create(role), profileStatus = ProfileStatus.create(profileStatus), - isApprovalExpired = approvedAt?.let { formatIsoToDate(it).isOver24Hours() } ?: false + isApprovalExpired = approvedAt?.let { + runCatching { formatIsoToDate(it).isOver24Hours() }.getOrDefault(false) + } ?: false )
🤖 Fix all issues with AI agents
In
@core/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.kt:
- Around line 6-33: The FakeNotificationRepository is missing the required
hasUnreadNotifications(): Boolean implementation from NotificationRepository
causing a compile error; add an override fun hasUnreadNotifications(): Boolean
in FakeNotificationRepository (alongside setNotificationsCount/unreadCount) that
returns unreadCount > 0 so the fake reflects unread state correctly.
🧹 Nitpick comments (4)
core/designsystem/src/main/res/drawable/ic_alarm_unread.xml (1)
1-12: LGTM! Well-structured vector drawable.The vector drawable is correctly defined with matching dimensions and viewport. The two-path approach cleanly separates the main alarm icon from the unread indicator dot.
Optional consideration: The colors are hardcoded (
#ffffffand#FF7490). If theme support or design system consistency becomes a concern, consider referencing color resources (e.g.,@color/icon_primary) instead. However, this is fine for a purpose-specific icon.core/testing/src/main/java/com/puzzle/testing/domain/model/user/MockUserInfo.kt (1)
9-19: Consider addingisApprovalExpiredas a configurable parameter.The other properties are exposed as function parameters with defaults, but
isApprovalExpiredis hardcoded. This limits test flexibility when you need to test scenarios whereisApprovalExpired = false.♻️ Proposed fix
fun default( userId: Int = 1, userRole: UserRole = UserRole.USER, profileStatus: ProfileStatus = ProfileStatus.APPROVED, + isApprovalExpired: Boolean = true, ): UserInfo { return UserInfo( userId = userId, userRole = userRole, profileStatus = profileStatus, - isApprovalExpired = true, + isApprovalExpired = isApprovalExpired, ) }feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
11-12: Unused imports detected.
rememberScrollState,verticalScroll(lines 11-12), andColor(line 20) are imported but not used in this file. The scrolling functionality is used in the child screens (MatchingPendingScreen,MatchingWaitingScreen), not in this parent file.🧹 Remove unused imports
-import androidx.compose.foundation.rememberScrollState -import androidx.compose.foundation.verticalScroll-import androidx.compose.ui.graphics.ColorAlso applies to: 20-20
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
73-75: Instance variables may have stale state after app process recreation.These in-memory flags (
isApprovalExpired,isBasicMatchingPoolEmptyDialogShownToday,lastCheckDate) will reset to their default values if the ViewModel is recreated (e.g., after process death). TheisBasicMatchingPoolEmptyDialogShownTodayis mitigated by the persisted check on line 390, butisApprovalExpireddefaults totrue, which could lead to premature dialog display beforefetchMatchInfocompletes.Consider initializing
isApprovalExpiredtofalseto prevent showing the matching pool empty dialog until user info is fetched and confirms expiration.♻️ Suggested change
- private var isApprovalExpired: Boolean = true // 심사 24시간 이후 여부 + private var isApprovalExpired: Boolean = false // 심사 24시간 이후 여부
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktcore/common/src/main/java/com/puzzle/common/TimeUtil.ktcore/common/src/test/kotlin/com/puzzle/common/TimeUtilTest.ktcore/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.ktcore/designsystem/src/main/res/drawable/ic_alarm_unread.xmlcore/designsystem/src/main/res/values/strings.xmlcore/domain/src/main/java/com/puzzle/domain/model/user/UserInfo.ktcore/domain/src/main/java/com/puzzle/domain/repository/NotificationRepository.ktcore/network/src/main/java/com/puzzle/network/api/PieceApi.ktcore/network/src/main/java/com/puzzle/network/model/user/GetUserInfoResponse.ktcore/network/src/main/java/com/puzzle/network/source/notification/NotificationDataSource.ktcore/network/src/main/java/com/puzzle/network/source/notification/NotificationDataSourceImpl.ktcore/testing/src/main/java/com/puzzle/testing/domain/model/user/MockUserInfo.ktcore/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.ktfeature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingPendingScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingWaitingScreen.ktfeature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.ktfeature/store/src/main/java/com/puzzle/store/StoreViewModel.kt
💤 Files with no reviewable changes (2)
- feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt
- feature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T10:27:20.895Z
Learnt from: comst19
Repo: Piece-Puzzly/Piece-Android PR: 203
File: feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:242-246
Timestamp: 2026-01-10T10:27:20.895Z
Learning: In the Piece-Android project's SettingScreen.kt and AccessRightsPage.kt: The notification toggle represents the actual notification receivable state (combining system permission and app setting), and when system permission becomes granted, the app automatically enables push notifications through onPushNotificationCheckedChange() callback - this is intentional behavior per the app's notification policy.
Applied to files:
core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
🧬 Code graph analysis (6)
feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (4)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (1)
getAvailableProducts(134-134)core/billing/src/main/java/com/puzzle/billing/domain/BillingHelper.kt (1)
getAvailableProducts(11-11)core/billing/src/main/java/com/puzzle/billing/testing/FakeBillingHelper.kt (1)
getAvailableProducts(20-20)
core/network/src/main/java/com/puzzle/network/model/user/GetUserInfoResponse.kt (1)
core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
formatIsoToDate(94-100)
feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingPendingScreen.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Box.kt (1)
PiecePullRefreshBox(11-36)core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
PieceSolidButton(44-72)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
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-40)feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (1)
showDialog(294-295)core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
getResetDateString(118-127)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (2)
feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingPendingScreen.kt (1)
MatchingPendingScreen(36-107)feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingWaitingScreen.kt (1)
MatchingWaitingScreen(35-102)
feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingWaitingScreen.kt (3)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Box.kt (1)
PiecePullRefreshBox(11-36)core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
PieceSolidButton(44-72)feature/matching/src/main/java/com/puzzle/matching/graph/main/component/card/MatchingCard.kt (1)
NewMatchingCard(52-89)
⏰ 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 (30)
core/designsystem/src/main/res/values/strings.xml (2)
10-10: LGTM!The generic error string is appropriately placed in the SnackBar section and provides a useful fallback for unexpected error scenarios.
87-87: LGTM!This change aligns with the existing
basicinfo_main_labelon line 150, creating consistent terminology ("오늘의 인연") throughout the app's matching-related UI.feature/store/src/main/java/com/puzzle/store/StoreViewModel.kt (1)
89-95: LGTM! Sensible addition to refresh products after purchase.The new call to
getAvailableProducts()after a successful purchase ensures the product list stays in sync with any backend changes (e.g., limited-time offers expiring, stock changes). Error handling is already in place withingetAvailableProducts().core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt (3)
22-24: LGTM!Import migration from
com.puzzle.domain.model.paymenttocom.puzzle.billing.model.*properly supports the billing module's independence from the Domain module.
100-113: Analytics tracking removed for purchase verification.The refactoring removes analytics tracking from purchase reward verification. While this achieves the goal of removing the Analytics module dependency, purchase revenue/conversion analytics are now lost.
Ensure this trade-off is acceptable from a business analytics perspective, or consider re-adding analytics at a higher layer (e.g., in the consuming feature module).
152-163: Improved product mapping logic.The refactored approach is cleaner and more efficient:
playProductMapprovides O(1) lookups instead of iterating to find matches- Iterating over
cashProducts.productsensures server-defined product ordering is preservedmapNotNullgracefully handles missing Play productsThis is a good improvement.
core/domain/src/main/java/com/puzzle/domain/model/user/UserInfo.kt (1)
7-7: LGTM!Clean addition of the
isApprovalExpiredproperty with an inline comment explaining its purpose.core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
129-141: LGTM!The
isOver24Hours()implementation is well-designed:
- Uses
runCatchingfor robust error handling- Returns
falseon parsing failures, which is a safe default- Correctly uses Seoul timezone for current time comparison
core/common/src/test/kotlin/com/puzzle/common/TimeUtilTest.kt (1)
206-264: LGTM!Comprehensive test coverage for
isOver24Hours():
- Parameterized tests for invalid format handling
- Boundary tests at 23, 24, and 25 hours correctly validate the >= 24 threshold
core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
207-209: LGTM!The new endpoint follows the established patterns in the interface and is appropriately placed alongside related notification methods.
core/network/src/main/java/com/puzzle/network/source/notification/NotificationDataSource.kt (1)
9-9: LGTM!The new interface method is well-defined. Minor observation: the method uses singular form (
getNotificationCount) while the API endpoint uses plural (getNotificationsCount) - both are acceptable, though singular is more idiomatic for count methods.core/network/src/main/java/com/puzzle/network/source/notification/NotificationDataSourceImpl.kt (1)
42-42: LGTM!The implementation correctly follows the established pattern of delegating to the API and unwrapping the response.
core/domain/src/main/java/com/puzzle/domain/repository/NotificationRepository.kt (1)
9-9: LGTM!Good domain abstraction - exposing a semantic boolean (
hasUnreadNotifications) rather than leaking the raw count to consumers. This hides implementation details and provides a cleaner API.core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.kt (1)
28-29: LGTM!The implementation correctly translates the data source count to the domain's boolean abstraction.
feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.kt (1)
22-22: LGTM!The new
hasUnreadNotificationsfield follows the existing pattern and has a sensible default value offalse.feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.kt (1)
35-57: LGTM!The test setup correctly wires the new
FakeNotificationRepositorydependency, following the established pattern for other fake repositories in this test class.feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingPendingScreen.kt (1)
55-106: LGTM! Pull-to-refresh implementation is correct.The
PiecePullRefreshBoxis properly integrated with scrollable content inside. Minor formatting note: line 58 has){without a space, but this is a nitpick if the project doesn't enforce this style.feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingWaitingScreen.kt (1)
43-101: LGTM! Pull-to-refresh implementation is consistent withMatchingPendingScreen.The structure correctly wraps content in
PiecePullRefreshBoxwith scrollable content, and properly includesNewMatchingCardwithin the scrollable area.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (6)
76-83: LGTM!The notification count fetch is properly integrated into the lifecycle initialization alongside other data fetches (
fetchMatchInfo,fetchFreeMatch,fetchPuzzleCount), ensuring notifications state is loaded when the screen starts.
85-102: LGTM!The expanded snackbar handling correctly differentiates between
onInfo,onTextOnly, andonMatchingmessage types, routing each to the appropriateSnackBarStatevariant. The structure is clean and extensible.
164-166: LGTM!The conditional alarm icon resource selection based on
hasUnreadNotificationsis a clean approach for indicating unread notification state visually.
209-246: LGTM!The alarm icon is consistently applied across both top bar variants (
PieceMainTopBarandPiecePuzzleTopBar), ensuring uniform UX regardless of the matching status.
249-278: LGTM!Pull-to-refresh and
isRefreshingparameters are properly wired to all matching status screens (MatchingPendingScreen,MatchingWaitingScreen,MatchingUserScreen), maintaining consistent refresh behavior across all states.
287-314: LGTM!Preview functions are updated to include the new
isRefreshingandonPullToRefreshparameters with sensible default values.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (6)
58-69: LGTM!The
NotificationRepositoryis properly injected via constructor injection, following the existing pattern for other repositories.
107-113: LGTM!The
fetchNotificationsCount()function follows the same pattern as other fetch functions, properly usingsuspendRunCatchingfor error handling and updating state on success.
198-208: LGTM!The
fetchNotificationsCount()is correctly added to thejoinAllcall, ensuring notification state is refreshed alongside other data during pull-to-refresh.
275-290: LGTM!The expanded error handling correctly differentiates between:
NOT_ENOUGH_USER_FOR_INSTANT_MATCH→ Shows appropriate pool empty dialog (Basic vs New type)EXPIRED_TOKEN,NOT_ENOUGH_PUZZLE→ Routes to error helper- Other/unknown errors → Shows generic snackbar
This provides better UX by showing contextual feedback.
390-394: This concern is not applicable to the codebase.
getLastMatchingPoolEmptyDialogShownDate()returns a cold Flow from DataStore that emits the current stored value immediately upon collection. DataStore is specifically designed for this usage pattern. The.first()call is appropriately placed within a suspend function and will not block.
378-403: The code is thread-safe; no race condition exists.All variable accesses (
isApprovalExpired,isBasicMatchingPoolEmptyDialogShownToday,lastCheckDate) occur onDispatchers.Main, which is single-threaded and guarantees sequential execution. The dispatcher architecture ensures memory visibility and mutual exclusion without requiring@Volatileannotations. While adding@Volatilecould serve as defensive documentation, it is not necessary for correctness.Likely an incorrect or invalid review comment.
core/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/data/src/test/java/com/puzzle/data/source/notification/SpyNotificationDataSource.kt (1)
12-12: Consider adding a configurable return value for better test flexibility.The stub works fine for basic tests, but if you need to test scenarios where unread notifications exist, consider adding a configurable property similar to the
updateDeviceTokenCallCountpattern.♻️ Optional improvement for test flexibility
class SpyNotificationDataSource : NotificationDataSource { var updateDeviceTokenCallCount: Int = 0 + var notificationCountToReturn: Int = 0 + override suspend fun updateDeviceToken(token: String) { updateDeviceTokenCallCount++ } - override suspend fun getNotificationCount(): Int = 0 + override suspend fun getNotificationCount(): Int = notificationCountToReturn override suspend fun postDeviceToken() {} override suspend fun getNotifications(cursor: Int?): List<GetNotificationResponse> = emptyList() override suspend fun readNotification(id: Int) {} }core/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.kt (1)
25-27: Consider adding call tracking for better test assertions.Currently,
readNotificationis a no-op andgetNotificationsignores thecursorparameter. If future tests need to verify these interactions, consider adding tracking:♻️ Optional enhancement for call verification
class FakeNotificationRepository : NotificationRepository { private var deviceToken: String = "" private var notifications: List<Notification> = emptyList() private var hasUnreadNotifications: Boolean = false + private val readNotificationIds: MutableList<Int> = mutableListOf() + private var lastCursor: Int? = null + fun getReadNotificationIds(): List<Int> = readNotificationIds + fun getLastCursor(): Int? = lastCursor - override suspend fun readNotification(id: Int) {} + override suspend fun readNotification(id: Int) { + readNotificationIds.add(id) + } - override suspend fun getNotifications(cursor: Int?): List<Notification> = notifications + override suspend fun getNotifications(cursor: Int?): List<Notification> { + lastCursor = cursor + return notifications + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/data/src/test/java/com/puzzle/data/source/notification/SpyNotificationDataSource.ktcore/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.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: build
🔇 Additional comments (1)
core/testing/src/main/java/com/puzzle/testing/domain/repository/FakeNotificationRepository.kt (1)
6-29: LGTM! Clean fake repository implementation for testing.The implementation correctly provides test doubles for all
NotificationRepositorymethods with configurable state.
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
전
후
전체비교
전
후
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Billing module 에서 Retrofit을 사용해서 자체적으로 api 호출까지 수행하고, 완전한 독립모듈로 만드는게 나을지,
현재 구조처럼 기존의 호출부(network) 는 연결하되, 나머지 부분만 독립적으로 진행할지. 의견 부탁드립니다 ~!
@tgyuuAn @comst19
Summary by CodeRabbit
New Features
Bug Fixes
UI / Content
✏️ Tip: You can customize this high-level summary in your review settings.