✨ #85 홈피드 구현 및 플레이리스트 타입 정리 #122
Conversation
리뷰어 가이드 by Sourcery이 풀 리퀘스트는 홈 피드 기능을 구현하고 재생목록 유형을 재구성합니다. 홈페이지, 재생목록 구성 요소 및 관련 서비스에 대한 중요한 변경 사항이 포함되어 있습니다. 구현은 팔로우한 사용자의 재생목록을 가져오고 표시하는 데 중점을 두며, 무한 스크롤링과 개선된 UI 구성 요소를 제공합니다. 파일 수준 변경 사항
팁Original review guide in EnglishReviewer's Guide by SourceryThis pull request implements the home feed functionality and reorganizes playlist types. It includes significant changes to the homepage, playlist components, and related services. The implementation focuses on fetching and displaying playlists from followed users, with infinite scrolling and improved UI components. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
안녕하세요 @seoyoonyi - 변경 사항을 검토했으며 훌륭합니다!
검토 중에 살펴본 내용입니다
- 🟡 일반 문제: 3개의 문제가 발견되었습니다
- 🟢 보안: 모두 양호합니다
- 🟢 테스트: 모두 양호합니다
- 🟡 복잡성: 1개의 문제가 발견되었습니다
- 🟢 문서화: 모두 양호합니다
더 유용하게 도와주세요! 각 댓글에 대해 👍 또는 👎를 클릭하여 도움이 되었는지 알려주세요.
Original comment in English
Hey @seoyoonyi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| const HomePage = () => { | ||
| return <div>Home</div> | ||
| const navigate = useNavigate() | ||
| const [allPlaylists, setAllPlaylists] = useState<FollowedPlaylist[]>([]) |
There was a problem hiding this comment.
suggestion (performance): 대규모 데이터셋에서 성능을 향상시키기 위해 가상화된 리스트를 사용하는 것을 고려하세요
플레이리스트 목록이 증가함에 따라 모든 항목을 한 번에 렌더링하면 성능 문제가 발생할 수 있습니다. react-window 또는 react-virtualized와 같은 가상화된 리스트 컴포넌트를 사용하여 보이는 항목만 렌더링하는 것을 고려하세요.
import { useState } from 'react'
import { FixedSizeList as List } from 'react-window'
import { FollowedPlaylist } from '../types'
const HomePage = () => {
const [allPlaylists, setAllPlaylists] = useState<FollowedPlaylist[]>([])
const [virtualizedPlaylists, setVirtualizedPlaylists] = useState<FollowedPlaylist[]>([])
Original comment in English
suggestion (performance): Consider using a virtualized list for better performance with large datasets
As the list of playlists grows, rendering all items at once can lead to performance issues. Consider using a virtualized list component like react-window or react-virtualized to render only the visible items.
import { useState } from 'react'
import { FixedSizeList as List } from 'react-window'
import { FollowedPlaylist } from '../types'
const HomePage = () => {
const [allPlaylists, setAllPlaylists] = useState<FollowedPlaylist[]>([])
const [virtualizedPlaylists, setVirtualizedPlaylists] = useState<FollowedPlaylist[]>([])
| const handleScroll = useCallback(() => { | ||
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) { |
There was a problem hiding this comment.
suggestion (performance): 고정 픽셀 값 대신 뷰포트 높이의 백분율을 스크롤 트리거로 사용하세요
고정 픽셀 값(100)을 사용하는 것은 다양한 장치와 화면 크기에서 잘 작동하지 않을 수 있습니다. 대신 뷰포트 높이의 백분율을 사용하는 것을 고려하세요, 예를 들어 '90%'의 document.documentElement.scrollHeight.
const handleScroll = useCallback(() => {
const scrollThreshold = 0.9; // 문서 높이의 90%
if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight * scrollThreshold) {
if (hasNextPage && !isFetchingNextPage) {
fetchNextPage();
}
}
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
Original comment in English
suggestion (performance): Use percentage of viewport height for scroll trigger instead of fixed pixel value
Using a fixed pixel value (100) might not work well across different devices and screen sizes. Consider using a percentage of the viewport height instead, e.g., '90%' of document.documentElement.scrollHeight.
const handleScroll = useCallback(() => {
const scrollThreshold = 0.9; // 90% of the document height
if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight * scrollThreshold) {
if (hasNextPage && !isFetchingNextPage) {
fetchNextPage();
}
}
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
| if (thumbnails.length === 3) { | ||
| displayThumbnails.push(defaultThumbnail) |
There was a problem hiding this comment.
suggestion: 세 개의 썸네일을 처리하는 접근 방식을 재고하세요
정확히 세 개의 썸네일이 있을 때 기본 썸네일을 추가하는 것은 임의로 보입니다. 썸네일 수에 따라 그리드 레이아웃을 조정하거나 이 경우 사용자 정의 동작을 위한 prop을 허용하는 등 더 유연한 접근 방식을 고려하세요.
const gridTemplateAreas = {
1: '"a"',
2: '"a b"',
3: '"a b" "c ."',
4: '"a b" "c d"'
};
const gridStyle = {
display: 'grid',
gridTemplateAreas: gridTemplateAreas[thumbnails.length] || gridTemplateAreas[4]
};
Original comment in English
suggestion: Reconsider the approach for handling three thumbnails
Adding a default thumbnail when there are exactly three thumbnails seems arbitrary. Consider a more flexible approach, such as adjusting the grid layout based on the number of thumbnails, or allowing the component to accept a prop for custom behavior in this case.
const gridTemplateAreas = {
1: '"a"',
2: '"a b"',
3: '"a b" "c ."',
4: '"a b" "c d"'
};
const gridStyle = {
display: 'grid',
gridTemplateAreas: gridTemplateAreas[thumbnails.length] || gridTemplateAreas[4]
};
| import { fontSize, fontWeight } from '@/constants/font' | ||
| import { useNavigate } from 'react-router-dom' | ||
| import { FollowedPlaylist, FollowedUserPlaylists } from '@/types/playlistType' | ||
|
|
||
| const HomePage = () => { |
There was a problem hiding this comment.
issue (complexity): 코드 조직 및 재사용성을 개선하기 위해 HomePage 컴포넌트를 리팩토링하는 것을 고려하세요.
변경 사항은 중요한 새로운 기능을 도입하지만 복잡성을 줄일 수 있는 기회가 있습니다:
- 무한 스크롤 로직을 사용자 정의 훅으로 추출:
// useInfiniteScroll.js
import { useCallback, useEffect } from 'react';
const useInfiniteScroll = (hasNextPage, isFetchingNextPage, fetchNextPage) => {
const handleScroll = useCallback(() => {
if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) {
if (hasNextPage && !isFetchingNextPage) {
fetchNextPage();
}
}
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
useEffect(() => {
window.addEventListener('scroll', handleScroll);
return () => window.removeEventListener('scroll', handleScroll);
}, [handleScroll]);
};
export default useInfiniteScroll;HomePage에서 사용:
useInfiniteScroll(hasNextPage, isFetchingNextPage, fetchNextPage);- 플레이리스트 렌더링을 별도의 컴포넌트로 이동:
const PlaylistItem = ({ playlist, onUserClick, onPlaylistClick }) => (
<li className="playlist-container">
<UserInfo
authorName={playlist.authorName}
authorImg={playlist.authorImg}
createdAt={playlist.createdAt}
className="user-info-container"
onClick={() => onUserClick(playlist.authorId)}
/>
<div onClick={() => onPlaylistClick(playlist.playlistId)} className="playlist-detail-container">
<h3 className="playlist-title">{truncateText(playlist.title, 60)}</h3>
<PlaylistThumbnailFeed thumbnails={playlist.thumbnails} />
</div>
<LikeButton playlistId={playlist.playlistId || ''} />
</li>
);- 데이터 변환을 단순화:
useEffect(() => {
if (data?.pages) {
const mergedPlaylists = data.pages
.flatMap(page => page.flatMap(user => user.playlists))
.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime());
setAllPlaylists(mergedPlaylists);
}
}, [data]);- 대규모 데이터셋에서 성능 문제가 발생할 경우 react-window를 사용하여 효율적인 리스트 렌더링을 고려하세요.
이러한 변경 사항은 기능을 유지하면서 코드 조직 및 유지 보수성을 향상시킬 것입니다.
Original comment in English
issue (complexity): Consider refactoring the HomePage component to improve code organization and reusability.
While the changes introduce important new features, there are opportunities to reduce complexity:
- Extract infinite scrolling logic into a custom hook:
// useInfiniteScroll.js
import { useCallback, useEffect } from 'react';
const useInfiniteScroll = (hasNextPage, isFetchingNextPage, fetchNextPage) => {
const handleScroll = useCallback(() => {
if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) {
if (hasNextPage && !isFetchingNextPage) {
fetchNextPage();
}
}
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
useEffect(() => {
window.addEventListener('scroll', handleScroll);
return () => window.removeEventListener('scroll', handleScroll);
}, [handleScroll]);
};
export default useInfiniteScroll;Use it in HomePage:
useInfiniteScroll(hasNextPage, isFetchingNextPage, fetchNextPage);- Move playlist rendering to a separate component:
const PlaylistItem = ({ playlist, onUserClick, onPlaylistClick }) => (
<li className="playlist-container">
<UserInfo
authorName={playlist.authorName}
authorImg={playlist.authorImg}
createdAt={playlist.createdAt}
className="user-info-container"
onClick={() => onUserClick(playlist.authorId)}
/>
<div onClick={() => onPlaylistClick(playlist.playlistId)} className="playlist-detail-container">
<h3 className="playlist-title">{truncateText(playlist.title, 60)}</h3>
<PlaylistThumbnailFeed thumbnails={playlist.thumbnails} />
</div>
<LikeButton playlistId={playlist.playlistId || ''} />
</li>
);- Simplify data transformation:
useEffect(() => {
if (data?.pages) {
const mergedPlaylists = data.pages
.flatMap(page => page.flatMap(user => user.playlists))
.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime());
setAllPlaylists(mergedPlaylists);
}
}, [data]);- Consider using react-window for efficient list rendering if performance becomes an issue with large datasets.
These changes will improve code organization and maintainability while preserving functionality.
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) { | ||
| if (hasNextPage && !isFetchingNextPage) { | ||
| fetchNextPage() | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): 중첩된 if 조건을 병합하세요 (merge-nested-ifs)
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) { | |
| if (hasNextPage && !isFetchingNextPage) { | |
| fetchNextPage() | |
| } | |
| } | |
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100 && (hasNextPage && !isFetchingNextPage)) { | |
| fetchNextPage() | |
| } | |
설명
깊게 중첩된 조건부 코드를 읽는 것은 혼란스러울 수 있습니다. 어떤 조건이 어떤 수준과 관련이 있는지 추적해야 하기 때문입니다. 따라서 가능한 경우 중첩을 줄이려고 노력하며, 두if 조건을 and로 결합할 수 있는 상황은 쉽게 개선할 수 있는 부분입니다.
Original comment in English
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100) { | |
| if (hasNextPage && !isFetchingNextPage) { | |
| fetchNextPage() | |
| } | |
| } | |
| if (window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 100 && (hasNextPage && !isFetchingNextPage)) { | |
| fetchNextPage() | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
| }, [hasNextPage, isFetchingNextPage, fetchNextPage]) | ||
|
|
||
| const truncateText = (text: string | undefined, maxLength: number) => { | ||
| if (!text) return '' |
There was a problem hiding this comment.
suggestion (code-quality): if, while 등에서 블록 중괄호를 사용하세요 (use-braces)
| if (!text) return '' | |
| if (!text) { |
설명
항상 중괄호를 사용하고 명시적인 문장 블록을 만드는 것이 권장됩니다.단일 문장을 작성하기 위해 허용된 구문을 사용하는 것은 매우 혼란스러운 상황을 초래할 수 있습니다. 특히 이후에 개발자가 중괄호를 추가하는 것을 잊고 다른 문장을 추가할 경우(이 경우 조건에 포함되지 않음) 더욱 그렇습니다.
Original comment in English
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!text) return '' | |
| if (!text) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| const unsubscribe = auth.onAuthStateChanged(async (user) => { | ||
| if (user) { | ||
| try { | ||
| const uid = user.uid |
There was a problem hiding this comment.
suggestion (code-quality): 속성을 접근하고 사용할 때 객체 구조 분해를 선호하세요 (use-object-destructuring)
| const uid = user.uid | |
| const {uid} = user |
설명
객체 구조 분해는 불필요한 임시 참조를 제거할 수 있으며, 코드를 더 간결하게 만듭니다.Original comment in English
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const uid = user.uid | |
| const {uid} = user |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| const unsubscribe = auth.onAuthStateChanged(async (user) => { | ||
| if (user) { | ||
| try { | ||
| const uid = user.uid |
There was a problem hiding this comment.
suggestion (code-quality): 속성을 접근하고 사용할 때 객체 구조 분해를 선호하세요 (use-object-destructuring)
| const uid = user.uid | |
| const {uid} = user |
설명
객체 구조 분해는 불필요한 임시 참조를 제거할 수 있으며, 코드를 더 간결하게 만듭니다.Original comment in English
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const uid = user.uid | |
| const {uid} = user |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
📋 풀리퀘스트 관련 코멘트
작업하면서 공통컴포넌트도 쪼개고, 파일이동 대거 했습니다.
플레이리스트 타입이 중복으로 되어 있어서 정리했고
홈피드 구현했습니다.
물론 무한스크롤링 적용을 했지만, api 양이 적기때문에... 늘려서 다시 테스트 해봐야될것 같습니다.
Sourcery에 의한 요약
팔로우한 사용자의 재생 목록을 표시하기 위해 무한 스크롤이 가능한 홈 피드를 구현하고, 일관성을 높이기 위해 재생 목록 유형을 리팩토링합니다. 사용자 정보와 재생 목록 썸네일을 표시하기 위한 새로운 구성 요소를 도입하고, 이러한 새로운 기능을 활용하기 위해 기존 구성 요소를 개선합니다.
새로운 기능:
개선 사항:
Original summary in English
Summary by Sourcery
Implement the home feed with infinite scrolling to display playlists from followed users, and refactor playlist types for better consistency. Introduce new components for displaying user information and playlist thumbnails, and enhance existing components to utilize these new features.
New Features:
Enhancements: