fix: Minimum release interval policy #272
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
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
|
![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 (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (2)
68-76: Consider adding error handling for invalid duration valuesWhile the initialization looks good, consider adding error handling for cases where
environmentPolicy.minimumReleaseIntervalmight contain invalid values. This would prevent the UI from breaking if the backend returns unexpected values.- const minimumReleaseInterval = prettyMilliseconds( - environmentPolicy.minimumReleaseInterval, - ); + const minimumReleaseInterval = environmentPolicy.minimumReleaseInterval + ? prettyMilliseconds(environmentPolicy.minimumReleaseInterval) + : "0ms";
Line range hint
251-279: Consider adding examples of valid duration formatsThe UI implementation looks good, but consider enhancing the form description to include examples of valid duration formats (e.g., "1d", "12h", "30m"). This would help users understand the expected input format.
<FormDescription> Setting a minimum release interval will ensure that a certain - amount of time has passed since the last active release. + amount of time has passed since the last active release. + Valid formats include: 1d (1 day), 12h (12 hours), 30m (30 minutes). </FormDescription>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx(4 hunks)packages/db/drizzle/0048_lucky_eddie_brock.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment.ts(1 hunks)packages/job-dispatch/src/policies/min-release-interval-policy.ts(1 hunks)packages/job-dispatch/src/policy-checker.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0048_lucky_eddie_brock.sql
🧰 Additional context used
📓 Path-based instructions (4)
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/policy-checker.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/min-release-interval-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.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.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 (10)
packages/job-dispatch/src/policies/min-release-interval-policy.ts (3)
21-31: Ensure sub-query performance & clarity
The creation of a sub-query to fetch the latest completed releases is well-structured. Consider whether an index on release/environment columns might further optimize frequent queries.
Would you like a script to check if these columns already have appropriate indexes?
86-90: Short-circuit return is neat
Returning early if there are no job triggers is a clean approach that prevents unnecessary queries and logic.
166-172: Double-check interval comparison
Using date-fns’ differenceInMilliseconds is effective. Confirm that the environment’s minimumReleaseInterval and the computed difference are both in milliseconds, avoiding any potential unit mismatch.
packages/job-dispatch/src/policy-checker.ts (2)
10-10: Thanks for adding new policy import
The import statement for isPassingMinReleaseIntervalPolicy is consistent with the existing naming convention.
18-27: Solid addition to baseChecks
Including the new isPassingMinReleaseIntervalPolicy in baseChecks ensures consistent enforcement of the release interval for all relevant checks.
packages/db/drizzle/meta/_journal.json (1)
342-346: Migration entry is consistent
Adding this entry follows the established incremented index pattern for migrations. The "breakpoints": true field matches existing entries.
packages/db/src/schema/environment.ts (2)
133-134: Good documentation
Including a code comment clarifies the new column’s purpose as the minimum release interval in milliseconds.
137-138: Safe default
Defaulting to 0 ensures backward compatibility, preventing unforeseen null or undefined issues for existing entries.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (2)
51-53: LGTM: Schema validation for minimumReleaseInterval
The validation is consistent with existing duration fields and properly integrated into the schema.
235-249: LGTM: UI improvements for rollout duration
The changes improve clarity and provide better user guidance with the placeholder. The styling is consistent with the design system.
| const minimumReleaseInterval = ms(data.minimumReleaseInterval); | ||
| const updates = { rolloutDuration, releaseWindows, minimumReleaseInterval }; | ||
| updatePolicy | ||
| .mutateAsync({ id: policyId, data: { rolloutDuration, releaseWindows } }) | ||
| .mutateAsync({ id: policyId, data: updates }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for non-negative duration
While the form submission logic is correct, consider adding validation to ensure that minimumReleaseInterval is non-negative. This would prevent potential issues with negative durations.
- const minimumReleaseInterval = ms(data.minimumReleaseInterval);
+ const minimumReleaseInterval = ms(data.minimumReleaseInterval);
+ if (minimumReleaseInterval < 0) {
+ throw new Error("Minimum release interval cannot be negative");
+ }📝 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 minimumReleaseInterval = ms(data.minimumReleaseInterval); | |
| const updates = { rolloutDuration, releaseWindows, minimumReleaseInterval }; | |
| updatePolicy | |
| .mutateAsync({ id: policyId, data: { rolloutDuration, releaseWindows } }) | |
| .mutateAsync({ id: policyId, data: updates }) | |
| const minimumReleaseInterval = ms(data.minimumReleaseInterval); | |
| if (minimumReleaseInterval < 0) { | |
| throw new Error("Minimum release interval cannot be negative"); | |
| } | |
| const updates = { rolloutDuration, releaseWindows, minimumReleaseInterval }; | |
| updatePolicy | |
| .mutateAsync({ id: policyId, data: updates }) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (1)
Line range hint
258-285: Consider adding duration format help text.While the implementation is solid, consider adding a tooltip or help text to explain the accepted duration formats (e.g., "1d", "2h", "30m", etc.) to improve user experience.
<FormDescription> Setting a minimum release interval will ensure that a certain - amount of time has passed since the last active release. + amount of time has passed since the last active release. + Accepted formats: 1d (1 day), 2h (2 hours), 30m (30 minutes), etc. </FormDescription>apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (1)
250-314: Interval-based release check implemented well.
This component calculates elapsed time since the latest completed release, then uses setInterval to update remaining time. The cleanup function in the effect hook prevents memory leaks. Overall logic is correct.As a small enhancement, consider debouncing or using a more event-driven approach if updates need not be second-by-second. This reduces updates when the user can tolerate coarser refresh intervals.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx(6 hunks)packages/api/src/router/release.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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]/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)/_components/environment-policy-drawer/RolloutAndTiming.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/release.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 (16)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (5)
37-44: LGTM! Well-structured duration validation.
The isValidDuration function is well-implemented with proper error handling and validation for non-negative durations.
58-60: LGTM! Consistent schema definition.
The minimumReleaseInterval schema is correctly defined using the same validation pattern as rolloutDuration.
75-82: LGTM! Consistent initialization pattern.
The minimumReleaseInterval is properly initialized and formatted using the same patterns as existing code.
97-100: LGTM! Form submission properly handles the new field.
The minimumReleaseInterval is correctly processed and included in the updates. The previous concern about non-negative validation has been addressed through the schema validation.
242-256: LGTM! Improved label clarity.
The label text change provides better clarity, and the input field configuration follows best practices.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (7)
1-1: No issues with typed import.
This import organizes schema access neatly under the SCHEMA namespace, promoting clarity and type safety.
10-10: React hooks import is appropriate.
No concerns with importing useEffect and useState.
12-12: Direct usage of date-fns is fine.
Importing differenceInMilliseconds from date-fns is suitable for precise interval calculations.
14-14: Utility import looks good.
pretty-ms helps user-friendly time display; no issues with the approach.
33-33: Enhanced property for environment policy.
Switching from a policyType string to a structured policy object offers more flexibility.
59-60: Loader styling fix is correct.
The revised order of utility classes ensures the spin animation is applied properly.
328-328: Integration of MinReleaseIntervalCheck is proper.
Rendering the new check in the node ensures minimum interval logic is central to release gating.
packages/api/src/router/release.ts (3)
10-12: Imports for conditional queries are fine.
Using exists and notExists from @ctrlplane/db is a clean way to form subqueries without overcomplicating conditions.
41-45: Active status, job condition, and JobStatus imports are correct.
These validators are used properly to refine the query logic below.
360-417: New 'latest.completed' logic is valid.
This procedure retrieves the most recent completed release for a given deployment/environment, filtering out active/pending job triggers. The query approach is correct and the ordering by release.createdAt ensures the newest record is returned.
Below is a script to check references to the newly introduced “latest.completed” usage. It searches for calls that might expect different return semantics:
✅ Verification successful
The 'latest.completed' endpoint is correctly used and handles null cases appropriately
The endpoint is used in ReleaseSequencingNode.tsx's MinReleaseIntervalCheck component, where:
- The query is properly enabled/disabled based on policy existence
- Null handling is explicit with early returns and null checks
- The result is used safely within useEffect with proper type guards
- The UI gracefully handles loading and null states with appropriate visual feedback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for usage of releaseRouter.latest.completed and verify that it handles no-result cases gracefully
rg -A 5 $'releaseRouter\\.latest\\.completed'
Length of output: 44
Script:
#!/bin/bash
# Let's try a broader search to find any references to 'latest.completed'
rg -A 5 'latest\.completed'
# Also search for imports of the release router to track potential usage
rg -l "from.*router/release"
# Check for potential usage through destructuring
rg "const.*latest.*=.*release"
Length of output: 1388
Script:
#!/bin/bash
# Let's examine the usage context in ReleaseSequencingNode.tsx to verify proper handling
cat 'apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx'
# Also check if there are any tests for this endpoint
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx latest.completed
Length of output: 9962
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx (1)
82-82: Passing the full policy object aligns with the new approach.
Replacing policyType with the entire policy fosters a more detailed policy-based workflow.
| { enabled: policy != null }, | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
generated by ai
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (1)
252-342: Consider optimizing the MinReleaseIntervalCheck component.While the implementation is functionally correct, there are a few opportunities for optimization:
- The interval setup could be more efficient by adding additional conditions to prevent unnecessary interval creation.
- The
calculateTimeLeftfunction could be memoized to prevent recreating it on each render.Consider applying these improvements:
const MinReleaseIntervalCheck: React.FC<ReleaseSequencingNodeProps["data"]> = ({ policy, deploymentId, environmentId, }) => { const [timeLeft, setTimeLeft] = useState<number | null>(null); const { setParams } = useQueryParams(); + const calculateTimeLeft = useCallback(() => { + if (!latestRelease || !policy?.minimumReleaseInterval) return 0; + const timePassed = differenceInMilliseconds( + new Date(), + latestRelease.createdAt, + ); + return Math.max(0, policy.minimumReleaseInterval - timePassed); + }, [latestRelease, policy?.minimumReleaseInterval]); const { data: latestRelease, isLoading } = api.release.latest.completed.useQuery( { deploymentId, environmentId }, - { enabled: policy != null }, + { enabled: policy != null && policy.minimumReleaseInterval > 0 }, ); useEffect(() => { - if (!latestRelease || !policy?.minimumReleaseInterval) return; + if (!latestRelease || !policy?.minimumReleaseInterval || policy.minimumReleaseInterval === 0) return; - const calculateTimeLeft = () => { - const timePassed = differenceInMilliseconds( - new Date(), - latestRelease.createdAt, - ); - return Math.max(0, policy.minimumReleaseInterval - timePassed); - }; setTimeLeft(calculateTimeLeft()); const interval = setInterval(() => { const remaining = calculateTimeLeft(); setTimeLeft(remaining); if (remaining <= 0) clearInterval(interval); }, 1000); return () => clearInterval(interval); }, [latestRelease, policy?.minimumReleaseInterval, calculateTimeLeft]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ReleaseSequencingNode.tsx (3)
Line range hint 1-35: LGTM! Type definitions and imports are well-organized.
The updated ReleaseSequencingNodeProps type properly reflects the new policy-based approach, replacing the simple policyType with a more comprehensive policy property.
344-370: LGTM! ReleaseSequencingNode component is well-structured.
The component properly integrates the new MinReleaseIntervalCheck while maintaining consistent styling and proper handle positioning for the flow diagram.
Line range hint 1-370: Verify policy type updates across the codebase.
Since we've changed from policyType to policy in ReleaseSequencingNodeProps, let's ensure all components using this type have been updated accordingly.
✅ Verification successful
Based on the search results, I can see that:
- There are no occurrences of
policyTypewith "cancel" or "wait" patterns in TypeScript files - The
ReleaseSequencingNodePropstype is only used within the same file (ReleaseSequencingNode.tsx) - All components using
ReleaseSequencingNodeProps["data"]are properly using the newpolicyprop
Let me generate the final response:
Changes from policyType to policy are correctly implemented.
The codebase shows no remaining instances of the old policyType usage, and all components using ReleaseSequencingNodeProps are correctly using the new policy prop. The change is self-contained within the ReleaseSequencingNode.tsx file and doesn't require updates in other parts of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of policyType in TypeScript files
rg "policyType.*(?:cancel|wait)" --type ts
# Search for components using ReleaseSequencingNodeProps
rg "ReleaseSequencingNodeProps" -A 5 --type ts
Length of output: 5276
Summary by CodeRabbit
New Features
minimumReleaseIntervalin the rollout timing settings, allowing users to specify the minimum time between active releases.FlowDiagramcomponent to improve access to policy attributes.MinReleaseIntervalCheckcomponent to manage minimum release interval logic in release sequencing.Bug Fixes
rolloutDurationfield for clarity.Database Changes
minimum_release_intervalto theenvironment_policytable to store the minimum release interval.