fix: Set release channels for policy and env #183
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several new React components and modifies existing ones to enhance the management of environments and their associated functionalities. Key additions include the Changes
Possibly related PRs
Suggested reviewers
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: 22
🧹 Outside diff range and nitpick comments (17)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx (2)
93-99: Enhance accessibility and user experience.
The delete menu item could be more descriptive and accessible.
Consider these improvements:
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
- className="flex cursor-pointer items-center gap-2"
+ className="flex cursor-pointer items-center gap-2 text-destructive"
+ aria-label={`Delete environment ${environment.name}`}
>
<IconTrash className="h-4 w-4 text-red-500" />
- <span>Delete</span>
+ <span>Delete Environment</span>
</DropdownMenuItem>80-104: Consider expanding menu options for better functionality.
The dropdown menu currently only supports deletion. Consider adding more common environment management actions.
Some suggestions for additional menu items:
- Clone Environment
- Edit Environment
- View Environment Details
- Export Environment Configuration
Would you like me to provide an implementation for any of these features?
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1)
64-110: Enhance accessibility and user experience.
While the UI structure is good, consider these improvements:
- Add ARIA labels for better screen reader support
- Add tooltips for truncated text
Consider these enhancements:
- <span className="w-40 truncate">{d.name}</span>
+ <span
+ className="w-40 truncate"
+ title={d.name}
+ role="textbox"
+ aria-label={`Deployment name: ${d.name}`}
+ >
+ {d.name}
+ </span>Also consider adding aria-label to the Select component for better accessibility:
- <Select value={value} onValueChange={onChange}>
+ <Select
+ value={value}
+ onValueChange={onChange}
+ aria-label={`Release channel for deployment ${d.name}`}
+ >apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1)
87-149: Enhance accessibility and user experience.
The UI components need accessibility improvements and better handling of truncated text.
<div className="space-y-4">
- <Label>Release Channels</Label>
+ <Label htmlFor="release-channels">Release Channels</Label>
<div className="space-y-2">
<div className="flex items-center gap-2 text-sm text-muted-foreground">
<span className="w-40">Deployment</span>
<span className="w-72">Release Channel</span>
</div>
{deploymentsWithReleaseChannels.map((d) => (
<div key={d.id} className="flex items-center gap-2">
- <span className="w-40 truncate">{d.name}</span>
+ <span className="w-40 truncate" title={d.name}>{d.name}</span>
<Select
value={value}
onValueChange={onChange}
+ aria-label={`Select release channel for ${d.name}`}
>apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (2)
118-122: Add null check for systemId in query.
While the enabled condition prevents the query from running without environmentPolicy, the systemId access could be more defensive.
Consider using optional chaining:
- environmentPolicy?.systemId ?? "",
+ environmentPolicy?.systemId ?? null,
- { enabled: isOpen && environmentPolicy != null },
+ { enabled: isOpen && environmentPolicy?.systemId != null },176-181: Consider a more intuitive icon for Release Channels.
The current use of IconFilter might not be the most intuitive representation for release channels. Consider using an icon that better represents the concept of release channels or deployment paths.
Consider using IconGitBranch or IconArrowFork which might better represent the concept of different release channels.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (3)
Line range hint 47-86: Fix potential issues in connection validation logic
The connection validation has several areas for improvement:
- The cycle detection algorithm might miss cycles due to the visited set not being cleared between different paths
- Some code paths implicitly return undefined instead of false
- The node type compatibility checks could be more maintainable
Consider this improved implementation:
const useValidConnection = () => {
const { getNodes, getEdges, getNode } = useReactFlow();
return useCallback(
(connection: Connection) => {
const nodes = getNodes();
const edges = getEdges();
const source = getNode(connection.source ?? "");
const target = getNode(connection.target ?? "");
- const hasCycle = (node: Node, visited = new Set()) => {
+ const hasCycle = (node: Node, visited = new Set<string>()): boolean => {
if (visited.has(node.id)) return false;
visited.add(node.id);
for (const outgoer of getOutgoers(node, nodes, edges)) {
if (outgoer.id === connection.source) return true;
- if (hasCycle(outgoer, visited)) return true;
+ if (hasCycle(outgoer, new Set(visited))) return true;
}
+ return false;
};
if (target == null) return false;
if (source == null) return false;
if (target.id === connection.source) return false;
if (hasCycle(target)) return false;
- if (
- target.type === NodeType.Environment &&
- source.type === NodeType.Environment
- )
- return false;
- if (target.type === NodeType.Policy && source.type === NodeType.Policy)
- return false;
+ // Define valid connection types
+ const validConnections = {
+ [NodeType.Environment]: [NodeType.Policy, NodeType.Trigger],
+ [NodeType.Policy]: [NodeType.Environment],
+ [NodeType.Trigger]: [NodeType.Environment],
+ };
+
+ if (!validConnections[source.type]?.includes(target.type)) return false;
if (
target.type === NodeType.Environment &&
source.type === NodeType.Policy
)
return !isPresent(target.data.policyId);
return true;
},
[getNodes, getEdges, getNode],
);
};Line range hint 88-161: Improve connection handling robustness and maintainability
The connection handling logic needs several improvements:
- Remove the commented-out code at the bottom
- Add error handling for mutations
- Reduce duplication in edge filtering logic
Consider this improved implementation:
const useOnConnect = () => {
const { setEdges, getNode } = useReactFlow();
const env = api.environment.update.useMutation();
const policyDeployment = api.environment.policy.deployment.create.useMutation();
+
+ const filterEdges = (targetId: string, additionalFilter?: (e: Edge) => boolean) => (eds: Edge[]) => {
+ const filtered = eds.filter(e => e.target !== targetId);
+ return additionalFilter ? filtered.filter(additionalFilter) : filtered;
+ };
+
+ const handleMutation = async (mutation: Promise<unknown>, params: Connection, edgeFilter: (eds: Edge[]) => Edge[]) => {
+ try {
+ await mutation;
+ setEdges(eds => addEdge({ ...params, markerEnd }, edgeFilter(eds)));
+ } catch (error) {
+ console.error('Failed to update connection:', error);
+ // Consider adding user notification here
+ }
+ };
const onConnect = useCallback<OnConnect>(
async (params: Connection) => {
if (!isPresent(params.source) || !isPresent(params.target)) return;
const source = getNode(params.source);
const target = getNode(params.target);
+ if (!source || !target) return;
const isSettingEnvironmentPolicy =
source?.type === NodeType.Policy &&
target?.type === NodeType.Environment;
if (isSettingEnvironmentPolicy) {
- env.mutate({ id: target.id, data: { policyId: source.id } });
- setEdges((eds) =>
- addEdge(
- { ...params, markerEnd },
- eds.filter((e) => e.target !== target.id),
- ),
+ await handleMutation(
+ env.mutateAsync({ id: target.id, data: { policyId: source.id } }),
+ params,
+ filterEdges(target.id)
);
return;
}
// Similar changes for other conditions...
-
- // Remove commented code
- // const isSettingPolicyDeployment = ...
},
- [getNode, setEdges, env, policyDeployment],
+ [getNode, setEdges, env, policyDeployment, handleMutation, filterEdges],
);
return onConnect;
};Line range hint 163-284: Enhance component organization and performance
The component implementation could benefit from several improvements:
- The layout effect's dependency array is missing
onLayout - The delete functionality could be extracted to a custom hook
- Node click handler should be memoized
Consider these improvements:
+ const useDeleteHandler = (
+ selectedNodeId: string | null,
+ selectedEdgeId: string | null,
+ edges: Edge[],
+ handleEdgeDelete: (edge: Edge) => void,
+ setOpen: (open: boolean) => void
+ ) => {
+ const deletePressed = useKeyPress(["Delete", "Backspace"]);
+
+ useEffect(() => {
+ if (!deletePressed) return;
+ if (selectedNodeId != null) {
+ setOpen(true);
+ return;
+ }
+ if (selectedEdgeId == null) return;
+
+ const edge = edges.find((e) => e.id === selectedEdgeId);
+ if (edge == null) return;
+ handleEdgeDelete(edge);
+ }, [deletePressed, selectedNodeId, selectedEdgeId, edges, handleEdgeDelete, setOpen]);
+ };
export const EnvFlowBuilder: React.FC<{...}> = ({...}) => {
// ... existing code ...
- useEffect(() => {
- if (reactFlowInstance != null) onLayout();
- }, [reactFlowInstance]);
+ useEffect(() => {
+ if (reactFlowInstance != null) onLayout();
+ }, [reactFlowInstance, onLayout]);
- const deletePressed = useKeyPress(["Delete", "Backspace"]);
- useEffect(() => {
- if (!deletePressed) return;
- // ... delete handling logic ...
- }, [edges, deletePressed, selectedNodeId, selectedEdgeId, handleEdgeDelete, setOpen]);
+ useDeleteHandler(selectedNodeId, selectedEdgeId, edges, handleEdgeDelete, setOpen);
+ const handleNodeClick = useCallback(
+ (_: MouseEvent, node: Node) => {
+ if (node.type === NodeType.Environment) setEnvironmentId(node.id);
+ if (node.type === NodeType.Policy) setEnvironmentPolicyId(node.id);
+ setSelectedNodeId(node.id);
+ },
+ [setEnvironmentId, setEnvironmentPolicyId, setSelectedNodeId]
+ );
return (
<ReactFlow
// ... other props ...
- onNodeClick={(_, node) => {
- if (node.type === NodeType.Environment) setEnvironmentId(node.id);
- if (node.type === NodeType.Policy) setEnvironmentPolicyId(node.id);
- setSelectedNodeId(node.id);
- }}
+ onNodeClick={handleNodeClick}
>
// ... rest of the component
);
};apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
256-261: Consider extracting messages to constants for better maintainability.
While the message updates are correct, consider extracting these status messages to named constants. This would make it easier to maintain and potentially support internationalization in the future.
+const DEPLOYMENT_STATUS_MESSAGES = {
+ BLOCKED_BY_RELEASE_CHANNEL: "Blocked by release channel",
+ NO_TARGETS: "No targets",
+ NO_JOB_AGENT: "No job agent",
+} as const;
<div className="text-center text-xs text-muted-foreground/70">
{isBlockedByReleaseChannel
- ? "Blocked by release channel"
- : hasJobAgent
- ? "No targets"
- : "No job agent"}
+ ? DEPLOYMENT_STATUS_MESSAGES.BLOCKED_BY_RELEASE_CHANNEL
+ : hasJobAgent
+ ? DEPLOYMENT_STATUS_MESSAGES.NO_TARGETS
+ : DEPLOYMENT_STATUS_MESSAGES.NO_JOB_AGENT}
</div>packages/api/src/router/deployment.ts (1)
337-340: LGTM! The implementation correctly handles release channels.
The changes effectively:
- Use left join to handle deployments without release channels
- Filter out null values using isPresent
- Maintain consistent ordering by deployment name
However, consider adding an index on releaseChannel.deploymentId to optimize the join performance if not already present.
Also applies to: 342-342, 349-351
packages/api/src/router/release.ts (1)
Line range hint 495-542: Consider refactoring to reduce code duplication.
Both bySystem and byWorkspace procedures share similar query structure and differ mainly in their final where clause. Consider extracting the common query builder logic into a reusable function.
Here's a suggested refactor:
const getMetadataKeysBase = (db: typeof ctx.db) =>
db
.selectDistinct({ key: releaseMetadata.key })
.from(release)
.innerJoin(releaseMetadata, eq(releaseMetadata.releaseId, release.id))
.innerJoin(deployment, eq(release.deploymentId, deployment.id));
const metadataKeys = createTRPCRouter({
bySystem: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser.perform(Permission.ReleaseGet).on({
type: "system",
id: input,
}),
})
.input(z.string().uuid())
.query(async ({ input, ctx }) =>
getMetadataKeysBase(ctx.db)
.where(eq(deployment.systemId, input))
.then((r) => r.map((row) => row.key)),
),
byWorkspace: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser.perform(Permission.ReleaseGet).on({
type: "workspace",
id: input,
}),
})
.input(z.string().uuid())
.query(async ({ input, ctx }) =>
getMetadataKeysBase(ctx.db)
.innerJoin(system, eq(deployment.systemId, system.id))
.where(eq(system.workspaceId, input))
.then((r) => r.map((row) => row.key)),
),
});apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (3)
Line range hint 143-164: Pass 'value' to form controls to ensure proper state management
In your FormField for targetFilter, you are only passing onChange to TargetViewsCombobox and TargetConditionRender, but not the value. This may cause the components to not reflect the current form state correctly.
Apply this diff to pass the value to your components:
- render={({ field: { onChange } }) => (
+ render={({ field: { onChange, value } }) => (
<FormItem>
<FormLabel>Target Filter</FormLabel>
<FormControl>
<>
<TargetViewsCombobox
workspaceId={workspaceId}
onChange={onChange}
+ value={value}
/>
<TargetConditionRender
- condition={targetFilter ?? defaultCondition}
+ condition={value ?? defaultCondition}
onChange={onChange}
/>
</>
</FormControl>
<FormMessage />
{form.formState.isDirty && (
<span className="text-xs text-muted-foreground">
Save to apply
</span>
)}
</FormItem>
)}Ensure that TargetViewsCombobox and TargetConditionRender accept and handle the value prop appropriately.
Line range hint 215-219: Handle potential undefined values when serializing 'targetFilter'
The form.getValues("targetFilter") might be undefined, which could cause JSON.stringify to throw an error. It's safer to provide a default empty object to prevent unexpected issues.
Apply this diff to ensure safe serialization:
<Link
href={`/${workspaceSlug}/targets?${new URLSearchParams({
filter: LZString.compressToEncodedURIComponent(
- JSON.stringify(form.getValues("targetFilter")),
+ JSON.stringify(form.getValues("targetFilter") ?? {}),
),
})}`}
className="flex items-center gap-1"
target="_blank"
rel="noopener noreferrer"
>Line range hint 46-48: Simplify the 'enabled' condition in the query
The enabled condition workspaceId !== "" may be unnecessary if workspaceId is always provided. This can simplify your code and avoid redundant checks.
Consider removing the enabled option or adjusting it if there's a possibility of workspaceId being an empty string.
packages/api/src/router/environment.ts (1)
302-315: Add braces to the if statement for clarity
In the updateReleaseChannels mutation (lines 302-315), the if statement lacks braces {}. While JavaScript allows single-line if statements without braces, adding them can improve readability and prevent potential errors if additional code is added in the future.
Modify the code as follows:
if (setChannels.length > 0) {
await db
.insert(environmentReleaseChannel)
.values(setChannels)
.onConflictDoUpdate({
target: [
environmentReleaseChannel.environmentId,
environmentReleaseChannel.deploymentId,
],
set: buildConflictUpdateColumns(environmentReleaseChannel, [
"channelId",
]),
});
}packages/api/src/router/environment-policy.ts (1)
399-402: Ensure proper handling of empty releaseChannels
When partitioning releaseChannels, if the input object is empty, both nulledIds and setChannels will be empty arrays. Ensure that subsequent database operations handle empty arrays without causing errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (6 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/layout.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/EnvFlowPanel.tsx (1 hunks)
- packages/api/src/router/deployment.ts (1 hunks)
- packages/api/src/router/environment-policy.ts (1 hunks)
- packages/api/src/router/environment.ts (3 hunks)
- packages/api/src/router/release.ts (1 hunks)
- packages/validators/src/auth/index.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/deployment.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
🔇 Additional comments (18)
apps/webservice/src/app/[workspaceSlug]/layout.tsx (1)
6-6: LGTM! Import path update aligns with project structure.
The updated import path follows the established pattern used by other drawer components, improving code organization and maintainability.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx (1)
1-21: LGTM! Well-structured imports and schema validation.
The schema validation constraints are appropriate for environment names and descriptions.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx (1)
1-25: LGTM! Dependencies are well-organized.
The imports are logically grouped and all necessary dependencies are properly imported.
packages/validators/src/auth/index.ts (1)
77-78: LGTM! The new permission follows established patterns.
The EnvironmentUpdate permission is correctly defined and follows the established naming convention of resource.action.
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (2)
1-24: LGTM! Well-structured imports and type definitions.
The types are properly defined with clear extensions of the schema types, and imports are appropriately granular.
26-29: Verify cleanup handling.
Please ensure that the component properly handles cleanup when unmounted, especially for pending mutations.
Let's check for similar components that might need cleanup:
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1)
1-38: LGTM! Well-structured type definitions and imports.
The type definitions are comprehensive and provide good type safety. The imports are well-organized and necessary.
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx (3)
33-33: LGTM: Clean type definition and import.
The new Deployment type correctly extends the schema type with releaseChannels, and the import is properly organized.
Also applies to: 63-65
47-51: LGTM: Improved URL handling.
The URL manipulation logic is now more robust using the URL API, ensuring consistent behavior across different URL formats.
81-83: Verify integration with ReleaseChannels component.
The conditional rendering looks correct, but let's verify the contract between components.
✅ Verification successful
Based on the search results, I can now generate the final response since we have found the implementation of both components and can verify their contract.
Integration with ReleaseChannels component is correct
The props contract between the parent EnvironmentPolicyDrawer and child ReleaseChannels component matches perfectly:
- Parent passes
policyanddeploymentsprops - Child component's implementation in
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsxaccepts these exact props and uses them correctly for:- Filtering deployments with release channels
- Managing release channel state
- Handling updates through the API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ReleaseChannels component props match the parent's provided props
# Test: Check ReleaseChannels component implementation
ast-grep --pattern 'const ReleaseChannels: React.FC<{ policy: $_; deployments: $_; }>'
Length of output: 88
Script:
#!/bin/bash
# Let's try a broader search for the ReleaseChannels component definition
ast-grep --pattern 'ReleaseChannels = $_'
# Also search for any file containing ReleaseChannels
fd ReleaseChannels
# And let's see the actual implementation using ripgrep for better context
rg "ReleaseChannels" -A 5
Length of output: 25703
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/EnvFlowPanel.tsx (2)
38-38: LGTM: Import path restructuring
The updated import path provides better organization by moving environment-related components to a dedicated directory.
38-38: Verify release channel implementation
The PR objectives mention fixing release channels for policy and environment, but I don't see any integration of release channels in the environment or policy creation flow. This might indicate missing implementation.
Let's verify if release channel handling exists in related files:
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/EnvFlowBuilder.tsx (1)
25-25: LGTM: Import path restructuring
The import path update aligns with the new component-based directory structure.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3)
207-208: LGTM! Clear and descriptive variable renaming.
The renaming from isBlockedByPolicyEvaluation to isBlockedByReleaseChannel better reflects the actual blocking mechanism being used.
215-216: LGTM! Logical condition ordering.
The canDeploy condition maintains a good order of checks, with the release channel block being the last check after verifying basic requirements like job agent and targets.
Line range hint 207-261: Verify consistent implementation of release channel blocking.
The changes look good, but let's verify that the release channel blocking mechanism is consistently implemented across related components.
✅ Verification successful
Release channel blocking implementation appears to be self-contained
The changes in DeploymentPageContent.tsx appear to be self-contained and properly implemented. The blocking logic is handled through the blockedEnvByRelease data structure and correctly displayed in the UI with appropriate messaging. Since we couldn't find any other instances of release channel blocking implementations in the codebase, this suggests that this is likely the primary (and possibly only) location where this logic needs to be implemented.
The implementation:
- Uses a data structure (
blockedEnvByRelease) to track blocked environments - Properly handles the UI state with
isBlockedByReleaseChannel - Shows appropriate messaging when blocked
- Integrates with the deployment flow by preventing deployment when blocked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of release channel blocking across components
# Test 1: Look for other components that might need similar updates
echo "Checking for related components that might need updates..."
rg -l "isBlockedByPolicyEvaluation|Blocked by policy" --type ts --type tsx
# Test 2: Verify the integration with ReleaseChannels component
echo "Checking ReleaseChannels component integration..."
rg -l "ReleaseChannels.*release.*channel" --type ts --type tsx
Length of output: 433
Script:
#!/bin/bash
# Let's try a different approach to search for release channel related code
# Search for release channel blocking logic
echo "Searching for release channel blocking implementations..."
rg -t typescript -t javascriptreact -t typescriptreact "blockedByRelease|isBlockedBy.*Release|Blocked by release"
# Search for ReleaseChannel component usage and definitions
echo "Searching for ReleaseChannel components and usage..."
rg -t typescript -t javascriptreact -t typescriptreact "ReleaseChannel"
# Search for environment blocking logic
echo "Searching for environment blocking patterns..."
rg -t typescript -t javascriptreact -t typescriptreact "blockedEnv.*Release|environmentId.*blocked"
Length of output: 737
Script:
#!/bin/bash
# Let's try with simpler file type flags and patterns
# Search for release channel blocking logic
echo "Searching for release channel blocking implementations..."
rg --type js "blockedByRelease|Blocked by release" -A 2 -B 2
# Search for ReleaseChannel component usage
echo "Searching for ReleaseChannel components..."
rg --type js "ReleaseChannel" -A 2 -B 2
# Look for files containing both release and block/blocked terms
echo "Searching for release blocking patterns..."
rg --type js "release.*block|block.*release" -i
Length of output: 467
packages/api/src/router/release.ts (1)
415-415: Verify the impact of modified grouping logic.
The grouping criteria has been changed from e.environment.id to [e.environment.id, e.release.id]. While this change provides more accurate grouping by treating each environment-release pair uniquely, it could affect downstream consumers expecting the old data structure.
Let's verify the usage of this query:
✅ Verification successful
The grouping change is safe to proceed
Based on the frontend implementation, the change in grouping logic from e.environment.id to [e.environment.id, e.release.id] is safe. The blocked query results are only used to check if a specific environment is blocked for a release through:
const isBlockedByReleaseChannel = (blockedEnvByRelease.data?.[release.id] ?? []).includes(env.id);The frontend component handles the data structure appropriately and uses it only for determining if deployments should be blocked, making the grouping change non-breaking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the blocked query to assess impact
# Look for frontend components or other API routes that might be affected
# Search for direct query calls
rg -A 5 "blocked\("
# Search for potential response type definitions
rg -A 5 "type.*Blocked.*Response"
Length of output: 57
Script:
#!/bin/bash
# Let's try a more comprehensive search for the blocked query usage
# Search for any mentions of 'blocked' in typescript/javascript files
rg -t ts -t js -A 5 "\.blocked"
# Search for the specific router definition
rg -t ts -t js -A 10 "router.*blocked"
# Search for any imports or references to the release router
rg -t ts -t js "from.*release.*router"
Length of output: 1086
Script:
#!/bin/bash
# Let's examine how the blocked query results are used in the frontend component
# Look at the component's implementation to understand data usage
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx"
# Search for any types or interfaces related to the blocked query response
rg -t ts "interface.*Blocked|type.*Blocked"
Length of output: 10849
packages/api/src/router/environment.ts (1)
266-316: updateReleaseChannels mutation is implemented correctly
The updateReleaseChannels mutation effectively handles setting and unsetting release channels within a transaction, ensuring data integrity and atomicity. The use of _.partition to separate null and non-null values is appropriate and improves code clarity.
| <FormField | ||
| control={form.control} | ||
| name="name" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Name</FormLabel> | ||
| <FormControl> | ||
| <Input placeholder="Staging, Production, QA..." {...field} /> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve form field accessibility and validation feedback.
Add aria-labels and error messages to form fields for better accessibility and user feedback.
<FormField
control={form.control}
name="name"
render={({ field }) => (
<FormItem>
<FormLabel>Name</FormLabel>
<FormControl>
- <Input placeholder="Staging, Production, QA..." {...field} />
+ <Input
+ placeholder="Staging, Production, QA..."
+ aria-label="Environment name"
+ aria-describedby="name-error"
+ {...field}
+ />
</FormControl>
+ {form.formState.errors.name && (
+ <div id="name-error" className="text-red-500">
+ {form.formState.errors.name.message}
+ </div>
+ )}
</FormItem>
)}
/>Also applies to: 58-73
There was a problem hiding this comment.
don't care about accessibility and validation feedback
There was a problem hiding this comment.
@adityachoudhari26, understood.
(\_/)
( •_•)
/ >💔
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx:46-57
Timestamp: 2024-10-30T00:03:57.515Z
Learning: Accessibility and validation feedback improvements are not desired for form fields in the `Overview` component within `apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const form = useForm({ schema, defaultValues: environment }); | ||
| const update = api.environment.update.useMutation(); | ||
| const envOverride = api.job.trigger.create.byEnvId.useMutation(); | ||
|
|
||
| const utils = api.useUtils(); | ||
|
|
||
| const { id, systemId } = environment; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API mutations.
Consider implementing error handling for the update and override mutations to provide feedback to users when operations fail.
const form = useForm({ schema, defaultValues: environment });
-const update = api.environment.update.useMutation();
-const envOverride = api.job.trigger.create.byEnvId.useMutation();
+const update = api.environment.update.useMutation({
+ onError: (error) => {
+ // Add error notification/toast here
+ console.error('Failed to update environment:', error);
+ }
+});
+const envOverride = api.job.trigger.create.byEnvId.useMutation({
+ onError: (error) => {
+ // Add error notification/toast here
+ console.error('Failed to override environment:', error);
+ }
+});Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
don't care about error handling
There was a problem hiding this comment.
@adityachoudhari26, understood. I'll omit error handling suggestions for API mutations.
(_/)
(•_•)
/💔
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx:28-34
Timestamp: 2024-10-30T00:03:58.738Z
Learning: In the Ctrlplane project, error handling for API mutations in React components is not required unless specified by the user.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const onSubmit = form.handleSubmit((data) => | ||
| update | ||
| .mutateAsync({ id, data }) | ||
| .then(() => form.reset(data)) | ||
| .then(() => utils.environment.bySystemId.invalidate(systemId)) | ||
| .then(() => utils.environment.byId.invalidate(id)), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add loading state feedback.
The submit handler should handle errors gracefully and provide feedback during the submission process.
const onSubmit = form.handleSubmit((data) =>
update
.mutateAsync({ id, data })
.then(() => form.reset(data))
.then(() => utils.environment.bySystemId.invalidate(systemId))
- .then(() => utils.environment.byId.invalidate(id)),
+ .then(() => utils.environment.byId.invalidate(id))
+ .catch((error) => {
+ console.error('Failed to submit form:', error);
+ // Add error notification/toast here
+ }),
);Committable suggestion was skipped due to low confidence.
| <Button | ||
| variant="outline" | ||
| onClick={() => | ||
| envOverride | ||
| .mutateAsync(id) | ||
| .then(() => utils.environment.bySystemId.invalidate(systemId)) | ||
| .then(() => utils.environment.byId.invalidate(id)) | ||
| } | ||
| > | ||
| Override | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the Override button implementation.
The Override button could benefit from additional safety measures and accessibility improvements:
- Add a disabled state during pending operations
- Add a confirmation dialog before triggering the override
- Improve accessibility with aria-label
<Button
variant="outline"
+ disabled={envOverride.isPending}
+ aria-label="Override environment settings"
onClick={() =>
- envOverride
- .mutateAsync(id)
- .then(() => utils.environment.bySystemId.invalidate(systemId))
- .then(() => utils.environment.byId.invalidate(id))
+ confirm("Are you sure you want to override this environment? This action cannot be undone.") &&
+ envOverride
+ .mutateAsync(id)
+ .then(() => utils.environment.bySystemId.invalidate(systemId))
+ .then(() => utils.environment.byId.invalidate(id))
+ .catch((error) => {
+ console.error('Failed to override environment:', error);
+ // Add error notification/toast here
+ })
}
>
- Override
+ {envOverride.isPending ? "Overriding..." : "Override"}
</Button>📝 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.
| <Button | |
| variant="outline" | |
| onClick={() => | |
| envOverride | |
| .mutateAsync(id) | |
| .then(() => utils.environment.bySystemId.invalidate(systemId)) | |
| .then(() => utils.environment.byId.invalidate(id)) | |
| } | |
| > | |
| Override | |
| </Button> | |
| <Button | |
| variant="outline" | |
| disabled={envOverride.isPending} | |
| aria-label="Override environment settings" | |
| onClick={() => | |
| confirm("Are you sure you want to override this environment? This action cannot be undone.") && | |
| envOverride | |
| .mutateAsync(id) | |
| .then(() => utils.environment.bySystemId.invalidate(systemId)) | |
| .then(() => utils.environment.byId.invalidate(id)) | |
| .catch((error) => { | |
| console.error('Failed to override environment:', error); | |
| // Add error notification/toast here | |
| }) | |
| } | |
| > | |
| {envOverride.isPending ? "Overriding..." : "Override"} | |
| </Button> |
| const DeleteEnvironmentDialog: React.FC<{ | ||
| environment: SCHEMA.Environment; | ||
| children: React.ReactNode; | ||
| onClose: () => void; | ||
| }> = ({ environment, children, onClose }) => { |
There was a problem hiding this comment.
Add type safety for required environment fields.
The component assumes environment.systemId exists but doesn't validate it through types.
Consider adding a type guard or making the required fields explicit:
- environment: SCHEMA.Environment;
+ environment: SCHEMA.Environment & {
+ systemId: string; // Make required fields explicit
+ };📝 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 DeleteEnvironmentDialog: React.FC<{ | |
| environment: SCHEMA.Environment; | |
| children: React.ReactNode; | |
| onClose: () => void; | |
| }> = ({ environment, children, onClose }) => { | |
| const DeleteEnvironmentDialog: React.FC<{ | |
| environment: SCHEMA.Environment & { | |
| systemId: string; // Make required fields explicit | |
| }; | |
| children: React.ReactNode; | |
| onClose: () => void; | |
| }> = ({ environment, children, onClose }) => { |
There was a problem hiding this comment.
the type already has an explicit systemId
There was a problem hiding this comment.
@adityachoudhari26, thank you for the clarification. You're correct that SCHEMA.Environment already includes systemId. No changes are necessary.
(🐰💭)
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx:27-31
Timestamp: 2024-10-30T00:04:33.373Z
Learning: The `SCHEMA.Environment` type includes the `systemId` field.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const updateResult = await tx.execute( | ||
| sql`UPDATE job | ||
| SET status = 'cancelled' | ||
| FROM release_job_trigger rjt | ||
| INNER JOIN environment env ON rjt.environment_id = env.id | ||
| WHERE job.status = 'pending' | ||
| AND rjt.job_id = job.id | ||
| AND rjt.release_id = ${input.releaseId} | ||
| AND env.policy_id = ${input.policyId}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer query builder over raw SQL for maintainability and safety
Using raw SQL queries can be error-prone and less secure. Consider rewriting the SQL update using the query builder to improve maintainability and prevent potential SQL injection vulnerabilities.
There was a problem hiding this comment.
unrelated
There was a problem hiding this comment.
@adityachoudhari26, understood.
(_/)
(>_<)
| .input( | ||
| z.object({ | ||
| policyId: z.string().uuid(), | ||
| releaseId: z.string().uuid(), | ||
| userId: z.string().uuid(), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
unrelated
| z.object({ | ||
| releaseId: z.string().uuid(), | ||
| policyId: z.string().uuid(), | ||
| userId: z.string().uuid(), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Avoid passing userId as input; use authenticated user ID from context
In the reject mutation, accepting userId as input can lead to security vulnerabilities. Use ctx.user.id to ensure that the action is performed by the authenticated user.
Apply this diff to fix the issue:
.input(
z.object({
releaseId: z.string().uuid(),
policyId: z.string().uuid(),
- userId: z.string().uuid(),
}),
)In the mutation, replace input.userId with ctx.user.id:
.update(environmentPolicyApproval)
- .set({ status: "rejected", userId: input.userId })
+ .set({ status: "rejected", userId: ctx.user.id })Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
unrelated
There was a problem hiding this comment.
@adityachoudhari26, understood.
(_/)
( •_•)
| const envApproval = await ctx.db | ||
| .update(environmentPolicyApproval) | ||
| .set({ status: "approved", userId: input.userId }) | ||
| .where( | ||
| and( | ||
| eq(environmentPolicyApproval.policyId, input.policyId), | ||
| eq(environmentPolicyApproval.releaseId, input.releaseId), | ||
| ), | ||
| ) | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
||
| const releaseJobTriggers = await ctx.db | ||
| .select() | ||
| .from(environmentPolicyApproval) | ||
| .innerJoin( | ||
| environmentPolicy, | ||
| eq(environmentPolicy.id, environmentPolicyApproval.policyId), | ||
| ) | ||
| .innerJoin( | ||
| environment, | ||
| eq(environment.policyId, environmentPolicy.id), | ||
| ) | ||
| .innerJoin( | ||
| releaseJobTrigger, | ||
| eq(releaseJobTrigger.environmentId, environment.id), | ||
| ) | ||
| .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) | ||
| .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) | ||
| .where( | ||
| and( | ||
| eq(environmentPolicyApproval.id, envApproval.id), | ||
| isNull(environment.deletedAt), | ||
| eq(release.id, input.releaseId), | ||
| eq(job.status, JobStatus.Pending), | ||
| ), | ||
| ); | ||
|
|
||
| await dispatchReleaseJobTriggers(ctx.db) | ||
| .releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger)) | ||
| .filter(isPassingAllPolicies) | ||
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | ||
| .dispatch(); | ||
| }), |
There was a problem hiding this comment.
Implement error handling for asynchronous operations
In the approve mutation, errors during database updates or job dispatching may not be caught, potentially causing unhandled promise rejections.
Wrap asynchronous operations in a try...catch block to handle exceptions gracefully:
.mutation(async ({ ctx, input }) => {
+ try {
const envApproval = await ctx.db
// existing code...
await dispatchReleaseJobTriggers(ctx.db)
// existing code...
+ } catch (error) {
+ // Handle error appropriately
+ throw new Error(`Approval failed: ${error.message}`);
+ }
}),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
unrelated
There was a problem hiding this comment.
@adityachoudhari26, understood.
(｡•́︿•̀｡)
| .then((rows) => { | ||
| const policy = rows.at(0)!; | ||
| const releaseChannels = rows | ||
| .map((r) => r.release_channel) | ||
| .filter(isPresent); | ||
|
|
||
| const releaseWindows = rows | ||
| .map((r) => r.environment_policy_release_window) | ||
| .filter(isPresent) | ||
| .map((r) => ({ | ||
| ...r, | ||
| startTime: new Date(r.startTime), | ||
| endTime: new Date(r.endTime), | ||
| })); | ||
|
|
||
| return { | ||
| ...policy.environment_policy, | ||
| releaseChannels, | ||
| releaseWindows, | ||
| }; | ||
| }), | ||
| ), |
There was a problem hiding this comment.
Handle the case when no policy is found to prevent runtime errors
Accessing rows.at(0)! without checking if the array is empty can lead to runtime errors if no policy matches the query.
Add a check to handle the scenario when no policy is returned:
.then((rows) => {
- const policy = rows.at(0)!;
+ const policyRow = rows.at(0);
+ if (!policyRow) {
+ throw new Error('Policy not found');
+ }
+ const policy = policyRow;📝 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.
| .then((rows) => { | |
| const policy = rows.at(0)!; | |
| const releaseChannels = rows | |
| .map((r) => r.release_channel) | |
| .filter(isPresent); | |
| const releaseWindows = rows | |
| .map((r) => r.environment_policy_release_window) | |
| .filter(isPresent) | |
| .map((r) => ({ | |
| ...r, | |
| startTime: new Date(r.startTime), | |
| endTime: new Date(r.endTime), | |
| })); | |
| return { | |
| ...policy.environment_policy, | |
| releaseChannels, | |
| releaseWindows, | |
| }; | |
| }), | |
| ), | |
| .then((rows) => { | |
| const policyRow = rows.at(0); | |
| if (!policyRow) { | |
| throw new Error('Policy not found'); | |
| } | |
| const policy = policyRow; | |
| const releaseChannels = rows | |
| .map((r) => r.release_channel) | |
| .filter(isPresent); | |
| const releaseWindows = rows | |
| .map((r) => r.environment_policy_release_window) | |
| .filter(isPresent) | |
| .map((r) => ({ | |
| ...r, | |
| startTime: new Date(r.startTime), | |
| endTime: new Date(r.endTime), | |
| })); | |
| return { | |
| ...policy.environment_policy, | |
| releaseChannels, | |
| releaseWindows, | |
| }; | |
| }), | |
| ), |
| .input( | ||
| z.object({ | ||
| policyId: z.string().uuid(), | ||
| releaseId: z.string().uuid(), | ||
| userId: z.string().uuid(), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
unrelated
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1)
106-110: Consider simplifying the display value calculation.
The nested ternary operation could be made more readable.
- const display =
- releaseChannel?.name ??
- policyReleaseChannel?.name ??
- "No release channel";
+ const display = [
+ releaseChannel?.name,
+ policyReleaseChannel?.name,
+ "No release channel"
+ ].find(Boolean) ?? "No release channel";apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (2)
24-44: Consider adding TypeScript return type for better type safety.
The hook implementation is clean and follows best practices. Consider adding an explicit return type for better type safety and documentation.
-export const useEnvironmentDrawer = () => {
+export const useEnvironmentDrawer = (): {
+ environmentId: string | null;
+ setEnvironmentId: (id: string | null) => void;
+ removeEnvironmentId: () => void;
+} => {82-88: Improve accessibility and add fallback for environment name.
Consider these improvements:
- Add a fallback for the environment name to prevent layout shift
- Add an aria-label to the menu button for better accessibility
-{environment?.name}
+{environment?.name ?? 'Loading...'}
{environment != null && (
<EnvironmentDropdownMenu environment={environment}>
- <Button variant="ghost" size="icon" className="h-6 w-6">
+ <Button variant="ghost" size="icon" className="h-6 w-6" aria-label="Environment options">apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (3)
Line range hint 43-91: Consider adding error state handling.
The component handles loading state well but doesn't show any feedback for API errors. This could lead to a confusing user experience if the target views fail to load.
Consider adding error handling:
{targetViewsQ.isLoading && (
<CommandItem disabled>
<IconLoader2 className="mr-2 h-4 w-4 animate-spin" />
Loading target views...
</CommandItem>
)}
+{targetViewsQ.error && (
+ <CommandItem disabled className="text-destructive">
+ <IconAlertCircle className="mr-2 h-4 w-4" />
+ Failed to load target views
+ </CommandItem>
+)}Line range hint 104-123: Add error handling for the update mutation.
The form submission lacks error handling which could lead to silent failures.
Consider adding error handling:
const onSubmit = form.handleSubmit((data) =>
update
.mutateAsync({
id: environment.id,
data: {
...data,
targetFilter: targetFilter ?? null,
},
})
.then(() => form.reset(data))
.then(() => utils.environment.bySystemId.invalidate(environment.systemId))
- .then(() => utils.environment.byId.invalidate(environment.id)),
+ .then(() => utils.environment.byId.invalidate(environment.id))
+ .catch((error) => {
+ console.error('Failed to update environment:', error);
+ // Show error toast or set form error state
+ }),
);Line range hint 124-214: Consider renaming the targets query variable.
The variable name targets could be more descriptive to indicate it's a query result.
Consider renaming:
-const targets = api.target.byWorkspaceId.list.useQuery(
+const targetListQuery = api.target.byWorkspaceId.list.useQuery(This makes it clearer that it's a query result object containing loading, error, and data states.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1 hunks)
- packages/api/src/router/environment-policy.ts (1 hunks)
- packages/api/src/router/environment.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx
🧰 Additional context used
📓 Learnings (2)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:55-57
Timestamp: 2024-10-29T23:49:24.855Z
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.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx:48-71
Timestamp: 2024-10-29T23:46:46.250Z
Learning: Prefer using `Object.fromEntries` over `.reduce` for transforming arrays into objects in React components to improve readability and maintainability.
🔇 Additional comments (12)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (4)
1-38: LGTM! Well-structured types and imports.
The type definitions are comprehensive and properly extend the schema types. The imports are well-organized and follow best practices.
40-47: LGTM! Clean component setup.
The component declaration and mutation setup follow React and TRPC best practices.
48-71: LGTM! Clean data transformation using Object.fromEntries.
The code effectively uses Object.fromEntries for transforming arrays into lookup objects, following the established best practices.
73-82: LGTM! Clear and concise event handlers.
The event handlers are well-implemented and follow React patterns.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (2)
1-23: LGTM: Imports are well-organized and complete.
The imports are properly structured with clear separation between React core, UI components, and local imports.
49-49: Fix 'setIsOpen' handler to properly handle the open state.
The current implementation might lead to unexpected behavior when opening or closing the drawer.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (3)
Line range hint 1-38: LGTM! Well-organized imports.
The imports are properly organized and the previous issue with the Label import has been addressed.
Line range hint 93-96: LGTM! Clear schema definition.
The form schema is well-defined using zod with appropriate optionality.
100-103: Props interface looks good.
The component props are well-typed and include the necessary workspaceId.
packages/api/src/router/environment.ts (1)
8-8: LGTM: Import additions are well-organized and necessary.
The new imports support the release channel functionality and are all utilized within the codebase.
Also applies to: 10-10, 19-21, 27-27
packages/api/src/router/environment-policy.ts (2)
1-479: Verify the impact of policy changes on existing deployments
Since this is a new file implementing policy management, we should verify that existing deployments won't be affected by these changes.
✅ Verification successful
Let me gather more specific information about the policy implementation and its usage.
Based on the comprehensive results, I can now provide a final response:
No impact on existing deployments - policy changes are safely implemented
The implementation follows a robust policy enforcement pattern where:
- Policy checks are performed through
isPassingAllPoliciesbefore any deployment - Changes are properly isolated through database schema with proper foreign key constraints
- Policy deployments are versioned and tracked through
environmentPolicyDeploymenttable - Existing deployments are protected by transaction boundaries and proper sequencing
The code shows a well-structured approach where policy changes cannot affect existing deployments without explicit approval, and all policy enforcement happens at dispatch time through dispatchReleaseJobTriggers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing deployments that might be affected
echo "Checking for existing deployments..."
rg -l "environmentPolicy|releaseChannel" --type ts
# Check for potential policy references
echo "Checking for policy references..."
ast-grep --pattern 'policy.$$$' --lang typescript
Length of output: 4123
Script:
#!/bin/bash
# Check how policies are enforced in job dispatch
echo "Checking policy enforcement in job dispatch..."
rg -A 10 "isPassingAllPolicies|dispatchReleaseJobTriggers" --type ts
# Check for existing policy implementations
echo "Checking policy implementations..."
ast-grep --pattern 'export function $NAME($$$) {
$$$
}' --lang typescript packages/job-dispatch/src/policies/*.ts
# Check for policy-related database schema
echo "Checking policy database schema..."
rg "environmentPolicy|policyId|PolicyDeployment" packages/db/src/schema/*.ts
Length of output: 21459
166-210:
Add error handling for asynchronous operations
The approve procedure lacks error handling for database operations and job dispatch.
Add try-catch block:
.mutation(async ({ ctx, input }) => {
+ try {
const envApproval = await ctx.db
// ... existing code ...
await dispatchReleaseJobTriggers(ctx.db)
.releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger))
.filter(isPassingAllPolicies)
.then(cancelOldReleaseJobTriggersOnJobDispatch)
.dispatch();
+ } catch (error) {
+ throw new Error(`Approval failed: ${error.message}`);
+ }
}),Likely invalid or redundant comment.
| {deploymentsWithReleaseChannels.map((d) => { | ||
| const releaseChannelId = releaseChannels[d.id]; | ||
| const releaseChannel = d.releaseChannels.find( | ||
| (rc) => rc.id === releaseChannelId, | ||
| ); | ||
| const policyReleaseChannelId = policyReleaseChannels[d.id]; | ||
| const policyReleaseChannel = d.releaseChannels.find( | ||
| (rc) => rc.id === policyReleaseChannelId, | ||
| ); | ||
|
|
||
| const onChange = (channelId: string) => | ||
| updateReleaseChannel(d.id, channelId === "null" ? null : channelId); | ||
|
|
||
| const value = releaseChannelId ?? undefined; | ||
|
|
||
| const display = | ||
| releaseChannel?.name ?? | ||
| policyReleaseChannel?.name ?? | ||
| "No release channel"; | ||
|
|
||
| const isFromPolicy = | ||
| releaseChannelId == null && policyReleaseChannelId != null; | ||
|
|
||
| return ( | ||
| <div key={d.id} className="flex items-center gap-2"> | ||
| <span className="w-40 truncate">{d.name}</span> | ||
| <Select value={value} onValueChange={onChange}> | ||
| <SelectTrigger className="flex w-72 items-center gap-2"> | ||
| <span className="truncate text-xs">{display}</span> | ||
| {isFromPolicy && ( | ||
| <span className="shrink-0 text-xs text-muted-foreground"> | ||
| (From policy) | ||
| </span> | ||
| )} | ||
| </SelectTrigger> | ||
| <SelectContent className="overflow-y-auto"> | ||
| <SelectItem value="null" className="w-72"> | ||
| No release channel | ||
| </SelectItem> | ||
| {d.releaseChannels.map((rc) => ( | ||
| <SelectItem key={rc.id} value={rc.id} className="w-72"> | ||
| <span className="truncate">{rc.name}</span> | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the deployment item into a separate component.
The mapping logic contains complex rendering that could be moved to a separate component for better maintainability and readability.
type DeploymentItemProps = {
deployment: Deployment;
releaseChannelId: string | null;
policyReleaseChannelId: string | null;
onChannelChange: (channelId: string) => void;
};
const DeploymentItem: React.FC<DeploymentItemProps> = ({
deployment,
releaseChannelId,
policyReleaseChannelId,
onChannelChange,
}) => {
const releaseChannel = deployment.releaseChannels.find(
(rc) => rc.id === releaseChannelId
);
const policyReleaseChannel = deployment.releaseChannels.find(
(rc) => rc.id === policyReleaseChannelId
);
const display = [
releaseChannel?.name,
policyReleaseChannel?.name,
"No release channel"
].find(Boolean) ?? "No release channel";
const isFromPolicy = releaseChannelId == null && policyReleaseChannelId != null;
return (
<div className="flex items-center gap-2">
<span className="w-40 truncate">{deployment.name}</span>
<Select
value={releaseChannelId ?? undefined}
onValueChange={onChannelChange}
>
{/* ... rest of the select component ... */}
</Select>
</div>
);
};There was a problem hiding this comment.
extracted component out
There was a problem hiding this comment.
@adityachoudhari26 Great to hear that you've extracted the component! This will definitely enhance the maintainability and readability of the code.
(\_/)
( ^.^ )
/️🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| <DrawerContent | ||
| showBar={false} | ||
| className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making drawer width more responsive.
The current fixed width of 2/3 viewport with a max-width might be too wide on certain screen sizes. Consider using responsive breakpoints for better adaptability.
-className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
+className="left-auto right-0 top-0 mt-0 h-screen w-full overflow-auto rounded-none focus-visible:outline-none md:w-3/4 lg:w-2/3 max-w-6xl focus-visible:outline-none"📝 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.
| <DrawerContent | |
| showBar={false} | |
| className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none" | |
| > | |
| <DrawerContent | |
| showBar={false} | |
| className="left-auto right-0 top-0 mt-0 h-screen w-full overflow-auto rounded-none focus-visible:outline-none md:w-3/4 lg:w-2/3 max-w-6xl focus-visible:outline-none" | |
| > |
| .query(async ({ ctx, input }) => { | ||
| const envRCSubquery = ctx.db | ||
| .select({ | ||
| releaseChannelEnvId: environmentReleaseChannel.environmentId, | ||
| releaseChannelDeploymentId: releaseChannel.deploymentId, | ||
| releaseChannelFilter: releaseChannel.releaseFilter, | ||
| releaseChannelId: releaseChannel.id, | ||
| releaseChannelName: releaseChannel.name, | ||
| }) | ||
| .from(environmentReleaseChannel) | ||
| .innerJoin( | ||
| releaseChannel, | ||
| eq(environmentReleaseChannel.channelId, releaseChannel.id), | ||
| ) | ||
| .as("envRCSubquery"); | ||
|
|
||
| const policyRCSubquery = ctx.db | ||
| .select({ | ||
| releaseChannelPolicyId: environmentPolicyReleaseChannel.policyId, | ||
| releaseChannelDeploymentId: releaseChannel.deploymentId, | ||
| releaseChannelFilter: releaseChannel.releaseFilter, | ||
| releaseChannelId: releaseChannel.id, | ||
| releaseChannelName: releaseChannel.name, | ||
| }) | ||
| .from(environmentPolicyReleaseChannel) | ||
| .innerJoin( | ||
| releaseChannel, | ||
| eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), | ||
| ) | ||
| .as("policyRCSubquery"); | ||
|
|
||
| return ctx.db | ||
| .select() | ||
| .from(environment) | ||
| .leftJoin( | ||
| environmentPolicy, | ||
| eq(environment.policyId, environmentPolicy.id), | ||
| ) | ||
| .innerJoin(system, eq(environment.systemId, system.id)) | ||
| .where(and(eq(environment.id, input), isNull(environment.deletedAt))) | ||
| .then(takeFirstOrNull) | ||
| .then((env) => | ||
| env == null | ||
| ? null | ||
| : { | ||
| ...env.environment, | ||
| policy: env.environment_policy, | ||
| system: env.system, | ||
| }, | ||
| ), | ||
| ), | ||
| .leftJoin( | ||
| envRCSubquery, | ||
| eq(environment.id, envRCSubquery.releaseChannelEnvId), | ||
| ) | ||
| .leftJoin( | ||
| policyRCSubquery, | ||
| eq(environmentPolicy.id, policyRCSubquery.releaseChannelPolicyId), | ||
| ) | ||
| .where(eq(environment.id, input)) | ||
| .then((rows) => { | ||
| const env = rows.at(0); | ||
| if (env == null) return null; | ||
| const policy = | ||
| env.environment_policy == null | ||
| ? null | ||
| : { | ||
| ...env.environment_policy, | ||
| releaseChannels: _.chain(rows) | ||
| .map((r) => r.policyRCSubquery) | ||
| .filter(isPresent) | ||
| .uniqBy((r) => r.releaseChannelId) | ||
| .map((r) => ({ | ||
| deploymentId: r.releaseChannelDeploymentId, | ||
| filter: r.releaseChannelFilter, | ||
| id: r.releaseChannelId, | ||
| name: r.releaseChannelName, | ||
| })) | ||
| .value(), | ||
| }; | ||
|
|
||
| const releaseChannels = _.chain(rows) | ||
| .map((r) => r.envRCSubquery) | ||
| .filter(isPresent) | ||
| .uniqBy((r) => r.releaseChannelId) | ||
| .map((r) => ({ | ||
| deploymentId: r.releaseChannelDeploymentId, | ||
| filter: r.releaseChannelFilter, | ||
| id: r.releaseChannelId, | ||
| name: r.releaseChannelName, | ||
| })) | ||
| .value(); | ||
|
|
||
| return { | ||
| ...env.environment, | ||
| policy, | ||
| releaseChannels, | ||
| system: env.system, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider type safety improvements and code simplification.
The code effectively handles release channels and addresses duplicate entries using _.uniqBy. However, there are opportunities for improvement:
- Consider defining TypeScript interfaces for the subquery results to improve type safety:
interface EnvReleaseChannelSubquery {
releaseChannelEnvId: string;
releaseChannelDeploymentId: string;
releaseChannelFilter: string;
releaseChannelId: string;
releaseChannelName: string;
}
interface PolicyReleaseChannelSubquery {
releaseChannelPolicyId: string;
// ... similar fields
}- The transformation logic could be simplified using a helper function:
const mapReleaseChannel = (r: EnvReleaseChannelSubquery | PolicyReleaseChannelSubquery) => ({
deploymentId: r.releaseChannelDeploymentId,
filter: r.releaseChannelFilter,
id: r.releaseChannelId,
name: r.releaseChannelName,
});
// Then use it in both transformations:
releaseChannels: _.chain(rows)
.map((r) => r.envRCSubquery)
.filter(isPresent)
.uniqBy((r) => r.releaseChannelId)
.map(mapReleaseChannel)
.value(),| updateReleaseChannels: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| id: z.string().uuid(), | ||
| releaseChannels: z.record(z.string().uuid().nullable()), | ||
| }), | ||
| ) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser | ||
| .perform(Permission.EnvironmentUpdate) | ||
| .on({ type: "environment", id: input.id }), | ||
| }) | ||
| .mutation(({ ctx, input }) => { | ||
| const [nulled, set] = _.partition( | ||
| Object.entries(input.releaseChannels), | ||
| ([_, channelId]) => channelId == null, | ||
| ); | ||
|
|
||
| const nulledIds = nulled.map(([deploymentId]) => deploymentId); | ||
| const setChannels = set.map(([deploymentId, channelId]) => ({ | ||
| environmentId: input.id, | ||
| deploymentId, | ||
| channelId: channelId!, | ||
| })); | ||
|
|
||
| return ctx.db.transaction(async (db) => { | ||
| await db | ||
| .delete(environmentReleaseChannel) | ||
| .where( | ||
| and( | ||
| eq(environmentReleaseChannel.environmentId, input.id), | ||
| inArray(environmentReleaseChannel.deploymentId, nulledIds), | ||
| ), | ||
| ); | ||
|
|
||
| if (setChannels.length > 0) | ||
| await db | ||
| .insert(environmentReleaseChannel) | ||
| .values(setChannels) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| environmentReleaseChannel.environmentId, | ||
| environmentReleaseChannel.deploymentId, | ||
| ], | ||
| set: buildConflictUpdateColumns(environmentReleaseChannel, [ | ||
| "channelId", | ||
| ]), | ||
| }); | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and type safety.
The transaction and partition logic are well-implemented, but there are opportunities for improvement:
- Add error handling for the case where the release channel doesn't exist:
.mutation(async ({ ctx, input }) => {
// Verify all channelIds exist before proceeding
const channelIds = Object.values(input.releaseChannels).filter(isPresent);
if (channelIds.length > 0) {
const existingChannels = await ctx.db
.select({ id: releaseChannel.id })
.from(releaseChannel)
.where(inArray(releaseChannel.id, channelIds));
if (existingChannels.length !== channelIds.length) {
throw new Error('One or more release channels do not exist');
}
}
// Rest of the implementation...
});- Improve type safety by avoiding the non-null assertion:
const setChannels = set.map(([deploymentId, channelId]) => {
if (!channelId) throw new Error('Unexpected null channelId');
return {
environmentId: input.id,
deploymentId,
channelId,
};
});| .input( | ||
| z.object({ | ||
| policyId: z.string().uuid(), | ||
| releaseId: z.string().uuid(), | ||
| userId: z.string().uuid(), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Security: Remove userId from input and use authenticated user
The approve procedure accepts userId as input which could lead to impersonation. Use the authenticated user's ID from the context instead.
Apply this diff:
.input(
z.object({
policyId: z.string().uuid(),
releaseId: z.string().uuid(),
- userId: z.string().uuid(),
}),
)
.mutation(async ({ ctx, input }) => {
const envApproval = await ctx.db
.update(environmentPolicyApproval)
- .set({ status: "approved", userId: input.userId })
+ .set({ status: "approved", userId: ctx.user.id })Also applies to: 167-170
| .then((rows) => { | ||
| const policy = rows.at(0)!; | ||
| const releaseChannels = _.chain(rows) |
There was a problem hiding this comment.
Add null check for policy retrieval
The code assumes that rows.at(0) will always return a value. Add a null check to handle cases where no policy is found.
Apply this diff:
.then((rows) => {
- const policy = rows.at(0)!;
+ const policy = rows.at(0);
+ if (!policy) {
+ throw new Error('Policy not found');
+ }📝 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.
| .then((rows) => { | |
| const policy = rows.at(0)!; | |
| const releaseChannels = _.chain(rows) | |
| .then((rows) => { | |
| const policy = rows.at(0); | |
| if (!policy) { | |
| throw new Error('Policy not found'); | |
| } | |
| const releaseChannels = _.chain(rows) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1)
35-75: Consider using relative width units instead of fixed width.
The Select component uses hardcoded widths (w-40, w-72) which might not be responsive across different viewport sizes.
Consider using relative units or CSS Grid for better responsiveness:
- <span className="w-40 truncate">{deployment.name}</span>
- <Select value={value} onValueChange={onChange}>
- <SelectTrigger className="w-72">
+ <span className="w-1/3 truncate">{deployment.name}</span>
+ <Select value={value} onValueChange={onChange} className="flex-1">
+ <SelectTrigger className="w-full">📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (2)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx:48-71
Timestamp: 2024-10-29T23:46:46.250Z
Learning: Prefer using `Object.fromEntries` over `.reduce` for transforming arrays into objects in React components to improve readability and maintainability.
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx:48-71
Timestamp: 2024-10-29T23:46:46.250Z
Learning: Prefer using `Object.fromEntries` over `.reduce` for transforming arrays into objects in React components to improve readability and maintainability.
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/ReleaseChannels.tsx (2)
1-33: LGTM! Well-structured type definitions and imports.
The type definitions are clear, properly extend the schema types, and the imports are well-organized.
104-110: Previous comment about error handling is still applicable.
The mutation still lacks error handling and user feedback.
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/ReleaseChannels.tsx (3)
121-127: Previous suggestion about memoizing policyReleaseChannels is still applicable
The earlier review comment regarding the use of useMemo to memoize the computation of policyReleaseChannels remains valid and applicable to the current code segment.
129-135: Previous suggestion about memoizing currEnvReleaseChannels is still applicable
The prior suggestion to use useMemo for memoizing the computation of currEnvReleaseChannels is still relevant and could enhance performance by preventing unnecessary recalculations on each render.
146-150: Previous suggestion about adding error handling for the mutation is still applicable
The previous review comment about adding error handling and user feedback for the updateReleaseChannels mutation in the onSubmit function remains valid. Implementing error handling will improve the application's resilience and user experience.
| const deploymentsWithReleaseChannels = deployments.filter( | ||
| (d) => d.releaseChannels.length > 0, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing filtered deployments for better performance.
The deploymentsWithReleaseChannels filter operation and its mapping in the render could be memoized to prevent unnecessary recalculations on re-renders.
Consider using useMemo:
+import { useMemo } from "react";
- const deploymentsWithReleaseChannels = deployments.filter(
- (d) => d.releaseChannels.length > 0,
- );
+ const deploymentsWithReleaseChannels = useMemo(
+ () => deployments.filter((d) => d.releaseChannels.length > 0),
+ [deployments]
+ );Also applies to: 120-127
| @@ -96,14 +107,20 @@ const PolicyDropdownMenu: React.FC<{ | |||
| export const EnvironmentPolicyDrawer: React.FC = () => { | |||
| const { environmentPolicyId, removeEnvironmentPolicyId } = | |||
| useEnvironmentPolicyDrawer(); | |||
| const isOpen = environmentPolicyId != null && environmentPolicyId != ""; | |||
| const isOpen = Boolean(environmentPolicyId); | |||
There was a problem hiding this comment.
Boolean does more falsy checks on the string than what we had before and its more concise, so i'd argue its more explicit because it covers all the bases
| ), | ||
| }), | ||
|
|
||
| approval: createTRPCRouter({ |
There was a problem hiding this comment.
I though we had all these endpoints
There was a problem hiding this comment.
I moved the environment policy stuff into its own file because it was getting too big
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/api/src/router/environment-policy.ts (1 hunks)
🔇 Additional comments (1)
packages/api/src/router/environment-policy.ts (1)
322-324:
Add null check for policy retrieval
The code assumes that rows.at(0) will always return a value, which could lead to runtime errors if no policy is found.
Add a null check:
.then((rows) => {
- const policy = rows.at(0)!;
+ const policy = rows.at(0);
+ if (!policy) {
+ throw new Error('Policy not found');
+ }Likely invalid or redundant comment.
| const updateResult = await tx.execute( | ||
| sql`UPDATE job | ||
| SET status = 'cancelled' | ||
| FROM release_job_trigger rjt | ||
| INNER JOIN environment env ON rjt.environment_id = env.id | ||
| WHERE job.status = 'pending' | ||
| AND rjt.job_id = job.id | ||
| AND rjt.release_id = ${input.releaseId} | ||
| AND env.policy_id = ${input.policyId}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using query builder instead of raw SQL
While the raw SQL query is functional, using the query builder would be more maintainable and type-safe.
Consider refactoring to use the query builder:
- const updateResult = await tx.execute(
- sql`UPDATE job
- SET status = 'cancelled'
- FROM release_job_trigger rjt
- INNER JOIN environment env ON rjt.environment_id = env.id
- WHERE job.status = 'pending'
- AND rjt.job_id = job.id
- AND rjt.release_id = ${input.releaseId}
- AND env.policy_id = ${input.policyId}`,
- );
+ const updateResult = await tx
+ .update(job)
+ .set({ status: 'cancelled' })
+ .where(
+ and(
+ eq(job.status, 'pending'),
+ sql`EXISTS (
+ SELECT 1 FROM release_job_trigger rjt
+ INNER JOIN environment env ON rjt.environment_id = env.id
+ WHERE rjt.job_id = job.id
+ AND rjt.release_id = ${input.releaseId}
+ AND env.policy_id = ${input.policyId}
+ )`
+ )
+ );Committable suggestion was skipped due to low confidence.
| .mutation(async ({ ctx, input }) => { | ||
| const envApproval = await ctx.db | ||
| .update(environmentPolicyApproval) | ||
| .set({ status: "approved", userId: ctx.session.user.id }) | ||
| .where( | ||
| and( | ||
| eq(environmentPolicyApproval.policyId, input.policyId), | ||
| eq(environmentPolicyApproval.releaseId, input.releaseId), | ||
| ), | ||
| ) | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
||
| const releaseJobTriggers = await ctx.db | ||
| .select() | ||
| .from(environmentPolicyApproval) | ||
| .innerJoin( | ||
| environmentPolicy, | ||
| eq(environmentPolicy.id, environmentPolicyApproval.policyId), | ||
| ) | ||
| .innerJoin( | ||
| environment, | ||
| eq(environment.policyId, environmentPolicy.id), | ||
| ) | ||
| .innerJoin( | ||
| releaseJobTrigger, | ||
| eq(releaseJobTrigger.environmentId, environment.id), | ||
| ) | ||
| .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) | ||
| .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) | ||
| .where( | ||
| and( | ||
| eq(environmentPolicyApproval.id, envApproval.id), | ||
| isNull(environment.deletedAt), | ||
| eq(release.id, input.releaseId), | ||
| eq(job.status, JobStatus.Pending), | ||
| ), | ||
| ); | ||
|
|
||
| await dispatchReleaseJobTriggers(ctx.db) | ||
| .releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger)) | ||
| .filter(isPassingAllPolicies) | ||
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | ||
| .dispatch(); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for job dispatch operations
The approve procedure should handle potential errors during job dispatch operations.
Add error handling:
.mutation(async ({ ctx, input }) => {
+ try {
const envApproval = await ctx.db
// ... existing approval update code ...
const releaseJobTriggers = await ctx.db
// ... existing job triggers query ...
await dispatchReleaseJobTriggers(ctx.db)
.releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger))
.filter(isPassingAllPolicies)
.then(cancelOldReleaseJobTriggersOnJobDispatch)
.dispatch();
+ } catch (error) {
+ throw new Error(`Failed to approve policy: ${error.message}`);
+ }
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .mutation(async ({ ctx, input }) => { | |
| const envApproval = await ctx.db | |
| .update(environmentPolicyApproval) | |
| .set({ status: "approved", userId: ctx.session.user.id }) | |
| .where( | |
| and( | |
| eq(environmentPolicyApproval.policyId, input.policyId), | |
| eq(environmentPolicyApproval.releaseId, input.releaseId), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirst); | |
| const releaseJobTriggers = await ctx.db | |
| .select() | |
| .from(environmentPolicyApproval) | |
| .innerJoin( | |
| environmentPolicy, | |
| eq(environmentPolicy.id, environmentPolicyApproval.policyId), | |
| ) | |
| .innerJoin( | |
| environment, | |
| eq(environment.policyId, environmentPolicy.id), | |
| ) | |
| .innerJoin( | |
| releaseJobTrigger, | |
| eq(releaseJobTrigger.environmentId, environment.id), | |
| ) | |
| .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) | |
| .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) | |
| .where( | |
| and( | |
| eq(environmentPolicyApproval.id, envApproval.id), | |
| isNull(environment.deletedAt), | |
| eq(release.id, input.releaseId), | |
| eq(job.status, JobStatus.Pending), | |
| ), | |
| ); | |
| await dispatchReleaseJobTriggers(ctx.db) | |
| .releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger)) | |
| .filter(isPassingAllPolicies) | |
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | |
| .dispatch(); | |
| }), | |
| .mutation(async ({ ctx, input }) => { | |
| try { | |
| const envApproval = await ctx.db | |
| .update(environmentPolicyApproval) | |
| .set({ status: "approved", userId: ctx.session.user.id }) | |
| .where( | |
| and( | |
| eq(environmentPolicyApproval.policyId, input.policyId), | |
| eq(environmentPolicyApproval.releaseId, input.releaseId), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirst); | |
| const releaseJobTriggers = await ctx.db | |
| .select() | |
| .from(environmentPolicyApproval) | |
| .innerJoin( | |
| environmentPolicy, | |
| eq(environmentPolicy.id, environmentPolicyApproval.policyId), | |
| ) | |
| .innerJoin( | |
| environment, | |
| eq(environment.policyId, environmentPolicy.id), | |
| ) | |
| .innerJoin( | |
| releaseJobTrigger, | |
| eq(releaseJobTrigger.environmentId, environment.id), | |
| ) | |
| .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) | |
| .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) | |
| .where( | |
| and( | |
| eq(environmentPolicyApproval.id, envApproval.id), | |
| isNull(environment.deletedAt), | |
| eq(release.id, input.releaseId), | |
| eq(job.status, JobStatus.Pending), | |
| ), | |
| ); | |
| await dispatchReleaseJobTriggers(ctx.db) | |
| .releaseTriggers(releaseJobTriggers.map((t) => t.release_job_trigger)) | |
| .filter(isPassingAllPolicies) | |
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | |
| .dispatch(); | |
| } catch (error) { | |
| throw new Error(`Failed to approve policy: ${error.message}`); | |
| } | |
| }), |
Summary by CodeRabbit
Release Notes
New Features
EnvironmentDrawercomponent for managing environment information.EnvironmentDropdownMenufor environment management with a delete option.Overviewcomponent for editing environment details.ReleaseChannelscomponent for managing release channels in environments.policyRouterwith new procedures for environment policies and deployments.DeploymentSelectcomponent to facilitate release channel selection for deployments.EnvFlowBuilderandEnvFlowPanelfor improved environment and policy management.EnvironmentUpdate, to the authorization system.Bug Fixes
Documentation
Chores