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

Add boundary component #310

Closed
wants to merge 2 commits into from
Closed

Conversation

roppazvan
Copy link
Collaborator

@roppazvan roppazvan commented Jun 19, 2024

User description

  • When we have no graph the CTA is reload page. When we have a graph it's download graph
Screenshot 2024-06-19 at 12 31 48 Screenshot 2024-06-19 at 12 37 08 Screenshot 2024-06-19 at 12 39 27

PR Type

Enhancement, Bug fix


Description

  • Added a new ErrorBoundaryContent component to handle error states, allowing users to save and download the current graph state or reload the page.
  • Integrated ErrorBoundary in key components within the layout controller and SubgraphExplorer to handle errors gracefully.
  • Updated react-error-boundary dependency version to 4.0.13.

Changes walkthrough 📝

Relevant files
Enhancement
ErrorBoundaryContent.tsx
Add `ErrorBoundaryContent` component for error handling   

packages/graph-editor/src/components/ErrorBoundaryContent.tsx

  • Added new ErrorBoundaryContent component to handle error states.
  • Implemented logic to save and download the current graph state.
  • Displayed appropriate messages and actions based on the presence of a
    graph reference.
  • +44/-0   
    layoutController.tsx
    Integrate `ErrorBoundary` in layout controller components

    packages/graph-editor/src/editor/layoutController.tsx

  • Wrapped key components with ErrorBoundary to handle errors gracefully.
  • Integrated ErrorBoundaryContent as the fallback component.
  • +27/-11 
    specifics.tsx
    Add error boundary to `SubgraphExplorer` component             

    packages/graph-editor/src/registry/specifics.tsx

  • Wrapped GraphEditor component with ErrorBoundary in SubgraphExplorer.
  • Integrated ErrorBoundaryContent as the fallback component.
  • +7/-3     
    Dependencies
    package.json
    Update `react-error-boundary` dependency version                 

    packages/graph-editor/package.json

  • Updated react-error-boundary dependency version from 4.0.11 to 4.0.13.

  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    changeset-bot bot commented Jun 19, 2024

    🦋 Changeset detected

    Latest commit: 72a6c2b

    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 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The onSave function in ErrorBoundaryContent.tsx assumes graphRef is not null when calling graphRef!.save(). This could lead to runtime errors if graphRef is null. Consider adding a null check before using graphRef.
    Code Duplication:
    The ErrorBoundary wrapper with ErrorBoundaryContent fallback is repeated multiple times across different components. Consider creating a higher-order component or a custom wrapper component to reduce duplication and improve maintainability.

    @roppazvan roppazvan linked an issue Jun 19, 2024 that may be closed by this pull request
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for undefined graphRef before saving

    Consider handling the case where graphRef is undefined before calling graphRef!.save().
    This will prevent potential runtime errors if graphRef is null or undefined.

    packages/graph-editor/src/components/ErrorBoundaryContent.tsx [12]

    -const saved = graphRef!.save();
    +if (graphRef) {
    +  const saved = graphRef.save();
    +  ...
    +} else {
    +  // Handle the case where graphRef is undefined
    +}
     
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a potential runtime error by adding a check for graphRef before calling its save method, which is crucial for preventing crashes.

    10
    Best practice
    Prevent runtime errors by using optional chaining when accessing properties

    Use optional chaining (?.) when accessing properties on potentially undefined objects to
    prevent runtime errors.

    packages/graph-editor/src/components/ErrorBoundaryContent.tsx [19]

    -link.download = (saved.annotations[title] || 'graph') + `.json`;
    +link.download = (saved?.annotations?.[title] || 'graph') + `.json`;
     
    Suggestion importance[1-10]: 9

    Why: Using optional chaining is a good practice to prevent runtime errors when accessing properties on potentially undefined objects, making the code more robust.

    9
    Use React state and effect for managing download operations

    Replace the direct manipulation of the DOM with React's state management to handle the
    download link creation and triggering. This approach aligns with React's best practices
    for DOM interactions.

    packages/graph-editor/src/components/ErrorBoundaryContent.tsx [17-21]

    -const link = document.createElement('a');
    -link.href = url;
    -link.download = (saved.annotations[title] || 'graph') + `.json`;
    -document.body.appendChild(link);
    -link.click();
    +// Use React state to manage the download URL
    +const [downloadUrl, setDownloadUrl] = useState(null);
    +useEffect(() => {
    +  if (downloadUrl) {
    +    const link = document.createElement('a');
    +    link.href = downloadUrl;
    +    link.download = (saved.annotations[title] || 'graph') + `.json`;
    +    document.body.appendChild(link);
    +    link.click();
    +    document.body.removeChild(link); // Clean up
    +  }
    +}, [downloadUrl]);
    +// Set the URL in your save function
    +setDownloadUrl(url);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion aligns with React's best practices by using state and effect hooks for managing the download link, improving code maintainability and readability.

    8
    Enhancement
    Improve user feedback for error scenarios where graphRef is unavailable

    Consider using a more descriptive error message or UI indication when graphRef is not
    available, instead of just disabling the download functionality.

    packages/graph-editor/src/components/ErrorBoundaryContent.tsx [32]

    -It looks like an error occurred. Try reloading the page.
    +It looks like an error occurred and the graph reference is not available. Please try reloading or contact support if the issue persists.
     
    Suggestion importance[1-10]: 7

    Why: Enhancing the error message provides better user feedback, which can improve user experience by giving more context about the issue and possible next steps.

    7

    Copy link
    Member

    @SorsOps SorsOps left a comment

    Choose a reason for hiding this comment

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

    Let's add a changeset for this and I think its ready to merge

    @roppazvan roppazvan changed the base branch from major/strong-typing to master June 19, 2024 11:28
    @roppazvan roppazvan changed the base branch from master to major/strong-typing June 19, 2024 11:28
    @roppazvan
    Copy link
    Collaborator Author

    close as I opnend another one based on master

    @roppazvan roppazvan closed this Jun 19, 2024
    @SorsOps SorsOps deleted the add-error-boundary branch July 3, 2024 09:33
    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.

    2 participants