Conversation
Member
Author
|
@coderabbitai 리뷰 해줘 |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthrough이 PR은 사용자 메뉴(햄버거 메뉴)의 외부 영역 클릭과 Escape 키 입력으로 메뉴를 닫는 기능을 추가합니다. 새로운 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/domains/auth/components/UserMenuButton/__tests__/UserMenuButton.test.tsx (1)
22-28:userEvent인스턴스를 일관되게 재사용하는 것을 권장합니다.
openUserMenu헬퍼 함수 내에서userEvent.setup()을 호출하면 테스트마다 새로운 인스턴스가 생성됩니다. 동일 테스트 내에서 헬퍼와 별도의userEvent.setup()호출을 혼용하면(예: Line 57) 이벤트 타이밍 문제로 flaky 테스트가 발생할 수 있습니다.♻️ userEvent 인스턴스를 인자로 받도록 수정 제안
-const openUserMenu = async () => { +const openUserMenu = async (user: ReturnType<typeof userEvent.setup>) => { const triggerButton = screen.getByRole('button', { name: '사용자 메뉴 열기' }); - await userEvent.setup().click(triggerButton); + await user.click(triggerButton); return triggerButton; };사용 예시:
it('메뉴 내부 클릭만으로는 메뉴가 닫히지 않는다', async () => { const user = userEvent.setup(); render(<UserMenuButton positioning="relative" />); await openUserMenu(user); await user.click(screen.getByRole('button', { name: '내 동선 목록' })); expect(screen.getByTestId('user-menu')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/domains/auth/components/UserMenuButton/__tests__/UserMenuButton.test.tsx` around lines 22 - 28, The helper openUserMenu currently calls userEvent.setup() internally which creates a new userEvent instance per call and can cause flaky timing issues when tests also call userEvent.setup() separately; change openUserMenu to accept a userEvent instance parameter (e.g., user) and use that to perform the click instead of calling userEvent.setup() inside the helper, updating all tests to pass a single shared user = userEvent.setup() into openUserMenu to ensure a consistent event instance across the test (reference: openUserMenu and userEvent.setup()).frontend/src/domains/auth/hooks/useUserMenuVisibility.ts (1)
8-14:closeUserMenu을useCallback으로 감싸는 것을 고려해주세요.
closeUserMenu함수가useEffect내부에서 사용되지만 의존성 배열에 포함되어 있지 않습니다. 현재 코드는setIsUserMenuOpen과userMenuTriggerButtonRef가 안정적인 참조이므로 정상 동작하지만, ESLintexhaustive-deps규칙에서 경고가 발생할 수 있습니다.♻️ useCallback 적용 제안
-import { useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; const useUserMenuVisibility = () => { const [isUserMenuOpen, setIsUserMenuOpen] = useState(false); const userMenuRef = useRef<HTMLDivElement>(null); const userMenuTriggerButtonRef = useRef<HTMLButtonElement>(null); - const closeUserMenu = (shouldRestoreFocus = false) => { + const closeUserMenu = useCallback((shouldRestoreFocus = false) => { setIsUserMenuOpen(false); if (shouldRestoreFocus) { userMenuTriggerButtonRef.current?.focus(); } - }; + }, []); useEffect(() => { if (!isUserMenuOpen) { return; } // ... handlers remain the same - }, [isUserMenuOpen]); + }, [isUserMenuOpen, closeUserMenu]);Also applies to: 16-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/domains/auth/hooks/useUserMenuVisibility.ts` around lines 8 - 14, Wrap the closeUserMenu function in useCallback to stabilize its identity for useEffect dependencies: change the standalone const closeUserMenu = (...) => { ... } to a useCallback hook that returns the same logic and include setIsUserMenuOpen and userMenuTriggerButtonRef in the dependency array; do the same for any analogous handlers in this file (the other functions mentioned around lines 16-43) so ESLint exhaustive-deps warnings are resolved and effects can safely list these callbacks as dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/src/domains/auth/components/UserMenuButton/__tests__/UserMenuButton.test.tsx`:
- Around line 22-28: The helper openUserMenu currently calls userEvent.setup()
internally which creates a new userEvent instance per call and can cause flaky
timing issues when tests also call userEvent.setup() separately; change
openUserMenu to accept a userEvent instance parameter (e.g., user) and use that
to perform the click instead of calling userEvent.setup() inside the helper,
updating all tests to pass a single shared user = userEvent.setup() into
openUserMenu to ensure a consistent event instance across the test (reference:
openUserMenu and userEvent.setup()).
In `@frontend/src/domains/auth/hooks/useUserMenuVisibility.ts`:
- Around line 8-14: Wrap the closeUserMenu function in useCallback to stabilize
its identity for useEffect dependencies: change the standalone const
closeUserMenu = (...) => { ... } to a useCallback hook that returns the same
logic and include setIsUserMenuOpen and userMenuTriggerButtonRef in the
dependency array; do the same for any analogous handlers in this file (the other
functions mentioned around lines 16-43) so ESLint exhaustive-deps warnings are
resolved and effects can safely list these callbacks as dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2795c37-1264-4ed2-9da7-83170b645f41
📒 Files selected for processing (4)
frontend/src/domains/auth/components/UserMenuButton/UserMenuButton.styles.tsfrontend/src/domains/auth/components/UserMenuButton/UserMenuButton.tsxfrontend/src/domains/auth/components/UserMenuButton/__tests__/UserMenuButton.test.tsxfrontend/src/domains/auth/hooks/useUserMenuVisibility.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #1096
As-Is
Escape입력으로도 닫히지 않아, 버튼을 다시 눌러야만 종료 가능To-Be
Escape입력 시 메뉴가 닫히도록 개선useUserMenuVisibility훅으로 분리button으로 감싸고aria-expanded,aria-haspopup,aria-controls속성 추가Escape로 닫힌 경우 트리거 버튼으로 포커스가 돌아가도록 처리Check List
Test Screenshot
(Optional) Additional Description
Escape닫기를 사용자 메뉴 전용 훅으로 분리해, 이후 유사한 오버레이 UI에도 재사용할 수 있도록 정리했습니다.Escape로 닫는 경우에만 적용했습니다. 외부 클릭 시에는 사용자가 실제로 클릭한 요소의 포커스를 유지하는 쪽이 자연스럽다고 판단했습니다.Escape입력 시 닫힘 및 포커스 복귀Escape외 키 입력 무시Escape입력 무영향Summary by CodeRabbit
릴리스 노트
개선 사항
테스트