fix(federation): error on first user interaction - #41689
Merged
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughThe PR validates federated usernames during registration, centralizes federated-user resolution for membership events, strengthens federated-user result validation, updates direct-message naming fallback, and adds first-contact federation tests. ChangesFederated user flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Synapse
participant MemberEvent
participant getOrCreateFederatedUser
participant createOrUpdateFederatedUser
participant UserDatabase
Synapse->>MemberEvent: Send federated invite
MemberEvent->>getOrCreateFederatedUser: Resolve remote Matrix user
getOrCreateFederatedUser->>UserDatabase: Find existing user
getOrCreateFederatedUser->>createOrUpdateFederatedUser: Create missing remote user
createOrUpdateFederatedUser->>UserDatabase: Upsert complete federated user
UserDatabase-->>MemberEvent: Return validated user
MemberEvent-->>Synapse: Apply room membership state
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41689 +/- ##
===========================================
+ Coverage 69.00% 69.03% +0.03%
===========================================
Files 4224 4227 +3
Lines 166094 166152 +58
Branches 29564 29515 -49
===========================================
+ Hits 114608 114698 +90
+ Misses 46330 46294 -36
- Partials 5156 5160 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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)
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts (1)
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the direct asymmetric error assertion.
Jest 30 accepts asymmetric matchers for
toThrow, soexpect.objectContaining({ cause })is supported at runtime; the double cast only masks the type signature. Use an explicit assertion such as.toMatchObject({ message: 'Error getting or creating federated user@alice:example.com', cause })to avoid the misleading cast.🤖 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 `@ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts` around lines 144 - 150, The test around getOrCreateFederatedUser should use Jest’s direct asymmetric error assertion instead of casting expect.objectContaining. Replace the current toThrow assertion with an explicit toMatchObject check for the expected message and cause, preserving validation of the underlying failure without the double cast.
🤖 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 `@ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts`:
- Around line 144-150: The test around getOrCreateFederatedUser should use
Jest’s direct asymmetric error assertion instead of casting
expect.objectContaining. Replace the current toThrow assertion with an explicit
toMatchObject check for the expected message and cause, preserving validation of
the underlying failure without the double cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65ef18a9-39ec-4a5a-afc4-af381d927f98
📒 Files selected for processing (10)
ee/packages/federation-matrix/docker-compose.test.ymlee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/tests/helper/config.tspackages/core-typings/src/IUser.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tspackages/core-typings/src/IUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tspackages/core-typings/src/IUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tspackages/core-typings/src/IUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tspackages/core-typings/src/IUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
🔇 Additional comments (11)
packages/core-typings/src/IUser.ts (1)
259-259: LGTM!ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts (1)
1-8: LGTM!Also applies to: 19-23, 59-64
ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts (1)
14-20: LGTM!Also applies to: 108-123
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts (1)
19-62: LGTM!ee/packages/federation-matrix/src/api/_matrix/client/account.ts (2)
159-171: LGTM!
96-108: 🔒 Security & Privacy | ⚡ Quick winBroken Authentication (CWE-290): Authentication Bypass by Spoofing
Reachability: External
Validation accepts a foreign server name in the registered user ID.
validateFederatedUsernamechecks only the syntax of the Matrix ID. It accepts any valid domain. Whenbody.usernamealready contains a colon, line 97 keeps that domain. An authenticated application service can therefore register@someone:other-server.exampleon this homeserver, whilecreateOrUpdateFederatedUserstoresfederation.originas the localserverName.That document then becomes the local identity for a remote Matrix ID, because
getOrCreateFederatedUserresolves an existing user by username before any origin check.Confirm whether an application service may register outside the local server name. If not, reject a derived user ID whose domain is not
serverName.🛡️ Proposed domain restriction
const userId: string = withSigil.includes(':') ? withSigil : `${withSigil}:${serverName}`; - if (!validateFederatedUsername(userId)) { + if (!validateFederatedUsername(userId) || !userId.endsWith(`:${serverName}`)) { logger.warn({ msg: 'Malformed user id during AS registration', username: body.username });#!/bin/bash # Verify whether AS registration is expected to be limited to the local server name. fd -t f 'account.ts' -p 'federation-matrix' --exec rg -n 'serverName|isExclusiveNamespace|body.username' {} rg -n -C4 'isExclusiveNamespace' --type=ts fd -t f -e ts -p 'federation-matrix' --exec rg -ln 'v3/register' {}ee/packages/federation-matrix/src/events/member.ts (1)
2-12: LGTM!Also applies to: 168-169, 255-255, 298-298
ee/packages/federation-matrix/tests/helper/config.ts (1)
9-27: LGTM!Also applies to: 71-81, 95-99, 112-116
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts (2)
30-40: LGTM!Also applies to: 42-69, 77-152, 170-186
74-74: 📐 Maintainability & Code QualityNo change needed.
SynapseClientexposes bothcloseandfindRoomMember.ee/packages/federation-matrix/docker-compose.test.yml (1)
44-44: 🩺 Stability & AvailabilityKeep
matrixdotorg/synapse:v1.157.2.
matrixdotorg/synapse:v1.157.2is publishable for Docker pull, so this pin does not need to change to the GHCR repository.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
sampaiodiego
force-pushed
the
fix-federation-first-user-interaction
branch
from
August 5, 2026 17:26
a90e5b0 to
4a1bbcb
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts (1)
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation comments from this spec.
The test names and assertion flow already describe these steps. Remove the explanatory comments to keep the test concise.
As per coding guidelines, “Avoid code comments in the implementation.”
Also applies to: 64-64, 72-72, 103-103, 117-118, 167-169
🤖 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 `@ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts` around lines 21 - 29, Remove the explanatory implementation comments from first-contact.spec.ts, including the highlighted block and the additional comment locations at the referenced lines. Preserve the test names, setup steps, and assertion flow unchanged.Source: Coding guidelines
🤖 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 `@ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts`:
- Around line 21-29: Remove the explanatory implementation comments from
first-contact.spec.ts, including the highlighted block and the additional
comment locations at the referenced lines. Preserve the test names, setup steps,
and assertion flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00bc0b59-9ecb-46d3-b87c-bdc4cbb1b997
📒 Files selected for processing (9)
ee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/tests/helper/config.tspackages/core-typings/src/IUser.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- ee/packages/federation-matrix/src/api/_matrix/client/account.ts
- ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts
- ee/packages/federation-matrix/src/events/member.ts
- packages/core-typings/src/IUser.ts
- ee/packages/federation-matrix/tests/helper/config.ts
- ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts
- ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
- ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
🧠 Learnings (6)
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.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:
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
🔇 Additional comments (1)
ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts (1)
1-20: LGTM!Also applies to: 30-63, 65-71, 73-166, 170-188
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation rationale comments.
The file adds comments at Lines 12-16, Lines 21-27, and Line 61. These comments explain implementation details. Remove them or move the rationale to documentation outside the implementation.
Proposed cleanup
-/** - * `name` is deliberately not required: Rocket.Chat users are valid with a username and no - * display name (`Accounts_RequireNameForSignUp` off), and every consumer falls back to the - * username, so demanding it would drop membership events for nameless local users. - */ type FederatedUser = RequiredField<IUser, 'username'>; const hasUsername = (user: IUser): user is FederatedUser => user.username !== undefined; -/** - * Resolves the local user for a Matrix user id, creating it on first contact. - * - * Only the last branch creates anything: once a user exists locally every later call - * short-circuits on the lookup, which is why a broken creation path only ever surfaces - * on the very first interaction with a given remote user. - */ export async function getOrCreateFederatedUser(userId: string): Promise<FederatedUser> { ... - // awaited so a failure is logged with context and wrapped, instead of escaping the catch below return await createOrUpdateFederatedUser({As per coding guidelines, avoid code comments in the implementation.
Also applies to: 21-27, 61-61
🤖 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 `@ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts` around lines 12 - 16, Remove the implementation rationale comments in getOrCreateFederatedUser.ts at the documented comment blocks around lines 12–16, 21–27, and 61, while leaving the surrounding logic and behavior unchanged.Source: Coding guidelines
🤖 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 `@ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts`:
- Around line 12-16: Remove the implementation rationale comments in
getOrCreateFederatedUser.ts at the documented comment blocks around lines 12–16,
21–27, and 61, while leaving the surrounding logic and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9746f0aa-37e8-4a0a-a1f2-026b137e08ff
📒 Files selected for processing (4)
ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts
- ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
- ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- 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:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts
🧠 Learnings (4)
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.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:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.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:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.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:
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts
🔇 Additional comments (3)
ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts (3)
1-1: LGTM!
17-19: LGTM!Also applies to: 28-32, 62-71
33-59: 🎯 Functional CorrectnessNo change needed. Calling code uses Matrix
UsernameSchema, and the existing-user/AppService paths have valid federated Matrix IDs.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
cardoso
previously approved these changes
Aug 6, 2026
cardoso
left a comment
cardoso
left a comment
Member
There was a problem hiding this comment.
Fix confirmed. Invite works on first interaction.
sampaiodiego
force-pushed
the
fix-federation-first-user-interaction
branch
from
August 12, 2026 18:55
95b0a00 to
d350a0d
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/server/services/room/service.ts (1)
329-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the implementation comment.
The fallback expression is clear without this comment. Remove Line 329 and keep the fallback logic on Line 330.
As per coding guidelines: “Avoid code comments in the implementation.”
🤖 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/server/services/room/service.ts` at line 329, Remove the implementation comment immediately preceding the fallback expression in the relevant room service method, while preserving the existing username fallback logic unchanged.Source: Coding guidelines
🤖 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/server/services/room/service.ts`:
- Line 329: Remove the implementation comment immediately preceding the fallback
expression in the relevant room service method, while preserving the existing
username fallback logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30f4516f-adab-4430-b040-9a765e02c15d
📒 Files selected for processing (10)
apps/meteor/server/services/room/service.tsee/packages/federation-matrix/src/api/_matrix/client/account.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.tsee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.tsee/packages/federation-matrix/tests/end-to-end/first-contact.spec.tsee/packages/federation-matrix/tests/helper/config.tspackages/core-typings/src/IUser.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/core-typings/src/IUser.ts
- ee/packages/federation-matrix/tests/helper/config.ts
- ee/packages/federation-matrix/tests/end-to-end/first-contact.spec.ts
- ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.spec.ts
- ee/packages/federation-matrix/src/api/_matrix/client/account.ts
- ee/packages/federation-matrix/src/events/member.ts
- ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts
- ee/packages/federation-matrix/src/helpers/getOrCreateFederatedUser.ts
- ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (5)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/server/services/room/service.ts
apps/meteor/**
📄 CodeRabbit inference engine (CLAUDE.md)
The main Rocket.Chat Meteor application resides in
apps/meteor/; place its application code there rather than in other monorepo areas.
Files:
apps/meteor/server/services/room/service.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '`#/components/schemas/IUser`'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
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.
📚 Learning: 2026-04-29T20:06:34.862Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40268
File: apps/meteor/client/startup/incomingMessages.ts:21-25
Timestamp: 2026-04-29T20:06:34.862Z
Learning: In `apps/meteor/client/startup/incomingMessages.ts`, the `Messages.state.update` predicate that strips `ignored` from records when `'ignored' in sub` is false (i.e., the subscription update has no `ignored` field) is intentional. Absence of `ignored` in a `subscriptions-changed` event means the user's ignore list is empty/reset, so clearing all existing `ignored` flags on messages for that room is the correct behavior. Do not flag this as an unintentional ignored-state reset on unrelated subscription updates.
Applied to files:
apps/meteor/server/services/room/service.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/server/services/room/service.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/server/services/room/service.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/server/services/room/service.ts
📚 Learning: 2026-04-30T14:57:45.520Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40185
File: packages/apps/deno-runtime/lib/room.ts:45-45
Timestamp: 2026-04-30T14:57:45.520Z
Learning: In `packages/apps/deno-runtime/lib/room.ts`, the `getUsernames()` method intentionally uses `await` before assigning the result to `this._USERNAMES` (typed as `Promise<Array<string>> | undefined`), storing the resolved `Array<string>` rather than the Promise. This is a deliberate backward-compatibility workaround with the existing API and should not be flagged as a type mismatch or cache-contract violation in future reviews.
Applied to files:
apps/meteor/server/services/room/service.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/room/service.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/room/service.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/room/service.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/server/services/room/service.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/server/services/room/service.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/server/services/room/service.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/server/services/room/service.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/server/services/room/service.ts
📚 Learning: 2026-08-05T22:02:59.828Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 41707
File: apps/meteor/server/hooks/messages/processThreads.ts:66-68
Timestamp: 2026-08-05T22:02:59.828Z
Learning: In Rocket.Chat Meteor server code, `callbacks.runAsync` returns its input item rather than the asynchronous callback promise. Callers of `afterReadMessages` must invoke `callbacks.runAsync` without awaiting it, keeping read-receipt I/O off the message-send path; this includes `apps/meteor/server/hooks/messages/processThreads.ts`.
Applied to files:
apps/meteor/server/services/room/service.ts
sampaiodiego
force-pushed
the
fix-federation-first-user-interaction
branch
from
August 13, 2026 15:04
d350a0d to
989d95a
Compare
sampaiodiego
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
ricardogarim
approved these changes
Aug 13, 2026
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
3 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.
Proposed changes (including videos or screenshots)
First federation user interaction was failing due to
createOrUpdateFederatedUserwas returning aprojectionwith less fields than needed. So nowcreateOrUpdateFederatedUsercorrectly returns a fullIUserand new tests were added to validate that.Issue(s)
CORE-2495
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Tests