refactor: migrate users.list endpoint to ajv schema#39368
refactor: migrate users.list endpoint to ajv schema#39368krishikajain28 wants to merge 16 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new server-side GET Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API.v1.users.list
participant Auth as Auth/Permissions
participant DB as MongoDB
Client->>API: GET /api/v1/users.list?query...&offset=&count=&sort=&fields=
API->>Auth: validate auth & permissions
Auth-->>API: authorized
API->>API: AJV validate & parse params
API->>DB: aggregation pipeline (filter, projection, facet: results + total)
DB-->>API: { results[], total }
API-->>Client: { users: results, count: results.length, offset, total, success: true }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/users.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/users.ts:823">
P1: `users.getAvatarSuggestion` was unintentionally removed by replacing its route entry with `users.list`, causing an API regression for existing clients.</violation>
<violation number="2" location="apps/meteor/app/api/server/v1/users.ts:862">
P1: `users.list` 200 response schema omits `success` even though `API.v1.success(...)` returns it, causing schema mismatch with `additionalProperties: false`.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 846-864: The response schema compiled with ajv.compile for the
users list is missing the top-level "success" boolean that API.v1.success()
injects; because additionalProperties is false this will fail validation. Update
the AJV schema used in this block (the ajv.compile<{ users: IUser[]; count:
number; offset: number; total: number; }>) to include a "success" property of
type "boolean" and add "success" to the required array so the schema matches
responses produced by API.v1.success().
- Around line 867-877: The new action() flow omitted the prior permission check
for "view-outside-room", causing a security regression; update action() so that
when constructing nonEmptyQuery (currently calling getNonEmptyQuery(query, await
hasPermissionAsync(this.userId, 'view-full-other-user-info'))), you also
evaluate whether the caller has hasPermissionAsync(this.userId,
'view-outside-room') (and respect the
API_Apply_permission_view-outside-room_on_users-list setting) and pass the
combined/appropriate boolean into getNonEmptyQuery (or otherwise gate the query
construction) so the users list honors the original view-outside-room permission
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98ef8de3-1303-4d64-881a-5afa7fc860b6
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/users.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/users.ts (2)
919-961: LGTM!The MongoDB aggregation pipeline implementation is correct. The facet pattern for combining paginated results with total count is efficient, and the fallback for empty
totalCountarray ({ total: 0 }) handles edge cases properly.
478-590: 🛠️ Refactor suggestion | 🟠 MajorRemove commented-out code instead of preserving it.
Large blocks of commented code should be removed rather than kept in the codebase. Version control (git) preserves history if the old implementation needs to be referenced later.
As per coding guidelines: "Avoid code comments in the implementation".
⛔ Skipped due to learnings
Learnt from: ahmed-n-abdeltwab Repo: RocketChat/Rocket.Chat PR: 38974 File: apps/meteor/app/api/server/v1/im.ts:220-221 Timestamp: 2026-02-24T19:09:09.561Z Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.Learnt from: tassoevan Repo: RocketChat/Rocket.Chat PR: 38219 File: packages/core-typings/src/cloud/Announcement.ts:5-6 Timestamp: 2026-01-17T01:51:47.764Z Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.Learnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementationLearnt from: ahmed-n-abdeltwab Repo: RocketChat/Rocket.Chat PR: 39230 File: apps/meteor/app/api/server/v1/chat.ts:214-222 Timestamp: 2026-03-03T11:11:48.541Z Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/meteor/app/api/server/v1/users.ts (2)
847-865:⚠️ Potential issue | 🔴 CriticalResponse schema must include
successto matchAPI.v1.success()output.With
additionalProperties: false, the current 200 schema rejects the top-levelsuccessboolean added byAPI.v1.success(...).🐛 Proposed fix
- 200: ajv.compile<{ - users: IUser[]; - count: number; - offset: number; - total: number; - }>({ + 200: ajv.compile<{ + users: IUser[]; + count: number; + offset: number; + total: number; + success: boolean; + }>({ type: 'object', properties: { users: { type: 'array', items: { type: 'object' }, }, count: { type: 'number' }, offset: { type: 'number' }, total: { type: 'number' }, + success: { type: 'boolean', enum: [true] }, }, - required: ['users', 'count', 'offset', 'total'], + required: ['users', 'count', 'offset', 'total', 'success'], additionalProperties: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/users.ts` around lines 847 - 865, The 200 response schema compiled via ajv.compile for the users endpoint currently has additionalProperties: false and thus rejects the top-level success boolean returned by API.v1.success(); update that schema (the object passed to ajv.compile in this users response block) to declare a "success" property with type "boolean" and include "success" in the required array so the API.v1.success(...) output is accepted.
869-879:⚠️ Potential issue | 🟠 MajorRestore
view-outside-roomenforcement forusers.list.The new handler dropped the setting-gated
view-outside-roompermission check that existed in the previous implementation, changing access behavior.🛡️ Proposed fix
async function action() { + if ( + settings.get('API_Apply_permission_view-outside-room_on_users-list') && + !(await hasPermissionAsync(this.userId, 'view-outside-room')) + ) { + return API.v1.forbidden(); + } + const { offset, count } = await getPaginationItems(this.queryParams); const { sort, fields, query } = await this.parseJsonQuery();Based on learnings: "In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/users.ts` around lines 869 - 879, Restore the dropped "view-outside-room" permission check in the users.list handler by calling hasPermissionAsync(this.userId, 'view-outside-room') and using its boolean result to preserve original access semantics; specifically, pass that permission flag into getNonEmptyQuery (or, if the original implementation enforced filtering after building the query, apply the same filtering logic when the permission is false) so the query/result set is restricted for users without view-outside-room exactly as before; update the block around getNonEmptyQuery/query handling (referencing getNonEmptyQuery and the users.list handler) to reintroduce this permission check without changing other logic.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
479-591: Remove commented-out legacy route and inline implementation comments.Keeping the large commented
users.listblock and inline comments in the active path adds noise and conflicts with repository guidance.As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 880-895
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/users.ts` around lines 479 - 591, Remove the large commented-out legacy API.v1.addRoute block for 'users.list' (the entire commented route and its inner async get() implementation) and strip any inline explanatory comments left in that area; ensure you only remove commented code (no active logic) and leave intact the real users list route/handlers elsewhere, referencing API.v1.addRoute('users.list') and the async get() implementation to locate the block to delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 847-865: The 200 response schema compiled via ajv.compile for the
users endpoint currently has additionalProperties: false and thus rejects the
top-level success boolean returned by API.v1.success(); update that schema (the
object passed to ajv.compile in this users response block) to declare a
"success" property with type "boolean" and include "success" in the required
array so the API.v1.success(...) output is accepted.
- Around line 869-879: Restore the dropped "view-outside-room" permission check
in the users.list handler by calling hasPermissionAsync(this.userId,
'view-outside-room') and using its boolean result to preserve original access
semantics; specifically, pass that permission flag into getNonEmptyQuery (or, if
the original implementation enforced filtering after building the query, apply
the same filtering logic when the permission is false) so the query/result set
is restricted for users without view-outside-room exactly as before; update the
block around getNonEmptyQuery/query handling (referencing getNonEmptyQuery and
the users.list handler) to reintroduce this permission check without changing
other logic.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 479-591: Remove the large commented-out legacy API.v1.addRoute
block for 'users.list' (the entire commented route and its inner async get()
implementation) and strip any inline explanatory comments left in that area;
ensure you only remove commented code (no active logic) and leave intact the
real users list route/handlers elsewhere, referencing
API.v1.addRoute('users.list') and the async get() implementation to locate the
block to delete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7e8dddf-f7c9-4636-856d-020c3bef60b2
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/users.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
824-963:⚠️ Potential issue | 🟠 MajorPotential client break: avatar suggestion consumer still targets
/v1/users.getAvatarSuggestion.
apps/meteor/client/components/avatar/UserAvatarEditor/useUserAvatarSuggestions.ts:1-11still calls/v1/users.getAvatarSuggestionand expectsdata.suggestions; this route/shape change tousers.listrisks breaking avatar suggestions unless backward compatibility is kept.⛔ Skipped due to learnings
Learnt from: tassoevan Repo: RocketChat/Rocket.Chat PR: 38219 File: packages/core-typings/src/cloud/Announcement.ts:5-6 Timestamp: 2026-01-17T01:51:47.764Z Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
479-591: Remove the commented-out legacyusers.listblock.This is now a dead second copy of the endpoint beside the live implementation, which will drift and make future reviews harder. A follow-up is fine if you want to keep this PR narrowly scoped. As per coding guidelines, "Avoid code comments in the implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/users.ts` around lines 479 - 591, Remove the entire commented-out legacy API.v1.addRoute block for 'users.list' (the large commented block starting with "API.v1.addRoute(" and ending with the matching closing parenthesis and semicolon) so only the active implementation remains; locate the commented block around the API.v1.addRoute('users.list', ...) symbol in apps/meteor/app/api/server/v1/users.ts and delete it wholesale to avoid duplicate dead code and future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 479-591: Remove the entire commented-out legacy API.v1.addRoute
block for 'users.list' (the large commented block starting with
"API.v1.addRoute(" and ending with the matching closing parenthesis and
semicolon) so only the active implementation remains; locate the commented block
around the API.v1.addRoute('users.list', ...) symbol in
apps/meteor/app/api/server/v1/users.ts and delete it wholesale to avoid
duplicate dead code and future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b57f162-4c8e-4f3e-83cf-42177f9f4038
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/users.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
875-895: No changes needed. The route configuration correctly omitsqueryOperationsbecause the new implementation does not rely on route-level metadata for query validation. TheparseJsonQuery()method only applies metadata-based validation whenqueryFieldsis defined on the route (line 142-143 of parseJsonQuery.ts), and the new route does not definequeryFields. Instead, the action function performs explicit validation at lines 945–965 with hardcoded allowed operations including$orand$and, which is independent of route metadata and will continue to work as before. This design is cleaner and avoids unnecessary metadata.> Likely an incorrect or invalid review comment.
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
where are the remaining steps?
Hi @ahmed-n-abdeltwab. Do you mean the typing export steps ( |
These are already implemented in the users file
yes |
…ishikajain28/Rocket.Chat into fix/migrate-users-list-schema
@ahmed-n-abdeltwab I have completely removed the legacy manual typings for users.list in packages/rest-typings/src/v1/users.ts, The code is pushed and ready (I see the Dionisio bot is just waiting on the QA label/milestone) . |
| url: string; | ||
| } | ||
| >; | ||
| success: boolean; |
| authRequired: true, | ||
| permissionsRequired: ['view-d-room'], |
There was a problem hiding this comment.
We need to stay consistent with the old API options
| '/v1/users.list': { | ||
| GET: (params: PaginatedRequest<{ fields: string }>) => PaginatedResult<{ | ||
| users: DefaultUserInfo[]; | ||
| }>; | ||
| }; |
There was a problem hiding this comment.
This object is separated into two sections. The first is params: PaginatedRequest<{ fields: string }>, which represents the API query or body (in your case, the query). You should use this instead of your current implementation to stay consistent with the project's codebase.
The second section is the API's return object:
PaginatedResult<{ users: DefaultUserInfo[]; }>;
We must ensure it matches this exactly because the client code expect this specific structure and it's Aline with the project pattern. Breaking changes are not acceptable. For reference, you can look at the implementations in #36916 and #36668.
There was a problem hiding this comment.
understood. I ve updated the endpoint to strictly use the PaginatedRequest and PaginatedResult<{ users: DefaultUserInfo[] }> generics to maintain the exact client server contract. I also restored queryOperations and removed the out-of-scope success additions from the schemas. lastly, I commented out the legacy ddp-client methods that were depending on the old users.list route. let me know it anything else needs adjustment
…gacy SDK dependencies
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/users.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/users.ts:787">
P1: Response schemas became inconsistent with framework helper output (`success` field), so strict AJV validation can reject valid `users.list` and `users.getAvatarSuggestion` responses.</violation>
</file>
<file name="packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts">
<violation number="1" location="packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts:62">
P1: `users.list` legacy helper methods were removed from the runtime `users` API, causing a breaking regression for existing `sdk.users.*` callers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| offset: { type: 'number' }, | ||
| total: { type: 'number' }, | ||
| }, | ||
| required: ['users', 'count', 'offset', 'total'], |
There was a problem hiding this comment.
P1: Response schemas became inconsistent with framework helper output (success field), so strict AJV validation can reject valid users.list and users.getAvatarSuggestion responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/users.ts, line 787:
<comment>Response schemas became inconsistent with framework helper output (`success` field), so strict AJV validation can reject valid `users.list` and `users.getAvatarSuggestion` responses.</comment>
<file context>
@@ -796,20 +783,18 @@ const usersEndpoints = API.v1
- success: { type: 'boolean', enum: [true] },
},
- required: ['users', 'count', 'offset', 'total', 'success'],
+ required: ['users', 'count', 'offset', 'total'],
additionalProperties: false,
}),
</file context>
| get users() { | ||
| const self = this; | ||
| return { | ||
| /* |
There was a problem hiding this comment.
P1: users.list legacy helper methods were removed from the runtime users API, causing a breaking regression for existing sdk.users.* callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts, line 62:
<comment>`users.list` legacy helper methods were removed from the runtime `users` API, causing a breaking regression for existing `sdk.users.*` callers.</comment>
<file context>
@@ -59,6 +59,7 @@ export class RocketchatSdkLegacyImpl extends DDPSDK implements RocketchatSDKLega
get users() {
const self = this;
return {
+ /*
all(fields?: { name: 1; username: 1; status: 1; type: 1 }): Promise<Serialized<OperationResult<'GET', '/v1/users.list'>>> {
return self.rest.get('/v1/users.list', { fields: JSON.stringify(fields) });
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/users.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/users.ts:762">
P1: `users.list` lost its unconditional `view-d-room` permission check; access is now gated only by a conditional setting, allowing broader user enumeration when that setting is off.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@ahmed-n-abdeltwab Removed success from the schema properties and deleted the permissionsRequired array to match the legacy behavior. Tested locally using CURL and it returns the expected PaginatedResult. Attached the 200 OK response. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/users.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/users.ts:762">
P2: `users.list` now has an unconditional `view-d-room` gate, changing prior setting-controlled access behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { | ||
| authRequired: true, | ||
| queryOperations: ['$or', '$and'], | ||
| permissionsRequired: ['view-d-room'], |
There was a problem hiding this comment.
P2: users.list now has an unconditional view-d-room gate, changing prior setting-controlled access behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/users.ts, line 762:
<comment>`users.list` now has an unconditional `view-d-room` gate, changing prior setting-controlled access behavior.</comment>
<file context>
@@ -759,7 +759,7 @@ const usersEndpoints = API.v1
authRequired: true,
queryOperations: ['$or', '$and'],
-
+ permissionsRequired: ['view-d-room'],
query: ajv.compile<PaginatedRequest<{ fields?: string; query?: string }>>({
type: 'object',
</file context>
There was a problem hiding this comment.
You shouldn't have changed this file. You should leave the endpoint definitions in rest-typings if sdkLegacy depends on them, as I previously mentioned in the chat group
3 participants
Proposed changes (including screenshots)
This PR updates the legacy
users.listREST API endpoint to use strict AJV schema validation.ajv.compileon all query params (offset,count,sort,fields,query).responseschema to enforce that the endpoint always returns the required shape (users,count,offset,total).(Note: Tested the UI after the migration, attaching screenshot below showing the Directory still loading users correctly).
Issue(s)
N/A
Steps to test or reproduce
yarn dsv.Further comments
Kept the scope strictly to adding the validation layer to avoid breaking any existing client integrations.
Summary by CodeRabbit