fix(tidal): add lossless manifest support#78
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe pull request introduces DASH manifest-based playback resolution for lossless and high-resolution Tidal audio tracks, adds supporting data models for the track manifests endpoint, improves application logging, and provisions a new hifi-api service in the development environment. ChangesTidal DASH Manifest Resolution
Development Environment & Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 4 seconds.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: 3
🧹 Nitpick comments (1)
crates/yoink-server/src/providers/tidal/manifest.rs (1)
102-112: ⚡ Quick winReuse this helper from
extract_download_payload.The new helper duplicates the existing
application/dash+xmlbranch above, so any later fix to DASH fallback behaviour can drift between the two resolution paths.Small dedupe
"application/dash+xml" => { let xml = String::from_utf8(decoded).context(Utf8Snafu)?; - if let Ok(urls) = extract_dash_segment_urls(&xml) - && !urls.is_empty() - { - return Ok(PlaybackInfo::SegmentUrls(urls)); - } - extract_dash_base_url(&xml) - .map(PlaybackInfo::DirectUrl) - .map_err(ProviderError::from) + extract_dash_download_payload(&xml) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/yoink-server/src/providers/tidal/manifest.rs` around lines 102 - 112, extract_dash_download_payload currently duplicates the DASH resolution logic (calling extract_dash_segment_urls and extract_dash_base_url) that already exists in extract_download_payload; replace the duplicated logic by delegating to the existing helper: have extract_dash_download_payload call the extract_download_payload helper (and return/propagate its Result<PlaybackInfo, ProviderError>) instead of reimplementing segment URL extraction and base URL fallback, so PlaybackInfo::SegmentUrls / PlaybackInfo::DirectUrl behavior is centralized and the extract_dash_segment_urls and extract_dash_base_url logic is not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compose.dev.yaml`:
- Around line 46-57: The yoink service needs to be wired to the new hifi-api
service by setting TIDAL_API_BASE_URL to point at the hifi-api container; update
the yoink service definition to add an environment entry (or .env entry)
TIDAL_API_BASE_URL=http://hifi-api:8000 so local Tidal requests are routed
through the hifi-api proxy (refer to the hifi-api service name and port 8000 and
the TIDAL_API_BASE_URL env var).
In `@crates/yoink-server/src/main.rs`:
- Line 72: The debug log currently dumps the entire AppConfig
(tracing::debug!(config = ?app_config)), which exposes sensitive fields; replace
that call so it does not serialize AppConfig — instead log a curated struct or
explicit safe fields (e.g., feature flags, port, log level) and omit sensitive
fields like database_url, slskd_password, auth.session_secret, and
auth.init_admin_password; locate the tracing::debug! invocation and change it to
log only those non-sensitive flags or a small SafeConfig view derived from
AppConfig.
In `@crates/yoink-server/src/providers/tidal/mod.rs`:
- Around line 461-486: Validate and reject untrusted attributes.uri before
calling self.http.get: parse the URL (attributes.uri), ensure scheme is https,
resolve and verify the host is not loopback/private/link-local, and (preferably)
check against an allowed Tidal/CDN host whitelist; on failure return an
appropriate ProviderError (e.g., a new ProviderError::InvalidUri or an existing
ProviderError::Http/Reqwest variant) instead of issuing the request, then only
call self.http.get(...) and pass the response body to
extract_dash_download_payload(&manifest_xml) when validation succeeds.
---
Nitpick comments:
In `@crates/yoink-server/src/providers/tidal/manifest.rs`:
- Around line 102-112: extract_dash_download_payload currently duplicates the
DASH resolution logic (calling extract_dash_segment_urls and
extract_dash_base_url) that already exists in extract_download_payload; replace
the duplicated logic by delegating to the existing helper: have
extract_dash_download_payload call the extract_download_payload helper (and
return/propagate its Result<PlaybackInfo, ProviderError>) instead of
reimplementing segment URL extraction and base URL fallback, so
PlaybackInfo::SegmentUrls / PlaybackInfo::DirectUrl behavior is centralized and
the extract_dash_segment_urls and extract_dash_base_url logic is not duplicated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bd0c069-6e4c-45be-bbe1-7b2715dc0e58
📒 Files selected for processing (5)
compose.dev.yamlcrates/yoink-server/src/main.rscrates/yoink-server/src/providers/tidal/manifest.rscrates/yoink-server/src/providers/tidal/mod.rscrates/yoink-server/src/providers/tidal/models.rs
| hifi-api: | ||
| image: ghcr.io/binimum/hifi-api:main | ||
| container_name: hifi-api | ||
| restart: unless-stopped | ||
| environment: | ||
| - PUID=1000 | ||
| - PGID=1000 | ||
| env_file: | ||
| - .env.hifi | ||
| ports: | ||
| - 3005:8000 | ||
|
|
There was a problem hiding this comment.
Wire yoink to the new hifi-api service.
Line 46 adds hifi-api, but yoink still lacks TIDAL_API_BASE_URL, so local Tidal requests will not be routed through this proxy.
Suggested fix
services:
yoink:
@@
environment:
RUST_BACKTRACE: 1
AUTH_DISABLED: true
DOWNLOAD_LYRICS: true
DATABASE_URL: postgres://yoink:yoink@postgres:5432/yoink
+ TIDAL_API_BASE_URL: http://hifi-api:8000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.dev.yaml` around lines 46 - 57, The yoink service needs to be wired
to the new hifi-api service by setting TIDAL_API_BASE_URL to point at the
hifi-api container; update the yoink service definition to add an environment
entry (or .env entry) TIDAL_API_BASE_URL=http://hifi-api:8000 so local Tidal
requests are routed through the hifi-api proxy (refer to the hifi-api service
name and port 8000 and the TIDAL_API_BASE_URL env var).
Resolve lossless and hi-res Tidal tracks via /trackManifests/ and parse DASH manifests into segment URLs using extract_dash_download_payload. Introduce resolve_lossless_by_track_manifest and model types for the trackManifests response. Replace manifest summarizer with debug logs and mark unused fields with #[expect(dead_code)]. Also require a linked provider for Tidal downloads and add a local hifi-api service to compose.dev.yaml for development.
FlyinPancake
force-pushed
the
fix/hifi-local-quality
branch
from
8314a11 to
af9c1fa
Compare
1 participant
Resolve lossless and hi-res Tidal tracks via /trackManifests/ and
parse DASH manifests into segment URLs using
extract_dash_download_payload.
Introduce resolve_lossless_by_track_manifest and model types for the
trackManifests response. Replace manifest summarizer with debug logs
and mark unused fields with #[expect(dead_code)]. Also require a linked
provider for Tidal downloads and add a local hifi-api service to
compose.dev.yaml for development.
Summary by CodeRabbit
Release Notes
New Features
Chores