Merged
Conversation
- `onFinally`에서 처리하던 로딩 종료 로직을 `onError` 발생 시점에 직접 처리하도록 변경
Closed
1 task
📝 WalkthroughWalkthroughTask 인증 기능의 로딩 상태 관리 및 UI 구조를 개선하는 변경입니다. ViewModel에서 로딩 상태의 라이프사이클을 재정의하여 onStart에서만 true로 설정되고 오류 시에만 false로 리셋되도록 변경했습니다. 상세 화면의 레이아웃을 Scaffold에서 Column으로 전환하고, 로딩 상태에 따라 콘텐츠 렌더링을 조건부로 제어합니다. SwipeableCard 컴포넌트에서 수직 드래그 로직을 제거하여 수평 드래그만 사용하도록 단순화하고, UI 상태에 isLoading 프로퍼티를 추가하며, 메서드명을 toSerializer()에서 toNavArgs()로 변경합니다. 주요 리뷰 포인트🤔 로딩 상태 관리 일관성 검토 필요현재 상황:
왜 문제인가: 개선 제안:
// 제안: 명시적인 상태 관리
launchResult(
flow = uploadFlow,
onStart = { reduce { copy(isLoading = true) } },
onSuccess = { reduce { copy(isLoading = false) } }, // 명시적 추가
onError = { reduce { copy(isLoading = false) } }
)✅ 조건부 렌더링 로직 확인좋은 점: 확인 사항:
📐 제스처 로직 단순화 - 신중한 검토 권장현재 상황: 확인할 사항:
📋 메서드명 변경 - 명확하고 좋습니다좋은 점: 🔗 공개 API 변경 추적확인 필요:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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
🧪 Generate unit tests (beta)
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/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt (1)
64-71:⚠️ Potential issue | 🟠 Major
onFinally에서isLoading = true고정하면 fetch 실패 시에도 기본 UI가 노출됩니다Line 70 로직은 성공/실패와 무관하게 본문 노출 조건을 true로 만듭니다. 초기 fetch 실패 시 기본값 기반 UI가 다시 보일 수 있어, 이번 PR의 깜빡임 방지 의도와 충돌합니다.
성공 시에만 본문 노출 플래그를 올리고, 실패 시에는 false 유지(또는hasFetchedOnce같은 별도 플래그 분리)로 정리하는 게 안전합니다.🔧 제안 수정안
private fun fetchPhotolog() { launchResult( block = { photologRepository.fetchPhotologs(argTargetDate) }, - onSuccess = { reduce { it.toUiState(argGoalId, argBetweenUs, argTargetDate) } }, + onSuccess = { + reduce { + it.toUiState(argGoalId, argBetweenUs, argTargetDate).copy(isLoading = true) + } + }, onError = { + reduce { copy(isLoading = false) } showToast(R.string.task_certification_detail_fetch_photolog_fail, ToastType.ERROR) }, - onFinally = { reduce { copy(isLoading = true) } }, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt` around lines 64 - 71, The current launchResult call sets isLoading = true in onFinally which forces the UI body visible even on fetch failure; move the state change so only successful fetch toggles visibility: remove or stop setting isLoading in the onFinally handler and instead update the UI in onSuccess (e.g., in the onSuccess lambda call reduce { it.toUiState(...).copy(isLoading = true) } or follow reduce { copy(isLoading = true) } inside onSuccess), leaving onError to keep isLoading false (or use a separate hasFetchedOnce flag if needed) so failures don't show the main content.
🧹 Nitpick comments (2)
feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt (1)
24-27:isLoading네이밍이 실제 동작(로드 완료 후 표시)과 반대라 오해를 유발합니다현재 플래그는 “로딩 중”이 아니라 “본문 렌더링 가능” 의미로 사용되고 있습니다. 이 상태로 유지되면 이후 조건문이 뒤집히거나 버그가 재발하기 쉬워집니다.
isContentReady/isInitialized로 이름을 맞추거나, 진짜 로딩 의미를 유지하려면 화면 조건을 반전하는 방식으로 정리해보실까요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt` around lines 24 - 27, The flag isLoading in TaskCertificationDetailUiState is misnamed (it actually represents "content ready" / "initialized"); rename the property to a clear name like isContentReady or isInitialized and update all usages (constructors, copies, condition checks) to match the new semantic; alternatively keep the name but invert its logic where it's consumed (flip boolean checks in UI rendering and any conditionals) so the variable truthfully means "loading" — search for isLoading, the TaskCertificationDetailUiState data class, and any places constructing or pattern-matching it to apply the rename or invert consistently.feature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.kt (1)
126-131: 재등장 시작 방향을 카드 타입이 아니라 실제 스와이프 방향 기준으로 맞추는 것을 권장합니다현재는 카드 종류(
isDisplayingMyPhoto) 기준으로 재등장 위치가 고정됩니다. 그래서 일부 케이스에서 사용자가 민 방향과 애니메이션 방향 체감이 어긋날 수 있습니다. 의도적으로 “카드별 고정 방향”을 원하신 게 아니라면,targetX부호를 재사용하면 더 일관됩니다.🔧 제안 수정안
- val reappearStartX = - if (isDisplayingMyPhoto) { - spec.dismissDistancePx * spec.reappearOffsetRatio - } else { - -spec.dismissDistancePx * spec.reappearOffsetRatio - } + val reappearStartX = -targetX * spec.reappearOffsetRatio🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.kt` around lines 126 - 131, The reappearStartX calculation currently uses isDisplayingMyPhoto to choose direction, which can desync the visual swipe motion; change it to derive sign from the actual swipe target (reuse targetX's sign) so the reappear offset follows the swipe direction. Replace the conditional that sets reappearStartX (using isDisplayingMyPhoto, spec.dismissDistancePx and spec.reappearOffsetRatio) with logic that multiplies spec.dismissDistancePx * spec.reappearOffsetRatio by sign(targetX) (or Math.sign(targetX) equivalent) so reappearStartX always aligns with the swipe target direction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt`:
- Around line 146-149: 업로드의 2단계 실패 시에도 isLoading 플래그를 해제해야 합니다:
TaskCertificationViewModel 내 uploadPhotolog()와 modifyPhotolog()의 onError 핸들러에 현재
2차 API 호출 실패 시 isLoading=true가 유지되는 문제가 있으니 각 onError 콜백에 reduce {
copy(isLoading = false) }를 추가하여 로딩 상태를 해제하고 기존 showToast(...) 호출을 유지하도록 수정하세요.
---
Outside diff comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt`:
- Around line 64-71: The current launchResult call sets isLoading = true in
onFinally which forces the UI body visible even on fetch failure; move the state
change so only successful fetch toggles visibility: remove or stop setting
isLoading in the onFinally handler and instead update the UI in onSuccess (e.g.,
in the onSuccess lambda call reduce { it.toUiState(...).copy(isLoading = true) }
or follow reduce { copy(isLoading = true) } inside onSuccess), leaving onError
to keep isLoading false (or use a separate hasFetchedOnce flag if needed) so
failures don't show the main content.
---
Nitpick comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.kt`:
- Around line 126-131: The reappearStartX calculation currently uses
isDisplayingMyPhoto to choose direction, which can desync the visual swipe
motion; change it to derive sign from the actual swipe target (reuse targetX's
sign) so the reappear offset follows the swipe direction. Replace the
conditional that sets reappearStartX (using isDisplayingMyPhoto,
spec.dismissDistancePx and spec.reappearOffsetRatio) with logic that multiplies
spec.dismissDistancePx * spec.reappearOffsetRatio by sign(targetX) (or
Math.sign(targetX) equivalent) so reappearStartX always aligns with the swipe
target direction.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt`:
- Around line 24-27: The flag isLoading in TaskCertificationDetailUiState is
misnamed (it actually represents "content ready" / "initialized"); rename the
property to a clear name like isContentReady or isInitialized and update all
usages (constructors, copies, condition checks) to match the new semantic;
alternatively keep the name but invert its logic where it's consumed (flip
boolean checks in UI rendering and any conditionals) so the variable truthfully
means "loading" — search for isLoading, the TaskCertificationDetailUiState data
class, and any places constructing or pattern-matching it to apply the rename or
invert consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.ktfeature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.ktfeature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.ktfeature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationCardContent.ktfeature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.ktfeature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.ktfeature/task-certification/src/main/java/com/twix/task_certification/navigation/TaskCertificationGraph.kt
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
chanho0908
commented
Feb 25, 2026
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
이슈 번호
#104
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)
현재 변경된 인터렉션을 구현하긴 했는데 가은이가 이야기하는거 보니까 아직 이것도 픽스는 아닌듯 ㅇㅅㅇ,,,
우선 데모데이를 위해 IOS랑 동일하게만 구현했어
조건부로 UI를 렌더링하게 만들었어
Topbar 까지 조건부 렌더링에 추가하면 아에 빈 화면이 렌더링 되서 뒤로가기도 못하는지라 Topbar는 제외했어
임시로 구현한거라 추후에 로딩 프로그래스바 디자인 나오면 적용하면 될듯 !