chore: clean up checks endpoints #552
Conversation
WalkthroughThis change removes the legacy deployment version checks API and associated React components, refactoring the frontend to use a new unified policy evaluation API for approval and version selector checks. Approval record mutations are now handled via a new endpoint. Deny window checks and related utilities are deleted, and environment data is now derived from release targets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApprovalCheck (React)
participant PolicyEvalAPI
participant ApprovalDialog (React)
participant AddApprovalRecordAPI
User->>ApprovalCheck (React): View approval status
ApprovalCheck (React)->>PolicyEvalAPI: Evaluate approval policy
PolicyEvalAPI-->>ApprovalCheck (React): Return approval status
User->>ApprovalDialog (React): Submit approval
ApprovalDialog (React)->>AddApprovalRecordAPI: addApprovalRecord mutation
AddApprovalRecordAPI-->>ApprovalDialog (React): Return approval record
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/api/src/router/deployment-version.ts (3)
391-396: Consider using a more specific permission for approval actions.While using
DeploymentVersionGetpermission works, since this is a mutation that creates an approval record, it might be more semantically appropriate to check for a more specific permission likeDeploymentVersionApproveif such a permission exists in your permission model.
397-411: Consider wrapping database operations in a transaction.The procedure performs multiple database operations (insert approval record, query release targets). To ensure atomicity, consider wrapping these operations in a transaction to prevent partial updates if an error occurs.
.mutation(async ({ ctx, input }) => { const { deploymentVersionId, environmentId, status, reason } = input; + return await ctx.db.transaction(async (tx) => { - const record = await ctx.db + const record = await tx .insert(SCHEMA.policyRuleAnyApprovalRecord) .values({ deploymentVersionId, userId: ctx.session.user.id, status, reason, approvedAt: status === SCHEMA.ApprovalStatus.Approved ? new Date() : null, }) .returning(); - const rows = await ctx.db + const rows = await tx // Rest of the query... // Queue operations and return record + return record; + }); });
412-436: Add error handling for queue operations.When adding jobs to the evaluation queue, there's no explicit error handling. While the coding guidelines allow for this, it would be beneficial to add explicit error handling for queue operations since they're external system calls.
if (targets.length > 0) - await getQueue(Channel.EvaluateReleaseTarget).addBulk( - targets.map((rt) => ({ - name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, - data: rt, - })), - ); + try { + await getQueue(Channel.EvaluateReleaseTarget).addBulk( + targets.map((rt) => ({ + name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, + data: rt, + })), + ); + } catch (error) { + console.error("Failed to add evaluation jobs to queue:", error); + // Consider how to handle this error - rethrow or continue + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/DenyWindow.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx(1 hunks)packages/api/src/router/deployment-version-checks/approvals.ts(0 hunks)packages/api/src/router/deployment-version-checks/deny-window.ts(0 hunks)packages/api/src/router/deployment-version-checks/router.ts(0 hunks)packages/api/src/router/deployment-version-checks/utils.ts(0 hunks)packages/api/src/router/deployment-version-checks/version-selector.ts(0 hunks)packages/api/src/router/deployment-version.ts(1 hunks)
💤 Files with no reviewable changes (7)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsx
- packages/api/src/router/deployment-version-checks/deny-window.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/DenyWindow.tsx
- packages/api/src/router/deployment-version-checks/router.ts
- packages/api/src/router/deployment-version-checks/approvals.ts
- packages/api/src/router/deployment-version-checks/utils.ts
- packages/api/src/router/deployment-version-checks/version-selector.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/page.tsxpackages/api/src/router/deployment-version.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx
🧬 Code Graph Analysis (1)
packages/api/src/router/deployment-version.ts (2)
packages/api/src/trpc.ts (1)
protectedProcedure(173-173)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx (1)
29-29: API endpoint update looks goodThe mutation hook has been updated to use the new consolidated endpoint
api.deployment.version.addApprovalRecord.useMutation()instead of the previous checks-specific endpoint. This aligns with the PR objective of cleaning up the checks endpoints.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/page.tsx (2)
4-4: Good addition of lodash importThe lodash import is appropriate for the new data transformation logic.
48-55: Good refactor to derive environments from release targetsThe implementation properly fetches release targets and extracts unique environments using
lodash.uniqBy. This approach is more normalized than having a specialized endpoint and aligns with the PR objective of cleaning up checks endpoints.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx (2)
8-11: API migration to unified policy evaluation looks goodThe component now correctly uses the new unified policy evaluation API with properly ordered parameters.
13-15: Improved version selector check logicThe new implementation properly extracts the
versionSelectorobject from policy evaluation rules and verifies that all values are truthy. This is more robust than the previous implementation and handles nested data structures appropriately.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsx (2)
13-15: API migration to unified policy evaluation looks goodThe component now correctly uses the new unified policy evaluation API and updates the invalidation function accordingly.
17-30: More granular approval status logicThe new implementation checks three distinct approval categories (
anyApprovals,userApprovals, androleApprovals) and ensures all rejection reasons arrays are empty for each. This provides a more comprehensive assessment of approval status.packages/api/src/router/deployment-version.ts (1)
381-439: LGTM: Good implementation of the new approval record mutation procedure.The new
addApprovalRecordprocedure properly handles creating approval records and triggering evaluation jobs for related release targets. This aligns with the PR objective of moving to a unified policy evaluation API for approval checks.Key strengths:
- Proper authorization checks
- Correctly setting
approvedAtbased on approval status- Triggering appropriate evaluation jobs after creating the record
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores