Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added copy/paste to graph editor #662

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

added copy/paste to graph editor #662

wants to merge 2 commits into from

Conversation

AlexBxl
Copy link
Collaborator

@AlexBxl AlexBxl commented Feb 21, 2025 •
edited
Loading

User description

Description

  • copies nodes as JSON to the system clipboard, when pasting strips viewport info
  • pasting works in subgraphs
  • Ctrl/Cmd+X/C/V for Cut, Copy and Paste (issues with focus notwithstanding)
  • added Cut and Copy to single node and selection context menus
  • added Paste to graph context menu

Limitations:

  • currently pasted nodes are centered in the viewport, putting them around the mouse pointer will be a separate issue

Resolves #552

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Video

  • Ctrl+C / Ctrl+V

15fd46d9-a37f-45b3-aaa7-eca44ed46488

  • Context menus

8fa28b8d-6b3a-45fc-bbfd-00db7848dd1a

copying & pasting between graph & subgraph

d570b793-a332-4079-a282-34e2929f4a25

PR Type

Enhancement

Description

  • Added copy/paste functionality for nodes in the graph editor.

  • Implemented Cut, Copy, and Paste actions with system clipboard integration.

  • Enhanced context menus with new options for node and selection handling.

  • Introduced a utility for deleting selected nodes and a hook for copy/paste operations.

Changes walkthrough 📝

Relevant files
Enhancement
nodeContextMenu.tsx
Add Cut and Copy options to node context menu                       

packages/graph-editor/src/components/contextMenus/nodeContextMenu.tsx

  • Added Cut and Copy actions to the node context menu.
  • Integrated useCopyPaste hook for clipboard operations.
  • Updated menu structure with separators for better organization.
    		• +17/-0   
    paneContextMenu.tsx
    Add Paste option to pane context menu                                       

    packages/graph-editor/src/components/contextMenus/paneContextMenu.tsx

  • Added Paste action to the pane context menu.
  • Integrated useCopyPaste hook for clipboard operations.
    		• +8/-0     
    selectionContextMenu.tsx
    Add Cut and Copy options to selection context menu             

    packages/graph-editor/src/components/contextMenus/selectionContextMenu.tsx

  • Added Cut and Copy actions to the selection context menu.
  • Integrated useCopyPaste hook for clipboard operations.
  • Updated menu structure with separators for better organization.
    		• +20/-2   
    index.tsx
    Add hotkey support for Cut, Copy, and Paste                           

    packages/graph-editor/src/components/hotKeys/index.tsx

  • Added hotkey support for Cut, Copy, and Paste actions.
  • Refactored delete logic into a reusable utility.
  • Integrated useCopyPaste hook for clipboard operations.
    		• +14/-57 
    deleteSelectedNodes.tsx
    Add utility for deleting selected nodes                                   

    packages/graph-editor/src/editor/actions/deleteSelectedNodes.tsx

  • Introduced a utility function to delete selected nodes.
  • Handles edge deletion and non-deletable node validation.
    		• +37/-0   
    useCopyPaste.ts
    Add useCopyPaste hook for clipboard operations                     

    packages/graph-editor/src/hooks/useCopyPaste.ts

  • Added a custom hook for copy/paste operations.
  • Supports copying nodes as JSON and pasting with position adjustments.
  • Includes error handling and user feedback via toast notifications.
    		• +187/-0 
    Documentation
    stale-countries-refuse.md
    Add changeset for copy/paste feature                                         

    .changeset/stale-countries-refuse.md

    • Documented the addition of copy/paste functionality.
    		 +5/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @changeset-bot changeset-bot
    Copy link

    changeset-bot bot commented Feb 21, 2025 •
    edited
    Loading

    🦋 Changeset detected

    Latest commit: bf0d39f

    The changes in this PR will be included in the next version bump.

    Not sure what this means? Click here to learn what changesets are.

    Click here if you're a maintainer who wants to add another changeset to this PR

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    552 - Partially compliant

    Compliant requirements:

    • Copy nodes as JSON to the system clipboard.
    • Paste nodes from the system clipboard, stripping viewport info.
    • Add Cut, Copy, and Paste to context menus.
    • Support keyboard shortcuts (Ctrl/Cmd+X/C/V) for Cut, Copy, and Paste.
    • Ensure compatibility with subgraphs.

    Non-compliant requirements:

    • Ensure Cut, Copy, and Paste actions are undoable.
    • Handle Cut operation to ensure recoverability in case of crashes.

    Requires further human verification:

    • Ensure Cut, Copy, and Paste actions are undoable.
    • Handle Cut operation to ensure recoverability in case of crashes.
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The pasteFromClipboard function assumes the clipboard content is always valid JSON. This could lead to runtime errors if invalid or unexpected data is present in the clipboard. Consider adding validation or error handling for such cases.

    const pasteFromClipboard = useCallback(() => {
  if (!graphRef) return;

  navigator.clipboard.readText().then(async (text) => {
    try {
      const { nodes: sourceNodes, edges: sourceEdges } = JSON.parse(text);

      // get target position in flow coordinates
      const flowContainer = document.querySelector('.react-flow');
      const flowBounds = flowContainer?.getBoundingClientRect() || {
        width: window.innerWidth,
        height: window.innerHeight,
      };

      const targetPosition = reactFlowInstance.project({
        x: flowBounds.width / 2,
        y: flowBounds.height / 2,
      });

      // calculate bounding box of source nodes (these are already in flow coordinates)
      const pasteBounds = sourceNodes.reduce(
        (acc, node) => ({
          minX: Math.min(acc.minX, node.annotations?.['ui.position.x'] || 0),
          minY: Math.min(acc.minY, node.annotations?.['ui.position.y'] || 0),
          maxX: Math.max(
            acc.maxX,
            node.annotations?.['ui.position.x'] ||
              0 + (node.annotations?.['ui.width'] || 200),
          ),
          maxY: Math.max(
            acc.maxY,
            node.annotations?.['ui.position.y'] ||
              0 + (node.annotations?.['ui.height'] || 100),
          ),
        }),
        {
          minX: Infinity,
          minY: Infinity,
          maxX: -Infinity,
          maxY: -Infinity,
        },
      );

      // Calculate offset from bounding box center to target position
      const offset = {
        x: targetPosition.x - (pasteBounds.minX + pasteBounds.maxX) / 2,
        y: targetPosition.y - (pasteBounds.minY + pasteBounds.maxY) / 2,
      };

      // create new nodes with updated positions and IDs
      const idMapping = new Map();
      const nodes = sourceNodes.map((node) => {
        const newId = uuidv4();
        idMapping.set(node.id, newId);
        return {
          ...node,
          id: newId,
          annotations: {
            ...node.annotations,
            'ui.position.x':
              (node.annotations?.['ui.position.x'] || 0) + offset.x,
            'ui.position.y':
              (node.annotations?.['ui.position.y'] || 0) + offset.y,
          },
        };
      });

      // create new edges with updated references
      const edges = sourceEdges
        .filter(
          (edge) => idMapping.has(edge.source) && idMapping.has(edge.target),
        )
        .map((edge) => ({
          ...edge,
          id: uuidv4(),
          source: idMapping.get(edge.source),
          target: idMapping.get(edge.target),
        }));

      // store viewport state
      const viewportState = reactFlowInstance.getViewport();

      // merge and load new graph with selected nodes
      const currentGraph = graphRef.getGraph().serialize();
      const mergedGraph = {
        ...currentGraph,
        nodes: [...currentGraph.nodes, ...nodes].map((node) => ({
          ...node,
          selected: idMapping.has(node.id),
        })),
        edges: [...currentGraph.edges, ...edges],
      };

      // load the graph with selection state
      await graphRef.loadRaw(mergedGraph);

      // restore viewport and update ReactFlow state
      reactFlowInstance.setViewport(viewportState);

      // update ReactFlow's internal state
      requestAnimationFrame(() => {
        reactFlowInstance.setNodes(reactFlowInstance.getNodes());
        reactFlowInstance.setEdges(reactFlowInstance.getEdges());
      });

      toast({
        title: 'Pasted',
        description: `${nodes.length} nodes pasted`,
      });
    } catch (error) {
      toast({
        title: 'Error',
        description: `Failed to paste: ${(error as Error).message}`,
      });
    }
  });
    Error Handling

    The deleteSelectedNodes function does not handle cases where reactFlowInstance.getEdges() or reactFlowInstance.getNodes() might return unexpected results (e.g., null or undefined). Consider adding safeguards or error handling.

    export const deleteSelectedNodes = (
  reactFlowInstance: ReactFlowInstance,
  graph: Graph,
  deleteNode: (id: string) => void,
  trigger: (options: ToastOptions) => void,
) => {
  // Delete selected edges
  const edges = reactFlowInstance.getEdges().filter((x) => x.selected);
  reactFlowInstance.deleteElements({ edges });

  // Get and delete selected nodes
  const selectedNodes = reactFlowInstance
    .getNodes()
    .filter((x) => x.selected)
    .map((x) => x.id);

  selectedNodes.forEach((id) => {
    const edgeNode = graph.getNode(id);
    if (edgeNode?.annotations[annotatedDeleteable] === false) {
      trigger({
        title: 'Node not deletable',
        description: `Node ${edgeNode.nodeType()} is not deletable`,
      });
      return;
    }
    deleteNode(id);
  });

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error handling for edge and node retrieval

    Add error handling for the reactFlowInstance.getEdges() and
    reactFlowInstance.getNodes() calls to ensure the function does not fail if these
    methods return unexpected results or null values.

    packages/graph-editor/src/editor/actions/deleteSelectedNodes.tsx [16-24]

    -const edges = reactFlowInstance.getEdges().filter((x) => x.selected);
+const edges = reactFlowInstance.getEdges()?.filter((x) => x.selected) || [];
 reactFlowInstance.deleteElements({ edges });
 
 const selectedNodes = reactFlowInstance
   .getNodes()
-  .filter((x) => x.selected)
-  .map((x) => x.id);
+  ?.filter((x) => x.selected)
+  .map((x) => x.id) || [];
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion adds robust error handling for potential null or undefined values returned by reactFlowInstance.getEdges() and reactFlowInstance.getNodes(). This is a critical improvement to prevent runtime errors and ensure the function operates reliably.

    		High
    Validate clipboard data format

    Add a check to ensure sourceNodes and sourceEdges are valid arrays before processing
    them in the pasteFromClipboard function to avoid runtime errors.

    packages/graph-editor/src/hooks/useCopyPaste.ts [66-104]

     const { nodes: sourceNodes, edges: sourceEdges } = JSON.parse(text);
+
+if (!Array.isArray(sourceNodes) || !Array.isArray(sourceEdges)) {
+  throw new Error('Invalid clipboard data format');
+}
 
 const pasteBounds = sourceNodes.reduce(
   (acc, node) => ({
     minX: Math.min(acc.minX, node.annotations?.['ui.position.x'] || 0),
     ...
   }),
   {
     minX: Infinity,
     minY: Infinity,
     maxX: -Infinity,
     maxY: -Infinity,
   },
 );
    Suggestion importance[1-10]: 8

    __

    Why: Adding a validation check for sourceNodes and sourceEdges ensures that the clipboard data is in the expected format, preventing runtime errors and improving the reliability of the pasteFromClipboard function.

    		Medium
    General
    Handle clipboard access errors

    Add error handling for navigator.clipboard.readText() to catch and handle cases
    where clipboard access is denied or unavailable.

    packages/graph-editor/src/hooks/useCopyPaste.ts [63-179]

    -navigator.clipboard.readText().then(async (text) => {
-  try {
-    const { nodes: sourceNodes, edges: sourceEdges } = JSON.parse(text);
-    ...
-  } catch (error) {
+navigator.clipboard.readText()
+  .then(async (text) => {
+    try {
+      const { nodes: sourceNodes, edges: sourceEdges } = JSON.parse(text);
+      ...
+    } catch (error) {
+      toast({
+        title: 'Error',
+        description: `Failed to paste: ${(error as Error).message}`,
+      });
+    }
+  })
+  .catch((error) => {
     toast({
-      title: 'Error',
-      description: `Failed to paste: ${(error as Error).message}`,
+      title: 'Clipboard Error',
+      description: `Unable to access clipboard: ${(error as Error).message}`,
     });
-  }
-});
+  });
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion introduces error handling for navigator.clipboard.readText(), addressing scenarios where clipboard access might fail. This is a significant enhancement to user experience and application stability.

    		High
    Validate DOM element before accessing properties

    Ensure that the flowContainer element is checked for null or undefined before
    accessing its getBoundingClientRect method to avoid runtime errors.

    packages/graph-editor/src/hooks/useCopyPaste.ts [71-75]

    -const flowBounds = flowContainer?.getBoundingClientRect() || {
-  width: window.innerWidth,
-  height: window.innerHeight,
-};
+const flowBounds = flowContainer
+  ? flowContainer.getBoundingClientRect()
+  : {
+      width: window.innerWidth,
+      height: window.innerHeight,
+    };
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion ensures that the flowContainer element is checked for null or undefined before accessing its properties, which prevents potential runtime errors and improves code robustness.

    		Medium

    @SorsOps
    Copy link
    Member

    SorsOps commented Feb 23, 2025

    So some edge cases we need to deal with

    1. With variadics, its likely that we should clear the inputs in these cases as otherwise its annoying to fix the value
    2025-02-23.17-11-25.mp4
    1. We currently allow copying singleton elements. This also appears to result in a problem where we break the port panel
    2025-02-23.17-13-54.mp4
    1. When pasting, we should set the new nodes to be selected afterwards so that they can be moved together easily
    2025-02-23.17-16-34.mp4
    1. When copying an instance of a sub graph and attempting to paste it inside of itself, you can no longer open the subgraph. This is likely because we are tracking the tabs for the graph by id and copying the graph does not result in a change of the graph id
    2025-02-23.17-17-46.mp4
    1. Even when not copying inside, we seem to reuse the same tab because the graph is not being updated
    2025-02-23.17-20-12.mp4

    @SorsOps
    Copy link
    Member

    SorsOps commented Feb 23, 2025

    We're also breaking variadics

    2025-02-23.17-50-02.mp4

    SorsOps
    SorsOps reviewed Feb 23, 2025
    View reviewed changes
    packages/graph-editor/src/hooks/useCopyPaste.ts
    // calculate bounding box of source nodes (these are already in flow coordinates)
    const pasteBounds = sourceNodes.reduce(
    (acc, node) => ({
    minX: Math.min(acc.minX, node.annotations?.['ui.position.x'] || 0),
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We should use the xpos and ypos, etc annotations so we can reduce the amount of hardcoded strings

    packages/graph-editor/src/hooks/useCopyPaste.ts
    ...currentGraph,
    nodes: [...currentGraph.nodes, ...nodes].map((node) => ({
    ...node,
    selected: idMapping.has(node.id),
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We don't actually store the selected property in the serialization, this would need to be set in reactflow to handle this

    packages/graph-editor/src/hooks/useCopyPaste.ts
    };

    // load the graph with selection state
    await graphRef.loadRaw(mergedGraph);
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    So this runs quite a lot of side code and the logger is saying we're deserializing the graph multiple times, I think we need to break up the loadRaw function to smaller useable pieces as we do need to use it to expose the node factories, etc as part of this

    packages/graph-editor/src/hooks/useCopyPaste.ts
    reactFlowInstance.setViewport(viewportState);

    // update ReactFlow's internal state
    requestAnimationFrame(() => {
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Do we need this? Its already included in the loadRaw call

    packages/graph-editor/src/hooks/useCopyPaste.ts
    ...currentGraph,
    nodes: [...currentGraph.nodes, ...nodes].map((node) => ({
    ...node,
    selected: idMapping.has(node.id),
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Also I don't think this is working as you're expecting, because the keys are the old ids not the new ones you created

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @SorsOps SorsOps SorsOps left review comments

    At least 1 approving review is required to merge this pull request.

    Assignees
    No one assigned
    Labels
    Review effort 4/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Add copy/paste
    2 participants
    @AlexBxl @SorsOps