feat(auth): add pagination support to Auth user list - #300
Conversation
📝 WalkthroughWalkthroughAdds backend admin endpoints for listing and deleting users, and implements paginated user browsing in the Auth page with guarded paginated fetches, create/delete refreshes, and a Pagination control under the user table. ChangesAdmin users (backend + frontend)
Sequence Diagram(s)sequenceDiagram
participant Client as AuthPage
participant Server as DashboardAPI
participant DB as MongoDB
Client->>Server: GET /:projectId/admin/users?page=&limit= (reqId)
Server->>DB: find(query, projection).sort(createdAt).skip((page-1)*limit).limit(limit)
Server->>DB: countDocuments(query)
DB-->>Server: items, total
Server-->>Client: { items, total, page, limit }
Client->>Client: if reqId matches → set users, set totalRecords
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Quick update: PR is up, implementation is completed. I'll do final testing and upload screenshots/recordings tonight.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web-dashboard/src/pages/Auth.jsx`:
- Around line 83-92: In the useEffect in Auth.jsx that fetches
`/api/projects/${projectId}/collections/users/data` wrap the post-await state
updates with a re-check for component liveness (or a requestId guard) to avoid
stale overwrites: after the `await api.get(...)` and before calling `setUsers`
and `setTotalRecords` verify `isMounted` (or compare the captured requestId
against the latest) and only proceed if still valid; apply this check around the
usage of `normalizeUsersResponse`, `setUsers`, and `setTotalRecords` so an
out-of-order response cannot overwrite newer pagination state.
- Around line 237-246: The delete path (handleDeleteUser) currently removes the
user from state with setUsers but doesn't update setTotalRecords, causing
pagination drift; after removing the user call setTotalRecords(prev =>
Math.max(prev - 1, 0)) to keep the total in sync and, if the current page
becomes empty (i.e., normalizeUsersResponse(users) length === 0 and page > 1),
decrement the page or trigger a refetch so the UI moves to the previous page;
update the logic in handleDeleteUser near the setUsers call and reference
setTotalRecords, setUsers, normalizeUsersResponse and page handling to implement
this fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2932cc8c-9005-4ecf-85a8-26d2a351bc59
📒 Files selected for processing (1)
apps/web-dashboard/src/pages/Auth.jsx
|
Some coderabbitai |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web-dashboard/src/pages/Auth.jsx`:
- Around line 173-176: The delete-success callback is using stale closure values
for users and page; change the mutations to use functional updaters so they read
the latest state: call setUsers(prevUsers => { const next =
normalizeUsersResponse(prevUsers).filter(u => u._id !== userId); return next;
}), call setTotalRecords(prev => Math.max(prev - 1, 0)), and compute page
rollback from the latest users/page by using setPage(prevPage => { if
(next.length === 0 && prevPage > 1) return prevPage - 1; return prevPage; }) (or
use synced refs for users/page if you prefer), ensuring you reference
normalizeUsersResponse, setUsers, setTotalRecords, setPage and userId in the
updated logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2479fb51-b6d5-4299-8ba3-9914c7494abc
📒 Files selected for processing (1)
apps/web-dashboard/src/pages/Auth.jsx
|
Addressed all CodeRabbit feedback:
Final pagination validation and screenshots are in progress. |
|
I can see that only frontend is changed |
Yes, only frontend changes were made. Pagination is handled by the existing collection data endpoint ( |
|
Final validation completed.
Screenshots attached below. 
|
|
2 months back when we integrated auth as a service let me know ur views @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find Auth.jsx
fd -i "Auth.jsx" --exec cat {}
# Find userauth route/controller files
fd -i "userauth" -e js -e ts -e jsx -e tsx --exec echo "FILE: {}" \; --exec cat {} \; 2>/dev/null
# Also search by pattern in route registrations
rg -rn "userauth" --type js --type ts -C3 2>/dev/null | head -100
# Look for any pagination support in userauth routes
rg -rn "page\|limit\|pagination" --type js --type ts -C2 2>/dev/null | grep -i "userauth\|auth" | head -50Length of output: 211489
The ProblemThe PR fetches the user list using: GET /api/projects/${projectId}/collections/users/data?page=${page}&limit=${limit}This is a generic collection CRUD endpoint — it has no awareness that the
The Inconsistency in This PRLooking at
Why This HappenedThere's no paginated list endpoint under // In dashboard-api userAuth.controller.js
module.exports.listAdminUsers = async (req, res) => {
try {
const project = req.project;
const page = Math.max(1, parseInt(req.query.page) || 1);
const limit = Math.min(100, Math.max(1, parseInt(req.query.limit) || 50));
const skip = (page - 1) * limit;
const usersColConfig = project.collections.find(c => c.name === 'users');
if (!usersColConfig) return res.status(404).json({ error: "Auth collection not found" });
const connection = await getConnection(project._id);
const Model = getCompiledModel(connection, usersColConfig, project._id, project.resources.db.isExternal);
const [items, total] = await Promise.all([
Model.find({}, { password: 0 }).skip(skip).limit(limit).lean(),
Model.countDocuments()
]);
res.json({ items, total, page, limit });
} catch (err) {
res.status(500).json({ error: err.message });
}
};Then register it in the dashboard-api admin routes and update SummaryThe PR author's implementation was a pragmatic workaround for a missing endpoint — the pagination logic itself is sound, but it's wired to the wrong API. The real fix needed here is:
Would you like me to open a follow-up issue for adding these missing admin endpoints, |
|
yh all routes should go through - /admin |
Done. I moved the Auth user list/delete flow to dedicated Also added paginated admin user APIs on the backend with password exclusion, and updated the frontend to use those endpoints consistently. Tested locally by changing page sizes (10/20/50/100), verifying paginated refetches, and confirming list/delete requests hit |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web-dashboard/src/pages/Auth.jsx (2)
252-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse current pagination when refreshing after create.
Lines 252-255 reuse
pageandlimitfrom the submit-time closure. If pagination changes before the POST resolves, this follow-up GET fetches the old page, incrementslatestUsersRequestId, and can overwrite the newer page's data with stale results. Please drive this refresh through a sharedfetchUsershelper or read the latest pagination from synced refs before issuing the GET.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/pages/Auth.jsx` around lines 252 - 263, The GET after create can use stale closure variables (page, limit) and overwrite newer data; update the flow to drive the refresh via a shared fetchUsers helper (e.g., create/fix a fetchUsers function called by UI and by the POST follow-up) or read the latest pagination from synced refs before requesting (use pageRef.current and limitRef.current) so the GET uses current pagination; keep the optimistic request id logic (latestUsersRequestId.current) and call setUsers/ setTotalRecords with normalizeUsersResponse only after confirming requestId === latestUsersRequestId.current.
75-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock stale row actions while a new page is loading.
Lines 75-103 refetch on every
page/limitchange, but the component never enters a users-loading state before those requests. After Line 339 changespage, the paginator can already show the new page whileUserTablestill renders the old rows, so edit/delete actions stay live against stale data. Add a dedicated loading state for paginated fetches and clear/disable the table until that request settles.Also applies to: 335-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/pages/Auth.jsx` around lines 75 - 103, Add a dedicated users-loading state (e.g., usersLoading / setUsersLoading) and use it around the paginated users fetch in the existing useEffect/fetchData flow: set usersLoading true (and optionally clear users via setUsers([])) immediately before issuing the users API call triggered by page/limit changes, and set usersLoading false in the finally block after the users request settles; keep the existing latestUsersRequestId.current check and only call setUsers/setTotalRecords when the request is current. Also update the table/paginator (or the row action handlers) to disable edit/delete when usersLoading is true so row actions are blocked until the paginated fetch completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/userAuth.controller.js`:
- Around line 241-268: Update listAdminUsers to conform to the API contract and
sanitize errors: change the handler signature to (req, res, next), and on
success return res.json({ success: true, data: { items, total, page, limit },
message: "" }) instead of the current payload; in the catch block do not expose
err.message — call next(new AppError("Failed to list admin users", 500)) (or
similar sanitized message) so failures are routed through the AppError flow;
apply the same pattern (add next param, standardized success object, and
AppError-based error forwarding) to the other admin handler referenced around
lines 270-288.
- Around line 281-286: Validate the userId at the start of deleteAdminUser using
mongoose.Types.ObjectId.isValid(userId) before constructing a new ObjectId; if
invalid, call next(new AppError("Invalid user id", 400)). After validation,
perform Model.deleteOne(...) and if deletedCount === 0 call next(new
AppError("User not found", 404)). For success, return the unified response shape
res.json({ success: true, data: null, message: "User deleted successfully" }).
Replace direct res.status(...).json({ error: ... }) error returns with AppError
via next(...) so global error handler formats errors consistently.
In `@apps/dashboard-api/src/routes/projects.js`:
- Around line 51-52: The test mock for the userAuth controller is missing two
exported handlers causing route registration to receive undefined; update the
mocked module in the failing test to include listAdminUsers: jest.fn(() => ...)
and deleteAdminUser: jest.fn(() => ...) so the router.get (listAdminUsers) and
router.delete (deleteAdminUser) in projects.js receive functions during Jest
runs; locate the mock of ../controllers/userAuth.controller in the test file and
add those two jest.fn mocks to the exported mock object.
---
Outside diff comments:
In `@apps/web-dashboard/src/pages/Auth.jsx`:
- Around line 252-263: The GET after create can use stale closure variables
(page, limit) and overwrite newer data; update the flow to drive the refresh via
a shared fetchUsers helper (e.g., create/fix a fetchUsers function called by UI
and by the POST follow-up) or read the latest pagination from synced refs before
requesting (use pageRef.current and limitRef.current) so the GET uses current
pagination; keep the optimistic request id logic (latestUsersRequestId.current)
and call setUsers/ setTotalRecords with normalizeUsersResponse only after
confirming requestId === latestUsersRequestId.current.
- Around line 75-103: Add a dedicated users-loading state (e.g., usersLoading /
setUsersLoading) and use it around the paginated users fetch in the existing
useEffect/fetchData flow: set usersLoading true (and optionally clear users via
setUsers([])) immediately before issuing the users API call triggered by
page/limit changes, and set usersLoading false in the finally block after the
users request settles; keep the existing latestUsersRequestId.current check and
only call setUsers/setTotalRecords when the request is current. Also update the
table/paginator (or the row action handlers) to disable edit/delete when
usersLoading is true so row actions are blocked until the paginated fetch
completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff911ad5-fea6-4cb4-8616-103fd86bb0f0
📒 Files selected for processing (3)
apps/dashboard-api/src/controllers/userAuth.controller.jsapps/dashboard-api/src/routes/projects.jsapps/web-dashboard/src/pages/Auth.jsx
|
|
yh this looks more prod ready |
Thanks! The implementation evolved a bit during review as well. Besides the Auth page pagination, I also moved the user list/delete flows to dedicated Please review whenever you get a chance, and I'm happy to make any additional changes if needed. |
| res.json({ | ||
| success: true, | ||
| data: { items, total, page, limit }, | ||
| message: "" | ||
| }); |
There was a problem hiding this comment.
Everything looks perfect.
but I think we also have success response class.
DOnt remember.
I think @Ayush4958 has created in earlier PR maybe?
pleae check if exist
then use it
else I will merge.
|
Give me 10–15 minutes. I'll check if the success response class already exists in the project and update the PR accordingly. I'll get back to you shortly. |
It exist on same place where ApiError utility exist but utility name is ApiResponse |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/dashboard-api/src/controllers/userAuth.controller.js (2)
299-299: ⚡ Quick winConsider logging the original error before forwarding a sanitized AppError.
The catch block forwards a generic
AppError(500, "Failed to delete admin user")without logging the original error, making it harder to diagnose production failures. Logging the error server-side (as done elsewhere in this file at line 129) preserves diagnostic context while keeping client responses sanitized.📊 Suggested enhancement
} catch (err) { + console.error('Failed to delete admin user:', err); next(new AppError(500, "Failed to delete admin user")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard-api/src/controllers/userAuth.controller.js` at line 299, In the catch block that currently does next(new AppError(500, "Failed to delete admin user")), log the original error before forwarding the sanitized AppError; e.g., call the existing server logger (or console.error) with the caught error (logger.error(err, "Failed to delete admin user") or console.error("Failed to delete admin user:", err)) and then call next(new AppError(500, "Failed to delete admin user")); reference the existing next and AppError usage so you update the same catch handler that wraps the admin-delete flow.
270-270: ⚡ Quick winConsider logging the original error before forwarding a sanitized AppError.
The catch block forwards a generic
AppError(500, "Failed to list admin users")without logging the original error. This makes debugging production issues difficult, as operators won't know whether the failure was a DB connection timeout, schema mismatch, or other root cause. Other handlers in this file log errors withconsole.log(err)(line 129), which helps diagnostics while still sanitizing client responses.📊 Suggested enhancement
} catch (err) { + console.error('Failed to list admin users:', err); next(new AppError(500, "Failed to list admin users")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard-api/src/controllers/userAuth.controller.js` at line 270, The catch block that currently does next(new AppError(500, "Failed to list admin users")) should first log the original error before forwarding the sanitized error; locate the catch handling for listing admin users (the block that calls next(new AppError(500, "Failed to list admin users"))) and add a concise log of the caught error (e.g., console.log(err) or processLogger.error(err)) immediately before the next(...) call so operators can see the root cause while the client still receives the generic AppError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/dashboard-api/src/controllers/userAuth.controller.js`:
- Line 299: In the catch block that currently does next(new AppError(500,
"Failed to delete admin user")), log the original error before forwarding the
sanitized AppError; e.g., call the existing server logger (or console.error)
with the caught error (logger.error(err, "Failed to delete admin user") or
console.error("Failed to delete admin user:", err)) and then call next(new
AppError(500, "Failed to delete admin user")); reference the existing next and
AppError usage so you update the same catch handler that wraps the admin-delete
flow.
- Line 270: The catch block that currently does next(new AppError(500, "Failed
to list admin users")) should first log the original error before forwarding the
sanitized error; locate the catch handling for listing admin users (the block
that calls next(new AppError(500, "Failed to list admin users"))) and add a
concise log of the caught error (e.g., console.log(err) or
processLogger.error(err)) immediately before the next(...) call so operators can
see the root cause while the client still receives the generic AppError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eab1ad3d-b487-4791-9198-f3a0b3ef7020
📒 Files selected for processing (2)
apps/dashboard-api/src/__tests__/routes.projects.storage.test.jsapps/dashboard-api/src/controllers/userAuth.controller.js
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard-api/src/tests/routes.projects.storage.test.js
|
Checked the codebase and confirmed that a shared I've updated the admin user endpoints to use it:
Commit: Thanks for pointing it out. |
Thanks for the quick pointer. I checked the utility and updated the admin user endpoints to use the shared Applied in commit |
|
yh Thank you. |
3 participants
🚀 Pull Request Description
Fixes #298
Implemented pagination support for the Auth page user list to make it consistent with the existing Database page pagination behavior.
Changes Made
page,limit,totalRecords) inAuth.jsxThis allows users to navigate through all available auth users instead of being restricted to the first set of records.
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Pending)
The current test project does not contain enough Auth users to demonstrate multi-page navigation visually.
Current verification completed:
pageandlimitparameters.Console verification output:
I am currently preparing additional test data to fully validate page navigation behavior and will update this PR with screenshots and/or recordings once testing is completed.
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
New Features
Bug Fixes