Fix incorrect total user count display on members page #426
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
frontend/src/components/drawers/WorkspaceDrawer.tsxOops! Something went wrong! :( ESLint: 9.12.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe changes in this pull request involve modifications to three main files: Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
frontend/src/components/drawers/WorkspaceDrawer.tsx (1)
1-4: Consolidate Material-UI icon importsConsider using the destructured import pattern for better maintainability and consistency:
-import KeyboardDoubleArrowLeftIcon from "@mui/icons-material/KeyboardDoubleArrowLeft"; -import KeyboardDoubleArrowRightIcon from "@mui/icons-material/KeyboardDoubleArrowRight"; -import PeopleIcon from "@mui/icons-material/People"; -import SpaceDashboardIcon from "@mui/icons-material/SpaceDashboard"; +import { + KeyboardDoubleArrowLeft as KeyboardDoubleArrowLeftIcon, + KeyboardDoubleArrowRight as KeyboardDoubleArrowRightIcon, + People as PeopleIcon, + SpaceDashboard as SpaceDashboardIcon +} from "@mui/icons-material";frontend/src/pages/workspace/member/Index.tsx (2)
73-73: Consider documenting standard component sizes.The change from "sm" to an explicit size of 20 is good for consistency, but consider documenting these standard sizes in a shared constants file or design system documentation.
Line range hint
65-78: Consider enhancing error handling for infinite scroll.While the infinite scroll implementation works correctly, consider adding error handling and user feedback for failed page loads. This would improve the user experience when network issues occur.
<InfiniteScroll pageStart={0} loadMore={() => fetchNextPage()} hasMore={hasNextPage} + threshold={100} loader={ <Box className="loader" key={0}> <CircularProgress size={20} /> </Box> } + error={ + <Box className="error" key={1}> + <Typography color="error">Failed to load more members. Click to retry.</Typography> + </Box> + } useWindow={false} style={{ width: "100%", }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/components/drawers/WorkspaceDrawer.tsx(4 hunks)frontend/src/constants/layout.ts(1 hunks)frontend/src/pages/workspace/member/Index.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/constants/layout.ts
🔇 Additional comments (3)
frontend/src/components/drawers/WorkspaceDrawer.tsx (1)
17-17: Verify the constant rename impact across the codebase
The constant has been renamed from COLLAPESED_DRAWER_WIDTH to COLLAPSED_DRAWER_WIDTH. Let's ensure all references are updated.
Also applies to: 67-67, 80-80
✅ Verification successful
All references to the drawer width constant are using the correct spelling
The codebase search shows that all instances are using the new correct spelling COLLAPSED_DRAWER_WIDTH. The constant is defined in frontend/src/constants/layout.ts and consistently used in WorkspaceDrawer.tsx with no occurrences of the old misspelled version COLLAPESED_DRAWER_WIDTH.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old constant name
rg "COLLAPESED_DRAWER_WIDTH"
# Search for all uses of the new constant name to verify consistency
rg "COLLAPSED_DRAWER_WIDTH"
Length of output: 482
frontend/src/pages/workspace/member/Index.tsx (2)
54-54: LGTM! The user count now correctly displays the total number of users.
This change, combined with the useMemo implementation above, ensures that all workspace users are counted, fixing the previous limitation where only the first page was considered.
34-41: LGTM! The useMemo implementation correctly aggregates all users.
The implementation properly flattens the paginated user list, addressing the core issue where only the first page of users was being counted. The null coalescing operator provides a safe fallback.
Let's verify that we're correctly processing all pages:
✅ Verification successful
The useMemo implementation correctly handles paginated user data
The implementation properly concatenates all pages of users, and the backend supports cursor-based pagination with a default page size of 10 users per request. The frontend's infinite query implementation correctly fetches and accumulates all pages, effectively removing the previous 10-user limitation. The dependency array is properly set to trigger recalculation when pages change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded page size limits in the API hooks
# that might affect the total user count
rg -A 5 "useGetWorkspaceUserListQuery|getWorkspaceUserList"
Length of output: 1706
Script:
#!/bin/bash
# Let's check the API response type and pagination parameters
ast-grep --pattern 'type GetWorkspaceUserListResponse = {
$$$
}'
# Also check the API call implementation for any page size parameters
rg -A 10 '/workspaces/\$\{workspaceId\}/users'
Length of output: 752
Script:
#!/bin/bash
# Let's check the type definitions for pagination-related types
ast-grep --pattern 'interface $_ {
$$$
cursor?: $_
$$$
}'
# And check for any page size constants or configurations
rg -i "pagesize|page_size|limit" --type ts
Length of output: 3145
| @@ -50,7 +51,7 @@ function MemberIndex() { | |||
| <Typography variant="h5" fontWeight="bold"> | |||
| {workspace?.title}{" "} | |||
| <Typography component="span" variant="inherit" color="primary"> | |||
| {workspaceUserPageList?.pages[0].workspaceUsers.length} | |||
| {userList.length} | |||
There was a problem hiding this comment.
As userList is paginated, we can't determine that the length is same with total users.
It seems it needs to modify backend API to know total number of users.
For reference, you can look at FindWorkspaceDocumentsResponse (Code Pointer).
What this PR does / why we need it?
This PR fixes the issue where the total user count displayed on the members page is incorrectly set to a maximum of 10, regardless of the actual number of users present in the workspace. The updated code will ensure that the total user count accurately reflects the number of users in the workspace.
Any background context you want to provide?
The inconsistency in the user count display could lead to confusion for users, as they may think that the workspace has fewer participants than it actually does. This fix addresses that by removing the cap on the displayed count.
What are the relevant tickets?
Fixes #300
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
COLLAPESED_DRAWER_WIDTHtoCOLLAPSED_DRAWER_WIDTH.Style