fix: address Copilot review - align queue threshold, downgrade logs, fix XSS
This commit is contained in:
parent
f79cb182d4
commit
a827ba936a
3 changed files with 112 additions and 48 deletions
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -9097,43 +9097,108 @@ function renderDiscoverSyncCard(playlist, container, sourceLabel) {
|
|||
|
||||
const trackLabel = isEmpty ? 'No tracks yet' : `${playlist.track_count} tracks`;
|
||||
|
||||
card.innerHTML = `
|
||||
<div class="discover-sync-card-icon">${playlist.icon}</div>
|
||||
<div class="discover-sync-card-info">
|
||||
<div class="discover-sync-card-name">${playlist.name}
|
||||
<span class="discover-sync-card-meta-inline">
|
||||
<span class="discover-sync-source-badge">${sourceLabel || 'unknown'}</span>
|
||||
<span class="discover-sync-separator">\u00b7</span>
|
||||
<span class="discover-sync-track-count">${trackLabel}</span>
|
||||
<span class="discover-sync-separator">\u00b7</span>
|
||||
<span class="discover-sync-status ${statusClass}">${statusText}</span>
|
||||
<span class="discover-sync-separator">\u00b7</span>
|
||||
<span class="discover-sync-last-synced">${lastSyncedText}</span>
|
||||
</span>
|
||||
</div>
|
||||
</div>
|
||||
<div class="discover-sync-card-actions">
|
||||
<div class="discover-sync-toggle-wrapper" title="${isEmpty ? 'No tracks available — visit Discover first' : 'Keep this playlist updated automatically'}">
|
||||
<label class="discover-sync-toggle-label">Keep updated</label>
|
||||
<label class="discover-sync-toggle">
|
||||
<input type="checkbox" ${playlist.auto_update ? 'checked' : ''} ${isEmpty ? 'disabled' : ''}
|
||||
onchange="toggleDiscoverAutoUpdate('${playlist.type}', this.checked)">
|
||||
<span class="discover-sync-toggle-slider"></span>
|
||||
</label>
|
||||
</div>
|
||||
<button class="discover-sync-btn" id="discover-sync-btn-${playlist.type}"
|
||||
onclick="syncDiscoverPlaylistFromTab('${playlist.type}', '${playlist.name}')"
|
||||
${playlist.sync_status === 'syncing' || isEmpty ? 'disabled' : ''}>
|
||||
\u27f3 Sync Now
|
||||
</button>
|
||||
</div>
|
||||
`;
|
||||
// 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));
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in a new issue