[PC-1485] 디자인관련 이슈 수정(스낵바, 카드) #181
Conversation
| private suspend fun getInstantNewMatch() = suspendRunCatching { | ||
| matchingRepository.getNewInstantMatch() | ||
| }.onSuccess { matchId -> | ||
| eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching("퍼즐을 2개 사용했어요"))) |
There was a problem hiding this comment.
PUZZLE_COUNT_NEW_MATCH(2) 이걸로 바꾸는게 좋을 거 같아요
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| modifier = Modifier | ||
| .padding(bottom = snackBarPosition, start = 20.dp, end = 20.dp) |
There was a problem hiding this comment.
bottom padding 없어지면서 스낵바가 Info,TextOnly일 때 좀 낮게 뜨는 거 같은데 확인 부탁드립니다.
| private suspend fun postOpponentContact(matchId: Int) = suspendRunCatching { | ||
| matchingRepository.postOpponentContacts(matchId) | ||
| }.onSuccess { | ||
| eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching("퍼즐을 3개 사용했어요"))) |
There was a problem hiding this comment.
PUZZLE_COUNT_ACCEPT_MATCHING(3) 이걸로 바꾸는게 좋을 거 같아요
|
|
||
| Box( | ||
| modifier = Modifier | ||
| .animateContentSize() |
There was a problem hiding this comment.
줄어들 때 확 줄어들지 않고 각각 애니메이션 주면 좀 더 부드럽게 될 거 같아요
📝 WalkthroughWalkthroughRefactors snackbar side-effect handling to a new SnackBarSideEffect API, removes PieceCardTransitionAnimation, adds pull-to-refresh (PiecePullRefreshBox), introduces dialog dismissal callbacks and OnRefresh intents, adjusts matching UI/state, and moves snackbar host out of the Scaffold. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as MatchingUserScreen
participant Pull as PiecePullRefreshBox
participant VM as MatchingViewModel
participant Side as SideEffectFlow
participant Snack as PieceSnackBarHost
User->>Pull: Pull to refresh
activate Pull
Pull->>UI: onRefresh()
UI->>VM: dispatch(OnRefresh)
activate VM
VM->>VM: refreshMatchingList() (concurrent tasks)
VM-->>Side: emit SnackBarSideEffect (optional)
VM-->>UI: update state (isRefreshing)
deactivate VM
Side->>Snack: ShowSnackBar(ShowSnackBar(SnackBarState.*))
Snack->>User: display snackbar
deactivate Pull
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
⏰ 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)
🔇 Additional comments (5)
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: 0
♻️ Duplicate comments (3)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
46-52: Address past review concern about snackbar positioning.A previous reviewer noted that removing bottom padding caused Info and TextOnly snackbars to appear too low. The new top-padding approach calculates position as
screenHeight - 36.dp - 82.dpfor these states, which should place them 82dp from the bottom. Please verify this positioning in the UI, especially on different screen sizes.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (2)
205-205: Use string resources and constants for puzzle counts.The hardcoded message contains a magic number (2) and isn't localized. As mentioned in previous reviews, consider using a constant like
PUZZLE_COUNT_NEW_MATCHand a string resource with a format parameter.🔎 Suggested approach
Define the constant (if not already present):
private const val PUZZLE_COUNT_NEW_MATCH = 2Add a string resource in
strings.xml:<string name="puzzle_used_message">퍼즐을 %d개 사용했어요</string>Then use:
-eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching("퍼즐을 2개 사용했어요"))) +val message = context.getString(R.string.puzzle_used_message, PUZZLE_COUNT_NEW_MATCH) +eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching(message)))
219-219: Use string resources and constants for puzzle counts.Similar to Line 205, this hardcoded message should use a constant (e.g.,
PUZZLE_COUNT_ACCEPT_MATCHING = 3) and a string resource for proper localization and maintainability.
🧹 Nitpick comments (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
152-165: Consider edge cases in padding calculation.The current calculation
screenHeight - snackBarHeight - snackBarBottomOffsetmay not account for:
- System insets (navigation bar, status bar)
- Different screen sizes and aspect ratios
- Scaffold content that might overlap
Consider extracting the constants to a companion object for better testability and verifying the positioning accounts for safe areas on devices with notches or gesture navigation.
🔎 Optional refactor: Extract constants
+private object SnackBarConstants { + val SNACK_BAR_HEIGHT = 36.dp + val SNACK_BAR_TOP_OFFSET = 112.dp + val SNACK_BAR_BOTTOM_OFFSET = 82.dp +} + @Composable private fun calculateSnackBarTopPadding(state: SnackBarState): Dp { val configuration = LocalConfiguration.current - val snackBarHeight = 36.dp - val snackBarTopOffset = 112.dp - val snackBarBottomOffset = 82.dp - val screenHeight = configuration.screenHeightDp.dp val isMatching = state is SnackBarState.Matching - return if (isMatching) snackBarTopOffset - else screenHeight - snackBarHeight - snackBarBottomOffset + return if (isMatching) SnackBarConstants.SNACK_BAR_TOP_OFFSET + else screenHeight - SnackBarConstants.SNACK_BAR_HEIGHT - SnackBarConstants.SNACK_BAR_BOTTOM_OFFSET }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/common-ui/src/main/java/com/puzzle/common/ui/Animation.kt(0 hunks)core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt(4 hunks)feature/matching/src/androidTest/java/com/puzzle/matching/graph/detail/MatchingDetailScreenTest.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(3 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/component/card/MatchingCard.kt(6 hunks)presentation/src/main/java/com/puzzle/presentation/MainActivity.kt(1 hunks)presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt(3 hunks)
💤 Files with no reviewable changes (1)
- core/common-ui/src/main/java/com/puzzle/common/ui/Animation.kt
🧰 Additional context used
🧬 Code graph analysis (1)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (2)
PieceSnackBarHost(77-97)PieceSnackBar(36-75)
⏰ 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/androidTest/java/com/puzzle/matching/graph/detail/MatchingDetailScreenTest.kt (1)
35-52: LGTM — new callback parameter properly stubbed.The addition of
onAcceptMatchingClick = {}correctly accommodates the updatedMatchingDetailScreenAPI. Consider adding a dedicated test case to verify the acceptance flow behavior (e.g., that clicking the accept button triggers this callback and that the snackbar message is emitted as described in the PR objectives).presentation/src/main/java/com/puzzle/presentation/MainActivity.kt (1)
177-178: Snackbar persistence across navigation needs clarification on scope and impact.Commenting out line 178 removes snackbar dismissal during all navigation events, causing messages from previous screens to persist in the navigation stack. While this enables the intended pattern of showing "puzzle consumed" messages during transitions like Matching → MatchingDetail, the global nature of this change affects all navigation flows.
Current behavior:
- Snackbars only clear when a new
ShowSnackBarevent is triggered (line 248) or if they auto-dismiss- Without an explicit
ShowSnackBarevent preceding navigation, stale snackbars from previous screens will remain visibleRecommend:
- Add a comment documenting why snackbar dismissal is intentionally skipped during navigation (is this for the Matching flow specifically, or a deliberate app-wide change?)
- Consider whether selective dismissal would be more appropriate (e.g., only preserve snackbars for specific navigation routes)
- Test the impact on navigation flows that don't explicitly trigger
ShowSnackBareventspresentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (4)
49-49: LGTM: Visibility restriction improves encapsulation.Changing
PieceApptointernalappropriately reduces the public API surface, preventing external modules from depending on this implementation detail.
108-111: Verify snackbar overlay behavior and z-index ordering.Moving
PieceSnackBarHostoutside the Scaffold changes its positioning and layering:
- Overlay behavior: The snackbar will now overlay the bottom navigation bar and FAB, which may or may not be intentional.
- Z-index:
NetworkScreenandForceUpdateDialog(declared after) will render on top of the snackbar, potentially obscuring it.Please confirm:
- Is overlaying the bottom bar the intended fix for the snackbar positioning issue mentioned in the PR?
- Should
NetworkScreenandForceUpdateDialogtake precedence over the snackbar in the rendering order?
121-121: LGTM: Proper encapsulation of internal helper.Changing
EdgeToEdgeContenttoprivatecorrectly limits its scope to this file, as it's an implementation detail ofPieceApp.
53-53: Navigation callback is properly implemented and consistently wired.The
navigateToBottomNaviDestinationcallback correctly extracts navigation logic and is properly threaded through the app. All bottom navigation destinations (Matching, Profile, Setting) are routed through this callback in AppBottomBar and the FAB, with no inconsistencies or missing integrations.feature/matching/src/main/java/com/puzzle/matching/graph/main/component/card/MatchingCard.kt (3)
103-103: Good semantic improvement with parameter rename.Renaming
matchValueCountTextColortonicknameTextColorbetter reflects the primary usage of this color, though it's also applied to the match count. This improves code readability.Also applies to: 175-175, 197-197, 203-203
380-383: Animation implementation looks good.The
animateContentSizemodifier with 300ms duration andFastOutSlowInEasingshould provide smooth transitions during expand/collapse, addressing the previous concern about jerky animations. The approach is cleaner than managing separate animations for different content states.
389-389: Verify that ContentScale.FillBounds is appropriate for the card background.ContentScale.FillBounds scales the image vertically and horizontally non-uniformly to fill the space, and this type of scaling distorts the image if the container doesn't match the exact ratio of the image. If
bg_matched_cardis a decorative background with patterns or gradients that don't depend on aspect ratio, FillBounds is acceptable. However, if the background has directional or proportional visual elements, consider alternatives:
ContentScale.Crop: Maintains aspect ratio, fills container by cropping edgesContentScale.Fit: Maintains aspect ratio, may leave letterboxing- Apply
Modifier.aspectRatio()to lock the card's aspect ratio, making FillBounds predictable
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
51-84: Verify snackbar positioning for Info and TextOnly states.The layout restructure from Row to Box with top padding is well-structured. However, per the past review comment, please verify that the snackbar position is correct for
InfoandTextOnlystates, as it was reported to appear lower after bottom padding removal.Run the following visual test to confirm positioning:
#!/bin/bash # Search for usages of SnackBarState.Info and SnackBarState.TextOnly to identify test/preview locations rg -n -C3 --type=kotlin 'SnackBarState\.(Info|TextOnly)' -g '!**/test/**'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| @Composable | ||
| private fun calculateSnackBarTopPadding(state: SnackBarState): Dp { | ||
| val density = LocalDensity.current | ||
| val configuration = LocalConfiguration.current | ||
|
|
||
| val systemBarsInsets = WindowInsets.systemBars | ||
|
|
||
| val statusBarHeight = with(density) { systemBarsInsets.getTop(this).toDp() } | ||
| val navigationBarHeight = with(density) { systemBarsInsets.getBottom(this).toDp() } | ||
|
|
||
| val snackBarHeight = 48.dp | ||
| val isMatching = state is SnackBarState.Matching | ||
|
|
||
| return if (isMatching) { | ||
| statusBarHeight + 112.dp | ||
| } else { | ||
| configuration.screenHeightDp.dp - navigationBarHeight - 82.dp - snackBarHeight | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SnackBar.kt" -type fRepository: Piece-Puzzly/Piece-Android
Length of output: 149
🏁 Script executed:
find . -path "*/core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt" -o -path "*/SnackBar.kt" | head -1Repository: Piece-Puzzly/Piece-Android
Length of output: 149
🏁 Script executed:
#!/bin/bash
find . -type f -name "*.kt" | xargs grep -l "calculateSnackBarTopPadding" 2>/dev/null | head -5Repository: Piece-Puzzly/Piece-Android
Length of output: 149
🏁 Script executed:
#!/bin/bash
fd -e kt "SnackBar" | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 214
🏁 Script executed:
#!/bin/bash
find . -type f -name "*.kt" -path "*/designsystem/*" 2>/dev/null | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 1332
🏁 Script executed:
#!/bin/bash
grep -r "112\.dp\|82\.dp\|48\.dp" . --include="*.kt" 2>/dev/null | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 704
🏁 Script executed:
cat -n ./core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt | head -200Repository: Piece-Puzzly/Piece-Android
Length of output: 7953
Replace hard-coded dimensions with measured values or named constants.
The padding calculation relies on inflexible hard-coded values that don't account for actual content:
-
Line 172:
snackBarHeight = 48.dpassumes a fixed height, but the Text component on line 78 hasmaxLines = 2, meaning content can wrap to multiple lines. Material Design specifies single-line snackbars at 48dp and multi-line at 80dp or greater, so this hard-coded value won't accommodate wrapped text. -
Lines 176, 178: Magic numbers
112.dpand82.dplack any documentation explaining their purpose or origin, making maintenance difficult. -
Line 178: The calculation
screenHeightDp - navigationBarHeight - 82.dp - snackBarHeightsimulates bottom positioning using top padding, which is brittle when dealing with different screen sizes, orientations, and edge-to-edge display modes.
Consider these improvements:
- Extract magic numbers as named constants with documentation explaining their purpose (e.g.,
MATCHING_TOP_OFFSET,NON_MATCHING_BOTTOM_OFFSET) - Use
onGloballyPositionedmodifier to measure the actual size and position of the snackbar composable after it is measured and laid out on the UI - Validate positioning logic across different device configurations, orientations, and content sizes (especially with multi-line text)
- Document the positioning strategy for each state
🤖 Prompt for AI Agents
In core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt
around lines 162-180, the top-padding calculation uses hard-coded sizes
(snackBarHeight = 48.dp, 112.dp and 82.dp) that don't account for multi-line
snackbars, different device configurations or edge-to-edge insets; replace these
magic numbers with named constants (e.g., MATCHING_TOP_OFFSET,
NON_MATCHING_BOTTOM_OFFSET, SINGLE_LINE_HEIGHT, MULTI_LINE_HEIGHT) and document
them, measure the actual snackbar height using onGloballyPositioned to decide
between single- and multi-line heights (or compute dynamic height), and compute
padding using measured values and WindowInsets instead of subtracting
screenHeightDp to ensure correct positioning across orientations and screen
sizes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
43-49: Magic number64.35.dpis unusual and fragile.The value
64.35.dpappears to be a pixel-perfect calculation for a specific device/design spec. This precise decimal is fragile and unclear. Consider:
- Defining named constants with documentation explaining the origin (e.g., status bar + some offset)
- Using
WindowInsetsAPIs to compute dynamic values based on actual system bar heightsThe
82.dpbottom padding also lacks documentation. As noted in previous review, these hard-coded values should be extracted as named constants with clear explanations.🔎 Suggested improvement
private object SnackBarPadding { /** Bottom offset for Info/TextOnly states - accounts for bottom navigation bar height */ val BOTTOM_OFFSET = 82.dp /** Top offset for Matching state - accounts for status bar + design margin */ val MATCHING_TOP_OFFSET = 64.dp // Consider using WindowInsets.statusBars instead }
🧹 Nitpick comments (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
110-138: Preview renders two overlapping snackbars - consider splitting for clarity.This preview renders two
PieceSnackBarcomponents without a container. Since each usesfillMaxSize()internally, they overlay each other (one atTopCenter, one atBottomCenter). While this happens to work visually to show both states, it's unconventional and could be confusing.Consider either:
- Splitting into separate preview functions for each state (like
PieceInfoSnackBarPreview)- Adding a comment explaining the intentional overlay behavior
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
78-96: LGTM! Side-effect collection properly implemented.The lifecycle-aware collection using
repeatOnStartedensures snackbar side effects are only processed when the screen is active, preventing leaks and stale UI updates.Optional: The side-effect handling logic (lines 82-91) is duplicated in
MatchingDetailScreen.kt(lines 87-97). Consider extracting this into a common composable function likeCollectMatchingSnackBarSideEffectto reduce duplication.🔎 Example extraction
Create a shared composable in a common utilities file:
@Composable internal fun CollectMatchingSnackBarSideEffect( sideEffects: Flow<MatchingSideEffect>, eventHelper: EventHelper ) { val context = LocalContext.current val lifecycleOwner = LocalLifecycleOwner.current LaunchedEffect(Unit) { lifecycleOwner.repeatOnStarted { sideEffects.collect { sideEffect -> when (sideEffect) { is MatchingSideEffect.ShowMatchingSnackBar -> { val message = sideEffect.count?.let { count -> context.getString(sideEffect.resId, count) } ?: context.getString(sideEffect.resId) eventHelper.sendEvent( PieceEvent.ShowSnackBar(SnackBarState.Matching(message)) ) } } } } } }Then use it in both screens:
CollectMatchingSnackBarSideEffect( sideEffects = viewModel.sideEffects, eventHelper = viewModel.eventHelper )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt(3 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt(2 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt(5 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/MatchingSideEffect.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/detail/contract/MatchingDetailSideEffect.kt(0 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt(5 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(7 hunks)
💤 Files with no reviewable changes (1)
- feature/matching/src/main/java/com/puzzle/matching/graph/detail/contract/MatchingDetailSideEffect.kt
🧰 Additional context used
🧬 Code graph analysis (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
⏰ 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 (11)
core/designsystem/src/main/java/com/puzzle/designsystem/component/SnackBar.kt (1)
51-83: Improved layout approach using alignment-based positioning.The refactored layout using
BoxwithcontentAlignmentandsystemBarsPadding()is cleaner than the previous calculation-heavy approach. The structure properly:
- Uses
fillMaxSize()on the container for positioning flexibility- Applies
systemBarsPadding()for proper edge-to-edge support- Wraps content width on the inner
Rowso the snackbar sizes to its contentfeature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/MatchingSideEffect.kt (1)
1-7: LGTM! Clean side-effect contract.The sealed class structure is well-designed with proper resource ID annotation and flexible count parameter for formatted strings.
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (2)
60-60: LGTM! Import updated correctly.The migration from
MatchingDetailSideEffecttoMatchingSideEffectis properly reflected in the import.
82-102: LGTM! Side-effect handling migrated correctly.The LaunchedEffect correctly collects and handles the new
MatchingSideEffect.ShowMatchingSnackBartype with proper string formatting logic.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (3)
65-66: LGTM! Side-effect channel properly initialized.The Channel-based side-effect mechanism is correctly set up with BUFFERED capacity and exposed as a Flow for UI consumption.
215-215: LGTM! Snackbar side effect emitted correctly.The side effect properly notifies the UI about puzzle usage when acquiring a new instant match.
260-263: LGTM! Formatting improved for readability.The multi-line formatting of the map update enhances code clarity without changing behavior.
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (4)
34-34: LGTM! Side-effect type successfully migrated.The Channel and import have been correctly updated from
MatchingDetailSideEffecttoMatchingSideEffect, maintaining consistency with the new contract.Also applies to: 60-61
118-122: LGTM! Refuse matching side effect updated correctly.The snackbar emission for match refusal now uses the unified
MatchingSideEffect.ShowMatchingSnackBartype.
190-194: LGTM! Accept matching side effect updated correctly.The snackbar emission for match acceptance now uses the unified
MatchingSideEffect.ShowMatchingSnackBartype.
247-252: LGTM! Profile image purchase side effect updated correctly.The snackbar emission for puzzle usage now uses the unified
MatchingSideEffect.ShowMatchingSnackBartype with the correct puzzle count.
| // 2개를 쓴건지, free로 들어간건지 처리 필요함. | ||
| _sideEffects.send( | ||
| ShowMatchingSnackBar( | ||
| R.string.puzzle_used, | ||
| PUZZLE_COUNT_ACCEPT_MATCHING | ||
| ) | ||
| ) | ||
| navigationHelper.navigate(To(ContactRoute(matchId))) | ||
| }.onFailure { handlePurchaseError(it) } |
There was a problem hiding this comment.
Address the TODO comment about free vs. paid match handling.
The TODO comment indicates uncertainty about whether 2 puzzles were used or if the user accessed the contact via free match. This ambiguity could lead to incorrect snackbar messages shown to users.
Consider checking currentState.canFreeMatch or the match type to determine the correct puzzle count (or omit the snackbar entirely for free matches).
Do you want me to help implement logic to differentiate between free and paid contact access?
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
around lines 229 to 237, implement the TODO: determine whether the contact
access was a free match or consumed puzzles and show the snackbar accordingly.
Inspect the current state (e.g., currentState?.canFreeMatch or the match
object/type) and if the access was paid, send
ShowMatchingSnackBar(R.string.puzzle_used, PUZZLE_COUNT_ACCEPT_MATCHING),
otherwise either omit the snackbar or send a different message/count for free
matches; ensure you handle null currentState safely and remove the TODO comment;
keep the existing navigationHelper.navigate(To(ContactRoute(matchId))) and the
onFailure { handlePurchaseError(it) } unchanged.
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)
231-237: Duplicate: TODO for free vs. paid match handling remains unaddressed.This TODO comment and the unconditional snackbar emission were already flagged in a previous review. The issue is identical to the concern at lines 215-217: the code doesn't differentiate between free contact access and paid contact access, potentially showing incorrect puzzle consumption messages to users.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.kt(4 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/block/contract/BlockIntent.kt(1 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt(3 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt(5 hunks)feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.kt (1)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
feature/matching/src/main/java/com/puzzle/matching/graph/block/contract/BlockIntent.kt (1)
9-9: LGTM!The new
OnDismissClickintent follows the established pattern and integrates cleanly with the existing sealed class hierarchy.feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.kt (1)
32-32: LGTM!The dismiss handling correctly resets the
isBlockDonestate, which will remove the dialog from the composition.feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt (2)
34-37: Verify the sequential callback pattern.The
onClickhandler callsonDismissRequest()followed immediately byonBlockDoneClick(). Both trigger asynchronous operations in the ViewModel:
onDismissRequest()→OnDismissClick→ asyncsetState { copy(isBlockDone = false) }onBlockDoneClick()→OnBlockDoneClick→ async navigationSince both operations execute nearly simultaneously, the state update might not complete before navigation begins. While this may work fine in practice due to Compose's recomposition behavior, please verify that:
- The dialog dismisses cleanly before navigation
- No visual glitches occur during the transition
- The timing behaves as intended across different devices
48-56: LGTM!The preview function is well-structured and provides a useful visualization of the dialog with localized content.
feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.kt (3)
43-43: LGTM!The
onDismissClickcallback is properly wired to the ViewModel intent, following the established pattern for other click handlers.
54-54: LGTM!The
onDismissClickparameter is properly added to theBlockScreensignature and correctly forwarded toBlockDoneDialogasonDismissRequest. The callback threading is clean and follows the established pattern.Also applies to: 73-73
181-181: LGTM!The preview is correctly updated with the new
onDismissClickparameter.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
65-67: LGTM!The side-effect channel pattern is correctly implemented using buffered capacity and exposed as a Flow for UI consumption. The internal visibility is appropriate for a feature module ViewModel.
There was a problem hiding this comment.
영상이 있엇내요 하하.
네비게이션 될 때 스낵바가 dismiss되는건 자연스러운 현상이지 않나요(?)
대부분 각 feature 모듈에 scaffold랑 snackbarHost를 두니까 네비게이션되면 당연히 스낵바가 제거되어요.
하지만 피스는 싱글톤으로 스낵바를 관리하고 있어서 네비게이션될때 명시적으로 숨겨주고있어요.
만약 한 화면보다 생명주기가 긴 스낵바가 필요하다면 navigationEvent에 파라미터가 추가되면 될 것 같아요.
이와 관련해서는 민수님께 이전에 이야기 드렸어요~! @comst19
| lifecycleOwner.repeatOnStarted { | ||
| viewModel.sideEffects.collect { sideEffect -> |
There was a problem hiding this comment.
이거 민수님 pr에도 있었는데, common 모듈에 따로 없나용? 없으면 정의하는 것도 괜찮을 것 같아요
There was a problem hiding this comment.
관련 로직 common 모듈에 공통화하여 푸시헀습니다~
There was a problem hiding this comment.
헉 죄송해요. sideEffect 가 아니라, lifecycleOwner.repeatOnStrated 였어요.
대부분의 feature에서 현재는 sideEffect가 불필요한 상황이라 sideEffect를 Base로 추출하는 것은 아직 이른 것 같아요.
제가 리뷰를 명확하게 하지 못했나봐요 죄송해요 ㅠㅠㅠㅠㅠ
There was a problem hiding this comment.
저도 고민하다가 분리한거긴한데,
PieceEvent.ShowSnackBar(SnackBarState.TextOnly("필수 권한을 허용해주세요"))
지금 어느 뷰모델에서는 위와같이 하드코딩으로 사용하고,
특정 뷰모델에서는 아래와 같이
postSideEffect(SnackBarSideEffect.Matching(R.string.puzzle_used, PUZZLE_COUNT_CHECK_PICTURE))
R.String으로 사용하고있는데, 다 통일해서 사용하면 어떨까 해서 아예 base 까지 추출한 것 같아요.
추가의견 주시면 감사하겠습니다! @tgyuuAn
There was a problem hiding this comment.
일단 matching 한정으로 분리해놓았습니다 !
추후 리팩토링하면서 더 고민해볼게요. 900bf01
|
|
||
| PieceSnackBarHost( | ||
| hostState = snackBarHostState, | ||
| snackbar = { snackBarData -> PieceSnackBar(snackBarData) }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Scaffold 밖으로 꺼낸 이유가 있나요?
Scaffold 내부에서는 snackbarHost 슬롯에 어떤 처리가 되고있나욥?.?
There was a problem hiding this comment.
스캐폴드 내부코드를 찾아보면 간단히
val snackbarOffsetFromBottom =
if (snackbarHeight != 0) {
snackbarHeight +
(fabOffsetFromBottom
?: bottomBarHeight
?: contentWindowInsets.getBottom(this@SubcomposeLayout))
} else {
0
}
요런식으로 fab 여부, bottombar 여부, contentWindowInsets 여부로 위치를 판단해주고 있네요.
퍼즐 소비 플로우 추가 이후 MatchMain ↔ MatchDetail 전환이 Snackbar 와 함께 자주 발생하면서,
화면별 BottomBar 유무 차이로 Snackbar 위치가 통일되지 않는 문제가 생겼습니다. (디테일은 바텀바 안씀)
기존 코드에 BottomBar 여부의 분기를 추가하는 대신, Snackbar 를 Scaffold 외부로 분리하는 선택을 했습니다!
| onClick = { | ||
| onDismissRequest() | ||
| onBlockDoneClick() | ||
| }, |
There was a problem hiding this comment.
이거 외부에서 처리하는게 더 깔끔할 것 같아요.
사용자가 실제론 onBlockDone만 클릭한거니까요.
상위 레벨에서 처리해주면 좋을 것 같아요
There was a problem hiding this comment.
넵 처리완료했습니다.
사실 저 로직이 추가된게, ViewModel에서 BlockDone이 클릭됨과 동시에 dialog가 닫히고, navigate 가 순차적으로 이루어져야 하는데, setState가 비동기적으로 이루어져서 나오는 타이밍 이슈 때문이었어요.
관련 이슈 대응 커밋 - ed67d68
There was a problem hiding this comment.
@kkh725 그렇다면 정석적으로 sideEffect로 대응하는 것도 방법일듯해요.
근데 BaseViewModel에 api 추가하는 것 보다는 기존 코드가 더 나은 것 같네요. 그대로 롤백 가능할까요 ?!
주석도 추가해주시면 좋을 것 같아요.
There was a problem hiding this comment.
롤백하고 사이드이펙트로 대응하겠습니다 ~!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
245-251: TODO comment about free vs. paid match handling remains unaddressed.This issue was already flagged in a previous review. The snackbar currently always shows
PUZZLE_COUNT_ACCEPT_MATCHINGregardless of whether the user accessed the contact via a free match or paid with puzzles.
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt (1)
44-47: DuplicatecontentDescriptionon both parent and child containers.Both
PiecePullRefreshBoxand the innerLazyColumnhave the samecontentDescription = "MatchingUserScreen". This can cause confusion in accessibility services and UI tests, as both elements will be announced/identified with the same description.Consider keeping the description only on one element (preferably the outer container) or using distinct descriptions.
🔎 Proposed fix
PiecePullRefreshBox( isRefreshing = isRefreshing, onRefresh = onRefresh, modifier = Modifier.semantics { contentDescription = "MatchingUserScreen" } ) { LazyColumn( - modifier = Modifier.semantics { contentDescription = "MatchingUserScreen" }, + modifier = Modifier, verticalArrangement = Arrangement.spacedBy(12.dp) ) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Box.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/contract/MatchingState.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/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setState(39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingState.kt (1)
20-20: LGTM!The
isRefreshingproperty is correctly added with a sensible default value and follows the existing state pattern.feature/matching/src/main/java/com/puzzle/matching/graph/main/contract/MatchingIntent.kt (1)
14-14: LGTM!The
OnRefreshintent follows the established pattern for other intents in this sealed class.feature/matching/src/main/java/com/puzzle/matching/graph/main/page/MatchingUserScreen.kt (1)
31-68: LGTM on the pull-to-refresh integration.The
PiecePullRefreshBoxwrapper is correctly wired withisRefreshingandonRefresh, and the content structure is properly maintained.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (4)
66-67: LGTM on the side effects Channel setup.Using
Channel<MatchingSideEffect>(BUFFERED)withreceiveAsFlow()is a standard pattern for emitting one-time UI events from ViewModels in Jetpack Compose.
160-170: Well-structured refresh implementation.The guard at line 161 prevents concurrent refresh operations, and the
try-finallyblock ensuresisRefreshingis always reset even if an error occurs. UsingjoinAllto wait for bothinitMatchInfo()andinitFreeMatch()is appropriate since both functions handle their own errors internally.
276-279: LGTM on the formatting adjustment.The multi-line formatting improves readability of the map update operation.
226-233: Snackbar condition logic requires verification for consistency.In the
getInstantNewMatch()function (lines 226-233), the snackbar is sent only when!currentState.canFreeMatchevaluates totrue. However, this function is called directly fromprocessNewMatchingCardOpen()(line 222) only whencanFreeMatchistrue, meaning the condition would evaluate tofalseand the snackbar would never show for that call path.The snackbar logic should either:
- Check if puzzles were actually consumed (perhaps by comparing state before/after the network call)
- Be inverted if the intent is to show a snackbar on successful free match retrieval
- Include a clarifying comment explaining the intent if this behavior is intentional
core/designsystem/src/main/java/com/puzzle/designsystem/component/Box.kt (1)
11-36: Clean implementation of the pull-to-refresh wrapper, but note API stability concerns.The composable correctly uses
rememberPullToRefreshStateand properly wires the state to both thePullToRefreshBoxandIndicator. The@OptIn(ExperimentalMaterial3Api::class)annotation appropriately marks the use of the experimental API.The PullToRefreshBox API remains experimental, which means it is subject to breaking changes in future releases. Previous versions introduced breaking changes to the API, and current versions still require the @ExperimentalMaterial3Api annotation. Consider the stability implications for your production roadmap, particularly if you plan to upgrade Material3 frequently.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt (1)
33-33: Optional: Remove redundant lambda wrapper.Since
onBlockDoneClickis already of type() -> Unit, wrapping it in another lambda is unnecessary. The original direct reference was cleaner:🔎 Simplify by removing the wrapper
- onClick = { onBlockDoneClick() }, + onClick = onBlockDoneClick,core/common-ui/src/main/java/com/puzzle/common/effect/CollectSideEffect.kt (1)
17-21: IncludesideEffectFlowas aLaunchedEffectkey to handle flow instance changes.If
sideEffectFlowchanges (e.g., different ViewModel), the effect won't restart and will continue collecting from the stale flow.🔎 Proposed fix
- LaunchedEffect(lifecycleOwner) { + LaunchedEffect(lifecycleOwner, sideEffectFlow) { lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { sideEffectFlow.collect { effect -> onSideEffect(effect) } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/common-ui/src/main/java/com/puzzle/common/base/BaseContract.ktcore/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.ktcore/common-ui/src/main/java/com/puzzle/common/effect/CollectSideEffect.ktcore/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.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/report/ReportScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/report/ReportViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- feature/matching/src/main/java/com/puzzle/matching/graph/report/ReportScreen.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/foundation/Theme.kt (1)
PieceTheme(15-18)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (1)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (2)
setState(43-44)postSideEffect(48-49)
feature/matching/src/main/java/com/puzzle/matching/graph/report/ReportViewModel.kt (1)
core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
setStateSync(46-46)
⏰ 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/block/dialog/BlockDoneDialog.kt (2)
8-8: LGTM! Imports support the preview function.The Preview and PieceTheme imports are necessary for the new preview composable added at the end of the file.
Also applies to: 15-15
44-51: LGTM! Well-structured preview function.The preview composable follows Compose best practices with proper theming and sample data, making it easy to visualize the dialog during development.
core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.kt (1)
6-25: LGTM!Clean sealed interface design with proper
@StringResannotations. Thehandle()extension provides a nice pattern for exhaustive handling of snackbar types.core/common-ui/src/main/java/com/puzzle/common/base/BaseContract.kt (1)
6-7: LGTM!Clean addition of the
UiSideEffectmarker interface to complete the unidirectional data flow contract.core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
26-27: LGTM!The buffered channel with
receiveAsFlow()is the correct pattern for one-shot side effects, ensuring events are not lost and are consumed by a single collector.feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (3)
95-105: LGTM!The refactored
refuseMatchingcorrectly sequences state update → side effect emission → navigation, aligning with the newSnackBarSideEffect-based architecture.
152-178: LGTM!The
acceptMatchingflow properly setsisShowDialog = falsebefore emitting the snackbar side effect and navigating, preventing UI state inconsistencies.
214-231: LGTM!The
purchaseProfileImageflow correctly usespostSideEffectwith thePUZZLE_COUNT_CHECK_PICTUREconstant, consistent with the new side effect architecture.feature/matching/src/main/java/com/puzzle/matching/graph/report/ReportViewModel.kt (1)
34-43: State reset is redundant but harmless.The ViewModel is automatically cleared when the Report destination is popped from the back stack, so the synchronous reset of
isReportDonetofalsebefore navigating is unnecessary. However, this defensive practice causes no harm and the code is acceptable as-is.
| protected fun setState(reduce: S.() -> S) = | ||
| viewModelScope.launch { _reducer.send(reduce) } | ||
|
|
||
| protected fun setStateSync(reduce: S.() -> S) { _state.update { currentState.reduce() }} |
There was a problem hiding this comment.
Race condition: use the lambda parameter instead of currentState.
currentState is read before the atomic compare-and-swap, so concurrent updates can cause lost state modifications. The update lambda receives the current value atomically—use that instead.
🔎 Proposed fix
- protected fun setStateSync(reduce: S.() -> S) { _state.update { currentState.reduce() }}
+ protected fun setStateSync(reduce: S.() -> S) { _state.update { it.reduce() } }📝 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.
| protected fun setStateSync(reduce: S.() -> S) { _state.update { currentState.reduce() }} | |
| protected fun setStateSync(reduce: S.() -> S) { _state.update { it.reduce() } } |
🤖 Prompt for AI Agents
In core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt around
line 46, the state update uses currentState inside _state.update which reads the
value outside the atomic lambda and can lose concurrent updates; change the code
to use the lambda parameter provided by update (e.g., use it as the receiver or
named parameter) so the reducer is applied to the current value passed into the
lambda, ensuring the update is performed atomically and avoiding race
conditions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
78-86: Consider simplifying the when expression.The
whenblock with a single branch checkingeffect is SnackBarSideEffectcan be simplified to anifstatement for better readability.🔎 Proposed simplification
- CollectSideEffect(viewModel.sideEffect) { effect -> - when { - effect is SnackBarSideEffect -> effect.handle { _, count -> - val message = effect.getMessage(context, count) - - viewModel.eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching(message))) - } - } - } + CollectSideEffect(viewModel.sideEffect) { effect -> + if (effect is SnackBarSideEffect) { + effect.handle { _, count -> + val message = effect.getMessage(context, count) + viewModel.eventHelper.sendEvent(PieceEvent.ShowSnackBar(SnackBarState.Matching(message))) + } + } + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt
- core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.kt
🔇 Additional comments (6)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (6)
26-31: Good refactoring: Common module integration.The addition of
getMessageto the common module aligns with the previous review feedback. The new imports support analytics tracking and side-effect handling, which are properly utilized in this file.Based on learnings, this addresses the previous concern about common module organization.
111-111: LGTM!The
onRefreshcallback is properly wired to the ViewModel throughMatchingIntent.OnRefresh, following the established pattern for other user interactions.
126-128: LGTM!The function signature properly extends the public API with
onRefresh, and the analytics helper is correctly retrieved for event tracking.
303-304: LGTM!The preview function is properly updated with sensible default values for the new refresh parameters.
236-242: Refresh state management is correctly implemented.The
state.isRefreshingis properly managed in the ViewModel: it's set totruewhen refresh starts (line 153) andfalsewhen complete (line 158 in finally block), with a guard clause preventing concurrent refreshes. The parameters are correctly wired toMatchingUserScreen.
135-141: Track analytics before calling the parent accept handler to ensure events are captured.Analytics events should be tracked before calling
onAcceptClick(NewMatching)to guarantee they are recorded before any dialog dismissal or navigation. In Jetpack Compose, when callbacks modify composition state, subsequent operations may not execute if the composable is immediately removed from the composition.Also applies to: 148-154, 159-165
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.kt (2)
7-8: Consider adding@StringResannotation to the interface property.While the implementations already use
@param:StringRes, adding@StringResto the interface property would improve type safety documentation and provide better IDE support.🔎 Proposed addition
sealed interface SnackBarSideEffect : UiSideEffect { - val resId: Int + @get:StringRes + val resId: Int
38-40: Remove trailing empty lines.The file has multiple trailing blank lines that can be removed for consistency.
| fun SnackBarSideEffect.getMessage( | ||
| context: Context, | ||
| ): String = when(this){ | ||
| is SnackBarSideEffect.Matching -> { | ||
| if (count != null) context.getString(resId,count) | ||
| else context.getString(resId) | ||
| } | ||
| else -> context.getString(resId) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "SnackBarSideEffect.kt" 2>/dev/nullRepository: Piece-Puzzly/Piece-Android
Length of output: 147
🏁 Script executed:
cat -n ./core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.ktRepository: Piece-Puzzly/Piece-Android
Length of output: 1513
🏁 Script executed:
rg -n 'SnackBarSideEffect\.Matching|getString.*count' --type kotlinRepository: Piece-Puzzly/Piece-Android
Length of output: 1280
🏁 Script executed:
find . -type f -name "strings.xml" -path "*/values*" | head -20Repository: Piece-Puzzly/Piece-Android
Length of output: 159
🏁 Script executed:
rg -n 'match_refuse|match_accept|puzzle_used' ./app/src/main/res/values/strings.xml ./core/designsystem/src/main/res/values/strings.xmlRepository: Piece-Puzzly/Piece-Android
Length of output: 389
Fix formatting issues in getMessage function
Two minor spacing violations need correction:
- Line 30: Add space after
whenkeyword:when (this)instead ofwhen(this) - Line 32: Add space after comma in function call:
getString(resId, count)instead ofgetString(resId,count)
String resources are properly formatted with placeholders matching their usage—no action needed.
fun SnackBarSideEffect.getMessage(
context: Context,
-): String = when(this){
+): String = when (this) {
is SnackBarSideEffect.Matching -> {
- if (count != null) context.getString(resId,count)
+ if (count != null) context.getString(resId, count)
else context.getString(resId)
}
else -> context.getString(resId)
}📝 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.
| fun SnackBarSideEffect.getMessage( | |
| context: Context, | |
| ): String = when(this){ | |
| is SnackBarSideEffect.Matching -> { | |
| if (count != null) context.getString(resId,count) | |
| else context.getString(resId) | |
| } | |
| else -> context.getString(resId) | |
| } | |
| fun SnackBarSideEffect.getMessage( | |
| context: Context, | |
| ): String = when (this) { | |
| is SnackBarSideEffect.Matching -> { | |
| if (count != null) context.getString(resId, count) | |
| else context.getString(resId) | |
| } | |
| else -> context.getString(resId) | |
| } |
🤖 Prompt for AI Agents
In core/common-ui/src/main/java/com/puzzle/common/effect/SnackBarSideEffect.kt
around lines 28 to 36, fix two spacing style issues: change `when(this)` to
`when (this)` and change `getString(resId,count)` to `getString(resId, count)`
so the code follows the project's spacing conventions.
| CollectSideEffect(viewModel.sideEffect) { effect -> | ||
| when { | ||
| effect is SnackBarSideEffect -> effect.handle { _, count -> | ||
| val message = effect.getMessage(context, count) |
There was a problem hiding this comment.
fun SnackBarSideEffect.getMessage(
context: Context,
): String = when(this){
is SnackBarSideEffect.Matching -> {
if (count != null) context.getString(resId,count)
else context.getString(resId)
}
else -> context.getString(resId)
}
위와 같이 바뀌었으면 count는 빠져야 할 거 같아요
최소한 빌드는 성공 확인하고 커밋하는게 좋을 거 같아요~
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
81-89: Review comment is accurate; refactoring is good with optional improvements suggested.The lifecycle-aware side-effect collection using CollectSideEffect is properly abstracted, and the SnackBarSideEffect handling correctly resolves messages and emits ShowSnackBar events.
Consider these optional improvements:
- Replace the single-branch
whenwith a simplerif (effect is SnackBarSideEffect)for improved clarity.- The
effect.handle { _, _ -> }lambda ignores both parameters. Verify this is intentional—this pattern only executes the onMatching callback while Info and TextOnly effects silently use empty defaults. If all SnackBarSideEffect types should be handled uniformly, a more direct approach may improve readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
core/common-ui/src/main/java/com/puzzle/common/effect/CollectSideEffect.kt (1)
CollectSideEffect(10-22)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (2)
core/common-ui/src/main/java/com/puzzle/common/effect/CollectSideEffect.kt (1)
CollectSideEffect(10-22)feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.kt (2)
PurchaseDialog(18-75)InsufficientPuzzleDialog(77-104)
🔇 Additional comments (6)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (5)
26-31: LGTM! Imports support new side effect handling and analytics.The new imports for
CollectSideEffect,SnackBarSideEffect, analytics helpers, and message utilities properly support the lifecycle-aware side effect collection and analytics tracking added in this PR.
78-86: LGTM! Lifecycle-aware side effect handling implemented correctly.The
CollectSideEffectpattern properly collects the viewModel's side effect flow with lifecycle awareness (per past review feedback) and correctly handlesSnackBarSideEffectby extracting the message and dispatching it viaPieceEvent.ShowSnackBar.
111-111: LGTM! Pull-to-refresh parameter properly propagated.The
onRefreshparameter is correctly added to bothMatchingRouteandMatchingScreensignatures, enabling pull-to-refresh functionality as described in the PR objectives.Also applies to: 126-126
236-237: LGTM! Refresh state and callback properly wired to MatchingUserScreen.The
isRefreshingstate andonRefreshcallback are correctly propagated toMatchingUserScreen, enabling pull-to-refresh UI indication and action handling.Also applies to: 242-242
303-304: LGTM! Preview updated with new refresh parameters.The preview correctly includes the new
isRefreshingandonRefreshparameters with appropriate default values for preview rendering.feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
31-34: LGTM! Clean import additions for the new side effect pattern.The new imports support the refactored side-effect handling mechanism and are all actively used.
| onAcceptClick = { | ||
| onAcceptClick(NewMatching) | ||
| analyticsHelper.trackClickEvent( | ||
| screenName = "new_matching_dialog", | ||
| buttonName = "new_matching", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Analytics screen name inconsistency across dialog tracking.
The analytics screenName values used here differ from those in the dialog components themselves:
| Dialog | Dialog TrackScreenViewEvent | MatchingScreen trackClickEvent |
|---|---|---|
| NewMatching | "match_main_new_match_dialog" |
"new_matching_dialog" (Line 138) |
| Contact | "match_main_contact_dialog" |
"contact_matching_dialog" (Line 151) |
| InSufficient | "match_main_accept_popup" |
"insufficient_puzzle_dialog" (Line 162) |
This inconsistency can fragment analytics data and make tracking user flows difficult. Consider using consistent naming conventions across screen view events and click events for the same UI component.
Based on relevant code snippets from:
feature/matching/src/main/java/com/puzzle/matching/graph/detail/dialog/PurchaseMatchingDialog.kt(lines 17-74, 76-103)
🔎 Suggested fix: align with dialog screen names
onAcceptClick = {
onAcceptClick(NewMatching)
analyticsHelper.trackClickEvent(
- screenName = "new_matching_dialog",
+ screenName = "match_main_new_match_dialog",
buttonName = "new_matching",
)
} onAcceptClick = {
onAcceptClick(Contact(state.dialogType.match))
analyticsHelper.trackClickEvent(
- screenName = "contact_matching_dialog",
+ screenName = "match_main_contact_dialog",
buttonName = "contact_matching",
)
} onAcceptClick = {
onAcceptClick(InSufficient)
analyticsHelper.trackClickEvent(
- screenName = "insufficient_puzzle_dialog",
+ screenName = "match_main_accept_popup",
buttonName = "purchase_puzzle",
)
}Also applies to: 148-154, 159-165
🤖 Prompt for AI Agents
In
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
around lines 135-141 (and similarly adjust lines 148-154 and 159-165), the
analytics trackClickEvent screenName values are inconsistent with the dialog
components; update the screenName strings to match the dialog
TrackScreenViewEvent names: replace "new_matching_dialog" with
"match_main_new_match_dialog", "contact_matching_dialog" with
"match_main_contact_dialog", and "insufficient_puzzle_dialog" with
"match_main_accept_popup" so click events and dialog view events use the same
identifiers across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/effect/SnackBarSideEffect.kt (1)
29-31: Fix spacing style issues.There are spacing violations that were previously flagged:
- Line 29: Add space after
whenand before{:when (this) {- Line 31: Add space after comma:
getString(resId, count)🔎 Proposed fix
fun SnackBarSideEffect.getMessage( context: Context, -): String = when(this){ +): String = when (this) { is SnackBarSideEffect.Matching -> { - if (count != null) context.getString(resId,count) + if (count != null) context.getString(resId, count) else context.getString(resId) } else -> context.getString(resId) }feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
139-145: Analytics screen name inconsistency still present.The analytics
screenNamevalues used in the click tracking here remain inconsistent with the dialog components'TrackScreenViewEventnames, as previously flagged:
Dialog Dialog screen name MatchingScreen screen name NewMatching "match_main_new_match_dialog""new_matching_dialog"(Line 142)Contact "match_main_contact_dialog""contact_matching_dialog"(Line 155)InSufficient "match_main_accept_popup""insufficient_puzzle_dialog"(Line 166)This inconsistency fragments analytics data and makes tracking user flows difficult. Please align the
screenNamevalues to match the dialog component names for consistent tracking.Also applies to: 152-158, 163-169
🧹 Nitpick comments (2)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.kt (1)
85-90: Consider simplifying side-effect handling.The
handle()function provides type-specific dispatch, but here you're callinggetMessage()uniformly for all types and sending the sameSnackBarState.Matchingregardless of the effect type. This could be simplified:viewModel.sideEffect.collect { effect -> val message = effect.getMessage(context) viewModel.eventHelper.sendEvent(ShowSnackBar(SnackBarState.Matching(message))) }Alternatively, if different effect types should produce different
SnackBarStatevariants (e.g.,SnackBarState.InfoforSnackBarSideEffect.Info), consider utilizinghandle()properly for type-specific behavior.feature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/effect/SnackBarSideEffect.kt (1)
37-39: Remove trailing blank lines.There are unnecessary trailing blank lines at the end of the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/common-ui/src/main/java/com/puzzle/common/base/BaseContract.ktcore/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/contract/BlockIntent.ktfeature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/effect/SnackBarSideEffect.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/report/ReportScreen.ktfeature/matching/src/main/java/com/puzzle/matching/graph/report/ReportViewModel.ktfeature/matching/src/main/java/com/puzzle/matching/graph/report/contract/ReportIntent.ktfeature/matching/src/main/java/com/puzzle/matching/graph/report/dialog/ReportDoneDialog.kt
✅ Files skipped from review due to trivial changes (1)
- core/common-ui/src/main/java/com/puzzle/common/base/BaseContract.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockViewModel.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/report/ReportViewModel.kt
- feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (1)
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/block/dialog/BlockDoneDialog.kt (1)
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/detail/dialog/PurchaseMatchingDialog.kt (2)
PurchaseDialog(18-75)InsufficientPuzzleDialog(77-104)
🔇 Additional comments (20)
feature/matching/src/main/java/com/puzzle/matching/graph/block/contract/BlockIntent.kt (1)
9-9: LGTM!The new
OnDismissClickintent follows the existing pattern and integrates well with the dismiss handling flow described in the PR objectives.feature/matching/src/main/java/com/puzzle/matching/graph/block/dialog/BlockDoneDialog.kt (2)
23-24: Consider wiringonDismissRequesttoPieceDialog.The new
onDismissRequestparameter is added to the function signature butPieceDialogstill receives an empty lambda. If users can dismiss the dialog by tapping outside (depending onPieceDialogimplementation), the dismiss callback won't fire.If this is intentional (forcing users to use the button), consider adding a brief comment to clarify.
48-56: LGTM!Preview composable is well-structured and follows the project's pattern for using
PieceTheme.feature/matching/src/main/java/com/puzzle/matching/graph/block/BlockScreen.kt (2)
43-44: LGTM!The
onDismissClickcallback is properly wired through the component hierarchy fromBlockRoutetoBlockDoneDialog. This enables the ViewModel to handle the dismiss action viaBlockIntent.OnDismissClick, addressing the timing issues mentioned in the PR discussion.Also applies to: 54-54, 73-73
171-183: LGTM!Preview updated correctly to include the new
onDismissClickparameter.core/common-ui/src/main/java/com/puzzle/common/base/BaseViewModel.kt (1)
39-40: LGTM! Clean refactor to expression body syntax.The refactor from block body to expression body syntax is a clean style improvement with no behavioral change. The function still correctly launches a coroutine and sends the reduce function to the _reducer channel.
feature/matching/src/main/java/com/puzzle/matching/graph/report/contract/ReportIntent.kt (1)
9-9: LGTM!The new
OnDismissClickintent follows the existing pattern and provides a clear, consistent way to handle dialog dismissal in the report flow.feature/matching/src/main/java/com/puzzle/matching/graph/report/dialog/ReportDoneDialog.kt (2)
19-22: LGTM!The addition of
onDismissRequestparameter provides proper separation between dialog dismissal and navigation logic. The empty lambda forPieceDialog.onDismissRequestenforces that users must interact with the button, which is appropriate for a confirmation dialog.
32-35: Verify callback order is intentional.The
onDismissRequest()is invoked beforeonReportDoneClick(). Ensure this order is intentional—typically dismiss/cleanup happens before navigation triggers.feature/matching/src/main/java/com/puzzle/matching/graph/report/ReportScreen.kt (1)
53-53: LGTM!The
onDismissClickcallback is properly wired through the component hierarchy fromReportRoute→ReportScreen→ReportDoneDialog, providing a clean separation of concerns for dialog dismissal handling.Also applies to: 65-65, 91-91
feature/matching/src/main/java/com/puzzle/matching/graph/detail/common/contract/effect/SnackBarSideEffect.kt (1)
6-13: LGTM on the sealed interface design.The
SnackBarSideEffecthierarchy provides a clean, type-safe way to represent different snackbar notification types with the sharedresIdproperty. TheMatchingvariant appropriately includes an optionalcountfor parameterized string formatting.feature/matching/src/main/java/com/puzzle/matching/graph/detail/MatchingDetailViewModel.kt (4)
61-62: LGTM on the side-effect channel setup.Using
Channel<SnackBarSideEffect>(BUFFERED)withreceiveAsFlow()is the appropriate pattern for one-shot UI events that should survive configuration changes until consumed.
105-107: Verify state update ordering with side-effect emission.
setStateinternally launches a coroutine (seeBaseViewModel.setState), while_sideEffect.send()suspends. Since these are in the samelaunchblock andsend()is awaited, the ordering should be correct, but be aware that the state update is dispatched asynchronously.If strict ordering is required (state updated before snackbar appears), consider using a synchronous state update or ensuring the UI observes state changes before processing side effects.
53-53: Verifyinternalvisibility foreventHelper.
eventHelperis exposed asinternal, which allows access from test code in the same module. Confirm this is intentional and not an oversight.
224-224: LGTM on parameterized snackbar message.The
SnackBarSideEffect.MatchingwithPUZZLE_COUNT_CHECK_PICTUREcount properly utilizes the formatted string resource for displaying puzzle usage information.feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (5)
12-12: LGTM: Import additions support new functionality.The new imports appropriately support side effect handling, lifecycle-aware collection, analytics tracking, and the ShowSnackBar wrapper pattern.
Also applies to: 26-26, 28-28, 30-30, 34-34, 44-45
70-70: LGTM: Lifecycle-aware side effect handling is correctly implemented.The pattern uses
repeatOnStartedto collect side effects only when the lifecycle is in STARTED state or higher, which prevents collecting flows when the screen is not visible. The conversion from side effects toShowSnackBarevents througheventHelpermaintains separation between ViewModel side effects and UI event handling.Also applies to: 80-90
97-97: LGTM: Consistent use of ShowSnackBar wrapper.The back-button snackbar correctly uses the
ShowSnackBarwrapper pattern, consistent with the new side effect handling system introduced in this PR.
115-115: LGTM: onRefresh callback properly wired for pull-to-refresh.The
onRefreshcallback is correctly added to the function signatures and properly propagated fromMatchingRoutethroughMatchingScreentoMatchingUserScreen, enabling the pull-to-refresh functionality mentioned in the PR objectives.Also applies to: 130-130
240-240: LGTM: Refresh state and callback properly propagated.The
isRefreshingstate andonRefreshcallback are correctly passed toMatchingUserScreen, and the preview is updated with appropriate default values. This completes the pull-to-refresh wiring introduced in earlier segments.Also applies to: 246-246, 307-308
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (1)
143-149: Analytics screen names are inconsistent with dialogTrackScreenViewEventvalues.As flagged in a previous review, the
screenNamevalues here don't match those in the dialog components:
Dialog Dialog TrackScreenViewEvent MatchingScreen trackClickEvent NewMatching "match_main_new_match_dialog""new_matching_dialog"Contact "match_main_contact_dialog""contact_matching_dialog"InSufficient "match_main_accept_popup""insufficient_puzzle_dialog"Also applies to: 156-162, 167-173
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.ktfeature/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/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)
⏰ 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 (3)
69-71: Side effect channel setup looks good.Using
Channel.BUFFEREDwithreceiveAsFlow()is appropriate for emitting side effects that should be collected by the UI layer.
135-139: Return type change:getRejectReasonnow returnsResult<*>.The function now returns the result of
suspendRunCatching, but the return value isn't used at the call site (line 114). This is fine but consider if the return value adds value or if the previous void-like behavior was intentional.
259-265: Snackbar always emitted on contact purchase success.Unlike
getInstantNewMatch, this path always emits the snackbar on success, which is correct sincepostOpponentContactis only called from the dialog confirmation (paid path).feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt (3)
119-119:onRefreshcallback wiring looks correct.The callback is properly passed from
MatchingRoutetoMatchingScreenand then toMatchingUserScreen.Also applies to: 134-134
243-251:MatchingUserScreennow receivesisRefreshingandonRefreshprops.The pull-to-refresh state and callback are correctly passed to enable the refresh functionality.
311-312: Preview updated with new parameters.Preview composable correctly includes the new
isRefreshingandonRefreshparameters with sensible defaults.
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingScreen.kt
Show resolved
Hide resolved
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
Show resolved
Hide resolved
| if (!currentState.canFreeMatch) | ||
| _sideEffect.send( | ||
| SnackBarSideEffect.Matching( | ||
| R.string.puzzle_used, | ||
| PUZZLE_COUNT_NEW_MATCH | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Unreachable code: snackbar emission will never execute.
getInstantNewMatch() is only called from processNewMatchingCardOpen() when currentState.canFreeMatch is true (line 231), or from processConfirmDialog() for NewMatching dialog (which appears after the free match check fails). However, the condition !currentState.canFreeMatch at line 238 will be false when called directly from line 231, making this snackbar emission unreachable in that path.
For the dialog confirmation path, the state may still reflect the pre-purchase value since it hasn't been refreshed after the dialog confirmation.
Consider:
- Passing a parameter indicating whether this is a paid match
- Always showing the snackbar when called from the dialog confirmation path
- Refreshing
canFreeMatchstate before this check
🔎 Proposed approach - pass explicit parameter
-private suspend fun getInstantNewMatch() = suspendRunCatching {
+private suspend fun getInstantNewMatch(isPaidMatch: Boolean = false) = suspendRunCatching {
matchingRepository.getNewInstantMatch()
}.onSuccess { matchId ->
- if (!currentState.canFreeMatch)
+ if (isPaidMatch)
_sideEffect.send(
SnackBarSideEffect.Matching(
R.string.puzzle_used,
PUZZLE_COUNT_NEW_MATCH
)
)
navigationHelper.navigate(To(MatchingDetailRoute(matchId)))
}.onFailure { handlePurchaseError(it) }Then update the call sites:
// In processNewMatchingCardOpen - free match, no snackbar
if (currentState.canFreeMatch) getInstantNewMatch(isPaidMatch = false)
// In processConfirmDialog - paid match from dialog
is MatchingDialogType.NewMatching -> getInstantNewMatch(isPaidMatch = true)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/MatchingViewModel.kt
around lines 238 to 245, the snackbar emission guarded by
"!currentState.canFreeMatch" is unreachable from the free-match call path and
uses stale state from the dialog confirmation path; change getInstantNewMatch to
accept a boolean parameter (e.g., isPaidMatch) and use that explicit flag to
decide whether to emit the snackbar, update all call sites
(processNewMatchingCardOpen -> pass false, processConfirmDialog for NewMatching
-> pass true), and remove the reliance on currentState.canFreeMatch inside
getInstantNewMatch (or refresh state before checking) so the snackbar behavior
is driven by the passed parameter.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
238-244: Unreachable/unreliable snackbar emission due to state timing.The condition
!currentState.canFreeMatchis unreliable:
- When called from the free match path (line 231),
canFreeMatchistrue, so the snackbar correctly doesn't emit- When called from dialog confirmation (line 252),
currentState.canFreeMatchmay still betrue(not yet refreshed after purchase), causing the snackbar to be skipped even though puzzles were consumedThe previous review suggested passing an explicit
isPaidMatchboolean parameter to make the logic deterministic. Consider implementing that approach:🔎 Recommended fix - add explicit parameter
-private suspend fun getInstantNewMatch() = suspendRunCatching { +private suspend fun getInstantNewMatch(isPaidMatch: Boolean = false) = suspendRunCatching { matchingRepository.getNewInstantMatch() }.onSuccess { matchId -> - if (!currentState.canFreeMatch) + if (isPaidMatch) _sideEffect.send( SnackBarSideEffect.Matching( R.string.puzzle_used, PUZZLE_COUNT_NEW_MATCH ) ) navigationHelper.navigate(To(MatchingDetailRoute(matchId))) }.onFailure { handlePurchaseError(it) }Then update call sites:
// Line 231: Free match, no snackbar if (currentState.canFreeMatch) getInstantNewMatch(isPaidMatch = false) // Line 252: Paid match from dialog is MatchingDialogType.NewMatching -> getInstantNewMatch(isPaidMatch = true)
🧹 Nitpick comments (1)
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)
135-139: Consider explicit return type or result handling.The function now returns
Result<T>but the call site at line 114 ignores the return value, relying solely on side effects inonSuccess/onFailure. While this works, it may be clearer to either:
- Explicitly declare the return type if it's intended for future use
- Keep it as a fire-and-forget suspend function if the result won't be consumed
📜 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/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)
⏰ 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)
69-71: LGTM! Side effect mechanism properly implemented.The Channel-to-Flow pattern for side effects is correctly set up with buffered capacity, allowing non-blocking emission of snackbar events to the UI layer.
87-87: LGTM! OnRefresh intent properly handled.The intent handler correctly delegates to the new refresh function, supporting the pull-to-refresh functionality mentioned in the PR objectives.
169-179: LGTM! Refresh logic correctly implemented.The function properly guards against concurrent refreshes, uses
joinAllfor concurrent data fetching, and ensuresisRefreshingstate is reset in thefinallyblock. The previous issue with non-existent method names has been resolved.
259-264: LGTM! Snackbar correctly emitted after contact access.The snackbar emission is unconditional and appropriate here, as accessing an opponent's contact via the dialog confirmation always consumes puzzles.
290-293: LGTM! Consistent use ofcurrentStateproperty.Using the
currentStateconvenience property is appropriate and consistent with the rest of the codebase.
1. ⭐️ 변경된 내용
스낵바 추가 이슈로 로직 수정해야할 듯 합니다 !!!! 리뷰 x..
2. 🖼️ 스크린샷(선택)
Screen_recording_20251218_221943.mp4
Screen_recording_20251218_222035.mp4
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.