feat: add dual frame webcam layout preset#347
Open
shreyas-makes wants to merge 6 commits intosiddharthvaddem:mainfrom
Open
feat: add dual frame webcam layout preset#347shreyas-makes wants to merge 6 commits intosiddharthvaddem:mainfrom
shreyas-makes wants to merge 6 commits intosiddharthvaddem:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new "dual-frame" (split) webcam layout preset and wires support through layout computation, rendering (center-crop and screenBorderRadius), settings/editor behavior, project normalization/tests, and i18n translations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc71ca49b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+498
to
+500
| compositeLayout.screenBorderRadius != null | ||
| ? compositeLayout.screenBorderRadius * canvasScaleFactor | ||
| : compositeLayout.screenCover |
There was a problem hiding this comment.
In updateLayout, compositeLayout.screenBorderRadius is already computed by computeCompositeLayout in the current export canvas coordinate space, so multiplying it by canvasScaleFactor scales it a second time when export dimensions differ from preview dimensions. In dual-frame exports this makes the screen corner radius much larger than the webcam corner radius (which is not similarly scaled), so the two panes no longer match visually.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/video-editor/SettingsPanel.tsx (1)
660-670: Make the preset mapping explicit.These branches currently treat every non-
"picture-in-picture"/ non-"vertical-stack"preset as dual-frame. That works with today’s three presets, but a future addition would be silently mislabeled and filtered into the landscape bucket. A small allow-list plus label map would fail closed here.♻️ Suggested cleanup
+ const visiblePresets = new Set<WebcamLayoutPreset>( + isPortraitCanvas + ? ["picture-in-picture", "vertical-stack"] + : ["picture-in-picture", "two-timer"], + ); + const presetLabels: Record<WebcamLayoutPreset, string> = { + "picture-in-picture": t("layout.pictureInPicture"), + "vertical-stack": t("layout.verticalStack"), + "two-timer": t("layout.twoTimer"), + }; + - {WEBCAM_LAYOUT_PRESETS.filter((preset) => { - if (preset.value === "picture-in-picture") return true; - if (preset.value === "vertical-stack") return isPortraitCanvas; - return !isPortraitCanvas; - }).map((preset) => ( + {WEBCAM_LAYOUT_PRESETS.filter((preset) => + visiblePresets.has(preset.value), + ).map((preset) => ( <SelectItem key={preset.value} value={preset.value} className="text-xs"> - {preset.value === "picture-in-picture" - ? t("layout.pictureInPicture") - : preset.value === "vertical-stack" - ? t("layout.verticalStack") - : t("layout.twoTimer")} + {presetLabels[preset.value]} </SelectItem> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 660 - 670, Current filter/map over WEBCAM_LAYOUT_PRESETS treats any unknown preset as the “dual-frame” (landscape) case; change the code to explicitly allow-list known preset values and use a label map or switch for translations so unknown presets are excluded and not silently mis-labeled. Update the filter to only include preset.value values in an explicit set (e.g., "picture-in-picture", "vertical-stack", "two-timer") and replace the nested ternary in the SelectItem rendering with a small lookup (or switch) keyed by preset.value that returns the correct t(...) key for each known preset; ensure any preset.value not in the allow-list is skipped and not rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 354-359: Persisted webcamLayoutPreset is accepted unconditionally
and can rehydrate an invalid layout for portrait projects; first derive the
normalized aspectRatio from the saved editor (use the same
AspectRatio/ASPECT_RATIOS/isPortraitAspectRatio utilities) and then coerce
editor.webcamLayoutPreset: if the aspect ratio is portrait and
editor.webcamLayoutPreset === "two-timer" (or any preset unsupported for
portrait), replace it with DEFAULT_WEBCAM_LAYOUT_PRESET; update the
normalization logic around webcamLayoutPreset (referencing webcamLayoutPreset,
editor.webcamLayoutPreset, DEFAULT_WEBCAM_LAYOUT_PRESET) and add a negative
regression test next to the existing "two-timer" test to assert portrait files
get coerced to the default preset.
---
Nitpick comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 660-670: Current filter/map over WEBCAM_LAYOUT_PRESETS treats any
unknown preset as the “dual-frame” (landscape) case; change the code to
explicitly allow-list known preset values and use a label map or switch for
translations so unknown presets are excluded and not silently mis-labeled.
Update the filter to only include preset.value values in an explicit set (e.g.,
"picture-in-picture", "vertical-stack", "two-timer") and replace the nested
ternary in the SelectItem rendering with a small lookup (or switch) keyed by
preset.value that returns the correct t(...) key for each known preset; ensure
any preset.value not in the allow-list is skipped and not rendered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60e5416c-9c48-4077-a727-ee70c712ee09
📒 Files selected for processing (11)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/compositeLayout.test.tssrc/lib/compositeLayout.tssrc/lib/exporter/frameRenderer.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/video-editor/SettingsPanel.tsx (1)
660-671: Consider an explicit mapping for layout preset labels to improve maintainability.The nested ternary on lines 666-670 maps labels by process of elimination — if it's not
"picture-in-picture"and not"vertical-stack", it must be"dual-frame". While this works correctly for the current three presets, adding a fourth preset would require updating both the type definition and this ternary; the ternary change could easily be overlooked, silently producing incorrect labels.A more explicit approach would prevent this:
♻️ Suggested refactor
const PRESET_LABEL_KEYS: Record<WebcamLayoutPreset, string> = { "picture-in-picture": "layout.pictureInPicture", "vertical-stack": "layout.verticalStack", "dual-frame": "layout.dualFrame", };Then replace the nested ternary with:
{t(PRESET_LABEL_KEYS[preset.value])}This makes the mapping explicit and ensures TypeScript will require adding an entry for any new preset added to the type union.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 660 - 671, The nested ternary that chooses translation keys for WEBCAM_LAYOUT_PRESETS inside the SelectItem maps by exclusion and is fragile if new presets are added; replace that logic with an explicit mapping constant (e.g., PRESET_LABEL_KEYS: Record<WebcamLayoutPreset, string>) that maps each preset.value to its translation key, then use t(PRESET_LABEL_KEYS[preset.value]) in the SelectItem render (affecting the mapping where SelectItem and preset.value are used) so TypeScript will force updates when new presets are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1684-1688: normalizeProjectEditor currently preserves
webcamPosition even when webcamLayoutPreset is not "picture-in-picture", causing
inconsistency with runtime behavior; update normalizeProjectEditor to set
webcamPosition to null whenever the normalized webcamLayoutPreset !==
"picture-in-picture". Locate the normalizeProjectEditor function and after you
determine/normalize webcamLayoutPreset, add logic to validate webcamPosition and
assign null unless webcamLayoutPreset === "picture-in-picture" (preserving the
existing position only for PiP). Ensure you reference the same property names
(webcamLayoutPreset, webcamPosition) and return the normalized editor object
with webcamPosition adjusted accordingly.
---
Nitpick comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 660-671: The nested ternary that chooses translation keys for
WEBCAM_LAYOUT_PRESETS inside the SelectItem maps by exclusion and is fragile if
new presets are added; replace that logic with an explicit mapping constant
(e.g., PRESET_LABEL_KEYS: Record<WebcamLayoutPreset, string>) that maps each
preset.value to its translation key, then use t(PRESET_LABEL_KEYS[preset.value])
in the SelectItem render (affecting the mapping where SelectItem and
preset.value are used) so TypeScript will force updates when new presets are
introduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a92e207d-e51d-444f-ba3a-999c8e42c149
📒 Files selected for processing (10)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/compositeLayout.test.tssrc/lib/compositeLayout.tssrc/lib/exporter/frameRenderer.ts
✅ Files skipped from review due to trivial changes (3)
- src/i18n/locales/zh-CN/settings.json
- src/i18n/locales/es/settings.json
- src/i18n/locales/en/settings.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/video-editor/projectPersistence.ts
- src/lib/compositeLayout.test.ts
- src/lib/exporter/frameRenderer.ts
- src/components/video-editor/projectPersistence.test.ts
- src/lib/compositeLayout.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/projectPersistence.ts (1)
181-192: Consider refactoring the nested ternary for readability.The logic is correct, but the 5-level nested ternary is difficult to follow. A
switchstatement or helper function would improve maintainability.♻️ Suggested refactor using switch
- const normalizedWebcamLayoutPreset: WebcamLayoutPreset = - editor.webcamLayoutPreset === "picture-in-picture" - ? editor.webcamLayoutPreset - : editor.webcamLayoutPreset === "vertical-stack" - ? isPortraitAspectRatio(normalizedAspectRatio) - ? editor.webcamLayoutPreset - : DEFAULT_WEBCAM_LAYOUT_PRESET - : editor.webcamLayoutPreset === "dual-frame" - ? isPortraitAspectRatio(normalizedAspectRatio) - ? DEFAULT_WEBCAM_LAYOUT_PRESET - : editor.webcamLayoutPreset - : DEFAULT_WEBCAM_LAYOUT_PRESET; + const isPortrait = isPortraitAspectRatio(normalizedAspectRatio); + let normalizedWebcamLayoutPreset: WebcamLayoutPreset; + switch (editor.webcamLayoutPreset) { + case "picture-in-picture": + normalizedWebcamLayoutPreset = "picture-in-picture"; + break; + case "vertical-stack": + normalizedWebcamLayoutPreset = isPortrait ? "vertical-stack" : DEFAULT_WEBCAM_LAYOUT_PRESET; + break; + case "dual-frame": + normalizedWebcamLayoutPreset = isPortrait ? DEFAULT_WEBCAM_LAYOUT_PRESET : "dual-frame"; + break; + default: + normalizedWebcamLayoutPreset = DEFAULT_WEBCAM_LAYOUT_PRESET; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 181 - 192, The nested 5-level ternary computing normalizedWebcamLayoutPreset is hard to read—replace it with a small helper function (e.g., computeNormalizedWebcamLayoutPreset) or a switch over editor.webcamLayoutPreset that implements the same rules: if "picture-in-picture" return editor.webcamLayoutPreset; if "vertical-stack" return editor.webcamLayoutPreset only when isPortraitAspectRatio(normalizedAspectRatio) else DEFAULT_WEBCAM_LAYOUT_PRESET; if "dual-frame" return DEFAULT_WEBCAM_LAYOUT_PRESET when isPortraitAspectRatio(normalizedAspectRatio) else editor.webcamLayoutPreset; default to DEFAULT_WEBCAM_LAYOUT_PRESET; call this helper to set normalizedWebcamLayoutPreset for clarity and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 181-192: The nested 5-level ternary computing
normalizedWebcamLayoutPreset is hard to read—replace it with a small helper
function (e.g., computeNormalizedWebcamLayoutPreset) or a switch over
editor.webcamLayoutPreset that implements the same rules: if
"picture-in-picture" return editor.webcamLayoutPreset; if "vertical-stack"
return editor.webcamLayoutPreset only when
isPortraitAspectRatio(normalizedAspectRatio) else DEFAULT_WEBCAM_LAYOUT_PRESET;
if "dual-frame" return DEFAULT_WEBCAM_LAYOUT_PRESET when
isPortraitAspectRatio(normalizedAspectRatio) else editor.webcamLayoutPreset;
default to DEFAULT_WEBCAM_LAYOUT_PRESET; call this helper to set
normalizedWebcamLayoutPreset for clarity and maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 300476a2-1428-4223-b60b-267e28bc9c9e
📒 Files selected for processing (2)
src/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/projectPersistence.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In Openscreen, we only have one preset called the "Picture in picture" mode. I was looking at a way in which there could be other presets too. A preset I was thinking of adding was a way in which the camera view could also be scaled and zoomed out to give equal representation for both the Camera and the Screenshare. This is the PR to enable this.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests