fix: validate project persistence state & restore vertical-stack layout bounds#361
Open
verivusOSS-releases wants to merge 3 commits intosiddharthvaddem:mainfrom
Open
Conversation
Prevent invalid project loads from poisoning currentProjectPath or session state. Move currentProjectPath assignment after structure validation in load-project-file. Add validation + stale-state cleanup in load-current-project-file. Make clear-current-video-path also clear currentProjectPath to prevent startup reviving stale projects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…te (U06) Fix vertical-stack layout regression: use centerRect with maxContentSize when no webcam is present instead of full-canvas cover mode. Fix bitrate regression: pass actual capture frame rate to computeBitrate instead of comparing the constant TARGET_FRAME_RATE against the threshold. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughProject/session loading handlers now validate parsed project objects and clear stale session state on failures; bitrate helper accepts an explicit frameRate parameter and callers pass the track's frameRate; the composite layout's vertical stack logic was reworked to compute shared widths, aspect-ratio heights, scaling, and centering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![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)
electron/ipc/handlers.ts (1)
120-163:⚠️ Potential issue | 🟠 Major
getApprovedProjectSessionis genuinely dead code — remove it or restore the path validationConfirmed: neither
load-project-filenorload-current-project-filecall this function. Worse, they're also skipping the path validation that lived inside it. Both handlers directly normalize media paths and set the session without checking againstRECORDINGS_DIRor the project's directory, kinda cursing the security model the function tried to enforce ("prevents crafted project files from approving reads to arbitrary locations").Either path validation should be restored in both handlers (using
approveReadableVideoPathagainst trusted dirs), or this function should be removed if the validation is intentionally being dropped. Right now it's just dead weight + a warning sign that something got refactored without thinking through the security implications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 120 - 163, getApprovedProjectSession is now unused but its path-validation logic was removed from load-project-file and load-current-project-file, reintroducing a security gap; either delete getApprovedProjectSession or restore its validation in those handlers by invoking approveReadableVideoPath for media.screenVideoPath and media.webcamVideoPath against the trusted dirs array (which must include RECORDINGS_DIR and the project file's directory when available) and throw on invalid paths, ensuring the handlers use the same sanitized screenVideoPath/webcamVideoPath values that getApprovedProjectSession would return.
🤖 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/ipc/handlers.ts`:
- Around line 866-887: The handler currently accepts media/video paths from
normalizeProjectMedia and normalizeVideoSourcePath and sets them into
setCurrentRecordingSessionState without checking filesystem permissions; before
storing, validate each resulting path (e.g., screenVideoPath and any files
returned by normalizeProjectMedia) using the same isPathAllowed approval logic
used by the load-project-file flow and reject or nullify any disallowed paths so
they won't later be blocked by isPathAllowed; update the branch that creates
"media" to run each path through the approval check and only
setCurrentRecordingSessionState with an approved-media object (or clear/return
failure if key paths are invalid).
- Around line 822-841: The new branch sets currentRecordingSessionState from raw
media without validating or approving file paths; revert to the prior approval
flow by running the same validation/approval used in getApprovedProjectSession
(or call getApprovedProjectSession(filePath)) to ensure media paths returned by
normalizeProjectMedia/normalizeVideoSourcePath actually exist and are inside the
trusted project directory or RECORDINGS_DIR, add those validated paths to
approvedPaths, and only then call setCurrentRecordingSessionState and update
currentProjectPath; ensure any invalid or unapproved media yields the same
failure response as before.
In `@src/lib/compositeLayout.ts`:
- Around line 200-207: The independent rounding of sHeight and wHeight can
produce a 1px mismatch between resolvedScreenHeight + resolvedWebcamHeight and
Math.round(sHeight + wHeight); change the logic to round the total first (use
resolvedTotalHeight = Math.round(sHeight + wHeight)) and then compute the parts
so they sum to that total (e.g., compute one part by Math.round(sHeight) and set
the other as resolvedTotalHeight - resolvedScreenHeight, or vice versa) to
ensure pixel-perfect stacking; update uses of resolvedScreenHeight,
resolvedWebcamHeight and offsetY accordingly.
- Around line 164-198: The scaling logic multiplies commonWidth, sHeight,
wHeight multiple times redundantly; replace the multi-pass adjustments with a
single uniform scale factor computed from all constraints: compute commonWidth =
Math.min(maxW, canvasWidth), derive sHeight/wHeight/totalHeight, then compute
scale = Math.min(1, maxH / totalHeight, canvasWidth / commonWidth, canvasHeight
/ totalHeight) and apply commonWidth *= scale, sHeight *= scale, wHeight *=
scale (update totalHeight if needed). Update the code that currently adjusts
commonWidth/sHeight/wHeight in the blocks referencing commonWidth, sHeight,
wHeight, totalHeight, maxContentSize, canvasWidth, canvasHeight to use this
single-pass scaling.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 120-163: getApprovedProjectSession is now unused but its
path-validation logic was removed from load-project-file and
load-current-project-file, reintroducing a security gap; either delete
getApprovedProjectSession or restore its validation in those handlers by
invoking approveReadableVideoPath for media.screenVideoPath and
media.webcamVideoPath against the trusted dirs array (which must include
RECORDINGS_DIR and the project file's directory when available) and throw on
invalid paths, ensuring the handlers use the same sanitized
screenVideoPath/webcamVideoPath values that getApprovedProjectSession would
return.
🪄 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: a9dc457e-bc64-44be-9420-97ceb247579f
📒 Files selected for processing (3)
electron/ipc/handlers.tssrc/hooks/useScreenRecorder.tssrc/lib/compositeLayout.ts
Comment on lines
+164
to
+198
| // Both screen and webcam share a common width; compute heights at that width | ||
| const screenAspect = screenWidth / screenHeight; | ||
| const webcamAspect = webcamWidth / webcamHeight; | ||
| const resolvedWebcamWidth = canvasWidth; | ||
| const resolvedWebcamHeight = Math.round(canvasWidth / webcamAspect); | ||
|
|
||
| // Screen: fills remaining space at the top (cover mode — may crop sides) | ||
| const screenRectHeight = canvasHeight - resolvedWebcamHeight; | ||
| // Start with the max content width, then derive heights | ||
| const { width: maxW, height: maxH } = maxContentSize; | ||
| let commonWidth = Math.min(maxW, canvasWidth); | ||
| let sHeight = commonWidth / screenAspect; | ||
| let wHeight = commonWidth / webcamAspect; | ||
| let totalHeight = sHeight + wHeight; | ||
|
|
||
| // If combined height exceeds the max, scale everything down uniformly | ||
| if (totalHeight > maxH) { | ||
| const heightScale = maxH / totalHeight; | ||
| commonWidth = commonWidth * heightScale; | ||
| sHeight = sHeight * heightScale; | ||
| wHeight = wHeight * heightScale; | ||
| totalHeight = sHeight + wHeight; | ||
| } | ||
|
|
||
| // Also ensure we don't exceed canvas dimensions | ||
| if (commonWidth > canvasWidth) { | ||
| const scale = canvasWidth / commonWidth; | ||
| commonWidth *= scale; | ||
| sHeight *= scale; | ||
| wHeight *= scale; | ||
| totalHeight = sHeight + wHeight; | ||
| } | ||
| if (totalHeight > canvasHeight) { | ||
| const scale = canvasHeight / totalHeight; | ||
| commonWidth *= scale; | ||
| sHeight *= scale; | ||
| wHeight *= scale; | ||
| totalHeight = sHeight + wHeight; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
the stacking math looks solid, but the double-pass scaling is a bit redundant
the logic flow:
- start with
commonWidth = min(maxW, canvasWidth) - if combined height > maxH, scale down
- then check if width > canvasWidth and scale again
- then check if height > canvasHeight and scale again
since commonWidth already starts at min(maxW, canvasWidth), the check at lines 185-191 can only trigger if the height scaling in 176-182 somehow increased the width... which it can't (it only shrinks things).
similarly, if maxContentSize is always <= canvasSize (which seems to be the contract), the canvas bounds checks are defensive but unlikely to ever fire.
not a bug, just lowkey redundant. the defensive coding is fine to keep though—protects against weird edge cases if someone passes a maxContentSize larger than canvas.
🧹 optional: simplify by combining bounds checks
// Start with the max content width, then derive heights
const { width: maxW, height: maxH } = maxContentSize;
- let commonWidth = Math.min(maxW, canvasWidth);
+ let commonWidth = Math.min(maxW, canvasWidth);
let sHeight = commonWidth / screenAspect;
let wHeight = commonWidth / webcamAspect;
let totalHeight = sHeight + wHeight;
- // If combined height exceeds the max, scale everything down uniformly
- if (totalHeight > maxH) {
- const heightScale = maxH / totalHeight;
- commonWidth = commonWidth * heightScale;
- sHeight = sHeight * heightScale;
- wHeight = wHeight * heightScale;
- totalHeight = sHeight + wHeight;
- }
-
- // Also ensure we don't exceed canvas dimensions
- if (commonWidth > canvasWidth) {
- const scale = canvasWidth / commonWidth;
- commonWidth *= scale;
- sHeight *= scale;
- wHeight *= scale;
- totalHeight = sHeight + wHeight;
- }
- if (totalHeight > canvasHeight) {
- const scale = canvasHeight / totalHeight;
+ // Scale down uniformly if we exceed any bounds
+ const maxAllowedHeight = Math.min(maxH, canvasHeight);
+ if (totalHeight > maxAllowedHeight) {
+ const scale = maxAllowedHeight / totalHeight;
commonWidth *= scale;
sHeight *= scale;
wHeight *= scale;
totalHeight = sHeight + wHeight;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/compositeLayout.ts` around lines 164 - 198, The scaling logic
multiplies commonWidth, sHeight, wHeight multiple times redundantly; replace the
multi-pass adjustments with a single uniform scale factor computed from all
constraints: compute commonWidth = Math.min(maxW, canvasWidth), derive
sHeight/wHeight/totalHeight, then compute scale = Math.min(1, maxH /
totalHeight, canvasWidth / commonWidth, canvasHeight / totalHeight) and apply
commonWidth *= scale, sHeight *= scale, wHeight *= scale (update totalHeight if
needed). Update the code that currently adjusts commonWidth/sHeight/wHeight in
the blocks referencing commonWidth, sHeight, wHeight, totalHeight,
maxContentSize, canvasWidth, canvasHeight to use this single-pass scaling.
Comment on lines
+200
to
+207
| const resolvedWidth = Math.round(commonWidth); | ||
| const resolvedScreenHeight = Math.round(sHeight); | ||
| const resolvedWebcamHeight = Math.round(wHeight); | ||
| const resolvedTotalHeight = resolvedScreenHeight + resolvedWebcamHeight; | ||
|
|
||
| // Center the stacked pair within the canvas | ||
| const offsetX = Math.max(0, Math.floor((canvasWidth - resolvedWidth) / 2)); | ||
| const offsetY = Math.max(0, Math.floor((canvasHeight - resolvedTotalHeight) / 2)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
minor: rounding could introduce 1px gaps
resolvedTotalHeight = resolvedScreenHeight + resolvedWebcamHeight might differ from Math.round(sHeight + wHeight) due to independent rounding. e.g., if sHeight = 540.4 and wHeight = 540.4, you get 540 + 540 = 1080 but Math.round(1080.8) = 1081.
in practice this probably manifests as a 1px gap or overlap between screen and webcam, which is pretty minor. just calling it out in case pixel-perfect alignment matters for the compositor.
🔧 optional: round total height first, then partition
- const resolvedWidth = Math.round(commonWidth);
- const resolvedScreenHeight = Math.round(sHeight);
- const resolvedWebcamHeight = Math.round(wHeight);
- const resolvedTotalHeight = resolvedScreenHeight + resolvedWebcamHeight;
+ const resolvedWidth = Math.round(commonWidth);
+ const resolvedTotalHeight = Math.round(sHeight + wHeight);
+ // partition total height proportionally to avoid 1px gaps
+ const resolvedScreenHeight = Math.round(resolvedTotalHeight * (sHeight / (sHeight + wHeight)));
+ const resolvedWebcamHeight = resolvedTotalHeight - resolvedScreenHeight;📝 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
| const resolvedWidth = Math.round(commonWidth); | |
| const resolvedScreenHeight = Math.round(sHeight); | |
| const resolvedWebcamHeight = Math.round(wHeight); | |
| const resolvedTotalHeight = resolvedScreenHeight + resolvedWebcamHeight; | |
| // Center the stacked pair within the canvas | |
| const offsetX = Math.max(0, Math.floor((canvasWidth - resolvedWidth) / 2)); | |
| const offsetY = Math.max(0, Math.floor((canvasHeight - resolvedTotalHeight) / 2)); | |
| const resolvedWidth = Math.round(commonWidth); | |
| const resolvedTotalHeight = Math.round(sHeight + wHeight); | |
| // partition total height proportionally to avoid 1px gaps | |
| const resolvedScreenHeight = Math.round(resolvedTotalHeight * (sHeight / (sHeight + wHeight))); | |
| const resolvedWebcamHeight = resolvedTotalHeight - resolvedScreenHeight; | |
| // Center the stacked pair within the canvas | |
| const offsetX = Math.max(0, Math.floor((canvasWidth - resolvedWidth) / 2)); | |
| const offsetY = Math.max(0, Math.floor((canvasHeight - resolvedTotalHeight) / 2)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/compositeLayout.ts` around lines 200 - 207, The independent rounding
of sHeight and wHeight can produce a 1px mismatch between resolvedScreenHeight +
resolvedWebcamHeight and Math.round(sHeight + wHeight); change the logic to
round the total first (use resolvedTotalHeight = Math.round(sHeight + wHeight))
and then compute the parts so they sum to that total (e.g., compute one part by
Math.round(sHeight) and set the other as resolvedTotalHeight -
resolvedScreenHeight, or vice versa) to ensure pixel-perfect stacking; update
uses of resolvedScreenHeight, resolvedWebcamHeight and offsetY accordingly.
![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: 8e85b69616
ℹ️ 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".
electron/ipc/handlers.ts
Outdated
Comment on lines
+832
to
+833
| currentProjectPath = filePath; | ||
| setCurrentRecordingSessionState(media ? { ...media, createdAt: Date.now() } : null); |
There was a problem hiding this comment.
This assignment writes the session directly from parsed project media and skips getApprovedProjectSession/approveReadableVideoPath, so loaded media files are never added to approvedPaths. In projects opened from outside RECORDINGS_DIR, later IPC reads guarded by isPathAllowed (notably read-binary-file, which StreamingVideoDecoder uses during export) are denied, so a valid project can open but then fail on export/telemetry access.
Useful? React with 👍 / 👎.
…tests - load-project-file: use getApprovedProjectSession to validate and approve media paths before storing in session state (prevents crafted project files from referencing arbitrary filesystem paths) - load-current-project-file: same validation restoration - Fix vertical-stack layout tests to match bounded layout behavior Addresses review feedback from CodeRabbit and Gemini (path validation gap).
verivusOSS-releases
pushed a commit
to verivusOSS-releases/openscreen
that referenced
this pull request
Apr 6, 2026
…-in-folder, and exports - set-current-video-path: use isPathAllowed (checks approvedPaths + trusted dirs) instead of isTrustedMediaPath so user-picked external videos are accepted - reveal-in-folder: use isPathAllowed so exported files in Downloads etc. can be revealed - save-exported-video: approve the save path so reveal-in-folder works after export - Remove unused resolveRecordingOutputPath (replaced by sanitizeFileName) - Remove vertical-stack layout test that belongs with the layout fix (PR siddharthvaddem#361) Addresses review feedback from Codex, CodeRabbit, and Gemini.
verivusOSS-releases
pushed a commit
to verivusOSS-releases/openscreen
that referenced
this pull request
Apr 6, 2026
…llpaper, fonts - Restore approvedPaths/isPathAllowed/approveReadableVideoPath/getApprovedProjectSession infrastructure that was inadvertently removed by exfiltration hardening commit - set-current-video-path: use isPathAllowed so user-picked external videos work - reveal-in-folder: use isPathAllowed so exported files can be revealed - save-exported-video: approve save path for subsequent reveal-in-folder - VideoPlayback: recognize app-media:// URLs in wallpaper resolution and isImageUrl - frameRenderer: recognize app-media:// URLs in wallpaper rendering during export - customFonts: allow semicolons in font URLs (Google Fonts weight lists use them) - Remove vertical-stack layout test that belongs with the layout fix (PR siddharthvaddem#361) Addresses review feedback from Codex, CodeRabbit, and Gemini across all findings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ipc/handlers.ts`:
- Around line 822-828: The current validation only checks typeof project ===
"object" which permits arrays; update the guard that returns the error to also
reject null and arrays (e.g. require project !== null &&
!Array.isArray(project)) so only plain objects pass, and apply the same stricter
check at the other validation block around the project handling (the block that
references project and filePath later in the file). Ensure the error return
shape stays the same but is triggered when project is null or an array.
In `@src/lib/compositeLayout.test.ts`:
- Around line 46-50: The test currently allows webcam placement to be missing
and accepts any positive x; update the assertions to (1) explicitly require
layout.webcamRect to be non-null (assert layout.webcamRect !== null/defined) and
use layout.webcamRect.height directly in the height sum instead of the
null-coalescing fallback, and (2) tighten the horizontal assertion on
layout.screenRect.x by asserting it lies within the allowed horizontal bounds
and is near center (for example assert screenRect.x >= 0 && screenRect.x +
screenRect.width <= 1536 and that the screenRect center is within an acceptable
pixel threshold of 1536/2) so off-center stack regressions are caught; refer to
layout.webcamRect and layout.screenRect.width/height/x when making the changes.
🪄 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: ec991f70-ee48-4878-a37d-d8ca3cf7844b
📒 Files selected for processing (2)
electron/ipc/handlers.tssrc/lib/compositeLayout.test.ts
Comment on lines
+822
to
+828
| if (!project || typeof project !== "object") { | ||
| return { | ||
| success: false, | ||
| path: filePath, | ||
| message: "Invalid project structure: file does not contain a valid project object", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Reject arrays here — typeof alone is too loose for project validation.
Line 822 and Line 857 currently allow [] (and other non-plain objects) through as “valid,” which can still flow into success paths and mutate state. kinda cursed, lowkey risky for malformed .openscreen inputs.
🛠️ Proposed fix
+function isValidProjectPayload(project: unknown): project is Record<string, unknown> {
+ return Boolean(project) && typeof project === "object" && !Array.isArray(project);
+}
+
ipcMain.handle("load-project-file", async () => {
try {
@@
- if (!project || typeof project !== "object") {
+ if (!isValidProjectPayload(project)) {
return {
success: false,
path: filePath,
message: "Invalid project structure: file does not contain a valid project object",
};
}
@@
ipcMain.handle("load-current-project-file", async () => {
@@
- if (!project || typeof project !== "object") {
+ if (!isValidProjectPayload(project)) {
const stalePath = currentProjectPath;
currentProjectPath = null;
setCurrentRecordingSessionState(null);
return {Also applies to: 857-866
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 822 - 828, The current validation only
checks typeof project === "object" which permits arrays; update the guard that
returns the error to also reject null and arrays (e.g. require project !== null
&& !Array.isArray(project)) so only plain objects pass, and apply the same
stricter check at the other validation block around the project handling (the
block that references project and filePath later in the file). Ensure the error
return shape stays the same but is triggered when project is null or an array.
Comment on lines
+46
to
+50
| expect(layout!.screenRect.width).toBeLessThanOrEqual(1536); | ||
| expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual( | ||
| 864 + 1, | ||
| ); | ||
| expect(layout!.screenRect.x).toBeGreaterThan(0); |
There was a problem hiding this comment.
Tighten this test: it can pass even if webcam placement breaks.
At Line 47, layout!.webcamRect?.height ?? 0 means the assertion still passes if webcamRect is unexpectedly null. Also, screenRect.x > 0 (Line 50) is pretty permissive and won’t catch off-center stack regressions. kinda lowkey risky for this fix area.
nit: cleaner + stronger assertions
expect(layout).not.toBeNull();
+ expect(layout!.webcamRect).not.toBeNull();
expect(layout!.screenRect.width).toBeLessThanOrEqual(1536);
expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual(
864 + 1,
);
- expect(layout!.screenRect.x).toBeGreaterThan(0);
+ const stackLeft = Math.min(layout!.screenRect.x, layout!.webcamRect!.x);
+ const stackRight = Math.max(
+ layout!.screenRect.x + layout!.screenRect.width,
+ layout!.webcamRect!.x + layout!.webcamRect!.width,
+ );
+ expect(stackLeft).toBeGreaterThanOrEqual(192);
+ expect(stackRight).toBeLessThanOrEqual(192 + 1536);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/compositeLayout.test.ts` around lines 46 - 50, The test currently
allows webcam placement to be missing and accepts any positive x; update the
assertions to (1) explicitly require layout.webcamRect to be non-null (assert
layout.webcamRect !== null/defined) and use layout.webcamRect.height directly in
the height sum instead of the null-coalescing fallback, and (2) tighten the
horizontal assertion on layout.screenRect.x by asserting it lies within the
allowed horizontal bounds and is near center (for example assert screenRect.x >=
0 && screenRect.x + screenRect.width <= 1536 and that the screenRect center is
within an acceptable pixel threshold of 1536/2) so off-center stack regressions
are caught; refer to layout.webcamRect and layout.screenRect.width/height/x when
making the changes.
codeCraft-Ritik
left a comment
codeCraft-Ritik
left a comment
There was a problem hiding this comment.
Great catch on both counts. Validating the project structure before mutation is a critical safety net to prevent state corruption, and using the actual capture framerate for bitrate calculation will significantly improve export consistency
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.
Description
Two independent bug fixes: (1) validate parsed project structure before mutating
currentProjectPathor recording session state, and (2) restore correct vertical-stack layout bounds withinmaxContentSizeand use actual capture framerate for bitrate calculation.Motivation
Persistence bug: When loading a malformed or empty
.openscreenproject file, the IPC handlers would silently setcurrentProjectPathand corruptcurrentRecordingSessionwith invalid data, leaving the editor in a broken state with no error feedback.Layout bug: In vertical-stack mode with padding, the screen+webcam stack was not being constrained within
maxContentSize, causing content to overflow the padded area. Additionally, the recorder used a hardcoded framerate for bitrate calculation instead of reading the actual framerate from the captured stream.Type of Change
Related Issue(s)
None.
Screenshots / Video
N/A — logic fixes with no visual changes to report.
Testing
npm test— all tests pass.openscreenfile with invalid JSON content (e.g."not an object") and open it → should show error message, editor state should remain unchangedChecklist
Methodology
These fixes were identified through a multi-LLM analysis workflow using llm-cli-gateway, an MCP-based tool for orchestrating parallel code review across Claude, Codex, and Gemini. The persistence validation gap and layout regression were flagged during the initial security audit as non-security issues worth fixing alongside the hardening work.
Thank you for contributing!
Summary by CodeRabbit
Bug Fixes
Improvements
Tests