fix: Resource associations visualization #243
Conversation
WalkthroughThis pull request implements significant changes to the handling of relationships and identifiers within the webservice and API components. The modifications primarily involve updating data access patterns to focus on 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
|
packages/api/src/router/job.ts
Outdated
| @@ -387,37 +394,37 @@ const releaseJobTriggerRouter = createTRPCRouter({ | |||
| WITH RECURSIVE reachable_relationships(id, visited, tr_id, source_id, target_id, type) AS ( | |||
| -- Base case: start with the given ID and no relationship | |||
| SELECT | |||
| ${data.resource.id}::uuid AS id, | |||
| ARRAY[${data.resource.id}::uuid] AS visited, | |||
| ${data.resource.identifier}::uuid AS identifier, | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx (1)
12-12: Use strict equality comparisonWhile the null check is a good addition, consider using strict equality (
===) instead of loose equality (==) for more precise type checking.- if (relationships == null) return notFound(); + if (relationships === null) return notFound();apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts (4)
Line range hint
9-9: Update type definition to reflect the new data structureThe type definition should be more specific about the structure containing
nodesto ensure type safety and better documentation of the expected data shape.-type Relationships = NonNullable<RouterOutputs["resource"]["relationships"]>; +type Relationships = NonNullable<RouterOutputs["resource"]["relationships"]> & { + nodes: Array<NonNullable<RouterOutputs["resource"]["relationships"]["nodes"][number]>>; +};
Line range hint
35-45: Consider using a more robust ID generation patternWhile the current implementation works, combining IDs with a hyphen could potentially lead to collisions if either ID contains hyphens themselves.
- id: `${r.provider.id}-${r.id}`, + id: `provider:${r.provider.id}:resource:${r.id}`,
Line range hint
49-57: Consider optimizing nested array operationsThe current implementation creates intermediate arrays through multiple flatMap operations. Consider using a more direct mapping approach.
- relationships.nodes - .flatMap((r) => r.workspace.systems) - .flatMap((s) => s.environments.map((e) => ({ s, e }))) - .map(({ s, e }) => ({ + relationships.nodes.reduce((acc, r) => { + r.workspace.systems.forEach((s) => { + s.environments.forEach((e) => { + acc.push({ + id: e.id, + type: NodeType.Environment, + data: { environment: e, label: `${s.name}/${e.name}` }, + position: { x: 0, y: 0 }, + }); + }); + }); + return acc; + }, []);
Line range hint
60-75: Optimize performance and improve ID generationThe current implementation has two areas for improvement:
- Multiple nested flatMap operations create unnecessary intermediate arrays
- The ID generation pattern could be more robust
- relationships.nodes.flatMap((r) => - r.workspace.systems.flatMap((system) => - system.environments.flatMap((environment) => - system.deployments.map((deployment) => ({ + relationships.nodes.reduce((acc, r) => { + r.workspace.systems.forEach((system) => { + system.environments.forEach((environment) => { + system.deployments.forEach((deployment) => { + acc.push({ - id: `${environment.id}-${deployment.id}`, + id: `env:${environment.id}:deploy:${deployment.id}`, type: NodeType.Deployment, data: { deployment, environment, resource: r, label: deployment.name, }, position: { x: 0, y: 0 }, - })), - ), - ), - ); + }); + }); + }); + }); + return acc; + }, []);packages/db/src/schema/resource.ts (2)
Line range hint
351-357: Handle Possible Null Value for 'rel' to Prevent Runtime ErrorsThe variable
relmay beundefinediftakeFirst()returns no results. Accessingrel.system.workspaceIdwithout checking could lead to a runtime error. Ensure thatrelis notundefinedbefore accessing its properties.Add a null check for
rel:const rel = await ctx.db .select() .from(release) .innerJoin(deployment, eq(release.deploymentId, deployment.id)) .innerJoin(system, eq(deployment.systemId, system.id)) .then(takeFirst); + if (!rel) { + throw new Error("Related release not found."); + }
Line range hint
397-401: Avoid Casting 'identifier' from Text to UUIDThe
resource.identifieris of typetext, but it's being cast touuidusing::uuid. This can cause runtime errors if the identifier is not a valid UUID. Since identifiers are stored astext, casting them touuidis unsafe.Modify the code to avoid casting:
WITH RECURSIVE reachable_relationships(id, visited, tr_id, source_identifier, target_identifier, type) AS ( -- Base case: start with the given ID and no relationship SELECT - ${data.resource.identifier}::uuid AS identifier, - ARRAY[${data.resource.identifier}::uuid] AS visited, + ${data.resource.identifier} AS identifier, + ARRAY[${data.resource.identifier}] AS visited, NULL::uuid AS tr_id, NULL::uuid AS source_identifier, NULL::uuid AS target_identifier, NULL::resource_relationship_type AS typeEnsure subsequent code handles
identifierastext.apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (1)
67-71: Consider memoizing release job triggersThe nested flatMap operations to collect release job triggers are performed for each resource, which could impact performance with large datasets.
Consider extracting and memoizing the job triggers:
+ const getAllReleaseJobTriggers = (relationships: Relationships) => + relationships.nodes + .flatMap((r) => r.workspace.systems) + .flatMap((s) => s.environments) + .flatMap((e) => e.latestActiveReleases) + .map((rel) => rel.releaseJobTrigger); const createEdgesFromDeploymentsToResources = (relationships: Relationships) => { + const allReleaseJobTriggers = getAllReleaseJobTriggers(relationships); return relationships.nodes.map((resource) => { const { parent } = resource; if (parent == null) return null; - const allReleaseJobTriggers = relationships.nodes - .flatMap((r) => r.workspace.systems) - .flatMap((s) => s.environments) - .flatMap((e) => e.latestActiveReleases) - .map((rel) => rel.releaseJobTrigger);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/RelationshipsDiagramDependencies.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipsDiagram.tsx(1 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts(2 hunks)packages/api/src/router/job.ts(3 hunks)packages/api/src/router/resources.ts(3 hunks)packages/db/drizzle/0043_chemical_black_bird.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource.ts(1 hunks)packages/job-dispatch/src/resource/delete.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx:27-31
Timestamp: 2024-11-12T09:55:38.456Z
Learning: The `SCHEMA.Environment` type includes the `systemId` field.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/RelationshipsDiagramDependencies.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#165
File: apps/webservice/src/app/[workspaceSlug]/_components/relationships/RelationshipsDiagramDependencies.tsx:185-189
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `RelationshipsDiagramDependencies.tsx`, when using React Flow's `BaseEdge`, the `markerStart` prop may intentionally use the `markerEnd` value to flip the arrow direction because 'source depends on target' logic is applied. These are intrinsic attributes of the React Flow `BaseEdge` component and cannot be renamed or changed.
🔇 Additional comments (22)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx (1)
12-13: Verify relationships data structure
The AI summary indicates that the codebase is moving towards using relationships.nodes, but this component still passes the entire relationships object to the provider. Please verify if this is intentional or if it needs to be updated for consistency.
✅ Verification successful
Based on the shell script results, I can generate the final response as I have sufficient information to verify the concern. The results show that:
- The
relationshipsobject is consistently used across the codebase, with both direct usage andrelationships.nodesaccess patterns. - The visualization components (
edges.tsandnodes.ts) internally handle the proper access torelationships.nodeswhen needed. - The
ResourceVisualizationDiagramProvideris part of a proper data flow where the parent component passes the completerelationshipsobject, and child components extract the necessary data usingrelationships.nodes.
The current implementation is correct and consistent
The component correctly passes the entire relationships object to the provider. The child components (edges.ts and nodes.ts) properly access relationships.nodes internally when needed. This pattern maintains proper data encapsulation and allows child components to access other properties of the relationships object if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage pattern of relationships.nodes vs relationships
# across the codebase to ensure consistency
# Search for relationships.nodes pattern
echo "Checking for relationships.nodes usage:"
rg -A 2 "relationships\.nodes"
# Search for direct relationships usage
echo -e "\nChecking for direct relationships usage:"
rg -A 2 "relationships\s*[^.]"
# Check ResourceVisualizationDiagramProvider props
echo -e "\nChecking ResourceVisualizationDiagramProvider implementation:"
ast-grep --pattern 'export $_ function ResourceVisualizationDiagramProvider({ relationships }: { relationships: $_ }) {
$$$
}'
Length of output: 24622
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts (2)
27-32: LGTM! Clean and correct data access pattern
The function correctly accesses the new data structure while maintaining the same output format.
Line range hint 18-24: Verify node type components exist
Let's ensure all referenced node type components exist and are properly exported.
✅ Verification successful
All node type components are properly defined and mapped
The verification confirms:
- All node components (ResourceNode, EnvironmentNode, ProviderNode, DeploymentNode) are properly exported and exist in their respective files
- The NodeType enum values match exactly with the nodeTypes mapping
- Each node type has a corresponding React component implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all node type components exist
for component in ResourceNode EnvironmentNode ProviderNode DeploymentNode; do
echo "Checking for $component..."
rg "export.*$component" -t tsx
done
Length of output: 838
Script:
#!/bin/bash
# Let's try with .ts and .tsx extensions explicitly using fd and rg
echo "Searching for node components..."
fd -e ts -e tsx | xargs rg "export.*\b(ResourceNode|EnvironmentNode|ProviderNode|DeploymentNode)\b"
# Also search for NodeType enum to verify all types are mapped
echo -e "\nSearching for NodeType enum..."
rg "enum\s+NodeType"
Length of output: 1445
Script:
#!/bin/bash
# Let's check the NodeType enum values to ensure all types are properly mapped
echo "Checking NodeType enum contents in the visualize/nodes/nodes.ts file..."
rg -A 10 "enum NodeType" "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts"
Length of output: 565
packages/db/src/schema/resource.ts (1)
Line range hint 437-450: Confirm 'allIdentifiers' Correctly Compiles All Unique Identifiers
The allIdentifiers array is constructed by combining fromIdentifiers, toIdentifiers, and data.resource.identifier. Ensure that all these variables contain the expected identifier values and that duplicates are appropriately handled by _.uniq().
Run the following script to check for inconsistencies:
✅ Verification successful
Let me check the actual code implementation to verify the identifiers handling.
Let me check how these identifiers are populated before they're combined.
Let me check the relationships array construction to complete the verification.
The allIdentifiers Implementation is Correctly Handling Unique Identifiers
Based on the code analysis:
fromIdentifiersandtoIdentifiersare properly populated from database results, with values explicitly cast to strings- The arrays are combined using spread operator and
_.uniq()handles deduplication - The implementation appears in both
resources.tsandjob.tswith consistent handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'fromIdentifiers', 'toIdentifiers', and 'data.resource.identifier' are correctly populated.
# Expected: The arrays contain valid identifiers without unexpected duplicates.
echo "fromIdentifiers: ${fromIdentifiers}"
echo "toIdentifiers: ${toIdentifiers}"
echo "data.resource.identifier: ${data.resource.identifier}"
# Check for duplicates
duplicates=$(echo "${allIdentifiers[@]}" | tr ' ' '\n' | sort | uniq -d)
if [ -n "$duplicates" ]; then
echo "Duplicate identifiers found: $duplicates"
else
echo "No duplicates in allIdentifiers."
fi
Length of output: 375
Script:
#!/bin/bash
# Search for the allIdentifiers implementation
ast-grep --pattern 'const allIdentifiers = $_'
Length of output: 681
Script:
#!/bin/bash
# Search for fromIdentifiers and toIdentifiers declarations
ast-grep --pattern 'const fromIdentifiers = $_'
ast-grep --pattern 'const toIdentifiers = $_'
# Also check the context around these usages
rg "fromIdentifiers|toIdentifiers" -A 3 -B 3
Length of output: 2088
Script:
#!/bin/bash
# Search for relationships array construction
rg "const relationships = " -A 5 -B 5
Length of output: 4557
packages/api/src/router/job.ts (2)
397-425: Review Recursive Query Logic for 'reachable_relationships'
The recursive CTE reachable_relationships includes changes to use identifiers instead of IDs. Ensure that the logic correctly identifies related resources using from_identifier and to_identifier.
Double-check the query to confirm that relationships are correctly traversed. If possible, add unit tests to validate the recursive logic.
437-450: Ensure Consistent Typing When Building 'relationships' and 'allIdentifiers'
When constructing the relationships array and the allIdentifiers list, ensure that the identifiers are of the correct type (string) and that there are no type mismatches or runtime errors.
Run the following script to validate the types:
✅ Verification successful
Let me gather more context about the type safety of these identifiers.
Let me gather more specific information about the type safety of these identifiers.
From the results, I can see that proper type safety is being maintained. The code explicitly converts the identifiers to strings using String() and has proper type assertions. Let me generate the final response.
Type Safety is Properly Maintained in Identifier Handling
The code already implements proper type safety:
- Source and target identifiers are explicitly converted to strings using
String() - The relationship type is properly type-asserted as
"associated_with" | "depends_on" - The SQL query properly handles UUID types with explicit casting
- All identifiers in the final array are guaranteed to be strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that identifiers are strings and not undefined.
# Check for 'undefined' or incorrect types in relationships
grep -rnw 'packages/api/src/router/job.ts' -e 'relationships.map' -A 5
# Ensure allIdentifiers contains only unique, valid strings
echo "${allIdentifiers[@]}" | tr ' ' '\n' | awk '{ if ($0 == "") print "Empty identifier found"; else if ($0 ~ /undefined/) print "Undefined identifier found" }'
Length of output: 531
Script:
#!/bin/bash
# Check the type definitions and usage of source_identifier and target_identifier
ast-grep --pattern 'r.source_identifier'
ast-grep --pattern 'r.target_identifier'
# Check the type of data.resource.identifier
ast-grep --pattern 'data.resource.identifier'
# Look for type definitions related to these identifiers
rg "type.*Identifier" -A 2
rg "interface.*Resource" -A 5
Length of output: 65610
Script:
#!/bin/bash
# Check the context around the relationships mapping
rg -B 5 -A 5 "const relationships = " packages/api/src/router/job.ts
# Look for the type definition of the relationship object
rg -B 2 -A 2 "source_identifier|target_identifier" packages/api/src/router/job.ts
# Check how data.resource.identifier is typed
ast-grep --pattern 'interface Resource {' -A 10
Length of output: 2060
packages/job-dispatch/src/resource/delete.ts (1)
15-16: Update Conditions to Match Identifier Types
In the deleteObjectsAssociatedWithResource function, the conditions have been updated to use fromIdentifier and toIdentifier. Ensure that these fields correctly reference the resource's identifier and that the deletion logic accurately targets the associated relationships.
packages/db/drizzle/0043_chemical_black_bird.sql (1)
1-17:
Ensure Data Integrity After Schema Changes
The SQL script alters the resource_relationship table by renaming columns and changing data types from uuid to text. This may lead to data inconsistencies if existing uuid values are not compatible with text fields. Additionally, the foreign key constraints on source_id and target_id have been dropped without adding new constraints for from_identifier and to_identifier.
Consider adding foreign key constraints to maintain referential integrity:
ALTER TABLE "resource_relationship"
ADD CONSTRAINT "resource_relationship_from_identifier_fkey"
FOREIGN KEY ("from_identifier") REFERENCES "resource" ("identifier") ON DELETE CASCADE;
ALTER TABLE "resource_relationship"
ADD CONSTRAINT "resource_relationship_to_identifier_fkey"
FOREIGN KEY ("to_identifier") REFERENCES "resource" ("identifier") ON DELETE CASCADE;Ensure that data migration scripts handle the conversion of uuid values to text identifiers correctly.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (1)
12-13: Update Type Definition for Environment
The type for Environment has been updated to reflect the structure change from relationships to relationships.nodes. Ensure that this change aligns with the data returned from the API and that all usages of Environment are updated accordingly.
apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (1)
46-47: LGTM: Improved response structure
Including the created relationship in the response is a good practice for REST APIs.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipsDiagram.tsx (2)
47-47: LGTM: Consistent identifier usage
The changes correctly use resource identifiers for node IDs and orphan detection, aligning with the API changes.
Also applies to: 54-56
63-65: LGTM: Edge creation with identifiers
Edge creation properly uses the new identifier fields from relationships.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (2)
114-130: LGTM: Improved association edge creation
The new fromEdges and toEdges creation properly handles bidirectional associations, improving the visualization.
137-138: LGTM: Complete edge collection
All edge types are properly collected and filtered.
packages/db/drizzle/meta/_journal.json (1)
305-311: LGTM!
The new migration entry follows the established pattern and maintains consistency with existing entries.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/RelationshipsDiagramDependencies.tsx (3)
76-81: LGTM: Graph construction logic is correct.
The function correctly builds an undirected graph using the new identifier naming convention.
106-115: LGTM: Node initialization and orphan detection logic is accurate.
The orphan node detection logic correctly uses the new identifier naming convention.
122-124: LGTM: Edge handling is consistent with React Flow expectations.
The edge creation, reset, and highlighting logic correctly uses the new identifier naming convention while maintaining React Flow's expected behavior for edge markers.
Let's verify the edge creation consistency:
Also applies to: 142-144, 181-181, 186-188
✅ Verification successful
Edge handling is consistent across relationship diagrams
The verification confirms that the edge creation pattern is consistent across the codebase. Both RelationshipsDiagram.tsx and RelationshipsDiagramDependencies.tsx use the same pattern for:
- Edge IDs: concatenating
fromIdentifier-toIdentifier - Source/target properties: using
fromIdentifierandtoIdentifierrespectively - Edge styling: applying consistent marker types and color schemes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify edge creation pattern across the codebase
# Test: Search for edge creation patterns. Expect: Consistent usage of fromIdentifier/toIdentifier
rg -A 2 'id: `\${.*?}-\${.*?}`.*source:.*target:'
Length of output: 181
Script:
#!/bin/bash
# Search for edge creation patterns with source and target properties
rg -U 'id:.*source:.*target:' -A 3
# Also search for specific pattern of identifier concatenation
rg -U 'fromIdentifier.*toIdentifier' -A 3
Length of output: 3602
packages/api/src/router/resources.ts (4)
53-85: LGTM: Recursive CTE correctly handles identifier renaming.
The SQL query has been updated to use the new identifier naming convention while maintaining the same recursive traversal logic.
442-446: Improved error handling with resource existence check.
Good addition of the resource existence check before proceeding with relationship queries.
502-523: LGTM: Response structure maintains clear separation.
The response structure clearly separates nodes and associations, making it easier for clients to consume the data.
449-496: LGTM: Relationship queries are well-structured.
The separation into fromNodes and toNodes queries improves code clarity and maintainability.
Let's verify the relationship query patterns:
✅ Verification successful
Relationship queries are indeed well-structured and consistent
The verification confirms that the relationship queries maintain consistent patterns across the codebase:
- In
resources.ts, the queries properly handle both directions (from/to) with consistent field usage - The same pattern is followed in the job-dispatch module
- All queries properly use workspaceId for scoping
- The join conditions and where clauses are consistently structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify relationship query patterns
# Test: Search for relationship queries. Expect: Consistent usage of fromIdentifier/toIdentifier
ast-grep --pattern 'select()
.from(schema.resourceRelationship)
.where(
and(
eq(schema.resourceRelationship.$_, $_),
$_
)
)'
Length of output: 152
Script:
#!/bin/bash
# Let's try a simpler pattern to find relationship queries
rg "from\(schema\.resourceRelationship\)" -A 10 -B 2
Length of output: 1795
Script:
#!/bin/bash
# Let's check for any other usages of resourceRelationship to ensure consistency
rg "resourceRelationship\." -A 2 -B 2
Length of output: 2035
| workspaceId: uuid("workspace_id") | ||
| .notNull() | ||
| .references(() => workspace.id, { onDelete: "cascade" }), | ||
| fromIdentifier: text("from_identifier").notNull(), | ||
| toIdentifier: text("to_identifier").notNull(), | ||
| type: resourceRelationshipType("type").notNull(), | ||
| }, | ||
| (t) => ({ uniq: uniqueIndex().on(t.targetId, t.sourceId) }), | ||
| (t) => ({ uniq: uniqueIndex().on(t.toIdentifier, t.fromIdentifier) }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Missing Foreign Key Constraints for Identifiers
The fromIdentifier and toIdentifier fields are defined as text without foreign key constraints to the resource.identifier. This could lead to referential integrity issues if resources are deleted or identifiers change. Consider adding foreign key constraints to ensure data consistency.
Modify the table definition to include foreign key references:
export const resourceRelationship = pgTable(
"resource_relationship",
{
id: uuid("id").primaryKey().defaultRandom(),
workspaceId: uuid("workspace_id")
.notNull()
.references(() => workspace.id, { onDelete: "cascade" }),
fromIdentifier: text("from_identifier")
.notNull()
+ .references(() => resource.identifier, { onDelete: "cascade" }),
toIdentifier: text("to_identifier")
.notNull()
+ .references(() => resource.identifier, { onDelete: "cascade" }),
type: resourceRelationshipType("type").notNull(),
},
(t) => ({ uniq: uniqueIndex().on(t.toIdentifier, t.fromIdentifier) }),
);Committable suggestion skipped: line range outside the PR's diff.
packages/api/src/router/job.ts
Outdated
| const rel = await ctx.db | ||
| .select() | ||
| .from(release) | ||
| .innerJoin(deployment, eq(release.deploymentId, deployment.id)) | ||
| .innerJoin(system, eq(deployment.systemId, system.id)) | ||
| .then(takeFirst); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize 'rel' Variable to Prevent Undefined Access
The variable rel may be undefined if the query does not return any results. Accessing rel.system.workspaceId later will cause a runtime error. Ensure that rel is properly initialized.
Add a check to handle the case when rel is undefined:
const rel = await ctx.db
.select()
.from(release)
.innerJoin(deployment, eq(release.deploymentId, deployment.id))
.innerJoin(system, eq(deployment.systemId, system.id))
.then(takeFirst);
+ if (!rel) {
+ throw new Error("Release not found for the given job.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rel = await ctx.db | |
| .select() | |
| .from(release) | |
| .innerJoin(deployment, eq(release.deploymentId, deployment.id)) | |
| .innerJoin(system, eq(deployment.systemId, system.id)) | |
| .then(takeFirst); | |
| const rel = await ctx.db | |
| .select() | |
| .from(release) | |
| .innerJoin(deployment, eq(release.deploymentId, deployment.id)) | |
| .innerJoin(system, eq(deployment.systemId, system.id)) | |
| .then(takeFirst); | |
| if (!rel) { | |
| throw new Error("Release not found for the given job."); | |
| } |
| const inWorkspace = eq(SCHEMA.resource.workspaceId, body.workspaceId); | ||
| const fromResource = await db.query.resource.findFirst({ | ||
| where: and( | ||
| inWorkspace, | ||
| eq(SCHEMA.resource.identifier, body.fromIdentifier), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Verify resource existence before creating relationship
The current implementation doesn't validate if the resources actually exist before creating the relationship. If either resource is not found, it will still create the relationship using the provided identifiers.
Apply this diff to add proper validation:
const fromResource = await db.query.resource.findFirst({
where: and(
inWorkspace,
eq(SCHEMA.resource.identifier, body.fromIdentifier),
),
});
+ if (!fromResource) {
+ return Response.json(
+ { error: "From resource not found" },
+ { status: 404 },
+ );
+ }
const toResource = await db.query.resource.findFirst({
where: and(
inWorkspace,
eq(SCHEMA.resource.identifier, body.toIdentifier),
),
});
+ if (!toResource) {
+ return Response.json(
+ { error: "To resource not found" },
+ { status: 404 },
+ );
+ }Also applies to: 32-37
| const relationship = await db.insert(SCHEMA.resourceRelationship).values({ | ||
| ...body, | ||
| fromIdentifier: fromResource?.identifier ?? body.fromIdentifier, | ||
| toIdentifier: toResource?.identifier ?? body.toIdentifier, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use validated resource identifiers
Since we're fetching the resources first, we should use their validated identifiers instead of falling back to the request body.
Apply this diff:
const relationship = await db.insert(SCHEMA.resourceRelationship).values({
...body,
- fromIdentifier: fromResource?.identifier ?? body.fromIdentifier,
- toIdentifier: toResource?.identifier ?? body.toIdentifier,
+ fromIdentifier: fromResource.identifier,
+ toIdentifier: toResource.identifier,
});Committable suggestion skipped: line range outside the PR's diff.
packages/api/src/router/resources.ts
Outdated
| .filter((n) => n.node != null) | ||
| .map((n) => ({ | ||
| ...n.resource_relationship, | ||
| resource: n.node!, | ||
| })), |
There was a problem hiding this comment.
if you do isPresent check, typescript will know in the next map node is not null
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/JobDrawer.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/RelationshipsDiagramDependencies.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/TargetDrawer.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipContent.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipsDiagram.tsx(0 hunks)packages/api/src/router/job.ts(1 hunks)packages/api/src/router/resources.ts(1 hunks)packages/job-dispatch/src/policy-checker.ts(0 hunks)
💤 Files with no reviewable changes (6)
- packages/job-dispatch/src/policy-checker.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/TargetDrawer.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/RelationshipsDiagramDependencies.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipContent.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-drawer/JobDrawer.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/relationships/RelationshipsDiagram.tsx
🔇 Additional comments (2)
packages/api/src/router/job.ts (1)
381-381: Initialize 'data' variable to prevent undefined access
The variable data might be undefined if the query does not return any results. Returning undefined may cause issues in the calling code. Ensure that data is properly initialized or handle the case when data is undefined.
Apply the following diff to handle the undefined case:
+ if (!data) {
+ throw new Error("No data found for the given job ID.");
+ }
return data;packages/api/src/router/resources.ts (1)
414-441: LGTM! Clear and type-safe results aggregation
The results aggregation is well-structured:
- Proper Promise.all usage for parallel execution
- Type-safe filtering of null nodes
- Clear separation of nodes and associations
| const resource = await ctx.db.query.resource.findFirst({ | ||
| where: eq(schema.resource.id, input), | ||
| }); | ||
| if (resource == null) return null; |
There was a problem hiding this comment.
Add isNotDeleted check for consistency
The resource existence check should include isNotDeleted to maintain consistency with other queries and prevent operating on soft-deleted resources.
const resource = await ctx.db.query.resource.findFirst({
- where: eq(schema.resource.id, input),
+ where: and(
+ eq(schema.resource.id, input),
+ isNotDeleted
+ ),
});Committable suggestion skipped: line range outside the PR's diff.
| const toNodesPromises = ctx.db | ||
| .select() | ||
| .from(schema.resourceRelationship) | ||
| .innerJoin( | ||
| schema.resource, | ||
| eq( | ||
| schema.resourceRelationship.toIdentifier, | ||
| schema.resource.identifier, | ||
| ), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(schema.resourceRelationship.workspaceId, resource.workspaceId), | ||
| eq(schema.resourceRelationship.fromIdentifier, resource.identifier), | ||
| ), | ||
| ) | ||
| .then((rows) => | ||
| rows.map(async (row) => ({ | ||
| ...row, | ||
| node: await getNodeDataForResource(ctx.db, row.resource.id), | ||
| })), | ||
| ) | ||
| .then((promises) => Promise.all(promises)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code and optimize queries
This code block is nearly identical to the from nodes retrieval. Consider extracting a reusable function and applying the same query optimization.
+const getRelatedNodes = async (
+ db: Tx,
+ workspaceId: string,
+ fromIdentifier: string,
+ toIdentifier: string,
+) => {
+ return db
+ .select({
+ resource: schema.resource,
+ relationship: schema.resourceRelationship,
+ provider: schema.resourceProvider,
+ workspace: schema.workspace,
+ })
+ .from(schema.resourceRelationship)
+ // ... (rest of the optimized query)
+};
-const toNodesPromises = ctx.db
- .select()
- // ... (current implementation)
+const toNodesPromises = getRelatedNodes(
+ ctx.db,
+ resource.workspaceId,
+ resource.identifier,
+ resource.identifier,
+);Committable suggestion skipped: line range outside the PR's diff.
| const fromNodesPromises = ctx.db | ||
| .select() | ||
| .from(schema.resourceRelationship) | ||
| .innerJoin( | ||
| schema.resource, | ||
| eq( | ||
| schema.resourceRelationship.fromIdentifier, | ||
| schema.resource.identifier, | ||
| ), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(schema.resourceRelationship.workspaceId, resource.workspaceId), | ||
| eq(schema.resourceRelationship.toIdentifier, resource.identifier), | ||
| ), | ||
| ) | ||
| .then((rows) => | ||
| rows.map(async (row) => ({ | ||
| ...row, | ||
| node: await getNodeDataForResource(ctx.db, row.resource.id), | ||
| })), | ||
| ) | ||
| .then((promises) => Promise.all(promises)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize N+1 query pattern
The current implementation makes N+1 queries by calling getNodeDataForResource for each relationship. This could be optimized by fetching all resources in a single query.
const fromNodesPromises = ctx.db
.select({
+ resource: schema.resource,
+ relationship: schema.resourceRelationship,
+ provider: schema.resourceProvider,
+ workspace: schema.workspace,
})
.from(schema.resourceRelationship)
.innerJoin(
schema.resource,
eq(
schema.resourceRelationship.fromIdentifier,
schema.resource.identifier,
),
)
+ .leftJoin(
+ schema.resourceProvider,
+ eq(schema.resource.providerId, schema.resourceProvider.id),
+ )
+ .innerJoin(
+ schema.workspace,
+ eq(schema.resource.workspaceId, schema.workspace.id),
+ )
.where(
and(
eq(schema.resourceRelationship.workspaceId, resource.workspaceId),
eq(schema.resourceRelationship.toIdentifier, resource.identifier),
),
)
- .then((rows) =>
- rows.map(async (row) => ({
- ...row,
- node: await getNodeDataForResource(ctx.db, row.resource.id),
- })),
- )
- .then((promises) => Promise.all(promises));Committable suggestion skipped: line range outside the PR's diff.
| where: eq(schema.resource.id, input), | ||
| }); | ||
| if (resource == null) return null; | ||
| const childrenNodes = await getNodesRecursively(ctx.db, input); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a depth limit to prevent stack overflow
The recursive implementation of getNodesRecursively could potentially cause stack overflow for deeply nested relationships. Consider adding a depth limit parameter.
-const getNodesRecursively = async (db: Tx, resourceId: string) => {
+const getNodesRecursively = async (
+ db: Tx,
+ resourceId: string,
+ maxDepth: number = 10
+) => {
const baseNode = await getNodeDataForResource(db, resourceId);
- return getNodesRecursivelyHelper(db, baseNode, []);
+ return getNodesRecursivelyHelper(db, baseNode, [], maxDepth);
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor