Skip to content

Conversation

@gabriellsh
Copy link
Member

@gabriellsh gabriellsh commented Nov 12, 2025

Proposed changes (including videos or screenshots)

Dispatch desktop notifications for incoming calls.

Consumes the API declared here

Issue(s)

VGA-61

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features
    • Native desktop notifications for incoming VoIP calls with caller name, avatar and persistent interaction while ringing.
  • Localization
    • Added English text for incoming call notification: "Incoming call...".
  • Chores
    • Release metadata updated to include the desktop notifications feature.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 12, 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 Nov 12, 2025

🦋 Changeset detected

Latest commit: 8170d16

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/i18n Minor
@rocket.chat/ui-voip Major
@rocket.chat/meteor Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-client Major
@rocket.chat/uikit-playground Patch
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/queue-worker Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/http-router Patch
@rocket.chat/model-typings Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/presence-service Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

Walkthrough

Adds desktop notification support for VoIP: new hook to dispatch desktop notifications on ringing, avatar→PNG conversion utility, desktop-api dependency and Window typing, session shape extended with callId, provider integration, story/provider renames, and a new English localization key.

Changes

Cohort / File(s) Summary
Localization
packages/i18n/src/locales/en.i18n.json
Added Incoming_call_ellipsis: "Incoming call..."
Desktop API dependency & globals
packages/ui-voip/package.json, packages/ui-voip/src/global.d.ts
Added dependency @rocket.chat/desktop-api; augmented Window with optional RocketChatDesktop?: IRocketChatDesktop
Session shape & derivation
packages/ui-voip/src/v2/useMediaSessionInstance.ts, packages/ui-voip/src/v2/useMediaSession.ts
Added callId to EmptySession (undefined) and CallSession (string); updated deriveWidgetStateFromCallState signature to include callRole and refined return type
Desktop notifications hook & provider integration
packages/ui-voip/src/v2/useDesktopNotifications.ts, packages/ui-voip/src/v2/MediaCallProvider.tsx
New useDesktopNotifications(session) hook dispatches desktop voice notifications on ringing; integrated into MediaCallProvider and subscribes to session changes
Avatar conversion utility
packages/ui-voip/src/v2/utils/convertAvatarUrlToPng.ts
Added convertAvatarUrlToPng(avatarUrl) returning PNG data URL or empty string using canvas rendering and crossOrigin handling
Type refinements (ReactNode)
packages/ui-voip/src/v2/MediaCallProvider.tsx, .../MockedMediaCallProvider.tsx, .../components/Widget/Widget.tsx, .../useKeypad.tsx, stories
Replaced inline React.ReactNode with imported ReactNode; introduced small props type aliases
Stories & mocks
packages/ui-voip/src/v2/*.stories.tsx, packages/ui-voip/src/v2/MockedMediaCallProvider.tsx
Renamed default/mock provider from MediaCallProviderMockMockedMediaCallProvider and updated imports/usages in multiple stories
Changeset
.changeset/sixty-numbers-tan.md
Added changeset noting version bumps and native desktop notifications for voice calls

Sequence Diagram(s)

sequenceDiagram
    participant Provider as MediaCallProvider
    participant Hook as useDesktopNotifications
    participant Session as Session State
    participant Util as convertAvatarUrlToPng
    participant API as RocketChatDesktop

    Provider->>Hook: useDesktopNotifications(session)
    Hook->>Session: subscribe to session changes

    alt session.state == "ringing"
      Hook->>Util: convertAvatarUrlToPng(avatarUrl)
      Util-->>Hook: pngDataUrl or ""
      Hook->>API: dispatchCustomNotification({type:"voice", title, body, avatar: png, requireInteraction:true})
      API-->>Hook: notification displayed
    else state != "ringing"
      Hook->>Hook: clear previous notification if present
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay extra attention to:
    • packages/ui-voip/src/v2/useDesktopNotifications.ts (effect dependencies, isMounted guard, notification lifecycle, payload shape).
    • packages/ui-voip/src/v2/utils/convertAvatarUrlToPng.ts (crossOrigin, canvas context and error handling).
    • Session propagation of callId (useMediaSessionInstance.ts, useMediaSession.ts).

Possibly related PRs

Suggested reviewers

  • aleksandernsilva
  • tassoevan
  • pierre-lehnen-rc

Poem

🐰 I hopped upon a morning ring,
I painted faces, made them sing,
A tiny ping across the pane,
The caller leaps — I cheer, "Incoming!" 🥕

Pre-merge checks and finishing touches

✅ 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: Media call notifications on desktop app' accurately summarizes the main change: adding desktop notifications for incoming VoIP calls.
Linked Issues check ✅ Passed Code changes successfully integrate the desktop notification API for incoming calls by adding useDesktopNotifications hook, desktop API dependency, and proper session state tracking VGA-61.
Out of Scope Changes check ✅ Passed All changes are within scope; they support desktop notifications or refactoring (ReactNode imports, MockedMediaCallProvider rename) directly related to the feature implementation.
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 feat/voiceNotification

📜 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 3cbb421 and 8170d16.

📒 Files selected for processing (1)
  • packages/i18n/src/locales/en.i18n.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/i18n/src/locales/en.i18n.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). (12)
  • GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
  • GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
  • GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
  • GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
  • GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
  • GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • 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.

@gabriellsh gabriellsh force-pushed the feat/voiceNotification branch from f844808 to 39377f3 Compare November 12, 2025 21:20
@gabriellsh gabriellsh marked this pull request as ready for review November 17, 2025 23:18
@gabriellsh gabriellsh added this to the 7.13.0 milestone Nov 17, 2025
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/ui-voip/src/v2/useMediaSession.ts (1)

9-19: Address the unmapped 'hangup' CallState and add conditional state dispatch

The core concern is valid: deriveWidgetStateFromCallState lacks a case for the 'hangup' CallState value (which exists in the media-signaling union), causing it to return undefined. When dispatched into the reducer payload, this creates a transient state: undefined, which violates the SessionInfo discriminated union type despite the as SessionInfo assertion.

While getMainCall() filters out 'hangup' calls before they reach updateSessionState, the type inconsistency and implicit undefined handling remain fragile. Two practical fixes:

  1. Explicitly map 'hangup' in the switch:

    case 'hangup':
        return undefined; // or map to a sensible state like 'closed'
  2. Conditionally dispatch state only when derived:

    const state = deriveWidgetStateFromCallState(callState, role);
    dispatch({
        type: 'instance_updated',
        payload: { ..., ...(state && { state }), ... }
    });

The callId plumbing is correct and properly leveraged by useDesktopNotifications—it extracts sessionInfo.callId only when state === 'ringing', which the discriminated union guarantees to be a string. The caller/callee mapping for 'none'/'ringing' is accurate.

🧹 Nitpick comments (4)
packages/ui-voip/src/v2/useMediaSessionInstance.ts (1)

22-33: callId threading at the SessionInfo level is consistent

Defining callId: undefined on EmptySession and callId: string on CallSession yields the expected string | undefined surface for SessionInfo.callId and matches how defaultSessionInfo initializes it in useMediaSession.

If you later need to access callId irrespective of session subtype, consider moving it onto BaseSession as callId: string | undefined to avoid repeating it in the subinterfaces.

packages/ui-voip/src/v2/utils/convertAvatarUrlToPng.ts (1)

1-36: Avatar → PNG conversion is robust with safe fallbacks

The utility cleanly handles missing URLs, canvas/context failures, and toDataURL errors by resolving to '', and using crossOrigin = 'anonymous' avoids tainted canvas crashes when CORS isn’t configured.

If you expect slow or broken avatar endpoints, consider adding a simple timeout (e.g., via setTimeout) that resolves to '' if neither onload nor onerror fires, so callers aren’t left awaiting a promise indefinitely.

packages/i18n/src/locales/en.i18n.json (1)

2559-2563: Verify trailing comma after the new entry

Looks good copy-wise and matches sentence case and "..." style used elsewhere. Please double‑check there’s a trailing comma after the new line to keep the JSON valid. If it’s missing, apply:

-  "Incoming_call_ellipsis": "Incoming call..."
+  "Incoming_call_ellipsis": "Incoming call...",

Also, keep sentence case for related notification strings as per prior guidance. Based on learnings.

packages/ui-voip/src/v2/useDesktopNotifications.ts (1)

56-65: Conditionally set notification id and track it only when defined

You currently pass id: sessionInfo.callId and always assign previousCallId.current = sessionInfo.callId, which means you might send id: undefined and never store an ID for closing if callId is absent. Given the desktop API’s contract that CustomNotificationOptions.id is optional and notifications without an ID can’t be closed programmatically, it would be cleaner to only set and track the ID when it’s actually present. Based on learnings.

A small refactor could look like:

-			window.RocketChatDesktop?.dispatchCustomNotification({
-				type: 'voice',
-				id: sessionInfo.callId,
-				payload: {
-					title: displayInfo.title,
-					body: t('Incoming_call_ellipsis'),
-					avatar: avatarAsPng || undefined,
-					requireInteraction: true,
-				},
-			});
+			const callId = sessionInfo.callId;
+
+			window.RocketChatDesktop?.dispatchCustomNotification({
+				type: 'voice',
+				...(callId && { id: callId }),
+				payload: {
+					title: displayInfo.title,
+					body: t('Incoming_call_ellipsis'),
+					avatar: avatarAsPng || undefined,
+					requireInteraction: true,
+				},
+			});

And then only update the ref when you have a real ID:

-		notifyDesktop();
-		previousCallId.current = sessionInfo.callId;
+		notifyDesktop();
+		if (sessionInfo.callId) {
+			previousCallId.current = sessionInfo.callId;
+		}

This keeps the close logic aligned with the desktop API semantics and avoids passing undefined as an ID.

Also applies to: 68-70

📜 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 150efb9 and a8e01a5.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (8)
  • packages/i18n/src/locales/en.i18n.json (1 hunks)
  • packages/ui-voip/package.json (1 hunks)
  • packages/ui-voip/src/global.d.ts (1 hunks)
  • packages/ui-voip/src/v2/MediaCallProvider.tsx (2 hunks)
  • packages/ui-voip/src/v2/useDesktopNotifications.ts (1 hunks)
  • packages/ui-voip/src/v2/useMediaSession.ts (5 hunks)
  • packages/ui-voip/src/v2/useMediaSessionInstance.ts (1 hunks)
  • packages/ui-voip/src/v2/utils/convertAvatarUrlToPng.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.594Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.246Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.
📚 Learning: 2025-11-17T22:38:48.594Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.594Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.

Applied to files:

  • packages/ui-voip/src/v2/useDesktopNotifications.ts
  • packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-17T14:30:36.246Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.246Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.

Applied to files:

  • packages/ui-voip/src/v2/useDesktopNotifications.ts
  • packages/ui-voip/src/global.d.ts
🧬 Code graph analysis (5)
packages/ui-voip/src/v2/useMediaSessionInstance.ts (1)
packages/ui-voip/src/v2/MediaCallContext.ts (1)
  • State (26-26)
packages/ui-voip/src/v2/useDesktopNotifications.ts (2)
packages/ui-voip/src/v2/useMediaSessionInstance.ts (1)
  • SessionInfo (33-33)
packages/ui-voip/src/v2/utils/convertAvatarUrlToPng.ts (1)
  • convertAvatarUrlToPng (1-36)
packages/ui-voip/src/v2/useMediaSession.ts (2)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (2)
  • CallState (21-27)
  • CallRole (17-17)
packages/ui-voip/src/v2/MediaCallContext.ts (1)
  • State (26-26)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
packages/ui-voip/src/v2/useDesktopNotifications.ts (1)
  • useDesktopNotifications (22-75)
packages/ui-voip/src/global.d.ts (2)
packages/desktop-api/src/index.ts (1)
  • IRocketChatDesktop (29-66)
apps/meteor/client/definitions/global.d.ts (1)
  • Window (5-29)
⏰ 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: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
packages/ui-voip/package.json (1)

21-28: Desktop API dependency addition looks correct

Adding @rocket.chat/desktop-api as a workspace dependency aligns with the new IRocketChatDesktop typing in ui-voip and keeps this package ready for desktop integration.

Please confirm that the workspace builds/tests are passing with this new dependency resolved for @rocket.chat/ui-voip.

packages/ui-voip/src/global.d.ts (1)

1-7: Window augmentation for RocketChatDesktop is well‑formed

Type‑only importing IRocketChatDesktop and augmenting Window via declare global matches the desktop API shape and lets ui-voip safely reference window.RocketChatDesktop in this package.

Ensure this global.d.ts is picked up by the ui-voip tsconfig (e.g., via include/files) so the Window augmentation is visible wherever window.RocketChatDesktop is used.

packages/ui-voip/src/v2/MediaCallProvider.tsx (2)

22-22: LGTM!

The import statement is correct and follows the established pattern for local imports.


41-42: The original review comment is incorrect; no actionable issue exists.

The code properly uses TypeScript's discriminated union pattern. SessionInfo is typed as EmptySession | CallSession, where:

  • EmptySession has callId: undefined (when state is 'closed' or 'new')
  • CallSession has callId: string (when state is 'calling', 'ringing', or 'ongoing')

The notification dispatch at line 58 is guarded by the check at line 50 (if (sessionInfo.state !== 'ringing')), which ensures the code only reaches the dispatch when state === 'ringing'. At that point, the type system guarantees we're in a CallSession state, so sessionInfo.callId is always a string, never undefined.

The notification is therefore always dispatched with a valid ID and can be closed programmatically, as required.

packages/ui-voip/src/v2/useDesktopNotifications.ts (1)

22-74: Desktop notification hook wiring looks solid

The overall flow (deriving display info, guarding on ringing state and desktop API presence, async avatar PNG conversion, and closing the previous notification when the call leaves ringing) is coherent and matches the media-call notification requirements. I don’t see correctness issues in the lifecycle handling here.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +12MiB
rocketchat 367MiB 355MiB +12MiB
omnichannel-transcript-service 141MiB 141MiB +1.3KiB
queue-worker-service 141MiB 141MiB +909B
ddp-streamer-service 127MiB 127MiB +680B
account-service 114MiB 114MiB -3.2KiB
stream-hub-service 111MiB 111MiB +36B
authorization-service 111MiB 111MiB -216B
presence-service 111MiB 111MiB -1.4KiB

📊 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 14:28", "11/19 16:07 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [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]
  line "ddp-streamer-service" [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.14]
  line "presence-service" [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.14]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 5 days):

  • 📊 Average: 1.4GiB
  • ⬇️ 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-37492
  • Baseline: develop
  • Timestamp: 2025-11-19 16:07:20 UTC
  • Historical data points: 5

Updated: Wed, 19 Nov 2025 16:07:21 GMT

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)
packages/ui-voip/src/v2/useMediaSession.ts (1)

52-65: Add explicit default case for clarity.

The switch statement implicitly returns undefined for unhandled CallState values, which matches the return type. However, adding an explicit default case would improve code clarity and maintainability.

Apply this diff:

 const deriveWidgetStateFromCallState = (
 	callState: CallState,
 	callRole: CallRole,
 ): Extract<State, 'ongoing' | 'ringing' | 'calling'> | undefined => {
 	switch (callState) {
 		case 'active':
 		case 'accepted':
 		case 'renegotiating':
 			return 'ongoing';
 		case 'none':
 		case 'ringing':
 			return callRole === 'callee' ? 'ringing' : 'calling';
+		default:
+			return undefined;
 	}
 };
📜 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 a8e01a5 and 3336522.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • .changeset/sixty-numbers-tan.md (1 hunks)
  • packages/ui-voip/src/v2/MediaCallProvider.tsx (2 hunks)
  • packages/ui-voip/src/v2/useMediaSession.ts (5 hunks)
  • packages/ui-voip/src/v2/useMediaSessionInstance.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/ui-voip/src/v2/MediaCallProvider.tsx
  • packages/ui-voip/src/v2/useMediaSessionInstance.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.

Applied to files:

  • .changeset/sixty-numbers-tan.md
📚 Learning: 2025-11-17T14:30:36.271Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.271Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.

Applied to files:

  • .changeset/sixty-numbers-tan.md
⏰ 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: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
.changeset/sixty-numbers-tan.md (1)

1-6: LGTM! Changeset is well-formed.

The changeset correctly declares minor version bumps for the affected packages and provides a clear description of the feature.

packages/ui-voip/src/v2/useMediaSession.ts (4)

11-11: LGTM! callId field added to default session state.

The addition of callId: undefined to defaultSessionInfo correctly establishes the default value for the new session identifier field used by desktop notifications.


150-161: LGTM! callId properly extracted from mainCall.

The destructuring correctly extracts callId from the main call object, making it available for inclusion in the session state updates.


167-182: LGTM! callId correctly included in SIP contact state updates.

The callId is properly threaded through to the dispatch payload for SIP contacts, maintaining consistency with the session state structure.


206-209: SessionInfo type and callId integration verified—no issues found.

The SessionInfo type correctly includes the callId field in both EmptySession (as undefined) and CallSession (as string). The desktop notification integration in useDesktopNotifications.ts already properly handles callId across lines 58, 69, and 74. The dispatch change at lines 206-209 is type-safe and consistent with existing patterns.

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 (2)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)

45-46: Consider error handling for desktop notifications.

The integration looks correct. However, reviewing useDesktopNotifications.ts (lines 27-46), the async notifyDesktop() function lacks error handling. If convertAvatarUrlToPng throws or the notification dispatch fails, errors will be silently swallowed.

Consider wrapping the async logic in a try-catch block in useDesktopNotifications.ts:

const notifyDesktop = async () => {
  try {
    const avatarAsPng = await convertAvatarUrlToPng(displayInfo.avatar);
    
    if (!isMounted) {
      return;
    }
    
    window.RocketChatDesktop?.dispatchCustomNotification({
      type: 'voice',
      id: sessionInfo.callId,
      payload: {
        title: displayInfo.title,
        body: t('Incoming_call_ellipsis'),
        avatar: avatarAsPng || undefined,
        requireInteraction: true,
      },
    });
  } catch (error) {
    console.error('Failed to dispatch desktop notification:', error);
  }
};

Would you like me to open an issue to track adding error handling to the notification hook?

packages/ui-voip/src/v2/MockedMediaCallProvider.tsx (1)

10-18: Props typing and defaults are consistent with MediaCallContext State

The new MockedMediaCallProviderProps type and the destructured defaults (state = 'closed', boolean flags defaulting to false) align with the State union and the existing context usage, while keeping transferredBy optional. This makes the mock safer and clearer to use in stories without changing runtime behavior.

If the mock is meant to stay in sync with the real MediaCallProvider props, consider deriving this type from that one (e.g., via Pick/Partial or a shared base interface) to avoid future drift.

Also applies to: 20-28

📜 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 3336522 and 3cbb421.

📒 Files selected for processing (11)
  • packages/ui-voip/src/v2/MediaCallProvider.tsx (2 hunks)
  • packages/ui-voip/src/v2/MediaCallWidget.stories.tsx (2 hunks)
  • packages/ui-voip/src/v2/MockedMediaCallProvider.tsx (2 hunks)
  • packages/ui-voip/src/v2/components/Widget/Widget.tsx (2 hunks)
  • packages/ui-voip/src/v2/useKeypad.tsx (1 hunks)
  • packages/ui-voip/src/v2/views/IncomingCall.stories.tsx (2 hunks)
  • packages/ui-voip/src/v2/views/IncomingCallTransfer.stories.tsx (2 hunks)
  • packages/ui-voip/src/v2/views/NewCall.stories.tsx (2 hunks)
  • packages/ui-voip/src/v2/views/OngoingCall.stories.tsx (3 hunks)
  • packages/ui-voip/src/v2/views/OutgoingCall.stories.tsx (2 hunks)
  • packages/ui-voip/src/v2/views/OutgoingCallTransfer.stories.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/ui-voip/src/v2/views/OutgoingCallTransfer.stories.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.

Applied to files:

  • packages/ui-voip/src/v2/useKeypad.tsx
🧬 Code graph analysis (3)
packages/ui-voip/src/v2/views/OngoingCall.stories.tsx (1)
packages/ui-voip/src/v2/MediaCallWidget.stories.tsx (1)
  • OngoingCall (86-90)
packages/ui-voip/src/v2/MockedMediaCallProvider.tsx (1)
packages/ui-voip/src/v2/MediaCallContext.ts (1)
  • State (26-26)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
packages/ui-voip/src/v2/useDesktopNotifications.ts (1)
  • useDesktopNotifications (22-75)
⏰ 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: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (11)
packages/ui-voip/src/v2/useKeypad.tsx (1)

2-8: LGTM! Type import modernization.

The change from React.ReactNode to directly imported ReactNode is correct and follows modern React TypeScript conventions. This is functionally equivalent with no runtime or type-safety impact.

packages/ui-voip/src/v2/components/Widget/Widget.tsx (1)

3-3: LGTM: Type import standardization.

The change from React.ReactNode to ReactNode is an idiomatic improvement that reduces reliance on the React namespace.

Also applies to: 31-31

packages/ui-voip/src/v2/MediaCallProvider.tsx (1)

14-14: LGTM: Type improvements and API clarification.

Extracting MediaCallProviderProps as a named type and standardizing on ReactNode improves type clarity and follows React best practices.

Also applies to: 29-31, 33-33

packages/ui-voip/src/v2/views/OutgoingCall.stories.tsx (1)

5-5: LGTM: Consistent provider rename.

The rename from MediaCallProviderMock to MockedMediaCallProvider improves naming clarity and is consistently applied across all story files.

Also applies to: 20-22

packages/ui-voip/src/v2/views/OngoingCall.stories.tsx (1)

5-5: LGTM: Consistent provider rename across all story variants.

All story variants correctly use MockedMediaCallProvider with appropriate props.

Also applies to: 15-17, 28-30, 36-38, 44-46, 52-54, 60-62

packages/ui-voip/src/v2/views/NewCall.stories.tsx (1)

5-5: LGTM: Consistent provider rename.

The story correctly uses MockedMediaCallProvider.

Also applies to: 21-23

packages/ui-voip/src/v2/views/IncomingCall.stories.tsx (1)

5-5: LGTM: Consistent provider rename.

The story correctly uses MockedMediaCallProvider.

Also applies to: 21-23

packages/ui-voip/src/v2/views/IncomingCallTransfer.stories.tsx (1)

5-5: LGTM: Consistent provider rename with correct props.

The story correctly uses MockedMediaCallProvider with the transferredBy prop.

Also applies to: 22-24

packages/ui-voip/src/v2/MediaCallWidget.stories.tsx (1)

7-7: LGTM: Consistent provider rename with dynamic props.

The story correctly uses MockedMediaCallProvider with spread props from story args, enabling flexible story configuration.

Also applies to: 29-31

packages/ui-voip/src/v2/MockedMediaCallProvider.tsx (2)

2-2: Importing ReactNode for typed children looks correct

ReactNode is correctly imported and used to type children, matching common React patterns; no issues here.


152-152: Verification confirms successful migration

The search found no remaining references to MediaCallProviderMock in the codebase, confirming all imports and usages have been properly updated to the new MockedMediaCallProvider name. The rename is complete and consistent throughout the project.

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.99%. Comparing base (4ecebe8) to head (8170d16).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37492      +/-   ##
===========================================
+ Coverage    68.97%   68.99%   +0.01%     
===========================================
  Files         3359     3359              
  Lines       114224   114213      -11     
  Branches     20535    20535              
===========================================
+ Hits         78788    78801      +13     
+ Misses       33345    33324      -21     
+ Partials      2091     2088       -3     
Flag Coverage Δ
e2e 57.41% <ø> (-0.03%) ⬇️
e2e-api 43.84% <ø> (+1.00%) ⬆️

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.

@tassoevan tassoevan added the stat: QA assured Means it has been tested and approved by a company insider label Nov 19, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 19, 2025
@kodiakhq kodiakhq bot merged commit 04f2685 into develop Nov 19, 2025
48 checks passed
@kodiakhq kodiakhq bot deleted the feat/voiceNotification branch November 19, 2025 16:29
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