feat(settings): full-screen agent definition editor with integrated prompt tab#589
Conversation
Replace the narrow AgentDefinitionFormDialog modal with a full-page AgentDefinitionEditor component that follows the existing PromptEditor pattern. The editor has three tabs: Definition (form fields), System Prompt (prompt editing, edit mode only), and Raw JSON. Remove the separate Prompts tab from the definitions page since prompt editing is now integrated into the editor. Update AgentDefinitionsTable to accept an onEdit callback instead of managing internal dialog state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…xity Extract the editor state and mutations into a dedicated hook so the AgentDefinitionEditor component stays within the complexity limit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✨ On it — checking the fullscreen agent definition editor PR Progress: [█░░░░░░░░░] 9% (iteration 6/70) 🔍 Code Review Update (1 min) I've started reviewing PR #589, focusing on understanding the existing Last updated: iteration 6 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Good architectural shift from modal dialog to full-screen editor, consistent with the existing PromptEditor pattern. However, the PR leaves dead code behind and introduces significant code duplication that should be addressed before merging.
Architecture & Design
-
[SHOULD_FIX] Dead code —
agent-definition-form.tsx: The oldAgentDefinitionFormDialogcomponent inweb/src/components/settings/agent-definition-form.tsxis no longer imported anywhere but remains in the repo. It should be deleted in this PR since this is the change that replaces it. -
[SHOULD_FIX] Dead code —
TemplateEditorinprompt-editor.tsx: The Prompts tab was removed fromdefinitions.tsx, meaningPromptEditoris now only ever called withtarget.type === 'partial'. TheTemplateEditorcomponent (lines 21–176 ofprompt-editor.tsx) is now unreachable dead code and should be cleaned up. TheEditTargettype'stemplatevariant is also dead. -
[SHOULD_FIX] Code duplication —
SystemPromptPanelvsTemplateEditor:SystemPromptPanel(lines 513–665 ofagent-definition-editor.tsx) is a near-verbatim copy ofTemplateEditor(lines 21–176 ofprompt-editor.tsx). Both implement the same save/reset/validate mutation pattern, the sameuseEffectfor content loading, and the same UI layout. The main differences are cosmetic (header text, textarea height). SinceTemplateEditoris now dead code, the simplest path is to delete it. But if prompt editing is needed as a standalone in the future, consider extracting a sharedusePromptEditorhook to avoid re-duplication.
Code Issues
Should Fix
-
agent-definition-editor.tsx:513-516—SystemPromptPanelis missingisDirtystate tracking. The originalTemplateEditortrackedisDirty(set on content change, cleared on save/reset). This was used to indicate unsaved changes. The newSystemPromptPaneldoesn't track this, so users get no visual feedback about unsaved prompt changes. While there's no unsaved-changes guard in either version, dropping the tracking makes it harder to add one later and loses a useful UX signal. -
definitions.tsx:140-142—PartialEditorWrapperis a trivial pass-through wrapper (return <PromptEditor target={{ type: 'partial', name }} onClose={onClose} />). The comment says "Lazy import to avoid circular dep" but this is not a lazy import — it's a regular function call. If there's no actual circular dependency issue, inline this at the call site. If there is, useReact.lazy()for an actual lazy import.
|
Looking into the dead code and duplication now Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔧 Review Response Update (1 min) I've started addressing the code review feedback. My initial focus is on identifying and deleting dead code, specifically Last updated: iteration 2 · respond-to-review |
nhopeatall
left a comment
There was a problem hiding this comment.
Review already submitted above (REQUEST_CHANGES). See review comment for full details.
…acking - Delete agent-definition-form.tsx (no longer imported) - Remove TemplateEditor component and template variant from prompt-editor.tsx - Add isDirty state tracking to SystemPromptPanel - Inline PartialEditorWrapper in definitions.tsx Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review Feedback Addressed ✅All review issues have been addressed: Changes Made
Verification
|
|
✨ On it — checking the fullscreen agent definition editor PR Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔍 Code Review Update (1 min) I've started reviewing the pull request by listing the files in Last updated: iteration 2 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid refactoring — replacing the modal with a full-screen editor is a clear UX improvement, and integrating the system prompt tab reduces navigation friction. The extraction of useDefinitionEditor is clean, and the component decomposition is well done. A few things to consider:
Should Fix
-
agent-definition-editor.tsx:881 — The
requiredattribute on the Agent Type<Input>is a leftover from the old<form>+onSubmitpattern. Since the form was replaced with a plain<div>and save usesonClick→handleSave(), native form validation never runs. A user can create a definition with an empty agent type string. Either add a manual check inhandleSave(e.g.if (!agentType.trim()) return;) or wrap in a<form>again. -
agent-definition-editor.tsx:517 —
isDirtystate inSystemPromptPanelis tracked (set on edit, cleared on save/reset) but never consumed for any purpose — no unsaved-changes warning, no conditional UI. It's dead state. Either remove it or use it (e.g. disable the Cancel/Back button, or show a confirmation before navigating away).
Nitpick
- agent-definition-editor.tsx:744 —
handleTabChangesilently swallows JSON parse errors when switching from the "json" tab back to "definition" — thecatchblock just leaves the form as-is with no feedback to the user. The user could have invalid JSON, switch to the Definition tab, and not realize their JSON edits were lost. Consider showing a brief toast or keeping the user on the JSON tab with the error displayed.
| value={agentType} | ||
| onChange={(e) => setAgentType(e.target.value)} | ||
| placeholder="e.g. implementation, review, debug" | ||
| required |
There was a problem hiding this comment.
The required attribute here is a no-op since this is no longer inside a <form> element. handleSave() is triggered via onClick, so native HTML validation never fires. A user can create a definition with an empty agentType.
Consider adding a guard in handleSave:
if (!isEdit && !agentType.trim()) return;There was a problem hiding this comment.
Fixed. Removed the no-op required attribute and added a guard at the top of handleSave: if (!isEdit && !agentType.trim()) return; — this prevents saving with an empty agent type in create mode.
| const queryClient = useQueryClient(); | ||
| const [content, setContent] = useState(''); | ||
| const [validationStatus, setValidationStatus] = useState<string | null>(null); | ||
| const [isDirty, setIsDirty] = useState(false); |
There was a problem hiding this comment.
isDirty is set on edit and cleared on save/reset, but its value is never read anywhere — no unsaved-changes guard, no conditional UI. This is dead state that should either be removed or put to use (e.g., warn before navigating away via the Back/Cancel buttons).
There was a problem hiding this comment.
Fixed. Removed the dead isDirty state entirely — it was being set but never read anywhere. The mutation callbacks and the loadDefault/onChange handlers no longer reference it.
| try { | ||
| setDef(JSON.parse(jsonText) as AgentDefinition); | ||
| setJsonError(null); | ||
| } catch { |
There was a problem hiding this comment.
Minor: when the user has invalid JSON and switches to the Definition tab, the parse error is silently swallowed and their JSON edits are discarded. Consider keeping the user on the JSON tab with a visible error, similar to what handleSave does (line 758-759).
There was a problem hiding this comment.
Fixed. handleTabChange now catches the JSON parse error, sets jsonError with the error message, and returns early — keeping the user on the JSON tab so they can see and fix the error instead of silently swallowing it.
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM with minor suggestions — see inline comments on earlier review.
|
@aaight address code review concerns |
Request AddressedWhat was askedAddress the remaining code review concerns from @nhopeatall's inline comments. Changes Made
Verification
|
- Add guard in handleSave to prevent saving with empty agentType in create mode - Remove no-op `required` attribute from Agent Type input (not in a <form>) - Remove dead `isDirty` state from SystemPromptPanel (was set but never read) - Show JSON parse error and keep user on JSON tab when switching to Definition tab with invalid JSON, instead of silently swallowing the error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✨ On it — checking the fullscreen agent definition editor PR Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔍 Code Review Update (1 min) I've started by reviewing the pull request changes to understand the modifications. I'm currently examining the diff and will specifically check for any import path issues, as a file rename was involved. My next step is to ensure the changes are correct and maintain code quality. Last updated: iteration 2 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that lifts the agent definition editor from a modal dialog to a full-screen view and integrates system prompt editing inline. The architecture is improved: state management is better separated via useDefinitionEditor, the table component is properly decoupled from the editor via the onEdit callback, and the user flow is more consistent (definitions, prompts, and partials all use the same full-screen editing pattern).
Should Fix
- agent-definition-editor.tsx:877 — The
requiredattribute was removed from the Agent Type<Input>in create mode. WhilehandleSavesilently bails withif (!isEdit && !agentType.trim()) return, the user gets no feedback — the save button appears clickable but nothing happens. Consider either restoringrequired(with a wrapping<form>for native validation) or adding an inline error message when the field is empty.
Minor Suggestions
-
prompt-editor.tsx:7 —
EditTargetis now a single-variant type ({ type: 'partial'; name: string }). Thetypediscriminator is vestigial since thetemplatevariant was removed andPromptEditorjust delegates toPartialEditordirectly. Consider simplifying the interface to accept{ name: string }instead — though this can wait for a follow-up. -
agent-definition-editor.tsx:737 — The
handleTabChangebidirectional JSON↔form sync is a nice improvement over the old one-way sync. Good catch adding the JSON→form direction.
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that lifts the agent definition editor from a modal dialog to a full-screen view and integrates system prompt editing inline. The architecture is improved: state management is better separated via useDefinitionEditor, the table component is properly decoupled via the onEdit callback, and the user flow is more consistent.
Should Fix
- agent-definition-editor.tsx:877 — The required attribute was removed from the Agent Type Input in create mode. While handleSave silently bails with if (!isEdit && !agentType.trim()) return, the user gets no feedback — the save button appears clickable but nothing happens. Consider restoring required or adding an inline error message.
Minor Suggestions
- prompt-editor.tsx:7 — EditTarget is now a single-variant type. The type discriminator is vestigial. Consider simplifying to accept { name: string } in a follow-up.
|
@aaight address code review concerns |
Request AddressedWhat was askedAddress the remaining code review concerns from @nhopeatall's latest review round. Changes Made
Verification
|
- Show inline error message when user tries to save with empty agent type in create mode, instead of silently bailing out - Highlight the input field with destructive border on validation error - Clear the error when user starts typing - Remove vestigial `type` discriminator from single-variant EditTarget type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the remaining review concerns from @nhopeatall's latest review:
Commit: 8460986 |
|
✨ On it — checking the fullscreen agent definition editor PR Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔍 Code Review Update (1 min) I've successfully loaded all necessary context from the pre-loaded data. I'm now beginning a systematic analysis of the pull request. My initial assessment is that the PR replaces a modal dialog ( Last updated: iteration 2 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean implementation of a full-screen agent definition editor with integrated system prompt tab. Previous review feedback has been addressed. The architecture is consistent with the existing PromptEditor full-page takeover pattern, and the component decomposition is well done.
Notes
- The
useDefinitionEditorhook extraction is a good way to manage complexity, keeping the main component focused on rendering. ReferencePanelreuse fromprompt-editor.tsxavoids duplication — good call exporting it rather than duplicating.- Tab synchronization between form/JSON views is correctly handled — JSON parse errors block the tab switch and show an error, matching the PR description.
- The
existing?.agentType ?? ''fallback on line 961 is safe since it's guarded byisEdit(which is!!existing). - All prior review comments (no-op
requiredattribute, deadisDirtystate, silent JSON error swallowing) have been addressed in follow-up commits. - CI checks all pass.
No blocking or should-fix issues found.
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - Clean implementation consistent with existing patterns. All prior review feedback addressed.
Summary
AgentDefinitionFormDialogmodal with a new full-screenAgentDefinitionEditorcomponent following the same full-page takeover pattern used by the existingPromptEditorAgentDefinitionsTableto accept anonEditcallback instead of managing its own internal dialog stateKey decisions
AgentDefinitionEditoris a plain<div>full-screen component (not a dialog), consistent with howPromptEditorworks for partials/templatesuseDefinitionEditorhook extracted to keep the main component's cognitive complexity within the linter's allowed thresholdReferencePanelexported fromprompt-editor.tsxso it can be reused inSystemPromptPanelwithout duplicationTest plan
Trello card: https://trello.com/c/69a41ed41d3e2867e47feeb2
🤖 Generated with Claude Code