Fix local search, playlist auto-update, and download fallback handling#241
Open
bobcc4 wants to merge 26 commits into
Open
Fix local search, playlist auto-update, and download fallback handling#241bobcc4 wants to merge 26 commits into
bobcc4 wants to merge 26 commits into
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:
📝 WalkthroughWalkthroughAdds network playlist auto-checking with configurable intervals, update badges ("!"), and sidebar UI controls. Reworks URL resolution fallback to support silent cross-source matching and return structured switched-source results containing original and alternative song info. Updates ChangesNetwork auto-check, URL fallback, download enrichment & lyric cache overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
public/music/app.js (2)
5216-5232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the auto-check interval before persisting it.
The invalid value is assigned to
settings, saved tolocalStorage, and pushed to the server before the null-check runs. That leaves a broken persisted config which silently disables auto-check on later loads even though the UI showed a validation error.Suggested fix
async function updateSetting(key, value) { + if (key === 'networkListAutoCheckInterval') { + const intervalMs = parseNetworkListAutoCheckInterval(value); + if (intervalMs === null) { + showError('无效的自动检测间隔，请使用 30m / 6h / 1d 等格式'); + syncSettingsUI(key, settings[key]); + return; + } + } + settings[key] = value; window.settings = settings; // 确保全局引用同步 try { localStorage.setItem('lx_settings', JSON.stringify(settings)); console.log(`[Settings] ${key} 已更新为:`, value); @@ // 实时同步 UI 并应用效果 syncSettingsUI(key, value); if (key === 'networkListAutoCheckInterval' || key === 'autoUpdateNetworkList') { - const intervalMs = parseNetworkListAutoCheckInterval(settings.networkListAutoCheckInterval); - if (key === 'networkListAutoCheckInterval' && intervalMs === null) { - showError('无效的自动检测间隔，请使用 30m / 6h / 1d 等格式'); - } setupNetworkListAutoCheck(); }🤖 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/app.js` around lines 5216 - 5232, The validation of the auto-check interval happens after the value has already been assigned to settings, persisted to localStorage, and synced to the UI. Move the validation logic before the settings assignment so that when parseNetworkListAutoCheckInterval returns null for an invalid networkListAutoCheckInterval value, the error is shown and the function returns early without executing settings[key] = value, localStorage.setItem, or syncSettingsUI. Only proceed with persisting and syncing the value if the validation passes.
8786-8790:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape remote playlist names before rendering them in the confirmation dialog.
list.namecomes from collected network playlist metadata, andshowSelect()rendersmessageviainnerHTML. A crafted playlist title can inject markup/event handlers here; the same raw-name path is also used in the new update toasts.🤖 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/app.js` around lines 8786 - 8790, The playlist name from `list.name` is being directly interpolated into the message string passed to `showSelect()`, which renders via `innerHTML`, creating an XSS vulnerability where a crafted playlist name could inject malicious markup or event handlers. Escape the `list.name` value using HTML entity encoding (such as replacing special characters like <, >, &, quotes) before inserting it into the confirmation dialog message template. Additionally, apply the same HTML escaping to `list.name` in any related update toast messages to prevent injection at those locations as well.
🤖 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 5065-5068: The network-list auto-check timer is initialized in
loadSettings() via setupNetworkListAutoCheck(), but when
fetchSettingsFromServer() later overwrites the settings object with server
values, it only calls syncSettingsUI() without re-arming the timer. This causes
auto-check settings restored from the server (autoUpdateNetworkList and
networkListAutoCheckInterval) to not take effect until a full page reload. After
syncSettingsUI() is called in the fetchSettingsFromServer() flow, you must also
call setupNetworkListAutoCheck() again to re-arm the timer with the new
server-restored settings values.
- Around line 241-249: The success message showing "所有网络歌单均为最新状态" is displayed
whenever changedLists is empty, regardless of whether any playlists failed to
check. Add a condition to the else block that checks failedLists.length === 0
before showing this success message. This ensures that the "all playlists are up
to date" message is only shown when there are genuinely no changes AND no
failures, preventing false success reports when all playlist checks have failed.
- Around line 2840-2852: The code returns the matchedSong with switchedSource
flag, but the playSong() function that receives this result still uses the
original song object for updating player state (currentPlayingSong, lyrics
lookup, history, and source badges). When a fallback source switch occurs, check
for the switchedSource flag in the returned result and update playSong() to use
the songInfo (matchedSong) from the fallback result instead of the original song
object when setting currentPlayingSong, performing lyric lookups, recording
history, and updating source badges. This ensures all player state metadata
stays synchronized with the actual source being played.
In `@public/music/js/batch_pagination.js`:
- Around line 95-96: The deselectAll() function currently unconditionally sets
window.batchMode = false, causing the "清空" (clear selection) button to
unexpectedly exit batch mode when it should only clear selections. Refactor by
renaming deselectAll() to clearSelection() and removing the window.batchMode =
false statement from this function, so it only deselects items without changing
batch mode state. Then, at the specific post-operation cleanup locations where
batch mode exit is actually intended (post-delete, post-download,
post-batch-collect-sync), explicitly call toggleBatchMode() or set
window.batchMode = false after calling clearSelection() to achieve the intended
exit behavior, maintaining the semantic separation between the "清空" and "退出"
buttons.
In `@src/server/fileCache.ts`:
- Around line 1452-1455: The parseLyrics function is being called
unconditionally on line 1452, even when lyric caching is not enabled. Move the
parseLyrics call inside the if (shouldCacheLyric) block so that lyrics are only
parsed when lyric-file caching is actually enabled. This prevents unnecessary
parsing operations in embed-only mode and ensures parse failures don't interfere
with other actions within the shared try block.
- Around line 1274-1302: In the fast path where isOnlyDownload, result.folder is
'cache', and result.path exists, the lyric file copying logic (the block
starting with the sourceLyricPath variable and the fs.copyFileSync call) is
being executed unconditionally without checking the shouldCacheLyric
configuration flag. Wrap the entire lyric file copying block (from the
sourceLyricPath declaration through the fs.copyFileSync and lyricFilename
assignment) with a guard condition that only executes this block when
shouldCacheLyric is true, ensuring that when shouldCacheLyric=false, no .lrc
file is copied even if the source exists in the cache folder.
- Around line 1337-1341: The fail() function updates cacheProgress before
checking if the promise is already settled, causing a race condition where
fail() can be called after abort cleanup (around line 1350) and reinsert a false
"error" state into the progress cache via the req.on('error') handler at line
1480. Add a guard check at the beginning of the fail() function to verify the
promise has not been settled yet before updating cacheProgress, preventing stale
error state from being reinserted after the abort operation has deleted the
progress.
In `@src/server/server.ts`:
- Around line 2338-2341: Move the entire if (namingPattern) block that calls
fileCache.setNamingPattern(namingPattern) and updates
global.lx.config['cache.namingPattern'] to execute AFTER the non-public auth
gate check at line 2348, not before. This ensures that unauthenticated requests
(which will result in 401 errors) cannot mutate the server-global cache naming
state. Apply the same ordering principle to the code block at lines 2348-2356 to
ensure all mutations of global cache state occur only after successful
authentication.
---
Outside diff comments:
In `@public/music/app.js`:
- Around line 5216-5232: The validation of the auto-check interval happens after
the value has already been assigned to settings, persisted to localStorage, and
synced to the UI. Move the validation logic before the settings assignment so
that when parseNetworkListAutoCheckInterval returns null for an invalid
networkListAutoCheckInterval value, the error is shown and the function returns
early without executing settings[key] = value, localStorage.setItem, or
syncSettingsUI. Only proceed with persisting and syncing the value if the
validation passes.
- Around line 8786-8790: The playlist name from `list.name` is being directly
interpolated into the message string passed to `showSelect()`, which renders via
`innerHTML`, creating an XSS vulnerability where a crafted playlist name could
inject malicious markup or event handlers. Escape the `list.name` value using
HTML entity encoding (such as replacing special characters like <, >, &, quotes)
before inserting it into the confirmation dialog message template. Additionally,
apply the same HTML escaping to `list.name` in any related update toast messages
to prevent injection at those locations as well.
🪄 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: 1ce94120-e16d-4919-8fe4-2590c1e92f37
📒 Files selected for processing (9)
.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.jssrc/server/fileCache.tssrc/server/server.ts
bobcc4
changed the title
Author
|
已按 CodeRabbit review 继续调整：
本地已通过 docker build 验证。 |
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)
public/music/app.js (1)
196-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent overlapping auto-check runs.
Line 196 starts a new
checkNetworkListUpdates()on every tick without waiting for the previous scan to finish. If one full pass takes longer than the configured interval, the next tick duplicates the network work and races updates towindow.networkListUpdateMap/ toast state.Suggested fix
let networkListAutoCheckTimer = null; +let networkListAutoCheckInFlight = false; ... - networkListAutoCheckTimer = setInterval(() => { - checkNetworkListUpdates().catch(err => console.error('[AutoCheck] 网络歌单检测失败:', err)); + networkListAutoCheckTimer = setInterval(async () => { + if (networkListAutoCheckInFlight) return; + networkListAutoCheckInFlight = true; + try { + await checkNetworkListUpdates(); + } catch (err) { + console.error('[AutoCheck] 网络歌单检测失败:', err); + } finally { + networkListAutoCheckInFlight = false; + } }, intervalMs);🤖 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/app.js` around lines 196 - 198, The setInterval callback at line 196-198 starts a new checkNetworkListUpdates() call on every tick without waiting for the previous execution to complete, causing overlapping runs that can race updates to window.networkListUpdateMap and toast state. Add a guard flag or mechanism to track whether checkNetworkListUpdates() is currently executing, and only initiate a new check when the previous one has fully completed. One approach is to set a flag to true before calling checkNetworkListUpdates() and reset it to false in the .catch() handler, checking the flag before starting a new scan in the setInterval callback.public/music/js/single_song_ops.js (1)
121-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate lyric-cache failures instead of resolving silently.
This helper currently resolves on non-2xx responses from both
/api/music/lyricand/api/music/cache/lyric, so callers thatawait requestServerLyricCache(...)treat failed sync attempts as success. That makes the manual retry flow inpublic/music/app.js:5900-5922and the batch补词 flow inpublic/music/js/local_music.js:819-846report success even when the server rejected the request. If you switch this helper to reject on failure, the.then(...)chain inpublic/music/js/download_manager.js:137-147also needs a.catch(...).💡 Suggested fix
- const lRes = await fetch(lyricUrl); - if (!lRes.ok) return; + const lRes = await fetch(lyricUrl); + if (!lRes.ok) { + throw new Error(`歌词获取失败: ${lRes.status}`); + } const lyricInfo = await lRes.json(); - if (!lyricInfo || (!lyricInfo.lyric && !lyricInfo.lrc)) return; + if (!lyricInfo || (!lyricInfo.lyric && !lyricInfo.lrc)) { + throw new Error('未获取到可缓存的歌词内容'); + } @@ - await fetch(cacheUrl, { + const cacheRes = await fetch(cacheUrl, { method: 'POST', headers, body: JSON.stringify({ songInfo: songInfoForCache, lyricsObj: lyricInfo, enableOnlyDownloadMode }) }); + if (!cacheRes.ok) { + throw new Error(`歌词缓存失败: ${cacheRes.status}`); + } console.log(`[Lyric] 歌曲下载触发的歌词缓存同步成功: ${song.name} (仅下载模式: ${enableOnlyDownloadMode})`); } catch (e) { console.warn(`[Lyric] 自动同步歌词缓存失败: ${song.name}`, e); + throw e; }🤖 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/single_song_ops.js` around lines 121 - 159, The requestServerLyricCache function in single_song_ops.js currently silently returns on fetch failures (both from the lyric endpoint and the cache endpoint), making callers treat failures as success. Replace the silent return statements with throwing or rejecting the promise on non-2xx responses from both the lyric fetch and the cache POST request. Additionally, add a .catch() handler to the promise chain in download_manager.js at lines 137-147 where requestServerLyricCache is called, since the function will now propagate errors instead of resolving silently.
🤖 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 5237-5244: The current validation accepts `0` / `off` values from
`parseNetworkListAutoCheckInterval()` which creates an inconsistent state where
`autoUpdateNetworkList` remains true but `setupNetworkListAutoCheck()` silently
disables the timer. This same inconsistent state is then persisted and restored
from server settings. To fix this, add sanitization logic before re-arming the
timer by checking if `autoUpdateNetworkList` is true AND the parsed interval
from `parseNetworkListAutoCheckInterval()` is not a finite positive number; if
so, reset `networkListAutoCheckInterval` to
`DEFAULT_SETTINGS.networkListAutoCheckInterval` before calling
`setupNetworkListAutoCheck()` to ensure the enabled state matches the actual
timer setup.
- Around line 3832-3840: The fetchLyric() function is called multiple times
(once for the original song in playSong() and again at line 3839 for the
switched source song), but without request tracking, responses can complete out
of order and cause stale lyrics to overwrite fresh ones. Add a lyric request ID
mechanism inside fetchLyric() that assigns a unique request identifier when
called, then before mutating the global lyric state (currentRawLrc and
currentLyricLines), verify the response belongs to the most recent request and
drop any stale completions to prevent older fetch results from overwriting newer
ones.
---
Outside diff comments:
In `@public/music/app.js`:
- Around line 196-198: The setInterval callback at line 196-198 starts a new
checkNetworkListUpdates() call on every tick without waiting for the previous
execution to complete, causing overlapping runs that can race updates to
window.networkListUpdateMap and toast state. Add a guard flag or mechanism to
track whether checkNetworkListUpdates() is currently executing, and only
initiate a new check when the previous one has fully completed. One approach is
to set a flag to true before calling checkNetworkListUpdates() and reset it to
false in the .catch() handler, checking the flag before starting a new scan in
the setInterval callback.
In `@public/music/js/single_song_ops.js`:
- Around line 121-159: The requestServerLyricCache function in
single_song_ops.js currently silently returns on fetch failures (both from the
lyric endpoint and the cache endpoint), making callers treat failures as
success. Replace the silent return statements with throwing or rejecting the
promise on non-2xx responses from both the lyric fetch and the cache POST
request. Additionally, add a .catch() handler to the promise chain in
download_manager.js at lines 137-147 where requestServerLyricCache is called,
since the function will now propagate errors instead of resolving silently.
🪄 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: 1e6ba3c0-50a6-465b-918b-27efcd61e13d
📒 Files selected for processing (5)
public/music/app.jspublic/music/js/batch_pagination.jspublic/music/js/single_song_ops.jssrc/server/fileCache.tssrc/server/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/server.ts
- src/server/fileCache.ts
Comment on lines
+3832
to
+3840
| const playbackSong = (urlResult.switchedSource && urlResult.songInfo) ? urlResult.songInfo : song; | ||
| if (playbackSong !== song) { | ||
| currentPlayingSong = playbackSong; | ||
| window.currentPlayingSong = playbackSong; | ||
| if (currentRecoveryState) currentRecoveryState.currentSong = playbackSong; | ||
| updatePlayerInfo(playbackSong); | ||
| updateMediaSessionMetadata(playbackSong); | ||
| fetchLyric(playbackSong, currentQuality); | ||
| } |
There was a problem hiding this comment.
The switched-source lyric refetch still races the original request.
Line 3839 starts a second fetchLyric() for the matched source, but the first lyric request for the original song was already kicked off earlier in playSong(). Because fetchLyric() writes global lyric state without a request token, whichever response finishes last wins, so the stale original-source lyrics can still overwrite the switched-source lyrics.
Suggested fix
- fetchLyric(song);
+ // Wait until the final playback source is known before fetching lyrics. const playbackSong = (urlResult.switchedSource && urlResult.songInfo) ? urlResult.songInfo : song;
if (playbackSong !== song) {
currentPlayingSong = playbackSong;
window.currentPlayingSong = playbackSong;
if (currentRecoveryState) currentRecoveryState.currentSong = playbackSong;
updatePlayerInfo(playbackSong);
updateMediaSessionMetadata(playbackSong);
- fetchLyric(playbackSong, currentQuality);
}
+ fetchLyric(playbackSong, currentQuality);If you need to keep the early fetch for latency, add a lyric request id inside fetchLyric() and drop stale completions before mutating currentRawLrc / currentLyricLines.
🤖 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/app.js` around lines 3832 - 3840, The fetchLyric() function is
called multiple times (once for the original song in playSong() and again at
line 3839 for the switched source song), but without request tracking, responses
can complete out of order and cause stale lyrics to overwrite fresh ones. Add a
lyric request ID mechanism inside fetchLyric() that assigns a unique request
identifier when called, then before mutating the global lyric state
(currentRawLrc and currentLyricLines), verify the response belongs to the most
recent request and drop any stale completions to prevent older fetch results
from overwriting newer ones.
Comment on lines
+5237
to
+5244
| if (key === 'networkListAutoCheckInterval') { | ||
| const intervalMs = parseNetworkListAutoCheckInterval(value); | ||
| if (intervalMs === null) { | ||
| showError('无效的自动检测间隔，请使用 30m / 6h / 1d 等格式'); | ||
| syncSettingsUI(key, settings[key]); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t persist an enabled-but-disabled auto-check state.
Line 5238 accepts off / 0 because parseNetworkListAutoCheckInterval() returns 0, so this path can save autoUpdateNetworkList = true while setupNetworkListAutoCheck() silently tears the timer down. The same inconsistent state is then restored again at Lines 7786-7787 from server settings.
Suggested fix
if (key === 'networkListAutoCheckInterval') {
const intervalMs = parseNetworkListAutoCheckInterval(value);
- if (intervalMs === null) {
+ if (intervalMs === null || intervalMs <= 0) {
showError('无效的自动检测间隔，请使用 30m / 6h / 1d 等格式');
syncSettingsUI(key, settings[key]);
return;
}
}// When restoring from server/local storage, sanitize before re-arming the timer.
const restoredInterval = parseNetworkListAutoCheckInterval(settings.networkListAutoCheckInterval);
if (settings.autoUpdateNetworkList && (!Number.isFinite(restoredInterval) || restoredInterval <= 0)) {
settings.networkListAutoCheckInterval = DEFAULT_SETTINGS.networkListAutoCheckInterval;
}
setupNetworkListAutoCheck();Also applies to: 7786-7787
🤖 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/app.js` around lines 5237 - 5244, The current validation accepts
`0` / `off` values from `parseNetworkListAutoCheckInterval()` which creates an
inconsistent state where `autoUpdateNetworkList` remains true but
`setupNetworkListAutoCheck()` silently disables the timer. This same
inconsistent state is then persisted and restored from server settings. To fix
this, add sanitization logic before re-arming the timer by checking if
`autoUpdateNetworkList` is true AND the parsed interval from
`parseNetworkListAutoCheckInterval()` is not a finite positive number; if so,
reset `networkListAutoCheckInterval` to
`DEFAULT_SETTINGS.networkListAutoCheckInterval` before calling
`setupNetworkListAutoCheck()` to ensure the enabled state matches the actual
timer setup.
Author
|
Update after local Docker testing:
Verified with the locally tested Docker image |
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 (2)
public/music/app.js (2)
8945-8946:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRefreshing a remote playlist still leaves a stored XSS path.
These lines copy
data.info.namefrom the remote provider intolist.name, butrenderMyLists()later interpolates that value intoinnerHTMLviacreateItem()/createMarqueeHtml(). Escaping the confirm/toast strings does not protect the sidebar render path, so a malicious remote playlist title can still execute script after refresh. Render playlist names withtextContent/DOM nodes or escape them before any HTML interpolation.🤖 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/app.js` around lines 8945 - 8946, The `list.name` is populated with unsanitized data from the remote provider via `data.info.name` and later rendered as HTML through `renderMyLists()` which uses `createItem()` and `createMarqueeHtml()` functions that interpolate the value into `innerHTML`. This creates a stored XSS vulnerability where malicious script in a remote playlist title can execute after refresh. Either escape the `data.info.name` value before assigning it to `list.name` using HTML entity encoding, or modify the rendering functions `createItem()` and `createMarqueeHtml()` to use `textContent` or DOM node creation instead of `innerHTML` interpolation when rendering playlist names.
184-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the auto-check loop against overlapping runs.
Line 196 schedules
checkNetworkListUpdates()withsetInterval(), but the check itself awaits a sequence of network fetches. If one run takes longer than the configured interval, the next tick starts a second pass against the samewindow.networkListUpdateMapand sidebar state, which can duplicate requests/toasts and leave badge state racing between two completions. A simple single-flight guard (or switching to a self-reschedulingsetTimeoutafter each run finishes) avoids that.Also applies to: 202-265
🤖 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/app.js` around lines 184 - 198, In the setupNetworkListAutoCheck() function, the setInterval on line 196 schedules checkNetworkListUpdates() without protection against overlapping executions. If one checkNetworkListUpdates() call takes longer than the configured interval, the next scheduled tick will start a concurrent execution, causing race conditions on window.networkListUpdateMap and sidebar state. Implement a guard using either a single-flight pattern (a boolean flag preventing concurrent runs) or replace setInterval with a self-rescheduling setTimeout that reschedules after each checkNetworkListUpdates() completes. Also review the code in the 202-265 range to ensure any related network update logic or state management is compatible with your chosen concurrency guard approach.
♻️ Duplicate comments (2)
public/music/app.js (2)
3818-3819:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe switched-source lyric race is still reachable.
playSong()still kicks offfetchLyric(song)before the final playback source is known, then starts a second lyric request after fallback switch.fetchLyric()mutates global lyric state without a request token, so the original-source response can still finish last and overwrite the matched-source lyrics. This needs a request-id check insidefetchLyric()or the initial fetch should be deferred untilplaybackSongis final.Also applies to: 3933-3941
🤖 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/app.js` around lines 3818 - 3819, There is a race condition in the playSong() function where fetchLyric(song) is called at line 3818-3819 before the final playback source is determined, and then called again at lines 3933-3941 after a fallback switch. Since fetchLyric() mutates global lyric state without any request tracking mechanism, an earlier response from the wrong source can still arrive and overwrite the correct lyrics. Fix this by either: (1) adding a request-id or token check inside fetchLyric() to ignore stale responses and only apply lyrics from the most recent request, or (2) remove the initial fetchLyric(song) call at line 3818-3819 and rely only on the fetched lyrics after playbackSong is final (after the fallback logic at lines 3933-3941).
5338-5345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t keep
autoUpdateNetworkList = truewith a nonpositive interval.Line 5339 still treats
off/0as valid becauseparseNetworkListAutoCheckInterval()returns0, notnull, and Line 7890 later re-arms the timer without sanitizing that restored state. The result is a persisted “enabled” toggle with no timer actually running. Reject<= 0on save and normalize server-restored values before callingsetupNetworkListAutoCheck().Suggested fix
if (key === 'networkListAutoCheckInterval') { const intervalMs = parseNetworkListAutoCheckInterval(value); - if (intervalMs === null) { + if (intervalMs === null || intervalMs <= 0) { showError('无效的自动检测间隔，请使用 30m / 6h / 1d 等格式'); syncSettingsUI(key, settings[key]); return; } }if (res.ok) { const serverSettings = await res.json(); console.log('[Settings] 从服务器加载设置成功:', serverSettings); // Merge settings settings = { ...settings, ...serverSettings }; + const restoredInterval = parseNetworkListAutoCheckInterval(settings.networkListAutoCheckInterval); + if (settings.autoUpdateNetworkList && (!Number.isFinite(restoredInterval) || restoredInterval <= 0)) { + settings.networkListAutoCheckInterval = DEFAULT_SETTINGS.networkListAutoCheckInterval; + } // Save to local localStorage.setItem('lx_settings', JSON.stringify(settings)); // Update UI syncSettingsUI(); setupNetworkListAutoCheck();Also applies to: 7885-7890
🤖 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/app.js` around lines 5338 - 5345, The validation logic for networkListAutoCheckInterval needs to reject zero and negative values in addition to null, and server-restored values must be sanitized before being passed to setupNetworkListAutoCheck(). At the first site around line 5338, change the condition from checking if intervalMs === null to checking if intervalMs <= 0, which will properly reject zero values that parseNetworkListAutoCheckInterval() can return. At the second site around line 7885-7890 where setupNetworkListAutoCheck() is called with restored settings, add validation to ensure the restored autoCheckInterval value is positive before calling setupNetworkListAutoCheck(), preventing a persisted enabled toggle from attempting to run a timer with an invalid interval.
🧹 Nitpick comments (1)
public/music/js/download_manager.js (1)
868-876: 💤 Low valueConsider extracting duplicate server POST payload construction.
Lines 788 and 894 both construct identical POST bodies for
/api/music/cache/download. Extracting this to a helper method would improve maintainability and ensure consistency if the payload structure changes.♻️ Suggested refactor
+ buildServerDownloadPayload(song, quality, rawUrl) { + return { + songInfo: this.getSongInfoForServer(song), + url: rawUrl, + quality, + enableOnlyDownloadMode: window.settings?.enableOnlyDownloadMode || false, + namingPattern: window.settings?.serverCacheNamingPattern || 'simple', + cacheLyric: window.settings?.enableServerLyricCache !== false, + embedLyric: !!(window.settings?.embedLyricToFile ?? true) + }; + }Then use in
startServerDownload,resumeTask, andretryAllFailed:body: JSON.stringify(this.buildServerDownloadPayload(resolvedSong, quality, rawUrl))Also applies to: 891-895
🤖 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 - 876, The POST payload for the `/api/music/cache/download` endpoint is being constructed identically at multiple locations (around lines 788 and 894-895). Extract this duplicate payload construction logic into a new helper method called buildServerDownloadPayload that accepts parameters like resolvedSong, quality, and rawUrl, and returns the constructed payload object. Then replace all instances of the duplicate inline payload construction with calls to this new helper method to ensure consistency and improve maintainability.
🤖 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/app.js`:
- Around line 8945-8946: The `list.name` is populated with unsanitized data from
the remote provider via `data.info.name` and later rendered as HTML through
`renderMyLists()` which uses `createItem()` and `createMarqueeHtml()` functions
that interpolate the value into `innerHTML`. This creates a stored XSS
vulnerability where malicious script in a remote playlist title can execute
after refresh. Either escape the `data.info.name` value before assigning it to
`list.name` using HTML entity encoding, or modify the rendering functions
`createItem()` and `createMarqueeHtml()` to use `textContent` or DOM node
creation instead of `innerHTML` interpolation when rendering playlist names.
- Around line 184-198: In the setupNetworkListAutoCheck() function, the
setInterval on line 196 schedules checkNetworkListUpdates() without protection
against overlapping executions. If one checkNetworkListUpdates() call takes
longer than the configured interval, the next scheduled tick will start a
concurrent execution, causing race conditions on window.networkListUpdateMap and
sidebar state. Implement a guard using either a single-flight pattern (a boolean
flag preventing concurrent runs) or replace setInterval with a self-rescheduling
setTimeout that reschedules after each checkNetworkListUpdates() completes. Also
review the code in the 202-265 range to ensure any related network update logic
or state management is compatible with your chosen concurrency guard approach.
---
Duplicate comments:
In `@public/music/app.js`:
- Around line 3818-3819: There is a race condition in the playSong() function
where fetchLyric(song) is called at line 3818-3819 before the final playback
source is determined, and then called again at lines 3933-3941 after a fallback
switch. Since fetchLyric() mutates global lyric state without any request
tracking mechanism, an earlier response from the wrong source can still arrive
and overwrite the correct lyrics. Fix this by either: (1) adding a request-id or
token check inside fetchLyric() to ignore stale responses and only apply lyrics
from the most recent request, or (2) remove the initial fetchLyric(song) call at
line 3818-3819 and rely only on the fetched lyrics after playbackSong is final
(after the fallback logic at lines 3933-3941).
- Around line 5338-5345: The validation logic for networkListAutoCheckInterval
needs to reject zero and negative values in addition to null, and
server-restored values must be sanitized before being passed to
setupNetworkListAutoCheck(). At the first site around line 5338, change the
condition from checking if intervalMs === null to checking if intervalMs <= 0,
which will properly reject zero values that parseNetworkListAutoCheckInterval()
can return. At the second site around line 7885-7890 where
setupNetworkListAutoCheck() is called with restored settings, add validation to
ensure the restored autoCheckInterval value is positive before calling
setupNetworkListAutoCheck(), preventing a persisted enabled toggle from
attempting to run a timer with an invalid interval.
---
Nitpick comments:
In `@public/music/js/download_manager.js`:
- Around line 868-876: The POST payload for the `/api/music/cache/download`
endpoint is being constructed identically at multiple locations (around lines
788 and 894-895). Extract this duplicate payload construction logic into a new
helper method called buildServerDownloadPayload that accepts parameters like
resolvedSong, quality, and rawUrl, and returns the constructed payload object.
Then replace all instances of the duplicate inline payload construction with
calls to this new helper method to ensure consistency and improve
maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fea9bc59-6dab-4f1d-a8f7-b45ccd520038
📒 Files selected for processing (2)
public/music/app.jspublic/music/js/download_manager.js
Author
|
Update after additional local testing:
Verified locally with Docker image |
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 (7)
public/music/app.js (3)
5194-5204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
window.settingssynchronized when restoring settings.Both restore paths replace the local
settingsobject, leaving consumers ofwindow.settingson the previous object. Server-restored values like download mode, naming pattern, cache toggles, and auto-check settings can diverge across modules.Suggested fix
- settings = { ...settings, ...loaded }; + Object.assign(settings, loaded); + window.settings = settings; console.log('[Settings] 加载设置成功:', settings);- settings = { ...settings, ...serverSettings }; + Object.assign(settings, serverSettings); + window.settings = settings; // Save to local localStorage.setItem('lx_settings', JSON.stringify(settings));Also applies to: 7899-7905
🤖 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/app.js` around lines 5194 - 5204, When restoring settings from a saved local copy using JSON.parse(saved) and merging with the local settings object using the spread operator, the code updates the local settings variable but does not synchronize window.settings, causing other modules referencing window.settings to have stale values. After the line where settings is reassigned with the merged object (settings = { ...settings, ...loaded }), explicitly update window.settings to point to the new settings object to ensure all consumers see the updated values. This synchronization is needed at both the primary location (around line 5194-5204 where settings are loaded from localStorage) and the secondary location (around line 7899-7905 where another restore path exists), ensuring that download mode, naming pattern, cache toggles, and auto-check settings are consistently visible across all modules.
2610-2636:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape marquee/list text on both render passes.
Remote playlist/song metadata still reaches
innerHTMLthroughcreateMarqueeHtml()and the sidebar list name path.applyMarqueeChecks()also reinjects decodeddata-text, so escape again before building the marquee HTML.Suggested fix
function createMarqueeHtml(text, className = '') { + const safeText = escapeHtmlText(text); // Return a container marked for dynamic checking // different screens are different, so we check overflow after render // Added min-w-0 to prevent flex item from expanding beyond parent - return `<div class="truncate dynamic-marquee min-w-0 ${className}" data-text="${text.replace(/"/g, '"')}">${text}</div>`; + return `<div class="truncate dynamic-marquee min-w-0 ${className}" data-text="${safeText.replace(/"/g, '"')}">${safeText}</div>`; }- const text = el.getAttribute('data-text') || el.innerText; + const text = el.getAttribute('data-text') || el.innerText; + const safeText = escapeHtmlText(text); const gap = '<span class="mx-8"></span>'; // 增加间距 @@ - <span>${text}</span>${gap}<span>${text}</span>${gap} + <span>${safeText}</span>${gap}<span>${safeText}</span>${gap}- const nameHtml = name.length > 8 ? createMarqueeHtml(name, 'flex-1') : `<span class="ml-2 flex-1 truncate">${name}</span>`; + const rawName = String(name ?? ''); + const safeName = escapeHtmlText(rawName); + const nameHtml = rawName.length > 8 ? createMarqueeHtml(rawName, 'flex-1') : `<span class="ml-2 flex-1 truncate">${safeName}</span>`; @@ - ${name.length > 8 ? `<div class="ml-2 flex-1 overflow-hidden">${nameHtml}</div>` : nameHtml} + ${rawName.length > 8 ? `<div class="ml-2 flex-1 overflow-hidden">${nameHtml}</div>` : nameHtml}Also applies to: 8475-8498
🤖 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/app.js` around lines 2610 - 2636, The functions createMarqueeHtml() and applyMarqueeChecks() are vulnerable to HTML injection because text is not properly escaped before being used in innerHTML operations. In createMarqueeHtml(), the current escaping only handles double quotes but not other HTML special characters. More critically, in applyMarqueeChecks(), the text retrieved from the data-text attribute is directly injected into the innerHTML template string without any escaping. Fix this by implementing a proper HTML escape function that handles all special characters (like &, <, >, quotes), then apply it consistently: escape the text parameter in createMarqueeHtml() before storing it in the data-text attribute, and escape the retrieved text again in applyMarqueeChecks() before inserting it into the innerHTML template where the span elements are created. This ensures protection against injection from remote playlist and song metadata at both render passes.
217-235:⚠️ Potential issue | 🟠 MajorLoop through all pages before overwriting playlist data in refresh operations.
The
/api/music/songList/detailendpoint uses pagination for some sources (e.g., Baidu: 10,000 songs per page). BothcheckNetworkListUpdates()(line 219) andhandleRefreshList()(line 8944) request only&page=1, which truncates playlists larger than the page limit. The update detection compares the full local list against a single page of remote data (causing false "stale" flags), and refresh overwrites the playlist with incomplete data—silently deleting songs.Verify the
totalfield returned in the API response and paginate through all pages before assigninglist.list = newList.Applies to: lines 217–235, 8944–8958
🤖 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/app.js` around lines 217 - 235, The code fetches only page 1 from the paginated API endpoint, which causes large playlists to be truncated. Fix this by checking the `total` field in the API response to determine the total number of items available, then loop through all necessary pages (calculating page count from the total and the response items per page) to fetch and concatenate all song data before comparing with the local list or assigning to list.list. This fix needs to be applied in both the fetch loop within checkNetworkListUpdates() (around line 219 where the API call is made) and in the similar pagination logic in handleRefreshList() (around line 8944).public/music/js/download_manager.js (1)
220-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove unreachable dead code after
returnstatement.Lines 223-235 are unreachable due to the
return;on line 222. This appears to be leftover code from beforecompleteServerTaskwas introduced to centralize completion logic.🗑️ Proposed fix to remove dead code
if (progressInfo.status === 'tagging' || progressInfo.status === 'finished' || progressInfo.status === 'exists') { this.completeServerTask(task, progressInfo.status === 'exists' ? 'exists' : 'finished'); return; - if (progressInfo.status === 'tagging') task.status = 'finished'; - task.progress = 100; - task.errorMsg = ''; - // 成功完成后触发歌词同步（补充） - if (this.shouldAutoSyncLyric(task)) { - window.requestServerLyricCache(task.song, task.quality).then(() => { - // 延时一下再检查，确保后端写入完成 - setTimeout(() => this.checkTaskLyric(task), 2000); - }); - } - this.saveTasks(); - // If it just finished, free up the slot - this.processQueue(); } 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 220 - 235, Remove the unreachable dead code that appears after the return statement in the download_manager.js file. In the condition block where progressInfo.status is checked against 'tagging', 'finished', or 'exists', there is a return statement that exits the function, making all subsequent code (task.status assignment, progress/error message updates, lyric sync logic, task saving, and queue processing) unreachable. Delete all code after the return statement through the end of that conditional block, keeping only the completeServerTask call and return statement since the centralized completion logic now handles what those removed lines were doing.src/server/server.ts (3)
1805-1809:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep restricted download settings from being dropped.
enableOnlyDownloadModeis missing from this allowlist, so restricted public saves silently discard the download-only setting; clampdownloadConcurrencyhere too so stale clients cannot persist6+again.Suggested fix
- const allowedKeys = ['enableServerCache', 'enableServerLyricCache', 'serverCacheLocation', 'serverCacheNamingPattern', 'downloadConcurrency'] + const allowedKeys = ['enableServerCache', 'enableServerLyricCache', 'serverCacheLocation', 'serverCacheNamingPattern', 'downloadConcurrency', 'enableOnlyDownloadMode'] allowedKeys.forEach(key => { if (settings[key] !== undefined) restrictedSettings[key] = settings[key] }) + if (restrictedSettings.downloadConcurrency !== undefined) { + const n = parseInt(String(restrictedSettings.downloadConcurrency), 10) + restrictedSettings.downloadConcurrency = Number.isFinite(n) ? Math.min(5, Math.max(1, n)) : 3 + }🤖 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 1805 - 1809, The allowedKeys array in the forEach loop that filters settings into restrictedSettings is missing the enableOnlyDownloadMode key, causing that setting to be silently discarded when restricted public saves are applied. Add enableOnlyDownloadMode to the allowedKeys array alongside the existing keys. Additionally, after the restrictedSettings object is populated, add logic to clamp the downloadConcurrency value to prevent stale clients from persisting values of 6 or higher.
2353-2356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate public
namingPatternmutations under public restrictions.Non-public auth now happens first, but a public request can still send
namingPatternand mutate process-global cache naming state whileuser.enablePublicRestrictionis enabled. Require admin auth for that public mutation or ignore per-download naming changes in restricted public mode.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: '权限不足：公共用户修改命名规则受限，请先验证管理员身份。' })) + 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 - 2356, The code in the namingPattern mutation block does not check if public restrictions are enabled before allowing the cache naming state to be mutated by public requests. Add a guard condition before the fileCache.setNamingPattern(namingPattern) call and the global.lx.config assignment to either verify that the user has admin authentication (non-public auth), or to skip the mutation when user.enablePublicRestriction is enabled. This ensures that process-global cache naming configuration cannot be modified by public requests when public restrictions are in place.
5558-5563:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore persisted cache settings before starting index sync.
syncCacheIndex()is fired before_open’s saved cache location/naming pattern are restored, so startup can build indexes against stale fileCache state. Move the settings-restore block above the background sync loop.Suggested ordering
- // Background sync cache index for active users - if (global.lx.config.users) { - for (const user of global.lx.config.users) { - void fileCache.syncCacheIndex(user.name) - } - } + // Restore persisted fileCache settings before any cache index scan. + try { + const openUserSpace = getUserSpace('_open') + const settingsPath = path.join(openUserSpace.dataManage.userDir, File.userSettingsJSON) + if (fs.existsSync(settingsPath)) { + const savedSettings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')) + if (savedSettings.serverCacheLocation) fileCache.setCacheLocation(savedSettings.serverCacheLocation) + if (savedSettings.serverCacheNamingPattern) fileCache.setNamingPattern(savedSettings.serverCacheNamingPattern) + } + } catch (err: any) { + console.warn('[Server] Failed to restore fileCache settings:', err.message) + } + + // Background sync cache index for active users + if (global.lx.config.users) { + for (const user of global.lx.config.users) { + void fileCache.syncCacheIndex(user.name) + } + }Also applies to: 5625-5639
🤖 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 5558 - 5563, The syncCacheIndex() call in the background sync loop is being invoked before the persisted cache settings from _open are restored, causing it to work with stale fileCache state. Move the block that restores saved cache location and naming pattern settings above the loop that iterates through global.lx.config.users and calls void fileCache.syncCacheIndex(user.name), ensuring that fileCache is properly initialized with persisted settings before any index synchronization begins. This same ordering issue also applies at the second location referenced in the comment.
🤖 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/modules/utils/musicSdk/tx/extendDetail.js`:
- Line 64: The issue is that the code uses logical OR (`||`) for fallback
assignment which treats `0` as falsy and incorrectly falls back to alternative
values when `item.total` is `0`. Replace the `||` operators with nullish
coalescing (`??`) in the total field assignment to preserve `0` as a valid value
and only fall back when the value is truly `null` or `undefined`, matching the
pattern already used in the frontend rendering for album data.
---
Outside diff comments:
In `@public/music/app.js`:
- Around line 5194-5204: When restoring settings from a saved local copy using
JSON.parse(saved) and merging with the local settings object using the spread
operator, the code updates the local settings variable but does not synchronize
window.settings, causing other modules referencing window.settings to have stale
values. After the line where settings is reassigned with the merged object
(settings = { ...settings, ...loaded }), explicitly update window.settings to
point to the new settings object to ensure all consumers see the updated values.
This synchronization is needed at both the primary location (around line
5194-5204 where settings are loaded from localStorage) and the secondary
location (around line 7899-7905 where another restore path exists), ensuring
that download mode, naming pattern, cache toggles, and auto-check settings are
consistently visible across all modules.
- Around line 2610-2636: The functions createMarqueeHtml() and
applyMarqueeChecks() are vulnerable to HTML injection because text is not
properly escaped before being used in innerHTML operations. In
createMarqueeHtml(), the current escaping only handles double quotes but not
other HTML special characters. More critically, in applyMarqueeChecks(), the
text retrieved from the data-text attribute is directly injected into the
innerHTML template string without any escaping. Fix this by implementing a
proper HTML escape function that handles all special characters (like &, <, >,
quotes), then apply it consistently: escape the text parameter in
createMarqueeHtml() before storing it in the data-text attribute, and escape the
retrieved text again in applyMarqueeChecks() before inserting it into the
innerHTML template where the span elements are created. This ensures protection
against injection from remote playlist and song metadata at both render passes.
- Around line 217-235: The code fetches only page 1 from the paginated API
endpoint, which causes large playlists to be truncated. Fix this by checking the
`total` field in the API response to determine the total number of items
available, then loop through all necessary pages (calculating page count from
the total and the response items per page) to fetch and concatenate all song
data before comparing with the local list or assigning to list.list. This fix
needs to be applied in both the fetch loop within checkNetworkListUpdates()
(around line 219 where the API call is made) and in the similar pagination logic
in handleRefreshList() (around line 8944).
In `@public/music/js/download_manager.js`:
- Around line 220-235: Remove the unreachable dead code that appears after the
return statement in the download_manager.js file. In the condition block where
progressInfo.status is checked against 'tagging', 'finished', or 'exists', there
is a return statement that exits the function, making all subsequent code
(task.status assignment, progress/error message updates, lyric sync logic, task
saving, and queue processing) unreachable. Delete all code after the return
statement through the end of that conditional block, keeping only the
completeServerTask call and return statement since the centralized completion
logic now handles what those removed lines were doing.
In `@src/server/server.ts`:
- Around line 1805-1809: The allowedKeys array in the forEach loop that filters
settings into restrictedSettings is missing the enableOnlyDownloadMode key,
causing that setting to be silently discarded when restricted public saves are
applied. Add enableOnlyDownloadMode to the allowedKeys array alongside the
existing keys. Additionally, after the restrictedSettings object is populated,
add logic to clamp the downloadConcurrency value to prevent stale clients from
persisting values of 6 or higher.
- Around line 2353-2356: The code in the namingPattern mutation block does not
check if public restrictions are enabled before allowing the cache naming state
to be mutated by public requests. Add a guard condition before the
fileCache.setNamingPattern(namingPattern) call and the global.lx.config
assignment to either verify that the user has admin authentication (non-public
auth), or to skip the mutation when user.enablePublicRestriction is enabled.
This ensures that process-global cache naming configuration cannot be modified
by public requests when public restrictions are in place.
- Around line 5558-5563: The syncCacheIndex() call in the background sync loop
is being invoked before the persisted cache settings from _open are restored,
causing it to work with stale fileCache state. Move the block that restores
saved cache location and naming pattern settings above the loop that iterates
through global.lx.config.users and calls void
fileCache.syncCacheIndex(user.name), ensuring that fileCache is properly
initialized with persisted settings before any index synchronization begins.
This same ordering issue also applies at the second location referenced in the
comment.
🪄 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: b831c9ff-ea50-4f3a-81d4-1fa296d77c77
📒 Files selected for processing (5)
public/music/app.jspublic/music/js/batch_pagination.jspublic/music/js/download_manager.jssrc/modules/utils/musicSdk/tx/extendDetail.jssrc/server/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- public/music/js/batch_pagination.js
| singer: item.info.author, | ||
| publishTime: item.info.publishTime, | ||
| total: item.total, | ||
| total: item.total || item.count || 0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for TX API album response handling to understand field structure
# Search for singer.getAlbumList implementation and TX album API response parsing
ast-grep --pattern 'getAlbumList($$$) {
$$$
}'
# Search for TX album API response examples or tests
rg -n -C5 'getAlbumList|getArtistAlbums' --type=js --type=ts -g '!node_modules'Repository: XCQ0607/lxserver
Length of output: 12138
🏁 Script executed:
# Read the file around line 64 to see full context
head -n 80 src/modules/utils/musicSdk/tx/extendDetail.js | tail -n 35Repository: XCQ0607/lxserver
Length of output: 1050
🏁 Script executed:
# Search for TX API documentation or response examples
rg -n "item\.total|item\.count" --type=js --type=ts -B2 -A2Repository: XCQ0607/lxserver
Length of output: 4513
🏁 Script executed:
# Check how frontend uses the total field
rg -n "album\.total|songCount" --type=js --type=ts -C3Repository: XCQ0607/lxserver
Length of output: 9061
Use nullish coalescing (??) instead of logical OR (||) to preserve 0 values.
The current implementation uses ||, which treats 0 as falsy and falls back to item.count. If TX API returns item.total = 0 for an empty album, this will incorrectly display the value from item.count instead of 0.
The frontend rendering already uses ?? for the exact same pattern: album.total ?? album.count ?? album.size ?? album.songCount ?? 0, which correctly distinguishes between 0 (a valid count) and null/undefined (missing data).
🔧 Proposed fix
- total: item.total || item.count || 0,
+ total: item.total ?? item.count ?? 0,📝 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.
Suggested change
| total: item.total || item.count || 0, | |
| total: item.total ?? item.count ?? 0, |
🤖 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/modules/utils/musicSdk/tx/extendDetail.js` at line 64, The issue is that
the code uses logical OR (`||`) for fallback assignment which treats `0` as
falsy and incorrectly falls back to alternative values when `item.total` is `0`.
Replace the `||` operators with nullish coalescing (`??`) in the total field
assignment to preserve `0` as a valid value and only fall back when the value is
truly `null` or `undefined`, matching the pattern already used in the frontend
rendering for album data.
Author
|
Updated this PR with an additional download manager fix for large server-side batches.\n\nChanges included:\n- keep server download progress polling aligned with the backend song key after URL resolution/source fallback\n- treat backend tagging as an active state instead of freeing the concurrency slot early\n- avoid skipping download-mode tasks when the file only exists in cache and still needs to land in the music folder\n- add virtualized rendering for the download drawer so 1000+ queued items can be scrolled without rendering every row at once\n- bump the music web service worker cache name to refresh the updated download manager script\n\nValidation:\n- tested the patched Docker image locally for the updated static files\n- ran JS syntax check with Node inside the Docker image: node --check public/music/js/download_manager.js\n- user reported the deployed image is currently downloading normally during batch testing |
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)
public/music/js/download_manager.js (1)
1215-1270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape task IDs in onclick handlers to prevent XSS from malformed song data.
task.idis used directly in onclick handlers without escaping. If a song'ssongmidoridcontains quotes or other special characters, it could break the JavaScript string or enable injection:// Current (vulnerable if task.id contains quotes): onclick="window.SystemDownloadManager.pauseTask('${task.id}')"Consider escaping or using data attributes with event delegation:
🛡️ Proposed fix using JSON encoding
-onclick="window.SystemDownloadManager.pauseTask('${task.id}')" +onclick="window.SystemDownloadManager.pauseTask(${JSON.stringify(task.id)})"Or better, use data attributes:
// In renderTaskHtml: `<button data-task-id="${this.escapeHtml(task.id)}" class="pause-btn ...">...` // In constructor, add event delegation: this.listContainer.addEventListener('click', (e) => { const btn = e.target.closest('[data-task-id]'); if (btn?.classList.contains('pause-btn')) { this.pauseTask(btn.dataset.taskId); } });🤖 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 1215 - 1270, Replace all inline onclick handlers in the button elements with data attributes to prevent XSS vulnerabilities from task IDs containing special characters. For each button currently using onclick handlers (pauseTask, resumeTask, deleteTask), add a data-task-id attribute with the escaped task.id value using this.escapeHtml(), remove the onclick attribute, and add a CSS class identifier like pause-btn, resume-btn, or delete-btn. Then implement event delegation in the constructor by adding a click listener to the taskListContainer that checks for these classes using e.target.closest(), retrieves the task ID from dataset.taskId, and calls the appropriate method (pauseTask, resumeTask, or deleteTask) instead of using inline onclick handlers.
🧹 Nitpick comments (2)
public/music/js/download_manager.js (2)
1088-1109: 💤 Low valueConsider saving the most recently added/active tasks instead of oldest.
slice(0, maxSavedTasks)keeps the first 200 tasks. For large batches where tasks are appended, newer waiting tasks at the end of the array won't be persisted. Consider prioritizing incomplete tasks:const tasksToSave = [...this.tasks] .sort((a, b) => { // Prioritize incomplete over completed const aComplete = a.status === 'finished' || a.status === 'exists'; const bComplete = b.status === 'finished' || b.status === 'exists'; if (aComplete !== bComplete) return aComplete ? 1 : -1; return 0; }) .slice(0, maxSavedTasks);🤖 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 1088 - 1109, In the saveTasks() method, the current implementation using slice(0, maxSavedTasks) preserves only the first 200 tasks in the array, which means newer incomplete tasks appended to the end may not be persisted. Replace this slice operation with a sort-then-slice approach that prioritizes incomplete tasks (those not in finished or exists status) before complete tasks, then slice the first 200 from the sorted result. This ensures newer active tasks are preserved in storage instead of older completed ones.
177-179: 💤 Low valueConsider simplifying the task filter condition for readability.
The current filter at line 178 is logically correct but complex. The condition could be clearer:
-const tasksToPoll = this.tasks.filter(t => (t.isServer || (t.status === 'downloading' && !t.isServer)) && (t.status === 'waiting' || t.status === 'downloading' || t.status === 'tagging')); +const tasksToPoll = this.tasks.filter(t => { + const isActiveServerTask = t.isServer && ['waiting', 'downloading', 'tagging'].includes(t.status); + const isActiveLocalTask = !t.isServer && t.status === 'downloading'; + return isActiveServerTask || isActiveLocalTask; +});🤖 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 177 - 179, Simplify the filter condition in the tasksToPoll constant assignment at line 178. The current filter logic combining the server task check and status validation is difficult to read due to nested parentheses and multiple conditions. Break down the condition into more readable parts by either using intermediate variables or a helper function that evaluates the task eligibility criteria, separating the server/non-server task logic from the status validation logic. Ensure the refactored condition maintains the same filtering behavior: including server tasks or non-server downloading tasks, while also checking that the status is one of the allowed values (waiting, downloading, or tagging).
🤖 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 1215-1270: Replace all inline onclick handlers in the button
elements with data attributes to prevent XSS vulnerabilities from task IDs
containing special characters. For each button currently using onclick handlers
(pauseTask, resumeTask, deleteTask), add a data-task-id attribute with the
escaped task.id value using this.escapeHtml(), remove the onclick attribute, and
add a CSS class identifier like pause-btn, resume-btn, or delete-btn. Then
implement event delegation in the constructor by adding a click listener to the
taskListContainer that checks for these classes using e.target.closest(),
retrieves the task ID from dataset.taskId, and calls the appropriate method
(pauseTask, resumeTask, or deleteTask) instead of using inline onclick handlers.
---
Nitpick comments:
In `@public/music/js/download_manager.js`:
- Around line 1088-1109: In the saveTasks() method, the current implementation
using slice(0, maxSavedTasks) preserves only the first 200 tasks in the array,
which means newer incomplete tasks appended to the end may not be persisted.
Replace this slice operation with a sort-then-slice approach that prioritizes
incomplete tasks (those not in finished or exists status) before complete tasks,
then slice the first 200 from the sorted result. This ensures newer active tasks
are preserved in storage instead of older completed ones.
- Around line 177-179: Simplify the filter condition in the tasksToPoll constant
assignment at line 178. The current filter logic combining the server task check
and status validation is difficult to read due to nested parentheses and
multiple conditions. Break down the condition into more readable parts by either
using intermediate variables or a helper function that evaluates the task
eligibility criteria, separating the server/non-server task logic from the
status validation logic. Ensure the refactored condition maintains the same
filtering behavior: including server tasks or non-server downloading tasks,
while also checking that the status is one of the allowed values (waiting,
downloading, or tagging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e7139b-52cf-4f3a-b11f-4896c1d9a6e5
📒 Files selected for processing (2)
public/music/js/download_manager.jspublic/music/sw.js
Author
|
Added one more fix for the song list page:\n\n- load each platform's song list tags/sort options before requesting the list\n- normalize the active sort id against the selected platform's sort list\n- guard tag/list requests against stale responses when switching platforms quickly\n\nThis fixes the QQ song list page using a stale sort value from the previous platform, which could make the latest/hot sort controls appear ineffective.\n\nValidation: node --check public/music/js/songlist_manager.js passed inside the Docker image. |
Author
|
Follow-up fix for QQ song list sorting:\n\nThe frontend calls changeSort('2'), so the backend received sortId as a string. QQ's playlist API treats order: '2' differently from order: 2; in testing, string '2' returned the same result set as hot/default, while numeric 2 returned the latest list.\n\nThis update coerces QQ song list sort IDs to numbers before building the musicu.fcg payload.\n\nValidation:\n- direct QQ API comparison confirmed numeric 2 and string '2' return different totals/results\n- node --check passed for src/modules/utils/musicSdk/tx/songList.js\n- exported test image includes this fix |
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.
📝 修改描述 (Description)
合并修复了一些问题。
1.本地音乐-右上方搜索框无效的bug。
2.给自动更新歌单添加了定时，以及可以手动检测全部歌单是否有更新，如果有更新，会在歌单用红色感叹号显示。那么可以手动更新需要更新的歌单。
3.修复直接下载歌单里面的歌曲，不会自动切换平台的问题。
4.修复了直接下载歌曲时，不会自动下载歌词，只会显示检测歌词的的bug。
5.修复了仅下载模式，下载歌曲到music，但是歌词依旧下载到cache的bug。
6.修复了仅下载模式下，歌曲命名方式不是按照设置的方式命名的bug。
7.修复了下载并发量修改无效的bug，并且测试了并发量为6时，ui会卡死，建议是直接删除并发量为6。
8.修复了收藏的歌手，点击该歌手全部音乐时，批量模式切换时传参顺序错了，source 和 order 会串位的bug。
9.修复了收藏多个歌手时，查看多个歌手的专辑，均显示为第一个歌手的专辑的bug。
10.修复了批量下载，歌曲数量超过1000时，浏览器卡死的bug。
11.修复了筛选缺封面时，原封面为0 kb的占位符，却被识别为封面的bug。
12.修复了批量下载时，全部暂停之后，不能继续下载的bug。
13.优化了打开本地歌曲的分页，当本地歌曲大于3000时，浏览器会卡顿的问题，以及把我的收藏的父项开关仅作为折叠/开启的作用。
🆎 修改类型 (Type of Change)
✅ 提交前检查清单 (Checklist)
在提交之前，请确保你完成了以下步骤：
dev分支，而不是main。📸 截图 (Screenshots - 可选)
Summary by CodeRabbit