jangjunwoo/ldf-7-login - #12
Conversation
/auth/gender components and screen
/auth/login /auth/login/gender
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough인증 로그인 기능에 성별 선택 및 IDP 로그인 화면이 추가되었습니다. 관련 번역, 로딩 UI, Storybook 스토리, 중첩 라우트와 공개 export가 구성되었으며, 공통 레이아웃 및 배경 스타일과 패키지 버전이 갱신되었습니다. Changes인증 로그인 기능
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginRoute
participant LoginFrame
participant LoginLayoutScreen
participant LoginControl
User->>LoginRoute: 로그인 경로 접속
LoginRoute->>LoginFrame: 프레임 렌더링
LoginFrame->>LoginLayoutScreen: 레이아웃 렌더링
LoginLayoutScreen->>LoginControl: IDP 버튼 또는 성별 선택 UI 렌더링
User->>LoginControl: 로그인 또는 시작 클릭
LoginControl->>LoginFrame: onLogin 콜백 호출
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
edit version
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/features/auth/views/components/gender-button/index.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win타입 import를 인라인 형식으로 맞춰 주세요.
import type { ReactNode } from 'react'대신import { type ReactNode } from 'react'를 사용해야 합니다.As per coding guidelines, 타입 import는 standalone
import type가 아니라 inline type import를 사용해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/views/components/gender-button/index.tsx` at line 1, Update the ReactNode import in the gender button component to use the inline type-import syntax within a regular import, replacing the standalone type import while preserving the same type-only behavior.Source: Coding guidelines
src/features/auth/views/screens/login-layout-screen.stories.tsx (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winexternal import를 alias import보다 먼저 배치해 주세요.
현재
@/features/authimport가 Storybook external import보다 앞에 있습니다. As per coding guidelines, external import를 먼저 두고 internal alias import를 뒤에 배치해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/views/screens/login-layout-screen.stories.tsx` around lines 1 - 3, Reorder the imports in the story module so the external Storybook type import from `@storybook/react-vite` appears before the internal `@/features/auth` alias import, without changing either import’s contents.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/components/layout/index.tsx`:
- Line 9: Update the layout container’s text color class in the div near the
root layout so it is not inherited by child controls such as the unselected
button rendered by the gender-button component. Restrict white text to the dark
elements that require it, or assign the unselected gender button an explicit
dark text color while preserving the existing selected-state styling.
In `@src/common/components/ui/loading/index.stories.tsx`:
- Around line 1-3: Update the imports in the Loading Storybook module so the
external `@storybook/react-vite` import appears before the sibling Loading import,
and merge Meta and StoryObj into the external import using inline type
specifiers instead of a standalone import type declaration.
In `@src/common/components/ui/loading/index.tsx`:
- Around line 5-6: Update the Loading component’s Loader2 element to expose the
loading state to assistive technology by accepting a translated label prop and
applying it as aria-label alongside role="status". Preserve the existing styling
and loading behavior.
In `@src/features/auth/views/components/gender-button/index.tsx`:
- Around line 14-22: Update the button in the gender-button component to expose
its selected state to assistive technologies by adding aria-pressed={selected}.
Preserve the existing styling, click behavior, disabled handling, and prop
spreading.
In `@src/features/auth/views/components/gender-select/index.tsx`:
- Around line 19-25: Update the login/start button in the component containing
clickLogin to use disabled={clicked || selected === null}, preventing activation
before a gender is selected or after clicking. Remove the console.log debug
statement from clickLogin while preserving its existing guard and onLogin
behavior.
- Around line 49-55: Update the button rendering around clickLogin so the
clicked loading state preserves an accessible name instead of exposing only the
Loader2 icon. Add aria-busy={clicked} and retain the existing start-label as an
accessible or visually hidden text while showing the loader, without changing
the non-loading behavior.
In `@src/features/auth/views/components/idp-login-button/index.tsx`:
- Around line 10-17: Update the button returned by the IdP login component to
bind its disabled state to the existing clicked state, so it becomes disabled
immediately after the first click while preserving the current onLogin flow.
In `@src/features/auth/views/frames/gender-select-frame.tsx`:
- Line 9: Remove the gender value from the console.log statement in
GenderSelectFrame, or remove the debug logging entirely; authentication flow
logs must not expose the user's gender.
- Around line 8-11: Update handleGenderSelect in GenderSelectFrame to invoke the
actual login request using the selected gender, navigate to the post-login
destination on success, and clear the loading state when the request fails.
Remove the console-only behavior and restore the real navigation flow,
preserving the callback’s loading-state contract.
In `@src/features/auth/views/frames/idp-login-frame.tsx`:
- Around line 1-10: Update IdpLoginFrame’s handleLogin to navigate to the
gender-selection route '/auth/login/gender' after successful login instead of
only logging. Re-enable and use the existing useNavigate integration, then keep
passing handleLogin to IdpLoginButton so the redirect occurs through the login
completion callback.
In `@src/features/auth/views/screens/login-layout-screen.tsx`:
- Line 1: Replace the standalone type import for ReactNode in
src/features/auth/views/screens/login-layout-screen.tsx lines 1-1 with inline
type-import syntax. Apply the same inline type-import conversion to Meta and
StoryObj in src/features/auth/views/screens/login-layout-screen.stories.tsx
lines 3-3; no other changes are needed.
---
Nitpick comments:
In `@src/features/auth/views/components/gender-button/index.tsx`:
- Line 1: Update the ReactNode import in the gender button component to use the
inline type-import syntax within a regular import, replacing the standalone type
import while preserving the same type-only behavior.
In `@src/features/auth/views/screens/login-layout-screen.stories.tsx`:
- Around line 1-3: Reorder the imports in the story module so the external
Storybook type import from `@storybook/react-vite` appears before the internal
`@/features/auth` alias import, without changing either import’s contents.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfa76a7e-2fb7-42fe-9245-88bd523ee806
📒 Files selected for processing (30)
AGENTS.mdpackage.jsonpublic/locales/en/_.jsonpublic/locales/ko/_.jsonsrc/common/components/index.tssrc/common/components/layout/index.tsxsrc/common/components/nav-bar/index.tsxsrc/common/components/ui/index.tssrc/common/components/ui/loading/index.stories.tsxsrc/common/components/ui/loading/index.tsxsrc/features/auth/index.tssrc/features/auth/models/gender.tssrc/features/auth/models/index.tssrc/features/auth/viewmodels/index.tssrc/features/auth/views/components/gender-button/index.tsxsrc/features/auth/views/components/gender-select/index.tsxsrc/features/auth/views/components/idp-login-button/index.tsxsrc/features/auth/views/components/index.tssrc/features/auth/views/frames/gender-select-frame.tsxsrc/features/auth/views/frames/idp-login-frame.tsxsrc/features/auth/views/frames/index.tssrc/features/auth/views/index.tssrc/features/auth/views/screens/index.tssrc/features/auth/views/screens/login-layout-screen.stories.tsxsrc/features/auth/views/screens/login-layout-screen.tsxsrc/features/user/views/index.tssrc/routes/auth/login/gender.tsxsrc/routes/auth/login/index.tsxsrc/routes/auth/login/route.tsxsrc/styles.css
| <div className={cn('bg-bg-surface h-dvh px-5', className)}> | ||
| <div className=" text-white mx-auto flex h-full min-h-0 w-full max-w-100 flex-col gap-5 bg-bg"> | ||
| <NavBar className={'border-b border-b-white mx-10'} /> | ||
| <div className="bg-bg mx-auto flex h-full min-h-0 w-full max-w-100 flex-col gap-5 text-white"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
하위 버튼에 text-white를 전역 상속시키지 않도록 수정해 주세요.
src/features/auth/views/components/gender-button/index.tsx의 비선택 상태는 bg-bg-surface만 지정하므로 이 컨테이너의 text-white를 상속합니다. 현재 src/styles.css Line 81의 #e5e5e5 배경과 흰색 텍스트 조합은 대비가 부족해 성별 선택 UI의 텍스트를 읽기 어렵습니다. 흰색 텍스트를 어두운 요소에만 적용하거나, 비선택 버튼에 명시적인 어두운 텍스트 색상을 지정해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/common/components/layout/index.tsx` at line 9, Update the layout
container’s text color class in the div near the root layout so it is not
inherited by child controls such as the unselected button rendered by the
gender-button component. Restrict white text to the dark elements that require
it, or assign the unselected gender button an explicit dark text color while
preserving the existing selected-state styling.
| export function Loading({ fullScreen = true, className, containerClassName }: Loading.Props) { | ||
| const loading = <Loader2 className={cn('text-icon size-12 animate-spin', className)} />; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
로딩 상태를 보조기술에 노출해 주세요.
Line 6의 Loader2는 시각적으로만 회전하며 role과 접근 가능한 이름이 없습니다. IDP 로그인이나 라우트 전환 중임을 스크린 리더 사용자가 알 수 없으므로, 번역된 label을 prop으로 받아 role="status"와 aria-label을 적용해 주세요.
권장 예시
- const loading = <Loader2 className={cn('text-icon size-12 animate-spin', className)} />;
+ const loading = (
+ <Loader2
+ role="status"
+ aria-label="Loading"
+ className={cn('text-icon size-12 animate-spin', className)}
+ />
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function Loading({ fullScreen = true, className, containerClassName }: Loading.Props) { | |
| const loading = <Loader2 className={cn('text-icon size-12 animate-spin', className)} />; | |
| export function Loading({ fullScreen = true, className, containerClassName }: Loading.Props) { | |
| const loading = ( | |
| <Loader2 | |
| role="status" | |
| aria-label="Loading" | |
| className={cn('text-icon size-12 animate-spin', className)} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/common/components/ui/loading/index.tsx` around lines 5 - 6, Update the
Loading component’s Loader2 element to expose the loading state to assistive
technology by accepting a translated label prop and applying it as aria-label
alongside role="status". Preserve the existing styling and loading behavior.
| <button | ||
| className={cn( | ||
| 'w-full rounded-lg py-2 text-sm transition-all duration-150', | ||
| selected ? 'bg-bg text-white' : 'bg-bg-surface', | ||
| className, | ||
| )} | ||
| onClick={onClick} | ||
| disabled={disabled} | ||
| {...props} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
선택 상태를 보조기술에 노출하세요.
현재 selected는 색상 클래스만 변경하므로 스크린 리더가 선택된 항목을 알 수 없습니다. aria-pressed={selected} 또는 radio semantics를 추가해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/auth/views/components/gender-button/index.tsx` around lines 14 -
22, Update the button in the gender-button component to expose its selected
state to assistive technologies by adding aria-pressed={selected}. Preserve the
existing styling, click behavior, disabled handling, and prop spreading.
| const clickLogin = () => { | ||
| if (!selected) { | ||
| console.log('not selected'); | ||
| return; | ||
| } | ||
| setClicked(true); | ||
| onLogin(selected); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
선택 전 시작 버튼을 비활성화하세요.
현재 성별을 선택하지 않아도 버튼이 활성화되어 있으며, 클릭하면 로그만 남기고 아무 동작도 하지 않습니다. disabled={clicked || selected === null}로 설정하고 디버그 로그는 제거해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/auth/views/components/gender-select/index.tsx` around lines 19 -
25, Update the login/start button in the component containing clickLogin to use
disabled={clicked || selected === null}, preventing activation before a gender
is selected or after clicking. Remove the console.log debug statement from
clickLogin while preserving its existing guard and onLogin behavior.
| const handleGenderSelect = (gender: Gender) => { | ||
| console.log(`gender selected: ${gender} in GenderSelectFrame`); | ||
| // navigate({ to: '/' }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
성별 선택 콜백을 실제 로그인 완료 흐름에 연결해야 합니다.
GenderSelect는 callback 호출 전에 로딩 상태를 고정하지만, 현재 handler는 console.log만 실행하고 이동 코드는 주석 처리되어 있습니다. 첫 클릭 후 로더가 영구적으로 남으므로 실제 로그인 요청과 성공 시 navigation, 실패 시 로딩 해제를 연결해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/auth/views/frames/gender-select-frame.tsx` around lines 8 - 11,
Update handleGenderSelect in GenderSelectFrame to invoke the actual login
request using the selected gender, navigate to the post-login destination on
success, and clear the loading state when the request fails. Remove the
console-only behavior and restore the real navigation flow, preserving the
callback’s loading-state contract.
| // const navigate = useNavigate(); | ||
|
|
||
| const handleGenderSelect = (gender: Gender) => { | ||
| console.log(`gender selected: ${gender} in GenderSelectFrame`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
성별 값을 콘솔에 기록하지 마세요.
인증 흐름에서 사용자의 성별 값을 브라우저 콘솔에 기록하고 있습니다. 클라이언트 로그 수집이나 디버그 접근을 통해 개인 속성이 노출될 수 있으므로 값을 제거하거나 비식별화해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/auth/views/frames/gender-select-frame.tsx` at line 9, Remove the
gender value from the console.log statement in GenderSelectFrame, or remove the
debug logging entirely; authentication flow logs must not expose the user's
gender.
| // import { useNavigate } from '@tanstack/react-router'; | ||
|
|
||
| import { IdpLoginButton, LoginLayoutScreen } from '@/features/auth'; | ||
|
|
||
| export function IdpLoginFrame() { | ||
| // const navigate = useNavigate(); | ||
|
|
||
| const handleLogin = () => { | ||
| console.log('Login complete in IdpLoginFrame'); | ||
| // navigate({ to: '/auth/login/gender' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'src/features/auth/views/frames/idp-login-frame\.tsx$|src/features/auth' | head -80
echo "== target file =="
cat -n src/features/auth/views/frames/idp-login-frame.tsx
echo "== relevant auth files outline =="
for f in $(git ls-files 'src/features/auth/**' | rg 'index\.tsx$|idp-login-button|gender|login' | head -80); do
echo "--- $f"
wc -l "$f"
done
rg -n "handleLogin|IdpLoginButton|gender|/auth/login/gender|useNavigate|onLogin|onSuccess" src/features/auth src/routes 2>/dev/null | head -200Repository: gsainfoteam/bbosong-fe
Length of output: 5695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== idp-login-frame =="
cat -n src/features/auth/views/frames/idp-login-frame.tsx
echo "== idp-login-button =="
cat -n src/features/auth/views/components/idp-login-button/index.tsx
echo "== gender-select-frame and gender route =="
cat -n src/features/auth/views/frames/gender-select-frame.tsx
cat -n src/routes/auth/login/gender.tsx
echo "== package deps relevant =="
if [ -f package.json ]; then
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
console.log(JSON.stringify({
dependencies: pkg.dependencies && Object.fromEntries(Object.entries(pkg.dependencies).filter(([k]) => /react-router|tanstack|react/.test(k))),
devDependencies: pkg.devDependencies && Object.fromEntries(Object.entries(pkg.devDependencies).filter(([k]) => /bun-types|typescript|react/.test(k))),
}, null, 2));
JS
fi
echo "== route files under auth =="
git ls-files 'src/routes/auth/**/*.tsx' 'src/routes/**/*.tsx' | sed -n '1,120p'Repository: gsainfoteam/bbosong-fe
Length of output: 3996
IDP 로그인 완료 후 성별 선택 라우트로 이동하도록 연결해야 합니다.
handleLogin이 console.log만 실행하고 onLogin 전달의 IdpLoginButton은 클릭 시 로딩 상태만으로 끝나기 때문에, 로그인 성공 처리 후 다음 라우트인 '/auth/login/gender'로 이동해야 합니다.
수정 예시
-// import { useNavigate } from '`@tanstack/react-router`';
+import { useNavigate } from '`@tanstack/react-router`';
export function IdpLoginFrame() {
- // const navigate = useNavigate();
+ const navigate = useNavigate();
const handleLogin = () => {
- console.log('Login complete in IdpLoginFrame');
- // navigate({ to: '/auth/login/gender' });
+ navigate({ to: '/auth/login/gender' });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // import { useNavigate } from '@tanstack/react-router'; | |
| import { IdpLoginButton, LoginLayoutScreen } from '@/features/auth'; | |
| export function IdpLoginFrame() { | |
| // const navigate = useNavigate(); | |
| const handleLogin = () => { | |
| console.log('Login complete in IdpLoginFrame'); | |
| // navigate({ to: '/auth/login/gender' }); | |
| import { useNavigate } from '`@tanstack/react-router`'; | |
| import { IdpLoginButton, LoginLayoutScreen } from '`@/features/auth`'; | |
| export function IdpLoginFrame() { | |
| const navigate = useNavigate(); | |
| const handleLogin = () => { | |
| navigate({ to: '/auth/login/gender' }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/auth/views/frames/idp-login-frame.tsx` around lines 1 - 10,
Update IdpLoginFrame’s handleLogin to navigate to the gender-selection route
'/auth/login/gender' after successful login instead of only logging. Re-enable
and use the existing useNavigate integration, then keep passing handleLogin to
IdpLoginButton so the redirect occurs through the login completion callback.
apply coderabbit suggestions
|
@coderabbitai review |
✅ Action performedReview finished.
|
apply coderabbit suggestions
* Login components and pages(LDF-7) /auth/gender components and screen * Login components and pages(LDF-7) /auth/login /auth/login/gender * Login components and pages(LDF-7) edit version * Login components and pages(LDF-7) apply coderabbit suggestions * Login components and pages(LDF-7) apply coderabbit suggestions
1 participant
Summary by CodeRabbit