Fix code review issues: deep clone, expansion logic, unused imports, edge cases

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>

Author: Copilot

packages/designer/src/__tests__/keyboard-shortcuts.test.tsx

@@ -1,5 +1,5 @@
 import { describe, it, expect, beforeEach } from 'vitest';
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, fireEvent } from '@testing-library/react';
 import { DesignerProvider, useDesigner } from '../context/DesignerContext';
 import { SchemaNode } from '@object-ui/core';
 import React from 'react';
@@ -31,6 +31,9 @@ const TestComponent = () => {
       <button onClick={() => pasteNode('root')} data-testid="paste">Paste</button>
       <button onClick={() => moveNodeUp('child-2')} data-testid="move-up">Move Up</button>
       <button onClick={() => moveNodeDown('child-1')} data-testid="move-down">Move Down</button>
+      <button onClick={() => moveNodeUp('child-1')} data-testid="move-up-first">Move Up First</button>
+      <button onClick={() => moveNodeDown('child-3')} data-testid="move-down-last">Move Down Last</button>
+      <button onClick={() => cutNode('nonexistent-id')} data-testid="cut-nonexistent">Cut Nonexistent</button>
     </div>
   );
 };
@@ -98,6 +101,64 @@ describe('Keyboard Shortcuts and Navigation', () => {
     // Schema should have an extra node
     const schema = JSON.parse(getByTestId('schema').textContent || '{}');
     expect(schema.body.length).toBe(initialLength + 1);
+    
+    // The duplicated node should have the same type as the original
+    expect(schema.body[1].type).toBe('text');
+    expect(schema.body[1].content).toBe('First');
+    
+    // The duplicated node should be positioned right after the original (at index 1)
+    expect(schema.body[1].id).not.toBe('child-1'); // Should have a new ID
+  });
+
+  it('should handle moving a node up when already at top', () => {
+    const { getByTestId } = render(
+      <DesignerProvider initialSchema={initialSchema}>
+        <TestComponent />
+      </DesignerProvider>
+    );
+
+    const schemaBeforeMove = JSON.parse(getByTestId('schema').textContent || '{}');
+    
+    // Try to move the first node up (should do nothing)
+    fireEvent.click(getByTestId('move-up-first'));
+    
+    const schemaAfterMove = JSON.parse(getByTestId('schema').textContent || '{}');
+    // Schema should remain unchanged
+    expect(schemaAfterMove.body[0].id).toBe(schemaBeforeMove.body[0].id);
+  });
+
+  it('should handle moving a node down when already at bottom', () => {
+    const { getByTestId } = render(
+      <DesignerProvider initialSchema={initialSchema}>
+        <TestComponent />
+      </DesignerProvider>
+    );
+
+    const schemaBeforeMove = JSON.parse(getByTestId('schema').textContent || '{}');
+    
+    // Try to move the last node down (should do nothing)
+    fireEvent.click(getByTestId('move-down-last'));
+    
+    const schemaAfterMove = JSON.parse(getByTestId('schema').textContent || '{}');
+    // Schema should remain unchanged
+    expect(schemaAfterMove.body[2].id).toBe(schemaBeforeMove.body[2].id);
+  });
+
+  it('should handle cutting a non-existent node gracefully', () => {
+    const { getByTestId } = render(
+      <DesignerProvider initialSchema={initialSchema}>
+        <TestComponent />
+      </DesignerProvider>
+    );
+
+    const initialLength = JSON.parse(getByTestId('schema').textContent || '{}').body.length;
+    
+    // Try to cut a node that doesn't exist
+    fireEvent.click(getByTestId('cut-nonexistent'));
+    
+    const schema = JSON.parse(getByTestId('schema').textContent || '{}');
+    // Schema should remain unchanged
+    expect(schema.body.length).toBe(initialLength);
   });
 
   it('should paste a copied node', () => {

packages/designer/src/components/ComponentTree.tsx

@@ -222,10 +222,16 @@ export const ComponentTree: React.FC<ComponentTreeProps> = React.memo(({ classNa
         return () => window.removeEventListener('keydown', handleKeyDown);
     }, [selectedNodeId, moveNodeUp, moveNodeDown]);
 
-    const expansionContextValue = useMemo(() => ({
-        expandAll: expandTrigger > 0,
-        collapseAll: collapseTrigger > 0
-    }), [expandTrigger, collapseTrigger]);
+    const expansionContextValue = useMemo(() => {
+        if (expandTrigger > collapseTrigger) {
+            return { expandAll: true, collapseAll: false };
+        }
+        if (collapseTrigger > expandTrigger) {
+            return { expandAll: false, collapseAll: true };
+        }
+        // Initial or neutral state: no global expand/collapse action
+        return { expandAll: false, collapseAll: false };
+    }, [expandTrigger, collapseTrigger]);
 
     return (
         <TreeExpansionContext.Provider value={expansionContextValue}>

packages/designer/src/components/ContextMenu.tsx

@@ -38,8 +38,7 @@ export const ContextMenu: React.FC<ContextMenuProps> = ({ position, targetNodeId
         removeNode, 
         moveNodeUp,
         moveNodeDown,
-        canPaste, 
-        selectedNodeId 
+        canPaste
     } = useDesigner();
 
     const handleCopy = useCallback(() => {

packages/designer/src/components/KeyboardShortcutsHelp.tsx

@@ -8,7 +8,6 @@ import {
 } from "@object-ui/components";
 import { Button } from '@object-ui/components';
 import { Keyboard, Command } from 'lucide-react';
-import { cn } from '@object-ui/components';
 
 interface ShortcutItem {
     keys: string[];

packages/designer/src/context/DesignerContext.tsx

@@ -163,7 +163,39 @@ const findNodeById = (node: SchemaNode, id: string): SchemaNode | null => {
     return null;
 };
 
-// Find Parent and Index of a node
+/**
+ * Deep clone a SchemaNode, creating new objects for all nested properties.
+ * This ensures that modifications to the cloned node don't affect the original.
+ * 
+ * @param node - The node to clone
+ * @returns A deep copy of the node with all nested children cloned
+ */
+const deepCloneNode = (node: SchemaNode): SchemaNode => {
+    const cloned: SchemaNode = { ...node };
+    
+    // Deep clone the body if it exists
+    if (node.body) {
+        if (Array.isArray(node.body)) {
+            cloned.body = node.body.map(child => deepCloneNode(child));
+        } else if (typeof node.body === 'object') {
+            cloned.body = deepCloneNode(node.body as SchemaNode);
+        }
+    }
+    
+    return cloned;
+};
+
+/**
+ * Find the parent node and index of a target node in the schema tree.
+ * This is used for operations that need to know a node's position within its parent,
+ * such as moving nodes up/down or duplicating nodes as siblings.
+ * 
+ * @param root - The root node to search from
+ * @param targetId - The ID of the node to find
+ * @param parent - Internal parameter for recursion, tracks the parent during traversal
+ * @returns An object with the parent node and the index of the target within the parent's body,
+ *          or null if the target is not found or is the root node
+ */
 const findParentAndIndex = (
     root: SchemaNode, 
     targetId: string, 
@@ -390,8 +422,9 @@ export const DesignerProvider: React.FC<DesignerProviderProps> = ({
   const cutNode = useCallback((id: string) => {
     const node = findNodeById(schema, id);
     if (node) {
-      // Copy the node to clipboard
-      const { id: originalId, ...nodeWithoutId } = node;
+      // Deep clone the node to clipboard to avoid reference issues
+      const clonedNode = deepCloneNode(node);
+      const { id: originalId, ...nodeWithoutId } = clonedNode;
       setClipboard(nodeWithoutId as SchemaNode);
       // Then remove it from the tree
       removeNode(id);
@@ -401,9 +434,9 @@ export const DesignerProvider: React.FC<DesignerProviderProps> = ({
   const duplicateNode = useCallback((id: string) => {
     const node = findNodeById(schema, id);
     if (node) {
-      // Create a deep copy without the ID
-      const { id: originalId, ...nodeWithoutId } = node;
-      setClipboard(nodeWithoutId as SchemaNode);
+      // Deep clone the node without modifying clipboard
+      const clonedNode = deepCloneNode(node);
+      const { id: originalId, ...nodeWithoutId } = clonedNode;
 
       // Find the parent to paste into
       const parentInfo = findParentAndIndex(schema, id);