feat: version pinning #604
Conversation
WalkthroughThis change introduces version pinning and unpinning functionality for release targets, including database schema migration, API endpoints with OpenAPI specifications, and end-to-end tests. It adds a nullable Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant RuleEngine
Client->>API: POST /v1/release-targets/{releaseTargetId}/pin {versionId|versionTag}
API->>DB: Authenticate & Authorize user
API->>DB: Fetch ReleaseTarget by ID
alt Pinning a Version
API->>DB: Fetch DeploymentVersion by ID or Tag
API->>DB: Update ReleaseTarget.desiredVersionId
else Unpinning
Client->>API: POST /v1/release-targets/{releaseTargetId}/unpin
API->>DB: Fetch ReleaseTarget by ID
API->>DB: Set ReleaseTarget.desiredVersionId to NULL
end
API->>RuleEngine: Dispatch evaluation job for ReleaseTarget
API-->>Client: 200 { success: true }
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 4
🧹 Nitpick comments (4)
openapi.v1.json (2)
3215-3224: Add adescriptionfor thereleaseTargetIdpath parameter for parity with neighbouring endpointsMost other paths (e.g.
/v1/release-targets/{releaseTargetId}/lock) include a human-readable description for the UUID parameter. Omitting it here is a small but visible inconsistency in the generated docs."schema": { "type": "string", "format": "uuid" - } + }, + "description": "The release target ID"
3278-3322: Align error response schemas with the rest of the specMost existing endpoints declare
"required": ["error"]for 400/404/500 bodies.
The new/pinresponses omit that, which causes lint / code-gen tools to treat the
shape differently.- "properties": { - "error": { - "type": "string" - } - } + "properties": { + "error": { + "type": "string" + } + }, + "required": ["error"]Doing this for each of 400, 404 and 500 keeps generated client types uniform.
e2e/tests/api/version-pinning.spec.ts (1)
84-106: Consider reducing hard-coded timeout values.The 8-second timeouts seem quite long for e2e tests. Consider if these can be reduced or made configurable to improve test execution time.
- await page.waitForTimeout(8_000); + await page.waitForTimeout(5_000); // Reduced from 8s to 5sAlternatively, consider implementing a polling mechanism that waits for the expected state rather than using fixed timeouts.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (1)
69-84: Consider making unpinning idempotentWhen no version is pinned, returning a BAD_REQUEST error might confuse API consumers. Unpinning an already unpinned target should be a successful no-op for better idempotency.
Consider this alternative implementation:
const unpinVersion = async (db: Tx, releaseTarget: schema.ReleaseTarget) => { if (releaseTarget.desiredVersionId == null) - return NextResponse.json( - { error: "No version pinned" }, - { status: BAD_REQUEST }, - ); + return NextResponse.json({ success: true }); await db .update(schema.releaseTarget) .set({ desiredVersionId: null }) .where(eq(schema.releaseTarget.id, releaseTarget.id)); await dispatchQueueJob().toEvaluate().releaseTargets([releaseTarget]); return NextResponse.json({ success: true }); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts(1 hunks)e2e/api/schema.ts(3 hunks)e2e/tests/api/version-pinning.spec.ts(1 hunks)e2e/tests/api/version-pinning.spec.yaml(1 hunks)openapi.v1.json(1 hunks)packages/db/drizzle/0119_majestic_devos.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(1 hunks)packages/rule-engine/src/manager/types.ts(1 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/cat...
**/*.{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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/rule-engine/src/manager/types.tspackages/db/src/schema/release.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.tse2e/tests/api/version-pinning.spec.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.tspackages/rule-engine/src/manager/version-manager.tse2e/api/schema.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
packages/rule-engine/src/manager/types.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.185Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
packages/db/src/schema/release.ts (8)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.185Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50
Timestamp: 2024-11-27T23:16:35.580Z
Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#561
File: packages/db/drizzle/0100_robust_eternals.sql:2-2
Timestamp: 2025-05-11T17:51:40.365Z
Learning: The deployment_variable_value table needs to support multiple records with null values for the same variable_id, which is why a unique index with COALESCE would be problematic as it would treat all nulls as equal.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/event-worker/src/workers/compute-systems-release-targets.ts:86-110
Timestamp: 2025-04-28T18:38:21.163Z
Learning: In SQL queries that use `inArray()` with arrays like `deploymentIds` or `environmentIds`, if these arrays are empty, it will generate an invalid `IN ()` clause that PostgreSQL rejects. Adding condition checks (e.g., `if (array.length > 0)`) before executing such queries prevents SQL syntax errors.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/jobs/src/ephemeral-env-checker/index.ts:57-0
Timestamp: 2024-10-30T23:10:58.869Z
Learning: In the codebase, deployments are decoupled from environments. When deleting environments (e.g., in `apps/jobs/src/ephemeral-env-checker/index.ts`), associated deployments should not be deleted.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: packages/api/src/router/deployment.ts:144-161
Timestamp: 2024-11-01T02:37:25.510Z
Learning: In `packages/api/src/router/deployment.ts`, when using Drizzle ORM, there is a limitation when referencing the same table twice in a relational builder query (rbq), requiring separate queries to avoid issues.
packages/db/drizzle/0119_majestic_devos.sql (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#561
File: packages/db/drizzle/0100_robust_eternals.sql:2-2
Timestamp: 2025-05-11T17:51:40.365Z
Learning: The deployment_variable_value table needs to support multiple records with null values for the same variable_id, which is why a unique index with COALESCE would be problematic as it would treat all nulls as equal.
e2e/tests/api/version-pinning.spec.yaml (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#517
File: e2e/tests/api/deployment-variable.spec.ts:70-76
Timestamp: 2025-04-28T21:59:04.723Z
Learning: In the ctrlplane e2e tests, tests can be isolated between runs using prefixed systems with random IDs in YAML templates, but tests within the same file may still need to handle isolation between test cases if they operate on the same resources.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#590
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts:3-74
Timestamp: 2025-06-14T06:43:08.415Z
Learning: In the ctrlplane project, OpenAPI specifications are organized as separate files for each endpoint and then merged into a single OpenAPI document. Individual endpoint OpenAPI files can reference schemas using $ref even if the schema is not defined locally in that file, as long as the schema exists in another OpenAPI file that will be part of the merge process.
e2e/tests/api/version-pinning.spec.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:96-99
Timestamp: 2025-06-24T23:53:36.327Z
Learning: In e2e test files, prefer explicit null safety checks and validation over non-null assertions (!). When validating API responses in tests, use type guards and throw descriptive errors rather than assuming values are always present.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (3)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.
packages/rule-engine/src/manager/version-manager.ts (3)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
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.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
🧬 Code Graph Analysis (2)
packages/rule-engine/src/manager/version-manager.ts (2)
packages/db/src/selectors/index.ts (1)
selector(13-13)packages/db/src/common.ts (1)
takeFirst(9-13)
e2e/api/schema.ts (1)
packages/node-sdk/src/schema.ts (1)
operations(1422-4459)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (13)
packages/db/drizzle/meta/_journal.json (1)
838-844: LGTM!The migration journal entry follows the established pattern and properly tracks the new migration for version pinning functionality.
packages/rule-engine/src/manager/types.ts (1)
7-7: LGTM!The addition of
desiredVersionIdproperty correctly aligns with the database schema and maintains consistent typing with nullable UUID fields.packages/db/src/schema/release.ts (1)
40-42: LGTM!The schema addition is well-designed with proper foreign key constraints and appropriate
onDelete: "set null"behavior for the optional version pinning functionality.packages/db/drizzle/0119_majestic_devos.sql (1)
1-2: LGTM!The migration is backwards-compatible and correctly implements the database schema changes for version pinning. The foreign key constraint with
ON DELETE SET NULLproperly handles cleanup when referenced versions are deleted.apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.ts (2)
1-100: LGTM! Well-structured OpenAPI specification.The overall structure follows OpenAPI v3 standards correctly with proper endpoint definition, parameter validation, and comprehensive response schemas. The path parameter validation and response structure are well-defined.
26-46: Review the nullable + required schema combination.The
oneOfschema defines bothversionIdandversionTagas nullable and required, which creates a logical inconsistency. A field cannot be both required and nullable in a meaningful way.For unpinning operations, you likely want to allow
nullvalues, but therequiredconstraint would prevent this. Consider one of these approaches:
- Remove
nullable: trueand use separate schemas for pin vs unpin operations- Make the fields optional instead of required when they can be null
- Use a different schema structure that better represents the pin/unpin intent
schema: { oneOf: [ { type: "object", properties: { versionId: { type: "string", format: "uuid", - nullable: true, }, }, - required: ["versionId"], + additionalProperties: false, }, { type: "object", properties: { - versionTag: { type: "string", nullable: true }, + versionTag: { type: "string" }, }, - required: ["versionTag"], + additionalProperties: false, + }, + { + type: "object", + properties: { + versionId: { type: "null" }, + }, + additionalProperties: false, }, ], },⛔ Skipped due to learnings
Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#580 File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts:21-21 Timestamp: 2025-06-02T17:55:33.889Z Learning: In OpenAPI schemas, a field marked as both "required" and "nullable" means the field must be present in the request/response object, but its value can be explicitly set to null. This is different from optional fields where the field can be omitted entirely. For example, `resourceSelector: null` is valid when resourceSelector is required+nullable, but would be invalid if resourceSelector was required+non-nullable.e2e/tests/api/version-pinning.spec.yaml (1)
1-32: LGTM! Well-structured test configuration.The YAML configuration properly sets up the test scenario with:
- Appropriate use of
{{ prefix }}templating for test isolation- Proper entity relationships (system → environment → resources)
- Good version selection (1.0.0, 2.0.0, 3.0.0) that supports the test scenarios
- Correct resource selector configuration
This follows the established patterns for e2e test configurations in the project.
e2e/tests/api/version-pinning.spec.ts (2)
62-107: LGTM! Comprehensive test coverage for pin/unpin functionality.The test flow effectively validates:
- Pinning to version 2.0.0
- Re-pinning to version 1.0.0
- Unpinning (reverting to default 3.0.0)
The test properly exercises both
versionTagandversionIdparameters as defined in the OpenAPI spec, and validates the expected behavior after each operation.
22-24: Replace non-null assertion with explicit null check.Based on your coding preferences, use explicit null safety checks instead of non-null assertions in e2e tests.
- expect(resourceResponse.data?.id).toBeDefined(); - const resourceId = resourceResponse.data!.id; + if (!resourceResponse.data?.id) { + throw new Error("Resource ID not found in response"); + } + const resourceId = resourceResponse.data.id;⛔ Skipped due to learnings
Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#601 File: e2e/tests/api/policies/retry-policy.spec.ts:96-99 Timestamp: 2025-06-24T23:53:36.327Z Learning: In e2e test files, prefer explicit null safety checks and validation over non-null assertions (!). When validating API responses in tests, use type guards and throw descriptive errors rather than assuming values are always present.Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#601 File: e2e/tests/api/policies/retry-policy.spec.ts:117-130 Timestamp: 2025-06-24T23:53:25.398Z Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#601 File: e2e/tests/api/policies/retry-policy.spec.ts:23-24 Timestamp: 2025-06-24T23:52:50.732Z Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#517 File: e2e/tests/api/deployment-variable.spec.ts:70-76 Timestamp: 2025-04-28T21:59:04.723Z Learning: In the ctrlplane e2e tests, tests can be isolated between runs using prefixed systems with random IDs in YAML templates, but tests within the same file may still need to handle isolation between test cases if they operate on the same resources.Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#237 File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50 Timestamp: 2024-11-27T23:16:35.580Z Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.e2e/api/schema.ts (3)
547-563: New API endpoint structure looks correct.The new
/v1/release-targets/{releaseTargetId}/pinendpoint follows the established pattern and correctly maps to thepinReleaseTargetoperation. The path structure and HTTP method (POST) are appropriate for a pinning action.
3048-3049: Response schema change improves API consistency.The change from returning
{ id: string }to{ success: boolean; message: string }provides more meaningful feedback to clients about the operation result.
3613-3680: Pin release target operation is well-defined.The operation definition is comprehensive with:
- Proper path parameter validation (
releaseTargetId: string)- Flexible request body allowing either version ID or version tag
- Complete error response coverage (400, 404, 500)
- Appropriate success response structure
The union type in the request body correctly models the either/or relationship between
versionIdandversionTag.packages/rule-engine/src/manager/version-manager.ts (1)
186-198: Well-structured version pinning implementationThe refactored method correctly prioritizes the pinned version when present, falling back to the standard evaluation logic otherwise. The extraction of helper methods improves code readability and maintainability.
| expect(releaseTargetResponse.data?.length).toBe(1); | ||
| return releaseTargetResponse.data![0].id; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertion with explicit validation.
Use explicit validation instead of non-null assertion for better error reporting.
- expect(releaseTargetResponse.data?.length).toBe(1);
- return releaseTargetResponse.data![0].id;
+ if (!releaseTargetResponse.data || releaseTargetResponse.data.length !== 1) {
+ throw new Error(`Expected exactly 1 release target, got ${releaseTargetResponse.data?.length ?? 0}`);
+ }
+ return releaseTargetResponse.data[0].id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(releaseTargetResponse.data?.length).toBe(1); | |
| return releaseTargetResponse.data![0].id; | |
| // previous lines remain unchanged | |
| - expect(releaseTargetResponse.data?.length).toBe(1); | |
| - return releaseTargetResponse.data![0].id; | |
| + if (!releaseTargetResponse.data || releaseTargetResponse.data.length !== 1) { | |
| + throw new Error(`Expected exactly 1 release target, got ${releaseTargetResponse.data?.length ?? 0}`); | |
| + } | |
| + return releaseTargetResponse.data[0].id; | |
| // following lines remain unchanged |
🤖 Prompt for AI Agents
In e2e/tests/api/version-pinning.spec.ts around lines 29 to 30, replace the
non-null assertion operator (!) used on releaseTargetResponse.data with an
explicit check to validate that data is not null or undefined before accessing
its elements. Add a conditional or assertion to handle the case when data is
missing, providing a clear error message to improve error reporting and avoid
runtime exceptions.
| const latestRelease = releaseResponse.data![0]; | ||
| expect(latestRelease.version.tag).toBe(expectedTag); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit null safety check for latest release.
Replace non-null assertion with explicit validation.
- const latestRelease = releaseResponse.data![0];
- expect(latestRelease.version.tag).toBe(expectedTag);
+ if (!releaseResponse.data || releaseResponse.data.length === 0) {
+ throw new Error("No releases found for release target");
+ }
+ const latestRelease = releaseResponse.data[0];
+ if (!latestRelease.version?.tag) {
+ throw new Error("Latest release missing version tag");
+ }
+ expect(latestRelease.version.tag).toBe(expectedTag);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const latestRelease = releaseResponse.data![0]; | |
| expect(latestRelease.version.tag).toBe(expectedTag); | |
| if (!releaseResponse.data || releaseResponse.data.length === 0) { | |
| throw new Error("No releases found for release target"); | |
| } | |
| const latestRelease = releaseResponse.data[0]; | |
| if (!latestRelease.version?.tag) { | |
| throw new Error("Latest release missing version tag"); | |
| } | |
| expect(latestRelease.version.tag).toBe(expectedTag); |
🤖 Prompt for AI Agents
In e2e/tests/api/version-pinning.spec.ts around lines 42 to 43, replace the
non-null assertion on releaseResponse.data with an explicit null or undefined
check before accessing the first element. Add a conditional statement to verify
that releaseResponse.data exists and has at least one element, and handle the
case where it does not to avoid runtime errors.
| async findDesiredVersion() { | ||
| const isMatchingPolicySelector = await this.getPolicySelectorSql(); | ||
|
|
||
| const { desiredVersionId } = this.releaseTarget; | ||
| if (desiredVersionId == null) return null; | ||
| return this.db | ||
| .select() | ||
| .from(schema.deploymentVersion) | ||
| .where( | ||
| and( | ||
| isReleaseTargetInDeployment, | ||
| isAfterLatestDeployedVersion, | ||
| isBeforeLatestVersionMatchingPolicy, | ||
| eq(schema.deploymentVersion.id, desiredVersionId), | ||
| isMatchingPolicySelector, | ||
| ), | ||
| ) | ||
| .then(takeFirst); | ||
| } |
There was a problem hiding this comment.
Use takeFirstOrNull to handle missing desired versions gracefully
The takeFirst utility throws an error if the result set doesn't contain exactly one row. This could happen if the desired version was deleted or no longer matches the policy selector, causing the entire evaluation to fail.
Replace takeFirst with takeFirstOrNull:
async findDesiredVersion() {
const isMatchingPolicySelector = await this.getPolicySelectorSql();
const { desiredVersionId } = this.releaseTarget;
if (desiredVersionId == null) return null;
return this.db
.select()
.from(schema.deploymentVersion)
.where(
and(
eq(schema.deploymentVersion.id, desiredVersionId),
isMatchingPolicySelector,
),
)
- .then(takeFirst);
+ .then(takeFirstOrNull);
}📝 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.
| async findDesiredVersion() { | |
| const isMatchingPolicySelector = await this.getPolicySelectorSql(); | |
| const { desiredVersionId } = this.releaseTarget; | |
| if (desiredVersionId == null) return null; | |
| return this.db | |
| .select() | |
| .from(schema.deploymentVersion) | |
| .where( | |
| and( | |
| isReleaseTargetInDeployment, | |
| isAfterLatestDeployedVersion, | |
| isBeforeLatestVersionMatchingPolicy, | |
| eq(schema.deploymentVersion.id, desiredVersionId), | |
| isMatchingPolicySelector, | |
| ), | |
| ) | |
| .then(takeFirst); | |
| } | |
| async findDesiredVersion() { | |
| const isMatchingPolicySelector = await this.getPolicySelectorSql(); | |
| const { desiredVersionId } = this.releaseTarget; | |
| if (desiredVersionId == null) return null; | |
| return this.db | |
| .select() | |
| .from(schema.deploymentVersion) | |
| .where( | |
| and( | |
| eq(schema.deploymentVersion.id, desiredVersionId), | |
| isMatchingPolicySelector, | |
| ), | |
| ) | |
| .then(takeFirstOrNull); | |
| } |
🤖 Prompt for AI Agents
In packages/rule-engine/src/manager/version-manager.ts between lines 124 and
139, replace the use of takeFirst with takeFirstOrNull in the findDesiredVersion
method to avoid throwing an error when no matching deployment version is found.
This change will allow the method to return null gracefully if the desired
version is missing or does not match the policy selector, preventing evaluation
failures.
There was a problem hiding this comment.
if we get to this point and takefirst breaks, we do want the entire thing to fail because something is wrong
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right! In this specific context, if takeFirst fails it means we have a desiredVersionId that either:
- No longer exists in the database (data integrity issue)
- No longer matches the policy selector (configuration problem)
Both scenarios represent error conditions that should fail loudly rather than be silently handled. Since we already handle the null case explicitly with the early return, using takeFirst here ensures we catch these problematic states. Thanks for the clarification!
🐰✨
/| /|
( :v: )
|(_)|
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#604
File: packages/rule-engine/src/manager/version-manager.ts:124-139
Timestamp: 2025-06-30T21:19:43.835Z
Learning: In packages/rule-engine/src/manager/version-manager.ts, the findDesiredVersion method should use takeFirst (not takeFirstOrNull) because if a desiredVersionId is set but the query fails to find exactly one matching version, it indicates a data integrity or configuration issue that should fail loudly rather than be handled silently.
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.
🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
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.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.894Z
Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#593
File: packages/rule-engine/src/manager/version-manager-rules/version-approval.ts:55-70
Timestamp: 2025-06-18T21:46:51.459Z
Learning: The `takeFirst` utility function from the database layer throws an error if no record is found, rather than returning `undefined`. This means functions using `takeFirst` already have error handling built-in and don't require additional null checks.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/event-worker/src/workers/compute-systems-release-targets.ts:86-110
Timestamp: 2025-04-28T18:38:21.163Z
Learning: In SQL queries that use `inArray()` with arrays like `deploymentIds` or `environmentIds`, if these arrays are empty, it will generate an invalid `IN ()` clause that PostgreSQL rejects. Adding condition checks (e.g., `if (array.length > 0)`) before executing such queries prevents SQL syntax errors.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#501
File: apps/webservice/src/app/api/v1/resources/route.ts:69-72
Timestamp: 2025-04-22T04:35:36.016Z
Learning: In Node.js applications, non-awaited Promises that reject after a function returns won't be caught by the outer try-catch block, potentially causing unhandled rejections. Either explicitly mark them with `void` and add `.catch()` handlers, or use `await` to ensure proper error handling.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:96-99
Timestamp: 2025-06-24T23:53:36.327Z
Learning: In e2e test files, prefer explicit null safety checks and validation over non-null assertions (!). When validating API responses in tests, use type guards and throw descriptive errors rather than assuming values are always present.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (2)
25-47: Consider adding cross-validation for versionId.The
getVersionfunction handles both cases correctly, but when usingversionId, it doesn't validate that the version belongs to the same deployment as the release target, which could lead to invalid pinning.Consider adding validation to ensure the version belongs to the correct deployment:
if ("versionId" in body) return db .select() .from(schema.deploymentVersion) - .where(eq(schema.deploymentVersion.id, body.versionId)) + .where( + and( + eq(schema.deploymentVersion.id, body.versionId), + eq(schema.deploymentVersion.deploymentId, releaseTarget.deploymentId), + ), + ) .then(takeFirstOrNull);
62-77: Remove commented-out code.This commented code appears to be leftover from development since there's now a dedicated unpin endpoint. Keeping dead code can cause confusion.
-// const unpinVersion = async (db: Tx, releaseTarget: schema.ReleaseTarget) => { -// if (releaseTarget.desiredVersionId == null) -// return NextResponse.json( -// { error: "No version pinned" }, -// { status: BAD_REQUEST }, -// ); - -// await db -// .update(schema.releaseTarget) -// .set({ desiredVersionId: null }) -// .where(eq(schema.releaseTarget.id, releaseTarget.id)); - -// await dispatchQueueJob().toEvaluate().releaseTargets([releaseTarget]); - -// return NextResponse.json({ success: true }); -// };e2e/api/schema.ts (2)
3630-3705: Excellent use of discriminated union for flexible version specificationThe
pinReleaseTargetoperation uses a well-designed discriminated union that allows specifying a version either by ID or tag. The response codes and error handling are comprehensive and appropriate.However, consider making the
successfield required instead of optional for consistency.content: { "application/json": { - success?: boolean; + success: boolean; }; };
3812-3868: Consistent design with pin endpointThe
unpinReleaseTargetoperation maintains excellent consistency with the pin endpoint in terms of response structure and error handling. The lack of request body is appropriate for an unpin operation.Same suggestion as pin endpoint: consider making
successfield required.content: { "application/json": { - success?: boolean; + success: boolean; }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/route.ts(1 hunks)e2e/api/schema.ts(5 hunks)e2e/tests/api/version-pinning.spec.ts(1 hunks)openapi.v1.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/openapi.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/openapi.ts
- e2e/tests/api/version-pinning.spec.ts
- openapi.v1.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/cat...
**/*.{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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/route.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.tse2e/api/schema.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/route.ts (3)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.
🧬 Code Graph Analysis (2)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/route.ts (6)
packages/db/src/common.ts (2)
Tx(22-22)takeFirstOrNull(15-20)packages/db/src/schema/release.ts (1)
releaseTarget(21-47)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (1)
POST(79-122)apps/webservice/src/app/api/v1/middleware.ts (1)
request(24-53)apps/webservice/src/app/api/v1/auth.ts (2)
authn(16-22)authz(26-62)packages/events/src/dispatch-jobs.ts (1)
dispatchQueueJob(161-166)
e2e/api/schema.ts (1)
packages/node-sdk/src/schema.ts (1)
operations(1422-4459)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unpin/route.ts (5)
1-16: Imports and logger setup look good.The imports are appropriate for the functionality and the logger setup follows good practices by creating a child logger with path context.
18-23: LGTM: Clean database query implementation.The
getReleaseTargetfunction properly uses thetakeFirstOrNullutility to handle single record queries, which is consistent with the codebase patterns.
25-37: Good defensive programming with null checks.The
getPinnedVersionfunction properly handles the case where no version is pinned by checking for nulldesiredVersionIdbefore querying the database.
39-43: LGTM: Straightforward unpin implementation.The
unpinVersionfunction correctly setsdesiredVersionIdto null to unpin the version.
45-86: Well-structured route handler with proper error handling.The POST handler follows the established middleware pattern with authentication, authorization, and comprehensive error handling. The logic flow is correct: validate release target exists, check if version is pinned, unpin if needed, and dispatch evaluation job.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/pin/route.ts (4)
1-18: Imports look appropriate for the functionality.All necessary imports are included for schema validation, database operations, authentication, and middleware.
20-23: Good validation schema design.The union schema allowing either
versionIdorversionTagprovides flexibility for different use cases while maintaining type safety.
49-60: Helper functions are well-implemented.Both
getReleaseTargetandpinVersionfunctions are correctly implemented and follow consistent patterns with the unpin route.
79-122: Excellent route handler implementation.The POST handler follows the established patterns with proper middleware chaining, authentication, authorization, body parsing, and error handling. The logic flow is correct and consistent with the unpin route.
e2e/api/schema.ts (3)
547-563: LGTM: Well-designed pin endpoint schemaThe pin endpoint follows RESTful conventions and provides a clear API for pinning versions to release targets. The endpoint structure is consistent with other release target operations.
598-614: LGTM: Consistent unpin endpoint designThe unpin endpoint appropriately requires no request body and follows the same pattern as the pin endpoint, maintaining consistency in the API design.
3065-3066: Confirmed: no internal usages of the oldidfield inupdateJobresponse
- Within the repository, no client code or tests reference
response.idfor thePATCH /v1/jobs/{jobId}endpoint.- The only implementation of
updateJobin
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
now returnsNextResponse.json({ success: true, message: "Job update queued" });- The e2e schema in
e2e/api/schema.tscorrectly reflects the new{ success: boolean; message: string }response.No changes to application logic are required. External API consumers should be notified of this intentional breaking change and update their integrations (or bump the API version/documentation accordingly).
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Refactor
Chores