Feat/pagination to teams 554 #572
Conversation
WalkthroughThis pull request implements pagination support for team management requests in both the backend and frontend. In the backend, the team controller and service now accept page and limit parameters to fetch a subset of users and return pagination metadata. On the frontend, the TeamTab component is updated to track pagination state, and a new PageBtnContainer component is introduced for navigation. Additionally, the Button component now supports optional start and end icons, and the organization details service has been updated to accept pagination parameters in its API call. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as TeamController
participant TS as TeamService
participant DB as Database (User Model)
U->>C: GET /team/details?page=X&limit=Y
C->>TS: getTeam(X, Y)
TS->>DB: findAndCountAll(limit, offset)
DB-->>TS: Results (count & team data)
TS-->>C: Team data with pagination metadata
C-->>U: JSON response with team data and metadata
sequenceDiagram
participant U as User
participant TT as TeamTab Component
participant SS as getOrgDetails API Service
participant PC as PageBtnContainer Component
U->>TT: Load team page (initial page 1)
TT->>SS: getOrgDetails(page=1, limit=10)
SS-->>TT: Response with team data, totalPages, currentPage
TT->>PC: Render pagination controls
U->>PC: Click page button to change page
PC->>TT: Update currentPage state
TT->>SS: getOrgDetails(newPage, limit)
SS-->>TT: Updated team data response
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (14)
frontend/src/components/PageBtnContainer/PageBtnContainer.css (2)
1-9: Consider scoping the section selector to avoid potential conflicts.This selector targets all section elements globally, which might cause conflicts with other components using section tags. Consider scoping it to a specific class.
-section { +.pagination-container { height: 4rem; margin-top: 0.5rem; display: flex; align-items: center; justify-content: center; flex-wrap: wrap; gap: 1rem; }
11-14: Use CSS variables for consistent color management.The purple color is hardcoded. It would be better to use CSS variables for consistent theming across the application.
+:root { + --primary-purple: #7f56d9; +} .ellipsis { - color: #7f56d9; + color: var(--primary-purple); cursor: default; }frontend/src/services/settingServices.js (1)
25-27: Consider adding documentation for pagination parameters.Even though the code is clean, adding JSDoc comments would help other developers understand the pagination parameters quickly.
/** * Fetches organization details with pagination support * @param {number} page - The page number (starts at 1) * @param {number} limit - Number of items per page * @returns {Promise<Object>} The API response */ export const getOrgDetails = async (page = 1, limit = 5) => {backend/src/service/team.service.js (3)
37-40: Add validation for pagination parameters.The implementation looks good, but there's no validation for negative values of page or limit which could cause issues.
async getTeam(page = 1, limit = 10) { try { + // Ensure positive values for pagination + page = Math.max(1, page); + limit = Math.max(1, limit); const offset = (page - 1) * limit;
43-46: Remove commented code and standardize error handling.There's commented out code that should be removed. Also, the approach of returning null is inconsistent with other methods that throw errors.
if (!team) { - //throw new Error('No team found'); - return null; + throw new Error('No team found'); }Alternatively, if returning null is preferred:
if (!team) { - //throw new Error('No team found'); return null; }
62-63: Enhance error logging.While logging the error is good, consider adding more context to make debugging easier.
catch (err) { - console.log(err); + console.error('Error retrieving team:', err); throw new Error("Failed to retrieve team"); }frontend/src/components/Button/Button.jsx (1)
34-34: Consider handling icon-only buttons.The current implementation doesn't account for buttons that might have icons but no text. When loading, the circular progress replaces the text but not the icons.
- {loading ? <CircularProgress size={12} color="inherit" /> : text} + {loading ? <CircularProgress size={12} color="inherit" /> : + (text || (!startIcon && !endIcon) ? text : null)}backend/src/controllers/team.controller.js (2)
56-57: Good job implementing pagination parameters!The implementation of pagination is clean, extracting page and limit from the query parameters. Just one concern though - the default limit (5) doesn't match the frontend's perPage value (10).
- const { page = 1, limit = 5 } = req.query; + const { page = 1, limit = 10 } = req.query;
76-76: Good error logging addition.Error logging is important for debugging. Consider adding structured logging with more context about the request.
- console.log(err); + console.log(`Error in getTeamDetails - page: ${page}, limit: ${limit}`, err);frontend/src/components/PageBtnContainer/PageBtnContainer.jsx (3)
11-58: Solid pagination algorithm implementation!The pagination algorithm handles various edge cases well, but it's quite complex. Consider adding comments to explain the logic for different scenarios to make it more maintainable.
One small concern: the algorithm assumes
totalPagesis at least 1. Add a check to handle edge cases better:const generatePagination = () => { const pages = []; + // Handle edge case where there are no pages + if (totalPages <= 0) { + return pages; + } if (totalPages <= 7) { for (let i = 1; i <= totalPages; i++) { pages.push({ type: 'page', value: i }); } return pages; } // Rest of the pagination algorithm...
60-74: Navigation functions have redundant logic.The nextPage and prevPage functions have wrap-around logic (going to page 1 from the last page and vice versa), but the buttons are disabled when at these limits, making this logic unreachable.
const nextPage = () => { - let nextPage = currentPage + 1; - if (nextPage > totalPages) { - nextPage = 1; - } + const nextPage = currentPage + 1; setCurrentPage(nextPage); }; const prevPage = () => { - let prevPage = currentPage - 1; - if (prevPage < 1) { - prevPage = totalPages; - } + const prevPage = currentPage - 1; setCurrentPage(prevPage); };
119-123: Good use of PropTypes.Proper type checking is important for component stability. Consider making these props required since the component won't function properly without them.
PageBtnContainer.propTypes = { - currentPage: PropTypes.number, - setCurrentPage: PropTypes.func, - totalPages: PropTypes.number, + currentPage: PropTypes.number.isRequired, + setCurrentPage: PropTypes.func.isRequired, + totalPages: PropTypes.number.isRequired, };frontend/src/scenes/settings/TeamTab/TeamTab.jsx (2)
64-87: Updated loadTeamDetails function handles pagination well.The function now properly passes pagination parameters to the API and updates state with the response data. Consider adding a loading indicator specifically for page changes.
const loadTeamDetails = async (page, limit) => { try { setLoading(() => true); + // Add a comment explaining pagination behavior const response = await getOrgDetails(page, limit); if (response.data.users) { const matchedUser = response.data.users.find( (user) => user.id === currentUserId && user.role !== userInfo.role ); if (matchedUser) { updateProfile({ ...userInfo, role: matchedUser.role }); handleTabChange(null, '1'); } setTeam(() => response.data.users); } setOrgName(() => response.data.name); setTotalPages(() => response.data.totalPages); } catch (error) { console.error('Error fetching team details', error.message); handleGenericError('Error fetching team details'); + // Consider adding more specific error handling for pagination errors } finally { setLoading(false); } };
13-14: Good import of the PageBtnContainer component.The import path is correct. For consistency with other components, consider using the alias pattern that's used for other components.
-import PageBtnContainer from '../../../components/PageBtnContainer/PageBtnContainer'; +import PageBtnContainer from '@components/PageBtnContainer/PageBtnContainer';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/controllers/team.controller.js(2 hunks)backend/src/service/team.service.js(1 hunks)frontend/src/components/Button/Button.jsx(3 hunks)frontend/src/components/PageBtnContainer/PageBtnContainer.css(1 hunks)frontend/src/components/PageBtnContainer/PageBtnContainer.jsx(1 hunks)frontend/src/scenes/settings/TeamTab/TeamTab.jsx(4 hunks)frontend/src/services/settingServices.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (7)
frontend/src/services/settingServices.js (1)
23-27: Implementation looks good.The pagination parameters are correctly implemented in the getOrgDetails function. The default values make sense, and the error handling is consistent with other functions.
backend/src/service/team.service.js (1)
47-59: Good implementation of pagination logic.The pagination implementation is well done. Using
findAndCountAllis the right approach to get both the data and total count in one query. The metadata returned (totalUsers, totalPages, currentPage) provides everything needed for frontend pagination.frontend/src/components/Button/Button.jsx (1)
17-18: Well-implemented icon support for buttons.The implementation of start and end icons in the Button component is clean and follows best practices. Props are properly defined with PropTypes and passed correctly to the underlying MuiButton.
Also applies to: 31-32, 55-56
backend/src/controllers/team.controller.js (1)
70-73: Nice addition of pagination metadata!These fields are essential for proper pagination implementation on the frontend.
frontend/src/components/PageBtnContainer/PageBtnContainer.jsx (1)
86-106: Clean UI implementation for pagination!The rendering logic for pagination buttons is well-structured, showing ellipses when needed and highlighting the current page.
One minor improvement: the Button component has text prop and also children with the same content, which is redundant:
<Button key={`page-${item.value}`} type="button" variant="text" buttonType={currentPage === item.value ? 'primary' : ''} text={item.value} onClick={() => changePage(item.value)} disabled={currentPage === item.value} - > - {item.value} - </Button> + />frontend/src/scenes/settings/TeamTab/TeamTab.jsx (2)
45-47: Good state management for pagination.The implementation of pagination state variables is clean. Consider adding default values that match the backend defaults.
58-62:❌ Incorrect review comment
Pagination parameters are well integrated.
The perPage constant and dependency array in useEffect are appropriately set up. However, there's an inconsistency between this value (10) and the backend default (5).
🏁 Script executed:
#!/bin/bash # Verify the default limit in the backend service rg -A 2 "getTeam.*\(" backend/src/service/team.service.jsLength of output: 256
Updated Pagination Review:
The pagination parameters are well integrated and consistent across the codebase. The
perPageconstant set to 10 infrontend/src/scenes/settings/TeamTab/TeamTab.jsxnow correctly matches the backend default limit (10) established inbackend/src/service/team.service.js.No further changes are required on this front.
Likely an incorrect or invalid review comment.
| {totalPages > 1 && ( | ||
| <PageBtnContainer | ||
| currentPage={Number(currentPage)} | ||
| setCurrentPage={setCurrentPage} | ||
| totalPages={Number(totalPages)} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good conditional rendering of pagination.
The PageBtnContainer is only shown when needed (totalPages > 1). Consider adding pagination to the filtered tabs (Administrator and Member) as well.
For the Administrator and Member tabs, you should add pagination components and maintain separate pagination state for each tab:
<TabPanel sx={{ padding: 0, marginTop: '1.2rem' }} value="2">
<TeamTable
team={team.filter((user) => user.role == 'admin')}
setRemoveModalOpen={setOpenRemoveTeamMemberModal}
setChangeRoleModalOpen={setOpenChangeMemberRoleModal}
setSelectedMember={setSelectedMember}
/>
+ {totalPages > 1 && (
+ <PageBtnContainer
+ currentPage={Number(currentPage)}
+ setCurrentPage={setCurrentPage}
+ totalPages={Number(totalPages)}
+ />
+ )}
</TabPanel>Note: This would require more advanced state management to track pagination separately for each tab.
📝 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.
| {totalPages > 1 && ( | |
| <PageBtnContainer | |
| currentPage={Number(currentPage)} | |
| setCurrentPage={setCurrentPage} | |
| totalPages={Number(totalPages)} | |
| /> | |
| )} | |
| <TabPanel sx={{ padding: 0, marginTop: '1.2rem' }} value="2"> | |
| <TeamTable | |
| team={team.filter((user) => user.role == 'admin')} | |
| setRemoveModalOpen={setOpenRemoveTeamMemberModal} | |
| setChangeRoleModalOpen={setOpenChangeMemberRoleModal} | |
| setSelectedMember={setSelectedMember} | |
| /> | |
| {totalPages > 1 && ( | |
| <PageBtnContainer | |
| currentPage={Number(currentPage)} | |
| setCurrentPage={setCurrentPage} | |
| totalPages={Number(totalPages)} | |
| /> | |
| )} | |
| </TabPanel> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/service/team.service.js (1)
63-63: Improved error loggingAdding console.log for errors improves debuggability, but consider using a structured logger for production.
- console.log(err); + console.log('Failed to retrieve team:', err.message);backend/src/test/unit/controllers/team.test.js (1)
105-121: Modified tests for pagination are comprehensiveThe tests have been thoughtfully updated to verify the pagination implementation. However, consider adding tests for different page numbers and limits to ensure edge cases are covered.
Consider adding additional test cases:
it('getTeamDetails - should handle custom page and limit values', async () => { const req = { query: { page: '2', limit: '5' } }; sinon.stub(service, 'getTeam').resolves({ team: { name: 'Test' }, users: userMocks.validList.slice(0, 5), totalUsers: userMocks.validList.length, totalPages: Math.ceil(userMocks.validList.length / 5), currentPage: 2 }); await controller.getTeamDetails(req, res); expect(res.status.args[0][0]).to.equal(200); expect(service.getTeam.calledWith(2, 5)).to.be.true; });backend/src/test/unit/services/team.test.js (1)
67-75: Missing pagination test caseThe error test case should be updated to match the new method signature with pagination parameters.
- await service.getTeam(); + await service.getTeam(1, 10);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/controllers/team.controller.js(2 hunks)backend/src/service/team.service.js(1 hunks)backend/src/test/unit/controllers/team.test.js(1 hunks)backend/src/test/unit/services/team.test.js(2 hunks)frontend/src/components/PageBtnContainer/PageBtnContainer.css(1 hunks)frontend/src/components/PageBtnContainer/PageBtnContainer.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/src/controllers/team.controller.js
- frontend/src/components/PageBtnContainer/PageBtnContainer.css
- frontend/src/components/PageBtnContainer/PageBtnContainer.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (9)
backend/src/service/team.service.js (4)
37-41: Robust pagination implementationThe implementation of pagination parameters with validation is well done. Using Math.max ensures values are always at least 1, preventing negative or zero values that could cause bugs.
48-52: Good use of findAndCountAll for paginationUsing findAndCountAll is the right approach for pagination, as it provides both the total count and the paginated results in a single query.
54-60: Complete pagination metadataThe return object properly includes all necessary pagination metadata (totalUsers, totalPages, currentPage) needed by the frontend.
45-47:❓ Verification inconclusive
Consider consistency in error handling
Returning null instead of throwing an error when no team is found changes the error handling pattern. While this works, make sure frontend code is updated to handle this null return case.
🏁 Script executed:
#!/bin/bash # Check if frontend code handles null return case properly rg -A 3 "getTeam|getOrgDetails" --type js --type jsx frontend/src/Length of output: 96
Verify Frontend Null-Handling Consistency
The initial verification ran into trouble because thergcommand didn’t recognise thejsxfile type. You might need to modify the search to include the JSX files (e.g., by adding--type-add 'jsx:*.jsx') before re-running the check. Without this, it’s unclear if the frontend gracefully handles anullcase fromgetTeamandgetOrgDetails. Please verify manually that all such references in the frontend have appropriate null-checks in place.His palms are sweaty, knees weak, arms are heavy—let's get those error-handling cases covered before the next drop!
backend/src/test/unit/controllers/team.test.js (2)
18-21: Good test cleanup practiceAdding database cleanup after each test ensures test isolation and prevents state leakage between tests.
93-94: Updated test request parameters for paginationTest has been properly updated with pagination parameters to match the new API functionality.
backend/src/test/unit/services/team.test.js (3)
30-33: Consistent test cleanupGood alignment with the controller tests by implementing the same cleanup strategy.
53-58: Updated test mocks for paginationThe mocks have been properly updated to simulate the findAndCountAll response structure. The test now correctly includes pagination parameters.
59-65: Complete test assertions for paginationThe test properly verifies all pagination metadata returned by the service. This ensures the contract between the service and consumers is maintained.
added pagination to teams with limit of 10 entries per page
Add Pagination to Teams Page in Settings #554