여행 그룹 생성#10
Conversation
Summary of ChangesHello @SongMinQQ, 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! 이 풀 리퀘스트는 그룹 생성 기능을 개선하기 위해 UI 컴포넌트의 책임 분리, API 로직의 명확화, 그리고 보안 강화를 목표로 합니다. 사용자 경험을 향상시키고 코드 유지보수성을 높이기 위해 폼 처리 방식과 파일 업로드 메커니즘을 재정비했으며, CSRF 토큰 처리를 도입하여 애플리케이션의 보안 수준을 한 단계 끌어올렸습니다. 또한, 새로운 E2E 테스트를 통해 이러한 변경 사항들이 의도대로 작동하는지 검증합니다. Highlights
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 pull request successfully refactors the group creation feature, enhancing responsibility separation, maintainability, and testability through a clear component structure and distinct API responsibilities. It also introduces a CSRF protection mechanism. However, a critical security vulnerability has been identified: the CSRF token storage in shared/lib/csrf-token.ts uses a global cache variable, which can cause user-specific tokens to leak between requests during server-side rendering (SSR), leading to session cross-pollination. This implementation should be restricted to the client-side. Please also review the minor improvements suggested in the specific code comments.
| const groupData: GroupGenerateData = { | ||
| name: trimmedName, | ||
| description: trimmedDescription, | ||
| memeberLimit: requiredForm.maxMembers, |
There was a problem hiding this comment.
GroupGenerateData 인터페이스에서 정의된 memeberLimit에 오타가 있습니다. memberLimit로 수정하는 것이 좋습니다. 이 오타는 shared/api/group/types.ts에서 비롯된 것으로 보이며, 해당 파일에서 수정하면 연관된 다른 파일들의 문제도 해결될 것입니다.
| memeberLimit: requiredForm.maxMembers, | |
| memberLimit: requiredForm.maxMembers, |
| const requestBody: GroupGenerateRequest = { | ||
| name: groupData.name, | ||
| description: groupData.description, | ||
| memberLimit: groupData.memeberLimit, |
There was a problem hiding this comment.
GroupGenerateData 인터페이스에서 정의된 memeberLimit에 오타가 있습니다. memberLimit로 수정하는 것이 좋습니다. 이 오타는 shared/api/group/types.ts에서 비롯된 것으로 보이며, 해당 파일에서 수정하면 연관된 다른 파일들의 문제도 해결될 것입니다.
| memberLimit: groupData.memeberLimit, | |
| memberLimit: groupData.memberLimit, |
| memeberLimit: number; | ||
| groupKind: string; | ||
| thumbNailUrl: string; |
There was a problem hiding this comment.
GroupGenerateData 인터페이스에 오타가 있습니다.
memeberLimit는memberLimit로thumbNailUrl은thumbnailUrl로
수정하는 것이 일관성 있고 명확해 보입니다.
이 변경은 아래 파일들에도 연쇄적으로 수정이 필요합니다.
features/group/ui/GroupGenerateSubmitBtn.tsxshared/api/group/generateGroup.ts(여기서는GroupGenerateRequest인터페이스도 함께 수정해야 합니다)
| memeberLimit: number; | |
| groupKind: string; | |
| thumbNailUrl: string; | |
| memberLimit: number; | |
| groupKind: string; | |
| thumbnailUrl: string; |
| export const setCsrfToken = (token: string | null | undefined): void => { | ||
| const normalizedToken = normalizeToken(token); | ||
| csrfTokenCache = normalizedToken; | ||
|
|
||
| if (typeof window === "undefined") { | ||
| return; | ||
| } | ||
|
|
||
| if (normalizedToken) { | ||
| window.sessionStorage.setItem(AUTH_STORAGE_KEYS.CSRF_TOKEN, normalizedToken); | ||
| return; | ||
| } | ||
|
|
||
| window.sessionStorage.removeItem(AUTH_STORAGE_KEYS.CSRF_TOKEN); | ||
| }; |
There was a problem hiding this comment.
The csrfTokenCache global variable is used to cache the CSRF token in memory. In a Next.js environment, modules are cached on the server across multiple requests. When setCsrfToken is called during a server-side request (e.g., in a Server Component or during SSR), it updates this global variable. Subsequent requests from different users handled by the same server process will then use the cached CSRF token from the previous user. This leads to CSRF token leakage between users and potential session cross-pollination, which can cause requests to fail or potentially bypass security checks if the token is leaked to the client.
To fix this, ensure that the in-memory cache is only used on the client-side by checking typeof window !== 'undefined' before setting or returning the cached value.
| export const setCsrfToken = (token: string | null | undefined): void => { | |
| const normalizedToken = normalizeToken(token); | |
| csrfTokenCache = normalizedToken; | |
| if (typeof window === "undefined") { | |
| return; | |
| } | |
| if (normalizedToken) { | |
| window.sessionStorage.setItem(AUTH_STORAGE_KEYS.CSRF_TOKEN, normalizedToken); | |
| return; | |
| } | |
| window.sessionStorage.removeItem(AUTH_STORAGE_KEYS.CSRF_TOKEN); | |
| }; | |
| export const setCsrfToken = (token: string | null | undefined): void => { | |
| const normalizedToken = normalizeToken(token); | |
| if (typeof window === "undefined") { | |
| return; | |
| } | |
| csrfTokenCache = normalizedToken; | |
| if (normalizedToken) { | |
| window.sessionStorage.setItem(AUTH_STORAGE_KEYS.CSRF_TOKEN, normalizedToken); | |
| return; | |
| } | |
| window.sessionStorage.removeItem(AUTH_STORAGE_KEYS.CSRF_TOKEN); | |
| }; |
There was a problem hiding this comment.
if (typeof window === "undefined") {
return;
}
현재 반환 전에 여기서 검사중인거 아니야?
| htmlFor="review-title" | ||
| label={"제목"} | ||
| currentLength={title.length} | ||
| maxLength={20} |
There was a problem hiding this comment.
maxLength가 20으로 하드코딩되어 있습니다. 그룹 생성 폼에서 하신 것처럼 이 값을 상수로 추출하여 관리하면 유지보수성이 향상될 것입니다. 예를 들어, shared/constants/review.ts 같은 파일에 REVIEW_TITLE_MAX_LENGTH 상수를 정의하고 사용할 수 있습니다.
| htmlFor="schedule-name" | ||
| label={"일정명"} | ||
| currentLength={title.length} | ||
| maxLength={20} |
There was a problem hiding this comment.
maxLength가 20으로 하드코딩되어 있습니다. 그룹 생성 폼에서 하신 것처럼 이 값을 상수로 추출하여 관리하면 유지보수성이 향상될 것입니다. 예를 들어, shared/constants/schedule.ts 같은 파일에 SCHEDULE_NAME_MAX_LENGTH 상수를 정의하고 사용할 수 있습니다.
| onGroupNameChange={(groupName) => | ||
| setRequiredForm((prev) => ({ ...prev, groupName })) | ||
| } | ||
| onDescriptionChange={(description) => | ||
| setRequiredForm((prev) => ({ ...prev, description })) | ||
| } | ||
| onMaxMembersChange={(maxMembers) => | ||
| setRequiredForm((prev) => ({ ...prev, maxMembers })) | ||
| } | ||
| onIsPublicChange={(isPublic) => | ||
| setRequiredForm((prev) => ({ ...prev, isPublic })) | ||
| } |
There was a problem hiding this comment.
on...Change 핸들러 함수들이 렌더링될 때마다 새로 생성되고 있습니다. 현재는 성능에 큰 영향이 없겠지만, 자식 컴포넌트가 React.memo 등으로 최적화될 경우 불필요한 리렌더링을 유발할 수 있습니다. useCallback을 사용하여 핸들러 함수를 메모이제이션하는 것을 고려해 보세요.
예시:
import { useCallback, ... } from 'react';
...
const onGroupNameChange = useCallback((groupName) => {
setRequiredForm((prev) => ({ ...prev, groupName }));
}, []);
...
<GroupGenerateRequiredFields
...
onGroupNameChange={onGroupNameChange}
...
/>| <button | ||
| type="button" | ||
| onClick={() => onIsPublicChange(true)} | ||
| className={`text-sm font-semibold transition-colors ${ | ||
| form.isPublic ? "text-foreground" : "text-muted-foreground" | ||
| }`} | ||
| > | ||
| {"공개"} | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => onIsPublicChange(false)} | ||
| className={`text-sm font-semibold transition-colors ${ | ||
| !form.isPublic ? "text-foreground" : "text-muted-foreground" | ||
| }`} | ||
| > | ||
| {"비공개"} | ||
| </button> |
There was a problem hiding this comment.
공개/비공개 버튼의 className을 동적으로 설정하기 위해 템플릿 리터럴을 사용하고 계십니다. 프로젝트의 다른 부분에서는 cn 유틸리티를 일관되게 사용하고 있으므로, 여기서도 cn을 사용하여 코드 스타일의 일관성을 유지하는 것이 좋겠습니다. (import { cn } from '@/shared/lib/utils' 추가 필요)
<button
type="button"
onClick={() => onIsPublicChange(true)}
className={cn(
"text-sm font-semibold transition-colors",
form.isPublic ? "text-foreground" : "text-muted-foreground",
)}
>
{"공개"}
</button>
<button
type="button"
onClick={() => onIsPublicChange(false)}
className={cn(
"text-sm font-semibold transition-colors",
!form.isPublic ? "text-foreground" : "text-muted-foreground",
)}
>
{"비공개"}
</button>
1 participant
#️⃣연관된 이슈
#7
📝작업 내용
1. 구현 목적
2. 컴포넌트 구조 정리
GroupGenerateForm: 상태 조합/전달 역할(컨테이너)GroupGenerateRequiredFields: 필수 입력 UI(그룹명, 설명, 최대 인원, 공개 여부)GroupGenerateOptionalFields: 선택 입력 UI(그룹 이미지)GroupGenerateSubmitBtn: 폼 제출 로직 전담3. 폼 상태 모델링
requiredForm/optionalForm으로 분리 관리4. 상수화
features/group/model/groupGenerateForm.tsfeatures/group/GroupGenerateForm.tsxfeatures/group/ui/GroupGenerateRequiredFields.tsx5. API 책임 분리
shared/api/group/generateGroup.ts내부에 파일 업로드 로직 포함shared/api/file/uploadFile.ts생성uploadFile(file)에서 presigned 발급/PUT/완료 확인 처리generateGroup은 그룹 생성에만 집중하고, 필요 시uploadFile호출해 key 사용6. 테스트
tests/e2e/group-create.spec.js스크린샷 (선택)