From a827ba936a65b04be7090a3ae7aadffaea65304f Mon Sep 17 00:00:00 2001 From: JohnBaumb <80135794+JohnBaumb@users.noreply.github.com> Date: Fri, 24 Apr 2026 01:23:46 -0700 Subject: [PATCH] fix: address Copilot review - align queue threshold, downgrade logs, fix XSS --- web_server.py | 25 ++++---- webui/static/discover.js | 133 +++++++++++++++++++++++++++++---------- webui/static/style.css | 2 - 3 files changed, 112 insertions(+), 48 deletions(-) diff --git a/web_server.py b/web_server.py index 2e247d1f..acf61f0e 100644 --- a/web_server.py +++ b/web_server.py @@ -2546,7 +2546,8 @@ def _get_file_lock(file_path): # Thread-safe state tracking for modal download functionality with batch management missing_download_executor = ThreadPoolExecutor(max_workers=3, thread_name_prefix="MissingTrackWorker") # Separate executor for analysis to prevent starvation when download workers are busy -analysis_executor = ThreadPoolExecutor(max_workers=2, thread_name_prefix="AnalysisWorker") +MAX_CONCURRENT_ANALYSIS = 3 +analysis_executor = ThreadPoolExecutor(max_workers=MAX_CONCURRENT_ANALYSIS, thread_name_prefix="AnalysisWorker") download_tasks = {} # task_id -> task state dict download_batches = {} # batch_id -> {queue, active_count, max_concurrent} tasks_lock = threading.Lock() @@ -28987,11 +28988,11 @@ def _on_download_completed(batch_id, task_id, success=True): def _submit_or_queue_batch(batch_id, playlist_id, tracks): - """Submit a batch for analysis, or queue it if 3 analysis slots are full.""" + """Submit a batch for analysis, or queue it if all analysis slots are full.""" with tasks_lock: active_analysis_count = sum(1 for b in download_batches.values() if b.get('phase') == 'analysis') - if active_analysis_count >= 3: + if active_analysis_count >= MAX_CONCURRENT_ANALYSIS: download_batches[batch_id]['phase'] = 'queued' download_batches[batch_id]['_queued_tracks'] = tracks download_batches[batch_id]['_queued_playlist_id'] = playlist_id @@ -29006,7 +29007,7 @@ def _promote_queued_batches(): with tasks_lock: active_analysis_count = sum(1 for b in download_batches.values() if b.get('phase') == 'analysis') - if active_analysis_count >= 3: + if active_analysis_count >= MAX_CONCURRENT_ANALYSIS: return # Find batches waiting in queue, ordered by creation (dict insertion order) for bid, batch in list(download_batches.items()): @@ -29018,7 +29019,7 @@ def _promote_queued_batches(): logger.info(f"[Queue] Promoting batch {bid} ('{batch.get('playlist_name')}') from queued -> analysis") analysis_executor.submit(_run_full_missing_tracks_process, bid, queued_pid, queued_tracks) active_analysis_count += 1 - if active_analysis_count >= 3: + if active_analysis_count >= MAX_CONCURRENT_ANALYSIS: break @@ -32652,9 +32653,9 @@ def _record_sync_history_completion(batch_id, batch): completed_count = 0 failed_count = len(batch.get('permanently_failed_tracks', [])) - logger.warning(f"[SyncHistory] Recording completion for batch {batch_id}: " - f"analysis_results={len(analysis_results)}, tracks_found={tracks_found}, " - f"queue_len={len(queue)}, failed={failed_count}") + logger.info(f"[SyncHistory] Recording completion for batch {batch_id}: " + f"analysis_results={len(analysis_results)}, tracks_found={tracks_found}, " + f"queue_len={len(queue)}, failed={failed_count}") # Build download status map: track_index → status download_status_map = {} @@ -32666,8 +32667,8 @@ def _record_sync_history_completion(batch_id, batch): if task.get('status') == 'completed': completed_count += 1 - logger.warning(f"[SyncHistory] Batch {batch_id}: completed_downloads={completed_count}, " - f"download_status_map_size={len(download_status_map)}") + logger.info(f"[SyncHistory] Batch {batch_id}: completed_downloads={completed_count}, " + f"download_status_map_size={len(download_status_map)}") # Build per-track results from analysis track_results = [] @@ -32709,12 +32710,12 @@ def _record_sync_history_completion(batch_id, batch): db = MusicDatabase() updated = db.update_sync_history_completion(batch_id, tracks_found, completed_count, failed_count) - logger.warning(f"[SyncHistory] DB update for batch {batch_id}: updated={updated}") + logger.info(f"[SyncHistory] DB update for batch {batch_id}: updated={updated}") # Save per-track results if track_results: tr_updated = db.update_sync_history_track_results(batch_id, json.dumps(track_results)) - logger.warning(f"[SyncHistory] Track results saved for batch {batch_id}: updated={tr_updated}, count={len(track_results)}") + logger.info(f"[SyncHistory] Track results saved for batch {batch_id}: updated={tr_updated}, count={len(track_results)}") except Exception as e: logger.warning(f"Failed to record sync history completion: {e}") diff --git a/webui/static/discover.js b/webui/static/discover.js index e056c772..a12b30e8 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -9097,43 +9097,108 @@ function renderDiscoverSyncCard(playlist, container, sourceLabel) { const trackLabel = isEmpty ? 'No tracks yet' : `${playlist.track_count} tracks`; - card.innerHTML = ` -
${playlist.icon}
-
-
${playlist.name} - - ${sourceLabel || 'unknown'} - \u00b7 - ${trackLabel} - \u00b7 - ${statusText} - \u00b7 - ${lastSyncedText} - -
-
-
-
- - -
- -
- `; + // Build card using DOM nodes to avoid XSS from unescaped external data + const iconArea = document.createElement('div'); + iconArea.className = 'discover-sync-card-icon'; + iconArea.textContent = playlist.icon || ''; + + const infoArea = document.createElement('div'); + infoArea.className = 'discover-sync-card-info'; + + const nameEl = document.createElement('div'); + nameEl.className = 'discover-sync-card-name'; + nameEl.appendChild(document.createTextNode(playlist.name || '')); + + const metaInline = document.createElement('span'); + metaInline.className = 'discover-sync-card-meta-inline'; + + const sourceBadge = document.createElement('span'); + sourceBadge.className = 'discover-sync-source-badge'; + sourceBadge.textContent = sourceLabel || 'unknown'; + + const sep1 = document.createElement('span'); + sep1.className = 'discover-sync-separator'; + sep1.textContent = '\u00b7'; + + const trackCountEl = document.createElement('span'); + trackCountEl.className = 'discover-sync-track-count'; + trackCountEl.textContent = trackLabel; + + const sep2 = document.createElement('span'); + sep2.className = 'discover-sync-separator'; + sep2.textContent = '\u00b7'; + + const statusEl = document.createElement('span'); + statusEl.className = `discover-sync-status ${statusClass}`; + statusEl.textContent = statusText; + + const sep3 = document.createElement('span'); + sep3.className = 'discover-sync-separator'; + sep3.textContent = '\u00b7'; + + const lastSyncedEl = document.createElement('span'); + lastSyncedEl.className = 'discover-sync-last-synced'; + lastSyncedEl.textContent = lastSyncedText; + + metaInline.appendChild(sourceBadge); + metaInline.appendChild(sep1); + metaInline.appendChild(trackCountEl); + metaInline.appendChild(sep2); + metaInline.appendChild(statusEl); + metaInline.appendChild(sep3); + metaInline.appendChild(lastSyncedEl); + nameEl.appendChild(metaInline); + infoArea.appendChild(nameEl); + + const actions = document.createElement('div'); + actions.className = 'discover-sync-card-actions'; + + const toggleWrapper = document.createElement('div'); + toggleWrapper.className = 'discover-sync-toggle-wrapper'; + toggleWrapper.title = isEmpty + ? 'No tracks available \u2014 visit Discover first' + : 'Keep this playlist updated automatically'; + + const toggleLabel = document.createElement('label'); + toggleLabel.className = 'discover-sync-toggle-label'; + toggleLabel.textContent = 'Keep updated'; + + const toggle = document.createElement('label'); + toggle.className = 'discover-sync-toggle'; + + const checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + checkbox.checked = !!playlist.auto_update; + checkbox.disabled = !!isEmpty; + checkbox.addEventListener('change', function () { + toggleDiscoverAutoUpdate(playlist.type, this.checked); + }); + + const slider = document.createElement('span'); + slider.className = 'discover-sync-toggle-slider'; + + toggle.appendChild(checkbox); + toggle.appendChild(slider); + toggleWrapper.appendChild(toggleLabel); + toggleWrapper.appendChild(toggle); + + const syncButton = document.createElement('button'); + syncButton.className = 'discover-sync-btn'; + syncButton.id = `discover-sync-btn-${playlist.type}`; + syncButton.disabled = playlist.sync_status === 'syncing' || isEmpty; + syncButton.textContent = '\u27f3 Sync Now'; + syncButton.addEventListener('click', () => syncDiscoverPlaylistFromTab(playlist.type, playlist.name)); + + actions.appendChild(toggleWrapper); + actions.appendChild(syncButton); + + card.appendChild(iconArea); + card.appendChild(infoArea); + card.appendChild(actions); // Make the icon + info area clickable to view tracks if (!isEmpty) { - const clickArea = card.querySelector('.discover-sync-card-info'); - const iconArea = card.querySelector('.discover-sync-card-icon'); - [clickArea, iconArea].forEach(el => { + [infoArea, iconArea].forEach(el => { el.style.cursor = 'pointer'; el.addEventListener('click', () => openDiscoverPlaylistModal(playlist.type, playlist.name, playlist.icon)); }); diff --git a/webui/static/style.css b/webui/static/style.css index 6b5a66c1..db67cbd0 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -59492,8 +59492,6 @@ body.reduce-effects *::after { .discover-sync-card-meta { display: none; } - color: rgba(255, 255, 255, 0.4); -} .discover-sync-separator { opacity: 0.4;