[feat] Split the agent Tools section into Integrations and Subagents - #6375
Open
mmabrouk wants to merge 13 commits into
Open
[feat] Split the agent Tools section into Integrations and Subagents#6375mmabrouk wants to merge 13 commits into
mmabrouk wants to merge 13 commits into
Conversation
… Subagents
The agent config panel had one Tools section whose body carried four sub-headers:
Integrations, Workflow references, Tool definitions and Built-in. The section header's
plus opened a four-item dropdown that spanned all of them.
"Tools" stops working as a name once subagents become their own section, because a
subagent is also a tool: a Tools header sitting next to a Subagents header claims the two
are different kinds of thing when one contains the other. So the section is now called
Integrations and holds only connected apps, and the referenced workflows move up into a
Subagents section of their own.
Each section now has exactly one add action, so both plus buttons open their drawer
directly and the dropdown is gone. AgentToolSelectorPopover had no other caller, so it and
its story go with it.
Tool definitions and provider built-ins are no longer rendered, because tool definitions
are being removed as a feature. An older config may still carry either kind. Those entries
stay in the saved config and still run, but this panel neither lists nor edits them.
Nothing on the wire changes. A subagent is still saved as {type: "reference"} inside the
same flat tools array, and both sections share the tools draft and validation indicator
key, so the section-diff machinery is untouched.
…s a robot icon The old surface was a two-panel master/detail drawer. It made the author choose one workflow, then pin a version or an environment, then read its resolved input schema before adding it. Three things were wrong with it. Authors add helpers in batches, so single-select was the wrong primitive. This is a checkable list. The whole card toggles, select-all acts on whatever the search is showing, and agents already added are listed for context but cannot be picked twice. Of everything the detail panel showed, only the description ever decided the choice. The card now carries the five fields that answer "what does this one do and what can it reach": the agent's own icon, its name, its description, the model it runs on, and the apps it has connected. Slug, version, revision and schema are gone. The model line draws the provider's own mark, the same way the app line draws each integration's logo. The words workflow and reference describe how the entry is saved, not what the author is doing. Nothing on this surface says either one. The section icon is now a robot. The component is presentational: every agent arrives as a prop, so the layout is storied and can be iterated without the project's workflow queries. Six stories cover the default list, a short list, no agents, loading, long content on every field, and all-already-added. Story logos are inline data URIs rather than a public CDN, because a story that reaches the network fails in CI and on a laptop with no egress. NOT WIRED YET. The container still opens the old selector. Two of the five fields, the model and the connected apps, are not on WorkflowReferenceUI today, so the bridge has to grow before this can replace it.
… across the agent pickers The add-subagent drawer had reinvented three things the codebase already had. This makes each one a single implementation that every surface draws through, and rebuilds the drawer on them. The row. The integration drawer settled the shape: a flat row divided by a hairline, a leading mark, a title with a meta line under it, and the action on the right. That action is what adds, and the same button removes afterwards, so the drawer's own footer only says Done. The subagent drawer had grown a different model, a card with a checkbox that staged a selection for a footer button to commit. It now uses the same row, and so do the integration drawer's own two rows, through the new CatalogListRow. Bulk add moved to the section header, where a bulk action belongs. The description. Long text looked bad because the subagent card clamped and stopped there. The permission drawer already had the answer: clamp, measure while collapsed, and offer Show more only when text is really hidden. That is now ExpandableDescription, which also carries the two rules that were easy to miss, that a newline is always expandable and that no description means no toggle. The permission drawer's tool rows and both integration rows now go through it, and the catalog rows gained Show more they never had. The logo run. Integrations have logos and the template cards already draw them well, as marks side by side with the name on hover, instead of a logo-plus-name pair per app. That is now LogoMarks in @agenta/ui, which is below both packages that need it. TemplateProviderMarks keeps its provider lookup and delegates the drawing, so a template card and a subagent row cannot drift. One behaviour changed outside the new drawer: an integration row in the All apps list can now expand its description. Everything else is the same markup reached through a shared component.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe agent template now separates integrations and subagents into distinct sections. Integrations use an agent-scoped drawer. Subagents use workflow catalog data and a dedicated drawer. Shared row, description, avatar, and logo components support the new displays. ChangesAgent template tool management
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The PR adds project-agent delegation and separates it from integrations, but selected subagents follow their latest revision and removing one can overwrite concurrent configuration changes. Merge should be slowed until the write path is concurrency-safe or explicitly accepted, with the mutable delegation behavior acknowledged. Sequence Diagram(s)sequenceDiagram
participant AgentTemplateControl
participant WorkflowReferenceBridge
participant SubagentDrawerContainer
participant AddSubagentDrawer
participant useAgentTools
AgentTemplateControl->>SubagentDrawerContainer: open subagent drawer
SubagentDrawerContainer->>WorkflowReferenceBridge: resolve workflow catalog
WorkflowReferenceBridge-->>SubagentDrawerContainer: return agent options
SubagentDrawerContainer->>AddSubagentDrawer: render options
AddSubagentDrawer->>useAgentTools: add selected references
useAgentTools-->>AgentTemplateControl: update tools configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Taken from the designer's five artboards, read as inline styles rather than eyeballed. The rows now sit in one bordered, rounded card instead of floating on the drawer ground. The integration drawer already did this; the subagent drawer was the odd one out, so this is parity rather than a new idea. Both list containers now use the same 6px radius. The meta line gained a dot between the model and the connected apps. A gap alone let the two runs read as one list of marks. The provider mark moved into a 13px tile the same size as an integration logo, so the line reads as one run, and the model name is tabular so a column of version digits does not look ragged. Row padding goes to 10px vertical, the title line to a 24px minimum matching the small button beside it, the icon chip to 28px, and the row divider to colorSplit, which is what a separator inside a bordered list should be rather than a full border weight. Two fixes came out of screenshotting rather than reading. The loading state was a diagonal staircase of overlapping bars. Skeleton in @agenta/ui is the antd COMPOSITE, an avatar plus a title plus three paragraph rows, so sizing it with a className puts four bars in a box meant for one. Its own doc comment records four call sites that made this mistake; this was the fifth. The skeleton now uses SkeletonBlock and carries the row's real anatomy, including the trailing action, so the list does not jump when the agents land. The long-content story now holds two rows with the same long description, so a reviewer can expand one and read it against the collapsed one without hunting for a second story. Not taken from the design: the drawer's close button stays on the left, because EnhancedDrawer owns that and it is used across the app. The section header's count keeps its bordered tag rather than becoming a filled pill, because SubSectionHeader has no story and one of its three call sites lives in oss/src with no coverage, so the change could not be verified here.
The designer's pass puts every small tile in a row on a 3px radius: the provider mark next to the model, and each connected app's logo. LogoMarks was still on the 4px scale radius, so the run read as slightly rounder than the tile beside it. The no-logo fallback also moves to the quaternary fill, which is the ground the design uses for a tile carrying a letter rather than a mark. This is shared with the template cards on the agent home page, where the marks render at 16px. A 2px radius change reads as nothing there, which is the point: one component, one look.
CI's TypeScript lint job went red on the branch. @agenta/ui runs its own lint script from inside the package, and the Next plugin does not match there, so the "eslint-disable-next-line @next/next/no-img-element" I had added named a rule that is not defined. ESLint treats that as an error in its own right: "Definition for rule ... was not found". It passed locally only because I ran eslint from web/ against the file path, where a different config block matched and the plugin was loaded. The disable was never needed. TemplateProviderMarks carried the same plain img with no disable before this branch, which is the precedent. The comment explaining why next/image is wrong here stays, since that is the part a reader needs.
…e token The replacement comment wrapped onto a line beginning with the eslint-disable token. ESLint matches a directive on that leading token, so the sentence could have been read as a file-wide suppression. A probe on this file was inconclusive, because the config reports nothing there either way. Rewording costs nothing and removes the question.
… split Six defects, all introduced by this branch, all found by an independent review and then verified against the code before fixing. The Subagents section never auto-expanded when its first subagent landed. CONTROLLED_SECTION_KEYS did not list it, so the open-state and count entries the section wrote were dead. Integrations and Subagents shared one validation key, so an integration that needed re-authentication put a warning on the Subagents header while every subagent row was clean. The two sections now own their validation separately. They still share the CHANGE mark, because both really do render out of the same `tools` schema field, and that is now stated in one map rather than implied. On a host with no gateway bridge, such as mobile, an empty Integrations section rendered a focusable "add an integration" link wired to undefined. The section is now gated the same way Subagents is, and the empty-state add is optional on both list bodies, so no dead control is reachable. Subagent rows were keyed by array position. Removing an earlier tool renumbers every row below it, so React reused the wrong subtree. Rows are keyed by the saved reference's own identity now. Accessibility. Every row rendered a button reading "Add", which a screen reader heard as a list of identical buttons; the buttons now name their agent. The Show more toggle had no expanded state, so it now carries aria-expanded and aria-controls and can name what it expands. The logo run exposed nothing: a logo-less app was aria-hidden and the "+N" tooltip hung off a non-focusable span, so neither name was reachable. It is a labeled list now. ExpandableDescription measured once. The answer depends on the element's own box, so a drawer that opens at another width kept the first measurement. It re-measures on resize. Two more, not defects but wrong all the same. The drawer reset its search only when the user closed it, so a close driven by the parent kept the last query; destroyOnClose unmounts the drawer body, not the component that owns the state. And the SubagentList story used `ref_by: "version"`, which cannot occur: the axis is "variant" or "environment", and the version is a field on the variant axis. The story looked right while exercising an impossible wire shape.
The drawer was a design until now; the Subagents plus still opened the old master/detail selector. It opens the new one, against the project's real agents. The data it needed did not exist. A row shows the model an agent runs on and the apps it has connected, and WorkflowReferenceUI is artifact-scoped: it carries id, slug, name, description and type, while those two facts belong to a particular revision. Appending them to the artifact would have shown one revision's model beside a reference bound to another. The seam is one batched hook, useSubagentCatalog, on the existing bridge. It reads the SAME cached latest-revision fetch the type badges already perform, so a row's model, its connected apps, its type and its variant binding all arrive with no extra request. Resolving them per row would have fired one request per visible agent. Icons and logos are read through derived atoms over the whole set for the same reason. Subagents now means agent workflows only, which is what the drawer's copy always claimed. A saved reference can still point at a prompt, a custom workflow or an evaluator, because that was allowed before. Those stay listed and removable, tagged "not an agent", so a reference saved under the old rules cannot become invisible. Only resolved types are tagged, so a slow revision fetch never labels a good agent. Two things the wiring had to get right. An agent is no longer offered itself, which would build a reference loop the runner cannot run. And a batch add runs sequentially, because handleAddWorkflowReference re-reads the freshest config after resolving an input schema, so firing several in parallel would let the last write win and drop the rest. Removal is by slug, not by name: a reference's display name is editable and need not match the workflow it points at, so removing by name could miss the entry or hit a different tool. Verified against the running stack, not only in Storybook: the drawer lists 13 real agents with their real models and app logos, excludes the agent being edited, excludes evaluators, and a full add-then-remove round trip writes a revision each way and is reflected in the panel. WorkflowReferenceSelector and its story are removed. Nothing reaches them any more.
…ents work
Three of the findings were live defects, not tidying.
The provider mark never drew. LLMIconMap is keyed by display names ("Anthropic"), while a saved
config stores the lowercase family ("anthropic"), so every row fell through to the generic glyph.
getProviderIcon normalizes the key and already exists for exactly this; the sibling prompt control
uses it. The bridge now reads the provider through connectionFromConfig too, rather than by hand.
A closed drawer fetched. The container is mounted for the panel's whole life, and nothing in it
was gated on `open`, so it subscribed to one Composio catalog query per distinct connected app
across every agent in the project, before the drawer had ever been opened. Every subscription is
gated on `open` now, and icons resolve for the listed agents rather than for every workflow in the
project, including the prompts and evaluators the list never draws.
The "not an agent" mark never appeared. ConnectedSubagentList resolved types by passing
bridge.workflows, which stays empty until the picker calls activate(). So useSubagentCatalog now
takes SLUGS: the saved list passes the handful it already has, the picker passes the project's.
That also removes a duplicated slug-key derivation.
The rest is cleanup the same review asked for.
One nameless subagent used to light BOTH section headers with two different messages, because the
integrations branch re-scanned the array the subagents branch had just scanned.
Four places read a reference's slug by hand, without the `type === "reference"` guard that
toolReferenceSlug carries, so a non-reference tool holding a `slug` could be mistaken for one. All
four go through the helper. subagentKey also tested `t.variant`, which no writer emits: the writer
saves `variant_id`.
ExpandableDescription attached a ResizeObserver per row, which the permission drawer pays 50 times
over on open. Only a multi-line clamp can change its answer on resize, and those lists are short,
so the single-line case measures once as it did before the extraction.
Dead after this branch deleted its only caller: the whole useWorkflowTypes path (the hook, the
evaluator-label atom, humanizeEvaluatorKey, the evaluatorKey field, and the bridge contract
member), four handlers on useAgentTools that fed the removed dropdown, a pass-through
IntegrationSection wrapper, and props on ExpandableDescription and LogoMarks that no call site
passes.
CHANGE_FIELD_FOR_SECTION was a one-entry lookup table read twice; it is a function now. The two
subagent-list branches shared seven props by copy; they share one object.
…w reference A subagent added to the config panel rendered through describeTool, which classifies a raw tool entry. For a reference that means the internal vocabulary: a "workflow" tag, a solid teal square, and a monospace name. None of it is true on this surface. The reader is looking at another agent. describeSubagent replaces it. No type tag at all, because "workflow" is how the entry is stored and nothing user-meaningful replaces it. The name renders as prose. The chip is the agent's OWN icon: its chosen colour at a low alpha behind a glyph in that same colour, with the dark-mode lightening agentIconChrome already handles, falling back to the section's robot glyph for an agent that never picked one. Getting that icon needed one more field. A saved reference stores only a slug, and an agent's icon is keyed by workflow id, so the catalog now carries the workflow id it already had in hand from the revision it fetches. No extra request. ItemDescriptor grew an optional chip class and style so the row can paint itself; every other item keeps the solid type square. The rest is the row geometry from the same handoff, applied to the shared row rather than to subagents alone, because these are the config panel's rows and one section looking different from its neighbours is the problem, not the fix: 8px radius, the secondary border, a fill on hover instead of a border-colour change, 10px vertical padding with a tighter right edge for the action cluster, a 13px name, the tertiary token on the description, and a 13px caret. The trash becomes a 24px ghost button with a red hover fill, because a hover-only 14px glyph with no box is a small thing to hit and a destructive action deserves a clear state. Checked on the running stack: the panel shows the new row, the word "workflow" is gone, and the Integrations rows read consistently with it.
…orbids
An independent review found this and I verified it against the contract before changing
anything.
A saved workflow reference carried variant_id. ReferenceToolConfig does not have that field, and
ToolConfigBase sets extra="forbid", so an entry holding it fails validation. Nothing consumes it
either: the callback identity is workflow.variant.{slug} or workflow.variant.{slug}.{version}, and
the server parser reads only those. The field predates this branch, but the old selector that
wrote it is gone, so this path owns it now.
References bind by slug on the variant axis with no pinned version, which the server resolves as
the workflow's latest revision. That is the same revision the catalog fetched to build the row, so
what the author saw is what the agent calls. The variant id is gone from the catalog too, since
nothing needs it.
Proof on the running stack, in one saved config: the entry added before this fix still carries
variant_id, and the entry added after it does not.
Two more from the same review.
The provider mark still did not draw. connectionFromConfig reads a ModelRef object, and I was
handing it the model string, so the provider came back null and the row drew the fallback glyph
again. This is the second time the same display broke in a different place.
Overlapping writes could lose one. Every add reads the freshest config and appends, so two rapid
clicks both start from the same array. The sequential loop only protected a single Add all. The
drawer now allows one write at a time and disables every add and remove until it settles.
Naming, also from the review. chipClassName and chipStyle become avatarClassName and avatarStyle,
because "chip" was implementation vocabulary for what the row calls an avatar. useSubagentCatalog
becomes useWorkflowReferenceCatalog, because it deliberately resolves non-agent workflows too.
…it path too
The add path stopped persisting variant_id, but the edit form for an existing reference still
wrote it, so opening a subagent and changing its binding put the field straight back. It is now
deleted on every write from that form, which also cleans an entry saved before the field was known
to be invalid.
The variant selection stays as UI state, because it drives which variant the picker shows as
chosen. It is simply never persisted: ReferenceToolConfig forbids unknown fields, has no
variant_id, and the server resolves workflow.variant.{slug}[.{version}].
mmabrouk
marked this pull request as ready for review
August 31, 2026 11:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AddSubagentDrawer.tsx (2)
1-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffShorten the new source comments. Each listed block exceeds one short line.
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AddSubagentDrawer.tsx#L1-L26: reduce this JSDoc block to one short line.web/packages/agenta-ui/src/drill-in/context/DrillInUIContext.tsx#L110-L115: reduce this JSDoc block to one short line.web/packages/agenta-entity-ui/src/DrillInView/bridges/useWorkflowReferenceBridge.ts#L140-L147: reduce this JSDoc block to one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/SubagentDrawerContainer.tsx#L1-L21: reduce this JSDoc block to one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ReferenceToolFormView.tsx#L83-L85: reduce this comment to one short line.
As per coding guidelines: “At most ONE short line per comment.”Source: Coding guidelines
293-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
classNames.bodyfor the drawer body utilities.EnhancedDrawerforwards this class todata-slot="drawer-body", so replacestyles.bodywith equivalent Tailwind classes.Source: Coding guidelines
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx (1)
996-998: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the repeated comment block.
Lines 993-995 and 996-998 contain the same three lines.
🧹 Proposed fix
// The connected list resolves each saved reference's type, so one pointing at a // prompt or an evaluator is marked rather than passed off as an agent. Without a // bridge there is nothing to resolve it with, so the plain list renders instead. - // The connected list resolves each saved reference's type, so one pointing at a - // prompt or an evaluator is marked rather than passed off as an agent. Without a - // bridge there is nothing to resolve it with, so the plain list renders instead.web/packages/agenta-ui/src/components/presentational/logo-marks/LogoMarks.tsx (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace this inline size style with a Tailwind size-class map. As per coding guidelines: “Always prefer Tailwind utility classes over CSS-in-JS or separate CSS files. Avoid … inline
style={{...}}.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d03d9c68-5abb-4246-aead-99b07c546373
📒 Files selected for processing (28)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ReferenceToolFormView.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/WorkflowReferenceSelector.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AddSubagentDrawer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentIntegrationDrawer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentToolSelectorPopover.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/CatalogListRow.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/IntegrationPermissionDrawer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/SubagentDrawerContainer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ToolManagementList.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useAgentTools.tsweb/packages/agenta-entity-ui/src/DrillInView/bridges/useWorkflowReferenceBridge.tsweb/packages/agenta-home-ui/src/TemplateProviderMarks.tsxweb/packages/agenta-ui/src/components/presentational/index.tsweb/packages/agenta-ui/src/components/presentational/logo-marks/LogoMarks.tsxweb/packages/agenta-ui/src/components/presentational/logo-marks/index.tsweb/packages/agenta-ui/src/drill-in/context.tsweb/packages/agenta-ui/src/drill-in/context/DrillInUIContext.tsxweb/packages/agenta-ui/src/drill-in/index.tsweb/storybook/stories/entity-ui/AddSubagentDrawer.stories.tsxweb/storybook/stories/entity-ui/AgentTemplateControl.stories.tsxweb/storybook/stories/entity-ui/AgentToolSelectorPopover.stories.tsxweb/storybook/stories/entity-ui/SubagentList.stories.tsxweb/storybook/stories/entity-ui/ToolManagementList.stories.tsxweb/storybook/stories/entity-ui/WorkflowReferenceSelector.stories.tsx
💤 Files with no reviewable changes (4)
- web/storybook/stories/entity-ui/AgentToolSelectorPopover.stories.tsx
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/WorkflowReferenceSelector.tsx
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentToolSelectorPopover.tsx
- web/storybook/stories/entity-ui/WorkflowReferenceSelector.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| </Button> | ||
| } | ||
| > | ||
| <ExpandableDescription description={integration.description ?? undefined} /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Name each expandable description control. Without label, screen readers announce repeated controls only as “Show more”.
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentIntegrationDrawer.tsx#L229-L229: passlabel={integration.name}.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/IntegrationPermissionDrawer.tsx#L140-L143: pass the resolved tool name aslabel.
📍 Affects 2 files
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentIntegrationDrawer.tsx#L229-L229(this comment)web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/IntegrationPermissionDrawer.tsx#L140-L143
Comment on lines
+3
to
+14
| /** | ||
| * One row in an agent-config picker: a leading mark, a title with an optional meta line under it, | ||
| * and an action on the right. | ||
| * | ||
| * Extracted from the integration drawer, which is where the shape was settled, so the two agent | ||
| * pickers cannot drift apart. Flat rows divided by a hairline, NOT cards with gaps between them: | ||
| * a picker is a list to run an eye down, and a stack of bordered cards turns every row into an | ||
| * object competing for attention. | ||
| * | ||
| * The action belongs to the row. Adding is a per-row decision the author makes and can undo with | ||
| * the same control, so the drawer's own button never carries the add. | ||
| */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Shorten the new multi-line comments to one short line each.
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/CatalogListRow.tsx#L3-L14: replace the block comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L5-L17: replace the block comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L18-L19: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L34-L35: replace the block comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L51-L52: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L66-L69: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/IntegrationPermissionDrawer.tsx#L113-L114: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L74-L75: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L129-L130: replace the multi-line comment with one short line.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L190-L192: replace the multi-line comment with one short line.web/storybook/stories/entity-ui/SubagentList.stories.tsx#L6-L7: replace the multi-line comment with one short line.web/storybook/stories/entity-ui/SubagentList.stories.tsx#L31-L35: replace the block comment with one short line.web/storybook/stories/entity-ui/SubagentList.stories.tsx#L105-L108: replace the block comment with one short line.web/storybook/stories/entity-ui/SubagentList.stories.tsx#L114-L117: replace the block comment with one short line.web/storybook/stories/entity-ui/ToolManagementList.stories.tsx#L120-L123: replace the block comment with one short line.
As per coding guidelines: “At most ONE short line per comment.”
📍 Affects 6 files
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/CatalogListRow.tsx#L3-L14(this comment)web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L5-L17web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L18-L19web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L34-L35web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L51-L52web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ExpandableDescription.tsx#L66-L69web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/IntegrationPermissionDrawer.tsx#L113-L114web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L74-L75web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L129-L130web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx#L190-L192web/storybook/stories/entity-ui/SubagentList.stories.tsx#L6-L7web/storybook/stories/entity-ui/SubagentList.stories.tsx#L31-L35web/storybook/stories/entity-ui/SubagentList.stories.tsx#L105-L108web/storybook/stories/entity-ui/SubagentList.stories.tsx#L114-L117web/storybook/stories/entity-ui/ToolManagementList.stories.tsx#L120-L123
Source: Coding guidelines
Comment on lines
+172
to
+175
| color: "transparent", | ||
| icon: chrome?.glyph ?? <Robot size={15} weight="fill" />, | ||
| avatarClassName: chrome?.className, | ||
| avatarStyle: chrome?.style, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Give the fallback avatar visible colours.
When the caller passes no chrome, avatarClassName stays undefined. ItemAvatar then treats the row as non-chipped, applies text-white, and paints background: "transparent". The Robot glyph then renders white on the light section sheet, so it disappears in the light theme. This path is reachable: AgentTemplateControl renders SubagentList without chromeBySlug when the workflow bridge is disabled, and a reference without a slug never gets chrome.
Set the same token-based fallback chip that SubagentDrawerContainer uses.
🎨 Proposed fix
mono: "",
color: "transparent",
icon: chrome?.glyph ?? <Robot size={15} weight="fill" />,
- avatarClassName: chrome?.className,
+ avatarClassName:
+ chrome?.className ??
+ "bg-[var(--ag-colorFillSecondary)] text-[var(--ag-colorTextSecondary)]",
avatarStyle: chrome?.style,As per coding guidelines: "When adding or changing UI elements, implement appearance and interaction states for both light and dark themes".
📝 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.
Suggested change
| color: "transparent", | |
| icon: chrome?.glyph ?? <Robot size={15} weight="fill" />, | |
| avatarClassName: chrome?.className, | |
| avatarStyle: chrome?.style, | |
| color: "transparent", | |
| icon: chrome?.glyph ?? <Robot size={15} weight="fill" />, | |
| avatarClassName: | |
| chrome?.className ?? | |
| "bg-[var(--ag-colorFillSecondary)] text-[var(--ag-colorTextSecondary)]", | |
| avatarStyle: chrome?.style, |
Source: Coding guidelines
| // A 24px ghost target, not a bare glyph. A hover-only icon with no box is a | ||
| // small thing to hit, and a destructive action deserves a clear hover | ||
| // state rather than only a colour change. | ||
| className="flex size-6 cursor-pointer items-center justify-center rounded border-0 bg-transparent p-0 text-[var(--ag-colorTextTertiary)] opacity-0 transition-opacity hover:bg-[var(--ag-colorErrorBg)] hover:text-[var(--ag-colorErrorText)] group-hover:opacity-100" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose the focused remove control. Add focus-visible:opacity-100 and a focus indicator; keyboard focus is currently invisible.
Comment on lines
786
to
+796
| if (key === "tools") { | ||
| if (tools.some((t) => ITEM_KINDS.tool.draftInvalid(t as Record<string, unknown>))) | ||
| // Integration entries only: a nameless SUBAGENT is reported on its own header, and | ||
| // reporting it here too lit two sections for one problem. | ||
| const subagentIndices = new Set(subagentTools.map(({index}) => index)) | ||
| if ( | ||
| tools.some( | ||
| (t, i) => | ||
| !subagentIndices.has(i) && | ||
| ITEM_KINDS.tool.draftInvalid(t as Record<string, unknown>), | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the Integrations invalid tip to integration entries.
The tools branch now excludes subagent indices, but it still scans every other entry, including function tool definitions and provider built-ins. The module doc for ToolManagementList states that those kinds render nowhere. An older config with a nameless function tool therefore shows a permanent "A tool is missing its name." indicator on the Integrations header, and the section shows no row that the author can open to fix it.
Scan only the indices the integration rows own.
🔧 Proposed fix
if (key === "tools") {
- // Integration entries only: a nameless SUBAGENT is reported on its own header, and
- // reporting it here too lit two sections for one problem.
- const subagentIndices = new Set(subagentTools.map(({index}) => index))
- if (
- tools.some(
- (t, i) =>
- !subagentIndices.has(i) &&
- ITEM_KINDS.tool.draftInvalid(t as Record<string, unknown>),
- )
- )
+ // Integration entries only: other kinds render nowhere, so a tip about them would
+ // point at a row the author cannot open.
+ const integrationIndices = integrationRows.flatMap(integrationRowIndices)
+ if (
+ integrationIndices.some((i) =>
+ ITEM_KINDS.tool.draftInvalid(tools[i] as Record<string, unknown>),
+ )
+ )
return "A tool is missing its name."📝 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.
Suggested change
| if (key === "tools") { | |
| if (tools.some((t) => ITEM_KINDS.tool.draftInvalid(t as Record<string, unknown>))) | |
| // Integration entries only: a nameless SUBAGENT is reported on its own header, and | |
| // reporting it here too lit two sections for one problem. | |
| const subagentIndices = new Set(subagentTools.map(({index}) => index)) | |
| if ( | |
| tools.some( | |
| (t, i) => | |
| !subagentIndices.has(i) && | |
| ITEM_KINDS.tool.draftInvalid(t as Record<string, unknown>), | |
| ) | |
| ) | |
| if (key === "tools") { | |
| // Integration entries only: other kinds render nowhere, so a tip about them would | |
| // point at a row the author cannot open. | |
| const integrationIndices = integrationRows.flatMap(integrationRowIndices) | |
| if ( | |
| integrationIndices.some((i) => | |
| ITEM_KINDS.tool.draftInvalid(tools[i] as Record<string, unknown>), | |
| ) | |
| ) | |
| return "A tool is missing its name." |
| import { | ||
| AddSubagentDrawer, | ||
| type SubagentOption, | ||
| } from "../../../packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AddSubagentDrawer" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Expose AddSubagentDrawer through an @agenta/entity-ui subpath and import that subpath. As per coding guidelines: “Never use relative paths for cross-package imports.”
Source: Coding guidelines
| import type {Meta, StoryObj} from "@storybook/nextjs" | ||
|
|
||
| // Imported from source: the DrillInView barrel does not re-export the subagent list. | ||
| import {SubagentList} from "../../../packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ToolManagementList" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use an exported package subpath for SubagentList. This relative import crosses packages. As per coding guidelines: “Never use relative paths for cross-package imports.”
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The agent config panel had one Tools section. Inside it, four sub-headings fought for the same space: Integrations, Workflow references, Tool definitions and Built-in. The section header's plus opened a four-item dropdown spanning all of them, so connecting an app and giving an agent a helper agent went through the same menu even though they are different jobs.
"Tools" also stopped working as a name once subagents became their own thing. A subagent is a tool, so a Tools heading next to a Subagents heading claims the two are different kinds of thing when one contains the other.
Changes
The section splits in two. Tools becomes Integrations, holding connected apps. Referenced workflows move up into Subagents. Each section has one add action, so both plus buttons open their drawer directly and the dropdown is gone.
A new add-subagent drawer, wired to real data. The old one made you choose one workflow, pin a version or an environment, and read its resolved input schema before adding it. The new one is a list of the project's agents. Each row shows five things: the agent's icon, its name, its description, the model it runs on, and the apps it has connected. The row's button adds, the same button removes, and the footer only closes.
A saved subagent renders as an agent. It used to render through
describeTool, which classifies a raw tool entry. For a reference that gives the internal vocabulary: a "workflow" tag, a solid teal square, a monospace name. A newdescribeSubagentreplaces it. No type tag, prose name, and the agent's own icon: its chosen colour at a low alpha behind a glyph in that colour.Three components became shared instead of duplicated.
CatalogListRowExpandableDescriptionLogoMarks(@agenta/ui)TemplateProviderMarkson the agent home pageThe data seam
A row's model and connected apps do not exist on
WorkflowReferenceUI, which is artifact-scoped:id,slug,name,description,type. Both live on a revision. Appending them to the artifact would have shown one revision's model beside a reference bound to another.useWorkflowReferenceCatalog(slugs)reads the same cached revision fetch the type badges already performed, and keepstype,workflowId,model,providerandintegrationsfrom it instead of discarding them. Resolving per row would have fired a request per visible agent. It takes slugs rather than the workflow list, because the saved rows must resolve the handful they already have; the project list is lazily activated and stays empty until the picker is opened.Scope decisions
Subagents means agent workflows only. A saved reference can still point at a prompt, a custom workflow or an evaluator, because that was allowed before. Those stay listed and removable, tagged "not an agent". Only resolved types are tagged, so a slow fetch never mislabels a good agent.
Tool definitions and provider built-ins are no longer rendered, because tool definitions are being removed as a feature.
Bugs found and fixed during review
An independent review at high effort plus a four-angle cleanup pass found real defects, not only tidying. Each was verified against the code before changing anything.
A saved reference carried
variant_id, which the contract forbids.ReferenceToolConfighas no such field andToolConfigBasesetsextra="forbid", so an entry holding it fails validation. Nothing consumed it either: the callback identity isworkflow.variant.{slug}[.{version}]and the server parser reads only that. The field predates this branch, but the selector that wrote it is gone, so this path owns it now. Both the add path and the edit form are fixed.Live proof, from one saved config on a running stack:
The provider mark never drew, twice. First
LLMIconMapwas keyed by display names while a config stores the lowercase family. ThenconnectionFromConfigwas handed the model string instead of the model-ref object. Both fell back to a generic glyph.A closed drawer fetched. The container is mounted for the panel's whole life and nothing was gated on
open, so it held one catalog subscription per connected app across every agent in the project before the drawer had ever been opened.The "not an agent" mark never appeared, because it resolved types from a list that stays empty until the picker activates it.
Overlapping writes could lose one. Every add reads the freshest config and appends, so two rapid clicks both started from the same array. One write at a time now, with the actions disabled until it settles.
Also: one nameless subagent lit both section headers with two different messages; four places read a reference slug without the guard that distinguishes a reference from any tool carrying a
slug; aResizeObserverper row cost the permission drawer fifty instances on open; and the loading state rendered as overlapping bars, becauseSkeletonis the antd composite and a single bar isSkeletonBlock.Follow-up, deliberately not in this PR
The catalog fans out one request per workflow.
Promise.allover slugs means 200 agents produce 200 requests, the whole-set cache key stops the picker and the saved rows sharing entries, and canonical workflow invalidation does not reach it.@agenta/entitiesalready owns a batching revision fetcher; this should move there. It also swallows per-slug failures, so a timeout currently reads as "No agents yet".No tests. This branch adds none. The contract of the persisted entry, two rapid adds, and the config-shape extraction are the three worth writing first.
Invalid tool entries no longer render. A malformed gateway entry used to appear as raw JSON so someone could remove it. The two new lists drop it, so it stays in the config and out of sight.
Editing a tool addresses it by array index, so a concurrent change can make Save write to a different tool. Predates this branch.
Tests
tsc --noEmitclean on@agenta/ui,@agenta/entity-uiand@agenta/home-ui.eslint-disablenamed a rule that package does not load, and naming an unloaded rule is itself an error.Checked on a running stack
Not only in Storybook. The panel shows Integrations and Subagents with no Tools section, and neither plus opens a dropdown. The drawer lists 13 real agents with their real models and app logos, excludes evaluators, and excludes the agent being edited. A full add-then-remove round trip writes a revision each way and the panel follows. The word "workflow" no longer appears in the panel.
What to QA
https://claude.ai/code/session_01392yCybQBk3qRyiYepsXnp