Skip to content

Conversation

@comst19
Copy link
Collaborator

@comst19 comst19 commented Jan 4, 2026

1. ⭐️ 변경된 내용

  • 중복 가입 방지 팝업 apple 추가
  • 인증번호 받기, 인증번호 재전송 버튼 padding 추가
  • 키패드가 인증번호 입력 영역 가리는 이슈 수정
  • 프로필 생성/수정 시 닉네임 입력 영역 가리는 이슈 수정
  • 프로필 생성 시 뒤로가기 하면 바로 프로필 생성 전 화면으로 이동하는 이슈 수정

2. 🖼️ 스크린샷(선택)

3. 💡 알게된 부분

4. 📌 이 부분은 꼭 봐주세요!

  • apple로 회원 가입 한 유저가 중복 회원 가입 테스트 필요

Summary by CodeRabbit

Release Notes

  • New Features

    • Apple OAuth added; unknown-provider handling improved.
    • Buttons now allow customizable content padding.
    • Verification screen gains IME-aware auto-scrolling and keyboard padding.
    • Sign-up state now persists across navigation.
  • Bug Fixes

    • Photo upload icon display corrected.
    • Unsupported OAuth attempts now produce a clear error.
    • Dialog title refined for already-registered accounts with localized text.

✏️ Tip: You can customize this high-level summary in your review settings.

@comst19 comst19 requested review from kkh725 and tgyuuAn January 4, 2026 13:18
@comst19 comst19 self-assigned this Jan 4, 2026
@comst19 comst19 added 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 ㅁㅅ민수 labels Jan 4, 2026
@coderabbitai
Copy link

coderabbitai bot commented Jan 4, 2026

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Design System - Buttons & Strings
core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt, core/designsystem/src/main/res/values/strings.xml
Added contentPadding: PaddingValues = PaddingValues(vertical = 14.dp) parameter to PieceSolidButton and PieceOutlinedButton; made preview helpers private; added already_registered_unknown_title string resource.
Domain Model – OAuth
core/domain/src/main/java/com/puzzle/domain/model/auth/OAuthProvider.kt
Added APPLE and UNKNOWN enum entries; changed create(code: String?) to return non-nullable OAuthProvider with UNKNOWN fallback; added map-based case-insensitive lookup.
Network Mapping
core/network/src/main/java/com/puzzle/network/model/auth/VerifyAuthCodeResponse.kt
Changed toDomain() return type from nullable OAuthProvider? to non-nullable OAuthProvider and now returns OAuthProvider.create(...).
Authentication — Login & Signup
feature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.kt, feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt
Login: added explicit else branch for unsupported OAuth providers to surface an error. Signup: injected SavedStateHandle, added updatePage() to persist current page and replaced direct state mutations with persisted updates.
Verification — State, ViewModel, Screen
feature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.kt, feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationViewModel.kt, feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt
Added authCodeTrigger to VerificationState; set authCodeTrigger = System.currentTimeMillis() on request success; made screen IME-aware (scrolling, imePadding), added rememberSaveable inputs, auto-scroll behavior, and private preview(s).
Verification Dialog
feature/auth/src/main/java/com/puzzle/auth/graph/verification/dialog/AlreadyRegisteredPhoneDialog.kt
Compute dialog title using provider (already_registered_unknown_title for UNKNOWN); added PreviewParameterProvider and preview composable.
Profile Registration — Dialog & Screens
feature/profile/src/main/java/com/puzzle/profile/graph/register/RegisterProfileScreen.kt, feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt
Replaced onDismiss with onContinueClick in ProfileWarningDialog; adjusted back navigation to show dialog on BASIC_PROFILE and delegate otherwise; updated dialog wiring (onDismissRequest → onBackClick).
Profile Pages — IME/Layout & Icons
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/BasicProfilePage.kt, feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt
Added imePadding() to BasicProfilePage Column; swapped photo icon logic (show edit when image present); added IME-driven bottom padding to ValueTalkPage LazyColumn and minor modifier adjustments.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

픽스 👨‍🔧

Suggested reviewers

  • tgyuuAn
  • kkh725

Poem

🐰 I nibble code with nimble feet,
Apple joins the OAuth suite.
Keyboard pads and saved pages wake,
Buttons padded, dialogs make.
Hoppy builds—now ready to eat! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'PC-1596 회원가입/로그인 qa 이슈 수정' directly references the ticket and describes the main changes related to authentication/signup QA issue fixes, which aligns with the changeset.
Description check ✅ Passed The pull request description follows the required template with all four sections completed: changed content clearly listed with 5 bullet points addressing specific issues, screenshot section marked as optional (not required), new learnings section included, and important notes about testing provided.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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: Redundant scope.launch inside LaunchedEffect.

LaunchedEffect already provides a coroutine scope, so wrapping the suspend call in scope.launch is 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 scope variable is no longer needed elsewhere, you can also remove val scope = rememberCoroutineScope() at line 111.

core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)

75-103: Consider adding contentPadding parameter to other button variants for consistency.

Now that PieceSolidButton has a customizable contentPadding parameter, consider adding the same flexibility to PieceOutlinedButton and 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 contentPadding is 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 setState twice: once to update termsCheckedInfo, and again via updatePage to update currentPage. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 41c22dc and 44baeb5.

📒 Files selected for processing (13)
  • core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt
  • core/designsystem/src/main/res/values/strings.xml
  • core/domain/src/main/java/com/puzzle/domain/model/auth/OAuthProvider.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationViewModel.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/verification/dialog/AlreadyRegisteredPhoneDialog.kt
  • feature/profile/src/main/java/com/puzzle/profile/graph/register/RegisterProfileScreen.kt
  • feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt
  • feature/profile/src/main/java/com/puzzle/profile/graph/register/page/BasicProfilePage.kt
  • feature/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 of rememberSaveable for input state.

Using rememberSaveable for phoneNumber and authCode ensures these values survive configuration changes (e.g., screen rotation), improving user experience.


119-123: LGTM!

The LaunchedEffect correctly observes authCodeTrigger and 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 Column with imePadding() ensures content adjusts when the keyboard is shown. The Next button being outside the scrollable area at BottomCenter is 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 contentPadding parameter correctly adjusts the button's internal padding to accommodate both vertical and horizontal spacing.


381-387: LGTM!

Consistent application of contentPadding matching the AuthCodeBody button style.


407-424: LGTM!

Changing the preview to private is 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 the LaunchedEffect in 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 authCodeTrigger property is well-designed:

  • Nullable type (Long?) with null default 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 after verticalScroll(), and the combination with animateScrollWhenFocus() on individual fields provides comprehensive IME handling.


873-874: Correct icon logic fix.

The updated logic now properly displays:

  • ic_plus_circle when no profile image exists (invite user to add)
  • ic_edit when 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 contentPadding parameter 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 functions private is 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 onDismiss to onContinueClick better 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 onContinueClick

This 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 else block 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 ProfileWarningDialog signature.

feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (4)

59-62: LGTM! Good abstraction for page updates.

The updatePage helper 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 updatePage helper, 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>(...) and savedStateHandle["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 PreviewParameterProvider to 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

@Piece-Puzzly Piece-Puzzly deleted a comment from coderabbitai bot Jan 4, 2026
@Piece-Puzzly Piece-Puzzly deleted a comment from coderabbitai bot Jan 4, 2026
Copy link

@coderabbitai coderabbitai bot left a 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 adding contentPadding to other button variants for API consistency.

Currently, only PieceSolidButton and PieceOutlinedButton expose the contentPadding parameter. For a more consistent API, consider adding it to other button variants (PieceSubButton, PieceIconButton, etc.) in a future refactor, especially since PieceRoundingSolidButton already 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 private visibility 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.TermPage in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 44baeb5 and a3685dc.

📒 Files selected for processing (7)
  • core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt
  • core/network/src/main/java/com/puzzle/network/model/auth/VerifyAuthCodeResponse.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/login/LoginViewModel.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt
  • feature/auth/src/main/java/com/puzzle/auth/graph/verification/VerificationScreen.kt
  • feature/profile/src/main/java/com/puzzle/profile/graph/register/dialog/ProfileWarnDialog.kt
  • feature/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 contentPadding parameter 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 contentPadding parameter addition mirrors the enhancement made to PieceSolidButton, 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 contentPadding on LazyColumn (rather than imePadding() modifier) ensures content can scroll above the keyboard
  • Modifier refactoring at line 102 appropriately removes redundant fillMaxSize() chaining since the parent already applies it

This 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 onDismiss to onContinueClick improves API clarity. The old naming was counterintuitive (right "continue" button triggered onBackClick), 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.kt uses the new two-parameter signature, and dialog dismissal via onDismissRequest = onBackClick correctly 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 updatePage helper
  • Proper key scoping

Also applies to: 29-29, 31-39, 64-67


108-110: LGTM! Clean delegation to updatePage helper.

Both functions correctly use the updatePage abstraction for page navigation, ensuring consistent state persistence.

Also applies to: 118-120


123-141: LGTM! Improved logic with consistent state persistence.

Computing nextPage upfront and using updatePage in 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 rememberSaveable ensures 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 contentPadding parameter 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 nullable String? parameter and returns a non-null OAuthProvider. 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 the UNKNOWN enum constant being returned. No changes are needed.

Comment on lines 184 to 194
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),
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd "VerificationScreen.kt" feature/auth/src/main/java

Repository: 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)
Copy link
Collaborator

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
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분은 어떤 의도로 쓰여지는 걸까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시스템 설정에서 권한 해제 시 configuration 되면서 첫 페이지부터 시작하는 이슈가 있어 추가했습니다

Copy link
Collaborator

@kkh725 kkh725 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니당

@comst19 comst19 merged commit 42a45eb into develop Jan 5, 2026
2 checks passed
@comst19 comst19 deleted the fix/PC-1596 branch January 5, 2026 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ㅁㅅ민수 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants