feat: Windows RDP sidecar#2
Conversation
Implements the Windows half of the RDP feature per native/rdp-spike/HANDOFF.md. - sidecar.c: _setmode(stdout, _O_BINARY) on Windows so \n isn't rewritten to \r\n and the binary frame stream isn't corrupted. - native/rdp-spike/CMakeLists.txt: static Windows build via vcpkg (x64-windows-static FreeRDP) + static CRT, producing rdp-sidecar.exe. - release.yml: build the Windows sidecar (vcpkg + cmake, cached) before electron-builder; only publish artifacts on a tag push so dispatch builds can verify the sidecars without creating a release. - electron-builder.yml: win.extraResources bundles rdp-sidecar.exe. - verify-sidecar.js: win32 branch (exists + best-effort dumpbin dep scan). - UI: rdpSupported helper (darwin + win32) replaces the darwin-only gate in AddConnectionModal, Sidebar, and Dashboard. Sidecar binary builds on the windows-latest CI runner (no local MSVC). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 10 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR extends the RDP sidecar from macOS-only to also support Windows. It adds a CMake build definition and Windows-specific initialization to ChangesWindows RDP Sidecar End-to-End
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/release.yml:
- Around line 59-67: The Cache vcpkg FreeRDP (Windows) step uses a floating tag
`actions/cache@v4` instead of a pinned commit SHA, and the cache key does not
include the current ref or release information. To fix this, pin the
actions/cache action to a specific commit SHA instead of the floating v4 tag,
and modify the cache key by appending the GitHub ref or release tag (such as
using ${{ github.ref }}) to ensure the cache is scoped to the specific release
and prevent unintended cache reuse across different branches and builds.
In `@native/rdp-spike/sidecar.c`:
- Around line 209-214: The `_setmode(_fileno(stdout), _O_BINARY)` call on line
213 does not check the return value for failure. If this call fails, stdout
remains in text mode, which will corrupt the binary frame stream. Capture the
return value of the `_setmode` call and check if it indicates failure (typically
returns -1), and if so, log an error message and exit the program before any
frames are written to prevent protocol corruption.
In `@scripts/verify-sidecar.js`:
- Around line 64-67: The catch block currently treats all non-prefixed errors as
if dumpbin is unavailable, which masks real dependency-scan failures. Instead of
immediately logging a success message for any error that doesn't start with
'[verify-sidecar]', add logic to distinguish between actual dumpbin
unavailability (check for specific error codes like ENOENT or error messages
containing "command not found") and other legitimate errors. Only log the skip
message for confirmed unavailability cases; throw or properly handle any other
error type to prevent broken artifacts from passing verification.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: adcda091-5695-4794-848f-1186c158075f
📒 Files selected for processing (9)
.github/workflows/release.ymlelectron-builder.ymlnative/rdp-spike/CMakeLists.txtnative/rdp-spike/sidecar.cscripts/verify-sidecar.jssrc/renderer/src/components/ConnectionManager/AddConnectionModal.tsxsrc/renderer/src/components/Dashboard/Dashboard.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/lib/platform.ts
| - name: Cache vcpkg FreeRDP (Windows) | ||
| if: runner.os == 'Windows' | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| C:/vcpkg/installed | ||
| C:/vcpkg/packages | ||
| key: vcpkg-freerdp-x64-windows-static-${{ hashFiles('native/rdp-spike/CMakeLists.txt') }} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify floating action refs in the workflow
rg -nP 'uses:\s*[^@\s]+@v[0-9]+' .github/workflows/release.yml
# Inspect cache usage and key scoping for the Windows vcpkg cache
rg -nP 'actions/cache|key:\s*vcpkg-freerdp' .github/workflows/release.yml -C 2Repository: Meanski/noxed
Length of output: 815
Pin actions/cache and scope cache key to release refs.
Line 61 uses actions/cache@v4 (floating tag instead of pinned commit SHA), and the cache key at line 66 is not scoped to a specific ref or release tag. This allows the same cache to be reused across different builds and branches, increasing cache-poisoning exposure in a release workflow.
Recommendations:
- Pin the action to a specific commit:
actions/cache@<commit-sha>(not a floating major version) - Scope the cache key to the release ref or commit to prevent unintended cache reuse
🧰 Tools
🪛 zizmor (1.25.2)
[error] 61-61: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 61-61: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/release.yml around lines 59 - 67, The Cache vcpkg FreeRDP
(Windows) step uses a floating tag `actions/cache@v4` instead of a pinned commit
SHA, and the cache key does not include the current ref or release information.
To fix this, pin the actions/cache action to a specific commit SHA instead of
the floating v4 tag, and modify the cache key by appending the GitHub ref or
release tag (such as using ${{ github.ref }}) to ensure the cache is scoped to
the specific release and prevent unintended cache reuse across different
branches and builds.
Source: Linters/SAST tools
- sidecar.c: check _setmode return; abort if stdout can't go binary rather than emit a corrupted frame stream. - verify-sidecar.js: only skip the dumpbin dep scan when dumpbin is genuinely absent (ENOENT); rethrow any other error so a real dependency-scan failure can't pass verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying noxed with
|
| Latest commit: |
e2c8e29
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f0f4895.noxed.pages.dev |
| Branch Preview URL: | https://feat-windows-rdp-sidecar.noxed.pages.dev |
|
Addressed CodeRabbit's 3 comments in d96e547:
|
FreeRDP's vcpkg config references transitive static deps (cjson) by bare library name, so the link failed with LNK1181: cannot open 'cjson.lib'. Add the vcpkg static lib dir to the linker search path. Also key the vcpkg cache on a stable marker instead of the CMakeLists hash so editing CMake doesn't trigger a full FreeRDP rebuild. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add actions/upload-artifact so branch/dispatch builds expose the installers (and the standalone sidecar) on the run itself — downloadable for testing without cutting a release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On Windows, getaddrinfo fails unless WSAStartup has been called, so freerdp_connect reported ERRCONNECT_DNS_NAME_NOT_FOUND (0x00020005) even for valid hosts. FreeRDP's own Windows client calls WSAStartup in its global init; mirror that — WSAStartup before connect, WSACleanup at exit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Internal RDP hosts almost always use self-signed certificates, so the TLS handshake failed with ERRCONNECT_TLS_CONNECT_FAILED (0x00020008). Set IgnoreCertificate + AutoAcceptCertificate so this view-only tool accepts the cert instead of rejecting the connection (mstsc just prompts the user to trust it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Connect at the actual pane size (even-aligned, min 640x480) rather than a fixed 1280x800, and let the canvas scale to fill its container via width/height 100% + object-fit contain (previously maxWidth/maxHeight only scaled down, so a larger window left the desktop small + centered). Live re-negotiation on resize is still TODO (FreeRDP DisplayControl). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The desktop was view-only. Now input events from the canvas are streamed to the sidecar over stdin and replayed via FreeRDP's input API. - sidecar.c: a WinPR reader thread reads fixed 8-byte input messages from stdin into a lock-guarded ring queue and signals an event added to the main wait loop, which drains it and calls freerdp_input_send_mouse/ keyboard/unicode_event on the (single-threaded) transport. stdin is set to binary mode on Windows so messages aren't mangled. - rdp.ts: keep stdin open after the password; add the rdp:input channel that encodes events to 8-byte messages. - preload: rdp.input() fire-and-forget send. - renderer: capture mouse (move/buttons/wheel, coords mapped through the object-fit letterbox) and keyboard (event.code -> RDP set-1 scancode in lib/rdpKeymap.ts) on the canvas. Key mapping lives in JS so the sidecar stays generic. Live resize re-negotiation (FreeRDP DisplayControl) is still TODO. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Implements the Windows half of the RDP feature, per `native/rdp-spike/HANDOFF.md`.
What's in here
Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Bug Fixes