Conversation
Summary of ChangesHello @yeeun426, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 관리자가 여러 위치의 배너를 효율적으로 관리할 수 있도록 통합 배너 관리 시스템을 도입합니다. 관리자 페이지에서 배너를 생성, 편집, 삭제할 수 있는 완전한 기능을 제공하며, 사용자에게 노출되는 기존 배너 섹션들은 이 새로운 통합 시스템을 통해 데이터를 가져오도록 업데이트되었습니다. 이를 통해 배너 관리 프로세스가 간소화되고 일관성이 강화됩니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 PR reorganizes the existing distributed banner management system into a 'unified banner' system, introducing new API hooks, components, and pages for administrators and user-facing sections. A critical Stored Cross-Site Scripting (XSS) vulnerability was identified, as the landingUrl input lacks validation on the admin side and is directly rendered into href attributes on the user side, potentially allowing malicious scripts via javascript: URIs. It is recommended to implement strict URL protocol validation (allowing only http: and https:) and sanitize these URLs before rendering. Additionally, several code-related improvements are needed, primarily concerning API design, efficiency, and potential UI logic bugs. These include ensuring unique IDs in API responses for list rendering, optimizing mutation hooks to prevent unnecessary requests, and fixing bugs related to image processing and data display.
| export const useToggleCommonBannerVisibility = ({ | ||
| successCallback, | ||
| errorCallback, | ||
| }: { | ||
| successCallback?: () => void; | ||
| errorCallback?: (error: Error) => void; | ||
| } = {}) => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation({ | ||
| mutationFn: async ({ | ||
| commonBannerId, | ||
| isVisible, | ||
| }: { | ||
| commonBannerId: number; | ||
| isVisible: boolean; | ||
| }) => { | ||
| const detailRes = await axios(`/admin/common-banner/${commonBannerId}`); | ||
| const detail = detailRes.data.data as CommonBannerDetailResponse; | ||
|
|
||
| const res = await axios.patch(`/admin/common-banner/${commonBannerId}`, { | ||
| commonBannerInfo: { | ||
| title: detail.commonBanner.title, | ||
| landingUrl: detail.commonBanner.landingUrl, | ||
| startDate: detail.commonBanner.startDate, | ||
| endDate: detail.commonBanner.endDate, | ||
| isVisible, | ||
| }, | ||
| commonBannerDetailInfoList: detail.commonBannerDetailList, | ||
| }); | ||
| return res.data; | ||
| }, |
There was a problem hiding this comment.
useToggleCommonBannerVisibility 훅의 구현이 비효율적입니다. isVisible 상태를 토글하기 위해 PATCH 요청 전에 GET 요청으로 배너의 전체 상세 정보를 가져오고 있습니다. 이는 불필요한 네트워크 요청을 발생시키고, GET과 PATCH 사이에 다른 사용자가 데이터를 수정할 경우 발생할 수 있는 경쟁 상태(race condition)에 취약합니다.
백엔드 API가 isVisible 필드만 받아서 부분 업데이트를 지원하도록 수정하고, 클라이언트에서는 GET 요청 없이 PATCH 요청만 보내도록 하는 것이 좋습니다. 이는 스타일 가이드의 '숨겨진 로직 드러내기' 원칙에도 부합합니다.
추천 패턴:
mutationFn: async ({ commonBannerId, isVisible }: { commonBannerId: number; isVisible: boolean; }) => {
const res = await axios.patch(`/admin/common-banner/${commonBannerId}`, {
commonBannerInfo: { isVisible },
});
return res.data;
},References
- 함수 시그니처가 암시하는 동작만 수행해야 하며, 숨겨진 부수 효과(여기서는 GET 요청)를 피해야 합니다. 현재 구현은 이름과 달리 데이터 조회 후 수정을 수행하여 예측 가능성을 저해합니다. (link)
| // PC/MOBILE 아이템을 같은 title+landingUrl 기준으로 그룹핑 | ||
| const groupMap = new Map<string, CommonBannerUserItem>(); | ||
| for (const item of parsed.commonBannerList) { | ||
| const key = `${item.title ?? ''}::${item.landingUrl ?? ''}`; | ||
| if (!groupMap.has(key)) { | ||
| groupMap.set(key, { | ||
| title: item.title, | ||
| landingUrl: item.landingUrl, | ||
| imgUrl: null, | ||
| mobileImgUrl: null, | ||
| }); | ||
| } | ||
| const group = groupMap.get(key)!; | ||
| if (item.agentType === 'PC') { | ||
| group.imgUrl = item.fileUrl; | ||
| } else { | ||
| group.mobileImgUrl = item.fileUrl; | ||
| } | ||
| } | ||
|
|
||
| return Array.from(groupMap.values()); |
There was a problem hiding this comment.
사용자단 배너를 가져오는 useGetCommonBannerListForUser 훅에서 사용하는 /common-banner API의 응답에 각 배너를 식별할 수 있는 고유 ID(commonBannerId)가 누락되어 있습니다. 이로 인해 여러 컴포넌트에서 React key로 index를 사용하는 문제가 발생하고 있습니다.
백엔드 API 응답에 commonBannerId를 포함하도록 수정하고, 이를 그룹핑과 React key 값으로 사용하는 것이 안정성과 성능 면에서 중요합니다. index를 key로 사용하면 리스트가 변경될 때 예기치 않은 동작이 발생할 수 있습니다.
CommonBannerUserItem 타입에 id를 추가하고, 그룹핑 로직에서 이 ID를 전달하도록 수정하는 것을 제안합니다.
| type="button" | ||
| className="absolute right-2 top-2 rounded-full bg-black/50 px-2 py-0.5 text-xs text-white" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onChange(null); | ||
| }} | ||
| > | ||
| ✕ | ||
| </button> |
There was a problem hiding this comment.
이미지 미리보기의 '✕' 버튼이 수정 모드에서 제대로 동작하지 않습니다. 이 버튼은 새로 업로드할 File 객체만 null로 만들고, 기존 이미지의 fileId는 그대로 유지합니다. 이로 인해 사용자가 기존 이미지를 삭제할 수 없습니다.
이미지를 삭제하려면, '✕' 버튼 클릭 시 해당 이미지의 fileId (예: homePcFileId)와 fileUrl도 null로 설정하도록 onChange 핸들러 로직을 수정해야 합니다.
| valueGetter: (_, row) => | ||
| `${dayjs(row.startDate).format('YYYY-MM-DD')} ~ ${dayjs( | ||
| row.endDate, | ||
| ).format('YYYY-MM-DD')}`, |
There was a problem hiding this comment.
DataGrid의 date 컬럼 valueGetter에서 startDate 또는 endDate가 null일 경우 dayjs(null)이 호출되어 "Invalid Date"가 표시되거나 오류가 발생할 수 있습니다. 스키마에 따르면 이 값들은 nullable하므로, 렌더링 전에 null 체크를 추가하여 안정성을 높여야 합니다.
valueGetter: (_, row) =>
`${row.startDate ? dayjs(row.startDate).format('YYYY-MM-DD') : '미지정'} ~ ${row.endDate ? dayjs(row.endDate).format('YYYY-MM-DD') : '미지정'}`,
| <SwiperSlide key={index}> | ||
| <a | ||
| href={banner.link || '#'} | ||
| href={banner.landingUrl || '#'} |
There was a problem hiding this comment.
The landingUrl from the banner data is rendered directly into the href attribute of an <a> tag without proper validation or sanitization. This allows for a Stored Cross-Site Scripting (XSS) vulnerability if an administrator provides a malicious URL using the javascript: pseudo-protocol (e.g., javascript:alert(document.cookie)). When a user clicks the banner, the script will execute in the context of their session.
| <SwiperSlide key={index}> | ||
| <HybridLink | ||
| href={banner.link || '#'} | ||
| href={banner.landingUrl || '#'} |
There was a problem hiding this comment.
The landingUrl from the banner data is rendered directly into the href attribute of a HybridLink component without proper validation or sanitization. This allows for a Stored Cross-Site Scripting (XSS) vulnerability if an administrator provides a malicious URL using the javascript: pseudo-protocol (e.g., javascript:alert(document.cookie)). When a user clicks the banner, the script will execute in the context of their session.
| <HybridLink | ||
| href={banner.link} | ||
| key={banner.id} | ||
| href={banner.landingUrl || '#'} |
There was a problem hiding this comment.
The landingUrl from the banner data is rendered directly into the href attribute of a HybridLink component without proper validation or sanitization. This allows for a Stored Cross-Site Scripting (XSS) vulnerability if an administrator provides a malicious URL using the javascript: pseudo-protocol (e.g., javascript:alert(document.cookie)). When a user clicks the banner, the script will execute in the context of their session.
| export const getBnnerListForAdminQueryKey = (type: bannerType) => [ | ||
| 'banner', | ||
| 'admin', | ||
| type, | ||
| ]; |
There was a problem hiding this comment.
getBnnerListForAdminQueryKey 함수 이름에 오타가 있습니다. getBannerListForAdminQueryKey로 수정하는 것이 좋겠습니다.
| export const getBnnerListForAdminQueryKey = (type: bannerType) => [ | |
| 'banner', | |
| 'admin', | |
| type, | |
| ]; | |
| export const getBannerListForAdminQueryKey = (type: bannerType) => [ | |
| 'banner', | |
| 'admin', | |
| type, | |
| ]; |
| const toDatetimeLocal = (iso: string) => { | ||
| const date = new Date(iso); | ||
| const pad = (n: number) => String(n).padStart(2, '0'); | ||
| return `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())}T${pad(date.getHours())}:${pad(date.getMinutes())}`; | ||
| }; |
There was a problem hiding this comment.
toDatetimeLocal 함수에서 new Date(iso)를 사용하여 날짜를 파싱하고 있습니다. 이 방식은 브라우저의 시간대에 따라 결과가 달라질 수 있어 잠재적인 버그를 유발할 수 있습니다. 프로젝트에 이미 dayjs가 도입되어 있으니, 일관성과 안정성을 위해 dayjs를 사용하여 날짜 형식을 변환하는 것이 좋습니다.
const toDatetimeLocal = (iso: string) => {
if (!iso) return '';
return dayjs(iso).format('YYYY-MM-DDTHH:mm');
};
| onChange: (file: File | null) => void; | ||
| }) => { | ||
| const inputRef = useRef<HTMLInputElement>(null); | ||
| const preview = file ? URL.createObjectURL(file) : previewUrl; |
There was a problem hiding this comment.
ImageUploadBox 컴포넌트에서 URL.createObjectURL(file)을 사용하여 이미지 미리보기를 생성하고 있습니다. 이렇게 생성된 URL은 수동으로 해제해주지 않으면 메모리 누수를 유발할 수 있습니다. useEffect의 cleanup 함수 내에서 URL.revokeObjectURL()을 호출하여 컴포넌트가 언마운트되거나 파일이 변경될 때 메모리를 해제하는 것이 좋습니다.
1 participant
연관 작업