mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-30 03:00:41 -04:00
fix(frontend/builder): address review comments on builder chat panel
- Replace fragile setTimeout double-toggle retry with dedicated retrySession() callback that resets sessionError and lets the session-creation effect re-run - Remove invalidateQueries after apply actions — caused server refetch to overwrite local Zustand state changes (sentry HIGH severity bug) - Deep-clone prevHardcoded before undo capture so sequential applies to the same node each have an independent snapshot - Remove unsolicited "What does this agent do?" question from seed prompt; invite user to initiate instead - Remove useCallback from handleUndoLastAction per project convention - Remove unused sendMessage and status from hook return - Remove JSDoc comment from BuilderChatPanel per project convention - Hoist nodeMap construction from ActionItem to parent parsedActions.map to avoid N identical Maps per render cycle - Make useChat mock configurable (mockChatMessages/mockChatStatus) and add tests for parsedActions integration, Escape key handler, retrySession, and handleSend input-clearing behavior
This commit is contained in:
@@ -26,17 +26,11 @@ interface Props {
|
||||
isGraphLoaded?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* BuilderChatPanel renders a collapsible AI chat panel for the flow builder.
|
||||
* All business logic lives in `useBuilderChatPanel`.
|
||||
*
|
||||
* `isGraphLoaded` controls when the seed message is sent to the AI — pass
|
||||
* `true` only once the graph has finished loading so the AI receives full context.
|
||||
*/
|
||||
export function BuilderChatPanel({ className, isGraphLoaded }: Props) {
|
||||
const {
|
||||
isOpen,
|
||||
handleToggle,
|
||||
retrySession,
|
||||
messages,
|
||||
stop,
|
||||
error,
|
||||
@@ -99,7 +93,7 @@ export function BuilderChatPanel({ className, isGraphLoaded }: Props) {
|
||||
parsedActions={parsedActions}
|
||||
appliedActionKeys={appliedActionKeys}
|
||||
onApplyAction={handleApplyAction}
|
||||
onRetry={handleToggle}
|
||||
onRetry={retrySession}
|
||||
seedMessageId={seedMessageId}
|
||||
messagesEndRef={messagesEndRef}
|
||||
/>
|
||||
@@ -220,11 +214,7 @@ function MessageList({
|
||||
<div className="rounded-lg border border-red-100 bg-red-50 px-3 py-2 text-xs text-red-600">
|
||||
<p>Failed to start chat session.</p>
|
||||
<button
|
||||
onClick={() => {
|
||||
onRetry();
|
||||
// Toggle twice: close then reopen to re-trigger session creation
|
||||
setTimeout(onRetry, 50);
|
||||
}}
|
||||
onClick={onRetry}
|
||||
className="mt-1 underline hover:no-underline"
|
||||
>
|
||||
Retry
|
||||
@@ -305,18 +295,21 @@ function MessageList({
|
||||
<p className="text-xs font-medium text-violet-700">
|
||||
Suggested changes
|
||||
</p>
|
||||
{parsedActions.map((action) => {
|
||||
const key = getActionKey(action);
|
||||
return (
|
||||
<ActionItem
|
||||
key={key}
|
||||
action={action}
|
||||
nodes={nodes}
|
||||
isApplied={appliedActionKeys.has(key)}
|
||||
onApply={onApplyAction}
|
||||
/>
|
||||
);
|
||||
})}
|
||||
{(() => {
|
||||
const nodeMap = new Map(nodes.map((n) => [n.id, n]));
|
||||
return parsedActions.map((action) => {
|
||||
const key = getActionKey(action);
|
||||
return (
|
||||
<ActionItem
|
||||
key={key}
|
||||
action={action}
|
||||
nodeMap={nodeMap}
|
||||
isApplied={appliedActionKeys.has(key)}
|
||||
onApply={onApplyAction}
|
||||
/>
|
||||
);
|
||||
});
|
||||
})()}
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -327,17 +320,15 @@ function MessageList({
|
||||
|
||||
function ActionItem({
|
||||
action,
|
||||
nodes,
|
||||
nodeMap,
|
||||
isApplied,
|
||||
onApply,
|
||||
}: {
|
||||
action: GraphAction;
|
||||
nodes: CustomNode[];
|
||||
nodeMap: Map<string, CustomNode>;
|
||||
isApplied: boolean;
|
||||
onApply: (action: GraphAction) => void;
|
||||
}) {
|
||||
const nodeMap = new Map(nodes.map((n) => [n.id, n]));
|
||||
|
||||
const label =
|
||||
action.type === "update_node_input"
|
||||
? `Set "${getNodeDisplayName(nodeMap.get(action.nodeId), action.nodeId)}" "${action.key}" = ${JSON.stringify(action.value)}`
|
||||
|
||||
@@ -32,10 +32,9 @@ function makeMockHook(
|
||||
return {
|
||||
isOpen: false,
|
||||
handleToggle: vi.fn(),
|
||||
retrySession: vi.fn(),
|
||||
messages: [],
|
||||
sendMessage: vi.fn(),
|
||||
stop: vi.fn(),
|
||||
status: "ready",
|
||||
error: undefined,
|
||||
isCreatingSession: false,
|
||||
sessionError: false,
|
||||
@@ -335,12 +334,15 @@ describe("BuilderChatPanel", () => {
|
||||
});
|
||||
|
||||
it("shows session error message with Retry when sessionError is true", () => {
|
||||
const retrySession = vi.fn();
|
||||
mockUseBuilderChatPanel.mockReturnValue(
|
||||
makeMockHook({ isOpen: true, sessionError: true }),
|
||||
makeMockHook({ isOpen: true, sessionError: true, retrySession }),
|
||||
);
|
||||
render(<BuilderChatPanel />);
|
||||
expect(screen.getByText(/Failed to start chat session/i)).toBeDefined();
|
||||
expect(screen.getByText("Retry")).toBeDefined();
|
||||
fireEvent.click(screen.getByText("Retry"));
|
||||
expect(retrySession).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("renders the panel with role=dialog and message list with role=log", () => {
|
||||
@@ -751,9 +753,15 @@ describe("buildSeedPrompt", () => {
|
||||
expect(result).toContain('"action": "connect_nodes"');
|
||||
});
|
||||
|
||||
it("ends with a question to prompt an AI response", () => {
|
||||
it("ends with a prompt inviting the user to interact", () => {
|
||||
const result = buildSeedPrompt("");
|
||||
expect(result.trim().endsWith("What does this agent do?")).toBe(true);
|
||||
expect(
|
||||
result
|
||||
.trim()
|
||||
.endsWith(
|
||||
"Ask me what you'd like to know about or change in this agent.",
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -60,12 +60,14 @@ vi.mock("@/components/molecules/Toast/use-toast", () => ({
|
||||
|
||||
const mockSendMessage = vi.fn();
|
||||
const mockStop = vi.fn();
|
||||
let mockChatMessages: unknown[] = [];
|
||||
let mockChatStatus = "ready";
|
||||
vi.mock("@ai-sdk/react", () => ({
|
||||
useChat: () => ({
|
||||
messages: [],
|
||||
messages: mockChatMessages,
|
||||
sendMessage: mockSendMessage,
|
||||
stop: mockStop,
|
||||
status: "ready",
|
||||
status: mockChatStatus,
|
||||
error: undefined,
|
||||
}),
|
||||
}));
|
||||
@@ -91,6 +93,8 @@ beforeEach(() => {
|
||||
mockFlowID = null;
|
||||
mockNodes.length = 0;
|
||||
mockEdges.length = 0;
|
||||
mockChatMessages = [];
|
||||
mockChatStatus = "ready";
|
||||
mockUpdateNodeData.mockClear();
|
||||
mockAddEdge.mockClear();
|
||||
mockRemoveEdge.mockClear();
|
||||
@@ -306,8 +310,8 @@ describe("useBuilderChatPanel – flowID reset", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – cache invalidation", () => {
|
||||
it("invalidates graph query cache after applying an update_node_input action", () => {
|
||||
describe("useBuilderChatPanel – apply does not trigger cache refetch", () => {
|
||||
it("does NOT call invalidateQueries after applying an update_node_input action (prevents refetch overwriting local state)", () => {
|
||||
mockNodes.push({
|
||||
id: "n1",
|
||||
data: { hardcodedValues: { existing: "val" } },
|
||||
@@ -325,44 +329,6 @@ describe("useBuilderChatPanel – cache invalidation", () => {
|
||||
});
|
||||
});
|
||||
|
||||
expect(mockInvalidateQueries).toHaveBeenCalledWith({
|
||||
queryKey: ["graphs", "flow-cache"],
|
||||
});
|
||||
});
|
||||
|
||||
it("invalidates graph query cache after applying a connect_nodes action", () => {
|
||||
mockNodes.push({ id: "src", data: {} }, { id: "tgt", data: {} });
|
||||
mockFlowID = "flow-edges";
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
act(() => {
|
||||
result.current.handleApplyAction({
|
||||
type: "connect_nodes",
|
||||
source: "src",
|
||||
target: "tgt",
|
||||
sourceHandle: "out",
|
||||
targetHandle: "in",
|
||||
});
|
||||
});
|
||||
|
||||
expect(mockInvalidateQueries).toHaveBeenCalledWith({
|
||||
queryKey: ["graphs", "flow-edges"],
|
||||
});
|
||||
});
|
||||
|
||||
it("does NOT invalidate cache when validation fails (node not found)", () => {
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
act(() => {
|
||||
result.current.handleApplyAction({
|
||||
type: "update_node_input",
|
||||
nodeId: "nonexistent",
|
||||
key: "query",
|
||||
value: "test",
|
||||
});
|
||||
});
|
||||
|
||||
expect(mockInvalidateQueries).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -693,3 +659,162 @@ describe("useBuilderChatPanel – undo", () => {
|
||||
expect(result.current.appliedActionKeys.size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – parsedActions integration", () => {
|
||||
it("returns parsed actions from assistant messages when status is ready", () => {
|
||||
mockChatMessages = [
|
||||
{
|
||||
id: "msg-1",
|
||||
role: "assistant",
|
||||
parts: [
|
||||
{
|
||||
type: "text",
|
||||
text: '```json\n{"action":"update_node_input","node_id":"n1","key":"query","value":"AI news"}\n```',
|
||||
},
|
||||
],
|
||||
},
|
||||
];
|
||||
mockChatStatus = "ready";
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
expect(result.current.parsedActions).toHaveLength(1);
|
||||
expect(result.current.parsedActions[0]).toEqual({
|
||||
type: "update_node_input",
|
||||
nodeId: "n1",
|
||||
key: "query",
|
||||
value: "AI news",
|
||||
});
|
||||
});
|
||||
|
||||
it("returns empty parsedActions when status is streaming", () => {
|
||||
mockChatMessages = [
|
||||
{
|
||||
id: "msg-1",
|
||||
role: "assistant",
|
||||
parts: [
|
||||
{
|
||||
type: "text",
|
||||
text: '```json\n{"action":"update_node_input","node_id":"n1","key":"query","value":"AI news"}\n```',
|
||||
},
|
||||
],
|
||||
},
|
||||
];
|
||||
mockChatStatus = "streaming";
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
expect(result.current.parsedActions).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("deduplicates identical actions from multiple assistant messages", () => {
|
||||
const actionBlock =
|
||||
'```json\n{"action":"update_node_input","node_id":"n1","key":"query","value":"AI news"}\n```';
|
||||
mockChatMessages = [
|
||||
{
|
||||
id: "msg-1",
|
||||
role: "assistant",
|
||||
parts: [{ type: "text", text: actionBlock }],
|
||||
},
|
||||
{
|
||||
id: "msg-2",
|
||||
role: "assistant",
|
||||
parts: [{ type: "text", text: actionBlock }],
|
||||
},
|
||||
];
|
||||
mockChatStatus = "ready";
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
expect(result.current.parsedActions).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – Escape key handler", () => {
|
||||
it("closes the panel when Escape is pressed while open", () => {
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
act(() => {
|
||||
result.current.handleToggle();
|
||||
});
|
||||
expect(result.current.isOpen).toBe(true);
|
||||
|
||||
act(() => {
|
||||
document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" }));
|
||||
});
|
||||
expect(result.current.isOpen).toBe(false);
|
||||
});
|
||||
|
||||
it("does not error when Escape is pressed while panel is closed", () => {
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
expect(result.current.isOpen).toBe(false);
|
||||
|
||||
act(() => {
|
||||
document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" }));
|
||||
});
|
||||
|
||||
expect(result.current.isOpen).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – retrySession", () => {
|
||||
it("clears sessionError so the session-creation effect can re-run", async () => {
|
||||
mockPostV2CreateSession.mockRejectedValueOnce(new Error("network error"));
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
await openAndFlush(() => result.current.handleToggle());
|
||||
expect(result.current.sessionError).toBe(true);
|
||||
|
||||
mockPostV2CreateSession.mockResolvedValue({
|
||||
status: 200,
|
||||
data: { id: "sess-retry" },
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
result.current.retrySession();
|
||||
await new Promise<void>((resolve) => setTimeout(resolve, 0));
|
||||
});
|
||||
|
||||
expect(result.current.sessionError).toBe(false);
|
||||
expect(result.current.sessionId).toBe("sess-retry");
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – handleSend", () => {
|
||||
it("clears inputValue after sending when session is ready", async () => {
|
||||
mockPostV2CreateSession.mockResolvedValue({
|
||||
status: 200,
|
||||
data: { id: "sess-send" },
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
await openAndFlush(() => result.current.handleToggle());
|
||||
|
||||
act(() => {
|
||||
result.current.setInputValue("hello world");
|
||||
});
|
||||
|
||||
act(() => {
|
||||
result.current.handleSend();
|
||||
});
|
||||
|
||||
expect(result.current.inputValue).toBe("");
|
||||
expect(mockSendMessage).toHaveBeenCalledWith({ text: "hello world" });
|
||||
});
|
||||
|
||||
it("does not send when inputValue is whitespace only", () => {
|
||||
const { result } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
act(() => {
|
||||
result.current.setInputValue(" ");
|
||||
});
|
||||
|
||||
act(() => {
|
||||
result.current.handleSend();
|
||||
});
|
||||
|
||||
expect(mockSendMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -124,8 +124,8 @@ export function buildSeedPrompt(summary: string): string {
|
||||
`To add a connection between nodes:\n` +
|
||||
`\`\`\`json\n{"action": "connect_nodes", "source": "<source node id>", "target": "<target node id>", "source_handle": "<output handle name>", "target_handle": "<input handle name>"}\n\`\`\`\n\n` +
|
||||
`Rules: the "action" key is required and must be exactly "update_node_input" or "connect_nodes". ` +
|
||||
`Do not use any other field names (e.g. "block", "change", "field", "from", "to" are NOT valid).\n\n` +
|
||||
`What does this agent do?`
|
||||
`Do not use any other field names (e.g. "block", "change", "field", "from", "to" are NOT valid). ` +
|
||||
`Ask me what you'd like to know about or change in this agent.`
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -1,14 +1,11 @@
|
||||
import { postV2CreateSession } from "@/app/api/__generated__/endpoints/chat/chat";
|
||||
import { getGetV1GetSpecificGraphQueryKey } from "@/app/api/__generated__/endpoints/graphs/graphs";
|
||||
import { getWebSocketToken } from "@/lib/supabase/actions";
|
||||
import { environment } from "@/services/environment";
|
||||
import { useToast } from "@/components/molecules/Toast/use-toast";
|
||||
import { useQueryClient } from "@tanstack/react-query";
|
||||
import { useChat } from "@ai-sdk/react";
|
||||
import { DefaultChatTransport } from "ai";
|
||||
import {
|
||||
type KeyboardEvent,
|
||||
useCallback,
|
||||
useEffect,
|
||||
useMemo,
|
||||
useRef,
|
||||
@@ -41,15 +38,6 @@ interface UseBuilderChatPanelArgs {
|
||||
isGraphLoaded?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* useBuilderChatPanel manages all business logic for the collapsible AI chat
|
||||
* panel in the flow builder. It owns session lifecycle, streaming transport,
|
||||
* graph context serialization, action parsing, apply/undo, and input handling.
|
||||
*
|
||||
* @param isGraphLoaded - When true the seed message is sent automatically.
|
||||
* Pass false (the default) until the graph has finished loading to avoid
|
||||
* sending an empty context to the AI.
|
||||
*/
|
||||
export function useBuilderChatPanel({
|
||||
isGraphLoaded = false,
|
||||
}: UseBuilderChatPanelArgs = {}) {
|
||||
@@ -72,7 +60,6 @@ export function useBuilderChatPanel({
|
||||
const isCreatingSessionRef = useRef(false);
|
||||
|
||||
const [{ flowID }] = useQueryStates({ flowID: parseAsString });
|
||||
const queryClient = useQueryClient();
|
||||
const { toast } = useToast();
|
||||
|
||||
const nodes = useNodeStore(useShallow((s) => s.nodes));
|
||||
@@ -233,13 +220,16 @@ export function useBuilderChatPanel({
|
||||
Boolean(sessionId) && !isCreatingSession && !sessionError && !isStreaming;
|
||||
|
||||
function handleToggle() {
|
||||
// Reset session error when reopening so the panel can retry session creation
|
||||
if (!isOpen && !sessionId) {
|
||||
setSessionError(false);
|
||||
}
|
||||
setIsOpen((o) => !o);
|
||||
}
|
||||
|
||||
// Resets session error state so the session-creation effect re-runs on
|
||||
// the next render without toggling the panel closed and back open.
|
||||
function retrySession() {
|
||||
setSessionError(false);
|
||||
isCreatingSessionRef.current = false;
|
||||
}
|
||||
|
||||
function handleSend() {
|
||||
const text = inputValue.trim();
|
||||
if (!text || !canSend) return;
|
||||
@@ -276,8 +266,10 @@ export function useBuilderChatPanel({
|
||||
});
|
||||
return;
|
||||
}
|
||||
// Capture a snapshot before mutating so we can undo.
|
||||
const prevHardcoded = node.data.hardcodedValues;
|
||||
// Deep-clone before mutating so sequential applies to the same node
|
||||
// each capture an independent snapshot — without this, the reference
|
||||
// would point to the same object after mutation.
|
||||
const prevHardcoded = structuredClone(node.data.hardcodedValues);
|
||||
const key = getActionKey(action);
|
||||
setUndoStack((prev) => [
|
||||
...prev,
|
||||
@@ -361,29 +353,23 @@ export function useBuilderChatPanel({
|
||||
return _;
|
||||
}
|
||||
setAppliedActionKeys((prev) => new Set([...prev, getActionKey(action)]));
|
||||
if (flowID) {
|
||||
queryClient.invalidateQueries({
|
||||
queryKey: getGetV1GetSpecificGraphQueryKey(flowID),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
const handleUndoLastAction = useCallback(() => {
|
||||
function handleUndoLastAction() {
|
||||
setUndoStack((prev) => {
|
||||
if (prev.length === 0) return prev;
|
||||
const last = prev[prev.length - 1];
|
||||
last.restore();
|
||||
return prev.slice(0, -1);
|
||||
});
|
||||
}, []);
|
||||
}
|
||||
|
||||
return {
|
||||
isOpen,
|
||||
handleToggle,
|
||||
retrySession,
|
||||
messages,
|
||||
sendMessage,
|
||||
stop,
|
||||
status,
|
||||
error,
|
||||
isCreatingSession,
|
||||
sessionError,
|
||||
|
||||
Reference in New Issue
Block a user