fix: Show parent nodes for job relationships #247
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/resources.ts (1)
484-485: Consider ensuring uniqueness in 'nodes' array to avoid duplicatesWhen combining
parentNodes,childrenNodesUpdated,fromNodes, andtoNodes, there may be duplicate nodes if the same node appears in multiple relationships. Consider deduplicating thenodesarray to prevent redundant processing or rendering.You can use a
Mapto ensure uniqueness based on node IDs.return { resource, - nodes: [ + nodes: Array.from( + new Map( [ ...parentNodes, ...childrenNodesUpdated, ...fromNodes.map((n) => n.node), ...toNodes.map((n) => n.node), ] .filter(isPresent) .map((node) => [node.id, node]) ).values() ), };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts(1 hunks)packages/api/src/router/resources.ts(6 hunks)
🔇 Additional comments (8)
packages/api/src/router/resources.ts (4)
295-298: Function 'getChildrenNodesRecursively' looks good
The wrapper function correctly initializes the base node and calls getChildrenNodesRecursivelyHelper.
356-358: Function 'getParentNodesRecursively' correctly initializes the base node
The function properly initializes the base node and calls getParentNodesRecursivelyHelper.
425-433: Integration of parent and child nodes in 'relationships' query
The code correctly retrieves the parent and child nodes and updates the children nodes with the base node data when IDs match.
Line range hint 435-461: Fetching additional node relationships
The code properly constructs fromNodesPromises and toNodesPromises to retrieve related nodes.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (3)
4-5: Imported 'cn' utility function for conditional class names
The import of cn from @ctrlplane/ui allows for conditional class name application in the component.
15-15: Extended 'ResourceNodeProps' with 'isBaseNode' property
Adding isBaseNode: boolean to ResourceNodeProps enables the component to receive and use the new property for conditional styling.
23-26: Applied conditional styling based on 'isBaseNode'
Using the cn function to conditionally apply a border style when data.isBaseNode is true enhances the visual differentiation of base nodes.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts (1)
30-34: Added 'isBaseNode' property to resource node data
Including isBaseNode in the node's data object allows components to conditionally render styles or behaviors based on whether the node is the base node. Setting isBaseNode to true when r.id === relationships.resource.id correctly identifies the base node.
| @@ -286,15 +286,76 @@ const getNodesRecursivelyHelper = async ( | |||
| const children = await Promise.all(childrenPromises); | |||
|
|
|||
| const childrenNodesPromises = children.map((c) => | |||
| getNodesRecursivelyHelper(db, c, []), | |||
| getChildrenNodesRecursivelyHelper(db, c, []), | |||
There was a problem hiding this comment.
Potential issue with node accumulation in recursive function
In getChildrenNodesRecursivelyHelper, you are passing an empty array [] as the nodes parameter in the recursive calls at line 289. This may result in only immediate child nodes being collected, without accumulating nodes from deeper levels of the tree. Consider passing the existing nodes array to properly accumulate all descendant nodes.
Apply this diff to pass the nodes array in recursive calls:
const childrenNodesPromises = children.map((c) =>
- getChildrenNodesRecursivelyHelper(db, c, []),
+ getChildrenNodesRecursivelyHelper(db, c, nodes),
);Committable suggestion skipped: line range outside the PR's diff.
| const { job_resource_relationship: parentRelationship } = parentJob; | ||
|
|
||
| const { parentNodes, node: parentNodeWithData } = | ||
| await getParentNodesRecursivelyHelper(db, parentNode, []); |
There was a problem hiding this comment.
Potential issue with node accumulation in recursive parent function
In getParentNodesRecursivelyHelper, you are passing an empty array [] as the nodes parameter in the recursive call at line 346. This could result in only immediate parent nodes being collected, without accumulating nodes from higher levels in the hierarchy. Consider passing the existing nodes array to properly accumulate all ancestor nodes.
Apply this diff to pass the nodes array in the recursive call:
const { parentNodes, node: parentNodeWithData } =
- await getParentNodesRecursivelyHelper(db, parentNode, []);
+ await getParentNodesRecursivelyHelper(db, parentNode, nodes);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/layout.ts (2)
118-137: Consider refactoring the view fitting logicWhile the logic is correct, it could be more maintainable if extracted into separate functions for clarity.
Consider this refactoring:
- if (config?.focusedNodeId == null) { - setDefaultView(); - return; - } - - const focusedNode = nodes.find((n) => n.id === config.focusedNodeId); - if (focusedNode == null) { - setDefaultView(); - return; - } - - reactFlowInstance.fitView({ - padding: config.padding ?? 0.12, - maxZoom: config.maxZoom ?? 2, - }); - reactFlowInstance.setCenter( - focusedNode.position.x + 100, - focusedNode.position.y + 100, - { zoom: 1.5, duration: 500 }, - ); + const setFocusedView = (node: Node) => { + reactFlowInstance.fitView({ + padding: config.padding ?? 0.12, + maxZoom: config.maxZoom ?? 2, + }); + reactFlowInstance.setCenter( + node.position.x + 100, + node.position.y + 100, + { zoom: 1.5, duration: 500 }, + ); + }; + + const focusedNode = config?.focusedNodeId + ? nodes.find((n) => n.id === config.focusedNodeId) + : null; + + focusedNode ? setFocusedView(focusedNode) : setDefaultView();
130-131: Consider making magic numbers configurableThe code uses several magic numbers that could be made configurable:
- maxZoom: 2
- position offsets: 100
- zoom level: 1.5
- animation duration: 500
Consider adding these to the LayoutConfig type:
type LayoutConfig = { direction?: "TB" | "LR" | "BT" | "RL"; extraEdgeLength?: number; nodesep?: number; padding?: number; maxZoom?: number; focusedNodeId?: string; + focusedNodeConfig?: { + maxZoom?: number; + positionOffset?: number; + zoomLevel?: number; + animationDuration?: number; + }; };Also applies to: 134-136
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/layout.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1)
33-33: Verify that relationships.resource.id is always defined
While the addition of focusedNodeId aligns with the layout changes, we should ensure that relationships.resource.id is always defined to prevent potential runtime issues.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The relationships.resource.id access is safe
The code is safe because:
- The relationships prop is typed as
NonNullable<RouterOutputs["resource"]["relationships"]> - The page component performs a null check (
if (relationships == null) return notFound()) before passing the data - The relationships data is only passed when it's guaranteed to be defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type definitions and usages of relationships to verify if resource.id is required
ast-grep --pattern 'type $name = NonNullable<RouterOutputs["resource"]["relationships"]>' -f ts,tsx
Length of output: 279
Script:
#!/bin/bash
# Search for relationships type definitions and usages
ast-grep --pattern 'type $_ = { relationships: $_ }'
# Search for how relationships.resource is used
rg -A 3 "relationships\.resource"
Length of output: 1568
Script:
#!/bin/bash
# Let's check the Relationships type definition and how resource is typed
ast-grep --pattern 'type Relationships = $_'
# Also check where this type is imported from
rg "import.*Relationships.*from"
Length of output: 636
Script:
#!/bin/bash
# Let's check the router output type for resource.relationships
rg -A 5 "resource: \{"
# Also check how the relationships data is fetched/passed
rg -B 2 -A 2 "useQuery.*resource.*relationships"
Length of output: 2689
Script:
#!/bin/bash
# Let's check the actual component usage to see how relationships is passed
rg -B 5 -A 2 "<ResourceVisualizationDiagram"
# And check the router definition for resource relationships
rg -A 3 "router\.resource\.relationships"
Length of output: 2147
apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/layout.ts (3)
73-73: LGTM: Clean type addition
The optional focusedNodeId property is properly typed and maintains backward compatibility.
103-109: LGTM: Well-structured default view handler
The setDefaultView function is properly memoized and uses safe defaults. Good separation of concerns by extracting this logic.
140-147: LGTM: Proper dependency management
The useEffect dependencies are correctly updated to include all required variables.
Summary by CodeRabbit
New Features
isBaseNode.Bug Fixes
Documentation
focusedNodeIdfor dynamic view adjustments in the layout management.