Skip to content

Conversation

@gabriellsh
Copy link
Member

@gabriellsh gabriellsh commented Dec 8, 2025

Proposed changes (including videos or screenshots)

This payload generation will also be used to generate the blocks for the history contextual bar, since the message might not be available (for example for external calls there are no emitted messages).

Issue(s)

VGA-101

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Refactor

    • Moved media-call message payload logic into a shared VoIP package and exposed it through the package public API.
  • Tests

    • Updated test configuration to exclude server-side media-call specs from the test suite.
  • Chores

    • Added and aligned package dependencies needed to support the shared VoIP functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

@gabriellsh gabriellsh requested a review from a team as a code owner December 8, 2025 22:05
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Dec 8, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Dec 8, 2025

⚠️ No Changeset found

Latest commit: 6f5411e

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 Dec 8, 2025

Walkthrough

Moved getHistoryMessagePayload into the ui-voip package, exported it from packages/ui-voip, updated the service import to use the package export, added package dependencies, and removed the media-call server spec from the server Jest config.

Changes

Cohort / File(s) Summary
Package manifest
packages/ui-voip/package.json
Added devDependencies: @rocket.chat/core-typings, @rocket.chat/ui-kit, date-fns (~4.1.0) and peerDependency: date-fns (*).
Public API export
packages/ui-voip/src/index.ts
Exported getHistoryMessagePayload from ./ui-kit/getHistoryMessagePayload.
Import update
apps/meteor/server/services/media-call/service.ts
Replaced local import (./getHistoryMessagePayload) with @rocket.chat/ui-voip/dist/ui-kit/getHistoryMessagePayload.
Test config change
apps/meteor/jest.config.ts
Removed the testMatch entry that excluded <rootDir>/server/services/media-call/**.spec.ts from the server test suite.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Check that the new package export path resolves correctly in built artifacts and TS path mappings.
  • Verify date-fns version and peerDependency alignment across workspace to avoid duplicate versions.
  • Confirm no missing tests due to the Jest config change and that exclusion was intentional.

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • tassoevan
  • ggazzo
  • dougfabris

Poem

🐰 I hopped a bit of code today,
A payload moved to find new play,
Exports set and tests trimmed down,
Quiet changes, not a frown —
Hooray for tidy modules, hooray!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive All changes are directly related to moving getHistoryMessagePayload to ui-voip. The removal of server test config entry appears related to test restructuring but warrants clarification on necessity. Clarify why the testMatch entry removal from jest.config.ts is necessary for this refactoring and whether tests were relocated.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely describes the main change: moving the getHistoryMessagePayload function to the ui-voip package, which aligns with the changeset.
Linked Issues check ✅ Passed The PR successfully relocates getHistoryMessagePayload to ui-voip package by exporting it from index.ts, updating the import in service.ts, and adding required dependencies, meeting the VGA-101 objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/moveHistoryPayloadGetter

📜 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 237360d and 393265d.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (1)
  • packages/ui-voip/package.json (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/ui-voip/package.json
⏰ 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: CodeQL-Build
  • GitHub Check: CodeQL-Build

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.

Copy link
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: 0

🧹 Nitpick comments (1)
apps/meteor/server/services/media-call/service.ts (1)

15-15: Consider importing via the package’s public entrypoint instead of /dist

The new import keeps behavior unchanged, but hard‑coding @rocket.chat/ui-voip/dist/ui-kit/getHistoryMessagePayload ties this service to the internal build layout of ui-voip. If the package exposes this symbol via its main entry (or a stable subpath), consider switching to that instead to reduce coupling:

-import { getHistoryMessagePayload } from '@rocket.chat/ui-voip/dist/ui-kit/getHistoryMessagePayload';
+import { getHistoryMessagePayload } from '@rocket.chat/ui-voip/dist/ui-kit/getHistoryMessagePayload';
+// or, if supported:
+// import { getHistoryMessagePayload } from '@rocket.chat/ui-voip';

Please adjust only if the build/export setup already supports a stable public path.

📜 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 dc8ecd3 and 237360d.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • apps/meteor/jest.config.ts (0 hunks)
  • apps/meteor/server/services/media-call/service.ts (1 hunks)
  • packages/ui-voip/package.json (4 hunks)
  • packages/ui-voip/src/index.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/meteor/jest.config.ts
🧰 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/server/services/media-call/service.ts
  • packages/ui-voip/src/index.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.

Applied to files:

  • apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.

Applied to files:

  • apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.

Applied to files:

  • packages/ui-voip/package.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests

Applied to files:

  • packages/ui-voip/package.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files

Applied to files:

  • packages/ui-voip/package.json
⏰ 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: CodeQL-Build
🔇 Additional comments (2)
packages/ui-voip/src/index.ts (1)

6-7: Public export of getHistoryMessagePayload looks consistent

Re-exporting getHistoryMessagePayload from the package index keeps the public surface centralized and matches the new shared usage intent.

packages/ui-voip/package.json (1)

32-32: Dependency wiring for typings/UI kit/date‑fns is consistent with existing pattern

Adding @rocket.chat/core-typings and @rocket.chat/ui-kit as devDependencies and date-fns as both devDependency and peerDependency aligns with how this package already treats shared runtime libs (e.g., React). Just make sure that:

  • Any @rocket.chat/ui-kit and @rocket.chat/core-typings imports in this package are type‑only or test‑only; if you depend on their runtime at library consumers, they should also be declared as peerDependencies.
  • Consumers of @rocket.chat/ui-voip declare a compatible date-fns version to satisfy the new peer requirement.

If all current imports are type‑only and tests pass in dependent apps, this setup looks good.

Also applies to: 46-46, 64-64, 87-87

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +12MiB
rocketchat 360MiB 349MiB +12MiB
omnichannel-transcript-service 132MiB 132MiB -9B
queue-worker-service 132MiB 132MiB +4B
ddp-streamer-service 126MiB 126MiB -1.3KiB
account-service 113MiB 113MiB -744B
stream-hub-service 111MiB 111MiB -492B
authorization-service 111MiB 111MiB -100B
presence-service 111MiB 111MiB -877B

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 17:27", "12/09 17:41 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 16 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.2GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37738
  • Baseline: develop
  • Timestamp: 2025-12-09 17:41:04 UTC
  • Historical data points: 16

Updated: Tue, 09 Dec 2025 17:41:10 GMT

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.73%. Comparing base (06991bc) to head (6f5411e).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37738      +/-   ##
===========================================
+ Coverage    67.71%   67.73%   +0.02%     
===========================================
  Files         3449     3448       -1     
  Lines       113927   113898      -29     
  Branches     20915    20910       -5     
===========================================
+ Hits         77143    77154      +11     
+ Misses       34661    34624      -37     
+ Partials      2123     2120       -3     
Flag Coverage Δ
e2e 57.33% <ø> (+0.02%) ⬆️
e2e-api 42.31% <ø> (+0.10%) ⬆️

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.

@gabriellsh gabriellsh added the stat: QA assured Means it has been tested and approved by a company insider label Dec 9, 2025
@gabriellsh gabriellsh added this to the 7.14.0 milestone Dec 9, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 9, 2025
@kodiakhq kodiakhq bot merged commit 1a9225e into develop Dec 9, 2025
86 of 88 checks passed
@kodiakhq kodiakhq bot deleted the chore/moveHistoryPayloadGetter branch December 9, 2025 18:57
@dougfabris dougfabris modified the milestones: 7.14.0, 8.0.0 Jan 19, 2026
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.

4 participants