diff --git a/frontend/index.ts b/frontend/index.ts
index fee2280..cb7396a 100644
--- a/frontend/index.ts
+++ b/frontend/index.ts
@@ -40,6 +40,7 @@ import { setBasePath } from '@awesome.me/webawesome/dist/webawesome.js';
import { registerBundledIcons } from './src/icons';
import { queueStore } from '@store/queue-store';
import { searchStore } from '@store/search-store';
+import { activeViewStore } from '@store/active-view-store';
import * as Player from '@go/player/player.js';
import * as Queue from '@go/queue/queue.js';
import { GetDefaultPage } from '@go/config/config.js';
@@ -279,6 +280,20 @@ async function handleNavigate(
// attribute keeps e2e selectors semantic instead of structural.
mainContent.dataset.activeView = view;
+ // And publishing it as a *value* is what the nav components read.
+ // They used to learn the active view from the `navigate` event,
+ // which only the outbound path dispatches -- so a back-navigation
+ // left both of them highlighting the view it had just left (#72).
+ // Re-dispatching `navigate` here is not the fix: this file is a
+ // document listener for it, so that is an infinite loop, and
+ // "please go to X" is not the statement being made.
+ //
+ // `view in VIEW_TAGS` is the primary/detail split, and it is passed
+ // rather than re-derived because this table is where it is written
+ // down. A detail view therefore leaves the tab it was opened from
+ // lit, which is what the report asks for.
+ activeViewStore.setView(view, view in VIEW_TAGS);
+
// --- Primary (cacheable) views ----------------------------------------
if (view in VIEW_TAGS) {
// Remove any active detail view first
diff --git a/frontend/src/components/bottom-nav/bottom-nav.ts b/frontend/src/components/bottom-nav/bottom-nav.ts
index 4cae8f2..24f6d7c 100644
--- a/frontend/src/components/bottom-nav/bottom-nav.ts
+++ b/frontend/src/components/bottom-nav/bottom-nav.ts
@@ -7,6 +7,7 @@ import { designTokens } from '../../styles/tokens.css';
import '../sidebar/app-sidebar.js';
import { nameDialog } from '@utils/name-dialog';
import { ICON_PLAYLIST } from '@utils/icon-language';
+import { ActiveViewController } from '@store/controllers/active-view-controller';
type View = 'home' | 'albums' | 'tracks' | 'playlists';
@@ -114,8 +115,19 @@ export class BottomNav extends LitElement {
}
`];
- @state()
- private activeView = 'home';
+ /**
+ * Which tab is lit, read from the shell rather than tracked here.
+ *
+ * This was a `@state()` field set from the `navigate` event, which
+ * only the outbound path dispatches -- so backing out of a detail
+ * view left the highlight wherever it had been (#72). It had no
+ * equivalent of `app-sidebar`'s `navItems.some(...)` guard either,
+ * so a detail view set it to a name matching no tab and *nothing*
+ * was lit; that asymmetry is why one nav looked broken and the
+ * other looked fine. The store answers both: a detail view leaves
+ * the tab it was opened from lit, in both components.
+ */
+ private activeCtrl = new ActiveViewController(this);
/**
* Whether the drawer has been asked for.
@@ -167,12 +179,9 @@ export class BottomNav extends LitElement {
nameDialog(this.drawer);
}
- private onGlobalNavigate = (e: Event) => {
- const detail = (e as CustomEvent<{ view?: string }>).detail;
-
- if (detail?.view) this.activeView = detail.view;
-
+ private onGlobalNavigate = () => {
// A navigation from inside the drawer is the drawer's job done.
+ // The highlight is not this listener's business any more.
this.drawerOpen = false;
};
@@ -206,9 +215,11 @@ export class BottomNav extends LitElement {
this.navigate(tab.id)}
diff --git a/frontend/src/components/sidebar/app-sidebar.ts b/frontend/src/components/sidebar/app-sidebar.ts
index 1c3c56f..dddb203 100644
--- a/frontend/src/components/sidebar/app-sidebar.ts
+++ b/frontend/src/components/sidebar/app-sidebar.ts
@@ -4,6 +4,7 @@ import '@awesome.me/webawesome/dist/components/icon/icon.js';
import { designTokens } from '../../styles/tokens.css';
import type { DragActiveDetail } from '@utils/drag-controller';
+import { ActiveViewController } from '@store/controllers/active-view-controller';
import {
ICON_PLAYLIST,
ICON_AUTOTAG,
@@ -159,11 +160,20 @@ export class AppSidebar extends LitElement {
/** Delay in ms before a drag-hover triggers navigation. */
private static readonly HOVER_NAV_DELAY = 600;
- /** Home, because that is where `index.ts` now navigates on startup
- * (H-8). The sidebar does not hear a `navigate` it did not send,
- * so this default is what keeps `aria-current` honest on arrival. */
- @state()
- private activeView: View = 'home';
+ /**
+ * Which item is lit, read from the shell rather than tracked here.
+ *
+ * This used to be a `@state()` field defaulting to `home` -- the
+ * landing view -- because "the sidebar does not hear a `navigate`
+ * it did not send". That default was the only honest moment it
+ * ever had: a back-navigation dispatches no `navigate`, so the
+ * highlight stayed on the view the user had just left (#72), and
+ * the copy of this component that `bottom-nav` mounts inside its
+ * drawer opened on `home` from whatever page you were standing on.
+ * The shell publishes the active view now, so there is nothing to
+ * default and nothing to keep in step.
+ */
+ private activeCtrl = new ActiveViewController(this);
@state()
private isDragging = false;
@@ -237,10 +247,6 @@ export class AppSidebar extends LitElement {
'yj-drag-active',
this.onDragActive as EventListener,
);
- document.addEventListener(
- 'navigate',
- this.onGlobalNavigate as EventListener,
- );
}
override disconnectedCallback() {
@@ -262,10 +268,6 @@ export class AppSidebar extends LitElement {
'yj-drag-active',
this.onDragActive as EventListener,
);
- document.removeEventListener(
- 'navigate',
- this.onGlobalNavigate as EventListener,
- );
this.clearDragHoverTimer();
}
@@ -282,8 +284,9 @@ export class AppSidebar extends LitElement {
${this.navItems.map((item) => {
+ const active = this.activeCtrl.isActive(item.id);
const classes = [
- this.activeView === item.id
+ active
? 'active'
: '',
this.dragHoverView === item.id
@@ -299,7 +302,7 @@ export class AppSidebar extends LitElement {
type="button"
class=${classes}
data-testid="nav-${item.id}"
- aria-current=${this.activeView === item.id
+ aria-current=${active
? 'page'
: 'false'}
@click=${() =>
@@ -382,19 +385,6 @@ export class AppSidebar extends LitElement {
private static readonly DROP_VIEWS: Set =
new Set(['playlists']);
- /** Keeps the highlighted nav item in sync with navigation that
- * originates outside the sidebar itself (e.g. the launch-page
- * dispatch in index.ts). */
- private onGlobalNavigate = (
- e: CustomEvent<{ view?: string }>,
- ) => {
- const view = e.detail.view;
-
- if (view && this.navItems.some((item) => item.id === view)) {
- this.activeView = view as View;
- }
- };
-
private onDragActive = (
e: CustomEvent,
) => {
@@ -460,7 +450,11 @@ export class AppSidebar extends LitElement {
}
private navigate(view: View) {
- this.activeView = view;
+ // No optimistic highlight: the shell answers, and it answers
+ // synchronously in `handleNavigate` before it awaits anything.
+ // Setting it here as well is the second opinion this fix
+ // removes -- it is what let a click's highlight survive a
+ // navigation the shell then handled differently.
this.dispatchEvent(new CustomEvent('navigate', {
detail: { view },
bubbles: true,
diff --git a/frontend/src/store/active-view-store.ts b/frontend/src/store/active-view-store.ts
new file mode 100644
index 0000000..665f448
--- /dev/null
+++ b/frontend/src/store/active-view-store.ts
@@ -0,0 +1,90 @@
+/**
+ * Which primary view the app is showing.
+ *
+ * The shell has always known this -- `handleNavigate()` sets
+ * `#main-content`'s `data-active-view` on every path, `_isBack`
+ * included -- and never told anyone. The nav components learned it
+ * from the `navigate` CustomEvent instead, which only the *outbound*
+ * path dispatches: the `popstate` listener calls `handleNavigate()`
+ * directly. So both navs kept highlighting the view you had just left
+ * (#72).
+ *
+ * The fix cannot be a re-dispatch of `navigate`. `index.ts` is itself a
+ * document listener for it, so emitting one from inside
+ * `handleNavigate` is an infinite loop -- and the two statements are
+ * different anyway: `navigate` means *please go to X*, and 28 call
+ * sites across 18 files say it. This says *the active view is now X*,
+ * which only the shell is in a position to say and only once per
+ * navigation.
+ *
+ * Three things about it are load-bearing.
+ *
+ * **It is a store rather than an event**, because a component that
+ * mounts *after* a navigation still has to know. `bottom-nav`'s "More"
+ * drawer creates its `` on open, and that copy had heard
+ * no `navigate` at all: standing on Albums, the drawer highlighted
+ * Home -- its `activeView` default, which existed to match the landing
+ * view and matched nothing else ever after. An event has no answer for
+ * a listener that was not there; a value does.
+ *
+ * **A detail view is not a view here.** Opening one leaves the primary
+ * view it was opened from lit, which is what #72 asks for and what
+ * `app-sidebar` used to do by accident -- it guarded on
+ * `navItems.some(...)`, so a name matching no item left its highlight
+ * alone. `bottom-nav` had no such guard and so lit nothing on a detail
+ * view. Neither was correct; the sidebar was stale-but-lucky, and
+ * stating the rule once is what makes the two agree.
+ *
+ * **Whether a view is primary is the shell's fact, not this store's.**
+ * `VIEW_TAGS` in `index.ts` is the list, and a copy of it here is a
+ * second list to forget -- so the caller passes the answer it already
+ * has rather than this file re-deriving it.
+ */
+
+type Subscriber = () => void;
+
+class ActiveViewStore {
+ /** Empty until the shell's first navigation, which happens at
+ * startup from `GetDefaultPage()`. Nothing is highlighted for that
+ * moment, which is honest: the alternative is a written-down
+ * default that is right only when the default page agrees with it. */
+ private activeView = '';
+
+ private subscribers = new Set();
+
+ /** The active primary view, e.g. `albums`. */
+ get(): string {
+ return this.activeView;
+ }
+
+ isActive(view: string): boolean {
+ return this.activeView !== '' && this.activeView === view;
+ }
+
+ /**
+ * Called by the shell on every navigation, `popstate` included.
+ *
+ * `isPrimary` is `view in VIEW_TAGS` at the call site: a detail
+ * view reports itself and deliberately changes nothing, so the view
+ * it was opened from stays lit until the user picks another one.
+ */
+ setView(view: string, isPrimary: boolean): void {
+ if (!isPrimary) return;
+ if (view === this.activeView) return;
+
+ this.activeView = view;
+ this.notify();
+ }
+
+ subscribe(fn: Subscriber): () => void {
+ this.subscribers.add(fn);
+
+ return () => this.subscribers.delete(fn);
+ }
+
+ private notify(): void {
+ this.subscribers.forEach((fn) => fn());
+ }
+}
+
+export const activeViewStore = new ActiveViewStore();
diff --git a/frontend/src/store/controllers/active-view-controller.ts b/frontend/src/store/controllers/active-view-controller.ts
new file mode 100644
index 0000000..35a367c
--- /dev/null
+++ b/frontend/src/store/controllers/active-view-controller.ts
@@ -0,0 +1,59 @@
+import type {
+ ReactiveController,
+ ReactiveControllerHost,
+} from 'lit';
+import { activeViewStore } from '../active-view-store';
+
+/**
+ * ActiveViewController connects a Lit component to the
+ * ActiveViewStore.
+ *
+ * Usage in a component:
+ *
+ * private activeCtrl = new ActiveViewController(this);
+ *
+ * render() {
+ * const lit = this.activeCtrl.isActive('albums');
+ * }
+ *
+ * It reads through to the store rather than copying the value into a
+ * `@state()` field, which is the point of #72: two components holding
+ * their own idea of the active view is what let them disagree with the
+ * shell and with each other.
+ */
+export class ActiveViewController implements ReactiveController {
+ private host: ReactiveControllerHost;
+ private unsubscribe?: () => void;
+
+ constructor(host: ReactiveControllerHost) {
+ this.host = host;
+ host.addController(this);
+ }
+
+ // ===============================================================
+ // LIFECYCLE HOOKS
+ // ===============================================================
+
+ hostConnected(): void {
+ this.unsubscribe = activeViewStore.subscribe(() => {
+ this.host.requestUpdate();
+ });
+ }
+
+ hostDisconnected(): void {
+ this.unsubscribe?.();
+ }
+
+ // ===============================================================
+ // DATA ACCESS
+ // ===============================================================
+
+ /** The active primary view, e.g. `albums`. */
+ get current(): string {
+ return activeViewStore.get();
+ }
+
+ isActive(view: string): boolean {
+ return activeViewStore.isActive(view);
+ }
+}
diff --git a/frontend/src/store/index.ts b/frontend/src/store/index.ts
index 2f8fc2a..d957fcc 100644
--- a/frontend/src/store/index.ts
+++ b/frontend/src/store/index.ts
@@ -9,6 +9,8 @@ export type { ThemeState, BackgroundShade } from './theme-store';
export { ThemeController } from './controllers/theme-controller';
export { searchStore } from './search-store';
export { SearchController } from './controllers/search-controller';
+export { activeViewStore } from './active-view-store';
+export { ActiveViewController } from './controllers/active-view-controller';
export { shortcutsStore } from './shortcuts-store';
export type { ShortcutsState } from './shortcuts-store';
export { ShortcutsController } from './controllers/shortcuts-controller';
diff --git a/frontend/test/components/bottom-nav.test.ts b/frontend/test/components/bottom-nav.test.ts
index 1949c7f..a8eb66b 100644
--- a/frontend/test/components/bottom-nav.test.ts
+++ b/frontend/test/components/bottom-nav.test.ts
@@ -3,15 +3,21 @@
*
* Three of these are about the thing that makes a second nav dangerous:
* it has to agree with the first one. `bottom-nav` emits the same
- * bubbling, composed `navigate` event `app-sidebar` does and listens
- * for that event globally, so a navigation from anywhere — a card, a
- * detail view, the drawer's own sidebar — moves its highlight too. A
- * tab bar that only tracks its own clicks looks right until the moment
- * the user arrives somewhere by another route.
+ * bubbling, composed `navigate` event `app-sidebar` does, and reads
+ * which tab is lit from `activeViewStore` — the shell's one statement
+ * of where the user is — so it follows a navigation from anywhere: a
+ * card, a detail view, the drawer's own sidebar, or the back gesture.
+ *
+ * That last one is why the source is the store and not the `navigate`
+ * event these tests used to dispatch. `popstate` dispatches no
+ * `navigate` (index.ts calls `handleNavigate` directly), so a tab bar
+ * listening for the event looked right until the user pressed back —
+ * #72.
*/
import { describe, expect, it, beforeEach } from 'vitest';
import '@components/bottom-nav/bottom-nav';
+import { activeViewStore } from '@store/active-view-store';
import type { BottomNav } from '@components/bottom-nav/bottom-nav';
import { fixture, shadow, shadowAll, update } from '@test/support/render';
import { resetHarness } from '@test/support/harness';
@@ -21,6 +27,12 @@ type Nav = BottomNav;
const tabs = (el: HTMLElement) =>
shadowAll(el, 'nav button');
+/** The testids of whatever the bar says is the current page. */
+const current = (el: HTMLElement) =>
+ tabs(el)
+ .filter((b) => b.getAttribute('aria-current') === 'page')
+ .map((b) => b.dataset.testid);
+
/** Resolve on one occurrence of an event, or reject loudly on time. */
const once = (el: Element, name: string, timeoutMs = 2000) =>
new Promise((resolve, reject) => {
@@ -70,36 +82,55 @@ describe('bottom-nav', () => {
it('follows a navigation it did not send', async () => {
const el = await fixture('bottom-nav');
- document.dispatchEvent(new CustomEvent('navigate', {
- detail: { view: 'tracks' },
- bubbles: true,
- composed: true,
- }));
+ activeViewStore.setView('tracks', true);
await update(el, {});
- const current = tabs(el)
- .filter((b) => b.getAttribute('aria-current') === 'page')
- .map((b) => b.dataset.testid);
-
- expect(current).toEqual(['tab-tracks']);
+ expect(current(el)).toEqual(['tab-tracks']);
});
it('marks exactly one tab current, and none for a view it has no tab for', async () => {
const el = await fixture('bottom-nav');
- document.dispatchEvent(new CustomEvent('navigate', {
- detail: { view: 'settings' },
- bubbles: true,
- composed: true,
- }));
+ activeViewStore.setView('settings', true);
await update(el, {});
// Settings lives in the drawer, so nothing in the bar is current.
// Leaving Home highlighted would be a tab bar lying about where
// the user is.
- expect(
- tabs(el).filter((b) => b.getAttribute('aria-current') === 'page'),
- ).toHaveLength(0);
+ expect(current(el)).toEqual([]);
+ });
+
+ it('keeps the parent tab lit while a detail view is open', async () => {
+ const el = await fixture('bottom-nav');
+
+ activeViewStore.setView('albums', true);
+ // A detail view reports itself and is not primary, so it changes
+ // nothing. This is the first half of #72: the bar used to take the
+ // name, match it against no tab, and light nothing at all — while
+ // `app-sidebar`, which guarded on its own item list, kept the
+ // highlight. Neither was deliberate and the two disagreed.
+ activeViewStore.setView('explore-album-details', false);
+ await update(el, {});
+
+ expect(current(el)).toEqual(['tab-albums']);
+ });
+
+ it('follows the back path, which dispatches no navigate event', async () => {
+ const el = await fixture('bottom-nav');
+
+ activeViewStore.setView('albums', true);
+ activeViewStore.setView('tracks', true);
+ await update(el, {});
+ expect(current(el)).toEqual(['tab-tracks']);
+
+ // What `popstate` does: the shell replays the entry through
+ // `handleNavigate` without dispatching `navigate`. A bar listening
+ // for the event stayed on Tracks — the view just left, confidently
+ // wrong rather than merely blank.
+ activeViewStore.setView('albums', true);
+ await update(el, {});
+
+ expect(current(el)).toEqual(['tab-albums']);
});
it('closes the drawer when a navigation happens', async () => {
diff --git a/frontend/test/components/chrome.test.ts b/frontend/test/components/chrome.test.ts
index 1384145..fe93685 100644
--- a/frontend/test/components/chrome.test.ts
+++ b/frontend/test/components/chrome.test.ts
@@ -10,6 +10,7 @@ import '@components/sidebar/app-sidebar';
import '@components/library-filter/library-filter';
import '@components/library-status-indicator/library-status-indicator';
import { Events } from '../../src/events';
+import { activeViewStore } from '@store/active-view-store';
import { emit, stub, flush, calls, lastArgs } from '@test/support/harness';
import {
fixture,
@@ -49,6 +50,8 @@ describe('', () => {
});
it('marks exactly one item as the current page', async () => {
+ activeViewStore.setView('home', true);
+
const el = await fixture('app-sidebar');
const current = shadowAll(el, 'li button').filter(
@@ -72,17 +75,49 @@ describe('', () => {
expect(seen).toEqual(['artists']);
});
- it('moves aria-current to the clicked destination', async () => {
+ it('moves aria-current with the shell, not with the click', async () => {
+ activeViewStore.setView('home', true);
+
const el = await fixture('app-sidebar');
shadow(el, '[data-testid="nav-genres"]')?.click();
await el.updateComplete;
+ // The click asks; it does not answer. The sidebar used to move its
+ // own highlight optimistically, which is the second opinion #72
+ // removed -- one component deciding where the user is, while the
+ // shell decided separately and `bottom-nav` decided a third way.
+ expect(
+ shadow(el, '[data-testid="nav-genres"]')?.getAttribute('aria-current'),
+ ).toBe('false');
+
+ // What the shell does with that event, in one line.
+ activeViewStore.setView('genres', true);
+ await update(el, {});
+
expect(
shadow(el, '[data-testid="nav-genres"]')?.getAttribute('aria-current'),
).toBe('page');
});
+ it('follows the back path, which dispatches no navigate event', async () => {
+ activeViewStore.setView('albums', true);
+
+ const el = await fixture('app-sidebar');
+
+ // `popstate` replays an entry through `handleNavigate` directly, so
+ // there is no `navigate` event to hear -- which is why the sidebar
+ // stayed on the view the user had just left (#72).
+ activeViewStore.setView('tracks', true);
+ await update(el, {});
+
+ expect(
+ shadowAll(el, 'li button')
+ .filter((item) => item.getAttribute('aria-current') === 'page')
+ .map((item) => item.getAttribute('data-testid')),
+ ).toEqual(['nav-tracks']);
+ });
+
it('looks the way it did last time', async () => {
const el = await fixture('app-sidebar');
diff --git a/frontend/test/stores/view-stores.test.ts b/frontend/test/stores/view-stores.test.ts
index b189459..84a1905 100644
--- a/frontend/test/stores/view-stores.test.ts
+++ b/frontend/test/stores/view-stores.test.ts
@@ -1,11 +1,13 @@
/**
- * The three small stores behind view chrome: the global search term,
- * the track list's column set, and the explore cache that keeps detail
- * pages from re-fetching what a search already returned.
+ * The small stores behind view chrome: the global search term, the
+ * active view both navs highlight, the track list's column set, and
+ * the explore cache that keeps detail pages from re-fetching what a
+ * search already returned.
*/
import { describe, expect, it, beforeEach } from 'vitest';
import { searchStore } from '@store/search-store';
+import { activeViewStore } from '@store/active-view-store';
import { trackListStore } from '@store/tracklist-store';
import { exploreCache, ARTIST_IMAGE_CACHE_LIMIT } from '@store/explore-cache';
import { Events } from '../../src/events';
@@ -80,6 +82,57 @@ describe('search store', () => {
});
});
+describe('active view store', () => {
+ beforeEach(() => {
+ activeViewStore.setView('home', true);
+ });
+
+ it('holds the primary view the shell navigated to', () => {
+ activeViewStore.setView('albums', true);
+
+ expect(activeViewStore.get()).toBe('albums');
+ expect(activeViewStore.isActive('albums')).toBe(true);
+ expect(activeViewStore.isActive('tracks')).toBe(false);
+ });
+
+ it('leaves the primary view lit while a detail view is open', () => {
+ activeViewStore.setView('albums', true);
+ activeViewStore.setView('explore-album-details', false);
+
+ // #72's third finding, made deliberate: a detail view is not a
+ // destination in either nav, and the tab it was opened from is
+ // where the user still is. `app-sidebar` did this by accident (it
+ // guarded on its own item list) and `bottom-nav` did not do it at
+ // all, which is why one looked right and the other looked broken.
+ expect(activeViewStore.get()).toBe('albums');
+ });
+
+ it('does not notify when the view is unchanged', () => {
+ let notifications = 0;
+ const off = activeViewStore.subscribe(() => {
+ notifications += 1;
+ });
+
+ activeViewStore.setView('albums', true);
+ activeViewStore.setView('albums', true);
+ activeViewStore.setView('explore-album-details', false);
+ off();
+
+ expect(notifications).toBe(1);
+ });
+
+ it('lights nothing for a view with no name', () => {
+ // The store starts empty rather than defaulting to a view, because
+ // a written-down default is right only while `GetDefaultPage()`
+ // agrees with it. That is only safe if the empty value matches
+ // nothing: `isActive` compares strings, and a component asking
+ // about an id it does not have must not light up.
+ activeViewStore.setView('', true);
+
+ expect(activeViewStore.isActive('')).toBe(false);
+ });
+});
+
describe('track list store', () => {
it('starts from the default column set', () => {
expect(trackListStore.getState().columnIds.length).toBeGreaterThan(0);