fix(frontend/builder): address round-5 review comments on BuilderChatPanel

- 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
This commit is contained in:
majdyz
2026-04-13 04:01:42 +00:00
parent ed65756d58
commit 3b7e678b97
8 changed files with 144 additions and 23 deletions

View File

@@ -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" }),
);
});
});
// -----------------------------------------------------------------------

View File

@@ -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", () => {

View File

@@ -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());

View File

@@ -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 ??

View File

@@ -60,6 +60,7 @@ function ActionItem({ action, nodeMap, isApplied, onApply }: ActionItemProps) {
</span>
) : (
<button
type="button"
onClick={() => onApply(action)}
aria-label={`Apply: ${label}`}
className="shrink-0 rounded bg-violet-100 px-2 py-0.5 text-xs font-medium text-violet-700 hover:bg-violet-200"

View File

@@ -110,6 +110,7 @@ export 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
type="button"
onClick={onRetry}
className="mt-1 underline hover:no-underline"
>
@@ -148,16 +149,27 @@ export function MessageList({
: "bg-slate-100 text-slate-800",
)}
>
{msg.role === "assistant"
? (msg.parts ?? []).map((part, i) => (
<MessagePartRenderer
key={`${msg.id}-${i}`}
part={normalizePartForRenderer(part)}
messageID={msg.id}
partIndex={i}
/>
))
: textParts}
{msg.role === "assistant" ? (
<>
{/* Render plain text content directly — MessagePartRenderer only
handles tool parts; passing UITextPart to it is a type mismatch. */}
{textParts && <span>{textParts}</span>}
{/* Render tool parts (edit_agent, run_agent, etc.) via the shared
copilot renderer which knows how to display each tool type. */}
{(msg.parts ?? [])
.filter(isDynamicToolPart)
.map((part, i) => (
<MessagePartRenderer
key={`${msg.id}-tool-${i}`}
part={normalizePartForRenderer(part)}
messageID={msg.id}
partIndex={i}
/>
))}
</>
) : (
textParts
)}
</div>
);
})}

View File

@@ -49,17 +49,19 @@ export function PanelInput({
/>
{isStreaming ? (
<button
type="button"
onClick={onStop}
className="flex h-9 w-9 items-center justify-center rounded-lg bg-red-100 text-red-600 transition-colors hover:bg-red-200"
className="flex h-9 w-9 items-center justify-center rounded-lg bg-red-100 text-red-600 transition-colors hover:bg-red-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-400 focus-visible:ring-offset-2"
aria-label="Stop"
>
<StopCircle size={18} />
</button>
) : (
<button
type="button"
onClick={onSend}
disabled={isDisabled || !value.trim()}
className="flex h-9 w-9 items-center justify-center rounded-lg bg-violet-600 text-white transition-colors hover:bg-violet-700 disabled:opacity-40"
className="flex h-9 w-9 items-center justify-center rounded-lg bg-violet-600 text-white transition-colors hover:bg-violet-700 disabled:opacity-40 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-violet-400 focus-visible:ring-offset-2"
aria-label="Send"
>
<PaperPlaneTilt size={18} />

View File

@@ -71,10 +71,11 @@ function cacheSetSession(flowID: string, sessionId: string): void {
/** Stable empty array so the useShallow selector returns the same reference when the panel is closed. */
const EMPTY_NODES: never[] = [];
/** Clears the session cache. Exported only for use in tests. */
export function clearGraphSessionCacheForTesting() {
graphSessionCache.clear();
}
/**
* @internal Exposed only for unit tests via `__tests__/testUtils.ts`.
* Do not call from production code.
*/
export const _graphSessionCache = graphSessionCache;
interface UseBuilderChatPanelArgs {
isGraphLoaded?: boolean;