Skip to content

Conversation

@yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Oct 7, 2025

Proposed changes (including videos or screenshots)

In E2EE rooms, quote messages work differently — the client parses message links and uses them to fetch the quoted message. When the quoted message isn’t accessible by the user, it crashes the UI. This PR fixes the crash.

Issue(s)

Steps to test or reproduce

Further comments

SUP-873

Summary by CodeRabbit

  • Bug Fixes

    • Prevents crashes in end-to-end encrypted rooms when quoting or sharing links to messages the user cannot access; the app now logs the error and continues gracefully.
  • Tests

    • Added end-to-end coverage for quoting and message links to inaccessible messages in encrypted rooms.
    • Added test helpers to create private groups and send messages as specific users.
  • Chores

    • Patch release version bump.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 7, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 7, 2025

🦋 Changeset detected

Latest commit: 76ae2f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/federation-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Walkthrough

Adds guarded handling when fetching quoted messages in the E2EE client to avoid crashes for inaccessible referenced messages, introduces E2E tests covering message-link behavior across private groups, adds test utilities for creating groups and sending messages as specific users, and includes a patch changeset.

Changes

Cohort / File(s) Summary of changes
E2EE client handling
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts
Wraps the REST call to /v1/chat.getMessage in a try/catch with early return on failure; imports Serialized from @rocket.chat/core-typings and types the quoted message as Serialized<IMessage>.
E2E tests — encryption scenarios
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
Adds new "E2EE Quotes" test block(s) validating posting message links to messages in inaccessible rooms and uses helpers to create groups and send messages as specific users; imports BASE_URL, createTargetGroupAndReturnFullRoom, deleteChannel/deleteRoom, and sendMessageFromUser.
E2E test utilities
apps/meteor/tests/e2e/utils/create-target-channel.ts, apps/meteor/tests/e2e/utils/sendMessage.ts
Adds createTargetGroupAndReturnFullRoom(...) to create a private group and return full room data; adds sendMessageFromUser(...) to post a message using a given user's token/id.
Release / changeset
.changeset/fast-forks-sin.md
Adds a patch changeset for @rocket.chat/meteor describing a fix for crashes in E2EE rooms when sending quotes or message links referencing messages outside the room.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as Sender
  participant C as Client (E2EE)
  participant S as Server (REST)
  participant R as Recipient

  U->>C: Send message containing quote or message_link
  C->>S: GET /v1/chat.getMessage (referenced msg id)
  alt Quoted message accessible
    S-->>C: 200 Serialized<IMessage>
    C->>C: Attach quote and continue
  else Not accessible / error
    S-->>C: error / 4xx/5xx
    note right of C #f9f0c1: try/catch -> early return from quote handling\n(no quote attachment)
    C->>C: Proceed without quote
  end
  C->>S: POST chat.sendMessage (encrypted)
  S-->>R: Deliver message
  R->>R: Decrypt and render
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • aleksandernsilva
  • ggazzo

Poem

A hop, I peek where quotes might be,
If hidden, I tuck it gently free.
No crash, no tumble—just a graceful skip,
The warren hums, encryption tight in grip.
Rabbit claps: "Tests pass, let carrots rip!" 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “fix(E2EE): Message quotes crashing room” succinctly captures the primary issue and fix related to preventing crashes in end-to-end encrypted rooms when quoting messages, following a clear conventional commit style.
Linked Issues Check ✅ Passed The changes implement a try/catch around the chat.getMessage call in E2EE contexts to prevent UI crashes when quoted messages are inaccessible, and new end-to-end tests simulate sharing message links from other rooms in encrypted direct messages using helper utilities. These modifications directly satisfy SUP-873 by handling errors gracefully and verifying behavior through automated tests.
Out of Scope Changes Check ✅ Passed All introduced changes, including the error handling in the E2EE client code, version bump, test suite additions, and helper utilities, are directly related to preventing crashes when quoting external messages and validating that behavior under SUP-873. No modifications unrelated to the linked issue objectives have been detected.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/e2ee-quotes-not-allowed

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff6c7e and 42e9ed7.

📒 Files selected for processing (1)
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.61%. Comparing base (2ae6dff) to head (76ae2f1).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37158      +/-   ##
===========================================
+ Coverage    67.57%   67.61%   +0.04%     
===========================================
  Files         3297     3297              
  Lines       112737   112740       +3     
  Branches     20466    20476      +10     
===========================================
+ Hits         76178    76231      +53     
+ Misses       33886    33836      -50     
  Partials      2673     2673              
Flag Coverage Δ
e2e 57.32% <80.00%> (+0.01%) ⬆️
unit 71.65% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yash-rajpal yash-rajpal changed the title fix(E2EE): Placeholder for not allowed quoted message fix(E2EE): Message quotes crashing room Oct 8, 2025
@yash-rajpal yash-rajpal marked this pull request as ready for review October 10, 2025 16:08
@yash-rajpal yash-rajpal requested a review from a team as a code owner October 10, 2025 16:08
@yash-rajpal yash-rajpal added this to the 7.12.0 milestone Oct 10, 2025
@yash-rajpal yash-rajpal requested a review from cardoso October 10, 2025 16:16
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (1)

151-192: Consider using test.step() to improve test organization and reporting.

This test performs multiple distinct operations (setup encrypted channel, create private group, send messages, verify behavior). Using test.step() would improve readability and provide better test reporting, aligning with the coding guidelines for organizing complex test scenarios.

[As per coding guidelines]

Example refactoring:

test('expect to not crash and not show quote message for a message_link which is not accessible to the user', async ({
	page,
	request,
	api,
}) => {
	const encryptedRoomPage = new EncryptedRoomPage(page);
	const sidenav = new HomeSidenav(page);
	const channelName = faker.string.uuid();

	await setupE2EEPassword(page);

	await test.step('create encrypted channel and send initial message', async () => {
		await sidenav.createEncryptedChannel(channelName);
		await expect(page).toHaveURL(`/group/${channelName}`);
		await expect(encryptedRoomPage.encryptedIcon).toBeVisible();
		await encryptedRoomPage.sendMessage('First encrypted message.');
	});

	await test.step('create inaccessible message link from another user', async () => {
		const { group: user1Channel } = await createTargetGroupAndReturnFullRoom(api, {
			excludeSelf: true,
			members: [Users.user2.data._id],
		});
		const sentMessage = (await sendMessageFromUser(request, Users.user2, user1Channel._id, 'This is a test message.')).message;
		const messageLink = `${BASE_URL}/group/${user1Channel.name}?msg=${sentMessage._id}`;
		await encryptedRoomPage.sendMessage(`This is a message with message link - ${messageLink}`);
	});

	await test.step('verify message displays without crashing', async () => {
		// Add assertions here
	});

	await test.step('verify behavior persists after page reload', async () => {
		await page.reload();
		await sidenav.waitForChannel();
		// Add assertions here
	});
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 67f9939 and 524386d.

📒 Files selected for processing (5)
  • .changeset/fast-forks-sin.md (1 hunks)
  • apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (2 hunks)
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (2 hunks)
  • apps/meteor/tests/e2e/utils/create-target-channel.ts (1 hunks)
  • apps/meteor/tests/e2e/utils/sendMessage.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation

Files:

  • apps/meteor/tests/e2e/utils/sendMessage.ts
  • apps/meteor/tests/e2e/utils/create-target-channel.ts
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently

Files:

  • apps/meteor/tests/e2e/utils/sendMessage.ts
  • apps/meteor/tests/e2e/utils/create-target-channel.ts
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts

Files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
🧬 Code graph analysis (4)
apps/meteor/tests/e2e/utils/sendMessage.ts (1)
apps/meteor/tests/e2e/fixtures/userStates.ts (1)
  • IUserState (7-21)
apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • IRoom (21-95)
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
  • IMessage (138-239)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (4)
apps/meteor/tests/e2e/page-objects/encrypted-room.ts (1)
  • EncryptedRoomPage (5-47)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
  • HomeSidenav (7-254)
apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
  • createTargetGroupAndReturnFullRoom (120-126)
apps/meteor/tests/e2e/utils/sendMessage.ts (1)
  • sendMessageFromUser (6-19)
⏰ 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). (21)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (3/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (4/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (5/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (2/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (1/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (3/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (2/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (1/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (4/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (3/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (4/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (1/4) - Alpine (Official)
  • GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (2/4) - Alpine (Official)
  • GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1) - Alpine (Official)
  • GitHub Check: 🔨 Test API (CE) / MongoDB 7.0 (1/1) - Alpine (Official)
  • GitHub Check: 🔨 Test API (CE) / MongoDB 5.0 (1/1) - Alpine (Official)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6b9b1d1 and 1cba136.

📒 Files selected for processing (5)
  • .changeset/fast-forks-sin.md (1 hunks)
  • apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (2 hunks)
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (2 hunks)
  • apps/meteor/tests/e2e/utils/create-target-channel.ts (1 hunks)
  • apps/meteor/tests/e2e/utils/sendMessage.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation

Files:

  • apps/meteor/tests/e2e/utils/create-target-channel.ts
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
  • apps/meteor/tests/e2e/utils/sendMessage.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently

Files:

  • apps/meteor/tests/e2e/utils/create-target-channel.ts
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
  • apps/meteor/tests/e2e/utils/sendMessage.ts
apps/meteor/tests/e2e/**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts

Files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior

Applied to files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
🧬 Code graph analysis (4)
apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • IRoom (21-95)
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
  • IMessage (138-239)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (4)
apps/meteor/tests/e2e/page-objects/encrypted-room.ts (1)
  • EncryptedRoomPage (5-47)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
  • HomeSidenav (7-254)
apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
  • createTargetGroupAndReturnFullRoom (120-126)
apps/meteor/tests/e2e/utils/sendMessage.ts (1)
  • sendMessageFromUser (6-19)
apps/meteor/tests/e2e/utils/sendMessage.ts (1)
apps/meteor/tests/e2e/fixtures/userStates.ts (1)
  • IUserState (7-21)
⏰ 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 comments (4)
.changeset/fast-forks-sin.md (1)

1-5: LGTM!

The changeset correctly documents the patch with a clear description of the fix for E2EE room crashes when handling message quotes from inaccessible rooms.

apps/meteor/tests/e2e/utils/create-target-channel.ts (1)

120-126: LGTM!

The helper function follows the established pattern of createTargetChannelAndReturnFullRoom and provides a consistent interface for creating groups in tests. The implementation is clean and properly typed.

apps/meteor/tests/e2e/utils/sendMessage.ts (1)

6-19: LGTM!

The utility function provides a clean interface for sending messages as specific users in tests. The implementation correctly uses the chat.postMessage endpoint with proper authentication headers.

apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (1)

785-791: LGTM! Error handling correctly scoped.

The try/catch now properly wraps only the REST call that can fail when fetching inaccessible quoted messages. The early return on error prevents crashes while allowing the message to render without the quote attachment. The explicit Serialized<IMessage> typing aligns with the API response structure.

Based on past review comments

cardoso
cardoso previously approved these changes Oct 10, 2025
Copy link
Member

@cardoso cardoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (3)

160-169: Missing E2EE password setup before creating encrypted channel.

The test creates an encrypted channel without calling setupE2EEPassword(page) first. All other E2EE tests in this file (lines 47, 87) call this function before creating encrypted channels, which is necessary for proper E2EE functionality.

Apply this diff to add the missing setup:

 		const encryptedRoomPage = new EncryptedRoomPage(page);
 		const sidenav = new HomeSidenav(page);
 		targetChannelName = faker.string.uuid();
 
+		await setupE2EEPassword(page);
+
 		await sidenav.createEncryptedChannel(targetChannelName);

197-201: Add explicit wait after page reload to prevent flakiness.

After page.reload(), the test immediately checks for elements without waiting for the page to be fully loaded and E2EE to be ready. This can cause race conditions and flaky test behavior.

Apply this diff to add proper waits:

 		await page.reload();
+		await expect(page).toHaveURL(`/group/${targetChannelName}`);
+		await expect(encryptedRoomPage.encryptedIcon).toBeVisible();
 
 		await expect(encryptedRoomPage.lastMessage.encryptedIcon).toBeVisible();

191-201: Missing verification of "not allowed" placeholder for inaccessible quoted messages.

According to the PR objectives, the fix should "replace the attachment with a 'not allowed' placeholder" when a quoted message is inaccessible. The test currently only verifies that the message text containing the link is displayed and the room doesn't crash, but it doesn't verify:

  1. The quoted message attachment is not rendered
  2. A "not allowed" placeholder is shown instead

Add assertions to verify the presence of the "not allowed" placeholder:

// After line 194, add:
// Verify no quoted message attachment is rendered for the inaccessible message
await expect(encryptedRoomPage.lastMessage.quotedMessage).not.toBeVisible();
// Or verify the "not allowed" placeholder is displayed (adjust selector as needed)
// await expect(encryptedRoomPage.lastMessage.placeholder).toContainText('not allowed');

Note: Adjust the selectors based on the actual DOM structure of your "not allowed" placeholder implementation.

🧹 Nitpick comments (1)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (1)

160-202: Consider using test.step() to organize test phases.

The test has multiple distinct phases (setup encrypted channel, create inaccessible group, send link, verify). Using test.step() would improve readability and make the test flow clearer, aligning with the coding guidelines for complex test scenarios.

Example structure:

await test.step('create encrypted channel and send initial message', async () => {
	// Lines 169-177
});

await test.step('create inaccessible private group for user2', async () => {
	// Lines 179-189
});

await test.step('send message with link to inaccessible message', async () => {
	// Lines 191-195
});

await test.step('verify message persists after reload without crash', async () => {
	// Lines 197-201
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ca9e767 and 6ff6c7e.

📒 Files selected for processing (1)
  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts

Files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation

Files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently

Files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior

Applied to files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Implement proper wait strategies for dynamic content

Applied to files:

  • apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts (4)
apps/meteor/tests/e2e/utils/create-target-channel.ts (3)
  • deleteRoom (48-50)
  • deleteChannel (44-46)
  • createTargetGroupAndReturnFullRoom (120-126)
apps/meteor/tests/e2e/page-objects/encrypted-room.ts (1)
  • EncryptedRoomPage (5-47)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
  • HomeSidenav (7-254)
apps/meteor/tests/e2e/utils/sendMessage.ts (1)
  • sendMessageFromUser (6-19)
⏰ 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). (2)
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build

@yash-rajpal yash-rajpal added the stat: QA assured Means it has been tested and approved by a company insider label Oct 16, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 16, 2025
@yash-rajpal yash-rajpal removed the request for review from cardoso October 16, 2025 19:06
@kodiakhq kodiakhq bot merged commit 3fc317e into develop Oct 16, 2025
49 checks passed
@kodiakhq kodiakhq bot deleted the fix/e2ee-quotes-not-allowed branch October 16, 2025 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants