feat(frontend): @agenta/navigation — the sidebar's model leaves the app - #5866
feat(frontend): @agenta/navigation — the sidebar's model leaves the app#5866ardaerzin wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Action performedReview finished.
|
|
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 ChangesShared Navigation Extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OSSSidebar
participant useSidebarDynamicChildren
participant sidebarEntitySourcesAtom
participant gatedSidebarSource
participant SessionQueries
OSSSidebar->>useSidebarDynamicChildren: provide project URL and entity icon resolver
useSidebarDynamicChildren->>sidebarEntitySourcesAtom: subscribe to registered entities
sidebarEntitySourcesAtom->>gatedSidebarSource: resolve each gated source
gatedSidebarSource->>SessionQueries: read session query state when enabled
SessionQueries-->>gatedSidebarSource: return session data and status
gatedSidebarSource-->>sidebarEntitySourcesAtom: return source state
sidebarEntitySourcesAtom-->>useSidebarDynamicChildren: return keyed entity sources
useSidebarDynamicChildren-->>OSSSidebar: inject resolved dynamic children
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 |
![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/packages/agenta-navigation/src/dynamic/sessionsSource.ts (1)
63-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate label fallback.
toSidebarRefalready replaces an empty name with"Untitled session". The registry entry atweb/packages/agenta-navigation/src/dynamic/registry.tsline 79 repeats the same fallback withsession.name || "Untitled session". Keep the fallback in one place so the copy does not diverge.web/oss/src/components/Sidebar/dynamic/sessionsSource.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export only the session symbols.
export * from "@agenta/navigation"re-exports the whole navigation barrel from a module namedsessionsSource. Consumers of./sessionsSourcethen see unrelated symbols, and duplicate star exports across sibling shims can create ambiguous re-exports. The sibling shimssource.tsandstatus.tsuse named re-exports. Use the same pattern here. The same problem exists inweb/oss/src/components/Sidebar/dynamic/registry.ts.♻️ Proposed change
-export * from "`@agenta/navigation`" +export {sidebarSessionsListAtom, type SessionSidebarRef} from "`@agenta/navigation`"web/packages/agenta-navigation/src/dynamic/source.ts (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead
queryAtomonce.Lines 50-52 call
get(queryAtom)three times. Extract it into one local for clarity.♻️ Proposed change
atom((get) => ({ data: get(dataAtom), - isPending: get(queryAtom).isPending ?? false, - isError: get(queryAtom).isError ?? false, - error: get(queryAtom).error ?? null, - })) + ...(({isPending, isError, error}) => ({ + isPending: isPending ?? false, + isError: isError ?? false, + error: error ?? null, + }))(get(queryAtom)), + }))Alternatively, use a block body with
const query = get(queryAtom).web/packages/agenta-navigation/src/dynamic/registry.ts (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or track the commented-out entity registrations.
Lines 112-127 hold two commented-out entity definitions. Lines 3 and 6-7 hold the matching commented imports.
TESTSETS_SIDEBAR_KEYandEVALUATORS_SIDEBAR_KEYat lines 33-34 are exported but referenced only from that commented code. Dead code in a new shared package is hard to date. Delete it, or add a short TODO that names the follow-up.Do you want me to open an issue to track re-enabling the test set and evaluator entities?
Also applies to: 33-34, 112-127
web/packages/agenta-navigation/src/dynamic/useSidebarDynamicChildren.ts (1)
141-182: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache and effect logic reads correctly.
The memo stays pure and reads only the ref. The effect writes the cache after commit, so Strict Mode's double render cannot corrupt it. One note:
kindIconsits in the memo dependency list at line 166. The OSS binding passes a module-level function, so identity is stable. Document that callers must pass a stablekindIcon, or the memo recomputes on every render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 01254659-e79e-49cb-be67-a79de2ebc59e
📒 Files selected for processing (25)
web/oss/src/components/Sidebar/dynamic/registry.tsweb/oss/src/components/Sidebar/dynamic/sessionsSource.tsweb/oss/src/components/Sidebar/dynamic/source.tsweb/oss/src/components/Sidebar/dynamic/status.tsweb/oss/src/components/Sidebar/dynamic/types.tsweb/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.tsweb/oss/src/components/Sidebar/engine/types.tsweb/oss/src/components/Sidebar/engine/visibility.tsweb/oss/src/components/Sidebar/scopes/constants.tsweb/oss/src/lib/atoms/sidebar.tsweb/packages/agenta-navigation/eslint.config.mjsweb/packages/agenta-navigation/package.jsonweb/packages/agenta-navigation/src/constants.tsweb/packages/agenta-navigation/src/dynamic/index.tsweb/packages/agenta-navigation/src/dynamic/registry.tsweb/packages/agenta-navigation/src/dynamic/sessionsSource.tsweb/packages/agenta-navigation/src/dynamic/source.tsweb/packages/agenta-navigation/src/dynamic/status.tsweb/packages/agenta-navigation/src/dynamic/types.tsweb/packages/agenta-navigation/src/dynamic/useSidebarDynamicChildren.tsweb/packages/agenta-navigation/src/index.tsweb/packages/agenta-navigation/src/state.tsweb/packages/agenta-navigation/src/types.tsweb/packages/agenta-navigation/src/visibility.tsweb/packages/agenta-navigation/tsconfig.json
| } | ||
| return sources | ||
| }) | ||
| export * from "@agenta/navigation" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two OSS shims star-export the entire @agenta/navigation barrel. Both modules run export * from "@agenta/navigation" instead of re-exporting the symbols they own. Any local barrel that re-exports both modules with export * then sees every shared symbol twice, and ES module semantics drop the ambiguous names silently. The sibling shims source.ts and status.ts already use named re-exports.
web/oss/src/components/Sidebar/dynamic/registry.ts#L1-L1: replace the star export with named exports of the registry symbols, for exampleSIDEBAR_ENTITIES,sidebarEntitySourcesAtom,defineSidebarEntity, and the sidebar key constants.web/oss/src/components/Sidebar/dynamic/sessionsSource.ts#L1-L1: replace the star export with named exports ofsidebarSessionsListAtomandtype SessionSidebarRef.
📍 Affects 2 files
web/oss/src/components/Sidebar/dynamic/registry.ts#L1-L1(this comment)web/oss/src/components/Sidebar/dynamic/sessionsSource.ts#L1-L1
| import { | ||
| injectDynamicChildren, | ||
| resolveChildren, | ||
| useSidebarDynamicChildren as useSharedSidebarDynamicChildren, | ||
| type SidebarConfig, | ||
| } from "@agenta/navigation" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check workspace dependency wiring for the new navigation package.
fd -t f 'package.json' web -d 4 --exclude node_modules --exec sh -c 'echo "== $1"; jq -r ".name, (.dependencies//{}|keys|join(\", \")), (.peerDependencies//{}|keys|join(\", \"))" "$1"' _ {} \
| rg -n -A3 'agenta-navigation|`@agenta/oss`|`@agenta/navigation`'
echo "--- imports of `@agenta/navigation` ---"
rg -n '`@agenta/navigation`' web --glob '!**/node_modules/**'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -u
echo "== relevant package.json files =="
for f in web/oss/package.json web/packages/agenta-navigation/package.json; do
if [ -f "$f" ]; then
echo "== $f =="
cat "$f"
else
echo "MISSING $f"
fi
done
echo
echo "== targeted dependency/peer declarations =="
python3 - <<'PY'
import json, pathlib
files = [
"web/oss/package.json",
"web/packages/agenta-navigation/package.json"
]
for path in files:
p = pathlib.Path(path)
if not p.exists():
print(f"MISSING {path}")
continue
data = json.loads(p.read_text())
print(f"== {path} ==")
print("name:", data.get("name"))
for sec in ["dependencies", "peerDependencies", "devDependencies"]:
deps = data.get(sec) or {}
hits = {k: v for k, v in deps.items() if k in [
"`@agenta/navigation`", "`@agenta/oss`", "`@agenta/entities`",
"`@agenta/sessions`", "`@agenta/shared`", "jotai",
"jotai-tanstack-query", "react", "`@phosphor-icons/react`"
] or k.startswith("`@agenta/`") or k == "typescript"]
if hits:
print(sec, hits)
PY
echo
echo "== `@agenta/navigation` imports =="
rg -n --fixed-strings '`@agenta/navigation`' web --glob '!**/node_modules/**' | sed -n '1,120p'Repository: Agenta-AI/agenta
Length of output: 9341
Add @agenta/navigation to the OSS dependency list.
web/oss/package.json imports @agenta/navigation through web/oss/src/lib/atoms/sidebar.ts, but the package does not declare it. Add the workspace dependency so OSS installs without relying on root/host package hoisting.
| "dependencies": { | ||
| "@agenta/entities": "workspace:../agenta-entities", | ||
| "@agenta/sessions": "workspace:../agenta-sessions", | ||
| "@agenta/shared": "workspace:../agenta-shared", | ||
| "@phosphor-icons/react": "^2.1.10", | ||
| "jotai-tanstack-query": "^0.11.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "@tanstack/react-query": ">=5.0.0", | ||
| "jotai": ">=2.0.0", | ||
| "react": ">=18.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@tanstack/react-query": "5.100.9", | ||
| "@types/node": "^20.19.20", | ||
| "@types/react": "^19.0.10", | ||
| "jotai": "^2.15.0", | ||
| "react": "^19.0.0", | ||
| "typescript": "^5.9.3" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jq '{dependencies, peerDependencies, devDependencies}' \
web/packages/agenta-navigation/package.json
rg -n \
-g '*.ts' -g '*.tsx' \
'from "react"|`@phosphor-icons/react`|ReactNode|ComponentType|MouseEvent|createElement' \
web/packages/agenta-navigation/srcRepository: Agenta-AI/agenta
Length of output: 2987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- package files mentioning agenta-navigation ---\n'
rg -n '"`@agenta/navigation`"|"agenta-navigation"|agenta-navigation' web -g 'package.json' -g 'pnpm-lock.yaml' || true
printf '\n--- files importing `@agenta/navigation` ---\n'
rg -n 'from ["'\'']`@agenta/navigation`["'\'']|from ["'\'']`@agenta/navigation/`' web -g '*.ts' -g '*.tsx' || true
printf '\n--- files importing agenta-navigation src types ---\n'
rg -n 'from ["'\'']\.?/?packages/agenta-navigation/src/(types|dynamic/types)["'\'']|from ["'\''].+\\.\.*\\./packages/agenta-navigation/src/' web -g '*.ts' -g '*.tsx' || true
printf '\n--- package workspace top files ---\n'
for f in web/pnpm-workspace.yaml web/package.json web/packages/agenta-navigation/package.json; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,220p' "$f"
fi
doneRepository: Agenta-AI/agenta
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package files mentioning agenta-navigation ---'
rg -n '"`@agenta/navigation`"|"agenta-navigation"|agenta-navigation' web -g 'package.json' -g 'pnpm-lock.yaml' || true
printf '%s\n' '';
printf '%s\n' '--- files importing `@agenta/navigation` ---'
rg -n 'from ["'\'']`@agenta/navigation`["'\'']|from ["'\'']`@agenta/navigation/`' web -g '*.ts' -g '*.tsx' || true
printf '%s\n' '';
printf '%s\n' '--- files importing agenta-navigation src types ---'
rg -n 'from ["'\'']\.\/?packages/agenta-navigation/src/(types|dynamic/types)["'\'']|from ["'\''].+\\.\.*\\./packages/agenta-navigation/src/' web -g '*.ts' -g '*.tsx' || true
printf '%s\n' '';
printf '%s\n' '--- workspace/package snippets ---'
for f in web/pnpm-workspace.yaml web/package.json web/packages/agenta-navigation/package.json; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,220p' "$f"
fi
doneRepository: Agenta-AI/agenta
Length of output: 8583
Keep @agenta/navigation framework-neutral.
This package is private, but its public model is still React-coupled. The shared interfaces expose React rendering types, event types, and React-only dependencies; React rendering should stay in OSS/EE adapter packages.
web/packages/agenta-navigation/package.json#L17-L35: keep React dependencies out of the navigation model unless downstream exports require React as a consumer peer/dependency.web/packages/agenta-navigation/src/types.ts#L1-L83: remove React-specificReactNode,ComponentType, and ReactMouseEventcontract details from shared navigation types.web/packages/agenta-navigation/src/dynamic/types.ts#L1-L89: replaceReactNodeicon return callbacks with framework-neutral icon metadata or adapter callbacks.
📍 Affects 3 files
web/packages/agenta-navigation/package.json#L17-L35(this comment)web/packages/agenta-navigation/src/types.ts#L1-L83web/packages/agenta-navigation/src/dynamic/types.ts#L1-L89
| defineSidebarEntity<SessionSidebarRef>(MAIN_SIDEBAR_SCOPE_ID, SESSIONS_SIDEBAR_KEY, { | ||
| kind: "app", | ||
| icon: createElement(ChatsCircleIcon, {size: 14}), | ||
| listAtom: sidebarSessionsListAtom, | ||
| getLabel: (session) => session.name || "Untitled session", | ||
| // The link navigates to the owning agent; the click hands over WHICH session, since the | ||
| // playground has no way to read that from the route. | ||
| childPath: (session) => `/apps/${session.appId}/playground`, | ||
| getOnClick: (session) => () => { | ||
| if (!session.appId) return | ||
| getDefaultStore().set(pendingSessionOpenAtom, { | ||
| appId: session.appId, | ||
| sessionId: session.sessionId, | ||
| title: session.name ?? undefined, | ||
| }) | ||
| }, | ||
| getGroup: (session) => (session.pinned ? "Pinned" : null), | ||
| getIcon: (session) => | ||
| session.pinned | ||
| ? createElement(PushPinIcon, {size: 14, weight: "fill"}) | ||
| : createElement(CircleIcon, { | ||
| size: 10, | ||
| weight: session.alive ? "fill" : "regular", | ||
| }), | ||
| emptyLabel: "No sessions", | ||
| maxItems: 7, | ||
| showAllPath: "/sessions", | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A session with a null appId renders a link to /apps/null/playground.
SessionSidebarRef.appId is string | null. childPath at line 82 interpolates it without a guard. toSidebarRef in sessionsSource.ts only drops rows when sessionOpenTarget(row) returns falsy; it keeps rows whose target.appId is null. getOnClick guards with if (!session.appId) return, but resolveChildren still assigns link, so the row remains clickable and navigates to a broken route. Drop these rows in toSidebarRef, or narrow appId to string in SessionSidebarRef.
🐛 Proposed fix in web/packages/agenta-navigation/src/dynamic/sessionsSource.ts
const toSidebarRef = (row: SessionStream, pinned: Set<string>): SessionSidebarRef | null => {
const target = sessionOpenTarget(row)
- if (!target) return null
+ if (!target?.appId) return nullThen narrow the field:
export interface SessionSidebarRef extends SidebarEntityRef {
sessionId: string
- appId: string | null
+ appId: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defineSidebarEntity<SessionSidebarRef>(MAIN_SIDEBAR_SCOPE_ID, SESSIONS_SIDEBAR_KEY, { | |
| kind: "app", | |
| icon: createElement(ChatsCircleIcon, {size: 14}), | |
| listAtom: sidebarSessionsListAtom, | |
| getLabel: (session) => session.name || "Untitled session", | |
| // The link navigates to the owning agent; the click hands over WHICH session, since the | |
| // playground has no way to read that from the route. | |
| childPath: (session) => `/apps/${session.appId}/playground`, | |
| getOnClick: (session) => () => { | |
| if (!session.appId) return | |
| getDefaultStore().set(pendingSessionOpenAtom, { | |
| appId: session.appId, | |
| sessionId: session.sessionId, | |
| title: session.name ?? undefined, | |
| }) | |
| }, | |
| getGroup: (session) => (session.pinned ? "Pinned" : null), | |
| getIcon: (session) => | |
| session.pinned | |
| ? createElement(PushPinIcon, {size: 14, weight: "fill"}) | |
| : createElement(CircleIcon, { | |
| size: 10, | |
| weight: session.alive ? "fill" : "regular", | |
| }), | |
| emptyLabel: "No sessions", | |
| maxItems: 7, | |
| showAllPath: "/sessions", | |
| }), | |
| const toSidebarRef = (row: SessionStream, pinned: Set<string>): SessionSidebarRef | null => { | |
| const target = sessionOpenTarget(row) | |
| if (!target?.appId) return null |
| defineSidebarEntity<SessionSidebarRef>(MAIN_SIDEBAR_SCOPE_ID, SESSIONS_SIDEBAR_KEY, { | |
| kind: "app", | |
| icon: createElement(ChatsCircleIcon, {size: 14}), | |
| listAtom: sidebarSessionsListAtom, | |
| getLabel: (session) => session.name || "Untitled session", | |
| // The link navigates to the owning agent; the click hands over WHICH session, since the | |
| // playground has no way to read that from the route. | |
| childPath: (session) => `/apps/${session.appId}/playground`, | |
| getOnClick: (session) => () => { | |
| if (!session.appId) return | |
| getDefaultStore().set(pendingSessionOpenAtom, { | |
| appId: session.appId, | |
| sessionId: session.sessionId, | |
| title: session.name ?? undefined, | |
| }) | |
| }, | |
| getGroup: (session) => (session.pinned ? "Pinned" : null), | |
| getIcon: (session) => | |
| session.pinned | |
| ? createElement(PushPinIcon, {size: 14, weight: "fill"}) | |
| : createElement(CircleIcon, { | |
| size: 10, | |
| weight: session.alive ? "fill" : "regular", | |
| }), | |
| emptyLabel: "No sessions", | |
| maxItems: 7, | |
| showAllPath: "/sessions", | |
| }), | |
| export interface SessionSidebarRef extends SidebarEntityRef { | |
| sessionId: string | |
| appId: string |
| const inlineOpen = (get(sidebarOpenGroupsAtomFamily(scopeId)) ?? []).includes(parentKey) | ||
| const popupOpen = get(sidebarPopupGroupsAtomFamily(scopeId)).includes(parentKey) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Inconsistent nullish guard between the two group families.
Line 21 guards sidebarOpenGroupsAtomFamily(scopeId) with ?? []. Line 22 reads sidebarPopupGroupsAtomFamily(scopeId) without a guard and calls .includes directly. If either family can hold null or undefined, line 22 throws a TypeError. Apply the same guard to both, or drop it from both if the state atoms always hold arrays.
🛡️ Proposed change
- const popupOpen = get(sidebarPopupGroupsAtomFamily(scopeId)).includes(parentKey)
+ const popupOpen = (get(sidebarPopupGroupsAtomFamily(scopeId)) ?? []).includes(parentKey)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const inlineOpen = (get(sidebarOpenGroupsAtomFamily(scopeId)) ?? []).includes(parentKey) | |
| const popupOpen = get(sidebarPopupGroupsAtomFamily(scopeId)).includes(parentKey) | |
| const inlineOpen = (get(sidebarOpenGroupsAtomFamily(scopeId)) ?? []).includes(parentKey) | |
| const popupOpen = (get(sidebarPopupGroupsAtomFamily(scopeId)) ?? []).includes(parentKey) |
| // Drop hidden entries at every level, not just the section root, so nested hidden items | ||
| // never render, auto-open, or become selected. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one short implementation comment.
Lines 3-4 add a two-line comment for ordinary filtering logic. Replace it with one short line or remove it. 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
The ref is only built from a resolved sessionOpenTarget, whose appId is a UUID-checked string, so the `| null` was a widening that made childPath capable of emitting /apps/null/playground and forced a dead guard in the sessions entity's getOnClick. Narrow the type and drop the guard.
581d662 to
de72623
Compare
1 participant
The sidebar knew too much: which items exist, which are gated by edition, which project and
workspace you are in, and where a click should land. All of that lived in OSS components.
@agenta/navigationtakes the headless half — the nav item model, the edition and entitlementgates, project/workspace/org resolution, and the route helpers. No React tree, no antd, so
/mand EE can build a different rail on the same model.
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/auth; review only this lane's diff.