fix: screen sharing bug #75
Conversation
WalkthroughThe pull request introduces a patch for 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
b791012 to
7ee3baa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/realtime-api/src/client.ts (1)
315-320: Extract duplicated screen sharing setup logic.The screen sharing initialization logic is duplicated between
setVideoInputDeviceandchangeVideoState. Consider extracting it into a reusable method.Example refactor:
+ private async initializeScreenSharing() { + try { + this.engine.setVideoSourceType( + StreamIndex.STREAM_INDEX_SCREEN, + VideoSourceType.VIDEO_SOURCE_TYPE_INTERNAL, + ); + await this.engine.startScreenCapture(this._videoConfig?.screenConfig); + await this.engine.publishScreen(MediaType.AUDIO_AND_VIDEO); + } catch (e) { + await this.engine.stopScreenCapture().catch(console.error); + throw e; + } + }Then use it in both places:
// In setVideoInputDevice if (isAutoCapture) { await this.initializeScreenSharing(); } // In changeVideoState if (isVideoOn && this._streamIndex !== StreamIndex.STREAM_INDEX_MAIN) { await this.initializeScreenSharing(); }
🛑 Comments failed to post (1)
packages/realtime-api/src/client.ts (1)
229-234: 🛠️ Refactor suggestion
Add error handling for screen sharing initialization.
While the screen sharing setup logic is correct, it lacks proper error handling. If screen capture fails, the publish operation might still be attempted, potentially leaving the system in an inconsistent state.
Consider wrapping the operations in a try-catch block:
- this.engine.setVideoSourceType( - StreamIndex.STREAM_INDEX_SCREEN, - VideoSourceType.VIDEO_SOURCE_TYPE_INTERNAL, - ); - await this.engine.startScreenCapture(this._videoConfig?.screenConfig); - await this.engine.publishScreen(MediaType.AUDIO_AND_VIDEO); + try { + this.engine.setVideoSourceType( + StreamIndex.STREAM_INDEX_SCREEN, + VideoSourceType.VIDEO_SOURCE_TYPE_INTERNAL, + ); + await this.engine.startScreenCapture(this._videoConfig?.screenConfig); + await this.engine.publishScreen(MediaType.AUDIO_AND_VIDEO); + } catch (e) { + // Cleanup on failure + await this.engine.stopScreenCapture().catch(console.error); + throw e; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { this.engine.setVideoSourceType( StreamIndex.STREAM_INDEX_SCREEN, VideoSourceType.VIDEO_SOURCE_TYPE_INTERNAL, ); await this.engine.startScreenCapture(this._videoConfig?.screenConfig); await this.engine.publishScreen(MediaType.AUDIO_AND_VIDEO); } catch (e) { // Cleanup on failure await this.engine.stopScreenCapture().catch(console.error); throw e; }
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 91.75% 92.50% +0.75%
==========================================
Files 115 46 -69
Lines 5325 2349 -2976
Branches 1081 426 -655
==========================================
- Hits 4886 2173 -2713
+ Misses 431 175 -256
+ Partials 8 1 -7
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/realtime-console/src/pages/main/header.tsx (1)
286-293: Enhanced error handling for video device changes.The function is now properly handling asynchronous device changes and provides user feedback through error messages.
Consider adding a loading state while the device change is in progress:
const handleVideoInputDeviceChange = async (value: string) => { + const prevDeviceId = videoInputDeviceId; try { + setIsChangingDevice(true); setVideoInputDeviceId(value); await clientRef.current?.setVideoInputDevice(value); } catch (error) { message.error(`Failed to set video input device: ${error}`); + setVideoInputDeviceId(prevDeviceId); console.error(error); + } finally { + setIsChangingDevice(false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-07-08-19.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-07-08-27.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-20.json(1 hunks)examples/realtime-console/src/pages/main/header.tsx(1 hunks)packages/realtime-api/package.json(1 hunks)packages/realtime-api/src/client.ts(4 hunks)packages/realtime-api/test/client.spec.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-07-08-27.json
- packages/realtime-api/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-07-08-19.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (5)
packages/realtime-api/src/client.ts (3)
315-320: Consistent video source type handling during state changes.The changes ensure consistent video source type configuration when enabling screen sharing, matching the initialization sequence in setVideoInputDevice.
327-327: Added proper cleanup for screen sharing.Added explicit unpublishing of screen video stream during cleanup, preventing potential resource leaks.
229-234: Improved screen sharing initialization sequence.The changes properly initialize screen sharing by setting the video source type before starting screen capture and publishing. This fixes potential synchronization issues in the screen sharing setup.
Let's verify the screen sharing initialization sequence:
✅ Verification successful
Screen sharing initialization sequence is properly implemented.
The initialization sequence (setVideoSourceType → startScreenCapture → publishScreen) is consistently implemented across the codebase, with proper test coverage validating the behavior. No other instances require similar fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other screen sharing initialization patterns that might need similar fixes # Search for screen capture initialization patterns ast-grep --pattern 'startScreenCapture($$$)' # Search for screen publishing patterns ast-grep --pattern 'publishScreen($$$)'Length of output: 88
Script:
#!/bin/bash # Search for screen-related patterns with proper TypeScript syntax ast-grep -l ts --pattern 'class $_ { $$$ startScreenCapture($$$) { $$$ } $$$ }' ast-grep -l ts --pattern 'class $_ { $$$ setVideoSourceType($$$) { $$$ } $$$ }' # Use ripgrep for broader context rg "startScreenCapture|publishScreen|setVideoSourceType" -A 2 -B 2Length of output: 3828
packages/realtime-api/test/client.spec.ts (1)
31-33: Added comprehensive test coverage for screen sharing.The mock engine now includes all necessary methods for testing screen sharing functionality, along with the required VideoSourceType constant.
Let's verify the test coverage:
Also applies to: 58-60
✅ Verification successful
Screen sharing test coverage is comprehensive and well-implemented
The test suite includes thorough coverage of screen sharing functionality:
- Screen share device selection and validation
- Screen capture lifecycle (start/stop)
- Stream management and player integration
- Error handling and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all screen sharing related methods are properly tested # Search for screen sharing related test cases rg -A 5 'describe.*screen'Length of output: 28
Script:
#!/bin/bash # Search for usage of screen sharing related methods in test files echo "=== Searching for screen sharing method usage ===" rg "unpublishScreen|publishScreen|setVideoSourceType" --type ts echo -e "\n=== Searching for VideoSourceType imports ===" rg "VideoSourceType" --type ts echo -e "\n=== Looking for test files containing these methods ===" fd -e spec.ts -e test.ts --exec grep -l "unpublishScreen\|publishScreen\|setVideoSourceType" {}Length of output: 1763
Script:
#!/bin/bash # Get the content of the test file focusing on screen sharing related tests echo "=== Content of client.spec.ts ===" rg -B 5 -A 10 "unpublishScreen|publishScreen|setVideoSourceType|VideoSourceType" packages/realtime-api/test/client.spec.tsLength of output: 1038
Script:
#!/bin/bash # Get the complete content of the test file to find test cases echo "=== Complete test file content ===" cat packages/realtime-api/test/client.spec.tsLength of output: 12935
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-20.json (1)
1-11: Appropriate version bump for bug fix.The patch version bump and change description accurately reflect the nature of the changes.
jackshen310
force-pushed
the
feat/realtime-api
branch
from
0f310e4 to
dee8bd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/realtime-api/src/client.ts (1)
315-327: Improve error handling for screen sharing cleanup.Consider wrapping the screen sharing operations in try-catch blocks to ensure proper cleanup even if an operation fails. This is particularly important for the
unpublishScreencall.if (isVideoOn) { if (this._streamIndex === StreamIndex.STREAM_INDEX_MAIN) { await this.engine.startVideoCapture(); } else { + try { this.engine.setVideoSourceType( StreamIndex.STREAM_INDEX_SCREEN, VideoSourceType.VIDEO_SOURCE_TYPE_INTERNAL, ); await this.engine.startScreenCapture(this._videoConfig?.screenConfig); await this.engine.publishScreen(MediaType.VIDEO); + } catch (error) { + // Ensure cleanup if any operation fails + await this.engine.stopScreenCapture().catch(console.error); + throw error; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-20.json(1 hunks)common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-32.json(1 hunks)examples/realtime-console/src/pages/main/header.tsx(1 hunks)packages/realtime-api/src/client.ts(4 hunks)packages/realtime-api/test/client.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-20.json
- examples/realtime-console/src/pages/main/header.tsx
- packages/realtime-api/test/client.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js v20 (ubuntu-latest)
🔇 Additional comments (4)
common/changes/@coze/realtime-api/feat-realtime-api_2025-01-08-06-32.json (1)
1-11: LGTM! Changelog entry is appropriate.The patch version increment and description accurately reflect the bug fix nature of the changes.
packages/realtime-api/src/client.ts (3)
10-10: LGTM! Required import for screen sharing functionality.The VideoSourceType import is necessary for the screen sharing enhancements.
Line range hint
264-269: LGTM! Improved error handling for video device checks.The addition of the video support check before throwing the device error prevents unnecessary errors when video is not supported. This change aligns well with the patch description.
229-234: Consider potential race condition in screen capture initialization.While the changes fix the screen sharing bug by setting the correct video source type, there's a potential race condition between
setVideoSourceTypeandstartScreenCapture. Consider combining these operations or ensuring proper synchronization.Run this script to check if there are any reported issues related to screen capture timing:
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Chores
@coze/realtime-apifrom1.0.3-beta.6to1.0.3-beta.7Tests