fix: Disable composer actions without subscription to channel - #41202
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 8f687e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
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:
WalkthroughComposer actions are disabled during public channel preview when sending is unavailable, recording is active, or editing is in progress. The timestamp, discussion, and location actions now receive disabled state from the toolbar, and the preview E2E test and changeset were updated. ChangesComposer action disabling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yash-rajpal
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/message-composer.spec.ts (2)
196-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
test.step()for readability.The new test mixes several distinct phases (switch user, navigate, assert toolbar disabled state, assert "More actions" menu items) in one flat block. Wrapping each phase in
test.step()would improve report readability and align with the project convention used elsewhere in this same file (e.g., lines 73-101).As per path instructions for
apps/meteor/tests/e2e/**/*.spec.ts: "Usetest.step()for complex test scenarios to improve organization in Playwright tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/e2e/message-composer.spec.ts` around lines 196 - 227, The new Playwright test in the disabled actions block is too flat and should be split into named steps for readability. Update the test in the `disabled actions` describe group by wrapping the login switch, navigation, disabled-toolbar assertions, and the “More actions” menu assertions in separate `test.step()` calls, following the pattern already used elsewhere in this spec and the `test('should disable all toolbar actions when user is not part of room', ...)` flow.Source: Path instructions
215-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
.locator(':not(:last-child)'); prefer semantic filtering.This CSS-selector chain is fragile (depends on DOM ordering to exclude the "More actions" button) and departs from the project's semantic-locator convention.
♻️ Suggested alternative
- const actions = await poHomeChannel.composer.allPrimaryActions.locator(':not(:last-child)').all(); + const actions = await poHomeChannel.composer.allPrimaryActions + .filter({ hasNotText: 'More actions' }) + .all();As per path instructions,
apps/meteor/tests/e2e/**/*.spec.tsguidelines state: "Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/e2e/message-composer.spec.ts` around lines 215 - 220, The test in message-composer.spec.ts is using a fragile CSS exclusion selector to skip the “More actions” button, so replace the `allPrimaryActions.locator(':not(:last-child)')` usage with a semantic locator/filter approach based on the actual button roles or labels. Update the `poHomeChannel.composer.allPrimaryActions` selection logic so the `actions` collection excludes the “More actions” control without relying on DOM order, and keep the `expect(action).toBeDisabled()` assertion unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsx`:
- Line 62: The WebDAV actions in MessageBoxActionsToolbar still leave the static
“Add Server” item enabled too early. Update the WebDAV action construction so
the `webdav-add` entry also respects `disableBasicActions` (in addition to the
existing success state), likely within `useWebdavActions`, and verify the
`webdavActions` returned to `MessageBoxActionsToolbar` disables “Add Server”
until joining is complete.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/message-composer.spec.ts`:
- Around line 196-227: The new Playwright test in the disabled actions block is
too flat and should be split into named steps for readability. Update the test
in the `disabled actions` describe group by wrapping the login switch,
navigation, disabled-toolbar assertions, and the “More actions” menu assertions
in separate `test.step()` calls, following the pattern already used elsewhere in
this spec and the `test('should disable all toolbar actions when user is not
part of room', ...)` flow.
- Around line 215-220: The test in message-composer.spec.ts is using a fragile
CSS exclusion selector to skip the “More actions” button, so replace the
`allPrimaryActions.locator(':not(:last-child)')` usage with a semantic
locator/filter approach based on the actual button roles or labels. Update the
`poHomeChannel.composer.allPrimaryActions` selection logic so the `actions`
collection excludes the “More actions” control without relying on DOM order, and
keep the `expect(action).toBeDisabled()` assertion unchanged.
🪄 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: 37c51c8e-55c8-443e-a5b2-7daf30b5ce7d
📒 Files selected for processing (7)
.changeset/light-geckos-start.mdapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useCreateDiscussionAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useTimestampAction.tsxapps/meteor/tests/e2e/message-composer.spec.tsapps/meteor/tests/e2e/page-objects/fragments/composer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useCreateDiscussionAction.tsxapps/meteor/tests/e2e/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.spec.tsapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useTimestampAction.tsx
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize existing page objects pattern from
apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/composer.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/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.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-composer.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-composer.spec.ts
🧠 Learnings (9)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/light-geckos-start.md
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useCreateDiscussionAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useTimestampAction.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useCreateDiscussionAction.tsxapps/meteor/tests/e2e/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.spec.tsapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useTimestampAction.tsx
📚 Learning: 2025-12-16T17:29:40.430Z
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:40.430Z
Learning: In all page-object files under apps/meteor/tests/e2e/page-objects/, import expect from ../../utils/test (Playwright's async expect) instead of from Jest. Jest's expect is synchronous and incompatible with web-first assertions like toBeVisible, which can cause TypeScript errors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/composer.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/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.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/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.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/page-objects/fragments/composer.tsapps/meteor/tests/e2e/message-composer.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-composer.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-composer.spec.ts
🔇 Additional comments (7)
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useCreateDiscussionAction.tsx (1)
8-8: LGTM!Also applies to: 31-31
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx (1)
9-9: LGTM!Also applies to: 31-31
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useTimestampAction.tsx (1)
8-8: LGTM!Also applies to: 25-25
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsx (1)
39-166: LGTM on the rest of the wiring —disableBasicActionsis correctly threaded intofileUploadAction,createDiscussionAction,shareLocationAction,timestampAction, and the generic app/message-box action items (line 126), which addresses the core reported bug of undisabled composer actions.apps/meteor/tests/e2e/message-composer.spec.ts (1)
201-213: LGTM!apps/meteor/tests/e2e/page-objects/fragments/composer.ts (1)
125-132: LGTM!.changeset/light-geckos-start.md (1)
1-5: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41202 +/- ##
===========================================
+ Coverage 68.27% 68.41% +0.13%
===========================================
Files 3968 3970 +2
Lines 154740 154939 +199
Branches 27847 27880 +33
===========================================
+ Hits 105652 105998 +346
+ Misses 44294 44193 -101
+ Partials 4794 4748 -46
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/preview-public-channel.spec.ts (1)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a simple loop instead of
Promise.all(map).Since assertions here don't need to run in parallel for correctness, a
for...ofloop is more idiomatic/readable than wrapping inPromise.all.♻️ Optional simplification
- const actions = await poHomeChannel.composer.allPrimaryActions.all(); - await Promise.all( - actions.map(async (action) => { - await expect(action).toBeDisabled(); - }), - ); + const actions = await poHomeChannel.composer.allPrimaryActions.all(); + for (const action of actions) { + await expect(action).toBeDisabled(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/e2e/preview-public-channel.spec.ts` around lines 52 - 57, The assertion block in preview-public-channel.spec.ts is using Promise.all with actions.map even though the checks do not need parallel execution. Update the test around poHomeChannel.composer.allPrimaryActions.all() to iterate through actions with a simple for...of loop and await expect(action).toBeDisabled() sequentially, keeping the same assertion behavior while improving readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/tests/e2e/preview-public-channel.spec.ts`:
- Around line 52-57: The assertion block in preview-public-channel.spec.ts is
using Promise.all with actions.map even though the checks do not need parallel
execution. Update the test around poHomeChannel.composer.allPrimaryActions.all()
to iterate through actions with a simple for...of loop and await
expect(action).toBeDisabled() sequentially, keeping the same assertion behavior
while improving readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb300b2d-1e89-435c-ad60-22d97ec9046f
📒 Files selected for processing (2)
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/tests/e2e/preview-public-channel.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (2)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ✅ **QA assured**
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 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/preview-public-channel.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/preview-public-channel.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/preview-public-channel.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/preview-public-channel.spec.ts
🧠 Learnings (6)
📚 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/preview-public-channel.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/preview-public-channel.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/preview-public-channel.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/preview-public-channel.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/preview-public-channel.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/e2e/preview-public-channel.spec.ts
🔇 Additional comments (2)
apps/meteor/tests/e2e/preview-public-channel.spec.ts (2)
1-117: LGTM!
45-58: 🎯 Functional CorrectnessNo change needed for the toolbar action assertions.
allPrimaryActionsalready covers every button in the composer primary-actions toolbar, including theMore_actionsmenu, so this test is fine.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
dougfabris
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
dougfabris
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
fe25f0d to
8f687e7
Compare
dougfabris
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
dougfabris
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dougfabris
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
dougfabris
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
added
stat: ready to merge
yash-rajpal
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
yash-rajpal
added
stat: ready to merge
yash-rajpal
added
stat: ready to merge
dougfabris
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
dougfabris
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
yash-rajpal
removed
the
stat: QA assured
2 participants
Proposed changes (including videos or screenshots)
Disables all composer actions when user is not subscribed to a channel.
Issue(s)
Steps to test or reproduce
Further comments
CORE-2349
Summary by CodeRabbit
Summary
Bug Fixes
Tests