feat: add priorities to deployment variable values #619
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
## Walkthrough
This change introduces a `priority` field to deployment variable values, updates schemas, APIs, and database logic to support it, and refactors variable resolution to unify direct and reference values into a single array. The system now resolves variable values for resources based on priority, and test coverage for priority-based resolution is added.
## Changes
| Files / Areas | Change Summary |
|----------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
| `packages/db/drizzle/0126_wandering_sasquatch.sql`, `packages/db/src/schema/deployment-variables.ts`, `packages/db/src/utils/deployment-variables/upsert-direct-value.ts`, `packages/db/src/utils/deployment-variables/upsert-reference-value.ts` | Add `priority` column to deployment variable values, update schema, and handle priority in upsert logic |
| `openapi.v1.json`, `e2e/api/schema.ts`, `apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts` | Make `resourceSelector` optional and add `priority` property to API schema |
| `packages/db/src/utils/deployment-variables/get-deployment-variables.ts`, `apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts` | Refactor to unify direct/reference values, update API response shape, and handle priority |
| `packages/rule-engine/src/manager/variables/db-variable-providers.ts`, `packages/rule-engine/src/manager/variables/resolve-variable-value.ts`, `packages/rule-engine/src/manager/variables/index.ts`, `packages/rule-engine/package.json` | Unify value resolution logic, add value resolver module, and update exports/dependencies |
| `packages/api/src/router/deployment-variable/by-deployment-id.ts` | Refactor variable value/resource resolution to use unified logic and priority handling |
| `apps/webservice/src/app/[workspaceSlug]/.../variables/page.tsx` | Remove transformation of variable values; fetch and pass variables directly |
| `apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx` | Add `PriorityFormField` component and include it in the direct value creation form |
| `e2e/tests/api/resource-variables.spec.ts` | Add test for priority-based variable value resolution |
| `packages/db/drizzle/meta/_journal.json` | Add migration journal entry for priority column |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant API
participant DB
participant RuleEngine
Client->>API: Request deployment variables for a resource
API->>DB: Fetch deployment variable values (with priorities)
DB-->>API: Return variables with values (priority included)
API->>RuleEngine: Resolve variable value for resource (considering priority)
RuleEngine->>DB: Check resourceSelector, priority, resolve value
RuleEngine-->>API: Return resolved value
API-->>Client: Respond with resolved variable valuePossibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (1)
20-20: Consider type precision and consistency with database constraints.Since the database schema uses a non-nullable integer column with a default value, consider:
- Using
"integer"type instead of"number"for better precision- Evaluating if this should be a required field in responses, since the database always provides a value
- priority: { type: "number" }, + priority: { type: "integer" },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx(1 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts(3 hunks)e2e/api/schema.ts(1 hunks)e2e/tests/api/resource-variables.spec.ts(1 hunks)openapi.v1.json(1 hunks)packages/api/src/router/deployment-variable/by-deployment-id.ts(3 hunks)packages/db/drizzle/0126_wandering_sasquatch.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment-variables.ts(4 hunks)packages/db/src/utils/deployment-variables/get-deployment-variables.ts(2 hunks)packages/db/src/utils/deployment-variables/upsert-direct-value.ts(2 hunks)packages/db/src/utils/deployment-variables/upsert-reference-value.ts(2 hunks)packages/rule-engine/package.json(1 hunks)packages/rule-engine/src/manager/variables/db-variable-providers.ts(3 hunks)packages/rule-engine/src/manager/variables/index.ts(1 hunks)packages/rule-engine/src/manager/variables/resolve-variable-value.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
⚙️ CodeRabbit Configuration File
**/*.{js,jsx,ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (17)
📓 Common learnings
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#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/package.json (2)
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: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T21:36:42.935Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,css,md,yml,yaml} : Formatting: Prettier is used with `@ctrlplane/prettier-config`
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx (7)
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#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
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#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#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: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, params in dynamic routes must be awaited before accessing their properties in async contexts. For example, `params.deploymentId` should be accessed as `(await params).deploymentId` in async functions.
packages/db/drizzle/0126_wandering_sasquatch.sql (1)
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.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (4)
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#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#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#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.
packages/db/src/utils/deployment-variables/upsert-reference-value.ts (2)
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#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.
packages/db/src/utils/deployment-variables/upsert-direct-value.ts (4)
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#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#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#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.
packages/rule-engine/src/manager/variables/index.ts (1)
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T21:36:42.935Z
Learning: Applies to **/*.{ts,tsx} : Consistent type imports: `import type { Type } from "module"`
openapi.v1.json (2)
undefined
<retrieved_learning>
Learnt from: adityachoudhari26
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: adityachoudhari26
PR: #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.
</retrieved_learning>
e2e/tests/api/resource-variables.spec.ts (6)
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.
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#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: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, params in dynamic routes must be awaited before accessing their properties in async contexts. For example, `params.deploymentId` should be accessed as `(await params).deploymentId` in async functions.
packages/rule-engine/src/manager/variables/resolve-variable-value.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#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 (2)
undefined
<retrieved_learning>
Learnt from: adityachoudhari26
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: adityachoudhari26
PR: #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.
</retrieved_learning>
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (10)
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: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
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#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.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, params in dynamic routes must be awaited before accessing their properties in async contexts. For example, `params.deploymentId` should be accessed as `(await params).deploymentId` in async functions.
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#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#580
File: packages/db/src/utils/deployment-variables/upsert-direct-value.ts:33-49
Timestamp: 2025-06-02T17:59:53.658Z
Learning: In the ctrlplane codebase's deployment variable deduplication logic, null values are intentionally converted to the string "null" using String(null) so that all null values hash to the same value for consistent deduplication behavior across deployment variables.
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#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
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.
packages/db/src/schema/deployment-variables.ts (4)
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#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#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
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.
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.
packages/rule-engine/src/manager/variables/db-variable-providers.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#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#604
File: packages/rule-engine/src/manager/version-manager.ts:124-139
Timestamp: 2025-06-30T21:19:43.866Z
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.
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.
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#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.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
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.
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.
packages/db/src/utils/deployment-variables/get-deployment-variables.ts (7)
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: 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#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#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#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.
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.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
packages/api/src/router/deployment-variable/by-deployment-id.ts (12)
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: 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#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.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#382
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts:82-88
Timestamp: 2025-03-16T19:41:44.129Z
Learning: In Next.js 15, dynamic route parameters (params) return Promises instead of direct values in async contexts. When accessing properties like `params.deploymentId` in an async function, use `(await params).deploymentId` to avoid the "params should be awaited before using its properties" error. This applies to API routes, page components, and other async contexts.
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#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#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.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
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.
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#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#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`.
🧬 Code Graph Analysis (5)
e2e/tests/api/resource-variables.spec.ts (3)
e2e/tests/fixtures.ts (1)
test(11-26)packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/schema/release.ts (1)
releaseTarget(21-47)
packages/rule-engine/src/manager/variables/resolve-variable-value.ts (7)
packages/secrets/src/index.ts (1)
variablesAES256(50-51)packages/db/src/common.ts (1)
Tx(22-22)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)packages/db/src/selectors/index.ts (1)
selector(13-13)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(172-174)packages/rule-engine/src/manager/variables/db-variable-providers.ts (1)
isSelectingResource(75-86)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(8-29)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (2)
packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(172-174)packages/rule-engine/src/manager/variables/resolve-variable-value.ts (1)
getResolvedDirectValue(10-20)
packages/rule-engine/src/manager/variables/db-variable-providers.ts (2)
packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(172-174)packages/rule-engine/src/manager/variables/resolve-variable-value.ts (1)
resolveVariableValue(46-75)
packages/db/src/utils/deployment-variables/get-deployment-variables.ts (2)
packages/db/src/schema/deployment-variables.ts (2)
DeploymentVariableValue(172-174)DeploymentVariable(52-52)packages/db/src/common.ts (1)
Tx(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (30)
packages/rule-engine/package.json (1)
29-29: LGTM! Dependency addition aligns with new encryption functionality.The addition of
@ctrlplane/secretsdependency is justified for the new variable resolution functionality that handles encryption/decryption of sensitive values.packages/db/drizzle/meta/_journal.json (1)
887-893: LGTM! Standard migration journal entry.The migration journal entry follows the correct format and structure for tracking the priority column addition.
packages/db/drizzle/0126_wandering_sasquatch.sql (1)
1-1: LGTM! Safe migration with appropriate defaults.The migration correctly adds the priority column with a sensible default value (0) and NOT NULL constraint, ensuring existing records remain valid.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx (1)
37-37: LGTM! Simplification aligns with backend refactoring.The removal of client-side variable transformation is appropriate given the backend now handles value resolution and prioritization. This improves maintainability by centralizing the logic.
packages/db/src/utils/deployment-variables/upsert-reference-value.ts (2)
42-42: LGTM! Correct priority handling in insert operation.The priority field is properly included in the insert values, maintaining consistency with the schema changes.
68-71: LGTM! Consistent priority handling in update operation.The priority field is correctly included in the update set clause alongside the resourceSelector, ensuring both insert and update operations handle the new field consistently.
packages/rule-engine/src/manager/variables/index.ts (1)
3-3: LGTM! Clean addition following established patterns.The new export follows the existing module pattern and aligns with the broader refactoring to unify variable value resolution.
packages/db/src/utils/deployment-variables/upsert-direct-value.ts (2)
66-66: LGTM! Priority field properly added to insert operation.The addition of the priority field to the insert values is correct and maintains consistency with the schema changes.
97-100: LGTM! Priority field properly added to update operation.The update operation correctly includes both
resourceSelectorandpriorityfields, maintaining consistency between insert and update operations for direct variable values.e2e/api/schema.ts (1)
1070-1074: LGTM! Schema changes align with priority-based variable resolution feature.The addition of the optional
priorityfield and makingresourceSelectoroptional are semantically correct for the deployment variable value enhancement. These changes properly support the new priority-based resolution while maintaining backward compatibility.Since this is an auto-generated file, these changes appropriately reflect updates to the underlying OpenAPI specification.
packages/db/src/schema/deployment-variables.ts (4)
9-9: LGTM: Import addition for priority field.The
integerimport is correctly added to support the new priority column definition.
65-65: LGTM: Priority column definition is well-structured.The priority field is properly defined as a non-null integer with a sensible default of 0, ensuring backwards compatibility with existing records.
103-103: LGTM: Consistent priority validation for direct values.The priority field is correctly added to the validation schema with an optional number type and default of 0, maintaining consistency with the database schema.
149-149: LGTM: Consistent priority validation for reference values.The priority field definition matches the direct values schema, ensuring consistent validation across both value types.
e2e/tests/api/resource-variables.spec.ts (1)
580-698: LGTM: Comprehensive test for priority-based resolution.This test effectively validates the priority-based tie-breaking functionality by:
- Creating a resource with a unique identifier
- Setting up two deployment variable values with different priorities (1 and 2) that both match the resource
- Creating a deployment version and waiting for propagation
- Verifying that the higher priority value (priority 2) is resolved
The test follows the established patterns in the file and provides good coverage for the new priority feature. The use of random identifiers ensures test isolation.
packages/rule-engine/src/manager/variables/resolve-variable-value.ts (3)
10-20: LGTM: Well-structured decryption logic for sensitive values.The
getResolvedDirectValuefunction correctly handles both sensitive and non-sensitive values:
- Returns values as-is when not sensitive
- Properly converts objects to JSON strings before decryption
- Uses the established
variablesAES256()service for decryptionThe type annotation accurately reflects the expected input structure.
22-37: LGTM: Resource selector evaluation is implemented correctly.The
getIsSelectingResourcefunction properly:
- Returns
falsefor null selectors (no filtering)- Uses the established
selector()query builder pattern- Combines resource ID equality with selector conditions using
and()- Returns boolean based on whether a matching resource is found
This aligns with the existing patterns seen in
packages/rule-engine/src/manager/variables/db-variable-providers.ts.
39-75: LGTM: Main resolution function handles all scenarios correctly.The
resolveVariableValuefunction is well-designed:
- Default handling: Bypasses resource selector evaluation when
isDefault=true- Early return: Returns
nullwhen resource selector doesn't match- Type discrimination: Uses schema type guards to handle direct vs reference values
- Consistent return structure: Both paths return
{value, sensitive}objects- Reference resolution: Delegates to the existing
getReferenceVariableValuefunction- Security: Marks reference values as non-sensitive (line 73)
The JSDoc comment clearly documents the function's purpose and return behavior.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (3)
15-15: LGTM: Import moved to correct location.The
getResolvedDirectValueimport is correctly moved from@ctrlplane/dbto@ctrlplane/rule-engine, reflecting the new centralized location of the resolution logic.
27-34: LGTM: Well-structured helper for default value formatting.The
formatDefaultValuefunction correctly:
- Handles undefined values gracefully
- Returns reference values as-is (no resolution needed in this context)
- Applies
getResolvedDirectValueonly to direct values- Maintains the original object structure while adding the resolved value
This helper promotes code reuse and maintains consistency.
61-85: LGTM: Value separation and formatting logic is well-implemented.The refactored logic correctly:
- Separates values: Uses schema type guards to filter direct vs reference values
- Resolves direct values: Applies
getResolvedDirectValueto decrypt sensitive values- Adds default flags: Compares each value's ID to the default value ID
- Maintains structure: Preserves all original fields while adding resolved values and flags
- Handles both types: Applies similar processing to both direct and reference values
The destructuring of
{ values, defaultValue, ...rest }cleanly separates the new unified values array from other variable properties.packages/api/src/router/deployment-variable/by-deployment-id.ts (4)
2-2: LGTM: Added utility import for filtering.The
isPresentimport fromts-is-presentis correctly added to support filtering out null values from the resolution promises.
5-7: LGTM: Import updates reflect the new unified approach.The imports are correctly updated:
- Removed unused utilities that are no longer needed
- Added
resolveVariableValuefrom@ctrlplane/rule-enginewhich centralizes the resolution logic- Simplified the import from
@ctrlplane/dbby removing no longer needed functionsThis reflects the consolidation of variable resolution logic into the rule engine package.
22-43: LGTM: Simplified resource matching using unified resolution.The refactored
getVariableValueWithMatchedResourcesfunction is much cleaner:
- Unified approach: Uses
resolveVariableValuefor all variable types instead of separate logic- Async mapping: Efficiently processes all resources in parallel
- Proper filtering: Uses
isPresentto filter out null results from unmatched resources- Security handling: Masks sensitive values with "***"
- Default support: Accepts
isDefaultparameter to handle default value resolutionThe function maintains the same return structure while significantly simplifying the implementation.
52-76: LGTM: Clean consolidation of values handling.The updated
getVariableWithMatchedResourcesfunction effectively:
- Unified values: Uses the single
valuesarray instead of separatedirectValuesandreferenceValues- Consistent processing: Applies the same resolution logic to all values regardless of type
- Resource tracking: Properly identifies matched vs unmatched resources
- Default handling: Resolves default values for unmatched resources with the
isDefault=trueflagThis consolidation simplifies the code while maintaining full functionality and aligns with the broader refactoring to unify variable value handling.
packages/db/src/utils/deployment-variables/get-deployment-variables.ts (3)
7-15: Type definition looks good!The
VariableValueDbResulttype correctly represents the structure expected from the left join query with nullable direct and reference values.
35-62: Well-structured unified query!The function correctly:
- Uses left joins to fetch both direct and reference values in a single query
- Orders by descending priority for proper resolution order
- Filters out null results cleanly with
isPresent
74-76: Clean implementation of unified values!The refactoring to use a single
valuesarray and finding the default value by ID is well implemented.packages/rule-engine/src/manager/variables/db-variable-providers.ts (2)
105-134: Well-implemented priority-based resolution!The refactored resolution logic:
- Correctly iterates through priority-ordered values
- Consistently uses
resolveVariableValuefor both regular and default values- Properly handles the fallback case when no value resolves
53-58: defaultValue optional handling is correctThe database
defaultValueIdcolumn is nullable (e.g., in the OpenAPI route we seeif (defaultValueId == null) return undefined), and the UI schema marksdefaultValueIdas nullable as well. Mappingnull→undefinedaligns with the newdefaultValue?: schema.DeploymentVariableValuetype, so there are no cases where a missing default would break. No further changes needed.
packages/rule-engine/src/manager/variables/db-variable-providers.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx (1)
162-203: Consider improving number input validation and UX.The PriorityFormField implementation follows the established patterns well, but there are a few areas for improvement:
- Input constraints: The number input lacks min/max constraints that would align with the backend validation (0-1000 based on the AI summary).
- Placeholder/default behavior: When the input is empty, it shows no visual indication of the expected range.
- Input width: The fixed
w-16class might be too narrow for 4-digit numbers.Consider this improved implementation:
<Input type="number" + min={0} + max={1000} + placeholder="0" value={value} onChange={(e) => onChange( Number.isNaN(e.target.valueAsNumber) ? undefined : e.target.valueAsNumber, ) } - className="w-16" + className="w-20" />Also consider updating the tooltip to mention the valid range:
<TooltipContent className="text-muted-foreground"> - Higher numbers take precedence when multiple values select - the same resource. + Higher numbers take precedence when multiple values select + the same resource. Valid range: 0-1000. </TooltipContent>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx(2 hunks)packages/api/src/router/deployment-variable/by-deployment-id.ts(3 hunks)packages/db/src/schema/deployment-variables.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/schema/deployment-variables.ts
- packages/api/src/router/deployment-variable/by-deployment-id.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
⚙️ CodeRabbit Configuration File
**/*.{js,jsx,ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (2)
📓 Common learnings
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#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/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx (6)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx:46-57
Timestamp: 2024-10-30T00:03:57.878Z
Learning: Accessibility and validation feedback improvements are not desired for form fields in the `Overview` component within `apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx`.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#408
File: apps/webservice/src/components/form/job-agent/JobAgentJenkinsPipelineConfig.tsx:26-31
Timestamp: 2025-04-12T22:08:13.790Z
Learning: For Jenkins job configuration, two approaches are needed: (1) a simple URL input form for airgapped environments (current focus) and (2) a dropdown selection interface for non-airgapped environments where the Jenkins server is accessible. A component similar to DeploymentJobAgentGithubConfig.tsx is preferred.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#408
File: apps/webservice/src/components/form/job-agent/JobAgentJenkinsPipelineConfig.tsx:26-31
Timestamp: 2025-04-12T22:08:13.790Z
Learning: For Jenkins job configuration, two approaches are needed: (1) a simple URL input form for airgapped environments (current focus) and (2) a dropdown selection interface for non-airgapped environments where the Jenkins server is accessible. A component similar to DeploymentJobAgentGithubConfig.tsx is preferred.
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#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#578
File: apps/webservice/src/app/[workspaceSlug]/(app)/resources/(sidebar)/providers/integrations/github/GithubDialog.tsx:58-58
Timestamp: 2025-05-30T21:48:48.868Z
Learning: In the ctrlplane codebase, the shadcn form UI version allows initializing forms with `useForm({ schema: formSchema })` directly, without needing to import and use `zodResolver` from '@hookform/resolvers/zod'. This is different from standard react-hook-form usage.
🧬 Code Graph Analysis (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx (3)
packages/ui/src/form.tsx (4)
FormField(220-220)FormItem(215-215)FormControl(217-217)FormLabel(216-216)packages/ui/src/tooltip.tsx (4)
TooltipProvider(30-30)Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)packages/ui/src/input.tsx (1)
Input(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/_components/direct-value/CreateDirectValueDialog.tsx (1)
275-275: Well-positioned form field integration.The PriorityFormField is appropriately positioned in the form flow - after the value configuration fields (Default, Sensitive) and before the resource selector, which makes logical sense from a UX perspective.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores