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

Next #645

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

Next #645

wants to merge 5 commits into from

Conversation

SorsOps
Copy link
Member

@SorsOps SorsOps commented Jan 21, 2025

User description

  • Changes the graph editor to expose a system object to better manipulate the editor programmatically

PR Type

Enhancement, Tests, Documentation


Description

  • Introduced a new System object for centralized configuration and state management.

  • Refactored components to use System for settings and data, replacing Redux selectors.

  • Added new features including unified input/output panels and marketplace integration.

  • Enhanced test coverage with async node loading and graph execution tests.


Changes walkthrough 📝

Relevant files
Enhancement
20 files
index.tsx
Refactor settings panel to use `System` object                     
+161/-168
graph.tsx
Integrate `System` object into graph editor logic               
+32/-44 
nodeV2.tsx
Update node wrapper to use `System` settings                         
+66/-34 
preview.tsx
Add framework preview panel with listener                               
[link]   
edge.tsx
Refactor edge rendering to use `System` settings                 
+104/-81
layoutController.tsx
Update layout controller to use `System` tab loader           
+17/-36 
array.tsx
Refactor array control to use `System` settings                   
+8/-11   
data.tsx
Add layout data factory for default configurations             
+166/-0 
index.tsx
Refactor editor to initialize with `System` object             
+26/-56 
index.tsx
Refactor port panel to use `System` controls                         
+16/-11 
index.tsx
Update command palette to handle async node creation         
+19/-14 
paneContextMenu.tsx
Refactor pane context menu to use `System` settings           
+8/-9     
index.tsx
Introduce `System` class for centralized configuration     
+98/-0   
registry.ts
Remove unused Redux registry state                                             
+5/-60   
dropPanel.tsx
Refactor drop panel to use `System` panel items                   
+7/-27   
index.tsx
Update hotkeys to use `System` settings                                   
+7/-19   
groups.tsx
Add tab group configurations for layout                                   
+78/-0   
index.tsx
Refactor input sheet for cleaner layout                                   
+10/-43 
marketplace.ts
Add new marketplace API endpoints for graph retrieval       
+44/-3   
settings.tsx
Add `SystemSettings` class for configurable editor settings
+91/-0   
Additional files
77 files
foo-bar.md +5/-0     
light-avocados-hammer.md +5/-0     
package.json +2/-0     
readme.md +4/-0     
selectionContextMenu.tsx +2/-2     
color.tsx +4/-7     
curve.tsx +20/-18 
interface.tsx +2/-0     
numeric.tsx +5/-7     
slider.tsx +5/-7     
string.tsx +3/-6     
text.tsx +4/-6     
findDialog.tsx +6/-9     
passthroughNode.tsx +3/-4     
DragItem.tsx +3/-4     
NodeEntry.tsx +8/-29   
dropPanel.module.css +15/-0   
index.ts +1/-0     
nodeEntry.module.css +15/-0   
dynamicInputs.tsx +0/-2     
index.tsx +3/-4     
index.tsx +6/-38   
styles.module.css +10/-0   
index.tsx +59/-0   
styles.module.css +14/-0   
add.tsx +6/-7     
toolbar.tsx +5/-4     
copyNodes.tsx +2/-2     
createNode.tsx +7/-6     
duplicate.ts +1/-7     
provider.tsx +3/-2     
editorTypes.ts +7/-35   
useAutolayout.ts +5/-9     
index.tsx +8/-27   
utils.ts +45/-0   
useOpenPanel.tsx +1/-1     
index.tsx +2/-1     
index.tsx +6/-36   
index.ts +0/-2     
root.ts +0/-2     
settings.ts +0/-133 
index.ts +0/-1     
registry.ts +0/-22   
roots.ts +0/-1     
settings.ts +0/-42   
store.tsx +0/-2     
icon.tsx +1/-1     
hook.tsx +8/-0     
vitest.config.ts +19/-0   
package.json +4/-0     
graph.ts +3/-3     
types.ts +4/-2     
subgraph.ts +7/-5     
input.ts +1/-1     
port.ts +1/-1     
basic.test.ts +2/-7     
loader.test.ts +71/-0   
nodeUsage.test.ts +3/-1     
index.tsx +9/-2     
token.tsx +2/-2     
tokenArray.tsx +58/-0   
tokenSet.tsx +8/-5     
package.json +1/-1     
schema.prisma +1/-0     
marketplace.ts +88/-1   
layout.ts +0/-80   
nodeTypes.tsx +4/-2     
styles.module.css +5/-0     
tabLoader.tsx +44/-0   
toolbar.tsx +4/-4     
aiSummary.tsx [link]   
marketPlace.tsx +52/-0   
index.tsx +8/-0     
styles.module.css +13/-0   
index.tsx +41/-0   
smoothShadow.json +2801/-0
default.json +73/-0   

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link

changeset-bot bot commented Jan 21, 2025

🦋 Changeset detected

Latest commit: 83753c9

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

This PR includes changesets to release 2 packages
Name Type
@tokens-studio/graph-editor Major
@tokens-studio/graph-engine Major

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

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

The system.nodeLoader is used in multiple places to dynamically load node factories. Ensure that the nodeLoader implementation is robust and handles errors gracefully, especially when a requested node type is not found.

      nodeLoader: system.nodeLoader,
      iconLookup,
      customUI: customNodeMap,
      dropPanelPosition,
      dispatch,
    }),
  [
    reactFlowInstance,
    graph,
    system.nodeLoader,
    iconLookup,
    customNodeMap,
    dropPanelPosition,
    dispatch,
  ],
);

useImperativeHandle(
  refProxy,
  () => ({
    clear: () => {
      clear(reactFlowInstance, graph);
    },
    save: () => {
      const serialized = graph.serialize();
      //Lazily handle the additional metadata injection
      serialized.annotations[uiViewport] = reactFlowInstance.getViewport();
      serialized.annotations[uiVersion] = version;
      return serialized;
    },
    loadRaw: async (serializedGraph) => {
      if (internalRef.current) {
        await graph.deserialize(serializedGraph, system.nodeLoader);
Performance Concern

The CustomEdgeInner component dynamically determines the edge path function based on settings.edgeType. This could introduce performance overhead if the edge type changes frequently. Consider memoizing the edge function if possible.

  ({
    settings,
    id,
    sourceX,
    sourceY,
    targetX,
    targetY,
    sourcePosition,
    targetPosition,
    style = {},
    data,
    markerEnd,
  }: ICustomEdge) => {
    const graph = useLocalGraph();

    const edge = graph.getEdge(id);
    let col = undefined;

    if (edge) {
      const sourceNode = graph.getNode(edge?.source);

      const sourcePort = sourceNode?.outputs[edge?.sourceHandle];

      if (sourcePort) {
        const { backgroundColor } = extractColor(sourcePort as Port);
        col = backgroundColor;
      }
    }

    let edgeFn;
    switch (settings.edgeType) {
      case EdgeType.bezier:
        edgeFn = getBetterBezierPath;
        break;
      case EdgeType.simpleBezier:
        edgeFn = getSimpleBezierPath;
        break;
      case EdgeType.smoothStep:
        edgeFn = getSmoothStepPath;
        break;
      case EdgeType.straight:
        edgeFn = getStraightPath;
        break;
      default:
        edgeFn = getBetterBezierPath;
    }

    const [edgePath] = edgeFn({
      sourceX,
      sourceY,
      sourcePosition,
      targetX,
      targetY,
      targetPosition,
    });

    return (
      <>
        <path className="react-flow__edge-path-back" d={edgePath} />
        <path
          id={id}
          className="react-flow__edge-path"
          d={edgePath}
          style={{ ...style, stroke: col }}
          markerEnd={markerEnd}
        />
        <path
          d={edgePath}
          fill="none"
          strokeOpacity="0"
          strokeWidth="20"
          className="react-flow__edge-interaction"
        />
        <text>
          <textPath
            href={`#${id}`}
            style={{ fontSize: 12 }}
            startOffset="50%"
            textAnchor="middle"
          >
            {data?.text}
          </textPath>
        </text>
      </>
    );
  },
);
Potential Memory Leak

The useFindOutput hook sets up a subscription to the graph's nodeAdded event but does not ensure cleanup in all cases. Verify that the unsub function is always called to prevent memory leaks.

const useFindOutput = (graph: Graph) => {
	const [outputNode, setOutputNode] = useState<Node | undefined>();

	useEffect(() => {
		let unsub = () => {};
		const foundOutput = outputNode || findOutput(graph);
		//Add a subscription to wait till we find it
		if (!foundOutput) {
			const unsubscribe = graph.on('nodeAdded', x => {
				if (x.factory.type == 'studio.tokens.generic.output') {
					setOutputNode(x);
				}
			});

			unsub = unsubscribe;
		} else {
			setOutputNode(foundOutput);
		}
		return unsub;
	}, [graph]);

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add error handling for async function

Ensure that the handleSelectItem function properly handles errors from the
handleSelectNewNodeType promise to avoid unhandled promise rejections.

packages/graph-editor/src/components/commandPalette/index.tsx [98-102]

 const handleSelectItem = async (item) => {
-  const newNode = await handleSelectNewNodeType({
-    position: reactflow.screenToFlowPosition(cursorPositionRef.current),
-    ...item,
-  });
+  try {
+    const newNode = await handleSelectNewNodeType({
+      position: reactflow.screenToFlowPosition(cursorPositionRef.current),
+      ...item,
+    });
+  } catch (error) {
+    console.error('Error selecting item:', error);
+  }
+};
Suggestion importance[1-10]: 9

Why: Adding error handling for the handleSelectItem function is crucial to avoid unhandled promise rejections, which can lead to runtime issues. This suggestion is highly relevant and directly improves the robustness of the code.

9
Add error handling for nodeLoader

Add error handling for the nodeLoader function to ensure it gracefully handles cases
where the requested node type is not found or the loader fails.

packages/graph-editor/src/editor/actions/createNode.tsx [62]

-const Factory = await nodeLoader(nodeRequest.type);
+let Factory;
+try {
+    Factory = await nodeLoader(nodeRequest.type);
+    if (!Factory) throw new Error('Node type not found');
+} catch (error) {
+    console.error('Failed to load node type:', error);
+    return;
+}
Suggestion importance[1-10]: 9

Why: Adding error handling for nodeLoader ensures that the application gracefully handles cases where the requested node type is not found or the loader fails. This is critical given the PR's changes to node creation logic.

9
Validate factory existence in lookup

Add a check to ensure that the lookup function returns a valid factory before
calling deserialize to prevent potential runtime errors.

packages/graph-engine/src/graph/graph.ts [487-490]

 await Promise.all(
   serialized.nodes.map(async node => {
     const factory = await lookup(node.type);
+    if (!factory) {
+      throw new Error(`Factory not found for node type: ${node.type}`);
+    }
     return await factory.deserialize({
       serialized: node,
       graph: this,
     });
   }),
 );
Suggestion importance[1-10]: 9

Why: Ensuring that the lookup function returns a valid factory before proceeding prevents potential runtime errors. This is a critical improvement for the reliability of the deserialization process.

9
Validate system.nodeLoader output

Ensure that system.nodeLoader is properly defined and returns a valid factory before
using it to create a new node in the onEdgeDblClick callback.

packages/graph-editor/src/editor/graph.tsx [577-589]

 const onEdgeDblClick = useCallback(
   async (event, clickedEdge) => {
     event.stopPropagation();
 
     const position = reactFlowInstance?.screenToFlowPosition({
       x: event.clientX,
       y: event.clientY,
     });
     const PassthroughFactory = await system.nodeLoader(PASSTHROUGH);
+    if (!PassthroughFactory) {
+      throw new Error(`Node loader failed for type: ${PASSTHROUGH}`);
+    }
 
     const newNode = new PassthroughFactory({
       graph,
     });
   },
   [reactFlowInstance, system, graph],
 );
Suggestion importance[1-10]: 9

Why: Adding a validation for the output of system.nodeLoader ensures that the code handles cases where the loader fails, preventing runtime errors. This is an important enhancement for error handling and stability.

9
Add fallback for invalid edge types

Add a fallback mechanism for settings.edgeType in case it is undefined or contains
an invalid value to prevent runtime errors.

packages/graph-editor/src/components/flow/edges/edge.tsx [81-95]

-switch (settings.edgeType) {
+const validEdgeTypes = [EdgeType.bezier, EdgeType.simpleBezier, EdgeType.smoothStep, EdgeType.straight];
+const edgeType = validEdgeTypes.includes(settings.edgeType) ? settings.edgeType : EdgeType.bezier;
+switch (edgeType) {
   case EdgeType.bezier:
     edgeFn = getBetterBezierPath;
     break;
   case EdgeType.simpleBezier:
     edgeFn = getSimpleBezierPath;
     break;
   case EdgeType.smoothStep:
     edgeFn = getSmoothStepPath;
     break;
   case EdgeType.straight:
     edgeFn = getStraightPath;
     break;
-  default:
-    edgeFn = getBetterBezierPath;
 }
Suggestion importance[1-10]: 8

Why: Adding a fallback mechanism for settings.edgeType ensures that the code handles invalid or undefined values gracefully, preventing runtime errors. This is a significant improvement for code robustness.

8
Validate system.settings initialization

Ensure that the system.settings object is properly initialized and contains the
expected properties (setShowGrid, setSnapGrid, etc.) to avoid runtime errors when
accessing or modifying these properties.

packages/graph-editor/src/components/hotKeys/index.tsx [226]

-system.settings.setShowGrid(!system.settings.showGrid);
+if (system.settings && typeof system.settings.setShowGrid === 'function') {
+    system.settings.setShowGrid(!system.settings.showGrid);
+}
Suggestion importance[1-10]: 8

Why: The suggestion ensures that system.settings is properly initialized and its methods exist before calling them, which prevents potential runtime errors. This is particularly important as the PR introduces significant changes to how settings are managed.

8
Validate sys.controls.find result

Ensure that the sys.controls.find method returns a valid field object before
attempting to access its component property to avoid potential runtime errors.

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

 const field = sys.controls.find((x) => x.matcher(port, { readOnly }));
-const Component = field?.component as React.FC<IField>;
+if (!field || !field.component) {
+    console.warn('No matching control found for port:', port);
+    return null;
+}
+const Component = field.component as React.FC<IField>;
Suggestion importance[1-10]: 8

Why: The suggestion ensures that the result of sys.controls.find is validated before accessing its properties, preventing potential runtime errors. This is important as the PR modifies how controls are retrieved and used.

8
Validate system.panelItems.groups before using reduce

Ensure that the system.panelItems.groups object is properly validated before calling
reduce to avoid runtime errors if groups is undefined or not an array.

packages/graph-editor/src/editor/graph.tsx [133-140]

 const iconLookup = useMemo(() => {
-  return system.panelItems.groups.reduce((acc, group) => {
+  return Array.isArray(system.panelItems.groups) ? system.panelItems.groups.reduce((acc, group) => {
     group.items.forEach((item) => {
       acc[item.type] = item.icon || group.icon;
     });
     return acc;
-  }, {});
+  }, {}) : {};
 }, [system.panelItems.groups]);
Suggestion importance[1-10]: 8

Why: Adding a validation for system.panelItems.groups ensures that the code does not throw runtime errors if the property is undefined or not an array. This is a significant improvement in terms of robustness and error handling.

8
Add null check for innergraph

Add a null check for innergraph before attempting to deserialize it to avoid
potential runtime errors when it is undefined.

packages/graph-engine/src/nodes/generic/subgraph.ts [127-132]

 const serializedInner = (opts.serialized as SerializedSubgraphNode)
-  .innergraph;
+  ?.innergraph;
 
 const innergraph = serializedInner
   ? await new Graph().deserialize(serializedInner, opts.lookup)
   : undefined;
Suggestion importance[1-10]: 7

Why: Adding a null check for innergraph improves the code's robustness by preventing potential runtime errors when the property is undefined. While not critical, it is a valuable safeguard.

7
General
Validate settings.delayedUpdate before use

Validate the settings.delayedUpdate property to ensure it exists and is a boolean
before using it in conditional checks.

packages/graph-editor/src/components/controls/array.tsx [82-83]

-if (settings.delayedUpdate) {
+if (settings?.delayedUpdate === true) {
   return;
 }
Suggestion importance[1-10]: 7

Why: Validating settings.delayedUpdate ensures that the property exists and is of the expected type, preventing potential runtime errors. This is a good improvement for code safety and reliability.

7
Handle undefined NodeEntry props

Verify that the NodeEntry component correctly handles cases where icon or text might
be undefined or null to prevent rendering issues.

packages/graph-editor/src/components/panels/dropPanel/DragItem.tsx [64]

-<NodeEntry icon={icon} text={title} />
+<NodeEntry icon={icon || defaultIcon} text={title || 'Untitled'} />
Suggestion importance[1-10]: 7

Why: The suggestion adds a fallback for icon and text properties in the NodeEntry component, improving robustness and preventing rendering issues when these props are undefined or null. This aligns with the changes in the PR that modify how NodeEntry is used.

7
Validate specifics before rendering

Ensure that system.specifics[node.factory.type] is validated before rendering
SpecificWrapper to avoid potential null or undefined errors.

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

-<SpecificWrapper specifics={system.specifics} node={node} />
+{system.specifics[node.factory.type] && (
+  <SpecificWrapper specifics={system.specifics} node={node} />
+)}
Suggestion importance[1-10]: 6

Why: Validating system.specifics[node.factory.type] before rendering avoids potential null or undefined errors, improving the code's stability. While not critical, it is a useful safeguard.

6

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

Successfully merging this pull request may close these issues.

1 participant