Repository navigation
feat(js)!: publishing never invents a timestamp - #5070
Conversation
|
Quest outcome: implemented in full;
(Written by Claude Opus 5.5) |
|
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 10 seconds. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (50)
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 |
|
Maintainer decisions, settled as recommended:
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@moq/json and @moq/flate producers take a Timed<T> ({ value, at? }) exported
from @moq/net; an absent at goes out untimed instead of stamped now. The
json window producer's push and pop take the timestamp too, and snapshot
mutate takes an optional at.
@moq/net drops writeString, writeJson, and writeBool from the group and
track producers, and zod.write with them; writeFrame already takes a
timestamp. In-repo publishers pass Time.Timestamp.now() explicitly.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
24c476e to
16a67f6
Compare
|
Automated review of head Clean, mechanical change: every default Non-blocking
Verdict: MERGE (once CI is green) This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 16a67f64aa0ffec0e8ae33d26c9b6ebb55b0acff.
- [P2] Migrate the drain harness before removing
writeString. test/drain/drain.ts:117–122 still callsgroup.writeStringat line 119. With that method deleted fromGroup.Producer, the documentedjust test drainrun throws on its first publisher tick, before exercising GOAWAY migration. Replace it withwriteFrame, UTF-8-encode the sequence, and pass an explicit timestamp because this track declaresTimescale.MILLI; then run the drain harness. - [P3] Import
Timein the stream example. js/json/README.md:48–53 now callsTime.Timestamp.now()but only importsStream. Addimport { Time } from "@moq/net"so the standalone stream snippet resolves the new symbol.
Overall direction is sound: Timed<T> and direct writeFrame calls make timestamp ownership explicit while preserving the per-track timing contract. Both observations from the earlier read-only pass remain open; the 49-file implementation patch is unchanged after rebasing.
Verification: GitHub-only static review of the patch and relevant producer/caller context. I did not execute builds, tests, or the drain harness.
…on stream example Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review follow-up, pushed in 468acc9:
(Written by Claude Opus 5.5) |
|
Merge summary for head
(Written by Claude Opus 5.5) |
|
The push answers the earlier finding about leftover callers of the removed
There's nothing new to flag. One point from the last review still stands and is non-blocking: plain-JS or Verdict: MERGE once CI finishes (the visible checks pass). Reviewed head: This is an automated review, not the maintainer's decision |
Problem
@moq/json,@moq/flate, and@moq/netfilled inTimestamp.now()when the caller gave no timestamp: the json/flate snapshot and stream producers defaultedat, the json window producer always stamped now, andwriteString/writeJson/writeBool(andzod.writethrough them) stamped now. A frame's timestamp is the publisher's statement of when content happened, so a library must never make one up. JS mirror ofquest/m1/publish-timestamp.md.Approach
@moq/netexportsTimed<T>({ value: T; at?: Time.Timestamp }), the Rust name and shape. An absentatmeans untimed, never now.update, json streamappend, json windowpush, flate snapshotupdate, and flate streamappendtake aTimed<T>. Json windowpop(count, at?)and json snapshotmutate(fn, at?)take an optional timestamp.writeString,writeJson,writeBoolare deleted from the group and track producers, as decided;writeFramealready takes a timestamp.zod.writegoes with them (no callers). Read helpers stay.Time.Timestamp.now()explicitly on their timed tracks.TimestampMismatchbefore, when the default stamped now). Net tests use atextFramehelper inmock.tsin place ofwriteString.doc/lib/js/{json,flate,net,publish}.mdand the json/flate READMEs.Deletes the quest file and its references.
Impact
@moq/net: newTimed<T>type export. Breaking: removedGroup.Producer.writeString/writeJson/writeBool,Track.Producer.writeString/writeJson/writeBool, and@moq/net/zod'swrite.@moq/json: breaking:Snapshot.Producer.update,Stream.Producer.append,Window.Producer.pushtakeTimed<T>;Window.Producer.popandSnapshot.Producer.mutategain an optionalat. No default timestamp.@moq/flate: breaking:Snapshot.Producer.updateandStream.Producer.appendtakeTimed<Uint8Array>. No default timestamp.Alternatives
writeString/writeJson/writeBoolto take a timestamp: rejected by the quest's 2026-10-08 decision.zod.writetaking aTimed<T>: possible, but it had no callers and was a thin wrapper overwriteJson.pop/mutatetaking aTimed-shaped argument: a count or callback is not a captured value, so a trailing optionalatreads better.Follow-ups
js/net/src/lite/publisher.tsstill sends an untimed frame's send time on the wire (wireTime), since no lite version encodes absence yet; that isquest/m1/lite-untimed.md.quest/m1/js-data-consumer-timestamps.mdis now unblocked.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)