early merge graph branch for general bug fixes#38
Conversation
Bumps the npm_and_yarn group with 1 update in the / directory: [form-data](https://github.com/form-data/form-data). Updates `form-data` from 4.0.2 to 4.0.4 - [Release notes](https://github.com/form-data/form-data/releases) - [Changelog](https://github.com/form-data/form-data/blob/master/CHANGELOG.md) - [Commits](form-data/form-data@v4.0.2...v4.0.4) --- updated-dependencies: - dependency-name: form-data dependency-version: 4.0.4 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a graph-based node editor system with internationalization improvements. The implementation includes a visual node editor with drag-and-drop functionality, edge connections between nodes, and type-safe node definitions for various operations.
- Adds complete graph editor infrastructure with node management, edge rendering, and execution engine
- Implements drag-and-drop canvas functionality with infinite scrolling
- Enhances internationalization with client-side error handling and fallback mechanisms
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles/node.css | Node styling with animations and drag states |
| src/lib/graph/* | Core graph functionality including node execution, edge rendering, and type compatibility |
| src/components/graph/* | React components for the graph editor UI |
| src/i18n/request.ts | Server-side i18n error handling |
| src/components/intl-error-handling-client-provider.tsx | Client-side i18n error handling |
| src/app/[locale]/layout.tsx | Integration of new i18n provider |
| src/app/[locale]/debug/graph/page.tsx | Debug page showcasing graph functionality |
| next.config.mjs | Configuration update for i18n |
| i18n/en.json | Translation keys for graph components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const formNodeIO = fromNode?.getAllIO().find(io => io.name === fromIO.nodeIOName); | ||
| const toNodeIO = toNode?.getAllIO().find(io => io.name === toIO.nodeIOName); | ||
|
|
||
| if (!formNodeIO || !toNodeIO) { |
There was a problem hiding this comment.
Variable reference 'formNodeIO' should be 'fromNodeIO' to match the corrected variable name.
| const formNodeIO = fromNode?.getAllIO().find(io => io.name === fromIO.nodeIOName); | |
| const toNodeIO = toNode?.getAllIO().find(io => io.name === toIO.nodeIOName); | |
| if (!formNodeIO || !toNodeIO) { | |
| const fromNodeIO = fromNode?.getAllIO().find(io => io.name === fromIO.nodeIOName); | |
| const toNodeIO = toNode?.getAllIO().find(io => io.name === toIO.nodeIOName); | |
| if (!fromNodeIO || !toNodeIO) { |
| const isFromIoOutput = fromNode.outputs.some(output => output.name === fromIO.nodeIOName); | ||
| const isToIoInput = toNode.inputs.some(input => input.name === toIO.nodeIOName); | ||
|
|
||
| if(isFromIoOutput !== isToIoInput) { // only input to outpot or output to input is valid |
There was a problem hiding this comment.
Comment contains typo 'outpot' which should be 'output'.
| if(isFromIoOutput !== isToIoInput) { // only input to outpot or output to input is valid | |
| if(isFromIoOutput !== isToIoInput) { // only input to output or output to input is valid |
| console.log({ | ||
| dragStartRef: dragStartRef.current, | ||
| scrollStartRef: scrollStartRef.current, | ||
| currentX: e.clientX, | ||
| currentY: e.clientY, | ||
| deltaX: e.clientX - dragStartRef.current.x, | ||
| deltaY: e.clientY - dragStartRef.current.y, | ||
| }); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production code.
| useEffect(() => { | ||
| console.log("onEndDraggingCanvas"); | ||
| return () => { | ||
| console.log("cleanup onEndDraggingCanvas"); | ||
| }; | ||
| }, [onEndDraggingCanvas]); | ||
| useEffect(() => { | ||
| console.log("setCurrentlyDraggingNode"); | ||
| return () => { | ||
| console.log("cleanup setCurrentlyDraggingNode"); | ||
| }; | ||
| }, [onEndDraggingCanvas]); | ||
| useEffect(() => { | ||
| console.log("onMouseMoveDragCanvas"); | ||
| return () => { | ||
| console.log("cleanup onMouseMoveDragCanvas"); | ||
| }; | ||
| }, [onEndDraggingCanvas]); |
There was a problem hiding this comment.
Multiple debug console.log statements and empty useEffect hooks should be removed from production code.
| } | ||
|
|
||
| areTypesCompatibleCache[cacheKey] = variant; | ||
| console.log(areTypesCompatibleCache); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production code.
| console.log(areTypesCompatibleCache); |
| graphRef.current?.addEventListener("transitionend", recalculateEdges); | ||
| graphRef.current?.addEventListener("scroll", recalculateEdges); | ||
| return () => { | ||
| graphRef.current?.removeEventListener("transitionend", recalculateEdges); | ||
| graphRef.current?.removeEventListener("scroll", recalculateEdges); |
There was a problem hiding this comment.
Event listeners are being removed using a stale reference to graphRef.current. Store the reference in a variable within the useEffect to ensure proper cleanup.
| graphRef.current?.addEventListener("transitionend", recalculateEdges); | |
| graphRef.current?.addEventListener("scroll", recalculateEdges); | |
| return () => { | |
| graphRef.current?.removeEventListener("transitionend", recalculateEdges); | |
| graphRef.current?.removeEventListener("scroll", recalculateEdges); | |
| const graphEl = graphRef.current; | |
| graphEl?.addEventListener("transitionend", recalculateEdges); | |
| graphEl?.addEventListener("scroll", recalculateEdges); | |
| return () => { | |
| graphEl?.removeEventListener("transitionend", recalculateEdges); | |
| graphEl?.removeEventListener("scroll", recalculateEdges); |
| e.dataTransfer.dropEffect = "move"; | ||
|
|
||
| const fromIoJSON = e.dataTransfer.getData("fromIO"); | ||
| if (fromIoJSON == undefined || fromIoJSON.length == 0) { |
There was a problem hiding this comment.
Use strict equality (===) instead of loose equality (==) for type safety.
| if (fromIoJSON == undefined || fromIoJSON.length == 0) { | |
| if (fromIoJSON === undefined || fromIoJSON === null || fromIoJSON.length === 0) { |
| <Node | ||
| key={nodeState.id} | ||
| nodeState={nodeState} | ||
| isBeingDragged={nodeState.id == currentlyDraggingNode?.nodeId} |
There was a problem hiding this comment.
Use strict equality (===) instead of loose equality (==) for type safety.
| isBeingDragged={nodeState.id == currentlyDraggingNode?.nodeId} | |
| isBeingDragged={nodeState.id === currentlyDraggingNode?.nodeId} |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.log({ | ||
| dragStartRef: dragStartRef.current, | ||
| scrollStartRef: scrollStartRef.current, | ||
| currentX: e.clientX, | ||
| currentY: e.clientY, | ||
| deltaX: e.clientX - dragStartRef.current.x, | ||
| deltaY: e.clientY - dragStartRef.current.y, | ||
| }); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production code. Consider using a proper logging solution or removing this debug output.
| console.log({ | |
| dragStartRef: dragStartRef.current, | |
| scrollStartRef: scrollStartRef.current, | |
| currentX: e.clientX, | |
| currentY: e.clientY, | |
| deltaX: e.clientX - dragStartRef.current.x, | |
| deltaY: e.clientY - dragStartRef.current.y, | |
| }); |
| const isFromIoOutput = fromNode.outputs.some(output => output.name === fromIO.nodeIOName); | ||
| const isToIoInput = toNode.inputs.some(input => input.name === toIO.nodeIOName); | ||
|
|
||
| if(isFromIoOutput !== isToIoInput) { // only input to output or output to input is valid |
There was a problem hiding this comment.
The logic is inverted. The condition should be if(isFromIoOutput === isToIoInput) because connecting output-to-output or input-to-input should be invalid, not output-to-input or input-to-output.
| if(isFromIoOutput !== isToIoInput) { // only input to output or output to input is valid | |
| if(isFromIoOutput === isToIoInput) { // only input to output or output to input is valid |
| e.dataTransfer.dropEffect = "move"; | ||
|
|
||
| const fromIoJSON = e.dataTransfer.getData("fromIO"); | ||
| if (fromIoJSON == undefined || fromIoJSON.length == 0) { |
There was a problem hiding this comment.
Use strict equality operators (=== and !==) instead of loose equality (== and !=) for better type safety and to avoid unexpected type coercion.
| if (fromIoJSON == undefined || fromIoJSON.length == 0) { | |
| if (fromIoJSON === undefined || fromIoJSON.length == 0) { |
| OutputKeys extends readonly string[] | ||
| >( | ||
| config: { | ||
| type: string; |
There was a problem hiding this comment.
The type parameter should be constrained to the valid node types ('input' | 'output' | 'operation') instead of accepting any string, to ensure type safety and prevent invalid node types.
| type: string; | |
| type: 'input' | 'output' | 'operation'; |
| <NextIntlClientProvider | ||
| locale={locale} | ||
| onError={(error) => { | ||
| if (error.code != "MISSING_MESSAGE") { |
There was a problem hiding this comment.
Use strict inequality operator (!==) instead of loose inequality (!=) for better type safety and to avoid unexpected type coercion.
| if (error.code != "MISSING_MESSAGE") { | |
| if (error.code !== "MISSING_MESSAGE") { |
No description provided.