-
Notifications
You must be signed in to change notification settings - Fork 13.1k
chore: allow media call clients to specify which call features they support #38574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38574 +/- ##
===========================================
- Coverage 70.38% 70.33% -0.05%
===========================================
Files 3162 3162
Lines 110697 110697
Branches 19903 19907 +4
===========================================
- Hits 77909 77859 -50
- Misses 30760 30803 +43
- Partials 2028 2035 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 21 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/media-signaling/src/definition/signals/client/request-call.ts">
<violation number="1" location="packages/media-signaling/src/definition/signals/client/request-call.ts:69">
P2: Schema allows `supportedFeatures: null`, which contradicts the TypeScript type (`CallFeature[] | undefined`). Consider making the schema non-nullable so validation matches the type.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
KevLehman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll let the nullable slide this time
| export type CallFeature = 'audio'; | ||
|
|
||
| export const callFeatureList: readonly CallFeature[] = ['audio']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about invert it?
| export type CallFeature = 'audio'; | |
| export const callFeatureList: readonly CallFeature[] = ['audio']; | |
| export const callFeatureList = ['audio'] as const; | |
| export type CallFeature = (typeof callFeatureList)[keyof typeof typeof callFeatureList]; |
Proposed changes (including videos or screenshots)
Issue(s)
VMUX-33
Steps to test or reproduce
Further comments
Once we integrate the media call system with Pexip and implement new features such as camera or screen sharing, we'll need to let clients specify which individual features they support. This PR is pre-creating the signal structure for that, so that the clients may already specify their supported features even before such features exist on the server.