perf: reduce deployment version selector sql call count#614
perf: reduce deployment version selector sql call count#614adityachoudhari26 merged 4 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as E2E Test Suite
participant API as API Server
participant VersionMgr as VersionReleaseManager
participant DB as Database
TestSuite->>API: Create deployment & deployment version
TestSuite->>API: Request release target for resource
API->>VersionMgr: Evaluate version selector policy
VersionMgr->>DB: Fetch deployment versions (batch)
VersionMgr->>VersionMgr: Cache versions
VersionMgr->>API: Return versions matching selector
API->>TestSuite: Return release target/release info
TestSuite->>API: Query releases for target
API->>TestSuite: Return releases (if any)
Possibly related PRs
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
e2e/tests/api/policies/version-selector-policy.spec.yaml (1)
36-36: Add newline at end of file.YAML files should end with a newline character for proper formatting.
- value: "{{ prefix }}" + value: "{{ prefix }}" +e2e/tests/api/policies/version-selector-policy.spec.ts (1)
131-131: Consider replacing hardcoded waits with condition-based polling.The 10-second waits can make tests slow and potentially flaky. Consider polling for the expected condition instead.
Replace the hardcoded wait with a polling mechanism:
// Helper function to poll for releases async function waitForRelease( api: Client<paths, `${string}/${string}`>, releaseTargetId: string, predicate: (releases: any[]) => boolean, timeout = 15000, ) { const startTime = Date.now(); while (Date.now() - startTime < timeout) { const releases = await getReleasesForTarget(api, releaseTargetId); if (predicate(releases)) { return releases; } await page.waitForTimeout(500); // Short poll interval } throw new Error("Timeout waiting for release condition"); } // Usage: const releases = await waitForRelease( api, releaseTarget.id, (releases) => releases.some(r => r.version.tag === versionTag), );Also applies to: 152-152
packages/rule-engine/src/manager/version-manager.ts (1)
143-147: Optimize Promise.all usage with synchronous function.Since
getMaxVersionCreatedAtSql()is now synchronous, wrapping it inPromise.alladds unnecessary overhead.- const [isAfterLatestDeployedVersion, isBeforeLatestVersionMatchingPolicy] = - await Promise.all([ - this.getMinVersionCreatedAtSql(), - this.getMaxVersionCreatedAtSql(), - ]); + const isAfterLatestDeployedVersion = await this.getMinVersionCreatedAtSql(); + const isBeforeLatestVersionMatchingPolicy = this.getMaxVersionCreatedAtSql();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/api/entity-fixtures.ts(1 hunks)e2e/tests/api/policies/version-selector-policy.spec.ts(1 hunks)e2e/tests/api/policies/version-selector-policy.spec.yaml(1 hunks)packages/rule-engine/src/manager/version-manager.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Use TypeScript with explicit types (prefer interfaces for public APIs) Consistent type imports: `import type { Type } from "module"`
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Consistent type imports:import type { Type } from "module"
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
e2e/api/entity-fixtures.tspackages/rule-engine/src/manager/version-manager.tse2e/tests/api/policies/version-selector-policy.spec.ts
`**/*.{js,jsx,ts,tsx}`: Import styles: Use named imports, group imports by sourc...
**/*.{js,jsx,ts,tsx}: Import styles: Use named imports, group imports by source (std lib > external > internal)
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
e2e/api/entity-fixtures.tspackages/rule-engine/src/manager/version-manager.tse2e/tests/api/policies/version-selector-policy.spec.ts
`**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}`: Formatting: Prettier is used with `@ctrlplane/prettier-config`
**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}: Formatting: Prettier is used with@ctrlplane/prettier-config
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
e2e/api/entity-fixtures.tse2e/tests/api/policies/version-selector-policy.spec.yamlpackages/rule-engine/src/manager/version-manager.tse2e/tests/api/policies/version-selector-policy.spec.ts
`**/*.{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:
e2e/api/entity-fixtures.tspackages/rule-engine/src/manager/version-manager.tse2e/tests/api/policies/version-selector-policy.spec.ts
🧠 Learnings (5)
📓 Common learnings
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: 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.
e2e/api/entity-fixtures.ts (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#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.
e2e/tests/api/policies/version-selector-policy.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.
packages/rule-engine/src/manager/version-manager.ts (10)
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#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.
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: packages/api/src/router/job.ts:382-390
Timestamp: 2024-11-27T23:18:59.456Z
Learning: In the codebase, releases are not scoped to a specific resource.
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#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#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: 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.
e2e/tests/api/policies/version-selector-policy.spec.ts (5)
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#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: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T21:36:42.935Z
Learning: Applies to **/*.test.{ts,tsx} : For tests, use vitest with mocks and typed test fixtures
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.
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/version-manager.ts (3)
packages/db/src/schema/deployment-version.ts (1)
DeploymentVersion(83-83)packages/db/src/schema/policy.ts (1)
policy(43-63)packages/db/src/selectors/index.ts (1)
selector(13-13)
🪛 YAMLlint (1.37.1)
e2e/tests/api/policies/version-selector-policy.spec.yaml
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
e2e/api/entity-fixtures.ts (1)
136-142: LGTM! Schema addition supports the new version selector policy feature.The optional
deploymentVersionSelectorproperty is well-structured with appropriate types. Using.passthrough()for the nested selector object provides good flexibility for different selector configurations.packages/rule-engine/src/manager/version-manager.ts (1)
38-38: Excellent performance optimization with version caching!The caching of deployment versions (up to 500) effectively reduces SQL query count by reusing fetched data. This directly addresses the PR's performance objectives.
Also applies to: 58-84
| expect(deploymentResponse.data?.id).toBeDefined(); | ||
| return deploymentResponse.data!.id; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertions with explicit null checks.
Based on project preferences, use explicit null safety checks instead of non-null assertions in e2e tests.
For example, replace:
- expect(deploymentResponse.data?.id).toBeDefined();
- return deploymentResponse.data!.id;
+ if (!deploymentResponse.data?.id) {
+ throw new Error("Failed to create deployment: missing id in response");
+ }
+ return deploymentResponse.data.id;Apply similar changes to lines 40, 48, 63, and 87.
Also applies to: 40-40, 48-48, 63-63, 87-87
🤖 Prompt for AI Agents
In e2e/tests/api/policies/version-selector-policy.spec.ts at lines 25-26 and
also at lines 40, 48, 63, and 87, replace the non-null assertion operator (!)
with explicit null or undefined checks before accessing properties. Instead of
using deploymentResponse.data!.id, add a conditional check to ensure
deploymentResponse.data is defined, and handle the case when it is not to avoid
runtime errors and improve code safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rule-engine/src/manager/version-manager.ts (1)
57-83: Consider cache invalidation strategy for production use.The caching mechanism doesn't include invalidation logic. In production environments, if deployment versions are added or modified during the lifetime of this manager instance, the cache could become stale.
Consider implementing cache invalidation or adding a TTL mechanism to ensure data freshness, especially for long-running processes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rule-engine/src/manager/version-manager.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Use TypeScript with explicit types (prefer interfaces for public APIs) Consistent type imports: `import type { Type } from "module"`
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Consistent type imports:import type { Type } from "module"
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/rule-engine/src/manager/version-manager.ts
`**/*.{js,jsx,ts,tsx}`: Import styles: Use named imports, group imports by sourc...
**/*.{js,jsx,ts,tsx}: Import styles: Use named imports, group imports by source (std lib > external > internal)
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/rule-engine/src/manager/version-manager.ts
`**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}`: Formatting: Prettier is used with `@ctrlplane/prettier-config`
**/*.{js,jsx,ts,tsx,json,css,md,yml,yaml}: Formatting: Prettier is used with@ctrlplane/prettier-config
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/rule-engine/src/manager/version-manager.ts
`**/*.{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/version-manager.ts
🧠 Learnings (2)
📓 Common learnings
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: 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/version-manager.ts (7)
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#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.
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#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: packages/api/src/router/job.ts:382-390
Timestamp: 2024-11-27T23:18:59.456Z
Learning: In the codebase, releases are not scoped to a specific resource.
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.
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/version-manager.ts (4)
packages/db/src/schema/deployment-version.ts (1)
DeploymentVersion(83-83)packages/db/src/schema/policy.ts (1)
policy(43-63)packages/db/src/selectors/index.ts (1)
selector(13-13)packages/db/src/common.ts (1)
takeFirst(9-13)
⏰ 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). (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
packages/rule-engine/src/manager/version-manager.ts (6)
2-2: Import addition looks good.The addition of
isAfterandisBeforefromdate-fnsis appropriate for the new date filtering logic in thegetVersionsForEvaluatemethod.
37-37: Version caching implementation is well-structured.The
versionsproperty enables caching of deployment versions, which aligns with the PR objective of reducing SQL calls.
57-83: Method refactoring successfully implements caching strategy.The renamed
getVersionsMatchingTargetmethod now fetches and caches versions instead of just finding the latest one. The implementation correctly:
- Applies policy selector filtering
- Limits results to 500 versions
- Orders by creation date
- Caches results for subsequent use
120-120: Policy selector condition removal is appropriate.Removing the policy selector condition from
findDesiredVersionis correct since policy filtering is now handled ingetVersionsMatchingTargetand cached versions are used for evaluation.
124-132: Asymmetric timestamp handling approach needs clarification.The
getMinVersionCreatedAtmethod still queries the database whilegetMaxVersionCreatedAtuses cached data. This asymmetry might be intentional but could be confusing for maintainability.Consider documenting why
getMinVersionCreatedAtrequires a fresh database query whilegetMaxVersionCreatedAtcan use cached data, or evaluate if both methods should follow the same pattern.
188-188: Evaluation method correctly uses new filtering approach.The
evaluatemethod now usesgetVersionsForEvaluate()instead of direct database queries, which aligns with the caching strategy and should reduce SQL calls.
this is responsible for most of the query load per the cloudsql insights
Summary by CodeRabbit
New Features
Bug Fixes
Chores