feat: add dual frame webcam layout preset#347
Open
shreyas-makes wants to merge 5 commits intosiddharthvaddem:mainfrom
Open
feat: add dual frame webcam layout preset#347shreyas-makes wants to merge 5 commits intosiddharthvaddem:mainfrom
shreyas-makes wants to merge 5 commits intosiddharthvaddem:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new "dual-frame" (split) webcam layout preset and propagates support across layout computation, rendering (center-crop and screenBorderRadius), settings/preset filtering and retention rules, project persistence/normalization, unit tests, and i18n translations. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsPanel
participant VideoEditor
participant CompositeLayout
participant FrameRenderer
participant ProjectPersistence
User->>SettingsPanel: Select webcam layout ("dual-frame")
SettingsPanel->>VideoEditor: onWebcamLayoutPresetChange("dual-frame")
VideoEditor->>VideoEditor: Update editor state
VideoEditor->>CompositeLayout: computeCompositeLayout(preset="dual-frame", aspectRatio)
CompositeLayout->>CompositeLayout: compute centered content, gap, split widths, screenBorderRadius, webcamRect
CompositeLayout-->>VideoEditor: Return WebcamCompositeLayout
VideoEditor->>FrameRenderer: updateLayout(compositeLayout)
FrameRenderer->>FrameRenderer: compute source crop (center-crop) for webcam
FrameRenderer->>FrameRenderer: draw screen + cropped webcam honoring screenBorderRadius
VideoEditor->>ProjectPersistence: persist normalized editor state
ProjectPersistence-->>VideoEditor: return normalized/saved state
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Comment on lines
1684
to
1688
| onWebcamLayoutPresetChange={(preset) => | ||
| pushState({ | ||
| webcamLayoutPreset: preset, | ||
| webcamPosition: preset === "vertical-stack" ? null : webcamPosition, | ||
| webcamPosition: preset === "picture-in-picture" ? webcamPosition : null, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if projectPersistence.ts normalizes webcamPosition based on preset
rg -n -A5 'webcamPosition' src/components/video-editor/projectPersistence.ts | head -50Repository: siddharthvaddem/openscreen
Length of output: 848
🏁 Script executed:
#!/bin/bash
# Get more context around the normalizeProjectEditor function
# Look for the function definition and full webcamLayoutPreset/webcamPosition handling
rg -n -B10 -A15 'normalizeProjectEditor' src/components/video-editor/projectPersistence.ts | head -100Repository: siddharthvaddem/openscreen
Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Get the complete normalizeProjectEditor function to see full webcamPosition handling
rg -n -A80 'export function normalizeProjectEditor' src/components/video-editor/projectPersistence.tsRepository: siddharthvaddem/openscreen
Length of output: 3629
🏁 Script executed:
#!/bin/bash
# Get webcamPosition normalization specifically within normalizeProjectEditor
sed -n '174,450p' src/components/video-editor/projectPersistence.tsRepository: siddharthvaddem/openscreen
Length of output: 8632
Inconsistency between runtime and project load behavior for webcamPosition.
When switching presets at runtime, webcamPosition is cleared to null for non-PiP presets. However, normalizeProjectEditor in projectPersistence.ts (lines 360-369) validates and preserves webcamPosition regardless of the webcamLayoutPreset — a project saved with {"webcamLayoutPreset": "dual-frame", "webcamPosition": {cx: 0.5, cy: 0.5}} will retain the stale position on load, contradicting the runtime behavior.
Normalize webcamPosition to null in normalizeProjectEditor when webcamLayoutPreset is not "picture-in-picture".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 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.
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