fix: Reuse filter endpoints for deployment resource drawer#239
Conversation
WalkthroughThis pull request introduces significant changes across several components and API routes related to deployment resources and job filtering. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/ReleaseTable.tsx (1)
15-15: Add JSDoc documentation for the ReleaseWithTriggers type.Consider adding JSDoc documentation to explain the type's purpose and structure for better maintainability.
+/** + * Represents a release with its associated triggers from the API response. + * @typedef {RouterOutputs["release"]["list"]["items"][number]} ReleaseWithTriggers + */ type ReleaseWithTriggers = RouterOutputs["release"]["list"]["items"][number];packages/api/src/router/release.ts (1)
124-125: Fix formatting: Remove unnecessary newline before orderBy.The
orderByclause should be on the same line as the previous chain for consistent formatting.), - ) - .orderBy(desc(releaseJobTrigger.createdAt)); + ).orderBy(desc(releaseJobTrigger.createdAt));apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx (1)
Line range hint
89-157: Enhance accessibility for the links hover cardWhile the hover card implementation for additional links is visually appealing, consider enhancing keyboard accessibility:
- Add keyboard navigation support for the hover card
- Include ARIA labels for better screen reader support
- Consider adding a tooltip to indicate that more links are available
<HoverCard> <HoverCardTrigger asChild> - <Button variant="secondary" size="sm" className="h-6"> + <Button + variant="secondary" + size="sm" + className="h-6" + aria-label={`Show ${remainingLinks.length} more links`} + role="button" + tabIndex={0} + > +{remainingLinks.length} more </Button> </HoverCardTrigger> <HoverCardContent className="flex max-w-40 flex-col gap-1 p-2" align="start" + role="menu" > {remainingLinks.map(([label, url]) => ( <Link key={label} href={url} target="_blank" rel="noopener noreferrer" - className="truncate text-sm underline-offset-1 hover:underline" + className="truncate text-sm underline-offset-1 hover:underline focus:outline-none focus:ring-2" + role="menuitem" + tabIndex={0} > {label} </Link> ))} </HoverCardContent> </HoverCard>apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (2)
274-277: Consider adding error handling for failed redeployments.While the success path is well-handled, there's no error handling for failed redeployments. Consider adding:
- Error state UI feedback
- Error message display
- Error recovery options
Example implementation:
redeploy .mutateAsync({ environmentId, resourceId: target.id, releaseId: release.id, }) .then(() => utils.release.list.invalidate()) .then(() => router.refresh()) .then(() => setIsOpen(false)) + .catch((error) => { + // Show error message to user + console.error('Redeployment failed:', error); + // Optionally add a toast notification or error state + });
Line range hint
1-394: Well-structured component architecture with consistent patterns.The file demonstrates good architectural decisions:
- Consistent mutation handling patterns across all dialogs
- Clear separation of concerns between UI components
- Proper state management with controlled dialogs
- Thoughtful UX with appropriate confirmations for destructive actions
Consider documenting these patterns in your team's style guide to maintain consistency across the codebase.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (2)
57-77: Avoid defaulting IDs to empty strings in 'jobFilter'Since
resourceId,environmentId, anddeploymentIdare guaranteed to be non-null whenisOpenistrue, defaulting them to empty strings using?? ""is unnecessary. Using empty strings may lead to unintended API behavior if empty strings are considered valid IDs. Consider using the IDs directly without default values to ensure accurate filtering.Apply this diff to remove unnecessary default values:
- value: resourceId ?? "", + value: resourceId,Repeat this change for
environmentIdanddeploymentId:- value: environmentId ?? "", + value: environmentId,- value: deploymentId ?? "", + value: deploymentId,
79-86: Avoid defaulting 'deploymentId' to empty string in API querySince
deploymentIdis guaranteed to be non-null whenisOpenistrue, defaulting it to an empty string using?? ""is unnecessary. Passing an empty string could lead to unintended results or API errors. UsedeploymentIddirectly without defaulting to an empty string.Apply this diff to update the API query:
- deploymentId: deploymentId ?? "", + deploymentId,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/ReleaseTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx(3 hunks)packages/api/src/router/job.ts(0 hunks)packages/api/src/router/release.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/api/src/router/job.ts
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/release.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
🔇 Additional comments (12)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/ReleaseTable.tsx (1)
15-15: Verify the type change impact across the codebase.
The type change from job-based to release-based structure looks good and aligns with the PR objective. However, let's ensure all consumers are updated accordingly.
✅ Verification successful
Type change is isolated and properly contained
The type ReleaseWithTriggers is only used within the ReleaseTable.tsx component and its props, with no other components depending on this type. Additionally, there are no traces of the old job-based type structure in the codebase, suggesting the migration is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all components that might be affected by the ReleaseWithTriggers type change
# Search for components using ReleaseWithTriggers type
echo "Components using ReleaseWithTriggers type:"
rg -t typescript -t tsx "ReleaseWithTriggers" ./apps/webservice/src
# Search for potential job config usage that might need updating
echo -e "\nPotential outdated job config usage:"
rg -t typescript -t tsx "RouterOutputs\[\"job\"\]\[\"config\"\]\[\"byDeploymentEnvAndResource\"\]" ./apps/webservice/src
Length of output: 438
Script:
#!/bin/bash
# Let's try without file type filters since they're not recognized
# Search for components using ReleaseWithTriggers type
echo "Components using ReleaseWithTriggers type:"
rg "ReleaseWithTriggers" ./apps/webservice/src
# Search for potential job config usage that might need updating
echo -e "\nPotential outdated job config usage:"
rg "RouterOutputs\[\"job\"\]\[\"config\"\]\[\"byDeploymentEnvAndResource\"\]" ./apps/webservice/src
# Also search for simpler patterns that might indicate usage
echo -e "\nSimpler job config pattern search:"
rg "\"job\"\]\[\"config\"\]\[\"byDeploymentEnvAndResource\"" ./apps/webservice/src
Length of output: 823
packages/api/src/router/release.ts (4)
24-24: LGTM: Import additions are appropriate.
The new imports for job filtering functionality are correctly placed and follow the existing import pattern.
Also applies to: 42-42
61-61: LGTM: Input schema extension is well-structured.
The addition of the optional jobFilter parameter maintains type safety and aligns with the PR objective to reuse filter endpoints.
116-116: LGTM: Job filtering logic is correctly implemented.
The jobMatchesCondition is properly integrated into the resource join conditions, maintaining consistent behavior with other filters.
61-61: Verify consistent filter implementation across endpoints.
Let's verify if there are other endpoints that might benefit from the same job filtering capability for consistency.
Also applies to: 116-116
✅ Verification successful
Filter implementation is consistent across endpoints
The verification shows that the job filtering implementation is consistent with the existing patterns in the codebase:
- The
jobFilteris properly implemented inrelease.tsusingjobCondition.optional() - Similar filter patterns are already present in other routers:
job.tsusesfilter: jobCondition.optional()resources.tsusesfilter: resourceCondition.optional()- All filters follow the same optional condition pattern with consistent pagination parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find similar router endpoints that might need job filtering
# Look for similar list procedures in router files
echo "Looking for similar list procedures in router files..."
rg -A 5 "list:\s+.*procedure" "packages/api/src/router"
# Look for jobFilter usage in other files
echo "Checking jobFilter usage across the codebase..."
rg "jobFilter" --type typescript
Length of output: 347
Script:
#!/bin/bash
# Let's try a different approach to find similar endpoints and job filter usage
# Look for similar list procedures in router files without file type restriction
echo "Looking for similar list procedures in router files..."
rg -A 5 "list:\s+.*procedure" packages/api/src/router/
# Search for jobFilter usage without file type restriction
echo "Checking jobFilter usage..."
rg "jobFilter" packages/api/src/
# Look for similar filter patterns in router files
echo "Looking for similar filter patterns..."
rg -A 3 "filter.*optional\(\)" packages/api/src/
Length of output: 1896
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/TableRow.tsx (2)
Line range hint 358-391: Well-structured implementation of collapsible release rows
The ReleaseRows component effectively manages the display of multiple job triggers with a clean parent-child pattern. The implementation:
- Handles all states gracefully (no triggers, single trigger, multiple triggers)
- Uses proper React patterns for state management
- Maintains clear separation of concerns
352-352: Verify type compatibility with the new Release structure
The change from job config to release list output type is a significant architectural shift. While the implementation looks correct, we should verify that all properties accessed from the Release type are available in the new structure.
✅ Verification successful
Type compatibility verified - no issues found
The verification shows that the properties accessed from the Release type (id, name) are basic properties that are commonly available in API responses. All usages are consistent and focus on fundamental properties that would be present in any release entity. The migration from job config to release list output appears to be safe as the accessed properties are standard identifiers and naming fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all properties accessed from Release type exist in the new structure
# Look for any potential type mismatches or missing properties
# Search for all properties accessed from release objects
echo "Properties accessed from release:"
rg -A 1 'release\.' apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/
# Search for the type definition to compare
echo "Release type definition:"
rg 'type.*Release.*=.*RouterOutputs' apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/
Length of output: 365
Script:
#!/bin/bash
# Let's try a different approach by escaping the special characters in the path
# and using more specific patterns to find release property access
# Search for all properties accessed from release objects
echo "Properties accessed from release:"
rg -A 1 'release\.' "apps/webservice/src/app/\[workspaceSlug\]/\(app\)/_components/deployment-resource-drawer/TableRow.tsx"
# Search for the type definition
echo "Release type definition:"
rg 'type.*Release.*=.*RouterOutputs' "apps/webservice/src/app/\[workspaceSlug\]/\(app\)/_components/deployment-resource-drawer/TableRow.tsx"
# Let's also try to find any usage of Release type
echo "Usage of Release type:"
rg ':\s*Release[\[\]\s{},]' "apps/webservice/src/app/\[workspaceSlug\]/\(app\)/_components/deployment-resource-drawer/TableRow.tsx"
Length of output: 1019
Script:
#!/bin/bash
# Let's try using fd to find the correct path first, then use it for our searches
# Find the correct path for TableRow.tsx
echo "Finding TableRow.tsx:"
fd "TableRow.tsx" --type f
# Once we find the file, we'll use its path to search for release properties
# and type definitions using a variable to store the path
FILE_PATH=$(fd "TableRow.tsx" --type f)
if [ ! -z "$FILE_PATH" ]; then
echo -e "\nProperties accessed from release:"
rg -A 1 'release\.' "$FILE_PATH"
echo -e "\nRelease type definition:"
rg 'type.*Release.*=.*RouterOutputs' "$FILE_PATH"
echo -e "\nUsage of Release type:"
rg ':\s*Release[\[\]\s{},]' "$FILE_PATH"
fi
Length of output: 1927
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (2)
94-94: LGTM: Appropriate cache invalidation sequence.
The addition of release list invalidation maintains data consistency after job status changes. The sequence of invalidations is logical, ensuring both job-specific and release-level caches are properly updated.
223-226: LGTM: Well-structured mutation handling with good UX.
The promise chain appropriately updates the UI state after force release:
- Invalidates the release list to reflect changes
- Refreshes the router for navigation updates
- Closes dialogs for a clean user experience
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (3)
3-3: Approved: Imported 'JobCondition' from '@ctrlplane/validators/jobs'
The import of JobCondition is necessary for defining the jobFilter used in the component.
12-17: Approved: Imported necessary filter types and operators
The imports of ColumnOperator, ComparisonOperator, FilterType, and JobFilterType are essential for constructing the filtering logic.
49-56: Handle potential null values when constructing 'releaseFilter'
The current logic for releaseFilter might return undefined if no matching release channel is found. Ensure that the API can handle an undefined filter gracefully. Consider adding a default or fallback filter if necessary to prevent unexpected behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores