Skip to content

Conversation

@kixelated
Copy link
Collaborator

and some nix stuff

@coderabbitai
Copy link

coderabbitai bot commented Dec 27, 2025

Walkthrough

This pull request adds rust-overlay to the Nix flake configuration and consolidates Rust tooling into a single rust-toolchain derivation with rust-src and rust-analyzer extensions targeting wasm32-unknown-unknown. Across multiple web-transport modules (quiche, trait, wasm, ws, quinn), pattern matching in RecvStream read operations was refined to treat zero-byte or empty reads as end-of-stream (None) instead of returning empty data chunks.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is vague and generic, using non-descriptive terms ('nix stuff') that don't convey meaningful information about the changeset. Expand the description to explain the purpose of the read_buf changes and how the nix modifications support them.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main focus of the changeset: adding safety checks to read_buf implementations across multiple files to handle zero-byte reads properly.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2db253 and c21da4f.

⛔ Files ignored due to path filters (1)
  • flake.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • flake.nix
  • web-transport-quiche/src/ez/recv.rs
  • web-transport-trait/src/lib.rs
  • web-transport-wasm/src/recv.rs
  • web-transport-ws/src/session.rs
  • web-transport/src/quinn.rs
🧰 Additional context used
🧬 Code graph analysis (3)
web-transport/src/quinn.rs (2)
web-transport-ws/src/varint.ts (1)
  • size (17-24)
web-transport-proto/src/varint.rs (1)
  • size (53-66)
web-transport-ws/src/session.rs (2)
web-transport-ws/src/varint.ts (1)
  • size (17-24)
web-transport-proto/src/varint.rs (1)
  • size (53-66)
web-transport-trait/src/lib.rs (2)
web-transport-ws/src/varint.ts (1)
  • size (17-24)
web-transport-proto/src/varint.rs (1)
  • size (53-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (10)
flake.nix (4)

7-10: LGTM!

The rust-overlay input is correctly configured with nixpkgs.follows to ensure consistent nixpkgs usage and avoid duplicate derivations.


18-27: LGTM!

The overlay application follows the standard rust-overlay pattern. Importing nixpkgs with the overlay correctly makes pkgs.rust-bin available.


29-35: LGTM!

The rust-toolchain configuration is well-structured with appropriate extensions for IDE support and the wasm32-unknown-unknown target. The default profile includes rustc, cargo, rustfmt, and clippy, consolidating what were previously separate entries.

Note: Using stable.latest means the Rust version may change when nixpkgs updates. If reproducibility is critical, consider pinning to a specific version (e.g., pkgs.rust-bin.stable."1.83.0".default).


37-59: LGTM!

The consolidation from individual Rust tools to a single rust-toolchain entry is cleaner and ensures all components (rustc, cargo, rustfmt, clippy) come from the same toolchain version. The shellHook reformatting improves readability.

web-transport-ws/src/session.rs (1)

703-710: LGTM: Zero-byte reads now correctly treated as end-of-stream.

The refined pattern matching ensures Some(size) is only returned when actual data (!data.is_empty()) is read. Empty chunks and None from read_chunk are both mapped to Ok(None), which correctly signals end-of-stream or no progress. This aligns with Rust async I/O semantics and prevents ambiguity when read_buf returns Some(0).

web-transport-wasm/src/recv.rs (1)

57-60: LGTM: Consistent zero-byte read handling.

The tightened match guard (!chunk.is_empty()) ensures that only non-empty chunks are treated as valid data. This change aligns the WASM implementation with the pattern applied across other modules, where zero-length reads signal end-of-stream rather than a successful read of zero bytes.

web-transport/src/quinn.rs (1)

261-264: LGTM: Zero-size reads properly handled.

The guard condition size > 0 ensures that only positive-sized reads are treated as data. Zero-size reads and None from the underlying self.inner.read() are both routed to Ok(None), correctly signaling end-of-stream. This matches the pattern applied in other transport implementations.

web-transport-quiche/src/ez/recv.rs (1)

285-290: LGTM: Zero-byte reads no longer advance buffers.

The condition n > 0 ensures that Some(n) is only returned when data is actually read. Zero-byte reads fall through to the catch-all arm returning Ok(None), preventing the buffer from being advanced without reading data. This aligns with the behavior in other transport layers.

web-transport-trait/src/lib.rs (2)

197-200: LGTM: Trait contract tightened for zero-byte reads.

The size > 0 guard ensures that read_buf implementations consistently return Some(n) only when n > 0. Zero-size and None reads both yield Ok(None), establishing a clear contract: Some(n) means data was read, None means end-of-stream or no progress. This trait-level change ensures all implementations follow the same semantics.


256-258: LGTM: Consistent treatment in read loop.

The n > 0 check in read_all_buf ensures the loop only continues when actual data is read. Zero-byte reads trigger the break, treating them as end-of-stream. This prevents infinite loops when the underlying stream returns Some(0) and aligns with the updated read_buf contract.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated kixelated enabled auto-merge (squash) December 27, 2025 10:57
@kixelated kixelated merged commit fe44b6d into main Jan 7, 2026
1 check passed
@kixelated kixelated deleted the safer-read-buf branch January 7, 2026 04:10
@github-actions github-actions bot mentioned this pull request Jan 7, 2026
@github-actions github-actions bot mentioned this pull request Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants