fix(recording): preserve browser cursor after WGC fallback#583
Conversation
|
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)
📝 WalkthroughWalkthroughThe PR adds a BrowserCaptureCursorPolicy and resolveBrowserCaptureCursorPolicy(), tracks native Windows capture start failure, applies the resolved policy in startRecording() to editor/OS cursor hiding and browser media constraints, and adds unit tests for the policy resolver. ChangesCursor-Capture Policy Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 review |
✅ Actions performedReview triggered.
|
![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/hooks/useScreenRecorder.test.ts (1)
112-130: 🏗️ Heavy liftAdd one wiring-level regression test for fallback side effects
These tests validate resolver output, but not that
startRecording()actually applies the fallback policy (skiphideOsCursor()+ request browser cursor capture) when native Windows start fails. A focused mocked-flow test would better lock down the user-facing regression.🤖 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 `@src/hooks/useScreenRecorder.test.ts` around lines 112 - 130, Add a wiring-level regression test that simulates native Windows capture failing to start and verifies startRecording() applies the fallback policy: mock/spy the native capture start path to return failure, spy on hideOsCursor() to assert it is NOT called, and assert that the browser capture path is invoked (e.g., a call that triggers request for browser cursor capture or sets streamCursor to "always"). Use the existing resolver resolveBrowserCaptureCursorPolicy for expected values and exercise the real startRecording() flow so the test ensures the side effects (skip hideOsCursor and request browser cursor capture) occur when nativeWindowsCaptureStartFailed is true.
🤖 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 `@src/hooks/useScreenRecorder.ts`:
- Around line 1545-1550: The current call to window.electronAPI.hideOsCursor
inside the browserCursorPolicy.hideOsCursorBeforeRecording block only catches
thrown errors but ignores a non-throwing failure result; update the logic in
useScreenRecorder to await the Promise result from
window.electronAPI.hideOsCursor(), check the returned shape (e.g., { success:
boolean }), and if success is false log a warning (via console.warn or
processLogger) describing the unsuccessful IPC response; keep the existing
try/catch to handle thrown errors and also include the returned-error-path
logging for window.electronAPI.hideOsCursor to avoid silent failures.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.test.ts`:
- Around line 112-130: Add a wiring-level regression test that simulates native
Windows capture failing to start and verifies startRecording() applies the
fallback policy: mock/spy the native capture start path to return failure, spy
on hideOsCursor() to assert it is NOT called, and assert that the browser
capture path is invoked (e.g., a call that triggers request for browser cursor
capture or sets streamCursor to "always"). Use the existing resolver
resolveBrowserCaptureCursorPolicy for expected values and exercise the real
startRecording() flow so the test ensures the side effects (skip hideOsCursor
and request browser cursor capture) occur when nativeWindowsCaptureStartFailed
is true.
🪄 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: d4f324c4-2ebd-4172-8c7a-13d89161d26b
📒 Files selected for processing (2)
src/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Motivation
Follow-up for #393 and the v1.3.0 Discord report where native Windows capture fails to start, browser fallback records, and the cursor appears left behind or delayed.
Testing
Notes
This does not change successful WGC capture or the native helper. It still needs runtime smoke on a Windows machine where WGC fails and browser fallback is used.
Summary by CodeRabbit
New Features
Tests