fix: Add more to systems page #341
Conversation
WalkthroughThe changes reorganize the project’s component structure and update import paths across several UI files. A previously removed deployment table component is reintroduced under a new path, and new components for deployment handling and release information have been added. Numerous import paths are updated to reflect a revised file structure, and backend logic is simplified by removing legacy queries and adding new procedures. Additionally, database schema changes include the creation of new indexes and a journal entry for tracking schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SDT as SystemDeploymentTable Component
participant API as API Server
participant DT as DeploymentTable Component
participant DD as Dropdown (CreateDeployment/DeleteSystem)
U->>SDT: Scrolls into view
SDT->>API: Fetch environments & deployments
API-->>SDT: Return data
SDT->>DT: Render deployment table
DT->>DD: Render dropdown options (Create/Delete)
U->>DD: Interact with dropdown
sequenceDiagram
participant U as User
participant DOD as DeploymentOptionsDropdown
participant CRD as CreateReleaseDialog
U->>DOD: Clicks "New Release" (IconRocket)
DOD-->>CRD: Open release creation dialog (prevent default)
CRD->>U: Render UI for new release creation
U->>CRD: Submit release details
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
adityachoudhari26
force-pushed
the
systems-page-ui
branch
from
ba44dbb to
bd7f9f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/release-cell/TableCells.tsx (1)
122-265: Consider refactoring the Release component for better maintainability.The component could benefit from:
- Extracting data fetching and transformation logic into a custom hook
- Splitting the visualization logic into a separate component
- Simplifying the lodash chain operations
Example of extracting data fetching into a custom hook:
const useReleaseJobTriggers = ( workspaceSlug: string, releaseId: string, environmentId: string ) => { const workspaceId = api.workspace.bySlug.useQuery(workspaceSlug); const filter: JobCondition = { type: FilterType.Comparison, operator: ComparisonOperator.And, conditions: [ { type: JobFilterType.Release, operator: ColumnOperator.Equals, value: releaseId, }, { type: JobFilterType.Environment, operator: ColumnOperator.Equals, value: environmentId, }, ], }; const releaseJobTriggersQ = api.job.config.byWorkspaceId.list.useQuery( { workspaceId: workspaceId.data?.id ?? "", filter }, { enabled: isPresent(workspaceId.data?.id) }, ); return { isLoading: releaseJobTriggersQ.isLoading, data: releaseJobTriggersQ.data ?? [], }; };apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx (1)
21-83: Reduce style duplication by extracting common styles.The styling pattern "rounded-full p-1 dark:text-black" is repeated across all status icons. Consider extracting these common styles to reduce duplication.
Here's how you could refactor this:
+const baseIconStyles = "rounded-full p-1 dark:text-black"; + export const StatusIcon: React.FC<{ statuses: JobStatusType[]; }> = ({ statuses }) => { const inProgress = statuses.some((s) => s === JobStatus.InProgress); if (inProgress) return ( - <div className="animate-spin rounded-full bg-blue-400 p-1 dark:text-black"> + <div className={`${baseIconStyles} animate-spin bg-blue-400`}> <IconLoader2 strokeWidth={2} className="h-4 w-4" /> </div> ); // ... similar changes for other status checks return ( - <div className="rounded-full bg-green-400 p-1 dark:text-black"> + <div className={`${baseIconStyles} bg-green-400`}> <IconCircleCheck strokeWidth={2} className="h-4 w-4" /> </div> ); };packages/api/src/router/release.ts (1)
451-494: Consider optimizing the database queries.The current implementation makes two separate database calls:
- First to fetch the release channel
- Then to fetch the release
Consider combining these into a single query using joins for better performance.
Here's a suggested optimization:
- const rc = await ctx.db - .select() - .from(environment) - .innerJoin( - environmentPolicy, - eq(environment.policyId, environmentPolicy.id), - ) - .innerJoin( - environmentPolicyReleaseChannel, - eq(environmentPolicyReleaseChannel.policyId, environmentPolicy.id), - ) - .innerJoin( - releaseChannel, - eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), - ) - .where( - and( - eq(environment.id, environmentId), - eq(releaseChannel.deploymentId, deploymentId), - ), - ) - .then(takeFirstOrNull); - - return ctx.db + return ctx.db .select() .from(release) + .leftJoin( + db + .select() + .from(environment) + .innerJoin( + environmentPolicy, + eq(environment.policyId, environmentPolicy.id), + ) + .innerJoin( + environmentPolicyReleaseChannel, + eq(environmentPolicyReleaseChannel.policyId, environmentPolicy.id), + ) + .innerJoin( + releaseChannel, + eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), + ) + .where( + and( + eq(environment.id, environmentId), + eq(releaseChannel.deploymentId, deploymentId), + ), + ) + .as("rc"), + eq(release.deploymentId, deploymentId), + ) .where( and( eq(release.deploymentId, deploymentId), - rc != null - ? releaseMatchesCondition( - ctx.db, - rc.release_channel.releaseFilter, - ) - : undefined, + releaseMatchesCondition( + ctx.db, + sql<any>`rc.release_channel_release_filter`, + ), ), ) .orderBy(desc(release.createdAt)) .limit(1) .then(takeFirstOrNull);Additionally, consider adding error handling for cases where the queries fail:
.query(async ({ ctx, input }) => { const { deploymentId, environmentId } = input; + try { // ... query implementation ... + } catch (error) { + ctx.logger.error("Failed to fetch release", { error, input }); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to fetch release", + }); + } }),apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/SystemDeploymentTable.tsx (1)
73-79: Consider adding loading states for better UX.While the lazy loading implementation is good, the component could benefit from displaying loading states when fetching environments and deployments data.
Add loading states to improve user experience:
<div ref={ref} className="overflow-hidden rounded-md border"> + {(environments.isLoading || deployments.isLoading) && ( + <div className="flex h-32 items-center justify-center"> + <IconLoader2 className="h-6 w-6 animate-spin text-muted-foreground" /> + </div> + )} + {!environments.isLoading && !deployments.isLoading && ( <DeploymentTable workspace={workspace} systemSlug={system.slug} environments={environments.data ?? []} deployments={deployments.data ?? []} /> + )} </div>apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/ReleaseInfo.tsx (2)
37-77: Add prop validation for required fields.Consider adding runtime validation for required props to prevent potential runtime errors.
+import { z } from "zod"; +const ReleaseProps = z.object({ + name: z.string(), + version: z.string(), + releaseId: z.string(), + environment: z.object({ + id: z.string(), + name: z.string() + }), + deployedAt: z.date(), + workspaceSlug: z.string(), + systemSlug: z.string(), + deploymentSlug: z.string(), + statuses: z.array(z.string()) +}); export const Release: React.FC<{ name: string; version: string; releaseId: string; environment: { id: string; name: string }; deployedAt: Date; workspaceSlug: string; systemSlug: string; deploymentSlug: string; statuses: JobStatusType[]; }> = (props) => { + try { + ReleaseProps.parse(props); + } catch (error) { + console.error('Invalid props:', error); + return null; + }
109-180: Refactor link href construction for maintainability.Extract the URL construction logic to improve maintainability and reusability.
+const getReleaseUrl = (params: { + workspaceSlug: string; + systemSlug: string; + deploymentSlug: string; + releaseId: string; +}) => { + const { workspaceSlug, systemSlug, deploymentSlug, releaseId } = params; + return `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/releases/${releaseId}`; +}; <Link - href={`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/releases/${releaseId}`} + href={getReleaseUrl({ workspaceSlug, systemSlug, deploymentSlug, releaseId })} className="flex w-full items-center gap-2" >apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (1)
37-71: Consider memoizing API calls to prevent unnecessary re-renders.The component makes multiple API calls that could benefit from memoization to optimize performance.
+import { useMemo } from 'react'; const { data: resourceInfo, isLoading: isResourceInfoLoading } = - api.resource.byWorkspaceId.list.useQuery( + api.resource.byWorkspaceId.list.useQuery(useMemo(() => ({ { workspaceId: workspace.id, filter: resourceFilter ?? undefined, limit: 0, }, - { enabled: resourceFilter != null }, + { enabled: resourceFilter != null } + }), [workspace.id, resourceFilter]));apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/TableDeployments.tsx (2)
74-140: Enhance table accessibility.The table implementation could benefit from additional ARIA attributes and keyboard navigation support.
-<Table className="w-full min-w-max bg-background"> +<Table + className="w-full min-w-max bg-background" + role="grid" + aria-label="Deployments across environments" +> <TableHeader className="[&_tr]:border-0"> - <TableRow className="hover:bg-transparent"> + <TableRow + className="hover:bg-transparent" + role="row" + > <TableHead className="sticky left-0 z-10 rounded-tl-md py-4 pl-6 backdrop-blur-lg" + role="columnheader" + aria-sort="none" >
92-137: Consider adding empty state handling.The table should handle cases where there are no deployments to display.
<TableBody> + {deployments.length === 0 && ( + <TableRow> + <TableCell + colSpan={environments.length + 1} + className="text-center py-8 text-muted-foreground" + > + No deployments found + </TableCell> + </TableRow> + )} {deployments.map((r, idx) => (
🛑 Comments failed to post (3)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx (1)
64-70:
⚠️ Potential issueFix potentially incorrect status check for rolling out state.
The variable name
isRollingOutsuggests a rolling out state, but it checks forJobStatus.Successful. This appears to be incorrect as successful and rolling out are different states.Apply this diff to fix the status check:
- const isRollingOut = statuses.some((s) => s === JobStatus.Successful); + const isRollingOut = statuses.some((s) => s === JobStatus.RollingOut);📝 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 isRollingOut = statuses.some((s) => s === JobStatus.RollingOut); if (isRollingOut) return ( <div className="rounded-full bg-green-400 p-1 dark:text-black"> <IconCalendarTime strokeWidth={2} className="h-4 w-4" /> </div> );apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/ReleaseInfo.tsx (1)
78-108: 🛠️ Refactor suggestion
Add error handling for failed queries.
The component should handle and display query errors gracefully.
const workspaceId = api.workspace.bySlug.useQuery(workspaceSlug); const releaseJobTriggersQ = api.job.config.byWorkspaceId.list.useQuery( { workspaceId: workspaceId.data?.id ?? "", filter }, { enabled: isPresent(workspaceId.data?.id) }, ); +if (workspaceId.error) { + return ( + <div className="text-sm text-red-500"> + Failed to load workspace: {workspaceId.error.message} + </div> + ); +} + +if (releaseJobTriggersQ.error) { + return ( + <div className="text-sm text-red-500"> + Failed to load job triggers: {releaseJobTriggersQ.error.message} + </div> + ); +}📝 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 workspaceId = api.workspace.bySlug.useQuery(workspaceSlug); const releaseJobTriggersQ = api.job.config.byWorkspaceId.list.useQuery( { workspaceId: workspaceId.data?.id ?? "", filter }, { enabled: isPresent(workspaceId.data?.id) }, ); if (workspaceId.error) { return ( <div className="text-sm text-red-500"> Failed to load workspace: {workspaceId.error.message} </div> ); } if (releaseJobTriggersQ.error) { return ( <div className="text-sm text-red-500"> Failed to load job triggers: {releaseJobTriggersQ.error.message} </div> ); } const releaseJobTriggers = releaseJobTriggersQ.data ?? []; const latestJobsByResource = _.chain(releaseJobTriggers) .groupBy((r) => r.resource.id) .mapValues((triggers) => _.maxBy(triggers, (t) => new Date(t.job.createdAt)), ) .values() .compact() .value(); const data = _.chain(latestJobsByResource) .groupBy((r) => r.job.status) .entries() .map(([name, value]) => ({ name, count: value.length })) .push(...Object.keys(statusColor).map((k) => ({ name: k, count: 0 }))) .unionBy((r) => r.name) .sortBy((r) => getStatusColor(r.name)) .value(); const configuredWithMessages = releaseJobTriggers.filter((d) => [d.job, d.resource, d.job.message].every(isPresent), );apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (1)
185-192: 🛠️ Refactor suggestion
Add error handling for deployment mutation.
The deployment mutation lacks error handling, which could lead to a poor user experience if the deployment fails.
onClick={() => deploy .mutateAsync({ environmentId: environment.id, releaseId: release.id, }) .then(() => router.refresh()) + .catch((error) => { + console.error('Deployment failed:', error); + // Add your error notification system here + // Example: toast.error('Failed to deploy: ' + 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.onClick={() => deploy .mutateAsync({ environmentId: environment.id, releaseId: release.id, }) .then(() => router.refresh()) .catch((error) => { console.error('Deployment failed:', error); // Add your error notification system here // Example: toast.error('Failed to deploy: ' + error.message); }) }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx (1)
32-72: Consider optimizing workspace data fetching.The component fetches workspace data using
api.workspace.bySlug.useQueryeven though the workspace object is available in the parent component (passed toDeploymentTable). Consider passing the workspace ID directly to avoid unnecessary API calls.const EnvIcon: React.FC<{ environment: Environment; workspaceSlug: string; systemSlug: string; + workspaceId: string; className?: string; -}> = ({ environment: env, workspaceSlug, systemSlug, className }) => { - const { data: workspace, isLoading: isWorkspaceLoading } = - api.workspace.bySlug.useQuery(workspaceSlug); - const workspaceId = workspace?.id ?? ""; +}> = ({ environment: env, workspaceSlug, systemSlug, workspaceId, className }) => {apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx (1)
21-83: Refactor duplicate styling.The rounded-full, padding, and dark mode styling is duplicated across all status divs. Consider extracting these common styles to improve maintainability.
+ const baseStyles = "rounded-full p-1 dark:text-black"; + const getStatusStyles = (color: string) => `${baseStyles} bg-${color}-400`; + if (inProgress) return ( - <div className="animate-spin rounded-full bg-blue-400 p-1 dark:text-black"> + <div className={`${getStatusStyles('blue')} animate-spin`}> <IconLoader2 strokeWidth={2} className="h-4 w-4" /> </div> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/SystemDeploymentTable.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/flow-diagram/ApprovalCheck.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentCTA.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/DeploymentPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/release-cell/ReleaseEnvironmentCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/release-cell/TableCells.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/settings/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/charts/DeploymentBarChart.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/dropdown/DeploymentOptionsDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/ReleaseInfo.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/SystemDeploymentTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/page.tsx(1 hunks)packages/api/src/router/deployment.ts(1 hunks)packages/api/src/router/release-deploy.ts(1 hunks)packages/api/src/router/release.ts(1 hunks)packages/db/drizzle/0072_dapper_sphinx.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/SystemDeploymentTable.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/charts/DeploymentBarChart.tsx
- packages/db/drizzle/0072_dapper_sphinx.sql
- packages/api/src/router/release-deploy.ts
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/flow-diagram/ApprovalCheck.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/release-cell/ReleaseEnvironmentCell.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/settings/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentCTA.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/DeploymentPageContent.tsx
- packages/db/drizzle/meta/_journal.json
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/SystemDeploymentTable.tsx
- packages/api/src/router/release.ts
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/system-deployment-table/TableDeployments.tsx
- packages/db/src/schema/release.ts
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/releases/release-cell/TableCells.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/ReleaseInfo.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsxpackages/api/src/router/deployment.tsapps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/dropdown/DeploymentOptionsDropdown.tsxapps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx
🧠 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-11-12T09:55:38.456Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
packages/api/src/router/deployment.ts (2)
558-559: LGTM! Inner join ensures data consistency.The addition of the inner join with the system table ensures that only deployments with valid system references are returned.
565-567: LGTM! Simplified system property assignment.The direct assignment of the system property from the join result maintains type safety and improves readability.
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/dropdown/DeploymentOptionsDropdown.tsx (2)
4-9: LGTM! Clean and well-organized imports.The imports are properly organized, with icons grouped together and components separated appropriately.
Also applies to: 20-20
46-57: LGTM! Well-implemented dropdown menu item.The new "New Release" option is well-integrated into the dropdown menu, following the established patterns for styling, event handling, and component structure.
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx (3)
3-16: LGTM! Clean and well-organized imports.The imports and type definitions are properly structured and follow consistent patterns.
18-30: LGTM! Well-structured reusable component.The Icon component is well-designed with proper type definitions and consistent styling.
74-153: LGTM! Well-structured table component.The DeploymentTable component is well-implemented with:
- Clean and organized structure
- Proper handling of responsive design
- Good use of accessibility attributes
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/StatusIcon.tsx (3)
1-14: LGTM! Well-organized imports.The imports are clean, specific, and all are utilized within the component.
15-17: LGTM! Clean component definition.The component interface is well-typed and focused on its specific purpose.
15-17: Handle empty status array.The component doesn't explicitly handle the case when the status array is empty. Consider adding a default state or prop validation.
export const StatusIcon: React.FC<{ statuses: JobStatusType[]; -}> = ({ statuses }) => { +}> = ({ statuses = [] }) => { + if (statuses.length === 0) { + return ( + <div className="rounded-full bg-neutral-400 p-1 dark:text-black"> + <IconCircleX strokeWidth={2} className="h-4 w-4" /> + </div> + ); + }
| import Link from "next/link"; | ||
| import { IconLoader2 } from "@tabler/icons-react"; | ||
|
|
||
| import { cn } from "@ctrlplane/ui"; | ||
| import { Badge } from "@ctrlplane/ui/badge"; | ||
|
|
||
| import { LazyReleaseEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/release-cell/ReleaseEnvironmentCell"; | ||
| import { DeploymentOptionsDropdown } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/dropdown/DeploymentOptionsDropdown"; | ||
| import { LazyDeploymentEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell"; |
There was a problem hiding this comment.
Fix incorrect import path for LazyDeploymentEnvironmentCell.
The import path and the imported component name don't match. The path ends with DeploymentEnvironmentCell.tsx but imports LazyDeploymentEnvironmentCell.
-import { LazyDeploymentEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell";
+import { LazyDeploymentEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/LazyDeploymentEnvironmentCell";📝 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.
| import { LazyDeploymentEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell"; | |
| import { LazyDeploymentEnvironmentCell } from "~/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/LazyDeploymentEnvironmentCell"; |
| const isRollingOut = statuses.some((s) => s === JobStatus.Successful); | ||
| if (isRollingOut) | ||
| return ( | ||
| <div className="rounded-full bg-green-400 p-1 dark:text-black"> | ||
| <IconCalendarTime strokeWidth={2} className="h-4 w-4" /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Fix potential status check conflict.
The isRollingOut check at line 64 uses some with Successful status, which could conflict with the isComplete check at line 56. This means that if all statuses are successful, both conditions will be true, but only the first one (isComplete) will be rendered due to the early return pattern.
Consider checking specifically for a mix of successful and non-successful statuses for the rolling out state.
- const isRollingOut = statuses.some((s) => s === JobStatus.Successful);
+ const isRollingOut = statuses.some((s) => s === JobStatus.Successful) &&
+ !statuses.every((s) => s === JobStatus.Successful);📝 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 isRollingOut = statuses.some((s) => s === JobStatus.Successful); | |
| if (isRollingOut) | |
| return ( | |
| <div className="rounded-full bg-green-400 p-1 dark:text-black"> | |
| <IconCalendarTime strokeWidth={2} className="h-4 w-4" /> | |
| </div> | |
| ); | |
| const isRollingOut = statuses.some((s) => s === JobStatus.Successful) && | |
| !statuses.every((s) => s === JobStatus.Successful); | |
| if (isRollingOut) | |
| return ( | |
| <div className="rounded-full bg-green-400 p-1 dark:text-black"> | |
| <IconCalendarTime strokeWidth={2} className="h-4 w-4" /> | |
| </div> | |
| ); |
| @@ -0,0 +1 @@ | |||
| CREATE INDEX IF NOT EXISTS "release_created_at_idx" ON "release" USING btree ("created_at"); | |||
There was a problem hiding this comment.
my query times went down by like half when I added this
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (1)
152-185: Improve error handling for async deploy actions.
Thedeploy.mutateAsynccall doesn’t provide any user feedback upon failure. Consider catching errors to display a toast or error message so users are informed if deployment fails.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/flow-diagram/ApprovalCheck.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(1 hunks)packages/api/src/router/release.ts(3 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/flow-diagram/ApprovalCheck.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx
- packages/db/drizzle/meta/_journal.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/release.tsapps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx
🧠 Learnings (1)
packages/api/src/router/release.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/api/src/router/release.ts (1)
23-34: Looks good on these new imports.
No immediate issues with addingenvironmentPolicyApproval,resourceMatchesMetadata, andsystemimports. They are used appropriately below in the new procedure and appear consistent with the rest of the schema-based imports.apps/webservice/src/app/[workspaceSlug]/(appv2)/systems/[systemSlug]/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (2)
82-86: Verify the deployment detection logic.
You currently consider the environment “already deployed” if there’s at least one job status (statuses.length > 0). Verify that this condition truly represents a fully deployed environment (e.g., whether a single failed or incomplete job should still count as “already deployed”).
108-119: Consider handling other release statuses or transitions.
You handle “Building” and “Failed” states, but if future statuses (e.g., “Pending”) are introduced, the user flow might not be clear. Consider clarifying how new or unexpected statuses should be rendered.
| byDeploymentAndEnvironment: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| deploymentId: z.string().uuid(), | ||
| environmentId: z.string().uuid(), | ||
| }), | ||
| ) | ||
| .meta({ | ||
| authorizationCheck: async ({ canUser, input }) => { | ||
| const { deploymentId, environmentId } = input; | ||
| const deploymentAuthzPromise = canUser | ||
| .perform(Permission.DeploymentGet) | ||
| .on({ type: "deployment", id: deploymentId }); | ||
| const environmentAuthzPromise = canUser | ||
| .perform(Permission.EnvironmentGet) | ||
| .on({ type: "environment", id: environmentId }); | ||
| const [deployment, environment] = await Promise.all([ | ||
| deploymentAuthzPromise, | ||
| environmentAuthzPromise, | ||
| ]); | ||
| return deployment && environment; | ||
| }, | ||
| }) | ||
| .query(async ({ ctx, input }) => { | ||
| const { deploymentId, environmentId } = input; | ||
|
|
||
| const env = await ctx.db | ||
| .select() | ||
| .from(environment) | ||
| .innerJoin(system, eq(environment.systemId, system.id)) | ||
| .innerJoin( | ||
| environmentPolicy, | ||
| eq(environment.policyId, environmentPolicy.id), | ||
| ) | ||
| .leftJoin( | ||
| environmentPolicyReleaseChannel, | ||
| eq(environmentPolicyReleaseChannel.policyId, environmentPolicy.id), | ||
| ) | ||
| .leftJoin( | ||
| releaseChannel, | ||
| and( | ||
| eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), | ||
| eq(environmentPolicyReleaseChannel.deploymentId, deploymentId), | ||
| ), | ||
| ) | ||
| .where(eq(environment.id, environmentId)) | ||
| .then(takeFirst); | ||
|
|
||
| const rel = await ctx.db | ||
| .select() | ||
| .from(release) | ||
| .leftJoin( | ||
| environmentPolicyApproval, | ||
| and( | ||
| eq(environmentPolicyApproval.releaseId, release.id), | ||
| eq(environmentPolicyApproval.policyId, env.environment.policyId), | ||
| ), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(release.deploymentId, deploymentId), | ||
| env.release_channel != null | ||
| ? releaseMatchesCondition( | ||
| ctx.db, | ||
| env.release_channel.releaseFilter, | ||
| ) | ||
| : undefined, | ||
| ), | ||
| ) | ||
| .orderBy(desc(release.createdAt)) | ||
| .limit(1) | ||
| .then(takeFirstOrNull); | ||
|
|
||
| if (rel == null) return null; | ||
|
|
||
| if (env.environment.resourceFilter == null) | ||
| return { | ||
| ...rel.release, | ||
| approval: rel.environment_policy_approval, | ||
| resourceCount: 0, | ||
| }; | ||
|
|
||
| const resourceCount = await ctx.db | ||
| .select({ count: count() }) | ||
| .from(resource) | ||
| .where( | ||
| and( | ||
| eq(resource.workspaceId, env.system.workspaceId), | ||
| resourceMatchesMetadata(ctx.db, env.environment.resourceFilter), | ||
| isNull(resource.deletedAt), | ||
| ), | ||
| ) | ||
| .then(takeFirst); | ||
|
|
||
| return { | ||
| ...rel.release, | ||
| approval: rel.environment_policy_approval, | ||
| resourceCount: resourceCount.count, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
Guard against null environment lookup to avoid runtime error.
If the environment is not found or the policy is missing, env will be undefined. Subsequent references like env.environment.resourceFilter will result in a runtime error. Consider an early return when env is null/undefined.
Here’s a suggested fix:
.where(eq(environment.id, environmentId))
.then(takeFirst);
+ if (!env) {
+ return null;
+ }
const rel = await ctx.db
.select()
...📝 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.
| byDeploymentAndEnvironment: protectedProcedure | |
| .input( | |
| z.object({ | |
| deploymentId: z.string().uuid(), | |
| environmentId: z.string().uuid(), | |
| }), | |
| ) | |
| .meta({ | |
| authorizationCheck: async ({ canUser, input }) => { | |
| const { deploymentId, environmentId } = input; | |
| const deploymentAuthzPromise = canUser | |
| .perform(Permission.DeploymentGet) | |
| .on({ type: "deployment", id: deploymentId }); | |
| const environmentAuthzPromise = canUser | |
| .perform(Permission.EnvironmentGet) | |
| .on({ type: "environment", id: environmentId }); | |
| const [deployment, environment] = await Promise.all([ | |
| deploymentAuthzPromise, | |
| environmentAuthzPromise, | |
| ]); | |
| return deployment && environment; | |
| }, | |
| }) | |
| .query(async ({ ctx, input }) => { | |
| const { deploymentId, environmentId } = input; | |
| const env = await ctx.db | |
| .select() | |
| .from(environment) | |
| .innerJoin(system, eq(environment.systemId, system.id)) | |
| .innerJoin( | |
| environmentPolicy, | |
| eq(environment.policyId, environmentPolicy.id), | |
| ) | |
| .leftJoin( | |
| environmentPolicyReleaseChannel, | |
| eq(environmentPolicyReleaseChannel.policyId, environmentPolicy.id), | |
| ) | |
| .leftJoin( | |
| releaseChannel, | |
| and( | |
| eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), | |
| eq(environmentPolicyReleaseChannel.deploymentId, deploymentId), | |
| ), | |
| ) | |
| .where(eq(environment.id, environmentId)) | |
| .then(takeFirst); | |
| const rel = await ctx.db | |
| .select() | |
| .from(release) | |
| .leftJoin( | |
| environmentPolicyApproval, | |
| and( | |
| eq(environmentPolicyApproval.releaseId, release.id), | |
| eq(environmentPolicyApproval.policyId, env.environment.policyId), | |
| ), | |
| ) | |
| .where( | |
| and( | |
| eq(release.deploymentId, deploymentId), | |
| env.release_channel != null | |
| ? releaseMatchesCondition( | |
| ctx.db, | |
| env.release_channel.releaseFilter, | |
| ) | |
| : undefined, | |
| ), | |
| ) | |
| .orderBy(desc(release.createdAt)) | |
| .limit(1) | |
| .then(takeFirstOrNull); | |
| if (rel == null) return null; | |
| if (env.environment.resourceFilter == null) | |
| return { | |
| ...rel.release, | |
| approval: rel.environment_policy_approval, | |
| resourceCount: 0, | |
| }; | |
| const resourceCount = await ctx.db | |
| .select({ count: count() }) | |
| .from(resource) | |
| .where( | |
| and( | |
| eq(resource.workspaceId, env.system.workspaceId), | |
| resourceMatchesMetadata(ctx.db, env.environment.resourceFilter), | |
| isNull(resource.deletedAt), | |
| ), | |
| ) | |
| .then(takeFirst); | |
| return { | |
| ...rel.release, | |
| approval: rel.environment_policy_approval, | |
| resourceCount: resourceCount.count, | |
| }; | |
| }), | |
| byDeploymentAndEnvironment: protectedProcedure | |
| .input( | |
| z.object({ | |
| deploymentId: z.string().uuid(), | |
| environmentId: z.string().uuid(), | |
| }), | |
| ) | |
| .meta({ | |
| authorizationCheck: async ({ canUser, input }) => { | |
| const { deploymentId, environmentId } = input; | |
| const deploymentAuthzPromise = canUser | |
| .perform(Permission.DeploymentGet) | |
| .on({ type: "deployment", id: deploymentId }); | |
| const environmentAuthzPromise = canUser | |
| .perform(Permission.EnvironmentGet) | |
| .on({ type: "environment", id: environmentId }); | |
| const [deployment, environment] = await Promise.all([ | |
| deploymentAuthzPromise, | |
| environmentAuthzPromise, | |
| ]); | |
| return deployment && environment; | |
| }, | |
| }) | |
| .query(async ({ ctx, input }) => { | |
| const { deploymentId, environmentId } = input; | |
| const env = await ctx.db | |
| .select() | |
| .from(environment) | |
| .innerJoin(system, eq(environment.systemId, system.id)) | |
| .innerJoin( | |
| environmentPolicy, | |
| eq(environment.policyId, environmentPolicy.id), | |
| ) | |
| .leftJoin( | |
| environmentPolicyReleaseChannel, | |
| eq(environmentPolicyReleaseChannel.policyId, environmentPolicy.id), | |
| ) | |
| .leftJoin( | |
| releaseChannel, | |
| and( | |
| eq(environmentPolicyReleaseChannel.channelId, releaseChannel.id), | |
| eq(environmentPolicyReleaseChannel.deploymentId, deploymentId), | |
| ), | |
| ) | |
| .where(eq(environment.id, environmentId)) | |
| .then(takeFirst); | |
| if (!env) { | |
| return null; | |
| } | |
| const rel = await ctx.db | |
| .select() | |
| .from(release) | |
| .leftJoin( | |
| environmentPolicyApproval, | |
| and( | |
| eq(environmentPolicyApproval.releaseId, release.id), | |
| eq(environmentPolicyApproval.policyId, env.environment.policyId), | |
| ), | |
| ) | |
| .where( | |
| and( | |
| eq(release.deploymentId, deploymentId), | |
| env.release_channel != null | |
| ? releaseMatchesCondition( | |
| ctx.db, | |
| env.release_channel.releaseFilter, | |
| ) | |
| : undefined, | |
| ), | |
| ) | |
| .orderBy(desc(release.createdAt)) | |
| .limit(1) | |
| .then(takeFirstOrNull); | |
| if (rel == null) return null; | |
| if (env.environment.resourceFilter == null) | |
| return { | |
| ...rel.release, | |
| approval: rel.environment_policy_approval, | |
| resourceCount: 0, | |
| }; | |
| const resourceCount = await ctx.db | |
| .select({ count: count() }) | |
| .from(resource) | |
| .where( | |
| and( | |
| eq(resource.workspaceId, env.system.workspaceId), | |
| resourceMatchesMetadata(ctx.db, env.environment.resourceFilter), | |
| isNull(resource.deletedAt), | |
| ), | |
| ) | |
| .then(takeFirst); | |
| return { | |
| ...rel.release, | |
| approval: rel.environment_policy_approval, | |
| resourceCount: resourceCount.count, | |
| }; | |
| }), |
Summary by CodeRabbit
New Features
Refactor / Chores