[NDGL-124] 인기 여행 목록 페이지 paging 구현 및 에러 뷰 추가#41
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough새로운 재사용 가능한 공통 에러 UI 컴포넌트를 추가하고, 인기 여행 목록 기능에 페이지네이션 및 스낵바 지원을 구현합니다. 인기 여행 목록 계약은 상태 관리 개선을 위해 봉인된 클래스 상태로 리팩토링되며, 템플릿 검색 화면은 새로운 공통 에러 컴포넌트를 사용하도록 업데이트됩니다. Changes
Sequence DiagramsequenceDiagram
actor User
participant Screen as PopularTravelListScreen
participant ViewModel as PopularTravelListViewModel
participant Repository as TravelTemplateRepository
participant API as Backend API
participant Snackbar as SnackbarHost
User->>Screen: 목록 아이템 보기
Screen->>ViewModel: 초기 상태 (Loading)
ViewModel->>Repository: getAllPopularTravelTemplates(page=0)
Repository->>API: 인기 여행 조회 (page=0)
API-->>Repository: 여행 목록 반환
ViewModel->>ViewModel: Success 상태 구성
Screen->>Screen: PopularTravelListContent 렌더링
User->>Screen: 목록 스크롤 (끝에 도달)
Screen->>ViewModel: onLoadMore(nextPage=1)
ViewModel->>ViewModel: isLoadingMore 확인
ViewModel->>Repository: getPopularTravelTemplates(page=1)
Repository->>API: 다음 페이지 조회 (page=1)
API-->>Repository: 추가 여행 목록 반환
ViewModel->>ViewModel: 기존 상태에 항목 병합
Screen->>Screen: LazyColumn 업데이트
alt 로드 실패
API-->>Repository: 에러 응답
ViewModel->>ViewModel: Error 상태 생성
ViewModel->>ViewModel: ShowSnackBar 사이드 이펙트 발행
Screen->>Snackbar: 에러 메시지 표시
User->>Screen: onRetryClick 클릭
ViewModel->>ViewModel: ClickRetry 처리 → loadMore 재시도
end
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt (1)
52-58:⚠️ Potential issue | 🟠 Major초기 템플릿 로드 실패가 성공 상태로 삼켜집니다.
getOrNull()사용으로 실패가null로 변환되고, Line 90에서 항상Success를 emit하고 있습니다. 전체 요청 실패 시 에러 뷰 대신 빈 목록이 노출될 수 있습니다. 최소한 “전체 실패” 케이스는PopularTravelListState.Error로 전환하는 분기가 필요합니다.🔧 수정 예시
val allResult = allTemplateDeferred.await() + val programResults = popularTemplateDeferred.awaitAll() + + if (allResult == null && programResults.all { (_, result) -> result == null }) { + reduce { PopularTravelListState.Error } + return@launch + } + val allTravels = buildList { allResult?.content?.forEach { add(PopularTravelListItem.Travel(it.toTravelContent())) } if (allResult?.hasNext == true) add(PopularTravelListItem.Loading(nextPage = 1)) }.toImmutableList() val travelsByProgram = mutableMapOf<Long, ImmutableList<PopularTravelListItem>>() - popularTemplateDeferred.awaitAll().forEach { (program, result) -> + programResults.forEach { (program, result) -> travelsByProgram[program.id] = buildList { result?.content?.forEach { add(PopularTravelListItem.Travel(it.toTravelContent())) } if (result?.hasNext == true) add(PopularTravelListItem.Loading(nextPage = 1)) }.toImmutableList() }Also applies to: 75-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt` around lines 52 - 58, The current use of suspendRunCatching(...).getOrNull() when calling travelTemplateRepository.getAllPopularTravelTemplates() and getPopularTravelTemplates(...) swallows failures and causes the ViewModel to always emit a Success state (e.g., in the flow that resolves popularTemplateDeferred), exposing empty lists instead of an error; change the logic in PopularTravelListViewModel to detect failures from suspendRunCatching (inspect the Result instead of calling getOrNull()), and when the overall fetch for all templates or any required aggregate request fails, emit PopularTravelListState.Error (rather than Success with empty data); update the code around popularTemplateDeferred and the aggregation/emit pathway to branch on Result.isSuccess/exceptionOrNull()/getOrElse so that real errors propagate to the Error state while successful results continue to emit Success.
🧹 Nitpick comments (4)
feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListContract.kt (2)
25-35:by lazy는@Immutabledata class 내에서 안정성 문제를 일으킬 수 있습니다.
lazy델리게이트는 내부적으로 mutable한 상태를 가지므로@Immutable어노테이션과 의미적으로 충돌합니다. Compose 컴파일러가 이 클래스를 불안정(unstable)으로 판단하여 불필요한 리컴포지션이 발생할 수 있습니다.♻️ 일반 함수로 변경하는 방안
- val selectedProgramTravels: ImmutableList<PopularTravelListItem> by lazy { - val selectTab = travelProgramTabs.getOrElse(selectedTabIndex) { TravelProgramTab.All } - - when (selectTab) { - TravelProgramTab.All -> allPopularTravels - is TravelProgramTab.Custom -> popularTravelsByProgram.getOrDefault( - selectTab.programId, - persistentListOf(), - ) - } - } + fun getSelectedProgramTravels(): ImmutableList<PopularTravelListItem> { + val selectTab = travelProgramTabs.getOrElse(selectedTabIndex) { TravelProgramTab.All } + + return when (selectTab) { + TravelProgramTab.All -> allPopularTravels + is TravelProgramTab.Custom -> popularTravelsByProgram.getOrDefault( + selectTab.programId, + persistentListOf(), + ) + } + }또는 호출 빈도가 높다면 ViewModel에서 미리 계산하여 Success 생성 시 별도 필드로 전달하는 방법도 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListContract.kt` around lines 25 - 35, The lazy delegate in the selectedProgramTravels property inside PopularTravelListContract introduces mutable state and conflicts with `@Immutable`; replace the by lazy usage with a plain computed val/getter (or have the ViewModel compute and pass the value) so selectedProgramTravels is derived deterministically from travelProgramTabs, selectedTabIndex, TravelProgramTab (e.g., TravelProgramTab.All), popularTravelsByProgram and allPopularTravels; specifically remove the by lazy delegate on selectedProgramTravels and implement it as a direct when-expression (or supply it as a precomputed field) to preserve immutability and avoid Compose instability.
66-70:ShowSnackBar의Typeenum 확장성을 고려해 주세요.현재
ERR_UNKNOWN만 정의되어 있는데, 향후 네트워크 에러, 타임아웃 등 구체적인 에러 타입이 추가될 가능성이 있다면 현재 구조로 충분합니다. 다만, 모든 에러를 하나의 타입으로 처리할 계획이라면 enum 없이 단순화할 수도 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListContract.kt` around lines 66 - 70, The ShowSnackBar side effect currently defines enum Type with a single value ERR_UNKNOWN which limits clarity or is unnecessary; update PopularTravelListContract's ShowSnackBar to either expand Type with concrete cases (e.g., NETWORK_ERROR, TIMEOUT, AUTH_ERROR) to explicitly represent expected error categories, or simplify by replacing enum Type with a payload type (e.g., a String message or an Error/Throwable reference) if you intend to handle all errors generically; adjust usages of ShowSnackBar and any consumers of PopularTravelListSideEffect to match the new Type shape.feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListScreen.kt (1)
235-252: LaunchedEffect의 키를nextPage로 변경하는 것을 권장합니다.현재
LaunchedEffect(Unit)을 사용하면 컴포저블이 컴포지션에 처음 진입할 때만onLoadMore가 호출됩니다. 만약 동일한 LoadingItem이 에러 후에도 컴포지션에 남아있다면, 재시도 로직이 트리거되지 않을 수 있습니다.
nextPage를 키로 사용하면 페이지 값이 변경될 때마다 effect가 재실행되어 더 예측 가능한 동작을 보장합니다.🔧 제안된 수정
`@Composable` private fun LoadingItem( nextPage: Int, onLoadMore: (Int) -> Unit, ) { Box( modifier = Modifier .fillMaxWidth() .padding(vertical = 16.dp), contentAlignment = Alignment.Center, ) { CircularProgressIndicator(color = NDGLTheme.colors.green500) } - LaunchedEffect(Unit) { + LaunchedEffect(nextPage) { onLoadMore(nextPage) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListScreen.kt` around lines 235 - 252, The LoadingItem composable currently uses LaunchedEffect(Unit) so onLoadMore is only invoked on first composition; change the LaunchedEffect key to nextPage so the effect re-runs whenever the page changes (i.e., replace LaunchedEffect(Unit) with LaunchedEffect(nextPage) in the LoadingItem function) ensuring onLoadMore(nextPage) is called on page updates and retry scenarios.core/ui/src/main/java/com/yapp/ndgl/core/ui/CommonErrorView.kt (1)
26-30: 공통 컴포넌트 내부의 고정 세로 패딩은 재사용성을 낮출 수 있습니다.Line 29의
vertical = 165.dp는 화면별 레이아웃 제어를 어렵게 만들 수 있어, 세로 여백은 호출부에서 주입하는 방식이 더 유연합니다.예시 리팩터링
+import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size @@ fun CommonErrorView( modifier: Modifier = Modifier, title: String? = null, description: String? = null, + contentPadding: PaddingValues = PaddingValues(horizontal = 24.dp), ) { Column( modifier = modifier .fillMaxWidth() - .padding(horizontal = 24.dp, vertical = 165.dp), + .padding(contentPadding), verticalArrangement = Arrangement.spacedBy(16.dp), horizontalAlignment = Alignment.CenterHorizontally, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/main/java/com/yapp/ndgl/core/ui/CommonErrorView.kt` around lines 26 - 30, The Column in CommonErrorView.kt uses a hardcoded vertical padding (.padding(horizontal = 24.dp, vertical = 165.dp)), which reduces reusability; change the CommonErrorView composable signature to accept a verticalPadding: Dp (with a sensible default) or a contentPadding: PaddingValues parameter, remove the fixed vertical = 165.dp from the modifier and apply the injected padding instead, and update callers to supply appropriate vertical spacing where needed (search for CommonErrorView and the Column modifier to locate the code to modify).
🤖 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/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt`:
- Around line 99-105: The isLoadingMore flag in loadMore is set to true before
launching but is not guaranteed to be reset if the early return occurs; wrap the
viewModelScope.launch body in a try/finally (or place the flag reset in a
finally) so isLoadingMore is always set to false after the coroutine completes
or errors, ensuring both the path where state.value is not
PopularTravelListState.Success and the normal completion paths (referencing
loadMore, isLoadingMore, viewModelScope.launch and
PopularTravelListState.Success) will clear the flag.
- Around line 133-147: The code unconditionally calls dropLast(1) on
allPopularTravels and current (in branches handling TravelProgramTab.All and
TravelProgramTab.Custom) which assumes the last element is always a Loading
marker; change both places to first inspect the last element and only drop it if
it is the Loading marker (otherwise keep all items), then append newItems and
convert toImmutableList(); update the logic that sets
popularTravelsByProgram[selectedTab.programId] and the allPopularTravels
assignment to use this conditional-last-item-removal behavior so real Travel
items are never accidentally removed.
---
Outside diff comments:
In
`@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt`:
- Around line 52-58: The current use of suspendRunCatching(...).getOrNull() when
calling travelTemplateRepository.getAllPopularTravelTemplates() and
getPopularTravelTemplates(...) swallows failures and causes the ViewModel to
always emit a Success state (e.g., in the flow that resolves
popularTemplateDeferred), exposing empty lists instead of an error; change the
logic in PopularTravelListViewModel to detect failures from suspendRunCatching
(inspect the Result instead of calling getOrNull()), and when the overall fetch
for all templates or any required aggregate request fails, emit
PopularTravelListState.Error (rather than Success with empty data); update the
code around popularTemplateDeferred and the aggregation/emit pathway to branch
on Result.isSuccess/exceptionOrNull()/getOrElse so that real errors propagate to
the Error state while successful results continue to emit Success.
---
Nitpick comments:
In `@core/ui/src/main/java/com/yapp/ndgl/core/ui/CommonErrorView.kt`:
- Around line 26-30: The Column in CommonErrorView.kt uses a hardcoded vertical
padding (.padding(horizontal = 24.dp, vertical = 165.dp)), which reduces
reusability; change the CommonErrorView composable signature to accept a
verticalPadding: Dp (with a sensible default) or a contentPadding: PaddingValues
parameter, remove the fixed vertical = 165.dp from the modifier and apply the
injected padding instead, and update callers to supply appropriate vertical
spacing where needed (search for CommonErrorView and the Column modifier to
locate the code to modify).
In
`@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListContract.kt`:
- Around line 25-35: The lazy delegate in the selectedProgramTravels property
inside PopularTravelListContract introduces mutable state and conflicts with
`@Immutable`; replace the by lazy usage with a plain computed val/getter (or have
the ViewModel compute and pass the value) so selectedProgramTravels is derived
deterministically from travelProgramTabs, selectedTabIndex, TravelProgramTab
(e.g., TravelProgramTab.All), popularTravelsByProgram and allPopularTravels;
specifically remove the by lazy delegate on selectedProgramTravels and implement
it as a direct when-expression (or supply it as a precomputed field) to preserve
immutability and avoid Compose instability.
- Around line 66-70: The ShowSnackBar side effect currently defines enum Type
with a single value ERR_UNKNOWN which limits clarity or is unnecessary; update
PopularTravelListContract's ShowSnackBar to either expand Type with concrete
cases (e.g., NETWORK_ERROR, TIMEOUT, AUTH_ERROR) to explicitly represent
expected error categories, or simplify by replacing enum Type with a payload
type (e.g., a String message or an Error/Throwable reference) if you intend to
handle all errors generically; adjust usages of ShowSnackBar and any consumers
of PopularTravelListSideEffect to match the new Type shape.
In
`@feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListScreen.kt`:
- Around line 235-252: The LoadingItem composable currently uses
LaunchedEffect(Unit) so onLoadMore is only invoked on first composition; change
the LaunchedEffect key to nextPage so the effect re-runs whenever the page
changes (i.e., replace LaunchedEffect(Unit) with LaunchedEffect(nextPage) in the
LoadingItem function) ensuring onLoadMore(nextPage) is called on page updates
and retry scenarios.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/ui/src/main/java/com/yapp/ndgl/core/ui/CommonErrorView.ktcore/ui/src/main/res/values/strings.xmldata/travel/src/main/java/com/yapp/ndgl/data/travel/repository/TravelTemplateRepository.ktfeature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListContract.ktfeature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListScreen.ktfeature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.ktfeature/home/src/main/java/com/yapp/ndgl/feature/home/search/TemplateSearchScreen.ktfeature/home/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- feature/home/src/main/res/values/strings.xml
feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt
Outdated
Show resolved
Hide resolved
feature/home/src/main/java/com/yapp/ndgl/feature/home/popular/PopularTravelListViewModel.kt
Show resolved
Hide resolved
jihee-dev
force-pushed
the
feature/NDGL-124/impl-popular-list-paging
branch
2 times, most recently
from
14108d8 to
0800a06
Compare
jihee-dev
force-pushed
the
feature/NDGL-124/impl-popular-list-paging
branch
from
0800a06 to
7420bfc
Compare
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
1 participant
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.
NDGL-124 인기 여행 목록 페이지 paging 구현 및 에러 뷰 추가
연관 문서
변경사항
테스트 체크 리스트
Summary by CodeRabbit
릴리스 노트
New Features
Bug Fixes
Refactor