Fix download metadata / video UI cards, fix dash downloads#586
Fix download metadata / video UI cards, fix dash downloads#586DanTheMan827 wants to merge 5 commits into
Conversation
Agent-Logs-Url: https://github.com/DanTheMan827/monochrome/sessions/1ba1ce02-f54a-4d49-99db-1ea858734abe Co-authored-by: DanTheMan827 <790119+DanTheMan827@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNormalize and backfill track/video metadata, hydrate missing download metadata before downloads, stop implicit per-request proxying of DASH/HLS/segment URLs (introduce configurable proxy and fetch patching), route video-card clicks into queueed playback, and surface enriched track info to download and filename/lyrics logic. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant Events as Events
participant Store as TrackDataStore
participant Player as VideoPlayer
User->>Events: click .video-card[data-video-id]
Events->>Store: collect sibling `.video-card[data-video-id]` -> map IDs to trackDataStore
alt video list available
Events->>Player: setQueue(videoList, startIndex)
Events->>Player: enableAutoplay()
Events->>Player: clearShuffleActive()
Events->>Player: play()
else
Events->>Player: playVideo(singleVideo)
end
sequenceDiagram
participant App as Client
participant API as LosslessAPI
participant Downloader as Downloader
participant Proxy as DevProxy
App->>API: request downloadTrack(track, opts)
API->>API: prepare/enrich track metadata (enrichTrack -> getTrackMetadata if missing)
API->>Downloader: pass streamUrl (raw)
alt patchFetch/dev proxy active
Downloader->>Proxy: fetch HEAD/GET/segments (fetch calls routed via patched fetch or /proxy)
Proxy-->>Downloader: proxied responses
else
Downloader->>Downloader: fetch direct stream URLs
end
Downloader-->>App: assembled blob + metadata (returns track)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/ui.js (2)
4105-4111:⚠️ Potential issue | 🟠 MajorFix the CI lint failure for the floating promise.
The lint job reports
@typescript-eslint/no-floating-promiseson Line 4111. Handle the promise explicitly; while here, remove the unused catch binding reported on Line 4105.🧹 Proposed fix
- } catch (e) { + } catch { rateCriticsEl.innerHTML = `<a style="color: var(--muted-foreground);">Unable to Fetch Critic Score</a>`; rateUsersEl.innerHTML = `<a style="color: var(--muted-foreground);">Unable to Fetch User Score</a>`; } } - fetchAotyWorker(album.title, album.artist.name); + fetchAotyWorker(album.title, album.artist.name).catch((error) => { + console.warn('Failed to fetch AOTY scores:', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 4105 - 4111, The call to fetchAotyWorker(album.title, album.artist.name) is a floating promise and the catch uses an unused binding (catch (e)); either await the call inside the surrounding async function or explicitly handle its result by appending a .catch handler (e.g., .catch(err => /* handle/log */)) to avoid the no-floating-promises lint error, and remove or use the unused catch binding by changing catch (e) to catch (_) or logging the error so the variable is used; locate the try/catch block that updates rateCriticsEl and rateUsersEl and the fetchAotyWorker(...) invocation to apply the fix.
5508-5518:⚠️ Potential issue | 🟠 MajorAdd the missing artist video playback handler.
Line 5516 fixes favorites to use the video store, but artist video cards still only bind data/like state. Unlike search and library video cards, the artist page never wires
.card-play-btn/.card-image-containertothis.player.playVideo(video), so those rendered controls do nothing.🎬 Proposed fix
if (el) { trackDataStore.set(el, video); await this.updateLikeState(el, 'video', video.id); + el.addEventListener('click', (e) => { + if (e.target.closest('.like-btn')) { + e.stopPropagation(); + return; + } + if (e.target.closest('.card-play-btn') || e.target.closest('.card-image-container')) { + e.stopPropagation(); + this.player.playVideo(video); + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 5508 - 5518, The artist page populates video cards but never wires play controls so clicks do nothing; inside the same loop after trackDataStore.set(el, video) and await this.updateLikeState(el, 'video', video.id) attach click handlers for the play UI by selecting el.querySelector('.card-play-btn') and el.querySelector('.card-image-container') and calling this.player.playVideo(video) (or delegating to the existing play helper used by search/library cards) and ensure to prevent default propagation as those handlers do for other pages; mirror the binding logic used in the search/library video card setup so artist video cards respond to play clicks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/api.js`:
- Around line 1660-1663: The current album merge spreads track.album after
fullTrack.album which lets null/undefined fields in track overwrite hydrated
values; change the merge so only defined (non-nullish) properties from
track.album override fullTrack.album—e.g., build a filtered version of
track.album that excludes null/undefined values (or use a helper like
compactObject) and then spread fullTrack.album first and the filtered trackAlbum
second in the album object (refer to the album merge where fullTrack and track
are spread).
In `@js/downloads.js`:
- Line 340: downloadDiscography currently calls downloadTrackBlob but ignores
the enriched track returned, continuing to build filenames/lyrics from the
original track; update downloadDiscography to await/accept the returned object
from downloadTrackBlob (blob, extension, track) and use that returned track when
constructing filenames, metadata and lyric files instead of the original input
track so discography exports include the hydrated metadata; ensure variable
names in downloadDiscography that previously referenced the input track are
replaced with the returned track and handle any null/undefined track cases
consistently.
---
Outside diff comments:
In `@js/ui.js`:
- Around line 4105-4111: The call to fetchAotyWorker(album.title,
album.artist.name) is a floating promise and the catch uses an unused binding
(catch (e)); either await the call inside the surrounding async function or
explicitly handle its result by appending a .catch handler (e.g., .catch(err =>
/* handle/log */)) to avoid the no-floating-promises lint error, and remove or
use the unused catch binding by changing catch (e) to catch (_) or logging the
error so the variable is used; locate the try/catch block that updates
rateCriticsEl and rateUsersEl and the fetchAotyWorker(...) invocation to apply
the fix.
- Around line 5508-5518: The artist page populates video cards but never wires
play controls so clicks do nothing; inside the same loop after
trackDataStore.set(el, video) and await this.updateLikeState(el, 'video',
video.id) attach click handlers for the play UI by selecting
el.querySelector('.card-play-btn') and el.querySelector('.card-image-container')
and calling this.player.playVideo(video) (or delegating to the existing play
helper used by search/library cards) and ensure to prevent default propagation
as those handlers do for other pages; mirror the binding logic used in the
search/library video card setup so artist video cards respond to play clicks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 986384a5-67ba-4354-99af-1537b877bd31
📒 Files selected for processing (5)
js/api.jsjs/dash-downloader.tsjs/downloads.jsjs/events.jsjs/ui.js
| album: { | ||
| ...(fullTrack?.album || {}), | ||
| ...(track?.album || {}), | ||
| }, |
There was a problem hiding this comment.
Preserve hydrated album fields when the original value is nullish.
Because track.album is spread after fullTrack.album, an incomplete source with album.numberOfTracks: null overwrites the hydrated value. That leaves the exact missing metadata from Lines 1641-1644 unresolved.
🔧 Proposed fix
album: {
...(fullTrack?.album || {}),
...(track?.album || {}),
+ numberOfTracks: track?.album?.numberOfTracks ?? fullTrack?.album?.numberOfTracks,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| album: { | |
| ...(fullTrack?.album || {}), | |
| ...(track?.album || {}), | |
| }, | |
| album: { | |
| ...(fullTrack?.album || {}), | |
| ...(track?.album || {}), | |
| numberOfTracks: track?.album?.numberOfTracks ?? fullTrack?.album?.numberOfTracks, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/api.js` around lines 1660 - 1663, The current album merge spreads
track.album after fullTrack.album which lets null/undefined fields in track
overwrite hydrated values; change the merge so only defined (non-nullish)
properties from track.album override fullTrack.album—e.g., build a filtered
version of track.album that excludes null/undefined values (or use a helper like
compactObject) and then spread fullTrack.album first and the filtered trackAlbum
second in the album object (refer to the album merge where fullTrack and track
are spread).
| const extension = await getExtensionFromBlob(blob); | ||
|
|
||
| return { blob, extension }; | ||
| return { blob, extension, track: downloadTrack }; |
There was a problem hiding this comment.
Apply the enriched-track return value to discography downloads too.
downloadTrackBlob() now returns the effective track, but downloadDiscography() still ignores it and builds filenames/lyrics from the original track at Lines 795-812. Discography exports can still miss the metadata this PR is trying to hydrate.
🔧 Proposed follow-up fix for the discography caller
- const { blob, extension } = await downloadTrackBlob(track, quality, api, signal, null);
- const filename = buildTrackFilename(track, quality, extension);
+ const {
+ blob,
+ extension,
+ track: enrichedTrack,
+ } = await downloadTrackBlob(track, quality, api, signal, null);
+ const effectiveTrack = enrichedTrack || track;
+ const filename = buildTrackFilename(effectiveTrack, quality, extension);- const lyricsData = await lyricsManager.fetchLyrics(track.id, track);
+ const lyricsData = await lyricsManager.fetchLyrics(effectiveTrack.id, effectiveTrack);
if (lyricsData) {
- const lrcContent = lyricsManager.generateLRCContent(lyricsData, track);
+ const lrcContent = lyricsManager.generateLRCContent(lyricsData, effectiveTrack);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/downloads.js` at line 340, downloadDiscography currently calls
downloadTrackBlob but ignores the enriched track returned, continuing to build
filenames/lyrics from the original track; update downloadDiscography to
await/accept the returned object from downloadTrackBlob (blob, extension, track)
and use that returned track when constructing filenames, metadata and lyric
files instead of the original input track so discography exports include the
hydrated metadata; ensure variable names in downloadDiscography that previously
referenced the input track are replaced with the returned track and handle any
null/undefined track cases consistently.
There was a problem hiding this comment.
Pull request overview
This PR improves download correctness and video UI interactions by normalizing metadata fields used during downloads, improving video-card behavior in the UI, and adjusting DASH download handling.
Changes:
- Add/normalize missing metadata for downloaded tracks (including bulk downloads) and normalize
copyrightto a string. - Improve video cards: better cover fallback selection and add click handling to play videos/queues.
- Adjust DASH download flow by shifting proxy usage away from the manifest URL and toward segment requests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| js/ui.js | Improves video cover selection (adds squareImage) and fixes like-state type for artist video cards. |
| js/events.js | Adds click behavior for video cards to play a video queue (or single video). |
| js/downloads.js | Enriches track metadata before bulk downloads and uses the enriched data for filenames/lyrics. |
| js/dash-downloader.ts | Imports proxy helper for DASH segment fetching. |
| js/api.js | Normalizes copyright field, improves video image normalization, hydrates missing download metadata, and changes DASH download invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.warn('Failed to hydrate full track metadata for download:', e); | ||
| } | ||
| } | ||
| const isVideo = track?.type?.toLowerCase().includes('video'); |
There was a problem hiding this comment.
enrichTrack() computes isVideo via track?.type?.toLowerCase().includes('video'), but type is not guaranteed to be present on prepared track objects (e.g., when prepareTrack() is called on an input object without a type field). If track.type is null/undefined this will throw because .includes(...) is invoked on undefined. Make this null-safe (e.g., use track?.type?.toLowerCase?.().includes('video') or coerce with (track?.type || '')).
| const isVideo = track?.type?.toLowerCase().includes('video'); | |
| const isVideo = (track?.type?.toLowerCase?.() || '').includes('video'); |
| const { | ||
| blob, | ||
| extension, | ||
| track: enrichedTrack, | ||
| } = await downloadTrackBlob(track, quality, api, signal, (p) => { |
There was a problem hiding this comment.
In bulkDownload(), the destructured track: enrichedTrack can actually be the original input track when enrichment fails (since downloadTrackBlob() falls back), so the name is misleading. Consider renaming this destructured value to something like downloadTrack / trackForMetadata to reflect what it contains.
| const effectiveTrack = enrichedTrack || track; | ||
| const filename = buildTrackFilename(effectiveTrack, quality, extension); |
There was a problem hiding this comment.
const effectiveTrack = enrichedTrack || track; is redundant here because downloadTrackBlob() always returns a non-null track value. Simplifying this avoids implying that enrichedTrack might be missing at this point.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
js/proxy-utils.js (1)
1-1: Scope the ESLint suppression to the Vite define global.The file-wide
no-undefdisable can hide unrelated undefined identifiers later. A targeted global declaration keeps the intended exception narrow.♻️ Proposed narrower ESLint directive
-/* eslint-disable no-undef */ +/* global __VITE_PROXY__ */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/proxy-utils.js` at line 1, Replace the file-wide "/* eslint-disable no-undef */" with a targeted global declaration for Vite's define to avoid hiding other undefineds: remove the disable and add a top-of-file comment like "/* global define */" so only the "define" global is allowed; ensure no other eslint-disable comments remain and run lint to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/global.d.ts`:
- Line 37: Update the ambient declaration for the injected Vite value
__VITE_PROXY__ so its type matches the runtime injection (null or string):
locate the const __VITE_PROXY__ declaration in the global type file and change
its type from string | undefined to string | null so consumers and type-checking
align with the actual injected value.
In `@vite.config.ts`:
- Around line 41-50: When proxying responses for HEAD requests, don't rely on
body.length (which will be 0) — preserve the upstream Content-Length header. In
the response building around res.writeHead/res.end, detect HEAD (e.g.,
req.method === 'HEAD') and set the 'content-length' header from
upstream.headers.get('content-length') if present; for non-HEAD keep using the
computed body.length (after Buffer.from(await upstream.arrayBuffer())). Also for
HEAD avoid sending a body (call res.end() with no body) while still writing the
preserved content-length header.
- Line 85: The build currently sets __VITE_PROXY__ based on _options.mode which
can be overridden; update the logic to use _options.command === 'serve' so the
proxy string '/proxy-audio' is only embedded during dev serve; locate the
assignment of __VITE_PROXY__ in vite.config.ts and replace the mode check
(_options.mode == "development") with a command check (_options.command ===
"serve") so JSON.stringify(_options.command === "serve" ? '/proxy-audio' : null)
is emitted.
- Around line 13-18: The proxyAudioPlugin middleware leaves types out and
mishandles HEAD and environment detection: add proper TypeScript types for
configureServer's server (ViteDevServer) and the middleware parameters req/ res/
next (use http.IncomingMessage, http.ServerResponse or Vite types) and type any
caught error, change the dev/production check to use server.config.command ===
'serve' (or accept a command param) so the proxy URL is only injected in dev,
and fix HEAD handling in the middleware to avoid buffering the upstream body —
on HEAD just proxy response headers (status and headers) and end the response
without reading the body; also ensure other methods stream the upstream response
to res and handle and log typed errors (the symbols to edit are
proxyAudioPlugin, configureServer and the middleware function passed to
server.middlewares.use).
---
Nitpick comments:
In `@js/proxy-utils.js`:
- Line 1: Replace the file-wide "/* eslint-disable no-undef */" with a targeted
global declaration for Vite's define to avoid hiding other undefineds: remove
the disable and add a top-of-file comment like "/* global define */" so only the
"define" global is allowed; ensure no other eslint-disable comments remain and
run lint to verify.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3e97608-9315-4a39-b951-72435207ac4e
📒 Files selected for processing (3)
js/global.d.tsjs/proxy-utils.jsvite.config.ts
|
|
||
| declare global { | ||
| const __COMMIT_HASH__: string | undefined; | ||
| const __VITE_PROXY__: string | undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the injected __VITE_PROXY__ value and ambient declaration stay aligned.
rg -n -C2 '__VITE_PROXY__' vite.config.ts js/global.d.ts js/proxy-utils.jsRepository: monochrome-music/monochrome
Length of output: 866
Align the global type with the injected value.
vite.config.ts injects either a string ('/proxy-audio') or null, but the ambient declaration is string | undefined. Change this to string | null to match the actual runtime type.
🛠️ Proposed type fix
- const __VITE_PROXY__: string | undefined;
+ const __VITE_PROXY__: string | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const __VITE_PROXY__: string | undefined; | |
| const __VITE_PROXY__: string | null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/global.d.ts` at line 37, Update the ambient declaration for the injected
Vite value __VITE_PROXY__ so its type matches the runtime injection (null or
string): locate the const __VITE_PROXY__ declaration in the global type file and
change its type from string | undefined to string | null so consumers and
type-checking align with the actual injected value.
| function proxyAudioPlugin() { | ||
| return { | ||
| name: 'proxy-audio-dev', | ||
| configureServer(server) { | ||
| // No longer needed: local proxy-audio middleware replaced by remote proxy | ||
| server.middlewares.use('/proxy-audio', async (req, res) => { | ||
| const url = new URL(req.url, 'http://localhost'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new middleware no longer relies on implicit/unsafe any-typed values.
rg -n -C2 'function proxyAudioPlugin|configureServer|middlewares\.use|catch \(error' vite.config.tsRepository: monochrome-music/monochrome
Length of output: 680
🏁 Script executed:
cat -n vite.config.tsRepository: monochrome-music/monochrome
Length of output: 8547
🏁 Script executed:
rg -n "type Plugin|type ViteDevServer" --type tsRepository: monochrome-music/monochrome
Length of output: 167
🏁 Script executed:
rg "ViteDevServer" --type ts -A2 -B2Repository: monochrome-music/monochrome
Length of output: 53
🏁 Script executed:
rg "configureServer" --type ts -A3 -B1Repository: monochrome-music/monochrome
Length of output: 678
🏁 Script executed:
cat package.json | grep -A2 -B2 "vite"Repository: monochrome-music/monochrome
Length of output: 1904
🏁 Script executed:
cat -n vite-plugin-blob.ts | head -20Repository: monochrome-music/monochrome
Length of output: 802
🏁 Script executed:
cat -n vite-plugin-blob.ts | sed -n '20,50p'Repository: monochrome-music/monochrome
Length of output: 1257
🏁 Script executed:
cat -n vite-plugin-blob.ts | sed -n '55,75p'Repository: monochrome-music/monochrome
Length of output: 872
🏁 Script executed:
cat -n vite-plugin-blob.ts | sed -n '80,105p'Repository: monochrome-music/monochrome
Length of output: 944
🏁 Script executed:
cat -n vite-plugin-blob.ts | sed -n '105,125p'Repository: monochrome-music/monochrome
Length of output: 914
🏁 Script executed:
npm view vite versions | grep "7.3"Repository: monochrome-music/monochrome
Length of output: 272
🏁 Script executed:
python3 << 'EOF'
import subprocess
import json
# Check Vite 7.3.1 type exports
result = subprocess.run(
["npm", "view", "vite@7.3.1", "types"],
capture_output=True,
text=True
)
print("Vite 7.3.1 types entry:", result.stdout.strip())
# Check if ViteDevServer is a valid export
# We'll search node_modules if available, but in sandbox it might not be
result2 = subprocess.run(
["npm", "view", "vite@7.3.1", "dist-tags"],
capture_output=True,
text=True
)
print("\nVite dist-tags:", result2.stdout.strip())
EOFRepository: monochrome-music/monochrome
Length of output: 229
🏁 Script executed:
web_search
Vite configureServer type ViteDevServer Plugin APIRepository: monochrome-music/monochrome
Length of output: 164
🌐 Web query:
Vite ViteDevServer type exports configureServer middleware Node.js types
💡 Result:
In Vite, the ViteDevServer type is exported from the 'vite' package and provides full TypeScript support for the development server created via createServer(inlineConfig?: InlineConfig): Promise. Key properties for middleware: - middlewares: Connect.Server - A Connect app instance for attaching custom Node.js middlewares (compatible with Express, etc.). Use server.middlewares.use to add middleware. Full ViteDevServer interface (from official docs): interface ViteDevServer { config: ResolvedConfig middlewares: Connect.Server httpServer: http.Server | null watcher: FSWatcher ws: WebSocketServer pluginContainer: PluginContainer moduleGraph: ModuleGraph resolvedUrls: ResolvedServerUrls | null // methods: listen, close, restart, etc. } For plugins, the configureServer hook receives a ViteDevServer parameter: export default defineConfig({ plugins: [{ name: 'example', configureServer(server: ViteDevServer) { // Add middleware server.middlewares.use((req, res, next) => { // Custom logic next }) } }] }) All Vite JS APIs are fully typed. Import as: import type { ViteDevServer } from 'vite' Vite's middlewares use Connect (senchalabs/connect), a Node.js middleware framework.
Citations:
- 1: https://main.vitejs.dev/guide/api-javascript.html
- 2: https://vite.dev/config/server-options
- 3: https://github.com/vitejs/vite/blob/main/docs/guide/api-javascript.md
- 4: Use
PreviewServerparameter type inconfigurePreviewServerhook vitejs/vite#11631 - 5: https://main.vitejs.dev/config/server-options
🏁 Script executed:
python3 << 'EOF'
# Check Node.js type definitions for IncomingMessage and ServerResponse
# These are standard Node.js http module types
print("Node.js http module types:")
print("- IncomingMessage: from 'http' or 'node:http'")
print("- ServerResponse: from 'http' or 'node:http'")
print("\nThese are standard Node.js built-in types, always available in `@types/node`")
EOFRepository: monochrome-music/monochrome
Length of output: 264
Type the Vite middleware to unblock lint and fix HEAD request handling.
The plugin leaves server, req, res, and error untyped, blocking CI. Additionally, HEAD requests unnecessarily buffer the entire upstream response body to compute content-length, and the proxy URL is injected based on mode rather than command, causing it to appear in production builds with --mode development.
Proposed fixes
-import { defineConfig } from 'vite';
+import { defineConfig, type Plugin, type ViteDevServer } from 'vite';
+import type { IncomingMessage, ServerResponse } from 'node:http';
-function proxyAudioPlugin() {
+function proxyAudioPlugin(): Plugin {
return {
name: 'proxy-audio-dev',
- configureServer(server) {
- server.middlewares.use('/proxy-audio', async (req, res) => {
- const url = new URL(req.url, 'http://localhost');
+ configureServer(server: ViteDevServer) {
+ server.middlewares.use('/proxy-audio', async (req: IncomingMessage, res: ServerResponse) => {
+ const url = new URL(req.url ?? '/', 'http://localhost');
const targetUrl = url.searchParams.get('url'); const upstream = await fetch(targetUrl, {
method: req.method,
headers,
redirect: 'follow',
});
- const body = Buffer.from(await upstream.arrayBuffer());
+ const body = req.method === 'HEAD'
+ ? Buffer.alloc(0)
+ : Buffer.from(await upstream.arrayBuffer());- } catch (error) {
+ } catch (error: unknown) {
res.writeHead(500);
- res.end('Proxy Error: ' + error.message);
+ const message = error instanceof Error ? error.message : String(error);
+ res.end('Proxy Error: ' + message);
}- __VITE_PROXY__: JSON.stringify(_options.mode == "development" ? '/proxy-audio' : null),
+ __VITE_PROXY__: JSON.stringify(_options.command === 'serve' ? '/proxy-audio' : null),🧰 Tools
🪛 GitHub Actions: Lint Codebase
[error] 17-17: ESLint (@typescript-eslint/no-unsafe-call): Unsafe call of an any typed value.
[error] 17-17: ESLint (@typescript-eslint/no-unsafe-member-access): Unsafe member access .middlewares on an any value.
[error] 18-18: ESLint (@typescript-eslint/no-unsafe-argument): Unsafe argument of type any assigned to a parameter of type string | URL.
[error] 18-18: ESLint (@typescript-eslint/no-unsafe-member-access): Unsafe member access .url on an any value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.ts` around lines 13 - 18, The proxyAudioPlugin middleware leaves
types out and mishandles HEAD and environment detection: add proper TypeScript
types for configureServer's server (ViteDevServer) and the middleware parameters
req/ res/ next (use http.IncomingMessage, http.ServerResponse or Vite types) and
type any caught error, change the dev/production check to use
server.config.command === 'serve' (or accept a command param) so the proxy URL
is only injected in dev, and fix HEAD handling in the middleware to avoid
buffering the upstream body — on HEAD just proxy response headers (status and
headers) and end the response without reading the body; also ensure other
methods stream the upstream response to res and handle and log typed errors (the
symbols to edit are proxyAudioPlugin, configureServer and the middleware
function passed to server.middlewares.use).
| define: { | ||
| __COMMIT_HASH__: JSON.stringify(commitHash), | ||
| __VITEST__: !!process.env.VITEST, | ||
| __VITE_PROXY__: JSON.stringify(_options.mode == "development" ? '/proxy-audio' : null), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify proxy selection uses the Vite command rather than mode.
rg -n -C2 '__VITE_PROXY__|defineConfig\(\(_options\)' vite.config.tsRepository: monochrome-music/monochrome
Length of output: 426
Use command === 'serve' instead of mode == "development" for dev-only proxy.
Using mode is unreliable because it's user-selectable. Running vite build --mode development would set mode to "development" during the build, causing the proxy to be baked into the production bundle. Only command === 'serve' guarantees the proxy is set during development.
🛠️ Proposed fix
- __VITE_PROXY__: JSON.stringify(_options.mode == "development" ? '/proxy-audio' : null),
+ __VITE_PROXY__: JSON.stringify(_options.command === 'serve' ? '/proxy-audio' : null),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| __VITE_PROXY__: JSON.stringify(_options.mode == "development" ? '/proxy-audio' : null), | |
| __VITE_PROXY__: JSON.stringify(_options.command === 'serve' ? '/proxy-audio' : null), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.ts` at line 85, The build currently sets __VITE_PROXY__ based on
_options.mode which can be overridden; update the logic to use _options.command
=== 'serve' so the proxy string '/proxy-audio' is only embedded during dev
serve; locate the assignment of __VITE_PROXY__ in vite.config.ts and replace the
mode check (_options.mode == "development") with a command check
(_options.command === "serve") so JSON.stringify(_options.command === "serve" ?
'/proxy-audio' : null) is emitted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/api.js (1)
1684-1696:⚠️ Potential issue | 🟠 MajorHydrated album fields still clobbered by nullish values on
track.album.Spreading
track?.albumafterfullTrack?.albumletsnull/undefinedfields on the incoming track (notablynumberOfTracks, which is one of the triggers forhasMissingDownloadMetadata) overwrite the freshly fetched values. The surrounding scalar fields (trackNumber,volumeNumber,discNumber) correctly use??fallbacks, but the nested album merge does not, so the hydration branch can still leavealbum.numberOfTracks == nulland the downstream download metadata stays incomplete.The same applies to
artists:track?.artists?.lengthis already guarded, but consider applying the same nullish-aware merge for album sub-fields.🔧 Proposed fix
- album: { - ...(fullTrack?.album || {}), - ...(track?.album || {}), - }, + album: (() => { + const base = { ...(fullTrack?.album || {}) }; + for (const [k, v] of Object.entries(track?.album || {})) { + if (v != null) base[k] = v; + } + return base; + })(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/api.js` around lines 1684 - 1696, The album merge currently spreads track?.album after fullTrack?.album which allows null/undefined album sub-fields (e.g., numberOfTracks) from track to overwrite hydrated values; in the block that builds the object passed to this.prepareTrack (the track/album merge), change the merge so you start with ...(fullTrack?.album || {}) and only copy non-nullish values from track.album (e.g., for each sub-field like numberOfTracks, releaseDate, etc. use the track value if it is not null/undefined, otherwise keep the fullTrack value), or perform a nullish-aware shallow merge utility before passing to this.prepareTrack; reference the variables fullTrack, track, and the call to this.prepareTrack when making the fix.
🧹 Nitpick comments (1)
js/api.js (1)
1857-1864: DASH proxy removal matchesdash-downloader.ts, HLS path is now asymmetric.Passing
streamUrldirectly intodownloadDashStreamis consistent withdash-downloader.tsapplyinggetProxyUrl()internally on each segment/HEAD (seejs/dash-downloader.tsLines 34, 79). Just flagging that the sibling HLS path on Line 1879 still wraps withgetProxyUrl(streamUrl)before handing off toHlsDownloader, so the two downloaders now have inverted responsibilities for proxy construction. Not a defect here, but it's an easy footgun for future edits — consider aligningHlsDownloaderto own its own proxying as well for symmetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/api.js` around lines 1857 - 1864, The Dash path passes raw streamUrl into DashDownloader.downloadDashStream (DashDownloader) which internally calls getProxyUrl for segments, while the HLS path wraps streamUrl with getProxyUrl(streamUrl) before creating HlsDownloader, creating asymmetry and future footguns; make these responsibilities consistent by either (A) removing the getProxyUrl wrapping at the HLS call site so HlsDownloader receives the raw streamUrl like DashDownloader, or (B) move proxy construction into HlsDownloader (add getProxyUrl usage inside HlsDownloader) and update callers to pass raw streamUrl; update the site that currently calls HlsDownloader and the HlsDownloader implementation accordingly (referencing HlsDownloader, DashDownloader, downloadDashStream, getProxyUrl, and streamUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/api.js`:
- Around line 1204-1207: The current creation of topTrackAlbumIds uses
.filter(Boolean) which removes numeric id 0; change the predicate to explicitly
exclude null/undefined and NaN instead. Update the topTrackAlbumIds construction
(the topTracksPool map -> Number(track?.album?.id) -> .filter(Boolean)) to use a
check like id != null && !Number.isNaN(id) (or Number.isFinite) so albums with
id 0 are kept; leave the rawReleases filtering logic (matchesArtistId and
topTrackAlbumIds.has(Number(album?.id))) intact so behavior matches
matchesArtistId's nullish/NaN style.
---
Duplicate comments:
In `@js/api.js`:
- Around line 1684-1696: The album merge currently spreads track?.album after
fullTrack?.album which allows null/undefined album sub-fields (e.g.,
numberOfTracks) from track to overwrite hydrated values; in the block that
builds the object passed to this.prepareTrack (the track/album merge), change
the merge so you start with ...(fullTrack?.album || {}) and only copy
non-nullish values from track.album (e.g., for each sub-field like
numberOfTracks, releaseDate, etc. use the track value if it is not
null/undefined, otherwise keep the fullTrack value), or perform a nullish-aware
shallow merge utility before passing to this.prepareTrack; reference the
variables fullTrack, track, and the call to this.prepareTrack when making the
fix.
---
Nitpick comments:
In `@js/api.js`:
- Around line 1857-1864: The Dash path passes raw streamUrl into
DashDownloader.downloadDashStream (DashDownloader) which internally calls
getProxyUrl for segments, while the HLS path wraps streamUrl with
getProxyUrl(streamUrl) before creating HlsDownloader, creating asymmetry and
future footguns; make these responsibilities consistent by either (A) removing
the getProxyUrl wrapping at the HLS call site so HlsDownloader receives the raw
streamUrl like DashDownloader, or (B) move proxy construction into HlsDownloader
(add getProxyUrl usage inside HlsDownloader) and update callers to pass raw
streamUrl; update the site that currently calls HlsDownloader and the
HlsDownloader implementation accordingly (referencing HlsDownloader,
DashDownloader, downloadDashStream, getProxyUrl, and streamUrl).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const topTrackAlbumIds = new Set(topTracksPool.map((track) => Number(track?.album?.id)).filter(Boolean)); | ||
| const rawReleases = Array.from(albumMap.values()).filter( | ||
| (album) => matchesArtistId(album) || topTrackAlbumIds.has(Number(album?.id)) | ||
| ); |
There was a problem hiding this comment.
.filter(Boolean) on numeric album IDs drops the id 0.
Number(track?.album?.id) returns 0 for an album id of 0 or "0", and .filter(Boolean) then strips it from the set. The rawReleases filter compares against this set with topTrackAlbumIds.has(Number(album?.id)), so albums with id 0 would be excluded from the artist page. Album 0 is unlikely in practice, but the safer predicate is an explicit nullish/NaN check to match the style of matchesArtistId.
🔧 Suggested tweak
- const topTrackAlbumIds = new Set(topTracksPool.map((track) => Number(track?.album?.id)).filter(Boolean));
+ const topTrackAlbumIds = new Set(
+ topTracksPool
+ .map((track) => (track?.album?.id != null ? Number(track.album.id) : null))
+ .filter((id) => id != null && !Number.isNaN(id))
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const topTrackAlbumIds = new Set(topTracksPool.map((track) => Number(track?.album?.id)).filter(Boolean)); | |
| const rawReleases = Array.from(albumMap.values()).filter( | |
| (album) => matchesArtistId(album) || topTrackAlbumIds.has(Number(album?.id)) | |
| ); | |
| const topTrackAlbumIds = new Set( | |
| topTracksPool | |
| .map((track) => (track?.album?.id != null ? Number(track.album.id) : null)) | |
| .filter((id) => id != null && !Number.isNaN(id)) | |
| ); | |
| const rawReleases = Array.from(albumMap.values()).filter( | |
| (album) => matchesArtistId(album) || topTrackAlbumIds.has(Number(album?.id)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/api.js` around lines 1204 - 1207, The current creation of topTrackAlbumIds
uses .filter(Boolean) which removes numeric id 0; change the predicate to
explicitly exclude null/undefined and NaN instead. Update the topTrackAlbumIds
construction (the topTracksPool map -> Number(track?.album?.id) ->
.filter(Boolean)) to use a check like id != null && !Number.isNaN(id) (or
Number.isFinite) so albums with id 0 are kept; leave the rawReleases filtering
logic (matchesArtistId and topTrackAlbumIds.has(Number(album?.id))) intact so
behavior matches matchesArtistId's nullish/NaN style.
DanTheMan827
force-pushed
the
copilot/fix-missing-album-metadata
branch
from
7378d71 to
ba9b374
Compare
This re-adds the proxy, but only for local development. If not dev, it uses the remote proxy.
Agent-Logs-Url: https://github.com/DanTheMan827/monochrome/sessions/8437a3f1-f7de-49d3-af64-b279ee108459 Co-authored-by: DanTheMan827 <790119+DanTheMan827@users.noreply.github.com>
DanTheMan827
force-pushed
the
copilot/fix-missing-album-metadata
branch
from
ba9b374 to
b77df88
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/player.js (1)
147-152:⚠️ Potential issue | 🟠 MajorRemove the no-op segment rewrite.
uris[i] = uris[i]does nothing and currently breaks CI onno-self-assign. If proxying really moved out of this Shaka filter, drop thetidal.combranch/import entirely; otherwise restore the actual rewrite here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/player.js` around lines 147 - 152, The branch inside the handler that checks request type (if (type === shaka.net.NetworkingEngine.RequestType.SEGMENT)) currently contains a no-op self-assignment for entries matching 'tidal.com' (the loop over request.uris with uris[i] = uris[i]) which triggers the no-self-assign lint error; either remove the entire 'tidal.com' branch/import if proxying was removed, or replace the no-op with the intended rewrite logic (e.g., transform uris[i] to the proxied URL or call the existing proxy helper) so the code performs a real mutation; update the code in the block that iterates request.uris to either delete the if (uris[i].includes('tidal.com')) branch or implement the correct rewrite using the project's proxy helper function (referencing request and uris variables).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/proxy-utils.js`:
- Around line 30-41: The fetch wrapper assumes input.url exists; instead detect
and handle string, URL, and Request inputs: if input is a string use new
URL(input, window.location.href); if input is a URL object use it directly; if
input is a Request create a URL via new URL(input.url, window.location.href) and
preserve the original Request options (method, headers, body, signal) when
making proxied calls (e.g. build a new Request or merge init with new Request
constructed from input) so ogFetch(window.fetch) is called with the correct
normalized URL (use getProxyUrl(url.href) when needed) and with preserved
request properties.
- Around line 11-23: The HTMLMediaElement.prototype 'src' setter currently
proxies every value (including '', blob:, data:, and same-origin URLs) and
silently no-ops when a value already contains __VITE_PROXY__; change it to only
rewrite remote http(s) Tidal URLs (i.e., external origins that should go through
the proxy) and otherwise delegate unchanged to the original setter: in the
HTMLMediaElement.prototype 'src' setter use URL parsing to detect scheme and
origin, skip rewriting for empty strings, blob: and data: schemes, and for
same-origin URLs, and when not rewriting call
originalSrcDescriptor.set.call(this, value) (or setAttribute('src', value) if no
original setter) so behavior and reset paths (e.g., src = '') and blob/data
playback remain intact; still use getProxyUrl(realUrl) only for the specific
remote http(s) cases and do not silently no-op when __VITE_PROXY__ is present —
instead delegate to the original setter.
In `@vite.config.ts`:
- Around line 48-58: The loop incorrectly does "await res.write(value)" which is
invalid and ignores backpressure; change the streaming loop that reads from
upstream.body?.getReader() so you call res.write(value) without await and, when
res.write(...) returns false, wait for the 'drain' event before continuing (e.g.
await a Promise that resolves on res.once('drain')), then continue reading; keep
the final res.end() after the loop. Target the reader loop surrounding
upstream.body?.getReader(), the res.write(...) call, and res.end() to implement
this backpressure handling.
---
Outside diff comments:
In `@js/player.js`:
- Around line 147-152: The branch inside the handler that checks request type
(if (type === shaka.net.NetworkingEngine.RequestType.SEGMENT)) currently
contains a no-op self-assignment for entries matching 'tidal.com' (the loop over
request.uris with uris[i] = uris[i]) which triggers the no-self-assign lint
error; either remove the entire 'tidal.com' branch/import if proxying was
removed, or replace the no-op with the intended rewrite logic (e.g., transform
uris[i] to the proxied URL or call the existing proxy helper) so the code
performs a real mutation; update the code in the block that iterates
request.uris to either delete the if (uris[i].includes('tidal.com')) branch or
implement the correct rewrite using the project's proxy helper function
(referencing request and uris variables).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 396ff90d-56d5-46cb-8acf-cb959ec95894
📒 Files selected for processing (8)
js/api.jsjs/app.jsjs/dash-downloader.tsjs/global.d.tsjs/hls-downloader.jsjs/player.jsjs/proxy-utils.jsvite.config.ts
✅ Files skipped from review due to trivial changes (1)
- js/dash-downloader.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- js/global.d.ts
- js/api.js
| Object.defineProperty(HTMLMediaElement.prototype, 'src', { | ||
| set(value) { | ||
| console.log(value); | ||
| const alreadyProxied = value.includes(__VITE_PROXY__); | ||
|
|
||
| if (!alreadyProxied) { | ||
| const realUrl = new URL(value, window.location.href).href; | ||
| if (originalSrcDescriptor && originalSrcDescriptor.set) { | ||
| originalSrcDescriptor.set.call(this, getProxyUrl(realUrl)); | ||
| } else { | ||
| this.setAttribute('src', getProxyUrl(realUrl)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.js" | grep -E "(proxy-utils|player|app)" | head -20Repository: monochrome-music/monochrome
Length of output: 143
🏁 Script executed:
cat -n js/proxy-utils.jsRepository: monochrome-music/monochrome
Length of output: 2247
🏁 Script executed:
rg "\.src\s*=" js/player.js -B 2 -A 2Repository: monochrome-music/monochrome
Length of output: 3075
🏁 Script executed:
# Verify URL behavior with empty string
node <<'EOF'
try {
const url = new URL('', 'https://example.com/path');
console.log('Empty string resolves to:', url.href);
} catch (e) {
console.log('Error:', e.message);
}
EOFRepository: monochrome-music/monochrome
Length of output: 121
🏁 Script executed:
# Check if there are any blob: or data: URLs being assigned in player.js
rg "blob:|data:" js/player.jsRepository: monochrome-music/monochrome
Length of output: 533
🏁 Script executed:
# Verify what types of URLs are used in streamUrl assignments
rg "streamUrl\s*=" js/player.js -B 3 -A 1Repository: monochrome-music/monochrome
Length of output: 1838
🏁 Script executed:
# Check what happens with already-proxied URLs - verify the missing delegation
rg -A 5 "alreadyProxied" js/proxy-utils.jsRepository: monochrome-music/monochrome
Length of output: 523
🏁 Script executed:
# Verify if there's a reason the setter doesn't delegate to original descriptor when already proxied
git log --oneline -20 js/proxy-utils.jsRepository: monochrome-music/monochrome
Length of output: 120
Don't proxy every media src.
This setter currently rewrites blob:, data:, same-origin, and even '' into the proxy URL. That breaks local-file playback, tracker blobs, podcasts, and the src = '' reset paths in js/player.js. When callers pass an already-proxied URL, the setter silently no-ops instead of delegating to the original setter.
Restrict rewriting to remote Tidal http(s) URLs, and delegate everything else unchanged to the original setter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/proxy-utils.js` around lines 11 - 23, The HTMLMediaElement.prototype 'src'
setter currently proxies every value (including '', blob:, data:, and
same-origin URLs) and silently no-ops when a value already contains
__VITE_PROXY__; change it to only rewrite remote http(s) Tidal URLs (i.e.,
external origins that should go through the proxy) and otherwise delegate
unchanged to the original setter: in the HTMLMediaElement.prototype 'src' setter
use URL parsing to detect scheme and origin, skip rewriting for empty strings,
blob: and data: schemes, and for same-origin URLs, and when not rewriting call
originalSrcDescriptor.set.call(this, value) (or setAttribute('src', value) if no
original setter) so behavior and reset paths (e.g., src = '') and blob/data
playback remain intact; still use getProxyUrl(realUrl) only for the specific
remote http(s) cases and do not silently no-op when __VITE_PROXY__ is present —
instead delegate to the original setter.
| const ogFetch = window.fetch; | ||
| window.fetch = async function (input, init, ...rest) { | ||
| let url = typeof input === 'string' ? new URL(input, window.location.href) : input.url; | ||
| if (url.href.includes(__VITE_PROXY__)) { | ||
| return await ogFetch(input, init, ...rest); | ||
| } else if ( | ||
| url.hostname.toLowerCase().endsWith('tidal.com') && | ||
| !url.hostname.toLowerCase().endsWith('api.tidal.com') | ||
| ) { | ||
| return await ogFetch(getProxyUrl(url.href), init, ...rest); | ||
| } else { | ||
| return await ogFetch(input, init, ...rest); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "proxy-utils.js" 2>/dev/null | head -20Repository: monochrome-music/monochrome
Length of output: 90
🏁 Script executed:
cat -n ./js/proxy-utils.jsRepository: monochrome-music/monochrome
Length of output: 2247
🏁 Script executed:
# Let's verify the Fetch API specification behavior for different input types
python3 << 'EOF'
# Let's document what the Fetch API actually accepts and how to handle each case
fetch_api_info = {
"String input": {
"example": "fetch('https://example.com')",
"properties": {
"typeof": "string",
".url": "does not exist",
"solution": "Parse as new URL()"
}
},
"URL object input": {
"example": "fetch(new URL('https://example.com'))",
"properties": {
"typeof": "object",
".url": "does not exist (URL objects don't have .url property)",
".href": "exists as string",
".hostname": "exists as string",
"solution": "Use directly or convert to string"
}
},
"Request object input": {
"example": "fetch(new Request('https://example.com'))",
"properties": {
"typeof": "object",
".url": "exists as STRING (not URL object)",
".href": "does not exist on Request",
".hostname": "does not exist on Request",
"solution": "Access .url as string, then parse as new URL()"
}
}
}
print("=== FETCH API INPUT TYPES ===\n")
for input_type, info in fetch_api_info.items():
print(f"{input_type}:")
print(f" Example: {info['example']}")
for prop, value in info['properties'].items():
print(f" {prop}: {value}")
print()
print("\n=== ISSUE ANALYSIS ===")
print("Current code (line 32):")
print(" let url = typeof input === 'string' ? new URL(input, window.location.href) : input.url;")
print("\nProblems:")
print("1. If input is URL object: input.url is undefined → url will be undefined")
print("2. If input is Request object: input.url is a STRING → url will be a string")
print("3. Then accessing url.href (line 33) will fail:")
print(" - If URL object: undefined.href → throws TypeError")
print(" - If Request: 'https://...'.href → undefined (strings don't have .href)")
print("4. Same issue with url.hostname (line 36)")
EOFRepository: monochrome-music/monochrome
Length of output: 1288
🏁 Script executed:
rg -t js "patchFetch\|new Request\|fetch(" --max-count 20Repository: monochrome-music/monochrome
Length of output: 183
🏁 Script executed:
rg -t js "patchFetch|new Request|window.fetch" --max-count 30Repository: monochrome-music/monochrome
Length of output: 1826
🏁 Script executed:
# Let's check if there are any direct calls to the patched fetch with Request objects in the client-side code
rg -t js "fetch\(" js/ --max-count 30 | head -50Repository: monochrome-music/monochrome
Length of output: 4294
Handle all fetch() input types: string, URL, and Request objects.
The code assumes input.url exists, which fails for URL objects (which have no .url property) and for Request objects (where .url is a string, not a URL object with .href/.hostname properties). This will throw errors when:
- Callers pass a URL:
urlbecomesundefined, thenurl.hrefthrowsTypeError - Callers pass a Request:
urlbecomes a string, thenurl.hrefandurl.hostnameareundefined
Detect each input type, normalize to a URL object, and preserve all Request properties (method, headers, body, signal) when proxying.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/proxy-utils.js` around lines 30 - 41, The fetch wrapper assumes input.url
exists; instead detect and handle string, URL, and Request inputs: if input is a
string use new URL(input, window.location.href); if input is a URL object use it
directly; if input is a Request create a URL via new URL(input.url,
window.location.href) and preserve the original Request options (method,
headers, body, signal) when making proxied calls (e.g. build a new Request or
merge init with new Request constructed from input) so ogFetch(window.fetch) is
called with the correct normalized URL (use getProxyUrl(url.href) when needed)
and with preserved request properties.
| const reader = upstream.body?.getReader(); | ||
|
|
||
| if (reader) { | ||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| await res.write(value); | ||
| } | ||
| } | ||
|
|
||
| res.end(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "vite.config.ts" -type fRepository: monochrome-music/monochrome
Length of output: 87
🏁 Script executed:
# Find vite.config.ts and check line count
fd "vite.config.ts" --type f -x wc -l {}Repository: monochrome-music/monochrome
Length of output: 91
🏁 Script executed:
sed -n '40,65p' ./vite.config.tsRepository: monochrome-music/monochrome
Length of output: 1057
🏁 Script executed:
head -30 ./vite.config.tsRepository: monochrome-music/monochrome
Length of output: 1303
🏁 Script executed:
# Check for eslint/tsconfig files
fd -e "eslint.config" -o -e ".eslintrc*" -o -e "tsconfig.json" | head -20Repository: monochrome-music/monochrome
Length of output: 187
🏁 Script executed:
# Check TypeScript config to understand await-thenable rule
cat ./tsconfig.json 2>/dev/null | head -40Repository: monochrome-music/monochrome
Length of output: 771
🏁 Script executed:
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \)Repository: monochrome-music/monochrome
Length of output: 89
🏁 Script executed:
# Let me also check the entire context around the proxy code to see all relevant lines
sed -n '30,70p' ./vite.config.tsRepository: monochrome-music/monochrome
Length of output: 1594
🏁 Script executed:
cat ./eslint.config.jsRepository: monochrome-music/monochrome
Length of output: 2411
Fix backpressure handling in proxy middleware — await res.write(value) triggers lint error and causes streaming buffer overflow.
ServerResponse.write() returns a boolean, not a Promise. Using await on it violates @typescript-eslint/await-thenable (set to error in eslint.config.js). Beyond the lint failure, this is a real streaming bug: the loop never waits for the 'drain' event when the write buffer fills, allowing the proxy to buffer arbitrarily far ahead of the client, causing memory issues with large media responses.
Suggested fix
+import { once } from 'node:events';
...
const reader = upstream.body?.getReader();
if (reader) {
while (true) {
const { done, value } = await reader.read();
if (done) break;
- await res.write(value);
+ if (!res.write(value)) {
+ await once(res, 'drain');
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reader = upstream.body?.getReader(); | |
| if (reader) { | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| await res.write(value); | |
| } | |
| } | |
| res.end(); | |
| const reader = upstream.body?.getReader(); | |
| if (reader) { | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| if (!res.write(value)) { | |
| await once(res, 'drain'); | |
| } | |
| } | |
| } | |
| res.end(); |
🧰 Tools
🪛 GitHub Actions: Lint Codebase
[error] 54-54: ESLint (@typescript-eslint/await-thenable): Unexpected await of a non-Promise (non-"Thenable") value
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.ts` around lines 48 - 58, The loop incorrectly does "await
res.write(value)" which is invalid and ignores backpressure; change the
streaming loop that reads from upstream.body?.getReader() so you call
res.write(value) without await and, when res.write(...) returns false, wait for
the 'drain' event before continuing (e.g. await a Promise that resolves on
res.once('drain')), then continue reading; keep the final res.end() after the
loop. Target the reader loop surrounding upstream.body?.getReader(), the
res.write(...) call, and res.end() to implement this backpressure handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
js/proxy-utils.js (2)
30-44:⚠️ Potential issue | 🔴 CriticalPast review feedback on the fetch wrapper is still unaddressed.
input.urlis only valid forRequestobjects, where it's a string — not aURL. Passing aURLinstance crashes here (undefined.href), and passing aRequestmakesurl.href/url.hostnameundefined, so the proxy branch is never taken. When the input is aRequest, itsmethod/headers/body/signalmust also be carried into the proxied call. Please normalize all three input types to aURLbefore the host checks and reconstruct theRequest(or mergeinit) when proxying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/proxy-utils.js` around lines 30 - 44, The fetch wrapper replaces window.fetch (ogFetch) but incorrectly assumes input.url is a URL and that url.hostname/href exist; normalize the incoming input to a URL by handling three cases — a string, a URL instance, or a Request (use new URL(request.url, window.location.href) for Request) — then perform the host checks on that URL; when you call getProxyUrl for proxied tidal requests, reconstruct the Request (or merge the provided init) preserving method, headers, body, and signal from the original Request or init before calling ogFetch(getProxyUrl(...), reconstructedRequestOrInit, ...rest) so headers/body/method are not lost; update window.fetch wrapper and use ogFetch/getProxyUrl and Request constructor names to locate the changes.
11-23:⚠️ Potential issue | 🔴 CriticalPast review feedback on this setter is still unaddressed.
The setter still rewrites
'',blob:,data:, and same-origin URLs through the proxy, and silently no-ops when the value is already proxied (instead of delegating to the original setter). This breakssrc = ''reset paths, blob playback, and any non-Tidal media. Please restrict rewriting to remotehttp(s)Tidal URLs and delegate all other values (including already-proxied) tooriginalSrcDescriptor.set.Additionally,
value.includes(__VITE_PROXY__)will produce false positives in dev where__VITE_PROXY__ === '/proxy'(any URL containing/proxyis treated as already-proxied), so the "already proxied" check needs to be more precise (e.g.,value.startsWith(__VITE_PROXY__ + '?')).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/proxy-utils.js` around lines 11 - 23, The HTMLMediaElement.prototype 'src' setter is too greedy: replace its logic so it only rewrites remote http(s) Tidal URLs (e.g., check URL.protocol is 'http:' or 'https:' and the hostname/path matches Tidal patterns) and delegates all other values—including empty string, blob:, data:, same-origin URLs (compare new URL.origin to window.location.origin), and already-proxied values—to the original setter; use a precise proxy check like value.startsWith(__VITE_PROXY__ + '?') instead of includes, and always call originalSrcDescriptor.set.call(this, value) (or fallback to this.setAttribute('src', value)) for cases that should not be proxied; when proxying, call originalSrcDescriptor.set.call(this, getProxyUrl(realUrl)) (or setAttribute) as before.js/api.js (2)
1204-1207:⚠️ Potential issue | 🟡 Minor
.filter(Boolean)still drops album id0.
Number(track?.album?.id)returns0for0or"0", which.filter(Boolean)then strips. TherawReleasesfilter (topTrackAlbumIds.has(Number(album?.id))) would then exclude an album with id0. Use an explicit nullish/NaNcheck to mirrormatchesArtistId.🔧 Proposed fix
- const topTrackAlbumIds = new Set(topTracksPool.map((track) => Number(track?.album?.id)).filter(Boolean)); + const topTrackAlbumIds = new Set( + topTracksPool + .map((track) => (track?.album?.id != null ? Number(track.album.id) : null)) + .filter((id) => id != null && !Number.isNaN(id)) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/api.js` around lines 1204 - 1207, The Set construction for topTrackAlbumIds uses .filter(Boolean) which drops valid id 0; change the filtering so it only removes nullish or NaN values after converting with Number, e.g., when building topTrackAlbumIds from topTracksPool use a predicate that keeps 0 (check id !== null && id !== undefined && !Number.isNaN(id)) so topTrackAlbumIds, topTracksPool, albumMap and the rawReleases filter (which uses matchesArtistId and topTrackAlbumIds.has(Number(album?.id))) will correctly include albums with id 0.
1690-1693:⚠️ Potential issue | 🟠 MajorAlbum merge still lets
track.albumnull fields overwrite hydrated values.Spreading
track.albumafterfullTrack.albummeans a source withalbum.numberOfTracks: null(which is exactly the trigger for hydration on Line 1674) blanks out the freshly-fetchednumberOfTracks. The hydration round-trip then has no effect on the very field it was meant to fix. Either filter nullish values out oftrack.albumbefore merging, or pin the critical fields explicitly with??.🔧 Proposed fix
album: { ...(fullTrack?.album || {}), ...(track?.album || {}), + numberOfTracks: track?.album?.numberOfTracks ?? fullTrack?.album?.numberOfTracks, + numberOfVolumes: track?.album?.numberOfVolumes ?? fullTrack?.album?.numberOfVolumes, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/api.js` around lines 1690 - 1693, The current album merge spreads track.album after fullTrack.album causing null/undefined fields from track.album (e.g., numberOfTracks) to overwrite hydrated values; update the merge in the album construction so you either filter out nullish values from track.album before spreading or explicitly nullish-coalesce critical fields: keep ...(fullTrack?.album || {}), then spread a filtered version of (track?.album || {}) that excludes properties with null/undefined, or explicitly set numberOfTracks: (track?.album?.numberOfTracks ?? fullTrack?.album?.numberOfTracks) to ensure hydrated values from fullTrack are not overwritten by nulls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/proxy-utils.js`:
- Line 13: Remove the noisy debug log by deleting the stray console.log(value)
statement (the debug logging of variable "value" in js/proxy-utils.js) so media
src assignments are not printed to the browser console; if retaining logs for
development is required, replace the raw console.log with a conditional or
proper debug/logger check (e.g., only log when in non-production or when a debug
flag is enabled) rather than leaving console.log(value) in the hot path.
- Around line 35-39: The hostname checks in the conditional are too permissive
because url.hostname.endsWith('tidal.com') will match look-alike domains; change
the logic in the block that references url.hostname to first compute a single
lowercased hostname (e.g., const hostname = url.hostname.toLowerCase()) and then
replace the condition with a dot-boundary check: allow when hostname ===
'tidal.com' || hostname.endsWith('.tidal.com') and exclude when hostname ===
'api.tidal.com' || hostname.endsWith('.api.tidal.com') and likewise for
'auth.tidal.com' (hostname === 'auth.tidal.com' ||
hostname.endsWith('.auth.tidal.com')); update the conditional around that block
(the else if using url.hostname.toLowerCase().endsWith('tidal.com')) to use
these stricter equality/.prefixed checks to avoid matching look-alike domains.
---
Duplicate comments:
In `@js/api.js`:
- Around line 1204-1207: The Set construction for topTrackAlbumIds uses
.filter(Boolean) which drops valid id 0; change the filtering so it only removes
nullish or NaN values after converting with Number, e.g., when building
topTrackAlbumIds from topTracksPool use a predicate that keeps 0 (check id !==
null && id !== undefined && !Number.isNaN(id)) so topTrackAlbumIds,
topTracksPool, albumMap and the rawReleases filter (which uses matchesArtistId
and topTrackAlbumIds.has(Number(album?.id))) will correctly include albums with
id 0.
- Around line 1690-1693: The current album merge spreads track.album after
fullTrack.album causing null/undefined fields from track.album (e.g.,
numberOfTracks) to overwrite hydrated values; update the merge in the album
construction so you either filter out nullish values from track.album before
spreading or explicitly nullish-coalesce critical fields: keep
...(fullTrack?.album || {}), then spread a filtered version of (track?.album ||
{}) that excludes properties with null/undefined, or explicitly set
numberOfTracks: (track?.album?.numberOfTracks ??
fullTrack?.album?.numberOfTracks) to ensure hydrated values from fullTrack are
not overwritten by nulls.
In `@js/proxy-utils.js`:
- Around line 30-44: The fetch wrapper replaces window.fetch (ogFetch) but
incorrectly assumes input.url is a URL and that url.hostname/href exist;
normalize the incoming input to a URL by handling three cases — a string, a URL
instance, or a Request (use new URL(request.url, window.location.href) for
Request) — then perform the host checks on that URL; when you call getProxyUrl
for proxied tidal requests, reconstruct the Request (or merge the provided init)
preserving method, headers, body, and signal from the original Request or init
before calling ogFetch(getProxyUrl(...), reconstructedRequestOrInit, ...rest) so
headers/body/method are not lost; update window.fetch wrapper and use
ogFetch/getProxyUrl and Request constructor names to locate the changes.
- Around line 11-23: The HTMLMediaElement.prototype 'src' setter is too greedy:
replace its logic so it only rewrites remote http(s) Tidal URLs (e.g., check
URL.protocol is 'http:' or 'https:' and the hostname/path matches Tidal
patterns) and delegates all other values—including empty string, blob:, data:,
same-origin URLs (compare new URL.origin to window.location.origin), and
already-proxied values—to the original setter; use a precise proxy check like
value.startsWith(__VITE_PROXY__ + '?') instead of includes, and always call
originalSrcDescriptor.set.call(this, value) (or fallback to
this.setAttribute('src', value)) for cases that should not be proxied; when
proxying, call originalSrcDescriptor.set.call(this, getProxyUrl(realUrl)) (or
setAttribute) as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12424499-0754-4125-9041-3a55e84e40a9
📒 Files selected for processing (2)
js/api.jsjs/proxy-utils.js
|
|
||
| Object.defineProperty(HTMLMediaElement.prototype, 'src', { | ||
| set(value) { | ||
| console.log(value); |
There was a problem hiding this comment.
Remove debug console.log.
This logs every media src assignment in production. It's noisy and may leak stream URLs (which can include signed query parameters) into the browser console.
🧹 Proposed fix
Object.defineProperty(HTMLMediaElement.prototype, 'src', {
set(value) {
- console.log(value);
const alreadyProxied = value.includes(__VITE_PROXY__);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/proxy-utils.js` at line 13, Remove the noisy debug log by deleting the
stray console.log(value) statement (the debug logging of variable "value" in
js/proxy-utils.js) so media src assignments are not printed to the browser
console; if retaining logs for development is required, replace the raw
console.log with a conditional or proper debug/logger check (e.g., only log when
in non-production or when a debug flag is enabled) rather than leaving
console.log(value) in the hot path.
| } else if ( | ||
| url.hostname.toLowerCase().endsWith('tidal.com') && | ||
| !url.hostname.toLowerCase().endsWith('api.tidal.com') && | ||
| !url.hostname.toLowerCase().endsWith('auth.tidal.com') | ||
| ) { |
There was a problem hiding this comment.
endsWith('tidal.com') allows look-alike hostnames.
hostname.endsWith('tidal.com') matches not only Tidal subdomains but also mytidal.com, faketidal.com, etc. — those would be routed through the proxy as if they were Tidal. The exclusion checks (endsWith('api.tidal.com'), endsWith('auth.tidal.com')) have the same shape. Anchor matches at the dot boundary (or use exact host equality) so look-alike domains aren't silently proxied.
🛡️ Proposed fix
- } else if (
- url.hostname.toLowerCase().endsWith('tidal.com') &&
- !url.hostname.toLowerCase().endsWith('api.tidal.com') &&
- !url.hostname.toLowerCase().endsWith('auth.tidal.com')
- ) {
+ } else {
+ const host = url.hostname.toLowerCase();
+ const isTidalHost = host === 'tidal.com' || host.endsWith('.tidal.com');
+ const isExcluded =
+ host === 'api.tidal.com' ||
+ host.endsWith('.api.tidal.com') ||
+ host === 'auth.tidal.com' ||
+ host.endsWith('.auth.tidal.com');
+ if (isTidalHost && !isExcluded) {
return await ogFetch(getProxyUrl(url.href), init, ...rest);
- } else {
+ }
return await ogFetch(input, init, ...rest);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/proxy-utils.js` around lines 35 - 39, The hostname checks in the
conditional are too permissive because url.hostname.endsWith('tidal.com') will
match look-alike domains; change the logic in the block that references
url.hostname to first compute a single lowercased hostname (e.g., const hostname
= url.hostname.toLowerCase()) and then replace the condition with a dot-boundary
check: allow when hostname === 'tidal.com' || hostname.endsWith('.tidal.com')
and exclude when hostname === 'api.tidal.com' ||
hostname.endsWith('.api.tidal.com') and likewise for 'auth.tidal.com' (hostname
=== 'auth.tidal.com' || hostname.endsWith('.auth.tidal.com')); update the
conditional around that block (the else if using
url.hostname.toLowerCase().endsWith('tidal.com')) to use these stricter
equality/.prefixed checks to avoid matching look-alike domains.
Description
This adds previously missing metadata to tracks downloaded from outside of the album page, fixes copyright with the new API, and adds click handlers for video cards.
It also fixes dash downloading
Type of Change
Checklist
By submitting this PR, I agree to follow the guidelines. I understand that the final decision to merge rests with the maintainers and that not all contributions can be accepted.
Summary by CodeRabbit
New Features
Bug Fixes
Chores