feat(camera-tab): video recording #755
Conversation
WalkthroughThe pull request introduces significant enhancements to media capture functionalities across multiple files. Key changes include the conditional configuration of the Changes
Possibly related PRs
🪧 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 (
|
feat(camera-tab): added destroy media sources feat(camera-tab): carry on feat: added mobile support
egordidenko
force-pushed
the
feat/camera-tab-video-record
branch
from
d20e2e8 to
0fbf343
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: 2
🧹 Outside diff range and nitpick comments (4)
blocks/CameraSource/CameraSource.js (2)
272-280: Consider Renaming_toggleRecordingto Reflect Its FunctionalityThe
_toggleRecordingmethod toggles the play and pause state of the video playback, not the recording. Renaming it to_togglePlaybackwould better describe its purpose and improve code clarity.
540-552: Avoid Reassigning themimeVariable to Different TypesReassigning
mimefrom a string to an array can be confusing and may lead to errors. Use a separate variable for the split result to enhance readability.Apply this diff to improve clarity:
-/** @type {string | string[]} */ (mime) = mime.split('/'); -if (mime?.[0] === 'video') { - // e.g. "x-matroska;codecs=avc1,opus" - mime = mime.slice(1).join('/'); +const mimeParts = mime.split('/'); +if (mimeParts[0] === 'video') { + // e.g. "x-matroska;codecs=avc1,opus" + const mimeSubtype = mimeParts.slice(1).join('/');blocks/CameraSource/camera-source.css (1)
112-120: Ensure Consistent Use of CSS VariablesFor maintainability and theming flexibility, consider using CSS variables for spacing and sizing in the new styles for
.uc-controls.abstract/UploaderPublicApi.js (1)
166-166: Consider extracting file type constantsThe file types
'image/*'and'video/*'should be extracted as constants to improve maintainability and reusability. This aligns with the existing pattern whereIMAGE_ACCEPT_LISTis used.+const CAMERA_FILE_TYPES = { + IMAGE: 'image/*', + VIDEO: 'video/*', +}; - fileInput.accept = this.cfg.enableVideoRecording ? ['image/*', 'video/*'].join(',') : 'image/*'; + fileInput.accept = this.cfg.enableVideoRecording + ? [CAMERA_FILE_TYPES.IMAGE, CAMERA_FILE_TYPES.VIDEO].join(',') + : CAMERA_FILE_TYPES.IMAGE;
🛑 Comments failed to post (2)
blocks/CameraSource/CameraSource.js (2)
645-656:
⚠️ Potential issuePreserve
DEFAULT_VIDEO_CONFIGWhen Settingconstraints.videoOverwriting
constraints.videowith the device ID results in losing the default video configurations. Merge theDEFAULT_VIDEO_CONFIGwith the device ID to retain the default settings.Apply this diff to fix the issue:
if (this._selectedCameraId) { - constraints.video = { - deviceId: { - exact: this._selectedCameraId, - }, - }; + constraints.video = { + ...DEFAULT_VIDEO_CONFIG, + deviceId: { + exact: this._selectedCameraId, + }, + }; }📝 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.const constraints = { video: DEFAULT_VIDEO_CONFIG, audio: this.cfg.enableAudioRecording ? {} : false, }; if (this._selectedCameraId) { constraints.video = { ...DEFAULT_VIDEO_CONFIG, deviceId: { exact: this._selectedCameraId, }, }; }
808-815:
⚠️ Potential issueCheck for Undefined Permission Responses Before Removing Listeners
In the
_destroymethod,this[${permission}Response]may be undefined if the permissions API is not supported. Add a check to prevent potential runtime errors.Apply this diff to fix the issue:
_destroy() { for (const permission of DEFAULT_PERMISSIONS) { - this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); + if (this[`${permission}Response`]) { + this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); + } } navigator.mediaDevices.removeEventListener('devicechange', this._getDevices); }📝 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._destroy() { for (const permission of DEFAULT_PERMISSIONS) { if (this[`${permission}Response`]) { this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); } } navigator.mediaDevices.removeEventListener('devicechange', this._getDevices); }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
blocks/CameraSource/camera-source.css (2)
111-120: Consider making the min-height responsive.The fixed
min-height: 74pxmight cause layout issues on smaller screens. Consider using relative units or media queries for better responsiveness.- min-height: 74px; + min-height: min(74px, 15vh);
190-194: Consider using CSS custom properties for spacing.Using CSS variables for spacing values would improve maintainability and consistency.
- inset: 0 var(--uc-padding) var(--uc-padding); + --camera-action-spacing: var(--uc-padding); + inset: 0 var(--camera-action-spacing) var(--camera-action-spacing);blocks/CameraSource/CameraSource.js (2)
26-34: Consider enhancing the time formatting utilityThe current implementation could be improved to handle edge cases and provide more flexibility.
Consider this enhanced version:
-function formatTime(time) { - const minutes = Math.floor(time / 60) - .toString() - .padStart(2, '0'); - const seconds = Math.floor(time % 60) - .toString() - .padStart(2, '0'); - return `${minutes}:${seconds}`; +function formatTime(time) { + if (time < 0) return '00:00'; + const hours = Math.floor(time / 3600); + const minutes = Math.floor((time % 3600) / 60); + const seconds = Math.floor(time % 60); + if (hours > 0) { + return `${hours}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; + } + return `${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; +}
Line range hint
841-933: Enhance accessibility in the templateThe template could benefit from improved accessibility attributes.
Add ARIA attributes and roles:
- <button type="button" class="uc-mini-btn" set="onclick: *historyBack" l10n="@title:back"> + <button + type="button" + class="uc-mini-btn" + set="onclick: *historyBack" + l10n="@title:back" + aria-label="Go back"> <uc-icon name="back"></uc-icon> </button> - <video + <video + aria-label="Camera preview" muted autoplay playsinline set="srcObject: video; style.transform: videoTransformCss; @hidden: videoHidden" ref="video" ></video>types/exported.d.ts (4)
82-87: LGTM! Consider enhancing the documentation.The type definition is well-structured. Consider adding
@sincetag to help track when this feature was introduced./** * The default tab to open in the camera modal, * it is possible to select video or photo capture * @default 'photo' + * @since 1.0.0 */
88-89: Add JSDoc documentation for recording flags.Please add documentation to clarify:
- The purpose and effect of each flag
- Whether these flags can be toggled during runtime
- The interaction between these flags (e.g., can audio be enabled without video?)
+ /** + * Enable audio recording capability in the camera tab + * @default false + */ enableAudioRecording: boolean; + /** + * Enable video recording capability in the camera tab + * @default false + */ enableVideoRecording: boolean;
91-95: Enhance documentation and add type constraints.The type definition needs more clarity:
- Explicitly mention that the unit is in seconds
- Consider adding validation constraints for minimum value
/** - * The maximum duration of the video recording in seconds + * The maximum duration of the video recording in seconds. + * Set to null for unlimited duration. + * Must be a positive number when specified. * @default null + * @minimum 1 */ - maxDurationVideoRecord: number | null + maxDurationVideoRecord: (number & { readonly brand: 'PositiveNumber' }) | null
97-101: Enhance documentation for MediaRecorder options.The documentation should:
- List commonly used properties from MediaRecorderOptions
- Provide example configurations
- Include a link to MDN documentation
/** - * A dictionary object that can contain - * the following properties from MediaRecorderOptions + * Configuration options for the MediaRecorder API. + * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/MediaRecorder#parameters} + * + * Supported options include: + * - mimeType: The MIME type of the recorded media + * - audioBitsPerSecond: The bits per second for audio encoding + * - videoBitsPerSecond: The bits per second for video encoding + * - bitsPerSecond: The combined bits per second for audio and video + * + * @example + * { + * mimeType: 'video/webm;codecs=vp9', + * videoBitsPerSecond: 2500000 + * } + * + * @default null */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (8)
blocks/themes/uc-basic/icons/camera-full.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/microphone-mute.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/microphone.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/pause.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/play.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/square.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/video-camera-full.svgis excluded by!**/*.svgblocks/themes/uc-basic/icons/video-camera.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
abstract/UploaderPublicApi.js(1 hunks)blocks/CameraSource/CameraSource.js(5 hunks)blocks/CameraSource/camera-source.css(4 hunks)blocks/Config/Config.js(2 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(3 hunks)blocks/themes/uc-basic/common.css(1 hunks)blocks/themes/uc-basic/svg-sprite.js(1 hunks)blocks/themes/uc-basic/theme.css(1 hunks)types/exported.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- blocks/themes/uc-basic/theme.css
- blocks/Config/initialConfig.js
- blocks/themes/uc-basic/common.css
- abstract/UploaderPublicApi.js
- blocks/Config/normalizeConfigValue.js
- blocks/Config/Config.js
🔇 Additional comments (4)
blocks/CameraSource/camera-source.css (2)
23-25: LGTM! Clean implementation of conditional visibility.
The selector ensures controls are hidden until the component is properly initialized.
141-147: LGTM! Well-structured state management.
Good use of :where() selector for managing specificity, and appropriate handling of pointer events during recording state.
blocks/themes/uc-basic/svg-sprite.js (1)
1-1: LGTM! Icons follow consistent style.
The new camera icons maintain consistency with the existing icon set and follow the established patterns.
types/exported.d.ts (1)
97-101: Verify browser compatibility for MediaRecorder options.
Please ensure that the supported MediaRecorder options are compatible across target browsers.
| _permissionAccess = async () => { | ||
| try { | ||
| for (const permission of DEFAULT_PERMISSIONS) { | ||
| // @ts-ignore https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API | ||
| this[`${permission}Response`] = await navigator.permissions.query({ name: permission }); | ||
|
|
||
| this[`${permission}Response`].addEventListener('change', this._handlePermissionsChange); | ||
| } | ||
| } catch (error) { | ||
| console.log('Failed to use permissions API. Fallback to manual request mode.', error); | ||
| this._capture(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Implement consistent error handling strategy
The error handling across different methods is inconsistent and could be more informative.
Consider implementing a consistent error handling strategy:
+class CameraError extends Error {
+ constructor(code, message, originalError = null) {
+ super(message);
+ this.code = code;
+ this.originalError = originalError;
+ }
+}
+
+const ErrorCodes = {
+ PERMISSION_DENIED: 'PERMISSION_DENIED',
+ DEVICE_NOT_FOUND: 'DEVICE_NOT_FOUND',
+ INITIALIZATION_FAILED: 'INITIALIZATION_FAILED',
+};
_permissionAccess = async () => {
try {
for (const permission of DEFAULT_PERMISSIONS) {
this[`${permission}Response`] = await navigator.permissions.query({ name: permission });
this[`${permission}Response`].addEventListener('change', this._handlePermissionsChange);
}
} catch (error) {
- console.log('Failed to use permissions API. Fallback to manual request mode.', error);
+ throw new CameraError(
+ ErrorCodes.PERMISSION_DENIED,
+ 'Failed to access permissions API',
+ error
+ );
}
};
_getDevices = async () => {
try {
const devices = await navigator.mediaDevices.enumerateDevices();
// ... rest of the method
} catch (error) {
- console.log('Failed to get devices', error);
+ throw new CameraError(
+ ErrorCodes.DEVICE_NOT_FOUND,
+ 'Failed to enumerate media devices',
+ error
+ );
}
};Also applies to: 718-756
| _handlePhoto = (status) => { | ||
| if (status === CameraSource.events.SHOT) { | ||
| this.set$({ | ||
| tabVideoHidden: true, | ||
| cameraHidden: true, | ||
| tabCameraHidden: true, | ||
| cameraActionsHidden: false, | ||
| cameraSelectHidden: true, | ||
| }); | ||
| } | ||
|
|
||
| if (status === CameraSource.events.RETAKE || status === CameraSource.events.ACCEPT) { | ||
| this.set$({ | ||
| tabVideoHidden: !this.cfg.enableVideoRecording, | ||
| cameraHidden: false, | ||
| tabCameraHidden: false, | ||
| cameraActionsHidden: true, | ||
| cameraSelectHidden: this.cameraDevices.length <= 1, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| /** @param {CameraStatus} status */ | ||
| _handleVideo = (status) => { | ||
| if (status === CameraSource.events.PLAY) { | ||
| this.set$({ | ||
| timerHidden: false, | ||
| tabCameraHidden: true, | ||
|
|
||
| cameraSelectHidden: true, | ||
| audioSelectHidden: true, | ||
|
|
||
| currentTimelineIcon: 'pause', | ||
| currentIcon: 'square', | ||
| mutableClassButton: 'uc-shot-btn uc-camera-action uc-stop-record', | ||
| }); | ||
| } | ||
|
|
||
| if (status === CameraSource.events.STOP) { | ||
| this.set$({ | ||
| timerHidden: false, | ||
| cameraHidden: true, | ||
| audioToggleMicorphoneHidden: true, | ||
| cameraActionsHidden: false, | ||
| }); | ||
| } | ||
|
|
||
| if (status === CameraSource.events.RETAKE || status === CameraSource.events.ACCEPT) { | ||
| this.set$({ | ||
| timerHidden: true, | ||
| tabCameraHidden: false, | ||
| cameraHidden: false, | ||
| cameraActionsHidden: true, | ||
| audioToggleMicorphoneHidden: !this.cfg.enableAudioRecording, | ||
| currentIcon: 'video-camera-full', | ||
| mutableClassButton: 'uc-shot-btn uc-camera-action', | ||
|
|
||
| audioSelectHidden: !this.cfg.enableAudioRecording, | ||
| cameraSelectHidden: this.cameraDevices.length <= 1, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * @private | ||
| * @param {CameraStatus} status | ||
| */ | ||
| _setCameraState = (status) => { | ||
| if ( | ||
| this._activeTab === CameraSource.types.PHOTO && | ||
| (status === 'shot' || status === 'retake' || status === 'accept') | ||
| ) { | ||
| this._handlePhoto(status); | ||
| } | ||
|
|
||
| if ( | ||
| this._activeTab === CameraSource.types.VIDEO && | ||
| (status === 'play' || | ||
| status === 'stop' || | ||
| status === 'retake' || | ||
| status === 'accept' || | ||
| status === 'pause' || | ||
| status === 'resume') | ||
| ) { | ||
| this._handleVideo(status); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing a state machine for UI management
The current implementation mixes UI state management with business logic, making it harder to maintain and test.
Consider implementing a state machine pattern:
+const CameraStates = {
+ IDLE: 'idle',
+ PHOTO_CAPTURE: 'photo_capture',
+ PHOTO_PREVIEW: 'photo_preview',
+ VIDEO_RECORDING: 'video_recording',
+ VIDEO_PREVIEW: 'video_preview',
+};
+
+const StateTransitions = {
+ [CameraStates.IDLE]: {
+ photo: CameraStates.PHOTO_CAPTURE,
+ video: CameraStates.VIDEO_RECORDING,
+ },
+ [CameraStates.PHOTO_CAPTURE]: {
+ accept: CameraStates.IDLE,
+ retake: CameraStates.IDLE,
+ },
+ // ... other state transitions
+};
+
+class CameraStateManager {
+ constructor(component) {
+ this.component = component;
+ this.currentState = CameraStates.IDLE;
+ }
+
+ transition(action) {
+ const nextState = StateTransitions[this.currentState][action];
+ if (nextState) {
+ this.currentState = nextState;
+ this._updateUI();
+ }
+ }
+
+ _updateUI() {
+ // Handle UI updates based on state
+ }
+}This would make the state transitions more predictable and easier to test.
Committable suggestion skipped: line range outside the PR's diff.
| await this._permissionAccess(); | ||
| await this._requestDeviceAccess(); | ||
| await this._capture(); | ||
| }; | ||
|
|
||
| _onDeactivate = async () => { | ||
| if (this._unsubPermissions) { | ||
| this._unsubPermissions(); | ||
| } | ||
|
|
||
| async initCallback() { | ||
| /** Calling this method here because safari and firefox don't support the inactive event yet */ | ||
| const isChromium = !!window.chrome; | ||
| if (!isChromium) { | ||
| this._setPermissionsState('denied'); | ||
| } | ||
|
|
||
| this._stopCapture(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Improve lifecycle management and resource cleanup
The current implementation could leak resources and doesn't handle all edge cases.
Consider these improvements:
_onActivate = async () => {
+ try {
await this._permissionAccess();
await this._requestDeviceAccess();
await this._capture();
+ } catch (error) {
+ console.error('Failed to activate camera:', error);
+ // Ensure cleanup happens even if initialization fails
+ await this._onDeactivate();
+ throw error;
+ }
};
_onDeactivate = async () => {
+ try {
if (this._unsubPermissions) {
this._unsubPermissions();
+ this._unsubPermissions = null;
}
/** Calling this method here because safari and firefox don't support the inactive event yet */
const isChromium = !!window.chrome;
if (!isChromium) {
this._setPermissionsState('denied');
}
this._stopCapture();
+
+ // Clean up any remaining resources
+ if (this._mediaRecorder) {
+ this._mediaRecorder = null;
+ }
+ this._chunks = [];
+ } catch (error) {
+ console.error('Error during deactivation:', error);
+ // Still throw to indicate failure
+ throw error;
+ }
};📝 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.
| _onActivate = async () => { | |
| await this._permissionAccess(); | |
| await this._requestDeviceAccess(); | |
| await this._capture(); | |
| }; | |
| _onDeactivate = async () => { | |
| if (this._unsubPermissions) { | |
| this._unsubPermissions(); | |
| } | |
| async initCallback() { | |
| /** Calling this method here because safari and firefox don't support the inactive event yet */ | |
| const isChromium = !!window.chrome; | |
| if (!isChromium) { | |
| this._setPermissionsState('denied'); | |
| } | |
| this._stopCapture(); | |
| }; | |
| _onActivate = async () => { | |
| try { | |
| await this._permissionAccess(); | |
| await this._requestDeviceAccess(); | |
| await this._capture(); | |
| } catch (error) { | |
| console.error('Failed to activate camera:', error); | |
| // Ensure cleanup happens even if initialization fails | |
| await this._onDeactivate(); | |
| throw error; | |
| } | |
| }; | |
| _onDeactivate = async () => { | |
| try { | |
| if (this._unsubPermissions) { | |
| this._unsubPermissions(); | |
| this._unsubPermissions = null; | |
| } | |
| /** Calling this method here because safari and firefox don't support the inactive event yet */ | |
| const isChromium = !!window.chrome; | |
| if (!isChromium) { | |
| this._setPermissionsState('denied'); | |
| } | |
| this._stopCapture(); | |
| // Clean up any remaining resources | |
| if (this._mediaRecorder) { | |
| this._mediaRecorder = null; | |
| } | |
| this._chunks = []; | |
| } catch (error) { | |
| console.error('Error during deactivation:', error); | |
| // Still throw to indicate failure | |
| throw error; | |
| } | |
| }; |
| try { | ||
| this._chunks = []; | ||
| this._options = { | ||
| ...this.cfg.optionsMediaRecorder, | ||
| }; | ||
|
|
||
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | ||
|
|
||
| if ( | ||
| this.cfg.optionsMediaRecorder?.mimeType && | ||
| MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType) | ||
| ) { | ||
| this._options.mimeType = this.cfg.optionsMediaRecorder.mimeType; | ||
| } else if ( | ||
| isFirefox && | ||
| this.cfg.optionsMediaRecorder?.mimeType && | ||
| MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType) | ||
| ) { | ||
| const mimeType = this.cfg.optionsMediaRecorder?.mimeType; | ||
|
|
||
| if (mimeType && !MediaRecorder.isTypeSupported(mimeType)) { | ||
| throw new Error(`MIME type ${mimeType} is not supported`); | ||
| } | ||
|
|
||
| this._options.mimeType = DEFAULT_VIDEO_FORMAT; | ||
| } | ||
|
|
||
| if (this._stream) { | ||
| this._mediaRecorder = new MediaRecorder(this._stream, this._options); | ||
| this._mediaRecorder.start(); | ||
|
|
||
| this._mediaRecorder.addEventListener('dataavailable', (e) => { | ||
| this._chunks.push(e.data); | ||
| }); | ||
|
|
||
| this._startTimer(); | ||
|
|
||
| this.classList.add('uc-recording'); | ||
| this._setCameraState(CameraSource.events.PLAY); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to start recording', error); |
There was a problem hiding this comment.
Improve error handling and memory management in recording
The recording implementation has several areas that could be improved:
- Memory leaks could occur if chunks aren't cleared on errors
- Error handling is too generic
- Browser compatibility checks could be more robust
Consider these improvements:
_startRecording = () => {
try {
this._chunks = [];
this._options = {
...this.cfg.optionsMediaRecorder,
};
const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1;
+ const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent);
if (
this.cfg.optionsMediaRecorder?.mimeType &&
MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType)
) {
this._options.mimeType = this.cfg.optionsMediaRecorder.mimeType;
- } else if (
- isFirefox &&
- this.cfg.optionsMediaRecorder?.mimeType &&
- MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType)
- ) {
+ } else {
+ // Try to find a supported MIME type
+ const mimeTypes = [
+ 'video/webm;codecs=vp9,opus',
+ 'video/webm;codecs=vp8,opus',
+ 'video/webm',
+ 'video/mp4'
+ ];
+
+ const supportedMimeType = mimeTypes.find(type => MediaRecorder.isTypeSupported(type));
+ if (!supportedMimeType) {
+ throw new Error('No supported MIME type found for video recording');
+ }
+ this._options.mimeType = supportedMimeType;
+ }
if (this._stream) {
this._mediaRecorder = new MediaRecorder(this._stream, this._options);
this._mediaRecorder.start();
+ // Clear chunks if recording is stopped due to errors
+ const clearChunksOnError = () => {
+ this._chunks = [];
+ this._mediaRecorder.removeEventListener('error', clearChunksOnError);
+ };
+ this._mediaRecorder.addEventListener('error', clearChunksOnError);
this._mediaRecorder.addEventListener('dataavailable', (e) => {
this._chunks.push(e.data);
});
this._startTimer();
this.classList.add('uc-recording');
this._setCameraState(CameraSource.events.PLAY);
}
} catch (error) {
- console.error('Failed to start recording', error);
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Failed to start recording:', errorMessage);
+ this._chunks = []; // Clean up chunks on error
+ throw new Error(`Recording failed: ${errorMessage}`);
}
};📝 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.
| _startRecording = () => { | |
| try { | |
| this._chunks = []; | |
| this._options = { | |
| ...this.cfg.optionsMediaRecorder, | |
| }; | |
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | |
| if ( | |
| this.cfg.optionsMediaRecorder?.mimeType && | |
| MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType) | |
| ) { | |
| this._options.mimeType = this.cfg.optionsMediaRecorder.mimeType; | |
| } else if ( | |
| isFirefox && | |
| this.cfg.optionsMediaRecorder?.mimeType && | |
| MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType) | |
| ) { | |
| const mimeType = this.cfg.optionsMediaRecorder?.mimeType; | |
| if (mimeType && !MediaRecorder.isTypeSupported(mimeType)) { | |
| throw new Error(`MIME type ${mimeType} is not supported`); | |
| } | |
| this._options.mimeType = DEFAULT_VIDEO_FORMAT; | |
| } | |
| if (this._stream) { | |
| this._mediaRecorder = new MediaRecorder(this._stream, this._options); | |
| this._mediaRecorder.start(); | |
| this._mediaRecorder.addEventListener('dataavailable', (e) => { | |
| this._chunks.push(e.data); | |
| }); | |
| this._startTimer(); | |
| this.classList.add('uc-recording'); | |
| this._setCameraState(CameraSource.events.PLAY); | |
| } | |
| } catch (error) { | |
| _startRecording = () => { | |
| try { | |
| this._chunks = []; | |
| this._options = { | |
| ...this.cfg.optionsMediaRecorder, | |
| }; | |
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | |
| const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); | |
| if ( | |
| this.cfg.optionsMediaRecorder?.mimeType && | |
| MediaRecorder.isTypeSupported(this.cfg.optionsMediaRecorder.mimeType) | |
| ) { | |
| this._options.mimeType = this.cfg.optionsMediaRecorder.mimeType; | |
| } else { | |
| // Try to find a supported MIME type | |
| const mimeTypes = [ | |
| 'video/webm;codecs=vp9,opus', | |
| 'video/webm;codecs=vp8,opus', | |
| 'video/webm', | |
| 'video/mp4' | |
| ]; | |
| const supportedMimeType = mimeTypes.find(type => MediaRecorder.isTypeSupported(type)); | |
| if (!supportedMimeType) { | |
| throw new Error('No supported MIME type found for video recording'); | |
| } | |
| this._options.mimeType = supportedMimeType; | |
| } | |
| if (this._stream) { | |
| this._mediaRecorder = new MediaRecorder(this._stream, this._options); | |
| this._mediaRecorder.start(); | |
| // Clear chunks if recording is stopped due to errors | |
| const clearChunksOnError = () => { | |
| this._chunks = []; | |
| this._mediaRecorder.removeEventListener('error', clearChunksOnError); | |
| }; | |
| this._mediaRecorder.addEventListener('error', clearChunksOnError); | |
| this._mediaRecorder.addEventListener('dataavailable', (e) => { | |
| this._chunks.push(e.data); | |
| }); | |
| this._startTimer(); | |
| this.classList.add('uc-recording'); | |
| this._setCameraState(CameraSource.events.PLAY); | |
| } | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | |
| console.error('Failed to start recording:', errorMessage); | |
| this._chunks = []; // Clean up chunks on error | |
| throw new Error(`Recording failed: ${errorMessage}`); | |
| } | |
| }; |
blocks/CameraSource/CameraSource.js
Outdated
| const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | ||
|
|
||
| if ( | ||
| this.cfg.optionsMediaRecorder?.mimeType && |
There was a problem hiding this comment.
Looks like we have a problem here.
In the v3 widget, we have a separate option videoPreferredMimeTypes that contains an array of preferred mimes, that was filtered with isTypeSupported. So user can specify multiple acceptable mimes and we select one of the supported ones.
In the current implementation, the user could specify the only one mime type and should check the support himself. Otherwise, uploader just fall in the error. I think we need to re-implement previous logic with a preferred mimes array.
There was a problem hiding this comment.
I'm suggesting to postpone releasing of this setting and add it later
nd0ut
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
blocks/Config/initialConfig.js (1)
70-74: Consider grouping related configuration properties.The camera-related configuration properties could be better organized. Consider grouping them under a single object for better maintainability:
- defaultCameraMode: 'photo', - enableAudioRecording: true, - enableVideoRecording: true, - maxVideoRecordingDuration: null, - mediaRecorerOptions: null, + camera: { + defaultMode: 'photo', + audio: { + enabled: true + }, + video: { + enabled: true, + maxRecordingDuration: null, + recorderOptions: null + } + },blocks/CameraSource/CameraSource.js (2)
8-18: Consider making video configuration configurable.The
DEFAULT_VIDEO_CONFIGis hardcoded. Consider making it configurable through the camera configuration to allow users to customize video quality settings:-const DEFAULT_VIDEO_CONFIG = { +const getVideoConfig = (userConfig) => ({ width: { - ideal: 1920, + ideal: userConfig?.width ?? 1920, }, height: { - ideal: 1080, + ideal: userConfig?.height ?? 1080, }, frameRate: { - ideal: 30, + ideal: userConfig?.frameRate ?? 30, }, -}; +});
Line range hint
866-880: Consider adding loading state for video element.The video element could benefit from a loading state indicator:
<video muted autoplay playsinline set="srcObject: video; style.transform: videoTransformCss; @hidden: videoHidden" + @loadstart="this.classList.add('loading')" + @loadeddata="this.classList.remove('loading')" ref="video" ></video> +<div class="uc-video-loader" set="@hidden: !videoHidden"> + <uc-icon name="spinner" class="uc-spinner"></uc-icon> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
blocks/CameraSource/CameraSource.js(5 hunks)blocks/Config/Config.js(2 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(3 hunks)types/exported.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- blocks/Config/normalizeConfigValue.js
- blocks/Config/Config.js
- types/exported.d.ts
🔇 Additional comments (3)
blocks/Config/initialConfig.js (1)
74-74:
Fix typo in configuration property name.
The property mediaRecorerOptions has a typo (missing 'd'). It should be mediaRecorderOptions.
blocks/CameraSource/CameraSource.js (2)
211-253:
Improve error handling in recording initialization.
The error handling in _startRecording needs improvement:
- Errors are only logged, not propagated
- MIME type support check is duplicated
- Firefox-specific handling could be simplified
Additionally, verify the MIME type support across browsers:
#!/bin/bash
# Description: Check browser compatibility for video MIME types
ast-grep --pattern 'MediaRecorder.isTypeSupported($mime)'758-776:
Improve lifecycle management in activation/deactivation.
The lifecycle management in _onActivate and _onDeactivate needs improvement:
- No error handling in activation sequence
- Incomplete cleanup in deactivation
- Browser-specific handling could be more robust
| _destroy() { | ||
| for (const permission of DEFAULT_PERMISSIONS) { | ||
| this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); | ||
| } | ||
|
|
||
| navigator.mediaDevices.removeEventListener('devicechange', this._getDevices); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance resource cleanup in destroy method.
The _destroy method should handle cases where permissions weren't granted:
_destroy() {
+ try {
for (const permission of DEFAULT_PERMISSIONS) {
- this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange);
+ if (this[`${permission}Response`]) {
+ this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange);
+ this[`${permission}Response`] = null;
+ }
}
navigator.mediaDevices.removeEventListener('devicechange', this._getDevices);
+ this._makeStreamInactive();
+ } catch (error) {
+ console.error('Error in cleanup:', error);
+ }
}📝 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.
| _destroy() { | |
| for (const permission of DEFAULT_PERMISSIONS) { | |
| this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); | |
| } | |
| navigator.mediaDevices.removeEventListener('devicechange', this._getDevices); | |
| } | |
| _destroy() { | |
| try { | |
| for (const permission of DEFAULT_PERMISSIONS) { | |
| if (this[`${permission}Response`]) { | |
| this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange); | |
| this[`${permission}Response`] = null; | |
| } | |
| } | |
| navigator.mediaDevices.removeEventListener('devicechange', this._getDevices); | |
| this._makeStreamInactive(); | |
| } catch (error) { | |
| console.error('Error in cleanup:', error); | |
| } | |
| } |
| if (!this._stream) return false; | ||
|
|
||
| const audioTracks = this._stream?.getAudioTracks(); | ||
| const videoTracks = this._stream?.getVideoTracks(); | ||
|
|
||
| /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => track.stop()); | ||
| /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => track.stop()); | ||
| }; | ||
|
|
||
| _stopCapture = () => { | ||
| if (this._capturing) { | ||
| this.ref.video.volume = 0; | ||
| this.$.video?.getTracks()[0].stop(); | ||
| this.$.video = null; | ||
|
|
||
| this._makeStreamInactive(); | ||
|
|
||
| this._capturing = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve stream cleanup in _makeStreamInactive.
The stream cleanup could be more robust:
- No error handling for track stopping
- No cleanup of MediaRecorder instance
- Stream reference isn't nullified after cleanup
Apply this improvement:
_makeStreamInactive = () => {
if (!this._stream) return false;
const audioTracks = this._stream?.getAudioTracks();
const videoTracks = this._stream?.getVideoTracks();
- /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => track.stop());
- /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => track.stop());
+ try {
+ /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => {
+ if (track.readyState === 'live') {
+ track.stop();
+ }
+ });
+ /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => {
+ if (track.readyState === 'live') {
+ track.stop();
+ }
+ });
+
+ if (this._mediaRecorder?.state !== 'inactive') {
+ this._mediaRecorder?.stop();
+ }
+ this._mediaRecorder = null;
+ this._stream = null;
+ return true;
+ } catch (error) {
+ console.error('Error cleaning up media stream:', error);
+ return false;
+ }
};📝 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.
| _makeStreamInactive = () => { | |
| if (!this._stream) return false; | |
| const audioTracks = this._stream?.getAudioTracks(); | |
| const videoTracks = this._stream?.getVideoTracks(); | |
| /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => track.stop()); | |
| /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => track.stop()); | |
| }; | |
| _stopCapture = () => { | |
| if (this._capturing) { | |
| this.ref.video.volume = 0; | |
| this.$.video?.getTracks()[0].stop(); | |
| this.$.video = null; | |
| this._makeStreamInactive(); | |
| this._capturing = false; | |
| _makeStreamInactive = () => { | |
| if (!this._stream) return false; | |
| const audioTracks = this._stream?.getAudioTracks(); | |
| const videoTracks = this._stream?.getVideoTracks(); | |
| try { | |
| /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => { | |
| if (track.readyState === 'live') { | |
| track.stop(); | |
| } | |
| }); | |
| /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => { | |
| if (track.readyState === 'live') { | |
| track.stop(); | |
| } | |
| }); | |
| if (this._mediaRecorder?.state !== 'inactive') { | |
| this._mediaRecorder?.stop(); | |
| } | |
| this._mediaRecorder = null; | |
| this._stream = null; | |
| return true; | |
| } catch (error) { | |
| console.error('Error cleaning up media stream:', error); | |
| return false; | |
| } | |
| }; | |
| _stopCapture = () => { | |
| if (this._capturing) { | |
| this.ref.video.volume = 0; | |
| this.$.video?.getTracks()[0].stop(); | |
| this.$.video = null; | |
| this._makeStreamInactive(); | |
| this._capturing = false; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
blocks/CameraSource/CameraSource.js (3)
8-18: Consider making video configuration more flexible.The DEFAULT_VIDEO_CONFIG uses fixed values which might not work well on all devices, especially on mobile with limited capabilities.
Consider making the configuration more adaptive:
const DEFAULT_VIDEO_CONFIG = { width: { - ideal: 1920, + min: 640, + ideal: 1920, + max: 3840 }, height: { - ideal: 1080, + min: 480, + ideal: 1080, + max: 2160 }, frameRate: { - ideal: 30, + min: 24, + ideal: 30, + max: 60 }, };
50-64: Add TypeScript annotations for better type safety.The private properties could benefit from more specific TypeScript types.
Consider adding more specific type annotations:
- /** @type {BlobPart[]} */ + /** @type {Blob[]} */ _chunks = []; - /** @type {string | null} */ + /** @type {MediaDeviceId | null} */ _selectedAudioId = null; - /** @type {string | null} */ + /** @type {MediaDeviceId | null} */ _selectedCameraId = null;
Line range hint
854-868: Enhance video element accessibility.The video element could benefit from better accessibility attributes.
Add ARIA attributes and labels:
<video muted autoplay playsinline + aria-label="Camera preview" + role="application" + tabindex="0" set="srcObject: video; style.transform: videoTransformCss; @hidden: videoHidden" ref="video" ></video>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
blocks/CameraSource/CameraSource.js(5 hunks)
🔇 Additional comments (2)
blocks/CameraSource/CameraSource.js (2)
211-241:
Improve MIME type handling and error management in recording.
The current implementation has several areas that could be improved:
- Error handling is too generic
- MIME type fallback logic could be more robust
- No cleanup on error
This issue was previously identified. The suggested improvements from the past review comment are still valid and should be implemented.
610-618:
Resource cleanup needs improvement.
The stream cleanup could be more robust:
- No error handling for track stopping
- Stream reference isn't nullified after cleanup
This issue was previously identified in the past review comments. The suggested improvements for proper resource cleanup should be implemented.
| _capture = async () => { | ||
| const constraints = { | ||
| video: DEFAULT_VIDEO_CONFIG, | ||
| audio: this.cfg.enableAudioRecording ? {} : false, | ||
| }; | ||
|
|
||
| if (this._selectedCameraId) { | ||
| constr.video.deviceId = { | ||
| exact: this._selectedCameraId, | ||
| constraints.video = { | ||
| deviceId: { | ||
| exact: this._selectedCameraId, | ||
| }, | ||
| }; | ||
| } | ||
| /** @private */ | ||
| this._canvas = document.createElement('canvas'); | ||
| /** @private */ | ||
| this._ctx = this._canvas.getContext('2d'); | ||
|
|
||
| if (this._selectedAudioId && this.cfg.enableAudioRecording) { | ||
| constraints.audio = { | ||
| deviceId: { | ||
| exact: this._selectedAudioId, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| // Mute the video to prevent feedback for Firefox | ||
| this.ref.video.volume = 0; | ||
|
|
||
| try { | ||
| this._setPermissionsState('prompt'); | ||
| let stream = await navigator.mediaDevices.getUserMedia(constr); | ||
| stream.addEventListener('inactive', () => { | ||
| this._stream = await navigator.mediaDevices.getUserMedia(constraints); | ||
|
|
||
| this._stream.addEventListener('inactive', () => { | ||
| this._setPermissionsState('denied'); | ||
| }); | ||
| this.$.video = stream; | ||
|
|
||
| this.$.video = this._stream; | ||
| /** @private */ | ||
| this._capturing = true; | ||
| this._setPermissionsState('granted'); | ||
| } catch (err) { | ||
| } catch (error) { | ||
| this._setPermissionsState('denied'); | ||
| console.error('Failed to capture camera', err); | ||
| console.log('Failed to capture camera', error); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in media capture.
The capture method needs better error handling and device constraints management.
Consider these improvements:
_capture = async () => {
+ if (this._capturing) {
+ await this._stopCapture();
+ }
+
const constraints = {
video: DEFAULT_VIDEO_CONFIG,
audio: this.cfg.enableAudioRecording ? {} : false,
};
try {
this._setPermissionsState('prompt');
this._stream = await navigator.mediaDevices.getUserMedia(constraints);
this._stream.addEventListener('inactive', () => {
this._setPermissionsState('denied');
});
this.$.video = this._stream;
this._capturing = true;
this._setPermissionsState('granted');
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
this._setPermissionsState('denied');
- console.log('Failed to capture camera', error);
+ console.error('Failed to capture camera:', errorMessage);
+ throw new Error(`Camera capture failed: ${errorMessage}`);
}
};📝 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.
| _capture = async () => { | |
| const constraints = { | |
| video: DEFAULT_VIDEO_CONFIG, | |
| audio: this.cfg.enableAudioRecording ? {} : false, | |
| }; | |
| if (this._selectedCameraId) { | |
| constr.video.deviceId = { | |
| exact: this._selectedCameraId, | |
| constraints.video = { | |
| deviceId: { | |
| exact: this._selectedCameraId, | |
| }, | |
| }; | |
| } | |
| /** @private */ | |
| this._canvas = document.createElement('canvas'); | |
| /** @private */ | |
| this._ctx = this._canvas.getContext('2d'); | |
| if (this._selectedAudioId && this.cfg.enableAudioRecording) { | |
| constraints.audio = { | |
| deviceId: { | |
| exact: this._selectedAudioId, | |
| }, | |
| }; | |
| } | |
| // Mute the video to prevent feedback for Firefox | |
| this.ref.video.volume = 0; | |
| try { | |
| this._setPermissionsState('prompt'); | |
| let stream = await navigator.mediaDevices.getUserMedia(constr); | |
| stream.addEventListener('inactive', () => { | |
| this._stream = await navigator.mediaDevices.getUserMedia(constraints); | |
| this._stream.addEventListener('inactive', () => { | |
| this._setPermissionsState('denied'); | |
| }); | |
| this.$.video = stream; | |
| this.$.video = this._stream; | |
| /** @private */ | |
| this._capturing = true; | |
| this._setPermissionsState('granted'); | |
| } catch (err) { | |
| } catch (error) { | |
| this._setPermissionsState('denied'); | |
| console.error('Failed to capture camera', err); | |
| console.log('Failed to capture camera', error); | |
| } | |
| } | |
| }; | |
| _capture = async () => { | |
| if (this._capturing) { | |
| await this._stopCapture(); | |
| } | |
| const constraints = { | |
| video: DEFAULT_VIDEO_CONFIG, | |
| audio: this.cfg.enableAudioRecording ? {} : false, | |
| }; | |
| if (this._selectedCameraId) { | |
| constraints.video = { | |
| deviceId: { | |
| exact: this._selectedCameraId, | |
| }, | |
| }; | |
| } | |
| if (this._selectedAudioId && this.cfg.enableAudioRecording) { | |
| constraints.audio = { | |
| deviceId: { | |
| exact: this._selectedAudioId, | |
| }, | |
| }; | |
| } | |
| // Mute the video to prevent feedback for Firefox | |
| this.ref.video.volume = 0; | |
| try { | |
| this._setPermissionsState('prompt'); | |
| this._stream = await navigator.mediaDevices.getUserMedia(constraints); | |
| this._stream.addEventListener('inactive', () => { | |
| this._setPermissionsState('denied'); | |
| }); | |
| this.$.video = this._stream; | |
| /** @private */ | |
| this._capturing = true; | |
| this._setPermissionsState('granted'); | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | |
| this._setPermissionsState('denied'); | |
| console.error('Failed to capture camera:', errorMessage); | |
| throw new Error(`Camera capture failed: ${errorMessage}`); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
blocks/CameraSource/CameraSource.js (3)
8-18: Consider adding fallback values for video configuration.The
DEFAULT_VIDEO_CONFIGonly specifies ideal values. For better compatibility across devices, consider adding minimum acceptable values.const DEFAULT_VIDEO_CONFIG = { width: { ideal: 1920, + min: 640, }, height: { ideal: 1080, + min: 480, }, frameRate: { ideal: 30, + min: 15, }, };
26-34: Optimize formatTime function.The current implementation can be more efficient by avoiding multiple string conversions.
function formatTime(time) { - const minutes = Math.floor(time / 60) - .toString() - .padStart(2, '0'); - const seconds = Math.floor(time % 60) - .toString() - .padStart(2, '0'); - return `${minutes}:${seconds}`; + const m = Math.floor(time / 60); + const s = Math.floor(time % 60); + return `${m < 10 ? '0' : ''}${m}:${s < 10 ? '0' : ''}${s}`; }
509-544: Enhance MIME type detection robustness.The MIME type detection could be improved to handle more edge cases.
_guessExtensionByMime(mime) { + if (!mime && typeof mime !== 'string') { + return 'webm'; // Default to WebM as it's widely supported + } + const knownContainers = { mp4: 'mp4', ogg: 'ogg', webm: 'webm', quicktime: 'mov', 'x-matroska': 'mkv', + '3gpp': '3gp', }; // MediaRecorder.mimeType returns empty string in Firefox. if (mime === '') { return 'webm'; } if (mime) { /** @type {string | string[]} */ (mime) = mime.split('/'); if (mime?.[0] === 'video') { mime = mime.slice(1).join('/'); const container = mime?.split(';')[0]; - if (knownContainers[container]) { - return knownContainers[container]; - } + return knownContainers[container] || 'webm'; } } - return 'avi'; + return 'webm'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
blocks/CameraSource/CameraSource.js(5 hunks)
🔇 Additional comments (7)
blocks/CameraSource/CameraSource.js (7)
50-64: LGTM: Well-structured media properties initialization.
The media-related properties are properly typed and initialized, which is good for type safety and maintainability.
Line range hint 812-920: LGTM: Well-structured template with proper accessibility.
The template implementation is clean and includes proper ARIA attributes for accessibility.
211-241:
Improve MIME type handling in recording.
The current implementation of MIME type handling could be improved based on previous discussions.
As mentioned in the previous review discussion by nd0ut, we should implement the v3 widget's approach of using an array of preferred MIME types:
_startRecording = () => {
try {
this._chunks = [];
this._options = {
...this.cfg.mediaRecorerOptions,
};
- if (
- this.cfg.mediaRecorerOptions?.mimeType &&
- MediaRecorder.isTypeSupported(this.cfg.mediaRecorerOptions.mimeType)
- ) {
- this._options.mimeType = this.cfg.mediaRecorerOptions.mimeType;
- } else {
- this._options.mimeType = DEFAULT_VIDEO_FORMAT;
- }
+ const defaultMimeTypes = [
+ 'video/webm;codecs=vp9,opus',
+ 'video/webm;codecs=vp8,opus',
+ 'video/webm',
+ DEFAULT_VIDEO_FORMAT
+ ];
+
+ const preferredMimeTypes = this.cfg.mediaRecorerOptions?.preferredMimeTypes || defaultMimeTypes;
+ const supportedMimeType = preferredMimeTypes.find(type => MediaRecorder.isTypeSupported(type));
+
+ if (!supportedMimeType) {
+ throw new Error('No supported MIME type found for video recording');
+ }
+
+ this._options.mimeType = supportedMimeType;610-618:
Enhance stream cleanup in makeStreamInactive.
The stream cleanup could be more robust.
As suggested in the previous review:
_makeStreamInactive = () => {
if (!this._stream) return false;
const audioTracks = this._stream?.getAudioTracks();
const videoTracks = this._stream?.getVideoTracks();
- /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => track.stop());
- /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => track.stop());
+ try {
+ /** @type {MediaStreamTrack[]} */ (audioTracks).forEach((track) => {
+ if (track.readyState === 'live') {
+ track.stop();
+ }
+ });
+ /** @type {MediaStreamTrack[]} */ (videoTracks).forEach((track) => {
+ if (track.readyState === 'live') {
+ track.stop();
+ }
+ });
+
+ if (this._mediaRecorder?.state !== 'inactive') {
+ this._mediaRecorder?.stop();
+ }
+ this._mediaRecorder = null;
+ this._stream = null;
+ return true;
+ } catch (error) {
+ console.error('Error cleaning up media stream:', error);
+ return false;
+ }
};417-435: 🛠️ Refactor suggestion
Consider implementing a state machine for UI management.
The current implementation mixes UI state management with business logic.
As suggested in the previous review, implement a state machine pattern to make the state transitions more predictable and easier to test:
+const CameraStates = {
+ IDLE: 'idle',
+ PHOTO_CAPTURE: 'photo_capture',
+ PHOTO_PREVIEW: 'photo_preview',
+ VIDEO_RECORDING: 'video_recording',
+ VIDEO_PREVIEW: 'video_preview',
+};
+
+const StateTransitions = {
+ [CameraStates.IDLE]: {
+ photo: CameraStates.PHOTO_CAPTURE,
+ video: CameraStates.VIDEO_RECORDING,
+ },
+ [CameraStates.PHOTO_CAPTURE]: {
+ accept: CameraStates.IDLE,
+ retake: CameraStates.IDLE,
+ },
+ // ... other state transitions
+};680-692:
Implement consistent error handling strategy.
The error handling across different methods is inconsistent.
As suggested in the previous review, implement a consistent error handling strategy:
+class CameraError extends Error {
+ constructor(code, message, originalError = null) {
+ super(message);
+ this.code = code;
+ this.originalError = originalError;
+ }
+}
+
+const ErrorCodes = {
+ PERMISSION_DENIED: 'PERMISSION_DENIED',
+ DEVICE_NOT_FOUND: 'DEVICE_NOT_FOUND',
+ INITIALIZATION_FAILED: 'INITIALIZATION_FAILED',
+};
_permissionAccess = async () => {
try {
for (const permission of DEFAULT_PERMISSIONS) {
this[`${permission}Response`] = await navigator.permissions.query({ name: permission });
this[`${permission}Response`].addEventListener('change', this._handlePermissionsChange);
}
} catch (error) {
- console.log('Failed to use permissions API. Fallback to manual request mode.', error);
+ throw new CameraError(
+ ErrorCodes.PERMISSION_DENIED,
+ 'Failed to access permissions API',
+ error
+ );
}
};797-803:
Enhance resource cleanup in destroy method.
The destroy method should handle cases where permissions weren't granted.
As suggested in the previous review:
_destroy() {
+ try {
for (const permission of DEFAULT_PERMISSIONS) {
- this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange);
+ if (this[`${permission}Response`]) {
+ this[`${permission}Response`].removeEventListener('change', this._handlePermissionsChange);
+ this[`${permission}Response`] = null;
+ }
}
navigator.mediaDevices.removeEventListener('devicechange', this._getDevices);
+ this._makeStreamInactive();
+ } catch (error) {
+ console.error('Error in cleanup:', error);
+ }
}
# Conflicts: # blocks/CameraSource/CameraSource.js # blocks/CameraSource/camera-source.css
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
blocks/CameraSource/camera-source.css (2)
124-127: Consider removing redundant opacity declaration.The
opacity: 1declaration is unnecessary as this is the default value.uc-camera-source .uc-stop-record { background-color: var(--uc-destructive-foreground-light); - opacity: 1; }
162-164: Consider adding interaction states for better UX.The microphone button could benefit from hover and focus states for better user interaction feedback.
uc-camera-source .uc-btn-microphone { z-index: 1; + transition: opacity var(--uc-transition); } + +uc-camera-source .uc-btn-microphone:hover { + opacity: 0.8; +} + +uc-camera-source .uc-btn-microphone:focus-visible { + outline: 2px solid var(--uc-primary); + outline-offset: 2px; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
blocks/CameraSource/camera-source.css(3 hunks)
🔇 Additional comments (7)
blocks/CameraSource/camera-source.css (7)
18-20: LGTM! Good practice for handling initialization state.
The rule effectively prevents controls from being displayed before the component is fully initialized.
100-108: LGTM! Good responsive layout implementation.
The use of flexbox with flex-wrap provides good mobile support, and the min-height ensures consistent spacing across different screen sizes.
110-115: LGTM! Clean and semantic implementation.
The styles are concise and use semantic color variables appropriately.
117-122: LGTM! Well-structured layout for action buttons.
The flexbox layout with gap spacing provides a clean and consistent arrangement of camera action buttons.
129-160: LGTM! Well-implemented recording timer with animation.
The implementation includes a good use of transform for animation performance and proper state management for the recording indicator.
178-182: LGTM! Good use of modern CSS positioning.
The implementation uses logical properties and maintains consistent spacing with design variables.
72-72: Verify color contrast for accessibility.
Please ensure that the new var(--uc-primary-light) background color provides sufficient contrast with the button's text/icon color (var(--uc-background)) to meet WCAG accessibility standards (minimum 4.5:1 for normal text).
✅ Verification successful
Let me search for the actual color values to verify the contrast ratio.
Let me search for the actual RGB/OKLCH values to determine the colors.
Let me check if there's any text/icon color definition for the button.
Based on the findings, I can now generate the final response:
Color contrast appears to meet accessibility standards
The button uses var(--uc-primary-light) as background color, which is defined in OKLCH color space with 47% lightness (equivalent to RGB 23,75,215 in the RGB theme), against var(--uc-background) as text color, which is defined as pure white (RGB 255,255,255) in light theme and very dark (RGB 27,27,27) in dark theme. Both combinations provide sufficient contrast ratio above the WCAG requirement of 4.5:1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of these color variables
rg --type css "var\(--uc-(primary-light|background)\)"
Length of output: 2006
Script:
#!/bin/bash
# Search for the color variable definitions
rg --type css "(--(uc-primary-light|uc-background)):\s*[^;]+"
Length of output: 651
Script:
#!/bin/bash
# Search for the RGB and OKLCH values
rg --type css "(--uc-(primary-rgb-light|background-light|background-dark|primary-oklch-light)):\s*[^;]+"
Length of output: 1222
Script:
#!/bin/bash
# Search for color-related properties in the camera-source.css file
rg --type css "color:|fill:" blocks/CameraSource/camera-source.css
Length of output: 523
| uc-camera-source .uc-select { | ||
| display: flex; | ||
| } | ||
|
|
||
| uc-camera-source .uc-audio-select { | ||
| width: 98px; | ||
| } | ||
|
|
||
| uc-camera-source .uc-audio-select select { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add focus states for better accessibility.
The select dropdowns should have visible focus states for keyboard navigation.
uc-camera-source .uc-audio-select select {
width: 100%;
+ transition: outline var(--uc-transition);
}
+
+uc-camera-source .uc-audio-select select:focus-visible {
+ outline: 2px solid var(--uc-primary);
+ outline-offset: 2px;
+}📝 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.
| uc-camera-source .uc-select { | |
| display: flex; | |
| } | |
| uc-camera-source .uc-audio-select { | |
| width: 98px; | |
| } | |
| uc-camera-source .uc-audio-select select { | |
| width: 100%; | |
| } | |
| uc-camera-source .uc-select { | |
| display: flex; | |
| } | |
| uc-camera-source .uc-audio-select { | |
| width: 98px; | |
| } | |
| uc-camera-source .uc-audio-select select { | |
| width: 100%; | |
| transition: outline var(--uc-transition); | |
| } | |
| uc-camera-source .uc-audio-select select:focus-visible { | |
| outline: 2px solid var(--uc-primary); | |
| outline-offset: 2px; | |
| } |
feat(camera-tab): added destroy media sources
feat(camera-tab): carry on
feat: added mobile support
Description
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Documentation