test(federation): add tests for re-inviting RC users after leaving or being kicked#39990
Merged
dionisio-bot[bot] merged 3 commits intorelease-8.3.0from Mar 31, 2026
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent 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). (3)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
🔇 Additional comments (1)
WalkthroughAdds an end-to-end test that verifies re-inviting a federated Rocket.Chat Matrix user (after they were kicked or left) yields a new RC subscription with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
sampaiodiego
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ee/packages/federation-matrix/tests/end-to-end/room.spec.ts">
<violation number="1" location="ee/packages/federation-matrix/tests/end-to-end/room.spec.ts:1837">
P2: Avoid fixed 2s sleeps to wait for join/kick/leave propagation; the timing can vary and make these tests flaky. Use the existing retry helper to poll for the expected membership state before continuing.
(Based on your team's feedback about concurrency/race-condition changes in async test flows.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (4)
1854-1862: Wrap subscription check inretry()for eventual consistency.The re-invite subscription check fetches subscriptions once and expects the invitation to exist immediately. Given federation event propagation can have variable latency, this pattern could be flaky. The existing test at line 1715 demonstrates using
retry()for this purpose.♻️ Suggested improvement
-// Step 7: Validate the invite was created successfully on the RC side -const subscriptions = await getSubscriptions(rc1AdminRequestConfig); -const pendingInvitation = subscriptions.update.find( - (subscription) => subscription.status === 'INVITED' && subscription.fname?.includes(channelName), -); - -expect(pendingInvitation).not.toBeUndefined(); -expect(pendingInvitation).toHaveProperty('rid'); -expect(pendingInvitation).toHaveProperty('fname'); -expect(pendingInvitation!.fname).toContain(channelName); +// Step 7: Validate the invite was created successfully on the RC side +await retry( + 'Waiting for re-invite to appear on RC side', + async () => { + const subscriptions = await getSubscriptions(rc1AdminRequestConfig); + const pendingInvitation = subscriptions.update.find( + (subscription) => subscription.status === 'INVITED' && subscription.fname?.includes(channelName), + ); + + expect(pendingInvitation).not.toBeUndefined(); + expect(pendingInvitation).toHaveProperty('rid'); + expect(pendingInvitation).toHaveProperty('fname'); + expect(pendingInvitation!.fname).toContain(channelName); + }, + { delayMs: 500 }, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` around lines 1854 - 1862, The subscription existence/assertions after re-invite should be wrapped in the existing retry() helper to handle eventual consistency: replace the single-shot call to getSubscriptions(rc1AdminRequestConfig) and direct expects on pendingInvitation with a retry loop that calls getSubscriptions(rc1AdminRequestConfig) until a subscription with status 'INVITED' and fname containing channelName is found, then assert it has properties 'rid' and 'fname' and that fname contains channelName; reference the existing getSubscriptions function and the pendingInvitation check so the logic and assertions move inside the retry callback.
1911-1919: Wrap the re-invite subscription check inretry()for consistency.Same recommendation as the "kicked" scenario - use
retry()to handle eventual consistency when checking for the new INVITED subscription after re-invite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` around lines 1911 - 1919, The re-invite subscription assertion should be wrapped with the retry helper to handle eventual consistency: call retry() around the getSubscriptions(rc1AdminRequestConfig) + search for the INVITED subscription (the logic that sets pendingInvitation) and move the expect assertions inside that retry block so the test repeatedly fetches subscriptions until a subscription with status 'INVITED' and fname including channelName is found; update the code that defines pendingInvitation and the subsequent expect(...) checks to run inside retry() (referencing getSubscriptions, rc1AdminRequestConfig, pendingInvitation and retry).
1837-1843: Consider usingretry()helper instead of fixed timeouts for event propagation.The hardcoded
setTimeoutdelays (2000ms) for waiting on join/kick/leave propagation could cause test flakiness if federation takes longer than expected. Other tests in this file use theretry()helper for eventual consistency (e.g., line 1715). Consider wrapping the subscription checks in retry logic instead of relying on fixed delays.♻️ Suggested refactor pattern
-// Wait for join to propagate -await new Promise((resolve) => setTimeout(resolve, 2000)); - -// Step 4: Kick RC user from Synapse -await hs1AdminApp.matrixClient.kick(matrixRoomId, federationConfig.rc1.adminMatrixUserId, 'Kicked for re-invite test'); - -// Wait for kick to propagate -await new Promise((resolve) => setTimeout(resolve, 2000)); +// Step 4: Kick RC user from Synapse +await hs1AdminApp.matrixClient.kick(matrixRoomId, federationConfig.rc1.adminMatrixUserId, 'Kicked for re-invite test'); + +// Wait for kick to propagate - verify subscription is removed +await retry( + 'Waiting for kick to propagate', + async () => { + const subs = await getSubscriptions(rc1AdminRequestConfig); + const sub = subs.update.find((s) => s.fname?.includes(channelName)); + expect(sub).toBeFalsy(); + }, + { delayMs: 500 }, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` around lines 1837 - 1843, Replace the two fixed 2000ms setTimeout waits around the kick call with the existing retry() helper to wait for the kick propagation: call hs1AdminApp.matrixClient.kick(matrixRoomId, federationConfig.rc1.adminMatrixUserId, ...) as before, then use retry(async () => { /* assert the rc client's membership state or that the room subscription/leave event is observed for federationConfig.rc1.adminMatrixUserId or check a helper like getRoomMembers(matrixRoomId) contains expected state */ }, { timeout: <sensible-ms> }) to poll until the expected state is seen; remove the await new Promise(...) calls and mirror the retry usage pattern already present in this file (retry()).
1889-1900: Same flakiness concerns with hardcoded timeouts in the "left" scenario.This test block has the same pattern of fixed
setTimeoutdelays for join/leave propagation. Apply the sameretry()pattern here for consistency and reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` around lines 1889 - 1900, Replace the fixed setTimeout waits around the "left" scenario with the existing retry() pattern used elsewhere: instead of awaiting new Promise(resolve => setTimeout(...)) before and after the rc1AdminRequestConfig.request.post(api('rooms.leave')) call, wrap the post and subsequent verification in retry() calls that poll the room/membership state until the join has propagated and then until the leave has been observed; use the same retry helper used in other tests and check the room membership via the Matrix API (e.g., querying room state or membership) to assert the user is no longer present after rc1AdminRequestConfig.request.post(api('rooms.leave')).
🤖 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/tests/end-to-end/room.spec.ts`:
- Around line 1854-1862: The subscription existence/assertions after re-invite
should be wrapped in the existing retry() helper to handle eventual consistency:
replace the single-shot call to getSubscriptions(rc1AdminRequestConfig) and
direct expects on pendingInvitation with a retry loop that calls
getSubscriptions(rc1AdminRequestConfig) until a subscription with status
'INVITED' and fname containing channelName is found, then assert it has
properties 'rid' and 'fname' and that fname contains channelName; reference the
existing getSubscriptions function and the pendingInvitation check so the logic
and assertions move inside the retry callback.
- Around line 1911-1919: The re-invite subscription assertion should be wrapped
with the retry helper to handle eventual consistency: call retry() around the
getSubscriptions(rc1AdminRequestConfig) + search for the INVITED subscription
(the logic that sets pendingInvitation) and move the expect assertions inside
that retry block so the test repeatedly fetches subscriptions until a
subscription with status 'INVITED' and fname including channelName is found;
update the code that defines pendingInvitation and the subsequent expect(...)
checks to run inside retry() (referencing getSubscriptions,
rc1AdminRequestConfig, pendingInvitation and retry).
- Around line 1837-1843: Replace the two fixed 2000ms setTimeout waits around
the kick call with the existing retry() helper to wait for the kick propagation:
call hs1AdminApp.matrixClient.kick(matrixRoomId,
federationConfig.rc1.adminMatrixUserId, ...) as before, then use retry(async ()
=> { /* assert the rc client's membership state or that the room
subscription/leave event is observed for federationConfig.rc1.adminMatrixUserId
or check a helper like getRoomMembers(matrixRoomId) contains expected state */
}, { timeout: <sensible-ms> }) to poll until the expected state is seen; remove
the await new Promise(...) calls and mirror the retry usage pattern already
present in this file (retry()).
- Around line 1889-1900: Replace the fixed setTimeout waits around the "left"
scenario with the existing retry() pattern used elsewhere: instead of awaiting
new Promise(resolve => setTimeout(...)) before and after the
rc1AdminRequestConfig.request.post(api('rooms.leave')) call, wrap the post and
subsequent verification in retry() calls that poll the room/membership state
until the join has propagated and then until the leave has been observed; use
the same retry helper used in other tests and check the room membership via the
Matrix API (e.g., querying room state or membership) to assert the user is no
longer present after rc1AdminRequestConfig.request.post(api('rooms.leave')).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93126b59-6b4b-4ff9-99ef-defac6dcde56
📒 Files selected for processing (1)
ee/packages/federation-matrix/tests/end-to-end/room.spec.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- 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/room.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/room.spec.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
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.
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.
📚 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/room.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/tests/end-to-end/room.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.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/tests/end-to-end/room.spec.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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/room.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/room.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/room.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/room.spec.ts
🔇 Additional comments (1)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
1812-1922: Good test coverage for the re-invite scenario described in FGA-57.The tests correctly exercise the key scenario: invite → accept → kick/leave → intervening message → re-invite. The assertions check for the presence of a new
INVITEDsubscription with the correctridandfname, which aligns with the PR objective of validating that re-invites are properly propagated after removal events.
sampaiodiego
force-pushed
the
fix-invite-user-after-leave
branch
from
ada4e8a to
82398a8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (2)
1900-1900: Add eslint-disable comment for non-null assertion consistency.Same as above - add the eslint-disable comment for consistency with the rest of the file.
Suggested fix
expect(pendingInvitation).not.toBeUndefined(); + // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` const rid = pendingInvitation!.rid!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` at line 1900, The line using non-null assertions on pendingInvitation and rid should include the same eslint-disable directive used elsewhere; add an eslint-disable comment (e.g., an eslint-disable-next-line for `@typescript-eslint/no-non-null-assertion`) immediately above the line that declares const rid = pendingInvitation!.rid! so the non-null assertion is allowed consistently with other occurrences in the file.
1833-1833: Add eslint-disable comment for non-null assertion consistency.Similar non-null assertions elsewhere in the file (e.g., line 1707-1708) include an eslint-disable comment. Add it here for consistency and to prevent potential lint warnings.
Suggested fix
expect(pendingInvitation).not.toBeUndefined(); + // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` const rid = pendingInvitation!.rid!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/tests/end-to-end/room.spec.ts` at line 1833, The line assigning const rid = pendingInvitation!.rid! uses non-null assertions without the eslint exemption; add the same eslint-disable comment used elsewhere (e.g., // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion`) immediately above this assignment to match the other occurrences and suppress the lint warning for pendingInvitation and rid non-null assertions.
🤖 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/tests/end-to-end/room.spec.ts`:
- Line 1900: The line using non-null assertions on pendingInvitation and rid
should include the same eslint-disable directive used elsewhere; add an
eslint-disable comment (e.g., an eslint-disable-next-line for
`@typescript-eslint/no-non-null-assertion`) immediately above the line that
declares const rid = pendingInvitation!.rid! so the non-null assertion is
allowed consistently with other occurrences in the file.
- Line 1833: The line assigning const rid = pendingInvitation!.rid! uses
non-null assertions without the eslint exemption; add the same eslint-disable
comment used elsewhere (e.g., // eslint-disable-next-line
`@typescript-eslint/no-non-null-assertion`) immediately above this assignment to
match the other occurrences and suppress the lint warning for pendingInvitation
and rid non-null assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77ce66e5-72e9-4484-8386-fdd1b6895c79
📒 Files selected for processing (1)
ee/packages/federation-matrix/tests/end-to-end/room.spec.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- 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/room.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/room.spec.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
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.
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: 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.
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.
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.
📚 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/room.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/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.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/tests/end-to-end/room.spec.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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 : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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/tests/end-to-end/room.spec.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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/room.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/room.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/room.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/room.spec.ts
🔇 Additional comments (1)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
1812-1950: Well-structured tests that accurately reproduce the bug scenario.These tests correctly implement the reproduction steps from issue FGA-57:
- Invite → Accept → Join propagation wait
- Kick/Leave → Leave propagation wait
- Send intervening message (critical step)
- Re-invite → Validate new INVITED subscription
The use of
retryhelper with appropriate delays handles federation propagation timing well. Test names are descriptive and the structure follows existing patterns in the file.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.3.0 #39990 +/- ##
=================================================
- Coverage 70.57% 70.56% -0.02%
=================================================
Files 3258 3258
Lines 115859 115859
Branches 21051 21038 -13
=================================================
- Hits 81767 81751 -16
- Misses 32030 32045 +15
- Partials 2062 2063 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
approved these changes
Mar 31, 2026
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Adds a new test to validate current broken scenario, after merging RocketChat/homeserver#391 the tests should pass.
Issue(s)
FGA-57
Steps to test or reproduce
Further comments
Summary by CodeRabbit