Merge pull request #528 from nandsha/fix/validate-json-on-tool-execution

fix: Validate JSON parameters before tool execution

client/src/components/ToolsTab.tsx

@@ -1,11 +1,11 @@
-import { Alert, AlertDescription } from "@/components/ui/alert";
+import { Alert, AlertDescription, AlertTitle } from "@/components/ui/alert";
 import { Button } from "@/components/ui/button";
 import { Checkbox } from "@/components/ui/checkbox";
 import { Input } from "@/components/ui/input";
 import { Label } from "@/components/ui/label";
 import { TabsContent } from "@/components/ui/tabs";
 import { Textarea } from "@/components/ui/textarea";
-import DynamicJsonForm from "./DynamicJsonForm";
+import DynamicJsonForm, { DynamicJsonFormRef } from "./DynamicJsonForm";
 import type { JsonValue, JsonSchemaType } from "@/utils/jsonUtils";
 import {
   generateDefaultValue,
@@ -22,10 +22,11 @@ import {
   Send,
   ChevronDown,
   ChevronUp,
+  AlertCircle,
   Copy,
   CheckCheck,
 } from "lucide-react";
-import { useEffect, useState } from "react";
+import { useEffect, useState, useRef } from "react";
 import ListPane from "./ListPane";
 import JsonView from "./JsonView";
 import ToolResults from "./ToolResults";
@@ -45,6 +46,7 @@ const ToolsTab = ({
   setSelectedTool,
   toolResult,
   nextCursor,
+  error,
   resourceContent,
   onReadResource,
 }: {
@@ -64,9 +66,20 @@ const ToolsTab = ({
   const [isToolRunning, setIsToolRunning] = useState(false);
   const [isOutputSchemaExpanded, setIsOutputSchemaExpanded] = useState(false);
   const [isMetaExpanded, setIsMetaExpanded] = useState(false);
+  const [hasValidationErrors, setHasValidationErrors] = useState(false);
+  const formRefs = useRef<Record<string, DynamicJsonFormRef | null>>({});
   const { toast } = useToast();
   const { copied, setCopied } = useCopy();
 
+  // Function to check if any form has validation errors
+  const checkValidationErrors = () => {
+    const errors = Object.values(formRefs.current).some(
+      (ref) => ref && !ref.validateJson().isValid,
+    );
+    setHasValidationErrors(errors);
+    return errors;
+  };
+
   useEffect(() => {
     const params = Object.entries(
       selectedTool?.inputSchema.properties ?? [],
@@ -79,6 +92,12 @@ const ToolsTab = ({
       ),
     ]);
     setParams(Object.fromEntries(params));
+
+    // Reset validation errors when switching tools
+    setHasValidationErrors(false);
+
+    // Clear form refs for the previous tool
+    formRefs.current = {};
   }, [selectedTool]);
 
   return (
@@ -112,7 +131,13 @@ const ToolsTab = ({
             </h3>
           </div>
           <div className="p-4">
-            {selectedTool ? (
+            {error ? (
+              <Alert variant="destructive">
+                <AlertCircle className="h-4 w-4" />
+                <AlertTitle>Error</AlertTitle>
+                <AlertDescription>{error}</AlertDescription>
+              </Alert>
+            ) : selectedTool ? (
               <div className="space-y-4">
                 <p className="text-sm text-gray-600 dark:text-gray-400">
                   {selectedTool.description}
@@ -174,6 +199,7 @@ const ToolsTab = ({
                         ) : prop.type === "object" || prop.type === "array" ? (
                           <div className="mt-1">
                             <DynamicJsonForm
+                              ref={(ref) => (formRefs.current[key] = ref)}
                               schema={{
                                 type: prop.type,
                                 properties: prop.properties,
@@ -189,6 +215,8 @@ const ToolsTab = ({
                                   ...params,
                                   [key]: newValue,
                                 });
+                                // Check validation after a short delay to allow form to update
+                                setTimeout(checkValidationErrors, 100);
                               }}
                             />
                           </div>
@@ -212,6 +240,7 @@ const ToolsTab = ({
                         ) : (
                           <div className="mt-1">
                             <DynamicJsonForm
+                              ref={(ref) => (formRefs.current[key] = ref)}
                               schema={{
                                 type: prop.type,
                                 properties: prop.properties,
@@ -224,6 +253,8 @@ const ToolsTab = ({
                                   ...params,
                                   [key]: newValue,
                                 });
+                                // Check validation after a short delay to allow form to update
+                                setTimeout(checkValidationErrors, 100);
                               }}
                             />
                           </div>
@@ -305,6 +336,9 @@ const ToolsTab = ({
                 <div className="flex gap-2">
                   <Button
                     onClick={async () => {
+                      // Validate JSON inputs before calling tool
+                      if (checkValidationErrors()) return;
+
                       try {
                         setIsToolRunning(true);
                         await callTool(selectedTool.name, params);
@@ -312,7 +346,7 @@ const ToolsTab = ({
                         setIsToolRunning(false);
                       }
                     }}
-                    disabled={isToolRunning}
+                    disabled={isToolRunning || hasValidationErrors}
                   >
                     {isToolRunning ? (
                       <>

client/src/components/__tests__/DynamicJsonForm.test.tsx

@@ -1,7 +1,8 @@
 import { render, screen, fireEvent, waitFor } from "@testing-library/react";
 import "@testing-library/jest-dom";
 import { describe, it, expect, jest } from "@jest/globals";
-import DynamicJsonForm from "../DynamicJsonForm";
+import { useRef } from "react";
+import DynamicJsonForm, { DynamicJsonFormRef } from "../DynamicJsonForm";
 import type { JsonSchemaType } from "@/utils/jsonUtils";
 
 describe("DynamicJsonForm String Fields", () => {
@@ -663,3 +664,221 @@ describe("DynamicJsonForm Copy JSON Functionality", () => {
     });
   });
 });
+
+describe("DynamicJsonForm Validation Functionality", () => {
+  const renderFormWithRef = (props = {}) => {
+    const TestComponent = () => {
+      const formRef = useRef<DynamicJsonFormRef>(null);
+      const defaultProps = {
+        schema: {
+          type: "object",
+          properties: {
+            nested: { oneOf: [{ type: "string" }, { type: "integer" }] },
+          },
+        } as unknown as JsonSchemaType,
+        value: { nested: "test value" },
+        onChange: jest.fn(),
+        ref: formRef,
+      };
+
+      return (
+        <div>
+          <DynamicJsonForm {...defaultProps} {...props} />
+          <button
+            onClick={() => {
+              const result = formRef.current?.validateJson();
+              // Add data attributes to make validation result testable
+              const button = document.querySelector(
+                '[data-testid="validate-button"]',
+              ) as HTMLElement;
+              if (button && result) {
+                button.setAttribute(
+                  "data-validation-valid",
+                  result.isValid.toString(),
+                );
+                button.setAttribute(
+                  "data-validation-error",
+                  result.error || "",
+                );
+              }
+            }}
+            data-testid="validate-button"
+          >
+            Validate
+          </button>
+        </div>
+      );
+    };
+
+    return render(<TestComponent />);
+  };
+
+  describe("validateJson method", () => {
+    it("should return valid for form mode", () => {
+      const simpleSchema = {
+        type: "string" as const,
+        description: "Test string field",
+      };
+
+      const TestComponent = () => {
+        const formRef = useRef<DynamicJsonFormRef>(null);
+
+        return (
+          <div>
+            <DynamicJsonForm
+              ref={formRef}
+              schema={simpleSchema}
+              value="test"
+              onChange={jest.fn()}
+            />
+            <button
+              onClick={() => {
+                const result = formRef.current?.validateJson();
+                const button = document.querySelector(
+                  '[data-testid="validate-button"]',
+                ) as HTMLElement;
+                if (button && result) {
+                  button.setAttribute(
+                    "data-validation-valid",
+                    result.isValid.toString(),
+                  );
+                  button.setAttribute(
+                    "data-validation-error",
+                    result.error || "",
+                  );
+                }
+              }}
+              data-testid="validate-button"
+            >
+              Validate
+            </button>
+          </div>
+        );
+      };
+
+      render(<TestComponent />);
+
+      const validateButton = screen.getByTestId("validate-button");
+      fireEvent.click(validateButton);
+
+      expect(validateButton.getAttribute("data-validation-valid")).toBe("true");
+      expect(validateButton.getAttribute("data-validation-error")).toBe("");
+    });
+
+    it("should return valid for valid JSON in JSON mode", () => {
+      renderFormWithRef();
+
+      const validateButton = screen.getByTestId("validate-button");
+      fireEvent.click(validateButton);
+
+      expect(validateButton.getAttribute("data-validation-valid")).toBe("true");
+      expect(validateButton.getAttribute("data-validation-error")).toBe("");
+    });
+
+    it("should return invalid for malformed JSON in JSON mode", async () => {
+      renderFormWithRef();
+
+      // Enter invalid JSON
+      const textarea = screen.getByRole("textbox");
+      fireEvent.change(textarea, { target: { value: '{ "invalid": json }' } });
+
+      // Wait a bit for any debounced updates
+      await waitFor(() => {
+        const validateButton = screen.getByTestId("validate-button");
+        fireEvent.click(validateButton);
+
+        expect(validateButton.getAttribute("data-validation-valid")).toBe(
+          "false",
+        );
+        expect(validateButton.getAttribute("data-validation-error")).toContain(
+          "JSON",
+        );
+      });
+    });
+
+    it("should return valid for empty JSON in JSON mode", () => {
+      renderFormWithRef();
+
+      // Clear the textarea
+      const textarea = screen.getByRole("textbox");
+      fireEvent.change(textarea, { target: { value: "" } });
+
+      const validateButton = screen.getByTestId("validate-button");
+      fireEvent.click(validateButton);
+
+      expect(validateButton.getAttribute("data-validation-valid")).toBe("true");
+      expect(validateButton.getAttribute("data-validation-error")).toBe("");
+    });
+
+    it("should set error state when validation fails", async () => {
+      renderFormWithRef();
+
+      // Enter invalid JSON
+      const textarea = screen.getByRole("textbox");
+      fireEvent.change(textarea, {
+        target: { value: '{ "trailing": "comma", }' },
+      });
+
+      // Trigger validation
+      const validateButton = screen.getByTestId("validate-button");
+      fireEvent.click(validateButton);
+
+      // Check that validation result shows error
+      expect(validateButton.getAttribute("data-validation-valid")).toBe(
+        "false",
+      );
+      expect(validateButton.getAttribute("data-validation-error")).toContain(
+        "JSON",
+      );
+    });
+  });
+
+  describe("forwardRef functionality", () => {
+    it("should expose validateJson method through ref", () => {
+      const TestComponent = () => {
+        const formRef = useRef<DynamicJsonFormRef>(null);
+
+        return (
+          <div>
+            <DynamicJsonForm
+              ref={formRef}
+              schema={{
+                type: "object",
+                properties: {
+                  test: { type: "string" },
+                },
+              }}
+              value={{ test: "value" }}
+              onChange={jest.fn()}
+            />
+            <button
+              onClick={() => {
+                const hasValidateMethod =
+                  typeof formRef.current?.validateJson === "function";
+                const button = document.querySelector(
+                  '[data-testid="ref-test-button"]',
+                ) as HTMLElement;
+                if (button) {
+                  button.setAttribute(
+                    "data-has-validate-method",
+                    hasValidateMethod.toString(),
+                  );
+                }
+              }}
+              data-testid="ref-test-button"
+            >
+              Test Ref
+            </button>
+          </div>
+        );
+      };
+
+      render(<TestComponent />);
+
+      const testButton = screen.getByTestId("ref-test-button");
+      fireEvent.click(testButton);
+
+      expect(testButton.getAttribute("data-has-validate-method")).toBe("true");
+    });
+  });
+});

client/src/components/__tests__/ToolsTab.test.tsx

@@ -637,4 +637,180 @@ describe("ToolsTab", () => {
       expect(screen.getByText(/version/i)).toBeInTheDocument();
     });
   });
+
+  describe("JSON Validation Integration", () => {
+    const toolWithJsonParams: Tool = {
+      name: "jsonTool",
+      description: "Tool with JSON parameters",
+      inputSchema: {
+        type: "object" as const,
+        properties: {
+          config: {
+            type: "object" as const,
+            // No properties defined - this will force JSON mode
+          },
+          data: {
+            type: "array" as const,
+            // No items defined - this will force JSON mode
+          },
+        },
+      },
+    };
+
+    it("should prevent tool execution when JSON validation fails", async () => {
+      const mockCallTool = jest.fn();
+      renderToolsTab({
+        tools: [toolWithJsonParams],
+        selectedTool: toolWithJsonParams,
+        callTool: mockCallTool,
+      });
+
+      // Find JSON editor textareas (there should be at least 1 for JSON parameters)
+      const textareas = screen.getAllByRole("textbox");
+      expect(textareas.length).toBeGreaterThanOrEqual(1);
+
+      // Enter invalid JSON in the first textarea
+      const configTextarea = textareas[0];
+      fireEvent.change(configTextarea, {
+        target: { value: '{ "invalid": json }' },
+      });
+
+      // Try to run the tool
+      const runButton = screen.getByRole("button", { name: /run tool/i });
+      await act(async () => {
+        fireEvent.click(runButton);
+      });
+
+      // Tool should not have been called due to validation failure
+      expect(mockCallTool).not.toHaveBeenCalled();
+    });
+
+    it("should allow tool execution when JSON validation passes", async () => {
+      const mockCallTool = jest.fn();
+      renderToolsTab({
+        tools: [toolWithJsonParams],
+        selectedTool: toolWithJsonParams,
+        callTool: mockCallTool,
+      });
+
+      // Find JSON editor textareas
+      const textareas = screen.getAllByRole("textbox");
+
+      // Enter valid JSON in the first textarea
+      fireEvent.change(textareas[0], {
+        target: {
+          value:
+            '{ "config": { "setting": "value" }, "data": ["item1", "item2"] }',
+        },
+      });
+
+      // Wait for debounced updates
+      await act(async () => {
+        await new Promise((resolve) => setTimeout(resolve, 350));
+      });
+
+      // Try to run the tool
+      const runButton = screen.getByRole("button", { name: /run tool/i });
+      await act(async () => {
+        fireEvent.click(runButton);
+      });
+
+      // Tool should have been called successfully
+      expect(mockCallTool).toHaveBeenCalled();
+    });
+
+    it("should handle mixed valid and invalid JSON parameters", async () => {
+      const mockCallTool = jest.fn();
+      renderToolsTab({
+        tools: [toolWithJsonParams],
+        selectedTool: toolWithJsonParams,
+        callTool: mockCallTool,
+      });
+
+      const textareas = screen.getAllByRole("textbox");
+
+      // Enter invalid JSON that contains both valid and invalid parts
+      fireEvent.change(textareas[0], {
+        target: {
+          value:
+            '{ "config": { "setting": "value" }, "data": ["unclosed array" }',
+        },
+      });
+
+      // Try to run the tool
+      const runButton = screen.getByRole("button", { name: /run tool/i });
+      await act(async () => {
+        fireEvent.click(runButton);
+      });
+
+      // Tool should not have been called due to validation failure
+      expect(mockCallTool).not.toHaveBeenCalled();
+    });
+
+    it("should work with tools that have no JSON parameters", async () => {
+      const mockCallTool = jest.fn();
+      const simpleToolWithStringParam: Tool = {
+        name: "simpleTool",
+        description: "Tool with simple parameters",
+        inputSchema: {
+          type: "object" as const,
+          properties: {
+            message: { type: "string" as const },
+            count: { type: "number" as const },
+          },
+        },
+      };
+
+      renderToolsTab({
+        tools: [simpleToolWithStringParam],
+        selectedTool: simpleToolWithStringParam,
+        callTool: mockCallTool,
+      });
+
+      // Fill in the simple parameters
+      const messageInput = screen.getByRole("textbox");
+      const countInput = screen.getByRole("spinbutton");
+
+      fireEvent.change(messageInput, { target: { value: "test message" } });
+      fireEvent.change(countInput, { target: { value: "5" } });
+
+      // Run the tool
+      const runButton = screen.getByRole("button", { name: /run tool/i });
+      await act(async () => {
+        fireEvent.click(runButton);
+      });
+
+      // Tool should have been called successfully (no JSON validation needed)
+      expect(mockCallTool).toHaveBeenCalledWith(
+        simpleToolWithStringParam.name,
+        {
+          message: "test message",
+          count: 5,
+        },
+      );
+    });
+
+    it("should handle empty JSON parameters correctly", async () => {
+      const mockCallTool = jest.fn();
+      renderToolsTab({
+        tools: [toolWithJsonParams],
+        selectedTool: toolWithJsonParams,
+        callTool: mockCallTool,
+      });
+
+      const textareas = screen.getAllByRole("textbox");
+
+      // Clear the textarea (empty JSON should be valid)
+      fireEvent.change(textareas[0], { target: { value: "" } });
+
+      // Try to run the tool
+      const runButton = screen.getByRole("button", { name: /run tool/i });
+      await act(async () => {
+        fireEvent.click(runButton);
+      });
+
+      // Tool should have been called (empty JSON is considered valid)
+      expect(mockCallTool).toHaveBeenCalled();
+    });
+  });
 });