fix: Optimize flow node query #190
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the handling of workspace-specific data in 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: 7
🧹 Outside diff range and nitpick comments (3)
packages/validators/src/jobs/conditions/release-condition.ts (1)
3-7: Add JSDoc documentation to explain the schema's purpose and usage.The schema would benefit from documentation explaining its role in release condition validation and providing examples of valid conditions.
+/** + * Validates a release condition used in job filtering. + * + * @example + * ```typescript + * const condition = { + * type: "release", + * operator: "equals", + * value: "123e4567-e89b-12d3-a456-426614174000" + * }; + * ``` + */ export const releaseCondition = z.object({ type: z.literal("release"), operator: z.literal("equals"), value: z.string().uuid(), });apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx (1)
Line range hint
27-33: Consider parallelizing API calls for better performance.The sequential API calls to fetch workspace, release, deployment, system, environments, and policies could be optimized using Promise.all() to reduce the total loading time.
Here's a suggested optimization:
- const release = await api.release.byId(params.versionId); - const deployment = await api.deployment.bySlug(params); - const workspace = await api.workspace.bySlug(params.workspaceSlug); - if (workspace == null || release == null || deployment == null) notFound(); - - const system = await api.system.bySlug(params); - const environments = await api.environment.bySystemId(system.id); - const policies = await api.environment.policy.bySystemId(system.id); - const policyDeployments = await api.environment.policy.deployment.bySystemId( - system.id, - ); + const [release, deployment, workspace] = await Promise.all([ + api.release.byId(params.versionId), + api.deployment.bySlug(params), + api.workspace.bySlug(params.workspaceSlug), + ]); + + if (workspace == null || release == null || deployment == null) notFound(); + + const system = await api.system.bySlug(params); + + const [environments, policies, policyDeployments] = await Promise.all([ + api.environment.bySystemId(system.id), + api.environment.policy.bySystemId(system.id), + api.environment.policy.deployment.bySystemId(system.id), + ]);apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (1)
126-132: Simplify conditional rendering logicThe rendering logic can be simplified for better readability by using a single conditional statement.
Apply this diff to simplify the JSX:
- {loading && <Loading />} - {!loading && isWaitingOnActive && ( - <> - <Waiting /> Another release is in progress - </> - )} - {!loading && !isWaitingOnActive && ( - <> - <Passing /> All other releases finished - </> - )} + {loading ? ( + <Loading /> + ) : isWaitingOnActive ? ( + <> + <Waiting /> Another release is in progress + </> + ) : ( + <> + <Passing /> All other releases finished + </> + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx(1 hunks)packages/db/src/schema/job.ts(1 hunks)packages/validators/src/jobs/conditions/comparison-condition.ts(4 hunks)packages/validators/src/jobs/conditions/index.ts(1 hunks)packages/validators/src/jobs/conditions/job-condition.ts(5 hunks)packages/validators/src/jobs/conditions/release-condition.ts(1 hunks)
🔇 Additional comments (12)
packages/validators/src/jobs/conditions/release-condition.ts (1)
9-9: LGTM!
The type export using Zod's inference is correct and follows TypeScript best practices.
packages/validators/src/jobs/conditions/comparison-condition.ts (1)
11-11: LGTM! Clean integration of the ReleaseCondition type.
The changes consistently integrate the new ReleaseCondition type across imports, schema definition, and type declaration, maintaining the existing code structure and ordering patterns.
Also applies to: 21-21, 39-39, 58-58
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx (1)
Line range hint 49-56: LGTM! Workspace prop integration looks correct.
The workspace prop is properly fetched, validated, and passed to the FlowDiagram component. The implementation aligns well with the PR objectives for optimizing flow node queries.
Let's verify the prop type consistency across components:
✅ Verification successful
Props type consistency verified for workspace integration
The workspace prop is properly typed and integrated:
- FlowDiagram component correctly imports the
Workspacetype from@ctrlplane/db/schema - Props interface properly declares
workspace: Workspaceas required prop - The type is consistently used across the component hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify FlowDiagram component's workspace prop type definition
# Expected: Should find the workspace prop type definition in FlowDiagram.tsx
# Search for FlowDiagram component props interface/type
ast-grep --pattern 'interface $PROPS {
$$$
workspace: $_
$$$
}'
# Backup search using ripgrep
rg -A 5 'interface.*FlowDiagram.*Props|type.*FlowDiagram.*Props'
Length of output: 392
Script:
#!/bin/bash
# Let's try a different approach to find the FlowDiagram component and its props
# First, let's locate the FlowDiagram component file
fd FlowDiagram.tsx
# Then let's examine its content focusing on props/interfaces
rg -A 10 "type.*Props|interface.*Props" $(fd FlowDiagram.tsx)
# Also search for workspace prop usage in FlowDiagram
rg "workspace[?:]\s*" $(fd FlowDiagram.tsx)
# Additionally, let's check ReleaseSequencingNode for workspace prop usage
fd ReleaseSequencingNode.tsx
rg "workspace[?:]\s*" $(fd ReleaseSequencingNode.tsx)
Length of output: 902
Script:
#!/bin/bash
# Let's examine the full content of FlowDiagram.tsx to understand the props interface
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx" | head -n 50
# Also check for Workspace type import and definition
rg -B 2 -A 2 "type\s+Workspace|interface\s+Workspace"
Length of output: 3529
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx (3)
8-8: LGTM: Clean import addition
The Workspace type import is properly grouped with other schema types.
Line range hint 77-83: LGTM: Proper workspace context integration
The workspaceId is correctly derived from the workspace prop and properly integrated into the release-sequencing node data structure.
77-77: Verify workspaceId handling in dependent components
Let's verify that all components consuming the release-sequencing node data are updated to handle the new workspaceId field.
✅ Verification successful
Let me try a different search strategy to verify the impact.
Based on the search results, I can see that the workspaceId field is consistently used across the codebase, particularly in the context of flow diagrams and release sequencing. Let me verify one more aspect to ensure there are no inconsistencies.
Based on the comprehensive analysis of the codebase, I can now provide a final response:
✓ The workspaceId field is properly integrated
The workspaceId field in the release sequencing node is consistent with its usage across the codebase:
- It's properly defined in the
ReleaseSequencingNodePropstype - The field is correctly populated from the workspace context in FlowDiagram.tsx
- All components consuming this data structure expect and handle the workspaceId field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might be affected by the workspaceId addition
# Look for files that reference release-sequencing node type
echo "Files potentially affected by release-sequencing node changes:"
rg -l "release-sequencing" --type tsx
# Look for direct usage of the node data
echo -e "\nComponents accessing release-sequencing node data:"
ast-grep --pattern 'data: { $$$workspaceId$$$, $$$}'
Length of output: 372
Script:
#!/bin/bash
# Search for components that might be affected by workspaceId in flow diagrams
# Look for files referencing flow diagrams and workspace data
echo "Files with flow diagram and workspace references:"
rg -l "FlowDiagram|workspaceId" --type-add 'tsx:*.{tsx,ts}'
# Look for data structure usage patterns
echo -e "\nRelevant data structure usage:"
rg -A 5 "workspaceId.*workspace\.id" --type-add 'tsx:*.{tsx,ts}'
Length of output: 35936
Script:
#!/bin/bash
# Search for release sequencing node data structure and usage
echo "Release sequencing node data structure:"
rg -A 5 "type ReleaseSequencingNode" --type-add 'tsx:*.{tsx,ts}'
echo -e "\nRelease sequencing node usage:"
rg -A 5 "ReleaseSequencingNode.*props" --type-add 'tsx:*.{tsx,ts}'
Length of output: 1296
packages/validators/src/jobs/conditions/job-condition.ts (4)
12-12: LGTM: Import statements follow consistent patterns.
The new imports for ReleaseCondition type and releaseCondition schema are properly structured and follow the established import patterns.
Also applies to: 27-27
38-39: LGTM: Type union extension is properly implemented.
The ReleaseCondition is correctly added to the JobCondition union type, maintaining type safety and consistency.
50-50: LGTM: Schema union is properly extended.
The releaseCondition is correctly added to the jobCondition Zod schema union, maintaining validation consistency.
12-12: Verify the release condition implementation.
Let's verify the implementation of the referenced release condition schema and type.
Also applies to: 27-27, 38-39, 50-50, 64-64
✅ Verification successful
Release condition implementation is properly integrated
The release condition is well-implemented with:
- A proper Zod schema definition with type literal, operator constraint, and UUID value validation
- Correct type exports and schema integration in the job condition union
- Consistent usage across the conditions system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of release-condition.js and its usage
# Test 1: Verify the release condition schema implementation
rg -t typescript "export const releaseCondition = " "./packages/validators/src/jobs/conditions/"
# Test 2: Check for any direct usage of the new release condition
rg -t typescript "type === (?:\"|')release(?:\"|')" -g '!job-condition.ts'
Length of output: 247
Script:
#!/bin/bash
# Let's try without typescript file type flag and check the implementation
# Check for release condition implementation
rg "export (type|interface) ReleaseCondition" ./packages/validators/src/jobs/conditions/
rg "export const releaseCondition" ./packages/validators/src/jobs/conditions/
# Check the usage context
rg "ReleaseCondition" -A 3 ./packages/validators/src/jobs/conditions/
Length of output: 1977
Script:
#!/bin/bash
# Let's check the implementation details of release-condition.ts
# Get the full content of release-condition.ts
cat ./packages/validators/src/jobs/conditions/release-condition.ts
# Check where the release condition is being used in the union
rg "releaseCondition" -A 2 ./packages/validators/src/jobs/conditions/job-condition.ts
Length of output: 529
packages/db/src/schema/job.ts (1)
222-222: LGTM! Verify table joins for release filtering.
The implementation of the release filter condition is correct and follows the established pattern. However, ensure that the necessary table joins for the release table are properly set up in the query where this condition is used.
Let's verify the table joins:
✅ Verification successful
Table joins are properly handled for release filtering
The implementation is correct. The releaseJobTriggerQuery function in the router properly sets up all necessary table joins, including the release table:
const releaseJobTriggerQuery = (tx: Tx) =>
tx
.select()
.from(releaseJobTrigger)
.innerJoin(job, eq(releaseJobTrigger.jobId, job.id))
.innerJoin(target, eq(releaseJobTrigger.targetId, target.id))
.innerJoin(release, eq(releaseJobTrigger.releaseId, release.id))
.innerJoin(deployment, eq(release.deploymentId, deployment.id))
.innerJoin(environment, eq(releaseJobTrigger.environmentId, environment.id))
.innerJoin(jobAgent, eq(jobAgent.id, deployment.jobAgentId));The release table is properly joined before the condition eq(release.id, cond.value) is used in the filter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if release table joins are properly handled in job queries
# Test: Look for JOIN clauses between job and release tables
rg -A 5 "FROM.*job.*JOIN.*release|FROM.*release.*JOIN.*job"
Length of output: 61
Script:
#!/bin/bash
# Let's check the entire file content to understand the query building logic
cat packages/db/src/schema/job.ts
Length of output: 6529
Script:
#!/bin/bash
# Let's check where this condition is used to verify table joins
rg -A 10 "jobMatchesCondition|buildCondition" --type ts
Length of output: 8230
Script:
#!/bin/bash
# Let's check the query construction in the API router to see how joins are handled
cat packages/api/src/router/job.ts
Length of output: 20520
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (1)
23-23: Ensure workspaceId is passed correctly
The workspaceId prop has been added to ReleaseSequencingNodeProps. Verify that all instances where this component is used are updated to pass the workspaceId to prevent potential runtime errors.
Run the following script to find all usages of ReleaseSequencingNode and check for the workspaceId prop:
✅ Verification successful
Let me gather more information to check where this component is being used.
Let me check the content of FlowDiagram.tsx since it's the only file that uses ReleaseSequencingNode.
Let me check how the nodes are created and passed to the ReleaseSequencingNode component.
Let me try a different approach to find the node creation and data passing logic.
No issues found - workspaceId is correctly passed
The verification shows that the workspaceId prop is correctly passed to the ReleaseSequencingNode component in FlowDiagram.tsx. Specifically, when creating the release sequencing nodes:
{
id: env.id + "-release-sequencing",
type: "release-sequencing",
position: { x: 0, y: 0 },
data: {
workspaceId: workspace.id, // workspaceId is properly passed here
releaseId: release.id,
deploymentId: release.deploymentId,
environmentId: env.id,
policyType: policy?.releaseSequencing,
label: `${env.name} - release sequencing`,
},
}The component is only used in this one location and receives all required props, including workspaceId, as defined in ReleaseSequencingNodeProps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of ReleaseSequencingNode include the workspaceId prop
# Find all JSX files containing ReleaseSequencingNode
fd --extension jsx 'ReleaseSequencingNode'
# Search for instances without workspaceId
rg -A 5 -B 2 '<ReleaseSequencingNode' | rg -B 5 -A 5 -v 'workspaceId='
Length of output: 2622
Script:
#!/bin/bash
# Find all files that import or use ReleaseSequencingNode
rg -l "ReleaseSequencingNode"
# Show the actual usage of the component with context
rg -B 5 -A 5 "<ReleaseSequencingNode"
Length of output: 3842
Script:
#!/bin/bash
# Show the content of FlowDiagram.tsx focusing on ReleaseSequencingNode usage
rg -B 10 -A 10 "ReleaseSequencingNode" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx"
Length of output: 1241
Script:
#!/bin/bash
# Search for node creation logic in FlowDiagram.tsx
rg -A 20 "const nodes = \[" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx"
# Also check for any createNode functions that might be creating ReleaseSequencing nodes
rg -l "createNode.*release-sequencing"
Length of output: 199
Script:
#!/bin/bash
# Show more content from FlowDiagram.tsx to understand the component usage
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx"
# Also check the type definition of ReleaseSequencingNodeProps
rg -A 10 "type ReleaseSequencingNodeProps" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx"
Length of output: 4561
| type: z.literal("release"), | ||
| operator: z.literal("equals"), | ||
| value: z.string().uuid(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider supporting additional comparison operators.
The schema currently only supports the 'equals' operator. Consider adding support for other operators like 'not_equals' if there are use cases where excluding specific releases would be valuable.
export const releaseCondition = z.object({
type: z.literal("release"),
- operator: z.literal("equals"),
+ operator: z.enum(["equals", "not_equals"]),
value: z.string().uuid(),
});📝 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.
| type: z.literal("release"), | |
| operator: z.literal("equals"), | |
| value: z.string().uuid(), | |
| type: z.literal("release"), | |
| operator: z.enum(["equals", "not_equals"]), | |
| value: z.string().uuid(), |
| @@ -4,3 +4,4 @@ export * from "./status-condition.js"; | |||
| export * from "./deployment-condition.js"; | |||
| export * from "./environment-condition.js"; | |||
| export * from "./job-target-condition.js"; | |||
| export * from "./release-condition.js"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Warning: Potential naming conflict in release condition modules
The verification revealed two different ReleaseCondition types and releaseCondition schemas in the codebase:
packages/validators/src/jobs/conditions/release-condition.ts: A simple condition for UUID comparisonpackages/validators/src/releases/conditions/release-condition.ts: A comprehensive release condition system with multiple types
The export in the jobs module might conflict with or be confused with the releases module. Consider:
- Renaming the jobs condition to be more specific (e.g.,
JobReleaseCondition) - Or moving the jobs condition logic into the releases module if they should be unified
🔗 Analysis chain
LGTM! Verify the referenced module.
The new export follows consistent patterns with existing exports in terms of file extension and naming convention.
Let's verify the existence and exports of the referenced module:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and exports of release-condition.js
# Test 1: Check if the file exists
fd -e ts "release-condition"
# Test 2: Examine the exports from the module
rg -t typescript "export (type|interface|const|function)" -A 2 "release-condition"
Length of output: 369
Script:
#!/bin/bash
# Let's examine the content of the release-condition.ts files to verify exports
echo "=== Content of packages/validators/src/jobs/conditions/release-condition.ts ==="
cat "packages/validators/src/jobs/conditions/release-condition.ts"
echo -e "\n=== Content of packages/validators/src/releases/conditions/release-condition.ts ==="
cat "packages/validators/src/releases/conditions/release-condition.ts"
Length of output: 3921
| workspace: Workspace; | ||
| systemId: string; | ||
| release: Release; | ||
| envs: Array<Environment>; | ||
| policies: Array<EnvironmentPolicy>; | ||
| policyDeployments: Array<EnvironmentPolicyDeployment>; | ||
| }> = ({ release, envs, policies, policyDeployments }) => { | ||
| }> = ({ workspace, release, envs, policies, policyDeployments }) => { |
There was a problem hiding this comment.
Remove unused systemId prop
The systemId prop is declared but never used within the component. Consider removing it if it's not needed.
Apply this diff to remove the unused prop:
export const FlowDiagram: React.FC<{
workspace: Workspace;
- systemId: string;
release: Release;
envs: Array<Environment>;
policies: Array<EnvironmentPolicy>;
policyDeployments: Array<EnvironmentPolicyDeployment>;
}> = ({ workspace, release, envs, policies, policyDeployments }) => {📝 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.
| workspace: Workspace; | |
| systemId: string; | |
| release: Release; | |
| envs: Array<Environment>; | |
| policies: Array<EnvironmentPolicy>; | |
| policyDeployments: Array<EnvironmentPolicyDeployment>; | |
| }> = ({ release, envs, policies, policyDeployments }) => { | |
| }> = ({ workspace, release, envs, policies, policyDeployments }) => { | |
| workspace: Workspace; | |
| release: Release; | |
| envs: Array<Environment>; | |
| policies: Array<EnvironmentPolicy>; | |
| policyDeployments: Array<EnvironmentPolicyDeployment>; | |
| }> = ({ workspace, release, envs, policies, policyDeployments }) => { |
| @@ -57,6 +61,7 @@ export enum JobFilterType { | |||
| Status = "status", | |||
| Deployment = "deployment", | |||
| Environment = "environment", | |||
| Release = "release", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing type guard function for ReleaseCondition.
While the Release enum value is correctly added, there should be a corresponding type guard function for consistency with other condition types. Consider adding:
export const isReleaseCondition = (
condition: JobCondition,
): condition is ReleaseCondition =>
condition.type === JobFilterType.Release;| const isSameEnvironment: EnvironmentCondition = { | ||
| type: JobFilterType.Environment, | ||
| operator: ColumnOperator.Equals, | ||
| value: environmentId, | ||
| }; | ||
|
|
||
| const isPending: StatusCondition = { | ||
| type: JobFilterType.Status, | ||
| operator: ColumnOperator.Equals, | ||
| value: JobStatus.Pending, | ||
| }; | ||
|
|
||
| const isInProgress: StatusCondition = { | ||
| type: JobFilterType.Status, | ||
| operator: ColumnOperator.Equals, | ||
| value: JobStatus.InProgress, | ||
| }; | ||
|
|
||
| const isSameRelease: ReleaseCondition = { | ||
| type: JobFilterType.Release, | ||
| operator: ColumnOperator.Equals, | ||
| value: releaseId, | ||
| }; | ||
|
|
||
| const isDifferentRelease: JobCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| not: true, | ||
| conditions: [isSameRelease], | ||
| }; | ||
|
|
||
| const pendingJobsForCurrentReleaseAndEnvFilter: JobCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| conditions: [isSameEnvironment, isPending, isSameRelease], | ||
| }; | ||
|
|
||
| const inProgressJobsForDifferentReleaseAndCurrentEnvFilter: JobCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| conditions: [isSameEnvironment, isInProgress, isDifferentRelease], | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor condition objects into reusable functions
The condition objects defined between lines 53 and 95 have similar structures. Consider refactoring them into reusable helper functions to reduce code duplication and enhance maintainability.
You could create helper functions like createEnvironmentCondition, createStatusCondition, and createReleaseCondition:
function createEnvironmentCondition(environmentId: string): EnvironmentCondition {
return {
type: JobFilterType.Environment,
operator: ColumnOperator.Equals,
value: environmentId,
};
}
function createStatusCondition(status: JobStatus): StatusCondition {
return {
type: JobFilterType.Status,
operator: ColumnOperator.Equals,
value: status,
};
}
function createReleaseCondition(releaseId: string): ReleaseCondition {
return {
type: JobFilterType.Release,
operator: ColumnOperator.Equals,
value: releaseId,
};
}Then refactor the condition objects:
- const isSameEnvironment: EnvironmentCondition = {
- type: JobFilterType.Environment,
- operator: ColumnOperator.Equals,
- value: environmentId,
- };
+ const isSameEnvironment = createEnvironmentCondition(environmentId);
- const isPending: StatusCondition = {
- type: JobFilterType.Status,
- operator: ColumnOperator.Equals,
- value: JobStatus.Pending,
- };
+ const isPending = createStatusCondition(JobStatus.Pending);
- const isInProgress: StatusCondition = {
- type: JobFilterType.Status,
- operator: ColumnOperator.Equals,
- value: JobStatus.InProgress,
- };
+ const isInProgress = createStatusCondition(JobStatus.InProgress);
- const isSameRelease: ReleaseCondition = {
- type: JobFilterType.Release,
- operator: ColumnOperator.Equals,
- value: releaseId,
- };
+ const isSameRelease = createReleaseCondition(releaseId);| const pendingJobsQ = api.job.config.byWorkspaceId.list.useQuery( | ||
| { | ||
| workspaceId, | ||
| filter: pendingJobsForCurrentReleaseAndEnvFilter, | ||
| limit: 1, | ||
| }, | ||
| { refetchInterval: 5_000 }, | ||
| ); | ||
|
|
||
| const isReleasePending = allJobs.data?.some( | ||
| (j) => | ||
| j.job.status === JobStatus.Pending && | ||
| j.release.id === releaseId && | ||
| j.environmentId === environmentId, | ||
| const inProgressJobsQ = api.job.config.byWorkspaceId.list.useQuery( | ||
| { | ||
| workspaceId, | ||
| filter: inProgressJobsForDifferentReleaseAndCurrentEnvFilter, | ||
| limit: 1, | ||
| }, | ||
| { refetchInterval: 5_000 }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize queries by combining them
Currently, there are two separate queries fetching pending and in-progress jobs. Consider combining these queries into a single query to reduce network requests and improve performance.
If the API supports fetching multiple statuses, you could adjust the filter:
const combinedStatusCondition: StatusCondition = {
type: JobFilterType.Status,
operator: ColumnOperator.In,
value: [JobStatus.Pending, JobStatus.InProgress],
};
const combinedJobsFilter: JobCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: [isSameEnvironment, combinedStatusCondition],
};
- const pendingJobsQ = api.job.config.byWorkspaceId.list.useQuery(
- {
- workspaceId,
- filter: pendingJobsForCurrentReleaseAndEnvFilter,
- limit: 1,
- },
- { refetchInterval: 5_000 },
- );
- const inProgressJobsQ = api.job.config.byWorkspaceId.list.useQuery(
- {
- workspaceId,
- filter: inProgressJobsForDifferentReleaseAndCurrentEnvFilter,
- limit: 1,
- },
- { refetchInterval: 5_000 },
- );
+ const jobsQ = api.job.config.byWorkspaceId.list.useQuery(
+ {
+ workspaceId,
+ filter: combinedJobsFilter,
+ limit: 1,
+ },
+ { refetchInterval: 5_000 },
+ );Update the loading and condition checks accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| const loading = pendingJobsQ.isLoading || inProgressJobsQ.isLoading; | ||
|
|
There was a problem hiding this comment.
Handle potential errors from queries
The current implementation checks for the loading state but doesn't handle errors that might occur during the queries. Incorporating error handling will improve robustness and user experience.
Modify the code to handle errors:
const loading = pendingJobsQ.isLoading || inProgressJobsQ.isLoading;
+ const error = pendingJobsQ.isError || inProgressJobsQ.isError;
...
return (
<div className="flex items-center gap-2">
+ {error && <ErrorComponent message="Failed to load job data" />}
{loading && <Loading />}
{!loading && isWaitingOnActive && (
<>
<Waiting /> Another release is in progress
</>
)}
{!loading && !isWaitingOnActive && (
<>
<Passing /> All other releases finished
</>
)}
</div>
);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
FlowDiagramcomponent to include workspace-specific information.ReleaseSequencingNodecomponent.ReleaseConditiontype for improved job condition evaluations.Bug Fixes
WaitingOnActiveCheckcomponent.Documentation
ReleaseConditionfor better type-checking.Chores
release-condition.jsto enhance modularity in job conditions.