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 merged 95 commits intomainfrom
react-hook-form and zod schemas from agents-core#1799Conversation
🦋 Changeset detectedLatest commit: 2c4ab19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| 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
|
TL;DR — Replaces the bespoke Zustand + imperative state management on the agent builder page with Key changes
Summary | 95 files | 2 commits | base:
|
| Change | Fields affected |
|---|---|
number → integer |
numEvents, timeInSeconds, stepCountIs, transferCountIs |
| Inline → shared ref | name, description, headers across Sub/Tool/Function/TeamAgent schemas |
z.any() → typed |
providerOptions, headersSchema, contextVariables |
Added .trim() |
model, prompt, executeCode, ResourceIdSchema |
Serialization rewrite — serialize.ts and deserialize.ts
Before:
serializeAgentData(nodes, edges, metadata)carried agent-level fields (models, prompt, contextConfig, stopWhen) through ametadatabag and performed JSON-parse validation inline.
After:serializeAgentData(nodes, edges, mcpRelations?, functionToolFormData?, ...)only producessubAgents,functions, andfunctionTools. Agent-level fields go directly from form data. A newhydrateNodesWithFormData()merges RHF state into node objects before serialization.
deserialize.ts input type narrowed from FullAgentDefinition to AgentGraphData. Removed isDefault from node data (default sub-agent is now derived from useWatch on defaultSubAgentId).
serialize.ts · deserialize.ts · serialize.test.ts
Error handling — from custom parser to formState.errors
Before: A 312-line
agent-error-parser.tsconverted zod issues to user-friendly messages with custom category mapping and node-error lookups via Zustand.
After: Errors derive directly fromformState.errors.useGroupedAgentErrorsdestructures them into semantic groups (subAgents, functionTools, tools, agentSettings).useProcessedErrorsextracts per-node errors forErrorIndicatorbadges.
How do server-side errors integrate?
On a 422 response, the submit handler parses the body asz.ZodIssue[]and callsform.setError()for each issue path, so backend validation errors appear inline alongside client-side ones.
agent-error-summary.tsx · use-grouped-agent-errors.ts · agent-error-parser.ts (deleted)
Node editors — from useNodeEditor to RHF field paths
Before: Each node editor used
useNodeEditor()for imperativeupdateField/updatePathhelpers, localuseStatesynced fromselectedNode.data, and manual JSON parse/validate loops.
After: Editors useuseFullAgentFormContext()with typed field paths (e.g.,`subAgents.${id}.name`). Generic components (GenericInput,GenericJsonEditor, etc.) auto-bind tocontrol+name. No local state or manualmarkUnsaved().
useNodeEditor.ts (161 lines) is deleted entirely. useDeleteNode replaces its delete logic. useAutoPrefillId replaces useAutoPrefillIdZustand with a form-aware implementation.
sub-agent-node-editor.tsx · metadata-editor.tsx · use-node-editor.ts (deleted)
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
1f0ef814ba74. The delta contains 3 commits (primarily mergingmainand one squashed refactor commit). This review verifies the status of previously identified issues and checks for new issues in delta changes.
🟠⚠️ Major (2) 🟠⚠️
Both MAJOR issues below were identified in prior reviews (Feb 10, 35+ days ago) and remain unresolved in the current code:
🟠 1) unsaved-changes-dialog.tsx:51-60 Navigation proceeds after failed save — data loss risk
Issue: Line 56 uses isValid (captured at render time from useFormState) to decide whether to proceed with navigation after await onSubmit(). This checks form validation state, not whether the submission actually succeeded. If the form passes client-side validation but the server rejects the save (network error, server validation error, permission denied), isValid remains true and users are navigated away.
Why: This is a confirmed data loss risk validated by Ito E2E test ROUTE-7. Users click "Save changes", the API fails, and they're navigated away believing their changes were saved. The dialog closes unconditionally (line 59).
Fix: Use isSubmitSuccessful from form state after submission:
async function handleSaveAndLeave() {
if (isSubmitting) return;
await onSubmit();
if (form.formState.isSubmitSuccessful) {
proceedWithNavigation();
}
setShowUnsavedDialog(false);
}Refs:
- react-hook-form formState docs —
isSubmitSuccessfulis set after handleSubmit completes without errors - Original review finding — raised Feb 10
🟠 2) validation.ts:255-262 Status update toggles don't persist disabled state
Issue: The serializeAgentForm function at lines 258-259 forces numEvents and timeInSeconds to default values (10 and 30) via nullish coalescing (??) even when they should be undefined to represent a disabled toggle state. The UI in metadata-editor.tsx uses checked={numEvents !== undefined} to determine checkbox state, so rehydrating with forced defaults makes both toggles appear enabled after reload.
Why: This is the root cause of Ito test failure LOGIC-4: "After disabling one status update mode and saving, reload restored both modes enabled." Users intentionally disable a mode, save, reload, and find both modes re-enabled.
Fix: Preserve undefined during serialization:
statusUpdates: {
...statusUpdates,
enabled: statusUpdates.enabled ?? false,
numEvents: statusUpdates.numEvents, // Don't force default
timeInSeconds: statusUpdates.timeInSeconds, // Don't force default
prompt: statusUpdates.prompt ?? '',
statusComponents: serializeJson(statusUpdates.statusComponents),
},Refs:
- Ito test LOGIC-4 — E2E test confirming the bug
- validation.ts:255-262 — current code
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
page.client.tsx:668Error parsing catch block loses original error string for debugging
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
generic-checkbox.tsx:15Local FormFieldWrapperProps duplicates shared type from peer components
🕐 Pending Recommendations (2)
Both MAJOR issues above were originally raised in prior automated reviews and have persisted across 8+ review cycles spanning 35+ days:
- 🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save (raised Feb 10) - 🟠 Status updates persistence — confirmed by Ito E2E test LOGIC-4
✅ Previously Resolved
The following issues from earlier reviews were addressed in prior commits:
Missing— ✅ AllshouldDirty: truein setValue callssetValuecalls now include{ shouldDirty: true }Error message references wrong component name— ✅ Error message now correctly references<FullAgentFormProvider />Subscription cleanup in— ✅ Now returns cleanup function fromcontext-config.tsxuseEffect(line 23)
ℹ️ Ito E2E Test Status
From the most recent test run (March 14):
| Test | Status | Issue |
|---|---|---|
| LOGIC-1 | ❌ Failed | Prompt autocomplete missing context/header suggestions — initial state not populated on page load |
Earlier test runs confirmed:
- LOGIC-4 ❌ — Status update toggles persistence (introduced by this PR — addressed in Major #2)
- ROUTE-7 ❌ — Save-and-leave with validation errors (introduced by this PR — addressed in Major #1)
🚫 REQUEST CHANGES
Summary: The delta from merging main contains minimal PR-specific code changes. However, two MAJOR issues identified in prior reviews remain unresolved after 35+ days:
-
Data loss risk —
handleSaveAndLeaveusesisValid(render-time validation state) instead ofisSubmitSuccessful(actual submission result). If the API call fails, users are navigated away thinking their changes were saved. -
Status update toggle persistence — Serialization forces defaults for
numEventsandtimeInSeconds, overwriting theundefinedstate that represents a disabled toggle. Users disable a mode, save, reload, and find it re-enabled.
Both issues have been validated by Ito E2E tests and have concrete fix suggestions above. Addressing these before merge will prevent user-facing bugs that cause data loss and confusing UX.
The overall refactoring architecture is sound — migrating from Zustand to react-hook-form with Zod validation is a good direction that improves code organization and leverages established patterns. These two fixes are the final blockers.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
validation.ts:209-351 |
serializeAgentForm missing unit tests | Existing serialize.test.ts covers serialization logic; incremental test improvement, not a blocker |
validation.ts:138-148 |
FullAgentSubAgentSchema untested | Schema composition is tested indirectly via integration; LOW severity |
validation.ts:95-102 |
MCPRelationSchema untested | Schema used in serialize.test.ts; coverage exists at integration level |
validation.ts:104-117 |
Function tool schemas untested | Similar — integration coverage exists |
validation.ts:302-313 |
@ts-expect-error for headers | Pre-existing pattern, not introduced by this PR's core changes |
node-types.tsx:42-62 |
Node data interfaces weakened | Intentional design — form state now lives in react-hook-form, not node data |
validation.ts:99 |
selectedTools nullable.optional | Valid design — null = all tools, undefined = not specified |
validation.ts:56-186 |
Mixed z.strictObject vs z.object | Intentional — strictObject provides better validation for form inputs |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-frontend |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-errors |
3 | 0 | 0 | 0 | 1 | 1 | 1 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-types |
5 | 0 | 0 | 0 | 0 | 2 | 3 |
pr-review-precision |
2 | 0 | 0 | 0 | 0 | 2 | 0 |
pr-review-consistency |
3 | 0 | 1 | 0 | 1 | 0 | 1 |
| Total | 19 | 0 | 1 | 0 | 2 | 7 | 9 |
Note: Main findings count is 0 because both MAJOR issues were already raised in prior reviews and are tracked as Pending Recommendations.
| issues.forEach(({ path, code, message }) => { | ||
| form.setError(path.join('.') as any, { type: code, message }); | ||
| }); | ||
| } catch (parseError) { |
There was a problem hiding this comment.
🟡 MINOR: Error parsing catch block loses original error string
Issue: When JSON.parse(res.error) fails at line 664, the catch block logs 'Failed to parse validation errors' with only parseError but doesn't include res.error in the log, losing the original malformed error string.
Why: When debugging parse failures, engineers will see a SyntaxError but won't know what string was being parsed. The original res.error value is lost.
Fix: Include the original error string:
} catch (parseError) {
console.error('Failed to parse validation errors:', parseError, 'Original error:', res.error);
toast.error('Failed to save agent', { closeButton: true });
}Refs:
| FormMessage, | ||
| } from '@/components/ui/form'; | ||
|
|
||
| export interface FormFieldWrapperProps< |
There was a problem hiding this comment.
💭 CONSIDER: Local FormFieldWrapperProps duplicates shared type
Issue: This component defines FormFieldWrapperProps locally (lines 15-25) while peer components (GenericCodeEditor, GenericJsonEditor, GenericPromptEditor) import from ./form-field-wrapper.
Why: Duplicate type definitions can drift over time and create inconsistencies.
Fix: Import the shared type:
import { type FormFieldWrapperProps } from './form-field-wrapper';
interface GenericCheckboxProps<TFieldValues extends FieldValues>
extends Omit<FormFieldWrapperProps<TFieldValues>, 'children'> {
// ... checkbox-specific props
}Refs:
- generic-json-editor.tsx:16 — imports shared type
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
720f3335eece. The delta contains 1 commit (fix inkeep pull tests) affecting 11 files — all test fixture updates and a minor schema ordering fix inschemas.ts.
✅ Delta Assessment
The delta is clean. Changes are limited to:
| Category | Files Changed | Impact |
|---|---|---|
| CLI test fixtures | 10 | Updated snapshots to use explicit sub-agent names instead of empty strings |
| Schema ordering | 1 | Moved .openapi() call after .omit() in FunctionApiInsertSchema |
No new issues introduced in the delta. The test fixture updates align with the schema change that now requires non-empty name fields for sub-agents.
🕐 Pending Recommendations (2)
Both issues below were identified in prior reviews (35+ days ago) and remain unresolved in unchanged code:
-
🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — usesisValid(render-time validation state) instead ofisSubmitSuccessfulto decide navigation after save. If API call fails, users are navigated away believing changes were persisted. Data loss risk. -
🟠
validation.ts:255-262Status update toggles don't persist disabled state —serializeAgentFormforcesnumEvents: 10andtimeInSeconds: 30defaults even whenundefined(disabled). On reload, disabled toggles appear re-enabled.
Note: These issues have been flagged in 8 consecutive automated reviews since Feb 10 (35+ days). The fixes are straightforward:
unsaved-changes-dialog.tsx:56: Changeif (isValid)→if (form.formState.isSubmitSuccessful)validation.ts:258-259: ChangestatusUpdates.numEvents ?? 10→statusUpdates.numEvents(preserve undefined)
🚫 REQUEST CHANGES
Summary: The delta is clean — test fixtures and schema ordering only. However, 2 MAJOR issues from prior reviews remain unresolved after 35+ days. The isValid vs isSubmitSuccessful bug is a confirmed data loss risk that should be addressed before merge. Both fixes are ~1-line changes.
Discarded (0)
No findings discarded.
Reviewers (9)
| 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-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-precision |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: All 9 reviewers reported no new issues in the delta. The delta only contains test fixture updates and minor schema ordering — no substantive code changes to review.
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
2b65e3688e43. The delta contains 84 commits, most from mergingmain. PR-specific changes include newsyncSavedAgentGraph.ts, validation schema updates, and React Query migration.
🟠⚠️ Major (2) 🟠⚠️
🟠 1) validation.test.ts Missing test coverage for duplicate sub-agent ID validation
Issue: The new .superRefine() at validation.ts:169-189 validates that sub-agent IDs are unique across all nodes, but this critical validation logic has zero test coverage.
Why: If a regression is introduced in the duplicate detection logic (e.g., off-by-one in the loop, incorrect path construction for error messages), the system would silently accept duplicate sub-agent IDs. Users could create agents with conflicting identifiers, causing runtime errors or data corruption when the backend processes them.
Fix: Add test case to validation.test.ts:
it('should reject duplicate sub-agent IDs', () => {
const result = FullAgentUpdateSchema.safeParse({
...createSchema(''),
subAgents: {
'node-1': { id: 'same-id', name: 'A', type: 'internal', ... },
'node-2': { id: 'same-id', name: 'B', type: 'internal', ... }
}
});
expect(result.success).toBe(false);
expect(result.error.issues).toContainEqual(
expect.objectContaining({
path: ['subAgents', 'node-1', 'id'],
message: expect.stringContaining('must be unique')
})
);
});Refs:
- validation.ts:169-189 — the superRefine logic
🟠 2) validation.test.ts Missing test coverage for defaultSubAgentId transform
Issue: The .transform() at validation.ts:211-217 maps defaultSubAgentId from the node key to the actual sub-agent ID via value.subAgents?.[defaultSubAgentId]?.id. This critical ID resolution logic has no test coverage.
Why: If the transform logic regresses (typo in property access, or optional chaining produces undefined unexpectedly), the defaultSubAgentId could become undefined after transformation, breaking agent configuration. The refine validates input, not the transformed output.
Fix: Add test cases:
it('should transform defaultSubAgentId to the actual sub-agent id', () => {
const result = FullAgentUpdateSchema.safeParse({
...createSchema(''),
defaultSubAgentId: 'node-key',
subAgents: {
'node-key': { id: 'actual-sub-agent-id', name: 'Test', type: 'internal', ... }
}
});
expect(result.data.defaultSubAgentId).toBe('actual-sub-agent-id');
});
it('should handle defaultSubAgentId when sub-agent ID is renamed', () => {
// Test when node key differs from the sub-agent id field
});Refs:
- validation.ts:211-217 — the transform logic
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
features/agent/domain/index.ts:2Missing barrel export forsyncSavedAgentGraph
💭 Consider (4) 💭
💭 1) sync-saved-agent-graph.test.ts Missing edge case: tool nodes referencing non-existent sub-agents
Issue: If a tool node references a sub-agent ID that the backend didn't return (e.g., concurrent deletion), the node is kept with a stale subAgentId.
Why: Could cause UI inconsistency where tools appear connected to deleted sub-agents.
Fix: Add test verifying orphaned tool nodes are properly filtered.
💭 2) sync-saved-agent-graph.test.ts Missing edge case: multiple tools claiming same relation ID
Issue: The claimRelationId function implements first-come-first-served allocation, but this deduplication behavior isn't tested.
Why: If logic regresses, multiple nodes could receive the same relationshipId.
Fix: Add test verifying unique relation ID assignment.
💭 3) sync-saved-agent-graph.test.ts Missing edge case: edgeId pointing to removed edges
Issue: The test covers nodeId becoming null when dropped, but not the edgeId path.
Fix: Add test: it('clears edgeId when the selected edge is removed', ...)
💭 4) validation.ts:211-217 Schema transform performs business logic
Issue: The transform derives defaultSubAgentId from sub-agent lookup. In agents-core, schema transforms typically handle coercion (e.g., transformToJson), not ID resolution.
Why: Introduces divergence where validation schemas perform business logic.
Fix: Consider extracting to serialization pipeline or adding clarifying comment.
🕐 Pending Recommendations (1)
- 🟠
unsaved-changes-dialog.tsx:56Navigation proceeds after failed save — usesisValidinstead ofisSubmitSuccessful(raised Feb 10, 43+ days ago)
🚫 REQUEST CHANGES
Summary: The delta introduces valuable improvements (React Query migration, duplicate sub-agent ID validation, graph sync logic), but the new validation logic (superRefine for duplicate IDs, transform for defaultSubAgentId) lacks test coverage. These are critical paths that affect agent configuration correctness. Additionally, the 43-day-old MAJOR issue (isValid vs isSubmitSuccessful in save-and-leave flow) remains unaddressed — this is a confirmed data loss risk where users navigate away after failed saves.
Blocking items:
- Add tests for duplicate sub-agent ID validation
- Add tests for defaultSubAgentId transform
- Fix the save-and-leave navigation issue (change
isValidtoform.formState.isSubmitSuccessful)
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
sub-agent-node-editor.tsx:77-78 |
isPersistedSubAgent timing with renamed IDs |
LOW confidence finding — the form.reset() is called synchronously after save, so timing is likely not an issue. Edge case requires more investigation. |
sync-saved-agent-graph.ts:1-158 |
Function not wired up to any component | The function IS being used — it's imported and tested. The inline synchronization in page.client.tsx is a different approach, not duplication. Both may be intentional for different use cases. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 0 | 1 | 0 | 0 | 1 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
5 | 2 | 3 | 0 | 0 | 0 | 0 |
pr-review-consistency |
6 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 15 | 2 | 4 | 0 | 1 | 1 | 2 |
Note: pr-review-consistency returned 3 INFO findings confirming patterns are consistent — not counted as actionable.
| @@ -1,3 +1,2 @@ | |||
| export * from './agent-initializer-helpers'; | |||
| export * from './deserialize'; | |||
| export * from './serialize'; | |||
There was a problem hiding this comment.
🟡 Minor: Missing barrel export for syncSavedAgentGraph
Issue: The new syncSavedAgentGraph function follows the same domain function pattern as deserializeAgentData and serializeAgentData, but isn't exported from this barrel file.
Why: This breaks the established convention where all domain functions are importable from @/features/agent/domain. Consumers would need to import directly from the file path instead.
Fix: (1-click apply)
| export * from './serialize'; | |
| export * from './deserialize'; | |
| export * from './serialize'; | |
| export * from './sync-saved-agent-graph'; |
Refs:
|
TL;DR — Replaces the imperative zustand-based form/validation/error state on the agent page with a declarative Key changes
Summary | 128 files | 2747 commits | base: Deterministic graph identity system
Factory functions (
|
| Record map | Key derivation | Fields |
|---|---|---|
mcpRelations |
getMcpRelationFormKey({ nodeId }) |
selectedTools, toolPolicies, headers, relationshipId |
functionToolRelations |
getFunctionToolRelationFormKey(nodeKey) |
needsApproval, relationshipId |
form-state-defaults.ts · serialize.ts · mcp-node-editor.tsx
Post-save graph sync and simplified serialization
Before:
serializeAgentDatareturned a fullFullAgentDefinitionincluding metadata, callinghydrateNodesWithFormData()to merge form data back into nodes before readingnode.data. ThesaveAgentservice branched between create and update.
After:editorToPayload()reads all business data from aSerializeAgentFormStatebundle withrequireFormValue()fail-fast guards — no hydration step. After save,syncSavedAgentGraphreconciles three categories of server-assigned IDs: toolcanUserelations, external agent delegate relations, and team agent delegate relations. Node IDs are renamed to deterministic graph keys; URL selection state is preserved viafindNodeByGraphKey.
The saveAgent service and createFullAgent client function are deleted — the page always calls updateFullAgentAction directly.
serialize.ts · sync-saved-agent-graph.ts · deserialize.ts
Zustand store slimmed to graph-only state
Before:
useAgentStoreheldmetadata,errors,showErrorsplus actions likesetMetadata,setErrors,clearErrors,getNodeErrors,getEdgeErrors,deleteSelected.
After: The store holds onlynodes,edges,dirty,history/future(undo/redo), and UI flags (isSidebarSessionOpen,hasOpenModelConfig,jsonSchemaMode). All form-related state moved to react-hook-form.
Dirty tracking combines agentDirtyState from zustand (node positions, graph structure) with isDirty from useFormState — both must be false for the page to be considered "clean." The onEdgesChange handler now synchronously calls form.unregister() for removed edges instead of deferred requestAnimationFrame cleanup.
use-agent-store.ts · unsaved-changes-dialog.tsx · use-animate-graph.ts
Error display driven by useFormState
Before: Errors were parsed by
agent-error-parser, stored in zustand viasetErrors, and displayed viaAgentErrorSummarywhich receivederrorSummary,onClose,onNavigateToEdgeprops.
After:AgentErrorSummaryreadsisSubmittedfromuseFormStateand groups errors viauseGroupedAgentErrors(categorizing bysubAgents,functionTools,externalAgents,teamAgents,tools,agentSettings). AuseWindowFocushook hides the panel when the user is actively editing an input.
The flatNestedFieldMessage utility recursively extracts messages from nested react-hook-form error objects, joining them with → separators for readable display.
agent-error-summary.tsx · use-grouped-agent-errors.ts · error-indicator.tsx
Claude Opus | 𝕏
Ito Test Report ❌20 test cases ran. 7 failed, 13 passed. Overall, 20 test cases ran with 13 passing and 7 failing, and all failures were high-severity blockers tied to a single production defect introduced by this PR: SubAgentNode assumes a model string and calls modelName.split(...) even though model fields are optional, causing deterministic crash/error-boundary behavior when model resolution is undefined. Despite that central crash breaking core editor access across agent and node routes (including deep links, invalid-link self-heal paths, unsaved save-and-leave/race/mobile interruption flows, and default settings load), the rest of the covered functionality passed when reachable, including graph integrity behaviors (default sub-agent init, duplicate ID validation with correct Go-to targeting, rename relation persistence, default-node delete protection, MCP disconnect cleanup), metadata/prompt/JSON validation flows, rapid submit stability, inert handling of payload-like text, and rejection of malicious/reserved IDs with acceptance of valid URL-safe IDs. ❌ Failed (7)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Rapid submit stress remained stable with one effective save and persisted data after reload. | |
| Adversarial | ID validation correctly rejects malicious/reserved values with 400 and accepts a URL-safe ID. | |
| Adversarial | Security payload strings persisted as inert text with no script execution side effects. | |
| Edge | Duplicate sub-agent IDs produced deterministic must be unique validation and both Go-to actions navigated to the correct nodes. |
|
| Edge | Sub-agent rename preserved linked tool relationships after save/reload, with supporting domain-path verification. | |
| Edge | Deleting the default sub-agent was prevented, and the node/default marker remained intact. | |
| Edge | Disconnecting the MCP edge cleared stale relation state and serialization remained consistent. | |
| Logic | Prompt autocomplete showed contextVariablesValue, headers.testHeadersJsonSchemaValue, and $env after JSON config save. | |
| Logic | Prompt with {{unknown}} and {{$env.MY_ENV}} produced one unknown-variable error and no $env error. |
|
| Logic | Invalid contextVariables JSON surfaced grouped validation, blocked successful save, and kept Save enabled for retry. |
|
| Happy-path | Created a new agent, initialized its default sub-agent, and saved cleanly without schema mismatch errors. | |
| Happy-path | Description edit persisted after save/reload, and literal null JSON input was rejected by validation. | |
| Happy-path | Unsaved dialog appeared for invalid dirty state, and Discard correctly navigated to /default/projects. |
Commit: 370ce17
Tell us how we did: Give Ito Feedback
* fix typecheck * fix modelName * fix * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63). * Group 1: Stabilize graph identity * Group 2: Finish RHF as the business source of truth * Group 2: Finish RHF as the business source of truth * Group 2: Finish RHF as the business source of truth * Group 3: Remove layout dependence on node business payload * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * upd * renames * Group 1: Fix remaining schema mismatches * Group 1: Fix remaining schema mismatches * Group 1: Fix remaining schema mismatches * Group 1: Fix remaining schema mismatches * upd * upd * upd * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * Group 3: Collapse connection state into one relation model * more * more * rm * rm * Group 3: Finish explicit graph identity * comment * upd * upd * upd * upd * upd * upd * simplify * lint * update snapshot * fix zod omit issue * fix cypress * style: auto-format with biome * fix review * Update mcp-node-editor.tsx * apply more review * fix: differentiate loading vs missing state in MCP node editor null guard * Update mcp-node-editor.tsx * fix inheritance in subagent node * fix needs approval * polish * fix issue * fix race condition * fix error * Add detailed changeset for graph identity refactor Co-authored-by: Dimitri POSTOLOV <[email protected]> Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Dimitri POSTOLOV <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
370ce17c3f54. The delta contains 34 commits (mostly merges frommainplus PR-specific changes). Key delta changes include graph identity system improvements, function tool relation tracking, and MCP relation key refactoring.
🟠⚠️ Major (1) 🟠⚠️
🟠 1) unsaved-changes-dialog.tsx:56 Navigation proceeds after failed save — data loss risk
Issue: The handleSaveAndLeave function at line 56 uses isValid (captured at render time from useFormState) to decide whether to proceed with navigation after await onSubmit(). This checks form validation state, not whether the submission actually succeeded. If the form passes client-side validation but the server rejects the save (network error, server validation error, permission denied), isValid remains true and users are navigated away.
Why: This is a confirmed data loss risk. Users click "Save", the API call fails (silently or with an error toast), and they're navigated away believing their changes were persisted. The dialog closes unconditionally at line 59. This issue has been flagged in 8+ consecutive automated reviews since Feb 10 (43+ days).
Fix: Use isSubmitSuccessful from form state after submission completes:
async function handleSaveAndLeave() {
if (isSubmitting) {
return;
}
await onSubmit();
if (form.formState.isSubmitSuccessful) {
proceedWithNavigation();
}
setShowUnsavedDialog(false);
}Refs:
- react-hook-form formState docs —
isSubmitSuccessfulis set after handleSubmit completes without errors - unsaved-changes-dialog.tsx:51-60
🟡 Minor (0) 🟡
Inline Comments:
- 🟡 Minor:
use-default-sub-agent-id-ref.ts:8useRef type mismatch with null vs undefined - 🟡 Minor:
use-animate-graph.ts:103Fragile path-based agentId extraction
💭 Consider (3) 💭
💭 1) agent-error-summary.tsx:201-237 Commented-out code in production
Issue: Lines 205-208 and 226-239 contain commented-out code for the error summary close button functionality. The comment explains the feature is temporarily disabled.
Why: Commented-out code reduces readability. The comment indicates this needs rethinking, suggesting it should be tracked as a TODO/issue rather than left as commented code.
Fix: Remove the commented code and create a GitHub issue to track the UX improvement, or add a // TODO(#issue-number): comment linking to a tracked issue.
💭 2) graph-keys.ts:44-71 Complex fallback priority logic lacks direct unit tests
Issue: The getMcpGraphKey function implements complex fallback priority logic with 6 distinct code paths but no dedicated unit tests. It's only tested indirectly through sync-saved-agent-graph.test.ts.
Why: If the priority order is accidentally changed, MCP node identities could be incorrectly computed, causing nodes to lose their connection to persisted data.
Fix: Add unit tests for getMcpGraphKey covering each priority branch.
💭 3) sub-agent-identity.ts:20-40 findSubAgentNodeId function lacks unit tests
Issue: This new utility finds a sub-agent's React Flow node ID given a sub-agent ID and form data mapping. It has no direct unit tests.
Why: If this function incorrectly matches nodes, the graph could select the wrong sub-agent after operations like save.
Fix: Add unit tests for findSubAgentNodeId covering edge cases like empty arrays or multiple nodes.
🕐 Pending Recommendations (1)
- 🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — originally raised Feb 10 (43+ days ago), still unresolved
🚫 REQUEST CHANGES
Summary: The delta introduces a well-structured graph identity system with good test coverage for the new domain logic. However, the MAJOR data loss risk in handleSaveAndLeave (using isValid instead of isSubmitSuccessful) has been pending for 43+ days across 8+ review cycles and must be addressed before merge. The fix is straightforward — replace isValid with form.formState.isSubmitSuccessful to ensure navigation only proceeds after confirmed successful save.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
validation.ts status toggle persistence |
Previously flagged as MAJOR in prior reviews | Status update schema no longer forces defaults — numEvents and timeInSeconds now use NullToUndefinedSchema.pipe() which preserves undefined when not provided. Issue appears resolved in delta. |
sync-saved-agent-graph.test.ts:522 |
Missing test for function tool relation ID claim collision | Low confidence (4/10), edge case, and the core logic is covered by existing integration tests |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
3 | 0 | 1 | 0 | 2 | 0 | 0 |
pr-review-tests |
3 | 0 | 2 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
orchestrator |
1 | 1 | 0 | 0 | 0 | 1 | 1 |
| Total | 7 | 1 | 3 | 0 | 2 | 1 | 2 |
| 'use memo'; | ||
|
|
||
| const form = useFullAgentFormContext(); | ||
| const defaultSubAgentNodeIdRef = useRef<string | undefined>(null); |
There was a problem hiding this comment.
🟡 Minor: useRef type mismatch
Issue: useRef<string | undefined>(null) initializes a ref typed as string | undefined with null. While TypeScript allows this, it creates a runtime inconsistency where defaultSubAgentNodeIdRef.current could be null but the type says string | undefined.
Why: Components consuming this ref might not handle the null case correctly since the type signature doesn't include it.
Fix: (1-click apply)
| const defaultSubAgentNodeIdRef = useRef<string | undefined>(null); | |
| const defaultSubAgentNodeIdRef = useRef<string | undefined>(undefined); |
Refs:
| @@ -51,45 +101,49 @@ export function useAnimateGraph(): void { | |||
| nodes: updateNodeStatus((node) => { | |||
| // this prevents the node from highlighting if the copilot triggers this event | |||
| if (data?.details?.agentId !== /* agentId */ location.pathname.split('/')[5]) { | |||
There was a problem hiding this comment.
🟡 Minor: Fragile path-based agentId extraction
Issue: location.pathname.split('/')[5] relies on a specific URL structure (/[tenantId]/projects/[projectId]/agents/[agentId]). If the URL structure changes or has extra segments, this will silently fail to match.
Why: This pattern can cause the animation to incorrectly skip highlighting the default sub-agent node if the URL structure changes. The comment /* agentId */ is helpful but the implementation is brittle.
Fix: Consider using Next.js useParams() hook to get agentId explicitly, or pass it through props/context. This makes the dependency explicit and self-documenting:
const params = useParams<{ agentId: string }>();
// then use: data?.details?.agentId !== params.agentIdRefs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
bcd4654687a6. The delta contains 1 commit (fix typecheck) affecting 3 files with minor cleanup changes.
✅ Delta Assessment
The delta is clean. Changes are limited to:
| Category | Files Changed | Impact |
|---|---|---|
| CLI generators | 2 | Added missing z import, extracted GeneratorParams type alias |
| UI cleanup | 1 | Removed unused destructured variables from useAgentErrors hook |
No new issues introduced in the delta.
Changes reviewed:
agent-generator.ts: Addedimport { z } from 'zod'— required for schema referencecontext-config-generator.ts: ExtractedGeneratorParamstype alias for cleaner type annotations, changedcontextVariablesparam to acceptnullsidepane.tsx: Removed unusedhasFieldError,getFieldErrorMessage,getFirstErrorFieldfrom destructuring — good cleanup
🕐 Pending Recommendations (1)
This issue was identified in prior reviews (43+ days ago, 8+ review cycles) and remains unresolved in unchanged code:
- 🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — usesisValid(render-time validation state) instead ofisSubmitSuccessfulto decide navigation after save. If API call fails, users are navigated away believing changes were persisted. Data loss risk. Fix:if (form.formState.isSubmitSuccessful)instead ofif (isValid)
💡 APPROVE WITH SUGGESTIONS
Summary: The delta is clean — typecheck fixes and unused import cleanup are well-done. The 1 pending MAJOR recommendation from prior reviews remains unresolved on unchanged code. The isValid → isSubmitSuccessful fix is a one-line change that would prevent data loss when save operations fail — recommend addressing before merge.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Minimal delta review — no sub-reviewers dispatched as changes were trivial typecheck fixes.
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Delta review — scoped to changes since commit
e3176a2939df. The delta contains 2 commits: a merge frommainand the Durable execution feature (#2615). No PR-specific code changes in the delta — the 40 files changed are entirely from mergingmain.
✅ Delta Assessment
The delta is clean and contains no PR-specific changes. All modifications are from the Durable execution (#2615) merge:
| Category | Files Changed | Impact |
|---|---|---|
agents-api/ |
27 | Durable execution workflow, routes, handlers |
packages/agents-core/ |
11 | Schema migrations, data access, types |
| Other | 2 | Changeset, docs script |
No agents-manage-ui changes in the delta. The PR's core functionality (react-hook-form migration, zod schemas) was not modified since the last review.
🕐 Pending Recommendations (1)
This issue was identified 43+ days ago across 8+ consecutive automated reviews and remains unresolved in unchanged code:
-
🟠
unsaved-changes-dialog.tsx:56Navigation proceeds after failed save — data loss riskIssue: Uses
isValid(render-time validation state) instead ofisSubmitSuccessfulto decide navigation afterawait onSubmit(). If API call fails (network error, server validation error, permission denied),isValidremainstrueand users are navigated away believing changes were saved.Fix (1 line): Change line 56 from
if (isValid)→if (form.formState.isSubmitSuccessful)async function handleSaveAndLeave() { if (isSubmitting) return; await onSubmit(); if (form.formState.isSubmitSuccessful) { // ← Fix: check submission result, not validation state proceedWithNavigation(); } setShowUnsavedDialog(false); }
Refs:
- react-hook-form formState docs —
isSubmitSuccessfulis set after handleSubmit completes without errors - unsaved-changes-dialog.tsx:51-60
- react-hook-form formState docs —
💡 APPROVE WITH SUGGESTIONS
Summary: The delta is clean — it consists entirely of the Durable execution feature merged from main with no PR-specific changes. The react-hook-form refactor code was not modified since the last review. The 1 pending MAJOR recommendation (43+ days old, 8+ review cycles) remains unresolved. The fix is a single-line change (isValid → form.formState.isSubmitSuccessful) that would prevent data loss when save operations fail. This is a straightforward fix that should be addressed before merge.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — no sub-reviewers dispatched as the delta contains no PR-specific changes (only merge from main).
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
This is a comprehensive review of the full PR scope. The PR introduces a significant architectural improvement by migrating from Zustand-based state management to react-hook-form with zod validation schemas.
🟠⚠️ Major (2) 🟠⚠️
🟠 1) validation.test.ts Missing test coverage for duplicate sub-agent ID validation
Issue: The FullAgentFormSchema includes a .superRefine() validator at validation.ts:171-191 that checks for duplicate sub-agent IDs across nodes and adds custom errors. This critical validation logic has zero test coverage.
Why: If a regression is introduced in the duplicate detection logic (e.g., off-by-one in iteration, incorrect path construction for error messages), users could create agents with conflicting sub-agent identifiers. This would cause runtime errors or data corruption when the backend processes them, as sub-agent IDs are used as keys in the subAgents record.
Fix: Add test case to validation.test.ts:
it('should reject duplicate sub-agent IDs across nodes', () => {
const result = FullAgentFormSchema.safeParse({
...createSchema(''),
defaultSubAgentNodeId: 'node-1',
subAgents: {
'node-1': {
id: 'same-id', name: 'A', prompt: 'Hi', type: 'internal',
models: { base: {}, structuredOutput: {}, summarizer: {} },
canUse: [], dataComponents: [], artifactComponents: [], stopWhen: {},
},
'node-2': {
id: 'same-id', name: 'B', prompt: 'Hi', type: 'internal',
models: { base: {}, structuredOutput: {}, summarizer: {} },
canUse: [], dataComponents: [], artifactComponents: [], stopWhen: {},
},
},
});
expect(result.success).toBe(false);
expect(result.error?.issues).toContainEqual(
expect.objectContaining({
path: expect.arrayContaining(['subAgents', 'node-1', 'id']),
message: expect.stringContaining('must be unique'),
})
);
});Refs:
Inline Comments:
- 🟠 Major:
page.client.tsx:678-687Server validation errors may fail silently when res.error isn't valid JSON
🟡 Minor (3) 🟡
🟡 1) schemas.ts StatusUpdateSchema changed from z.object() to z.strictObject() — potential breaking change
Issue: The StatusUpdateSchema was changed from z.object() to z.strictObject(). This causes validation to fail on unknown keys.
Why: If API consumers were sending additional properties (even if ignored), their requests will now fail validation. This could break existing integrations.
Fix: If this is intentional, document it in the changeset. If backward compatibility is needed, use .passthrough() on the strict object to allow extra keys while still enforcing known keys.
Refs:
Inline Comments:
- 🟡 Minor:
schemas.ts:161-162Dev comment// cc @sarahleft in production code - 🟡 Minor:
use-grouped-agent-errors.ts:77MCP relation errors collapse to empty key when nodeId lookup fails - 🟡 Minor:
use-processed-errors.tsx:28flatNestedFieldMessage may return undefined, showing cryptic "undefined" to users
💭 Consider (2) 💭
💭 1) graph-keys.ts Naming inconsistency between graph key and form key functions
Issue: The PR introduces two sets of key-generation utilities with different naming conventions: get*GraphKey functions in graph-keys.ts for node identity, and get*FormKey functions in form-state-defaults.ts for form field paths. The distinction between 'graph key' (identity in React Flow) vs 'form key' (path in react-hook-form) is not immediately obvious.
Why: This sets a naming precedent for future development. Clarity between these two concepts will help maintainability.
Fix: Consider adding a brief JSDoc comment to each module clarifying the domain, or prefix form key functions with getFormPath* instead of get*FormKey.
Refs:
💭 2) graph-keys.ts:32-36 getSubAgentGraphKey is identity function unlike other graph key functions
Issue: The getSubAgentGraphKey() returns the subAgentId unchanged, while other get*GraphKey() functions add prefixes (mcp:, function-tool:, etc.). This asymmetry may be confusing when debugging.
Why: Sub-agents are the primary domain entity, so using raw IDs may be intentional for URL compatibility, but this isn't documented.
Fix: Consider adding a comment explaining why sub-agent graph keys are unprefixed.
Refs:
🕐 Pending Recommendations (1)
This issue was identified 43+ days ago across 8+ consecutive automated reviews and remains unresolved in unchanged code:
-
🟠
unsaved-changes-dialog.tsx:51-60Navigation proceeds after failed save — data loss riskIssue: Uses
isValid(render-time validation state) instead ofisSubmitSuccessfulto decide navigation afterawait onSubmit(). If API call fails (network error, server validation error, permission denied),isValidremainstrueand users are navigated away believing changes were saved.Fix (1 line): Change line 56 from
if (isValid)→if (form.formState.isSubmitSuccessful)
🚫 REQUEST CHANGES
Summary: This is a well-structured architectural refactor with solid patterns. The react-hook-form + zod migration is clean and the graph identity system is well-designed. However, the 43+ day old data loss bug in unsaved-changes-dialog.tsx must be addressed before merge — it's a one-line fix (isValid → isSubmitSuccessful). Additionally, the missing test coverage for duplicate sub-agent ID validation should be added to prevent regressions in critical validation logic.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
context-config.tsx:21-41 |
Missing subscription cleanup | Actually correct — the return form.subscribe({...}) pattern properly returns the unsubscribe function |
full-agent-form.tsx:20 |
'use memo' directive concern |
React Compiler is likely configured; directive appears 28+ times across codebase intentionally |
validation.ts:337-343 |
Status updates default values | Already raised in prior reviews (Feb 10) — tracked as pending |
validation.ts:56-61 |
ContextConfigSchema .default(null) |
Intentional design choice for API compatibility — empty string → null is expected behavior |
form-state-defaults.ts:51-67 |
Form key functions are identity transforms | Intentional semantic wrappers for future extensibility |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 0 | 0 | 0 | 0 | 1 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
4 | 0 | 0 | 0 | 4 | 0 | 0 |
pr-review-types |
7 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-consistency |
8 | 0 | 2 | 0 | 0 | 0 | 0 |
| Total | 27 | 2 | 2 | 0 | 4 | 1 | 5 |
Note: pr-review-standards returned 0 findings as instructed not to re-raise the known isValid issue. Duplicate findings across reviewers were merged.
| try { | ||
| const issues: z.ZodIssue[] = JSON.parse(res.error); | ||
| issues.forEach(({ path, code, message }) => { | ||
| form.setError(path.join('.') as any, { type: code, message }); | ||
| }); | ||
| } catch (parseError) { | ||
| // Fallback for unparseable errors | ||
| console.error('Failed to parse validation errors:', parseError); | ||
| toast.error('Failed to save agent', { closeButton: true }); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR Server validation errors may fail silently
Issue: When res.error is not valid JSON (e.g., server returns HTML error page, network interruption, or malformed response), the catch block only logs to console.error and shows a generic "Failed to save agent" toast. The original error message is discarded.
Why: Users see no specific information about what went wrong, making it harder to diagnose save failures. The original res.error string could contain useful debugging information.
Fix:
| try { | |
| const issues: z.ZodIssue[] = JSON.parse(res.error); | |
| issues.forEach(({ path, code, message }) => { | |
| form.setError(path.join('.') as any, { type: code, message }); | |
| }); | |
| } catch (parseError) { | |
| // Fallback for unparseable errors | |
| console.error('Failed to parse validation errors:', parseError); | |
| toast.error('Failed to save agent', { closeButton: true }); | |
| } | |
| // Handle validation errors (422 status - unprocessable_entity) | |
| try { | |
| const issues: z.ZodIssue[] = JSON.parse(res.error); | |
| issues.forEach(({ path, code, message }) => { | |
| form.setError(path.join('.') as any, { type: code, message }); | |
| }); | |
| } catch (parseError) { | |
| // Fallback for unparseable errors - show original message | |
| console.error('Failed to parse validation errors:', parseError); | |
| toast.error(res.error || 'Failed to save agent', { closeButton: true }); | |
| } |
Refs:
| transferCountIs: z | ||
| .number() | ||
| .int() |
There was a problem hiding this comment.
🟡 Minor Dev comment left in production code
Issue: This comment references a specific person and uses informal language ("cc @sarah"). This appears to be a dev note that was accidentally left in production code.
Why: Schema files should have documentation-style comments explaining the validation rules, not person-specific notes.
Fix:
| transferCountIs: z | |
| .number() | |
| .int() | |
| // Maximum transfer count - matches frontend validation limit |
Refs:
| functionTools: { | ||
| ...remapErrorGroup(functionTools ?? EMPTY_GROUP, (toolId) => | ||
| functionToolGraphKeysByToolId.get(toolId) | ||
| ), |
There was a problem hiding this comment.
🟡 Minor MCP relation errors collapse to single empty-string key when nodeId lookup fails
Issue: The remapErrorGroup uses nodeGraphKeysByNodeId.get(nodeId) ?? '' which means if the nodeId is not found in the map, all such errors collapse into a single entry keyed by empty string. Multiple distinct MCP relation errors would overwrite each other.
Why: Users would only see the last error, losing visibility into all validation issues for MCP relations.
Fix: Preserve the original key as fallback:
| ), | |
| const mcpRelations = remapErrorGroup(rawMcpRelations, (nodeId) => nodeGraphKeysByNodeId.get(nodeId) ?? nodeId); |
Refs:
|
|
||
| const fieldErrors = errors?.[entity]?.[key] ?? {}; | ||
| const processedErrors = Object.entries(fieldErrors).map(([key, value]) => ({ | ||
| field: key, |
There was a problem hiding this comment.
🟡 Minor flatNestedFieldMessage may return undefined, showing cryptic "undefined" to users
Issue: When flatNestedFieldMessage(value) returns undefined, the resulting error object has message: undefined. The ErrorIndicator component displays this as : undefined.
Why: Users see cryptic "undefined" text instead of a meaningful error message.
Fix: Provide a fallback message:
| field: key, | |
| message: flatNestedFieldMessage(value) || 'Invalid value', |
Refs:
Ito Test Report ✅14 test cases ran. 14 passed. The unified QA run passed with 14/14 executed test cases and 0 failures (with one additional report containing no test cases), indicating stable behavior across the agent builder, canvas, settings, and playground flows. Key findings were that deep-link pane loading and URL state were coherent, function-tool edge constraints and default sub-agent protections held, duplicate/invalid ID and integer/null JSON validations correctly blocked bad saves, rapid save/unsaved-dialog and edge-toggle race scenarios resolved deterministically (with prior blockage attributed to environment connectivity, not product code), and adversarial prompt/header inputs were handled safely without crashes. ✅ Passed (14)
Commit: Tell us how we did: Give Ito Feedback |
json editors now uses GenericJsonEditor and have expand button too
