Conversation
- 인물 합치기 완료 후 뒤로가기 시 빈 화면이 노출되는 이슈 해결 (isInitialized 플래그 도입)
📝 WalkthroughWalkthroughPersonDetailPresenter.kt now includes a LaunchedEffect lifecycle handler that tracks initialization state. When the person object becomes non-null, it sets an internal flag; when person becomes null after initialization with no active loading, it automatically navigates back using the navigator. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/detail/src/main/java/com/example/metasearch/feature/detail/person/PersonDetailPresenter.kt (1)
127-134: Add error handling to ensureisLoadingis always reset.If
savePersonInfo()throws an exception,isLoadingwill remain true indefinitely, leaving the UI stuck in a loading state.🔎 Proposed fix
PersonDetailUiEvent.OnConfirmMergeSave -> { scope.launch { - isLoading = true - savePersonInfo() - showMergeConfirmDialog = false - isLoading = false + try { + isLoading = true + savePersonInfo() + showMergeConfirmDialog = false + } finally { + isLoading = false + } } }
🤖 Fix all issues with AI Agents
In
@feature/detail/src/main/java/com/example/metasearch/feature/detail/person/PersonDetailPresenter.kt:
- Around line 68-74: The LaunchedEffect in PersonDetailPresenter currently only
keys on person so it won't re-run when isLoading flips, causing a race where
person becomes null during loading and navigator.pop() never runs; update the
LaunchedEffect to key on both person and isLoading (e.g., LaunchedEffect(person,
isLoading)) and keep the existing logic that sets isInitialized when person !=
null and calls navigator.pop() when person == null && isInitialized &&
!isLoading so the effect will re-evaluate after the merge at the code altering
isLoading (the merge around lines 129-132) and correctly navigate away.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/detail/src/main/java/com/example/metasearch/feature/detail/person/PersonDetailPresenter.kt
⏰ 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: ci-build
🔇 Additional comments (1)
feature/detail/src/main/java/com/example/metasearch/feature/detail/person/PersonDetailPresenter.kt (1)
66-66: Good approach using an initialization flag.The
isInitializedflag effectively distinguishes between "person hasn't loaded yet" (initial null) and "person was deleted/merged" (null after being non-null), preventing premature navigation during initial load.
| LaunchedEffect(person) { | ||
| if (person != null) { | ||
| isInitialized = true | ||
| } else if (isInitialized && !isLoading) { | ||
| navigator.pop() | ||
| } | ||
| } |
There was a problem hiding this comment.
LaunchedEffect must also key on isLoading to prevent race condition.
The effect only triggers when person changes, not when isLoading changes. If the person becomes null while isLoading is true (during the merge at lines 129-132), the navigation check will fail. When isLoading later becomes false, the effect won't re-trigger, leaving the user on an empty screen—the exact issue this PR aims to fix.
🔎 Proposed fix
- LaunchedEffect(person) {
+ LaunchedEffect(person, isLoading) {
if (person != null) {
isInitialized = true
} else if (isInitialized && !isLoading) {
navigator.pop()
}
}🤖 Prompt for AI Agents
In
@feature/detail/src/main/java/com/example/metasearch/feature/detail/person/PersonDetailPresenter.kt
around lines 68 - 74, The LaunchedEffect in PersonDetailPresenter currently only
keys on person so it won't re-run when isLoading flips, causing a race where
person becomes null during loading and navigator.pop() never runs; update the
LaunchedEffect to key on both person and isLoading (e.g., LaunchedEffect(person,
isLoading)) and keep the existing logic that sets isInitialized when person !=
null and calls navigator.pop() when person == null && isInitialized &&
!isLoading so the effect will re-evaluate after the merge at the code altering
isLoading (the merge around lines 129-132) and correctly navigate away.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
isInitialized 플래그 도입
테스트 영상