fix: Clean up system filters #240
Conversation
WalkthroughThis pull request involves the deletion of several components and types related to filter management in a web application. Specifically, the files 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsTable.tsx (1)
18-19: Add accessibility attributes to enhance user experienceThe table structure could benefit from improved accessibility:
- <Table className="table-fixed"> + <Table className="table-fixed" aria-label="Systems list"> <TableBody> {systems.map((system) => ( <TableRow - className="cursor-pointer border-b-neutral-800/50" + className="border-b-neutral-800/50" key={system.id}Also applies to: 20-24
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx (2)
18-25: Consider these improvements to the search implementationThe search implementation is good, but could be enhanced for better maintainability.
Consider these improvements:
+ const SEARCH_DEBOUNCE_MS = 500; + const [query, setQuery] = useState<string | undefined>(undefined); const [debouncedQuery, setDebouncedQuery] = useState<string | undefined>(undefined); - useDebounce(() => setDebouncedQuery(query == "" ? undefined : query), 500, [query]); + useDebounce(() => setDebouncedQuery(query?.trim() || undefined), SEARCH_DEBOUNCE_MS, [query]);This change:
- Extracts the magic number into a named constant
- Uses optional chaining and trim() for more robust empty string handling
11-11: Consider moving SearchInput to shared componentsThe SearchInput component is currently imported from the targets feature folder but is being reused here. This creates unwanted coupling between features.
Consider moving the SearchInput component to a shared UI components folder to improve modularity and reusability. This would make the component's purpose clearer and prevent potential circular dependencies.
- import { SearchInput } from "../(targets)/targets/TargetPageContent"; + import { SearchInput } from "@ctrlplane/ui/search-input";Also applies to: 33-34
packages/api/src/router/system.ts (1)
39-39: Consider performance and search behavior optimizationsWhile the implementation is secure, consider these improvements:
- Add case-insensitive search if not handled by DB collation
- Consider full-text search for better performance with large datasets
- Add input trimming to handle whitespace consistently
- input.query ? like(system.name, `%${input.query}%`) : undefined, + input.query ? like(system.name, `%${input.query.trim()}%`) : undefined,For future scalability, consider:
- Adding database indexes optimized for text search
- Implementing full-text search capabilities (e.g., using PostgreSQL's
tsvector)- Caching frequently searched results
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/Filter.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/FilterDropdown.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/FilterDropdownItems.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemFilter.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsTable.tsx(1 hunks)packages/api/src/router/system.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/Filter.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/FilterDropdown.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/FilterDropdownItems.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemFilter.tsx
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsTable.tsx (1)
1-16: LGTM! Clean and well-structured component interface
The component's interface is well-defined with proper TypeScript types and minimal, focused imports that align with the simplified table implementation.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx (1)
27-30: Consider adding error handling for the API query
While the query implementation is good, it lacks error handling which could impact user experience if the API call fails.
Let's verify the error handling in the API route:
Consider adding error handling:
const systems = api.system.list.useQuery(
{ workspaceId: workspace.id, query: debouncedQuery },
- { placeholderData: (prev) => prev },
+ {
+ placeholderData: (prev) => prev,
+ onError: (error) => {
+ console.error('Failed to fetch systems:', error);
+ // Add error notification here
+ }
+ },
);packages/api/src/router/system.ts (2)
5-5: LGTM: Import cleanup aligns with simplified filtering
The removal of unused imports and retention of necessary ones reflects the architectural shift to simpler filtering.
29-29: Verify client-side adaptations to the new query parameter
The simplified input schema is a good improvement. Let's verify that all client-side components have been updated to use the new query parameter instead of the old filters array.
✅ Verification successful
All client-side components are already using the new query parameter structure
Based on the search results, all client-side components in the webservice app are correctly using the new simplified API structure with just workspaceId and optional query parameter. The schema definition in packages/api/src/router/system.ts shows the current implementation, and all API calls from various components (SystemsList.tsx, AppSidebar.tsx, SearchDialog.tsx, etc.) are aligned with this schema. No references to the old filters array were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old filters array in client code
rg -t tsx -t ts "filters.*workspaceId"
# Search for components using the new query parameter
rg -t tsx -t ts "query.*workspaceId.*system"
Length of output: 143
Script:
#!/bin/bash
# Search for any remaining references to the old filters array in client code
rg "filters.*workspaceId"
# Search for components using the new query parameter
rg "query.*workspaceId.*system"
# Let's also check for any TypeScript/JavaScript files specifically
fd "\.(ts|js|tsx|jsx)$" --exec rg -l "filters.*workspaceId" {} \;
# And check for API calls related to system endpoints
rg "system.*workspaceId"
Length of output: 4535
Script:
#!/bin/bash
# Let's check the specific file for the old schema definition
rg -A 5 "input = z\.object" packages/api/src/router/system.ts
# And check for any client-side API calls that might show the request structure
rg -A 5 "system\.list" packages/api/src/client
# Also check for any test files that might reveal the old structure
fd "test|spec" --exec rg "system.*list.*filters" {} \;
Length of output: 236
Script:
#!/bin/bash
# Let's look at the actual API usage in the web application
rg -A 5 "api\.system\.list" apps/webservice/
# Check the current schema definition
rg -B 5 -A 10 "query.*optional" packages/api/src/router/system.ts
# Look for any migration files or changes that might show the schema evolution
fd "migration|schema" --exec rg "system.*filters" {} \;
Length of output: 5129
| <TableCell | ||
| onClick={() => | ||
| router.push( | ||
| `/${workspaceSlug}/systems/${system.slug}/deployments`, | ||
| ) | ||
| } | ||
| > | ||
| {system.name} | ||
| </TableCell> | ||
| <TableCell> | ||
| <div className="flex justify-end"> | ||
| <SystemActionsDropdown system={system}> | ||
| <Button variant="ghost" size="icon" className="h-6 w-6"> | ||
| <IconDotsVertical className="h-3 w-3" /> | ||
| </Button> | ||
| </SystemActionsDropdown> | ||
| </div> | ||
| </TableCell> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize table structure and interaction handling
Consider these improvements to the table structure:
- Move the cursor-pointer class to the clickable cell only
- Extract the navigation handler for better reusability
- Add aria-label to the actions button for accessibility
key={system.id}
>
<TableCell
+ className="cursor-pointer"
onClick={() =>
router.push(
`/${workspaceSlug}/systems/${system.slug}/deployments`,
)
}
>
{system.name}
</TableCell>
<TableCell>
<div className="flex justify-end">
<SystemActionsDropdown system={system}>
- <Button variant="ghost" size="icon" className="h-6 w-6">
+ <Button
+ variant="ghost"
+ size="icon"
+ className="h-6 w-6"
+ aria-label={`Actions for ${system.name}`}
+ >
<IconDotsVertical className="h-3 w-3" />
</Button>
</SystemActionsDropdown>Consider extracting the navigation handler:
const handleSystemClick = (systemSlug: string) => {
router.push(`/${workspaceSlug}/systems/${systemSlug}/deployments`);
};| export const SystemsList: React.FC<{ | ||
| workspace: Workspace; | ||
| systemsCount: number; | ||
| }> = ({ workspace, systemsCount }) => { | ||
| const { filters, addFilters, removeFilter, clearFilters } = | ||
| useFilters<SystemFilter>(); | ||
| }> = ({ workspace }) => { |
There was a problem hiding this comment.
Remove unused prop from type definition
The systemsCount prop is defined in the component's type but not used in the implementation. This creates unnecessary complexity in the type system.
Apply this diff to fix the type definition:
export const SystemsList: React.FC<{
workspace: Workspace;
- systemsCount: number;
}> = ({ workspace }) => {📝 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.
| export const SystemsList: React.FC<{ | |
| workspace: Workspace; | |
| systemsCount: number; | |
| }> = ({ workspace, systemsCount }) => { | |
| const { filters, addFilters, removeFilter, clearFilters } = | |
| useFilters<SystemFilter>(); | |
| }> = ({ workspace }) => { | |
| export const SystemsList: React.FC<{ | |
| workspace: Workspace; | |
| }> = ({ workspace }) => { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx (1)
33-34: Consider adding accessibility attributes to the search inputThe search functionality would benefit from proper accessibility attributes to improve screen reader support.
- <SearchInput value={query ?? ""} onChange={setQuery} /> + <SearchInput + value={query ?? ""} + onChange={setQuery} + aria-label="Search systems" + />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/page.tsx(1 hunks)
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/page.tsx (2)
39-39: LGTM! Verify styling implications.
The removal of the wrapper div aligns with the simplified approach. However, ensure that the SystemsList component properly handles its own styling and scroll behavior.
✅ Verification successful
Styling is properly handled in SystemsList component
The SystemsList component already implements proper styling and scroll behavior:
- Has a border and padding for the search input section
- Implements custom scrollbar styling with proper height calculation
- Contains proper spacing and layout classes
The removal of the wrapper div from the parent component is safe as the SystemsList component handles its own styling requirements comprehensively using Tailwind CSS classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SystemsList component has proper styling definitions
# Look for style-related code in SystemsList component
# Check for styling in SystemsList
ast-grep --pattern 'className = $_' SystemsList.tsx
# Check for CSS module imports
rg -l '\.module\.(css|scss)' apps/webservice/src/app/
Length of output: 172
Script:
#!/bin/bash
# First, let's find the correct path to SystemsList component
fd -e tsx -e ts SystemsList
# Then, let's check for styling in the component
rg -A 2 'className=' $(fd -e tsx -e ts SystemsList)
# Check for any style imports in the component
rg -A 1 'import.*style|from.*style' $(fd -e tsx -e ts SystemsList)
# Look for Tailwind classes or other styling approaches
rg 'className=' $(fd -e tsx -e ts SystemsList)
Length of output: 1325
Line range hint 1-42: Verify the impact of filter removal on search functionality.
The removal of the complex filtering system in favor of a simplified search approach is a good architectural decision. However, we should verify that all essential filtering capabilities are maintained through the new search implementation.
✅ Verification successful
Based on the examination of the SystemsList component and related files, I can now generate the final response:
Search functionality properly replaces the previous filtering system
The implementation shows a well-designed search approach that:
- Uses debounced queries for performance optimization
- Maintains real-time search capabilities through the TRPC API
- Provides immediate feedback with loading states
- Preserves previous results as placeholder data for smooth UX
The simplified search-based approach is sufficient for the use case, as it:
- Allows free-text search across systems
- Maintains responsive UI with proper loading states
- Integrates cleanly with the backend API
- Provides a better user experience than complex filtering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new search implementation covers essential filtering needs
# Check for search-related code in SystemsList
rg -A 5 'search|filter|query' apps/webservice/src/app/*/systems/SystemsList.tsx
# Look for any remaining filter-related code that might have been missed
rg -l 'filter|Filter' apps/webservice/src/app/
Length of output: 11182
Script:
#!/bin/bash
# Let's check the actual implementation of SystemsList component and search functionality
fd -e tsx "SystemsList.tsx" apps/webservice/src/app/
# After finding the file, let's examine its content
cat $(fd -e tsx "SystemsList.tsx" apps/webservice/src/app/)
Length of output: 1972
Script:
#!/bin/bash
# Let's examine the API implementation to understand the search functionality
# First, let's find the system.list API implementation
fd -e ts "system" apps/webservice/src/server/api/routers/
# After finding it, let's look at its content
cat $(fd -e ts "system" apps/webservice/src/server/api/routers/)
Length of output: 376
Script:
#!/bin/bash
# Let's try a different approach to find the API implementation
# First, let's find all files that might contain the system list implementation
rg -l "system.list" --type ts
# Let's also check for any trpc router definitions related to systems
rg -l "createTRPCRouter.*system" --type ts
Length of output: 813
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsList.tsx (3)
14-17: Remove unused prop from type definition
The systemsCount prop is defined in the component's type but not used in the implementation. This creates unnecessary complexity in the type system.
18-25: LGTM! Well-implemented debounced search.
The implementation uses proper debouncing with a reasonable delay and handles empty strings appropriately.
27-30: Verify API changes across the codebase
The API query structure has changed from filters to a single query parameter. Let's verify this change is consistent across the codebase.
✅ Verification successful
API changes are consistent across the codebase
All instances of system.list.useQuery are using the new API structure with workspaceId and optional query parameters. The router implementation in system.ts confirms this is the correct interface. No inconsistencies found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old filter-based API calls
# Search for old filter-based API calls
rg -A 3 "system.list.useQuery.*filters"
# Search for potential direct trpc calls with filters
rg -A 3 "caller.system.list.*filters"
Length of output: 81
Script:
#!/bin/bash
# Let's broaden our search to find all system.list.useQuery usage patterns
rg -A 3 "system\.list\.useQuery"
# Also search for any direct router calls
rg -A 3 "router\.system\.list"
# And check the API definition itself
fd "system.ts" --exec cat {}
Length of output: 19018
Summary by CodeRabbit
New Features
Bug Fixes
Refactor