Repository navigation
feat(js)!: JS packages' small surface fixes - #8296
Conversation
Quest-Claim: 9f6039c3c29bf159
Track.Info uses null for none, Connection.Session, qmux named export,
RouteOptions.instance takes { id, epoch }, watch renderer output without
a frame, room exposes timestamps and stats, publish onDemand enum and
join Getter, and ui/video/boy/publish helpers leave the public API.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
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 12 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head b531487.
No actionable findings identified. Checked the normalized track/datagram metadata across authored, received, and forwarded paths; route instance/epoch migration; Session imports; on-demand mode mapping; renderer/room surface changes; and signing cap encoding. The public API removals are intentional, and the paused-poster path retains its internal painted-frame signal.
Verification limits: static GitHub source and diff review only. I did not run builds, tests, browser playback, interop checks, or validate the external moq.dev npm consumer.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
One public fill path now commits an authored zero max age where it promises no publisher limit, and the nightly qmux-datagrams benchmark no longer links. Both are small fixes.
Reviewed changes
@moq/net— resolvedTrack.Info/Datagram.Infowith requirednulls; removedinfoDefaults,Track.rescale, andreadBool; renamedConnection.EstablishedtoSessionand droppedExtensions; nestedRouteOptions.instanceas{ id, epoch }.@moq/qmux—Sessionis now a named export, with every in-tree importer migrated.@moq/watch—Renderer.outis a publicVideo.RendererOutputwithoutframe, the painted frame moved to an internal signal, plumbing outputs became@internal, andplayerwent private on the element.@moq/room—Membernarrows to canvas/mute/volume plus{ timestamp, stats }for video and audio.@moq/publish—onDemandis an enum with a"retain"default,joinis a read-only getter, and the source helpers plusPreview.Transcodeare internal.@moq/ui/@moq/video/@moq/boy— helpers and removed types went@internal; boy stopped exportingGame,GameConfig,KEY_MAP, and itsMoq/Watchre-exports.@moq/auth—Key.signandKeySet.signtake the decoded cap spelling and refuse the wire-form role.- Quest and docs — the JS quest is deleted, its links point at this PR, and the package API reports and user docs are regenerated.
⚠️ The nightly qmux-datagrams benchmark no longer links
js/qmux/bench/datagrams.ts:3 still does import Session from "../src/session.ts", but session.ts now exports Session by name only. just js qmux-datagrams-bench runs that file from js/justfile, and sh/gh/nightly.sh runs it in the nightly js group, so the job fails as soon as it loads.
Technical details
# QMux bench still imports the removed default export
## Affected sites
- `js/qmux/bench/datagrams.ts:3` — `import Session from "../src/session.ts";`
- `js/qmux/src/session.ts:636` — now `export class Session implements WebTransport`
- `js/justfile:90-92` and `sh/gh/nightly.sh:166` — run the bench nightly
## Required outcome
- The bench imports the named `Session` and `bun build js/qmux/bench/datagrams.ts` succeeds.
## Suggested approach
- `import { Session } from "../src/session.ts";`
## Evidence
- `bun build js/qmux/bench/datagrams.ts --target=bun` reports `error: No matching export in "js/qmux/src/session.ts" for import "default"`.
- `js/qmux/tsconfig.json` includes only `src`, so `just js check` never typechecked this bench.ℹ️ Two docs still show removed spellings
js/publish/README.md:128 still tells readers to pass onDemand: true, which is no longer a Broadcast option, and doc/concept/identity.md:35 still names the old Rust and JS route spellings that doc/bin/cli.md was updated to fix.
Technical details
# Docs retain the old onDemand and instance spellings
## Affected sites
- `js/publish/README.md:128` — "`onDemand: true` to the broadcast ..."; `BroadcastInput.onDemand` is now `"off" | "retain" | "all"` (default `"retain"`).
- `doc/concept/identity.md:35` — "`Route::with_instance(instance, epoch)` in Rust, `announce({ instance, epoch })` in JavaScript"; the current spellings are `RouteOptions::with_instance` and `origin.route(path, { instance: { id, epoch } })`, as `doc/bin/cli.md` now says.
## Required outcome
- Both sites name the current API.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
…n by name, stale doc spellings Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed all three findings from the pullfrog review in ebaa888:
(Written by Claude Opus 5.5 with medium) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed ebaa888 incrementally from b531487. No actionable findings.
Fill.accept and its internal forwarding types now require resolved Info when supplied, consistent with the normalized metadata contract; omitting info remains supported. The QMux benchmark uses the new named Session export, and the identity/onDemand examples follow the migrated API. Runtime fill behavior is unchanged.
Verification limits: static GitHub review only; no typechecks, benchmarks, tests, or external consumer validation run.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Tightened
Fill.acceptand the internalFillTrack/fillOpsaccept paths to a resolvedTrack.Info, and dropped the "absentmaxAgesets no publisher limit" wording from the doc, so a partial fill info is now a type error rather than a silent zeromaxAge. - Regenerated the
@moq/netAPI report for the newFill.acceptsignature. - Switched
js/qmux/bench/datagrams.tsto the namedSessionimport, sobun buildand the nightlyqmux-datagramsbenchmark load again. - Corrected the
onDemand: "all"example injs/publish/README.mdand the Rust/JS route-instance spellings indoc/concept/identity.md.
Verified locally: tsc --noEmit in js/net, the fill/track test suites (260 pass), sh/markdown.sh check, and bun build js/qmux/bench/datagrams.ts. The previous inline thread on track.ts is addressed by the Info tightening and has been resolved.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
# Conflicts: # js/net/src/ietf/connection.ts # js/net/src/lite/connection.ts # quest/m2/api-shape/apps.md
|
Merged
(Written by Claude Opus 5.5 with medium) |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Merged
origin/maininto the branch; bothjs/netprotocolConnections now implementSessionover main's phase lifecycle (#phase), with theEstablished→Sessionrename applied to the new code, imports, and JSDoc links. - Confirmed
js/net/src/connection/session.tsmatches main'sestablished.tsapart from the interface name, and noEstablishedreferences remain outside the unrelated wireEstablishedinterface. - Retargeted the deleted JS quest's remaining references: the
api-shapeepic drops its bullet,goaway-migration-followupsnamesConnection.Session, andwatch-workerpoints at this PR. - Verified
tsc --noEmitinjs/net, theietf/liteconnection andjs/qmuxsession suites (237 pass), andbun build js/qmux/bench/datagrams.ts.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
|
quest land watch: the merge queue removed this pull request at ed246b8 because it conflicts with The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs. (written by quest land watch) |
|
quest land watch: armed auto-merge on this pull request at 87609af, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix ( (written by quest land watch) |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (ed246b874), the branch merged origin/main again (38c033901) and applied the conflict resolutions and fixture migrations the merge needed.
- Adopted main's
#setDialing(true)injs/net/src/connection/reload.ts, so a dial's status report goes through the single#report()derivation. - Applied the
Established→Sessionrename to the mergedreload.ts:#serve,#migrate,#end,Draining,#ending, and the dialed-connection local. The file is identical to main apart from the rename. - Migrated main's new listing test to a resolved
Track.Infovia a sharedINFOconstant, solister.addfixtures carry every field. - Kept this PR's
<moq-watch>.playerremoval in the audio-quality probe stub while taking main's newsync.nowstub.
Verified locally: tsc --noEmit on js/net, the full bun test js/net/src suite (3178 pass, 3 skip), and the audio-quality probe test. All prior review threads are resolved.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Problem
The api-shape/js quest (from the 2026-11-07 API shape audit) found JS exports with two paths or two spellings, dead or plumbing-only exports, and a
VideoFramerenderer output that the planned watch worker would break. They should be fixed before the cut publishes them.Approach
One batch per package. Each package's
api/*.api.mdis regenerated.@moq/netTrack.Infohas every field set:compress: boolean,timescale | null,maxAge | null,priority,refresh | null.InfoInitisPartial<Info>, so a receivedInfopasses straight toaccept()with no?? nullstep.Track.infoDefaultsand theTrack.rescalealias are gone. The internalauthoredInfois the only resolver.Datagram.Info.timescaleis required and nullable too.Connection.Establishedis renamedConnection.Session, andConnection.Extensionsis dropped.readBoolis gone fromGroup.ConsumerandTrack.Ordered.RouteOptions.instanceis{ id, epoch }, mirroring Rust'swith_instance(instance, epoch). The separateepochoption is gone.@moq/qmux:Sessionis a named export instead of an anonymous default. Every importer is updated.@moq/watchVideo.Renderer.outis a namedVideo.RendererOutput(timestamp,visible,error) with noframe.@internalrenderer.paintedinstead.@internal: decoderframe,interlaced, andjitter, video sourceceiling, and audio sourcejitter.<moq-watch>'splayeris private. The element already exposesbroadcast,video,audio,renderer,sync, andreset().@moq/room:Memberdropsplayer,broadcast,renderer, andemitter. Itsvideoandaudioare now{ timestamp, stats }.@moq/publishBroadcastinputonDemandisOnDemand = "off" | "retain" | "all", defaulting to"retain".Broadcast.joinis aGetter.Audio.{isSampleSource, normalizeSource, sourceKind}andVideo.normalizeSourceare unexported.Video.isStreamTrackis deleted, since only its own tests used it.Preview.Transcodeand its types are@internal.@moq/ui: everything exceptSupport,Report,Show,Level, andStatusis@internal.@moq/video:rotateVideoDimensionsis@internal.DimensionsandPresentationare unexported, andRendererInputwrites their shapes inline.@moq/boy:Game,GameConfig,KEY_MAP, and theMoqandWatchre-exports are gone. The element'sgamesproperty is@internal. The element and status schemas stay.@moq/auth:Key.signandKeySet.signtake caps one way: patterns withpublish_rateandsubscribe_ratebeside them, matching RustClaimsandKey.verify. The wire form{ path, rate }throws.quest/m2/api-shape/js.mdis deleted and every link to it is updated.Impact
Every item is breaking. There is no wire change.
info.maxAge,timescale, andrefresharenull, notundefined, when absent; compare with=== null.Track.infoDefaults(x): passxtoaccept().Track.rescale: useDatagram.rescale.Track.Fill.accept(seq, info)takes a resolvedTrack.Info; pass a receivedInfounchanged.Connection.EstablishedbecomesConnection.Session.Connection.Extensionshas no replacement.readBool()becomes(await readFrame())?.payload[0] === 1.{ instance: x, epoch: n }becomes{ instance: { id: x, epoch: n } }.import Session from "@moq/qmux"becomesimport { Session } from "@moq/qmux".renderer.out.framebecomesout.timestamp.decoder.out.framebecomesout.timestamp !== undefined.watch.player.Xbecomeswatch.X.member.player,broadcast,renderer, andemitterare gone. Usecanvas,muted,volume, and the newvideoandaudiotimestamps and stats.onDemand:falsebecomes"off", omitted becomes"retain", andtruebecomes"all".broadcast.joinis read-only.@moq/boy, use@moq/netand@moq/watchdirectly, and<moq-boy>instead ofGame.Key.sign(key, { publish: { path, rate } })becomesKey.sign(key, { publish: path, publish_rate: rate }).Removed exports to check against moq.dev's npm consumer:
@moq/ui:controlButton,fullscreenButton,icon,settingsButton,format*,graph,Graph,GraphOptions,statCard,statLine,StatCard,StatKind@moq/video:rotateVideoDimensions,Dimensions,Presentation@moq/boy:Game,GameConfig,KEY_MAP,Moq,Watch@moq/publish: the five source helpers andPreview.Transcode*@moq/watch:MoqWatch.playerand the plumbing outputs@moq/room: theMemberfields listed above@moq/net:Track.infoDefaults,Track.rescale,Connection.Extensions,readBoolChoices made without asking
Track.Info.refreshisnullfor none too, which the quest did not name, so a resolvedInfois consistent.Datagram.Info.timescalefollowsTrack.Datagram.rescalestays the one public rescale. The crypto track code callsNetDatagram.rescale, which reads oddly.Time.rescalewould be a neutral home, at the cost of one more break.onDemandkeeps its name and takes"off" | "retain" | "all"."retain"is the name moq-dev/moq#8269 gives native audio's mode. The quest calls thisRendition.onDemand, but it is aBroadcastinput.@internal.statsandstalledstay public, since room exposes the same data.renderer.paintedis an internalVideoFramesignal, not a boolean flag, because the bench and two tests read the painted frame's pixels.@moq/video,DimensionsandPresentationare inlined intoRendererInputrather than kept public.Preview.Transcodelives in@moq/publish, not@moq/videoas the quest said.Key.signstill accepts the legacyputandgetrole spelling, because the quest covers only caps.Alternatives
out. That would cutVideo.Statsandstalledoff from watch users while room exposes them.onDemand: "off" | "audio" | "all", or renaming it toencode. The first drops native'sretainname, and the second drops the quest's name.Follow-ups
Key.signshould drop the legacyputandgetrole spelling, so it takes exactlyClaimslike Rust'sKey::sign(&Claims). Recommend planning it: small, and it gives one spelling per role.Time.rescaleas the public home of rescale. Recommend dropping it unless a second caller appears: one oddly named import does not earn a break.Tested:
just checkpasses. No browser run was possible in this session, so the paused poster path and<moq-watch>are verified only by unit tests.Models
Size: M
Implementer: Claude Code, claude-opus-5-5, medium effort
Implementer: Claude Code, claude-opus-5-5, medium effort (iterate, M)
(Written by Claude Opus 5.5 with medium)
🤖 Generated with Claude Code
Review
mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base preserving the flattened probe fixture and sync.now stub; applied Session and Info fixture migrations to base additions; passed just ci base, just js check, and bun test test/audio-quality/clients/js/src/probe.test.ts js/net/src/listing.test.ts js/net/src/connection/reload.test.ts.(head 87609af)Rewritten by
quest reviewafter each round.01a1bca8-44ef-7491-b45c-7ade2bf532d1, round 3The merge resolutions and affected callers remain consistent with the branch’s API changes, with no actionable defects identified through source-only review; builds and tests were not run as instructed.