fix: Implement avatar validation for federated users and add correspo…#40047
Open
kushkumarkashyap7280 wants to merge 3 commits intoRocketChat:developfrom
Open
fix: Implement avatar validation for federated users and add correspo…#40047kushkumarkashyap7280 wants to merge 3 commits intoRocketChat:developfrom
kushkumarkashyap7280 wants to merge 3 commits intoRocketChat:developfrom
Conversation
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 |
🦋 Changeset detectedLatest commit: 3b4389a 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 |
Contributor
WalkthroughQueries remote federated profiles on membership joins, compares remote avatar URLs with stored federation avatar state, conditionally downloads/sets or clears local avatars and persists federation avatar URLs. Tests cover matching, changed, profile-failure, absent-avatar, and concurrent-join coalescing. Changes
Sequence DiagramsequenceDiagram
participant Event as Federation Join Event
participant Handler as Member Event Handler
participant SDK as Federation SDK
participant Remote as Remote Avatar Server
participant DB as Database (Users)
participant Avatar as Avatar Operations
Event->>Handler: join event (room_id, state_key, content.avatar_url)
Handler->>SDK: queryProfile(userId)
alt Profile Query Success
SDK-->>Handler: profile { avatar_url }
Handler->>Handler: currentAvatarUrl = profile.avatar_url
else Profile Query Fails
SDK-->>Handler: error
Handler->>Handler: currentAvatarUrl = event.content.avatar_url (fallback)
end
Handler->>DB: load stored federation avatar for user
Handler->>Handler: compare currentAvatarUrl vs storedAvatarUrl
alt URLs Differ
Handler->>Remote: download avatar (MXC/URL)
Remote-->>Handler: avatar bytes
Handler->>Avatar: reset/set local avatar
Avatar->>DB: Users.setAvatarById(...)
Handler->>DB: Users.setFederationAvatarUrlById(currentAvatarUrl)
else URLs Match
Handler-->>Handler: no action (skip download/update)
end
Handler-->>Event: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/events/member.spec.ts (1)
76-89: Add the failed-profile / missing-avatar_urlregression case.The new fallback behavior is only safe when the event explicitly carries an avatar value. A test where
queryProfile()rejects andemitJoinEvent()omitsavatar_urlwould lock in the “unknown != removed” behavior and catch the accidental-reset path.Also applies to: 129-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/member.spec.ts` around lines 76 - 89, Add a regression test where queryProfile() rejects and the join event omits avatar_url to ensure the new fallback only runs when the event explicitly carries an avatar value: update the spec by adding a case that stubs/mocks queryProfile to throw/reject, calls emitJoinEvent without an avatar_url (leave avatarUrl undefined), and asserts the handler does not treat the missing avatar as an explicit removal (i.e., preserves the “unknown != removed” behavior). Locate the helper emitJoinEvent and getHandler in this file and add the new test near the other join tests (also mirror the same change around the referenced block at lines 129-145) so both places cover the rejection + missing avatar_url scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/packages/federation-matrix/src/events/member.ts`:
- Around line 17-24: The function getCurrentFederatedAvatarUrl is collapsing
"avatar removed" and "unknown/missing" into null; change its semantics to
preserve "unknown" by not coercing undefined to null: update the signature to
return Promise<string | null | undefined> (and accept fallbackAvatarUrl: string
| null | undefined), return profile?.avatar_url (which may be undefined) when
queryProfile succeeds, and on catch return the original fallbackAvatarUrl value
unchanged (do not default to null). Update any callers that rely on the old
strict null-only contract to handle undefined as "unknown" and null as explicit
removal.
---
Nitpick comments:
In `@ee/packages/federation-matrix/src/events/member.spec.ts`:
- Around line 76-89: Add a regression test where queryProfile() rejects and the
join event omits avatar_url to ensure the new fallback only runs when the event
explicitly carries an avatar value: update the spec by adding a case that
stubs/mocks queryProfile to throw/reject, calls emitJoinEvent without an
avatar_url (leave avatarUrl undefined), and asserts the handler does not treat
the missing avatar as an explicit removal (i.e., preserves the “unknown !=
removed” behavior). Locate the helper emitJoinEvent and getHandler in this file
and add the new test near the other join tests (also mirror the same change
around the referenced block at lines 129-145) so both places cover the rejection
+ missing avatar_url scenario.
🪄 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: 1c95268b-f47a-4d64-87c7-a3783d64d6ee
📒 Files selected for processing (2)
ee/packages/federation-matrix/src/events/member.spec.tsee/packages/federation-matrix/src/events/member.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (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/src/events/member.spec.tsee/packages/federation-matrix/src/events/member.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/src/events/member.spec.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 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/events/member.spec.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
ee/packages/federation-matrix/src/events/member.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/src/events/member.spec.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
ee/packages/federation-matrix/src/events/member.spec.ts
📚 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/events/member.spec.tsee/packages/federation-matrix/src/events/member.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/src/events/member.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/src/events/member.spec.tsee/packages/federation-matrix/src/events/member.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/events/member.spec.tsee/packages/federation-matrix/src/events/member.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/src/events/member.spec.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:
ee/packages/federation-matrix/src/events/member.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/events/member.ts (1)
45-48: Empty string vs null inconsistency when resetting avatar.When resetting the avatar (user removed their avatar), you persist an empty string
''tofederation.avatarUrl. However, the comparison at line 327 uses|| null, which means an empty string stored value would be coerced tonullon subsequent comparisons. This works but creates a subtle inconsistency: the stored value is''but comparison treats it asnull.Consider persisting
nullexplicitly on reset for consistency, or document this intentional coercion.🔧 Optional: Use null consistently
if (!avatarUrl) { await Upload.resetUserAvatar(user); - await Users.setFederationAvatarUrlById(user._id, ''); + await Users.setFederationAvatarUrlById(user._id, null); return; }This would require updating the model method signature to accept
string | null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/member.ts` around lines 45 - 48, The reset flow writes an empty string to federation.avatarUrl which is later coerced with "|| null" causing inconsistency; change the reset to persist null instead of '' by calling Users.setFederationAvatarUrlById(user._id, null) from the Upload.resetUserAvatar branch, update Users.setFederationAvatarUrlById (and its underlying model/update) to accept string|null and persist null to the DB, and scan other callers of setFederationAvatarUrlById and any consumers that expect '' to handle null appropriately so comparisons remain consistent with the "|| null" logic.ee/packages/federation-matrix/src/events/member.spec.ts (2)
64-69: Re-registering the event handler in every test may cause issues.Calling
member()inbeforeEachregisters a new handler oneventEmitterService.onfor each test. Since the mock doesn't truly accumulate handlers, this works, but thegetHandler()always retrieves the handler from the most recentmember()call. If the real emitter accumulated handlers, this pattern would cause duplicate processing.Consider restructuring to call
member()once in abeforeAllblock, or explicitly verify that only one handler is registered.🧪 Suggested restructure
+ beforeAll(() => { + member(); + }); + beforeEach(() => { jest.clearAllMocks(); - onMembershipEvent.mockClear(); - - member(); });Note: This would require adjusting
getHandler()to useonMembershipEvent.mock.calls[0]which would still work since it's set inbeforeAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/member.spec.ts` around lines 64 - 69, The test currently calls member() inside beforeEach which re-registers the event handler for every test; instead call member() once in a beforeAll (or move the registration to the top of the suite) so the handler is registered only once, and update getHandler() to read from onMembershipEvent.mock.calls[0] (or add an explicit assertion that onMembershipEvent was called exactly once) to ensure you retrieve the single registered handler; this prevents duplicate handler registration and makes tests robust if the real emitter accumulates handlers.
50-193: Consider adding a test for avatar removal (null URL).The test suite covers URL changes and matches well, but doesn't explicitly test the scenario where the remote user removes their avatar (
avatar_urlisnull), which should triggerresetUserAvatarand persist an empty federation URL.🧪 Suggested additional test
it('resets avatar when remote user removes their avatar', async () => { findOneByUsernameMock.mockResolvedValue({ _id: 'u1', username: '@alice:remote.server', name: 'Alice', federation: { avatarUrl: 'mxc://remote.server/old' }, } as any); findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any); findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any); queryProfileMock.mockResolvedValue({ avatar_url: null } as any); await emitJoinEvent({ avatarUrl: null }); expect(resetUserAvatarMock).toHaveBeenCalled(); expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', ''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/member.spec.ts` around lines 50 - 193, Add a new spec that simulates the remote user removing their avatar by having queryProfileMock.resolve to { avatar_url: null } and emitting a join event with avatarUrl: null (use emitJoinEvent), ensuring findOneByUsernameMock, findOneFederatedByMridMock and findOneByRoomIdAndUserIdMock return the existing user/subscription; assert Upload.resetUserAvatar (resetUserAvatarMock) is called and Users.setFederationAvatarUrlById (setFederationAvatarUrlByIdMock) is called with the user id and an empty string to persist avatar removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ee/packages/federation-matrix/src/events/member.spec.ts`:
- Around line 64-69: The test currently calls member() inside beforeEach which
re-registers the event handler for every test; instead call member() once in a
beforeAll (or move the registration to the top of the suite) so the handler is
registered only once, and update getHandler() to read from
onMembershipEvent.mock.calls[0] (or add an explicit assertion that
onMembershipEvent was called exactly once) to ensure you retrieve the single
registered handler; this prevents duplicate handler registration and makes tests
robust if the real emitter accumulates handlers.
- Around line 50-193: Add a new spec that simulates the remote user removing
their avatar by having queryProfileMock.resolve to { avatar_url: null } and
emitting a join event with avatarUrl: null (use emitJoinEvent), ensuring
findOneByUsernameMock, findOneFederatedByMridMock and
findOneByRoomIdAndUserIdMock return the existing user/subscription; assert
Upload.resetUserAvatar (resetUserAvatarMock) is called and
Users.setFederationAvatarUrlById (setFederationAvatarUrlByIdMock) is called with
the user id and an empty string to persist avatar removal.
In `@ee/packages/federation-matrix/src/events/member.ts`:
- Around line 45-48: The reset flow writes an empty string to
federation.avatarUrl which is later coerced with "|| null" causing
inconsistency; change the reset to persist null instead of '' by calling
Users.setFederationAvatarUrlById(user._id, null) from the Upload.resetUserAvatar
branch, update Users.setFederationAvatarUrlById (and its underlying
model/update) to accept string|null and persist null to the DB, and scan other
callers of setFederationAvatarUrlById and any consumers that expect '' to handle
null appropriately so comparisons remain consistent with the "|| null" logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3075967-6ba2-43de-a179-097da902ea8a
📒 Files selected for processing (2)
ee/packages/federation-matrix/src/events/member.spec.tsee/packages/federation-matrix/src/events/member.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (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/src/events/member.tsee/packages/federation-matrix/src/events/member.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/src/events/member.spec.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 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:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-03-15T14:31:23.493Z
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.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-03-15T14:31:28.969Z
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:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 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/events/member.tsee/packages/federation-matrix/src/events/member.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/src/events/member.tsee/packages/federation-matrix/src/events/member.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/src/events/member.tsee/packages/federation-matrix/src/events/member.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/events/member.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/src/events/member.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/src/events/member.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/src/events/member.spec.ts
🔇 Additional comments (6)
ee/packages/federation-matrix/src/events/member.ts (2)
17-40: LGTM on the coalescing implementation.The in-flight lookup cache correctly addresses the previous review concern about duplicate network requests. The
finallyblock properly cleans up only if the current promise matches, preventing removal of a newer in-flight lookup.
325-331: Correct implementation of the undefined-aware comparison.The logic properly distinguishes between:
undefined(unknown/lookup failed with no fallback) → skip updatenull(explicit removal) → triggers avatar reset if different from storedstring(valid URL) → triggers download if different from storedThe
'avatar_url' in contentcheck preserves the "unknown" state when the event payload omits the field entirely.ee/packages/federation-matrix/src/events/member.spec.ts (4)
91-108: Good test coverage for the skip-when-matching scenario.The test correctly verifies that when the remote avatar URL matches the stored federation avatar URL, no download, avatar set, or persistence operations occur.
110-127: Good test for the avatar change scenario.Properly verifies that a changed avatar triggers download and persistence. The PNG magic bytes in the buffer ensure the content-type detection succeeds.
147-164: Critical test for preserving unknown state.This test validates the fix from the previous review: when
queryProfilefails and the event payload has noavatar_urlfield, the system correctly treats this as "unknown" and does not reset or modify the avatar.
166-192: Good test for coalescing concurrent lookups.The test properly validates that concurrent join events for the same user result in only one
queryProfilecall, confirming the in-flight cache works correctly.
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
1 participant
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.
Avatar Validation for Federated Member Events
issue resolved (#39908)
What I changed
I added avatar validation in the Matrix membership join flow so avatar updates only happen when the remote avatar actually changed.
avatar_urlfrom Matrix.federation.avatarUrl.Why this is needed
Before this change, membership events could trigger redundant avatar operations, especially with stale or repeated join events. This update avoids unnecessary work and keeps local avatar state aligned with current remote profile state.
Implementation summary
Files touched
ee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/member.spec.tsTest scenarios covered
Summary by CodeRabbit