Skip to content

Conversation

@kixelated
Copy link
Collaborator

Summary

  • Refactors Client and Server session builders to use a fluent builder pattern
  • Adds with_publish() and with_consume() methods for configuring sessions
  • The with_stats() methods are commented out pending the observability feature merge from feat/observability

Test plan

  • just check passes
  • Manual testing of client/server connections

🤖 Generated with Claude Code

This refactors the Client and Server session builders to use a fluent
builder pattern with with_publish() and with_consume() methods.

The with_stats() methods are commented out pending the observability
feature merge.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 23, 2026

Walkthrough

The PR refactors MoQ session setup from positional calls (e.g. connect(url, publish, consume) / accept(publish, consume)) to a builder pattern using with_publish() and with_consume() followed by connect() or accept(). It adds Client and Server types and builder methods in moq-lite, updates callers across multiple crates to the new API, adjusts Session visibility and constructors, and replaces several mutable bindings with immutable or shadowed bindings.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: introducing a builder pattern for client and server construction, which is the core refactoring across all modified files.
Description check ✅ Passed The description is directly related to the changeset, explaining the builder pattern refactoring, the new methods, and the commented-out observability placeholders.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@rs/hang/examples/video.rs`:
- Around line 30-33: The comment above the call to
client.with_publish(origin).connect(url).await? is inaccurate: it refers to
"consuming" and `OriginProducer` but this line uses `with_publish()` which
accepts an `OriginConsumer` and indicates we are only publishing (omitting
`with_consume()`). Update the comment to state that this establishes a
WebTransport/QUIC connection and MoQ handshake for publishing only, that
`with_publish(origin)` registers an OriginConsumer for outgoing data, and note
that `with_consume()` would be used if we wanted to subscribe/consume; keep the
reference to optional WebSocket fallback if desired.

In `@rs/moq-lite/src/server.rs`:
- Around line 85-101: The code wrongly decodes server.parameters instead of the
peer's parameters: change the call to use client.parameters when deriving
request_id_max so the server reads the client's MaxRequestId (use
ietf::Parameters::decode(&mut client.parameters, version)), keep the rest of the
logic that constructs request_id_max (ietf::RequestId(...)) and passes it into
ietf::start(session.clone(), stream, request_id_max, false,
self.publish.clone(), self.consume.clone(), version). This ensures the server
learns the client's maximum request ID rather than re-reading its own.
🧹 Nitpick comments (4)
rs/moq-lite/src/server.rs (1)

107-110: Consider if session.clone() is needed before wrapping in Session::new.

On line 109, session.clone() is called, but the session is moved into the Session::new() wrapper. Since Session::new already wraps the session in an Arc, the clone may be unnecessary unless it's needed for the lite::start / ietf::start calls earlier (which do use session.clone()).

If the clone is intentional for symmetry, consider adding a brief comment explaining why.

Suggested simplification
-		Ok(Session::new(session.clone()))
+		Ok(Session::new(session))
rs/moq-native/src/client.rs (2)

252-270: Review the error handling in the transport race.

The tokio::select! pattern has a subtle issue: if WebSocket is disabled (ws_handle returns None), and QUIC fails with an error, the else branch triggers with "failed to connect to server" rather than surfacing the actual QUIC error.

Consider propagating the QUIC error when WebSocket is disabled:

Suggested improvement for better error reporting
 		// Race the connection futures
 		Ok(tokio::select! {
 			Ok(quic) = quic_handle => self.moq.connect(quic).await?,
 			Some(Ok(ws)) = ws_handle => self.moq.connect(ws).await?,
 			// If both attempts fail, return an error
-			else => anyhow::bail!("failed to connect to server"),
+			else => {
+				if !self.websocket.enabled {
+					anyhow::bail!("QUIC connection failed (WebSocket disabled)");
+				}
+				anyhow::bail!("failed to connect to server via QUIC or WebSocket");
+			}
 		})

339-341: Redundant enabled check.

The connect_websocket method is only called from ws_handle which already guards on self.websocket.enabled. The anyhow::ensure! on line 340 is defensive but will never trigger in the current code flow.

This is fine to keep for safety if the method might be called directly in the future.

rs/moq-native/src/server.rs (1)

248-256: Consider documenting the server cloning semantics.

The moq_lite::Server is cloned for each incoming connection (self.moq.clone()). This means each Request gets its own copy of the publish/consume configuration, which is correct for the per-request builder pattern (Request::with_publish/with_consume).

However, if OriginConsumer/OriginProducer contain shared state (e.g., Arc internally), changes to the Request's server won't affect other connections, which is likely the intended behavior.

A brief doc comment on Server::accept or the moq field clarifying that each request gets an independent copy of the server config would help future maintainers.

Also applies to: 278-278, 306-317, 320-320, 330-343

- Fix comment in video.rs to accurately describe with_publish()
- Fix server.rs to decode client.parameters instead of server.parameters
  to get the client's MaxRequestId

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) January 23, 2026 16:36
@kixelated kixelated merged commit d277555 into main Jan 23, 2026
1 check passed
@kixelated kixelated deleted the feat/builder-pattern branch January 23, 2026 16:44
This was referenced 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