Conversation
egg-zz
added
⛳️ team-frontend
🌊 squad-__init__
preview
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive notice management system for both client and admin interfaces, featuring notice listing, detail views, and a nested commenting system. Key administrative functionalities include creating, editing, importing, and deleting notices. Feedback highlights a critical security vulnerability involving unsanitized HTML rendering via dangerouslySetInnerHTML and a significant performance bottleneck in the admin notice table caused by an N+1 query pattern. Further recommendations focus on enhancing code maintainability by replacing hardcoded styles and colors with theme variables, optimizing sorting logic in useMemo hooks, and removing unused component props.
| <div | ||
| className="prose mt-4 max-w-none whitespace-pre-wrap text-[16px]" | ||
| dangerouslySetInnerHTML={{ __html: notice.content }} | ||
| /> |
There was a problem hiding this comment.
Using dangerouslySetInnerHTML can expose your application to Cross-Site Scripting (XSS) attacks if the content (notice.content) is not properly sanitized. Even if the content comes from a trusted source like an instructor, it's a security best practice to sanitize it. Please sanitize the HTML content before rendering it, for example by using a library like dompurify on the client-side, or ensuring it's sanitized on the backend before being saved to the database.
| const { data: notices = [], isLoading } = useQuery({ | ||
| queryKey: ['adminCourseNotices', Number(groupId)], | ||
| queryFn: async () => { | ||
| const [fixedResponse, normalResponse] = await Promise.all([ | ||
| safeFetcherWithAuth | ||
| .get(`course/${groupId}/notice/all`, { | ||
| searchParams: { | ||
| take: '100', | ||
| fixed: 'true', | ||
| readFilter: 'all', | ||
| order: 'createTime-desc' | ||
| } | ||
| }) | ||
| .json<CourseNoticeListResponse>(), | ||
| safeFetcherWithAuth | ||
| .get(`course/${groupId}/notice/all`, { | ||
| searchParams: { | ||
| take: '100', | ||
| fixed: 'false', | ||
| readFilter: 'all', | ||
| order: 'createTime-desc' | ||
| } | ||
| }) | ||
| .json<CourseNoticeListResponse>() | ||
| ]) | ||
|
|
||
| const combined = [...fixedResponse.data, ...normalResponse.data] | ||
|
|
||
| const detailResults = await Promise.all( | ||
| combined.map(async (notice) => { | ||
| const detail = await safeFetcherWithAuth | ||
| .get(`course/notice/${notice.id}`) | ||
| .json<{ | ||
| current: { | ||
| title: string | ||
| content: string | ||
| isPublic: boolean | ||
| createTime: string | ||
| updateTime: string | ||
| } | ||
| }>() | ||
|
|
||
| return { | ||
| ...notice, | ||
| content: detail.current.content, | ||
| isPublic: detail.current.isPublic, | ||
| createTime: detail.current.createTime, | ||
| updateTime: detail.current.updateTime | ||
| } | ||
| }) | ||
| ) | ||
|
|
||
| return detailResults | ||
| } | ||
| }) |
There was a problem hiding this comment.
This queryFn implementation introduces a significant performance bottleneck due to the N+1 query problem. It first fetches lists of notices and then, within a Promise.all, makes an additional API call for each notice to get its details. For a list of N notices, this will result in at least N+1 network requests.
To fix this, the backend API should be updated to provide all the necessary notice data in the initial list-fetching endpoint, avoiding the need for subsequent individual requests.
There was a problem hiding this comment.
N+1 문제 해결은 몹시중요! 이건 무조건 고쳐야함!!
제미나이 봇이 말이 많긴한데 Critical이나 High Priority에서 시뻘겋게 뜬거 위주로 먼저 보면 좋아요. 가끔 Medum Priority는 이상한거 괜히 억까할때도 있긴한데 코파일럿 보다는 훨씬 잘해서 참고해보세요!
| row.original.isFixed && | ||
| "before:bg-primary before:absolute before:left-[-16px] before:top-[-18px] before:h-[57px] before:w-[3px] before:rounded-full before:content-['']" |
There was a problem hiding this comment.
These before: pseudo-element styles are complex and contain magic numbers (e.g., left-[-16px], top-[-18px]). This makes the code hard to read and maintain. Consider extracting this into a separate, well-named utility class in your Tailwind configuration for better readability and reusability.
| 'relative w-full text-center text-sm text-[#666666]', | ||
| row.original.isFixed && | ||
| "before:bg-primary before:absolute before:left-[-16px] before:top-[-18px] before:h-[57px] before:w-[3px] before:rounded-full before:content-['']" | ||
| )} | ||
| > | ||
| {row.original.no} | ||
| </div> | ||
| ), | ||
| enableSorting: false | ||
| }, | ||
| { | ||
| accessorKey: 'title', | ||
| header: 'Title', | ||
| cell: ({ row }) => ( | ||
| <div className="flex items-center justify-start gap-2 overflow-hidden text-sm text-black"> | ||
| <span className="line-clamp-1">{row.original.title}</span> | ||
| {!row.original.isRead && ( | ||
| <span className="bg-primary h-[6px] w-[6px] shrink-0 rounded-full" /> | ||
| )} | ||
| </div> | ||
| ), | ||
| enableSorting: false | ||
| }, | ||
| { | ||
| accessorKey: 'date', | ||
| header: 'Date', | ||
| cell: ({ row }) => ( | ||
| <span className="text-sm text-[#666666]"> | ||
| {row.original.date | ||
| ? dateFormatter(row.original.date, 'YY-MM-DD HH:mm') | ||
| : '-'} | ||
| </span> | ||
| ), | ||
| enableSorting: false | ||
| }, | ||
| { | ||
| accessorKey: 'createdBy', | ||
| header: 'Writer', | ||
| cell: ({ row }) => ( | ||
| <span className="text-sm text-[#666666]">{row.original.createdBy}</span> |
There was a problem hiding this comment.
Throughout this file, the hardcoded color text-[#666666] is used (lines 23, 50, 62). It's recommended to use theme variables from your design system (e.g., text-neutral-500 or similar) for better consistency and maintainability across the application.
| const tableData: CourseNoticeRow[] = useMemo(() => { | ||
| const filteredNotices = | ||
| filterType === 'unread' | ||
| ? notices.filter((notice) => !notice.isRead) | ||
| : notices | ||
|
|
||
| const noMap = new Map( | ||
| [...filteredNotices] | ||
| .sort((a, b) => getTime(a) - getTime(b)) | ||
| .map((notice, index) => [notice.id, index + 1]) | ||
| ) | ||
|
|
||
| return [...filteredNotices] | ||
| .sort((a, b) => { | ||
| if (a.isFixed !== b.isFixed) { | ||
| return a.isFixed ? -1 : 1 | ||
| } | ||
| return orderType === 'oldest' | ||
| ? getTime(a) - getTime(b) | ||
| : getTime(b) - getTime(a) | ||
| }) | ||
| .map((notice) => ({ | ||
| id: notice.id, | ||
| no: String(noMap.get(notice.id) ?? 0).padStart(2, '0'), | ||
| title: notice.title, | ||
| createdBy: notice.createdBy ?? 'Unknown', | ||
| date: notice.createTime ?? notice.updateTime ?? '', | ||
| isRead: notice.isRead, | ||
| isFixed: notice.isFixed | ||
| })) | ||
| }, [notices, filterType, orderType]) |
There was a problem hiding this comment.
The logic inside this useMemo hook for preparing tableData could be more efficient. It currently involves multiple sorts and iterations over the notices array:
- The
filteredNoticesare sorted to createnoMap. filteredNoticesare sorted again to apply the final display order.
This can be optimized by reducing the number of sorts. For instance, you could perform the final sort first, and then map over the sorted array to generate the no value based on the index.
| onEditStart: (comment: CourseNoticeCommentItem) => void | ||
| onDelete: (commentId: number) => void | ||
| renderEditEditor: () => React.ReactNode | ||
| renderReplyEditor: () => React.ReactNode |
There was a problem hiding this comment.
The renderReplyEditor prop is defined but it's not used within this component. It should either be used or removed to avoid confusion and keep the component's API clean.
| @@ -0,0 +1,130 @@ | |||
| 'use client' | |||
There was a problem hiding this comment.
This component contains several hardcoded color values (e.g., border-[#E5E5E5], placeholder:text-[#C4C4C4], text-blue-500). It's best practice to use theme variables from your Tailwind configuration to ensure consistency and make future theme updates easier.
| secret: boolean | ||
| setSecret: (value: boolean) => void | ||
| onSubmit: () => void | ||
| onCancel?: () => void |
There was a problem hiding this comment.
The onCancel prop is defined but never used within the component. Please either implement its functionality or remove it to simplify the component's interface.
| isCommentsLoading: boolean | ||
| groupedComments: CourseNoticeCommentGroup[] | ||
| profileUsername?: string | ||
| isInstructor?: boolean |
There was a problem hiding this comment.
The isInstructor prop is defined but not used within the component. It should be removed to keep the component's API clean.
| disabled={ | ||
| !title.trim() || | ||
| !mainText.trim() || | ||
| charCount > 400 || |
There was a problem hiding this comment.
The character limit 400 is hardcoded here and also displayed to the user on line 207. It's better to define this as a constant at the top of the file or in a shared constants file. This improves maintainability and reduces the risk of inconsistencies if the value needs to be changed.
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
There was a problem hiding this comment.
create 안된다했는데 어떤 상황인지 좀만더 구체적으로 알려주세요...! 백엔드 문제인건가유?
There was a problem hiding this comment.
지금 prisma가 업데이트 되면서 설정이 달라졌는데, 그래서 tsconfig에서 조금 문제가 생긴거같아요 이 pr뿐만 아니라 다른 곳에서도 문제 될거같은데 왜 아무도 모르지...? 한번 알아볼게요
There was a problem hiding this comment.
일단 백엔드 상관없는 프론트엔드 에러부터 수정할 수 있는거 먼저 하기! gemini가 리뷰해준거 크리티컬부터! 그리고 에러난다는거 에러문이나 스크린샷 좀 공유해주세요
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
|
✅ Syncing Preview App Succeeded Application: |
2 participants
Description
course의 notice 페이지를 만듭니다.
조금 덜 해서 미완입니다,,,
Additional context
Before submitting the PR, please make sure you do the following
fixes #123).closes TAS-2604