Merge remote-tracking branch 'origin/main' into fix/197-duplicate-column-label

default

CLAUDE.md

@@ -3286,6 +3286,27 @@ its own duplicates apart) — and changing either is invisible against an
 existing `YJ_HOME`, whose `config.toml` already holds the old list, so
 `make sandbox-seed NAME=default` before believing the app.
 
+**And the *valid* columns are declared twice too, which is the pair
+that drifted.** `tracklist.AllColumnIDs` is what the backend accepts;
+`COLUMN_DEFS` is what the frontend knows how to draw, and they are not
+the same set — `titleArtist` is a definition and not a choice, since it
+is the phone's stacked column and is picked by width in
+`PHONE_COLUMN_IDS`. Settings built its list from `Object.keys(
+COLUMN_DEFS)` and so offered it: **two rows both called "Track Name"**
+(#197), the second unselectable, because ticking it sends a column set
+Go rejects with `unknown track-list column ID` and `config-page`
+swallows that into a `console.error`. `CONFIGURABLE_COLUMN_IDS` is what
+the configurator reads now, derived from a `configurable` flag on the
+definition, and `settings-column-list.test.ts` reads Go's own list out
+of the source rather than writing it down a third time — the rule being
+about every column, so checking one checks nothing.
+
+One thing it does **not** fix, because it is reachable from any invalid
+input rather than from that row: `SetTrackListColumns` assigns before it
+validates, so a rejected list stays in memory and `Save()` validates the
+whole config — one tick and **no setting saves for the rest of the
+session**, silently. That is #231.
+
 **Event-driven communication**: Backend emits events via Wails runtime; frontend stores subscribe to them. Event names are constants in `backend/events/`.
 
 `frontend/src/events.ts` is **generated** from `backend/events/events.go`

backend/download/provider_ytdlp_test.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"syscall"
 	"testing"
 )
 
@@ -17,6 +18,21 @@ import (
 
 // stubYtDlp writes an executable script that echoes the given stdout
 // and returns it as a provider config binary path.
+//
+// The write is held under syscall.ForkLock, and that is not tidiness:
+// the kernel refuses to exec a file that is open for writing anywhere
+// in the process, and these tests are parallel, so a *sibling* test's
+// fork can duplicate this descriptor in the moment it is open and
+// carry it past our close — the exec a moment later then fails with
+// ETXTBSY, "text file busy".  That is #146, seen once in CI and once
+// locally, on trees containing no Go at all.  Closing sooner is not
+// available (os.WriteFile has already closed the file before anything
+// execs it) and O_CLOEXEC does not help, because the window is between
+// another goroutine's fork and its own exec.  ForkLock is the lock
+// syscall.forkExec takes across that fork, so holding it here means no
+// child can exist while the descriptor does.  Measured on this helper
+// under 12 concurrent writers: 176-189 of 2400 execs refused without
+// it, 0 of 2400 with it.
 func stubYtDlp(t *testing.T, script string) string {
 	t.Helper()
 
@@ -26,9 +42,11 @@ func stubYtDlp(t *testing.T, script string) string {
 
 	path := filepath.Join(t.TempDir(), "yt-dlp")
 
-	if err := os.WriteFile(
-		path, []byte("#!/bin/sh\n"+script), 0o700,
-	); err != nil {
+	syscall.ForkLock.Lock()
+	err := os.WriteFile(path, []byte("#!/bin/sh\n"+script), 0o700)
+	syscall.ForkLock.Unlock()
+
+	if err != nil {
 		t.Fatalf("write stub: %v", err)
 	}
 

frontend/src/components/config-page/config-page.ts

@@ -51,7 +51,7 @@ import type { BackgroundShade } from '@store/theme-store';
 import type { IconStyle } from '@store/favorites-store';
 import {
     COLUMN_DEFS,
-    ALL_COLUMN_IDS,
+    CONFIGURABLE_COLUMN_IDS,
 } from '@components/track-list/columns';
 
 import './config-field';
@@ -1640,7 +1640,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
             ...this.trackListCtrl.columnIds,
         ];
 
-        const disabledIds = ALL_COLUMN_IDS.filter(
+        const disabledIds = CONFIGURABLE_COLUMN_IDS.filter(
             (id) => !enabledIds.includes(id),
         );
 

frontend/src/components/first-run-wizard/first-run-wizard.ts

@@ -2,12 +2,14 @@ import { LitElement, html, css, nothing } from 'lit';
 import { customElement, state, query } from 'lit/decorators.js';
 import '@awesome.me/webawesome/dist/components/dialog/dialog.js';
 import '@awesome.me/webawesome/dist/components/icon/icon.js';
+import { EventsOn } from '@runtime/runtime';
 import {
     AddLibrary,
     GetAllLibrariesWithTrackCounts,
 } from '@go/library/library.js';
 import { describeError, explainError } from '@utils/describe-error';
 import { nameDialogsIn } from '@utils/name-dialog';
+import { Events } from '../../events';
 import { pickDirectory } from '../../utils/pick-directory';
 
 /**
@@ -19,6 +21,13 @@ import { pickDirectory } from '../../utils/pick-directory';
  * prompting the user to pick their music folder, registers it through the
  * library CRUD API, and dismisses itself.  AddLibrary emits LibraryAdded
  * and kicks off the initial scan automatically.
+ *
+ * **The dismissal follows the library existing, not the button being
+ * pressed.** `AddLibrary` emits `LibraryAdded` whoever calls it, so the
+ * wizard waits on the state it exists to wait for rather than on a step
+ * in its own flow — a library arriving by any other route (Settings, a
+ * direct call) leaves a full-screen modal up otherwise, intercepting
+ * every pointer event.
  */
 @customElement('first-run-wizard')
 export class FirstRunWizard extends LitElement {
@@ -37,9 +46,18 @@ export class FirstRunWizard extends LitElement {
     /** Error message from a failed pick/save, if any. */
     @state() private errorMessage = '';
 
+    /** Unsubscribe from LibraryAdded, while this element is connected. */
+    private cancelLibraryAdded?: () => void;
+
     override async connectedCallback(): Promise<void> {
         super.connectedCallback();
 
+        // Subscribed before the read below, so a library arriving while
+        // that call is in flight is not answered with a stale empty list.
+        this.cancelLibraryAdded = EventsOn(Events.LibraryAdded, () => {
+            this.dismiss();
+        });
+
         try {
             const existing = await GetAllLibrariesWithTrackCounts();
 
@@ -54,6 +72,8 @@ export class FirstRunWizard extends LitElement {
             return;
         }
 
+        if (this.finished) return;
+
         this.active = true;
 
         await this.updateComplete;
@@ -61,6 +81,13 @@ export class FirstRunWizard extends LitElement {
         if (this.dialog) this.dialog.open = true;
     }
 
+    override disconnectedCallback(): void {
+        this.cancelLibraryAdded?.();
+        this.cancelLibraryAdded = undefined;
+
+        super.disconnectedCallback();
+    }
+
     static override styles = css`
         wa-dialog {
             --width: 480px;
@@ -239,6 +266,20 @@ export class FirstRunWizard extends LitElement {
         if (!this.finished) e.preventDefault();
     };
 
+    /**
+     * Close, and stay closed: a library exists, so setup is over.
+     *
+     * `finished` is set first, or `preventClose` cancels the hide this
+     * asks for.
+     */
+    private dismiss(): void {
+        this.finished = true;
+
+        if (this.dialog) this.dialog.open = false;
+
+        this.active = false;
+    }
+
     private handleChoose = async (): Promise<void> => {
         this.errorMessage = '';
 
@@ -264,11 +305,7 @@ export class FirstRunWizard extends LitElement {
         try {
             await AddLibrary(this.selectedDirectory);
 
-            this.finished = true;
-
-            if (this.dialog) this.dialog.open = false;
-
-            this.active = false;
+            this.dismiss();
         } catch (err) {
             this.errorMessage = explainError(
                 err,

frontend/src/components/track-list/columns.ts

@@ -32,6 +32,18 @@ export interface ColumnDef {
     id: string;
     /** Human-readable header label. */
     label: string;
+    /**
+     * Whether Settings may offer this column. Defaults to true.
+     *
+     * A definition is not the same thing as a *choice*. `titleArtist`
+     * is the phone's stacked column, picked by width in
+     * `PHONE_COLUMN_IDS`, and `tracklist.AllColumnIDs` in Go does not
+     * list it — so a tick in the configurator sends a column set the
+     * backend rejects with `unknown track-list column ID`, the tick
+     * reverts on the next render, and the only trace is a
+     * `console.error` (#197).
+     */
+    configurable?: boolean;
     /** Extracts the display value from a track. */
     accessor: (track: library.Track) => string;
     /** Default CSS width (used when no saved width exists). */
@@ -98,11 +110,16 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
     },
     titleArtist: {
         id: 'titleArtist',
-        // Named for what it sorts by, since that is the only place the
-        // label is user-visible: the phone has no column headers, and
-        // the page header's sort list is built from the *configured*
-        // columns rather than the drawn ones.
+        // Named for what it sorts by. That label is drawn nowhere
+        // today: the phone has no column headers, and the page header's
+        // sort list is built from the *configured* columns, which this
+        // one can never be — see `configurable` below.
         label: 'Track Name',
+        // Chosen by width, never by the user, and rejected by the
+        // backend if it ever were. #197: Settings listed it anyway, so
+        // there were two rows called "Track Name" and the second one
+        // could not be selected.
+        configurable: false,
         accessor: (t) => t.TrackName,
         defaultWidth: '1fr',
         comparator: (a, b) => compareStr(a.TrackName, b.TrackName),
@@ -266,10 +283,15 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
 };
 
 /**
- * All column IDs in default display order.
- * Used by the settings UI to list available columns.
+ * The column IDs Settings may offer, in default display order.
+ *
+ * Not every definition is one: a column the user cannot choose has no
+ * row in the configurator, because a checkbox that cannot change
+ * anything is worse than an absent one — see `ColumnDef.configurable`.
  */
-export const ALL_COLUMN_IDS: string[] = Object.keys(COLUMN_DEFS);
+export const CONFIGURABLE_COLUMN_IDS: string[] = Object.keys(
+    COLUMN_DEFS,
+).filter((id) => COLUMN_DEFS[id]?.configurable !== false);
 
 /**
  * Column IDs that are always searched regardless of visibility.

frontend/test/components/first-run-wizard.test.ts

@@ -0,0 +1,123 @@
+/**
+ * #175: the first-run wizard's dismissal follows the library existing,
+ * not its own button being pressed.
+ *
+ * The wizard is a modal that blocks every pointer event, so a library
+ * arriving by another route — Settings, a direct call — used to leave
+ * it up over an app that was already set up. `LibraryAdded` is emitted
+ * by `AddLibrary` whoever calls it, which is what makes one
+ * subscription the whole fix.
+ */
+import { beforeEach, describe, expect, it } from 'vitest';
+
+import { Events } from '../../src/events';
+import { emit, stub } from '../support/harness';
+import { fixture, shadow, shadowAll } from '../support/render';
+import { wails } from '../support/wails-fake';
+
+import '@components/first-run-wizard/first-run-wizard';
+
+import type { FirstRunWizard } from '@components/first-run-wizard/first-run-wizard';
+
+/** A library row, as `GetAllLibrariesWithTrackCounts` returns one. */
+const aLibrary = {
+    id: 1,
+    name: 'Music',
+    path: '/home/logan/Music',
+    trackCount: 9,
+};
+
+/** Mount the wizard on a fresh install: no libraries yet. */
+async function wizardOnAFreshInstall(): Promise<FirstRunWizard> {
+    stub('library.Library.GetAllLibrariesWithTrackCounts', []);
+
+    return fixture<FirstRunWizard>('first-run-wizard');
+}
+
+/** Whether the wizard is rendering its modal at all. */
+function isShowing(el: FirstRunWizard): boolean {
+    return shadow(el, 'wa-dialog') !== null;
+}
+
+beforeEach(() => {
+    stub('library.Library.AddLibrary', aLibrary);
+});
+
+describe('first-run-wizard', () => {
+    it('shows on a fresh install and stays up until a library exists', async () => {
+        const el = await wizardOnAFreshInstall();
+
+        expect(isShowing(el)).toBe(true);
+    });
+
+    it('stays hidden when a library is already configured', async () => {
+        stub('library.Library.GetAllLibrariesWithTrackCounts', [aLibrary]);
+
+        const el = await fixture<FirstRunWizard>('first-run-wizard');
+
+        expect(isShowing(el)).toBe(false);
+    });
+
+    it('dismisses when a library appears by another route', async () => {
+        const el = await wizardOnAFreshInstall();
+
+        expect(isShowing(el)).toBe(true);
+
+        emit(Events.LibraryAdded, aLibrary);
+        await el.updateComplete;
+
+        expect(isShowing(el)).toBe(false);
+    });
+
+    it('does not raise itself when a library arrives while it is asking', async () => {
+        // The read is still in flight when the event lands, so its
+        // answer — an empty list — is stale by the time it returns.
+        let answer: (libraries: unknown[]) => void = () => {};
+
+        stub(
+            'library.Library.GetAllLibrariesWithTrackCounts',
+            () =>
+                new Promise((resolve) => {
+                    answer = resolve;
+                }),
+        );
+
+        const el = await fixture<FirstRunWizard>('first-run-wizard');
+
+        emit(Events.LibraryAdded, aLibrary);
+        answer([]);
+
+        await el.updateComplete;
+        await new Promise((r) => setTimeout(r, 0));
+        await el.updateComplete;
+
+        expect(isShowing(el)).toBe(false);
+    });
+
+    it('still dismisses through its own Get Started button', async () => {
+        stub('frontendutil.FrontendUtil.HasNativeDirectoryPicker', true);
+        stub('frontendutil.FrontendUtil.DirectoryPicker', '/home/logan/Music');
+
+        const { resetDirectoryPickerCache } = await import(
+            '@utils/pick-directory'
+        );
+
+        resetDirectoryPickerCache();
+
+        const el = await wizardOnAFreshInstall();
+        const [choose, finish] = shadowAll<HTMLButtonElement>(el, '.btn');
+
+        choose?.click();
+        await new Promise((r) => setTimeout(r, 0));
+        await el.updateComplete;
+
+        finish?.click();
+        await new Promise((r) => setTimeout(r, 0));
+        await el.updateComplete;
+
+        expect(
+            wails.calls.filter((c) => c.path === 'library.Library.AddLibrary'),
+        ).toHaveLength(1);
+        expect(isShowing(el)).toBe(false);
+    });
+});

frontend/test/components/settings-column-list.test.ts

@@ -0,0 +1,190 @@
+/**
+ * Settings offers the columns the backend will accept, and no others.
+ *
+ * The list is built from `COLUMN_DEFS`, which is the *drawing* table:
+ * every definition the track list knows how to render, including
+ * `titleArtist` — the phone's stacked column, chosen by width in
+ * `PHONE_COLUMN_IDS` and never by a person. `tracklist.AllColumnIDs` in
+ * Go does not list that id, so the configurator offered a nineteenth
+ * row that could not be ticked:
+ *
+ * ```
+ * validate = unknown track-list column ID: "titleArtist"
+ * titleArtist valid = false
+ * ```
+ *
+ * What a user saw was **two rows both called "Track Name"** (#197), one
+ * of which did nothing — and a screen reader heard "Show the Track Name
+ * column" twice with nothing to tell them apart, which is `a11y.32`'s
+ * complaint inside the list that was fixed for exactly that.
+ *
+ * It is worse than an inert control, which is why the duplicate name
+ * was not the thing to fix. `SetTrackListColumns` assigns before it
+ * validates, so a rejected list stays in memory and `Save()` validates
+ * the whole config:
+ *
+ * ```
+ * later, unrelated SetThemeAccentColor = could not save config: invalid
+ *   config: ... unknown track-list column ID: "titleArtist"
+ * ```
+ *
+ * — one tick and no setting saves for the rest of the session. That
+ * half is filed separately; this file keeps the row from being offered.
+ *
+ * The last test is the one that would have caught it when the column
+ * was added: the two lists are in different languages, so nothing but a
+ * sweep can hold them together.
+ */
+import { beforeEach, describe, expect, it } from 'vitest';
+
+import '@components/config-page/config-page';
+
+import {
+  COLUMN_DEFS,
+  CONFIGURABLE_COLUMN_IDS,
+} from '@components/track-list/columns';
+import { flush, stub } from '@test/support/harness';
+import { fixture, shadowAll } from '@test/support/render';
+
+/** Go's own list of column ids, as text. */
+const GO_CONFIG = Object.values(
+  import.meta.glob<string>('../../../backend/tracklist/config.go', {
+    eager: true,
+    query: '?raw',
+    import: 'default',
+  }),
+)[0];
+
+/**
+ * The ids `tracklist.AllColumnIDs` actually contains.
+ *
+ * Read out of the source rather than written down here, because a
+ * third copy of this list is a third thing to forget — which is the
+ * defect, one copy earlier.
+ */
+function goColumnIDs(source: string): string[] {
+  const constants = new Map<string, string>();
+  const constBlock = /const \(([\s\S]*?)\n\)/.exec(source)?.[1] ?? '';
+
+  for (const [, name, id] of constBlock.matchAll(
+    /(\w+)\s+ColumnID\s*=\s*"([^"]+)"/g,
+  )) {
+    constants.set(name!, id!);
+  }
+
+  const listBlock =
+    /var AllColumnIDs = \[\]ColumnID\{([\s\S]*?)\n\}/.exec(source)?.[1] ?? '';
+
+  return [...listBlock.matchAll(/(\w+),/g)]
+    .map(([, name]) => constants.get(name!))
+    .filter((id): id is string => id !== undefined);
+}
+
+/**
+ * The column rows, and only those.
+ *
+ * Settings’ view-visibility list (#25) is drawn with the same two
+ * classes, so a bare `.column-label` sweeps 29 rows across two
+ * sections — and "Albums" the destination sitting beside "Album" the
+ * column is not the fault this file is about. The `for`/`id` prefix is
+ * what tells them apart.
+ */
+const COLUMN_ROW_LABEL = 'label.column-label[for^="column-"]';
+const COLUMN_ROW_BOX = 'input.column-toggle[id^="column-"]';
+
+/** The rows the configurator draws, by their visible name. */
+async function columnRowNames(): Promise<string[]> {
+  const page = await fixture('config-page');
+
+  await flush();
+  await page.updateComplete;
+
+  // Every section renders collapsed, and a collapsed body is `hidden`.
+  for (const section of shadowAll<HTMLElement>(page, 'config-section')) {
+    section.shadowRoot
+      ?.querySelector<HTMLButtonElement>('button[aria-expanded="false"]')
+      ?.click();
+  }
+
+  await flush();
+  await page.updateComplete;
+
+  return shadowAll<HTMLElement>(page, COLUMN_ROW_LABEL).map(
+    (label) => label.textContent?.trim() ?? '',
+  );
+}
+
+describe('the Settings column list', () => {
+  beforeEach(() => {
+    for (const path of [
+      'library.Library.GetAllLibrariesWithTrackCounts',
+      'jobs.Service.GetJobs',
+      'download.Service.ListProviders',
+      'download.Service.ProviderKinds',
+    ]) {
+      stub(path, []);
+    }
+
+    stub('config.Config.GetShortcuts', {});
+    stub('config.Config.GetDownloadPreferences', {});
+    stub('config.Config.GetThemeAccentColor', '#ffd43b');
+    stub('config.Config.GetThemeBackgroundShade', 'dark');
+  });
+
+  it('names each row once', async () => {
+    const names = await columnRowNames();
+
+    // A sweep over nothing passes.
+    expect(names.length, 'the page draws column rows').toBeGreaterThan(5);
+
+    const seen = new Set<string>();
+    const duplicated = names.filter((name) => !seen.add(name));
+
+    expect(duplicated).toEqual([]);
+    expect(names.filter((n) => n === 'Track Name')).toHaveLength(1);
+  });
+
+  it('gives each checkbox a name that identifies it', async () => {
+    // The visible half above is what was reported; this is the half a
+    // screen reader gets, and it is the one `config-page` computes
+    // from the same string.
+    const page = await fixture('config-page');
+
+    await flush();
+    await page.updateComplete;
+
+    const labels = shadowAll<HTMLInputElement>(page, COLUMN_ROW_BOX).map(
+      (box) => box.getAttribute('aria-label') ?? '',
+    );
+
+    expect(labels.length, 'the page draws column checkboxes').toBeGreaterThan(5);
+    expect(new Set(labels).size).toBe(labels.length);
+  });
+});
+
+describe('the column table', () => {
+  it('offers no column the backend would reject', async () => {
+    const accepted = goColumnIDs(GO_CONFIG ?? '');
+
+    // Two non-vacuity guards: a glob that stopped matching, and a
+    // parse that stopped finding the list it names.
+    expect(GO_CONFIG, 'backend/tracklist/config.go is readable').toBeTruthy();
+    expect(accepted.length, 'AllColumnIDs was parsed').toBeGreaterThan(10);
+
+    expect(
+      CONFIGURABLE_COLUMN_IDS.filter((id) => !accepted.includes(id)),
+    ).toEqual([]);
+  });
+
+  it('still knows how to draw every column it offers', async () => {
+    // The filter must not have taken a column *out* of the drawing
+    // table: `configurable` says what Settings may list, not what the
+    // list may render.
+    expect(
+      CONFIGURABLE_COLUMN_IDS.filter((id) => COLUMN_DEFS[id] === undefined),
+    ).toEqual([]);
+    expect(CONFIGURABLE_COLUMN_IDS).not.toContain('titleArtist');
+    expect(COLUMN_DEFS['titleArtist'], 'the phone still has its column')
+      .toBeTruthy();
+  });
+});