feat(frontend): agent config playground controls#4775
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:L
📝 WalkthroughWalkthroughAdds an "agent" app type to the platform. The ChangesAgent App Type
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/oss/src/components/pages/app-management/modals/CreateAppTypeModal/index.tsx (1)
4-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect three app types.
The comment references "two built-in app types (Chat / Completion)" but the modal now includes a third type (Agent).
📝 Proposed fix
- * Onboarding modal that surfaces the two built-in app types (Chat / - * Completion) as large, equal-weight choices. Used by the welcome-card + * Onboarding modal that surfaces the built-in app types (Chat, Completion, + * and Agent) as large, equal-weight choices. Used by the welcome-card
🧹 Nitpick comments (2)
web/oss/src/components/pages/app-management/modals/CreateAppTypeModal/index.tsx (1)
126-126: ⚡ Quick winConsider grid layout with three items.
The grid uses
grid-cols-2but now displays three options, which will create an asymmetric 2+1 layout. Consider switching togrid-cols-3for equal spacing or use a flex layout that wraps naturally.♻️ Proposed fix for three-column grid
- <div className="grid grid-cols-2 gap-3"> + <div className="grid grid-cols-3 gap-3">web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx (1)
49-56: ⚡ Quick winConsider memoizing
AgentConfigControlfor performance.This is a complex composite control with multiple child controls and array mappings. Wrapping the component with
React.memowould prevent unnecessary re-renders when parent props haven't changed.♻️ Proposed fix
-export function AgentConfigControl({ +export const AgentConfigControl = memo(function AgentConfigControl({ schema, value, onChange, withTooltip, disabled, className, }: AgentConfigControlProps) { + // ... component body +}) -}Don't forget to import
memo:-import {useCallback, useMemo} from "react" +import {memo, useCallback, useMemo} from "react"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20bbe915-9e47-49b1-baf5-80f46cfeddd5
📒 Files selected for processing (8)
web/oss/src/components/pages/app-management/components/CreateAppDropdown/index.tsxweb/oss/src/components/pages/app-management/modals/CreateAppTypeModal/index.tsxweb/oss/src/components/pages/prompts/assets/iconHelpers.tsxweb/packages/agenta-entities/src/workflow/state/appUtils.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/McpServerItemControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SchemaPropertyRenderer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.ts
| const setField = useCallback( | ||
| (key: string, fieldValue: unknown) => onChange({...config, [key]: fieldValue}), | ||
| [config, onChange], | ||
| ) |
There was a problem hiding this comment.
Stabilize the setField callback to prevent unnecessary re-renders.
The setField callback includes config in its dependency array, causing it to be recreated on every config change. This breaks referential stability and can trigger unnecessary re-renders in child components that receive callbacks derived from setField (like setTools).
🚀 Proposed fix using functional update
const setField = useCallback(
- (key: string, fieldValue: unknown) => onChange({...config, [key]: fieldValue}),
- [config, onChange],
+ (key: string, fieldValue: unknown) => {
+ onChange((prev) => ({...(prev ?? {}), [key]: fieldValue}))
+ },
+ [onChange],
)If onChange doesn't support functional updates, wrap it:
+const stableOnChange = useCallback((updater: (prev: Record<string, unknown>) => Record<string, unknown>) => {
+ onChange(updater(config))
+}, [onChange, config])
+
const setField = useCallback(
- (key: string, fieldValue: unknown) => onChange({...config, [key]: fieldValue}),
- [config, onChange],
+ (key: string, fieldValue: unknown) => stableOnChange((prev) => ({...prev, [key]: fieldValue})),
+ [stableOnChange],
)
Reviewer guide: interesting codeThe agent config form is schema-generated, not hand-built. Read these in order:
|
| if (xAgTypeRef === "code" || xAgType === "code") { | ||
| return "code" | ||
| } | ||
| if (xAgTypeRef === "agent_config" || xAgType === "agent_config") { |
There was a problem hiding this comment.
This is the whole schema-to-form contract: an `agent_config` x-ag-type-ref (or x-ag-type) routes the field to AgentConfigControl. The typed shape stays in the SDK `AgentConfigSchema`; the playground just follows the marker. Confirm the agent service actually ships this ref on the config field, otherwise the field falls through to a generic control.
| const agentsMd = | ||
| (config.agents_md as string | null | undefined) ?? | ||
| (config.instructions as string | null | undefined) ?? | ||
| null |
There was a problem hiding this comment.
Read fallback: the editor populates from `agents_md`, then `instructions` as a legacy key. But every write goes to `agents_md` only (setField below). So a config stored under the old `instructions` key shows in the editor, and the first edit silently migrates it to `agents_md`. Confirm `agents_md` is the field the backend reads, so this migration lands somewhere valid.
| } | ||
| }, [value]) | ||
|
|
||
| const handleEditorChange = useCallback( |
There was a problem hiding this comment.
`onChange` fires only when the text parses as JSON; invalid text stays in the editor and is not propagated. That keeps a half-typed server out of the config, but it also means a user can leave the editor showing text that was never saved. Worth confirming the surrounding form signals unsaved/invalid state, otherwise an MCP server edit can look applied but silently not be.
Reviewer guide: interesting codeSuggested reading order and the load-bearing lines:
|
| if (xAgTypeRef === "code" || xAgType === "code") { | ||
| return "code" | ||
| } | ||
| if (xAgTypeRef === "agent_config" || xAgType === "agent_config") { |
There was a problem hiding this comment.
This is the dispatch seam. x-ag-type-ref: "agent_config" routes the whole field object to one composite control instead of per-property rendering. The control then reads each sub-field's schema off schema.properties by name, so the form stays correct only as long as the SDK field names hold. A backend rename surfaces as a missing sub-control, not a type error.
| } | ||
|
|
||
| /** Read the function name of a tool object (the gateway slug for Composio tools). */ | ||
| function toolName(tool: unknown): string | undefined { |
There was a problem hiding this comment.
Every tool helper keys off this toolName (it reads function.name): the selected-set, remove-by-name, and the visible/added check all depend on it. A tool object that lacks function.name is silently invisible to those paths, so it can neither be deduped nor removed by name. Worth confirming every tool shape the picker emits carries that field.
| // Reset the editor text when the value changes from outside (add/remove/reorder). | ||
| const lastExternalRef = useRef<string>(safeStringify(serverObj ?? {})) | ||
| useEffect(() => { | ||
| const next = safeStringify(toServerObj(value) ?? {}) |
There was a problem hiding this comment.
The lastExternalRef guard is the subtle part: it resets the editor text only when the value changes from outside (add/remove/reorder), not on the user's own keystrokes. onChange propagates only when the text parses as JSON, so a half-typed server entry is held local and never reaches the form value. Same model as ToolItemControl, just flagging that in-progress invalid JSON is intentionally not persisted.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Codex subagent review for #4775
I found one blocking correctness concern, but GitHub would not allow this account to submit REQUEST_CHANGES on its own PR, so this is posted as a comment review.
Findings:
-
P1
web/oss/src/components/pages/app-management/components/CreateAppDropdown/index.tsx:37andweb/oss/src/components/pages/app-management/modals/CreateAppTypeModal/index.tsx:51- The Agent option is unconditional, butcreateEphemeralAppFromTemplateonly succeeds ifmatchTemplateForTypefinds an application catalog template foragent. I verified this against the actual backend stack rather than relying only on #4779 docs: #4772'sservices/oss/src/agent/app.pyregisters the handler directly and explicitly notes there is noagenta:builtin:agent:v0/ first-class workflow type yet. With this PR and no agent catalog template, users can select Agent and always hitCouldn't start app creation — please retry. Please gate the Agent option on a real catalog template, or land the first-class template in the dependency, so the UI does not surface a dead create path. -
P2
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx:215- ThisToolSelectorPopoveromitsonRemoveBuiltinTool, while the existingPromptSchemaControlpasses it. Built-in tools still render as selected throughselectedTools, but clicking the checked built-in item does nothing becauseBuiltinToolsPaneonly removes viaonRemoveBuiltinTool. Users can still remove the card manually, but the selector toggle is broken and inconsistent with prompt tools. Please add the same payload-based remove handler used by the prompt control. -
P2
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/McpServerItemControl.tsx:69- The MCP editor treats any parseable JSON as valid and callsonChange, including arrays and primitives.mcp_serversis an array of server objects, so storing[],1, or"x"creates an invalid config rather than keeping the editor state local/invalid. Please only propagate object values (empty string ->{}is fine), or attach a validation schema that rejects non-object JSON.
I did not run tests; this was a GitHub diff/context review. Residual risk: the agent_config property-name contract still needs coverage because a backend rename would silently drop a subcontrol.
1 participant
Agent-workflows: functional PR set
Sliced by functional area, final code only (no intermediate churn). Most PRs are independent off
main; two pairs are stacked. This PR's base ismain.Context
The playground needs to edit a typed agent config: instructions, model, tools, MCP servers, and runtime settings (harness, sandbox, permission policy). The backend already exposes an
agent_configcatalog type generated from the SDK model, and ships a thinx-ag-type-ref: "agent_config"marker on the field. This PR is the frontend that resolves that marker and renders the form. It branches frommainand is one functional slice of the agent-workflows feature, showing the final code for the playground controls.What this changes
The schema renderer now recognizes the
agent_configmarker and dispatches the whole config object to one composite control,AgentConfigControl. That control does not invent new widgets. It reuses the model selector, the tool picker, the enum selects, and a textarea, and adds one new piece:McpServerItemControl, a JSON editor for a single MCP server entry. The create-app dropdown and the create-app type modal both gain an "Agent" option with a robot icon. An agent app runs in chat mode:is_chatis now true for typechatoragent.Key architectural decision to review
The form is driven by the catalog type, not hand-built. The backend marks the field with
x-ag-type-ref, and the frontend dispatches on that marker (SchemaPropertyRenderer.tsx:130). The control then reads each sub-field's schema offschema.propertiesand renders it with an existing control. This keeps the form in sync with the typed schema as long as the property names match:agents_md,model,tools,mcp_servers,harness,sandbox,permission_policy. The tradeoff is the coupling lives in property-name string keys, not in types. If the SDK renames a field, the control silently drops it rather than failing the build. Scrutinize whether that contract is documented and tested well enough to survive a backend rename, and whether the two read-back fallbacks (agents_mdfalling back toinstructions) are the right place for legacy compatibility.The second decision worth a look:
toolsandmcp_serversare both flat arrays on the config, edited as raw JSON, because the backend resolver parses them. The MCP editor only propagatesonChangewhen the text parses as JSON, and keeps invalid text local until it does. That matches howToolItemControlalready works, so it is consistent, but it means a half-typed server entry is held outside the form value.How to review this PR
Read
AgentConfigControl.tsxfirst. Check that every sub-control receives the matchingprops.<field>schema and thatsetFieldmerges rather than replaces the config object. Confirm the tool helpers (handleAddTool,handleRemoveToolByName,selectedToolNames) all key offtoolName, which readsfunction.name. A tool object missing that path would be invisible to the selected-set and the remove-by-name path, which is the likely regression.Then read
McpServerItemControl.tsx. ThelastExternalRefguard resets the editor text only on external value changes (add, remove, reorder) and not on the user's own keystrokes. Check that the parse-then-propagate path does not clobber in-progress edits, and that an empty string maps to{}rather than throwing.Then
appUtils.ts:211for theis_chatchange, and the two create-app menus plusiconHelpers.tsxfor the Agent option and robot icon.Tests / notes
No automated tests ship with this slice. Watch the schema-name contract: the control depends on the SDK field names staying stable, and a rename surfaces as a missing sub-control rather than a type error.