Fix cursor duplication and offset on windowed screens#659
Open
dylan-sloan wants to merge 3 commits into
Open
Conversation
Contributor
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWindows window bounds are now retrieved via PowerShell script with DPI awareness and normalized to Electron DIP coordinates. Cursor telemetry is refactored with explicit nullable returns and a centralized session initialization helper. Recording-state management includes native start timestamps and WGC capture sizes. The native recording pipeline coordinates bounds telemetry and cursor capture sessions using the native start time. Cursor presentation logic generalizes across Linux portal and Windows browser capture. Native Windows capture is now available for window sources. ChangesWindows bounds telemetry and cursor capture session coordination
Sequence Diagram(s)sequenceDiagram
participant nativeRecordingStart
participant applyWindowsWindowTelemetryBounds
participant beginCursorCaptureSession
participant startWindowBoundsCapture
nativeRecordingStart->>nativeRecordingStart: record videoStartedAtMs
nativeRecordingStart->>nativeRecordingStart: set nativeVideoRecordingStartedAtMs
alt window source
nativeRecordingStart->>applyWindowsWindowTelemetryBounds: apply bounds telemetry
nativeRecordingStart->>startWindowBoundsCapture: start periodic capture
end
nativeRecordingStart->>beginCursorCaptureSession: begin with startTimeMs from native
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
dylan-sloan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/cursor/bounds.ts (1)
315-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate x and y coordinates are finite numbers.
Line 347 validates that
widthandheightare positive, but doesn't verify thatxandyare finite numbers. If the PowerShell script returns malformed JSON withNaNorInfinityfor coordinates, these would pass through and potentially cause errors in downstream calculations.🛡️ Proposed fix to add coordinate validation
const bounds = JSON.parse(trimmedStdout) as WindowsWindowBounds; - if (!bounds || bounds.width <= 0 || bounds.height <= 0) { + if ( + !bounds || + !Number.isFinite(bounds.x) || + !Number.isFinite(bounds.y) || + bounds.width <= 0 || + bounds.height <= 0 + ) { return null; }🤖 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 `@electron/ipc/cursor/bounds.ts` around lines 315 - 355, In resolveWindowsWindowBounds, after parsing stdout into bounds (the WindowsWindowBounds object), validate that bounds.x and bounds.y are finite numeric values in addition to the existing width/height checks: ensure typeof bounds.x === "number" && Number.isFinite(bounds.x) and similarly for bounds.y (reject if not), so malformed JSON with NaN/Infinity is treated as invalid and the function returns null; reference the parsed variable bounds and the function resolveWindowsWindowBounds.
🧹 Nitpick comments (3)
electron/ipc/cursor/bounds.test.ts (1)
26-74: ⚡ Quick winConsider expanding test coverage for edge cases.
The current tests validate happy paths effectively, but several edge cases and code paths remain untested:
parseWgcCaptureSizewith zero, negative, or malformed dimensionsnormalizeWindowsWindowBoundsToElectronDipwhen scale factors match (early return path at line 213-220)alignWindowBoundsToWgcCaptureSizewith zero or negative scale factor (tests line 240 guard)resolveWindowsWindowTelemetryBounds,applyWindowsWindowTelemetryBounds, andensureSelectedWindowBoundsReadyare not testedWhile not blocking for this PR, additional test coverage would improve confidence in the coordinate transformation pipeline.
🤖 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 `@electron/ipc/cursor/bounds.test.ts` around lines 26 - 74, Add unit tests covering edge cases noted: for parseWgcCaptureSize add cases with zero, negative, and malformed strings to assert null or proper handling; for normalizeWindowsWindowBoundsToElectronDip add a test where dpi results in scale factor 1 to exercise the early-return path in normalizeWindowsWindowBoundsToElectronDip; for alignWindowBoundsToWgcCaptureSize add tests for zero and negative scale factors to hit the guard at alignWindowBoundsToWgcCaptureSize; and add basic tests exercising resolveWindowsWindowTelemetryBounds, applyWindowsWindowTelemetryBounds, and ensureSelectedWindowBoundsReady to validate telemetry/ready-state behavior and coordinate outputs. Ensure each test references the exact function names above so reviewers can locate the new tests.electron/ipc/cursor/bounds.ts (2)
28-46: 💤 Low valueConsider adding validation for script existence.
If none of the candidates exist, Line 45 returns a non-existent path, leading to a cryptic
ENOENTerror whenexecFileattempts to run the script. While the build process should ensure the script exists, a clearer error message would improve debuggability in edge cases.🛡️ Optional improvement for error clarity
for (const candidate of candidates) { if (existsSync(candidate)) { return candidate; } } + console.warn("PowerShell script not found at any candidate path, using fallback"); return candidates[candidates.length - 1] ?? "get-window-bounds.ps1"; }🤖 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 `@electron/ipc/cursor/bounds.ts` around lines 28 - 46, The resolver function resolveWindowsWindowBoundsScriptPath can return a non-existent path causing a cryptic ENOENT when later executing the script; update resolveWindowsWindowBoundsScriptPath to validate that a candidate was actually found and if none exist throw a clear, descriptive Error (including the list of attempted candidate paths and a hint about expected build output) instead of returning a fallback string, so callers (e.g., code that calls execFile) fail fast with a readable message.
294-305: 💤 Low valuePotential redundant Windows bounds resolution.
Lines 295 and 300-305 both invoke the same bounds-resolution logic on Windows without any delay between attempts.
refreshSelectedWindowBoundscallsresolveWindowsWindowTelemetryBounds, and if that fails to set bounds,applyWindowsWindowTelemetryBoundsimmediately retries the identical operation. Consider whether the second attempt adds value, or if the retry should only happen in the next loop iteration with the 200ms delay.🤖 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 `@electron/ipc/cursor/bounds.ts` around lines 294 - 305, The code redundantly attempts Windows bounds resolution twice in the same loop iteration: refreshSelectedWindowBounds (which calls resolveWindowsWindowTelemetryBounds) then immediately applyWindowsWindowTelemetryBounds; remove the immediate duplicate by deleting the applyWindowsWindowTelemetryBounds call inside the same iteration and let the loop retry in the next iteration (or alternatively, if you must keep it, add an explicit delay before calling applyWindowsWindowTelemetryBounds). Update the logic around refreshSelectedWindowBounds, resolveWindowsWindowTelemetryBounds, applyWindowsWindowTelemetryBounds and checks of selectedWindowBounds/selectedSource so only one resolution attempt happens per loop iteration.
🤖 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 `@electron/ipc/register/recording.ts`:
- Around line 576-578: The code sets nativeVideoRecordingStartedAtMs via
setNativeVideoRecordingStartedAtMs(videoStartedAtMs) before attempting a native
start and does not clear it on failure, allowing the timestamp to leak into the
next session; update the native-start paths (including the branch guarded by
captureTarget.kind === "window" and corresponding failure branches around the
native start calls referenced in the diff) to clear the timestamp on any failure
by calling setNativeVideoRecordingStartedAtMs(undefined) (or a reset helper)
whenever native start fails or is aborted so nativeVideoRecordingStartedAtMs is
only retained for successful native starts.
---
Outside diff comments:
In `@electron/ipc/cursor/bounds.ts`:
- Around line 315-355: In resolveWindowsWindowBounds, after parsing stdout into
bounds (the WindowsWindowBounds object), validate that bounds.x and bounds.y are
finite numeric values in addition to the existing width/height checks: ensure
typeof bounds.x === "number" && Number.isFinite(bounds.x) and similarly for
bounds.y (reject if not), so malformed JSON with NaN/Infinity is treated as
invalid and the function returns null; reference the parsed variable bounds and
the function resolveWindowsWindowBounds.
---
Nitpick comments:
In `@electron/ipc/cursor/bounds.test.ts`:
- Around line 26-74: Add unit tests covering edge cases noted: for
parseWgcCaptureSize add cases with zero, negative, and malformed strings to
assert null or proper handling; for normalizeWindowsWindowBoundsToElectronDip
add a test where dpi results in scale factor 1 to exercise the early-return path
in normalizeWindowsWindowBoundsToElectronDip; for
alignWindowBoundsToWgcCaptureSize add tests for zero and negative scale factors
to hit the guard at alignWindowBoundsToWgcCaptureSize; and add basic tests
exercising resolveWindowsWindowTelemetryBounds,
applyWindowsWindowTelemetryBounds, and ensureSelectedWindowBoundsReady to
validate telemetry/ready-state behavior and coordinate outputs. Ensure each test
references the exact function names above so reviewers can locate the new tests.
In `@electron/ipc/cursor/bounds.ts`:
- Around line 28-46: The resolver function resolveWindowsWindowBoundsScriptPath
can return a non-existent path causing a cryptic ENOENT when later executing the
script; update resolveWindowsWindowBoundsScriptPath to validate that a candidate
was actually found and if none exist throw a clear, descriptive Error (including
the list of attempted candidate paths and a hint about expected build output)
instead of returning a fallback string, so callers (e.g., code that calls
execFile) fail fast with a readable message.
- Around line 294-305: The code redundantly attempts Windows bounds resolution
twice in the same loop iteration: refreshSelectedWindowBounds (which calls
resolveWindowsWindowTelemetryBounds) then immediately
applyWindowsWindowTelemetryBounds; remove the immediate duplicate by deleting
the applyWindowsWindowTelemetryBounds call inside the same iteration and let the
loop retry in the next iteration (or alternatively, if you must keep it, add an
explicit delay before calling applyWindowsWindowTelemetryBounds). Update the
logic around refreshSelectedWindowBounds, resolveWindowsWindowTelemetryBounds,
applyWindowsWindowTelemetryBounds and checks of
selectedWindowBounds/selectedSource so only one resolution attempt happens per
loop iteration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32c85754-627e-4724-bcc8-5258ceb794ca
📒 Files selected for processing (10)
electron/ipc/cursor/bounds.test.tselectron/ipc/cursor/bounds.tselectron/ipc/cursor/get-window-bounds.ps1electron/ipc/cursor/interaction.tselectron/ipc/cursor/telemetry.tselectron/ipc/register/recording.tselectron/ipc/state.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.tsvite.config.ts
💤 Files with no reviewable changes (1)
- electron/ipc/cursor/interaction.ts
…or for 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
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.
Pull Request Template
Description
I'm proposing these fixes for two distinct bugs, both related to recording windowed applications on Windows machines. This fixes the Windows default cursor showing along with the rendered cursor, and also fixes a mouse cursor telemetry issue that was causing the rendered cursor to show in incorrect positions.
Motivation
When recording windowed screens with any dimensions smaller than fullscreen, the windows default cursor would show along side the rendered cursor. In addition, the rendered cursor would be far off from where it actually was during the recording.
Type of Change
Related Issue(s)
#634
Screenshots / Video
Video:
recordly-bugfix.mp4
Testing Guide
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Bug Fixes
Tests