refactor!: pre-bump API polish across the release batch - #2423
Conversation
…native Land breaking API cleanups so they ride the pending semver release instead of costing another major later. kio: restore Sync/RefUnwindSafe on Waiter/Pending/Shared by switching the lazy waker cell from OnceCell to OnceLock (the lazy fast path is preserved); add a compile-time Sync assertion so it cannot silently regress. Drop the vestigial R: Unpin bound on Consumer::wait. moq-net: mark Role #[non_exhaustive] (the wire already tolerates unknown role codes); delete the deprecated with_publish/with_consume builder aliases; add Route::announced() so the advertised publish path is one call; rename bandwidth::Producer::close to abort for crate-wide idiom consistency; move serde_json to dev-dependencies; document every public item and turn on warn(missing_docs); remove em dashes from prose. moq-token: mark Error and KeyError #[non_exhaustive]. moq-native: mark the Backoff config and Status enum #[non_exhaustive]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Give the C ABI one verb per concept before the 0.4.0 bump. `_free` now
always means "release a value handed to the caller" and `_close` always
means "stop a task or subscription", with no overlap: rename
moq_consume_{frame,track_frame,datagram,json_value}_close to _free.
Move the catalog section accessors into the moq_consume_catalog_* family
that owns the handle's lifecycle: moq_catalog_section_count/_at and
moq_catalog_get_section become moq_consume_catalog_section_count/_at and
moq_consume_catalog_section.
Regenerates moq.h and updates cpp/obs, the C smoke client, and the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
moq-stats: rename the producer config Config -> ProducerConfig so it reads unambiguously next to ConsumerConfig before 0.1.0 freezes the name; fix the traffic_track doc that mixed the publisher/subscriber roles. hang: accept the pre-0.20 displayRatioWidth/Height catalog keys as serde aliases for the renamed displayAspectWidth/Height fields, so an old catalog still decodes its aspect ratio instead of silently dropping it. Emitted names stay the new ones. Adds a legacy-decode regression test. moq-mux: rename the stuttering container::ts::catalog::Catalog trait to Carrier (it carries the MPEG-TS section) to clear the foo::foo::Foo path and the three-way Catalog name collision; document that Container::poll_read/read may yield an empty batch as a poll-again state and only None ends the group. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
moq-hls: Broadcaster::new returns crate::Result instead of leaking moq_mux::Error; make the internally-built playlist Snapshot/Segment/ render_media pub(crate) (no external users) which also clears the collision with the recorder segments::Segment; mark the renditions Event cursor enum #[non_exhaustive]. moq-rtc: re-export axum and url (leaked in router/dial signatures) with the major-bump caveat; make the empty client whep/whip submodules private; document Client::new and every Error variant. moq-rtmp, moq-srt: take path: impl AsPath uniformly across every publish/ pull/accept entry point so the mirrored surfaces read consistently; origin ownership left as each call site actually uses it. Turn on warn(missing_docs) across all four crates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 19 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes rename consumed-resource and catalog-section C APIs, update all callers and tests, and correct video-frame cleanup paths. They add legacy catalog-key deserialization, thread-safe waiter initialization, future-status handling, and public API adjustments across media, statistics, network, HLS, RTC, RTMP, and SRT crates. Deprecated builder aliases are removed, several types and modules change visibility or exhaustiveness, and documentation linting plus Rustdoc clarifications are added throughout. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-net/src/client.rs (1)
39-40: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore deprecated builder aliases.
As per coding guidelines, "deprecated public items must have both
#[doc(hidden)]and#[deprecated(note = "...")]" and you must "keep deprecated paths functional but hidden from published surfaces." The removal of thewith_publishandwith_consumealiases violates this rule. Please restore them to maintain backward compatibility.♻️ Proposed fix
+ #[deprecated(note = "use with_publisher")] + #[doc(hidden)] + pub fn with_publish(self, publish: impl Consume<origin::Consumer>) -> Self { + self.with_publisher(publish) + } + + #[deprecated(note = "use with_subscriber")] + #[doc(hidden)] + pub fn with_consume(self, subscribe: origin::Producer) -> Self { + self.with_subscriber(subscribe) + }🤖 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 `@rs/moq-net/src/client.rs` around lines 39 - 40, Restore the deprecated public builder aliases with_publish and with_consume, keeping them functional as aliases of the current publish/consume methods. Mark both aliases with #[doc(hidden)] and #[deprecated(note = "...")] using the project’s established deprecation wording, while leaving the replacement methods unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/moq-net/src/client.rs`:
- Around line 39-40: Restore the deprecated public builder aliases with_publish
and with_consume, keeping them functional as aliases of the current
publish/consume methods. Mark both aliases with #[doc(hidden)] and
#[deprecated(note = "...")] using the project’s established deprecation wording,
while leaving the replacement methods unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99b47dbb-67b5-4f17-8cfd-3bd2b3eb7419
📒 Files selected for processing (64)
cpp/obs/src/moq-source.cppdoc/concept/layer/hang.mddoc/lib/c/index.mdrs/CLAUDE.mdrs/hang/src/catalog/video/mod.rsrs/kio/src/consumer.rsrs/kio/src/waiter.rsrs/libmoq/README.mdrs/libmoq/src/api.rsrs/libmoq/src/test.rsrs/moq-bench/src/connection.rsrs/moq-gst/src/sink/session.rsrs/moq-hls/src/export/mod.rsrs/moq-hls/src/export/playlist.rsrs/moq-hls/src/export/renditions.rsrs/moq-hls/src/lib.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/container/ts/catalog.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/import/container.rsrs/moq-native/src/client.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/server.rsrs/moq-net/Cargo.tomlrs/moq-net/src/client.rsrs/moq-net/src/coding/decode.rsrs/moq-net/src/coding/encode.rsrs/moq-net/src/coding/varint.rsrs/moq-net/src/error.rsrs/moq-net/src/ietf/adapter.rsrs/moq-net/src/ietf/properties.rsrs/moq-net/src/ietf/subscribe_namespace.rsrs/moq-net/src/lib.rsrs/moq-net/src/lite/connecting.rsrs/moq-net/src/lite/priority.rsrs/moq-net/src/lite/setup.rsrs/moq-net/src/model/bandwidth.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rsrs/moq-net/src/model/time.rsrs/moq-net/src/model/track.rsrs/moq-net/src/path.rsrs/moq-net/src/server.rsrs/moq-net/src/setup.rsrs/moq-net/src/version.rsrs/moq-relay/src/connection.rsrs/moq-relay/src/stats.rsrs/moq-rtc/src/client/mod.rsrs/moq-rtc/src/error.rsrs/moq-rtc/src/lib.rsrs/moq-rtmp/src/dial.rsrs/moq-rtmp/src/lib.rsrs/moq-rtmp/src/server.rsrs/moq-srt/src/dial.rsrs/moq-srt/src/lib.rsrs/moq-srt/src/server.rsrs/moq-stats/src/consume.rsrs/moq-stats/src/lib.rsrs/moq-stats/src/produce.rsrs/moq-token/src/error.rstest/smoke/clients/c/subscribe.c
💤 Files with no reviewable changes (3)
- rs/moq-native/src/client.rs
- rs/moq-native/src/server.rs
- rs/kio/src/consumer.rs
Name the MPEG-TS catalog-carriage capability ts::Catalog: make the `catalog` submodule private and re-export its surface flat at the `ts` module, so the trait and its section types read as ts::Catalog, ts::Mpegts, ts::Track, ... instead of stuttering under container::ts::catalog. This clears the earlier foo::foo::Foo path without inventing a bespoke trait name, and lines up with the hang::Catalog / msf::Catalog family. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
moq-native's Status became #[non_exhaustive], so the ConnectionStatus mapping in the reconnect loop needs a wildcard. Map any non-Connected state to Disconnected (no behavior change today; future variants fall through safely). moq-gst doesn't build on the macOS dev shell (no GStreamer), so this only surfaced in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why
The dev batch just merged to
mainand the release-plz bump PR (#2395) is pending. Since almost every core crate is taking a breaking major in that release anyway, this lands the breaking API cleanups now so they ride this bump for free instead of costing another major later. Everything here is a shape/naming/visibility fix or a doc/lint tightening; no behavior changes except where noted.This is the source-side follow-up to the pre-bump API review. The version bumps themselves stay with release-plz / the manual edits on #2395; no
version =is touched here.What changed
kio (the load-bearing one)
Waiter's lazy waker cell wasstd::cell::OnceCell, which accidentally madeWaiter,Pending, andShared!Sync/!RefUnwindSafe. That!Synccascades intomoq_net::track::Subscriberand the moq-json / moq-mux consumers (it is most of theauto_trait_impl_removednoise in chore: release #2395's semver-checks output). Switched tostd::sync::OnceLock— the lazy fast path is preserved (get_or_initis one atomic load) and the auto traits are restored. Added a compile-timeSyncassertion so it cannot silently regress again.R: Unpinbound onConsumer::wait(crate::waitnever needed it).moq-net
Roleis now#[non_exhaustive](the wire already maps unknown role codes toNone, so new roles are anticipated).with_publish/with_consumebuilder aliases onClient/Server(a major is the canonical removal point; theRequestpair was already gone). No live callers.Route::announced()so the advertised publish path is one call.Route::new()still defaultsannounce: false(deliberate — reachable only by exact path); both constructors are now documented so the choice is obvious.bandwidth::Producer::close(err)→abort(err)to match the crate-wide idiom (abort= terminal-with-error,finish= graceful).serde_jsontodev-dependencies(only used under#[cfg(test)]).#. Removed the em dashes from prose. Reworded theTimescale::MICROdoc that read as if it were the crate default (it isMILLI).moq-token
ErrorandKeyErrorare now#[non_exhaustive](this release adds variants to both, which tripped the exhaustive-enum lint).moq-native
Backoffconfig and theStatusenum are now#[non_exhaustive].libmoq (C ABI)
_freenow always means "release a value handed to the caller",_closealways means "stop a task/subscription". Renamedmoq_consume_{frame,track_frame,datagram,json_value}_close→_free.moq_catalog_section_count/_atandmoq_catalog_get_section→moq_consume_catalog_section_count/_atandmoq_consume_catalog_section.moq.hregenerated;cpp/obs, the C smoke client, and the docs reconciled.moq-stats
Config→ProducerConfigso it reads unambiguously next toConsumerConfigbefore 0.1.0 freezes the name. Fixed thetraffic_trackdoc that mixed the publisher/subscriber roles.hang
displayRatioWidth/displayRatioHeightcatalog keys as serde aliases for the renameddisplayAspectWidth/Heightfields, so an old catalog still decodes its aspect ratio instead of silently dropping it. Emitted names stay the new ones. Adds a legacy-decode regression test.moq-mux
container::ts::catalog::Catalogtrait tots::Catalog: thecatalogsubmodule is now private and its surface (Catalog,Mpegts,Track, ...) is re-exported flat atts, clearing thefoo::foo::Foopath without a bespoke trait name and matching thehang::Catalog/msf::Catalogfamily.Container::poll_read/readmay yield an empty batch as a poll-again state and that onlyNoneends the group (both in-tree callers already loop over empties, so this is a contract clarification, not a behavior change).moq-hls / moq-rtc / moq-rtmp / moq-srt
Broadcaster::newreturnscrate::Resultinstead of leakingmoq_mux::Error; the internally-built playlistSnapshot/Segment/render_mediaare nowpub(crate)(no external users, and it dissolves the collision withsegments::Segment); therenditions::Eventcursor enum is#[non_exhaustive].axumandurl(leaked in router/dial signatures) with the major-bump caveat; the emptyclient::whep/whipsubmodules are now private;Client::newand everyErrorvariant documented.path: impl AsPathuniformly across everypublish/pull/acceptentry point so the mirrored surfaces read consistently; origin ownership left as each call site actually uses it.#![warn(missing_docs)]turned on across all four.Not done here (deliberate follow-ups)
0.3.0not the proposed0.2.34patch, and moq-relay/moq-cli should be hand-capped to patch per policy — those are edits to the release PR, not this branch.js/hangneeds the matchingdisplayRatioWidth/Heightalias (a zod preprocess, not a serde attribute) so JS consumers decode old catalogs too. It belongs with the paired@moq/*version bump PR, not this Rust branch.Carrierbound relaxation: the trait still bounds the format-agnosticimport::Container<E>, so every catalog extension carries an MPEG-TS impl even when TS is unused. Untangling that from the sharedContainerImplenum is a deeper refactor left for a follow-up.Verification
just fix+just check(whole workspace) exit 0, andRUSTDOCFLAGS="-D warnings" cargo docis clean for every touched crate. Per-crate test runs pass (kio 18, moq-net 636, moq-mux 385, hang 31 incl. the new legacy-decode test, moq-stats 14, plus the gateways). The three#[non_exhaustive]additions forced conservativematcharm updates in moq-relay, moq-bench, and moq-gst, all included.🤖 Generated with Claude Code
(written by Opus 4.8)