feat: add checks endpoint to release and enhance deployment version environment cell with job and approval handling#539
Conversation
…nvironment cell with job and approval handling
WalkthroughThis update introduces a comprehensive refactor of the deployment version environment UI component, making it modular and policy-aware. It adds new backend API endpoints for policy evaluation and release target listing, as well as new utility functions and types for policy evaluation results. The rule engine is refactored to remove the context parameter from rule evaluation methods, simplifying interfaces and signatures throughout the codebase. Several new UI subcomponents encapsulate specific deployment and policy states, while backend logic now supports querying policy evaluation results and release target data. The codebase is streamlined for clearer separation of concerns and improved maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DeploymentVersionEnvironmentCell
participant API as TRPC API
participant DB as Database
UI->>API: Fetch release targets, policy evaluations, jobs
API->>DB: Query release targets by deployment/environment
API->>DB: Evaluate policies for deployment version/environment
API->>DB: Get jobs for deployment version/environment
API-->>UI: Return release targets, policy evaluation results, jobs
alt Loading
UI->>UI: Show SkeletonCell
else Active jobs
UI->>UI: Show ActiveJobsCell
else No resources
UI->>UI: Show NoReleaseTargetsCell
else Blocked by version selector
UI->>UI: Show BlockedByVersionSelectorCell
else Approval required
UI->>UI: Show ApprovalRequiredCell
else No job agent
UI->>UI: Show NoJobAgentCell
end
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
62-76:⚠️ Potential issue
ConstantMapconstruction may be invalid – verify expected constructor signature
new ConstantMap<string, string>(result.rejectionReason ?? "")passes a string where mostMap-like constructors expect an iterable of [key, value] tuples.
IfConstantMapmirrors the standardMapAPI, this will throw at runtime.Suggested defensive pattern:
- rejectionReasons: new ConstantMap<string, string>( - result.rejectionReason ?? "", - ), + rejectionReasons: new ConstantMap<string, string>( + result.rejectionReason ? [["global", result.rejectionReason]] : [], + ),Replace
"global"with an appropriate key that callers understand, or expose a helper such asConstantMap.fromSingle(key, value).
🧹 Nitpick comments (10)
packages/api/src/router/deployment-version-jobs.ts (1)
140-179: Consider extracting the heavy-join query into a shared helper to avoid duplicationThe new
byEnvironmentprocedure re-implements almost the same multi-join query already present in thelistprocedure (lines 51-95). Maintaining two large, nearly-identical query blocks increases the risk of future divergence (e.g., a new join or filter added in one place but forgotten in the other).A tiny utility that returns a
SelectQueryBuilderfor “deployment-version ↔ release-target ↔ job” with optional filters (environment, search string, etc.) would:
- Centralise business logic / access control,
- Reduce boiler-plate,
- Simplify unit-testing.
Example sketch (showing only the extraction pattern):
function baseDeploymentVersionJobQuery(db: Tx) { return db .from(SCHEMA.releaseTarget) /* … all the joins … */ .innerJoin(SCHEMA.job, eq(SCHEMA.releaseJob.jobId, SCHEMA.job.id)); } // inside list: await baseDeploymentVersionJobQuery(ctx.db) .where(and(eq(SCHEMA.versionRelease.versionId, version.id), /* search */)) // inside byEnvironment: await baseDeploymentVersionJobQuery(ctx.db) .where(and( eq(SCHEMA.versionRelease.versionId, versionId), eq(SCHEMA.releaseTarget.environmentId, environmentId), ))This keeps each router focused on its specific filtering logic rather than the full join choreography.
packages/rule-engine/src/manager/version-rule-engine.ts (2)
80-100:candidatesis mutated in place – prefer a fresh variable to avoid accidental leaksInside the filter loop,
candidatesis reassigned:candidates = result.allowedCandidates;Because
candidatesis also an input parameter, later code might accidentally rely on the original reference (e.g., if this method is refactored to run in parallel). Creating a new variable (nextCandidates) and re-assigning at the end of the iteration keeps intent explicit and guards against subtle bugs.Not critical today, but a low-effort readability win.
130-139: Method name now mismatches its purpose
selectFinalReleasestill talks about release while the engine works with versions. A rename toselectFinalVersion(and corresponding JSDoc tweak) will prevent confusion for future contributors.packages/api/src/router/policy/evaluate.ts (3)
102-123: Parameter naming could mislead future maintainers
getApprovalReasonsreceives a parameter namedversionthat is typedVersion[](plural). The singular name suggests a single object and has already caused at least one earlier bug report in this repo.- version: Version[], + versions: Version[],…and adjust internal references accordingly:
- const result = await rule.filter(version); + const result = await rule.filter(versions);This nitpick improves readability at zero runtime cost.
75-88:getVersionSelectorruns an additional query per policy – consider batchingIn the worst case (large policy sets), the
Promise.allat lines 196-203 will execute N almost-identicalSELECTstatements, each filtering the samedeployment_versionrow against a differentselectorQuery.A more scalable approach is:
- Build one
UNION ALLorVALUEStable of(policy_id, selector_sql)pairs.- Join it once against
deployment_version.- Return a single result set keyed by policy id.
The current implementation is perfectly functional for modest policy counts, but the above can reduce query overhead by an order of magnitude when policies scale.
189-205: Returned structure omits explicit “isAllowed” field – double-check consumer expectationsThe API yields:
rules: { userApprovals: { [policyId]: string[] }, roleApprovals: { … }, anyApprovals: { … }, versionSelector: { [policyId]: boolean } }If the frontend needs a single boolean “allowed” flag per policy (i.e., no rejections and selector passes), it must reconstruct that client-side. This can be error-prone and duplicates backend logic.
Consider returning an explicit
policyResults: Array<{ policy: Policy; allowed: boolean; rejectionReasons: string[]; }>or similar to keep business logic server-side.
packages/rule-engine/src/rules/version-approval-rule.ts (2)
54-60: Guard against negative “missing approvals” valuesWhen
minApprovalsis0,approvals.length ≥ minApprovalsis alwaystrue, but if someone passes a negative number (e.g. mis-configured-1) the subtraction below turns negative and the message becomes confusing:`Missing ${this.options.minApprovals - approvals.length} approvals.`Consider clamping at
0or validatingminApprovalsupfront:-`Missing ${this.options.minApprovals - approvals.length} approvals.`, +`Missing ${Math.max( + 0, + this.options.minApprovals - approvals.length, +)} approvals.`,Even better, throw early if
minApprovals < 0– a negative quorum is logically impossible.
70-80: Potential property collision in approval-record mappers
{ ...record, versionId: record.deploymentVersionId }blindly spreads the DB row.
If the underlying table is ever migrated to include a nativeversionIdcolumn, your override silently wins in runtime JS but TypeScript will not warn because both arestring. Prefer explicit construction to avoid future foot-guns:-return records.map((record) => ({ - ...record, - versionId: record.deploymentVersionId, -})); +return records.map(({ deploymentVersionId, status, userId, reason }) => ({ + versionId: deploymentVersionId, + status, + userId, + reason, +}));The same comment applies to the two functions below.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx (2)
338-341: Loading state can flicker due to derived flag
isLoadingis a composite of three queries’isLoadingflags. When any query finishes early the component re-renders, dropping the skeleton and potentially showing incomplete data until the remaining queries resolve.If you want the skeleton to persist until all queries complete, keep it as is. Otherwise, consider progressive rendering or a dedicated
isFetchingflag which remainstrueduring background refetches:const isInitialLoad = releaseTargets === undefined || policyEvaluations === undefined || jobs === undefined; if (isInitialLoad) return <SkeletonCell />;This avoids UI flicker and makes intent explicit.
208-246: Repeated_.uniqBywork can be memoised
getPoliciesWithApprovalRequiredrecomputes, and because it’s created inside the component it is re-declared every render. With large policy sets this is avoidable churn.const policiesWithApprovalRequired = React.useMemo( () => (policyEvaluations ? getPoliciesWithApprovalRequired(policyEvaluations) : []), [policyEvaluations], );Similarly for
getPoliciesBlockingByVersionSelector.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx(2 hunks)apps/webservice/src/app/urls.ts(1 hunks)packages/api/src/root.ts(2 hunks)packages/api/src/router/deployment-version-jobs.ts(2 hunks)packages/api/src/router/policy/evaluate.ts(1 hunks)packages/api/src/router/policy/router.ts(1 hunks)packages/api/src/router/release-target.ts(1 hunks)packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts(7 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(3 hunks)packages/rule-engine/src/manager/version-manager.ts(2 hunks)packages/rule-engine/src/manager/version-rule-engine.ts(5 hunks)packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(10 hunks)packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts(6 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(2 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(3 hunks)packages/rule-engine/src/types.ts(2 hunks)packages/rule-engine/src/utils/merge-policies.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/urls.tspackages/api/src/root.tspackages/api/src/router/policy/router.tspackages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.tspackages/rule-engine/src/manager/version-manager.tspackages/rule-engine/src/rules/deployment-deny-rule.tspackages/rule-engine/src/utils/merge-policies.tspackages/api/src/router/release-target.tspackages/rule-engine/src/types.tspackages/rule-engine/src/manager/__tests__/version-rule-engine.test.tspackages/rule-engine/src/rules/__tests__/version-approval-rule.test.tspackages/rule-engine/src/manager/version-rule-engine.tspackages/api/src/router/deployment-version-jobs.tspackages/api/src/router/policy/evaluate.tspackages/rule-engine/src/rules/version-approval-rule.tspackages/rule-engine/src/manager/version-manager-rules.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx
🧠 Learnings (1)
packages/api/src/router/release-target.ts (1)
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.
🧬 Code Graph Analysis (6)
packages/api/src/root.ts (1)
packages/api/src/router/release-target.ts (1)
releaseTargetRouter(10-80)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
packages/rule-engine/src/types.ts (1)
PreValidationResult(38-41)
packages/api/src/router/release-target.ts (1)
packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(173-173)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
packages/rule-engine/src/types.ts (2)
RuleSelectionResult(80-83)isPreValidationRule(60-64)
packages/api/src/router/deployment-version-jobs.ts (2)
packages/api/src/trpc.ts (1)
protectedProcedure(173-173)packages/db/src/schema/job.ts (1)
job(75-107)
packages/rule-engine/src/rules/version-approval-rule.ts (2)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(11-17)packages/rule-engine/src/types.ts (1)
RuleEngineRuleResult(23-26)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (26)
apps/webservice/src/app/urls.ts (1)
223-223: Looks good - Added checks URL endpoint for releasesThis change adds a new method to access the "checks" endpoint for a release, which aligns with the PR objective of enhancing deployment version environment cells with job and approval handling capabilities.
packages/api/src/root.ts (1)
8-8: Properly integrated release target routerGood job adding the new release target router. This integration follows the existing pattern of imports and router registrations in the file.
Also applies to: 27-27
packages/api/src/router/policy/router.ts (1)
14-14: Policy evaluation endpoints correctly integratedThe addition of the policy evaluation endpoint follows the established pattern in the router file. This change supports the PR's objective of enhancing deployment version handling with policy evaluation results.
Also applies to: 21-21
packages/rule-engine/src/utils/merge-policies.ts (1)
38-43: Improved null handling in policy mergingThis is a good enhancement that properly handles null values during policy merging. The change ensures that non-null values are preserved when merging with null values, which prevents data loss and maintains integrity of the merged policies.
Before this change, the merge function only had special handling for arrays, but now it correctly handles cases where one of the values is null while the other contains meaningful data.
Also applies to: 44-44, 46-46
packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts (2)
31-31: Method signature update looks goodThe removal of the context parameter from
rule.filtercalls and the updated mock function expectation align with the broader refactoring to simplify rule evaluation interfaces.Also applies to: 35-35
88-88: Consistent parameter removal across testsAll calls to
rule.filter()have been consistently updated to remove the context parameter, maintaining test coverage while aligning with the simplified interface.Also applies to: 142-142, 174-174, 226-226
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (3)
2-2: Import statement update is appropriateThe import reordering looks good and follows best practices.
32-32: Method signature update looks goodThe removal of the context parameter from the
passing()method calls aligns with the broader refactoring effort to simplify the rule engine interfaces.Also applies to: 53-53, 74-74
95-95: Consistent parameter removal across test casesAll remaining tests have been consistently updated to reflect the simplified
passing()method signature, ensuring tests remain valid with the refactored interface.Also applies to: 103-103, 111-111, 130-130, 138-138, 161-161, 168-168, 182-182, 189-189, 212-212, 219-219, 233-233, 240-240
packages/rule-engine/src/rules/deployment-deny-rule.ts (2)
10-10: Import cleanup looks goodProperly simplified the import statement to remove unused types, retaining only the necessary
PreValidationResultandPreValidationRuletypes.
90-90: Method signature simplification is appropriateThe
passing()method signature has been updated to remove the unused context parameter while maintaining the same return type and functionality, which aligns with the overall refactoring effort.packages/rule-engine/src/manager/version-manager.ts (2)
18-18: Import cleanup is appropriateProperly simplified the import statement to remove the unused
RuleEngineContexttype while retaining the necessary types.
174-174: Method call update is consistent with the refactoringThe call to
engine.evaluate()now correctly passes only the versions array, aligning with the simplified signature from the rule engine refactoring. This change maintains functionality while simplifying the interface.packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts (3)
3-3: Import updated to match interface changes.The import statement was updated to use only the necessary types after the removal of
RuleEngineContextfrom the codebase.
12-12: Method signature simplified by removing context parameter.The
filtermethod signature has been simplified to only accept the candidates array, removing the previously required context parameter. This aligns with the broader refactoring to simplify the rule engine interfaces.
69-69: Evaluate method calls updated to match new signature.All calls to
engine.evaluate()have been updated to remove the context parameter, aligning with the simplified interface in theVersionRuleEngineclass.Also applies to: 82-82, 93-93, 104-104, 112-112, 126-126
packages/rule-engine/src/manager/version-manager-rules.ts (3)
23-33: Function exported for external use.The
versionAnyApprovalRulefunction has been exported, making it available for use outside this module. This likely supports the new policy evaluation procedure mentioned in the PR summary.
35-46: Function exported for external use.The
versionRoleApprovalRulefunction has been exported, making it available for use outside this module. This enables the new policy evaluation procedure to use this rule directly.
48-59: Function exported for external use.The
versionUserApprovalRulefunction has been exported, making it available for use outside this module. This completes the set of approval rule functions that can now be used by external modules.packages/rule-engine/src/types.ts (3)
31-36: Interface simplified by removing context parameter.The
FilterRuleinterface has been updated to remove thecontextparameter from thefiltermethod signature, streamlining the interface and simplifying rule implementations.
44-46: Method signature simplified by removing context parameter.The
passingmethod in thePreValidationRuleinterface no longer takes any parameters, simplifying the interface and making it more focused on the rule's internal state.
85-87: Interface simplified by removing context parameter.The
RuleEnginetype'sevaluatemethod signature has been updated to only accept the candidates array, removing the previously required context parameter. This makes the interface more straightforward and focuses rule evaluation solely on the candidate inputs.packages/api/src/router/release-target.ts (3)
10-80: Well-implemented TRPC router for release targets.The new
releaseTargetRouterprovides a well-structured query procedure with comprehensive authorization checks and appropriate filters. The implementation follows best practices by:
- Enforcing at least one filter parameter to prevent querying all records
- Performing authorization checks for each entity type
- Eagerly loading related entities for common use cases
- Limiting results to 500 entries to prevent excessive data retrieval
27-53: Thorough authorization checks for all entity types.The implementation properly checks permissions for each provided entity ID (resource, environment, deployment), ensuring the user has appropriate access before returning data.
58-78: Query implementation with proper filtering and relations.The database query implementation:
- Dynamically builds the where clause based on provided filters
- Filters out undefined conditions using
isPresent- Includes related entities to minimize additional queries
- Sets a reasonable limit to prevent excessive data retrieval
packages/api/src/router/deployment-version-jobs.ts (1)
156-178: Double-checkDISTINCT ONordering guarantees
selectDistinctOn([SCHEMA.releaseTarget.id])
combined with
.orderBy(SCHEMA.releaseTarget.id, desc(SCHEMA.job.createdAt))is the correct Postgres idiom: the first
orderByexpression matches theDISTINCT ONkey, and the subsequent expression chooses the “latest” job per target. 👍Just make sure every future edit keeps the
releaseTarget.idas the firstORDER BYelement; otherwise Postgres will silently return unexpected rows.
| async filter(candidates: Version[]): Promise<RuleEngineRuleResult<Version>> { | ||
| const rejectionReasons = new Map<string, string>(); | ||
| const versionIds = _(candidates) | ||
| .map((r) => r.id) | ||
| .uniq() | ||
| .value(); | ||
| const approvalRecords = await this.options.getApprovalRecords( | ||
| context, | ||
| versionIds, | ||
| ); | ||
| const approvalRecords = await this.options.getApprovalRecords(versionIds); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pre-index approvalRecords to avoid O(N²) scans
Array.filter is executed for every candidate, causing a second pass over approvalRecords for each element (O(candidates × records)).
For a large release set this grows quickly and is unnecessary because the records never change during this method.
-const approvalRecords = await this.options.getApprovalRecords(versionIds);
-const allowedCandidates = candidates.filter((release) => {
- const records = approvalRecords.filter((r) => r.versionId === release.id);
+const approvalRecords = await this.options.getApprovalRecords(versionIds);
+const recordsByVersion = _.groupBy(approvalRecords, "versionId");
+
+const allowedCandidates = candidates.filter((release) => {
+ const records = recordsByVersion[release.id] ?? [];This drops the complexity to O(candidates + records) while leaving behaviour unchanged.
📝 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 filter(candidates: Version[]): Promise<RuleEngineRuleResult<Version>> { | |
| const rejectionReasons = new Map<string, string>(); | |
| const versionIds = _(candidates) | |
| .map((r) => r.id) | |
| .uniq() | |
| .value(); | |
| const approvalRecords = await this.options.getApprovalRecords( | |
| context, | |
| versionIds, | |
| ); | |
| const approvalRecords = await this.options.getApprovalRecords(versionIds); | |
| async filter(candidates: Version[]): Promise<RuleEngineRuleResult<Version>> { | |
| const rejectionReasons = new Map<string, string>(); | |
| const versionIds = _(candidates) | |
| .map((r) => r.id) | |
| .uniq() | |
| .value(); | |
| const approvalRecords = await this.options.getApprovalRecords(versionIds); | |
| const recordsByVersion = _.groupBy(approvalRecords, "versionId"); | |
| const allowedCandidates = candidates.filter((release) => { | |
| const records = recordsByVersion[release.id] ?? []; | |
| // …rest of your filter logic here… |
| const hasNoJobAgent = deployment.jobAgentId == null; | ||
| if (hasNoJobAgent) return <NoJobAgentCell tag={deploymentVersion.tag} />; | ||
|
|
||
| return <NoReleaseTargetsCell tag={deploymentVersion.tag} />; | ||
| }; |
There was a problem hiding this comment.
Fallback path repeats “No resources” even when resources exist
The final else returns <NoReleaseTargetsCell> although you already asserted hasNoReleaseTargets === false.
This branch will execute when:
- There are release targets,
- There’s a job agent,
- No blocking policies,
- No approvals required,
- No active jobs.
In that case showing “No resources” is misleading. Perhaps an “Idle” or “Ready” state is intended?
-return <NoReleaseTargetsCell tag={deploymentVersion.tag} />;
+return (
+ <div className="flex h-full w-full items-center justify-center text-xs text-muted-foreground">
+ Idle
+ </div>
+);📝 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 hasNoJobAgent = deployment.jobAgentId == null; | |
| if (hasNoJobAgent) return <NoJobAgentCell tag={deploymentVersion.tag} />; | |
| return <NoReleaseTargetsCell tag={deploymentVersion.tag} />; | |
| }; | |
| const hasNoJobAgent = deployment.jobAgentId == null; | |
| if (hasNoJobAgent) return <NoJobAgentCell tag={deploymentVersion.tag} />; | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-xs text-muted-foreground"> | |
| Idle | |
| </div> | |
| ); | |
| }; |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores