Conversation
brycetham
requested review from
SomeBody16,
antsukanova,
bbaldino,
edvujic,
k-wasniowski and
szotrabh
WalkthroughThe changes focus on enhancing media stream creation capabilities in the device management module. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant DeviceManagement
participant Browser
participant CaptureController
User->>DeviceManagement: createDisplayMedia(options)
DeviceManagement->>Browser: getDisplayMedia(constraints)
Browser-->>DeviceManagement: Display Stream
opt Audio Capture
DeviceManagement->>Browser: Capture System Audio
Browser-->>DeviceManagement: System Audio Stream
end
opt Focus Control
DeviceManagement->>CaptureController: setFocusBehavior
end
DeviceManagement-->>User: [DisplayStream, AudioStream]
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| constructor: Constructor<T>, | ||
| videoContentHint?: VideoContentHint | ||
| ): Promise<T> { | ||
| export async function createDisplayMedia< |
There was a problem hiding this comment.
Honestly I'm a little iffy about the name of this API. Nothing about createDisplayMedia tells me it's a more advanced alternative to createDisplayStreamWithAudio but I'm not sure I have any better ideas.
There was a problem hiding this comment.
I like this name for sure. It's not telling you that it's a more advanced alternative, but as an option, if we need to explain this difference to someone, we can just add an API description in README.
src/device/device-management.ts
Outdated
| surfaceSwitching: options.video.surfaceSwitching, | ||
| systemAudio: options.audio?.systemAudio, | ||
| monitorTypeSurfaces: options.video.monitorTypeSurfaces, | ||
| } as any); // eslint-disable-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Most of these additional options are experimental so TypeScript does not have the proper types yet. Casting as any is necessary here to prevent TypeScript from yelling at us.
There was a problem hiding this comment.
@brycetham Could we consider having global.d.ts with the right types api getDisplayMedia for our needs instead of casting to any? I feel for this purpose having own d.ts feels right
There was a problem hiding this comment.
This is a good call. Actually, I noticed that we use our own getDisplayMedia definition from media/index.ts, so I think I can just redefine the parameters there. That way, I don't have to mess with the global scope and it is still well defined somewhere in our repo.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/device/device-management.ts (3)
153-168: Potential overshadowing of'constructor'property.
While using a property namedconstructoris valid, it can overshadow the global property. Consider renaming it to something likestreamConstructorfor clarity.- constructor: Constructor<T>; + streamConstructor: Constructor<T>;
173-182: Avoid usingas anyif possible.
Casting toanycan mask type inconsistencies. A more robust type definition may help catch potential issues at compile-time.
214-214: Consider renaming the'constructor'parameter.
Renaming avoids confusion with the globalconstructorproperty. For instance, use something likestreamConstructorfor clarity.- constructor: Constructor<T>, + streamConstructor: Constructor<T>,🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/device/device-management.spec.ts(8 hunks)src/device/device-management.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/device/device-management.ts
[error] 214-214: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (22)
src/device/device-management.ts (8)
28-34: Interface definition is well-structured.
TheCaptureControllerinterface competently outlines the behavior options. Its typed approach forsetFocusBehavioris clear and coherent.
124-147: Thorough documentation.
This docstring effectively conveys the purpose, parameters, and return value forcreateDisplayMedia, aiding maintainability and discoverability.
149-152: Generics usage is well designed.
The function signature accommodates typed returns for both display and system audio streams without unnecessary duplication.
170-171: Revisit default constraints assignment.
Setting these fields totrueor a boolean when no explicit constraints are provided may lead to unintended defaults. Ensure this aligns with expected behavior.
186-186: Error handling is consistent.
This classification of capture errors aligns well with existing error patterns, maintaining uniformity across the codebase.
189-206: Clean separation of display and audio streams.
Returning[displayStream, null]when audio is unavailable is a straightforward approach. Good job adhering to the fail-safe pattern.
213-216: Backward-compatible helper function.
createDisplayStreamdelegates tocreateDisplayMediawhile preserving the simpler API signature for existing consumers.🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
241-244: Nice reuse ofcreateDisplayMedia.
Pulling common logic into a single function reduces redundancy and potential maintenance overhead.src/device/device-management.spec.ts (14)
1-1: Added import from '../errors'.
No concerns here. This simply references error classes for test usage.
11-11: ImportedCaptureController.
Allows testing the newly introduced capture session manipulation.
14-14: AddedcreateDisplayMediaimport for tests.
Extends the suite to cover the new function under various scenarios.
30-30: MockinggetUserMediafor microphone tests.
Ensures reproducible and isolated test conditions.
79-79: MockinggetUserMediafor camera tests.
Ensures camera-based functionality is tested consistently without requiring actual devices.
126-126: Spy ongetUserMediafor combined streams.
Remains consistent with other test mocks, providing a controlled environment.
172-192: Initial tests forcreateDisplayMedia.
Verifies standard usage, including video-only and video+audio capture. This covers essential use cases thoroughly.
194-214: Ensuring constraints and content hints are honored.
These test cases confirm that custom constraints are passed togetDisplayMediaand content hints are correctly assigned to the resulting stream.
216-297: Robust coverage of advanced capture options.
The suite testspreferCurrentTab,selfBrowserSurface,surfaceSwitching,systemAudio, andCaptureController, providing comprehensive confidence in the new logic.
300-300: Mock setup forcreateDisplayStream.
Demonstrates consistent mocking ofgetDisplayMediajust as in the other display-related tests.
306-306: VerifyinggetDisplayMediainvocation arguments.
Ensures only video is requested with no audio track forcreateDisplayStream.
310-310: Assertion count check.
expect.assertions(1)helps ensure the test is verifying the correct number of expectations.
325-325: MockinggetDisplayMediafor audio-inclusive tests.
Maintains a uniform approach in all display-stream test scenarios.
356-356: Selective mocking ensures audio track presence.
This approach is essential for testing audio track logic distinctly.
src/device/device-management.ts
Outdated
| // So we define the interface ourselves here. | ||
| // See https://developer.mozilla.org/en-US/docs/Web/API/CaptureController. | ||
| export interface CaptureController { | ||
| setFocusBehavior(behavior: 'auto' | 'always' | 'never'): Promise<void>; |
There was a problem hiding this comment.
Question about these behavior states should we keep wording from the spec?
enum CaptureStartFocusBehavior {
"focus-capturing-application",
"focus-captured-surface",
"no-focus-change"
};
There was a problem hiding this comment.
You're right, thanks for catching that. Admittedly, I think I tried using copilot here but didn't verify that the types were correct. Lesson learned 😅
brycetham
dismissed stale reviews from szotrabh, k-wasniowski, and edvujic
via
de695d1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/media/index.ts (1)
9-16: Add JSDoc documentation for the CaptureController interface.The interface is well-defined, but adding JSDoc documentation would improve code maintainability and IDE support.
// CaptureController is experimental so TypeScript doesn't have a type for it yet. // So we define the interface ourselves here. // See https://developer.mozilla.org/en-US/docs/Web/API/CaptureController. +/** + * Experimental interface for controlling capture behavior. + * @see https://developer.mozilla.org/en-US/docs/Web/API/CaptureController + */ export interface CaptureController { + /** + * Sets the focus behavior for the capture session. + * @param behavior - The desired focus behavior. + * @returns A promise that resolves when the behavior is set. + */ setFocusBehavior( behavior: 'focus-capturing-application' | 'focus-captured-surface' | 'no-focus-change' ): Promise<void>; }src/device/device-management.spec.ts (1)
172-300: Consider adding error handling test cases.The test suite is comprehensive for successful scenarios, but could benefit from additional test cases:
- Error handling when getDisplayMedia fails
- Edge cases with invalid option combinations
Example test case:
it('should handle getDisplayMedia failure', async () => { expect.assertions(1); jest.spyOn(media, 'getDisplayMedia').mockRejectedValueOnce(new Error('Permission denied')); await expect(createDisplayMedia({ video: { displayStreamConstructor: LocalDisplayStream } })).rejects.toThrow('Permission denied'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/device/device-management.spec.ts(8 hunks)src/device/device-management.ts(6 hunks)src/media/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/device-management.ts
🔇 Additional comments (4)
src/media/index.ts (2)
31-40: LGTM!The documentation is clear and properly references the experimental options.
41-51: LGTM!The function signature is well-structured with proper typing for all experimental options.
src/device/device-management.spec.ts (2)
1-26: LGTM!The test setup is well-organized with proper imports and mocking configuration.
30-34: LGTM!The mock implementations are well-documented and correctly configured.
Also applies to: 303-303, 328-328
src/device/device-management.spec.ts
Outdated
| jest | ||
| .spyOn(media, 'getUserMedia') | ||
| .mockReturnValue(Promise.resolve(mockStream as unknown as MediaStream)); | ||
| jest.spyOn(media, 'getUserMedia').mockReturnValue(Promise.resolve(mockStream)); |
There was a problem hiding this comment.
https://jestjs.io/docs/mock-function-api#mockfnmockresolvedvaluevalue
mockFn.mockResolvedValue(value)
Shorthand for:
jest.fn().mockImplementation(() => Promise.resolve(value));
brycetham
dismissed stale reviews from SomeBody16, edvujic, and antsukanova
via
a59db52
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/device/device-management.spec.ts (2)
172-300: Comprehensive test coverage for createDisplayMedia API.The test suite thoroughly covers all aspects of the new API:
- Basic video-only and video+audio scenarios
- Constraint handling
- Content hint preservation
- Advanced options (preferCurrentTab, selfBrowserSurface, etc.)
- CaptureController integration
One suggestion to improve test maintainability:
Consider extracting common test setup into a beforeEach block:
describe('createDisplayMedia', () => { jest.spyOn(media, 'getDisplayMedia').mockResolvedValue(mockStream); + + const defaultVideoOptions = { + displayStreamConstructor: LocalDisplayStream, + }; + + const defaultAudioOptions = { + systemAudioStreamConstructor: LocalSystemAudioStream, + };This would reduce repetition in test cases and make them more focused on the specific scenarios being tested.
288-289: Consider adding type assertion alternatives.Instead of using type assertion with
as, consider creating a proper mock object:-const fakeController: CaptureController = {} as CaptureController; +const fakeController: Partial<CaptureController> = { + // Add minimum required properties based on CaptureController interface +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device/device-management.spec.ts(8 hunks)
🔇 Additional comments (4)
src/device/device-management.spec.ts (4)
1-19: LGTM! Clean import organization.The imports are well-organized, with the new
CaptureControllerimport properly placed among other media-related imports.
29-30: Great improvement in mock setup!The change from
mockReturnValue(Promise.resolve(...))tomockResolvedValuemakes the test code more concise and readable.Also applies to: 79-80, 126-127, 173-174, 303-304, 328-329, 357-358
308-309: LGTM! Proper test expectations.The test correctly verifies that
getDisplayMediais called withaudio: falsefor video-only streams.
338-340: LGTM! Proper test expectations.The test correctly verifies that
getDisplayMediais called withaudio: truewhen audio is requested.
# [2.12.0](webex/webrtc-core@v2.11.0...v2.12.0) (2025-02-04) ### Features * add createDisplayMedia API ([webex#88](webex#88)) ([329d98d](webex@329d98d))
7 participants
This PR adds a new
createDisplayMediaAPI that callsgetDisplayMediawith additional options not currently possible with the existingcreateDisplayStreamandcreateDisplayStreamWithAudioAPIs, as part of SPARK-610118. In particular, they add the following options:I've also cleaned up some of the unit tests to remove some logic that I don't believe are necessary.
Summary by CodeRabbit
New Features
CaptureControllerfor advanced media capture session management.Improvements