[REFACTOR/#252] 온보딩 일부 구조를 리펙토링 합니다. #262
Conversation
- KakaoLoginSdk 클래스를 별도로 분리해 login/logout/unlink 책임 위임 - ViewModel 내 직접 호출하던 카카오 로그인 로직 제거 - LoginSdk, LoginAccessToken 등 인터페이스 및 value class 도입 - DI 구조(LoginModule) 정리로 테스트와 확장성 개선
- TermsOfService, Nickname screen 분리 구조를 SignUpRoute 단일화 구조로 통합 - Mavericks 기반 MVI 패턴으로 SignUp 상태 및 이벤트 처리 구조 전환 - fcmTokenProvider, LoginSdk 등 SOC 기반 의존성 분리 적용 - back navigation 대응을 위한 navigateToPrevious 전달 구조 반영
…ing 으로 변경 - 기존 TextFieldValue 기반 로직을 단순한 String 기반으로 리팩토링 - 내부 상태를 ViewModel에서 통합 관리할 수 있도록 구조 정비
- 광고 관련 클래스들을 ad 패키지로 이동 - calendar을 calendar 패키지로 이동
WalkthroughThis change refactors the onboarding flow (Splash, Login, SignUp) to use the Mavericks MVI architecture, consolidates and simplifies navigation, and modularizes authentication logic. It introduces new contract, state, and view model classes for Splash, Login, and SignUp, restructures UI screens, and moves social login and FCM logic to the core package for better separation of concerns. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SplashViewModel
participant LoginViewModel
participant SignUpViewModel
participant LoginSdk
participant FcmTokenProvider
participant TokenRepository
participant UserRepository
App->>SplashViewModel: postIntent(InitSplash)
SplashViewModel->>SplashViewModel: autoLogin & versionCheck
SplashViewModel-->>App: sideEffect (NavigateToLogin or NavigateToHome)
App->>LoginViewModel: postIntent(LoginWithKakao)
LoginViewModel->>LoginSdk: login(context)
LoginSdk-->>LoginViewModel: Result<LoginAccessToken>
LoginViewModel->>FcmTokenProvider: getToken()
LoginViewModel->>UserRepository: signIn(token, fcmToken)
UserRepository-->>LoginViewModel: Result<SignInResponse>
LoginViewModel->>TokenRepository: store tokens
LoginViewModel-->>App: sideEffect (NavigateToHome or NavigateToSignUp or ShowError)
App->>SignUpViewModel: postIntent(CompleteSignUp)
SignUpViewModel->>LoginSdk: login(context)
SignUpViewModel->>FcmTokenProvider: getToken()
SignUpViewModel->>UserRepository: signUp(token, nickname, fcmToken)
UserRepository-->>SignUpViewModel: Result<SignUpResponse>
SignUpViewModel->>TokenRepository: store tokens
SignUpViewModel-->>App: sideEffect (NavigateToTimeReminder or ShowMessage)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope changes detected) Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MoonsuKang
changed the base branch from
refactor/#260-navigation-structure
to
develop
MoonsuKang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
app/src/main/java/com/sopt/clody/core/ad/RewardAdShower.kt (1)
1-1: Package relocation tocom.sopt.clody.core.adaligns the interface with AD-related classes. Consider adding KDoc to clarify the interface’s responsibilities.app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/CalendarDateData.kt (1)
1-22: Approve package relocation, but note file/class naming discrepancy.The package refactoring and the date utility functions (
daysInMonthandgenerateCalendarDates) are correct and unchanged in logic. However, the file is namedCalendarDateData.ktwhile the primary data class isCalendarDate. For consistency and maintainability, consider renaming the file toCalendarDate.ktor renaming the class toCalendarDateData.app/src/main/java/com/sopt/clody/core/fcm/FcmTokenProvider.kt (1)
9-18: Well-implemented FCM token provider with proper error handling.The implementation correctly uses Firebase KTX extensions with coroutines and handles exceptions appropriately. The null return value clearly indicates failure cases.
Minor suggestion: Consider using English for the log message to maintain consistency if the codebase follows English logging conventions.
- Timber.e("FCM 토큰 수신 실패: ${e.message}") + Timber.e("Failed to retrieve FCM token: ${e.message}")app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
10-14: Good implementation of standardized preview setup.The custom
ClodyPreviewannotation effectively combines multiple preview configurations, and the commented example for foldable devices shows thoughtful consideration for different form factors.Consider using English for comments to maintain consistency if that's the project standard.
-// 아래 폴드는 예시이고 fontScale 같은 값도 조정이 가능합니다. -//@Preview(name = "Galaxy Z Fold3 접힌화면 (840x2289)", widthDp = 320, heightDp = 870, showBackground = true) +// Example for foldable devices - fontScale and other values can also be adjusted +//@Preview(name = "Galaxy Z Fold3 Folded (840x2289)", widthDp = 320, heightDp = 870, showBackground = true)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (1)
14-15: Update outdated comment to reflect current implementation.The comment still mentions "Scaffold 레이아웃을 관리하고" but the implementation now uses
Boxinstead ofScaffold.- * 앱 전반의 테마와 시스템 UI 설정, Scaffold 레이아웃을 관리하고 + * 앱 전반의 테마와 시스템 UI 설정을 관리하고app/src/main/java/com/sopt/clody/presentation/ui/login/LoginContract.kt (1)
13-16: Consider abstracting the Context dependency.While the intent structure is correct, passing
Contextdirectly in the intent couples the business logic to Android framework. Consider injecting a login use case that handles the context internally.sealed class LoginIntent { - data class LoginWithKakao(val context: Context) : LoginIntent() + data object LoginWithKakao : LoginIntent() data object ClearError : LoginIntent() }app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt (1)
92-109: Consider the "agree all" checkbox behavior.The current implementation shows the "agree all" checkbox but doesn't automatically sync its state when individual checkboxes change. Users might expect the "agree all" checkbox to be checked when both individual terms are selected.
Row( modifier = Modifier.fillMaxWidth(), verticalAlignment = Alignment.CenterVertically, ) { Text( text = stringResource(R.string.terms_agree_all), modifier = Modifier.weight(1f), color = ClodyTheme.colors.gray01, style = ClodyTheme.typography.head3, ) CustomCheckbox( - checked = allChecked, + checked = serviceChecked && privacyChecked, onCheckedChange = onToggleAll, size = 25.dp, checkedImageRes = R.drawable.ic_terms_check_on_25, uncheckedImageRes = R.drawable.ic_terms_check_off_25, ) }app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt (1)
31-33: Address the TODO comment for error handling.The comment indicates uncertainty about error handling strategy. Consider implementing a consistent error handling pattern across the app.
is SignUpContract.SignUpSideEffect.ShowMessage -> { - // 삐용삐용 에러 대응을 어떻게 할까요? + _sideEffects.send(SignUpContract.SignUpSideEffect.ShowError(effect.message)) }Would you like me to help design a comprehensive error handling strategy for the app?
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (1)
166-169: Optimize regex pattern by removing duplicate character range.The regex contains duplicate
가-힣character range. While functionally correct, it can be simplified.Apply this diff to optimize:
private fun validateNickname(nickname: String): Boolean { - val regex = "^[a-zA-Z가-힣0-9ㄱ-ㅎㅏ-ㅣ가-힣]{2,10}$".toRegex() + val regex = "^[a-zA-Z가-힣0-9ㄱ-ㅎㅏ-ㅣ]{2,10}$".toRegex() return nickname.matches(regex) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
app/build.gradle.kts(1 hunks)app/src/main/java/com/sopt/clody/ClodyApplication.kt(2 hunks)app/src/main/java/com/sopt/clody/core/ad/RewardAdShower.kt(1 hunks)app/src/main/java/com/sopt/clody/core/fcm/FcmTokenProvider.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/KakaoAccessToken.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/KakaoLoginSdk.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/LoginAccessToken.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/LoginException.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/LoginModule.kt(1 hunks)app/src/main/java/com/sopt/clody/core/login/LoginSdk.kt(1 hunks)app/src/main/java/com/sopt/clody/data/ad/RewardAdShowerImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/di/AdModule.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/di/ViewModelsModule.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/component/textfield/NickNameTextField.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/login/SignInState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/NicknameScreen.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/TermsOfServiceScreen.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/NicknamePage.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/ClodyCalendar.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/MonthlyItem.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/CalendarDateData.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/DiaryDateData.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/navigation/LoginNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/MainActivity.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/extension/LifeCycle.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt(1 hunks)app/src/main/java/com/sopt/clody/ui/theme/Theme.kt(1 hunks)gradle/libs.versions.toml(3 hunks)
💤 Files with no reviewable changes (4)
- app/src/main/java/com/sopt/clody/presentation/ui/auth/login/SignInState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/TermsOfServiceScreen.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/NicknameScreen.kt
🧰 Additional context used
🧬 Code Graph Analysis (10)
app/src/main/java/com/sopt/clody/presentation/ui/main/MainActivity.kt (1)
app/src/main/java/com/sopt/clody/ui/theme/Theme.kt (1)
ClodyTheme(7-12)
app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt (1)
ClodyNavHost(50-124)
app/src/main/java/com/sopt/clody/presentation/ui/login/navigation/LoginNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt (1)
LoginRoute(36-73)
app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
app/src/main/java/com/sopt/clody/ui/theme/Theme.kt (1)
ClodyTheme(7-12)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/NicknamePage.kt (4)
app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt (1)
ClodyButton(14-43)app/src/main/java/com/sopt/clody/presentation/ui/auth/component/textfield/NickNameTextField.kt (1)
NickNameTextField(28-97)app/src/main/java/com/sopt/clody/presentation/ui/component/LoadingScreen.kt (1)
LoadingScreen(15-30)app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
BasePreview(16-23)
app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt (1)
signUpScreen(10-20)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt (5)
app/src/main/java/com/sopt/clody/presentation/ui/component/button/ClodyButton.kt (1)
ClodyButton(14-43)app/src/main/java/com/sopt/clody/presentation/ui/auth/component/checkbox/CustomCheckBox.kt (1)
CustomCheckbox(18-47)app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/HorizontalDivider.kt (1)
HorizontalDivider(14-26)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt (1)
onClickSettingOption(112-115)app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
BasePreview(16-23)
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/FailureDialog.kt (1)
FailureDialog(32-95)app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
BasePreview(16-23)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
BasePreview(16-23)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt (1)
SignUpRoute(16-51)
🔇 Additional comments (71)
app/src/main/java/com/sopt/clody/data/ad/RewardAdShowerImpl.kt (1)
6-6: Import path updated TheRewardAdShowerimport now correctly referencescom.sopt.clody.core.ad.RewardAdShowerafter the package change.app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt (1)
6-6: Import path adjusted Now usingcom.sopt.clody.core.ad.RewardAdShowerin the ViewModel to match the moved interface.app/src/main/java/com/sopt/clody/di/AdModule.kt (1)
3-3: Adjusted import for DI binding TheRewardAdShowerHilt binding now points to the relocated packagecom.sopt.clody.core.ad.app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/model/DiaryDateData.kt (1)
1-8: Approve package relocation and data class integrity.The package declaration has been updated correctly, and the
DiaryDateDatadata class retains its intended functionality. The default initialization usingLocalDate.now()is appropriate, and the import forjava.time.LocalDateis necessary.app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/MonthlyItem.kt (1)
18-18: Approve updated import forCalendarDate.The import path was updated to the new
calendar.modelpackage correctly. There are no changes to the surrounding logic, and the component remains functionally equivalent.app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (1)
9-9: Approve updated import forDiaryDateData.The
DiaryDateDataimport has been adjusted to the refactoredcalendar.modelpackage. No other code or functionality is affected.app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
29-29: Approve updated import forDiaryDateData.The import path is correctly updated to
com.sopt.clody.presentation.ui.home.calendar.model.DiaryDateData, aligning with the package reorganization. No functional changes detected.app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/ClodyCalendar.kt (1)
20-20: Import path correctly updated to the new calendar model package
ThegenerateCalendarDatesfunction is now imported fromcom.sopt.clody.presentation.ui.home.calendar.model, matching the recent package reorganization for calendar-related data classes.app/src/main/java/com/sopt/clody/ui/theme/Theme.kt (1)
8-8: LGTM! Improved naming convention.The rename from
CLODYThemetoClodyThemefollows Kotlin naming conventions and improves consistency with the existingClodyThemeobject.app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
16-23: Excellent BasePreview composable for consistent theming.The
BasePreviewcomposable provides a clean, reusable way to apply consistent theming and background to all UI previews. The use ofClodyTheme.colors.whiteensures proper theme integration.app/src/main/java/com/sopt/clody/presentation/ui/main/MainActivity.kt (1)
10-10: Correctly updated to use the renamed theme function.The import and usage changes properly align with the
CLODYThemetoClodyThemerename, maintaining consistency across the codebase.Also applies to: 33-33
gradle/libs.versions.toml (3)
120-124: Well-structured Mavericks library declarations.The library declarations follow the established patterns and properly reference the version. The modular approach with separate entries for core, compose, and hilt integrations allows for flexible dependency management.
191-195: Excellent use of dependency bundling.The mavericks bundle groups related libraries together, making it easier to include all necessary Mavericks dependencies in modules that need the full MVI framework.
51-51:Details
❓ Verification inconclusive
Verify Mavericks version compatibility and stability.
The Mavericks version 3.0.9 should be verified for stability and compatibility with the project's other dependencies, particularly with the Compose BOM version 2024.12.01 and Hilt 2.54.
🌐 Web query:
Is Mavericks 3.0.9 the latest stable version and is it compatible with Compose BOM 2024.12.01 and Hilt 2.54?💡 Result:
Yes, Mavericks 3.0.9 is the latest stable version of the Mavericks framework. This version was released on January 25, 2024. (mvnrepository.com)
Regarding compatibility with Jetpack Compose BOM 2024.12.01 and Hilt 2.54, the official documentation does not specify explicit compatibility between Mavericks 3.0.9 and these versions. The latest Compose BOM version available is 2025.02.00, and the latest Hilt version is 2.52. (developer.android.com, developer.android.com)
Given that Mavericks 3.0.9 was released before these versions of Compose and Hilt, it's advisable to test your application thoroughly to ensure compatibility. If you encounter issues, consider updating Mavericks to the latest version or consulting the official Mavericks repository for guidance on compatibility with newer versions of Compose and Hilt.
Citations:
- 1: https://mvnrepository.com/artifact/com.airbnb.android/mavericks-compose/3.0.9?utm_source=openai
- 2: https://developer.android.com/develop/ui/compose/bom?utm_source=openai
- 3: https://developer.android.com/jetpack/androidx/releases/hilt?utm_source=openai
Mavericks 3.0.9 is the latest stable release; please validate compatibility
- Mavericks 3.0.9 (released Jan 25, 2024) is confirmed as the latest stable framework version.
- There’s no official guidance on compatibility with Compose BOM 2024.12.01 or Hilt 2.54.
- Run your full integration and CI test suite to catch any regressions.
- If you hit issues, consider bumping Compose BOM to 2025.02.00 or Hilt to the newest release, and check the Mavericks repo for compatibility notes.
app/build.gradle.kts (1)
123-124: Proper integration of Mavericks dependency bundle.The addition of the Mavericks bundle is correctly placed and follows the established dependency organization pattern. This will include all necessary Mavericks libraries (core, compose, and hilt) as defined in the version catalog.
app/src/main/java/com/sopt/clody/core/login/LoginAccessToken.kt (1)
3-5: Clean abstraction for login access tokens.The interface design is excellent - it's simple, focused, and provides a clear contract for access token implementations. The read-only property ensures immutability and the generic nature allows for different login provider implementations.
app/src/main/java/com/sopt/clody/ClodyApplication.kt (2)
4-4: Correct Mavericks import.The import for Mavericks is properly added and follows the standard import pattern.
18-18: Proper Mavericks initialization placement.The Mavericks initialization is correctly placed after Firebase initialization and before Amplitude initialization. This ensures that the MVI framework is available early in the application lifecycle while respecting the dependency order.
app/src/main/java/com/sopt/clody/core/login/KakaoAccessToken.kt (1)
3-4: Excellent use of inline value class for type safety.The implementation demonstrates best practices by using
@JvmInlinevalue class to create a type-safe wrapper around the access token string. This provides compile-time type safety while maintaining runtime performance through inlining, and properly implements theLoginAccessTokeninterface.app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (1)
28-33: LGTM! Layout simplification is appropriate.The replacement of
ScaffoldwithBoxeliminates redundant layout wrapping sinceClodyNavHostalready uses aBoxinternally. This simplification aligns well with the broader navigation refactoring objectives.app/src/main/java/com/sopt/clody/core/login/LoginModule.kt (1)
1-16: Excellent implementation of dependency injection pattern.This Hilt module correctly abstracts the login functionality through interface binding, supporting the modularity objectives mentioned in the PR. The use of
SingletonComponentscope is appropriate for a login service, and the@Bindsannotation properly maps the concreteKakaoLoginSdkimplementation to theLoginSdkinterface.app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt (1)
19-20: SignUp route consolidation aligns perfectly with refactoring objectives.The consolidation of separate
TermsOfServiceandNicknameroutes into a singleSignUproute effectively implements the page-based approach mentioned in the PR objectives. This simplifies the navigation structure while maintaining type safety through the@Serializableannotation.app/src/main/java/com/sopt/clody/presentation/utils/extension/LifeCycle.kt (1)
1-27: Excellent lifecycle-aware utility function implementation.This extension function correctly implements Android's recommended pattern for lifecycle-aware coroutine execution. The use of
repeatOnLifecyclewithSTARTEDstate is the best practice for UI-related coroutines, ensuring they're only active when the UI is visible to users. The comprehensive documentation and proper API usage make this a valuable addition supporting the Mavericks MVI architecture.app/src/main/java/com/sopt/clody/core/login/LoginException.kt (1)
3-6: Well-designed exception hierarchy for typed error handling.The sealed class design with specific exception types (
CancelExceptionandAuthException) provides excellent type safety and enables exhaustive error handling in when expressions. This aligns well with the Result-based error handling pattern used in theLoginSdkinterface.app/src/main/java/com/sopt/clody/core/login/LoginSdk.kt (1)
5-9: Excellent interface design for authentication abstraction.The interface follows best practices with:
- Suspend functions for proper coroutine support
Result<T>return types for type-safe error handling- Clear separation of authentication operations
- Context parameter for Android-specific login requirements
This abstraction enables dependency injection, testing, and aligns perfectly with the MVI architecture goals.
app/src/main/java/com/sopt/clody/presentation/ui/login/navigation/LoginNavigation.kt (3)
1-1: Package structure simplified appropriately.The package path change from
com.sopt.clody.presentation.ui.auth.login.navigationtocom.sopt.clody.presentation.ui.login.navigationsimplifies the structure and aligns with the architectural refactor.
7-7: Import updated correctly for new package structure.The import path correctly reflects the new package structure.
11-11: Navigation parameter renamed to reflect consolidated signup flow.The parameter name change from
navigateToTermsOfServicetonavigateToSignUpcorrectly reflects the consolidation of the signup flow mentioned in the PR description, where separate terms of service and nickname screens were unified into a single SignUpRoute.Also applies to: 16-16
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/NicknamePage.kt (6)
36-47: Well-designed composable with proper state management.The composable function signature follows excellent practices for MVI architecture:
- Clear parameter-based state management
- Proper separation of data and actions
- Comprehensive state representation (validation, loading, focus)
50-60: Nice implementation of character count display.The annotated string approach for showing character count with different colors is clean and provides good visual feedback to users.
89-89: Proper button enabled state logic.The condition
nickname.isNotEmpty() && isValidNicknamecorrectly ensures the user can only proceed when they have entered a valid nickname.
118-131: Excellent validation feedback with color-coded messages.The conditional color logic for the nickname message provides clear visual feedback:
- Gray for empty/valid states
- Red for invalid states
- Character count display with proper styling
136-138: Proper loading state handling.The conditional loading screen overlay correctly handles the loading state without interfering with the main UI.
141-157: Well-structured preview function.The preview uses the new
@ClodyPreviewannotation andBasePreviewwrapper, which provides consistent theming and follows the standardized preview pattern mentioned in the PR description.app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt (2)
12-13: LGTM! Import updates align with the MVI refactor.The imports correctly reflect the new unified signup navigation structure and updated login architecture.
Also applies to: 20-21
70-76: Navigation flow successfully consolidated.The change from separate terms/nickname screens to a unified
signUpScreenproperly implements the page-based approach mentioned in the PR objectives. The navigation callbacks are correctly wired.app/src/main/java/com/sopt/clody/presentation/ui/login/LoginContract.kt (2)
8-11: Well-structured MVI state definition.The
LoginStateproperly encapsulates the UI state with loading and error message fields, following Mavericks conventions.
18-22: Side effects properly defined for navigation and error handling.The side effects correctly separate concerns between business logic and UI actions, enabling proper unidirectional data flow.
app/src/main/java/com/sopt/clody/presentation/di/ViewModelsModule.kt (2)
14-16: Proper Mavericks + Hilt integration setup.The module is correctly scoped to
MavericksViewModelComponentenabling proper dependency injection for Mavericks ViewModels.
18-37: ViewModel factory bindings follow established patterns.All three ViewModels (Splash, Login, SignUp) are properly bound into the multibinding map with correct annotations, enabling Mavericks to resolve them via Hilt.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/page/TermsOfServicePage.kt (5)
37-50: Well-designed component parameters and state logic.The component properly separates state from UI logic through callback parameters, enabling proper MVI integration. The agree button enablement logic correctly requires both service and privacy terms to be checked.
51-76: Scaffold structure follows Compose best practices.The top bar with back navigation and bottom bar with the agree button are properly positioned and styled. The navigation callbacks enable proper flow control.
113-125: External URL integration handled properly.The terms and privacy policy links correctly use the existing
onClickSettingOptionutility and predefined URLs, maintaining consistency with the app's settings screen.
131-159: Excellent reusable component design.The
TermsCheckboxRowcomponent is well-designed for reusability with clear parameters and proper UI structure. The "More" button integration is intuitive.
161-176: Preview implementation supports development workflow.The preview properly uses
BasePreviewand provides a good representation of the component's appearance during development.app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt (1)
7-23: LGTM! Clean MVI contract definition.The contract follows proper Mavericks MVI patterns with clear separation between state, intents, and side effects. The structure is well-organized and maintains immutability principles.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpScreen.kt (2)
16-51: Excellent MVI implementation with proper Mavericks usage.The
SignUpRoutecorrectly implements the Mavericks pattern with proper state collection and side effect handling. The separation between route management and UI rendering is well done.
53-88: Well-structured page navigation based on state.The step-based navigation using
whenexpression is clean and maintainable. The intent passing pattern maintains unidirectional data flow correctly.app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt (2)
23-44: Excellent Mavericks ViewModel implementation.The ViewModel correctly implements the MVI pattern with proper dependency injection, intent handling via channels, and side effect management. The architecture is clean and follows best practices.
71-92:Details
✅ Verification successful
Verify FCM token handling and repository error types.
Please ensure that the
FcmTokenProvider.getToken()handles failures gracefully and that the repository returns structured errors for better error handling.
🏁 Script executed:
#!/bin/bash # Check if FcmTokenProvider has proper error handling ast-grep --pattern 'interface FcmTokenProvider { $$$ getToken(): $_ $$$ }' # Check AuthRepository error handling ast-grep --pattern 'interface AuthRepository { $$$ signIn($_, $_): $_ $$$ }'Length of output: 167
Let’s locate where (and how)
FcmTokenProviderandAuthRepositoryare defined and used.This will confirm whether
getToken()andsignIn()surface errors or exceptions we need to handle explicitly.
🏁 Script executed:
#!/bin/bash # Find all mentions of FcmTokenProvider rg "FcmTokenProvider" -n # Find the getToken() declaration/implementation rg "getToken" -n app/src # Find all mentions of AuthRepository rg "AuthRepository" -n # Find the signIn() declaration/implementation rg "signIn" -n app/srcLength of output: 4870
Let’s inspect the actual implementations to confirm error handling:
🏁 Script executed:
#!/bin/bash # Show FcmTokenProvider implementation sed -n '1,200p' app/src/main/java/com/sopt/clody/core/fcm/FcmTokenProvider.kt # Show AuthRepository interface sed -n '1,200p' app/src/main/java/com/sopt/clody/domain/repository/AuthRepository.kt # Show AuthRepositoryImpl implementation sed -n '1,200p' app/src/main/java/com/sopt/clody/data/repositoryimpl/AuthRepositoryImpl.kt # Show data source signIn and handleApiResponse usage sed -n '1,200p' app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/AuthDataSourceImpl.kt # Locate handleApiResponse extension rg "fun handleApiResponse" -n app/srcLength of output: 3517
Inspecting the
handleApiResponseextension to confirm how API errors are mapped:
🏁 Script executed:
#!/bin/bash # Locate all references to handleApiResponse rg "handleApiResponse" -n # Print out its implementation for review rg -l "handleApiResponse" | xargs -I {} sed -n '1,200p' {}Length of output: 11180
FCM token and repository error handling verified
FcmTokenProvider.getToken()already wrapsFirebase.messaging.token.await()in atry/catch, logs failures via Timber, and returnsnullon error.AuthRepositoryImpl.signIn()(andsignUp()) usehandleApiResponse()to map HTTP status codes intoResult<T>failures with descriptive exception messages.No further changes are required for basic error‐handling. Consider introducing domain-specific error types later if you need finer-grained handling in the ViewModel.
app/src/main/java/com/sopt/clody/presentation/ui/auth/component/textfield/NickNameTextField.kt (3)
29-47: Good simplification from TextFieldValue to String.The refactoring simplifies the component interface by using a plain String instead of TextFieldValue, which aligns better with the MVI state management approach. The length validation logic is correctly updated.
86-91: Improved validation state visualization.The underline color logic properly reflects the different states (empty, invalid, focused, valid) providing clear visual feedback to users.
102-112: Preview updated to match new component signature.The preview correctly uses the simplified String-based interface, maintaining consistency with the refactored component.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt (1)
1-26: Well-structured navigation consolidation!The refactoring successfully consolidates the previously separated signup flow into a single
SignUpRoute, aligning perfectly with the MVI architecture pattern. The navigation functions maintain proper separation of concerns by passing navigation callbacks rather than the NavController directly.app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt (2)
67-72: Clean error handling implementation!The error dialog pattern with state-based display and intent-based clearing follows MVI best practices perfectly.
134-143: Good preview setup for multi-device support!The use of
@ClodyPreviewwithBasePreviewwrapper provides consistent theming and supports the multi-device preview capability mentioned in the PR objectives.app/src/main/java/com/sopt/clody/core/login/KakaoLoginSdk.kt (2)
17-58: Excellent coroutine implementation with proper error handling!The login implementation demonstrates several best practices:
- Proper cancellation handling with
continuation.isActivecheck- Smart fallback logic for AuthError 302 (KakaoTalk installed but not logged in)
- Clear separation between cancellation and authentication errors
- Automatic fallback to web login when KakaoTalk is unavailable
60-84: Consistent implementation across all SDK methods!The
logoutandunlinkmethods maintain the same error handling pattern aslogin, ensuring consistent behavior throughout the SDK.app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt (3)
22-30: LGTM! Appropriate imports for Mavericks integration.The added imports properly support the migration to Mavericks MVI architecture with lifecycle-aware state collection and standardized preview utilities.
34-79: Excellent implementation of Mavericks MVI pattern!The refactoring correctly implements:
- Proper ViewModel instantiation with
mavericksViewModel()- State collection using
collectAsState()- Intent-based initialization in
LaunchedEffect- Lifecycle-aware side effect collection with
repeatOnStarted- Clean separation of concerns with unidirectional data flow
This follows Mavericks best practices and improves the architecture significantly.
133-139: Good standardization of preview setup!Using
@ClodyPreviewannotation andBasePreviewwrapper ensures consistent theming and preview configurations across the app.app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpContract.kt (3)
7-22: Well-structured state definition!The
SignUpStateproperly captures all UI state with:
- Clear property naming and appropriate default values
- Proper modeling of multi-step flow with the
Stepenum- Complete coverage of UI states (loading, error, validation, checkbox states)
- Correct implementation of
MavericksState
24-36: Comprehensive intent modeling!The sealed class properly models all user interactions with appropriate use of data classes for parameterized intents and objects for simple actions. Covers the complete sign-up flow interactions.
38-41: Clean side effect definition!Good use of sealed interface for side effects. The two defined effects cover the necessary one-time events for navigation and user feedback.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (3)
26-33: Proper Mavericks ViewModel setup!Correctly uses
@AssistedInjectwith initial state parameter and extendsMavericksViewModel. Good abstraction withLoginSdkinterface and appropriate dependency injection.
35-48: Well-implemented intent/side-effect channels!The setup correctly implements:
- Buffered intent channel to prevent event loss
- Flow-based intent processing in init block
- Clean
postIntentAPI for the view layer- Proper lifecycle-aware collection with
launchIn
171-178: Correct Mavericks factory setup!The AssistedFactory interface and companion object properly implement the standard Mavericks + Hilt integration pattern.
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt (4)
25-29: LGTM! Proper Mavericks ViewModel setup.The class declaration correctly implements the Mavericks pattern with
@AssistedInjectand appropriate dependencies.
31-40: Well-structured intent and side effect handling.The channel setup with buffered capacity and the intent collection in the init block properly implement the unidirectional data flow pattern.
62-66: Clean authentication state check.The method correctly validates both access and refresh tokens before determining login state.
89-96: Correct Mavericks factory implementation.The factory setup properly integrates with Hilt for dependency injection.
| if (message.contains("404") || message.contains("유저가 없습니다")) { | ||
| _sideEffects.send(LoginContract.LoginSideEffect.NavigateToSignUp) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using structured error types instead of string matching.
The current approach of checking error messages with contains() is fragile and could break if error messages change.
Consider defining proper error types:
sealed class AuthError {
object UserNotFound : AuthError()
data class GenericError(val message: String) : AuthError()
}Then handle errors based on type rather than string content.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt
around lines 85 to 86, the code currently checks error messages using string
matching with contains(), which is fragile. Refactor this by defining a sealed
class for error types such as AuthError with specific subclasses like
UserNotFound and GenericError. Then update the error handling logic to check the
error type instead of matching strings, improving robustness and
maintainability.
| is LoginContract.LoginSideEffect.ShowError -> { | ||
| // 삐용삐용 여기 뭘로 처리할까? | ||
| } |
There was a problem hiding this comment.
Handle the ShowError side effect case.
The ShowError side effect is caught but not handled, with a TODO comment asking what to do. Since you already have error handling via the FailureDialog for state.errorMessage, this case appears redundant. Consider either removing this side effect type or clarifying its distinct purpose.
Either remove the unused side effect:
- is LoginContract.LoginSideEffect.ShowError -> {
- // 삐용삐용 여기 뭘로 처리할까?
- }Or implement proper handling if it serves a different purpose than state.errorMessage.
📝 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.
| is LoginContract.LoginSideEffect.ShowError -> { | |
| // 삐용삐용 여기 뭘로 처리할까? | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/login/LoginScreen.kt around
lines 52 to 54, the ShowError side effect is caught but not handled, making it
redundant with the existing error handling via state.errorMessage and
FailureDialog. To fix this, either remove the ShowError side effect type
entirely if it is not used elsewhere, or implement distinct handling logic here
if it serves a different purpose than the current errorMessage handling,
ensuring no redundant or unused code remains.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt
Outdated
Show resolved
Hide resolved
| private fun checkVersionAndNavigate() { | ||
| viewModelScope.launch { | ||
| val state = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | ||
| _updateState.value = state | ||
| val updateState = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | ||
| setState { copy(updateState = updateState) } | ||
|
|
||
| if (updateState == AppUpdateState.Latest) { | ||
| delay(1000) | ||
|
|
||
| val isLoggedIn = withState(this@SplashViewModel) { | ||
| it.isUserLoggedIn | ||
| } | ||
|
|
||
| if (isLoggedIn == true) { | ||
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToHome) | ||
| } else { | ||
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToLogin) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to make checkVersionAndNavigate a suspend function.
The current implementation launches a coroutine internally but doesn't wait for completion, which could lead to race conditions if multiple intents are processed quickly.
Additionally:
- The 1-second delay on line 74 needs documentation explaining its purpose
- The
== truecomparison on line 80 is redundant
Apply this refactor to fix the issues:
-private fun checkVersionAndNavigate() {
- viewModelScope.launch {
+private suspend fun checkVersionAndNavigate() {
val updateState = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME)
setState { copy(updateState = updateState) }
if (updateState == AppUpdateState.Latest) {
+ // Add explanation for the delay (e.g., "Delay to show splash screen")
delay(1000)
val isLoggedIn = withState(this@SplashViewModel) {
it.isUserLoggedIn
}
- if (isLoggedIn == true) {
+ if (isLoggedIn) {
_sideEffects.send(SplashContract.SplashSideEffect.NavigateToHome)
} else {
_sideEffects.send(SplashContract.SplashSideEffect.NavigateToLogin)
}
}
- }
}📝 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.
| private fun checkVersionAndNavigate() { | |
| viewModelScope.launch { | |
| val state = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | |
| _updateState.value = state | |
| val updateState = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | |
| setState { copy(updateState = updateState) } | |
| if (updateState == AppUpdateState.Latest) { | |
| delay(1000) | |
| val isLoggedIn = withState(this@SplashViewModel) { | |
| it.isUserLoggedIn | |
| } | |
| if (isLoggedIn == true) { | |
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToHome) | |
| } else { | |
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToLogin) | |
| } | |
| } | |
| } | |
| } | |
| private suspend fun checkVersionAndNavigate() { | |
| val updateState = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | |
| setState { copy(updateState = updateState) } | |
| if (updateState == AppUpdateState.Latest) { | |
| // Delay to show splash screen | |
| delay(1000) | |
| val isLoggedIn = withState(this@SplashViewModel) { | |
| it.isUserLoggedIn | |
| } | |
| if (isLoggedIn) { | |
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToHome) | |
| } else { | |
| _sideEffects.send(SplashContract.SplashSideEffect.NavigateToLogin) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt
between lines 68 and 87, refactor the checkVersionAndNavigate function to be a
suspend function instead of launching a coroutine internally. Remove the
internal viewModelScope.launch and make the caller responsible for coroutine
scope. Add a comment explaining the purpose of the 1-second delay on line 74.
Replace the redundant "if (isLoggedIn == true)" check with a simpler "if
(isLoggedIn)" condition.
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
후하 .. 레전드 난이도 급상승
이해 못한게 아직 남아있지만 .. 작업을 위해 일단 어프하고 나중에 찾아가겠습니다 ㅜ
There was a problem hiding this comment.
[3]
타 프로젝트에서 MVI를 사용하면서 고민했던 문제 중 하나인데, Intent와 SideEffect를 구분하는 기준이 궁금합니다!
유저가 클릭하는 등의 인터렉션이 없어도 뷰를 진입하면서 시스템 내부적으로 진행되는 동작들이 있을텐데(ex. 자동로그인 검증 등) 이런 부분에서 유저가 의도하지 않아도 일어나는 것들도 Intent일까? 라는 궁금증이 들었기 때문인데요,
나름대로 내린 결론은 상태의 변경 여부 였습니다. State를 변경하는 일체의 동작들은 Intent로 규정하고, 그 외 동작들은 SideEffect로 규정하였습니다. 어떻게 생각하시나요 😀
There was a problem hiding this comment.
음.. "상태 변경 여부"를 기준으로 Intent, SideEffect를 구분하는 방법 좋습니다.
MVI에서는 Intent가 State를 변경하기 위한 주요 진입점 중 하나고,(Intent - reduce - state)
요 구조를 통해서 상태 변경의 흐름을 추적하고 예측 가능하게 만드는 것이 이 디자인 패턴의 목적이죠.
Toast, SnackBar, Navigate Func 같이 일회성 이벤트고 state를 남길 필요가 없는 행위들을 SideEffect로 구분하는게 적절하다고 생각합니다.
이런 동작까지 State로 표현하면 오히려 관리성 측면에서 복잡해지고 명확성이 떨어질 수 있으니깐요.
결론적으로, 유저의 '의도' 가 없는 시스템 동작이라도 state를 변화시키는 지의 여부를 중심으로 Intent, SideEffect를 나누는 기준은 매우 좋은 접근 방법이라고 생각합니다ㅎ
There was a problem hiding this comment.
[3]
몇 가지 궁금중 남깁니다 ..!
- LaunchedEffect에서 viewModel을 변수로 받는 이유? Unit으로 해도 충분하지 않을까 생각합니다. ViewModel이 변경되는 일이 거의 없을 것 같기도 하고, 바뀔 때 기존 block을 취소하고 새로 실행되는 과정에서 발생하는 문제점은 없을까요? 또한 SideEffect를 collect 하는 것도 한번 실행해놓으면 자동으로 변경될 때마다 emit을 받아오는 것으로 알고 있어서 viewModel을 구독해야하는 이유가 궁금합니다!
- 아래와 같이 앱 종료나 마켓 이동 등의 동작도 SideEffect로 관리하는 것이 MVI 적으로 일관성 있지 않을까 생각합니다!
// ViewModel에서
_sideEffects.send(SplashContract.SplashSideEffect.NavigateToMarketAndFinish)
_sideEffects.send(SplashContract.SplashSideEffect.FinishApp)
// View에서
when (effect) {
is NavigateToMarketAndFinish -> AppUpdateUtils.navigateToMarketAndFinish(context as Activity)
is FinishApp -> (context as Activity).finishAffinity()
}
There was a problem hiding this comment.
- viewmodel을 key로 설정한 이유는 ViewModel Instance가 교체되는 상황에서도 초기화 로직을 정확히 재실행 하기 위해서 사용했씁니다.
예를들어서 프로세스 restore, nav pop 같은 경우 mavericksViewmodel()가 new instance 가능성이 있기 때문이죠.
이때 Viewmodel 내부의 state는 초기화가 되는데, 외부에서 실행하던 postIntent나 SideEffect같은 수집 로직이 재실행 안된다면? 어떻게 될까요?(나도 잘 모름..)
Splash처럼 진입점 역할을 하는 View단에서는 혹시나 흐름이 꼬인다면 사용자 경험에 영항이 갈수가 있어서 ViewModel이 재생성 되도 초기 kintent를 정확히 전달하고 , SideEffect 수집이 resumse되는게 중요하다고 생각했습니다.
요러한 사고 과정을 통해 viewModel Instance를 key로 사용해서 viewModel이 바뀌는 경우(configuration change, death & recreation 등등)에도 LauchedEffect가 재실행되게 명시했습니다.
- 오호 좋습니다잉 관리하는 측면이 훨씬 좋아보이네요
There was a problem hiding this comment.
[3]
Factory 의존성 부분이 너무 어려워옹.. ㅠㅠㅠㅠ
뷰모델에서 공통되게 사용될 부분들을 템플릿화 시켜서 BaseMavericksViewModel을 만들어보는 것은 어떨까요?
| private suspend fun handleIntent(intent: LoginContract.LoginIntent) { | ||
| when (intent) { | ||
| is LoginContract.LoginIntent.LoginWithKakao -> loginWithKakao(intent.context) | ||
| LoginContract.LoginIntent.ClearError -> setState { copy(errorMessage = null) } |
There was a problem hiding this comment.
[3]
sealed class라 else문이 불필요하고 마지막에 이렇게 해도 되긴하는군요! 하지만 통일성을 위해 is를 붙이는 것은 어떨까요 ?
| val errorMessage: String? = null, | ||
| val serviceChecked: Boolean = false, | ||
| val privacyChecked: Boolean = false, | ||
| val allChecked: Boolean = false, |
There was a problem hiding this comment.
[3] 아래처럼 derivedState 활용하면 조금 더 효율적으로 처리할 수 있지 않을까 슥 건의해봅니다 ㅋ.ㅋ
val allChecked: Boolean
get() = serviceChecked && privacyChecked
…함수분리 - Soft/Hard 업데이트 시 직접 처리하던 market 이동 및 종료 로직을 SideEffect로 분리 - HandleHardUpdate, HandleSoftUpdateConfirm Intent 추가 - NavigateToMarket, NavigateToMarketAndFinish, FinishApp SideEffect 추가 - ViewModel 내부에서 Intent를 처리하도록 구조 개선 - SplashRoute에서 UI 이벤트 발생 시 ViewModel에 Intent 전달하도록 변경
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt (3)
69-85: Address the existing refactoring suggestion.The previous review comment about making this function suspend and removing redundant comparisons is still valid and should be addressed.
1-110: Consider creating a BaseMavericksViewModel for common functionality.As suggested in the previous review, creating a base class could reduce boilerplate code across ViewModels. The current pattern of intent handling with Channels and side effects will likely be repeated in other ViewModels.
Consider creating a base class like this:
abstract class BaseMavericksViewModel<S : MavericksState, I, SE>( initialState: S ) : MavericksViewModel<S>(initialState) { private val _intents = Channel<I>(BUFFERED) private val _sideEffects = Channel<SE>(BUFFERED) val sideEffects = _sideEffects.receiveAsFlow() init { _intents .receiveAsFlow() .onEach(::handleIntent) .launchIn(viewModelScope) } fun postIntent(intent: I) { viewModelScope.launch { _intents.send(intent) } } protected suspend fun sendSideEffect(sideEffect: SE) { _sideEffects.send(sideEffect) } protected abstract suspend fun handleIntent(intent: I) }
69-85: Address previously identified issues in checkVersionAndNavigate.The same issues identified in previous reviews still exist:
- The function launches a coroutine internally, which could lead to race conditions
- The 1-second delay lacks documentation
- The
== truecomparison is redundant
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt (1)
9-12: Consider providing default values for better state management.While nullable properties allow for "unknown" initial states, consider if explicit default values would provide better predictability:
data class SplashState( - val isUserLoggedIn: Boolean? = null, - val updateState: AppUpdateState? = null, + val isUserLoggedIn: Boolean = false, + val updateState: AppUpdateState = AppUpdateState.Latest, ) : MavericksStateThis would eliminate null checks throughout the codebase and provide clearer initial state semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt
- app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpContract.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (20)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt (5)
3-5: Import organization looks good.The imports are properly organized with Android framework imports first, followed by Mavericks imports, and then domain model imports.
9-12: Well-structured state definition with nullable properties.The
SplashStatefollows MVI principles correctly by implementingMavericksState. The nullable properties (isUserLoggedInandupdateState) appropriately represent the loading states during initialization.
14-19: Intent design effectively captures user interactions and system events.The sealed class structure provides type safety and covers all necessary splash screen actions:
InitSplashwith start intent parameter for initialization- Update-related intents for handling app version checks
- Clear action for state cleanup
21-27: Side effects provide clear navigation outcomes.The sealed interface properly represents one-time events that don't need to be stored in state:
- Navigation actions to different screens
- App lifecycle actions (finish, market navigation)
This aligns well with the MVI principle of separating persistent state from ephemeral side effects.
7-28: Well-structured MVI contract implementation.The contract follows MVI principles effectively with clear separation of State, Intent, and SideEffect. The use of sealed interface for SideEffect is a modern Kotlin best practice.
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt (15)
3-7: Mavericks imports are properly organized.The new Mavericks-related imports are correctly added to support the MVI architecture migration.
25-29: Constructor successfully migrated to Mavericks pattern.The
@AssistedInjectconstructor with initial state parameter properly follows Mavericks ViewModel creation pattern. The inheritance change fromViewModeltoMavericksViewModel<SplashContract.SplashState>is correct.
31-40: Intent handling architecture is well-implemented.The Channel-based intent handling with buffered capacity provides:
- Thread-safe intent processing
- Proper separation of side effects from state
- Clean unidirectional data flow
The initialization block correctly sets up the intent processing pipeline.
42-44: Intent posting method provides clean API.The
postIntentmethod offers a clean interface for sending intents while properly handling coroutine scope management.
46-53: Intent dispatcher effectively handles all intent types.The
handleIntentmethod provides a centralized dispatch point for all intents with proper suspend function support for async operations.
55-61: Initialization logic properly migrated to intent handling.The
handleInitSplashmethod correctly processes FCM intent extras and triggers auto-login and version checking. The migration from lifecycle-based initialization to intent-driven approach improves testability.
63-67: Auto-login logic properly uses Mavericks state management.The
attemptAutoLoginmethod correctly usessetStateto update the immutable state, replacing the previous mutable StateFlow approach.
87-93: Hard update handling provides clear user choice outcomes.The method properly handles both user confirmation scenarios:
- Navigate to market and finish app on confirmation
- Finish app immediately on rejection
95-97: Soft update handling is straightforward and appropriate.The method correctly sends side effect to navigate to market for optional updates.
99-101: Update state clearing is properly implemented.The method correctly resets the update state to
Latestusing Mavericks state management.
103-110: Mavericks factory integration is correctly implemented.The
@AssistedFactoryinterface and companion object properly integrate with Hilt for Mavericks ViewModel creation. This follows the recommended Mavericks-Hilt integration pattern.
25-29: Excellent adoption of Mavericks pattern.The migration to Mavericks with assisted injection is well-implemented, providing proper dependency injection and initial state management.
31-44: Clean intent handling implementation.The Channel-based intent handling with buffered channels and flow collection provides a robust foundation for unidirectional data flow. The separation of concerns between intent posting and handling is well-structured.
87-101: Well-implemented update handling methods.The new handler methods for update-related intents follow the MVI pattern correctly, using suspend functions and emitting appropriate side effects for different update scenarios.
103-109: Proper Mavericks factory implementation.The AssistedFactory and companion object setup correctly integrates with Hilt and Mavericks for dependency injection and ViewModel creation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (5)
1-33: LGTM: Proper Mavericks setup and dependency injection.The migration to Mavericks MVI architecture is well-implemented with correct imports, assisted injection, and constructor setup. The dependency abstraction through
LoginSdkandFcmTokenProviderinterfaces improves modularity as intended.
35-48: LGTM: Clean intent and side effect channel setup.The channel-based intent handling follows Mavericks best practices. The buffered channels and flow conversion are correctly implemented for handling UI intents and emitting side effects.
50-62: LGTM: Centralized intent handling pattern.The intent dispatching through a centralized
handleIntentfunction promotes maintainability and follows the MVI pattern correctly. All intent types are properly handled.
64-115: LGTM: State management logic is sound.All the intent handlers correctly use
setStateto update the immutable state. The nickname validation is appropriately synchronous, and the state transitions between terms and nickname steps are properly managed.
150-162: LGTM: Proper validation logic and Mavericks factory setup.The nickname validation regex correctly handles Korean, English, and numeric characters with appropriate length constraints. The AssistedFactory and companion object setup follows Mavericks patterns correctly for Hilt integration.
| private fun signUp(context: Context) { | ||
| viewModelScope.launch { | ||
| _nickname | ||
| .debounce(NICKNAME_VALIDATION_DELAY) | ||
| .collectLatest { nickname -> | ||
| validateNickname(nickname) | ||
| } | ||
| val state = withState(this@SignUpViewModel) { it } | ||
| if (!networkUtil.isNetworkAvailable()) { | ||
| setState { copy(errorMessage = "네트워크 연결을 확인해주세요.") } | ||
| return@launch | ||
| } | ||
| setState { copy(isLoading = true) } | ||
| loginSdk.login(context).fold( | ||
| onSuccess = { accessToken -> | ||
| launch { | ||
| val fcm = fcmTokenProvider.getToken().orEmpty() | ||
| val req = SignUpRequestDto("kakao", state.nickname, fcm) | ||
| authRepository.signUp("Bearer ${accessToken.value}", req).fold( | ||
| onSuccess = { | ||
| tokenRepository.setTokens(it.accessToken, it.refreshToken) | ||
| _sideEffects.send(SignUpContract.SignUpSideEffect.NavigateToTimeReminder) | ||
| }, | ||
| onFailure = { | ||
| setState { copy(errorMessage = it.message ?: "알 수 없는 오류") } | ||
| }, | ||
| ) | ||
|
|
||
| setState { copy(isLoading = false) } | ||
| } | ||
| }, | ||
| onFailure = { | ||
| setState { copy(errorMessage = it.message ?: "로그인 실패", isLoading = false) } | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary nested coroutine launch.
The nested launch block inside the onSuccess callback is unnecessary and makes the code structure confusing. Since the parent function is already a suspend function, you can directly execute the sign-up logic without additional launching.
Apply this diff to simplify the coroutine structure:
loginSdk.login(context).fold(
onSuccess = { accessToken ->
- launch {
- val fcm = fcmTokenProvider.getToken().orEmpty()
- val req = SignUpRequestDto("kakao", state.nickname, fcm)
- authRepository.signUp("Bearer ${accessToken.value}", req).fold(
- onSuccess = {
- tokenRepository.setTokens(it.accessToken, it.refreshToken)
- _sideEffects.send(SignUpContract.SignUpSideEffect.NavigateToTimeReminder)
- },
- onFailure = {
- setState { copy(errorMessage = it.message ?: "알 수 없는 오류") }
- },
- )
-
- setState { copy(isLoading = false) }
- }
+ val fcm = fcmTokenProvider.getToken().orEmpty()
+ val req = SignUpRequestDto("kakao", state.nickname, fcm)
+ authRepository.signUp("Bearer ${accessToken.value}", req).fold(
+ onSuccess = {
+ tokenRepository.setTokens(it.accessToken, it.refreshToken)
+ _sideEffects.send(SignUpContract.SignUpSideEffect.NavigateToTimeReminder)
+ },
+ onFailure = {
+ setState { copy(errorMessage = it.message ?: "알 수 없는 오류") }
+ },
+ )
+ setState { copy(isLoading = false) }
},
onFailure = {
setState { copy(errorMessage = it.message ?: "로그인 실패", isLoading = false) }
},
)📝 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.
| private fun signUp(context: Context) { | |
| viewModelScope.launch { | |
| _nickname | |
| .debounce(NICKNAME_VALIDATION_DELAY) | |
| .collectLatest { nickname -> | |
| validateNickname(nickname) | |
| } | |
| val state = withState(this@SignUpViewModel) { it } | |
| if (!networkUtil.isNetworkAvailable()) { | |
| setState { copy(errorMessage = "네트워크 연결을 확인해주세요.") } | |
| return@launch | |
| } | |
| setState { copy(isLoading = true) } | |
| loginSdk.login(context).fold( | |
| onSuccess = { accessToken -> | |
| launch { | |
| val fcm = fcmTokenProvider.getToken().orEmpty() | |
| val req = SignUpRequestDto("kakao", state.nickname, fcm) | |
| authRepository.signUp("Bearer ${accessToken.value}", req).fold( | |
| onSuccess = { | |
| tokenRepository.setTokens(it.accessToken, it.refreshToken) | |
| _sideEffects.send(SignUpContract.SignUpSideEffect.NavigateToTimeReminder) | |
| }, | |
| onFailure = { | |
| setState { copy(errorMessage = it.message ?: "알 수 없는 오류") } | |
| }, | |
| ) | |
| setState { copy(isLoading = false) } | |
| } | |
| }, | |
| onFailure = { | |
| setState { copy(errorMessage = it.message ?: "로그인 실패", isLoading = false) } | |
| }, | |
| ) | |
| } | |
| } | |
| private fun signUp(context: Context) { | |
| viewModelScope.launch { | |
| val state = withState(this@SignUpViewModel) { it } | |
| if (!networkUtil.isNetworkAvailable()) { | |
| setState { copy(errorMessage = "네트워크 연결을 확인해주세요.") } | |
| return@launch | |
| } | |
| setState { copy(isLoading = true) } | |
| loginSdk.login(context).fold( | |
| onSuccess = { accessToken -> | |
| val fcm = fcmTokenProvider.getToken().orEmpty() | |
| val req = SignUpRequestDto("kakao", state.nickname, fcm) | |
| authRepository.signUp("Bearer ${accessToken.value}", req).fold( | |
| onSuccess = { | |
| tokenRepository.setTokens(it.accessToken, it.refreshToken) | |
| _sideEffects.send(SignUpContract.SignUpSideEffect.NavigateToTimeReminder) | |
| }, | |
| onFailure = { | |
| setState { copy(errorMessage = it.message ?: "알 수 없는 오류") } | |
| }, | |
| ) | |
| setState { copy(isLoading = false) } | |
| }, | |
| onFailure = { | |
| setState { copy(errorMessage = it.message ?: "로그인 실패", isLoading = false) } | |
| }, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt
between lines 117 and 148, remove the nested launch block inside the onSuccess
callback of loginSdk.login. Since the parent coroutine scope is already
launched, directly execute the sign-up logic sequentially without launching a
new coroutine. This simplifies the coroutine structure and avoids unnecessary
nesting.
📌 ISSUE
closed #252
📄 Work Description
1. Mavericks MVI 도입 및 초기 셋업
2. Splash / Login / SignUp 흐름에 MVI 구조 반영
3. 기존 screen 분리 구조를 SignUpRoute 단일화로 통합
4. 핵심 의존성 추상화 (FcmTokenProvider, LoginSdk)
5. ViewModel 개선 및 SOC 기반 의존성 주입 적용
6. BasePreview 도입
✨ PR Point
코드 중 이해가 안되는 부분이 있으면 우선 Maverick 정리 이거 한번 확인해주시면 감사하겠습니다잉
커밋 내용에 추가 설명이 있으니 한 번 확인해보는 것도 좋을지도?"
📸 ScreenShot/Video
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores