[feat]: app structure with agents#4987
Conversation
…ubmenu integration
…mance and clarity
…visibility handling
…ve navigation handling - Changed SidebarIslandProps to accept a SidebarView instead of showSettingsView and lastPath. - Refactored Sidebar component to resolve sidebar scope based on SidebarView. - Removed unused imports and cleaned up ListOfProjects component. - Updated dynamic sidebar registry to use scope IDs for better organization. - Enhanced gatedSidebarSource to accept scopeId as a parameter. - Simplified SidebarConfig interface by removing unnecessary properties. - Improved useSidebarConfig hook to return structured sidebar items. - Added constants for main and settings sidebar scope IDs. - Refactored settingsScope to utilize new settings tab management logic. - Introduced new navigation functions for settings tab resolution and visibility. - Added tests for settings tab resolution logic. - Updated projectSwitchHref to handle navigation context more effectively. - Cleaned up settings page to utilize new settings access logic. - Updated org hooks to use the new projectSwitchHref for navigation.
- Introduced AgentsPage and ArchivedAgentsPage components for managing agents. - Implemented agents selection logic with hooks for handling row selections. - Added sidebar entries for agents and prompts with appropriate icons. - Created store atoms for managing agents workflows and their states. - Enhanced ApplicationManagementSection to support agent-specific views. - Added tests for agent classification logic to ensure correct flag handling.
…rgs and ListOfProjects components
…ntroduce SidebarBackButton
…jects, WorkflowIdentity, and WorkflowPicker for improved responsiveness and animations
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI 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:
✨ Finishing Touches🧪 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 |
…uators from workflow switcher
…d improve timestamp handling
dosubot
Bot
added
size:XXL
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review — sidebar view-registry / agents app structure
This PR replaces the boolean-driven single Sidebar (settings-vs-app) with a view-registry + scope engine (viewRegistry → SidebarScope → SidebarShell/SidebarMenu), splits the settings/workflow/main sidebars into scopes, adds dynamic lazy-loaded entity groups, and introduces the agents app structure (pages, store, columns). The architecture is clean and most wiring checks out (unknown-view fallback, moved buildProjectSwitchHref, deleted-module importers, settings-tab permission gating, and pathMatchesLink boundary logic all verified fine).
A reviewer ran an 8-angle pass (correctness / removed-behavior / cross-file / reuse / simplification / efficiency / altitude / conventions) and verified each surviving finding against the actual PR-head files (b9e1b06). Findings below, most severe first. Two are behavior regressions vs the deleted code, one is a re-render/remount bug, the rest are altitude/efficiency/reuse cleanups.
Findings
Sidebar.tsx/Layout.tsx— workflow & settings header remount on every in-view navigation.sidebarView(Layout) allocates a fresh object on each pathname change;scope = useMemo(create, [view])then rebuilds the scope, andcreateWorkflowHeader/settings build a NEW header component identity per call, so<scope.header/>unmounts+remountsWorkflowPicker/back-button on every navigation. Defeats theSidebarIslandmemo.Layout.tsx— Settings "Back" skips the workflow you came from.lastBasePathRefonly records base (main-layer) paths; workflow routes are never captured, so Back lands on the last main page.useWorkflowSwitcher.tsx— evaluator mislabeled as Prompt/Agent when the evaluators list hasn't loaded (or is archived).isEvaluatoris derived fromnonArchivedEvaluatorsAtommembership instead ofcontext.workflowKind.workflowItems.ts— per-workflow filtering hardcodes the literal key"app-variants-link"with a?? truefall-through — renaming the key silently leaks Variants into evaluator sidebars.atoms/sidebar.ts/SidebarMenu.tsx— collapsed-flyout fetch gate can stick open (globalsidebarPopupGroupsAtom,onMouseEntersetstruewith no matching leave).SidebarShell.tsx—renderSectionre-runsfilterVisibleItemsun-memoized, duplicating theallItemswalk on every render.ListOfProjects.tsx/ListOfOrgs.tsx— the selection-button JSX is copy-pasted verbatim (the PR already extractedSidebarBackButtonfor this reason).useWorkflowSwitcher.tsx— switcher list narrowed to non-deterministic evaluators (documented as intended; confirm the current deterministic evaluator vanishing from its own switcher is acceptable).
Inline comments on each below.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/oss/src/components/pages/prompts/store.ts (1)
158-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFetch all workflow pages before filtering prompts.
queryWorkflowsis cursor-paginated, but this atom only consumes the first page (windowing.nextis ignored). After filtering out evaluator workflows, the prompts list can underfill and miss entries on later pages.web/oss/src/components/pages/app-management/store/appWorkflowStore.ts (1)
197-218: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAgent-scoped archived fetch triples an already-unbounded full-list scan.
fetchArchivedAppWorkflowsfetches the entire archived list and, foragentScope, bulk-fetches latest revisions for every workflow before filtering. This function is invoked independently by three separate atoms (paginatedfetchPage, total-count query, filtered-count query), each with its own query key/cache, so none of them share the fetched data. On top of the pre-existing "full-list-then-client-slice" pagination pattern, this PR adds a bulk revision fetch that now runs 3x for every mount/search-term change on the Archived Agents page, and again on every page-cursor advance sincefetchPagere-fetches the whole list each time.Consider having the count/total-count atoms derive from the paginated store's own fetched data (or share a single cached query) instead of independently re-fetching and re-filtering the full archived set each time.
Also applies to: 467-489, 510-531, 556-582
♻️ Duplicate comments (1)
web/oss/src/components/Sidebar/engine/SidebarMenu.tsx (1)
1-1: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winSame missing
Reactimport asengine/types.ts.
React.FC(Line 17),React.MouseEvent | React.KeyboardEvent(Line 42), andReact.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>(Lines 186-187) all reference theReactnamespace, but Line 1 only destructures named hooks from"react"—Reactis never imported. This is the sameTS2686UMD-global hazard flagged inengine/types.ts.🛠️ Proposed fix
-import {memo, useCallback, useMemo, useRef} from "react" +import {memo, useCallback, useMemo, useRef} from "react" +import type {FC, MouseEvent, KeyboardEvent} from "react"then replace
React.FC,React.MouseEvent,React.KeyboardEventwith the imported type names.Also applies to: 17-17, 42-42, 186-187
🧹 Nitpick comments (13)
web/oss/src/components/Sidebar/scopes/workflowItems.ts (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm
ITEM_WORKFLOW_SUPPORTcoverage is intentional.Only
"app-variants-link"is restricted by category; every other sidebar key defaults to visible for all categories (?? trueat line 33). If future workflow-specific items (agent-only or evaluator-only rows) are added without an entry here, they will silently show for all categories. Worth a short note or lint to keep this map in sync as new items are added.web/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.ts (1)
100-128: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffRef mutation during render (inside
useMemo).
cachedChildrenRef.currentis mutated in place while computing theuseMemovalue, which runs during the render phase. React documents this as a purity violation; under Strict Mode's double-invocation the callback runs twice per commit, and since the cache object is mutated (not replaced) both invocations act on the same object. The end result is currently idempotent given the logic, but this pattern is fragile against future edits (e.g., adding a mutation that isn't idempotent) and against React's rendering guarantees.Consider moving the caching into a
useEffect(after commit) or deriving the memo purely and updating the ref as a side effect in a separateuseEffect.web/oss/src/components/Sidebar/engine/SidebarShell.tsx (1)
14-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError boundary silently swallows render errors.
SidebarErrorBoundarycatches errors and renders an empty<div />with no logging, so a broken sidebar section fails silently with no diagnostic trail.♻️ Suggested addition
class SidebarErrorBoundary extends React.Component<React.PropsWithChildren, {hasError: boolean}> { state = {hasError: false} - static getDerivedStateFromError() { + static getDerivedStateFromError(error: Error) { + console.error("Sidebar section failed to render", error) return {hasError: true} }web/oss/src/components/Sidebar/scopes/viewRegistry.ts (1)
32-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BASE_VIEWderivation contradicts the maintenance comment above it.The comment says "Add a new full-sidebar view by appending one entry here," but
BASE_VIEW = SIDEBAR_VIEWS[SIDEBAR_VIEWS.length - 1]assumes the base (isBase: true) entry is always last. Literally following the comment's instructions (appending aftermainSidebarScope) would silently makeBASE_VIEWresolve to the wrong entry, breaking the fallback used byresolveSidebarView/getSidebarViewDefinition.♻️ Suggested fix
-const BASE_VIEW = SIDEBAR_VIEWS[SIDEBAR_VIEWS.length - 1] +const BASE_VIEW = SIDEBAR_VIEWS.find((view) => view.isBase) ?? SIDEBAR_VIEWS[SIDEBAR_VIEWS.length - 1]web/oss/src/components/pages/settings/assets/navigation.ts (1)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile cast masks a latent gap in
SETTINGS_LABELS.
SETTINGS_LABELSexcludes"billing", andgetSettingsTabLabelrelies on thebillingentry inSETTINGS_TABSalways defininggetLabelto avoid hitting the cast with a"billing"key. If a future edit ever removesgetLabelfrom thebillingdefinition (Line 44-47) or the lookup at Line 69 fails to find the tab, this silently returnsundefinedinstead of a compile error, since theas Exclude<...>cast bypasses type checking.Consider making
SETTINGS_LABELSaRecord<SettingsTabKey, string>(with a placeholder/derived billing label) or asserting non-null after the lookup instead of casting away the exclusion.web/oss/src/pages/w/[workspace_id]/p/[project_id]/settings/index.tsx (1)
72-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DEFAULT_SETTINGS_TABinstead of the hardcoded"workspace"fallback.
navigation.tsexportsDEFAULT_SETTINGS_TABfor exactly this purpose; hardcoding"workspace"here duplicates that value and can silently drift out of sync if the default ever changes.🔧 Proposed fix
-import { - getSettingsTabLabel, - resolveSettingsTab, - type SettingsAccess, -} from "`@/oss/components/pages/settings/assets/navigation`" +import { + DEFAULT_SETTINGS_TAB, + getSettingsTabLabel, + resolveSettingsTab, + type SettingsAccess, +} from "`@/oss/components/pages/settings/assets/navigation`" ... - const tab = tabQuery ?? settingsTab ?? "workspace" + const tab = tabQuery ?? settingsTab ?? DEFAULT_SETTINGS_TABweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx (2)
172-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMulti-line comments exceed the one-line guideline.
Both comment blocks span 3 lines each; per coding guidelines, comments should be kept to one short line unless a genuinely surprising constraint requires a brief exception. Consider condensing to a single concise line (or moving the rationale to a short docstring on the shared
isHiddenpredicate suggested below).As per coding guidelines, "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception."
Also applies to: 200-202
Source: Coding guidelines
164-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
isHiddencondition across all fiveappItemsentries.
!hasAppContext && !currentApp && !recentlyVisitedAppIdis repeated verbatim 5 times. Extracting it into a single memoized boolean would reduce duplication and make future changes to the visibility rule less error-prone.♻️ Suggested extraction
+ const isAppItemsHidden = !hasAppContext && !currentApp && !recentlyVisitedAppId + const appItems = useMemo<SidebarConfig[]>( () => [ { key: "overview-link", title: "Overview", link: `${appURL || recentlyVisitedAppURL}/overview`, icon: <DesktopIcon size={14} />, - isHidden: !hasAppContext && !currentApp && !recentlyVisitedAppId, + isHidden: isAppItemsHidden, disabled: !hasProjectURL, }, ...web/oss/src/components/Sidebar/components/ListOfOrgs.tsx (1)
304-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
renderSelectionButtonis duplicated near-verbatim inListOfProjects.tsx.The button markup, transition classes,
InitialsAvatar, label span, and caret span (lines 304-352 here) are essentially identical toListOfProjects.tsxlines 198-246. Consider extracting a sharedSidebarSelectionButtoncomponent (label, placeholder, isOpen, showCaret, collapsed, disabled, sharedButtonProps) to avoid maintaining two copies of the same collapse/expand transition logic.♻️ Suggested extraction sketch
// web/oss/src/components/Sidebar/components/SidebarSelectionButton.tsx export const SidebarSelectionButton = ({ label, placeholder, isOpen, showCaret, disabled, collapsed, sharedButtonProps, }: SidebarSelectionButtonProps) => ( <Button type={sharedButtonProps.type ?? "text"} className={clsx( "flex items-center justify-between overflow-hidden h-9 transition-[width,padding,gap] duration-300 ease-in-out", collapsed ? "!w-8 !p-1 gap-0" : "w-full px-1.5 py-3 gap-2", sharedButtonProps.className, )} disabled={disabled || sharedButtonProps.disabled} {...sharedButtonProps.rest} > {/* ...same label/caret markup... */} </Button> )web/packages/agenta-entities/src/workflow/state/store.ts (1)
778-805: 🚀 Performance & Scalability | 🔵 TrivialPrompt/agent lists now block on a second full-batch fetch before rendering.
Both
promptWorkflowsListQueryStateAtomandagentWorkflowsListQueryStateAtomstayisPendinguntilappWorkflowsWithAgentFlagsQueryAtomresolves, which requires afetchWorkflowsBatchcall over the entire app-workflow list. This adds a sequential round trip before any row can render, compared to consumingappWorkflowsListQueryAtomdirectly. This appears to be an accepted trade-off given classification requires revision data (per the doc comment inhelpers.ts), but worth confirming the added latency is acceptable for the Prompts/Agents list UX.web/oss/src/components/pages/prompts/store.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fetch/classify pattern — extract a shared helper.
The filter-deleted →
fetchWorkflowsBatch→ classify → map-to-row sequence here is duplicated verbatim inweb/oss/src/components/pages/agents/store.ts. Extracting a shared helper (e.g. in the entities package) would avoid divergence — which has already happened, since the agents store's refetch atom is missing the cache-invalidation logic present here.♻️ Proposed shared helper
// e.g. web/packages/agenta-entities/src/workflow/state/helpers.ts export async function fetchAndClassifyWorkflows( projectId: string, workflows: Workflow[], classify: (workflows: Workflow[], latestRevisions: ReadonlyMap<string, Workflow>) => Workflow[], ): Promise<Workflow[]> { const nonDeleted = workflows.filter((w) => !w.deleted_at) const latestRevisions = await fetchWorkflowsBatch(projectId, nonDeleted.map((w) => w.id)) return classify(nonDeleted, latestRevisions) }- const workflows = response.workflows.filter((workflow) => !workflow.deleted_at) - const latestRevisions = await fetchWorkflowsBatch( - projectId, - workflows.map((workflow) => workflow.id), - ) - - return filterPromptWorkflows(workflows, latestRevisions).map(mapWorkflowToRow) + const workflows = await fetchAndClassifyWorkflows( + projectId, + response.workflows, + filterPromptWorkflows, + ) + return workflows.map(mapWorkflowToRow)Also applies to: 166-172
web/oss/src/components/pages/app-management/components/ApplicationManagementSection.tsx (1)
70-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated nested ternary reduces readability.
The
isArchived ? (agentScope ? … : …) : …pattern is duplicated three times forscopeId,columnVisibilityStorageKey, andexportFilename. Consider deriving a singlevariantkey ("agent" | "app" | "active") once and looking up the three strings from a small map/object.web/oss/src/components/pages/agents/AgentsTableSection.tsx (1)
42-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebounce the agent search input.
onChangeupdatesagentsSearchTermAtomon every keystroke, andagentsWorkflowsQueryAtomdepends on that value in itsqueryKey, so typing can trigger a request per character.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aab48ac-a40f-42c9-9f6a-85c188b5acd6
📒 Files selected for processing (60)
web/ee/src/pages/w/[workspace_id]/p/[project_id]/agents/archived/index.tsxweb/ee/src/pages/w/[workspace_id]/p/[project_id]/agents/index.tsxweb/oss/src/components/Layout/Layout.tsxweb/oss/src/components/Layout/SidebarIsland.tsxweb/oss/src/components/References/ReferenceTag.tsxweb/oss/src/components/References/index.tsweb/oss/src/components/Sidebar/SettingsSidebar.tsxweb/oss/src/components/Sidebar/Sidebar.tsxweb/oss/src/components/Sidebar/components/ListOfOrgs.tsxweb/oss/src/components/Sidebar/components/ListOfProjects.tsxweb/oss/src/components/Sidebar/components/SidebarBackButton.tsxweb/oss/src/components/Sidebar/components/SidebarMenu.tsxweb/oss/src/components/Sidebar/components/WorkflowEntityCard.tsxweb/oss/src/components/Sidebar/components/WorkflowIdentity.tsxweb/oss/src/components/Sidebar/components/WorkflowPicker.tsxweb/oss/src/components/Sidebar/components/assets/workflowEntitySelection.test.tsweb/oss/src/components/Sidebar/components/assets/workflowEntitySelection.tsweb/oss/src/components/Sidebar/dynamic/registry.tsweb/oss/src/components/Sidebar/dynamic/source.tsweb/oss/src/components/Sidebar/dynamic/types.tsweb/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.tsweb/oss/src/components/Sidebar/engine/SidebarMenu.tsxweb/oss/src/components/Sidebar/engine/SidebarShell.tsxweb/oss/src/components/Sidebar/engine/types.tsweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsxweb/oss/src/components/Sidebar/hooks/useWorkflowSwitcher.tsxweb/oss/src/components/Sidebar/scopes/constants.tsweb/oss/src/components/Sidebar/scopes/mainScope.tsxweb/oss/src/components/Sidebar/scopes/settingsScope.tsxweb/oss/src/components/Sidebar/scopes/viewRegistry.tsweb/oss/src/components/Sidebar/scopes/workflowItems.tsweb/oss/src/components/Sidebar/scopes/workflowScope.tsxweb/oss/src/components/Sidebar/types.d.tsweb/oss/src/components/Sidebar/types.tsweb/oss/src/components/pages/agents/AgentsPage.tsxweb/oss/src/components/pages/agents/AgentsTableSection.tsxweb/oss/src/components/pages/agents/ArchivedAgentsPage.tsxweb/oss/src/components/pages/agents/hooks/useAgentsSelection.tsweb/oss/src/components/pages/agents/store.tsweb/oss/src/components/pages/app-management/components/ApplicationManagementSection.tsxweb/oss/src/components/pages/app-management/store/appWorkflowStore.tsweb/oss/src/components/pages/prompts/store.tsweb/oss/src/components/pages/settings/assets/navigation.test.tsweb/oss/src/components/pages/settings/assets/navigation.tsweb/oss/src/lib/atoms/sidebar.tsweb/oss/src/lib/navigation/projectSwitchHref.test.tsweb/oss/src/lib/navigation/projectSwitchHref.tsweb/oss/src/pages/w/[workspace_id]/p/[project_id]/agents/archived/index.tsxweb/oss/src/pages/w/[workspace_id]/p/[project_id]/agents/index.tsxweb/oss/src/pages/w/[workspace_id]/p/[project_id]/settings/index.tsxweb/oss/src/state/org/hooks.tsweb/oss/src/state/workflow/flags.tsweb/oss/tests/playwright/acceptance/evaluators/index.tsweb/packages/agenta-entities/src/workflow/core/schema.tsweb/packages/agenta-entities/src/workflow/index.tsweb/packages/agenta-entities/src/workflow/state/evaluatorUtils.tsweb/packages/agenta-entities/src/workflow/state/helpers.tsweb/packages/agenta-entities/src/workflow/state/index.tsweb/packages/agenta-entities/src/workflow/state/store.tsweb/packages/agenta-entities/tests/unit/workflow-agent-list-classification.test.ts
💤 Files with no reviewable changes (4)
- web/oss/src/components/Sidebar/types.d.ts
- web/oss/src/components/Sidebar/SettingsSidebar.tsx
- web/oss/src/components/Sidebar/components/SidebarMenu.tsx
- web/oss/src/components/Sidebar/components/WorkflowEntityCard.tsx
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
The patch introduces a TypeScript-breaking AntD prop value and lets archived agents leak into the archived apps table.
- Updated SidebarConfig to include workflowCategories and lastPath properties. - Introduced filterVisibleItems and filterVisibleSections to manage item visibility based on isHidden property. - Refactored useSidebarConfig to simplify item creation and visibility logic. - Added useSettingsAccess hook to centralize settings access logic. - Implemented workflow item filtering based on categories in workflowItemSupport. - Enhanced sidebar state management with new atoms for collapsed state and popup groups. - Updated projectSwitchHref to handle multiple query tabs correctly. - Improved error handling in AgentsPage during agent creation. - Refactored various components to utilize new hooks and streamline code. - Added tests to ensure proper functionality of new filtering logic and sidebar behavior.
dosubot
Bot
added
size:L
ashrafchowdury
force-pushed
the
feat/app-structure-with-agents
branch
from
fc24eb7 to
f83ae93
Compare
dosubot
Bot
added
size:XXL
4 participants
No description provided.