feat: add cameraModes option, deprecate enableVideoRecording and defaultCameraMode options
#781
Conversation
WalkthroughThis pull request introduces significant changes to the camera and configuration handling in the application. The modifications focus on enhancing the flexibility of camera source types, improving configuration management, and introducing more dynamic handling of camera modes. Key changes include updating type definitions, introducing new constants for camera sources, and refactoring how camera modes and video recording are configured and managed across multiple components. Changes
Sequence DiagramsequenceDiagram
participant User
participant UploaderPublicApi
participant CameraSource
participant Config
User->>Config: Set camera configuration
Config->>Config: Run assertions
Config->>Config: Apply side effects
Config->>UploaderPublicApi: Update file input settings
UploaderPublicApi->>CameraSource: Configure camera modes
CameraSource-->>User: Update camera interface
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
nd0ut
force-pushed
the
feat/camera-options-split
branch
from
ff5227e to
605c829
Compare
nd0ut
changed the title
cameraModes optioncameraModes option, deprecate enableVideoRecording and defaultCameraMode options
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
blocks/CameraSource/constants.js (2)
1-4: LGTM! Well-structured camera source types.Good use of
Object.freeze()to ensure immutability of the constants.Consider adding JSDoc comments to document the purpose and usage of these constants:
+/** + * Defines the available camera source types. + * @readonly + * @enum {string} + */ export const CameraSourceTypes = Object.freeze({ PHOTO: 'photo', VIDEO: 'video', });
6-17: LGTM! Well-organized camera events.Good practices observed:
- Logical grouping of related events with empty lines
- Consistent event naming pattern
- Use of
Object.freeze()for immutabilityConsider adding JSDoc comments to document the purpose and usage of these events:
+/** + * Defines all possible camera source events. + * @readonly + * @enum {string} + */ export const CameraSourceEvents = Object.freeze({ // ... events });blocks/utils/comma-separated.js (1)
4-4: LGTM! Good improvements to CSV handling.Improvements:
- Fixed typo in function name
- Added filtering of falsy values for more robust handling
- Maintained type safety with JSDoc annotations
Consider enhancing the JSDoc comments to be more descriptive:
-/** @param {string} value */ +/** + * Deserializes a comma-separated string into an array, removing empty values. + * @param {string} value - The comma-separated string to deserialize + * @returns {string[]} Array of non-empty trimmed values + */Also applies to: 9-12
blocks/Config/side-effects.js (2)
13-21: Add validation for camera modes.Consider validating that the modes match the
CameraSourceTypesto prevent invalid modes.if (key === 'enableVideoRecording' && value !== null) { let cameraModes = deserializeCsv(getValue('cameraModes')); + // Validate that all modes are valid + if (!cameraModes.every(mode => Object.values(CameraSourceTypes).includes(mode))) { + throw new Error('Invalid camera mode detected'); + } if (value && !cameraModes.includes('video')) { cameraModes = cameraModes.concat('video'); } else if (!value) { cameraModes = cameraModes.filter((mode) => mode !== 'video'); } setValue('cameraModes', serializeCsv(cameraModes)); }
23-31: Simplify the sort function.The current sort implementation can be simplified.
if (key === 'defaultCameraMode' && value !== null) { let cameraModes = deserializeCsv(getValue('cameraModes')); - cameraModes = cameraModes.sort((a, b) => { - if (a === value) return -1; - if (b === value) return 1; - return 0; - }); + // Move default mode to the front if it exists + const index = cameraModes.indexOf(value); + if (index > 0) { + cameraModes.unshift(cameraModes.splice(index, 1)[0]); + } setValue('cameraModes', serializeCsv(cameraModes)); }blocks/Config/assertions.js (2)
4-24: LGTM! Clear and maintainable assertions.Good practices:
- Clear warning messages
- Well-structured assertions array
- Proper handling of deprecated features
Consider adding an assertion for invalid camera modes:
const ASSERTIONS = [ // ... existing assertions + { + test: (cfg) => { + if (!cfg.cameraModes) return false; + const modes = deserializeCsv(cfg.cameraModes); + return !modes.every(mode => + Object.values(CameraSourceTypes).includes(mode) + ); + }, + message: + 'Invalid camera mode detected in `cameraModes`.\n' + + 'Valid modes are: ' + Object.values(CameraSourceTypes).join(', '), + }, ];
26-37: Consider adjusting the debounce delay.A zero delay might still cause performance issues if assertions are run very frequently.
export const runAssertions = debounce( /** @param {import('../../types').ConfigType} cfg */ (cfg) => { for (const { test, message } of ASSERTIONS) { if (test(cfg)) { warnOnce(message); } } }, - 0, + 100, // Add a small delay to prevent excessive calls );types/exported.d.ts (2)
251-257: Consider removing deprecated option in next major version.The
defaultCameraModeis properly marked as deprecated in favor ofcameraModes.Consider removing this option in the next major version to maintain clean API surface.
265-268: Consider removing deprecated option in next major version.The
enableVideoRecordingis properly marked as deprecated in favor ofcameraModes.Consider removing this option in the next major version to maintain clean API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
abstract/UploaderPublicApi.js(2 hunks)blocks/CameraSource/CameraSource.js(23 hunks)blocks/CameraSource/camera-source.css(1 hunks)blocks/CameraSource/constants.js(1 hunks)blocks/CloudImageEditor/src/lib/parseTabs.js(1 hunks)blocks/Config/Config.js(5 hunks)blocks/Config/assertions.js(1 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(3 hunks)blocks/Config/side-effects.js(1 hunks)blocks/utils/comma-separated.js(1 hunks)package.json(2 hunks)types/exported.d.ts(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocks/CloudImageEditor/src/lib/parseTabs.js
🔇 Additional comments (22)
blocks/Config/side-effects.js (1)
3-11: LGTM! Well-typed side effects handler.Good use of TypeScript templates for type safety and comprehensive JSDoc documentation.
blocks/Config/initialConfig.js (2)
71-72: LGTM! Improved camera mode configuration.The introduction of
cameraModesprovides a more flexible way to configure available camera modes, while settingdefaultCameraModeto null maintains backward compatibility during the transition.
74-74: LGTM! Aligned video recording configuration.
- Setting
enableVideoRecordingto null aligns with the newcameraModesapproach- Fixed typo in
mediaRecorderOptionsproperty nameAlso applies to: 76-76
blocks/Config/normalizeConfigValue.js (4)
3-4: LGTM! Added required imports.The new imports support the enhanced camera mode configuration functionality.
42-48: LGTM! Improved camera mode validation.The function now validates against
CameraSourceTypesenum, providing better type safety and maintainability.
50-57: LGTM! Added CSV validation for camera modes.The new
asCameraModesfunction properly validates each mode in the CSV string againstCameraSourceTypes.
181-185: LGTM! Updated configuration mapping.
- Added
cameraModeswith CSV validation- Updated
defaultCameraModeto use the improved validation- Fixed typo in
mediaRecorderOptionsblocks/Config/Config.js (4)
7-8: LGTM! Added validation imports.Added imports for configuration validation and side effects management.
141-148: LGTM! Enhanced configuration validation.Added proper validation and side effects handling when setting configuration values.
158-158: LGTM! Improved value retrieval.Added fallback to shared configuration when local property is undefined.
26-26: LGTM! Fixed property name typo.Corrected the spelling of
mediaRecorderOptionsin type definition and array.Also applies to: 37-37
abstract/UploaderPublicApi.js (2)
7-7: LGTM! Added required imports.Added imports to support the enhanced camera modes functionality.
Also applies to: 12-12
158-159: LGTM! Improved file input configuration.The file input's accept attribute is now dynamically set based on enabled camera modes, providing better flexibility and maintainability.
types/exported.d.ts (3)
4-4: LGTM: Clean import of CameraTabId type.The import is correctly placed and properly scoped.
244-249: LGTM: Well-documented cameraModes configuration option.The new
cameraModesoption is properly documented with:
- Clear description of functionality
- Default value specification
- Proper typing as string
297-312: LGTM: Clean error type definitions.The error types are well-structured with:
- Clear separation between custom and predefined errors
- Proper type composition
blocks/CameraSource/CameraSource.js (4)
4-4: LGTM: Clean imports.The imports are properly organized and scoped:
- Utility function for string handling
- Constants for camera source types and events
Also applies to: 8-8
562-565: LGTM: Clean implementation of cameraModes getter.The private getter properly uses the utility function to parse the comma-separated string.
779-788: LGTM: Well-structured camera modes handler.The
_handleCameraModesmethod:
- Properly updates UI visibility based on available modes
- Handles default tab selection
- Maintains current tab if valid
806-809: LGTM: Clean config subscription.The subscription to cameraModes changes:
- Only triggers for active camera
- Properly delegates to handler method
blocks/CameraSource/camera-source.css (1)
112-112: LGTM: Clean CSS enhancement.The gap property properly uses the design system variable for consistent spacing.
package.json (1)
102-102: Verify compatibility with updated dependencies.The TypeScript-related dependencies have been updated:
- @total-typescript/ts-reset: ^0.5.1 → ^0.6.1
- typescript: ^5.2.2 → ^5.7.3
Also applies to: 133-133
✅ Verification successful
Dependencies update is safe to proceed
The updates to both packages are compatible:
- TypeScript 5.7.3: Contains no breaking changes
- @total-typescript/ts-reset 0.6.1: Adds new type features while maintaining backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in the updated dependencies # Check typescript changelog gh api graphql -f query=' { repository(owner: "microsoft", name: "TypeScript") { releases(first: 10) { nodes { tagName description } } } }' | jq '.data.repository.releases.nodes[] | select(.tagName | contains("5.7"))' # Check ts-reset changelog gh api graphql -f query=' { repository(owner: "total-typescript", name: "ts-reset") { releases(first: 5) { nodes { tagName description } } } }' | jq '.data.repository.releases.nodes[] | select(.tagName | contains("0.6"))'Length of output: 3409
Script:
#!/bin/bash # Check ts-reset changelog and commits for breaking changes fd "CHANGELOG.md" | xargs rg -i "0\.6\.0|0\.6\.1" -A 5 -B 5 # Fetch package.json to check if there are any peer dependency changes curl -s https://raw.githubusercontent.com/total-typescript/ts-reset/v0.6.1/package.json | jq '.peerDependencies' curl -s https://raw.githubusercontent.com/total-typescript/ts-reset/v0.5.1/package.json | jq '.peerDependencies'Length of output: 1628
Script:
#!/bin/bash # Query npm registry for package details curl -s https://registry.npmjs.org/@total-typescript/ts-reset | jq '.versions["0.6.1"], .versions["0.5.1"]' # Check for any TypeScript-specific files that might be using ts-reset rg -l "@total-typescript/ts-reset"Length of output: 9786
nd0ut
force-pushed
the
feat/camera-options-split
branch
from
605c829 to
1d41325
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
blocks/CameraSource/CameraSource.js (3)
780-789: Enhance error handling in_handleCameraModes.The camera modes handling looks good, but consider:
- Validating the input array is not empty
- Adding error handling for invalid mode values
/** @param {CameraMode[]} cameraModes */ _handleCameraModes = (cameraModes) => { + if (!Array.isArray(cameraModes) || cameraModes.length === 0) { + console.warn('Invalid camera modes configuration'); + cameraModes = [CameraSourceTypes.PHOTO]; + } + + const validModes = cameraModes.filter(mode => + Object.values(CameraSourceTypes).includes(mode) + ); + + if (validModes.length === 0) { + console.warn('No valid camera modes found'); + validModes.push(CameraSourceTypes.PHOTO); + } + - this.$.tabVideoHidden = !cameraModes.includes(CameraSourceTypes.VIDEO); - this.$.tabCameraHidden = !cameraModes.includes(CameraSourceTypes.PHOTO); + this.$.tabVideoHidden = !validModes.includes(CameraSourceTypes.VIDEO); + this.$.tabCameraHidden = !validModes.includes(CameraSourceTypes.PHOTO); - const defaultTab = cameraModes[0]; + const defaultTab = validModes[0]; if (!this._activeTab || !cameraModes.includes(this._activeTab)) { this._handleActiveTab(defaultTab); } };
807-810: Consider debouncing the camera modes configuration handler.The configuration subscription looks good, but since it triggers UI updates, consider debouncing the handler to prevent rapid re-renders if the configuration changes frequently.
this.subConfigValue('cameraModes', (val) => { if (!this.isActivityActive) return; const cameraModes = deserializeCsv(val); - this._handleCameraModes(cameraModes); + debounce(() => this._handleCameraModes(cameraModes), 100)(); });
Line range hint
722-752: Consider adding retry logic for device enumeration.The device enumeration looks good, but consider:
- Adding retry logic for transient failures
- Handling cases where device labels are empty (privacy mode)
_getDevices = async () => { + const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; + + const retry = async (fn, retries) => { + try { + return await fn(); + } catch (error) { + if (retries > 0) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return retry(fn, retries - 1); + } + throw error; + } + }; + try { - const devices = await navigator.mediaDevices.enumerateDevices(); + const devices = await retry(() => + navigator.mediaDevices.enumerateDevices(), + MAX_RETRIES + ); this._cameraDevices = devices .filter((device) => device.kind === 'videoinput') .map((device, index) => ({ - text: device.label.trim() || `${this.l10n('caption-camera')} ${index + 1}`, + text: device.label?.trim() || `${this.l10n('caption-camera')} ${index + 1}`, value: device.deviceId, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
abstract/UploaderPublicApi.js(2 hunks)blocks/CameraSource/CameraSource.js(24 hunks)blocks/CameraSource/camera-source.css(1 hunks)blocks/CameraSource/constants.js(1 hunks)blocks/CloudImageEditor/src/lib/parseTabs.js(1 hunks)blocks/Config/Config.js(5 hunks)blocks/Config/assertions.js(1 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(3 hunks)blocks/Config/side-effects.js(1 hunks)blocks/utils/comma-separated.js(1 hunks)types/exported.d.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- blocks/utils/comma-separated.js
- blocks/CameraSource/camera-source.css
- blocks/CameraSource/constants.js
- blocks/CloudImageEditor/src/lib/parseTabs.js
- blocks/Config/side-effects.js
- blocks/Config/initialConfig.js
- blocks/Config/normalizeConfigValue.js
- types/exported.d.ts
- blocks/Config/assertions.js
- blocks/Config/Config.js
🔇 Additional comments (5)
abstract/UploaderPublicApi.js (2)
7-7: LGTM! Import statements are correctly updated.The new imports support the camera modes functionality:
deserializeCsvfor parsing the camera modes configurationCameraSourceTypesfor type-safe camera mode checksAlso applies to: 12-12
158-159: Consider handling edge cases in accept attribute generation.The dynamic accept attribute generation looks good, but there are a few considerations:
- What happens if
cameraModesis undefined or empty?- Should we validate the parsed modes against valid
CameraSourceTypes?Let's verify the handling of these edge cases:
✅ Verification successful
Edge cases are properly handled in the codebase
The codebase includes robust validation through the
asCameraModesnormalizer which ensures:
- Invalid modes throw errors
- Empty/undefined values are safely deserialized
- Only valid
CameraSourceTypesare accepted🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for validation of cameraModes in the codebase # Test 1: Search for cameraModes validation rg -A 5 "cameraModes.*validation|asCameraModes" # Test 2: Search for default values or initialization rg -A 5 "cameraModes.*=|cameraModes.*default"Length of output: 3134
blocks/CameraSource/CameraSource.js (3)
4-9: LGTM! Type definitions and imports are well organized.Good job on:
- Importing utility functions for string handling
- Defining clear type for
CameraMode- Using constants from a dedicated file
Also applies to: 42-42
563-566: LGTM! Clean implementation of camera modes getter.The
_cameraModesgetter provides a clean way to access parsed camera modes configuration.
218-221: Verify media recorder options handling.The media recorder options handling looks good, but let's verify the configuration structure:
✅ Verification successful
Media recorder options handling is properly implemented
The configuration structure and handling are well-implemented with proper type safety, null checks, and format validation. The spread operation is used correctly with appropriate fallbacks for unsupported mime types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check media recorder options configuration # Test: Search for mediaRecorderOptions configuration and validation rg "mediaRecorderOptions.*=|mediaRecorderOptions.*validation"Length of output: 63
Script:
#!/bin/bash # Broader search for media recorder related code and configuration echo "=== Media Recorder Usage ===" rg -i "mediarecorder" -A 3 -B 3 echo -e "\n=== Configuration Structure ===" fd -e js -e ts -e json "config|types?" --exec rg -l "mediaRecorder" echo -e "\n=== Type Definitions ===" ast-grep --pattern 'interface $_ { $$$ mediaRecorder $$$ }'Length of output: 7793
nd0ut
force-pushed
the
feat/camera-options-split
branch
from
4baa420 to
643f37b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocks/CameraSource/CameraSource.js (1)
Line range hint
320-332: Improve error handling in camera state transitions.The camera state management logic could benefit from additional error handling to gracefully handle edge cases.
Consider adding try-catch blocks and state validation:
_setCameraState = (status) => { + try { if ( this._activeTab === CameraSourceTypes.PHOTO && (status === 'shot' || status === 'retake' || status === 'accept') ) { + if (!this._stream) { + throw new Error('Camera stream not available'); + } this._handlePhoto(status); } + } catch (error) { + console.error('Failed to set camera state:', error); + this._setPermissionsState('denied'); + } };Also applies to: 355-371, 378-413
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
abstract/UploaderPublicApi.js(2 hunks)blocks/CameraSource/CameraSource.js(24 hunks)blocks/CameraSource/camera-source.css(1 hunks)blocks/CameraSource/constants.js(1 hunks)blocks/CloudImageEditor/src/lib/parseTabs.js(1 hunks)blocks/Config/Config.js(5 hunks)blocks/Config/assertions.js(1 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(3 hunks)blocks/Config/side-effects.js(1 hunks)blocks/utils/comma-separated.js(1 hunks)types/exported.d.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- blocks/CameraSource/camera-source.css
- blocks/utils/comma-separated.js
- blocks/CloudImageEditor/src/lib/parseTabs.js
- blocks/CameraSource/constants.js
- blocks/Config/assertions.js
- blocks/Config/initialConfig.js
- blocks/Config/side-effects.js
- blocks/Config/Config.js
- blocks/Config/normalizeConfigValue.js
- types/exported.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (6)
abstract/UploaderPublicApi.js (2)
7-7: LGTM! Import changes align with new camera modes functionality.The addition of
deserializeCsvandCameraSourceTypesimports supports the new camera modes implementation.Also applies to: 12-12
158-159: Verify the migration fromenableVideoRecordingtocameraModes.The code correctly uses
cameraModesto determine video recording capability. However, since this is a breaking change that deprecatesenableVideoRecording, we should ensure backward compatibility.Run the following script to check for existing usages of the deprecated option:
✅ Verification successful
Migration from
enableVideoRecordingtocameraModesis properly handled ✅The codebase includes comprehensive backward compatibility:
- The deprecated option is properly marked with warnings
- Side effects ensure both parameters stay in sync
- Default values are non-breaking
- Clear migration path is documented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of deprecated enableVideoRecording option echo "Searching for enableVideoRecording usage in JavaScript/TypeScript files..." rg -l "enableVideoRecording" --type js --type ts echo "Searching for enableVideoRecording in configuration files..." rg -l "enableVideoRecording" --type json --type yamlLength of output: 538
Script:
#!/bin/bash # Check type definitions and config handling echo "Checking type definitions..." cat types/exported.d.ts | grep -A 5 -B 5 "enableVideoRecording" echo -e "\nChecking config normalization..." cat blocks/Config/normalizeConfigValue.js | grep -A 10 -B 10 "enableVideoRecording" echo -e "\nChecking config assertions..." cat blocks/Config/assertions.js | grep -A 10 -B 10 "enableVideoRecording" echo -e "\nChecking initial config..." cat blocks/Config/initialConfig.js | grep -A 5 -B 5 "enableVideoRecording" echo -e "\nChecking side effects..." cat blocks/Config/side-effects.js | grep -A 10 -B 10 "enableVideoRecording"Length of output: 3687
blocks/CameraSource/CameraSource.js (4)
4-9: LGTM! Type definitions and imports are well-structured.The code correctly defines the
CameraModetype and imports necessary utilities.Also applies to: 42-42
563-566: LGTM! Well-implemented camera modes handling.The
_cameraModesgetter and_handleCameraModesmethod provide a clean implementation for managing available camera modes. The code correctly:
- Parses camera modes from configuration
- Updates UI visibility based on available modes
- Sets default tab when necessary
Also applies to: 780-789
807-810: LGTM! Configuration subscription handles camera modes changes.The code correctly subscribes to
cameraModeschanges and updates the UI accordingly.
Line range hint
722-752: LGTM! Improved variable naming and encapsulation.The renaming of variables to use the
_prefix properly indicates their private nature:
cameraDevices→_cameraDevicesaudioDevices→_audioDevices
Description
Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores