feat(dashboard): refactor agent page to use react-hook-form and zod schemas from agents-core#1799
feat(dashboard): refactor agent page to use react-hook-form and zod schemas from agents-core#1799dimaMachina wants to merge 47 commits intomainfrom
react-hook-form and zod schemas from agents-core#1799Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| min="1" | ||
| max="100" |
There was a problem hiding this comment.
I removed because we have zod validation for min/max
|
No docs changes detected. This PR refactors internal dashboard state management to use |
| agentData: FullAgentDefinition | ||
| ): Promise<ActionResult<FullAgentDefinition>> { | ||
| ): Promise<ActionResult<FullAgentResponse>> { |
There was a problem hiding this comment.
mistake, should be response and not definition
| ): Promise<ActionResult<FullAgentDefinition>> { | ||
| ): Promise<ActionResult<FullAgentResponse>> { |
There was a problem hiding this comment.
mistake, should be response and not definition
| export type GetAgentResponse = SingleResponse<FullAgentDefinition>; | ||
| export type UpdateFullAgentResponse = SingleResponse<FullAgentDefinition>; | ||
| export type GetAgentResponse = SingleResponse<FullAgentResponse>; | ||
| export type UpdateFullAgentResponse = SingleResponse<FullAgentResponse>; |
There was a problem hiding this comment.
mistake, should be response
Co-authored-by: dimaMachina <5765049+dimaMachina@users.noreply.github.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
Delta review — scoped to 21 commits since last automated review. The delta introduces the
Editorcomposition pattern and newGenericJsonEditor/GenericPromptEditorform components.
💭 Consider (2) 💭
💭 1) generic-json-editor.tsx / generic-prompt-editor.tsx Accessibility: editor components lack explicit label association
Issue: Both GenericJsonEditor and GenericPromptEditor render FormLabel and editor components as siblings without explicit htmlFor/id pairing or aria-labelledby. The commented-out // aria-labelledby={id} in generic-prompt-editor.tsx:61 suggests this was considered but not implemented.
Why: Monaco-based editors are complex custom widgets. Without explicit ARIA labeling, screen readers may not announce the label when focus moves to the editor.
Fix: Add id association: <FormLabel id={\${name}-label`}>paired witharia-labelledby={`${name}-label`}` on the editor, or verify if the underlying editor components already handle labeling internally.
Refs: generic-prompt-editor.tsx:61
💭 2) knip.config.ts:16-17 Knip ignore entries may be unnecessary
Issue: The new entries for generic-prompt-editor.tsx and generic-json-editor.tsx use ['files'] to suppress unused file warnings. However, these files are now actively imported (per the diff showing import path changes to @/components/form/).
Why: Unnecessary knip ignores can mask legitimate issues in the future.
Fix: Verify if these entries are still needed after the import path changes. If the components are now imported, remove the ignore entries. If needed, consider ['exports'] for unused export warnings vs ['files'] for unused file warnings.
Refs: knip.config.ts:16-17
🕐 Pending Recommendations (3)
Prior feedback from previous review that remains unaddressed:
- 🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — usesisValidinstead ofisSubmitSuccessful - 🟡
metadata-editor.tsx:162-166MissingshouldDirty: truein setValue calls - 🟡
full-agent-form.tsx:36-40Error message references wrong component name
💡 APPROVE WITH SUGGESTIONS
Summary: The delta introduces a clean Editor composition pattern following Vercel composition patterns, with well-structured GenericJsonEditor and GenericPromptEditor components. The refactoring correctly preserves behavior while improving reusability. No bugs or issues found in the delta — just two minor "consider" items for accessibility and configuration cleanup.
The 3 pending recommendations from the previous review remain unaddressed on unchanged code. The Major issue (navigation after failed save) is worth addressing before merge to prevent potential data loss.
Overall, nice work on the editor composition refactoring! 🎨
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
editor.tsx:25-26 |
Children rendered twice in EditorDialog | Intentional design — shows content in both collapsed and expanded states with URI prefixing to maintain separate Monaco instances |
generic-prompt-editor.tsx |
Doesn't use FormFieldWrapper abstraction | Justified deviation — complex label content (AddVariableAction, DialogTrigger) doesn't fit FormFieldWrapper pattern |
generic-json-editor.tsx |
Doesn't use FormFieldWrapper abstraction | Same justification as above — Editor.Dialog wrapper requires different structure |
generic-json-editor.tsx:48-55 |
Accessibility label association | Merged with similar finding for GenericPromptEditor — addressed as single Consider item |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-consistency |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 2 | 0 | 0 | 3 | 5 |
Note: pr-review-standards found no issues — the delta code is clean and follows existing patterns well.
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
Delta review — scoped to 4 commits since last automated review. The delta includes layout/styling fixes for the editor components and Sentry configuration improvements.
Delta Changes Reviewed
The delta introduces:
- Layout fixes in
expandable-field.tsx,generic-json-editor.tsx,generic-prompt-editor.tsx— Addedspace-y-2wrapper andflexcontainer withinline-flexon FormLabel for proper alignment - Sentry configuration in
next.config.ts— AddedauthToken,deleteSourcemapsAfterUpload: true, andreactComponentAnnotation - Documentation — New
manage-database-usageskill and docs (out of scope for this PR's core functionality)
Findings: No issues found in the delta. The layout changes are clean refactoring. The Sentry config improvements are actually security-positive (sourcemaps deleted after upload prevents public exposure while maintaining Sentry debugging capability).
🕐 Pending Recommendations (3)
Prior feedback from previous automated reviews that remains unaddressed:
- 🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — usesisValidinstead ofisSubmitSuccessful - 🟡
metadata-editor.tsx:162-166MissingshouldDirty: truein setValue calls - 🟡
full-agent-form.tsx:36-40Error message references wrong component name
💡 APPROVE WITH SUGGESTIONS
Summary: The delta is clean — layout fixes and Sentry config improvements are well-done. The 3 pending recommendations from previous reviews remain unaddressed on unchanged code. The Major issue (navigation after failed save) is worth addressing before merge to prevent potential data loss, but this is not a blocker for the delta changes themselves.
Nice polish on the editor components! 🎨
Discarded (0)
No findings discarded — all reviewer findings were either clean passes or already covered in prior reviews.
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-devops |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 3 | 0 |
Note: All reviewers found no issues in the delta — the code changes are clean and follow existing patterns.
sarah-inkeep
left a comment
sarah-inkeep
left a comment
There was a problem hiding this comment.
- I noticed that there are a few fields Im not able to remove after I've set the values:
context config, headers json, max transfers, provider options on the base model
example loom
Could you go through and double check all of the fields are able to be removed, I may have missed some in my testing. - After I hit save "try it" is still disabled and I still see the
Unsaved changesmodal - Editing the
Provider optionsfield for the base model of an agent does not re-enable the "save" button - When I try updating a field like the agent description using the copilot the field is updated but it's not reflected in the UI, the UI shows the old description (it does work for the subagent fields so I think it's like something with the refactor?)
- Can you take a look at the comments Claude left I think some may be accurate.
| }; | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: -- only on mount | ||
| useEffect(() => { | ||
| // make sure to unsubscribe; |
There was a problem hiding this comment.
do we need to unsubscribe when this is unmounted?
2 participants
json editors now uses GenericJsonEditor and have expand button too
