fix: camera remains active after closing video recording modal#38491
fix: camera remains active after closing video recording modal#38491abhinavkrin wants to merge 4 commits intodevelopfrom
Conversation
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 05f756f The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
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 |
WalkthroughAdds a changeset, unit tests, and modifies VideoRecorder to use a sessionId guard and a stopStreamTracks utility so pending getUserMedia results are stopped if stop() is called before getUserMedia resolves. Also updates Jest to discover client-side spec files. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VideoRecorder
participant BrowserAPI as "navigator.mediaDevices"
User->>VideoRecorder: start()
Note over VideoRecorder: capture currentSessionId = 1
VideoRecorder->>BrowserAPI: getUserMedia()
User->>VideoRecorder: stop()
Note over VideoRecorder: increment sessionId = 2\nstop active stream if any
BrowserAPI-->>VideoRecorder: getUserMedia resolves (stream)
Note over VideoRecorder: captured 1 != sessionId 2\ncall stopStreamTracks(stream)
Note over BrowserAPI: Media tracks stopped (camera released)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.ts`:
- Line 127: The code currently references the video pause method without
invoking it (this.videoel.pause), so update the call in videoRecorder.ts to
invoke the method (this.videoel.pause()); also guard the call by checking the
element and the method exist (e.g., if (this.videoel && typeof
this.videoel.pause === 'function') this.videoel.pause()) to avoid runtime errors
when videoel is null/undefined.
🧹 Nitpick comments (1)
apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.spec.ts (1)
52-54: Consider resetting VideoRecorder state between tests.Since
VideoRecorderis a singleton and tests directly mutate internal state (e.g., lines 167-169), there's potential for state leakage between tests. Consider adding a reset mechanism or callingVideoRecorder.stop()inafterEachto ensure clean state.♻️ Suggested improvement
afterEach(() => { + // Reset VideoRecorder state to ensure test isolation + VideoRecorder.stop(); jest.clearAllMocks(); });
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38491 +/- ##
=========================================
Coverage 70.44% 70.44%
=========================================
Files 3162 3161 -1
Lines 110340 110740 +400
Branches 19841 19911 +70
=========================================
+ Hits 77726 78008 +282
- Misses 30581 30705 +124
+ Partials 2033 2027 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
abhinavkrin
deleted the
fix/camera-remains-active-after-closing-video-modal
branch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.ts`:
- Around line 20-22: Remove inline implementation comments that describe how the
code works; specifically delete the implementation comments around the
session-guard logic for the private field sessionId and the other noted inline
comments (near the block containing sessionId, the block at lines referenced as
36-42, and the block at 117-118). Keep the code and any necessary JSDoc/type
comments that describe public intent or API, but remove explanatory
implementation comments that repeat what the code already expresses (e.g.,
comments immediately above or beside the sessionId field and the internal guard
logic in start/stop or initialization functions).
📦 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/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 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/05 09:59 (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.00]
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.00]
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.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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.00]
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.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.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.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 05 Feb 2026 09:59:56 GMT |
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('Race condition fix', () => { |
There was a problem hiding this comment.
Although this is a test suite aimed at the race condition fix, describing it like that seems very specific imo. I recommend changing to something along the lines of start and stop edge case handling or Asynchronous start and stop handling, etc.
Something that describes what we are testing instead of why the tests were added.
| const streamPromise = new Promise<MediaStream>((resolve) => { | ||
| setTimeout(() => resolve(mockStream), 100); | ||
| }); |
There was a problem hiding this comment.
You could reduce the use of setTimeout in this file by using the util createDeferredPromise. In this this instance the implementation could change to something like:
const streamDeferred = createDeferredPromise<MediaStream>();
getUserMediaMock.mockReturnValue(streamDeferred.promise);
// [...]
streamDeferred.resolve(mockStream);
// [...]| VideoRecorder.stop(); | ||
|
|
||
| await streamPromise; | ||
| await new Promise((resolve) => setTimeout(resolve, 150)); |
There was a problem hiding this comment.
Could we take advantage of jest fake timers and jest.runAllTicks to eliminate some of these arbitrary timeouts?
| const stream1 = { | ||
| getVideoTracks: jest.fn(() => [{ stop: jest.fn() } as unknown as MediaStreamTrack]), | ||
| getAudioTracks: jest.fn(() => [{ stop: jest.fn() } as unknown as MediaStreamTrack]), | ||
| } as unknown as MediaStream; | ||
|
|
||
| const stream2 = { | ||
| getVideoTracks: jest.fn(() => [mockVideoTrack]), | ||
| getAudioTracks: jest.fn(() => [mockAudioTrack]), | ||
| } as unknown as MediaStream; |
There was a problem hiding this comment.
Nitpick: You could create a small utility function to facilitate creating these mock streams. Would improve code readability as well to some extent,
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
| stop: jest.fn(), | ||
| } as unknown as MediaStreamTrack; | ||
|
|
||
| mockStream = { |
There was a problem hiding this comment.
mockStream = createMockStream(...)
| await Promise.resolve(); | ||
| jest.runAllTimers(); | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
await Promise.resolve() is doing all the heavy lifting here, rendering the fake timers unnecessary. This applies to all other tests in this file.
| await Promise.resolve(); | |
| jest.runAllTimers(); | |
| await Promise.resolve(); | |
| await jest.runAllTimersAsync(); |
| getAudioTracks: jest.fn(() => [mockAudioTrack]), | ||
| } as unknown as MediaStream; | ||
|
|
||
| mockVideoElement = document.createElement('video'); |
There was a problem hiding this comment.
jsdom doesn't seem to support any loading or playback media operations. When running these tests it ends up generating a lot of console.error's which makes it difficult to read the logs. Could you mock the necessary functions as well?
3 participants
Proposed changes (including videos or screenshots)
When users close the video recording modal quickly before the camera has fully initialized, the camera light remains on until the page is refreshed. This occurs due to a race condition where stop() is called before the asynchronous getUserMedia() callback completes, causing the stream to initialize after cleanup has been attempted
Why Session ID Mechanism is Needed
Without a way to invalidate pending async callbacks, the original code had no mechanism to prevent stale getUserMedia success handlers from executing after stop() was called. Even though stop() cleared the stream reference, the pending callback would still execute and reinitialize the camera, leaving it active indefinitely.
Screen.Recording.2026-02-04.at.6.51.09.PM.mov
Issue(s)
Steps to test or reproduce
Further comments
CORE-1301
Summary by CodeRabbit