refactor: Media Call client data structure and rendering#38778
Merged
dionisio-bot[bot] merged 36 commits intodevelopfrom Mar 2, 2026
Merged
refactor: Media Call client data structure and rendering#38778dionisio-bot[bot] merged 36 commits intodevelopfrom
dionisio-bot[bot] merged 36 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughSplits the legacy MediaCallContext into instance-level (MediaCallInstanceContext) and view-level (MediaCallViewContext) contexts; introduces providers, new hooks (peek/state, external controls, media session/controls, autocomplete), utilities, tests, and updates consumers and exports to the new context surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InstanceProvider as MediaCallInstanceProvider
participant SignalEmitter as signalEmitter
participant ViewProvider as MediaCallViewProvider
participant Widget as MediaCallWidget
Client->>InstanceProvider: mounts -> creates instance, signalEmitter, audioElement
Note right of InstanceProvider: exposes MediaCallInstanceContext
Client->>ViewProvider: mounts -> subscribes to instance via peek hooks
Client->>Widget: UI action (click call button)
Widget->>SignalEmitter: emit toggleWidget({ peerInfo })
SignalEmitter->>ViewProvider: delivers toggleWidget signal
ViewProvider->>InstanceProvider: calls instance.startCall / controls via instance API
ViewProvider->>Widget: updates sessionState -> re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📦 Docker Image Size Report📈 Changes
📊 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/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 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/18 23:15", "02/19 21:31", "02/19 22:23 (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, 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, 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, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 19 Feb 2026 22:23:51 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38778 +/- ##
===========================================
+ Coverage 70.72% 70.88% +0.16%
===========================================
Files 3195 3208 +13
Lines 113108 113426 +318
Branches 20479 20499 +20
===========================================
+ Hits 79991 80405 +414
+ Misses 31067 30974 -93
+ Partials 2050 2047 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…aSessionControls`
…`MediaCallInstanceContext`
gabriellsh
force-pushed
the
refactor/mediaCallClient
branch
from
08451d4 to
4521add
Compare
gabriellsh
force-pushed
the
refactor/mediaCallClient
branch
2 times, most recently
from
fd6eb8b to
2e47def
Compare
…w(Context | Provder)`
…ediaSession(PeerInfo | State)`
…InfoFromInstanceContact`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx (1)
107-117:⚠️ Potential issue | 🟠 MajorStrengthen cancellation assertion to catch exported error contract regressions.
The test assertion at line 107 only validates that an instance of
PermissionRequestCancelledCallRejectedErroris thrown, but won't catch mismatches in the error'snameandmessageproperties. The implementation currently setsthis.name = 'PermissionRequestDeniedError'(line 59 of useDevicePermissionPrompt.tsx), which differs from the class name and should be validated in the test.Update the assertion to verify both properties:
Proposed assertion fix
- const rejectionExpectation = expect(rejectPromise).rejects.toThrow(PermissionRequestCancelledCallRejectedError); + const rejectionExpectation = expect(rejectPromise).rejects.toMatchObject({ + name: 'PermissionRequestDeniedError', + message: 'Permission request modal closed', + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx` around lines 107 - 117, The test currently only checks that rejectPromise rejects with a PermissionRequestCancelledCallRejectedError class, but doesn't verify the exported error contract (name/message) and will miss regressions in useDevicePermissionPrompt.tsx where the error's this.name is set incorrectly; update the test around rejectPromise (and the rejectionExpectation) to assert the rejected error object includes the correct name and message properties (e.g., assert error.name equals 'PermissionRequestCancelledCallRejectedError' and error.message matches the expected cancellation text) so the test fails if useDevicePermissionPrompt.tsx sets the wrong name or message for PermissionRequestCancelledCallRejectedError.apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx (1)
121-127:⚠️ Potential issue | 🟡 MinorMisleading test description.
The test description says "if state is not closed, new, or unlicensed" but the actual implementation in
useUserMediaCallAction.ts(line 33) checksstate !== 'available'. The description references old state names that no longer exist in the newusePeekMediaSessionStatereturn values ('unavailable' | 'available' | 'ongoing' | 'ringing' | 'calling').📝 Suggested fix
- it('should be disabled if state is not closed, new, or unlicensed', () => { + it('should be disabled if state is not available', () => { usePeekMediaSessionStateMock.mockReturnValueOnce('calling');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx` around lines 121 - 127, The test description is outdated: change the spec title in useUserMediaCallAction.spec.tsx to reflect the current state check in useUserMediaCallAction (which checks state !== 'available') — e.g. rename the it(...) to "should be disabled if state is not available" or "should be disabled for unavailable/ongoing/ringing/calling states"; keep the mocked return (usePeekMediaSessionStateMock.mockReturnValueOnce('calling')) and assertions as-is so the test matches the new return values ('unavailable' | 'available' | 'ongoing' | 'ringing' | 'calling').
🧹 Nitpick comments (16)
packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.spec.tsx (1)
9-12: Unnecessaryasynckeyword.The
renderfunction is synchronous and noawaitis used in this test. Removingasyncwould be slightly cleaner, though this is a minor nit.♻️ Suggested change
-test.each(testCases)(`renders %s without crashing`, async (_storyname, Story) => { +test.each(testCases)(`renders %s without crashing`, (_storyname, Story) => { const view = render(<Story />); expect(view.baseElement).toMatchSnapshot(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.spec.tsx` around lines 9 - 12, Remove the unnecessary async from the test callback in the test case that uses test.each(testCases)(`renders %s without crashing`, async (_storyname, Story) => { ... }); specifically change the callback passed to test.each (the function that calls render(<Story />) and asserts baseElement snapshot) to a synchronous function (drop the async keyword) since render is synchronous and no await is used.packages/ui-voip/src/components/Keypad/Keypad.stories.tsx (1)
14-14: Avoid theas anytype cast.The cast bypasses type safety. If
playToneexpects a specific DTMF key type, consider narrowing the type explicitly or adjusting the Keypad'sonKeyPresscallback type to match.♻️ Suggested improvement
- return <Keypad onKeyPress={(key) => playTone(key as any)} />; + return <Keypad onKeyPress={(key) => playTone(key as DTMFTone)} />;Where
DTMFToneis the expected type from the tone player. If the types genuinely don't align, this story's skip tag makes sense, but the cast should still be to the specific expected type rather thanany.As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/components/Keypad/Keypad.stories.tsx` at line 14, The story is using an unsafe cast in the Keypad story—replace the `as any` by aligning the types: update the Keypad's onKeyPress callback signature or explicitly cast to the expected DTMF type (e.g., DTMFTone) so the call to playTone(key) is type-safe; locate the Keypad component prop `onKeyPress` and the `playTone` function in this file and change the handler to either (key: DTMFTone) => playTone(key) or adjust KeypadProps.onKeyPress to emit the DTMFTone type so no `as any` is necessary.packages/ui-voip/src/components/Keypad/Keypad.spec.tsx (1)
7-9: Consider adding explicit type annotations for better type safety.The
testCasesarray and the mapped Story component lack explicit typing, which could cause issues if the stories structure changes.♻️ Suggested improvement
+import type { StoryFn } from '@storybook/react'; + -const testCases = Object.values(composeStories(stories)) +const testCases: [string, StoryFn][] = Object.values(composeStories(stories)) .filter((Story) => !Story.tags?.includes('skip')) .map((Story) => [Story.storyName || 'Story', Story]);As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/components/Keypad/Keypad.spec.tsx` around lines 7 - 9, The testCases declaration lacks explicit TypeScript types; change it to annotate the composed stories and the mapped tuple so the array is strongly typed — e.g., type the result of composeStories(stories) (or cast it to Record<string, ComponentStory<any>>/ReturnStories type), annotate the mapped parameter (rename Story parameter to story: ComponentStory or a proper StoryType) and the final testCases as Array<[string, ComponentStory]> (or the precise tuple type used in tests) so references like testCases and Story.storyName are type-checked; update the const testCases = ... .filter(...).map(...) expression to include those explicit types.packages/ui-voip/src/components/Widget/Widget.spec.tsx (2)
7-7: Preserve tuple typing intestCasesfor stronger test callback types.Current inference can become too loose. Using a const tuple keeps
%sandStorypositions strongly typed.♻️ Proposed change
-const testCases = Object.values(composeStories(stories)).map((Story) => [Story.storyName || 'Story', Story]); +const testCases = Object.values(composeStories(stories)).map((Story) => [Story.storyName ?? 'Story', Story] as const);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/components/Widget/Widget.spec.tsx` at line 7, The testCases array loses tuple typing; update the mapping so each entry is a const tuple to preserve strong types for the "%s" and Story parameters — e.g. when building testCases from composeStories(stories) map each Story to [Story.storyName || 'Story', Story] as const (or explicitly type testCases as ReadonlyArray<readonly [string, typeof Story]>), ensuring testCases remains an array of readonly 2-tuples rather than a loose any[].
9-12: Remove unusedasyncin snapshot test.No
awaitis used in this test body, soasyncis unnecessary noise.♻️ Proposed change
-test.each(testCases)(`renders %s without crashing`, async (_storyname, Story) => { +test.each(testCases)(`renders %s without crashing`, (_storyname, Story) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/components/Widget/Widget.spec.tsx` around lines 9 - 12, The test registered with test.each over testCases uses an unnecessary async on the test callback because no await is used; remove the async keyword from the test callback signature (the anonymous function that receives _storyname and Story) so the test becomes a plain synchronous function, leaving the render(<Story />) and expect(view.baseElement).toMatchSnapshot() unchanged.packages/ui-voip/src/components/ToggleButton.spec.tsx (1)
7-7: Use export-name fallback to keep test/snapshot names deterministic.Line 7 falls back to a generic
'Story', which can make test output and snapshots ambiguous when metadata is missing. Use the composed export key as fallback instead.Proposed change
-const testCases = Object.values(composeStories(stories)).map((Story) => [Story.storyName || 'Story', Story]); +const testCases = Object.entries(composeStories(stories)).map(([exportName, Story]) => [ + Story.storyName || exportName, + Story, +] as const);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/components/ToggleButton.spec.tsx` at line 7, The test snapshot names use Story.storyName || 'Story', which can produce ambiguous names; change the mapping to use the composed export key as a fallback by iterating Object.entries(composeStories(stories)) and use the exported key when storyName is missing (i.e., map ([key, Story]) => [Story.storyName || key, Story]) so testCases remain deterministic; update the testCases declaration that currently references composeStories(stories) and Story.storyName accordingly.packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx (1)
73-73: Wrap modal dismissal assertions inwaitFor()to handle async DOM updates.After
userEvent.click()at lines 73, 153, and 189, the negative assertions lack explicit synchronization. Wrap them inwaitFor()to ensure the modal has completed its removal from the DOM before asserting. Line 117 already has synchronization viaawait rejectionExpectation.Proposed fix
- expect(cancel).not.toBeInTheDocument(); + await waitFor(() => { + expect(cancel).not.toBeInTheDocument(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx` at line 73, The test contains negative DOM assertions that run immediately after userEvent.click() (e.g., the expect(cancel).not.toBeInTheDocument() assertion) and can race with async removal of the modal; wrap those assertions in waitFor() so they wait for the modal to be removed (for example, replace immediate negative expects after the userEvent.click() calls with await waitFor(() => expect(...).not.toBeInTheDocument())); apply the same change for the assertions following the clicks at the other spots referenced (the other userEvent.click() sites in this spec) — note that line 117 already uses await rejectionExpectation as a model for synchronization.packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.tsx (1)
43-44: Remove inline implementation comment in JSX.Keep the cell, but drop the explanatory comment to match repo style.
Proposed cleanup
- {/* This cell has to be `td` because of a11y guidelines (Table header cells cannot be empty) */} <GenericTableCell key='menu' width={44} aria-label={t('Options')} is='td' />As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.tsx` around lines 43 - 44, Remove the inline JSX comment above the GenericTableCell in MediaCallHistoryTable; keep the GenericTableCell element (<GenericTableCell key='menu' width={44} aria-label={t('Options')} is='td' />) exactly as-is but delete the comment node {/* This cell has to be `td` because of a11y guidelines (Table header cells cannot be empty) */} so the implementation no longer contains an explanatory inline comment.packages/ui-voip/src/providers/useGetAutocompleteOptions.ts (2)
25-33: Consider simplifying the conditions construction.The current approach works but the inline boolean short-circuits can be confusing. A more explicit approach improves readability.
Alternative approach
- const conditions = - peerExtension || forceSIPRouting - ? { - $and: [ - forceSIPRouting && { freeSwitchExtension: { $exists: true } }, - peerExtension && { freeSwitchExtension: { $ne: peerExtension } }, - ].filter(Boolean), - } - : undefined; + const conditionFilters = [ + ...(forceSIPRouting ? [{ freeSwitchExtension: { $exists: true } }] : []), + ...(peerExtension ? [{ freeSwitchExtension: { $ne: peerExtension } }] : []), + ]; + + const conditions = conditionFilters.length > 0 ? { $and: conditionFilters } : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/useGetAutocompleteOptions.ts` around lines 25 - 33, The construction of the conditions object in useGetAutocompleteOptions.ts is hard to read due to inline boolean short-circuiting; update the code that builds the conditions variable (the block referencing peerExtension, forceSIPRouting and freeSwitchExtension) to build an explicit array of clauses (e.g., start with an empty clauses array, push the { freeSwitchExtension: { $exists: true } } clause when forceSIPRouting is true and push the { freeSwitchExtension: { $ne: peerExtension } } clause when peerExtension is set), then set conditions to { $and: clauses } only if clauses.length > 0, otherwise undefined—this removes the need for .filter(Boolean) and improves readability while preserving the same logic.
40-54: Redundant fallback|| []after.map().The
|| []on line 53 is unnecessary sinceitems.map()always returns an array (empty ifitemsis empty).Suggested fix
return ( - items.map((user) => { + items.map((user) => { const label = user.name || user.username; // TODO: This endpoint does not provide the extension number, which is necessary to show in the UI. const identifier = user.username !== label ? user.username : undefined; return { value: user._id, label, identifier, status: user.status, avatarUrl: getAvatarPath({ username: user.username, etag: user.avatarETag }), }; - }) || [] + }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/useGetAutocompleteOptions.ts` around lines 40 - 54, Remove the redundant "|| []" fallback after items.map() inside useGetAutocompleteOptions so the function simply returns the result of items.map(...); locate the return that constructs each option (uses items.map((user) => { const label = user.name || user.username; const identifier = ...; return { value: user._id, label, identifier, status: user.status, avatarUrl: getAvatarPath(...) }; })) and delete the trailing "|| []" so the mapped array is returned directly.packages/ui-voip/src/providers/MockedMediaCallProvider.tsx (3)
131-142: Type assertion onsessionStatemay mask type mismatches.Using
as SessionStatebypasses TypeScript's structural type checking. If theSessionStatetype changes in the future, this mock might silently become invalid.Consider defining the mock to satisfy the type directly, or using
satisfiesto get type checking while preserving the literal type:- } as SessionState; + } satisfies SessionState;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/MockedMediaCallProvider.tsx` around lines 131 - 142, The mock `sessionState` is being force-cast with `as SessionState`, which can hide mismatches; update the mock to satisfy the `SessionState` type instead (e.g., remove the `as SessionState` cast and either declare `const sessionState: SessionState = { ... }` or use the `satisfies SessionState` operator) so TypeScript will validate fields like `state`/`widgetState`, `connectionState`, `peerInfo`, `transferredBy`, `muted`/`mutedState`, `held`/`heldState`, `remoteMuted`, `remoteHeld`, and `callId` against the real `SessionState` shape; adjust any literal types (e.g., `'CONNECTED' as const`) to match the type definition if needed.
159-177: Mockinstance.onshould return a cleanup function for proper listener cleanup.The
onmethod returningundefineddoesn't match the typical event emitter pattern whereonreturns an unsubscribe function. If any consumer relies on the return value to clean up listeners, this could cause subtle issues in tests.Suggested improvement
const instanceContextValue = { instance: { getMainCall: () => ({ contact: { type: 'user', id: '1234567890', }, role: 'caller', reject: () => undefined, hangup: () => undefined, }), - on: () => undefined, + on: () => () => undefined, } as unknown as MediaSignalingSession,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/MockedMediaCallProvider.tsx` around lines 159 - 177, The mock in MockedMediaCallProvider sets instance.on to a function returning undefined, but consumers expect an unsubscribe/cleanup function; update the instance.on implementation (on the mocked MediaSignalingSession) to return a cleanup function that removes the registered listener or is a no-op function that can be called safely; ensure it accepts the same parameters as the real on (event, handler) and returns a function to unsubscribe so tests and components that call the returned function for cleanup behave correctly.
55-67: Consider removing or silencingconsole.logstatements in mocked provider.The
console.logcalls inonDeviceChange,onForward, andonTonemay clutter test output. Consider using no-op functions or a configurable logging option.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/MockedMediaCallProvider.tsx` around lines 55 - 67, Remove noisy console.log calls from the mocked provider by replacing them with no-op functions or using a configurable logger: remove console.log from onDeviceChange, onForward and onTone in MockedMediaCallProvider and either make these handlers call an injected/logging prop (e.g., logger?.debug) or simply return early (no-op) so tests aren’t cluttered; ensure that setWidgetState('closed') and clearState() in onForward remain unchanged while moving logging behind a configurable logger option.apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx (1)
107-119: Redundant mock setup.Line 108 sets
usePeekMediaSessionStateMock.mockReturnValueOnce('available'), butbeforeEachon line 31 already sets this as the default. This call is unnecessary.♻️ Suggested fix
it('should call onClick handler correctly', () => { - usePeekMediaSessionStateMock.mockReturnValueOnce('available'); - const { result } = renderHook(() => useUserMediaCallAction(fakeUser, mockRid));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx` around lines 107 - 119, Remove the redundant mock setup inside the test: the call to usePeekMediaSessionStateMock.mockReturnValueOnce('available') in the 'should call onClick handler correctly' test is unnecessary because beforeEach already sets that default; delete that mockReturnValueOnce line and keep the rest of the test that renders useUserMediaCallAction and asserts toggleWidgetMock was called with the expected payload (references: usePeekMediaSessionStateMock, beforeEach, useUserMediaCallAction, toggleWidgetMock, fakeUser, mockRid).packages/ui-voip/src/context/MediaCallInstanceContext.ts (1)
13-20: Consider exportingMediaCallInstanceContextValuetype.The
MediaCallInstanceContextValuetype is defined but not exported. Consumers that need to type-check context values (e.g., for testing or type-safe wrappers) would benefit from having this type exported.♻️ Suggested fix
-type MediaCallInstanceContextValue = { +export type MediaCallInstanceContextValue = { instance: MediaSignalingSession | undefined; signalEmitter: Emitter<Signals>; audioElement: RefObject<HTMLAudioElement> | undefined; openRoomId: string | undefined; setOpenRoomId: (openRoomId: string | undefined) => void; getAutocompleteOptions: (filter: string) => Promise<PeerAutocompleteOptions[]>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/context/MediaCallInstanceContext.ts` around lines 13 - 20, Export the MediaCallInstanceContextValue type so consumers can import and use it for typing; update the declaration of MediaCallInstanceContextValue to be exported (e.g., export type MediaCallInstanceContextValue = { ... }) and ensure any related exports (context/provider hooks that rely on this type) remain compatible with the exported type to avoid breaking changes.packages/ui-voip/src/providers/MediaCallViewProvider.tsx (1)
212-233: Consider memoizingcontextValueto prevent unnecessary re-renders.The
contextValueobject is recreated on every render, which could cause unnecessary re-renders of context consumers even when the actual values haven't changed. Since most callbacks are stable (defined at component level), memoizing would be beneficial.♻️ Suggested refactor
+ const contextValue = useMemo( + () => ({ + sessionState, + onClickDirectMessage, + onMute, + onHold, + onDeviceChange, + onForward, + onTone, + onEndCall, + onCall, + onAccept, + onSelectPeer, + }), + [sessionState, onClickDirectMessage, onMute, onHold, onDeviceChange, onForward, onTone, onEndCall, onCall, onAccept, onSelectPeer], + ); - const contextValue = { - sessionState, - onClickDirectMessage, - onMute, - onHold, - onDeviceChange, - onForward, - onTone, - onEndCall, - onCall, - onAccept, - onSelectPeer, - };Note: This would also require wrapping
onMute,onHold,onDeviceChange,onForward,onTone,onEndCall, andonSelectPeerwithuseCallbackfor the memoization to be effective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx` around lines 212 - 233, The contextValue object is recreated each render causing unnecessary consumer re-renders; wrap the handlers (onMute, onHold, onDeviceChange, onForward, onTone, onEndCall, onSelectPeer, and any other inline callbacks like onClickDirectMessage, onCall, onAccept, onSelectPeer if not already stable) with React.useCallback and then compute contextValue with React.useMemo (referencing sessionState and the memoized callbacks) so MediaCallViewContext.Provider receives a stable object; ensure dependencies arrays only include values that actually change (e.g., sessionState) to avoid stale closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx`:
- Around line 22-33: The current logic in CallHistoryRowExternalUser.tsx sets
disabled = state !== 'available' and always shows tooltip t('Call_in_progress')
which is wrong for 'ringing' and 'calling'; update the button/state mapping so
disabling only occurs for the actual ongoing session (e.g., state === 'ongoing')
or otherwise derive tooltip text from state: keep the 'voiceCall' action and its
onClick (toggleWidget({ number: contact.number })), but change the disabled
condition and set tooltip = state === 'ongoing' ? t('Call_in_progress') : state
=== 'ringing' ? t('Call_ringing') : state === 'calling' ? t('Call_calling') :
undefined (use your project's i18n keys) so the tooltip reflects
ringing/calling/ongoing correctly.
In `@packages/ui-voip/src/context/definitions.d.ts`:
- Line 31: Decide whether the optional field startedAt?: Date | null on the type
definition should exist: search codebase for references to the startedAt
property (uses in components, reducers, serialization or tests) and if it is
unused remove the startedAt declaration entirely; if it is used, keep the field
and remove the inline TODO, replacing it with a short JSDoc like "Timestamp when
the call/session started" to document its purpose. Target the declaration named
startedAt in the type definition in
packages/ui-voip/src/context/definitions.d.ts when making the change.
In `@packages/ui-voip/src/context/usePeekMediaSessionPeerInfo.ts`:
- Around line 7-9: areEqual currently only iterates keys of a, so missing keys
present in b can be ignored and produce asymmetric equality; update areEqual
(used for comparing PeerInfo) to first compare the sets/lengths of keys for a
and b (e.g., const keysA = Object.keys(a), keysB = Object.keys(b); if
(keysA.length !== keysB.length) return false) and then ensure every key in keysA
has a strictly equal value in b (keysA.every(k => a[k as keyof PeerInfo] === b[k
as keyof PeerInfo])); apply the same symmetric-key check to the other identical
comparison occurrence in this file.
In `@packages/ui-voip/src/index.ts`:
- Line 3: The export line incorrectly re-exports the type
PeekMediaSessionStateReturn as a value; change the single export into two
exports: keep the value exports for MediaCallInstanceContext,
useWidgetExternalControls, and usePeekMediaSessionState as before, and add a
type-only export for PeekMediaSessionStateReturn (using `export type {
PeekMediaSessionStateReturn } from './context'`) so the symbol is exported as a
TypeScript type rather than a runtime value.
In `@packages/ui-voip/src/providers/MediaCallProvider.tsx`:
- Around line 10-17: MediaCallViewProvider is being rendered without the
children so consumers cannot access MediaCallViewContext; update
MediaCallProvider to render MediaCallViewProvider as a wrapper that receives and
renders {children} (i.e., place
<MediaCallViewProvider>{children}</MediaCallViewProvider> inside
<MediaCallInstanceProvider>) so children are nested inside MediaCallViewProvider
and can access its context.
In `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx`:
- Around line 153-160: In onDeviceChange, the promise returned by requestDevice
inside the audioinput branch is missing error handling; wrap the async call to
requestDevice (the chain that calls stopTracks(stream) and
setInputMediaDevice(device)) in a try/catch (or add a .catch) so rejections are
handled, and inside the catch log the error (e.g., console.error or the module
logger) and optionally surface a user-facing fallback/cleanup; update the code
paths around requestDevice, stopTracks, and setInputMediaDevice to ensure errors
do not remain unhandled.
In `@packages/ui-voip/src/providers/useMediaSession.ts`:
- Around line 10-22: The reducer is using a singleton defaultSessionInfo object
which freezes startedAt at module-load time and shares the same reference on
resets; replace the shared object with a factory that returns a fresh
SessionState (e.g., makeDefaultSessionInfo or getDefaultSessionState) and use
that factory both for the reducer initial state and in the reducer's reset
branch (references: defaultSessionInfo and the reducer reset case in
useMediaSession), as well as any other reset usages (the other occurrences
noted), so each reset creates a new object with startedAt = new Date() and
avoids reusing the same reference.
- Around line 113-190: The effect currently wires listeners
(instance.on('sessionStateChange', updateSessionState) and
instance.on('hiddenCall', updateSessionState)) but does not invoke
updateSessionState immediately, so the UI can remain stale if instance already
has a main call; modify the useEffect for instance in useMediaSession.ts to call
updateSessionState() right after registering the offCbs (or after the listeners
are added) so the reducer is seeded with the current session state; keep the
existing cleanup that iterates offCbs to remove listeners.
- Line 192: The lint failure is due to the arrow callback for offCbs.forEach
implicitly returning the result of offCb(); update the callback to have an
explicit void return (e.g., convert to a block-bodied callback that calls
offCb() and returns nothing) so the forEach callback returns void and satisfies
the useIterableCallbackReturn rule; locate the offCbs.forEach call in
useMediaSession and change the arrow to a block that calls offCb() without
returning its value.
In `@packages/ui-voip/src/providers/useMediaSessionControls.ts`:
- Around line 37-46: The acceptCall function should remain synchronous and must
not await call.accept() (keep acceptCall as-is); instead ensure the changeDevice
flow awaits the async setDeviceId() call on the MediaSignalingSession (use await
session.setDeviceId(...)) so completion and error propagation are preserved—look
for functions named acceptCall, changeDevice and the
MediaSignalingSession.setDeviceId symbol to apply the fix.
In `@packages/ui-voip/src/providers/useMediaStream.ts`:
- Around line 26-45: The effect only updates remoteStream on non-null emissions
and misses an initial existing stream and never clears when stream becomes null;
change the useEffect in useMediaStream to: (1) on mount call
getRemoteStream(instance) and setRemoteStream with that value (allowing null to
clear state), (2) in the sessionStateChange handler compute const newStream =
getRemoteStream(instance) and call setRemoteStream(old => old === newStream ?
old : newStream) so null clears stale refs, and (3) when clearing (newStream ===
null) also clear the audio element by accessing actualRef.current and setting
srcObject = null; keep returning the instance.on(...) unsubscribe for cleanup.
In `@packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx`:
- Line 40: The tooltip text for disabled voice-call actions is incorrect when
the user's state is 'unavailable'; update the tooltip logic in
CallHistoryActions.tsx so that when action === 'voiceCall' and state ===
'unavailable' the tooltip shows an "unavailable" message (e.g.,
"User_unavailable" or similar) and when state !== 'unavailable' but disabled due
to an active call it shows "Call_in_progress". Locate the disabled calculation
(const disabled = action === 'voiceCall' && state !== 'available') and the
tooltip rendering for the voiceCall branch, and branch the tooltip string based
on state (check state === 'unavailable' vs other disabled reasons) rather than
always returning "Call_in_progress".
---
Outside diff comments:
In
`@apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx`:
- Around line 121-127: The test description is outdated: change the spec title
in useUserMediaCallAction.spec.tsx to reflect the current state check in
useUserMediaCallAction (which checks state !== 'available') — e.g. rename the
it(...) to "should be disabled if state is not available" or "should be disabled
for unavailable/ongoing/ringing/calling states"; keep the mocked return
(usePeekMediaSessionStateMock.mockReturnValueOnce('calling')) and assertions
as-is so the test matches the new return values ('unavailable' | 'available' |
'ongoing' | 'ringing' | 'calling').
In `@packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx`:
- Around line 107-117: The test currently only checks that rejectPromise rejects
with a PermissionRequestCancelledCallRejectedError class, but doesn't verify the
exported error contract (name/message) and will miss regressions in
useDevicePermissionPrompt.tsx where the error's this.name is set incorrectly;
update the test around rejectPromise (and the rejectionExpectation) to assert
the rejected error object includes the correct name and message properties
(e.g., assert error.name equals 'PermissionRequestCancelledCallRejectedError'
and error.message matches the expected cancellation text) so the test fails if
useDevicePermissionPrompt.tsx sets the wrong name or message for
PermissionRequestCancelledCallRejectedError.
---
Nitpick comments:
In
`@apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsx`:
- Around line 107-119: Remove the redundant mock setup inside the test: the call
to usePeekMediaSessionStateMock.mockReturnValueOnce('available') in the 'should
call onClick handler correctly' test is unnecessary because beforeEach already
sets that default; delete that mockReturnValueOnce line and keep the rest of the
test that renders useUserMediaCallAction and asserts toggleWidgetMock was called
with the expected payload (references: usePeekMediaSessionStateMock, beforeEach,
useUserMediaCallAction, toggleWidgetMock, fakeUser, mockRid).
In `@packages/ui-voip/src/components/Keypad/Keypad.spec.tsx`:
- Around line 7-9: The testCases declaration lacks explicit TypeScript types;
change it to annotate the composed stories and the mapped tuple so the array is
strongly typed — e.g., type the result of composeStories(stories) (or cast it to
Record<string, ComponentStory<any>>/ReturnStories type), annotate the mapped
parameter (rename Story parameter to story: ComponentStory or a proper
StoryType) and the final testCases as Array<[string, ComponentStory]> (or the
precise tuple type used in tests) so references like testCases and
Story.storyName are type-checked; update the const testCases = ...
.filter(...).map(...) expression to include those explicit types.
In `@packages/ui-voip/src/components/Keypad/Keypad.stories.tsx`:
- Line 14: The story is using an unsafe cast in the Keypad story—replace the `as
any` by aligning the types: update the Keypad's onKeyPress callback signature or
explicitly cast to the expected DTMF type (e.g., DTMFTone) so the call to
playTone(key) is type-safe; locate the Keypad component prop `onKeyPress` and
the `playTone` function in this file and change the handler to either (key:
DTMFTone) => playTone(key) or adjust KeypadProps.onKeyPress to emit the DTMFTone
type so no `as any` is necessary.
In `@packages/ui-voip/src/components/ToggleButton.spec.tsx`:
- Line 7: The test snapshot names use Story.storyName || 'Story', which can
produce ambiguous names; change the mapping to use the composed export key as a
fallback by iterating Object.entries(composeStories(stories)) and use the
exported key when storyName is missing (i.e., map ([key, Story]) =>
[Story.storyName || key, Story]) so testCases remain deterministic; update the
testCases declaration that currently references composeStories(stories) and
Story.storyName accordingly.
In `@packages/ui-voip/src/components/Widget/Widget.spec.tsx`:
- Line 7: The testCases array loses tuple typing; update the mapping so each
entry is a const tuple to preserve strong types for the "%s" and Story
parameters — e.g. when building testCases from composeStories(stories) map each
Story to [Story.storyName || 'Story', Story] as const (or explicitly type
testCases as ReadonlyArray<readonly [string, typeof Story]>), ensuring testCases
remains an array of readonly 2-tuples rather than a loose any[].
- Around line 9-12: The test registered with test.each over testCases uses an
unnecessary async on the test callback because no await is used; remove the
async keyword from the test callback signature (the anonymous function that
receives _storyname and Story) so the test becomes a plain synchronous function,
leaving the render(<Story />) and expect(view.baseElement).toMatchSnapshot()
unchanged.
In `@packages/ui-voip/src/context/MediaCallInstanceContext.ts`:
- Around line 13-20: Export the MediaCallInstanceContextValue type so consumers
can import and use it for typing; update the declaration of
MediaCallInstanceContextValue to be exported (e.g., export type
MediaCallInstanceContextValue = { ... }) and ensure any related exports
(context/provider hooks that rely on this type) remain compatible with the
exported type to avoid breaking changes.
In `@packages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsx`:
- Line 73: The test contains negative DOM assertions that run immediately after
userEvent.click() (e.g., the expect(cancel).not.toBeInTheDocument() assertion)
and can race with async removal of the modal; wrap those assertions in waitFor()
so they wait for the modal to be removed (for example, replace immediate
negative expects after the userEvent.click() calls with await waitFor(() =>
expect(...).not.toBeInTheDocument())); apply the same change for the assertions
following the clicks at the other spots referenced (the other userEvent.click()
sites in this spec) — note that line 117 already uses await rejectionExpectation
as a model for synchronization.
In `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx`:
- Around line 212-233: The contextValue object is recreated each render causing
unnecessary consumer re-renders; wrap the handlers (onMute, onHold,
onDeviceChange, onForward, onTone, onEndCall, onSelectPeer, and any other inline
callbacks like onClickDirectMessage, onCall, onAccept, onSelectPeer if not
already stable) with React.useCallback and then compute contextValue with
React.useMemo (referencing sessionState and the memoized callbacks) so
MediaCallViewContext.Provider receives a stable object; ensure dependencies
arrays only include values that actually change (e.g., sessionState) to avoid
stale closures.
In `@packages/ui-voip/src/providers/MockedMediaCallProvider.tsx`:
- Around line 131-142: The mock `sessionState` is being force-cast with `as
SessionState`, which can hide mismatches; update the mock to satisfy the
`SessionState` type instead (e.g., remove the `as SessionState` cast and either
declare `const sessionState: SessionState = { ... }` or use the `satisfies
SessionState` operator) so TypeScript will validate fields like
`state`/`widgetState`, `connectionState`, `peerInfo`, `transferredBy`,
`muted`/`mutedState`, `held`/`heldState`, `remoteMuted`, `remoteHeld`, and
`callId` against the real `SessionState` shape; adjust any literal types (e.g.,
`'CONNECTED' as const`) to match the type definition if needed.
- Around line 159-177: The mock in MockedMediaCallProvider sets instance.on to a
function returning undefined, but consumers expect an unsubscribe/cleanup
function; update the instance.on implementation (on the mocked
MediaSignalingSession) to return a cleanup function that removes the registered
listener or is a no-op function that can be called safely; ensure it accepts the
same parameters as the real on (event, handler) and returns a function to
unsubscribe so tests and components that call the returned function for cleanup
behave correctly.
- Around line 55-67: Remove noisy console.log calls from the mocked provider by
replacing them with no-op functions or using a configurable logger: remove
console.log from onDeviceChange, onForward and onTone in MockedMediaCallProvider
and either make these handlers call an injected/logging prop (e.g.,
logger?.debug) or simply return early (no-op) so tests aren’t cluttered; ensure
that setWidgetState('closed') and clearState() in onForward remain unchanged
while moving logging behind a configurable logger option.
In `@packages/ui-voip/src/providers/useGetAutocompleteOptions.ts`:
- Around line 25-33: The construction of the conditions object in
useGetAutocompleteOptions.ts is hard to read due to inline boolean
short-circuiting; update the code that builds the conditions variable (the block
referencing peerExtension, forceSIPRouting and freeSwitchExtension) to build an
explicit array of clauses (e.g., start with an empty clauses array, push the {
freeSwitchExtension: { $exists: true } } clause when forceSIPRouting is true and
push the { freeSwitchExtension: { $ne: peerExtension } } clause when
peerExtension is set), then set conditions to { $and: clauses } only if
clauses.length > 0, otherwise undefined—this removes the need for
.filter(Boolean) and improves readability while preserving the same logic.
- Around line 40-54: Remove the redundant "|| []" fallback after items.map()
inside useGetAutocompleteOptions so the function simply returns the result of
items.map(...); locate the return that constructs each option (uses
items.map((user) => { const label = user.name || user.username; const identifier
= ...; return { value: user._id, label, identifier, status: user.status,
avatarUrl: getAvatarPath(...) }; })) and delete the trailing "|| []" so the
mapped array is returned directly.
In
`@packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.spec.tsx`:
- Around line 9-12: Remove the unnecessary async from the test callback in the
test case that uses test.each(testCases)(`renders %s without crashing`, async
(_storyname, Story) => { ... }); specifically change the callback passed to
test.each (the function that calls render(<Story />) and asserts baseElement
snapshot) to a synchronous function (drop the async keyword) since render is
synchronous and no await is used.
In `@packages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.tsx`:
- Around line 43-44: Remove the inline JSX comment above the GenericTableCell in
MediaCallHistoryTable; keep the GenericTableCell element (<GenericTableCell
key='menu' width={44} aria-label={t('Options')} is='td' />) exactly as-is but
delete the comment node {/* This cell has to be `td` because of a11y guidelines
(Table header cells cannot be empty) */} so the implementation no longer
contains an explanatory inline comment.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/ui-voip/src/components/Keypad/__snapshots__/Keypad.spec.tsx.snapis excluded by!**/*.snappackages/ui-voip/src/components/PeerInfo/__snapshots__/PeerInfo.spec.tsx.snapis excluded by!**/*.snappackages/ui-voip/src/components/Widget/__snapshots__/Widget.spec.tsx.snapis excluded by!**/*.snappackages/ui-voip/src/components/__snapshots__/ToggleButton.spec.tsx.snapis excluded by!**/*.snappackages/ui-voip/src/views/MediaCallHistoryTable/__snapshots__/MediaCallHistoryTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (74)
apps/meteor/client/providers/MediaCallProvider.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxapps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tsapps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.spec.tsxapps/meteor/client/views/room/hooks/useUserInfoActions/actions/useUserMediaCallAction.tspackages/ui-voip/src/components/DevicePicker.tsxpackages/ui-voip/src/components/Keypad/Keypad.spec.tsxpackages/ui-voip/src/components/Keypad/Keypad.stories.tsxpackages/ui-voip/src/components/PeerInfo/PeerInfo.spec.tsxpackages/ui-voip/src/components/ToggleButton.spec.tsxpackages/ui-voip/src/components/Widget/Widget.spec.tsxpackages/ui-voip/src/components/Widget/Widget.stories.tsxpackages/ui-voip/src/context/MediaCallContext.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/context/MediaCallProvider.tsxpackages/ui-voip/src/context/MediaCallViewContext.tspackages/ui-voip/src/context/definitions.d.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/context/useMediaSession.tspackages/ui-voip/src/context/usePeekMediaSessionPeerInfo.spec.tsxpackages/ui-voip/src/context/usePeekMediaSessionPeerInfo.tspackages/ui-voip/src/context/usePeekMediaSessionState.spec.tsxpackages/ui-voip/src/context/usePeekMediaSessionState.tspackages/ui-voip/src/context/usePeerAutocomplete.spec.tsxpackages/ui-voip/src/context/usePeerAutocomplete.tspackages/ui-voip/src/context/useWidgetExternalControls.tspackages/ui-voip/src/hooks/useDevicePermissionPrompt.spec.tsxpackages/ui-voip/src/hooks/useDevicePermissionPrompt.tsxpackages/ui-voip/src/hooks/useMediaCallAction.spec.tsxpackages/ui-voip/src/hooks/useMediaCallAction.tspackages/ui-voip/src/hooks/useMediaCallOpenRoomTracker.spec.tsxpackages/ui-voip/src/hooks/useMediaCallOpenRoomTracker.tspackages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/index.tspackages/ui-voip/src/providers/MediaCallInstanceProvider.tsxpackages/ui-voip/src/providers/MediaCallLogger.tspackages/ui-voip/src/providers/MediaCallProvider.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/providers/useCallSounds.tspackages/ui-voip/src/providers/useDesktopNotifications.tspackages/ui-voip/src/providers/useGetAutocompleteOptions.tspackages/ui-voip/src/providers/useMediaSession.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/providers/useWidgetExternalControlSignalListener.tspackages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.spec.tspackages/ui-voip/src/utils/derivePeerInfoFromInstanceContact.tspackages/ui-voip/src/utils/deriveWidgetStateFromCallState.spec.tspackages/ui-voip/src/utils/deriveWidgetStateFromCallState.tspackages/ui-voip/src/utils/instanceControlsGetters.tspackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.stories.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.stories.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsxpackages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.spec.tsxpackages/ui-voip/src/views/MediaCallHistoryTable/MediaCallHistoryTable.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCall.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/NewCall.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/NewCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCall.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCall.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCallTransfer.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCallTransfer.tsx
💤 Files with no reviewable changes (5)
- packages/ui-voip/src/providers/useMediaSessionInstance.ts
- packages/ui-voip/src/hooks/useDevicePermissionPrompt.tsx
- packages/ui-voip/src/context/MediaCallProvider.tsx
- packages/ui-voip/src/context/MediaCallContext.ts
- packages/ui-voip/src/context/useMediaSession.ts
apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx
Show resolved
Hide resolved
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 79 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui-voip/src/providers/MediaCallViewProvider.tsx">
<violation number="1" location="packages/ui-voip/src/providers/MediaCallViewProvider.tsx:154">
P2: Handle `requestDevice` rejections in the audio-input change path to avoid unhandled promise rejections and provide consistent error handling.</violation>
</file>
<file name="packages/ui-voip/src/providers/useMediaSession.ts">
<violation number="1" location="packages/ui-voip/src/providers/useMediaSession.ts:210">
P2: The status update effect won’t run when the peer changes to a different user with the same status string, leaving the new peer’s status unset/stale. Include the peer userId in the dependencies so the effect runs on peer changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
tassoevan
requested changes
Feb 26, 2026
tassoevan
reviewed
Feb 26, 2026
packages/ui-voip/src/views/MediaCallWidget/OutgoingCallTransfer.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui-voip/src/context/MediaCallInstanceContext.ts (1)
13-31: Consider exportingMediaCallInstanceContextValuetype.The type is used as the return type of
useMediaCallInstance, but isn't exported. Consumers may need this type for explicit typing of variables or function parameters.♻️ Suggested change
-type MediaCallInstanceContextValue = { +export type MediaCallInstanceContextValue = { instance: MediaSignalingSession | undefined; signalEmitter: Emitter<Signals>; audioElement: RefObject<HTMLAudioElement> | undefined; openRoomId: string | undefined; setOpenRoomId: (openRoomId: string | undefined) => void; getAutocompleteOptions: (filter: string) => Promise<PeerAutocompleteOptions[]>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/context/MediaCallInstanceContext.ts` around lines 13 - 31, Export the MediaCallInstanceContextValue type so consumers can import it for explicit typing; change the type declaration to an exported type (export type MediaCallInstanceContextValue = { ... }) and keep the existing usages of MediaCallInstanceContext and useMediaCallInstance unchanged so their signatures continue to reference the now-exported MediaCallInstanceContextValue type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui-voip/src/context/MediaCallInstanceContext.ts`:
- Around line 13-31: Export the MediaCallInstanceContextValue type so consumers
can import it for explicit typing; change the type declaration to an exported
type (export type MediaCallInstanceContextValue = { ... }) and keep the existing
usages of MediaCallInstanceContext and useMediaCallInstance unchanged so their
signatures continue to reference the now-exported MediaCallInstanceContextValue
type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/ui-voip/src/components/DevicePicker.tsxpackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/context/MediaCallViewContext.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/context/usePeekMediaSessionPeerInfo.tspackages/ui-voip/src/context/usePeekMediaSessionState.tspackages/ui-voip/src/context/usePeerAutocomplete.tspackages/ui-voip/src/context/useWidgetExternalControls.tspackages/ui-voip/src/hooks/useMediaCallOpenRoomTracker.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/ui-voip/src/providers/useMediaSessionControls.tspackages/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/providers/useWidgetExternalControlSignalListener.tspackages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/NewCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/OutgoingCallTransfer.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/ui-voip/src/views/MediaCallWidget/OutgoingCallTransfer.tsx
- packages/ui-voip/src/context/MediaCallViewContext.ts
- packages/ui-voip/src/components/DevicePicker.tsx
- packages/ui-voip/src/hooks/useMediaCallOpenRoomTracker.ts
- packages/ui-voip/src/providers/useMediaSessionControls.ts
- packages/ui-voip/src/context/usePeekMediaSessionPeerInfo.ts
- packages/ui-voip/src/context/useWidgetExternalControls.ts
- packages/ui-voip/src/views/MediaCallWidget/OutgoingCall.tsx
- packages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsx
- packages/ui-voip/src/context/usePeekMediaSessionState.ts
- packages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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:
packages/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/providers/useWidgetExternalControlSignalListener.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/NewCall.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsxpackages/ui-voip/src/context/usePeerAutocomplete.ts
🧠 Learnings (6)
📚 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: In packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx, when the media session state is 'unavailable', the voiceCall action is not included in the actions object passed to CallHistoryActions, so it won't appear in the menu at all. The action filtering happens upstream before getItems is called, preventing any tooltip confusion for the unavailable state.
Applied to files:
packages/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/context/index.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 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/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/providers/useWidgetExternalControlSignalListener.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/context/usePeerAutocomplete.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/ui-voip/src/providers/useMediaStream.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/providers/useWidgetExternalControlSignalListener.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/context/usePeerAutocomplete.ts
📚 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/MediaCallWidget/IncomingCall.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsxpackages/ui-voip/src/views/MediaCallWidget/NewCall.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsx
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
packages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsxpackages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsxpackages/ui-voip/src/context/usePeerAutocomplete.ts
📚 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:
packages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsx
🧬 Code graph analysis (5)
packages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsx (2)
packages/ui-voip/src/context/index.ts (1)
useMediaCallView(3-3)packages/ui-voip/src/context/MediaCallViewContext.ts (1)
useMediaCallView(48-48)
packages/ui-voip/src/context/MediaCallInstanceContext.ts (5)
packages/ui-voip/src/index.ts (2)
PeerInfo(5-5)MediaCallInstanceContext(3-3)packages/ui-voip/src/context/definitions.d.ts (1)
PeerInfo(18-18)packages/media-signaling/src/lib/Session.ts (1)
MediaSignalingSession(39-597)packages/ui-voip/src/components/PeerAutocomplete.tsx (1)
PeerAutocompleteOptions(8-14)packages/ui-voip/src/context/index.ts (2)
MediaCallInstanceContext(2-2)useMediaCallInstance(2-2)
packages/ui-voip/src/providers/useWidgetExternalControlSignalListener.ts (3)
packages/ui-voip/src/context/MediaCallInstanceContext.ts (2)
Signals(9-11)useMediaCallInstance(31-31)packages/livechat/src/widget.ts (1)
callback(509-511)packages/ui-voip/src/context/index.ts (1)
useMediaCallInstance(2-2)
packages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsx (3)
packages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsx (1)
IncomingCallTransfer(68-73)packages/ui-voip/src/context/index.ts (1)
useMediaCallView(3-3)packages/ui-voip/src/context/MediaCallViewContext.ts (1)
useMediaCallView(48-48)
packages/ui-voip/src/context/usePeerAutocomplete.ts (2)
packages/ui-voip/src/context/MediaCallInstanceContext.ts (1)
useMediaCallInstance(31-31)packages/ui-voip/src/context/index.ts (1)
useMediaCallInstance(2-2)
🔇 Additional comments (15)
packages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.stories.tsx (2)
6-7: LGTM!Import paths correctly updated to reflect the new module organization with hooks moved to the context module and
MockedMediaCallProviderrelocated to the providers directory.
39-54: LGTM!The story component is correctly updated to use the new dual-context pattern:
useMediaCallView()for accessingsessionStateandonCalluseWidgetExternalControls()for widget control viatoggleWidgetThe nested state access through
sessionState.statealigns with the PR's consolidation of session information into a singlesessionStateobject.packages/ui-voip/src/providers/useWidgetExternalControlSignalListener.ts (1)
1-12: LGTM!Clean implementation of a signal listener hook. The effect correctly subscribes to the signal and returns the cleanup function from
signalEmitter.on(). Dependencies are properly tracked.packages/ui-voip/src/views/MediaCallWidget/NewCall.tsx (1)
15-24: LGTM!Clean migration to the new context hooks. The component correctly sources
peerInfofromsessionStateand usestoggleWidgetfrom the external controls hook.packages/ui-voip/src/views/MediaCallWidget/IncomingCallTransfer.tsx (1)
5-11: LGTM!Clean migration to the new
useMediaCallViewhook. The component correctly extractspeerInfoandtransferredByfromsessionState.packages/ui-voip/src/context/usePeerAutocomplete.ts (1)
6-8: LGTM!Import paths correctly updated to the new context module.
getAutocompleteOptionsis now sourced fromuseMediaCallInstance(), aligning with the context split refactor.packages/ui-voip/src/views/MediaCallWidget/IncomingCall.tsx (1)
5-16: LGTM!Clean migration to the new
useMediaCallViewhook. The component correctly extractspeerInfofromsessionStateand preserves the runtime guard.packages/ui-voip/src/providers/useMediaStream.ts (2)
26-50: Initial state doesn't capture pre-existing stream.The
useStateis initialized withnullrather than lazily initializing fromgetRemoteStream(instance). WhilesyncRemoteStream()is called on effect mount, there's a brief window where the state is stale if a stream already exists.
52-77: LGTM!The ref callback correctly assigns
remoteStreamto the audio element'ssrcObjectand handles cleanup on unmount.packages/ui-voip/src/providers/MediaCallViewProvider.tsx (4)
31-60: LGTM!Well-structured provider setup. The hooks are properly composed and the audio input device change effect correctly triggers
controls.changeDevicewhen conditions are met.
83-133: LGTM!The
onCallandonAccepthandlers have proper state guards and error handling. The device permission flow correctly stops tracks after obtaining permission, andonAcceptproperly handles thePermissionRequestCancelledCallRejectedErrorcase.
153-164: Error handling added for device change.The
.catch()handler was added to handlerequestDevicefailures, addressing the previous review concern.
206-214: LGTM!The signal listener is correctly wired with a memoized callback that depends on
toggleWidget.packages/ui-voip/src/context/MediaCallInstanceContext.ts (1)
9-20: LGTM!Clean type definitions for the context value. The
Signalstype properly constrains the signal names and payloads.packages/ui-voip/src/context/index.ts (1)
1-7: LGTM!The barrel export file is well-organized and properly consolidates the new VOIP context architecture. Good use of
export type *for type re-exports andexport type { ... }for the specific type export on line 7, which aids tree-shaking. The export structure cleanly reflects the context split described in the PR objectives.
tassoevan
approved these changes
Mar 2, 2026
gabriellsh
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Member
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
useMediaSessionintouseMediaSessionanduseMediaSessionControlsuseMediaSessionControlshook into it's own file.useMediaSessionreturn fromstatetosessionStatesessionState.MediaCallContextintoMediaCallWidgetContextandMediaCallInstanceContextgetAutocompleteOptionsfromMediaCallProviderto it's own hookuseMediaSessionreducerMediaCallWidget(Context | Provder)toMediaCallView(Context | Provder)Issue(s)
CORE-1901
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
New Features
Chores
Style