fix: Shared form for policy drawer #263
Conversation
WalkthroughThe changes in this pull request involve a comprehensive refactoring of several components related to environment policy management. The primary modification is the transition from using an 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: 8
🧹 Outside diff range and nitpick comments (11)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (3)
Line range hint
192-196: Remove commented out code.This commented code block appears to be obsolete after the refactoring. If it's no longer needed, it should be removed to maintain code cleanliness.
- // const isSettingPolicyDeployment = - // source?.type === NodeType.Environment && - // target?.type === NodeType.Policy; - // if (isSettingEnvironmentPolicy) - // policy.mutate({ id: target.id, data: { policyId: source.id } });
Line range hint
283-286: Consider fixing the eslint warning instead of disabling it.The effect hook has a disabled eslint warning for exhaustive dependencies. Consider including
onLayoutin the dependency array and wrapping it withuseCallbackif needed.useEffect(() => { if (reactFlowInstance != null) onLayout(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [reactFlowInstance]); + }, [reactFlowInstance, onLayout]);
Line range hint
289-307: Consider simplifying the delete key handling logic.The current implementation checks conditions sequentially. This could be simplified using early returns for better readability.
useEffect(() => { - if (!deletePressed) return; - if (selectedNodeId != null) { - setOpen(true); - return; - } - if (selectedEdgeId == null) return; - - const edge = edges.find((e) => e.id === selectedEdgeId); - if (edge == null) return; - handleEdgeDelete(edge); + if (!deletePressed || !selectedNodeId && !selectedEdgeId) return; + + if (selectedNodeId) { + setOpen(true); + return; + } + + const edge = edges.find(e => e.id === selectedEdgeId); + if (edge) handleEdgeDelete(edge); }, [ edges, deletePressed, selectedNodeId, selectedEdgeId, handleEdgeDelete, setOpen, ]);apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseManagement.tsx (2)
15-25: Consider adding ARIA attributes for better accessibilityThe component structure is clean and well-organized. Consider adding
aria-describedbyto connect the heading with its description for better screen reader support.- <h1 className="text-lg font-medium">Release Management</h1> - <span className="text-sm text-muted-foreground"> + <h1 id="release-management-title" className="text-lg font-medium">Release Management</h1> + <span id="release-management-desc" className="text-sm text-muted-foreground" aria-describedby="release-management-title">
26-60: Consider adding error state handlingWhile the form implementation is solid, consider adding error state handling to improve user feedback. The
FormFieldcomponent likely supports error states through the form context.<FormField control={form.control} name="releaseSequencing" - render={({ field: { value, onChange } }) => ( + render={({ field: { value, onChange }, fieldState: { error } }) => ( <FormItem className="space-y-4"> <div className="flex flex-col gap-1"> <FormLabel>Release Sequencing</FormLabel> <FormDescription> Specify whether pending releases should be cancelled or awaited when a new release is triggered. </FormDescription> + {error && ( + <span className="text-sm text-destructive">{error.message}</span> + )} </div>apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/Overview.tsx (1)
18-49: Consider enhancing form field consistency and accessibility.While the implementation is clean, consider the following improvements:
- Add placeholder text to the name field for consistency
- Add aria-label attributes for better accessibility
- Apply consistent null handling across both fields
<FormField control={form.control} name="name" render={({ field }) => ( <FormItem> <FormLabel>Name</FormLabel> <FormControl> - <Input {...field} /> + <Input + {...field} + placeholder="Enter policy name..." + aria-label="Policy name" + value={field.value ?? ""} + /> </FormControl> <FormMessage /> </FormItem> )} /> <FormField control={form.control} name="description" render={({ field: { value, onChange } }) => ( <FormItem> <FormLabel>Description</FormLabel> <FormControl> <Textarea placeholder="Add a description..." + aria-label="Policy description" value={value ?? ""} onChange={onChange} /> </FormControl> <FormMessage /> </FormItem> )} />apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (2)
100-124: Consider consolidating time zone handling logicThe time zone handling is duplicated in both DateTimePicker onChange handlers. This could lead to inconsistencies and maintenance issues.
Consider extracting the time zone conversion logic into a shared utility:
+ const handleTimeChange = (value: any, time: ZonedDateTime) => ({ + ...value, + time: time.toDate(Intl.DateTimeFormat().resolvedOptions().timeZone), + }); <DateTimePicker value={toZonedDateTime(value.startTime)} aria-label="Start Time" - onChange={(t) => { - onChange({ - ...value, - startTime: t.toDate( - Intl.DateTimeFormat().resolvedOptions().timeZone, - ), - }); - }} + onChange={(t) => onChange(handleTimeChange(value, t))} />
66-216: Consider enhancing form field descriptionsWhile the form has good accessibility, users might benefit from more context:
- Add helper text explaining valid duration formats (e.g., "1d", "2h", "30m")
- Consider adding tooltips to explain the impact of different recurrence options
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseChannels.tsx (1)
117-117: PassingpolicyReleaseChannelssafely toDeploymentSelectWhile you are using the nullish coalescing operator here to default to an empty object, if you apply the fix above, this additional fallback may be redundant. Ensure consistency in handling
policyReleaseChannels.Consider simplifying the code:
- releaseChannels={policyReleaseChannels ?? {}} + releaseChannels={policyReleaseChannels}apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx (1)
32-81: Enhance form field implementationThe form fields are well-structured, but could benefit from the following improvements:
- Add debounce to setValue calls to prevent unnecessary rerenders
- Add aria-labels for better accessibility
- Add min/max attributes to the number input
<Input disabled={value !== "some"} type="number" + min="1" + max="100" + aria-label="Maximum concurrent jobs" value={concurrencyLimit} onChange={(e) => form.setValue( "concurrencyLimit", e.target.valueAsNumber, ) } className="border-b-1 h-6 w-16 text-xs" />packages/db/src/schema/environment.ts (1)
330-344: Improve type safety and readability of the releaseChannels refinement.The refinement logic for
releaseChannelscould be improved for better type safety and readability:
- The type inference for deployment IDs and channel IDs in the refinement could be misleading
- The filtering logic could be simplified
Consider this improved implementation:
releaseChannels: z .record(z.string().uuid(), z.string().uuid().nullable()) .optional() .refine((channels) => { if (channels == null) return true; - const channelsWithNonNullDeploymentIds = Object.entries( - channels, - ).filter(([_, channelId]) => channelId != null); - const deploymentIds = new Set( - channelsWithNonNullDeploymentIds.map( - ([deploymentId, _]) => deploymentId, - ), - ); - return deploymentIds.size === channelsWithNonNullDeploymentIds.length; + // Explicitly type the entries for better type inference + const entries: [string, string | null][] = Object.entries(channels); + const validEntries = entries.filter(([_, channelId]) => channelId !== null); + const deploymentIds = new Set(validEntries.map(([deploymentId]) => deploymentId)); + return deploymentIds.size === validEntries.length; }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/Overview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyDeleteDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyFormSchema.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseChannels.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseManagement.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useEnvironmentPolicyDrawer.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.tsx(1 hunks)packages/api/src/router/environment-policy.ts(1 hunks)packages/db/src/schema/environment.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyDeleteDialog.tsx
🧰 Additional context used
📓 Path-based instructions (12)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.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/useEnvironmentPolicyDrawer.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.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.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/Overview.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/ReleaseManagement.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/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/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseChannels.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/DeploymentControl.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/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)/_components/environment-policy-drawer/PolicyFormSchema.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-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.
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyFormSchema.ts
[error] 7-7: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (20)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx (3)
20-24: LGTM! Clean interface refactoring.
The component interface has been properly updated to use the form-based approach, with correct TypeScript typing.
28-38: LGTM! Well-structured and accessible header section.
The header section provides clear context with proper semantic markup and spacing.
40-68: LGTM! Well-implemented form fields with good accessibility.
The form fields are well-structured with:
- Clear labels and descriptions
- Proper form control binding
- Consistent use of shared UI components
- Good accessibility practices
Also applies to: 70-125
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (3)
26-26: LGTM: Import addition aligns with the refactoring.
The new import for useEnvironmentPolicyDrawer is correctly placed and follows the project's module resolution pattern.
Line range hint 134-190: LGTM: Well-structured connection handling logic.
The onConnect function effectively handles different connection scenarios with clear separation of concerns and proper edge management.
Line range hint 309-310: Verify integration with PolicyFormSchema.
The component now uses the environment policy drawer hook, but let's verify its integration with the new form handling approach.
✅ Verification successful
Integration with PolicyFormSchema is properly implemented
The verification shows that:
PolicyFormSchemais properly defined and imported across all drawer components- The environment policy drawer components correctly use the form schema and hook
- The form integration follows the new approach with proper type definitions and zod schema validation
- The
EnvFlowBuildercomponent correctly integrates with the drawer through theuseEnvironmentPolicyDrawerhook
The implementation aligns with the new form handling approach and is consistently used across all related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of PolicyFormSchema with the environment policy drawer
# Check for PolicyFormSchema usage in the drawer component
ast-grep --pattern 'type PolicyFormSchema = {
$$$
}'
# Check for form handling in the drawer component
ast-grep --pattern 'const { $$ } = useEnvironmentPolicyDrawer();'
Length of output: 124
Script:
#!/bin/bash
# Let's try a broader search for PolicyFormSchema and form handling patterns
# Search for PolicyFormSchema type definition or imports
rg "PolicyFormSchema" -A 3
# Search for environment policy drawer hook usage
rg "useEnvironmentPolicyDrawer" -A 3
# Search for form handling patterns in environment-related files
fd "env.*drawer" --type f --exec rg "useForm|FormProvider|zodResolver" -A 3 {}
Length of output: 15117
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseManagement.tsx (2)
10-10: LGTM: Clean transition to form-based approach
The change from environmentPolicy to form prop simplifies the component's interface while maintaining type safety through PolicyFormSchema.
Also applies to: 13-14
10-10: Verify PolicyFormSchema integration
Let's ensure the PolicyFormSchema is consistently used across related components.
Also applies to: 13-14
✅ Verification successful
PolicyFormSchema integration is correctly implemented
The PolicyFormSchema is consistently used across all related components in the environment-policy-drawer:
- Defined in
PolicyFormSchema.tsas a form type using react-hook-form - Properly imported and used in all form components including Overview, RolloutAndTiming, ApprovalAndGovernance, DeploymentControl, and ReleaseChannels
- Correctly integrated in the parent EnvironmentPolicyDrawer component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using PolicyFormSchema
rg -t tsx "PolicyFormSchema" "apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/"
# Verify the schema definition
ast-grep --pattern 'type PolicyFormSchema = {
$$$
releaseSequencing: $_
$$$
}'
Length of output: 230
Script:
#!/bin/bash
# Search for PolicyFormSchema usage with TypeScript extension
rg "PolicyFormSchema" --type ts --type tsx
# Find the schema definition file
fd "PolicyFormSchema.ts" --type f
# Look for related form components
rg "ReleaseManagement" --type ts --type tsx -A 5
Length of output: 285
Script:
#!/bin/bash
# Check the content of PolicyFormSchema.ts
cat "apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyFormSchema.ts"
# Search for any usage without file type flags
rg "PolicyFormSchema"
# Search for ReleaseManagement without file type flags
rg "ReleaseManagement"
Length of output: 3622
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/Overview.tsx (2)
1-13: LGTM! Imports are well-organized.
The imports are properly scoped and all necessary UI components and types are included.
14-17: Verify the impact of the breaking interface change.
The component's interface has changed from accepting environmentPolicy to form. While this aligns with the refactoring goal, we should verify that all consumers of this component have been updated.
✅ Verification successful
Interface change is properly handled across the codebase
The verification shows that the breaking interface change is properly handled. The Overview component is only used once within EnvironmentPolicyDrawer.tsx, where it's correctly passed the form prop along with other props through the spread operator: <Overview {...props} />. The props object includes the form as shown in the View component's type definition: PolicyConfigProps & { form: PolicyFormSchema }.
The form is properly initialized in the parent PolicyConfigForm component and passed down through the component hierarchy, ensuring the interface change is consistently handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the Overview component to ensure they've been updated
# Search for Overview component imports and usage
rg -A 5 "import.*Overview.*environment-policy-drawer"
# Search for any remaining references to environmentPolicy prop
rg "environmentPolicy.*Overview"
Length of output: 90
Script:
#!/bin/bash
# Let's try a broader search to find usages of the Overview component
# Find the Overview component file first to confirm its location
fd Overview.tsx
# Search for any imports or usage of Overview component more broadly
rg -l "Overview" --type tsx
# Search for actual usage of Overview component in TSX files
rg -A 5 "<Overview" --type tsx
Length of output: 415
Script:
#!/bin/bash
# Let's try searching without the file type flag since it's not recognized
# Search for actual usage of the Overview component from environment-policy-drawer
rg -A 5 "<Overview" "$(fd -p "*environment-policy-drawer*")"
# Also search in the parent directories for potential usage
rg -A 5 "<Overview.*environment-policy-drawer" "$(fd -p "*/(app)/*")"
# Search for any imports of the Overview component
rg "import.*Overview.*environment-policy-drawer"
Length of output: 865
Script:
#!/bin/bash
# Let's try with fixed strings and proper glob patterns
# Find the environment-policy-drawer directory files
fd --glob "*environment-policy-drawer*"
# Search for Overview usage in all files
rg -A 5 "<Overview" apps/webservice/src/app
# Search for imports specifically
rg "from.*environment-policy-drawer.*Overview" apps/webservice/src/app
Length of output: 7054
Script:
#!/bin/bash
# Let's examine the EnvironmentPolicyDrawer.tsx file to understand how the Overview component is used
# Check the EnvironmentPolicyDrawer.tsx content
cat apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx
# Also check if there are any other files in the environment-policy-drawer directory
fd . apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/environment-policy-drawer/
Length of output: 10395
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (1)
29-30: LGTM: Type changes align with shared form approach
The transition from environmentPolicy prop to form: PolicyFormSchema aligns with the PR objective of implementing a shared form approach.
Also applies to: 57-58
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ReleaseChannels.tsx (3)
15-15: Import statement for PolicyFormSchema is correct
The import of PolicyFormSchema from "./PolicyFormSchema" is appropriate and correctly adds the necessary type definitions.
21-24: Updated ReleaseChannelProps to include form prop
Replacing the policy prop with form: PolicyFormSchema in the ReleaseChannelProps type aligns with the new form handling approach and ensures consistent state management.
90-93: Destructuring form and ensuring correct usage
The destructuring of form and deployments in the ReleaseChannels component props is correctly updated to match the new ReleaseChannelProps type.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/DeploymentControl.tsx (3)
13-17: LGTM! Clean transition to form-based props
The component interface has been properly updated to use the PolicyFormSchema type, aligning with the broader refactoring goals.
21-31: LGTM! Well-structured UI with clear hierarchy
The component layout uses appropriate semantic elements and spacing, with clear and informative content.
13-13: Verify PolicyFormSchema implementation
Let's ensure the form schema is properly implemented and consistent across components.
✅ Verification successful
Based on the search results from previous commands, I can generate the final response as we have enough context. The ripgrep results show that:
PolicyFormSchemais properly defined in a dedicated file and is a type based on a zod schema (z.infer<typeof policyFormSchema>).- It's consistently imported and used across multiple components (DeploymentControl, RolloutAndTiming, Overview, ApprovalAndGovernance, ReleaseManagement, etc.).
- All components use it consistently as a form prop type with proper typing.
- The schema is being actively used for form validation and type safety.
PolicyFormSchema implementation is properly structured and consistently used
The form schema is well-implemented and follows a consistent pattern across all related components in the environment policy drawer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check PolicyFormSchema implementation and usage
ast-grep --pattern 'type PolicyFormSchema = {
$$$
concurrencyType: $_
concurrencyLimit: $_
$$$
}'
# Check for consistent form field usage
rg -A 2 'PolicyFormSchema' --type typescript
Length of output: 187
Script:
#!/bin/bash
# Search for PolicyFormSchema implementation with ripgrep
rg -g '*.ts*' -A 5 'PolicyFormSchema'
# Find the file containing PolicyFormSchema
fd PolicyFormSchema
Length of output: 11358
Script:
#!/bin/bash
# Let's check the actual implementation of PolicyFormSchema
cat apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/PolicyFormSchema.ts
Length of output: 290
Script:
#!/bin/bash
# Escape the parentheses in the path
cat "apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/environment-policy-drawer/PolicyFormSchema.ts"
Length of output: 253
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (1)
110-115: 🛠️ Refactor suggestion
Add error handling to the onSubmit function
The onSubmit function does not handle errors that might occur during the mutation. It's important to catch and handle these errors to provide feedback to the user if the update fails.
Consider wrapping the mutation in a try...catch block:
const onSubmit = form.handleSubmit(async (policy) => {
const data = { ...policy, rolloutDuration: ms(policy.rolloutDuration) };
- await updateEnvironmentPolicy
+ try {
+ await updateEnvironmentPolicy
.mutateAsync({ data, id })
.then(() => form.reset(policy))
.then(() => utils.environment.policy.byId.invalidate(id))
.then(() => utils.environment.policy.bySystemId.invalidate(systemId));
+ } catch (error) {
+ // Handle the error, e.g., display a notification to the user
+ console.error('Failed to update environment policy:', error);
+ }
});Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useEnvironmentPolicyDrawer.ts (1)
1-55: LGTM!
The custom hooks useEnvironmentPolicyDrawer and useEnvironmentPolicyDrawerTab are well-implemented, managing URL search parameters effectively.
packages/db/src/schema/environment.ts (1)
327-329: LGTM: Schema extension approach is correct.
The approach of extending the partial schema is correct and maintains type safety while allowing optional updates.
| <Input | ||
| disabled={value !== "some"} | ||
| type="number" | ||
| value={successMinimum} | ||
| onChange={(e) => | ||
| form.setValue("successMinimum", e.target.valueAsNumber) | ||
| } | ||
| className="border-b-1 h-6 w-16 text-xs" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the success minimum input.
The number input directly sets the form value without any validation. Consider:
- Adding min/max constraints
- Handling invalid number inputs
- Using form's field registration instead of direct setValue
<Input
disabled={value !== "some"}
type="number"
+ min={1}
value={successMinimum}
- onChange={(e) =>
- form.setValue("successMinimum", e.target.valueAsNumber)
- }
+ {...form.register("successMinimum", {
+ valueAsNumber: true,
+ min: { value: 1, message: "Minimum must be at least 1" }
+ })}
className="border-b-1 h-6 w-16 text-xs"
/>📝 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.
| <Input | |
| disabled={value !== "some"} | |
| type="number" | |
| value={successMinimum} | |
| onChange={(e) => | |
| form.setValue("successMinimum", e.target.valueAsNumber) | |
| } | |
| className="border-b-1 h-6 w-16 text-xs" | |
| /> | |
| <Input | |
| disabled={value !== "some"} | |
| type="number" | |
| min={1} | |
| value={successMinimum} | |
| {...form.register("successMinimum", { | |
| valueAsNumber: true, | |
| min: { value: 1, message: "Minimum must be at least 1" } | |
| })} | |
| className="border-b-1 h-6 w-16 text-xs" | |
| /> |
| <Input | ||
| type="string" | ||
| {...field} | ||
| placeholder="1d" | ||
| className="border-b-1 h-6 w-16 text-xs" | ||
| /> |
There was a problem hiding this comment.
Add validation for rollout duration format
The duration input accepts any string but expects a format compatible with the ms package (e.g., "1d", "2h"). Missing validation could lead to runtime errors.
Consider adding validation and proper error handling:
<Input
type="string"
{...field}
placeholder="1d"
+ onBlur={(e) => {
+ try {
+ ms(e.target.value);
+ field.onChange(e);
+ } catch (err) {
+ form.setError('rolloutDuration', {
+ message: 'Invalid duration format. Use format like "1d", "2h", "30m"'
+ });
+ }
+ }}
className="border-b-1 h-6 w-16 text-xs"
/>Committable suggestion skipped: line range outside the PR's diff.
| </SelectGroup> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => remove(index)} | ||
| className="h-6 w-6" | ||
| > | ||
| <IconX className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ); | ||
| }} | ||
| /> | ||
| ); | ||
| })} | ||
| </div> | ||
| )} | ||
|
|
||
| <Button | ||
| type="button" | ||
| variant="outline" | ||
| className="w-fit" | ||
| onClick={() => | ||
| append({ | ||
| policyId: environmentPolicy.id, | ||
| recurrence: "weekly", | ||
| startTime: new Date(), | ||
| endTime: new Date(), | ||
| }) | ||
| } | ||
| > | ||
| Add Release Window | ||
| </Button> | ||
| </div> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="rolloutDuration" | ||
| render={({ field }) => ( | ||
| <FormItem className="space-y-4"> | ||
| <div className="flex flex-col gap-1"> | ||
| <FormLabel>Gradual Rollout</FormLabel> | ||
| <FormDescription> | ||
| Enabling gradual rollouts will spread deployments out over a | ||
| given duration. A default duration of 0ms means that | ||
| deployments will be rolled out immediately. | ||
| </FormDescription> | ||
| </div> | ||
| <FormControl> | ||
| <div className="flex flex-col gap-1"> | ||
| <div className="flex items-center gap-2"> | ||
| <span className="text-sm text-muted-foreground"> | ||
| Spread deployments out over | ||
| </span> | ||
| <Input | ||
| type="string" | ||
| {...field} | ||
| placeholder="1d" | ||
| className="border-b-1 h-6 w-16 text-xs" | ||
| /> | ||
| </div> | ||
| <FormMessage /> | ||
| </div> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <SelectTrigger className="h-8"> | ||
| <SelectValue> | ||
| {_.capitalize(value.recurrence)} | ||
| </SelectValue> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectGroup> | ||
| <SelectItem value="hourly">hourly</SelectItem> | ||
| <SelectItem value="daily">daily</SelectItem> | ||
| <SelectItem value="weekly">weekly</SelectItem> | ||
| <SelectItem value="monthly">monthly</SelectItem> | ||
| </SelectGroup> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => remove(index)} | ||
| className="h-6 w-6" | ||
| > | ||
| <IconX className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add validation for release window time ranges
The review comment is valid. The schema in packages/db/src/schema/environment.ts only validates the basic types for release windows without any time range validations. The setPolicyReleaseWindow schema only ensures that startTime and endTime are valid timestamps, and recurrence is a valid enum value.
Add these validations to updateEnvironmentPolicy schema:
releaseWindows: z.array(setPolicyReleaseWindow)
.refine(
(windows) => windows.every(w => w.startTime < w.endTime),
"Start time must be before end time"
)
.refine(
(windows) => {
// Check for overlaps within same recurrence
const windowsByRecurrence = windows.reduce((acc, w) => {
acc[w.recurrence] = acc[w.recurrence] || [];
acc[w.recurrence].push(w);
return acc;
}, {});
return Object.values(windowsByRecurrence).every(group =>
group.every((w1, i) =>
group.every((w2, j) =>
i === j || w1.endTime <= w2.startTime || w2.endTime <= w1.startTime
)
)
);
},
"Release windows with same recurrence must not overlap"
)
.optional()🔗 Analysis chain
Add validation for release window time ranges
The current implementation lacks validation for:
- Ensuring start time is before end time
- Detecting overlapping release windows
Let's check if these validations exist in the schema:
Consider adding validation rules to prevent invalid time ranges:
// In PolicyFormSchema.ts
releaseWindows: z.array(
z.object({
startTime: z.date(),
endTime: z.date(),
recurrence: z.enum(['hourly', 'daily', 'weekly', 'monthly'])
})
).refine(
(windows) => windows.every(w => w.startTime < w.endTime),
"Start time must be before end time"
).refine(
(windows) => {
// Add overlap detection logic here
return true;
},
"Release windows must not overlap"
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation rules in PolicyFormSchema
rg -A 10 "releaseWindows.*schema"
Length of output: 35
Script:
#!/bin/bash
# Let's try a broader search for schema definitions related to release windows
rg -A 10 "releaseWindows.*z\."
# Also search for any form validation related to release windows
rg -A 10 "releaseWindows.*validate"
# Search for any files that might contain the schema definition
fd "schema|validation" --type f
Length of output: 438
Script:
#!/bin/bash
# Let's check the content of the PolicyFormSchema and the setPolicyReleaseWindow schema
cat apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/environment-policy-drawer/PolicyFormSchema.ts
# Also check the environment schema that contains the release window definition
cat packages/db/src/schema/environment.ts
Length of output: 11085
| const policyReleaseChannels = form.watch("releaseChannels"); | ||
|
|
||
| const updateReleaseChannel = ( | ||
| deploymentId: string, | ||
| channelId: string | null, | ||
| ) => setReleaseChannels((prev) => ({ ...prev, [deploymentId]: channelId })); | ||
|
|
||
| const onSubmit = () => | ||
| updateReleaseChannels | ||
| .mutateAsync({ | ||
| id: policy.id, | ||
| releaseChannels, | ||
| }) | ||
| .then(() => utils.environment.policy.byId.invalidate(policy.id)); | ||
| ) => | ||
| form.setValue( | ||
| "releaseChannels", | ||
| { ...policyReleaseChannels, [deploymentId]: channelId }, | ||
| { shouldValidate: true, shouldDirty: true, shouldTouch: true }, | ||
| ); |
There was a problem hiding this comment.
Handle potential undefined in policyReleaseChannels
If policyReleaseChannels is undefined, spreading it with { ...policyReleaseChannels } in updateReleaseChannel will cause an error. To prevent this, default policyReleaseChannels to an empty object.
Apply this diff to fix the issue:
- const policyReleaseChannels = form.watch("releaseChannels");
+ const policyReleaseChannels = form.watch("releaseChannels") ?? {};This ensures that policyReleaseChannels is always an object before using the spread operator.
📝 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 policyReleaseChannels = form.watch("releaseChannels"); | |
| const updateReleaseChannel = ( | |
| deploymentId: string, | |
| channelId: string | null, | |
| ) => setReleaseChannels((prev) => ({ ...prev, [deploymentId]: channelId })); | |
| const onSubmit = () => | |
| updateReleaseChannels | |
| .mutateAsync({ | |
| id: policy.id, | |
| releaseChannels, | |
| }) | |
| .then(() => utils.environment.policy.byId.invalidate(policy.id)); | |
| ) => | |
| form.setValue( | |
| "releaseChannels", | |
| { ...policyReleaseChannels, [deploymentId]: channelId }, | |
| { shouldValidate: true, shouldDirty: true, shouldTouch: true }, | |
| ); | |
| const policyReleaseChannels = form.watch("releaseChannels") ?? {}; | |
| const updateReleaseChannel = ( | |
| deploymentId: string, | |
| channelId: string | null, | |
| ) => | |
| form.setValue( | |
| "releaseChannels", | |
| { ...policyReleaseChannels, [deploymentId]: channelId }, | |
| { shouldValidate: true, shouldDirty: true, shouldTouch: true }, | |
| ); |
| const { concurrencyLimit } = form.watch(); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for concurrencyLimit
The form watches concurrencyLimit but lacks validation. Consider adding checks for negative values and NaN.
- const { concurrencyLimit } = form.watch();
+ const { concurrencyLimit } = form.watch();
+ const isValidLimit = !isNaN(concurrencyLimit) && concurrencyLimit > 0;Committable suggestion skipped: line range outside the PR's diff.
| }), | ||
| ) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser | ||
| .perform(Permission.SystemUpdate) | ||
| .on({ type: "environmentPolicy", id: input.id }), | ||
| }) | ||
| .mutation(({ ctx, input }) => { | ||
| const { id, releaseChannels } = input; | ||
| const [nulled, set] = _.partition( | ||
| Object.entries(releaseChannels), | ||
| ([_, channelId]) => channelId == null, | ||
| ); | ||
| const { releaseWindows } = input.data; | ||
| if (releaseWindows != null) | ||
| await ctx.db.transaction((tx) => | ||
| tx | ||
| .delete(environmentPolicyReleaseWindow) | ||
| .where(eq(environmentPolicyReleaseWindow.policyId, input.id)) | ||
| .then(() => | ||
| releaseWindows.length === 0 | ||
| ? [] | ||
| : tx | ||
| .insert(environmentPolicyReleaseWindow) | ||
| .values(releaseWindows) | ||
| .returning(), | ||
| ), | ||
| ); | ||
|
|
||
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | ||
| const setChannels = set.map(([deploymentId, channelId]) => ({ | ||
| policyId: id, | ||
| deploymentId, | ||
| channelId: channelId!, | ||
| })); | ||
| if (input.data.releaseChannels != null) { | ||
| const [nulled, set] = _.partition( | ||
| Object.entries(input.data.releaseChannels), | ||
| ([_, channelId]) => channelId == null, | ||
| ); | ||
|
|
||
| return ctx.db.transaction(async (db) => { | ||
| await db | ||
| .delete(environmentPolicyReleaseChannel) | ||
| .where( | ||
| inArray(environmentPolicyReleaseChannel.deploymentId, nulledIds), | ||
| ); | ||
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | ||
| const setChannels = set.map(([deploymentId, channelId]) => ({ | ||
| policyId: input.id, | ||
| deploymentId, | ||
| channelId: channelId!, | ||
| })); | ||
|
|
||
| await ctx.db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(environmentPolicyReleaseChannel) | ||
| .where( | ||
| and( | ||
| inArray( | ||
| environmentPolicyReleaseChannel.deploymentId, | ||
| nulledIds, | ||
| ), | ||
| eq(environmentPolicyReleaseChannel.policyId, input.id), | ||
| ), | ||
| ); | ||
|
|
||
| if (setChannels.length > 0) | ||
| await db | ||
| .insert(environmentPolicyReleaseChannel) | ||
| .values(setChannels) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| environmentPolicyReleaseChannel.policyId, | ||
| environmentPolicyReleaseChannel.deploymentId, | ||
| ], | ||
| set: buildConflictUpdateColumns(environmentPolicyReleaseChannel, [ | ||
| "channelId", | ||
| ]), | ||
| }); | ||
| }); | ||
| if (setChannels.length > 0) | ||
| await tx | ||
| .insert(environmentPolicyReleaseChannel) | ||
| .values(setChannels) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| environmentPolicyReleaseChannel.policyId, | ||
| environmentPolicyReleaseChannel.deploymentId, | ||
| ], | ||
| set: buildConflictUpdateColumns( | ||
| environmentPolicyReleaseChannel, | ||
| ["channelId"], | ||
| ), | ||
| }); | ||
| }); | ||
|
|
||
| return policy; | ||
| } |
There was a problem hiding this comment.
Ensure the update mutation always returns the updated policy
The return policy; statement is currently inside the if block checking for input.data.releaseChannels != null. If releaseChannels is null or undefined, the function won't return the policy, which could lead to unexpected behavior.
Move the return policy; statement outside of the if block to ensure the policy is always returned:
});
if (input.data.releaseChannels != null) {
// ... existing code ...
}
+ return policy;
}),📝 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.
| .mutation(async ({ ctx, input }) => { | |
| const policy = await ctx.db | |
| .update(environmentPolicy) | |
| .set(input.data) | |
| .where(eq(environmentPolicy.id, input.id)) | |
| .returning() | |
| .then(takeFirst), | |
| ), | |
| .then(takeFirst); | |
| updateReleaseChannels: protectedProcedure | |
| .input( | |
| z.object({ | |
| id: z.string().uuid(), | |
| releaseChannels: z.record(z.string().uuid().nullable()), | |
| }), | |
| ) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.SystemUpdate) | |
| .on({ type: "environmentPolicy", id: input.id }), | |
| }) | |
| .mutation(({ ctx, input }) => { | |
| const { id, releaseChannels } = input; | |
| const [nulled, set] = _.partition( | |
| Object.entries(releaseChannels), | |
| ([_, channelId]) => channelId == null, | |
| ); | |
| const { releaseWindows } = input.data; | |
| if (releaseWindows != null) | |
| await ctx.db.transaction((tx) => | |
| tx | |
| .delete(environmentPolicyReleaseWindow) | |
| .where(eq(environmentPolicyReleaseWindow.policyId, input.id)) | |
| .then(() => | |
| releaseWindows.length === 0 | |
| ? [] | |
| : tx | |
| .insert(environmentPolicyReleaseWindow) | |
| .values(releaseWindows) | |
| .returning(), | |
| ), | |
| ); | |
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | |
| const setChannels = set.map(([deploymentId, channelId]) => ({ | |
| policyId: id, | |
| deploymentId, | |
| channelId: channelId!, | |
| })); | |
| if (input.data.releaseChannels != null) { | |
| const [nulled, set] = _.partition( | |
| Object.entries(input.data.releaseChannels), | |
| ([_, channelId]) => channelId == null, | |
| ); | |
| return ctx.db.transaction(async (db) => { | |
| await db | |
| .delete(environmentPolicyReleaseChannel) | |
| .where( | |
| inArray(environmentPolicyReleaseChannel.deploymentId, nulledIds), | |
| ); | |
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | |
| const setChannels = set.map(([deploymentId, channelId]) => ({ | |
| policyId: input.id, | |
| deploymentId, | |
| channelId: channelId!, | |
| })); | |
| await ctx.db.transaction(async (tx) => { | |
| await tx | |
| .delete(environmentPolicyReleaseChannel) | |
| .where( | |
| and( | |
| inArray( | |
| environmentPolicyReleaseChannel.deploymentId, | |
| nulledIds, | |
| ), | |
| eq(environmentPolicyReleaseChannel.policyId, input.id), | |
| ), | |
| ); | |
| if (setChannels.length > 0) | |
| await db | |
| .insert(environmentPolicyReleaseChannel) | |
| .values(setChannels) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| environmentPolicyReleaseChannel.policyId, | |
| environmentPolicyReleaseChannel.deploymentId, | |
| ], | |
| set: buildConflictUpdateColumns(environmentPolicyReleaseChannel, [ | |
| "channelId", | |
| ]), | |
| }); | |
| }); | |
| if (setChannels.length > 0) | |
| await tx | |
| .insert(environmentPolicyReleaseChannel) | |
| .values(setChannels) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| environmentPolicyReleaseChannel.policyId, | |
| environmentPolicyReleaseChannel.deploymentId, | |
| ], | |
| set: buildConflictUpdateColumns( | |
| environmentPolicyReleaseChannel, | |
| ["channelId"], | |
| ), | |
| }); | |
| }); | |
| return policy; | |
| } | |
| .mutation(async ({ ctx, input }) => { | |
| const policy = await ctx.db | |
| .update(environmentPolicy) | |
| .set(input.data) | |
| .where(eq(environmentPolicy.id, input.id)) | |
| .returning() | |
| .then(takeFirst); | |
| const { releaseWindows } = input.data; | |
| if (releaseWindows != null) | |
| await ctx.db.transaction((tx) => | |
| tx | |
| .delete(environmentPolicyReleaseWindow) | |
| .where(eq(environmentPolicyReleaseWindow.policyId, input.id)) | |
| .then(() => | |
| releaseWindows.length === 0 | |
| ? [] | |
| : tx | |
| .insert(environmentPolicyReleaseWindow) | |
| .values(releaseWindows) | |
| .returning(), | |
| ), | |
| ); | |
| if (input.data.releaseChannels != null) { | |
| const [nulled, set] = _.partition( | |
| Object.entries(input.data.releaseChannels), | |
| ([_, channelId]) => channelId == null, | |
| ); | |
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | |
| const setChannels = set.map(([deploymentId, channelId]) => ({ | |
| policyId: input.id, | |
| deploymentId, | |
| channelId: channelId!, | |
| })); | |
| await ctx.db.transaction(async (tx) => { | |
| await tx | |
| .delete(environmentPolicyReleaseChannel) | |
| .where( | |
| and( | |
| inArray( | |
| environmentPolicyReleaseChannel.deploymentId, | |
| nulledIds, | |
| ), | |
| eq(environmentPolicyReleaseChannel.policyId, input.id), | |
| ), | |
| ); | |
| if (setChannels.length > 0) | |
| await tx | |
| .insert(environmentPolicyReleaseChannel) | |
| .values(setChannels) | |
| .onConflictDoUpdate({ | |
| target: [ | |
| environmentPolicyReleaseChannel.policyId, | |
| environmentPolicyReleaseChannel.deploymentId, | |
| ], | |
| set: buildConflictUpdateColumns( | |
| environmentPolicyReleaseChannel, | |
| ["channelId"], | |
| ), | |
| }); | |
| }); | |
| } | |
| return policy; |
|
|
||
| import * as SCHEMA from "@ctrlplane/db/schema"; | ||
|
|
||
| const isValidDuration = (str: string) => !isNaN(ms(str)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Number.isNaN instead of isNaN for safer validation
Using the global isNaN function can lead to unexpected results due to type coercion. Number.isNaN is safer because it checks if the value is exactly NaN without coercion.
[static_analysis_hints]
Update the isValidDuration function:
- const isValidDuration = (str: string) => !isNaN(ms(str));
+ const isValidDuration = (str: string) => !Number.isNaN(ms(str));📝 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 isValidDuration = (str: string) => !isNaN(ms(str)); | |
| const isValidDuration = (str: string) => !Number.isNaN(ms(str)); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| ); | ||
| return deploymentIds.size === channelsWithNonNullDeploymentIds.length; | ||
| }), | ||
| releaseWindows: z.array(setPolicyReleaseWindow).optional(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for releaseWindows.
The releaseWindows field is added without any additional validation. Consider adding refinements to ensure:
- Non-overlapping time windows
- Valid time ranges (end time after start time)
Add validation like this:
- releaseWindows: z.array(setPolicyReleaseWindow).optional(),
+ releaseWindows: z
+ .array(setPolicyReleaseWindow)
+ .optional()
+ .refine((windows) => {
+ if (!windows) return true;
+ return windows.every(window =>
+ new Date(window.startTime) < new Date(window.endTime)
+ );
+ }, "End time must be after start time")
+ .refine((windows) => {
+ if (!windows) return true;
+ // Sort windows by start time for easier overlap detection
+ const sorted = [...windows].sort((a, b) =>
+ new Date(a.startTime).getTime() - new Date(b.startTime).getTime()
+ );
+ // Check for overlaps
+ for (let i = 0; i < sorted.length - 1; i++) {
+ if (new Date(sorted[i].endTime) > new Date(sorted[i + 1].startTime)) {
+ return false;
+ }
+ }
+ return true;
+ }, "Release windows must not overlap"),📝 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.
| releaseWindows: z.array(setPolicyReleaseWindow).optional(), | |
| releaseWindows: z | |
| .array(setPolicyReleaseWindow) | |
| .optional() | |
| .refine((windows) => { | |
| if (!windows) return true; | |
| return windows.every(window => | |
| new Date(window.startTime) < new Date(window.endTime) | |
| ); | |
| }, "End time must be after start time") | |
| .refine((windows) => { | |
| if (!windows) return true; | |
| // Sort windows by start time for easier overlap detection | |
| const sorted = [...windows].sort((a, b) => | |
| new Date(a.startTime).getTime() - new Date(b.startTime).getTime() | |
| ); | |
| // Check for overlaps | |
| for (let i = 0; i < sorted.length - 1; i++) { | |
| if (new Date(sorted[i].endTime) > new Date(sorted[i + 1].startTime)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }, "Release windows must not overlap"), |
Summary by CodeRabbit
New Features
PolicyFormSchemafor managing policy forms.Bug Fixes
Documentation
environmentPolicytoform.Refactor
PolicyConfigFormcomponent for better organization.