feat: add rooms.join endpoint for all room types#40996
Conversation
Add a unified POST /v1/rooms.join REST endpoint that lets a user join any room type, replicating the behavior of the deprecated joinRoom DDP method. Unlike channels.join (public channels only), it resolves all room types through the shared Room.join service, applying access, join-code, federation and omnichannel checks. Switch the client join callers from channels.join to rooms.join.
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: b52aead 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:
WalkthroughIntroduces a new Changesrooms.join REST endpoint
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40996 +/- ##
===========================================
+ Coverage 70.11% 70.13% +0.02%
===========================================
Files 3357 3359 +2
Lines 129542 129609 +67
Branches 22404 22447 +43
===========================================
+ Hits 90832 90905 +73
+ Misses 35420 35397 -23
- Partials 3290 3307 +17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
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="packages/rest-typings/src/v1/rooms.ts">
<violation number="1" location="packages/rest-typings/src/v1/rooms.ts:767">
P2: `RoomsJoinProps` is out of sync with its Ajv schema: schema accepts `joinCode: null` but the type does not. This creates unsound typing for consumers of `isRoomsJoinProps` and downstream `joinCode` handling.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| export const isRoomsOpenProps = ajv.compile<RoomsOpenProps>(roomsOpenSchema); | ||
|
|
||
| export type RoomsJoinProps = { roomId: string; joinCode?: string } | { roomName: string; joinCode?: string }; |
There was a problem hiding this comment.
P2: RoomsJoinProps is out of sync with its Ajv schema: schema accepts joinCode: null but the type does not. This creates unsound typing for consumers of isRoomsJoinProps and downstream joinCode handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rest-typings/src/v1/rooms.ts, line 767:
<comment>`RoomsJoinProps` is out of sync with its Ajv schema: schema accepts `joinCode: null` but the type does not. This creates unsound typing for consumers of `isRoomsJoinProps` and downstream `joinCode` handling.</comment>
<file context>
@@ -764,6 +764,45 @@ const roomsOpenSchema = {
export const isRoomsOpenProps = ajv.compile<RoomsOpenProps>(roomsOpenSchema);
+export type RoomsJoinProps = { roomId: string; joinCode?: string } | { roomName: string; joinCode?: string };
+
+const roomsJoinSchema = {
</file context>
| export type RoomsJoinProps = { roomId: string; joinCode?: string } | { roomName: string; joinCode?: string }; | |
| export type RoomsJoinProps = { roomId: string; joinCode?: string | null } | { roomName: string; joinCode?: string | null }; |
Covers joining a public channel by id and name, joining a discussion (a room with a parent room), the not-allowed case for a private group, and the join-code required/invalid/correct flows.
![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
🤖 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/tests/end-to-end/api/rooms.ts`:
- Around line 1991-1999: In the after() teardown hook, add a deleteRoom call to
explicitly clean up the testDiscussion room that was created in setup at line
1983. Add a Promise.all entry that calls deleteRoom with testDiscussion._id as
the roomId parameter and the appropriate type parameter (check how the
discussion room was created to determine the correct type value). This ensures
the discussion room is explicitly deleted during teardown rather than relying on
parent-room cascade deletion, preventing residual data and cross-test flakiness.
🪄 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: 43218c3a-20e4-4edb-ad0c-bd94e6fbcf31
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/rooms.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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/end-to-end/api/rooms.ts
🧠 Learnings (3)
📚 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/end-to-end/api/rooms.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/end-to-end/api/rooms.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/end-to-end/api/rooms.ts
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/end-to-end/api/rooms.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/rooms.ts:1995">
P2: Missing cleanup of testDiscussion in the after block. The discussion room created in `before` will remain orphaned after tests finish, polluting the database.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Covers a non-member joining a public channel and a discussion through the "Join channel" UI, which calls /v1/rooms.join via useJoinRoom.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/rooms-join.spec.ts (2)
55-55: ⚡ Quick winUse a precise type for
discussioninstead ofRecord<string, string>.
Record<string, string>is too broad for_id/nameusage and weakens compile-time guarantees.Suggested typing improvement
-let discussion: Record<string, string>; +let discussion: Awaited<ReturnType<typeof createTargetDiscussion>>;As per coding guidelines, “Write concise, technical TypeScript/JavaScript with accurate typing 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/rooms-join.spec.ts` at line 55, The `discussion` variable is typed too broadly as `Record<string, string>`, which weakens type safety for the `_id` and `name` properties used throughout the test. Replace this generic Record type with a precise interface or type definition that explicitly declares the expected properties (such as `_id: string` and `name: string`) to improve compile-time guarantees and provide better type checking in the Playwright test.Source: Coding guidelines
12-13: ⚡ Quick winRemove inline implementation comments in this spec.
Move this intent into helper naming or test-step naming instead of comments.
As per coding guidelines, “Avoid code comments in the implementation.”
🤖 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/rooms-join.spec.ts` around lines 12 - 13, The inline comments at lines 12-13 in rooms-join.spec.ts explaining the intent about restricting preview to admin and the useJoinRoom behavior should be removed per coding guidelines to avoid implementation comments. Instead, refactor the test setup and steps to use descriptive helper function names or clear test step descriptions that self-document the intent without requiring explanatory comments, so the purpose is evident from the code structure itself.Source: Coding guidelines
🤖 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/tests/e2e/rooms-join.spec.ts`:
- Around line 11-19: The test suite is using test.beforeAll and test.afterAll
hooks to globally modify the preview-c-room permission, which persists across
all tests in the suite and affects parallel test execution in other spec files.
Replace test.beforeAll with test.beforeEach and test.afterAll with
test.afterEach so that the permission change is scoped to individual test
execution rather than the entire suite. This ensures each test modifies and
restores the permission independently, maintaining test isolation when running
tests in parallel.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/rooms-join.spec.ts`:
- Line 55: The `discussion` variable is typed too broadly as `Record<string,
string>`, which weakens type safety for the `_id` and `name` properties used
throughout the test. Replace this generic Record type with a precise interface
or type definition that explicitly declares the expected properties (such as
`_id: string` and `name: string`) to improve compile-time guarantees and provide
better type checking in the Playwright test.
- Around line 12-13: The inline comments at lines 12-13 in rooms-join.spec.ts
explaining the intent about restricting preview to admin and the useJoinRoom
behavior should be removed per coding guidelines to avoid implementation
comments. Instead, refactor the test setup and steps to use descriptive helper
function names or clear test step descriptions that self-document the intent
without requiring explanatory comments, so the purpose is evident from the code
structure itself.
🪄 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: 711b8c8b-b35e-4280-a9ba-db4d13b220ab
📒 Files selected for processing (1)
apps/meteor/tests/e2e/rooms-join.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 (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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.spec.ts
🔇 Additional comments (1)
apps/meteor/tests/e2e/rooms-join.spec.ts (1)
40-49: LGTM!Also applies to: 65-74
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="packages/rest-typings/src/v1/rooms.ts">
<violation number="1" location="packages/rest-typings/src/v1/rooms.ts:767">
P2: `RoomsJoinProps` is out of sync with its Ajv schema: schema accepts `joinCode: null` but the type does not. This creates unsound typing for consumers of `isRoomsJoinProps` and downstream `joinCode` handling.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/rooms.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/rooms.ts:1995">
P2: Missing cleanup of testDiscussion in the after block. The discussion room created in `before` will remain orphaned after tests finish, polluting the database.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A member of the private parent (but not of the discussion) joins the type-`p` discussion via rooms.join; access is inherited from the parent, a case channels.join could not handle.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/rooms-join.spec.ts (2)
57-69: ⚡ Quick winRemove duplicated
HomeChannelinitialization inbeforeEachhooks.Line 58 and Line 67 both assign
poHomeChannel = new HomeChannel(page). The first hook is redundant.Proposed simplification
- test.beforeEach(async ({ page }) => { - poHomeChannel = new HomeChannel(page); - }); - test.beforeEach(async ({ api }) => { targetChannel = await createTargetChannel(api); await sendTargetChannelMessage(api, targetChannel, { msg: 'message from a channel the user has not joined' }); });🤖 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/rooms-join.spec.ts` around lines 57 - 69, The first beforeEach hook (lines 57-59) contains a redundant initialization of poHomeChannel with new HomeChannel(page) that is duplicated in the third beforeEach hook (lines 65-69). Remove the entire first beforeEach hook that only contains the poHomeChannel initialization, keeping only the second beforeEach hook that handles API setup and the third beforeEach hook that initializes poHomeChannel and navigates to the channel.
124-126: ⚡ Quick winUse
test.step()for this complex flow and remove inline TODO comments.This scenario has multiple phases (join action + cache-workaround verification). Wrapping them with
test.step()will make reporting clearer and keeps intent out of inline implementation comments.As per coding guidelines, “Use
test.step()for complex test scenarios to improve organization in Playwright tests” and “Avoid code comments in the implementation.”Also applies to: 143-145, 137-150
🤖 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/rooms-join.spec.ts` around lines 124 - 126, The test contains complex multi-phase scenarios described with inline comments instead of using Playwright's test.step() for organization. Replace the inline comment blocks (such as the comments explaining the discussion type inheritance and user membership scenarios around lines 124-126, 137-150, and 143-145) with corresponding test.step() calls that describe each phase, removing the implementation comments and letting the step descriptions capture the intent and context of each phase in the test flow.Source: Coding guidelines
🤖 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/tests/e2e/rooms-join.spec.ts`:
- Around line 19-50: The test suite at line 19 is incorrectly using test.skip()
which is only for individual tests; change it to test.describe.skip() to
properly skip the entire suite with its hooks and nested tests. Additionally,
the variable poHomeChannel is referenced in the nested test (around line 46) but
is never initialized, so you need to declare and initialize poHomeChannel in the
beforeEach hook (the one that takes the page parameter) to set up the page
object before each test runs, similar to how it would be done in other test
suites.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/rooms-join.spec.ts`:
- Around line 57-69: The first beforeEach hook (lines 57-59) contains a
redundant initialization of poHomeChannel with new HomeChannel(page) that is
duplicated in the third beforeEach hook (lines 65-69). Remove the entire first
beforeEach hook that only contains the poHomeChannel initialization, keeping
only the second beforeEach hook that handles API setup and the third beforeEach
hook that initializes poHomeChannel and navigates to the channel.
- Around line 124-126: The test contains complex multi-phase scenarios described
with inline comments instead of using Playwright's test.step() for organization.
Replace the inline comment blocks (such as the comments explaining the
discussion type inheritance and user membership scenarios around lines 124-126,
137-150, and 143-145) with corresponding test.step() calls that describe each
phase, removing the implementation comments and letting the step descriptions
capture the intent and context of each phase in the test flow.
🪄 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: ffbe1720-355b-4356-8daf-06b7c34d8b1a
📒 Files selected for processing (1)
apps/meteor/tests/e2e/rooms-join.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.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/rooms-join.spec.ts
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="packages/rest-typings/src/v1/rooms.ts">
<violation number="1" location="packages/rest-typings/src/v1/rooms.ts:767">
P2: `RoomsJoinProps` is out of sync with its Ajv schema: schema accepts `joinCode: null` but the type does not. This creates unsound typing for consumers of `isRoomsJoinProps` and downstream `joinCode` handling.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
useJoinRoom invalidated a stale React Query key (['rooms', { reference,
type }]) that no longer matched the open-room query key
(roomsQueryKeys.roomReference => ['rooms', reference, type, uid]). The
invalidation matched nothing, so the 'not subscribed' screen kept
rendering until a manual reload. Invalidate the correct ['rooms',
reference, type] prefix instead, and re-enable the e2e coverage.
Co-authored-by: dougfabris <devfabris@gmail.com>
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes
Follow-up to #40711, which left this
ddp-removalTODO:This PR adds that unified endpoint.
Adds
POST /v1/rooms.join, which lets a user join any room type, replicating the behavior of the deprecatedjoinRoomDDP method (slated for removal in 9.0.0).channels.joinonly resolves publiccchannels — private groups, DMs and livechat error out. The new endpoint funnels all room types through the sharedRoom.joinservice, so it applies the same access checks, join-code validation, federation and omnichannel rules as the DDP method.Changes
rest-typings: newRoomsJoinProps+isRoomsJoinProps(accepts{ roomId | roomName, joinCode? }) and/v1/rooms.joinendpoint type returning{ room }.apps/meteorAPI: newrooms.joinroute callingRoom.join({ room, user, joinCode }).useJoinRoom,chats/data,ComposerJoinWithPassword,ComposerReadOnly) fromchannels.jointorooms.join; removed theddp-removalTODOs added in feat: extend 4 REST endpoints + migrate 7 DDP callers #40711.Todo
rooms.join(public channel, discussion, private-group not-allowed, join-code cases)tsconapps/meteorIssue(s)
Follow-up to #40711.
How to test the changes
POST /api/v1/rooms.joinwith{ "roomId": "<public channel>" }→ joins.channels.join).error-code-requiredwithout code, joins with correct code.ARCH-2164
Summary by CodeRabbit
POST /v1/rooms.joinso users can join any room type usingroomIdorroomName, with optional join-code support.rooms.join.rooms.joinfor consistent join behavior.https://rocketchat.atlassian.net/browse/CORE-2296