refactor: Migrate the powershell cursor script into native cursor sampler#621
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwaps Windows cursor sampling from dynamically-generated PowerShell scripts to a compiled C++ executable. The bridge now resolves and spawns ChangesPowerShell to C++ Cursor Sampler Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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. 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 |
![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: 49ee3ac0db
ℹ️ 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".
![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 (2)
electron/native/wgc-capture/CMakeLists.txt (1)
53-67: 💤 Low valuenit: missing
WIN32_LEAN_AND_MEANfor consistency
wgc-capturedefinesWIN32_LEAN_AND_MEANbutcursor-samplerdoesn't. not a big deal for a small target, but it does slightly reduce include bloat and keeps the two targets consistent.also lowkey nice to avoid accidentally pulling in winsock1 vs winsock2 issues if networking ever gets added.
🧹 optional diff
target_compile_definitions(cursor-sampler PRIVATE NOMINMAX + WIN32_LEAN_AND_MEAN _WIN32_WINNT=0x0A00 )🤖 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/native/wgc-capture/CMakeLists.txt` around lines 53 - 67, Add WIN32_LEAN_AND_MEAN to the cursor-sampler target's compile definitions to match wgc-capture and reduce Windows header bloat: update the target_compile_definitions call for the cursor-sampler executable (symbol: cursor-sampler) to include WIN32_LEAN_AND_MEAN alongside NOMINMAX and _WIN32_WINNT=0x0A00 so the preprocessor define is applied consistently.electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts (1)
18-30: 💤 Low valueheads up: arm64 windows won't find the helper in bin dir
the build script only produces
win32-x64artifacts, soarchTag = 'win32-arm64'will look for a path that doesn't exist. users would need to setOPENSCREEN_CURSOR_SAMPLER_EXEmanually or rely on the build dir fallback.probably fine if arm64 isn't officially supported (or x64 emulation handles it), but might be worth a comment noting the limitation.
🤖 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/native-bridge/cursor/recording/windowsNativeRecordingSession.ts` around lines 18 - 30, getCursorSamplerCandidates currently sets archTag to "win32-arm64" for arm64 Windows but the build only produces "win32-x64" artifacts; update the function to handle this by falling back to the x64 bin path when the arm64 artifact is missing (e.g., set archTag to "win32-x64" for arm64 or check the resolved path exists and try the x64 variant), and add a brief comment explaining the limitation; reference getCursorSamplerCandidates, the archTag variable, and OPENSCREEN_CURSOR_SAMPLER_EXE when making the change.
🤖 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/native/wgc-capture/src/cursor-sampler.cpp`:
- Around line 475-477: sampler.detach() races with teardown
(UnhookWindowsHookEx(g_mouseHook) and Gdiplus::GdiplusShutdown), so replace the
detach with an explicit stop signalling and join: add/use a thread-safe stop
flag or a sampler.request_stop() method, signal it before teardown, then call
sampler.join() (or the thread's join) to wait for the sampler loop to exit; only
after the join return should you call UnhookWindowsHookEx(g_mouseHook) and
Gdiplus::GdiplusShutdown(gdipToken) to ensure no code is running while
hooks/GDI+ are torn down.
- Line 416: The code uses std::max to compute intervalMs (const int intervalMs =
std::max(1, std::atoi(argv[1]));) but the file lacks an explicit `#include`
<algorithm>, which can break compilation (especially with NOMINMAX); add an
`#include` <algorithm> to the includes at the top of cursor-sampler.cpp so
std::max is defined and unambiguous.
---
Nitpick comments:
In `@electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts`:
- Around line 18-30: getCursorSamplerCandidates currently sets archTag to
"win32-arm64" for arm64 Windows but the build only produces "win32-x64"
artifacts; update the function to handle this by falling back to the x64 bin
path when the arm64 artifact is missing (e.g., set archTag to "win32-x64" for
arm64 or check the resolved path exists and try the x64 variant), and add a
brief comment explaining the limitation; reference getCursorSamplerCandidates,
the archTag variable, and OPENSCREEN_CURSOR_SAMPLER_EXE when making the change.
In `@electron/native/wgc-capture/CMakeLists.txt`:
- Around line 53-67: Add WIN32_LEAN_AND_MEAN to the cursor-sampler target's
compile definitions to match wgc-capture and reduce Windows header bloat: update
the target_compile_definitions call for the cursor-sampler executable (symbol:
cursor-sampler) to include WIN32_LEAN_AND_MEAN alongside NOMINMAX and
_WIN32_WINNT=0x0A00 so the preprocessor define is applied consistently.
🪄 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: CHILL
Plan: Pro
Run ID: d7343f8c-0613-407b-837f-3640feeeee3f
📒 Files selected for processing (5)
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native/wgc-capture/CMakeLists.txtelectron/native/wgc-capture/src/cursor-sampler.cppscripts/build-windows-wgc-helper.mjs
💤 Files with no reviewable changes (1)
- electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.ts
2 participants
Description
Removed the powershell script that handles the cursor and implemented a native version with the same functionality and output patterns.
Motivation
About the issue about antivirus flags, it is necessary to find a different approach, because the cursors "recording" method via the powershell is often used by spyware.
Type of Change
Related Issue(s)
#620
Screenshots / Video
This update does not include any visual change.
The cursor editor works like before.
Checklist
Summary by CodeRabbit