fix(windows): repair first-run hardware access, land screen capture, fix the capture pipeline#124
Merged
Merged
Conversation
The 0.2.1 installer ran PawnIO's setup but skipped the HypercolorSmBus broker, and the broker is what loads PawnIO modules on behalf of the unelevated daemon. Without it CpuTempReader::new() fails its probe read, so cpu_temp_celsius stays null forever: motherboard and DRAM SMBus lighting never appear, and any display face bound to cpu_temp renders a flat zero. Verified on a clean 0.2.1 install: PawnIO service running, modules deployed, HypercolorSmBus absent, /api/v1/system/sensors reporting cpu_temp_celsius null next to a working gpu_temp_celsius. Install time is the only moment Hypercolor holds administrator rights. Deferring broker setup meant either an out-of-nowhere UAC prompt later or silently broken hardware, and in practice it was the latter, because the only remaining path was a Hardware Support panel gated behind motherboard detection. Run the existing hardware-support orchestrator from the postinstall hook instead, which installs PawnIO, stages the verified module blobs, then registers and starts the broker in one elevated pass. Every path handed to the orchestrator sits under $INSTDIR, so the broker installer's rejection of user-writable service paths still holds; that guard, not the deferral, is what keeps a LocalSystem service from loading user-rewritable code. A failed pass no longer implies a failed install: USB and network lighting work without any of this, and the details log now says so. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Splatting an array into a PowerShell script binds every element
positionally, so @("-AssetRoot", $path, ...) handed the child script the
literal string "-AssetRoot" as its first parameter, shifted every real
value one slot left, and dropped -Silent, -Force, -Reinstall and -Start
entirely. Only native executables parse "-Name value" pairs out of a
splatted array; scripts need a hashtable. The other splat sites in
scripts/ all target real executables and are fine as they are.
The blast radius was the whole Windows hardware-access story. The
orchestrator is what the app's Settings > Discovery > "Install support"
button runs, and it threw before reaching the broker every single time,
which is why an install could never be repaired from inside the app.
PawnIO itself survived only because the NSIS hook called its installer
directly with real named parameters.
Verified by running the fixed orchestrator elevated against a stock
0.2.1 install: PawnIO reported ready, HypercolorSmBus registered and
started, and the running daemon picked up two previously invisible ASUS
Aura DRAM devices over SMBus without needing a restart.
Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
WindowsSensorExtras opened the PawnIO reader once at construction and cached the failure for the daemon's lifetime, so cpu_temp_celsius stayed null until someone restarted the daemon. That is precisely backwards from how the broker actually arrives: the installer registers it while an upgrade's daemon is already running, PawnIO's kernel driver can need a reboot before it binds to SCM, and Settings can install hardware support at any moment. Confirmed on this host, where starting the broker brought SMBus DRAM devices online immediately while CPU temperature stayed dark. Retry on a 20 second backoff while the reader is absent, and drop the reader on a failed read so a stopped or crashed broker heals once SCM restarts it instead of staying dead. The all-sources-absent early return is gone: it would have made the retry unreachable on exactly the hosts that need it, and merge_snapshot already no-ops when every source is missing. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Whitespace only. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Screen capture on Windows had no backend at all. The only implementation was WaylandScreenCaptureInput, and build_input_manager registered a screen source solely under cfg(target_os = "linux"), so toggling capture.enabled on Windows did nothing whatsoever and every screen-reactive effect rendered against an empty input. Desktop Duplication is the right primitive here. It is the compositor's own presented output, it costs nothing while the screen is static, and unlike Windows.Graphics.Capture it draws no yellow border and shows no picker. It also needs no permission grant: Windows has no equivalent of the XDG portal handshake or macOS TCC prompt, so a screen-reactive effect can simply work with nothing to consent to. This lands the FFI layer only; wiring into the input manager follows. It is a separate crate because the workspace forbids unsafe_code and COM interop cannot, matching hypercolor-windows-pawnio and hypercolor-windows-gpu-interop. Readback subsamples by an integer stride instead of copying native resolution. Ambient lighting reduces the frame to a coarse sector grid, so point sampling is indistinguishable in the output while letting the loop skip most mapped staging rows, which matters when a 4K desktop is 33 MB per frame of write-combined memory. Frames whose LastPresentTime is zero carry only a pointer move and are dropped before readback, and access loss from mode changes, full-screen takeover, or the UAC secure desktop rebuilds the duplication interface in place. Verified on a 3840x2160 display: the live smoke tests captured real frames at 1280x720, tightly packed and fully opaque, across repeated acquisitions. The tests degrade to skips on hosts without a duplicatable output so the Linux-only CI stays green. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Registers WindowsScreenCaptureInput so Desktop Duplication actually reaches the render pipeline. A worker thread owns the capture session and the analysis pipeline and publishes ScreenData snapshots, mirroring the Wayland source's shape; the render loop only clones the latest snapshot. The duplication interface opens on demand and is released the moment capture goes idle, because Windows allows one duplication per output per process and other ambient-lighting tools want it too. capture.enabled now defaults to true on Windows and stays false elsewhere. This is a privacy-relevant default, so the reasoning matters: Desktop Duplication has no permission prompt, no source picker, and no capture indicator, meaning there is nothing for a user to consent to and nothing an opt-in toggle would be protecting. The XDG portal opens a picker and macOS raises a TCC prompt, and forcing either at daemon start would be an ambush, so both stay opt-in. Enabling only grants permission: capture stays closed until a screen-reactive effect creates demand, so the common case is still no capture at all. Multi-monitor selection reuses the existing free-form capture.source field, which the portal leaves at "auto" on Linux. Windows addresses outputs directly and accepts "monitor:N", "display:N", or a bare index, falling back to the primary display for anything else so an unknown value is never an error. Verified: clippy -D warnings clean on types, core, the capture crate and the daemon; 36 types config tests, 43 core screen tests, and 9 capture crate tests pass, including a live capture off a 3840x2160 display. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
A second Rust on PATH fails late and selectively. Cached crates check fine, so a build looks healthy right up until something needs a fresh link and dies inside a dependency's build script with "cannot execute 'ld'", or hits E0514 rustc-mismatch errors against artifacts the pinned toolchain left in target/. Neither symptom names the real cause, and rustup show happily reports the pinned MSVC toolchain while a different compiler is doing the work. Chocolatey's `rust` package is the usual culprit: it installs a GNU-host toolchain under C:\ProgramData\chocolatey\bin, which lives in the system PATH and therefore beats ~\.cargo\bin in the user PATH. Passing rustup's cargo.exe by absolute path does not help either, since cargo shells out to whichever rustc PATH resolves. Assert the host triple before entering the build, and report the resolved rustc path plus the removal command. Costs one process spawn against a class of failure that reads as a broken repo. Verified both directions: the wrapper builds clean with no PATH workaround, and a GNU-host rustc placed ahead on PATH aborts with the explanatory error instead of reaching cargo. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
CI has been red on main since 2026-07-21 with a single lint, clippy::collapsible_match, fired by the REL_WHEEL arm in the evdev reader. It failed three jobs because the same code compiles as both lib and lib test. The lint never showed up locally on Windows because evdev.rs is behind cfg(target_os = "linux"), so Windows contributors get a clean clippy run while Linux-only code rots. The inverse of this trap already bit us the other way around. Fold the guard into a match guard as clippy suggests. Behaviour is identical: a REL_WHEEL event on a hi-res device previously entered the arm and did nothing, and now falls through to the catch-all instead. Not verified locally: evdev.rs cannot compile on Windows, and cross- checking against x86_64-unknown-linux-gnu fails in aws-lc-sys, which needs a Linux C toolchain. CI is the verifier here. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The readback picked one pixel per stride block. I justified that on the grounds that ambilight averages the result into a coarse sector grid anyway, which was the wrong call: the same buffer is published as canvas_downscale and consumed as an actual image by screen-reactive effects, which then downscale it a second time. Two successive point samplings of a 4K desktop shred thin text into aliased noise, and screen-reactive effects looked correspondingly bad. The Wayland path never hit this because PipeWire delivers an already-filtered frame, so the pipeline only ever downscaled once from something clean. Windows gets the raw desktop and has to do that filtering itself. Average every source pixel in the block instead. Reads scale from one pixel per block to all of them, which is the cost of the frame actually being correct; the loop stays row-major so the reads are sequential through mapped staging memory. Also adds two dump examples, which is how the above was diagnosed rather than guessed at. Averaged zone colors hide wrong row pitches, swapped channels, and off-by-one strides equally well, so writing the frame out is the only way to see what the pipeline really has: dump_frame for the raw backend output, dump_screen_canvas for the published surface that effects consume. Verified by comparing dumps before and after against the same desktop. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Windows DXGI Desktop Duplication backend, integrates it into core, daemon, and UI capture flows, enables Windows-specific defaults and sensor recovery, and updates hardware installation, documentation, tests, examples, and build-toolchain validation. ChangesWindows support
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CaptureSection
participant CaptureAPI
participant Daemon
participant WindowsScreenCaptureInput
participant DesktopDuplicator
CaptureSection->>CaptureAPI: Fetch capture monitors
CaptureAPI->>Daemon: GET /api/v1/capture/monitors
Daemon-->>CaptureSection: Return monitor metadata
WindowsScreenCaptureInput->>DesktopDuplicator: Open selected monitor
DesktopDuplicator-->>WindowsScreenCaptureInput: Return processed frame
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 5
🧹 Nitpick comments (3)
crates/hypercolor-core/src/input/screen/windows.rs (1)
160-168: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGeneration is bumped even when the config lock is poisoned.
Line 167 sits outside the
if let, so a poisoned mutex makes everyreconfigurecall bump the generation without storing anything; the worker then rebuilds its analyzer fromCaptureConfig::default()(viasnapshot's poison fallback) on each call. Moving the bump inside the successful branch keeps the no-op case honest.🤖 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 `@crates/hypercolor-core/src/input/screen/windows.rs` around lines 160 - 168, Update reconfigure so settings.generation.fetch_add is executed only after successfully locking and updating self.settings.config, inside the if let Ok branch. Preserve the early return when the existing configuration matches, and do not bump the generation when the mutex is poisoned.crates/hypercolor-windows-capture/src/duplication.rs (1)
328-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent
unwrap_or_default()onCPUAccessFlagsdegrades into an unmappable staging texture.If the conversion ever failed, the flags become
0, texture creation succeeds, and the failure only surfaces later as an opaque "map staging texture" error.D3D11_CPU_ACCESS_READ.0is a positive constant, so consider making the intent explicit instead of a default fallback.♻️ Proposed change
- CPUAccessFlags: u32::try_from(D3D11_CPU_ACCESS_READ.0).unwrap_or_default(), + CPUAccessFlags: D3D11_CPU_ACCESS_READ.0.cast_unsigned(),(or
#[expect(clippy::cast_sign_loss)] D3D11_CPU_ACCESS_READ.0 as u32ifcast_unsignedis unavailable on the pinned toolchain)🤖 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 `@crates/hypercolor-windows-capture/src/duplication.rs` around lines 328 - 334, Update the staging_desc construction in the texture duplication flow to convert D3D11_CPU_ACCESS_READ.0 explicitly to u32 without unwrap_or_default(), preserving the required read CPUAccessFlags and avoiding a silent zero fallback. Use try_from with an explicit failure strategy or the reviewed cast approach, depending on the pinned toolchain.crates/hypercolor-windows-capture/tests/duplication_tests.rs (1)
121-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis is the only test that fails rather than skips on a capture error.
duplicator_or_skipandwait_for_frameboth treat runtime capture errors as "skip", but line 130 panics. A resolution change or a fullscreen app grabbing the output mid-run will red the suite on a real Windows host. Consider matching the skip policy used elsewhere.♻️ Proposed change
- Err(error) => panic!("repeated acquisition failed: {error}"), + Err(error) => { + eprintln!("skipping: repeated acquisition failed ({error})"); + return; + }🤖 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 `@crates/hypercolor-windows-capture/tests/duplication_tests.rs` around lines 121 - 131, Update the Err branch in the repeated frame-acquisition loop using duplicator.next_frame so runtime capture errors follow the same skip behavior as duplicator_or_skip and wait_for_frame instead of panicking. Preserve successful frame validation and the existing handling of Ok(None), and exit or skip the test cleanly when an acquisition error occurs.
🤖 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 `@crates/hypercolor-core/src/input/screen/windows.rs`:
- Around line 302-311: Replace both blocking `thread::sleep(REOPEN_BACKOFF)`
calls in the Windows capture retry paths with an interruptible wait on the
command channel, so receiving `Stop` exits immediately while the backoff still
elapses before retrying. Update the surrounding loop and channel handling to
preserve normal reopen behavior and ensure `shutdown_worker` can complete
without waiting for the full backoff.
- Around line 280-311: Update the generation-change branch in the screen capture
worker so a changed config.monitor invalidates the existing duplicator before
the session-selection match. Drop or recreate the active duplicator when the
monitor changes, while preserving the current analyzer refresh and max-width
configuration behavior; this ensures DesktopDuplicator::new uses the newly
selected monitor immediately.
In `@crates/hypercolor-daemon/src/startup/services.rs`:
- Line 695: Update WindowsScreenCaptureInput::run_worker to detect changes to
config.monitor and drop/recreate the existing DesktopDuplicator when the
selected monitor changes, alongside the generation-based refresh logic. Ensure
subsequent capture samples use the newly selected display while preserving
existing analyzer and max-width updates.
In `@crates/hypercolor-windows-capture/Cargo.toml`:
- Around line 11-12: Remove the unsafe_code = "allow" exception from the
[lints.rust] configuration in Cargo.toml, then update the DXGI/COM
implementation to compile without any unsafe Rust and ensure no unsafe_code
allowance remains anywhere in the workspace.
In `@docs/specs/14-screen-capture.md`:
- Around line 1628-1632: Update the “Subsample during readback” section in
14-screen-capture.md to describe box-filtered readback rather than
integer-stride point sampling. Align the wording with the implemented Windows
capture behavior while preserving the mapped staging-memory readback and
approximate 1280px target details.
---
Nitpick comments:
In `@crates/hypercolor-core/src/input/screen/windows.rs`:
- Around line 160-168: Update reconfigure so settings.generation.fetch_add is
executed only after successfully locking and updating self.settings.config,
inside the if let Ok branch. Preserve the early return when the existing
configuration matches, and do not bump the generation when the mutex is
poisoned.
In `@crates/hypercolor-windows-capture/src/duplication.rs`:
- Around line 328-334: Update the staging_desc construction in the texture
duplication flow to convert D3D11_CPU_ACCESS_READ.0 explicitly to u32 without
unwrap_or_default(), preserving the required read CPUAccessFlags and avoiding a
silent zero fallback. Use try_from with an explicit failure strategy or the
reviewed cast approach, depending on the pinned toolchain.
In `@crates/hypercolor-windows-capture/tests/duplication_tests.rs`:
- Around line 121-131: Update the Err branch in the repeated frame-acquisition
loop using duplicator.next_frame so runtime capture errors follow the same skip
behavior as duplicator_or_skip and wait_for_frame instead of panicking. Preserve
successful frame validation and the existing handling of Ok(None), and exit or
skip the test cleanly when an acquisition error occurs.
🪄 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 Plus
Run ID: 6b63c6e1-4d2f-44f5-bdbf-527ac8691332
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
crates/hypercolor-app/installer-hooks.nshcrates/hypercolor-app/tests/packaging_tests.rscrates/hypercolor-core/Cargo.tomlcrates/hypercolor-core/examples/dump_screen_canvas.rscrates/hypercolor-core/src/input/evdev.rscrates/hypercolor-core/src/input/screen/mod.rscrates/hypercolor-core/src/input/screen/windows.rscrates/hypercolor-core/src/input/sensor.rscrates/hypercolor-core/tests/screen_tests.rscrates/hypercolor-daemon/src/startup/services.rscrates/hypercolor-types/src/config.rscrates/hypercolor-types/tests/config_tests.rscrates/hypercolor-windows-capture/Cargo.tomlcrates/hypercolor-windows-capture/examples/dump_frame.rscrates/hypercolor-windows-capture/src/duplication.rscrates/hypercolor-windows-capture/src/lib.rscrates/hypercolor-windows-capture/src/shared.rscrates/hypercolor-windows-capture/src/stubs.rscrates/hypercolor-windows-capture/tests/duplication_tests.rscrates/hypercolor-windows-capture/tests/subsample_tests.rsdocs/specs/14-screen-capture.mdscripts/cargo-cache-build.ps1scripts/install-windows-hardware-support.ps1
Comment on lines
+280
to
+311
| let latest_generation = settings.generation.load(Ordering::Acquire); | ||
| if latest_generation != generation { | ||
| generation = latest_generation; | ||
| config = settings.snapshot(); | ||
| analyzer = ScreenCaptureInput::new(config.clone()); | ||
| if let Some(duplicator) = duplicator.as_mut() { | ||
| duplicator.set_max_width(CAPTURE_TARGET_WIDTH); | ||
| } | ||
| } | ||
|
|
||
| let session = match duplicator.as_mut() { | ||
| Some(session) => session, | ||
| None => match DesktopDuplicator::new(config.monitor, CAPTURE_TARGET_WIDTH) { | ||
| Ok(session) => { | ||
| let (width, height) = session.native_extent(); | ||
| info!( | ||
| monitor = config.monitor, | ||
| width, height, "Windows screen capture online" | ||
| ); | ||
| open_failure_logged = false; | ||
| duplicator.insert(session) | ||
| } | ||
| Err(error) => { | ||
| if !open_failure_logged { | ||
| log_open_failure(&error); | ||
| open_failure_logged = true; | ||
| } | ||
| thread::sleep(REOPEN_BACKOFF); | ||
| continue; | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Changing config.monitor never takes effect on a live session.
On a generation bump the worker refreshes config and the analyzer, but only calls set_max_width(CAPTURE_TARGET_WIDTH) — a constant, so that call is a no-op. DesktopDuplicator::new(config.monitor, …) runs only when duplicator is None, so an active session stays bound to the previously selected output until an unrelated failure or a deactivate/reactivate cycle. Given monitor selection is a headline of this PR, the duplicator should be dropped when the selected monitor changes.
🐛 Proposed fix
let latest_generation = settings.generation.load(Ordering::Acquire);
if latest_generation != generation {
generation = latest_generation;
+ let previous_monitor = config.monitor;
config = settings.snapshot();
analyzer = ScreenCaptureInput::new(config.clone());
- if let Some(duplicator) = duplicator.as_mut() {
- duplicator.set_max_width(CAPTURE_TARGET_WIDTH);
+ if config.monitor != previous_monitor {
+ // Rebind on the next iteration; the session is tied to one output.
+ duplicator = None;
+ } else if let Some(duplicator) = duplicator.as_mut() {
+ duplicator.set_max_width(CAPTURE_TARGET_WIDTH);
}
}📝 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
| let latest_generation = settings.generation.load(Ordering::Acquire); | |
| if latest_generation != generation { | |
| generation = latest_generation; | |
| config = settings.snapshot(); | |
| analyzer = ScreenCaptureInput::new(config.clone()); | |
| if let Some(duplicator) = duplicator.as_mut() { | |
| duplicator.set_max_width(CAPTURE_TARGET_WIDTH); | |
| } | |
| } | |
| let session = match duplicator.as_mut() { | |
| Some(session) => session, | |
| None => match DesktopDuplicator::new(config.monitor, CAPTURE_TARGET_WIDTH) { | |
| Ok(session) => { | |
| let (width, height) = session.native_extent(); | |
| info!( | |
| monitor = config.monitor, | |
| width, height, "Windows screen capture online" | |
| ); | |
| open_failure_logged = false; | |
| duplicator.insert(session) | |
| } | |
| Err(error) => { | |
| if !open_failure_logged { | |
| log_open_failure(&error); | |
| open_failure_logged = true; | |
| } | |
| thread::sleep(REOPEN_BACKOFF); | |
| continue; | |
| } | |
| }, | |
| }; | |
| let latest_generation = settings.generation.load(Ordering::Acquire); | |
| if latest_generation != generation { | |
| generation = latest_generation; | |
| let previous_monitor = config.monitor; | |
| config = settings.snapshot(); | |
| analyzer = ScreenCaptureInput::new(config.clone()); | |
| if config.monitor != previous_monitor { | |
| // Rebind on the next iteration; the session is tied to one output. | |
| duplicator = None; | |
| } else if let Some(duplicator) = duplicator.as_mut() { | |
| duplicator.set_max_width(CAPTURE_TARGET_WIDTH); | |
| } | |
| } | |
| let session = match duplicator.as_mut() { | |
| Some(session) => session, | |
| None => match DesktopDuplicator::new(config.monitor, CAPTURE_TARGET_WIDTH) { | |
| Ok(session) => { | |
| let (width, height) = session.native_extent(); | |
| info!( | |
| monitor = config.monitor, | |
| width, height, "Windows screen capture online" | |
| ); | |
| open_failure_logged = false; | |
| duplicator.insert(session) | |
| } | |
| Err(error) => { | |
| if !open_failure_logged { | |
| log_open_failure(&error); | |
| open_failure_logged = true; | |
| } | |
| thread::sleep(REOPEN_BACKOFF); | |
| continue; | |
| } | |
| }, | |
| }; |
🤖 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 `@crates/hypercolor-core/src/input/screen/windows.rs` around lines 280 - 311,
Update the generation-change branch in the screen capture worker so a changed
config.monitor invalidates the existing duplicator before the session-selection
match. Drop or recreate the active duplicator when the monitor changes, while
preserving the current analyzer refresh and max-width configuration behavior;
this ensures DesktopDuplicator::new uses the newly selected monitor immediately.
Comment on lines
+302
to
+311
| Err(error) => { | ||
| if !open_failure_logged { | ||
| log_open_failure(&error); | ||
| open_failure_logged = true; | ||
| } | ||
| thread::sleep(REOPEN_BACKOFF); | ||
| continue; | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Blocking thread::sleep(REOPEN_BACKOFF) delays stop()/drop() by up to 2 seconds.
Neither sleep observes cancel or the command channel, so shutdown_worker (and therefore Drop for WindowsScreenCaptureInput) can block the caller for the full backoff — and it is reached on every iteration while another application holds the duplication interface, which is the documented common case. Wait on the channel instead so a Stop interrupts the backoff.
🐛 Proposed fix
+/// Sleep for `backoff`, returning early when a stop is requested.
+fn backoff_or_stop(command_rx: &mpsc::Receiver<WorkerCommand>, active: &mut bool) -> ControlFlow {
+ match command_rx.recv_timeout(REOPEN_BACKOFF) {
+ Ok(WorkerCommand::SetActive(next)) => {
+ *active = next;
+ ControlFlow::Continue
+ }
+ Ok(WorkerCommand::Stop) | Err(mpsc::RecvTimeoutError::Disconnected) => ControlFlow::Stop,
+ Err(mpsc::RecvTimeoutError::Timeout) => ControlFlow::Continue,
+ }
+}Then at both call sites:
- thread::sleep(REOPEN_BACKOFF);
- continue;
+ match backoff_or_stop(command_rx, &mut active) {
+ ControlFlow::Stop => break,
+ ControlFlow::Continue => continue,
+ } duplicator = None;
- thread::sleep(REOPEN_BACKOFF);
+ if matches!(backoff_or_stop(command_rx, &mut active), ControlFlow::Stop) {
+ break;
+ }Also applies to: 324-329
🤖 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 `@crates/hypercolor-core/src/input/screen/windows.rs` around lines 302 - 311,
Replace both blocking `thread::sleep(REOPEN_BACKOFF)` calls in the Windows
capture retry paths with an interruptible wait on the command channel, so
receiving `Stop` exits immediately while the backoff still elapses before
retrying. Update the surrounding loop and channel handling to preserve normal
reopen behavior and ensure `shutdown_worker` can complete without waiting for
the full backoff.
| } | ||
| .clamped(), | ||
| restore_token: capture.restore_token.clone(), | ||
| monitor: hypercolor_core::input::screen::monitor_index_from_source(&capture.source), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reopen the Windows session when the selected monitor changes.
Line 695 feeds monitor into the live worker configuration, but WindowsScreenCaptureInput::run_worker only refreshes the analyzer and max width when a generation changes; it retains the existing DesktopDuplicator. Changing capture.source while capture is active therefore continues sampling the old display. Drop and recreate the duplicator when config.monitor changes.
🤖 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 `@crates/hypercolor-daemon/src/startup/services.rs` at line 695, Update
WindowsScreenCaptureInput::run_worker to detect changes to config.monitor and
drop/recreate the existing DesktopDuplicator when the selected monitor changes,
alongside the generation-based refresh logic. Ensure subsequent capture samples
use the newly selected display while preserving existing analyzer and max-width
updates.
Comment on lines
+11
to
+12
| [lints.rust] | ||
| unsafe_code = "allow" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Enforce the workspace prohibition on unsafe Rust.
Line 12 explicitly sets unsafe_code = "allow", permitting the DXGI/COM implementation to contain unsafe code even though the repository rule forbids it everywhere. Remove this exception and resolve the implementation accordingly before merging.
As per coding guidelines, **/*.rs: Do not use unsafe_code anywhere in the workspace.
🤖 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 `@crates/hypercolor-windows-capture/Cargo.toml` around lines 11 - 12, Remove
the unsafe_code = "allow" exception from the [lints.rust] configuration in
Cargo.toml, then update the DXGI/COM implementation to compile without any
unsafe Rust and ensure no unsafe_code allowance remains anywhere in the
workspace.
Source: Coding guidelines
Comment on lines
+1628
to
+1632
| - **Subsample during readback.** Frames are point-sampled by an integer stride | ||
| to roughly 1280px wide while being copied out of mapped staging memory, | ||
| which matches what PipeWire is asked to deliver on Linux. Sampling after a | ||
| full-resolution copy would move 33 MB per frame on a 4K desktop for a result | ||
| the sector grid cannot distinguish. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document box-filtered readback, not point sampling.
The PR objective states that Windows capture uses box-filtered readback, but this section describes integer-stride point sampling. Update the wording to match the implemented filtering behavior so the specification does not mislead future implementations.
Suggested wording
-Frames are point-sampled by an integer stride
+Frames are box-filtered over an integer stride📝 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
| - **Subsample during readback.** Frames are point-sampled by an integer stride | |
| to roughly 1280px wide while being copied out of mapped staging memory, | |
| which matches what PipeWire is asked to deliver on Linux. Sampling after a | |
| full-resolution copy would move 33 MB per frame on a 4K desktop for a result | |
| the sector grid cannot distinguish. | |
| - **Subsample during readback.** Frames are box-filtered over an integer stride | |
| to roughly 1280px wide while being copied out of mapped staging memory, | |
| which matches what PipeWire is asked to deliver on Linux. Sampling after a | |
| full-resolution copy would move 33 MB per frame on a 4K desktop for a result | |
| the sector grid cannot distinguish. |
🤖 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 `@docs/specs/14-screen-capture.md` around lines 1628 - 1632, Update the
“Subsample during readback” section in 14-screen-capture.md to describe
box-filtered readback rather than integer-stride point sampling. Align the
wording with the implemented Windows capture behavior while preserving the
mapped staging-memory readback and approximate 1280px target details.
Ambilight rendered garbage from a real desktop because the letterbox detector reported bars from all four edges at once. Measured against a live 3840x2160 capture, an 8x6 grid came back as top=6 bottom=6 left=8 right=8: every row and every column classified as a black bar, so cropping removed the entire picture, and because the verdict moves with ordinary content changes it strobed frame to frame. The cause is that pixel_luminance is linear while the threshold reads like an sRGB one. Linear luminance punishes dark content brutally: sRGB 30/255 is 0.013 linear, well under the 0.02 default. A dark-themed desktop is therefore "black" from every edge, which is exactly the content ambient lighting is most often pointed at. Two changes. Detection now discards an axis whose bars would swallow the whole grid, because real letterboxing always leaves something in the middle and a uniformly dark frame is dark rather than letterboxed. And letterbox defaults to off, since ambient lighting almost always mirrors a desktop rather than a letterboxed film; the guard alone still left two of eight columns cropped on the same desktop, quietly discarding a quarter of the screen. An existing test asserted the old degenerate path, where an all-black frame produced bars that consumed everything. That premise is gone, so it now asserts no bars are reported; crop_letterbox's own refusal to leave nothing behind is still covered, driven by explicit bars rather than by detection. Verified on the same live desktop: bars went from [6,6,8,8] to [0,0,0,2], and to none with the default off. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
CI runs cargo with --locked, so the image dev-dependency added to hypercolor-windows-capture for the frame-dump examples failed every Rust job in seconds with "cannot update the lock file". Local builds hid it by updating the lock in place and leaving it uncommitted. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Selecting a bundled preset never stuck. The dropdown snapped back to whatever was already applied, so Screen Cast in particular was pinned to whichever preset its layer happened to be created with — Center Zoom, whose 40% viewport crop is why the capture looked like a blurry mess. Saved presets and bundled presets took different routes. Saved presets go through apply_preset, which targets the focused zone. Bundled presets went through update_controls, which PATCHes the legacy global /effects/current/controls. There is no legacy "current effect" in a zone scene, so that 404s, and the error branch restores the previous selection — the snap-back was the failure handler doing its job on a request that could never succeed. Send them the same place a manual control edit already goes: the zone's synthetic legacy layer, whose group and layer ids are both the zone id. The legacy endpoint stays as the fallback for when no zone scene is active, which is the only case it was ever right for. Verified: cargo check and clippy -D warnings clean for the wasm target. Not exercised in a browser — the failing request is gone by construction, but the applied-preset round trip still wants a manual pass. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The published capture surface targeted the canvas bounds directly, so a 16:9 desktop was resampled into 640x480 — 2x horizontally but 1.5x vertically, a 1.33x vertical stretch. No downstream fit mode can undo that, because the distortion is baked into the pixels before any effect sees them; circles reached screen-mirroring effects as ellipses. Fit within the canvas bounds instead of filling them. A 4K or 1080p desktop now publishes 640x360, a square source 480x480, and portrait or ultrawide sources scale on whichever axis binds first. An existing test asserted a 40x40 frame produced a 640x480 surface, which was the squash itself written down as an expectation; a square source now yields a square surface. Also aligns the core CaptureConfig letterbox default with the one in config::CaptureConfig, which was flipped off in the previous commit. Leaving them disagreeing meant a caller building the core config directly still got the old cropping behaviour. Verified against a live 3840x2160 desktop: canvas_downscale went from 640x480 to 640x360. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/hypercolor-ui/src/components/preset_panel.rs`:
- Around line 249-270: Update the scoped branch of the bundled-preset PATCH flow
inside the async task to handle the returned LayerStackOutcome and refresh or
apply it to ZonesContext after a successful patch. Mirror the existing
zone-scoped success path, while preserving the global update_controls behavior
when scene_scoped_target() returns None.
🪄 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 Plus
Run ID: fcbddcec-70a8-46ae-b733-5f44928484de
📒 Files selected for processing (2)
crates/hypercolor-ui/src/components/preset_panel.rscrates/hypercolor-ui/src/zones.rs
Comment on lines
+249
to
+270
| // Bundled presets write the same controls a manual edit | ||
| // does, so they must take the same route: the zone's | ||
| // synthetic legacy layer, keyed by the zone id. The old | ||
| // global endpoint 404s whenever no legacy "current | ||
| // effect" exists, which is always in a zone scene — and | ||
| // the failure branch below then snapped the dropdown | ||
| // back, so bundled presets looked like they refused to | ||
| // stick. | ||
| let scoped = zones_ctx.scene_scoped_target(); | ||
| leptos::task::spawn_local(async move { | ||
| match api::update_controls(&controls_json).await { | ||
| let result = match scoped { | ||
| Some((scene_id, zone_id)) => api::patch_layer_controls( | ||
| &scene_id, | ||
| &zone_id, | ||
| &zone_id, | ||
| &controls_json, | ||
| None, | ||
| ) | ||
| .await | ||
| .map(|_| ()), | ||
| None => api::update_controls(&controls_json).await, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh the active scene after a scoped bundled-preset update.
When scene_scoped_target() returns Some, this branch mutates the zone layer but never refreshes ZonesContext or applies the returned LayerStackOutcome. The UI can therefore retain stale zone controls and later overwrite or display state older than the preset just applied. Mirror the existing zone-scoped success path by refreshing the scene after a successful PATCH, or consume the response to update the cache.
Proposed fix
let result = match scoped {
- Some((scene_id, zone_id)) => api::patch_layer_controls(
- &scene_id,
- &zone_id,
- &zone_id,
- &controls_json,
- None,
- )
- .await
- .map(|_| ()),
+ Some((scene_id, zone_id)) => {
+ let result = api::patch_layer_controls(
+ &scene_id,
+ &zone_id,
+ &zone_id,
+ &controls_json,
+ None,
+ )
+ .await
+ .map(|_| ());
+ if result.is_ok() {
+ zones_ctx.refresh.run(());
+ }
+ result
+ }📝 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
| // Bundled presets write the same controls a manual edit | |
| // does, so they must take the same route: the zone's | |
| // synthetic legacy layer, keyed by the zone id. The old | |
| // global endpoint 404s whenever no legacy "current | |
| // effect" exists, which is always in a zone scene — and | |
| // the failure branch below then snapped the dropdown | |
| // back, so bundled presets looked like they refused to | |
| // stick. | |
| let scoped = zones_ctx.scene_scoped_target(); | |
| leptos::task::spawn_local(async move { | |
| match api::update_controls(&controls_json).await { | |
| let result = match scoped { | |
| Some((scene_id, zone_id)) => api::patch_layer_controls( | |
| &scene_id, | |
| &zone_id, | |
| &zone_id, | |
| &controls_json, | |
| None, | |
| ) | |
| .await | |
| .map(|_| ()), | |
| None => api::update_controls(&controls_json).await, | |
| }; | |
| // Bundled presets write the same controls a manual edit | |
| // does, so they must take the same route: the zone's | |
| // synthetic legacy layer, keyed by the zone id. The old | |
| // global endpoint 404s whenever no legacy "current | |
| // effect" exists, which is always in a zone scene — and | |
| // the failure branch below then snapped the dropdown | |
| // back, so bundled presets looked like they refused to | |
| // stick. | |
| let scoped = zones_ctx.scene_scoped_target(); | |
| leptos::task::spawn_local(async move { | |
| let result = match scoped { | |
| Some((scene_id, zone_id)) => { | |
| let result = api::patch_layer_controls( | |
| &scene_id, | |
| &zone_id, | |
| &zone_id, | |
| &controls_json, | |
| None, | |
| ) | |
| .await | |
| .map(|_| ()); | |
| if result.is_ok() { | |
| zones_ctx.refresh.run(()); | |
| } | |
| result | |
| } | |
| None => api::update_controls(&controls_json).await, | |
| }; |
🤖 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 `@crates/hypercolor-ui/src/components/preset_panel.rs` around lines 249 - 270,
Update the scoped branch of the bundled-preset PATCH flow inside the async task
to handle the returned LayerStackOutcome and refresh or apply it to ZonesContext
after a successful patch. Mirror the existing zone-scoped success path, while
preserving the global update_controls behavior when scene_scoped_target()
returns None.
The Settings "Source" dropdown offered "Auto" and "PipeWire" — backend implementation names, one of them Linux-only, shown to Windows users as the only way to aim screen capture. The real selector, capture.source = "monitor:N", was reachable only by hand-editing config, with nothing telling the user what N meant on their machine. The capture crate now describes every attached output — capture index, OS device name, desktop extent, and whether it anchors the virtual desktop origin — and GET /api/v1/capture/monitors serves the list with a ready-to-store capture.source value per entry. An empty list is the capability signal: portal platforms return nothing, and the UI keeps the portal picker there instead of a dropdown. Index order matches DesktopDuplicator's adapter-then-output enumeration, so what the picker shows is exactly what a duplicator opens. Verified live against real hardware: two outputs reported with correct geometry, including a portrait 1440x2560 secondary, and the listing test asserts the count matches, extents are nonzero, and a primary exists. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The Screen Capture section was thirteen rows of raw engine knobs —
scene-cut thresholds, letterbox luminance floors, sector grid dimensions
— presented flat, with a Source dropdown whose entries were backend
implementation names ("PipeWire" on a Windows machine). Users met video
pipeline internals before they could answer the only two questions that
matter: is capture on, and which screen.
The section is now those two decisions. The dropdown lists real
monitors from /api/v1/capture/monitors with resolution and a primary
tag; on portal platforms the list is empty and the portal picker button
renders in its place, so each platform shows only the selector it
actually honours.
Everything else moves behind a collapsed Advanced disclosure with
descriptions written for humans: smoothing becomes "Color response",
the letterbox toggle says it is for video and warns that dark desktop
scenes can be mistaken for bars, and the sampling grid rows say
outright that layouts reference the zones so changing them remaps
things. Nothing was removed and no keys changed — everything is still
there for the users it exists for, in the place that says how often
that is.
Verified: cargo check and clippy -D warnings clean for the wasm target;
daemon api and openapi tests pass (192 + 3).
Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/hypercolor-windows-capture/src/shared.rs (1)
107-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded
describe_outputs()error.
unwrap_or_default()erases a real DXGI failure into the same empty vector that means "no picker on this platform", so a Windows enumeration failure is indistinguishable from Linux and leaves no trace for diagnosis.♻️ Proposed change
#[cfg(target_os = "windows")] { - crate::duplication::describe_outputs().unwrap_or_default() + match crate::duplication::describe_outputs() { + Ok(monitors) => monitors, + Err(error) => { + tracing::warn!(%error, "Failed to enumerate DXGI outputs"); + Vec::new() + } + } }As per coding guidelines, "Use
tracingfor logging and neverprintln!in library code."🤖 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 `@crates/hypercolor-windows-capture/src/shared.rs` around lines 107 - 116, Update list_monitors to handle the Err case from crate::duplication::describe_outputs() explicitly instead of using unwrap_or_default(), and log the discarded DXGI error through the library’s tracing facility. Preserve the empty-vector result for failures and non-Windows builds.Source: Coding guidelines
crates/hypercolor-ui/src/components/settings_sections.rs (2)
234-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedto the disclosure button.The chevron and label change convey state visually only; screen readers get no expanded/collapsed cue.
♻️ Proposed change
<button type="button" + aria-expanded=move || show_advanced.get().to_string() class="flex w-full items-center gap-1.5 py-3 text-left text-[12px] font-medium text-fg-tertiary hover:text-fg-secondary transition-colors"🤖 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 `@crates/hypercolor-ui/src/components/settings_sections.rs` around lines 234 - 247, Add an aria-expanded attribute to the disclosure button that is bound to show_advanced, returning the current boolean state so assistive technologies receive the expanded/collapsed status while preserving the existing toggle behavior.
149-156: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMonitor list is fetched once and fetch errors are indistinguishable from "no picker".
LocalResourcenever refetches, so a monitor hot-plug or a daemon reconnect leaves a stale list, and a failed request collapses to an empty vec — which on Windows renders the portal "Choose source" button that does nothing there. Key the resource onconnection_generationand log/surface the error case separately.As per coding guidelines, "Do not rely on browser polling in the UI; use WebSocket events, hint signals, and
connection_generation-based refetching instead."🤖 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 `@crates/hypercolor-ui/src/components/settings_sections.rs` around lines 149 - 156, The monitor resource in the settings component currently fetches only once and converts fetch failures into an empty list. Key `LocalResource::new` to the existing `connection_generation` signal so reconnects and monitor changes trigger refetching, and handle/log the `Err` case separately before deriving `has_monitor_picker`, ensuring failures do not appear as a valid empty monitor list.Source: Coding guidelines
🤖 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 `@crates/hypercolor-ui/src/components/settings_sections.rs`:
- Line 158: Update the option initialization in the settings source-selection
flow so the "auto" entry describes the actual capture-index-0 behavior rather
than labeling it as the primary monitor; keep the source value "auto" unchanged
and use wording consistent with adapter/output enumeration.
---
Nitpick comments:
In `@crates/hypercolor-ui/src/components/settings_sections.rs`:
- Around line 234-247: Add an aria-expanded attribute to the disclosure button
that is bound to show_advanced, returning the current boolean state so assistive
technologies receive the expanded/collapsed status while preserving the existing
toggle behavior.
- Around line 149-156: The monitor resource in the settings component currently
fetches only once and converts fetch failures into an empty list. Key
`LocalResource::new` to the existing `connection_generation` signal so
reconnects and monitor changes trigger refetching, and handle/log the `Err` case
separately before deriving `has_monitor_picker`, ensuring failures do not appear
as a valid empty monitor list.
In `@crates/hypercolor-windows-capture/src/shared.rs`:
- Around line 107-116: Update list_monitors to handle the Err case from
crate::duplication::describe_outputs() explicitly instead of using
unwrap_or_default(), and log the discarded DXGI error through the library’s
tracing facility. Preserve the empty-vector result for failures and non-Windows
builds.
🪄 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 Plus
Run ID: 231855c4-83cf-41f5-80f1-cde2aa9c02c3
📒 Files selected for processing (10)
crates/hypercolor-core/src/input/screen/mod.rscrates/hypercolor-daemon/src/api/capture.rscrates/hypercolor-daemon/src/api/mod.rscrates/hypercolor-ui/src/api/config.rscrates/hypercolor-ui/src/components/settings_sections.rscrates/hypercolor-windows-capture/Cargo.tomlcrates/hypercolor-windows-capture/src/duplication.rscrates/hypercolor-windows-capture/src/lib.rscrates/hypercolor-windows-capture/src/shared.rscrates/hypercolor-windows-capture/tests/duplication_tests.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/hypercolor-windows-capture/Cargo.toml
- crates/hypercolor-windows-capture/src/lib.rs
- crates/hypercolor-windows-capture/tests/duplication_tests.rs
- crates/hypercolor-windows-capture/src/duplication.rs
| }); | ||
| let has_monitor_picker = Signal::derive(move || !monitors.get().is_empty()); | ||
| let monitor_options = Signal::derive(move || { | ||
| let mut options = vec![("auto".to_owned(), "Primary monitor".to_owned())]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"auto" is not guaranteed to be the primary monitor.
capture.source = "auto" resolves to capture index 0 in monitor_index_from_source, but index 0 is adapter-then-output enumeration order while primary is derived from the desktop origin in describe_outputs. On multi-GPU or rearranged desktops these differ, so the label misdescribes what will be captured.
♻️ Proposed change
- let mut options = vec![("auto".to_owned(), "Primary monitor".to_owned())];
+ let mut options = vec![("auto".to_owned(), "Default monitor".to_owned())];📝 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
| let mut options = vec![("auto".to_owned(), "Primary monitor".to_owned())]; | |
| let mut options = vec![("auto".to_owned(), "Default monitor".to_owned())]; |
🤖 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 `@crates/hypercolor-ui/src/components/settings_sections.rs` at line 158, Update
the option initialization in the settings source-selection flow so the "auto"
entry describes the actual capture-index-0 behavior rather than labeling it as
the primary monitor; keep the source value "auto" unchanged and use wording
consistent with adapter/output enumeration.
CI's Linux shared check failed on dead code: CaptureError::windows() is pub(crate) and its only callers live in the cfg(windows) duplication module, so on Linux the constructor compiles with no callers and the workspace's -D warnings turns that into a build failure. Windows clippy can never surface this, the mirror image of the evdev lint that kept main red for a week. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
hyperb1iss
changed the title
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.
Windows shipped in 0.2.1 with its hardware access stack half-installed, no
screen capture backend at all, and a capture pipeline that mangled what it did
capture. This PR fixes the install path end to end, lands Desktop Duplication
capture, repairs four pipeline defects that made screen effects look broken on
every platform, and unblocks the CI that had been red on
mainsince2026-07-21.
Everything was diagnosed against a stock 0.2.1 install or a live 4K desktop
and verified on real hardware — receipts inline.
The 0.2.1 Windows install was broken in three stacked ways
A Corsair AIO LCD showing
0%turned out to be the visible end of a chain:keep broker setup "an explicit administrator action" — but install time is
the only moment the app holds administrator rights, so the fallback was
nothing at all. The NSIS hook now runs the hardware-support orchestrator
(PawnIO + verified modules + broker) in one elevated pass, with every path
under
$INSTDIRso the broker installer's rejection of user-writableservice paths still holds.
install-windows-hardware-support.ps1splatted an array into a PowerShell script; array splatting binds
positionally, so
-AssetRootarrived as a parameter value, everythingshifted one slot, and no switch ever bound. Settings → "Install support"
has therefore never worked. Now hashtable splatting, with a packaging test
pinning it.
now re-probes on a 20s backoff and drops a dead reader so a broker
restarted by SCM heals on its own.
Evidence: on a clean 0.2.1 install,
PawnIOwas Running with modulesdeployed while
HypercolorSmBusdid not exist. Running the fixed orchestratorelevated brought two previously invisible ASUS Aura DRAM devices up over SMBus
without a daemon restart (device count 14 → 16, one rendering immediately).
Windows screen capture now exists
build_input_managerregistered a screen source only undercfg(target_os = "linux"); togglingcapture.enabledon Windows did nothing.hypercolor-windows-captureadds a DXGI Desktop Duplication backend — its owncrate because the workspace forbids
unsafe_codeand COM interop cannot,matching the existing Windows FFI crates. Desktop Duplication is the
compositor's own presented output: free while the screen is static, no border,
no picker, and no permission grant (Windows has no portal/TCC equivalent).
capture.enableddefaults totrueon Windows only. Flaggedexplicitly: there is nothing to consent to on Windows, so the toggle
protected nothing. Portal (Linux) and TCC (macOS) are real consent moments
and stay opt-in. Enabling only grants permission — capture opens on effect
demand and the duplication interface is released when idle, since Windows
allows one per output per process.
wrong, because the buffer is also published as
canvas_downscaleandconsumed as an image, then downscaled again — two point samplings of a 4K
desktop shred text into aliased noise. Found by dumping real frames
(
dump_frame/dump_screen_canvasexamples are in-tree).Four capture pipeline defects, all platforms
These predate the Windows backend and explain "ambilight looks like garbage":
pixel_luminanceis linear;sRGB 30/255 is 0.013 linear, under the 0.02 "black bar" threshold — so a
dark desktop read as bars from every edge. Measured live: an 8×6 grid
reported bars
[6,6,8,8](the entire grid), cropping the picture tonothing and strobing as content changed. Detection now refuses bars that
swallow an axis, and letterbox defaults off — desktops are not
letterboxed films.
vertical stretch baked into the pixels before any effect sees them. Now
fits within the canvas bounds: 4K publishes 640×360, portrait and
ultrawide scale on the binding axis.
/effects/current/controls, which 404s in a zone scene; the error handlerrestored the previous dropdown selection, so presets visibly "refused to
stick" and Screen Cast stayed pinned to Center Zoom's 40% crop. They now
take the same zone-scoped route as manual control edits.
color lag; relabeled "Color response" with a sane default).
Monitor selection and a usable settings panel
GET /api/v1/capture/monitorsserves real outputs (index, name, extent,primary) with ready-to-store
capture.sourcevalues;monitor:N/display:Nparsing lands in core. Verified against real hardware includinga portrait 1440×2560 secondary.
(with a "Source" dropdown offering literally "PipeWire" on Windows) to two
decisions — enabled, and which monitor — plus a collapsed Advanced
disclosure with human-language descriptions. No keys removed or renamed.
Portal platforms get an empty monitor list and keep the picker button.
CI
mainhad been red since 2026-07-21: oneclippy::collapsible_matchin theLinux-only evdev reader. Fixed here (behaviour-preserving match guard) — and
this PR hit the mirror-image trap once itself (a Windows-only constructor dead
on the Linux check), which is its own commit. A stale
Cargo.lockfrom thedump-example dev-dependency is also fixed.
Also here
scripts/cargo-cache-build.ps1now asserts the resolvedrustchost is MSVCbefore building, with the resolved path in the error. A GNU-host Rust
shadowing rustup fails late and selectively (mingw
lderrors, E0514mismatches) and neither symptom names the cause.
Blast radius
(letterbox, aspect, presets — deliberate, they were broken everywhere), the
capture.enableddefault (cfg-guarded, unchanged off Windows), and the newoptional
monitorfield on core'sCaptureConfig.most directly on real hardware.
Verification
-D warningsclean: types, core, capture crate, daemon, UI (wasm)against a 3840×2160 display) · 192 daemon API tests · 18→20 packaging tests
verified against live hardware as described above