Add the functions for creating, deleting and editing nodes #87
Conversation
WalkthroughAdds server- and client-side mutation APIs (create/edit operations and client callbacks), a full event system for mutations, frontend runtime methods to mutate and notify the server, an interactive edit demo with styles, and bumps project version to 5.3.0-SNAPSHOT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
New API includes methods to add siblings, add children, add parent, update and remove nodes. Close #78
mlopezFC
left a comment
mlopezFC
left a comment
There was a problem hiding this comment.
Maybe some demo related thing, but if I edit a node, it will delete its children, is that an expected behavior?
@mlopezFC No, if you delete a node it will delete its children but not when editing. Maybe I did something wrong in the demo. Let me check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java(6 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/event/ChildrenAddedEvent.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/event/NodeUpdatedEvent.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/event/NodesRemovedEvent.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/event/ParentAddedEvent.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/orgchart/event/SiblingsAddedEvent.java(1 hunks)src/main/resources/META-INF/frontend/fc-orgchart.js(2 hunks)src/test/java/com/flowingcode/vaadin/addons/orgchart/EditChartDemo.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/orgchart/OrgchartDemoView.java(1 hunks)src/test/resources/META-INF/resources/frontend/styles/orgchart/edit-chart-demo-styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/com/flowingcode/vaadin/addons/orgchart/event/ChildrenAddedEvent.java
- src/main/java/com/flowingcode/vaadin/addons/orgchart/event/NodesRemovedEvent.java
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/orgchart/event/NodeUpdatedEvent.java
- src/test/resources/META-INF/resources/frontend/styles/orgchart/edit-chart-demo-styles.css
- src/main/resources/META-INF/frontend/fc-orgchart.js
- src/main/java/com/flowingcode/vaadin/addons/orgchart/event/SiblingsAddedEvent.java
- src/test/java/com/flowingcode/vaadin/addons/orgchart/EditChartDemo.java
🔇 Additional comments (15)
src/test/java/com/flowingcode/vaadin/addons/orgchart/OrgchartDemoView.java (1)
44-44: LGTM! Demo registration follows the established pattern.The new
EditChartDemois properly registered in the demo view, maintaining consistency with other demo registrations.src/main/java/com/flowingcode/vaadin/addons/orgchart/event/ParentAddedEvent.java (1)
26-54: LGTM! Well-designed event class following Vaadin patterns.The event class is properly structured with:
- Correct inheritance from
ComponentEvent<OrgChart>- Appropriate constructor parameters including
fromClientflag- Clear JavaDoc documentation
- Consistent naming and access modifiers
The implementation follows established Vaadin event patterns and integrates well with the broader event-driven architecture.
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (13)
26-30: LGTM! Event imports are properly organized.The new event class imports are correctly added and follow the package structure.
41-45: LGTM! Required utility imports added.The additional imports (
JsonArray,ArrayList,Collections,Objects) are necessary for the new mutation functionality.
84-85: LGTM! Initialization method updated correctly.The chart initialization properly passes the required parameters to the client-side JavaScript.
216-217: LGTM! Stream-based filtering implemented correctly.The drag-and-drop logic properly filters out the dragged item from the old parent's children using a clean stream-based approach.
347-360: LGTM! Recursive parent finding algorithm is correct.The
findParentmethod implements a proper recursive search algorithm to locate the parent of a given node ID. The null checks and base cases are handled correctly.
368-374: LGTM! JSON array conversion is implemented correctly.The utility method properly converts JsonArray to List with appropriate type casting.
405-424: LGTM! Sibling addition logic is well-implemented.The method properly:
- Validates that siblings cannot be added to the root node
- Updates the internal data structure
- Synchronizes with the client-side visualization
- Includes comprehensive JavaDoc documentation
434-447: LGTM! Client callback properly handles sibling addition events.The method correctly:
- Converts JsonArray to Integer list
- Maps IDs to actual OrgChartItem objects
- Filters out null values
- Fires the appropriate event
502-516: LGTM! Children addition callback is properly implemented.The client callback correctly handles the conversion and event firing for children addition events.
552-569: LGTM! Node removal logic is comprehensive.The method properly:
- Finds and clears the target node's children
- Removes the node from its parent's children list
- Updates the client-side visualization
- Handles the case where the parent might not exist
606-618: LGTM! Parent addition updates both data and visualization correctly.The method properly:
- Sets the current root as a child of the new parent
- Updates the internal root reference
- Synchronizes with the client-side
666-689: LGTM! Node update method provides comprehensive data merging.The method correctly:
- Updates only non-null properties from the new data
- Handles both simple properties and custom data
- Synchronizes with client-side visualization
- Includes proper null checks
691-717: LGTM! Node update callback and event firing are properly implemented.The client callback and event firing methods follow the established pattern correctly.
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java
Outdated
Show resolved
Hide resolved
It's something I missed. Will fix it. |
Fixed. @mlopezFC |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (3)
456-458: Listener returns Registration now — resolved.Matches the convention of other listener registrations.
376-391: Ensure children list is modifiable and handle null itemsToAdd.If parentNode.getChildren() returns an unmodifiable list (e.g., Collections.emptyList or singletonList), addAll will throw. Also guard against null/empty itemsToAdd.
Apply this diff:
- private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { - if (parentNode != null) { - List<OrgChartItem> currentChildren = parentNode.getChildren(); - if (currentChildren == null) { - currentChildren = new ArrayList<>(); - } - currentChildren.addAll(itemsToAdd); - parentNode.setChildren(currentChildren); - } - } + private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { + if (parentNode == null || itemsToAdd == null || itemsToAdd.isEmpty()) { + return; + } + List<OrgChartItem> currentChildren = parentNode.getChildren(); + List<OrgChartItem> modifiableChildren = (currentChildren == null) + ? new ArrayList<>() + : new ArrayList<>(currentChildren); + modifiableChildren.addAll(itemsToAdd); + parentNode.setChildren(modifiableChildren); + }Resolved JavaDoc formatting issue here from a prior review. Thanks for fixing it.
483-500: Null-safety for children addition (current NPE risk and post-append logic).
- targetNode.getChildren().isEmpty() can NPE when children == null.
- You compute currentChildrenEmpty before verifying targetNode != null.
Apply this diff:
public void addChildren(Integer nodeId, List<OrgChartItem> children) { // Update the internal data structure - OrgChartItem targetNode = getById(nodeId, orgChartItem); - boolean currentChildrenEmpty = targetNode.getChildren().isEmpty(); - if (targetNode != null) { - // Add new children while preserving existing ones - appendItemsToParent(targetNode, children); - } + OrgChartItem targetNode = getById(nodeId, orgChartItem); + if (targetNode == null) { + throw new IllegalArgumentException("Node not found: " + nodeId); + } + boolean currentChildrenEmpty = + targetNode.getChildren() == null || targetNode.getChildren().isEmpty(); + // Add new children while preserving existing ones + appendItemsToParent(targetNode, children); // Update the visual representation String itemsJson = convertToJsonObj(children); if (currentChildrenEmpty) { this.getElement().executeJs("this.addChildren($0, $1)", nodeId, itemsJson); } else { this.getElement().executeJs("this.addSiblings($0, $1)", targetNode.getChildren().get(0).getId(), itemsJson); } }
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (2)
84-85: Initialization and deprecation notice are fine.
- Moving initialization to onAttach and deprecating initializeChart is sound.
- Optional: Consider adding since/forRemoval metadata to @deprecated in a future cleanup.
Also applies to: 89-93
248-253: Minor JavaDoc typo in this block."recieves" should be "receives".
Outside this selected range, update the earlier line in the same JavaDoc as follows:
* functionBody} is the body of a javascript function that receives one parameter (the JSON
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java(6 hunks)src/main/resources/META-INF/frontend/fc-orgchart.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/META-INF/frontend/fc-orgchart.js
🔇 Additional comments (5)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (5)
26-30: New event and utility imports look consistent.Imports for event types, JsonArray, and collections/utilities are appropriate and scoped correctly.
Also applies to: 41-46
51-53: Docs link addition looks good.Including a direct link to the upstream library is helpful.
338-360: Parent-finding helper looks good.Depth-first search with null-safe child traversal is correct; returning null for root requests is expected.
368-374: ID array conversion OK.Conversion from JsonArray to List is straightforward and adequate for typical ID ranges.
467-470: Event plumbing (fireXxx/addXxxListener) looks consistent.The event classes are wired correctly; listener registration returns Registration; firing methods pass along the source and fromClient flags as expected.
Also applies to: 525-528, 537-540, 571-575, 583-585, 593-595, 641-643, 651-653, 705-707, 715-717
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (9)
433-447: Null-guard target node on client callback; fix comment wording
- If the target node no longer exists on the server (desync), don’t fire the event.
- Comment says “parent” and “child items” but this method deals with siblings of a target item.
@ClientCallable private void onSiblingsAdded(String nodeId, JsonArray siblingIds) { // Find the node where siblings were added - OrgChartItem targetItem = getById(Integer.valueOf(nodeId), orgChartItem); + OrgChartItem targetItem = getById(Integer.valueOf(nodeId), orgChartItem); + if (targetItem == null) { + return; + } @@ - // Fire the event with the parent and the fully populated list of child items + // Fire the event with the target item and the fully populated list of new sibling items fireSiblingsAddedEvent(targetItem, newSiblingItems, true); }
503-517: Guard against missing parent on client callbackAvoid firing events with a null parent if client/server are out of sync.
@ClientCallable private void onChildrenAdded(String nodeId, JsonArray childIds) { // Find the parent node where children were added - OrgChartItem parentItem = getById(Integer.valueOf(nodeId), orgChartItem); + OrgChartItem parentItem = getById(Integer.valueOf(nodeId), orgChartItem); + if (parentItem == null) { + return; + } @@ // Fire the event with the parent and the fully populated list of child items fireChildrenAddedEvent(parentItem, newChildItems, true); }
598-606: JavaDoc @param name mismatch with method signature@param says parentItem but method arg is newParentItem. Keep them aligned.
- * @param parentItem the new root item of the chart + * @param newParentItem the new root item of the chart
215-219: Guard against NPE in drag-and-drop old parent update; validate inputsoldParentItem.getChildren() may be null (NPE on stream). Also, if any of draggedItem, oldParentItem, or newParentItem are null the method should bail early to avoid server/client desync.
Apply this diff:
- // update old parent (remove item) - List<OrgChartItem> oldParentUpdated = oldParentItem.getChildren().stream() - .filter(i -> !draggedNodeId.equals(i.getId())).collect(Collectors.toList()); - oldParentItem.setChildren(oldParentUpdated); + // update old parent (remove item) + if (draggedItem == null || oldParentItem == null || newParentItem == null) { + return; + } + List<OrgChartItem> oldChildren = oldParentItem.getChildren(); + List<OrgChartItem> oldParentUpdated = + oldChildren == null + ? new ArrayList<>() + : oldChildren.stream() + .filter(i -> !draggedNodeId.equals(i.getId())) + .collect(Collectors.toCollection(ArrayList::new)); + oldParentItem.setChildren(oldParentUpdated);
405-424: Validate inputs and avoid client call if server update didn’t happen
- Add guards for null/empty siblings and null nodeId.
- Fail-fast if target node or parent can’t be found.
- Only call client-side addSiblings when the server-side state was updated to prevent desync.
- public void addSiblings(Integer nodeId, List<OrgChartItem> siblings) { - // First check if selected node is not the root node - if (nodeId.equals(this.orgChartItem.getId())) { - throw new IllegalArgumentException("Cannot add siblings to the root node."); - } - // Update the internal data structure - OrgChartItem targetNode = getById(nodeId, orgChartItem); - if (targetNode != null) { - // Find parent of the target node - OrgChartItem parentNode = findParent(nodeId, orgChartItem); - if (parentNode != null) { - // Update parent's children list with the new siblings - appendItemsToParent(parentNode, siblings); - } - } - - // Update the visual representation by calling the client-side method addSiblings - String siblingsJson = convertToJsonObj(siblings); - this.getElement().executeJs("this.addSiblings($0, $1)", nodeId, siblingsJson); - } + public void addSiblings(Integer nodeId, List<OrgChartItem> siblings) { + Objects.requireNonNull(nodeId, "nodeId must not be null"); + if (siblings == null || siblings.isEmpty()) { + return; + } + if (nodeId.equals(this.orgChartItem.getId())) { + throw new IllegalArgumentException("Cannot add siblings to the root node."); + } + OrgChartItem targetNode = getById(nodeId, orgChartItem); + if (targetNode == null) { + throw new IllegalArgumentException("Node not found: " + nodeId); + } + OrgChartItem parentNode = findParent(nodeId, orgChartItem); + if (parentNode == null) { + throw new IllegalStateException("Parent not found for node: " + nodeId); + } + appendItemsToParent(parentNode, siblings); + String siblingsJson = convertToJsonObj(siblings); + this.getElement().executeJs("this.addSiblings($0, $1)", nodeId, siblingsJson); + }
483-501: Null-dereference bug: targetNode accessed before null-check; also avoid client call if server update didn’t happencurrentChildrenEmpty is computed via targetNode.getChildren() before verifying targetNode != null, which can throw an NPE. Also add input validation like in addSiblings.
- public void addChildren(Integer nodeId, List<OrgChartItem> children) { - // Update the internal data structure - OrgChartItem targetNode = getById(nodeId, orgChartItem); - boolean currentChildrenEmpty = - targetNode.getChildren() == null || targetNode.getChildren().isEmpty(); - if (targetNode != null) { - // Add new children while preserving existing ones - appendItemsToParent(targetNode, children); - } - - // Update the visual representation - String itemsJson = convertToJsonObj(children); - if (currentChildrenEmpty) { - this.getElement().executeJs("this.addChildren($0, $1)", nodeId, itemsJson); - } else { - this.getElement().executeJs("this.addSiblings($0, $1)", - targetNode.getChildren().get(0).getId(), itemsJson); - } - } + public void addChildren(Integer nodeId, List<OrgChartItem> children) { + Objects.requireNonNull(nodeId, "nodeId must not be null"); + if (children == null || children.isEmpty()) { + return; + } + // Update the internal data structure + OrgChartItem targetNode = getById(nodeId, orgChartItem); + if (targetNode == null) { + throw new IllegalArgumentException("Node not found: " + nodeId); + } + boolean currentChildrenEmpty = + targetNode.getChildren() == null || targetNode.getChildren().isEmpty(); + // Add new children while preserving existing ones + appendItemsToParent(targetNode, children); + + // Update the visual representation + String itemsJson = convertToJsonObj(children); + if (currentChildrenEmpty) { + this.getElement().executeJs("this.addChildren($0, $1)", nodeId, itemsJson); + } else { + this.getElement().executeJs("this.addSiblings($0, $1)", + targetNode.getChildren().get(0).getId(), itemsJson); + } + }
553-570: Use modifiable lists and handle root removal to keep server and client in sync
- Collections.emptyList() is unmodifiable; future additions will fail.
- parentNode.getChildren() can be null; guard it.
- If removing the root, clear orgChartItem so server state reflects the removal.
public void removeNodes(Integer nodeId) { // Find the node set for removal OrgChartItem nodeToRemove = getById(nodeId, orgChartItem); if (nodeToRemove != null) { // Clear the removed node's children - nodeToRemove.setChildren(Collections.emptyList()); + nodeToRemove.setChildren(new ArrayList<>()); // Find parent and remove node from its children OrgChartItem parentNode = findParent(nodeId, orgChartItem); if (parentNode != null) { - List<OrgChartItem> currentChildren = parentNode.getChildren(); - currentChildren.removeIf(child -> nodeId.equals(child.getId())); - parentNode.setChildren(currentChildren); + List<OrgChartItem> currentChildren = parentNode.getChildren(); + List<OrgChartItem> modifiableChildren = + currentChildren == null ? new ArrayList<>() : new ArrayList<>(currentChildren); + modifiableChildren.removeIf(child -> nodeId.equals(child.getId())); + parentNode.setChildren(modifiableChildren); + } else if (this.orgChartItem != null && nodeId.equals(this.orgChartItem.getId())) { + // Removing root: clear internal model + this.orgChartItem = null; } // Update the visual representation this.getElement().executeJs("this.removeNodes($0)", nodeId); } }
607-619: Avoid using singletonList for root’s children (unmodifiable); keep server state mutableUse a modifiable list to avoid UnsupportedOperationException on subsequent modifications.
public void addParent(OrgChartItem newParentItem) { // Update the internal data structure if (this.orgChartItem != null) { // Set the old root as the only child of the new parent node - newParentItem.setChildren(Collections.singletonList(this.orgChartItem)); + newParentItem.setChildren(new ArrayList<>(Collections.singletonList(this.orgChartItem))); } // Update the chart's root to point to the new parent this.orgChartItem = newParentItem; // Update the visual representation by calling the client-side method addParent String parentJson = convertToJsonObj(newParentItem); this.getElement().executeJs("this.addParent($0)", parentJson); }
667-690: Pass nodeId as number to JS (not String); consider partial-update semantics for booleans
- Standardize with other JS calls by passing nodeId as a number to avoid type inconsistencies client-side.
- Optional: with primitive boolean you can’t express “no change”; if partial updates are desired, consider evolving hybrid to a boxed Boolean or apply conditional update only when explicitly intended.
- this.getElement().executeJs("this.updateNode($0, $1)", nodeId.toString(), newDataJson); + this.getElement().executeJs("this.updateNode($0, $1)", nodeId, newDataJson);Optional conditional hybrid update (if you want to minimize unintended overwrites):
if (nodeToUpdate.isHybrid() != newDataItem.isHybrid()) { nodeToUpdate.setHybrid(newDataItem.isHybrid()); }
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (2)
338-360: Handle null root defensively in findParentMinor: add a fast null guard for root to avoid accidental NPEs if invoked with a null root (e.g., after root removal) and short-circuit earlier.
- private OrgChartItem findParent(Integer childId, OrgChartItem root) { - if (root.getChildren() != null) { + private OrgChartItem findParent(Integer childId, OrgChartItem root) { + if (root == null) { + return null; + } + if (root.getChildren() != null) { for (OrgChartItem child : root.getChildren()) { if (childId.equals(child.getId())) { return root; } OrgChartItem parent = findParent(childId, child); if (parent != null) { return parent; } } } return null; }
382-391: Guard against null/empty itemsToAdd and return earlyAvoids unnecessary list creation and setter calls.
- private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { - if (parentNode != null) { + private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { + if (parentNode != null && itemsToAdd != null && !itemsToAdd.isEmpty()) { List<OrgChartItem> currentChildren = parentNode.getChildren(); if (currentChildren == null) { currentChildren = new ArrayList<>(); } currentChildren.addAll(itemsToAdd); parentNode.setChildren(currentChildren); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java(6 hunks)
🔇 Additional comments (7)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (7)
26-30: Imports look consistent with new API surfaceNew event types and JsonArray import align with added functionality.
Also applies to: 41-46
84-85: Initialize with explicit parameters — verify expected types in frontendYou’re passing a pretty-printed JSON string for state and a plain String for value and id. Ensure fc-orgchart.js’ initializeOrgChart expects a JSON string (not an object). If it expects an object, parse or send the object directly to avoid double-serialization issues.
Would you like me to scan the frontend file to confirm the expected parameter types and suggest adjustments?
88-91: Good deprecation notice and pointerClear guidance to use onAttach improves API clarity.
368-374: Helper conversion is clear and robustStraightforward conversion from JsonArray to integer list; fits the event plumbing.
456-458: Listener registration now returns Registration — goodConsistency with other listener APIs enables proper listener removal.
627-633: LGTM — client callback for parent additionResolves new parent from server model and fires event conditionally.
692-698: LGTM — node updated callbackLooks correct with null guard and event firing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (2)
405-424: Prevent server-client desync: validate inputs and only call client when server state is updatedCurrently, if the node or its parent cannot be found, the method still executes the client call, desynchronizing server and client state. Also, siblings may be null/empty.
public void addSiblings(Integer nodeId, List<OrgChartItem> siblings) { - // First check if selected node is not the root node + Objects.requireNonNull(nodeId, "nodeId must not be null"); + if (siblings == null || siblings.isEmpty()) { + return; + } + if (this.orgChartItem == null) { + throw new IllegalStateException("Chart is empty; cannot add siblings"); + } + // First check if selected node is not the root node if (nodeId.equals(this.orgChartItem.getId())) { throw new IllegalArgumentException("Cannot add siblings to the root node."); } // Update the internal data structure - OrgChartItem targetNode = getById(nodeId, orgChartItem); - if (targetNode != null) { - // Find parent of the target node - OrgChartItem parentNode = findParent(nodeId, orgChartItem); - if (parentNode != null) { - // Update parent's children list with the new siblings - appendItemsToParent(parentNode, siblings); - } - } + OrgChartItem targetNode = getById(nodeId, orgChartItem); + if (targetNode == null) { + throw new IllegalArgumentException("Node not found: " + nodeId); + } + // Find parent of the target node + OrgChartItem parentNode = findParent(nodeId, orgChartItem); + if (parentNode == null) { + throw new IllegalStateException("Parent not found for node: " + nodeId); + } + // Update parent's children list with the new siblings + appendItemsToParent(parentNode, siblings); // Update the visual representation by calling the client-side method addSiblings String siblingsJson = convertToJsonObj(siblings); this.getElement().executeJs("this.addSiblings($0, $1)", nodeId, siblingsJson); }
486-504: Fix NPE and avoid desync in addChildren; validate inputs and compute hadChildren after null-check
- targetNode.getChildren() is dereferenced before checking targetNode for null → NPE.
- If the node doesn't exist, the method still calls the client API, causing server-client desync.
- Also add guards for null/empty children and empty chart.
- public void addChildren(Integer nodeId, List<OrgChartItem> children) { - // Update the internal data structure - OrgChartItem targetNode = getById(nodeId, orgChartItem); - boolean currentChildrenEmpty = - targetNode.getChildren() == null || targetNode.getChildren().isEmpty(); - if (targetNode != null) { - // Add new children while preserving existing ones - appendItemsToParent(targetNode, children); - } - - // Update the visual representation - String itemsJson = convertToJsonObj(children); - if (currentChildrenEmpty) { - this.getElement().executeJs("this.addChildren($0, $1)", nodeId, itemsJson); - } else { - this.getElement().executeJs("this.addSiblings($0, $1)", - targetNode.getChildren().get(0).getId(), itemsJson); - } - } + public void addChildren(Integer nodeId, List<OrgChartItem> children) { + Objects.requireNonNull(nodeId, "nodeId must not be null"); + if (children == null || children.isEmpty()) { + return; + } + if (this.orgChartItem == null) { + throw new IllegalStateException("Chart is empty; cannot add children"); + } + // Update the internal data structure + OrgChartItem targetNode = getById(nodeId, orgChartItem); + if (targetNode == null) { + throw new IllegalArgumentException("Node not found: " + nodeId); + } + boolean hadChildren = targetNode.getChildren() != null && !targetNode.getChildren().isEmpty(); + Integer anchorId = hadChildren ? targetNode.getChildren().get(0).getId() : null; + // Add new children while preserving existing ones + appendItemsToParent(targetNode, children); + + // Update the visual representation + String itemsJson = convertToJsonObj(children); + if (!hadChildren) { + this.getElement().executeJs("this.addChildren($0, $1)", nodeId, itemsJson); + } else { + this.getElement().executeJs("this.addSiblings($0, $1)", anchorId, itemsJson); + } + }
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (2)
347-360: Add a null guard to findParent for robustnessMinor improvement: return early when root is null to avoid potential NPEs if invoked after the chart becomes empty (e.g., after root removal).
private OrgChartItem findParent(Integer childId, OrgChartItem root) { + if (root == null) { + return null; + } if (root.getChildren() != null) { for (OrgChartItem child : root.getChildren()) { if (childId.equals(child.getId())) { return root; } OrgChartItem parent = findParent(childId, child); if (parent != null) { return parent; } } } return null; }
382-391: Avoid NPE/UOE by guarding inputs and copying the children list before mutation
- itemsToAdd can be null, which would NPE on addAll.
- If getChildren() returns an unmodifiable list, addAll will throw. Copy before mutation to be safe.
- private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { - if (parentNode != null) { - List<OrgChartItem> currentChildren = parentNode.getChildren(); - if (currentChildren == null) { - currentChildren = new ArrayList<>(); - } - currentChildren.addAll(itemsToAdd); - parentNode.setChildren(currentChildren); - } - } + private void appendItemsToParent(OrgChartItem parentNode, List<OrgChartItem> itemsToAdd) { + if (parentNode == null || itemsToAdd == null || itemsToAdd.isEmpty()) { + return; + } + List<OrgChartItem> base = parentNode.getChildren(); + List<OrgChartItem> updated = base == null ? new ArrayList<>() : new ArrayList<>(base); + updated.addAll(itemsToAdd); + parentNode.setChildren(updated); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T20:02:58.053Z
Learnt from: paodb
PR: FlowingCode/OrgChartAddon#87
File: src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java:0-0
Timestamp: 2025-08-12T20:02:58.053Z
Learning: In FlowingCode OrgChart addon, using Collections.emptyList() in removeNodes() method is safe because OrgChartItem.setChildren() wraps any provided list in a new ArrayList, preventing issues with unmodifiable collections.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java
📚 Learning: 2025-08-12T19:49:33.136Z
Learnt from: paodb
PR: FlowingCode/OrgChartAddon#87
File: src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java:0-0
Timestamp: 2025-08-12T19:49:33.136Z
Learning: In the FlowingCode OrgChart addon, the OrgChartItem.setChildren() method always wraps the provided list in a new ArrayList, making it safe to pass immutable lists like Collections.singletonList() to it.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java
🔇 Additional comments (6)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (6)
84-86: Initialization call looks consistent with client APIPassing the serialized state, its value, and the element id aligns with typical Vaadin executeJs patterns. No issues spotted here.
215-219: Formatting-only change; ignoring pre-existing logicThis section appears to be only reflowed; not functionally changed by this PR. Skipping to keep this review focused on new functionality.
433-450: Good null-guard in client callbackBailing out when targetItem is null avoids firing misleading events on desynced state. Looks solid.
506-523: Client callback null-guard is correctThe early return when parentItem is null prevents misleading events and NPEs. Good defensive coding.
559-581: Collections.emptyList() usage is safe here; root clearing is handledUsing Collections.emptyList() is fine because OrgChartItem.setChildren() wraps the provided list in a new ArrayList internally, ensuring mutability thereafter. Clearing orgChartItem when removing the root keeps server state consistent with the client.
Note: This aligns with the retrieved learning that setChildren() always wraps in a new ArrayList, making immutable inputs safe.
618-630: LGTM; singleton list is safe due to setChildren implementationSetting the old root as the only child of the new parent with Collections.singletonList(...) is safe since setChildren() wraps it into a modifiable ArrayList internally. The server root pointer and client UI are updated consistently.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (1)
686-712: Consider input validation for updateNode method.The method correctly handles partial updates by checking for null values, but lacks input validation that other methods have. Consider adding null checks for
nodeIdandnewDataItemparameters for consistency.public void updateNode(Integer nodeId, OrgChartItem newDataItem) { + Objects.requireNonNull(nodeId, "nodeId must not be null"); + Objects.requireNonNull(newDataItem, "newDataItem must not be null"); OrgChartItem nodeToUpdate = getById(nodeId, this.orgChartItem);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T20:02:58.053Z
Learnt from: paodb
PR: FlowingCode/OrgChartAddon#87
File: src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java:0-0
Timestamp: 2025-08-12T20:02:58.053Z
Learning: In FlowingCode OrgChart addon, using Collections.emptyList() in removeNodes() method is safe because OrgChartItem.setChildren() wraps any provided list in a new ArrayList, preventing issues with unmodifiable collections.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java
📚 Learning: 2025-08-12T19:49:33.136Z
Learnt from: paodb
PR: FlowingCode/OrgChartAddon#87
File: src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java:0-0
Timestamp: 2025-08-12T19:49:33.136Z
Learning: In the FlowingCode OrgChart addon, the OrgChartItem.setChildren() method always wraps the provided list in a new ArrayList, making it safe to pass immutable lists like Collections.singletonList() to it.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java
🔇 Additional comments (24)
src/main/java/com/flowingcode/vaadin/addons/orgchart/OrgChart.java (24)
26-30: LGTM! Event imports properly support the new chart editing functionality.The imports for the new event classes are correctly added to support the chart editing capabilities.
41-46: LGTM! Required imports for JSON handling and utilities.The new imports for
JsonArray,ArrayList,Collections, andObjectsare appropriate for the chart editing functionality.
347-360: LGTM! Recursive parent finding implementation is correct.The
findParentmethod correctly traverses the tree structure to locate the parent of a given node. The null checks and recursive logic are well implemented.
368-374: LGTM! JSON array conversion utility is well implemented.The
convertJsonArrayToIntegerListmethod safely converts JsonArray elements to integers using the appropriategetNumbermethod.
382-391: LGTM! Helper method properly handles children list initialization.The
appendItemsToParentmethod correctly initializes the children list if null and safely appends new items. This aligns with the learning thatsetChildrenwraps the list in a new ArrayList.
405-426: LGTM! Well-implemented sibling addition with proper validation.The
addSiblingsmethod includes appropriate validation for root nodes and missing nodes, updates both server and client state correctly, and follows the established pattern for chart mutations.
436-452: LGTM! Client callback properly handles sibling addition events.The
onSiblingsAddedmethod correctly validates the target node exists before processing and firing events, following the established pattern from previous review feedback.
461-463: LGTM! Listener registration returns Registration for proper cleanup.The
addSiblingsAddedListenermethod correctly returns aRegistrationobject, allowing callers to remove the listener later.
472-475: LGTM! Event firing method follows established pattern.The
fireSiblingsAddedEventmethod correctly creates and fires the event with the appropriate parameters.
489-508: LGTM! Smart children addition handles existing children correctly.The
addChildrenmethod intelligently uses different client-side functions based on whether the parent already has children, following the library's API requirements. The null-safe check for existing children is properly implemented.
511-527: LGTM! Client callback properly validates parent before firing events.The
onChildrenAddedmethod includes the null check forparentItemas addressed in previous reviews, preventing misleading events when server and client are out of sync.
536-539: LGTM! Children added listener registration follows standard pattern.The method signature and return type are consistent with other listener registration methods.
548-551: LGTM! Event firing method properly structured.The
fireChildrenAddedEventmethod follows the established pattern for firing component events.
564-588: LGTM! Node removal handles root removal correctly.The
removeNodesmethod properly handles the special case of removing the root node by clearing the internal reference, addressing previous review feedback. The use ofCollections.emptyList()is safe based on the learning thatsetChildrenwraps lists in ArrayList.
591-593: LGTM! Simple client callback for node removal.The
onNodesRemovedmethod correctly fires the removal event with the node ID.
602-604: LGTM! Node removal listener registration is consistent.The listener registration method follows the established pattern and returns a
Registration.
612-614: LGTM! Event firing method for node removal.The
fireNodesRemovedEventmethod correctly creates and fires the removal event.
625-637: LGTM! Parent addition correctly restructures the chart hierarchy.The
addParentmethod properly makes the current root a child of the new parent and updates the internal root reference. The use ofCollections.singletonList()is safe based on the learning aboutsetChildrenimplementation.
646-651: LGTM! Client callback validates parent exists before firing event.The
onParentAddedmethod includes proper null validation for the new parent item.
660-662: LGTM! Parent added listener registration is consistent.The method follows the established pattern for listener registration.
670-672: LGTM! Parent added event firing method.The
fireParentAddedEventmethod correctly creates and fires the event.
715-720: LGTM! Client callback validates updated node exists.The
onNodeUpdatedmethod includes proper null validation before firing the event.
728-730: LGTM! Node updated listener registration follows pattern.The listener registration method is consistent with other event listener methods.
738-740: LGTM! Node updated event firing method.The
fireNodeUpdatedEventmethod correctly creates and fires the update event.
mlopezFC
left a comment
mlopezFC
left a comment
There was a problem hiding this comment.
If there are no unanswered comments from the bunny, I've tested it and it LGTM
This PR includes the addition of new methods to allow chart editing. Now it is possible to add siblings to a selected node, add children to a selected node, add a new root node or parent to the chart, update nodes data and remove nodes.
AddSiblings, addChildren, add¨Parent and removeNodes methods are calling the existing methods for those functions in the core library to update the chart. For updating/editing an specific node data there were no particular method so it was implemented from scratch.
The PR also includes a new demo view to showcase how editing a chart works.
Close #78
Summary by CodeRabbit
New Features
Tests
Chores