fix: Show children resources in visualization #230
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 8
🧹 Outside diff range and nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx (2)
9-9: Add type annotation for relationshipsThe
relationshipsvariable lacks type annotation, which could lead to type-related issues downstream.Consider adding type annotation:
- const relationships = await api.resource.relationships(targetId); + const relationships: Relationships = await api.resource.relationships(targetId);Make sure to import the
Relationshipstype from your types definition file.
10-10: Consider adding loading state handlingSince this is an async operation fetching data, it would be beneficial to handle loading states for better user experience.
Consider using React Suspense or a loading component:
+import { Suspense } from 'react'; +import { LoadingSpinner } from '~/components/ui/loading-spinner'; export default async function VisualizePage({ params: { targetId }, }: { params: { targetId: string }; }) { const relationships = await api.resource.relationships(targetId); - return <ResourceVisualizationDiagramProvider relationships={relationships} />; + return ( + <Suspense fallback={<LoadingSpinner />}> + <ResourceVisualizationDiagramProvider relationships={relationships} /> + </Suspense> + ); }packages/db/src/schema/job.ts (1)
Line range hint
200-209: Consider enhancing resource filtering capabilities.The
buildConditionfunction'sJobFilterType.JobTargetcase currently only filters by direct resource ID. With the new job-resource relationships, consider enhancing this to support filtering by related resources through thejobResourceRelationshiptable.Here's a suggested enhancement:
if (cond.type === JobFilterType.JobTarget) - return and(eq(resource.id, cond.value), isNull(resource.deletedAt))!; + return exists( + tx + .select() + .from(jobResourceRelationship) + .where( + and( + eq(jobResourceRelationship.jobId, job.id), + eq(jobResourceRelationship.resourceId, cond.value), + isNull(resource.deletedAt) + ), + ), + );apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (4)
77-77: Avoid leaving commented-out codeOn line 77, there's a commented-out
refetchIntervaloption:// { refetchInterval: 5_000 },If the
refetchIntervalis no longer needed, consider removing this line to keep the codebase clean. If it's temporarily disabled, add a comment explaining why it's commented out.
149-149: Avoid magic numbers intopOffsetcalculationOn line 149, the
topOffsetcalculation uses hardcoded values:const topOffset = 44 + (56 + 16) * index + 56 / 2;Using magic numbers can make the code harder to maintain and understand. Consider defining these values as constants or calculating them dynamically based on the actual sizes of the elements.
Suggest defining constants for better readability:
const HEADER_HEIGHT = 44; // Header height plus padding const CARD_HEIGHT = 56; // Height of each card const GAP = 16; // Gap between cards const topOffset = HEADER_HEIGHT + (CARD_HEIGHT + GAP) * index + CARD_HEIGHT / 2;
153-156: Removeconsole.logstatements from production codeThe
console.logstatement on lines 153-156 is likely intended for debugging:console.log({ latestActiveReleaseJobId: latestActiveRelease?.releaseJobTrigger.job.id, });It's best practice to remove debugging statements like
console.logbefore committing code to ensure clean and efficient production code.Suggest removing the
console.logstatement:- console.log({ - latestActiveReleaseJobId: latestActiveRelease?.releaseJobTrigger.job.id, - });
42-42: Remove duplicateborderclass inclassNameOn line 42, the
classNameincludes duplicatedborderclasses with an extra space:"flex h-14 w-72 flex-grow items-center gap-12 rounded-md border border-neutral-800 px-4 py-3",This redundancy may have no effect but can be cleaned up for clarity.
Suggest removing the extra
border:"flex h-14 w-72 flex-grow items-center gap-12 rounded-md border-neutral-800 px-4 py-3",packages/api/src/router/resources.ts (1)
333-519: Add unit tests for new functions to ensure correctnessThe new functions
latestActiveReleaseByResourceAndEnvironmentId,getNodeDataForResource,getNodesRecursivelyHelper, andgetNodesRecursivelyare central to the resource relationship logic. Consider adding unit tests to verify their correctness and handle edge cases, such as resources with no relationships or cyclic dependencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx(2 hunks)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(7 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx(1 hunks)packages/api/src/router/resources.ts(2 hunks)packages/db/src/schema/job.ts(2 hunks)packages/db/src/schema/resource.ts(2 hunks)
🔇 Additional comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/page.tsx (1)
9-10: Verify the impact of removing resource prop
The removal of the resource prop in favor of only relationships represents a significant architectural change.
Let's verify the completeness of this change:
✅ Verification successful
The removal of the resource prop is a valid architectural change
Based on the code analysis:
- The
ResourceVisualizationDiagramPropstype only requiresrelationshipsprop - The
ResourceVisualizationDiagramProvidercomponent is consistently used with only therelationshipsprop - All necessary resource data is available through the
relationshipsobject, as seen in the component implementations where resource information is accessed via the relationships data structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed resource prop
# and verify that relationships contain all necessary data
# Check for any remaining ResourceVisualizationDiagramProvider usage with resource prop
rg -A 2 "ResourceVisualizationDiagramProvider.*resource="
# Check the type definition to ensure it's updated
ast-grep --pattern 'type ResourceVisualizationDiagramProps = {
$$$
}'
# Check for any TODO or FIXME comments related to this change
rg -i "todo.*resource|fixme.*resource" --type ts --type tsx
Length of output: 664
Script:
#!/bin/bash
# Let's perform a more comprehensive check
# Check for any ResourceVisualizationDiagramProvider usage
rg "ResourceVisualizationDiagramProvider" --type typescript --type tsx -A 3
# Check the implementation of ResourceVisualizationDiagram component
ast-grep --pattern 'function ResourceVisualizationDiagram({ $$ }) {
$$$
}'
# Check for any resource-related props or types in the visualization components
rg "resource" --type typescript --type tsx -A 2 "apps/webservice/src/app/\[workspaceSlug\]/\(app\)/\(targets\)/targets/\[targetId\]/visualize/"
Length of output: 368
Script:
#!/bin/bash
# Let's try with correct file extensions and without type flags
# Check for ResourceVisualizationDiagramProvider usage
rg "ResourceVisualizationDiagramProvider" -A 3
# Check the implementation and props in the visualization directory
fd . "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/" -e tsx -e ts --exec cat {}
Length of output: 18711
packages/db/src/schema/job.ts (1)
50-50: LGTM! Verify schema migration.
The addition of the many-to-many relationship between jobs and resources through jobResourceRelationship looks good and aligns with the PR objective.
Let's verify if the schema migration exists:
Also applies to: 100-100
✅ Verification successful
Schema migration verified and looks good!
The schema changes are properly handled in migration file 0042_absent_cerebro.sql which:
- Creates the
job_resource_relationshiptable - Sets up the foreign key constraint to the
jobtable - Creates a unique index on
job_idandresource_identifier - Includes proper cleanup of the old
deployment_resource_relationshiptable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration files that add the job-resource relationship.
# Look for migration files
fd --type f "migration" --exec rg -l "job.*resource|resource.*job"
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search for migration files first
fd "migration|schema"
# Then let's look for any SQL files that might contain the schema changes
fd -e sql
Length of output: 2184
Script:
#!/bin/bash
# Let's check the most recent migration files for job-resource relationship changes
for file in packages/db/drizzle/00{40,41,42}_*.sql; do
echo "=== $file ==="
cat "$file"
done
Length of output: 1644
packages/db/src/schema/resource.ts (2)
81-81: LGTM: Relationship properly defined
The jobRelationships relationship is correctly added to the resourceRelations definition, following the established naming conventions and properly using the many-to-many relationship pattern.
287-300:
Review relationship design and foreign key constraints
There are several concerns with the current implementation:
-
Using
resourceIdentifieras a foreign key reference toresource.identifier:- This references a non-primary key column which could lead to referential integrity issues
- If resource identifiers change, it could break the relationships
- PostgreSQL won't enforce foreign key constraints on non-unique columns
-
The relationships are defined as one-to-one but the table structure suggests many-to-many:
- The unique index allows multiple relationships
- The relations should use
many()instead ofone()
Consider these improvements:
export const jobResourceRelationship = pgTable(
"job_resource_relationship",
{
id: uuid("id").primaryKey().defaultRandom(),
jobId: uuid("job_id")
.references(() => job.id, { onDelete: "cascade" })
.notNull(),
- resourceIdentifier: text("resource_identifier").notNull(),
+ resourceId: uuid("resource_id")
+ .references(() => resource.id, { onDelete: "cascade" })
+ .notNull(),
},
- (t) => ({ uniq: uniqueIndex().on(t.jobId, t.resourceIdentifier) }),
+ (t) => ({ uniq: uniqueIndex().on(t.jobId, t.resourceId) }),
);
export const jobResourceRelationshipRelations = relations(
jobResourceRelationship,
- ({ one }) => ({
+ ({ many }) => ({
- job: one(job, {
+ job: many(job, {
fields: [jobResourceRelationship.jobId],
references: [job.id],
}),
- resource: one(resource, {
+ resource: many(resource, {
- fields: [jobResourceRelationship.resourceIdentifier],
- references: [resource.identifier],
+ fields: [jobResourceRelationship.resourceId],
+ references: [resource.id],
}),
}),
);Let's verify if there are any existing relationships that would be affected by this change:
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (4)
1-1: Import statement is appropriate
The import of RouterOutputs from "@ctrlplane/api" is necessary for type definitions used below.
35-35: Update to 'source' property ensures unique edge identification
Changing the source property to ${provider.id}-${resource.id} aligns with the edge id and ensures consistent identification of edges between providers and resources.
43-43: Definition of 'Relationships' type provides type safety
Defining Relationships as NonNullable<RouterOutputs["resource"]["relationships"]> ensures that the relationships variable is non-nullable, improving type safety and preventing potential null reference errors.
66-68: Verify correctness of 'source' and 'sourceHandle' assignments
Setting source to releaseJobTrigger.environmentId and sourceHandle to releaseJobTrigger.jobId assumes that these IDs correspond to existing nodes and handles in the visualization. Ensure that these values correctly reference valid nodes and that the jobId is an appropriate handle for the environmentId node.
Run the following script to verify that the environmentId and jobId correspond to existing nodes and handles:
✅ Verification successful
Let me gather more information about the nodes and their structure to verify the correctness of these assignments.
Let me check one more crucial piece - the node structure in the visualization component where these edges are being used.
The edge assignments are correct and properly structured
The code analysis reveals that the edge creation is well-structured and valid:
- The
environmentIdis correctly used as the source node, as it represents the environment where the job is running - The
jobIdassourceHandleis appropriate as it uniquely identifies the job within that environment that created the resource - The edge structure is consistent with the rest of the visualization's edge definitions in the file
- The relationships data structure properly maps environments to their associated jobs and resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that environment IDs and job IDs exist as nodes and handles.
# Check for nodes with matching environment IDs
rg 'id:.*environmentId' --type ts -A 5
# Check for handles with matching job IDs
rg 'sourceHandle:.*jobId' --type ts -A 5
Length of output: 4562
Script:
#!/bin/bash
# Check for node definitions and their structure in the visualization
ast-grep --pattern 'const nodes = $$$'
# Check for any other usage of environmentId in node creation
rg 'type.*environment.*id' --type ts -A 3
# Look for job-related node definitions
rg 'type.*job.*id' --type ts -A 3
Length of output: 8265
Script:
#!/bin/bash
# Check the visualization component structure
rg -A 10 'function.*visualize' --type ts
# Look for node type definitions related to the visualization
ast-grep --pattern 'type.*Node.*=.*{$$$}'
# Check the file containing the edges code for node creation
cat "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts"
Length of output: 2236
packages/api/src/router/resources.ts (2)
333-402: Function latestActiveReleaseByResourceAndEnvironmentId implementation looks good
The function correctly retrieves the latest active release for a given resource and environment, using a rank subquery to select the most recent releases per deployment.
646-646: Update to use getNodesRecursively in relationships query enhances maintainability
The relationships query now leverages getNodesRecursively to fetch resource relationships recursively, simplifying the code and improving maintainability.
| const relationships = await api.resource.relationships(targetId); | ||
| return <ResourceVisualizationDiagramProvider relationships={relationships} />; |
There was a problem hiding this comment.
Add error handling for the API call
The current implementation lacks error handling for the API call. If api.resource.relationships fails or returns null/undefined, it could lead to runtime errors.
Consider adding error handling:
- const relationships = await api.resource.relationships(targetId);
- return <ResourceVisualizationDiagramProvider relationships={relationships} />;
+ try {
+ const relationships = await api.resource.relationships(targetId);
+ if (!relationships) {
+ return { notFound: true };
+ }
+ return <ResourceVisualizationDiagramProvider relationships={relationships} />;
+ } catch (error) {
+ console.error('Failed to fetch relationships:', error);
+ throw error; // Let the nearest error boundary handle it
+ }📝 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 relationships = await api.resource.relationships(targetId); | |
| return <ResourceVisualizationDiagramProvider relationships={relationships} />; | |
| try { | |
| const relationships = await api.resource.relationships(targetId); | |
| if (!relationships) { | |
| return { notFound: true }; | |
| } | |
| return <ResourceVisualizationDiagramProvider relationships={relationships} />; | |
| } catch (error) { | |
| console.error('Failed to fetch relationships:', error); | |
| throw error; // Let the nearest error boundary handle it | |
| } |
| export const createEdgesFromDeploymentsToResources = ( | ||
| relationships: Relationships, | ||
| ) => | ||
| relationships.map((resource) => { | ||
| const { parent } = resource; | ||
| if (parent == null) return null; | ||
|
|
||
| const allReleaseJobTriggers = relationships.flatMap((r) => | ||
| r.workspace.systems.flatMap((s) => | ||
| s.environments.flatMap((e) => | ||
| e.latestActiveRelease.map((rel) => rel.releaseJobTrigger), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| const releaseJobTrigger = allReleaseJobTriggers.find( | ||
| (j) => j.jobId === parent.jobId, | ||
| ); | ||
| if (releaseJobTrigger == null) return null; | ||
|
|
||
| return { | ||
| id: `${releaseJobTrigger.jobId}-${resource.id}`, | ||
| source: releaseJobTrigger.environmentId, | ||
| sourceHandle: releaseJobTrigger.jobId, | ||
| target: resource.id, | ||
| style: { stroke: colors.neutral[700] }, | ||
| markerEnd, | ||
| label: "created", | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Handle potential null or undefined properties in nested structures
In the createEdgesFromDeploymentsToResources function, accessing nested properties like r.workspace.systems, s.environments, and e.latestActiveRelease without checking for null or undefined values may lead to runtime errors if any of these properties are missing.
To prevent potential runtime errors, consider adding null checks or using optional chaining (?.) to safely access these properties. For example:
- const allReleaseJobTriggers = relationships.flatMap((r) =>
- r.workspace.systems.flatMap((s) =>
- s.environments.flatMap((e) =>
- e.latestActiveRelease.map((rel) => rel.releaseJobTrigger),
- ),
- ),
- );
+ const allReleaseJobTriggers = relationships.flatMap((r) =>
+ r.workspace?.systems?.flatMap((s) =>
+ s?.environments?.flatMap((e) =>
+ e?.latestActiveRelease?.map((rel) => rel.releaseJobTrigger) ?? [],
+ ) ?? [],
+ ) ?? []
+ ) ?? [];This ensures that if any property in the chain is null or undefined, the code will not throw an error and will continue safely.
📝 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.
| export const createEdgesFromDeploymentsToResources = ( | |
| relationships: Relationships, | |
| ) => | |
| relationships.map((resource) => { | |
| const { parent } = resource; | |
| if (parent == null) return null; | |
| const allReleaseJobTriggers = relationships.flatMap((r) => | |
| r.workspace.systems.flatMap((s) => | |
| s.environments.flatMap((e) => | |
| e.latestActiveRelease.map((rel) => rel.releaseJobTrigger), | |
| ), | |
| ), | |
| ); | |
| const releaseJobTrigger = allReleaseJobTriggers.find( | |
| (j) => j.jobId === parent.jobId, | |
| ); | |
| if (releaseJobTrigger == null) return null; | |
| return { | |
| id: `${releaseJobTrigger.jobId}-${resource.id}`, | |
| source: releaseJobTrigger.environmentId, | |
| sourceHandle: releaseJobTrigger.jobId, | |
| target: resource.id, | |
| style: { stroke: colors.neutral[700] }, | |
| markerEnd, | |
| label: "created", | |
| }; | |
| }); | |
| export const createEdgesFromDeploymentsToResources = ( | |
| relationships: Relationships, | |
| ) => | |
| relationships.map((resource) => { | |
| const { parent } = resource; | |
| if (parent == null) return null; | |
| const allReleaseJobTriggers = relationships.flatMap((r) => | |
| r.workspace?.systems?.flatMap((s) => | |
| s?.environments?.flatMap((e) => | |
| e?.latestActiveRelease?.map((rel) => rel.releaseJobTrigger) ?? [], | |
| ) ?? [], | |
| ) ?? [] | |
| ) ?? []; | |
| const releaseJobTrigger = allReleaseJobTriggers.find( | |
| (j) => j.jobId === parent.jobId, | |
| ); | |
| if (releaseJobTrigger == null) return null; | |
| return { | |
| id: `${releaseJobTrigger.jobId}-${resource.id}`, | |
| source: releaseJobTrigger.environmentId, | |
| sourceHandle: releaseJobTrigger.jobId, | |
| target: resource.id, | |
| style: { stroke: colors.neutral[700] }, | |
| markerEnd, | |
| label: "created", | |
| }; | |
| }); |
| id: r.id, | ||
| type: NodeType.Resource, | ||
| data: { ...resource, label: resource.identifier }, | ||
| data: { ...r, label: r.identifier }, | ||
| position: { x: 0, y: 0 }, | ||
| }, | ||
| ...systems.flatMap((system) => | ||
| system.environments.map((env) => ({ | ||
| id: env.id, | ||
| type: NodeType.Environment, | ||
| data: { | ||
| environment: { | ||
| ...env, | ||
| deployments: system.deployments, | ||
| resource, | ||
| })), | ||
| ...relationships.flatMap((r) => | ||
| r.provider != null | ||
| ? [ | ||
| { | ||
| id: `${r.provider.id}-${r.id}`, | ||
| type: NodeType.Provider, | ||
| data: { ...r.provider, label: r.provider.name }, | ||
| position: { x: 0, y: 0 }, | ||
| }, | ||
| ] | ||
| : [], | ||
| ), | ||
| ...relationships.flatMap((r) => | ||
| r.workspace.systems.flatMap((s) => | ||
| s.environments.map((e) => ({ | ||
| id: e.id, | ||
| type: NodeType.Environment, | ||
| data: { | ||
| environment: { | ||
| ...e, | ||
| deployments: s.deployments, | ||
| resource: r, | ||
| }, | ||
| label: `${s.name}/${e.name}`, | ||
| }, | ||
| label: `${system.name}/${env.name}`, | ||
| }, | ||
| position: { x: 0, y: 0 }, | ||
| })), | ||
| position: { x: 0, y: 0 }, | ||
| })), |
There was a problem hiding this comment.
Ensure Node IDs are Globally Unique to Prevent Conflicts
The current implementation uses r.id for resource nodes, e.id for environment nodes, and ${r.provider.id}-${r.id} for provider nodes. There is a potential risk of ID collisions if different entities share the same id, which can cause unexpected behavior in the React Flow diagram.
To prevent node ID conflicts, consider prefixing each id with the node type to ensure global uniqueness across all nodes.
Apply this diff to adjust the node IDs:
// For resource nodes
- id: r.id,
+ id: `resource-${r.id}`,
// For provider nodes
- id: `${r.provider.id}-${r.id}`,
+ id: `provider-${r.provider.id}-${r.id}`,
// For environment nodes
- id: e.id,
+ id: `environment-${e.id}`,📝 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.
| id: r.id, | |
| type: NodeType.Resource, | |
| data: { ...resource, label: resource.identifier }, | |
| data: { ...r, label: r.identifier }, | |
| position: { x: 0, y: 0 }, | |
| }, | |
| ...systems.flatMap((system) => | |
| system.environments.map((env) => ({ | |
| id: env.id, | |
| type: NodeType.Environment, | |
| data: { | |
| environment: { | |
| ...env, | |
| deployments: system.deployments, | |
| resource, | |
| })), | |
| ...relationships.flatMap((r) => | |
| r.provider != null | |
| ? [ | |
| { | |
| id: `${r.provider.id}-${r.id}`, | |
| type: NodeType.Provider, | |
| data: { ...r.provider, label: r.provider.name }, | |
| position: { x: 0, y: 0 }, | |
| }, | |
| ] | |
| : [], | |
| ), | |
| ...relationships.flatMap((r) => | |
| r.workspace.systems.flatMap((s) => | |
| s.environments.map((e) => ({ | |
| id: e.id, | |
| type: NodeType.Environment, | |
| data: { | |
| environment: { | |
| ...e, | |
| deployments: s.deployments, | |
| resource: r, | |
| }, | |
| label: `${s.name}/${e.name}`, | |
| }, | |
| label: `${system.name}/${env.name}`, | |
| }, | |
| position: { x: 0, y: 0 }, | |
| })), | |
| position: { x: 0, y: 0 }, | |
| })), | |
| id: `resource-${r.id}`, | |
| type: NodeType.Resource, | |
| data: { ...r, label: r.identifier }, | |
| position: { x: 0, y: 0 }, | |
| })), | |
| ...relationships.flatMap((r) => | |
| r.provider != null | |
| ? [ | |
| { | |
| id: `provider-${r.provider.id}-${r.id}`, | |
| type: NodeType.Provider, | |
| data: { ...r.provider, label: r.provider.name }, | |
| position: { x: 0, y: 0 }, | |
| }, | |
| ] | |
| : [], | |
| ), | |
| ...relationships.flatMap((r) => | |
| r.workspace.systems.flatMap((s) => | |
| s.environments.map((e) => ({ | |
| id: `environment-${e.id}`, | |
| type: NodeType.Environment, | |
| data: { | |
| environment: { | |
| ...e, | |
| deployments: s.deployments, | |
| resource: r, | |
| }, | |
| label: `${s.name}/${e.name}`, | |
| }, | |
| position: { x: 0, y: 0 }, | |
| })), |
| _.range(numDeployments).map((i) => ( | ||
| <Skeleton | ||
| key={i} | ||
| className="h-9 w-full" | ||
| style={{ opacity: 1 * (1 - i / 3) }} | ||
| className="h-14 w-72" | ||
| style={{ opacity: 1 * (1 - i / numDeployments) }} |
There was a problem hiding this comment.
Prevent potential division by zero in opacity calculation
In the opacity calculation on line 114:
style={{ opacity: 1 * (1 - i / numDeployments) }}If numDeployments is zero, this will result in a division by zero error. Ensure that numDeployments is greater than zero before performing this calculation.
Suggest modifying the code to handle the case when numDeployments is zero:
{latestActiveReleasesQ.isLoading &&
_.range(numDeployments || 1).map((i) => (
<Skeleton
key={i}
className="h-14 w-72"
- style={{ opacity: 1 * (1 - i / numDeployments) }}
+ style={{ opacity: numDeployments > 0 ? 1 * (1 - i / numDeployments) : 1 }}
/>
))}| const latestActiveRelease = data.environment.latestActiveRelease.find( | ||
| (r) => r.releaseJobTrigger.release.deploymentId === deployment.id, | ||
| ); |
There was a problem hiding this comment.
Fix potential undefined property access on latestActiveRelease
On lines 150-152, you're accessing data.environment.latestActiveRelease:
const latestActiveRelease = data.environment.latestActiveRelease.find(
(r) => r.releaseJobTrigger.release.deploymentId === deployment.id,
);However, latestActiveRelease may not be a property of data.environment. Earlier in the code, you use latestActiveReleases, which suggests that the latest active releases are obtained via the query result. To prevent potential undefined property access or inconsistency, ensure that data.environment.latestActiveRelease is correctly defined or replace it with latestActiveReleases.
Suggest updating the code to use latestActiveReleases:
- const latestActiveRelease = data.environment.latestActiveRelease.find(
+ const latestActiveRelease = latestActiveReleases.find(
(r) => r.releaseJobTrigger.release.deploymentId === deployment.id,
);| "h-2 w-2 rounded-full border border-neutral-800 bg-neutral-800", | ||
| isInProgress && "border-blue-500", | ||
| isPending && "border-neutral-500", | ||
| isCompleted && "border-green-500", | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use deployment-specific job status for Handle styling
The styling of the Handle component on lines 163-167 uses isInProgress, isPending, and isCompleted variables that are calculated based on all deployments' latest active releases. When rendering Handles for individual deployments, it's more appropriate to base the styling on the status of each specific deployment's job. This ensures the visual feedback accurately reflects the state of each deployment.
Suggest calculating the status for each deployment:
+ const deploymentLatestActiveRelease = latestActiveReleases.find(
+ (r) => r.releaseJobTrigger.release.deploymentId === deployment.id,
+ );
+
+ const deploymentJobStatus = deploymentLatestActiveRelease?.releaseJobTrigger.job.status;
...
<Handle
key={deployment.id}
id={deploymentLatestActiveRelease?.releaseJobTrigger.job.id}
type="source"
className={cn(
"h-2 w-2 rounded-full border border-neutral-800 bg-neutral-800",
- isInProgress && "border-blue-500",
- isPending && "border-neutral-500",
- isCompleted && "border-green-500",
+ deploymentJobStatus === JobStatus.InProgress && "border-blue-500",
+ deploymentJobStatus === JobStatus.Pending && "border-neutral-500",
+ deploymentJobStatus === JobStatus.Completed && "border-green-500",
)}
position={Position.Right}
style={{ top: topOffset }}
/>Committable suggestion skipped: line range outside the PR's diff.
packages/api/src/router/resources.ts
Outdated
| const { systems } = resource.workspace; | ||
| const systemsWithResource = await _.chain( | ||
| systems.map(async (s) => | ||
| _.chain(s.environments) | ||
| .filter((e) => isPresent(e.resourceFilter)) | ||
| .map((e) => | ||
| environmentHasResource(db, resource.id, e.resourceFilter!).then( | ||
| async (t) => | ||
| t | ||
| ? { | ||
| ...e, | ||
| resource, | ||
| latestActiveRelease: | ||
| await latestActiveReleaseByResourceAndEnvironmentId( | ||
| db, | ||
| resource.id, | ||
| e.id, | ||
| ), | ||
| } | ||
| : null, | ||
| ), | ||
| ) | ||
| .thru((promises) => Promise.all(promises)) | ||
| .thru((results) => { | ||
| return results; | ||
| }) | ||
| .value() | ||
| .then((t) => t.filter(isPresent)) | ||
| .then((t) => (t.length > 0 ? { ...s, environments: t } : null)), | ||
| ), | ||
| ) | ||
| .thru((promises) => Promise.all(promises)) | ||
| .value() | ||
| .then((t) => t.filter(isPresent)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize resource filtering logic to improve performance
The getNodeDataForResource function performs environmentHasResource checks within nested loops over systems and environments, which may result in a large number of database queries, potentially impacting performance. Consider optimizing this logic to reduce the number of queries, such as batching queries or restructuring the data retrieval process.
packages/api/src/router/resources.ts
Outdated
| const getNodesRecursivelyHelper = async ( | ||
| db: Tx, | ||
| node: Node, | ||
| nodes: NonNullable<Node>[], | ||
| ): Promise<NonNullable<Node>[]> => { | ||
| if (node == null) return nodes; | ||
| const activeReleaseJobs = node.workspace.systems.flatMap((s) => | ||
| s.environments.flatMap((e) => | ||
| e.latestActiveRelease.map((r) => r.releaseJobTrigger.job), | ||
| ), | ||
| ); | ||
|
|
||
| const relationships = await db.query.jobResourceRelationship.findMany({ | ||
| where: inArray( | ||
| schema.jobResourceRelationship.jobId, | ||
| activeReleaseJobs.map((j) => j.id), | ||
| ), | ||
| with: { resource: true }, | ||
| }); | ||
|
|
||
| const childrenPromises = relationships.map((r) => | ||
| getNodeDataForResource(db, r.resource.id), | ||
| ); | ||
| const children = await Promise.all(childrenPromises); | ||
|
|
||
| const childrenNodesPromises = children.map((c) => | ||
| getNodesRecursivelyHelper(db, c, []), | ||
| ); | ||
| const childrenNodes = (await Promise.all(childrenNodesPromises)).flat(); | ||
| return [...nodes, node, ...childrenNodes].filter(isPresent); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Add cycle detection to prevent infinite recursion in getNodesRecursivelyHelper
The recursive function getNodesRecursivelyHelper may encounter cycles in resource relationships, leading to infinite recursion and stack overflow errors. Consider implementing cycle detection by maintaining a set of visited node IDs to prevent revisiting the same nodes.
Apply this diff to implement cycle detection:
const getNodesRecursivelyHelper = async (
db: Tx,
node: Node,
- nodes: NonNullable<Node>[],
+ visitedNodeIds: Set<string>,
): Promise<NonNullable<Node>[]> => {
- if (node == null) return nodes;
+ if (node == null || visitedNodeIds.has(node.id)) return [];
+ visitedNodeIds.add(node.id);
const activeReleaseJobs = node.workspace.systems.flatMap((s) =>
s.environments.flatMap((e) =>
e.latestActiveRelease.map((r) => r.releaseJobTrigger.job),
),
);
// ... rest of the function ...
- return [...nodes, node, ...childrenNodes].filter(isPresent);
+ return [node, ...childrenNodes].filter(isPresent);
};Committable suggestion skipped: line range outside the PR's diff.
| })) | ||
| .value(); | ||
|
|
||
| const getDeploymentNodes = (relationships: Relationships) => |
There was a problem hiding this comment.
doing nested flatmap here because we need to carry through the resource, and environment, so chaining means I have to pass the extra stuff through the chain so it beomes less readavble
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (2)
9-11: Consider breaking down the complex type definitionWhile using
RouterOutputsis good practice, the deep nested type extraction could be fragile. Consider creating intermediate type aliases for better maintainability.+type SystemEnvironment = RouterOutputs["resource"]["relationships"][number]["workspace"]["systems"][number]; + type Environment = NonNullable< - RouterOutputs["resource"]["relationships"][number] ->["workspace"]["systems"][number]["environments"][number]; + SystemEnvironment["environments"][number] +>;
22-28: Enhance accessibilityConsider adding ARIA attributes to improve accessibility:
-<div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3"> +<div + role="group" + aria-label={`Environment: ${data.label}`} + className="relative flex w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3" +> <div className="flex items-center gap-2"> - <IconPlant className="h-4 w-4 text-green-500" /> + <IconPlant className="h-4 w-4 text-green-500" aria-hidden="true" /> <span className="text-xs">Environment</span> </div> <div className="text-sm">{data.label}</div> </div>apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (2)
26-26: Consider maintaining visual distinction between provider types.While the simplified styling improves consistency, consider using a subtle visual indicator (like an icon or badge) to help users quickly distinguish between provider types without relying on text color.
- if (google != null) return <span className="text-xs ">Google Provider</span>; + if (google != null) return <span className="text-xs flex items-center gap-1"><span className="inline-flex items-center rounded-full bg-red-500/10 px-2 py-0.5 text-xs font-medium text-red-500">G</span>Google Provider</span>;
34-34: Consider using flexible width constraints.The fixed width of 250px might truncate longer provider names or labels. Consider using min/max width constraints for better responsiveness.
- <div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3"> + <div className="relative flex min-w-[250px] max-w-[350px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3">apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts (2)
13-18: Consider adding JSDoc documentation for the NodeType enum.Adding documentation would help explain the purpose and use case of each node type in the visualization context.
+/** + * Represents the different types of nodes that can be rendered in the visualization. + * @enum {string} + */ enum NodeType { + /** Represents a resource node in the system */ Resource = "resource", + /** Represents an environment where resources are deployed */ Environment = "environment", + /** Represents a provider that manages resources */ Provider = "provider", + /** Represents a deployment of resources */ Deployment = "deployment", }
1-86: Consider performance optimizations and layout algorithms.For better scalability and performance:
- Consider memoizing the node generation functions using React.useMemo or lodash.memoize to prevent unnecessary recalculations.
- Implement a proper layout algorithm (e.g., dagre or elk) to handle node positioning automatically.
- Consider implementing virtualization for large graphs to improve rendering performance.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx (1)
26-30: Consider optimizing the data fetching strategy.The 5-second refetch interval might be aggressive and could impact performance:
- With many concurrent users, this could generate significant server load
- Consider implementing websockets for real-time updates instead
- Add error handling for the query
Consider this alternative approach:
const latestActiveReleasesQ = api.resource.activeReleases.byResourceAndEnvironmentId.useQuery( { resourceId, environmentId }, - { refetchInterval: 5_000 }, + { + refetchInterval: 5_000, + retry: 3, + onError: (error) => { + console.error('Failed to fetch active releases:', error); + // Add error handling logic here + } + }, );packages/api/src/router/resources.ts (1)
333-402: Consider adding indexes to optimize query performanceThe query involves multiple joins and a window function for ranking. Consider adding indexes on frequently joined columns to improve query performance:
release.deploymentIdrelease.createdAtenvironment.systemIdreleaseJobTrigger.releaseIdreleaseJobTrigger.environmentId
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx(1 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/ProviderNode.tsx(2 hunks)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(3 hunks)
🔇 Additional comments (24)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (2)
13-16: LGTM! Well-structured props type definition
The props type is clean, properly extends NodeProps, and clearly defines the expected data shape.
22-28: LGTM! Clean and well-structured component implementation
The component has a clear visual hierarchy and consistent styling.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (4)
30-30: Verify the impact of changing layout direction
The layout direction has been changed from left-to-right ("LR") to top-to-bottom ("TB"). While this might improve visualization, it represents a significant visual change that could affect users' experience.
Consider:
- Adding this change to the changelog
- Documenting the rationale for this change
- Potentially making the direction configurable via props
25-25: Previous concern: Node ID uniqueness still needs to be addressed
The previous review raised a valid concern about ensuring node IDs are globally unique. This issue should still be addressed in the getNodes implementation to prevent potential conflicts in the React Flow diagram.
12-13: Verify the implementation of imported visualization logic
The separation of edge and node logic into dedicated files is a good architectural decision. However, we should verify the implementation of these critical visualization functions.
✅ Verification successful
Imports are properly implemented and functional
The verification confirms that both imported modules are well-implemented:
edges.tsprovides theedgeTypesandgetEdgesfunctions with proper edge creation logic for different relationship typesnodes/nodes.tsexportsnodeTypesandgetNodeswith comprehensive node handling for Resources, Environments, Providers, and Deployments
The separation of concerns is well-executed with clear, type-safe implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of edge and node handling functions
# Expected: Find the implementation of getEdges and getNodes functions
# Check for the edge handling implementation
echo "Checking edges.ts implementation:"
fd --type f edges.ts --exec cat {}
# Check for the node handling implementation
echo "Checking nodes.ts implementation:"
fd --type f "nodes/nodes.ts" --exec cat {}
Length of output: 7595
Script:
#!/bin/bash
# Let's check for nodes.ts in a different location since the previous search didn't find it
echo "Checking for nodes.ts file:"
fd nodes.ts
# Let's also check the content of any nodes.ts files found
echo -e "\nContent of nodes.ts files:"
fd nodes.ts --exec cat {}
# Let's also check for nodeTypes export specifically
echo -e "\nChecking for nodeTypes exports:"
rg "export.*nodeTypes" -A 5
Length of output: 4341
16-19: Breaking Change: Verify impact of removing resource prop
The removal of the resource prop in favor of only using relationships is a breaking change. While this simplifies the interface, it may affect existing consumers of this component.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (2)
Line range hint 1-53: Overall changes look good!
The modifications successfully update the visual structure while maintaining component functionality. The changes align well with the PR's objective of improving resource visualization.
43-44: Verify handle positions across all node types.
The change from horizontal (Left/Right) to vertical (Top/Bottom) handle positions represents a significant layout change. Ensure this is consistent with other node types in the visualization to maintain a coherent flow direction.
Also applies to: 48-49
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (3)
18-20: Clean up commented code and clarify resource type handling
Instead of commenting out the resource type detection, we should either:
- Remove it completely if it's no longer needed as part of the relationship-based visualization
- Keep it if resource type distinction is still valuable for users
Let's check if these resource types are still relevant elsewhere:
✅ Verification successful
The search results show that resource type checks are actively used in multiple components for visual styling and icons:
getBorderColor.tsx- determines border colors based on resource typesTargetNode.tsx- uses these checks for node styling in relationships viewTargetIcon.tsx- selects appropriate icons based on resource type
Let's verify if the ResourceNode component still uses any visual styling that would require these checks:
Remove commented code as resource-specific styling has been intentionally removed
The code review shows that all resource type-specific styling in ResourceNode has been intentionally removed, while keeping the base neutral styling. The component still handles resource type visualization through the ResourceIcon component, which maintains the type-specific icons. This appears to be a deliberate design change to standardize node appearance in the visualization while preserving type identification through icons.
- Remove commented type checks (lines 18-20)
- Remove commented styling conditions in the
classNameprops (lines 26-29, 42-45, 52-55)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar resource type checks in the codebase
rg -A 2 "includes\(\"kubernetes\"\)|includes\(\"terraform\"\)|includes\(\"sharedcluster\"\)"
Length of output: 3020
Script:
#!/bin/bash
# Check ResourceNode.tsx for any remaining visual styling based on resource types
cat "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx"
Length of output: 2085
26-29: Consider UX implications of removing visual resource type distinctions
The removal of resource type-specific styling (kubernetes/blue, terraform/purple) might make it harder for users to quickly identify different resource types in the visualization.
Consider:
- Maintaining visual distinction through alternative means (e.g., icons, labels)
- Documenting the rationale for removing visual type distinctions
- Getting user feedback on this change
Let's check for any design system guidelines or existing color schemes:
42-57: Verify impact of Handle position changes on existing visualizations
The change from Left/Right to Top/Bottom handle positions represents a significant layout change that could affect:
- Existing resource relationship visualizations
- Flow direction and readability
- Overall diagram layout and space utilization
Let's check for related layout configurations:
Consider:
- Documenting the rationale for the vertical flow layout
- Ensuring consistent handle positions across all node types
- Validating that this change aligns with any existing layout guidelines
✅ Verification successful
Let me analyze the search results. The codebase shows consistent usage of vertical (Top/Bottom) handle positions in visualization components under the targets/[targetId]/visualize/nodes/ directory, including:
- ResourceNode.tsx
- ProviderNode.tsx
- EnvironmentNode.tsx
- DeploymentNode.tsx
However, I notice that there are different handle position patterns in other parts of the codebase:
- Left/Right positions in deployment flows (ReleaseSequencingNode, FlowPolicyNode, etc.)
- Top/Bottom positions in environment flows and target visualizations
Let me verify if there's any layout configuration that dictates these different patterns:
Let me try a different search pattern to find layout configurations:
Based on the search results, I can now provide a definitive response about the handle position changes:
Handle position changes align with the diagram's layout direction
The change from Left/Right to Top/Bottom handle positions in ResourceNode is correct and consistent with the intended visualization pattern:
- Target/Resource visualizations use Top/Bottom handles with "TB" (top-to-bottom) layout direction, as seen in the same directory
- Different diagram types in the codebase intentionally use different layouts:
- Deployment flows: "LR" (left-to-right) with Left/Right handles
- Environment flows: "TB" (top-to-bottom) with Top/Bottom handles
- Target visualizations: "TB" (top-to-bottom) with Top/Bottom handles
- Relationship diagrams: "BT" (bottom-to-top) with Top/Bottom handles
The handle position change is part of a consistent pattern where handle positions match the diagram's layout direction, making the visualization more intuitive and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using Position.Left/Right
ast-grep --pattern 'Position.$_' | rg "Position\.(Left|Right|Top|Bottom)"
# Look for layout-related configurations
fd -e tsx -e ts --exec rg -A 2 "layout|direction|flow"
Length of output: 41800
Script:
#!/bin/bash
# Search for layout configuration in visualization components
rg -A 3 "getLayoutedElementsDagre|direction.*=.*[\"']" apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/
# Check for any layout-related props or configurations
ast-grep --pattern 'layout = $_ || direction = $_'
Length of output: 323
Script:
#!/bin/bash
# Search for layout configurations in visualization components
rg -A 3 "getLayoutedElementsDagre|direction.*="
# Search for ReactFlow layout configurations
ast-grep --pattern 'getLayoutedElementsDagre($$$)'
Length of output: 17073
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts (3)
1-11: LGTM! Well-structured imports and type definitions.
The imports are appropriate, and the use of NonNullable for the Relationships type ensures type safety.
20-25: LGTM! Complete and well-structured node type mapping.
The nodeTypes mapping correctly associates each NodeType with its corresponding React component.
80-86: Verify the impact of node ordering on visualization layout.
The current implementation combines nodes in a specific order: resources, providers, environments, and deployments. This ordering might affect the final visualization layout.
✅ Verification successful
Node ordering does not impact visualization layout
The visualization uses ReactFlow with a custom useLayoutAndFitView hook that applies a "TB" (top-to-bottom) direction layout using Dagre. The final positioning of nodes is handled by the layout engine, not by the order in which nodes are provided. The getNodes function's ordering is used only for initial collection of nodes and doesn't affect the final visualization layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any layout-related code in the visualization components
rg -l "layout|position|arrange" --type ts --type tsx $(fd -e tsx -e ts . | grep -i "visualiz")
# Check for any custom positioning logic in the node components
ast-grep --pattern 'position = { x: $_, y: $_ }'
Length of output: 1132
Script:
#!/bin/bash
# Check for layout-related code in visualization components with content
rg "layout|position|arrange" --type typescript --type tsx apps/webservice/src/app/[workspaceSlug]/\(app\)/\(targets\)/targets/[targetId]/visualize/ -C 2
# Check if there's any dependency on node order in the diagram component
rg "getNodes" apps/webservice/src/app/[workspaceSlug]/\(app\)/\(targets\)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx -C 3
Length of output: 853
Script:
#!/bin/bash
# Check for layout hook implementation
rg -A 10 "useLayoutAndFitView" apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/reactflow/layout.ts
# Check for node layout configuration in ResourceVisualizationDiagram
rg -A 10 "ReactFlow" apps/webservice/src/app/[workspaceSlug]/\(app\)/\(targets\)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx
# Check if there's any custom layout engine or positioning
rg "layoutEngine|dagre|elkjs" apps/webservice/src/app/[workspaceSlug]/\(app\)/\(targets\)/targets/[targetId]/visualize/
Length of output: 1799
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (5)
1-20: LGTM: Imports and type definitions are well-structured
The imports and type definitions are appropriate for the edge creation functionality.
21-33: LGTM: Clean implementation of resource-to-environment edge creation
The function follows functional programming principles and maintains consistent styling.
67-95:
Add null checks and simplify the chain operation
The nested property access could lead to runtime errors if any property in the chain is null/undefined. Additionally, the chain operation can be simplified.
Apply these improvements:
- const allReleaseJobTriggers = _.chain(relationships)
- .flatMap((r) => r.workspace.systems)
- .flatMap((s) => s.environments)
- .flatMap((e) => e.latestActiveReleases)
- .map((rel) => rel.releaseJobTrigger)
- .value();
+ const allReleaseJobTriggers = _.chain(relationships)
+ .flatMap((r) => r.workspace?.systems ?? [])
+ .flatMap((s) => s?.environments ?? [])
+ .flatMap((e) => e?.latestActiveReleases ?? [])
+ .map((rel) => rel?.releaseJobTrigger)
+ .compact()
+ .value();Also add null checks for the release properties:
- const { deploymentId } = releaseJobTrigger.release;
- const { environmentId } = releaseJobTrigger;
+ const deploymentId = releaseJobTrigger.release?.deploymentId;
+ const environmentId = releaseJobTrigger.environmentId;
+ if (!deploymentId || !environmentId) return null;34-48:
Fix inconsistency between edge id and source properties
The edge's id and source properties use different formats:
id:${provider.id}-${resource.id}source:${provider.id}-${resource.id}
This could cause issues with edge identification or rendering if other parts of the code expect these to match.
Apply this fix:
- id: `${provider.id}-${resource.id}`,
- source: `${provider.id}-${resource.id}`,
+ const compositeId = `${provider.id}-${resource.id}`;
+ id: compositeId,
+ source: compositeId,Likely invalid or redundant comment.
1-120: Verify test coverage for edge creation logic
Given the complexity of the relationship visualization and the recent changes, ensure that there are comprehensive tests covering:
- Edge creation with null/undefined values in the relationship tree
- Different combinations of resources, environments, and deployments
- Edge styling and properties
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx (2)
1-18: LGTM! Well-structured imports and type definitions.
The imports are appropriate and the DeploymentNodeProps type is well-defined with clear typing for all required properties.
1-107: Verify integration with ResourceVisualizationDiagram component.
Please ensure:
- All possible job states are properly handled
- The component integrates correctly with the parent ResourceVisualizationDiagram
✅ Verification successful
DeploymentNode component is properly integrated and handles all job states
The verification shows that:
- The component correctly handles all possible job states from the
JobStatusenum: Completed, Cancelled, Skipped, InProgress, ActionRequired, Pending, Failure, InvalidJobAgent, InvalidIntegration, and ExternalRunNotFound - The component is properly integrated into the ResourceVisualizationDiagram through the
nodeTypesmapping - The component's props interface matches the data structure passed from the parent diagram
- The component implements proper visual feedback for different states through conditional styling
While there's no explicit error boundary, the component's implementation is robust with:
- Null checks for
releaseJobTrigger - Safe access to optional data with fallbacks
- Proper TypeScript typing for all props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration points and job state handling
# Check for all possible job states in the codebase
echo "Checking for all possible job states..."
rg -A 1 "enum.*JobStatus"
# Check how DeploymentNode is used in ResourceVisualizationDiagram
echo "Checking DeploymentNode usage..."
rg -A 5 "DeploymentNode"
# Verify proper error boundaries exist in parent components
echo "Checking for error boundaries..."
rg -A 5 "ErrorBoundary.*DeploymentNode"
Length of output: 6354
Script:
#!/bin/bash
# Let's verify the job states and error handling more thoroughly
# Check complete JobStatus enum definition
echo "Checking complete JobStatus enum..."
rg -A 10 "enum.*JobStatus"
# Check ResourceVisualizationDiagram implementation
echo "Checking ResourceVisualizationDiagram implementation..."
fd "ResourceVisualizationDiagram" --exec cat {}
# Check error handling in the visualization components
echo "Checking error handling in visualization components..."
fd "visualize" --type d --exec rg -l "try|catch|error" {}
# Check if there are any ErrorBoundary components
echo "Checking ErrorBoundary components..."
rg -l "ErrorBoundary"
Length of output: 2823
packages/api/src/router/resources.ts (3)
514-517: LGTM!
The wrapper function is well-structured and correctly initializes the recursive traversal.
562-568: LGTM!
The router endpoints are properly updated to use the new functions while maintaining authorization checks.
Also applies to: 579-579
482-512:
Add cycle detection to prevent infinite recursion
The recursive traversal lacks cycle detection, which could lead to infinite recursion if there are circular relationships between resources.
Apply this diff to implement cycle detection:
const getNodesRecursivelyHelper = async (
db: Tx,
node: Node,
- nodes: NonNullable<Node>[],
+ nodes: NonNullable<Node>[],
+ visited: Set<string> = new Set(),
): Promise<NonNullable<Node>[]> => {
- if (node == null) return nodes;
+ if (node == null || visited.has(node.id)) return nodes;
+ visited.add(node.id);
const activeReleaseJobs = _.chain(node.workspace.systems)
.flatMap((s) => s.environments)
.flatMap((e) => e.latestActiveReleases)
.map((r) => r.releaseJobTrigger.job)
.value();
const relationships = await db.query.jobResourceRelationship.findMany({
where: inArray(
schema.jobResourceRelationship.jobId,
activeReleaseJobs.map((j) => j.id),
),
with: { resource: true },
});
const childrenPromises = relationships.map((r) =>
getNodeDataForResource(db, r.resource.id),
);
const children = await Promise.all(childrenPromises);
const childrenNodesPromises = children.map((c) =>
- getNodesRecursivelyHelper(db, c, []),
+ getNodesRecursivelyHelper(db, c, [], visited),
);
const childrenNodes = (await Promise.all(childrenNodesPromises)).flat();
return [...nodes, node, ...childrenNodes].filter(isPresent);
};| const getResourceNodes = (relationships: Relationships) => | ||
| relationships.map((r) => ({ | ||
| id: r.id, | ||
| type: NodeType.Resource, | ||
| data: { ...r, label: r.identifier }, | ||
| position: { x: 0, y: 0 }, | ||
| })); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving node positioning and error handling.
Several potential improvements for the node generation functions:
-
Hardcoded positions:
The current implementation sets all node positions to {x: 0, y: 0}, which might cause nodes to overlap. Consider implementing a layout algorithm or accepting positions as parameters. -
Complex environment nodes logic:
The lodash chain in getEnvironmentNodes could be simplified for better readability. -
Missing error handling:
Add validation for required fields and handle potential undefined values.
Here's a suggested improvement for the environment nodes function:
const getEnvironmentNodes = (relationships: Relationships) =>
- _.chain(relationships)
- .flatMap((r) => r.workspace.systems)
- .flatMap((s) => s.environments.map((e) => ({ s, e })))
- .map(({ s, e }) => ({
- id: e.id,
- type: NodeType.Environment,
- data: { environment: e, label: `${s.name}/${e.name}` },
- position: { x: 0, y: 0 },
- }))
- .value();
+ relationships.flatMap((r) => {
+ if (!r.workspace?.systems) return [];
+
+ return r.workspace.systems.flatMap((system) => {
+ if (!system.environments?.length) return [];
+
+ return system.environments.map((env) => ({
+ id: env.id,
+ type: NodeType.Environment,
+ data: {
+ environment: env,
+ label: `${system.name}/${env.name}`,
+ },
+ // TODO: Implement proper positioning logic
+ position: { x: 0, y: 0 },
+ }));
+ });
+ });Also applies to: 35-48, 49-60, 61-78
| export const getEdges = (relationships: Relationships) => { | ||
| const resourceToEnvEdges = relationships.flatMap((r) => | ||
| createEdgesFromResourceToEnvironments( | ||
| r, | ||
| r.workspace.systems.flatMap((s) => s.environments), | ||
| ), | ||
| ); | ||
| const environmentToDeploymentEdges = relationships.flatMap((r) => | ||
| r.workspace.systems.flatMap((s) => | ||
| createEdgesFromEnvironmentToDeployments(s.environments, s.deployments), | ||
| ), | ||
| ); | ||
| const providerEdges = relationships.flatMap((r) => | ||
| r.provider != null ? [createEdgeFromProviderToResource(r.provider, r)] : [], | ||
| ); | ||
| const deploymentEdges = createEdgesFromDeploymentsToResources(relationships); | ||
|
|
||
| return _.compact([ | ||
| ...resourceToEnvEdges, | ||
| ...environmentToDeploymentEdges, | ||
| ...providerEdges, | ||
| ...deploymentEdges, | ||
| ]); | ||
| }; |
There was a problem hiding this comment.
Add null checks to prevent runtime errors
The flatMap operations in the getEdges function could throw runtime errors if nested properties are undefined.
Apply these improvements:
const resourceToEnvEdges = relationships.flatMap((r) =>
createEdgesFromResourceToEnvironments(
r,
- r.workspace.systems.flatMap((s) => s.environments),
+ r.workspace?.systems?.flatMap((s) => s?.environments ?? []) ?? [],
),
);
const environmentToDeploymentEdges = relationships.flatMap((r) =>
- r.workspace.systems.flatMap((s) =>
+ r.workspace?.systems?.flatMap((s) =>
createEdgesFromEnvironmentToDeployments(s.environments, s.deployments),
),
);Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <> | ||
| <div | ||
| className={cn( | ||
| "relative flex w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-1", | ||
| isInProgress && "border-blue-500", | ||
| isPending && "border-neutral-500", | ||
| isCompleted && "border-green-500", | ||
| releaseJobTrigger == null && "border-neutral-800", | ||
| releaseJobTrigger != null && "cursor-pointer", | ||
| )} | ||
| onClick={() => { | ||
| if (releaseJobTrigger != null) setJobId(releaseJobTrigger.job.id); | ||
| }} | ||
| > | ||
| <div className="flex h-14 items-center"> | ||
| <div className="flex min-w-0 flex-1 flex-grow items-center gap-2"> | ||
| <span className="truncate">{deployment.name}</span> | ||
| </div> | ||
| {releaseJobTrigger != null && ( | ||
| <div className="flex flex-shrink-0 items-center gap-2"> | ||
| <ReleaseIcon job={releaseJobTrigger.job} /> | ||
| <div className="text-sm"> | ||
| <div>{releaseJobTrigger.release.version}</div> | ||
| <div>{JobStatusReadable[releaseJobTrigger.job.status]}</div> | ||
| </div> | ||
| </div> | ||
| )} | ||
| {releaseJobTrigger == null && ( | ||
| <div className="flex flex-shrink-0 justify-end pr-4 text-sm text-muted-foreground"> | ||
| No active job | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| <Handle | ||
| type="target" | ||
| className={cn( | ||
| "h-2 w-2 rounded-full border border-neutral-800 bg-neutral-800", | ||
| isInProgress && "border-blue-500", | ||
| isPending && "border-neutral-500", | ||
| isCompleted && "border-green-500", | ||
| releaseJobTrigger == null && "border-neutral-800", | ||
| )} | ||
| position={Position.Top} | ||
| /> | ||
| <Handle | ||
| type="source" | ||
| position={Position.Bottom} | ||
| className={cn( | ||
| "h-2 w-2 rounded-full border border-neutral-800 bg-neutral-800", | ||
| isInProgress && "border-blue-500", | ||
| isPending && "border-neutral-500", | ||
| isCompleted && "border-green-500", | ||
| releaseJobTrigger == null && "border-neutral-800", | ||
| )} | ||
| /> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance render logic with better code organization and accessibility.
Several improvements could be made to the render logic:
- Duplicate styling logic between div and handles
- Missing accessibility attributes
- Click handler could be extracted
Consider these improvements:
+ const getStatusStyles = (status: {
+ isInProgress: boolean;
+ isPending: boolean;
+ isCompleted: boolean;
+ hasJobTrigger: boolean;
+ }) => cn(
+ "border-neutral-800",
+ status.isInProgress && "border-blue-500",
+ status.isPending && "border-neutral-500",
+ status.isCompleted && "border-green-500",
+ !status.hasJobTrigger && "border-neutral-800",
+ );
+ const handleJobClick = (jobId: string | undefined) => {
+ if (jobId != null) setJobId(jobId);
+ };
return (
<>
<div
+ role="button"
+ tabIndex={releaseJobTrigger ? 0 : -1}
+ aria-label={`Deployment: ${deployment.name}`}
className={cn(
"relative flex w-[250px] flex-col gap-2 rounded-md border bg-neutral-900/30 px-4 py-1",
- isInProgress && "border-blue-500",
- isPending && "border-neutral-500",
- isCompleted && "border-green-500",
- releaseJobTrigger == null && "border-neutral-800",
+ getStatusStyles({
+ isInProgress,
+ isPending,
+ isCompleted,
+ hasJobTrigger: releaseJobTrigger != null
+ }),
releaseJobTrigger != null && "cursor-pointer",
)}
- onClick={() => {
- if (releaseJobTrigger != null) setJobId(releaseJobTrigger.job.id);
- }}
+ onClick={() => handleJobClick(releaseJobTrigger?.job.id)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ handleJobClick(releaseJobTrigger?.job.id);
+ }
+ }}
>Committable suggestion skipped: line range outside the PR's diff.
| const isInProgress = latestActiveReleases.some( | ||
| (r) => r.releaseJobTrigger.job.status === JobStatus.InProgress, | ||
| ); | ||
| const isPending = latestActiveReleases.some( | ||
| (r) => r.releaseJobTrigger.job.status === JobStatus.Pending, | ||
| ); | ||
| const isCompleted = latestActiveReleases.every( | ||
| (r) => r.releaseJobTrigger.job.status === JobStatus.Completed, | ||
| ); |
There was a problem hiding this comment.
Improve status computation logic robustness.
The current status computation has several potential issues:
- Status checks are not mutually exclusive
- Missing handling for failed/error states
- The order of checks could affect status determination
Consider this more robust approach:
- const isInProgress = latestActiveReleases.some(
- (r) => r.releaseJobTrigger.job.status === JobStatus.InProgress,
- );
- const isPending = latestActiveReleases.some(
- (r) => r.releaseJobTrigger.job.status === JobStatus.Pending,
- );
- const isCompleted = latestActiveReleases.every(
- (r) => r.releaseJobTrigger.job.status === JobStatus.Completed,
- );
+ const getDeploymentStatus = (releases: typeof latestActiveReleases) => {
+ if (releases.length === 0) return 'none';
+ if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.Failed)) {
+ return 'failed';
+ }
+ if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.InProgress)) {
+ return 'in_progress';
+ }
+ if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.Pending)) {
+ return 'pending';
+ }
+ if (releases.every(r => r.releaseJobTrigger.job.status === JobStatus.Completed)) {
+ return 'completed';
+ }
+ return 'unknown';
+ };
+
+ const status = getDeploymentStatus(latestActiveReleases);
+ const isInProgress = status === 'in_progress';
+ const isPending = status === 'pending';
+ const isCompleted = status === 'completed';
+ const isFailed = status === 'failed';📝 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 isInProgress = latestActiveReleases.some( | |
| (r) => r.releaseJobTrigger.job.status === JobStatus.InProgress, | |
| ); | |
| const isPending = latestActiveReleases.some( | |
| (r) => r.releaseJobTrigger.job.status === JobStatus.Pending, | |
| ); | |
| const isCompleted = latestActiveReleases.every( | |
| (r) => r.releaseJobTrigger.job.status === JobStatus.Completed, | |
| ); | |
| const getDeploymentStatus = (releases: typeof latestActiveReleases) => { | |
| if (releases.length === 0) return 'none'; | |
| if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.Failed)) { | |
| return 'failed'; | |
| } | |
| if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.InProgress)) { | |
| return 'in_progress'; | |
| } | |
| if (releases.some(r => r.releaseJobTrigger.job.status === JobStatus.Pending)) { | |
| return 'pending'; | |
| } | |
| if (releases.every(r => r.releaseJobTrigger.job.status === JobStatus.Completed)) { | |
| return 'completed'; | |
| } | |
| return 'unknown'; | |
| }; | |
| const status = getDeploymentStatus(latestActiveReleases); | |
| const isInProgress = status === 'in_progress'; | |
| const isPending = status === 'pending'; | |
| const isCompleted = status === 'completed'; | |
| const isFailed = status === 'failed'; |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (1)
21-47: Add return type annotations for better type safetyThe edge creation utility functions would benefit from explicit return type annotations to ensure type safety and improve code maintainability.
Apply these improvements:
const createEdgesFromResourceToEnvironments = ( resource: SCHEMA.Resource, environments: SCHEMA.Environment[], + ): Edge[] => environments.map((environment) => ({ id: `${resource.id}-${environment.id}`, source: resource.id, target: environment.id, style: { stroke: colors.neutral[800] }, markerEnd, label: "in", })); const createEdgeFromProviderToResource = ( provider: Provider | null, resource: SCHEMA.Resource, + ): Edge | null =>packages/api/src/router/resources.ts (1)
333-402: Consider adding index hints and error handlingThe complex query with multiple joins could benefit from performance optimization and better error handling.
Consider adding index hints for frequently queried columns:
release.deploymentIdrelease.createdAtenvironment.systemIdreleaseJobTrigger.releaseIdreleaseJobTrigger.environmentIdresource.idAdd error handling for edge cases where:
- The resource doesn't exist
- The environment doesn't exist
- No releases exist
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts(1 hunks)packages/api/src/router/resources.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/nodes.ts
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts (1)
1-118: Verify edge creation for all possible scenarios
Let's verify that all edge cases are handled correctly by checking the usage patterns in the codebase.
✅ Verification successful
Edge creation logic is correctly implemented and consistently used
The verification shows that:
- The edge creation functions are used exclusively for resource visualization in the target view
- The
DepEdgecomponent is consistently used across different diagram types - All edge creation functions handle null cases appropriately
- The edge types and styling are consistent across the codebase
The implementation follows the established patterns in the codebase and properly handles all edge cases, including null providers and missing relationships.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for edge creation patterns and potential edge cases
# Check for places where edges are created
echo "Checking edge creation patterns..."
rg -A 5 "createEdge" --type typescript
# Check for places where getEdges is used
echo "Checking getEdges usage..."
rg -A 5 "getEdges" --type typescript
# Check for potential null access patterns
echo "Checking null access patterns..."
ast-grep --pattern 'flatMap((\_) => \_.workspace.systems)' --lang typescript
Length of output: 441
Script:
#!/bin/bash
# Let's try with correct ripgrep patterns without typescript file type specification
# Check for edge creation patterns
echo "Checking edge creation patterns..."
rg "createEdge"
# Check for getEdges usage
echo "Checking getEdges usage..."
rg "getEdges"
# Check for places where edges are used in visualization
echo "Checking visualization edge usage..."
rg -A 3 "edges.*reactflow|reactflow.*edges"
# Check for DepEdge component usage
echo "Checking DepEdge usage..."
rg "DepEdge"
Length of output: 11997
packages/api/src/router/resources.ts (1)
561-567: LGTM: Router endpoints correctly utilize new functions
The changes to activeReleases.byResourceAndEnvironmentId and relationships endpoints properly integrate with the newly added functions.
Also applies to: 578-578
| const allReleaseJobTriggers = relationships | ||
| .flatMap((r) => r.workspace.systems) | ||
| .flatMap((s) => s.environments) | ||
| .flatMap((e) => e.latestActiveReleases) | ||
| .map((rel) => rel.releaseJobTrigger); |
There was a problem hiding this comment.
Add null checks for nested property access
The flatMap operations could throw runtime errors if any of the nested properties are undefined.
Apply these improvements:
const allReleaseJobTriggers = relationships
- .flatMap((r) => r.workspace.systems)
- .flatMap((s) => s.environments)
- .flatMap((e) => e.latestActiveReleases)
- .map((rel) => rel.releaseJobTrigger);
+ .flatMap((r) => r.workspace?.systems ?? [])
+ .flatMap((s) => s?.environments ?? [])
+ .flatMap((e) => e?.latestActiveReleases ?? [])
+ .map((rel) => rel?.releaseJobTrigger)
+ .filter(isPresent);📝 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 allReleaseJobTriggers = relationships | |
| .flatMap((r) => r.workspace.systems) | |
| .flatMap((s) => s.environments) | |
| .flatMap((e) => e.latestActiveReleases) | |
| .map((rel) => rel.releaseJobTrigger); | |
| const allReleaseJobTriggers = relationships | |
| .flatMap((r) => r.workspace?.systems ?? []) | |
| .flatMap((s) => s?.environments ?? []) | |
| .flatMap((e) => e?.latestActiveReleases ?? []) | |
| .map((rel) => rel?.releaseJobTrigger) | |
| .filter(isPresent); |
| export const getEdges = (relationships: Relationships) => { | ||
| const resourceToEnvEdges = relationships.flatMap((r) => | ||
| createEdgesFromResourceToEnvironments( | ||
| r, | ||
| r.workspace.systems.flatMap((s) => s.environments), | ||
| ), | ||
| ); | ||
| const environmentToDeploymentEdges = relationships.flatMap((r) => | ||
| r.workspace.systems.flatMap((s) => | ||
| createEdgesFromEnvironmentToDeployments(s.environments, s.deployments), | ||
| ), | ||
| ); | ||
| const providerEdges = relationships.flatMap((r) => | ||
| r.provider != null ? [createEdgeFromProviderToResource(r.provider, r)] : [], | ||
| ); | ||
| const deploymentEdges = createEdgesFromDeploymentsToResources(relationships); | ||
|
|
||
| return [ | ||
| ...resourceToEnvEdges, | ||
| ...environmentToDeploymentEdges, | ||
| ...providerEdges, | ||
| ...deploymentEdges, | ||
| ].filter(isPresent); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and optimize edge creation
The edge creation has two issues:
- Unsafe property access in nested flatMap operations
- Multiple iterations over the relationships array
Apply these improvements:
export const getEdges = (relationships: Relationships) => {
+ // Pre-compute systems and environments to avoid multiple iterations
+ const systemsAndEnvs = relationships.flatMap((r) =>
+ r.workspace?.systems?.map(s => ({
+ system: s,
+ environments: s?.environments ?? [],
+ deployments: s?.deployments ?? []
+ })) ?? []
+ );
const resourceToEnvEdges = relationships.flatMap((r) =>
createEdgesFromResourceToEnvironments(
r,
- r.workspace.systems.flatMap((s) => s.environments),
+ systemsAndEnvs.flatMap(({ environments }) => environments),
),
);
const environmentToDeploymentEdges = systemsAndEnvs.flatMap(
- r.workspace.systems.flatMap((s) =>
createEdgesFromEnvironmentToDeployments(
s.environments,
s.deployments
),
),
);
const providerEdges = relationships.flatMap((r) =>
r.provider != null ? [createEdgeFromProviderToResource(r.provider, r)] : [],
);
const deploymentEdges = createEdgesFromDeploymentsToResources(relationships);
return [
...resourceToEnvEdges,
...environmentToDeploymentEdges,
...providerEdges,
...deploymentEdges,
].filter(isPresent);
};Committable suggestion skipped: line range outside the PR's diff.
| const getNodesRecursivelyHelper = async ( | ||
| db: Tx, | ||
| node: Node, | ||
| nodes: NonNullable<Node>[], | ||
| ): Promise<NonNullable<Node>[]> => { | ||
| if (node == null) return nodes; | ||
| const activeReleaseJobs = node.workspace.systems | ||
| .flatMap((s) => s.environments) | ||
| .flatMap((e) => e.latestActiveReleases) | ||
| .map((r) => r.releaseJobTrigger.job); | ||
|
|
||
| const relationships = await db.query.jobResourceRelationship.findMany({ | ||
| where: inArray( | ||
| schema.jobResourceRelationship.jobId, | ||
| activeReleaseJobs.map((j) => j.id), | ||
| ), | ||
| with: { resource: true }, | ||
| }); | ||
|
|
||
| const childrenPromises = relationships.map((r) => | ||
| getNodeDataForResource(db, r.resource.id), | ||
| ); | ||
| const children = await Promise.all(childrenPromises); | ||
|
|
||
| const childrenNodesPromises = children.map((c) => | ||
| getNodesRecursivelyHelper(db, c, []), | ||
| ); | ||
| const childrenNodes = (await Promise.all(childrenNodesPromises)).flat(); | ||
| return [...nodes, node, ...childrenNodes].filter(isPresent); | ||
| }; | ||
|
|
||
| const getNodesRecursively = async (db: Tx, resourceId: string) => { | ||
| const baseNode = await getNodeDataForResource(db, resourceId); | ||
| return getNodesRecursivelyHelper(db, baseNode, []); | ||
| }; |
There was a problem hiding this comment.
Add cycle detection to prevent infinite recursion
The recursive function getNodesRecursivelyHelper may encounter cycles in resource relationships, leading to infinite recursion and stack overflow errors. Consider implementing cycle detection by maintaining a set of visited node IDs.
Apply this diff to implement cycle detection:
const getNodesRecursivelyHelper = async (
db: Tx,
node: Node,
- nodes: NonNullable<Node>[],
+ visitedNodeIds: Set<string>,
): Promise<NonNullable<Node>[]> => {
- if (node == null) return nodes;
+ if (node == null || visitedNodeIds.has(node.id)) return [];
+ visitedNodeIds.add(node.id);
const activeReleaseJobs = node.workspace.systems.flatMap((s) =>
s.environments.flatMap((e) => e.latestActiveReleases)
.map((r) => r.releaseJobTrigger.job),
);
// ... rest of the function ...
- return [...nodes, node, ...childrenNodes].filter(isPresent);
+ return [node, ...childrenNodes].filter(isPresent);
};
const getNodesRecursively = async (db: Tx, resourceId: string) => {
const baseNode = await getNodeDataForResource(db, resourceId);
- return getNodesRecursivelyHelper(db, baseNode, []);
+ return getNodesRecursivelyHelper(db, baseNode, new Set());
};📝 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 getNodesRecursivelyHelper = async ( | |
| db: Tx, | |
| node: Node, | |
| nodes: NonNullable<Node>[], | |
| ): Promise<NonNullable<Node>[]> => { | |
| if (node == null) return nodes; | |
| const activeReleaseJobs = node.workspace.systems | |
| .flatMap((s) => s.environments) | |
| .flatMap((e) => e.latestActiveReleases) | |
| .map((r) => r.releaseJobTrigger.job); | |
| const relationships = await db.query.jobResourceRelationship.findMany({ | |
| where: inArray( | |
| schema.jobResourceRelationship.jobId, | |
| activeReleaseJobs.map((j) => j.id), | |
| ), | |
| with: { resource: true }, | |
| }); | |
| const childrenPromises = relationships.map((r) => | |
| getNodeDataForResource(db, r.resource.id), | |
| ); | |
| const children = await Promise.all(childrenPromises); | |
| const childrenNodesPromises = children.map((c) => | |
| getNodesRecursivelyHelper(db, c, []), | |
| ); | |
| const childrenNodes = (await Promise.all(childrenNodesPromises)).flat(); | |
| return [...nodes, node, ...childrenNodes].filter(isPresent); | |
| }; | |
| const getNodesRecursively = async (db: Tx, resourceId: string) => { | |
| const baseNode = await getNodeDataForResource(db, resourceId); | |
| return getNodesRecursivelyHelper(db, baseNode, []); | |
| }; | |
| const getNodesRecursivelyHelper = async ( | |
| db: Tx, | |
| node: Node, | |
| visitedNodeIds: Set<string>, | |
| ): Promise<NonNullable<Node>[]> => { | |
| if (node == null || visitedNodeIds.has(node.id)) return []; | |
| visitedNodeIds.add(node.id); | |
| const activeReleaseJobs = node.workspace.systems | |
| .flatMap((s) => s.environments) | |
| .flatMap((e) => e.latestActiveReleases) | |
| .map((r) => r.releaseJobTrigger.job); | |
| const relationships = await db.query.jobResourceRelationship.findMany({ | |
| where: inArray( | |
| schema.jobResourceRelationship.jobId, | |
| activeReleaseJobs.map((j) => j.id), | |
| ), | |
| with: { resource: true }, | |
| }); | |
| const childrenPromises = relationships.map((r) => | |
| getNodeDataForResource(db, r.resource.id), | |
| ); | |
| const children = await Promise.all(childrenPromises); | |
| const childrenNodesPromises = children.map((c) => | |
| getNodesRecursivelyHelper(db, c, visitedNodeIds), | |
| ); | |
| const childrenNodes = (await Promise.all(childrenNodesPromises)).flat(); | |
| return [node, ...childrenNodes].filter(isPresent); | |
| }; | |
| const getNodesRecursively = async (db: Tx, resourceId: string) => { | |
| const baseNode = await getNodeDataForResource(db, resourceId); | |
| return getNodesRecursivelyHelper(db, baseNode, new Set()); | |
| }; |
| const getNodeDataForResource = async (db: Tx, resourceId: string) => { | ||
| const hasFilter = isNotNull(schema.environment.resourceFilter); | ||
| const resource = await db.query.resource.findFirst({ | ||
| where: and(eq(schema.resource.id, resourceId), isNotDeleted), | ||
| with: { | ||
| provider: { with: { google: true } }, | ||
| workspace: { | ||
| with: { | ||
| systems: { | ||
| with: { environments: { where: hasFilter }, deployments: true }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| if (resource == null) return null; | ||
|
|
||
| const matchesIdentifier = eq( | ||
| schema.jobResourceRelationship.resourceIdentifier, | ||
| resource.identifier, | ||
| ); | ||
| const parent = await db.query.jobResourceRelationship.findFirst({ | ||
| where: matchesIdentifier, | ||
| }); | ||
|
|
||
| const { systems } = resource.workspace; | ||
| const systemsWithResource = await _.chain( | ||
| systems.map(async (s) => | ||
| _.chain(s.environments) | ||
| .filter((e) => isPresent(e.resourceFilter)) | ||
| .map((e) => | ||
| environmentHasResource(db, resource.id, e.resourceFilter!).then( | ||
| async (t) => | ||
| t | ||
| ? { | ||
| ...e, | ||
| resource, | ||
| latestActiveReleases: | ||
| await latestActiveReleaseByResourceAndEnvironmentId( | ||
| db, | ||
| resource.id, | ||
| e.id, | ||
| ), | ||
| } | ||
| : null, | ||
| ), | ||
| ) | ||
| .thru((promises) => Promise.all(promises)) | ||
| .thru((results) => { | ||
| return results; | ||
| }) | ||
| .value() | ||
| .then((t) => t.filter(isPresent)) | ||
| .then((t) => (t.length > 0 ? { ...s, environments: t } : null)), | ||
| ), | ||
| ) | ||
| .thru((promises) => Promise.all(promises)) | ||
| .value() | ||
| .then((t) => t.filter(isPresent)); | ||
|
|
||
| const provider = | ||
| resource.provider == null | ||
| ? null | ||
| : { | ||
| ...resource.provider, | ||
| google: resource.provider.google[0] ?? null, | ||
| }; | ||
|
|
||
| return { | ||
| ...resource, | ||
| workspace: { ...resource.workspace, systems: systemsWithResource }, | ||
| provider, | ||
| parent: parent ?? null, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify promise chaining and handle edge cases
- The nested promise chains in
systemsWithResourcecould be simplified using async/await. - The Google provider access at line 469 assumes array index 0 exists without checking array length.
Consider this refactor:
- const systemsWithResource = await _.chain(
- systems.map(async (s) =>
- _.chain(s.environments)
+ const systemsWithResource = await Promise.all(
+ systems.map(async (system) => {
+ const validEnvironments = await Promise.all(
+ system.environments
+ .filter((env) => isPresent(env.resourceFilter))
+ .map(async (env) => {
+ const hasResource = await environmentHasResource(
+ db,
+ resource.id,
+ env.resourceFilter!
+ );
+ if (!hasResource) return null;
+
+ return {
+ ...env,
+ resource,
+ latestActiveReleases: await latestActiveReleaseByResourceAndEnvironmentId(
+ db,
+ resource.id,
+ env.id
+ )
+ };
+ })
+ );
+
+ const filteredEnvironments = validEnvironments.filter(isPresent);
+ return filteredEnvironments.length > 0
+ ? { ...system, environments: filteredEnvironments }
+ : null;
+ })
+ );
- const provider =
- resource.provider == null
- ? null
- : {
- ...resource.provider,
- google: resource.provider.google[0] ?? null,
- };
+ const provider = resource.provider && {
+ ...resource.provider,
+ google: resource.provider.google?.[0] ?? null
+ };📝 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 getNodeDataForResource = async (db: Tx, resourceId: string) => { | |
| const hasFilter = isNotNull(schema.environment.resourceFilter); | |
| const resource = await db.query.resource.findFirst({ | |
| where: and(eq(schema.resource.id, resourceId), isNotDeleted), | |
| with: { | |
| provider: { with: { google: true } }, | |
| workspace: { | |
| with: { | |
| systems: { | |
| with: { environments: { where: hasFilter }, deployments: true }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| if (resource == null) return null; | |
| const matchesIdentifier = eq( | |
| schema.jobResourceRelationship.resourceIdentifier, | |
| resource.identifier, | |
| ); | |
| const parent = await db.query.jobResourceRelationship.findFirst({ | |
| where: matchesIdentifier, | |
| }); | |
| const { systems } = resource.workspace; | |
| const systemsWithResource = await _.chain( | |
| systems.map(async (s) => | |
| _.chain(s.environments) | |
| .filter((e) => isPresent(e.resourceFilter)) | |
| .map((e) => | |
| environmentHasResource(db, resource.id, e.resourceFilter!).then( | |
| async (t) => | |
| t | |
| ? { | |
| ...e, | |
| resource, | |
| latestActiveReleases: | |
| await latestActiveReleaseByResourceAndEnvironmentId( | |
| db, | |
| resource.id, | |
| e.id, | |
| ), | |
| } | |
| : null, | |
| ), | |
| ) | |
| .thru((promises) => Promise.all(promises)) | |
| .thru((results) => { | |
| return results; | |
| }) | |
| .value() | |
| .then((t) => t.filter(isPresent)) | |
| .then((t) => (t.length > 0 ? { ...s, environments: t } : null)), | |
| ), | |
| ) | |
| .thru((promises) => Promise.all(promises)) | |
| .value() | |
| .then((t) => t.filter(isPresent)); | |
| const provider = | |
| resource.provider == null | |
| ? null | |
| : { | |
| ...resource.provider, | |
| google: resource.provider.google[0] ?? null, | |
| }; | |
| return { | |
| ...resource, | |
| workspace: { ...resource.workspace, systems: systemsWithResource }, | |
| provider, | |
| parent: parent ?? null, | |
| }; | |
| }; | |
| const getNodeDataForResource = async (db: Tx, resourceId: string) => { | |
| const hasFilter = isNotNull(schema.environment.resourceFilter); | |
| const resource = await db.query.resource.findFirst({ | |
| where: and(eq(schema.resource.id, resourceId), isNotDeleted), | |
| with: { | |
| provider: { with: { google: true } }, | |
| workspace: { | |
| with: { | |
| systems: { | |
| with: { environments: { where: hasFilter }, deployments: true }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| if (resource == null) return null; | |
| const matchesIdentifier = eq( | |
| schema.jobResourceRelationship.resourceIdentifier, | |
| resource.identifier, | |
| ); | |
| const parent = await db.query.jobResourceRelationship.findFirst({ | |
| where: matchesIdentifier, | |
| }); | |
| const { systems } = resource.workspace; | |
| const systemsWithResource = await Promise.all( | |
| systems.map(async (system) => { | |
| const validEnvironments = await Promise.all( | |
| system.environments | |
| .filter((env) => isPresent(env.resourceFilter)) | |
| .map(async (env) => { | |
| const hasResource = await environmentHasResource( | |
| db, | |
| resource.id, | |
| env.resourceFilter! | |
| ); | |
| if (!hasResource) return null; | |
| return { | |
| ...env, | |
| resource, | |
| latestActiveReleases: await latestActiveReleaseByResourceAndEnvironmentId( | |
| db, | |
| resource.id, | |
| env.id | |
| ) | |
| }; | |
| }) | |
| ); | |
| const filteredEnvironments = validEnvironments.filter(isPresent); | |
| return filteredEnvironments.length > 0 | |
| ? { ...system, environments: filteredEnvironments } | |
| : null; | |
| }) | |
| ); | |
| const provider = resource.provider && { | |
| ...resource.provider, | |
| google: resource.provider.google?.[0] ?? null | |
| }; | |
| return { | |
| ...resource, | |
| workspace: { ...resource.workspace, systems: systemsWithResource }, | |
| provider, | |
| parent: parent ?? null, | |
| }; | |
| }; |
Summary by CodeRabbit
New Features
ResourceVisualizationDiagramcomponent.DeploymentNodecomponent to represent deployment statuses visually.EnvironmentNodecomponent for improved user interaction.ProviderNodeandResourceNodecomponents for consistent styling and layout.Bug Fixes
VisualizePagecomponent.Documentation