๐ :: (#816) ๋ง์ดํ์ด์ง ๊ตฌํ #817
Conversation
๐ WalkthroughWalkthroughAdds profile image upload/adjust flows, point-history and password-reset screens with viewmodels, file/user repositories and DI bindings, design-system updates, datetime migration to kotlinx.datetime, Gradle dependencies, and two dev manifest permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectUI as SelectProfile Screen
participant SelectVM as SelectProfileViewModel
participant FileRepo as FileRepository
participant NetworkDS as NetworkFileDataSource
participant FileAPI as File API Service
participant MediaStore
User->>SelectUI: open gallery (request permission)
SelectUI->>MediaStore: query images
MediaStore-->>SelectUI: image URIs
User->>SelectUI: select uri
SelectUI->>SelectVM: selectImage(uri)
User->>SelectUI: tap "์ ํ" (upload)
SelectUI->>SelectVM: uploadProfileImage(uri)
SelectVM->>FileRepo: fetchPresignedUrl(fileName)
FileRepo->>NetworkDS: fetchPresignedUrl(fileName)
NetworkDS->>FileAPI: GET presigned URL
FileAPI-->>NetworkDS: presigned url response
NetworkDS-->>FileRepo: PresignedFileUrl
FileRepo-->>SelectVM: PresignedFileUrl
SelectVM->>FileRepo: uploadFile(presignedUrl, file)
FileRepo->>NetworkDS: uploadFile(presignedUrl, file)
NetworkDS->>FileAPI: PUT file to presigned URL
FileAPI-->>NetworkDS: file upload response (fileUrl)
NetworkDS-->>FileRepo: FileUrl
FileRepo-->>SelectVM: FileUrl
SelectVM-->>SelectUI: emit SuccessProfileImage (profileImageUrl)
SelectUI->>User: navigate to AdjustProfile
sequenceDiagram
participant User
participant CheckUI as CheckPassword Screen
participant CheckVM as CheckPasswordViewModel
participant UserRepo as UserRepository
participant UserAPI as User API
User->>CheckUI: enter current password
CheckUI->>CheckVM: resetPassword()
CheckVM->>UserRepo: comparePassword(currentPassword)
UserRepo->>UserAPI: verify endpoint
UserAPI-->>UserRepo: success / error
alt success
UserRepo-->>CheckVM: success
CheckVM-->>CheckUI: emit SuccessCheckPassword
CheckUI->>User: navigate to ResetPassword screen
else failure
UserRepo-->>CheckVM: error
CheckVM-->>CheckUI: emit FailCheckPassword(message)
CheckUI->>User: show snackbar
end
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
๐ฅ 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 |
Summary of ChangesHello @uson1004, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! ์ด PR์ ์ฌ์ฉ์์๊ฒ ๊ณ์ ์ค์ ์ ๊ด๋ฆฌํ๊ณ ๊ฐ์ธ ์ ๋ณด๋ฅผ ํ์ธํ ์ ์๋ ์ค์ ์ง์ค์ ํ๋ธ๋ฅผ ์ ๊ณตํ๋ ํฌ๊ด์ ์ธ '๋ง์ดํ์ด์ง' ๊ธฐ๋ฅ์ ๋์ ํฉ๋๋ค. ์์ ํ ๋น๋ฐ๋ฒํธ ์ฌ์ค์ , ์ด๋ฏธ์ง ์ ํ ๋ฐ ์กฐ์ ๊ธฐ๋ฅ์ ํตํ ํ๋กํ ์ฌ์ง ์ฌ์ฉ์ ์ ์, ํธ์ ์๋ฆผ ํ๊ฒฝ ์ค์ ๊ด๋ฆฌ, ๊ทธ๋ฆฌ๊ณ ์์ ํ ๋ก๊ทธ์์์ ์ํ ์ ์ฉ ํ๋ฉด๊ณผ ๋ด๋น๊ฒ์ด์ ํ๋ฆ์ ํฌํจํฉ๋๋ค. ๋ํ, ์ฌ์ฉ์์ ์๋ฒ์ ๋ด์ญ์ ์์ธํ๊ฒ ๋ณด์ฌ์ฃผ๋ ํญ ํ์์ ๋ทฐ๋ฅผ ์ ๊ณตํ์ฌ ์ฌ์ฉ์ ๊ฒฝํ์ ํฅ์์ํต๋๋ค. Highlights
๐ง New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with ๐ and ๐ on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Fix all issues with AI Agents ๐ค
In @app/src/dev/AndroidManifest.xml:
- Line 10: Add backward compatibility for media permissions: update
AndroidManifest.xml to include the legacy permission
android.permission.READ_EXTERNAL_STORAGE in addition to
android.permission.READ_MEDIA_IMAGES, and modify runtime permission logic in
SelectProfileScreen.kt to check Build.VERSION.SDK_INT so it requests
android.permission.READ_MEDIA_IMAGES on Android 13+ (API 33+) and
android.permission.READ_EXTERNAL_STORAGE on Android 12 and below, mirroring the
pattern used in SignInScreen.kt for notifications; ensure permission strings and
request flow are switched based on the SDK check.
In @app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt:
- Around line 279-287: The code reconstructs the nav key instead of using the
provided entry parameter; update the entry<AdjustProfileScreenNav> lambda to
accept a named parameter (e.g., entry) and call backStack.remove(entry) in
onBackPressed, and use entry.model for the model parameter rather than
constructing AdjustProfileScreenNav(it.model).
- Around line 190-199: The onNavigateBack lambda reconstructs a VoteScreenNav
instance and calls backStack.remove(...) which is fragile because removal relies
on exact equality; instead remove the actual entry object passed into the
entry<VoteScreenNav> { voteNav -> } lambda. Locate the entry block that creates
VoteRoute (entry<VoteScreenNav> { voteNav -> }) and change the onNavigateBack
implementation to call backStack.remove(voteNav) (use the voteNav parameter
directly) rather than creating a new VoteScreenNav instance.
- Around line 244-247: The current lambda constructs a new PointHistoryScreenNav
to remove (fragile); instead capture the nav entry provided by the entry<T>
block and remove that exact instance. Change the entry<PointHistoryScreenNav>
block to accept the nav variable (e.g., entry<PointHistoryScreenNav> { nav ->
PointHistoryRoute(onBackClick = { backStack.remove(nav) } ) }) so
PointHistoryRoute calls backStack.remove with the original nav instance rather
than recreating one.
- Around line 69-70: PointHistoryScreenNav is @Serializable but its property
PointType is not, causing runtime failures; mark the PointType enum itself with
@Serializable. Edit the PointType enum declarations (both the prod and dev
variants of the PointType enum) and add the @Serializable annotation to the enum
definition so PointHistoryScreenNav can be serialized correctly.
In
@core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/dialog/AlertDialog.kt:
- Around line 25-26: The titleContentColor and textContentColor default to
DmsTheme.colorScheme.surfaceContainer which is a background color; change their
defaults to appropriate "on" content colors (e.g.,
DmsTheme.colorScheme.onSurface or DmsTheme.colorScheme.onSurfaceVariant) so
text/heading use accessible contrast. Update the default values for
titleContentColor and textContentColor in AlertDialog.kt to use the
onSurface/onSurfaceVariant variants from DmsTheme.colorScheme.
In @data/src/dev/kotlin/team.aliens.dms.android.data/point/model/Point.kt:
- Line 3: Remove the unused import
team.aliens.dms.android.shared.date.toLocalDate from PointMapper.kt; keep the
kotlinx.datetime.LocalDate import and ensure no code references to toLocalDate
remain (update/remove any unused helper calls), then re-run the build or linter
to confirm the unused-import warning is resolved.
In
@data/src/dev/kotlin/team.aliens.dms.android.data/user/repository/UserRepository.kt:
- Around line 5-10: The dev variant API in UserRepository.kt exposes
editPassword and comparePassword returning Result<Unit>, which mismatches the
prod variant; update the signatures for both abstract suspend fun
editPassword(currentPassword: String, newPassword: String) and abstract suspend
fun comparePassword(password: String) to the same return type as the prod
variant (or vice versa) across build variants โ choose one consistent contract
(preferably Result<Unit> for unified error handling), then update all
implementing classes and usages to match that return type so both dev and prod
have identical method signatures.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt:
- Line 7: Remove the unused import navigationBarsPadding from the top of
ApplicationScreen.kt; if you intended to use system navigation padding instead
apply navigationBarsPadding() to the relevant Modifier chain in the composable
(e.g., the root Modifier in ApplicationScreen) otherwise simply delete the
import statement to eliminate the unused import.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/OptionsContent.kt:
- Around line 31-62: OptionsContent currently creates Option items with empty
onClick lambdas; add callback parameters to the OptionsContent composable
signature (e.g., onChangePassword: () -> Unit, onOpenNotificationSettings: () ->
Unit, onThemeChange: () -> Unit, onSignOut: () -> Unit, onWithdraw: () -> Unit)
alongside the existing onNavigatePointHistory(PointType) and then replace each
empty lambda in the Option lists so "๋น๋ฐ๋ฒํธ ๋ณ๊ฒฝ" calls onChangePassword, "์๋ฆผ ์ค์ "
calls onOpenNotificationSettings, "ํ
๋ง ๋ณ๊ฒฝ" calls onThemeChange, "๋ก๊ทธ์์" calls
onSignOut, and "ํ์ ํํด" calls onWithdraw (leave onNavigatePointHistory for "์๋ฒ์
๋ด์ญ ํ์ธ" as-is).
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/PointCards.kt:
- Around line 56-60: The when expression for pointType currently falls through
to else for PointType.ALL; add an explicit branch for PointType.ALL that returns
a visible label (e.g. "์ ์ฒด") and appropriate colors so it no longer relies on the
generic else case โ update the destructuring assignment (val (text, textColor,
backgroundColor)) to include PointType.ALL -> Triple("์ ์ฒด",
DmsTheme.colorScheme.onSurface, DmsTheme.colorScheme.surface) (or other
project-appropriate neutral colors) while keeping the existing PointType.BONUS
and PointType.MINUS branches.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/component/PointItem.kt:
- Around line 47-63: The destructured backgroundColor from the Triple
(backgroundColor, icon, pointColor) is unused; update the Box in PointItem.kt to
apply a background using that backgroundColor (e.g., via
Modifier.background(backgroundColor, shape) or a suitable Material/DmsTheme
shape) so the backgroundColor is actually used for the icon container; ensure
the Box keeps its contentAlignment and existing Image usage and preserve
pointColor for any text/icon tinting if needed.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt:
- Around line 100-106: The remember call for pointHistoryList uses an incomplete
key causing stale values: update the keys to include all dependencies (page plus
state.allPointList, state.bonusPointList, and state.minusPointList) so the
lambda recomputes when any list changes; locate the remember(...) that defines
pointHistoryList (which switches on tabData[page] with PointTab.All,
PointTab.Bonus, PointTab.Minus) and expand its key list to cover page and each
of state.allPointList, state.bonusPointList, state.minusPointList.
- Around line 40-42: The empty LaunchedEffect(Unit) block in PointHistoryScreen
(the composable in PointHistoryScreen.kt) should be removed because it has no
effect; locate the LaunchedEffect(Unit) invocation and delete the entire block
(or replace it with the intended side-effect logic if there was supposed to be
one), ensuring no dangling braces or unused imports remain after removal.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/point/viewmodel/PointHistoryViewModel.kt:
- Line 3: Remove the unused import androidx.lifecycle.SavedStateHandle from
PointHistoryViewModel.kt; locate the import statement at the top of the file and
delete it (or let your IDE/organize imports remove it), then rebuild or run lint
to ensure no other unused imports remain.
- Line 8: Remove the unused import NotificationRepository from the top of
PointHistoryViewModel.kt; open the PointHistoryViewModel file, delete the line
"import
team.aliens.dms.android.data.notification.repository.NotificationRepository",
and run your IDE's "Optimize Imports" (or ktlint/gradle format) to ensure no
other unused imports remain.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.kt:
- Line 63: The permission usage in SelectProfileScreen (the
rememberPermissionState call assigning request) uses
Manifest.permission.READ_MEDIA_IMAGES which exists only on API 33+; update the
permission selection to check Build.VERSION.SDK_INT >=
Build.VERSION_CODES.TIRAMISU and pass READ_MEDIA_IMAGES for API 33+, otherwise
pass Manifest.permission.READ_EXTERNAL_STORAGE for pre-33 devices, ensuring the
rememberPermissionState call (and any permission request flow) uses the chosen
permission string so older devices fall back correctly.
- Around line 71-75: The side-effect name
SelectProfileSideEffect.ProfileImageBadRequest is misleading because it triggers
a success snackbar; rename this variant to something like
ProfileImageUploadSuccess and update all usages (the when branch in
SelectProfileScreen.kt where ProfileImageBadRequest is matched, the sealed
class/enum declaration for SelectProfileSideEffect, any places that emit this
side effect in ViewModel or repository code, and any tests or handlers that
reference the old name) so the variant and its handling consistently reflect a
successful upload.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/AdjustProfileViewModel.kt:
- Around line 17-26: The editProfile function in AdjustProfileViewModel lacks a
loading guard and state toggling, so add a loading flag to the UI state (e.g.,
isLoading) and update editProfile to first check and return if already loading,
then set isLoading = true (and set buttonEnabled = false) before invoking
studentRepository.editProfile, and ensure you set isLoading = false (and restore
buttonEnabled appropriately) in a finally block after the runCatching so the
button is disabled during the request and re-enabled only when appropriate;
update sendEffect calls unchanged but rely on the state changes via setState to
drive the UI.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt:
- Around line 86-107: In fetchPresignedUrl, you currently pass file.toString()
to uploadFile instead of getting a presigned URL; modify the implementation to
first call fileRepository.fetchPresignedUrl(file.name) (or appropriate filename
getter) inside the coroutine, then pass the obtained presignedUrl to
fileRepository.uploadFile(presignedUrl = fetchedUrl, file = file), keeping the
existing runCatching/onSuccess/onFailure flow and updating the onSuccess
handling to use the upload response (fileUrl.fileUrl) and emit the same setState
and sendEffect as before.
- Around line 30-54: getUriImage currently queries MediaStore on the main thread
and calls setState inside the cursor loop; move the MediaStore query and cursor
iteration off the main thread (use a coroutine / withContext(Dispatchers.IO) or
another background dispatcher) to avoid UI jank, accumulate uriList while
iterating the cursor on the background thread, then call setState a single time
after the loop to update the ViewModel state; keep using cursor?.use { ... }
inside the background block and reference getUriImage and setState when applying
this change.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/CheckPasswordViewModel.kt:
- Around line 15-28: The method resetPassword() is misnamed and hides actual
error details; rename it to checkPassword() or verifyCurrentPassword() wherever
referenced (e.g., ViewModel method and any call sites) and update
documentation/tests accordingly, and change the onFailure branch to pass the
real error message from the repository into
sendEffect(CheckPasswordSideEffect.FailCheckPassword(...)) instead of the
hardcoded "๋น๋ฐ๋ฒํธ๊ฐ ์ผ์นํ์ง ์์์" (e.g., use the Throwable.message or a mapped
user-friendly message derived from the Throwable) while keeping the existing
state updates in setState.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt:
- Around line 35-43: The current code only calls
userRepository.comparePassword(...) which only validates the password; replace
or extend this flow to call userRepository.editPassword(currentPassword =
uiState.value.currentPassword, newPassword = uiState.value.newPassword) to
persist the change (or call editPassword after comparePassword succeeds), await
its result, and onSuccess setState { it.copy(isLoading = false, buttonEnabled =
true) } and sendEffect(ResetPasswordSideEffect.SuccessResetPassword), while
onFailure setState { it.copy(isLoading = false, buttonEnabled = true) } and
sendEffect(ResetPasswordSideEffect.FailResetPassword("๋น๋ฐ๋ฒํธ ๋ณ๊ฒฝ์ ์คํจํ์ด์")) so the
UI reflects the real backend update.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt:
- Around line 120-124: The DmsItemButton for "ํ๋กํ ์ฌ์ง ๋ณ๊ฒฝ" is using the calendar
icon R.drawable.img_calendar; update the iconRes on that DmsItemButton (the one
with text = "ํ๋กํ ์ฌ์ง ๋ณ๊ฒฝ" and onClick = onNavigateSelectProfile) to a
profile-appropriate drawable (e.g., a profile or camera resource such as
R.drawable.img_profile or R.drawable.ic_camera) so the icon matches the action.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt:
- Around line 49-63: In fetchNotificationStatus(), avoid calling .toString() on
the nullable uiState.value.deviceToken which turns null into the literal "null"
and breaks the API; instead bail out early if deviceToken is null (or emit an
appropriate effect) before calling
notificationRepository.fetchNotificationStatus, e.g. obtain val token =
uiState.value.deviceToken ?: run { setState { it.copy(isOnNotification = false)
}; sendEffect(SettingSideEffect.CannotFetchNotificationStatus); return }, then
use token when calling notificationRepository.fetchNotificationStatus and keep
the existing success/failure handling with setState and sendEffect.
- Around line 41-47: The signOut() coroutine only handles the success branch and
ignores failures; update signOut() to handle the failure case from
authRepository.signOut() by adding an onFailure { } callback that calls
sendEffect with a new side effect (e.g., SettingSideEffect.SignOutFailure or a
variant that carries an error message/Throwable) so the UI can react; ensure you
reference the existing sendEffect function and SettingSideEffect sealed
class/enum and include any error info or a user-friendly message when emitting
the failure side effect.
- Around line 65-70: The updateNotificationStatus function's parameter and logic
are inverted: rename the parameter (e.g., currentIsOn -> previousIsOn or
desiredIsOn -> newIsOn) to reflect intent, update the setState call in
SettingViewModel.updateNotificationStatus to set isOnNotification to the new
desired value (do not unconditionally negate the incoming parameter), and invert
the conditional so that when turning notifications on you perform the subscribe
flow and when turning off you perform the unsubscribe flow by calling
setNotificationStatus appropriately (adjust setNotificationStatus to accept an
explicit desired state or add separate subscribe/unsubscribe helpers) instead of
the current if (isOnNotification) ... else return pattern.
In @gradle/libs.versions.toml:
- Line 5: Update the library versions in gradle/libs.versions.toml: change the
accompanist entry from "0.36.0" to "0.37.3" and update the telephoto entry from
"0.14.0" to "0.18.0" so the project uses the latest stable releases; locate and
modify the accompanist and telephoto keys in the TOML file and ensure any
dependency usages referencing those version variables continue to resolve
correctly (adjust lockfiles or run a Gradle sync/build to apply the changes).
In
@network/src/dev/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSource.kt:
- Line 9: The prod flavor still uses the old signature returning Unit; update
the abstract declaration abstract suspend fun uploadFile(...) in
NetworkFileDataSource to return FetchFileUrlResponse (matching the dev flavor)
and change the implementation NetworkFileDataSourceImpl.uploadFile to have an
explicit return type of FetchFileUrlResponse and return the appropriate
FetchFileUrlResponse value from the method body so both abstract and concrete
match.
In
@network/src/dev/kotlin/team.aliens.dms.android.network/file/model/FetchFileUrlResponse.kt:
- Around line 5-7: The model FetchFileUrlResponse assumes fileUrl is always
present and non-null which will cause Gson deserialization to throw and crash if
the API omits or nulls "file_url"; either make the property defensive (e.g.,
change FetchFileUrlResponse.fileUrl to a nullable type or provide a safe default
and validate empty strings) and enforce validation where the response is
consumed, or update the network error handling in handleNetworkRequest to catch
JSON parse errors (e.g., JsonSyntaxException/JsonIOException) and surface a
clear error instead of letting the app crash; reference
FetchFileUrlResponse.fileUrl and handleNetworkRequest when implementing these
changes.
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
feature/build.gradle.kts (1)
74-96: Update telephoto-zoomable-image-coil to latest version and consolidate shared dependencies.
telephoto-zoomable-image-coil (line 96): Currently v0.14.0, but v0.18.0 (released Oct 4, 2025) is available. Update to the latest version.
kotlinx.datetime (line 94): Duplication detectedโalready declared in
data/build.gradle.kts. Move shared dependencies to a higher-level configuration (app or shared build file) rather than duplicating across modules.accompanist.permissions (line 95): No single official Compose replacement exists. If migrating away from Accompanist, use Activity Result APIs with
rememberLauncherForActivityResultandActivityResultContractsinstead.
๐ Major comments (18)
gradle/libs.versions.toml-5-5 (1)
5-5: Update accompanist and telephoto to their latest versions.
accompanistshould be updated from0.36.0(Sep 2024) to0.37.3(Apr 2025)telephotoshould be updated from0.14.0to0.18.0The library selections are appropriate for the MyPage profile image features, but the versions are outdated. Update to the latest stable versions to ensure you have the latest bug fixes, security patches, and features.
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/AdjustProfileViewModel.kt-17-26 (1)
17-26: Add loading state to prevent duplicate submissions.The function doesn't track loading state, allowing users to tap the button multiple times before the API call completes, potentially causing duplicate profile update requests.
๐ Proposed fix to add loading state
internal fun editProfile(profileImageUrl: String) = viewModelScope.launch(Dispatchers.IO) { + setState { it.copy(buttonEnabled = false) } runCatching { studentRepository.editProfile(profileImageUrl) }.onSuccess { sendEffect(AdjustProfileSideEffect.ProfileImageSet) - setState { it.copy(buttonEnabled = false) } }.onFailure { sendEffect(AdjustProfileSideEffect.ProfileImageBadRequest) + setState { it.copy(buttonEnabled = true) } } }feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/viewmodel/MyPageViewModel.kt-17-28 (1)
17-28: Add error handling and loading state management.The current implementation has two reliability concerns that degrade user experience:
Empty error handling (lines 25-26): The
onFailureblock silently swallows all errors without logging, user notification, or state updates. Users won't receive feedback when data fetching fails.Missing loading state: The UI cannot display loading indicators because
MyPageStatedoesn't track the loading status.Additionally, consider moving
Dispatchers.IOto the repository layer, as ViewModels should remain dispatcher-agnostic and let the data layer handle threading.๐ Proposed improvements
First, update
MyPageStateto include loading and error states:internal data class MyPageState( val myPage: MyPage = MyPage(), + val isLoading: Boolean = false, + val error: String? = null, )Then update the
getMyPagemethod to properly manage state:private fun getMyPage() { + setState { it.copy(isLoading = true, error = null) } - viewModelScope.launch(Dispatchers.IO) { + viewModelScope.launch { studentRepository.fetchMyPage().onSuccess { myPage -> - setState { it.copy(myPage = myPage) } + setState { it.copy(myPage = myPage, isLoading = false) } }.onFailure { + setState { it.copy(isLoading = false, error = it.message) } } } }Ensure the repository handles
Dispatchers.IOinternally for its suspend functions.Committable suggestion skipped: line range outside the PR's diff.
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt-41-47 (1)
41-47: Add error handling for sign-out failures.The
signOut()function only handles the success case. If the sign-out operation fails, no side effect is emitted, leaving the UI unaware of the failure. This could result in a poor user experience where the app appears unresponsive or stuck.๐ Proposed fix
internal fun signOut() { viewModelScope.launch(Dispatchers.IO) { authRepository.signOut().onSuccess { sendEffect(SettingSideEffect.SignOutSuccess) + }.onFailure { + sendEffect(SettingSideEffect.SignOutFailure) } } }Then add the corresponding side effect:
sealed class SettingSideEffect { object CannotFetchNotificationStatus : SettingSideEffect() object SignOutSuccess : SettingSideEffect() + object SignOutFailure : SettingSideEffect() }Committable suggestion skipped: line range outside the PR's diff.
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt-65-70 (1)
65-70: Confusing logic and parameter naming in notification status update.The function has several issues that make the logic hard to follow:
- Parameter naming:
isOnNotificationsuggests it's the new desired state, but the logic treats it as the current state.- State update on line 67: Negates the current value regardless of intended behavior.
- Line 69: The conditional logic is inverted and unclearโ
if (isOnNotification)triggers unsubscribe, but thenelse returnprevents subscription when turning notifications on.This likely results in incorrect behavior when toggling notification status.
๐ Proposed fix
Clarify the parameter name and fix the logic:
- internal fun updateNotificationStatus(isOnNotification: Boolean) { + internal fun updateNotificationStatus(currentIsOn: Boolean) { setState { settingState -> - settingState.copy(isOnNotification = !isOnNotification) + settingState.copy(isOnNotification = !currentIsOn) } - if (isOnNotification) /* ๊ตฌ๋ ์ทจ์ */ setNotificationStatus() else return // TODO ๊ตฌ๋ ์ ๋ฐ์ดํธ (false -> true) + if (currentIsOn) { + // ๊ตฌ๋ ์ทจ์ (currently on โ turning off) + setNotificationStatus(shouldSubscribe = false) + } else { + // ๊ตฌ๋ ํ์ฑํ (currently off โ turning on) + setNotificationStatus(shouldSubscribe = true) // TODO: Implement subscription + } }Update the placeholder function signature accordingly:
- private fun setNotificationStatus() { + private fun setNotificationStatus(shouldSubscribe: Boolean) { + // TODO: Implement subscription/unsubscription logic }Committable suggestion skipped: line range outside the PR's diff.
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt-30-54 (1)
30-54: Performance: Move MediaStore query off main thread and batch state update.Two issues:
- Main thread blocking: The MediaStore query runs on the main thread during
init, which can cause UI jank.- Inefficient state updates:
setStateis called inside the loop (line 51), triggering multiple state updates. Call it once after the loop.๐ Proposed fix
- private fun getUriImage() { - val uriList = mutableListOf<String>() - val contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI - val projection = arrayOf(MediaStore.Images.Media._ID) - - - // contentResolver๋ฅผ ์ฌ์ฉํ์ฌ ์ธ๋ถ ์ ์ฅ์์์ ์ด๋ฏธ์ง ๋ฐ์ดํฐ ์ฟผ๋ฆฌ - val cursor = context.contentResolver.query( - contentUri, - projection, - null, - null, - "${MediaStore.Images.Media.DATE_ADDED} DESC" - ) - - cursor?.use { - val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID) - while (cursor.moveToNext()) { - val id = cursor.getLong(idColumn) - val imageUri = ContentUris.withAppendedId(contentUri, id) - uriList.add(imageUri.toString()) - setState { it.copy(uriList = uriList) } + private fun getUriImage() { + viewModelScope.launch(Dispatchers.IO) { + val uriList = mutableListOf<String>() + val contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI + val projection = arrayOf(MediaStore.Images.Media._ID) + + val cursor = context.contentResolver.query( + contentUri, + projection, + null, + null, + "${MediaStore.Images.Media.DATE_ADDED} DESC" + ) + + cursor?.use { + val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID) + while (cursor.moveToNext()) { + val id = cursor.getLong(idColumn) + val imageUri = ContentUris.withAppendedId(contentUri, id) + uriList.add(imageUri.toString()) + } } + setState { it.copy(uriList = uriList) } } }feature/src/dev/kotlin/team/aliens/dms/android/feature/point/viewmodel/PointHistoryViewModel.kt-3-3 (1)
3-3: Remove unused import.
SavedStateHandleis imported but not used anywhere in the file.๐ Proposed fix
-import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.viewModelScopefeature/src/dev/kotlin/team/aliens/dms/android/feature/point/viewmodel/PointHistoryViewModel.kt-8-8 (1)
8-8: Remove unused import.
NotificationRepositoryis imported but not used anywhere in the file.๐ Proposed fix
import team.aliens.dms.android.core.ui.viewmodel.BaseStateViewModel -import team.aliens.dms.android.data.notification.repository.NotificationRepository import team.aliens.dms.android.data.point.model.Pointapp/src/dev/AndroidManifest.xml-10-10 (1)
10-10: Add backward compatibility for READ_EXTERNAL_STORAGE on Android 12 and below.The runtime permission request for
READ_MEDIA_IMAGESis properly implemented inSelectProfileScreen.ktusing the Accompanist library. However, the code lacks backward compatibility handling. TheREAD_MEDIA_IMAGESpermission was introduced in Android 13 (API 33) and does not exist on lower versions.To fix this:
- Check
Build.VERSION.SDK_INTbefore requesting permissions- Request
READ_MEDIA_IMAGESfor Android 13+ andREAD_EXTERNAL_STORAGEfor Android 12 and below- Add
READ_EXTERNAL_STORAGEtoAndroidManifest.xmlfor devices running older Android versionsThe codebase already uses this pattern elsewhere (e.g., in
SignInScreen.ktfor notifications), so follow the same approach for media permissions.feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.kt-63-63 (1)
63-63: Permission compatibility issue:READ_MEDIA_IMAGESrequires API 33+.
Manifest.permission.READ_MEDIA_IMAGESis only available on Android 13 (API 33) and above. On older devices, this will fail silently or crash. Consider handling the API level check to fall back toREAD_EXTERNAL_STORAGEfor pre-33 devices.๐ Proposed fix
+import android.os.Build + @OptIn(ExperimentalPermissionsApi::class) @Composable internal fun SelectProfile( onBackPressed: () -> Unit, onNavigateAdjustProfile: (String) -> Unit, onShowSnackBar: (DmsSnackBarType, String) -> Unit, ) { - val request = rememberPermissionState(permission = Manifest.permission.READ_MEDIA_IMAGES) + val permission = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + Manifest.permission.READ_MEDIA_IMAGES + } else { + Manifest.permission.READ_EXTERNAL_STORAGE + } + val request = rememberPermissionState(permission = permission)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/PointCards.kt-56-60 (1)
56-60: HandlePointType.ALLexplicitly in the when expression.The
PointTypeenum includesALL,BONUS, andMINUS. Currently,ALLfalls into theelsebranch, which returns empty text and default colors that may cause UI issues. Add explicit handling forALLwith appropriate text and color styling.val (text, textColor, backgroundColor) = when (pointType) { PointType.BONUS -> Triple("์์ ", DmsTheme.colorScheme.secondary, DmsTheme.colorScheme.primary) PointType.MINUS -> Triple("๋ฒ์ ", DmsTheme.colorScheme.onErrorContainer, DmsTheme.colorScheme.error) else -> Triple("", DmsTheme.colorScheme.onSurface, DmsTheme.colorScheme.surface) }app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt-279-287 (1)
279-287: Same fragile pattern forAdjustProfileScreenNav.Use the entry parameter directly instead of reconstructing the nav key.
๐ Proposed fix
entry<AdjustProfileScreenNav> { AdjustProfileRoute( - onBackPressed = { backStack.remove(AdjustProfileScreenNav(it.model)) }, + onBackPressed = { backStack.remove(it) }, model = it.model, onShowSnackBar = { snackBar, message -> appState.showSnackBar(snackBar, message) } ) }feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt-100-106 (1)
100-106: Incorrectrememberkeys cause stale data.The
rememberblock usesstate.allPointListas the only key, but the computation also depends onstate.bonusPointListandstate.minusPointList. When these lists update independently, the cached value won't recompute.๐ Proposed fix
- val pointHistoryList = remember(page, state.allPointList) { + val pointHistoryList = remember(page, state.allPointList, state.bonusPointList, state.minusPointList) { when (tabData[page]) { PointTab.All -> state.allPointList PointTab.Bonus -> state.bonusPointList PointTab.Minus -> state.minusPointList } }feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt-40-42 (1)
40-42: Remove emptyLaunchedEffectblock.This
LaunchedEffecthas an empty body and serves no purpose. Remove it or add the intended logic.๐ Proposed fix
- LaunchedEffect(Unit) { - - } - PointHistoryScreen(app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt-190-199 (1)
190-199: Fragile back navigation with reconstructed keys.Creating a new
VoteScreenNavinstance for removal relies on exact equality. Use the entry parametervoteNavdirectly:๐ Proposed fix
entry<VoteScreenNav> { voteNav -> VoteRoute( title = voteNav.title, startTime = voteNav.startTime, endTime = voteNav.endTime, - onNavigateBack = { backStack.remove(VoteScreenNav(voteNav.title, voteNav.startTime, voteNav.endTime)) }, + onNavigateBack = { backStack.remove(voteNav) }, onShowSnackBar = { snackBarType, message -> appState.showSnackBar(snackBarType, message) } ) }app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt-244-247 (1)
244-247: Same fragile pattern forPointHistoryScreenNav.๐ Proposed fix
entry<PointHistoryScreenNav> { PointHistoryRoute( - onBackClick = { backStack.remove(PointHistoryScreenNav(it.pointType)) } + onBackClick = { backStack.remove(it) } ) }app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt-69-70 (1)
69-70: EnsurePointTypeis marked as@Serializable.
PointHistoryScreenNavis annotated with@Serializablebut contains apointTypeproperty of typePointType, which is not serializable. This will cause serialization failures at runtime. Add@Serializableto thePointTypeenum definition indata/src/prod/kotlin/team/aliens/dms/android/data/point/model/PointType.ktanddata/src/dev/kotlin/team.aliens.dms.android.data/point/model/PointType.kt.feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/CheckPasswordViewModel.kt-15-28 (1)
15-28: Function nameresetPassword()is misleading.The function verifies the current password by calling
comparePassword, but doesn't actually reset anything. This creates confusion about the function's purpose. Consider renaming tocheckPassword()orverifyCurrentPassword()to accurately reflect its behavior.Additionally, Line 25 hardcodes the error message "๋น๋ฐ๋ฒํธ๊ฐ ์ผ์นํ์ง ์์์", which discards the actual error from the repository. If the repository fails due to network errors, server issues, or other reasons, the user will still see "password doesn't match," which is misleading.
๐ Proposed fixes
- internal fun resetPassword() { + internal fun checkPassword() { viewModelScope.launch { setState { it.copy(isLoading = true, buttonEnabled = false) } userRepository.comparePassword( password = uiState.value.currentPassword, ).onSuccess { setState { it.copy(isLoading = false, buttonEnabled = true) } sendEffect(CheckPasswordSideEffect.SuccessCheckPassword) }.onFailure { setState { it.copy(isLoading = false, buttonEnabled = true) } - sendEffect(CheckPasswordSideEffect.FailCheckPassword("๋น๋ฐ๋ฒํธ๊ฐ ์ผ์นํ์ง ์์์")) + sendEffect(CheckPasswordSideEffect.FailCheckPassword( + it.message ?: "๋น๋ฐ๋ฒํธ ํ์ธ์ ์คํจํ์ต๋๋ค" + )) } } }
๐ก Minor comments (6)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt-7-7 (1)
7-7: Remove unused import.The
navigationBarsPaddingimport is declared but never used in this file. Remove it unless you intended to apply the padding to one of the Modifier chains.network/src/dev/kotlin/team.aliens.dms.android.network/file/model/FetchFileUrlResponse.kt-5-7 (1)
5-7: Verify that the API consistently returnsfile_urlin successful responses and consider defensive deserialization handling.The data class structure is clean and follows Kotlin conventions. The non-nullable
fileUrlproperty assumes the API always provides this field in successful responses. However,handleNetworkRequestonly catches HTTP exceptions and will not catch JSON deserialization failuresโiffile_urlis missing or null in the response, Gson will throw an exception that crashes the app.Ensure the API contract guarantees
file_urlis always present and non-null in successful responses. Additionally, consider whether empty strings should be validated, or add defensive error handling for deserialization failures if the API contract cannot be guaranteed.data/src/dev/kotlin/team.aliens.dms.android.data/point/model/Point.kt-3-3 (1)
3-3: Clean up unused import in PointMapper.kt.The import change from
java.time.LocalDatetokotlinx.datetime.LocalDateis properly implemented. The mapper correctly parses the String date fromFetchPointsResponseusingLocalDate.parse(), and the feature layer (PointItem.kt) handles formatting withkotlinx.datetime.formatutilities. However, line 9 importsteam.aliens.dms.android.shared.date.toLocalDatewhich is unusedโremove it.feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.kt-71-75 (1)
71-75: Confusing side effect naming:ProfileImageBadRequestshows success message.
SelectProfileSideEffect.ProfileImageBadRequesttriggers a success snackbar ("์ ๋ก๋ ์ฑ๊ณต!"). The naming is misleadingโconsider renaming to something likeProfileImageUploadSuccessfor clarity.feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/component/PointItem.kt-47-63 (1)
47-63: UnusedbackgroundColorvariable.The
backgroundColoris destructured from the Triple but never used. TheBoxat lines 56-63 lacks a background modifier. Either apply the background or remove the unused variable.๐ Proposed fix: apply background to the Box
Box( + modifier = Modifier + .size(40.dp) + .clip(CircleShape) + .background(backgroundColor), contentAlignment = Alignment.Center, ) { Image( painter = painterResource(id = icon), contentDescription = null, ) }feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt-120-124 (1)
120-124: Mismatched icon for profile picture change.
R.drawable.img_calendar(calendar icon) is used for "ํ๋กํ ์ฌ์ง ๋ณ๊ฒฝ" (profile picture change). This seems unintentionalโconsider using a profile or camera-related icon.
| titleContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | ||
| textContentColor: Color = DmsTheme.colorScheme.surfaceContainer, |
There was a problem hiding this comment.
Fix incorrect content color values.
surfaceContainer is a surface/background color, not a content color. Using it for titleContentColor and textContentColor will likely result in poor contrast or invisible text. Material3 content colors should use "on" variants like onSurface or onSurfaceVariant.
๐ Proposed fix
- titleContentColor: Color = DmsTheme.colorScheme.surfaceContainer,
- textContentColor: Color = DmsTheme.colorScheme.surfaceContainer,
+ titleContentColor: Color = DmsTheme.colorScheme.onSurface,
+ textContentColor: Color = DmsTheme.colorScheme.onSurfaceVariant,๐ 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.
| titleContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | |
| textContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | |
| titleContentColor: Color = DmsTheme.colorScheme.onSurface, | |
| textContentColor: Color = DmsTheme.colorScheme.onSurfaceVariant, |
๐ค Prompt for AI Agents
In
@core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/dialog/AlertDialog.kt
around lines 25-26, The titleContentColor and textContentColor default to
DmsTheme.colorScheme.surfaceContainer which is a background color; change their
defaults to appropriate "on" content colors (e.g.,
DmsTheme.colorScheme.onSurface or DmsTheme.colorScheme.onSurfaceVariant) so
text/heading use accessible contrast. Update the default values for
titleContentColor and textContentColor in AlertDialog.kt to use the
onSurface/onSurfaceVariant variants from DmsTheme.colorScheme.
data/src/dev/kotlin/team.aliens.dms.android.data/user/repository/UserRepository.kt
Show resolved
Hide resolved
...re/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/OptionsContent.kt
Outdated
Show resolved
Hide resolved
...dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt
Outdated
Show resolved
Hide resolved
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt
Show resolved
Hide resolved
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt
Show resolved
Hide resolved
| abstract class NetworkFileDataSource { | ||
| abstract suspend fun fetchPresignedUrl(fileName: String): FetchPresignedUrlResponse | ||
| abstract suspend fun uploadFile(presignedUrl: String, file: File) | ||
| abstract suspend fun uploadFile(presignedUrl: String, file: File): FetchFileUrlResponse |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Find all implementations of NetworkFileDataSource
echo "=== Searching for NetworkFileDataSource implementations ==="
rg -n 'class.*:\s*NetworkFileDataSource' --type=kt
echo -e "\n=== Looking for uploadFile implementations ==="
rg -n 'fun uploadFile' --type=kt -B2 -A2
echo -e "\n=== Checking the abstract class definition ==="
find . -name "NetworkFileDataSource.kt" -type f | head -5 | xargs -I {} cat -n {}Repository: team-aliens/DMS-Android
Length of output: 1225
๐ Script executed:
#!/bin/bash
# Find all implementations of NetworkFileDataSource without type filter
echo "=== Searching for all classes that implement/extend NetworkFileDataSource ==="
rg 'class.*NetworkFileDataSource' -g '*.kt'
echo -e "\n=== Finding all uploadFile implementations ==="
rg 'fun uploadFile' -g '*.kt' -B3 -A5
echo -e "\n=== List all .kt files in the file/datasource directory ==="
find . -path '*/file/datasource/*.kt' -type f
echo -e "\n=== Search for any class extending NetworkFileDataSource ==="
rg ': NetworkFileDataSource' -g '*.kt' -B2 -A10Repository: team-aliens/DMS-Android
Length of output: 17742
๐ Script executed:
#!/bin/bash
# View the complete prod NetworkFileDataSourceImpl to see the full uploadFile signature
echo "=== Prod NetworkFileDataSourceImpl.kt complete content ==="
cat -n network/src/prod/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSourceImpl.kt
echo -e "\n=== Dev NetworkFileDataSourceImpl.kt complete content ==="
cat -n network/src/dev/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSourceImpl.ktRepository: team-aliens/DMS-Android
Length of output: 3320
Update the prod flavor's NetworkFileDataSource to match the new return type.
The concern is valid. While the dev flavor has been updated to return FetchFileUrlResponse, the prod flavor's abstract class and implementation still use the old signature returning Unit. Both the abstract class definition and NetworkFileDataSourceImpl in the prod flavor must be updated to match:
network/src/prod/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSource.ktline 8: Change signature to returnFetchFileUrlResponsenetwork/src/prod/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSourceImpl.ktline 22-23: Add explicit return typeFetchFileUrlResponse
This inconsistency between build flavors will cause compilation or type-mismatch issues.
๐ค Prompt for AI Agents
In
@network/src/dev/kotlin/team.aliens.dms.android.network/file/datasource/NetworkFileDataSource.kt
around line 9, The prod flavor still uses the old signature returning Unit;
update the abstract declaration abstract suspend fun uploadFile(...) in
NetworkFileDataSource to return FetchFileUrlResponse (matching the dev flavor)
and change the implementation NetworkFileDataSourceImpl.uploadFile to have an
explicit return type of FetchFileUrlResponse and return the appropriate
FetchFileUrlResponse value from the method body so both abstract and concrete
match.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
๋ง์ดํ์ด์ง ๊ตฌํ์ ์ํ ๋๊ท๋ชจ ์ ๋ฐ์ดํธ ์ ๋ณด์์ต๋๋ค. ์ ๋ฐ์ ์ผ๋ก ์๋ก์ด ๊ธฐ๋ฅ๋ค์ด ์ ํตํฉ๋๊ณ ์๋ ๊ฒ ๊ฐ์ต๋๋ค. ๋ช ๊ฐ์ง ์น๋ช ์ ์ธ ๋ฒ๊ทธ์ ๊ฐ์ ์ ์ ๋ฐ๊ฒฌํ์ฌ ๋ฆฌ๋ทฐ์ ๋จ๊ฒผ์ต๋๋ค. ํนํ ํ์ผ ์ ๋ก๋ ๋ก์ง๊ณผ ๋น๋ฐ๋ฒํธ ์ฌ์ค์ ๋ก์ง์ ์น๋ช ์ ์ธ ์ค๋ฅ๊ฐ ์์ผ๋ ๊ผญ ํ์ธ ๋ถํ๋๋ฆฝ๋๋ค. ๋ํ, ์๋๋ก์ด๋ ํ์ ๋ฒ์ ํธํ์ฑ์ ์ํ ๊ถํ ์ฒ๋ฆฌ์ ์ผ๋ถ ํ๋ฉด์ ๋ค๋น๊ฒ์ด์ ๋ฐ ํจ๋ฉ ๋ฌธ์ ๋ ์์ ์ด ํ์ํด ๋ณด์ ๋๋ค. ์ถ๊ฐ์ ์ผ๋ก ์ฌ์ฉ๋์ง ์๋ ์ปดํฌ๋ํธ๋ค์ด ์์ผ๋ ํ์ธ ํ ์ ๋ฆฌํด์ฃผ์๋ฉด ์ข๊ฒ ์ต๋๋ค.
| internal fun fetchPresignedUrl( | ||
| file: File, | ||
| ) { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| fileRepository.uploadFile( | ||
| presignedUrl = file.toString(), | ||
| file = file, | ||
| ) | ||
| }.onSuccess { fileUrl -> | ||
| setState { | ||
| it.copy( | ||
| profileImageUrl = fileUrl.fileUrl, | ||
| buttonEnabled = true, | ||
| ) | ||
| } | ||
| sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl.fileUrl)) | ||
| }.onFailure { | ||
| sendEffect(SelectProfileSideEffect.FailProfileImage) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ํ๋กํ ์ด๋ฏธ์ง ์
๋ก๋ ๋ก์ง์ ์ฌ๊ฐํ ์ค๋ฅ๊ฐ ์์ต๋๋ค. fileRepository.uploadFile์ ํธ์ถํ ๋ presignedUrl ํ๋ผ๋ฏธํฐ์ file.toString()์ ์ ๋ฌํ๊ณ ์์ต๋๋ค. file.toString()์ ํ์ผ ๊ฒฝ๋ก ๋ฌธ์์ด์ผ ๋ฟ, ์ ํจํ ์
๋ก๋ URL์ด ์๋๋๋ค. ๋จผ์ fileRepository.fetchPresignedUrl()์ ํธ์ถํ์ฌ ์
๋ก๋์ ํ์ํ Presigned URL์ ๋ฐ์์จ ํ, ๊ทธ URL์ ์ฌ์ฉํ์ฌ uploadFile์ ํธ์ถํด์ผ ํฉ๋๋ค. ๋ํ, ํ์ฌ fetchPresignedUrl์ด๋ผ๋ ํจ์๋ช
์ ์ค์ ๋ก๋ ํ์ผ ์
๋ก๋๋ฅผ ์๋ํ๊ณ ์์ด ํผ๋์ ์ค๋๋ค. ํจ์๋ช
์ uploadFile ๋ฑ์ผ๋ก ๋ช
ํํ๊ฒ ๋ณ๊ฒฝํ๊ณ , ๋ด๋ถ ๋ก์ง์ ์์ ํด์ผ ํฉ๋๋ค.
internal fun uploadFileAndGetUrl(
file: File,
) {
viewModelScope.launch(Dispatchers.IO) {
runCatching {
val presignedInfo = fileRepository.fetchPresignedUrl(file.name)
fileRepository.uploadFile(
presignedUrl = presignedInfo.fileUploadUrl,
file = file,
)
presignedInfo.fileUrl
}.onSuccess { fileUrl ->
setState {
it.copy(
profileImageUrl = fileUrl,
buttonEnabled = true,
)
}
sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl))
}.onFailure {
sendEffect(SelectProfileSideEffect.FailProfileImage)
}
}
}| internal fun resetPassword() { | ||
| viewModelScope.launch { | ||
| setState { it.copy(isLoading = true, buttonEnabled = false) } | ||
| userRepository.comparePassword( | ||
| password = uiState.value.newPassword, | ||
| ).onSuccess { | ||
| setState { it.copy(isLoading = false, buttonEnabled = true) } | ||
| sendEffect(ResetPasswordSideEffect.SuccessResetPassword) | ||
| }.onFailure { | ||
| setState { it.copy(isLoading = false, buttonEnabled = true) } | ||
| sendEffect(ResetPasswordSideEffect.FailResetPassword("๋น๋ฐ๋ฒํธ ๋ณ๊ฒฝ์ ์คํจํ์ด์")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
resetPassword ํจ์์์ userRepository.comparePassword๋ฅผ ํธ์ถํ๊ณ ์์ต๋๋ค. ์ด ํ๋ฉด์ ๋น๋ฐ๋ฒํธ๋ฅผ '์ฌ์ค์ 'ํ๋ ํ๋ฉด์ด๋ฏ๋ก, comparePassword๊ฐ ์๋ editPassword์ ๊ฐ์ ๋น๋ฐ๋ฒํธ ๋ณ๊ฒฝ API๋ฅผ ํธ์ถํด์ผ ํฉ๋๋ค. ํ์ฌ ๋ก์ง์ ์ ๋น๋ฐ๋ฒํธ๋ฅผ ๊ธฐ์กด ๋น๋ฐ๋ฒํธ์ ๋น๊ตํ๋ ๊ฒ์ผ๋ก ๋ณด์ฌ, ๊ธฐ๋ฅ ๋ช
์ธ์ ๋ง์ง ์์ต๋๋ค. editPassword๋ฅผ ์ฌ์ฉํ๋๋ก ์์ ํด์ฃผ์ธ์.
| <uses-permission android:name="android.permission.CAMERA" /> | ||
| <uses-permission android:name="android.permission.VIBRATE" /> | ||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> |
There was a problem hiding this comment.
READ_MEDIA_IMAGES ๊ถํ์ API 33 ์ด์์์๋ง ์ ํจํฉ๋๋ค. minSdk๊ฐ 26์ผ๋ก ์ค์ ๋์ด ์์ผ๋ฏ๋ก, ํ์ ๋ฒ์ (API 32 ์ดํ)์ ์ง์ํ๊ธฐ ์ํด READ_EXTERNAL_STORAGE ๊ถํ๋ ์ถ๊ฐํด์ผ ํฉ๋๋ค. maxSdkVersion="32" ์์ฑ๊ณผ ํจ๊ป ์ถ๊ฐํ์ฌ ๋ฒ์ ๋ณ๋ก ์ฌ๋ฐ๋ฅธ ๊ถํ์ด ์ฌ์ฉ๋๋๋ก ํ๋ ๊ฒ์ด ์ข์ต๋๋ค. ๋ํ, SelectProfileScreen.kt์์ ๊ถํ์ ์์ฒญํ ๋๋ Build.VERSION.SDK_INT์ ๋ฐ๋ผ ๋ถ๊ธฐ ์ฒ๋ฆฌ๊ฐ ํ์ํฉ๋๋ค.
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> | |
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" android:maxSdkVersion="32" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> |
| titleContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | ||
| textContentColor: Color = DmsTheme.colorScheme.surfaceContainer, |
There was a problem hiding this comment.
AlertDialog์ titleContentColor์ textContentColor์ ๊ธฐ๋ณธ๊ฐ์ผ๋ก DmsTheme.colorScheme.surfaceContainer๋ฅผ ์ฌ์ฉํ๊ณ ์์ต๋๋ค. surfaceContainer๋ ๋ฐฐ๊ฒฝ์์ผ๋ก ์ฌ์ฉ๋๋ ์์์ด๋ฏ๋ก, ํ
์คํธ ์์์ผ๋ก ์ฌ์ฉํ๋ฉด ๋ฐฐ๊ฒฝ๊ณผ ์์ด ๋น์ทํด์ ธ ํ
์คํธ๊ฐ ๋ณด์ด์ง ์์ ์ ์์ต๋๋ค. Material3 ๋์์ธ ๊ฐ์ด๋์ ๋ฐ๋ผ DmsTheme.colorScheme.onSurface๋ DmsTheme.colorScheme.onSurfaceVariant์ ๊ฐ์ 'on' ๊ณ์ด ์์์ ์ฌ์ฉํ๋ ๊ฒ์ด ์ ์ ํฉ๋๋ค.
| titleContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | |
| textContentColor: Color = DmsTheme.colorScheme.surfaceContainer, | |
| titleContentColor: Color = DmsTheme.colorScheme.onSurface, | |
| textContentColor: Color = DmsTheme.colorScheme.onSurfaceVariant, |
| Column( | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) { |
There was a problem hiding this comment.
ํ๋ฉด์ ๋ฃจํธ Column์ Modifier.navigationBarsPadding()์ด ์ ์ฉ๋์ง ์์์ต๋๋ค. ์ ์ค์ฒ ๋ค๋น๊ฒ์ด์
์ ์ฌ์ฉํ๋ ๊ธฐ๊ธฐ์์๋ ํ๋จ UI ์์๋ค์ด ์์คํ
๋ค๋น๊ฒ์ด์
๋ฐ์ ๊ฐ๋ ค์ง ์ ์์ต๋๋ค. DmsApp์์ ์ ์ญ์ผ๋ก ์ฒ๋ฆฌํ๋ navigationBarsPadding์ด ์ ๊ฑฐ๋์์ผ๋ฏ๋ก, ์ด์ ๊ฐ ํ๋ฉด์์ ๊ฐ๋ณ์ ์ผ๋ก ์ฒ๋ฆฌํด์ผ ํฉ๋๋ค. ํ๋จ์ UI ์์๊ฐ ์๋ ๋ชจ๋ ์ ํ๋ฉด์ ์ด ํจ๋ฉ์ ์ถ๊ฐํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.
| Column( | |
| modifier = Modifier.fillMaxSize(), | |
| ) { | |
| Column( | |
| modifier = Modifier.fillMaxSize().navigationBarsPadding(), | |
| ) { |
|
|
||
| private fun fetchNotificationStatus() { | ||
| viewModelScope.launch { | ||
| notificationRepository.fetchNotificationStatus(uiState.value.deviceToken.toString()).onSuccess { statuses -> |
There was a problem hiding this comment.
deviceToken์ด null์ผ ๊ฒฝ์ฐ uiState.value.deviceToken.toString() ํธ์ถ ์ NullPointerException์ด ๋ฐ์ํ ์ ์์ต๋๋ค. fetchDeviceToken()์ด ๋น๋๊ธฐ๋ก ๋์ํ๋ฏ๋ก fetchNotificationStatus()๊ฐ ํธ์ถ๋๋ ์์ ์ deviceToken์ด ์์ง ์ค์ ๋์ง ์์์ ์ ์์ต๋๋ค. deviceToken์ด null์ด ์๋ ๋๋ง fetchNotificationStatus๋ฅผ ํธ์ถํ๊ฑฐ๋, deviceToken์ ์์ ํ๊ฒ ์ฒ๋ฆฌํด์ฃผ์ธ์.
val deviceToken = uiState.value.deviceToken ?: return@launch
notificationRepository.fetchNotificationStatus(deviceToken).onSuccess { statuses ->| }.onFailure { | ||
| } |
There was a problem hiding this comment.
studentRepository.fetchMyPage() ํธ์ถ ์ onFailure ๋ธ๋ก์ด ๋น์ด์์ต๋๋ค. ๋คํธ์ํฌ ์์ฒญ ์คํจ ๋ฑ ์์ธ ์ํฉ์ด ๋ฐ์ํ์ ๋ ์ฌ์ฉ์์๊ฒ ์๋ฌด๋ฐ ํผ๋๋ฐฑ์ด ์ ๊ณต๋์ง ์์ผ๋ฉด ์ฑ์ด ๋ฉ์ถ ๊ฒ์ฒ๋ผ ๋ณด์ผ ์ ์์ต๋๋ค. ์คํจ ์ ์ค๋ต๋ฐ๋ฅผ ํตํด ์ฌ์ฉ์์๊ฒ ์ค๋ฅ๋ฅผ ์๋ฆฌ๊ฑฐ๋, ์ต์ํ ๋ก๊ทธ๋ฅผ ๋จ๊ฒจ ๋๋ฒ๊น
์ด ์ฉ์ดํ๋๋ก ์ฒ๋ฆฌํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
}.onFailure { exception ->
// TODO: post side effect to show error message
exception.printStackTrace()
}
...re/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/OptionsContent.kt
Outdated
Show resolved
Hide resolved
| pointRepository.fetchPoints( | ||
| type = PointType.ALL, | ||
| page = null, | ||
| size = null, |
There was a problem hiding this comment.
์๋ฒ์ ๋ด์ญ์ ์กฐํํ ๋ page์ size๋ฅผ null๋ก ์ ๋ฌํ์ฌ ๋ชจ๋ ๋ฐ์ดํฐ๋ฅผ ํ ๋ฒ์ ๊ฐ์ ธ์ค๊ณ ์์ต๋๋ค. ์๋ฒ์ ๋ด์ญ์ด ๋ง์์ง ๊ฒฝ์ฐ ์ฑ ์ฑ๋ฅ ์ ํ ๋ฐ ๊ณผ๋ํ ๋ฉ๋ชจ๋ฆฌ ์ฌ์ฉ์ ์ ๋ฐํ ์ ์์ต๋๋ค. ํ์ด์ง(Paging)์ ๊ตฌํํ์ฌ ์ฌ์ฉ์๊ฐ ์คํฌ๋กคํ ๋๋ง๋ค ํ์ํ ๋งํผ์ ๋ฐ์ดํฐ๋ง ์ ์ง์ ์ผ๋ก ๋ถ๋ฌ์ค๋๋ก ๊ฐ์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.
| pointRepository.fetchPoints( | |
| type = PointType.ALL, | |
| page = null, | |
| size = null, | |
| pointRepository.fetchPoints( | |
| type = PointType.ALL, | |
| page = 1, // TODO: Implement pagination | |
| size = 20, // TODO: Implement pagination | |
| ).onSuccess { pointHistory -> |
| viewModel.sideEffect.collect { | ||
| when (it) { | ||
| is SelectProfileSideEffect.SuccessProfileImage -> onNavigateAdjustProfile(it.profileImageUrl) | ||
| is SelectProfileSideEffect.ProfileImageBadRequest -> onShowSnackBar(DmsSnackBarType.SUCCESS, "์ ๋ก๋ ์ฑ๊ณต!") |
There was a problem hiding this comment.
ProfileImageBadRequest ์ฌ์ด๋ ์ดํํธ๊ฐ ๋ฐ์ํ์ ๋ "์
๋ก๋ ์ฑ๊ณต!"์ด๋ผ๋ ์ฑ๊ณต ์ค๋ต๋ฐ๋ฅผ ๋ณด์ฌ์ฃผ๊ณ ์์ต๋๋ค. ์ด๋ ์ฌ์ฉ์์๊ฒ ํผ๋์ ์ค ์ ์์ต๋๋ค. BadRequest๋ ๋ณดํต ์ค๋ฅ ์ํฉ์ ์๋ฏธํ๋ฏ๋ก, ์ค๋ฅ ์ค๋ต๋ฐ๋ฅผ ํ์ํ๊ณ ๋ฉ์์ง๋ "์๋ชป๋ ์์ฒญ์
๋๋ค." ์ ๊ฐ์ด ์ค๋ฅ ์ํฉ์ ๋ง๊ฒ ์์ ํด์ผ ํฉ๋๋ค.
| is SelectProfileSideEffect.ProfileImageBadRequest -> onShowSnackBar(DmsSnackBarType.SUCCESS, "์ ๋ก๋ ์ฑ๊ณต!") | |
| is SelectProfileSideEffect.ProfileImageBadRequest -> onShowSnackBar(DmsSnackBarType.ERROR, "์ด๋ฏธ์ง๋ฅผ ์ ํํด์ฃผ์ธ์.") |
There was a problem hiding this comment.
Actionable comments posted: 7
๐ค Fix all issues with AI agents
In @app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt:
- Around line 75-76: The ResetPasswordScreenNav data class currently carries
currentPassword which exposes secrets in the navigation back stack; remove the
currentPassword field from ResetPasswordScreenNav and stop passing the password
via NavKey, instead keep the password in a transient, non-navigation holder such
as a shared ViewModel (e.g., a ResetPasswordViewModel) or use the ResultStore to
transfer the value, or require re-validation on the ResetPassword screen; update
any callers that construct ResetPasswordScreenNav and consumers that read it to
use the chosen alternative (shared ViewModel or ResultStore) and ensure no
logging/serialization of the password occurs.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt:
- Around line 84-106: fetchPresignedUrl is called outside the runCatching, so
exceptions from fileRepository.fetchPresignedUrl(file.name) will escape and not
trigger the FailProfileImage side effect; wrap both the fetchPresignedUrl and
the subsequent uploadFile call inside a single runCatching (or a try/catch)
within viewModelScope.launch(Dispatchers.IO) so any exception during fetch or
upload is caught, and onFailure calls
sendEffect(SelectProfileSideEffect.FailProfileImage) (and update state as
needed, e.g., disable button) while onSuccess processes fileUrl as before.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt:
- Line 14: The userRepository property in ResetPasswordViewModel is publicly
exposed; make it private by changing its declaration to a private val so the
injected UserRepository cannot be accessed externally and the class follows
encapsulation best practices (update the ResetPasswordViewModel's userRepository
declaration to private val userRepository: UserRepository).
- Line 3: The file ResetPasswordViewModel.kt currently imports android.util.Log
but never uses it; remove the unused import statement for android.util.Log from
ResetPasswordViewModel.kt to clean up imports and eliminate the unused import
warning.
- Around line 34-56: In resetPassword in ResetPasswordViewModel: validate the
actual newPassword (use uiState.value.newPassword) when calling
checkIfPasswordValid and pass newPassword to userRepository.editPassword rather
than uiState.value.checkNewPassword to make intent clear and ensure youโre
updating to the primary input; also fix the failure message construction in the
onFailure handler to avoid the literal "null" by using a safe fallback (e.g.,
prefer it.cause?.message, then it.message, then a default string) when calling
sendEffect(ResetPasswordSideEffect.FailResetPassword).
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt:
- Around line 103-105: The root Column in SettingScreen (the Composable where
Column is created with modifier = Modifier.fillMaxSize()) can have its bottom
content obscured by system gesture/navigation bars; update the Column modifier
to include navigationBarsPadding by combining Modifier.fillMaxSize() with
Modifier.navigationBarsPadding() (e.g.,
Modifier.fillMaxSize().navigationBarsPadding()) so the UI respects system
navigation insets.
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt:
- Around line 65-70: The updateNotificationStatus function currently flips UI
state optimistically using the incoming isOnNotification parameter, checks the
old value, and only calls a placeholder setNotificationStatus() for unsubscribe;
fix by making the UI update reflect the result of a backend operation (or
disable the toggle until backend exists): in updateNotificationStatus, determine
the intended new state (newValue = !isOnNotification), call an implemented async
operation that handles both subscribe and unsubscribe (implement
subscribe/unsubscribe logic inside setNotificationStatus or split into
subscribeToNotifications / unsubscribeFromNotifications), await the backend
response, only then update state via setState(settingState ->
settingState.copy(isOnNotification = newValue)) and on failure revert state and
surface an error; if backend integration is unavailable, disable the toggle path
by preventing state change and returning an error message instead of
optimistically toggling.
๐งน Nitpick comments (12)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/viewmodel/MyPageViewModel.kt (1)
21-29: Consider logging the exception for debugging purposes.The error handling now properly sends a side effect to notify the user, which is good. However, logging the exception would help with debugging production issues.
๐ Proposed improvement
}.onFailure { + it.printStackTrace() // or use a logging framework sendEffect(MyPageSideEffect.FailFetchMyPage("๋ง์ดํ์ด์ง ์ ๋ณด ์กฐํ๋ฅผ ์คํจํ์ด์")) }feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/CheckPasswordScreen.kt (2)
43-52: Potential stale state capture in side effect handler.At Line 46,
state.currentPasswordis captured when the side effect is emitted, butstateis obtained outside thecollectblock. If the password changes between emission and navigation, the stale value could be passed. Consider passing the password directly from the side effect:is CheckPasswordSideEffect.SuccessCheckPassword -> onNavigateResetPassword(it.password)This requires updating
SuccessCheckPasswordto carry the validated password.
89-99: Consider settingkeyboardType = KeyboardType.Passwordfor the password field.The password text field doesn't specify a
keyboardType. SettingkeyboardType = KeyboardType.Passwordprovides a better UX by showing a keyboard optimized for password entry.๐ Suggested change
DmsTextField( modifier = Modifier .fillMaxWidth() .padding(top = 32.dp) .horizontalPadding(24.dp), label = "๋น๋ฐ๋ฒํธ", value = state.currentPassword, hint = "๋น๋ฐ๋ฒํธ ์ ๋ ฅ", onValueChange = onPasswordChange, showVisibleIcon = true, + keyboardType = KeyboardType.Password, )feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/ResetPasswordScreen.kt (1)
94-115: AddkeyboardType = KeyboardType.Passwordto both password fields.Both password text fields should specify the password keyboard type for better UX.
๐ Suggested change
DmsTextField( modifier = Modifier .fillMaxWidth() .padding(top = 32.dp) .horizontalPadding(24.dp), label = "๋น๋ฐ๋ฒํธ", value = state.newPassword, hint = "๋น๋ฐ๋ฒํธ ์ ๋ ฅ", onValueChange = onNewPasswordChange, showVisibleIcon = true, + keyboardType = KeyboardType.Password, ) DmsTextField( modifier = Modifier .fillMaxWidth() .padding(top = 32.dp) .horizontalPadding(24.dp), label = "๋น๋ฐ๋ฒํธ ์ฌ์ ๋ ฅ", value = state.checkNewPassword, hint = "๋น๋ฐ๋ฒํธ ์ฌ์ ๋ ฅ", onValueChange = onCheckNewPasswordChange, showVisibleIcon = true, + keyboardType = KeyboardType.Password, )feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt (2)
27-32: Rename misleading variableisSignInValueNotBlank.The variable name references "SignIn" but this is password reset logic. Consider a more descriptive name like
isPasswordFieldsValid.๐ Suggested fix
private fun setButtonEnabled() = setState { with(uiState.value) { - val isSignInValueNotBlank = newPassword.isNotBlank() && checkNewPassword.isNotBlank() && newPassword == checkNewPassword - it.copy(buttonEnabled = isSignInValueNotBlank) + val isPasswordFieldsValid = newPassword.isNotBlank() && checkNewPassword.isNotBlank() && newPassword == checkNewPassword + it.copy(buttonEnabled = isPasswordFieldsValid) } }
66-70: Consider consolidatingFailResetPasswordandPasswordMismatchside effects.Both
FailResetPasswordandPasswordMismatchare handled identically in the UI (showing an error snackbar). Consider whether a single error side effect with a message would simplify the code, or if the semantic distinction is intentional for future differentiation.feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (2)
28-52: Consider batching state updates for better performance.The current implementation updates the state on every iteration (line 49), which could trigger many re-compositions if the device contains hundreds or thousands of images. If the incremental loading is not required for the UI, consider collecting all URIs first and updating the state once after the loop completes.
๐ Potential optimization
cursor?.use { val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID) while (cursor.moveToNext()) { val id = cursor.getLong(idColumn) val imageUri = ContentUris.withAppendedId(contentUri, id) uriList.add(imageUri.toString()) - setState { it.copy(uriList = uriList) } } + setState { it.copy(uriList = uriList) } }However, if the current implementation is intentional to provide progressive loading feedback to the UI, you can safely ignore this suggestion.
84-84: Rename function to reflect its actual behavior.The function is named
fetchPresignedUrlbut it actually performs both fetching the presigned URL and uploading the file. Consider renaming it to better reflect its complete behavior, such asuploadFileWithPresignedUrloruploadFile.feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt (1)
29-30: Remove unused imports.The imports
titleBandtitleMare not used anywhere in this file.๐ Proposed fix
-import team.aliens.dms.android.core.designsystem.titleB -import team.aliens.dms.android.core.designsystem.titleMfeature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt (3)
15-18: Make injected dependencies private.The constructor parameters are declared as
val, which exposes them as public properties. ViewModels should encapsulate their dependencies as implementation details.๐ 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()) {
40-46: Add error handling for sign-out failures.The
signOutfunction only handles the success case. If the sign-out fails (e.g., network error), the user receives no feedback and remains in an ambiguous state.๐ Proposed fix
internal fun signOut() { viewModelScope.launch(Dispatchers.IO) { authRepository.signOut().onSuccess { sendEffect(SettingSideEffect.SignOutSuccess) + }.onFailure { + // Consider adding a side effect like SettingSideEffect.SignOutFailed + // to show an error message to the user } } }
78-78: Reconsider default value for isOnNotification.The default value is
true, but the actual notification status is unknown untilfetchNotificationStatus()completes. This could mislead users into thinking notifications are enabled when they might not be. Consider defaulting tofalseor introducing a separate loading state.data class SettingState( val deviceToken: String? = null, - val isOnNotification: Boolean = true, + val isOnNotification: Boolean = false, val notificationTopicStatus: List<NotificationTopicGroup.Status> = emptyList() )
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (15)
app/src/dev/AndroidManifest.xmlapp/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.ktdata/src/dev/kotlin/team.aliens.dms.android.data/user/repository/UserRepositoryImpl.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/viewmodel/MyPageViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/CheckPasswordRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/ResetPasswordRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/CheckPasswordScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/ResetPasswordScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt
๐ง Files skipped from review as they are similar to previous changes (4)
- data/src/dev/kotlin/team.aliens.dms.android.data/user/repository/UserRepositoryImpl.kt
- feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/ui/SelectProfileScreen.kt
- feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/ResetPasswordRoute.kt
- feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/CheckPasswordRoute.kt
๐งฐ Additional context used
๐งฌ Code graph analysis (10)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/viewmodel/MyPageViewModel.kt (2)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.kt (1)
MyPage(30-54)
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (14)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/navigation/OnboardingRoute.kt (1)
OnboardingRoute(6-13)feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt (1)
SignInRoute(7-20)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/navigation/HomeRoute.kt (1)
HomeRoute(9-22)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.kt (1)
ApplicationRoute(9-24)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
VoteRoute(8-23)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.kt (1)
RemainApplicationRoute(7-16)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageRoute.kt (1)
MyPageRoute(8-19)feature/src/dev/kotlin/team/aliens/dms/android/feature/meal/navigation/MealRoute.kt (1)
MealRoute(6-13)feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/navigation/SettingRoute.kt (1)
SettingRoute(7-22)feature/src/dev/kotlin/team/aliens/dms/android/feature/point/navigation/PointHistoryRoute.kt (1)
PointHistoryRoute(7-14)feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/ResetPasswordRoute.kt (1)
ResetPasswordRoute(8-21)feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/CheckPasswordRoute.kt (1)
CheckPasswordRoute(7-18)feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/route/SelectProfileRoute.kt (1)
SelectProfileRoute(7-18)feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/route/AdjustProfileRoute.kt (1)
AdjustProfileRoute(8-19)
feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/CheckPasswordScreen.kt (3)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/foundation/DmsSymbol.kt (1)
DmsSymbolContent(18-52)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/textfield/DmsTextField.kt (1)
DmsTextField(39-107)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsButton(260-356)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)
feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/ResetPasswordScreen.kt (3)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/foundation/DmsSymbol.kt (1)
DmsSymbolContent(18-52)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/textfield/DmsTextField.kt (1)
DmsTextField(39-107)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsButton(260-356)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageRoute.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.kt (1)
MyPage(30-54)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.kt (5)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/DmsIconButton.kt (1)
DmsIconButton(17-40)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/ProfileContent.kt (1)
ProfileContent(31-77)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/component/PhraseContent.kt (1)
PhraseContent(16-34)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/model/DmsPointCotent.kt (1)
DmsPointContent(25-71)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/DmsItemButton.kt (1)
DmsItemButton(33-84)
feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt (2)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)shared/validator/src/main/java/team/aliens/dms/android/shared/validator/PasswordValidator.kt (1)
checkIfPasswordValid(11-11)
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt (3)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsButton(260-356)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/DmsItemButton.kt (1)
DmsItemButton(33-84)feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/component/SettingRotateContent.kt (1)
SettingRotateContent(30-74)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (14)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageRoute.kt (1)
9-19: LGTM!Clean routing implementation that properly forwards navigation and snackbar callbacks to the
MyPagecomposable. The parameter types align correctly with theMyPagesignature.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/viewmodel/MyPageViewModel.kt (1)
32-38: LGTM!Clean state and side effect definitions. The sealed interface pattern for
MyPageSideEffectensures exhaustive handling in the UI layer.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/ui/MyPageScreen.kt (2)
30-54: LGTM!Well-structured composable with proper separation of concerns:
- ViewModel injection via
hiltViewModel()- State collection with lifecycle awareness using
collectAsStateWithLifecycle()- Side effect handling via
LaunchedEffectthat correctly collects from the ViewModel's side effect flowThe exhaustive
whenblock ensures all side effects are handled.
56-107: LGTM!Clean stateless UI implementation that properly:
- Separates the screen rendering from state management
- Uses design system components consistently (
DmsTopAppBar,DmsIconButton,DmsItemButton,DmsPointContent)- Composes profile, phrase, and point content from the state
Minor note:
modifier = Modifieron line 96 is the default value and could be omitted, but it's harmless.app/src/dev/AndroidManifest.xml (1)
10-11: Manifest permissions correctly configured for SDK version compatibility.The permission declarations properly address the API level differences:
READ_MEDIA_IMAGESfor Android 13+ (API 33+)READ_EXTERNAL_STORAGEwithmaxSdkVersion="32"for Android 12L and belowThe runtime permission logic in
SelectProfileScreen.kt(lines 58-62) correctly branches based onBuild.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISUand requests the appropriate permission for each SDK level, matching the manifest configuration.feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/ui/ResetPasswordScreen.kt (1)
33-66: LGTM on the side effect handling and ViewModel wiring.The composable correctly wires the ViewModel, collects side effects lifecycle-aware, and passes
currentPasswordto the reset function only when needed. The separation between the wiring composable and the stateless screen composable follows good practices.feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (4)
17-21: LGTM!The ViewModel setup with Hilt dependency injection is correct. The injected dependencies (
ContextandFileRepository) are appropriate for the file selection and upload operations.
54-66: LGTM!The image selection toggle logic is clear and correct. It properly handles both selecting and deselecting images.
68-82: LGTM!The null check and delegation pattern is appropriate. The function correctly handles the invalid case by emitting a
ProfileImageBadRequestside effect.
109-123: LGTM!The state and side effect definitions are well-structured. The use of
data objectfor singleton side effects is the correct Kotlin pattern.app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (4)
26-40: LGTM - Imports properly aligned with new features.The new imports for PointType and the additional route components support the navigation features being added in this PR.
87-87: Verify runtime JWT status changes are handled correctly.The parameter type changed from
StateFlow<Boolean>to plainBoolean, which meansisJwtAvailableis no longer reactive. If the JWT token becomes available or is invalidated during runtime (e.g., after login/logout), this won't trigger a recomposition or navigation update automatically.Confirm this is intentional and that JWT changes are handled through explicit navigation actions (e.g., logout navigates to SignIn explicitly) rather than reactive updates.
142-289: LGTM - Navigation implementation is comprehensive and well-structured.The navigation setup properly implements all the new screens with correct back stack management:
- Object-based NavKeys use direct reference removal
- Data class NavKeys create instances with matching parameters for removal (proper equals/hashCode from @serializable)
- Multi-step flows like password reset (lines 254-255) correctly pop multiple screens
- All routes consistently wire up callbacks and snackbar handling
The entryProvider pattern is properly utilized for Compose Navigation 3.
138-138: LGTM - Correct padding usage with Scaffold.Using
padding(paddingValues)from Scaffold is the appropriate approach, as it automatically handles system bar insets including navigation bars.
| @Serializable | ||
| data class ResetPasswordScreenNav(val currentPassword: String) : NavKey |
There was a problem hiding this comment.
Security: Consider not passing currentPassword via navigation parameter.
Passing the current password as a navigation parameter means it will be stored in the back stack and could be logged or exposed through debugging tools. Consider alternative approaches such as:
- Using a shared ViewModel to hold the password temporarily
- Storing it in the ResultStore
- Re-validating the password in the ResetPassword screen
๐ค Prompt for AI Agents
In @app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt around lines 75 -
76, The ResetPasswordScreenNav data class currently carries currentPassword
which exposes secrets in the navigation back stack; remove the currentPassword
field from ResetPasswordScreenNav and stop passing the password via NavKey,
instead keep the password in a transient, non-navigation holder such as a shared
ViewModel (e.g., a ResetPasswordViewModel) or use the ResultStore to transfer
the value, or require re-validation on the ResetPassword screen; update any
callers that construct ResetPasswordScreenNav and consumers that read it to use
the chosen alternative (shared ViewModel or ResultStore) and ensure no
logging/serialization of the password occurs.
...dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt
Outdated
Show resolved
Hide resolved
...dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt
Outdated
Show resolved
Hide resolved
...dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt
Show resolved
Hide resolved
| Column( | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) { |
There was a problem hiding this comment.
Add navigationBarsPadding to prevent UI overlap with system navigation.
As noted in previous reviews, the root Column needs Modifier.navigationBarsPadding() to avoid bottom UI elements being obscured by gesture navigation bars on devices that use them.
๐ Proposed fix
Column(
- modifier = Modifier.fillMaxSize(),
+ modifier = Modifier.fillMaxSize().navigationBarsPadding(),
) {๐ 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(), | |
| ) { | |
| Column( | |
| modifier = Modifier.fillMaxSize().navigationBarsPadding(), | |
| ) { |
๐ค Prompt for AI Agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt
around lines 103 - 105, The root Column in SettingScreen (the Composable where
Column is created with modifier = Modifier.fillMaxSize()) can have its bottom
content obscured by system gesture/navigation bars; update the Column modifier
to include navigationBarsPadding by combining Modifier.fillMaxSize() with
Modifier.navigationBarsPadding() (e.g.,
Modifier.fillMaxSize().navigationBarsPadding()) so the UI respects system
navigation insets.
| internal fun updateNotificationStatus(isOnNotification: Boolean) { | ||
| setState { settingState -> | ||
| settingState.copy(isOnNotification = !isOnNotification) | ||
| } | ||
| if (isOnNotification) /* ๊ตฌ๋ ์ทจ์ */ setNotificationStatus() else return // TODO ๊ตฌ๋ ์ ๋ฐ์ดํธ (false -> true) | ||
| } |
There was a problem hiding this comment.
Critical: Incomplete notification toggle implementation causes state inconsistency.
The function has several issues:
- Line 67 optimistically toggles
isOnNotificationin the UI state before any backend operation - Line 69 only implements unsubscribe (when
isOnNotificationwas true) but not subscribe (TODO comment confirms this) - The condition checks the OLD parameter value, not the new toggled value, which is confusing
setNotificationStatus()(lines 72-73) is an empty placeholder
This creates a critical state inconsistency: the UI shows the toggle switched, but the backend subscription state might not change (especially for the falseโtrue case). Users will see notifications as "ON" but won't receive any.
๐ Recommended approach
Option 1: Disable the toggle until the feature is fully implemented:
internal fun updateNotificationStatus(isOnNotification: Boolean) {
- setState { settingState ->
- settingState.copy(isOnNotification = !isOnNotification)
- }
- if (isOnNotification) /* ๊ตฌ๋
์ทจ์ */ setNotificationStatus() else return // TODO ๊ตฌ๋
์
๋ฐ์ดํธ (false -> true)
+ // TODO: Implement complete notification toggle
+ return
}Option 2: Implement the complete flow (requires backend implementation):
internal fun updateNotificationStatus(isOnNotification: Boolean) {
- setState { settingState ->
- settingState.copy(isOnNotification = !isOnNotification)
- }
- if (isOnNotification) /* ๊ตฌ๋
์ทจ์ */ setNotificationStatus() else return // TODO ๊ตฌ๋
์
๋ฐ์ดํธ (false -> true)
+ val newValue = !isOnNotification
+ viewModelScope.launch {
+ val deviceToken = uiState.value.deviceToken ?: return@launch
+ if (newValue) {
+ // Subscribe to notifications
+ notificationRepository.subscribeToTopics(deviceToken, /*topics*/)
+ } else {
+ // Unsubscribe from notifications
+ notificationRepository.unsubscribeFromTopics(deviceToken, /*topics*/)
+ }
+ // Only update UI state after successful backend call
+ setState { it.copy(isOnNotification = newValue) }
+ }
}Would you like me to help implement the complete notification toggle flow, or should this feature be disabled until the backend integration is ready?
๐ค Prompt for AI Agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt
around lines 65 - 70, The updateNotificationStatus function currently flips UI
state optimistically using the incoming isOnNotification parameter, checks the
old value, and only calls a placeholder setNotificationStatus() for unsubscribe;
fix by making the UI update reflect the result of a backend operation (or
disable the toggle until backend exists): in updateNotificationStatus, determine
the intended new state (newValue = !isOnNotification), call an implemented async
operation that handles both subscribe and unsubscribe (implement
subscribe/unsubscribe logic inside setNotificationStatus or split into
subscribeToNotifications / unsubscribeFromNotifications), await the backend
response, only then update state via setState(settingState ->
settingState.copy(isOnNotification = newValue)) and on failure revert state and
surface an error; if backend integration is unavailable, disable the toggle path
by preventing state change and returning an error message instead of
optimistically toggling.
There was a problem hiding this comment.
Actionable comments posted: 4
๐ค Fix all issues with AI agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt:
- Around line 82-104: The call to fileRepository.fetchPresignedUrl(file.name) in
SelectProfileViewModel.fetchPresignedUrl is executed outside the runCatching, so
any exception it throws will escape the coroutine; wrap the fetchPresignedUrl
call inside the same runCatching (or surround it with try/catch) so both
fetching the presigned info and fileRepository.uploadFile are guarded, and on
any failure call sendEffect(SelectProfileSideEffect.FailProfileImage) and
preserve the existing onSuccess handling that updates state and emits
SuccessProfileImage.
- Around line 28-50: The MediaStore query in getUriImage blocks the calling
thread; wrap the contentResolver.query and cursor iteration inside a background
IO coroutine using withContext(Dispatchers.IO) so the heavy I/O runs off the UI
thread, then post the resulting uriList back to the main thread (or call
setState from the coroutine after switching back) to update state; specifically
modify getUriImage (and any callers like loadImagesIfNeeded) to perform the
query + cursor.use inside Dispatchers.IO and only update setState on the main
dispatcher.
- Around line 66-80: uploadProfileImage currently calls
team.aliens.dms.android.core.file.File.toFile(...) on the calling thread which
may perform blocking I/O; move the URI-to-File conversion onto the IO dispatcher
by launching a coroutine on Dispatchers.IO (or using
viewModelScope.launch(Dispatchers.IO)) and call File.toFile inside that
coroutine, then switch back to Main if needed to call setState/sendEffect or
call fetchPresignedUrl from the IO coroutine; also ensure fetchPresignedUrl's
contract is clear (either remove its internal coroutine launch so it can be
called from this coroutine or keep it and only pass a ready File) to avoid
double-launching.
๐งน Nitpick comments (9)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
261-268: Password passed to navigation at line 264.This is where the password gets passed to
ResetPasswordScreenNav. As noted above, consider usingresultStore(similar to howvote_resultis handled on line 184) to pass the password transiently instead:โป๏ธ Alternative approach using ResultStore
entry<CheckPasswordScreenNav> { CheckPasswordRoute( onBackPressed = { backStack.remove(CheckPasswordScreenNav) }, - onNavigateResetPassword = { backStack.add(ResetPasswordScreenNav(it)) }, + onNavigateResetPassword = { password -> + resultStore.setResult<String?>("current_password", password) + backStack.add(ResetPasswordScreenNav) + }, onShowSnackBar = { snackBar, message -> appState.showSnackBar(snackBar, message) } ) }This would require changing
ResetPasswordScreenNavto adata objectand retrieving the password fromresultStorein theResetPasswordRoute.feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt (4)
26-26: Track or address the TODO comment.The TODO suggests future work on text field description message handling. Consider creating a tracking issue or implementing the logic now if it's essential for this feature.
Would you like me to open a tracking issue for this TODO, or would you prefer to implement the text field description message handling in this PR?
28-28: Rename variable for context clarity.The variable name
isSignInValueNotBlankappears to be a copy-paste artifact from a sign-in screen. In the password reset context, consider renaming it to something likearePasswordFieldsValidorisPasswordInputValid.โป๏ธ Proposed fix
- val isSignInValueNotBlank = newPassword.isNotBlank() && checkNewPassword.isNotBlank() && newPassword == checkNewPassword - it.copy(buttonEnabled = isSignInValueNotBlank) + val arePasswordFieldsValid = newPassword.isNotBlank() && checkNewPassword.isNotBlank() && newPassword == checkNewPassword + it.copy(buttonEnabled = arePasswordFieldsValid)
33-56: UsenewPasswordas the primary password field for semantic clarity.Throughout the
resetPasswordfunction,checkNewPasswordis used as the "source of truth" for the new password (lines 34, 38, 46). Semantically,newPasswordshould be the primary input andcheckNewPasswordshould be the confirmation field. While the current implementation is functionally correct (sincesetButtonEnabledenforces equality), usingcheckNewPasswordas the actual password to set is semantically backwards and reduces code clarity.Consider updating to use
newPasswordconsistently as the primary field.โป๏ธ Proposed fix
internal fun resetPassword(currentPassword: String) = run { - if(!checkIfPasswordValid(uiState.value.checkNewPassword)) { + if(!checkIfPasswordValid(uiState.value.newPassword)) { sendEffect(ResetPasswordSideEffect.PasswordMismatch("๋น๋ฐ๋ฒํธ๊ฐ ํ์์ ๋ง์ง ์์ต๋๋ค")) return@run } - if (currentPassword == uiState.value.checkNewPassword) { + if (currentPassword == uiState.value.newPassword) { sendEffect(ResetPasswordSideEffect.PasswordMismatch("๊ธฐ์กด ๋น๋ฐ๋ฒํธ๋ ๋ณ๊ฒฝ ๋ถ๊ฐํฉ๋๋ค")) return@run } viewModelScope.launch { setState { it.copy(isLoading = true, buttonEnabled = false) } userRepository.editPassword( currentPassword = currentPassword, - newPassword = uiState.value.checkNewPassword, + newPassword = uiState.value.newPassword, ).onSuccess { setState { it.copy(isLoading = false, buttonEnabled = true) } sendEffect(ResetPasswordSideEffect.SuccessResetPassword) }.onFailure { setState { it.copy(isLoading = false, buttonEnabled = true) } sendEffect(ResetPasswordSideEffect.FailResetPassword("๋น๋ฐ๋ฒํธ ๋ณ๊ฒฝ์ ์คํจํ์ต๋๋ค")) } } }
65-65: Track or address the TODO comment.The TODO suggests considering better alternatives for the side effect structure. The current sealed class pattern is a standard and appropriate approach for handling one-time events in MVVM. If you have specific concerns about this design, consider documenting them or creating a tracking issue.
Would you like me to open a tracking issue for exploring alternative patterns, or should this TODO be removed if the current approach is satisfactory?
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
116-121: Consider renaming ProfileImageBadRequest for clarity.The
ProfileImageBadRequestside effect is triggered when the URI is null (line 78). A more descriptive name likeInvalidUriorNullUriwould better communicate the specific error condition.๐ Suggested rename
sealed class SelectProfileSideEffect { data class SuccessProfileImage(val profileImageUrl: String) : SelectProfileSideEffect() - data object ProfileImageBadRequest : SelectProfileSideEffect() + data object InvalidUri : SelectProfileSideEffect() data object FailProfileImage : SelectProfileSideEffect() }Don't forget to update the reference on line 78:
- sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) + sendEffect(SelectProfileSideEffect.InvalidUri)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt (1)
93-112: Consider replacing non-null assertion with safe access.While the
!!operator is technically safe here (the button is only enabled whenselectId != null), using explicit null checks improves code resilience and readability.โป๏ธ Safer alternative using early return
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 { + val selectedId = uiState.value.selectId ?: return + viewModelScope.launch(Dispatchers.IO) { + setState { uiState.value.copy(isLoading = true, buttonEnabled = false) } + voteRepository.fetchCreateVotingItem( + votingTopicId = uiState.value.vote.id, + selectedId = selectedId, + ).onSuccess { + setState { uiState.value.copy(buttonEnabled = false, isLoading = false) } + sendEffect(VoteSideEffect.VoteSuccess) + }.onFailure { + setState { uiState.value.copy(isLoading = false, buttonEnabled = true) } + when (it) { + is ConflictException -> sendEffect(VoteSideEffect.VoteConflict) + else -> sendEffect(VoteSideEffect.VoteFail) - setState { uiState.value.copy(buttonEnabled = false, isLoading = false) } - sendEffect(VoteSideEffect.VoteSuccess) - }.onFailure { - setState { uiState.value.copy(isLoading = false, buttonEnabled = true) } - when (it) { - is ConflictException -> sendEffect(VoteSideEffect.VoteConflict) - else -> sendEffect(VoteSideEffect.VoteFail) - } } } } +} -}feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
6-6: Remove unused import.The
java.util.UUIDimport is not referenced in this file.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
41-41: Simplify redundant nullable cast.The safe cast operator
as?already returns a nullable type, soas? AllVoteSearch?is redundant. Useas? AllVoteSearchinstead.
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (6)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
๐งฐ Additional context used
๐งฌ Code graph analysis (3)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (12)
feature/src/dev/kotlin/team/aliens/dms/android/feature/onboarding/navigation/OnboardingRoute.kt (1)
OnboardingRoute(6-13)feature/src/dev/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.kt (1)
SignInRoute(7-20)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.kt (1)
ApplicationRoute(9-24)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
VoteRoute(8-17)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.kt (1)
RemainApplicationRoute(7-16)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/mypage/navigation/MyPageRoute.kt (1)
MyPageRoute(8-19)feature/src/dev/kotlin/team/aliens/dms/android/feature/meal/navigation/MealRoute.kt (1)
MealRoute(6-13)feature/src/dev/kotlin/team/aliens/dms/android/feature/point/navigation/PointHistoryRoute.kt (1)
PointHistoryRoute(7-14)feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/ResetPasswordRoute.kt (1)
ResetPasswordRoute(8-21)feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/CheckPasswordRoute.kt (1)
CheckPasswordRoute(7-18)feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/route/SelectProfileRoute.kt (1)
SelectProfileRoute(7-18)feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/route/AdjustProfileRoute.kt (1)
AdjustProfileRoute(8-19)
feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt (2)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)shared/validator/src/main/java/team/aliens/dms/android/shared/validator/PasswordValidator.kt (1)
checkIfPasswordValid(11-11)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (11)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (4)
103-114: LGTM!The refactoring to accept
Booleaninstead ofStateFlow<Boolean>simplifies this composable and moves state collection responsibility to the caller. TheLaunchedEffectdependencies are correctly specified.
243-247: LGTM!The navigation entry correctly captures the
pointTypefrom the NavKey and uses it to reconstruct the key for removal. Data class equality ensures the correct entry is removed.
248-260: LGTM!The navigation logic for
ResetPasswordScreenNavcorrectly handles the back navigation by removing both the current screen andCheckPasswordScreenNavwhen navigating back to settings, ensuring a clean navigation state.
279-286: LGTM!The
AdjustProfileRouteentry is correctly wired with the model parameter and navigation callbacks.feature/src/dev/kotlin/team/aliens/dms/android/feature/resetpassword/viewmodel/ResetPasswordViewModel.kt (2)
11-14: LGTM!The ViewModel is properly structured with Hilt dependency injection, and the
userRepositoryis correctly marked as private following encapsulation best practices.
16-24: LGTM!The password setter methods correctly update state and re-evaluate button enablement.
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (3)
1-21: LGTM: Clean setup with proper DI configuration.The imports, class declaration, and dependency injection setup are well-structured. The use of
@ApplicationContextfor Context injection and internal visibility for the ViewModel are appropriate.
23-26: LGTM: Efficient guard for image loading.The guard check prevents unnecessary reloading of images when the list is already populated.
52-64: LGTM: Correct toggle logic for image selection.The selection and deselection logic properly maintains the
selectedUriandenabledstate flags.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt (1)
31-42: LGTM! Type-safe refactoring.The migration from
StringtoLocalDateTimefor time parameters improves type safety and eliminates potential parsing errors. The addition ofidassignment in the vote copy aligns with the new result-driven initialization pattern.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
9-17: LGTM! Clean API simplification.Removing the explicit parameters in favor of the result store pattern reduces coupling and simplifies the route signature.
| internal fun getUriImage() { | ||
| val uriList = mutableListOf<String>() | ||
| val contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI | ||
| val projection = arrayOf(MediaStore.Images.Media._ID) | ||
|
|
||
| val cursor = context.contentResolver.query( | ||
| contentUri, | ||
| projection, | ||
| null, | ||
| null, | ||
| "${MediaStore.Images.Media.DATE_ADDED} DESC" | ||
| ) | ||
|
|
||
| cursor?.use { | ||
| val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID) | ||
| while (cursor.moveToNext()) { | ||
| val id = cursor.getLong(idColumn) | ||
| val imageUri = ContentUris.withAppendedId(contentUri, id) | ||
| uriList.add(imageUri.toString()) | ||
| } | ||
| } | ||
| setState { it.copy(uriList = uriList) } | ||
| } |
There was a problem hiding this comment.
Move MediaStore query to IO dispatcher.
The ContentResolver.query operation can be slow when there are many images in the gallery, potentially blocking the calling thread. Since loadImagesIfNeeded() may be invoked from the UI thread, this could cause UI jank.
๐ Recommended refactor to use IO dispatcher
internal fun getUriImage() {
- val uriList = mutableListOf<String>()
- val contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI
- val projection = arrayOf(MediaStore.Images.Media._ID)
-
- val cursor = context.contentResolver.query(
- contentUri,
- projection,
- null,
- null,
- "${MediaStore.Images.Media.DATE_ADDED} DESC"
- )
-
- cursor?.use {
- val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID)
- while (cursor.moveToNext()) {
- val id = cursor.getLong(idColumn)
- val imageUri = ContentUris.withAppendedId(contentUri, id)
- uriList.add(imageUri.toString())
+ viewModelScope.launch(Dispatchers.IO) {
+ val uriList = mutableListOf<String>()
+ val contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI
+ val projection = arrayOf(MediaStore.Images.Media._ID)
+
+ val cursor = context.contentResolver.query(
+ contentUri,
+ projection,
+ null,
+ null,
+ "${MediaStore.Images.Media.DATE_ADDED} DESC"
+ )
+
+ cursor?.use {
+ val idColumn = cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID)
+ while (cursor.moveToNext()) {
+ val id = cursor.getLong(idColumn)
+ val imageUri = ContentUris.withAppendedId(contentUri, id)
+ uriList.add(imageUri.toString())
+ }
}
+ setState { it.copy(uriList = uriList) }
}
- setState { it.copy(uriList = uriList) }
}๐ค Prompt for AI Agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt
around lines 28 - 50, The MediaStore query in getUriImage blocks the calling
thread; wrap the contentResolver.query and cursor iteration inside a background
IO coroutine using withContext(Dispatchers.IO) so the heavy I/O runs off the UI
thread, then post the resulting uriList back to the main thread (or call
setState from the coroutine after switching back) to update state; specifically
modify getUriImage (and any callers like loadImagesIfNeeded) to perform the
query + cursor.use inside Dispatchers.IO and only update setState on the main
dispatcher.
| snapshotFlow { | ||
| resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch? | ||
| }.collect { result -> | ||
| if (result != null) { | ||
| viewModel.initState(result.topicName, result.startTime, result.endTime, result.id) | ||
| resultStore.removeResult<String?>(resultKey = "vote_result") | ||
| } else { | ||
| onShowSnackBar(DmsSnackBarType.ERROR, "์ ๋ณด๋ฅผ ๊ฐ์ ธ์ค์ง ๋ชป ํ์ด์") | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix initialization error handling logic.
The snapshotFlow emits immediately upon collection. If the result store doesn't contain a value at "vote_result" when the screen first loads, it will emit null and trigger the error snackbar on Line 47, even though no actual error occurred.
This causes the error message "์ ๋ณด๋ฅผ ๊ฐ์ ธ์ค์ง ๋ชป ํ์ด์" to appear on every initial navigation to this screen.
Consider one of these approaches:
- Check if the result exists before starting collection
- Use a flag to track whether initialization has been attempted
- Handle the initial null state differently from an error state
๐ ๏ธ Suggested fix using initial state check
LaunchedEffect(Unit) {
+ val initialResult = resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch
+ if (initialResult == null) {
+ onShowSnackBar(DmsSnackBarType.ERROR, "์ ๋ณด๋ฅผ ๊ฐ์ ธ์ค์ง ๋ชป ํ์ด์")
+ return@LaunchedEffect
+ }
+
+ viewModel.initState(
+ initialResult.topicName,
+ initialResult.startTime,
+ initialResult.endTime,
+ initialResult.id
+ )
+ resultStore.removeResult<AllVoteSearch>(resultKey = "vote_result")
- snapshotFlow {
- resultStore.resultStateMap["vote_result"]?.value as? AllVoteSearch?
- }.collect { result ->
- if (result != null) {
- viewModel.initState(result.topicName, result.startTime, result.endTime, result.id)
- resultStore.removeResult<String?>(resultKey = "vote_result")
- } else {
- onShowSnackBar(DmsSnackBarType.ERROR, "์ ๋ณด๋ฅผ ๊ฐ์ ธ์ค์ง ๋ชป ํ์ด์")
- }
- }
-
-
viewModel.sideEffect.collect {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
๐ค Fix all issues with AI agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt:
- Around line 84-106: The call to fileRepository.fetchPresignedUrl(file.name) in
SelectProfileViewModel.fetchPresignedUrl is outside the runCatching block so any
exception from fetchPresignedUrl will crash the coroutine; move the
fetchPresignedUrl call inside a single runCatching that wraps both fetching the
presignedInfo and the subsequent fileRepository.uploadFile call, then keep the
existing .onSuccess { setState { ... };
sendEffect(SelectProfileSideEffect.SuccessProfileImage(...)) } and .onFailure {
sendEffect(SelectProfileSideEffect.FailProfileImage) } handlers so any exception
from either fetch or upload is caught and handled.
- Around line 68-82: uploadProfileImage currently calls
team.aliens.dms.android.core.file.File.toFile(...) on the calling thread and
without error handling; wrap the blocking call in a coroutine using
Dispatchers.IO (e.g., viewModelScope.launch or withContext(Dispatchers.IO)) and
surround the toFile call with try/catch to handle exceptions (invalid URI,
inaccessible file) โ on error
sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) or an appropriate
error effect, and on success switch back to Main to setState { it.copy(uri =
uri) } and call fetchPresignedUrl(file = ...) so UI updates and network flow
remain on the correct dispatcher.
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt
๐งฐ Additional context used
๐งฌ Code graph analysis (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
๐ Additional comments (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt (1)
28-52: LGTM! MediaStore query is now properly dispatched.The MediaStore query and cursor iteration are now correctly wrapped in
Dispatchers.IO, which resolves the previous concern about blocking the calling thread.
| internal fun uploadProfileImage( | ||
| uri: Uri?, | ||
| ) { | ||
| if (uri != null) { | ||
| setState { it.copy(uri = uri) } | ||
| fetchPresignedUrl( | ||
| file = team.aliens.dms.android.core.file.File.toFile( | ||
| context = context, | ||
| uri = uri, | ||
| ), | ||
| ) | ||
| } else { | ||
| sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) | ||
| } | ||
| } |
There was a problem hiding this comment.
File.toFile() blocks the calling thread and lacks error handling.
Two issues remain in this function:
-
Blocking I/O (previously flagged): Line 74 calls
File.toFile(context, uri)synchronously, which can block the calling thread and cause UI jank or ANR. -
Missing error handling:
File.toFile()can throw exceptions (e.g., invalid URI, inaccessible file) which will crash the coroutine uncaught.
๐ Recommended fix to move to IO dispatcher and add error handling
internal fun uploadProfileImage(
uri: Uri?,
) {
if (uri != null) {
setState { it.copy(uri = uri) }
- fetchPresignedUrl(
- file = team.aliens.dms.android.core.file.File.toFile(
- context = context,
- uri = uri,
- ),
- )
+ viewModelScope.launch(Dispatchers.IO) {
+ runCatching {
+ team.aliens.dms.android.core.file.File.toFile(
+ context = context,
+ uri = uri,
+ )
+ }.onSuccess { file ->
+ fetchPresignedUrl(file)
+ }.onFailure {
+ sendEffect(SelectProfileSideEffect.FailProfileImage)
+ }
+ }
} else {
sendEffect(SelectProfileSideEffect.ProfileImageBadRequest)
}
}๐ 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.
| internal fun uploadProfileImage( | |
| uri: Uri?, | |
| ) { | |
| if (uri != null) { | |
| setState { it.copy(uri = uri) } | |
| fetchPresignedUrl( | |
| file = team.aliens.dms.android.core.file.File.toFile( | |
| context = context, | |
| uri = uri, | |
| ), | |
| ) | |
| } else { | |
| sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) | |
| } | |
| } | |
| internal fun uploadProfileImage( | |
| uri: Uri?, | |
| ) { | |
| if (uri != null) { | |
| setState { it.copy(uri = uri) } | |
| viewModelScope.launch(Dispatchers.IO) { | |
| runCatching { | |
| team.aliens.dms.android.core.file.File.toFile( | |
| context = context, | |
| uri = uri, | |
| ) | |
| }.onSuccess { file -> | |
| fetchPresignedUrl(file) | |
| }.onFailure { | |
| sendEffect(SelectProfileSideEffect.FailProfileImage) | |
| } | |
| } | |
| } else { | |
| sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) | |
| } | |
| } |
๐ค Prompt for AI Agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt
around lines 68 - 82, uploadProfileImage currently calls
team.aliens.dms.android.core.file.File.toFile(...) on the calling thread and
without error handling; wrap the blocking call in a coroutine using
Dispatchers.IO (e.g., viewModelScope.launch or withContext(Dispatchers.IO)) and
surround the toFile call with try/catch to handle exceptions (invalid URI,
inaccessible file) โ on error
sendEffect(SelectProfileSideEffect.ProfileImageBadRequest) or an appropriate
error effect, and on success switch back to Main to setState { it.copy(uri =
uri) } and call fetchPresignedUrl(file = ...) so UI updates and network flow
remain on the correct dispatcher.
| internal fun fetchPresignedUrl( | ||
| file: File, | ||
| ) { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| val presignedInfo = fileRepository.fetchPresignedUrl(file.name) | ||
| runCatching { | ||
| fileRepository.uploadFile( | ||
| presignedUrl = presignedInfo.fileUploadUrl, | ||
| file = file, | ||
| ) | ||
| }.onSuccess { fileUrl -> | ||
| setState { | ||
| it.copy( | ||
| profileImageUrl = fileUrl.fileUrl, | ||
| buttonEnabled = true, | ||
| ) | ||
| } | ||
| sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl.fileUrl)) | ||
| }.onFailure { | ||
| sendEffect(SelectProfileSideEffect.FailProfileImage) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: fetchPresignedUrl exception will crash the coroutine.
Line 88 calls fileRepository.fetchPresignedUrl(file.name) outside the runCatching block. Any exception thrown by this call will crash the coroutine without being caught, and the user won't receive the FailProfileImage side effect.
This critical issue was flagged in previous reviews and remains unfixed.
๐ Proposed fix to move fetchPresignedUrl inside runCatching
internal fun fetchPresignedUrl(
file: File,
) {
viewModelScope.launch(Dispatchers.IO) {
- val presignedInfo = fileRepository.fetchPresignedUrl(file.name)
runCatching {
+ val presignedInfo = fileRepository.fetchPresignedUrl(file.name)
fileRepository.uploadFile(
presignedUrl = presignedInfo.fileUploadUrl,
file = file,
)
}.onSuccess { fileUrl ->
setState {
it.copy(
profileImageUrl = fileUrl.fileUrl,
buttonEnabled = true,
)
}
sendEffect(SelectProfileSideEffect.SuccessProfileImage(fileUrl.fileUrl))
}.onFailure {
sendEffect(SelectProfileSideEffect.FailProfileImage)
}
}
}๐ค Prompt for AI Agents
In
@feature/src/dev/kotlin/team/aliens/dms/android/feature/profile/viewmodel/SelectProfileViewModel.kt
around lines 84 - 106, The call to fileRepository.fetchPresignedUrl(file.name)
in SelectProfileViewModel.fetchPresignedUrl is outside the runCatching block so
any exception from fetchPresignedUrl will crash the coroutine; move the
fetchPresignedUrl call inside a single runCatching that wraps both fetching the
presignedInfo and the subsequent fileRepository.uploadFile call, then keep the
existing .onSuccess { setState { ... };
sendEffect(SelectProfileSideEffect.SuccessProfileImage(...)) } and .onFailure {
sendEffect(SelectProfileSideEffect.FailProfileImage) } handlers so any exception
from either fetch or upload is caught and handled.
๊ฐ์
์์ ์ฌํญ
์ถ๊ฐ ๋ก ํ ๋ง
Summary by CodeRabbit
New Features
Permissions
Design & UI Updates
โ๏ธ Tip: You can customize this high-level summary in your review settings.