๐ฅ๐ฅ๐ฅ์ฝ๋๋ฆฌ๋ทฐ๐ฅ๐ฅ๐ฅ #123
Conversation
๋ฆฌ๋ทฐ์ด ๊ฐ์ด๋ by Sourcery์ด ํ ๋ฆฌํ์คํธ๋ ํ๋ก์ ํธ ๊ตฌ์กฐ์ ๋๋์ ์ธ ๊ฐํธ์ ๊ตฌํํ๊ณ React ๊ธฐ๋ฐ ์น ์ ํ๋ฆฌ์ผ์ด์ ์ ์ํ ์๋ก์ด ๊ธฐ๋ฅ์ ๋์ ํฉ๋๋ค. ๋ณ๊ฒฝ ์ฌํญ์๋ ๋ผ์ฐํ ์ค์ , ์ธ์ฆ ํ๋ฆ ๊ตฌํ, ๋ค์ํ ํ์ด์ง ๋ฐ ๊ตฌ์ฑ ์์ ์์ฑ, ๋ฐฑ์๋ ์๋น์ค๋ฅผ ์ํ Firebase ํตํฉ์ด ํฌํจ๋ฉ๋๋ค. ํ์ผ ์์ค ๋ณ๊ฒฝ ์ฌํญ
ํ
Original review guide in EnglishReviewer's Guide by SourceryThis pull request implements a significant overhaul of the project structure and introduces new features for a React-based web application. The changes include setting up routing, implementing authentication flows, creating various pages and components, and integrating with Firebase for backend services. 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 - ๋ณ๊ฒฝ ์ฌํญ์ ๊ฒํ ํ์ต๋๋ค - ๋ค์์ ํผ๋๋ฐฑ์ ๋๋ค:
์ ๋ฐ์ ์ธ ์๊ฒฌ:
- ํ๋ก์ ํธ๊ฐ ์ฑ์ฅํจ์ ๋ฐ๋ผ ์ ๋ขฐ์ฑ์ ๋ณด์ฅํ๊ธฐ ์ํด ์๋ก์ด ์ปดํฌ๋ํธ์ ํ ์ ๋ํ ๋จ์ ํ ์คํธ๋ฅผ ์ถ๊ฐํ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์.
- ํนํ ๋คํธ์ํฌ ์์ฒญ๊ณผ Firebase ์์ ์ ๋ํด ๋ ํฌ๊ด์ ์ธ ์ค๋ฅ ์ฒ๋ฆฌ ๋ฐ ๋ก๊น ์ ๊ตฌํํ๋ ๊ฒ์ด ์ ์ตํ ์ ์์ต๋๋ค.
๊ฒํ ์ค์ ์ดํด๋ณธ ๋ด์ฉ์ ๋๋ค
- ๐ก ์ผ๋ฐ ๋ฌธ์ : 10๊ฐ์ ๋ฌธ์ ๋ฐ๊ฒฌ
- ๐ก ๋ณด์: 1๊ฐ์ ๋ฌธ์ ๋ฐ๊ฒฌ
- ๐ข ํ ์คํธ: ๋ชจ๋ ์ํธ
- ๐ข ๋ณต์ก์ฑ: ๋ชจ๋ ์ํธ
- ๐ก ๋ฌธ์ํ: 3๊ฐ์ ๋ฌธ์ ๋ฐ๊ฒฌ
Sourcery๋ ์คํ ์์ค์ ๋ฌด๋ฃ์ ๋๋ค - ๋ฆฌ๋ทฐ๊ฐ ๋ง์์ ๋์ จ๋ค๋ฉด ๊ณต์ ๋ฅผ ๊ณ ๋ คํด ์ฃผ์ธ์ โจ
Original comment in English
Hey @seoyoonyi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests for the new components and hooks to ensure reliability as the project grows.
- It might be beneficial to implement more comprehensive error handling and logging throughout the application, especially for network requests and Firebase operations.
Here's what I looked at during the review
- ๐ก General issues: 10 issues found
- ๐ก Security: 1 issue found
- ๐ข Testing: all looks good
- ๐ข Complexity: all looks good
- ๐ก Documentation: 3 issues found
Help me be more useful! Please click ๐ or ๐ on each comment to tell me if it was helpful.
src/App.tsx
Outdated
| <p className="read-the-docs">Click on the Vite and React logos to learn more</p> | ||
| </> | ||
| ) | ||
| const PrivateRoute = () => { |
There was a problem hiding this comment.
suggestion: ๋ ๋์ ์กฐ์งํ๋ฅผ ์ํด PrivateRoute๋ฅผ ๋ณ๋์ ํ์ผ๋ก ์ด๋ํ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์
์ด๋ ๊ฒ ํ๋ฉด ์ฝ๋ ์กฐ์ง์ด ๊ฐ์ ๋๊ณ PrivateRoute ์ปดํฌ๋ํธ๋ฅผ ๋ณ๋๋ก ํ ์คํธํ๊ณ ์ ์ง ๊ด๋ฆฌํ๊ธฐ๊ฐ ๋ ์ฌ์์ง๋๋ค.
// src/components/PrivateRoute.tsx
import { Navigate, Outlet } from 'react-router-dom';
import { checkAuth } from '../utils/auth';
import { PATH } from '../constants/paths';
export const PrivateRoute = () => {
return checkAuth() ? <Outlet /> : <Navigate to={PATH.LOGIN} replace />;
};
Original comment in English
suggestion: Consider moving PrivateRoute to a separate file for better organization
This would improve code organization and make it easier to test and maintain the PrivateRoute component separately.
// src/components/PrivateRoute.tsx
import { Navigate, Outlet } from 'react-router-dom';
import { checkAuth } from '../utils/auth';
import { PATH } from '../constants/paths';
export const PrivateRoute = () => {
return checkAuth() ? <Outlet /> : <Navigate to={PATH.LOGIN} replace />;
};
| import { useNavigate } from 'react-router-dom' | ||
| import { getLoggedInUserUID, getUserIdFromUID } from '@/utils/userDataUtils' | ||
|
|
||
| const CreatePlaylistPage = () => { |
There was a problem hiding this comment.
suggestion: ์ด ํฐ ์ปดํฌ๋ํธ๋ฅผ ๋ ์๊ณ ์ง์ค๋ ์ปดํฌ๋ํธ๋ก ๋๋๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์
์ด ์ปดํฌ๋ํธ๋ ์ํ ๊ด๋ฆฌ, ํผ ์ฒ๋ฆฌ, API ํธ์ถ ๋ฑ ์ฌ๋ฌ ์ฑ ์์ ์ฒ๋ฆฌํ๊ณ ์์ต๋๋ค. ์ด๋ฅผ ๋๋๋ฉด ๊ฐ๋ ์ฑ๊ณผ ์ ์ง ๊ด๋ฆฌ๊ฐ ๊ฐ์ ๋ฉ๋๋ค.
import { CreatePlaylistForm } from '@/components/CreatePlaylistForm'
import { PlaylistMetadata } from '@/components/PlaylistMetadata'
const CreatePlaylistPage = () => {
return (
<>
<PlaylistMetadata />
<CreatePlaylistForm />
</>
)
}
Original comment in English
suggestion: Consider breaking down this large component into smaller, more focused components
This component is handling multiple responsibilities including state management, form handling, and API calls. Breaking it down would improve readability and maintainability.
import { CreatePlaylistForm } from '@/components/CreatePlaylistForm'
import { PlaylistMetadata } from '@/components/PlaylistMetadata'
const CreatePlaylistPage = () => {
return (
<>
<PlaylistMetadata />
<CreatePlaylistForm />
</>
)
}
| import { BasicVideoProps } from '@/types/playlistType' | ||
| import { useState } from 'react' | ||
|
|
||
| const usePLItem = () => { |
There was a problem hiding this comment.
suggestion: ์ด ์ปค์คํ ํ ์ ๋ณต์กํ ๋ก์ง์ ์ค๋ช ํ๋ ์ฃผ์์ ์ถ๊ฐํ์ธ์
์ด ํ ์ ๋ก์ง์ ๊ฝค ๋ณต์กํฉ๋๋ค. ๊ฐ ํจ์์ ์ํ ๋ณ์์ ๋ชฉ์ ์ ์ค๋ช ํ๋ ์ฃผ์์ ์ถ๊ฐํ๋ฉด ๊ฐ๋ ์ฑ๊ณผ ์ ์ง ๊ด๋ฆฌ๊ฐ ํฌ๊ฒ ๊ฐ์ ๋ฉ๋๋ค.
Original comment in English
suggestion: Add comments explaining the complex logic in this custom hook
The logic in this hook is quite complex. Adding comments to explain the purpose of each function and state variable would greatly improve its readability and maintainability.
| import { getLoggedInUserUID } from '@/utils/userDataUtils' | ||
| import { addDoc, collection, getDoc, doc } from 'firebase/firestore' | ||
|
|
||
| const createNewPlaylist = async ( |
There was a problem hiding this comment.
suggestion: ๋คํธ์ํฌ ์คํจ๋ ์์์น ๋ชปํ ์๋ต์ ๋ํ ์ค๋ฅ ์ฒ๋ฆฌ๋ฅผ ์ถ๊ฐํ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์
ํ์ฌ ํจ์๋ ์ค๋ฅ๋ฅผ ์ฝ์์๋ง ๊ธฐ๋กํฉ๋๋ค. ํนํ ๋คํธ์ํฌ ์คํจ๋ ์์์น ๋ชปํ ์๋ฒ ์๋ต์ ๋ํด ์ฌ์ฉ์์๊ฒ ์ค๋ฅ๋ฅผ ์ฒ๋ฆฌํ๊ณ ์ ๋ฌํ๋ ๋ฐฉ๋ฒ์ ๊ณ ๋ คํด๋ณด์ธ์.
const createNewPlaylist = async (
title: string,
tags: string[],
): Promise<string | null> => {
try {
// Existing function body...
} catch (error) {
console.error('Failed to create playlist:', error);
throw new Error('Failed to create playlist. Please try again later.');
}
}
Original comment in English
suggestion: Consider adding error handling for network failures or unexpected responses
The function currently only logs errors to the console. Consider how to handle and communicate errors to the user, especially for network failures or unexpected server responses.
const createNewPlaylist = async (
title: string,
tags: string[],
): Promise<string | null> => {
try {
// Existing function body...
} catch (error) {
console.error('Failed to create playlist:', error);
throw new Error('Failed to create playlist. Please try again later.');
}
}
| playlistId: string | ||
| } | ||
|
|
||
| const Comments: React.FC<CommentsProps> = ({ playlistId }) => { |
There was a problem hiding this comment.
suggestion (performance): ๋๊ธ์ ๋ํด ํ์ด์ง๋ค์ด์ ๋๋ ๋ฌดํ ์คํฌ๋กค์ ๊ตฌํํ๋ ๊ฒ์ ๊ณ ๋ คํด๋ณด์ธ์
ํ๋ ์ด๋ฆฌ์คํธ์ ๋ง์ ๋๊ธ์ด ์์ ์ ์๋ค๋ฉด, ์ด๋ฅผ ํ ๋ฒ์ ๋ชจ๋ ๋ก๋ํ๋ ๊ฒ์ ์ฑ๋ฅ์ ์ํฅ์ ๋ฏธ์น ์ ์์ต๋๋ค. ํ์ด์ง๋ค์ด์ ๋๋ ๋ฌดํ ์คํฌ๋กค์ ๊ตฌํํ๋ฉด ๋ง์ ๋๊ธ์ด ์๋ ํ๋ ์ด๋ฆฌ์คํธ์ ์ฌ์ฉ์ ๊ฒฝํ๊ณผ ์ฑ๋ฅ์ ๊ฐ์ ํ ์ ์์ต๋๋ค.
const Comments: React.FC<CommentsProps> = ({ playlistId }) => {
const [page, setPage] = useState(1);
const [hasMore, setHasMore] = useState(true);
const { data: comments, fetchNextPage, isFetchingNextPage } = useInfiniteQuery(
['comments', playlistId],
({ pageParam = 1 }) => fetchComments(playlistId, pageParam),
{
getNextPageParam: (lastPage, pages) => lastPage.nextPage,
}
);
Original comment in English
suggestion (performance): Consider implementing pagination or infinite scrolling for comments
If a playlist can have many comments, loading them all at once could impact performance. Implementing pagination or infinite scrolling could improve the user experience and performance for playlists with many comments.
const Comments: React.FC<CommentsProps> = ({ playlistId }) => {
const [page, setPage] = useState(1);
const [hasMore, setHasMore] = useState(true);
const { data: comments, fetchNextPage, isFetchingNextPage } = useInfiniteQuery(
['comments', playlistId],
({ pageParam = 1 }) => fetchComments(playlistId, pageParam),
{
getNextPageParam: (lastPage, pages) => lastPage.nextPage,
}
);
| // ํ์ฌ ๋ก๊ทธ์ธ๋ ์ฌ์ฉ์๊ฐ ํ๋ก์ฐํ ์ฌ์ฉ์๋ค์ ํ๋ ์ด๋ฆฌ์คํธ๋ฅผ ๊ฐ์ ธ์ค๋ ํจ์ | ||
| const getFollowingUsersPlaylists = async () => { | ||
| try { | ||
| const currentUser = auth.currentUser |
There was a problem hiding this comment.
suggestion (code-quality): ์์ฑ์ ์ก์ธ์คํ๊ณ ์ฌ์ฉํ ๋ ๊ฐ์ฒด ๊ตฌ์กฐ ๋ถํด๋ฅผ ์ ํธํ์ธ์. (use-object-destructuring)
| const currentUser = auth.currentUser | |
| const {currentUser} = auth |
Explanation
๊ฐ์ฒด ๊ตฌ์กฐ ๋ถํด๋ ๋ถํ์ํ ์์ ์ฐธ์กฐ๋ฅผ ์ ๊ฑฐํ ์ ์์ผ๋ฉฐ, ์ฝ๋๋ฅผ ๋ ๊ฐ๊ฒฐํ๊ฒ ๋ง๋ญ๋๋ค.Airbnb Javascript Style Guide์์ ๋ฐ์ท
Original comment in English
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const currentUser = auth.currentUser | |
| const {currentUser} = auth |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| throw new Error('User is not logged in') | ||
| } | ||
|
|
||
| const uid = currentUser.uid |
There was a problem hiding this comment.
suggestion (code-quality): ์์ฑ์ ์ก์ธ์คํ๊ณ ์ฌ์ฉํ ๋ ๊ฐ์ฒด ๊ตฌ์กฐ ๋ถํด๋ฅผ ์ ํธํ์ธ์. (use-object-destructuring)
| const uid = currentUser.uid | |
| const {uid} = currentUser |
Explanation
๊ฐ์ฒด ๊ตฌ์กฐ ๋ถํด๋ ๋ถํ์ํ ์์ ์ฐธ์กฐ๋ฅผ ์ ๊ฑฐํ ์ ์์ผ๋ฉฐ, ์ฝ๋๋ฅผ ๋ ๊ฐ๊ฒฐํ๊ฒ ๋ง๋ญ๋๋ค.Airbnb Javascript Style Guide์์ ๋ฐ์ท
Original comment in English
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const uid = currentUser.uid | |
| const {uid} = currentUser |
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 followingPlaylists = await Promise.all( | ||
| followingUserIds.map(async (userId: string) => { | ||
| const userPlaylists = await getUserPlaylists(userId) | ||
| return { userId, playlists: userPlaylists } | ||
| }) | ||
| ) | ||
|
|
||
| return followingPlaylists |
There was a problem hiding this comment.
suggestion (code-quality): ์ฆ์ ๋ฐํ๋๋ ๋ณ์๋ฅผ ์ธ๋ผ์ธํ์ธ์ (inline-immediately-returned-variable)
| const followingPlaylists = await Promise.all( | |
| followingUserIds.map(async (userId: string) => { | |
| const userPlaylists = await getUserPlaylists(userId) | |
| return { userId, playlists: userPlaylists } | |
| }) | |
| ) | |
| return followingPlaylists | |
| return await Promise.all( | |
| followingUserIds.map(async (userId: string) => { | |
| const userPlaylists = await getUserPlaylists(userId) | |
| return { userId, playlists: userPlaylists } | |
| }) | |
| ); | |
Explanation
์ฌ๋๋ค์ ์ฝ๋์์ ์์ฃผ ๋ณผ ์ ์๋ ๊ฒ์ ๊ฒฐ๊ณผ ๋ณ์์ ํ ๋นํ ํ ์ฆ์ ๋ฐํํ๋ ๊ฒ์ ๋๋ค.๊ฒฐ๊ณผ๋ฅผ ์ง์ ๋ฐํํ๋ฉด ์ฝ๋๋ฅผ ๋จ์ถํ๊ณ ๋ถํ์ํ ๋ณ์๋ฅผ ์ ๊ฑฐํ์ฌ ํจ์๋ฅผ ์ฝ๋ ๋ฐ ๋๋ ์ ์ ์ ๋ถ๋ด์ ์ค์ ๋๋ค.
์ค๊ฐ ๋ณ์๊ฐ ์ ์ฉํ ์ ์๋ ๊ฒฝ์ฐ๋ ๋งค๊ฐ๋ณ์๋ ์กฐ๊ฑด์ผ๋ก ์ฌ์ฉ๋ ๋์ด๋ฉฐ, ๋ณ์ ์ด๋ฆ์ด ๋ณ์์ ์๋ฏธ๋ฅผ ์ค๋ช ํ๋ ์ฃผ์์ฒ๋ผ ์์ฉํ ์ ์์ต๋๋ค. ํจ์์์ ๋ฐํํ๋ ๊ฒฝ์ฐ ํจ์ ์ด๋ฆ์ด ๊ฒฐ๊ณผ๊ฐ ๋ฌด์์ธ์ง ์๋ ค์ฃผ๋ฏ๋ก ๋ณ์ ์ด๋ฆ์ด ๋ถํ์ํฉ๋๋ค.
Original comment in English
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const followingPlaylists = await Promise.all( | |
| followingUserIds.map(async (userId: string) => { | |
| const userPlaylists = await getUserPlaylists(userId) | |
| return { userId, playlists: userPlaylists } | |
| }) | |
| ) | |
| return followingPlaylists | |
| return await Promise.all( | |
| followingUserIds.map(async (userId: string) => { | |
| const userPlaylists = await getUserPlaylists(userId) | |
| return { userId, playlists: userPlaylists } | |
| }) | |
| ); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const videos = await Promise.all( | ||
| videoSnapshot.docs.map(async (videoDoc) => { | ||
| try { | ||
| const videoData = videoDoc.data() | ||
| return { | ||
| channelTitle: videoData.channelTitle, | ||
| title: videoData.title, | ||
| thumbnail: videoData.thumbnail, | ||
| url: videoData.url, | ||
| author: userData?.id, | ||
| authorImg: userData?.img, | ||
| uploadDate: playlistData.createdAt ? formatDate(playlistData.createdAt) : '', | ||
| tags: playlistData.tags, | ||
| playlistTitle: playlistData.title, | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error fetching video data for ${videoDoc.id}:`, error) | ||
| return { | ||
| channelTitle: 'Unknown', | ||
| title: 'Unknown Video', | ||
| thumbnail: 'not valid thumbnail', | ||
| url: '', | ||
| author: userData?.id, | ||
| authorImg: userData?.img, | ||
| uploadDate: '', | ||
| tags: [], | ||
| playlistTitle: playlistData.title, | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
|
|
||
| return videos |
There was a problem hiding this comment.
suggestion (code-quality): ์ฆ์ ๋ฐํ๋๋ ๋ณ์๋ฅผ ์ธ๋ผ์ธํ์ธ์ (inline-immediately-returned-variable)
| const videos = await Promise.all( | |
| videoSnapshot.docs.map(async (videoDoc) => { | |
| try { | |
| const videoData = videoDoc.data() | |
| return { | |
| channelTitle: videoData.channelTitle, | |
| title: videoData.title, | |
| thumbnail: videoData.thumbnail, | |
| url: videoData.url, | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: playlistData.createdAt ? formatDate(playlistData.createdAt) : '', | |
| tags: playlistData.tags, | |
| playlistTitle: playlistData.title, | |
| } | |
| } catch (error) { | |
| console.error(`Error fetching video data for ${videoDoc.id}:`, error) | |
| return { | |
| channelTitle: 'Unknown', | |
| title: 'Unknown Video', | |
| thumbnail: 'not valid thumbnail', | |
| url: '', | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: '', | |
| tags: [], | |
| playlistTitle: playlistData.title, | |
| } | |
| } | |
| }) | |
| ) | |
| return videos | |
| return await Promise.all( | |
| videoSnapshot.docs.map(async (videoDoc) => { | |
| try { | |
| const videoData = videoDoc.data() | |
| return { | |
| channelTitle: videoData.channelTitle, | |
| title: videoData.title, | |
| thumbnail: videoData.thumbnail, | |
| url: videoData.url, | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: playlistData.createdAt ? formatDate(playlistData.createdAt) : '', | |
| tags: playlistData.tags, | |
| playlistTitle: playlistData.title, | |
| } | |
| } catch (error) { | |
| console.error(`Error fetching video data for ${videoDoc.id}:`, error) | |
| return { | |
| channelTitle: 'Unknown', | |
| title: 'Unknown Video', | |
| thumbnail: 'not valid thumbnail', | |
| url: '', | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: '', | |
| tags: [], | |
| playlistTitle: playlistData.title, | |
| } | |
| } | |
| }) | |
| ); | |
Explanation
์ฌ๋๋ค์ ์ฝ๋์์ ์์ฃผ ๋ณผ ์ ์๋ ๊ฒ์ ๊ฒฐ๊ณผ ๋ณ์์ ํ ๋นํ ํ ์ฆ์ ๋ฐํํ๋ ๊ฒ์ ๋๋ค.๊ฒฐ๊ณผ๋ฅผ ์ง์ ๋ฐํํ๋ฉด ์ฝ๋๋ฅผ ๋จ์ถํ๊ณ ๋ถํ์ํ ๋ณ์๋ฅผ ์ ๊ฑฐํ์ฌ ํจ์๋ฅผ ์ฝ๋ ๋ฐ ๋๋ ์ ์ ์ ๋ถ๋ด์ ์ค์ ๋๋ค.
์ค๊ฐ ๋ณ์๊ฐ ์ ์ฉํ ์ ์๋ ๊ฒฝ์ฐ๋ ๋งค๊ฐ๋ณ์๋ ์กฐ๊ฑด์ผ๋ก ์ฌ์ฉ๋ ๋์ด๋ฉฐ, ๋ณ์ ์ด๋ฆ์ด ๋ณ์์ ์๋ฏธ๋ฅผ ์ค๋ช ํ๋ ์ฃผ์์ฒ๋ผ ์์ฉํ ์ ์์ต๋๋ค. ํจ์์์ ๋ฐํํ๋ ๊ฒฝ์ฐ ํจ์ ์ด๋ฆ์ด ๊ฒฐ๊ณผ๊ฐ ๋ฌด์์ธ์ง ์๋ ค์ฃผ๋ฏ๋ก ๋ณ์ ์ด๋ฆ์ด ๋ถํ์ํฉ๋๋ค.
Original comment in English
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const videos = await Promise.all( | |
| videoSnapshot.docs.map(async (videoDoc) => { | |
| try { | |
| const videoData = videoDoc.data() | |
| return { | |
| channelTitle: videoData.channelTitle, | |
| title: videoData.title, | |
| thumbnail: videoData.thumbnail, | |
| url: videoData.url, | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: playlistData.createdAt ? formatDate(playlistData.createdAt) : '', | |
| tags: playlistData.tags, | |
| playlistTitle: playlistData.title, | |
| } | |
| } catch (error) { | |
| console.error(`Error fetching video data for ${videoDoc.id}:`, error) | |
| return { | |
| channelTitle: 'Unknown', | |
| title: 'Unknown Video', | |
| thumbnail: 'not valid thumbnail', | |
| url: '', | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: '', | |
| tags: [], | |
| playlistTitle: playlistData.title, | |
| } | |
| } | |
| }) | |
| ) | |
| return videos | |
| return await Promise.all( | |
| videoSnapshot.docs.map(async (videoDoc) => { | |
| try { | |
| const videoData = videoDoc.data() | |
| return { | |
| channelTitle: videoData.channelTitle, | |
| title: videoData.title, | |
| thumbnail: videoData.thumbnail, | |
| url: videoData.url, | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: playlistData.createdAt ? formatDate(playlistData.createdAt) : '', | |
| tags: playlistData.tags, | |
| playlistTitle: playlistData.title, | |
| } | |
| } catch (error) { | |
| console.error(`Error fetching video data for ${videoDoc.id}:`, error) | |
| return { | |
| channelTitle: 'Unknown', | |
| title: 'Unknown Video', | |
| thumbnail: 'not valid thumbnail', | |
| url: '', | |
| author: userData?.id, | |
| authorImg: userData?.img, | |
| uploadDate: '', | |
| tags: [], | |
| playlistTitle: playlistData.title, | |
| } | |
| } | |
| }) | |
| ); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const unsubscribe = onSnapshot(targetUserDocRef, (docSnapshot) => { | ||
| if (docSnapshot.exists()) { | ||
| const data = docSnapshot.data() | ||
| onUpdate( | ||
| { | ||
| followers: data.followers || [], | ||
| following: data.followings || [], | ||
| }, | ||
| data.followers?.includes(currentUID) || false | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| return unsubscribe |
There was a problem hiding this comment.
suggestion (code-quality): ์ฆ์ ๋ฐํ๋๋ ๋ณ์๋ฅผ ์ธ๋ผ์ธํ์ธ์ (inline-immediately-returned-variable)
| const unsubscribe = onSnapshot(targetUserDocRef, (docSnapshot) => { | |
| if (docSnapshot.exists()) { | |
| const data = docSnapshot.data() | |
| onUpdate( | |
| { | |
| followers: data.followers || [], | |
| following: data.followings || [], | |
| }, | |
| data.followers?.includes(currentUID) || false | |
| ) | |
| } | |
| }) | |
| return unsubscribe | |
| return onSnapshot(targetUserDocRef, (docSnapshot) => { | |
| if (docSnapshot.exists()) { | |
| const data = docSnapshot.data() | |
| onUpdate( | |
| { | |
| followers: data.followers || [], | |
| following: data.followings || [], | |
| }, | |
| data.followers?.includes(currentUID) || false | |
| ) | |
| } | |
| }); | |
Explanation
์ฌ๋๋ค์ ์ฝ๋์์ ์์ฃผ ๋ณผ ์ ์๋ ๊ฒ์ ๊ฒฐ๊ณผ ๋ณ์์ ํ ๋นํ ํ ์ฆ์ ๋ฐํํ๋ ๊ฒ์ ๋๋ค.๊ฒฐ๊ณผ๋ฅผ ์ง์ ๋ฐํํ๋ฉด ์ฝ๋๋ฅผ ๋จ์ถํ๊ณ ๋ถํ์ํ ๋ณ์๋ฅผ ์ ๊ฑฐํ์ฌ ํจ์๋ฅผ ์ฝ๋ ๋ฐ ๋๋ ์ ์ ์ ๋ถ๋ด์ ์ค์ ๋๋ค.
์ค๊ฐ ๋ณ์๊ฐ ์ ์ฉํ ์ ์๋ ๊ฒฝ์ฐ๋ ๋งค๊ฐ๋ณ์๋ ์กฐ๊ฑด์ผ๋ก ์ฌ์ฉ๋ ๋์ด๋ฉฐ, ๋ณ์ ์ด๋ฆ์ด ๋ณ์์ ์๋ฏธ๋ฅผ ์ค๋ช ํ๋ ์ฃผ์์ฒ๋ผ ์์ฉํ ์ ์์ต๋๋ค. ํจ์์์ ๋ฐํํ๋ ๊ฒฝ์ฐ ํจ์ ์ด๋ฆ์ด ๊ฒฐ๊ณผ๊ฐ ๋ฌด์์ธ์ง ์๋ ค์ฃผ๋ฏ๋ก ๋ณ์ ์ด๋ฆ์ด ๋ถํ์ํฉ๋๋ค.
Original comment in English
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const unsubscribe = onSnapshot(targetUserDocRef, (docSnapshot) => { | |
| if (docSnapshot.exists()) { | |
| const data = docSnapshot.data() | |
| onUpdate( | |
| { | |
| followers: data.followers || [], | |
| following: data.followings || [], | |
| }, | |
| data.followers?.includes(currentUID) || false | |
| ) | |
| } | |
| }) | |
| return unsubscribe | |
| return onSnapshot(targetUserDocRef, (docSnapshot) => { | |
| if (docSnapshot.exists()) { | |
| const data = docSnapshot.data() | |
| onUpdate( | |
| { | |
| followers: data.followers || [], | |
| following: data.followings || [], | |
| }, | |
| data.followers?.includes(currentUID) || false | |
| ) | |
| } | |
| }); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
There was a problem hiding this comment.
๊ณ ์ํ์ จ์ต๋๋ค.
์ ๋ฐ์ ์ผ๋ก ํฐ ๋ฌธ์ ์์ด ๊น๋ํ๊ฒ ์ ์์ฑํ์๊ณ ๊ตฌํํ์ฌ ์ฃผ์
จ์ต๋๋ค.
์ผ๋ถ ๋ก๋ฉ์ด ์์ด ์์ฒญ์ด ๊ฐ๋์ง ํ์ธ์ด ์ด๋ ค์ด ๋ถ๋ถ์ด๋
๋ก์ง์ด ์ ๋งคํ ๋ถ๋ถ ๊ฐ์ ๊ธฐ๋ฅ์ธ๋ฐ ๊ตฌํ ๋ฐฉ์์ด ์ผ๊ด ๋์ง ์์ ๋ถ๋ถ
๋ฑ ์ ๋ํ ์ฝ๋ฉํธ๋ฅผ ๋ฌ์์ต๋๋ค. ์ค๋ณต๋๋ ๋ถ๋ถ์ ์๋ตํ์์ต๋๋ค.
๋ฐฑ์๋๊ฐ ์๋ค๋ณด๋ ์์ฒญ ์ฒ๋ฆฌ ์๊ฐ์ด ์ค๋ ๊ฑธ๋ฆฌ๋ ๋ถ๋ถ์ ๋ํ ๋ฆฌ๋ทฐ๋ ์งํํ์ง ์์์ต๋๋ค.
์ผ๋ถ ๊นจ์ง๋ ํ๋ฉด๋ ์๋ ๊ฒ ๊ฐ์ต๋๋ค.
๊ณตํต๋ ๊ธฐ๋ฅ์ ๋ํ ๋ถ๋ฆฌ, ๊ตฌํ ๋ฐฉ์ ํต์ผ, ์ ํํ ๋ช
์นญ ๋ฑ
์กฐ๊ธ ๋ ๋ค๋ฌ์ด ์ฃผ์๋ฉด ์ ๋ง ์ข์ ํ๋ก์ ํธ๊ฐ ๋ ๊ฒ ๊ฐ์ต๋๋ค.
src/styles/GlobalStyles.tsx
Outdated
| @@ -0,0 +1,57 @@ | |||
| import { Global, css } from '@emotion/react' | |||
| import 'normalize.css' | |||
| import { colors } from '@/constants/color' | |||
There was a problem hiding this comment.
์๋ฏธ์ ์ผ๋ก css variable๋ก ํ์ฉ๋์ด constants ํด๋ ๋ณด๋ค๋ ์คํ์ผ์ชฝ์ ์๋ ๊ฒ์ด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
font ๋์ผ
src/styles/GlobalStyles.tsx
Outdated
| const GlobalStyles = () => ( | ||
| <Global | ||
| styles={css` | ||
| @import url('https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/variable/pretendardvariable.min.css'); |
There was a problem hiding this comment.
https://careerly.co.kr/qnas/8085
orioncactus/pretendard#177
๊ฐ๋ณ ํฐํธ ์ฌ์ฉ ์ ๊ฐํน OS๋ ๋ธ๋ผ์ฐ์ ๋ฒ์ ๋ฑ์ ๋ฐ๋ผ ๋ฌธ์ ๊ฐ ๋ ์ ์์ด
์ฐธ๊ณ ๋ก ์์๋์๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
There was a problem hiding this comment.
ํด๋น ๊ธ์ ํ์ธํ ํ, ์ ํฌ ํ๋ก์ ํธ์์ ๋ฌธ์ ๊ฐ ์๋์ง ๊ฒํ ํด๋ณด์์ต๋๋ค. ํ๋ฆฌํ ๋ค๋ ํฐํธ ๋ฌธ์ ๋ ์ฃผ๋ก Flutter ๊ฐ๋ฐ ์ ๋ฐ์ํ๋ ๊ฒ์ผ๋ก ๋ณด์์ผ๋, ์ ํฌ๊ฐ ๋ฐฐํฌํ ์ฌ์ดํธ๋ฅผ ๋ชจ๋ฐ์ผ ํ๊ฒฝ์์ ํ ์คํธํ ๊ฒฐ๊ณผ, ํฐํธ๊ฐ ๊นจ์ง๋ ํ์์ ๋ฐ๊ฒฌ๋์ง ์์์ต๋๋ค. ๋ฐ๋ผ์ ๊ฐ๋ณ ํฐํธ๋ ๊ทธ๋๋ก ์ ์ฉํ๊ธฐ๋ก ๊ฒฐ์ ํ์ต๋๋ค. ์๋ ค์ฃผ์ ์ ๊ฐ์ฌํฉ๋๋ค!
src/App.tsx
Outdated
| }, | ||
| ]) | ||
|
|
||
| const App = () => <RouterProvider router={router} /> |
There was a problem hiding this comment.
์์ฑ ์คํ์ผ์ ์ฐจ์ด์ฌ์ ์ฐธ๊ณ ๋ง ํด์๋ฉด ๋ฉ๋๋ค.
main.tsx์ ์๋ query, style๋ฑ ์ App.tsx์ ์์ฑํ๊ณ
router๋ฅผ ๋ณ๋ routerํ์ผ ๋ด์ ์์ฑํด๋ ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
router์์ ์ฒ๋ฆฌํ๋ ๋ด์ฉ๋ ์๊ธธ ์ ์๊ธฐ ๋๋ฌธ์ ๋ถ๋ฆฌ๋๋ ๊ฒ์ด
๊ด๋ฆฌํ๊ธฐ ์ฉ์ดํ ๊ฒ ๊ฐ์ต๋๋ค.
src/App.tsx
Outdated
| }, | ||
|
|
||
| { path: PATH.EDITPW, element: <EditPwPage /> }, | ||
| { |
There was a problem hiding this comment.
๋ก๊ทธ์ธ ํ ํด๋น path๋ก ๋ค์ด์จ ํ์๋ ๋ค์ ์คํ๋์ง ์์ต๋๋ค.
RootLayout์์๋ useLocation ๋๋ฌธ์ checkAuth๊ฐ ๊ณ์ ์ฒดํฌ๋์ด
PrivateRoute์ checkAuth๊ณผ ๊ฒฐ๊ณผ์ ์ผ๋ก ๋ค๋ฅธ ๊ฐ์ ๊ฐ์ง๊ฒ๋๋ ๋ฌธ์ ๊ฐ ์์ต๋๋ค.
| } | ||
|
|
||
| return ( | ||
| <Form title={'ํ์๊ฐ์ '} getDataForm={handleSignupAndLogin} firebaseError={firebaseError} /> |
There was a problem hiding this comment.
title์ด ์๋ button์ text๋ก ๋ค์ด๊ฐ๋๋ค.
prop ๋ค์ด๋ฐ๋ ์ ๊ฒฝ์ฐ๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
src/pages/ProfilePage.tsx
Outdated
| const filterBtns = [ | ||
| { | ||
| label: '์ ์ฒด', | ||
| value: 'all' as filterPlaylist, | ||
| }, | ||
| { | ||
| label: '๊ณต๊ฐ', | ||
| value: 'public' as filterPlaylist, | ||
| }, | ||
| { | ||
| label: '๋น๊ณต๊ฐ', | ||
| value: 'private' as filterPlaylist, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
์ปดํฌ๋ํธ ๋ฐ์ ์์ฑํ์ฌ๋ ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
src/pages/ProfilePage.tsx
Outdated
| {filteredLists.length > 4 && !isOpen && ( | ||
| <Button variant="text" onClick={handleToggleOpen}> | ||
| ํ๋ ์ด๋ฆฌ์คํธ ๋ชจ๋ ๋ณด๊ธฐ | ||
| </Button> | ||
| )} | ||
| {filteredLists.length > 4 && isOpen && ( | ||
| <Button variant="text" onClick={handleToggleOpen}> | ||
| ํ๋ ์ด๋ฆฌ์คํธ ๋ชจ๋ ์ ๊ธฐ | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
text๋ฅผ isOpen์ผ๋ก ๋ถ๊ธฐํ๋ฉด ๋ ๊ฒ ๊ฐ์ต๋๋ค.
src/pages/ProfilePage.tsx
Outdated
| import NPProfile from '@/assets/np_logo.svg' | ||
| import { useUserData } from '@/hooks/useUserData' | ||
| import { usePlaylistData } from '@/hooks/usePlaylistData' | ||
| import { filterPlaylist, PlaylistBaseProps } from '@/types/playlistType' |
There was a problem hiding this comment.
import type ์ผ๋ก ๊ด๋ฆฌ๋๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
| setFilter(newFilter) | ||
| } | ||
|
|
||
| const filteredMap: Record<filterPlaylist, (playlistData: PlaylistBaseProps) => boolean> = { |
There was a problem hiding this comment.
all ์ ๋ํ ํ์
์ด ๋ง์ง ์๋ ๊ฒ ๊ฐ์ต๋๋ค.
์ถ๊ฐ๋ก eslint์ ๋ฃฐ์
'no-unused-vars': 'warn' ๋ฅผ
'@typescript-eslint/no-unused-vars': 'error' ๋ก ๋ณ๊ฒฝํด์ผ ์๋ฌ๊ฐ ์ ๋ฐ ๊ฒ ๊ฐ์ต๋๋ค.
| onChange={(e) => setCurrentTag(e.target.value)} | ||
| onKeyDown={handleAddTag} | ||
| /> | ||
| <TagList tags={tags} onTagDelete={handleDeleteTag} /> |
There was a problem hiding this comment.
์ํฐ๋ฅผ ์ณ์ผ tag๊ฐ ์๊ธด๋ค๋ ๊ฒ์ ์๊ธฐ ์ด๋ ค์ด ๊ฒ ๊ฐ์ต๋๋ค.
| export default MusicItem | ||
|
|
||
| export const Container = styled.div<{ variant: 'profilePL' | 'createPL'; isPrivate?: boolean }>` | ||
| .item-video { |
There was a problem hiding this comment.
container ๋ด๋ถ์ class๋ก ์์ฑํ์๋ฉด
class ๋ช
๋ง๋ค ์ด๋ค ์์ฑ์ด ์ ์ฉ๋๋์ง ์ถ์ธกํ๊ธฐ ์ด๋ ค์ธ ๊ฒ ๊ฐ์ต๋๋ค.
Refactor/playlist
๐จ ๋ผ์ฐํฐ๊ณผ layout ๋ฑ ํ๋ก์ ํธ ๊ณตํต๋ถ๋ถ ์ฝ๋๋ฆฌ๋ทฐ์์
[refactor] ํ๋กํ ํ์ด์ง pr ๋ฆฌ๋ทฐ ์ฐธ๊ณ ํ์ฌ ์์
๐ฅ์ฝ๋๋ฆฌ๋ทฐ ์๋ถํ๋๋ฆฝ๋๋ค๐ฅ
Sourcery์ ์ํ ์์ฝ
์ ํ๋ฆฌ์ผ์ด์ ์ ํฌ๊ด์ ์ธ ๊ฐํธ์ ๊ตฌํํ์ฌ ์ฌ์ฉ์ ์ธ์ฆ, ํ๋ ์ด๋ฆฌ์คํธ ๊ด๋ฆฌ ๋ฐ ๊ฒ์ ๊ธฐ๋ฅ๊ณผ ๊ฐ์ ์๋ก์ด ๊ธฐ๋ฅ์ ๋์ ํฉ๋๋ค. ์ต์ React ๋ฐ TypeScript ๊ดํ์ ์ฌ์ฉํ์ฌ ์ฝ๋๋ฒ ์ด์ค๋ฅผ ๋ฆฌํฉํฐ๋งํ๊ณ , ๋น๋ ๋ฐ CI ํ๋ก์ธ์ค๋ฅผ ๊ฐ์ ํ๋ฉฐ, Firebase Hosting์ ์ฌ์ฉํ์ฌ ๋ฐฐํฌ๋ฅผ ์ค์ ํฉ๋๋ค.
์๋ก์ด ๊ธฐ๋ฅ:
๊ฐ์ ์ฌํญ:
๋น๋:
CI:
๋ฐฐํฌ:
๋ฌธ์ํ:
ํ ์คํธ:
์ก์ผ:
Original summary in English
Summary by Sourcery
Implement a comprehensive overhaul of the application, introducing new features such as user authentication, playlist management, and search functionality. Refactor the codebase to use modern React and TypeScript practices, enhance the build and CI processes, and set up deployment with Firebase Hosting.
New Features:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Tests:
Chores: