feat: concurrency rule #579
Conversation
WalkthroughA concurrency rule feature was introduced throughout the policy management system. This includes database schema changes, API and OpenAPI updates, backend logic for concurrency enforcement, and frontend UI enhancements for displaying and editing concurrency settings within policies. The change spans the database, rule engine, API, and web application layers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant API
participant DB
participant RuleEngine
User->>WebApp: Edit/Create Policy (set concurrency)
WebApp->>API: PATCH/POST /policies/{id} { concurrency }
API->>DB: Upsert policy and policy_rule_concurrency
DB-->>API: Store concurrency value
API-->>WebApp: Return updated policy with concurrency
Note over WebApp,API: Policy now includes concurrency info
User->>WebApp: View Policy Table/Details
WebApp->>API: GET /policies
API->>DB: Query policies with concurrency relation
DB-->>API: Return policies with concurrency
API-->>WebApp: Return data
WebApp-->>User: Show concurrency badge/info
Note over RuleEngine,DB: On job completion or evaluation
RuleEngine->>DB: Query policies and concurrency rules for release target
DB-->>RuleEngine: Return applicable policies
RuleEngine->>DB: Count active jobs in concurrency group
DB-->>RuleEngine: Return count
RuleEngine-->>API: Allow or block job based on concurrency limit
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (2)
packages/job-dispatch/src/job-update.ts (1)
121-178: Review complex concurrency group query logic.The
getReleaseTargetsInConcurrencyGroupfunction implements correct logic but uses a complex two-step query approach:
- First query finds policies with concurrency rules matching the given release target
- Second query finds other release targets associated with those policies
Consider optimizing with a single query if performance becomes an issue:
-- Potential single query optimization SELECT DISTINCT rt.* FROM release_target rt JOIN computed_policy_target_release_target cptrt ON rt.id = cptrt.release_target_id JOIN policy_target pt ON cptrt.policy_target_id = pt.id JOIN policy_rule_concurrency prc ON pt.policy_id = prc.policy_id WHERE rt.id != ? AND pt.policy_id IN ( SELECT DISTINCT pt2.policy_id FROM computed_policy_target_release_target cptrt2 JOIN policy_target pt2 ON cptrt2.policy_target_id = pt2.id JOIN policy_rule_concurrency prc2 ON pt2.policy_id = prc2.policy_id WHERE cptrt2.release_target_id = ? )However, the current approach is clear and maintainable.
openapi.v1.json (1)
5862-5881: Define newPolicyConcurrencyschema
ThePolicyConcurrencyobject is well-scoped withname,description, andconcurrency, andadditionalPropertiesis locked down. Sinceconcurrencyrepresents a count, consider switching its type tointegerand adding a description for clarity:"PolicyConcurrency": { "type": "object", "properties": { "name": { "type": "string" }, "description": { "type": "string" }, - "concurrency": { - "type": "number", + "concurrency": { + "type": "integer", "minimum": 1 + "description": "Maximum number of concurrent releases allowed" } }, "required": [ "name", "concurrency" ], "additionalProperties": false },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/policies/openapi.ts(2 hunks)openapi.v1.json(3 hunks)packages/db/drizzle/0109_loose_ben_grimm.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/schema/rules/concurrency.ts(1 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/rule-relations.ts(2 hunks)packages/job-dispatch/src/job-update.ts(4 hunks)packages/rule-engine/src/db/create-policy.ts(2 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(2 hunks)packages/rule-engine/src/rules/concurrency-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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/rules/index.tspackages/rule-engine/src/types.tspackages/db/src/schema/policy-relations.tspackages/db/src/schema/policy.tsapps/webservice/src/app/api/v1/policies/openapi.tspackages/rule-engine/src/db/create-policy.tspackages/db/src/schema/rules/rule-relations.tspackages/rule-engine/src/manager/version-manager-rules.tspackages/rule-engine/src/rules/concurrency-rule.tspackages/db/src/schema/rules/concurrency.tsapps/webservice/src/app/api/v1/policies/[policyId]/openapi.tspackages/job-dispatch/src/job-update.tspackages/rule-engine/src/db/update-policy.ts
🧬 Code Graph Analysis (6)
packages/rule-engine/src/types.ts (1)
packages/db/src/schema/rules/concurrency.ts (1)
PolicyRuleConcurrency(21-21)
packages/db/src/schema/policy-relations.ts (1)
packages/db/src/schema/rules/concurrency.ts (1)
policyRuleConcurrency(7-19)
packages/db/src/schema/policy.ts (1)
packages/db/src/schema/rules/concurrency.ts (1)
createPolicyRuleConcurrency(23-25)
packages/rule-engine/src/db/create-policy.ts (1)
packages/db/src/common.ts (1)
buildConflictUpdateColumns(30-46)
packages/db/src/schema/rules/rule-relations.ts (2)
packages/db/src/schema/rules/concurrency.ts (1)
policyRuleConcurrency(7-19)packages/db/src/schema/policy.ts (1)
policy(43-63)
packages/db/src/schema/rules/concurrency.ts (2)
packages/db/src/schema/rules/base.ts (1)
basePolicyRuleFields(7-17)packages/db/src/schema/policy.ts (1)
policy(43-63)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (26)
packages/db/drizzle/meta/_journal.json (1)
768-774: LGTM! Migration journal entry is properly formatted.The new migration entry follows the established pattern and correctly tracks the concurrency table creation migration.
packages/rule-engine/src/types.ts (1)
72-72: LGTM! Concurrency property properly added to Policy type.The nullable type ensures backward compatibility, and the property follows the established pattern of other policy rule properties.
packages/db/src/schema/rules/index.ts (1)
12-12: LGTM! Export addition follows existing patterns.The concurrency module export is properly integrated following the established convention of other rule exports.
packages/db/src/schema/policy-relations.ts (2)
9-9: LGTM! Import follows established pattern.The import for
policyRuleConcurrencyis properly placed with other rule imports and uses correct ES module syntax.
32-32: LGTM! One-to-one relation is appropriately defined.The concurrency relation correctly uses
one()which aligns with the uniquepolicyIdconstraint in thepolicyRuleConcurrencytable schema.packages/db/src/schema/rules/rule-relations.ts (1)
18-18: LGTM! Consistent with existing patterns.The import and relation definition follow the established patterns in the file. The one-to-one relationship is correctly configured using the
policyIdfield that referencespolicy.id.Also applies to: 132-140
apps/webservice/src/app/api/v1/policies/openapi.ts (2)
70-79: Well-designed schema with appropriate constraints.The
PolicyConcurrencyschema is properly structured with:
- Required fields for essential properties (
name,concurrency)- Minimum value constraint on
concurrencyto prevent invalid valuesadditionalProperties: falsefor schema validation security
112-114: Correct integration of concurrency into Policy schema.Adding
concurrencyas an optional property maintains backward compatibility while enabling the new feature.apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts (1)
80-82: Consistent addition to PATCH endpoint.The
concurrencyproperty is correctly added as an optional field in the request body, maintaining consistency with other policy properties.packages/rule-engine/src/db/create-policy.ts (1)
95-95: Correct extraction of concurrency from input.The destructuring follows the established pattern for other policy rules.
packages/db/src/schema/policy.ts (3)
38-38: LGTM: Clean import addition.The import of
createPolicyRuleConcurrencyfollows the established pattern for other policy rule imports.
152-159: LGTM: Proper concurrency validation.The concurrency field implementation follows the established pattern for optional policy rules with appropriate validation:
- Uses
omit({ policyId: true })consistently with other rule fields- Validates concurrency > 0 when provided
- Properly handles nullable/optional cases
186-193: LGTM: Consistent validation in update schema.The concurrency field in
updatePolicymirrors the validation logic fromcreatePolicy, ensuring consistency across create and update operations.packages/db/drizzle/0109_loose_ben_grimm.sql (1)
1-11: LGTM: Well-structured database migration.The migration properly creates the concurrency rule table with:
- Appropriate UUID primary key with default generation
- Unique constraint on
policy_id(ensures one concurrency rule per policy)- Foreign key with cascade delete (maintains referential integrity)
- Sensible default value of 1 for concurrency limit
- Standard audit fields (created_at)
The table structure aligns well with the concurrency feature requirements.
packages/job-dispatch/src/job-update.ts (3)
3-11: LGTM: Import additions support concurrency query needs.The additional SQL helper functions (
and,inArray,ne) are necessary for the new concurrency group query logic.
101-119: LGTM: Simplified getReleaseTarget function.The change from selecting specific columns to
select()(all columns) simplifies the query while providing more complete data. The explicit return ofrow?.release_target ?? nullmaintains the same contract.
222-228: LGTM: Proper concurrency group evaluation trigger.After a job completes, the code correctly:
- Enqueues the job's release target for evaluation
- Finds related release targets in the same concurrency group
- Enqueues each related target for evaluation
This ensures that concurrency constraints are properly re-evaluated when jobs complete and potentially free up concurrency slots.
packages/db/src/schema/rules/concurrency.ts (2)
1-6: LGTM: Proper imports and dependencies.The imports correctly include necessary Drizzle ORM components, base policy rule fields, and the policy table reference.
21-25: LGTM: Proper type and schema exports.The type inference and insert schema creation follow established patterns and provide the necessary exports for the concurrency rule functionality.
packages/rule-engine/src/db/update-policy.ts (2)
138-152: LGTM! Clean implementation following established patterns.The
updateConcurrencyfunction follows the same pattern as other update functions in this file. The implementation correctly handles null checks, deletes existing records, and inserts new data within the transaction.
166-166: Proper integration of concurrency into the update workflow.The concurrency field is correctly:
- Extracted from the input object (line 166)
- Updated in parallel with other policy components (line 191)
- Included in the query to return updated data (line 203)
Also applies to: 191-191, 203-203
packages/rule-engine/src/manager/version-manager-rules.ts (2)
75-104: Well-structured concurrency rule implementation.The
getConcurrencyRulefunction follows good practices:
- Proper null checks with early return
- Clean separation of concerns with the helper function
- Logical SQL joins to traverse the relationship from policy to release targets
The database query correctly joins the necessary tables to find release targets associated with the policy's concurrency group.
112-112: Proper integration into the rules system.The concurrency rule is correctly added to the existing rules array, maintaining the established pattern.
packages/rule-engine/src/rules/concurrency-rule.ts (1)
16-36:❓ Verification inconclusive
Critical issue: Query counts all jobs, not just active ones.
The current query counts all jobs ever associated with the release targets, not just currently active jobs. This would make concurrency limiting ineffective since completed jobs would still count toward the limit.
Add a WHERE clause to filter for only active jobs:
async getNumberOfActiveJobs(releaseTargetsIds: string[]) { + if (releaseTargetsIds.length === 0) return 0; + return db .select({ count: count() }) .from(schema.job) .innerJoin(schema.releaseJob, eq(schema.job.id, schema.releaseJob.jobId)) .innerJoin( schema.release, eq(schema.releaseJob.releaseId, schema.release.id), ) .innerJoin( schema.versionRelease, eq(schema.release.versionReleaseId, schema.versionRelease.id), ) .innerJoin( schema.releaseTarget, eq(schema.versionRelease.releaseTargetId, schema.releaseTarget.id), ) - .where(inArray(schema.releaseTarget.id, releaseTargetsIds)) + .where( + and( + inArray(schema.releaseTarget.id, releaseTargetsIds), + eq(schema.job.status, "active") // Adjust based on your job status enum + ) + ) .then(takeFirst) .then(({ count }) => count); }Please verify what values are used for active job status in your schema:
🏁 Script executed:
#!/bin/bash # Description: Find job status values in the schema to determine correct filter ast-grep --pattern 'status: $$$'Length of output: 34
I couldn’t locate the
statusfield in the schema with the previous command. Let’s pinpoint the job schema file and inspect it directly:#!/bin/bash # 1. Find the file that exports `schema.job` job_schema_file=$(rg -l "export const job" -n . | head -n1) echo "Job schema file: $job_schema_file" # 2. Search for any `status` definitions or enums in that file echo "Searching for 'status' in $job_schema_file:" grep -n "status" "$job_schema_file" || echo "No 'status' found in $job_schema_file" # 3. As a fallback, list all enums in schema directory echo "Listing enums in schema directory:" rg -n "export enum" -g "packages/rule-engine/src/schema/*.ts"
Let’s locate the job schema and inspect any status field or enums:
#!/bin/bash # 1. Find the job schema file that defines schema.job job_file=$(rg -l "schema\\.job" -g "packages/**/src/schema/*.ts" | head -n1) echo "Job schema file: $job_file" # 2. Check for a status property or related enum in that file echo "Searching for 'status' in $job_file:" grep -n "status" "$job_file" || echo "No 'status' found in $job_file" # 3. As a fallback, list all enum definitions in the schema directory echo "All enums in schema directory:" rg -n "export enum" -g "packages/**/src/schema/*.ts"
Filter by active job status and handle empty targets
The current
getNumberOfActiveJobsquery counts all jobs for the given release targets, not just those still running. That will break your concurrency limits once jobs complete. Also, ifreleaseTargetsIdsis empty, theinArrayfilter may behave unexpectedly.Proposed changes:
async getNumberOfActiveJobs(releaseTargetsIds: string[]) { + // Return zero when there are no targets + if (releaseTargetsIds.length === 0) { + return 0; + } return db .select({ count: count() }) .from(schema.job) .innerJoin(schema.releaseJob, eq(schema.job.id, schema.releaseJob.jobId)) .innerJoin(schema.release, eq(schema.releaseJob.releaseId, schema.release.id)) .innerJoin( schema.versionRelease, eq(schema.release.versionReleaseId, schema.versionRelease.id), ) .innerJoin( schema.releaseTarget, eq(schema.versionRelease.releaseTargetId, schema.releaseTarget.id), ) - .where(inArray(schema.releaseTarget.id, releaseTargetsIds)) + .where( + and( + inArray(schema.releaseTarget.id, releaseTargetsIds), + // Only count jobs with the “active” status + eq(schema.job.status, /* TODO: insert your active-status value here */) + ) + ) .then(takeFirst) .then(({ count }) => count); }• Verify the exact field name and value used in your schema for a running/active job (e.g.
schema.job.status === "running"or similar).
• Ensure the filter matches that enum or literal so only in-flight jobs are counted.
• Confirm thatinArrayon an empty array returns an empty set (or guard as shown above).openapi.v1.json (2)
1916-1920: Add concurrency property to policy update schema
ThePATCH /v1/policies/{policyId}request schema now correctly includes aconcurrencyfield referencing thePolicyConcurrencyschema. This integrates the concurrency rule into policy updates without breaking existing fields.
5938-5942: Include concurrency inPolicy1response schema
ThePolicy1schema now surfaces theconcurrencyproperty, ensuring that clients see the configured concurrency settings when a policy is created.
| export const policyRuleConcurrency = pgTable("policy_rule_concurrency", { | ||
| ...basePolicyRuleFields, | ||
|
|
||
| name: text("name").notNull(), | ||
| description: text("description"), | ||
|
|
||
| policyId: uuid("policy_id") | ||
| .notNull() | ||
| .unique() | ||
| .references(() => policy.id, { onDelete: "cascade" }), | ||
|
|
||
| concurrency: integer("concurrency").notNull().default(1), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential policyId field conflict.
The schema redefines policyId with a unique constraint (lines 13-16) even though basePolicyRuleFields already includes a policyId field. This redefinition might override the base field unexpectedly.
Consider using table constraints instead of redefining the field:
export const policyRuleConcurrency = pgTable("policy_rule_concurrency", {
...basePolicyRuleFields,
name: text("name").notNull(),
description: text("description"),
- policyId: uuid("policy_id")
- .notNull()
- .unique()
- .references(() => policy.id, { onDelete: "cascade" }),
concurrency: integer("concurrency").notNull().default(1),
-});
+}, (t) => ({
+ uniquePolicyId: unique().on(t.policyId),
+}));This approach maintains the base field definition while adding the unique constraint.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/db/src/schema/rules/concurrency.ts between lines 7 and 19, avoid
redefining the policyId field since it is already included in
basePolicyRuleFields. Remove the policyId field redefinition and instead add a
unique constraint on policyId at the table level. This preserves the original
field definition and applies the uniqueness requirement without overriding the
base field.
There was a problem hiding this comment.
this is fine because with typescript it will overwrite which is the intended behavior
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification! You're absolutely right - the TypeScript field overwriting behavior is exactly what's needed here. Spreading basePolicyRuleFields and then redefining policyId with the unique constraint and foreign key reference is a clean way to extend the base schema with more specific requirements.
I appreciate you explaining the intended behavior. This pattern makes perfect sense for customizing base field definitions while maintaining the schema structure.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.512Z
Learning: In the ctrlplane codebase, when defining database schemas with Drizzle ORM, it's an intentional pattern to spread base fields (like `basePolicyRuleFields`) and then redefine specific fields to add additional constraints (like unique constraints or foreign key references). The TypeScript field overwriting behavior is deliberately used to override base field definitions with more specific requirements.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| type ConcurrencyRuleOptions = { | ||
| concurrency: number; | ||
| getReleaseTargetsInConcurrencyGroup: () => Promise<schema.ReleaseTarget[]>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for concurrency limit.
Consider adding validation to ensure the concurrency limit is a positive number to prevent unexpected behavior.
type ConcurrencyRuleOptions = {
concurrency: number;
getReleaseTargetsInConcurrencyGroup: () => Promise<schema.ReleaseTarget[]>;
};
export class ConcurrencyRule implements PreValidationRule {
public readonly name = "ConcurrencyRule";
- constructor(private readonly options: ConcurrencyRuleOptions) {}
+ constructor(private readonly options: ConcurrencyRuleOptions) {
+ if (options.concurrency <= 0) {
+ throw new Error("Concurrency limit must be a positive number");
+ }
+ }Also applies to: 44-44
🤖 Prompt for AI Agents
In packages/rule-engine/src/rules/concurrency-rule.ts at lines 7 to 10 and line
44, add validation to ensure the concurrency property in ConcurrencyRuleOptions
is a positive number. Implement a check that throws an error or handles invalid
values if concurrency is zero or negative to prevent unexpected behavior during
rule execution.
There was a problem hiding this comment.
the db and zod schemas all already account for this, i'm okay with this rule being "dumb" and just ingesting the number since its just doing a < check
There was a problem hiding this comment.
@adityachoudhari26 Understood! You're absolutely right that validation at the schema boundary is sufficient, and keeping the rule focused on its core logic (the comparison check) makes sense from an architectural perspective. Thanks for the clarification on your approach to validation placement.
🐰💭
Understanding the bigger picture...
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.094Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/workers/evaluate-release-target.ts(1 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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/event-worker/src/workers/evaluate-release-target.tspackages/rule-engine/src/db/get-applicable-policies.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
111-119: LGTM: Enhanced locking for concurrency control.The addition of joins to policy-related tables in the locking query is a good approach for ensuring consistency when evaluating release targets with concurrency rules. The
FOR UPDATE NOWAITstrategy will prevent deadlocks while ensuring that all relevant data is locked during evaluation.packages/rule-engine/src/db/get-applicable-policies.ts (1)
43-43: LGTM: Concurrency data inclusion for policy evaluation.Adding the
concurrencyfield to the policy fetch query is the correct approach for supporting concurrency rules. This ensures that concurrency constraints are available when evaluating applicable policies for release targets.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsx (1)
108-113:⚠️ Potential issueAdd URL handling for concurrency rule type.
The URL generation logic doesn't handle the "concurrency" rule type, so concurrency badges will incorrectly link to deny windows.
Add a case for concurrency in the URL generation:
const url = rule === "approval-gate" ? baseUrl.approvalGates() : rule === "deployment-version-selector" ? baseUrl.versionConditions() + : rule === "concurrency" + ? baseUrl.concurrency() : baseUrl.denyWindows();
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx (1)
71-74: Consider using a different icon for better visual distinction.Both the "Concurrency" tab and "Approval Gates" tab use the same
IconCircleCheckicon, which could be confusing for users. Consider using a more semantically appropriate icon for concurrency, such asIconUsers,IconStack, orIconClockto better represent the concept of concurrent operations.- <TabsTrigger value="concurrency" className="flex items-center gap-1"> - <IconCircleCheck className="h-4 w-4" /> - Concurrency - </TabsTrigger> + <TabsTrigger value="concurrency" className="flex items-center gap-1"> + <IconUsers className="h-4 w-4" /> + Concurrency + </TabsTrigger>apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
56-56: Remove debug console.log statement.The console.log statement should be removed before production deployment.
- console.log(form.getValues());packages/db/drizzle/0109_nervous_johnny_storm.sql (1)
5-5: Consider adding a check constraint for concurrency validation.The
concurrencyfield defaults to 1, but consider adding a check constraint to ensure the value is always positive, as negative concurrency values would be invalid.- "concurrency" integer DEFAULT 1 NOT NULL, + "concurrency" integer DEFAULT 1 NOT NULL CHECK ("concurrency" > 0),apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx (1)
23-24: Ensure consistent formatting in RuleType union.The formatting is inconsistent - some entries use quoted strings while the new "concurrency" entry is unquoted. Consider maintaining consistent formatting for better readability.
| "approval-gate" - | "deployment-version-selector" - | "concurrency"; + | "deployment-version-selector" + | "concurrency";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/event-worker/src/workers/evaluate-release-target.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/EditConcurrency.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/Concurrency.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyCreationTabs.tsx(3 hunks)apps/webservice/src/app/api/v1/policies/openapi.ts(2 hunks)apps/webservice/src/app/urls.ts(1 hunks)packages/api/src/router/policy/evaluate.ts(1 hunks)packages/api/src/router/policy/router.ts(2 hunks)packages/db/drizzle/0109_nervous_johnny_storm.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/policy.ts(2 hunks)packages/db/src/schema/rules/concurrency.ts(1 hunks)packages/rule-engine/src/db/create-policy.ts(3 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/api/src/router/policy/evaluate.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx
- packages/api/src/router/policy/router.ts
- apps/webservice/src/app/urls.ts
- packages/db/src/schema/policy.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/db/drizzle/meta/_journal.json
- apps/webservice/src/app/api/v1/policies/openapi.ts
- apps/event-worker/src/workers/evaluate-release-target.ts
- packages/rule-engine/src/db/create-policy.ts
- packages/db/src/schema/rules/concurrency.ts
- packages/rule-engine/src/db/update-policy.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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)/policies/(sidebar)/_components/PolicyTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyCreationTabs.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/EditConcurrency.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/Concurrency.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/_components/PolicyTabs.tsx
🧬 Code Graph Analysis (3)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyCreationTabs.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/Concurrency.tsx (1)
Concurrency(11-45)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/EditConcurrency.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
usePolicyFormContext(36-43)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/Concurrency.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsx (1)
usePolicyContext(60-62)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsx (1)
54-55: LGTM!The concurrency rule detection follows the same pattern as other rule types and correctly checks for non-null concurrency policy.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyCreationTabs.tsx (3)
15-15: LGTM!Clean import addition for the new Concurrency component.
40-44: LGTM!The concurrency tab configuration follows the same pattern as other tabs with appropriate label and description.
65-66: LGTM!Correctly adds the concurrency case to render the Concurrency component, maintaining consistency with the existing switch statement pattern.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/Concurrency.tsx (1)
11-45: LGTM! Well-implemented form component.The component demonstrates excellent practices:
- Proper form integration using
usePolicyContextandFormField- Robust number input handling with NaN checking
- Consistent UI structure and styling
- Clean separation of concerns
The number input handling is particularly well done - converting
valueAsNumberand gracefully handling invalid input by setting tonull.apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsx (1)
24-24: LGTM! Concurrency field integration looks good.The addition of the "concurrency" field to the PolicyTab union type and the initialization to
nullin bothdefaultPolicyand formdefaultValuesfollows the established pattern and integrates well with the existing codebase.Also applies to: 49-49, 87-87
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/concurrency/EditConcurrency.tsx (1)
14-55: LGTM! Well-implemented concurrency editing component.The implementation demonstrates good practices:
- Proper use of the form context hook with error handling via the provider
- Robust number input handling that correctly converts NaN to null
- Appropriate save button state management (disabled during submission or when form is not dirty)
- Clean UI structure with semantic HTML and proper accessibility
The number parsing logic on lines 34-38 correctly handles edge cases where
valueAsNumbermight be NaN, ensuring type safety.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
26-26: LGTM! Proper handling of nested concurrency data structure.The implementation correctly extracts the concurrency value from the nested structure (
policy.concurrency?.concurrency ?? null) and integrates it into the form's default values. This approach properly handles the difference between the backend data model (nested concurrency rules) and the form's flat structure.Also applies to: 49-54
packages/db/drizzle/0109_nervous_johnny_storm.sql (2)
1-7: LGTM! Well-designed table schema with appropriate constraints.The table design effectively implements a one-to-one relationship between policies and concurrency rules with proper referential integrity. The unique constraint on
policy_idensures each policy can have at most one concurrency rule.
9-9:✅ Verification successful
Verify the foreign key constraint configuration.
The foreign key constraint uses
ON DELETE cascade ON UPDATE no action. This means when a policy is deleted, the concurrency rule will be automatically deleted (which is appropriate), but updates to the policy ID won't cascade. Ensure this aligns with your application's data management strategy.Let me verify if this constraint configuration is consistent with other policy rule tables in the schema:
🏁 Script executed:
#!/bin/bash # Description: Check foreign key constraint patterns in other policy rule tables # Expected: Consistent constraint patterns across policy rule tables # Search for similar foreign key constraints in migration files rg -A 2 -B 2 "policy.*fk.*ON DELETE.*ON UPDATE" --type sqlLength of output: 20493
Constraint configuration is consistent across policy_rule tables
All existingpolicy_rule_*tables (e.g.policy_rule_deployment_version_selector,policy_rule_user_approval,policy_rule_role_approval,policy_rule_any_approval,policy_rule_deny_window, etc.) useON DELETE cascade ON UPDATE no actionfor the"policy_id"foreign key. This new constraint in0109_nervous_johnny_storm.sqlmatches that established pattern, so no changes are required.apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx (2)
8-8: Good icon choice for concurrency concept.The
IconArrowsSpliticon effectively represents the concept of concurrent/parallel execution paths, making it semantically appropriate for the concurrency rule type.
51-56: Consistent implementation pattern for concurrency rule theming.The implementation follows the established pattern correctly, with appropriate icon, styling, and color choice. The yellow theme provides good visual distinction from existing rule types.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsx
Outdated
Show resolved
Hide resolved
| "release-dependency": "Release Dependency", | ||
| "approval-gate": "Approval Gate", | ||
| "deployment-version-selector": "Deployment Version Selector", | ||
| concurrency: "Concurrency", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify color accessibility and contrast.
The yellow color scheme looks appropriate, but ensure the text-yellow-400 provides sufficient contrast against the background for accessibility compliance, especially in different themes (light/dark mode).
Also applies to: 92-94
🌐 Web query:
What are the WCAG contrast ratio requirements for text-yellow-400 on typical dark and light backgrounds in web applications?
💡 Result:
WCAG Contrast Ratio Requirements for Text
Contrast Ratio Requirements
- Normal (body) text: Minimum contrast ratio of 4.5:1 between text and background[1][2][3][4][5][6][7][8].
- Large text (≥18pt or 14pt bold): Minimum contrast ratio of 3:1[1][2][3][4][5][6][7][8].
- AAA level: 7:1 for normal text, 4.5:1 for large text[2][8].
- UI Components: 3:1 for graphical objects and interface elements[2][4].
Contrast Performance of text-yellow-400 (TailwindCSS) on Typical Backgrounds
text-yellow-400 in TailwindCSS is #facc15, a light yellow shade.
On Light Backgrounds (e.g., #ffffff or #f8fafc)
- Contrast ratio: Very low (typically < 2:1).
- Example: #facc15 (yellow) on #ffffff (white) has a contrast ratio of approximately 1.17:1, far below both AA and AAA requirements.
- Result: Fails all WCAG contrast requirements for both normal and large text.
- Guidance: Avoid using
text-yellow-400on light backgrounds for any accessible text[5][9].
On Dark Backgrounds (e.g., #000000 or #18181b)
- Contrast ratio: Much higher (often > 10:1 depending on the darkness of the background).
- Example: #facc15 (yellow) on #000000 (black) has a contrast ratio of approximately 13.33:1.
- Result: Passes WCAG AA/AAA for both normal and large text.
- Guidance:
text-yellow-400is accessible for both normal and large text on very dark backgrounds[5][9].
Key Points and Practical Advice
- Yellow is challenging: Lighter yellows rarely meet contrast requirements on light backgrounds[9].
- Always check contrast ratios: Use a color contrast checker to verify your specific background and text combination[2][8].
- Design alternatives:
- For yellow text on light backgrounds, consider a much darker shade of yellow or use yellow as a background and place darker text on it[9].
- For yellow text, prefer dark or near-black backgrounds to ensure accessibility compliance.
Summary Table
| Foreground | Background | Contrast Ratio | Meets AA (Normal) | Meets AA (Large) |
|---|---|---|---|---|
| #facc15 (yellow) | #ffffff (white) | ~1.17:1 | No | No |
| #facc15 (yellow) | #000000 (black) | ~13.33:1 | Yes | Yes |
References
- WCAG 2.2 minimums: 4.5:1 for normal text, 3:1 for large text[1][2][3][4][5][7][8].
- Specifics on yellow failing on light backgrounds, passing on dark[9].
Conclusion:
text-yellow-400 passes WCAG contrast on dark backgrounds but fails on light backgrounds for all text sizes. Always check your text/background combination with a contrast checker for compliance.
Citations:
- 1: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum
- 2: https://webaim.org/resources/contrastchecker/
- 3: https://dequeuniversity.com/rules/axe/4.8/color-contrast
- 4: https://webaim.org/articles/contrast/
- 5: https://dequeuniversity.com/tips/color-contrast
- 6: https://kb.ndsu.edu/it/page.php?id=147732
- 7: https://www.boia.org/blog/designing-for-color-contrast-guidelines-for-accessibility
- 8: https://accessibleweb.com/color-contrast-checker/
- 9: https://stephaniewalter.design/blog/tips-create-accessible-color-palette/
- 10: https://www.makethingsaccessible.com/guides/contrast-requirements-for-wcag-2-2-level-aa/
Ensure WCAG Contrast for text-yellow-400 in Both Light and Dark Themes
Based on WCAG 2.2:
- On light backgrounds (e.g. #ffffff, #f8fafc),
#facc15yields ~1.17:1 (fails AA/AAA for normal and large text). - On dark backgrounds (e.g. #000000, #18181b),
#facc15yields ~13.33:1 (passes AA/AAA).
Action Items:
- Review usages of
text-yellow-400in
• apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx at lines 70 and 92–94. - In light mode, swap to a darker yellow (e.g.
text-yellow-600or higher) or adjust the background to meet ≥4.5:1 contrast. - In dark mode, no change is needed as
#facc15already passes.
🤖 Prompt for AI Agents
In
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx
at lines 70 and 92–94, the use of TailwindCSS class text-yellow-400 (#facc15)
does not meet WCAG contrast requirements on light backgrounds, failing
accessibility standards. To fix this, update the text color in light mode to a
darker yellow shade such as text-yellow-600 or higher to ensure a contrast ratio
of at least 4.5:1, or alternatively adjust the background color to improve
contrast. No changes are needed for dark mode where text-yellow-400 passes
contrast requirements.
There was a problem hiding this comment.
we don't care about accessibility I fear
There was a problem hiding this comment.
@adityachoudhari26 Understood! I'll keep that in mind for future reviews and won't flag accessibility contrast issues going forward.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/rule-themes.tsx:0-0
Timestamp: 2025-06-01T19:13:41.544Z
Learning: The ctrlplane project does not prioritize WCAG accessibility compliance. Avoid flagging color contrast or other accessibility issues in future reviews as this is not a concern for the project maintainers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
New Features
Enhancements
Database Changes