Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough학생 전용 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as 사용자
participant Form as LocalLoginForm
participant Query as ReactQuery
participant API as authApi.localLogin
participant Store as useUserStore
participant Router as useNavigate
User->>Form: 폼 제출(name,email,oAuthToken,studentId?)
Form->>Form: 입력 검증 (필수/학생 여부)
Form->>Query: localMutations.localLogin(params) 호출
Query->>API: POST /auth/login (provider: 'LOCAL', payload)
API->>API: 응답 검증 (kakaoLoginApiResponseSchema)
API-->>Query: 성공 응답 (accessToken, memberId, role)
Query-->>Form: mutation 성공 콜백
Form->>Store: login(accessToken, memberId, userType)
Form->>Router: userType에 따라 네비게이션(/admin 또는 /student)
Router-->>User: 대상 페이지로 이동
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/pages/common/UserIdInputPage.tsx (1)
138-144: 로컬 로그인 폼 통합이 잘 되었습니다테스트용 로컬 로그인 토글이 깔끔하게 구현되었습니다.
한 가지 확인이 필요한 부분:
isAdmin이false일 때studentId로userIdString을 전달하는데, 학번 입력이 완료되지 않은 상태(userIdString.length < 7)에서도LocalLoginForm이 렌더링될 수 있습니다.💡 제안: 학번 입력 완료 여부 검증 추가
LocalLoginForm내부에서 검증하거나, 조건부 렌더링 시isComplete체크를 추가하는 것을 고려해보세요:- {showLocalForm && <LocalLoginForm isAdmin={isAdmin} studentId={isAdmin ? undefined : userIdString} />} + {showLocalForm && ( + <LocalLoginForm + isAdmin={isAdmin} + studentId={isAdmin ? undefined : (isComplete ? userIdString : undefined)} + /> + )}또는
LocalLoginForm컴포넌트에서studentId유효성을 검증하여 submit을 막는 방법도 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/common/UserIdInputPage.tsx` around lines 138 - 144, The LocalLoginForm can be rendered with an incomplete student ID when isAdmin is false because you pass userIdString unguarded; update the conditional render around LocalLoginForm (the showLocalForm + LocalLoginForm usage) to only render when either isAdmin is true or userIdString meets the completed-length check (e.g., userIdString.length >= 7), or alternatively add a validity prop (e.g., isStudentIdComplete) passed into LocalLoginForm and block submission inside LocalLoginForm; locate the showLocalForm state and the LocalLoginForm component invocation to implement the chosen guard so the form never receives an incomplete studentId.src/features/auth/local/api/localMutations.ts (1)
1-8: 뮤테이션 구조가 적절합니다React Query 뮤테이션 패턴을 잘 따르고 있습니다.
type키워드를 사용한 type-only import도 가이드라인에 부합합니다.📝 파일명 컨벤션 제안 (선택사항)
코딩 가이드라인에 따르면 일반 파일명은 kebab-case를 사용해야 합니다:
localMutations.ts → local-mutations.ts다만 기존 코드베이스의 다른 뮤테이션 파일들이 어떤 컨벤션을 따르는지 확인 후 일관성 있게 적용하시면 됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/auth/local/api/localMutations.ts` around lines 1 - 8, The file naming doesn't follow the kebab-case convention; rename the module file from localMutations.ts to local-mutations.ts and update all imports referencing localMutations to the new filename, ensuring exported symbol localMutations and its members (localLogin, mutationKey, mutationFn) and the type import LocalLoginParams remain unchanged; run the TypeScript/IDE import fixer or search-replace across the repo to update usages and verify builds/tests pass.src/entities/auth/api/authApi.ts (1)
30-42: 엔드포인트 네이밍 개선을 제안합니다현재 코드는
ENDPOINTS.AUTH.KAKAO_LOGIN엔드포인트에provider: 'LOCAL'을 함께 보내는 구조로, 이는 백엔드가 단일 엔드포인트(/oauth2/authorization)에서 여러 인증 제공자(Kakao, Local 등)를provider파라미터로 구분하는 설계로 보입니다.다만, 엔드포인트명
KAKAO_LOGIN은 Local 인증에도 사용되므로 명확하지 않습니다. 다음과 같이 개선하는 것을 권장합니다:
ENDPOINTS.AUTH.KAKAO_LOGIN→ENDPOINTS.AUTH.LOGIN또는ENDPOINTS.AUTH.AUTH_LOGIN으로 변경- 반환 타입
KakaoLoginApiResponse→LoginResponse또는AuthLoginResponse로 변경 (Kakao 특화 타입이 아니므로)이렇게 하면 향후 추가 인증 제공자 지원 시 코드 의도가 더욱 명확해질 것입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entities/auth/api/authApi.ts` around lines 30 - 42, Rename the Kakao-specific endpoint and response types to generic auth names: update usages of ENDPOINTS.AUTH.KAKAO_LOGIN to ENDPOINTS.AUTH.LOGIN (or AUTH_LOGIN) and rename the return type KakaoLoginApiResponse to a generic LoginResponse (and corresponding kakaoLoginApiResponseSchema to loginApiResponseSchema) so localLogin and any other auth methods use a non-provider-specific identifier; update the import/exports where those symbols are defined and adjust localLogin to return LoginResponse and parse response.data.response with the renamed schema to keep naming consistent with provider-agnostic behavior.src/features/auth/local/ui/LocalLoginForm.tsx (2)
37-45: 공백만 입력된 경우 대응 추가를 고려해보세요.현재 빈 문자열 체크만 하고 있어서, 공백만 입력해도 통과됩니다. 테스트용 폼이라 큰 문제는 아니지만,
trim()추가로 간단히 개선할 수 있습니다.💡 trim() 적용 예시
const handleLocalSubmit = () => { - if (!localName || !localEmail || !localOAuthToken) { + if (!localName.trim() || !localEmail.trim() || !localOAuthToken.trim()) { alert('이름, 이메일, OAuthToken을 모두 입력해주세요.'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/auth/local/ui/LocalLoginForm.tsx` around lines 37 - 45, The validation in handleLocalSubmit only checks for empty strings, allowing whitespace-only input to pass; update the checks to trim inputs before validation (e.g., use localName.trim(), localEmail.trim(), localOAuthToken.trim(), and studentId.trim()) so whitespace-only values are treated as empty and trigger the existing alert paths; adjust the conditional logic in handleLocalSubmit to use the trimmed values (or check .trim().length === 0) and keep the same alerts/flow.
55-103: 시맨틱 HTML:<form>요소 사용을 권장합니다.현재
<div>로 감싸져 있어 Enter 키로 폼 제출이 되지 않고, 스크린 리더가 폼으로 인식하지 못합니다. 또한<label>과<input>이htmlFor/id로 연결되어 있지 않아 접근성이 떨어집니다.테스트용 로그인 폼이지만, 시맨틱 HTML 사용은 좋은 습관이니까요! 🎯
참고: MDN - Form 접근성
♻️ 시맨틱 폼 구조로 개선하는 예시
return ( - <div className='flex flex-col gap-3 w-95 text-left mt-6 animate-in fade-in slide-in-from-top-2 duration-300'> + <form + onSubmit={(e) => { + e.preventDefault(); + handleLocalSubmit(); + }} + className='flex flex-col gap-3 w-95 text-left mt-6 animate-in fade-in slide-in-from-top-2 duration-300'> <div className='flex flex-col gap-2'> - <label className='text-xs font-medium text-secondary-black'> + <label htmlFor='local-name' className='text-xs font-medium text-secondary-black'> 테스트용 이름 </label> <input + id='local-name' type='text' placeholder='이름' value={localName} onChange={(e) => setLocalName(e.target.value)} className='border border-stroke rounded-md px-3 py-2.5 text-sm focus:border-primary outline-none transition-colors' /> </div> <div className='flex flex-col gap-2'> - <label className='text-xs font-medium text-secondary-black'> + <label htmlFor='local-email' className='text-xs font-medium text-secondary-black'> 테스트용 이메일 </label> <input + id='local-email' type='email' placeholder='이메일' value={localEmail} onChange={(e) => setLocalEmail(e.target.value)} className='border border-stroke rounded-md px-3 py-2.5 text-sm focus:border-primary outline-none transition-colors' /> </div> <div className='flex flex-col gap-2'> - <label className='text-xs font-medium text-secondary-black'> + <label htmlFor='local-oauth-token' className='text-xs font-medium text-secondary-black'> OAuth Token </label> <input + id='local-oauth-token' type='password' placeholder='OAuthToken' value={localOAuthToken} onChange={(e) => setLocalOAuthToken(e.target.value)} className='border border-stroke rounded-md px-3 py-2.5 text-sm focus:border-primary outline-none transition-colors' /> </div> <button + type='submit' disabled={isPending} - onClick={handleLocalSubmit} className='bg-primary text-white text-sm font-semibold py-3.5 rounded-lg cursor-pointer disabled:opacity-40 disabled:cursor-not-allowed hover:bg-primary/90 transition-all mt-2'> {isPending ? '로그인 중...' : '로컬 로그인 실행'} </button> - </div> + </form> );Based on learnings: JiiminHa prefers using semantic HTML tags over excessive div usage for better code structure and accessibility in React components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/auth/local/ui/LocalLoginForm.tsx` around lines 55 - 103, The form wrapper is a plain <div>, and labels aren't linked to inputs, which breaks keyboard submit and accessibility; update the LocalLoginForm JSX to use a semantic <form> (replace the outer <div> with a <form>), move the click handler to an onSubmit that calls handleLocalSubmit and prevents default, set the submit button to type="submit" (and remove onClick or keep but ensure onSubmit is primary), and add unique id attributes to the inputs (for localName, localEmail, localOAuthToken) with corresponding label htmlFor values so labels are programmatically associated; keep existing state variables and isPending logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/entities/auth/api/authApi.ts`:
- Around line 30-42: Rename the Kakao-specific endpoint and response types to
generic auth names: update usages of ENDPOINTS.AUTH.KAKAO_LOGIN to
ENDPOINTS.AUTH.LOGIN (or AUTH_LOGIN) and rename the return type
KakaoLoginApiResponse to a generic LoginResponse (and corresponding
kakaoLoginApiResponseSchema to loginApiResponseSchema) so localLogin and any
other auth methods use a non-provider-specific identifier; update the
import/exports where those symbols are defined and adjust localLogin to return
LoginResponse and parse response.data.response with the renamed schema to keep
naming consistent with provider-agnostic behavior.
In `@src/features/auth/local/api/localMutations.ts`:
- Around line 1-8: The file naming doesn't follow the kebab-case convention;
rename the module file from localMutations.ts to local-mutations.ts and update
all imports referencing localMutations to the new filename, ensuring exported
symbol localMutations and its members (localLogin, mutationKey, mutationFn) and
the type import LocalLoginParams remain unchanged; run the TypeScript/IDE import
fixer or search-replace across the repo to update usages and verify builds/tests
pass.
In `@src/features/auth/local/ui/LocalLoginForm.tsx`:
- Around line 37-45: The validation in handleLocalSubmit only checks for empty
strings, allowing whitespace-only input to pass; update the checks to trim
inputs before validation (e.g., use localName.trim(), localEmail.trim(),
localOAuthToken.trim(), and studentId.trim()) so whitespace-only values are
treated as empty and trigger the existing alert paths; adjust the conditional
logic in handleLocalSubmit to use the trimmed values (or check .trim().length
=== 0) and keep the same alerts/flow.
- Around line 55-103: The form wrapper is a plain <div>, and labels aren't
linked to inputs, which breaks keyboard submit and accessibility; update the
LocalLoginForm JSX to use a semantic <form> (replace the outer <div> with a
<form>), move the click handler to an onSubmit that calls handleLocalSubmit and
prevents default, set the submit button to type="submit" (and remove onClick or
keep but ensure onSubmit is primary), and add unique id attributes to the inputs
(for localName, localEmail, localOAuthToken) with corresponding label htmlFor
values so labels are programmatically associated; keep existing state variables
and isPending logic intact.
In `@src/pages/common/UserIdInputPage.tsx`:
- Around line 138-144: The LocalLoginForm can be rendered with an incomplete
student ID when isAdmin is false because you pass userIdString unguarded; update
the conditional render around LocalLoginForm (the showLocalForm + LocalLoginForm
usage) to only render when either isAdmin is true or userIdString meets the
completed-length check (e.g., userIdString.length >= 7), or alternatively add a
validity prop (e.g., isStudentIdComplete) passed into LocalLoginForm and block
submission inside LocalLoginForm; locate the showLocalForm state and the
LocalLoginForm component invocation to implement the chosen guard so the form
never receives an incomplete studentId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d94860c-66a4-404f-965a-d88c8b846597
📒 Files selected for processing (9)
src/App.tsxsrc/entities/auth/api/authApi.tssrc/features/auth/local/api/localMutations.tssrc/features/auth/local/ui/LocalLoginForm.tsxsrc/features/auth/sync-user-role/model/useSyncUserRole.tssrc/pages/chat/ChatPage.tsxsrc/pages/common/UserIdInputPage.tsxsrc/shared/config/routes.tssrc/shared/ui/Header.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/entities/auth/model/useUserStore.ts (1)
48-55:⚠️ Potential issue | 🟠 Major
logout()메서드에서memberId를 초기화하지 않아 세션에 이전 사용자 식별자가 남습니다48-54줄의
logout()함수에서userType,userName,accessToken,isAuthenticated를 초기화하지만memberId는 누락되어 있습니다. 60-66줄의partialize설정이memberId를 포함해 sessionStorage에 저장하므로, 로그아웃 후에도 이전 사용자의memberId가 세션에 남게 됩니다. 개인정보 보호 차원에서 다음과 같이 수정하세요:수정 제안 (diff)
logout: () => { set({ userType: 'guest', userName: '', + memberId: null, accessToken: null, isAuthenticated: false, }); },Zustand persist 미들웨어 관련 더 자세한 내용은 공식 문서를 참고하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entities/auth/model/useUserStore.ts` around lines 48 - 55, The logout() method currently resets userType, userName, accessToken, and isAuthenticated but omits memberId, so previous memberId remains in sessionStorage via the persist partialize; update the logout() implementation (function logout) to also reset memberId (e.g., to null or empty string) and verify the persist partialize configuration includes memberId so it is cleared from sessionStorage on logout (check the partialize closure that lists memberId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/entities/auth/model/useUserStore.ts`:
- Around line 48-55: The logout() method currently resets userType, userName,
accessToken, and isAuthenticated but omits memberId, so previous memberId
remains in sessionStorage via the persist partialize; update the logout()
implementation (function logout) to also reset memberId (e.g., to null or empty
string) and verify the persist partialize configuration includes memberId so it
is cleared from sessionStorage on logout (check the partialize closure that
lists memberId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ab41a0b-4c40-4e81-8737-af5b24c7aaa1
📒 Files selected for processing (3)
src/entities/auth/model/useUserStore.tssrc/features/auth/local/ui/LocalLoginForm.tsxsrc/pages/common/UserIdInputPage.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/common/UserIdInputPage.tsx
- src/features/auth/local/ui/LocalLoginForm.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/shared/ui/Header.tsx (1)
105-105: 여긴 store 전체보다 필요한 slice만 구독하는 편이 낫습니다.
useUserStore()전체를 구독하면accessToken,memberId처럼 이 컴포넌트가 쓰지 않는 값이 바뀔 때도Header가 함께 리렌더됩니다. Zustand 문서도 selector로 필요한 상태만 읽는 방식을 권장합니다. 지금 파일의AuthenticatedHeader처럼 필드별 selector로 맞추면 더 가볍습니다. 관련 문서: Zustand selectors. (zustand.docs.pmnd.rs)♻️ 제안 코드
export default function Header() { - const {userType, isAuthenticated} = useUserStore(); + const userType = useUserStore((state) => state.userType); + const isAuthenticated = useUserStore((state) => state.isAuthenticated); if (!isAuthenticated) { return <GuestHeader />;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/Header.tsx` at line 105, Header subscribes to the entire useUserStore which causes unnecessary re-renders when unrelated fields (like accessToken/memberId) change; change the subscriptions to selector-based reads so the component only selects the fields it needs (e.g., replace the combined destructure in Header with separate selector calls for userType and isAuthenticated via useUserStore(state => state.userType) and useUserStore(state => state.isAuthenticated)), and mirror this selector approach in AuthenticatedHeader so each component only subscribes to its required slices.src/shared/lib/stompClient.ts (1)
5-6:brokerURL조합이 env 값의 path 형태에 묶여 있습니다.
VITE_API_BASE_URL에 trailing slash나/api같은 path가 들어오면 현재 구현은 그 path까지 이어붙여//ws/stomp또는/api/ws/stomp를 만들 수 있습니다. WebSocket endpoint를 항상/ws/stomp로 쓸 의도라면new URL('/ws/stomp', baseURL)로 경로를 고정한 뒤 protocol만ws:/wss:로 바꾸는 편이 더 안전합니다. STOMPJS의brokerURL은 브로커의 WebSocket endpoint 전체 URL을 기대합니다. 관련 문서: STOMPJSClient#brokerURL. (stomp-js.github.io)🔧 제안 코드
export const createStompClient = () => { const baseURL = import.meta.env.VITE_API_BASE_URL; - const brokerURL = baseURL.replace('http', 'ws') + '/ws/stomp'; + const url = new URL('/ws/stomp', baseURL); + url.protocol = url.protocol === 'https:' ? 'wss:' : 'ws:'; + const brokerURL = url.toString(); return new Client({ brokerURL: brokerURL,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/lib/stompClient.ts` around lines 5 - 6, brokerURL is built by string replace on baseURL (import.meta.env.VITE_API_BASE_URL), which can produce incorrect paths when baseURL contains a trailing slash or path; instead construct the WebSocket endpoint using the URL API to ensure the path is exactly '/ws/stomp' and then switch the protocol to 'ws:' or 'wss:' based on the original URL's protocol; update the code that computes brokerURL (the variable named brokerURL that currently does baseURL.replace('http','ws') + '/ws/stomp') to create a new URL('/ws/stomp', baseURL) and set its protocol appropriately before passing it to STOMPJS Client#brokerURL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/ui/Header.tsx`:
- Around line 111-114: The switch on userType in Header.tsx returns the same
AuthenticatedHeader with showChat={true} for both 'admin' and 'student', which
lets students see admin-only assignment button; update the switch to
differentiate roles—either return different props (e.g., showAssignment or
showChat) per case or split into explicit cases returning <AuthenticatedHeader
showChat={true} showAssignment={userType==='admin'} /> (or a student-specific
header) so that assignmentButton (inside AuthenticatedHeader) is only rendered
for admins and routes using ROUTES.ADMIN.ASSIGNMENTS.MANAGE are not exposed to
students; adjust AuthenticatedHeader prop signature and its assignmentButton
render logic accordingly and ensure navigation uses role-scoped paths (e.g.,
/admin/* vs /student/*) with useNavigate-aware handlers.
---
Nitpick comments:
In `@src/shared/lib/stompClient.ts`:
- Around line 5-6: brokerURL is built by string replace on baseURL
(import.meta.env.VITE_API_BASE_URL), which can produce incorrect paths when
baseURL contains a trailing slash or path; instead construct the WebSocket
endpoint using the URL API to ensure the path is exactly '/ws/stomp' and then
switch the protocol to 'ws:' or 'wss:' based on the original URL's protocol;
update the code that computes brokerURL (the variable named brokerURL that
currently does baseURL.replace('http','ws') + '/ws/stomp') to create a new
URL('/ws/stomp', baseURL) and set its protocol appropriately before passing it
to STOMPJS Client#brokerURL.
In `@src/shared/ui/Header.tsx`:
- Line 105: Header subscribes to the entire useUserStore which causes
unnecessary re-renders when unrelated fields (like accessToken/memberId) change;
change the subscriptions to selector-based reads so the component only selects
the fields it needs (e.g., replace the combined destructure in Header with
separate selector calls for userType and isAuthenticated via useUserStore(state
=> state.userType) and useUserStore(state => state.isAuthenticated)), and mirror
this selector approach in AuthenticatedHeader so each component only subscribes
to its required slices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0220715c-d463-450b-bf1f-995817e038e8
📒 Files selected for processing (3)
src/App.tsxsrc/shared/lib/stompClient.tssrc/shared/ui/Header.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.tsx
⚙️ Related ISSUE Number
close #77
📄 Work Description
로컬 로그인 폼 적용 및 학생용 헤더에 채팅 추가
📷 Screenshot
💬 To Reviewers
🔗 Reference