feat: group pagination #92
Conversation
WalkthroughThe changes implement pagination in the Changes
Assessment against linked issues
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 59.37% 59.42% +0.04%
==========================================
Files 145 146 +1
Lines 14100 14187 +87
==========================================
+ Hits 8372 8430 +58
- Misses 5728 5757 +29 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/groups/components/myGroups.tsx (1)
Line range hint
34-58: Consider performance and flexibility improvementsThe pagination implementation is correct, but consider these enhancements:
- Memoize
totalPagesandpaginatedGroupscalculations to prevent unnecessary recalculations on re-renders- Make
pageSizeconfigurable via props to improve component reusability+ const pageSize = props.pageSize ?? 8; + const totalPages = useMemo(() => + Math.ceil(filteredGroups.length / pageSize), + [filteredGroups.length, pageSize] + ); + const paginatedGroups = useMemo(() => + filteredGroups.slice((currentPage - 1) * pageSize, currentPage * pageSize), + [filteredGroups, currentPage, pageSize] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/groups/components/myGroups.tsx(6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/groups/components/myGroups.tsx
[warning] 234-235: components/groups/components/myGroups.tsx#L234-L235
Added lines #L234 - L235 were not covered by tests
[warning] 244-266: components/groups/components/myGroups.tsx#L244-L266
Added lines #L244 - L266 were not covered by tests
[warning] 271-272: components/groups/components/myGroups.tsx#L271-L272
Added lines #L271 - L272 were not covered by tests
🔇 Additional comments (3)
components/groups/components/myGroups.tsx (3)
Line range hint 174-214: LGTM! Loading skeleton matches page size
The skeleton rows count has been correctly updated to match the pagination size of 8, maintaining consistent UI during loading states.
Line range hint 214-227: LGTM! Pagination correctly applied to group rendering
The switch to paginatedGroups is implemented correctly, maintaining all existing functionality while adding pagination support.
34-35: Handle pagination edge cases
Consider adding logic to handle edge cases when the total number of items changes:
- Reset to page 1 when search term changes
- Adjust current page when the last page becomes empty due to filtering
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
hooks/useIsMobile.ts (2)
12-14: Consider debouncing the resize handler for better performance.The current implementation might trigger frequent rerenders during resize events. Consider adding debouncing to optimize performance.
import { useState, useEffect } from 'react'; +import debounce from 'lodash/debounce'; const useIsMobile = (breakpoint: number = 1024) => { const [isMobile, setIsMobile] = useState(false); useEffect(() => { if (typeof window === 'undefined') { return; } - const checkIsMobile = () => { + const checkIsMobile = debounce(() => { setIsMobile(window.innerWidth <= breakpoint); - }; + }, 150);
26-29: LGTM! Consider extending for broader device detection.The hook effectively serves its current purpose. For future scalability, consider extending it to support multiple breakpoints or device types (e.g., tablet, desktop) if needed by other components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/groups/components/myGroups.tsx(7 hunks)hooks/index.tsx(1 hunks)hooks/useIsMobile.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/groups/components/myGroups.tsx
[warning] 238-239: components/groups/components/myGroups.tsx#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 248-270: components/groups/components/myGroups.tsx#L248-L270
Added lines #L248 - L270 were not covered by tests
[warning] 275-276: components/groups/components/myGroups.tsx#L275-L276
Added lines #L275 - L276 were not covered by tests
hooks/useIsMobile.ts
[warning] 9-9: hooks/useIsMobile.ts#L9
Added line #L9 was not covered by tests
🔇 Additional comments (9)
hooks/index.tsx (2)
8-8: LGTM! Export syntax is correct.
The named export with default import syntax is the appropriate pattern for re-exporting the default export from useIsMobile.
8-8: Consider extracting the breakpoint value to a shared constant.
The useIsMobile hook uses a hardcoded breakpoint of 1024px. To maintain consistency across the application, consider extracting this value to a shared constants file.
Let's check if there are other breakpoint values in use:
✅ Verification successful
Let me analyze the search results to verify if there are other breakpoint values in use.
Let me check if there are any other files using this hook or defining breakpoint values.
Let me check the contents of the constants file and theme-related files to see if there are any existing breakpoint definitions.
Consider extracting the breakpoint value to a shared constant in the Tailwind configuration.
The useIsMobile hook uses a default breakpoint of 1024px, which aligns with Tailwind's default lg breakpoint. Since the project already uses Tailwind CSS with a custom configuration (tailwind.config.js) that defines breakpoints under the screens property, it would be more maintainable to:
- Add the mobile breakpoint to the Tailwind screens configuration
- Import and use this value in the
useIsMobilehook
This ensures consistency with the project's responsive design system defined in Tailwind.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for pixel-based breakpoints in the codebase
# Expected: Find other breakpoint definitions to ensure consistency
# Look for pixel values that might be breakpoints
rg -g '*.{ts,tsx,css,scss}' '\d+px'
# Look for potential breakpoint-related constants
rg -g '*.{ts,tsx}' '(breakpoint|BREAKPOINT|screen|SCREEN|viewport|VIEWPORT)'
Length of output: 87085
Script:
# Let's check the useIsMobile hook implementation
cat hooks/useIsMobile.ts
Length of output: 665
Script:
# Search for useIsMobile hook usage
rg "useIsMobile"
# Search for any breakpoint-related constants or configuration files
fd -e ts -e tsx -e js -e jsx "breakpoint|config|constant|theme"
Length of output: 532
Script:
# Check contents of potential files containing breakpoint definitions
cat utils/constants.ts
cat tailwind.config.js
cat contexts/theme.tsx
Length of output: 9124
hooks/useIsMobile.ts (3)
1-3: LGTM! Clean imports and well-structured hook signature.
The hook follows React naming conventions and provides a sensible default breakpoint value.
16-24: LGTM! Proper event listener management.
The implementation follows React best practices with:
- Proper event listener cleanup
- Correct dependencies array
- Initial state setup
4-11: Add test coverage for SSR compatibility check.
While the SSR check is correctly implemented, it lacks test coverage. Consider adding tests for both server and client-side scenarios.
Here's a script to verify other SSR checks in the codebase:
Would you like me to help generate test cases for this hook? The tests should cover:
- Server-side rendering scenario
- Client-side initialization
- Breakpoint changes
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-9: hooks/useIsMobile.ts#L9
Added line #L9 was not covered by tests
components/groups/components/myGroups.tsx (4)
Line range hint 23-39: LGTM! Clean implementation of pagination state management
The pagination state setup is well-structured with proper mobile responsiveness consideration.
Line range hint 178-218: LGTM! Consistent loading state with page size
The loading skeleton count now matches the pagination size, providing a consistent user experience during loading.
345-346: LGTM! Clean code improvement
57-61: Verify pagination with filtered results
The pagination implementation looks correct, but let's verify its integration with the search functionality.
✅ Verification successful
Pagination and filtering implementation verified successfully
The implementation correctly handles both filtering and pagination:
- The filtering is applied first using
filteredGroupsarray, which filters based on group title - Pagination is then correctly applied to the filtered results using
sliceon the filtered array - The test file confirms proper functionality with search and group selection tests
- The pagination controls are properly implemented with accessibility considerations
- The component maintains the correct order of operations: filter first, then paginate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the component handles both filtering and pagination correctly
# Look for any direct manipulation of filteredGroups array that might affect pagination
# Search for filteredGroups usage
rg "filteredGroups" --type tsx -C 3
# Check for any existing tests related to filtering
rg -t test "filteredGroups|searchTerm|setSearchTerm"
Length of output: 147
Script:
#!/bin/bash
# Let's try with more generic file extensions and patterns
# Search for filteredGroups usage in all possible React/TypeScript files
rg "filteredGroups" -t ts -t tsx -t jsx -t js -C 3
# Look for search/filter related logic
rg "setSearchTerm|searchTerm|filterGroups" -t ts -t tsx -t jsx -t js -C 3
# Find test files related to groups
fd ".*\.(test|spec)\.(ts|tsx|js|jsx)$" | grep -i "group"
Length of output: 1277
Script:
#!/bin/bash
# Let's examine the myGroups test file and component implementation
# Check myGroups component implementation
cat components/groups/components/myGroups.tsx
# Check myGroups test file
cat components/groups/components/__tests__/myGroups.test.tsx
Length of output: 21878
| {totalPages > 1 && ( | ||
| <div | ||
| className="flex items-center justify-center gap-2" | ||
| onClick={e => e.stopPropagation()} | ||
| > | ||
| <button | ||
| onClick={e => { | ||
| e.stopPropagation(); | ||
| setCurrentPage(prev => Math.max(1, prev - 1)); | ||
| }} | ||
| disabled={currentPage === 1 || isLoading} | ||
| className="p-2 hover:bg-[#FFFFFF1A] rounded-lg transition-colors disabled:opacity-50 disabled:cursor-not-allowed" | ||
| > | ||
| ‹ | ||
| </button> | ||
|
|
||
| {[...Array(totalPages)].map((_, index) => { | ||
| const pageNum = index + 1; | ||
| if ( | ||
| pageNum === 1 || | ||
| pageNum === totalPages || | ||
| (pageNum >= currentPage - 1 && pageNum <= currentPage + 1) | ||
| ) { | ||
| return ( | ||
| <button | ||
| key={pageNum} | ||
| onClick={e => { | ||
| e.stopPropagation(); | ||
| setCurrentPage(pageNum); | ||
| }} | ||
| className={`w-8 h-8 flex items-center justify-center rounded-lg transition-colors | ||
| ${currentPage === pageNum ? 'bg-[#FFFFFF1A] text-white' : 'hover:bg-[#FFFFFF1A]'}`} | ||
| > | ||
| {pageNum} | ||
| </button> | ||
| ); | ||
| } else if (pageNum === currentPage - 2 || pageNum === currentPage + 2) { | ||
| return <span key={pageNum}>...</span>; | ||
| } | ||
| return null; | ||
| })} | ||
|
|
||
| <button | ||
| onClick={e => { | ||
| e.stopPropagation(); | ||
| setCurrentPage(prev => Math.min(totalPages, prev + 1)); | ||
| }} | ||
| disabled={currentPage === totalPages || isLoading} | ||
| className="p-2 hover:bg-[#FFFFFF1A] rounded-lg transition-colors disabled:opacity-50 disabled:cursor-not-allowed" | ||
| > | ||
| › | ||
| </button> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Add test coverage for pagination logic
Please add test coverage for:
- Page navigation (prev/next buttons)
- Page number selection
- Ellipsis rendering logic
- Disabled states handling
Would you like me to help generate comprehensive test cases for the pagination functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 238-239: components/groups/components/myGroups.tsx#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 248-270: components/groups/components/myGroups.tsx#L248-L270
Added lines #L248 - L270 were not covered by tests
[warning] 275-276: components/groups/components/myGroups.tsx#L275-L276
Added lines #L275 - L276 were not covered by tests
🛠️ Refactor suggestion
Enhance accessibility of pagination controls
While the pagination implementation is solid, it needs accessibility improvements:
<div
className="flex items-center justify-center gap-2"
+ role="navigation"
+ aria-label="Group list pagination"
onClick={e => e.stopPropagation()}
>
<button
onClick={e => {
e.stopPropagation();
setCurrentPage(prev => Math.max(1, prev - 1));
}}
disabled={currentPage === 1 || isLoading}
className="p-2 hover:bg-[#FFFFFF1A] rounded-lg transition-colors disabled:opacity-50 disabled:cursor-not-allowed"
+ aria-label="Go to previous page"
>
‹
</button>
{/* ... */}
<button
onClick={e => {
e.stopPropagation();
setCurrentPage(prev => Math.min(totalPages, prev + 1));
}}
disabled={currentPage === totalPages || isLoading}
className="p-2 hover:bg-[#FFFFFF1A] rounded-lg transition-colors disabled:opacity-50 disabled:cursor-not-allowed"
+ aria-label="Go to next page"
>
›
</button>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 238-239: components/groups/components/myGroups.tsx#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 248-270: components/groups/components/myGroups.tsx#L248-L270
Added lines #L248 - L270 were not covered by tests
[warning] 275-276: components/groups/components/myGroups.tsx#L275-L276
Added lines #L275 - L276 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
components/groups/components/__tests__/myGroups.test.tsx (2)
51-66: Consider improving mock data setup for better maintainability.The mock data setup could be enhanced in the following ways:
-const mockPropsWithManyGroups = { +// Generate test data for pagination (8 per page + 4 overflow to test multiple pages) +const GROUPS_PER_PAGE = 8; +const TOTAL_TEST_GROUPS = GROUPS_PER_PAGE + 4; +const mockPropsWithManyGroups = { groups: { - groups: Array(12) - .fill(null) - .map((_, index) => ({ + groups: Array.from({ length: TOTAL_TEST_GROUPS }, (_, index) => ({ id: `${index + 1}`, ipfsMetadata: { title: `Group ${index + 1}` }, policies: [{ address: `policy${index + 1}`, decision_policy: { threshold: '1' } }], admin: `admin${index + 1}`, members: [{ member: { address: `member${index + 1}` } }], total_weight: '1', })), }, - proposals: {}, + proposals: Object.fromEntries( + Array.from({ length: TOTAL_TEST_GROUPS }, (_, i) => [`policy${i + 1}`, []]) + ), isLoading: false, };
142-142: Replace magic number with a constant.The page size (8) should be defined as a constant to maintain consistency with the component implementation.
-const totalPages = Math.ceil(mockPropsWithManyGroups.groups.groups.length / 8); +const DESKTOP_PAGE_SIZE = 8; +const totalPages = Math.ceil(mockPropsWithManyGroups.groups.groups.length / DESKTOP_PAGE_SIZE);components/groups/components/myGroups.tsx (2)
38-38: Consider defining pageSize as a constantMove the pageSize definition outside the component to improve maintainability and reusability.
+const DESKTOP_PAGE_SIZE = 8; +const MOBILE_PAGE_SIZE = 6; export function YourGroups({ // ... }) { - const pageSize = isMobile ? 6 : 8; + const pageSize = isMobile ? MOBILE_PAGE_SIZE : DESKTOP_PAGE_SIZE;
239-293: Add keyboard navigation support for paginationEnhance user experience by adding keyboard navigation support for the pagination controls.
<div className="flex items-center justify-center gap-2" onClick={e => e.stopPropagation()} role="navigation" aria-label="Pagination" + onKeyDown={e => { + if (e.key === 'ArrowLeft' && currentPage > 1) { + setCurrentPage(prev => Math.max(1, prev - 1)); + } else if (e.key === 'ArrowRight' && currentPage < totalPages) { + setCurrentPage(prev => Math.min(totalPages, prev + 1)); + } + }} + tabIndex={0} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/groups/components/__tests__/myGroups.test.tsx(2 hunks)components/groups/components/myGroups.tsx(7 hunks)
🔇 Additional comments (1)
components/groups/components/myGroups.tsx (1)
Line range hint 1-356: Implementation looks good with suggested improvements
The pagination implementation is well-structured and includes proper accessibility attributes. Consider implementing the suggested improvements for better maintainability and user experience.
| describe('Pagination', () => { | ||
| test('renders pagination controls when there are more items than page size', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| expect(screen.getByRole('navigation', { name: /pagination/i })).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: /next page/i })).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: /previous page/i })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('pagination controls navigate between pages correctly', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| // Should start with page 1 | ||
| expect(screen.getByRole('button', { name: 'Page 1', current: 'page' })).toBeInTheDocument(); | ||
|
|
||
| // Click next page | ||
| fireEvent.click(screen.getByRole('button', { name: /next page/i })); | ||
| expect(screen.getByRole('button', { name: 'Page 2', current: 'page' })).toBeInTheDocument(); | ||
|
|
||
| // Click previous page | ||
| fireEvent.click(screen.getByRole('button', { name: /previous page/i })); | ||
| expect(screen.getByRole('button', { name: 'Page 1', current: 'page' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('previous button is disabled on first page', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| const prevButton = screen.getByRole('button', { name: /previous page/i }); | ||
| expect(prevButton).toBeDisabled(); | ||
| }); | ||
|
|
||
| test('next button is disabled on last page', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| // Navigate to last page | ||
| const totalPages = Math.ceil(mockPropsWithManyGroups.groups.groups.length / 8); | ||
| for (let i = 1; i < totalPages; i++) { | ||
| fireEvent.click(screen.getByRole('button', { name: /next page/i })); | ||
| } | ||
|
|
||
| const nextButton = screen.getByRole('button', { name: /next page/i }); | ||
| expect(nextButton).toBeDisabled(); | ||
| }); | ||
|
|
||
| test('direct page selection works correctly', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| // Click page 2 button | ||
| fireEvent.click(screen.getByRole('button', { name: 'Page 2' })); | ||
| expect(screen.getByRole('button', { name: 'Page 2', current: 'page' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('shows correct number of items per page', () => { | ||
| renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />); | ||
|
|
||
| // On desktop (non-mobile), should show 8 items per page | ||
| const groupRows = screen.getAllByRole('button', { name: /Select Group \d+ group/i }); | ||
| expect(groupRows).toHaveLength(8); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for important edge cases.
While the current test suite covers basic pagination functionality well, consider adding the following test cases:
- Test filtering with pagination:
test('maintains correct pagination state when filtering results', () => {
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Navigate to second page
fireEvent.click(screen.getByRole('button', { name: /next page/i }));
// Apply filter that matches items on second page
const searchInput = screen.getByPlaceholderText('Search for a group...');
fireEvent.change(searchInput, { target: { value: 'Group 9' } });
// Verify we see the filtered results
expect(screen.getAllByRole('button', { name: /Select Group 9 group/i })).toHaveLength(1);
});- Test empty state after filtering:
test('handles empty pages gracefully when filtering', () => {
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Apply filter that matches no items
const searchInput = screen.getByPlaceholderText('Search for a group...');
fireEvent.change(searchInput, { target: { value: 'nonexistent' } });
// Verify pagination is hidden and empty state is shown
expect(screen.queryByRole('navigation', { name: /pagination/i })).not.toBeInTheDocument();
expect(screen.getByText(/No groups found/i)).toBeInTheDocument();
});- Test mobile view pagination:
test('adjusts items per page for mobile view', () => {
// Mock useIsMobile to return true
mock.module('@/hooks', () => ({
useIsMobile: () => true
}));
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Verify we see 6 items instead of 8 on mobile
const groupRows = screen.getAllByRole('button', { name: /Select Group \d+ group/i });
expect(groupRows).toHaveLength(6);
});| @@ -163,7 +175,7 @@ | |||
| </thead> | |||
| <tbody className="space-y-4" role="rowgroup"> | |||
| {isLoading | |||
| ? Array(10) | |||
| ? Array(8) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pageSize for skeleton row count
The number of skeleton rows should match the pageSize variable to maintain consistency between loading and loaded states.
- ? Array(8)
+ ? Array(pageSize)📝 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.
| ? Array(8) | |
| ? Array(pageSize) |
| @@ -31,6 +32,11 @@ | |||
| isLoading: boolean; | |||
| }) { | |||
| const [searchTerm, setSearchTerm] = useState(''); | |||
| const [currentPage, setCurrentPage] = useState(1); | |||
There was a problem hiding this comment.
Reset currentPage when search term changes
The currentPage should be reset to 1 when the search term changes to avoid showing empty pages.
Add a useEffect hook to handle this:
+useEffect(() => {
+ setCurrentPage(1);
+}, [searchTerm]);Committable suggestion skipped: line range outside the PR's diff.
| const totalPages = Math.ceil(filteredGroups.length / pageSize); | ||
| const paginatedGroups = filteredGroups.slice( | ||
| (currentPage - 1) * pageSize, | ||
| currentPage * pageSize | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize pagination calculations with useMemo
The paginatedGroups calculation should be memoized to prevent unnecessary recalculations on every render.
- const totalPages = Math.ceil(filteredGroups.length / pageSize);
- const paginatedGroups = filteredGroups.slice(
- (currentPage - 1) * pageSize,
- currentPage * pageSize
- );
+ const { totalPages, paginatedGroups } = useMemo(() => {
+ const total = Math.max(1, Math.ceil(filteredGroups.length / pageSize));
+ const paginated = filteredGroups.slice(
+ (currentPage - 1) * pageSize,
+ currentPage * pageSize
+ );
+ return { totalPages: total, paginatedGroups: paginated };
+ }, [filteredGroups, currentPage, pageSize]);📝 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 totalPages = Math.ceil(filteredGroups.length / pageSize); | |
| const paginatedGroups = filteredGroups.slice( | |
| (currentPage - 1) * pageSize, | |
| currentPage * pageSize | |
| ); | |
| const { totalPages, paginatedGroups } = useMemo(() => { | |
| const total = Math.max(1, Math.ceil(filteredGroups.length / pageSize)); | |
| const paginated = filteredGroups.slice( | |
| (currentPage - 1) * pageSize, | |
| currentPage * pageSize | |
| ); | |
| return { totalPages: total, paginatedGroups: paginated }; | |
| }, [filteredGroups, currentPage, pageSize]); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
components/groups/components/__tests__/myGroups.test.tsx (1)
56-71: Enhance mock data to include proposals.The mock data is well-structured for pagination testing, but the empty proposals object might miss testing scenarios where proposals affect the group display or filtering.
Consider enhancing the mock data:
const mockPropsWithManyGroups = { groups: { groups: Array(12).fill(null).map((_, index) => ({ id: `${index + 1}`, ipfsMetadata: { title: `Group ${index + 1}` }, policies: [{ address: `policy${index + 1}`, decision_policy: { threshold: '1' } }], admin: `admin${index + 1}`, members: [{ member: { address: `member${index + 1}` } }], total_weight: '1', })), }, proposals: { // Add some proposals for testing proposal-related scenarios policy1: [{ id: '1', title: 'Proposal 1' }], policy2: [{ id: '2', title: 'Proposal 2' }], }, isLoading: false, };components/groups/components/myGroups.tsx (2)
194-196: Simplify the proposals null checkThe nested conditions can be simplified using optional chaining.
-group.policies && -group.policies.length > 0 && -proposals[group.policies[0].address] +proposals[group.policies?.[0]?.address]
328-329: Remove redundant commentThe comment "Add pagination controls" can be removed as the pagination controls are already implemented above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/groups/components/__tests__/myGroups.test.tsx(3 hunks)components/groups/components/myGroups.tsx(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/groups/components/myGroups.tsx
[warning] 246-248: components/groups/components/myGroups.tsx#L246-L248
Added lines #L246 - L248 were not covered by tests
[warning] 250-250: components/groups/components/myGroups.tsx#L250
Added line #L250 was not covered by tests
[warning] 252-253: components/groups/components/myGroups.tsx#L252-L253
Added lines #L252 - L253 were not covered by tests
🔇 Additional comments (6)
components/groups/components/__tests__/myGroups.test.tsx (2)
34-37: 🛠️ Refactor suggestion
Add test coverage for mobile view pagination.
While the mobile detection mock is set up correctly, there's no test case that verifies the mobile view behavior. Consider adding a test case that checks the mobile-specific pagination behavior.
Add a test case for mobile view:
test('adjusts items per page for mobile view', () => {
const useIsMobile = require('@/hooks/useIsMobile').default;
useIsMobile.mockReturnValue(true);
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Verify mobile-specific behavior
const groupRows = screen.getAllByRole('button', { name: /Select Group \d+ group/i });
expect(groupRows).toHaveLength(6); // Assuming mobile shows 6 items per page
});112-171: 🛠️ Refactor suggestion
Add test coverage for pagination edge cases.
While the current test suite covers basic pagination functionality well, it's missing important edge cases around filtering and empty states.
Add these test cases:
test('maintains correct pagination state when filtering results', () => {
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Navigate to second page
fireEvent.click(screen.getByRole('button', { name: /next page/i }));
// Apply filter that matches items on second page
const searchInput = screen.getByPlaceholderText('Search for a group...');
fireEvent.change(searchInput, { target: { value: 'Group 9' } });
// Verify we see the filtered results
expect(screen.getAllByRole('button', { name: /Select Group 9 group/i })).toHaveLength(1);
});
test('handles empty pages gracefully when filtering', () => {
renderWithChainProvider(<YourGroups {...mockPropsWithManyGroups} />);
// Apply filter that matches no items
const searchInput = screen.getByPlaceholderText('Search for a group...');
fireEvent.change(searchInput, { target: { value: 'nonexistent' } });
// Verify pagination is hidden and empty state is shown
expect(screen.queryByRole('navigation', { name: /pagination/i })).not.toBeInTheDocument();
expect(screen.getByText(/No groups found/i)).toBeInTheDocument();
});components/groups/components/myGroups.tsx (4)
35-38: Reset currentPage when searchTerm changes
The currentPage should be reset to 1 when the search term changes to avoid showing empty pages.
53-57: Optimize pagination calculations with useMemo
The paginatedGroups calculation should be memoized to prevent unnecessary recalculations on every render.
149-149: Use pageSize for skeleton row count
The number of skeleton rows should match the pageSize variable to maintain consistency between loading and loaded states.
205-268: Add test coverage for pagination edge cases
Please add test coverage for:
- Ellipsis rendering logic (when currentPage is near start/end)
- Page number visibility conditions
- Edge cases when totalPages is small
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 246-248: components/groups/components/myGroups.tsx#L246-L248
Added lines #L246 - L248 were not covered by tests
[warning] 250-250: components/groups/components/myGroups.tsx#L250
Added line #L250 was not covered by tests
[warning] 252-253: components/groups/components/myGroups.tsx#L252-L253
Added lines #L252 - L253 were not covered by tests
partially fixes #15
Summary by CodeRabbit
useIsMobilehook to determine if the user is on a mobile device, enhancing responsive design.