fix: Optimize release list query and use lazy loading for release cells #269
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several enhancements to the deployment and release management functionality across multiple components of the web application. The changes focus on improving the handling of job statuses, environment deployment tracking, and streamlining the rendering of release information. A new dependency, Changes
Sequence DiagramsequenceDiagram
participant User
participant DeploymentTable
participant LazyReleaseEnvironmentCell
participant API
User->>DeploymentTable: View Deployments
DeploymentTable->>LazyReleaseEnvironmentCell: Render Lazily
LazyReleaseEnvironmentCell->>API: Fetch Release Status
API-->>LazyReleaseEnvironmentCell: Return Job Statuses
LazyReleaseEnvironmentCell->>User: Display Environment Status
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (3)
49-96: Robust job status checks
The conditional logic for job statuses is properly separated, making it straightforward to identify each status. However, keep an eye on potential overlap or future expansions where multiple statuses might apply simultaneously (e.g., "InProgress" plus "ActionRequired").
154-160: Query reference
Using the "isPresent" helper for enabling the query only when "workspaceId.data?.id" is present is good. This ensures that you’re not triggering the query prematurely.
214-220: Loading state
Showing a spinner while "releaseJobTriggersQ.isLoading" is true is standard. Consider adding some user-friendly text or skeleton placeholders for more clarity.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (2)
43-45: Loading state handled gracefully.The simple fallback "Loading..." text is appropriate. Consider providing additional context or skeleton UI if needed for a more polished user experience.
52-58: showRelease & showDeployButton conditions are clear.These combined checks ensure the correct UI element is displayed. Be mindful of future expansions (e.g., more environment states) to keep these conditions maintainable.
packages/api/src/router/release.ts (2)
324-354: Consider adding an index for performance optimization.The implementation looks good, but for optimal query performance, consider adding an index on (releaseId, environmentId) in the releaseJobTrigger table since these columns are frequently used in the WHERE clause.
339-353: Add error handling for no statuses found.Consider handling the case when no statuses are found by either:
- Returning a default/empty state
- Throwing a meaningful error
.query(async ({ input }) => { const statuses = await db .selectDistinctOn([job.status]) .from(job) .innerJoin(releaseJobTrigger, eq(job.id, releaseJobTrigger.jobId)) .orderBy(job.status, desc(job.createdAt)) .where( and( eq(releaseJobTrigger.releaseId, input.releaseId), eq(releaseJobTrigger.environmentId, input.environmentId), ), ); + if (statuses.length === 0) { + // Option 1: Return default state + return []; + // Option 2: Throw error + // throw new TRPCError({ + // code: 'NOT_FOUND', + // message: 'No job statuses found for the given release and environment', + // }); + } return statuses; }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/webservice/package.json(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(3 hunks)packages/api/src/router/release.ts(3 hunks)packages/validators/src/jobs/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
packages/validators/src/jobs/index.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/release.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (40)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (10)
1-1: Consider grouping imports
Although there's nothing incorrect here, remember that as this file grows in complexity, you may logically group or alphabetically sort imports for clarity. However, there's no strict requirement unless indicated by your lint or style guide.
30-39: Imports look appropriate
These imports from "@ctrlplane/validators/conditions" and "@ctrlplane/validators/jobs" are consistent with the usage below. Everything seems well-referenced and necessary.
47-48: Props signature updated
The new prop "statuses" aligns with the refactoring that handles direct job statuses instead of more complex objects. This simplification makes the component easier to read and maintain.
120-131: Prop rename ensures consistency
Renaming/introducing "statuses" in the Release component is consistent with the "ReleaseIcon" approach. Good job maintaining naming consistency.
134-139: Clean filter definition
Using "isSameRelease" and "isSameEnvironment" is more readable than combining into a single condition object. Nicely done.
140-150: Filter composition
The filter that aggregates multiple conditions with "ComparisonOperator.And" is correct. This approach is flexible if you want to add more conditions (like a "resource ID") in other places.
152-153: Query usage
The workspace ID check is correct. The destructuring from "workspaceSlug" → "workspaceId" is well-handled to avoid undefined references.
188-188: Resourceful link
The dynamic href path for the release is formatted coherently. This is consistent with Next.js patterns.
191-191: Reusing ReleaseIcon
Good approach to unify the usage of “ReleaseIcon” by passing the new "statuses" prop. This helps maintain visual consistency.
250-250: Leverage activeStatusType
Using "statuses.some" in conjunction with "activeStatusType" is clean and ensures that the code’s intention is explicit.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx (12)
3-3: Importing JobCondition
Importing JobCondition from "@ctrlplane/validators/jobs" is a good addition for strongly typing your filter objects.
17-23: Multiple new imports
Bringing in "ColumnOperator", "ComparisonOperator", "FilterType", and "ReservedMetadataKey" from "@ctrlplane/validators/conditions" clarifies usage for your filter-based logic.
25-25: TRPC usage
Importing "api" from "~/trpc/react" is consistent with the rest of the codebase. Looks good.
267-268: Workspace fetching
The pattern of retrieving "workspace" using "workspaceSlug" is standard. This ensures you can pass the correct ID to subsequent queries.
272-276: Defining isSameRelease
Separating out "isSameRelease" with the release ID is readable. This is a standard approach.
278-282: Defining isSameResource
Same pattern as isSameRelease, continuing the consistent approach.
284-288: Defining isSameEnvironment
With environment-based filtering, your approach is symmetrical with the other conditions.
290-294: Combining conditions
Creating a single filter using "ComparisonOperator.And" elegantly captures all three constraints.
296-301: Query enabling
Conditionally enabling the query based on a valid "workspace" is good. Also ensures no extraneous network calls.
302-303: Confidence check
"hasOtherReleaseJobTriggers" helps you decide whether to expand the collapsible or not. Straightforward check.
313-316: Parent row props
Supplying the job trigger to the parent row is flexible. The key usage is correct to avoid React warnings.
Line range hint 320-330: Child row mapping
Properly ignoring the first item in the array to be displayed by the parent row. Straightforward approach to a nested row design.
packages/validators/src/jobs/index.ts (1)
36-36: Newly introduced activeStatusType
Exporting "activeStatusType" as a typed version of "activeStatus" clarifies references in other files. This is beneficial for strongly typed logic checks like “statuses.some((s) => activeStatusType.includes(s))”. Nicely integrated.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (9)
1-2: "use client" directive successfully applied.
No issues found with the initial client-side configuration. This ensures that the component will render client-side as intended.
3-6: Imports appear coherent and well-separated.
Imports from next/navigation, react-intersection-observer, and local modules are organized logically. Continue maintaining proper scoping and usage of imported functionality.
14-16: Type alias usage is clear.
Using RouterOutputs to define the Environment and BlockedEnv types keeps this component strongly typed. Good practice.
17-22: Props definition is succinct and readable.
All necessary fields for environment, deployment, release, and blockedEnv are clearly defined. This ensures easy integration with the rest of the codebase.
30-39: Regular polling approach (refetchInterval = 2000ms).
With a 2-second refetch interval, the data stays fresh. However, confirm whether 2 seconds is optimal or might risk unnecessary load on the server if the environment sees high traffic.
46-48: Consider verifying environment resource list length.
The code checks environment.resources.length > 0. Confirm that no special handling is required if resources is an undefined or null array. Nonetheless, this may already be guaranteed by the back-end.
49-51: Blocked environment logic is straightforward.
This boolean check supports a clear UI path to show or disable the deployment button accordingly.
75-102: Clear fallback UI for not deployed scenarios.
Code appropriately displays a text message or a link to the release channel if blocked. This clarifies next steps to the user.
107-118: Lazy loading pattern effectively optimizes performance.
The LazyReleaseEnvironmentCell uses react-intersection-observer to only render when in view. This helps avoid unnecessary overhead for offscreen components. Great implementation.
apps/webservice/package.json (1)
75-75: Dependency addition for lazy loading.
The dependency "react-intersection-observer": "^9.14.0" aligns with your lazy-loading strategy in ReleaseEnvironmentCell. Ensure the version meets long-term compatibility needs.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx (2)
12-12: Importing LazyReleaseEnvironmentCell.
Replacing the previous release rendering component with LazyReleaseEnvironmentCell indicates a good move towards more modular and performant UI elements.
138-149: Conditional rendering for release presence.
If release exists, LazyReleaseEnvironmentCell is displayed; otherwise, a fallback "No release" message is shown. This ensures minimal UI confusion and clearly indicates the resource state.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (4)
33-33: Importing LazyReleaseEnvironmentCell.
A consistent approach across files. This aids code reusability and maintains a unified rendering logic.
42-42: Refined Deployment type alias.
Using NonNullable<RouterOutputs["deployment"]["bySlug"]> clarifies that deployment must be a valid object, preventing null checks in multiple locations.
45-45: More specific type for deployment prop.
Switching from schema plus release channels to a typed Deployment ensures better type safety and simpler code.
244-263: TableCell click handling and lazy rendering.
Stopping event propagation inside each and leveraging is a clean approach, ensuring row-level clicks don’t conflict with environment-level interactions. Meets performance and clarity goals.
packages/api/src/router/release.ts (1)
69-71: LGTM! Query optimization looks good.
The changes improve query construction by:
- Using a more maintainable pattern for combining conditions
- Removing redundant
and()calls - Maintaining type safety through
isPresentfiltering
Also applies to: 73-81, 102-102
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores