Repository navigation
feat: UPnP/DLNA renderer control — pure wire-protocol half (#41) - #90
Open
ErfanBagheri404 wants to merge 2 commits into
Open
ErfanBagheri404 wants to merge 2 commits into
ErfanBagheri404 wants to merge 2 commits into
Conversation
…trol (#41) - modules/upnpRenderer.ts: SSDP reply parse + USN device merge, device description read, absolute control URLs, AVTransport-gated renderer build - DIDL-Lite metadata with duration/protocolInfo/artwork, zero-padded queue index - SOAP: SetAVTransportURI/SetNext/Play/Stop/Set+GetVolume/GetPositionInfo, URN headers, per-action control URL routing, fault + RelTime parse - scope note: renderers can only play URLs they can reach (Subsonic/direct http); phone-resolved signed URLs are out of scope - regression: tests/upnp-renderer-regression.cjs 24/24, sabotage-verified
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ErfanBagheri404, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 4 days and 11 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Contributor
|
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 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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 |
Reviewer's GuideImplements the pure, testable UPnP/DLNA renderer-control half: SSDP/device-description parsing, DIDL-Lite queue metadata, and correctly routed SOAP transport/volume operations, with a 24-check standalone regression suite; multicast discovery remains intentionally out of scope. Sequence diagram for UPnP renderer discovery parsingsequenceDiagram
participant SSDP as Native SSDP module
participant Parser as Protocol module
participant Device as Renderer device
SSDP->>Parser: parseSsdpReply(raw)
Parser->>Parser: usnDevice(usn)
Parser->>Device: Read device description at location
Device-->>Parser: Device description XML
Parser->>Parser: parseDeviceDescription(xml)
alt AVTransport service present
Parser->>Parser: buildRenderer(reply, xml)
Parser-->>SSDP: UpnpRenderer with absolute control URLs
else AVTransport service absent
Parser-->>SSDP: null
end
Sequence diagram for UPnP queue playback and volume routingsequenceDiagram
participant Controller as Renderer controller
participant Renderer as UPnP renderer
participant AVTransport as AVTransport service
participant RenderingControl as RenderingControl service
Controller->>Controller: buildDidlWithIndex(track, index)
Controller->>Controller: setAvTransportUri(track, index)
Controller->>AVTransport: SetAVTransportURI
Controller->>Controller: setNextAvTransportUri(track, index)
Controller->>AVTransport: SetNextAVTransportURI
Controller->>AVTransport: Play
Controller->>AVTransport: GetPositionInfo
AVTransport-->>Controller: RelTime
Controller->>RenderingControl: SetVolume or GetVolume
RenderingControl-->>Controller: volume response
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
LlamaPReview — No blocking issues found
Read bounded PR-head context from package.json.
Change flow
One renderer's request topology: the AVTransport gate is loud at the picker, but a missing RenderingControl is silent at the routing branch.
sequenceDiagram
participant S as SSDPReply
participant B as buildRenderer
participant P as Picker
participant R as controlUrlFor
participant D as Renderer
S->>B: LOCATION + USN + ST
B->>B: parseDeviceDescription
B->>B: has serviceType containing #35;58#59;AVTransport#35;58#59;?
alt no AVTransport
B-->>P: null — device never listed
else AVTransport present
B-->>P: renderer with two control URLs
P->>R: route action
note over B,R: PR change — AVTransport gate is enforced, routing branch can return an empty RenderingControl URL
opt SetVolume or GetVolume
R-->>D: renderingControlUrl — may be empty string
end
opt all other actions
R-->>D: AVTransport controlUrl
end
D-->>P: SOAP response or fault
end
Review details and evidence
Material unknowns
- Is
typescriptdeclared inpackage.jsonand is this suite registered? The suite doesrequire('typescript')at module load; iftypescriptis absent,node tests/upnp-renderer-regression.cjsthrows before any check runs and the 24/24 claim fails as written. Deep judged this a documentation/evidence defect only — it changes confidence in the 24/24 claim, not the merge posture.- Check: Confirm
typescriptis in dependencies and wire the suite into a check that runs at PR head.
- Check: Confirm
- Executed green run of the suite at PR head. No exact-head execution output exists for the suite; the 24/24 claim rests only on the author's report. Corroborative, not decisive — the module is additive, wired to no caller, and no repository check runs this suite.
- Check: Run
node tests/upnp-renderer-regression.cjsat head and record the output.
- Check: Run
- No consumer references the new module. Searches for
upnpRendererreturned no hits; this is a coverage gap, not proof of absence. The PR states the module is the first half, so this is a phasing fact rather than a defect.- Check: None required by Deep.
LlamaPReview checks
- Read bounded PR-head context from
package.json. - Reviewed changed regions in
tests/upnp-renderer-regression.cjs. - Read the complete PR-head file
.github/workflows/pr-checks.yml. - Reviewed changed regions in
modules/upnpRenderer.ts. - Read bounded PR-head context from
modules/upnpRenderer.ts.
LlamaPReview is an open-source pull request reviewer. See exactly what it will and will not publish.
soapActionHeader hardcoded AVTransport:1 / RenderingControl:1 regardless of what the renderer advertised, so a device exposing AVTransport:2 rejected every action even though the SOAP body was correct. buildRenderer already read the real service types; they are now kept on the UpnpRenderer and fed into soapActionHeader via the new actionUrn / serviceTypeFor helpers. RenderingControl's version is stored independently — the two families version separately. Tests: 3 new regressions (v2 device advertised version, independent family versions, no RenderingControl at all). 27/27 pass; sabotage mutations of the version extraction and family selection all caught.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The testable half of UPnP playback: everything a renderer speaks on the wire, with no native dependency.
What this does NOT do
The discovery half — SSDP multicast sockets — needs a small native module (React Native has no multicast stdlib). That is the remaining work, and this PR is written so the native module only has to produce strings this module already parses.
Equally deliberate: renderers can only play URLs they can reach. googlevideo URLs are signed and Expiring, so the queue handed to a renderer is limited to phone-reachable sources (Subsonic/Navidrome, local files served over the LAN, direct http). The scope note in the module header says this plainly.
Tests
node tests/upnp-renderer-regression.cjs— 24/24.Sabotage-verified: routing volume to the transport URL drops it to 23/24.
Summary by Sourcery
Implement the pure wire-protocol layer for controlling UPnP/DLNA renderers.
New Features:
Enhancements:
Tests:
Closes #41