fix: Remove unnecessary concurrency column and remove save button for concurrency section#270
Conversation
… concurrency section
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 12 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. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces changes to the environment policy management system across multiple files. The modifications primarily focus on refactoring the concurrency and policy invalidation mechanisms. Key changes include removing the Changes
Sequence DiagramsequenceDiagram
participant User
participant DeploymentControl
participant useInvalidatePolicy
participant API
User->>DeploymentControl: Modify concurrency limit
DeploymentControl->>API: Mutate environment policy
API-->>DeploymentControl: Policy updated
DeploymentControl->>useInvalidatePolicy: Invalidate policy cache
useInvalidatePolicy->>API: Clear cached policy data
Possibly related PRs
Suggested reviewers
Poem
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx (2)
16-22: Consider improving type safety and null handling.The state initialization could be more explicit and handle the nullable concurrencyLimit more elegantly.
- const [concurrencyLimit, setConcurrencyLimit] = useState( - environmentPolicy.concurrencyLimit?.toString() ?? "", - ); + const [concurrencyLimit, setConcurrencyLimit] = useState<string>(() => + environmentPolicy.concurrencyLimit != null + ? String(environmentPolicy.concurrencyLimit) + : "" + );
57-87: Improve accessibility and code organization.The radio group implementation could benefit from better accessibility and code organization.
<RadioGroup value={environmentPolicy.concurrencyLimit != null ? "some" : "all"} - onValueChange={(value) => { - const concurrencyLimit = value === "some" ? 1 : null; - setConcurrencyLimit(String(concurrencyLimit ?? "")); - updatePolicy - .mutateAsync({ id, data: { concurrencyLimit } }) - .then(invalidatePolicy); - }} + onValueChange={handleConcurrencyTypeChange} > <div className="flex items-center space-x-3 space-y-0"> <RadioGroupItem value="all" /> <Label className="flex items-center gap-2 font-normal"> All jobs can run concurrently </Label> </div> <div className="flex items-center space-x-3 space-y-0"> <RadioGroupItem value="some" className="min-w-4" /> <Label className="flex flex-wrap items-center gap-2 font-normal"> A maximum of <Input + aria-label="Maximum concurrent jobs" disabled={environmentPolicy.concurrencyLimit == null} type="number" + min="1" value={concurrencyLimit} onChange={(e) => setConcurrencyLimit(e.target.value)} className="border-b-1 h-6 w-16 text-xs" /> jobs can run concurrently </Label> </div> </RadioGroup>Extract the handler to improve readability:
const handleConcurrencyTypeChange = (value: string) => { const limit = value === "some" ? 1 : null; setConcurrencyLimit(String(limit ?? "")); updatePolicy .mutateAsync({ id, data: { concurrencyLimit: limit } }) .then(() => { invalidatePolicy(); toast.success("Concurrency type updated successfully"); }) .catch(() => toast.error("Failed to update concurrency type")); };packages/db/drizzle/0047_square_william_stryker.sql (1)
1-2: Simplify NULL constraintsSetting the default to NULL and then dropping NOT NULL constraint is redundant. You can achieve the same result with just dropping the NOT NULL constraint.
-ALTER TABLE "environment_policy" ALTER COLUMN "concurrency_limit" SET DEFAULT NULL;--> statement-breakpoint -ALTER TABLE "environment_policy" ALTER COLUMN "concurrency_limit" DROP NOT NULL;--> statement-breakpoint +ALTER TABLE "environment_policy" ALTER COLUMN "concurrency_limit" DROP NOT NULL;--> statement-breakpointpackages/job-dispatch/src/policies/concurrency-policy.ts (1)
Line range hint
1-93: Consider adding documentation for the SQL query structure.The SQL query construction is quite complex with multiple joins and subqueries. Consider adding inline comments explaining:
- Purpose of the active job subquery
- The grouping logic
- The chain operations on the result
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx (1)
16-16: LGTM! Clean refactoring of policy invalidation.The introduction of
useInvalidatePolicyhook improves code organization and follows React best practices.Consider adding a comment explaining the purpose of the
useInvalidatePolicyhook for better documentation:+// Hook to handle policy cache invalidation after updates import { useInvalidatePolicy } from "./useInvalidatePolicy";Also applies to: 22-23
packages/db/src/schema/environment.ts (1)
125-125: Add documentation for nullable concurrencyLimit.Consider adding a comment explaining why
concurrencyLimitis nullable and its implications for concurrency control.+ // When null, no concurrency limit is applied concurrencyLimit: integer("concurrency_limit").default(sql`NULL`),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useInvalidatePolicy.ts(1 hunks)apps/webservice/src/app/api/v1/environments/[environmentId]/openapi.ts(1 hunks)packages/db/drizzle/0047_square_william_stryker.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment.ts(1 hunks)packages/job-dispatch/src/job-creation.ts(1 hunks)packages/job-dispatch/src/policies/concurrency-policy.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useInvalidatePolicy.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/job-dispatch/src/job-creation.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/job-dispatch/src/policies/concurrency-policy.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/environment.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/environments/[environmentId]/openapi.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx (2)
1-15: LGTM! Clean imports and well-typed component setup.
1-89: Implementation aligns well with PR objectives.
The changes successfully remove the save button and implement a more streamlined approach with automatic updates. The use of debouncing and immediate feedback improves the user experience while maintaining data consistency.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useInvalidatePolicy.ts (1)
5-14: LGTM! Clean and focused hook implementation.
The hook follows React conventions, properly manages cache invalidation, and has a clear single responsibility.
packages/job-dispatch/src/policies/concurrency-policy.ts (1)
80-81: LGTM! Simplified concurrency check logic.
The change from checking concurrencyType to directly checking concurrencyLimit makes the code more straightforward while maintaining the same functionality.
apps/webservice/src/app/api/v1/environments/[environmentId]/openapi.ts (1)
73-73: LGTM! Verify impact on API consumers.
The schema changes align with the PR objective. The concurrencyLimit is now nullable, and concurrencyType has been removed.
Let's verify the impact on API consumers:
✅ Verification successful
Schema changes are safe and handled properly across the codebase
The codebase already handles nullable concurrencyLimit appropriately:
- UI component in
DeploymentControl.tsxproperly handles null values and type conversions - Database schema in
environment.tshas default NULL value - Job dispatch logic in
concurrency-policy.tsexplicitly checks for null values - Node SDK's schema in
schema.tsalready defines it as optional
The removal of concurrencyType is also safe as it's only referenced in the SDK schema as an optional field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the removed concurrencyType field
rg -l "concurrencyType" --type ts --type tsx
# Search for places that might break due to nullable concurrencyLimit
rg "concurrencyLimit\s*[:]\s*number" --type ts --type tsx
Length of output: 160
Script:
#!/bin/bash
# Search for direct usage of the removed concurrencyType field
rg -t ts "concurrencyType"
# Search for places that might break due to nullable concurrencyLimit
rg -t ts "concurrencyLimit\s*[:]\s*number"
# Search for any usage of concurrencyLimit to understand the impact
rg -t ts "concurrencyLimit" -A 2
Length of output: 6044
packages/job-dispatch/src/job-creation.ts (1)
100-103: Verify behavior when concurrencyLimit is null.
The concurrency check has been simplified, but we should ensure this doesn't affect existing job dispatch behavior.
Let's check for potential edge cases:
Consider adding a comment explaining the concurrency check logic:
+ // Check if job should wait based on environment's concurrency limit
isWaitingOnConcurrencyRequirementInSameRelease = and(
isNotNull(schema.environmentPolicy.concurrencyLimit),
eq(schema.environment.id, triggers.release_job_trigger.environmentId),
eq(schema.releaseJobTrigger.releaseId, triggers.release.id),
eq(schema.job.status, JobStatus.Pending),
);✅ Verification successful
Let me gather more information about the concurrency behavior by searching for test files and implementation details.
Let me search for more context about the environment policy and job status handling.
Let me search for the database schema definition to understand how concurrencyLimit is defined.
Based on the gathered evidence, I can now provide the final response:
Behavior with null concurrencyLimit is correctly handled
The concurrency check implementation is safe and intentional:
- When concurrencyLimit is null, the
isWaitingOnConcurrencyRequirementInSameReleasecondition evaluates to false due to theisNotNullcheck - This aligns with the UI implementation where null concurrencyLimit represents unlimited concurrent jobs
- The concurrency policy implementation in
concurrency-policy.tsexplicitly checks for non-null values before applying limits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering concurrency behavior
rg -A 5 "describe.*concurrency" --type test
# Look for other places that might be affected by null concurrencyLimit
ast-grep --pattern 'concurrencyLimit'
Length of output: 1220
Script:
#!/bin/bash
# Search for test files with .test or .spec extension
fd -e test.ts -e spec.ts --exec rg -A 5 "concurrency|ConcurrencyType"
# Look for implementation of job dispatch logic
rg -A 10 "isNotNull.*concurrencyLimit|ConcurrencyType"
# Search for schema definitions related to environment policy
ast-grep --pattern 'interface EnvironmentPolicy'
Length of output: 1029
Script:
#!/bin/bash
# Search for environment policy type definitions
rg -A 10 "type.*EnvironmentPolicy|interface.*EnvironmentPolicy"
# Look for job status handling with concurrency
rg -B 5 -A 10 "JobStatus.Pending.*concurrency"
# Search for database schema or model definitions
ast-grep --pattern 'model EnvironmentPolicy'
Length of output: 8238
Script:
#!/bin/bash
# Search for environment policy table definition
rg -A 15 "environmentPolicy.*=.*pgTable"
# Look for the actual implementation where concurrencyLimit is used in job dispatch
rg -B 5 -A 10 "isWaitingOnConcurrencyRequirement"
# Search for any validation or checks on concurrencyLimit
rg -A 5 "concurrencyLimit.*null"
Length of output: 11806
packages/db/drizzle/meta/_journal.json (1)
333-339: LGTM! Migration entry is properly structured.
The new migration entry follows the established pattern and includes all required fields.
packages/db/src/schema/environment.ts (1)
125-125: Verify handling of nullable concurrencyLimit across the codebase.
The change to make concurrencyLimit nullable is aligned with the PR objectives. However, this is a breaking schema change that requires careful verification.
Let's verify the handling of null values:
✅ Verification successful
Proper null handling is already implemented across the codebase
The codebase already handles null values for concurrencyLimit appropriately:
- UI component (
DeploymentControl.tsx) properly handles both null and non-null values:- Uses optional chaining for initial value
- Implements explicit null handling in state management
- Includes UI controls to toggle between null and numeric values
- Backend validation (
job-creation.ts) usesisNotNullcheck - API schema (
openapi.ts) correctly defines it as nullable - Concurrency policy implementation (
concurrency-policy.ts) includes null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for direct usage of concurrencyLimit to ensure proper null handling
# Search for direct field access
rg -t typescript "concurrencyLimit" --glob '!packages/db/src/schema/**'
# Search for potential SQL queries that might need NULL checks
rg -t typescript "SELECT.*concurrency_limit|WHERE.*concurrency_limit" --glob '!packages/db/src/schema/**'
Length of output: 249
Script:
#!/bin/bash
# Let's try without the typescript file type flag and use more specific patterns
# Search for direct field access of concurrencyLimit
rg "concurrencyLimit" --glob '!packages/db/src/schema/**'
# Search for SQL queries with concurrency_limit
rg "concurrency_limit" --glob '!packages/db/src/schema/**'
# Find TypeScript/JavaScript files that might use this field
fd "\.(ts|js)$" --exec grep -l "concurrencyLimit\|concurrency_limit" {} \;
Length of output: 11014
| useDebounce( | ||
| () => { | ||
| if (concurrencyLimit === "") return; | ||
| const limit = Number(concurrencyLimit); | ||
| if (Number.isNaN(limit)) return; | ||
| updatePolicy | ||
| .mutateAsync({ id, data: { concurrencyLimit: limit } }) | ||
| .then(invalidatePolicy) | ||
| .catch((e) => toast.error(e.message)); | ||
| }, | ||
| 300, | ||
| [concurrencyLimit], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and user feedback.
Consider adding better validation and user feedback for a better user experience.
useDebounce(
() => {
if (concurrencyLimit === "") return;
const limit = Number(concurrencyLimit);
- if (Number.isNaN(limit)) return;
+ if (Number.isNaN(limit) || limit < 1) {
+ toast.error("Please enter a valid number greater than 0");
+ return;
+ }
updatePolicy
.mutateAsync({ id, data: { concurrencyLimit: limit } })
- .then(invalidatePolicy)
- .catch((e) => toast.error(e.message));
+ .then(() => {
+ invalidatePolicy();
+ toast.success("Concurrency limit updated successfully");
+ })
+ .catch((e) => toast.error("Failed to update concurrency limit. Please try again."));
},
300,
[concurrencyLimit],
);📝 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.
| useDebounce( | |
| () => { | |
| if (concurrencyLimit === "") return; | |
| const limit = Number(concurrencyLimit); | |
| if (Number.isNaN(limit)) return; | |
| updatePolicy | |
| .mutateAsync({ id, data: { concurrencyLimit: limit } }) | |
| .then(invalidatePolicy) | |
| .catch((e) => toast.error(e.message)); | |
| }, | |
| 300, | |
| [concurrencyLimit], | |
| ); | |
| useDebounce( | |
| () => { | |
| if (concurrencyLimit === "") return; | |
| const limit = Number(concurrencyLimit); | |
| if (Number.isNaN(limit) || limit < 1) { | |
| toast.error("Please enter a valid number greater than 0"); | |
| return; | |
| } | |
| updatePolicy | |
| .mutateAsync({ id, data: { concurrencyLimit: limit } }) | |
| .then(() => { | |
| invalidatePolicy(); | |
| toast.success("Concurrency limit updated successfully"); | |
| }) | |
| .catch((e) => toast.error("Failed to update concurrency limit. Please try again.")); | |
| }, | |
| 300, | |
| [concurrencyLimit], | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openapi.v1.json (1)
214-215: Consider adding deprecation noticesWhile removing unnecessary functionality is good, consider adding deprecation notices in the API responses for a transition period. This would help API consumers adapt to the changes gracefully.
Consider:
- Adding a deprecation header in API responses
- Including migration guides in API documentation
- Setting up monitoring for usage of deprecated fields
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openapi.v1.json(1 hunks)packages/node-sdk/src/schema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/node-sdk/src/schema.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (2)
packages/node-sdk/src/schema.ts (1)
581-581:
Verify the impact of making concurrencyLimit nullable
The change to make concurrencyLimit nullable is a breaking change that could affect API consumers. We should ensure all consumers are updated to handle the null case.
✅ Verification successful
Based on the search results, I can now generate the final response:
Making concurrencyLimit nullable is safe and properly handled
The codebase already handles the nullable concurrencyLimit appropriately:
- The database schema in
packages/db/src/schema/environment.tsdefaults it to NULL - The job dispatch logic in
packages/job-dispatch/src/policies/concurrency-policy.tsexplicitly checks for null - The UI component in
DeploymentControl.tsxproperly handles both null and non-null cases with appropriate null checks - The OpenAPI spec in
openapi.tscorrectly defines it as nullable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for direct usage of concurrencyLimit to identify potential breaking changes
# Search for concurrencyLimit usage in TypeScript/JavaScript files
rg -t ts -t js 'concurrencyLimit' -A 3
# Search for type checks or null checks on concurrencyLimit
rg -t ts -t js 'concurrencyLimit.*\?' -A 3
Length of output: 9066
openapi.v1.json (1)
214-215: Verify schema changes align with database updates
The change to make concurrencyLimit nullable in the OpenAPI spec aligns with the TypeScript interface changes. This is part of removing the unnecessary concurrency column.
✅ Verification successful
Based on the search results, I can see that the database schema in packages/db/src/schema/environment.ts shows that concurrencyLimit is defined as a nullable integer column:
concurrencyLimit: integer("concurrency_limit").default(sql`NULL`),This aligns with the OpenAPI spec change to make concurrencyLimit nullable. The codebase also shows consistent handling of nullable concurrency limits in various components and policies, particularly in DeploymentControl.tsx and concurrency-policy.ts.
OpenAPI schema change is correctly aligned with database schema
The change to make concurrencyLimit nullable in the OpenAPI spec matches the database schema definition and is consistently implemented across the codebase. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify database schema changes for concurrency columns
# Search for database migration files that modify concurrency columns
fd -e sql -e ts 'concurrency' -x cat {} \; -x echo "---" \;
# Search for database schema definitions related to concurrency
rg -t ts 'concurrency.*Column' -A 3
Length of output: 3000
Script:
#!/bin/bash
# Let's search for database schema and migration files more broadly
# Search for migration files
fd migration -e sql -e ts -x cat {} \; -x echo "---" \;
# Search for environment policy schema definitions
rg -t ts "environmentPolicy|concurrencyLimit" -A 5
# Search specifically in schema definition files
fd schema -e ts -x cat {} \; -x echo "---" \;
Length of output: 69327
Summary by CodeRabbit
Release Notes
New Features
DeploymentControlcomponent for a more reactive state management approach, enhancing user responsiveness.Bug Fixes
Documentation
Chores