π :: (#818) μλ¦Ό λ° κ³΅μ§ νλ©΄ ꡬν#819
Conversation
π WalkthroughWalkthroughAdds Notice and Notification features (UI, viewmodels, repositories, network endpoints), migrates many network/data sources and repositories to Result-wrapped return types using suspendRunCatching/runCatching, applies system-bar insets across many screens, updates design assets/icons, and adds date/time helpers and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NotificationScreen
participant NotificationViewModel
participant NotificationRepository
participant NetworkNotificationDataSource
participant NotificationApi
User->>NotificationScreen: open notifications
NotificationScreen->>NotificationViewModel: loadNotifications()
NotificationViewModel->>NotificationRepository: fetchNotifications()
NotificationRepository->>NetworkNotificationDataSource: fetchNotifications()
NetworkNotificationDataSource->>NotificationApi: GET /notifications
NotificationApi-->>NetworkNotificationDataSource: Response (list)
NetworkNotificationDataSource-->>NotificationRepository: Result<FetchNotificationsResponse>
NotificationRepository-->>NotificationViewModel: Result<List<Notification>>
NotificationViewModel-->>NotificationScreen: update uiState (notifications/notices)
sequenceDiagram
participant User
participant NotificationScreen
participant NotificationViewModel
participant NotificationRepository
participant NetworkNotificationDataSource
participant NotificationApi
participant LocalResultStore
User->>NotificationScreen: tap notification item (UUID)
NotificationScreen->>NotificationViewModel: onNotificationClick(UUID)
NotificationViewModel->>NotificationRepository: updateNotificationReadStatus(UUID)
NotificationRepository->>NetworkNotificationDataSource: updateNotificationReadStatus(UUID)
NetworkNotificationDataSource->>NotificationApi: PATCH /notifications/{id}/read
NotificationApi-->>NetworkNotificationDataSource: Response<Unit>
NetworkNotificationDataSource-->>NotificationRepository: Result<Unit>
NotificationRepository-->>NotificationViewModel: Result<Unit>
NotificationViewModel->>NotificationViewModel: refresh notifications
NotificationViewModel->>LocalResultStore: store notice_detail_result (UUID)
NotificationScreen->>User: navigate to NoticeDetailRoute (reads LocalResultStore)
Estimated code review effortπ― 4 (Complex) | β±οΈ ~45 minutes Possibly related PRs
Suggested reviewers
π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 warning)
β Passed checks (2 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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 (5)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
40-76: Critical: Secondcollectblock is unreachable β side effects will never be handled.The
snapshotFlow { }.collect { }on lines 41-50 is a terminal operator that suspends indefinitely. TheviewModel.sideEffect.collect { }block starting at line 53 will never execute, meaningVoteSuccess,VoteConflict,VoteFail, andVoteLoadFailside effects are silently ignored.Each
collectshould run in its own coroutine scope:π Proposed fix β launch separate coroutines for each collector
LaunchedEffect(Unit) { - snapshotFlow { - resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch? - }.collect { result -> - if (result != null) { - viewModel.initState(result) - resultStore.removeResult<String?>(resultKey = "vote_result") - } else { - onShowSnackBar(DmsSnackBarType.ERROR, "μ 보λ₯Ό κ°μ Έμ€μ§ λͺ» νμ΄μ") + launch { + snapshotFlow { + resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch? + }.collect { result -> + if (result != null) { + viewModel.initState(result) + resultStore.removeResult<String?>(resultKey = "vote_result") + } else { + onShowSnackBar(DmsSnackBarType.ERROR, "μ 보λ₯Ό κ°μ Έμ€μ§ λͺ» νμ΄μ") + } } } - - - viewModel.sideEffect.collect { - when (it) { - // ... side effect handling + launch { + viewModel.sideEffect.collect { + when (it) { + // ... side effect handling + } } } }app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
119-130: MissingisJwtAvailableinLaunchedEffectdependencies.The
LaunchedEffectusesisJwtAvailable(line 122) to determine the initial screen, but the effect only depends onisOnboardingCompleted. IfisJwtAvailablechanges after the initial composition (e.g., token expiration detected), the navigation won't update accordingly.π Proposed fix
- LaunchedEffect(isOnboardingCompleted) { + LaunchedEffect(isOnboardingCompleted, isJwtAvailable) {data/src/dev/kotlin/team.aliens.dms.android.data/student/repository/StudentRepositoryImpl.kt (2)
41-60: Double-wrapping Result leads to incorrect error propagation.
networkStudentDataSource.signUp()already returnsResult<SignUpResponse>, so wrapping it inrunCatchingcreatesResult<Result<SignUpResponse>>. The current implementation:
- Doesn't propagate network failures to the caller (outer
runCatchingsucceeds even when inner Result fails)- Returns
Result<Unit>success even whenresponseis a failureπ Proposed fix
override suspend fun signUp( schoolVerificationCode: String, schoolVerificationAnswer: String, email: String, emailVerificationCode: String, grade: Int, classRoom: Int, number: Int, accountId: String, password: String, profileImageUrl: String?, - ): Result<Unit> = runCatching { - val response: Result<SignUpResponse> = networkStudentDataSource.signUp( + ): Result<Unit> = + networkStudentDataSource.signUp( request = SignUpRequest( schoolVerificationCode = schoolVerificationCode, schoolVerificationAnswer = schoolVerificationAnswer, email = email, emailVerificationCode = emailVerificationCode, grade = grade, classRoom = classRoom, number = number, accountId = accountId, password = password, profileImageUrl = profileImageUrl, ), - ) - response.onSuccess { + ).onSuccess { jwtProvider.updateTokens(tokens = it.extractTokens()) schoolProvider.updateFeatures(features = it.extractFeatures()) - } - } + }.map { }
108-124: Same double-wrapping issue assignUp.
networkStudentDataSource.resetPassword()returnsResult<Unit>, so the outerrunCatchingcreatesResult<Result<Unit>>. Network failures won't propagate correctly.π Proposed fix
override suspend fun resetPassword( accountId: String, studentName: String, email: String, emailVerificationCode: String, newPassword: String, - ): Result<Unit> = runCatching { - networkStudentDataSource.resetPassword( + ): Result<Unit> = + networkStudentDataSource.resetPassword( ResetPasswordRequest( accountId = accountId, studentName = studentName, email = email, emailVerificationCode = emailVerificationCode, newPassword = newPassword, ), ) - }feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.kt (1)
73-77: Potential logic inconsistency:ProfileImageBadRequestshows success snackbar.The side effect
ProfileImageBadRequest(implying an error) triggers aSUCCESSsnackbar with "μ λ‘λ μ±κ³΅!" (Upload success). This appears contradictoryβeither rename the side effect to reflect success, or change the snackbar type toERRORif this is truly an error case.
π€ Fix all issues with AI agents
In `@app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt`:
- Around line 189-194: The onNavigateNotice callback currently just pushes
NoticeDetailScreenNav without storing the notice ID in resultStore, so
NoticeDetailScreen cannot read the ID; update the HomeRoute callback signature
and its invocations so onNavigateNotice accepts a UUID (change from () -> Unit
to (UUID) -> Unit), then before adding NoticeDetailScreenNav call
resultStore.setResult<UUID?>("notice_result", theId); mirror the pattern used in
onNavigateNotificationDetailClick (which calls
resultStore.setResult<UUID?>("notice_result", it) then
backStack.add(NotificationScreenNav)) and update any callers to pass the
selected notice ID when invoking onNavigateNotice.
In `@core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt`:
- Around line 67-73: The onSuccess block in JwtProviderImpl currently logs
sensitive token data (tokens.refreshToken) and the onFailure block silently
swallows exceptions; remove the Log.d call so no token values are written to
logs and instead keep assigning this@JwtProviderImpl._cachedAccessToken and
_cachedRefreshToken as before, and implement proper failure handling in the
onFailure handler by either logging a non-sensitive error message with the
caught exception (without printing token values) or rethrowing/propagating the
exception so callers can handle it; update references to the onSuccess/onFailure
handlers in JwtProviderImpl accordingly and ensure no other debug logging prints
token contents.
In `@core/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.kt`:
- Around line 9-10: The toDateTimeString() implementation in LocalDateTime
incorrectly classifies noon and doesn't convert to 12βhour clock: change the
AM/PM condition to use hour >= 12 to mark PM for 12:00 and convert the hour
value to 12βhour format using a formula like adjustedHour = if (hour % 12 == 0)
12 else hour % 12 before inserting into the string; update
LocalDateTime.toDateTimeString() to use the adjustedHour for display (optionally
zeroβpad minute if desired) or replace the whole function with a
DateTimeFormatter-based solution for locale-aware formatting.
In
`@data/src/dev/kotlin/team.aliens.dms.android.data/meal/repository/MealRepositoryImpl.kt`:
- Around line 22-27: The current updateMeal implementation uses Result.map so
exceptions thrown inside the transform (like throwing CannotFindMealException
when find returns null or any exception from databaseMealDataSource.saveMeals)
escape instead of being wrapped; change the call to use mapCatching on the
Result returned by networkMealDataSource.fetchMeals(date) and perform the same
transform inside its lambda so any thrown exceptions (including from saveMeals
or the explicit CannotFindMealException) are captured as Result.failure, keeping
the rest of the logic (response.toModel(), saveMeals(meals.toEntity()), and
finding the specific date) unchanged.
In
`@data/src/dev/kotlin/team.aliens.dms.android.data/student/repository/StudentRepository.kt`:
- Around line 7-8: StudentRepository currently exposes network-layer types
(ExamineStudentNumberResponse, FindIdResponse) in its abstract interface,
leaking implementation details; change the abstract methods in StudentRepository
back to return the domain models (e.g., StudentName, HashedEmail or other domain
DTOs) and perform mapping from ExamineStudentNumberResponse/FindIdResponse to
those domain types inside the concrete implementation (the class implementing
StudentRepository), ensuring only domain types appear in the repository API.
In
`@data/src/dev/kotlin/team.aliens.dms.android.data/student/repository/StudentRepositoryImpl.kt`:
- Around line 140-143: The caches are being cleared before the network call, so
move jwtProvider.clearCaches() and schoolProvider.clearCaches() to execute only
after networkStudentDataSource.withdraw() succeeds: call
networkStudentDataSource.withdraw(), check its Result (or catch exceptions) and
only when it indicates success perform jwtProvider.clearCaches() and
schoolProvider.clearCaches(), then return the successful Result; if the network
call fails, return the failure without clearing caches.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/notice/ui/NoticeDetailScreen.kt`:
- Around line 46-57: The code is removing the wrong result key and treating the
initial null emission as an error; change the remove call to
removeResult(resultKey = "notice_result") to match the read from
resultStateMap["notice_result"], and prevent the immediate null-triggered
snackbar by ignoring initial null emissions β for example, use snapshotFlow {
resultStore.resultStateMap["notice_result"]?.value as? UUID? }.filterNotNull()
(or check for presence of the key before showing the snackbar / use a first-time
flag or timeout) inside LaunchedEffect so only real results trigger
viewModel.getNotificationDetail(...) and removeResult(...), and only unexpected
nulls after a valid state produce onShowSnackBar.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/notice/viewmodel/NoticeDetailViewModel.kt`:
- Around line 21-32: The code wraps a Result-returning call in runCatching and
then swallows failures, so remove the redundant runCatching around
noticeRepository.fetchNoticeDetails(noticeId) and handle the Result directly
(e.g., call fetchNoticeDetails(...).onSuccess { notice -> build
NotificationDetailUi and call setState { it.copy(notice = noticeUi) }
}.onFailure { error -> update the ViewModel state to reflect the failure or
populate an error field so the UI can show feedback, or emit an
event/side-effect for an error dialog). Keep references to
NoticeDetailViewModel, fetchNoticeDetails, NotificationDetailUi, and setState
when making the change.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/NotificationScreen.kt`:
- Around line 123-139: The pager content lambda is using pagerState.currentPage
instead of the lambda's page index, causing offscreen pages to render wrong
content; change the HorizontalPager content lambda to accept the page index
parameter (e.g., use "page" from the lambda) and switch the conditional to "if
(page == 0)" so NotificationItems is rendered for page 0 and NoticeItems for
other pages; update the lambda in HorizontalPager that currently references
pagerState.currentPage to use the provided page parameter instead.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/viewmodel/NotificationViewModel.kt`:
- Around line 52-63: The success/failure handlers in
NotificationViewModel.updateNotificationReadStatus are inverted:
notificationRepository.updateNotificationReadStatus(...).fold currently calls
fetchNotifications() on failure and sends
NotificationSideEffect.SuccessUpdateNotification on failure; swap the handlers
so onSuccess calls sendEffect(NotificationSideEffect.SuccessUpdateNotification)
and then fetchNotifications() (or at least send the success effect), and
onFailure sends an appropriate failure effect (e.g.,
NotificationSideEffect.FailureUpdateNotification) and/or logs the error; update
the fold handlers accordingly and ensure sendEffect and NotificationSideEffect
constants are used consistently.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt`:
- Around line 105-107: The onFailure lambda for fetchPresignedUrl in
SelectProfileViewModel is empty and swallows errors; update the onFailure
handler inside the fetchPresignedUrl call to log the Throwable (e.g., via Timber
or logger), update the ViewModel state to reflect the error (e.g., set an error
LiveData/StateFlow or call a showError method), and surface a user-visible
message (Toast/Snackbar or UI error state) so the UI can stop any loading
indicator and inform the user; ensure the handler accepts the Throwable
parameter and use the existing error reporting/logging utilities in
SelectProfileViewModel.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt`:
- Around line 140-149: The Find ID and Reset Password handlers are not wired:
update the SignInScreen signature to accept navigateToFindId and
navigateToFindPassword, propagate those two callbacks into the SignIn composable
call, and pass them into UserInformationInputs instead of the empty lambdas;
specifically modify SignInScreen, the SignIn(...) invocation, and the
UserInformationInputs(...) parameters so onFindId uses navigateToFindId and
onResetPassword uses navigateToFindPassword.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/auth/datasource/NetworkAuthDataSourceImpl.kt`:
- Around line 12-30: The suspend overrides (signIn, sendEmailVerificationCode,
checkEmailVerificationCode, checkIdExists) use Kotlin's runCatching which
swallows CancellationException and breaks coroutine cancellation; replace
runCatching with a cancellation-aware wrapper (e.g., a runSuspendCatching that
rethrows CancellationException and wraps other exceptions into Result.failure)
and call that wrapper from each of these methods (or explicitly catch exceptions
and rethrow CancellationException) so cancellation propagates correctly while
still returning Result<T>.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/notification/datasource/NetworkNotificationDataSource.kt`:
- Line 33: The method signature updateNotificationReadStatus currently returns a
nullable Result<Unit>? which introduces an invalid third state; change its
return type to non-nullable Result<Unit>, and inside the implementation convert
any nullable API Response<Unit>? into a concrete Result by mapping a null
response to Result.success(Unit) (or a meaningful Result.failure when
appropriate) instead of returning null; ensure the runCatching { ... } block
(and any usage of the API call within updateNotificationReadStatus) always
returns a non-null Result and remove cascading nullable handling in callers.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/remains/datasource/NetworkRemainsDataSourceImpl.kt`:
- Around line 12-22: The current suspend functions updateRemainsOption,
fetchAppliedRemainsOption, fetchRemainsApplicationTime and fetchRemainsOptions
use runCatching which will swallow CancellationException and break structured
concurrency; replace runCatching with a coroutine-safe wrapper (e.g., a
runSuspendCatching that rethrows CancellationException and returns
Result.success/failure) and call that wrapper inside each of those methods
instead of runCatching; extract the runSuspendCatching utility to a shared util
file used across data sources (auth, user, student, notification, meal, remains)
and update all usages to avoid swallowing cancellations.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSourceImpl.kt`:
- Around line 20-24: The current implementation of
NetworkVotingDataSourceImpl.fetchCreateVotingItem double-wraps the API result by
calling votingApiService.fetchCreateVotingItem(...) inside runCatching and then
getOrThrow(), which rewraps exceptions; instead, return the API Result directly
and flatten nulls like fetchDeleteVotingItem does β call
votingApiService.fetchCreateVotingItem(votingTopicId, selectedId)?.map { Unit }
?: Result.success(Unit) so you don't re-wrap errors and maintain consistency
with fetchDeleteVotingItem.
π‘ Minor comments (15)
feature/src/dev/kotlin/team/aliens/dms/android/feature/meal/ui/MealScreen.kt-153-154 (1)
153-154: Redundant padding:navigationBarsPadding()is already included insystemBarsPadding().
systemBarsPadding()applies insets for both status bar and navigation bar. AddingnavigationBarsPadding()afterward doubles the bottom padding.Suggested fix
modifier = Modifier .fillMaxSize() .background(DmsTheme.colorScheme.background) .systemBarsPadding() - .navigationBarsPadding() .verticalScroll(rememberScrollState()),core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt-44-46 (1)
44-46: Remove debug logging before merge.The
Log.d("TEST ...")statement is a development artifact that should be removed before merging todevelop.if (_cachedRefreshToken!!.isExpired()) { - Log.d("TEST _cachedRefreshToken!!.isExpired()", _cachedRefreshToken!!.isExpired().toString()) throw CannotUseRefreshTokenException() }core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt-81-85 (1)
81-85: Remove debug logging statement.The
Log.d("TEST", ...)is a debug artifact that should be removed. If error logging is needed for production, use a more descriptive tag and appropriate log level.Proposed fix
CoroutineScope(Dispatchers.Default).launch { runCatching { jwtDataStoreDataSource.storeTokens(tokens = tokens) - }.onFailure { Log.d("TEST", it.message.toString()) } + }.onFailure { exception -> + // Use proper logging if needed: + // Log.e("JwtProvider", "Failed to store tokens", exception) + } }network/src/dev/kotlin/team.aliens.dms.android.network/notification/datasource/NetworkNotificationDataSourceImpl.kt-41-42 (1)
41-42: Remove nullable return type fromupdateNotificationReadStatusin both interface and implementation.
runCatchingalways returns aResultinstance, nevernull. The nullable return typeResult<Unit>?is inconsistent with all other methods in this class and the interface, which correctly use non-nullableResult<T>.Suggested fix
Interface (NetworkNotificationDataSource.kt, line 33):
- abstract suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit>? + abstract suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit>Implementation (NetworkNotificationDataSourceImpl.kt, lines 41-42):
- override suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit>? = + override suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit> = runCatching { notificationApiService.updateNotificationReadStatus(notification) }feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt-119-122 (1)
119-122: Redundant navigation bar padding.
systemBarsPadding()already applies padding for both status bar and navigation bar. AddingnavigationBarsPadding()after it results in double padding for the navigation bar area.π§ Suggested fix
modifier = Modifier .fillMaxSize() .imePadding() .background(DmsTheme.colorScheme.surfaceTint) .systemBarsPadding() - .navigationBarsPadding() .pointerInput(Unit) { // TODO KMP ꡬνfeature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt-99-105 (1)
99-105:rememberkey doesn't capture all dependencies.The remembered computation accesses
state.plusPointListandstate.minusPointList, but the key only includesstate.allPointList. If those lists update independently, the UI may show stale data.π οΈ Suggested fix
- val pointHistoryList = remember(page, state.allPointList) { + val pointHistoryList = remember(page, state.allPointList, state.plusPointList, state.minusPointList) { when (tabData[page]) { PointTab.All -> state.allPointList PointTab.BONUS -> state.plusPointList PointTab.Minus -> state.minusPointList } }Alternatively, you could remove the
rememberentirely since accessing lists from state is already cheap:- val pointHistoryList = remember(page, state.allPointList, state.plusPointList, state.minusPointList) { - when (tabData[page]) { - PointTab.All -> state.allPointList - PointTab.BONUS -> state.plusPointList - PointTab.Minus -> state.minusPointList - } - } + val pointHistoryList = when (tabData[page]) { + PointTab.All -> state.allPointList + PointTab.BONUS -> state.plusPointList + PointTab.Minus -> state.minusPointList + }feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt-68-71 (1)
68-71: UsingpointType.ordinalfor initial page is fragile.The enum order (
ALL,BONUS,MINUS) currently matches thetabDatalist order, but this coupling is implicit and brittle. If either thePointTypeenum order or thetabDatalist order changes without the other, the indices will silently mismatch. Use an explicit mapping instead:val pagerState = rememberPagerState( pageCount = { tabData.size }, - initialPage = pointType.ordinal, + initialPage = tabData.indexOfFirst { it.pointType == pointType }.coerceAtLeast(0), )network/src/dev/kotlin/team.aliens.dms.android.network/notification/datasource/NetworkNotificationDataSource.kt-3-3 (1)
3-3: Unused import:retrofit2.http.PATCHThe
@PATCHannotation is a Retrofit HTTP method annotation used in API service interfaces, not in data source classes. This import appears unused in this abstract class and should be removed.π§Ή Proposed fix
-import retrofit2.http.PATCHfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt-75-77 (1)
75-77: Redundant padding:systemBarsPadding()already includes navigation bar insets.
systemBarsPadding()applies padding for all system bars (status bar + navigation bar). AddingnavigationBarsPadding()afterward results in double bottom padding. Remove one of these calls.Suggested fix
.background(DmsTheme.colorScheme.background) - .systemBarsPadding() - .navigationBarsPadding(), + .systemBarsPadding(),feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/viewmodel/NotificationViewModel.kt-54-61 (1)
54-61: Handle null result fromupdateNotificationReadStatusexplicitly.The method returns
Result<Unit>?(nullable). Using?.fold()means if the result isnull, neither success nor failure handlers execute, leaving the operation untracked. Add explicit null handling or verify that null can never be returned and update the return type accordingly.feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt-100-100 (1)
100-100: Inconsistent nullable handling onfileUrl.Line 96 accesses
fileUrl.fileUrl(non-nullable), but line 100 usesfileUrl?.fileUrl.toString()with a safe call. IffileUrlcould be null, line 96 would throw NPE. If it can't be null (which seems correct given theonSuccesscontext), the?.is misleading and produces the string"null"if the innerfileUrlproperty is null.π Proposed fix
- sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl?.fileUrl.toString())) + sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl.fileUrl))data/src/dev/kotlin/team.aliens.dms.android.data/notification/repository/NotificationRepository.kt-38-39 (1)
38-39: Inconsistent return type:Result<Unit>?vsResult<Unit>.All other methods in this repository return non-nullable
Result<T>, butupdateNotificationReadStatusreturnsResult<Unit>?. This creates an inconsistencyβcallers must handle null-checking (?.fold()) even thoughrunCatchingalways returns a non-nullResult<Unit>.Change to
Result<Unit>for consistency:β»οΈ Suggested fix
- abstract suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit>? + abstract suspend fun updateNotificationReadStatus(notification: UUID): Result<Unit>feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.kt-70-72 (1)
70-72: Redundant padding modifiers.
systemBarsPadding()already applies insets for both status bar and navigation bar. AddingnavigationBarsPadding()after it will cause double padding at the bottom of the screen.π Proposed fix
modifier = Modifier .fillMaxSize() .background(DmsTheme.colorScheme.background) - .systemBarsPadding() - .navigationBarsPadding(), + .systemBarsPadding(),feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/NotificationScreen.kt-203-229 (1)
203-229: Modifier parameter incorrectly reused on child elements.The
modifierparameter received byNotificationItemis being applied to child elements (Column, Row, Icon, Text). This is incorrect because the parent's modifier (which may include sizing, padding, or weight constraints from the caller) should not propagate to children. Each child should useModifieror a dedicated modifier.The same issue exists in
NoticeItem(lines 263-289).π Proposed fix for NotificationItem
Column( - modifier = modifier.startPadding(12.dp), + modifier = Modifier.startPadding(12.dp), ) { Text( text = notification.title, style = DmsTheme.typography.bodyM, ) Row( - modifier = modifier.topPadding(6.dp) + modifier = Modifier.topPadding(6.dp) ) { if (!notification.isRead) { Icon( - modifier = modifier.size(4.dp), + modifier = Modifier.size(4.dp), imageVector = Icons.Filled.Circle, contentDescription = null, tint = DmsTheme.colorScheme.primaryContainer, ) } Text( - modifier = modifier + modifier = Modifier .startPadding(4.dp), text = notification.content, style = DmsTheme.typography.labelM, ) } } - Spacer(modifier = modifier.weight(1f)) + Spacer(modifier = Modifier.weight(1f))feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.kt-86-88 (1)
86-88: Redundant navigation bar padding causes double inset.
systemBarsPadding()already applies padding for both the status bar and navigation bar. Adding.navigationBarsPadding()afterwards results in the navigation bar inset being applied twice, causing extra unwanted space at the bottom.π§ Proposed fix
modifier = Modifier .fillMaxSize() .background(DmsTheme.colorScheme.background) - .systemBarsPadding() - .navigationBarsPadding(), + .systemBarsPadding(),
π§Ή Nitpick comments (28)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/foundation/DmsIcon.kt (1)
33-33: LGTM!The icon mappings correctly distinguish between the notification bell icon and the notice megaphone icon, supporting the new notification/notice screens.
Minor nit: The
Noticeproperty (line 56) is added at the end of the object rather than in alphabetical order afterNotification(line 33). Consider placing it alphabetically for consistency if the team follows that convention.Also applies to: 56-56
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/AdjustProfileScreen.kt (1)
118-120: Remove emptyonSizeChangedcallback.This callback is invoked on size changes but does nothing. Either implement the intended behavior or remove the dead code.
β»οΈ Suggested fix
Box( modifier = Modifier .horizontalPadding(16.dp) .verticalPadding(24.dp) .clip(RoundedCornerShape(32.dp)) - .onSizeChanged { - - } ) {feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/component/SettingRotateContent.kt (1)
40-47: LGTM! Animation changes are well-structured.The switch from 360Β° to 180Β° rotation is semantically correct for a toggle component, and the explicit
tweenconfiguration provides better control over the animation timing.Minor nitpick:
delayMillis = 0is the default value fortween()and could be omitted for brevity.Optional: Remove redundant default parameter
val rotationAngle by animateFloatAsState( targetValue = if (rotated) 180f else 0f, animationSpec = tween( durationMillis = 500, - delayMillis = 0, easing = LinearEasing ), )network/src/dev/kotlin/team.aliens.dms.android.network/notification/apiservice/NotificationApiService.kt (1)
56-59: NullableResponse<Unit>?return type is likely unnecessary.Retrofit suspend functions throw exceptions on network failures or HTTP errors rather than returning null. The nullable return type here is unusual and inconsistent with the other methods in this interface. This may lead to unnecessary null-checks in the calling code.
Consider changing to
Response<Unit>(non-nullable) if you need response metadata, or simply omit the return type entirely (implicitUnit) if you only care about success/failure:Suggested fix
`@PATCH`("/notifications/{notification-of-user-id}/read") suspend fun updateNotificationReadStatus( `@Path`("notification-of-user-id") notificationOfUserId: UUID, - ): Response<Unit>? + ): Response<Unit>network/src/dev/kotlin/team.aliens.dms.android.network/user/apiservice/UserApiService.kt (1)
3-3: Unused import after refactoring.The
retrofit2.Responseimport is no longer used sinceeditPasswordnow returnsUnitdirectly instead ofResponse<Unit>?.π§Ή Proposed fix
package team.aliens.dms.android.network.user.apiservice -import retrofit2.Response import retrofit2.http.Body import retrofit2.http.GET import retrofit2.http.PATCHnetwork/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt (1)
3-3: Unused import remaining from previous error handling.
PasswordMismatchExceptionis imported but no longer used after removing thehandleNetworkRequest-based implementation.π§Ή Proposed fix
package team.aliens.dms.android.network.user.datasource import team.aliens.dms.android.network.user.apiservice.UserApiService -import team.aliens.dms.android.network.user.exception.PasswordMismatchException import team.aliens.dms.android.network.user.model.EditPasswordRequest import javax.inject.Injectcore/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt (2)
14-14: Unused import.The
nowimport is not used anywhere in the visible code. Remove it to keep imports clean.-import team.aliens.dms.android.shared.date.util.now
3-3: Consider removingLogimport if all debug statements are removed.Once the debug
Log.dcalls are removed as suggested above, this import will become unused and should also be removed.feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt (2)
79-85: Redundant SDK version check.The
notificationPermissionGrantedhelper already returnstruefor SDK versions below TIRAMISU, so if the condition at line 80 is true, we're guaranteedBuild.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU. The inner check is unnecessary.β»οΈ Suggested simplification
LaunchedEffect(Unit) { if (!notificationPermissionGranted(context)) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS) - } + requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS) } }
106-106: UnnecessaryExperimentalLayoutApiopt-in.The
imePadding(),systemBarsPadding(), andnavigationBarsPadding()APIs used here are stable and don't require this opt-in. Consider removing it unless experimental APIs are intended to be added.feature/src/dev/kotlin/team/aliens/dms/android/feature/point/viewmodel/PointHistoryViewModel.kt (2)
13-15: Consider makingpointRepositoryprivate.The repository is an injected dependency and should not be exposed publicly. This is a common convention for ViewModel dependencies.
β»οΈ Suggested fix
internal class PointHistoryViewModel `@Inject` constructor( - val pointRepository: PointRepository + private val pointRepository: PointRepository ) : BaseStateViewModel<PointHistoryState, PointHistorySideEffect>(PointHistoryState()) {
43-45:initTab()andinitialTabappear to be dead code.The screen now uses
pointType.ordinalfor the initial page (line 70 in PointHistoryScreen.kt), makinginitTab()andstate.initialTabunused. Consider removing this dead code.β»οΈ Suggested cleanup
init { getPoints() - initTab() } - - private fun initTab() { - setState { it.copy(initialTab = 0) } - } } internal data class PointHistoryState( val allPointList: List<Point> = emptyList(), val plusPointList: List<Point> = emptyList(), val minusPointList: List<Point> = emptyList(), - val initialTab: Int = 0, )Also applies to: 52-52
feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt (1)
131-133: Inconsistent naming convention inPointTabsealed class.
BONUSuses SCREAMING_CASE whileAllandMinususe PascalCase. Consider using a consistent naming convention for all data objects.β»οΈ Suggested fix for consistency
internal sealed class PointTab( val title: String, val pointType: PointType, ) { - data object All : PointTab(title = "μ 체", pointType = PointType.ALL) - data object BONUS : PointTab(title = "μμ ", pointType = PointType.BONUS) - data object Minus : PointTab(title = "λ²μ ", pointType = PointType.MINUS) + data object All : PointTab(title = "μ 체", pointType = PointType.ALL) + data object Bonus : PointTab(title = "μμ ", pointType = PointType.BONUS) + data object Minus : PointTab(title = "λ²μ ", pointType = PointType.MINUS) }This would require updating references at lines 65 and 102 back to
PointTab.Bonus.network/src/dev/kotlin/team.aliens.dms.android.network/student/apiservice/StudentApiService.kt (1)
3-3: Unused import:retrofit2.ResponseAfter removing the
Response<Unit>?return types fromresetPasswordandeditProfile, theResponseimport appears to be unused in this file.π§Ή Suggested fix
-import retrofit2.Responsenetwork/src/dev/kotlin/team.aliens.dms.android.network/student/datasource/NetworkStudentDataSource.kt (1)
32-32: Minor formatting issue: missing space beforeResult.π§Ή Suggested fix
- abstract suspend fun resetPassword(request: ResetPasswordRequest):Result<Unit> + abstract suspend fun resetPassword(request: ResetPasswordRequest): Result<Unit>shared/date/src/main/java/team/aliens/dms/android/shared/date/Extensions.kt (2)
43-58: Consider handling edge case whenthisis afternow.If
thisis in the future relative tonow,ChronoUnit.*.between()returns negative values, resulting in strings like "-5μ΄ μ " (Korean: "-5 seconds ago"). Consider adding a guard or documenting the expected invariant.π‘ Suggested defensive guard
fun LocalDateTime.toElapsedText(now: LocalDateTime): String { val seconds = ChronoUnit.SECONDS.between(this, now) + if (seconds < 0) return "λ°©κΈ μ " // or throw, or handle as needed if (seconds < 60) return "${seconds}μ΄ μ "
53-57: Minor inconsistency between day and month thresholds.Using
days < 30as the cutoff before switching to months may cause edge cases. For example, on day 30 the code switches toChronoUnit.MONTHS.between(), which uses calendar months (varying 28-31 days). A date 30 days ago might show as "0λ¬ μ " if it falls within the same calendar month.Consider using a slightly higher day threshold (e.g., 31) or using
months >= 1as the switch condition for consistency.data/src/dev/kotlin/team.aliens.dms.android.data/file/repository/FileRepository.kt (1)
3-3: Unused import.
FileUriExposedExceptiondoes not appear to be used in this abstract class. Consider removing it if it's not needed.π§Ή Suggested fix
package team.aliens.dms.android.data.file.repository -import android.os.FileUriExposedException import team.aliens.dms.android.data.file.model.FileUrl import team.aliens.dms.android.data.file.model.PresignedFileUrl import java.io.Filefeature/src/dev/kotlin/team/aliens/dms/android/feature/notification/viewmodel/NotificationViewModel.kt (1)
79-83: Unused fieldisRecentin state.The
isRecentfield is declared with a default value but is never updated anywhere in the ViewModel. Consider removing it if unused, or implementing the intended functionality.data/src/dev/kotlin/team.aliens.dms.android.data/student/repository/StudentRepositoryImpl.kt (2)
74-81: Remove commented-out code.The TODO comment and commented block should be addressed or removed. Leaving commented code in the codebase reduces readability and creates maintenance burden.
98-106: Remove commented-out code.Same as aboveβthis commented block should be removed to keep the codebase clean.
data/src/dev/kotlin/team.aliens.dms.android.data/notification/model/Notification.kt (1)
26-26:PointType.valueOf()may throw on unexpected input from the backend.The mapping uses
PointType.valueOf(this.pointDetailTopic ?: "ALL"). While the null case is handled with the elvis operator,valueOf()will throwIllegalArgumentExceptionif the backend sends a non-null string that doesn't match any enum constant (e.g., a typo, unexpected value, or new enum value added to the backend before the app is updated).Consider using a safer approach:
Suggested safer mapping
pointDetailTopic = runCatching { PointType.valueOf(this.pointDetailTopic ?: "ALL") } .getOrDefault(PointType.ALL),Or, if PointType uses Kotlin 1.9+
entries:pointDetailTopic = PointType.entries.find { it.name == this.pointDetailTopic } ?: PointType.ALL,feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/viewmodel/SignInViewModel.kt (1)
73-79: Fragile error code extraction from exception message.Parsing the error code via
it.message?.drop(5)?.trim()assumes a rigid message format (e.g.,"HTTP 400 ..."). If the backend or network layer changes the message format, this will silently fall through to the generic fallback. Consider one of these alternatives:
- Define a custom exception type with an explicit
errorCodeproperty- Use a sealed class hierarchy for authentication errors
- Parse the message with a regex that validates the expected format
The TODO comment indicates awarenessβflagging this for tracking.
β»οΈ Example: Custom exception with explicit error code
// In a shared/errors package sealed class AuthException(message: String, val code: Int) : Exception(message) { class BadRequest(message: String) : AuthException(message, 400) class InvalidCredentials(message: String) : AuthException(message, 422) class NotFound(message: String) : AuthException(message, 404) class ServerError(message: String) : AuthException(message, 500) } // In SignInViewModel val errorMessage = when ((it as? AuthException)?.code) { 400 -> "μλͺ»λ λΉλ°λ²νΈ νμμ΄μμ" 422 -> "λΉλ°λ²νΈκ° μΌμΉνμ§ μμμ" 404 -> "μ‘΄μ¬νμ§ μλ νμμ λλ€" 500 -> "μλ²μ λ¬Έμ κ° λ°μνμ΄μ" else -> "μΌμμ μΈ μ€λ₯λ‘ λ‘κ·ΈμΈμ ν μ μμ΄μ" }data/src/dev/kotlin/team.aliens.dms.android.data/auth/repository/AuthRepository.kt (1)
5-6: Network layer models leaking into repository interface.The repository interface imports
SignInResponseandCheckIdExistsResponsedirectly from the network layer (team.aliens.dms.android.network.auth.model). This creates coupling between the data layer and network layer, breaking clean architecture boundaries.Compare with
HashedEmailon line 4, which correctly uses a data layer model. Consider defining data layer equivalents and mapping in the implementation.β»οΈ Suggested approach
// In data/auth/model/ data class SignInResult( val accessToken: String, val refreshToken: String, // other relevant fields ) data class IdExistsResult( val schoolId: UUID, val email: String, ) // In AuthRepository.kt abstract suspend fun signIn(...): Result<SignInResult> abstract suspend fun checkIdExists(accountId: String): Result<IdExistsResult> // Mapping happens in AuthRepositoryImplAlso applies to: 14-14, 27-27
feature/src/dev/kotlin/team/aliens/dms/android/feature/notice/viewmodel/NoticeDetailViewModel.kt (1)
37-55: Naming inconsistency between Notice and Notification.The ViewModel is named
NoticeDetailViewModel, but the state and UI classes useNotificationprefix (NotificationDetailUi,NotificationDetailState,NotificationDetailSideEffect). Since this screen is for notices (곡μ§), consider renaming for consistency:
NotificationDetailUiβNoticeDetailUiNotificationDetailStateβNoticeDetailStateNotificationDetailSideEffectβNoticeDetailSideEffectfeature/src/dev/kotlin/team/aliens/dms/android/feature/notice/ui/NoticeDetailScreen.kt (1)
91-97: Consider using vertical scroll instead of horizontal scroll for text content.Applying
horizontalScrollto a Column containing text content is unusual. Users typically expect to scroll vertically through long text. This will cause the content to scroll horizontally, which may truncate text or create unexpected UX.β»οΈ Suggested change
Column( modifier = modifier .fillMaxWidth() .horizontalPadding(10.dp) .background(color = DmsTheme.colorScheme.surfaceTint, shape = RoundedCornerShape(32.dp)) .padding(vertical = 24.dp, horizontal = 32.dp) - .horizontalScroll(rememberScrollState()), + .verticalScroll(rememberScrollState()), ) {feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/NotificationScreen.kt (1)
186-302: Consider extracting shared layout logic between NotificationItem and NoticeItem.Both composables share nearly identical structure with only minor differences (icon, click handler, slight padding). Consider extracting a common base composable to reduce duplication.
data/src/dev/kotlin/team.aliens.dms.android.data/auth/repository/AuthRepositoryImpl.kt (1)
62-68: Remove commented-out code and address TODO.Lines 64-67 contain commented-out code and a TODO comment. If the usecase implementation is planned, consider creating an issue to track it. Otherwise, remove the dead code to keep the codebase clean.
β»οΈ Proposed cleanup
override suspend fun checkIdExists(accountId: String): Result<CheckIdExistsResponse> = networkAuthDataSource.checkIdExists(accountId = accountId) -// TODO :: usecase νμ© -// networkAuthDataSource.checkIdExists( -// accountId = accountId, -// ).email -Would you like me to open an issue to track the usecase implementation mentioned in the TODO?
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt
Show resolved
Hide resolved
core/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.kt
Outdated
Show resolved
Hide resolved
data/src/dev/kotlin/team.aliens.dms.android.data/meal/repository/MealRepositoryImpl.kt
Show resolved
Hide resolved
data/src/dev/kotlin/team.aliens.dms.android.data/student/repository/StudentRepository.kt
Show resolved
Hide resolved
...rc/dev/kotlin/team.aliens.dms.android.network/point/datasource/NetworkPointDataSourceImpl.kt
Outdated
Show resolved
Hide resolved
...ev/kotlin/team.aliens.dms.android.network/remains/datasource/NetworkRemainsDataSourceImpl.kt
Outdated
Show resolved
Hide resolved
| override suspend fun fetchSchools(): FetchSchoolsResponse = | ||
| handleNetworkRequest { schoolApiService.fetchSchools() } | ||
| schoolApiService.fetchSchools() | ||
|
|
||
| override suspend fun fetchSchoolVerificationQuestion(schoolId: UUID): FetchSchoolVerificationQuestionResponse = | ||
| handleNetworkRequest { schoolApiService.fetchSchoolVerificationQuestion(schoolId) } | ||
| schoolApiService.fetchSchoolVerificationQuestion(schoolId) | ||
|
|
||
| override suspend fun examineSchoolVerificationQuestion( | ||
| schoolId: UUID, | ||
| answer: String, | ||
| ) = handleNetworkRequest { schoolApiService.examineSchoolVerificationQuestion(schoolId, answer) } | ||
| ) = schoolApiService.examineSchoolVerificationQuestion(schoolId, answer) | ||
|
|
||
| override suspend fun examineSchoolVerificationCode(code: String): ExamineSchoolVerificationCodeResponse = | ||
| handleNetworkRequest { schoolApiService.examineSchoolVerificationCode(code) } | ||
| schoolApiService.examineSchoolVerificationCode(code) |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# First, let's look at the actual file to confirm the current state
find . -type f -name "NetworkSchoolDataSourceImpl.kt" | head -5Repository: team-aliens/DMS-Android
Length of output: 279
π Script executed:
# Find repository implementations that use NetworkSchoolDataSource
rg -l "NetworkSchoolDataSource" --type=kotlin | grep -i repositoryRepository: team-aliens/DMS-Android
Length of output: 158
π Script executed:
# Search for where NetworkSchoolDataSource is injected/used
rg "NetworkSchoolDataSource" --type=kotlin -B 2 -A 5 | head -100Repository: team-aliens/DMS-Android
Length of output: 10193
π Script executed:
cat -n data/src/prod/kotlin/team/aliens/dms/android/data/school/repository/SchoolRepositoryImpl.ktRepository: team-aliens/DMS-Android
Length of output: 1622
π Script executed:
# Search for handleNetworkRequest implementation/definition
rg -B 2 -A 10 "fun handleNetworkRequest|protected fun handleNetworkRequest" --type=kotlinRepository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
# Check if SchoolRepositoryImpl wraps calls with Result or error handling
rg -B 5 -A 15 "class SchoolRepositoryImpl" --type=kotlin data/Repository: team-aliens/DMS-Android
Length of output: 2882
π Script executed:
# Find the definition of handleNetworkRequest in the base class
rg -B 5 -A 15 "handleNetworkRequest" --type=kotlin network/src/prod/Repository: team-aliens/DMS-Android
Length of output: 50379
π Script executed:
# Check if there are any Result-based error handlers elsewhere
rg "Result<" --type=kotlin data/src/prod/kotlin/team/aliens/dms/android/data/school/ | head -20Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
# Find the handleNetworkRequest implementation
find . -name "*.kt" -type f | xargs grep -l "fun handleNetworkRequest" | head -5Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
# Look for the core network util module
fd -t f "handleNetworkRequest" --type=kotlin core/Repository: team-aliens/DMS-Android
Length of output: 240
π Script executed:
# Find handleNetworkRequest in the core module
find . -path "*/core/network*" -name "*.kt" | xargs grep -l "fun handleNetworkRequest" 2>/dev/null | head -5Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
# Search for handleNetworkRequest definition anywhere
rg "fun handleNetworkRequest" --type=kotlin -B 2 -A 20Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
# Search for handleNetworkRequest in core network util
find . -path "*core*network*util*" -name "*.kt" | head -20Repository: team-aliens/DMS-Android
Length of output: 606
π Script executed:
# Try a broader search without type restriction
rg "handleNetworkRequest" --type-list | head -20Repository: team-aliens/DMS-Android
Length of output: 456
π Script executed:
# Search more broadly for where handleNetworkRequest is defined
rg -i "handleNetworkRequest" -A 5 | grep -A 5 "fun handleNetworkRequest" | head -50Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
cat -n ./core/network/src/main/java/team/aliens/dms/android/core/network/util/NetworkRequestHandler.ktRepository: team-aliens/DMS-Android
Length of output: 2811
π Script executed:
# Also check the dev version to see if it still uses handleNetworkRequest
cat -n ./network/src/dev/kotlin/team.aliens.dms.android.network/school/datasource/NetworkSchoolDataSourceImpl.ktRepository: team-aliens/DMS-Android
Length of output: 1474
Missing error handling after handleNetworkRequest removal.
The removal of the handleNetworkRequest wrapper from the dev version has eliminated centralized error handling. The handleNetworkRequest function (in core/network/util/NetworkRequestHandler.kt) catches HttpException and other network errors, converting them to custom exceptions. With this wrapper removed, network exceptions will propagate uncaught to callers.
The repository layer (SchoolRepositoryImpl.kt) provides no error handlingβit directly calls the datasource methods and unwraps responses without any Result wrapper or try-catch blocks. This leaves the dev build vulnerable to unhandled network exceptions.
Add error handling at the repository level using Result wrapper or try-catch blocks to safely handle network failures from the datasource.
| override suspend fun fetchStudyRoomApplicationTime(): FetchStudyRoomApplicationTimeResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchStudyRoomApplicationTime() } | ||
| studyRoomApiService.fetchStudyRoomApplicationTime() | ||
|
|
||
| override suspend fun applySeat( | ||
| seatId: UUID, | ||
| timeslot: UUID, | ||
| ) { | ||
| handleNetworkRequest { studyRoomApiService.applySeat(seatId, timeslot) } | ||
| studyRoomApiService.applySeat(seatId, timeslot) | ||
| } | ||
|
|
||
| override suspend fun cancelSeat( | ||
| seatId: UUID, | ||
| timeslot: UUID, | ||
| ) { | ||
| handleNetworkRequest { studyRoomApiService.cancelSeat(seatId, timeslot) } | ||
| studyRoomApiService.cancelSeat(seatId, timeslot) | ||
| } | ||
|
|
||
| override suspend fun fetchStudyRooms(timeslot: UUID): FetchStudyRoomsResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchStudyRooms(timeslot) } | ||
| studyRoomApiService.fetchStudyRooms(timeslot) | ||
|
|
||
| override suspend fun fetchStudyRoomDetails( | ||
| studyRoomId: UUID, | ||
| timeslot: UUID, | ||
| ): FetchStudyRoomDetailsResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchStudyRoomDetails(studyRoomId, timeslot) } | ||
| studyRoomApiService.fetchStudyRoomDetails(studyRoomId, timeslot) | ||
|
|
||
| override suspend fun fetchAppliedStudyRoom(): FetchAppliedStudyRoomResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchAppliedStudyRoom() } | ||
| studyRoomApiService.fetchAppliedStudyRoom() | ||
|
|
||
| override suspend fun fetchSeatTypes(studyRoomId: UUID): FetchSeatTypesResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchSeatTypes(studyRoomId) } | ||
| studyRoomApiService.fetchSeatTypes(studyRoomId) | ||
|
|
||
| override suspend fun fetchAvailableStudyRoomTimes(): FetchAvailableStudyRoomTimesResponse = | ||
| handleNetworkRequest { studyRoomApiService.fetchAvailableStudyRoomTimes() } | ||
| studyRoomApiService.fetchAvailableStudyRoomTimes() |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# Find repository implementations that call these data source methods
rg -n -A 10 "NetworkStudyRoomDataSource|studyRoomDataSource" --type kotlin -g '!**/datasource/**'Repository: team-aliens/DMS-Android
Length of output: 5355
π Script executed:
cat -n data/src/prod/kotlin/team/aliens/dms/android/data/studyroom/repository/StudyRoomRepositoryImpl.ktRepository: team-aliens/DMS-Android
Length of output: 2654
π Script executed:
rg -n "StudyRoomRepository\|studyRoomRepository" --type kotlin -g '!**/datasource/**' -g '!**/repository/**' | head -30Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
rg -n "StudyRoomRepository" --type kotlin | grep -v "datasource\|repository/StudyRoomRepository" | head -40Repository: team-aliens/DMS-Android
Length of output: 1590
π Script executed:
cat -n feature/src/prod/kotlin/team/aliens/dms/android/feature/studyroom/details/StudyRoomDetailsViewModel.ktRepository: team-aliens/DMS-Android
Length of output: 18803
π Script executed:
cat -n feature/src/prod/kotlin/team/aliens/dms/android/feature/main/application/ApplicationViewModel.ktRepository: team-aliens/DMS-Android
Length of output: 4805
Address incomplete error handling throughout the data flow.
Removing the handleNetworkRequest wrapper without implementing proper error handling at downstream layers has created a critical gap. The repository methods have no error handling, and ViewModels only implement .onSuccess handlersβnetwork exceptions will either silently fail (read operations) or only log to console (write operations with TODO comments) without informing the UI. Implement .onFailure handlers in all repository call sites to properly handle and report errors to the UI state.
| override suspend fun fetchCreateVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> = | ||
| runCatching { | ||
| votingApiService.fetchCreateVotingItem(votingTopicId, selectedId) | ||
| ?.getOrThrow() ?: Unit | ||
| } |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# First, find the VotingApiService file
fd -t f "VotingApiService" --extension=ktRepository: team-aliens/DMS-Android
Length of output: 253
π Script executed:
# Search for the VotingApiService interface definition
rg -n --type=kotlin "interface VotingApiService" -A 30Repository: team-aliens/DMS-Android
Length of output: 7958
π Script executed:
# Read the NetworkVotingDataSourceImpl.kt file in full to see all methods
cat -n network/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSourceImpl.ktRepository: team-aliens/DMS-Android
Length of output: 1892
Refactor to avoid double-Result wrapping.
The dev version's votingApiService.fetchCreateVotingItem() returns Result<Unit>?, and wrapping it again in runCatching causes the API's Result to be unwrapped with getOrThrow() and then re-wrapped when exceptions are caughtβa double-wrapping pattern.
Use the flattened approach instead:
override suspend fun fetchCreateVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> =
votingApiService.fetchCreateVotingItem(votingTopicId, selectedId)
?.map { Unit }
?: Result.success(Unit)This aligns with how fetchDeleteVotingItem() handles its optional responseβboth are write operations returning Result<Unit> and should use consistent patterns.
π€ Prompt for AI Agents
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSourceImpl.kt`
around lines 20 - 24, The current implementation of
NetworkVotingDataSourceImpl.fetchCreateVotingItem double-wraps the API result by
calling votingApiService.fetchCreateVotingItem(...) inside runCatching and then
getOrThrow(), which rewraps exceptions; instead, return the API Result directly
and flatten nulls like fetchDeleteVotingItem does β call
votingApiService.fetchCreateVotingItem(votingTopicId, selectedId)?.map { Unit }
?: Result.success(Unit) so you don't re-wrap errors and maintain consistency
with fetchDeleteVotingItem.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (4)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
40-51: Potential premature error snackbar on initial composition.
snapshotFlowemits the current value immediately on collection. IfresultStore.resultStateMap["vote_result"]is not yet set when the screen opens,resultwill benulland the error snackbar will display immediatelyβeven though no actual failure occurred.Consider filtering out the initial null or tracking whether a result was expected:
Suggested fix: filter initial null emission
LaunchedEffect(Unit) { snapshotFlow { resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch? - }.collect { result -> + }.filterNotNull() + .collect { result -> - if (result != null) { viewModel.initState(result) resultStore.removeResult<String?>(resultKey = "vote_result") - } else { - onShowSnackBar(DmsSnackBarType.ERROR, "μ 보λ₯Ό κ°μ Έμ€μ§ λͺ» νμ΄μ") - } } }If you need to handle the case where navigation occurs without a result, consider checking this separately (e.g., via a timeout or an explicit "no result" sentinel value).
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/viewmodel/HomeViewModel.kt (1)
22-30: Unhandled failure crashes the app.Throwing the exception on failure (line 27) will crash the application when
fetchMyPage()fails (e.g., network error). This is inconsistent with the Result-based error handling pattern used elsewhere in this PR.π Proposed fix
private fun getMyPage() { viewModelScope.launch(Dispatchers.IO) { studentRepository.fetchMyPage().onSuccess { myPage -> setState { it.copy(myPage = myPage) } }.onFailure { - throw it + // Handle error gracefully - e.g., log or emit a side effect } } }feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt (2)
25-36: Missing failure handling for device token loading.If
loadDeviceToken()fails, the failure is silently ignored andfetchNotificationStatus()is never called. Consider handling the failure to provide user feedback or retry logic.π Proposed fix
private fun fetchDeviceToken() { viewModelScope.launch { deviceDataStoreDataSource.loadDeviceToken().onSuccess { deviceToken -> setState { settingState -> settingState.copy( deviceToken = deviceToken, ) } fetchNotificationStatus() + }.onFailure { + // Consider logging or emitting a side effect for user feedback } } }
38-44: Missing failure handling for sign out.If
signOut()fails, the failure is silently ignored. The user may believe they signed out when they didn't.π Proposed fix
internal fun signOut() { viewModelScope.launch(Dispatchers.IO) { authRepository.signOut().onSuccess { sendEffect(SettingSideEffect.SignOutSuccess) + }.onFailure { + // TODO: Send error side effect (e.g., SignOutFailed) } } }
π€ Fix all issues with AI agents
In `@app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt`:
- Around line 326-329: The result key used when navigating is mismatched:
resultStore.setResult<UUID?> is setting "notice_result" but NoticeDetailScreen
expects "notice_detail_result"; update the setter in the
onNavigateNotificationDetailClick handler (the call to resultStore.setResult in
DmsApp.kt where NoticeDetailScreenNav is pushed) to use the exact key
"notice_detail_result" (or alternatively change NoticeDetailScreen to read
"notice_result") so both sides use the same unique key name.
In
`@core/device/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSource.kt`:
- Line 5: Change the abstract function loadDeviceToken to be a suspend function
to match storeDeviceToken and clearDeviceToken; update the override in
DeviceDataStoreDataSourceImpl to be suspend and wrap the call to
deviceStore.loadDeviceToken() with suspendRunCatching so it is invoked from a
suspend context (keep the existing deviceStore.loadDeviceToken() call but ensure
it is called inside suspendRunCatching within the suspend override).
In
`@core/device/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.kt`:
- Around line 10-18: Prod variant of NotificationRepositoryImpl must be updated
to match the new Result-returning data source methods: change getDeviceToken()
to accept/return the Result<String> from deviceDataStore.loadDeviceToken() (or
propagate it) instead of expecting a raw String, and change saveDeviceToken() to
accept/return the Result<Unit> from deviceDataStore.storeDeviceToken() (or
propagate/handle errors) instead of assigning a Unit; update calls to
deviceDataStore.loadDeviceToken() and deviceDataStore.storeDeviceToken() in
NotificationRepositoryImpl (prod) to work with Result types (e.g., propagate
Result, map/throw as appropriate) so signatures and behavior match the dev
variant and the updated interface.
In `@core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt`:
- Around line 44-46: Remove the development debug log before throwing in
JwtProviderImpl: delete the Log.d("TEST _cachedRefreshToken!!.isExpired()", ...)
call that precedes the throw in the branch that checks
_cachedRefreshToken!!.isExpired(); after removing it, ensure the
android.util.Log import is removed if it is no longer referenced anywhere in
JwtProviderImpl.kt so there are no unused imports.
- Around line 80-84: JwtProviderImpl currently launches
CoroutineScope(Dispatchers.Default).launch and calls
jwtDataStoreDataSource.storeTokens inside runCatching with an empty onFailure,
which silences persistence errors; change this to handle failures by either
logging the exception via your logger (e.g., processLogger or Android Log) or by
propagating the error to the caller (e.g., emit a failure result) so token
persistence failures are visible. Locate the
CoroutineScope(Dispatchers.Default).launch block in JwtProviderImpl and replace
the empty onFailure { } with a concrete handler that logs the throwable
(including message and stack) and/or notifies the calling API (returning a
Result or throwing) when jwtDataStoreDataSource.storeTokens fails.
In
`@data/src/dev/kotlin/team.aliens.dms.android.data/meal/repository/MealRepositoryImpl.kt`:
- Around line 17-21: The recoverCatching in fetchMeal masks all updateMeal
failures by converting them to CannotSaveMealException; change it so it
preserves CannotFindMealException and only maps other failures to
CannotSaveMealException. Locate fetchMeal (the runCatching {
databaseMealDataSource.queryMeal(date).toModel() } block) and in the
recoverCatching handler inspect the Throwable/result from updateMeal(date) β if
it's a CannotFindMealException rethrow it, otherwise wrap/throw
CannotSaveMealException; ensure you use updateMeal(...).getOrThrow()/fold or
check the exception type instead of unconditionally using getOrElse { throw
CannotSaveMealException() }.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt`:
- Around line 105-106: The hardcoded Korean message in the onFailure lambda of
SelectProfileViewModel misrepresents different failure causes; update the
onFailure handler that calls
sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl(...)) to pass a
meaningful message from the Throwable (or via an error-mapper utility) instead
of the fixed "μ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€" so network/permission/server errors are surfaced
(e.g., use throwable.message ?: defaultMessage or
mapThrowableToUserMessage(throwable) and forward that to FailFetchPresignedUrl).
- Around line 93-101: The success branch uses unnecessary safe-call and toString
on a guaranteed non-null value; inside the onSuccess lambda, replace the
sendEffect call to pass the non-null FileUrl.fileUrl directly (matching the
setState update) by using fileUrl.fileUrl when creating
SelectProfileSideEffect.SuccessProfileImage in SelectProfileViewModelβs
onSuccess handler.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt`:
- Around line 63-68: The Column was changed to fillMaxWidth(), removing vertical
constraints so Spacer(modifier = Modifier.weight(1f)) cannot work; restore a
finite height on the Column (e.g., use fillMaxSize() or add
Modifier.fillMaxHeight()) so weight-based Spacer pushes content to the bottom.
Update the modifier on the Column in RemainApplicationScreen.kt (the Column with
Modifier.fillMaxWidth()) to include a height constraint (fillMaxSize() or
fillMaxHeight()) so Modifier.weight(1f) on the Spacer behaves as intended.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt`:
- Line 3: Remove the leftover debug logging by deleting the android.util.Log
import and any Log.d("TEST", ...) calls in VoteViewModel (e.g., the Log.d usage
around line 98 in VoteViewModel.kt); ensure you remove both the import statement
and all occurrences of Log.d so there are no unused imports or debug prints left
in the class.
- Around line 99-102: The when-block in VoteViewModel that inspects
it.message?.drop(5)?.trim() to detect "409" is brittle; replace this
string-slice parsing with a robust check: detect the actual exception type or
structured status code (e.g., check if the throwable is an HttpException or a
ConflictException and read its status/code property) and then call
sendEffect(VoteSideEffect.VoteConflict) or VoteFail accordingly; update the
error-handling branch that currently contains the when(...) so it uses the
exception class or explicit status code field instead of message manipulation.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt`:
- Line 3: Remove the unused import of PasswordMismatchException from
NetworkUserDataSourceImpl (it is not referenced after migrating to
suspendRunCatching); open the NetworkUserDataSourceImpl.kt file, delete the line
importing
team.aliens.dms.android.network.user.exception.PasswordMismatchException, run a
Kotlin/IDE quick cleanup or recompile to ensure no other references remain, and
commit the change.
β»οΈ Duplicate comments (2)
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt (1)
70-72: Silent exception swallowing inloadTokensremains unaddressed.The empty
onFailureblock silently swallows exceptions when token loading fails, making debugging difficult. At minimum, consider logging the failure (without sensitive token data).Proposed fix
- }.onFailure { - + }.onFailure { exception -> + Log.w("JwtProvider", "Failed to load tokens", exception) }feature/src/dev/kotlin/team/aliens/dms/android/feature/notice/ui/NoticeDetailScreen.kt (1)
47-58: Two issues remain: type mismatch and spurious error snackbar on initial composition.
Line 53 uses
removeResult<String?>but should beremoveResult<UUID?>to match the type being stored and read.
snapshotFlowemitsnullimmediately on first composition (before any result is set), triggering the error snackbar on every screen open. Consider filtering out the initial null emission.π Proposed fix
LaunchedEffect(Unit) { snapshotFlow { resultStore.resultStateMap["notice_detail_result"]?.value as? UUID? - }.collect { result -> + }.filterNotNull().collect { result -> - if (result != null) { - viewModel.getNotificationDetail(result) - resultStore.removeResult<String?>(resultKey = "notice_detail_result") - } else { - onShowSnackBar(DmsSnackBarType.ERROR, "μ 보λ₯Ό κ°μ Έμ€μ§ λͺ» νμ΄μ") - } + viewModel.getNotificationDetail(result) + resultStore.removeResult<UUID?>(resultKey = "notice_detail_result") } }
π§Ή Nitpick comments (10)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (1)
5-5: Unused import:Boxis imported but not used.This import appears to be dead code. Consider removing it to keep imports clean.
π§Ή Suggested fix
-import androidx.compose.foundation.layout.Boxfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt (1)
87-89: Unusedwithreceiver scope.The
with(uiState.value)block is declared but not utilizedβcode inside still referencesuiState.valueexplicitly. Either remove thewithwrapper or use the receiver scope.Proposed fix: remove unused with
internal fun postVote() { - with(uiState.value) { - viewModelScope.launch(Dispatchers.IO) { - setState { uiState.value.copy(isLoading = true, buttonEnabled = false) } - voteRepository.fetchCreateVotingItem( - votingTopicId = uiState.value.vote.id, - selectedId = uiState.value.selectId!!, - ).onSuccess { + viewModelScope.launch(Dispatchers.IO) { + setState { uiState.value.copy(isLoading = true, buttonEnabled = false) } + voteRepository.fetchCreateVotingItem( + votingTopicId = uiState.value.vote.id, + selectedId = uiState.value.selectId!!, + ).onSuccess { // ... rest unchanged - } } } }core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt (1)
14-14: Unused import.The
nowimport fromteam.aliens.dms.android.shared.date.utilis not used anywhere in this file.Proposed fix
-import team.aliens.dms.android.shared.date.util.nowshared/exception/src/main/java/team/aliens/dms/android/shared/exception/util/SuspendRunCatching.kt (1)
5-11: Good implementation addressing CancellationException handling.This correctly rethrows
CancellationExceptionto preserve structured concurrency, which addresses the issue flagged in previous reviews aboutrunCatchingswallowing cancellation signals.One minor observation: the function is named
suspendRunCatchingbut accepts a non-suspend lambda() -> T. This works because callers invoke suspend API methods that return values synchronously within the lambda, but the naming could be slightly misleading. Consider either:
- Renaming to
runCatchingCancellableor similar, or- Changing the signature to
suspend () -> Tif you want the name to match the behaviorThis is a minor naming nitβthe current implementation is functionally correct.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/viewmodel/HomeViewModel.kt (1)
14-16: Consider makingstudentRepositoryprivate.The repository is injected but exposed as a public
val. Since it's only used internally, consider making itprivate.β»οΈ Proposed fix
`@HiltViewModel` internal class HomeViewModel `@Inject` constructor( - val studentRepository: StudentRepository, + private val studentRepository: StudentRepository, ) : BaseStateViewModel<HomeState, HomeSideEffect>(HomeState()) {feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt (1)
15-19: Consider making injected dependencies private.The repositories and data source are exposed as public
valbut appear to only be used internally.β»οΈ Proposed fix
`@HiltViewModel` class SettingViewModel `@Inject` constructor( - val notificationRepository: NotificationRepository, - val authRepository: AuthRepository, - val deviceDataStoreDataSource: DeviceDataStoreDataSource, + private val notificationRepository: NotificationRepository, + private val authRepository: AuthRepository, + private val deviceDataStoreDataSource: DeviceDataStoreDataSource, ): BaseStateViewModel<SettingState, SettingSideEffect>(SettingState()) {feature/src/dev/kotlin/team/aliens/dms/android/feature/notice/viewmodel/NoticeDetailViewModel.kt (2)
55-57: Consider including error details in the failure side effect.
FailNoticeDetailis a singleton object that doesn't carry error information. Including the error message would help the UI display meaningful feedback to the user.β»οΈ Proposed enhancement
internal sealed interface NoticeDetailSideEffect { - object FailNoticeDetail : NoticeDetailSideEffect + data class FailNoticeDetail(val message: String? = null) : NoticeDetailSideEffect }Then update the emission:
- sendEffect(NoticeDetailSideEffect.FailNoticeDetail) + sendEffect(NoticeDetailSideEffect.FailNoticeDetail(it.message))
45-53: Default state usesUUID.randomUUID()as placeholder.Using
UUID.randomUUID()in the default state creates a new random ID each time. This is acceptable for a placeholder, but consider using a constant sentinel value or makingnoticenullable to clearly distinguish "not loaded" from "loaded" states.network/src/dev/kotlin/team.aliens.dms.android.network/student/datasource/NetworkStudentDataSourceImpl.kt (1)
79-79: Add explicit return type towithdraw()for consistency.All other methods explicitly declare their
Result<T>return type, butwithdraw()relies on type inference. Adding the explicit type improves readability and consistency.β»οΈ Suggested change
- override suspend fun withdraw() = suspendRunCatching { studentApiService.withdraw() } + override suspend fun withdraw(): Result<Unit> = suspendRunCatching { studentApiService.withdraw() }feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/NotificationScreen.kt (1)
202-258: Optional: extract a shared row composable to reduce duplication.NotificationItem and NoticeItem share nearly identical layout/behavior; a small shared composable would make future UI tweaks safer and cheaper.
Also applies to: 261-318
...ice/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSource.kt
Outdated
Show resolved
Hide resolved
| override fun loadDeviceToken() = suspendRunCatching { deviceStore.loadDeviceToken() } | ||
|
|
||
| override suspend fun storeDeviceToken(deviceToken: String) { | ||
| override suspend fun storeDeviceToken(deviceToken: String) = suspendRunCatching { | ||
| deviceStore.storeDeviceToken(deviceToken) | ||
| } | ||
|
|
||
| override suspend fun clearDeviceToken() { | ||
| override suspend fun clearDeviceToken() = suspendRunCatching { | ||
| deviceStore.clearDeviceToken() | ||
| } |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Find the DeviceDataStoreDataSource interface
echo "=== Looking for DeviceDataStoreDataSource interface ==="
find . -name "DeviceDataStoreDataSource.kt" -o -name "DeviceDataStoreDataSource.java" | head -20Repository: team-aliens/DMS-Android
Length of output: 225
π Script executed:
#!/bin/bash
# Search in the device datastore directory for the interface
echo "=== Listing files in device datastore directory ==="
fd -e kt -e java "datastore" --type f | grep -i "DeviceDataStore" | head -30Repository: team-aliens/DMS-Android
Length of output: 324
π Script executed:
#!/bin/bash
# Get more context about the implementation file structure
echo "=== Checking the directory structure around DeviceDataStoreDataSourceImpl.kt ==="
fd -e kt "DeviceDataStore" | head -30Repository: team-aliens/DMS-Android
Length of output: 353
π Script executed:
#!/bin/bash
# Examine the interface definition
echo "=== DeviceDataStoreDataSource interface ==="
cat "./core/device/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSource.kt"Repository: team-aliens/DMS-Android
Length of output: 399
π Script executed:
#!/bin/bash
# Examine the implementation again to compare with interface
echo "=== DeviceDataStoreDataSourceImpl implementation ==="
cat "./core/device/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.kt"Repository: team-aliens/DMS-Android
Length of output: 835
π Script executed:
#!/bin/bash
# Search for all usages of these three methods across the codebase
echo "=== Searching for calls to loadDeviceToken, storeDeviceToken, clearDeviceToken ==="
rg -n "\.loadDeviceToken\(\)|\.storeDeviceToken\(|\.clearDeviceToken\(" --type kotlin -B 3 -A 3Repository: team-aliens/DMS-Android
Length of output: 5876
Critical: Prod variant of NotificationRepositoryImpl is incompatible with these changes.
The implementation correctly matches the updated interface signature (loadDeviceToken(): Result<String>, storeDeviceToken(): Result<Unit>, clearDeviceToken(): Result<Unit>). However, the prod variant of NotificationRepositoryImpl.kt has not been updated and will fail to compile:
- Line 67:
getDeviceToken()expects to returnStringbutloadDeviceToken()now returnsResult<String> - Line 63:
saveDeviceToken()expects to assignUnitbutstoreDeviceToken()now returnsResult<Unit>
The dev variant correctly handles the Result types. Align the prod variant signatures and implementations with the dev variant.
Note: loadDeviceToken() is correctly a non-suspend function matching the interface contract, so suspendRunCatching is appropriate for exception-to-Result conversion here.
π€ Prompt for AI Agents
In
`@core/device/src/main/java/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.kt`
around lines 10 - 18, Prod variant of NotificationRepositoryImpl must be updated
to match the new Result-returning data source methods: change getDeviceToken()
to accept/return the Result<String> from deviceDataStore.loadDeviceToken() (or
propagate it) instead of expecting a raw String, and change saveDeviceToken() to
accept/return the Result<Unit> from deviceDataStore.storeDeviceToken() (or
propagate/handle errors) instead of assigning a Unit; update calls to
deviceDataStore.loadDeviceToken() and deviceDataStore.storeDeviceToken() in
NotificationRepositoryImpl (prod) to work with Result types (e.g., propagate
Result, map/throw as appropriate) so signatures and behavior match the dev
variant and the updated interface.
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt
Show resolved
Hide resolved
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt
Show resolved
Hide resolved
| onFailure = { | ||
| sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl("μ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€")) |
There was a problem hiding this comment.
Failure message is potentially misleading.
Hardcoding βμ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€β for all presigned URL failures can misrepresent network/permission/server errors. Consider using the Throwable (or an error mapper) to pick a more accurate message.
π‘ Minimal adjustment to surface the actual failure
- onFailure = {
- sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl("μ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€"))
+ onFailure = { throwable ->
+ sendEffect(
+ SelectProfileSideEffect.FailFetchPresignedUrl(
+ throwable.message ?: "νλ‘ν μ΄λ―Έμ§ μ
λ‘λμ μ€ν¨νμ΅λλ€"
+ )
+ )
}π 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.
| onFailure = { | |
| sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl("μ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€")) | |
| onFailure = { throwable -> | |
| sendEffect( | |
| SelectProfileSideEffect.FailFetchPresignedUrl( | |
| throwable.message ?: "νλ‘ν μ΄λ―Έμ§ μ λ‘λμ μ€ν¨νμ΅λλ€" | |
| ) | |
| ) | |
| } |
π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt`
around lines 105 - 106, The hardcoded Korean message in the onFailure lambda of
SelectProfileViewModel misrepresents different failure causes; update the
onFailure handler that calls
sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl(...)) to pass a
meaningful message from the Throwable (or via an error-mapper utility) instead
of the fixed "μ΄λ―Έμ§κ° μ‘΄μ¬νμ§ μμ΅λλ€" so network/permission/server errors are surfaced
(e.g., use throwable.message ?: defaultMessage or
mapThrowableToUserMessage(throwable) and forward that to FailFetchPresignedUrl).
| Column( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(DmsTheme.colorScheme.background), | ||
| .fillMaxWidth() | ||
| .background(DmsTheme.colorScheme.background) | ||
| .systemBarsPadding(), | ||
| ) { |
There was a problem hiding this comment.
Potential layout issue: weight() modifier requires finite height constraints.
Changing from fillMaxSize() to fillMaxWidth() removes the explicit height constraint on the Column. The Spacer(modifier = Modifier.weight(1f)) at line 109 relies on finite height bounds to push the button to the bottom. Without fillMaxSize() (or at least a height constraint), the weight modifier will have no effect, and the button may not position correctly at the bottom of the screen.
π§ Suggested fix
Column(
modifier = Modifier
- .fillMaxWidth()
+ .fillMaxSize()
.background(DmsTheme.colorScheme.background)
.systemBarsPadding(),
) {π 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.
| Column( | |
| modifier = Modifier | |
| .fillMaxSize() | |
| .background(DmsTheme.colorScheme.background), | |
| .fillMaxWidth() | |
| .background(DmsTheme.colorScheme.background) | |
| .systemBarsPadding(), | |
| ) { | |
| Column( | |
| modifier = Modifier | |
| .fillMaxSize() | |
| .background(DmsTheme.colorScheme.background) | |
| .systemBarsPadding(), | |
| ) { |
π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt`
around lines 63 - 68, The Column was changed to fillMaxWidth(), removing
vertical constraints so Spacer(modifier = Modifier.weight(1f)) cannot work;
restore a finite height on the Column (e.g., use fillMaxSize() or add
Modifier.fillMaxHeight()) so weight-based Spacer pushes content to the bottom.
Update the modifier on the Column in RemainApplicationScreen.kt (the Column with
Modifier.fillMaxWidth()) to include a height constraint (fillMaxSize() or
fillMaxHeight()) so Modifier.weight(1f) on the Spacer behaves as intended.
| @@ -1,5 +1,6 @@ | |||
| package team.aliens.dms.android.feature.vote.viewmodel | |||
|
|
|||
| import android.util.Log | |||
There was a problem hiding this comment.
Remove debug logging before merge.
Log.d("TEST", ...) appears to be leftover debug code. Remove it along with the unused android.util.Log import.
Proposed fix
-import android.util.Log }.onFailure {
setState { uiState.value.copy(isLoading = false, buttonEnabled = true) }
- Log.d("TEST", it.toString())
when (it.message?.drop(5)?.trim()) {Also applies to: 98-98
π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt`
at line 3, Remove the leftover debug logging by deleting the android.util.Log
import and any Log.d("TEST", ...) calls in VoteViewModel (e.g., the Log.d usage
around line 98 in VoteViewModel.kt); ensure you remove both the import statement
and all occurrences of Log.d so there are no unused imports or debug prints left
in the class.
| when (it.message?.drop(5)?.trim()) { | ||
| "409" -> sendEffect(VoteSideEffect.VoteConflict) | ||
| else -> sendEffect(VoteSideEffect.VoteFail) | ||
| } |
There was a problem hiding this comment.
Fragile error code parsing from exception message.
Parsing the HTTP status code via it.message?.drop(5)?.trim() assumes a very specific message format (e.g., "code 409"). This is brittle and will silently fail if the message format changes or differs across error sources.
Consider either:
- Restoring type-based checking (e.g.,
is ConflictException) - Using a structured error type that exposes the status code explicitly
- At minimum, using a regex or more robust parsing
Suggested approach using exception type
}.onFailure {
setState { uiState.value.copy(isLoading = false, buttonEnabled = true) }
- Log.d("TEST", it.toString())
- when (it.message?.drop(5)?.trim()) {
- "409" -> sendEffect(VoteSideEffect.VoteConflict)
+ when (it) {
+ is ConflictException -> sendEffect(VoteSideEffect.VoteConflict)
else -> sendEffect(VoteSideEffect.VoteFail)
}
}π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt`
around lines 99 - 102, The when-block in VoteViewModel that inspects
it.message?.drop(5)?.trim() to detect "409" is brittle; replace this
string-slice parsing with a robust check: detect the actual exception type or
structured status code (e.g., check if the throwable is an HttpException or a
ConflictException and read its status/code property) and then call
sendEffect(VoteSideEffect.VoteConflict) or VoteFail accordingly; update the
error-handling branch that currently contains the when(...) so it uses the
exception class or explicit status code field instead of message manipulation.
.../src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
119-130: LaunchedEffect missingisJwtAvailabledependency.The effect uses
isJwtAvailablein its logic (line 122) but only depends onisOnboardingCompleted. If the JWT state changes after onboarding is complete (e.g., token expiration or external logout), the navigation won't update to reflect the new state.π Proposed fix
- LaunchedEffect(isOnboardingCompleted) { + LaunchedEffect(isOnboardingCompleted, isJwtAvailable) { val initialScreen = when { !isOnboardingCompleted -> OnboardingScreenNav isJwtAvailable -> HomeScreenNav else -> SignInScreenNav }
π€ Fix all issues with AI agents
In
`@data/src/dev/kotlin/team.aliens.dms.android.data/meal/repository/MealRepositoryImpl.kt`:
- Around line 19-26: The recoverCatching block currently catches Throwable and
thus swallows coroutine cancellations; update the exception handling in the
block that calls updateMeal(date).getOrThrow() to add a specific catch for
kotlinx.coroutines.CancellationException (re-throw it immediately) before the
generic catch (_: Throwable) that wraps failures into CannotSaveMealException,
and add the corresponding import for CancellationException; keep the existing
catches for CannotFindMealException and CannotSaveMealException handling as-is.
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt`:
- Around line 16-19: The dev implementation of comparePassword in
NetworkUserDataSourceImpl uses suspendRunCatching which preserves a 401 as
HttpException instead of mapping it to PasswordMismatchException; change the
implementation of comparePassword to recover 401 responses and throw
PasswordMismatchException (e.g. catch HttpException with statusCode 401 or use
Result.recover/exception mapping) so that comparePassword(password: String)
returns Result.failure(PasswordMismatchException()) for 401 while preserving
other errors unchanged; reference the comparePassword method, suspendRunCatching
call, and PasswordMismatchException when making the change.
β»οΈ Duplicate comments (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
105-106: Hardcoded failure message still doesn't surface the actual error.The previous review suggested using the
Throwableto provide a more accurate error message. While the message text was improved, theonFailurelambda still doesn't capture the throwable to provide context about what actually failed (network error, permission error, server error, etc.).π Proposed fix to capture and use the throwable
- onFailure = { - sendEffect(SelectProfileSideEffect.FailFetchPresignedUrl("νλ‘ν μ΄λ―Έμ§ μ λ‘λμ μ€ν¨νμ΅λλ€")) + onFailure = { throwable -> + sendEffect( + SelectProfileSideEffect.FailFetchPresignedUrl( + throwable.message ?: "νλ‘ν μ΄λ―Έμ§ μ λ‘λμ μ€ν¨νμ΅λλ€" + ) + ) }
π§Ή Nitpick comments (1)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
143-152: Clarify the back-stack management logic with a comment.The
whenblock only has one case forHomeScreenNav, and the subsequentremoveAllalways executes. While the logic works correctly for bottom navigation behavior (ensuring single instances of bottom nav destinations), a brief comment would improve readability.βοΈ Optional: Add clarifying comment
if (currentScreen != destination) { + // Remove the destination if already in stack to avoid duplicates when (destination) { is HomeScreenNav -> { backStack.remove(HomeScreenNav) } } + // Clear other bottom nav destinations to maintain single-screen-per-tab behavior backStack.removeAll { it is ApplicationScreenNav || it is MyPageScreenNav }
data/src/dev/kotlin/team.aliens.dms.android.data/meal/repository/MealRepositoryImpl.kt
Show resolved
Hide resolved
| override suspend fun comparePassword(password: String): Result<Unit> = | ||
| suspendRunCatching { | ||
| userApiService.comparePassword(password) | ||
| } |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Find the file in question
find . -name "NetworkUserDataSourceImpl.kt" -type fRepository: team-aliens/DMS-Android
Length of output: 271
π Script executed:
#!/bin/bash
# Search for handleNetworkRequest to understand previous implementation
rg "handleNetworkRequest" --type kt -A 5Repository: team-aliens/DMS-Android
Length of output: 93
π Script executed:
#!/bin/bash
# Search for PasswordMismatchException
rg "PasswordMismatchException" --type ktRepository: team-aliens/DMS-Android
Length of output: 93
π Script executed:
#!/bin/bash
# Look at git history for changes to comparePassword method
git log --oneline --all -- "*/NetworkUserDataSourceImpl.kt" | head -20Repository: team-aliens/DMS-Android
Length of output: 328
π Script executed:
#!/bin/bash
# Read the NetworkUserDataSourceImpl.kt file
cat -n "./network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt"Repository: team-aliens/DMS-Android
Length of output: 985
π Script executed:
#!/bin/bash
# Search for handleNetworkRequest in kotlin files
rg "handleNetworkRequest" --glob "*.kt" -A 5Repository: team-aliens/DMS-Android
Length of output: 38093
π Script executed:
#!/bin/bash
# Search for PasswordMismatchException
rg "PasswordMismatchException" --glob "*.kt"Repository: team-aliens/DMS-Android
Length of output: 2060
π Script executed:
#!/bin/bash
# Look at the git diff for the most recent user datasource change
git show 759e5db79 -- "*/NetworkUserDataSourceImpl.kt" | head -100Repository: team-aliens/DMS-Android
Length of output: 1346
π Script executed:
#!/bin/bash
# Find suspendRunCatching implementation
rg "fun suspendRunCatching" --glob "*.kt" -A 10Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
#!/bin/bash
# Find the full handleNetworkRequest implementation
cat -n core/network/src/main/java/team/aliens/dms/android/core/network/util/NetworkRequestHandler.kt | head -50Repository: team-aliens/DMS-Android
Length of output: 2811
π Script executed:
#!/bin/bash
# Check if there are recent changes to the dev user datasource
git log --oneline --all -- "network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt" | head -20Repository: team-aliens/DMS-Android
Length of output: 136
π Script executed:
#!/bin/bash
# Check the domain layer for error mapping
find . -path "*/data/*/auth/*" -name "*Repository*.kt" | head -5 | xargs grep -l "PasswordMismatch"Repository: team-aliens/DMS-Android
Length of output: 51
π Script executed:
#!/bin/bash
# Search for suspendRunCatching - might be in shared exception util
rg "suspendRunCatching" --glob "*.kt" -B 2 -A 10 | head -50Repository: team-aliens/DMS-Android
Length of output: 7093
π Script executed:
#!/bin/bash
# Look for where suspendRunCatching is imported from
rg "suspendRunCatching" --glob "*.kt" -B 5 | grep -E "(import|package)" | head -20Repository: team-aliens/DMS-Android
Length of output: 3484
π Script executed:
#!/bin/bash
# Check the recent code review commit for dev file
git show d304aac79 -- "network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt"Repository: team-aliens/DMS-Android
Length of output: 1416
π Script executed:
#!/bin/bash
# Look for domain layer AuthRepository to see if it handles 401 β PasswordMismatchException
cat -n data/src/prod/kotlin/team/aliens/dms/android/data/auth/repository/AuthRepositoryImpl.kt | grep -A 20 -B 5 "onUnauthorized\|comparePassword"Repository: team-aliens/DMS-Android
Length of output: 1131
π Script executed:
#!/bin/bash
# Get the full suspendRunCatching implementation
cat -n shared/exception/src/main/java/team/aliens/dms/android/shared/exception/util/SuspendRunCatching.ktRepository: team-aliens/DMS-Android
Length of output: 454
π Script executed:
#!/bin/bash
# Check if there's a comparePassword in the domain layer and how it handles exceptions
rg "comparePassword" --glob "*.kt" -B 3 -A 10Repository: team-aliens/DMS-Android
Length of output: 10965
π Script executed:
#!/bin/bash
# Find the statusMapping function to understand domain error handling
rg "fun statusMapping" --glob "*.kt" -A 20 | head -80Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
#!/bin/bash
# Check if there's diff/history showing when prod switched away from Result pattern
git log --oneline --all -- "network/src/prod/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt" | head -20Repository: team-aliens/DMS-Android
Length of output: 141
π Script executed:
#!/bin/bash
# Look at prod repository to confirm it doesn't use Result pattern
cat -n data/src/prod/kotlin/team/aliens/dms/android/data/user/repository/UserRepositoryImpl.kt | head -40Repository: team-aliens/DMS-Android
Length of output: 1076
Implement 401 β PasswordMismatchException mapping in dev flavor.
The dev flavor lacks the 401 error mapping that exists in the prod implementation. With suspendRunCatching, a 401 will wrap as Result.failure(HttpException), losing the semantic distinction that prod provides via handleNetworkRequest(on401 = { throw PasswordMismatchException() }). This breaks consistency between flavors and prevents features from distinguishing password failures from other HTTP errors.
Add exception recovery to preserve the mapping:
Suggested fix
+import retrofit2.HttpException
+import team.aliens.dms.android.network.user.exception.PasswordMismatchException
@@
override suspend fun comparePassword(password: String): Result<Unit> =
suspendRunCatching {
userApiService.comparePassword(password)
}
+ .recoverCatching { e ->
+ if (e is HttpException && e.code() == 401) throw PasswordMismatchException()
+ throw e
+ }π€ Prompt for AI Agents
In
`@network/src/dev/kotlin/team.aliens.dms.android.network/user/datasource/NetworkUserDataSourceImpl.kt`
around lines 16 - 19, The dev implementation of comparePassword in
NetworkUserDataSourceImpl uses suspendRunCatching which preserves a 401 as
HttpException instead of mapping it to PasswordMismatchException; change the
implementation of comparePassword to recover 401 responses and throw
PasswordMismatchException (e.g. catch HttpException with statusCode 401 or use
Result.recover/exception mapping) so that comparePassword(password: String)
returns Result.failure(PasswordMismatchException()) for 401 while preserving
other errors unchanged; reference the comparePassword method, suspendRunCatching
call, and PasswordMismatchException when making the change.
1 participant
κ°μ
μμ μ¬ν
μΆκ° λ‘ ν λ§
Summary by CodeRabbit
New Features
Improvements
βοΈ Tip: You can customize this high-level summary in your review settings.