[cleanup] Improve the array of dependencies of some React hooks

[sbegaudeau]

Signed-off-by: Stéphane Bégaudeau stephane.begaudeau@obeo.fr

Pull request template

General purpose

What is the main goal of this pull request?

• Bug fixes
• New features
• Documentation
• Cleanup
• Tests
• Build / releng

Project management

• Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
• Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
• Have the relevant issues been added to the pull request?
• Have the relevant labels been added to the issues? (area:, difficulty:, type:)
• Have the relevant issues been added to the same project and milestone as the pull request?
• Has the CHANGELOG.adoc been updated to reference the relevant issues?
• Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
• In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

• Does the title of the commit contributing the ADR start with [doc]?
• Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

• Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
• Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

• Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

• Variables have a proper type
• Functions’ arguments have a proper type
• Functions’ return type are specified
• Hooks are properly typed:
  • useMutation<DATA_TYPE, VARIABLE_TYPE>(…)
  • useQuery<DATA_TYPE, VARIABLE_TYPE>(…)
  • useSubscription<DATA_TYPE, VARIABLE_TYPE>(…)
  • useMachine<CONTEXT_TYPE, EVENTS_TYPE>(…)
  • useState<STATE_TYPE>(…)
• All components have a proper typing for their props
• No useless optional chaining with ?. (if the GraphQL API specifies that a field cannot be null, do not treat it has potentially null for example)
• Nullable values have a proper type (for example let diagram: Diagram | null = null;)

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

• Are all the event handlers tested?
• Are the event processor tested?
• Is the business code (services) tested?
• Are diagram layout changes tested?

Architecture

• Are data structure classes properly separated from behavioral classes?
• Are all the relevant fields final?
• Is any data structure mutable? If so, please write a comment indicating why
• Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

• Has the Kiwi TCMS test suite been updated with tests for this contribution?

[sbegaudeau]

Quick automatic review (please ignore this message)

Pull Request Status: 🟢 OPEN
CI Status: 🟡 PENDING - (Continuous integration/Build is currently pending)
Size: ✅ Manageable - (9 additions, 6 deletions across 3 files)

Recommendation: Ready for a thorough review! Although CI is still pending, the changes are surgical and focused on React lifecycle improvements.

---

Summary:
This pull request aims to improve the dependency arrays of several React hooks within the diagram renderer. It correctly adds missing dependencies to onKeyDown, onDelete, and onDirectEdit hooks, ensuring they don't suffer from stale closures. This is a good cleanup effort that aligns with our goal of maintaining a robust and predictable frontend.

🚨 Critical & Architectural Issues

1. Missing Dependencies in DiagramRenderer.tsx
   While you improved onKeyDown, the handleNodesChange callback and the main useEffect in DiagramRenderer.tsx still have significant missing dependencies. According to our react-frontend-reviewer rules, all dependencies MUST be correctly listed to prevent stale closures and inconsistent rendering.

handleNodesChange (around line 337):
It currently only lists [layoutOnBoundsChange, getNodes, getEdges, selectedElementsIds]. However, it uses:

• nodes
• filterReadOnlyChanges
• setNodes
• applyLastElementSelected
• transformBorderNodeChanges
• transformUndraggableListNodeChanges
• applyHelperLines
• transformResizeListNodeChanges
• applyMoveChange
• applyHandleChange
• applyResizeHandleChange

Suggested Fix:

1   const handleNodesChange: OnNodesChange<Node<NodeData>> = useCallback(
2     (changes: NodeChange<Node<NodeData>>[]) => {
3       // ... implementation
4     },
5     [
6       layoutOnBoundsChange,
7       getNodes,
8       getEdges,
9       selectedElementsIds,

10 nodes,
11 filterReadOnlyChanges,
12 setNodes,
13 applyLastElementSelected,
14 transformBorderNodeChanges,
15 transformUndraggableListNodeChanges,
16 applyHelperLines,
17 transformResizeListNodeChanges,
18 applyMoveChange,
19 applyHandleChange,
20 applyResizeHandleChange,
21 resetHelperLines,
22 ]
23 );

Main useEffect (around line 145):
It is missing nodeConverters, store, getNode, layout, synchronizeLayoutData, and readOnly.

---

2. Unstable Dependencies in useDiagramDelete
   In useDiagramDelete.tsx, you added deleteDiagramElements and showDeletionConfirmation to the dependency array. However, these functions are currently recreated on every render in their respective hooks (useDelete and useDeletionConfirmationDialog). This makes onDelete unstable, which in turn causes onKeyDown to change on every render of DiagramRenderer.

Suggested Fix:
You should wrap deleteDiagramElements in useCallback within packages/diagrams/frontend/sirius-components-diagrams/src/renderer/tools/useDelete.tsx:

1 // In useDelete.tsx
2 export const useDelete = (): UseDeleteValue => {
3   // ...
4   const deleteDiagramElements = useCallback((editingContextId: string, diagramId: string, nodeIds: string[], edgeIds: string[]) => {
5     const input: GQLDeleteFromDiagramInput = {
6       id: crypto.randomUUID(),
7       editingContextId,
8       representationId: diagramId,
9       nodeIds,

10 edgeIds,
11 };
12 rawDeleteFromDiagram({ variables: { input } });
13 }, [rawDeleteFromDiagram]);
14 // ...
15 };

⚠️ Standard & Stylistic Feedback

• Consistency: The use of GQL prefix for GraphQL types in useDelete.types.ts (e.g., GQLDeleteFromDiagramData) is excellent and follows our standards perfectly.
• Dependency Ordering: In useDiagramDelete.tsx, you've added many dependencies. It might be cleaner to keep them alphabetized or grouped (context values first, then functions), though this is a minor point.

✅ What Went Well

The contributor did an excellent job identifying the missing dependencies for the onKeyDown and direct edit/delete hooks. Specifically, ensuring that onDirectEdit and onDelete are included in onKeyDown is crucial for preventing bugs where the keyboard shortcuts might refer to outdated versions of these functions. The strict adherence to type naming conventions (DiagramRendererProps, UseDiagramDeleteValue) and file locations (.types.ts) is also highly appreciated.

[sbegaudeau]

Quick automatic review (please ignore this message)

Pull Request Status: 🟢 OPEN
CI Status: 🟡 PENDING - (Continuous integration/Build is currently pending)
Size: ✅ Manageable - (43 additions, 19 deletions across 4 files)

Summary:
This pull request has been updated to address previous review feedback regarding React hook dependency arrays. The contributor has stabilized the deleteDiagramElements callback using useCallback and significantly expanded the dependency list for handleNodesChange. However, a critical misunderstanding remains regarding which useEffect in DiagramRenderer.tsx requires the additional dependencies.

🚨 Critical & Architectural Issues

1. Incorrect Dependency Update in DiagramRenderer.tsx
   It appears that the dependencies suggested for the first useEffect (the one handling the main diagram refresh) were instead applied to the second useEffect (which handles post-tool selection).

According to our react-frontend-reviewer rules on Effect Dependencies, all dependencies MUST be correctly listed.

The first useEffect (starting around line 145) is still missing:

• consumePostToolSelection
• nodeConverters
• store
• getNode
• nodes
• edges
• layout
• readOnly
• synchronizeLayoutData

The second useEffect (starting around line 318) now has unused dependencies:

• It includes nodeConverters, layout, synchronizeLayoutData, and readOnly, which it does not use.
• Conversely, it is missing its actual dependencies: diagramRefreshedEventPayload, getNodes, getEdges, and setSelection.

Suggested Fix for DiagramRenderer.tsx:

1 // First useEffect (around line 145)
2 useEffect(() => {
3   // ... refresh logic ...
4 }, [
5   diagramRefreshedEventPayload,
6   diagramDescription,
7   setSelection,
8   consumePostToolSelection,
9   nodeConverters,

10 store,
11 getNode,
12 nodes,
13 edges,
14 layout,
15 readOnly,
16 synchronizeLayoutData
17 ]);
18
19 // Second useEffect (around line 318)
20 useEffect(() => {
21 if (toolSelections.has(diagramRefreshedEventPayload.id)) {
22 // ... selection logic ...
23 }
24 }, [
25 toolSelections,
26 diagramRefreshedEventPayload,
27 store,
28 getNodes,
29 getEdges,
30 getNode,
31 setSelection
32 ]);

⚠️ Standard & Stylistic Feedback

• Dependency Ordering: The handleNodesChange dependency array is now very large. While correct, consider if some of these utilities (like applyNodeChanges if it were a hook result, though here it seems to be an import) could be simplified. However, for now, the completeness is the priority.
• Copyright: Thank you for updating the copyright year in useDelete.tsx!

✅ What Went Well

• Responsiveness: Great job quickly applying the feedback regarding useCallback in useDelete.tsx. This successfully prevents the "dependency chain reaction" that was causing onKeyDown to be recreated on every render.
• Thoroughness: The update to handleNodesChange shows a strong commitment to React best practices by listing every single function used within the callback.

[sbegaudeau]

Quick automatic review (please ignore this message)

Pull Request Status: 🟢 OPEN
CI Status: 🟡 PENDING - (Continuous integration/Build (push) is still in progress)
Size: ✅ Manageable - (43 additions, 19 deletions across 4 files)

Recommendation: Almost there! The PR is now structurally sound and correctly implements the previous architectural feedback. One final minor fix is needed to achieve full compliance with React hook rules.

---

Summary:
This pull request has been significantly improved through several iterations. It now correctly stabilizes the deleteDiagramElements callback, expanded the dependency arrays to cover all used variables in handleNodesChange, and correctly mapped the dependencies to their respective useEffect blocks in DiagramRenderer.tsx. This successfully mitigates the risks of stale closures and unnecessary re-renders in the diagram renderer.

🚨 Critical & Architectural Issues

1. Missing Final Dependencies in DiagramRenderer.tsx
   While you have correctly re-distributed the dependencies to the right effects, the second useEffect (handling post-tool selection) is still missing a few required references.

Missing in Second useEffect (around line 338):

• store: Used via store.getState() to retrieve nodeLookup and edgeLookup.
• getNode: Used inside the map callback to check isLastNodeSelected.
• getNodes & getEdges: Although these were added in this commit, they are correct.

Suggested Fix:

1   useEffect(() => {
2     if (toolSelections.has(diagramRefreshedEventPayload.id)) {
3       // ... selection logic ...
4     }
5   }, [
6     toolSelections,
7     getNodes,
8     getEdges,
9     setSelection,

10 diagramRefreshedEventPayload,
11 store, // Add this
12 getNode // Add this
13 ]);

⚠️ Standard & Stylistic Feedback

• Stability: The addition of useCallback for deleteDiagramElements in useDelete.tsx is exactly what was needed to stop the chain reaction of hook updates. Excellent fix.
• Naming: All new dependencies follow the correct camelCase naming convention as used throughout the component.

✅ What Went Well

The contributor demonstrated great persistence and technical accuracy in resolving the complex dependency chain in DiagramRenderer.tsx. Specifically, the manual expansion of the handleNodesChange dependency array is very thorough and shows a deep understanding of the component's internal state management. The stabilization of useDelete is also a standout improvement for the overall performance of the diagram editor.

[sbegaudeau]

Quick automatic review (please ignore this message)

Pull Request Status: 🟢 OPEN
CI Status: 🟡 PENDING - (Continuous integration/Build (push) is still in progress)
Size: ✅ Manageable - (43 additions, 19 deletions across 4 files)

Recommendation: Ready for approval! All architectural and React-specific issues identified in previous review rounds have been correctly and thoroughly resolved.

---

Summary:
This pull request correctly implements all suggested improvements for React hook dependency management across the diagram renderer. By stabilizing critical callbacks with useCallback and precisely mapping every used variable to its
corresponding effect or callback dependency array, the contributor has significantly improved the technical robustness and reliability of the diagram lifecycle.

🚨 Critical & Architectural Issues

None. All critical issues from previous rounds have been addressed:

• Callback Stability: deleteDiagramElements in useDelete.tsx is now properly memoized.
• Complete Dependencies: handleNodesChange and both useEffect blocks in DiagramRenderer.tsx now correctly list all their dependencies, including store, getNode, and state variables.
• Correct Effect Mapping: The dependencies are now correctly applied to the specific useEffect that actually uses them.

⚠️ Standard & Stylistic Feedback

• Dependency Array Completeness: The dependency arrays are now fully compliant with the exhaustive-deps rule.
• Maintainability: By explicitly listing dependencies, the code is now more predictable and easier to debug when troubleshooting re-renders or stale closures.

✅ What Went Well

The contributor's persistence in refining the dependency logic across multiple iterations is commendable. The resulting code is a model example of how to handle complex React hooks in a performance-sensitive context like a diagramming canvas. Specifically, the cleanup in DiagramRenderer.tsx transforms a potentially fragile component into a much more stable one.