Repository navigation
feat(moq-ffi): browser parity inventory, insert_datagram, largest, broadcast closed - #7304
Conversation
Quest-Claim: 4b42bff3e04ca4a2
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Landing. Maintainer decisions (2026-10-30): land the browser-parity slice and skip the moq-c methods. Codex reviewed (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: efd01fb
No actionable bugs found in the changed bindings, wrappers, and harness cases. The direction is sound: these thin, additive APIs reuse moq-net's existing sequence, timestamp validation, position, and broadcast-lifetime semantics rather than duplicating them. Keeping connection-layer parity separate makes this slice easier to verify; the explicit-sequence and >2^53 browser cases are useful coverage.
Verification limits: static review of the diff and relevant Rust implementation only; I did not run builds, tests, browser relays, or Swift compilation. This is not a claim that those checks passed.
|
quest land removed this pull request from the queue: it conflicts with the base or the PRs queued before it in dart/moq_ffi/lib/src/moq.dart, kt/README.md, swift/README.md; merge the base, push, and land it again (run). |
|
Re-landing after the queue stopped with
(Written by Claude Opus 5.5) |
Port main's new insert_datagram (#7304) onto exact MoqTimestamp records: the moq-ffi producer, the Swift wrapper, the wasm uniffi cases, the moq-ffi tests, and the kt/swift READMEs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
First slice of Complete the browser WASM bindings. The quest is
[L]and does not finish here, so it is rewritten to what remains.Summary
@moq/net's in-tree consumers are now compared against moq-ffi on wasm32. The new "Parity with@moq/net" table is intest/wasm/README.md. The model layer is mostly covered. The large gaps are at the connection layer: reconnect with backoff, the WebSocket fallback, andprobe. Smaller gaps are groupclosed/arrived/expired, frame arrival time,tryReadFrame,abort(Error), timescaleTimestamp, and the group cache constants.js/publish/src/video/encoder.ts,js/e2ee/src/track.ts, andjs/watch/src/broadcast.ts:MoqTrackProducer::insert_datagram(sequence, frame)sends a datagram at the caller's own sequence.append_datagramcontinues past it.MoqTrackProducer::largest() -> Option<MoqPosition>adds a newMoqPosition { group, frame }record.MoqBroadcastConsumer::closed()resolves once the broadcast ends.Positionalias.test/wasm/uniffino longer type-checked onmain.followgained anepochargument in moq-dev/moq#7193, which broke the nightlywasm-baseline --uniffi. This PR fixes it.largest,insertDatagramabove 2^53, and local broadcastclosed. Also a remote case where the publisher closes the broadcast and the subscriber'sclosedresolves. The remote case runs on the lite-06, transport-22, and lite-02/SETUP relays..taplo.toml: now excludesc/moq/build/**.just c checkleaves ac/moq/build/odd/<triple>symlink totarget/. Through it, taplo found the ubrn install's.crates.tomland failedjust checkonce the UniFFI harness had run.Public API: additive in moq-ffi and every wrapper. Wire: none.
Choices made without asking
WebSockettransport), andprobe. Frame arrival time is left there too, because it needs the Rust and JS clocks to agree first.MoqGroupConsumer::closedis not added. The group's state lives in theTaskthatread_frameholds. A concurrentclosedwould need its own handle to the group, and that would weakencancel()'s guarantee that it releases the group.insertDatagram(_ payload:, sequence:, timestampUs:)followsappendDatagram's payload-first shape.Decisions
Maintainer decisions, 2026-10-30:
rs/moq-cis a separate C ABI written directly over moq-net, not over moq-ffi (rs/moq-c/src/publish.rs:1141), andcpp/moqwraps no datagram API today.insert_datagram,largest, and broadcastclosedto the C ABI.Checks
just check origin/mainpasses: rs (281 passed), py (107 passed), go, kt, dart, c, cpp, js, toml, quest. Swift was skipped: this host has no Swift toolchain, so CI is the first to compile the Swift wrapper.just test wasm --uniffipasses every case on all three relays, in both the normal and the strict-CSP (interpreted) modes. It ran on an earlier head with the same Rust and cases.just test wasm-baseline --uniffisweep. Its consumer change is thefollow(path, undefined)argument, which is type-checked.Suggested follow-ups
Transporttraits requireSend, and it usestokio::spawn.wasm-baseline --uniffibroke silently when moq-dev/moq#7193 changedfollow. Consider type-checkingtest/wasm/uniffiin the PR gate wheneverrs/moq-ffichanges. That needs the ubrn build, so it may stay nightly-only.Review
quest review), gpt-6.1-sol, medium effort; quest built fromorigin/main'sflake.nixpin (01d051cf).68d74d41: one finding. The new moq-ffi methods lacked hand-written wrappers.rs/moq-cis its own C ABI over moq-net (rs/moq-c/src/publish.rs:1141,rs/moq-c/src/api.rs:3034), andcpp/moq/includehas no datagram API.efd01fb76d9ce0029994a94869f9a2b742c92677(a new session, since the AGENTS.md set changed, which weighed the responses): no findings.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code