[PC-1514] MatchingScreen 에서 발생하는 에러 예외 처리 추가 #190
Conversation
📝 WalkthroughWalkthroughAdds a MatchingPoolEmpty dialog and strings; renames pull-to-refresh intents/callbacks; wires MatchingPoolEmpty handling with daily gating persisted in datastore; introduces AnalyticsEvent.Track/Revenue and revenue tracking; injects analytics into billing/store/matching; clears dialog state on logout/withdraw. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant MatchingScreen
participant MatchingViewModel
participant LocalUserStore
participant Nav
rect `#F3FAFF`
Note over MatchingViewModel,LocalUserStore: Instant-match flow with daily gating for MatchingPoolEmpty
end
User->>MatchingScreen: Tap "Get New Match"
MatchingScreen->>MatchingViewModel: getInstantNewMatch(isFree)
MatchingViewModel->>LocalUserStore: request instant match -> response (matchId / error)
alt success (matchId)
MatchingViewModel->>MatchingViewModel: checkMatchingPiece(matchId)
alt piece exists
MatchingViewModel->>Nav: navigate to MatchingDetailRoute(matchId)
else piece missing
MatchingViewModel->>MatchingScreen: show InSufficient dialog
end
else failure (NOT_ENOUGH_USER_FOR_INSTANT_MATCH)
MatchingViewModel->>LocalUserStore: read lastMatchingPoolEmptyDialogShownDate()
alt not shown today and pool empty
MatchingViewModel->>MatchingScreen: set dialogType = MatchingPoolEmpty(Basic|New)
MatchingScreen->>User: render MatchingPoolEmptyDialog
User->>MatchingPoolEmptyDialog: tap primary button
MatchingPoolEmptyDialog->>MatchingScreen: onNextTryClick()
MatchingScreen->>MatchingViewModel: retry / log analytics
else fallback
MatchingViewModel->>MatchingScreen: show other dialog
end
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
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
320-345: PassmatchIdtohandleMatchingErrorin operations that may receiveALREADY_PURCHASED_CONTACTerrors.The error handling structure is sound, but two functions have a critical issue: they receive
matchIdbut don't pass it tohandleMatchingError, breaking error recovery:
postOpponentContact(matchId: Int)(line 268): CallspostOpponentContacts(matchId)which can returnALREADY_PURCHASED_CONTACT. Should passmatchIdso navigation toContactRoutesucceeds.checkMatchingPiece(matchId: Int)(line 275): Should passmatchIdwhen callinghandleMatchingError(it, matchId).Without
matchId, theALREADY_PURCHASED_CONTACThandler navigates nowhere and falls back to generic error handling.
🧹 Nitpick comments (4)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/TimerCompleteDialog.kt (1)
13-13: Good refactor to reduce API surface.Changing the visibility to
internalappropriately limits the scope of this dialog to the feature module, following the principle of least privilege for API design.If you want to be thorough, verify that no other modules depend on this function:
#!/bin/bash # Description: Check if TimerCompleteDialog is referenced outside the feature/matching module # Search for imports or direct references to TimerCompleteDialog in other modules rg -n 'TimerCompleteDialog' -g '!feature/matching/**' -g '*.kt'feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (2)
13-13: Reminder: Address the TODO comment.The TODO indicates that BASIC matching pool logic with a different button label is not yet implemented. This may lead to inconsistent UX if BASIC matching scenarios are triggered.
Do you want me to help design the logic to differentiate button labels for BASIC vs. other matching pool scenarios, or open an issue to track this?
38-44: Consider making the preview function private.Preview composables are typically marked
privateto avoid exposing them in the public API surface.🔎 Suggested fix
@Preview @Composable -fun PreviewMatchingPoolEmptyDialog() { +private fun PreviewMatchingPoolEmptyDialog() { PieceTheme { MatchingPoolEmptyDialog({}) } }feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
43-43: Unused import:PROFILE_TIMER_COMPLETEThis constant doesn't appear to be used anywhere in this file. Consider removing it if it was added unintentionally.
🔎 Suggested fix
-import com.puzzle.matching.graph.detail.common.constant.MatchingDetailDialogType.PROFILE_TIMER_COMPLETE
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/designsystem/src/main/res/values/strings.xmlfeature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/TimerCompleteDialog.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/constant/MatchingDialogType.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (9)
core/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.kt (1)
checkMatchingPiece(79-80)core/data/src/test/java/com/puzzle/data/source/matching/FakeMatchingDataSource.kt (1)
checkMatchingPiece(86-86)core/domain/src/main/java/com/puzzle/domain/repository/MatchingRepository.kt (1)
checkMatchingPiece(19-19)core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
checkMatchingPiece(168-169)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSourceImpl.kt (1)
checkMatchingPiece(61-61)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSource.kt (1)
checkMatchingPiece(26-26)core/testing/src/main/java/com/puzzle/testing/domain/repository/SpyMatchingRepository.kt (1)
checkMatchingPiece(77-77)core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (3)
PieceDialog(33-58)PieceDialogIconTop(164-214)PieceDialogOneButtonBottom(243-260)core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
MatchingPoolEmptyDialog(14-36)
⏰ 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 (5)
feature/matching/src/main/java/com/puzzle/matching/graph/main/constant/MatchingDialogType.kt (1)
9-9: LGTM!The new
MatchingPoolEmptydialog type is correctly added as a data object, consistent with the existing pattern for simple dialog types likeInSufficientandNewMatching.core/designsystem/src/main/res/values/strings.xml (1)
92-94: LGTM!The new string resources for the matching pool empty dialog are well-named, consistent with existing patterns, and the messaging appropriately informs users that their puzzles have been returned.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
238-247: LGTM!The updated flow correctly:
- Shows a puzzle-used snackbar when free matches are exhausted
- Delegates navigation to
checkMatchingPiecewhich handles the piece verification step before proceeding to the detail screen
254-254: LGTM!Closing the dialog on confirmation is the appropriate behavior since the server has already refunded the puzzles.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
177-185: LGTM!The new
MatchingPoolEmptydialog handling follows the established pattern consistently, including proper analytics tracking for the "next_try" button click.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
275-275: PassmatchIdtohandleMatchingErrorto enableALREADY_PURCHASED_CONTACThandling.This issue was previously flagged but remains unresolved. The
handleMatchingErrorfunction now accepts an optionalmatchIdparameter to navigate toContactRoutewhen theALREADY_PURCHASED_CONTACTerror occurs. However,matchIdis not passed here, preventing that navigation path from triggering whencheckMatchingPiecefails.🔎 Proposed fix
private fun checkMatchingPiece(matchId: Int) = viewModelScope.launch(dispatcher) { suspendRunCatching { matchingRepository.checkMatchingPiece(matchId) }.onSuccess { navigationHelper.navigate(To(MatchingDetailRoute(matchId))) - }.onFailure { handleMatchingError(it) } + }.onFailure { handleMatchingError(it, matchId) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.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 (6)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (6)
254-254: LGTM!The handling for
MatchingPoolEmptydialog correctly closes the dialog, which is appropriate for an informational message.
268-268: Good improvement.Passing
matchIdtohandleMatchingErrorenables theALREADY_PURCHASED_CONTACTerror case to navigate correctly to the contact screen.
320-320: Good API design.Adding the optional
matchIdparameter tohandleMatchingErrorprovides the context needed for error-specific navigation while maintaining backward compatibility.
324-327: Good practice: synchronizing puzzle count before showing the dialog.Calling
fetchPuzzleCount()before displaying the insufficient puzzle dialog ensures that the UI reflects the current puzzle count, providing accurate information to the user.
329-331: Appropriate error handling for empty matching pool.Showing the
MatchingPoolEmptydialog for theNOT_ENOUGH_USER_FOR_INSTANT_MATCHerror provides clear feedback to users when instant matching is unavailable.
333-337: LGTM!The
ALREADY_PURCHASED_CONTACTerror handling correctly navigates to the contact screen when amatchIdis available, with proper null-safety.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
feature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.kt (1)
38-38: LGTM! Test helper correctly updated for new API.The addition of the
onRefreshparameter with a no-op lambda is appropriate for the test helper, ensuring all existing tests continue to work with the updatedMatchingScreensignature.Optionally, consider adding a test case to verify that the refresh callback is invoked in the expected scenarios (e.g., after handling puzzle shortage errors or when the MatchingPoolEmpty dialog is dismissed), especially since the PR introduces error handling and synchronization logic tied to this callback.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
325-328:fetchPuzzleCount()is async; dialog may show stale count.
fetchPuzzleCount()launches a coroutine and returns immediately. TheInSufficientdialog will display before the puzzle count state updates, potentially showing a stale value.If accurate display is important, consider awaiting the fetch or showing a loading state.
🔎 Option: await the fetch before showing the dialog
PieceErrorCode.NOT_ENOUGH_PUZZLE -> { - fetchPuzzleCount() + fetchPuzzleCount().join() showPurchaseDialog(MatchingDialogType.InSufficient) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
🔇 Additional comments (6)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (6)
230-233: LGTM!The explicit
isPuzzleUsed: falseparameter clearly indicates that no puzzle is consumed for free matches, making the intent readable and correct.
235-247: Good fix for the previously flagged issue.The
isPuzzleUsedparameter approach is cleaner than relying on state checks. The caller now explicitly declares intent, ensuring the snackbar displays correctly when a puzzle is consumed.
249-255: LGTM!
isPuzzleUsed: truecorrectly indicates that confirming the NewMatching dialog consumes a puzzle. TheMatchingPoolEmptycase appropriately just dismisses the dialog.
268-268: LGTM!Passing
matchIdtohandleMatchingErrorenables context-aware handling for theALREADY_PURCHASED_CONTACTcase.
270-277: Consider settingopenedMatchIdbefore navigation.
openedMatchIdis set afternavigationHelper.navigate(). If navigation triggers a lifecycle event that refreshes the match list (viafetchMatchInfo→getMatchInfoList), theopenedMatchIdcheck at line 148 would use the stale value, potentially expanding the wrong card on return.🔎 Suggested fix
}.onSuccess { + openedMatchId = matchId navigationHelper.navigate(To(MatchingDetailRoute(matchId))) - openedMatchId = matchId }.onFailure { handleMatchingError(it) }
330-338: Good error recovery patterns.
NOT_ENOUGH_USER_FOR_INSTANT_MATCHnow shows the newMatchingPoolEmptydialog instead of a snackbar, aligning with the PR objective. TheALREADY_PURCHASED_CONTACThandling provides graceful recovery by navigating to the contact screen when the user already has access.
|
|
||
| @Preview | ||
| @Composable | ||
| fun PreviewMatchingPoolEmptyDialog() { |
| } | ||
|
|
||
| private suspend fun getInstantNewMatch() = suspendRunCatching { | ||
| private suspend fun getInstantNewMatch(isPuzzleUsed: Boolean) = suspendRunCatching { |
There was a problem hiding this comment.
퍼즐을 처리한 뒤에 함수가 실행되는것이 아닌데 isPuzzleUsed 가 네이밍이 헷갈릴 것 같아요.
There was a problem hiding this comment.
isPuzzleUsed -> isFree로 변경했습니다.
=> b965f6e
|
피알 제목에 지라티켓명 달아주세요 ~~ |
comst19
changed the title
| onCheckMyProfileClick = {}, | ||
| onEditProfileClick = {}, | ||
| onNotificationClick = {}, | ||
| onRefresh = {}, |
There was a problem hiding this comment.
onRefresh는 사용자의 동작일까요? 뭔가 swipeRefresh 그런 것 같은데 사용자의 intent를 나타내는 더 좋은 네이밍이 있을지도 ?!
There was a problem hiding this comment.
네, 사용자가 메인 매칭 화면에서 아래로 당겨서 새로고침하는 동작입니다.
아래로 당겨서 하는 행위라 onPullToRefresh로 변경했습니다.
=> 9ca87f9
tgyuuAn
added
머지 해도될듯염🌟
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
321-346: Consider passingmatchIdfromcheckMatchingPiecetohandleMatchingError.The
ALREADY_PURCHASED_CONTACTerror handling (lines 334-338) silently does nothing whenmatchIdisnull. Currently,checkMatchingPieceon line 276 doesn't pass itsmatchIdtohandleMatchingError, so if that call returnsALREADY_PURCHASED_CONTACT, no navigation occurs.If this error can occur from
checkMatchingPiece, consider updating line 276:- }.onFailure { handleMatchingError(it) } + }.onFailure { handleMatchingError(it, matchId) }feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
50-50: Optional: Consider explicit imports over wildcard import.The wildcard import
MatchingDialogType.*works but explicit imports (Contact,InSufficient,NewMatching,MatchingPoolEmpty) improve readability and make it clearer which dialog types are used in this file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feature/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/MatchingIntent.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
MatchingPoolEmptyDialog(14-36)
feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Box.kt (1)
PiecePullRefreshBox(11-36)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (9)
core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)core/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.kt (1)
checkMatchingPiece(79-80)core/data/src/test/java/com/puzzle/data/source/matching/FakeMatchingDataSource.kt (1)
checkMatchingPiece(86-86)core/domain/src/main/java/com/puzzle/domain/repository/MatchingRepository.kt (1)
checkMatchingPiece(19-19)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSource.kt (1)
checkMatchingPiece(26-26)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSourceImpl.kt (1)
checkMatchingPiece(61-61)core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
checkMatchingPiece(168-169)core/testing/src/main/java/com/puzzle/testing/domain/repository/SpyMatchingRepository.kt (1)
checkMatchingPiece(77-77)core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)
⏰ 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 (9)
feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingIntent.kt (1)
14-14: LGTM!The rename from
OnRefreshtoOnPullToRefreshis more descriptive and aligns with the updated callback naming throughout the matching flow.feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt (1)
39-43: LGTM!The parameter rename to
onPullToRefreshis consistent with the broader API changes. The internal wiring toPiecePullRefreshBox.onRefreshis correctly maintained.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
87-87: LGTM!Intent handling correctly updated to match the renamed
OnPullToRefreshintent.
231-247: LGTM!The
isPuzzleUsedparameter correctly addresses the previous review feedback about the unreachable snackbar condition. The flow now properly shows the puzzle-used snackbar only when a paid match is initiated, andcheckMatchingPieceis consistently called for all new matches.
249-255: LGTM!Dialog confirmation handling is correctly implemented:
NewMatchingtriggers a paid match withisPuzzleUsed=trueMatchingPoolEmptysimply dismisses the dialog as expected for an informational alert
268-277: LGTM!Error handling now includes
matchIdcontext for proper navigation onALREADY_PURCHASED_CONTACT. TheopenedMatchIdupdate after navigation correctly tracks the opened match for subsequent list refreshes.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (3)
176-184: LGTM!The
MatchingPoolEmptyDialogintegration follows the established pattern for dialog handling, with proper analytics tracking. The dialog correctly routes throughonAcceptClickto close via the ViewModel.
120-135: LGTM!The
onPullToRefreshcallback is properly threaded fromMatchingRoutethroughMatchingScreen, with consistent naming throughout the component hierarchy.
253-261: LGTM!
MatchingUserScreencorrectly receives the renamedonPullToRefreshcallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
13-13: Address the TODO for BASIC matching pool logic.The comment indicates that different button labels are needed when the matching pool is empty for BASIC matches. This functionality should be implemented or tracked separately.
Would you like me to open a separate issue to track this TODO, or do you have plans to address it in a follow-up PR?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/TimerCompleteDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/TimerCompleteDialog.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (3)
PieceDialog(33-58)PieceDialogIconTop(164-214)PieceDialogOneButtonBottom(243-260)core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (6)
core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)core/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.kt (1)
checkMatchingPiece(79-80)core/domain/src/main/java/com/puzzle/domain/repository/MatchingRepository.kt (1)
checkMatchingPiece(19-19)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSource.kt (1)
checkMatchingPiece(26-26)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSourceImpl.kt (1)
checkMatchingPiece(61-61)core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)
⏰ 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 (5)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
34-34: Verify the no-op dismiss behavior is intentional.The dialog cannot be dismissed by tapping outside since
onDismissRequestis a no-op. This forces users to click the button, which may be the intended UX to ensure they acknowledge the empty pool state.Confirm this is the desired behavior. If users should be able to dismiss the dialog by tapping outside, update this to hide the dialog:
🔎 Proposed fix if dismissal should be allowed
- onDismissRequest = {} + onDismissRequest = { onNextTryClick() }feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
230-247: LGTM! The isFree parameter fix resolves the previous unreachable code issue.The refactored logic correctly handles both free and paid instant matches:
- Free matches (line 231): Skip puzzle usage snackbar
- Paid matches (line 252): Show puzzle usage snackbar before checking piece
The addition of
checkMatchingPiece(matchId)(line 246) ensures proper piece validation before navigation, aligning with the PR objectives.
254-254: LGTM! MatchingPoolEmpty dialog handling is appropriate.The dialog dismissal allows users to acknowledge the empty pool state and return to the matching screen.
270-277: LGTM! The checkMatchingPiece integration is well-structured.The function properly:
- Validates the matching piece via repository
- Navigates to detail screen on success
- Preserves card expansion state with
openedMatchId- Delegates error handling appropriately
321-346: LGTM! Enhanced error handling with proper context propagation.The improvements include:
- Line 326: Puzzle count synchronization before showing InSufficient dialog (as per PR objectives)
- Lines 330-332: Correct integration of MatchingPoolEmpty dialog for empty pool errors
- Lines 334-338: Smart recovery for ALREADY_PURCHASED_CONTACT using matchId context
Note:
fetchPuzzleCount()at line 326 is fire-and-forget, so the InSufficient dialog shows immediately while the count updates asynchronously. This is acceptable since the dialog doesn't directly display the puzzle count in its content.
* [PC-1537] analyticsHelper 미구현 screen 로깅 추가 및 최상단이 아닌 컴포넌트단으로 위치 이동 * [PC-1537] MatchingDetailScreen에 entryRoute 추가 * [PC-1537] entryRoute -> entryRouteForLog 로 파라미터명 수정 * [PC-1537] entryRoute -> entryRouteForLog 로 파라미터명 수정 * [PC-1537] AmplitudeEvent를 track, revenue 타입으로 변경 * [PC-1537] billing 모듈에 analytics 의존성 추가 * [PC-1537] trackRevenue 추가
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
147-199: Inconsistent screen name for BasicInfoPage analytics.At Line 137,
BasicInfoPagemaps to"match_detail_basic_profile", but at Line 378-379 inMatchingDetailBottomBar, the same page uses"match_detail_basic_info"for the photo_view click event. This inconsistency will cause analytics fragmentation.🔎 Proposed fix
onShowPicturesClick() analyticsHelper.trackClickEvent( - screenName = "match_detail_basic_info", + screenName = "match_detail_basic_profile", buttonName = "photo_view" )
🧹 Nitpick comments (5)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/AcceptMatchingDialog.kt (1)
45-48: Verify screen name consistency for analytics.The click event uses
screenName = "match_dialog"(line 46), while the screen view event usesscreenName = "match_main_accept_popup"(line 55). This inconsistency might make it difficult to correlate click events with screen views in analytics.Consider aligning the screen names or verify this is intentional.
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/ProfileImageDialog.kt (1)
28-31: Verify screen name consistency for analytics.Similar to AcceptMatchingDialog, the click event uses
screenName = "match_dialog"while the screen view event usesscreenName = "match_detail_photo". Consider aligning these names for better analytics correlation.debug/src/main/java/com/puzzle/debug/AnalyticsScreen.kt (1)
105-110: Consider displayingquantityfor Revenue events.The Revenue event has a
quantityfield that isn't displayed here. For a complete debug view, consider including it.🔎 Proposed fix
is AnalyticsEvent.Revenue -> Text( - text = "${event.productId} : ${event.price}", + text = "${event.productId} : ${event.price} x ${event.quantity}", style = PieceTheme.typography.bodySSB, color = PieceTheme.colors.dark2, )feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.kt (1)
80-82: Minor: Remove extra blank line.There's an unnecessary blank line at Line 81 after the closing brace of
PurchaseDialog.🔎 Proposed fix
onDismissRequest = onDismissRequest, ) - }core/analytics/src/main/java/com/puzzle/analytics/AnalyticsEvent.kt (1)
5-8: Consider using immutableMapinstead ofMutableMapfor properties.Using
MutableMap<String, Any?>?in a data class can lead to unintended mutations after creation. PreferMap<String, Any?>?for the public API, and convert to mutable only where needed internally.🔎 Proposed fix
data class Track( val type: String, - val properties: MutableMap<String, Any?>? = null + val properties: Map<String, Any?>? = null ) : AnalyticsEvent {This may require adjusting call sites that currently construct a
mutableMapOfand pass it directly—they can still usemutableMapOf(...).toMap()or just pass the map as-is sinceMutableMapis a subtype ofMap.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/src/release/generated/baselineProfiles/baseline-prof.txtis excluded by!**/generated/**app/src/release/generated/baselineProfiles/startup-prof.txtis excluded by!**/generated/**
📒 Files selected for processing (17)
core/analytics/src/main/java/com/puzzle/analytics/AmplitudeAnalyticsHelper.ktcore/analytics/src/main/java/com/puzzle/analytics/AnalyticsEvent.ktcore/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.ktcore/billing/build.gradle.ktscore/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktdebug/src/main/java/com/puzzle/debug/AnalyticsScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/contact/ContactScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/AcceptMatchingDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/ProfileImageDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/RefuseMatchingDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/component/card/MatchingCard.ktfeature/store/src/main/java/com/puzzle/store/StoreScreen.ktfeature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.ktfeature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (1)
core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt (1)
TrackScreenViewEvent(86-109)
core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt (2)
core/analytics/src/main/java/com/puzzle/analytics/AmplitudeAnalyticsHelper.kt (1)
logEvent(11-16)core/analytics/src/main/java/com/puzzle/analytics/DebugAnalyticsHelper.kt (1)
logEvent(17-21)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/RefuseMatchingDialog.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (5)
PieceDialog(33-58)PieceDialogDefaultTop(60-84)PieceDialogDefaultTop(86-110)PieceDialogDefaultTop(112-136)PieceDialogDefaultTop(138-162)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (3)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.kt (2)
PurchaseDialog(19-82)InsufficientPuzzleDialog(84-119)feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
MatchingPoolEmptyDialog(14-36)core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt (1)
TrackScreenViewEvent(86-109)
⏰ 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 (15)
core/billing/build.gradle.kts (1)
14-14: LGTM! The analytics dependency addition is necessary to support the revenue tracking integration in BillingHelperImpl. Theimplementationscope is appropriate for internal usage, and no circular dependency exists between core/billing and core/analytics.feature/store/src/main/java/com/puzzle/store/StoreScreen.kt (1)
103-106: LGTM!The screen view tracking is correctly placed and uses
key = Unitto ensure it fires once per screen instance. The implementation aligns with the analytics pattern established in the codebase.feature/matching/src/main/java/com/puzzle/matching/graph/contact/ContactScreen.kt (1)
137-142: LGTM!The analytics tracking for profile image clicks is correctly implemented. The event is tracked immediately when the image is clicked, and the naming convention is clear and consistent.
feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.kt (1)
37-44: LGTM - Analytics tracking correctly integrated.The click handler properly invokes the purchase callback first, then tracks the analytics event. The pattern is consistent with other analytics integrations in this PR.
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/RefuseMatchingDialog.kt (1)
23-56: LGTM - Analytics instrumentation properly integrated.The analytics helper is correctly obtained from composition local, click events are tracked after the action callback, and screen view is tracked appropriately. The dialog title composition update with the suffix string resource is properly structured.
core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt (2)
60-72: LGTM - New revenue tracking method well-structured.The
trackRevenueEventmethod is clean with sensible defaults (quantity = 1). It properly creates aRevenueevent and delegates tologEventfor dispatch.
102-108: LGTM - Event construction migrated to type-safe Track variant.The migration from generic
AnalyticsEventtoAnalyticsEvent.Trackis consistent throughout the file and aligns with the new sealed interface design.feature/matching/src/main/java/com/puzzle/matching/graph/main/component/card/MatchingCard.kt (2)
140-154: LGTM - Dynamic button naming for analytics.The labeled block pattern with early return for unmatched states is a clean approach to conditionally track analytics only for known match status/type combinations.
159-170: Verify: Analytics tracking removed for collapsed card clicks.The
CollapsedContentonClick previously tracked a "collapsed_match_button" event but now only invokesonCardClick. Confirm this intentional removal aligns with analytics requirements.feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
136-145: LGTM! Centralized screen tracking is a good pattern.The screen-level
TrackScreenViewEventwith a keyedcurrentPageensures consistent analytics tracking across page transitions.feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.kt (2)
36-41: LGTM! Button name mapping for analytics is clear and consistent.The
whenexpression cleanly maps eachPurchaseTypeto its corresponding analytics button name.
84-119: Hardcoded screen name inTrackScreenViewEventmay conflict withentryRouteForLog.Line 118 uses a hardcoded
"match_main_accept_popup"forTrackScreenViewEvent, but this dialog can be opened from different screens (e.g.,match_detail_basic_profile,match_detail_value_pick). TheentryRouteForLogparameter is only used for button click tracking, not for the screen view event. Consider whether this is intentional or if the screen view should also reflect the entry route.core/analytics/src/main/java/com/puzzle/analytics/AnalyticsEvent.kt (1)
24-28: LGTM! Revenue event structure is well-defined.The
Revenuedata class with nullableproductId,price, andquantityprovides a clean model for revenue tracking events.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (2)
178-180: LGTM! MatchingPoolEmpty dialog wiring is correct.The new
MatchingPoolEmptybranch correctly routes toMatchingPoolEmptyDialogand wiresonNextTryClickto trigger the accept action, enabling retry flow from the pool-empty state.
139-144: LGTM! Centralized screenName computation.The
screenNamemapping based onmatchingStatusprovides a single source of truth for analytics screen names used throughout the composable.
| private fun AnalyticsEvent.Revenue.toRevenue(): Revenue = Revenue().apply { | ||
| productId = this.productId | ||
| price = this.price | ||
| quantity = this.quantity | ||
| } |
There was a problem hiding this comment.
Critical: this shadowing causes self-assignment instead of mapping from event.
Inside the apply block, this refers to the new com.amplitude.android.events.Revenue instance being built, not the AnalyticsEvent.Revenue receiver. The assignments productId = this.productId result in self-assignment (no-op), leaving the Amplitude Revenue object with default/null values.
🔎 Proposed fix using qualified `this` reference
- private fun AnalyticsEvent.Revenue.toRevenue(): Revenue = Revenue().apply {
- productId = this.productId
- price = this.price
- quantity = this.quantity
- }
+ private fun AnalyticsEvent.Revenue.toRevenue(): Revenue = Revenue().apply {
+ productId = this@toRevenue.productId
+ price = this@toRevenue.price
+ quantity = this@toRevenue.quantity
+ }🤖 Prompt for AI Agents
In core/analytics/src/main/java/com/puzzle/analytics/AmplitudeAnalyticsHelper.kt
around lines 27-31, the apply block assigns fields from the newly created
Revenue instance (shadowing `this`) instead of the AnalyticsEvent.Revenue
receiver, resulting in no data being copied; fix by referencing the extension
receiver when assigning (e.g., use this@toRevenue.productId /
this@toRevenue.price / this@toRevenue.quantity) or capture the source in a local
val (val src = this) before apply and assign from src.productId, src.price,
src.quantity so the Amplitude Revenue object is populated correctly.
| if (verifyResult.rewardPuzzleCount > 0){ | ||
| analyticsHelper.trackRevenueEvent( | ||
| productId = productId, | ||
| price = verifyResult.rewardPuzzleCount.toDouble(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and find the BillingHelperImpl.kt file
find . -name "BillingHelperImpl.kt" -type f 2>/dev/null | head -5Repository: Piece-Puzzly/Piece-Android
Length of output: 143
🏁 Script executed:
# Look at the BillingHelperImpl.kt file around the flagged lines
git ls-files | grep -i billinghelperimplRepository: Piece-Puzzly/Piece-Android
Length of output: 141
🏁 Script executed:
# Search for trackRevenueEvent definition and usage
rg -n "trackRevenueEvent" --type=kotlin -B 2 -A 5Repository: Piece-Puzzly/Piece-Android
Length of output: 1654
🏁 Script executed:
# Search for price-related information in the billing context
rg -n "priceAmountMicros|formattedPrice|price.*Double|monetaryPrice|purchasePrice" --type=kotlin -B 1 -A 1Repository: Piece-Puzzly/Piece-Android
Length of output: 2383
🏁 Script executed:
# Read the BillingHelperImpl.kt file to understand the context around lines 116-121
cat -n core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt | sed -n '100,140p'Repository: Piece-Puzzly/Piece-Android
Length of output: 1661
🏁 Script executed:
# Search for verifyResult definition and where it comes from
rg -n "verifyResult|VerifyResult" --type=kotlin -B 2 -A 2 | head -60Repository: Piece-Puzzly/Piece-Android
Length of output: 1889
🏁 Script executed:
# Look for the function signature that contains this code
ast-grep --pattern 'fun $FUNC($$$verifyResult$$$) {
$$$
trackRevenueEvent($$$)
$$$
}'Repository: Piece-Puzzly/Piece-Android
Length of output: 52
🏁 Script executed:
# Check if product price information is accessible in the function
rg -n "fun.*purchase\|fun.*verify" --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt -A 20 | head -80Repository: Piece-Puzzly/Piece-Android
Length of output: 52
🏁 Script executed:
# Search for Purchase class definition and its properties
rg -n "class Purchase|data class Purchase" --type=kotlin -A 20Repository: Piece-Puzzly/Piece-Android
Length of output: 2743
🏁 Script executed:
# Check what properties are available on the purchase object
rg -n "purchase\." --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 281
🏁 Script executed:
# Look for PurchaseProduct or verifyResult structure
rg -n "data class.*VerifyResult|data class.*PurchaseProduct" --type=kotlin -A 10Repository: Piece-Puzzly/Piece-Android
Length of output: 3965
🏁 Script executed:
# Search for how price information should be obtained from purchase details
rg -n "priceAmountMicros|originalPrice|originalJson" --type=kotlin -B 2 -A 2 | head -50Repository: Piece-Puzzly/Piece-Android
Length of output: 1261
🏁 Script executed:
# Check what properties are available on Google Play's Purchase object
# Look for how Purchase properties are used in the file
rg -n "purchase\.[a-zA-Z]+" --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt -o | sort | uniqRepository: Piece-Puzzly/Piece-Android
Length of output: 144
🏁 Script executed:
# Search for Google Play Billing library imports and Purchase usage
rg -n "import.*Purchase|class Purchase|com\.android\.billingclient" --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 824
🏁 Script executed:
# Check if there's documentation or usage of priceAmountMicros from Purchase object
rg -n "priceAmountMicros" --type=kotlin -B 3 -A 3 core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 540
🏁 Script executed:
# Verify what the trackRevenueEvent is actually meant to track in Amplitude
rg -n "trackRevenueEvent|Revenue" --type=kotlin core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt -B 2 -A 15 | head -40Repository: Piece-Puzzly/Piece-Android
Length of output: 652
🏁 Script executed:
# Check all usages of trackRevenueEvent to understand the pattern
rg -n "trackRevenueEvent" --type=kotlin -B 5 -A 2Repository: Piece-Puzzly/Piece-Android
Length of output: 1654
🏁 Script executed:
# Check if Purchase object has price information available
# Look at the full fetchPurchaseReward function to confirm what properties are accessible
cat -n core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt | sed -n '104,125p'Repository: Piece-Puzzly/Piece-Android
Length of output: 945
🏁 Script executed:
# Search for how other parts of the codebase handle revenue tracking
rg -n "Revenue|revenue" --type=kotlin core/analytics/src/main/java/com/puzzle/analytics/ -B 1 -A 1Repository: Piece-Puzzly/Piece-Android
Length of output: 2337
🏁 Script executed:
# Check if ProductDetails or pricing information is available in the context
# Look at how products are queried and if price is accessible
rg -n "queryProductDetails|ProductDetails" --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt -B 2 -A 5Repository: Piece-Puzzly/Piece-Android
Length of output: 2420
🏁 Script executed:
# Check the full Purchase flow to understand what information is available
cat -n core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt | sed -n '60,110p'Repository: Piece-Puzzly/Piece-Android
Length of output: 2689
🏁 Script executed:
# Look for where purchases are processed and what data flows through
rg -n "purchasesUpdatedListener|handlePurchase" --type=kotlin core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt -B 2 -A 10Repository: Piece-Puzzly/Piece-Android
Length of output: 1262
🏁 Script executed:
# Let's check the Google Play Purchase object to see if price information is available
rg -n "originalJson|priceAmountMicros|originalPrice" --type=kotlin -C 2Repository: Piece-Puzzly/Piece-Android
Length of output: 1261
🏁 Script executed:
# Check the AnalyticsEvent.Revenue and Amplitude Revenue to understand what price field means
cat -n core/analytics/src/main/java/com/puzzle/analytics/AnalyticsEvent.kt | sed -n '20,35p'Repository: Piece-Puzzly/Piece-Android
Length of output: 340
🏁 Script executed:
# Check AmplitudeAnalyticsHelper to see how revenue is processed
cat -n core/analytics/src/main/java/com/puzzle/analytics/AmplitudeAnalyticsHelper.kt | sed -n '1,35p'Repository: Piece-Puzzly/Piece-Android
Length of output: 1304
Revenue tracking uses puzzle count instead of actual monetary price.
The trackRevenueEvent is designed to track monetary revenue, but price is set to rewardPuzzleCount.toDouble() (puzzle count), not the actual purchase price. This will result in incorrect revenue analytics in Amplitude.
Extract the actual monetary price from the purchase object (available via Google Play Billing Library) or pass product price information to this function instead.
🤖 Prompt for AI Agents
In core/billing/src/main/java/com/puzzle/billing/data/BillingHelperImpl.kt
around lines 116-121, the code currently passes
verifyResult.rewardPuzzleCount.toDouble() as the monetary price to
analyticsHelper.trackRevenueEvent; replace this with the actual monetary price
from the purchase/product metadata: extract the price (and currency if required)
from the Google Play Billing purchase or the ProductDetails provided by the
billing response (or add a price parameter to this method and propagate it from
the caller), convert that localized price/amount to a numeric monetary value,
and pass that numeric price into trackRevenueEvent instead of
rewardPuzzleCount.toDouble().
| val onNewMatchingCardClick = { | ||
| onNewMatchingCardClick() | ||
|
|
||
| val buttonName = if (state.canFreeMatch) "new_matching_free" else "new_matching_purchase" | ||
|
|
||
| analyticsHelper.trackClickEvent( | ||
| screenName = screenName, | ||
| buttonName = buttonName, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid shadowing the parameter with a local variable of the same name.
The local val onNewMatchingCardClick shadows the function parameter onNewMatchingCardClick: () -> Unit, which is confusing and error-prone. Rename the local lambda to something distinct like handleNewMatchingCardClick or onNewMatchingCardClickWithAnalytics.
🔎 Proposed fix
- val onNewMatchingCardClick = {
+ val handleNewMatchingCardClick = {
onNewMatchingCardClick()
val buttonName = if (state.canFreeMatch) "new_matching_free" else "new_matching_purchase"
analyticsHelper.trackClickEvent(
screenName = screenName,
buttonName = buttonName,
)
}Then update usages at lines 245 and 255:
- onNewMatchingCardClick = onNewMatchingCardClick,
+ onNewMatchingCardClick = handleNewMatchingCardClick,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
around lines 146 to 155, the local val onNewMatchingCardClick shadows the
function parameter of the same name; rename the local lambda to a distinct
identifier (e.g., handleNewMatchingCardClick or
onNewMatchingCardClickWithAnalytics), keep its body calling the original
parameter and performing analytics, and update all references/usages (notably at
lines 245 and 255) to use the new local name.
| analyticsHelper.trackClickEvent( | ||
| screenName = "store_main", | ||
| buttonName = "store_main_banner_${product.name}_${product.price}", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check analytics button naming in PromotionProductList
rg -n -A2 -B2 "trackClickEvent" feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Also check the full context in NormalProductList around the analytics call
rg -n -A5 -B5 "trackClickEvent" feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 547
🏁 Script executed:
#!/bin/bash
# Search for all trackClickEvent calls to understand naming patterns
rg -n "trackClickEvent" feature/store/src/main/java/com/puzzle/store/ui/list/Repository: Piece-Puzzly/Piece-Android
Length of output: 336
🏁 Script executed:
#!/bin/bash
# Examine NormalProductList and PromotionProductList to understand the distinction
head -50 feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 1779
🏁 Script executed:
#!/bin/bash
# Check PromotionProductList context
head -50 feature/store/src/main/java/com/puzzle/store/ui/list/PromotionProductList.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 1721
🏁 Script executed:
#!/bin/bash
# Search for analytics naming conventions or documentation
rg -i "analytics|button.*name|naming.*convention" feature/store/ -l | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 280
Reconsider button name semantics for NormalProductList analytics.
The button name uses "store_main_banner_..." but NormalProductList renders card components, not banners. Compare with PromotionProductList, which also uses "store_main_banner" for actual image banners—this creates semantic confusion. Consider renaming to "store_main_product_..." to accurately reflect the component type and align with analytics naming clarity.
Additionally, embedding product.name and product.price directly into the button name may cause inconsistencies if these values contain special characters or whitespace, whereas PromotionProductList uses static naming. Align the naming strategy across both list types.
🤖 Prompt for AI Agents
In feature/store/src/main/java/com/puzzle/store/ui/list/NormalProductList.kt
around lines 41 to 44, the analytics buttonName currently uses
"store_main_banner_${product.name}_${product.price}" which is semantically
incorrect for a product card and embeds raw product.name/product.price; change
the buttonName to use a product-specific but stable identifier and correct
component type (for example "store_main_product_${product.id}" or simply
"store_main_product" if no id available) to match PromotionProductList naming
conventions, and stop interpolating raw name/price (or sanitize them) to avoid
special-character/whitespace issues.
* [PC-1534] 매칭 풀 다이얼로그(기본) show/hide 로직 구현 * [PC-1534] 매칭 풀 다이얼로그 show/hide 로직 수정 * [PC-1534] showPurchaseDialog -> showDialog * [PC-1534] MatchingPoolType -> MatchingPoolEmptyType * [PC-1534] isModalShownToday -> isBasicMatchingPoolEmptyDialogShownToday * [PC-1534] getStandardDate -> getResetDateString * [PC-1534] getResetDateString 함수 테스트 코드 작성 * [PC-1534] LastModal -> LastMatchingPoolEmptyDialog * [PC-1534] 로그아웃, 탈퇴 시 매칭 풀 소진 다이얼로그 clear * [PC-1534] LastModal -> LastMatchingPoolEmptyDialog * [PC-1534] LastModal -> LastMatchingPoolEmptyDialog * [PC-1534] 로그인, 로그아웃 테스트 코드에 lastMatchingPoolEmptyDialogShownDate 빈 값 확인 코드 추가 * [PC-1534] 매칭 풀 소진 다이얼로그(기본) show/hide 테스트 코드 추가 * [PC-1534] 매칭 풀 소진 다이얼로그(기본) subText 수정 * [PC-1534] 매칭 풀 소진 다이얼로그 표출 확인 테스트 코드 작성 * [PC-1534] 매칭 풀 소진 다이얼로그 본 날짜 토큰 만료시 제거하도록 변경 * [PC-1534] 맛보기 매칭 풀 소진일 때는 Basic 매칭 풀 소진 표출되도록 변경 * [PC-1534] lastMatchingPoolEmptyDialogShownDate 타입 non-null로 변경
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
146-155: Parameter shadowing still present.The local
val onNewMatchingCardClickshadows the function parameter of the same name. This was flagged in a previous review and should be renamed to something distinct likehandleNewMatchingCardClick.🔎 Proposed fix
- val onNewMatchingCardClick = { - onNewMatchingCardClick() + val handleNewMatchingCardClick = { + onNewMatchingCardClick() val buttonName = if (state.canFreeMatch) "new_matching_free" else "new_matching_purchase" analyticsHelper.trackClickEvent( screenName = screenName, buttonName = buttonName, ) }Then update usages at lines 252 and 262:
- onNewMatchingCardClick = onNewMatchingCardClick, + onNewMatchingCardClick = handleNewMatchingCardClick,
🧹 Nitpick comments (8)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
356-381: Consider awaitingfetchPuzzleCount()before showing the dialog.In
NOT_ENOUGH_PUZZLEhandling,fetchPuzzleCount()launches a coroutine but the dialog is shown immediately after. If the dialog displays the puzzle count, users might briefly see stale data.🔎 Proposed fix
PieceErrorCode.NOT_ENOUGH_PUZZLE -> { - fetchPuzzleCount() + fetchPuzzleCount().join() showDialog(MatchingDialogType.InSufficient) }Alternatively, if the dialog doesn't depend on the updated count, document this as intentional fire-and-forget behavior.
core/data/src/test/java/com/puzzle/data/repository/AuthRepositoryImplTest.kt (1)
70-70: LGTM! Consider adding positive test cases for the setter.The assertions correctly verify that the dialog date is cleared during logout and withdrawal flows.
Consider adding test cases that verify
setLastMatchingPoolEmptyDialogShownDateactually persists the date value before clearing. This would provide more comprehensive coverage of the dialog date lifecycle.Also applies to: 85-85
core/data/src/main/java/com/puzzle/data/repository/AuthRepositoryImpl.kt (1)
47-57: Consider aligning job join order with launch order for clarity.The implementation correctly clears the dialog date during logout and withdrawal. The parallel launch with sequential join pattern is appropriate.
For consistency and maintainability, consider joining jobs in the same order they're launched:
🔎 Proposed refactor for consistent ordering
override suspend fun logout() { coroutineScope { val clearUserRoleJob = launch { localUserDataSource.clearUserRole() } val clearLastMatchingPoolEmptyDialogShownDateJob = launch { localUserDataSource.clearLastMatchingPoolEmptyDialogShownDate() } val clearTokenJob = launch { localTokenDataSource.clearToken() } - clearTokenJob.join() + clearUserRoleJob.join() clearLastMatchingPoolEmptyDialogShownDateJob.join() - clearUserRoleJob.join() + clearTokenJob.join() } }Apply the same pattern to
withdraw()for consistency.Also applies to: 59-71
core/datastore/src/main/java/com/puzzle/datastore/datasource/user/LocalUserDataSource.kt (1)
7-7: Consider documenting the expected date string format.The API surface is clean and follows the existing pattern for
userRole.Since dates are stored as strings, consider adding KDoc to specify the expected format (e.g., "yyyy-MM-dd" or ISO 8601). This would prevent inconsistent formatting across the codebase.
Example:
/** * Flow of the last date the matching pool empty dialog was shown. * Format: yyyy-MM-dd (e.g., "2025-12-28") */ val lastMatchingPoolEmptyDialogShownDate: Flow<String> /** * Sets the last date the matching pool empty dialog was shown. * @param dateString Date in yyyy-MM-dd format */ suspend fun setLastMatchingPoolEmptyDialogShownDate(dateString: String)Also applies to: 10-11
core/datastore/src/main/java/com/puzzle/datastore/datasource/user/LocalUserDataSourceImpl.kt (1)
37-37: Consider renaming the DataStore key to match the semantic purpose.The key name
LAST_MODAL_SHOWN_DATEis generic and doesn't reflect that it's specifically for the "matching pool empty dialog." Consider renaming toLAST_MATCHING_POOL_EMPTY_DIALOG_SHOWN_DATEfor clarity.🔎 Proposed naming improvement
companion object { private val USER_ROLE = stringPreferencesKey("USER_ROLE") - private val LAST_MODAL_SHOWN_DATE = stringPreferencesKey("LAST_MODAL_SHOWN_DATE") + private val LAST_MATCHING_POOL_EMPTY_DIALOG_SHOWN_DATE = stringPreferencesKey("LAST_MATCHING_POOL_EMPTY_DIALOG_SHOWN_DATE") }And update references on lines 17, 28, and 32.
Note: Changing the key name will reset any existing stored values, so coordinate this with your migration strategy if users already have this data persisted.
core/testing/src/main/java/com/puzzle/testing/domain/repository/FakeUserRepository.kt (1)
71-79: Consider usingMutableStateFlowfor reactive state in test fake.The current implementation uses
flowOf(), which emits the current value once and completes. If a test collects the flow and then callssetLastMatchingPoolEmptyDialogShownDate(), the collector won't receive the updated value.For a more robust test fake that supports reactive updates:
🔎 Proposed refactor using MutableStateFlow
- private var lastMatchingPoolEmptyDialogShownDate: String = "" + private val lastMatchingPoolEmptyDialogShownDateFlow = MutableStateFlow("")- override fun getLastMatchingPoolEmptyDialogShownDate(): Flow<String> = flowOf(lastMatchingPoolEmptyDialogShownDate) + override fun getLastMatchingPoolEmptyDialogShownDate(): Flow<String> = lastMatchingPoolEmptyDialogShownDateFlow override suspend fun setLastMatchingPoolEmptyDialogShownDate(dateString: String) { - this.lastMatchingPoolEmptyDialogShownDate = dateString + lastMatchingPoolEmptyDialogShownDateFlow.value = dateString } override suspend fun clearLastMatchingPoolEmptyDialogShownDate() { - this.lastMatchingPoolEmptyDialogShownDate = "" + lastMatchingPoolEmptyDialogShownDateFlow.value = "" }core/data/src/test/java/com/puzzle/data/source/user/FakeLocalUserDataSource.kt (2)
12-12: Naming inconsistency:currentLastModalShownDatevsMatchingPoolEmptyDialog.The private field uses the generic term "Modal" while the public property is specifically for "MatchingPoolEmptyDialog". Consider renaming to
currentLastMatchingPoolEmptyDialogShownDatefor consistency and clarity.🔎 Proposed naming fix
- private var currentLastModalShownDate = "" + private var currentLastMatchingPoolEmptyDialogShownDate = "" override val lastMatchingPoolEmptyDialogShownDate: Flow<String> get() = flowOf(currentLastMatchingPoolEmptyDialogShownDate)Update lines 24 and 28 accordingly:
override suspend fun setLastMatchingPoolEmptyDialogShownDate(dateString: String) { - currentLastModalShownDate = dateString + currentLastMatchingPoolEmptyDialogShownDate = dateString } override suspend fun clearLastMatchingPoolEmptyDialogShownDate() { - currentLastModalShownDate = "" + currentLastMatchingPoolEmptyDialogShownDate = "" }
13-13: Consider usingMutableStateFlowfor reactive state.Similar to other test fakes in this PR, using
flowOf()creates a cold flow that emits once and completes. This won't notify collectors of subsequent state changes, which could affect tests that rely on observing updates.🔎 Proposed refactor using MutableStateFlow
- private var currentLastModalShownDate = "" - override val lastMatchingPoolEmptyDialogShownDate: Flow<String> get() = flowOf(currentLastModalShownDate) + private val _lastMatchingPoolEmptyDialogShownDate = MutableStateFlow("") + override val lastMatchingPoolEmptyDialogShownDate: Flow<String> get() = _lastMatchingPoolEmptyDialogShownDateoverride suspend fun setLastMatchingPoolEmptyDialogShownDate(dateString: String) { - currentLastModalShownDate = dateString + _lastMatchingPoolEmptyDialogShownDate.value = dateString } override suspend fun clearLastMatchingPoolEmptyDialogShownDate() { - currentLastModalShownDate = "" + _lastMatchingPoolEmptyDialogShownDate.value = "" }Also applies to: 23-29
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/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/AuthRepositoryImpl.ktcore/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.ktcore/data/src/main/java/com/puzzle/data/repository/UserRepositoryImpl.ktcore/data/src/test/java/com/puzzle/data/repository/AuthRepositoryImplTest.ktcore/data/src/test/java/com/puzzle/data/source/user/FakeLocalUserDataSource.ktcore/datastore/src/main/java/com/puzzle/datastore/datasource/user/LocalUserDataSource.ktcore/datastore/src/main/java/com/puzzle/datastore/datasource/user/LocalUserDataSourceImpl.ktcore/designsystem/src/main/res/values/strings.xmlcore/domain/src/main/java/com/puzzle/domain/repository/UserRepository.ktcore/testing/src/main/java/com/puzzle/testing/domain/repository/FakeUserRepository.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/constant/MatchingDialogType.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.ktpresentation/src/main/java/com/puzzle/presentation/MainViewModel.kt
💤 Files with no reviewable changes (1)
- core/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- feature/matching/src/androidTest/java/com/puzzle/matching/graph/main/MatchingScreenTest.kt
- core/designsystem/src/main/res/values/strings.xml
- feature/matching/src/main/java/com/puzzle/matching/graph/main/constant/MatchingDialogType.kt
🧰 Additional context used
🧬 Code graph analysis (3)
core/common/src/test/kotlin/com/puzzle/common/TimeUtilTest.kt (1)
core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
getResetDateString(112-121)
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (3)
PieceDialog(33-58)PieceDialogIconTop(164-214)PieceDialogOneButtonBottom(243-260)core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (7)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)core/data/src/main/java/com/puzzle/data/repository/MatchingRepositoryImpl.kt (1)
checkMatchingPiece(78-79)core/domain/src/main/java/com/puzzle/domain/repository/MatchingRepository.kt (1)
checkMatchingPiece(19-19)core/network/src/main/java/com/puzzle/network/source/matching/MatchingDataSourceImpl.kt (1)
checkMatchingPiece(61-61)core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
checkMatchingPiece(168-169)core/testing/src/main/java/com/puzzle/testing/domain/repository/SpyMatchingRepository.kt (1)
checkMatchingPiece(77-77)core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
getResetDateString(112-121)
⏰ 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 (15)
core/common/src/test/kotlin/com/puzzle/common/TimeUtilTest.kt (2)
161-175: LGTM! Test correctly validates pre-reset behavior.The test properly verifies that when the current hour is before the daily reset hour (22:00), the function returns the previous day's date. Using a fixed clock ensures deterministic test results.
177-191: LGTM! Test correctly validates at-reset-hour behavior.The test properly verifies the boundary condition where the hour equals the reset hour (22:00). The implementation uses strict less-than comparison, so at exactly 22:00, the current day is returned as expected.
core/common/src/main/java/com/puzzle/common/TimeUtil.kt (3)
3-3: LGTM! Import necessary for testable time-based logic.The Clock import enables dependency injection of time for the new
getResetDateStringfunction, which is a best practice for testing time-dependent behavior.
111-121: LGTM! Clean implementation with good testability.The function correctly implements reset date logic using a 22:00 cutoff. Key strengths:
- Clock injection enables deterministic testing
- Hour-based comparison provides clear boundary semantics
- Thread-safe and stateless
- Returns standard ISO date format
The implementation only checks the hour field (not minutes/seconds), so 21:59:59 and 21:00:00 both return the previous day, while 22:00:00 and 23:59:59 both return the current day. This simplification is appropriate for daily reset logic.
123-123: LGTM! Constant appropriately named and scoped.The
DAILY_RESET_HOURconstant is well-defined at 22:00 (10 PM KST), making the reset logic configurable and clear. Top-level placement enables usage in both production code and tests.feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt (1)
15-44: LGTM!The dialog implementation correctly uses destructuring for
subTextandlabelbased onmatchingPoolEmptyType, and properly wires thePieceDialogcomponents. The emptyonDismissRequestenforces user interaction through the button.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
77-79: LGTM!Good initialization pattern to check the daily dialog status on ViewModel creation, ensuring the state is ready before any user interactions.
258-281: LGTM!The
getInstantNewMatchfunction now correctly handles theisFreeparameter and shows appropriate dialogs based on the error type. The snackbar only shows when a puzzle is consumed (!isFree), which addresses the previous review concern about the unreachable condition.
392-401: Defensive handling on failure is good.Setting
isBasicMatchingPoolEmptyDialogShownToday = trueon failure is a safe default that prevents dialog spam if the data source fails. The.first()usage is acceptable here since the flow from DataStore should always emit at least one value.
403-416: LGTM!The
handleMatchingPoolEmptyDialogfunction correctly gates showing the dialog based on both the daily check and the presence of BASIC matches in the source list. The flag management ensures this check runs only once per fetch cycle.feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.kt (1)
28-28: Default value prevents premature dialog display.Setting
isBasicMatchingPoolEmptyDialogShownToday = trueas default is a good defensive choice—it prevents the dialog from flashing before the actual check from DataStore completes.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (2)
178-187: LGTM!The new
MatchingPoolEmptydialog branch correctly wires theMatchingPoolEmptyDialogcomponent with the appropriate type and analytics tracking. TheonNextTryClickcallback properly delegates toonAcceptClickwith the current dialog type.
139-144: Good analytics mapping.The
screenNamecomputation provides clear, descriptive screen names for each matching status, improving analytics clarity.core/datastore/src/main/java/com/puzzle/datastore/datasource/user/LocalUserDataSourceImpl.kt (1)
17-17: LGTM! Implementation follows established patterns.The implementation correctly uses DataStore utilities (
getValue,setValue,clear) and follows the same pattern as the existinguserRoleimplementation.Also applies to: 27-33
core/domain/src/main/java/com/puzzle/domain/repository/UserRepository.kt (1)
21-23: LGTM! Clean domain API for dialog date persistence.The new methods provide a clear interface for managing the matching pool empty dialog state and follow the repository pattern consistently.
| override fun getLastMatchingPoolEmptyDialogShownDate(): Flow<String> = | ||
| localUserDataSource.lastMatchingPoolEmptyDialogShownDate | ||
|
|
||
| override suspend fun setLastMatchingPoolEmptyDialogShownDate(dateString: String) { | ||
| localUserDataSource.setLastMatchingPoolEmptyDialogShownDate(dateString) | ||
| } | ||
|
|
||
| override suspend fun clearLastMatchingPoolEmptyDialogShownDate() { | ||
| localUserDataSource.clearLastMatchingPoolEmptyDialogShownDate() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "UserRepositoryImpl.kt" | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 144
🏁 Script executed:
git ls-files | grep -E "(UserRepository|DataSource)" | grep -i kotlinRepository: Piece-Puzzly/Piece-Android
Length of output: 52
🏁 Script executed:
rg -nP --type=kotlin 'getResetDateString|lastMatchingPoolEmptyDialogShownDate' -B2 -A5Repository: Piece-Puzzly/Piece-Android
Length of output: 14580
🏁 Script executed:
rg -nP --type=kotlin 'DateTimeFormatter|SimpleDateFormat|LocalDate|Instant' -C2 | head -100Repository: Piece-Puzzly/Piece-Android
Length of output: 10370
🏁 Script executed:
cat -n core/common/src/main/java/com/puzzle/common/TimeUtil.kt | sed -n '112,130p'Repository: Piece-Puzzly/Piece-Android
Length of output: 670
🏁 Script executed:
rg -nP --type=kotlin 'fun setLastMatchingPoolEmptyDialogShownDate|fun clearLastMatchingPoolEmptyDialogShownDate' -A10 core/datastoreRepository: Piece-Puzzly/Piece-Android
Length of output: 2202
🏁 Script executed:
cat -n core/data/src/main/java/com/puzzle/data/repository/UserRepositoryImpl.kt | sed -n '1,50p'Repository: Piece-Puzzly/Piece-Android
Length of output: 2389
🏁 Script executed:
rg -nP --type=kotlin 'setLastMatchingPoolEmptyDialogShownDate' --no-headingRepository: Piece-Puzzly/Piece-Android
Length of output: 1403
Consider documenting the expected date format.
The date format is standardized to ISO-8601 (yyyy-MM-dd) via getResetDateString(), which uses DateTimeFormatter.ISO_LOCAL_DATE. While this ensures consistency for current usages (all code paths call getResetDateString() before storing), adding a KDoc comment documenting the expected format would clarify the contract for future maintainers and prevent accidental misuse of the public setter.
Optional: Add format validation in the setter if the API needs to be more defensive.
🤖 Prompt for AI Agents
In core/data/src/main/java/com/puzzle/data/repository/UserRepositoryImpl.kt
around lines 34-43, the public setter/getter for
lastMatchingPoolEmptyDialogShownDate lacks documentation of the expected date
string format; add a concise KDoc above the property/each public method stating
the expected ISO-8601 date format ("yyyy-MM-dd",
DateTimeFormatter.ISO_LOCAL_DATE) so callers know the contract, and optionally
add defensive validation in setLastMatchingPoolEmptyDialogShownDate to parse the
input with LocalDate.parse(..., DateTimeFormatter.ISO_LOCAL_DATE) and throw an
IllegalArgumentException (or return/fail gracefully) on parse failure.
| val shouldShowSpecialModal: Boolean = | ||
| userRole == UserRole.USER && | ||
| (matchInfoList.isEmpty() || matchInfoList.none { it.matchType == MatchType.BASIC }) && | ||
| !isBasicMatchingPoolEmptyDialogShownToday |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "shouldShowSpecialModal" --type kotlin -C 2Repository: Piece-Puzzly/Piece-Android
Length of output: 713
Remove unused shouldShowSpecialModal computed property.
The property defined at lines 50-53 is never used anywhere in the codebase and should be removed to eliminate dead code.
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.kt
around lines 50 to 53, remove the unused computed property
shouldShowSpecialModal; delete its declaration and any associated imports or
references (none exist), and run a build/IDE clean to ensure no remaining
references or warnings remain.
| @Preview | ||
| @Composable | ||
| private fun PreviewNewMatchingPoolEmptyDialog() { | ||
| PieceTheme { | ||
| Column { | ||
| MatchingPoolEmptyDialog(MatchingPoolEmptyType.Basic,{}) | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Copy-paste bug: Preview for "New" type uses MatchingPoolEmptyType.Basic.
PreviewNewMatchingPoolEmptyDialog should use MatchingPoolEmptyType.New to properly preview the "New" variant.
🔎 Proposed fix
@Preview
@Composable
private fun PreviewNewMatchingPoolEmptyDialog() {
PieceTheme {
Column {
- MatchingPoolEmptyDialog(MatchingPoolEmptyType.Basic,{})
+ MatchingPoolEmptyDialog(MatchingPoolEmptyType.New) {}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Preview | |
| @Composable | |
| private fun PreviewNewMatchingPoolEmptyDialog() { | |
| PieceTheme { | |
| Column { | |
| MatchingPoolEmptyDialog(MatchingPoolEmptyType.Basic,{}) | |
| } | |
| } | |
| } | |
| @Preview | |
| @Composable | |
| private fun PreviewNewMatchingPoolEmptyDialog() { | |
| PieceTheme { | |
| Column { | |
| MatchingPoolEmptyDialog(MatchingPoolEmptyType.New) {} | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/dialog/MatchingPoolEmptyDialog.kt
around lines 56-64, the PreviewNewMatchingPoolEmptyDialog currently passes
MatchingPoolEmptyType.Basic by mistake; update the call to pass
MatchingPoolEmptyType.New so the preview renders the "New" variant (keep the
existing lambda parameter as-is).
| PieceErrorCode.EXPIRED_REFRESH_TOKEN -> { | ||
| viewModelScope.launch { | ||
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() | ||
|
|
||
| navigationHelper.navigate( | ||
| TopLevelTo(AuthGraphBaseRoute) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for clearLastMatchingPoolEmptyDialogShownDate().
If clearLastMatchingPoolEmptyDialogShownDate() throws an exception, navigation to AuthGraphBaseRoute will never occur, potentially leaving the user in an invalid state. Wrap the clear operation in a runCatching block to ensure navigation proceeds even if the clear operation fails.
🔎 Proposed fix with error handling
PieceErrorCode.EXPIRED_REFRESH_TOKEN -> {
viewModelScope.launch {
- userRepository.clearLastMatchingPoolEmptyDialogShownDate()
+ runCatching {
+ userRepository.clearLastMatchingPoolEmptyDialogShownDate()
+ }
navigationHelper.navigate(
TopLevelTo(AuthGraphBaseRoute)
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PieceErrorCode.EXPIRED_REFRESH_TOKEN -> { | |
| viewModelScope.launch { | |
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() | |
| navigationHelper.navigate( | |
| TopLevelTo(AuthGraphBaseRoute) | |
| ) | |
| } | |
| } | |
| PieceErrorCode.EXPIRED_REFRESH_TOKEN -> { | |
| viewModelScope.launch { | |
| runCatching { | |
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() | |
| } | |
| navigationHelper.navigate( | |
| TopLevelTo(AuthGraphBaseRoute) | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt around
lines 82 to 90, the call to
userRepository.clearLastMatchingPoolEmptyDialogShownDate() can throw and prevent
navigation to AuthGraphBaseRoute; wrap that call in a runCatching { ...
}.onFailure { /* optional logging */ } so that regardless of errors the
navigationHelper.navigate(TopLevelTo(AuthGraphBaseRoute)) runs inside the
viewModelScope.launch block; keep the clear call asynchronous within the
coroutine and avoid rethrowing so navigation always proceeds.
| (exception is IllegalArgumentException || | ||
| (exception is HttpResponseException && exception.status == HttpResponseStatus.Unauthorized) | ||
| ) -> { | ||
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() |
There was a problem hiding this comment.
Add error handling for clearLastMatchingPoolEmptyDialogShownDate().
If clearLastMatchingPoolEmptyDialogShownDate() throws an exception, it will be caught by the onFailure block at line 152, but navigation to OnboardingRoute will not occur, potentially leaving the user in an invalid state. Wrap the clear operation to ensure navigation proceeds even if it fails.
🔎 Proposed fix with error handling
(exception is IllegalArgumentException ||
(exception is HttpResponseException && exception.status == HttpResponseStatus.Unauthorized)
) -> {
- userRepository.clearLastMatchingPoolEmptyDialogShownDate()
+ runCatching {
+ userRepository.clearLastMatchingPoolEmptyDialogShownDate()
+ }
navigationHelper.navigate(TopLevelTo(route = OnboardingRoute))
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() | |
| runCatching { | |
| userRepository.clearLastMatchingPoolEmptyDialogShownDate() | |
| } |
🤖 Prompt for AI Agents
In presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt around
line 147, the call to userRepository.clearLastMatchingPoolEmptyDialogShownDate()
can throw and prevent subsequent navigation in the onFailure path; wrap this
call in a try/catch (or use runCatching { ... }.onFailure { /* log */ }) so that
any exception is caught and logged/ignored, ensuring the navigation to
OnboardingRoute still executes regardless of
clearLastMatchingPoolEmptyDialogShownDate() failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
356-363: Consider awaiting puzzle count fetch before showing dialog.Line 361 launches
fetchPuzzleCount()asynchronously but doesn't await the result before showing the InSufficient dialog on line 362. This creates a race condition where the dialog may display stale puzzle count data.Consider awaiting the fetch or documenting this intentional behavior if immediate feedback is preferred.
🔎 Proposed fix to await puzzle count
PieceErrorCode.NOT_ENOUGH_PUZZLE -> { - fetchPuzzleCount() + fetchPuzzleCount().join() showDialog(MatchingDialogType.InSufficient) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-40)core/common/src/main/java/com/puzzle/common/TimeUtil.kt (1)
getResetDateString(112-121)
⏰ 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 (5)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (5)
11-11: LGTM!The new imports and instance variables correctly support the daily dialog gating feature.
Also applies to: 32-32, 52-52, 71-72
254-281: LGTM!The
isFreeparameter correctly tracks puzzle usage, the snackbar is shown only when a puzzle is consumed, and error handling appropriately branches based on the match type.
287-289: LGTM!The dialog confirmation logic correctly calls
getInstantNewMatch(false)when the user confirms they want to use a puzzle, and appropriately dismisses the MatchingPoolEmpty dialog.
383-390: LGTM!The date comparison logic correctly resets the dialog check flag when crossing the daily reset boundary.
403-416: Daily gating logic is well-implemented.The
handleMatchingPoolEmptyDialogfunction correctly gates the dialog display to once per day using the reset date and persists the shown date to datastore.Note: The issue with using the unfiltered list was already flagged in an earlier comment.
| }.onSuccess { originalList -> | ||
|
|
||
| handleMatchingPoolEmptyDialog(originalList) | ||
|
|
||
| val filteredList = originalList.filterNot { | ||
| it.matchStatus == MatchStatus.REFUSED || it.isBlocked | ||
| } |
There was a problem hiding this comment.
Use filtered list for pool-empty dialog check.
The function passes originalList to handleMatchingPoolEmptyDialog at line 159, but this list includes REFUSED and blocked matches. The dialog logic at line 406 checks whether any BASIC matches exist, but REFUSED/blocked BASIC matches won't be visible to the user after filtering (lines 161-163).
This could prevent the dialog from showing when the user actually has no visible BASIC matches.
🔎 Proposed fix
Call handleMatchingPoolEmptyDialog with the filtered list instead:
- }.onSuccess { originalList ->
-
- handleMatchingPoolEmptyDialog(originalList)
-
- val filteredList = originalList.filterNot {
+ }.onSuccess { originalList ->
+ val filteredList = originalList.filterNot {
it.matchStatus == MatchStatus.REFUSED || it.isBlocked
}
+
+ handleMatchingPoolEmptyDialog(filteredList)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }.onSuccess { originalList -> | |
| handleMatchingPoolEmptyDialog(originalList) | |
| val filteredList = originalList.filterNot { | |
| it.matchStatus == MatchStatus.REFUSED || it.isBlocked | |
| } | |
| }.onSuccess { originalList -> | |
| val filteredList = originalList.filterNot { | |
| it.matchStatus == MatchStatus.REFUSED || it.isBlocked | |
| } | |
| handleMatchingPoolEmptyDialog(filteredList) |
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
around lines 157-163, the code calls handleMatchingPoolEmptyDialog(originalList)
before filtering out REFUSED and blocked entries, so the dialog check uses items
the user won't see; change the call to pass the filtered list instead (i.e.,
compute filteredList first by excluding REFUSED and blocked items, then call
handleMatchingPoolEmptyDialog(filteredList)) so the pool-empty dialog reflects
the actual visible BASIC matches.
| private fun checkTodayMatchingPoolEmptyDialogStatus() = viewModelScope.launch(dispatcher) { | ||
| suspendRunCatching { | ||
| userRepository.getLastMatchingPoolEmptyDialogShownDate().first() | ||
| }.onSuccess { lastDate -> | ||
| val currentDate = getResetDateString() | ||
| setState { copy(isBasicMatchingPoolEmptyDialogShownToday = lastDate == currentDate) } | ||
| }.onFailure { | ||
| setState { copy(isBasicMatchingPoolEmptyDialogShownToday = true) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MatchingViewModel.kt" -type fRepository: Piece-Puzzly/Piece-Android
Length of output: 154
🏁 Script executed:
cat -n ./feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt | head -100Repository: Piece-Puzzly/Piece-Android
Length of output: 5509
🏁 Script executed:
cat -n ./feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt | sed -n '385,410p'Repository: Piece-Puzzly/Piece-Android
Length of output: 1375
🏁 Script executed:
cat -n ./feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt | sed -n '403,420p'Repository: Piece-Puzzly/Piece-Android
Length of output: 1029
🏁 Script executed:
rg "isBasicMatchingPoolEmptyDialogShownToday" ./feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt -B 2 -A 2Repository: Piece-Puzzly/Piece-Android
Length of output: 861
The defensive default silently suppresses the dialog when datastore access fails.
On line 399, when reading the last shown date fails, isBasicMatchingPoolEmptyDialogShownToday defaults to true, which prevents the dialog from appearing. The dialog only shows when this flag is false (line 408), so an error causes silent suppression rather than fallback display or retry.
While this prevents potential dialog spam on repeated errors, it also means users won't see the dialog if the datastore has a persistent issue, hiding functionality without notification.
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
around lines 392-401, the onFailure block currently sets
isBasicMatchingPoolEmptyDialogShownToday = true which suppresses the dialog on
datastore errors; change the failure handling to set
isBasicMatchingPoolEmptyDialogShownToday = false so the dialog will display as a
safe default, and add logging/reporting of the exception (e.g., processLogger or
Timber) so the error is visible for debugging; optionally consider adding a
lightweight retry or a metric but at minimum flip the boolean on failure and log
the error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.kt (1)
132-150: Consider removing or reviving the commented-out test.This commented-out test appears to cover error handling for user not-found scenarios. Given that the PR adds error exception handling for the MatchingScreen, this test scenario may still be relevant.
Options:
- Remove: If the scenario is no longer applicable, delete the dead code.
- Revive: If the scenario is still valid, update it to use current APIs (
setMatchInfoListResult,fetchMatchInfo, etc.) and uncomment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.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 (2)
feature/matching/src/test/java/com/puzzle/matching/graph/main/MatchingViewModelTest.kt (2)
71-73: LGTM!The test setup correctly reflects the updated API (
setMatchInfoListResult), and the comment clearly explains the test intent.
84-130: Well-structured parameterized test with comprehensive coverage.The test improvements are solid:
- Explicit user info setup improves readability
advanceUntilIdle()ensures coroutine completion before assertions- Descriptive assertion message aids debugging
- Coverage of all
MatchTypevariants (BASIC, AUTO, TRIAL, PREMIUM) is thoroughOne consideration: the logic at line 123 assumes only
REFUSEDprevents timer starts. Verify this matches the actual business logic—if otherMatchStatusvalues (e.g.,EXPIRED,MATCHED) should also skip timers, the test expectation would need adjustment.
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.