danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Tighten Soulseek handoff + per-source request tokens after self-audit

Browse source 
Two bugs in the previous review-fix commits, found during a Cin-standard
re-audit:

A) Soulseek handoff stale state.query overrode the global widget's query

   The previous fix pre-set basicInput.value before clicking the Search
   page's Soulseek icon. But the click triggers onSoulseekSelected with
   the controller's CURRENT state.query — which is whatever the user
   last typed on /search, not the global widget's query. The Search
   page callback then ran `if (query) basicInput.value = query;` and
   overwrote the just-set value with the stale one before firing
   performDownloadsSearch.

   Fix: expose searchController as `_searchPageController` (mirrors
   `_searchPageRestoreOnEnter` already at module scope). Global
   widget's _gsNavigateToSearchPage syncs `_searchPageController.state.query`
   to its own query before clicking the icon. Also added a fallback
   for the case where the icon doesn't exist yet (controller still
   mid-init): swap sections + run performDownloadsSearch directly.

B) Single _requestSeq token leaked loadingSources across sources

   The earlier "stale request" fix used one global _requestSeq. But
   when the user switched Spotify → Deezer mid-fetch, the Spotify
   abort's catch block bailed (1 !== 2), leaving 'spotify' in
   loadingSources forever — permanent spinner on the Spotify icon
   even though no fetch was running for it.

   Fix: per-source `_sourceRequestIds[src]` map. Same-source
   supersession bails (correct), cross-source supersession still
   clears the old source's loadingSources entry (correct).

Bonus defensive: submitQuery now invalidates every per-source token
and aborts the in-flight fetch when the query string changes. Catches
the residual edge case where user clears the input — the in-flight
fetch's settle would otherwise write stale data into the just-cleared
state.sources.
This commit is contained in:
Broque Thomas 2026-04-23 22:37:07 -07:00
parent 325292ce5a
commit 527b51d69b
3 changed files with 74 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
webui/static/downloads.js
View file

@ -5188,15 +5188,35 @@ function _gsNavigateToSearchPage(query, src) {
// metadata results when they clicked the Soulseek icon.
setTimeout(() => {
if (src === 'soulseek') {
// Pre-fill the basic input first, then click the Search page's
// Soulseek icon. The icon's click triggers the controller's
// onSoulseekSelected callback, which owns the section swap and
// re-runs performDownloadsSearch with whatever's in the basic
// input (i.e., the value we just wrote).
const basicInput = document.getElementById('downloads-search-input');
if (basicInput && query) basicInput.value = query;
// Sync the Search page controller's state.query to the widget's
// query BEFORE clicking the Soulseek icon. Otherwise the icon
// click fires onSoulseekSelected(state.query) where state.query
// is whatever the user last typed on /search (often stale), and
// the callback would overwrite basicInput.value with that stale
// value before running performDownloadsSearch.
if (typeof _searchPageController !== 'undefined' && _searchPageController) {
_searchPageController.state.query = query || '';
}
const soulseekIcon = document.querySelector('#enh-source-row [data-source="soulseek"]');
if (soulseekIcon) soulseekIcon.click();
if (soulseekIcon) {
soulseekIcon.click();
return;
}
// Fallback: controller hasn't initialized yet (slow /api/settings
// fetches at first /search visit). Run the search directly + swap
// sections so the user still gets results. Icon row will catch up
// visually on the next render.
const basicSection = document.getElementById('basic-search-section');
const enhancedSection = document.getElementById('enhanced-search-section');
if (basicSection) basicSection.classList.add('active');
if (enhancedSection) enhancedSection.classList.remove('active');
if (basicInput && basicInput.value && typeof performDownloadsSearch === 'function') {
performDownloadsSearch();
}
return;
}
const input = document.getElementById('enhanced-search-input');

6
webui/static/search.js
View file

@ -41,6 +41,11 @@ let searchModeToggleInitialized = false;
// click is treated as outside-click and dismisses the dropdown, so when
// the user returns to /search we need to re-render whatever was cached.
let _searchPageRestoreOnEnter = null;
// Exposed so the global-search widget's Soulseek handoff can sync the
// controller's state.query to the widget's query before clicking the
// Soulseek icon — otherwise onSoulseekSelected fires with whatever the
// user last typed on /search and overwrites the basic input.
let _searchPageController = null;
function initializeSearchModeToggle() {
// Subsequent invocations: just re-display cached results so they don't
@ -207,6 +212,7 @@ function initializeSearchModeToggle() {
},
});
searchController.init();
_searchPageController = searchController;
// Expose a re-render hook so navigate-back to /search restores cached
// results instead of leaving the dropdown hidden. Deferred to the next

64
webui/static/shared-helpers.js
View file

@ -175,13 +175,20 @@ function createSearchController({
for (const src of SOURCE_ORDER) state.configuredSources[src] = true;
let abortCtrl = null;
// Monotonic request token. Each _fetchSource call captures the next
// value; settle/error blocks bail before mutating shared state if a
// newer request has superseded them. Without this, a fast retype lets
// the in-flight fetch's catch (or settle) clear loadingSources / write
// stale data into state.sources, causing a flash of empty/error UI
// while the new query's fetch is still running.
// Per-source request tokens. Each _fetchSource call increments the
// monotonic _requestSeq and stamps it into _sourceRequestIds[src].
// Settle/error blocks bail before mutating shared state if their
// requestId no longer matches the latest id for THAT source —
// protecting against the fast-retype race (same-source supersession)
// without dropping cleanup for cross-source supersession.
//
// A single global token would mishandle cross-source: switching
// Spotify → Deezer aborts Spotify's fetch, but Spotify's catch needs
// to clear 'spotify' from loadingSources (Deezer's request hasn't
// touched it). Per-source tracking lets each source's catch own its
// own loadingSources entry.
let _requestSeq = 0;
const _sourceRequestIds = Object.create(null);
function _notify() { if (onStateChange) onStateChange(state); }
@ -316,6 +323,7 @@ function createSearchController({
if (!query) return;
const requestId = ++_requestSeq;
_sourceRequestIds[src] = requestId;
state.loadingSources.add(src);
renderSourceRow();
@ -332,8 +340,11 @@ function createSearchController({
source: src,
signal: abortCtrl.signal,
});
// Bail without writing if a newer query has superseded us.
if (requestId !== _requestSeq) return;
// Bail without writing if a newer request for THIS source
// has superseded us. Cross-source supersession (different
// src entirely) is handled by the loadingSources cleanup
// below — each source's catch owns its own entry.
if (_sourceRequestIds[src] !== requestId) return;
state.sources[src] = {
artists: data.spotify_artists || [],
albums: data.spotify_albums || [],
@ -345,18 +356,20 @@ function createSearchController({
if (served && served !== src) state.fallbacks[src] = served;
}
// Only the latest request gets to clear loadingSources + notify.
// A stale completion would otherwise wipe the spinner the new
// request just set.
if (requestId !== _requestSeq) return;
if (_sourceRequestIds[src] !== requestId) return;
state.loadingSources.delete(src);
renderSourceRow();
_notify();
} catch (err) {
if (requestId !== _requestSeq) return;
state.loadingSources.delete(src);
renderSourceRow();
_notify();
// Only clear loadingSources if no newer request for THIS source
// is in flight. Cross-source supersession (e.g. user switched
// Spotify → Deezer) still falls through here so Spotify's
// spinner gets cleared on its own AbortError.
if (_sourceRequestIds[src] === requestId) {
state.loadingSources.delete(src);
renderSourceRow();
_notify();
}
if (err.name !== 'AbortError') {
console.debug(`Source fetch failed for ${src}:`, err);
}
@ -372,8 +385,9 @@ function createSearchController({
});
if (!res.ok) throw new Error(`YouTube search failed: ${res.status}`);
// Bail before allocating cache entry if superseded by a newer request.
if (requestId !== _requestSeq) return;
// Bail before allocating cache entry if a newer YouTube request
// has superseded us.
if (_sourceRequestIds['youtube_videos'] !== requestId) return;
state.sources['youtube_videos'] = {
artists: [], albums: [], tracks: [], videos: [], db_artists: [],
@ -386,9 +400,7 @@ function createSearchController({
while (true) {
const { done, value } = await reader.read();
if (done) break;
// Mid-stream supersession check — abort cleanly without writing
// additional chunks into stale cache.
if (requestId !== _requestSeq) return;
if (_sourceRequestIds['youtube_videos'] !== requestId) return;
buffer += decoder.decode(value, { stream: true });
let idx;
while ((idx = buffer.indexOf('\n')) !== -1) {
@ -399,7 +411,6 @@ function createSearchController({
const chunk = JSON.parse(line);
if (chunk.type === 'videos') {
cache.videos = chunk.data;
// Live-render if still the active source.
if (state.activeSource === 'youtube_videos') _notify();
}
} catch (_) { /* best-effort NDJSON parse */ }
@ -413,6 +424,15 @@ function createSearchController({
state.sources = {};
state.fallbacks = {};
state.loadingSources = new Set();
// Invalidate every in-flight per-source token. Without this, a
// settle that arrives AFTER a query reset (e.g. user typed 'a',
// fetch started, then user cleared the input) would still
// pass the per-source token check and write stale data back
// into the just-cleared state.sources. Setting fresh tokens
// when each new _fetchSource fires re-stamps as needed.
for (const k in _sourceRequestIds) delete _sourceRequestIds[k];
// Abort the active fetch — its results are useless now.
if (abortCtrl) { abortCtrl.abort(); abortCtrl = null; }
renderSourceRow();
}