Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Oct 6, 2025

Proposed changes (including videos or screenshots)

Issue(s)

Related to VGA-9

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features
    • Calls now display who transferred them, providing clearer context during handoffs.
    • Transfer details are preserved across related call events, ensuring consistent information for both caller and callee.
    • Enhanced call data in new call notifications and ongoing call views to include transfer origin when available.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 6, 2025

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

  • This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2025

⚠️ No Changeset found

Latest commit: 2065a63

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
Contributor

coderabbitai bot commented Oct 6, 2025

Walkthrough

Adds transfer attribution to media calls. Introduces getNewCallTransferredBy, computes transferredBy for new/existing call signals, conditionally includes parentCallId and transferredBy in server-emitted payloads, extends public signal and client call types with transferredBy, and stores/exposes it in the client Call during remote initialization.

Changes

Cohort / File(s) Summary
Transfer attribution utility
ee/packages/media-calls/src/server/getNewCallTransferredBy.ts
Adds helper to derive who transferred a call, returning CallContact or null; exported as named function.
Signal emission updates
ee/packages/media-calls/src/internal/SignalProcessor.ts, ee/packages/media-calls/src/internal/agents/UserActorAgent.ts
Imports and uses getNewCallTransferredBy; computes transferredBy and conditionally spreads it and replacingCallId/parentCallId into various new/existing call signal payloads.
Public signaling types
packages/media-signaling/src/definition/call/IClientMediaCall.ts, packages/media-signaling/src/definition/signals/server/new.ts
Adds transferredBy to IClientMediaCall (nullable) and as optional field on ServerMediaSignalNewCall.
Client call tracking
packages/media-signaling/src/lib/Call.ts
Adds private _transferredBy, public getter, initializes to null, and sets from incoming new-call signal during remote call initialization.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as UserActorAgent
  participant SP as SignalProcessor
  participant Util as getNewCallTransferredBy
  participant C as Client (ClientMediaCall)

  rect rgb(240,248,255)
    U->>Util: getNewCallTransferredBy(call)
    Util-->>U: transferredBy | null
    U->>SP: emit NewCall signal { ..., parentCallId?, transferredBy? }
  end

  Note over SP: Build payloads for caller/callee variants

  SP-->>C: ServerMediaSignalNewCall { ..., transferredBy? }

  rect rgb(245,255,250)
    C->>C: initializeRemoteCall(signal)
    C->>C: this._transferredBy = signal.transferredBy || null
    C-->>C: getter transferredBy exposes value
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my ears at calls that hop,
A transfer tag—plop, plop, plop!
Now trails are clear where handoffs fly,
Who passed the carrot? “TransferredBy.”
I thump the ground, the signal sings,
A tidy log of rabbit things. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly captures the main change by stating that the client will now receive the contact information of the user who requested a call transfer, directly reflecting the core implementation in this PR without extraneous details or noise.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/media-call-identify-who-transferred

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 287f302 and 8ebc923.

📒 Files selected for processing (6)
  • ee/packages/media-calls/src/internal/SignalProcessor.ts (6 hunks)
  • ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (3 hunks)
  • ee/packages/media-calls/src/server/getNewCallTransferredBy.ts (1 hunks)
  • packages/media-signaling/src/definition/call/IClientMediaCall.ts (1 hunks)
  • packages/media-signaling/src/definition/signals/server/new.ts (1 hunks)
  • packages/media-signaling/src/lib/Call.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/media-signaling/src/definition/signals/server/new.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • CallContact (7-15)
ee/packages/media-calls/src/server/getNewCallTransferredBy.ts (2)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
  • IMediaCall (35-63)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • CallContact (7-15)
packages/media-signaling/src/lib/Call.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • CallContact (7-15)
ee/packages/media-calls/src/internal/SignalProcessor.ts (2)
packages/media-signaling/src/lib/Call.ts (1)
  • transferredBy (83-85)
ee/packages/media-calls/src/server/getNewCallTransferredBy.ts (1)
  • getNewCallTransferredBy (4-20)
ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (2)
packages/media-signaling/src/lib/Call.ts (1)
  • transferredBy (83-85)
ee/packages/media-calls/src/server/getNewCallTransferredBy.ts (1)
  • getNewCallTransferredBy (4-20)
⏰ 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: 📦 Build Packages
  • GitHub Check: Builds matrix rust bindings against alpine
🔇 Additional comments (11)
packages/media-signaling/src/definition/signals/server/new.ts (1)

18-19: LGTM!

The addition of the optional transferredBy field is well-documented and appropriately typed. The comment clearly explains when this field will be populated.

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

80-80: LGTM!

The public API extension is well-typed and consistent with the server signal payload. Using null instead of undefined is appropriate since the field is always explicitly initialized.

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

4-20: LGTM!

The logic correctly identifies third-party transfer initiators by excluding the caller and callee from the new call. The implementation is clean and the conditional checks properly handle cases where no transfer occurred.

ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (2)

8-8: LGTM!

The import of the new utility function is appropriate for this module's needs.


171-183: LGTM!

The implementation correctly computes and conditionally includes transferredBy in the signal payload. The pattern is consistent with other optional fields like replacingCallId and requestedCallId.

ee/packages/media-calls/src/internal/SignalProcessor.ts (3)

17-17: LGTM!

The import is correctly placed and used throughout the file.


147-184: LGTM!

The transferredBy computation is correctly placed before the signal emissions and efficiently reused for both caller and callee roles. The conditional spreading pattern is consistent with replacingCallId.


278-295: LGTM!

The implementation correctly computes and includes transferredBy for existing requested calls, following the same pattern used in reactToUnknownCall.

packages/media-signaling/src/lib/Call.ts (3)

81-85: LGTM!

The private field and public getter follow the established pattern for immutable state in this class.


201-201: LGTM!

Proper initialization of the field to null in the constructor.


265-265: LGTM!

The field is correctly populated from the remote signal. Using || null ensures consistent null handling for missing values.


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.

@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review October 6, 2025 18:51
@codecov
Copy link

codecov bot commented Oct 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.44%. Comparing base (6d605e6) to head (2065a63).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37150      +/-   ##
===========================================
+ Coverage    67.42%   67.44%   +0.02%     
===========================================
  Files         3289     3287       -2     
  Lines       111887   111747     -140     
  Branches     20437    20404      -33     
===========================================
- Hits         75438    75373      -65     
+ Misses       33763    33688      -75     
  Partials      2686     2686              
Flag Coverage Δ
e2e 57.30% <ø> (-0.02%) ⬇️
unit 71.45% <ø> (+<0.01%) ⬆️

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 7.12.0 milestone Oct 6, 2025
@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Oct 13, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 13, 2025
@pierre-lehnen-rc pierre-lehnen-rc added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Oct 14, 2025
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Oct 14, 2025
@pierre-lehnen-rc pierre-lehnen-rc added stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider and removed stat: QA assured Means it has been tested and approved by a company insider labels Oct 14, 2025
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Oct 14, 2025
@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Oct 14, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 14, 2025
@kodiakhq kodiakhq bot merged commit 40e68e5 into develop Oct 14, 2025
49 checks passed
@kodiakhq kodiakhq bot deleted the chore/media-call-identify-who-transferred branch October 14, 2025 19:46
antm-rp pushed a commit to antm-rp/Rocket.Chat that referenced this pull request Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants