diff --git a/CLAUDE.md b/CLAUDE.md index d3b5942..4fd4699 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3275,6 +3275,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` diff --git a/frontend/src/components/config-page/config-page.ts b/frontend/src/components/config-page/config-page.ts index 522ac2c..502c557 100644 --- a/frontend/src/components/config-page/config-page.ts +++ b/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), ); diff --git a/frontend/src/components/track-list/columns.ts b/frontend/src/components/track-list/columns.ts index 059863a..a0f8a38 100644 --- a/frontend/src/components/track-list/columns.ts +++ b/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 = { }, 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 = { }; /** - * 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. diff --git a/frontend/test/components/settings-column-list.test.ts b/frontend/test/components/settings-column-list.test.ts new file mode 100644 index 0000000..ab1531c --- /dev/null +++ b/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('../../../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(); + 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 { + 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(page, 'config-section')) { + section.shadowRoot + ?.querySelector('button[aria-expanded="false"]') + ?.click(); + } + + await flush(); + await page.updateComplete; + + return shadowAll(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(); + 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(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(); + }); +});