도메인 모델 정렬 및 UI/UX 원안(Figma) 복구 (Issue #180) - #180
Conversation
…nd runner contexts
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Domain Model and Navigation Specification docs/engineering/domain-model-realignment.md |
Documents four bounded contexts and prescribes frontend navigation constraints tied to the Figma mock; explicitly disallows "Prompt Studio" and generic "Settings" from replacing core mail navigation. |
Sidebar Component Refactoring frontend/src/components/DashboardLayout.tsx |
Redefines lucide icon imports and nav data for Mail/AI Hub/Projects/Labels; makes NavLink.description optional; adjusts desktop link styling; adds a top “메일 작성” compose button, grouped nav sections (AI Hub BETA), active-state colored labels, and an “오늘의 인사이트” visualization card. |
Test Expectations Update frontend/src/components/DashboardLayout.test.tsx |
Updates tests to query desktop navigation by aria-label="Mail sections", narrows expected mobile nav labels to mail sections, removes assertions for Prompt Studio and workspace settings, and deletes a stray console.log. |
Settings Provider Edit & API Client
| Layer / File(s) | Summary |
|---|---|
Settings Provider Edit Plan & Dependency docs/plans/2026-05-12-settings-provider-edit.md, frontend/package.json |
Adds an implementation plan describing PUT/DELETE support and UI changes; adds @radix-ui/react-tabs dependency. |
Tabs UI wrapper frontend/src/components/ui/tabs.tsx |
Introduces typed forwardRef wrappers around Radix Tabs primitives and re-exports Tabs, TabsList, TabsTrigger, TabsContent. |
ApiClient PUT/DELETE frontend/src/lib/api-client.ts |
Adds put<T> and delete<T> methods using fetch, throwing on non-OK responses and safely handling empty/no-content responses. |
Settings page: edit/delete provider UX frontend/src/app/settings/page.tsx |
Refactors provider loading; adds editingId/isDeleting state; switches submit to PUT when editing and POST when creating; adds edit and delete actions with confirmation and refresh logic; moves access-denied rendering into the workspace-llm tab and introduces a Tabs-based layout. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Seongho-Bae/naruon#159: Backend LLM provider CRUD endpoints/models — client PUT/DELETE work here aligns with that backend surface.
- UAT 네비게이션 링크 라우팅 적용 #171: Overlapping edits to
DashboardLayout.tsxsidebar navigation structure and nav-item behavior. - Naruon 관리자 설정 화면 UI 도입 (T-005) #161: Prior Settings page provider-management UI — this PR extends that UI with edit/delete flows and API client support.
Poem
🐰 I hopped through Figma, nose alight,
I nudged the mail button to the top just right,
Labels wear tiny colored dots with flair,
Providers can be edited with care,
A rabbit cheers — the sidebar's bright!
🚥 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 clearly summarizes the main changes: domain model realignment and UI/UX recovery to match the Figma design (Issue #180), which aligns with the primary objectives of fixing design intent distortion and recovering the original design. |
| Linked Issues check | ✅ Passed | The pull request addresses all key coding objectives from Issue #180: domain model documentation, sidebar navigation restructuring with grouped nav items (mail, AI hub, projects, labels), removal of out-of-scope menu items, settings page enhancement with edit/delete provider functionality, and API client updates for PUT/DELETE operations. |
| Out of Scope Changes check | ✅ Passed | All changes are scoped to the objectives: domain documentation, dashboard sidebar redesign, settings page provider management, test updates, and necessary infrastructure changes (tabs component, API client methods). No unrelated or extraneous modifications are present. |
✏️ 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/domain-realignment-20260512
Comment @coderabbitai help to get the list of available commands and usage tips.
|
@coderabbitai approve |
1 similar comment
Greptile Summary
Confidence Score: 3/5P1 신규(fetchProviders await 누락) + P1 이월(grid-cols-4/aside overflow) 3건이 있어 머지 전 수정이 권장됩니다. 신규 P1(fetchProviders 미수신)과 이전 리뷰에서 지적된 P1 두 건(모바일 grid 불일치, aside 스크롤 누락)이 미수정 상태로 남아 있어 천장 4점에서 복수 P1로 인해 3점으로 조정합니다. frontend/src/app/settings/page.tsx (fetchProviders await 누락), frontend/src/components/DashboardLayout.tsx (grid-cols-4 vs 5 items, aside overflow-y-auto 누락) Important Files Changed
Reviews (4): Last reviewed commit: "build(deps): install radix tabs dependen..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
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.tsx (1)
332-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick win모바일 하단 네비 그리드 컬럼 수가 항목 수와 맞지 않습니다.
Line 332는
grid-cols-4인데 실제 항목은 5개(mailNavItems)라 한 칸이 줄바꿈되어 UI가 깨집니다.Proposed fix
- <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"> + <nav aria-label="Mobile workspace sections" className="fixed inset-x-3 bottom-3 z-40 grid grid-cols-5 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">🤖 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 332 - 344, The mobile bottom nav uses a fixed CSS class grid-cols-4 but mailNavItems contains 5 entries, causing a wrap; update the nav's grid column count (change the grid-cols-4 class to grid-cols-5 or compute it dynamically) in DashboardLayout.tsx so the container matches mailNavItems.length, ensuring the mapped Link items (from the mailNavItems map that uses isActivePath and setIsWorkspaceMenuOpen) render without wrapping.
🧹 Nitpick comments (1)
frontend/src/components/DashboardLayout.test.tsx (1)
48-66: ⚡ Quick win핵심 복구 요소인
메일 작성CTA 존재도 테스트에 포함하는 게 좋겠습니다.현재 테스트가 새 핵심 액션의 존재를 검증하지 않아 회귀를 놓칠 수 있습니다.
Proposed test addition
expect(sidebar?.textContent ?? "").toContain("Naruon"); expect(sidebar?.textContent ?? "").toContain("흐름을 건너, 더 나은 판단과 실행으로."); + expect(sidebar?.textContent ?? "").toContain("메일 작성");🤖 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 48 - 66, The test is missing an assertion to verify the new primary CTA "메일 작성" exists; update the DashboardLayout.test.tsx assertions (e.g., near checks that reference banner, mobileNav, mobileMenuButton, and banner?.textContent) to assert the presence of the "메일 작성" CTA by selecting the CTA node (or checking banner?.textContent/main?.textContent) and expecting it to contain "메일 작성" (or expecting the queried button element to not be null and have the expected text/attributes), so the test fails if the CTA is removed or renamed.
🤖 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/components/DashboardLayout.tsx`:
- Line 219: The bottom-alignment uses duplicate mt-auto which causes
unpredictable spacing between bottom widgets; in DashboardLayout remove the
redundant mt-auto so only one element controls push-to-bottom behavior (either
keep mt-auto on the single bottom wrapper div with className "mt-auto pt-6 px-3
pb-4" or remove mt-auto from the card component at the other occurrence), or
alternatively convert the parent flex column to use justify-between to space top
and bottom regions consistently; update the elements referenced (the div with
className "mt-auto pt-6 px-3 pb-4" and the card component that also has mt-auto)
so only one mechanism is used for bottom alignment.
- Around line 162-167: The "메일 작성" CTA in DashboardLayout currently has no click
handler; add an onClick that invokes a compose action: define a handler (e.g.,
handleCompose) inside the DashboardLayout component and either call your router
navigation (e.g., router.push('/compose') or navigate('/compose')) or toggle the
compose modal state (e.g., setIsComposeOpen(true)) from that handler, then
attach it to the button's onClick. Ensure you import/use the correct router hook
(useRouter/useNavigate) or the component state setter so the button actually
triggers navigation or opens the compose UI.
---
Outside diff comments:
In `@frontend/src/components/DashboardLayout.tsx`:
- Around line 332-344: The mobile bottom nav uses a fixed CSS class grid-cols-4
but mailNavItems contains 5 entries, causing a wrap; update the nav's grid
column count (change the grid-cols-4 class to grid-cols-5 or compute it
dynamically) in DashboardLayout.tsx so the container matches
mailNavItems.length, ensuring the mapped Link items (from the mailNavItems map
that uses isActivePath and setIsWorkspaceMenuOpen) render without wrapping.
---
Nitpick comments:
In `@frontend/src/components/DashboardLayout.test.tsx`:
- Around line 48-66: The test is missing an assertion to verify the new primary
CTA "메일 작성" exists; update the DashboardLayout.test.tsx assertions (e.g., near
checks that reference banner, mobileNav, mobileMenuButton, and
banner?.textContent) to assert the presence of the "메일 작성" CTA by selecting the
CTA node (or checking banner?.textContent/main?.textContent) and expecting it to
contain "메일 작성" (or expecting the queried button element to not be null and have
the expected text/attributes), so the test fails if the CTA is removed or
renamed.
🪄 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: 985e27f2-ecfc-4813-a4ab-ad72b320e123
📒 Files selected for processing (3)
docs/engineering/domain-model-realignment.mdfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsx
✅ Actions performedComments resolved and changes approved. |
…from greptile review
|
@coderabbitai approve |
2 similar comments
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
1 similar comment
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
2 similar comments
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
… bounds for BYOK and Runners
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 (2)
63-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTop-nav action label drifts from PR requirement.
The third action is
할 일 만들기, but the objective specifies의사결정 메모.Proposed fix
const headerActions = [ { label: '캘린더 반영', icon: CalendarDays }, { label: '답장 초안', icon: PenLine }, - { label: '할 일 만들기', icon: CheckCircle2 }, + { label: '의사결정 메모', icon: CheckCircle2 }, ];🤖 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 63 - 67, Update the headerActions array so the third item's label matches the PR requirement: change the third entry in headerActions (currently { label: '할 일 만들기', icon: CheckCircle2 }) to use label '의사결정 메모' (leave the icon CheckCircle2 unless otherwise specified).
329-330:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMobile bottom nav grid column count is mismatched with item count.
grid-cols-4is hardcoded, but 5 mail items are rendered, causing layout breakage on mobile.Proposed fix
- <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"> + <nav aria-label="Mobile workspace sections" className="fixed inset-x-3 bottom-3 z-40 grid grid-cols-5 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">Also applies to: 347-347
🤖 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 329 - 330, The mobile nav uses a fixed "grid-cols-4" while rendering mailNavItems (5 items), causing layout breakage; update the DashboardLayout component to use a column count that matches the rendered items (either change the hardcoded class "grid-cols-4" to "grid-cols-5" or compute the tailwind grid class from mailNavItems.length and apply it to the nav), and make the same change at the other occurrence that renders the mobile nav so both instances (the nav rendering using mailNavItems) stay in sync.
♻️ Duplicate comments (1)
frontend/src/components/DashboardLayout.tsx (1)
159-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCore CTA is still non-functional (
메일 작성).The compose button has no click behavior or navigation, so the primary action is dead.
Proposed fix
- <div className="px-3 pb-4"> - <button className="w-full bg-primary hover:bg-primary/90 text-primary-foreground font-bold rounded-lg py-2.5 px-4 flex items-center justify-center gap-2 transition-colors"> - <Edit3 className="w-4 h-4" /> - 메일 작성 - </button> - </div> + <div className="px-3 pb-4"> + <Link + href="/compose" + aria-label="메일 작성" + className="w-full bg-primary hover:bg-primary/90 text-primary-foreground font-bold rounded-lg py-2.5 px-4 flex items-center justify-center gap-2 transition-colors" + > + <Edit3 className="w-4 h-4" aria-hidden={true} /> + 메일 작성 + </Link> + </div>🤖 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 159 - 164, The primary CTA button in DashboardLayout ("메일 작성") is missing click behavior; add an onClick handler on that button to start the compose flow by either navigating to your compose route or opening the compose modal. Inside DashboardLayout, import and use your router (e.g. useNavigate or next/router/useRouter) or local state (e.g. [isComposeOpen, setIsComposeOpen]) and wire the button's onClick to router.push('/compose') or setIsComposeOpen(true); ensure any compose component or route (ComposeModal / ComposePage) is mounted or available so the button actually opens the composer.
🤖 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.tsx`:
- Around line 63-67: Update the headerActions array so the third item's label
matches the PR requirement: change the third entry in headerActions (currently {
label: '할 일 만들기', icon: CheckCircle2 }) to use label '의사결정 메모' (leave the icon
CheckCircle2 unless otherwise specified).
- Around line 329-330: The mobile nav uses a fixed "grid-cols-4" while rendering
mailNavItems (5 items), causing layout breakage; update the DashboardLayout
component to use a column count that matches the rendered items (either change
the hardcoded class "grid-cols-4" to "grid-cols-5" or compute the tailwind grid
class from mailNavItems.length and apply it to the nav), and make the same
change at the other occurrence that renders the mobile nav so both instances
(the nav rendering using mailNavItems) stay in sync.
---
Duplicate comments:
In `@frontend/src/components/DashboardLayout.tsx`:
- Around line 159-164: The primary CTA button in DashboardLayout ("메일 작성") is
missing click behavior; add an onClick handler on that button to start the
compose flow by either navigating to your compose route or opening the compose
modal. Inside DashboardLayout, import and use your router (e.g. useNavigate or
next/router/useRouter) or local state (e.g. [isComposeOpen, setIsComposeOpen])
and wire the button's onClick to router.push('/compose') or
setIsComposeOpen(true); ensure any compose component or route (ComposeModal /
ComposePage) is mounted or available so the button actually opens the composer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32db809c-7f95-4288-bdd9-f0e4d2f568a6
📒 Files selected for processing (3)
docs/engineering/domain-model-realignment.mdfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsx
|
@coderabbitai approve |
1 similar comment
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/app/settings/page.tsx (1)
38-55: ⚡ Quick winDeduplicate provider-fetch logic to one function.
Line 39-54 and Line 57-71 repeat the same fetch/error flow. Consolidating to one function will reduce maintenance drift.
Suggested patch
- useEffect(() => { - const fetchProvidersData = async () => { - try { - const data = await apiClient.get<LLMProvider[]>('/api/llm-providers'); - setProviders(data); - setError(null); - } catch (err: unknown) { - if (((err as Error).message || '').includes('403')) { - setError('워크스페이스(Organization) 관리자 권한이 필요합니다. 관리자 계정으로 로그인해주세요.'); - } else { - setError('데이터를 불러오는 데 실패했습니다.'); - } - } finally { - setLoading(false); - } - }; - void fetchProvidersData(); - }, []); + useEffect(() => { + void fetchProviders(); + }, []);Also applies to: 57-71
🤖 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/settings/page.tsx` around lines 38 - 55, The provider-fetch and error-handling logic is duplicated; extract it into a single reusable async function (e.g., fetchProvidersData) outside or above the two useEffect blocks and have both effects call that one function; ensure the extracted function performs the same actions (apiClient.get<LLMProvider[]>('/api/llm-providers'), setProviders, setError with the 403 branch, and setLoading(false) in finally) and update the callers to invoke it (retaining any void or async invocation pattern) so setProviders, setError, and setLoading are used only from the centralized implementation.
🤖 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/settings/page.tsx`:
- Around line 87-92: The conditional uses a truthy check on editingId so an id
of 0 will be treated as "create" instead of "edit"; change the branch that calls
apiClient.put(`/api/llm-providers/${editingId}`, payload) to run only when
editingId is explicitly not null (e.g. editingId !== null), leaving the
apiClient.post(payload) for the null case and still calling setEditingId(null)
after a successful PUT; update any typing if needed so editingId can be
distinguished as null versus a numeric id.
In `@frontend/src/lib/api-client.ts`:
- Around line 62-64: The thrown Error for failed fetches only includes
response.statusText (at the checks using response and endpoint around the
PUT/other request branches), so callers cannot inspect numeric HTTP codes like
403; update those throw sites to include response.status in the error message
and also attach the numeric status (and optionally statusText) as properties on
the Error object (e.g., error.status = response.status) so callers can reliably
branch on HTTP status codes; apply this change to both occurrences that
currently use `throw new Error(\`API ... failed: ${response.statusText}\`)`.
---
Nitpick comments:
In `@frontend/src/app/settings/page.tsx`:
- Around line 38-55: The provider-fetch and error-handling logic is duplicated;
extract it into a single reusable async function (e.g., fetchProvidersData)
outside or above the two useEffect blocks and have both effects call that one
function; ensure the extracted function performs the same actions
(apiClient.get<LLMProvider[]>('/api/llm-providers'), setProviders, setError with
the 403 branch, and setLoading(false) in finally) and update the callers to
invoke it (retaining any void or async invocation pattern) so setProviders,
setError, and setLoading are used only from the centralized implementation.
🪄 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: f5eea57e-366a-4f0a-b0b6-9242ae882f93
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
docs/plans/2026-05-12-settings-provider-edit.mdfrontend/package.jsonfrontend/src/app/settings/page.tsxfrontend/src/components/ui/tabs.tsxfrontend/src/lib/api-client.ts
✅ Files skipped from review due to trivial changes (1)
- docs/plans/2026-05-12-settings-provider-edit.md
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
목표
이전 구현(T-004, T-005, T-006)에서 발생한 기획 의도 왜곡(사이드바를 덮어버린 범용 설정/스튜디오 메뉴 등)을 바로잡고, 원래 제공된 디자인 시안(
uiux4.png)의 맥락으로 프론트엔드를 완벽하게 복구(Realignment)합니다.변경 사항
docs/engineering/에 등재했습니다.관련 이슈
Resolves: #180
Summary by CodeRabbit
New Features
Documentation
Tests