Revert the track change to the catalog. #613
Conversation
WalkthroughTrackSchema in js/hang/src/catalog/track.ts was changed from a union (string | { name, priority }) with a transform to a single object schema Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang/src/publish/priority.ts (1)
1-14: Seed catalog.priority from the centralized PRIORITY map.Ensure each catalog track’s
priorityfield is initialized viaPRIORITY[trackName]so that watch modules (e.g., watch/preview.ts, watch/location/peers.ts, watch/chat/*) can rely on a single source of truth. Document this authority and consider removing the standalone map if all priorities are stored on catalog objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
js/hang/src/catalog/track.ts(1 hunks)js/hang/src/publish/preview.ts(3 hunks)js/hang/src/publish/priority.ts(1 hunks)js/hang/src/watch/audio/captions.ts(1 hunks)js/hang/src/watch/audio/source.ts(1 hunks)js/hang/src/watch/audio/speaking.ts(1 hunks)js/hang/src/watch/broadcast.ts(1 hunks)js/hang/src/watch/chat/message.ts(1 hunks)js/hang/src/watch/chat/typing.ts(1 hunks)js/hang/src/watch/location/peers.ts(1 hunks)js/hang/src/watch/location/window.ts(1 hunks)js/hang/src/watch/preview.ts(1 hunks)js/hang/src/watch/video/detection.ts(1 hunks)js/hang/src/watch/video/source.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{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:
js/hang/src/watch/preview.tsjs/hang/src/watch/audio/speaking.tsjs/hang/src/catalog/track.tsjs/hang/src/watch/video/detection.tsjs/hang/src/publish/priority.tsjs/hang/src/watch/audio/captions.tsjs/hang/src/watch/chat/message.tsjs/hang/src/watch/location/window.tsjs/hang/src/watch/chat/typing.tsjs/hang/src/publish/preview.tsjs/hang/src/watch/audio/source.tsjs/hang/src/watch/broadcast.tsjs/hang/src/watch/location/peers.tsjs/hang/src/watch/video/source.ts
🧬 Code graph analysis (11)
js/hang/src/watch/preview.ts (5)
js/hang/src/watch/audio/captions.ts (1)
effect(32-55)js/hang/src/watch/audio/speaking.ts (1)
effect(32-56)js/hang/src/watch/chat/message.ts (1)
effect(41-66)js/hang/src/watch/chat/typing.ts (1)
effect(39-61)js/hang/src/watch/location/peers.ts (2)
effect(34-48)track(50-62)
js/hang/src/watch/audio/speaking.ts (3)
js/hang/src/publish/broadcast.ts (1)
broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/video/detection.ts (3)
js/hang/src/publish/broadcast.ts (2)
track(126-148)broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/audio/captions.ts (3)
js/hang/src/publish/broadcast.ts (1)
broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/chat/message.ts (3)
js/hang/src/publish/broadcast.ts (2)
track(126-148)broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/location/window.ts (3)
js/hang/src/publish/broadcast.ts (2)
track(126-148)broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/chat/typing.ts (2)
js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/publish/preview.ts (2)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-14)js/hang/src/watch/preview.ts (1)
Preview(10-61)
js/hang/src/watch/audio/source.ts (4)
js/moq/src/lite/publisher.ts (2)
sub(178-200)sub(209-236)js/hang/src/publish/broadcast.ts (1)
broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/location/peers.ts (2)
js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
js/hang/src/watch/video/source.ts (3)
js/hang/src/publish/broadcast.ts (1)
broadcast(73-124)js/moq/src/lite/subscriber.ts (1)
broadcast(101-134)js/moq/src/ietf/subscriber.ts (1)
broadcast(86-114)
⏰ 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 (14)
js/hang/src/watch/chat/typing.ts (1)
48-48: LGTM! Subscription updated to use per-track properties.The change correctly uses
catalog.nameandcatalog.priorityinstead of passing the catalog object and a static PRIORITY constant, aligning with the moq library's subscribe signature.js/hang/src/watch/video/source.ts (1)
144-144: LGTM! Video track subscription updated correctly.The subscription now uses
selected.track.nameandselected.track.priorityfrom the video rendition, consistent with the standardized track representation across the codebase.js/hang/src/watch/broadcast.ts (1)
4-4: LGTM! Import path updated to centralized priority definitions.The PRIORITY import has been relocated to
../publish/priority, aligning with the centralization of priority constants. The runtime behavior remains unchanged.js/hang/src/watch/video/detection.ts (1)
43-43: LGTM! Detection track subscription updated correctly.The subscription now uses
catalog.track.nameandcatalog.track.priority, following the standardized pattern for track identification and prioritization.js/hang/src/watch/audio/speaking.ts (1)
44-44: LGTM! Speaking track subscription updated correctly.The subscription now uses
info.speaking.track.nameandinfo.speaking.track.priority, consistent with the standardized approach across all watch modules.js/hang/src/watch/audio/source.ts (1)
153-153: LGTM! Audio track subscription updated correctly.The subscription now uses
selected.track.nameandselected.track.priority, aligning with the standardized track representation used throughout the codebase.js/hang/src/watch/chat/message.ts (1)
50-50: LGTM! Chat message subscription updated correctly.The subscription now uses
catalog.nameandcatalog.priority, maintaining consistency with the updated track schema across the application.js/hang/src/publish/preview.ts (3)
4-4: LGTM! PRIORITY import added for centralized priority management.The import brings in the centralized priority definitions, enabling consistent priority assignment across the codebase.
13-13: LGTM! Static PRIORITY property added for external access.The static
PRIORITYproperty exposes the preview priority, providing a consistent public interface for priority values.
28-28: LGTM! Catalog structure updated to include both name and priority.The catalog now provides both
nameandpriorityas an object, enabling watch modules to accesscatalog.nameandcatalog.priorityseparately. This aligns with the standardized track schema used throughout the PR.js/hang/src/watch/location/peers.ts (1)
44-44: LGTM!The subscription correctly uses
catalog.nameandcatalog.priority, consistent with the standardized track representation across the codebase.js/hang/src/watch/preview.ts (1)
36-40: LGTM!The changes correctly standardize the subscription to use
catalog.nameandcatalog.priority, consistent with the pattern across other watch modules. The null check forcatalog(line 37) is properly in place.js/hang/src/watch/location/window.ts (1)
53-53: Approve broadcast.subscribe usageThe zod TrackSchema enforces required name and priority, and the existing null guard handles missing tracks.
js/hang/src/watch/audio/captions.ts (1)
44-44: TrackSchema enforces requirednameandpriority
CaptionsSchemarequires aTrackSchema, and the code checksinfo.captionsbefore accessingtrack; subscription call is safe.
| export const TrackSchema = z.object({ | ||
| name: z.string(), | ||
| priority: z.number().int().min(0).max(255), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Breaking change: Verify impact on existing clients and data.
The TrackSchema now requires an object with { name, priority } fields, removing the previous union type that allowed string tracks. This is a breaking change that could affect:
- Existing catalog data that uses the old format
- Clients expecting the previous schema
- Backward compatibility with older versions
Additionally, the PR title says "Revert the track change to the catalog", but these changes appear to move forward with the object-based approach rather than reverting. Please clarify:
- Is this intentionally a breaking change?
- Are there migration steps needed for existing data?
- Does the PR title accurately reflect the changes?
Run the following script to check for existing string track usage:
🏁 Script executed:
#!/bin/bash
# Description: Search for potential string-based track usage in the codebase
# Search for track assignments that might be strings
rg -nP --type=ts "track:\s*['\"]" -A 2 -B 2
# Search for TrackSchema usage and validation
rg -nP --type=ts 'TrackSchema\.(parse|safeParse)' -A 3 -B 3
# Check for any backward compatibility code or migrations
rg -nP --type=ts '(migrate|compat|backward|legacy).*[Tt]rack' -A 3 -B 3Length of output: 867
String‐track usage broken by schema change—critical fixes required
Found string‐only tracks in js/moq-clock/src/main.ts (e.g., track: "seconds", config.track) that no longer conform to the new object‐only TrackSchema. Restore backward compatibility (e.g., re-introduce the union or add a migration/transform) or update all consumers before merging. Clarify whether this truly reverts the change or is intentionally breaking.
🤖 Prompt for AI Agents
In js/hang/src/catalog/track.ts around lines 3 to 6, the TrackSchema change to
an object-only zod schema breaks existing string-only usage (e.g., track:
"seconds" in js/moq-clock/src/main.ts); restore backward compatibility by making
TrackSchema accept either the object or a string (add a z.union(z.string(),
z.object({...})) or use z.preprocess to convert strings into the object form),
update exported types accordingly, and/or add a migration/transform that maps
legacy string tracks to the new object shape so existing consumers continue to
work until they are updated.
(by hitting tab a bunch of times)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
js/hang/src/publish/audio/captions.ts(3 hunks)js/hang/src/publish/audio/encoder.ts(3 hunks)js/hang/src/publish/audio/speaking.ts(3 hunks)js/hang/src/publish/chat/message.ts(2 hunks)js/hang/src/publish/chat/typing.ts(2 hunks)js/hang/src/publish/location/peers.ts(3 hunks)js/hang/src/publish/location/window.ts(3 hunks)js/hang/src/publish/priority.ts(1 hunks)js/hang/src/publish/video/detection.ts(3 hunks)js/hang/src/publish/video/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang/src/publish/priority.ts
🧰 Additional context used
📓 Path-based instructions (1)
{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:
js/hang/src/publish/chat/typing.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/publish/location/peers.tsjs/hang/src/publish/chat/message.tsjs/hang/src/publish/video/index.tsjs/hang/src/publish/location/window.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/video/detection.tsjs/hang/src/publish/audio/encoder.ts
🧬 Code graph analysis (9)
js/hang/src/publish/chat/typing.ts (3)
js/hang/src/watch/chat/typing.ts (3)
TypingProps(5-9)Typing(11-66)effect(39-61)js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/publish/audio/captions.ts (1)
effect(42-53)
js/hang/src/publish/audio/speaking.ts (2)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/watch/audio/speaking.ts (1)
Speaking(9-61)
js/hang/src/publish/location/peers.ts (2)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/watch/location/peers.ts (1)
Peers(10-71)
js/hang/src/publish/chat/message.ts (2)
js/hang/src/watch/chat/message.ts (3)
MessageProps(5-9)Message(11-71)effect(41-66)js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)
js/hang/src/publish/video/index.ts (1)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)
js/hang/src/publish/location/window.ts (2)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/watch/location/window.ts (1)
Window(10-77)
js/hang/src/publish/audio/captions.ts (3)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/watch/audio/captions.ts (1)
Captions(9-60)js/hang/src/catalog/audio/captions.ts (1)
Captions(9-9)
js/hang/src/publish/video/detection.ts (2)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)js/hang/src/watch/video/detection.ts (1)
Detection(12-63)
js/hang/src/publish/audio/encoder.ts (1)
js/hang/src/publish/priority.ts (1)
PRIORITY(3-15)
⏰ 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 (10)
js/hang/src/publish/location/window.ts (3)
5-5: LGTM!The import is necessary for the static
PRIORITYfield introduced on Line 20.
20-20: LGTM!The static
PRIORITYfield follows the pattern adopted across other publish modules (encoder, captions, etc.) and correctly referencesPRIORITY.location(value: 20).
41-41: [node:script]#!/bin/bash # Display the contents of window.ts around the track usage sed -n '1,200p' js/hang/src/publish/location/window.tsjs/hang/src/publish/audio/speaking.ts (1)
4-4: LGTM! Clean priority integration.The import of
PRIORITY, addition of the staticPRIORITYfield, and catalog track object construction are well-structured and consistent with the broader refactoring to object-based track entries.Also applies to: 16-16, 38-41
js/hang/src/publish/location/peers.ts (1)
5-5: LGTM! Consistent priority wiring.The priority integration for location peers follows the established pattern and correctly uses
PRIORITY.locationwith an appropriate priority level (20) for non-critical position updates.Also applies to: 14-14, 30-30
js/hang/src/publish/chat/message.ts (1)
4-4: LGTM! Appropriate high priority for chat messages.The integration correctly assigns a high priority (90) to chat messages, reflecting their user-facing, real-time nature. The object-based catalog track construction aligns with the schema changes.
Also applies to: 12-12, 31-31
js/hang/src/publish/audio/encoder.ts (1)
8-8: LGTM! Audio priority correctly configured.The encoder properly integrates priority metadata with an appropriate high value (80) for audio streams. The track object construction in the catalog is correct and consistent with the schema changes.
Also applies to: 37-37, 245-248
js/hang/src/publish/audio/captions.ts (1)
5-5: LGTM! Captions priority appropriately positioned.The priority integration for captions is well-structured with an appropriate value (70) that positions captions between the main audio stream and lower-priority features. The track object construction follows the established pattern.
Also applies to: 20-20, 47-50
js/hang/src/publish/video/detection.ts (1)
5-5: LGTM! Detection priority correctly configured as low-priority feature.The changes correctly remove the
Catalog.Tracktype fromTRACK(consistent with the schema change) and add priority metadata with an appropriate low value (30) for object detection, which is a non-critical ML feature.Also applies to: 17-18, 45-45
js/hang/src/publish/video/index.ts (1)
3-3: LGTM! Smart priority ordering for adaptive streaming.The separate priority constants for HD (55) and SD (60) enable intelligent adaptive streaming, with SD prioritized slightly higher to ensure baseline quality loads first. The track object construction correctly applies these priorities in the catalog.
Also applies to: 21-25, 85-86
Summary by CodeRabbit
New Features
Refactor
Documentation