NR-122 타이머와 힌트 화면이 viewModel을 공유해서 사용하도록 리팩토링한다.

[juhwankim-dev]

AI Review 워크플로우를 테스트하기 위한 PR입니다.

---

as-is) 타이머 화면에서 힌트 화면으로 넘어갈 때 argument를 넘겨준다.
to-be) 하나의 viewModel을 같이 공유해서 사용하도록 한다. (graph viewModel)

왜?
힌트 화면에서 진행률만 확인하고 힌트 차감없이 되돌아가는 기능이 새로 개발될 예정임
기존에는 힌트를 차감하고 다음 화면으로 넘어가는 방식이였으나
앞으로는 힌트 화면에서 데이터의 변경을 담당하게 될거임.
근데 이걸 setFragmentResult로 하면 다소 코드가 너저분해지고
나중에 기능이 확장될 것을 생각하면 이 방식은 누가 누구에게 무엇을 넘겼는지 파악하기 어려워짐

[github-actions]

🤖 AI Code Review (Gemini)

안녕하세요, NextRoom PR에 대한 코드 리뷰를 시작하겠습니다.

PR 제목: NR-122 타이머와 힌트 화면이 viewModel을 공유해서 사용하도록 리팩토링한다.
PR 설명: AI Review 워크플로우를 테스트하기 위한 PR입니다.
as-is) 타이머 화면에서 힌트 화면으로 넘어갈 때 argument를 넘겨준다.
to-be) 하나의 viewModel을 같이 공유해서 사용하도록 한다. (graph viewModel)
왜? 힌트 화면에서 진행률만 확인하고 힌트 차감없이 되돌아가는 기능이 새로 개발될 예정임. 기존에는 힌트를 차감하고 다음 화면으로 넘어가는 방식이였으나 앞으로는 힌트 화면에서 데이터의 변경을 담당하게 될거임. 근데 이걸 setFragmentResult로 하면 다소 코드가 너저분해지고 나중에 기능이 확장될 것을 생각하면 이 방식은 누가 누구에게 무엇을 넘겼는지 파악하기 어려워짐.

---

이번 PR은 TimerFragment와 HintFragment 간에 Hint 및 SubscribeStatus 데이터를 Navigation Component의 HiltNavGraphViewModel을 통해 공유하도록 리팩토링하는 것을 목표로 합니다. 이는 향후 힌트 화면에서 데이터 변경이 발생할 경우를 대비하여 더 유연하고 관리하기 쉬운 구조를 만드는 데 기여합니다.

전반적으로 변경 사항은 잘 구현되었으며, 제시된 목표를 달성합니다.

💡 Suggestion (제안)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/main/GameViewModel.kt
Line: 12

@HiltViewModel
class GameSharedViewModel @Inject constructor(
    savedStateHandle: SavedStateHandle
) : ViewModel() {
    // ...
}

GameSharedViewModel은 game_navigation 그래프에 스코프된 ViewModel입니다. TimerFragment가 이 그래프의 시작 지점이고, TimerFragmentArgs를 통해 subscribeStatus를 받으므로, GameSharedViewModel의 SavedStateHandle은 TimerFragment의 SavedStateHandle과 동일한 데이터를 가집니다. 이 방식은 그래프 스코프드 ViewModel에 초기 인자를 전달하는 올바른 방법입니다.

다만, GameSharedViewModel의 이름이 GameViewModel이 아닌 GameSharedViewModel로 명확하게 "공유"의 의미를 담고 있는 점은 좋습니다. 하지만 GameSharedViewModel은 ViewModel을 상속하고 있는데, 프로젝트의 다른 ViewModel들은 BaseViewModel을 상속하고 Orbit MVI 패턴을 따르고 있습니다. GameSharedViewModel이 현재는 단순한 데이터 홀더 역할을 하므로 ViewModel을 직접 상속하는 것이 문제가 되지는 않습니다. 하지만 향후 이 ViewModel에서 비즈니스 로직이나 복잡한 상태 관리가 필요해진다면, BaseViewModel을 상속하고 Orbit MVI를 적용하는 것을 고려해볼 수 있습니다. 현재로서는 이대로 유지하는 것이 더 간결할 수 있습니다.

💡 Suggestion (제안)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/main/GameViewModel.kt
Line: 43

    /**
     * Clear hint when returning to timer screen
     */
    fun clearCurrentHint() {
        _currentHint.value = null
    }

clearCurrentHint() 메서드가 추가되었는데, 현재 PR에서는 이 메서드를 호출하는 부분이 없습니다. PR 설명에서 "힌트 화면에서 진행률만 확인하고 힌트 차감없이 되돌아가는 기능이 새로 개발될 예정임"이라고 언급된 것을 보면, 힌트 화면에서 되돌아올 때 이 힌트를 초기화하는 로직이 필요할 것으로 예상됩니다.

향후 HintFragment에서 힌트 확인 후 TimerFragment로 돌아올 때, TimerFragment에서 이 clearCurrentHint()를 호출하여 GameSharedViewModel의 currentHint를 null로 초기화하는 로직을 추가하는 것을 고려해볼 수 있습니다. 예를 들어, TimerFragment의 onResume이나 특정 이벤트 발생 시 호출하는 방식입니다. 이는 HintFragment에서 힌트를 확인만 하고 실제 사용하지 않았을 경우, TimerFragment로 돌아왔을 때 이전 힌트 정보가 남아있지 않도록 하는 데 도움이 됩니다.

💡 Suggestion (제안)

File: presentation/src/main/java/com/nextroom/nextroom/presentation/ui/hint/HintFragment.kt
Line: 47

        // Shared ViewModel에서 hint 수집
        viewLifecycleOwner.repeatOnStarted {
            gameSharedViewModel.currentHint.collect { hint ->
                hint?.let { viewModel.setHint(it) }
            }
        }

        // Shared ViewModel에서 subscribeStatus 수집
        viewLifecycleOwner.repeatOnStarted {
            gameSharedViewModel.subscribeStatus.collect { subscribeStatus ->
                viewModel.setSubscribeStatus(subscribeStatus)
            }
        }

HintFragment에서 gameSharedViewModel의 currentHint와 subscribeStatus를 수집하여 HintViewModel에 전달하는 방식은 올바릅니다. repeatOnStarted를 사용하여 Lifecycle-aware하게 Flow를 수집하는 것도 좋습니다.

다만, currentHint는 MutableStateFlow<Hint?>(null)로 초기화되므로, HintFragment가 처음 생성될 때 null이 먼저 발행될 수 있습니다. hint?.let { viewModel.setHint(it) }으로 null 체크를 하고 있어 문제는 없지만, HintViewModel의 초기 상태가 HintState()이므로, HintFragment가 STARTED 상태가 되고 gameSharedViewModel에서 실제 힌트가 발행될 때까지 HintViewModel은 빈 Hint 객체를 가지고 있게 됩니다. 이는 UI 렌더링에 미묘한 지연을 줄 수 있지만, 일반적으로 사용자에게 인지될 정도는 아닐 것입니다.

만약 이 초기 상태가 중요하거나 UI 깜빡임이 우려된다면, HintViewModel의 HintState 초기화를 HintFragment에서 gameSharedViewModel의 초기 값을 사용하여 직접 수행하는 방법도 고려할 수 있습니다. 하지만 현재 구현 방식도 유효하며, 더 간결합니다.

---

긍정적인 측면 (Positive Aspects)

• Clean Architecture 준수: GameSharedViewModel이 presentation 모듈에 위치하며 UI 관련 상태를 관리하는 것은 Clean Architecture 원칙에 부합합니다.
• Hilt NavGraph ViewModel 활용: hiltNavGraphViewModels를 사용하여 Navigation Graph 스코프의 ViewModel을 생성하고 공유하는 방식은 Navigation Component와 Hilt를 함께 사용하는 모범 사례입니다.
• 데이터 흐름 개선: Fragment Argument 대신 공유 ViewModel을 사용함으로써, 데이터의 소스가 명확해지고 향후 힌트 화면에서 데이터 변경이 발생했을 때 TimerFragment가 이를 쉽게 관찰할 수 있는 기반을 마련했습니다.
• 안전한 Navigation: safeNavigate() 확장 함수를 계속 사용하여 빠른 클릭으로 인한 IllegalArgumentException을 방지하는 점이 좋습니다.
• Lifecycle-aware Flow Collection: repeatOnStarted를 사용하여 Flow를 수집하는 것은 메모리 누수를 방지하고 리소스 효율성을 높이는 좋은 패턴입니다.
• 코드 가독성 및 유지보수성 향상: TimerEvent.OnOpenHint에서 subscribeStatus 인자가 제거되고, HintFragment 및 HintViewModel에서 SavedStateHandle을 통한 인자 추출 로직이 제거되어 코드가 더 간결해졌습니다.

---

최종 의견

이번 PR은 TimerFragment와 HintFragment 간의 데이터 공유 방식을 효과적으로 리팩토링하여, 향후 기능 확장을 위한 견고한 기반을 마련했습니다. HiltNavGraphViewModel의 적절한 사용과 Flow를 통한 데이터 전달 방식은 프로젝트의 아키텍처 원칙을 잘 따르고 있습니다. 위에 제시된 제안들은 현재 구현의 개선점이라기보다는 향후 고려사항이나 미세한 최적화에 가깝습니다. 현재 변경 사항은 머지하기에 충분히 좋습니다.

수고하셨습니다!

---

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