feat(camera-tab): added aspectRatio #762
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
blocks/Config/initialConfig.js (1)
75-75: Add JSDoc documentation for the aspectRatio propertyPlease add JSDoc comments to document the expected format and valid values for the
aspectRatioproperty. This will help developers understand how to properly configure this new feature.Example documentation:
+ /** @type {Record<string, string> | null} Defines aspect ratio presets for camera capture. Example: { "16:9": "1.77", "4:3": "1.33" } */ aspectRatio: null,blocks/Config/Config.js (1)
Line range hint
1-1: Consider adding tests and documentation for the aspectRatio featureWhile the implementation is technically sound, the feature would benefit from:
- Unit tests for the new configuration and validation
- Documentation in README or relevant docs explaining:
- Purpose and use cases of the aspect ratio feature
- Expected format and valid values
- Example configurations
Would you like me to help generate:
- Unit tests for the aspectRatio configuration?
- Documentation template for the feature?
types/exported.d.ts (1)
79-82: Enhance the JSDoc documentation for aspectRatio.The current documentation is minimal. Consider adding more details about:
- Expected format of the Record (e.g., "16:9", "4:3")
- Example usage
- Default behavior when null
/** - * The aspect ratio of the image/video to be cropped + * The aspect ratio configuration for image/video capture + * @example + * { + * "16:9": "1.77777778", + * "4:3": "1.33333333" + * } + * @default null - Uses the default aspect ratio of the device */blocks/CameraSource/CameraSource.js (1)
642-643: Consider merging deviceId with existing constraints.The current implementation might override other video constraints when setting deviceId.
-constraints.video.deviceId = { - exact: this._selectedCameraId, -}; +constraints.video = { + ...constraints.video, + deviceId: { + exact: this._selectedCameraId, + }, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
blocks/CameraSource/CameraSource.js(1 hunks)blocks/Config/Config.js(2 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(1 hunks)types/exported.d.ts(1 hunks)
🔇 Additional comments (2)
blocks/Config/Config.js (1)
25-25: LGTM! Correctly configured as a complex key
The addition of aspectRatio to complexConfigKeys is appropriate as it handles object values that cannot be passed as attributes.
Also applies to: 37-37
blocks/CameraSource/CameraSource.js (1)
635-637: 🛠️ Refactor suggestion
Add error handling for unsupported aspect ratios.
The code should handle cases where the device doesn't support the specified aspect ratio.
Consider adding error handling:
video: {
- aspectRatio: this.cfg.aspectRatio,
+ ...this._getSupportedVideoConstraints(),
},
+_getSupportedVideoConstraints() {
+ try {
+ if (!this.cfg.aspectRatio) return DEFAULT_VIDEO_CONFIG;
+ return { aspectRatio: this.cfg.aspectRatio };
+ } catch (error) {
+ console.warn('Aspect ratio not supported:', error);
+ return DEFAULT_VIDEO_CONFIG;
+ }
+}| @@ -171,6 +171,7 @@ const mapping = { | |||
| mediaRecorerOptions: asObject, | |||
|
|
|||
| maxVideoRecordingDuration: asNumber, | |||
| aspectRatio: asObject, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance validation for aspectRatio values
The current validation only ensures the value is an object. Consider adding specific validation for aspect ratio format to prevent runtime issues with camera initialization.
Example implementation:
- aspectRatio: asObject,
+ aspectRatio: (value) => {
+ const obj = asObject(value);
+ if (obj === null) return null;
+
+ for (const [key, ratio] of Object.entries(obj)) {
+ if (!/^\d+:\d+$/.test(key) || !/^\d+(\.\d+)?$/.test(ratio)) {
+ throw new Error(
+ `Invalid aspect ratio format. Expected format: { "width:height": "decimal" }, got: {"${key}": "${ratio}"}`
+ );
+ }
+ }
+ return obj;
+ },📝 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.
| aspectRatio: asObject, | |
| aspectRatio: (value) => { | |
| const obj = asObject(value); | |
| if (obj === null) return null; | |
| for (const [key, ratio] of Object.entries(obj)) { | |
| if (!/^\d+:\d+$/.test(key) || !/^\d+(\.\d+)?$/.test(ratio)) { | |
| throw new Error( | |
| `Invalid aspect ratio format. Expected format: { "width:height": "decimal" }, got: {"${key}": "${ratio}"}` | |
| ); | |
| } | |
| } | |
| return obj; | |
| }, |
| video: { | ||
| aspectRatio: this.cfg.aspectRatio, | ||
| }, |
There was a problem hiding this comment.
Validate aspectRatio before using in constraints.
The aspectRatio is directly used from config without validation. This could lead to invalid constraints if the config contains incorrect values.
Consider adding validation:
video: {
- aspectRatio: this.cfg.aspectRatio,
+ ...(this.cfg.aspectRatio && Object.keys(this.cfg.aspectRatio).length > 0
+ ? { aspectRatio: this.cfg.aspectRatio }
+ : DEFAULT_VIDEO_CONFIG),
},Committable suggestion skipped: line range outside the PR's diff.
| @@ -632,15 +632,15 @@ export class CameraSource extends UploaderBlock { | |||
|
|
|||
| _capture = async () => { | |||
| const constraints = { | |||
| video: DEFAULT_VIDEO_CONFIG, | |||
| video: { | |||
There was a problem hiding this comment.
Looks like DEFAULT_VIDEO_CONFIG was forgotten here
| @@ -632,15 +632,15 @@ export class CameraSource extends UploaderBlock { | |||
|
|
|||
| _capture = async () => { | |||
| const constraints = { | |||
| video: DEFAULT_VIDEO_CONFIG, | |||
| video: { | |||
| aspectRatio: this.cfg.aspectRatio, | |||
There was a problem hiding this comment.
What browser will do if this type of constraint is not supported?
| @@ -33,6 +34,7 @@ export const complexConfigKeys = [ | |||
| 'fileValidators', | |||
| 'collectionValidators', | |||
| 'mediaRecorerOptions', | |||
| 'aspectRatio', | |||
There was a problem hiding this comment.
It looks too common config property name for me. Maybe something like mediaRecorderAspectRatio?
Description
Checklist
Summary by CodeRabbit
New Features
aspectRatio, enhancing video capture settings.Bug Fixes
Documentation
aspectRatio, providing clarity on its usage.