From d65ed01922b64afff5790460046d2ef09c238563 Mon Sep 17 00:00:00 2001 From: Nandha Reddy Date: Thu, 19 Jun 2025 02:32:40 +1000 Subject: [PATCH 01/19] Add JSON validation to tool execution Prevents execution of tools with invalid JSON parameters by validating JSON syntax before tool execution and showing error UI for invalid input. - Add validateJson method to DynamicJsonForm via forwardRef - Add validation check in ToolsTab before tool execution - Reuse existing JsonEditor error display for consistent UX --- client/src/components/DynamicJsonForm.tsx | 31 ++++++++++++++++++++--- client/src/components/ToolsTab.tsx | 26 +++++++++++++++---- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/client/src/components/DynamicJsonForm.tsx b/client/src/components/DynamicJsonForm.tsx index 6a5993c3..fe2cfd2d 100644 --- a/client/src/components/DynamicJsonForm.tsx +++ b/client/src/components/DynamicJsonForm.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useCallback, useRef } from "react"; +import { useState, useEffect, useCallback, useRef, forwardRef, useImperativeHandle } from "react"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import JsonEditor from "./JsonEditor"; @@ -13,6 +13,10 @@ interface DynamicJsonFormProps { maxDepth?: number; } +export interface DynamicJsonFormRef { + validateJson: () => { isValid: boolean; error: string | null }; +} + const isSimpleObject = (schema: JsonSchemaType): boolean => { const supportedTypes = ["string", "number", "integer", "boolean", "null"]; if (supportedTypes.includes(schema.type)) return true; @@ -22,12 +26,12 @@ const isSimpleObject = (schema: JsonSchemaType): boolean => { ); }; -const DynamicJsonForm = ({ +const DynamicJsonForm = forwardRef(({ schema, value, onChange, maxDepth = 3, -}: DynamicJsonFormProps) => { +}, ref) => { const isOnlyJSON = !isSimpleObject(schema); const [isJsonMode, setIsJsonMode] = useState(isOnlyJSON); const [jsonError, setJsonError] = useState(); @@ -108,6 +112,25 @@ const DynamicJsonForm = ({ } }; + const validateJson = () => { + if (!isJsonMode) return { isValid: true, error: null }; + try { + const jsonStr = rawJsonValue.trim(); + if (!jsonStr) return { isValid: true, error: null }; + JSON.parse(jsonStr); + setJsonError(undefined); + return { isValid: true, error: null }; + } catch (err) { + const errorMessage = err instanceof Error ? err.message : "Invalid JSON"; + setJsonError(errorMessage); + return { isValid: false, error: errorMessage }; + } + }; + + useImperativeHandle(ref, () => ({ + validateJson, + })); + const renderFormFields = ( propSchema: JsonSchemaType, currentValue: JsonValue, @@ -303,6 +326,6 @@ const DynamicJsonForm = ({ )} ); -}; +}); export default DynamicJsonForm; diff --git a/client/src/components/ToolsTab.tsx b/client/src/components/ToolsTab.tsx index 8a7f6578..1ce034fb 100644 --- a/client/src/components/ToolsTab.tsx +++ b/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 } from "@/utils/schemaUtils"; import { @@ -13,8 +13,8 @@ import { ListToolsResult, Tool, } from "@modelcontextprotocol/sdk/types.js"; -import { Loader2, Send, ChevronDown, ChevronUp } from "lucide-react"; -import { useEffect, useState } from "react"; +import { Loader2, Send, ChevronDown, ChevronUp, AlertCircle } from "lucide-react"; +import { useEffect, useState, useRef } from "react"; import ListPane from "./ListPane"; import JsonView from "./JsonView"; import ToolResults from "./ToolResults"; @@ -28,6 +28,7 @@ const ToolsTab = ({ setSelectedTool, toolResult, nextCursor, + error, }: { tools: Tool[]; listTools: () => void; @@ -42,6 +43,7 @@ const ToolsTab = ({ const [params, setParams] = useState>({}); const [isToolRunning, setIsToolRunning] = useState(false); const [isOutputSchemaExpanded, setIsOutputSchemaExpanded] = useState(false); + const formRefs = useRef>({}); useEffect(() => { const params = Object.entries( @@ -84,7 +86,13 @@ const ToolsTab = ({
- {selectedTool ? ( + {error ? ( + + + Error + {error} + + ) : selectedTool ? (

{selectedTool.description} @@ -137,6 +145,7 @@ const ToolsTab = ({ ) : prop.type === "object" || prop.type === "array" ? (

(formRefs.current[key] = ref)} schema={{ type: prop.type, properties: prop.properties, @@ -174,6 +183,7 @@ const ToolsTab = ({ ) : (
(formRefs.current[key] = ref)} schema={{ type: prop.type, properties: prop.properties, @@ -232,6 +242,12 @@ const ToolsTab = ({ )} -
- ))} - -
-
- ); - } - - // For complex arrays, fall back to JSON editor + const renderFormFields = ( + propSchema: JsonSchemaType, + currentValue: JsonValue, + path: string[] = [], + depth: number = 0, + parentSchema?: JsonSchemaType, + propertyName?: string, + ) => { + if ( + depth >= maxDepth && + (propSchema.type === "object" || propSchema.type === "array") + ) { + // Render as JSON editor when max depth is reached return ( { try { const parsed = JSON.parse(newValue); @@ -392,93 +200,296 @@ const DynamicJsonForm = forwardRef(({ /> ); } - default: - return null; - } - }; - const handleFieldChange = (path: string[], fieldValue: JsonValue) => { - if (path.length === 0) { - onChange(fieldValue); - return; - } + // Check if this property is required in the parent schema + const isRequired = + parentSchema?.required?.includes(propertyName || "") ?? false; - try { - const newValue = updateValueAtPath(value, path, fieldValue); - onChange(newValue); - } catch (error) { - console.error("Failed to update form value:", error); - onChange(value); - } - }; + switch (propSchema.type) { + case "string": + return ( + { + const val = e.target.value; + // Always allow setting string values, including empty strings + handleFieldChange(path, val); + }} + placeholder={propSchema.description} + required={isRequired} + /> + ); + case "number": + return ( + { + const val = e.target.value; + if (!val && !isRequired) { + handleFieldChange(path, undefined); + } else { + const num = Number(val); + if (!isNaN(num)) { + handleFieldChange(path, num); + } + } + }} + placeholder={propSchema.description} + required={isRequired} + /> + ); + case "integer": + return ( + { + const val = e.target.value; + if (!val && !isRequired) { + handleFieldChange(path, undefined); + } else { + const num = Number(val); + // Only update if it's a valid integer + if (!isNaN(num) && Number.isInteger(num)) { + handleFieldChange(path, num); + } + } + }} + placeholder={propSchema.description} + required={isRequired} + /> + ); + case "boolean": + return ( + handleFieldChange(path, e.target.checked)} + className="w-4 h-4" + required={isRequired} + /> + ); + case "object": + if (!propSchema.properties) { + return ( + { + try { + const parsed = JSON.parse(newValue); + handleFieldChange(path, parsed); + setJsonError(undefined); + } catch (err) { + setJsonError( + err instanceof Error ? err.message : "Invalid JSON", + ); + } + }} + error={jsonError} + /> + ); + } - const shouldUseJsonMode = - schema.type === "object" && - (!schema.properties || Object.keys(schema.properties).length === 0); + return ( +
+ {Object.entries(propSchema.properties).map(([key, subSchema]) => ( +
+ + {renderFormFields( + subSchema as JsonSchemaType, + (currentValue as Record)?.[key], + [...path, key], + depth + 1, + propSchema, + key, + )} +
+ ))} +
+ ); + case "array": { + const arrayValue = Array.isArray(currentValue) ? currentValue : []; + if (!propSchema.items) return null; - useEffect(() => { - if (shouldUseJsonMode && !isJsonMode) { - setIsJsonMode(true); - } - }, [shouldUseJsonMode, isJsonMode]); + // If the array items are simple, render as form fields, otherwise use JSON editor + if (isSimpleObject(propSchema.items)) { + return ( +
+ {propSchema.description && ( +

+ {propSchema.description} +

+ )} - return ( -
-
- {isJsonMode && ( - - )} - {!isOnlyJSON && ( - + {propSchema.items?.description && ( +

+ Items: {propSchema.items.description} +

+ )} + +
+ {arrayValue.map((item, index) => ( +
+ {renderFormFields( + propSchema.items as JsonSchemaType, + item, + [...path, index.toString()], + depth + 1, + )} + +
+ ))} + +
+
+ ); + } + + // For complex arrays, fall back to JSON editor + return ( + { + try { + const parsed = JSON.parse(newValue); + handleFieldChange(path, parsed); + setJsonError(undefined); + } catch (err) { + setJsonError( + err instanceof Error ? err.message : "Invalid JSON", + ); + } + }} + error={jsonError} + /> + ); + } + default: + return null; + } + }; + + const handleFieldChange = (path: string[], fieldValue: JsonValue) => { + if (path.length === 0) { + onChange(fieldValue); + return; + } + + try { + const newValue = updateValueAtPath(value, path, fieldValue); + onChange(newValue); + } catch (error) { + console.error("Failed to update form value:", error); + onChange(value); + } + }; + + const shouldUseJsonMode = + schema.type === "object" && + (!schema.properties || Object.keys(schema.properties).length === 0); + + useEffect(() => { + if (shouldUseJsonMode && !isJsonMode) { + setIsJsonMode(true); + } + }, [shouldUseJsonMode, isJsonMode]); + + return ( +
+
+ {isJsonMode && ( + + )} + {!isOnlyJSON && ( + + )} +
+ + {isJsonMode ? ( + { + // Always update local state + setRawJsonValue(newValue); + + // Use the debounced function to attempt parsing and updating parent + debouncedUpdateParent(newValue); + }} + error={jsonError} + /> + ) : // If schema type is object but value is not an object or is empty, and we have actual JSON data, + // render a simple representation of the JSON data + schema.type === "object" && + (typeof value !== "object" || + value === null || + Object.keys(value).length === 0) && + rawJsonValue && + rawJsonValue !== "{}" ? ( +
+

+ Form view not available for this JSON structure. Using simplified + view: +

+ 
+              {rawJsonValue}
+
+

+ Use JSON mode for full editing capabilities. +

+
+ ) : ( + renderFormFields(schema, value) )}
- - {isJsonMode ? ( - { - // Always update local state - setRawJsonValue(newValue); - - // Use the debounced function to attempt parsing and updating parent - debouncedUpdateParent(newValue); - }} - error={jsonError} - /> - ) : // If schema type is object but value is not an object or is empty, and we have actual JSON data, - // render a simple representation of the JSON data - schema.type === "object" && - (typeof value !== "object" || - value === null || - Object.keys(value).length === 0) && - rawJsonValue && - rawJsonValue !== "{}" ? ( -
-

- Form view not available for this JSON structure. Using simplified - view: -

- 
-            {rawJsonValue}
-
-

- Use JSON mode for full editing capabilities. -

-
- ) : ( - renderFormFields(schema, value) - )} -
- ); -}); + ); + }, +); export default DynamicJsonForm; diff --git a/client/src/components/ToolsTab.tsx b/client/src/components/ToolsTab.tsx index 9f0d2008..7258eaa8 100644 --- a/client/src/components/ToolsTab.tsx +++ b/client/src/components/ToolsTab.tsx @@ -13,7 +13,13 @@ import { ListToolsResult, Tool, } from "@modelcontextprotocol/sdk/types.js"; -import { Loader2, Send, ChevronDown, ChevronUp, AlertCircle } from "lucide-react"; +import { + Loader2, + Send, + ChevronDown, + ChevronUp, + AlertCircle, +} from "lucide-react"; import { useEffect, useState, useRef } from "react"; import ListPane from "./ListPane"; import JsonView from "./JsonView"; @@ -254,9 +260,9 @@ const ToolsTab = ({ + <> + + + )} {!isOnlyJSON && ( +
+ ); + }; + + return render(); + }; + + 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(null); + + return ( +
+ + +
+ ); + }; + + render(); + + 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(null); + + return ( +
+ + +
+ ); + }; + + render(); + + const testButton = screen.getByTestId("ref-test-button"); + fireEvent.click(testButton); + + expect(testButton.getAttribute('data-has-validate-method')).toBe('true'); + }); + }); +}); diff --git a/client/src/components/__tests__/ToolsTab.test.tsx b/client/src/components/__tests__/ToolsTab.test.tsx index bc75401d..a9e7b5fb 100644 --- a/client/src/components/__tests__/ToolsTab.test.tsx +++ b/client/src/components/__tests__/ToolsTab.test.tsx @@ -637,4 +637,171 @@ 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(); + }); + }); }); From ad10912e12823cb8407722ca948d596816350af9 Mon Sep 17 00:00:00 2001 From: olaservo Date: Wed, 6 Aug 2025 21:08:15 -0700 Subject: [PATCH 05/19] Fix formatting --- .../__tests__/DynamicJsonForm.test.tsx | 80 +++++++++++++------ .../components/__tests__/ToolsTab.test.tsx | 41 ++++++---- 2 files changed, 81 insertions(+), 40 deletions(-) diff --git a/client/src/components/__tests__/DynamicJsonForm.test.tsx b/client/src/components/__tests__/DynamicJsonForm.test.tsx index e1ad2c4f..541da33d 100644 --- a/client/src/components/__tests__/DynamicJsonForm.test.tsx +++ b/client/src/components/__tests__/DynamicJsonForm.test.tsx @@ -676,10 +676,18 @@ describe("DynamicJsonForm Validation Functionality", () => { onClick={() => { const result = formRef.current?.validateJson(); // Add data attributes to make validation result testable - const button = document.querySelector('[data-testid="validate-button"]') as HTMLElement; + 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 || ''); + button.setAttribute( + "data-validation-valid", + result.isValid.toString(), + ); + button.setAttribute( + "data-validation-error", + result.error || "", + ); } }} data-testid="validate-button" @@ -702,7 +710,7 @@ describe("DynamicJsonForm Validation Functionality", () => { const TestComponent = () => { const formRef = useRef(null); - + return (
{
@@ -235,6 +253,8 @@ const ToolsTab = ({ ...params, [key]: newValue, }); + // Check validation after a short delay to allow form to update + setTimeout(checkValidationErrors, 100); }} />
@@ -317,10 +337,7 @@ const ToolsTab = ({