perf: optimize firestore reads and restore public routes#2601
perf: optimize firestore reads and restore public routes#2601iamitprakash wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAuthentication is removed from Public Paginated User Status & Search
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
models/users.js (1)
675-692: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSlice after the archived/onboarding filters.
finalItems.slice(skip, skip + limit)runs before!doc.roles?.archivedand beforegetUsersWithOnboardingStateInRange(...). That lets archived or out-of-range users consume page slots, andtotalCount = finalItems.lengthno longer matches the result set this endpoint actually returns. Compute the filtered result set (or at least its pre-pagination total) before slicing.🤖 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 `@models/users.js` around lines 675 - 692, The pagination in the users query is applied too early in the main flow around paginatedFinalItems, userDocs filtering, and getUsersWithOnboardingStateInRange, so archived or out-of-range users can still consume page slots. Move skip/limit handling to after the archived filter and onboarding-state filtering, and make sure totalCount reflects the full filtered result set rather than finalItems.length.controllers/users.js (1)
975-990: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply
page/sizeto both search branches.This controller only forwards
skip/limitwhendev === "true". Requests like/users/search?role=member&page=1&size=20still go throughretreiveFilteredUsers(req.query)with no pagination, even though the lower layers now support it.🤖 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 `@controllers/users.js` around lines 975 - 990, Update the users search controller so pagination is applied in both branches, not only when dev === "true". In the users search handler that calls dataAccess.retreiveFilteredUsers, parse page and size once and pass the computed skip and limit into every retreiveFilteredUsers call, including the non-dev path, so requests like /users/search?role=member&page=1&size=20 are paginated consistently.controllers/userStatus.js (1)
80-104: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPaginating raw statuses breaks visible-page semantics.
userStatusModel.getAllUserStatus(req.query)now slices status docs before this controller removes archived users. Archived rows can consume page slots, so asize=Nresponse may return fewer thanNvisible users even when more valid rows exist later.totalUserStatusalso becomes the current page length, not the total matching count.🤖 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 `@controllers/userStatus.js` around lines 80 - 104, The pagination in userStatusController.getAllUserStatus is applied before filtering archived users, so page slots can be wasted and totalUserStatus only reflects the visible page count. Move the archived-user filtering into the query or data-access layer, or rework getAllUserStatus/userStatusModel.getAllUserStatus so pagination is based on visible, non-archived statuses; also return an accurate total matching count separate from the page length.
🤖 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 `@middlewares/validators/userStatus.js`:
- Around line 88-89: The pagination schema in userStatus validation currently
allows page and size independently, but models/userStatus.js only applies
pagination when both are present. Update the Joi rules in the validator so page
and size are required together (or both omitted), using the existing schema area
around the page/size definitions to enforce the paired constraint and prevent
single-parameter requests from passing.
---
Outside diff comments:
In `@controllers/users.js`:
- Around line 975-990: Update the users search controller so pagination is
applied in both branches, not only when dev === "true". In the users search
handler that calls dataAccess.retreiveFilteredUsers, parse page and size once
and pass the computed skip and limit into every retreiveFilteredUsers call,
including the non-dev path, so requests like
/users/search?role=member&page=1&size=20 are paginated consistently.
In `@controllers/userStatus.js`:
- Around line 80-104: The pagination in userStatusController.getAllUserStatus is
applied before filtering archived users, so page slots can be wasted and
totalUserStatus only reflects the visible page count. Move the archived-user
filtering into the query or data-access layer, or rework
getAllUserStatus/userStatusModel.getAllUserStatus so pagination is based on
visible, non-archived statuses; also return an accurate total matching count
separate from the page length.
In `@models/users.js`:
- Around line 675-692: The pagination in the users query is applied too early in
the main flow around paginatedFinalItems, userDocs filtering, and
getUsersWithOnboardingStateInRange, so archived or out-of-range users can still
consume page slots. Move skip/limit handling to after the archived filter and
onboarding-state filtering, and make sure totalCount reflects the full filtered
result set rather than finalItems.length.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36d18818-afae-467d-aee4-51970349d554
📒 Files selected for processing (10)
controllers/userStatus.jscontrollers/users.jsmiddlewares/validators/userStatus.jsmodels/userStatus.jsmodels/users.jsroutes/userStatus.jsroutes/users.jsservices/dataAccessLayer.jstest/integration/userStatus.test.jstest/integration/usersFilter.test.js
💤 Files with no reviewable changes (1)
- test/integration/usersFilter.test.js
| page: Joi.number().integer().min(0).error(new Error(`page must be a non-negative integer.`)), | ||
| size: Joi.number().integer().min(1).max(1000).error(new Error(`size must be a number between 1 and 1000.`)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require page and size together.
models/userStatus.js only paginates when both values are present, but this schema accepts either one alone. Requests like ?page=1 or ?size=50 pass validation and then silently fall back to the unpaginated query.
💡 Suggested change
const schema = Joi.object()
.keys({
aggregate: Joi.boolean().valid(true).error(new Error(`Invalid boolean value passed for aggregate.`)),
state: Joi.string()
.trim()
.valid(userState.IDLE, userState.ACTIVE, userState.OOO, userState.ONBOARDING)
.error(new Error(`Invalid State. State must be either IDLE, ACTIVE, OOO, or ONBOARDING`)),
page: Joi.number().integer().min(0).error(new Error(`page must be a non-negative integer.`)),
size: Joi.number().integer().min(1).max(1000).error(new Error(`size must be a number between 1 and 1000.`)),
})
+ .and("page", "size")
.messages({📝 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.
| page: Joi.number().integer().min(0).error(new Error(`page must be a non-negative integer.`)), | |
| size: Joi.number().integer().min(1).max(1000).error(new Error(`size must be a number between 1 and 1000.`)), | |
| page: Joi.number().integer().min(0).error(new Error(`page must be a non-negative integer.`)), | |
| size: Joi.number().integer().min(1).max(1000).error(new Error(`size must be a number between 1 and 1000.`)), | |
| }) | |
| .and("page", "size") |
🤖 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 `@middlewares/validators/userStatus.js` around lines 88 - 89, The pagination
schema in userStatus validation currently allows page and size independently,
but models/userStatus.js only applies pagination when both are present. Update
the Joi rules in the validator so page and size are required together (or both
omitted), using the existing schema area around the page/size definitions to
enforce the paired constraint and prevent single-parameter requests from
passing.
1 participant
No description provided.