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

#6228 [enhancement]: Outline error nodes in red #7444

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

achuthanmukundan00
Copy link

@achuthanmukundan00 achuthanmukundan00 commented Dec 14, 2024

Summary

This PR adds a red box shadow ("outline") to nodes that have missing required inputs or execution errors. Addresses issue #6228

image

Key Changes

  1. Error State Tracking:

    • Added isErrorNode and fieldErrors to the InvocationNodeData type for tracking the overall error state of a node and specific errors for individual inputs.
    • Dynamically update fieldErrors and isErrorNode in the node's data when an input field becomes valid/invalid.
  2. Visual Feedback for Error Nodes:

    • I added a red outline at first, but that disappeared when zoomed out, making this feature useless for large workflows.
      image
      image

    • Instead I opted to use a box shadow which remained visible even when zoomed out very far.

    image
    image

  3. State Initialization:

    • Updated all places where invocation nodes are initialized (e.g., buildInvocationNode, graphToWorkflow) to include default values for isErrorNode and fieldErrors.
    • Modified test cases and mock nodes (e.g., validateWorkflow.test.ts) to account for the new fields.
  4. Behavior:

    • Nodes with missing or invalid inputs are highlighted with a red shadow.
    • Once all required inputs are valid, the red shadow disappears automatically.
  5. Testing:

    • Functionality was manually tested locally, no frontend tests are implemented in line with project conventions

@psychedelicious
Copy link
Collaborator

Thank you for such a well-documented PR!

I like the user experience this change provides, but I don't think this is the right implementation.

Field validity depends on node/field templates, which are dynamic and could change at any time. The error state is thus dependent on the templates and field's value. It is derived state.

The solution in this PR persists the error state by writing it into the workflow. Technically, this works, but the error state should not be in the workflow (or in the state management system, which persists the app state to browser storage).

I think it will be fairly straightforward to do this in a hook that checks all field error states for a given node and returns a single boolean (or whatever you think is appropriate). Taking a look at how field validity is handled...

There is a redux selector in useFieldIsInvalid that derives a field's validity. It has a few dependencies:

  • The node id and field name.
  • connectionState.isConnected via useConnectionState. This boolean is itself derived via redux selector.
  • The field template, via useFieldInputTemplate. That hook in turn uses useNodeTemplate.

The selector creators could be extracted into builder functions instead of inlined in the hooks, then be used in the proposed useNodeIsInvalid hook and the existing useFieldIsInvalid hook.

How does that sound to you?

@@ -71,6 +75,7 @@ const NodeWrapper = (props: NodeWrapperProps) => {
transitionDuration="0.1s"
cursor="grab"
opacity={opacity}
boxShadow={isErrorNode ? '0 0 10px 6px #7F3A41' : undefined}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Instead of a hex code, we can use a CSS variable. For example, 'error.500' resolves to var(--invoke-colors-error-500).

There's also a hook:

const [error500] = useToken('colors', ['error.500']);

At runtime, error500 will be the CSS variable as a string, so you could use it in the shadow:

boxShadow={isErrorNode ? `0 0 10px 6px ${error500}` : undefined}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants