feat: v2 approval ui #488
feat: v2 approval ui #488
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApprovalCheck (UI)
participant ApprovalDialog (UI)
participant API
participant DB
User->>ApprovalCheck (UI): Clicks "Approve" or "Reject"
ApprovalCheck (UI)->>ApprovalDialog (UI): Opens dialog
User->>ApprovalDialog (UI): Submits approval/rejection with optional reason
ApprovalDialog (UI)->>API: Calls addRecord mutation
API->>DB: Inserts approval/rejection record
API->>API: Enqueue evaluation jobs for release targets
API-->>ApprovalDialog (UI): Returns success
ApprovalDialog (UI)->>ApprovalCheck (UI): Triggers data refresh
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (3)
packages/api/src/router/deployment-version-checks.ts (1)
79-138: Well-structured approval record mutation implementationThe new
addRecordmutation is well-implemented with:
- Proper input validation using zod schema
- Appropriate authorization checks
- Transaction-safe database operations
- Notification of affected systems via the event queue
The code handles the approval status correctly and ensures that approval timestamps are only set for approved records.
Just one suggestion to consider for error handling:
addRecord: protectedProcedure .input( z.object({ deploymentVersionId: z.string().uuid(), environmentId: z.string().uuid(), status: z.nativeEnum(SCHEMA.ApprovalStatus), reason: z.string().optional(), }), ) .meta({ authorizationCheck: ({ canUser, input }) => canUser.perform(Permission.DeploymentVersionGet).on({ type: "deploymentVersion", id: input.deploymentVersionId, }), }) .mutation(async ({ ctx, input }) => { const { deploymentVersionId, environmentId, status, reason } = input; + try { const record = await ctx.db .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 .select() .from(SCHEMA.deploymentVersion) .innerJoin( SCHEMA.releaseTarget, eq( SCHEMA.deploymentVersion.deploymentId, SCHEMA.releaseTarget.deploymentId, ), ) .where( and( eq(SCHEMA.deploymentVersion.id, deploymentVersionId), eq(SCHEMA.releaseTarget.environmentId, environmentId), ), ); const targets = rows.map((row) => row.release_target); if (targets.length > 0) await getQueue(Channel.EvaluateReleaseTarget).addBulk( targets.map((rt) => ({ name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, data: rt, })), ); return record; + } catch (error) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to create approval record: ${error instanceof Error ? error.message : String(error)}`, + }); + } }),apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/Approval.tsx (2)
66-83: Consider simplifying dialog footer layout.The footer layout uses several nested flex containers and complex class combinations. Consider simplifying this while maintaining the desired layout.
-<DialogFooter className="flex w-full flex-row items-center justify-between sm:justify-between"> - <Button variant="outline" onClick={() => setOpen(false)}> - Cancel - </Button> - <div className="flex gap-2"> - <Button - variant="outline" - onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Rejected)} - > - Reject - </Button> - <Button - onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Approved)} - > - Approve - </Button> - </div> +<DialogFooter className="flex justify-between"> + <Button variant="outline" onClick={() => setOpen(false)}> + Cancel + </Button> + <div className="flex gap-2"> + <Button + variant="outline" + onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Rejected)} + > + Reject + </Button> + <Button + onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Approved)} + > + Approve + </Button> + </div> </DialogFooter>
125-130: Consider extracting duplicated UI structure.There's duplicated UI structure between this section and lines 147-152. Consider extracting this into a reusable component or function to improve maintainability.
+ const renderApprovalUI = () => ( + <div className="flex items-center justify-between"> + <div className="flex items-center gap-2"> + <Waiting /> Not enough approvals + </div> + <ApprovalDialog {...props} onSubmit={invalidate} /> + </div> + ); if (rejectionReasonEntries.length > 0) { return ( <TooltipProvider> <Tooltip> - <TooltipTrigger> - <div className="flex items-center justify-between"> - <div className="flex items-center gap-2"> - <Waiting /> Not enough approvals - </div> - <ApprovalDialog {...props} onSubmit={invalidate} /> - </div> - </TooltipTrigger> + <TooltipTrigger>{renderApprovalUI()}</TooltipTrigger> <TooltipContent> <ul> {rejectionReasonEntries.map(([reason, comment]) => ( <li key={reason}> {reason}: {comment} </li> ))} </ul> </TooltipContent> </Tooltip> </TooltipProvider> ); } - return ( - <div className="flex items-center justify-between"> - <div className="flex items-center gap-2"> - <Waiting /> Not enough approvals - </div> - <ApprovalDialog {...props} onSubmit={invalidate} /> - </div> - ); + return renderApprovalUI();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/Approval.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/DenyWindow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx(0 hunks)packages/api/src/router/deployment-version-checks.ts(3 hunks)packages/db/src/schema/rules/approval-base.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx
🧰 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]/checksv2/_components/flow-diagram/checks/DenyWindow.tsxpackages/rule-engine/src/manager/version-manager-rules.tspackages/db/src/schema/rules/approval-base.tspackages/api/src/router/deployment-version-checks.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/Approval.tsx
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/version-manager-rules.ts (1)
packages/db/src/schema/policy.ts (1)
policy(30-46)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
packages/db/src/schema/rules/approval-base.ts (1)
45-48: LGTM: Clean TypeScript enum implementation for approval statusThe new
ApprovalStatusenum nicely mirrors the values in the existing PostgreSQL enum, providing type safety when working with approval statuses in TypeScript code.packages/rule-engine/src/manager/version-manager-rules.ts (1)
72-76:❓ Verification inconclusive
Temporary disabling of deny windows functionality appears well documented
The code changes temporarily remove deny windows functionality from the rule engine due to issues with the rrule package. The change is clearly documented with a comment and GitHub issue reference, making it clear this is a temporary modification.
However, to ensure this temporary change doesn't remain permanently, consider:
🏁 Script executed:
#!/bin/bash # Check if there are any issues or TODOs tracking the re-enablement of this functionality gh issue list --repo "jkbrzt/rrule" --search "478" --json number,title,state echo "Searching for internal tracking issues..." rg -i "rrule|deny window" --glob "*.{md,txt,yaml,yml}"Length of output: 399
Temporary Removal Verified – Consider Adding a Tracking Note
- The temporary disabling of the deny windows functionality is appropriately documented with a GitHub issue reference.
- The verification script did not return any active issues for rrule#478, nor did it uncover internal documentation tracking this functionality.
- To ensure this temporary change doesn’t become permanent, consider adding a TODO or internal tracking ticket to revisit re-enabling deny windows once the upstream rrule issue is addressed.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/DenyWindow.tsx (2)
23-24: Updates to DenyWindow component align with backend changesThe UI updates correctly handle the temporary disabling of deny window functionality. The component now properly interprets the simplified API response.
29-29: Loading message updated to be more specificGood adjustment of the loading message to reflect what is actually being loaded.
packages/api/src/router/deployment-version-checks.ts (2)
6-6: Added event imports for approval workflowThe addition of Channel and getQueue imports supports the new approval workflow enqueuing functionality.
156-156: Simplified deny window router for temporary disablingThis change aligns with the temporary disabling of deny window functionality in the rule engine.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checksv2/_components/flow-diagram/checks/Approval.tsx (6)
24-29: Props typing looks good.Clean definition of the ApprovalDialog component with well-typed props. Using proper React.FC typing with explicit parameter definitions enhances type safety.
47-58: Dialog structure looks good.The dialog structure is well-organized with proper header, description and content. Including the version tag in the confirmation message provides good context for the user.
60-64: Textarea implementation is correct.Properly implemented controlled component for the textarea with value and onChange handler.
89-94: Props refactoring looks good.Converting from destructured props to a single props object makes the component more maintainable as props can be easily passed to child components and API calls. Adding the versionTag prop is appropriate for the dialog functionality.
95-99: Query and invalidation implementation is correct.Properly passing the props object to both the useQuery hook and the invalidation function ensures consistency.
147-152: New layout structure improves UX.The new layout structure with justified content between the status message and approval button creates a better user experience and makes the approval action more prominent.
| const addRecord = | ||
| api.deployment.version.checks.approval.addRecord.useMutation(); | ||
|
|
||
| const [reason, setReason] = useState(""); | ||
|
|
||
| const handleSubmit = (status: SCHEMA.ApprovalStatus) => | ||
| addRecord | ||
| .mutateAsync({ | ||
| deploymentVersionId: versionId, | ||
| environmentId, | ||
| status, | ||
| reason, | ||
| }) | ||
| .then(() => setOpen(false)) | ||
| .then(() => onSubmit()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state and error handling to mutation.
The mutation lacks loading state feedback and error handling. If the API call fails, the user won't see any feedback.
- const addRecord =
- api.deployment.version.checks.approval.addRecord.useMutation();
+ const addRecord =
+ api.deployment.version.checks.approval.addRecord.useMutation({
+ onError: (error) => {
+ // Handle error (e.g., show toast notification)
+ console.error("Failed to submit approval:", error);
+ }
+ });
const [reason, setReason] = useState("");
const handleSubmit = (status: SCHEMA.ApprovalStatus) =>
addRecord
.mutateAsync({
deploymentVersionId: versionId,
environmentId,
status,
reason,
})
.then(() => setOpen(false))
.then(() => onSubmit());Consider also disabling the buttons during submission:
<Button
variant="outline"
+ disabled={addRecord.isLoading}
onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Rejected)}
>
- Reject
+ {addRecord.isLoading ? "Submitting..." : "Reject"}
</Button>
<Button
+ disabled={addRecord.isLoading}
onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Approved)}
>
- Approve
+ {addRecord.isLoading ? "Submitting..." : "Approve"}
</Button>📝 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 addRecord = | |
| api.deployment.version.checks.approval.addRecord.useMutation(); | |
| const [reason, setReason] = useState(""); | |
| const handleSubmit = (status: SCHEMA.ApprovalStatus) => | |
| addRecord | |
| .mutateAsync({ | |
| deploymentVersionId: versionId, | |
| environmentId, | |
| status, | |
| reason, | |
| }) | |
| .then(() => setOpen(false)) | |
| .then(() => onSubmit()); | |
| const addRecord = | |
| api.deployment.version.checks.approval.addRecord.useMutation({ | |
| onError: (error) => { | |
| // Handle error (e.g., show toast notification) | |
| console.error("Failed to submit approval:", error); | |
| } | |
| }); | |
| const [reason, setReason] = useState(""); | |
| const handleSubmit = (status: SCHEMA.ApprovalStatus) => | |
| addRecord | |
| .mutateAsync({ | |
| deploymentVersionId: versionId, | |
| environmentId, | |
| status, | |
| reason, | |
| }) | |
| .then(() => setOpen(false)) | |
| .then(() => onSubmit()); | |
| // In the JSX render block: | |
| <Button | |
| variant="outline" | |
| disabled={addRecord.isLoading} | |
| onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Rejected)} | |
| > | |
| {addRecord.isLoading ? "Submitting..." : "Reject"} | |
| </Button> | |
| <Button | |
| disabled={addRecord.isLoading} | |
| onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Approved)} | |
| > | |
| {addRecord.isLoading ? "Submitting..." : "Approve"} | |
| </Button> |
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Other Changes