fix: Gradual rollout starts at approval date if approval gate on environment#292
fix: Gradual rollout starts at approval date if approval gate on environment#292adityachoudhari26 merged 6 commits intomainfrom
Conversation
WalkthroughThis pull request introduces enhancements to the environment policy approval process by adding an Changes
Sequence DiagramsequenceDiagram
participant User
participant ApprovalSystem
participant JobDispatch
participant Database
User->>ApprovalSystem: Request Release
ApprovalSystem->>Database: Check Approval Policy
Database-->>ApprovalSystem: Return Policy Details
ApprovalSystem->>ApprovalSystem: Calculate Rollout Window
ApprovalSystem->>Database: Record Approval
Database->>Database: Set approvedAt Timestamp
JobDispatch->>Database: Check Approval Status
JobDispatch->>JobDispatch: Determine Job Trigger Eligibility
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/job-dispatch/src/policies/gradual-rollout.ts (1)
21-25: Validate distribution approach inisReleaseJobTriggerInRolloutWindow.
The condition(murmurhash.v3(session, 11) % 100) < timeWindowPercent(...)effectively gates rollouts. Confirm that the 0–99 range is broad enough to achieve the desired distribution granularity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/src/schema/environment.ts(1 hunks)packages/job-dispatch/src/gradual-rollout.ts(0 hunks)packages/job-dispatch/src/policies/gradual-rollout.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/job-dispatch/src/gradual-rollout.ts
🧰 Additional context used
📓 Path-based instructions (2)
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.
packages/job-dispatch/src/policies/gradual-rollout.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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (8)
packages/job-dispatch/src/policies/gradual-rollout.ts (7)
1-3: Ensure hashing collisions are acceptable.
Usingmurmurhashis a practical choice. However, confirm that potential collisions—and thus unanticipated rollout overlaps—are acceptable for your use case.
4-8: Imports look fine.
No concerns here; the imports from@ctrlplane/dband schema references are consistent with the rest of the file.
9-19: Double-check edge cases intimeWindowPercent.
The logic comprehensively covers zero or negative durations, future start dates, and durations already passed. Ensure test coverage for each condition, including very large durations that might yield floating-point edge cases.
27-34: Confirm partial rollout date calculations.
getRolloutDateForReleaseJobTriggeroffsetsstartDateby a fraction ofdurationbased on the hash. Consider clamping or handling extremely large durations, or negative durations if that’s possible upstream.
66-75: Validate join performance.
Using.leftJoinonenvironmentPolicyApprovalis appropriate for optional approvals. Confirm that indexes exist on(policyId, releaseId)to maintain efficient queries at scale.
86-96: Automatic approval logic is consistent.
WhenapprovalRequirementis"automatic", usingp.release.createdAtfor rollout timing aligns with the new policy. Ensure the code handles short-living releases (whererolloutDurationis small).
97-106: Manual approval logic is synchronized withapprovedAt.
This conditional rightly checksstatus === "approved" && approvedAt != nullbefore applying the rollout window. Ensure that concurrency or unexpected delays in updatingapprovedAtdo not mismatch real-time rollout triggers.packages/db/src/schema/environment.ts (1)
244-247: NewapprovedAttimestamp field is appropriate.
Storing the approval timestamp is beneficial for auditing. The defaultNULLis consistent for unapproved rows. Confirm that time zone handling aligns with the rest of the schema.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx (1)
53-68: LGTM with a suggestion for error handling.The checkbox implementation is clean, accessible, and follows React best practices. The state management and event handling are well-structured.
Consider adding error handling to the Promise chain:
onCheckedChange={(value) => { updatePolicy .mutateAsync({ id, data: { approvalRequirement: value ? "manual" : "automatic" }, }) - .then(invalidatePolicy); + .then(invalidatePolicy) + .catch((error) => { + // Handle or report error + console.error('Failed to update approval requirement:', error); + }); }}apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (4)
61-83: Consider adding error handling for the policy query.The loading state is handled well, but there's no handling for potential query errors. Consider adding error feedback to improve user experience.
{policyQ.isLoading && ( <AlertDialogDescription className="flex items-center justify-center"> <IconLoader2 className="animate-spin" /> </AlertDialogDescription> )} + {policyQ.error && ( + <AlertDialogDescription className="text-red-500"> + Failed to load environments. Please try again. + </AlertDialogDescription> + )} {!policyQ.isLoading && (
32-37: Consider moving invalidation logic to a custom hook.The invalidation logic could be reused across components. Consider extracting it to a custom hook:
+// Create a new hook: useApprovalInvalidation.ts +export const useApprovalInvalidation = (policyId: string, releaseId: string) => { + const utils = api.useUtils(); + return () => utils.environment.policy.approval.statusByReleasePolicyId.invalidate({ + policyId, + releaseId, + }); +}; // In ApprovalCheck.tsx - const utils = api.useUtils(); - const invalidateApproval = () => - utils.environment.policy.approval.statusByReleasePolicyId.invalidate({ - policyId, - releaseId: release.id, - }); + const invalidateApproval = useApprovalInvalidation(policyId, release.id);
61-65: Consider using a shared loading spinner component.The loading spinner could be reused across the application. Consider extracting it to a shared component:
+// Create a new component: LoadingSpinner.tsx +export const LoadingSpinner: React.FC = () => ( + <IconLoader2 className="animate-spin" /> +); // In ApprovalCheck.tsx - <AlertDialogDescription className="flex items-center justify-center"> - <IconLoader2 className="animate-spin" /> - </AlertDialogDescription> + <AlertDialogDescription className="flex items-center justify-center"> + <LoadingSpinner /> + </AlertDialogDescription>
71-79: Consider extracting environment badge to a reusable component.The environment badge could be reused across the application. Consider extracting it to a shared component:
+// Create a new component: EnvironmentBadge.tsx +export const EnvironmentBadge: React.FC<{ name: string }> = ({ name }) => ( + <Badge variant="secondary" className="max-w-40"> + <span className="truncate">{name}</span> + </Badge> +); // In ApprovalCheck.tsx - {policyQ.data?.environments.map((env) => ( - <Badge - key={env.id} - variant="secondary" - className="max-w-40" - > - <span className="truncate">{env.name}</span> - </Badge> - ))} + {policyQ.data?.environments.map((env) => ( + <EnvironmentBadge key={env.id} name={env.name} /> + ))}apps/webservice/src/app/api/v1/releases/[releaseId]/openapi.ts (1)
34-37: Consider defining a more specific schema for jobAgentConfig.The current schema allows any object structure. Consider defining specific properties and their types to ensure data consistency and provide better documentation.
jobAgentConfig: { type: "object", - additionalProperties: true, + properties: { + // Define specific properties here + // Example: + // timeout: { type: "number" }, + // retries: { type: "number" }, + }, + additionalProperties: false, },openapi.v1.json (2)
2581-2584: Add property description for better API documentation.The
jobAgentConfigproperty lacks a description explaining its purpose and expected content.Add a description to improve API documentation:
"jobAgentConfig": { "type": "object", + "description": "Configuration settings for the job agent that will process this release", "additionalProperties": true },
2581-2584: Consider adding schema validation for jobAgentConfig properties.The
jobAgentConfigis defined withadditionalProperties: truewithout any property constraints. This could lead to inconsistent data and make it harder to maintain backward compatibility.Consider defining a more specific schema:
"jobAgentConfig": { "type": "object", + "properties": { + "type": { + "type": "string", + "description": "Type of job agent to use" + }, + "version": { + "type": "string", + "description": "Version of job agent configuration schema" + }, + "settings": { + "type": "object", + "description": "Job agent specific settings", + "additionalProperties": true + } + }, + "required": ["type", "version"], "additionalProperties": true },Also applies to: 1218-1221, 1286-1289
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (2)
95-97: Consider combining loading states and conditions for better maintainability.The loading states and conditions are spread across multiple lines. Consider combining them:
- const isLoading = - isWorkspaceLoading || - isStatusesLoading || - isResourcesLoading || - isBlockedEnvsLoading || - isApprovalLoading; - - if (isLoading) - return <p className="text-xs text-muted-foreground">Loading...</p>; - - const hasResources = total > 0; - const isAlreadyDeployed = statuses != null && statuses.length > 0; - - const hasJobAgent = deployment.jobAgentId != null; - const isBlockedByReleaseChannel = blockedEnv != null; - - const isPendingApproval = approval?.status === "pending"; - - const showBlockedByReleaseChannel = - isBlockedByReleaseChannel && - !statuses?.some((s) => s.job.status === JobStatus.InProgress); - - const showRelease = - isAlreadyDeployed && !showBlockedByReleaseChannel && !isPendingApproval; + const states = { + isLoading: isWorkspaceLoading || isStatusesLoading || isResourcesLoading || + isBlockedEnvsLoading || isApprovalLoading, + hasResources: total > 0, + isAlreadyDeployed: statuses != null && statuses.length > 0, + hasJobAgent: deployment.jobAgentId != null, + isBlockedByReleaseChannel: blockedEnv != null, + isPendingApproval: approval?.status === "pending", + }; + + if (states.isLoading) + return <p className="text-xs text-muted-foreground">Loading...</p>; + + const showBlockedByReleaseChannel = + states.isBlockedByReleaseChannel && + !statuses?.some((s) => s.job.status === JobStatus.InProgress); + + const showRelease = + states.isAlreadyDeployed && !showBlockedByReleaseChannel && !states.isPendingApproval;Also applies to: 107-108, 113-114
174-185: Consider extracting button styles to a shared component.The button styling could be reused across the application. Consider extracting it to a shared component:
+// Create a new component: PendingApprovalButton.tsx +export const PendingApprovalButton: React.FC<ButtonProps> = (props) => ( + <Button + className="w-full border-dashed border-neutral-800/50 bg-transparent text-center + text-neutral-800 hover:border-blue-400 hover:bg-transparent hover:text-blue-400" + variant="outline" + size="sm" + {...props} + /> +); // In ReleaseEnvironmentCell.tsx - <Button - className="w-full border-dashed border-neutral-800/50 bg-transparent text-center - text-neutral-800 hover:border-blue-400 hover:bg-transparent hover:text-blue-400" - variant="outline" - size="sm" - > - Pending approval - </Button> + <PendingApprovalButton> + Pending approval + </PendingApprovalButton>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx(0 hunks)apps/webservice/src/app/api/v1/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/releases/[releaseId]/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/releases/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/releases/route.ts(2 hunks)openapi.v1.json(6 hunks)
💤 Files with no reviewable changes (2)
- 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]/ResourceReleaseTable.tsx
🧰 Additional context used
📓 Path-based instructions (9)
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.
apps/webservice/src/app/api/v1/openapi.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/environment-policy-drawer/ApprovalAndGovernance.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/releases/openapi.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/api/v1/releases/[releaseId]/openapi.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/api/v1/releases/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)/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.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.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]/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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (15)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx (1)
5-5: LGTM!The Checkbox import is correctly placed and follows the project's import structure.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (1)
3-3: LGTM! Clean removal of unused environment dependencies.The removal of the
linkedEnvironmentsprop and its related type import simplifies the component's interface while maintaining its core functionality.Also applies to: 20-20
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (2)
132-132: LGTM! Clean removal of unused environment dependencies.The removal of the
linkedEnvironmentsprop fromApprovalCheckusage simplifies the component's interface while maintaining its core functionality.
132-132: LGTM! Prop removal is consistent with the refactoring.The removal of the
linkedEnvironmentsprop fromApprovalCheckaligns with the broader changes to simplify the approval workflow.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (2)
21-21: LGTM! Well-structured approval status integration.The changes effectively integrate approval status handling with:
- Proper loading state management
- Clear user feedback for pending approvals
- Consistent UI state transitions
Also applies to: 52-56, 95-97, 107-108, 113-114, 174-186, 187-187
52-56: LGTM! Well-structured query with proper type safety.The approval status query is correctly configured with:
- Proper type safety through the query parameters
- Appropriate enabling condition based on policy existence
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (1)
3-3: LGTM! Well-structured approval dialog enhancements.The changes effectively:
- Add proper loading states with visual feedback
- Ensure cache consistency through invalidation
- Improve the user interface with conditional rendering
Also applies to: 26-28, 32-37, 43-43, 49-49, 61-83, 85-90, 99-99, 142-142
apps/webservice/src/app/api/v1/releases/[releaseId]/openapi.ts (1)
57-57: LGTM! Good use of schema reference.The change promotes schema reuse and maintains consistency across the API specification.
apps/webservice/src/app/api/v1/releases/openapi.ts (2)
28-31: Schema specificity concern.Same concern as in
[releaseId]/openapi.tsabout defining a more specific schema forjobAgentConfig.
52-52: LGTM! Consistent schema reference.Same approval as in
[releaseId]/openapi.tsfor using schema reference.apps/webservice/src/app/api/v1/releases/route.ts (2)
75-75: LGTM! Consistent conflict update handling.The
jobAgentConfigfield is correctly added to the list of updatable columns.
120-125: LGTM! Improved code formatting.The reformatting improves readability while maintaining the same functionality.
apps/webservice/src/app/api/v1/openapi.ts (2)
64-64: Schema specificity concern.Same concern as in other files about defining a more specific schema for
jobAgentConfig.
76-76: Verify impact of making jobAgentConfig required.Adding
jobAgentConfigto required fields is a breaking change. Please verify that:
- All existing releases have this field.
- All creation/update flows are updated to include this field.
openapi.v1.json (1)
2581-2584: Verify the impact of makingjobAgentConfigrequired in the Release schema.The
jobAgentConfigproperty has been added to the Release schema and marked as required. This is a breaking change that could affect existing clients.Run the following script to check for existing releases that might be affected:
Also applies to: 2604-2605
| "jobAgentConfig": { | ||
| "type": "object", | ||
| "additionalProperties": true | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent schema usage across PATCH and POST endpoints.
The jobAgentConfig property has been added to both PATCH and POST request bodies for releases. However, there's an inconsistency:
- POST
/v1/releasesrequiresjobAgentConfig(via Release schema) - PATCH
/v1/releases/{releaseId}makes it optional
This inconsistency could lead to confusion. Consider making it consistently required or optional across both endpoints.
Also applies to: 1286-1289
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/job-dispatch/src/policies/gradual-rollout.ts (2)
22-26: Consider extracting the magic number and improving documentation.The hash seed (11) appears to be a magic number. Consider extracting it to a named constant and documenting why this specific value was chosen.
+const HASH_SEED = 11; // Document why this specific seed was chosen + export const isReleaseJobTriggerInRolloutWindow = ( session: string, startDate: Date, duration: number, -) => murmurhash.v3(session, 11) % 100 < timeWindowPercent(startDate, duration); +) => murmurhash.v3(session, HASH_SEED) % 100 < timeWindowPercent(startDate, duration);
52-59: Consider improving the readability of the date calculation.The date calculation logic is complex and could benefit from being broken down into more readable steps.
export const getRolloutDateForReleaseJobTrigger = ( session: string, startDate: Date, duration: number, -) => - new Date( - startDate.getTime() + ((duration * murmurhash.v3(session, 11)) % 100) / 100, - ); +) => { + const hashValue = murmurhash.v3(session, HASH_SEED); + const percentageOffset = (hashValue % 100) / 100; + const timeOffset = duration * percentageOffset; + return new Date(startDate.getTime() + timeOffset); +};apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)
77-81: Consider adding error handling for the query.While the current implementation handles loading state well, it would be beneficial to handle query errors explicitly.
Consider adding error handling:
- const { data: approvalStatus, isLoading } = + const { data: approvalStatus, isLoading, error } = api.environment.policy.approval.statusByReleasePolicyId.useQuery( { policyId: data.id, releaseId: data.release.id }, { enabled: data.approvalRequirement === "manual" }, ); + + if (error) { + return ( + <div className="flex items-center gap-2 text-destructive"> + <Cancelled /> Error loading approval status + </div> + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx(3 hunks)packages/api/src/router/environment-approval.ts(1 hunks)packages/db/drizzle/0064_misty_stryfe.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/job-dispatch/src/index.ts(0 hunks)packages/job-dispatch/src/policies/gradual-rollout.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/job-dispatch/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0064_misty_stryfe.sql
🧰 Additional context used
📓 Path-based instructions (3)
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/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/job-dispatch/src/policies/gradual-rollout.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.
🔇 Additional comments (10)
packages/db/drizzle/meta/_journal.json (1)
452-458: LGTM! Verify the associated migration file.The new migration entry follows the established format and maintains sequential ordering.
Let's verify the existence and content of the associated migration file:
✅ Verification successful
Migration file verified successfully
The migration file
0064_misty_stryfe.sqlexists and contains the expectedapproved_atcolumn addition to theenvironment_policy_approvaltable with appropriate timestamp type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the migration file # Check if the migration file exists ls -l packages/db/drizzle/0064_misty_stryfe.sql # If it exists, display its content to verify the approved_at column addition cat packages/db/drizzle/0064_misty_stryfe.sqlLength of output: 297
packages/job-dispatch/src/policies/gradual-rollout.ts (3)
1-9: LGTM! Good choice on the dependency changes.The replacement of lodash with murmurhash is a good optimization, as it provides a more focused and lightweight solution for hash generation.
28-50: LGTM! Well-structured approval status handling.The function properly handles both automatic and manual approval scenarios, with appropriate null handling for invalid states.
98-125: 🛠️ Refactor suggestionConsider adding error handling for database operations.
While the Promise.all usage is correct, consider adding error handling for potential database operation failures.
return Promise.all( policies.map(async (p) => { + try { const { release_job_trigger, environment_policy, release } = p; if ( environment_policy == null || environment_policy.rolloutDuration === 0 ) return release_job_trigger; const rolloutStart = await getRolloutStart( db, release, environment_policy, ); if (rolloutStart == null) return null; if ( isReleaseJobTriggerInRolloutWindow( release_job_trigger.resourceId, rolloutStart, environment_policy.rolloutDuration, ) ) return release_job_trigger; return null; + } catch (error) { + console.error('Failed to process policy:', error); + return null; + } }), ).then((results) => results.filter(isPresent));Likely invalid or redundant comment.
packages/api/src/router/environment-approval.ts (2)
74-74: LGTM! Good variable extraction.Extracting
userIdimproves code readability and reusability.
77-77: LGTM! Good timestamp tracking implementation.Adding
approvedAttimestamp enhances audit capabilities by tracking when approvals occur.Let's verify the database schema supports this new field:
✅ Verification successful
Schema verification successful:
approvedAttimestamp is properly supportedThe database schema includes the
approved_attimestamp column in theenvironment_policy_approvaltable with the correct type (timestamp with time zone), making it fully compatible with the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify environment_policy_approval table schema includes approvedAt field rg -A 5 "environment_policy_approval.*approvedAt"Length of output: 51
Script:
#!/bin/bash # Find SQL files and search for environment-related tables echo "=== Looking for SQL files with environment tables ===" fd -e sql -x cat {} | grep -i "environment" -A 10 echo -e "\n=== Looking for migration files ===" fd -e ts migration | xargs cat | grep -i "environment" -A 10Length of output: 25019
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (4)
18-18: LGTM! Good icon organization.Importing additional status icons enhances UI feedback capabilities.
104-104: LGTM! Proper dependency update.Correctly updated useEffect dependency to use startDate instead of data.release.createdAt.
108-133: LGTM! Comprehensive UI state handling.Good implementation of loading and approval states with appropriate visual feedback.
The implementation:
- Shows loading state while fetching approval status
- Handles pending approval state
- Shows rejection feedback
- Uses appropriate icons for each state
77-86: LGTM! Well-structured approval status handling.Good implementation of conditional query and startDate calculation based on approval requirement.
Let's verify the API endpoint exists and returns the expected data:
✅ Verification successful
API endpoint verification successful
The
statusByReleasePolicyIdendpoint is properly implemented with correct input parameters, authorization checks, and returns the expected approval status data including theapprovedAtfield used in the frontend.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the API endpoint implementation ast-grep --pattern 'statusByReleasePolicyId = protectedProcedure .input($$$) .query($$$)'Length of output: 95
Script:
#!/bin/bash # Find potential router files fd -e ts -e tsx router fd -e ts -e tsx approvalLength of output: 542
Script:
#!/bin/bash # Check the router implementation cat packages/api/src/router/environment-approval.ts # Backup search for the endpoint name rg "statusByReleasePolicyId" -A 10Length of output: 13955
Summary by CodeRabbit
Release Notes
New Features
Database Changes
approved_atcolumn toenvironment_policy_approvaltableImprovements
Code Cleanup