refactor(frontend): split AgentTemplateControl into focused modules#4923
Conversation
…s (phase 1) Pure, verbatim move out of the 2070-line AgentTemplateControl into a new `agentTemplate/` folder — no logic change: - agentTemplateUtils.ts — enumLabel, countSummary, cloneItem - itemDescriptors.tsx — ItemDescriptor + describeTool/Mcp/Skill/Instruction and their classifiers (toolName, isFunctionTool, isStaticSkill, isEmbedRefSkill, mdPreview, …) - ItemRow.tsx — ItemAvatar, ItemRow, InstructionsFileRow Orchestrator re-imports them; dropped the now-dead phosphor/toolUtils imports. 2070 → 1628 lines. tsc + lint clean.
…ry (phase 2) Tools, MCP servers and skills were three near-identical copies of: a list body, a draft-then-save drawer, and the editing/commit/validate state. Collapse them onto one code path, driven by a per-kind registry: - itemKinds.tsx — ITEM_KINDS: field, describe, FormView, drawerTitle/width, editView, jsonOnly, isReadOnly, createSeed, draftInvalid per kind. - useConfigItemDrawer.ts — the shared editing/draft/commit/remove/validate machine, writing to each kind's array via the registry. - ConfigItemList.tsx — one list body (rows + empty state) for all three. - The three ConfigItemDrawer blocks become one registry-driven render. Behavior preserved exactly (each per-kind rule mapped 1:1). Orchestrator 1638 → 1423 lines. tsc + lint clean.
…Harness Move the agent-template model/connection/harness/sandbox state and the Model & harness + Advanced section JSX out of AgentTemplateControl into a dedicated useModelHarness hook. The two sections share one coupled state machine (the model/connection state feeds both), so they live together and the hook returns the summaries + section bodies the orchestrator renders. AgentTemplateControl drops from ~1.4k to 672 lines; behavior is unchanged.
Move the agent-template Tools add/remove logic (inline function tools, builtin/gateway tools, async workflow-reference tools, and the derived selected-name set + referenceable-workflow pool) out of AgentTemplateControl into a useAgentTools hook. The orchestrator now just wires the handlers into the picker and renders. AgentTemplateControl drops to 580 lines (from ~2.1k before the split); behavior is unchanged.
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
Frontend
refactoring
|
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:
📝 WalkthroughWalkthrough
ChangesAgent Template Control Refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsx (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the static avatar styles into Tailwind classes.
Only
backgroundis dynamic here;fontSize,fontWeight, andlineHeightshould live in classes to match the frontend styling guideline. As per coding guidelines, "Prefer Tailwind utility classes over CSS-in-JS, inlinestyle={{...}}, and separate CSS files; use CSS-in-JS only for complex Ant Design overrides, JS-calculated dynamic theme styles, or legacy components."Source: Coding guidelines
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim copied narrative comments to concise notes.
Several extracted comments exceed the project’s one-line comment preference. Keep only the surprising constraints inline and move background/tracking detail to docs. 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: 178-186, 323-332, 650-651, 680-685
Source: Coding guidelines
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx (1)
96-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the new explanatory comment blocks.
This block and the nearby newly added multi-line prose comments should be reduced to a single short comment or moved to docs.
As per coding guidelines, “Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception.”
Suggested local cleanup
- // The item-config drawer (tools / MCP servers / skills). Edits happen on a local `draft`; they - // only apply to the config on Save, so creating an item never pollutes the config until - // confirmed, and editing an existing item can be cancelled cleanly (Cancel/X discards the - // draft). The shared machine writes to each kind's array via the ITEM_KINDS registry. + // Shared draft-then-save drawer for tools, MCP servers, and skills.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cebc83a-2316-4421-9e4c-a1a6a212a457
📒 Files selected for processing (9)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ConfigItemList.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ItemRow.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/agentTemplateUtils.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemKinds.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useAgentTools.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useConfigItemDrawer.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx
| <ItemRow | ||
| key={`${kind}-${index}`} | ||
| descriptor={def.describe(item)} | ||
| onEdit={() => openEdit(kind, index, item, def.editView(item))} | ||
| onRemove={() => { | ||
| removeItem(kind, index) | ||
| closeEditor() | ||
| }} | ||
| // Read-only items (static `__ag__*` skills) can't be removed and open disabled. | ||
| disabled={disabled || def.isReadOnly(item)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't wire openEdit into rows that are meant to be read-only.
ConfigItemList treats disabled || def.isReadOnly(item) as if it disables the row, but ItemRow still fires onEdit on click and keyboard activation. Right now static/read-only skills only lose the trash button; they can still open the drawer and be saved back through useConfigItemDrawer.
Suggested fix
{items.map((item, index) => (
+ (() => {
+ const rowDisabled = Boolean(disabled || def.isReadOnly(item))
+ return (
<ItemRow
key={`${kind}-${index}`}
descriptor={def.describe(item)}
- onEdit={() => openEdit(kind, index, item, def.editView(item))}
+ onEdit={() => {
+ if (rowDisabled) return
+ openEdit(kind, index, item, def.editView(item))
+ }}
onRemove={() => {
removeItem(kind, index)
closeEditor()
}}
- // Read-only items (static `__ag__*` skills) can't be removed and open disabled.
- disabled={disabled || def.isReadOnly(item)}
+ disabled={rowDisabled}
/>
+ )
+ })()
))}📝 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.
| <ItemRow | |
| key={`${kind}-${index}`} | |
| descriptor={def.describe(item)} | |
| onEdit={() => openEdit(kind, index, item, def.editView(item))} | |
| onRemove={() => { | |
| removeItem(kind, index) | |
| closeEditor() | |
| }} | |
| // Read-only items (static `__ag__*` skills) can't be removed and open disabled. | |
| disabled={disabled || def.isReadOnly(item)} | |
| {items.map((item, index) => ( | |
| (() => { | |
| const rowDisabled = Boolean(disabled || def.isReadOnly(item)) | |
| return ( | |
| <ItemRow | |
| key={`${kind}-${index}`} | |
| descriptor={def.describe(item)} | |
| onEdit={() => { | |
| if (rowDisabled) return | |
| openEdit(kind, index, item, def.editView(item)) | |
| }} | |
| onRemove={() => { | |
| removeItem(kind, index) | |
| closeEditor() | |
| }} | |
| disabled={rowDisabled} | |
| /> | |
| ) | |
| })() | |
| ))} |
| editView: (item) => (isFunctionTool(item) ? "form" : "json"), | ||
| jsonOnly: (draft) => !isFunctionTool(draft), | ||
| isReadOnly: () => false, | ||
| createSeed: () => ({}), | ||
| draftInvalid: (draft) => { | ||
| const fn = draft.function as Record<string, unknown> | undefined | ||
| if (fn && typeof fn === "object") return !String(fn.name ?? "").trim() | ||
| return false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fresh tool drafts are saveable as {}.
useConfigItemDrawer uses ITEM_KINDS[editing.kind].draftInvalid(draft) as the only save gate and then commits draft directly. With createSeed: () => ({}) and this validator, a brand-new tool is immediately valid unless it already has a function object, so the drawer can persist an empty {} into config.tools.
Suggested fix
tool: {
kind: "tool",
field: "tools",
@@
- createSeed: () => ({}),
+ createSeed: () => ({
+ type: "function",
+ function: {name: "", description: "", parameters: {type: "object", properties: {}}},
+ }),
draftInvalid: (draft) => {
const fn = draft.function as Record<string, unknown> | undefined
if (fn && typeof fn === "object") return !String(fn.name ?? "").trim()
- return false
+ return !String((draft as Record<string, unknown>).type ?? "").trim()
},
},📝 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.
| editView: (item) => (isFunctionTool(item) ? "form" : "json"), | |
| jsonOnly: (draft) => !isFunctionTool(draft), | |
| isReadOnly: () => false, | |
| createSeed: () => ({}), | |
| draftInvalid: (draft) => { | |
| const fn = draft.function as Record<string, unknown> | undefined | |
| if (fn && typeof fn === "object") return !String(fn.name ?? "").trim() | |
| return false | |
| editView: (item) => (isFunctionTool(item) ? "form" : "json"), | |
| jsonOnly: (draft) => !isFunctionTool(draft), | |
| isReadOnly: () => false, | |
| createSeed: () => ({ | |
| type: "function", | |
| function: {name: "", description: "", parameters: {type: "object", properties: {}}}, | |
| }), | |
| draftInvalid: (draft) => { | |
| const fn = draft.function as Record<string, unknown> | undefined | |
| if (fn && typeof fn === "object") return !String(fn.name ?? "").trim() | |
| return !String((draft as Record<string, unknown>).type ?? "").trim() |
| <div | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={onEdit} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault() | ||
| onEdit() | ||
| } | ||
| }} | ||
| className="group flex cursor-pointer items-center gap-2.5 rounded border border-solid border-[var(--ag-c-EAEFF5,#eaeff5)] px-3 py-2 transition-colors hover:border-[var(--ag-c-97A4B0,#97a4b0)]" | ||
| > | ||
| <ItemAvatar descriptor={descriptor} /> | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="truncate font-mono text-xs font-medium">{descriptor.name}</div> | ||
| {descriptor.description ? ( | ||
| <Typography.Text | ||
| type="secondary" | ||
| className="block truncate text-xs leading-tight" | ||
| > | ||
| {descriptor.description} | ||
| </Typography.Text> | ||
| ) : null} | ||
| </div> | ||
| <div className="flex shrink-0 items-center gap-1.5"> | ||
| {descriptor.tags.map((tag) => ( | ||
| <Tag key={tag} className="m-0 text-[11px]"> | ||
| {tag} | ||
| </Tag> | ||
| ))} | ||
| {onRemove && !disabled ? ( | ||
| <button | ||
| type="button" | ||
| aria-label="Remove" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| onRemove() | ||
| }} | ||
| className="flex cursor-pointer items-center border-0 bg-transparent p-0 text-[var(--ag-c-97A4B0,#97a4b0)] opacity-0 transition-opacity hover:text-[var(--ag-c-FF4D4F,#ff4d4f)] group-hover:opacity-100" | ||
| > | ||
| <Trash size={14} /> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disabled/read-only rows still open, and keyboarding the trash button can also fire edit.
ConfigItemList passes disabled specifically to prevent opening read-only rows, but this container still calls onEdit on click/Enter/Space unconditionally. Because the key handler is on the parent, pressing Enter/Space while the nested remove button has focus can also bubble up and open the editor.
Suggested fix
export function ItemRow({
@@
}) {
+ const handleOpen = () => {
+ if (!disabled) onEdit()
+ }
+
return (
<div
role="button"
- tabIndex={0}
- onClick={onEdit}
+ tabIndex={disabled ? -1 : 0}
+ aria-disabled={disabled || undefined}
+ onClick={handleOpen}
onKeyDown={(e) => {
+ if (e.currentTarget !== e.target || disabled) return
if (e.key === "Enter" || e.key === " ") {
e.preventDefault()
- onEdit()
+ onEdit()
}
}}
- className="group flex cursor-pointer items-center gap-2.5 rounded border border-solid border-[var(--ag-c-EAEFF5,`#eaeff5`)] px-3 py-2 transition-colors hover:border-[var(--ag-c-97A4B0,`#97a4b0`)]"
+ className="group flex items-center gap-2.5 rounded border border-solid border-[var(--ag-c-EAEFF5,`#eaeff5`)] px-3 py-2 transition-colors hover:border-[var(--ag-c-97A4B0,`#97a4b0`)] data-[disabled=true]:cursor-default"
+ data-disabled={disabled || undefined}
>📝 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.
| <div | |
| role="button" | |
| tabIndex={0} | |
| onClick={onEdit} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault() | |
| onEdit() | |
| } | |
| }} | |
| className="group flex cursor-pointer items-center gap-2.5 rounded border border-solid border-[var(--ag-c-EAEFF5,#eaeff5)] px-3 py-2 transition-colors hover:border-[var(--ag-c-97A4B0,#97a4b0)]" | |
| > | |
| <ItemAvatar descriptor={descriptor} /> | |
| <div className="min-w-0 flex-1"> | |
| <div className="truncate font-mono text-xs font-medium">{descriptor.name}</div> | |
| {descriptor.description ? ( | |
| <Typography.Text | |
| type="secondary" | |
| className="block truncate text-xs leading-tight" | |
| > | |
| {descriptor.description} | |
| </Typography.Text> | |
| ) : null} | |
| </div> | |
| <div className="flex shrink-0 items-center gap-1.5"> | |
| {descriptor.tags.map((tag) => ( | |
| <Tag key={tag} className="m-0 text-[11px]"> | |
| {tag} | |
| </Tag> | |
| ))} | |
| {onRemove && !disabled ? ( | |
| <button | |
| type="button" | |
| aria-label="Remove" | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| onRemove() | |
| }} | |
| className="flex cursor-pointer items-center border-0 bg-transparent p-0 text-[var(--ag-c-97A4B0,#97a4b0)] opacity-0 transition-opacity hover:text-[var(--ag-c-FF4D4F,#ff4d4f)] group-hover:opacity-100" | |
| > | |
| <Trash size={14} /> | |
| </button> | |
| const handleOpen = () => { | |
| if (!disabled) onEdit() | |
| } | |
| <div | |
| role="button" | |
| tabIndex={disabled ? -1 : 0} | |
| aria-disabled={disabled || undefined} | |
| onClick={handleOpen} | |
| onKeyDown={(e) => { | |
| if (e.currentTarget !== e.target || disabled) return | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault() | |
| onEdit() | |
| } | |
| }} | |
| className="group flex items-center gap-2.5 rounded border border-solid border-[var(--ag-c-EAEFF5,`#eaeff5`)] px-3 py-2 transition-colors hover:border-[var(--ag-c-97A4B0,`#97a4b0`)] data-[disabled=true]:cursor-default" | |
| data-disabled={disabled || undefined} | |
| > |
| const nextModelId = patch.modelId !== undefined ? patch.modelId : modelId | ||
| // When the model changes, derive the provider from the picked model; otherwise keep it. | ||
| let nextProvider: string | null | ||
| if (patch.provider !== undefined) { | ||
| nextProvider = patch.provider | ||
| } else if (patch.modelId !== undefined) { | ||
| nextProvider = | ||
| providerForModel(capabilities, harnessValue, nextModelId) ?? connection.provider | ||
| } else { | ||
| nextProvider = connection.provider | ||
| } | ||
| setAgentField( | ||
| "llm", | ||
| composeModelValue({ | ||
| modelId: nextModelId, | ||
| provider: nextProvider, | ||
| mode: patch.mode !== undefined ? patch.mode : connection.mode, | ||
| slug: patch.slug !== undefined ? patch.slug : connection.slug, | ||
| existing: llm, | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear stale named connections when the provider changes.
writeModel({modelId}) can derive a new provider while retaining the previous connection.slug. Since named connection options are filtered by provider, this can persist an old-provider credential slug that is no longer selectable for the new model/provider.
Proposed fix
} else {
nextProvider = connection.provider
}
+ const nextMode = patch.mode !== undefined ? patch.mode : connection.mode
+ const nextSlug =
+ patch.slug !== undefined
+ ? patch.slug
+ : nextMode !== "agenta" || nextProvider !== connection.provider
+ ? null
+ : connection.slug
setAgentField(
"llm",
composeModelValue({
modelId: nextModelId,
provider: nextProvider,
- mode: patch.mode !== undefined ? patch.mode : connection.mode,
- slug: patch.slug !== undefined ? patch.slug : connection.slug,
+ mode: nextMode,
+ slug: nextSlug,
existing: llm,
}),
)Also applies to: 193-196
| view={drawerView} | ||
| onViewChange={setDrawerView} | ||
| onCancel={closeEditor} | ||
| onSave={commitDraft} | ||
| saveDisabled={draftInvalid || (drawerView === "json" && jsonInvalid)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset stale JSON validity when switching drawer views.
If the user makes JSON invalid, switches to Form, then switches back to JSON, the editor remounts from the valid draft but jsonInvalid remains true, leaving Save disabled until another JSON edit.
Proposed fix
- onViewChange={setDrawerView}
+ onViewChange={(view) => {
+ setDrawerView(view)
+ setJsonInvalid(false)
+ }}📝 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.
| view={drawerView} | |
| onViewChange={setDrawerView} | |
| onCancel={closeEditor} | |
| onSave={commitDraft} | |
| saveDisabled={draftInvalid || (drawerView === "json" && jsonInvalid)} | |
| view={drawerView} | |
| onViewChange={(view) => { | |
| setDrawerView(view) | |
| setJsonInvalid(false) | |
| }} | |
| onCancel={closeEditor} | |
| onSave={commitDraft} | |
| saveDisabled={draftInvalid || (drawerView === "json" && jsonInvalid)} |
Address review nitpicks: move the item-avatar's static font styles into Tailwind classes (only background stays inline/dynamic), and compress the extracted modules' multi-line narrative comments to one-/two-line notes, keeping the genuinely surprising constraints (deliberate model non-clearing, vault-async slug, tracked harness-capabilities gap). No behavior change.
Clarify in itemKinds that tool creation seeds from the picker, not the registry's createSeed stub (defuses a false-positive review read of an empty-tool save path). Record two review items from PR #4923 as deferred against the upcoming schema-driven config redesign: tighter per-kind tool draft validation (kind discriminated union) and re-binding the named-connection slug on a provider change (gated on first-class provider via ModelSpec). Both are correct for the redesign, not the current code, so they live in the design doc's Deferred list.
|
Thanks for the thorough pass. I dug into each against the current code and the agent-config design docs. Summary: all five describe pre-existing behavior (this PR is a verbatim extraction of Read-only skills "saved back" ( Empty Stale connection slug on provider change ( Stale Nitpicks (avatar inline styles → Tailwind, comment length) are addressed in |
1 participant
Context
AgentTemplateControl.tsxhad grown to ~2,070 lines. It held the whole agent-template config panel in one file: the model/connection/harness/sandbox state machine, the tool/MCP/skill list logic, three near-identical item-drawer code paths, the section descriptors, and the render for all three layouts. That made it hard to read and hard to touch without scrolling past unrelated concerns.This is a behavior-preserving refactor. No feature or UX change. The public
AgentTemplateControlexport is unchanged; only its internals moved into focused modules.Changes
Split the orchestrator into a small set of co-located modules under a new
agentTemplate/folder. The orchestrator now just assembles the schema-gatedsectionsarray and renders it; everything else lives in a hook or a registry.Before: one 2,070-line file.
After: a 580-line orchestrator + 8 focused modules.
AgentTemplateControl.tsxuseModelHarness.tsxitemDescriptors.tsxuseAgentTools.tsItemRow.tsxitemKinds.tsxITEM_KINDSregistry: the per-kind config that collapses 3 near-identical paths into 1useConfigItemDrawer.tsConfigItemList.tsxagentTemplateUtils.tsenumLabel,countSummary,cloneItem)The two real code-sharing wins: tools, MCP servers, and skills now run through one
ITEM_KINDSregistry and oneuseConfigItemDrawerstate machine instead of three copies, and the Model & harness + Advanced sections share oneuseModelHarnesshook (their state is coupled, so they belong together).Tests / notes
@agenta/entity-uitypes:checkclean; eslint clean across the orchestrator and allagentTemplate/files.tscholds exactly at the pre-existing baseline (593 errors, unchanged) — no errors leaked into consumers.What to QA