[fix] #89: 랜덤 포즈 화면 개선#93
Conversation
포즈피드 화면에 다시 진입했을 때, 포즈 목록이 이미 존재하면 초기 포즈를 다시 불러오는 API가 호출되지 않도록 수정했습니다.
랜덤 포즈 추천 화면의 첫 방문 여부를 저장하고 확인하기 위해, `UserDataStore`를 새로 추가했습니다. 이를 통해 `isFirstVisitRandomPose` 값을 `Flow`로 관찰하고, 값을 업데이트하는 기능을 구현했습니다.
랜덤 포즈 추천 화면에 처음 방문하는 사용자를 위해 튜토리얼을 표시하는 기능을 추가했습니다. `UserRepository`를 사용하여 첫 방문 여부를 확인하고, 첫 방문일 경우 튜토리얼을 노출한 뒤 방문 상태를 업데이트합니다.
포즈를 불러오는 데 실패했을 때 사용자에게 "포즈를 불러오는데 실패했어요"라는 토스트 메시지를 표시하는 기능을 추가했습니다.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough랜덤 포즈 기능에 첫 방문 추적(DataStore 기반 Flow), 포즈 조회 로직 단순화(재시도 제거, excludeIds 쿼리 도입), 스크랩 동기화 개선(뷰모델 즉시 UI 반영·서버 동기화), API 예외 타입 추가 및 관련 예외 삭제가 적용되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as RandomPose VM
participant UR as UserRepository
participant DS as DataStore
participant PR as PoseRepository
participant API as PoseService
Note over VM,UR: 초기 진입 흐름
VM->>UR: checkFirstVisit()
UR->>DS: read hasVisitedRandomPose (Flow.first)
DS-->>UR: Boolean (false)
UR->>DS: setRandomPoseVisited()
DS-->>UR: 저장 완료
UR-->>VM: 첫 방문 처리 완료
VM->>PR: getMultipleRandomPose(excludeIds=[])
PR->>API: getRandomPose(excludeIds="")
API-->>PR: PoseDetailResponse
PR-->>VM: List<Pose>
VM->>VM: UI에 포즈 반영
Note over VM,PR: 스크랩 토글 흐름
VM->>VM: handleScrapToggle(poseId, isScrapped) — 즉시 UI 업데이트
VM->>PR: updateScrapStatus(poseId, isScrapped)
PR->>API: updatePose(...)
alt 성공
API-->>PR: 200 OK
PR-->>VM: 성공
else NO_MORE_RANDOM_POSE / 실패
API-->>PR: Error / ClientApiException
PR-->>VM: 실패 알림
VM->>VM: UI 상태 롤백
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 분 Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
랜덤 포즈 추천 화면의 첫 방문 여부를 확인하는 로직의 네이밍과 흐름을 개선했습니다. `UserRepository`의 `isFirstVisitRandomPose`를 `hasVisitedRandomPose`로 변경하고, `setFirstVisitRandomPose(value: Boolean)` 메서드를 `markRandomPoseAsVisited()`로 수정하여 방문 기록을 남기는 역할임을 명확히 했습니다. 이와 함께 `RandomPoseViewModel`의 관련 로직도 수정하여 가독성을 높였습니다.
![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/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt (1)
157-171:⚠️ Potential issue | 🔴 Critical프리페치된 포즈가
committedScraps에 추가되지 않아 스크랩 변경이 유실됩니다.
handleMoveNext에서 프리페치된 포즈는poseList에만 추가되고committedScraps에는 추가되지 않습니다. 이로 인해:
handleScrapToggle(Line 100-101):committedScrap == null→return@launch로 네트워크 호출이 스킵됩니다.onCleared(Line 238):committedScrap != null조건 실패 → 동기화도 수행되지 않습니다.결과적으로 프리페치된 포즈의 스크랩 토글은 UI에서만 반영되고 서버에 전혀 저장되지 않습니다.
제안된 수정
).onSuccess { pose -> - reduce { copy(poseList = (poseList + pose).toImmutableList()) } + reduce { + copy( + poseList = (poseList + pose).toImmutableList(), + committedScraps = committedScraps + (pose.id to pose.isScrapped), + ) + } }.onFailure { error ->
🤖 Fix all issues with AI agents
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt`:
- Around line 183-193: The condition in checkFirstVisit is inverted: instead of
hiding the tutorial when hasVisitedRandomPose is false, flip the branch so that
when userRepository.hasVisitedRandomPose.first() is false (first visit) you call
userRepository.markRandomPoseAsVisited() and leave isShowTutorial true, and when
it is true (not first visit) call reduce { copy(isShowTutorial = false) };
update the branches in checkFirstVisit accordingly to use these symbols
(checkFirstVisit, userRepository.hasVisitedRandomPose, markRandomPoseAsVisited,
reduce { copy(isShowTutorial = false) }).
🧹 Nitpick comments (3)
feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/navigation/PoseEntryProvider.kt (1)
87-88:RandomPoseRoute의 기본 파라미터 제거 권장
RandomPoseViewModel이@AssistedInject와 Factory 패턴으로 구현되었기 때문에,RandomPoseRoute의 기본 파라미터인viewModel: RandomPoseViewModel = hiltViewModel()은 더 이상 호환되지 않습니다. 현재는PoseEntryProvider에서 명시적으로 viewModel을 전달하므로 런타임 오류는 발생하지 않으나, 기본값 파라미터를 제거하여 향후 호출 시 실수를 방지하는 것이 좋습니다.core/data-api/src/main/java/com/neki/android/core/dataapi/datastore/DataStoreKey.kt (1)
9-9: 키 이름(IS_FIRST_VISIT)과 실제 사용 의미(hasVisited)가 반대입니다.
IS_FIRST_VISIT_RANDOM_POSE는 "첫 방문인지"를 의미하지만, 실제로는markRandomPoseAsVisited()에서true로 설정되며hasVisitedRandomPose로 노출됩니다. 즉,true일 때 "방문한 적 있음"을 뜻하게 되어 키 이름의 의미와 반대입니다.이 혼동이
RandomPoseViewModel.checkFirstVisit()의 조건 반전 버그의 원인으로 보입니다. 키 이름을HAS_VISITED_RANDOM_POSE로 변경하는 것을 권장합니다.제안된 수정
- val IS_FIRST_VISIT_RANDOM_POSE = booleanPreferencesKey("is_first_visit_random_pose") + val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("has_visited_random_pose")feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt (1)
174-181:checkFirstVisit와fetchInitialPoses가 독립적인 코루틴으로 동시에 실행됩니다.
checkFirstVisit는viewModelScope.launch로 비동기 실행되고,fetchInitialPoses도 바로 이어서 호출됩니다. 첫 방문 시 튜토리얼 표시 여부가 결정되기 전에 데이터 로딩이 완료될 수 있어, 튜토리얼이 잠깐 깜빡이는 현상이 발생할 수 있습니다.필요하다면
checkFirstVisit를 먼저 완료한 후fetchInitialPoses를 호출하는 순차 실행을 고려해 보세요.순차 실행 예시
private fun fetchInitialData( state: RandomPoseUiState, reduce: (RandomPoseUiState.() -> RandomPoseUiState) -> Unit, postSideEffect: (RandomPoseEffect) -> Unit, ) { - checkFirstVisit(reduce) - fetchInitialPoses(state, reduce, postSideEffect) + viewModelScope.launch { + val hasVisited = userRepository.hasVisitedRandomPose.first() + if (!hasVisited) { + userRepository.markRandomPoseAsVisited() + } else { + reduce { copy(isShowTutorial = false) } + } + fetchInitialPoses(state, reduce, postSideEffect) + } }이 경우
checkFirstVisit메서드와fetchInitialPoses내부의viewModelScope.launch를 적절히 리팩터링해야 합니다.
`IS_FIRST_VISIT_RANDOM_POSE`에서 `HAS_VISITED_RANDOM_POSE`로 DataStore 키 이름을 변경하여, 방문 여부를 확인하는 변수임을 명확히 했습니다. 이에 따라 `UserRepositoryImpl`에서도 변경된 키를 사용하도록 수정했습니다.
랜덤 포즈 추천 화면 첫 방문 시에만 튜토리얼이 노출되도록 `hasVisitedRandomPose`의 조건절을 수정했습니다.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt`:
- Around line 96-124: The prefetch path doesn't populate committedScraps so
scrap toggles for prefetched poses get skipped; update handleMoveNext to add an
entry into committedScraps when you append a prefetched pose (add
committedScraps = committedScraps + (poseId to pose.isScrapped) inside the same
reduce that appends to poseList), and ensure onCleared and the scrapJobs flow
(scrapJobs map handling and the launch block in RandomPoseViewModel where
committedScrap is read) treat that entry as the source of truth (so the check
`committedScrap == null` no longer causes valid prefetched toggles to be
skipped). This ensures the prefetched pose state is initialized and
flush-on-clear works correctly; touch handleMoveNext, committedScraps usage, the
reduce call that appends to poseList, the scrapJobs launch block, and onCleared
to apply the change.
- Around line 174-181: checkFirstVisit is launched asynchronously so its result
can race with fetchInitialPoses causing isShowTutorial to be applied after
loading; make the sequence deterministic by running checkFirstVisit
synchronously before starting fetchInitialPoses: either convert checkFirstVisit
to a suspend function (or remove its internal viewModelScope.launch) and await
its completion in fetchInitialData, or wrap both calls in a single
viewModelScope.launch and call checkFirstVisit first then fetchInitialPoses (or
make fetchInitialPoses suspend and call it next); ensure references to
isShowTutorial updates occur before invoking fetchInitialPoses to avoid the
tutorial flicker.
🧹 Nitpick comments (1)
core/data-api/src/main/java/com/neki/android/core/dataapi/datastore/DataStoreKey.kt (1)
9-9: 상수명과 실제 저장 키 문자열 간 의미 불일치
HAS_VISITED_RANDOM_POSE는 "방문한 적 있음"을 의미하지만, 실제 저장되는 키 문자열은"is_first_visit_random_pose"("첫 방문 여부")로 반대 의미입니다. 커밋 히스토리를 보면 상수명이 리네이밍되었지만 내부 문자열은 업데이트되지 않은 것으로 보입니다.기능상 버그는 아니지만, 향후 유지보수 시 혼란을 줄 수 있으므로 키 문자열도 맞춰주는 것을 권장합니다.
♻️ 제안
- val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("is_first_visit_random_pose") + val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("has_visited_random_pose")
⚠️ 이미 배포된 버전이 있다면, 키 문자열 변경 시 기존 사용자의 저장된 값이 무시됩니다. 마이그레이션 필요 여부를 확인하세요.
클라이언트에서 `maxRetry`를 통해 중복 포즈를 제거하던 로직을 서버 API에 `excludeIds` 파라미터를 직접 전달하여 처리하도록 변경했습니다. 이를 통해 불필요한 클라이언트 측 재시도 로직과 파라미터를 제거하고, API 호출을 최적화했습니다.
기존에 사용하던 커스텀 예외(`RandomPoseRetryExhaustedException`)를 삭제하고, `HttpException`을 사용하여 더 이상 추천할 포즈가 없을 때의 예외 처리를 변경했습니다. API로부터 특정 에러 코드(400)를 받으면, 사용자에게 실패 토스트 메시지를 표시하도록 로직을 수정했습니다.
더 이상 추천할 새로운 포즈가 없을 경우, `hasNewPose` 상태를 `false`로 업데이트하여 불필요하게 다음 포즈를 미리 캐싱하려는 API 요청을 보내지 않도록 수정했습니다.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@core/data-api/src/main/java/com/neki/android/core/dataapi/repository/PoseRepository.kt`:
- Around line 9-10: In PoseRepositoryImpl, don't treat any HttpException with
code == NO_MORE_RANDOM_POSE as "no more poses" unconditionally; instead inspect
the response body or error payload (e.g., response.body()?.errorType or parsed
error JSON) and only map to the "no more poses" domain result when that
errorType/marker equals the agreed token (e.g., "NO_MORE_POSES"); keep
rethrowing other 400s. Update the catch/handling around the network call that
currently checks NO_MORE_RANDOM_POSE to add this extra validation, and
coordinate with the server to add a distinct errorType field (or use a separate
domain-specific 4xx code) if not already present.
In
`@core/data/src/main/java/com/neki/android/core/data/repository/impl/PoseRepositoryImpl.kt`:
- Line 16: The Retrofit HttpException import and catch are wrong for
PoseRepositoryImpl: replace the import of retrofit2.HttpException and the catch
(e: HttpException) block in the method that calls PoseService with a Ktor
exception type (preferably io.ktor.client.plugins.ClientRequestException or
io.ktor.client.plugins.ResponseException) so the 400 NO_MORE_RANDOM_POSE branch
is reachable; update imports accordingly and in the new catch check
e.response.status.value (or e.response.status) for 400 and map it to the
NO_MORE_RANDOM_POSE handling just as before.
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseContract.kt`:
- Line 13: The hasNewPose flag is never set true, so prefetch never triggers;
update the state in RandomPoseViewModel by setting hasNewPose = true after
successful initial load in fetchInitialPoses and also set hasNewPose = true when
a prefetch call completes successfully (the success branch that currently
updates poses). Specifically, locate RandomPoseContract.hasNewPose and modify
the reducers/intent handlers used by fetchInitialPoses and the prefetch success
handler so they emit state copies with hasNewPose = true (and preserve other
state fields), ensuring handleMoveNext sees state.hasNewPose === true and can
run the prefetch logic.
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt`:
- Around line 166-169: When handling HttpException with code NO_MORE_RANDOM_POSE
in RandomPoseViewModel, update the side effect message to reflect the normal "no
more poses" state instead of an error: keep the reduce { copy(hasNewPose =
false) } behavior but change postSideEffect(RandomPoseEffect.ShowToast(...)) to
show a neutral/info message such as "모든 포즈를 불러왔어요" rather than "포즈를 불러오는데
실패했어요", so users see the correct guidance when NO_MORE_RANDOM_POSE occurs.
🧹 Nitpick comments (2)
core/data/src/main/java/com/neki/android/core/data/repository/impl/PoseRepositoryImpl.kt (1)
88-105: 서버가excludeIds에도 불구하고 중복 포즈를 반환하면 요청한 수보다 적은 결과가 반환됩니다.
repeat(poseSize)내에서 중복 포즈가 반환되면 (Line 101-104) 해당 반복은 소비되지만result에는 추가되지 않습니다. 서버가excludeIds를 올바르게 처리한다면 문제없지만, 그렇지 않은 경우poseSize보다 적은 포즈가 반환될 수 있습니다.서버 측에서
excludeIds를 정확히 처리한다면 Lines 101-104의 중복 체크는 불필요한 방어 코드이므로, 의도에 맞는지 확인해 주세요.feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt (1)
70-76:ScrapChanged(상세 화면 결과)가 디바운스 경로를 불필요하게 거침상세 화면에서 이미 서버에 반영된 스크랩 변경(
ScrapChanged)이handleScrapToggle을 통해 500ms 딜레이 후updateScrap을 재호출합니다. 이는 불필요한 중복 API 호출이며, 만약 사용자가 500ms 내에 수동 토글하면 두 변경이 충돌할 수 있습니다.상세 화면에서 온 결과는 UI +
committedScraps만 동기화하고 API 호출을 스킵하는 별도 경로가 적절합니다.제안된 수정
- is RandomPoseIntent.ScrapChanged -> handleScrapToggle(intent.poseId, intent.isScrapped, reduce) + is RandomPoseIntent.ScrapChanged -> syncScrapFromDetail(intent.poseId, intent.isScrapped, reduce)private fun syncScrapFromDetail( poseId: Long, newScrapStatus: Boolean, reduce: (RandomPoseUiState.() -> RandomPoseUiState) -> Unit, ) { scrapJobs[poseId]?.cancel() reduce { copy( poseList = poseList.map { pose -> if (pose.id == poseId) pose.copy(isScrapped = newScrapStatus) else pose }.toImmutableList(), committedScraps = committedScraps + (poseId to newScrapStatus), ) } }
서버 및 클라이언트 오류 처리를 위한 `ServerApiException`과 `ClientApiException`을 추가했습니다. 각 예외 클래스는 HTTP 상태 코드와 메시지를 포함하여 API 통신 오류를 구체적으로 나타냅니다.
기존 `HttpException`을 사용하여 처리하던 랜덤 포즈 관련 예외를 커스텀 `ClientApiException`으로 대체하여 처리하도록 수정했습니다. 모든 포즈를 불러왔을 때(NO_MORE_RANDOM_POSE), "포즈를 불러오는데 실패했어요"라는 토스트 메시지가 나타나던 문제를 "모든 포즈를 불러왔어요"로 수정하여 사용자에게 더 정확한 피드백을 제공합니다.
포즈피드에서 포즈를 스크랩했을 때, 해당 상태가 즉시 UI에 반영되지 않던 문제를 해결했습니다. 새로운 포즈를 불러올 때 `committedScraps` 목록에도 스크랩 상태를 함께 업데이트하여 데이터 정합성을 맞춥니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/data/src/main/java/com/neki/android/core/data/repository/impl/PoseRepositoryImpl.kt`:
- Line 75: The current building of excludeIdsString in PoseRepositoryImpl uses
excludeIds.joinToString(",") which yields an empty string for an empty set;
update the logic so that when excludeIds.isEmpty() you pass null instead of ""
(or change PoseService's excludeIds parameter to nullable first), e.g. compute
excludeIdsString = excludeIds.takeIf { it.isNotEmpty() }?.joinToString(",") and
pass that to the PoseService call; ensure PoseService's parameter is nullable
(or the API explicitly accepts empty string) and update any call sites
accordingly to avoid sending a bare empty string to the server.
🧹 Nitpick comments (4)
detekt-config.yml (1)
55-56:SwallowedException규칙을 전역으로 비활성화하면 예외가 조용히 무시되는 경우를 감지하지 못합니다.새로 도입된 예외 처리 코드(
ClientApiException,ServerApiException등)에서 의도적으로 예외를 catch하는 부분이 있을 수 있지만, 규칙 자체를 전역적으로 끄는 것보다는 해당 catch 블록에@Suppress("SwallowedException")을 사용하는 것이 더 안전합니다. 전역 비활성화 시 의도하지 않은 예외 무시가 정적 분석에서 누락될 수 있습니다.feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt (1)
97-125:ScrapChanged(디테일 화면 복귀) 시 불필요한 API 재호출 가능성
ScrapChanged인텐트는 디테일 화면에서 이미 서버에 반영된 스크랩 변경을 전달받는 경우인데,handleScrapToggle을 그대로 호출하면 500ms 디바운스 후 동일한updateScrapAPI를 다시 호출합니다.현재
committedScraps에 저장된 값(구 상태)과newScrapStatus(신 상태)가 다르므로 API 호출이 발생합니다. 멱등성이 보장되면 기능적 문제는 아니지만, 불필요한 네트워크 비용이 발생합니다.
ScrapChanged경로에서는 UI 업데이트 +committedScraps갱신만 수행하고 API 호출을 건너뛰는 것이 더 효율적입니다.제안된 수정 예시
- is RandomPoseIntent.ScrapChanged -> handleScrapToggle(intent.poseId, intent.isScrapped, reduce) + is RandomPoseIntent.ScrapChanged -> { + // 디테일 화면에서 이미 서버 반영 완료, UI + committedScraps만 동기화 + reduce { + copy( + poseList = poseList.map { pose -> + if (pose.id == intent.poseId) pose.copy(isScrapped = intent.isScrapped) + else pose + }.toImmutableList(), + committedScraps = committedScraps + (intent.poseId to intent.isScrapped), + ) + } + }core/data/src/main/java/com/neki/android/core/data/repository/impl/PoseRepositoryImpl.kt (1)
96-113:repeat(poseSize)사용 시, 중복 발생 시 요청한 수보다 적은 결과를 반환할 수 있습니다.Line 109의 방어적 중복 체크(
pose.id !in collectedIds)에서 중복이 발생하면 해당 반복은 result에 추가하지 않고 넘어가지만,repeat(poseSize)는 고정 횟수만큼만 반복합니다. 서버가excludeIds를 올바르게 처리한다면 실제로 중복이 발생할 가능성은 낮지만, 만약 발생한다면result.size < poseSize인 채로 반환됩니다.현재 동작이 의도된 것이라면 괜찮지만, 정확히
poseSize개를 채워야 한다면while (result.size < poseSize)루프로 변경하는 것이 좋습니다 (무한 루프 방지를 위한 최대 시도 횟수 포함).core/data/src/main/java/com/neki/android/core/data/repository/impl/UserRepositoryImpl.kt (1)
22-25: DataStore 키 이름과 프로퍼티 이름의 의미가 반대입니다.키 이름
"is_first_visit_random_pose"는 "첫 방문인가?"를 의미하고, 프로퍼티 이름hasVisitedRandomPose는 "방문한 적이 있는가?"를 의미합니다. 값이true일 때 키는 "첫 방문이다", 프로퍼티는 "방문한 적 있다"로 반대 의미가 됩니다.내부적으로만 사용되므로 동작에 문제는 없지만, 향후 유지보수 시 혼동을 줄 수 있습니다. 키 이름을 프로퍼티와 일치시키는 것을 권장합니다.
제안
- private val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("is_first_visit_random_pose") + private val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("has_visited_random_pose")
⚠️ 기존에 저장된 데이터가 있다면 키 변경 시 마이그레이션이 필요합니다. 이 PR이 최초 도입이라면 문제없습니다.Also applies to: 45-47
Ojongseok
left a comment
Ojongseok
left a comment
There was a problem hiding this comment.
Q. DataStore 에서 불린값을 가져오는 부분을 함수가 아닌 변수로 선언했습니다. 데이터스트림이 아니기 때문에 Flow 대신 단순 Boolean 값만 사용하도록 수정할까요??
Boolean타입의 함수, Flow<Boolean>타입의 변수 중 선택해야 하는 것이죠?
개인적으로 단순 플래그로 사용하기에 사용하는 구문에서 first()키워드를 제거할 수 있는 Boolean 타입의 함수가 좋을 것 같습니다!
|
|
||
| interface UserRepository { | ||
| val hasVisitedRandomPose: Flow<Boolean> | ||
| suspend fun markRandomPoseAsVisited() |
There was a problem hiding this comment.
저 같은 경우에는 온보딩 완료 플래그로 바꾸는 함수명을 setOnboardingCompleted()로 작성했는데 로컬 플래그를 설정하는 함수명을 조금 더 직관적인 setRandomPoseVisited()or markRandomPoseVisited()는 어떨까요?
#91 (comment) 해당 코멘트에서 말씀해주신 것 처럼 get플래그 : has + pp + 명사, set플래그에 대한 규칙도 정하면 좋을 것 같네요
There was a problem hiding this comment.
setRandomPoseVisited() 이런 식으로 통일하겠습니다! a853151
| import com.neki.android.core.model.SortOrder | ||
| import kotlinx.coroutines.flow.Flow | ||
|
|
||
| const val NO_MORE_RANDOM_POSE = 400 |
There was a problem hiding this comment.
에러코드에 대한 정의는 ApiException과 동일한 경로인 core:common에 있는게 어떨까요?
There was a problem hiding this comment.
common에 저 변수를 선언해버리면,
400일 경우 랜덤포즈가 없다,는 내용이 공통적이어야할 것 같다고 생각됩니다.
한 곳으로 모으는 부분에 대해선 찬성이나,
음... common 혹은 network 모듈에 Exception 형태로 하는 건 어떨까요??
저 NO MORE RANDOM POSE 400은 해당 부분에서만 사용하니 냅두고 private 으로 변경한 다음에,
NoMoreRandomPoseException 으로 만들어서 뷰모델에서 참조하면 될 것 같습니다.
There was a problem hiding this comment.
NetworkModule에 선언한다면 feature에서 core:data 의존성이 없어 viewmodel에서 접근이 안되지 않나요?
말씀하신대로 private으로 변경 혹은 제거한 후 아래 코멘트에서 말씀드린 것 처럼 발생 가능한 Exception 각각을 정의하고 throw하면 error is ClientApiException && error.code == NO_MORE_RANDOM_POSE ViewModel에서 현재처럼 NO_MORE_RANDOM_POSE에 접근할 필요가 없게 될 것 같습니다.
| package com.neki.android.core.common.exception | ||
|
|
||
| class ServerApiException( | ||
| val code: Int, | ||
| message: String, | ||
| ) : Throwable(message) | ||
|
|
||
| class ClientApiException( | ||
| val code: Int, | ||
| message: String, | ||
| ) : Throwable(message) |
There was a problem hiding this comment.
Exception을 서버와 클라이언트 단위로 구분하지 않고, 예측할 수 있는 Exception 각각을 정의하는건 어떨까요?
그리고 결국 저희는 ClientRequestException 같은 Exception에 대한 예외처리를 하기 때문에 특별한 이유가 아니라면? Throwable의 서브타입인 Exception을 상속받아 구현해도 충분할 것 같습니다.
sealed class NekiCustomException(
open val code: Int,
override val message: String,
) : Exception(message)
// 서버 예외?
class ServerCustomException(
override val code: Int,
message: String,
) : NekiCustomException(code, message)
class NoMorePoseException(
override val code: Int,
message: String,
) : NekiCustomException(code, message)
ViewModel이나 RepositoryImpl에서 예외를 분기할 때 가독성이 좋아지기도 하고, 앞으로 추가될 Exception에 대비하기도 좋을 것 같습니다!
There was a problem hiding this comment.
이 부분은 새롭게 이슈업해서 논의해보면 좋을 것 같습니다!
sealed 로 하는 것 좋아보여요
There was a problem hiding this comment.
넵넵 현재 Exception으로 구분할 예외가 랜덤포즈가 유일하니 현행 유지하고, 차차 서버 스펙에 대해서도 의논해보고 개선하시죠!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt (2)
147-151:⚠️ Potential issue | 🟡 Minor마지막 인덱스 도달 시 "모든 포즈를 불러왔어요" 메시지가 부정확할 수 있습니다.
hasNewPose가true인 상태에서도currentIndex >= lastIndex이면 "모든 포즈를 불러왔어요"가 표시됩니다. 프리페치가 느린 네트워크에서 완료되지 않은 경우, 서버에 더 많은 포즈가 있음에도 이 메시지가 보일 수 있습니다.hasNewPose상태에 따라 메시지를 분기하거나, 프리페치 중임을 안내하는 것을 고려해 보세요.
233-247:⚠️ Potential issue | 🟡 Minor
onCleared에서viewModelScope취소와applicationScope플러시 간 미묘한 타이밍 이슈
onCleared호출 시viewModelScope가 취소되면서 진행 중인scrapJobs도 취소됩니다. 서버 호출은 성공했지만committedScraps업데이트 전에 취소된 경우,onCleared에서 동일한 업데이트를applicationScope로 중복 전송할 수 있습니다. 스크랩 API가 멱등성을 가지면 실질적 문제는 없지만, 불필요한 네트워크 호출이 발생할 수 있습니다.
🧹 Nitpick comments (2)
core/data/src/main/java/com/neki/android/core/data/repository/impl/UserRepositoryImpl.kt (1)
46-46: DataStore 키 이름이 프로퍼티 의미와 반대입니다.
HAS_VISITED_RANDOM_POSE는 방문 여부(true= 방문함)를 저장하지만, 실제 키 문자열은"is_first_visit_random_pose"로, "첫 방문인지 여부"를 의미합니다. 의미가 반전되어 있어 향후 유지보수 시 혼란을 줄 수 있습니다.제안된 수정
- private val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("is_first_visit_random_pose") + private val HAS_VISITED_RANDOM_POSE = booleanPreferencesKey("has_visited_random_pose")core/data-api/src/main/java/com/neki/android/core/dataapi/repository/UserRepository.kt (1)
7-8:Flow<Boolean>vssuspend fun: PR 설명에서 언급된 질문에 대해현재
hasVisitedRandomPose는 ViewModel에서.first()로 한 번만 읽히므로,suspend fun hasVisitedRandomPose(): Boolean이 의도를 더 명확히 전달합니다. 다만 DataStore의data프로퍼티가Flow를 반환하므로 현재 구현도 관용적입니다. 향후 여러 곳에서 반응형으로 관찰할 필요가 없다면suspend fun으로 단순화하는 것을 고려해 보세요.
2 participants
🔗 관련 이슈
📙 작업 설명
poseList가 이미 있으면 초기 로딩 스킵), 이 방법으로 인해 스크랩 버그도 수정됨UserDataStore추가 (IS_FIRST_VISIT_RANDOM_POSE키)checkFirstVisit로직 추가)hasNextPost기본값 수정DataStoreKey에서HAS_VISITED_RANDOM_POSE키를UserRepositorycompanion object로 이동💬 추가 설명 or 리뷰 포인트 (선택)
Q. DataStore 에서 불린값을 가져오는 부분을 함수가 아닌 변수로 선언했습니다. 데이터스트림이 아니기 때문에 Flow 대신 단순 Boolean 값만 사용하도록 수정할까요??
Summary by CodeRabbit
New Features
Refactor
Bug Fixes