feat(api): migrate users.getPreferences to OpenAPI pattern#38325
feat(api): migrate users.getPreferences to OpenAPI pattern#38325Mohamed-Sobea wants to merge 11 commits intoRocketChat:developfrom
Conversation
🦋 Changeset detectedLatest commit: 0b372c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughAdded AJV-based 200/400/401 response validation and public typing for the users.getPreferences endpoint; moved handler logic into an action that returns standardized responses and throws Meteor errors on missing data; exported Changes
Sequence Diagram(s)(Skipped — changes are localized to a single endpoint and do not introduce a multi-component sequential flow warranting a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 CodeRabbit can generate a title for your PR based on the changes.Add |
hi @Mohamed-Sobea , Please include the Swagger documentation and clean up the old code in rest-typings so we don't lose track of it across PRs. It’s also a good idea to finish the API in a single PR to keep things clean and concise. Also I noticed there are a few missing spaces and linting issues that need fixing |
|
Hi @ahmed-n-abdeltwab , thank you for the guidance and the detailed feedback! I have implemented the requested changes and consolidated everything into this PR: Swagger Documentation: I’ve added the response block to the route definition and verified that the documentation is correctly generated at the /api-docs/ route. I've also updated the PR description with a verification screenshot. Typing Cleanup: I purged the legacy manual definitions from packages/rest-typings as the route now uses ExtractRoutesFromAPI for a single source of truth. Linting & Formatting: I resolved the spacing issues and other linting errors by running yarn lint -- --fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 5-7: There is a duplicate import of the symbol "ajv" causing a
TypeScript duplicate identifier error; remove the second/duplicate import so
"ajv" is only imported once (keep the original import line where "ajv" appears
alongside validateBadRequestErrorResponse and validateUnauthorizedErrorResponse
and delete the later redundant import statement or entry that re-exports "ajv"),
then run a build to confirm the compilation error is resolved.
- Around line 875-879: Remove the duplicated UsersEndpoints/type export and
module augmentation added here and instead merge its definitions into the
existing augmentation: delete the new declaration of UsersEndpoints and the
declare module '@rocket.chat/rest-typings' block in this file, then update the
original augmentation (the existing declare module block that defines Endpoints)
to include the additional endpoint routes by combining
ExtractRoutesFromAPI<typeof usersEndpoints> with any other relevant endpoint
types (e.g., ExtractRoutesFromAPI<typeof usersGetPreferencesEndpoint>), and use
the original pattern interface Endpoints extends UsersEndpoints {} (or extend
the combined type via an interface) so the module augmentation remains
consistent and non-duplicative.
- Around line 851-873: The issue is a duplicate const declaration of
usersEndpoints (first used for the users.createToken endpoint and again for
users.getPreferences), so rename the second declaration (e.g.,
usersEndpointsGetPreferences) or refactor to chain the API.v1.get calls into a
single variable instead of redeclaring; then update the exported type/combined
endpoints (the export that currently references usersEndpoints) to include both
endpoint variables or the single chained variable so both users.createToken and
users.getPreferences are included. Ensure you update all references to
usersEndpoints accordingly (including the combined endpoints/type export).
|
Hey @Mohamed-Sobea , I noticed CodeRabbit flagged a few items on this PR. I know AI bots can sometimes be hit-or-miss, but it’s usually worth double-checking their logic before dismissing them. If you ever feel like the bot is 'hallucinating' or being too picky, try pushing back! You can actually reply to its comment with something like, 'Are you sure?' or 'Can you explain why this is a problem?' Usually, it will either admit it was wrong or give you a really clear explanation that helps the fix make sense. Could you take these issue? |
Nice work on the linting! Just wanted to share the workflow I usually use to double-check everything. it might save you some time: For formatting and fixing issues, I usually run: To make sure the logic is actually working, I run the specific API test like this: Finally, I always use ripgrep to make sure the endpoint is registered correctly and the paths match between the frontend and the SDK. I usually run something like this: Or for simplicity, you can just use your editor to search for things like |
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38325 +/- ##
===========================================
- Coverage 70.86% 70.86% -0.01%
===========================================
Files 3160 3160
Lines 109775 109775
Branches 19709 19715 +6
===========================================
- Hits 77795 77793 -2
- Misses 29957 29965 +8
+ Partials 2023 2017 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
bot
added
the
type: feature
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 912-920: The current change makes users.getPreferences return an
error when a valid user lacks a settings object; restore legacy behavior by
treating a missing settings as empty preferences: locate the
users.getPreferences handler (the block that checks user?.settings and calls
API.v1.success) and change the guard to check for a valid user instead of
user?.settings, build preferences from (user.settings?.preferences || {}), set
preferences.language = user.language, and return API.v1.success({ preferences })
so users without settings still get a preferences response rather than an error.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c989b8ca-efde-4253-9501-60f754ee4382
📒 Files selected for processing (2)
.vscode/settings.jsonapps/meteor/app/api/server/v1/users.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
📜 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 (14)
📓 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.
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 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: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
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-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the permission model distinguishes between token pass-through and true impersonation: `server-endpoints.call` is required for all endpoint access, while `server-endpoints.impersonate` is only required when `info.user.id` is provided without `info.user.token` (lines 48-53), meaning the bridge needs to mint a token. When both user ID and token are provided, it's considered legitimate credential usage, not impersonation.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the impersonation permission (`server-endpoints.impersonate`) is enforced at lines 48-49 specifically when `info.user` is provided without `info.user.token`, which represents the impersonation scenario where a token needs to be minted. Token pass-through (when both user ID and token are provided) doesn't require the impersonate permission since the app already possesses a valid token.
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: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
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-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 (4)
apps/meteor/app/api/server/v1/users.ts (4)
177-179: Looks good — no behavioral drift in the 2FA options normalization.This formatting-only update keeps the same runtime behavior.
542-545: LGTM on the pagination pipeline formatting update.No functional change introduced here.
758-772: Good addition of a dedicated AJV validator for the 200 response.This improves route contract clarity and Swagger/OpenAPI generation readiness.
923-923: Type export + module augmentation wiring is clean.
UsersEndpointsextraction andEndpointsaugmentation are aligned and scoped to the migrated endpoint chain.Also applies to: 1581-1583
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
could you reviews these issues
| async function action() { | ||
| const user = await Users.findOneById(this.userId); | ||
| if (user?.settings) { | ||
| const { preferences = {} } = user?.settings; | ||
| preferences.language = user?.language; | ||
|
|
||
| return API.v1.success({ | ||
| preferences, | ||
| }); | ||
| if (!user) { | ||
| throw new Meteor.Error('error-invalid-user', 'Invalid user'); | ||
| } | ||
| return API.v1.failure(i18n.t('Accounts_Default_User_Preferences_not_available').toUpperCase()); | ||
|
|
||
| const preferences = { | ||
| ...(user.settings?.preferences ?? {}), | ||
| language: user.language, | ||
| }; | ||
|
|
||
| return API.v1.success({ preferences }); |
There was a problem hiding this comment.
we shouldn't change the action function code
| type UsersEndpoints = ExtractRoutesFromAPI<typeof usersEndpoints>; | ||
|
|
||
| declare module '@rocket.chat/rest-typings' { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface | ||
| interface Endpoints extends UsersEndpoints {} |
There was a problem hiding this comment.
this's was right, you could return it as it's
| // eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface | ||
| interface Endpoints extends UsersEndpoints {} | ||
| interface Endpoints extends UsersEndpoints { } | ||
| } |
There was a problem hiding this comment.
we need to remove the users.getPreferences from the rest-typings
There was a problem hiding this comment.
its my mistake I removed it on my machine but totally forgot to git add it before committing :)
| "tshow" | ||
| ] | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
.vscode settings is out of the scoop for this PR could you fix this
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@rocket.chat/meteor': patch | |||
There was a problem hiding this comment.
we use minor instead of patch anymore for this project
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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:909">
P2: `users.getPreferences` regresses null-safe behavior by failing when `settings` is missing and may throw if `settings.preferences` is `null`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| async function action() { | ||
| const user = await Users.findOneById(this.userId); | ||
|
|
||
| if (user?.settings) { |
There was a problem hiding this comment.
P2: users.getPreferences regresses null-safe behavior by failing when settings is missing and may throw if settings.preferences is null.
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 909:
<comment>`users.getPreferences` regresses null-safe behavior by failing when `settings` is missing and may throw if `settings.preferences` is `null`.</comment>
<file context>
@@ -909,21 +906,19 @@ const usersEndpoints = API.v1
- if (!user) {
- throw new Meteor.Error('error-invalid-user', 'Invalid user');
- }
+ if (user?.settings) {
+ const { preferences = {} } = user.settings;
+ preferences.language = user.language;
</file context>
5 participants
Description:
This PR completes the migration of the users.getPreferences endpoint to the new OpenAPI + AJV pattern. It introduces robust schema validation for responses, improves type safety by utilizing ExtractRoutesFromAPI, and ensures the endpoint is fully documented and verified within the Swagger UI.
Key Changes:
Full Migration: Moved users.getPreferences to the new API pattern with compiled AJV validation.
Swagger Documentation: Included the response block in the route definition to enable automatic documentation generation in the API Explorer.
Typing Cleanup: Purged legacy manual interfaces and endpoint types from packages/rest-typings to ensure a single source of truth via ExtractRoutesFromAPI.
Linting & Formatting: Resolved all spacing and formatting issues by running yarn lint -- --fix.
Testing & Verification:
Swagger UI: Verified at /api-docs/. The schema correctly reflects the validated response structure.
Functional Test: Successfully authorized and executed the endpoint in the Swagger explorer, receiving a 200 OK with the expected user preferences.
E2E Tests: Ran yarn testapi --grep users.getPreferences in apps/meteor; all assertions passed.
Successful API Response (200 OK):
Ready for final review and merge.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
COMM-144