fix: Prevent video calls from being started in archived rooms #37109
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 97484bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughAdds an archived-room check to the video-call action disabling logic, introduces a test utility to create and archive a channel, and extends video-conference e2e tests to cover the archived-channel scenario (setup/teardown and a test asserting the video call button is disabled). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client UI (Room)
participant Hook as useVideoCallRoomAction
participant Store as Room State Store
User->>UI: Open room
UI->>Hook: Request video-call action state
Hook->>Store: Read room flags (archived, federated, folder/perm readonly)
alt room.archived or other disable conditions
Hook-->>UI: disabled = true (tooltip via disabled flow)
UI-->>User: Video call button shown disabled
else
Hook-->>UI: disabled = false
User->>UI: Click video call
UI->>Server: Initiate call
Server-->>UI: Call started
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ 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). (6)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37109 +/- ##
========================================
Coverage 67.37% 67.38%
========================================
Files 3330 3330
Lines 113482 113482
Branches 20601 20605 +4
========================================
+ Hits 76464 76470 +6
+ Misses 34408 34402 -6
Partials 2610 2610
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/utils/create-target-channel.ts (2)
107-111: Improve error message interpolation.The error handling uses
${error}which may produce unhelpful output like[object Object]. Consider using${error?.message}or${String(error)}for better error visibility during test failures.Apply this diff:
} catch (error) { - throw new Error(`Error archiving the channel: ${error}`); + throw new Error(`Error archiving the channel: ${error instanceof Error ? error.message : String(error)}`); }
113-115: Validation occurs after archive operation.The channel name validation happens after the channel is archived. If the validation fails, an archived channel remains in the system. Consider validating
channel.nameimmediately after line 105, before the archive operation.Apply this diff:
export async function createArchivedChannel(api: BaseTest['api']): Promise<string> { const { channel } = await createTargetChannelAndReturnFullRoom(api); + if (!channel.name) { + throw new Error('Invalid channel was created'); + } + try { await api.post('/channels.archive', { roomId: channel._id }); } catch (error) { throw new Error(`Error archiving the channel: ${error instanceof Error ? error.message : String(error)}`); } - if (!channel.name) { - throw new Error('Invalid channel was created'); - } - return channel.name; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/hooks/roomActions/useVideoCallRoomAction.tsx(1 hunks)apps/meteor/tests/e2e/utils/create-target-channel.ts(1 hunks)apps/meteor/tests/e2e/video-conference.spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/utils/create-target-channel.tsapps/meteor/tests/e2e/video-conference.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/utils/create-target-channel.tsapps/meteor/tests/e2e/video-conference.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/video-conference.spec.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/video-conference.spec.ts (1)
apps/meteor/tests/e2e/utils/create-target-channel.ts (6)
createTargetChannel(11-16)createArchivedChannel(104-118)createTargetTeam(59-64)createDirectMessage(70-74)deleteChannel(44-46)deleteTeam(66-68)
🔇 Additional comments (5)
apps/meteor/client/hooks/roomActions/useVideoCallRoomAction.tsx (1)
53-53: LGTM!The addition of
room.archivedto the disabled condition correctly prevents video calls from being initiated in archived rooms, directly addressing the issue described in CORE-1339.apps/meteor/tests/e2e/video-conference.spec.ts (4)
4-12: LGTM!The import of
createArchivedChannelis correctly added and the import structure follows the existing pattern.
22-22: LGTM!Variable declaration follows the existing pattern and naming convention.
28-28: LGTM!Archived channel creation in
beforeAllfollows the existing setup pattern.
33-40: LGTM!The cleanup properly deletes the archived channel using
Promise.allfor efficient parallel cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/video-conference.spec.ts (1)
69-69: Remove unnecessaryasyncfromtest.describe()callbacks.The
test.describe()blocks at lines 69, 87, and 119 haveasync ()callbacks, but describe blocks do not need to be async in Playwright. The async keyword serves no purpose here and deviates from standard Playwright patterns.Apply this diff to remove the unnecessary async keywords:
- test.describe('video conference message block', async () => { + test.describe('video conference message block', () => {- test.describe('verify if user2 received a invite call in targetChannel', async () => { + test.describe('verify if user2 received a invite call in targetChannel', () => {- test.describe('verify if user2 received from a targetTeam', async () => { + test.describe('verify if user2 received from a targetTeam', () => {Also applies to: 87-87, 119-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/tests/e2e/video-conference.spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/video-conference.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/video-conference.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/video-conference.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/tests/e2e/video-conference.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/tests/e2e/video-conference.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/tests/e2e/video-conference.spec.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/video-conference.spec.ts (1)
apps/meteor/tests/e2e/utils/create-target-channel.ts (6)
createTargetChannel(11-16)createArchivedChannel(104-118)createTargetTeam(59-64)createDirectMessage(70-74)deleteChannel(44-46)deleteTeam(66-68)
⏰ 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 / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/tests/e2e/video-conference.spec.ts (6)
4-12: LGTM!The import of
createArchivedChannelis correctly added and used in the test setup.
22-22: LGTM!The
targetArchivedChannelvariable follows the existing naming convention and is properly scoped.
28-28: LGTM!The archived channel is correctly created in the setup phase using the appropriate helper function.
33-40: LGTM!The cleanup is properly implemented with parallel execution via
Promise.all, and correctly includes the new archived channel. This ensures test isolation as per coding guidelines.[Based on coding guidelines]
143-147: LGTM!The test correctly uses the web-first
.toBeDisabled()assertion to verify the video call button is disabled in read-only channels.[Based on coding guidelines]
149-153: LGTM! Test correctly implements the PR objective.The test properly verifies that users cannot initiate video calls from archived rooms, directly addressing issue CORE-1339. The web-first
.toBeDisabled()assertion is correctly used as per coding guidelines.[Based on coding guidelines]
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR fixes an issue where its possible to initiate a video call in archived rooms
Issue(s)
Steps to test or reproduce
Further comments
CORE-1339
Summary by CodeRabbit
Bug Fixes
Tests
Chores