Release 8.4.2#40529
Open
rocketchat-github-ci wants to merge 10 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 69df443 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
Contributor
|
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 |
Contributor
|
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 autotranslate room-access checks (API + DDP method + tests), fixes users.presence ids parsing/schema and adds E2E tests, and updates users.deactivateIdle to clear login tokens, notify per-user, with corresponding model method and tests; multiple changesets document patch releases. ChangesAutotranslate: API + method + tests
users.presence: schema and tests
users.deactivateIdle: model, API, and tests
Changesets metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…ion (#40527) Co-authored-by: Ricardo Garim <rswarovsky@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/users.ts (1)
1419-1420: ⚡ Quick winRemove inline implementation comments from the tests.
Line 1419 and Line 1433 add explanatory inline comments; please remove them and keep intent in test names/assertions.
As per coding guidelines “Avoid code comments in the implementation”.♻️ Suggested cleanup
- // only rocket.cat is guaranteed to be online; admin may be offline expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat'); @@ - // only rocket.cat is guaranteed to be online; admin may be offline expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat');Also applies to: 1433-1434
🤖 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/meteor/tests/end-to-end/api/users.ts` around lines 1419 - 1420, Remove the inline implementation comments inside the test assertions (the comment preceding the expect(...) that mentions "only rocket.cat is guaranteed to be online; admin may be offline" and the similar comment later) and instead ensure the test name or assertion expresses the intent; update the test description or assertion message for clarity and delete the two inline explanatory comments around the expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat') and the other commented assertion so the implementation contains no inline comments.
🤖 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/meteor/tests/end-to-end/api/users.ts`:
- Around line 1419-1420: Remove the inline implementation comments inside the
test assertions (the comment preceding the expect(...) that mentions "only
rocket.cat is guaranteed to be online; admin may be offline" and the similar
comment later) and instead ensure the test name or assertion expresses the
intent; update the test description or assertion message for clarity and delete
the two inline explanatory comments around the expect(res.body.users.map((u:
IUser) => u._id)).to.include('rocket.cat') and the other commented assertion so
the implementation contains no inline comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11d5097f-678e-48a8-b7ea-22398cd96d22
📒 Files selected for processing (3)
.changeset/fix-presence-comma-ids.mdapps/meteor/tests/end-to-end/api/users.tspackages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-presence-comma-ids.md
📜 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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:
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.tsapps/meteor/tests/end-to-end/api/users.ts
🧠 Learnings (4)
📚 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:
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.tsapps/meteor/tests/end-to-end/api/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:
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.tsapps/meteor/tests/end-to-end/api/users.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.tsapps/meteor/tests/end-to-end/api/users.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts
🪛 OpenGrep (1.20.0)
apps/meteor/tests/end-to-end/api/users.ts
[ERROR] 1424-1426: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🔇 Additional comments (2)
packages/rest-typings/src/v1/users/UsersPresenceParamsGET.ts (1)
13-14: LGTM!apps/meteor/tests/end-to-end/api/users.ts (1)
1395-1407: LGTM!Also applies to: 1409-1418, 1421-1421, 1423-1432, 1435-1435
ricardogarim
approved these changes
May 14, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40529 +/- ##
==========================================
+ Coverage 69.90% 69.91% +0.01%
==========================================
Files 3307 3307
Lines 120581 120581
Branches 21604 21606 +2
==========================================
+ Hits 84288 84304 +16
+ Misses 33001 32979 -22
- Partials 3292 3298 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…type (#40539) Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/autotranslate/server/methods/translateMessage.ts (1)
25-26: 💤 Low valueConsider making the message existence check more explicit.
The optional chaining in
check(message?._id, String)is redundant becausecheckwill throw a Match.Error ifmessageis undefined. For clarity, you could explicitly check message existence first or remove the optional chaining since the check itself validates the value.♻️ Optional refactor for explicitness
- check(message?._id, String); + check(message, Object); + check(message._id, String); check(targetLanguage, String);🤖 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/meteor/app/autotranslate/server/methods/translateMessage.ts` around lines 25 - 26, The optional chaining in the validation is redundant and unclear: make the presence check explicit by first asserting message exists (e.g., throw or use check(message, Object) / check(message, Match.Any) before validating its _id), or simply remove the optional chaining and call check(message._id, String) so the existing check throws if message is undefined; update the validation around the message variable in translateMessage.ts (the check(...) calls) accordingly.
🤖 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/meteor/app/autotranslate/server/methods/translateMessage.ts`:
- Around line 25-26: The optional chaining in the validation is redundant and
unclear: make the presence check explicit by first asserting message exists
(e.g., throw or use check(message, Object) / check(message, Match.Any) before
validating its _id), or simply remove the optional chaining and call
check(message._id, String) so the existing check throws if message is undefined;
update the validation around the message variable in translateMessage.ts (the
check(...) calls) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc79ed78-8a6c-44ab-bb34-8644698dcd14
📒 Files selected for processing (3)
.changeset/neat-trams-juggle.mdapps/meteor/app/autotranslate/server/methods/translateMessage.tsapps/meteor/tests/end-to-end/api/autotranslate.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/neat-trams-juggle.md
📜 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/autotranslate/server/methods/translateMessage.tsapps/meteor/tests/end-to-end/api/autotranslate.ts
🧠 Learnings (3)
📚 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/autotranslate/server/methods/translateMessage.tsapps/meteor/tests/end-to-end/api/autotranslate.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/autotranslate/server/methods/translateMessage.tsapps/meteor/tests/end-to-end/api/autotranslate.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/autotranslate/server/methods/translateMessage.tsapps/meteor/tests/end-to-end/api/autotranslate.ts
🔇 Additional comments (5)
apps/meteor/app/autotranslate/server/methods/translateMessage.ts (3)
3-7: LGTM!
19-24: LGTM!
27-35: LGTM!apps/meteor/tests/end-to-end/api/autotranslate.ts (2)
6-6: LGTM!
379-465: LGTM!
…oint (#40547) Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/users.ts (2)
4816-4837: ⚡ Quick winEnsure cleanup runs even when this test fails mid-way.
idleUserdeletion currently depends on all assertions passing. Usetry/finallyso fixtures are always cleaned up.Suggested reliability refactor
it('should revoke login tokens of deactivated idle users', async () => { - const idleUser = await createUser(); - await request.post(api('roles.addUserToRole')).set(credentials).send({ roleId: testRoleId, username: idleUser.username }).expect(200); - - const idleUserCredentials = await login(idleUser.username, password); - await request.get(api('me')).set(idleUserCredentials).expect(200); - - await updatePermission('edit-other-user-active-status', ['admin']); - await request - .post(api('users.deactivateIdle')) - .set(credentials) - .send({ daysIdle: 0, role: testRoleId }) - .expect(200) - .expect((res: Response) => { - expect(res.body).to.have.property('success', true); - expect(res.body).to.have.property('count').that.is.greaterThan(0); - }); - - await request.get(api('me')).set(idleUserCredentials).expect(401); - - await deleteUser(idleUser); + const idleUser = await createUser(); + try { + await request.post(api('roles.addUserToRole')).set(credentials).send({ roleId: testRoleId, username: idleUser.username }).expect(200); + + const idleUserCredentials = await login(idleUser.username, password); + await request.get(api('me')).set(idleUserCredentials).expect(200); + + await updatePermission('edit-other-user-active-status', ['admin']); + await request + .post(api('users.deactivateIdle')) + .set(credentials) + .send({ daysIdle: 0, role: testRoleId }) + .expect(200) + .expect((res: Response) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('count').that.is.greaterThan(0); + }); + + await request.get(api('me')).set(idleUserCredentials).expect(401); + } finally { + await deleteUser(idleUser); + } });🤖 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/meteor/tests/end-to-end/api/users.ts` around lines 4816 - 4837, The test 'should revoke login tokens of deactivated idle users' currently only calls deleteUser(idleUser) at the end, so if an assertion throws the idleUser fixture is not cleaned up; refactor the test to declare idleUser above the try block, execute the existing test steps inside a try, and move the deleteUser(idleUser) call into a finally block (or guard with if (idleUser) deleteUser(idleUser)) so cleanup always runs; preserve existing calls to createUser(), login(), updatePermission(), request.post(api('users.deactivateIdle')) and request.get(api('me')) but ensure deleteUser is executed from the finally.
1419-1419: ⚡ Quick winRemove inline comments from the new tests.
Please drop these inline notes and encode intent in the test name/assertions instead.
As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 1433-1433
🤖 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/meteor/tests/end-to-end/api/users.ts` at line 1419, Remove the inline comment(s) that read like notes (e.g., "// only rocket.cat is guaranteed to be online; admin may be offline") from the tests in apps/meteor/tests/end-to-end/api/users.ts and instead encode that intent in the test names and assertions: rename the affected test(s) so the description states the expectation (for example "returns only rocket.cat as online when admin is offline") and add explicit assertions that verify which users are online/offline (use assertion messages where helpful). Locate the tests by the surrounding test functions (e.g., the it()/test() blocks around the commented lines) and update their names and assertions accordingly; remove the comment lines after you have made the intent explicit in the test name/assertions.
🤖 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/meteor/tests/end-to-end/api/users.ts`:
- Around line 1423-1435: The test "should return presence for repeated ids
params" currently uses `rocket.cat` as the first `ids` value so the test passes
even if the server ignores repeated params; change the first `ids` value to a
non-existent id (e.g., "not.real.user") and keep `rocket.cat` as the second
param in the `.query(...)` call to ensure repeated-param parsing is exercised,
then update the assertions on `res.body.users` (the mapping used in the existing
expects) to assert that `rocket.cat` is present and that the non-existent id is
not returned (or that only `rocket.cat` is present) to prove the second param is
parsed.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/users.ts`:
- Around line 4816-4837: The test 'should revoke login tokens of deactivated
idle users' currently only calls deleteUser(idleUser) at the end, so if an
assertion throws the idleUser fixture is not cleaned up; refactor the test to
declare idleUser above the try block, execute the existing test steps inside a
try, and move the deleteUser(idleUser) call into a finally block (or guard with
if (idleUser) deleteUser(idleUser)) so cleanup always runs; preserve existing
calls to createUser(), login(), updatePermission(),
request.post(api('users.deactivateIdle')) and request.get(api('me')) but ensure
deleteUser is executed from the finally.
- Line 1419: Remove the inline comment(s) that read like notes (e.g., "// only
rocket.cat is guaranteed to be online; admin may be offline") from the tests in
apps/meteor/tests/end-to-end/api/users.ts and instead encode that intent in the
test names and assertions: rename the affected test(s) so the description states
the expectation (for example "returns only rocket.cat as online when admin is
offline") and add explicit assertions that verify which users are online/offline
(use assertion messages where helpful). Locate the tests by the surrounding test
functions (e.g., the it()/test() blocks around the commented lines) and update
their names and assertions accordingly; remove the comment lines after you have
made the intent explicit in the test name/assertions.
🪄 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: CHILL
Plan: Pro
Run ID: 525c5f1f-5281-4205-be47-02f79da1d5e5
📒 Files selected for processing (5)
.changeset/good-rules-lie.mdapps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/good-rules-lie.md
📜 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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
🧰 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.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.tsapps/meteor/tests/end-to-end/api/users.ts
🧠 Learnings (7)
📚 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.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.tsapps/meteor/tests/end-to-end/api/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.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.tsapps/meteor/tests/end-to-end/api/users.ts
📚 Learning: 2026-03-15T14:31:25.380Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:25.380Z
Learning: Do not flag this type/schema misalignment in the OpenAPI/migration review for apps/meteor/app/api/server/v1/users.ts. The UserCreateParamsPOST type intentionally uses non-optional fields: fields: string and settings?: IUserSettings without an AJV schema entry, carried over from the original rest-typings (PR `#39647`). Treat this as a known pre-existing divergence and document it as a separate follow-up fix; do not block or mark it as a review issue during the migration.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-16T23:33:11.443Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:11.443Z
Learning: In rockets OpenAPI/AJV migration reviews for RocketChat/Rocket.Chat, when reviewing migrations that involve apps/meteor/app/api/server/v1/users.ts, do not require or flag a missing query AJV schema for the fields consumed by parseJsonQuery() (i.e., fields, sort, query) as part of this endpoint's migration PR. The addition of global query-param schemas for parseJsonQuery() usage is a cross-cutting concern and out of scope for individual endpoint migrations. Only flag violations related to the specific scope of the migration, not the absence of a query schema for parseJsonQuery() in this file.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/api/server/v1/users.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.tsapps/meteor/tests/end-to-end/api/users.ts
🪛 OpenGrep (1.20.0)
apps/meteor/tests/end-to-end/api/users.ts
[ERROR] 1424-1426: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🔇 Additional comments (4)
packages/models/src/models/Users.ts (2)
953-963: LGTM!
2965-2976: LGTM!packages/model-typings/src/models/IUsersModel.ts (1)
396-396: LGTM!apps/meteor/tests/end-to-end/api/users.ts (1)
1395-1407: LGTM!
Comment on lines
+1423
to
+1435
| it('should return presence for repeated ids params', async () => { | ||
| const res = await request | ||
| .get(api('users.presence')) | ||
| .query(`ids=rocket.cat&ids=${credentials['X-User-Id']}`) | ||
| .set(credentials) | ||
| .expect('Content-Type', 'application/json') | ||
| .expect(200); | ||
|
|
||
| expect(res.body).to.have.property('success', true); | ||
| expect(res.body).to.have.property('full', false); | ||
| // only rocket.cat is guaranteed to be online; admin may be offline | ||
| expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Repeated-ids test can pass even if only the first ids value is parsed.
Current assertion still passes when the backend ignores repeated params, because rocket.cat is the first value and also the only asserted one. Make the first id non-existent and keep rocket.cat second to prove repeated-param parsing works.
Suggested test hardening
it('should return presence for repeated ids params', async () => {
const res = await request
.get(api('users.presence'))
- .query(`ids=rocket.cat&ids=${credentials['X-User-Id']}`)
+ .query('ids=non-existent-user-id&ids=rocket.cat')
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200);
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('full', false);
- // only rocket.cat is guaranteed to be online; admin may be offline
expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat');
});🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 1424-1426: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 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/meteor/tests/end-to-end/api/users.ts` around lines 1423 - 1435, The test
"should return presence for repeated ids params" currently uses `rocket.cat` as
the first `ids` value so the test passes even if the server ignores repeated
params; change the first `ids` value to a non-existent id (e.g.,
"not.real.user") and keep `rocket.cat` as the second param in the `.query(...)`
call to ensure repeated-param parsing is exercised, then update the assertions
on `res.body.users` (the mapping used in the existing expects) to assert that
`rocket.cat` is present and that the non-existent id is not returned (or that
only `rocket.cat` is present) to prove the second param is parsed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 13 files
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:551">
P2: This read-then-update-then-notify sequence can broadcast incorrect user state for users that no longer match the update filter at update time.</violation>
</file>
<file name=".changeset/good-rules-lie.md">
<violation number="1" location=".changeset/good-rules-lie.md:7">
P2: The changeset summary appears unrelated to this PR’s actual fixes, so release notes for this patch will be misleading.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/users.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/users.ts:4834">
P2: This test does not prove login tokens were revoked; a 401 can happen just because the user was deactivated.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| lastLoggedIn.setDate(lastLoggedIn.getDate() - daysIdle); | ||
|
|
||
| // since we're deactiving users that are not logged in, there is no need to send data through WS | ||
| const ids = await Users.findActiveNotLoggedInAfterWithRole(lastLoggedIn, role, { projection: { _id: 1 } }) |
Contributor
There was a problem hiding this comment.
P2: This read-then-update-then-notify sequence can broadcast incorrect user state for users that no longer match the update filter at update time.
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 551:
<comment>This read-then-update-then-notify sequence can broadcast incorrect user state for users that no longer match the update filter at update time.</comment>
<file context>
@@ -548,9 +548,20 @@ API.v1.post(
lastLoggedIn.setDate(lastLoggedIn.getDate() - daysIdle);
- // since we're deactiving users that are not logged in, there is no need to send data through WS
+ const ids = await Users.findActiveNotLoggedInAfterWithRole(lastLoggedIn, role, { projection: { _id: 1 } })
+ .map(({ _id }: { _id: string }) => _id)
+ .toArray();
</file context>
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle |
Contributor
There was a problem hiding this comment.
P2: The changeset summary appears unrelated to this PR’s actual fixes, so release notes for this patch will be misleading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/good-rules-lie.md, line 7:
<comment>The changeset summary appears unrelated to this PR’s actual fixes, so release notes for this patch will be misleading.</comment>
<file context>
@@ -0,0 +1,7 @@
+'@rocket.chat/meteor': patch
+---
+
+Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle
</file context>
Suggested change
| Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle | |
| Fixes users.presence handling for multiple IDs and strengthens autotranslate input validation and room-access enforcement. |
| expect(res.body).to.have.property('count').that.is.greaterThan(0); | ||
| }); | ||
|
|
||
| await request.get(api('me')).set(idleUserCredentials).expect(401); |
Contributor
There was a problem hiding this comment.
P2: This test does not prove login tokens were revoked; a 401 can happen just because the user was deactivated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/users.ts, line 4834:
<comment>This test does not prove login tokens were revoked; a 401 can happen just because the user was deactivated.</comment>
<file context>
@@ -4770,6 +4812,29 @@ describe('[Users]', () => {
+ expect(res.body).to.have.property('count').that.is.greaterThan(0);
+ });
+
+ await request.get(api('me')).set(idleUserCredentials).expect(401);
+
+ await deleteUser(idleUser);
</file context>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Yasmim Nagat <117310290+yasnagat@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
You can see below a preview of the release change log:
8.4.2
Engine versions
22.22.22.3.18.01.62.0Patch Changes
Bump @rocket.chat/meteor version.
(#40527 by @dionisio-bot) Fixes the
users.presenceendpoint returning an empty array when called with multiple comma-separated IDs, caused byajvQuerycoercing the string into a single-element array after the OpenAPI migration(#40559 by @dionisio-bot) Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle
(#40539 by @dionisio-bot) Ensures the Meteor method for translateMessage validates access and types
(#40547 by @dionisio-bot) Ensures the autotranslate.translateMessage endpoint checks for room access
Updated dependencies [f422eb6, 3a3f0e1]: