feat: screen streaming and remote control — nexus-stream crate, StreamService, agent integration (ADR 0014)#43
feat: screen streaming and remote control — nexus-stream crate, StreamService, agent integration (ADR 0014)#43b5119 wants to merge 8 commits into
Conversation
…mService, agent integration (ADR 0014)
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new ChangesScreen Streaming Feature
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/agent/src/host.rs (1)
982-1096: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
qualityis still a no-op. The CLI advertises a low/medium/high preset, buthost::runonly logs it and the encoder API has no way to consume it. Update the help text or wire the preset into encoding.🤖 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/agent/src/host.rs` around lines 982 - 1096, The `quality` argument in `host::run` is currently unused except for logging, so the advertised low/medium/high preset has no effect. Either remove it from the CLI/help path if it is not supported, or thread the preset into the streaming pipeline by updating `nexus_stream::encode::Encoder::new` and the `enable_streaming` branch so the encoder actually consumes `quality` and changes behavior accordingly.
🟡 Minor comments (11)
crates/stream/src/main.rs-22-22 (1)
22-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve rustfmt failure. CI
0_fmt & clippyreports a formatting diff here; runcargo fmt.🤖 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/stream/src/main.rs` at line 22, Resolve the rustfmt mismatch in the main module by running cargo fmt and keeping the formatting consistent around the closing brace in main.rs; this is a formatting-only fix, so adjust the surrounding block in the entrypoint code if needed and ensure the rustfmt output is clean.Source: Pipeline failures
crates/stream/src/display.rs-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve rustfmt failures. CI
0_fmt & clippyreports formatting diffs (import ordering and event patterns) at these lines; runcargo fmt.Also applies to: 67-67, 100-100, 122-122, 135-135
🤖 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/stream/src/display.rs` at line 1, The file has rustfmt mismatches in the import ordering and a few event pattern matches, so update the formatting in the affected display module and re-run cargo fmt. Check the top-level use declarations and the matching logic around the referenced event-handling blocks in the display code, then let rustfmt normalize the ordering/pattern layout so CI no longer reports diffs.Source: Pipeline failures
crates/stream/src/decode.rs-103-110 (1)
103-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEAGAIN path returns a black frame, causing visible flicker.
When the decoder needs more input it returns
EAGAIN, but here you fabricate a fully-zeroed BGRA buffer and return it as a validDecodedFrame. The viewer loop unconditionally renders whateverdecodereturns (self.display.render(decoded)?inviewer.rs), so every buffering event paints a black frame over the last good one, producing flicker.Prefer returning
Ok(None)(change the signature toResult<Option<DecodedFrame>>) and skip rendering onNone, so the previous frame stays on screen until a real frame is decoded.🤖 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/stream/src/decode.rs` around lines 103 - 110, The EAGAIN handling in decode::Decoder::decode currently fabricates a zeroed DecodedFrame, which causes the viewer to render black frames during buffering. Change decode to return an optional frame result (for example through the decode method and its callers, including viewer::Viewer::render flow) so the EAGAIN branch yields no frame and the existing image remains displayed. Update the caller to skip self.display.render(decoded)? when decode produces None, and keep the real-frame path unchanged.crates/stream/src/decode.rs-35-35 (1)
35-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve rustfmt failures. CI
0_fmt & clippyreports formatting diffs at these lines; runcargo fmtto fix.Also applies to: 55-55, 81-81, 100-100
🤖 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/stream/src/decode.rs` at line 35, rustfmt is flagging formatting differences in the ffmpeg-gated code in decode.rs, so re-run cargo fmt and commit the resulting formatting changes. Make sure the cfg(feature = "ffmpeg") blocks and the related items around the referenced locations are reformatted consistently so CI `0_fmt & clippy` passes.Source: Pipeline failures
crates/stream/src/viewer.rs-4-4 (1)
4-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve rustfmt failures. CI
0_fmt & clippyreports formatting diffs in imports and parameter indentation; runcargo fmt.Also applies to: 24-24
🤖 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/stream/src/viewer.rs` at line 4, Resolve the rustfmt diffs in the viewer module by running cargo fmt and accepting the resulting import ordering and parameter indentation changes. Focus on the import block around use tonic::transport::Channel and any affected function signatures or parameter lists in the same file so the formatting matches rustfmt’s output and CI passes.Source: Pipeline failures
crates/stream/src/host.rs-7-10 (1)
7-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix rustfmt formatting to unblock CI.
CI
0_fmt & clippyflags rustfmt diffs at Lines 4, 34, 73, and 117 (imports, associated type, interval expression, and theResponse::newcall). Runcargo fmt.Also applies to: 37-38, 73-73, 120-120
🤖 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/stream/src/host.rs` around lines 7 - 10, The rustfmt check is failing in the `StreamService` implementation because several expressions are not formatted consistently. Run `cargo fmt` and ensure the `use nexus_proto::stream::v1` import, the associated type declaration, the interval expression, and the `Response::new` call in `host.rs` are all rewritten to the formatter’s expected style.Source: Pipeline failures
crates/stream/src/encode.rs-47-51 (1)
47-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix rustfmt formatting to unblock CI.
CI
0_fmt & clippyflags rustfmt diffs at Lines 47, 87, 97, and 109. Runcargo fmt.Also applies to: 87-91, 97-97, 109-114
🤖 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/stream/src/encode.rs` around lines 47 - 51, The rustfmt output for FfmpegEncoder in encode.rs is out of sync with CI, so update the formatting to match cargo fmt. Reformat the affected blocks in FfmpegEncoder::new and the nearby sections around the referenced use/import and match/closure code so the diffs at the reported locations disappear, then verify with cargo fmt before committing.Source: Pipeline failures
crates/stream/src/capture.rs-47-52 (1)
47-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix rustfmt formatting to unblock CI.
Pipeline reports a rustfmt diff on this
tracing::info!call (CI0_fmt & clippy). Runcargo fmtto resolve.🤖 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/stream/src/capture.rs` around lines 47 - 52, The `tracing::info!` call in `capture.rs` is formatted in a way that rustfmt rejects, so fix the formatting to match standard Rust style and rerun formatting on the crate; use `capture.rs` and the `tracing::info!` logging block as the anchor, and apply `cargo fmt` so the CI `0_fmt & clippy` check passes.Source: Pipeline failures
crates/stream/src/host.rs-75-77 (1)
75-77: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against
fps <= 0before computing the interval.
Duration::from_secs_f64(1.0 / fps)panics forfps == 0(non-finite) and behaves oddly for negative values. The non-LinuxX11Capturestub returns0.0fromfps(), and any misconfiguration could hit this too. Clampfpsto a sane minimum first.🤖 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/stream/src/host.rs` around lines 75 - 77, The interval setup in host.rs should guard against invalid capture rates before using capture.fps() in the tokio::time::interval calculation. Update the logic around the fps() call so fps values at or below zero are clamped to a safe minimum or handled by an early fallback before passing them into Duration::from_secs_f64, and make sure the X11Capture stub and any misconfigured capture implementation cannot reach the interval creation with a non-positive rate.crates/stream/src/host.rs-73-118 (1)
73-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
remote_controlis effectively single-viewer today —encoder.lock().awaitandcapture.lock().awaitstay held for the full stream, so a second client will block until the first disconnects. If that’s intentional, reject or document the limit explicitly; otherwise acquire the locks per tick or otherwise avoid serializing sessions.🤖 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/stream/src/host.rs` around lines 73 - 118, The stream loop in host.rs is holding the encoder and capture mutexes for the entire session, which makes remote_control effectively single-viewer. Update the logic around the streaming task so that encoder.lock().await and capture.lock().await are not kept across the whole loop in the same task; instead acquire them only when needed per iteration or restructure the shared state so multiple clients can stream concurrently. If single-viewer is intended, explicitly enforce or document that behavior in the streaming setup around the spawn block.crates/stream/src/encode.rs-65-66 (1)
65-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPTS cadence should match the encoder clock
time_base = (1, 1000)plusself.pts += 1makes each frame look 1 ms apart while the encoder is set up for 30 fps. Derive PTS from the capture interval or switch the encoder time base to the frame cadence.🤖 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/stream/src/encode.rs` around lines 65 - 66, The PTS generation in encode.rs is using a millisecond clock while the encoder is configured for 30 fps, so update the timing in the encode path to use a cadence consistent with the encoder’s frame rate. Adjust the time base setup in the encoder configuration and the PTS advancement logic in the frame encode flow so they derive from the capture/frame interval rather than incrementing by 1 ms per frame. Refer to the encoder setup around set_time_base and set_frame_rate and the self.pts update in the same encode flow.
🧹 Nitpick comments (9)
crates/stream/src/viewer.rs (1)
33-37: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueViewer resolution is hardcoded to 1920×1080.
The decoder and display are fixed at 1920×1080 regardless of the host's actual frame size; incoming frames are always scaled to this. Consider negotiating or deriving dimensions from the first
VideoFrame(vf.width/vf.height).🤖 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/stream/src/viewer.rs` around lines 33 - 37, The Viewer setup is hardcoding the decoder and display size to 1920×1080, so update the initialization in Viewer::new to derive the dimensions from the incoming video stream instead of fixed constants. Use the first VideoFrame’s vf.width and vf.height (or negotiate equivalent dimensions before creating Decoder and ViewerDisplay) and then pass those values into Decoder::new and ViewerDisplay::new so the viewer matches the host frame size.crates/stream/src/main.rs (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated config-dir logic.
config_dir()reimplementscrate::config::config_dir()used byTrustedCertsStore. Reusing the existing helper (andTrustedCertsStore) would keep the trusted-cert path/shape in sync and avoid the parsing drift flagged above.🤖 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/stream/src/main.rs` around lines 9 - 15, The config directory logic is duplicated in config_dir() instead of reusing crate::config::config_dir(), which can drift from TrustedCertsStore’s path handling. Update the main crate to call the existing config helper and use TrustedCertsStore so trusted-cert path/shape stays consistent with the rest of the codebase.crates/stream/src/decode.rs (1)
66-69: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrain the decoder rather than receiving a single frame per packet.
send_packetmay yield zero or several frames; callingreceive_frameonce can leave decoded frames buffered and add latency. Loopreceive_frameuntil it returnsEAGAIN.🤖 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/stream/src/decode.rs` around lines 66 - 69, The packet decode path in `Decoder::decode` only pulls one frame after `self.video.send_packet(&pkt)?`, which can leave additional decoded frames buffered. Update the `receive_frame` handling to keep draining frames in a loop until it returns `EAGAIN`, collecting or forwarding each decoded `Video` frame before returning. Preserve the existing error handling for non-`EAGAIN` failures and keep the logic localized around `send_packet`/`receive_frame`.crates/agent/src/host.rs (1)
973-984: 📐 Maintainability & Code Quality | 🔵 Trivial
runnow has 10 positional parameters; consider grouping streaming config into a struct.The
#[allow(clippy::too_many_arguments)]suppresses the lint rather than addressing the underlying readability/maintainability concern. Consider bundlingenable_streaming,fps, andquality(and perhaps the GC-related params) into a config struct to keep the call sites and signature manageable as more options are added.♻️ Sketch
+pub struct StreamingOptions { + pub enable: bool, + pub fps: u32, + pub quality: String, +} + pub async fn run( serve_dir: String, port: u16, auth_token: String, device_id: nexus_common::DeviceId, gc_interval_hours: u64, tombstone_ttl_hours: u64, max_store_entries: usize, - enable_streaming: bool, - fps: u32, - quality: String, + streaming: StreamingOptions, ) -> Result<()> {🤖 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/agent/src/host.rs` around lines 973 - 984, The run function has too many positional parameters, and the clippy allow is only masking the readability issue. Refactor run in host.rs to take a config struct for related settings, grouping at least enable_streaming, fps, and quality (and optionally the GC parameters too), then update the call sites to construct and pass that struct instead of individual arguments.crates/stream/Cargo.toml (2)
10-12: 🚀 Performance & Scalability | 🔵 TrivialConsider gating
winit/pixelsbehind a feature for headless CI, mirroring theffmpeggate.
ffmpeg-nextis optional and off by default specifically for CI, per the PR description, butwinit/pixels/x11(needed for the viewer/display path) are unconditional dependencies. This means CI must still have X11 dev headers and GPU-capable build toolchains available just to compile this crate, even without ever exercising the display path.Also applies to: 17-30
🤖 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/stream/Cargo.toml` around lines 10 - 12, The stream crate still pulls in the viewer/display stack unconditionally, so headless CI cannot avoid the X11/GPU build requirements. Update the Cargo feature setup in Cargo.toml to mirror the existing ffmpeg gate by making the winit/pixels/x11 path optional and enabled only through a dedicated feature, then wire the display-specific code to that feature so the non-display build stays CI-friendly. Use the existing ffmpeg feature and the viewer/display-related code paths referenced by the crate to keep the gating consistent.
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize
x11,libc, andclapversions in the workspace, for consistency.This PR just introduced a workspace-dependency pattern for
ffmpeg-next,winit,pixels,tokio-stream, andfutures-util(rootCargo.toml), butx11,libc, andclapare pinned directly here instead. This risks version drift if these crates are (or become) used elsewhere in the workspace with a different pinned version.♻️ Proposed refactor
[dependencies] ... -clap = { version = "4", features = ["derive", "env"] } +clap = { workspace = true, features = ["derive", "env"] } [target.'cfg(target_os = "linux")'.dependencies] -x11 = { version = "2.21", features = ["xlib"] } -libc = "0.2" +x11 = { workspace = true, features = ["xlib"] } +libc.workspace = true(Requires adding
x11,libc, andclapentries to[workspace.dependencies]in the rootCargo.tomlif not already present.)🤖 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/stream/Cargo.toml` around lines 31 - 35, Centralize the directly pinned `clap`, `x11`, and `libc` dependencies used in the `stream` crate by moving their version declarations to the workspace’s `[workspace.dependencies]` in the root `Cargo.toml`. Then update the `stream` crate’s dependency entries to reference the workspace versions consistently, alongside the existing workspace-dependency pattern already used for `ffmpeg-next`, `winit`, `pixels`, `tokio-stream`, and `futures-util`.crates/proto/proto/stream_service.proto (1)
46-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument coordinate semantics for
x/yonMOUSEevents.The contract doesn't specify whether
x/yrepresent absolute screen/window coordinates or relative deltas forInputAction.MOVE. This ambiguity is a likely contributor to a real mismatch found downstream: the viewer sends absolute cursor positions, while the host'sinject.rsinjects them as relative motion deltas (see review comment oncrates/stream/src/inject.rs). Clarifying this in the proto comments (and picking one convention consistently) would prevent this class of bug.🤖 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/proto/proto/stream_service.proto` around lines 46 - 53, Clarify the coordinate contract for InputEvent.x and InputEvent.y in stream_service.proto and make it consistent with the injector path. Update the InputEvent/MouseEvent comments to state whether MOVE uses absolute screen/window coordinates or relative deltas, then align the downstream handling in inject.rs so the host interprets the values using the same convention. Refer to InputEvent, InputEventType, and InputAction when adjusting the proto and the injection logic.crates/stream/src/host.rs (1)
124-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment overstates behavior.
The comment states this "register[s] the StreamService on the given tonic server builder," but the function only constructs and returns a
StreamHost— no server builder is taken or registered. Align the doc with the actual behavior.🤖 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/stream/src/host.rs` around lines 124 - 144, The doc comment for run_stream_host overstates what the function does by claiming it registers StreamService on a tonic server builder, but the implementation only initializes and returns a StreamHost. Update the comment above run_stream_host to describe only the actual behavior: starting the stream host state by wrapping capture, encoder, and injector in Arc<Mutex<...>> and returning the StreamHost; remove any mention of server builder registration.crates/stream/src/encode.rs (1)
107-114: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOnly one packet is drained per input frame.
send_framecan enqueue more than one output packet (or buffer with B-frames/lookahead), but only a singlereceive_packetis issued. Extra packets accumulate and lag behind the live stream. Consider loopingreceive_packetuntilEAGAIN.🤖 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/stream/src/encode.rs` around lines 107 - 114, The encode path in the frame handling logic only drains one packet after calling send_frame, which can leave buffered packets queued behind live output. Update the packet retrieval in the encode routine to keep calling receive_packet on the same codec context until it returns EAGAIN, collecting or emitting every available packet before moving on. Use the existing encode flow around self.ctx.send_frame and receive_packet to locate the fix.
🤖 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/agent/src/host.rs`:
- Around line 1077-1098: The streaming bootstrap in the host server currently
uses `?` for `X11Capture::new`, `Encoder::new`, `Injector::new`, and
`nexus_stream::host::run_stream_host`, which causes `run` to abort before
`router.serve(addr)` and prevents `FileServiceServer` from starting. Update the
`enable_streaming` branch in `host.rs` to treat streaming setup as optional:
catch and log setup failures with a warning, skip adding `StreamServiceServer`
when initialization fails, and continue serving the existing router so
`FileService` still runs. Keep the success path intact by only constructing
`stream_svc` and calling `router.add_service(...)` after all streaming
components initialize successfully.
In `@crates/agent/src/main.rs`:
- Around line 38-49: The `Args` fields for `fps` and `enable_streaming` need
adjustment: `fps` currently allows 0, which later reaches
`Duration::from_secs_f64(1.0 / fps)` in `host.rs`, so add a lower-bound parser
or validation to enforce `>= 1`; also update the `enable_streaming` help text in
`main.rs` to avoid implying FFmpeg is always required, since non-ffmpeg builds
use the scaffold encoder fallback, or make that mismatch a runtime validation if
intended.
In `@crates/stream/src/capture.rs`:
- Around line 80-96: The capture path in `capture_frame` currently discards the
scanline stride, which can cause sheared output when the buffer is consumed by
`encode.rs`. Update `CapturedFrame` to retain `bytes_per_line` (or repack the
data to a tight `width * 4` layout here), and adjust the copy logic so the
FFmpeg upload copies each row using the stored stride instead of assuming a flat
buffer. Use `capture_frame` and `CapturedFrame` as the key symbols to update
together with the consumer in `encode.rs`.
In `@crates/stream/src/display.rs`:
- Around line 107-110: The key_code assignment in display handling is using the
winit enum discriminant instead of a Linux input code, so `Injector::inject`
ends up sending the wrong `EV_KEY` values. Update the `kev.physical_key`
handling in `display.rs` to translate `winit::keyboard::PhysicalKey::Code` /
`KeyCode` into the corresponding Linux `KEY_*` code before storing it in
`InputEvent.key_code`, and keep the fallback for unmapped keys explicit.
- Around line 125-137: The CursorMoved handler in display::stream::display.rs is
sending absolute window coordinates into the mouse input path instead of
relative movement. Update the WindowEvent::CursorMoved branch to track the
previous cursor position and send deltas (current minus last) in the InputEvent
you build, while keeping the existing InputEvent, InputAction::Move, and
MouseButton::None flow intact. Make sure the logic in the CursorMoved match arm
computes and forwards relative x/y values before calling self.input_tx.send.
In `@crates/stream/src/encode.rs`:
- Around line 27-33: The scale_dimensions helper can return odd or zero sizes,
which are not safe for YUV420P encoder setup. Update scale_dimensions to always
round both width and height down to even values and clamp each to at least 2,
including the early return path for already-small inputs and the scaled path
used for larger inputs. Keep the fix localized to scale_dimensions so the
encoder always receives even, valid dimensions.
In `@crates/stream/src/host.rs`:
- Around line 54-62: The async input-processing task in the `tokio::spawn` block
uses `injector_clone.blocking_lock()` inside an async context, which can panic
and break stream injection. Update the logic around `input_stream.next()` and
`inject()` to acquire the `tokio::sync::Mutex` with `.lock().await` instead of
`blocking_lock()`, then call `inject` on the guarded injector and keep the
existing warning handling for failures.
In `@crates/stream/src/inject.rs`:
- Around line 184-213: Mouse Move is being injected as relative motion even
though InputAction::Move carries absolute cursor coordinates from the viewer.
Update the InputEventType::Mouse handling in inject.rs so the Move branch uses
the device’s absolute axes (EV_ABS with ABS_X/ABS_Y) instead of
EV_REL/REL_X/REL_Y, matching the existing ABS registration on this input device.
Keep Press/Release behavior unchanged and route the x/y values directly as
absolute positions in the same write_ev/sync flow.
- Around line 147-160: The initial uinput_user_dev write in the device setup
path ignores the return value from libc::write, unlike write_ev which already
checks for failures; update this same block to capture the write result, verify
it wrote the full descriptor, and return an error if the write fails or is short
before continuing to UI_DEV_CREATE. Keep the handling consistent with the
existing write_ev error path so the caller sees configuration failures instead
of proceeding with a broken virtual device.
- Around line 168-182: The Keyboard branch in inject() is writing
InputEvent.key_code directly to EV_KEY, but that value comes from
winit::keyboard::KeyCode and must be translated to Linux evdev codes first.
Update the injection path in inject.rs (around inject and the EV_KEY write_ev
calls) to convert the incoming key_code to the correct evdev keycode before
sending press/release events, using a dedicated mapping helper if needed so the
numbering matches what Linux expects.
In `@crates/stream/src/main.rs`:
- Around line 25-31: load_trusted_cert is deserializing trusted-certs.json into
the wrong shape, so the certificate lookup by device_id never finds a match.
Update the parsing logic in load_trusted_cert in main.rs to first read the
top-level "hosts" object (or switch to using
TrustedCertsStore/TrustedHostEntry), then look up device_id inside that nested
map and extract cert_pem from the matched entry. Ensure the lookup path matches
the structure written by TrustedCertsStore in pairing.rs.
In `@crates/stream/src/viewer.rs`:
- Around line 93-108: The auth token is currently ignored in connect_channel
because it is bound as _token and never attached to the gRPC client. Update
connect_channel in viewer.rs to preserve the token and use it when building the
Channel, ideally by adding an interceptor/metadata injector like the one used by
FileService so each request carries the authorization credential. Make sure the
resulting client path used by remote_control sends the same auth metadata
instead of connecting anonymously.
In `@docs/adr/0014-screen-streaming-and-remote-control.md`:
- Around line 54-60: Update the enum tables in the ADR to match
stream_service.proto exactly: event_type should be UNSPECIFIED=0, KEYBOARD=1,
MOUSE=2, TOUCH=3; button should be NONE=1, LEFT=2, RIGHT=3, MIDDLE=4; and action
should be PRESS=1, RELEASE=2, MOVE=3. Keep the existing table structure in this
section of the document, and make sure the numeric values reflected in the prose
are consistent with the proto definitions so readers can rely on the ADR as the
source of truth.
- Around line 101-109: The `/dev/uinput` startup error text in the host binary
should no longer suggest `CAP_SYS_ADMIN`, since that is broader than needed.
Update the accessibility check message to keep only the least-privilege
remediation in the relevant startup/error path (the code that emits “/dev/uinput
not accessible” during host initialization), and retain guidance for the `input`
group or a udev ACL/rule while removing the capability-based instruction.
---
Outside diff comments:
In `@crates/agent/src/host.rs`:
- Around line 982-1096: The `quality` argument in `host::run` is currently
unused except for logging, so the advertised low/medium/high preset has no
effect. Either remove it from the CLI/help path if it is not supported, or
thread the preset into the streaming pipeline by updating
`nexus_stream::encode::Encoder::new` and the `enable_streaming` branch so the
encoder actually consumes `quality` and changes behavior accordingly.
---
Minor comments:
In `@crates/stream/src/capture.rs`:
- Around line 47-52: The `tracing::info!` call in `capture.rs` is formatted in a
way that rustfmt rejects, so fix the formatting to match standard Rust style and
rerun formatting on the crate; use `capture.rs` and the `tracing::info!` logging
block as the anchor, and apply `cargo fmt` so the CI `0_fmt & clippy` check
passes.
In `@crates/stream/src/decode.rs`:
- Around line 103-110: The EAGAIN handling in decode::Decoder::decode currently
fabricates a zeroed DecodedFrame, which causes the viewer to render black frames
during buffering. Change decode to return an optional frame result (for example
through the decode method and its callers, including viewer::Viewer::render
flow) so the EAGAIN branch yields no frame and the existing image remains
displayed. Update the caller to skip self.display.render(decoded)? when decode
produces None, and keep the real-frame path unchanged.
- Line 35: rustfmt is flagging formatting differences in the ffmpeg-gated code
in decode.rs, so re-run cargo fmt and commit the resulting formatting changes.
Make sure the cfg(feature = "ffmpeg") blocks and the related items around the
referenced locations are reformatted consistently so CI `0_fmt & clippy` passes.
In `@crates/stream/src/display.rs`:
- Line 1: The file has rustfmt mismatches in the import ordering and a few event
pattern matches, so update the formatting in the affected display module and
re-run cargo fmt. Check the top-level use declarations and the matching logic
around the referenced event-handling blocks in the display code, then let
rustfmt normalize the ordering/pattern layout so CI no longer reports diffs.
In `@crates/stream/src/encode.rs`:
- Around line 47-51: The rustfmt output for FfmpegEncoder in encode.rs is out of
sync with CI, so update the formatting to match cargo fmt. Reformat the affected
blocks in FfmpegEncoder::new and the nearby sections around the referenced
use/import and match/closure code so the diffs at the reported locations
disappear, then verify with cargo fmt before committing.
- Around line 65-66: The PTS generation in encode.rs is using a millisecond
clock while the encoder is configured for 30 fps, so update the timing in the
encode path to use a cadence consistent with the encoder’s frame rate. Adjust
the time base setup in the encoder configuration and the PTS advancement logic
in the frame encode flow so they derive from the capture/frame interval rather
than incrementing by 1 ms per frame. Refer to the encoder setup around
set_time_base and set_frame_rate and the self.pts update in the same encode
flow.
In `@crates/stream/src/host.rs`:
- Around line 7-10: The rustfmt check is failing in the `StreamService`
implementation because several expressions are not formatted consistently. Run
`cargo fmt` and ensure the `use nexus_proto::stream::v1` import, the associated
type declaration, the interval expression, and the `Response::new` call in
`host.rs` are all rewritten to the formatter’s expected style.
- Around line 75-77: The interval setup in host.rs should guard against invalid
capture rates before using capture.fps() in the tokio::time::interval
calculation. Update the logic around the fps() call so fps values at or below
zero are clamped to a safe minimum or handled by an early fallback before
passing them into Duration::from_secs_f64, and make sure the X11Capture stub and
any misconfigured capture implementation cannot reach the interval creation with
a non-positive rate.
- Around line 73-118: The stream loop in host.rs is holding the encoder and
capture mutexes for the entire session, which makes remote_control effectively
single-viewer. Update the logic around the streaming task so that
encoder.lock().await and capture.lock().await are not kept across the whole loop
in the same task; instead acquire them only when needed per iteration or
restructure the shared state so multiple clients can stream concurrently. If
single-viewer is intended, explicitly enforce or document that behavior in the
streaming setup around the spawn block.
In `@crates/stream/src/main.rs`:
- Line 22: Resolve the rustfmt mismatch in the main module by running cargo fmt
and keeping the formatting consistent around the closing brace in main.rs; this
is a formatting-only fix, so adjust the surrounding block in the entrypoint code
if needed and ensure the rustfmt output is clean.
In `@crates/stream/src/viewer.rs`:
- Line 4: Resolve the rustfmt diffs in the viewer module by running cargo fmt
and accepting the resulting import ordering and parameter indentation changes.
Focus on the import block around use tonic::transport::Channel and any affected
function signatures or parameter lists in the same file so the formatting
matches rustfmt’s output and CI passes.
---
Nitpick comments:
In `@crates/agent/src/host.rs`:
- Around line 973-984: The run function has too many positional parameters, and
the clippy allow is only masking the readability issue. Refactor run in host.rs
to take a config struct for related settings, grouping at least
enable_streaming, fps, and quality (and optionally the GC parameters too), then
update the call sites to construct and pass that struct instead of individual
arguments.
In `@crates/proto/proto/stream_service.proto`:
- Around line 46-53: Clarify the coordinate contract for InputEvent.x and
InputEvent.y in stream_service.proto and make it consistent with the injector
path. Update the InputEvent/MouseEvent comments to state whether MOVE uses
absolute screen/window coordinates or relative deltas, then align the downstream
handling in inject.rs so the host interprets the values using the same
convention. Refer to InputEvent, InputEventType, and InputAction when adjusting
the proto and the injection logic.
In `@crates/stream/Cargo.toml`:
- Around line 10-12: The stream crate still pulls in the viewer/display stack
unconditionally, so headless CI cannot avoid the X11/GPU build requirements.
Update the Cargo feature setup in Cargo.toml to mirror the existing ffmpeg gate
by making the winit/pixels/x11 path optional and enabled only through a
dedicated feature, then wire the display-specific code to that feature so the
non-display build stays CI-friendly. Use the existing ffmpeg feature and the
viewer/display-related code paths referenced by the crate to keep the gating
consistent.
- Around line 31-35: Centralize the directly pinned `clap`, `x11`, and `libc`
dependencies used in the `stream` crate by moving their version declarations to
the workspace’s `[workspace.dependencies]` in the root `Cargo.toml`. Then update
the `stream` crate’s dependency entries to reference the workspace versions
consistently, alongside the existing workspace-dependency pattern already used
for `ffmpeg-next`, `winit`, `pixels`, `tokio-stream`, and `futures-util`.
In `@crates/stream/src/decode.rs`:
- Around line 66-69: The packet decode path in `Decoder::decode` only pulls one
frame after `self.video.send_packet(&pkt)?`, which can leave additional decoded
frames buffered. Update the `receive_frame` handling to keep draining frames in
a loop until it returns `EAGAIN`, collecting or forwarding each decoded `Video`
frame before returning. Preserve the existing error handling for non-`EAGAIN`
failures and keep the logic localized around `send_packet`/`receive_frame`.
In `@crates/stream/src/encode.rs`:
- Around line 107-114: The encode path in the frame handling logic only drains
one packet after calling send_frame, which can leave buffered packets queued
behind live output. Update the packet retrieval in the encode routine to keep
calling receive_packet on the same codec context until it returns EAGAIN,
collecting or emitting every available packet before moving on. Use the existing
encode flow around self.ctx.send_frame and receive_packet to locate the fix.
In `@crates/stream/src/host.rs`:
- Around line 124-144: The doc comment for run_stream_host overstates what the
function does by claiming it registers StreamService on a tonic server builder,
but the implementation only initializes and returns a StreamHost. Update the
comment above run_stream_host to describe only the actual behavior: starting the
stream host state by wrapping capture, encoder, and injector in Arc<Mutex<...>>
and returning the StreamHost; remove any mention of server builder registration.
In `@crates/stream/src/main.rs`:
- Around line 9-15: The config directory logic is duplicated in config_dir()
instead of reusing crate::config::config_dir(), which can drift from
TrustedCertsStore’s path handling. Update the main crate to call the existing
config helper and use TrustedCertsStore so trusted-cert path/shape stays
consistent with the rest of the codebase.
In `@crates/stream/src/viewer.rs`:
- Around line 33-37: The Viewer setup is hardcoding the decoder and display size
to 1920×1080, so update the initialization in Viewer::new to derive the
dimensions from the incoming video stream instead of fixed constants. Use the
first VideoFrame’s vf.width and vf.height (or negotiate equivalent dimensions
before creating Decoder and ViewerDisplay) and then pass those values into
Decoder::new and ViewerDisplay::new so the viewer matches the host frame size.
🪄 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 Plus
Run ID: 518f115c-7296-4516-ab81-a1146d677c0b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlcrates/agent/Cargo.tomlcrates/agent/src/host.rscrates/agent/src/main.rscrates/proto/build.rscrates/proto/proto/stream_service.protocrates/proto/src/lib.rscrates/stream/Cargo.tomlcrates/stream/src/capture.rscrates/stream/src/decode.rscrates/stream/src/display.rscrates/stream/src/encode.rscrates/stream/src/host.rscrates/stream/src/inject.rscrates/stream/src/lib.rscrates/stream/src/main.rscrates/stream/src/viewer.rsdocs/adr/0014-screen-streaming-and-remote-control.md
| let router = Server::builder() | ||
| .tls_config(tls_config)? | ||
| .add_service(FileServiceServer::with_interceptor(service, interceptor)) | ||
| .serve(addr) | ||
| .await?; | ||
| .add_service(FileServiceServer::with_interceptor(service, interceptor.clone())); | ||
|
|
||
| let router = if enable_streaming { | ||
| let capture = nexus_stream::capture::X11Capture::new(fps as f64)?; | ||
| let (width, height) = capture.dimensions(); | ||
| let encoder = nexus_stream::encode::Encoder::new(width, height)?; | ||
| let injector = nexus_stream::inject::Injector::new()?; | ||
|
|
||
| let stream_host = | ||
| nexus_stream::host::run_stream_host(capture, encoder, injector).await?; | ||
| let stream_svc = nexus_stream::host::StreamHostService::new(Arc::new(stream_host)); | ||
|
|
||
| tracing::info!(width, height, fps, quality, "screen streaming enabled"); | ||
|
|
||
| router.add_service(StreamServiceServer::with_interceptor(stream_svc, interceptor)) | ||
| } else { | ||
| router | ||
| }; | ||
|
|
||
| router.serve(addr).await?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Streaming setup failure aborts the whole server, including FileService.
X11Capture::new, Encoder::new, Injector::new, and run_stream_host are all invoked with ? before router.serve(addr) is reached. If any of these fail — e.g., no DISPLAY/X11 available, or /dev/uinput lacks the right permissions — run returns early and FileService never starts, even though a user only opted into the additional streaming capability. Given --enable-streaming is an add-on feature layered onto the primary file-serving server, a misconfigured or unsupported streaming environment (headless host, missing uinput permissions) will take down the entire agent.
Consider catching setup failures for the streaming pipeline, logging a warning, and continuing to serve FileService alone (or making the failure mode explicit/documented) rather than failing the whole process.
🛡️ Sketch
let router = if enable_streaming {
- let capture = nexus_stream::capture::X11Capture::new(fps as f64)?;
- let (width, height) = capture.dimensions();
- let encoder = nexus_stream::encode::Encoder::new(width, height)?;
- let injector = nexus_stream::inject::Injector::new()?;
-
- let stream_host =
- nexus_stream::host::run_stream_host(capture, encoder, injector).await?;
- let stream_svc = nexus_stream::host::StreamHostService::new(Arc::new(stream_host));
-
- tracing::info!(width, height, fps, quality, "screen streaming enabled");
-
- router.add_service(StreamServiceServer::with_interceptor(stream_svc, interceptor))
+ match try_build_stream_service(fps, quality.clone(), interceptor.clone()) {
+ Ok(svc) => router.add_service(svc),
+ Err(e) => {
+ tracing::error!("failed to enable screen streaming: {e:#}; continuing without it");
+ router
+ }
+ }
} else {
router
};🧰 Tools
🪛 GitHub Actions: CI / 0_fmt & clippy.txt
[error] 1084-1084: rustfmt check failure: formatting diff detected (run_stream_host call).
[error] 1092-1092: rustfmt check failure: formatting diff detected (StreamServiceServer::with_interceptor call).
🤖 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/agent/src/host.rs` around lines 1077 - 1098, The streaming bootstrap
in the host server currently uses `?` for `X11Capture::new`, `Encoder::new`,
`Injector::new`, and `nexus_stream::host::run_stream_host`, which causes `run`
to abort before `router.serve(addr)` and prevents `FileServiceServer` from
starting. Update the `enable_streaming` branch in `host.rs` to treat streaming
setup as optional: catch and log setup failures with a warning, skip adding
`StreamServiceServer` when initialization fails, and continue serving the
existing router so `FileService` still runs. Keep the success path intact by
only constructing `stream_svc` and calling `router.add_service(...)` after all
streaming components initialize successfully.
|
|
||
| /// Enable screen streaming on the same server (X11 + uinput + FFmpeg required). | ||
| #[arg(long)] | ||
| enable_streaming: bool, | ||
|
|
||
| /// Capture frame rate when streaming (default 30). | ||
| #[arg(long, default_value_t = 30)] | ||
| fps: u32, | ||
|
|
||
| /// Encoding quality preset (low/medium/high). | ||
| #[arg(long, default_value = "medium")] | ||
| quality: String, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== main.rs ==\n'
sed -n '1,120p' crates/agent/src/main.rs
printf '\n== host.rs ==\n'
sed -n '1,220p' crates/stream/src/host.rs
printf '\n== encode.rs ==\n'
sed -n '1,240p' crates/stream/src/encode.rs
printf '\n== search for fps / streaming / ffmpeg references ==\n'
rg -n "fps|enable_streaming|ffmpeg|Duration::from_secs_f64|1.0 / fps|from_secs_f64" crates/agent crates/streamRepository: b5119/nexus02
Length of output: 20760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== crates/agent/src/main.rs (around streaming call) ==\n'
sed -n '112,140p' crates/agent/src/main.rs
printf '\n== crates/agent/src/host.rs (streaming setup) ==\n'
sed -n '1076,1105p' crates/agent/src/host.rs
printf '\n== crates/stream/src/capture.rs ==\n'
sed -n '1,170p' crates/stream/src/capture.rs
printf '\n== crate feature declarations ==\n'
sed -n '1,120p' crates/stream/Cargo.tomlRepository: b5119/nexus02
Length of output: 6768
Clamp --fps to >= 1 and tighten the streaming help text
fps: u32 accepts 0, which flows into Duration::from_secs_f64(1.0 / fps) in crates/stream/src/host.rs and can panic. Add a lower-bound parser. The enable_streaming help text also overstates the FFmpeg requirement: non-ffmpeg builds fall back to a scaffold encoder that emits empty frames, so this should be reworded or made a runtime error.
🤖 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/agent/src/main.rs` around lines 38 - 49, The `Args` fields for `fps`
and `enable_streaming` need adjustment: `fps` currently allows 0, which later
reaches `Duration::from_secs_f64(1.0 / fps)` in `host.rs`, so add a lower-bound
parser or validation to enforce `>= 1`; also update the `enable_streaming` help
text in `main.rs` to avoid implying FFmpeg is always required, since non-ffmpeg
builds use the scaffold encoder fallback, or make that mismatch a runtime
validation if intended.
| let bytes_per_line = unsafe { (*image).bytes_per_line } as usize; | ||
| let size = (self.height as usize) * bytes_per_line; | ||
| let data = unsafe { | ||
| let ptr = (*image).data as *const u8; | ||
| std::slice::from_raw_parts(ptr, size).to_vec() | ||
| }; | ||
|
|
||
| unsafe { | ||
| x11::xlib::XDestroyImage(image); | ||
| } | ||
|
|
||
| Ok(CapturedFrame { | ||
| data, | ||
| width: self.width, | ||
| height: self.height, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline crates/stream/src/capture.rs --view expanded
printf '\n---\n'
ast-grep outline crates/stream/src/encode.rs --view expandedRepository: b5119/nexus02
Length of output: 2185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sections with line numbers
sed -n '1,220p' crates/stream/src/capture.rs | cat -n
printf '\n---\n'
sed -n '1,260p' crates/stream/src/encode.rs | cat -nRepository: b5119/nexus02
Length of output: 13132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for CapturedFrame and any stride/linesize handling
rg -n "CapturedFrame|bytes_per_line|linesize|stride|XGetImage|XDestroyImage|BGRA" crates/stream/srcRepository: b5119/nexus02
Length of output: 1744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/stream/src/capture.rs --view expanded
printf '\n--- capture.rs ---\n'
sed -n '1,220p' crates/stream/src/capture.rs | cat -n
printf '\n--- encode.rs ---\n'
sed -n '1,260p' crates/stream/src/encode.rs | cat -n
printf '\n--- search ---\n'
rg -n "CapturedFrame|bytes_per_line|linesize|stride|XGetImage|XDestroyImage|BGRA" crates/stream/srcRepository: b5119/nexus02
Length of output: 15627
Preserve scanline stride in CapturedFrame
capture_frame drops bytes_per_line, and encode.rs copies the buffer flat into an FFmpeg BGRA frame. If the source or destination stride differs from width * 4, rows will shift and the image will shear. Store stride on CapturedFrame and copy row-by-row, or repack to a tight width * 4 buffer here.
🤖 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/stream/src/capture.rs` around lines 80 - 96, The capture path in
`capture_frame` currently discards the scanline stride, which can cause sheared
output when the buffer is consumed by `encode.rs`. Update `CapturedFrame` to
retain `bytes_per_line` (or repack the data to a tight `width * 4` layout here),
and adjust the copy logic so the FFmpeg upload copies each row using the stored
stride instead of assuming a flat buffer. Use `capture_frame` and
`CapturedFrame` as the key symbols to update together with the consumer in
`encode.rs`.
| let key_code = match kev.physical_key { | ||
| winit::keyboard::PhysicalKey::Code(c) => c as u32, | ||
| _ => 0, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
winit KeyCode enum cast to u32 discriminant value stability
💡 Result:
In Rust, casting an enum to an integer type (such as u32) to retrieve its discriminant value is not guaranteed to be stable across different compilations or library versions unless the enum is explicitly marked with a primitive representation (e.g., #[repr(u32)]) [1]. The winit::keyboard::KeyCode enum does not currently have such a representation, meaning its discriminant values are implementation-defined and subject to change [2][3][1]. Relying on the u32 cast of KeyCode for long-term storage or cross-version compatibility is unsafe because the compiler or the winit crate authors may reorder variants or add new ones, which shifts the implicit discriminant values assigned to each variant [4][1]. For stable identification of physical keys, winit provides explicit alternatives: 1. PhysicalKey: This is the recommended type for tracking physical keys [5][6]. It includes a KeyCode variant for standard keys and an Unidentified(NativeKeyCode) variant for platform-specific raw scancodes, which are more reliable for keybindings [5][7]. 2. Platform-specific scancodes: If you must interact with low-level platform identifiers, use the methods provided by the winit API (such as those for extracting platform-native scancodes) rather than casting the enum directly [8][7]. If you need a stable identifier for a key, you should map KeyCode variants to a custom, stable integer or string representation within your own application code, rather than relying on the winit enum's internal discriminant [9][1].
Citations:
- 1: https://doc.rust-lang.org/1.83.0/reference/items/enumerations.html
- 2: https://docs.rs/winit/latest/winit/keyboard/enum.KeyCode.html
- 3: https://docs.rs/winit/latest/src/winit/keyboard.rs.html
- 4: https://doc.rust-lang.org/std/mem/fn.discriminant.html
- 5: https://github.com/rust-windowing/winit/blob/c4afadbf/winit-core/src/keyboard.rs
- 6: https://docs.rs/winit/latest/winit/keyboard/enum.PhysicalKey.html
- 7: New keyboard API for Windows rust-windowing/winit#1788
- 8: https://github.com/rust-windowing/winit/blob/c4afadbf/winit-win32/src/keyboard.rs
- 9: https://rust-lang.github.io/rfcs/1696-discriminant.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby key handling code.
git ls-files 'crates/stream/src/display.rs' 'crates/stream/src/**' | sed -n '1,120p'
echo '--- display.rs outline ---'
ast-grep outline crates/stream/src/display.rs --view expanded || true
echo '--- nearby lines ---'
sed -n '90,140p' crates/stream/src/display.rs
echo '--- search for KeyCode / uinput mapping ---'
rg -n "KeyCode|PhysicalKey|uinput|KEY_|evdev|scancode|key_code" crates/stream/src -S
echo '--- candidate mapping files ---'
fd -a -t f '.*(key|input|uinput|display).*' crates/stream/src || trueRepository: b5119/nexus02
Length of output: 6128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inject.rs around key handling ---'
sed -n '160,190p' crates/stream/src/inject.rs
echo '--- InputEvent definition / enums ---'
sed -n '1,160p' crates/stream/src/host.rs
echo '--- search for any KeyCode translation tables ---'
rg -n "KeyCode::|PhysicalKey::Code|EV_KEY|KEY_[A-Z0-9_]+|map.*key|translate.*key|linux.*key|uinput" crates/stream/src -SRepository: b5119/nexus02
Length of output: 8146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- protocol definition for InputEvent ---'
rg -n "message InputEvent|enum InputEventType|enum InputAction|key_code|button|x|y" -S .
echo '--- likely proto files ---'
fd -a -t f '.+\.proto$' . || trueRepository: b5119/nexus02
Length of output: 50370
Map winit::keyboard::KeyCode to Linux KEY_* codes here
nexus_proto::stream::v1::InputEvent.key_code is a Linux keycode, and Injector::inject forwards it directly to EV_KEY. PhysicalKey::Code(c) as u32 only serializes winit’s enum discriminant, so the host will inject the wrong keys unless this is translated into the Linux code space.
🤖 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/stream/src/display.rs` around lines 107 - 110, The key_code assignment
in display handling is using the winit enum discriminant instead of a Linux
input code, so `Injector::inject` ends up sending the wrong `EV_KEY` values.
Update the `kev.physical_key` handling in `display.rs` to translate
`winit::keyboard::PhysicalKey::Code` / `KeyCode` into the corresponding Linux
`KEY_*` code before storing it in `InputEvent.key_code`, and keep the fallback
for unmapped keys explicit.
| WindowEvent::CursorMoved { | ||
| position, .. | ||
| } => { | ||
| let ev = InputEvent { | ||
| event_type: InputEventType::Mouse as i32, | ||
| key_code: 0, | ||
| x: position.x as u32, | ||
| y: position.y as u32, | ||
| button: MouseButton::None as i32, | ||
| action: InputAction::Move as i32, | ||
| }; | ||
| self.input_tx.send(ev).ok(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file outline ---\n'
ast-grep outline crates/stream/src/display.rs --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '100,180p' crates/stream/src/display.rs
printf '\n--- search for InputEvent handling and coordinate scaling ---\n'
rg -n "InputEventType::Mouse|MouseInput|CursorMoved|window.*width|host.*width|scale|dpi|position.x|position.y|last known position" crates/stream/src -S
printf '\n--- inspect related types ---\n'
fd -a ".*" crates | rg "input|event|display|stream" || trueRepository: b5119/nexus02
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline crates/stream/src/display.rs --view expanded || true
echo
echo '--- relevant lines ---'
sed -n '100,180p' crates/stream/src/display.rs
echo
echo '--- search for mouse/event handling and scaling ---'
rg -n "InputEventType::Mouse|MouseInput|CursorMoved|position\.x|position\.y|scale|dpi|window.*width|window.*height|host.*width|host.*height|last.*position" crates/stream/src -S || true
echo
echo '--- related files ---'
fd -a -t f crates/stream/src || trueRepository: b5119/nexus02
Length of output: 8577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- display.rs around window creation and event handling ---'
sed -n '1,190p' crates/stream/src/display.rs
echo
echo '--- inject.rs mouse handling ---'
sed -n '160,240p' crates/stream/src/inject.rs
echo
echo '--- search for resize handling in display.rs ---'
rg -n "Resized|ScaleFactorChanged|host_width|host_height|set_inner_size|request_inner_size|LogicalSize|PhysicalSize" crates/stream/src/display.rs -SRepository: b5119/nexus02
Length of output: 12143
Send mouse moves as deltas, not window coordinates.
CursorMoved forwards position.x/y straight into EV_REL, so each event injects the full cursor position instead of the movement since the last event. Compute and send relative deltas here.
🧰 Tools
🪛 GitHub Actions: CI / 0_fmt & clippy.txt
[error] 135-135: rustfmt check failure: formatting diff detected in WindowEvent::MouseInput pattern.
🤖 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/stream/src/display.rs` around lines 125 - 137, The CursorMoved handler
in display::stream::display.rs is sending absolute window coordinates into the
mouse input path instead of relative movement. Update the
WindowEvent::CursorMoved branch to track the previous cursor position and send
deltas (current minus last) in the InputEvent you build, while keeping the
existing InputEvent, InputAction::Move, and MouseButton::None flow intact. Make
sure the logic in the CursorMoved match arm computes and forwards relative x/y
values before calling self.input_tx.send.
| t if t == InputEventType::Mouse as i32 => { | ||
| match event.action { | ||
| a if a == InputAction::Move as i32 => { | ||
| self.write_ev(sys::EV_REL, sys::REL_X, event.x as i32)?; | ||
| self.write_ev(sys::EV_REL, sys::REL_Y, event.y as i32)?; | ||
| self.sync()?; | ||
| } | ||
| a if a == InputAction::Press as i32 => { | ||
| let btn = match event.button { | ||
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | ||
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | ||
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | ||
| _ => return Ok(()), | ||
| }; | ||
| self.write_ev(sys::EV_KEY, btn, 1)?; | ||
| self.sync()?; | ||
| } | ||
| a if a == InputAction::Release as i32 => { | ||
| let btn = match event.button { | ||
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | ||
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | ||
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | ||
| _ => return Ok(()), | ||
| }; | ||
| self.write_ev(sys::EV_KEY, btn, 0)?; | ||
| self.sync()?; | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Mouse Move injected via EV_REL but the viewer sends absolute coordinates.
The viewer's display.rs (context) sends the raw CursorMoved window position (absolute pixel coordinates) as x/y for InputAction::Move. Here, write_ev(sys::EV_REL, sys::REL_X, event.x as i32) treats those values as relative motion deltas. EV_REL values are interpreted by the kernel as deltas from the current pointer position, so injecting an absolute coordinate (e.g. 800) will cause the cursor to jump by 800px on every frame rather than moving to (800, y) — making mouse control effectively unusable.
Since EV_ABS/ABS_X/ABS_Y are already registered on this device (lines 118-119) for touch, switching mouse Move to EV_ABS is a minimal local fix.
🐛 Proposed fix
a if a == InputAction::Move as i32 => {
- self.write_ev(sys::EV_REL, sys::REL_X, event.x as i32)?;
- self.write_ev(sys::EV_REL, sys::REL_Y, event.y as i32)?;
+ self.write_ev(sys::EV_ABS, sys::ABS_X, event.x as i32)?;
+ self.write_ev(sys::EV_ABS, sys::ABS_Y, event.y as i32)?;
self.sync()?;
}📝 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.
| t if t == InputEventType::Mouse as i32 => { | |
| match event.action { | |
| a if a == InputAction::Move as i32 => { | |
| self.write_ev(sys::EV_REL, sys::REL_X, event.x as i32)?; | |
| self.write_ev(sys::EV_REL, sys::REL_Y, event.y as i32)?; | |
| self.sync()?; | |
| } | |
| a if a == InputAction::Press as i32 => { | |
| let btn = match event.button { | |
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | |
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | |
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | |
| _ => return Ok(()), | |
| }; | |
| self.write_ev(sys::EV_KEY, btn, 1)?; | |
| self.sync()?; | |
| } | |
| a if a == InputAction::Release as i32 => { | |
| let btn = match event.button { | |
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | |
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | |
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | |
| _ => return Ok(()), | |
| }; | |
| self.write_ev(sys::EV_KEY, btn, 0)?; | |
| self.sync()?; | |
| } | |
| _ => {} | |
| } | |
| } | |
| t if t == InputEventType::Mouse as i32 => { | |
| match event.action { | |
| a if a == InputAction::Move as i32 => { | |
| self.write_ev(sys::EV_ABS, sys::ABS_X, event.x as i32)?; | |
| self.write_ev(sys::EV_ABS, sys::ABS_Y, event.y as i32)?; | |
| self.sync()?; | |
| } | |
| a if a == InputAction::Press as i32 => { | |
| let btn = match event.button { | |
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | |
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | |
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | |
| _ => return Ok(()), | |
| }; | |
| self.write_ev(sys::EV_KEY, btn, 1)?; | |
| self.sync()?; | |
| } | |
| a if a == InputAction::Release as i32 => { | |
| let btn = match event.button { | |
| b if b == MouseButton::Left as i32 => sys::BTN_LEFT, | |
| b if b == MouseButton::Right as i32 => sys::BTN_RIGHT, | |
| b if b == MouseButton::Middle as i32 => sys::BTN_MIDDLE, | |
| _ => return Ok(()), | |
| }; | |
| self.write_ev(sys::EV_KEY, btn, 0)?; | |
| self.sync()?; | |
| } | |
| _ => {} | |
| } | |
| } |
🤖 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/stream/src/inject.rs` around lines 184 - 213, Mouse Move is being
injected as relative motion even though InputAction::Move carries absolute
cursor coordinates from the viewer. Update the InputEventType::Mouse handling in
inject.rs so the Move branch uses the device’s absolute axes (EV_ABS with
ABS_X/ABS_Y) instead of EV_REL/REL_X/REL_Y, matching the existing ABS
registration on this input device. Keep Press/Release behavior unchanged and
route the x/y values directly as absolute positions in the same write_ev/sync
flow.
| | event_type | enum | KEYBOARD = 0, MOUSE = 1, TOUCH = 2 | | ||
| | key_code | uint32 | Linux keycode (when type == KEYBOARD) | | ||
| | x | uint32 | X coordinate (mouse/touch), normalized to host res | | ||
| | y | uint32 | Y coordinate (mouse/touch), normalized to host res | | ||
| | button | enum | NONE = 0, LEFT = 1, RIGHT = 2, MIDDLE = 3 | | ||
| | action | enum | PRESS = 0, RELEASE = 1, MOVE = 2 | | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the enum tables with stream_service.proto.
These numeric values are off: the proto uses UNSPECIFIED=0, KEYBOARD=1, MOUSE=2, TOUCH=3, NONE=1, LEFT=2, RIGHT=3, MIDDLE=4, and PRESS=1, RELEASE=2, MOVE=3. Leaving the ADR with zero-based values will mislead any client or host implementation that follows the doc instead of the proto.
♻️ Proposed doc fix
-| event_type | enum | KEYBOARD = 0, MOUSE = 1, TOUCH = 2 |
+| event_type | enum | UNSPECIFIED = 0, KEYBOARD = 1, MOUSE = 2, TOUCH = 3 |
@@
-| button | enum | NONE = 0, LEFT = 1, RIGHT = 2, MIDDLE = 3 |
+| button | enum | NONE = 1, LEFT = 2, RIGHT = 3, MIDDLE = 4 |
@@
-| action | enum | PRESS = 0, RELEASE = 1, MOVE = 2 |
+| action | enum | PRESS = 1, RELEASE = 2, MOVE = 3 |📝 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.
| | event_type | enum | KEYBOARD = 0, MOUSE = 1, TOUCH = 2 | | |
| | key_code | uint32 | Linux keycode (when type == KEYBOARD) | | |
| | x | uint32 | X coordinate (mouse/touch), normalized to host res | | |
| | y | uint32 | Y coordinate (mouse/touch), normalized to host res | | |
| | button | enum | NONE = 0, LEFT = 1, RIGHT = 2, MIDDLE = 3 | | |
| | action | enum | PRESS = 0, RELEASE = 1, MOVE = 2 | | |
| | event_type | enum | UNSPECIFIED = 0, KEYBOARD = 1, MOUSE = 2, TOUCH = 3 | | |
| | key_code | uint32 | Linux keycode (when type == KEYBOARD) | | |
| | x | uint32 | X coordinate (mouse/touch), normalized to host res | | |
| | y | uint32 | Y coordinate (mouse/touch), normalized to host res | | |
| | button | enum | NONE = 1, LEFT = 2, RIGHT = 3, MIDDLE = 4 | | |
| | action | enum | PRESS = 1, RELEASE = 2, MOVE = 3 | |
🤖 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/adr/0014-screen-streaming-and-remote-control.md` around lines 54 - 60,
Update the enum tables in the ADR to match stream_service.proto exactly:
event_type should be UNSPECIFIED=0, KEYBOARD=1, MOUSE=2, TOUCH=3; button should
be NONE=1, LEFT=2, RIGHT=3, MIDDLE=4; and action should be PRESS=1, RELEASE=2,
MOVE=3. Keep the existing table structure in this section of the document, and
make sure the numeric values reflected in the prose are consistent with the
proto definitions so readers can rely on the ADR as the source of truth.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (1)
92-93: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winClippy doesn't lint ffmpeg-gated code paths.
The clippy step runs without
--features nexus-stream/ffmpeg, so all code behind#[cfg(feature = "ffmpeg")]— including the realEncoderimplementation, thenexus-viewerbinary (which hasrequired-features = ["ffmpeg"]), and the loopback integration test — is excluded from linting. Since clippy is enforced with-D warnings, this creates a blind spot for a significant portion of the newnexus-streamcrate.Add a second clippy invocation with the ffmpeg feature enabled:
🔧 Proposed fix
- name: clippy run: cargo clippy --workspace --all-targets -- -D warnings + + - name: clippy (ffmpeg feature) + run: cargo clippy --features nexus-stream/ffmpeg --workspace --all-targets -- -D warnings🤖 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/ci.yml around lines 92 - 93, Clippy is only running on the default feature set, so the ffmpeg-gated parts of the workspace are not being linted. Update the CI clippy step in the workflow to add a second cargo clippy invocation that enables the ffmpeg feature for nexus-stream, so the #[cfg(feature = "ffmpeg")] Encoder path, the nexus-viewer binary guarded by required-features = ["ffmpeg"], and the loopback integration test are all covered. Keep the existing clippy job and extend it with the additional feature-enabled run using the same -D warnings enforcement.crates/stream/src/main.rs (1)
30-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire token-mode credentials before connecting.
The non-
--trustedpath passesNonefor CA/client materials and allows an empty token, but the agent auth contract requires a validx-nexus-tokenwhen no client certificate is presented. Mirror the mount CLI by requiring--tokenin token mode and add/pass a--ca-certinput for the host certificate.Also applies to: 96-98
🤖 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/stream/src/main.rs` around lines 30 - 32, The token-mode connection flow currently allows missing credentials, which violates the auth contract for non-`--trusted` connections. Update the CLI around the `token` option in `main` to require `--token` when no client certificate is used, and add a `--ca-cert` input so the host certificate can be passed through. Then wire both `token` and `ca-cert` into the connection setup path so the non-trusted mode always provides the required `x-nexus-token` plus CA material, matching the mount CLI behavior.
🧹 Nitpick comments (1)
crates/stream/tests/loopback.rs (1)
42-53: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBlocking FFmpeg encode on the async runtime.
encoder.encode(frame)is synchronous CPU-intensive work executed while holding atokio::sync::Mutexlock inside an async task. This blocks the tokio worker thread for the duration of each encode. For a test this is tolerable, but considertokio::task::spawn_blockingif the encode path is ever reused outside tests.🤖 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/stream/tests/loopback.rs` around lines 42 - 53, The `encoder.encode(frame)` call inside the `tokio::spawn` block is synchronous CPU-heavy work being run on the async runtime while holding the `tokio::sync::Mutex` guard. Update the test helper around `encoder.encode` so the blocking encode work is moved onto `tokio::task::spawn_blocking` (or otherwise kept off the core runtime), and keep the mutex hold time minimal around `encoder` in `loopback.rs` to avoid tying up a Tokio worker thread.
🤖 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/fs/src/main.rs`:
- Around line 246-252: The mount hint in main’s host parsing is stripping
everything after the last colon, which breaks bracketed IPv6 hosts; update the
logic that computes host_part to match pair_with_host’s digit-only port
detection instead of using rsplit_once unconditionally. Keep the existing scheme
trimming, but only remove the suffix when it is a numeric port, so the println!
remote URL remains valid for IPv6 and other colon-containing hosts.
In `@crates/stream/src/inject.rs`:
- Around line 315-323: The test in Injector::new/inject currently sends an
EV_KEY press without a matching release, which can leave key_code 42 stuck when
/dev/uinput is available. Update the test to only run the inject/assert path
when has_uinput indicates the missing-device case is being exercised, or send a
matching release event after the press so the virtual key is not latched.
In `@crates/stream/src/main.rs`:
- Around line 82-95: The host selection logic in the device lookup currently
allows cert to be None, which lets execution continue without a trusted host CA.
Update the lookup path in main to fail immediately when the selected host entry
for device_id does not contain cert_pem, returning an error instead of building
the tuple with None. Use the existing device_id/hosts lookup and the cert
extraction chain to locate the change, and keep the client_cert/client_key
loading behavior unchanged.
In `@crates/stream/tests/loopback.rs`:
- Around line 32-33: The test file has rustfmt violations across the type alias,
helper setup, and test blocks, so the fix is to run cargo fmt --all and commit
the resulting formatting-only changes. Make sure the formatting is applied
consistently throughout loopback.rs, especially around the RemoteControlStream
alias and the affected test functions, without changing any test logic.
- Around line 113-119: The spawned server task in the loopback test is using an
unwrap on the result of Server::serve_with_incoming, which hides the real
failure and causes the test to time out instead of failing fast. Update the
tokio::spawn block in loopback.rs to surface the server startup/runtime error,
either by replacing unwrap with a descriptive expect or by sending the error
back to the test through a channel so the test can report the actual cause
immediately.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 92-93: Clippy is only running on the default feature set, so the
ffmpeg-gated parts of the workspace are not being linted. Update the CI clippy
step in the workflow to add a second cargo clippy invocation that enables the
ffmpeg feature for nexus-stream, so the #[cfg(feature = "ffmpeg")] Encoder path,
the nexus-viewer binary guarded by required-features = ["ffmpeg"], and the
loopback integration test are all covered. Keep the existing clippy job and
extend it with the additional feature-enabled run using the same -D warnings
enforcement.
In `@crates/stream/src/main.rs`:
- Around line 30-32: The token-mode connection flow currently allows missing
credentials, which violates the auth contract for non-`--trusted` connections.
Update the CLI around the `token` option in `main` to require `--token` when no
client certificate is used, and add a `--ca-cert` input so the host certificate
can be passed through. Then wire both `token` and `ca-cert` into the connection
setup path so the non-trusted mode always provides the required `x-nexus-token`
plus CA material, matching the mount CLI behavior.
---
Nitpick comments:
In `@crates/stream/tests/loopback.rs`:
- Around line 42-53: The `encoder.encode(frame)` call inside the `tokio::spawn`
block is synchronous CPU-heavy work being run on the async runtime while holding
the `tokio::sync::Mutex` guard. Update the test helper around `encoder.encode`
so the blocking encode work is moved onto `tokio::task::spawn_blocking` (or
otherwise kept off the core runtime), and keep the mutex hold time minimal
around `encoder` in `loopback.rs` to avoid tying up a Tokio worker thread.
🪄 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 Plus
Run ID: 10321c11-6774-4695-8e6c-e2d586b8ec8e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/ci.ymlCargo.tomlcrates/agent/src/host.rscrates/agent/src/main.rscrates/fs/src/main.rscrates/fs/src/pairing.rscrates/stream/Cargo.tomlcrates/stream/src/capture.rscrates/stream/src/decode.rscrates/stream/src/display.rscrates/stream/src/encode.rscrates/stream/src/host.rscrates/stream/src/inject.rscrates/stream/src/lib.rscrates/stream/src/main.rscrates/stream/src/viewer.rscrates/stream/tests/loopback.rs
💤 Files with no reviewable changes (1)
- crates/agent/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/stream/src/lib.rs
- crates/stream/Cargo.toml
- crates/stream/src/host.rs
- crates/stream/src/decode.rs
- crates/agent/src/host.rs
- crates/stream/src/encode.rs
- crates/stream/src/display.rs
| let host_part = host | ||
| .trim_start_matches("http://") | ||
| .trim_start_matches("https://") | ||
| .rsplit_once(':') | ||
| .map(|(h, _)| h) | ||
| .unwrap_or(host.trim_start_matches("http://").trim_start_matches("https://")); | ||
| println!("To mount, run: NEXUS_TRUSTED_HOST_ID={host_id} nexus-mount mount --remote https://{host_part}:50051 --mountpoint <dir> --trusted"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only strip a port when the suffix is numeric.
This unconditionally truncates after the last colon, so a bracketed IPv6 host like [::1] produces a broken hint. Match pair_with_host’s digit-only port detection.
🐛 Proposed fix
- let host_part = host
+ let input = host
.trim_start_matches("http://")
- .trim_start_matches("https://")
- .rsplit_once(':')
- .map(|(h, _)| h)
- .unwrap_or(host.trim_start_matches("http://").trim_start_matches("https://"));
+ .trim_start_matches("https://");
+ let host_part = match input.rsplit_once(':') {
+ Some((h, p)) if p.chars().all(|c| c.is_ascii_digit()) => h,
+ _ => input,
+ };📝 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.
| let host_part = host | |
| .trim_start_matches("http://") | |
| .trim_start_matches("https://") | |
| .rsplit_once(':') | |
| .map(|(h, _)| h) | |
| .unwrap_or(host.trim_start_matches("http://").trim_start_matches("https://")); | |
| println!("To mount, run: NEXUS_TRUSTED_HOST_ID={host_id} nexus-mount mount --remote https://{host_part}:50051 --mountpoint <dir> --trusted"); | |
| let input = host | |
| .trim_start_matches("http://") | |
| .trim_start_matches("https://"); | |
| let host_part = match input.rsplit_once(':') { | |
| Some((h, p)) if p.chars().all(|c| c.is_ascii_digit()) => h, | |
| _ => input, | |
| }; | |
| println!("To mount, run: NEXUS_TRUSTED_HOST_ID={host_id} nexus-mount mount --remote https://{host_part}:50051 --mountpoint <dir> --trusted"); |
🧰 Tools
🪛 GitHub Actions: CI / fmt & clippy
[error] 248-248: cargo fmt --all --check failed (rustfmt formatting mismatch). Run 'cargo fmt --all' to apply formatting.
🤖 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/fs/src/main.rs` around lines 246 - 252, The mount hint in main’s host
parsing is stripping everything after the last colon, which breaks bracketed
IPv6 hosts; update the logic that computes host_part to match pair_with_host’s
digit-only port detection instead of using rsplit_once unconditionally. Keep the
existing scheme trimming, but only remove the suffix when it is a numeric port,
so the println! remote URL remains valid for IPv6 and other colon-containing
hosts.
| let event = InputEvent { | ||
| event_type: InputEventType::Keyboard as i32, | ||
| action: InputAction::Press as i32, | ||
| key_code: 42, | ||
| x: 0, | ||
| y: 0, | ||
| button: 0, | ||
| }; | ||
| assert!(injector.inject(&event).is_ok(), "inject must not fail even when uinput is missing"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Test injects a key press with no release, leaving a stuck key when /dev/uinput is accessible.
When the device is available (root/input-group dev boxes or CI), Injector::new() creates a real virtual device and this inject call emits EV_KEY press (value 1) for key_code: 42 with no matching release, leaving that key latched on the host. Since the test's purpose is the missing-device path, guard the inject/assert on has_uinput, or emit a release.
💚 Proposed fix
- assert!(injector.inject(&event).is_ok(), "inject must not fail even when uinput is missing");
- if !has_uinput {
+ if !has_uinput {
+ assert!(injector.inject(&event).is_ok(), "inject must not fail when uinput is missing");
assert!(injector.fd.is_none(), "fd should be None when uinput is missing");
}📝 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.
| let event = InputEvent { | |
| event_type: InputEventType::Keyboard as i32, | |
| action: InputAction::Press as i32, | |
| key_code: 42, | |
| x: 0, | |
| y: 0, | |
| button: 0, | |
| }; | |
| assert!(injector.inject(&event).is_ok(), "inject must not fail even when uinput is missing"); | |
| let event = InputEvent { | |
| event_type: InputEventType::Keyboard as i32, | |
| action: InputAction::Press as i32, | |
| key_code: 42, | |
| x: 0, | |
| y: 0, | |
| button: 0, | |
| }; | |
| if !has_uinput { | |
| assert!(injector.inject(&event).is_ok(), "inject must not fail when uinput is missing"); | |
| assert!(injector.fd.is_none(), "fd should be None when uinput is missing"); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI / fmt & clippy
[error] 320-320: cargo fmt --all --check failed (rustfmt formatting mismatch). Run 'cargo fmt --all' to apply formatting.
🤖 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/stream/src/inject.rs` around lines 315 - 323, The test in
Injector::new/inject currently sends an EV_KEY press without a matching release,
which can leave key_code 42 stuck when /dev/uinput is available. Update the test
to only run the inject/assert path when has_uinput indicates the missing-device
case is being exercised, or send a matching release event after the press so the
virtual key is not latched.
| let cert = hosts | ||
| .get(&device_id) | ||
| .and_then(|e| e.get("cert_pem")) | ||
| .and_then(|c| c.as_str()) | ||
| .map(|c| c.to_string()); | ||
|
|
||
| // Load client identity cert+key for mTLS. | ||
| let cfg_dir = config_dir()?; | ||
| let client_cert = std::fs::read_to_string(cfg_dir.join("cert.pem")) | ||
| .with_context(|| format!("reading cert.pem from {}", cfg_dir.display()))?; | ||
| let client_key = std::fs::read_to_string(cfg_dir.join("key.pem")) | ||
| .with_context(|| format!("reading key.pem from {}", cfg_dir.display()))?; | ||
|
|
||
| (device_id, cert, Some(client_cert), Some(client_key)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast when the selected paired host has no certificate.
--trusted --host-id <missing> currently yields cert = None, then proceeds with client identity but no host CA, so trusted mTLS is never configured downstream. Return an error at lookup time instead.
🐛 Proposed fix
- let cert = hosts
+ let cert = hosts
.get(&device_id)
.and_then(|e| e.get("cert_pem"))
.and_then(|c| c.as_str())
- .map(|c| c.to_string());
+ .map(|c| c.to_string())
+ .with_context(|| format!("paired host {device_id} not found in trusted-certs.json"))?;🤖 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/stream/src/main.rs` around lines 82 - 95, The host selection logic in
the device lookup currently allows cert to be None, which lets execution
continue without a trusted host CA. Update the lookup path in main to fail
immediately when the selected host entry for device_id does not contain
cert_pem, returning an error instead of building the tuple with None. Use the
existing device_id/hosts lookup and the cert extraction chain to locate the
change, and keep the client_cert/client_key loading behavior unchanged.
| tokio::spawn(async move { | ||
| Server::builder() | ||
| .add_service(StreamServiceServer::new(host)) | ||
| .serve_with_incoming(incoming) | ||
| .await | ||
| .unwrap(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Server .unwrap() in spawned task masks real failures.
If serve_with_incoming fails, the panic occurs in a detached spawned task and the test will hang for 10 seconds before timing out with a misleading "timed out waiting for frame" message instead of surfacing the actual server error. Use expect with a descriptive message, or propagate the error via a channel so the test fails fast with the real cause.
🛡️ Suggested fix: propagate server error
tokio::spawn(async move {
- Server::builder()
- .add_service(StreamServiceServer::new(host))
- .serve_with_incoming(incoming)
- .await
- .unwrap();
+ if let Err(e) = Server::builder()
+ .add_service(StreamServiceServer::new(host))
+ .serve_with_incoming(incoming)
+ .await
+ {
+ eprintln!("server error: {e}");
+ }
});🤖 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/stream/tests/loopback.rs` around lines 113 - 119, The spawned server
task in the loopback test is using an unwrap on the result of
Server::serve_with_incoming, which hides the real failure and causes the test to
time out instead of failing fast. Update the tokio::spawn block in loopback.rs
to surface the server startup/runtime error, either by replacing unwrap with a
descriptive expect or by sending the error back to the test through a channel so
the test can report the actual cause immediately.
1 participant
Summary
nexus-streamcrate (capture/encode/decode/inject/display/host/viewer — 1,446 lines)nexus-viewerbinary (standalone viewer CLI)StreamServiceproto (bidirectional streaming RPC)stream-hostintegrated intonexus-agent servevia--enable-streamingflag (same port 50051, same auth interceptor asFileService)XGetImageKnown Limitations (from ADR 0014)
References
CI Note
CI runs without the ffmpeg feature (default off), so the ffmpeg-gated code is not exercised in CI — this is a known limitation of the optional feature approach, documented in ADR 0014.
Summary by CodeRabbit