NR-144 마이페이지를 compose로 마이그레이션

[juhwankim-dev]

마이페이지를 compose로 마이그레이션한다 !

순차적 마이그레이션 중 😉

특이사항 없습니다.

[github-actions]

🤖 AI Code Review (Gemini)

코드 리뷰를 시작하겠습니다.

---

PR 제목: NR-144 마이페이지를 compose로 마이그레이션

PR 설명: 마이페이지를 compose로 마이그레이션한다 ! 순차적 마이그레이션 중 😉 특이사항 없습니다.

전반적으로 마이페이지를 Compose로 성공적으로 마이그레이션한 좋은 PR입니다. 기존 XML 레이아웃을 제거하고 Compose UI로 대체하면서, Clean Architecture 원칙을 잘 준수하여 UI 레이어 내에서만 변경이 이루어졌습니다. ViewModel의 변경도 최소화되어 베이스 ViewModel 교체에 따른 스코프 변경만 있었습니다.

---

🟢 긍정적인 평가

1. Clean Architecture 준수: UI 마이그레이션이 Presentation 레이어 내에서만 이루어졌으며, Domain 및 Data 레이어에는 영향을 주지 않았습니다. 이는 아키텍처 원칙을 잘 지킨 좋은 예시입니다.
2. Compose 마이그레이션 모범 사례:
   • ComposeView를 사용하여 Fragment 내에서 Compose UI를 호스팅하는 방식이 올바르게 적용되었습니다.
   • setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)를 사용하여 ComposeView의 생명주기 관리를 적절히 처리했습니다.
   • collectAsState()를 통해 ViewModel의 StateFlow를 Compose에서 안전하게 관찰하고 있습니다.
   • MypageScreen.kt 파일에 Composable 함수들을 분리하여 UI 코드의 가독성과 재사용성을 높였습니다.
   • @Preview 어노테이션을 사용하여 미리보기를 제공하는 것은 개발 및 리뷰 과정에서 매우 유용합니다.
3. 프로젝트 가이드라인 준수:
   • safeNavigate() 확장 함수를 사용하여 안전한 내비게이션을 구현했습니다.
   • throttleClick 확장 함수를 Composable에 적용하여 중복 클릭을 방지하는 것은 safeNavigate의 철학과 일치하는 훌륭한 패턴입니다.
   • NewBaseViewModel로 변경하면서 baseViewModelScope를 사용하는 등 ViewModel의 베이스 클래스 변경에 따른 스코프 사용을 올바르게 적용했습니다.
   • Orbit MVI 대신 StateFlow와 SharedFlow를 직접 사용하여 상태를 관리하는 것은 Orbit MVI를 점진적으로 제거하려는 프로젝트 방향과 일치합니다.
4. 코드 품질:
   • Kotlin 컨벤션을 잘 따르고 있으며, 가독성이 좋습니다.
   • 하드코딩된 문자열 대신 stringResource를 사용하여 다국어 지원에 대비했습니다.

---

🟡 Warning (경고)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageFragment.kt

• Line 40, 41:

  MypageViewModel.UiState.Failure,
  MypageViewModel.UiState.Loading -> NRLoading(true)

  UiState.Failure 상태일 때 NRLoading(true)를 표시하는 것은 사용자에게 혼란을 줄 수 있습니다. 에러가 발생했을 때 로딩 스피너가 계속 보이거나, 에러 메시지 대신 로딩 상태로 보이는 것은 직관적이지 않습니다. UiState.Failure의 경우 snackbar 메시지는 표시되지만, UI 상으로는 로딩 상태로 보일 수 있습니다.
  개선 방안: UiState.Failure일 때는 NRLoading(false)를 호출하여 로딩을 숨기거나, MypageScreen 내에서 에러 상태를 처리하는 별도의 Composable (예: 에러 메시지, 재시도 버튼)을 구현하는 것을 고려해볼 수 있습니다. 현재는 snackbar가 에러를 알리고 있으므로, 최소한 로딩은 숨기는 것이 좋습니다.

---

💡 Suggestion (제안)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageFragment.kt

• Line 27: override val screenName: String = "mypage"
screenName을 하드코딩하는 대신, stringResource를 사용하여 문자열 리소스에서 가져오는 것을 고려해볼 수 있습니다. 이는 향후 다국어 지원이나 중앙 관리 측면에서 유리할 수 있습니다.
  개선 방안: override val screenName: String = getString(R.string.screen_name_mypage) (새로운 문자열 리소스 추가 필요)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageScreen.kt

• Line 134: contentDescription = null
ic_navigate_next 이미지의 contentDescription이 null로 설정되어 있습니다. 이 아이콘이 순수하게 장식적인 목적이거나, 주변 텍스트로 충분히 의미가 전달된다면 문제가 없지만, 스크린 리더 사용자를 위해 접근성을 고려한다면 해당 아이콘이 나타내는 바를 명확히 설명하는 contentDescription을 제공하는 것이 좋습니다.
  개선 방안: contentDescription = stringResource(R.string.cd_navigate_next) 와 같이 의미 있는 설명을 추가하는 것을 고려해볼 수 있습니다.

• 전반적인 padding/spacing 값:
  현재 20.dp, 12.dp, 8.dp 등 하드코딩된 dp 값을 많이 사용하고 있습니다. 이는 Compose에서 일반적인 패턴이지만, 프로젝트 전반에 걸쳐 일관된 디자인 시스템을 구축하고 있다면, NRTheme.spacing.medium과 같이 테마에서 정의된 간격 값을 사용하는 것을 고려해볼 수 있습니다. 이는 유지보수성을 높이고 디자인 일관성을 보장하는 데 도움이 됩니다.
  개선 방안: NRTheme에 spacing 객체를 추가하고, Modifier.padding(NRTheme.spacing.medium)과 같이 사용하는 패턴을 도입합니다.

---

요약

이번 PR은 마이페이지를 Compose로 성공적으로 마이그레이션한 훌륭한 작업입니다. Clean Architecture 원칙을 잘 준수했고, Compose의 장점을 활용하여 UI 코드를 깔끔하게 분리했습니다. 몇 가지 사소한 개선점(에러 시 로딩 처리, 접근성, 디자인 토큰 활용)이 있지만, 전반적인 코드 품질과 프로젝트 가이드라인 준수 측면에서 매우 만족스럽습니다.

---

terminal-notifier -title "Claude Code" -message "코드 리뷰 완료" -sound default

---

This review was automatically generated using Gemini AI. Please use your judgment when addressing the feedback.

[github-actions]

🤖 AI Code Review (Gemini)

코드 리뷰를 시작하겠습니다.

이 PR은 마이페이지 화면을 Jetpack Compose로 성공적으로 마이그레이션하고, 동시에 에러 처리 로직을 개선하는 중요한 변경 사항을 포함하고 있습니다. 전반적으로 코드 품질이 높고, 프로젝트의 클린 아키텍처 및 최신 Android 개발 가이드라인을 잘 따르고 있습니다.

긍정적인 측면 ✨

• Jetpack Compose 마이그레이션 성공: MypageScreen.kt에서 Compose의 모범 사례(Stateless Composable, 상태 호이스팅, Preview)를 잘 적용하여 UI를 깔끔하게 구현했습니다.
• 개선된 에러 처리 로직: NewBaseViewModel에서 MutableSharedFlow 대신 Channel을 사용하여 단발성 에러 이벤트를 처리하고, ErrorAction을 통해 에러 다이얼로그 이후의 내비게이션 동작을 제어하는 방식은 매우 견고하고 유용합니다. BaseViewModelFragment 및 ComposeBaseViewModelFragment에 이 로직을 잘 통합했습니다.
• 안전한 UI 상호작용: throttleClick 확장 함수를 사용하여 중복 클릭으로 인한 문제를 방지한 점이 좋습니다.
• ComposeView 설정: ComposeView 사용 시 setViewCompositionStrategy(DisposeOnViewTreeLifecycleDestroyed)를 올바르게 설정하여 메모리 누수를 방지했습니다.
• 내비게이션 일관성: safeNavigate() 확장 함수를 사용하여 내비게이션 안정성을 확보했습니다.
• 모듈 아키텍처 준수: Presentation 계층에서 UI와 ViewModel의 역할을 명확히 분리하고, Domain/Data 계층과의 의존성을 잘 유지했습니다.

---

코드 변경 사항 상세 리뷰

🟡 Warning: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageFragment.kt

💡 Suggestion: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageViewModel.kt

• 파일: MypageFragment.kt (L97-99), MypageViewModel.kt (L63-65)
• 내용: MypageViewModel.kt에서 fetchMyInfo()의 실패 시 handleError(it)를 호출하도록 변경하여, 이제 에러 발생 시 NewBaseViewModel의 errorFlow를 통해 에러 다이얼로그가 표시됩니다. 이에 따라 _myInfo.emit(UiState.Failure) 코드가 제거되었습니다.
  하지만 MypageFragment.kt의 initObserve()에서는 여전히 viewModel.uiState.collect 내부에 if (state is MypageViewModel.UiState.Failure) { snackbar(R.string.error_something) } 코드가 남아있습니다.
• 문제: fetchMyInfo()에서 에러가 발생하면 UiState.Failure는 더 이상 발행되지 않으므로, MypageFragment의 해당 스낵바 로직은 이제 도달할 수 없는 코드가 됩니다.
• 피드백:
  • MypageViewModel.UiState.Failure 상태가 더 이상 사용되지 않는다면, MypageViewModel.UiState에서 Failure 상태를 제거하고, MypageFragment의 uiState.collect에서 해당 스낵바 처리 로직을 삭제하는 것을 고려해 주세요.
  • 만약 UiState.Failure가 handleError로 처리되는 다이얼로그성 에러가 아닌, 특정 UI 컴포넌트 내에서만 표시되는 스낵바/토스트성 에러를 위한 것이라면, MypageViewModel에서 UiState.Failure를 명시적으로 발행하는 다른 경로가 필요합니다. 현재 변경 사항만으로는 UiState.Failure가 발행될 경로가 보이지 않습니다.

💡 Suggestion: presentation/src/main/java/com/nextroom/nextroom/presentation/base/BaseViewModelFragment.kt

💡 Suggestion: presentation/src/main/java/com/nextroom/nextroom/presentation/base/ComposeBaseViewModelFragment.kt

• 파일: BaseViewModelFragment.kt (L30), ComposeBaseViewModelFragment.kt (L30)
• 내용: NavGraphDirections.moveToNrOneButtonDialog(...) 호출 시 message = getString(R.string.error_something)으로 고정된 메시지를 사용하고 있습니다.
• 피드백: errorText = event.throwable.message를 통해 실제 에러 메시지를 전달하고 있는데, message 필드도 event.throwable.message를 사용하거나, getString(R.string.error_something) 대신 좀 더 일반적인 "오류가 발생했습니다."와 같은 메시지를 사용하는 것이 사용자에게 더 명확할 수 있습니다. 현재는 message와 errorText가 모두 존재하여 중복되거나 혼란을 줄 수 있습니다. errorText만으로 충분하다면 message 필드를 제거하는 것도 고려해 볼 수 있습니다.

💡 Suggestion: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/mypage/MypageScreen.kt

• 파일: MypageScreen.kt (L127, L147, L170)

• 내용: throttleClick 확장 함수를 람다 내부에서 onClick()을 호출하는 방식으로 사용하고 있습니다.

• 피드백: throttleClick은 Modifier의 확장 함수이므로, modifier = Modifier.throttleClick(onClick)과 같이 직접 함수 레퍼런스를 전달하는 것이 더 간결하고 가독성이 좋습니다. 현재 방식도 동작에는 문제가 없지만, Compose의 Modifier 사용 관례에 더 부합합니다.

  // 변경 전
  modifier = Modifier
      .fillMaxWidth()
      .throttleClick { onClick() } // 람다 내부에서 호출
  
  // 변경 후 (권장)
  modifier = Modifier
      .fillMaxWidth()
      .throttleClick(onClick) // 함수 레퍼런스 직접 전달

---

전반적인 평가

이 PR은 마이페이지를 Compose로 성공적으로 전환하고, 에러 처리 메커니즘을 개선하는 등 프로젝트에 긍정적인 영향을 미치는 변경 사항을 포함하고 있습니다. 위에 언급된 몇 가지 제안 사항을 검토하여 코드의 일관성과 가독성을 더욱 높일 수 있습니다.

계속해서 좋은 코드 부탁드립니다!

---

This review was automatically generated using Gemini AI. Please use your judgment when addressing the feedback.

[juhwankim-dev]

UiState.Failure 상태일 때 NRLoading(true)를 표시하는 것은 사용자에게 혼란을 줄 수 있습니다
-> 첫번째 리뷰 반영 완료. 에러 팝업을 띄우고 확인 버튼을 누르면 이전 화면으로 이동하게 수정함

MypageFragment의 uiState.collect에서 해당 스낵바 처리 로직을 삭제하는 것을 고려해 주세요.
-> 두번째 리뷰 반영 완료. 더이상 필요없는 로직이므로 제거함

그 외 리뷰는 중요도가 높지 않거나 의도한 것들이라 반영하지 않음