Refactor Dynamic Node Callbacks

[camielvs]

Description

generateDynamicNodeCallbacks has now been replaced with convertNodeCallbacksToTaskCallbacks, which is much simpler in logic and has clearer typing.

This relates to the automatic generation of Task Callbacks during node creation, and, to make the distinction clearer, the input NodeData type has been separated from the internal TaskNodeData type.

Background explanation
A Task Node's callbacks are defined at the Flow-Canvas level in useNodeCallbacks and require the taskId and nodeId to be passed in so ReactFlow & ComponentSpec can be updated accordingly. However, within a task node, where these callbacks are executed, taskId and nodeId are already known and are static. Thus, we currently have a transform that occurs when Task Nodes are created to inject the known ids into the callback function. This saves us from having to manually pass in the ids when executing the callbacks within a node (e.g. in TaskNodeProvider).

This PR simplifies that conversion function significantly and institutes a clear division between the internal TaskNode data and the external node data needed to create the task node.

This PR is intended to start laying the groundwork for a node compatibility layer to be introduced, which will ultimately involve id generation at the task node creation level. Having simplified task creation code will make this process easier.

No change to app functionality.

Note/Follow-up:

• Previously generateDynamicNodeCallbacks did the conversion dynamically. this new system requires the methods to be manually coded. Consider if there is a way to go back to a dynamic model so that any new methods don't need to be manually added

Related Issue and Pull requests

Closes https://github.com/Shopify/oasis-frontend/issues/334

Type of Change

• Cleanup/Refactor

Checklist

• I have tested this does not break current pipelines / runs functionality
• I have tested the changes on staging

Screenshots (if applicable)

Test Instructions

1. Create Task Nodes

• Open a pipeline editor
• Add multiple task nodes to the canvas
• Verify nodes render correctly with proper positioning

2. Test Node Callbacks

• setArguments: Modify task arguments and verify they persist
• setAnnotations: Move/resize nodes and verify position updates save
• onDelete: Delete a task node and verify it's removed from the spec
• onDuplicate: Duplicate a node (with/without selection) and verify the copy is created
• onUpgrade: If applicable, upgrade a component and verify the node updates

3. Create Input/Output Nodes

• Create/verify input nodes render correctly
• Create/verify output nodes render correctly
• Connect task nodes to input/output nodes

4. Edge Cases

• Load a complex existing pipeline to verify node reconstruction works
• Test with nodes that have no callbacks defined
• Test with read-only nodes

5. Regression Check

• Run through a complete pipeline creation workflow
• Save, reload, and verify state persistence
• Execute a pipeline and verify it runs successfully

Expected Results
All functionality should behave identically to before the refactor. No visual or functional changes should be observed.

Additional Comments

[camielvs]

• useNodeManager Hook #1082 : 2 dependent PRs (#930 , #1004 )
• Provide Node Manager #1081
• Add Node Manager #929
• Groundwork for Id Conversions #1090
• Separate IONodeData type from TaskNodeData type #928
• Refactor Dynamic Node Callbacks #927 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[maxy-shpfy]

For future: do we have any instance of Graph to be readonly, BUT connectable? I feel that this is single property "readOnly".

[camielvs]

It's more the opposite scenario that's the concern: the graph is not readonly and node connection is disabled - specifically this lock button

I've been growing increasingly discontent with "readOnly" and how it's used specifically for the run route. I think in future we will want to take a look and properly make readOnly an actual read-only state regardless of the route.

[maxy-shpfy]

For future: Do we need to have more arbitrary properties on this object? extending it from Record makes it harder to type causing weird code to appear down the road, e.g.

TaskNode:

const typedData = useMemo(() => data as TaskNodeData, [data]);

[camielvs]

I vaguely recall trying to not extend and ReactFlow threw up a huge amount of type errors. There may be some larger changes need to our type structure to make this happen, but I do agree with the idea.

[maxy-shpfy]

For future: highlighted might not belong to the Node properties/data, but I'm not sure yet.

[camielvs]

Most likely the node overlay or highlighter you made would solve this but we haven't gotten around to looking into it.

[maxy-shpfy]

Tested, no immediate issues found