-
Notifications
You must be signed in to change notification settings - Fork 0
[Fix] MyPage UiState/UiEvent 리팩토링 및 UserNickname 관련 로컬 저장소 로직 변경 #382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @HI-JIN2, 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은 MyPage의 UI 상태 관리 및 이벤트 처리 방식을 개선하고, 사용자 닉네임 관련 로직을 최적화하는 데 중점을 둡니다. 특히, 닉네임 정보의 로컬 저장소 활용을 강화하고, UI 업데이트의 즉각성을 확보하며, 전반적인 코드의 효율성과 사용자 경험을 향상시키는 변경 사항들을 포함합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
이번 PR은 MyPage의 UI 상태 관리 로직을 리팩토링하고 닉네임 관련 로직을 개선하는 내용을 담고 있습니다. 전반적으로 MVI 패턴을 도입하여 상태 관리를 개선하고, 불필요한 Flow를 제거하여 코드를 간소화한 점이 좋습니다. 몇 가지 개선점과 치명적인 버그를 발견하여 리뷰에 포함했습니다. 특히 UserInfoViewModel의 닉네임 변경 로직에 있는 경쟁 조건은 반드시 수정이 필요합니다. 그 외에는 코드 가독성 및 유지보수성을 높이기 위한 제안들입니다.
app/src/main/java/com/eatssu/android/presentation/mypage/userinfo/UserInfoViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/domain/usecase/user/GetUserNickNameUseCase.kt
Show resolved
Hide resolved
| binding.tvNickname.text = state.nickname | ||
| } else { | ||
| // 필요 시 미설정 안내 문구 | ||
| binding.tvNickname.text = "닉네임을 설정해주세요" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 권한 미허용이면 스위치 원복 | ||
| binding.alarmSwitch.setOnCheckedChangeListener(null) | ||
| binding.alarmSwitch.isChecked = false | ||
| binding.alarmSwitch.setOnCheckedChangeListener { _, checked -> | ||
| handleAlarmSwitchChange(checked) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data class MyPageState( | ||
| var loading: Boolean = true, | ||
| var error: Boolean = false, | ||
|
|
||
| var toastMessage: String = "", | ||
|
|
||
| var nickname: String = "", | ||
| var platform: String = "", | ||
| var isAlarmOn: Boolean = false, | ||
| var appVersion: String = "0.0.0", | ||
|
|
||
| var isNicknameNull: Boolean = false, | ||
| ) No newline at end of file | ||
| val nickname: String? = null, | ||
| val platform: String = "KAKAO", | ||
| val isAlarmOn: Boolean = false, | ||
| val appVersion: String = "0.0.0" | ||
| ) { | ||
| val hasNickname: Boolean get() = !nickname.isNullOrBlank() && !nickname.startsWith("user-") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
말씀해주신 것처럼 갑자기 닉네임 변경 창이 뜬다고 해서 바꿔야함을 인지못할 것 같아요. |
어제 pr 머지하고 시작한 작업이 없는데 그럼 이 작업부터 시작하겠습니다~!
그러게요 뭔가 영상으로 보니까 오류같네요 ..ㅋㅋㅋ |
kangyuri1114
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코루틴, flow를 잘 사용해주신 코드 잘 봤습니다!👍
궁금한 점 몇가지 코멘트 달아놔서 확인부탁드려요!
|
|
||
| suspend fun getUserReviews(): Flow<BaseResponse<MyReviewResponse>> | ||
| suspend fun getUserNickName(): Flow<BaseResponse<MyNickNameResponse>> | ||
| suspend fun getUserNickName(): String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홋 앞으로 단순 api 호출 작업에서는 flow를 덜어내는 방향으로 진행하실 예정일까요?
저번에 제휴지도 부분에서 리뷰 코멘트로 단순 api호출 결과를 flow로 계속 방출하는 게 잘 이해가 안된다고 답변을 남겼던거같은데
그 후로 팀내에서 정확히 정한 부분은 없는 것 같아서요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유리님 말씀이 맞는 것 같아 viewmodel -> ui 말고는 suspend로 수정하려고 합니다! 이부분을 이야기하지 않은 것을 까먹었네요 😅 좋은 인사이트 감사합니다
| ) | ||
| _uiState.value = UiState.Loading | ||
| runCatching { | ||
| withContext(Dispatchers.IO) { getUserNickNameUseCase() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
바뀐 코드가 더 좋네요!!
| val uiState: StateFlow<UiState<MyPageState>> = | ||
| _state | ||
| .map { UiState.Success(it) } | ||
| .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), UiState.Init) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 코드는 제가 처음봐요!
SharingStarted.WhileSubscribed(5_000) 이렇게 5000으로 값을 정하신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5000은 관용적인 표현인 것 같아요..! https://velog.io/@mraz3068/5000
저렇게 단일값만을 success의 데이터를 갖는 uiState에서 이런 표현을 종종 쓰더라구욥
| override fun onResume() { | ||
| super.onResume() | ||
| myPageViewModel.fetchMyInfo() // 닉네임 변경 등으로부터 복귀 시 정보 갱신 | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
결과적으로 닉네임 수정 후 화면에 바로 반영 안되는 오류는 onresume에서 호출하는 방식으로 해결하신것 같아요
onResume에서 api로 닉네임을 계속 호출하고 있잖아요
사실 이 이슈를 제가 가져가려고 전부터 머리로만 계속 생각했었는데
저는 myInfoActivity에서 성공적으로 닉네임을 반영한 경우 sharedPreference를 업데이트하고,
mypage에서 닉네임은 sharedPreference값을 보여주는 방식으로 수정할 생각이었거든요
혹시 이 부분에 대해서는 어떻게 생각하시나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 사실 이슈를 수정하려고 한건 아닌데 어쩌다보니 함께 수정이 되었습니다.. 😮
유리님께서 말씀하신 방법으로 동작합니다!
닉네임 수정하면 setNicknameUsecase에서 api호출이랑 sharedPreference set 함수 둘다 호출되고
getNicknameUsecase 호출하면 sharedPreference 먼저 검사해서 있으면 로컬 값 주고 비어있으면 api호출하고 sharedPreference set합니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 그렇군요!! 수고하셨씁니당~!
|
@PeraSite @kangyuri1114 일단 토스트는 실행됩니다! UI적으로 캐릭터랑 같이 잇슈 서비스에서 당신을 표현할 닉네임을 정해주세요~ 뭐 이런 귀여운 문구를 쓰면 좋겠는데, 이건 나령님에게 토스하도록 할게요... |
PeraSite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍👍👍👍
Summary
MyPage UiState/UiEvent 리팩토링 및 UserNickname 관련 로컬 저장소 로직 변경
Describe your changes
Screen_recording_20251001_001357.webm
*영상은 toast 통일 이전에 찍었습니다
Issue
To reviewers