๐ :: (#803) ์จ๋ณด๋ฉ ๊ธฐ๋ฅ ๊ตฌํ#806
Conversation
|
/gemini review |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new onboarding module with DataStore-backed persistence, wires onboarding completion into app startup navigation and ViewModel state, extends DI and build configs, expands the design system (colors, buttons, utilities), and introduces multiple feature screens, viewmodels, and navigator interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant DevApp as DmsApp (dev)
participant VM as MainActivityViewModel
participant DI as Hilt/Dagger
participant OnboardingDS as OnboardingDataStoreDataSource
participant Store as PreferencesDataStore
DevApp->>DI: request backStack & NavDisplay (on start)
DI->>VM: inject MainActivityViewModel(onboardingDataSource)
DevApp->>VM: LaunchedEffect requests initial screen decision
VM->>OnboardingDS: getOnboardingCompleted()
OnboardingDS->>Store: read "onboarding-completed" preference
Store-->>OnboardingDS: return Boolean
OnboardingDS-->>VM: returns completion flag
VM-->>DevApp: update isOnboardingCompleted StateFlow
DevApp->>DevApp: select initial route (Onboarding / SignIn / Main)
DevApp-->>NavDisplay: push selected route
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touchesโ Failed checks (1 warning)
โ Passed checks (2 passed)
โจ Finishing touches
๐งช Generate unit tests (beta)
๐ Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ๐ Files selected for processing (1)
๐ง Files skipped from review as they are similar to previous changes (1)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @uson1004, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! ์ด Pull Request๋ ์ฑ์ ์จ๋ณด๋ฉ ๊ธฐ๋ฅ์ ์๋กญ๊ฒ ๋์
ํ๊ณ , ๊ธฐ์กด MVI(Model-View-Intent) ํจํด์ ๊ฐ์ํ๋ Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with ๐ and ๐ on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR implements an onboarding feature for the DMS Android app, introducing a new architectural pattern (BaseStateViewModel) and persistent storage for onboarding completion state. The implementation includes animated UI components for the onboarding flow and integrates with the app's navigation structure.
Key Changes
- Introduced
BaseStateViewModel- a simplified ViewModel pattern without Intent processing for simpler use cases - Implemented
OnboardingDataStoremodule for persisting onboarding completion state using DataStore Preferences - Created onboarding UI screens with animated content transitions
Reviewed changes
Copilot reviewed 56 out of 71 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
core/ui/src/dev/java/.../BaseStateViewModel.kt |
New simplified ViewModel base class without Intent pattern |
core/onboarding/** |
New module for onboarding data persistence with DataStore |
feature/src/dev/.../onboarding/** |
Onboarding UI screens and navigation implementation |
app/src/dev/.../DmsApp.kt |
Updated app navigation to include onboarding flow |
core/design-system/** |
Design system updates for button components and color scheme |
feature/src/{dev,prod}/.../main/** |
Main feature screens refactored to use new patterns |
๐ก Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @Composable | ||
| internal fun OnboardingScreen( | ||
| navigateToSignIn: ()-> Unit, |
There was a problem hiding this comment.
Missing space after closing parenthesis. Should be navigateToSignIn: () -> Unit, for consistency with Kotlin formatting conventions.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| protected fun setState(newState: () -> S) { | ||
| _stateFlow.update { | ||
| newState() | ||
| } | ||
| } |
There was a problem hiding this comment.
The setState function accepts a lambda that returns the new state, but the implementation doesn't pass the current state to this lambda. Consider changing the signature to setState(transform: (S) -> S) to allow state transformations based on the current state, which is a common pattern and more flexible.
| LaunchedEffect(isOnboardingCompleted, isJwtAvailableState) { | ||
| val initialScreen = when { | ||
| !isOnboardingCompleted -> OnboardingScreenNav | ||
| isJwtAvailableState -> MainScreenNav | ||
| else -> SignInScreenNav | ||
| } | ||
| mainViewModel.consumeUpdateFailed() | ||
| } | ||
|
|
||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .systemBarsPadding(), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| NavDisplay( | ||
| backStack = backStack, | ||
| onBack = { backStack.removeLastOrNull() }, | ||
| entryProvider = entryProvider { | ||
| entry<ScreenA> { | ||
| Button(onClick = { backStack.add(ScreenB) }) { | ||
| Text("Go to Screen B") | ||
| } | ||
| } | ||
| entry<ScreenB> { | ||
| Text( | ||
| text = "Screen B", | ||
| color = DmsTheme.colorScheme.onSurface, | ||
| ) | ||
| } | ||
| }, | ||
| ) | ||
| if (backStack.lastOrNull() != initialScreen) { | ||
| backStack.clear() | ||
| backStack.add(initialScreen) | ||
| } |
There was a problem hiding this comment.
The navigation logic runs on every recomposition when isOnboardingCompleted or isJwtAvailableState changes, which could cause unnecessary navigation stack manipulations. Consider using a more stable approach or adding a flag to track if initial navigation has been performed.
|
|
||
| fun updateMeal() = | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| println("LOGLOG1") |
There was a problem hiding this comment.
Debug logging statement should be removed before merging to production. This println statement appears to be for debugging purposes.
| }.onFailure { | ||
| postSideEffect(HomeSideEffect.MealUpdateFailed) | ||
| }.also { | ||
| println("LOGLOG ${it.isSuccess}") |
There was a problem hiding this comment.
Debug logging statement should be removed before merging to production. This println statement appears to be for debugging purposes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
์ด PR์ ์จ๋ณด๋ฉ ๊ธฐ๋ฅ์ ๊ตฌํํ๋ฉฐ, core/onboarding ๋ชจ๋ ์ ์ค, DataStore๋ฅผ ์ด์ฉํ ์จ๋ณด๋ฉ ์ํ ๊ด๋ฆฌ, ์จ๋ณด๋ฉ UI ๊ตฌํ์ ํฌํจํฉ๋๋ค. ๋ํ ViewModel ๊ตฌํ์ ๊ฐ์ํํ๋ BaseStateViewModel์ด ๋์
๋์์ต๋๋ค. ์ถ๊ฐ์ ์ผ๋ก ๋์์ธ ์์คํ
์ ์์์ ๋ฆฌํฉํ ๋งํ๊ณ feature ๋ชจ๋์ dev์ prod ์์ค ์ธํธ๋ฅผ ๋ถ๋ฆฌํ๋ ๋ฑ ํฐ ๊ท๋ชจ์ ๋ฆฌํฉํ ๋ง๋ ํจ๊ป ์งํ๋์์ต๋๋ค.
์ ๋ฐ์ ์ธ ๊ตฌํ์ ๊ตฌ์กฐ๊ฐ ์ ์กํ์๊ณ , ์๋ก์ด ์จ๋ณด๋ฉ ๋ชจ๋๊ณผ BaseStateViewModel์ ์ข์ ์ถ๊ฐ ์ฌํญ์ผ๋ก ๋ณด์
๋๋ค. ์ฝ๋๊ฐ ๊น๋ํ๊ณ ์ข์ ๊ฐ๋ฐ ๊ดํ์ ๋ฐ๋ฅด๊ณ ์์ต๋๋ค.
๋ช ๊ฐ์ง ๊ฐ์ ์ ์ ์ฌํญ์ด ์์ผ๋ฉฐ, ์ฃผ๋ก ๋ด๋น๊ฒ์ด์ ๋ก์ง, ์๋ฌ ํธ๋ค๋ง, ๋๋ฒ๊ทธ ์ฝ๋ ์ ๊ฑฐ์ ๊ดํ ๊ฒ์ ๋๋ค. ์์ธํ ๋ด์ฉ์ ๊ฐ๋ณ ์ฝ๋ฉํธ๋ฅผ ํ์ธํด์ฃผ์ธ์. PR ์ค๋ช ์ ์จ๋ณด๋ฉ ๊ธฐ๋ฅ ์ธ์ ๋ฆฌํฉํ ๋ง ๋ด์ฉ์ ์ถ๊ฐํ๋ฉด ๋ค๋ฅธ ํ์๋ค์ด ๋ณ๊ฒฝ ์ฌํญ์ ์ดํดํ๋ ๋ฐ ๋์์ด ๋ ๊ฒ์ ๋๋ค.
| ResourcesCompat.getFont(this, fontId) | ||
| } | ||
| } catch (e: Exception) { | ||
| // Font preloading failed, but continue app initialization |
There was a problem hiding this comment.
preloadFonts ํจ์ ๋ด์ catch ๋ธ๋ก์ด ๋น์ด์์ต๋๋ค. ํฐํธ ํ๋ฆฌ๋ก๋ฉ์ ์คํจํ๋๋ผ๋ ์ฑ ์คํ์ ๊ณ์ํ๋ ๊ฒ์ ์ข์ ๊ฒฐ์ ์ด์ง๋ง, ๋๋ฒ๊น
์ ์ํด ์คํจ ์ ์์ธ๋ฅผ ๋ก๊น
ํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
// Font preloading failed, but continue app initialization
e.printStackTrace() // ๋๋ Logger๋ฅผ ์ฌ์ฉํด ๋ก๊ทธ๋ฅผ ๋จ๊ธฐ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.| else -> throw IllegalArgumentException("Unhandled ButtonColor: $this") | ||
| } |
There was a problem hiding this comment.
underlinecolorScheme ํจ์์ when ํํ์์์ ๋ชจ๋ ButtonColor enum ์ผ์ด์ค๊ฐ ์ด๋ฏธ ์ฒ๋ฆฌ๋๊ณ ์์ด else ๋ธ๋์น๋ ๋๋ฌํ ์ ์๋ ์ฝ๋์
๋๋ค. ์ด else ๋ธ๋์น๋ฅผ ์ ๊ฑฐํ๋ฉด ์๋ก์ด ButtonColor๊ฐ ์ถ๊ฐ๋์์ ๋ ์ปดํ์ผ ํ์์ ์๋ฌ๊ฐ ๋ฐ์ํ์ฌ ๋ชจ๋ ์ผ์ด์ค๋ฅผ ์ฒ๋ฆฌํ๋๋ก ๊ฐ์ ํ ์ ์์ด ์ฝ๋ ์์ ์ฑ์ ๋์ผ ์ ์์ต๋๋ค.
| stateFlow.value.copy(notices = notices) | ||
| } | ||
| }.onFailure { | ||
| it.printStackTrace() |
There was a problem hiding this comment.
it.printStackTrace()๋ ๋๋ฒ๊น
์๋ ์ ์ฉํ์ง๋ง, ํ๋ก๋์
์ฝ๋์์๋ ์ฌ์ฉ์ ์ง์ํด์ผ ํฉ๋๋ค. Timber์ ๊ฐ์ ์ ์ ๋ก๊น
๋ผ์ด๋ธ๋ฌ๋ฆฌ๋ฅผ ์ฌ์ฉํ์ฌ ์๋ฌ๋ฅผ ๊ธฐ๋กํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
| it.printStackTrace() | |
| it.printStackTrace() // TODO: ๋ก๊น ๋ผ์ด๋ธ๋ฌ๋ฆฌ๋ฅผ ์ฌ์ฉํด์ฃผ์ธ์. |
| viewModelScope.launch(Dispatchers.IO) { | ||
| println("LOGLOG1") | ||
| val capturedDate = stateFlow.value.selectedDate | ||
| runCatching { | ||
| mealRepository.updateMeal(capturedDate) | ||
| }.onSuccess { meal -> | ||
| setState { | ||
| stateFlow.value.copy( | ||
| selectedDate = capturedDate, | ||
| currentMeal = meal, | ||
| ) | ||
| } | ||
| postSideEffect(HomeSideEffect.MealUpdated) | ||
| }.onFailure { | ||
| postSideEffect(HomeSideEffect.MealUpdateFailed) | ||
| }.also { | ||
| println("LOGLOG ${it.isSuccess}") | ||
| } | ||
| } |
There was a problem hiding this comment.
updateMeal ํจ์ ๋ด์ ๋๋ฒ๊น
๋ชฉ์ ์ผ๋ก ์ฌ์ฉ๋ println ๋ฌธ๋ค์ด ๋จ์์์ต๋๋ค. ๋ฆด๋ฆฌ์ฆ ๋น๋์ ํฌํจ๋์ง ์๋๋ก ์ ๊ฑฐํด์ฃผ์ธ์.
viewModelScope.launch(Dispatchers.IO) {
val capturedDate = stateFlow.value.selectedDate
runCatching {
mealRepository.updateMeal(capturedDate)
}.onSuccess { meal ->
setState {
stateFlow.value.copy(
selectedDate = capturedDate,
currentMeal = meal,
)
}
postSideEffect(HomeSideEffect.MealUpdated)
}.onFailure {
postSideEffect(HomeSideEffect.MealUpdateFailed)
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt (1)
220-247: Avoid crashing UI whenSex.ALLis presentCurrent handling will throw if
Sex.ALLreaches the UI:
- Line 235:
Sex.ALL -> throw IllegalArgumentException()- Line 290:
Sex.ALL -> throw IllegalStateException()Even with the TODOs, this means a valid backend value (or future enum usage) can crash the screen during composition.
Consider treating
Sex.ALLsimilarly tonull(e.g., gray style and empty/โ๋ฏธ์ง์ โ label) instead of throwing, so the UI remains robust to unexpected values.Example adjustment:
- Sex.ALL -> throw IllegalArgumentException() - // TODO: fix - null -> ButtonDefaults.roundedGrayButtonColors() + Sex.ALL, // TODO: adjust style if needed + null -> ButtonDefaults.roundedGrayButtonColors() ... - Sex.ALL -> throw IllegalStateException() + Sex.ALL -> stringResource(R.string.sex_all) // or return empty string / reuse null pathAdjust the exact UX as desired, but avoid exceptions in UI code paths.
Also applies to: 285-292
๐ก Minor comments (8)
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt-109-117 (1)
109-117: Simplify redundant condition in color assignment.The background color ternary on line 114 checks
if (selectedTab == index)inside a block that already ensuresselectedTab == indexis true. The color will always beDmsTheme.colorScheme.onBackground.Apply this diff to simplify:
if (selectedTab == index) { Box( modifier = Modifier .fillMaxWidth() .height(2.dp) - .background(if (selectedTab == index) DmsTheme.colorScheme.onBackground else DmsTheme.colorScheme.line) + .background(DmsTheme.colorScheme.onBackground) .align(Alignment.BottomCenter), ) }feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt-210-210 (1)
210-210: Fix typo in function name.The function is named
voteTapContentbut should bevoteTabContentto match the naming convention ofapplicationTabContentand accurately reflect that it's tab content, not tap content.Apply this diff:
-fun LazyListScope.voteTapContent( +fun LazyListScope.voteTabContent(Don't forget to update the call site at line 158.
Committable suggestion skipped: line range outside the PR's diff.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt-97-111 (1)
97-111: Dialogs remain open after confirming sign-out or withdrawal.When the user confirms the action, the dialog should dismiss. Currently, it only dismisses when the user clicks cancel or taps outside.
confirmButton = { TextButton( - onClick = { viewModel.signOut() }, + onClick = { + onShouldShowSignOutDialogChange(false) + viewModel.signOut() + }, ) { Text(text = stringResource(id = R.string.accept)) } },Apply the same pattern to the withdrawal dialog.
Also applies to: 119-133
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt-285-292 (1)
285-292: Same issue:Sex.ALLthrowsIllegalStateException.Apply the same defensive handling here:
private val Sex.text: String @Composable inline get() = stringResource( id = when (this) { Sex.MALE -> R.string.sex_male Sex.FEMALE -> R.string.sex_female - Sex.ALL -> throw IllegalStateException() + Sex.ALL -> R.string.sex_all // or provide a fallback string resource }, )Committable suggestion skipped: line range outside the PR's diff.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt-230-247 (1)
230-247:Sex.ALLthrows an exception that could crash the app.If
Sex.ALLis ever passed (e.g., due to a backend change or data corruption), the app will crash. Consider handling it gracefully with a fallback:RoundedButton( onClick = {}, colors = when (sex) { Sex.MALE -> ButtonDefaults.roundedButtonColors() Sex.FEMALE -> ButtonDefaults.roundedRefuseButtonColors() - Sex.ALL -> throw IllegalArgumentException() - // TODO: fix - null -> ButtonDefaults.roundedGrayButtonColors() + Sex.ALL, null -> ButtonDefaults.roundedGrayButtonColors() },core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt-76-89 (1)
76-89: Gray button enabled/pressed states appear identical.For
ButtonColor.Gray, theenabledandpressedstates have identicaltextColorandbackgroundColorvalues (surface). This means there's no visual feedback when the button is pressed. Consider differentiating the pressed state.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt-195-195 (1)
195-195:borderColorparameter is computed but never applied.The
borderColoris animated and passed toBasicButton(Line 316), butBasicButtonnever uses it to draw a border. Either apply the border or remove the unused parameter.Also applies to: 283-291, 316-316
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt-27-41 (1)
27-41: Improve error handling beyond stack trace.The
onFailureblock only prints the stack trace (Line 38), leaving the UI with an empty list and no indication of failure. Consider updating the UI state to reflect the error so users can retry or see an error message.}.onFailure { it.printStackTrace() + // Consider: setState { copy(error = it) } or emit a side effect }
๐งน Nitpick comments (29)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt (1)
11-15: Consider usingderivedStateOffor more idiomatic Compose state derivation.While
rememberUpdatedStatewill work, it's typically used for capturing the latest value of lambda parameters. For derived state computations like this,derivedStateOfis the more idiomatic and efficient pattern in Compose.Apply this diff to use the recommended pattern:
@Composable fun keyboardAsState(): State<Boolean> { - val isImeVisible = WindowInsets.ime.getBottom(LocalDensity.current) > 0 - return rememberUpdatedState(newValue = isImeVisible) + val ime = WindowInsets.ime + val density = LocalDensity.current + return remember { + derivedStateOf { + ime.getBottom(density) > 0 + } + } }core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
37-38: Consider if returning Job is necessary.Most side effect APIs are fire-and-forget (return
Unit). ReturningJobsuggests callers might need to cancel or await it, but the intended usage pattern is unclear.If Job return is intentional for advanced use cases, consider adding KDoc to clarify. Otherwise, simplify to:
- protected fun postSideEffect(sideEffect: E): Job = - viewModelScope.launch { sideEffectChannel.send(sideEffect) } + protected fun postSideEffect(sideEffect: E) { + viewModelScope.launch { sideEffectChannel.send(sideEffect) } + }feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (4)
121-126: Replace hardcoded color with theme color.The hardcoded color
Color(0xFFE5E5E5)should use a theme color for consistency and maintainability. Consider usingDmsTheme.colorScheme.lineor adding a dedicated color to the theme if this shade is semantically distinct.
317-334: Remove redundant null check inside AnimatedVisibility.The
AnimatedVisibilityalready ensuresappliedTitle != nullat line 318, making the inner null check at line 322 redundant. The non-null assertion or smart cast can be used directly.Apply this diff:
AnimatedVisibility( visible = appliedTitle != null, enter = slideInVertically() + fadeIn(), exit = slideOutVertically() + fadeOut(), ) { - if (appliedTitle != null) { - RoundedButton( - onClick = { }, - fillMinSize = false, - contentPadding = PaddingValues( - horizontal = PaddingDefaults.Medium, - vertical = PaddingDefaults.Small, - ), - ) { - Text(text = appliedTitle) - } + RoundedButton( + onClick = { }, + fillMinSize = false, + contentPadding = PaddingValues( + horizontal = PaddingDefaults.Medium, + vertical = PaddingDefaults.Small, + ), + ) { + Text(text = appliedTitle!!) } }
366-366: Remove unused parameter.The
appliedTitleparameter is defined but never passed whenVoteCardis instantiated in thevoteTapContentfunction (lines 237-274). Consider removing it to simplify the function signature.Apply this diff:
private fun VoteCard( modifier: Modifier = Modifier, topStartTimeTitle: LocalDateTime, topEndTimeTitle: LocalDateTime, title: String, description: String, - appliedTitle: String? = null, isVoted: Boolean, onButtonClick: () -> Unit, ) {And remove the related AnimatedVisibility block (lines 410-427) that uses this parameter.
410-427: Remove dead code related to unused appliedTitle parameter.Since the
appliedTitleparameter is unused (see previous comment), this entireAnimatedVisibilityblock is dead code and should be removed.Apply this diff:
Text( modifier = Modifier.weight(1f), text = title, color = DmsTheme.colorScheme.onSurface, style = DmsTheme.typography.title2, ) - AnimatedVisibility( - visible = appliedTitle != null, - enter = slideInVertically() + fadeIn(), - exit = slideOutVertically() + fadeOut(), - ) { - if (appliedTitle != null) { - RoundedButton( - onClick = { }, - fillMinSize = false, - contentPadding = PaddingValues( - horizontal = PaddingDefaults.Medium, - vertical = PaddingDefaults.Small, - ), - ) { - Text(text = appliedTitle) - } - } - } }core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/composeExt.kt (1)
5-8: Consider marking asinlinefor better performance.Higher-order functions that take lambdas benefit from being marked
inlineto avoid lambda object allocation at call sites, especially for utility modifiers that may be called frequently.-fun Modifier.modifyIf( +inline fun Modifier.modifyIf( condition: Boolean, - modify: Modifier.() -> Modifier, + noinline modify: Modifier.() -> Modifier, ): Modifier = if (condition) modify() else thisNote: Use
noinlineif the lambda is stored or passed to non-inline functions, orcrossinlineif it's invoked in a different execution context. For direct invocation like this, plaininlinewithoutnoinlinewould work:-fun Modifier.modifyIf( +inline fun Modifier.modifyIf( condition: Boolean, modify: Modifier.() -> Modifier, ): Modifier = if (condition) modify() else thisfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt (1)
78-96: Consider adding loading state for better UX.The
ApplicationUiStatecurrently has no loading indicators, making it difficult for the UI to distinguish between:
- Initial state (data not yet loaded)
- Loading state (fetch in progress)
- Loaded state (data available)
- Error state (fetch failed)
Consider adding a loading flag:
internal data class ApplicationUiState( + val isLoading: Boolean, val appliedStudyRoom: AppliedStudyRoom?, val appliedRemainsOption: AppliedRemainsOption?, val modelStudentVoteList: List<AllVoteSearch>, val selectedVoteList: List<AllVoteSearch>, val studentVoteList: List<AllVoteSearch>, val approvalVoteList: List<AllVoteSearch>, ) : UiState() { companion object { fun initial() = ApplicationUiState( + isLoading = true, appliedStudyRoom = null,Or use a sealed class hierarchy for more robust state management (initial, loading, success, error).
app/src/prod/kotlin/team/aliens/dms/android/app/DmsApplication.kt (1)
26-43: Consider logging the exception for debugging.While swallowing the exception is appropriate here (to prevent app crashes during initialization), consider logging the error to help diagnose font loading issues during development.
Apply this diff to add logging:
} catch (e: Exception) { // Font preloading failed, but continue app initialization + android.util.Log.w("DmsApplication", "Failed to preload fonts", e) }core/onboarding/src/test/java/team/aliens/dms/android/onboarding/ExampleUnitTest.kt (1)
1-17: Replace placeholder test with actual onboarding tests.This is a template test file. Consider adding tests for the onboarding functionality, particularly for
OnboardingStore,OnboardingDataStoreDataSource, and related components introduced in this module.Would you like me to help generate unit tests for the onboarding data layer or open an issue to track this work?
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageNavigator.kt (1)
7-7: Remove or document the commented parameter.The commented parameter suggests incomplete or planned functionality. Either remove the comment if not needed, or add a TODO comment to track future implementation.
Apply one of these approaches:
Option 1: Remove the comment if not needed
- fun openEditProfileImage(/*selectImageType: SelectImageType = SelectImageType.NONE*/) + fun openEditProfileImage()Option 2: Document as planned feature
+ // TODO: Add selectImageType parameter when SelectImageType support is implemented fun openEditProfileImage(/*selectImageType: SelectImageType = SelectImageType.NONE*/)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/indecator/DmsDotsLoadingIndicator.kt (1)
27-67: Add basic argument validation for safer usageThe animation logic looks good. To make the API more robust against misuse (e.g., accidentally passing nonโpositive values), consider adding simple precondition checks:
@Composable fun DmsDotsLoadingIndicator( modifier: Modifier = Modifier, dotCount: Int = 3, dotSize: Dp = 6.dp, activeColor: Color = Color.White, baseColor: Color = activeColor.copy(alpha = 0.5f), durationMillis: Int = 1000, ) { + require(dotCount > 0) { "dotCount must be > 0" } + require(durationMillis > 0) { "durationMillis must be > 0" }This fails fast in dev builds and prevents surprising behavior if someone passes 0 or negative values later.
app/src/dev/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt (1)
24-31: Differentiate onboarding load failure from โnot completedโ
_isOnboardingCompleteddefaults tofalse, and any exception fromonboardingDataSource.getOnboardingCompleted()will currently crash the coroutine and leave the state atfalse. That conflates โuser has not completed onboardingโ with โwe failed to read the flagโ.Consider handling failures explicitly and (optionally) representing a third โunknown/loadingโ state:
init { viewModelScope.launch { - _isOnboardingCompleted.value = onboardingDataSource.getOnboardingCompleted() + runCatching { onboardingDataSource.getOnboardingCompleted() } + .onSuccess { completed -> + _isOnboardingCompleted.value = completed + } + .onFailure { + // TODO: log or send a side effect + // Decide whether to keep `false` as fallback or introduce a nullable/tri-state flag. + } } }This avoids silent crashes and makes the behavior around datastore failures more intentional.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (3)
43-59: Handle non-CannotFindMealExceptionfailures inupdateDateIn
updateDate, onlyCannotFindMealExceptionresults in a side effect; all other exceptions are silently ignored:}.onFailure { exception -> when (exception) { is CannotFindMealException -> postSideEffect(HomeSideEffect.CannotFindMeal) } }It would be better to at least log or emit a generic failure side effect for unexpected errors so the UI (and you) can react:
}.onFailure { exception -> - when (exception) { - is CannotFindMealException -> postSideEffect(HomeSideEffect.CannotFindMeal) - } + when (exception) { + is CannotFindMealException -> postSideEffect(HomeSideEffect.CannotFindMeal) + else -> postSideEffect(HomeSideEffect.MealUpdateFailed) // or introduce a dedicated error + } }This keeps behavior consistent with
updateMeal()and avoids silent failures.
62-80: Remove debugprintlnlogs fromupdateMeal
updateMealstill contains temporary debug logging:println("LOGLOG1") // ... }.also { println("LOGLOG ${it.isSuccess}") }These will spam logcat and donโt integrate with any logging framework.
Consider removing them or replacing with your structured logging mechanism:
- fun updateMeal() = - viewModelScope.launch(Dispatchers.IO) { - println("LOGLOG1") + fun updateMeal() = + viewModelScope.launch(Dispatchers.IO) { val capturedDate = stateFlow.value.selectedDate runCatching { mealRepository.updateMeal(capturedDate) }.onSuccess { meal -> // ... - }.onFailure { - postSideEffect(HomeSideEffect.MealUpdateFailed) - }.also { - println("LOGLOG ${it.isSuccess}") - } + }.onFailure { + postSideEffect(HomeSideEffect.MealUpdateFailed) + } }
118-250: Consider removing or extracting the large commented-out legacy blockThe legacy MVI implementation is kept as a big commented block at the bottom of the file. This can make the file noisy and harder to scan.
If you donโt plan to restore it, consider deleting it (history is in git), or moving it to a separate โ_legacyโ file to keep the new implementation focused.
app/build.gradle.kts (1)
105-110: Confirm feature module scope change across flavors
- Adding
implementation(project(ProjectPaths.Core.ONBOARDING))is consistent with the new core module.project(ProjectPaths.FEATURE)is now a plainimplementation, so the feature module will be available in bothdevandprodflavors (previously prod-only per AI summary).androidx.hilt.navigation.composeis now present for bothdevandprod, which aligns with Compose navigation usage.Please doubleโcheck that exposing the
featuremodule to thedevflavor is intentional and doesnโt pull in any prodโonly behavior into dev builds.Also applies to: 124-129
feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt (1)
12-24: Address the TODO: Implement the SignIn screen.This is a placeholder screen with a TODO comment. The onboarding flow navigates to this screen upon completion, but the actual sign-in functionality is not yet implemented.
Do you want me to generate a basic sign-in screen implementation or open a new issue to track this task?
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/IntroScreen.kt (1)
31-121: Intro animation flow looks good; consider externalizing stringsThe step-based animation and color transition logic are coherent and side-effect safe in
LaunchedEffect. To keep the onboarding copy maintainable and localizable, consider moving the hard-coded Korean strings into string resources (and usingstringResource) instead of inline literals.feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt (1)
12-36: OnboardingViewModel logic is sound; consider handling failures explicitlyInitialization and
completeOnboarding()correctly mirror the DataStore state intoOnboardingState. Right now exceptions fromgetOnboardingCompleted()are swallowed byrunCatchingwith noonFailurehandling; adding minimal logging or metrics inonFailurewould make unexpected DataStore issues easier to diagnose without changing behavior.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt (1)
10-10: Remove unused import.
BaseViewModelis imported but not used in this file.-import team.aliens.dms.android.core.ui.viewmodel.BaseViewModelfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListScreen.kt (1)
129-133: Hardcoded navigation bar height spacer.The TODO is noted. Consider using
WindowInsets.navigationBarsor passing the height as a parameter for more accurate spacing across different devices.feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/CompleteScreen.kt (2)
74-88: Reuse a singlerememberInfiniteTransitioninstance.Two separate
rememberInfiniteTransition()calls create redundant transition instances. Share one transition for both color animations:- val startColor by rememberInfiniteTransition().animateColor( + val infiniteTransition = rememberInfiniteTransition(label = "gradient") + val startColor by infiniteTransition.animateColor( initialValue = Color( 0xFFC176D0, ), targetValue = Color(0xFF62A4FF), animationSpec = infiniteRepeatable(animation = tween(durationMillis = 600)), + label = "startColor", ) - val endColor by rememberInfiniteTransition().animateColor( + val endColor by infiniteTransition.animateColor( initialValue = Color(0xFF62A4FF), targetValue = Color(0xFFC176D0), animationSpec = infiniteRepeatable(animation = tween(durationMillis = 600)), + label = "endColor", )
68-68: Extract hardcoded Korean strings to string resources.Hardcoded strings (
"3d ๋ก๊ณ ์ด๋ฏธ์ง","์ต๊ณ ์","๊ธฐ์์ฌ ๊ด๋ฆฌ ์์คํ .","์์ํ๊ธฐ") should be moved to string resources for proper localization support.Also applies to: 94-99, 113-113
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
36-52: Consider handling initial loading state to avoid UI flicker.The
backStackis initialized withOnboardingScreenNav(Line 39), but theLaunchedEffectmay immediately change it based onisOnboardingCompletedandisJwtAvailableState. This could cause a brief flash of the onboarding screen before redirecting.Consider initializing with a loading/splash state or checking if initial values are ready before rendering
NavDisplay.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt (1)
20-21: Variable names are misleading.
noticesAscByDateis populated fromOrder.NEWwhich typically means newest-first (descending by date), not ascending. The names suggest the opposite of what they contain. Consider renaming for clarity:- private var noticesAscByDate: List<Notice> = emptyList() - private var noticesDescByDate: List<Notice> = emptyList() + private var noticesByNew: List<Notice> = emptyList() + private var noticesByOld: List<Notice> = emptyList()Also applies to: 32-33
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (2)
145-146: Unreachableelsebranches can be removed.All
ButtonColorenum values (Primary,Gray,Error) are handled in thewhenexpressions. Theelsebranches throwingIllegalArgumentExceptionare unreachable and can be removed. Kotlin's exhaustivewhenon sealed types/enums makes them unnecessary.ButtonColor.Error -> ButtonState( // ... ) - - else -> throw IllegalArgumentException("Unhandled ButtonColor: $this") }Also applies to: 186-187
107-108: Inconsistent function naming convention.
containedColorScheme()uses proper camelCase, buttextcolorScheme()andunderlinecolorScheme()have lowercase 'c' in 'color'. Consider consistent naming:-private fun ButtonColor.textcolorScheme() = when (this) { +private fun ButtonColor.textColorScheme() = when (this) {-private fun ButtonColor.underlinecolorScheme() = when (this) { +private fun ButtonColor.underlineColorScheme() = when (this) {Also applies to: 148-149
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Colors.kt (1)
187-218:toString()omitsiconandlineproperties.The
toString()method includes all new tokens but doesn't outputiconorline, which could make debugging harder since these are public properties. Consider adding them for completeness."outlineVariant=$outlineVariant" + + "icon=$icon, " + + "line=$line" + ")"
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
โ Files ignored due to path filters (1)
core/design-system/src/dev/res/drawable/img_3d_logo.pngis excluded by!**/*.png
๐ Files selected for processing (57)
app/build.gradle.kts(2 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/DmsApplication.kt(2 hunks)build.gradle.kts(1 hunks)buildSrc/src/main/kotlin/ProjectPaths.kt(1 hunks)core/datastore/build.gradle.kts(1 hunks)core/datastore/src/main/java/team/aliens/dms/android/core/datastore/DataStore.kt(1 hunks)core/datastore/src/main/java/team/aliens/dms/android/core/datastore/Qualifier.kt(1 hunks)core/datastore/src/main/java/team/aliens/dms/android/core/datastore/di/DataStoreModule.kt(2 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Colors.kt(10 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Typography.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/indecator/DmsDotsLoadingIndicator.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/Clickable.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/composeExt.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt(1 hunks)core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Button.kt(1 hunks)core/onboarding/.gitignore(1 hunks)core/onboarding/build.gradle.kts(1 hunks)core/onboarding/proguard-rules.pro(1 hunks)core/onboarding/src/androidTest/java/team/aliens/dms/android/onboarding/ExampleInstrumentedTest.kt(1 hunks)core/onboarding/src/main/AndroidManifest.xml(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/OnboardingDataStoreDataSource.kt(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/OnboardingDataStoreDataSourceImpl.kt(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/store/OnboardingStore.kt(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/store/OnboardingStoreImpl.kt(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/di/DataSourceModule.kt(1 hunks)core/onboarding/src/main/java/team/aliens/dms/android/onboarding/di/StoreModule.kt(1 hunks)core/onboarding/src/test/java/team/aliens/dms/android/onboarding/ExampleUnitTest.kt(1 hunks)core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt(1 hunks)feature/build.gradle.kts(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/CompleteScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/IntroScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/navigation/OnboardingRoute.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/ui/OnboardingScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/Main.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/announcement/navigation/AnnouncementNavigator.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationNavigator.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/home/navigation/HomeNavigator.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageNavigator.kt(1 hunks)feature/src/prod/kotlin/team/aliens/dms/android/feature/main/navigation/MainNavigator.kt(1 hunks)settings.gradle.kts(1 hunks)
๐งฐ Additional context used
๐งฌ Code graph analysis (11)
feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt (1)
SignInScreen(12-24)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/ui/OnboardingScreen.kt (2)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/CompleteScreen.kt (1)
CompleteContent(37-121)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/IntroScreen.kt (1)
IntroContent(31-122)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(31-35)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(31-35)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt (4)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Dialog.kt (1)
AlertDialog(10-41)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (2)
TextButton(151-176)RoundedButton(178-203)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Scaffold.kt (1)
Scaffold(11-28)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/AppBars.kt (1)
DmsTopAppBar(13-37)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/component/CompleteScreen.kt (1)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (1)
DmsButton(253-350)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (2)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/navigation/OnboardingRoute.kt (1)
OnboardingRoute(6-13)feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt (1)
SignInRoute(6-9)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(31-35)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (2)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt (1)
keyboardAsState(11-15)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/indecator/DmsDotsLoadingIndicator.kt (1)
DmsDotsLoadingIndicator(27-68)
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (3)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Scaffold.kt (1)
Scaffold(11-28)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/AppBars.kt (1)
DmsTopAppBar(13-37)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (2)
RoundedButton(178-203)ContainedButton(94-119)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(31-35)
๐ช detekt (1.23.8)
app/src/prod/kotlin/team/aliens/dms/android/app/DmsApplication.kt
[warning] 40-40: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (39)
core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Button.kt (1)
1-2: Package declaration looks correctThe package name matches the directory structure and follows Kotlin conventions; no issues here.
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (1)
371-371: Clarify button text semantics.The button displays "ํฌํ ์ข ๋ฃ" (voting ended) when
isVotedis true. Verify that this correctly reflects the intended UX: doesisVotedmean the user has already voted, or that the voting period has ended? If it means the user has already voted, consider text like "ํฌํ ์๋ฃ" (voting completed) to better indicate user action rather than time-based status.build.gradle.kts (1)
24-24: LGTM! Compose compiler plugin added correctly.The Compose compiler plugin is properly configured at the root level with
apply false, enabling Compose compilation for modules that will apply it.core/onboarding/src/main/AndroidManifest.xml (1)
1-4: LGTM! Standard library module manifest.This is a minimal but valid AndroidManifest.xml for the onboarding library module.
core/onboarding/proguard-rules.pro (1)
1-21: LGTM! Standard ProGuard template.This is a standard ProGuard configuration template. No active rules are required at this stage.
app/src/prod/kotlin/team/aliens/dms/android/app/DmsApplication.kt (1)
21-24: LGTM! Font preloading improves startup performance.Preloading fonts during app initialization is a good practice to prevent text rendering delays.
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationNavigator.kt (1)
1-8: LGTM! Clean navigation interface.The interface is well-structured and follows the navigation pattern used across the application.
core/datastore/build.gradle.kts (1)
48-48: Theapi()exposure is justified becausePreferencesDataStoreis returned by public Hilt provider functions.The
core:datastoremodule exportsPreferencesDataStorethrough Hilt@Providesfunctions (providePreferenceDataStore,provideFeaturesDataStore,provideDeviceDataStore,provideOnboardingDataStore) that are part of the module's public API. SincePreferencesDataStoreis a typealias forDataStore<Preferences>, consumers that depend oncore:datastoreneed access to theandroidx.datastore.preferencestypes to work with these injected dependencies. Theapi()configuration is correct.core/onboarding/src/androidTest/java/team/aliens/dms/android/onboarding/ExampleInstrumentedTest.kt (1)
1-24: Template instrumentation test looks fineThe test correctly asserts the module test package and follows the standard AndroidJUnit4 pattern.
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/Main.kt (1)
55-241: Navigation setup and bottom bar wiring look solidNice use of a dedicated
navigateToextension withpopUpTo(findStartDestination()),launchSingleTop, andrestoreStateโthis should give predictable bottom-nav behavior without back stack bloat. TheMainSectionsenum keeps routes/icons/labels centralized and readable.No functional issues spotted here.
core/onboarding/.gitignore (1)
1-1: Ignoring module build directory is appropriate
/buildis the right thing to ignore for this new module; keeps build artifacts out of version control.feature/src/prod/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt (1)
245-256: No behavioral change in this hunkThe added blank line between
DefaultHomeScreenVerticalSpaceandHomeBackgroundBrushdoes not affect behavior; nothing else to adjust in this segment.settings.gradle.kts (1)
40-40: Onboarding module correctly wired into settingsIncluding
":core:onboarding"here lines up withProjectPaths.Core.ONBOARDINGand the new module dependencies; no issues seen.feature/src/prod/kotlin/team/aliens/dms/android/feature/main/announcement/navigation/AnnouncementNavigator.kt (1)
1-8: AnnouncementNavigator interface looks goodClear, single-responsibility API for opening notice details with a type-safe
UUID.buildSrc/src/main/kotlin/ProjectPaths.kt (1)
8-22: ONBOARDING path constant is consistent
ProjectPaths.Core.ONBOARDINGis correctly added and matches the module name used in settings and Gradle scripts.feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt (1)
1-9: SignInRoute composable is clean and consistentThin wrapper around
SignInScreen()keeps navigation entry separate from UI implementation; aligns with typical route patterns.feature/build.gradle.kts (1)
67-76: Feature module correctly depends on core onboardingThe new
implementation(project(ProjectPaths.Core.ONBOARDING))ties the feature layer to the onboarding core module as expected; no issues spotted.feature/src/prod/kotlin/team/aliens/dms/android/feature/main/home/navigation/HomeNavigator.kt (1)
1-8: HomeNavigator interface looks appropriateAPI is minimal and focused on home-screen navigation responsibilities; good abstraction for higher-level navigator implementations.
core/datastore/src/main/java/team/aliens/dms/android/core/datastore/Qualifier.kt (1)
17-19: LGTM!The new
OnboardingDataStorequalifier follows the exact same pattern as existing data store qualifiers in this file, maintaining consistency with the established DI design.core/datastore/src/main/java/team/aliens/dms/android/core/datastore/DataStore.kt (1)
12-12: LGTM!The new
onboardingStorefollows the same pattern as existing data store bindings in this file, maintaining consistency.core/onboarding/build.gradle.kts (1)
1-54: LGTM!The Gradle configuration for the new onboarding module follows standard Android library patterns with appropriate dependencies for DataStore-backed onboarding state management.
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/navigation/OnboardingRoute.kt (1)
6-13: LGTM!The navigation route properly delegates to
OnboardingScreen, following the common pattern of separating route definitions from screen implementations in Compose.feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/ui/OnboardingScreen.kt (1)
23-56: LGTM!The onboarding screen correctly implements a two-step flow:
- Shows
IntroContentinitially- Transitions to
CompleteContentafter the intro animation ends- Calls
completeOnboarding()and navigates to sign-in when the user clicks the complete buttonThe use of
AnimatedContentfor smooth transitions and proper system bar padding are good implementation choices.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Typography.kt (1)
194-194: Verify the necessity of makingnotoSansFontFamilypublic.The visibility of
notoSansFontFamilyhas been changed frominternalto public, expanding the design system's public API surface. This allows external modules to directly access the font family.Is this change intentional? If the font family only needs to be accessed within the design-system module and through the
Typographyclass, consider keeping itinternalto maintain encapsulation.core/onboarding/src/main/java/team/aliens/dms/android/onboarding/di/DataSourceModule.kt (1)
11-18: LGTM!The DI module correctly binds
OnboardingDataStoreDataSourceImpltoOnboardingDataStoreDataSourceusing the standard Dagger/Hilt pattern with appropriate singleton scoping.core/onboarding/src/main/java/team/aliens/dms/android/onboarding/di/StoreModule.kt (1)
11-18: Store binding is correctly scoped and wired
OnboardingStoreImplis bound toOnboardingStoreinSingletonComponent, which is appropriate for a DataStore-backed onboarding store. No issues from a DI/scoping perspective.core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/OnboardingDataStoreDataSource.kt (1)
3-8: OnboardingDataStoreDataSource contract is clear and minimalThe two suspend functions provide a clean, focused abstraction for onboarding completion state; this should keep higher layers decoupled from underlying storage details.
core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/store/OnboardingStore.kt (1)
3-7: Internal OnboardingStore abstraction is reasonableInternalizing the storage contract behind
OnboardingDataStoreDataSourcekeeps the layering clear and allows implementation changes without affecting feature modules.core/datastore/src/main/java/team/aliens/dms/android/core/datastore/di/DataStoreModule.kt (1)
12-18: Onboarding DataStore provider is consistent with existing patternThe
@OnboardingDataStoreprovider mirrors the existing JWT/features/device providers and correctly returnscontext.onboardingStoreas a singleton. Wiring looks good assuming the qualifier and extension are defined as in related files.Also applies to: 45-50
core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/OnboardingDataStoreDataSourceImpl.kt (1)
6-15: Delegation to OnboardingStore is straightforward and correct
OnboardingDataStoreDataSourceImplcleanly delegates toOnboardingStorefor both read and write operations; no additional concerns here.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.kt (1)
14-43: Color scheme remapping is internally consistentThe updated light/dark
ColorSchememappings use the DmsColor token sets consistently and keep the public theming API unchanged. Assuming the underlying token values follow your design spec and contrast requirements, this looks fine.Also applies to: 45-74
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt (1)
39-46: OnboardingState shape and initialState are appropriateA single
isOnboardingCompletedflag with a clearinitialState()factory aligns well with the current requirements and integrates cleanly withBaseStateViewModel.core/onboarding/src/main/java/team/aliens/dms/android/onboarding/datastore/store/OnboardingStoreImpl.kt (2)
24-27: Implementation looks correct.Using
.first()for a one-shot read from the DataStore flow is the appropriate pattern. The default value offalseensures a safe fallback when no value is stored.
16-22: Thetransformwrapper provides error handling and is not redundant.The
transformutility (fromteam.aliens.dms.android.core.datastore.util.TransformHandler) wraps theeditcall withrunCatching, automatically handling failures by throwing aTransformFailureExceptionif the operation fails. However, the current usage relies only on default error handling without leveraging theonSuccessandonFailurecallback parameters. If explicit error handling or logging is needed, add custom callbacks; otherwise, consider whether the wrapper adds sufficient value for this simple operation.app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (2)
21-28: LGTM on navigation key declarations.The navigation keys are properly defined as serializable data objects implementing
NavKey, following the navigation3 pattern.
67-72: Placeholder main screen noted.The
MainScreenNaventry is a placeholder. Ensure this is addressed before release.feature/src/prod/kotlin/team/aliens/dms/android/feature/main/navigation/MainNavigator.kt (1)
6-37: LGTM on navigation interface design.The
MainNavigatorinterface provides a clean abstraction for navigation operations with well-named methods. The use ofUUIDfor entity IDs and appropriate parameter types is correct.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Colors.kt (2)
114-117: Clarify theicon/linevstint/dividernaming discrepancy.The constructor takes
tintanddividerparameters (Lines 43-44), but the class exposesiconandlineproperties (Lines 114-117). Thecopy()function usesicon/lineas parameter names but passes them to constructor astint/divider(Lines 183-184).If this is for backward compatibility, consider adding a comment to clarify. Otherwise, align the naming throughout.
Also applies to: 150-151, 183-184
314-380: LGTM on expanded color token definitions.The
lightColors()anddarkColors()functions are properly updated to include all new color tokens with appropriate default values from theDmsColorpalette.Also applies to: 382-448
| val scale by animateFloatAsState( | ||
| targetValue = if (isPressed) { | ||
| pressDepth | ||
| } else { | ||
| 1f | ||
| }, | ||
| animationSpec = tween(delayMillis = DURATION_MILLIS), | ||
| ) |
There was a problem hiding this comment.
Bug: Use durationMillis instead of delayMillis in tween().
The tween() function is using delayMillis which delays the start of the animation by 100ms, then animates instantly. Based on the constant name DURATION_MILLIS, the intent appears to be setting the animation duration to 100ms.
val scale by animateFloatAsState(
targetValue = if (isPressed) {
pressDepth
} else {
1f
},
- animationSpec = tween(delayMillis = DURATION_MILLIS),
+ animationSpec = tween(durationMillis = DURATION_MILLIS),
)๐ 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.
| val scale by animateFloatAsState( | |
| targetValue = if (isPressed) { | |
| pressDepth | |
| } else { | |
| 1f | |
| }, | |
| animationSpec = tween(delayMillis = DURATION_MILLIS), | |
| ) | |
| val scale by animateFloatAsState( | |
| targetValue = if (isPressed) { | |
| pressDepth | |
| } else { | |
| 1f | |
| }, | |
| animationSpec = tween(durationMillis = DURATION_MILLIS), | |
| ) |
๐ค Prompt for AI Agents
In
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/Clickable.kt
around lines 40 to 47, the tween() call incorrectly uses delayMillis =
DURATION_MILLIS which delays the animation start instead of setting its length;
change the parameter to durationMillis = DURATION_MILLIS so the animation runs
over 100ms as intended (no delay), keeping the rest of the animateFloatAsState
call unchanged.
| private val sideEffectChannel: Channel<E> = Channel() | ||
| val sideEffectFlow: Flow<E> = sideEffectChannel.receiveAsFlow() |
There was a problem hiding this comment.
Specify channel capacity to prevent suspended coroutines.
The default Channel() constructor creates a rendezvous channel (capacity 0), which suspends send() until a receiver is ready. For side effects, this can cause:
- Suspended coroutines if no collector is attached
- Memory pressure from multiple suspended
postSideEffectcalls - Potential side effect loss if the ViewModel is cleared while sends are suspended
Apply this diff to use a buffered channel:
- private val sideEffectChannel: Channel<E> = Channel()
+ private val sideEffectChannel: Channel<E> = Channel(Channel.BUFFERED)Alternatively, use Channel.UNLIMITED if you need to guarantee delivery of all side effects, though this may consume more memory.
๐ 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 val sideEffectChannel: Channel<E> = Channel() | |
| val sideEffectFlow: Flow<E> = sideEffectChannel.receiveAsFlow() | |
| private val sideEffectChannel: Channel<E> = Channel(Channel.BUFFERED) | |
| val sideEffectFlow: Flow<E> = sideEffectChannel.receiveAsFlow() |
๐ค Prompt for AI Agents
In
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt
around lines 28-29 the sideEffect Channel is created with the default rendezvous
capacity (Channel()), which causes send() to suspend when no collector is
attached; change the declaration to use a buffered capacity (e.g.,
Channel(Channel.BUFFERED)) to avoid suspended coroutines and lost side-effects,
or use Channel(Channel.UNLIMITED) if you must guarantee delivery of all side
effects; update the sideEffectChannel instantiation accordingly and leave
sideEffectFlow as receiveAsFlow().
| private fun fetchAppliedStudyRoom() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| studyRoomRepository.fetchAppliedStudyRoom() | ||
| }.onSuccess { appliedStudyRoom -> | ||
| setState { stateFlow.value.copy(appliedStudyRoom = appliedStudyRoom) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun fetchAppliedRemainsOption() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| remainsRepository.fetchAppliedRemainsOption() | ||
| }.onSuccess { appliedRemainsOption -> | ||
| setState { stateFlow.value.copy(appliedRemainsOption = appliedRemainsOption) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun fetchAllVoteSearch() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| votingRepository.fetchAllVoteSearch() | ||
| }.onSuccess { fetchedVoteSearch -> | ||
| setState { | ||
| stateFlow.value.copy( | ||
| modelStudentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.MODEL_STUDENT_VOTE }, | ||
| selectedVoteList = fetchedVoteSearch.filter { it.voteType == Vote.OPTION_VOTE }, | ||
| studentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.STUDENT_VOTE }, | ||
| approvalVoteList = fetchedVoteSearch.filter { it.voteType == Vote.APPROVAL_VOTE }, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for failed data fetches.
All three fetch methods use runCatching with only onSuccess handlers, silently ignoring failures. This leads to:
- No user feedback when data fetches fail
- Difficult debugging in production
- Confusing UI states (empty data vs. loading vs. error)
Consider adding error handling:
runCatching {
studyRoomRepository.fetchAppliedStudyRoom()
}.onSuccess { appliedStudyRoom ->
setState { stateFlow.value.copy(appliedStudyRoom = appliedStudyRoom) }
+ }.onFailure { exception ->
+ // Log error or update UI state
+ Log.e("ApplicationViewModel", "Failed to fetch applied study room", exception)
}Alternatively, add error state to ApplicationUiState to display error messages to users.
๐ 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 fetchAppliedStudyRoom() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| studyRoomRepository.fetchAppliedStudyRoom() | |
| }.onSuccess { appliedStudyRoom -> | |
| setState { stateFlow.value.copy(appliedStudyRoom = appliedStudyRoom) } | |
| } | |
| } | |
| } | |
| private fun fetchAppliedRemainsOption() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| remainsRepository.fetchAppliedRemainsOption() | |
| }.onSuccess { appliedRemainsOption -> | |
| setState { stateFlow.value.copy(appliedRemainsOption = appliedRemainsOption) } | |
| } | |
| } | |
| } | |
| private fun fetchAllVoteSearch() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| votingRepository.fetchAllVoteSearch() | |
| }.onSuccess { fetchedVoteSearch -> | |
| setState { | |
| stateFlow.value.copy( | |
| modelStudentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.MODEL_STUDENT_VOTE }, | |
| selectedVoteList = fetchedVoteSearch.filter { it.voteType == Vote.OPTION_VOTE }, | |
| studentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.STUDENT_VOTE }, | |
| approvalVoteList = fetchedVoteSearch.filter { it.voteType == Vote.APPROVAL_VOTE }, | |
| ) | |
| } | |
| } | |
| } | |
| } | |
| private fun fetchAppliedStudyRoom() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| studyRoomRepository.fetchAppliedStudyRoom() | |
| }.onSuccess { appliedStudyRoom -> | |
| setState { stateFlow.value.copy(appliedStudyRoom = appliedStudyRoom) } | |
| }.onFailure { exception -> | |
| // Log error or update UI state | |
| Log.e("ApplicationViewModel", "Failed to fetch applied study room", exception) | |
| } | |
| } | |
| } | |
| private fun fetchAppliedRemainsOption() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| remainsRepository.fetchAppliedRemainsOption() | |
| }.onSuccess { appliedRemainsOption -> | |
| setState { stateFlow.value.copy(appliedRemainsOption = appliedRemainsOption) } | |
| } | |
| } | |
| } | |
| private fun fetchAllVoteSearch() { | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| votingRepository.fetchAllVoteSearch() | |
| }.onSuccess { fetchedVoteSearch -> | |
| setState { | |
| stateFlow.value.copy( | |
| modelStudentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.MODEL_STUDENT_VOTE }, | |
| selectedVoteList = fetchedVoteSearch.filter { it.voteType == Vote.OPTION_VOTE }, | |
| studentVoteList = fetchedVoteSearch.filter { it.voteType == Vote.STUDENT_VOTE }, | |
| approvalVoteList = fetchedVoteSearch.filter { it.voteType == Vote.APPROVAL_VOTE }, | |
| ) | |
| } | |
| } | |
| } | |
| } |
๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt
around lines 40 to 75, the three fetch methods call runCatching but only handle
onSuccess so failures are swallowed; add onFailure handlers that log the
exception (use your logger) and update the UI state with an error field (or an
isError/message property in ApplicationUiState) so the UI can show an error and
recover; ensure state updates happen on the main thread (switch to
Dispatchers.Main or use withContext) when calling setState, and consider
resetting/loading flags appropriately on success/failure.
| OutlinedCard( | ||
| modifier = modifier | ||
| .padding(PaddingDefaults.Small) | ||
| .pointerInput(currentCardType) { | ||
| detectHorizontalDragGestures( | ||
| onDragEnd = { | ||
| when (dragDirection) { | ||
| DragDirection.LEFT -> onSwipeToLeft() | ||
| DragDirection.RIGHT -> onSwipeToRight() | ||
| null -> {} | ||
| } | ||
| }, | ||
| ) { _, dragAmount -> | ||
| if (currentCardType == LUNCH) { | ||
| when { | ||
| dragAmount > 0 -> onSwipeToLeft() | ||
| dragAmount < 0 -> onSwipeToRight() | ||
| } | ||
| } else { | ||
| when { | ||
| dragAmount > 0 -> dragDirection = DragDirection.LEFT | ||
| dragAmount < 0 -> dragDirection = DragDirection.RIGHT | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix horizontal drag handling to avoid multiple navigations per gesture
In MealCard, drag handling is inconsistent for the LUNCH card:
detectHorizontalDragGestures(
onDragEnd = {
when (dragDirection) {
DragDirection.LEFT -> onSwipeToLeft()
DragDirection.RIGHT -> onSwipeToRight()
null -> {}
}
},
) { _, dragAmount ->
if (currentCardType == LUNCH) {
when {
dragAmount > 0 -> onSwipeToLeft()
dragAmount < 0 -> onSwipeToRight()
}
} else {
when {
dragAmount > 0 -> dragDirection = DragDirection.LEFT
dragAmount < 0 -> dragDirection = DragDirection.RIGHT
}
}
}For LUNCH, onSwipeToLeft/Right is called on every drag event, which can cause multiple page transitions for a single gesture and differs from BREAKFAST/DINNER behavior (which trigger only on drag end).
A more consistent approach is to always record direction during drag and act only on onDragEnd:
.pointerInput(currentCardType) {
- detectHorizontalDragGestures(
- onDragEnd = {
- when (dragDirection) {
- DragDirection.LEFT -> onSwipeToLeft()
- DragDirection.RIGHT -> onSwipeToRight()
- null -> {}
- }
- },
- ) { _, dragAmount ->
- if (currentCardType == LUNCH) {
- when {
- dragAmount > 0 -> onSwipeToLeft()
- dragAmount < 0 -> onSwipeToRight()
- }
- } else {
- when {
- dragAmount > 0 -> dragDirection = DragDirection.LEFT
- dragAmount < 0 -> dragDirection = DragDirection.RIGHT
- }
- }
- }
+ detectHorizontalDragGestures(
+ onDragEnd = {
+ when (dragDirection) {
+ DragDirection.LEFT -> onSwipeToLeft()
+ DragDirection.RIGHT -> onSwipeToRight()
+ null -> Unit
+ }
+ dragDirection = null
+ },
+ ) { _, dragAmount ->
+ dragDirection = when {
+ dragAmount > 0f -> DragDirection.LEFT
+ dragAmount < 0f -> DragDirection.RIGHT
+ else -> dragDirection
+ }
+ }
}Also apply the same change to the prod variant (feature/src/prod/kotlin/.../HomeScreen.kt) to keep behavior in sync.
๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt
around lines 531-555, the LUNCH branch calls onSwipeToLeft/onSwipeToRight on
every drag event causing multiple navigations per gesture; change the drag
handler so it only records dragDirection during drag (set dragDirection = LEFT
when dragAmount > 0, RIGHT when < 0, otherwise leave null) and remove direct
calls to onSwipe* from the drag lambda, then perform the actual navigation only
in onDragEnd by switching on dragDirection and invoking
onSwipeToLeft/onSwipeToRight and finally reset dragDirection to null; apply the
identical change to the prod variant file at
feature/src/prod/kotlin/.../HomeScreen.kt to keep behavior consistent.
| // TODO | ||
| @Suppress("DEPRECATION") | ||
| private fun vibrateOnMealCardPaging( | ||
| context: Context, | ||
| ) { | ||
| val vibrator = context.getSystemService(Context.VIBRATOR_SERVICE) as Vibrator | ||
| vibrator.vibrate(3L) |
There was a problem hiding this comment.
Guard against missing vibrator service to avoid potential NPE
vibrateOnMealCardPaging assumes the vibrator service is always present:
val vibrator = context.getSystemService(Context.VIBRATOR_SERVICE) as Vibrator
vibrator.vibrate(3L)On devices without a vibrator, getSystemService can return null, and the unsafe cast will crash.
Safer version:
@Suppress("DEPRECATION")
private fun vibrateOnMealCardPaging(
context: Context,
) {
- val vibrator = context.getSystemService(Context.VIBRATOR_SERVICE) as Vibrator
- vibrator.vibrate(3L)
+ val vibrator = context.getSystemService(Context.VIBRATOR_SERVICE) as? Vibrator
+ vibrator?.vibrate(3L)
}You can later also migrate to VibrationEffect/Vibrator.vibrate(VibrationEffect) when you tackle the TODO.
๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt
around lines 647 to 653, the function vibrateOnMealCardPaging unsafely casts the
vibrator service and will NPE if the service is missing; change the code to use
a safe cast or null check when obtaining the Vibrator (e.g.,
context.getSystemService(Context.VIBRATOR_SERVICE) as? Vibrator), then only call
vibrate(...) if the vibrator is non-null (no-op otherwise), and keep the
DEPRECATION suppression and TODO about migrating to VibrationEffect for a later
change.
| private fun fetchMyPage() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| studentRepository.fetchMyPage() | ||
| }.onSuccess { myPage -> | ||
| setState { | ||
| stateFlow.value.copy(myPage = myPage) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun signOut() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| authRepository.signOut() | ||
| }.onSuccess { | ||
| postSideEffect(MyPageSideEffect.SignOutSuccess) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun withdraw() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| studentRepository.withdraw() | ||
| }.onSuccess { | ||
| postSideEffect(MyPageSideEffect.WithdrawalSuccess) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failure: errors are swallowed without user feedback.
All three methods (fetchMyPage, signOut, withdraw) use runCatching but only handle onSuccess. Failures are silently ignored, leaving users with no indication that an operation failed.
Consider adding error handling via onFailure to emit an error side effect or update UI state:
private fun fetchMyPage() {
viewModelScope.launch(Dispatchers.IO) {
runCatching {
studentRepository.fetchMyPage()
}.onSuccess { myPage ->
setState {
stateFlow.value.copy(myPage = myPage)
}
+ }.onFailure {
+ // Handle error: emit side effect or update state
}
}
}๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt
around lines 27 to 57, the three suspend operations use runCatching but only
handle onSuccess so exceptions are swallowed and users receive no feedback; add
onFailure handlers for fetchMyPage, signOut, and withdraw to handle errors
(e.g., post a MyPageSideEffect.Error or specific failure side effects, update
state with an error flag/message, and log the exception), ensuring any UI-facing
state/sideEffect posting is done on the main dispatcher if required.
| item { | ||
| ApplicationCard( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| title = stringResource(id = R.string.remains_application), | ||
| appliedTitle = remainsOptionTitle, | ||
| description = stringResource(id = R.string.remains_description), | ||
| buttonText = stringResource(id = R.string.remains_do_application), | ||
| onButtonClick = onNavigateToRemains, | ||
| ) | ||
| ApplicationCard( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| title = stringResource(id = R.string.outing_application), | ||
| description = stringResource(id = R.string.outing_description), | ||
| buttonText = stringResource(id = R.string.outing_do_application), | ||
| onButtonClick = onNavigateToOuting, | ||
| ) | ||
| ApplicationCard( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .topPadding(), | ||
| title = stringResource(id = R.string.volunteers_application), | ||
| description = stringResource(id = R.string.volunteers_description), | ||
| buttonText = stringResource(id = R.string.volunteers_do_description), | ||
| onButtonClick = onNavigateToVolunteers, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Split cards into separate items for proper lazy loading.
All three ApplicationCard components are wrapped in a single item {} block, which prevents the LazyColumn from efficiently virtualizing them. Each card should be in its own item {} block to enable proper lazy loading and improve performance, especially if the list grows or if cards are expensive to render.
Apply this diff:
- item {
+ item {
ApplicationCard(
modifier = Modifier.fillMaxWidth(),
title = stringResource(id = R.string.remains_application),
appliedTitle = remainsOptionTitle,
description = stringResource(id = R.string.remains_description),
buttonText = stringResource(id = R.string.remains_do_application),
onButtonClick = onNavigateToRemains,
)
+ }
+ item {
ApplicationCard(
modifier = Modifier.fillMaxWidth(),
title = stringResource(id = R.string.outing_application),
description = stringResource(id = R.string.outing_description),
buttonText = stringResource(id = R.string.outing_do_application),
onButtonClick = onNavigateToOuting,
)
+ }
+ item {
ApplicationCard(
modifier = Modifier
.fillMaxWidth()
.topPadding(),
title = stringResource(id = R.string.volunteers_application),
description = stringResource(id = R.string.volunteers_description),
buttonText = stringResource(id = R.string.volunteers_do_description),
onButtonClick = onNavigateToVolunteers,
)
}๐ค Prompt for AI Agents
In
feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt
around lines 182 to 207, the three ApplicationCard calls are all inside a single
item { } block; split them so each ApplicationCard is wrapped in its own item {
} block (i.e., create three separate item { ApplicationCard(...) } entries)
keeping the existing modifiers, parameters and order intact to enable proper
LazyColumn virtualization and improve performance.
There was a problem hiding this comment.
Code Review
์จ๋ณด๋ฉ ๊ธฐ๋ฅ ๊ตฌํ์ ์ํ PR ์ ๋ณด์์ต๋๋ค. ์ ๋ฐ์ ์ผ๋ก ์๋ก์ด ๊ธฐ๋ฅ๋ค์ด ์ ๊ตฌํ๋์๊ณ , ๋์์ธ ์์คํ ์ปดํฌ๋ํธ๋ค์ด ์ถ๊ฐ/๊ฐ์ ๋ ์ ์ด ์ธ์์ ์ ๋๋ค. ๋ช ๊ฐ์ง ๊ฐ์ ์ ์ ์ ์๋๋ฆฝ๋๋ค. ViewModel์์ API ์์ฒญ ์คํจ ์ ์ค๋ฅ ์ฒ๋ฆฌ๊ฐ ๋๋ฝ๋ ๋ถ๋ถ๋ค์ด ์์ด ์ฌ์ฉ์ ๊ฒฝํ์ ํด์น ์ ์์ต๋๋ค. ๋ํ, ์ผ๋ถ UI ์ฝ๋์์ ์ผ๊ด์ฑ์ด๋ ๊ฐ๋ ์ฑ์ ๋์ผ ์ ์๋ ๋ถ๋ถ์ด ๋ณด์ ๋๋ค. ์์ธํ ๋ด์ฉ์ ๊ฐ ํ์ผ์ ๋จ๊ธด ๋ฆฌ๋ทฐ ์ฝ๋ฉํธ๋ฅผ ํ์ธํด์ฃผ์ธ์.
| runCatching { | ||
| studyRoomRepository.fetchAppliedStudyRoom() | ||
| }.onSuccess { appliedStudyRoom -> | ||
| setState { stateFlow.value.copy(appliedStudyRoom = appliedStudyRoom) } | ||
| } |
There was a problem hiding this comment.
runCatching์ onFailure ๋ธ๋ก์ด ๋น์ด์์ต๋๋ค. ๋ฐ์ดํฐ ๋ก๋ฉ์ ์คํจํ ๊ฒฝ์ฐ ์ค๋ฅ๊ฐ ์กฐ์ฉํ ๋ฌด์๋์ด ์ฌ์ฉ์๋ ์๋ฌด๋ฐ ํผ๋๋ฐฑ์ ๋ฐ์ง ๋ชปํ๊ณ , ์ฑ์ด ๋น์ ์์ ์ผ๋ก ๋์ํ๋ ๊ฒ์ฒ๋ผ ๋ณด์ผ ์ ์์ต๋๋ค. SideEffect๋ฅผ ํตํด UI์ ์ค๋ฅ๋ฅผ ์๋ฆฌ๊ณ ์ฌ์ฉ์์๊ฒ ์ ์ ํ ๋ฉ์์ง๋ฅผ ๋ณด์ฌ์ฃผ๋ ๊ฒ์ด ์ค์ํฉ๋๋ค. fetchAppliedRemainsOption๊ณผ fetchAllVoteSearch ๋ฉ์๋์๋ ๋์ผํ ๋ฌธ์ ๊ฐ ์์ต๋๋ค.
| private fun fetchMyPage() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| studentRepository.fetchMyPage() | ||
| }.onSuccess { myPage -> | ||
| setState { | ||
| stateFlow.value.copy(myPage = myPage) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun signOut() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| authRepository.signOut() | ||
| }.onSuccess { | ||
| postSideEffect(MyPageSideEffect.SignOutSuccess) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun withdraw() { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| studentRepository.withdraw() | ||
| }.onSuccess { | ||
| postSideEffect(MyPageSideEffect.WithdrawalSuccess) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ViewModel ๋ด์ ์ฌ๋ฌ runCatching ๋ธ๋ก์์ onFailure ์ฒ๋ฆฌ๊ฐ ๋๋ฝ๋์์ต๋๋ค. (fetchMyPage, signOut, withdraw)
API ํธ์ถ์ด ์คํจํ ๊ฒฝ์ฐ ์ค๋ฅ๊ฐ ์กฐ์ฉํ ๋ฌด์๋์ด ์ฌ์ฉ์ ๊ฒฝํ์ ํด์น ์ ์์ต๋๋ค. ๊ฐ ์ผ์ด์ค์ ๋ง๋ ์ค๋ฅ๋ฅผ SideEffect๋ก UI ๋ ์ด์ด์ ์ ๋ฌํ์ฌ ์ฌ์ฉ์์๊ฒ ์ ์ ํ ํผ๋๋ฐฑ(์: ํ ์คํธ ๋ฉ์์ง)์ ์ ๊ณตํด์ผ ํฉ๋๋ค.
| runCatching { | ||
| onboardingDataSource.getOnboardingCompleted() | ||
| }.onSuccess { isCompleted -> | ||
| setState { | ||
| stateFlow.value.copy( | ||
| isOnboardingCompleted = isCompleted, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
runCatching์ onFailure ๋ธ๋ก์ด ๋น์ด์์ต๋๋ค. ์จ๋ณด๋ฉ ์๋ฃ ์ฌ๋ถ๋ฅผ ๊ฐ์ ธ์ค๋ ๋ฐ ์คํจํ๋ฉด ์ค๋ฅ๊ฐ ์กฐ์ฉํ ๋ฌด์๋ฉ๋๋ค. ์ด ๊ฒฝ์ฐ ๊ธฐ๋ณธ๊ฐ์ธ false๋ก ์ฒ๋ฆฌ๋์ด, ์ด๋ฏธ ์จ๋ณด๋ฉ์ ์๋ฃํ ์ฌ์ฉ์์๊ฒ๋ ์จ๋ณด๋ฉ ํ๋ฉด์ด ๋ค์ ๋ณด์ผ ์ ์์ต๋๋ค. ์คํจ ์ ์ ์ ํ ์ฒ๋ฆฌ๊ฐ ํ์ํฉ๋๋ค.
| add("prodImplementation", libs.androidx.hilt.navigation.compose) | ||
| add("devImplementation", libs.androidx.navigation3.runtime) | ||
| add("devImplementation", libs.androidx.navigation3.ui) | ||
| add("devImplementation", libs.androidx.hilt.navigation.compose) |
There was a problem hiding this comment.
hilt-navigation-compose ์ข
์์ฑ์ด prodImplementation๊ณผ devImplementation์ ์ค๋ณต์ผ๋ก ์ถ๊ฐ๋์์ต๋๋ค. ์ด ์ข
์์ฑ์ด ๋ ๋น๋ ์ ํ ๋ชจ๋์ ํ์ํ๋ค๋ฉด implementation์ผ๋ก ํ ๋ฒ๋ง ์ ์ธํ๋ ๊ฒ์ด ์ข์ต๋๋ค. ์ด๋ ๊ฒ ํ๋ฉด ์ค๋ณต์ ํผํ๊ณ Gradle ์คํฌ๋ฆฝํธ๋ฅผ ๋ ๊น๋ํ๊ฒ ์ ์งํ ์ ์์ต๋๋ค.
implementation(libs.androidx.hilt.navigation.compose)
add("devImplementation", libs.androidx.navigation3.runtime)
add("devImplementation", libs.androidx.navigation3.ui)| ), | ||
| ) | ||
|
|
||
| else -> throw IllegalArgumentException("Unhandled ButtonColor: $this") |
There was a problem hiding this comment.
when ํํ์์์ ButtonColor์ ๋ชจ๋ enum ์ผ์ด์ค(Primary, Gray, Error)๋ฅผ ์ด๋ฏธ ์ฒ๋ฆฌํ๊ณ ์์ผ๋ฏ๋ก else ๋ถ๊ธฐ๋ ๋๋ฌํ ์ ์๋ ์ฝ๋์
๋๋ค. else๋ฅผ ์ ๊ฑฐํ๋ฉด ์ปดํ์ผ๋ฌ๊ฐ ๋ชจ๋ ์ผ์ด์ค๊ฐ ์ฒ๋ฆฌ๋์๋์ง ํ์ธํ ์ ์์ด, ํฅํ ์๋ก์ด ButtonColor๊ฐ ์ถ๊ฐ๋ ๋ ์ค์๋ฅผ ๋ฐฉ์งํ ์ ์์ต๋๋ค.
| }.onFailure { | ||
| it.printStackTrace() | ||
| } |
There was a problem hiding this comment.
๋คํธ์ํฌ ์์ฒญ ์คํจ ์ it.printStackTrace()๋ง ํธ์ถํ๊ณ ์์ต๋๋ค. ๊ฐ๋ฐ ์ค์๋ ๋ฌธ์ ๊ฐ ์์ด ๋ณด์ผ ์ ์์ง๋ง, ์ฌ์ฉ์์๊ฒ ์ค๋ฅ์ ๋ํ ํผ๋๋ฐฑ์ ์ ๊ณตํ์ง ์์ต๋๋ค. SideEffect๋ฅผ Unit์ด ์๋ ๋ณ๋์ sealed class/interface๋ก ์ ์ํ์ฌ UI ๋ ์ด์ด์ ์ค๋ฅ ์ํ๋ฅผ ์ ๋ฌํ๊ณ , ์ฌ์ฉ์์๊ฒ ํ ์คํธ ๋ฉ์์ง ๋ฑ์ผ๋ก ์ค๋ฅ ์ํฉ์ ์๋ ค์ฃผ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์.
| ) { _, dragAmount -> | ||
| if (currentCardType == LUNCH) { | ||
| when { | ||
| dragAmount > 0 -> onSwipeToLeft() | ||
| dragAmount < 0 -> onSwipeToRight() | ||
| } | ||
| } else { | ||
| when { | ||
| dragAmount > 0 -> dragDirection = DragDirection.LEFT | ||
| dragAmount < 0 -> dragDirection = DragDirection.RIGHT | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
์ ์ฌ(LUNCH) ์นด๋์ ๋ค๋ฅธ ์นด๋(์์นจ, ์ ๋ )์ ์ค์์ดํ ๋์ ์ฒ๋ฆฌ๊ฐ ๋ค๋ฆ ๋๋ค. ์ ์ฌ ์นด๋๋ ๋๋๊ทธํ๋ ๋์ ํ์ด์ง ์ ํ์ด ์ฆ์ ๋ฐ์ํ๋ ๋ฐ๋ฉด, ๋ค๋ฅธ ์นด๋๋ค์ ๋๋๊ทธ๊ฐ ๋๋์ผ ์ ํ๋ฉ๋๋ค. ์ด๋ ์ฌ์ฉ์์๊ฒ ์ผ๊ด์ฑ ์๋ ๊ฒฝํ์ ์ค ์ ์์ต๋๋ค. ๋ชจ๋ ์นด๋์ ๋ํด ๋์ผํ ์ค์์ดํ ๋์(์: ๋๋๊ทธ๊ฐ ๋๋ฌ์ ๋ ํ์ด์ง ์ ํ)์ ์ ์ฉํ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์.
| println("LOGLOG1") | ||
| val capturedDate = stateFlow.value.selectedDate | ||
| runCatching { | ||
| mealRepository.updateMeal(capturedDate) | ||
| }.onSuccess { meal -> | ||
| setState { | ||
| stateFlow.value.copy( | ||
| selectedDate = capturedDate, | ||
| currentMeal = meal, | ||
| ) | ||
| } | ||
| postSideEffect(HomeSideEffect.MealUpdated) | ||
| }.onFailure { | ||
| postSideEffect(HomeSideEffect.MealUpdateFailed) | ||
| }.also { | ||
| println("LOGLOG ${it.isSuccess}") |
There was a problem hiding this comment.
๋๋ฒ๊น
๋ชฉ์ ์ผ๋ก ์ฌ์ฉ๋ println ๋ฌธ๋ค์ด ๋จ์์์ต๋๋ค. ํ๋ก๋์
์ฝ๋์ ํฌํจ๋์ง ์๋๋ก ๋ณํฉ ์ ์ ์ ๊ฑฐํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
| LaunchedEffect(Unit) { | ||
| delay(300) | ||
| step = 1 | ||
| delay(800) | ||
| isPointVisible = true | ||
| delay(1000) | ||
| step = 0 | ||
| delay(300) | ||
| step = 2 | ||
| delay(1300) | ||
| step = 0 | ||
| delay(300) | ||
| step = 3 | ||
| delay(1200) | ||
| onAnimatedEnd() |
There was a problem hiding this comment.
์ธํธ๋ก ์ ๋๋ฉ์ด์ ์ ์ด ์ฌ์ ์๊ฐ์ด 5์ด ์ด์์ผ๋ก ๋ค์ ๊ธด ํธ์ ๋๋ค. ์ฌ์ฉ์๊ฐ ์ฑ์ ์ฒ์ ์คํํ ๋ ์ง๋ฃจํจ์ ๋๋ ์ ์์ผ๋, ์ ์ฒด์ ์ธ ์ ๋๋ฉ์ด์ ์๊ฐ์ ์ค์ด๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด๋ ๊ฒ์ด ์ข๊ฒ ์ต๋๋ค.
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(2.dp) | ||
| .background(if (selectedTab == index) DmsTheme.colorScheme.onBackground else DmsTheme.colorScheme.line) |
There was a problem hiding this comment.
if (selectedTab == index) ์กฐ๊ฑด๋ฌธ ์์ ์๊ธฐ ๋๋ฌธ์ background์ else ๋ถ๊ธฐ๋ ์ ๋ ์คํ๋์ง ์์ต๋๋ค. ๋ถํ์ํ ์ฝ๋๋ฅผ ์ ๊ฑฐํ์ฌ ๊ฐ๋
์ฑ์ ๋์ผ ์ ์์ต๋๋ค.
| .background(if (selectedTab == index) DmsTheme.colorScheme.onBackground else DmsTheme.colorScheme.line) | |
| .background(DmsTheme.colorScheme.onBackground) |
There was a problem hiding this comment.
Actionable comments posted: 4
๐งน Nitpick comments (2)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt (1)
10-14: PreferderivedStateOffor deriving state from WindowInsets.While this implementation works,
rememberUpdatedStateis typically used to capture the latest callback reference, not to derive state from other state sources. The idiomatic pattern for observing IME visibility isderivedStateOfwithremember.Apply this diff to use the more idiomatic approach:
@Composable fun keyboardAsState(): State<Boolean> { - val isImeVisible = WindowInsets.ime.getBottom(LocalDensity.current) > 0 - return rememberUpdatedState(newValue = isImeVisible) + val density = LocalDensity.current + return remember { + derivedStateOf { + WindowInsets.ime.getBottom(density) > 0 + } + } }core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (1)
104-181: Kotlin naming polish for readability.textcolorScheme/underlinecolorScheme(Lines 105, 144) andbuttoncolorScheme(Line 261) break standard camelCase (textColorScheme,underlineColorScheme,buttonColorScheme).Also applies to: 261-265
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (11)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/DmsNavigator.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavGraph.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavigator.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Typography.kt(2 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/composeExt.kt(1 hunks)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt(1 hunks)core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Button.kt(1 hunks)core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Theme.kt(1 hunks)
โ Files skipped from review due to trivial changes (2)
- app/src/prod/kotlin/team/aliens/dms/android/app/navigation/DmsNavigator.kt
- core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Theme.kt
๐ง Files skipped from review as they are similar to previous changes (2)
- core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Typography.kt
- core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/composeExt.kt
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (7)
app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavGraph.kt (1)
17-17: LGTM! Import organization improved.The import reordering has no functional impact and improves code organization.
Also applies to: 26-26
app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavigator.kt (1)
14-14: LGTM! Import relocation with no functional impact.The import statement reordering maintains the same interface contract and has no effect on runtime behavior.
core/design-system/src/prod/java/team/aliens/dms/android/core/designsystem/Button.kt (1)
1-1: Unable to review - file content not fully provided.Only the package declaration is visible in the provided code. According to the AI summary, this file should contain
ButtonState,ButtonColor,ButtonType,ButtonTheme, extension functions, and composables (BasicButton,DmsButton), but none of these are shown in the annotated code section.Please ensure the complete file content is provided for a thorough review of the button component implementation, including validation of:
- Enum and data class definitions
- Color scheme mappings
- Composable button logic and accessibility
- State management and interaction handling
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (3)
59-66: Well implemented: Addresses past review feedback.The navigation from onboarding to sign-in correctly clears the back stack before adding
SignInScreenNav, which prevents users from navigating back to the onboarding screen after completion. This addresses the past review comment from gemini-code-assist[bot].
54-77: Navigation structure looks good.The NavDisplay setup with three top-level navigation entries (Onboarding, SignIn, Main) is clean and appropriate for the app's navigation hierarchy. The system bars padding modifier ensures proper edge-to-edge display.
41-52: This review comment raises incorrect concerns. TheisOnboardingCompletedStateFlow is read only once duringMainActivityViewModelinitialization and never updated afterward, so it will not re-emit the same value or cause unnecessaryLaunchedEffectre-runs. The actual navigation toSignInScreenNavafter onboarding completion happens via the manual callback (lines 62-63), not through the reactiveLaunchedEffectpattern. The guard condition and concerns about concurrent navigation are not relevant to the current code flow since the effect itself does not re-trigger when onboarding completes.Likely an incorrect or invalid review comment.
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.kt (1)
12-12: Good disambiguation ofTypographyvia alias. Line 12 keepsMaterialTheme(typography=...)readable and avoids name collisions.
| @Composable | ||
| private fun ButtonColor.containedColorScheme() = when (this) { | ||
| ButtonColor.Primary -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimary, | ||
| backgroundColor = DmsTheme.colorScheme.primary, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimary, | ||
| backgroundColor = DmsTheme.colorScheme.secondaryContainer, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimaryContainer, | ||
| backgroundColor = DmsTheme.colorScheme.inversePrimary, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Gray -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| backgroundColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| backgroundColor = DmsTheme.colorScheme.surfaceVariant, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| backgroundColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Error -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimary, | ||
| backgroundColor = DmsTheme.colorScheme.onError, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimary, | ||
| backgroundColor = DmsTheme.colorScheme.inverseOnSurface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimaryContainer, | ||
| backgroundColor = DmsTheme.colorScheme.errorContainer, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| @Composable | ||
| private fun ButtonColor.textcolorScheme() = when (this) { | ||
| ButtonColor.Primary -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.primaryContainer, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Gray -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onSurfaceVariant, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Error -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.primaryContainer, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onPrimary, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| @Composable | ||
| private fun ButtonColor.underlinecolorScheme() = when (this) { | ||
| ButtonColor.Primary -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.primaryContainer, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Gray -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onSurfaceVariant, | ||
| ), | ||
| ) | ||
|
|
||
| ButtonColor.Error -> ButtonState( | ||
| enabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.surface, | ||
| ), | ||
| pressed = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.errorContainer, | ||
| ), | ||
| disabled = ButtonTheme( | ||
| textColor = DmsTheme.colorScheme.onError, | ||
| ), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
find . -type f -name "*.kt" | xargs grep -l "DmsTheme\|Colors" | head -20Repository: team-aliens/DMS-Android
Length of output: 1822
๐ Script executed:
cat -n ./core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Colors.ktRepository: team-aliens/DMS-Android
Length of output: 20195
๐ Script executed:
cat -n ./core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.ktRepository: team-aliens/DMS-Android
Length of output: 5977
๐ Script executed:
sed -n '56,181p' ./core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt | cat -nRepository: team-aliens/DMS-Android
Length of output: 4848
๐ Script executed:
# Check if there are other Button-related files that might provide context
find . -type f -name "*Button*" | grep -E "\.(kt|xml)$"Repository: team-aliens/DMS-Android
Length of output: 420
๐ Script executed:
# Check how ButtonTheme is used - does it require backgroundColor?
rg -n "data class ButtonTheme|class ButtonTheme" --type=kotlin -A10Repository: team-aliens/DMS-Android
Length of output: 1354
ButtonColor.Gray contained button is unreadable and Error button uses inverted semantic tokens.
In containedColorScheme():
- Gray enabled (lines 20โ21): Sets
textColorandbackgroundColorboth tosurface(white/light), producing unreadable white-on-white text. - Error enabled (line 36): Sets
backgroundColor = onError, using a foreground token (semantically meant for text) as a background, which inverts the design system's token semantics.
๐ค Prompt for AI Agents
In
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt
(lines 56โ181) the containedColorScheme has two issues: Gray.enabled sets both
textColor and backgroundColor to surface causing unreadable text, and
Error.enabled uses the foreground token onError as a background. Fix by changing
ButtonColor.Gray.enabled to use a readable foreground (e.g. textColor =
DmsTheme.colorScheme.onSurface) while keeping backgroundColor =
DmsTheme.colorScheme.surface (or surfaceVariant if intended), and change
ButtonColor.Error.enabled to use the error background token and matching
foreground (e.g. backgroundColor = DmsTheme.colorScheme.error and textColor =
DmsTheme.colorScheme.onError) so semantic token roles are preserved.
| private val DmsLightColorScheme = lightColorScheme( | ||
| primary = DmsColor.Light.blue400, | ||
| primary = DmsColor.Light.blue300, | ||
| onPrimary = DmsColor.Light.gray50, | ||
| primaryContainer = DmsColor.Light.blue100, | ||
| onPrimaryContainer = DmsColor.Light.blue500, | ||
| error = DmsColor.Light.red300, | ||
| onError = DmsColor.Light.gray50, | ||
| errorContainer = DmsColor.Light.red50, | ||
| onErrorContainer = DmsColor.Light.red500, | ||
| secondary = DmsColor.Light.blue300, | ||
| onSecondary = DmsColor.Light.gray50, | ||
| primaryContainer = DmsColor.Light.blue200, | ||
| onPrimaryContainer = DmsColor.Light.gray100, | ||
| inversePrimary = DmsColor.Light.blue400, | ||
| secondary = DmsColor.Light.blue500, | ||
| onSecondary = DmsColor.Light.gray200, | ||
| secondaryContainer = DmsColor.Light.blue100, | ||
| onSecondaryContainer = DmsColor.Light.blue500, | ||
| tertiary = DmsColor.Light.blue200, | ||
| onTertiary = DmsColor.Light.gray50, | ||
| tertiaryContainer = DmsColor.Light.blue50, | ||
| onTertiaryContainer = DmsColor.Light.blue500, | ||
| background = DmsColor.Light.gray50, | ||
| onSecondaryContainer = DmsColor.Light.gray300, | ||
| tertiary = DmsColor.Light.blue50, | ||
| onTertiary = DmsColor.Light.gray500, | ||
| tertiaryContainer = DmsColor.Light.gray600, | ||
| onTertiaryContainer = DmsColor.Light.gray700, | ||
| background = DmsColor.Light.background, | ||
| onBackground = DmsColor.Light.gray900, | ||
| surface = DmsColor.Light.gray100, | ||
| onSurface = DmsColor.Light.gray900, | ||
| surfaceVariant = DmsColor.Light.gray200, | ||
| onSurfaceVariant = DmsColor.Light.gray700, | ||
| inverseSurface = DmsColor.Light.gray800, | ||
| inverseOnSurface = DmsColor.Light.gray50, | ||
| outline = DmsColor.Light.gray400, | ||
| outlineVariant = DmsColor.Light.gray300, | ||
| surfaceTint = DmsColor.Light.blue300, | ||
| surface = DmsColor.Light.container, | ||
| onSurface = DmsColor.Light.gray800, | ||
| surfaceVariant = DmsColor.Light.gray400, | ||
| onSurfaceVariant = DmsColor.Light.button, | ||
| surfaceTint = DmsColor.Light.hover, | ||
| inverseSurface = DmsColor.Light.red50, | ||
| inverseOnSurface = DmsColor.Light.red100, | ||
| error = DmsColor.Light.red200, | ||
| onError = DmsColor.Light.red300, | ||
| errorContainer = DmsColor.Light.red400, | ||
| onErrorContainer = DmsColor.Light.red500, | ||
| outline = DmsColor.Light.black, | ||
| outlineVariant = DmsColor.Light.pressed, | ||
| ) |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
find . -type f -name "Theme.kt" | grep -E "designsystem|dms" | head -20Repository: team-aliens/DMS-Android
Length of output: 237
๐ Script executed:
cat "core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Theme.kt"Repository: team-aliens/DMS-Android
Length of output: 5018
๐ Script executed:
find . -type f -name "*.kt" | xargs grep -l "object DmsColor" | head -5Repository: team-aliens/DMS-Android
Length of output: 49
๐ Script executed:
find . -type f -name "*.kt" -o -name "*.md" | xargs grep -l "inverseSurface\|Material.*Color" | head -10Repository: team-aliens/DMS-Android
Length of output: 368
๐ Script executed:
cat "core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Colors.kt"Repository: team-aliens/DMS-Android
Length of output: 17045
๐ Web query:
Material Design 3 inverseSurface inverseOnSurface token semantics meaning
๐ก Result:
Short answer
- inverseSurface: a surface role whose tone is the โcontrastโflippedโ surface relative to the regular surface (used where you need a surface that stands out from the page, e.g., SnackBar, dark bottom bar on a light screen). [1][3][4]
- inverseOnSurface: the content (text/icon) color that is guaranteed to contrast well on inverseSurface. Use it for labels, icons, and text placed on inverseSurface. [1][3][10]
Why it exists
- Material 3 separates semantic roles (system tokens) from raw colors so components can pick colors by role. inverseSurface/inverseOnSurface are the pair for surfaces that intentionally invert contrast to draw attention or provide strong separation. [10][11]
Typical uses (examples)
- SnackBar background = inverseSurface; SnackBar text/icon = inverseOnSurface. [3]
- App bars, bottom bars, banners, or other surfaces that must sharply contrast with the surrounding surface. [4][5]
Implementation notes
- Platform libraries expose these as ColorScheme tokens (android/Compose, Flutter, Material Web). If unspecified some implementations derive inverseSurface from onSurface or use generated dynamic colors. Always use the on- counterpart for accessible contrast. [1][3][6][10]
Sources
[1] Android Material3 ColorScheme docs (inverseSurface / inverseOnSurface).
[3] Flutter ColorScheme.inverseSurface docs (behavior & example).
[4] Material Web theming & color-token explanation.
[5] Practical guides / blog posts explaining inverseSurface usage in M3.
๐ Script executed:
rg "inverseSurface|inverseOnSurface" --type kt | head -20Repository: team-aliens/DMS-Android
Length of output: 93
๐ Script executed:
rg -i "snackbar|Toast" --type kt | grep -E "(class|fun|import)" | head -20Repository: team-aliens/DMS-Android
Length of output: 93
๐ Script executed:
cat "core/ui/src/main/java/team/aliens/dms/android/core/ui/toast/ToastLayout.kt"Repository: team-aliens/DMS-Android
Length of output: 173
๐ Script executed:
python3 << 'EOF'
# Calculate contrast ratios between colors
def hex_to_rgb(hex_color):
hex_color = hex_color.lstrip('#')
return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4))
def relative_luminance(r, g, b):
# Normalize to 0-1
r, g, b = r/255, g/255, b/255
# Apply gamma correction
r = r/12.92 if r <= 0.03928 else ((r+0.055)/1.055) ** 2.4
g = g/12.92 if g <= 0.03928 else ((g+0.055)/1.055) ** 2.4
b = b/12.92 if b <= 0.03928 else ((b+0.055)/1.055) ** 2.4
return 0.2126*r + 0.7152*g + 0.0722*b
def contrast_ratio(l1, l2):
lighter = max(l1, l2)
darker = min(l1, l2)
return (lighter + 0.05) / (darker + 0.05)
# Colors from the theme
surface = hex_to_rgb("FFFAFAFA") # Light background
inverse_surface = hex_to_rgb("FFFCE4EC") # Toast background
inverse_on_surface = hex_to_rgb("FFF8BBD0") # Toast text
# Calculate luminances
l_surface = relative_luminance(*surface)
l_inverse = relative_luminance(*inverse_surface)
l_inverse_on = relative_luminance(*inverse_on_surface)
# Calculate contrasts
contrast_bg = contrast_ratio(l_surface, l_inverse)
contrast_text = contrast_ratio(l_inverse, l_inverse_on)
print(f"Surface: {surface} -> Luminance: {l_surface:.4f}")
print(f"inverseSurface: {inverse_surface} -> Luminance: {l_inverse:.4f}")
print(f"inverseOnSurface: {inverse_on_surface} -> Luminance: {l_inverse_on:.4f}")
print()
print(f"Contrast (surface vs inverseSurface): {contrast_bg:.2f}:1")
print(f"Contrast (inverseSurface vs inverseOnSurface): {contrast_text:.2f}:1")
print()
print("WCAG AA requires 4.5:1 for text, 3:1 for graphics")
print(f"Toast bg vs page bg: {'โ FAILS' if contrast_bg < 3 else 'โ PASSES'}")
print(f"Toast text vs toast bg: {'โ FAILS' if contrast_text < 4.5 else 'โ PASSES'}")
EOFRepository: team-aliens/DMS-Android
Length of output: 153
๐ Script executed:
python3 << 'EOF'
def hex_to_rgb(hex_str):
h = hex_str.lstrip('#')
return tuple(int(h[i:i+2], 16) for i in (0, 2, 4))
def luminance(rgb):
r, g, b = [x/255.0 for x in rgb]
r = r/12.92 if r <= 0.03928 else ((r+0.055)/1.055)**2.4
g = g/12.92 if g <= 0.03928 else ((g+0.055)/1.055)**2.4
b = b/12.92 if b <= 0.03928 else ((b+0.055)/1.055)**2.4
return 0.2126*r + 0.7152*g + 0.0722*b
def contrast(l1, l2):
return (max(l1, l2) + 0.05) / (min(l1, l2) + 0.05)
# From Colors.kt
surface_rgb = hex_to_rgb("FFFAFAFA")
inverse_surface_rgb = hex_to_rgb("FFFCE4EC")
inverse_on_surface_rgb = hex_to_rgb("FFF8BBD0")
l_surface = luminance(surface_rgb)
l_inverse_surf = luminance(inverse_surface_rgb)
l_inverse_on = luminance(inverse_on_surface_rgb)
c1 = contrast(l_surface, l_inverse_surf)
c2 = contrast(l_inverse_surf, l_inverse_on)
print(f"Toast bg vs page bg contrast: {c1:.2f}:1 (needs 3:1, actual: {'FAIL' if c1 < 3 else 'PASS'})")
print(f"Toast text vs toast bg contrast: {c2:.2f}:1 (needs 4.5:1, actual: {'FAIL' if c2 < 4.5 else 'PASS'})")
EOFRepository: team-aliens/DMS-Android
Length of output: 153
๐ Script executed:
python3 << 'EOF'
def hex_to_rgb(hex_str):
h = hex_str.lstrip('#')
return tuple(int(h[i:i+2], 16) for i in (0, 2, 4))
def luminance(rgb):
r, g, b = [x/255.0 for x in rgb]
r = r/12.92 if r <= 0.03928 else ((r+0.055)/1.055)**2.4
g = g/12.92 if g <= 0.03928 else ((g+0.055)/1.055)**2.4
b = b/12.92 if b <= 0.03928 else ((b+0.055)/1.055)**2.4
return 0.2126*r + 0.7152*g + 0.0722*b
def contrast(l1, l2):
return (max(l1, l2) + 0.05) / (min(l1, l2) + 0.05)
print("=" * 60)
print("LIGHT THEME")
print("=" * 60)
# Light theme from Colors.kt
surface_light = hex_to_rgb("FFFFFFFF") # container
inverse_surface_light = hex_to_rgb("FFFFE7E7") # red50
inverse_on_surface_light = hex_to_rgb("FFFEB1B1") # red100
on_surface_light = hex_to_rgb("FF121212") # gray800
l_surface = luminance(surface_light)
l_inverse = luminance(inverse_surface_light)
l_inverse_on = luminance(inverse_on_surface_light)
l_on_surface = luminance(on_surface_light)
c_surface_vs_inverse = contrast(l_surface, l_inverse)
c_inverse_vs_text = contrast(l_inverse, l_inverse_on)
print(f"surface: #FFFFFFFF -> luminance: {l_surface:.4f}")
print(f"inverseSurface: #FFFFE7E7 -> luminance: {l_inverse:.4f}")
print(f"inverseOnSurface: #FFFEB1B1 -> luminance: {l_inverse_on:.4f}")
print(f"onSurface: #FF121212 -> luminance: {l_on_surface:.4f}")
print()
print(f"Contrast (surface vs inverseSurface): {c_surface_vs_inverse:.2f}:1 (WCAG requires 3:1) -> {'โ FAIL' if c_surface_vs_inverse < 3 else 'โ PASS'}")
print(f"Contrast (inverseSurface vs inverseOnSurface text): {c_inverse_vs_text:.2f}:1 (WCAG requires 4.5:1) -> {'โ FAIL' if c_inverse_vs_text < 4.5 else 'โ PASS'}")
print("\n" + "=" * 60)
print("DARK THEME")
print("=" * 60)
# Dark theme from Colors.kt
surface_dark = hex_to_rgb("FF171717") # container
inverse_surface_dark = hex_to_rgb("FF530505") # red50
inverse_on_surface_dark = hex_to_rgb("FFCB0C0C") # red100
on_surface_dark = hex_to_rgb("FFF9F9F9") # gray800
l_surface_d = luminance(surface_dark)
l_inverse_d = luminance(inverse_surface_dark)
l_inverse_on_d = luminance(inverse_on_surface_dark)
l_on_surface_d = luminance(on_surface_dark)
c_surface_vs_inverse_d = contrast(l_surface_d, l_inverse_d)
c_inverse_vs_text_d = contrast(l_inverse_d, l_inverse_on_d)
print(f"surface: #FF171717 -> luminance: {l_surface_d:.4f}")
print(f"inverseSurface: #FF530505 -> luminance: {l_inverse_d:.4f}")
print(f"inverseOnSurface: #FFCB0C0C -> luminance: {l_inverse_on_d:.4f}")
print(f"onSurface: #FFF9F9F9 -> luminance: {l_on_surface_d:.4f}")
print()
print(f"Contrast (surface vs inverseSurface): {c_surface_vs_inverse_d:.2f}:1 (WCAG requires 3:1) -> {'โ FAIL' if c_surface_vs_inverse_d < 3 else 'โ PASS'}")
print(f"Contrast (inverseSurface vs inverseOnSurface text): {c_inverse_vs_text_d:.2f}:1 (WCAG requires 4.5:1) -> {'โ FAIL' if c_inverse_vs_text_d < 4.5 else 'โ PASS'}")
EOFRepository: team-aliens/DMS-Android
Length of output: 1024
Fix inverseSurface/inverseOnSurface color mappingsโcritical WCAG accessibility failure. Current mappings violate accessibility standards on both themes. Light theme produces 1.01:1 contrast between surface and inverseSurface, and 1.03:1 between inverseSurface and text (both require 3:1 and 4.5:1 minimum). Dark theme similarly fails with 1.21:1 and 2.13:1 ratios. These tokens are actively used in Toast components, making notifications invisible to users. Material 3 specifies inverseSurface should be a sharply contrasting surface (dark in light themes, bright in dark themes) with inverseOnSurface providing readable text contrast on top.
There was a problem hiding this comment.
Actionable comments posted: 4
โป๏ธ Duplicate comments (14)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (1)
61-80: Remove debug println statements before merging.Debug logging statements at lines 63 and 78 should be removed from production code.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt (2)
534-556: Fix inconsistent drag handling to prevent multiple navigations per gesture.The LUNCH card triggers navigation on every drag event (lines 544-548), while BREAKFAST and DINNER cards only trigger on drag end. This causes multiple page transitions for a single gesture on the LUNCH card and provides an inconsistent user experience.
647-654: Guard against missing vibrator service to prevent NPE.The unsafe cast at line 652 will crash on devices without a vibrator.
getSystemServicecan returnnull, and the cast will throw an exception.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt (2)
66-146: Remove commented-out legacy code.This large block of commented-out MVI implementation adds noise and can be retrieved from version control if needed.
Apply this diff to remove lines 66-146:
-/* - -import androidx.lifecycle.viewModelScope -... -(all commented code) -... -internal sealed class AnnouncementsSideEffect : MviSideEffect -*/
27-41: Replace printStackTrace with proper logging.Line 38 uses
printStackTrace()which is not suitable for production. Use a logging library (e.g., Timber) instead.Apply this diff:
}.onSuccess { notices -> noticesAscByDate = notices noticesDescByDate = notices.reversed() setState { uiState -> uiState.copy(notices = notices) } }.onFailure { - it.printStackTrace() + // TODO: Use Timber.e(it, "Failed to fetch notices") }feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt (1)
17-29: Handle DataStore read failures.When
getOnboardingCompleted()fails, the error is silently ignored and users default to seeing onboarding again. Consider addingonFailureto log the error or emit a side effect.Apply this diff:
runCatching { onboardingDataSource.getOnboardingCompleted() }.onSuccess { isCompleted -> setState { - uiState.value.copy( + it.copy( isOnboardingCompleted = isCompleted, ) } + }.onFailure { error -> + // Log error or emit side effect }feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt (3)
26-36: Handle repository failures and fix reducer pattern usage.Two issues:
onFailureis missing, so fetch errors are silently ignored- Line 32 uses
uiState.valueinstead of the lambda parameteritApply this diff:
private fun fetchMyPage() { viewModelScope.launch(Dispatchers.IO) { runCatching { studentRepository.fetchMyPage() }.onSuccess { myPage -> setState { - stateFlow.value.copy(myPage = myPage) + it.copy(myPage = myPage) } + }.onFailure { error -> + // Log error or emit side effect } } }
38-46: Handle sign-out failures.
onFailureis missing. If sign-out fails, users receive no feedback and may assume they've been signed out when they haven't.Apply this diff:
fun signOut() { viewModelScope.launch(Dispatchers.IO) { runCatching { authRepository.signOut() }.onSuccess { sendEffect(MyPageSideEffect.SignOutSuccess) + }.onFailure { error -> + // Log error or emit failure side effect } } }
48-56: Handle withdrawal failures.
onFailureis missing. Account withdrawal is a critical operation that requires proper error handling and user feedback.Apply this diff:
fun withdraw() { viewModelScope.launch(Dispatchers.IO) { runCatching { studentRepository.withdraw() }.onSuccess { sendEffect(MyPageSideEffect.WithdrawalSuccess) + }.onFailure { error -> + // Log error or emit failure side effect } } }feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListScreen.kt (1)
119-134: Add key parameter to items() for stable recomposition.Without a unique key, Compose may incorrectly reuse item state when the list changes. Use
notice.idas the key.Apply this diff:
LazyColumn( modifier = modifier, ) { - items(notices) { notice -> + items(notices, key = { it.id }) { notice -> NoticeCard( modifier = Modifier.fillMaxWidth(), notice = notice, onClick = onNavigateToNoticeDetails, ) }feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt (3)
30-38: Consider initializing data ininitblock instead ofonCreate.Using
DefaultLifecycleObserver.onCreatein a ViewModel is unconventional. ViewModels are lifecycle-aware but typically outlive their associated lifecycle owners (e.g., across configuration changes). Initializing data fetches in theonCreatecallback can lead to multiple re-fetches if the lifecycle is recreated and potential memory leaks if the observer isn't properly removed.
33-37: Refactor redundant coroutine nesting for clarity and efficiency.The current pattern launches a parent coroutine that calls three fetch methods, each of which launches its own
Dispatchers.IOcoroutine. This creates unnecessary nesting and unclear execution semantics where the parent completes immediately after launching the children.
40-75: Add error handling for failed data fetches.All three fetch methods use
runCatchingwith onlyonSuccesshandlers, silently ignoring failures. This leads to no user feedback when data fetches fail, difficult debugging in production, and confusing UI states.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (1)
73-86: Error button uses inverted semantic tokens.Line 76 sets
backgroundColor = DmsTheme.colorScheme.onError, using a foreground token (semantically meant for text) as a background. This inverts the design system's token semantics.Consider using the proper error background token:
ButtonColor.Error -> ButtonState( enabled = ButtonTheme( - textColor = DmsTheme.colorScheme.onPrimary, - backgroundColor = DmsTheme.colorScheme.onError, + textColor = DmsTheme.colorScheme.onError, + backgroundColor = DmsTheme.colorScheme.error, ),
๐งน Nitpick comments (10)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (3)
25-40: LGTM! Consider adding error handling for network failures.The initialization and notice fetching logic is clear. However,
fetchWhetherNewNoticeExistssilently ignores failures. Consider logging or handling errors if notice availability is important for user experience.
68-72: Use lambda parameter for consistency with setState pattern.Line 69 accesses
uiState.valuedirectly instead of using the lambda parameterit. This deviates from the pattern used elsewhere (lines 35-36, 47-50) and thesetStatedesign shown in BaseStateViewModel.Apply this diff to align with the established pattern:
}.onSuccess { meal -> setState { - uiState.value.copy( + it.copy( selectedDate = capturedDate, currentMeal = meal, )
117-249: Remove commented legacy code.The 132 lines of commented-out legacy MVI implementation should be removed. If this code needs to be referenced later, it's available in version control history.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt (3)
332-344: Add descriptive message to defensive exception.The
elsebranch at line 342 should never be reached since allDayOfWeekvalues are covered, but the exception should include a message for debuggability.Apply this diff:
DayOfWeek.FRIDAY -> R.string.friday_abb DayOfWeek.SATURDAY -> R.string.saturday_abb - else -> throw IllegalArgumentException() + else -> throw IllegalArgumentException("Unexpected DayOfWeek: $this") }, )
467-493: Add descriptive messages to defensive exceptions.The
elsebranches at lines 474 and 486 are defensive code for impossible states (page values are constrained to 0-2), but the exceptions should include messages for debuggability.Apply this diff:
Lunch -> Dinner Dinner -> Breakfast.also { onNextDay() } - else -> throw IllegalStateException() + else -> throw IllegalStateException("Unexpected page: $page") }, ) } ... Lunch -> Breakfast Dinner -> Lunch - else -> throw IllegalStateException() + else -> throw IllegalStateException("Unexpected page: $page") }, ) }
496-503: Add descriptive message to exception.The exception at line 501 should include the invalid value to aid debugging if an unexpected integer is passed.
Apply this diff:
0 -> BREAKFAST 1 -> LUNCH 2 -> DINNER - else -> throw IllegalArgumentException() + else -> throw IllegalArgumentException("Invalid meal card type index: $this") } }core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
7-7: Remove unused import.The
trySendBlockingimport is unused since Line 38 usestrySendinstead.Apply this diff:
-import kotlinx.coroutines.channels.trySendBlockingfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (3)
182-207: Consider splitting cards into separate LazyList items.Lines 182-207 render all three application cards within a single
item { }block. This treats them as a single list item, which can impact scroll performance, item animations, and recycling behavior.Consider splitting into separate items:
fun LazyListScope.applicationTabContent( onNavigateToRemains: () -> Unit, onNavigateToOuting: () -> Unit, onNavigateToVolunteers: () -> Unit, remainsOptionTitle: String?, ) { - item { + item { ApplicationCard( modifier = Modifier.fillMaxWidth(), title = stringResource(id = R.string.remains_application), appliedTitle = remainsOptionTitle, description = stringResource(id = R.string.remains_description), buttonText = stringResource(id = R.string.remains_do_application), onButtonClick = onNavigateToRemains, ) + } + item { ApplicationCard( modifier = Modifier.fillMaxWidth(), title = stringResource(id = R.string.outing_application), description = stringResource(id = R.string.outing_description), buttonText = stringResource(id = R.string.outing_do_application), onButtonClick = onNavigateToOuting, ) + } + item { ApplicationCard( modifier = Modifier .fillMaxWidth() .topPadding(), title = stringResource(id = R.string.volunteers_application), description = stringResource(id = R.string.volunteers_description), buttonText = stringResource(id = R.string.volunteers_do_description), onButtonClick = onNavigateToVolunteers, ) } }
317-334: Applied badge uses clickable button with empty handler.Lines 323-332 render the applied status as a
RoundedButtonwith an emptyonClick = { }handler. If the badge is not intended to be interactive, consider using a non-clickable component likeSurfaceorTextwith appropriate styling instead of a button.If the badge should not be clickable:
- RoundedButton( - onClick = { }, - fillMinSize = false, + Surface( + shape = DmsTheme.shapes.circle, + color = DmsTheme.colorScheme.primaryContainer, contentPadding = PaddingValues( horizontal = PaddingDefaults.Medium, vertical = PaddingDefaults.Small, ), ) { - Text(text = appliedTitle) + Text( + text = appliedTitle, + modifier = Modifier.padding( + horizontal = PaddingDefaults.Medium, + vertical = PaddingDefaults.Small, + ), + ) }
410-427: Applied badge uses clickable button with empty handler.Lines 416-425 render the applied status as a
RoundedButtonwith an emptyonClick = { }handler. If the badge is not intended to be interactive, consider using a non-clickable component instead of a button (same issue as inApplicationCard).
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (11)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt(1 hunks)core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt(1 hunks)feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt(1 hunks)
๐งฐ Additional context used
๐งฌ Code graph analysis (9)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt (2)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(33-35)feature/src/main/java/team/aliens/dms/android/feature/main/announcement/AnnouncementListViewModel.kt (1)
noticeRepository(16-58)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/announcement/AnnouncementListScreen.kt (3)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Scaffold.kt (1)
Scaffold(11-28)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/AppBars.kt (1)
DmsTopAppBar(13-37)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (1)
OutlinedButton(121-149)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(33-35)sendEffect(37-39)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(33-35)sendEffect(37-39)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt (7)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Modal.kt (1)
ModalBottomSheet(16-46)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Calendars.kt (1)
DmsCalendar(12-46)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Scaffold.kt (1)
Scaffold(11-28)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/AppBars.kt (1)
DmsTopAppBar(13-37)core/ui/src/main/java/team/aliens/dms/android/core/ui/composable/AppLogo.kt (1)
AppLogo(10-26)core/ui/src/main/java/team/aliens/dms/android/core/ui/composable/FloatingNotice.kt (1)
FloatingNotice(22-54)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (2)
OutlinedButton(121-149)TextButton(151-176)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (3)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Scaffold.kt (1)
Scaffold(11-28)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/AppBars.kt (1)
DmsTopAppBar(13-37)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (2)
RoundedButton(178-203)ContainedButton(94-119)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (2)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/util/keyboardAsState.kt (1)
keyboardAsState(10-14)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/indecator/DmsDotsLoadingIndicator.kt (1)
DmsDotsLoadingIndicator(27-68)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(33-35)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(33-35)
๐ช detekt (1.23.8)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt
[warning] 342-342: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 474-474: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 486-486: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 501-501: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt
[warning] 235-235: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 290-290: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (14)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeViewModel.kt (2)
42-59: LGTM! Function correctly handles date updates and meal fetching.The date update logic properly handles the
CannotFindMealExceptioncase. Other exceptions are silently ignored, which may be acceptable depending on your error handling strategy.
83-115: Well-structured state and side effect definitions.The
HomeUiStateandHomeSideEffectclasses are properly designed with clear semantics and a thoughtful initial state factory.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.kt (1)
99-248: Well-structured screen implementation with proper state management.The
HomeScreencomposable demonstrates good practices: lifecycle-aware state collection, proper side effect handling via toast notifications, and clean separation of UI concerns with callbacks.core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
25-45: LGTM!The base ViewModel implementation correctly uses:
Channel.CONFLATEDto handle side effects (avoiding suspended coroutines)- Reducer pattern for
setState(enabling safe state updates based on current state)- Proper lifecycle cleanup in
onCleared()This addresses the previous review concerns effectively.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.kt (1)
78-96: LGTM!The
ApplicationUiStatedata class is well-structured with nullable fields for optional data and empty lists as defaults. The companion object'sinitial()factory provides clear initialization semantics.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/Button.kt (6)
33-54: LGTM!The button state model is well-structured with clear separation between state variants (enabled/pressed/disabled) and visual variants (Primary/Error and Contained/Text/Underline).
89-114: LGTM!The text color scheme function properly maps ButtonColor cases to their corresponding color states.
116-141: LGTM!The underline color scheme function properly maps ButtonColor cases to their corresponding color states.
143-211: LGTM!The
BasicButtoncorrectly applies the border at lines 191-197 usingmodifyIfwhenborderColor != Color.Transparent. The keyboard-aware padding and shape adjustments are well-implemented.
213-269: LGTM!The button properly computes
clickEnabledasenabled && !isLoading(line 227) and uses it consistently for all color animations and the BasicButton's enabled state. This correctly prevents clicks during loading.
270-308: LGTM!The button content properly switches between the loading indicator and text based on
isLoading, and correctly applies text decoration for underline buttons.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ApplicationScreen.kt (3)
80-80: Lifecycle observer registration couples screen to ViewModel's unconventional pattern.Line 80 manually adds the ViewModel as a lifecycle observer, which is part of the unconventional
DefaultLifecycleObserver.onCreatepattern flagged inApplicationViewModel.kt. If the ViewModel is refactored to use the standardinitblock pattern, this line can be removed.Based on learnings, this pattern should be avoided in favor of standard ViewModel initialization.
210-277: LGTM!The vote content properly handles the empty state and renders each vote list type with appropriate navigation callbacks. The use of separate
items()calls for each vote type is clear and maintainable.
438-448: LGTM!The vote button correctly disables itself when
isVotedis true (line 445) and adapts the button text accordingly. This provides clear feedback to users about voting status.
| RoundedButton( | ||
| onClick = {}, | ||
| colors = when (sex) { | ||
| Sex.MALE -> ButtonDefaults.roundedButtonColors() | ||
| Sex.FEMALE -> ButtonDefaults.roundedRefuseButtonColors() | ||
| Sex.ALL -> throw IllegalArgumentException() | ||
| // TODO: fix | ||
| null -> ButtonDefaults.roundedGrayButtonColors() | ||
| }, | ||
| fillMinSize = false, | ||
| contentPadding = PaddingValues( | ||
| horizontal = PaddingDefaults.Medium, | ||
| vertical = PaddingDefaults.ExtraSmall, | ||
| ), | ||
| ) { | ||
| // TODO: Fix | ||
| Text(text = sex?.text ?: "") | ||
| } |
There was a problem hiding this comment.
Add exception message for Sex.ALL case.
Line 235 throws IllegalArgumentException() without a message. This makes debugging harder.
Apply this diff:
RoundedButton(
onClick = {},
colors = when (sex) {
Sex.MALE -> ButtonDefaults.roundedButtonColors()
Sex.FEMALE -> ButtonDefaults.roundedRefuseButtonColors()
- Sex.ALL -> throw IllegalArgumentException()
+ Sex.ALL -> throw IllegalArgumentException("Sex.ALL is not applicable for individual users")
// TODO: fix
null -> ButtonDefaults.roundedGrayButtonColors()
},Based on static analysis hints.
๐ 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.
| RoundedButton( | |
| onClick = {}, | |
| colors = when (sex) { | |
| Sex.MALE -> ButtonDefaults.roundedButtonColors() | |
| Sex.FEMALE -> ButtonDefaults.roundedRefuseButtonColors() | |
| Sex.ALL -> throw IllegalArgumentException() | |
| // TODO: fix | |
| null -> ButtonDefaults.roundedGrayButtonColors() | |
| }, | |
| fillMinSize = false, | |
| contentPadding = PaddingValues( | |
| horizontal = PaddingDefaults.Medium, | |
| vertical = PaddingDefaults.ExtraSmall, | |
| ), | |
| ) { | |
| // TODO: Fix | |
| Text(text = sex?.text ?: "") | |
| } | |
| RoundedButton( | |
| onClick = {}, | |
| colors = when (sex) { | |
| Sex.MALE -> ButtonDefaults.roundedButtonColors() | |
| Sex.FEMALE -> ButtonDefaults.roundedRefuseButtonColors() | |
| Sex.ALL -> throw IllegalArgumentException("Sex.ALL is not applicable for individual users") | |
| // TODO: fix | |
| null -> ButtonDefaults.roundedGrayButtonColors() | |
| }, | |
| fillMinSize = false, | |
| contentPadding = PaddingValues( | |
| horizontal = PaddingDefaults.Medium, | |
| vertical = PaddingDefaults.ExtraSmall, | |
| ), | |
| ) { | |
| // TODO: Fix | |
| Text(text = sex?.text ?: "") | |
| } |
๐งฐ Tools
๐ช detekt (1.23.8)
[warning] 235-235: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt
around lines 230-247 the when branch for Sex.ALL throws
IllegalArgumentException() with no message; replace that throw with
IllegalArgumentException("Unsupported sex: ALL") (or a similar clear, contextual
message) so the exception includes useful debugging information. Ensure the
message clearly identifies the unexpected enum value and context (e.g., "Invalid
sex value: ALL in MyPageScreen button color selection").
| private val Sex.text: String | ||
| @Composable inline get() = stringResource( | ||
| id = when (this) { | ||
| Sex.MALE -> R.string.sex_male | ||
| Sex.FEMALE -> R.string.sex_female | ||
| Sex.ALL -> throw IllegalStateException() | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Add exception message for Sex.ALL case.
Line 290 throws IllegalStateException() without a message. Provide a descriptive message for debugging.
Apply this diff:
private val Sex.text: String
@Composable inline get() = stringResource(
id = when (this) {
Sex.MALE -> R.string.sex_male
Sex.FEMALE -> R.string.sex_female
- Sex.ALL -> throw IllegalStateException()
+ Sex.ALL -> throw IllegalStateException("Sex.ALL is not applicable for text display")
},
)Based on static analysis hints.
๐งฐ Tools
๐ช detekt (1.23.8)
[warning] 290-290: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
๐ค Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/MyPageScreen.kt
around lines 285 to 292, the Sex.text getter throws IllegalStateException() with
no message for the Sex.ALL branch; update the thrown exception to include a
descriptive message (for example include the unexpected enum value and context
like "Unexpected Sex value in Sex.text: Sex.ALL") so callers and logs show
useful debugging information.
๊ฐ์
์์ ์ฌํญ
์ถ๊ฐ ๋ก ํ ๋ง
Summary by CodeRabbit
New Features
Updates
Chores
โ๏ธ Tip: You can customize this high-level summary in your review settings.