Enhancement: WQHD/High-DPI compatibility and Zoom Algorithm Refinements#372
Enhancement: WQHD/High-DPI compatibility and Zoom Algorithm Refinements#372jsaraivx wants to merge 7 commits intosiddharthvaddem:mainfrom
Conversation
![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: a682d4fb6a
ℹ️ 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".
src/hooks/useScreenRecorder.ts
Outdated
| const TARGET_HEIGHT = 2160; | ||
| const TARGET_WIDTH = 7680; | ||
| const TARGET_HEIGHT = 4320; | ||
| const FOUR_K_PIXELS = TARGET_WIDTH * TARGET_HEIGHT; |
There was a problem hiding this comment.
This change ties FOUR_K_PIXELS to the new 8K target dimensions, so computeBitrate() no longer classifies 3840×2160 captures as 4K. In practice, common 4K recordings now fall through to the QHD branch and get ~28 Mbps instead of ~45 Mbps, which is a clear quality regression introduced by this constant update.
Useful? React with 👍 / 👎.
| } | ||
| pushRunIfDwell(runStart, samples.length); | ||
|
|
||
| return dwellCandidates; | ||
| if (rawDwells.length === 0) return []; |
There was a problem hiding this comment.
detectZoomDwellCandidates now only pushes a dwell when movement crosses DWELL_MOVE_THRESHOLD, but it never flushes the last run after the loop. When the cursor remains still until the telemetry ends (a common case near clip end), that final dwell is silently dropped, so valid zoom suggestions are missed.
Useful? React with 👍 / 👎.
| zoomRegions: ZoomRegion[]; | ||
| onZoomAdded: (span: Span) => void; | ||
| onZoomSuggested?: (span: Span, focus: ZoomFocus) => void; | ||
| onZoomSuggested?: (span: Span, focus: ZoomFocus, depth?: number) => void; |
There was a problem hiding this comment.
Declaring onZoomSuggested with depth? lets existing handlers compile even if they ignore adaptive depth. The current VideoEditor handler still hardcodes DEFAULT_ZOOM_DEPTH, so the depth calculated in zoomSuggestionUtils is effectively discarded and all auto-suggested zooms keep the default depth.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis PR adds Windows installer customization (NSIS configuration, execution level, icons), enforces single-instance app behavior with DPI/identity settings, refactors zoom-dwell detection with temporal grouping and adaptive depth tracking, threads depth through the zoom callback chain, and introduces 8K bitrate support for screen recording. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useScreenRecorder.ts (1)
507-511: 🧹 Nitpick | 🔵 Trivialnit: stale comment references 4K but target is now 8K
the comment still says "4K/60fps" but you've bumped the target to 8K. small thing but might confuse future readers.
📝 suggested fix
} catch (constraintError) { console.warn( - "Unable to lock 4K/60fps constraints, using best available track settings.", + "Unable to lock 8K/60fps constraints, using best available track settings.", constraintError, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 507 - 511, Update the stale console.warn message in the useScreenRecorder catch block: replace the "4K/60fps" text with the current target "8K/60fps" (the warning in the catch (constraintError) handler that says "Unable to lock 4K/60fps constraints, using best available track settings.") so the log accurately reflects the new target; ensure the variable constraintError remains included in the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron-builder.json5`:
- Around line 41-47: The entitlement key com.apple.security.device.audio-input
is incorrectly placed inside extendInfo in electron-builder.json5 (Info.plist
keys belong there but code-signing entitlements do not); move that entitlement
into a macOS entitlements plist and reference it from the build config by
enabling hardened runtime and setting the entitlements/entitlementsInherit
fields (set mac.hardenedRuntime true and point entitlements and
entitlementsInherit to your entitlements plist), while keeping
NSMicrophoneUsageDescription and NSAudioCaptureUsageDescription in extendInfo so
permission prompts still show.
In `@electron/main.ts`:
- Around line 21-34: The code calls app.quit() when
app.requestSingleInstanceLock() returns false but continues module execution
causing a race where the second instance still runs initialization; modify the
module so no further app setup runs when gotTheLock is false—either move all
initialization (including app.whenReady(), window creation, IPC registration and
any listeners referencing mainWindow) inside the existing else block that
handles the single-instance case, or immediately stop execution after app.quit()
by returning/throwing (e.g., throw new Error("Another instance is already
running")) so functions like app.whenReady(), mainWindow creation, and IPC
handlers never run when gotTheLock is false.
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts`:
- Around line 113-117: The current grouping condition compares curr.startTimeMs
(which already includes ZOOM_REACTION_DELAY_MS) against prev.endTimeMs,
effectively shrinking the effective threshold by the delay; instead compute the
raw gap by subtracting ZOOM_REACTION_DELAY_MS from curr.startTimeMs and compare
that to GROUPING_THRESHOLD_MS. Replace the condition in the grouping loop with
something like: (curr.startTimeMs - ZOOM_REACTION_DELAY_MS - prev.endTimeMs) <
GROUPING_THRESHOLD_MS, keeping references to curr.startTimeMs, prev.endTimeMs,
GROUPING_THRESHOLD_MS, and ZOOM_REACTION_DELAY_MS and leaving
mergeDwellGroup/currentGroup/groupedDwells logic unchanged.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1654-1658: The current onAspectRatioChange handler calls
pushState({ aspectRatio: ar, padding: ar === "native" ? 0 : padding,
borderRadius: ar === "native" ? 0 : borderRadius }) which permanently overwrites
user padding/borderRadius when switching to "native"; instead only update
aspectRatio in pushState and stop writing zeros to padding/borderRadius, and
derive the zeroed values at render/export time (e.g., compute
effectivePadding/effectiveBorderRadius = aspectRatio === "native" ? 0 : padding)
or, if you prefer restoring, stash previous values into separate keys (e.g.,
prevPadding/prevBorderRadius) on non-native switch and restore them when leaving
"native"; update the onAspectRatioChange, pushState usage, and any consumers
that render/export to use the derived effectivePadding/effectiveBorderRadius.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 507-511: Update the stale console.warn message in the
useScreenRecorder catch block: replace the "4K/60fps" text with the current
target "8K/60fps" (the warning in the catch (constraintError) handler that says
"Unable to lock 4K/60fps constraints, using best available track settings.") so
the log accurately reflects the new target; ensure the variable constraintError
remains included in the warning.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5574bdd-6bd4-4635-a97c-fa45e4b07f09
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
electron-builder.json5electron/main.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/hooks/useScreenRecorder.ts
| "extendInfo": { | ||
| "NSAudioCaptureUsageDescription": "OpenScreen needs audio capture permission to record system audio.", | ||
| "NSMicrophoneUsageDescription": "OpenScreen needs microphone access to record voice audio.", | ||
| "NSCameraUsageDescription": "OpenScreen needs camera access to record webcam video.", | ||
| "NSCameraUseContinuityCameraDeviceType": true, | ||
| "com.apple.security.device.audio-input": true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to configure macOS entitlements in electron-builder for audio input access?
💡 Result:
To get microphone (audio input) access working on macOS with electron-builder, you need both:
- an entitlements plist used at code-sign time, and
- the right Info.plist usage-description keys (or macOS won’t show the permission prompt / will deny capture).
1) Create an entitlements file (recommended)
build/entitlements.mac.plist
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<!-- Microphone / audio input -->
<key>com.apple.security.device.audio-input</key>
<true/>
</dict>
</plist>Apple documents com.apple.security.device.audio-input as the entitlement that allows microphone/audio-input access. [1]
2) Point electron-builder at the entitlements (+ hardened runtime)
In your electron-builder config:
package.json
{
"build": {
"mac": {
"hardenedRuntime": true,
"entitlements": "build/entitlements.mac.plist",
"entitlementsInherit": "build/entitlements.mac.plist",
"extendInfo": {
"NSMicrophoneUsageDescription": "This app needs microphone access for voice input."
}
}
}
}entitlementssigns the main app with your entitlements.entitlementsInheritsigns helper apps/frameworks with inherited entitlements (electron-builder explicitly supports this and recommends using it). [2]- Notarized macOS builds require hardened runtime + appropriate entitlements. [3]
3) If you capture system/desktop audio, add NSAudioCaptureUsageDescription too (macOS 14.2+)
If you use desktopCapturer to capture audio, Electron documents that on macOS 14.2+ you must add:
NSAudioCaptureUsageDescriptiontoInfo.plist(viaextendInfo) [4]
Example:
"extendInfo": {
"NSMicrophoneUsageDescription": "Mic is used for voice input.",
"NSAudioCaptureUsageDescription": "Audio capture is used for screen sharing with audio."
}4) Trigger the permission prompt at runtime
macOS won’t prompt just because the keys exist; you must actually request/use the microphone, e.g.:
navigator.mediaDevices.getUserMedia({ audio: true })(renderer), orsystemPreferences.askForMediaAccess('microphone')(main)
(And you must have NSMicrophoneUsageDescription present, as above.)
Common gotcha
If permissions work in development but not after packaging: the signed .app is a different binary identity than the dev Electron app, so macOS permissions must be granted to (and prompted by) the packaged app.
Sources: Apple entitlement reference [1]; electron-builder Mac configuration (entitlementsInherit) [2]; electron-builder hardened runtime/notarization guidance [3]; Electron desktopCapturer docs for NSAudioCaptureUsageDescription requirement [4].
com.apple.security.device.audio-input needs to be in an entitlements file, not extendInfo
heads up - com.apple.security.device.audio-input is a code signing entitlement, not an Info.plist key. sticking it in extendInfo won't actually do anything. if you're relying on this for sandboxed audio input, it's gonna silently fail.
the NS* keys are legit though - those are meant for extendInfo.
you'll need to set this up properly via electron-builder:
- create
build/entitlements.mac.plistwith the entitlement - configure in
electron-builder.json5(orpackage.json):"mac": { "hardenedRuntime": true, "entitlements": "build/entitlements.mac.plist", "entitlementsInherit": "build/entitlements.mac.plist" }
keep the NSMicrophoneUsageDescription and NSAudioCaptureUsageDescription in extendInfo - those are needed for the permission prompts to actually show up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron-builder.json5` around lines 41 - 47, The entitlement key
com.apple.security.device.audio-input is incorrectly placed inside extendInfo in
electron-builder.json5 (Info.plist keys belong there but code-signing
entitlements do not); move that entitlement into a macOS entitlements plist and
reference it from the build config by enabling hardened runtime and setting the
entitlements/entitlementsInherit fields (set mac.hardenedRuntime true and point
entitlements and entitlementsInherit to your entitlements plist), while keeping
NSMicrophoneUsageDescription and NSAudioCaptureUsageDescription in extendInfo so
permission prompts still show.
| // Single instance lock | ||
| const gotTheLock = app.requestSingleInstanceLock(); | ||
|
|
||
| if (!gotTheLock) { | ||
| app.quit(); | ||
| } else { | ||
| app.on("second-instance", (_event, _commandLine, _workingDirectory) => { | ||
| // Someone tried to run a second instance, we should focus our window. | ||
| if (mainWindow) { | ||
| if (mainWindow.isMinimized()) mainWindow.restore(); | ||
| mainWindow.focus(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
race condition after app.quit() - second instance still runs init code
yo, this is lowkey risky. when gotTheLock is false, you call app.quit() but the rest of the file keeps executing since quit is async. the second instance will still hit app.whenReady(), try to create windows, register IPC handlers, etc. before actually dying.
common fix is to wrap all app initialization inside the else block or early-return the module.
🛠️ suggested fix - wrap init code in the else block
const gotTheLock = app.requestSingleInstanceLock();
if (!gotTheLock) {
app.quit();
-} else {
+ // Note: app.quit() is async, but we need to prevent further initialization
+ // The process will exit soon, but we should not continue setup
+}
+
+// Only proceed with app initialization if we got the lock
+if (gotTheLock) {
app.on("second-instance", (_event, _commandLine, _workingDirectory) => {
// Someone tried to run a second instance, we should focus our window.
if (mainWindow) {
if (mainWindow.isMinimized()) mainWindow.restore();
mainWindow.focus();
}
});
-}
+
+ // ... rest of initialization should be inside this block or guardedalternatively, you could add a guard before app.whenReady():
if (!gotTheLock) {
app.quit();
// Prevent any further initialization
throw new Error("Another instance is already running");
}or restructure to check the lock before any heavy setup runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.ts` around lines 21 - 34, The code calls app.quit() when
app.requestSingleInstanceLock() returns false but continues module execution
causing a race where the second instance still runs initialization; modify the
module so no further app setup runs when gotTheLock is false—either move all
initialization (including app.whenReady(), window creation, IPC registration and
any listeners referencing mainWindow) inside the existing else block that
handles the single-instance case, or immediately stop execution after app.quit()
by returning/throwing (e.g., throw new Error("Another instance is already
running")) so functions like app.whenReady(), mainWindow creation, and IPC
handlers never run when gotTheLock is false.
| if (curr.startTimeMs - prev.endTimeMs < GROUPING_THRESHOLD_MS) { | ||
| currentGroup.push(curr); | ||
| } else { | ||
| groupedDwells.push(mergeDwellGroup(currentGroup)); | ||
| currentGroup = [curr]; |
There was a problem hiding this comment.
The reaction delay is lowkey eating 300ms of grouping.
curr.startTimeMs already includes the 300ms delay, so this check only merges dwells when the raw gap is < GROUPING_THRESHOLD_MS - ZOOM_REACTION_DELAY_MS. Stops 700–1000ms apart now split even though the constant says they should merge. Compare against the raw dwell gap here instead.
nit: cleaner threshold check
- if (curr.startTimeMs - prev.endTimeMs < GROUPING_THRESHOLD_MS) {
+ if (
+ curr.startTimeMs - ZOOM_REACTION_DELAY_MS - prev.endTimeMs <
+ GROUPING_THRESHOLD_MS
+ ) {📝 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.
| if (curr.startTimeMs - prev.endTimeMs < GROUPING_THRESHOLD_MS) { | |
| currentGroup.push(curr); | |
| } else { | |
| groupedDwells.push(mergeDwellGroup(currentGroup)); | |
| currentGroup = [curr]; | |
| if ( | |
| curr.startTimeMs - ZOOM_REACTION_DELAY_MS - prev.endTimeMs < | |
| GROUPING_THRESHOLD_MS | |
| ) { | |
| currentGroup.push(curr); | |
| } else { | |
| groupedDwells.push(mergeDwellGroup(currentGroup)); | |
| currentGroup = [curr]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts` around lines 113
- 117, The current grouping condition compares curr.startTimeMs (which already
includes ZOOM_REACTION_DELAY_MS) against prev.endTimeMs, effectively shrinking
the effective threshold by the delay; instead compute the raw gap by subtracting
ZOOM_REACTION_DELAY_MS from curr.startTimeMs and compare that to
GROUPING_THRESHOLD_MS. Replace the condition in the grouping loop with something
like: (curr.startTimeMs - ZOOM_REACTION_DELAY_MS - prev.endTimeMs) <
GROUPING_THRESHOLD_MS, keeping references to curr.startTimeMs, prev.endTimeMs,
GROUPING_THRESHOLD_MS, and ZOOM_REACTION_DELAY_MS and leaving
mergeDwellGroup/currentGroup/groupedDwells logic unchanged.
| onAspectRatioChange={(ar) => | ||
| pushState({ | ||
| aspectRatio: ar, | ||
| padding: ar === "native" ? 0 : padding, | ||
| borderRadius: ar === "native" ? 0 : borderRadius, |
There was a problem hiding this comment.
native shouldn't nuke the stored padding/radius.
Switching to native here overwrites the user's previous padding and borderRadius, and the prefs/project autosave path will persist those zeroes. When they switch back, the old styling is gone unless they rebuild it by hand. keep native as a derived render/export override, or stash and restore the previous non-native values instead.
safer shape
onAspectRatioChange={(ar) =>
pushState({
aspectRatio: ar,
- padding: ar === "native" ? 0 : padding,
- borderRadius: ar === "native" ? 0 : borderRadius,
webcamLayoutPreset:
!isPortraitAspectRatio(ar) && webcamLayoutPreset === "vertical-stack"
? "picture-in-picture"
: webcamLayoutPreset,
})
}const effectivePadding = aspectRatio === "native" ? 0 : padding;
const effectiveBorderRadius = aspectRatio === "native" ? 0 : borderRadius;🤖 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 1654 - 1658, The
current onAspectRatioChange handler calls pushState({ aspectRatio: ar, padding:
ar === "native" ? 0 : padding, borderRadius: ar === "native" ? 0 : borderRadius
}) which permanently overwrites user padding/borderRadius when switching to
"native"; instead only update aspectRatio in pushState and stop writing zeros to
padding/borderRadius, and derive the zeroed values at render/export time (e.g.,
compute effectivePadding/effectiveBorderRadius = aspectRatio === "native" ? 0 :
padding) or, if you prefer restoring, stash previous values into separate keys
(e.g., prevPadding/prevBorderRadius) on non-native switch and restore them when
leaving "native"; update the onAspectRatioChange, pushState usage, and any
consumers that render/export to use the derived
effectivePadding/effectiveBorderRadius.
1 participant
Description
This PR introduces a series of improvements focused on better support for
modern high-resolution displays (WQHD/4K+) and refines the automatic zoom
suggestion algorithm for a smoother editing experience.
Description
This PR introduces a series of improvements focused on better support for
modern high-resolution displays (WQHD/4K+) and refines the automatic zoom
suggestion algorithm for a smoother editing experience.
Key Changes
to ensure crisp rendering on Windows with scaling.
simultaneously.
shortcuts, icons, and consistent naming (Openscreen-Windows-...).
7680x4320. This prevents aliasing and ensures maximum quality when
recording on WQHD or 4K monitors.
the editor, padding and border-radius are automatically zeroed to
eliminate unwanted black borders.
a single, more meaningful zoom suggestion.
cursor's distance from the center and the duration of the dwell.
feel more natural (as it happens after the user stops moving the mouse).
high-resolution screens.
Commits
How to test?
Run the app on a Windows machine with >100% display scaling. Observe the
UI clarity.
Record a video on a WQHD monitor and check the output quality in the
editor.
Select "Native" aspect ratio and confirm the video fills the entire
canvas without borders.
Generate zoom suggestions and verify they feel more "intentional" and
less jittery. Key Changes
Windows Platform Enhancements
to ensure crisp rendering on Windows with scaling.
simultaneously.
shortcuts, icons, and consistent naming (Openscreen-Windows-...).
7680x4320. This prevents aliasing and ensures maximum quality when
recording on WQHD or 4K monitors.
the editor, padding and border-radius are automatically zeroed to
eliminate unwanted black borders.
a single, more meaningful zoom suggestion.
cursor's distance from the center and the duration of the dwell.
feel more natural (as it happens after the user stops moving the mouse).
high-resolution screens.
Commits
How to test?
UI clarity.
editor.
canvas without borders.
less jittery.
Summary by CodeRabbit
New Features
Bug Fixes