[REFACTOR/#260] 네비게이션 구조를 개선합니다.#261
Conversation
- Nickname 및 TermsOfService 화면 내 네비게이션 함수명을 새로운 구조에 맞게 수정
|
""" WalkthroughThis change refactors the navigation architecture by replacing string-based routes and Navigator classes with a sealed Changes
Sequence Diagram(s)sequenceDiagram
participant MainActivity
participant ClodyApp
participant ClodyNavHost
participant NavController
participant Screen
MainActivity->>ClodyApp: Launch app with startIntent
ClodyApp->>ClodyNavHost: Provide appState, startIntent
ClodyNavHost->>NavController: Set up NavHost with Route sealed interface
NavController->>Screen: Navigate using type-safe Route (e.g., Route.Home, Route.Login)
Screen->>NavController: Trigger navigation via explicit lambda (e.g., navigateToHome())
NavController->>ClodyNavHost: Handle navigation/back safely with safePopBackStack
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 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: 1
🔭 Outside diff range comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (1)
78-85: 🛠️ Refactor suggestionReset
writeDiaryStateafter navigation to prevent duplicate navigations on recomposition
LaunchedEffect(writeDiaryState)fires every time the value changes toSuccessorNoReply.
If the user returns to this screen while the state is stillSuccess, the effect will run again and immediately navigate away, creating an unintended loop.- when (writeDiaryState) { - is WriteDiaryState.Success -> navigateToReplyLoading(year, month, date) - is WriteDiaryState.NoReply -> navigateToHome(year, month) + when (writeDiaryState) { + is WriteDiaryState.Success -> { + navigateToReplyLoading(year, month, date) + viewModel.resetWriteDiaryState() // <-- consume once + } + is WriteDiaryState.NoReply -> { + navigateToHome(year, month) + viewModel.resetWriteDiaryState() + }Add a small helper in the
ViewModel(e.g.,fun resetWriteDiaryState()) that sets the state back toIdle.
This guarantees single-shot navigation even after process death/restoration.
🧹 Nitpick comments (9)
app/src/main/java/com/sopt/clody/presentation/utils/navigation/NavControllerExtensions.kt (1)
8-12: Propagate the result ofpopBackStack()(or document why it is ignored)
popBackStack()returns aBooleanthat signals whether the pop actually happened.
Silently discarding this value removes the caller’s ability to react to an unsuccessful back-navigation (e.g., by showing a dialog or finishing the activity).-fun NavController.safePopBackStack() { - if (currentBackStackEntry?.lifecycle?.currentState == androidx.lifecycle.Lifecycle.State.RESUMED) { - popBackStack() - } -} +fun NavController.safePopBackStack(): Boolean { + return if ( + currentBackStackEntry?.lifecycle?.currentState == + androidx.lifecycle.Lifecycle.State.RESUMED + ) { + popBackStack() + } else { + false + } +}Returning the flag keeps parity with the original API and lets call-sites make informed decisions.
If you intentionally ignore the result, add a KDoc note explaining that choice.app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (1)
55-61: Minor naming inconsistency:datevsdayThe public API now uses
date, but deeper in the hierarchy the parameter is still forwarded asday(day = dateat line 105).
Consider renaming the parameter inWriteDiaryScreenas well (or usingdayeverywhere) to avoid confusion for future maintainers.app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (1)
14-14: Cross-feature import: consider relocating shared state
ImportingSignInStatefrom theloginpackage into signup introduces a coupling between features. You may want to moveSignInStateto a sharedauthpackage (e.g.,com.sopt.clody.presentation.ui.auth) to better express its cross-cutting nature.app/src/main/java/com/sopt/clody/presentation/utils/navigation/ReplyStatus.kt (1)
5-8: Add documentation for enum values
While the three statuses are clear, consider adding KDoc to each enum constant or the class itself to explain their semantic meaning (e.g., when each state is used).app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (1)
23-27: Possible duplication of push notification trackingThe LaunchedEffect block that tracks push notification events appears to duplicate functionality already present in the SplashRoute composable (as seen in the relevant code snippet). Consider consolidating this logic to avoid duplicate event tracking.
Consider either:
- Removing this tracking from SplashRoute if it's more appropriate at the app level, or
- Moving this logic exclusively to SplashRoute if the event should only be tracked during the splash screen phase
-LaunchedEffect(startIntent) { - if (startIntent.hasExtra("google.message_id")) { - AmplitudeUtils.trackEvent(AmplitudeConstraints.ALARM) - } -}app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt (1)
115-115: Consider updating parameter type for consistency.The
onClickReplyDiaryparameter inDiaryListScreenstill accepts aStringtype for the reply status, but it's being called with an enum value (Route.ReplyLoading.ReplyLoadingFrom.DIARY_LIST). For full type safety and consistency with the refactoring goals:-onClickReplyDiary: (Int, Int, Int, String) -> Unit, +onClickReplyDiary: (Int, Int, Int, Route.ReplyLoading.ReplyLoadingFrom) -> Unit,app/src/main/java/com/sopt/clody/presentation/ui/auth/login/LoginScreen.kt (2)
28-28: Consider renaming the ViewModel for consistency.The screen has been renamed from SignUp to Login, but it still uses
SignUpViewModel. Consider renaming the ViewModel to match the screen's purpose for better code organization and clarity.
51-55: Consider updating resource strings for consistency.Since this screen was renamed from SignUp to Login, check if resource strings like
R.string.signup_btn_kakaoshould also be updated to maintain naming consistency throughout the app.app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigation.kt (1)
11-28: Clean type-safe navigation implementation with multiple navigation paths.The implementation correctly separates navigation concerns from UI logic by passing navigation lambdas to the screen composable. However, there's a parameter naming inconsistency.
The navigation lambda is defined with parameter
daybut the WriteDiaryRoute usesdate:- navigateToReplyLoading: (year: Int, month: Int, day: Int) -> Unit, + navigateToReplyLoading: (year: Int, month: Int, date: Int) -> Unit,This would improve consistency with how the parameter is named in the WriteDiaryRoute composable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/sopt/clody/ClodyApplication.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/GuideScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/navigation/GuideNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/login/LoginScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/login/SignInState.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/login/navigation/LoginNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/navigation/AuthNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/navigation/AuthNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/NicknameScreen.kt(4 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(2 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/timereminder/TimeReminderNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(5 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyAppState.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/main/MainActivity.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/navigatior/MainNavHost.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt(5 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/WebViewScreen.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/navigation/SplashNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavGraph.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigator.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/NavControllerExtensions.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/ReplyStatus.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt(1 hunks)
💤 Files with no reviewable changes (14)
- app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/navigation/AuthNavGraph.kt
- app/src/main/java/com/sopt/clody/presentation/ui/auth/navigation/AuthNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/navigatior/MainNavHost.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigator.kt
- app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigator.kt
🧰 Additional context used
🧬 Code Graph Analysis (19)
app/src/main/java/com/sopt/clody/presentation/ui/splash/navigation/SplashNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt (1)
SplashRoute(33-84)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/navigation/GuideNavigation.kt (1)
navigateToGuide(20-24)
app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
HomeRoute(39-116)
app/src/main/java/com/sopt/clody/presentation/ui/main/MainActivity.kt (3)
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/ClodyAppState.kt (1)
rememberClodyAppState(26-29)app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (1)
ClodyApp(18-39)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt (1)
navigateToReplyLoading(33-45)app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
navigateToHome(40-47)
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt (1)
navigateToWebView(81-86)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (1)
TimeReminderRoute(49-116)
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/navigation/LoginNavigation.kt (1)
navigateToLogin(22-26)
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/navigation/LoginNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/LoginScreen.kt (1)
LoginRoute(34-55)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (1)
WriteDiaryRoute(53-114)
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt (1)
DiaryListRoute(30-92)
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/navigation/GuideNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/GuideScreen.kt (1)
GuideRoute(47-52)
app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
navigateToHome(40-47)
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/LoginScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
navigateToHome(40-47)
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt (3)
app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (1)
navigateToHome(40-47)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt (1)
navigateToDiaryList(26-31)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt (1)
navigateToReplyDiary(28-36)
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt (1)
ReplyLoadingRoute(55-135)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/TermsOfServiceScreen.kt (1)
TermsOfServiceRoute(41-66)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/NicknameScreen.kt (1)
NicknameRoute(51-99)
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/TermsOfServiceScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/navigation/LoginNavigation.kt (1)
navigateToLogin(22-26)
app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt (1)
ReplyDiaryRoute(46-97)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (94)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (1)
80-82: Check the third parameter fornavigateToHome
NavController.navigateToHome()accepts(year, month, selectedDay: Int?).
The lambda wired into this screen takes only two arguments, soselectedDayfalls back to its default (LocalDate.now().dayOfMonth).
Confirm this is intentional; otherwise today’s date will be selected even when the diary being edited is on another day.app/src/main/java/com/sopt/clody/presentation/ui/auth/login/SignInState.kt (1)
1-1: Verify package rename for SignInState
The package was changed fromsignuptologinto better reflect its context. Ensure all imports and references toSignInStateacross the codebase have been updated accordingly.app/src/main/java/com/sopt/clody/ClodyApplication.kt (1)
11-11: Rename application class toClodyApplication
Updating the class name fromClodyApptoClodyApplicationaligns with standard Android naming conventions and avoids confusion with the newClodyAppcomposable.app/src/main/AndroidManifest.xml (1)
11-11: Align manifest with renamed application class
Theandroid:nameattribute was updated to.ClodyApplication. Confirm this matches the package declaration (com.sopt.clody) and that no leftover references to the old class remain.app/src/main/java/com/sopt/clody/presentation/utils/navigation/ReplyStatus.kt (1)
1-4: Package and import are correctly declared
The new enum resides inpresentation.utils.navigation, which is a suitable location for shared navigation types. Thekotlinx.serialization.Serializableimport is appropriate for saving/restoring route arguments.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (2)
39-40: Navigation refactoring: Lambda instead of NavigatorThe parameter type has been changed from a
SettingNavigatorinstance to a simple lambda functionnavigateToPrevious: () -> Unit. This aligns with the PR objectives to improve navigation structure through modularization.
77-77: Navigation call updated to use lambdaThe navigation callback has been updated to use the lambda directly instead of calling a method on the navigator instance. This simplifies the code and reduces coupling between components.
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (2)
50-51: Navigation refactoring: Lambda instead of NavigatorThe parameter type has been changed from an
AuthNavigatorinstance to a simple lambda functionnavigateToGuide: () -> Unit. This is consistent with the navigation refactoring across the app.
83-83: Navigation call updated to use lambdaThe navigation callback has been updated to directly invoke the provided lambda instead of calling a method on the navigator. This aligns with the overall refactoring to simplify navigation and improve modularity.
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/WebViewScreen.kt (2)
21-23: Navigation refactoring: Lambda instead of NavigatorThe parameter type has been changed from a
SettingNavigatorinstance to a simple lambda functionnavigateToPrevious: () -> Unit. This is consistent with the navigation refactoring pattern across the app.
26-28: Navigation call updated to use lambdaThe onClickBack parameter now directly passes the navigateToPrevious lambda instead of calling a method on the navigator instance. This reduces coupling and improves composability.
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/GuideScreen.kt (2)
48-50: Navigation refactoring: Lambda instead of NavigatorThe parameter type has been changed from an
AuthNavigatorinstance to a simple lambda functionnavigateToHome: () -> Unit. This change is consistent with the PR's goal of improving navigation structure.
51-51: Simplified navigation call using lambdaThe navigation handling has been simplified to directly pass the lambda to the GuideScreen composable, removing the dependency on the Navigator class. This improves modularity and makes the code more maintainable.
app/src/main/java/com/sopt/clody/presentation/ui/splash/navigation/SplashNavigation.kt (1)
9-21: Well-structured navigation extension functionThe
splashScreenextension function follows a clear and consistent pattern for navigation setup, taking the necessary parameters to handle both the splash screen's initialization and its navigation paths. The use of typedRoute.Splashwith the generic composable function ensures type safety.The implementation correctly separates navigation concerns by passing navigation lambdas rather than concrete implementations, which enhances testability and maintainability.
app/src/main/java/com/sopt/clody/presentation/ui/auth/guide/navigation/GuideNavigation.kt (2)
10-18: Good implementation of navigation graph builderThe
guideScreenextension function correctly sets up a composable destination for the Guide route using type-safe navigation. The pattern of accepting navigation lambdas as parameters is consistent with the project's new navigation architecture.
20-24: Clean navigation extension implementationThe
navigateToGuideextension function provides a convenient way to navigate to the Guide screen with optional navigation options. Using a default empty lambda fornavOptionsis a good practice that allows for both simple navigation calls and customized navigation when needed.app/src/main/java/com/sopt/clody/presentation/ui/auth/login/navigation/LoginNavigation.kt (2)
10-20: Well-structured login screen navigationThe
loginScreenextension function correctly sets up the login screen composable with the necessary navigation paths. The implementation follows the consistent pattern seen in other navigation files, accepting navigation lambdas for different destinations.
22-26: Clean navigation extension implementationThe
navigateToLoginextension function provides a straightforward way to navigate to the Login screen with optional navigation options. The default empty lambda fornavOptionsallows for flexible navigation configuration.app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyApp.kt (2)
13-22: Good root composable structure and documentationThe
ClodyAppcomposable is well-documented and clearly defines its role as the root composable for the application. The parameters are appropriately named and follow a logical structure, accepting the app state and the start intent.
29-38: Clean Scaffold implementationThe Scaffold setup is clean and follows proper Compose patterns. The use of
fillMaxSize()ensures the Scaffold takes up the entire available space, and theClodyNavHostis correctly configured with the necessary parameters and padding.app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/TermsOfServiceScreen.kt (4)
1-1: Package name change improves organization.Relocating from
auth.screentoauth.signupmakes the code structure more intuitive by grouping related signup screens together.
42-45: Good refactoring to lambda-based navigation.Replacing the
AuthNavigatorparameter with explicit navigation lambda parameters improves:
- Testability - easier to mock navigation actions
- Readability - clearer navigation intent
- Decoupling - screen doesn't depend on navigator implementation
55-61: Clean implementation of back press handling.The back press handler now calls the navigation lambda directly, simplifying the code and making the navigation flow more explicit.
62-65: Improved screen parameter passing.Passing navigation lambdas directly to the screen composable improves clarity about which navigation actions are available to this screen.
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt (3)
27-27: Good migration to type-safe Route import.Replacing string-based navigation with the sealed
Routeinterface improves type safety and helps catch navigation errors at compile time.
34-35: Well-structured navigation parameter types.Using typed parameters and enum values (
Route.ReplyLoading.ReplyLoadingFrom) instead of strings provides better type safety and self-documentation of the navigation API.
85-90: Consistent implementation of navigation calls.The implementation correctly uses the navigation lambdas with the new enum parameter approach.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/NicknameScreen.kt (4)
1-1: Package reorganization improves code structure.Moving from
auth.screentoauth.signupcreates a more logical organization by grouping related signup functionality.
52-55: Well-implemented lambda-based navigation.The refactoring to use navigation lambdas makes the component more modular and testable compared to the previous approach with the navigator object.
69-69: Clean implementation of back navigation.The back navigation is correctly implemented with the lambda approach, making it more explicit and easier to understand.
78-78: Properly implemented success navigation.Navigation to reminder screen is correctly implemented with the lambda approach in the success handler.
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderNavigation.kt (3)
1-8: Well-structured navigation module.The file organization and imports are clean and focused, providing just what's needed for the navigation functionality.
9-17: Excellent NavGraphBuilder extension implementation.This extension function follows best practices:
- Clear parameter naming for the navigation destination
- Uses type-safe
composable<Route.TimeReminder>approach with the sealed interface- Cleanly forwards the navigation lambda to the screen
19-23: Well-implemented NavController extension.The
navigateToTimeReminderextension function:
- Provides a clean, standardized navigation API
- Uses the type-safe
Routeobject- Offers flexibility with optional navigation options
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/SettingScreen.kt (2)
29-34: Excellent transition to a more modular navigation approach.The change from using a navigator object to explicit navigation lambdas improves modularity and testability. This approach clearly separates navigation concerns from the screen's UI logic.
44-49: Clean implementation of navigation callbacks.The navigation callbacks are well-implemented and maintain good separation of concerns. The use of a specific lambda for WebView navigation with URL encoding is particularly well-structured.
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementScreen.kt (3)
38-41: Good conversion to lambda-based navigation.The explicit navigation lambda parameters improve code clarity and testability compared to the previous navigator approach.
65-69: Type-safe account revocation navigation.The navigation is now handled in a type-safe manner through a dedicated lambda, which is an improvement over the previous approach.
71-75: Consistent navigation pattern.This maintains the same pattern as used for account revocation, providing good consistency across the codebase.
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt (3)
34-38: Good separation of navigation concerns.The change from using a navigator to explicit navigation lambdas enhances testability and modularity. Including the
startIntentparameter allows for handling deep links or push notifications directly in the composable.
45-50: Well-implemented push notification tracking.This new feature correctly tracks when the app is launched from a push notification. The implementation is clean and well-placed in the splash screen, which is the app's entry point.
52-61: Clean conditional navigation logic.The navigation logic is concise and makes good use of the lambda parameters, replacing what was likely more complex navigator code previously.
app/src/main/java/com/sopt/clody/presentation/ui/auth/login/LoginScreen.kt (2)
35-38: Good lambda-based navigation pattern.The explicit navigation lambdas improve code clarity and testability compared to using a navigator object.
43-49: Clean navigation state handling.The
LaunchedEffectblock effectively observes the sign-in state and triggers the appropriate navigation action. The code is clean and follows a consistent pattern with other screens.app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyAppState.kt (2)
15-20: Well-structured app state management with Compose stability optimization.The
@Stableannotation is correctly applied to ensure optimal recomposition behavior. The class is designed for extensibility with clear documentation indicating future state management possibilities.
26-29: Clean implementation of state remembering function.The
rememberClodyAppStatecomposable function properly usesrememberto preserve state across recompositions and has appropriate documentation. The default parameter for NavController simplifies usage in most cases.app/src/main/java/com/sopt/clody/presentation/ui/replydiary/navigation/ReplyDiaryNavigation.kt (2)
12-26: Type-safe navigation implementation using Compose Navigation and extension functions.This implementation cleanly extracts route parameters and provides them to the screen composable. The separation of navigation logic from UI logic improves maintainability and testability.
28-36: Well-designed navigation extension function with sensible defaults.The
navigateToReplyDiaryfunction has appropriate parameters with a default value forreplyStatusand optional navigation options. This makes the API both flexible and easy to use in common cases.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt (2)
10-24: Type-safe navigation configuration with clear parameter passing.The implementation correctly extracts route parameters and passes appropriate navigation callbacks to the composable. Using strongly typed parameters like
Route.ReplyLoading.ReplyLoadingFrominstead of string literals improves type safety.
26-31: Simple and direct navigation function.The
navigateToDiaryListextension function provides a clean way to navigate to the diary list with the required parameters.app/src/main/java/com/sopt/clody/presentation/ui/writediary/navigation/WriteDiaryNavigation.kt (1)
30-37: Consistent navigation function with optional navigation options.The function provides a clean API for navigating to the write diary screen. It correctly creates the route with year, month, and day parameters.
app/src/main/java/com/sopt/clody/presentation/ui/main/ClodyNavHost.kt (4)
41-57: Well-documented central navigation component.The documentation clearly explains the purpose and responsibilities of
ClodyNavHostas the app's central navigation component. The comments provide good context about the component's role in connecting navigation graphs and handling external intents.
58-64: Clean setup of NavHost with appropriate parameters.The NavHost setup follows best practices by:
- Extracting the NavController from the provided appState
- Using Box with fillMaxSize to ensure the NavHost fills the available space
- Using the startDestination from appState for flexible routing
65-69: Good handling of external intent in splash screen.The splash screen navigation correctly passes the startIntent parameter, allowing proper handling of external navigation triggers like push notifications.
71-127: Comprehensive and consistent navigation setup.The navigation setup is well-structured with consistent patterns:
- Each screen registration follows the same pattern of passing appropriate navigation lambdas
- Back navigation is handled properly using
safePopBackStack- Navigation between screens is implemented using type-safe extension functions
This approach provides good separation of concerns and maintainability.
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/navigation/SignUpNavigation.kt (4)
12-22: Good separation of navigation concerns for Terms of Service screen.The
termsOfServiceScreenextension function properly encapsulates the navigation setup for the Terms of Service screen, accepting navigation lambdas to handle forward and back navigation.
24-34: Consistent navigation pattern for nickname screen.The
nicknameScreenfunction follows the same pattern as other navigation setup functions, providing a clean and predictable structure for the navigation graph.
36-40: Type-safe navigation to nickname screen.The
navigateToNicknameextension function on NavHostController provides a clean, type-safe way to navigate to the nickname screen with optional navigation options.
42-46: Type-safe navigation to terms of service screen.The
navigateToTermsOfServiceextension function on NavController provides a clean, type-safe way to navigate to the terms of service screen with optional navigation options.app/src/main/java/com/sopt/clody/presentation/ui/home/navigation/HomeNavigation.kt (2)
13-38: Well-structured home screen navigation with type-safe parameter extraction.The
homeScreenextension function follows the established pattern for navigation setup. It properly:
- Extracts route parameters using type-safe
backStackEntry.toRoute<Route.Home>()- Passes extracted parameters and navigation lambdas to the HomeRoute composable
This approach ensures type safety and provides a clean separation between navigation and UI logic.
40-47: Convenient home navigation with smart defaults.The
navigateToHomeextension function provides sensible defaults usingLocalDate.now()components, making it easy to navigate to the current date while still allowing specific date navigation when needed.app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingScreen.kt (6)
49-50: Improved type safety with enum imports.Replacing string literals with strongly-typed enums (
ReplyStatusandRoute.ReplyLoading.ReplyLoadingFrom) improves type safety and makes the code more maintainable.
56-65: Parameter refactoring for improved type safety and clarity.Good changes in the parameter list:
- Renaming
daytodatefor clarity- Using strongly typed enums instead of strings for
fromandreplyStatus- Replacing navigator dependency with explicit navigation lambdas
This approach decouples the screen from specific navigation implementations and improves type safety.
77-77: Updated parameter name in viewModel call.The viewModel method call has been updated to use
dateinstead ofday, matching the parameter rename.
83-89: Centralized back navigation logic.The new
handleBackNavigationlambda centralizes the back navigation logic, making it more maintainable and easier to understand. It properly handles navigation based on the origin screen using the type-safe enum.
91-98: Updated back handler with centralized navigation.The BackHandler now uses the centralized
handleBackNavigationlambda, ensuring consistent behavior when handling back navigation.
108-111: Updated navigation in success state.The navigation callbacks now use the explicit navigation lambdas passed to the composable, maintaining consistency with the new navigation architecture and improving code clarity.
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/navigation/ReplyLoadingNavigation.kt (3)
1-11: Well-structured imports with appropriate dependencies.The imports appropriately include the necessary navigation and route-related classes, aligning with the navigation refactoring objectives.
12-31: Clean extension function implementation for ReplyLoading navigation graph.The
replyLoadingScreenextension function effectively modularizes the navigation path for the reply loading feature, taking clearly-named function parameters for different navigation paths. This implementation follows the PR objective of "separation and extensibility of navigation by feature."I particularly like the type-safe parameter passing and clear lambda signatures with named parameters that make the navigation intent explicit.
33-45: Type-safe navigation function with sensible defaults.The
navigateToReplyLoadingextension function provides a clean API for navigation with:
- Properly typed parameters
- Sensible defaults for optional parameters
- Clean integration with the new sealed Route interface
This implementation successfully eliminates string literal-based navigation arguments in favor of type-safe enums, as outlined in the PR objectives.
app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryScreen.kt (7)
43-43: Good replacement of string-based parameters with typed enums.The import of
ReplyStatusenum aligns with the PR objective of replacing string literals with type-safe enums.
51-52: Improved function signature with type-safe parameter and explicit navigation lambda.The function now accepts:
- A typed
ReplyStatusenum instead of a string- An explicit navigation lambda instead of a navigator instance
This change supports the PR's goal of removing navigator classes and improving type safety.
68-68: Direct lambda invocation replaces navigator dependency.Successfully replaced navigator call with direct lambda invocation, supporting the architectural change to remove navigator classes.
82-83: Clean lambda passing in success state handler.The navigation lambda is properly passed down to the inner composable, maintaining the navigation flow.
102-103: Function parameter updates for type safety and lambda approach.Inner composable function updated to:
- Accept a simple lambda instead of navigator instance
- Use the typed ReplyStatus enum
These changes maintain consistency with the outer composable changes.
109-109: Type-safe enum comparison replaces string literal check.The change from string comparison to enum comparison (
replyStatus == ReplyStatus.READY_NOT_READ) makes the code more robust against typos and provides better IDE support.
127-127: Simple lambda invocation in event handler.IconButton click handler now directly uses the lambda, simplifying the code and removing navigator dependency.
app/src/main/java/com/sopt/clody/presentation/utils/navigation/Route.kt (5)
1-11: Well-designed sealed interface for type-safe routing.The
Routesealed interface with Kotlin serialization provides a solid foundation for type-safe navigation, addressing the PR objective of improving the route structure. The Kotlin documentation comments clearly explain the purpose of this interface.
13-30: Clean implementation of simple route objects.The simple screens without parameters are implemented as
data objecttypes, which is appropriate for these cases. Each is properly annotated with@Serializablefor consistent serialization support.
31-49: Well-structured parameterized routes for Home, DiaryList, and WriteDiary.These route implementations:
- Use proper data classes with clear parameter names
- Include default values where appropriate
- Are correctly annotated for serialization
The type-safe parameter naming and structure supports the PR goal of eliminating string literals.
51-64: Comprehensive implementation of ReplyLoading route with nested enum.The
ReplyLoadingroute demonstrates good design by:
- Including all necessary parameters with appropriate defaults
- Defining a nested enum
ReplyLoadingFromto represent the origin screen- Using the enum-based approach described in the PR objectives
This implementation effectively replaces string literals with typesafe enums for navigation arguments.
66-85: Complete coverage of all routes with consistent implementation patterns.The remaining routes (ReplyDiary, Setting, AccountManagement, NotificationSetting, WebView) follow the same pattern, ensuring consistency across the navigation system. The WebView route appropriately handles the URL as a parameter.
app/src/main/java/com/sopt/clody/presentation/ui/setting/navigation/SettingNavigation.kt (5)
1-13: Well-structured imports for Settings navigation.The imports are clean and include all necessary navigation-related classes and route composables.
14-28: Effective navigation graph setup for Settings screen.The
settingScreenextension function:
- Takes explicit navigation lambdas for all possible navigation paths
- Uses the type-safe Route.Setting object
- Properly passes the navigation lambdas to the route composable
This follows the pattern established in the PR for consistent navigation handling.
30-48: Consistent implementation of AccountManagement and NotificationSetting screen navigation.Both the
accountManagementScreenandnotificationSettingScreenextension functions follow the same pattern as thesettingScreenfunction, providing consistency across the navigation system.
50-61: WebView screen navigation with parameter extraction.The
webViewScreenfunction demonstrates how to:
- Extract a parameter (encodedUrl) from the route using
toRoute<Route.WebView>()- Pass that parameter to the route composable
This shows effective handling of parameterized routes.
63-86: Clean NavController extension functions for Settings navigation.The
navigateTo*extension functions:
- Provide a simple API for navigation to settings screens
- Accept optional navigation options
- Use the type-safe Route objects/classes
- Maintain consistency with the pattern established throughout the PR
These extension functions eliminate the need for navigator classes as specified in the PR objectives.
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (7)
34-37: Import changes align with the new navigation structureThe new imports for
RouteandReplyStatusare part of the type-safe navigation refactoring. These replace string-based navigation arguments with enum classes.
40-55: Function parameter changes follow the new navigation architectureThe
HomeRoutecomposable has been updated to:
- Accept a nullable
selectedDayparameter for more flexibility- Replace the Navigator dependency with strongly-typed navigation lambdas
- Use typed enums (
Route.ReplyLoading.ReplyLoadingFromandReplyStatus) instead of string parametersThis aligns well with the PR objective of creating a more type-safe and modular navigation structure.
58-58: Collection flow with proper lifecycle awarenessProperly using
collectAsStateWithLifecycle()for the replyStatus flow to prevent unnecessary recompositions.
71-82: Enhanced date handling logicThe LaunchedEffect now properly handles the nullable
selectedDayparameter, falling back to the current date when null. This provides more flexibility while maintaining the expected behavior.
96-111: Type-safe navigation implementationNavigation handlers now use lambda-based routing with proper enum-based arguments instead of string literals. The use of
Route.ReplyLoading.ReplyLoadingFrom.HOMEandReplyStatus.valueOf(replyStatus)provides type safety, making the code more robust.
124-129: Type-safe parameter definition for reply diary navigationThe
onClickReplyDiaryparameter now uses theRoute.ReplyLoading.ReplyLoadingFromenum type instead of a string, ensuring type safety and compiler-checked parameter passing.
247-247: Enum-based route source parameterUsing
Route.ReplyLoading.ReplyLoadingFrom.HOMEinstead of a string literal improves type safety and helps prevent errors from mistyped string values.
| setContent { | ||
| CLODYTheme { | ||
| val navController = rememberNavController() | ||
| val currentIntent by rememberUpdatedState(newValue = intent) | ||
| var processedIntent by remember { mutableStateOf<Intent?>(null) } | ||
|
|
||
| LaunchedEffect(key1 = currentIntent) { | ||
| if (processedIntent != currentIntent) { | ||
| handleIntent(currentIntent) | ||
| processedIntent = currentIntent | ||
| } | ||
| } | ||
|
|
||
| SideEffect { | ||
| if (processedIntent != intent) { | ||
| handleIntent(intent) | ||
| processedIntent = intent | ||
| } | ||
| } | ||
|
|
||
| LaunchedEffect(key1 = intent.getBooleanExtra("NAVIGATE_TO_LOGIN", false)) { | ||
| if (intent.getBooleanExtra("NAVIGATE_TO_LOGIN", false)) { | ||
| navController.navigate("register_graph") { | ||
| popUpTo(0) { inclusive = true } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| val authNavigator = remember(navController) { AuthNavigator(navController) } | ||
| val homeNavigator = remember(navController) { HomeNavigator(navController) } | ||
| val diaryListNavigator = remember(navController) { DiaryListNavigator(navController) } | ||
| val writeDiaryNavigator = remember(navController) { WriteDiaryNavigator(navController) } | ||
| val settingNavigator = remember(navController) { SettingNavigator(navController) } | ||
| val replyLoadingNavigator = remember(navController) { ReplyLoadingNavigator(navController) } | ||
| val replyDiaryNavigator = remember(navController) { ReplyDiaryNavigator(navController) } | ||
|
|
||
| MainNavHost( | ||
| navController = navController, | ||
| authNavigator = authNavigator, | ||
| homeNavigator = homeNavigator, | ||
| diaryListNavigator = diaryListNavigator, | ||
| writeDiaryNavigator = writeDiaryNavigator, | ||
| settingNavigator = settingNavigator, | ||
| replyLoadingNavigator = replyLoadingNavigator, | ||
| replyDiaryNavigator = replyDiaryNavigator, | ||
| ) | ||
| val appState = rememberClodyAppState() | ||
| ClodyApp(appState = appState, startIntent = intent) | ||
| } | ||
| } |
There was a problem hiding this comment.
Intent updates are no longer handled – deep links & push taps may break
startIntent is captured once in setContent.
If the activity is already running and a new intent arrives (onNewIntent), the composable tree will never receive it, so:
- deep-link navigation won’t trigger,
- push-notification analytics (
AmplitudeUtils.trackEvent(ALARM)) will be missed.
class MainActivity : ComponentActivity() {
...
+ override fun onNewIntent(intent: android.content.Intent?) {
+ super.onNewIntent(intent)
+ intent ?: return
+ setContent { // re-compose with the fresh intent
+ CLODYTheme {
+ val appState = rememberClodyAppState()
+ ClodyApp(appState = appState, startIntent = intent)
+ }
+ }
+ }Alternatively expose a MutableState<Intent> to the Compose layer and update it here to avoid a full recomposition.
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
앱 전체를 뜯어고치셨네요 .. 대단합니다 .. ㅜㅜ 전체적으로 들었던 궁금증 몇가지를 더 첨부하자면
- 현재 연(year), 월(month), 일(date), 요일(day) + dayOfMonth 로 사용되고 있는거 맞나요? 일자와 요일이 약간 헷갈려서 !
- 제가 학습한 바로는 NavGraphBuilder를 통해 NavGraph를 분리하는 이유가,
2-1. 각 모듈(기능)별로 NavGraph를 분리
2-2. 해당 NavGraph마다 명확한 진입점(startDestination)을 지정
2-3. 그곳을 통해서만 진입할 수 있도록 하여 의존도를 낮춤 (중첩 네비)
2-4. NavController도 분리를 할 수 있는 것으로? 알 수ㅇ있음 ..!
인 것으로 알고 있는데 지금은 거의 Screen 별로 다 따로 NavGraphBuilder를 사용하다보니 어떤 의미(효과)가 있는 것인지 궁금합니다!
There was a problem hiding this comment.
[3]
생명주기를 고려한 개발.. 제 목표인데 멋있네요
CREATED와 STARTED를 거쳐 사용자가 포그라운드에서 UI를 볼 때는 RESUMED 상태가 되기때문에
사용자가 포그라운드에서 앱을 사용 중인지, 화면의 구성이 끝났는지를 확인하는 작업을 추가한 것으로 보이는데 맞을까요?
예상 기대 효과 : 중복 처리 방지(연타할때)
There was a problem hiding this comment.
네네 맞습니다!
navigation 중간에 popBackStack 겹쳐서 호출되면 이상한 곳으로 갈 수 있죠.
There was a problem hiding this comment.
[3]
이 클래스가 필요한 이유가 궁금합니다!
MainActivity에서 선언된 뒤 App -> NavHost로 넘겨지는 이 ClodyAppState라는 객체가 정확히 어떤 상태를 관리하고 왜 필요한 것인가요?
There was a problem hiding this comment.
음
앱의 전역 상태를 하나로 묶어서 전달하기 위해 필요한 클래스 입니다.
NavController를 직접 넘기기보다는 상태 객체로 캡슐화해서 확장성과 구조 유지를 위한 기반으로 사용하는 거죠.
지금은 NavHostController만 포함하고 있긴 한데 추후 여러 상태를 관리하게 된다면 Context-free한 구조로 앱의 상태를 캡슐화 할 수 있답니다?
There was a problem hiding this comment.
[2]
Domain적인 요소라고 생각되는데 어떻게 생각하시나요? 네비게이션 뿐 아니라 ReplyStatus에 대한 클래스가 필요한 곳이 더러 있는데, 추후 Domain 레이어로 이전한 후 거기서 사용할 수도 있을까요?
아니면 @serializable이 붙어있는 별도의 네비게이션을 위한 클래스라고 생각하면 될까요?
There was a problem hiding this comment.
UI 전환을 위한 Parameter로 사용되고 있어서 @serializable을 붙여 navigation 목적에 맞춰 Presentation Layer에 뒀습니다.
지금 찾아보니 네비게이션 뿐만 아니라 비즈니스 로직에도 사용되는거 같은데.. 음
// Domain
enum class ReplyStatus { UNREADY, READY_READ, READY_NOT_READ }
// Navigation
@Serializable
enum class ReplyStatusArg { UNREADY, READY_READ, READY_NOT_READ }이렇게 ReplyStatus 자체는 Domain 레이어로 옮기고,
Navigation에 필요한 serialization용 enum으로 별도로 만들어 매핑하는 방식으로 처리하는 방법은 어떨까요?
There was a problem hiding this comment.
별도로 관리! 확인했습니다. 확실히 분리하는게 좋은 것 같다는 생각이 드네요!
There was a problem hiding this comment.
[3]
Login 이냐 SignIn이냐 그것이 문제로다 ..
Login/Join 또는 SignIn/SignUp으로 통일하는 방향은 어떠신가요?
There was a problem hiding this comment.
SignIn/SignUp 이게 너무 햇갈려서
Login / Signup으로 분리 하려고 했습니다.
여러 리포지토리를 찾아봤는데 이 Naming을 사용하고 있더라구요!
| fun NavHostController.navigateToNickname( | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.Nickname, navOptions) | ||
| } | ||
|
|
||
| fun NavController.navigateToTermsOfService( | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.TermsOfService, navOptions) | ||
| } |
There was a problem hiding this comment.
[3]
NavHostController와 NavController 따로 분리되어 있는 이유가 궁금합니다. 실제로 우리가 ClodyNavHost 에서 사용하고 있는 navController는 NavHostController 클래스의 인스턴스로 알고 있는데, 이렇게되면 navigateToTermsOfService 는 어떻게 되는것이죠? ㅜㅜ
There was a problem hiding this comment.
[3]
LoginRoute 쪽에서는 navigateToTerms로 네이밍 되어있는데 여기선 navigateToTermsOfService라 통일이 된다면 좋을 것 같아요
There was a problem hiding this comment.
[3] NavHostController와 NavController 따로 분리되어 있는 이유가 궁금합니다. 실제로 우리가 ClodyNavHost 에서 사용하고 있는 navController는 NavHostController 클래스의 인스턴스로 알고 있는데, 이렇게되면 navigateToTermsOfService 는 어떻게 되는것이죠? ㅜㅜ
그쵸 NavHostController는 NavController의 구체 구현체죠.
DIP 원칙을 따르자면 내부 구현체보단 상위 타입을 기준으로 확장하는게 좋습니다.
실제 사용하는 쪽은 NavHostController지만, 인터페이스로만 바라보면 충분하니깐요.
저 이동하는 함수 부분은 NavController기준으로 작성하는게 맞습니다. it's my mistake
| fun NavController.navigateToSetting( | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.Setting, navOptions) | ||
| } | ||
|
|
||
| fun NavController.navigateToAccountManagement( | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.AccountManagement, navOptions) | ||
| } | ||
|
|
||
| fun NavController.navigateToNotificationSetting( | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.NotificationSetting, navOptions) | ||
| } | ||
|
|
||
| fun NavController.navigateToWebView( | ||
| encodedUrl: String, | ||
| navOptions: NavOptionsBuilder.() -> Unit = {}, | ||
| ) { | ||
| navigate(Route.WebView(encodedUrl), navOptions) |
There was a problem hiding this comment.
[3]
이 부분도 NavHostController가 아닌 NavController의 확장함수네요. 어떤 차이가 있는지 궁금합니다.
| navigateToReplyDiary: (Int, Int, Int, ReplyStatus) -> Unit, | ||
| navigateToHome: (year: Int, month: Int, day: Int) -> Unit, | ||
| navigateToDiaryList: (Int, Int) -> Unit, |
There was a problem hiding this comment.
[3]
파라미터 람다식 매개변수의 이름이 명시가 된 부분이 있고 없는 부분이 있습니다. 이 파일 뿐만 아니라 곳곳에 혼재되어 있는데 통일하는게 좋을 것 같습니다.
명시를 하고 하지않고 어떤 것이 더 좋을까요? 명시를 한다면 코드의 가독성이 높아질테고, 생략한다면 간결해지는 그런 이점이 있을 것 같은데 어떤 것을 더 선호하시나요?
There was a problem hiding this comment.
명시하는게 좋다고 생각합니다.
NIA 나 Android Sample을 봐도 다 명시하는 형태로 되어있거든여.
명시가 안된 부분은 추가하도록 해보겠습니다잉
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt (1)
48-151: 💡 Verification agent🧩 Analysis chain
Verify migration completeness across the codebase.
The migration from string literals to
ReplyStatusenum appears comprehensive in this file. However, ensure all related files have been updated consistently to prevent runtime errors from mixed usage.Run the following script to verify complete migration:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining string literal usage for reply status # Expected: No hardcoded reply status strings should remain # Look for potential missed string literals related to reply status rg -i "(ready_not_read|unready|ready_read)" --type kt -A 1 -B 1 # Check for any String parameters in functions that might still expect reply status rg "replyStatus.*String" --type kt -A 2 -B 2 # Verify all ReplyStatus enum usages are consistent ast-grep --pattern 'ReplyStatus.$_'Length of output: 1676
Remaining string-based replyStatus conversion must be replaced
While enum comparisons are now used throughout most of the code, there is still a call to
ReplyStatus.valueOf(replyStatus)that relies on a raw String. This should be updated to pass the enum directly (e.g. change the navigation/view-model parameter fromStringtoReplyStatus) to avoid runtime failures if the String value changes.• app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt:109
// before ReplyStatus.valueOf(replyStatus) // after (for example) replyStatusArg: ReplyStatus = ReplyStatus.UNREADYPlease update the navigation argument and any call sites to use the enum instead of converting from String.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyDiaryResponseDto.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/MonthlyDiaryList.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/sopt/clody/presentation/ui/diarylist/navigation/DiaryListNavigation.kt
- app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/MonthlyDiaryList.kt (1)
14-14: LGTM: Improved type safety with ReplyStatus enum.The change from
StringtoReplyStatusenum in the callback signature enhances type safety and prevents invalid reply status values from being passed through the callback chain.Also applies to: 22-22
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/component/DailyDiaryCard.kt (3)
32-32: LGTM: Consistent type-safe callback signatures.The callback signature updates from
StringtoReplyStatusmaintain consistency with the overall refactor and improve type safety throughout the component hierarchy.Also applies to: 45-45, 124-124
48-49: LGTM: Eliminated magic strings with type-safe enum comparisons.Replacing string literal comparisons with
ReplyStatusenum constants improves type safety, provides better IDE support, and eliminates the risk of typos in string literals.
151-151: LGTM: Consistent enum usage in conditional rendering.The change from string literal to
ReplyStatus.READY_NOT_READenum comparison maintains consistency with the refactor and ensures type-safe conditional rendering logic.
| data class DailyDiary( | ||
| @SerialName("diaryCount") val diaryCount: Int, | ||
| @SerialName("replyStatus") val replyStatus: String, | ||
| @SerialName("replyStatus") val replyStatus: ReplyStatus, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify ReplyStatus enum values match server contract.
Changing from String to ReplyStatus enum improves type safety but could cause deserialization failures if the server sends values not defined in the enum.
Please run the following script to verify that all possible server replyStatus values are covered by the ReplyStatus enum:
🏁 Script executed:
#!/bin/bash
# Description: Check ReplyStatus enum constants and verify they match server usage
# Expected: Find enum constants and check against codebase usage
# Find the ReplyStatus enum definition
ast-grep --pattern 'enum class ReplyStatus {
$$$
}'
# Search for any hardcoded replyStatus strings that might indicate server values
rg -i "replyStatus.*=.*\"" -A 2 -B 2
# Look for ReplyStatus usage in tests that might show expected values
rg "ReplyStatus\." -A 2 -B 2 --type ktLength of output: 3172
🏁 Script executed:
#!/bin/bash
# Locate and inspect the ReplyStatus enum definition
echo "Searching for ReplyStatus enum file..."
enum_files=$(rg -l "enum class ReplyStatus" -n .)
if [ -z "$enum_files" ]; then
echo "⚠️ No ReplyStatus enum definition found."
exit 1
fi
echo "Found ReplyStatus enum in:"
echo "$enum_files"
echo
for file in $enum_files; do
echo "=== $file ==="
# Print the first 200 lines to capture the enum constants and annotations
sed -n '1,200p' "$file"
echo
echo "SerialName annotations in $file:"
rg "@SerialName" -n "$file" || echo " (none found)"
echo
echo "Checking for expected constants UNREADY and READY_NOT_READ:"
grep -E "UNREADY|READY_NOT_READ" "$file" || echo " (one or both constants missing)"
echo
doneLength of output: 1704
ReplyStatus enum covers current server values – update usages and add fallback
The ReplyStatus enum defines UNREADY, READY_READ, and READY_NOT_READ, which matches the JSON values the server currently returns (kotlinx.serialization will use the constant names by default since there are no @SerialName annotations).
However, to ensure robustness and consistency across the app:
- Guard against unknown values
• Consider adding@SerialNameon each enum constant for explicit mapping, or introduce a fallback (e.g., anUNKNOWNconstant with@SerialNameannotations) to avoid runtime failures if the server contract evolves. - Migrate remaining String-based comparisons to the enum
• app/src/main/java/com/sopt/clody/presentation/ui/type/DiaryCloverType.kt – replacediaryData.replyStatus == "…"withdiaryData.replyStatus == ReplyStatus.…
• app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt – change_replyStatus: MutableStateFlow<String>toMutableStateFlow<ReplyStatus>(and its publicStateFlow)
• app/src/main/java/com/sopt/clody/presentation/ui/home/calendar/component/DayItem.kt – update theif (diaryData.replyStatus == "…")checks to useReplyStatus
These changes will preserve type safety, prevent silent deserialization errors, and keep the UI logic in sync with the new enum-based model.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/dto/response/MonthlyDiaryResponseDto.kt
at line 15, the ReplyStatus enum is used for replyStatus but lacks @SerialName
annotations and a fallback for unknown values. Add @SerialName annotations to
each enum constant to explicitly map JSON values, and introduce an UNKNOWN enum
constant with a matching @SerialName to handle unexpected server values safely.
Then, update all code using replyStatus string comparisons in
DiaryCloverType.kt, HomeViewModel.kt, and DayItem.kt to use the ReplyStatus enum
instead, including changing MutableStateFlow<String> to
MutableStateFlow<ReplyStatus> where applicable, ensuring consistent type-safe
usage across the app.
2 participants
📌 ISSUE
closed #260
네비게이션 구조를 개선하고 통합 관리 체계를 확립했습니다.
📄 Work Description
1. 기능별 Navigation 분리 및 확장성 개선
2. 중앙 네비게이션 구조 정립
setContent { CLODYTheme { val appState = rememberClodyAppState() ClodyApp(appState = appState, startIntent = intent) } }3. Route 구조 개선
4. Enum 기반 인자 전달
5. 전역 구조를 요약하자면...?
ClodyAppClodyAppStateClodyNavHost✨ PR Point
간단하게 끝날 줄 알았는데 생각보다 할게 많았네요....
코드수가 1000줄이네...? 죄송합니다 ㅋㅋㅋㅋ....
파일 변경이 엄청 많은데 핵심만 먼저 보고싶다! 라고 한다면
ClodyApp, ClodyAppState, ClodyNavHost, MainActivity, 아무 네비게이션 패키지 하나 이렇게 보시면 될것 같습니다!
그러면 구조 이해하기에는 충분할 것 같아요.
아 그리고 지금은 그냥 Route단에서 바로 명시적으로 navigate함수를 가져다 쓰고 있는데
이후 MVI 구조로 마이그레이션을 하면 SideEffect로 빼는 방법으로 수정하는 방향으로 생각하고 있습니당
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores