Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough사이드바 상태 로직을 Changes
Sequence Diagram(s)sequenceDiagram
participant User as "사용자"
participant Sidebar as "Sidebar\n(`Sidebar.tsx`)"
participant Hook as "useSidebar\n(`useSidebar.ts`)"
participant Router as "라우터\n(NavLink/navigate)"
participant SubMenu as "SubMenu / SidebarItem"
rect rgba(99,102,241,0.5)
User->>Sidebar: 클릭 / 호버 이벤트
end
Sidebar->>Hook: handleItemClick(id, hasChildren)
alt hasChildren
Hook->>Hook: openId 토글 (열림/닫힘)
Hook-->>Sidebar: isCollapsed, openId 상태 갱신
Sidebar->>SubMenu: open이면 하위항목 렌더
else leaf item (path)
Hook->>Router: navigate(path)
Hook-->>Sidebar: openId = null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/components/Sidebar/Sidebar.tsx`:
- Around line 99-105: The submenu currently only opens via mouse because the div
handling each item uses onMouseEnter/onMouseLeave; add corresponding onFocus and
onBlur handlers to the same element so keyboard users can open/close the submenu
(e.g., onFocus => isCollapsed && setOpenId(item.id), onBlur => isCollapsed &&
setOpenId(null)). Keep the isCollapsed guard, mirror the mouse behavior, and
also ensure the interactive element has appropriate keyboard
focusability/semantics (e.g., focusable container or inner button and ARIA like
aria-expanded/role as needed) so setOpenId and the submenu logic continue to
work with keyboard focus.
- Around line 106-135: The chevron <button> is being rendered inside the
interactive NavLink produced by SidebarItem, causing nested interactive elements
and accessibility issues; update SidebarItem usage so that items which show a
chevron render as a non-link (button) parent or expose a prop to switch
SidebarItem into a "button mode" (e.g., renderButtonInsteadOfLink) and move the
ChevronIcon button outside the NavLink, or alternatively pass an onToggle prop
into SidebarItem so it renders the chevron button as a sibling element rather
than a child; locate SidebarItem, its usage here (props: item, isCollapsed,
isOpen, className from getMainItemClass, onClick => handleItemClick) and the
chevron toggle logic that calls setOpenId, and change rendering to avoid placing
the <button> inside the link while preserving handleItemClick/onToggle behavior
and aria-label text.
In `@src/components/Sidebar/SidebarItem.tsx`:
- Around line 69-78: The button currently always sets aria-haspopup="true" which
incorrectly signals a popup for items without submenus; update the JSX in
SidebarItem (the button rendering that uses isOpen, onClick, className) to set
aria-haspopup conditionally based on the presence of children (e.g., use a
boolean like Boolean(item.children?.length) or !!item.children) and only include
or set aria-expanded when a popup exists (i.e., when item.children is present)
so footer actions (notifications/logout) are not exposed as popups.
In `@src/components/Sidebar/SubMenu.tsx`:
- Around line 26-34: The submenu currently renders NavLink for every item even
when child.path is falsy, causing unwanted hash navigation; update the rendering
in SubMenu: either filter items before the items.map to only include children
with a valid child.path, or inside the map conditionally render a non-link
element (e.g., a disabled button or span) for entries missing child.path,
ensuring you stop passing to={child.path ?? "#"} to NavLink and apply the same
styling via getSubItemClass (or a disabled variant) for those non-link items;
keep key={child.id} and render child.label consistently.
In `@src/hooks/sidebar/useSidebar.ts`:
- Around line 14-29: The collapse/toggle logic clears openId but leaves
lastPathRef unchanged so reopening doesn't restore the active submenu; update
toggleSidebar (which calls setIsCollapsed and setOpenId) to also reset
lastPathRef.current (or alternatively include openId in the useEffect dependency
and handle when openId is null) so that when toggling back the useEffect that
scans mainNav (in useEffect) will re-evaluate and setOpenId(activeParent.id) for
the current location.pathname; reference toggleSidebar, lastPathRef, setOpenId,
setIsCollapsed, useEffect, and mainNav when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Sidebar/Sidebar.tsxsrc/components/Sidebar/SidebarItem.tsxsrc/components/Sidebar/SubMenu.tsxsrc/hooks/sidebar/useSidebar.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/Sidebar/SidebarItem.tsx (1)
23-23:aria-haspopup/aria-expanded조건부 처리 반영 완료 ✅이전 리뷰에서 지적된
aria-haspopup무조건 노출 문제가hasChildren조건으로 올바르게 수정되었습니다.Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/SidebarItem.tsx` at line 23, The conditional for showing aria-haspopup was added via the hasChildren constant, but the same conditional must be applied at the other render site(s) that set aria attributes (the second location that previously unconditionally emitted aria-haspopup/aria-expanded). Update the JSX where the item button/link renders (the same component using hasChildren) to only include aria-haspopup and aria-expanded when hasChildren is true (use hasChildren ? { 'aria-haspopup': true, 'aria-expanded': isOpen } : {} or conditional props) so both places consistently guard those attributes.src/hooks/sidebar/useSidebar.ts (1)
26-33: 사이드바 접기 시lastPathRef초기화 반영 완료 ✅이전 리뷰에서 지적된 collapse 후 재펼침 시
openId가 복원되지 않던 문제가if (next) lastPathRef.current = ""처리를 통해 올바르게 수정되었습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/sidebar/useSidebar.ts` around lines 26 - 33, Sidebar collapse previously cleared state incorrectly so openId wasn't restored; to fix, in toggleSidebar ensure when computing next collapsed state you clear lastPathRef (lastPathRef.current = "") only on collapse (as already added) and still call setOpenId(null) to reset openId; verify that the expansion logic elsewhere reads lastPathRef to restore openId so that the current change to toggleSidebar (setIsCollapsed, lastPathRef, setOpenId) fully addresses the restore-on-expand flow.
🧹 Nitpick comments (3)
src/components/Sidebar/SidebarItem.tsx (2)
58-58:NavLinkclassName불필요한 함수 래핑 제거 검토
NavLink의classNameprop은 문자열(string)과 함수((props: { isActive: boolean }) => string | undefined) 모두 허용합니다. 현재 코드는isActive/isPending상태를 무시하고 항상 같은 문자열을 반환하는 함수 형태를 사용하고 있어, 불필요한 함수 인스턴스를 매 렌더마다 생성합니다. 문자열로 직접 전달하면 더 간결합니다.♻️ 개선 제안
- className={() => className} + className={className}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/SidebarItem.tsx` at line 58, The NavLink in SidebarItem.tsx is passing className as a no-op function (className={() => className}) which creates a new function each render and ignores isActive/isPending; change it to pass the string directly (use className={className}) so NavLink receives the static class string and no unnecessary function is allocated.
64-64: NavLink 브랜치의 불필요한 중첩div제거 검토
content내부에 이미flex items-center w-fulldiv가 포함되어 있는데, NavLink 브랜치에서만 동일한 클래스의<div>로 한 번 더 감싸고 있습니다. button 브랜치는{content}를 직접 렌더링하는 것과 불일치하며, 불필요한 DOM 노드를 추가합니다.♻️ 개선 제안
<NavLink to={item.path} className={className} onClick={(e) => { if (e.defaultPrevented) return; onClick(); }} > - <div className="flex w-full items-center">{content}</div> + {content} </NavLink>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/SidebarItem.tsx` at line 64, In SidebarItem.tsx the NavLink branch wraps {content} with an extra <div className="flex w-full items-center"> even though content already contains the same flex container, causing an unnecessary DOM node; remove that outer div in the NavLink branch so it renders {content} directly (making NavLink and button branches consistent) and ensure any props or event handlers previously on the removed wrapper are moved onto the inner element or the NavLink itself (look for the NavLink render path and the content variable in the SidebarItem component).src/hooks/sidebar/useSidebar.ts (1)
26-43:toggleSidebar,handleItemClickuseCallback적용 검토두 함수 모두 매 렌더마다 새로운 참조가 생성됩니다.
SidebarItem,SubMenu등의 자식 컴포넌트가React.memo로 최적화되더라도 props로 전달된 핸들러가 매번 교체되면 메모이제이션 효과가 없어집니다. 코딩 가이드라인에서 "useCallback, useMemo의 적절한 사용"을 강조하고 있으므로 검토를 권장합니다.♻️ 개선 제안
- const toggleSidebar = () => { + const toggleSidebar = useCallback(() => { setIsCollapsed((prev) => { const next = !prev; if (next) lastPathRef.current = ""; return next; }); setOpenId(null); - }; + }, []); - const handleItemClick = (id: string, hasChildren: boolean, path?: string) => { + const handleItemClick = useCallback((id: string, hasChildren: boolean, path?: string) => { if (hasChildren) { setOpenId((prev) => (prev === id ? null : id)); if (path) navigate(path); } else if (path) { navigate(path); setOpenId(null); } - }; + }, [navigate]);As per coding guidelines, "useCallback, useMemo의 적절한 사용."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/sidebar/useSidebar.ts` around lines 26 - 43, Wrap toggleSidebar and handleItemClick with useCallback to stableize their references so memoized children (SidebarItem, SubMenu) receive stable props; for toggleSidebar create a useCallback that calls setIsCollapsed(prev => { const next = !prev; if (next) lastPathRef.current = ""; return next; }) and setOpenId(null) with dependencies [setIsCollapsed, lastPathRef, setOpenId]; for handleItemClick create a useCallback that accepts (id: string, hasChildren: boolean, path?: string) and performs the existing logic (setOpenId toggle, navigate(path) and setOpenId(null)) with dependencies [navigate, setOpenId]; ensure dependency arrays include the exact stable setters/refs used.
🤖 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/components/Sidebar/SidebarItem.tsx`:
- Around line 59-65: The NavLink onClick currently calls onClick() which
triggers handleItemClick(id, false, path) and causes a duplicate navigate(path)
and submenu flicker; modify the click handling so NavLink clicks only perform
state cleanup (no navigate) — either pass a dedicated cleanup handler from
Sidebar.tsx to the NavLink onClick or add a flag/option (e.g., skipNavigate) to
handleItemClick to skip calling navigate when invoked from NavLink; update the
NavLink onClick to call that cleanup-only handler (or handleItemClick with
skipNavigate) and keep handleItemClick behavior unchanged for non-NavLink
triggers, ensuring setOpenId/null cleanup remains.
In `@src/hooks/sidebar/useSidebar.ts`:
- Around line 35-43: The flicker happens because handleItemClick navigates then
immediately calls setOpenId(null) for leaf items, which closes the submenu
before the route change effect reopens it; update handleItemClick to avoid
unconditional closing: change its signature or call site to provide the clicked
item's parentId (or otherwise obtain it), then only call setOpenId(null) when
openId is different from that parentId (i.e., navigating to a different parent);
otherwise skip setOpenId(null) and just navigate so the existing useEffect that
sets openId from activeParent can control the open state without a redundant
close/open cycle (adjust references to handleItemClick, setOpenId, navigate, and
activeParent accordingly).
---
Duplicate comments:
In `@src/components/Sidebar/SidebarItem.tsx`:
- Line 23: The conditional for showing aria-haspopup was added via the
hasChildren constant, but the same conditional must be applied at the other
render site(s) that set aria attributes (the second location that previously
unconditionally emitted aria-haspopup/aria-expanded). Update the JSX where the
item button/link renders (the same component using hasChildren) to only include
aria-haspopup and aria-expanded when hasChildren is true (use hasChildren ? {
'aria-haspopup': true, 'aria-expanded': isOpen } : {} or conditional props) so
both places consistently guard those attributes.
In `@src/hooks/sidebar/useSidebar.ts`:
- Around line 26-33: Sidebar collapse previously cleared state incorrectly so
openId wasn't restored; to fix, in toggleSidebar ensure when computing next
collapsed state you clear lastPathRef (lastPathRef.current = "") only on
collapse (as already added) and still call setOpenId(null) to reset openId;
verify that the expansion logic elsewhere reads lastPathRef to restore openId so
that the current change to toggleSidebar (setIsCollapsed, lastPathRef,
setOpenId) fully addresses the restore-on-expand flow.
---
Nitpick comments:
In `@src/components/Sidebar/SidebarItem.tsx`:
- Line 58: The NavLink in SidebarItem.tsx is passing className as a no-op
function (className={() => className}) which creates a new function each render
and ignores isActive/isPending; change it to pass the string directly (use
className={className}) so NavLink receives the static class string and no
unnecessary function is allocated.
- Line 64: In SidebarItem.tsx the NavLink branch wraps {content} with an extra
<div className="flex w-full items-center"> even though content already contains
the same flex container, causing an unnecessary DOM node; remove that outer div
in the NavLink branch so it renders {content} directly (making NavLink and
button branches consistent) and ensure any props or event handlers previously on
the removed wrapper are moved onto the inner element or the NavLink itself (look
for the NavLink render path and the content variable in the SidebarItem
component).
In `@src/hooks/sidebar/useSidebar.ts`:
- Around line 26-43: Wrap toggleSidebar and handleItemClick with useCallback to
stableize their references so memoized children (SidebarItem, SubMenu) receive
stable props; for toggleSidebar create a useCallback that calls
setIsCollapsed(prev => { const next = !prev; if (next) lastPathRef.current = "";
return next; }) and setOpenId(null) with dependencies [setIsCollapsed,
lastPathRef, setOpenId]; for handleItemClick create a useCallback that accepts
(id: string, hasChildren: boolean, path?: string) and performs the existing
logic (setOpenId toggle, navigate(path) and setOpenId(null)) with dependencies
[navigate, setOpenId]; ensure dependency arrays include the exact stable
setters/refs used.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Sidebar/Sidebar.tsx (1)
103-104: 접힌 상태 서브메뉴가 여전히 마우스 전용입니다
onMouseEnter/onMouseLeave만 있고onFocus/onBlur는 여전히 추가되지 않아 키보드 사용자는 접힌 상태에서 서브메뉴에 접근할 수 없습니다. 이전 리뷰에서 지적된 내용이 미반영 상태입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/Sidebar.tsx` around lines 103 - 104, The collapsed submenu currently only opens on pointer hover via the onMouseEnter/onMouseLeave handlers, so keyboard users cannot access it; add equivalent keyboard focus handlers by wiring onFocus to the same handler used in onMouseEnter (e.g., onFocus={() => isCollapsed && setOpenId(item.id)}) and onBlur to the same handler used in onMouseLeave (e.g., onBlur={() => isCollapsed && setOpenId(null)}), ensuring you reference the same isCollapsed, setOpenId, and item.id variables so focus and hover behave identically for accessibility.
🧹 Nitpick comments (2)
src/components/Sidebar/Sidebar.tsx (2)
129-134: ChevronIcon 회전에 트랜지션이 없습니다
rotate-0↔rotate-180전환에transition-transform클래스가 없어서 아이콘이 부드럽게 돌아가지 않고 즉시 변경됩니다. 사이드바 접기/펴기 버튼(line 78)과 동일하게transition-transform duration-300을 추가해 주세요.♻️ 수정 제안
<ChevronIcon className={[ - "h-3 w-3", + "h-3 w-3 transition-transform duration-300", isOpen ? "rotate-0" : "rotate-180", ].join(" ")} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/Sidebar.tsx` around lines 129 - 134, The ChevronIcon rotation lacks a CSS transition so toggling isOpen switches between rotate-0 and rotate-180 instantly; update the ChevronIcon's className concatenation (the JSX where <ChevronIcon ... /> is rendered) to include "transition-transform duration-300" alongside the existing "h-3 w-3" and conditional rotate classes so the icon animates smoothly when isOpen changes.
62-69: 로고 스팬에 중복된transition클래스가 있습니다
transition-opacity duration-300과transition-all ease-in-out이 동시에 선언되어 있는데, 둘 다 CSS의transition속성을 덮어씁니다. 결과적으로transition-all만 적용되고transition-opacity는 무의미해집니다.transition-all duration-300 ease-in-out으로 통일해 주세요.♻️ 수정 제안
- "text-[22px] font-semibold whitespace-nowrap bg-gradient-to-r from-logo-1 to-logo-2 bg-clip-text text-transparent transition-opacity duration-300 transition-all ease-in-out overflow-hidden", + "text-[22px] font-semibold whitespace-nowrap bg-gradient-to-r from-logo-1 to-logo-2 bg-clip-text text-transparent transition-all duration-300 ease-in-out overflow-hidden",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/Sidebar.tsx` around lines 62 - 69, The span rendering the logo (the element with className array in Sidebar.tsx, referencing isCollapsed) has duplicate transition classes so only the latter takes effect; update the class list to a single transition declaration by replacing the two entries "transition-opacity duration-300" and "transition-all ease-in-out" with one combined string like "transition-all duration-300 ease-in-out" so the intended duration and easing apply consistently to the span (the element containing "WhereYouAd").
🤖 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/components/Sidebar/Sidebar.tsx`:
- Around line 150-170: footerNav의 notifications/logout 항목에는 path가 없어
SidebarItem이 button으로 렌더링되지만 onClick은 현재 handleItemClick(item.id, false)만 호출해 실제
알림열기/로그아웃 동작이 실행되지 않습니다; fix: restore action handling by either (A) adding an
onAction callback field to footerNav items and passing item.onAction to
SidebarItem's onClick, or (B) in Sidebar.tsx update the onClick handler to
branch on item.id (e.g., 'notifications', 'logout') and call the appropriate
functions (notification open handler, logout routine) while still calling
useSidebar.handleItemClick to maintain openId state; update references:
footerNav, SidebarItem, handleItemClick, useSidebar, openId, and the
notification/logout handler functions accordingly.
---
Duplicate comments:
In `@src/components/Sidebar/Sidebar.tsx`:
- Around line 103-104: The collapsed submenu currently only opens on pointer
hover via the onMouseEnter/onMouseLeave handlers, so keyboard users cannot
access it; add equivalent keyboard focus handlers by wiring onFocus to the same
handler used in onMouseEnter (e.g., onFocus={() => isCollapsed &&
setOpenId(item.id)}) and onBlur to the same handler used in onMouseLeave (e.g.,
onBlur={() => isCollapsed && setOpenId(null)}), ensuring you reference the same
isCollapsed, setOpenId, and item.id variables so focus and hover behave
identically for accessibility.
---
Nitpick comments:
In `@src/components/Sidebar/Sidebar.tsx`:
- Around line 129-134: The ChevronIcon rotation lacks a CSS transition so
toggling isOpen switches between rotate-0 and rotate-180 instantly; update the
ChevronIcon's className concatenation (the JSX where <ChevronIcon ... /> is
rendered) to include "transition-transform duration-300" alongside the existing
"h-3 w-3" and conditional rotate classes so the icon animates smoothly when
isOpen changes.
- Around line 62-69: The span rendering the logo (the element with className
array in Sidebar.tsx, referencing isCollapsed) has duplicate transition classes
so only the latter takes effect; update the class list to a single transition
declaration by replacing the two entries "transition-opacity duration-300" and
"transition-all ease-in-out" with one combined string like "transition-all
duration-300 ease-in-out" so the intended duration and easing apply consistently
to the span (the element containing "WhereYouAd").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Sidebar/Sidebar.tsxsrc/components/Sidebar/SidebarItem.tsxsrc/hooks/sidebar/useSidebar.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Sidebar/SidebarItem.tsx
- src/hooks/sidebar/useSidebar.ts
jjjsun
reviewed
Feb 25, 2026
Collaborator
jjjsun
left a comment
jjjsun
left a comment
There was a problem hiding this comment.
P3: duration-300이 살짝 느린느낌이 있는것같아서 duration-200으로 변경하는건 어떨까요?
Seojegyeong
approved these changes
Feb 25, 2026
Collaborator
Seojegyeong
left a comment
Seojegyeong
left a comment
There was a problem hiding this comment.
P4: 확인했습니다! 고생하셨어요 :)
Seojegyeong
approved these changes
Feb 25, 2026
Collaborator
Collaborator
|
P4: 저는 둘다 괜찮은것같아서 시도해보는것도 좋은것 같아요! |
Collaborator
Author
|
저도 괜찮은 것 같아요! |
jjjsun
approved these changes
Feb 26, 2026
Seojegyeong
approved these changes
Feb 26, 2026
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
3 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 #51
✨ 변경사항
✏️ 작업 내용
사이드바의 유지보수성을 높이기 위해 로직을 분리하고, 전환 애니메이션과 디자인을 수정했습니다.
1. 코드 구조 개선
useSidebar커스텀 훅으로 사이드바의 열림/닫힘 및 경로 관련 로직 캡슐화SidebarItem,SubMenu로 파일 분리2. UX
3. Design
😅 미완성 작업
N/A
📢 논의 사항 및 참고 사항
로고 텍스트 디자인이 잘 생각나지 않아서 일단 사진과 같이 그라데이션 넣어놨습니다ㅠ 생각하시는 방향이나 디자인 있으시면 말해주시면 감사하겠습니다!
Summary by CodeRabbit
신규 기능
개선