Conversation
# Conflicts: # public/music/js/download_manager.js
# Conflicts: # public/music/app.js # src/server/server.ts
|
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:
📝 WalkthroughWalkthroughThis PR adds network playlist auto-check, updates source-switch playback resolution, expands download/cache progress handling, adds local music pagination, and adjusts batch, artist, SDK, server, and service worker flows. ChangesMusic Platform Feature Update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
public/music/js/songlist_manager.js (1)
558-570: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset sort state or abort when tag loading fails.
changeSource()clearstags, but it keeps the previous source’ssortList/sortId. SinceloadTags()swallows its own fetch errors, thisawaitstill completes andloadList(1)can run with a stalesortId, mixing the new source with the old source’s sort settings.Suggested fix
changeSource: async function () { currentState.source = document.getElementById('songlist-source').value; @@ currentState.tagId = ''; currentState.tagName = '全部分类'; document.getElementById('current-tag-name').innerText = '全部分类'; currentState.tags = []; + currentState.sortList = []; + currentState.sortId = ''; renderSortTabs(); - await loadTags(); - loadList(1); + const loaded = await loadTags(); + if (!loaded) return; + loadList(1); },// outside this hunk, make loadTags() return true on success / false on failure🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/songlist_manager.js` around lines 558 - 570, In changeSource(), the previous source’s sort state is left intact, so a failed or partially loaded tag refresh can still let loadList(1) run with a stale sortId. Update the source-switch flow to either clear/reset currentState.sortList and currentState.sortId before reloading, or make loadTags() return success/failure and have changeSource() abort before loadList(1) when tag loading fails. Use the existing changeSource() and loadTags() symbols to keep the new source from inheriting the old source’s sort settings.public/music/js/download_manager.js (3)
972-1029: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid double-starting failed server retries.
For server tasks, this branch pushes the task as
waitingand also starts an async/api/music/cache/downloadPOST. The finalprocessQueue()can pick the same waiting task before that async path resumes, causing duplicate server downloads and bypassing the concurrency limit. LetprocessQueue()callstartServerDownload().Proposed fix
if (t.isServer) { - // 云端任务：重新 resolve URL 并触发后端下载 t.status = 'waiting'; this.tasks.push(t); - this.renderTask(t); - - // 异步重新触发云端下载 - (async () => { - try { - const downloadResolver = await this.waitForDownloadResolver(); - // 尝试通过 QualityManager 将可能是显示名称的 quality 转换为原始 code - let quality = t.quality; - if (window.QualityManager) { - // getBestQuality 能处理原始 code 和 preferred 偏好，传入 t.quality 作为偏好，让其降级匹配 - quality = window.QualityManager.getBestQuality(t.song, quality); - } - const result = await downloadResolver(t.song, quality, true); - if (!result || !result.url) throw new Error('获取地址失败'); - - const resolvedSong = result.songInfo || t.song; - if (resolvedSong !== t.song) { - t.song = resolvedSong; - } - t.serverSongKey = this.getServerSongKey(resolvedSong, quality); - this.renderTask(t); - - // [Fix] 还原代理 URL 为原始外部 URL - let rawUrl = this.extractRawDownloadUrl(result.url); - if (!rawUrl.startsWith('http')) throw new Error('无法获取有效的外部下载地址'); - - const headers = { 'Content-Type': 'application/json', ...(window.getUserAuthHeaders ? window.getUserAuthHeaders() : {}) }; - - const res = await fetch('/api/music/cache/download', { - method: 'POST', - headers, - body: JSON.stringify({ songInfo: this.getSongInfoForServer(resolvedSong), url: rawUrl, quality, enableOnlyDownloadMode: window.settings?.enableOnlyDownloadMode || false, namingPattern: window.settings?.serverCacheNamingPattern || 'simple', cacheLyric: window.settings?.enableServerLyricCache !== false, embedLyric: !!(window.settings?.embedLyricToFile ?? true) }) - }); - if (!res.ok) throw new Error('服务器拒绝缓存'); - - t.status = 'downloading'; - this.renderTask(t); - this.saveTasks(); - } catch (err) { - console.warn('[DownloadManager] Retry cloud task failed:', t.song.name, err); - t.status = 'error'; - t.errorMsg = err.message || '重试失败'; - this.renderTask(t); - } - })(); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/download_manager.js` around lines 972 - 1029, The server retry path in the download manager is starting the download twice by both pushing a failed server task back as waiting and also kicking off the async cache POST directly. Update the retry flow in the relevant branch of download task handling so it only re-queues the task and lets processQueue() invoke startServerDownload() for server jobs, instead of launching the request inline. Keep the existing waiting/status updates and ensure startServerDownload() remains the single entry point for server-side retries.
868-882: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winContinue the queue after pausing an active server task.
Pausing a server
downloading/taggingtask frees a concurrency slot, but this path only updates progress and never callsprocessQueue(), so queued tasks can stall until another user action.Proposed fix
this.renderTask(task); this.saveTasks(); this.updateGlobalProgress(); + this.processQueue();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/download_manager.js` around lines 868 - 882, Pausing an active server task in the DownloadManager flow updates the task state but does not restart queue processing, so queued items can remain blocked. In the branch that handles `task.status === 'downloading' || task.status === 'waiting' || task.status === 'tagging'`, after setting the task to paused and refreshing UI/state, invoke `this.processQueue()` so the freed concurrency slot is immediately reused. Keep the change localized to the existing pause path in `download_manager.js` and preserve the current stop-request behavior.
1056-1065: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winInclude auth headers on the stop-all request. For non-public users,
/api/music/cache/stoprequiresverifyUserAuth(req), so sending onlyx-user-namemakes this call fail while the UI still clears the tasks locally. ReusegetUserAuthHeaders()here like the other stop paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/download_manager.js` around lines 1056 - 1065, The stop-all cache request in DownloadManager is missing the authenticated headers required by the server, so it can fail for non-public users while the UI still clears tasks locally. Update the fetch call in the stop-all path to reuse getUserAuthHeaders() (as done in the other stop flows) and include those auth headers together with the existing x-user-name value when calling /api/music/cache/stop.src/server/fileCache.ts (1)
1515-1525: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle cover download request errors and timeouts. The cover fetch only listens to the response stream; request-level DNS/connect errors on
p.get(...)can go unhandled, and a stalled server can leavetaggingstuck indefinitely. The other cover-download path in this file already attachesreq.on('error')and a timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/fileCache.ts` around lines 1515 - 1525, The cover download logic in the image fetch block only handles response stream events, so request-level failures and hangs can leave tagging stuck. Update the download path that uses p.get in fileCache.ts to keep a reference to the request, attach a request error handler and a timeout like the other cover-download code in this file, and make sure the Promise rejects/cleans up on DNS/connect failures or stalled responses.
🧹 Nitpick comments (1)
src/server/server.ts (1)
3783-3790: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding or caching the 100-page artist fetch fan-out.
This endpoint can now make up to 100 serial SDK calls for one request. A small cache, timeout, or lower configurable cap would reduce rate-limit and latency risk for large artists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 3783 - 3790, The artist song fetch loop in the server endpoint can fan out to 100 serial SDK calls per request, which risks latency and rate limiting. Update the logic around musicSdk[source].extendDetail.getArtistSongs to use a lower configurable page cap or add caching/timeout protection, and keep the MAX_PAGES bound from driving unbounded work for large artists. Make the change in the loop that builds allSongs so repeated requests are less expensive while preserving the existing pagination break conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/music/app.js`:
- Around line 5414-5418: The download concurrency value is only being clamped
for the UI/manager action, while settings.downloadConcurrency and localStorage
can still keep an invalid server-loaded value. Update the normalization path
around syncSettingsUI and the settings save/load flow to apply the same
normalize hook before assigning or persisting the value, so the stored
concurrency always matches the validated range used by the UI and manager
action.
- Around line 2668-2671: The image fallback handler in img.onerror still leaves
the original data-src value in place, so lazyLoadImages(...) can keep
re-observing and retrying the same broken URL. Update the error path for the
image loading logic to clear or remove the failed source metadata after
switching to the /music/assets/logo.svg fallback, and make sure the behavior is
applied in the same image-loading flow where img.onerror is defined.
- Around line 2873-2874: The retry key in the candidate handling logic is
collapsing several song identities into an empty fallback, which can cause
distinct candidates to be treated as duplicates. Update the key generation
around the candidate retry check to include all available identifiers used by
different sources, especially hash, copyrightId, mid, and mediaMid, alongside
source and quality. Use the existing candidateSong/tried flow to ensure each
unique song candidate gets its own retry key without changing the retry dedup
behavior for truly identical entries.
- Around line 2922-2926: The singer matching logic in isSingerMatch is too
permissive because it returns true when either singer value is empty, allowing
candidates without a singer to match a target that requires one. Update the
matching flow so empty sourceSinger or targetSinger does not count as a match
when the target song has a singer, and ensure the candidate selection/download
path that uses isSingerMatch rejects these cases. Use isSingerMatch and the
candidate match check near the target-song validation to locate the fix.
- Around line 3955-3959: The retry path in retryHandler/playSong is removing the
failed cache entry with targetQuality, but the cached URL was stored under the
resolved currentQuality, so the stale entry can remain after source switching or
degradation. Update the localStorage key removal in the currentSourceType ===
'cache' branch to use the resolved quality identifier from the failed entry, and
keep the change scoped to the retry logic in retryHandler and related playSong
cache-key handling.
In `@public/music/js/download_manager.js`:
- Around line 1370-1371: The marquee update path in applyMarqueeChecks is using
data-text and then writing it back through innerHTML, which can turn decoded
song names into markup; update the helper so downloaded song rows are rendered
from textContent or safe DOM nodes instead of HTML. Make the fix in the
applyMarqueeChecks flow and any related downloaded-row title rendering logic in
download_manager.js so escaped song names are never passed into the marquee
helper or reinserted as HTML.
- Around line 516-523: `DownloadManager` is using `serverSongKey` as `task.id`,
which then gets interpolated by `renderTaskHtml()` into DOM IDs and inline
`onclick` handlers via `innerHTML`. Keep `serverSongKey` only for backend
polling, and generate a separate opaque, UI-safe task identifier when building
the task object in the task creation path so `renderTaskHtml()`, the related
click handlers, and any task lookup logic use the safe ID instead of
song-derived metadata.
- Around line 52-63: `extractRawDownloadUrl()` only unwraps relative proxy
paths, so absolute same-origin download URLs are being left as proxy URLs
instead of the external media URL. Update the guard in `extractRawDownloadUrl`
to recognize both `/api/music/download` and absolute URLs that point to the same
origin plus that path, then continue extracting the `url` query parameter and
decoding it as today. Keep the change localized to `download_manager.js` and
preserve the existing fallback behavior for non-proxy URLs and parse failures.
In `@public/music/js/local_music.js`:
- Around line 696-699: The paginated re-render in local_music.js leaves old
cover elements observed by the shared lazy-load observer, so clean up the
previous page’s lazy-image targets before replacing container.innerHTML or add
removal handling inside window.lazyLoadImages in public/music/app.js. Use the
existing symbols container and window.lazyLoadImages to either unobserve
detached nodes/scoped entries or reset the observer state before each
page/filter render so stale offscreen covers do not accumulate.
- Around line 507-525: `updatePagination()` in `localMusic` should normalize
`this.currentPage` before building the pagination UI, because deleting items can
leave it pointing past `getTotalPages()` and produce invalid page text. Clamp
`currentPage` to the valid range using the current `displayData.length` and
`pageSize`, then continue updating `lm-page-info`, `lm-page-prev`, and
`lm-page-next` with the corrected value. Make sure the same adjustment also
covers the empty-state path that skips `getPageSlice()` so the page state stays
consistent after deletions.
In `@public/music/js/single_song_ops.js`:
- Around line 116-118: The lyric-cache sync path in the `song` handling logic
currently returns a fulfilled promise when `source` or `songmid` is missing,
which makes retry callers think the request succeeded. Update the early-exit
branch in `single_song_ops.js` so the relevant async flow explicitly signals a
skipped/failed state instead of resolving normally, and make sure the retry
callers that invoke this path check that status before showing success.
In `@src/server/fileCache.ts`:
- Around line 1491-1493: The download completion path in fileCache.ts is using
fileStream’s close event as a success signal, which can mark aborted or partial
writes as complete. Update the completion handling in the fileStream listener to
use finish for the success path, keep close/error cleanup for failures, and
ensure tagging only proceeds when total === 0 or received >= total. Make the
change around the fileStream event handlers and the tagging flow that updates
cacheProgress so partial reads are rejected before any rename/index work
happens.
In `@src/server/server.ts`:
- Around line 3248-3251: The progress state is left stuck in tagging when
metadata tagging fails and the code falls back to Buffer.concat(chunks), so
polling never reaches a terminal status. Update the tagging completion flow in
server.ts around the proxyRes end handler and the fallback catch path to set
taskId to a finished state after the response is actually finalized, using the
existing fileCache.cacheProgress entry so clients see completion instead of
lingering in status: 'tagging'. Make the same adjustment in the other tagging
fallback path referenced by the diff so both code paths consistently finalize
progress.
- Around line 2353-2356: The download endpoint is mutating server-wide naming
settings by applying `namingPattern` directly in `server.ts` via
`fileCache.setNamingPattern` and `global.lx.config`, which bypasses the normal
settings-save authorization flow. Update the request handling so `namingPattern`
from downloads is only applied after the same permission checks used for saving
settings, or ignore it for unauthorized/restricted users; keep the change
localized around the download handler and the `fileCache`/`global.lx.config`
update path.
---
Outside diff comments:
In `@public/music/js/download_manager.js`:
- Around line 972-1029: The server retry path in the download manager is
starting the download twice by both pushing a failed server task back as waiting
and also kicking off the async cache POST directly. Update the retry flow in the
relevant branch of download task handling so it only re-queues the task and lets
processQueue() invoke startServerDownload() for server jobs, instead of
launching the request inline. Keep the existing waiting/status updates and
ensure startServerDownload() remains the single entry point for server-side
retries.
- Around line 868-882: Pausing an active server task in the DownloadManager flow
updates the task state but does not restart queue processing, so queued items
can remain blocked. In the branch that handles `task.status === 'downloading' ||
task.status === 'waiting' || task.status === 'tagging'`, after setting the task
to paused and refreshing UI/state, invoke `this.processQueue()` so the freed
concurrency slot is immediately reused. Keep the change localized to the
existing pause path in `download_manager.js` and preserve the current
stop-request behavior.
- Around line 1056-1065: The stop-all cache request in DownloadManager is
missing the authenticated headers required by the server, so it can fail for
non-public users while the UI still clears tasks locally. Update the fetch call
in the stop-all path to reuse getUserAuthHeaders() (as done in the other stop
flows) and include those auth headers together with the existing x-user-name
value when calling /api/music/cache/stop.
In `@public/music/js/songlist_manager.js`:
- Around line 558-570: In changeSource(), the previous source’s sort state is
left intact, so a failed or partially loaded tag refresh can still let
loadList(1) run with a stale sortId. Update the source-switch flow to either
clear/reset currentState.sortList and currentState.sortId before reloading, or
make loadTags() return success/failure and have changeSource() abort before
loadList(1) when tag loading fails. Use the existing changeSource() and
loadTags() symbols to keep the new source from inheriting the old source’s sort
settings.
In `@src/server/fileCache.ts`:
- Around line 1515-1525: The cover download logic in the image fetch block only
handles response stream events, so request-level failures and hangs can leave
tagging stuck. Update the download path that uses p.get in fileCache.ts to keep
a reference to the request, attach a request error handler and a timeout like
the other cover-download code in this file, and make sure the Promise
rejects/cleans up on DNS/connect failures or stalled responses.
---
Nitpick comments:
In `@src/server/server.ts`:
- Around line 3783-3790: The artist song fetch loop in the server endpoint can
fan out to 100 serial SDK calls per request, which risks latency and rate
limiting. Update the logic around musicSdk[source].extendDetail.getArtistSongs
to use a lower configurable page cap or add caching/timeout protection, and keep
the MAX_PAGES bound from driving unbounded work for large artists. Make the
change in the loop that builds allSongs so repeated requests are less expensive
while preserving the existing pagination break conditions.
🪄 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 Plus
Run ID: ce3ea529-be15-4620-a3c4-7a1b5cc9e41e
📒 Files selected for processing (13)
.gitignorepublic/music/app.jspublic/music/index.htmlpublic/music/js/batch_pagination.jspublic/music/js/download_manager.jspublic/music/js/local_music.jspublic/music/js/single_song_ops.jspublic/music/js/songlist_manager.jspublic/music/sw.jssrc/modules/utils/musicSdk/tx/extendDetail.jssrc/modules/utils/musicSdk/tx/songList.jssrc/server/fileCache.tssrc/server/server.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
public/music/js/local_music.js (1)
547-707: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEscape local metadata before writing the page HTML.
Line 704 injects a template built from
item.name,item.singer,item.album,item.subPath, anditem.filenamedirectly intoinnerHTML. Those values come from local file names / tags, so a crafted track can inject markup or break the inlineonclick/ attribute contexts here. Escape every interpolated field, and avoid embedding raw filenames in inline handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/local_music.js` around lines 547 - 707, The page renderer in the LocalMusic list is inserting untrusted local metadata directly into innerHTML, which can break markup or inject script. In the rendering block that builds each row and assigns to container.innerHTML, escape all interpolated fields from item.name, item.singer, item.album, item.subPath, item.filename, and item.source before templating them. Also remove raw filenames from inline onclick/onchange handlers in LocalMusicManager row actions and use a safe data-driven event binding approach instead.Source: Linters/SAST tools
public/music/js/download_manager.js (1)
620-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t send
namingPatternby default from cache downloads.The server rejects
namingPatternchanges for restricted public users unless admin auth is present, but this payload always includes one via the'simple'fallback. That turns normal public server-cache downloads into 403s. Only includenamingPatternwhen the client is intentionally authorized to change it, or persist it through the settings flow instead.Proposed fix
- const res = await fetch('/api/music/cache/download', { + const payload = { + songInfo: this.getSongInfoForServer(resolvedSong), + url: rawUrl, + quality, + enableOnlyDownloadMode: window.settings?.enableOnlyDownloadMode || false, + cacheLyric: window.settings?.enableServerLyricCache !== false, + embedLyric: !!(window.settings?.embedLyricToFile ?? true) + }; + const authHeaders = window.getUserAuthHeaders ? window.getUserAuthHeaders() : {}; + if (window.settings?.serverCacheNamingPattern && authHeaders['x-frontend-auth']) { + payload.namingPattern = window.settings.serverCacheNamingPattern; + } + + const res = await fetch('/api/music/cache/download', { method: 'POST', - headers, - body: JSON.stringify({ - songInfo: this.getSongInfoForServer(resolvedSong), - url: rawUrl, - quality, - enableOnlyDownloadMode: window.settings?.enableOnlyDownloadMode || false, - namingPattern: window.settings?.serverCacheNamingPattern || 'simple', - cacheLyric: window.settings?.enableServerLyricCache !== false, - embedLyric: !!(window.settings?.embedLyricToFile ?? true) - }) + headers, + body: JSON.stringify(payload) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/music/js/download_manager.js` around lines 620 - 628, The download request payload in the cache-download path is always sending a default namingPattern value, which triggers forbidden changes for public users. Update the request construction around body: JSON.stringify in the download flow to omit namingPattern unless the client is explicitly authorized to override it (or only include it when a persisted settings value is intentionally set), while keeping the other fields unchanged.src/server/server.ts (1)
3270-3338: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the temp tag file on fallback errors.
If
MusicTaggerfails aftertempPathis written, the catch path returns the original buffer but never unlinks the temp file. Move cleanup into afinallyor tracktempPathoutside the try.Proposed fix
+ let tempPath = '' try { const buffer = Buffer.concat(chunks) if (buffer.length < 100) throw new Error('File too small, possibly invalid'); // Use filename extension for temp file so MusicTagger can identify container format const ext = path.extname(filename) || '.mp3' - const tempPath = path.join(os.tmpdir(), `lx_tag_${Date.now()}${ext}`) + tempPath = path.join(os.tmpdir(), `lx_tag_${Date.now()}${ext}`) fs.writeFileSync(tempPath, new Uint8Array(buffer)) ... const tagged = fs.readFileSync(tempPath) - fs.unlink(tempPath, () => { }) headers['Content-Length'] = tagged.length.toString() ... } catch (e: any) { if (!res.headersSent) { res.writeHead(200, headers) res.end(Buffer.concat(chunks)) } finishProgress() + } finally { + if (tempPath) fs.unlink(tempPath, () => { }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 3270 - 3338, The temp metadata file created in the download/tagging flow is only deleted on the success path, so fallback errors in the MusicTagger logic can leave files behind. Update the download handler around the MusicTagger/save flow to ensure tempPath is always cleaned up, even when the catch path returns the original buffer, by moving fs.unlink into a finally block or otherwise guaranteeing cleanup after tagger.loadPath/save/dispose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@public/music/js/download_manager.js`:
- Around line 620-628: The download request payload in the cache-download path
is always sending a default namingPattern value, which triggers forbidden
changes for public users. Update the request construction around body:
JSON.stringify in the download flow to omit namingPattern unless the client is
explicitly authorized to override it (or only include it when a persisted
settings value is intentionally set), while keeping the other fields unchanged.
In `@public/music/js/local_music.js`:
- Around line 547-707: The page renderer in the LocalMusic list is inserting
untrusted local metadata directly into innerHTML, which can break markup or
inject script. In the rendering block that builds each row and assigns to
container.innerHTML, escape all interpolated fields from item.name, item.singer,
item.album, item.subPath, item.filename, and item.source before templating them.
Also remove raw filenames from inline onclick/onchange handlers in
LocalMusicManager row actions and use a safe data-driven event binding approach
instead.
In `@src/server/server.ts`:
- Around line 3270-3338: The temp metadata file created in the download/tagging
flow is only deleted on the success path, so fallback errors in the MusicTagger
logic can leave files behind. Update the download handler around the
MusicTagger/save flow to ensure tempPath is always cleaned up, even when the
catch path returns the original buffer, by moving fs.unlink into a finally block
or otherwise guaranteeing cleanup after tagger.loadPath/save/dispose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c0d01b0-f009-458b-91a2-730fc3c834e2
📒 Files selected for processing (7)
public/music/app.jspublic/music/js/download_manager.jspublic/music/js/local_music.jspublic/music/js/single_song_ops.jspublic/music/js/songlist_manager.jssrc/server/fileCache.tssrc/server/server.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- public/music/js/single_song_ops.js
- public/music/js/songlist_manager.js
- src/server/fileCache.ts
- public/music/app.js
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)
src/server/server.ts (1)
3794-3798: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist
artist.maxFetchPageswith the rest of the server config.This route reads
global.lx.config['artist.maxFetchPages'], but the shown/api/configGET/POST persistence block does not return, update, or write that key. Any later config save can silently drop the setting and revert artist fetching to the default.Suggested fix
'singer.sourcePriority': (global.lx.config['singer.sourcePriority'] || ['tx', 'wy']).join(','), + 'artist.maxFetchPages': global.lx.config['artist.maxFetchPages'] ?? 20, 'system.allowUnsafeVM': global.lx.config['system.allowUnsafeVM'] || false,if (newConfig['singer.sourcePriority'] !== undefined) { const priority = String(newConfig['singer.sourcePriority']).split(',').filter(s => s === 'tx' || s === 'wy') as Array<'tx' | 'wy'> if (priority.length > 0) global.lx.config['singer.sourcePriority'] = priority } + if (newConfig['artist.maxFetchPages'] !== undefined) { + const maxPages = Number(newConfig['artist.maxFetchPages']) + global.lx.config['artist.maxFetchPages'] = Number.isFinite(maxPages) && maxPages > 0 + ? Math.min(Math.floor(maxPages), 100) + : 20 + }'subsonic.path': global.lx.config['subsonic.path'], + 'artist.maxFetchPages': global.lx.config['artist.maxFetchPages'], 'system.allowUnsafeVM': global.lx.config['system.allowUnsafeVM'],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 3794 - 3798, The `/api/config` persistence flow is missing `artist.maxFetchPages`, so reads in the artist-fetching logic can be lost on save. Update the config GET/POST handling in the server config block to include `artist.maxFetchPages` when serializing, accepting updates, and writing back to `global.lx.config` alongside the other persisted settings. Make sure the same key used in the `MAX_PAGES` calculation remains round-trippable through the config save path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/server.ts`:
- Around line 3265-3272: The temporary tag file creation in the download/tagging
flow is not collision-safe, and the tagger can be left undisposed if an
exception occurs before cleanup. Update the logic around the temp file setup in
the server handler to use a unique temporary filename/creation approach instead
of Date.now()-based naming, and make sure the native tagger is always released
via a finally-style cleanup path in the same flow. Keep the fix localized to the
tempPath creation and the tagger lifecycle in the download processing code.
---
Outside diff comments:
In `@src/server/server.ts`:
- Around line 3794-3798: The `/api/config` persistence flow is missing
`artist.maxFetchPages`, so reads in the artist-fetching logic can be lost on
save. Update the config GET/POST handling in the server config block to include
`artist.maxFetchPages` when serializing, accepting updates, and writing back to
`global.lx.config` alongside the other persisted settings. Make sure the same
key used in the `MAX_PAGES` calculation remains round-trippable through the
config save path.
🪄 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 Plus
Run ID: bed6e3fa-4dd3-4582-8d9e-8483aadf8e11
📒 Files selected for processing (3)
public/music/js/download_manager.jspublic/music/js/local_music.jssrc/server/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- public/music/js/download_manager.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/server.ts (1)
2353-2364: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep server-wide naming changes behind admin auth.
The new guard only blocks restricted public users. Authenticated named users can still pass
namingPatternto/api/music/cache/downloadand mutatefileCacheplusglobal.lx.config['cache.namingPattern'], bypassing the admin-gated/api/configflow at Lines 4496-4502.Suggested fix
if (namingPattern) { - if (isPublic && global.lx.config['user.enablePublicRestriction']) { - const auth = req.headers['x-frontend-auth'] - if (auth !== global.lx.config['frontend.password']) { - res.writeHead(403, { 'Content-Type': 'application/json' }) - res.end(JSON.stringify({ success: false, error: 'Unauthorized to change cache naming pattern' })) - return - } + const auth = req.headers['x-frontend-auth'] + if (auth !== global.lx.config['frontend.password']) { + res.writeHead(403, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ success: false, error: 'Unauthorized to change cache naming pattern' })) + return } fileCache.setNamingPattern(namingPattern) if (global.lx.config) global.lx.config['cache.namingPattern'] = namingPattern }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 2353 - 2364, The naming pattern update in the cache download handler is not sufficiently protected, since only restricted public users are blocked and authenticated users can still mutate fileCache and global.lx.config['cache.namingPattern']. Move the namingPattern write behind the same admin-only authorization used by the /api/config flow, or add an equivalent admin check in this handler before calling fileCache.setNamingPattern and updating global.lx.config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/server.ts`:
- Around line 2353-2364: The naming pattern update in the cache download handler
is not sufficiently protected, since only restricted public users are blocked
and authenticated users can still mutate fileCache and
global.lx.config['cache.namingPattern']. Move the namingPattern write behind the
same admin-only authorization used by the /api/config flow, or add an equivalent
admin check in this handler before calling fileCache.setNamingPattern and
updating global.lx.config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4956ea66-253d-41f2-8db7-014a5559a9b5
📒 Files selected for processing (3)
src/defaultConfig.tssrc/server/server.tssrc/types/config.d.ts
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.
Summary
Notes
Summary by CodeRabbit