refactor: system table cell uses new jobs#533
Conversation
WalkthroughThis change refactors the deployment environment cell rendering in the web application's deployment tables. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DeploymentEnvironmentCell
participant API as system.table.cell (tRPC)
participant DB as Database
UI->>API: Request cell data (environmentId, deploymentId)
API->>DB: Query for deployment version and job statuses
DB-->>API: Return deployment version and statuses
API-->>UI: Respond with deployment version info and statuses
UI->>UI: Render cell UI based on data
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (4)
35-38: Remove debugconsole.logbefore merging
console.log(environmentId, deployment, systemSlug);will leak potentially sensitive identifiers to every browser console in production. It can also have a measurable performance cost when the table renders hundreds of cells.- console.log(environmentId, deployment, systemSlug);
44-48: Tiny optimisation: build URLs once
urls.workspace(workspaceSlug)…is re-executed on every render, even when onlyisLoadingtoggles. Wrapping it inuseMemokeeps renders cheap on large tables.- const deploymentUrls = urls - .workspace(workspaceSlug) - .system(systemSlug) - .deployment(deployment.slug); + const deploymentUrls = React.useMemo( + () => + urls + .workspace(workspaceSlug) + .system(systemSlug) + .deployment(deployment.slug), + [workspaceSlug, systemSlug, deployment.slug], + );
51-59: Clarify “No jobs” copy“No jobs” could be mis-read as “no background jobs” instead of “no deployment runs”.
A slightly richer message reduces ambiguity and makes the link purpose clearer.- No jobs + No deployment runs
89-95: Accessibility: ensure focus outline is visibleThe wrapper
divsets fixed width/height but the child link relies on browser default focus styles, which can be clipped. Addingfocus-visible:outlineclasses (Tailwind) keeps keyboard navigation usable.- <Link + <Link + className="focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(2 hunks)packages/api/src/router/system-table.ts(1 hunks)packages/api/src/router/system.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/system.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsxpackages/api/src/router/system-table.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx
🧬 Code Graph Analysis (2)
packages/api/src/router/system.ts (1)
packages/api/src/router/system-table.ts (1)
systemTableRouter(9-68)
packages/api/src/router/system-table.ts (3)
packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(173-173)packages/db/src/schema/job.ts (1)
JobStatus(141-141)packages/db/src/common.ts (1)
takeFirstOrNull(15-20)
🔇 Additional comments (7)
packages/api/src/router/system.ts (2)
39-39: Logical integration of the system table router.The import for
systemTableRouteris correctly added, preparing for its usage in the main system router.
319-319: Well-placed integration of the table router.The
systemTableRouteris properly integrated into the main system router under thetableproperty, providing a clear namespace for table-related operations.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx (1)
231-231: Simplified props passing.The component now receives only the environment ID rather than the entire environment object, which is a more efficient approach for data passing. This change aligns with the broader refactoring to simplify the deployment environment cell components.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsx (1)
211-215: Streamlined component implementation.The
LazyDeploymentEnvironmentCellimplementation has been properly refactored to:
- Remove unnecessary wrapper div
- Pass only the environment ID instead of the full environment object
- Remove the workspace prop entirely
This change aligns with the broader refactoring effort to simplify the component API and improve maintainability.
packages/api/src/router/system-table.ts (3)
1-8: Well-structured imports for the new router.Imports are organized logically, with standard libraries first followed by internal imports. The necessary utilities and schema definitions are properly imported.
9-22: Well-defined router with proper permission checks.The router is implemented correctly with:
- Clear input validation using zod
- Proper permission checks to ensure the user has access to the requested deployment
- Well-defined types for parameters
This provides a secure foundation for the API endpoint.
23-67: Efficient database query implementation.The query implementation:
- Properly joins the necessary tables to fetch deployment-related data
- Uses SQL aggregation to collect job statuses in a single query
- Applies appropriate filtering based on input parameters
- Adds sorting to return the most recent version first
- Limits results to one record for efficiency
- Handles the null case with takeFirstOrNull
This consolidated query is a significant improvement over potentially multiple separate queries.
| const { data, isLoading } = api.system.table.cell.useQuery({ | ||
| environmentId, | ||
| deploymentId: deployment.id, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface query errors to avoid silent failures
useQuery exposes error / isError; if the request fails the UI stays in a perpetual loading skeleton or falls through to the “No jobs” branch, which can mask backend/permission issues.
Consider adding an explicit error state so users have feedback and you have telemetry.
- const { data, isLoading } = api.system.table.cell.useQuery({
+ const { data, isLoading, isError, error } = api.system.table.cell.useQuery({
environmentId,
deploymentId: deployment.id,
});
+
+ if (isError) {
+ return (
+ <div className="flex h-full w-full items-center justify-center text-destructive">
+ {error.message ?? "Failed to load"}
+ </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 { data, isLoading } = api.system.table.cell.useQuery({ | |
| environmentId, | |
| deploymentId: deployment.id, | |
| }); | |
| const { data, isLoading, isError, error } = api.system.table.cell.useQuery({ | |
| environmentId, | |
| deploymentId: deployment.id, | |
| }); | |
| if (isError) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-destructive"> | |
| {error.message ?? "Failed to load"} | |
| </div> | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (1)
37-41:⚠️ Potential issueSurface query errors to avoid silent failures
The query implementation still doesn't handle error states, which could lead to silent failures where users see either a perpetual loading skeleton or "No jobs" message when there are actually permission or backend issues.
- const { data, isLoading } = api.system.table.cell.useQuery({ + const { data, isLoading, isError, error } = api.system.table.cell.useQuery({ environmentId, deploymentId: deployment.id, }); + + if (isError) { + return ( + <div className="flex h-full w-full items-center justify-center text-destructive"> + {error.message ?? "Failed to load"} + </div> + ); + }
🧹 Nitpick comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (3)
74-76: Consider localization for date formattingThe date formatting is currently hardcoded with a specific format. For better internationalization support, consider using a localized date format approach.
- {format(data.versionCreatedAt, "MMM d, hh:mm aa")} + {format(data.versionCreatedAt, + new Intl.DateTimeFormat(navigator.language, { + month: 'short', + day: 'numeric', + hour: 'numeric', + minute: 'numeric', + hour12: true + }).format(data.versionCreatedAt) + )}Alternatively, if you're using a localization library throughout the application, consider integrating with that for consistency.
89-91: Use CSS variables for consistent dimensionsThe hardcoded dimensions (
h-[70px]andw-[220px]) could be extracted to CSS variables or theme constants for better maintainability and consistency across the application.< <div < className="flex h-[70px] w-[220px] items-center justify-center" < ref={ref} < >This would make it easier to adjust these values globally if needed.
30-81: Add JSDoc documentationConsider adding JSDoc documentation to the component to explain its purpose, usage, and the structure of the data it expects. This would help other developers understand how to use this component correctly.
+ /** + * DeploymentEnvironmentCell displays deployment status and version information for a specific environment. + * + * @param {string} environmentId - The ID of the environment to display + * @param {Object} deployment - A minimal deployment object containing id and slug + * @param {string} systemSlug - The slug of the system this deployment belongs to + */ const DeploymentEnvironmentCell: React.FC<DeploymentEnvironmentCellProps> = ({ environmentId, deployment, systemSlug, }) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (5)
14-22: Well-structured skeleton componentThe new
CellSkeletoncomponent is well-structured and provides appropriate visual feedback during loading. The component's extraction improves code reusability and maintains consistency.
24-34: Props simplification is a good improvementThe simplification of props to only require
environmentIdand a minimal deployment object is a good architecture decision that reduces unnecessary prop drilling and makes the component more focused and reusable.
35-36: Good use of route params instead of propsUsing
useParamsto extract theworkspaceSlugdirectly from the route is a better approach than passing it through props, reducing prop drilling and making the component less dependent on its parent.
61-80: Improved UI with clear visual hierarchyThe redesigned UI has a clearer visual hierarchy with the status icon, version tag, and formatted date. The component is now more focused on its primary responsibility of displaying deployment environment information.
47-59: Clean empty state handlingThe empty state handling with a direct link to the releases page is clean and provides users with a clear path to find more information, which is a good UX improvement.
Summary by CodeRabbit
Refactor
New Features