Skip to content

Conversation

@roppazvan
Copy link
Contributor

@roppazvan roppazvan commented Mar 13, 2025

User description

  • ADd an input error annotation
  • Use the annotation to display an error message in the input sheet
Screenshot 2025-03-13 at 15 50 41

PR Type

  • Bug fix
  • Enhancement

Description

  • Add UI error message display in PortPanel

  • Revise external tokens node execution logic

  • Introduce annotatedInputError for input annotations

  • Update changeset patch notes for error handling


Changes walkthrough 📝

Relevant files
Enhancement
index.tsx
Add error message display in PortPanel component                 

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

  • Import Message component from UI
  • Check for input error using annotatedInputError
  • Render danger-styled Message when error exists
  • +9/-1     
    index.ts
    Introduce annotatedInputError constant                                     

    packages/graph-engine/src/annotations/index.ts

  • Define new constant annotatedInputError
  • Provide documentation for input error annotation
  • +7/-0     
    Bug fix
    externalTokens.ts
    Revise external tokens error handling logic                           

    packages/nodes-design-tokens/src/nodes/externalTokens.ts

  • Override execute to throw error on missing uri
  • Set error annotation when token loading fails
  • Clear errors when tokens load successfully
  • +29/-5   
    Documentation
    tame-bobcats-shop.md
    Document changeset update for token error enhancements     

    .changeset/tame-bobcats-shop.md

    • Add patch changeset for improved error handling
    +7/-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.
  • @roppazvan roppazvan requested a review from SorsOps March 13, 2025 14:01
    @changeset-bot
    Copy link

    changeset-bot bot commented Mar 13, 2025

    🦋 Changeset detected

    Latest commit: a155946

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

    This PR includes changesets to release 3 packages
    Name Type
    @tokens-studio/graph-engine-nodes-design-tokens Patch
    @tokens-studio/graph-editor Patch
    @tokens-studio/graph-engine 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

    @github-actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Behavior Change

    The new execute method now throws an error when no uri is provided, which alters the previous behavior where an empty output was set. Ensure that this change is compatible with downstream expectations.

    	throw new Error('No uri specified');
    }
    Error Display

    The Message component is used to display errors based on the annotatedInputError value. Confirm that the errorMessage is always defined, or provide a safe fallback to ensure user clarity.

    const errorMessage = port.annotations[annotatedInputError]?.message;
    
    const graph = useGraph();

    @github-actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error wrapper

    Wrap the asynchronous token loading with a try-catch block to capture unexpected
    exceptions during the load process.

    packages/nodes-design-tokens/src/nodes/externalTokens.ts [34-58]

    -const tokens = await this.load(uri);
    -if (!tokens) {
    -  // set this so we can show a red border around the node to indicate an error
    +let tokens;
    +try {
    +  tokens = await this.load(uri);
    +} catch (error) {
    +  tokens = null;
       this.error = new Error('Failed to load tokens');
       if (this.inputs['uri']) {
    -    // set this so we can show an error in the graph editor input sheet
         this.inputs['uri'].annotations[annotatedInputError] = {
           message:
    -        'Failed to load tokens. Check if the uri is valid and the set was not deleted or renamed.',
    +        'Failed to load tokens due to an exception. Check if the uri is valid.',
         };
       }
    +}
    +if (!tokens) {
    +  // token loading failed; exit early to prevent further processing
    +  return;
     } else {
       if (this.outputs && this.outputs.tokenSet) {
         this.outputs.tokenSet.set(tokens);
       }
    -  // clear the error if there is one
       if (this.error) {
         this.error = null;
         if (this.inputs['uri']) {
           delete this.inputs['uri'].annotations[annotatedInputError];
         }
       }
     }
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion enhances error handling by wrapping the token load in a try-catch block, which can catch unexpected exceptions. However, it alters the control flow by returning early on error, so while useful, its impact is moderate.

    Medium
    General
    Add error fallback

    Provide a default fallback text for the error message to ensure a meaningful message
    is always displayed.

    packages/graph-editor/src/components/portPanel/index.tsx [176]

    -{hasError && <Message appearance="danger">{errorMessage}</Message>}
    +{hasError && <Message appearance="danger">{errorMessage || "An error occurred."}</Message>}
    Suggestion importance[1-10]: 5

    __

    Why: Providing a default error message ensures a meaningful UI feedback when errorMessage is undefined. This is a minor yet useful usability enhancement.

    Low


    if (!tokens) {
    // set this so we can show a red border around the node to indicate an error
    this.error = new Error('Failed to load tokens');
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    This should be automatically populated as there is a run handler around a node that calls the execute function. It populates the error value with whatever error is thrown

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    I just tried with the panic node and it feels like the error is being swallowed, we need to find where this is occurring as its more fundamental and would solve the problem easier. Then we can just service the error message directly in the editor without needing an annotation

    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.

    4 participants