From 3b7e678b97fa25e4c0e000b5de7b97ef500eef7f Mon Sep 17 00:00:00 2001 From: majdyz Date: Mon, 13 Apr 2026 04:01:42 +0000 Subject: [PATCH] fix(frontend/builder): address round-5 review comments on BuilderChatPanel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add type="button" and focus-visible ring to Stop/Send buttons in PanelInput - Add type="button" to Retry button in MessageList and Apply button in ActionList - Fix MessageList to render plain text directly and only pass dynamic-tool parts to MessagePartRenderer (text parts were being misrouted through a tool renderer) - Replace clearGraphSessionCacheForTesting export with _graphSessionCache for tests — avoids leaking test scaffolding into the production bundle - Add toast notification in undo restore when target node was deleted between apply and undo (prevents silent no-op) - Fix misleading test: remove red-herring mockNodes.push from 'no auto-send' test since the guard is isGraphLoaded===false, not the node array - Add truncation-path coverage to helpers.test.ts (MAX_NODES/MAX_EDGES branches) - Add deleted-node undo test to actionApplicators.test.ts --- .../__tests__/actionApplicators.test.ts | 32 ++++++++++ .../__tests__/helpers.test.ts | 63 +++++++++++++++++++ .../__tests__/useBuilderChatPanel.test.ts | 12 ++-- .../BuilderChatPanel/actionApplicators.ts | 12 ++++ .../components/ActionList.tsx | 1 + .../components/MessageList.tsx | 32 +++++++--- .../components/PanelInput.tsx | 6 +- .../BuilderChatPanel/useBuilderChatPanel.ts | 9 +-- 8 files changed, 144 insertions(+), 23 deletions(-) diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/actionApplicators.test.ts b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/actionApplicators.test.ts index ca190b97f0..0048dd1422 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/actionApplicators.test.ts +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/actionApplicators.test.ts @@ -432,6 +432,38 @@ describe("applyUpdateNodeInput", () => { // Key did not exist before apply → undo should remove it entirely. expect(Object.prototype.hasOwnProperty.call(hardcoded, "text")).toBe(false); }); + + it("undo toasts and skips setNodes when the target node was deleted after apply", () => { + mockNodes = [ + makeNode({ + id: "node-1", + data: { + id: "node-1", + title: "T", + inputSchema: { type: "object", properties: { text: {} } }, + hardcodedValues: {}, + }, + } as unknown as CustomNode), + ]; + const deps = makeDeps(); + applyUpdateNodeInput( + { type: "update_node_input", nodeId: "node-1", key: "text", value: "v" }, + deps, + ); + const stack = deps.setUndoStack.mock.calls[0][0]([]); + + // Simulate node deletion between apply and undo. + mockNodes = []; + mockSetNodes.mockClear(); + stack[0].restore(); + + // setNodes must NOT be called — there is nothing to restore. + expect(mockSetNodes).not.toHaveBeenCalled(); + // User must be notified via toast. + expect(deps.toast).toHaveBeenCalledWith( + expect.objectContaining({ variant: "destructive" }), + ); + }); }); // ----------------------------------------------------------------------- diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/helpers.test.ts b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/helpers.test.ts index a772cbe1c1..62ba000ec5 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/helpers.test.ts +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/helpers.test.ts @@ -1,6 +1,69 @@ import { describe, expect, it } from "vitest"; import { serializeGraphForChat } from "../helpers"; import type { CustomNode } from "../../FlowEditor/nodes/CustomNode/CustomNode"; +import type { CustomEdge } from "../../FlowEditor/edges/CustomEdge"; + +function makeNode(id: string, title = "Node"): CustomNode { + return { + id, + data: { + title, + description: "", + hardcodedValues: {}, + inputSchema: {}, + outputSchema: {}, + uiType: 1, + block_id: id, + costs: [], + categories: [], + }, + type: "custom" as const, + position: { x: 0, y: 0 }, + } as unknown as CustomNode; +} + +function makeEdge(source: string, target: string): CustomEdge { + return { + id: `${source}-${target}`, + source, + target, + sourceHandle: "result", + targetHandle: "text", + type: "custom", + } as unknown as CustomEdge; +} + +describe("serializeGraphForChat – truncation", () => { + it("includes a truncation note when node count exceeds MAX_NODES (100)", () => { + const nodes = Array.from({ length: 101 }, (_, i) => makeNode(`n${i}`)); + const result = serializeGraphForChat(nodes, []); + expect(result).toContain("1 additional nodes not shown"); + }); + + it("does NOT include a truncation note when node count is exactly MAX_NODES", () => { + const nodes = Array.from({ length: 100 }, (_, i) => makeNode(`n${i}`)); + const result = serializeGraphForChat(nodes, []); + expect(result).not.toContain("additional nodes not shown"); + }); + + it("includes a truncation note when edge count exceeds MAX_EDGES (200)", () => { + const nodes = [makeNode("src"), makeNode("dst")]; + const edges = Array.from({ length: 201 }, (_, i) => + makeEdge(`src${i}`, `dst${i}`), + ); + const result = serializeGraphForChat(nodes, edges); + expect(result).toContain("1 additional connections not shown"); + }); + + it("does NOT include an edge truncation note when edge count is exactly MAX_EDGES", () => { + const nodes = [makeNode("src"), makeNode("dst")]; + const edges = Array.from({ length: 200 }, (_, i) => + makeEdge(`src${i}`, `dst${i}`), + ); + const result = serializeGraphForChat(nodes, edges); + expect(result).not.toContain("additional connections not shown"); + }); +}); describe("serializeGraphForChat – XML injection prevention", () => { it("escapes < and > in node names before embedding in prompt", () => { diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/useBuilderChatPanel.test.ts b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/useBuilderChatPanel.test.ts index 2f06fbc20c..c2af0620e6 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/useBuilderChatPanel.test.ts +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/__tests__/useBuilderChatPanel.test.ts @@ -91,7 +91,7 @@ vi.mock("nuqs", () => ({ // Import after mocks import { useBuilderChatPanel, - clearGraphSessionCacheForTesting, + _graphSessionCache, } from "../useBuilderChatPanel"; beforeEach(() => { @@ -107,7 +107,7 @@ beforeEach(() => { mockSetMessages.mockClear(); mockToast.mockClear(); mockSetQueryStates.mockClear(); - clearGraphSessionCacheForTesting(); + _graphSessionCache.clear(); }); afterEach(() => { @@ -230,15 +230,13 @@ describe("useBuilderChatPanel – session lifecycle", () => { }); describe("useBuilderChatPanel – no auto-send on open", () => { - it("does NOT auto-send any message when the panel opens", async () => { + it("does NOT auto-send any message when the panel opens (isGraphLoaded defaults to false)", async () => { mockPostV2CreateSession.mockResolvedValue({ status: 200, data: { id: "sess-open" }, }); - mockNodes.push({ - id: "n1", - data: { title: "Search Block", description: "" }, - }); + // Note: the guard that prevents the seed is `isGraphLoaded === false` (the + // default). The node array content is irrelevant here — no node push needed. const { result } = renderHook(() => useBuilderChatPanel()); diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/actionApplicators.ts b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/actionApplicators.ts index fa65cd2d10..8dc252a80a 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/actionApplicators.ts +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/actionApplicators.ts @@ -177,6 +177,18 @@ export function applyUpdateNodeInput( // revert `action.key` on the target node. This preserves any other // edits (to this node or other nodes) that happened after apply. const currentNodes = useNodeStore.getState().nodes; + // If the target node was deleted between apply and undo, skip the + // restore and notify the user so they aren't left wondering why nothing + // changed. The stale undo entry is still popped by the caller. + if (!currentNodes.some((n) => n.id === action.nodeId)) { + toast({ + title: "Undo skipped", + description: `Node "${action.nodeId}" no longer exists in the graph.`, + variant: "destructive", + }); + removeAppliedActionKey(setAppliedActionKeys, key); + return; + } const restoredNodes = currentNodes.map((n) => { if (n.id !== action.nodeId) return n; const { [action.key]: _omitted, ...rest } = (n.data.hardcodedValues ?? diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/components/ActionList.tsx b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/components/ActionList.tsx index 7bdc210897..495d469548 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/components/ActionList.tsx +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/components/ActionList.tsx @@ -60,6 +60,7 @@ function ActionItem({ action, nodeMap, isApplied, onApply }: ActionItemProps) { ) : ( ) : (