Skip to content

feat: integrate SIP calls with DM rooms#40752

Open
pierre-lehnen-rc wants to merge 2 commits into
developfrom
feat/sip-dm
Open

feat: integrate SIP calls with DM rooms#40752
pierre-lehnen-rc wants to merge 2 commits into
developfrom
feat/sip-dm

Conversation

@pierre-lehnen-rc
Copy link
Copy Markdown
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Jun 1, 2026

Proposed changes (including videos or screenshots)

Issue(s)

DMV-32

Steps to test or reproduce

Further comments

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Enhanced media call room detection for more reliable participant matching
    • Fixed conditional display of voice call controls (hold, screen-sharing) based on available capabilities
    • Improved contact identification and information handling for media calls
    • Refined voice call test scenarios for better call workflow handling

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Jun 1, 2026

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

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 1, 2026

⚠️ No Changeset found

Latest commit: e3552ae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Walkthrough

This PR extends the media-calls contact system by adding user identity (uid) tracking across type definitions, server contact enrichment, and client-side peer derivation. The changes refactor media call room detection to use bidirectional identity matching, remove feature-gate complexity from room wrapping logic, and add feature-aware rendering for UI controls. E2E tests are updated to ensure stable call widget behavior.

Changes

Media Calls Contact Identity & UI

Layer / File(s) Summary
Contact type extensions
packages/core-typings/src/mediaCalls/IMediaCall.ts, packages/media-signaling/src/definition/call/common.ts
Extended MediaCallContactInformation and CallContact types with optional uid?: string field to support user identity tracking throughout the contact lifecycle.
Server contact enrichment
ee/packages/media-calls/src/server/CastDirector.ts
Server-side contact building now populates the new uid field from user _id when constructing contact lists for both user and SIP entries.
Client peer info derivation
packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts
Updated peer derivation to validate user contacts by contact.uid, derive userId primarily from uid with fallback to id, and route SIP contacts to external mapping only when uid is present.
Media call room detection
apps/meteor/client/views/room/body/MediaCallRoom.tsx
Refactored isMediaCallRoom predicate to use bidirectional list-equality for room uids validation against both current and peer user IDs; updated MediaCallRoom component to fetch current user ID and remove screen-share feature gate from wrapper logic.
Feature-aware UI controls
packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
Added feature detection via usePeekMediaSessionFeatures hook to conditionally render "Hold" and "Share_screen" toggle buttons only when corresponding features (hold, screen-share) are available; tightened peerInfo early-return validation.
E2E test scenario updates
apps/meteor/tests/e2e/voice-calls-ee.spec.ts
Updated enterprise voice-calls tests to navigate back to home screen after opening DM and call widget before initiating calls, improving test stability for active call controls and call-transfer scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

type: feature

Suggested reviewers

  • KevLehman
  • tassoevan
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: integrate SIP calls with DM rooms' accurately and concisely summarizes the main objective of the changeset—integrating SIP calling functionality with direct message rooms across multiple components and type definitions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • DMV-32: Request failed with status code 401

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 18.18182% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.73%. Comparing base (0ef1002) to head (e3552ae).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #40752      +/-   ##
===========================================
- Coverage    69.76%   69.73%   -0.03%     
===========================================
  Files         3327     3327              
  Lines       123134   123145      +11     
  Branches     21987    21923      -64     
===========================================
- Hits         85904    85880      -24     
- Misses       33869    33910      +41     
+ Partials      3361     3355       -6     
Flag Coverage Δ
e2e 59.32% <20.00%> (+<0.01%) ⬆️
e2e-api 46.11% <ø> (-0.89%) ⬇️
unit 70.47% <14.28%> (-0.03%) ⬇️

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

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

@pierre-lehnen-rc pierre-lehnen-rc added this to the POC-VOICE milestone Jun 2, 2026
@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review June 2, 2026 14:52
@pierre-lehnen-rc pierre-lehnen-rc requested review from a team as code owners June 2, 2026 14:52
@coderabbitai coderabbitai Bot added the type: feature Pull requests that introduces new feature label Jun 2, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts (1)

28-34: ⚡ Quick win

Consider adding comments to explain uid-based routing logic.

The routing logic at line 29 implements a non-obvious pattern: SIP contacts with uid are routed to internal derivation (user-based) while SIP contacts without uid go to external derivation (phone-number-based). This distinction is central to the DM-SIP integration feature but isn't documented in the code.

📋 Suggested documentation
 export const derivePeerInfoFromInstanceContact = (contact: CallContact) => {
+	// Route SIP contacts without uid to external (phone number-based) derivation.
+	// SIP contacts with uid represent user-backed contacts and use internal (user-based) derivation.
 	if (contact.type === 'sip' && !contact.uid) {
 		return deriveExternalPeerInfoFromInstanceContact(contact);
 	}
🤖 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 `@packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts` around lines
28 - 34, Add a concise comment above derivePeerInfoFromInstanceContact
explaining the uid-based routing: clarify that when contact.type === 'sip' and
contact.uid is absent the function routes to
deriveExternalPeerInfoFromInstanceContact (treat as external phone-number-based
SIP peers), whereas SIP contacts with a uid and all non-SIP contacts are routed
to deriveInternalPeerInfoFromInstanceContact (treat as internal/user-based
routing); reference the CallContact shape and the uid field in the comment so
future readers understand this DM-SIP integration distinction.
apps/meteor/tests/e2e/voice-calls-ee.spec.ts (1)

61-67: 💤 Low value

Consider incorporating comment explanations into test.step descriptions.

The inline comments on lines 61 and 67 explain the navigation flow and rationale, which is valuable context. However, as per coding guidelines, TypeScript files should avoid inline comments. Consider moving this explanation into a more descriptive test.step() description or splitting into separate steps for better clarity.

For example:

await test.step('open voice call widget with user2', async () => {
  await user1.poHomeChannel.navbar.openChat('user2');
  await expect(user1.poHomeChannel.composer.inputMessage).toBeVisible();
  await user1.poHomeChannel.content.btnVoiceCall.click();
});

await test.step('navigate to home to establish call', async () => {
  await user1.poHomeChannel.navbar.btnHome.click();
  await user1.poHomeChannel.waitForHome();
  await user1.poHomeChannel.voiceCalls.initiateCall();
});

This approach maintains consistency with the transfer test scenario (lines 118-125) which follows a similar pattern without inline comments.

As per coding guidelines, TypeScript files should 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/voice-calls-ee.spec.ts` around lines 61 - 67, Convert
the inline comments into explicit test.step blocks: wrap the sequence that opens
the DM and clicks the voice call button (calls to
user1.poHomeChannel.navbar.openChat, user1.poHomeChannel.composer.inputMessage,
user1.poHomeChannel.content.btnVoiceCall) in a test.step with a descriptive
title like "open voice call widget with user2", and wrap the navigation back to
home (user1.poHomeChannel.navbar.btnHome) plus any follow-up actions (e.g.,
user1.poHomeChannel.waitForHome and user1.poHomeChannel.voiceCalls.initiateCall)
in a second test.step titled like "navigate to home to establish call"; ensure
each step replaces the comment and preserves existing awaits and expectations.
🤖 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/body/MediaCallRoom.tsx`:
- Around line 11-23: The isSameList function currently misses a length check
causing subset arrays to be considered equal; update isSameList to first compare
list1.length and list2.length and return false if they differ, then keep a
single loop (e.g., iterate over list1 and ensure list2.includes(item)) or switch
to Set-based equality to avoid duplicate/ordering issues; reference the
isSameList function and remove the redundant second loop after adding the length
check.

In `@apps/meteor/tests/e2e/voice-calls-ee.spec.ts`:
- Line 122: After clicking user1.poHomeChannel.navbar.btnHome.click(), wait for
the home page to be visible before calling initiateCall to avoid races; add an
await on a home-page visibility assertion (e.g., await
user1.poHomeChannel.home.waitForDisplayed() or the appropriate page/component
selector on the PO) and only then call user1.clientA.initiateCall(), ensuring
the await is added so initiateCall runs after the home page is rendered.
- Line 65: After the click on user1.poHomeChannel.navbar.btnHome, add an
explicit await that waits for the home page UI to be visible before calling
initiateCall(); for example, await a stable home-root selector via
user1.page.waitForSelector(...) or call a PO helper like
user1.poHome.waitForVisible(), then proceed to initiateCall()—do not use
hardcoded timeouts.

In `@packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts`:
- Around line 15-26: The thrown Error in
deriveInternalPeerInfoFromInstanceContact is misleading because the function
rejects only when contact.type !== 'user' AND no contact.uid; update the error
message to accurately reflect that (e.g., "Contact must be a user or have a
uid") so it describes the actual validation (check of contact.type and
contact.uid) before returning the derived object.

---

Nitpick comments:
In `@apps/meteor/tests/e2e/voice-calls-ee.spec.ts`:
- Around line 61-67: Convert the inline comments into explicit test.step blocks:
wrap the sequence that opens the DM and clicks the voice call button (calls to
user1.poHomeChannel.navbar.openChat, user1.poHomeChannel.composer.inputMessage,
user1.poHomeChannel.content.btnVoiceCall) in a test.step with a descriptive
title like "open voice call widget with user2", and wrap the navigation back to
home (user1.poHomeChannel.navbar.btnHome) plus any follow-up actions (e.g.,
user1.poHomeChannel.waitForHome and user1.poHomeChannel.voiceCalls.initiateCall)
in a second test.step titled like "navigate to home to establish call"; ensure
each step replaces the comment and preserves existing awaits and expectations.

In `@packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts`:
- Around line 28-34: Add a concise comment above
derivePeerInfoFromInstanceContact explaining the uid-based routing: clarify that
when contact.type === 'sip' and contact.uid is absent the function routes to
deriveExternalPeerInfoFromInstanceContact (treat as external phone-number-based
SIP peers), whereas SIP contacts with a uid and all non-SIP contacts are routed
to deriveInternalPeerInfoFromInstanceContact (treat as internal/user-based
routing); reference the CallContact shape and the uid field in the comment so
future readers understand this DM-SIP integration distinction.
🪄 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: 722df00b-7928-47d7-a21d-e4f8c8fd38c8

📥 Commits

Reviewing files that changed from the base of the PR and between 0ef1002 and e3552ae.

📒 Files selected for processing (7)
  • apps/meteor/client/views/room/body/MediaCallRoom.tsx
  • apps/meteor/tests/e2e/voice-calls-ee.spec.ts
  • ee/packages/media-calls/src/server/CastDirector.ts
  • packages/core-typings/src/mediaCalls/IMediaCall.ts
  • packages/media-signaling/src/definition/call/common.ts
  • packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts
  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
📜 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). (2)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Hacktron Security Check
🧰 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:

  • packages/media-signaling/src/definition/call/common.ts
  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
  • ee/packages/media-calls/src/server/CastDirector.ts
  • packages/core-typings/src/mediaCalls/IMediaCall.ts
  • apps/meteor/tests/e2e/voice-calls-ee.spec.ts
  • packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts
  • apps/meteor/client/views/room/body/MediaCallRoom.tsx
**/*.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.ts extension for test files (e.g., login.spec.ts)

Files:

  • apps/meteor/tests/e2e/voice-calls-ee.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 in apps/meteor/tests/e2e/ directory
Avoid using page.locator() in Playwright tests - always prefer semantic locators such as page.getByRole(), page.getByLabel(), page.getByText(), or page.getByTitle()
Use test.beforeAll() and test.afterAll() for setup/teardown in Playwright tests
Use test.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
Use expect matchers for assertions (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements in Playwright tests
Use page.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/voice-calls-ee.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/voice-calls-ee.spec.ts
🧠 Learnings (9)
📚 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:

  • packages/media-signaling/src/definition/call/common.ts
  • ee/packages/media-calls/src/server/CastDirector.ts
  • packages/core-typings/src/mediaCalls/IMediaCall.ts
  • apps/meteor/tests/e2e/voice-calls-ee.spec.ts
  • packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.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:

  • packages/media-signaling/src/definition/call/common.ts
  • ee/packages/media-calls/src/server/CastDirector.ts
  • packages/core-typings/src/mediaCalls/IMediaCall.ts
  • apps/meteor/tests/e2e/voice-calls-ee.spec.ts
  • packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.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:

  • packages/media-signaling/src/definition/call/common.ts
  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
  • ee/packages/media-calls/src/server/CastDirector.ts
  • packages/core-typings/src/mediaCalls/IMediaCall.ts
  • apps/meteor/tests/e2e/voice-calls-ee.spec.ts
  • packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts
  • apps/meteor/client/views/room/body/MediaCallRoom.tsx
📚 Learning: 2026-02-26T19:22:29.385Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:29.385Z
Learning: For TSX files in the UI VOIP package, ensure that when a media session state is 'unavailable', the voiceCall action is excluded from the actions object passed to CallHistoryActions so it does not render in the menu. This filtering should occur upstream (before getItems is called) to avoid tooltips or UI hints for unavailable actions. If there are multiple actions with availability states, implement a centralized helper to filter actions based on session state.

Applied to files:

  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
📚 Learning: 2026-05-05T12:34:29.042Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 40331
File: packages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx:69-69
Timestamp: 2026-05-05T12:34:29.042Z
Learning: In Rocket.Chat’s `packages/ui-voip` UI (e.g., media/call widgets), voice/media calls are only supported in Direct Message (DM) rooms. Rocket.Chat models a DM as a “room” with exactly two participants, so handlers like `onClickDirectMessage` are the correct destination—even when the UI text/element says “Open in room” (e.g., on the shared screen card/`StreamCard`). During review, don’t flag a “DM vs room” mismatch for these cases; they intentionally map to the same destination.

Applied to files:

  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
📚 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:

  • packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx
  • apps/meteor/client/views/room/body/MediaCallRoom.tsx
📚 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/voice-calls-ee.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/voice-calls-ee.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/voice-calls-ee.spec.ts
🔇 Additional comments (5)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)

23-23: LGTM!

packages/media-signaling/src/definition/call/common.ts (1)

8-8: LGTM!

ee/packages/media-calls/src/server/CastDirector.ts (1)

123-123: LGTM!

apps/meteor/client/views/room/body/MediaCallRoom.tsx (1)

3-3: LGTM!

Also applies to: 25-37, 46-49

packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx (1)

20-20: LGTM!

Also applies to: 82-89, 147-164

Comment thread apps/meteor/client/views/room/body/MediaCallRoom.tsx
await user1.poHomeChannel.navbar.openChat('user2');
await expect(user1.poHomeChannel.composer.inputMessage).toBeVisible();
await user1.poHomeChannel.content.btnVoiceCall.click();
await user1.poHomeChannel.navbar.btnHome.click();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wait for home page to be visible after navigation.

After clicking btnHome, the test should wait for the home page to render before initiating the call. Without an explicit wait, initiateCall() on line 68 may execute before the home UI is ready, causing flaky test failures.

Based on learnings, prefer page.waitFor() with specific conditions instead of hardcoded timeouts in Playwright tests.

🔧 Proposed fix to add home page wait
 await user1.poHomeChannel.content.btnVoiceCall.click();
 await user1.poHomeChannel.navbar.btnHome.click();
+await user1.poHomeChannel.waitForHome();

 // Initiate the call from the home screen so the DM doesn't affect how its rendered
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await user1.poHomeChannel.navbar.btnHome.click();
await user1.poHomeChannel.navbar.btnHome.click();
await user1.poHomeChannel.waitForHome();
🤖 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/voice-calls-ee.spec.ts` at line 65, After the click on
user1.poHomeChannel.navbar.btnHome, add an explicit await that waits for the
home page UI to be visible before calling initiateCall(); for example, await a
stable home-root selector via user1.page.waitForSelector(...) or call a PO
helper like user1.poHome.waitForVisible(), then proceed to initiateCall()—do not
use hardcoded timeouts.

await user1.poHomeChannel.navbar.openChat('user2');
await expect(user1.poHomeChannel.composer.inputMessage).toBeVisible();
await user1.poHomeChannel.content.btnVoiceCall.click();
await user1.poHomeChannel.navbar.btnHome.click();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wait for home page to be visible after navigation.

Similar to line 65, after clicking btnHome, the test should wait for the home page to render before calling initiateCall() on line 123. This prevents potential race conditions and test flakiness.

🔧 Proposed fix to add home page wait
 await user1.poHomeChannel.content.btnVoiceCall.click();
 await user1.poHomeChannel.navbar.btnHome.click();
+await user1.poHomeChannel.waitForHome();
 await user1.poHomeChannel.voiceCalls.initiateCall();
🤖 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/voice-calls-ee.spec.ts` at line 122, After clicking
user1.poHomeChannel.navbar.btnHome.click(), wait for the home page to be visible
before calling initiateCall to avoid races; add an await on a home-page
visibility assertion (e.g., await user1.poHomeChannel.home.waitForDisplayed() or
the appropriate page/component selector on the PO) and only then call
user1.clientA.initiateCall(), ensuring the await is added so initiateCall runs
after the home page is rendered.

Comment thread packages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.ts
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 7 files

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add-to-poc type: feature Pull requests that introduces new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant