feat: add screen sharing support to realtime API #72
Conversation
WalkthroughThe pull request introduces screen sharing support to the Changes
Possibly related PRs
Suggested Reviewers
Poem
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 (
|
jackshen310
force-pushed
the
feat/realtime-api
branch
from
324ba06 to
d40b22e
Compare
jackshen310
changed the title
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 92.12% 92.47% +0.35%
==========================================
Files 115 46 -69
Lines 5267 2338 -2929
Branches 1073 426 -647
==========================================
- Hits 4852 2162 -2690
+ Misses 409 175 -234
+ Partials 6 1 -5
|
![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
🔭 Outside diff range comments (1)
packages/realtime-api/src/client.ts (1)
Line range hint
247-271: Await the video device setup for proper error handling.
setVideoInputDeviceis async but not awaited, so any error would be uncaught increateLocalStream. Consider awaiting it to ensure proper error propagation and to confirm the device is ready before continuing.- this.setVideoInputDevice( + await this.setVideoInputDevice( videoConfig?.videoInputDeviceId || devices.videoInputs[0].deviceId, videoConfig?.videoOnDefault, );
🧹 Nitpick comments (7)
packages/realtime-api/src/client.ts (2)
213-246: Consider skipping repeated stop & start calls.
Currently, the code unconditionally callsawait this.changeVideoState(false);before starting a new capture. This is correct but may cause unnecessary flicker if the device doesn’t actually change.await this.changeVideoState(false); + // Optionally, you could check if _streamIndex truly differs + // or if deviceId remains the same before stopping.
306-316: Minor duplication between screen and camera logic.
The code is clear and functional, but you might consider factoring out repeated start/stop operations for a more concise approach.packages/realtime-api/test/client.spec.ts (1)
135-155: Device tests look good but consider coverage for multiple devices.
Currently, the tests handle single audio input and output. Adding more device variants could ensure robust coverage.packages/realtime-api/src/utils.ts (1)
55-68: Adding a screen share device is an effective approach, but consider multiple screens.
You currently label it “Screen Share” with a fixeddeviceId. If multi-monitor screen sharing becomes relevant, you might need a more dynamic approach.packages/realtime-api/test/utils.spec.ts (1)
35-39: Avoid usingdeleteon global properties.
Usingdelete global.navigatorcan degrade performance. Instead, set it toundefinedor reassign to an empty object if you only want to remove references.- delete global.navigator; + // For test cleanup, assign undefined: + global.navigator = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/realtime-api/src/event-handler.ts (1)
84-88: Event name addition is consistent.
VIDEO_INPUT_DEVICE_CHANGEDaligns with existing naming. If you foresee more specialized cases, consider a more generic “VIDEO_DEVICE_UPDATE” to handle various video-related changes.packages/realtime-api/src/index.ts (1)
11-12: Documentation is clear, but consider highlighting default behavior.While these new properties (
videoInputDeviceIdandscreenConfig) are straightforward, clarifying the default selection whenvideoInputDeviceIdis not provided could improve clarity for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json(1 hunks)examples/realtime-console/src/pages/main/header.tsx(6 hunks)packages/realtime-api/README.md(1 hunks)packages/realtime-api/src/client.ts(6 hunks)packages/realtime-api/src/event-handler.ts(1 hunks)packages/realtime-api/src/index.ts(5 hunks)packages/realtime-api/src/utils.ts(3 hunks)packages/realtime-api/test/client.spec.ts(1 hunks)packages/realtime-api/test/index.spec.ts(1 hunks)packages/realtime-api/test/utils.spec.ts(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/realtime-api/test/utils.spec.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (27)
packages/realtime-api/src/client.ts (6)
12-12: No issues with the new import.
22-24: New private fields look fine.
These fields effectively track video configuration, stream index, and room user ID without polluting the public interface.
32-32: OptionalvideoConfigconstructor parameter is acceptable.
Helps keep things flexible if video is not always enabled.
55-55: Assigning_videoConfigis straightforward.
No issues observed.
278-278: Graceful approach to stopping video viachangeVideoState.
This prevents scattering start/stop logic in multiple places.
280-280: Consistent use ofchangeAudioState.
Similar benefit as with video state handling, promoting clarity and reuse.packages/realtime-api/test/client.spec.ts (5)
10-31: Mocking engine methods is well-structured.
The comprehensive mock allows precise verification of calls without relying on external factors.
65-71: Utility mocking covers screen share checks nicely.
Ensures you can test without real device dependencies or screen share support in the environment.
239-255: Thorough tests for setting video input device.
Validates both camera and screen share paths. This ensuressetVideoInputDevicelogic is well-covered.
268-281: Checks for empty devices increateLocalStreamare well tested.
This ensures you properly throw an error rather than proceeding with invalid device states.
323-350: Event handling verifications are comprehensive.
From player events to audio property reports, these tests ensure robust coverage of the event-dispatching logic.packages/realtime-api/src/utils.ts (3)
35-39: NewcheckDevicePermissionfunction is clearly documented.
Provides a streamlined approach to enabling devices.
89-90:isScreenShareDeviceis straightforward and efficient.
No issues found.
92-98:isScreenShareSupportedcheck is well implemented.
Simple presence detection ofgetDisplayMediais sufficient for modern browsers.packages/realtime-api/test/utils.spec.ts (1)
134-145: Screen share support tests are thorough.
You cover both scenarios—presence and absence ofgetDisplayMedia, ensuring correctness across different browsers.packages/realtime-api/test/index.spec.ts (1)
208-226: Thorough test coverage for setVideoInputDevice.This test suite properly verifies that the
setVideoInputDevicemethod calls the internal method and dispatches the appropriate event with the correct payload. No issues found.packages/realtime-api/src/index.ts (4)
1-1: Import is consistent with new screen sharing functionality.The addition of
ScreenConfigfrom@volcengine/rtcaligns nicely with the screen sharing capability. No problems are observed here.
72-81: Well-documented videoConfig field.The JSDoc comments for
videoConfigare well-structured and provide clarity in both English and Chinese. This should significantly reduce confusion for developers onboarding the feature.
132-132: Pass-through configuration looks good.The
videoConfigis properly passed toEngineClient, ensuring that the new video-sharing features are appropriately initialized.
304-307: New setVideoInputDevice implementation is correct.The asynchronous call properly delegates to
_clientand dispatches theVIDEO_INPUT_DEVICE_CHANGEDevent with the correct payload. No issues observed.examples/realtime-console/src/pages/main/header.tsx (6)
18-18: isShowVideo import integration check.Verifying that
isShowVideois utilized consistently ensures that video features are rendered conditionally. Good approach to maintain separation of concerns.
53-55: New state for videoInputDeviceId.The default value
'default'is a sensible fallback, but confirm that'default'actually maps to a valid device or skip logic if invalid.
62-65: Video input device options array introduced.Storing these devices in state is consistent with existing audio device state handling. This approach helps keep device options updated and easily accessible.
Line range hint
104-116: Retrieving video devices alongside audio devices.Using
{ video: true }ensures that both audio and video inputs are fetched. Make sure that handling empty or unsupported device lists is robust (e.g., fallback or user notice).Also applies to: 120-120
286-290: Video input device change handler is straightforward.The handler updates component state and invokes the client’s
setVideoInputDevicemethod. This ensures a consistent user experience when switching devices.
372-382: Conditionally rendering video device selector.Showing the video device dropdown only when
isShowVideoreturns true prevents unnecessary UI clutter. Implementation is good for user-driven toggles.packages/realtime-api/README.md (1)
61-64: Good addition of videoConfig documentation.These lines clearly describe how to enable video and specify the DOM for rendering. This documentation aligns well with the new features.
jackshen310
force-pushed
the
feat/realtime-api
branch
2 times, most recently
from
612508a to
0c7a0f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/realtime-api/src/index.ts (1)
303-307: Convenient method to swap video devices.
ThesetVideoInputDevicemethod is straightforward. Consider adding validation around unsupported devices to avoid potential runtime errors.packages/realtime-api/test/utils.spec.ts (1)
37-37: Consider avoidingdelete.
delete global.navigator;can degrade performance. Assigningglobal.navigator = undefinedis safer and recommended.- delete global.navigator; + global.navigator = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/realtime-api/src/client.ts (1)
306-316: Consider refactoring changeVideoState for better maintainability.The method contains nested if conditions that could be simplified. Consider extracting the stream type specific logic into separate private methods.
-async changeVideoState(isVideoOn: boolean) { - try { - if (isVideoOn) { - if (this._streamIndex === StreamIndex.STREAM_INDEX_MAIN) { - await this.engine.startVideoCapture(); - } else { - await this.engine.startScreenCapture(this._videoConfig?.screenConfig); - } - } else { - if (this._streamIndex === StreamIndex.STREAM_INDEX_MAIN) { - await this.engine.stopVideoCapture(); - } else { - await this.engine.stopScreenCapture(); - } - } +async changeVideoState(isVideoOn: boolean) { + try { + await this._handleVideoStateChange(isVideoOn); + } catch (e) { + this.dispatch(EventNames.ERROR, e); + throw e; + } +} + +private async _handleVideoStateChange(isVideoOn: boolean) { + const isMainStream = this._streamIndex === StreamIndex.STREAM_INDEX_MAIN; + if (isVideoOn) { + await (isMainStream + ? this.engine.startVideoCapture() + : this.engine.startScreenCapture(this._videoConfig?.screenConfig)); + } else { + await (isMainStream + ? this.engine.stopVideoCapture() + : this.engine.stopScreenCapture()); + } }examples/realtime-console/src/pages/main/header.tsx (2)
53-54: Consider using a reducer for device state management.The component manages multiple related device states. Consider using useReducer for better state management and to avoid potential state synchronization issues.
type DeviceState = { videoInputDeviceId: string; videoInputDeviceOptions: Array<{ label: string; value: string }>; // ... other device states }; type DeviceAction = | { type: 'SET_VIDEO_DEVICE'; payload: string } | { type: 'SET_VIDEO_OPTIONS'; payload: Array<{ label: string; value: string }> } // ... other actionsAlso applies to: 62-64
372-382: Enhance accessibility of video device selector.The video device selector should include proper ARIA labels and keyboard navigation support.
{isShowVideo() && ( <span> - <Text style={{ marginRight: 8 }}>Video Device:</Text> + <Text style={{ marginRight: 8 }} id="video-device-label">Video Device:</Text> <Select style={{ width: 200 }} value={videoInputDeviceId} onChange={handleVideoInputDeviceChange} options={videoInputDeviceOptions} + aria-labelledby="video-device-label" + role="combobox" /> </span> )}packages/realtime-api/test/client.spec.ts (1)
297-311: Add test cases for video config validation.The createLocalStream tests should verify that video configuration is properly validated and applied.
it('should validate video config', async () => { const invalidConfig = { renderDom: null, screenConfig: {} }; await expect( client.createLocalStream('test-user', invalidConfig) ).rejects.toThrow(); }); it('should apply default video config', async () => { await client.createLocalStream('test-user'); expect(mockEngine.setLocalVideoPlayer).toHaveBeenCalledWith( expect.any(Number), expect.objectContaining({ renderDom: 'local-player' }) ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-49.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-13-03.json(1 hunks)examples/realtime-console/src/pages/main/header.tsx(6 hunks)packages/realtime-api/README.md(1 hunks)packages/realtime-api/src/client.ts(6 hunks)packages/realtime-api/src/event-handler.ts(1 hunks)packages/realtime-api/src/index.ts(5 hunks)packages/realtime-api/src/utils.ts(3 hunks)packages/realtime-api/test/client.spec.ts(1 hunks)packages/realtime-api/test/index.spec.ts(1 hunks)packages/realtime-api/test/utils.spec.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-13-03.json
🚧 Files skipped from review as they are similar to previous changes (4)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json
- packages/realtime-api/src/event-handler.ts
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json
- packages/realtime-api/test/index.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/realtime-api/test/utils.spec.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (14)
packages/realtime-api/src/index.ts (4)
1-1: Import usage looks good.
No issues noted with importingScreenConfigfrom '@volcengine/rtc'.
11-12: Well-defined optional properties.
The addition ofvideoInputDeviceIdandscreenConfigcleanly extends theVideoConfiginterface. This approach maintains backward compatibility.
72-81: Helpful inline documentation.
These doc comments provide a clear reference for integrators, ensuring they understand the purpose and usage of each property in the newvideoConfig.
132-132: PassingvideoConfigto the EngineClient.
Ensuring thatvideoConfigis passed intoEngineClientis crucial for enabling screen sharing and other video use cases. Good job.common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-49.json (1)
1-11: Changelog entry looks good.
You’re indicating a minor version bump for adding screen sharing support. Keep an eye on potentially sensitive emails in public commits. Otherwise, it looks fine.packages/realtime-api/src/utils.ts (4)
35-39: Aligned approach for device permissions.
Replacing the oldcheckPermissionwithcheckDevicePermissionis clearer and more flexible. Good job.
55-63: Neat approach to treat screen share as a video device.
Nice integration of screen sharing into the device list. Ensure that downstream code handles it gracefully if enumerated but not actually usable (e.g., older browsers that might partially support it).
84-85: Method for clarity.
isScreenShareDeviceis straightforward. This aids in preventing confusion when toggling different video devices.
91-93: Simple check for screen share support.
isScreenShareSupported()is minimal and effective. Good coverage for older browsers.packages/realtime-api/README.md (1)
61-64: Video configuration walkthrough.
DocumentingvideoConfigwith clear defaults significantly improves developer onboarding.packages/realtime-api/test/utils.spec.ts (2)
Line range hint
92-108: Positive test coverage for screen share device enumeration.
The test scenario properly validates the presence of screen share invideoInputs. This is important for verifying the user experience in real-world scenarios.
164-174: Coupled test scenario for enumeration failures.
Good job verifying both modern and legacy enumeration pathways. This thorough approach catches edge-case regressions early.packages/realtime-api/src/client.ts (1)
22-24: LGTM: Well-structured private properties.The new private properties are well-organized and properly scoped for managing video configuration, stream type tracking, and user identification.
packages/realtime-api/test/client.spec.ts (1)
10-31: LGTM: Well-structured mock implementation.The mock engine implementation covers all necessary methods for testing the video functionality.
jackshen310
force-pushed
the
feat/realtime-api
branch
from
0c7a0f0 to
a40de64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
examples/realtime-console/src/pages/main/header.tsx (1)
Line range hint
104-116: Add error handling or fallback logic when no video input devices are detected.
Right now, you assumedevices.videoInputswill always return an array. If no devices are available,videoInputDeviceOptionsbecomes empty, and the UI might confuse end users. Consider providing a fallback UI or error message.Also applies to: 120-120
🧹 Nitpick comments (31)
examples/realtime-console/src/pages/main/header.tsx (3)
53-54: Initialize video input state with an empty string or null for clarity.
Currently, thevideoInputDeviceIddefaults to'default'. If there’s ever a discrepancy between “default” as a label versus the actual default device, it could cause confusion. Consider using a more descriptive initial value (e.g., an empty string ornull) and handle that case accordingly.
62-64: Consider naming consistency for state setters.
You havevideoInputDeviceIdandsetVideoInputDeviceId; in parallel, this is good. However, ensure your naming is consistent across devices (e.g.,audioCapture,audioPlayback) to help maintain clarity.
372-382: Promote a better UX for the video device dropdown.
Consider dynamically disabling or hiding this dropdown if no video devices are found, or providing a helper text ifvideoInputDeviceOptionsis empty. This fosters a more user-friendly experience.packages/realtime-api/src/utils.ts (3)
35-39: Prefer explicit type definitions for clarity.Currently,
checkDevicePermissionimplicitly returns a Promise with the structure determined byVERTC.enableDevices({ audio: true, video: checkVideo }). Consider adding explicit types for the returned object properties for better clarity and maintainability.
55-63: Consider adding a user-facing label for screen share detection.While the code adds a new device object for screen sharing, consider clarifying user-facing labels if localization or consistent naming across different parts of the UI is required. This helps avoid confusion when multiple “unnamed” devices appear.
91-93: Further guard against missing mediaDevices
Ifnavigator.mediaDevicesis not available in some older browsers, the check might throw. Consider wrapping the expression in a conditional or try/catch to gracefully handle the possibility ofnavigator.mediaDevicesbeing undefined.packages/realtime-api/test/utils.spec.ts (6)
10-10: Provide type clarity on mockEngine usage
This line references the mocked engine in a generic manner. It may benefit from typed mocks or dedicated interfaces to ensure code completion and reduce potential runtime errors if the engine’s API changes.
64-64: Test for thrown exceptions
This line verifies error handling. Ensure you also consider different exception types or messages (e.g., permission prompt canceled). Doing so can reveal nuanced edge cases.
72-72: Expand coverage
The current test covers audio permission success; consider a test to confirm that video is handled correctly ifcheckDevicePermission(true)is called.
Line range hint
92-108: Validate device labeling
The lines here introduce a screen share device. Confirm that the label displayed to users is consistent (e.g., “Screen Share”). If you plan on i18n, ensure that dynamic or localized labels are tested.
Line range hint
115-131: Consider verifying console warnings
When no devices are found, do you want to warn the user or log a message? Testing for console output may be optional, but it can catch unexpected behavior in production.
178-180: Positive screen share test
This test ensuresisScreenShareSupportedreturns true ifgetDisplayMediais available. Might also test what happens ifgetDisplayMediathrows or is restricted by browser policy.packages/realtime-api/src/index.ts (2)
1-1: Import grouping
Given the file’s size, consider grouping or sorting imports by functionality (e.g., external libs, local imports). This is optional, but can help with readability, especially as the codebase grows.
305-307: Use descriptive event data
When dispatchingVIDEO_INPUT_DEVICE_CHANGED, consider including more info, e.g., old device vs. new device, so the receiving side can better handle transitions without needing an additional query.packages/realtime-api/src/client.ts (5)
12-12: Centralize utility imports
You’re importing everything from'./utils'plus named imports. To keep the code consistent, consider using a single approach for importing. This is stylistic, but helps maintain clarity.
55-55: Add debug logs
Since the system is in debug mode, consider logging or highlighting the fact that_videoConfigis in use. That can help in diagnosing user issues related to device settings.
249-249: Potential concurrency
Ensure thatcreateLocalStreamis not called multiple times simultaneously. If so, you may want to add a guard (like a busy state) or queue the calls to avoid conflicts with device selection.
278-278: Edge case when changeVideoState is false
Line #L278 callsawait this.changeVideoState(false). If the engine is already stopped or in an error state, gracefully handle or confirm the engine’s readiness before calling unpublish.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 278-278: packages/realtime-api/src/client.ts#L278
Added line #L278 was not covered by tests
312-316: Ensure timely stop calls
When stopping capture, confirm that the engine is not in a transitional state (e.g., device changing). Otherwise, add a short wait or verification step to ensure consistent capture states.packages/realtime-api/test/client.spec.ts (12)
1-3: Avoid wildcard imports for better clarity
Lines 1-3 import multiple items fromvitestand@volcengine/rtc. Avoiding wildcard imports (or re-export patterns) can reduce naming conflicts, though this is largely stylistic.
5-6: Improve test coverage
We see references to* as utilsand theEventNames. Ensure that all newly added methods inutilsare covered in these client tests if relevant. This helps to confirm end-to-end usage.
33-46: Check for correct engine creation
These lines mockcreateEngine. Verify that the rest of your tests confirm correct usage (i.e., you callcreateEngineexactly once, passing the correct appId and parameters).
52-54: MediaType coverage
Although you’re mocking outMediaType, consider also testing invalid or unhandled media types if the real engine might handle them (likevideoorscreen).
80-87: Ensure coverage for engine creation
This scenario verifies the test environment. Also consider verifying in a separate test that the engine is created without test environment ifisTestEnv = false.
91-110: Event binding test
You bind and remove events, then verify the counts. Great approach. Optionally, test that each engine event is specifically bound with the correct method references for more granular coverage.
135-174: Ensure thorough device coverage
You handle ‘invalid audio device’ and ‘invalid video device’ well. Consider adding a scenario for rapidly switching devices or switching devices while a stream is active, as concurrency can cause edge cases.
180-201: Thorough state toggling
This test properly toggles audio and video states. Also test toggling them multiple times in quick succession, especially with concurrency or streaming in progress, if relevant to your use case.
220-230: Test enabling audio noise reduction
You confirm the correct config is passed. Might consider testing an edge case where the extension is unavailable or fails to activate. That would reveal if your code handles extension load errors gracefully.
251-284: Set video input device tests
You handle both camera and screen share. Great thoroughness. The test toggles them once each. If concurrency or fallback transitions are a concern, consider extended testing to handle partial states or errors during transitions.
325-339: Disconnect workflow
This snippet ensures unpublishing and leaving the room. Good coverage includes verifying that subsequent calls todisconnect()do not produce errors or that the engine can handle repeated disconnect attempts gracefully.
363-390: Player event handling
This deals with local and remote audio property reports. The logs are tested. If your system uses these properties for advanced features, consider verifying that the correct events are dispatched or transformations are applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-49.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-13-03.json(1 hunks)examples/realtime-console/src/pages/main/header.tsx(6 hunks)packages/realtime-api/README.md(1 hunks)packages/realtime-api/package.json(2 hunks)packages/realtime-api/src/client.ts(6 hunks)packages/realtime-api/src/event-handler.ts(1 hunks)packages/realtime-api/src/index.ts(5 hunks)packages/realtime-api/src/utils.ts(3 hunks)packages/realtime-api/test/client.spec.ts(1 hunks)packages/realtime-api/test/index.spec.ts(1 hunks)packages/realtime-api/test/utils.spec.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-11-41.json
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-21.json
- packages/realtime-api/src/event-handler.ts
- packages/realtime-api/README.md
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-12-49.json
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-02-13-03.json
- packages/realtime-api/test/index.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/realtime-api/test/utils.spec.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
packages/realtime-api/src/utils.ts
[warning] 85-85: packages/realtime-api/src/utils.ts#L85
Added line #L85 was not covered by tests
packages/realtime-api/src/client.ts
[warning] 268-271: packages/realtime-api/src/client.ts#L268-L271
Added lines #L268 - L271 were not covered by tests
[warning] 278-278: packages/realtime-api/src/client.ts#L278
Added line #L278 was not covered by tests
🔇 Additional comments (27)
packages/realtime-api/package.json (1)
3-3: Good version bump to beta release.Transitioning to a beta version indicates a feature-in-progress. Ensure that you coordinate versioning with other dependent packages so that they properly reference this beta release without unintended dependency conflicts.
examples/realtime-console/src/pages/main/header.tsx (1)
286-289: Add error handling to the video device change handler.
See the past suggestion on addingtry...catchand user feedback. This is especially important ifsetVideoInputDeviceis asynchronous or fails for any reason.packages/realtime-api/src/utils.ts (1)
84-85: Provide test coverage for isScreenShareDevice
According to static analysis hints, line #L85 appears uncovered by tests. Consider adding a simple test to ensure that the method correctly identifies the special “screenShare” deviceId.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-85: packages/realtime-api/src/utils.ts#L85
Added line #L85 was not covered by testspackages/realtime-api/test/utils.spec.ts (8)
22-28: Mocking global navigator
Modifyingglobal.navigatoris suitable for test environments, but be cautious with concurrency. If tests run in parallel, other tests might see the alterednavigatorobject. Consider using scoped or sandboxed test approaches.
30-38: Reset the navigator in afterAll
While the code is doing a cleanup, ensure that future tests aren’t impacted if new test suites are introduced. The cleanup routine is acceptable, but keep in mind potential conflicts if the same file is extended with additional tests.🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
53-53: Mock function usage
This line triggers themockResolvedValueaction onVERTC.enableDevices. Confirm you’re resetting or altering the mock’s return values between tests if the mock is used multiple times, to avoid cross-test interference.
59-59: Negative test coverage
You’re testing the scenario where audio permission is denied. This is good coverage. Consider adding more edge cases such as a thrown exception or partial permission (audio yes, video no).
78-78: Add mocking or verification for video
This line tests the scenario for audio false (permission denied), but you might also want to validate the behavior ifvideoproperty returns success/failure from the engine. It helps ensure full coverage ofcheckDevicePermission.
134-161: Check the fallback logic
These lines confirm fallback to legacy device enumeration. Ensure that your test environment covers all possible fallback paths (e.g., partial success fromenumerateAudioCaptureDevicesbut failure fromenumerateAudioPlaybackDevices).
164-174: Edge case testing
When both modern and legacy enumeration fail, an empty array is returned. Good coverage here. Consider verifying that the user receives a clear error message or prompt in higher-level code if no devices can be set.
183-188: Fallback to false
This snippet checks the scenario wheregetDisplayMediais not available. Good coverage. You may also consider bridging this logic with user prompts or logs if the functionality is unexpectedly disabled.packages/realtime-api/src/index.ts (3)
11-12: Inline documentation
PropertiesvideoInputDeviceIdandscreenConfigare well-documented. Ensure any consumer of this interface understands how they interact (e.g., ifvideoInputDeviceIdis'screenShare',screenConfigbecomes relevant). This looks good so far.
72-81: JSDoc completeness
These lines add or update JSDoc for the videoConfig-related parameters. The detail is thorough. Very helpful for fellow developers.
132-132: Constructor param check
You passthis._config.videoConfigto theEngineClient. Ensure that undefined values are handled gracefully ifvideoConfigis omitted.packages/realtime-api/src/client.ts (4)
22-24: Leverage optional chaining
For lines referencing_videoConfig, you already do some optional chaining. Ensure you remain consistent throughout the code for readability and to avoid potential null checks scattered around.
257-258: Check for audio device fallback
If no audio devices are available, you throw aDEVICE_ACCESS_ERROR. Confirm at a higher level that the user receives a meaningful message in the UI or logs.
268-271: Raise coverage
The static analysis warns these lines aren’t covered by tests. Consider adding a test case wherevideoOnDefaultis true but no valid device is explicitly passed, verifying that the correct default device is used.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-271: packages/realtime-api/src/client.ts#L268-L271
Added lines #L268 - L271 were not covered by tests
213-246:⚠️ Potential issueImprove error handling in setVideoInputDevice
There’s a possibility of capturing failures duringstartScreenCaptureorstartVideoCapture. Surround these blocks with try/catch to ensure that any engine-level errors are caught and dispatched.+ try { if (isScreenShareDevice(deviceId)) { ... await this.engine.startScreenCapture(this._videoConfig?.screenConfig); } else { ... await this.engine.startVideoCapture(deviceId); } + } catch (error) { + this.dispatch(EventNames.ERROR, error); + throw new RealtimeAPIError( + RealtimeError.DEVICE_ACCESS_ERROR, + error instanceof Error ? error.message : '' + ); + }Likely invalid or redundant comment.
packages/realtime-api/test/client.spec.ts (9)
10-31: Maintain a consistent mock hierarchy
This chunk is setting up a comprehensive mock engine. Keeping the mock structure well-documented and ensuring each function is tested at least once helps maintain reliability when the real engine changes.
57-57: AI ANS extension mock
There’s a mock for@volcengine/rtc/extension-ainr. You might want to test error cases or unexpected extension states if your code relies on that extension under certain conditions.
65-71: Check that mock implementations match usage
Ensure that your tests callgetAudioDevices,isScreenShareDevice,isScreenShareSupportedin the ways you’ve configured. If an untested scenario arises, it might be incorrectly mocked or lead to partial coverage.
76-77: Resetting test state
Excellent practice to clear mocks inbeforeEach. This avoids cross-test interference. Keep it consistent.
112-131: Join room flow
The lines here test the success and error flow. The coverage is good. You could also test partial success states (if the engine can return partial success or if it times out).
205-216: Message sending coverage
You verify normal and error handling. That’s good. You might want to also ensure the result is properly returned or forwarded to any downstream handlers if your system uses those statuses.
236-247: Audio playback device test
The coverage ofstartAudioPlaybackDeviceTestandstopAudioPlaybackDeviceTestis good. If the engine can fail mid-test, consider verifying the error path as well.
286-321: Create local stream coverage
You verify successful creation and handle the “no devices available” scenario. In production, ensure the user is informed if no devices can be used. Currently, you’re throwing. This is fine if you catch it at a higher level.
343-359: Stop message
You test normal and error scenarios. Consider verifying that a “conversation.chat.cancel” event is actually recognized in another part of your system, ensuring end-to-end coverage if relevant.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Version Update