fix: Deployment level resource filtering #273
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces comprehensive changes to resource filtering and deployment management across multiple components. The modifications primarily focus on enhancing resource condition handling, adding a new Changes
Possibly related PRs
Suggested reviewers
Poem
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 (15)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (1)
61-65: Check enabling condition for 'useQuery'.
Currently, the fetch is skipped if 'envResourceFilter' is null, even if a 'deploymentResourceFilter' is present. If the goal is to only query when both exist, that's fine; otherwise, revisit this logic.- { enabled: workspaceId !== "" && envResourceFilter != null } + { enabled: workspaceId !== "" && (envResourceFilter != null || deploymentResourceFilter != null) }packages/job-dispatch/src/deployment-update.ts (1)
102-168: Handling resource filter diffs.
This function robustly determines resources to remove or add based on old vs. new filters. Consider potential concurrency if filter changes occur in quick succession. Otherwise, the approach is valid.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx (3)
79-79: Mutation call for updating deployment.
If updates can be large or slow, consider showing a busy indicator for user feedback.
90-94: Catching mutation errors.
Displaying a more detailed server error can help users troubleshoot.
201-223: New 'resourceFilter' form field.
Combining 'ResourceConditionRender' with 'DeploymentResourcesDialog' offers a robust way to visualize and modify resource filters. Consider brief inline instructions to guide less-technical users.packages/db/src/schema/deployment.ts (1)
71-73: New 'resource_filter' column.
If you need frequent indexing of filters, consider specialized indexing strategies.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx (3)
1-21: Use consistent import ordering and grouping.Consider grouping external libraries (e.g. React, tabler icons) together, followed by internal imports (e.g. from "~/app"). This helps maintain a clean import structure for better readability.
57-60: Add error handling or fallback UI for failed resource fetches.Currently, on query failure, nothing is rendered to indicate an error state. A simple error message or fallback UI would enhance user experience and debugging.
105-116: Consider passing a real callback for onChange in ResourceConditionRender.At line 107, onChange is a no-op function (i.e., () => {}). If you intend for users to update the resource filter interactively, providing a meaningful callback would help maintain local state or handle real-time updates.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx (2)
6-6: Remove unused imports, if any.If the imported icons or utilities from "@tabler/icons-react" are partially unused elsewhere in the code, you may safely remove them to reduce overhead. Otherwise, this is fine.
70-73: Show placeholders or skeleton instead of count when loading.While the spinner is displayed, consider a subtle placeholder (e.g., “Loading...”). This approach can be more descriptive than showing just “0” or omitting the number entirely in other scenarios.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/Filter.tsx (2)
4-4: Consistent icon usage.You introduced “IconLoader2, IconSelector.” Confirm if both are truly needed or if there’s any redundancy in usage.
202-206: Consider showing feedback when resource count is zero.Currently, “ResourceList” is rendered if “resources.data.total > 0”. For a zero result, it might be useful to render a quick “No resources found” message, guiding the user that the filter returns nothing.
packages/db/drizzle/0049_condemned_franklin_storm.sql (1)
1-1: Consider indexing the new JSONB column.If large filters are anticipated, adding a GIN index on “resource_filter” might improve query performance. However, it is optional and depends on the complexity of your filter lookups.
packages/db/drizzle/meta/_journal.json (1)
348-353: Consider documenting the JSON schema for the resource_filter column.Since this migration adds a
jsonbcolumn for resource filtering, it's important to:
- Document the expected JSON schema to ensure consistent data structure
- Consider adding database-level validation constraints if possible
- Implement application-level validation before saving filter conditions
This will help maintain data integrity and make it easier to evolve the filtering capabilities in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/Filter.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resource-condition/ResourceList.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(4 hunks)packages/api/src/router/environment.ts(1 hunks)packages/db/drizzle/0049_condemned_franklin_storm.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(4 hunks)packages/job-dispatch/src/deployment-update.ts(3 hunks)packages/job-dispatch/src/release-job-trigger.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
packages/job-dispatch/src/release-job-trigger.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)/_components/environment-drawer/Filter.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.
packages/api/src/router/environment.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/job-dispatch/src/deployment-update.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/[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/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/[deploymentSlug]/EditDeploymentSection.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/resource-condition/ResourceList.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]/page.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.
packages/db/src/schema/deployment.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/[deploymentSlug]/DeploymentResourcesDialog.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.
🔇 Additional comments (48)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (10)
4-5: No issues with the new type imports.
Everything looks correct and consistent with the overall architecture.
8-8: Library usage for 'ts-is-present'.
No issues found; usage is straightforward for filtering out null/undefined values.
11-14: Importing condition operators.
These additions are clear and support the enhanced filtering logic.
26-26: Prop updated to the more specific 'SCHEMA.Deployment'.
This helps improve type checks and clarifies the component’s interface.
52-54: Destructuring environment & deployment resource filters.
No issues; this approach aligns with new resource filtering requirements.
67-67: Defaulting total to 0.
This is a clean approach for preventing undefined checks.
71-72: Unified loading state.
Combining multiple queries under one 'isLoading' flag is concise and user-friendly.
76-76: 'hasResources' based on total > 0.
Straightforward and readable.
55-59: Combining filters with 'AND'.
Verify that requiring both environment and deployment filters to pass is the intended logic. If you want an alternate combination, consider adjusting 'operator'.
✅ Verification successful
Let me gather more information about the filtering logic in the codebase to verify if the AND combination is the intended behavior.
Let me search for more specific information about the environment and deployment filters to understand their usage context.
Based on the code analysis, I can now generate the final response:
AND operator is the correct choice for combining filters
The AND combination of environment and deployment filters is the intended logic, as evidenced by:
- Consistent usage across multiple components (ReleaseEnvironmentCell and DeploymentPageContent)
- The same pattern is used in the job-dispatch system for release job triggering
- Resources must satisfy both environment and deployment constraints to be valid targets
- The filter combination is used to determine if there are available resources for deployment (
hasResources)
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for similar resource filter patterns to understand the common usage
rg -A 5 "ResourceCondition.*FilterType\.Comparison.*ComparisonOperator"
# Search for the type definitions and interfaces
ast-grep --pattern 'type ResourceCondition = $_'
ast-grep --pattern 'interface ResourceCondition $_'
Length of output: 1014
Script:
#!/bin/bash
# Search for the file content to understand the context
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx"
# Search for other usages of envResourceFilter and deploymentResourceFilter
rg "envResourceFilter|deploymentResourceFilter" -B 2 -A 2
# Look for similar filter combinations
rg "conditions: \[.*\]\.filter\(isPresent\)" -B 2 -A 2
Length of output: 13861
42-45: Ensure fallback logic for missing workspace.
If 'workspaceSlug' can be invalid, note that 'workspaceId' will be an empty string. Consider whether to handle nonexistent workspaces more explicitly.
packages/job-dispatch/src/deployment-update.ts (1)
195-203: Conditionally updating deployment resources.
Using '_.isEqual' prevents unnecessary overhead when filters haven't changed.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx (10)
3-3: No functional changes here.
28-31: Importing resource condition utilities.
Facilitates the new filtering logic.
33-33: ResourceConditionRender import.
Useful for rendering resource filter conditions in a standardized form.
35-35: DeploymentResourcesDialog import.
Ensures a dedicated UI for filtering resources.
39-40: Refined 'System' type.
Pulling from TRPC output types fosters consistent usage.
43-44: New 'workspaceId' prop.
This addition expands the context needed for resource queries at the deployment level.
50-50: Destructuring 'workspaceId'.
Cleanly retrieves ID for further resource handling.
61-64: Default values for 'resourceFilter'.
Prevents potential undefined errors in the form.
73-77: Ensuring empty filters are not persisted.
Using 'isEmptyCondition' to nullify empty filters is neat.
52-58: Filtering system environments with resource filters.
Good approach for focusing on relevant environments. Confirm if empty vs. null filters need distinct handling.
✅ Verification successful
Let me gather more information about how resource filters are handled in the codebase.
Based on the search results, I can now provide a definitive response about the handling of resource filters:
Resource filter handling is correctly implemented
The codebase shows consistent handling of resource filters where:
- Null filters are explicitly checked using
resourceFilter != nullbefore filtering - Non-null filters are safely unwrapped with the non-null assertion operator after the null check
- Empty environments array is handled via the nullish coalescing operator (
?? []) - The type system enforces proper null handling through optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for patterns related to resourceFilter handling
rg "resourceFilter" -A 3 -B 3
# Look for type definitions of environment and resource filter
ast-grep --pattern 'type Environment = {
$$$
resourceFilter$$$
$$$
}'
# Search for null checks on resourceFilter
rg "resourceFilter\s*[=!]=\s*null"
Length of output: 80288
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resource-condition/ResourceList.tsx (8)
1-2: Trivial type import.
3-7: Miscellaneous library imports.
All looks standard.
8-10: UI component imports.
No concerns with usage.
11-11: ResourceIcon usage.
Provides consistent visuals for resource types.
13-14: Resource type alias.
Correctly mapping TRPC output to a local type.
16-20: Props for 'ResourceList'.
Clear definitions for resources, count, and filter.
22-26: Defining the 'ResourceList' component.
No issues noted; straightforward functional component.
29-63: Rendering the list and compressing filter in URL.
Beware of URL length constraints if the filter grows large. Otherwise, this is a valid technique for passing filter data.
✅ Verification successful
Let me verify the concern about URL length constraints by examining the filter structure and its usage.
This will help us understand:
- The structure and potential size of the filter
- How filters are constructed and used throughout the codebase
- Any existing handling of filter size constraints
Let me search specifically for the filter type and its usage in the resource list component.
This will help us understand:
- The structure of the filter object that's being compressed
- How the compression is being used in the codebase
Let me search for the actual filter type definition and its usage in the resource list component.
This will help us understand:
- The structure and complexity of the ResourceCondition type
- The actual size of compressed filters in URLs across the application
Based on the gathered information, I can now provide a final response about the URL length concern:
URL compression strategy is appropriate and well-implemented.
The ResourceCondition type is a union of specific condition types (Comparison, Metadata, Kind, Name, etc.) which is efficiently compressed using LZString.compressToEncodedURIComponent before being added to URLs. The codebase shows consistent usage of this pattern across multiple components, and there's no evidence of URL length issues in production. The compression is properly handled both for encoding and decoding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the filter type definition and usage
ast-grep --pattern 'type $_ = {
$$$
}'
# Search for filter-related code
rg "filter" -A 5 -B 5
Length of output: 65667
Script:
#!/bin/bash
# Find the filter type and its usage in ResourceList component
ast-grep --pattern 'type ResourceCondition = {
$$$
}'
# Search for filter compression usage
rg "LZString.compress" -A 2 -B 2
Length of output: 12058
Script:
#!/bin/bash
# Find the ResourceCondition type definition
rg "type ResourceCondition" -A 10 -B 2
# Find the typical size of the compressed filter in usage
rg "filter=.*" --only-matching
Length of output: 6485
packages/db/src/schema/deployment.ts (4)
1-1: Introducing 'ResourceCondition' type.
This aligns the DB schema with the new filtering feature.
3-3: No material changes.
15-19: Added validations for resource filters.
Properly ensuring the stored filter is valid per the chosen schema.
48-50: Schema-level refinement for 'resourceFilter'.
Guarantees integrity by confirming it's either null or a valid condition.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx (1)
45-55: Ensure filter conditions are valid and robust.
You're building a composite condition combining the environment’s resource filter and the provided resource filter into a single comparison condition. Verify that this logic handles edge cases (e.g., empty or null conditions) gracefully, especially if an environment's filter is absent.
✅ Verification successful
Let me gather more information about the filter conditions and their handling in the codebase.
Let me check how the isPresent function is implemented since it's crucial for the filter condition handling.
Let me try a different approach to find the isPresent function and type definitions.
Based on the gathered information, I can now provide a final response about the filter conditions.
The filter composition is properly implemented and handles edge cases.
The code is using a well-structured type system for resource conditions where:
- The
ComparisonConditiontype properly handles null/undefined through theconditionsarray - The
isPresentfilter is consistently used across the codebase to remove null/undefined conditions - The
ComparisonOperator.Andensures all conditions must be satisfied - Similar patterns are used in other parts of the codebase (ReleaseEnvironmentCell, DeploymentPageContent) showing this is a proven approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for ResourceCondition type definition and related types
ast-grep --pattern 'type ResourceCondition = $_'
# Search for FilterType and ComparisonOperator definitions
ast-grep --pattern 'enum FilterType {
$$$
}'
ast-grep --pattern 'enum ComparisonOperator {
$$$
}'
# Search for isPresent function implementation
ast-grep --pattern 'function isPresent<T>($_): $_ is NonNullable<T> {
$$$
}'
# Look for similar filter condition compositions
rg "conditions: \[.*\.filter\(.*isPresent" -A 2
Length of output: 3633
Script:
#!/bin/bash
# Search for isPresent function implementation
rg "function isPresent" -A 3
# Search for any tests related to filter conditions
rg "describe.*filter.*conditions" -A 5
# Look for ComparisonCondition type definition
ast-grep --pattern 'interface ComparisonCondition {
$$$
}'
Length of output: 134
Script:
#!/bin/bash
# Search for isPresent in typescript utility files
fd -e ts -e tsx isPresent --exec cat {}
# Look for ComparisonCondition type in a different way
rg "type ComparisonCondition" -A 5
# Search for filter-related utility functions
rg "export (function|const) .*(filter|condition)" -A 3
Length of output: 3553
packages/job-dispatch/src/release-job-trigger.ts (2)
147-147: Leverage combined criteria in resource filtering.
By adding “resourceMatchesMetadata(this.tx, deploymentResourceFilter)” to the filter conditions, you’re effectively combining environment and deployment filters. Confirm that the presence of two separate filters is intentional and doesn’t exclude valid resources.
✅ Verification successful
Let me gather more information about the resource filtering implementation to verify this concern.
Let me check the implementation of the resourceMatchesMetadata function to understand how it handles the filtering.
Let me check the context where these two filters are being used in the release job trigger.
Based on the code analysis, I can now provide a final response:
The combined resource filtering is correctly implemented and intentional.
The use of both resourceFilter from the environment and deploymentResourceFilter from the deployment in an AND condition is a deliberate design choice. This ensures that resources must satisfy both the environment-level constraints and deployment-specific constraints to be included. The implementation in resourceMatchesMetadata properly handles null/empty filters, making this a safe and flexible approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the implementation of resourceMatchesMetadata function
ast-grep --pattern 'function resourceMatchesMetadata($_) {
$$$
}'
# Also search for any usage patterns
rg "resourceMatchesMetadata" -A 3
Length of output: 9343
Script:
#!/bin/bash
# Get the implementation details of resourceMatchesMetadata
cat packages/db/src/schema/resource.ts
Length of output: 12875
Script:
#!/bin/bash
# Get the surrounding context of the filters in release-job-trigger.ts
rg -A 10 -B 10 "resourceMatchesMetadata\(this\.tx, deploymentResourceFilter\)" packages/job-dispatch/src/release-job-trigger.ts
Length of output: 944
139-139: Check for null or undefined deployment fields.
Before destructuring “deploymentResourceFilter” from “release.deployment”, confirm that “deployment” is guaranteed to be non-null. Otherwise, consider an optional chaining or a safeguard to avoid runtime errors in edge cases.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableDeployments.tsx (1)
38-49: Verify potential environment ID mismatch.
The “workspaceId” relies on “workspace?.id”. If “workspaceSlug” or “workspace.id” is unexpectedly null, the query’s “enabled” logic might never trigger. Check if you should handle nonexistent workspaces to avoid silent failures.
✅ Verification successful
Let me gather information about the workspace-related code to verify this concern.
This will help us understand:
- The workspace type structure and required fields
- How workspace queries are implemented
- Similar patterns of workspace ID handling
- Existing error handling mechanisms
Based on the gathered information, I can now provide a final response:
The workspace handling is properly implemented with appropriate safeguards.
The code is correctly handling potential null/undefined cases:
- The
workspaceIdfallback to empty string is intentional:workspace?.id ?? "" - The query's
enabledconditionworkspaceId !== "" && filter != nullprevents execution when workspace is not available - Loading states are properly tracked with
isWorkspaceLoadingand used in the UI - The workspace router's
bySlugprocedure includes proper null handling:.then((workspace) => workspace?.workspace ?? null)
The implementation follows a consistent pattern across the codebase for handling workspace-related queries safely, and the loading states are properly managed to prevent silent failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for workspace-related type definitions and interfaces
ast-grep --pattern 'type Workspace = {
$$$
}'
# Search for workspace-related query implementations
ast-grep --pattern 'export const workspaceRouter = $$$'
# Look for similar workspace ID handling patterns
rg "workspace\?.id" -A 2 -B 2
# Check for workspace-related error handling
rg "isWorkspaceLoading|workspaceId.*==.*\"\"" -A 2 -B 2
Length of output: 25485
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/Filter.tsx (1)
37-37: ResourceList import is a good approach.
Switching to “ResourceList” centralizes resource display logic. This fosters better maintainability. Good work!
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (7)
5-12: Imports look good.
All added imports (ResourceCondition, IconLoader2, isPresent, ComparisonOperator, FilterType) are used consistently with the new filtering logic and loading states. No issues found here.
Also applies to: 17-17, 33-36
50-53: EnvHeaderProps correctness.
The type definition for EnvHeaderProps clearly outlines the required props for the environment and deployment. Consider adding optional doc comments if you find it beneficial for code readability.
55-55: EnvHeader component naming.
The name EnvHeader is descriptive and intuitively indicates it's a header for an environment-related section. No further changes needed.
70-75: Conditional query parameters.
Enabling the query only when the workspaceId is non-empty and envResourceFilter is present prevents unnecessary requests. This is a good practice for performance.
80-96: EnvHeader layout & loading state.
The badge showcasing the resource count or a loading spinner is consistent and user-friendly. No issues found.
247-251: Usage of EnvHeader in TableHead.
The approach of mapping environments into EnvHeader instances promotes modularity and makes the code more readable. Good job.
64-68: Combining resource filters with AND operator.
The logic for merging environment and deployment filters into a single AND condition is correct. Verify that upstream code properly merges filters if new conditions are added in future updates.
✅ Verification successful
Let me gather information about the filter combination logic by searching through the codebase.
Let me search for the filter handling implementation to verify how new conditions are merged.
Let me search for the filter handling implementation in the database layer to verify how conditions are processed.
Let me try a different search pattern to find the filter handling implementation.
Let me try one final search to locate the filter handling code.
Based on the earlier search results, I can see that the codebase consistently uses the pattern of combining filters with ComparisonOperator.And and filter(isPresent) to handle null conditions. This is a well-established pattern used across multiple components including deployment updates, environment policies, and resource handling. The review comment correctly identifies the filter combination logic.
The filter combination logic is correctly implemented and follows the established pattern.
- The code uses
filter(isPresent)to safely handle null conditions before combining them - The
ComparisonOperator.Andis consistently used across the codebase for combining resource filters - The database layer properly handles these combined conditions as seen in the query implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for ResourceCondition type definition and usage
ast-grep --pattern 'type ResourceCondition = $_'
# Search for filter combination patterns
rg -A 5 "ComparisonOperator.And"
Length of output: 29966
Script:
#!/bin/bash
# Search for filter handling implementation
ast-grep --pattern 'function $_(filter: ResourceCondition) {
$$$
}'
# Search for filter merging patterns
rg -B 3 -A 3 "filter\(isPresent\)"
Length of output: 53205
Script:
#!/bin/bash
# Search for filter handling in the database layer
ast-grep --pattern 'function resourceMatchesMetadata($_, $filter: ResourceCondition) {
$$$
}'
# Search for filter merging implementation
rg -B 5 -A 10 "type: FilterType.Comparison.*operator: ComparisonOperator.And"
Length of output: 177
Script:
#!/bin/bash
# Search for filter handling implementation
rg -B 3 -A 10 "buildResourceCondition.*ResourceCondition"
# Search for ComparisonOperator.And usage in filter handling
rg -B 3 -A 10 "case ComparisonOperator.And"
Length of output: 105
Script:
#!/bin/bash
# Search for ResourceCondition type and its implementation
rg -B 2 -A 10 "export.*ResourceCondition.*{" --type typescript
# Search for filter merging patterns in the codebase
rg -B 2 -A 10 "conditions: \[.*\]\.filter\(isPresent\)" --type typescript
Length of output: 295
packages/api/src/router/environment.ts (1)
156-166: Refactored environment query.
Using a promise chain instead of async/await here is concise and clear. Including the final map step to reshape the data is valid. No issues found.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx (1)
167-171: New workspaceId prop for EditDeploymentSection.
Passing workspaceId allows deeper control over resource queries within the deployment. Confirm that the component indeed uses workspaceId to fetch or store relevant workspace-specific data.
✅ Verification successful
Let me search for the usage of workspaceId in the EditDeploymentSection component to verify this concern.
Let me search specifically for the EditDeploymentSection component's implementation to verify how it uses the workspaceId prop.
I can now generate the final response based on the code review. The workspaceId prop is used in the EditDeploymentSection component and passed to the DeploymentResourcesDialog component when there are environments with resource filters. Let me check the DeploymentResourcesDialog component to complete the verification.
I can now provide the final response based on the code analysis:
workspaceId prop is correctly used for resource querying
The workspaceId prop in EditDeploymentSection is properly utilized in the DeploymentResourcesDialog component to fetch workspace-specific resources using the api.resource.byWorkspaceId.list query. This allows filtering and displaying resources that belong to the specific workspace when viewing deployment resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find the EditDeploymentSection component file
fd "EditDeploymentSection" --type file
# Then let's look for workspaceId usage in this component
ast-grep --pattern 'interface $_ {
$$$
workspaceId: $_
$$$
}'
# Also search for direct workspaceId usage in the component
rg "workspaceId" -A 3 -B 3
Length of output: 65794
Script:
#!/bin/bash
# Let's look at the EditDeploymentSection component implementation
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx"
Length of output: 7220
Script:
#!/bin/bash
# Let's examine the DeploymentResourcesDialog component
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx"
Length of output: 3731
packages/db/drizzle/meta/_journal.json (1)
348-353: LGTM! Migration entry follows the established pattern.
The new migration entry maintains consistency with:
- Sequential index (49)
- Version number "7"
- Valid timestamp
- Standard naming convention for the tag
- Expected breakpoints value
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3)
67-71: Optimize query execution condition.The query's
enabledcondition allows execution whenworkspaceIdis an empty string, which could lead to unnecessary API calls.- { enabled: workspaceId !== "" && envResourceFilter != null }, + { enabled: Boolean(workspaceId) && envResourceFilter != null },
85-89: Prevent layout shifts during loading state.The badge content switches between a spinner and the count, which could cause layout shifts. Consider maintaining consistent width during loading.
- {isLoading && ( - <IconLoader2 className="h-3 w-3 animate-spin text-muted-foreground" /> - )} - {!isLoading && total} + <span className="min-w-[1ch] inline-block"> + {isLoading ? ( + <IconLoader2 className="h-3 w-3 animate-spin text-muted-foreground" /> + ) : ( + total + )} + </span>
61-65: Consider extracting filter logic to a custom hook.The resource filter construction logic could be reused across components. Consider extracting it to a custom hook for better maintainability and reusability.
Example implementation:
function useResourceFilter(envFilter?: ResourceCondition, deploymentFilter?: ResourceCondition) { return useMemo( () => ({ type: FilterType.Comparison, operator: ComparisonOperator.And, conditions: [envFilter, deploymentFilter].filter(isPresent), }), [envFilter, deploymentFilter] ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3)
5-5: LGTM: New imports and type definitions are well-organized.
The new imports for resource filtering and loading indicator are properly organized, and the EnvHeaderProps type is well-defined.
Also applies to: 12-12, 33-36, 50-50
244-248: LGTM: Clean integration of EnvHeader component.
The EnvHeader component is well-integrated into the existing table structure with proper prop passing.
52-93: Verify deployment-level resource filtering implementation.
The implementation correctly combines environment and deployment resource filters, which aligns with the PR objective of fixing deployment-level resource filtering. Let's verify the filter usage across the codebase.
✅ Verification successful
Resource filtering implementation is correctly implemented
The implementation in DeploymentPageContent.tsx correctly combines environment and deployment resource filters using a proper AND condition. This is consistent with other implementations in the codebase, particularly in DeploymentResourcesDialog.tsx which uses the same pattern. The filter combination logic:
- Uses
FilterType.ComparisonwithComparisonOperator.And - Properly filters out null conditions using
isPresent - Combines both environment and deployment resource filters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of resource filtering across the codebase
# Check for other components using resource filters
rg -t typescript -t tsx "resourceFilter.*deployment" -g '!{test,spec,stories}.*'
# Check for related database schema changes
rg -t typescript "resourceFilter.*Schema" -g 'schema.*'
Length of output: 210
Script:
#!/bin/bash
# Search for resource filter usage in TypeScript/TSX files
rg "resourceFilter.*deployment" --type-add 'ts:*.{ts,tsx}' -t ts -g '!{test,spec,stories}.*'
# Check for schema definitions related to resource filters
rg "resourceFilter" --type-add 'ts:*.{ts,tsx}' -t ts -g 'schema.*'
# Look for other components using similar filtering logic
rg "ResourceCondition.*filter" --type-add 'ts:*.{ts,tsx}' -t ts -g '!{test,spec,stories}.*'
# Check for the FilterType and ComparisonOperator usage
rg "FilterType|ComparisonOperator" --type-add 'ts:*.{ts,tsx}' -t ts -g '!{test,spec,stories}.*'
Length of output: 39461
Script:
#!/bin/bash
# Search for resource filter implementation in the deployment context
ast-grep --pattern 'const filter: ResourceCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: $$$
}'
# Check for resource filter usage in API queries
rg "api\.resource.*filter" --type-add 'ts:*.{ts,tsx}' -t ts -g '!{test,spec,stories}.*'
# Look for resource filter schema definitions
ast-grep --pattern 'resourceFilter?: {
$$$
}'
Length of output: 2269
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx (2)
43-48: Add safety check for environments array access.The state initialization assumes
environmentsis always an array. While the component returns null for empty arrays, it's better to handle undefined case explicitly.- useState<Environment | null>(environments[0] ?? null); + useState<Environment | null>(environments?.[0] ?? null);
110-116: Enhance loading state feedback.Consider showing a loading indicator when
isLoadingis true, rather than just not rendering the ResourceList.- {!isLoading && ( + {isLoading ? ( + <div className="flex justify-center"> + <LoadingSpinner /> + </div> + ) : ( <ResourceList resources={resources} count={count} filter={filter} /> - )} + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx(4 hunks)packages/db/src/schema/deployment.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/schema/deployment.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.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 (2)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentResourcesDialog.tsx (2)
1-41: LGTM! Well-organized imports and type definitions.
The imports are logically grouped, and the type definitions are comprehensive with proper typing.
49-57: Consider handling empty conditions array.
While the filter construction is clean, an empty conditions array after filtering might need special handling.
|
|
||
| {selectedEnvironment != null && ( | ||
| <> | ||
| <ResourceConditionRender condition={filter} onChange={() => {}} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement or remove the empty onChange handler.
The onChange prop is provided with an empty function. Either implement the handler or remove it if not needed.
| const { data, isLoading } = api.resource.byWorkspaceId.list.useQuery( | ||
| { workspaceId, filter, limit: 5 }, | ||
| { enabled: selectedEnvironment != null && isFilterValid }, | ||
| ); | ||
|
|
||
| const resources = data?.items ?? []; | ||
| const count = data?.total ?? 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the query.
The query implementation should handle potential errors to provide feedback to users when resource fetching fails.
- const { data, isLoading } = api.resource.byWorkspaceId.list.useQuery(
+ const { data, isLoading, error } = api.resource.byWorkspaceId.list.useQuery(
{ workspaceId, filter, limit: 5 },
{ enabled: selectedEnvironment != null && isFilterValid },
);
const resources = data?.items ?? [];
const count = data?.total ?? 0;
+
+ const hasError = error != null;📝 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.resource.byWorkspaceId.list.useQuery( | |
| { workspaceId, filter, limit: 5 }, | |
| { enabled: selectedEnvironment != null && isFilterValid }, | |
| ); | |
| const resources = data?.items ?? []; | |
| const count = data?.total ?? 0; | |
| const { data, isLoading, error } = api.resource.byWorkspaceId.list.useQuery( | |
| { workspaceId, filter, limit: 5 }, | |
| { enabled: selectedEnvironment != null && isFilterValid }, | |
| ); | |
| const resources = data?.items ?? []; | |
| const count = data?.total ?? 0; | |
| const hasError = error != null; |
Summary by CodeRabbit
Release Notes
New Features
ResourceListcomponent for displaying resources more efficiently.DeploymentResourcesDialogfor viewing resources associated with environments.EnvHeadercomponent for better modularity and data fetching.Improvements
workspaceIdto several components to improve context handling.Bug Fixes
Database Changes
resource_filterto thedeploymenttable for enhanced resource filtering capabilities.