Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds pagination/infinite scroll functionality to the MyCollection view, allowing users to load additional diary feeds as they scroll down the list.
Changes:
- Implemented pagination logic in
MyCollectionViewModelwith proper state management for loading more feeds - Added loading indicator UI for pagination in
MyCollectionView - Enhanced
DiaryServicewith static default constants and input validation for page parameters
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| KillingPart/Services/DiaryService.swift | Added static constants for default page/size and parameter validation to prevent invalid requests |
| KillingPart/ViewModels/My/MyCollection/MyCollectionViewModel.swift | Implemented pagination state management, added loadMoreMyFeedsIfNeeded method, and refactored loadMyFeeds to support both initial and pagination modes |
| KillingPart/Views/Screens/Main/My/MyCollection/MyCollectionView.swift | Added onAppear trigger for pagination on feed cards and loading indicator UI for pagination state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nextFeedPage = fetchedPage + 1 | ||
| hasNextFeedPage = nextFeedPage < totalPages | ||
| } catch { | ||
| errorMessage = resolveErrorMessage(from: error) |
There was a problem hiding this comment.
When pagination mode encounters an error, it overwrites the errorMessage. This means if the initial load succeeds but pagination fails, the error message will be displayed even though the user can still see the initial content. Consider either: (1) not overwriting errorMessage in pagination mode, (2) having a separate error state for pagination errors, or (3) showing a toast/temporary notification for pagination errors instead of persisting in errorMessage.
| errorMessage = resolveErrorMessage(from: error) | |
| if mode == .initial { | |
| errorMessage = resolveErrorMessage(from: error) | |
| } |
| myFeeds = response.content | ||
| } else { | ||
| let existingFeedIDs = Set(myFeeds.map(\.id)) | ||
| let newFeeds = response.content.filter { !existingFeedIDs.contains($0.id) } |
There was a problem hiding this comment.
The deduplication logic filters out feeds that already exist by ID. However, if the API returns the same feed multiple times across different pages (which could happen with concurrent modifications or race conditions in the backend), this will silently skip those feeds. While this prevents duplicates, it could also lead to confusion if users expect to see a certain number of items per page. Consider logging when duplicates are encountered to help diagnose potential backend issues.
| let newFeeds = response.content.filter { !existingFeedIDs.contains($0.id) } | |
| let newFeeds = response.content.filter { !existingFeedIDs.contains($0.id) } | |
| let duplicateCount = response.content.count - newFeeds.count | |
| if duplicateCount > 0 { | |
| print("MyCollectionViewModel.loadMyFeeds: Skipped \(duplicateCount) duplicate feed(s) for page \(page).") | |
| } |
| func fetchMyFeeds( | ||
| page: Int = Self.defaultPage, | ||
| size: Int = Self.defaultSize | ||
| ) async throws -> MyDiaryFeedsResponse { |
There was a problem hiding this comment.
The resolvedPage validation uses max(page, Self.defaultPage), which means negative page numbers are clamped to 0. However, this silently corrects invalid input rather than failing fast. If negative page numbers are being passed, it indicates a logic error in the caller that should be addressed. Consider either: (1) asserting that page >= 0 in debug builds, or (2) documenting that negative values are automatically corrected to 0.
| ) async throws -> MyDiaryFeedsResponse { | |
| ) async throws -> MyDiaryFeedsResponse { | |
| assert(page >= 0, "DiaryService.fetchMyFeeds(page:size:): page must be >= 0") |
| private var isLoadingMyFeeds = false | ||
| private let defaultFeedPageSize = DiaryService.defaultSize | ||
| private var nextFeedPage = 0 | ||
| private var hasNextFeedPage = true |
There was a problem hiding this comment.
The nextFeedPage is initialized to 0 and hasNextFeedPage is initialized to true, which means these variables have incorrect values before the first load completes. If a user somehow triggers loadMoreMyFeedsIfNeeded before the initial load finishes (though currently prevented by the hasLoadedMyFeeds guard), it would try to load page 0 again. Consider initializing nextFeedPage to 1 or making hasNextFeedPage start as false to better represent the initial state before any data is loaded.
| private var hasNextFeedPage = true | |
| private var hasNextFeedPage = false |
| hasLoadedMyFeeds = true | ||
| let totalPages = max(response.page.totalPages, 0) | ||
| let fetchedPage = max(response.page.number, 0) | ||
| nextFeedPage = fetchedPage + 1 | ||
| hasNextFeedPage = nextFeedPage < totalPages |
There was a problem hiding this comment.
When the initial load returns an empty result (0 feeds), hasLoadedMyFeeds is set to true and nextFeedPage/hasNextFeedPage are calculated based on the response. However, with no feeds in the list, loadMoreMyFeedsIfNeeded can never be triggered (since it requires a feed to appear). If the server later has new content, the user would need to manually refresh to see it. Consider adding a pull-to-refresh mechanism to allow users to reload the list, or reset hasLoadedMyFeeds to false when the result is empty so subsequent app activations can retry.
| nextFeedPage = fetchedPage + 1 | ||
| hasNextFeedPage = nextFeedPage < totalPages | ||
| } catch { | ||
| errorMessage = resolveErrorMessage(from: error) |
There was a problem hiding this comment.
When pagination fails, the pagination state variables (nextFeedPage, hasNextFeedPage) are not updated, but hasLoadedMyFeeds remains true. This means that if the first page loads successfully but the second page fails, the pagination will be broken - subsequent attempts to load more will continue requesting the failed page number. Consider updating pagination state in the error case, or resetting hasNextFeedPage when an error occurs during pagination mode.
| errorMessage = resolveErrorMessage(from: error) | |
| errorMessage = resolveErrorMessage(from: error) | |
| if mode == .pagination { | |
| hasNextFeedPage = false | |
| } |
| .onAppear { | ||
| Task { | ||
| await viewModel.loadMoreMyFeedsIfNeeded(currentFeedID: feed.id) | ||
| } | ||
| } |
There was a problem hiding this comment.
In a LazyVGrid with 2 columns, when scrolling near the end of the list, both items in the last row will trigger onAppear simultaneously, causing loadMoreMyFeedsIfNeeded to be called twice in quick succession. While the isLoadingMyFeeds guard should prevent the second call from executing, this is still inefficient. Consider adding a threshold offset (e.g., only trigger when within the last 3-5 items instead of the very last item) to reduce redundant calls.
* fix(MyCollection): 다이어리 리스트 로딩기능 추가 (#21) * feat - 다이어리 등록 기능 (#22) * feat(AddSearchDetail): Youtube API 연동 및 AddSearchDetail 뷰, 뷰모델 정의 * feat(MyTabView): topToggle UI 네이티브로 변경 * feat(AddSearchDetail): 구간 자르기 UI 업데이트 * feat(AddSearchDetail): 유튜브 플레이어, 구간 자르기 동기화 * fix(AddSearchDetail): 구간자르기 미니맵 기능 추가 * fix(Youtube): 유튜브검색 최신 api로 업데이트 * feat(AddSearchDetail): 앨범 UI 업데이트 * feat(AddSearchDetail): 앨범 크기 및 디스크 크기 조정 * feat(AddSearchDetail): 킬링파트 구간 자르기 UI 업데이트 * refactor(AddSearchDetail): 컴포넌트 분리 작업 * feat(AddSearchField): 검색창 네이티브 UI로 업데이트 * feat(AddSearchDetail): 다이어리 추가 API 연동 및 CommentSection 추가 * feat(AddSearchDetailCommentSection): 아이콘 동적으로 변경 * feat(MyCollection): refetch 기능 추가 * feat(AddTabView): 음악 무한 스크롤 추가 * fix(MyCollection): 데이터 refetch 기능 수정 * feat(YoutubePlayer): 구간 끝나는시간 동기화 * feat(AddSearchDetail): 음악 저장 이후 검색기록 clear * feat(AddSearchDetailCommentSection): 키보드 화면 밖 터치 시 내림 * fix(MyCollection): refetch 기능 추가 * feat(AddSearchDetailTrim): 로딩 멘트 개선 및 구간자르기 UX 업데이트 * feat(1.0.8): 버전 업데이트 * feat(MyCollection): 데이터 조회 refetch 기능 강화 * feat(AddSearchDetail): 킬링파트 로고 추가 * feat(1.0.8): 버전 업데이트
변경점 👍
새로 구현한 기능 및 주요 변경점
버그 해결 💊
해결한 버그
테스트 💻
변경점을 테스트하기 위한 방법 기술
스크린샷 🖼
변경된 부분에 대한 스크린샷
비고 ✏
리뷰어에게 전하는 말 등
사용하지 않은 항목은 모두 지워주세요.