feat(frontend): @agenta/navigation-ui — the rail's renderers and switchers, antd-free - #5867
feat(frontend): @agenta/navigation-ui — the rail's renderers and switchers, antd-free#5867ardaerzin wants to merge 3 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
WalkthroughThe PR adds the ChangesMobile context and navigation
Shared navigation UI package
OSS sidebar migration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant NavDrawer
participant NavPanel
participant NavMenu
participant DrawerProjectSwitcher
NavDrawer->>NavPanel: render the responsive navigation panel
NavPanel->>NavMenu: provide Home and Sessions entries
NavPanel->>DrawerProjectSwitcher: provide active project and workspace data
NavMenu->>NavDrawer: report link navigation
DrawerProjectSwitcher->>NavDrawer: navigate or log out
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 |
✅ 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: 6
🧹 Nitpick comments (5)
web/mobile/src/components/AgentaLogo.tsx (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace hard-coded logo colors with supported theme values.
fill-[#1E1C1D]anddark:fill-[#F2F25C]bypass the theme token system. Use an approved semantic Tailwind color or a supportedvar(--ag-color*)value instead. Runpnpm lint-fixfromwebbefore committing.Source: Coding guidelines
web/mobile/src/components/ContentRail.tsx (1)
5-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten this implementation comment.
This comment describes ordinary layout behavior. Remove it or reduce it to one short line. Keep long comments only for surprising constraints.
Source: Coding guidelines
web/oss/src/components/Sidebar/engine/SidebarShell.tsx (1)
268-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a supported semantic color token for the sidebar background.
bg-[var(--ag-sidebar-bg)]bypasses the supported semantic-token contract. Replace it with an Ant Design semantic token, a Tailwind color utility, or a supportedvar(--ag-color*)value. Verify the replacement in both light and dark themes.As per coding guidelines, consume theme colors through semantic tokens, Tailwind color utilities, or supported
var(--ag-color*)variables. Based on learnings, validate color-token replacements in both appearances.Sources: Coding guidelines, Learnings
web/mobile/src/features/nav/DrawerProjectSwitcher.tsx (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove project fetching to an
atomWithQueryatom.This new API query uses
useQuerydirectly. Put the query in the appropriate mobile Jotai store and consume it with an atom hook. Keep the query key, stale time, and fetch function in that atom.As per coding guidelines, use
atomWithQuerywith TanStack Query for API data fetching.Source: Coding guidelines
web/packages/agenta-navigation-ui/src/SidebarSelectionButton.tsx (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the implementation comment.
This comment describes normal component behavior. Reduce it to one short line. Remove the migration history from the source file.
Proposed change
-/** - * The rail's selection trigger (org/project, workflow): avatar + label + caret, collapsing - * to the avatar alone with the rail. Ported from the OSS antd Button verbatim, styling and - * transitions included. - */ +/** Sidebar selection trigger for organization, project, and workflow. */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: 933c2c3a-a05e-413a-9dae-538b8c2a08f2
📒 Files selected for processing (39)
web/mobile/src/components/AgentaLogo.tsxweb/mobile/src/components/ContentRail.tsxweb/mobile/src/components/ui/sheet.tsxweb/mobile/src/features/context/ContextResolver.tsxweb/mobile/src/features/context/ProjectList.tsxweb/mobile/src/features/context/ProjectSwitcher.tsxweb/mobile/src/features/context/WorkspaceSelector.tsxweb/mobile/src/features/context/contextTarget.tsweb/mobile/src/features/context/states/SignedOutNotice.tsxweb/mobile/src/features/context/useBindProjectContext.tsweb/mobile/src/features/nav/AppShell.tsxweb/mobile/src/features/nav/DrawerProjectSwitcher.tsxweb/mobile/src/features/nav/NavDrawer.tsxweb/mobile/src/features/nav/NavPanel.tsxweb/mobile/src/features/nav/NavRail.tsxweb/mobile/src/features/nav/useMobileNavItems.tsxweb/mobile/tests/unit/contextTarget.test.tsweb/oss/src/components/Sidebar/components/ProjectOrgSwitcher/index.tsxweb/oss/src/components/Sidebar/components/SidebarBackButton.tsxweb/oss/src/components/Sidebar/components/SidebarSelectionButton.tsxweb/oss/src/components/Sidebar/components/SidebarSkeletonLoader.tsxweb/oss/src/components/Sidebar/components/WorkflowPicker.tsxweb/oss/src/components/Sidebar/engine/SidebarMenu.tsxweb/oss/src/components/Sidebar/engine/SidebarShell.tsxweb/oss/src/components/Sidebar/hooks/useDropdownItems/index.tsxweb/oss/src/components/Sidebar/hooks/useDropdownItems/types.d.tsweb/oss/src/components/Sidebar/hooks/useProjectOrgSwitcher.tsweb/oss/src/components/Sidebar/hooks/useWorkflowSwitcher.tsxweb/packages/agenta-navigation-ui/eslint.config.mjsweb/packages/agenta-navigation-ui/package.jsonweb/packages/agenta-navigation-ui/src/NamePromptModal.tsxweb/packages/agenta-navigation-ui/src/NavMenu.tsxweb/packages/agenta-navigation-ui/src/ProjectOrgSwitcher.tsxweb/packages/agenta-navigation-ui/src/SidebarBackButton.tsxweb/packages/agenta-navigation-ui/src/SidebarSelectionButton.tsxweb/packages/agenta-navigation-ui/src/SidebarSkeletonLoader.tsxweb/packages/agenta-navigation-ui/src/WorkflowPickerView.tsxweb/packages/agenta-navigation-ui/src/index.tsweb/packages/agenta-navigation-ui/tsconfig.json
💤 Files with no reviewable changes (7)
- web/oss/src/components/Sidebar/hooks/useDropdownItems/types.d.ts
- web/mobile/src/features/context/states/SignedOutNotice.tsx
- web/mobile/src/features/context/WorkspaceSelector.tsx
- web/mobile/src/features/context/ProjectSwitcher.tsx
- web/oss/src/components/Sidebar/engine/SidebarMenu.tsx
- web/oss/src/components/Sidebar/hooks/useDropdownItems/index.tsx
- web/mobile/src/features/context/ProjectList.tsx
| const query = useQuery({ | ||
| queryKey: ["mobile", "projects"], | ||
| queryFn: () => fetchProjects(), | ||
| enabled: !shortcut, | ||
| enabled: !stored, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate persisted context against current project access.
readLastContext() only validates stored JSON structure. If the stored project was deleted, access was revoked, or the session expired, /m/ skips fetchProjects(), redirects to the stale URL, and cannot redirect to /auth or select the first valid project.
web/mobile/src/features/context/ContextResolver.tsx#L27-L30: fetch project state even when persisted context exists, and wait for that result before redirecting.web/mobile/src/features/context/contextTarget.ts#L27-L29: returnshortcutonly when its workspace and project exist ingroups; otherwise continue with desktop continuity and first-project fallback.web/mobile/tests/unit/contextTarget.test.ts#L60-L77: add coverage for a persisted shortcut whose project is absent fromgroups.
📍 Affects 3 files
web/mobile/src/features/context/ContextResolver.tsx#L27-L30(this comment)web/mobile/src/features/context/contextTarget.ts#L27-L29web/mobile/tests/unit/contextTarget.test.ts#L60-L77
| // There is no picker page — switching lives in the drawer, so the resolver always lands | ||
| // somewhere and the drawer corrects it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove or shorten this comment.
The test name and assertion already describe the fallback behavior. This two-line comment exceeds the configured one-short-line limit.
Source: Coding guidelines
| <label className="text-xs text-colorTextSecondary">{label}</label> | ||
| <input | ||
| autoFocus | ||
| value={name} | ||
| placeholder={placeholder} | ||
| onChange={(event) => setName(event.target.value)} | ||
| className="box-border h-8 w-full rounded-md border border-solid border-colorBorder bg-colorBgContainer px-2 text-[13px] text-colorText outline-none focus:border-colorPrimary" | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -a package.json web -x jq -r '"\(.name): react=\(.dependencies.react // .devDependencies.react // "inherited")"'
rg -n --glob '*.{ts,tsx}' '\buseId\s*\(' web/packages/agenta-navigation-ui web/oss | head -n 30Repository: Agenta-AI/agenta
Length of output: 1405
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,120p' web/packages/agenta-navigation-ui/src/NamePromptModal.tsxRepository: Agenta-AI/agenta
Length of output: 2287
Associate the NamePromptModal label with the input.
The label has no htmlFor and the input has no id. Click the label to focus the input by generating a stable ID with useId, setting htmlFor on the label, and id on the input.
| onClick={(event) => { | ||
| event.stopPropagation() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow link clicks to reach the navigation host.
NavPanel uses the bubbled link click at web/mobile/src/features/nav/NavPanel.tsx Line 56 to call onNavigate. event.stopPropagation() prevents that callback, so NavDrawer stays open after inline link navigation. Remove the propagation stop after link handling.
| const FlyoutChildren = ({items, selectedKeys}: {items: NavItem[]; selectedKeys: string[]}) => ( | ||
| <> | ||
| {items.map((child) => | ||
| child.isPlaceholder ? ( | ||
| <DropdownMenuLabel key={child.key} className="text-xs text-colorTextTertiary"> | ||
| {child.title} | ||
| </DropdownMenuLabel> | ||
| ) : child.divider ? ( | ||
| <DropdownMenuSeparator key={child.key} /> | ||
| ) : ( | ||
| <DropdownMenuItem | ||
| key={child.key} | ||
| disabled={child.disabled} | ||
| className={clsx(selectedKeys.includes(child.key) && "font-medium")} | ||
| asChild={Boolean(child.link)} | ||
| onSelect={child.link ? undefined : () => child.onClick?.(undefined as never)} | ||
| > | ||
| {child.link ? ( | ||
| <Link | ||
| href={child.link} | ||
| className="!text-inherit no-underline" | ||
| target={isExternal(child.link) ? "_blank" : undefined} | ||
| rel={isExternal(child.link) ? "noopener noreferrer" : undefined} | ||
| onClick={(event) => child.onClick?.(event)} | ||
| > | ||
| {child.title} | ||
| </Link> | ||
| ) : ( | ||
| <span>{child.title}</span> | ||
| )} | ||
| </DropdownMenuItem> | ||
| ), | ||
| )} | ||
| </> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply controlled selection in flyout children.
FlyoutChildren does not receive onItemSelect. When a collapsed or vertical group contains a linked child, the child navigates directly instead of invoking controlled selection. It also ignores child.inert. Thread the selection callback into this component and apply the same inert and controlled-navigation rules as RowLabel.
| <button | ||
| type="button" | ||
| aria-label={item.title} | ||
| className={clsx( | ||
| "mx-auto flex cursor-pointer items-center rounded-md border-0 bg-transparent", | ||
| ROW_INTERACTIVE, | ||
| selected && ROW_SELECTED, | ||
| collapsed | ||
| ? "size-8 justify-center" | ||
| : "h-9 w-[94%] justify-start gap-2 px-3 text-xs", | ||
| )} | ||
| > | ||
| {item.icon} | ||
| {!collapsed ? <span>{item.title}</span> : null} | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable disabled group triggers.
A disabled group still renders an enabled button with ROW_INTERACTIVE. The user can open its flyout. Set disabled={item.disabled} and use ROW_DISABLED when the group is disabled.
RowLabel's link handler opened with an unconditional stopPropagation. It was dead defense — rowClickHandler already returns undefined whenever item.link is set, so the ancestor row it guarded against has no handler — and it broke the mobile nav drawer, which closes itself by listening for the link click on its way up (NavPanel's notifyOnNavigate). The three <Link> paths now share one linkClickHandler, so the flyout honors child.inert and routes through controlled selection exactly like an inline row instead of navigating past the host. A disabled group also stops opening its flyout: the trigger takes item.disabled and ROW_DISABLED, matching the leaf. NamePromptModal's label gets a useId htmlFor, giving the input an accessible name and a clickable label.
The root resolver was the only thing that could route a signed-out session to the sign-in page, and it is the one screen that never gets to: a remembered pair skipped the projects fetch entirely (enabled: !stored) and redirected on the first render, so the unauthenticated branch was dead for every returning user. They landed on a screen that could not load, with no way to sign in. AuthGate moves the verdict app-wide, next to ContextSync. It reads the same projects query key and staleTime every screen already uses, so it costs no request of its own, and it is off on /auth* so it cannot loop on the page it redirects to. The resolver keeps its fast path — a stored pair still forwards before any network — but the fetch now always runs, and selectContextTarget drops a shortcut the fetched tree no longer holds. "No longer holds" is gated on an explicit groupsLoaded flag rather than groups.length, because an empty groups means "not fetched yet" far more often than it means "no projects", and the fast path must survive that.
581d662 to
de72623
Compare
6d24e20 to
b904ca7
Compare
1 participant
The rendered half of the sidebar: the nav item renderers, the project and workspace switchers,
and the rail's own layout, rebuilt without antd so the mobile app can mount the same components.
Pairs with
@agenta/navigationbelow it — that lane owns the model, this one owns the pixels.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/navigation; review only this lane's diff.