refactor(frontend): SidebarShell and the logo move into @agenta/navigation-ui — one shell for desktop and mobile - #5878
refactor(frontend): SidebarShell and the logo move into @agenta/navigation-ui — one shell for desktop and mobile#5878ardaerzin wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesSidebar navigation migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NavDrawer
participant useMobileNavScope
participant SidebarShell
participant NavMenu
NavDrawer->>useMobileNavScope: create workspace/project navigation scope
NavDrawer->>SidebarShell: pass route, atoms, and scope
SidebarShell->>NavMenu: render scoped navigation items
NavMenu-->>SidebarShell: render grouped and bottom navigation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
9519345 to
892272f
Compare
632dab3 to
7125323
Compare
|
@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: 1
🧹 Nitpick comments (4)
web/packages/agenta-navigation/src/supportLinks.ts (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the explanatory comment to one short line.
These lines describe normal composition behavior. The implementation already shows this behavior. Keep only a short constraint comment if needed.
As per coding guidelines, “Keep in-code comments to at most one short line.”
Source: Coding guidelines
web/packages/agenta-navigation-ui/src/SidebarLogo.tsx (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace raw SVG fill colors with supported theme tokens.
fill-[#1E1C1D]anddark:fill-[#F2F25C]bypass the shared color system. Use a supported semantic token orvar(--ag-color*)value for both appearance states.As per coding guidelines, “do not use raw hex colors.”
Source: Coding guidelines
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the toggle behavior comment to one short line.
The JSX expresses the expanded and collapsed behavior. Keep a short comment only if a non-obvious constraint remains.
As per coding guidelines, “Keep in-code comments to at most one short line.”
Source: Coding guidelines
web/mobile/src/features/nav/useMobileNavItems.tsx (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce narrative comments to one short line.
These comments describe expected component structure. They do not document a surprising constraint.
web/mobile/src/features/nav/useMobileNavItems.tsx#L59-L63: Remove the block or replace it with one short constraint-focused comment.web/mobile/src/features/nav/NavDrawer.tsx#L20-L24: Remove the block or replace it with one short constraint-focused comment.web/mobile/src/features/nav/NavRail.tsx#L7-L11: Remove the block or replace it with one short constraint-focused comment.As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 14b98250-6119-47db-8207-8eadf0cac54b
📒 Files selected for processing (20)
web/mobile/src/features/nav/NavDrawer.tsxweb/mobile/src/features/nav/NavPanel.tsxweb/mobile/src/features/nav/NavRail.tsxweb/mobile/src/features/nav/mobileNavScope.tsxweb/mobile/src/features/nav/useMobileNavItems.tsxweb/oss/src/components/Sidebar/Sidebar.tsxweb/oss/src/components/Sidebar/components/SidebarLogo.tsxweb/oss/src/components/Sidebar/hooks/useWorkflowSwitcher.tsxweb/oss/src/components/Sidebar/scopes/bottomSection.tsxweb/oss/src/components/Sidebar/scopes/mainScope.tsxweb/packages/agenta-navigation-ui/package.jsonweb/packages/agenta-navigation-ui/src/NavMenu.tsxweb/packages/agenta-navigation-ui/src/SidebarLogo.tsxweb/packages/agenta-navigation-ui/src/SidebarShell.tsxweb/packages/agenta-navigation-ui/src/index.tsweb/packages/agenta-navigation/src/dynamic/registry.tsweb/packages/agenta-navigation/src/dynamic/useSidebarDynamicChildren.tsweb/packages/agenta-navigation/src/index.tsweb/packages/agenta-navigation/src/supportLinks.tsweb/packages/agenta-navigation/src/types.ts
💤 Files with no reviewable changes (2)
- web/oss/src/components/Sidebar/components/SidebarLogo.tsx
- web/mobile/src/features/nav/NavPanel.tsx
| const distinctGroups = new Set(visibleRefs.map((ref) => entity.getGroup?.(ref) ?? null)) | ||
| const groupsAreInformative = distinctGroups.size > 1 | ||
| const children: SidebarConfig[] = [] | ||
| let currentGroup: string | null = null | ||
| for (const ref of visibleRefs) { | ||
| // Headings are inserted, not sorted — an entity supplying `getGroup` must already order | ||
| // its refs by group, or the same heading would appear more than once. | ||
| const group = entity.getGroup?.(ref) ?? null | ||
| if (group && group !== currentGroup) { | ||
| if (groupsAreInformative && group && group !== currentGroup) { | ||
| children.push({ | ||
| key: `${entity.parentKey}-group-${group}`, | ||
| title: group, | ||
| icon: icon(), | ||
| disabled: true, | ||
| isDynamic: true, | ||
| isPlaceholder: true, | ||
| isGroupLabel: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not treat ungrouped entries as a separate labelled group.
Line 90 counts null as a distinct group. Lines 98-104 do not render a label for null. A provider that groups only some references can render one named heading above unlabelled entries.
Enable group labels only when every visible reference has a group label. Otherwise, add an explicit label for ungrouped entries.
…ation-ui — one shell for desktop and mobile
…t some distinctGroups counted the ungrouped null as a group, so a provider that labels only part of its list cleared the size > 1 bar and rendered one named heading above an unlabelled remainder — the stray row the sessions registry avoids by naming both halves.
892272f to
6f21584
Compare
7125323 to
a4bfc3d
Compare
1 participant
SidebarShelland the logo were the last piece of the rail still owned by the app. They moveinto
@agenta/navigation-ui, so desktop and/mmount the same shell instead of two that haveto be kept in sync by hand.
Not run in a browser — static gates only (
pnpm lint-fix24/24,tsc --noEmitcleanfor
@agenta/shared,ui,entities,entity-ui,settings-ui,oss,ee,mobile).Stacked on
pkg/entity-ui-drive; review only this lane's diff.