fix: Add override policy to env #295
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of the environment policy management system. The changes span multiple components and database schemas, focusing on enhancing the flexibility and granularity of environment policy configurations. Key modifications include restructuring the environment drawer, introducing new policy-related components, and updating database schemas to support more detailed policy management. The changes enable more sophisticated control over deployment processes, including approval workflows, release sequencing, and timing configurations. Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (19)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseManagement.tsx (1)
46-47: Add error handling and loading state management.Consider these improvements:
- Add error handling for the Promise-based update
- Disable the RadioGroup during updates
- Show loading state during updates
Here's a suggested implementation:
<RadioGroup value={releaseSequencing} + disabled={isLoading} onValueChange={(releaseSequencing: "wait" | "cancel") => - onUpdate({ releaseSequencing }) + onUpdate({ releaseSequencing }) + .catch((error) => { + console.error('Failed to update release sequencing:', error); + // Consider adding a toast notification here + }) } >apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx (2)
19-21: Consider reducing doubleNonNullable.Using
NonNullable<NonNullable<T>>can be slightly redundant if you are certain the outer reference is never null. This is minor, but simplifying it to a singleNonNullablemight make the type definition cleaner.
103-103: Potential naming conflict.The local variable
releaseChannelsshadows the same property name in the destructuredenvironmentPolicy, which can be slightly confusing. Consider renaming one of them (e.g.,existingChannels) for clarity.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/RolloutAndTiming.tsx (3)
93-94: Consider adding error handling to the form submission.While the change to use
onUpdatefollows React's best practices, consider adding error handling to manage potential failures:- onUpdate(updates); + try { + await onUpdate(updates); + } catch (error) { + // Handle or surface the error appropriately + console.error('Failed to update policy:', error); + // Optionally: Set form error state or show user feedback + }
280-280: Consider adding loading state feedback to the button text.While the disable condition is good, consider enhancing the UX by reflecting the loading state in the button text:
- <Button type="submit" disabled={isLoading || !form.formState.isDirty}> - Save - </Button> + <Button type="submit" disabled={isLoading || !form.formState.isDirty}> + {isLoading ? "Saving..." : "Save Changes"} + </Button>
Line range hint
29-34: Consider extracting duration validation logic.The
isValidDurationfunction could be moved to a shared utility file since duration validation is likely needed elsewhere in the application. This would promote code reuse and consistency in duration validation across the application.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/DeploymentControl.tsx (2)
28-35: Enhance error handling for better debugging.The current error handling could be improved in several ways:
- Catch specific error types instead of using a generic catch block
- Include more context in the error message
- Handle Promise rejections from
onUpdateConsider this improvement:
try { if (concurrencyLimit === "") return; const limit = Number(concurrencyLimit); if (Number.isNaN(limit)) return; - onUpdate({ concurrencyLimit: limit }); + onUpdate({ concurrencyLimit: limit }).catch((error) => { + console.error('Failed to update concurrency limit:', error); + toast.error(`Failed to update concurrency limit: ${error.message}`); + }); - } catch { - toast.error("Failed to update concurrency limit"); + } catch (error) { + console.error('Error processing concurrency limit update:', error); + toast.error(`Error processing concurrency limit: ${error.message}`); }
69-69: Consider consistent debouncing behavior.The radio group handler directly calls
onUpdatewhile input changes are debounced. Consider using consistent debouncing behavior across all update scenarios to prevent potential race conditions or API throttling issues.Consider extracting the update logic into a shared debounced function:
const debouncedUpdate = useCallback( (value: number | null) => { onUpdate({ concurrencyLimit: value }).catch((error) => { console.error('Failed to update concurrency limit:', error); toast.error(`Failed to update concurrency limit: ${error.message}`); }); }, [onUpdate] );packages/db/src/schema/environment.ts (3)
101-105: Clarify mixed usage of"all","some","optional".
The definitions forenvironmentPolicyDeploymentSuccessTypeare intuitive, but consider clarifying the difference between"some"and"optional"in documentation or constants to avoid confusion among collaborators.
173-190: Partial policy creation.
UsingcreateEnvironmentPolicy.partial()forupdateEnvironmentPolicyis a great approach to keep insert and update logic consistent. Consider adding field-level constraints (e.g., minimum concurrency limit) as needed.
198-220: Release windows design.
DefiningenvironmentPolicyReleaseWindowwithstartTime,endTime,recurrenceis well-structured. For future expansions, consider indexingstartTimeandendTimeif queries will often filter by release windows.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (2)
35-35: Use relevant docstring.
Consider adding a brief docstring touseUpdatePolicyto explain its usage, especially if it manages side effects or triggers asynchronous requests.
124-129: Separate concerns of deployments query.
While you’re correctly handling the query for deployments, consider placing deployment-specific logic in a custom hook if it becomes more complex, improving code organization.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (1)
225-226: Use optional chaining to simplify null checks.
You can merge lines 225 and 226 into a single optional chain, as recommended by static analysis.- {environment.policy != null && - environment.policy.isOverride && ( + {environment?.policy?.isOverride && ( <UpdateOverride environment={environment} ...🧰 Tools
🪛 Biome (1.9.4)
[error] 225-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useUpdatePolicy.ts (1)
16-17: Consider enhancing error handling in the update function.The
onUpdatefunction could benefit from explicit error handling to manage failed updates gracefully.- const onUpdate = (data: SCHEMA.UpdateEnvironmentPolicy) => - updatePolicy.mutateAsync({ id, data }).then(invalidatePolicy); + const onUpdate = async (data: SCHEMA.UpdateEnvironmentPolicy) => { + try { + await updatePolicy.mutateAsync({ id, data }); + await invalidatePolicy(); + } catch (error) { + // Handle or rethrow the error as needed + throw error; + } + };apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts (1)
52-58: Consider enhancing error handling in the onUpdate function.Similar to the previous suggestion, the
onUpdatefunction could benefit from explicit error handling.- const onUpdate = (data: SCHEMA.UpdateEnvironmentPolicy) => - updatePolicy - .mutateAsync({ - id: policy.id, - data, - }) - .then(invalidateQueries); + const onUpdate = async (data: SCHEMA.UpdateEnvironmentPolicy) => { + try { + await updatePolicy.mutateAsync({ + id: policy.id, + data, + }); + await invalidateQueries(); + } catch (error) { + // Handle or rethrow the error as needed + throw error; + } + };apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx (1)
38-76: Consider optimizing tab-to-component mapping.The current implementation uses multiple conditional renders. This could be simplified using a mapping object to reduce repetition and improve maintainability.
+ const TAB_COMPONENTS = { + [EnvironmentDrawerTab.Approval]: ApprovalAndGovernance, + [EnvironmentDrawerTab.Concurrency]: DeploymentControl, + [EnvironmentDrawerTab.Management]: ReleaseManagement, + [EnvironmentDrawerTab.ReleaseChannels]: ReleaseChannels, + [EnvironmentDrawerTab.Rollout]: RolloutAndTiming, + } as const; + return ( - <> - {activeTab === EnvironmentDrawerTab.Approval && ( - <ApprovalAndGovernance - environmentPolicy={environmentPolicy} - onUpdate={onUpdate} - isLoading={isUpdating} - /> - )} - {/* ... other conditions ... */} - </> + <> + {TAB_COMPONENTS[activeTab] && + React.createElement(TAB_COMPONENTS[activeTab], { + environmentPolicy, + onUpdate, + isLoading: isUpdating, + ...(activeTab === EnvironmentDrawerTab.ReleaseChannels && { deployments }), + })} + </> );apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/CreateOverride.tsx (1)
44-91: Consider memoizing form components for better performance.The component structure is clean and follows React best practices. However, since each policy form component receives the same props on every render, consider memoizing them to prevent unnecessary re-renders.
+import { useMemo } from "react"; export const CreateOverride: React.FC<CreateOverrideProps> = ({ environment, activeTab, deployments, }) => { const { onCreate, isCreating } = useCreateOverridePolicy(environment); + + const approvalComponent = useMemo( + () => ( + <ApprovalAndGovernance + environmentPolicy={DEFAULT_ENVIRONMENT_POLICY} + onUpdate={onCreate} + isLoading={isCreating} + /> + ), + [onCreate, isCreating] + ); + + // Similar memoization for other form components return ( <> - {activeTab === EnvironmentDrawerTab.Approval && ( - <ApprovalAndGovernance - environmentPolicy={DEFAULT_ENVIRONMENT_POLICY} - onUpdate={onCreate} - isLoading={isCreating} - /> - )} + {activeTab === EnvironmentDrawerTab.Approval && approvalComponent} // Similar changes for other tabs </> ); };apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx (1)
117-158: Remove commented-out code.There's a significant block of commented-out code at the end of the file. This should be removed as it adds noise and could lead to confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/CreateOverride.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/tabs.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useUpdatePolicy.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/DeploymentControl.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseManagement.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/RolloutAndTiming.tsx(3 hunks)packages/api/src/router/environment-policy.ts(1 hunks)packages/api/src/router/environment.ts(3 hunks)packages/db/drizzle/0065_strange_the_hunter.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment-policy-deployment.ts(1 hunks)packages/db/src/schema/environment-policy-relations.ts(1 hunks)packages/db/src/schema/environment-policy.ts(0 hunks)packages/db/src/schema/environment-relations.ts(1 hunks)packages/db/src/schema/environment.ts(4 hunks)packages/db/src/schema/index.ts(0 hunks)packages/db/src/schema/release-channel-relations.ts(1 hunks)packages/db/src/schema/release-channel.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/db/src/schema/index.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/ApprovalAndGovernance.tsx
- packages/db/src/schema/environment-policy.ts
✅ Files skipped from review due to trivial changes (5)
- packages/db/src/schema/release-channel-relations.ts
- packages/db/src/schema/environment-policy-relations.ts
- packages/db/src/schema/release-channel.ts
- packages/db/src/schema/environment-relations.ts
- packages/db/src/schema/environment-policy-deployment.ts
🧰 Additional context used
📓 Path-based instructions (15)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/tabs.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.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/useUpdatePolicy.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.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/policy-form-components/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-drawer/policy-override/UpdateOverride.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/policy-form-components/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/policy-form-components/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.
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-drawer/policy-override/useOverridePolicy.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/policy-form-components/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-drawer/policy-override/CreateOverride.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-drawer/EnvironmentDrawer.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/policy-form-components/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.
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.
📓 Learnings (3)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/tabs.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx
[error] 225-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (34)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseManagement.tsx (3)
8-9: LGTM! Props type definition is well-structured.The type changes improve type safety and component focus by:
- Explicitly defining the required policy fields
- Using a strongly-typed update callback
14-16: LGTM! Component parameters are properly structured.The destructured props align with the type definition and follow React conventions.
18-18: LGTM! Clean implementation of state management.The destructuring and loading indicator implementation are clear and follow React best practices.
Also applies to: 25-25
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx (5)
27-27: Good approach to abstract policy updates.Exposing an
onUpdatecallback that returns aPromise<void>decouples data mutation logic from the component, promoting reusability and testability.
36-36: Async update method is sensible.Returning a
Promise<void>forupdateReleaseChannelis a good pattern here, avoiding confusion with synchronous updates and preserving room for future enhancements without blocking the UI.
97-97: DestructuringonUpdate.Destructuring
onUpdatein the component props aligns well with React best practices and keeps the function usage consistent with the new callback-based approach.
114-117: Merging updated channels correctly.This approach correctly merges the updated value for one deployment into the existing
releaseChannelsobject before passing it toonUpdate. Ensure downstream handlers handle partial updates as expected.
123-123: Loading spinner usage looks good.Displaying the spinner when
isLoadingistruehelps communicate the ongoing operation to the user, improving UX. No changes needed here.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/RolloutAndTiming.tsx (2)
59-65: LGTM! Props type changes improve type safety and component flexibility.The changes to
RolloutAndTimingPropsmake the component's requirements more explicit and reduce coupling with the schema types. The addition ofonUpdatecallback follows React's best practices for state management.
70-71: LGTM! Clean prop destructuring.The addition of
onUpdateto the destructured props is consistent with the type changes.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/DeploymentControl.tsx (2)
12-13: LGTM! Props type changes follow interface segregation principle.The simplified
environmentPolicytype and the addition ofonUpdateprop with proper typing improve the component's interface design.
46-46: LGTM! Clean implementation of loading state.The loading indicator implementation is clean and provides good user feedback during updates.
packages/db/src/schema/environment.ts (6)
38-52: Validate thepolicyIdforeign key usage.
Yourenvironmenttable now referencesenvironmentPolicyvia thepolicyIdFkforeign key and setsonDelete("set null"). Ensure you have upstream logic or data validation so that orphaned references to non-existent or deleted policies do not compromise data integrity.
96-99: Enum definition looks good.
TheapprovalRequirementenum is well-defined with"manual"and"automatic". This offers a clear choice for deployment approvals.
106-110: Release sequencing approach is well-structured.
Storing the release-sequencing type ("wait"vs."cancel") as an enum is a good design choice for clarity and validation.
111-158: Consider validations on concurrency parameters.
While the concurrency limits, rollout duration, and minimum release interval fields are defined, ensure these values undergo range validation (e.g., non-negative constraints) in higher-level logic to prevent invalid data from being inserted.
161-172: Double-check the foreign key usage inoverridePolicyFKConstraint.
The override policy FK referencesenvironmentPolicy.environmentId→environment.id. Confirm that a singleenvironmentIdwill not cause issues if multiple policies share the same environment in the future.
227-249: Approval tracking logic.
TheenvironmentPolicyApprovaltable is a solid foundation for tracking user approvals with statuses and timestamps. Ensure that rejected or partially approved states are managed consistently in your application’s business logic.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (4)
27-28: Imports reorganization is coherent.
The new imports forCreateOverrideandUpdateOverridefrompolicy-form-componentsare consistent with the refactored structure.
116-122: Strengthen type coverage.
You’re returning a combined type forenvironmentPolicythat includesreleaseWindowsandreleaseChannels. Verify that the shape aligns with your server responses to avoid runtime mismatches.
130-175: Manage tab loading states carefully.
Combining loading states (isDeploymentsLoadingorisUpdating) is fine. Just verify you don’t mask certain states that may require user feedback (e.g., partial failures).
258-258: Validate the policy object before rendering.
Rendering<View>whenenvironmentPolicyis non-null is valid. Ensure any UI or data binding reliant on nested fields gracefully handles missing or null fields.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (4)
6-7: Icon usage is consistent.
Importing the new icons –IconCalendar,IconChecklist,IconLock, andIconRocket– keeps your tab design thematically aligned. No issues here.Also applies to: 12-12, 14-14
26-28: Transition toCreateOverrideandUpdateOverridecomponents.
Replacing or augmenting older policy override logic with these new components appears consistent with the revised approach.
47-48: Typed approach for tab parsing.
CastingtabtoEnvironmentDrawerTabis a neat solution. However, ensure that any invalid tab param gracefully defaults rather than throwing an error.Also applies to: 49-49
102-103: External policy detection.
isUsingExternalPolicycheck is straightforward. Confirm that the front-end logic remains consistent if the environment’s policy changes from external to override on the backend.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/tabs.ts (1)
1-9: LGTM! Well-structured enum implementation.The
EnvironmentDrawerTabenum follows TypeScript best practices with clear, descriptive values and consistent naming conventions. The kebab-case string values are appropriate for URL-friendly usage.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/CreateOverride.tsx (1)
26-42: LGTM! Well-structured default policy configuration.The DEFAULT_ENVIRONMENT_POLICY constant is well-defined with appropriate default values for all required fields. The
isOverride: trueflag correctly identifies this as an override policy.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx (1)
10-18: LGTM! Well-defined prop types.The prop types are clearly defined with appropriate types for each property.
packages/api/src/router/environment-policy.ts (1)
298-298: LGTM! Proper association of release windows with policy.The update correctly associates each release window with the policy by adding the policyId. This ensures proper data relationships in the database.
packages/api/src/router/environment.ts (1)
101-104: LGTM! Proper join with release windows table.The left join with environmentPolicyReleaseWindow is correctly implemented to fetch associated release windows.
packages/db/drizzle/0065_strange_the_hunter.sql (2)
2-6: LGTM on foreign key constraint.The foreign key constraint with CASCADE delete and NO ACTION update is well-designed:
- CASCADE delete ensures referential integrity by automatically removing policies when an environment is deleted
- NO ACTION update is appropriate as UUID primary keys shouldn't change
1-6: Verify data migration strategy.The migration adds a new required column but doesn't specify how existing rows will be populated with valid environment IDs. Please ensure there's a strategy to handle existing data:
- Is there a data migration script?
- Are there any existing rows in the
environment_policytable?Run this script to check for existing rows that need migration:
✅ Verification successful
Migration follows safe schema evolution pattern
The migration is safe as it:
- Adds a nullable column, allowing existing rows to remain valid
- Includes proper foreign key constraints for data integrity
- Follows the recommended pattern for additive schema changes
This approach enables gradual data population without requiring immediate updates to existing rows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if environment_policy table has existing rows echo "SELECT COUNT(*) FROM environment_policy;" | psql -tLength of output: 154
Script:
#!/bin/bash # Find all migration files related to environment_policy table rg -l "environment_policy" packages/db/drizzle/ # Show content of these files to understand the context echo "=== Content of migration files ===" for file in $(rg -l "environment_policy" packages/db/drizzle/); do echo -e "\n=== $file ===\n" cat "$file" doneLength of output: 94671
Script:
#!/bin/bash # Check content of the previous migration cat packages/db/drizzle/0064_misty_stryfe.sqlLength of output: 157
packages/db/drizzle/meta/_journal.json (1)
460-465: LGTM on migration journal entry.The new entry maintains consistency with:
- Correct sequential index (65)
- Matching version number (7)
- Chronological timestamp
- Standard naming pattern
| const onCreate = (data: SCHEMA.UpdateEnvironmentPolicy) => | ||
| createPolicy | ||
| .mutateAsync({ | ||
| environmentId: environment.id, | ||
| ...environment, | ||
| ...data, | ||
| }) | ||
| .then(invalidateQueries); |
There was a problem hiding this comment.
💡 Codebase verification
Unnecessary environment object spread should be removed
The current implementation spreads the entire environment object unnecessarily when only the environmentId is needed. This deviates from the established patterns in the codebase where other components use a clean separation of concerns.
const onCreate = (data: SCHEMA.UpdateEnvironmentPolicy) =>
createPolicy
.mutateAsync({
environmentId: environment.id,
- ...environment,
...data,
})
.then(invalidateQueries);🔗 Analysis chain
Review object spread order in onCreate function.
The current implementation spreads the environment object before the data object, which means properties from data could potentially override important environment properties unintentionally.
- .mutateAsync({
- environmentId: environment.id,
- ...environment,
- ...data,
- })
+ .mutateAsync({
+ environmentId: environment.id,
+ ...data,
+ ...environment,
+ })🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find UpdateEnvironmentPolicy type definition
ast-grep --pattern 'type UpdateEnvironmentPolicy = $_'
# Search for similar mutation patterns with environment
rg "mutateAsync.*environment" -A 2 -B 2
Length of output: 3724
Script:
#!/bin/bash
# Get the schema definition
cat packages/db/src/schema/environment.ts
# Find related policy mutations
rg "createPolicy|updatePolicy" -A 5 -B 5
Length of output: 24892
| disabled={environmentPolicy.successType !== "some"} | ||
| type="number" | ||
| value={environmentPolicy.successMinimum} | ||
| onChange={(e) => { | ||
| const value = e.target.valueAsNumber; | ||
| const successMinimum = Number.isNaN(value) ? 0 : value; | ||
| onUpdate({ successMinimum }); | ||
| }} | ||
| className="h-6 w-16 text-xs" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for minimum success count.
The number input for minimum successful deployments lacks validation. Consider adding min/max constraints and handling invalid inputs more robustly.
<Input
disabled={environmentPolicy.successType !== "some"}
type="number"
+ min={0}
+ max={999}
value={environmentPolicy.successMinimum}
onChange={(e) => {
const value = e.target.valueAsNumber;
- const successMinimum = Number.isNaN(value) ? 0 : value;
+ const successMinimum = Number.isNaN(value) ? 0 : Math.max(0, Math.min(999, value));
onUpdate({ successMinimum });
}}
className="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.
| disabled={environmentPolicy.successType !== "some"} | |
| type="number" | |
| value={environmentPolicy.successMinimum} | |
| onChange={(e) => { | |
| const value = e.target.valueAsNumber; | |
| const successMinimum = Number.isNaN(value) ? 0 : value; | |
| onUpdate({ successMinimum }); | |
| }} | |
| className="h-6 w-16 text-xs" | |
| disabled={environmentPolicy.successType !== "some"} | |
| type="number" | |
| min={0} | |
| max={999} | |
| value={environmentPolicy.successMinimum} | |
| onChange={(e) => { | |
| const value = e.target.valueAsNumber; | |
| const successMinimum = Number.isNaN(value) ? 0 : Math.max(0, Math.min(999, value)); | |
| onUpdate({ successMinimum }); | |
| }} | |
| className="h-6 w-16 text-xs" |
| releaseWindows: _.chain(rows) | ||
| .map((r) => r.environment_policy_release_window) | ||
| .filter(isPresent) | ||
| .uniqBy((r) => r.id) | ||
| .value(), | ||
| isOverride: | ||
| env.environment_policy.environmentId === env.environment.id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding date transformation for release windows.
The release windows are added to the policy object, but unlike in the byId query of the policy router, the dates aren't being transformed to Date objects here.
releaseWindows: _.chain(rows)
.map((r) => r.environment_policy_release_window)
.filter(isPresent)
.uniqBy((r) => r.id)
+ .map((r) => ({
+ ...r,
+ startTime: new Date(r.startTime),
+ endTime: new Date(r.endTime),
+ }))
.value(),📝 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: _.chain(rows) | |
| .map((r) => r.environment_policy_release_window) | |
| .filter(isPresent) | |
| .uniqBy((r) => r.id) | |
| .value(), | |
| isOverride: | |
| env.environment_policy.environmentId === env.environment.id, | |
| releaseWindows: _.chain(rows) | |
| .map((r) => r.environment_policy_release_window) | |
| .filter(isPresent) | |
| .uniqBy((r) => r.id) | |
| .map((r) => ({ | |
| ...r, | |
| startTime: new Date(r.startTime), | |
| endTime: new Date(r.endTime), | |
| })) | |
| .value(), | |
| isOverride: | |
| env.environment_policy.environmentId === env.environment.id, |
| @@ -0,0 +1,6 @@ | |||
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;--> statement-breakpoint | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding NOT NULL constraint.
The new environment_id column should likely be non-nullable since every policy should be associated with an environment. Consider modifying the column definition:
-ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;
+ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid NOT NULL;📝 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.
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;--> statement-breakpoint | |
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid NOT NULL;--> statement-breakpoint |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts (2)
5-17: Consider adding error handling for query invalidation failures.The function should handle potential failures during query invalidation to prevent silent failures and improve debugging.
Consider wrapping the invalidations in a try-catch block:
const invalidateQueries = () => { + try { utils.environment.policy.byId.invalidate(policyId); utils.environment.policy.bySystemId.invalidate(systemId); utils.environment.byId.invalidate(environmentId); + } catch (error) { + console.error('Failed to invalidate queries:', error); + throw error; + } };
30-36: Add error handling for failed mutations.The mutation chain should handle potential failures to prevent unhandled promise rejections and improve error reporting.
Consider adding error handling:
const onUpdate = (data: SCHEMA.UpdateEnvironmentPolicy) => updatePolicy .mutateAsync({ id: policy.id, data, }) - .then(invalidateQueries); + .then(invalidateQueries) + .catch((error) => { + console.error('Failed to update policy:', error); + throw error; + });packages/db/drizzle/0065_strange_the_hunter.sql (1)
8-17: Optimize backfill query and add transaction safety.Consider these improvements to the backfill operation:
- Use NOT EXISTS or LEFT JOIN instead of NOT IN for better performance
- Wrap the operation in a transaction for atomicity
Here's an optimized version:
+BEGIN; INSERT INTO "environment_policy" ("name", "environment_id", "system_id") SELECT e."name" AS "name", e."id" AS "environment_id", e."system_id" FROM "environment" e -WHERE e."id" NOT IN ( - SELECT "environment_id" FROM "environment_policy" -); +WHERE NOT EXISTS ( + SELECT 1 FROM "environment_policy" ep + WHERE ep."environment_id" = e."id" +); +COMMIT;apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (1)
156-157: Consider using a more specific variable name.While the destructuring is correct, the variable name
filtercould be more specific to match the property namereleaseFilterfor better clarity and consistency.- const { releaseFilter: filter } = envReleaseChannel ?? - policyReleaseChannel ?? { releaseFilter: null }; + const { releaseFilter } = envReleaseChannel ?? + policyReleaseChannel ?? { releaseFilter: null };This change would make the code more consistent with the schema update and improve readability. Remember to update all references to
filterin the subsequent code accordingly.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx (2)
20-25: Consider adding validation for required props.The props interface is well-defined, but consider adding runtime validation for required props to prevent undefined errors.
+import { z } from "zod"; +const propsSchema = z.object({ + environment: z.any(), // Replace with specific SCHEMA.Environment shape + environmentPolicy: z.any(), // Replace with specific Policy shape + activeTab: z.nativeEnum(EnvironmentDrawerTab), + deployments: z.array(z.any()), // Replace with specific Deployment shape +}); export const UpdateOverridePolicy: React.FC<UpdateOverridePolicyProps> = ({ environment, environmentPolicy, activeTab, deployments, -}) => { +}) => { + propsSchema.parse({ environment, environmentPolicy, activeTab, deployments });
38-77: Consider memoizing the conditional renders.The component has multiple conditional renders that could benefit from memoization to prevent unnecessary re-renders.
+import { useMemo } from 'react'; export const UpdateOverridePolicy: React.FC<UpdateOverridePolicyProps> = ({ environment, environmentPolicy, activeTab, deployments, }) => { const { onUpdate, isUpdating } = useUpdateOverridePolicy( environment, environmentPolicy, ); + const renderContent = useMemo(() => { + switch (activeTab) { + case EnvironmentDrawerTab.Approval: + return ( + <ApprovalAndGovernance + environmentPolicy={environmentPolicy} + onUpdate={onUpdate} + isLoading={isUpdating} + /> + ); + // Add other cases... + } + }, [activeTab, environmentPolicy, onUpdate, isUpdating]); - return ( - <> - {activeTab === EnvironmentDrawerTab.Approval && ( - <ApprovalAndGovernance - environmentPolicy={environmentPolicy} - onUpdate={onUpdate} - isLoading={isUpdating} - /> - )} - // ... other conditions - </> - ); + return <>{renderContent}</>;apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx (1)
111-114: Consider adding optimistic updates.The update mechanism could benefit from optimistic updates to improve the user experience.
+import { useState } from 'react'; +type OptimisticState = Record<string, string | null>; export const ReleaseChannels: React.FC<ReleaseChannelProps> = ({ environmentPolicy, deployments, isLoading, onUpdate, }) => { + const [optimisticState, setOptimisticState] = useState<OptimisticState>({}); const updateReleaseChannel = async ( deploymentId: string, channelId: string | null, ) => { + setOptimisticState(prev => ({ ...prev, [deploymentId]: channelId })); try { await onUpdate({ releaseChannels: { ...currReleaseChannels, [deploymentId]: channelId }, }); + } catch (error) { + setOptimisticState(prev => { + const { [deploymentId]: _, ...rest } = prev; + return rest; + }); + throw error; } };apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (1)
101-101: Consider adding a loading state for policy check.The policy override check might need a loading state to prevent UI flicker.
- const isUsingOverridePolicy = environment?.policy.isOverride ?? false; + const isPolicyLoading = environmentQ.isLoading; + const isUsingOverridePolicy = !isPolicyLoading && (environment?.policy.isOverride ?? false);packages/api/src/router/environment.ts (1)
125-128: Enhance error message with more context.The error message could be more specific about which environment is missing a policy.
- "No policy found on environment, this should never happen. Please contact support.", + `No policy found on environment ${input}. This should never happen. Please contact support.`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/useReleaseChannel.ts(1 hunks)packages/api/src/router/environment.ts(5 hunks)packages/db/drizzle/0065_strange_the_hunter.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx
🧰 Additional context used
📓 Path-based instructions (8)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.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]/useReleaseChannel.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-drawer/policy-override/useOverridePolicy.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/policy-form-components/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.
packages/api/src/router/environment.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.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-drawer/EnvironmentDrawer.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.
📓 Learnings (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50
Timestamp: 2024-11-27T23:16:35.580Z
Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:55-57
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, we prefer to fetch all data upfront and show a loading indicator until everything is fetched, unless performance becomes an issue.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
🔇 Additional comments (14)
packages/db/drizzle/0065_strange_the_hunter.sql (2)
1-1: Consider adding NOT NULL constraint.The new
environment_idcolumn should likely be non-nullable since every policy should be associated with an environment. Consider modifying the column definition:-ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid; +ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid NOT NULL;
2-6: Verify CASCADE delete behavior.The foreign key constraint uses
ON DELETE cascade, which means when an environment is deleted, all associated policies will be automatically deleted. Please ensure:
- The application code handles this cascading deletion gracefully
- Users are properly warned before deleting environments
- Any necessary audit logging is in place to track policy deletions
✅ Verification successful
Cascade delete behavior is properly handled through UI confirmations
The codebase implements proper safeguards through confirmation dialogs before environment deletion. However, consider adding audit logging to track environment and policy deletions for better traceability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for environment deletion handling in the codebase echo "Searching for environment deletion handling..." rg -A 5 "deleteEnvironment|removeEnvironment|destroyEnvironment" echo "Searching for related audit logging..." rg "audit|log.*environment.*delet|track.*environment"Length of output: 12597
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (1)
51-54: Verify schema changes and consider refactoring the filter lookup.The change from
.filterto.releaseFiltersuggests a schema update. While the implementation is correct, we could improve readability by extracting the lookup logic.- const releaseFilter = - environment?.releaseChannels.find((rc) => rc.deploymentId === deploymentId) - ?.releaseFilter ?? - environment?.policy.releaseChannels.find( - (rc) => rc.deploymentId === deploymentId, - )?.releaseFilter ?? - undefined; + const findReleaseChannel = (channels?: Array<{ deploymentId: string; releaseFilter?: unknown }>) => + channels?.find((rc) => rc.deploymentId === deploymentId)?.releaseFilter; + + const releaseFilter = + findReleaseChannel(environment?.releaseChannels) ?? + findReleaseChannel(environment?.policy?.releaseChannels) ?? + undefined;Let's verify the schema changes:
✅ Verification successful
Schema changes are correctly implemented across the codebase.
The change from
.filterto.releaseFilteris consistent with the database schema and type definitions. All usages across the codebase follow this pattern, and no legacy references were found. The implementation is type-safe and complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the schema changes from 'filter' to 'releaseFilter' # Look for any remaining uses of the old 'filter' property # Search for old filter property usage echo "Searching for old filter property usage..." rg -t typescript -t tsx "\.filter\s+[?]{2}|\.filter\s*[:=]" --no-heading # Search for new releaseFilter property usage echo "Searching for new releaseFilter property usage..." rg -t typescript -t tsx "\.releaseFilter\s+[?]{2}|\.releaseFilter\s*[:=]" --no-headingLength of output: 437
Script:
#!/bin/bash # Add TypeScript file type definition for ripgrep RG_ARGS="--type-add 'ts:*.{ts,tsx}' --type ts" echo "Searching for type definitions with releaseFilter..." rg $RG_ARGS "type.*\{.*releaseFilter.*\}" -A 2 -B 2 echo -e "\nSearching for releaseFilter property usage..." rg $RG_ARGS "releaseFilter[^(]" -A 1 -B 1 echo -e "\nSearching for old filter property usage..." rg $RG_ARGS "\.filter\s*[:=]|\.filter\s*\?{2}" -A 1 -B 1Length of output: 822
Script:
#!/bin/bash echo "Searching for releaseFilter property usage..." rg -g "*.{ts,tsx}" "releaseFilter" -A 1 -B 1 echo -e "\nSearching for potential old filter property usage..." rg -g "*.{ts,tsx}" "\.filter\s*[?:=]" -A 1 -B 1 echo -e "\nSearching for type definitions..." rg -g "*.{ts,tsx}" "interface.*ReleaseChannel|type.*ReleaseChannel" -A 3 -B 1Length of output: 37482
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/useReleaseChannel.ts (1)
27-28: Verify the default value for releaseFilter.The destructuring with a default value of
nullforreleaseFilteris correct, but let's verify that this aligns with how the value is used throughout the codebase.✅ Verification successful
The default value of
nullforreleaseFilteris correctly implemented.The codebase consistently handles
nullvalues forreleaseFilteracross UI components, API layer, and database operations, confirming this is the intended behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of releaseFilter to verify null handling rg -A 5 'releaseFilter.*null'Length of output: 9587
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx (2)
12-14: LGTM! Well-defined types using NonNullable.The type definitions are well-structured, using NonNullable to ensure type safety for the Policy type.
Also applies to: 16-18
33-36: LGTM! Proper hook usage.The
useUpdateOverridePolicyhook is correctly used with the required dependencies.apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx (1)
18-18: LGTM! Simplified Policy type.The Policy type has been correctly simplified to focus only on releaseChannels.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (2)
46-47: LGTM! Type-safe tab handling.The type casting of the tab parameter is handled safely with proper null checking.
138-192: LGTM! Tab implementation follows learned patterns.The implementation of tab buttons aligns with the learned preferences from PR #188, keeping individual definitions for a small number of tabs.
packages/api/src/router/environment.ts (5)
24-24: LGTM!The import of
environmentPolicyReleaseWindowis correctly placed and follows the project's naming conventions.
68-68: LGTM!The addition of
releaseChannelDescriptionto both subqueries maintains consistency and follows the established naming pattern.Also applies to: 84-84
103-106: LGTM!The left join for
environmentPolicyReleaseWindowis correctly implemented, using the appropriate join type and condition.
144-148: Consider adding date transformation for release windows.The release windows are added to the policy object, but the dates aren't being transformed to Date objects.
releaseWindows: _.chain(rows) .map((r) => r.environment_policy_release_window) .filter(isPresent) .uniqBy((r) => r.id) + .map((r) => ({ + ...r, + startTime: new Date(r.startTime), + endTime: new Date(r.endTime), + })) .value(),
159-160: LGTM!The addition of the description field to release channels mapping is consistent with the policy object structure.
| @@ -147,14 +147,14 @@ const ReleaseChannelCheck: React.FC<EnvironmentNodeProps["data"]> = ({ | |||
| (rc) => rc.deploymentId === deploymentId, | |||
| ); | |||
|
|
|||
| const policyReleaseChannel = environment.data?.policy?.releaseChannels.find( | |||
| const policyReleaseChannel = environment.data?.policy.releaseChannels.find( | |||
There was a problem hiding this comment.
Add null check for policy access.
The removal of optional chaining from policy access could lead to runtime errors if policy is undefined on environment.data.
- const policyReleaseChannel = environment.data?.policy.releaseChannels.find(
+ const policyReleaseChannel = environment.data?.policy?.releaseChannels.find(📝 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 policyReleaseChannel = environment.data?.policy.releaseChannels.find( | |
| const policyReleaseChannel = environment.data?.policy?.releaseChannels.find( |
| @@ -20,12 +20,12 @@ export const useReleaseChannel = ( | |||
| const envReleaseChannel = environment.data?.releaseChannels.find( | |||
| (rc) => rc.deploymentId === deploymentId, | |||
| ); | |||
| const policyReleaseChannel = environment.data?.policy?.releaseChannels.find( | |||
| const policyReleaseChannel = environment.data?.policy.releaseChannels.find( | |||
There was a problem hiding this comment.
Add optional chaining for policy access.
The direct access to policy.releaseChannels could cause runtime errors if policy is undefined. Add optional chaining to ensure safe access.
- const policyReleaseChannel = environment.data?.policy.releaseChannels.find(
+ const policyReleaseChannel = environment.data?.policy?.releaseChannels?.find(📝 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 policyReleaseChannel = environment.data?.policy.releaseChannels.find( | |
| const policyReleaseChannel = environment.data?.policy?.releaseChannels?.find( |
| type ReleaseChannelProps = { | ||
| environmentPolicy: Policy; | ||
| deployments: Deployment[]; | ||
| isLoading: boolean; | ||
| onUpdate: (data: SCHEMA.UpdateEnvironmentPolicy) => Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Promise-based updates.
The Promise-based update functions should include error handling to manage failed updates gracefully.
type ReleaseChannelProps = {
environmentPolicy: Policy;
deployments: Deployment[];
isLoading: boolean;
- onUpdate: (data: SCHEMA.UpdateEnvironmentPolicy) => Promise<void>;
+ onUpdate: (data: SCHEMA.UpdateEnvironmentPolicy) => Promise<void>;
+ onError?: (error: Error) => void;
};
const updateReleaseChannel = (
deploymentId: string,
channelId: string | null,
-) =>
- onUpdate({
- releaseChannels: { ...currReleaseChannels, [deploymentId]: channelId },
- });
+) => {
+ try {
+ return onUpdate({
+ releaseChannels: { ...currReleaseChannels, [deploymentId]: channelId },
+ });
+ } catch (error) {
+ onError?.(error as Error);
+ return Promise.reject(error);
+ }
+};Also applies to: 33-33
| {environment.policy.isOverride && ( | ||
| <UpdateOverridePolicy | ||
| environment={environment} | ||
| environmentPolicy={environment.policy} | ||
| activeTab={tab ?? EnvironmentDrawerTab.Approval} | ||
| deployments={deployments ?? []} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for policy override component.
Consider wrapping the UpdateOverridePolicy component with an error boundary to handle potential rendering errors gracefully.
+import { ErrorBoundary } from 'react-error-boundary';
+const PolicyErrorFallback = () => (
+ <div className="p-4 text-red-500">
+ Failed to load policy settings. Please try again.
+ </div>
+);
{environment.policy.isOverride && (
+ <ErrorBoundary FallbackComponent={PolicyErrorFallback}>
<UpdateOverridePolicy
environment={environment}
environmentPolicy={environment.policy}
activeTab={tab ?? EnvironmentDrawerTab.Approval}
deployments={deployments ?? []}
/>
+ </ErrorBoundary>
)}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/db/src/schema/environment.ts (4)
133-133: Use type-safe null default.Instead of using
sql\NULL``, use the built-in nullable column definition.- concurrencyLimit: integer("concurrency_limit").default(sql`NULL`), + concurrencyLimit: integer("concurrency_limit"),
137-146: Consider using a more appropriate type for time durations.Using
bigintfor time durations might lead to confusion. Consider:
- Using a more semantic type (e.g., interval in PostgreSQL)
- Adding validation to ensure positive values
- Adding documentation about the time unit (milliseconds)
176-176: Improve type safety of release channels schema.The current schema uses a record with nullable UUID values, which might be too permissive.
Consider using a more structured type:
- releaseChannels: z.record(z.string().uuid().nullable()).optional(), + releaseChannels: z + .array( + z.object({ + channelId: z.string().uuid(), + enabled: z.boolean(), + }), + ) + .optional(),
198-245: Add performance-optimizing indexes.Consider adding the following indexes to improve query performance:
- Index on
startTimeandendTimefor efficient release window lookups- Index on
statusandapprovedAtfor filtering approvals}, + (t) => ({ + timeIdx: index("release_window_time_idx").on(t.startTime, t.endTime), + }),(t) => ({ - uniq: uniqueIndex().on(t.policyId, t.releaseId) + uniq: uniqueIndex().on(t.policyId, t.releaseId), + statusIdx: index("approval_status_idx").on(t.status, t.approvedAt), }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/api/src/router/environment.ts(5 hunks)packages/db/drizzle/0065_familiar_jazinda.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/drizzle/meta/_journal.json
- packages/api/src/router/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/db/src/schema/environment.ts (1)
96-109: LGTM! Well-defined enums with clear values.The enum definitions follow consistent naming and value conventions.
packages/db/drizzle/0065_familiar_jazinda.sql (2)
18-22: Add validation after policy updates.Consider adding a validation check after the UPDATE to ensure all environments have been assigned a policy before making the column non-nullable.
UPDATE "environment" SET "policy_id" = ep."id" FROM "environment_policy" ep WHERE "environment"."id" = ep."environment_id" AND "environment"."policy_id" IS NULL; + +-- Verify all environments have a policy +DO $$ +BEGIN + IF EXISTS (SELECT 1 FROM "environment" WHERE "policy_id" IS NULL) THEN + RAISE EXCEPTION 'Some environments are missing policies'; + END IF; +END $$;
1-24: Verify current database state before applying migration.Before applying this migration, we should verify the current state of environment-policy relationships to ensure the migration will succeed.
✅ Verification successful
Migration is safe to proceed
The migration safely establishes a bidirectional relationship between environments and policies with proper backfill strategy:
- Adds environment_id with ON DELETE CASCADE (consistent with existing policy_id behavior)
- Backfills data without affecting existing relationships
- Makes policy_id NOT NULL only after ensuring all environments have policies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check current environment-policy relationships # Connect to database and run checks psql -c " -- Check for environments without policies SELECT COUNT(*) as environments_without_policies FROM environment WHERE policy_id IS NULL; -- Check for duplicate environment-policy relationships SELECT environment_id, COUNT(*) FROM environment_policy GROUP BY environment_id HAVING COUNT(*) > 1; -- Check for orphaned policies SELECT COUNT(*) as orphaned_policies FROM environment_policy ep LEFT JOIN environment e ON ep.environment_id = e.id WHERE e.id IS NULL; "Length of output: 576
Script:
#!/bin/bash # Check for related migration files and schema fd . packages/db/drizzle/ -e sql | sortLength of output: 3118
Script:
#!/bin/bash # Search for migrations containing environment or policy table definitions/modifications rg -l "CREATE TABLE.*environment|ALTER TABLE.*environment|CREATE TABLE.*environment_policy|ALTER TABLE.*environment_policy" packages/db/drizzle/Length of output: 985
Script:
#!/bin/bash # Check content of key migrations echo "=== Initial table creation (0000) ===" cat packages/db/drizzle/0000_busy_firebird.sql echo -e "\n=== Recent modifications (0063, 0064) ===" cat packages/db/drizzle/0063_mute_william_stryker.sql cat packages/db/drizzle/0064_misty_stryfe.sqlLength of output: 39838
| policyId: uuid("policy_id").references(() => environmentPolicy.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| policyId: uuid("policy_id").notNull(), |
There was a problem hiding this comment.
Fix inconsistent null handling and circular dependency.
There are two issues in the environment table definition:
- The
policyIdis marked asnotNullbut the foreign key'sonDeleteis set to "set null", which would violate the not-null constraint. - The foreign key references
environmentPolicybefore it's declared, creating a circular dependency.
Suggested fixes:
- Either make
policyIdnullable or change theonDeletebehavior to "cascade" - Move the foreign key constraint to after the
environmentPolicytable declaration
- policyId: uuid("policy_id").notNull(),
+ policyId: uuid("policy_id"),Or if the policy should never be deleted:
- }).onDelete("set null"),
+ }).onDelete("cascade"),Also applies to: 48-51
| environmentId: uuid("environment_id").references( | ||
| (): any => environment.id, | ||
| { onDelete: "cascade" }, | ||
| ), |
There was a problem hiding this comment.
Remove type cast to any.
Using any bypasses TypeScript's type checking. The reference to environment.id should be properly typed.
- (): any => environment.id,
+ () => environment.id,Committable suggestion skipped: line range outside the PR's diff.
| export const environmentPolicyApproval = pgTable( | ||
| "environment_policy_approval", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| policyId: uuid("policy_id") | ||
| .notNull() | ||
| .references(() => environmentPolicy.id, { onDelete: "cascade" }), | ||
| releaseId: uuid("release_id") | ||
| .notNull() | ||
| .references(() => release.id, { onDelete: "cascade" }), | ||
| status: approvalStatusType("status").notNull().default("pending"), | ||
| userId: uuid("user_id").references(() => user.id, { onDelete: "set null" }), | ||
| approvedAt: timestamp("approved_at", { | ||
| withTimezone: true, | ||
| precision: 0, | ||
| }).default(sql`NULL`), | ||
| }, | ||
| (t) => ({ uniq: uniqueIndex().on(t.policyId, t.releaseId) }), | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Status transition validation needed in application layer
The codebase uses a Postgres enum approvalStatusType with states: "pending", "approved", "rejected", but lacks explicit transition validation. While the schema correctly defines the states, it's recommended to implement the validation at the application layer for better flexibility and error handling.
- Add validation in the service layer before status updates
- Consider tracking the transition history for audit purposes
- Implement proper error messages for invalid transitions
🔗 Analysis chain
Verify approval status transitions.
The current schema doesn't prevent invalid approval status transitions (e.g., from 'rejected' to 'approved'). Consider adding a trigger or application-level validation to enforce valid state transitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing approval status transition validation
rg -l "approval.*status.*transition" .
ast-grep --pattern 'function $FUNC_NAME($_) {
$$$
status $$$
$$$
}'
Length of output: 116
Script:
#!/bin/bash
# Search for approvalStatusType definition and usage
rg "approvalStatusType" -A 5 -B 5
# Search for any approval status validation patterns
rg -g "*.{ts,js}" "(validateApproval|checkApproval|updateApproval|handleApproval)" -A 5
# Look for enum or type definitions related to approval status
ast-grep --pattern 'type $NAME = "pending" | "approved" | $$$'
ast-grep --pattern 'enum $NAME { $$$ "pending" $$$ "approved" $$$ }'
Length of output: 1789
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;--> statement-breakpoint | ||
| DO $$ BEGIN | ||
| ALTER TABLE "environment_policy" ADD CONSTRAINT "environment_policy_environment_id_environment_id_fk" FOREIGN KEY ("environment_id") REFERENCES "public"."environment"("id") ON DELETE cascade ON UPDATE no action; | ||
| EXCEPTION | ||
| WHEN duplicate_object THEN null; | ||
| END $$; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making environment_id non-nullable.
While the foreign key constraint and error handling look good, the environment_id column should be non-nullable to prevent orphaned policies. This aligns with the apparent 1:1 relationship between environments and their policies.
-ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;
+ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid NOT NULL;📝 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.
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid;--> statement-breakpoint | |
| DO $$ BEGIN | |
| ALTER TABLE "environment_policy" ADD CONSTRAINT "environment_policy_environment_id_environment_id_fk" FOREIGN KEY ("environment_id") REFERENCES "public"."environment"("id") ON DELETE cascade ON UPDATE no action; | |
| EXCEPTION | |
| WHEN duplicate_object THEN null; | |
| END $$; | |
| ALTER TABLE "environment_policy" ADD COLUMN "environment_id" uuid NOT NULL;--> statement-breakpoint | |
| DO $$ BEGIN | |
| ALTER TABLE "environment_policy" ADD CONSTRAINT "environment_policy_environment_id_environment_id_fk" FOREIGN KEY ("environment_id") REFERENCES "public"."environment"("id") ON DELETE cascade ON UPDATE no action; | |
| EXCEPTION | |
| WHEN duplicate_object THEN null; | |
| END $$; |
| -- Backfill default environment policies for each environment | ||
| INSERT INTO "environment_policy" ("name", "environment_id", "system_id") | ||
| SELECT | ||
| e."name" AS "name", | ||
| e."id" AS "environment_id", | ||
| e."system_id" AS "system_id" | ||
| FROM "environment" e | ||
| LEFT JOIN environment_policy ep ON e.id = ep.environment_id | ||
| WHERE ep.environment_id IS NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap operations in an explicit transaction.
While the backfill logic is correct, these operations should be wrapped in an explicit transaction to prevent race conditions and ensure data consistency, especially since this migration involves multiple related operations.
+BEGIN;
-- Backfill default environment policies for each environment
INSERT INTO "environment_policy" ("name", "environment_id", "system_id")
SELECT
e."name" AS "name",
e."id" AS "environment_id",
e."system_id" AS "system_id"
FROM "environment" e
LEFT JOIN environment_policy ep ON e.id = ep.environment_id
WHERE ep.environment_id IS NULL;📝 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.
| -- Backfill default environment policies for each environment | |
| INSERT INTO "environment_policy" ("name", "environment_id", "system_id") | |
| SELECT | |
| e."name" AS "name", | |
| e."id" AS "environment_id", | |
| e."system_id" AS "system_id" | |
| FROM "environment" e | |
| LEFT JOIN environment_policy ep ON e.id = ep.environment_id | |
| WHERE ep.environment_id IS NULL; | |
| BEGIN; | |
| -- Backfill default environment policies for each environment | |
| INSERT INTO "environment_policy" ("name", "environment_id", "system_id") | |
| SELECT | |
| e."name" AS "name", | |
| e."id" AS "environment_id", | |
| e."system_id" AS "system_id" | |
| FROM "environment" e | |
| LEFT JOIN environment_policy ep ON e.id = ep.environment_id | |
| WHERE ep.environment_id IS NULL; |
| WHERE "environment"."id" = ep."environment_id" | ||
| AND "environment"."policy_id" IS NULL; | ||
|
|
||
| ALTER TABLE "environment" ALTER COLUMN "policy_id" SET NOT NULL;--> statement-breakpoint |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete the transaction after NOT NULL constraint.
After adding the NOT NULL constraint, complete the transaction to ensure all operations are atomic.
ALTER TABLE "environment" ALTER COLUMN "policy_id" SET NOT NULL;--> statement-breakpoint
+COMMIT;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/db/src/schema/environment.ts (2)
38-38:⚠️ Potential issueFix inconsistent null handling in foreign key constraint.
The
policyIdcolumn is marked asnotNull, but the foreign key'sonDeleteis set to "set null". This creates a constraint conflict that will cause runtime errors when deleting policies.Choose one of these approaches:
- Make
policyIdnullable:- policyId: uuid("policy_id").notNull(), + policyId: uuid("policy_id"),
- Or change the
onDeletebehavior:- }).onDelete("set null"), + }).onDelete("cascade"),Also applies to: 48-51
125-128:⚠️ Potential issueRemove type cast to
any.Using
anybypasses TypeScript's type checking. The reference toenvironment.idshould be properly typed.- (): any => environment.id, + () => environment.id,
🧹 Nitpick comments (4)
packages/db/src/create-env.ts (1)
28-34: Consider adding error handling for policy creation.The policy creation lacks explicit error handling. Consider wrapping it in a try-catch block to handle potential insertion failures gracefully.
- const overridePolicyId = await db - .insert(environmentPolicy) - .values({ name: input.name, systemId: input.systemId }) - .returning() - .then(takeFirst) - .then((policy) => policy.id); + let overridePolicyId: string; + try { + overridePolicyId = await db + .insert(environmentPolicy) + .values({ name: input.name, systemId: input.systemId }) + .returning() + .then(takeFirst) + .then((policy) => policy.id); + } catch (error) { + throw new Error(`Failed to create override policy: ${error.message}`); + }apps/webservice/src/app/api/v1/environments/route.ts (1)
53-62: Consider handling empty release channels array.The release channel query will execute even with an empty array. Consider adding an early return for efficiency.
+ if (releaseChannels.length === 0) { + return []; + } const channels = await tx .select() .from(schema.releaseChannel) .where(inArray(schema.releaseChannel.id, releaseChannels))apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx (1)
27-78: Consider enhancing error handling and loading state feedback.While the component structure is clean and well-organized, consider the following improvements:
- Add error handling for failed updates
- Provide visual feedback during the loading state
Here's a suggested implementation:
export const UpdateOverridePolicy: React.FC<UpdateOverridePolicyProps> = ({ environment, environmentPolicy, activeTab, deployments, }) => { - const { onUpdate, isUpdating } = useUpdateOverridePolicy( + const { onUpdate, isUpdating, error } = useUpdateOverridePolicy( environment, environmentPolicy, ); return ( <> + {error && ( + <div className="text-red-500 mb-4"> + Failed to update policy: {error.message} + </div> + )} + {isUpdating && ( + <div className="text-gray-500 mb-4"> + Updating policy... + </div> + )} {activeTab === EnvironmentDrawerTab.Approval && ( <ApprovalAndGovernance environmentPolicy={environmentPolicy} onUpdate={onUpdate} isLoading={isUpdating} /> )} // ... rest of the component </> ); };packages/db/src/schema/environment.ts (1)
100-113: Enhance type safety with const assertions for enums.Consider using const assertions to improve type safety and enable better TypeScript inference.
export const approvalRequirement = pgEnum( "environment_policy_approval_requirement", - ["manual", "automatic"], + ["manual", "automatic"] as const, ); export const environmentPolicyDeploymentSuccessType = pgEnum( "environment_policy_deployment_success_type", - ["all", "some", "optional"], + ["all", "some", "optional"] as const, ); export const releaseSequencingType = pgEnum("release_sequencing_type", [ - "wait", - "cancel", + "wait" as const, + "cancel" as const, ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.ts(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.tsx(3 hunks)apps/webservice/src/app/api/v1/environments/route.ts(2 hunks)packages/api/src/router/environment.ts(7 hunks)packages/api/src/router/system.ts(1 hunks)packages/db/src/create-env.ts(1 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/schema/environment.ts(6 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/environment.ts
🧰 Additional context used
📓 Path-based instructions (10)
packages/db/src/index.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/system.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/environments/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.
packages/job-dispatch/src/__test__/job-variables-deployment.test.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]/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.
packages/db/src/create-env.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-drawer/EnvironmentDrawer.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-drawer/policy-override/UpdateOverride.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/reactflow/edges.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
📓 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (2)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:55-57
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `EnvironmentDrawer.tsx`, we prefer to fetch all data upfront and show a loading indicator until everything is fetched, unless performance becomes an issue.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (17)
packages/db/src/index.ts (1)
9-9: LGTM!The new export statement follows the existing pattern and correctly exposes the create-env module.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.ts (3)
1-1: LGTM! Clean type import refactor.The namespace import improves code organization and reduces import clutter.
38-38: LGTM! Type updates are consistent.The type updates using the SCHEMA namespace are consistent across all function signatures.
Also applies to: 48-49
15-18: Verify the edge creation logic for standalone policies.The new logic for determining edge sources based on standalone policies needs verification to ensure it handles all edge cases correctly.
Run this script to analyze the usage patterns:
✅ Verification successful
Edge creation logic for standalone policies is correctly implemented
The implementation properly handles all edge cases:
- Correctly identifies environments using standalone policies
- Maintains consistent edge structure across different scenarios
- Properly manages source-target relationships
- Uses appropriate type safety with schema types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all policy-related edge creation calls rg -A 5 "createEdgesWhereEnvironmentHasNoPolicy" # Find all places where standalone policies are defined/used ast-grep --pattern 'standalonePolicies = $_'Length of output: 2889
Script:
#!/bin/bash # Get the complete implementation of the edge creation function ast-grep --pattern 'export const createEdgesWhereEnvironmentHasNoPolicy = $_' # Find all edge creation functions in the file rg -A 10 "export const create.*Edge" apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.ts # Get type definitions rg "type.*Edge" apps/webservice/src/app/[workspaceSlug]/(app)/_components/reactflow/edges.tsLength of output: 2376
Script:
#!/bin/bash # Get the complete file content cat apps/webservice/src/app/\[workspaceSlug\]/\(app\)/_components/reactflow/edges.ts # Find all files that might contain related edge types rg -l "Edge.*type|type.*Edge" --type typescriptLength of output: 1698
packages/db/src/create-env.ts (2)
54-57: Verify policy update atomicity.The final policy update should be verified to ensure it maintains data consistency if the transaction fails.
✅ Verification successful
Transaction handling is properly implemented ✅
The policy update is atomic as it's executed within a transaction context (Tx type). If any operation fails, the entire transaction will be rolled back, maintaining data consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar policy update patterns ast-grep --pattern 'update($environmentPolicy).set({ environmentId: $_ })'Length of output: 76
Script:
#!/bin/bash # Search for the file content to understand the transaction context cat packages/db/src/create-env.ts # Search for similar update patterns and transaction handling rg -A 5 -B 5 "update\(environmentPolicy\)" # Search for transaction patterns ast-grep --pattern 'transaction($db, async ($tx) { $$$ })'Length of output: 3276
Script:
#!/bin/bash # Check Tx type definition cat packages/db/src/common.js # Look for transaction usage patterns rg -B 5 -A 5 "type Tx = "Length of output: 644
42-49: Consider validating metadata keys.The metadata insertion could benefit from key validation to prevent invalid or harmful keys.
Run this script to check existing metadata key patterns:
✅ Verification successful
Basic validation exists, additional constraints optional
The codebase already implements fundamental validation through:
- Type validation (Zod schema ensuring string key-value pairs)
- Database constraints (non-null and unique key per environment)
Additional key validation could be added as an enhancement, but it's not critical for security or functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for metadata key patterns in the codebase rg -A 2 "environmentMetadata.*key"Length of output: 36
Script:
#!/bin/bash # Search for schema definitions and environment metadata echo "=== Schema Definition ===" rg -g '*.ts' -A 5 "SCHEMA.*environmentMetadata" echo -e "\n=== Environment Metadata Usage ===" rg -g '*.ts' "environmentMetadata" echo -e "\n=== Validation Patterns ===" rg -g '*.ts' -A 3 "validate.*metadata|metadata.*validate"Length of output: 1732
Script:
#!/bin/bash # Check the environment metadata schema definition echo "=== Environment Metadata Schema ===" cat packages/db/src/schema/environment.tsLength of output: 7505
apps/webservice/src/app/api/v1/environments/route.ts (1)
64-68: LGTM! Clean integration with createEnv.The createEnv integration properly handles all required parameters and maintains transaction context.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (1)
172-172: LGTM! Improved clarity by filtering standalone policies.The introduction of
standalonePoliciesand its consistent usage throughout the code makes the intent clearer and helps maintain the single source of truth for standalone policies.Also applies to: 181-185, 202-206
packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (1)
251-251: LGTM! Consistent policy ID updates across test cases.The
policyIdfield has been consistently updated to"0"across all test cases, maintaining the test integrity while aligning with the new policy management system.Also applies to: 320-320, 386-386, 452-452, 518-518, 614-614, 710-710
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/UpdateOverride.tsx (1)
1-25: LGTM! Well-structured type definitions.The type definitions are clear, well-organized, and make good use of TypeScript features for type safety.
packages/db/src/schema/environment.ts (3)
141-150: Well-documented time duration fields with appropriate types.Good use of:
bigintfor storing large millisecond values- Clear comments explaining the duration units
202-219: LGTM! Well-structured release window implementation.Good practices observed:
- Proper timezone handling with
withTimezone: true- Appropriate timestamp precision settings
- Well-defined foreign key relationships
231-249: Status transition validation needed in application layer.The schema correctly defines approval states but lacks explicit transition validation.
Run this script to check for existing transition validation:
#!/bin/bash # Check for existing approval status transition validation rg -l "approval.*status.*transition" . ast-grep --pattern 'function $FUNC_NAME($_) { $$$ status $$$ $$$ }'apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/EnvironmentDrawer.tsx (4)
46-48: LGTM! Good type safety improvement.The explicit typing of the tab parameter enhances type safety.
138-192: LGTM! TabButton implementation aligns with team preferences.The individual TabButton definitions follow the team's preferred approach for small sets of tabs, as indicated in the retrieved learnings.
194-209: LGTM! Clean implementation of non-override UI.Good separation of concerns between override and non-override cases, providing a simplified interface when needed.
223-230: Add error boundary for policy override component.Consider wrapping the UpdateOverridePolicy component with an error boundary to handle potential rendering errors gracefully.
+import { ErrorBoundary } from 'react-error-boundary'; +const PolicyErrorFallback = () => ( + <div className="p-4 text-red-500"> + Failed to load policy settings. Please try again. + </div> +); {environment.policy.isOverride && ( + <ErrorBoundary FallbackComponent={PolicyErrorFallback}> <UpdateOverridePolicy environment={environment} environmentPolicy={environment.policy} activeTab={tab} deployments={deployments ?? []} /> + </ErrorBoundary> )}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx (1)
92-102: 🛠️ Refactor suggestionAdd input validation for minimum success count.
The number input for minimum successful deployments lacks validation. Consider adding min/max constraints and handling invalid inputs more robustly.
<Input disabled={environmentPolicy.successType !== "some"} type="number" + min={0} + max={999} value={environmentPolicy.successMinimum} onChange={(e) => { const value = e.target.valueAsNumber; - const successMinimum = Number.isNaN(value) ? 0 : value; + const successMinimum = Number.isNaN(value) ? 0 : Math.max(0, Math.min(999, value)); onUpdate({ successMinimum }); }} className="h-6 w-16 text-xs" />
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/AppSidebarPopoverSystem.tsx (1)
60-68: Add null check and improve UX for policy links.The policy link implementation could benefit from the following improvements:
<SidebarMenuAction> <Link href={`/${workspace.slug}/systems/${system.data?.slug}/environments?environment_policy_id=${policyId}`} onClick={() => setActiveSidebarItem(null)} - className="flex items-center gap-1 " + className="flex items-center gap-1" > - <IconLock className="size-4 text-muted-foreground" /> + <TooltipProvider> + <Tooltip delayDuration={0}> + <TooltipTrigger asChild> + <IconLock className="size-4 text-muted-foreground" /> + </TooltipTrigger> + <TooltipContent> + View environment policy settings + </TooltipContent> + </Tooltip> + </TooltipProvider> </Link> </SidebarMenuAction>Consider these additional improvements:
- Add a null check for
policyIdto handle cases where no policy exists- Add loading and error states for better UX
- Remove the trailing space in the className
packages/job-dispatch/src/environment-creation.ts (1)
54-56: LGTM! Consider adding documentation.The priority chain for release channel selection is clear from the code, but it would be helpful to document this behavior for future maintainers.
Add a comment explaining the precedence:
+ // Find release channels with precedence: environment-specific > policy-defined const policyReleaseChannel = environmentPolicyReleaseChannels.find( (prc) => prc.deploymentId === deployment.id, );apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx (1)
20-24: Add JSDoc documentation for the component.Consider adding JSDoc documentation to describe the component's purpose, props, and usage examples. This will improve maintainability and help other developers understand how to use the component correctly.
+/** + * A component for managing approval and governance policies in deployments. + * @param {Object} props + * @param {Object} props.environmentPolicy - The current environment policy configuration + * @param {Function} props.onUpdate - Callback function to update the policy + * @param {boolean} props.isLoading - Loading state indicator + */ export const ApprovalAndGovernance: React.FC<ApprovalAndGovernanceProps> = ({ environmentPolicy, onUpdate, isLoading, }) => (packages/job-dispatch/src/resource/dispatch-resource.ts (1)
74-76: Consider optimizing release channel lookups.The current implementation uses
Array.find()for both environment and policy release channel lookups. For better performance with large datasets, consider using a Map for O(1) lookup complexity.Example optimization:
const { releaseChannels, policy, system } = environment; const { deployments } = system; const { environmentPolicyReleaseChannels } = policy; + const policyChannelMap = new Map( + environmentPolicyReleaseChannels.map(channel => [channel.deploymentId, channel]) + ); + const envChannelMap = new Map( + releaseChannels.map(channel => [channel.deploymentId, channel]) + ); const deploymentsWithReleaseFilter = deployments.map((deployment) => { - const envReleaseChannel = releaseChannels.find( - (erc) => erc.deploymentId === deployment.id, - ); - const policyReleaseChannel = environmentPolicyReleaseChannels.find( - (prc) => prc.deploymentId === deployment.id, - ); + const envReleaseChannel = envChannelMap.get(deployment.id); + const policyReleaseChannel = policyChannelMap.get(deployment.id);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/AppSidebarPopoverSystem.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(1 hunks)packages/api/src/router/environment-policy.ts(3 hunks)packages/job-dispatch/src/environment-creation.ts(2 hunks)packages/job-dispatch/src/resource/dispatch-resource.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/environment-policy.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/webservice/src/app/[workspaceSlug]/(app)/AppSidebarPopoverSystem.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.
packages/job-dispatch/src/environment-creation.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/resource/dispatch-resource.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/policy-form-components/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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ApprovalAndGovernance.tsx (2)
1-18: LGTM! Well-structured imports and type definitions.The imports are logically organized and the type definitions are comprehensive with proper TypeScript types.
25-114: LGTM! Well-organized UI with clear sections and proper loading state handling.The UI implementation is clean and user-friendly with:
- Clear section headings and descriptive text
- Proper loading state indicator
- Logical grouping of related controls
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (1)
54-57: Consider adding null check and error handling for the approval status query.The removal of the null check on
environment.policyIdcould lead to unnecessary API calls. Additionally, there's no error handling for failed queries.Please verify:
- Does the API endpoint handle null
policyIdgracefully?- Should we prevent the query when
policyIdis null?Consider adding error handling and the null check:
const { data: approval, isLoading: isApprovalLoading } = api.environment.policy.approval.statusByReleasePolicyId.useQuery({ releaseId: release.id, policyId: environment.policyId, + }, { + enabled: environment.policyId != null, + onError: (error) => { + console.error('Failed to fetch approval status:', error); + } });packages/job-dispatch/src/resource/dispatch-resource.ts (1)
69-69: Verify policy nullability handling.The direct destructuring of
environmentPolicyReleaseChannelsassumespolicyis always defined. Verify if this assumption holds true in all cases.Let's check the database schema and usage patterns:
✅ Verification successful
Direct policy destructuring is safe
The database schema enforces a NOT NULL constraint on environment's policy_id and auto-creates default policies, guaranteeing policy existence. Direct destructuring is appropriate here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if policy can be null and how it's handled elsewhere # Check schema definition rg -A 5 "environment.*policy" --type-add 'schema:*.{prisma,sql}' --type schema # Check other policy access patterns ast-grep --pattern 'const { $_ } = policy'Length of output: 21881
| @@ -33,8 +33,7 @@ export const createJobsForNewEnvironment = async ( | |||
|
|
|||
| const { releaseChannels: envReleaseChannels, system } = releaseChannels; | |||
| const { workspaceId, deployments } = system; | |||
| const policyReleaseChannels = | |||
| releaseChannels.policy?.environmentPolicyReleaseChannels ?? []; | |||
| const { environmentPolicyReleaseChannels } = releaseChannels.policy; | |||
There was a problem hiding this comment.
💡 Codebase verification
Align policy access pattern with established codebase convention
The current destructuring pattern differs from the safer approach used elsewhere in the codebase. Consider restructuring to match the pattern in dispatch-resource.ts:
const { policy } = releaseChannels;
const { environmentPolicyReleaseChannels } = policy;🔗 Analysis chain
Verify null safety for policy destructuring.
The direct destructuring of environmentPolicyReleaseChannels might cause runtime errors if releaseChannels.policy is undefined. Consider adding a fallback or null check.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type definitions and usages to verify null safety
ast-grep --pattern 'interface Environment {
$$$
policy?: {
$$$
environmentPolicyReleaseChannels?: $_
$$$
}
$$$
}'
Length of output: 135
Script:
#!/bin/bash
# Search for type definitions of releaseChannels
ast-grep --pattern 'type $_ReleaseChannels = {
$$$
}'
# Search for interface/type definitions containing 'policy'
ast-grep --pattern 'interface $_ {
policy: $_
$$$
}'
# Look for null checks or error handling around the destructuring
rg -B 3 -A 3 "const \{ environmentPolicyReleaseChannels \}"
Length of output: 1486
Script:
#!/bin/bash
# Find the file where releaseChannels is defined/imported
rg -l "releaseChannels.*policy" packages/job-dispatch/src/
# Check type definitions in the file
ast-grep --pattern 'type $_ = {
policy: $_
$$$
}'
Length of output: 281
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Improvements
Bug Fixes
Performance