Fix admin nodes list and filtering #2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ai-coding-guardrails[bot]](https://avatars.githubusercontent.com/in/1202546?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've got 26 comments for you to consider
The PR title and description are not accurate. Here are my suggestions:
Title: Comprehensive admin list filtering system overhaul with enhanced state management and error handling
Description: This PR implements a major refactoring of the admin list filtering and pagination system with the following key improvements:
Core Changes:
- Replaced manual state management with centralized state hooks (useAdminListState)
- Enhanced AdminList, AdminNodes, and AdminListTable components with better error handling
- Upgraded filter controls with validation, loading states, and user feedback
- Added comprehensive error boundaries and recovery mechanisms
- Implemented performance optimizations including memoization and debouncing
New Features:
- URL synchronization for filters and pagination state
- Settings persistence across sessions
- Enhanced accessibility with improved ARIA labels
- Comprehensive integration and unit test coverage
- Performance monitoring and error logging
Infrastructure:
- Updated Node.js base image from 18-alpine to 20-alpine
- Added extensive project documentation and architecture specs
- Created comprehensive design documents and implementation plans
Testing:
- Added integration tests for complex filtering workflows
- Pagination validation and edge case handling
- State synchronization and error recovery scenarios
This change significantly improves the reliability, performance, and user experience of the admin interface while establishing a solid foundation for future enhancements.
List of skipped files due to configuration
WARNING: The number of files to review exceeded the limit. We reviewed some files and skipped others.
Skipped files due to being over the limit
- modules/admin/components/AdminLists/__tests__/AdminNodes.integration.test.tsx
- modules/admin/components/AdminLists/__tests__/AdminNodesFilters.integration.test.tsx
- modules/admin/components/AdminLists/__tests__/AdminNodesPagination.integration.test.tsx
- modules/admin/components/index.ts
- modules/admin/examples/useAdminListStateExample.tsx
- modules/admin/hooks/README.md
- modules/admin/hooks/__tests__/useAdminListErrorHandling.test.ts
- modules/admin/hooks/__tests__/useAdminListState.integration.test.ts
- modules/admin/hooks/__tests__/useUpdateQueryString.test.ts
- modules/admin/hooks/index.ts
- modules/admin/hooks/useAdminListErrorHandling.ts
- modules/admin/hooks/useAdminListState.ts
- modules/admin/hooks/useUpdateQueryString.ts
- modules/admin/index.ts
- modules/admin/store/adminSelectors.ts
- modules/admin/types/AdminFilterControlProps.d.ts
- modules/admin/types/AdminListState.d.ts
- modules/admin/types/AdminQuery.d.ts
- modules/admin/utils/README.md
- modules/admin/utils/__tests__/enhancedFilterIntegration.test.ts
- modules/admin/utils/__tests__/errorHandling.test.ts
- modules/admin/utils/__tests__/filterStateIntegration.test.ts
- modules/admin/utils/__tests__/filterStateManager.test.ts
- modules/admin/utils/__tests__/paginationIntegration.test.ts
- modules/admin/utils/__tests__/paginationManager.test.ts
- modules/admin/utils/__tests__/performanceIntegration.test.ts
- modules/admin/utils/__tests__/performanceOptimization.test.ts
- modules/admin/utils/__tests__/settingsPersistence.integration.test.ts
- modules/admin/utils/__tests__/stateSynchronization.test.ts
- modules/admin/utils/__tests__/stateSynchronizationIntegration.test.ts
- modules/admin/utils/__tests__/urlParameterUtils.test.ts
- modules/admin/utils/adminListStateUtils.ts
- modules/admin/utils/enhancedFilterIntegration.ts
- modules/admin/utils/errorHandling.ts
- modules/admin/utils/filterStateManager.ts
- modules/admin/utils/index.ts
- modules/admin/utils/paginationManager.ts
- modules/admin/utils/performanceOptimization.ts
- modules/admin/utils/stateSynchronization.ts
- modules/admin/utils/urlParameterUtils.ts
- next.config.js
- package.json
- test/setup.ts
- tsconfig.dev.json
- tsconfig.json
- vitest.config.ts
You can split this PR into smaller ones to get those files reviewed.
Are we missing a feature you'd like? Submit it here!
Comment on lines
+59
to
+60
| docker build --build-arg NEXT_PUBLIC_VERCEL_ENV=production \ | ||
| --build-arg NEXT_PUBLIC_API_URL=value -t blockvisor . |
There was a problem hiding this comment.
Critical Risk: The Docker build command contains a placeholder value NEXT_PUBLIC_API_URL=value which could lead to broken deployments if copied directly. This should either use a real example URL or clearly indicate it's a placeholder.
Suggested change
| docker build --build-arg NEXT_PUBLIC_VERCEL_ENV=production \ | |
| --build-arg NEXT_PUBLIC_API_URL=value -t blockvisor . | |
| docker build --build-arg NEXT_PUBLIC_VERCEL_ENV=production \ | |
| --build-arg NEXT_PUBLIC_API_URL=https://api.example.com -t blockvisor . |
Comment on lines
+77
to
+94
| it('should render without crashing', () => { | ||
| // This test verifies that the component structure is correct | ||
| // and that the refactoring maintains the basic functionality | ||
| expect(() => { | ||
| // Component would render here in a real test environment | ||
| // For now, we're just checking that the component can be imported | ||
| // and the props interface is correct | ||
| const component = AdminList; | ||
| expect(component).toBeDefined(); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| it('should have the correct prop types', () => { | ||
| // Verify that the component accepts the expected props | ||
| const component = AdminList; | ||
| expect(typeof component).toBe('function'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests don't actually test the component functionality - they only verify the component exists and is a function. The tests mock all dependencies but never render the component or verify its behavior. Consider using a testing library like @testing-library/react to render the component and test actual functionality:
Suggested change
| it('should render without crashing', () => { | |
| // This test verifies that the component structure is correct | |
| // and that the refactoring maintains the basic functionality | |
| expect(() => { | |
| // Component would render here in a real test environment | |
| // For now, we're just checking that the component can be imported | |
| // and the props interface is correct | |
| const component = AdminList; | |
| expect(component).toBeDefined(); | |
| }).not.toThrow(); | |
| }); | |
| it('should have the correct prop types', () => { | |
| // Verify that the component accepts the expected props | |
| const component = AdminList; | |
| expect(typeof component).toBe('function'); | |
| }); | |
| }); | |
| import { render, screen } from '@testing-library/react'; | |
| it('should render without crashing', () => { | |
| render(<AdminList {...mockProps} />); | |
| expect(screen.getByRole('table')).toBeInTheDocument(); // or appropriate element | |
| }); |
Comment on lines
+109
to
+113
| enableMonitoring: true, | ||
| logMetrics: process.env.NODE_ENV === 'development', | ||
| slowOperationThreshold: 100, | ||
| maxMetricsHistory: 30, | ||
| }); |
There was a problem hiding this comment.
Critical: Potential security vulnerability with environment variable exposure
The logMetrics configuration directly exposes process.env.NODE_ENV which could leak environment information in client-side code. In React components, environment variables can be bundled and exposed to the browser.
Suggested change
| enableMonitoring: true, | |
| logMetrics: process.env.NODE_ENV === 'development', | |
| slowOperationThreshold: 100, | |
| maxMetricsHistory: 30, | |
| }); | |
| logMetrics: false, // Consider using a build-time constant or secure configuration |
Comment on lines
+676
to
+711
| export const AdminList = memo(AdminListComponent, (prevProps, nextProps) => { | ||
| // Custom comparison function to prevent unnecessary re-renders | ||
| const keysToCompare = [ | ||
| 'name', | ||
| 'idPropertyName', | ||
| 'hidePagination', | ||
| 'defaultSortField', | ||
| 'defaultSortOrder', | ||
| 'selectedIds', | ||
| 'tagsAdded', | ||
| 'tagsRemoved', | ||
| ]; | ||
|
|
||
| for (const key of keysToCompare) { | ||
| if (prevProps[key as keyof Props] !== nextProps[key as keyof Props]) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Deep compare arrays | ||
| if (JSON.stringify(prevProps.columns) !== JSON.stringify(nextProps.columns)) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( | ||
| JSON.stringify(prevProps.protocols) !== JSON.stringify(nextProps.protocols) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (JSON.stringify(prevProps.users) !== JSON.stringify(nextProps.users)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }); |
There was a problem hiding this comment.
High: Performance anti-pattern in memo comparison function
The memo comparison uses JSON.stringify() for deep comparison of arrays, which is expensive and defeats the purpose of memoization. This will cause performance degradation on every render.
Suggested change
| export const AdminList = memo(AdminListComponent, (prevProps, nextProps) => { | |
| // Custom comparison function to prevent unnecessary re-renders | |
| const keysToCompare = [ | |
| 'name', | |
| 'idPropertyName', | |
| 'hidePagination', | |
| 'defaultSortField', | |
| 'defaultSortOrder', | |
| 'selectedIds', | |
| 'tagsAdded', | |
| 'tagsRemoved', | |
| ]; | |
| for (const key of keysToCompare) { | |
| if (prevProps[key as keyof Props] !== nextProps[key as keyof Props]) { | |
| return false; | |
| } | |
| } | |
| // Deep compare arrays | |
| if (JSON.stringify(prevProps.columns) !== JSON.stringify(nextProps.columns)) { | |
| return false; | |
| } | |
| if ( | |
| JSON.stringify(prevProps.protocols) !== JSON.stringify(nextProps.protocols) | |
| ) { | |
| return false; | |
| } | |
| if (JSON.stringify(prevProps.users) !== JSON.stringify(nextProps.users)) { | |
| return false; | |
| } | |
| return true; | |
| }); | |
| export const AdminList = memo(AdminListComponent, (prevProps, nextProps) => { | |
| // Use shallow comparison or a proper deep comparison library | |
| const keysToCompare = [ | |
| 'name', | |
| 'idPropertyName', | |
| 'hidePagination', | |
| 'defaultSortField', | |
| 'defaultSortOrder', | |
| 'selectedIds', | |
| 'tagsAdded', | |
| 'tagsRemoved', | |
| ]; | |
| for (const key of keysToCompare) { | |
| if (prevProps[key as keyof Props] !== nextProps[key as keyof Props]) { | |
| return false; | |
| } | |
| } | |
| // Use length check and reference equality for better performance | |
| if (prevProps.columns.length !== nextProps.columns.length || | |
| prevProps.protocols?.length !== nextProps.protocols?.length || | |
| prevProps.users?.length !== nextProps.users?.length) { | |
| return false; | |
| } | |
| return true; | |
| }); |
Comment on lines
+10
to
+162
| background-color: #fef2f2; | ||
| border: 1px solid #fecaca; | ||
| border-radius: 8px; | ||
| margin: 1rem 0; | ||
| min-height: 200px; | ||
| text-align: center; | ||
| `, | ||
|
|
||
| errorIcon: css` | ||
| font-size: 3rem; | ||
| margin-bottom: 1rem; | ||
| `, | ||
|
|
||
| errorTitle: css` | ||
| color: #dc2626; | ||
| font-size: 1.5rem; | ||
| font-weight: 600; | ||
| margin: 0 0 1rem 0; | ||
| `, | ||
|
|
||
| errorMessage: css` | ||
| color: #7f1d1d; | ||
| font-size: 1rem; | ||
| line-height: 1.5; | ||
| margin: 0 0 1.5rem 0; | ||
| max-width: 500px; | ||
| `, | ||
|
|
||
| errorDetails: css` | ||
| margin: 1rem 0; | ||
| text-align: left; | ||
| width: 100%; | ||
| max-width: 600px; | ||
| background-color: #fff; | ||
| border: 1px solid #e5e7eb; | ||
| border-radius: 6px; | ||
| `, | ||
|
|
||
| errorDetailsSummary: css` | ||
| padding: 0.75rem 1rem; | ||
| background-color: #f9fafb; | ||
| border-bottom: 1px solid #e5e7eb; | ||
| cursor: pointer; | ||
| font-weight: 500; | ||
| color: #374151; | ||
| &:hover { | ||
| background-color: #f3f4f6; | ||
| } | ||
| `, | ||
|
|
||
| errorDetailsContent: css` | ||
| padding: 1rem; | ||
| font-family: 'Monaco', 'Menlo', 'Ubuntu Mono', monospace; | ||
| font-size: 0.875rem; | ||
| line-height: 1.4; | ||
| color: #374151; | ||
| background-color: #fff; | ||
| overflow-x: auto; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| margin: 0; | ||
| `, | ||
|
|
||
| errorActions: css` | ||
| display: flex; | ||
| gap: 0.75rem; | ||
| margin: 1.5rem 0 1rem 0; | ||
| flex-wrap: wrap; | ||
| justify-content: center; | ||
| `, | ||
|
|
||
| button: css` | ||
| padding: 0.5rem 1rem; | ||
| border-radius: 6px; | ||
| font-size: 0.875rem; | ||
| font-weight: 500; | ||
| cursor: pointer; | ||
| transition: all 0.2s ease-in-out; | ||
| border: 1px solid transparent; | ||
| &:focus { | ||
| outline: 2px solid transparent; | ||
| outline-offset: 2px; | ||
| box-shadow: 0 0 0 2px rgba(59, 130, 246, 0.5); | ||
| } | ||
| &:disabled { | ||
| opacity: 0.5; | ||
| cursor: not-allowed; | ||
| } | ||
| `, | ||
|
|
||
| primaryButton: css` | ||
| background-color: #dc2626; | ||
| color: white; | ||
| border-color: #dc2626; | ||
| &:hover:not(:disabled) { | ||
| background-color: #b91c1c; | ||
| border-color: #b91c1c; | ||
| } | ||
| &:active:not(:disabled) { | ||
| background-color: #991b1b; | ||
| border-color: #991b1b; | ||
| } | ||
| `, | ||
|
|
||
| secondaryButton: css` | ||
| background-color: #fff; | ||
| color: #374151; | ||
| border-color: #d1d5db; | ||
| &:hover:not(:disabled) { | ||
| background-color: #f9fafb; | ||
| border-color: #9ca3af; | ||
| } | ||
| &:active:not(:disabled) { | ||
| background-color: #f3f4f6; | ||
| border-color: #6b7280; | ||
| } | ||
| `, | ||
|
|
||
| tertiaryButton: css` | ||
| background-color: transparent; | ||
| color: #6b7280; | ||
| border-color: transparent; | ||
| &:hover:not(:disabled) { | ||
| color: #374151; | ||
| background-color: #f9fafb; | ||
| } | ||
| &:active:not(:disabled) { | ||
| color: #111827; | ||
| background-color: #f3f4f6; | ||
| } | ||
| `, | ||
|
|
||
| errorHelp: css` | ||
| margin-top: 1rem; | ||
| padding-top: 1rem; | ||
| border-top: 1px solid #fecaca; | ||
| `, | ||
|
|
||
| errorHelpText: css` | ||
| color: #7f1d1d; | ||
| font-size: 0.875rem; | ||
| margin: 0; | ||
| font-style: italic; | ||
| `, |
There was a problem hiding this comment.
Hard-coded color values throughout styles object
This file contains numerous hard-coded hex color values (e.g., #fef2f2, #dc2626, #7f1d1d) that should be replaced with design system tokens or CSS custom properties for better maintainability and theming support.
Suggested change
| background-color: #fef2f2; | |
| border: 1px solid #fecaca; | |
| border-radius: 8px; | |
| margin: 1rem 0; | |
| min-height: 200px; | |
| text-align: center; | |
| `, | |
| errorIcon: css` | |
| font-size: 3rem; | |
| margin-bottom: 1rem; | |
| `, | |
| errorTitle: css` | |
| color: #dc2626; | |
| font-size: 1.5rem; | |
| font-weight: 600; | |
| margin: 0 0 1rem 0; | |
| `, | |
| errorMessage: css` | |
| color: #7f1d1d; | |
| font-size: 1rem; | |
| line-height: 1.5; | |
| margin: 0 0 1.5rem 0; | |
| max-width: 500px; | |
| `, | |
| errorDetails: css` | |
| margin: 1rem 0; | |
| text-align: left; | |
| width: 100%; | |
| max-width: 600px; | |
| background-color: #fff; | |
| border: 1px solid #e5e7eb; | |
| border-radius: 6px; | |
| `, | |
| errorDetailsSummary: css` | |
| padding: 0.75rem 1rem; | |
| background-color: #f9fafb; | |
| border-bottom: 1px solid #e5e7eb; | |
| cursor: pointer; | |
| font-weight: 500; | |
| color: #374151; | |
| &:hover { | |
| background-color: #f3f4f6; | |
| } | |
| `, | |
| errorDetailsContent: css` | |
| padding: 1rem; | |
| font-family: 'Monaco', 'Menlo', 'Ubuntu Mono', monospace; | |
| font-size: 0.875rem; | |
| line-height: 1.4; | |
| color: #374151; | |
| background-color: #fff; | |
| overflow-x: auto; | |
| white-space: pre-wrap; | |
| word-break: break-word; | |
| margin: 0; | |
| `, | |
| errorActions: css` | |
| display: flex; | |
| gap: 0.75rem; | |
| margin: 1.5rem 0 1rem 0; | |
| flex-wrap: wrap; | |
| justify-content: center; | |
| `, | |
| button: css` | |
| padding: 0.5rem 1rem; | |
| border-radius: 6px; | |
| font-size: 0.875rem; | |
| font-weight: 500; | |
| cursor: pointer; | |
| transition: all 0.2s ease-in-out; | |
| border: 1px solid transparent; | |
| &:focus { | |
| outline: 2px solid transparent; | |
| outline-offset: 2px; | |
| box-shadow: 0 0 0 2px rgba(59, 130, 246, 0.5); | |
| } | |
| &:disabled { | |
| opacity: 0.5; | |
| cursor: not-allowed; | |
| } | |
| `, | |
| primaryButton: css` | |
| background-color: #dc2626; | |
| color: white; | |
| border-color: #dc2626; | |
| &:hover:not(:disabled) { | |
| background-color: #b91c1c; | |
| border-color: #b91c1c; | |
| } | |
| &:active:not(:disabled) { | |
| background-color: #991b1b; | |
| border-color: #991b1b; | |
| } | |
| `, | |
| secondaryButton: css` | |
| background-color: #fff; | |
| color: #374151; | |
| border-color: #d1d5db; | |
| &:hover:not(:disabled) { | |
| background-color: #f9fafb; | |
| border-color: #9ca3af; | |
| } | |
| &:active:not(:disabled) { | |
| background-color: #f3f4f6; | |
| border-color: #6b7280; | |
| } | |
| `, | |
| tertiaryButton: css` | |
| background-color: transparent; | |
| color: #6b7280; | |
| border-color: transparent; | |
| &:hover:not(:disabled) { | |
| color: #374151; | |
| background-color: #f9fafb; | |
| } | |
| &:active:not(:disabled) { | |
| color: #111827; | |
| background-color: #f3f4f6; | |
| } | |
| `, | |
| errorHelp: css` | |
| margin-top: 1rem; | |
| padding-top: 1rem; | |
| border-top: 1px solid #fecaca; | |
| `, | |
| errorHelpText: css` | |
| color: #7f1d1d; | |
| font-size: 0.875rem; | |
| margin: 0; | |
| font-style: italic; | |
| `, | |
| // Consider using design tokens like: | |
| background-color: var(--color-error-bg); | |
| color: var(--color-error-text); | |
| border-color: var(--color-error-border); |
| customValidator: (values: string[]) => { | ||
| // Custom validation: warn about performance with many regions | ||
| if (values.length > 8) { | ||
| return 'Selecting many regions may impact query performance'; |
There was a problem hiding this comment.
High: Hard-coded magic number 8 for performance warning threshold should be configurable. This business logic constraint may need to vary based on system capabilities or user requirements.
Suggested change
| if (values.length > 8) { | |
| if (values.length > (settings.regionPerformanceThreshold || 8)) { |
| // Enhanced validation rules for status filter | ||
| const filterValidationRules = useMemo( | ||
| () => ({ | ||
| maxSelections: 10, // Allow up to 10 status selections |
There was a problem hiding this comment.
Critical: Hard-coded magic number 10 for maxSelections should be configurable or defined as a named constant. This creates maintenance issues and inconsistency across the codebase.
Suggested change
| maxSelections: 10, // Allow up to 10 status selections | |
| const MAX_STATUS_SELECTIONS = 10; | |
| // ... | |
| maxSelections: MAX_STATUS_SELECTIONS, // Allow up to 10 status selections |
| customValidator: (values: string[]) => { | ||
| // Custom validation: warn if too many statuses selected | ||
| if (values.length > 5) { | ||
| return 'Selecting many statuses may slow down the query'; |
There was a problem hiding this comment.
High: Hard-coded magic number 5 creates inconsistency with the maxSelections value of 10. This could confuse users who can select up to 10 items but get a warning at 5.
Suggested change
| if (values.length > 5) { | |
| const WARNING_THRESHOLD = 5; | |
| // ... | |
| if (values.length > WARNING_THRESHOLD) { |
| describe('Settings Persistence', () => { | ||
| it('should initialize settings if they do not exist', async () => { | ||
| // Mock empty settings | ||
| mockAdminSettings.nodes = undefined; |
There was a problem hiding this comment.
Critical Issue: Mutating mock object directly
Directly mutating mockAdminSettings.nodes = undefined can cause test pollution and unpredictable behavior in other tests. This mutation persists across test runs and can lead to flaky tests.
Suggested change
| mockAdminSettings.nodes = undefined; | |
| // Mock empty settings using vi.mocked or create a new mock | |
| const mockUseRecoilValue = vi.mocked(useRecoilValue); | |
| mockUseRecoilValue.mockReturnValueOnce({ nodes: undefined }); |
| it('should handle state conflicts gracefully', async () => { | ||
| // Set conflicting URL and settings | ||
| mockRouter.query = { name: 'nodes', pageSize: '48' }; | ||
| mockAdminSettings.nodes.pageSize = 96; |
There was a problem hiding this comment.
High Risk: Direct mutation of shared mock object
Similar to line 372, directly mutating mockAdminSettings.nodes.pageSize = 96 can cause test interference. This pattern appears multiple times in the test file and should be addressed consistently.
Suggested change
| mockAdminSettings.nodes.pageSize = 96; | |
| // Create isolated mock for this test | |
| const mockUseRecoilValue = vi.mocked(useRecoilValue); | |
| mockUseRecoilValue.mockReturnValueOnce({ | |
| nodes: { ...mockAdminSettings.nodes, pageSize: 96 } | |
| }); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.