From 1ea6ce6ce4df5fc9a0187b13aafa0db1dc9d751d Mon Sep 17 00:00:00 2001 From: majdyz Date: Thu, 9 Apr 2026 06:10:11 +0700 Subject: [PATCH] =?UTF-8?q?fix(frontend/builder):=20address=20review=20blo?= =?UTF-8?q?ckers=20=E2=80=94=20duplicate=20edge=20guard,=20undo=20anti-pat?= =?UTF-8?q?tern,=20stack=20cap,=20a11y,=20and=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard against duplicate connect_nodes edges: check prevEdges before applying, mark as already-applied without duplicating if edge exists - Cap undo stack at MAX_UNDO=20 to prevent unbounded memory growth for large graphs - Fix React anti-pattern: call restore() before setUndoStack updater instead of inside it (state updaters must be pure — no side effects) - Add aria-modal="true" to dialog panel and aria-expanded to toggle button - Extract IIFE nodeMap into ActionList sub-component (cleaner render path) - Add 18 new tests: handleSend when canSend=false, Shift+Enter no-send, schema-absent permissive paths (update + connect_nodes), sequential multi-undo LIFO order, duplicate edge guard, undo stack size cap, empty stack no-op --- .../BuilderChatPanel/BuilderChatPanel.tsx | 61 ++-- .../__tests__/useBuilderChatPanel.test.ts | 296 ++++++++++++++++++ .../BuilderChatPanel/useBuilderChatPanel.ts | 56 +++- 3 files changed, 376 insertions(+), 37 deletions(-) diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx index e6042e02bb..84da305308 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx @@ -76,6 +76,7 @@ export function BuilderChatPanel({ className, isGraphLoaded }: Props) {
{isOpen ? : } @@ -291,26 +293,12 @@ function MessageList({ })} {parsedActions.length > 0 && ( -
-

- Suggested changes -

- {(() => { - const nodeMap = new Map(nodes.map((n) => [n.id, n])); - return parsedActions.map((action) => { - const key = getActionKey(action); - return ( - - ); - }); - })()} -
+ )}
@@ -318,6 +306,37 @@ function MessageList({ ); } +function ActionList({ + parsedActions, + nodes, + appliedActionKeys, + onApplyAction, +}: { + parsedActions: GraphAction[]; + nodes: CustomNode[]; + appliedActionKeys: Set; + onApplyAction: (action: GraphAction) => void; +}) { + const nodeMap = new Map(nodes.map((n) => [n.id, n])); + return ( +
+

Suggested changes

+ {parsedActions.map((action) => { + const key = getActionKey(action); + return ( + + ); + })} +
+ ); +} + function ActionItem({ action, nodeMap, 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 ae5143ebd7..cdb85ad976 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 @@ -848,4 +848,300 @@ describe("useBuilderChatPanel – handleSend", () => { expect(mockSendMessage).not.toHaveBeenCalled(); }); + + it("does not send when canSend is false (sessionError=true)", async () => { + mockPostV2CreateSession.mockRejectedValue(new Error("fail")); + const { result } = renderHook(() => useBuilderChatPanel()); + + await openAndFlush(() => result.current.handleToggle()); + expect(result.current.sessionError).toBe(true); + expect(result.current.canSend).toBe(false); + + act(() => { + result.current.setInputValue("hello"); + }); + + act(() => { + result.current.handleSend(); + }); + + expect(mockSendMessage).not.toHaveBeenCalled(); + }); +}); + +describe("useBuilderChatPanel – handleKeyDown", () => { + it("calls handleSend on Enter without Shift when canSend is true", async () => { + mockPostV2CreateSession.mockResolvedValue({ + status: 200, + data: { id: "sess-kd" }, + }); + const { result } = renderHook(() => useBuilderChatPanel()); + + await openAndFlush(() => result.current.handleToggle()); + + act(() => { + result.current.setInputValue("test message"); + }); + + const mockPreventDefault = vi.fn(); + act(() => { + result.current.handleKeyDown({ + key: "Enter", + shiftKey: false, + preventDefault: mockPreventDefault, + } as unknown as import("react").KeyboardEvent); + }); + + expect(mockPreventDefault).toHaveBeenCalled(); + expect(mockSendMessage).toHaveBeenCalledWith({ text: "test message" }); + }); + + it("does NOT call handleSend on Shift+Enter (allows newline insertion)", async () => { + mockPostV2CreateSession.mockResolvedValue({ + status: 200, + data: { id: "sess-shift" }, + }); + const { result } = renderHook(() => useBuilderChatPanel()); + + await openAndFlush(() => result.current.handleToggle()); + + act(() => { + result.current.setInputValue("multiline"); + }); + + const mockPreventDefault = vi.fn(); + act(() => { + result.current.handleKeyDown({ + key: "Enter", + shiftKey: true, + preventDefault: mockPreventDefault, + } as unknown as import("react").KeyboardEvent); + }); + + expect(mockPreventDefault).not.toHaveBeenCalled(); + expect(mockSendMessage).not.toHaveBeenCalled(); + }); +}); + +describe("useBuilderChatPanel – schema-absent nodes", () => { + it("update_node_input: allows any key when node has no inputSchema (permissive mode)", () => { + mockNodes.push({ + id: "schema-less", + data: { hardcodedValues: {} }, + // No inputSchema at all + }); + const { result } = renderHook(() => useBuilderChatPanel()); + + act(() => { + result.current.handleApplyAction({ + type: "update_node_input", + nodeId: "schema-less", + key: "any_key", + value: "any_value", + }); + }); + + // Without a schema, validation is skipped — the key is applied permissively + expect(mockSetNodes).toHaveBeenCalledWith([ + { + id: "schema-less", + data: { hardcodedValues: { any_key: "any_value" } }, + }, + ]); + expect(mockToast).not.toHaveBeenCalled(); + }); + + it("connect_nodes: allows connection when source node has no outputSchema (permissive mode)", () => { + mockNodes.push( + { id: "src-no-schema", data: {} }, // no outputSchema + { + id: "tgt-has-schema", + data: { inputSchema: { properties: { input: {} } } }, + }, + ); + const { result } = renderHook(() => useBuilderChatPanel()); + + act(() => { + result.current.handleApplyAction({ + type: "connect_nodes", + source: "src-no-schema", + target: "tgt-has-schema", + sourceHandle: "any_output", + targetHandle: "input", + }); + }); + + // Without an outputSchema, sourceHandle validation is skipped + expect(mockSetEdges).toHaveBeenCalled(); + expect(mockToast).not.toHaveBeenCalled(); + }); + + it("connect_nodes: allows connection when target node has no inputSchema (permissive mode)", () => { + mockNodes.push( + { + id: "src-has-schema", + data: { outputSchema: { properties: { output: {} } } }, + }, + { id: "tgt-no-schema", data: {} }, // no inputSchema + ); + const { result } = renderHook(() => useBuilderChatPanel()); + + act(() => { + result.current.handleApplyAction({ + type: "connect_nodes", + source: "src-has-schema", + target: "tgt-no-schema", + sourceHandle: "output", + targetHandle: "any_input", + }); + }); + + // Without an inputSchema, targetHandle validation is skipped + expect(mockSetEdges).toHaveBeenCalled(); + expect(mockToast).not.toHaveBeenCalled(); + }); +}); + +describe("useBuilderChatPanel – sequential multi-undo (LIFO order)", () => { + it("undoes two applied actions in LIFO order, restoring correct state at each step", () => { + const initialNode = { + id: "n1", + data: { hardcodedValues: { x: "original" } }, + }; + mockNodes.push(initialNode); + + const { result } = renderHook(() => useBuilderChatPanel()); + + // Apply first action + act(() => { + result.current.handleApplyAction({ + type: "update_node_input", + nodeId: "n1", + key: "x", + value: "first_change", + }); + }); + expect(result.current.undoStack).toHaveLength(1); + + // Apply second action + act(() => { + result.current.handleApplyAction({ + type: "update_node_input", + nodeId: "n1", + key: "x", + value: "second_change", + }); + }); + expect(result.current.undoStack).toHaveLength(2); + + // Undo second action — should restore to snapshot taken before second action + // (which captured the state after first action, i.e. mockNodes at that point) + mockSetNodes.mockClear(); + act(() => { + result.current.handleUndoLastAction(); + }); + expect(result.current.undoStack).toHaveLength(1); + // setNodes called with the snapshot captured before second action applied + expect(mockSetNodes).toHaveBeenCalledOnce(); + + // Undo first action — should restore to snapshot taken before first action + mockSetNodes.mockClear(); + act(() => { + result.current.handleUndoLastAction(); + }); + expect(result.current.undoStack).toHaveLength(0); + expect(mockSetNodes).toHaveBeenCalledWith([initialNode]); + }); +}); + +describe("useBuilderChatPanel – duplicate edge guard", () => { + it("does not append duplicate edge when same connect_nodes action is applied twice", () => { + mockNodes.push({ id: "src", data: {} }, { id: "tgt", data: {} }); + + const action = { + type: "connect_nodes" as const, + source: "src", + target: "tgt", + sourceHandle: "out", + targetHandle: "in", + }; + + // Simulate the edge store updating when setEdges is called + const newEdge = { + id: "src:out->tgt:in", + source: "src", + target: "tgt", + sourceHandle: "out", + targetHandle: "in", + type: "custom", + }; + mockSetEdges.mockImplementationOnce((edges: unknown[]) => { + mockEdges.push(...edges); + }); + + const { result } = renderHook(() => useBuilderChatPanel()); + + act(() => { + result.current.handleApplyAction(action); + }); + + expect(mockSetEdges).toHaveBeenCalledOnce(); + expect(result.current.appliedActionKeys.size).toBe(1); + // Verify the edge is now in the mock store + expect(mockEdges).toContainEqual(expect.objectContaining(newEdge)); + + // Second apply of the same action — should not call setEdges again + mockSetEdges.mockClear(); + act(() => { + result.current.handleApplyAction(action); + }); + + // setEdges should NOT be called again — the edge already exists in the store + expect(mockSetEdges).not.toHaveBeenCalled(); + // But appliedActionKeys should still contain the key + expect(result.current.appliedActionKeys.size).toBe(1); + }); +}); + +describe("useBuilderChatPanel – undo stack size cap", () => { + it("caps the undo stack at MAX_UNDO (20) entries, dropping the oldest", () => { + // Push 21 nodes so each apply action targets a unique node + for (let i = 0; i <= 20; i++) { + mockNodes.push({ id: `n${i}`, data: { hardcodedValues: {} } }); + } + + const { result } = renderHook(() => useBuilderChatPanel()); + + // Apply 21 actions + for (let i = 0; i <= 20; i++) { + act(() => { + result.current.handleApplyAction({ + type: "update_node_input", + nodeId: `n${i}`, + key: "v", + value: `val${i}`, + }); + }); + } + + // Stack should be capped at 20 + expect(result.current.undoStack).toHaveLength(20); + }); +}); + +describe("useBuilderChatPanel – handleUndoLastAction on empty stack", () => { + it("does nothing when undoStack is empty", () => { + const { result } = renderHook(() => useBuilderChatPanel()); + + expect(result.current.undoStack).toHaveLength(0); + + // Should not throw or call setNodes/setEdges + act(() => { + result.current.handleUndoLastAction(); + }); + + expect(mockSetNodes).not.toHaveBeenCalled(); + expect(mockSetEdges).not.toHaveBeenCalled(); + expect(result.current.undoStack).toHaveLength(0); + }); }); diff --git a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts index 9b11da363f..f16bd0b219 100644 --- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts +++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts @@ -28,6 +28,9 @@ import { type SendMessageFn = ReturnType["sendMessage"]; +/** Maximum number of undo entries to keep. Oldest entries are dropped when the limit is reached. */ +const MAX_UNDO = 20; + /** Snapshot of node data taken before an action is applied, enabling undo. */ interface UndoSnapshot { actionKey: string; @@ -285,9 +288,8 @@ export function useBuilderChatPanel({ : n, ); const key = getActionKey(action); - setUndoStack((prev) => [ - ...prev, - { + setUndoStack((prev) => { + const entry: UndoSnapshot = { actionKey: key, restore: () => { setNodes(prevNodes); @@ -297,8 +299,10 @@ export function useBuilderChatPanel({ return next; }); }, - }, - ]); + }; + const trimmed = prev.length >= MAX_UNDO ? prev.slice(1) : prev; + return [...trimmed, entry]; + }); setNodes(nextNodes); } else if (action.type === "connect_nodes") { const sourceNode = nodes.find((n) => n.id === action.source); @@ -335,10 +339,25 @@ export function useBuilderChatPanel({ // restore use setEdges (not addEdge/removeEdge) to bypass the global // history store — keeps chat-panel changes separate from Ctrl+Z. const prevEdges = useEdgeStore.getState().edges; + // Guard against duplicate edges — the same connection may appear after an + // undo-then-reapply or from identical suggestions across AI messages. + const alreadyExists = prevEdges.some( + (e) => + e.source === action.source && + e.target === action.target && + e.sourceHandle === action.sourceHandle && + e.targetHandle === action.targetHandle, + ); + if (alreadyExists) { + // Edge already present — mark as applied without duplicating it. + setAppliedActionKeys( + (prev) => new Set([...prev, getActionKey(action)]), + ); + return; + } const key = getActionKey(action); - setUndoStack((prev) => [ - ...prev, - { + setUndoStack((prev) => { + const entry: UndoSnapshot = { actionKey: key, restore: () => { setEdges(prevEdges); @@ -348,8 +367,10 @@ export function useBuilderChatPanel({ return next; }); }, - }, - ]); + }; + const trimmed = prev.length >= MAX_UNDO ? prev.slice(1) : prev; + return [...trimmed, entry]; + }); setEdges([ ...prevEdges, { @@ -370,12 +391,15 @@ export function useBuilderChatPanel({ } function handleUndoLastAction() { - setUndoStack((prev) => { - if (prev.length === 0) return prev; - const last = prev[prev.length - 1]; - last.restore(); - return prev.slice(0, -1); - }); + // Read the current stack directly rather than inside the setUndoStack updater. + // Calling restore() (which triggers setNodes/setEdges) inside a state updater + // is a React anti-pattern — state updaters must be pure. Reading from the ref + // here is safe because this function is only called from event handlers. + const stack = undoStack; + if (stack.length === 0) return; + const last = stack[stack.length - 1]; + last.restore(); + setUndoStack((prev) => prev.slice(0, -1)); } return {