-
Notifications
You must be signed in to change notification settings - Fork 0
[PC-1596] 회원가입/로그인 qa 이슈 수정 #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This reverts commit 5ae7dca.
📝 WalkthroughWalkthroughAdds Apple/Unknown OAuth handling and non-null provider mapping, makes verification UI IME-aware with authCodeTrigger state, persists signup page via SavedStateHandle, exposes configurable button contentPadding, refactors profile warning dialog callbacks, and makes several preview functions private. Changes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (6)
feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt (1)
110-131: Redundantscope.launchinsideLaunchedEffect.
LaunchedEffectalready provides a coroutine scope, so wrapping the suspend call inscope.launchis unnecessary and creates an additional coroutine that won't be cancelled if the LaunchedEffect is cancelled.🔎 Proposed simplification
LaunchedEffect(imeBottom, state.isAuthCodeRequested) { if (imeBottom > 0 && state.isAuthCodeRequested) { - scope.launch { - scrollState.animateScrollTo(scrollState.maxValue) - } + scrollState.animateScrollTo(scrollState.maxValue) } }If the
scopevariable is no longer needed elsewhere, you can also removeval scope = rememberCoroutineScope()at line 111.core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
75-103: Consider addingcontentPaddingparameter to other button variants for consistency.Now that
PieceSolidButtonhas a customizablecontentPaddingparameter, consider adding the same flexibility toPieceOutlinedButtonand other button variants for API consistency. This would provide uniform customization capabilities across the design system.Example for PieceOutlinedButton
@Composable fun PieceOutlinedButton( label: String, onClick: () -> Unit, modifier: Modifier = Modifier, + contentPadding: PaddingValues = PaddingValues(vertical = 14.dp), enabled: Boolean = true, shape: Shape = RoundedCornerShape(8.dp), ) { Button( onClick = onClick, enabled = enabled, shape = shape, border = BorderStroke(width = 1.dp, color = PieceTheme.colors.primaryDefault), colors = ButtonDefaults.buttonColors( containerColor = PieceTheme.colors.white, contentColor = PieceTheme.colors.primaryDefault, disabledContainerColor = PieceTheme.colors.light1, disabledContentColor = PieceTheme.colors.primaryDefault, ), - contentPadding = PaddingValues(vertical = 14.dp), + contentPadding = contentPadding, modifier = modifier .heightIn(min = 52.dp) .widthIn(min = 100.dp), ) {feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
98-105: Good approach for IME padding, but clean up the comment.The IME padding implementation using
contentPaddingis appropriate for preventing the keyboard from covering input fields in this scrollable list. The modifier pass-through change at line 102 is also a good practice improvement.However, the comment has minor issues:
- Numbered "3." without steps "1." and "2." in this file
- Written in Korean while the rest of the codebase uses English comments
🔎 Suggested comment cleanup
- // 3. contentPadding의 bottom에 키보드 높이를 적용합니다. - // 이렇게 하면 배경색은 가만히 있고, 리스트의 마지막 항목 아래에만 키보드 높이만큼 빈 공간이 생깁니다. + // Apply IME bottom padding to ensure last items remain accessible when keyboard appears contentPadding = PaddingValues(bottom = imeBottom)feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (1)
41-41: Minor formatting: Add space before opening brace.🔎 Proposed fix
-private fun PreviewProfileWarningDialog(){ +private fun PreviewProfileWarningDialog() {feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (2)
80-88: Consider combining state updates into a single setState call.The current implementation calls
setStatetwice: once to updatetermsCheckedInfo, and again viaupdatePageto updatecurrentPage. While this works correctly due to the reducer channel, it queues two state updates instead of one.🔎 Proposed optimization to combine both updates
private fun agreeTerm(termId: Int) { - setState { - val updatedTermsCheckedInfo = termsCheckedInfo.toMutableMap() - .apply { this[termId] = true } - .toMap() - copy(termsCheckedInfo = updatedTermsCheckedInfo) - } - updatePage(SignUpState.SignUpPage.TermPage) + savedStateHandle["current_page"] = SignUpState.SignUpPage.TermPage + setState { + val updatedTermsCheckedInfo = termsCheckedInfo.toMutableMap() + .apply { this[termId] = true } + .toMap() + copy( + termsCheckedInfo = updatedTermsCheckedInfo, + currentPage = SignUpState.SignUpPage.TermPage + ) + } }
115-133: Optional: Reduce duplication in next-page navigation.The pattern
nextPage?.let { updatePage(it) }is repeated on both paths (lines 125 and 131). While the current implementation is correct, you could reduce duplication by extracting the common navigation logic.🔎 Proposed refactor to reduce duplication
private fun onNextClick() { val currentPage = currentState.currentPage val nextPage = SignUpState.SignUpPage.getNextPage(currentPage) if (currentPage == SignUpState.SignUpPage.TermPage) { viewModelScope.launch { val agreeTermsIds = currentState.termsCheckedInfo.filter { it.value }.map { it.key } suspendRunCatching { termsRepository.agreeTerms(agreeTermsIds) }.onSuccess { - nextPage?.let { updatePage(it) } + navigateToNextPage(nextPage) }.onFailure { errorHelper.sendError(it) } } } else { - nextPage?.let { updatePage(it) } + navigateToNextPage(nextPage) } } + +private fun navigateToNextPage(nextPage: SignUpState.SignUpPage?) { + nextPage?.let { updatePage(it) } +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.ktcore/designsystem/src/main/res/values/strings.xmlcore/domain/src/main/java/com/puzzle/domain/model/auth/OAuthProvider.ktfeature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.ktfeature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.ktfeature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.ktfeature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationViewModel.ktfeature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.ktfeature/auth/src/main/java/com/puzzle/auth/graph/verification/dialog/AlreadyRegisteredPhoneDialog.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/RegisterProfileScreen.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/page/BasicProfilePage.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt
🧰 Additional context used
🧬 Code graph analysis (5)
feature/auth/src/main/java/com/puzzle/auth/graph/verification/dialog/AlreadyRegisteredPhoneDialog.kt (1)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (2)
PieceDialog(33-58)PieceDialogIconTop(164-214)
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (1)
PieceDialog(33-58)core/analytics/src/main/java/com/puzzle/analytics/AnalyticsHelper.kt (1)
TrackScreenViewEvent(86-109)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.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/ResultUtil.kt (1)
suspendRunCatching(5-13)
feature/profile/src/main/java/com/puzzle/profile/graph/register/RegisterProfileScreen.kt (1)
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (1)
ProfileWarningDialog(12-37)
feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/TopBar.kt (1)
PieceSubCloseTopBar(93-125)core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
PieceSolidButton(44-72)
⏰ 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 (26)
feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt (7)
7-22: LGTM!The new imports for scroll state, IME handling, density, and coroutine scope are correctly added to support the IME-aware scrolling behavior.
116-117: Good use ofrememberSaveablefor input state.Using
rememberSaveableforphoneNumberandauthCodeensures these values survive configuration changes (e.g., screen rotation), improving user experience.
119-123: LGTM!The
LaunchedEffectcorrectly observesauthCodeTriggerand moves focus to the auth code input when a new request is made. The null check ensures focus only moves when the trigger is set.
140-200: Layout structure looks correct for IME handling.The scrollable
ColumnwithimePadding()ensures content adjusts when the keyboard is shown. TheNextbutton being outside the scrollable area atBottomCenteris acceptable since it's only enabled after verification is complete (when the keyboard is typically dismissed).If users might need to tap "Next" while the keyboard is still visible, consider adding
imePadding()to the button as well:modifier = Modifier .align(Alignment.BottomCenter) .imePadding() .fillMaxWidth() .padding(horizontal = 20.dp) .padding(bottom = 10.dp),
308-314: LGTM!The
contentPaddingparameter correctly adjusts the button's internal padding to accommodate both vertical and horizontal spacing.
381-387: LGTM!Consistent application of
contentPaddingmatching the AuthCodeBody button style.
407-424: LGTM!Changing the preview to
privateis appropriate since it's only used for development purposes and shouldn't be exposed as part of the public API.feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationViewModel.kt (1)
53-63: LGTM!Using
System.currentTimeMillis()as a trigger value is a valid pattern to signal state changes in Compose. Each successful auth code request generates a unique timestamp, ensuring theLaunchedEffectin the UI correctly responds to trigger the focus change.feature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.kt (1)
9-16: LGTM!The
authCodeTriggerproperty is well-designed:
- Nullable type (
Long?) withnulldefault correctly represents the "no trigger" initial state- Using a timestamp as the value ensures each trigger is unique, preventing stale recompositions
- Placement in the data class is appropriate alongside other state properties
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/BasicProfilePage.kt (3)
19-19: LGTM!The import addition is necessary for the
imePadding()modifier usage below.
103-103: LGTM!The
imePadding()modifier correctly addresses the keyboard visibility issues mentioned in the PR objectives. The modifier is properly positioned afterverticalScroll(), and the combination withanimateScrollWhenFocus()on individual fields provides comprehensive IME handling.
873-874: Correct icon logic fix.The updated logic now properly displays:
ic_plus_circlewhen no profile image exists (invite user to add)ic_editwhen a profile image is present (invite user to edit/change)This reverses the previous incorrect behavior and improves UX clarity.
core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (2)
49-49: LGTM! Good enhancement for button customization.The addition of the
contentPaddingparameter allows callers to customize button padding while maintaining backward compatibility through the default value. This directly supports the PR objective of adding padding to verification screen buttons.Also applies to: 62-62
343-343: Preview functions should remain private as best practice. Making preview functionsprivateis correct since they're intended solely for IDE preview and shouldn't expose the public API. Verification confirms no external modules reference these functions, so the change introduces no breaking dependencies.feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
7-7: LGTM: Necessary imports for IME padding.The added imports support the IME-aware padding implementation to handle keyboard visibility.
Also applies to: 10-11, 15-15
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (1)
14-31: LGTM! Clear semantic improvement in dialog callbacks.The parameter rename from
onDismisstoonContinueClickbetter reflects the intent of the right button action. The wiring is correct:
- Left button ("Back") and dismiss request both trigger
onBackClick- Right button ("Continue") triggers
onContinueClickThis provides clear separation between exiting the flow and continuing without navigation.
feature/profile/src/main/java/com/puzzle/profile/graph/register/RegisterProfileScreen.kt (2)
176-180: LGTM! Explicit BackHandler logic fixes the navigation issue.The added
elseblock ensures that on non-BASIC_PROFILE pages, the system back button properly navigates to the previous profile page rather than skipping the entire flow. This aligns with the PR objective to fix the back navigation behavior.
- On
BASIC_PROFILE: Shows warning dialog before allowing back navigation- On other pages: Directly navigates to the previous page
184-188: LGTM! Dialog integration correctly implements the new callback pattern.The dialog callbacks are properly wired:
onBackClick: Closes dialog and triggers navigation back (for the "Back" button and dismiss actions)onContinueClick: Only closes dialog without navigation (for the "Continue" button)This provides clear user intent handling and matches the updated
ProfileWarningDialogsignature.feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (4)
59-62: LGTM! Good abstraction for page updates.The
updatePagehelper cleanly centralizes the logic for persisting the page state to both SavedStateHandle (for process death recovery) and the view model state. This ensures consistency and reduces duplication across navigation transitions.
100-102: LGTM! Clean delegation to updatePage.The navigation to TermDetailPage is now handled consistently through the
updatePagehelper, ensuring state persistence.
104-113: LGTM! Back navigation correctly uses updatePage.The back navigation logic now properly persists the page state via
updatePage, ensuring the navigation history survives process death.
3-3: No action needed — SavedStateHandle fully supports enums.SignUpPage is defined as an enum class, which Android's SavedStateHandle natively supports. Enums are automatically serializable and do not require explicit Parcelable implementation. The usage pattern in the code (
savedStateHandle.get<SignUpState.SignUpPage>(...)andsavedStateHandle["current_page"] = newPage) is correct and will work without modifications.Likely an incorrect or invalid review comment.
core/designsystem/src/main/res/values/strings.xml (1)
142-142: LGTM!The new string resource provides a user-friendly fallback message for unknown OAuth providers, complementing the existing parameterized title.
core/domain/src/main/java/com/puzzle/domain/model/auth/OAuthProvider.kt (1)
5-7: LGTM!The addition of APPLE and UNKNOWN providers properly extends OAuth support. The UNKNOWN case provides a type-safe alternative to nullable returns, improving null-safety across the authentication flow.
feature/auth/src/main/java/com/puzzle/auth/graph/verification/dialog/AlreadyRegisteredPhoneDialog.kt (2)
25-32: LGTM!The conditional title logic elegantly handles the UNKNOWN provider case by displaying a generic message, while known providers receive a personalized message with their display name. This improves UX when the OAuth provider cannot be determined.
Also applies to: 39-39
8-10: LGTM!The preview implementation is well-structured and follows Compose best practices. Using
PreviewParameterProviderto iterate through all OAuth providers enables quick visual verification of the dialog's appearance for each provider type, including the new APPLE and UNKNOWN cases.Also applies to: 57-70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
45-104: Consider addingcontentPaddingto other button variants for API consistency.Currently, only
PieceSolidButtonandPieceOutlinedButtonexpose thecontentPaddingparameter. For a more consistent API, consider adding it to other button variants (PieceSubButton,PieceIconButton, etc.) in a future refactor, especially sincePieceRoundingSolidButtonalready uses custom padding values.This is not urgent for the current PR scope, but would improve the overall API symmetry.
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (1)
39-43: LGTM! Preview function follows conventions.The preview function correctly uses
privatevisibility and follows naming conventions. Empty lambdas are appropriate for static previews.Optional: Add @Preview parameters for better documentation
-@Preview +@Preview(name = "Profile Warning Dialog", showBackground = true) @Composable private fun PreviewProfileWarningDialog() { ProfileWarningDialog({}, {}) }feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (1)
85-96: Consider extracting the target page to reduce duplication.The function hardcodes
SignUpState.SignUpPage.TermPagein two places (lines 86 and 93). While the logic is correct, extracting it to a local variable would reduce duplication and make future changes easier.🔎 Optional refactor to reduce duplication
private fun agreeTerm(termId: Int) { - savedStateHandle[KEY_CURRENT_PAGE] = SignUpState.SignUpPage.TermPage + val targetPage = SignUpState.SignUpPage.TermPage + savedStateHandle[KEY_CURRENT_PAGE] = targetPage setState { val updatedTermsCheckedInfo = termsCheckedInfo.toMutableMap() .apply { this[termId] = true } .toMap() copy( termsCheckedInfo = updatedTermsCheckedInfo, - currentPage = SignUpState.SignUpPage.TermPage + currentPage = targetPage ) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.ktcore/network/src/main/java/com/puzzle/network/model/auth/VerifyAuthCodeResponse.ktfeature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.ktfeature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.ktfeature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.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/ResultUtil.kt (1)
suspendRunCatching(5-13)
feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/TopBar.kt (1)
PieceSubCloseTopBar(93-125)core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
PieceSolidButton(44-72)
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (1)
PieceDialog(33-58)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/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (3)
49-49: LGTM! Good API enhancement for padding customization.The
contentPaddingparameter allows callers to customize button padding while maintaining backward compatibility through the default value. This aligns with the PR objective of adding padding to verification buttons.Also applies to: 62-62
79-79: LGTM! Consistent padding API across button variants.The
contentPaddingparameter addition mirrors the enhancement made toPieceSolidButton, providing consistent configurability across button types.Also applies to: 94-94
344-344: LGTM! Good practice to reduce public API surface.Making all preview functions private is the correct approach, as they are only used by Android Studio's preview system and should not be part of the public API.
Also applies to: 358-358, 372-372, 386-386, 401-401, 416-416, 430-430, 444-444
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
86-86: LGTM! IME-aware padding correctly implemented.The IME padding implementation properly addresses the keyboard covering input fields:
WindowInsets.ime.asPaddingValues().calculateBottomPadding()correctly subscribes to keyboard visibility changes- Using
contentPaddingonLazyColumn(rather thanimePadding()modifier) ensures content can scroll above the keyboard- Modifier refactoring at line 102 appropriately removes redundant
fillMaxSize()chaining since the parent already applies itThis follows Compose best practices and aligns with the PR objective of fixing keyboard obstruction issues in profile screens.
Also applies to: 98-98, 102-103
feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt (2)
13-16: LGTM! Clearer callback naming.The parameter rename from
onDismisstoonContinueClickimproves API clarity. The old naming was counterintuitive (right "continue" button triggeredonBackClick), while the new naming semantically aligns with user actions.
18-18: All call sites updated and dialog dismissal behavior is correctly wired.The callback changes are complete: the single call site in
RegisterProfileScreen.ktuses the new two-parameter signature, and dialog dismissal viaonDismissRequest = onBackClickcorrectly triggers back navigation, consistent with the left "back" button action.feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (3)
3-3: LGTM! SavedStateHandle integration correctly persists signup page state.The implementation correctly addresses the PR objective of fixing navigation issues when going back from the profile creation screen. The pattern is solid:
- Initial state restoration with safe fallback
- Centralized persistence via
updatePagehelper- Proper key scoping
Also applies to: 29-29, 31-39, 64-67
108-110: LGTM! Clean delegation toupdatePagehelper.Both functions correctly use the
updatePageabstraction for page navigation, ensuring consistent state persistence.Also applies to: 118-120
123-141: LGTM! Improved logic with consistent state persistence.Computing
nextPageupfront and usingupdatePagein both branches makes the flow clearer and ensures consistent persistence behavior across all navigation paths.feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt (5)
107-114: LGTM! Good state management.Using
rememberSaveableensures that user input is preserved across configuration changes (e.g., screen rotation), and the IME detection setup is correct.
115-119: LGTM! Proper focus management.The focus automatically moves to the auth code input field when an auth code is requested, providing a smooth user experience.
121-125: LGTM! Auto-scroll addresses keyboard visibility issue.This implementation directly addresses the PR objective of preventing the keyboard from covering the auth code input area by automatically scrolling to the bottom when the keyboard appears.
306-306: LGTM! Button padding added as per PR objectives.The
contentPaddingparameter correctly addresses the PR objective of adding padding to the verification code request/resend buttons, improving the button's visual appearance and touch target size.Also applies to: 379-379
403-403: LGTM! Preview function correctly scoped as private.Reducing the visibility of preview functions is a good practice that minimizes the public API surface.
core/network/src/main/java/com/puzzle/network/model/auth/VerifyAuthCodeResponse.kt (1)
14-14: The implementation correctly handles null input.
OAuthProvider.create()accepts a nullableString?parameter and returns a non-nullOAuthProvider. The implementation uses the elvis operator (?: UNKNOWN) to ensure that null values—whether from a null parameter, safe call (.lowercase()), or unknown API values—result in theUNKNOWNenum constant being returned. No changes are needed.
| PieceSolidButton( | ||
| label = stringResource(R.string.next), | ||
| onClick = onNextClick, | ||
| enabled = state.authCodeStatus == AuthCodeStatus.VERIFIED, | ||
| modifier = Modifier | ||
| .align(Alignment.BottomCenter) | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 20.dp) | ||
| .padding(bottom = 10.dp), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
fd "VerificationScreen.kt" feature/auth/src/main/javaRepository: Piece-Puzzly/Piece-Android
Length of output: 153
🏁 Script executed:
cat -n feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt | sed -n '130,200p'Repository: Piece-Puzzly/Piece-Android
Length of output: 3066
Apply imePadding() to the button or adjust padding to account for keyboard insets.
The button is positioned at BottomCenter of the Box with a fixed bottom padding of 10.dp, but lacks imePadding(). Since the Column containing the input fields already has imePadding(), the button will be exposed to the keyboard when it appears. The fixed 10.dp padding is insufficient to guarantee the button remains visible above the keyboard.
Add .imePadding() to the button's modifier, or dynamically calculate the bottom padding to include keyboard insets:
PieceSolidButton(
label = stringResource(R.string.next),
onClick = onNextClick,
enabled = state.authCodeStatus == AuthCodeStatus.VERIFIED,
modifier = Modifier
.align(Alignment.BottomCenter)
.fillMaxWidth()
.padding(horizontal = 20.dp)
.imePadding()
.padding(bottom = 10.dp),
)🤖 Prompt for AI Agents
In
feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt
around lines 184 to 194, the bottom-aligned PieceSolidButton uses a fixed bottom
padding of 10.dp and can be covered by the IME; update the button modifier to
account for keyboard insets by applying imePadding() before the final bottom
padding (or otherwise incorporate the IME inset into the bottom padding) so the
button is pushed above the keyboard when it appears.
| state = listState, | ||
| modifier = modifier.fillMaxSize(), | ||
| modifier = modifier, | ||
| contentPadding = PaddingValues(bottom = imeBottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에는 imePadding 대신 계산해서 넣으시는 이유는 뭔가요 ??
| SignUpState( | ||
| currentPage = savedStateHandle.get<SignUpState.SignUpPage>(KEY_CURRENT_PAGE) | ||
| ?: SignUpState.SignUpPage.TermPage | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분은 어떤 의도로 쓰여지는 걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
시스템 설정에서 권한 해제 시 configuration 되면서 첫 페이지부터 시작하는 이슈가 있어 추가했습니다
kkh725
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니당
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.