fix: "Reply in direct message" action not visible for users with create DM permission #40078
Merged
dionisio-bot[bot] merged 5 commits intodevelopfrom Apr 13, 2026
Merged
fix: "Reply in direct message" action not visible for users with create DM permission #40078dionisio-bot[bot] merged 5 commits intodevelopfrom
dionisio-bot[bot] merged 5 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 704f48b 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughInverts DM-reply lookup so the “Reply in direct message” action appears when a user has Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ClientHook as useReplyInDMAction
participant Permissions as PermissionsService
participant Rooms as RoomsService
participant Router
User->>ClientHook: open message menu / click "Reply in direct message" option
ClientHook->>Permissions: check "create-d" for current user
alt user has create-d
ClientHook->>Router: navigate to /reply (no DM lookup)
else user lacks create-d
ClientHook->>Rooms: find existing DM room & subscriptions for target user
Rooms-->>ClientHook: dmRoom / dmSubs (or null)
alt dm exists
ClientHook->>Router: navigate to /reply
else no dm
ClientHook-->>User: hide "Reply in direct message" option
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ricardogarim
force-pushed
the
fix/reply-in-dm-visibility
branch
from
e96db3c to
69e575d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40078 +/- ##
===========================================
- Coverage 70.56% 70.18% -0.38%
===========================================
Files 3271 3279 +8
Lines 116804 116798 -6
Branches 21060 20729 -331
===========================================
- Hits 82424 81976 -448
+ Misses 32331 31534 -797
- Partials 2049 3288 +1239
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
juliajforesti
previously approved these changes
Apr 8, 2026
aleksandernsilva
previously requested changes
Apr 8, 2026
dougfabris
requested changes
Apr 8, 2026
Member
dougfabris
left a comment
dougfabris
left a comment
There was a problem hiding this comment.
Unfortunately we cannot move ahead without any tests covering this case!
aleksandernsilva
dismissed
their stale review
Better context was provided and my original concern does not apply.
![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: 1
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/permissions.spec.ts (2)
202-205: Consider extracting message locator or using semantic alternative.The
.rcx-messageCSS selector pattern is used throughout this file, so it's consistent with existing code. However, consider extracting it to a constant or page object method for reuse, aligning with the guideline to store commonly used locators.♻️ Optional: Extract to page object or constant
// Could add to HomeContent page object: getMessageByText(text: string) { return this.page.locator('.rcx-message', { hasText: text }); }Or define a constant within the test suite if keeping it local.
As per coding guidelines: "Store commonly used locators in variables/constants for reuse" and "Follow Page Object Model pattern consistently in Playwright tests".
Also applies to: 218-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/permissions.spec.ts` around lines 202 - 205, Tests repeatedly use the '.rcx-message' locator inline (page.locator('.rcx-message', { hasText: ... })); extract this into a reusable locator by adding a method like getMessageByText(text: string) to the HomeContent page object (or a constant factory in the test file) and replace the inline usages (e.g., the call that checks aria-busy for 'message from admin for reply in DM') with that method call to improve reuse and follow the page-object guideline.
208-208: Preferpage.getByRole()overpage.locator('role=...').The locator string
'role=menuitem[name="Reply in direct message"]'should use the semantic locator API for consistency with project guidelines.♻️ Proposed refactor
- await expect(page.locator('role=menuitem[name="Reply in direct message"]')).toBeHidden(); + await expect(page.getByRole('menuitem', { name: 'Reply in direct message' })).toBeHidden();- await page.locator('role=menuitem[name="Reply in direct message"]').click(); + await page.getByRole('menuitem', { name: 'Reply in direct message' }).click();As per coding guidelines: "Avoid using
page.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()".Also applies to: 224-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/permissions.spec.ts` at line 208, Replace the use of page.locator with Playwright's semantic locator API: instead of using page.locator with the role selector string for the "Reply in direct message" menu item, use page.getByRole with role "menuitem" and name "Reply in direct message" and keep the same expectation (toBeHidden()); apply the same change for the other occurrence referenced around the same test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/e2e/permissions.spec.ts`:
- Around line 189-228: In the "Reply in direct message" test.describe.serial
block the tests mutate the create-d permission via
api.post('/permissions.update') but never restore it; capture the original
create-d permission at the start of test.beforeAll (e.g., call the API to fetch
the current create-d roles) and add a test.afterAll in the same describe.serial
that calls api.post('/permissions.update') to restore the saved original roles
for _id: 'create-d' so other tests are not affected; locate the describe named
"Reply in direct message" (around createTargetChannel / sendTargetChannelMessage
and the existing api.post calls) and implement the beforeAll capture + afterAll
restore using the same API endpoints.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/permissions.spec.ts`:
- Around line 202-205: Tests repeatedly use the '.rcx-message' locator inline
(page.locator('.rcx-message', { hasText: ... })); extract this into a reusable
locator by adding a method like getMessageByText(text: string) to the
HomeContent page object (or a constant factory in the test file) and replace the
inline usages (e.g., the call that checks aria-busy for 'message from admin for
reply in DM') with that method call to improve reuse and follow the page-object
guideline.
- Line 208: Replace the use of page.locator with Playwright's semantic locator
API: instead of using page.locator with the role selector string for the "Reply
in direct message" menu item, use page.getByRole with role "menuitem" and name
"Reply in direct message" and keep the same expectation (toBeHidden()); apply
the same change for the other occurrence referenced around the same test.
🪄 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: 75232eaa-c1e2-437b-a945-aa319932a056
📒 Files selected for processing (1)
apps/meteor/tests/e2e/permissions.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). (8)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/permissions.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/permissions.spec.ts
🧠 Learnings (17)
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/tests/e2e/permissions.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.spec.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/permissions.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/permissions.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/e2e/permissions.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/permissions.spec.ts
📚 Learning: 2026-03-14T14:58:58.834Z
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.
Applied to files:
apps/meteor/tests/e2e/permissions.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/e2e/permissions.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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.spec.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.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:
apps/meteor/tests/e2e/permissions.spec.ts
![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 (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/e2e/permissions.spec.ts">
<violation number="1" location="apps/meteor/tests/e2e/permissions.spec.ts:189">
P2: Missing `afterAll` cleanup to restore the `create-d` permission. The first test restricts `create-d` to `['admin']` only, and while the second test restores it, if that test fails or is skipped the permission remains restricted, potentially breaking downstream tests. Other `test.describe.serial` blocks in this file follow the pattern of restoring state in `afterAll`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ricardogarim
force-pushed
the
fix/reply-in-dm-visibility
branch
from
fb94129 to
bea8082
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/message-actions.spec.ts (1)
176-176: Preferpage.getByRole()overpage.locator('role=...').As per coding guidelines, avoid using
page.locator()in Playwright tests — prefer semantic locators likepage.getByRole().♻️ Suggested refactor
- await expect(page.locator('role=menuitem[name="Reply in direct message"]')).toBeHidden(); + await expect(page.getByRole('menuitem', { name: 'Reply in direct message' })).toBeHidden();- await page.locator('role=menuitem[name="Reply in direct message"]').click(); + await page.getByRole('menuitem', { name: 'Reply in direct message' }).click();Also applies to: 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/message-actions.spec.ts` at line 176, Replace usages of page.locator('role=menuitem[name="Reply in direct message"]') with the semantic Playwright API: use page.getByRole('menuitem', { name: 'Reply in direct message' }) and similarly replace the other occurrence for the same menu item (the one around the second instance mentioned). Update the assertion calls (e.g., toBeHidden) to operate on the getByRole result; locate the calls by the exact locator string 'role=menuitem[name="Reply in direct message"]' in message-actions.spec.ts to make the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/e2e/message-actions.spec.ts`:
- Line 176: Replace usages of page.locator('role=menuitem[name="Reply in direct
message"]') with the semantic Playwright API: use page.getByRole('menuitem', {
name: 'Reply in direct message' }) and similarly replace the other occurrence
for the same menu item (the one around the second instance mentioned). Update
the assertion calls (e.g., toBeHidden) to operate on the getByRole result;
locate the calls by the exact locator string 'role=menuitem[name="Reply in
direct message"]' in message-actions.spec.ts to make the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81b54048-427d-4fe3-b595-d7e0427d4590
📒 Files selected for processing (1)
apps/meteor/tests/e2e/message-actions.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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/message-actions.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/message-actions.spec.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/e2e/message-actions.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/e2e/message-actions.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/message-actions.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/e2e/message-actions.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/message-actions.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/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/message-actions.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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/message-actions.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/message-actions.spec.ts
🔇 Additional comments (4)
apps/meteor/tests/e2e/message-actions.spec.ts (4)
5-5: LGTM!Import addition is appropriate for the new test functionality.
16-16: LGTM!Adding
user2as a channel member is required for the new test suite that runs underUsers.user2.state.
165-195: Good test structure with proper cleanup.The serial test block correctly:
- Uses
Users.user2.stateto test from a non-admin perspective- Seeds a message from admin in
beforeAllto test the "Reply in DM" action- Tests both permission-denied and permission-granted scenarios
- Restores permissions in
afterAllto prevent side effects on other testsThis aligns well with the PR objective to verify the "Reply in direct message" action visibility based on
create-dpermission.
172-177: Permission changes propagate in real-time via Rocket.Chat's cached store synchronization. The pattern of callingpermissions.update()followed immediately by menu interaction without a page reload is established and used consistently across 20+ similar tests in the codebase (message-actions.spec.ts and team-management.spec.ts) without flakiness. TheopenLastMessageMenu()method includes implicit waits via.waitFor()for DOM elements, which aligns with when client-side permissions are updated.
juliajforesti
requested changes
Apr 8, 2026
juliajforesti
previously approved these changes
Apr 8, 2026
Contributor
|
Hey! I saw that when clicking the option and being redirected to the DM, we can see what message we are answering to, but at the moment of sending the message, the quote disappears. Is this expected? Screen.Recording.2026-04-09.at.6.35.34.PM.movProbably it's out of this PR's scope, but as you've been in touch with product team maybe you might be able to clarify this |
Member
|
@nazabucciarelli Nice catch! Actually the customer complained about this as well but our team couldn't reproduce it consistently, I will check so maybe we can fix in the same PR! |
This was referenced Apr 9, 2026
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dougfabris
requested changes
Apr 10, 2026
johnrommeloctaviano
approved these changes
Apr 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts (1)
8-11: Use the WHATWGURLconstructor instead of deprecatedURL.parse().
URL.parse()is deprecated (emits DEP0169 runtime warning as of Node.js late 2024) and prone to security vulnerabilities. Replace with:const getMessageIdFromUrl = (url: string): string | undefined => { try { const { searchParams } = new URL(url, 'http://localhost'); return searchParams.get('msg') || undefined; } catch { return undefined; } };The
searchParams.get()method maintains the current security behavior (returns only the first value, rejecting array pollution attempts) while using the standardized WHATWG API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts` around lines 8 - 11, Replace the deprecated URL.parse usage in getMessageIdFromUrl by constructing a WHATWG URL; wrap new URL(url, 'http://localhost') in a try/catch, extract searchParams, and return searchParams.get('msg') || undefined so you preserve single-value behavior and avoid parse warnings or security issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/e2e/utils/getPermissionRoles.ts`:
- Around line 4-6: The helper currently swallows a missing permission by
returning an empty array; modify the logic in getPermissionRoles so that after
calling api.get('/permissions.listAll') and locating the permission via
update.find((p) => p._id === permissionId) you throw an explicit error
(including permissionId and maybe the fetched update list length) if the
permission is not found instead of using the fallback `?? []`, ensuring callers
fail fast on unexpected lookups.
---
Nitpick comments:
In `@apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts`:
- Around line 8-11: Replace the deprecated URL.parse usage in
getMessageIdFromUrl by constructing a WHATWG URL; wrap new URL(url,
'http://localhost') in a try/catch, extract searchParams, and return
searchParams.get('msg') || undefined so you preserve single-value behavior and
avoid parse warnings or security issues.
🪄 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: d442bac8-980d-464c-b587-d9b7da99f17e
📒 Files selected for processing (7)
.changeset/shy-pillows-repair.mdapps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/server/services/messages/service.tsapps/meteor/tests/e2e/message-actions.spec.tsapps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/tests/e2e/utils/index.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
💤 Files with no reviewable changes (1)
- apps/meteor/server/services/messages/service.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/shy-pillows-repair.md
- apps/meteor/tests/e2e/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/e2e/message-actions.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:
apps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/utils/getPermissionRoles.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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:
apps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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:
apps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/utils/getPermissionRoles.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/utils/getPermissionRoles.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/e2e/utils/getPermissionRoles.tsapps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.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:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts
🔇 Additional comments (4)
apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts (2)
145-178: Good test coverage for the hostname-agnostic quote detection.This new test correctly validates the core fix: quote attachments are now created based on the
?msg=query parameter presence rather than hostname matching. The assertions properly verify both theignoreParse: trueflag and the attachment content.
120-143: Good security coverage for query parameter pollution.The test correctly validates that
msg[$gt]=style NoSQL injection patterns are rejected since the parsed query value would be an object/array rather than a string.apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts (2)
98-106: Clean refactoring of the linked message extraction.The new approach correctly:
- Extracts
msgIdper URL item using the helper- Preserves the
urlItemreference for laterignoreParsemutation- Passes only valid msgIds to
getMessagesThis eliminates the previous
siteUrldependency while maintaining the security guarantees through downstream room access checks.
132-145: LGTM on the quote attachment creation loop.The iteration over
linkedMessagescorrectly:
- Matches fetched messages by
_id- Sets
ignoreParseon the original URL item to prevent OEmbed re-fetching- Passes the correct URL to
createQuoteAttachment
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts">
<violation number="1" location="apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts:100">
P1: Restoring quote detection without checking the URL origin makes any `?msg=` link look like an internal message link.</violation>
</file>
<file name=".changeset/shy-pillows-repair.md">
<violation number="1" location=".changeset/shy-pillows-repair.md:5">
P2: This changeset describes a different bug than the code in this PR, so the generated release notes will be incorrect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ricardogarim
force-pushed
the
fix/reply-in-dm-visibility
branch
from
a869f72 to
c144392
Compare
ricardogarim
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/e2e/message-actions.spec.ts">
<violation number="1" location="apps/meteor/tests/e2e/message-actions.spec.ts:178">
P2: Run the no-create-d case before the DM-opening case, or reset the DM state between them. The current serial order can leave an existing DM behind and make the later hidden-visibility assertion exercise the wrong precondition.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
6 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)
Introduced here: #36217
Fixes the "Reply in direct message" action not being shown in the message menu when the user has
create-dpermission but no existing DM conversation exists with the message author.The condition
canCreateDMwas changed to!canCreateDMin two places, restoring the original intended logic that was accidentally inverted during a previous refactor.Issue(s)
Steps to test or reproduce
create-dpermissionFurther comments
Summary by CodeRabbit
Bug Fixes
Tests
Chores