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 error boundary component #315

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Add error boundary component #315

merged 2 commits into from
Jun 19, 2024

Conversation

roppazvan
Copy link
Collaborator

@roppazvan roppazvan commented Jun 19, 2024

PR Type

Enhancement, Bug fix


Description

  • Added a new ErrorBoundaryContent component to handle error states and provide options to save the current graph state or reload the page.
  • Integrated ErrorBoundary in key components (DropPanel, GraphEditor, Inputsheet, OutputSheet) within layoutController and SubgraphExplorer.
  • Updated react-error-boundary dependency to version 4.0.13.
  • Added a changeset note for the enhancements.

Changes walkthrough 📝

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

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

  • Added a new ErrorBoundaryContent component to handle error states.
  • Implemented functionality to save the current graph state as a JSON
    file.
  • Included UI elements like Button and Text for user interaction during
    errors.
  • +44/-0   
    layoutController.tsx
    Integrate ErrorBoundary in layoutController components     

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

  • Wrapped key components (DropPanel, GraphEditor, Inputsheet,
    OutputSheet) in ErrorBoundary.
  • Integrated ErrorBoundaryContent as the fallback component.
  • +27/-11 
    specifics.tsx
    Add ErrorBoundary to SubgraphExplorer component                   

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

  • Wrapped GraphEditor in ErrorBoundary within SubgraphExplorer.
  • Integrated ErrorBoundaryContent as the fallback component.
  • +7/-3     
    Documentation
    angry-dolls-call.md
    Add changeset for error boundaries enhancement                     

    .changeset/angry-dolls-call.md

  • Added a changeset note for adding error boundaries to the graph editor
    panels.
  • +5/-0     
    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: 58dfcf1

    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

    @roppazvan roppazvan linked an issue Jun 19, 2024 that may be closed by this pull request
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Jun 19, 2024
    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 calling save.
    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 that includes this error boundary setup to reduce redundancy and improve maintainability.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure saved is not null before proceeding to create a Blob

    Consider handling the case where saved is null or undefined before attempting to create a
    Blob. This will prevent potential runtime errors if graphRef.save() fails to return a
    valid object.

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

     const saved = graphRef!.save();
    -const blob = new Blob([JSON.stringify(saved)], {
    -  type: 'application/json',
    -});
    +if (saved) {
    +  const blob = new Blob([JSON.stringify(saved)], {
    +    type: 'application/json',
    +  });
    +  // continue with the rest of the code
    +} else {
    +  console.error('Failed to save the graph.');
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that saved is not null or undefined before proceeding. This is a crucial check that enhances the robustness of the code.

    9
    Handle potential undefined graphRef to prevent runtime errors

    Remove the non-null assertion operator (!) and handle the case where graphRef might be
    undefined to prevent runtime errors.

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

    -const saved = graphRef!.save();
    +if (graphRef) {
    +  const saved = graphRef.save();
    +  // continue with the rest of the code
    +} else {
    +  console.error('Graph reference is not available.');
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by removing the non-null assertion operator and handling the case where graphRef might be undefined. This enhances the robustness of the code.

    9
    Best practice
    Use optional chaining to safely access properties on potentially undefined objects

    Use optional chaining when accessing saved.annotations[title] to avoid potential runtime
    errors if annotations is undefined or does not have a property title.

    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]: 8

    Why: This suggestion improves the code by using optional chaining, which is a best practice to avoid runtime errors when accessing properties on potentially undefined objects.

    8
    Use a type guard to ensure the object is of the expected type before using it

    Instead of forcibly casting mainGraph?.ref to ImperativeEditorRef | undefined, use a type
    guard to ensure the object is of the expected type before using it.

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

    -const graphRef = mainGraph?.ref as (ImperativeEditorRef | undefined);
    +const graphRef = mainGraph?.ref instanceof ImperativeEditorRef ? mainGraph.ref : undefined;
     
    Suggestion importance[1-10]: 7

    Why: Using a type guard is a best practice that ensures type safety and prevents potential runtime errors, although the current casting approach is also commonly used.

    7

    @SorsOps SorsOps merged commit d45e99c into master Jun 19, 2024
    2 checks passed
    @SorsOps SorsOps deleted the add-error-boundary-component 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.

    Make a error boundary on the graph editor
    2 participants