fix: Init release channels ui #181
Conversation
WalkthroughThis pull request introduces several new React components and functionalities related to managing release channels within a web service. Key additions include the Changes
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: 27
🧹 Outside diff range and nitpick comments (18)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx (1)
6-10: Add JSDoc documentation for better code maintainability.Consider adding JSDoc documentation to describe the component's purpose, parameters, and return value.
+/** + * Page component for managing release channels within a deployment. + * @param {Object} props - Component props + * @param {Object} props.params - URL parameters + * @param {string} props.params.workspaceSlug - Workspace identifier + * @param {string} props.params.systemSlug - System identifier + * @param {string} props.params.deploymentSlug - Deployment identifier + * @returns {Promise<JSX.Element>} Rendered release channels table + */ export default async function ReleaseChannelsPage({ params: { workspaceSlug, systemSlug, deploymentSlug },apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/useReleaseChannelDrawer.ts (2)
1-3: Consider using a more specific constant name.The constant
paramcould be more descriptive to prevent potential naming conflicts and improve code clarity.-const param = "release_channel_id"; +const RELEASE_CHANNEL_URL_PARAM = "release_channel_id";
21-24: Consider adding explicit return type for better type safety.While TypeScript can infer the return type, an explicit interface would improve maintainability and documentation.
+interface ReleaseChannelDrawerHook { + releaseChannelId: string | null; + setReleaseChannelId: (id: string | null) => void; + removeReleaseChannelId: () => void; +} -export const useReleaseChannelDrawer = () => { +export const useReleaseChannelDrawer = (): ReleaseChannelDrawerHook => {apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/ReleaseChannelsTable.tsx (3)
18-20: Add JSDoc comments to document the types.The
ReleaseChanneltype extends the schema with atotalproperty, but its purpose and usage aren't immediately clear. Consider adding JSDoc comments to improve code documentation.+/** Extends the base ReleaseChannel schema with a count of associated releases */ type ReleaseChannel = SCHEMA.ReleaseChannel & { total: number }; +/** Props for the ReleaseChannelsTable component */ type ReleaseChannelTableProps = { releaseChannels: ReleaseChannel[] };
27-27: Consider using responsive table layout.The
table-fixedclass forces a fixed layout which might truncate content or cause layout issues with varying content lengths. Consider using a responsive layout or adding horizontal scrolling for better content accommodation.
46-50: Add tooltip for release filter badge.The
ReleaseConditionBadgemight benefit from a tooltip explaining the filter condition, especially for complex filters.{releaseChannel.releaseFilter != null && ( <ReleaseConditionBadge condition={releaseChannel.releaseFilter} + title={`Filter: ${releaseChannel.releaseFilter}`} /> )}packages/db/src/schema/rbac.ts (1)
Line range hint
36-48: Consider documenting the scope types.The scopeType enum is growing and contains various related but distinct concepts. Consider adding JSDoc comments to document the purpose and usage of each scope type, particularly the new "releaseChannel" scope.
Example documentation:
export const scopeType = pgEnum("scope_type", [ + /** Scope for managing releases and their configurations */ "release", + /** Scope for managing release channels and their settings */ "releaseChannel", "target", // ... rest of the enum values ]);apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (1)
1-22: Consider organizing imports by categoryConsider grouping imports into categories for better maintainability:
- External dependencies (react, next, zod)
- Internal UI components
- Local imports (trpc)
// External dependencies import type * as SCHEMA from "@ctrlplane/db/schema"; import type React from "react"; import { useRouter } from "next/navigation"; import { z } from "zod"; // UI Components import { Button } from "@ctrlplane/ui/button"; import { Form, FormControl, FormField, FormItem, FormLabel, useForm, } from "@ctrlplane/ui/form"; import { Input } from "@ctrlplane/ui/input"; import { Textarea } from "@ctrlplane/ui/textarea"; // Local imports import { api } from "~/trpc/react";apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx (1)
43-47: Enhance loading state accessibility.Add appropriate ARIA attributes to improve accessibility during loading.
- {loading && ( - <div className="flex h-full w-full items-center justify-center"> + {loading && ( + <div + className="flex h-full w-full items-center justify-center" + role="status" + aria-label="Loading release channel details" + > <IconLoader2 className="h-8 w-8 animate-spin" /> </div> )}apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx (1)
68-68: Enhance user feedback for release listThe current implementation could benefit from more comprehensive user feedback states.
Consider replacing the current implementation with:
-{releases != null && <ReleaseBadgeList releases={releases} />} +{releases != null ? ( + releases.items.length > 0 ? ( + <ReleaseBadgeList releases={releases} /> + ) : ( + <div className="text-sm text-muted-foreground"> + No matching releases found for the current condition + </div> + ) +) : null}apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
103-103: Document the preventDefault usage.Add a comment explaining why preventDefault is necessary here.
-onSelect={(e) => e.preventDefault()} +// Prevent the dropdown from closing when selecting the delete option +// as we want to show the confirmation dialog first +onSelect={(e) => e.preventDefault()}apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (1)
82-87: Enhance accessibility with ARIA attributes.The UI elements should have proper ARIA attributes for better accessibility.
Consider these improvements:
if (loading) return ( <div className="flex h-full w-full items-center justify-center"> - <IconLoader2 className="h-8 w-8 animate-spin" /> + <IconLoader2 className="h-8 w-8 animate-spin" aria-label="Loading releases" role="status" /> </div> ); return ( <div className="space-y-2 p-6"> <Label>Release Filter ({releases?.total ?? "-"})</Label> {filter != null && <ReleaseConditionBadge condition={filter} />} <div className="flex items-center gap-2"> <ReleaseConditionDialog condition={filter ?? defaultCondition} deploymentId={deploymentId} onChange={onUpdate} > <Button variant="outline" size="sm" className="flex items-center gap-2" + aria-label="Edit release filter" > <IconFilter className="h-4 w-4" /> Edit filter </Button> </ReleaseConditionDialog> {releaseFilterUrl != null && ( <Link href={releaseFilterUrl} target="_blank"> <Button variant="outline" size="sm" className="flex items-center gap-2" + aria-label="View releases in new tab" > <IconExternalLink className="h-4 w-4" /> View releases </Button> </Link> )} </div> {releases != null && <ReleaseBadgeList releases={releases} />} </div> );Also applies to: 89-122
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (1)
Line range hint
111-122: Fix incorrect URL in Jobs navigation itemThe Jobs navigation item is currently using the variables URL (
variablesUrl) instead of what should presumably be a jobs-specific URL.Apply this fix:
- <Link href={variablesUrl} legacyBehavior passHref> + <Link href={`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/jobs`} legacyBehavior passHref>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (1)
186-188: Enhance loading state feedback.The submit button could provide better visual feedback during submission.
Consider adding a loading spinner and disabled state:
- <Button type="submit" disabled={createReleaseChannel.isPending}> - Create - </Button> + <Button + type="submit" + disabled={createReleaseChannel.isPending} + className="min-w-[80px]" + > + {createReleaseChannel.isPending ? ( + <span className="flex items-center gap-2"> + <span className="animate-spin">⌛</span> + Creating... + </span> + ) : ( + "Create" + )} + </Button>packages/db/src/schema/release.ts (2)
65-67: Consider explicit null handling for releaseFilter schema.The table definition allows
releaseFilterto be null, but the schema validation might not handle this case explicitly. Consider updating the schema to clearly handle null values:export const createReleaseChannel = createInsertSchema(releaseChannel, { - releaseFilter: releaseCondition, + releaseFilter: releaseCondition.nullable(), }).omit({ id: true });
65-68: Add JSDoc documentation for schema constants.These schema constants are crucial for release channel management. Consider adding JSDoc documentation to improve maintainability:
+/** + * Schema for creating a new release channel. + * Validates the input data and ensures type safety. + * @remarks The releaseFilter field is validated against the releaseCondition schema + */ export const createReleaseChannel = createInsertSchema(releaseChannel, { releaseFilter: releaseCondition, }).omit({ id: true }); + +/** + * Schema for updating an existing release channel. + * Allows partial updates of the release channel fields. + */ export const updateReleaseChannel = createReleaseChannel.partial();packages/auth/src/utils/rbac.ts (1)
102-118: Add error handling and documentation.While the implementation follows the established pattern correctly, consider these improvements:
- Add error handling for non-existent release channel IDs
- Add JSDoc documentation for consistency with other functions
Consider adding:
+/** + * Returns the scope hierarchy for a release channel, including its parent deployment, + * system, and workspace scopes. + */ const getReleaseChannelScopes = async (id: string) => { + if (!id) throw new Error("Release channel ID is required"); const result = await dbpackages/api/src/router/deployment.ts (1)
145-150: RedundantorderByClause When Fetching by IDIn the
byIdprocedure, the use of.orderBy(releaseChannel.name)is unnecessary since fetching by a unique ID will return at most one record.Recommend removing the
orderByclause to simplify the query:ctx.db .select() .from(releaseChannel) .where(eq(releaseChannel.id, input)) - .orderBy(releaseChannel.name) .then(takeFirst),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
- apps/webservice/src/app/[workspaceSlug]/_components/ReleaseBadgeList.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/useReleaseChannelDrawer.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx (4 hunks)
- apps/webservice/src/app/[workspaceSlug]/layout.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/ReleaseChannelsTable.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx (1 hunks)
- packages/api/src/router/deployment.ts (3 hunks)
- packages/auth/src/utils/rbac.ts (3 hunks)
- packages/db/src/schema/rbac.ts (1 hunks)
- packages/db/src/schema/release.ts (1 hunks)
- packages/validators/src/auth/index.ts (1 hunks)
🔇 Additional comments (26)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx (1)
4-4: Verify ReleaseChannelsTable component integration.Let's verify that the ReleaseChannelsTable component exists and accepts the correct props.
Also applies to: 21-21
✅ Verification successful
ReleaseChannelsTable component integration is properly implemented
The component exists and its integration is correct:
- The component accepts the correct prop type
releaseChannelsof typeReleaseChannel[]- The page component correctly passes the
releaseChannelsdata fetched from the API- The component implementation includes proper table rendering with all necessary fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ReleaseChannelsTable component implementation # Test: Check if ReleaseChannelsTable component exists and its props interface ast-grep --pattern 'interface $props { releaseChannels: $type }' # Test: Check component implementation ast-grep --pattern 'export function ReleaseChannelsTable({ releaseChannels }: $props) { $$$ }'Length of output: 167
Script:
#!/bin/bash # Let's try a different approach to verify the ReleaseChannelsTable component # First, let's locate the ReleaseChannelsTable file fd ReleaseChannelsTable.tsx # Then, let's examine its content rg -A 10 "export.*ReleaseChannelsTable" --type tsx # Also check for any Props/Properties interface rg "interface.*Props.*{" -A 5 --type tsx --glob "*ReleaseChannelsTable*" # Check how it's being used in the page component rg "<ReleaseChannelsTable" -A 3 --type tsxLength of output: 492
Script:
#!/bin/bash # Let's fix the command syntax and try again # Check the content of ReleaseChannelsTable component cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/ReleaseChannelsTable.tsx" # Check how it's being used in the page component cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx"Length of output: 2721
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/useReleaseChannelDrawer.ts (1)
5-9: LGTM! Hook initialization follows best practices.The hook properly initializes Next.js navigation utilities and retrieves the release channel ID from URL parameters.
apps/webservice/src/app/[workspaceSlug]/_components/ReleaseBadgeList.tsx (2)
12-14: Verify component integration with ReleaseConditionDialog.Let's verify the integration point and prop passing pattern.
✅ Verification successful
Component integration verified - no issues found
The ReleaseBadgeList component is properly integrated in three locations:
- CreateReleaseChannelDialog
- ReleaseFilter
- ReleaseConditionDialog
All instances use the component correctly with the required
releasesprop, and there's no evidence of prop drilling issues. The component follows a clean, direct prop passing pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how ReleaseBadgeList is imported and used # Test 1: Find direct imports of ReleaseBadgeList echo "=== Direct imports ===" rg -l "import.*ReleaseBadgeList" --type ts --type tsx # Test 2: Find usage patterns echo -e "\n=== Usage patterns ===" rg "<ReleaseBadgeList" -A 3 --type ts --type tsx # Test 3: Check for potential prop drilling echo -e "\n=== Potential prop drilling ===" rg "releases.*items.*total" -A 3 --type ts --type tsxLength of output: 441
Script:
#!/bin/bash # Description: Check how ReleaseBadgeList is imported and used (corrected file types) # Test 1: Find direct imports of ReleaseBadgeList echo "=== Direct imports ===" rg -l "import.*ReleaseBadgeList" # Test 2: Find usage patterns echo -e "\n=== Usage patterns ===" rg "<ReleaseBadgeList" -A 3 # Test 3: Check for potential prop drilling echo -e "\n=== Potential prop drilling ===" rg "releases.*items.*total" -A 3Length of output: 2768
5-10: 🛠️ Refactor suggestionConsider enhancing type safety with additional constraints.
The type definition could be improved to handle edge cases and provide better type safety.
Consider this enhanced type definition:
type ReleaseBadgeListProps = { releases: { - items: SCHEMA.Release[]; + items: readonly SCHEMA.Release[]; total: number; - }; + } & { + // Ensure total is always >= items length + total: number extends infer T + ? T extends number + ? T >= items["length"] + : never + : never; + }; };Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (3)
9-9: LGTM! Import follows consistent patterns.The import statement follows the established pattern of co-locating dialog components with their related features.
30-36: LGTM! UI structure follows consistent patterns.The CreateReleaseChannelDialog implementation follows the established patterns for dialog components and maintains consistent button styling.
38-38: Verify the navigation conditions are comprehensive.The condition ensures mutual exclusivity between variables, release channels, and releases views.
✅ Verification successful
Based on the search results, I can see that the navigation paths in the codebase are well-defined. Looking at the
DeploymentNavBar.tsxfile, there are three main navigation paths:
/releases- tracked byisReleasesActive/variables- tracked byisVariablesActive/release-channels- tracked byisReleaseChannelsActiveLet me verify if there are any other navigation paths by searching specifically for route definitions:
Let me try a different approach to verify the route structure:
Let me try one final search to verify the routes:
Navigation conditions are correctly implemented and comprehensive
The condition
!isVariablesActive && !isReleaseChannelsActiveis correct as it's part of the navigation system that handles all possible routes, which are clearly defined in the codebase:
/variables- Variables page/release-channels- Release channels page/releases- Releases page (default when neither variables nor release channels are active)/configure/job-agent- Configuration page/targets- Targets pageThe mutual exclusivity check between variables and release channels is appropriate, as these represent distinct navigation states, and the condition helps determine when to show release-related content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all possible navigation paths are handled # Test: Search for navigation path definitions to ensure we haven't missed any cases rg -g '*.tsx' -g '*.ts' "release-channels|variables|releases" --type-add 'tsx:*.tsx' --type tsxLength of output: 38236
Script:
#!/bin/bash # Search for route definitions in the deployment slug directory fd . "apps/webservice/src/app/\[workspaceSlug\]/systems/\[systemSlug\]/deployments/\[deploymentSlug\]" --type dLength of output: 296
Script:
#!/bin/bash # Search for page.tsx files in the deployment slug directory to identify routes rg -g "page.tsx" "export.*page" -A 5 "apps/webservice/src/app/.*deployments/.*/"Length of output: 162
Script:
#!/bin/bash # Search for all page.tsx files in the deployments directory structure fd page.tsx "apps/webservice/src/app" --type f | grep "deployments"Length of output: 1043
apps/webservice/src/app/[workspaceSlug]/layout.tsx (2)
9-9: LGTM!The import follows the established pattern and is properly organized with other drawer imports.
41-41: Verify z-index stacking order with other drawers.The component placement is correct. However, since multiple drawers can potentially be opened simultaneously, ensure proper z-index handling to prevent overlay issues.
Let's verify the z-index configuration:
✅ Verification successful
Z-index configuration is properly handled
The Drawer component is built on top of Radix UI's Dialog primitive and has proper z-index handling:
- The DrawerOverlay has
z-50ensuring it sits above regular content- The DrawerContent also has
z-50and is rendered in a Portal, which ensures proper stacking context- All drawers in the application use this same base component, so they will share consistent z-index values
The implementation uses Radix UI's battle-tested primitives which handle multiple open dialogs correctly through their portal and overlay system. No z-index conflicts will occur between different drawer instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check z-index configuration in drawer components # Expected: Consistent z-index values across drawer components # Search for z-index related styles in drawer components rg -g '**/*drawer*.tsx' -g '**/*drawer*.css' -g '**/*drawer*.scss' 'z-index|zIndex' -A 2Length of output: 90
Script:
#!/bin/bash # Let's broaden our search to find drawer-related components and their z-index configurations # First, let's find the ReleaseChannelDrawer component rg -g '*.tsx' -g '*.css' -g '*.scss' 'ReleaseChannelDrawer' -A 5 # Also search for any drawer-related styles in the codebase rg -g '*.tsx' -g '*.css' -g '*.scss' 'drawer.*{.*z-index|.*zIndex|position:\s*fixed}' -A 2 # Search for any styled-components or CSS-in-JS definitions related to drawers rg -g '*.tsx' -g '*.ts' 'styled.*drawer|createStyles.*drawer' -A 5Length of output: 6744
Script:
#!/bin/bash # Let's try a different approach to find drawer styling # Search for Drawer component imports and their related styling rg -g '*.tsx' -g '*.ts' 'import.*Drawer.*from' -B 2 -A 5 # Look for any z-index related styles in the UI components rg -g '*.tsx' -g '*.css' -g '*.scss' 'z-index:|zIndex:' -B 2 -A 2 # Search for drawer-related component definitions ast-grep --pattern 'const $_ = styled.div` $$$ `'Length of output: 25900
Script:
#!/bin/bash # Let's check the Drawer component implementation from the UI package cat packages/ui/src/drawer.tsx # Also search for any Dialog/Sheet related z-index configurations since Drawer might be using these internally rg -g '*.tsx' -g '*.css' 'z-index:' -B 2 -A 2 packages/ui/src/Length of output: 3537
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (1)
24-27: Verify field length constraints with database schemaThe validation schema should align with database constraints. Consider adding maximum length validation for both name and description fields.
✅ Verification successful
Schema validation aligns with database constraints
The database schema uses
texttype for bothnameanddescriptionfields, which doesn't impose specific length limits. The current validation schema requiring a non-empty string fornameand optional string fordescriptionis sufficient and consistent with the database constraints.
- Database schema at
packages/db/drizzle/0026_blushing_kang.sql:"name" text NOT NULL, "description" text DEFAULT '',🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check database schema for field constraints on ReleaseChannel table # Search for ReleaseChannel schema definition ast-grep --pattern 'createTable("release_channel", (table) => { $$$ })'Length of output: 75
Script:
#!/bin/bash # Let's try different approaches to find the database schema # Search for migration files related to release channel fd -e sql -e ts migration | grep -i "release.*channel" || true # Search for table definition using ripgrep rg -i "create.*table.*release.*channel" -A 10 || true # Search for Prisma schema definition fd schema.prisma | xargs cat || trueLength of output: 2299
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx (2)
1-15: LGTM! Well-organized imports.The imports are properly organized and follow good practices, with clear separation between external dependencies and internal modules.
16-69: Verify workspace context integration.Since this component is within a workspace-specific route, ensure it properly handles workspace context and permissions.
✅ Verification successful
Workspace context is properly handled through the API layer
The verification shows that workspace context is correctly managed:
- The release channel API endpoints are protected and include workspace-level checks
- Workspace context is enforced through database joins and where clauses in the API implementation
- The component is placed within the workspace-scoped route
[workspaceSlug]- API calls automatically include workspace context through the router structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for workspace context usage and permission checks rg -A 3 "useWorkspace|usePermissions" "apps/webservice/src/app/\[workspaceSlug\]/_components/release-channel-drawer/"Length of output: 237
Script:
#!/bin/bash # Let's first verify if the file exists and its correct path fd "ReleaseChannelDrawer.tsx" --type f # Then check for workspace context usage in the correct file rg -A 3 "useWorkspace|usePermissions" $(fd "ReleaseChannelDrawer.tsx" --type f) # Also check the store/hooks that manage the drawer state fd "useReleaseChannelDrawer" --type f # Check the content of the hook file to understand the context management rg -A 5 "useReleaseChannelDrawer" $(fd "useReleaseChannelDrawer" --type f)Length of output: 852
Script:
#!/bin/bash # Let's check how the release channel API is being used and if it includes workspace context ast-grep --pattern 'api.deployment.releaseChannel.byId.useQuery' # Check the API router definition for release channels fd "deployment.ts" --type f rg -A 5 "releaseChannel.*byId" $(fd "deployment.ts" --type f) # Check if there are any workspace-level checks in the API implementation rg -A 5 "workspaceId|workspace" $(fd "deployment.ts" --type f)Length of output: 9400
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx (2)
22-23: LGTM: Clean implementation of new imports and propsThe new imports and the optional
deploymentIdprop are well-structured and properly typed.Also applies to: 28-28, 35-35
23-23: Verify ReleaseBadgeList integrationLet's ensure the ReleaseBadgeList component properly handles all release data types.
Also applies to: 68-68
✅ Verification successful
Let me gather more information about the component's integration and type definitions.
Integration is properly implemented
The ReleaseBadgeList component is correctly integrated in ReleaseConditionDialog:
- The component's props type (
ReleaseBadgeListProps) expects areleasesobject withitemsandtotalfields- The usage in ReleaseConditionDialog passes the
releasesprop with proper null check- The component renders a list of release badges using the expected data structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check ReleaseBadgeList implementation and its usage ast-grep --pattern 'type $props = { releases: $_ }' rg -A 10 "export .* ReleaseBadgeList"Length of output: 1233
Script:
#!/bin/bash # Check ReleaseBadgeListProps type definition and usage in ReleaseConditionDialog ast-grep --pattern 'type ReleaseBadgeListProps = { $_ }' rg -A 5 "ReleaseBadgeList" "apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionDialog.tsx"Length of output: 1201
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
1-26: LGTM! Well-structured imports and type definitions.The type definitions are properly typed with clear interfaces, and the imports are organized logically.
Also applies to: 27-31, 83-86
packages/validators/src/auth/index.ts (2)
81-86: Verify predefined roles behavior with new permissions.The new permissions will be automatically included in the predefined roles:
- Admin role: Gets all new permissions
- Viewer role: Gets
ReleaseChannelGetandReleaseChannelList(via.getand.listfilter)- No Access role: Gets no new permissions
This behavior aligns with the intended role-based access control.
Let's verify the role definitions are working as expected:
✅ Verification successful
Role definitions automatically handle new ReleaseChannel permissions correctly
The verification confirms the permissions will work as expected because:
- Admin role gets all permissions via
Object.values(Permission)- Viewer role automatically includes any permission ending with
.getor.listvia the filtera.includes(".list") || a.includes(".get")- No Access role has no permission assignments, so new permissions are excluded by default
The implementation uses a pattern-based approach rather than explicit permission lists, ensuring new permissions are automatically handled according to their naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify role definitions with new permissions # Expected: Find role definitions and permission assignments # Test: Search for role definitions and permission assignments rg "permissions.*ReleaseChannel" --type tsLength of output: 44
Script:
#!/bin/bash # Let's try a broader search to find role definitions and permissions # Search for predefined roles rg "Admin|Viewer|NoAccess.*role" --type ts -A 5 # Search for permission enum definitions rg "enum.*Permission" --type ts -A 10 # Search for role-based permission assignments ast-grep --pattern 'const $_ = { $$$ permissions: $$$ }'Length of output: 1621
81-85: LGTM! New permissions follow established patterns.The new release channel permissions are well-structured and follow the existing naming conventions and patterns. They provide complete CRUD operation coverage which aligns with the PR objectives for initializing release channels UI.
Let's verify that these permissions are properly utilized in the codebase:
✅ Verification successful
Let me verify the predefined roles to ensure they properly incorporate these new permissions.
Let me try a different approach to verify the role definitions.
New release channel permissions are properly integrated and utilized
The verification confirms that:
- The permissions are correctly defined in the Permission enum
- They are automatically included in predefined roles:
- Viewer role gets
ReleaseChannelGetandReleaseChannelList(via.getand.listfilter)- Admin role gets all release channel permissions
- No Access role remains permission-less
- The permissions are actively used in
packages/api/src/router/deployment.tsfor authorization checks🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new release channel permissions # Expected: Find references in components and API routes # Test 1: Search for permission usage in components and routes rg "ReleaseChannel(Get|List|Create|Update|Delete)" --type ts # Test 2: Verify permission checks in API routes ast-grep --pattern 'checkPermission($_, "releaseChannel.$_")'Length of output: 1013
Script:
#!/bin/bash # Search for role definitions and their permissions ast-grep --pattern 'export const $_ = { $$$ permissions: $_ $$$ }' # Also search for any direct references to roles with these permissions rg -A 5 -B 5 "permissions.*ReleaseChannel" --type tsLength of output: 129
Script:
#!/bin/bash # Search for role definitions in auth-related files fd "auth" --type f --exec rg -A 10 "export const (Admin|Viewer|NoAccess)" {} # Search specifically in the auth index file cat packages/validators/src/auth/index.tsLength of output: 3772
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (1)
1-23: LGTM! Well-organized imports and clean type definition.The imports are logically grouped and the component's props are clearly defined.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
55-55: LGTM: Clean implementation of release channels state managementThe new URL construction and active state handling for release channels is implemented correctly and follows the existing pattern.
Also applies to: 61-66
89-98: LGTM: Well-structured Release channels navigation itemThe new Release channels navigation item maintains consistency with other navigation items in terms of structure and styling.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (1)
1-46: LGTM! Clean imports and well-defined types.The imports are organized logically, and the props interface is clear and concise.
packages/db/src/schema/release.ts (1)
68-68: Consider field-specific update restrictions.The current update schema allows partial updates of all fields. Consider if certain fields should be immutable after creation or have specific update validation rules.
Let's check if there are any field-specific update restrictions in the router implementation:
✅ Verification successful
No field-specific update restrictions are needed at the schema level
After examining the code, I can see that the release channel schema and its usage in the router are appropriately designed:
- The schema only includes basic fields (name, description, deploymentId, releaseFilter) that are all safe to update
- The router implementation properly validates permissions before updates
- The fields that could be sensitive (like id) are already excluded from the update schema
- The partial update approach is appropriate for this use case as all fields can be safely updated independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the router implementation to verify update field handling rg -A 10 "releaseChannelRouter.*update"Length of output: 41
Script:
#!/bin/bash # Let's first find the router file related to release channels fd "release.*router" --type f # Then let's look for any update-related code patterns in the schema and router files rg -l "update.*release" # Also search for the router implementation using a different pattern rg -A 10 "router.*release.*update"Length of output: 132
Script:
#!/bin/bash # Let's examine the release schema definition to understand the field structure cat packages/db/src/schema/release.ts # Also check the deployment router file that was found cat packages/api/src/router/deployment.tsLength of output: 21357
packages/auth/src/utils/rbac.ts (2)
14-14: LGTM: Import added correctly.The
releaseChannelimport is properly placed and follows the existing import pattern.
383-383: LGTM: Handler registered correctly.The release channel scope handler is properly integrated into the
scopeHandlersobject.packages/api/src/router/deployment.ts (2)
74-87: Prevent Updating theidField inupdateReleaseChannelSchemaIn the
updateprocedure, ensure that theupdateReleaseChannelschema does not allow theidfield to be updated throughinput.data. Updating the primary key can lead to inconsistencies and should be prevented.Run the following script to confirm that the
idfield is not included in theupdateReleaseChannelschema:#!/bin/bash # Description: Verify that 'updateReleaseChannel' schema does not include the 'id' field. # Test: Search for the 'id' field in the 'updateReleaseChannel' schema definition. rg -A 5 'const updateReleaseChannel' | rg "'id'" # Expected result: The 'id' field should not be present in the schema.
117-126: EnsurereleaseMatchesConditionHandles Undefined Filters SafelyThe variable
filtermay beundefinedifchannel.releaseFilterisnull. Ensure that thereleaseMatchesConditionfunction can handleundefinedinputs without causing errors.Run the following script to verify how
releaseMatchesConditionhandles undefined filters:✅ Verification successful
From the search results, I can see the implementation of
releaseMatchesConditioninpackages/db/src/schema/release.ts. Let me verify its implementation more thoroughly.
releaseMatchesConditionSafely Handles Undefined FiltersThe implementation in
packages/db/src/schema/release.tsshows thatreleaseMatchesConditionexplicitly handles undefined filters:export function releaseMatchesCondition( tx: Tx, condition?: ReleaseCondition, ): SQL<unknown> | undefined { return condition == null || Object.keys(condition).length === 0 ? undefined : buildCondition(tx, condition); }The function safely:
- Accepts an optional
conditionparameter- Returns
undefinedwhen the filter isnull,undefined, or an empty object- Only builds the condition when valid filter data is provided
This implementation correctly handles the case where
channel.releaseFilterisnullin the original code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'releaseMatchesCondition' handles undefined filters safely. # Test: Search for the definition of 'releaseMatchesCondition' and check its handling of undefined filters. rg -A 10 'function releaseMatchesCondition' | rg -B 5 'if.*filter.*undefined' # Expected result: The function should include checks for undefined filters to prevent errors.Length of output: 203
Script:
#!/bin/bash # Let's try to find the releaseMatchesCondition function definition using ast-grep ast-grep --pattern 'function releaseMatchesCondition($_) { $$$ }' # Also search with ripgrep for any occurrence of this function rg -A 15 'releaseMatchesCondition'Length of output: 11033
Script:
#!/bin/bash # Get the full implementation of releaseMatchesCondition and the buildCondition function it uses cat packages/db/src/schema/release.tsLength of output: 7961
.../[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx
Show resolved
Hide resolved
.../[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx
Show resolved
Hide resolved
.../[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx
Show resolved
Hide resolved
...ervice/src/app/[workspaceSlug]/_components/release-channel-drawer/useReleaseChannelDrawer.ts
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/ReleaseBadgeList.tsx
Show resolved
Hide resolved
...ms/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx
Show resolved
Hide resolved
...ms/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx
Show resolved
Hide resolved
| const schema = z.object({ | ||
| name: z.string().min(1), | ||
| description: z.string().optional(), | ||
| releaseFilter: releaseCondition | ||
| .optional() | ||
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | ||
| }); | ||
|
|
||
| const getFinalFilter = (filter?: ReleaseCondition) => | ||
| filter != null ? (isEmptyCondition(filter) ? undefined : filter) : undefined; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance form validation constraints.
The current schema validation could be strengthened to prevent potential issues:
Consider applying these additional constraints:
const schema = z.object({
- name: z.string().min(1),
- description: z.string().optional(),
+ name: z.string()
+ .min(1)
+ .max(50)
+ .regex(/^[a-zA-Z0-9-_]+$/, 'Only alphanumeric characters, hyphens, and underscores are allowed'),
+ description: z.string().max(200).optional(),
releaseFilter: releaseCondition
.optional()
.refine((cond) => cond == null || isValidReleaseCondition(cond)),
});📝 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 schema = z.object({ | |
| name: z.string().min(1), | |
| description: z.string().optional(), | |
| releaseFilter: releaseCondition | |
| .optional() | |
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | |
| }); | |
| const getFinalFilter = (filter?: ReleaseCondition) => | |
| filter != null ? (isEmptyCondition(filter) ? undefined : filter) : undefined; | |
| const schema = z.object({ | |
| name: z.string() | |
| .min(1) | |
| .max(50) | |
| .regex(/^[a-zA-Z0-9-_]+$/, 'Only alphanumeric characters, hyphens, and underscores are allowed'), | |
| description: z.string().max(200).optional(), | |
| releaseFilter: releaseCondition | |
| .optional() | |
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | |
| }); | |
| const getFinalFilter = (filter?: ReleaseCondition) => | |
| filter != null ? (isEmptyCondition(filter) ? undefined : filter) : undefined; |
| const getReleaseChannelScopes = async (id: string) => { | ||
| const result = await db | ||
| .select() | ||
| .from(workspace) | ||
| .innerJoin(system, eq(system.workspaceId, workspace.id)) | ||
| .innerJoin(deployment, eq(deployment.systemId, system.id)) | ||
| .innerJoin(releaseChannel, eq(releaseChannel.deploymentId, deployment.id)) | ||
| .where(eq(releaseChannel.id, id)) | ||
| .then(takeFirst); | ||
|
|
||
| return [ | ||
| { type: "releaseChannel" as const, id: result.release_channel.id }, | ||
| { type: "deployment" as const, id: result.deployment.id }, | ||
| { type: "system" as const, id: result.system.id }, | ||
| { type: "workspace" as const, id: result.workspace.id }, | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Missing scope type definition for release channels
The verification revealed that:
- Release channel permissions are properly defined in
packages/validators/src/auth/index.ts - However, "releaseChannel" is not included in the scope types enum in
packages/db/src/schema/rbac.ts, which is required for the RBAC system to work correctly - This mismatch could lead to authorization issues when accessing release channel resources
Required changes:
- Add "releaseChannel" to the scope types enum in
packages/db/src/schema/rbac.ts
🔗 Analysis chain
Verify integration with permissions system.
Let's ensure the release channel scope type is properly integrated with the permissions system.
Also applies to: 383-383
Let me gather more information about the schema definitions since the previous path was incorrect.
Let me check the scope type schema definition to ensure it includes the release channel type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify release channel scope type and permissions integration
# Test 1: Check if releaseChannel is defined in the schema's ScopeType
echo "Checking ScopeType definition..."
rg -A 5 "export (type|interface) ScopeType" packages/db/schema
# Test 2: Check if release channel permissions are properly defined
echo "Checking permission definitions..."
rg -A 10 "ReleaseChannel(Get|List|Create|Update|Delete)" packages/validators/src/auth
Length of output: 1439
Script:
#!/bin/bash
# Search for ScopeType definition in the entire codebase
echo "Searching for ScopeType definition..."
rg -A 5 "export (type|interface) ScopeType"
# Search for schema-related files that might contain the scope types
echo "Looking for schema files..."
fd "schema.(ts|js)" --type f
# Check for releaseChannel type definitions
echo "Checking releaseChannel type definitions..."
rg -A 5 "type.*releaseChannel"
Length of output: 3853
Script:
#!/bin/bash
# Check the scope type schema definition
echo "Checking scope type schema definition..."
rg -B 5 -A 5 "scopeTypeSchema" packages/db/src/schema/rbac.ts
# Check for any scope type validations
echo "Checking scope type validations..."
rg -A 5 "z\.enum.*releaseChannel" packages/db/src/schema/rbac.ts
Length of output: 578
There was a problem hiding this comment.
the scope is already added in this file, you hallucinating
There was a problem hiding this comment.
@adityachoudhari26, you are correct. The releaseChannel scope type is already included in packages/db/src/schema/rbac.ts. I apologize for the oversight. Thank you for pointing that out.
(_/)
( •_•)
/ ▶
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.037Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
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 promises = channels.map(async (channel) => { | ||
| const filter = channel.releaseFilter ?? undefined; | ||
| const total = await ctx.db | ||
| .select({ count: count() }) | ||
| .from(release) | ||
| .where( | ||
| and( | ||
| eq(release.deploymentId, channel.deploymentId), | ||
| releaseMatchesCondition(ctx.db, filter), | ||
| ), | ||
| ) | ||
| .then(takeFirst) | ||
| .then((r) => r.count); | ||
| return { ...channel, total }; | ||
| }); | ||
| return Promise.all(promises); |
There was a problem hiding this comment.
Potential N+1 Database Queries in list.byDeploymentId
In the list.byDeploymentId procedure, a separate database query is executed for each release channel to obtain the count of related releases. This could lead to performance issues due to multiple database calls (N+1 problem), especially when dealing with a large number of release channels.
Consider optimizing this by performing a single aggregated query. You can use a GROUP BY clause to fetch the counts for all channels in one database call. This will reduce the number of queries and improve performance.
There was a problem hiding this comment.
required due to the limitation of the releaseMatchesCondition
There was a problem hiding this comment.
@adityachoudhari26, understood. Thank you for explaining that the limitations of releaseMatchesCondition require this approach.
🐰🤔
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.133Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
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 getFinalFilter = (filter?: ReleaseCondition) => | ||
| filter != null ? (isEmptyCondition(filter) ? undefined : filter) : undefined; |
There was a problem hiding this comment.
| const getFinalFilter = (filter?: ReleaseCondition) => | |
| filter != null ? (isEmptyCondition(filter) ? undefined : filter) : undefined; | |
| const getFinalFilter = (filter?: ReleaseCondition) => | |
| filter && !isEmptyCondition(filter) ? filter : undefined; |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
95-110: Enhance dropdown accessibility.
The dropdown menu needs better keyboard navigation and ARIA support:
Apply these accessibility improvements:
- <DropdownMenu open={open} onOpenChange={setOpen}>
+ <DropdownMenu open={open} onOpenChange={setOpen} modal={true}>
- <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
+ <DropdownMenuTrigger asChild aria-label="Release channel actions">
+ {children}
+ </DropdownMenuTrigger>
<DropdownMenuContent>
<DeleteReleaseChannelDialog
releaseChannelId={releaseChannelId}
onClose={() => setOpen(false)}
>
<DropdownMenuItem
className="flex cursor-pointer items-center gap-2"
onSelect={(e) => e.preventDefault()}
+ role="menuitem"
+ aria-label="Delete release channel"
>
<IconTrash className="h-4 w-4 text-red-500" />
Delete
</DropdownMenuItem>
</DeleteReleaseChannelDialog>
</DropdownMenuContent>
</DropdownMenu>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (1)
186-188: Enhance loading state feedback.
The submit button should show a loading indicator.
- <Button type="submit" disabled={createReleaseChannel.isPending}>
- Create
- </Button>
+ <Button type="submit" disabled={createReleaseChannel.isPending}>
+ {createReleaseChannel.isPending ? (
+ <>
+ <Loader2 className="mr-2 h-4 w-4 animate-spin" />
+ Creating...
+ </>
+ ) : (
+ 'Create'
+ )}
+ </Button>apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (2)
53-53: Consider adding error message display in the form.
The form should display validation and submission errors to users.
<form onSubmit={onSubmit} className="space-y-6 p-6">
+ {error && (
+ <div className="text-red-500 text-sm mb-4">
+ {error}
+ </div>
+ )}
<FormField
// ... existing form fields
/>82-87: Consider enhancing the submit button with a loading state.
The submit button should indicate when the form is being submitted.
-<Button type="submit">Save</Button>
+<Button
+ type="submit"
+ disabled={updateReleaseChannel.isLoading || !form.formState.isDirty}
+>
+ {updateReleaseChannel.isLoading ? 'Saving...' : 'Save'}
+</Button>apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (1)
83-87: Add Accessible Label to Loading Icon
The loading spinner lacks an accessible label, which may hinder users relying on screen readers.
Include an aria-label for better accessibility:
<div className="flex h-full w-full items-center justify-center">
- <IconLoader2 className="h-8 w-8 animate-spin" />
+ <IconLoader2 className="h-8 w-8 animate-spin" aria-label="Loading" />
</div>📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (1 hunks)
- packages/db/drizzle/0027_abandoned_living_mummy.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDrawer.tsx
🔇 Additional comments (16)
packages/db/drizzle/0027_abandoned_living_mummy.sql (2)
1-1: LGTM! Safe enum modification.
The ALTER TYPE statement is syntactically correct and adding a new enum value is a safe, non-breaking change.
1-1: Verify enum value naming convention consistency.
Let's verify that 'releaseChannel' follows the same naming convention as existing enum values.
✅ Verification successful
Naming convention inconsistency detected in scope_type enum
The existing enum values follow camelCase for compound words:
- Single words: 'release', 'target', 'system', 'deployment', 'job'
- Compound words: 'targetProvider', 'targetMetadataGroup', 'environmentPolicy', 'variableSet', 'jobAgent', 'deploymentVariable', 'targetView'
The new value 'releaseChannel' correctly follows this established camelCase convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing scope_type enum values to verify naming convention consistency
# Expected: List of current enum values to compare against
# Look for enum definition in previous migrations
fd -e sql . packages/db | xargs rg -A 5 'CREATE TYPE.*scope_type'
# Look for other enum value additions
fd -e sql . packages/db | xargs rg 'ALTER TYPE.*scope_type.*ADD VALUE'
Length of output: 1174
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseChannelDropdown.tsx (1)
27-31: LGTM! Type definitions are well-structured.
The type definitions are clear, properly typed, and follow TypeScript best practices.
Also applies to: 84-87
packages/db/drizzle/meta/_journal.json (1)
194-199: LGTM! Migration entry follows the established pattern.
The new migration entry maintains consistency with existing entries and follows proper sequential ordering.
Let's verify the migration ordering and uniqueness:
✅ Verification successful
Let me try a simpler verification script to check the migration entries:
Migration entry is valid and properly ordered
The verification confirms that:
- Index values are sequential (idx 27 follows the proper order)
- Timestamps are monotonically increasing
- No duplicate timestamps exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration entries are properly ordered and have unique timestamps
# Expected: No duplicate idx values, timestamps are in ascending order
# Extract and check idx values and timestamps
jq -r '.entries | sort_by(.idx) | . as $sorted |
if . == $sorted then
"✓ IDX values are properly ordered\n"
else
"❌ IDX values are not in sequence\n"
end |
if (map(.when) | sort | . == map(.when)) then
. + "✓ Timestamps are in ascending order"
else
. + "❌ Timestamps are not in ascending order"
end' packages/db/drizzle/meta/_journal.json
# Check for duplicate timestamps
jq -r '.entries | group_by(.when) | map(select(length > 1)) |
if length > 0 then
"❌ Found duplicate timestamps"
else
"✓ No duplicate timestamps"
end' packages/db/drizzle/meta/_journal.json
Length of output: 704
Script:
#!/bin/bash
# Get the raw entries and verify ordering
jq -r '.entries[] | "\(.idx) \(.when)"' packages/db/drizzle/meta/_journal.json | \
awk '
BEGIN { prev_idx=-1; prev_when=0; ordered=1; timestamps=1 }
{
if ($1 != prev_idx + 1) {
ordered=0
}
if ($2 < prev_when) {
timestamps=0
}
prev_idx=$1
prev_when=$2
}
END {
if (ordered) print "✓ IDX values are sequential"
else print "❌ IDX values are not sequential"
if (timestamps) print "✓ Timestamps are monotonically increasing"
else print "❌ Timestamps are not monotonically increasing"
}'
Length of output: 646
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (2)
74-81:
Add error handling and loading feedback.
The form submission lacks error handling and user feedback.
const onSubmit = form.handleSubmit((data) => {
const filter = getFinalFilter(data.releaseFilter);
+ const toastId = toast.loading('Creating release channel...');
createReleaseChannel
.mutateAsync({ ...data, deploymentId, releaseFilter: filter })
.then(() => {
+ toast.success('Release channel created successfully', { id: toastId });
form.reset();
router.refresh();
setOpen(false);
- });
+ })
+ .catch((error) => {
+ console.error('Failed to create release channel:', error);
+ toast.error('Failed to create release channel', { id: toastId });
+ });
});104-105:
Add unsaved changes protection.
Users might accidentally close the dialog with unsaved changes.
+ const handleOpenChange = (open: boolean) => {
+ if (!open && form.formState.isDirty) {
+ const confirmed = window.confirm('You have unsaved changes. Are you sure you want to close?');
+ if (!confirmed) return;
+ }
+ setOpen(open);
+ };
- <Dialog open={open} onOpenChange={setOpen}>
+ <Dialog open={open} onOpenChange={handleOpenChange}>apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Overview.tsx (8)
1-4: LGTM!
The imports are correctly organized and follow the project's conventions.
6-17: LGTM!
The imports from @ctrlplane/ui/* are correctly organized and follow the project's conventions.
19-19: LGTM!
The import of the api object from ~/trpc/react is correct.
21-23: LGTM!
The OverviewProps type is correctly defined and uses the SCHEMA.ReleaseChannel type from @ctrlplane/db/schema.
25-28: LGTM!
The schema object is correctly defined using zod for form validation. The field validations are appropriate:
nameis a required string with a minimum length of 1 and a maximum length of 50.descriptionis an optional string with a maximum length of 1000.
30-38: LGTM!
- The
Overviewcomponent is correctly defined as a functional component with theOverviewPropstype. - The
defaultValuesobject is correctly initialized from thereleaseChannelprop. - The
useFormhook is correctly used with theschemaanddefaultValues. - The
routerandutilsobjects are correctly initialized.
51-91: LGTM!
- The
Formcomponent is correctly used with theformobject. - The
onSubmitfunction is correctly passed to theformelement. - The
FormFieldcomponents are correctly used for thenameanddescriptionfields. - The
Buttoncomponent is correctly used for form submission, with thedisabledprop correctly set based on the mutation status and form state.
39-49: 🛠️ Refactor suggestion
Consider handling loading and error states for better user experience.
The mutation handling should provide feedback to users about the operation status. You can add loading and error states to enhance the user experience.
const updateReleaseChannel =
api.deployment.releaseChannel.update.useMutation();
+const [error, setError] = React.useState<string | null>(null);
const onSubmit = form.handleSubmit((data) =>
updateReleaseChannel
.mutateAsync({ id: releaseChannel.id, data })
.then(() => form.reset(data))
.then(() =>
utils.deployment.releaseChannel.byId.invalidate(releaseChannel.id),
)
- .then(() => router.refresh()),
+ .then(() => {
+ router.refresh();
+ toast.success('Release channel updated successfully');
+ })
+ .catch((err) => {
+ setError(err.message);
+ toast.error('Failed to update release channel');
+ }),
);Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/ReleaseFilter.tsx (2)
53-54: Add Error Handling to Mutation
The updateReleaseChannel mutation lacks error handling, which could lead to unhandled exceptions.
This issue has been previously noted. Refer to the past review comment on lines 63-71 for a detailed solution.
28-42:
Sanitize URL Parameters to Prevent Injection
The URL constructed in getReleaseFilterUrl uses parameters that could be manipulated. Ensure all parameters are properly encoded to prevent security issues.
Implement encoding for all URL components:
const getReleaseFilterUrl = (
workspaceSlug: string,
systemSlug?: string,
deploymentSlug?: string,
filter?: ReleaseCondition,
) => {
if (filter == null || systemSlug == null || deploymentSlug == null)
return null;
- const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`;
+ const baseUrl = `/${encodeURIComponent(workspaceSlug)}/systems/${encodeURIComponent(
+ systemSlug
+ )}/deployments/${encodeURIComponent(deploymentSlug)}`;
const filterHash = LZString.compressToEncodedURIComponent(
JSON.stringify(filter),
);
return `${baseUrl}/releases?filter=${filterHash}`;
};Likely invalid or redundant comment.
| const onDelete = () => | ||
| deleteReleaseChannel | ||
| .mutateAsync(releaseChannelId) | ||
| .then(() => removeReleaseChannelId()) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)); |
There was a problem hiding this comment.
Enhance error handling and user feedback for deletion.
The deletion operation needs better error handling and user feedback:
- Add error handling with user notification
- Update the button text to reflect the loading state
- Consider adding a toast notification for successful deletion
Apply this diff to enhance the user experience:
const onDelete = () =>
deleteReleaseChannel
.mutateAsync(releaseChannelId)
.then(() => removeReleaseChannelId())
.then(() => router.refresh())
- .then(() => setOpen(false));
+ .then(() => {
+ setOpen(false);
+ toast.success('Release channel deleted successfully');
+ })
+ .catch((error) => {
+ toast.error('Failed to delete release channel');
+ console.error('Delete error:', error);
+ }); <AlertDialogAction
onClick={onDelete}
disabled={deleteReleaseChannel.isPending}
className={buttonVariants({ variant: "destructive" })}
>
- Delete
+ {deleteReleaseChannel.isPending ? "Deleting..." : "Delete"}
</AlertDialogAction>📝 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 onDelete = () => | |
| deleteReleaseChannel | |
| .mutateAsync(releaseChannelId) | |
| .then(() => removeReleaseChannelId()) | |
| .then(() => router.refresh()) | |
| .then(() => setOpen(false)); | |
| const onDelete = () => | |
| deleteReleaseChannel | |
| .mutateAsync(releaseChannelId) | |
| .then(() => removeReleaseChannelId()) | |
| .then(() => router.refresh()) | |
| .then(() => { | |
| setOpen(false); | |
| toast.success('Release channel deleted successfully'); | |
| }) | |
| .catch((error) => { | |
| toast.error('Failed to delete release channel'); | |
| console.error('Delete error:', error); | |
| }); | |
| <AlertDialogAction | |
| onClick={onDelete} | |
| disabled={deleteReleaseChannel.isPending} | |
| className={buttonVariants({ variant: "destructive" })} | |
| > | |
| {deleteReleaseChannel.isPending ? "Deleting..." : "Delete"} | |
| </AlertDialogAction> |
| const filterHash = | ||
| filter != null | ||
| ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) | ||
| : undefined; | ||
|
|
||
| const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`; | ||
| const releaseFilterUrl = | ||
| filterHash != null | ||
| ? `${baseUrl}/releases?filter=${filterHash}` | ||
| : `${baseUrl}/releases`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize URL generation with useMemo.
The filter URL computation should be memoized to prevent unnecessary recalculations.
+ import { useMemo } from 'react';
- const filterHash =
- filter != null
- ? LZString.compressToEncodedURIComponent(JSON.stringify(filter))
- : undefined;
-
- const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`;
- const releaseFilterUrl =
- filterHash != null
- ? `${baseUrl}/releases?filter=${filterHash}`
- : `${baseUrl}/releases`;
+ const releaseFilterUrl = useMemo(() => {
+ const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`;
+ if (!filter) return `${baseUrl}/releases`;
+
+ const filterHash = LZString.compressToEncodedURIComponent(JSON.stringify(filter));
+ return `${baseUrl}/releases?filter=${filterHash}`;
+ }, [workspaceSlug, systemSlug, deploymentSlug, filter]);📝 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 filterHash = | |
| filter != null | |
| ? LZString.compressToEncodedURIComponent(JSON.stringify(filter)) | |
| : undefined; | |
| const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`; | |
| const releaseFilterUrl = | |
| filterHash != null | |
| ? `${baseUrl}/releases?filter=${filterHash}` | |
| : `${baseUrl}/releases`; | |
| const releaseFilterUrl = useMemo(() => { | |
| const baseUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}`; | |
| if (!filter) return `${baseUrl}/releases`; | |
| const filterHash = LZString.compressToEncodedURIComponent(JSON.stringify(filter)); | |
| return `${baseUrl}/releases?filter=${filterHash}`; | |
| }, [workspaceSlug, systemSlug, deploymentSlug, filter]); |
| const schema = z.object({ | ||
| name: z.string().min(1).max(50), | ||
| description: z.string().max(1000).optional(), | ||
| releaseFilter: releaseCondition | ||
| .optional() | ||
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance schema validation for better data quality.
The current schema validation could be strengthened:
- Add pattern validation for the name field to ensure valid characters
- Consider reducing the description's max length for better UI/UX
const schema = z.object({
- name: z.string().min(1).max(50),
+ name: z.string()
+ .min(1)
+ .max(50)
+ .regex(/^[a-zA-Z0-9-]+$/, 'Only alphanumeric characters and hyphens allowed')
+ .transform((val) => val.toLowerCase()),
- description: z.string().max(1000).optional(),
+ description: z.string().max(250).optional(),
releaseFilter: releaseCondition
.optional()
.refine((cond) => cond == null || isValidReleaseCondition(cond)),
});📝 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 schema = z.object({ | |
| name: z.string().min(1).max(50), | |
| description: z.string().max(1000).optional(), | |
| releaseFilter: releaseCondition | |
| .optional() | |
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | |
| }); | |
| const schema = z.object({ | |
| name: z.string() | |
| .min(1) | |
| .max(50) | |
| .regex(/^[a-zA-Z0-9-]+$/, 'Only alphanumeric characters and hyphens allowed') | |
| .transform((val) => val.toLowerCase()), | |
| description: z.string().max(250).optional(), | |
| releaseFilter: releaseCondition | |
| .optional() | |
| .refine((cond) => cond == null || isValidReleaseCondition(cond)), | |
| }); |
| condition={filter ?? defaultCondition} | ||
| deploymentId={deploymentId} | ||
| onChange={onUpdate} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Passing Unused Props
The deploymentId prop is being passed to ReleaseConditionDialog, but it may not be used inside the dialog component.
If deploymentId is unnecessary, consider removing it:
<ReleaseConditionDialog
condition={filter ?? defaultCondition}
- deploymentId={deploymentId}
onChange={onUpdate}
>Committable suggestion was skipped due to low confidence.
| const { workspaceSlug, systemSlug, deploymentSlug } = useParams<{ | ||
| workspaceSlug: string; | ||
| systemSlug?: string; | ||
| deploymentSlug?: string; | ||
| }>(); |
There was a problem hiding this comment.
Ensure Correct Type Inference for Route Parameters
The useParams hook may return undefined for optional parameters like systemSlug and deploymentSlug. To prevent potential runtime errors, consider using explicit type assertions or default values.
Apply this diff to handle possible undefined values:
const { workspaceSlug, systemSlug, deploymentSlug } = useParams<{
- workspaceSlug: string;
+ workspaceSlug?: string;
systemSlug?: string;
deploymentSlug?: string;
}>();
+if (!workspaceSlug) {
+ throw new Error("workspaceSlug is required");
+}📝 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 { workspaceSlug, systemSlug, deploymentSlug } = useParams<{ | |
| workspaceSlug: string; | |
| systemSlug?: string; | |
| deploymentSlug?: string; | |
| }>(); | |
| const { workspaceSlug, systemSlug, deploymentSlug } = useParams<{ | |
| workspaceSlug?: string; | |
| systemSlug?: string; | |
| deploymentSlug?: string; | |
| }>(); | |
| if (!workspaceSlug) { | |
| throw new Error("workspaceSlug is required"); | |
| } |
| <Link href={releaseFilterUrl} target="_blank"> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className="flex items-center gap-2" | ||
| > | ||
| <IconExternalLink className="h-4 w-4" /> | ||
| View releases | ||
| </Button> | ||
| </Link> | ||
| )} |
There was a problem hiding this comment.
Ensure Proper Usage of Link with Button
Wrapping a Button inside a Link without proper attributes can cause accessibility and styling issues.
Modify the code to use the asChild prop on Button:
{releaseFilterUrl != null && (
- <Link href={releaseFilterUrl} target="_blank">
- <Button
- variant="outline"
- size="sm"
- className="flex items-center gap-2"
- >
+ <Button
+ asChild
+ variant="outline"
+ size="sm"
+ className="flex items-center gap-2"
+ >
+ <Link href={releaseFilterUrl} target="_blank">
<IconExternalLink className="h-4 w-4" />
View releases
- </Button>
- </Link>
+ </Link>
+ </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.
| <Link href={releaseFilterUrl} target="_blank"> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="flex items-center gap-2" | |
| > | |
| <IconExternalLink className="h-4 w-4" /> | |
| View releases | |
| </Button> | |
| </Link> | |
| )} | |
| {releaseFilterUrl != null && ( | |
| <Button | |
| asChild | |
| variant="outline" | |
| size="sm" | |
| className="flex items-center gap-2" | |
| > | |
| <Link href={releaseFilterUrl} target="_blank"> | |
| <IconExternalLink className="h-4 w-4" /> | |
| View releases | |
| </Link> | |
| </Button> | |
| )} |
| const releasesQ = api.release.list.useQuery({ deploymentId, filter }); | ||
| const releases = releasesQ.data; | ||
|
|
There was a problem hiding this comment.
Handle Errors in Release Query
Currently, there's no error handling for the releasesQ query. If the query fails, it could cause the component to break without feedback to the user.
Add error handling to inform the user of any issues:
const releasesQ = api.release.list.useQuery({ deploymentId, filter });
+const { data: releases, isLoading, isError, error } = releasesQ;
-if (loading)
+if (isLoading)
return (
<div className="flex h-full w-full items-center justify-center">
<IconLoader2 className="h-8 w-8 animate-spin" />
</div>
);
+
+if (isError) {
+ return (
+ <div className="p-6">
+ <p className="text-red-500">Failed to load releases: {error.message}</p>
+ </div>
+ );
+}
-const releases = releasesQ.data;Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
ReleaseBadgeList,Overview,ReleaseChannelDrawer,ReleaseChannelDropdown,ReleaseFilter,CreateReleaseChannelDialog, andReleaseChannelsTable.DeploymentNavBar.Bug Fixes
ReleaseConditionDialog.Documentation