Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] cursor 방식 페이지네이션 구현 #74

Merged
merged 12 commits into from
Nov 26, 2024
Merged

Conversation

keemsebin
Copy link
Member

@keemsebin keemsebin commented Nov 25, 2024

관련 이슈

close #69

📑 작업 내용

  • useInfiniteQuery 사용하여 cursor 방식 페이지네이션 구현했습니다. size 만큼 데이터를 불러오다가, hasNext 값이 존재할 경우 true일 경우 다음 pageParmsnextCursor를 요청하는 방법입니다.
  • IntersectionObserver는 브라우저 API로, 특정 요소가 부모 요소나 뷰포트에 보이는지 여부를 관찰하는 객체입니다.
  • 따라서 스크롤 시 객체가 더이상 존재하지 않을 때 다음 데이터를 불러오도록 구현했습니다.
12222.mov

💬 리뷰 중점 사항/기타 참고 사항

  • Marker 조회시, 다음 데이터가 요청될때마다 마커를 보여주는 형식으로 구현했습니다.

Summary by CodeRabbit

  • 새로운 기능

    • 버튼 컴포넌트의 크기 변형 업데이트: 큰 버튼과 중간 버튼의 높이가 줄어들었습니다.
    • 모달 컴포넌트의 콘텐츠 패딩이 조정되어 레이아웃이 개선되었습니다.
    • 북마크 목록과 선택 목록에서 무한 스크롤 기능이 추가되었습니다.
    • 북마크 및 마커 목록을 위한 페이지네이션 지원이 향상되었습니다.
    • 북마크 세부 정보 컴포넌트의 상태 관리 및 렌더링 성능이 개선되었습니다.
  • 버그 수정

    • 북마크와 마커 데이터의 렌더링 로직이 개선되어 불필요한 업데이트를 방지합니다.
  • 스타일

    • 커스텀 마커의 제목 스타일이 조정되어 가독성이 향상되었습니다.

@keemsebin keemsebin added the feat 기능 label Nov 25, 2024
@keemsebin keemsebin self-assigned this Nov 25, 2024
@keemsebin keemsebin requested a review from houony as a code owner November 25, 2024 14:57
Copy link

coderabbitai bot commented Nov 25, 2024

Walkthrough

이 풀 리퀘스트는 여러 컴포넌트의 스타일과 기능을 수정하여 페이지네이션 및 사용자 경험을 개선하는 데 중점을 두고 있습니다. Button 컴포넌트의 크기 변형이 조정되었고, Modal 컴포넌트의 패딩 값이 변경되었습니다. BookmarkDetail, BookmarkList, BookmarkSelectionList 컴포넌트는 새로운 훅과 상태 관리 로직을 도입하여 무한 스크롤 및 데이터 페칭 기능을 강화했습니다. 또한, useBookMarkListuseMarkerList 훅이 useInfiniteQuery로 변경되어 커서 기반 페이지네이션을 지원합니다.

Changes

파일 경로 변경 요약
src/components/common/Button/Button.variants.ts 버튼의 largemedium 크기 변형의 높이를 각각 h-16에서 h-14, h-14에서 h-12로 수정.
src/components/common/Modal/index.tsx Modal 컴포넌트의 Content 스타일에서 수평 패딩을 px-6에서 px-4, 수직 패딩을 py-7에서 py-4로 변경.
src/components/features/BookmarkDetail/BookmarkDetail.tsx 새로운 훅과 상태 관리 로직 추가, useMarkerList의 반환값 확장 및 마커 렌더링 로직 업데이트.
src/components/features/BookmarkList/BookmarkList.tsx 새로운 훅 및 상태 관리 로직 추가, 북마크 데이터의 플랫맵 구조로 변경, 무한 스크롤 기능 구현.
src/components/features/SearchResultsList/BookmarkSelectionList.tsx 새로운 훅 추가 및 무한 스크롤 기능 구현, 북마크 데이터 렌더링 로직 수정.
src/hooks/api/bookmarks/useBookMarkList.ts useQuery에서 useInfiniteQuery로 변경, 커서 기반 페이지네이션을 위한 수정.
src/hooks/api/marker/useMarkerList.ts useQuery에서 useInfiniteQuery로 변경, 커서 기반 페이지네이션을 위한 수정.
src/utils/CustomMarker.ts <span> 요소의 스타일 변경: 폰트 크기 및 두께 조정, 마진 값 수정.

Assessment against linked issues

Objective Addressed Explanation
cursor 방식 페이지네이션 적용 (69)

Suggested reviewers

  • houony

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

github-actions bot commented Nov 25, 2024

Update: 2024년 11월 26일 15시 49분 57초
Storybook 미리보기: https://6709628f8a6eec04b29d4f39-tangkjpnwv.chromatic.com/

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (11)
src/hooks/api/bookmarks/useBookMarkList.ts (2)

1-1: import 문 사이의 빈 줄을 제거해주세요.

import 문 사이에 불필요한 빈 줄이 있습니다.

다음과 같이 수정해주세요:

import { useInfiniteQuery } from '@tanstack/react-query';
-
import { get } from '@/lib/axios';
🧰 Tools
🪛 eslint

[error] 1-1: There should be no empty line within import group

(import/order)


23-32: 커서 기반 페이지네이션이 잘 구현되었습니다.

페이지네이션 구현이 PR의 목적에 잘 부합하며, useInfiniteQuery를 효과적으로 활용하고 있습니다. 다만 몇 가지 제안사항이 있습니다:

  1. size=7의 값이 하드코딩되어 있는데, 이 값을 상수로 분리하거나 설정으로 관리하는 것이 좋을 것 같습니다.
  2. 에러 처리 로직이 없는데, 네트워크 오류나 서버 오류에 대한 처리를 추가하면 좋을 것 같습니다.

다음과 같이 개선해보는 것은 어떨까요?:

+const BOOKMARK_PAGE_SIZE = 7;
+
 export const useBookMarkList = (token: string) => {
   return useInfiniteQuery<Bookmarks>({
     queryKey: ['bookmarklist', token],
     queryFn: ({ pageParam = 0 }) =>
-      get<Bookmarks>(`api/bookmarks?cursor=${pageParam}&size=7`, {
+      get<Bookmarks>(`api/bookmarks?cursor=${pageParam}&size=${BOOKMARK_PAGE_SIZE}`, {
         headers: {
           Authorization: `Bearer ${token}`,
         },
-      }),
+      }).catch((error) => {
+        console.error('북마크 목록을 불러오는데 실패했습니다:', error);
+        throw error;
+      }),
     initialPageParam: 0,
     getNextPageParam: (lastPage) => (lastPage.hasNext ? lastPage.nextCursor : undefined),
   });
 };
src/hooks/api/marker/useMarkerList.ts (2)

1-1: 임포트 순서 개선이 필요합니다

임포트 그룹 사이에 빈 줄이 있어 코드 스타일 일관성이 떨어집니다.

다음과 같이 수정하는 것을 제안합니다:

import { useInfiniteQuery } from '@tanstack/react-query';
-
import { get } from '@/lib/axios';
🧰 Tools
🪛 eslint

[error] 1-1: There should be no empty line within import group

(import/order)


31-41: 커서 기반 페이지네이션이 잘 구현되었습니다

useInfiniteQuery를 사용한 구현이 적절하며, 다음 사항들이 잘 처리되었습니다:

  • 초기 pageParam 설정
  • 커서 기반 페이지네이션 로직
  • 타입 안전성

하나의 개선 제안이 있습니다: 페이지 크기(size=7)가 하드코딩되어 있어 재사용성이 제한될 수 있습니다.

다음과 같이 페이지 크기를 매개변수로 받는 것을 고려해보세요:

-export const useMarkerList = (markerId: number, token: string) => {
+export const useMarkerList = (markerId: number, token: string, pageSize: number = 7) => {
   return useInfiniteQuery<Marker>({
     queryKey: ['marker', markerId, token],
     queryFn: ({ pageParam = 0 }) =>
-      get<Marker>(`api/markers/${markerId}?cursor=${pageParam}&size=7`, {
+      get<Marker>(`api/markers/${markerId}?cursor=${pageParam}&size=${pageSize}`, {
src/components/features/SearchResultsList/BookmarkSelectionList.tsx (4)

24-24: observerTarget의 타입 지정 개선

useRef<HTMLDivElement>(null);에서 초기값으로 null을 제공하고 있으므로, 타입을 React.MutableRefObject<HTMLDivElement | null>로 지정하는 것이 더 명확합니다.

다음과 같이 수정할 것을 제안합니다:

- const observerTarget = useRef<HTMLDivElement>(null);
+ const observerTarget = useRef<HTMLDivElement | null>(null);

76-76: onClick 핸들러의 메모이제이션 고려

onClick 핸들러가 리스트 아이템마다 새로운 함수로 생성되고 있습니다. 성능 최적화를 위해 useCallback을 활용하여 메모이제이션할 것을 고려해보세요.

다음과 같이 수정할 수 있습니다:


70-73: data 존재 여부 확인 및 안전한 접근

data가 없을 경우를 대비하여 data?.pages를 사용할 때 기본값을 제공하거나 조건부 렌더링을 추가하여 에러를 방지할 수 있습니다.

다음과 같이 수정할 것을 제안합니다:


94-95: 아이콘과 텍스트의 정렬 개선

아이콘과 텍스트의 크기 및 정렬이 일관되지 않을 수 있으므로, CSS 클래스나 스타일을 조정하여 UI 일관성을 높일 수 있습니다.

src/components/features/BookmarkDetail/BookmarkDetail.tsx (1)

103-103: 북마크 이름이 없을 경우를 대비해주세요

data?.pages[0]?.bookmarkNameundefined일 경우 대비하여 기본값을 설정하면 안전합니다.

src/components/features/BookmarkList/BookmarkList.tsx (2)

71-80: handleObserver의 의존성 배열을 정확하게 설정하세요

useCallback 훅의 의존성 배열에 hasNextPageisFetchingNextPage가 포함되어 있습니다. 그러나 이러한 값들이 변경될 때마다 handleObserver 함수가 재생성되어 불필요한 리렌더링이 발생할 수 있습니다.

hasNextPageisFetchingNextPage는 불필요한 의존성일 수 있으므로, 의존성 배열에서 제거하거나 useRef로 관리하는 것을 고려해보세요.


68-69: 선택한 아이템의 이름을 안전하게 접근하세요

선택한 아이템이 없는 경우 undefined가 반환될 수 있으므로, 기본 값 설정을 위해 논리 연산자 || 대신 널 병합 연산자 ??를 사용하는 것이 안전합니다.

-    data?.pages.flatMap((page) => page.data).find((item) => item.bookmarkId === selectedId)?.name ||
+    data?.pages.flatMap((page) => page.data).find((item) => item.bookmarkId === selectedId)?.name ?? 
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 63133bc and 89d8eb8.

📒 Files selected for processing (8)
  • src/components/common/Button/Button.variants.ts (1 hunks)
  • src/components/common/Modal/index.tsx (1 hunks)
  • src/components/features/BookmarkDetail/BookmarkDetail.tsx (5 hunks)
  • src/components/features/BookmarkList/BookmarkList.tsx (4 hunks)
  • src/components/features/SearchResultsList/BookmarkSelectionList.tsx (3 hunks)
  • src/hooks/api/bookmarks/useBookMarkList.ts (2 hunks)
  • src/hooks/api/marker/useMarkerList.ts (2 hunks)
  • src/utils/CustomMarker.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/utils/CustomMarker.ts
🧰 Additional context used
🪛 eslint
src/hooks/api/bookmarks/useBookMarkList.ts

[error] 1-1: There should be no empty line within import group

(import/order)

src/hooks/api/marker/useMarkerList.ts

[error] 1-1: There should be no empty line within import group

(import/order)

🔇 Additional comments (8)
src/components/common/Button/Button.variants.ts (1)

13-14: 버튼 크기 변경에 대한 검증이 필요합니다.

버튼의 높이가 전체적으로 줄어들었습니다:

  • large: h-16 → h-14
  • medium: h-14 → h-12

이러한 변경이 기존 UI의 일관성에 영향을 미칠 수 있으므로, 다른 컴포넌트들과의 조화를 확인해주세요.

✅ Verification successful

Let me fix the shell script and try again to gather the necessary information.


Let me try one more search with corrected ripgrep command:


버튼 크기 변경이 기존 UI에 미치는 영향이 확인되었습니다.

검증 결과를 바탕으로 다음과 같은 사항들이 확인되었습니다:

  • 15개 이상의 위치에서 large 또는 medium 크기의 버튼이 사용되고 있습니다.
  • Modal, BottomSheet, BookmarkList 등 주요 UI 컴포넌트에서 버튼이 활용되고 있습니다.
  • 높이 변경(large: h-16→h-14, medium: h-14→h-12)이 전체적인 UI의 일관성 향상에 도움이 될 것으로 판단됩니다.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: 버튼 컴포넌트의 사용 현황을 확인합니다.

# Button 컴포넌트의 사용 패턴을 검색
echo "Button 컴포넌트 사용 현황 확인:"
rg -A 2 "Button.*size=(\"|\')(large|medium)" --type ts --type tsx

# size prop이 동적으로 설정되는 경우 확인
echo "\n동적 size prop 설정 확인:"
ast-grep --pattern 'size={$_}'

Length of output: 324


Script:

#!/bin/bash
# Description: 버튼 컴포넌트의 사용 현황을 확인합니다.

# Button 컴포넌트의 사용 패턴을 검색
echo "Button 컴포넌트 사용 현황 확인:"
rg -A 2 'Button.*size="(large|medium)"' --type ts --type tsx
rg -A 2 "Button.*size='(large|medium)'" --type ts --type tsx

# size prop이 동적으로 설정되는 경우 확인
echo -e "\n동적 size prop 설정 확인:"
ast-grep --pattern 'Button $$$size={$_}$$$'

# Button 컴포넌트 관련 파일들 확인
echo -e "\nButton 관련 파일 확인:"
fd -e ts -e tsx Button

Length of output: 665


Script:

#!/bin/bash
# Button 컴포넌트 사용 검색
echo "Button 컴포넌트 사용 패턴:"
rg 'Button.*size="(large|medium)"'
rg "Button.*size='(large|medium)'"

# Button 스토리북 파일 내용 확인
echo -e "\nButton 스토리북 내용:"
cat src/components/common/Button/Button.stories.tsx

# Button 타입 정의 확인
echo -e "\nButton 타입 정의:"
cat src/components/common/Button/Button.types.ts

Length of output: 3611

src/components/common/Modal/index.tsx (2)

Line range hint 1-44: 전반적인 구현이 잘 되어있습니다! 👍

모달 컴포넌트가 다음과 같은 모범 사례들을 잘 따르고 있습니다:

  • Portal을 사용한 적절한 렌더링
  • Framer Motion을 활용한 애니메이션 처리
  • 외부 클릭 처리
  • TypeScript를 활용한 타입 안전성

16-16: 모달 패딩 값 변경에 대한 검토가 필요합니다.

패딩 값이 감소되어 모달 컨텐츠가 더 조밀해졌습니다. 다음 사항들을 확인해주시기 바랍니다:

  • 모든 모달 사용 케이스에서 컨텐츠가 적절하게 표시되는지 확인
  • 디자인 시스템과의 일관성 검토
✅ Verification successful

모달 패딩 변경이 적절합니다.

모달 컴포넌트의 사용 사례들을 검토한 결과, 패딩 값 변경(px-4, py-4)이 모든 사용 컨텍스트에서 적절하게 작동할 것으로 판단됩니다:

  • 로그인 모달, 삭제 확인 모달, 북마크 추가 모달 등 모든 사용 사례에서 내부 컨텐츠가 자체 여백을 가지고 있어 모달의 패딩 감소가 문제되지 않습니다.
  • 각 모달 컨텐츠들이 flexgap 속성으로 자체적인 간격을 제어하고 있습니다.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: 모달 컴포넌트의 사용 현황을 확인합니다.

# 모달 컴포넌트가 사용된 모든 파일을 검색
rg -l "Modal" --type tsx

Length of output: 52


Script:

#!/bin/bash
# Description: 모달 컴포넌트의 사용 현황을 확인합니다.

# TypeScript/React 파일에서 Modal 컴포넌트 사용 검색
rg -t ts -t tsx -l "Modal"

# Modal 컴포넌트의 실제 구현 확인
rg -t ts -t tsx -A 5 "className.*modal" 

Length of output: 124


Script:

#!/bin/bash
# Description: 모달 컴포넌트의 사용 현황을 확인합니다.

# 파일 확장자로 직접 검색
rg -l "Modal" --glob "*.ts" --glob "*.tsx"

# Modal 클래스네임 패턴 검색
rg "className.*modal" --glob "*.ts" --glob "*.tsx" -A 5

Length of output: 879


Script:

#!/bin/bash
# 모달 컴포넌트의 실제 사용 컨텍스트 확인
rg -A 10 "<Modal" --glob "*.tsx" 

# 모달 컴포넌트의 스토리북 구현 확인
cat "src/components/common/Modal/Modal.stories.tsx"

Length of output: 5502

src/components/features/SearchResultsList/BookmarkSelectionList.tsx (1)

45-47: ⚠️ Potential issue

종속성 배열에 handleObserver 추가 필요

useEffect 훅에서 handleObserver를 사용하고 있으므로, 종속성 배열에 handleObserver를 추가하여 의존성을 명시해야 합니다.

다음과 같이 수정할 것을 제안합니다:

Likely invalid or redundant comment.

src/components/features/BookmarkDetail/BookmarkDetail.tsx (4)

1-1: 좋습니다!


59-64: 코드가 잘 작성되었습니다!


66-75: 코드가 잘 작성되었습니다!


77-88: 코드가 잘 작성되었습니다!

Comment on lines +42 to +51
const handleObserver = useCallback(
(entries: IntersectionObserverEntry[]) => {
entries.forEach((entry) => {
if (entry.isIntersecting && hasNextPage && !isFetchingNextPage) {
fetchNextPage();
}
});
},
[fetchNextPage, hasNextPage, isFetchingNextPage]
);
Copy link

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Intersection Observer의 observerTarget 위치 확인 필요

observerTargetdata를 반복하는 map 내부에서 여러 번 생성되고 있습니다. 이는 여러 개의 observerTarget을 생성하여 Intersection Observer가 의도한 대로 작동하지 않을 수 있습니다. observerTarget은 단일 요소에 할당되어야 하며, 리스트의 마지막 요소 이후에 위치시키는 것이 적절합니다.

다음과 같이 observerTargetmap 외부로 이동하여 수정할 것을 제안합니다:

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment로 적어주시겠어요??

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keemsebin, 알겠습니다. observerTargetmap 바깥으로 이동하여 리스트의 마지막에 추가하시는 것을 제안드립니다:

{data?.pages
  .flatMap((page) => page.data)
  .map((item, index) => (
    <div key={item.bookmarkId}>
      {/* 기존 코드 */}
    </div>
  ))}

// observerTarget을 리스트의 마지막에 추가합니다
<div ref={observerTarget} className="h-1" />

이렇게 하면 observerTarget이 단일 요소로 생성되어 Intersection Observer가 의도한 대로 작동할 수 있습니다.

Comment on lines +50 to +55
if (allMarkers.length) {
setMarkers((prevMarkers) => {
const newMarkersStr = JSON.stringify(allMarkers);
const prevMarkersStr = JSON.stringify(prevMarkers);
return newMarkersStr !== prevMarkersStr ? allMarkers : prevMarkers;
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

배열 비교를 위한 더 효율적인 방법 고려하기

JSON.stringify를 사용하여 배열을 비교하면 성능에 영향을 줄 수 있습니다. 대신 배열의 길이와 각 요소를 직접 비교하는 방법이나 lodashisEqual 함수를 사용하는 것을 고려해주세요.

다음과 같이 수정할 수 있습니다:

-useEffect(() => {
-  if (allMarkers.length) {
-    setMarkers((prevMarkers) => {
-      const newMarkersStr = JSON.stringify(allMarkers);
-      const prevMarkersStr = JSON.stringify(prevMarkers);
-      return newMarkersStr !== prevMarkersStr ? allMarkers : prevMarkers;
-    });
-  }
-}, [allMarkers, setMarkers]);

+useEffect(() => {
+  if (allMarkers.length) {
+    setMarkers((prevMarkers) => {
+      if (allMarkers.length !== prevMarkers.length) {
+        return allMarkers;
+      }
+      const isDifferent = allMarkers.some(
+        (marker, index) => marker.markerId !== prevMarkers[index].markerId
+      );
+      return isDifferent ? allMarkers : prevMarkers;
+    });
+  }
+}, [allMarkers, setMarkers]);

Committable suggestion skipped: line range outside the PR's diff.


{index < data.markers.data.length - 1 && <hr className="border-dashed pt-2" />}
{index < allMarkers.length - 1 && <hr className="border-dashed pt-2" />}
<div ref={observerTarget} />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

map 함수 내에서 ref 사용 시 주의 필요

ref={observerTarget}map 함수 내부에서 사용하면 여러 요소에 동일한 ref가 할당되어 예상치 못한 동작이 발생할 수 있습니다. Intersection Observer를 적용하려면 마지막 요소에만 ref를 할당하는 것이 좋습니다.

다음과 같이 수정할 수 있습니다:

-<div ref={observerTarget} />
+{index === allMarkers.length - 1 && <div ref={observerTarget} />}

Committable suggestion skipped: line range outside the PR's diff.

.map((item, index) => (
<div key={item.bookmarkId}>
<div
ref={observerTarget}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

반복문 내에서 ref를 여러 요소에 할당하면 예기치 않은 동작이 발생할 수 있습니다

map 함수 내에서 동일한 observerTarget ref를 여러 <div> 요소에 할당하고 있습니다. 이는 ref가 마지막 요소만을 참조하게 되어 의도치 않은 동작을 일으킬 수 있습니다.

문제 해결을 위해 리스트의 마지막 요소나 별도의 하단 <div>에만 ref를 할당하여 관찰 대상(observer)로 사용하는 것을 권장합니다. 예를 들면:

-                <div
-                    ref={observerTarget}
-                    onClick={() => handleBookmarkClick(item.bookmarkId)}
-                    className="flex flex-row justify-between items-center cursor-pointer"
-                >
+                <div
+                    onClick={() => handleBookmarkClick(item.bookmarkId)}
+                    className="flex flex-row justify-between items-center cursor-pointer"
+                >

그리고 리스트 렌더링 이후에 다음과 같이 observerTarget을 추가합니다:

<div ref={observerTarget} />

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
src/components/features/BookmarkDetail/BookmarkDetail.tsx (3)

22-26: 타입 안정성 개선 제안

observerTarget ref와 마커 상태에 대한 타입 정의를 더 명확하게 할 수 있습니다.

-const [, setMarkers] = useAtom(markersAtom);
-const observerTarget = useRef<HTMLDivElement>(null);
+const [, setMarkers] = useAtom<Marker[]>(markersAtom);
+const observerTarget = useRef<HTMLDivElement | null>(null);

66-88: 에러 처리 및 로딩 상태 관리 개선 필요

무한 스크롤 구현 시 에러 처리와 로딩 상태에 대한 사용자 피드백이 누락되어 있습니다.

다음과 같은 개선을 제안합니다:

const handleObserver = useCallback(
  (entries: IntersectionObserverEntry[]) => {
    entries.forEach((entry) => {
      if (entry.isIntersecting && hasNextPage && !isFetchingNextPage) {
-       fetchNextPage();
+       fetchNextPage().catch((error) => {
+         console.error('Failed to fetch next page:', error);
+         // 에러 상태 처리
+       });
      }
    });
  },
  [fetchNextPage, hasNextPage, isFetchingNextPage]
);

// JSX 부분에 로딩 인디케이터 추가
+{isFetchingNextPage && (
+  <div className="flex justify-center py-4">
+    <LoadingSpinner />
+  </div>
+)}

Line range hint 108-140: 접근성 개선 필요

마커 목록의 접근성을 개선하여 스크린 리더 사용자의 경험을 향상시킬 수 있습니다.

다음과 같은 개선을 제안합니다:

-<div key={item.markerId}>
+<div 
+  key={item.markerId}
+  role="listitem"
+  aria-label={`마커: ${item.title}`}
+>
  <div
    className={`flex flex-row justify-between gap-4 items-center ${
      index !== allMarkers.length - 1 && 'pb-2'
    } `}
  >

또한 무한 스크롤 로딩을 위한 observer target에 적절한 aria 레이블을 추가하는 것이 좋습니다:

-<div ref={observerTarget} />
+<div 
+  ref={observerTarget}
+  aria-hidden="true"
+  role="presentation" 
+/>
src/components/features/BookmarkList/BookmarkList.tsx (2)

25-29: 타입 안전성 개선이 필요합니다

useBookMarkList 훅의 반환 타입이 명시적으로 정의되어 있지 않아 타입 안전성이 보장되지 않습니다. 다음과 같이 개선하는 것을 제안합니다:

interface BookMarkPage {
  data: Array<{
    bookmarkId: number;
    name: string;
    youtuberProfile?: string;
    markersCount: number;
  }>;
  nextCursor?: string;
}

interface UseBookMarkListReturn {
  data?: {
    pages: BookMarkPage[];
  };
  fetchNextPage: () => Promise<void>;
  hasNextPage: boolean;
  isFetchingNextPage: boolean;
}

71-93: Intersection Observer 성능 최적화가 필요합니다

현재 구현은 정상적으로 작동하지만, 다음과 같은 최적화를 고려해보세요:

  1. rootMarginthreshold 값을 상수로 분리하여 재사용성을 높이고 성능을 개선할 수 있습니다.
  2. observer 콜백에서 불필요한 반복을 줄일 수 있습니다.
+const OBSERVER_OPTIONS = {
+  root: null,
+  rootMargin: '20px',
+  threshold: 0.1,
+} as const;

 const handleObserver = useCallback(
   (entries: IntersectionObserverEntry[]) => {
-    entries.forEach((entry) => {
-      if (entry.isIntersecting && hasNextPage && !isFetchingNextPage) {
-        fetchNextPage();
-      }
-    });
+    const target = entries[0];
+    if (target?.isIntersecting && hasNextPage && !isFetchingNextPage) {
+      fetchNextPage();
+    }
   },
   [fetchNextPage, hasNextPage, isFetchingNextPage]
 );
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 89d8eb8 and 07d9ea0.

📒 Files selected for processing (3)
  • src/components/features/BookmarkDetail/BookmarkDetail.tsx (5 hunks)
  • src/components/features/BookmarkList/BookmarkList.tsx (4 hunks)
  • src/components/features/SearchResultsList/BookmarkSelectionList.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/components/features/SearchResultsList/BookmarkSelectionList.tsx
🔇 Additional comments (1)
src/components/features/BookmarkList/BookmarkList.tsx (1)

111-153: 이전 리뷰 피드백이 잘 반영되었습니다

observerTarget ref를 map 내부에서 제거하고 별도의 div로 분리한 것이 좋은 개선입니다. 현재 구현은 무한 스크롤 기능을 안정적으로 제공할 수 있습니다.

Comment on lines 44 to 64
const allMarkers = useMemo(
() => (data?.pages ? data.pages.flatMap((page) => page.markers.data) : []),
[data?.pages]
);

useEffect(() => {
if (data?.markers?.data) {
setMarkers(data.markers.data);
if (allMarkers.length) {
setMarkers((prevMarkers) => {
const newMarkersStr = JSON.stringify(allMarkers);
const prevMarkersStr = JSON.stringify(prevMarkers);
return newMarkersStr !== prevMarkersStr ? allMarkers : prevMarkers;
});
}
}, [data?.markers?.data, setMarkers]);
}, [allMarkers, setMarkers]);

const selectedMarkerName = useMemo(
() =>
data?.pages.flatMap((page) => page.markers.data).find((item) => item.markerId === selectedId)
?.title || '마커',
[data?.pages, selectedId]
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

성능 최적화 필요

현재 구현에서 두 가지 주요 성능 이슈가 있습니다:

  1. allMarkers 계산 시 매번 전체 페이지를 flatMap하는 것은 비효율적일 수 있습니다.
  2. selectedMarkerName을 찾기 위해 다시 전체 페이지를 flatMap하는 것은 중복 연산입니다.

다음과 같이 개선할 수 있습니다:

const allMarkers = useMemo(
  () => (data?.pages ? data.pages.flatMap((page) => page.markers.data) : []),
  [data?.pages]
);

-const selectedMarkerName = useMemo(
-  () =>
-    data?.pages.flatMap((page) => page.markers.data).find((item) => item.markerId === selectedId)
-      ?.title || '마커',
-  [data?.pages, selectedId]
-);
+const selectedMarkerName = useMemo(
+  () => allMarkers.find((item) => item.markerId === selectedId)?.title || '마커',
+  [allMarkers, selectedId]
+);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const allMarkers = useMemo(
() => (data?.pages ? data.pages.flatMap((page) => page.markers.data) : []),
[data?.pages]
);
useEffect(() => {
if (data?.markers?.data) {
setMarkers(data.markers.data);
if (allMarkers.length) {
setMarkers((prevMarkers) => {
const newMarkersStr = JSON.stringify(allMarkers);
const prevMarkersStr = JSON.stringify(prevMarkers);
return newMarkersStr !== prevMarkersStr ? allMarkers : prevMarkers;
});
}
}, [data?.markers?.data, setMarkers]);
}, [allMarkers, setMarkers]);
const selectedMarkerName = useMemo(
() =>
data?.pages.flatMap((page) => page.markers.data).find((item) => item.markerId === selectedId)
?.title || '마커',
[data?.pages, selectedId]
);
const allMarkers = useMemo(
() => (data?.pages ? data.pages.flatMap((page) => page.markers.data) : []),
[data?.pages]
);
useEffect(() => {
if (allMarkers.length) {
setMarkers((prevMarkers) => {
const newMarkersStr = JSON.stringify(allMarkers);
const prevMarkersStr = JSON.stringify(prevMarkers);
return newMarkersStr !== prevMarkersStr ? allMarkers : prevMarkers;
});
}
}, [allMarkers, setMarkers]);
const selectedMarkerName = useMemo(
() => allMarkers.find((item) => item.markerId === selectedId)?.title || '마커',
[allMarkers, selectedId]
);

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/components/features/BookmarkDetail/BookmarkDetail.tsx (1)

100-100: data가 없을 경우 기본값 추가 권장

data?.pages[0]?.bookmarkName를 사용할 때, datapages가 없을 경우를 대비하여 기본값을 제공하는 것이 좋습니다. 예를 들어:

- <span className="text-primary mr-2">{data?.pages[0]?.bookmarkName}</span>
+ <span class="text-primary mr-2">{data?.pages[0]?.bookmarkName ?? '북마크 이름'}</span>
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 07d9ea0 and 777ecef.

📒 Files selected for processing (1)
  • src/components/features/BookmarkDetail/BookmarkDetail.tsx (4 hunks)
🔇 Additional comments (1)
src/components/features/BookmarkDetail/BookmarkDetail.tsx (1)

63-72: Intersection Observer를 활용한 무한 스크롤 구현이 훌륭합니다

handleObserver 함수와 useEffect를 사용하여 무한 스크롤 기능을 잘 구현하셨습니다. 코드의 구조와 의도가 명확하여 유지보수에 용이할 것으로 보입니다.

@keemsebin keemsebin merged commit 45c8018 into main Nov 26, 2024
5 checks passed
@keemsebin keemsebin deleted the feat/#69-pagination branch November 26, 2024 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 기능
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] cursor 방식 페이지네이션 적용
2 participants