fix: address code review findings from PR #14

- Add StatePanel.test.tsx with 12 tests covering render, delete/undo
  flow, confirm-delete for in-use fields, add field, node navigation
- Add removeStateFields readonly guard and addStateFields dedup tests
- Guard against NaN in LLMNodeConfig temperature/max_tokens handlers
- Clear undo timer on StatePanel unmount
- Remove api_key local var from models.py, use logger.error over
  logger.exception to avoid leaking keys in stack frames

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: prosdev

packages/canvas/src/components/panels/StatePanel.tsx

@@ -2,7 +2,7 @@ import { useCanvasContext } from "@contexts/CanvasContext";
 import type { NodeSchema, StateField } from "@shared/schema";
 import { useGraphStore } from "@store/graphSlice";
 import { Sheet } from "@ui/Sheet";
-import { useCallback, useMemo, useRef, useState } from "react";
+import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import { extractRootKey } from "../canvas/runInputUtils";
 import { AddFieldForm } from "./AddFieldForm";
 import { StateFieldRow } from "./StateFieldRow";
@@ -67,6 +67,13 @@ export function StatePanel() {
     [stateFields],
   );
 
+  // Clear undo timer on unmount to avoid setting state on unmounted component
+  useEffect(() => {
+    return () => {
+      if (undoTimerRef.current) clearTimeout(undoTimerRef.current);
+    };
+  }, []);
+
   const handleDelete = useCallback(
     (field: StateField) => {
       removeStateFields([field.key]);

packages/canvas/src/components/panels/__tests__/StatePanel.test.tsx

@@ -0,0 +1,172 @@
+import type { NodeSchema, StateField } from "@shared/schema";
+import { act, fireEvent, render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { StatePanel } from "../StatePanel";
+
+// Mock CanvasContext
+const mockSetSelectedNodeId = vi.fn();
+const mockSetStatePanelOpen = vi.fn();
+vi.mock("@contexts/CanvasContext", () => ({
+  useCanvasContext: () => ({
+    statePanelOpen: true,
+    setStatePanelOpen: mockSetStatePanelOpen,
+    setSelectedNodeId: mockSetSelectedNodeId,
+  }),
+}));
+
+// Mock graphSlice
+const mockAddStateFields = vi.fn();
+const mockRemoveStateFields = vi.fn();
+let mockState: StateField[] = [];
+let mockNodes: NodeSchema[] = [];
+
+vi.mock("@store/graphSlice", () => ({
+  useGraphStore: (selector: (s: Record<string, unknown>) => unknown) =>
+    selector({
+      graph: { state: mockState },
+      nodes: mockNodes,
+      addStateFields: mockAddStateFields,
+      removeStateFields: mockRemoveStateFields,
+    }),
+  DEFAULT_FIELD_KEYS: new Set(["messages", "user_input", "llm_response"]),
+}));
+
+const DEFAULT_FIELDS: StateField[] = [
+  { key: "messages", type: "list", reducer: "append", readonly: true },
+  { key: "user_input", type: "string", reducer: "replace" },
+  { key: "llm_response", type: "string", reducer: "replace" },
+];
+
+beforeEach(() => {
+  vi.clearAllMocks();
+  mockState = [...DEFAULT_FIELDS];
+  mockNodes = [];
+});
+
+describe("StatePanel", () => {
+  it("renders all state fields", () => {
+    render(<StatePanel />);
+    expect(screen.getByText("messages")).toBeInTheDocument();
+    expect(screen.getByText("user_input")).toBeInTheDocument();
+    expect(screen.getByText("llm_response")).toBeInTheDocument();
+  });
+
+  it("shows empty hint when no fields exist", () => {
+    mockState = [];
+    render(<StatePanel />);
+    expect(screen.getByText(/no state fields yet/i)).toBeInTheDocument();
+  });
+
+  it("shows readonly badge on readonly fields", () => {
+    render(<StatePanel />);
+    expect(screen.getByText("RO")).toBeInTheDocument();
+  });
+
+  it("hides delete button for readonly fields", () => {
+    render(<StatePanel />);
+    expect(screen.queryByLabelText("Delete messages")).not.toBeInTheDocument();
+  });
+
+  it("shows delete button for non-readonly fields", () => {
+    render(<StatePanel />);
+    expect(screen.getByLabelText("Delete user_input")).toBeInTheDocument();
+  });
+
+  it("calls removeStateFields when delete is clicked", () => {
+    render(<StatePanel />);
+    fireEvent.click(screen.getByLabelText("Delete user_input"));
+    expect(mockRemoveStateFields).toHaveBeenCalledWith(["user_input"]);
+  });
+
+  it("shows undo toast after delete", () => {
+    render(<StatePanel />);
+    fireEvent.click(screen.getByLabelText("Delete user_input"));
+    expect(screen.getByText(/removed field/i)).toBeInTheDocument();
+    expect(screen.getByText("Undo")).toBeInTheDocument();
+  });
+
+  it("undo toast disappears after 5 seconds", () => {
+    vi.useFakeTimers();
+    render(<StatePanel />);
+    fireEvent.click(screen.getByLabelText("Delete user_input"));
+    expect(screen.getByText("Undo")).toBeInTheDocument();
+    act(() => {
+      vi.advanceTimersByTime(5000);
+    });
+    expect(screen.queryByText("Undo")).not.toBeInTheDocument();
+    vi.useRealTimers();
+  });
+
+  it("undo restores the deleted field", () => {
+    render(<StatePanel />);
+    fireEvent.click(screen.getByLabelText("Delete user_input"));
+    fireEvent.click(screen.getByText("Undo"));
+    expect(mockAddStateFields).toHaveBeenCalledWith([
+      { key: "user_input", type: "string", reducer: "replace" },
+    ]);
+    expect(screen.queryByText("Undo")).not.toBeInTheDocument();
+  });
+
+  it("shows confirm dialog when deleting a field with usage", () => {
+    mockNodes = [
+      {
+        id: "llm-1",
+        type: "llm",
+        label: "Chat",
+        position: { x: 0, y: 0 },
+        config: {
+          provider: "openai",
+          model: "gpt-4o",
+          system_prompt: "",
+          temperature: 0.7,
+          max_tokens: 1024,
+          input_map: { prompt: "user_input" },
+          output_key: "llm_response",
+        },
+      } as NodeSchema,
+    ];
+    render(<StatePanel />);
+    fireEvent.click(screen.getByLabelText("Delete user_input"));
+    // First click shows confirmation, not delete
+    expect(mockRemoveStateFields).not.toHaveBeenCalled();
+    expect(screen.getByText(/referenced by nodes/i)).toBeInTheDocument();
+    // Click "Remove anyway" to actually delete
+    fireEvent.click(screen.getByText("Remove anyway"));
+    expect(mockRemoveStateFields).toHaveBeenCalledWith(["user_input"]);
+  });
+
+  it("adds a new field via AddFieldForm", async () => {
+    render(<StatePanel />);
+    const input = screen.getByPlaceholderText("field_name");
+    await userEvent.type(input, "custom_data");
+    fireEvent.click(screen.getByText("Add field"));
+    expect(mockAddStateFields).toHaveBeenCalledWith([
+      { key: "custom_data", type: "string", reducer: "replace" },
+    ]);
+  });
+
+  it("clicking a writer node name calls setSelectedNodeId", () => {
+    mockNodes = [
+      {
+        id: "llm-1",
+        type: "llm",
+        label: "ChatBot",
+        position: { x: 0, y: 0 },
+        config: {
+          provider: "openai",
+          model: "gpt-4o",
+          system_prompt: "",
+          temperature: 0.7,
+          max_tokens: 1024,
+          input_map: {},
+          output_key: "llm_response",
+        },
+      } as NodeSchema,
+    ];
+    render(<StatePanel />);
+    // llm_response is written by "ChatBot" node — click the usage link
+    const writerLinks = screen.getAllByText(/ChatBot/);
+    fireEvent.click(writerLinks[0] as HTMLElement);
+    expect(mockSetSelectedNodeId).toHaveBeenCalledWith("llm-1");
+  });
+});

packages/canvas/src/components/panels/config/LLMNodeConfig.tsx

@@ -174,14 +174,16 @@ function LLMNodeConfigComponent({ node, onChange }: LLMNodeConfigProps) {
 
   const handleTemperatureChange = useCallback(
     (e: ChangeEvent<HTMLInputElement>) => {
-      onChange({ config: { temperature: Number.parseFloat(e.target.value) } });
+      const parsed = Number.parseFloat(e.target.value);
+      if (!Number.isNaN(parsed)) onChange({ config: { temperature: parsed } });
     },
     [onChange],
   );
 
   const handleMaxTokensChange = useCallback(
     (e: ChangeEvent<HTMLInputElement>) => {
-      onChange({ config: { max_tokens: Number.parseInt(e.target.value, 10) } });
+      const parsed = Number.parseInt(e.target.value, 10);
+      if (!Number.isNaN(parsed)) onChange({ config: { max_tokens: parsed } });
     },
     [onChange],
   );

packages/canvas/src/store/__tests__/graphSlice.test.ts

@@ -155,4 +155,27 @@ describe("graphSlice", () => {
     expect(graph?.name).toBe("New Name");
     expect(dirty).toBe(true);
   });
+
+  it("removeStateFields ignores readonly fields", () => {
+    useGraphStore.getState().newGraph("G");
+    const before = useGraphStore.getState().graph?.state ?? [];
+    // messages is readonly, user_input is not
+    useGraphStore.getState().removeStateFields(["messages", "user_input"]);
+    const after = useGraphStore.getState().graph?.state ?? [];
+    expect(after.find((f) => f.key === "messages")).toBeDefined();
+    expect(after.find((f) => f.key === "user_input")).toBeUndefined();
+    expect(after).toHaveLength(before.length - 1);
+  });
+
+  it("addStateFields deduplicates against existing keys", () => {
+    useGraphStore.getState().newGraph("G");
+    const before = useGraphStore.getState().graph?.state ?? [];
+    useGraphStore
+      .getState()
+      .addStateFields([
+        { key: "messages", type: "string", reducer: "replace" },
+      ]);
+    const after = useGraphStore.getState().graph?.state ?? [];
+    expect(after).toHaveLength(before.length);
+  });
 });

packages/execution/app/models.py

@@ -29,8 +29,7 @@
 
 async def list_openai_models() -> list[str]:
     """List available OpenAI chat models."""
-    api_key = os.getenv("OPENAI_API_KEY")
-    if not api_key:
+    if not os.getenv("OPENAI_API_KEY"):
         return []
     try:
         from openai import AsyncOpenAI
@@ -43,19 +42,18 @@ async def list_openai_models() -> list[str]:
             if not m.id.startswith(_OPENAI_NON_CHAT_PREFIXES) and "realtime" not in m.id
         )
     except Exception:
-        logger.exception("Failed to list OpenAI models")
+        logger.error("Failed to list OpenAI models", exc_info=True)
         return []
 
 
 async def list_gemini_models() -> list[str]:
     """List available Gemini models that support generateContent."""
-    api_key = os.getenv("GEMINI_API_KEY")
-    if not api_key:
+    if not os.getenv("GEMINI_API_KEY"):
         return []
     try:
         import google.generativeai as genai
 
-        genai.configure(api_key=api_key)
+        genai.configure(api_key=os.getenv("GEMINI_API_KEY"))
         # genai.list_models() is synchronous — wrap to avoid blocking event loop
         raw_models = await asyncio.to_thread(genai.list_models)
         models = []
@@ -66,7 +64,7 @@ async def list_gemini_models() -> list[str]:
                     models.append(name)
         return sorted(models)
     except Exception:
-        logger.exception("Failed to list Gemini models")
+        logger.error("Failed to list Gemini models", exc_info=True)
         return []