UI/UX 컴포넌트 마이그레이션 결함 해결 및 라우팅 보완 - #176
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Top-level layout wiring frontend/src/app/layout.tsx |
Import DashboardLayout and wrap <body> children with <DashboardLayout>{children}</DashboardLayout>. |
Page mobile rendering changes frontend/src/app/page.tsx |
Remove page-level DashboardLayout wrapper; replace mobileView-driven UI with isDesktopWorkspace and selectedEmail state, add mobile back button and inbox/detail conditional panes; hide mobile NetworkGraph via showMobileActions = false. |
DashboardLayout mobile navigation refactor frontend/src/components/DashboardLayout.tsx |
Remove exported MobileWorkspaceView type and mobileView/onMobileViewChange props; render mailNavItems as Next.js Links for both expanded workspace menu and bottom nav, compute active state via isActivePath(pathname, href), and close workspace menu on link click. |
Test updates for mobile nav markup frontend/src/components/DashboardLayout.test.tsx |
Change mobile nav selector from button to a, update expected mobile nav label array, insert console.log of mobile nav HTML, and assert #mobile-workspace-menu contains "Prompt Studio". |
Sequence Diagram
sequenceDiagram
participant Router
participant AppLayout
participant DashboardLayout
participant Page(Home)
Router->>AppLayout: render page
AppLayout->>DashboardLayout: wrap children in <body>
DashboardLayout->>Page(Home): provide navigation UI (mailNavItems links)
Page(Home)->>DashboardLayout: navigate via Link (pathname changes)
DashboardLayout->>DashboardLayout: compute active state via isActivePath
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- Seongho-Bae/naruon#171: Related DashboardLayout navigation item and active-state logic changes using pathname and mailNavItems.
- Seongho-Bae/naruon#173: Overlaps test updates for DashboardLayout mobile navigation labels and "Prompt Studio" expectation.
Poem
🐰 I hopped through anchors, links, and light,
Wrapped the body, made the mobile right,
"Prompt Studio" appeared in view,
Tests now match the nav that's new,
A tiny thump for UI delight.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title in Korean refers to 'UI/UX component migration defect resolution and routing enhancement,' which directly aligns with the PR's core objectives: fixing DashboardLayout mobile navigation issues, elevating the layout to app/layout.tsx for global routing, and resolving UI/UX bugs from the frontend redesign. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feature/ui-ux-polish-20260512
Comment @coderabbitai help to get the list of available commands and usage tips.
|
@coderabbitai approve |
1 similar comment
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)
frontend/src/components/DashboardLayout.test.tsx (1)
39-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMobile menu labels are asserted from the wrong DOM scope/state.
At Line 39, labels are collected from
mobileNavbefore expansion, and CI shows this resolves to[], which breaks Line 54. Assert link labels from#mobile-workspace-menuafter clicking the menu button.Proposed test fix
- const mobileNavButtons = Array.from(mobileNav?.querySelectorAll('a') ?? []).map( - (button) => button.textContent, - ); @@ - expect(mobileNavButtons).toEqual(["받은 메일", "AI Hub", "Prompt Studio", "워크스페이스 설정"]); + expect(mobileNav?.textContent ?? "").toContain("워크스페이스"); @@ act(() => { mobileMenuButton?.click(); }); expect(mobileMenuButton?.getAttribute("aria-expanded")).toBe("true"); - expect(container.querySelector('#mobile-workspace-menu')?.textContent ?? "").toContain("Prompt Studio"); + const mobileMenuLinks = Array.from( + container.querySelectorAll('#mobile-workspace-menu a'), + ).map((link) => link.textContent?.trim() ?? ""); + expect(mobileMenuLinks).toEqual(["받은 메일", "AI Hub", "Prompt Studio", "워크스페이스 설정"]); + expect(mobileMenuLinks).toContain("Prompt Studio");🤖 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 `@frontend/src/components/DashboardLayout.test.tsx` around lines 39 - 54, The test collects mobile menu labels from mobileNav before the menu is expanded, causing mobileNavButtons to be empty; update the test to click the mobileMenuButton (mobileMenuButton) to open the menu, wait for aria-expanded to be true, then query the menu container by id '#mobile-workspace-menu' (instead of mobileNav) and collect its anchor textContent for assertion (mobileNavButtons) so the labels reflect the expanded state.
🤖 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.
Outside diff comments:
In `@frontend/src/components/DashboardLayout.test.tsx`:
- Around line 39-54: The test collects mobile menu labels from mobileNav before
the menu is expanded, causing mobileNavButtons to be empty; update the test to
click the mobileMenuButton (mobileMenuButton) to open the menu, wait for
aria-expanded to be true, then query the menu container by id
'#mobile-workspace-menu' (instead of mobileNav) and collect its anchor
textContent for assertion (mobileNavButtons) so the labels reflect the expanded
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0ff0549-d64a-4c3b-8285-6cb32395b2c6
📒 Files selected for processing (1)
frontend/src/components/DashboardLayout.test.tsx
✅ Actions performedComments resolved and changes approved. |
Greptile Summary
Confidence Score: 5/5P2 수준의 정리 항목만 존재하므로 머지 자체에 리스크는 없음. P0/P1 결함 없음. 주요 지적 사항(flex+hidden 충돌, 테스트 단언 불일치)은 이전 스레드에서 이미 논의됨. 이번 추가 발견은 console.log 잔류, aiNavItems 고아 프로퍼티, type="button" 누락으로 모두 P2. frontend/src/components/DashboardLayout.test.tsx (console.log 제거 필요), frontend/src/components/DashboardLayout.tsx (aiNavItems 고아 프로퍼티 정리 권장) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["app/layout.tsx (Server Component)"]
A -->|"글로벌 래퍼로 격상"| B["DashboardLayout (Client Component)"]
B --> C["Desktop Sidebar\n(mailNavItems → NavLink)"]
B --> D["Mobile Header\n(Menu Button → Overlay)"]
B --> E["Mobile Bottom Nav\n(mailNavItems → Link)"]
B --> F["main#main-content\n{children}"]
F --> G["page.tsx (/)"]
G -->|"selectedEmail === null"| H["모바일: 받은편지함 목록"]
G -->|"selectedEmail !== null"| I["모바일: 메일 상세\n+ ← 목록으로 버튼"]
D -->|"클릭"| J["#mobile-workspace-menu Overlay\n(mailNavItems → Link)"]
E -->|"Link 라우팅"| K["/, /ai-hub,\n/prompt-studio, /settings"]
Reviews (5): Last reviewed commit: "fix(frontend): refine mobile view state ..." | Re-trigger Greptile |
|
@coderabbitai approve |
1 similar comment
|
@coderabbitai approve |
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 (2)
frontend/src/components/DashboardLayout.tsx (1)
60-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMatch active state for nested routes too.
The exact
pathname === hrefcheck only highlights the top-level pages. Routes like/settings/profileor/ai-hub/historywill lose their active state in the sidebar and both mobile navs.Possible fix
+function isActivePath(pathname: string, href: string) { + return href === '/' + ? pathname === '/' + : pathname === href || pathname.startsWith(`${href}/`); +} + function NavLink({ label, description, icon: Icon, href = '#main-content', @@ }) { const pathname = usePathname(); - const active = pathname === href; + const active = isActivePath(pathname, href); @@ <nav aria-label="Mobile workspace menu" className="grid gap-2"> {mailNavItems.map(({ label, description, icon: Icon, href }) => { - const active = pathname === href; + const active = isActivePath(pathname, href); return ( @@ <nav aria-label="Mobile workspace sections" className="fixed inset-x-3 bottom-3 z-40 grid grid-cols-4 rounded-3xl border border-border bg-card/95 p-2 shadow-[0_18px_50px_rgba(15,23,42,0.14)] backdrop-blur-xl lg:hidden"> {mailNavItems.map(({ label, icon: Icon, href }) => { - const active = pathname === href; + const active = isActivePath(pathname, href); return (Also applies to: 211-218, 232-239
🤖 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 `@frontend/src/components/DashboardLayout.tsx` around lines 60 - 61, The active-state check uses strict equality (const active = pathname === href) which fails for nested routes; change the logic in the places using pathname and active (where usePathname() is called, e.g., the NavItem/dashboard link code in DashboardLayout.tsx) to treat a link as active when pathname === href OR pathname starts with the href plus a slash (e.g., pathname.startsWith(href + '/')), and handle the root case so "/" doesn't incorrectly match everything; update all occurrences mentioned (around lines 60–61, 211–218, 232–239) to use this starts-with approach to include nested routes in the active state.frontend/src/app/page.tsx (1)
20-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the blank first paint on mobile.
Phones initially take the desktop branch because
isDesktopWorkspacestarts astrue, but that branch is alsohiddenbelowlg. The result is an empty shell until the media-query effect runs.Possible fix
-function getDesktopWorkspaceMatch() { - if (typeof window === 'undefined') return true; - return window.matchMedia('(min-width: 1024px)').matches; -} - export default function Home() { const [selectedEmail, setSelectedEmail] = useState<number | null>(null); - - const [isDesktopWorkspace, setIsDesktopWorkspace] = useState(true); const showMobileActions = false; // network graph hidden on mobile inbox view for simplicity const handleSelectEmail = (emailId: number) => { setSelectedEmail(emailId); - }; - - useEffect(() => { - const mediaQuery = window.matchMedia('(min-width: 1024px)'); - const handleChange = () => setIsDesktopWorkspace(getDesktopWorkspaceMatch()); - - handleChange(); - mediaQuery.addEventListener('change', handleChange); - return () => mediaQuery.removeEventListener('change', handleChange); - }, []); return ( <> - {isDesktopWorkspace ? ( - <ResizablePanelGroup orientation="horizontal" className="hidden h-full items-stretch rounded-3xl border border-border/80 bg-card/70 shadow-[0_24px_80px_rgba(15,23,42,0.08)] backdrop-blur-xl lg:flex"> + <ResizablePanelGroup orientation="horizontal" className="hidden h-full items-stretch rounded-3xl border border-border/80 bg-card/70 shadow-[0_24px_80px_rgba(15,23,42,0.08)] backdrop-blur-xl lg:flex"> ... - </ResizablePanelGroup> - ) : ( - <div className="h-full overflow-hidden rounded-3xl border border-border/80 bg-card/70 shadow-[0_24px_80px_rgba(15,23,42,0.08)] backdrop-blur-xl"> + </ResizablePanelGroup> + <div className="h-full overflow-hidden rounded-3xl border border-border/80 bg-card/70 shadow-[0_24px_80px_rgba(15,23,42,0.08)] backdrop-blur-xl lg:hidden"> ... - </div> - )} + </div> </> ); }Also applies to: 37-68
🤖 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 `@frontend/src/app/page.tsx` around lines 20 - 34, The blank first paint happens because isDesktopWorkspace is initialized to true; change its initialization to use the actual media-query result when available by calling getDesktopWorkspaceMatch() (or checking window.matchMedia) during initialization so the client immediately reflects mobile/desktop; ensure you guard access with typeof window !== 'undefined' for SSR (use getDesktopWorkspaceMatch() when window exists, otherwise keep the current fallback), and keep the existing useEffect and setIsDesktopWorkspace handling (refs: isDesktopWorkspace, setIsDesktopWorkspace, getDesktopWorkspaceMatch).
🧹 Nitpick comments (1)
frontend/src/app/page.tsx (1)
21-21: ⚡ Quick winDon’t mount
NetworkGraphwhen the mobile panel is disabled.
showMobileActionsis hard-coded tofalse, but the section still renders and mountsNetworkGraph; only the CSS changes. That still pays the bundle/init cost for UI that users can’t open.Possible fix
- <section - aria-label="모바일 AI 실행" - className={`h-full ${showMobileActions ? 'flex' : 'hidden'} flex-col bg-gradient-to-b from-primary/5 via-background to-emerald-500/5 p-4`} - > - <div className="mb-4 rounded-2xl border border-primary/15 bg-card p-4 shadow-sm"> - <div className="flex items-center gap-2"> - <span className="grid size-9 place-items-center rounded-xl bg-primary/10 text-primary"> - <Network className="size-4" aria-hidden="true" /> - </span> - <div> - <h3 className="font-bold text-sm text-foreground">맥락 그래프</h3> - <p className="text-xs text-muted-foreground">메일과 관계의 흐름을 시각화합니다.</p> - </div> - </div> - </div> - <div className="min-h-0 flex-1 overflow-hidden rounded-2xl border border-border bg-card shadow-sm"> - <NetworkGraph /> - </div> - </section> + {showMobileActions ? ( + <section + aria-label="모바일 AI 실행" + className="h-full flex flex-col bg-gradient-to-b from-primary/5 via-background to-emerald-500/5 p-4" + > + <div className="mb-4 rounded-2xl border border-primary/15 bg-card p-4 shadow-sm"> + <div className="flex items-center gap-2"> + <span className="grid size-9 place-items-center rounded-xl bg-primary/10 text-primary"> + <Network className="size-4" aria-hidden="true" /> + </span> + <div> + <h3 className="font-bold text-sm text-foreground">맥락 그래프</h3> + <p className="text-xs text-muted-foreground">메일과 관계의 흐름을 시각화합니다.</p> + </div> + </div> + </div> + <div className="min-h-0 flex-1 overflow-hidden rounded-2xl border border-border bg-card shadow-sm"> + <NetworkGraph /> + </div> + </section> + ) : null}Also applies to: 91-109
🤖 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 `@frontend/src/app/page.tsx` at line 21, showMobileActions is hard-coded false but NetworkGraph is still mounted (paying bundle/init cost); update the render logic so NetworkGraph is only instantiated when showMobileActions is true (use a conditional render/short-circuit around the JSX that includes NetworkGraph) and apply the same conditional to the other block that renders the mobile panel (the section currently rendering NetworkGraph at the second occurrence). Target the showMobileActions flag and the NetworkGraph component in your changes so the component is not mounted when the mobile actions panel is disabled.
🤖 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.
Outside diff comments:
In `@frontend/src/app/page.tsx`:
- Around line 20-34: The blank first paint happens because isDesktopWorkspace is
initialized to true; change its initialization to use the actual media-query
result when available by calling getDesktopWorkspaceMatch() (or checking
window.matchMedia) during initialization so the client immediately reflects
mobile/desktop; ensure you guard access with typeof window !== 'undefined' for
SSR (use getDesktopWorkspaceMatch() when window exists, otherwise keep the
current fallback), and keep the existing useEffect and setIsDesktopWorkspace
handling (refs: isDesktopWorkspace, setIsDesktopWorkspace,
getDesktopWorkspaceMatch).
In `@frontend/src/components/DashboardLayout.tsx`:
- Around line 60-61: The active-state check uses strict equality (const active =
pathname === href) which fails for nested routes; change the logic in the places
using pathname and active (where usePathname() is called, e.g., the
NavItem/dashboard link code in DashboardLayout.tsx) to treat a link as active
when pathname === href OR pathname starts with the href plus a slash (e.g.,
pathname.startsWith(href + '/')), and handle the root case so "/" doesn't
incorrectly match everything; update all occurrences mentioned (around lines
60–61, 211–218, 232–239) to use this starts-with approach to include nested
routes in the active state.
---
Nitpick comments:
In `@frontend/src/app/page.tsx`:
- Line 21: showMobileActions is hard-coded false but NetworkGraph is still
mounted (paying bundle/init cost); update the render logic so NetworkGraph is
only instantiated when showMobileActions is true (use a conditional
render/short-circuit around the JSX that includes NetworkGraph) and apply the
same conditional to the other block that renders the mobile panel (the section
currently rendering NetworkGraph at the second occurrence). Target the
showMobileActions flag and the NetworkGraph component in your changes so the
component is not mounted when the mobile actions panel is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14a165ce-3b2c-4c73-8a9c-577e5b154559
📒 Files selected for processing (4)
frontend/src/app/layout.tsxfrontend/src/app/page.tsxfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/DashboardLayout.test.tsx
✅ Actions performedComments resolved and changes approved. |
1 similar comment
✅ Actions performedComments resolved and changes approved. |
|
PR governance metadata gate is not ready for
|
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai approve |
1 similar comment
✅ Actions performedComments resolved and changes approved. |
|
PR governance metadata gate is not ready for
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/components/DashboardLayout.test.tsx (1)
54-54: ⚡ Quick winRemove temporary debug logging from the test.
console.loghere will pollute test output and make CI failures harder to scan.💡 Suggested patch
- console.log('MOBILENAV HTML:', mobileNav?.outerHTML);🤖 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 `@frontend/src/components/DashboardLayout.test.tsx` at line 54, Remove the temporary debug log in the test: delete the console.log('MOBILENAV HTML:', mobileNav?.outerHTML) call in DashboardLayout.test.tsx (where mobileNav is referenced) so tests no longer print debug output to stdout; if you need to assert structure instead, replace the log with an appropriate expect on mobileNav or use testing-library's debug only when actively troubleshooting.
🤖 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 `@frontend/src/app/page.tsx`:
- Around line 71-78: The visibility toggles use truthy checks on selectedEmail
(a numeric id), which fails when selectedEmail === 0; update the conditional
className logic to use explicit null/undefined checks (e.g., selectedEmail ==
null or selectedEmail !== null) so the inbox and detail panes render correctly;
locate usages in this component where selectedEmail is referenced for visibility
(the section wrapping EmailList and the "모바일 메일 상세" section) and replace the
ternary conditions that rely on truthiness with explicit comparisons to
null/undefined to determine 'block' vs 'hidden' or 'flex' vs 'hidden'.
- Around line 17-21: The component currently initializes isDesktopWorkspace to
true which can cause the desktop-only branch to render briefly on mobile; change
the initialization to derive the value from the client viewport (use a lazy
initializer that checks typeof window !== 'undefined' and
window.matchMedia('(min-width: 1024px)').matches) so the first paint on the
client reflects the actual breakpoint, and keep using setIsDesktopWorkspace in
your resize/media effect to update thereafter; update the Home component's
isDesktopWorkspace/setIsDesktopWorkspace initialization (and ensure
showMobileActions logic remains consistent) to avoid a desktop-first blank on
mobile.
In `@frontend/src/components/DashboardLayout.tsx`:
- Around line 243-246: When a bottom-nav Link is clicked the mobile workspace
overlay remains open; update the Link in DashboardLayout.tsx to close the
overlay on navigation by adding an onClick handler that calls the state updater
(e.g., setIsWorkspaceMenuOpen(false)) or invokes the existing close helper if
present (e.g., closeWorkspaceMenu()), ensuring this is applied to the Link
rendering (key={label} href={href} data-mobile-view={label}) so any navigation
from the fixed bottom nav will set isWorkspaceMenuOpen to false.
---
Nitpick comments:
In `@frontend/src/components/DashboardLayout.test.tsx`:
- Line 54: Remove the temporary debug log in the test: delete the
console.log('MOBILENAV HTML:', mobileNav?.outerHTML) call in
DashboardLayout.test.tsx (where mobileNav is referenced) so tests no longer
print debug output to stdout; if you need to assert structure instead, replace
the log with an appropriate expect on mobileNav or use testing-library's debug
only when actively troubleshooting.
🪄 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: cc14fd6b-6084-4e13-bc80-450059eb2ca9
📒 Files selected for processing (4)
frontend/src/app/layout.tsxfrontend/src/app/page.tsxfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsx
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
1 similar comment
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
목표
이전 프론트엔드 리디자인 작업 시
DashboardLayout내의 모바일 네비게이션과 전체 앱 라우팅이 누락되거나 각 페이지 접속 시 사이드바가 아예 보이지 않던(메뉴 사라짐 현상) 치명적 UI/UX 버그를 해결합니다.변경 사항
page.tsx에만 종속되어 있던DashboardLayout컴포넌트를app/layout.tsx의 글로벌 레이아웃으로 끌어올려, 어떠한 설정, AI Hub 등의 하위 메뉴로 이동하더라도 좌측 사이드바가 그대로 유지되도록 했습니다./ai-hub,/settings,/prompt-studio)로 Next.js Link 라우팅 되도록 매핑을 수정했습니다.관련 이슈
Summary by CodeRabbit
Updates
Tests