fix: Visualization diagram v2 #228
Conversation
WalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
packages/job-dispatch/src/resource/delete.ts (1)
Line range hint
20-42: Consider enhancing error handling in parallel operationsWhile the parallel execution of cleanup tasks is efficient, consider adding error handling to ensure partial failures don't leave the system in an inconsistent state.
Consider this improvement:
export const deleteResources = async (tx: Tx, resources: Resource[]) => { + try { const eventsPromises = Promise.all( resources.map(getEventsForResourceDeleted), ); const events = await eventsPromises.then((res) => res.flat()); await Promise.all(events.map(handleEvent)); const resourceIds = resources.map((r) => r.id); const deleteAssociatedObjects = Promise.all( resources.map((r) => deleteObjectsAssociatedWithResource(tx, r)), ); await Promise.all([ deleteAssociatedObjects, tx .update(resource) .set({ deletedAt: new Date() }) .where(inArray(resource.id, resourceIds)), ]); + } catch (error) { + // Log the error for debugging + console.error('Failed to delete resources:', error); + // Re-throw to allow transaction rollback + throw error; + } };apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (1)
47-47: Consider updating diagram documentation.The change from vertical to horizontal layout is a significant visual change that affects how the diagram is rendered and interpreted.
Consider:
- Adding a comment explaining the layout convention
- Updating any relevant documentation about the visualization diagram
- Adding a screenshot example in the documentation
<Handle type="target" className={cn( "h-2 w-2 rounded-full border border-neutral-500 bg-neutral-800", isKubernetes && "border-blue-500/70", isTerraform && "border-purple-500/70", isSharedCluster && "border-blue-500/70", )} + // Horizontal layout: connections flow left-to-right position={Position.Left} />Also applies to: 57-57
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (1)
Line range hint
1-64: Consider documenting the new visualization architecture.Given this is part of a larger visualization refactor (v2) with significant changes including:
- Horizontal flow direction
- Removal of DeploymentNode
- Changes to resource relationships
Consider adding documentation about:
- The new visualization architecture
- Node connection patterns
- Migration guide for existing visualizations
Would you like me to help create a documentation template for these architectural changes?
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/variables/page.tsx (2)
37-39: Standardize API query parameter style.While the refactoring to
resourceIdis good, there's an inconsistency in how parameters are passed to the queries:
- Line 38 uses object style:
{ resourceId }- Line 39 uses direct parameter style:
resourceIdThis inconsistency could lead to confusion. Consider standardizing to the object parameter style for both queries.
const resourceId = params.targetId; const deployments = api.deployment.byTargetId.useQuery({ resourceId }); - const variables = api.deployment.variable.byTargetId.useQuery(resourceId); + const variables = api.deployment.variable.byTargetId.useQuery({ resourceId });
37-39: Add explicit loading and error handling states.The component could benefit from explicit handling of loading and error states from the queries for better user experience.
const resourceId = params.targetId; const deployments = api.deployment.byTargetId.useQuery({ resourceId }); const variables = api.deployment.variable.byTargetId.useQuery(resourceId); + + if (deployments.isLoading || variables.isLoading) { + return <div>Loading...</div>; + } + + if (deployments.error || variables.error) { + return <div>Error loading data</div>; + }apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/DeploymentContent.tsx (2)
42-46: Simplify status styling logicThe current className conditional logic is hard to maintain and has redundant classes. Consider using a status-to-style mapping for better maintainability.
Consider refactoring to:
+ const statusStyles = { + null: "bg-neutral-800 text-muted-foreground", + completed: "bg-green-500/30 text-green-400 text-muted-foreground", + }; + <div className={cn( - deployment.releaseJobTrigger?.job == null && - "bg-neutral-800 text-muted-foreground", - deployment.releaseJobTrigger?.job.status === "completed" && - "bg-green-500/30 text-green-400 text-muted-foreground", + statusStyles[deployment.releaseJobTrigger?.job?.status ?? null] )} >
48-50: Clarify the fallback messageThe message "No deployments" is misleading since we're already inside a deployment mapping. This suggests the version is missing, not that there are no deployments.
Consider updating the fallback message:
- {deployment.releaseJobTrigger?.release.version ?? "No deployments"} + {deployment.releaseJobTrigger?.release.version ?? "No version"}apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/ReleaseCell.tsx (1)
47-77: Consider refactoring for better maintainabilityWhile the new status handlers are functionally correct, consider these improvements:
- Extract repeated styling classes into a constant or utility class
- Centralize the status-to-color mapping in a constant/enum
- Add unit tests for the new states
Example refactor:
const STATUS_STYLES = { [JobStatus.Cancelled]: { bgColor: 'bg-neutral-400', icon: IconCircleX, }, [JobStatus.Failure]: { bgColor: 'bg-red-400', icon: IconCircleX, }, // ... other statuses } as const; const baseStyles = 'rounded-full p-1 dark:text-black'; export const ReleaseIcon: React.FC<{ job?: Job }> = ({ job }) => { if (!job?.status || !(job.status in STATUS_STYLES)) return null; const { bgColor, icon: Icon } = STATUS_STYLES[job.status]; return ( <div className={`${baseStyles} ${bgColor}`}> <Icon strokeWidth={2} /> </div> ); };apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1)
Line range hint
1-114: Consider documenting the architectural decisions.The significant simplification of the visualization architecture (removing System and Deployment nodes) improves maintainability. However, consider:
- Documenting these architectural decisions for future reference
- Adding comments explaining the new data structure and relationship model
- Updating any related documentation or diagrams
Consider creating an Architecture Decision Record (ADR) to document the rationale behind removing the System and Deployment nodes and how this affects the overall system visualization strategy.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/VariablesContent.tsx (1)
107-107: Remove redundant variable assignmentThe
resourceIdvariable assignment is redundant as it's just renamingtargetIdwithout any transformation.Consider using
targetIddirectly or remove the assignment:- const resourceId = targetId; const deployments = api.deployment.byTargetId.useQuery({ resourceId: targetId }); const variables = api.deployment.variable.byTargetId.useQuery({ resourceId: targetId });apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/page.tsx (1)
28-30: Consider updating component interface and API consistency.A few suggestions to improve the code:
- Update the component props to use
resourceIdinstead oftargetIdfor consistency with the internal usage.- Standardize the API query parameters -
byResourceIduses a direct string parameter whilebyTargetIduses an object parameter.-const DeploymentsTable: React.FC<{ targetId: string }> = ({ targetId }) => { - const resourceId = targetId; +const DeploymentsTable: React.FC<{ resourceId: string }> = ({ resourceId }) => { const jobs = api.job.byResourceId.useQuery(resourceId); - const deployments = api.deployment.byTargetId.useQuery({ resourceId }); + const deployments = api.deployment.byTargetId.useQuery(resourceId);packages/api/src/router/resources.ts (1)
397-397: Consider adding defensive programming for the google provider access.While the current implementation works, it might be more robust to handle the case where the
- google: resource.provider.google[0] ?? null, + google: resource.provider.google?.[0] ?? null,This change would prevent potential runtime errors if the
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (3)
93-99: Simplify opacity calculation in theSkeletoncomponentThe opacity calculation in the
styleprop can be simplified by removing the unnecessary multiplication by1.Apply this diff to simplify the code:
-{ opacity: 1 * (1 - i / 3) } +{ opacity: 1 - i / 3 }
68-75: Handle additional job statuses for robustnessThe status checks for
isInProgress,isPending, andisCompletedconsider specific job statuses. To make the component more robust against future status additions or changes, consider handling other possible statuses or using a default case.For example, you could define a function to determine the status color:
const getStatusBorderColor = (): string => { if (isInProgress) return "border-blue-500"; if (isPending) return "border-neutral-500"; if (isCompleted) return "border-green-500"; return "border-default"; };And then use it in your
className:className={cn( "relative flex min-w-[250px] flex-col gap-2 rounded-md border px-4 py-3", - isInProgress && "border-blue-500", - isPending && "border-neutral-500", - isCompleted && "border-green-500", + getStatusBorderColor(), )}
80-85: Avoid redundant border classes inclassNameThe base class includes
border border-neutral-800, but conditional classes also apply border colors. This could lead to redundant or conflicting styles. Consider removing the base border color or adjusting the conditional classes.Apply this diff to remove the redundant base border style:
className={cn( - "relative flex min-w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3", + "relative flex min-w-[250px] flex-col gap-2 rounded-md bg-neutral-900/30 px-4 py-3", isInProgress && "border-blue-500", isPending && "border-neutral-500", isCompleted && "border-green-500", )}packages/api/src/router/deployment.ts (1)
618-622: Add Validation to Prevent Non-positive 'jobsPerDeployment' ValuesThe
jobsPerDeploymentparameter could potentially be zero or negative, leading to unexpected behavior in the query. Consider adding a.min(1)constraint to ensure it is at least 1.Apply this change to the input schema:
.input( z.object({ resourceId: z.string().uuid(), environmentIds: z.array(z.string().uuid()).optional(), deploymentIds: z.array(z.string().uuid()).optional(), - jobsPerDeployment: z.number().optional().default(30), + jobsPerDeployment: z.number().min(1).optional().default(30), showAllStatuses: z.boolean().optional().default(false), }), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/ReleaseCell.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/variables/page.tsx(1 hunks)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(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx(0 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(1 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/SystemNode.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/DeploymentContent.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/VariablesContent.tsx(1 hunks)apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts(0 hunks)packages/api/src/router/deployment.ts(7 hunks)packages/api/src/router/resources.ts(1 hunks)packages/db/drizzle/0042_lonely_warstar.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource.ts(0 hunks)packages/job-dispatch/src/resource/delete.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/SystemNode.tsx
- apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts
- packages/db/src/schema/resource.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0042_lonely_warstar.sql
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/deployment.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
🔇 Additional comments (14)
packages/job-dispatch/src/resource/delete.ts (2)
6-6: LGTM: Import changes align with schema updates
The removal of the deploymentResourceRelationship import aligns with the broader refactoring effort to simplify the resource relationship model.
10-18: Verify the impact of removing deployment relationship cleanup
The simplified relationship cleanup looks correct, but let's verify there are no remaining references to these resources in other tables.
✅ Verification successful
Let me check the SQL files to understand the nature of these references.
The deployment relationship cleanup is properly handled
The SQL migrations show that the deployment_resource_relationship table has been explicitly dropped (in migration 0042), and all other resource relationships are properly handled with CASCADE delete constraints. The current implementation correctly focuses on cleaning up resource_relationship table entries, while other related tables (resource_session, resource_metadata, resource_variable, etc.) are automatically handled through their CASCADE delete constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to resources in deployment-related tables
# This helps ensure we haven't missed any cleanup requirements after removing deployment_resource_relationship
# Search for any deployment-resource relationship references in SQL files
echo "Checking for deployment-resource references in SQL files..."
rg -l "deployment.*resource|resource.*deployment" --type sql
# Search for any deployment-resource relationship references in TypeScript files
echo "Checking for deployment-resource references in TypeScript code..."
ast-grep --pattern 'deployment$_resource' --lang ts
Length of output: 473
Script:
#!/bin/bash
# Check the content of the SQL files that have references
echo "Content of 0039_hard_joystick.sql:"
cat packages/db/drizzle/0039_hard_joystick.sql
echo -e "\nContent of 0042_lonely_warstar.sql:"
cat packages/db/drizzle/0042_lonely_warstar.sql
# Let's also check if there are any other tables referencing resource.id
echo -e "\nChecking for other foreign key references to resource.id:"
rg "REFERENCES.*resource.*id" --type sql
Length of output: 4530
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (1)
55-55: LGTM! Verify the impact of handle position changes.
The change from vertical (Top/Bottom) to horizontal (Left/Right) flow makes sense and follows common visualization patterns. The changes are consistent with similar modifications in ResourceNode.tsx.
Let's verify the handle positions across all node types and check for any affected connections:
Also applies to: 63-63
✅ Verification successful
Handle positions are consistently updated across all node components
The verification shows that all node components (ProviderNode, EnvironmentNode, and ResourceNode) consistently use Left/Right positions for their handles, confirming a systematic change to horizontal flow layout.
- ProviderNode: Left/Right positions ✓
- EnvironmentNode: Left/Right positions ✓
- ResourceNode: Left/Right positions ✓
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in handle positions across node components
# and identify any potentially affected connections
# Check for other node components using Handle positions
rg -A 2 "position={Position\." "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize"
# Look for any connection configurations that might need updates
rg "Position\.(Top|Bottom|Left|Right)" "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize"
Length of output: 1215
apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/DeploymentContent.tsx (1)
12-12: Verify consistent usage of resourceId
Let's verify that the transition from targetId to resourceId is consistent across the codebase.
✅ Verification successful
Consistent usage of resourceId confirmed
The change from targetId to resourceId in DeploymentContent.tsx is part of a consistent pattern across the codebase. The search results show that resourceId is the standardized identifier used throughout the application, particularly in:
- Database schemas and queries
- API routes and endpoints
- Job dispatch and deployment logic
- Resource management components
The change aligns with the codebase's conventions and maintains consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of targetId that might need updating
# Search for targetId usage in TypeScript/JavaScript files
echo "Checking for targetId usage in TS/JS files:"
rg -t typescript -t javascript 'targetId' -A 2
# Search for resourceId usage to verify the new pattern
echo -e "\nVerifying resourceId usage:"
rg -t typescript -t javascript 'resourceId' -A 2
Length of output: 333
Script:
#!/bin/bash
# Let's try with correct file extensions
echo "Checking for targetId usage:"
rg "targetId" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -A 2
echo -e "\nVerifying resourceId usage:"
rg "resourceId" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -A 2
Length of output: 84754
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/ReleaseCell.tsx (2)
11-18: LGTM! Icon imports are well-organized
The new icon imports are properly organized and align with the new job status handlers.
23-23: LGTM! Component export enhances reusability
The ReleaseIcon component is now properly exported, allowing it to be reused across the application.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (3)
90-90: LGTM! Explicit layout direction improves visualization.
The left-to-right layout is a good choice for relationship diagrams, making the flow of dependencies more intuitive.
87-87: Verify all necessary relationships are represented in the simplified edge structure.
The edge creation has been simplified to only include resource-to-environment and provider-to-resource relationships.
✅ Verification successful
Edge relationships are correctly represented for the resource visualization context
The simplified edge structure is complete and appropriate for this visualization context. The code shows two essential relationships:
- Resource-to-Environment edges showing where resources are deployed ("in" relationship)
- Provider-to-Resource edges showing resource discovery ("discovered" relationship)
These relationships align with the component's purpose of visualizing resource relationships, and no other edge types are necessary for this specific diagram.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other relationship types that might need edges
rg -A 5 "createEdgesFrom.*To.*" --type typescript
Length of output: 86
Script:
#!/bin/bash
# Search for edge creation patterns in the codebase
rg -l "createEdge|Edge.*From|.*ToEdge"
# Get the content of the current file to understand the context better
cat "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx"
Length of output: 4017
Script:
#!/bin/bash
# Let's check the edges.ts file to understand all edge creation functions
cat "apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/edges.ts"
Length of output: 1118
60-67: Consider performance and validation implications of the new data structure.
While consolidating deployments into environment data simplifies the visualization, please consider:
- Performance impact when handling environments with many deployments
- Validation of system and environment names in the label to handle special characters
Consider implementing pagination or virtualization if environments can contain a large number of deployments.
packages/db/drizzle/meta/_journal.json (1)
298-307: LGTM! Verify the associated migration file.
The new migration entry follows the established pattern and maintains consistency with existing entries.
Let's verify the associated migration file exists and contains the expected schema changes:
✅ Verification successful
Migration file verified and matches expected changes
The migration file 0042_lonely_warstar.sql exists and correctly implements the removal of the deployment_resource_relationship table as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration file exists and contains expected changes
# Check if the migration file exists
fd "0042_lonely_warstar.sql" packages/db/drizzle
# Display the migration file contents to verify the removal of deployment_resource_relationship table
fd "0042_lonely_warstar.sql" packages/db/drizzle --exec cat {}
Length of output: 204
packages/api/src/router/resources.ts (1)
397-397: LGTM! Clean simplification of the google provider handling.
The use of the nullish coalescing operator (??) is a more concise and idiomatic way to handle the default case.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (1)
54-63: Verify the parameter name in api.deployment.byTargetId.useQuery
The useQuery hook is calling api.deployment.byTargetId with a resourceId instead of a targetId. Please confirm that this is intentional and that the byTargetId method accepts resourceId as a parameter.
Run the following script to check the method definition:
✅ Verification successful
The parameter name resourceId is correct in the byTargetId query
Looking at the implementation in packages/api/src/router/deployment.ts, the byTargetId procedure accepts an input object that explicitly includes resourceId as a parameter. The procedure is correctly typed and the parameter name matches the API contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if `byTargetId` method accepts `resourceId` or `targetId`.
# Search for the definition of `byTargetId` and inspect its parameters.
fd --type f --extension ts --extension tsx | xargs grep -nE 'byTargetId\('
Length of output: 76
Script:
#!/bin/bash
# Let's try a more comprehensive search using ripgrep to find the API definition
rg -l "deployment.*byTargetId" --type ts --type tsx
# Also search for the router definition since it's likely a tRPC API
rg "deployment.*router" --type ts
Length of output: 395
Script:
#!/bin/bash
# Let's check the deployment router implementation
cat packages/api/src/router/deployment.ts
# Also search with ast-grep to find the procedure definition
ast-grep --pattern 'procedure($$$).query($$$byTargetId$$$)'
Length of output: 23123
packages/api/src/router/deployment.ts (2)
713-713: Confirm Correct Usage of 'lte' Function in Query
Verify that the lte function is correctly used in the condition:
lte(rankSubquery.rank, jobsPerDeployment),Ensure that rankSubquery.rank and jobsPerDeployment are of compatible types and that this condition effectively limits the number of jobs per deployment as intended.
659-669: Review the Implementation of 'rankSubquery'
The rankSubquery calculates the rank of releases per deployment. Confirm that it integrates correctly with the main query and that the ranking behaves as expected, especially when limiting jobs per deployment.
| @@ -44,7 +44,7 @@ export const ResourceNode: React.FC<ResourceNodeProps> = (node) => { | |||
| isTerraform && "border-purple-500/70", | |||
| isSharedCluster && "border-blue-500/70", | |||
| )} | |||
| position={Position.Top} | |||
| position={Position.Left} | |||
There was a problem hiding this comment.
💡 Codebase verification
Found inconsistent handle positions in the visualization nodes
The search reveals that while most nodes use the new horizontal layout (Left/Right positions), there are still some nodes using vertical positions (Top/Bottom). Specifically:
Position.TopandPosition.Bottomare still present in some components- Most nodes have been updated to use
Position.LeftandPosition.Right
To maintain consistency across the visualization:
- Update remaining Top/Bottom positions to match the new Left/Right convention
- Check if the Top/Bottom positions in other nodes are intentional or were missed in this update
🔗 Analysis chain
LGTM! Verify handle position consistency across all node types.
The change from vertical (Top/Bottom) to horizontal (Left/Right) handle positions makes sense for infrastructure diagrams. This layout is more intuitive for visualizing resource relationships.
Let's verify that all node types follow this new convention:
Also applies to: 57-57
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handle positions across all node components
# Look for any remaining Top/Bottom handle positions that might have been missed
# Search for Position.Top or Position.Bottom in node components
rg -l "Position\.(Top|Bottom)" --type typescript | grep -i "node\.tsx$"
# Search for all node components to ensure we haven't missed any
fd -e tsx$ -e ts$ | grep -i "node\.tsx$"
Length of output: 155
Script:
#!/bin/bash
# Let's try a different approach to find node components and their handle positions
# First, find all Node.tsx files
fd "Node\.tsx$"
# Then search for Position enum usage in these files
echo "=== Handle Positions ==="
rg "Position\." -A 1 -B 1 --no-filename $(fd "Node\.tsx$")
Length of output: 3345
| const resourceId = targetId; | ||
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | ||
| const targetValues = api.deployment.variable.byTargetId.useQuery(resourceId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize query parameter passing
The query parameter passing is inconsistent between the two queries. While deployment.byTargetId uses an object parameter, deployment.variable.byTargetId uses a direct string parameter.
Apply this diff to standardize the approach:
const resourceId = targetId;
const deployments = api.deployment.byTargetId.useQuery({ resourceId });
- const targetValues = api.deployment.variable.byTargetId.useQuery(resourceId);
+ const targetValues = api.deployment.variable.byTargetId.useQuery({ resourceId });📝 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 resourceId = targetId; | |
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | |
| const targetValues = api.deployment.variable.byTargetId.useQuery(resourceId); | |
| const resourceId = targetId; | |
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | |
| const targetValues = api.deployment.variable.byTargetId.useQuery({ resourceId }); |
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | ||
| const variables = api.deployment.variable.byTargetId.useQuery(resourceId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency in API call patterns
The deployment query uses an object parameter { resourceId } while the variables query uses the resourceId directly. This inconsistency could lead to maintenance issues.
Consider using the same parameter structure for both queries:
const deployments = api.deployment.byTargetId.useQuery({ resourceId });
- const variables = api.deployment.variable.byTargetId.useQuery(resourceId);
+ const variables = api.deployment.variable.byTargetId.useQuery({ resourceId });📝 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 deployments = api.deployment.byTargetId.useQuery({ resourceId }); | |
| const variables = api.deployment.variable.byTargetId.useQuery(resourceId); | |
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | |
| const variables = api.deployment.variable.byTargetId.useQuery({ resourceId }); |
| const resourceId = params.targetId; | ||
| const target = api.resource.byId.useQuery(resourceId); | ||
| const jobs = api.job.byResourceId.useQuery(resourceId); | ||
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); |
There was a problem hiding this comment.
Fix error handling and standardize API parameters.
Two issues to address:
- The target query result is used in the notFound() check before confirming loading is complete.
- The API queries show inconsistent parameter passing patterns.
const resourceId = params.targetId;
const target = api.resource.byId.useQuery(resourceId);
const jobs = api.job.byResourceId.useQuery(resourceId);
- const deployments = api.deployment.byTargetId.useQuery({ resourceId });
+ const deployments = api.deployment.byTargetId.useQuery(resourceId);
const unlockTarget = api.resource.unlock.useMutation();
const lockTarget = api.resource.lock.useMutation();
const utils = api.useUtils();
const isLoading = target.isLoading || jobs.isLoading || deployments.isLoading;
- if (!target.isLoading && target.data == null) notFound();
- if (isLoading)
+ if (isLoading) {
return (
<div className="flex h-full w-full items-center justify-center">
<IconLoader2 className="h-8 w-8 animate-spin" />
</div>
);
+ }
+
+ if (target.data == null) {
+ notFound();
+ }📝 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 resourceId = params.targetId; | |
| const target = api.resource.byId.useQuery(resourceId); | |
| const jobs = api.job.byResourceId.useQuery(resourceId); | |
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); | |
| const resourceId = params.targetId; | |
| const target = api.resource.byId.useQuery(resourceId); | |
| const jobs = api.job.byResourceId.useQuery(resourceId); | |
| const deployments = api.deployment.byTargetId.useQuery(resourceId); | |
| const unlockTarget = api.resource.unlock.useMutation(); | |
| const lockTarget = api.resource.lock.useMutation(); | |
| const utils = api.useUtils(); | |
| const isLoading = target.isLoading || jobs.isLoading || deployments.isLoading; | |
| if (isLoading) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center"> | |
| <IconLoader2 className="h-8 w-8 animate-spin" /> | |
| </div> | |
| ); | |
| } | |
| if (target.data == null) { | |
| notFound(); | |
| } |
| const resourceId = targetId; | ||
| const jobs = api.job.byResourceId.useQuery(resourceId); | ||
| const deployments = api.deployment.byTargetId.useQuery({ resourceId }); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent parameter passing detected in API calls
There are inconsistencies in how resourceId is being passed to API queries:
api.job.byResourceId.useQuery()takesresourceIddirectlyapi.deployment.byTargetId.useQuery()takes{ resourceId }as an objectapi.deployment.variable.byTargetId.useQuery()takesresourceIddirectly
Additionally, there's one instance in JobsContent.tsx still using targetId instead of resourceId:
const jobs = api.job.byResourceId.useQuery(targetId);🔗 Analysis chain
Verify consistent usage of resourceId across the codebase.
Let's ensure the refactoring from targetId to resourceId is consistently applied across all components and API calls.
Also applies to: 110-113
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of targetId in API calls and verify resourceId usage
# Check for remaining targetId usage in API calls
echo "Checking for targetId in API calls..."
rg -A 2 "api\.[a-zA-Z]+\.byTargetId\.useQuery"
# Check for resourceId usage pattern
echo "Checking resourceId usage pattern..."
rg -A 2 "api\.[a-zA-Z]+\.byResourceId\.useQuery"
# Check for potential inconsistencies in parameter passing
echo "Checking parameter passing patterns..."
rg "\.useQuery\(\{.*resourceId"
Length of output: 4944
| const tg = await ctx.db | ||
| .select() | ||
| .from(resource) | ||
| .where(and(eq(resource.id, input), isNull(resource.deletedAt))) | ||
| .where(and(eq(resource.id, resourceId), isNull(resource.deletedAt))) |
There was a problem hiding this comment.
Check for Existence of Resource Before Proceeding
After fetching the resource with resourceId, ensure that it exists (tg is not null) before using it. This prevents potential null reference errors if an invalid resourceId is provided.
Consider adding a check:
if (!tg) {
throw new Error("Resource not found");
}| deploymentIds != null | ||
| ? inArray(deployment.id, deploymentIds) | ||
| : undefined, |
There was a problem hiding this comment.
Handle Empty 'deploymentIds' Array in Filter Condition
Similarly, if deploymentIds is an empty array, the inArray condition will exclude all deployments. Ensure a length check is in place to prevent unintended filtering.
Modify the condition like this:
- deploymentIds != null
+ deploymentIds != null && deploymentIds.length > 0
? inArray(deployment.id, deploymentIds)
: undefined,📝 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.
| deploymentIds != null | |
| ? inArray(deployment.id, deploymentIds) | |
| : undefined, | |
| deploymentIds != null && deploymentIds.length > 0 | |
| ? inArray(deployment.id, deploymentIds) | |
| : undefined, |
| environmentIds != null | ||
| ? inArray(environment.id, environmentIds) | ||
| : undefined, |
There was a problem hiding this comment.
Handle Empty 'environmentIds' Array in Filter Condition
If environmentIds is provided as an empty array, the inArray condition will result in no environments being matched, which might not be the intended behavior. Add a length check to ensure the condition is only applied when environmentIds is not empty.
Update the filter condition as follows:
- environmentIds != null
+ environmentIds != null && environmentIds.length > 0
? inArray(environment.id, environmentIds)
: undefined,📝 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.
| environmentIds != null | |
| ? inArray(environment.id, environmentIds) | |
| : undefined, | |
| environmentIds != null && environmentIds.length > 0 | |
| ? inArray(environment.id, environmentIds) | |
| : undefined, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1)
57-68: Consider performance and maintainability implications of nested data structureThe new structure embeds deployment data within environment nodes, which:
- Could impact performance if there are many deployments per environment
- Increases nesting level, potentially making debugging more challenging
- Might cause unnecessary re-renders if deployments change frequently
Consider extracting deployments to a separate state if they need frequent updates.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx(3 hunks)
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (3)
88-88: LGTM! Edge creation logic has been simplified
The edge creation has been streamlined to focus only on resource-to-environment and provider-to-resource relationships, making the visualization clearer and more maintainable.
91-91: Verify layout direction with various node configurations
The enforced left-to-right layout direction might affect readability with:
- Large number of nodes
- Deep hierarchies
- Wide resource-to-environment relationships
Please test the visualization with different data sets to ensure the layout remains clear and usable.
Consider making the direction configurable based on:
- const setReactFlowInstance = useLayoutAndFitView(nodes, { direction: "LR" });
+ const setReactFlowInstance = useLayoutAndFitView(nodes, {
+ direction: window.innerWidth > window.innerHeight ? "LR" : "TB"
+ });25-25: Verify if System type definition should be retained
The System type definition appears to be retained while the AI summary indicates that system concepts have been removed from the visualization logic. This might indicate an incomplete refactoring.
Let's verify the usage of System type in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (3)
27-49: Consider enhancing accessibility for job status indicators.While the component is well-structured, consider adding ARIA labels or role attributes to improve accessibility for users relying on screen readers, especially for the job status indicators.
- <div className="flex items-center gap-2"> + <div + className="flex items-center gap-2" + role="status" + aria-label={`Job status: ${JobStatusReadable[job.status]}`} + >
77-82: Enhance status visualization beyond just border colors.Using only border colors for status indication may not be sufficient for accessibility and UX. Consider adding icons or badges to make the status more prominent.
className={cn( "relative flex min-w-[250px] flex-col gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4 py-3", - isInProgress && "border-blue-500", - isPending && "border-neutral-500", - isCompleted && "border-green-500", + isInProgress && "border-blue-500 before:content-['In_Progress'] before:absolute before:-top-2 before:left-2 before:text-xs before:bg-blue-500/10 before:px-2 before:rounded", + isPending && "border-neutral-500 before:content-['Pending'] before:absolute before:-top-2 before:left-2 before:text-xs before:bg-neutral-500/10 before:px-2 before:rounded", + isCompleted && "border-green-500 before:content-['Completed'] before:absolute before:-top-2 before:left-2 before:text-xs before:bg-green-500/10 before:px-2 before:rounded", )}
98-113: Optimize rendering performance.The mapping operation inside the render function could benefit from memoization to prevent unnecessary re-renders.
+ const memoizedDeployments = React.useMemo( + () => + data.environment.deployments.map((deployment) => { + const latestActiveRelease = latestActiveReleases.find( + (r) => r.releaseJobTrigger.release.deploymentId === deployment.id, + ); + return ( + <DeploymentCard + key={deployment.id} + deploymentName={deployment.name} + job={latestActiveRelease?.releaseJobTrigger.job} + releaseVersion={ + latestActiveRelease?.releaseJobTrigger.release.version ?? "" + } + /> + ); + }), + [data.environment.deployments, latestActiveReleases] + ); {!latestActiveReleasesQ.isLoading && - data.environment.deployments.map((deployment) => { - const latestActiveRelease = latestActiveReleases.find( - (r) => - r.releaseJobTrigger.release.deploymentId === deployment.id, - ); - return ( - <DeploymentCard - key={deployment.id} - deploymentName={deployment.name} - job={latestActiveRelease?.releaseJobTrigger.job} - releaseVersion={ - latestActiveRelease?.releaseJobTrigger.release.version ?? "" - } - /> - ); - })} + memoizedDeployments}packages/db/src/schema/resource.ts (1)
275-286: Consider adding foreign key constraint and audit fields.While the basic table structure is good, consider these improvements:
- Add a foreign key constraint on
resourceIdentifierto maintain referential integrity with resources- Add
created_atandupdated_attimestamps for auditing purposesexport 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(), + resourceIdentifier: text("resource_identifier") + .references(() => resource.identifier) + .notNull(), + createdAt: timestamp("created_at", { withTimezone: true }) + .notNull() + .defaultNow(), + updatedAt: timestamp("updated_at", { withTimezone: true }) + .$onUpdate(() => new Date()), }, (t) => ({ uniq: uniqueIndex().on(t.jobId, t.resourceIdentifier), }), );packages/api/src/router/deployment.ts (2)
674-674: Consider implementing paginationWhile adding a limit of 500 records helps prevent excessive data retrieval, consider implementing proper pagination using cursor-based or offset-based pagination for better scalability and user experience.
Example implementation:
-.limit(500) +.offset(input.offset ?? 0) +.limit(input.limit ?? 50)And update the input type:
.input(z.object({ id: z.string().uuid(), offset: z.number().min(0).optional(), limit: z.number().min(1).max(100).optional() }))
Based on the search results, I can now generate the final response since I have found all the job statuses defined in the codebase:
Consider including
FailedandCancelledstatuses in the job status filterThe current job status filtering includes
Completed,Pending, andInProgress, but it's missing important operational statuses that are defined and used in the codebase:
Failure- Used to track failed jobsCancelled- Used to track cancelled jobsSkipped- Used to track skipped jobsIncluding these additional statuses would provide a more complete operational view of the deployment status.
🔗 Analysis chain
Line range hint
664-673: Consider handling additional job statusesThe current job status filtering includes
Completed,Pending, andInProgress, but consider whether other statuses likeFailedorCancelledshould be included for a complete operational view.Let's check if there are any other job statuses in use:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JobStatus enum usage across the codebase ast-grep --pattern 'JobStatus.$_'Length of output: 42989
packages/api/src/router/resources.ts (1)
362-449: LGTM! Authorization and input validation are properly implemented.The implementation correctly handles permissions, validates input, and uses proper SQL query builder patterns to prevent SQL injection.
Consider adding an index on
(deployment_id, created_at)to optimize the rank calculation in the subquery, as this operation could be expensive for large datasets. The query uses this combination frequently in the window function:ROW_NUMBER() OVER (PARTITION BY release.deployment_id ORDER BY release.created_at DESC)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/variables/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/DeploymentContent.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/VariablesContent.tsx(1 hunks)packages/api/src/router/deployment.ts(3 hunks)packages/api/src/router/resources.ts(2 hunks)packages/db/drizzle/0042_absent_cerebro.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/variables/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/DeploymentContent.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/target-drawer/VariablesContent.tsx
- packages/db/drizzle/meta/_journal.json
🔇 Additional comments (10)
packages/db/drizzle/0042_absent_cerebro.sql (3)
8-12: LGTM! Foreign key constraint with appropriate cascade behavior.
The constraint is well-defined with proper cascade behavior and includes idempotency handling.
1-5: LGTM! Verify cascade behavior for resource deletion.
The table schema is well-structured with appropriate data types and constraints.
Let's verify if resources have cascade delete behavior to prevent orphaned relationships:
✅ Verification successful
Cascade delete behavior is properly configured
The migration correctly sets up cascade delete behavior with ON DELETE cascade in the foreign key constraint job_resource_relationship_job_id_job_id_fk. This ensures that when a job is deleted, its associated resource relationships will be automatically removed, preventing orphaned records.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if resources table has cascade delete behavior
rg -A 10 'CREATE TABLE.*resource.*'
Length of output: 3139
Script:
#!/bin/bash
# Let's check if there's a foreign key constraint with cascade delete in the job table
rg -A 10 'CREATE TABLE.*job"'
Length of output: 908
14-14: LGTM! Verify index usage in application queries.
The unique composite index is well-designed and will prevent duplicate relationships while optimizing queries.
Let's verify the application code uses this index effectively:
✅ Verification successful
Index design is appropriate for the schema definition
The unique index on job_resource_relationship(job_id, resource_identifier) aligns well with the table schema and its usage patterns:
- The table definition shows these columns are marked as
notNulland have a composite unique constraint - The index matches the foreign key relationship with the
jobtable - The schema suggests lookups will be primarily by
job_id, making it an efficient leading column
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if queries in the codebase will utilize this index
# Look for queries involving job_id and resource_identifier
rg -A 5 'job.*resource.*identifier|resource.*job.*id' --type typescript
Length of output: 108
Script:
#!/bin/bash
# Search for SQL queries or database operations involving job_resource_relationship table
rg -l "job_resource_relationship"
# Then examine the specific files for query patterns
rg -A 5 "FROM.*job_resource_relationship|JOIN.*job_resource_relationship|INSERT.*job_resource_relationship|SELECT.*job_resource_relationship"
Length of output: 300
Script:
#!/bin/bash
# Let's examine the schema definition and any query builders in the resource.ts file
cat packages/db/src/schema/resource.ts
Length of output: 9500
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (2)
1-16: LGTM! Imports and setup are well-organized.
The imports are properly structured and the "use client" directive is correctly placed.
17-25: LGTM! Types are well-defined and provide good type safety.
The Environment and EnvironmentNodeProps types correctly model the relationships and required data.
packages/db/src/schema/resource.ts (2)
34-34: LGTM!
The import of the job schema is correctly added to support the new job-resource relationship.
275-286: Verify data migration from deploymentResourceRelationship.
Since this table replaces deploymentResourceRelationship, ensure there's a proper data migration strategy in place.
✅ Verification successful
The previous search didn't yield results, so I'm broadening the search to look for recent migrations, references to the new table, and recent changes to understand the context better. This will help determine if this is a new table addition or a replacement of an existing one.
Migration is properly handled - no action needed
The migration files show a clear transition strategy:
- Creation of new
job_resource_relationshiptable - Explicit dropping of old
deployment_resource_relationshiptable - Proper foreign key constraints and indexes are set up
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration files and verify table existence
# Check for migration files that handle the table transition
echo "Checking for relevant migration files..."
fd --type f ".*deployment.*resource.*relationship.*" packages/db/drizzle/
# Verify if old table still exists in schema
echo "Checking for old table definition..."
rg "deploymentResourceRelationship" packages/db/src/schema/
# Check for any remaining references to the old table
echo "Checking for remaining references to old table..."
rg "deployment.*resource.*relationship" --type ts
Length of output: 470
Script:
#!/bin/bash
# Let's check for any recent migrations that might be related to this change
echo "Checking recent migration files..."
fd --type f "\.sql$" packages/db/drizzle/ --exec head -n 20 {}
# Let's also check for any references to job_resource_relationship
echo "Checking references to new table..."
rg "job.*resource.*relationship" --type ts -A 3
# Let's check git history for recent changes related to this
echo "Checking recent changes..."
git log --oneline -n 5 -- packages/db/
Length of output: 18346
packages/api/src/router/deployment.ts (2)
373-373: LGTM: Addition of resourceId to query projection
The addition of resourceId to the select statement enhances the query's data retrieval capabilities and aligns with the schema requirements.
680-688: LGTM: Proper null handling for releaseJobTrigger
The implementation correctly handles the nullable release_job_trigger with proper type assertions. The non-null assertions are safe here due to the where clause conditions.
packages/api/src/router/resources.ts (1)
482-486: LGTM! Function parameters are correctly ordered.
The environmentHasResource function call maintains proper parameter ordering and type safety.
| "resource_identifier" text NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| DROP TABLE "deployment_resource_relationship";--> statement-breakpoint |
There was a problem hiding this comment.
Consider data migration before dropping the table.
This is a breaking change that will permanently delete all deployment-resource relationships. Consider adding a data migration step to preserve existing relationships by converting them to job-resource relationships if applicable.
Example migration approach:
- Create new table (done)
- Migrate relevant data from deployment relationships to job relationships
- Verify data consistency
- Then drop the old table
| const resourceId = data.environment.resource.id; | ||
| const environmentId = data.environment.id; | ||
| const latestActiveReleasesQ = | ||
| api.resource.activeReleases.byResourceAndEnvironmentId.useQuery( | ||
| { resourceId, environmentId }, | ||
| { refetchInterval: 5_000 }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the TRPC query.
The query lacks error handling which could lead to a poor user experience if the API request fails.
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 notification/handling here
+ }
+ },
);Committable suggestion skipped: line range outside the PR's diff.
| releaseJobTrigger: | ||
| row.release_job_trigger != null | ||
| ? { | ||
| ...row.release_job_trigger, | ||
| job: row.job!, | ||
| release: row.release!, | ||
| resourceId: row.resource.id, | ||
| } | ||
| : null, | ||
| })), |
There was a problem hiding this comment.
could we convert this to with query builder?
Summary by CodeRabbit
Release Notes
New Features
ReleaseIconcomponent.DeploymentCardto theEnvironmentNodefor displaying deployment information.Improvements
deploymentRouterfor more precise data retrieval.Bug Fixes
DeploymentContentandVariablesContent.Refactor
SystemandDeploymentnodes to simplify the visualization structure.