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.

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

added discard/save dialog & clear current graph when loading new graph #632

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

Conversation

AlexBxl
Copy link
Contributor

@AlexBxl AlexBxl commented Jan 14, 2025

User description

Description

  • Now when you try to upload a graph, if there are any nodes in the current graph it will ask you if you want to discard or save.
  • I took the graph's name and made it lowercase and replaced the spaces with hyphens for the name, so currently graphs are downloaded as 'untitled-graph.json'.
  • Maybe we should ask the user to pick a file name when saving?

Solves #618

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

How to test this

Create some nodes, then try to load a graph.

Video

e7191363-795e-4fd8-80d1-9c4f93dca2f2


PR Type

Enhancement, Bug fix


Description

  • Added a dialog to save or discard the current graph when uploading a new one.

  • Implemented a utility to clear and load a new graph file.

  • Introduced a utility to save the current graph as a JSON file.

  • Ensured the current graph is cleared before loading a new one.


Changes walkthrough 📝

Relevant files
Enhancement
upload.tsx
Add dialog and logic for graph upload handling                     

packages/graph-editor/src/components/toolbar/buttons/upload.tsx

  • Added a dialog to prompt saving or discarding the current graph.
  • Integrated loadGraph and saveGraph utilities for graph management.
  • Updated the upload button to handle graph state before loading a new
    graph.
  • +73/-24 
    loadGraph.ts
    Implement utility to load and clear graphs                             

    packages/graph-editor/src/utils/loadGraph.ts

  • Created a utility to clear and load a new graph.
  • Ensured the graph is cleared before loading a new one.
  • Added file input handling for JSON graph files.
  • +34/-0   
    saveGraph.ts
    Implement utility to save graphs as JSON                                 

    packages/graph-editor/src/utils/saveGraph.ts

  • Created a utility to save the current graph as a JSON file.
  • Added functionality to download the graph with a specified filename.
  • +24/-0   
    Documentation
    metal-jokes-jam.md
    Add changeset for graph upload enhancements                           

    .changeset/metal-jokes-jam.md

  • Documented the changes for clearing and saving graphs when uploading.
  • +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    changeset-bot bot commented Jan 14, 2025

    🦋 Changeset detected

    Latest commit: 4671edb

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

    This PR includes changesets to release 1 package
    Name Type
    @tokens-studio/graph-editor Patch

    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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    618 - Partially compliant

    Fully compliant requirements:

    • Merge inputs into one when a graph is uploaded.
    • If one input is empty, the result should be the other input.

    Not compliant requirements:

    • If both inputs have data, the result should be a combined input.
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The onSave function assumes the graph.annotations[title] is always defined. If it is undefined, the code might throw an error or behave unexpectedly.

    const onSave = async () => {
      if (!graphRef) return;
      await saveGraph(
        graphRef,
        graph,
        (graph.annotations[title] || 'graph').toLowerCase().replace(/\s+/g, '-'),
      );
      setIsDialogOpen(false);
      loadGraph(graphRef, graph, reactFlowInstance);
    Error Handling

    The loadGraph function does not handle errors when parsing the JSON file or loading the graph. This could lead to runtime issues if the file is invalid.

    export default function loadGraph(
      graphRef: ImperativeEditorRef | undefined,
      graph: Graph,
      reactFlowInstance: ReactFlowInstance,
    ) {
      if (!graphRef) return;
    
      // always clear the graph before loading a new one,
      // the user already had the choice to save it
      clear(reactFlowInstance, graph);
    
      const input = document.createElement('input');
      input.type = 'file';
      input.accept = '.json';
      //@ts-expect-error
      input.onchange = (e: HTMLInputElement) => {
        //@ts-expect-error
        const file = e.target.files[0];
        if (!file) return;
        const reader = new FileReader();
        reader.onload = (e: ProgressEvent<FileReader>) => {
          const text = e.target?.result as string;
          const data = JSON.parse(text);
          graphRef.loadRaw(data);
        };
        reader.readAsText(file);
      };
      input.click();
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add error handling for the graph saving process to ensure robustness and user feedback

    Ensure that the onSave function handles potential errors during the saveGraph
    operation, such as file system issues or permission errors, and provides user
    feedback in case of failure.

    packages/graph-editor/src/components/toolbar/buttons/upload.tsx [40-44]

    -await saveGraph(
    -  graphRef,
    -  graph,
    -  (graph.annotations[title] || 'graph').toLowerCase().replace(/\s+/g, '-'),
    -);
    +try {
    +  await saveGraph(
    +    graphRef,
    +    graph,
    +    (graph.annotations[title] || 'graph').toLowerCase().replace(/\s+/g, '-'),
    +  );
    +} catch (error) {
    +  console.error('Failed to save graph:', error);
    +  // Optionally, display an error message to the user
    +}
    Suggestion importance[1-10]: 9

    Why: Adding error handling to the onSave function is a critical improvement as it ensures robustness by managing potential failures during the save operation. This also enhances user experience by providing feedback in case of errors, making the application more reliable.

    9
    Add error handling for file reading and parsing to ensure robustness against corrupted or unreadable files

    Add error handling for the file reading process in the loadGraph function to manage
    scenarios where the file cannot be read or is corrupted.

    packages/graph-editor/src/utils/loadGraph.ts [26-30]

     reader.onload = (e: ProgressEvent<FileReader>) => {
    -  const text = e.target?.result as string;
    -  const data = JSON.parse(text);
    -  graphRef.loadRaw(data);
    +  try {
    +    const text = e.target?.result as string;
    +    const data = JSON.parse(text);
    +    graphRef.loadRaw(data);
    +  } catch (error) {
    +    console.error('Error reading or parsing the file:', error);
    +    // Optionally, display an error message to the user
    +  }
     };
    Suggestion importance[1-10]: 9

    Why: Adding error handling for file reading and parsing is a crucial improvement as it ensures the application can gracefully handle corrupted or unreadable files. This enhances the robustness and user experience of the application.

    9
    Add validation for the JSON file content to ensure it matches the expected graph structure before loading

    Validate the JSON file content in the loadGraph function to ensure it matches the
    expected graph structure before calling graphRef.loadRaw(data).

    packages/graph-editor/src/utils/loadGraph.ts [27-28]

    -const data = JSON.parse(text);
    -graphRef.loadRaw(data);
    +try {
    +  const data = JSON.parse(text);
    +  if (isValidGraphData(data)) { // Assume isValidGraphData is a validation function
    +    graphRef.loadRaw(data);
    +  } else {
    +    console.error('Invalid graph data structure');
    +    // Optionally, display an error message to the user
    +  }
    +} catch (error) {
    +  console.error('Failed to parse graph data:', error);
    +  // Optionally, display an error message to the user
    +}
    Suggestion importance[1-10]: 8

    Why: Validating the JSON file content before loading is a significant enhancement that prevents potential runtime errors and ensures the integrity of the graph data. This improves the reliability and security of the application.

    8
    Security
    Sanitize the filename to prevent invalid characters or potential security issues

    Sanitize the filename parameter in the saveGraph function to prevent potential
    security issues or invalid file names.

    packages/graph-editor/src/utils/saveGraph.ts [17]

    -link.download = filename + '.json';
    +const sanitizedFilename = filename.replace(/[^a-z0-9_\-]/gi, '_');
    +link.download = sanitizedFilename + '.json';
    Suggestion importance[1-10]: 8

    Why: Sanitizing the filename is an important security measure to prevent invalid characters or potential vulnerabilities. This ensures compatibility across different file systems and avoids unexpected behavior.

    8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant