fix: Make approvals per-environment and clean up release reactflow #279
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of environment policy approvals across multiple components and services. The primary change involves renaming Changes
Sequence DiagramsequenceDiagram
participant User
participant WebUI
participant API
participant Database
User->>WebUI: Initiate Deployment
WebUI->>API: Request Environment Approval
API->>Database: Check Approval Status
Database-->>API: Return Approval Details
API-->>WebUI: Provide Approval Information
User->>WebUI: Approve/Reject Deployment
WebUI->>API: Submit Approval Decision
API->>Database: Update Approval Status
Database-->>API: Confirm Update
API-->>WebUI: Confirm Approval Status
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
adityachoudhari26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/StatusIcons.tsx (6)
3-7: Add accessibility attributes for screen readers.The success icon should be labeled for screen readers.
-export const Passing: React.FC = () => ( +export const Passing: React.FC = () => ( <div className="rounded-full bg-green-400 p-0.5 dark:text-black"> - <IconCheck strokeWidth={3} className="h-3 w-3" /> + <IconCheck strokeWidth={3} className="h-3 w-3" aria-label="Success" role="img" /> </div> );
9-13: Add accessibility attributes for screen readers.The error icon should be labeled for screen readers.
-export const Failing: React.FC = () => ( +export const Failing: React.FC = () => ( <div className="rounded-full bg-red-400 p-0.5 dark:text-black"> - <IconX strokeWidth={3} className="h-3 w-3" /> + <IconX strokeWidth={3} className="h-3 w-3" aria-label="Error" role="img" /> </div> );
15-19: Move animation to icon for better performance.The animation should be applied to the icon instead of the container for better performance.
export const Waiting: React.FC = () => ( - <div className="animate-spin rounded-full bg-blue-400 p-0.5 dark:text-black"> - <IconLoader2 strokeWidth={3} className="h-3 w-3" /> + <div className="rounded-full bg-blue-400 p-0.5 dark:text-black"> + <IconLoader2 strokeWidth={3} className="h-3 w-3 animate-spin" aria-label="Loading" role="img" /> </div> );
21-25: Add accessibility attributes for screen readers.The loading icon should be labeled for screen readers.
export const Loading: React.FC = () => ( <div className="rounded-full bg-muted-foreground p-0.5 dark:text-black"> - <IconLoader2 strokeWidth={3} className="h-3 w-3 animate-spin" /> + <IconLoader2 strokeWidth={3} className="h-3 w-3 animate-spin" aria-label="Loading" role="img" /> </div> );
27-31: Add accessibility attributes for screen readers.The cancelled icon should be labeled for screen readers.
export const Cancelled: React.FC = () => ( <div className="rounded-full bg-neutral-400 p-0.5 dark:text-black"> - <IconMinus strokeWidth={3} className="h-3 w-3" /> + <IconMinus strokeWidth={3} className="h-3 w-3" aria-label="Cancelled" role="img" /> </div> );
1-31: Consider adding TypeScript props interface for future extensibility.While the components are currently stateless, adding a props interface would make them more maintainable and extensible.
interface StatusIconProps { className?: string; size?: number; strokeWidth?: number; } export const Passing: React.FC<StatusIconProps> = ({ className = "h-3 w-3", strokeWidth = 3, ...props }) => ( <div className="rounded-full bg-green-400 p-0.5 dark:text-black"> <IconCheck strokeWidth={strokeWidth} className={className} aria-label="Success" role="img" {...props} /> </div> );apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/PolicyApprovalRow.tsx (1)
Line range hint
20-33: Consider improving error handling for undefined environment.While the null check for undefined environment is good, consider providing more context in the error message for easier debugging.
- console.error("Environment is undefined for approval:", approval); + console.error( + `Environment is undefined for approval. Release ID: ${approval.releaseId}, Environment ID: ${approval.environmentId}`, + approval + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/jobs/src/policy-checker/index.ts(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/PolicyApprovalRow.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/StatusIcons.tsx(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(3 hunks)packages/api/src/router/environment-approval.ts(1 hunks)packages/api/src/router/environment-policy.ts(1 hunks)packages/api/src/router/environment.ts(2 hunks)packages/db/drizzle/0052_graceful_taskmaster.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment.ts(1 hunks)packages/job-dispatch/src/policies/manual-approval.ts(3 hunks)packages/job-dispatch/src/policy-create.ts(3 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx
🧰 Additional context used
📓 Path-based instructions (16)
packages/api/src/router/environment.ts (1)
Pattern **/*.{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.
packages/job-dispatch/src/policies/manual-approval.ts (1)
Pattern **/*.{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.
packages/job-dispatch/src/policy-create.ts (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (1)
Pattern **/*.{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/api/v1/jobs/[jobId]/route.ts (1)
Pattern **/*.{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)/_components/reactflow/edges.ts (1)
Pattern **/*.{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.
packages/db/src/schema/environment.ts (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)
Pattern **/*.{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.
packages/api/src/router/environment-policy.ts (1)
Pattern **/*.{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.
packages/api/src/router/environment-approval.ts (1)
Pattern **/*.{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/jobs/src/policy-checker/index.ts (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/StatusIcons.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/PolicyApprovalRow.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (1)
Pattern **/*.{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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (25)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/StatusIcons.tsx (1)
1-2: LGTM! Clean and specific imports.All imported icons are used in the components with no unnecessary imports.
packages/api/src/router/environment-approval.ts (3)
16-64: 'byReleaseId' procedure is correctly implemented.The
byReleaseIdquery procedure accurately retrieves environment approval records based on the provided release ID and optional status. The joins and filtering logic are properly constructed, and the authorization checks are appropriately set up.
66-112: 'approve' mutation updates approval status and dispatches jobs correctly.The
approvemutation procedure effectively updates the environment approval status to "approved" and dispatches the pending job triggers associated with the release and environment. Database updates and job dispatch are handled correctly with appropriate authorization.
153-178: 'statusByReleaseEnvironmentId' procedure retrieves approval status effectively.The
statusByReleaseEnvironmentIdquery procedure correctly fetches the approval status for a specific release and environment ID. The use oftakeFirstOrNullensures that either the first matching record ornullis returned, which is appropriate.packages/api/src/router/environment-policy.ts (1)
22-22: Import of 'handleEnvironmentPolicyReleaseChannelUpdate' is appropriate and necessary.The addition of the
handleEnvironmentPolicyReleaseChannelUpdateimport aligns with its usage in theupdatemutation. This ensures that updates to release channels are properly handled when environment policies are modified.packages/job-dispatch/src/policy-create.ts (2)
8-8: Updated import statements reflect schema changes.The import of
environmentApprovalreplacesenvironmentPolicyApproval, consistent with the schema updates. This change ensures that the code aligns with the new database schema.
Line range hint
19-54: Insertion into 'environmentApproval' uses correct identifiers.The
createJobApprovalsfunction now inserts intoenvironmentApprovalusingenvironmentIdandreleaseId, which accurately reflects the updated schema where approvals are associated directly with environments rather than policies. This change is appropriate and maintains data integrity.apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.ts (1)
27-34: LGTM! Clean and simplified edge creation logic.The refactoring improves code clarity by:
- Removing the intermediate release sequencing step
- Creating direct connections between policies and environments
- Maintaining the same functional behavior
packages/job-dispatch/src/policies/manual-approval.ts (3)
3-3: LGTM! Added required import forandoperator.The import of the
andoperator is necessary for the new join condition.
36-40: LGTM! Updated join condition for environment approvals.The new join condition correctly uses both
releaseIdandenvironmentIdto ensure proper approval matching.
53-53: LGTM! Updated approval status check.The filter condition correctly references the new
environment_approvalschema.apps/jobs/src/policy-checker/index.ts (2)
16-16: LGTM! Updated approval gate condition.The condition correctly references the new
environmentApprovalschema.
32-38: LGTM! Updated join conditions for environment approvals.The join conditions correctly use:
environmentIdandreleaseIdfor proper approval matching- Consistent schema references
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/PolicyApprovalRow.tsx (2)
5-5: LGTM! Updated types to reflect environment focus.The type changes correctly reflect the new schema structure.
Also applies to: 15-17
Line range hint
35-65: LGTM! Updated mutation paths and parameters.The mutations correctly:
- Use the new API paths under
environment.approval- Pass the correct parameters (
releaseIdandenvironmentId)packages/db/drizzle/0052_graceful_taskmaster.sql (1)
11-11: Verify cascade delete implications.The foreign key constraints use
ON DELETE cascadefor environment and release references, which could lead to unintended data loss. Consider ifON DELETE restrictwould be more appropriate to prevent accidental deletions.Run this script to check for potential data loss scenarios:
Also applies to: 17-17, 23-23
✅ Verification successful
Cascade delete constraints are appropriate for environment approvals
The cascade delete constraints are correctly designed. Approvals are tightly coupled to their environments and releases, and keeping orphaned approval records would create data inconsistency. The design also preserves audit information by setting user references to null rather than deleting them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing approvals that could be affected by cascade delete. # Check for approvals with non-existent environments rg -A 5 'SELECT.*FROM.*environment_approval.*WHERE.*environment_id.*NOT.*IN.*SELECT.*id.*FROM.*environment' # Check for approvals with non-existent releases rg -A 5 'SELECT.*FROM.*environment_approval.*WHERE.*release_id.*NOT.*IN.*SELECT.*id.*FROM.*release'Length of output: 211
Script:
#!/bin/bash # Check schema definitions and related tables fd -e sql . packages/db/drizzle/ -x cat {} # Look for delete operations and transaction handling rg -t ts -t js "delete.*environment" -B 2 -A 2 rg -t ts -t js "delete.*release" -B 2 -A 2 rg -t ts -t js "transaction" -B 2 -A 2 # Look for environment approval related business logic ast-grep --pattern 'class $_ { $$$ delete$_($$$) { $$$ } $$$ }'Length of output: 75449
packages/api/src/router/environment.ts (1)
43-43: LGTM!The addition of the approval router to the environment router is clean and follows the existing pattern.
Also applies to: 53-53
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (2)
36-43: LGTM! Props type update aligns with environment-centric approach.The renaming of
ReleaseSequencingNodePropstoEnvironmentNodePropsand the addition ofenvironmentNameprop improves clarity and reusability.
316-334: Verify ApprovalCheck integration.The component has been updated to use the new environment-centric UI elements and ApprovalCheck component. Please ensure that the ApprovalCheck component correctly handles the environment-based approval logic.
Run this script to verify the ApprovalCheck usage:
✅ Verification successful
ApprovalCheck component verified for environment-based logic
The component correctly implements environment-based approval handling with proper API integration, state management, and UI feedback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ApprovalCheck component integration. # Check if ApprovalCheck is consistently used with environment props ast-grep --pattern $'ApprovalCheck {...data}' # Check for any remaining policy-based approval checks rg -A 5 'policy.*approval'Length of output: 91187
Script:
#!/bin/bash # Search for ApprovalCheck component definition ast-grep --pattern 'const ApprovalCheck = $_' # Search for environment approval related code in the component rg -A 5 'ApprovalCheck.*environment'Length of output: 86
Script:
#!/bin/bash # Try different component patterns ast-grep --pattern 'function ApprovalCheck' # Find files containing ApprovalCheck fd ApprovalCheck # Search for approval-related logic rg -A 5 'approval.*environment|environment.*approval'Length of output: 105588
Script:
#!/bin/bash # Get ApprovalCheck component implementation cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx" # Search for environment-related props or types rg -A 5 "interface.*Props.*environment|type.*Props.*environment" "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx"Length of output: 3228
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (1)
65-71: LGTM! Approval query and filtering updated correctly.The changes to use
approval.byReleaseIdand filter byenvironmentIdalign with the new environment-based approval system.packages/db/drizzle/meta/_journal.json (1)
368-374: LGTM! Migration entry follows the established pattern.The new migration entry is properly formatted and maintains consistency with the existing journal structure.
packages/db/src/schema/environment.ts (3)
233-246: Schema changes align with environment-based approval model.The table definition correctly implements the transition from policy-based to environment-based approvals:
- Foreign key relationship properly references the environment table
- Cascade deletion ensures referential integrity
- Status and user tracking fields maintained
249-249: Type definition updated to reflect schema changes.The renamed type
EnvironmentApprovalcorrectly reflects the schema changes and maintains type safety throughout the application.
246-246: Verify the impact of unique constraint change.The unique constraint has been updated to enforce one approval per environment-release pair. Please ensure that existing approval records are compatible with this constraint before applying the migration.
✅ Verification successful
The unique constraint change is safe to apply
The constraint change is part of a larger refactoring where
environment_policy_approvaltable is renamed toenvironment_approvalandpolicy_idis renamed toenvironment_id. The semantic meaning of the unique constraint remains the same - ensuring one approval per environment-release pair. The migration handles all necessary structural changes properly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential duplicate approvals that might violate the new constraint echo "Checking for potential constraint violations..." ast-grep --pattern 'INSERT INTO environment_policy_approval' rg -A 2 'environment_policy_approval.*INSERT'Length of output: 215
Script:
#!/bin/bash # Search for migration files and table definitions echo "Searching for migration files and table definitions..." fd -e sql -e ts migration rg -l "environment_policy_approval" rg -A 5 "CREATE TABLE.*environment_policy_approval" rg -A 5 "ALTER TABLE.*environment_policy_approval" # Check for any existing indexes or constraints echo "Checking existing constraints..." rg -A 3 "INDEX.*environment_policy_approval" rg -A 3 "CONSTRAINT.*environment_policy_approval" # Look for any data manipulation patterns echo "Checking data manipulation patterns..." rg -A 3 "environment_policy_approval.*WHERE"Length of output: 10631
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
16-18: LGTM! Updated approval schema to use environment-based approvals.The changes correctly implement the transition from policy-based to environment-based approvals in the database queries and types.
Also applies to: 20-31
| const updateResult = await tx.execute( | ||
| sql`UPDATE job | ||
| SET status = 'cancelled' | ||
| FROM release_job_trigger rjt | ||
| WHERE job.status = 'pending' | ||
| AND rjt.job_id = job.id | ||
| AND rjt.release_id = ${input.releaseId} | ||
| AND rjt.environment_id = ${input.environmentId}`, |
There was a problem hiding this comment.
Verify parameterization in the SQL update statement to prevent SQL injection.
In the reject mutation, the raw SQL query uses template literals with ${input.releaseId} and ${input.environmentId}. Please ensure that these inputs are properly parameterized by the database library to prevent SQL injection vulnerabilities.
As a follow-up, consider using parameterized query methods provided by your database library. For example:
await tx.execute(
sql`UPDATE job
SET status = 'cancelled'
FROM release_job_trigger rjt
WHERE job.status = 'pending'
AND rjt.job_id = job.id
AND rjt.release_id = ${input.releaseId}
AND rjt.environment_id = ${input.environmentId}`,
);Ensure that the sql tagged template handles parameterization safely, or refactor to use query builder methods that automatically parameterize inputs.
There was a problem hiding this comment.
drizzle's sql operator guards against injection
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification. I understand now that Drizzle's sql tagged template provides built-in protection against SQL injection.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#279
File: packages/api/src/router/environment-approval.ts:139-146
Timestamp: 2025-01-10T00:25:43.590Z
Learning: Drizzle's `sql` tagged template literal operator automatically guards against SQL injection when interpolating parameters, making it safe to use template literals within sql`...` strings.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| ALTER TABLE "environment_policy_approval" RENAME TO "environment_approval";--> statement-breakpoint | ||
| ALTER TABLE "environment_approval" RENAME COLUMN "policy_id" TO "environment_id";--> statement-breakpoint |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a down migration for table and column rename.
The migration is non-reversible. Consider adding a down migration to handle rollback scenarios.
-- Down migration
ALTER TABLE "environment_approval" RENAME TO "environment_policy_approval";
ALTER TABLE "environment_policy_approval" RENAME COLUMN "environment_id" TO "policy_id";| import { api } from "~/trpc/react"; | ||
| import { JobDropdownMenu } from "./JobDropdownMenu"; | ||
| import { PolicyApprovalRow } from "./PolicyApprovalRow"; | ||
| import { EnvironmentApprovalRow } from "./PolicyApprovalRow"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import path to match component name.
The component is imported as EnvironmentApprovalRow but from a file named PolicyApprovalRow. This suggests the file hasn't been renamed to match the new naming convention.
Rename the file to match the exported component:
-import { EnvironmentApprovalRow } from "./PolicyApprovalRow";
+import { EnvironmentApprovalRow } from "./EnvironmentApprovalRow";Committable suggestion skipped: line range outside the PR's diff.
| }; | ||
|
|
||
| const policyId = je.environment?.policyId; | ||
| const environmentId = je.environment?.id; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for environmentId.
While the optional chaining is used, consider adding an early return or error response when environmentId is undefined.
const environmentId = je.environment?.id;
+if (je.release?.id && !environmentId) {
+ return NextResponse.json(
+ { error: "Environment not found for job." },
+ { status: 404 }
+ );
+}
const approval =
je.release?.id && environmentId
? await getApprovalDetails(je.release.id, environmentId)
: undefined;Also applies to: 129-130
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (2)
Line range hint
27-32: Consider enhancing error handling for undefined environment.While the error is logged, consider providing a more user-friendly fallback UI instead of returning null.
if (!environment) { console.error("Environment is undefined for approval:", approval); - return null; + return ( + <div className="text-sm text-red-400"> + Error: Environment configuration missing + </div> + ); }
Line range hint
35-45: Verify error messages in toast notifications.The error messages "Error rejecting release" and "Error approving release" are quite generic. Consider including more specific error details from the API response.
- onError: () => toast.error("Error rejecting release"), + onError: (error) => toast.error(`Failed to reject release: ${error.message}`),- onError: () => toast.error("Error approving release"), + onError: (error) => toast.error(`Failed to approve release: ${error.message}`),Also applies to: 47-55
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (2)
Line range hint
226-315: Consider optimizing the interval cleanup in MinReleaseIntervalCheck.While the implementation is functional, there are a few improvements that could be made:
- Initialize timeLeft with the calculated value
- Ensure interval cleanup on component unmount even if timeLeft > 0
Consider this optimization:
const MinReleaseIntervalCheck: React.FC<EnvironmentNodeProps["data"]> = ({ policy, deploymentId, environmentId, }) => { - const [timeLeft, setTimeLeft] = useState<number | null>(null); + const [timeLeft, setTimeLeft] = useState(() => { + if (!latestRelease || !policy?.minimumReleaseInterval) return null; + return Math.max(0, policy.minimumReleaseInterval - differenceInMilliseconds( + new Date(), + latestRelease.createdAt, + )); + }); useEffect(() => { if (!latestRelease || !policy?.minimumReleaseInterval) return; + let interval: NodeJS.Timeout; const calculateTimeLeft = () => { const timePassed = differenceInMilliseconds( new Date(), latestRelease.createdAt, ); return Math.max(0, policy.minimumReleaseInterval - timePassed); }; setTimeLeft(calculateTimeLeft()); - const interval = setInterval(() => { + interval = setInterval(() => { const remaining = calculateTimeLeft(); setTimeLeft(remaining); if (remaining <= 0) clearInterval(interval); }, 1000); - return () => clearInterval(interval); + return () => { + if (interval) clearInterval(interval); + }; }, [latestRelease, policy?.minimumReleaseInterval]);
316-333: Consider enhancing accessibility for the environment node.While the visual structure is well-organized, consider adding ARIA attributes to improve accessibility:
export const EnvironmentNode: React.FC<EnvironmentNodeProps> = ({ data }) => ( <> <div - className={cn("relative w-[250px] space-y-1 rounded-md border text-sm")} + className={cn("relative w-[250px] space-y-1 rounded-md border text-sm")} + role="region" + aria-label={`Environment: ${data.environmentName}`} > <div className="flex items-center gap-2 p-2"> <div className="flex-shrink-0 rounded bg-green-500/20 p-1 text-green-400"> - <IconPlant className="h-3 w-3" /> + <IconPlant className="h-3 w-3" aria-hidden="true" /> </div> {data.environmentName} </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (1)
Pattern **/*.{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.
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (1)
15-17: LGTM! Type definitions are clear and consistent.The type definitions accurately reflect the shift from policy-based to environment-based approvals.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (3)
38-38: LGTM! Import path is correctly updated.The import statement now correctly reflects the new component name, resolving the previous review comment.
65-72: LGTM! API endpoint and filtering logic are consistent.The changes to the API endpoint and filtering logic correctly implement the transition from policy-based to environment-based approvals.
Line range hint
145-149: LGTM! Component usage is correct.The EnvironmentApprovalRow component is correctly integrated with appropriate props.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (2)
36-44: LGTM! Type definition changes align with the component's purpose.The renaming from
ReleaseSequencingNodePropstoEnvironmentNodePropsand the addition of theenvironmentNameproperty better reflect the component's responsibility of representing environment nodes.
332-332: Verify integration with the new approval system.The ApprovalCheck component is properly integrated, but let's verify its usage across the codebase to ensure consistent implementation of the per-environment approval system.
✅ Verification successful
Approval system integration verified successfully
The ApprovalCheck component is properly integrated with the per-environment approval system, using consistent API calls and proper state management. The implementation includes proper error handling, loading states, and user feedback mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of ApprovalCheck component and related approval system changes # Check for any remaining references to the old approval system rg -l "PolicyApprovalRow|ReleaseSequencingNode" --type ts --type tsx # Verify ApprovalCheck component usage rg -l "ApprovalCheck" --type ts --type tsx # Check for any environment policy related components that might need updates ast-grep --pattern 'import { $_ } from "$_/environment-policy-$_"'Length of output: 238
Script:
#!/bin/bash # Description: Verify approval system integration with corrected syntax # Check for any remaining references to old approval system rg "PolicyApprovalRow|ReleaseSequencingNode" -t ts -t tsx -C 2 # Verify ApprovalCheck component usage rg "ApprovalCheck" -t ts -t tsx -C 2 # Look for environment policy related files fd -e ts -e tsx | grep -i "policy\|approval" # Check imports related to approvals rg "import.*Approval" -t ts -t tsxLength of output: 2034
Script:
#!/bin/bash # Check the content of approval-related components cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx" cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx" # Look for direct usage of these components rg -g "!*.{test,spec}.*" "import.*{.*ApprovalCheck.*}" . rg -g "!*.{test,spec}.*" "<ApprovalCheck" .Length of output: 6182
Summary by CodeRabbit
Release Notes
New Features
Changes
Improvements
Bug Fixes