got merge problems - #142
Closed
petercr wants to merge 2 commits into
Closed
Conversation
Fix Windows sandbox and screen preset handling
📝 WalkthroughWalkthroughThe main Electron window now enables sandboxing according to platform. Backend preset validation now uses dedicated native screen and window source classifiers, including Windows DXGI and GDI Grab identifiers, with expanded coverage. ChangesElectron sandbox configuration
Native compositor source validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
crates/videorc-backend/src/live_layout.rs (1)
260-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify boolean helpers.
You can make these helpers more concise and idiomatic by using boolean chaining (
||) andOption::is_some_and.♻️ Proposed refactor
fn screen_source_is_native(screen_id: Option<&str>) -> bool { - let Some(screen_id) = screen_id else { - return false; - }; - if parse_screencapturekit_display_id(screen_id).is_some() { - return true; - } - if parse_windows_dxgi_output_index(screen_id).is_some() { - return true; - } - is_windows_gdigrab_desktop_screen_id(screen_id) + screen_id.is_some_and(|id| { + parse_screencapturekit_display_id(id).is_some() + || parse_windows_dxgi_output_index(id).is_some() + || is_windows_gdigrab_desktop_screen_id(id) + }) } fn window_source_is_native(window_id: Option<&str>) -> bool { - let Some(window_id) = window_id else { - return false; - }; - parse_screencapturekit_window_id(window_id).is_some() + window_id.is_some_and(|id| parse_screencapturekit_window_id(id).is_some()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/videorc-backend/src/live_layout.rs` around lines 260 - 279, Refactor screen_source_is_native and window_source_is_native to return their boolean conditions directly instead of using early returns and separate if blocks. Chain the screen ID parser checks with || and use Option::is_some_and for the optional IDs, preserving false for None and the existing native-source detection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/index.ts`:
- Line 343: Update the mainWindowSandboxEnabled configuration to always be true
instead of depending on isWindows, ensuring the main window’s
webPreferences.sandbox remains enabled on every platform.
In `@crates/videorc-backend/src/live_layout.rs`:
- Around line 1373-1380: Update the Windows ScreenOnly test around
preset_selection_blocker to configure a dummy screen_id instead of leaving the
screen unselected, then assert the returned rejection message indicates that the
native screen is unsupported. Preserve the existing test setup and target the
native compositor rejection path rather than the “none is selected” message.
---
Nitpick comments:
In `@crates/videorc-backend/src/live_layout.rs`:
- Around line 260-279: Refactor screen_source_is_native and
window_source_is_native to return their boolean conditions directly instead of
using early returns and separate if blocks. Chain the screen ID parser checks
with || and use Option::is_some_and for the optional IDs, preserving false for
None and the existing native-source detection behavior.
🪄 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: 8d1e52b5-75c4-4103-93df-00932f134f9e
📒 Files selected for processing (2)
apps/desktop/src/main/index.tscrates/videorc-backend/src/live_layout.rs
| type GlassVibrancyMaterial = NonNullable<Parameters<BrowserWindow['setVibrancy']>[0]> | ||
| const isMac = process.platform === 'darwin' | ||
| const isWindows = process.platform === 'win32' | ||
| const mainWindowSandboxEnabled = !isWindows |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n --no-heading \
'mainWindowSandboxEnabled|isWindows|sandbox\s*:' \
apps/desktop/src/main/index.ts
rg -n --no-heading \
'Enable `?sandbox: true`|sandbox: true|Step 3' \
plans/011-sandbox-main-electron-window.md plans/README.md
fd -t f 'package.json' . -x \
rg -n --no-heading '"electron"\s*:' {}Repository: TheOrcDev/videorc
Length of output: 1054
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== apps/desktop/src/main/index.ts (relevant ranges) ==\n'
sed -n '320,360p;960,1005p' apps/desktop/src/main/index.ts
printf '\n== plan excerpt ==\n'
sed -n '170,255p' plans/011-sandbox-main-electron-window.md
printf '\n== sandbox flag usages ==\n'
rg -n --no-heading 'mainWindowSandboxEnabled|sandbox\s*:' apps/desktop/src/main/index.tsRepository: TheOrcDev/videorc
Length of output: 6290
Keep the main window sandbox enabled on Windows too.
!isWindows makes webPreferences.sandbox false on Windows, which weakens the renderer security model and conflicts with plans/011-sandbox-main-electron-window.md’s sandbox: true requirement. Keep this hard-coded to true unless there’s a platform-specific blocker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/index.ts` at line 343, Update the
mainWindowSandboxEnabled configuration to always be true instead of depending on
isWindows, ensuring the main window’s webPreferences.sandbox remains enabled on
every platform.
Comment on lines
+1373
to
+1380
| let windows_screen = config(LayoutPreset::ScreenOnly, false, false); | ||
| assert_eq!( | ||
| preset_selection_blocker(&windows_screen), | ||
| Some( | ||
| "Layout preset ScreenOnly needs a screen or window, but none is selected. Pick one, then switch." | ||
| .to_string() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test does not set an unsupported screen ID.
Because config(..., false, false) is used, screen_id is initialized to None. As a result, this test falls into the "none is selected" code path rather than testing that an unsupported Windows screen ID is blocked.
To properly test the native compositor rejection path for Windows screens (e.g. testing an unsupported gdigrab screen index), you should set a dummy screen ID and assert that the rejection message mentions "native screen".
💚 Proposed fix to test the unsupported screen path
- let windows_screen = config(LayoutPreset::ScreenOnly, false, false);
+ let mut windows_screen = config(LayoutPreset::ScreenOnly, false, false);
+ windows_screen.sources.screen_id = Some("screen:gdigrab:1".to_string());
assert_eq!(
preset_selection_blocker(&windows_screen),
Some(
- "Layout preset ScreenOnly needs a screen or window, but none is selected. Pick one, then switch."
- .to_string()
+ "Layout preset ScreenOnly needs a native screen or window source, but the selected source cannot feed the native compositor. Pick a screen or window again, then switch.".to_string()
)
);📝 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.
Suggested change
| let windows_screen = config(LayoutPreset::ScreenOnly, false, false); | |
| assert_eq!( | |
| preset_selection_blocker(&windows_screen), | |
| Some( | |
| "Layout preset ScreenOnly needs a screen or window, but none is selected. Pick one, then switch." | |
| .to_string() | |
| ) | |
| ); | |
| let mut windows_screen = config(LayoutPreset::ScreenOnly, false, false); | |
| windows_screen.sources.screen_id = Some("screen:gdigrab:1".to_string()); | |
| assert_eq!( | |
| preset_selection_blocker(&windows_screen), | |
| Some( | |
| "Layout preset ScreenOnly needs a native screen or window source, but the selected source cannot feed the native compositor. Pick a screen or window again, then switch.".to_string() | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/live_layout.rs` around lines 1373 - 1380, Update
the Windows ScreenOnly test around preset_selection_blocker to configure a dummy
screen_id instead of leaving the screen unselected, then assert the returned
rejection message indicates that the native screen is unsupported. Preserve the
existing test setup and target the native compositor rejection path rather than
the “none is selected” message.
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
2 participants
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.
Whoops I meant to send this to my fork.
I will figure this out tomorrow
Summary by CodeRabbit