Move some examples into code. #596
Conversation
WalkthroughAdds development-only dependencies to rs/hang/Cargo.toml and rs/moq/Cargo.toml. Replaces inline examples in rs/hang/README.md and rs/moq/README.md with links. Introduces two new examples: rs/hang/examples/video.rs (video broadcast with origin/session tasks) and rs/moq/examples/chat.rs (chat broadcast with concurrent broadcast/session tasks). Adds a new public convenience method Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (9)
rs/moq/README.md (1)
17-18: Docs: link-only example is good; add a run hint.Add a one-liner so readers can run it immediately.
## Examples -- [Publishing a chat track](examples/chat.rs) +- [Publishing a chat track](examples/chat.rs) — run with: + `cargo run -p moq-lite --example chat`rs/moq/src/model/track.rs (2)
4-5: Doc typos.“sequest” → “sequence”; “MoQ Tranport” → “MoQ Transport”; minor plural fixes.
-//! The sequest number is used to determine the order of streams, while the priority is used to determine which stream to transmit first. +//! The sequence number determines stream order, while priority determines which stream to transmit first. ... -//! These streams are meant to be transmitted over congested networks and the key to MoQ Tranport is to not block on them. -//! streams will be cached for a potentially limited duration added to the unreliable nature. -//! A cloned [Consumer] will receive a copy of all new stream going forward (fanout). +//! These streams are meant for congested networks; the key to MoQ Transport is to not block on them. +//! Streams may be cached for a limited duration. +//! A cloned [Consumer] will receive a copy of all new streams going forward (fanout).Also applies to: 9-11
201-245: Add unit tests for write_frame (inline with source).Per guidelines, add minimal tests to assert a single-frame group is produced.
Additional code (outside the shown ranges):
#[cfg(test)] mod tests { use super::*; #[test] fn write_frame_creates_group() { let track = Track::new("t").produce(); let mut prod = track.producer; let mut cons = track.consumer; prod.write_frame(bytes::Bytes::from_static(b"x")); let group = cons.assert_group(); // Expect exactly one frame in the group (implementation-specific; adjust if helper exists). assert_eq!(group.info.sequence, 0); } }rs/moq/examples/chat.rs (1)
46-47: Nit: trailing punctuation.Double period.
-// Create and publish a broadcast to the origin.. +// Create and publish a broadcast to the origin.rs/hang/README.md (1)
26-27: Docs: add run hint.Link-only example is fine; add command to run it.
## Examples -- [Publishing a video track](examples/video.rs) +- [Publishing a video track](examples/video.rs) — run with: + `cargo run -p hang --example video`rs/hang/examples/video.rs (2)
51-52: Verify priority semantics in comment.Confirm whether lower numeric value means higher priority; the comment may be inverted.
41-43: Treat normal session closure as Ok to avoid error exit (optional).Keeps example “green” when it finishes.
-// Wait until the session is closed. -Err(session.closed().await.into()) +// Wait until the session is closed. +session.closed().await.ok(); +Ok(())rs/hang/src/model/catalog.rs (1)
129-129: Consider implementing catalog delta support.The comment indicates that delta support is not yet implemented. This could be a valuable optimization for large catalogs to avoid retransmitting the entire catalog on each update.
Would you like me to help implement delta support for catalog updates or create an issue to track this enhancement?
rs/hang/src/catalog/root.rs (1)
17-17: Update remaining documentation references fromCatalog→RootCode references were updated; remaining matches to change:
- rs/hang/src/lib.rs:11 (doc comment)
- rs/hang/README.md:11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
rs/hang/Cargo.toml(1 hunks)rs/hang/README.md(1 hunks)rs/hang/examples/video.rs(1 hunks)rs/hang/src/catalog/root.rs(8 hunks)rs/hang/src/catalog/video/mod.rs(1 hunks)rs/hang/src/cmaf/import.rs(7 hunks)rs/hang/src/lib.rs(0 hunks)rs/hang/src/model/catalog.rs(1 hunks)rs/hang/src/model/mod.rs(1 hunks)rs/moq/Cargo.toml(1 hunks)rs/moq/README.md(1 hunks)rs/moq/examples/chat.rs(1 hunks)rs/moq/src/model/track.rs(1 hunks)
💤 Files with no reviewable changes (1)
- rs/hang/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
{rs,js}/hang/**: All media-specific logic (encoding/streaming, codec handling) must live in the hang layer.
Keep the hang layer generic: expose primitives (e.g., access to individual frames) and avoid application/UI-specific code.
Files:
rs/hang/README.mdrs/hang/src/model/catalog.rsrs/hang/Cargo.tomlrs/hang/examples/video.rsrs/hang/src/cmaf/import.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/video/mod.rsrs/hang/src/model/mod.rs
rs/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write Rust tests integrated within source files (unit tests alongside code).
Files:
rs/hang/src/model/catalog.rsrs/hang/src/cmaf/import.rsrs/moq/src/model/track.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/video/mod.rsrs/hang/src/model/mod.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: nicolaschan
PR: kixelated/moq#568
File: js/moq/README.md:50-52
Timestamp: 2025-09-02T04:35:10.240Z
Learning: For README examples in the moq repository, prefer minimal working examples that demonstrate core API usage clearly, rather than including production-level error handling patterns like try-finally blocks. Production concerns should be left for actual implementation.
📚 Learning: 2025-09-02T04:35:10.240Z
Learnt from: nicolaschan
PR: kixelated/moq#568
File: js/moq/README.md:50-52
Timestamp: 2025-09-02T04:35:10.240Z
Learning: For README examples in the moq repository, prefer minimal working examples that demonstrate core API usage clearly, rather than including production-level error handling patterns like try-finally blocks. Production concerns should be left for actual implementation.
Applied to files:
rs/moq/README.md
🧬 Code graph analysis (6)
rs/moq/examples/chat.rs (1)
rs/hang/examples/video.rs (3)
main(5-22)run_broadcast(99-137)run_session(25-43)
rs/hang/src/model/catalog.rs (4)
rs/hang/src/cmaf/import.rs (2)
new(59-73)init(113-146)rs/moq/src/model/track.rs (7)
new(31-36)new(59-64)consume(122-128)close(113-115)from(145-147)next_group(162-186)closed(189-194)rs/moq/src/model/group.rs (1)
read_frame(171-187)rs/hang/src/catalog/root.rs (1)
from_slice(55-57)
rs/hang/examples/video.rs (3)
rs/moq/examples/chat.rs (3)
main(4-21)run_broadcast(45-85)run_session(24-42)rs/hang/src/catalog/root.rs (1)
produce(85-92)rs/moq/src/model/track.rs (3)
produce(38-42)new(31-36)new(59-64)
rs/hang/src/cmaf/import.rs (3)
js/hang/src/watch/broadcast.ts (1)
catalog(144-161)js/hang/src/catalog/video.ts (2)
Video(56-56)VideoConfig(57-57)js/hang/src/catalog/root.ts (1)
Root(24-24)
rs/moq/src/model/track.rs (1)
rs/moq/src/model/group.rs (1)
write_frame(94-102)
rs/hang/src/model/mod.rs (3)
js/hang/src/watch/broadcast.ts (1)
catalog(144-161)js/hang/src/publish/video/detection-worker.ts (1)
frame(35-86)js/hang/src/publish/broadcast.ts (1)
track(123-146)
⏰ 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: Check
🔇 Additional comments (16)
rs/moq/examples/chat.rs (1)
62-80: Example reads clean; good use of TrackProducer::write_frame.No functional issues spotted.
rs/hang/Cargo.toml (1)
34-37: Dev deps added appropriately.Matches example usage; no API impact.
rs/hang/src/model/mod.rs (1)
1-1: Module exposure looks right; aligns with layering.Keeping media-specific types under hang::model and re-exporting is consistent with guidelines.
Also applies to: 7-7
rs/hang/examples/video.rs (2)
80-90: Good: catalog track published before media track; clear example flow.Looks correct and easy to follow.
108-136: Example frame flow LGTM.Demonstrates keyframe/new-group behavior clearly.
rs/moq/Cargo.toml (1)
34-38: Dev deps look right — confirm Tokio feature unification for examplesrs/moq/Cargo.toml lists tokio in [dependencies] (features: macros, io-util, sync, test-util) and in [dev-dependencies] (features: rt-multi-thread); feature unification should enable rt-multi-thread for examples. Sandbox couldn't run cargo (rustup not configured) — run locally: cargo check -p moq-lite --examples
rs/hang/src/catalog/video/mod.rs (2)
49-53: LGTM! Clean separation of optional fields.The refactoring to extract optional video configuration fields into a separate
VideoConfigOptionalstruct is a good design choice. This enables cleaner initialization patterns with..Default::default()and better aligns with the serde flattening approach.
55-106: Well-structured optional configuration.The
VideoConfigOptionalstruct properly encapsulates all optional video track metadata with appropriate serde attributes and derives. The fields are well-documented and the use ofOption<T>for each field with#[serde(default)]ensures backward compatibility and flexible initialization.rs/hang/src/catalog/root.rs (2)
86-92: LGTM! Clean producer pattern implementation.The
producemethod correctly creates aCatalogProducerwith the renamedRoottype and properly initializes the track with the default configuration.
104-105: Test updates properly reflect the new structure.The test correctly uses
VideoConfigOptionalwith the flattened pattern, demonstrating the improved initialization approach with..Default::default().Also applies to: 159-166
rs/hang/src/cmaf/import.rs (4)
2-6: Import statements correctly updated for the refactored catalog module.The imports properly reflect the new structure with
VideoConfigOptionaland align with the catalog module reorganization.
60-60: Catalog initialization updated to use Root type.The change from
Catalog::default()tocatalog::Root::default()correctly reflects the renamed type.
176-181: Consistent pattern for VideoConfigOptional initialization.The H.264 codec configuration properly uses the new
VideoConfigOptionalstructure with the flattened pattern. This is cleaner than the previous approach of setting individual fields.
191-195: All video codec paths consistently updated.The VP8, VP9, AV1, and H.265 codec initialization paths all correctly use the new
VideoConfigOptionalpattern. The consistent use of..Default::default()for unspecified optional fields is good practice.Also applies to: 216-220, 247-251, 280-285
rs/hang/src/model/catalog.rs (2)
7-16: Well-documented catalog producer abstraction.The
CatalogProducerstruct provides a clean API for managing catalog updates with proper encapsulation usingArc<Mutex<catalog::Root>>for thread-safe access.
111-135: Robust async consumer implementation.The
nextmethod properly handles both track updates and frame reading with appropriate error handling and state management. The use oftokio::select!for concurrent operations is appropriate.
rs/hang/src/model/catalog.rs
Outdated
| pub fn publish(&mut self) { | ||
| let current = self.current.lock().unwrap(); | ||
| let mut group = self.track.append_group(); | ||
|
|
||
| // TODO decide if this should return an error, or be impossible to fail | ||
| let frame = current.to_string().expect("invalid catalog"); | ||
| group.write_frame(frame); | ||
| group.close(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in publish method.
The publish method currently uses expect which could panic if serialization fails. Since the comment suggests this should be addressed, consider returning a Result or handling the error more gracefully.
- pub fn publish(&mut self) {
+ pub fn publish(&mut self) -> Result<()> {
let current = self.current.lock().unwrap();
let mut group = self.track.append_group();
- // TODO decide if this should return an error, or be impossible to fail
- let frame = current.to_string().expect("invalid catalog");
+ let frame = current.to_string()?;
group.write_frame(frame);
group.close();
+ Ok(())
}📝 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.
| pub fn publish(&mut self) { | |
| let current = self.current.lock().unwrap(); | |
| let mut group = self.track.append_group(); | |
| // TODO decide if this should return an error, or be impossible to fail | |
| let frame = current.to_string().expect("invalid catalog"); | |
| group.write_frame(frame); | |
| group.close(); | |
| } | |
| pub fn publish(&mut self) -> Result<()> { | |
| let current = self.current.lock().unwrap(); | |
| let mut group = self.track.append_group(); | |
| let frame = current.to_string()?; | |
| group.write_frame(frame); | |
| group.close(); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In rs/hang/src/model/catalog.rs around lines 67 to 75, the publish method panics
via expect when serializing current; change it to return a Result and propagate
errors instead of panicking: update the function signature to return Result<(),
E> (use a concrete error type like anyhow::Error or a crate-specific error),
replace the unwrap/expect calls with proper error handling (map mutex poisoning
and serialization errors into the chosen Result), and propagate or map any
errors from writing/closing the group so callers can handle failures gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rs/moq/src/model/track.rs (1)
110-116: Add a unit test for the new public API.Per repo guidelines, add an inline test to exercise
write_frameend‑to‑end.+#[cfg(test)] +mod tests { + use super::*; + use crate::Produce; + use bytes::Bytes; + + #[test] + fn write_frame_appends_group() { + let Produce { mut producer, mut consumer } = Track::new("t").produce(); + producer.write_frame(Bytes::from_static(b"hi")); + let group = consumer.assert_group(); + assert_eq!(group.info.sequence, 0); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rs/hang/Cargo.toml(1 hunks)rs/moq/Cargo.toml(1 hunks)rs/moq/src/model/track.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/hang/Cargo.toml
- rs/moq/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
rs/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write Rust tests integrated within source files (unit tests alongside code).
Files:
rs/moq/src/model/track.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: nicolaschan
PR: kixelated/moq#568
File: js/moq/README.md:50-52
Timestamp: 2025-09-02T04:35:10.240Z
Learning: For README examples in the moq repository, prefer minimal working examples that demonstrate core API usage clearly, rather than including production-level error handling patterns like try-finally blocks. Production concerns should be left for actual implementation.
🧬 Code graph analysis (1)
rs/moq/src/model/track.rs (1)
rs/moq/src/model/group.rs (1)
write_frame(94-102)
⏰ 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: Check
🔇 Additional comments (1)
rs/moq/src/model/track.rs (1)
110-116: Remove redundant.into(); passframethrough.
GroupProducer::write_framealready acceptsB: Into<Bytes>. Avoid the extra conversion and move.pub fn write_frame<B: Into<bytes::Bytes>>(&mut self, frame: B) { let mut group = self.append_group(); - group.write_frame(frame.into()); + group.write_frame(frame); group.close(); }
Summary by CodeRabbit