pr feedback: Only use form if object is simple

Author: max-stytch

client/src/components/DynamicJsonForm.tsx

@@ -1,10 +1,9 @@
 import { useState, useEffect, useCallback, useRef } from "react";
 import { Button } from "@/components/ui/button";
 import { Input } from "@/components/ui/input";
-import { Label } from "@/components/ui/label";
 import JsonEditor from "./JsonEditor";
-import { updateValueAtPath, JsonObject } from "@/utils/jsonPathUtils";
-import { generateDefaultValue, formatFieldLabel } from "@/utils/schemaUtils";
+import { updateValueAtPath } from "@/utils/jsonPathUtils";
+import { generateDefaultValue } from "@/utils/schemaUtils";
 
 export type JsonValue =
   | string
@@ -36,17 +35,25 @@ interface DynamicJsonFormProps {
   value: JsonValue;
   onChange: (value: JsonValue) => void;
   maxDepth?: number;
-  onlyJSON?: boolean;
 }
 
+const isSimpleObject = (schema: JsonSchemaType): boolean => {
+  const supportedTypes = ["string", "number", "integer", "boolean", "null"];
+  if (supportedTypes.includes(schema.type)) return true;
+  if (schema.type !== "object") return false;
+  return Object.values(schema.properties ?? {}).every((prop) =>
+    supportedTypes.includes(prop.type),
+  );
+};
+
 const DynamicJsonForm = ({
   schema,
   value,
   onChange,
   maxDepth = 3,
-  onlyJSON = false,
 }: DynamicJsonFormProps) => {
-  const [isJsonMode, setIsJsonMode] = useState(onlyJSON);
+  const isOnlyJSON = !isSimpleObject(schema);
+  const [isJsonMode, setIsJsonMode] = useState(isOnlyJSON);
   const [jsonError, setJsonError] = useState<string>();
   // Store the raw JSON string to allow immediate feedback during typing
   // while deferring parsing until the user stops typing
@@ -233,111 +240,6 @@ const DynamicJsonForm = ({
             required={propSchema.required}
           />
         );
-      case "object": {
-        // Handle case where we have a value but no schema properties
-        const objectValue = (currentValue as JsonObject) || {};
-
-        // If we have schema properties, use them to render fields
-        if (propSchema.properties) {
-          return (
-            <div className="space-y-4 border rounded-md p-4">
-              {Object.entries(propSchema.properties).map(([key, prop]) => (
-                <div key={key} className="space-y-2">
-                  <Label>{formatFieldLabel(key)}</Label>
-                  {renderFormFields(
-                    prop,
-                    objectValue[key],
-                    [...path, key],
-                    depth + 1,
-                  )}
-                </div>
-              ))}
-            </div>
-          );
-        }
-        // If we have a value but no schema properties, render fields based on the value
-        else if (Object.keys(objectValue).length > 0) {
-          return (
-            <div className="space-y-4 border rounded-md p-4">
-              {Object.entries(objectValue).map(([key, value]) => (
-                <div key={key} className="space-y-2">
-                  <Label>{formatFieldLabel(key)}</Label>
-                  <Input
-                    type="text"
-                    value={String(value)}
-                    onChange={(e) =>
-                      handleFieldChange([...path, key], e.target.value)
-                    }
-                  />
-                </div>
-              ))}
-            </div>
-          );
-        }
-        // If we have neither schema properties nor value, return null
-        return null;
-      }
-      case "array": {
-        const arrayValue = Array.isArray(currentValue) ? currentValue : [];
-        if (!propSchema.items) return null;
-        return (
-          <div className="space-y-4">
-            {propSchema.description && (
-              <p className="text-sm text-gray-600">{propSchema.description}</p>
-            )}
-
-            {propSchema.items?.description && (
-              <p className="text-sm text-gray-500">
-                Items: {propSchema.items.description}
-              </p>
-            )}
-
-            <div className="space-y-2">
-              {arrayValue.map((item, index) => (
-                <div key={index} className="flex items-center gap-2">
-                  {renderFormFields(
-                    propSchema.items as JsonSchemaType,
-                    item,
-                    [...path, index.toString()],
-                    depth + 1,
-                  )}
-                  <Button
-                    variant="outline"
-                    size="sm"
-                    onClick={() => {
-                      const newArray = [...arrayValue];
-                      newArray.splice(index, 1);
-                      handleFieldChange(path, newArray);
-                    }}
-                  >
-                    Remove
-                  </Button>
-                </div>
-              ))}
-              <Button
-                variant="outline"
-                size="sm"
-                onClick={() => {
-                  const defaultValue = generateDefaultValue(
-                    propSchema.items as JsonSchemaType,
-                  );
-                  handleFieldChange(path, [
-                    ...arrayValue,
-                    defaultValue ?? null,
-                  ]);
-                }}
-                title={
-                  propSchema.items?.description
-                    ? `Add new ${propSchema.items.description}`
-                    : "Add new item"
-                }
-              >
-                Add Item
-              </Button>
-            </div>
-          </div>
-        );
-      }
       default:
         return null;
     }
@@ -376,7 +278,7 @@ const DynamicJsonForm = ({
             Format JSON
           </Button>
         )}
-        {!onlyJSON && (
+        {!isOnlyJSON && (
           <Button variant="outline" size="sm" onClick={handleSwitchToFormMode}>
             {isJsonMode ? "Switch to Form" : "Switch to JSON"}
           </Button>

client/src/components/ToolsTab.tsx

@@ -234,7 +234,6 @@ const ToolsTab = ({
                         ) : (
                           <div className="mt-1">
                             <DynamicJsonForm
-                              onlyJSON
                               schema={{
                                 type: prop.type,
                                 properties: prop.properties,

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

@@ -1,4 +1,4 @@
-import { render, screen, fireEvent } from "@testing-library/react";
+import { render, screen, fireEvent, waitFor } from "@testing-library/react";
 import { describe, it, expect, jest } from "@jest/globals";
 import DynamicJsonForm from "../DynamicJsonForm";
 import type { JsonSchemaType } from "../DynamicJsonForm";
@@ -93,3 +93,47 @@ describe("DynamicJsonForm Integer Fields", () => {
     });
   });
 });
+
+describe("DynamicJsonForm Complex Fields", () => {
+  const renderForm = (props = {}) => {
+    const defaultProps = {
+      schema: {
+        type: "object",
+        properties: {
+          // The simplified JsonSchemaType does not accept oneOf fields
+          // But they exist in the more-complete JsonSchema7Type
+          nested: { oneOf: [{ type: "string" }, { type: "integer" }] },
+        },
+      } as unknown as JsonSchemaType,
+      value: undefined,
+      onChange: jest.fn(),
+    };
+    return render(<DynamicJsonForm {...defaultProps} {...props} />);
+  };
+
+  describe("Basic Operations", () => {
+    it("should render textbox and autoformat button, but no switch-to-form button", () => {
+      renderForm();
+      const input = screen.getByRole("textbox");
+      expect(input).toHaveProperty("type", "textarea");
+      const buttons = screen.getAllByRole("button");
+      expect(buttons).toHaveLength(1);
+      expect(buttons[0]).toHaveProperty("textContent", "Format JSON");
+    });
+
+    it("should pass changed values to onChange", () => {
+      const onChange = jest.fn();
+      renderForm({ onChange });
+
+      const input = screen.getByRole("textbox");
+      fireEvent.change(input, {
+        target: { value: `{ "nested": "i am string" }` },
+      });
+
+      // The onChange handler is debounced when using the JSON view, so we need to wait a little bit
+      waitFor(() => {
+        expect(onChange).toHaveBeenCalledWith(`{ "nested": "i am string" }`);
+      });
+    });
+  });
+});