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

fix(273): Fix input output execution #274

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

georgebuciuman
Copy link
Contributor

@georgebuciuman georgebuciuman commented Jun 13, 2024

User description

Fixes #273


PR Type

Bug fix, Enhancement


Description

  • Improved value handling and rendering in nodeV2.tsx by checking for undefined and adding support for string type.
  • Enhanced input and output node execution to dynamically manage outputs based on inputs.
  • Added a new method getAllOutputs in the node class to retrieve all outputs.

Changes walkthrough 📝

Relevant files
Enhancement
nodeV2.tsx
Improve value handling and rendering in node wrapper.       

packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx

  • Changed condition to check for undefined instead of falsy values.
  • Added handling for string type in getValuePreview.
  • Updated JSX to use nullish coalescing and conditional rendering.
  • +6/-4     
    input.ts
    Enhance input node execution to manage outputs dynamically.

    packages/graph-engine/src/nodes/generic/input.ts

  • Added logic to retain existing outputs if they match inputs.
  • Removed outputs that no longer have corresponding inputs.
  • +16/-9   
    output.ts
    Enhance output node execution to manage outputs dynamically.

    packages/graph-engine/src/nodes/generic/output.ts

  • Added logic to retain existing outputs if they match inputs.
  • Removed outputs that no longer have corresponding inputs.
  • +16/-9   
    node.ts
    Add method to retrieve all outputs in node class.               

    packages/graph-engine/src/programmatic/node.ts

  • Added getAllOutputs method to retrieve all node outputs.
  • Reorganized imports for better readability.
  • +11/-5   

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

    @georgebuciuman georgebuciuman requested a review from SorsOps June 13, 2024 13:22
    @georgebuciuman georgebuciuman self-assigned this Jun 13, 2024
    Copy link

    changeset-bot bot commented Jun 13, 2024

    ⚠️ No Changeset found

    Latest commit: 65997de

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a 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:
    In both input.ts and output.ts, the logic for deleting outputs not present in inputs might inadvertently delete outputs that are still needed if not properly managed or if there are asynchronous operations affecting outputs. This needs careful review to ensure outputs are managed correctly without unintended side effects.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent potential runtime errors by ensuring necessary properties exist before use

    Add a check to ensure rawInput.type exists before adding an output to prevent potential
    runtime errors.

    packages/graph-engine/src/nodes/generic/input.ts [32-36]

    -if(!(input in outputs)) {
    +if(!(input in outputs) && rawInput.type) {
       this.addOutput(input, {
         type: rawInput.type,
         visible: true,
       });
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error, which is crucial for the stability of the application. Ensuring rawInput.type exists before use is a good practice.

    9
    Maintainability
    Ensure consistent object references for better maintainability and to avoid errors

    Ensure consistent use of input.name instead of port.name to avoid potential reference
    errors or inconsistencies.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx [265-266]

     {inlineValuesValue && <Text css={{ fontSize: '$small', color: '$gray12' }}>{getValuePreview(input.value, input.type) ?? input.name}</Text>}
    -{port.value !== undefined ? <Text css={{ fontSize: '$medium', color: '$gray11' }}>{input.name}</Text> : null}
    +{input.value !== undefined ? <Text css={{ fontSize: '$medium', color: '$gray11' }}>{input.name}</Text> : null}
     
    Suggestion importance[1-10]: 7

    Why: Ensuring consistent object references improves maintainability and reduces the chance of errors. However, the impact is minor as it only affects readability and consistency.

    7
    Enhancement
    Improve code clarity by renaming the function to better reflect its functionality

    Use a more descriptive function name for getAllOutputs to clarify that it returns output
    values, not output objects.

    packages/graph-engine/src/programmatic/node.ts [285-289]

    -getAllOutputs = <T = Record<string, any>>(): T => {
    +getAllOutputValues = <T = Record<string, any>>(): T => {
       return Object.fromEntries(
         Object.entries(this.outputs).map(([key, value]) => [key, value.value])
       ) as T;
     };
     
    Suggestion importance[1-10]: 6

    Why: Renaming the function to better reflect its functionality improves code clarity and maintainability, but it is not a critical change.

    6
    Possible issue
    Improve the robustness of the value check by handling both null and undefined

    Replace the strict equality check for undefined with a falsy check to handle both null and
    undefined values gracefully.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx [166-167]

    -if (value === undefined) {
    +if (!value) {
       return null;
     }
     
    Suggestion importance[1-10]: 4

    Why: While handling both null and undefined can be useful, the original change was likely made to differentiate between null and other falsy values like 0 or ''. The suggestion might reintroduce issues the PR aimed to fix.

    4

    @SorsOps
    Copy link
    Member

    SorsOps commented Jun 13, 2024

    Looks like there's a side effect with not setting the node typing correctly

    @SorsOps SorsOps self-requested a review June 13, 2024 13:32
    @georgebuciuman
    Copy link
    Contributor Author

    Looks like there's a side effect with not setting the node typing correctly

    @SorsOps We should also set the type of the output to always be the same as the input then, I guess

    @SorsOps SorsOps merged commit 6d14dbc into major/strong-typing Jun 14, 2024
    2 checks passed
    @SorsOps SorsOps deleted the fix-input-output-execuion branch June 14, 2024 09:21
    SorsOps added a commit that referenced this pull request Jun 14, 2024
    SorsOps added a commit that referenced this pull request Jun 14, 2024
    @SorsOps SorsOps restored the fix-input-output-execuion branch June 15, 2024 16:30
    SorsOps added a commit that referenced this pull request Jun 18, 2024
    * Stash
    
    * Stash
    
    * WIP
    
    * Cleanup inline nodes and re-enable array nodes
    
    * Add drop panel to show windows
    
    * Remove settings dialog
    
    * Fix bug with values sticking if selecting between two nodes of the same type
    
    * Fix not reinvalidating the node settings
    
    * Fix issue with duplication
    
    * Fix audio source
    
    * Update UI package and simplify dialog
    
    * Cleanup file explorer
    
    * Update files system
    
    * Add support for audio buffer conversions
    
    * Fix changed bufferschema refs in audio pkg
    
    * add ceil and floor nodes
    
    * Subgraphs mostly working
    
    * Cleanup not selecting the correct graph on tab transition
    
    * Cleanup typescript types
    
    * Cleanup UI and fix deserialization of edges
    
    * Additional UI cleanup
    
    * Fix padding on settings
    
    * Minor update on documentation
    
    * Cleanup on styles
    
    * Fix nodenext resolution
    
    * Cleanup ts config
    
    * Cleanup ts import for consistency
    
    * Replace all .ts? imports
    
    * Fix issue with typing
    
    * Add assets for apple touch startup
    
    * Add initial k8s for mailcatcher for local dev
    
    * Add docker-compose for open telemetry for nextjs
    
    * Cleanup styles
    
    * Add postgres for persistence
    
    * Fix circular import
    
    * Fix menu Item boolean value issue
    
    * Remove console log
    
    * Add prometheus and otel config
    
    * Set default to be darktheme
    
    * Cleanup menubar rest render spread
    
    * Move controls for token arrays to the token package
    
    * Cleanup old redux models
    
    * Remove global css usage
    
    * Set darktheme to be the default and remove global css usage
    
    * Remove old output providers from preview. They will need to be reworked to handle routing the data
    
    * Add support for client side storage providers
    
    * Add mailcatcher and postgres to docker-compose
    
    * Add otel instrumentation
    
    * Update packge for otel
    
    * Setup ory kratos, replace mailcatcher with mailslurper
    
    * Add tracing for debugging
    
    * Restyle node (#245)
    
    * restyle node
    
    * restyle node
    
    * restyle node
    
    * remove unused background
    
    * remove unused background
    
    * remove string from import
    
    * fix pointer events on handle
    
    * Fix storybook setup
    
    * add rail
    
    * style graph editor
    
    * update node style to have more contrast
    
    * add exp and sqrt node
    
    * Add initial scaffold for backend
    
    * change icon for type any
    
    * mute down handle background
    
    * fix transparent background on panels
    
    * Fix graph loading
    
    * Swap ui to use scss and remove unnecessary styling
    
    * Add scss support for Marco and fix non .js imports
    
    * fix reference to styles.css to .scss
    
    * Add example of barebones implementation of the graph-editor
    
    * Cleanup file
    
    * Fix issue with zustand not being the same
    
    * Extend color ranges and fix colors showing up correctly in port previews
    
    * Remove console log during graph serialization
    
    * Update backend to use prisma, add in DI, setup ory kratos connection
    
    * Add oathkeeper and kratos setup
    
    * Add next link for rail
    
    * Remove drag initiator styling
    
    * Add generated SDK
    
    * Update backend package
    
    * Remove copied changelog
    
    * Get backend docker build working
    
    * Move docker resources and add a readme entry for running the docker compose
    
    * Remove all icon packages except for iconoir
    
    * Remove import assertion for importing json.  We assume this is unnecessary
    
    * Remove DB, we will assume migration as needed
    
    * Replace graphlib with later version
    
    * Ignore users local db
    
    * Move docker compose env to root
    
    * Graphlib replace
    
    * Optimize icon usage
    
    * Add basic keto client, setup proper documentation
    
    * Add initial controllers
    
    * Update SDK
    
    * Add SDK to UI
    
    * Cleanup docs
    
    * Update edge docs
    
    * Fix oathkeeper origins
    
    * Expand models
    
    * Ignore noisy generated changes
    
    * Ignore noisy files
    
    * Export typing on the serialized graph
    
    * Clean cors
    
    * Remove cors as expected to be handled externally
    
    * Update kratos
    
    * Fix ref
    
    * Untrack generated
    
    * ignore json import which should be external
    
    * Persist dynamic type
    
    * Integrate API
    
    * Remove console log
    
    * Remove prometheus
    
    * Add example for preview
    
    * add preview nodes
    
    * Force generation on build
    
    * add viewport saving, math expression visualisation, default values for number
    
    * add gradient stops and gradient handle positions
    
    * Setup storybook test
    
    * remove gradient handles, add gradient schema
    
    * Export util functions
    
    * Don't require typing on input unless its intentionally dynamic
    
    * Add storybook example
    
    * Add delayed updates to controls, fix issue with choosing a type and not having the control update
    
    * Fix non standard curve change that was breaking preview
    
    * Fix issue with token set controls and expose them correctly
    
    * Fix token saving not serializing correctly
    
    * Remove live update of position annotation, was causing unnecessary recalc
    
    * Clean up hot key usage and first pass at fixing subgraphs
    
    * Add manifest for PWA in the future
    
    * Cleanup UI and start app router migration
    
    * Clean up docs
    
    * Mention initial db setup for prisma
    
    * Setup for gitpod
    
    * Cleanup build type errors
    
    * More gitpod setup
    
    * Additional setup for app router conversion
    
    * Fix for Marco
    
    * Fix style ordering
    
    * Update graph-engine to use tsup over rollup. Remove aliasing as the esbuild does not seem to respect the alias transform
    
    * Finish converting other node packages to use tsup
    
    * Fix missing tsx for tsup
    
    * Fix issue with token preview if undefined
    
    * Fix menu bombing due to nextjs removing svg loader
    
    * Remove old rollup file
    
    * Continued rollup removal
    
    * add new colors for types on handles
    
    * change size of handle icon
    
    * Update tsup configs
    
    * Fix type problem with engine bundling
    
    * Fix build splitting
    
    * Cleanup hotkeys
    
    * Fix external sanitize usage
    
    * add radix ui colors
    
    * Cleanup docker build
    
    * Update dep for design-tokens
    
    * Fix host name on deploy
    
    * Fix for readiness probe
    
    * Stop redirecting on index
    
    * Remove second level domain
    
    * Revert
    
    * Setup lit example of context usage
    
    * Add typograph preview specifics
    
    * Add building the backend and docs to workflow
    
    * Make the docs use the same dev command as everything else
    
    * Fix variadic typing for Marco
    
    * Change build command for documentation
    
    * Allow dynamic inputs for array map subgraph
    
    * Fix issue with subgraph typing for tokens
    
    * Minor fix to display the font size
    
    * Add cypress testing to graph-editor
    
    * Deploy graph docs
    
    * Re add up to date position changes
    
    * Add basic vector 2 support
    
    * Simplify typing on arrays
    
    * Cleanup from changes
    
    * Add setting to hide minimap and to show the title in the subgraph
    
    * Fix finding nodes
    
    * Improve searching
    
    * Add copy value to port
    
    * Stop subgraph from gathering input values every loop
    
    * Add reduce subgraph
    
    * Expose convert color
    
    * Fix poline
    
    * Make series consistent with naming
    
    * Add advanced blending to design tokens
    
    * Cleanup schemas on token arrays
    
    * Fix cases where input is not detected
    
    * Add secret and deployment
    
    * Add alignment and distribution
    
    * Add better support for network messages
    
    * Fix bug where we don't disconnect an edge and minor cleanup in tests
    
    * Complete subgraph creation , convert output to multi output
    
    * Cleanup typing on graph
    
    * Update docs
    
    * Update docs
    
    * Fix issue with subgraph not reconnecting nodes
    
    * Fix grouping
    
    * Start releasing on dockerhub
    
    * Remove tauri
    
    * Update versions
    
    * Fix ignore
    
    * Update Gimlet
    
    * Update deployment for docs and backend
    
    * fix bugs
    
    * rm log
    
    * fix input inputs
    
    * add dimension
    
    * 250: fix shift+k (#251)
    
    * fix(250): Fix quick search triggered in input fields
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * Reorder variadic ports (#258)
    
    
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * fix color wheel (#261)
    
    * Show value preview in nodes (#257)
    
    * Show value preview in nodes
    
    * Add dot for variadic and other improvements
    
    * Fix edge case with object variadics and allow toggling on and off of inline values
    
    * Merge george's changes
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * fix valid check (#263)
    
    * Feat/debugger (#259)
    
    * Add debugger and remove flamegraph
    
    * add xyz65 to color convert
    
    * fix flatten node
    
    * add flatten alpha node
    
    * fix(1013): Subgraph ouput
    
    * Setup alpha and beta deploys (#270)
    
    * Hide unfinished work (#271)
    
    * Setup alpha and beta deploys
    
    * Hide unfinished work
    
    * Fix arrayMap and arrayReduce
    
    * fix(273): Fix input output execution
    
    * Copy type to outputs
    
    * fix(273): Fix input output execution (#274)
    
    * fix(273): Fix input output execution
    
    * Copy type to outputs
    
    * Revert "fix(273): Fix input output execution (#274)" (#275)
    
    This reverts commit 6d14dbc.
    
    * Revert "Fix input output execution" (#277)
    
    * feat: Update input.ts to handle dynamic type changes in Input.setValue()
    
    * Feat/documentation (#279)
    
    * Add script to generate documentation
    Remove 'NodeTypes` in favor of direct values for the node types
    Change input/output nodes to not clear and instead diff values
    Fixed some issues with flexbox of the node wrapper
    Add in an array indicator
    Fixed subgraph to be reactive to additional outputs
    Fix find menu
    
    * add contrast alpha node (#272)
    
    * add contrast alpha node
    
    * Remove console logs
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * UI rework (#278)
    
    * add subtitle and style ui
    
    * Fix select and download
    
    * Add constant for graph id
    
    ---------
    
    Co-authored-by: Marco Christian Krenn <marco.krenn@gmail.com>
    
    * prevent delete when annotation is present (#281)
    
    * Fix inputs which are any type to be overridden with new values. (#282)
    
    Fix typing with the array subgraph
    Allow creating array input types
    
    * add colors to legend (#283)
    
    * Feat/token sets (#284)
    
    * Fix inputs which are any type to be overridden with new values.
    Fix typing with the array subgraph
    Allow creating array input types
    
    * Expose additional vars in the array map and add the ability to convert to a token set
    
    * Fix/command k (#287)
    
    * Fix inputs which are any type to be overridden with new values.
    Fix typing with the array subgraph
    Allow creating array input types
    
    * Expose additional vars in the array map and add the ability to convert to a token set
    
    * Fix command palette messing up positions
    
    * Fix contrasting alpha import
    
    * Add markdown support
    
    * Change objectmerge and flatten to not  be variadic
    
    * fix: input bug
    
    * Remove console logs
    
    * Remove console logs
    
    * Fix color wheel (#300)
    
    * fix color wheel calculation
    
    * add changeset
    
    * fix position reference (#298)
    
    * fix pos reference
    
    * spread annotations
    
    * Feat/prep master (#303)
    
    * Fix can delete.
    Fix issues with variadic passing through type for any array.
    Prep for the major overhaul
    Add back in examples
    
    * Setup gimlet for deploys
    
    ---------
    
    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
    Co-authored-by: popdrazvan <popdrazvan@gmail.com>
    Co-authored-by: Vaibhav Bhosale <mail2vsbhosale@gmail.com>
    Co-authored-by: Jan Six <six7@github.com>
    Co-authored-by: Jan Six <six.jan@gmail.com>
    
    ---------
    
    Co-authored-by: Marco Christian Krenn <marco.krenn@gmail.com>
    Co-authored-by: popdrazvan <popdrazvan@gmail.com>
    Co-authored-by: georgebuciuman <georgebuciuman@gmail.com>
    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
    Co-authored-by: Vaibhav Bhosale <mail2vsbhosale@gmail.com>
    Co-authored-by: Jan Six <six7@github.com>
    Co-authored-by: Jan Six <six.jan@gmail.com>
    @SorsOps SorsOps deleted the fix-input-output-execuion 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