fix(frontend/builder): address reviewer comments on BuilderChatPanel

- Overlapping placeholders: add !seedMessage guard to empty-state block so the "Ask me to explain…" and "Graph context sent" banners are mutually exclusive - aria-modal without focus trap: replace role="dialog"/aria-modal="true" with role="complementary" since this is a side panel, not a blocking modal - Stale closure in handleApplyAction: use useNodeStore/useEdgeStore.getState() for both validation and mutation so rapid applies see live data - Gate nodes/edges Zustand subscriptions behind isOpen to prevent chat-panel hook re-running on every node drag/resize when panel is closed - inputValue not cleared on flowID change: add setInputValue("") to flowID reset - ReactMarkdown links: add custom <a> component with target="_blank" and rel="noopener noreferrer" - XML sanitization: apply sanitizeForXml() to n.id and edge handle names - Regex statefulness: move JSON_BLOCK_REGEX inside parseGraphActions() to avoid shared lastIndex state (eliminates fragile lastIndex=0 reset) - Type guard soundness: add typeof p.text === "string" to extractTextFromParts filter - Session ID validation: validate format before interpolating into streaming URL - Shallow-copy undo snapshots: spread prevNodes/prevEdges so closures hold independent arrays - Set spread optimisation: use new Set(prev).add(key) instead of new Set([...prev, key]) - Tests: remove dead getGetV1GetSpecificGraphQueryKey mock, add markerEnd assertion to connect_nodes tests, add transport prepareSendMessagesRequest coverage, add Enter-with-empty-input and inputValue-reset-on-flowID-change tests
2026-04-30 03:00:41 -04:00 · 2026-04-09 08:12:35 +07:00
parent 3107789867
commit abcf0830a6
5 changed files with 196 additions and 46 deletions
--- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx
+++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/BuilderChatPanel.tsx
@@ -74,10 +74,9 @@ export function BuilderChatPanel({ className, isGraphLoaded }: Props) {
    >
      {isOpen && (
        <div
-          role="dialog"
+          role="complementary"
          aria-label="Builder chat panel"
-          aria-modal="true"
-          className="pointer-events-auto flex h-[70vh] w-96 flex-col overflow-hidden rounded-xl border border-slate-200 bg-white shadow-2xl"
+          className="pointer-events-auto flex h-[70vh] w-96 max-w-[calc(100vw-2rem)] flex-col overflow-hidden rounded-xl border border-slate-200 bg-white shadow-2xl"
        >
          <PanelHeader
            onClose={handleToggle}
@@ -230,16 +229,23 @@ function MessageList({
        </div>
      )}

-      {visibleMessages.length === 0 && !isCreatingSession && !sessionError && (
-        <div className="flex flex-col items-center gap-2 py-6 text-center text-xs text-slate-400">
-          <ChatCircle size={28} weight="duotone" className="text-violet-300" />
-          <p>Ask me to explain or modify your agent.</p>
-          <p className="text-slate-300">
-            You can say things like &ldquo;What does this agent do?&rdquo; or
-            &ldquo;Add a step that formats the output.&rdquo;
-          </p>
-        </div>
-      )}
+      {visibleMessages.length === 0 &&
+        !isCreatingSession &&
+        !sessionError &&
+        !messages.some((m) => m.id === seedMessageId) && (
+          <div className="flex flex-col items-center gap-2 py-6 text-center text-xs text-slate-400">
+            <ChatCircle
+              size={28}
+              weight="duotone"
+              className="text-violet-300"
+            />
+            <p>Ask me to explain or modify your agent.</p>
+            <p className="text-slate-300">
+              You can say things like &ldquo;What does this agent do?&rdquo; or
+              &ldquo;Add a step that formats the output.&rdquo;
+            </p>
+          </div>
+        )}

      {visibleMessages.length === 0 &&
        messages.some((m) => m.id === seedMessageId) && (
@@ -281,6 +287,16 @@ function MessageList({
                      {children}
                    </pre>
                  ),
+                  a: ({ href, children }) => (
+                    <a
+                      href={href}
+                      target="_blank"
+                      rel="noopener noreferrer"
+                      className="underline hover:no-underline"
+                    >
+                      {children}
+                    </a>
+                  ),
                }}
              >
                {textParts}
--- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/tests/BuilderChatPanel.test.tsx
+++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/tests/BuilderChatPanel.test.tsx
@@ -345,10 +345,10 @@ describe("BuilderChatPanel", () => {
    expect(retrySession).toHaveBeenCalledOnce();
  });

-  it("renders the panel with role=dialog and message list with role=log", () => {
+  it("renders the panel with role=complementary and message list with role=log", () => {
    mockUseBuilderChatPanel.mockReturnValue(makeMockHook({ isOpen: true }));
    render(<BuilderChatPanel />);
-    expect(screen.getByRole("dialog")).toBeDefined();
+    expect(screen.getByRole("complementary")).toBeDefined();
    expect(screen.getByRole("log")).toBeDefined();
  });

--- 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
@@ -44,10 +44,6 @@ vi.mock("@/app/api/__generated__/endpoints/chat/chat", () => ({
  postV2CreateSession: (...args: unknown[]) => mockPostV2CreateSession(...args),
 }));

-vi.mock("@/app/api/__generated__/endpoints/graphs/graphs", () => ({
-  getGetV1GetSpecificGraphQueryKey: (id: string) => ["graphs", id],
-}));
-
 vi.mock("@/lib/supabase/actions", () => ({
  getWebSocketToken: vi.fn().mockResolvedValue({ token: "tok", error: null }),
 }));
@@ -409,6 +405,7 @@ describe("useBuilderChatPanel – handleApplyAction", () => {
          sourceHandle: "output",
          targetHandle: "input",
          type: "custom",
+          markerEnd: expect.objectContaining({ type: "arrowclosed" }),
        }),
      ]),
    );
@@ -581,6 +578,7 @@ describe("useBuilderChatPanel – handleApplyAction", () => {
          sourceHandle: "result",
          targetHandle: "input",
          type: "custom",
+          markerEnd: expect.objectContaining({ type: "arrowclosed" }),
        }),
      ]),
    );
@@ -1148,3 +1146,113 @@ describe("useBuilderChatPanel – handleUndoLastAction on empty stack", () => {
    expect(result.current.undoStack).toHaveLength(0);
  });
 });
+
+describe("useBuilderChatPanel – transport prepareSendMessagesRequest", () => {
+  it("calls getWebSocketToken and returns correct request body", async () => {
+    const { getWebSocketToken } = await import("@/lib/supabase/actions");
+    const { DefaultChatTransport } = await import("ai");
+    const MockTransport = DefaultChatTransport as ReturnType<typeof vi.fn>;
+
+    mockPostV2CreateSession.mockResolvedValue({
+      status: 200,
+      data: { id: "sess-transport" },
+    });
+
+    const { result } = renderHook(() => useBuilderChatPanel());
+
+    await openAndFlush(() => result.current.handleToggle());
+
+    expect(MockTransport).toHaveBeenCalled();
+    const ctorArg = MockTransport.mock.calls[
+      MockTransport.mock.calls.length - 1
+    ][0] as {
+      prepareSendMessagesRequest: (args: {
+        messages: unknown[];
+      }) => Promise<unknown>;
+    };
+    expect(typeof ctorArg.prepareSendMessagesRequest).toBe("function");
+
+    const messages = [
+      { role: "user", parts: [{ type: "text", text: "hello" }] },
+    ];
+    const req = await ctorArg.prepareSendMessagesRequest({ messages });
+
+    expect(getWebSocketToken).toHaveBeenCalled();
+    expect(req).toMatchObject({
+      body: { message: "hello", is_user_message: true },
+      headers: { Authorization: "Bearer tok" },
+    });
+  });
+
+  it("throws when getWebSocketToken returns null token", async () => {
+    const { getWebSocketToken } = await import("@/lib/supabase/actions");
+    const { DefaultChatTransport } = await import("ai");
+    const MockTransport = DefaultChatTransport as ReturnType<typeof vi.fn>;
+
+    vi.mocked(getWebSocketToken).mockResolvedValueOnce({
+      token: null,
+      error: "auth failed",
+    });
+
+    mockPostV2CreateSession.mockResolvedValue({
+      status: 200,
+      data: { id: "sess-auth-fail" },
+    });
+
+    const { result } = renderHook(() => useBuilderChatPanel());
+
+    await openAndFlush(() => result.current.handleToggle());
+
+    const ctorArg = MockTransport.mock.calls[
+      MockTransport.mock.calls.length - 1
+    ][0] as {
+      prepareSendMessagesRequest: (args: {
+        messages: unknown[];
+      }) => Promise<unknown>;
+    };
+    const messages = [{ role: "user", parts: [{ type: "text", text: "hi" }] }];
+    await expect(
+      ctorArg.prepareSendMessagesRequest({ messages }),
+    ).rejects.toThrow("Authentication failed");
+  });
+});
+
+describe("useBuilderChatPanel – handleKeyDown empty input guard", () => {
+  it("does NOT call sendMessage on Enter when inputValue is empty", async () => {
+    mockPostV2CreateSession.mockResolvedValue({
+      status: 200,
+      data: { id: "sess-empty" },
+    });
+    const { result } = renderHook(() => useBuilderChatPanel());
+
+    await openAndFlush(() => result.current.handleToggle());
+
+    const mockPreventDefault = vi.fn();
+    act(() => {
+      result.current.handleKeyDown({
+        key: "Enter",
+        shiftKey: false,
+        preventDefault: mockPreventDefault,
+      } as unknown as import("react").KeyboardEvent<HTMLTextAreaElement>);
+    });
+
+    expect(mockSendMessage).not.toHaveBeenCalled();
+  });
+});
+
+describe("useBuilderChatPanel – inputValue resets on flowID change", () => {
+  it("clears inputValue when flowID changes", () => {
+    mockFlowID = "flow-a";
+    const { result, rerender } = renderHook(() => useBuilderChatPanel());
+
+    act(() => {
+      result.current.setInputValue("typed text");
+    });
+    expect(result.current.inputValue).toBe("typed text");
+
+    mockFlowID = "flow-b";
+    rerender();
+
+    expect(result.current.inputValue).toBe("");
+  });
+});
--- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/helpers.ts
+++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/helpers.ts
@@ -7,8 +7,6 @@ const MAX_NODES = 100;
 const MAX_EDGES = 200;
 /** Maximum characters of a node description included in the seed prompt. */
 const MAX_DESC_CHARS = 500;
-/** Matches fenced JSON code blocks in AI responses. Module-scoped to avoid recompilation. */
-const JSON_BLOCK_REGEX = /```(?:json)?\s*\n?([\s\S]*?)\n?```/g;

 /** Escapes XML special characters in user-controlled strings before embedding in prompts. */
 function sanitizeForXml(s: string): string {
@@ -64,7 +62,7 @@ export function serializeGraphForChat(
    const name = sanitizeForXml(getNodeDisplayName(n, ""));
    const rawDesc = n.data.description?.slice(0, MAX_DESC_CHARS) ?? "";
    const desc = rawDesc ? ` — ${sanitizeForXml(rawDesc)}` : "";
-    return `- Node ${n.id}: "${name}"${desc}`;
+    return `- Node ${sanitizeForXml(n.id)}: "${name}"${desc}`;
  });

  const truncationNote =
@@ -82,7 +80,7 @@ export function serializeGraphForChat(
    const tgtName = sanitizeForXml(
      getNodeDisplayName(nodeMap.get(e.target), e.target),
    );
-    return `- "${srcName}" (${e.sourceHandle}) → "${tgtName}" (${e.targetHandle})`;
+    return `- "${srcName}" (${sanitizeForXml(e.sourceHandle ?? "")}) → "${tgtName}" (${sanitizeForXml(e.targetHandle ?? "")})`;
  });

  const edgeTruncationNote =
@@ -168,7 +166,10 @@ export function extractTextFromParts(
  parts: ReadonlyArray<{ type: string; text?: string }> | null | undefined,
 ): string {
  return (parts ?? [])
-    .filter((p): p is { type: "text"; text: string } => p.type === "text")
+    .filter(
+      (p): p is { type: "text"; text: string } =>
+        p.type === "text" && typeof p.text === "string",
+    )
    .map((p) => p.text)
    .join("");
 }
@@ -186,10 +187,10 @@ export function extractTextFromParts(
 */
 export function parseGraphActions(text: string): GraphAction[] {
  const actions: GraphAction[] = [];
-  JSON_BLOCK_REGEX.lastIndex = 0;
+  const jsonBlockRegex = /```(?:json)?\s*\n?([\s\S]*?)\n?```/g;
  let match: RegExpExecArray | null;

-  while ((match = JSON_BLOCK_REGEX.exec(text)) !== null) {
+  while ((match = jsonBlockRegex.exec(text)) !== null) {
    try {
      const parsed = JSON.parse(match[1]) as unknown;
      if (
--- a/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts
+++ b/autogpt_platform/frontend/src/app/(platform)/build/components/BuilderChatPanel/useBuilderChatPanel.ts
@@ -66,8 +66,8 @@ export function useBuilderChatPanel({
  const [{ flowID }] = useQueryStates({ flowID: parseAsString });
  const { toast } = useToast();

-  const nodes = useNodeStore(useShallow((s) => s.nodes));
-  const edges = useEdgeStore(useShallow((s) => s.edges));
+  const nodes = useNodeStore(useShallow((s) => (isOpen ? s.nodes : [])));
+  const edges = useEdgeStore(useShallow((s) => (isOpen ? s.edges : [])));
  const setNodes = useNodeStore((s) => s.setNodes);
  const setEdges = useEdgeStore((s) => s.setEdges);

@@ -78,6 +78,7 @@ export function useBuilderChatPanel({
    setSessionError(false);
    setAppliedActionKeys(new Set());
    setUndoStack([]);
+    setInputValue("");
    hasSentSeedMessageRef.current = false;
    // Also reset the creation ref so a new session can be started after
    // navigation, even if one was in-flight when flowID changed.
@@ -101,7 +102,15 @@ export function useBuilderChatPanel({
        const res = await postV2CreateSession(null);
        if (cancelled) return;
        if (res.status === 200) {
-          setSessionId(res.data.id);
+          const id = res.data.id;
+          // Validate the session ID is a safe non-empty identifier before
+          // interpolating it into the streaming URL — rejects values that
+          // contain path-traversal characters or whitespace.
+          if (typeof id !== "string" || !id || !/^[\w-]+$/i.test(id)) {
+            setSessionError(true);
+            return;
+          }
+          setSessionId(id);
        } else {
          setSessionError(true);
        }
@@ -256,7 +265,10 @@ export function useBuilderChatPanel({

  function handleApplyAction(action: GraphAction) {
    if (action.type === "update_node_input") {
-      const node = nodes.find((n) => n.id === action.nodeId);
+      // Read live state for both validation and mutation so rapid successive
+      // applies see the latest nodes rather than a stale render-cycle snapshot.
+      const liveNodes = useNodeStore.getState().nodes;
+      const node = liveNodes.find((n) => n.id === action.nodeId);
      if (!node) {
        toast({
          title: "Cannot apply change",
@@ -276,12 +288,15 @@ export function useBuilderChatPanel({
        });
        return;
      }
-      // Capture a full nodes snapshot before mutating. Both the apply and the
-      // restore use setNodes (not updateNodeData) to bypass the global history
-      // store — this keeps chat-panel changes completely separate from Ctrl+Z,
-      // preventing the "Applied" badge from going stale after a global undo.
-      const prevNodes = useNodeStore.getState().nodes;
-      const nextNodes = prevNodes.map((n) =>
+      // Capture a shallow-copied nodes snapshot before mutating. Spreading
+      // ensures the undo restore references an independent array rather than
+      // the same reference that the store may update in-place.
+      // Both the apply and the restore use setNodes (not updateNodeData) to
+      // bypass the global history store — this keeps chat-panel changes
+      // completely separate from Ctrl+Z, preventing the "Applied" badge from
+      // going stale after a global undo.
+      const prevNodes = [...liveNodes];
+      const nextNodes = liveNodes.map((n) =>
        n.id === action.nodeId
          ? {
              ...n,
@@ -313,8 +328,11 @@ export function useBuilderChatPanel({
      });
      setNodes(nextNodes);
    } else if (action.type === "connect_nodes") {
-      const sourceNode = nodes.find((n) => n.id === action.source);
-      const targetNode = nodes.find((n) => n.id === action.target);
+      // Read live state so validation reflects the current graph even when
+      // multiple actions are applied within the same render cycle.
+      const liveNodes = useNodeStore.getState().nodes;
+      const sourceNode = liveNodes.find((n) => n.id === action.source);
+      const targetNode = liveNodes.find((n) => n.id === action.target);
      if (!sourceNode || !targetNode) {
        toast({
          title: "Cannot apply connection",
@@ -343,10 +361,11 @@ export function useBuilderChatPanel({
        return;
      }
      const edgeId = `${action.source}:${action.sourceHandle}->${action.target}:${action.targetHandle}`;
-      // Capture a full edges snapshot before mutating. Both the apply and the
-      // 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;
+      // Shallow-copy the edges snapshot so the undo restore references an
+      // independent array rather than the same reference the store may update.
+      // Both the apply and the restore use setEdges (not addEdge/removeEdge)
+      // to bypass the global history store — keeps chat-panel changes separate.
+      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(
@@ -358,9 +377,11 @@ export function useBuilderChatPanel({
      );
      if (alreadyExists) {
        // Edge already present — mark as applied without duplicating it.
-        setAppliedActionKeys(
-          (prev) => new Set([...prev, getActionKey(action)]),
-        );
+        setAppliedActionKeys((prev) => {
+          const next = new Set(prev);
+          next.add(getActionKey(action));
+          return next;
+        });
        return;
      }
      const key = getActionKey(action);
@@ -402,7 +423,11 @@ export function useBuilderChatPanel({
      const _: never = action;
      return _;
    }
-    setAppliedActionKeys((prev) => new Set([...prev, getActionKey(action)]));
+    setAppliedActionKeys((prev) => {
+      const next = new Set(prev);
+      next.add(getActionKey(action));
+      return next;
+    });
  }

  function handleUndoLastAction() {