Address PR review feedback from JohnBaumb and Cin
Four fixes from the review:
**library.js — back button stack (JohnBaumb):**
Replace the single-slot `artistDetailPageState.originPage` with an origin
stack. Chained navigation like Search → Artist A → similar Artist B →
similar Artist C now walks back one step at a time (C → B → A → Search)
instead of jumping straight to Search and skipping A and B.
`navigateToArtistDetail` takes an optional `{skipOriginPush}` flag so the
back button can re-enter a prior artist without re-pushing onto the stack.
Fresh entries from a non-artist page clear any stale stack from a prior
chain. Duplicate-click detection avoids pushing the same target twice.
Label derivation (`_updateArtistDetailBackButtonLabel`) reads the stack top
so the button says "Back to <ArtistName>" mid-chain and "Back to Search"
at the root.
**library.js — checkArtistEnhanceEligibility after library upgrade (Cin):**
The quality-analysis endpoint only works on library PKs. After the
library-upgrade branch rewrites `currentArtistId` from the source ID to
the library PK, the check was still using the original closure arg, so
upgraded source artists never hit `/api/library/artist/<id>/quality-analysis`.
Use `artistDetailPageState.currentArtistId` so the call gets the resolved id.
**init.js — isPageAllowed + home page recursion (Cin):**
- artist-detail is reachable from both Library and Search results now, so
permission check accepts either grant (plus legacy 'downloads'/'artists'
aliases). Search-only profiles can open source artists; legacy artists-only
profiles no longer recurse on the home redirect.
- `getProfileHomePage` rewrites 'artists' → 'search' (it already rewrote
'downloads') so legacy home_page values resolve correctly.
- Legacy-compat expanded in isPageAllowed to treat 'artists' as equivalent
to 'search' in both directions.
**init.js — profile edit forms dropping values on save (Cin):**
Both pageLabels maps (admin edit form + self-edit form) referenced the
legacy 'downloads'/'artists' keys. When editing a profile saved with
`home_page: 'search'` and `allowed_pages: ['search', 'library']`, the
home select didn't render a 'search' option, and the allowed_pages
checkboxes used 'downloads' as their value — so saving the form dropped
both values.
Update both maps to use 'search' as the canonical key. Add
`_normalizeLegacyAllowedPages` and `_normalizeLegacyHomePage` helpers that
migrate any legacy ids in allowed_pages/home_page on read, so a legacy
profile's first save upgrades its stored ids to the new canonical form.
This commit is contained in:
Broque Thomas
parent
1b3751598d
commit
b547909604
2 changed files with 137 additions and 45 deletions
|
|
@ -187,10 +187,28 @@ function applyReduceEffects(enabled) {
|
|||
// ── Profile System ─────────────────────────────────────────────
|
||||
let currentProfile = null;
|
||||
|
||||
// Normalize legacy allowed_pages entries so the profile edit forms (which are
|
||||
// built from the new pageLabels map containing 'search') don't drop access
|
||||
// when saving a profile that was stored before the Search page rename.
|
||||
// 'downloads' was the old Search id, 'artists' was the retired inline page.
|
||||
function _normalizeLegacyAllowedPages(pages) {
|
||||
if (!Array.isArray(pages)) return pages;
|
||||
const mapped = pages.map(id => (id === 'downloads' || id === 'artists') ? 'search' : id);
|
||||
return [...new Set(mapped)];
|
||||
}
|
||||
|
||||
function _normalizeLegacyHomePage(homePage) {
|
||||
if (homePage === 'downloads' || homePage === 'artists') return 'search';
|
||||
return homePage;
|
||||
}
|
||||
|
||||
function getProfileHomePage() {
|
||||
if (!currentProfile) return 'dashboard';
|
||||
// Legacy profiles stored the Search page as 'downloads'.
|
||||
const home = currentProfile.home_page === 'downloads' ? 'search' : currentProfile.home_page;
|
||||
// Legacy profiles stored the Search page as either 'downloads' (the old id
|
||||
// before the rename) or 'artists' (the retired inline Artists page).
|
||||
// Both fold into the unified Search page now.
|
||||
let home = currentProfile.home_page;
|
||||
if (home === 'downloads' || home === 'artists') home = 'search';
|
||||
if (home) return home;
|
||||
return currentProfile.is_admin ? 'dashboard' : 'discover';
|
||||
}
|
||||
|
|
@ -199,19 +217,24 @@ function isPageAllowed(pageId) {
|
|||
if (!currentProfile) return true;
|
||||
if (currentProfile.id === 1) return true;
|
||||
if (pageId === 'help' || pageId === 'issues') return true;
|
||||
if (pageId === 'settings') return currentProfile.is_admin;
|
||||
if (pageId === 'artist-detail') {
|
||||
// artist-detail requires library access
|
||||
// artist-detail is reachable from both Library and Search results, so
|
||||
// either grant unlocks it. Without this, a Search-only profile couldn't
|
||||
// open a source artist, and a legacy artists-only profile would hit the
|
||||
// home-redirect recursion path below.
|
||||
const ap = currentProfile.allowed_pages;
|
||||
if (!ap) return true;
|
||||
return ap.includes('library');
|
||||
return ap.includes('library') || ap.includes('search')
|
||||
|| ap.includes('downloads') || ap.includes('artists');
|
||||
}
|
||||
if (pageId === 'settings') return currentProfile.is_admin;
|
||||
const ap = currentProfile.allowed_pages;
|
||||
if (!ap) return true; // null = all pages
|
||||
if (ap.includes(pageId)) return true;
|
||||
// Legacy compat: the Search page used to be saved as 'downloads'.
|
||||
if (pageId === 'search' && ap.includes('downloads')) return true;
|
||||
if (pageId === 'downloads' && ap.includes('search')) return true;
|
||||
// Legacy compat: 'downloads' (old Search id) and 'artists' (retired inline
|
||||
// Artists page) both fold into the unified 'search' page.
|
||||
if (pageId === 'search' && (ap.includes('downloads') || ap.includes('artists'))) return true;
|
||||
if ((pageId === 'downloads' || pageId === 'artists') && ap.includes('search')) return true;
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
@ -1574,9 +1597,11 @@ function showProfileEditForm(profileId, currentName, currentColor, currentAvatar
|
|||
const isAdmin = currentProfile && currentProfile.is_admin;
|
||||
const isEditingAdmin = profileSettings.is_admin;
|
||||
const editColors = ['#6366f1', '#ec4899', '#10b981', '#f59e0b', '#3b82f6', '#ef4444', '#8b5cf6', '#14b8a6'];
|
||||
// 'search' replaces the legacy 'downloads'/'artists' ids; legacy values are
|
||||
// normalized on read below so saving a legacy profile upgrades it.
|
||||
const pageLabels = {
|
||||
dashboard: 'Dashboard', sync: 'Sync', downloads: 'Search', discover: 'Discover',
|
||||
artists: 'Artists', automations: 'Automations', library: 'Library', stats: 'Listening Stats',
|
||||
dashboard: 'Dashboard', sync: 'Sync', search: 'Search', discover: 'Discover',
|
||||
automations: 'Automations', library: 'Library', stats: 'Listening Stats',
|
||||
'playlist-explorer': 'Playlist Explorer', import: 'Import', help: 'Help & Docs'
|
||||
};
|
||||
|
||||
|
|
@ -1628,14 +1653,16 @@ function showProfileEditForm(profileId, currentName, currentColor, currentAvatar
|
|||
defaultOpt.value = '';
|
||||
defaultOpt.textContent = isEditingAdmin ? 'Default (Dashboard)' : 'Default (Discover)';
|
||||
homeSelect.appendChild(defaultOpt);
|
||||
// Filter home page options to only allowed pages
|
||||
const allowedSet = profileSettings.allowed_pages;
|
||||
// Normalize legacy ids so the form shows the right options/selection for
|
||||
// profiles saved before the Search page rename.
|
||||
const allowedSet = _normalizeLegacyAllowedPages(profileSettings.allowed_pages);
|
||||
const normalizedHome = _normalizeLegacyHomePage(profileSettings.home_page);
|
||||
Object.entries(pageLabels).forEach(([id, label]) => {
|
||||
if (allowedSet && !allowedSet.includes(id)) return; // Skip non-permitted
|
||||
const opt = document.createElement('option');
|
||||
opt.value = id;
|
||||
opt.textContent = label;
|
||||
if (id === profileSettings.home_page) opt.selected = true;
|
||||
if (id === normalizedHome) opt.selected = true;
|
||||
homeSelect.appendChild(opt);
|
||||
});
|
||||
form.appendChild(homeSelect);
|
||||
|
|
@ -1760,9 +1787,11 @@ function showSelfEditForm() {
|
|||
const existing = document.getElementById('self-edit-form');
|
||||
if (existing) existing.remove();
|
||||
|
||||
// 'search' replaces the legacy 'downloads'/'artists' ids; legacy values are
|
||||
// normalized on read below so saving a legacy profile upgrades it.
|
||||
const pageLabels = {
|
||||
dashboard: 'Dashboard', sync: 'Sync', downloads: 'Search', discover: 'Discover',
|
||||
artists: 'Artists', automations: 'Automations', library: 'Library', stats: 'Listening Stats',
|
||||
dashboard: 'Dashboard', sync: 'Sync', search: 'Search', discover: 'Discover',
|
||||
automations: 'Automations', library: 'Library', stats: 'Listening Stats',
|
||||
'playlist-explorer': 'Playlist Explorer', import: 'Import', help: 'Help & Docs'
|
||||
};
|
||||
|
||||
|
|
@ -1797,13 +1826,16 @@ function showSelfEditForm() {
|
|||
defaultOpt.value = '';
|
||||
defaultOpt.textContent = 'Default (Discover)';
|
||||
homeSelect.appendChild(defaultOpt);
|
||||
const ap = currentProfile.allowed_pages;
|
||||
// Normalize legacy ids so the form shows the right options/selection for
|
||||
// profiles saved before the Search page rename.
|
||||
const ap = _normalizeLegacyAllowedPages(currentProfile.allowed_pages);
|
||||
const normalizedHome = _normalizeLegacyHomePage(currentProfile.home_page);
|
||||
Object.entries(pageLabels).forEach(([id, label]) => {
|
||||
if (ap && !ap.includes(id)) return;
|
||||
const opt = document.createElement('option');
|
||||
opt.value = id;
|
||||
opt.textContent = label;
|
||||
if (id === currentProfile.home_page) opt.selected = true;
|
||||
if (id === normalizedHome) opt.selected = true;
|
||||
homeSelect.appendChild(opt);
|
||||
});
|
||||
form.appendChild(homeSelect);
|
||||
|
|
|
|||
|
|
@ -640,7 +640,11 @@ let artistDetailPageState = {
|
|||
currentArtistId: null,
|
||||
currentArtistName: null,
|
||||
currentArtistSource: null,
|
||||
originPage: null, // page id captured by navigateToArtistDetail for the back button
|
||||
// Stack of origins captured by navigateToArtistDetail for the back button.
|
||||
// Each entry is either {type:'page', pageId} or {type:'artist', id, name, source}
|
||||
// so chained navigation (Search → A → similar B → similar C) walks back one
|
||||
// step at a time instead of jumping straight to Search.
|
||||
originStack: [],
|
||||
enhancedView: false,
|
||||
enhancedData: null,
|
||||
expandedAlbums: new Set(),
|
||||
|
|
@ -672,19 +676,45 @@ const _ARTIST_DETAIL_BACK_LABELS = {
|
|||
'active-downloads': 'Back to Downloads',
|
||||
};
|
||||
|
||||
function navigateToArtistDetail(artistId, artistName, sourceOverride = null) {
|
||||
function navigateToArtistDetail(artistId, artistName, sourceOverride = null, options = {}) {
|
||||
console.log(`🎵 Navigating to artist detail: ${artistName} (ID: ${artistId}${sourceOverride ? `, source: ${sourceOverride}` : ''})`);
|
||||
|
||||
// Capture where the user is coming from BEFORE navigateToPage flips
|
||||
// currentPage. The back button on the artist-detail page reads this so it
|
||||
// can say "Back to Search" / "Back to Discover" / etc. instead of the
|
||||
// hardcoded "Back to Library". Falls back to library when origin is
|
||||
// unknown or when chaining from one artist-detail page to another.
|
||||
let origin = (typeof currentPage === 'string' && currentPage) ? currentPage : null;
|
||||
if (!origin || origin === 'artist-detail') {
|
||||
origin = artistDetailPageState.originPage || 'library';
|
||||
// Capture the current location on the origin stack BEFORE navigateToPage
|
||||
// flips currentPage. The back button walks this stack one step at a time,
|
||||
// so a chain like Search → A → similar B → similar C steps back through
|
||||
// C → B → A → Search instead of jumping straight home. `skipOriginPush`
|
||||
// lets the back button re-enter a prior artist without re-pushing.
|
||||
if (!options.skipOriginPush) {
|
||||
// Fresh entry (from a non-artist page) starts a new chain; any stale
|
||||
// entries from a prior artist-detail visit are dropped.
|
||||
if (currentPage !== 'artist-detail') {
|
||||
artistDetailPageState.originStack = [];
|
||||
}
|
||||
|
||||
let entry;
|
||||
if (currentPage === 'artist-detail' && artistDetailPageState.currentArtistId) {
|
||||
entry = {
|
||||
type: 'artist',
|
||||
id: artistDetailPageState.currentArtistId,
|
||||
name: artistDetailPageState.currentArtistName,
|
||||
source: artistDetailPageState.currentArtistSource,
|
||||
};
|
||||
} else {
|
||||
const pageId = (typeof currentPage === 'string' && currentPage && currentPage !== 'artist-detail')
|
||||
? currentPage : 'library';
|
||||
entry = { type: 'page', pageId };
|
||||
}
|
||||
|
||||
// Avoid pushing a duplicate top entry on repeated clicks of the same target.
|
||||
const top = artistDetailPageState.originStack[artistDetailPageState.originStack.length - 1];
|
||||
const isDuplicate = top && top.type === entry.type && (
|
||||
(entry.type === 'page' && top.pageId === entry.pageId) ||
|
||||
(entry.type === 'artist' && String(top.id) === String(entry.id))
|
||||
);
|
||||
if (!isDuplicate) {
|
||||
artistDetailPageState.originStack.push(entry);
|
||||
}
|
||||
}
|
||||
artistDetailPageState.originPage = origin;
|
||||
|
||||
// Abort any in-progress completion stream
|
||||
if (artistDetailPageState.completionController) {
|
||||
|
|
@ -731,12 +761,8 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null) {
|
|||
// Navigate to artist detail page
|
||||
navigateToPage('artist-detail');
|
||||
|
||||
// Update back-button label for the captured origin.
|
||||
const backBtnLabel = document.querySelector('#artist-detail-back-btn span');
|
||||
if (backBtnLabel) {
|
||||
const friendly = _ARTIST_DETAIL_BACK_LABELS[origin] || _ARTIST_DETAIL_BACK_LABELS.library;
|
||||
backBtnLabel.textContent = `← ${friendly}`;
|
||||
}
|
||||
// Update back-button label to reflect where the next pop will land.
|
||||
_updateArtistDetailBackButtonLabel();
|
||||
|
||||
// Initialize if needed and load data
|
||||
if (!artistDetailPageState.isInitialized) {
|
||||
|
|
@ -747,27 +773,58 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null) {
|
|||
loadArtistDetailData(artistId, artistName);
|
||||
}
|
||||
|
||||
function _updateArtistDetailBackButtonLabel() {
|
||||
const backBtnLabel = document.querySelector('#artist-detail-back-btn span');
|
||||
if (!backBtnLabel) return;
|
||||
const stack = artistDetailPageState.originStack || [];
|
||||
const top = stack[stack.length - 1];
|
||||
if (!top) {
|
||||
backBtnLabel.textContent = `← ${_ARTIST_DETAIL_BACK_LABELS.library}`;
|
||||
} else if (top.type === 'artist') {
|
||||
backBtnLabel.textContent = `← Back to ${top.name}`;
|
||||
} else {
|
||||
const friendly = _ARTIST_DETAIL_BACK_LABELS[top.pageId] || _ARTIST_DETAIL_BACK_LABELS.library;
|
||||
backBtnLabel.textContent = `← ${friendly}`;
|
||||
}
|
||||
}
|
||||
|
||||
function initializeArtistDetailPage() {
|
||||
console.log("🔧 Initializing Artist Detail page...");
|
||||
|
||||
// Initialize back button — navigates back to whichever page initiated the
|
||||
// current artist-detail view (Search, Discover, Watchlist, Library, etc.),
|
||||
// captured by navigateToArtistDetail before the page swap.
|
||||
// Initialize back button — pops the origin stack one step at a time so a
|
||||
// chain like Search → A → B → C walks back through C → B → A → Search
|
||||
// instead of jumping straight to the original entry page.
|
||||
const backBtn = document.getElementById("artist-detail-back-btn");
|
||||
if (backBtn) {
|
||||
backBtn.addEventListener("click", () => {
|
||||
const dest = artistDetailPageState.originPage || 'library';
|
||||
console.log(`🔙 Returning to ${dest}`);
|
||||
// Abort any in-progress completion stream
|
||||
// Abort any in-progress completion stream regardless of destination
|
||||
if (artistDetailPageState.completionController) {
|
||||
artistDetailPageState.completionController.abort();
|
||||
artistDetailPageState.completionController = null;
|
||||
}
|
||||
// Clear artist detail state so we go back to the list view
|
||||
|
||||
const stack = artistDetailPageState.originStack || [];
|
||||
if (stack.length > 0) {
|
||||
const target = stack.pop();
|
||||
if (target.type === 'artist') {
|
||||
// Re-enter a prior artist in the chain without re-pushing,
|
||||
// so the stack keeps shrinking as the user steps back.
|
||||
navigateToArtistDetail(target.id, target.name, target.source, { skipOriginPush: true });
|
||||
return;
|
||||
}
|
||||
// target.type === 'page' — fully exit the artist-detail chain
|
||||
artistDetailPageState.currentArtistId = null;
|
||||
artistDetailPageState.currentArtistName = null;
|
||||
artistDetailPageState.originStack = [];
|
||||
navigateToPage(target.pageId);
|
||||
return;
|
||||
}
|
||||
|
||||
// No history — default to library
|
||||
artistDetailPageState.currentArtistId = null;
|
||||
artistDetailPageState.currentArtistName = null;
|
||||
artistDetailPageState.originPage = null;
|
||||
navigateToPage(dest);
|
||||
artistDetailPageState.originStack = [];
|
||||
navigateToPage('library');
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -874,8 +931,11 @@ async function loadArtistDetailData(artistId, artistName) {
|
|||
}
|
||||
}
|
||||
|
||||
// Check if artist has tracks eligible for quality enhancement
|
||||
checkArtistEnhanceEligibility(artistId);
|
||||
// Check if artist has tracks eligible for quality enhancement.
|
||||
// Use currentArtistId (not the closure arg) because the library-upgrade
|
||||
// branch above may have rewritten it from the source ID to the library PK,
|
||||
// and /api/library/artist/<id>/quality-analysis only works on library PKs.
|
||||
checkArtistEnhanceEligibility(artistDetailPageState.currentArtistId);
|
||||
|
||||
} catch (error) {
|
||||
console.error(`❌ Error loading artist detail data:`, error);
|
||||
|
|
|
|||
Loading…
Reference in a new issue