yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

Settings lists two columns both called "Track Name", and one of them may be unselectable #197

New Issue
Open
opened 2026-08-22 02:49:19 +00:00 by logan · 2 comments
logan commented 2026-08-22 02:49:19 +00:00
Collaborator
Copy Link
Copy Source

Report

Settings -> Track List Columns lists two rows both called "Track
Name", one enabled and one not, with no way to tell which is which.
Seen on the reference device (TLP301, 424x439) and it is not
phone-specific -- the list is the same at every width.

They are titleArtist and trackName in
frontend/src/components/track-list/columns.ts, and both declare
label: 'Track Name'.

Findings

  • The accessible names collide too, which is worse than the visual
    duplicate: config-page renders
    aria-label="Show the ${columnLabel} column", so a screen reader
    hears "Show the Track Name column" twice in one list with nothing
    distinguishing them. That is a11y.32's complaint -- a name that
    exists and identifies nothing -- in a list that was fixed for exactly
    that in the settings-names pass.
  • The collision is deliberate at one end and accidental at the
    other.     titleArtist carries a comment explaining its label: it is
    named for what it sorts by, because the page header's sort list is
    built from the configured columns and the phone has no column
    headers, so "Track Name" is the right word in the sort dropdown. The
    configurator is the second reader of that same string, and nothing
    reconciled them.
  • The real question is whether titleArtist belongs in the
    configurator at all.     It is the phone's stacked column
    (PHONE_COLUMN_IDS), and CLAUDE.md is explicit that a phone's
    widths are "neither loaded nor saved" and that what is drawn and
    what can be sorted are different questions. A user on a desktop
    cannot make this column appear by ticking it -- the phone's column
    set is chosen by width, not by this list -- so the row is at best
    inert and at worst a control that does nothing.
  • Found while raising this list's controls to the touch floor (#186),
    which made it more visible rather than causing it: the column's name
    is now the click target, so the ambiguity is on the thing you tap.

Direction

Two candidate answers and they are not the same size.

The cheap one is a second string: a configLabel (or a
sortLabel) so the sort dropdown keeps "Track Name" and the
configurator says "Track and artist". That is one field and one
fallback, and it leaves the inert-row question open.

The honest one is to ask whether the configurator should list a column
the user cannot select. If titleArtist is phone-only and chosen by
width, it should be filtered out of renderColumnsSection the way
PHONE_COLUMN_IDS is derived rather than declared -- and then there
is no duplicate to rename, because there is only one row called
"Track Name".

Whichever wins, COLUMN_DEFS should not be able to carry two
identical labels silently: a test over the table (the shape
icon-language.test.ts uses) costs nothing and is what would have
caught this when the second one was added.

**Report** Settings -> Track List Columns lists **two rows both called "Track Name"**, one enabled and one not, with no way to tell which is which. Seen on the reference device (TLP301, 424x439) and it is not phone-specific -- the list is the same at every width. They are `titleArtist` and `trackName` in `frontend/src/components/track-list/columns.ts`, and both declare `label: 'Track Name'`. **Findings** - **The accessible names collide too**, which is worse than the visual duplicate: `config-page` renders `aria-label="Show the ${columnLabel} column"`, so a screen reader hears "Show the Track Name column" twice in one list with nothing distinguishing them. That is `a11y.32`'s complaint -- a name that exists and identifies nothing -- in a list that was fixed for exactly that in the settings-names pass. - **The collision is deliberate at one end and accidental at the other.** `titleArtist` carries a comment explaining its label: it is named for what it *sorts* by, because the page header's sort list is built from the configured columns and the phone has no column headers, so "Track Name" is the right word in the sort dropdown. The configurator is the second reader of that same string, and nothing reconciled them. - **The real question is whether `titleArtist` belongs in the configurator at all.** It is the phone's stacked column (`PHONE_COLUMN_IDS`), and CLAUDE.md is explicit that a phone's widths are "neither loaded nor saved" and that what is drawn and what can be sorted are different questions. A user on a desktop cannot make this column appear by ticking it -- the phone's column set is chosen by width, not by this list -- so the row is at best inert and at worst a control that does nothing. - Found while raising this list's controls to the touch floor (#186), which made it more visible rather than causing it: the column's name is now the click target, so the ambiguity is on the thing you tap. **Direction** Two candidate answers and they are not the same size. The cheap one is a second string: a `configLabel` (or a `sortLabel`) so the sort dropdown keeps "Track Name" and the configurator says "Track and artist". That is one field and one fallback, and it leaves the inert-row question open. The honest one is to ask whether the configurator should list a column the user cannot select. If `titleArtist` is phone-only and chosen by width, it should be filtered out of `renderColumnsSection` the way `PHONE_COLUMN_IDS` is derived rather than declared -- and then there is no duplicate to rename, because there is only one row called "Track Name". Whichever wins, `COLUMN_DEFS` should not be able to carry two identical labels silently: a test over the table (the shape `icon-language.test.ts` uses) costs nothing and is what would have caught this when the second one was added.
logan self-assigned this 2026-08-30 08:36:51 +00:00
logan added the
Status
In Progress
 label 2026-08-30 08:36:52 +00:00
logan commented 2026-08-30 08:36:52 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up on fix/197-duplicate-column-label.

The open question in the Direction is settled by the code, and the
answer is the "honest" one — it is forced rather than preferred.

titleArtist is not a column the backend will accept.
backend/tracklist/config.go's AllColumnIDs does not list it, so
isValidColumnID("titleArtist") is false and Config.Validate()
returns unknown track-list column ID: "titleArtist". Measured:

validate = unknown track-list column ID: "titleArtist"
albumArt valid = true
titleArtist valid = false

The Settings row is therefore not "at best inert": ticking it sends a
column set the backend rejects, config-page swallows the rejection
into console.error('Failed to update columns:', err), and the tick
reverts on the next render with nothing said. So there is no
configLabel decision to make — the cheap answer would give a distinct
name to a control that cannot work.

And it is worse than a control that does nothing. SetTrackListColumns
assigns c.TrackList.Columns = columns before validating, so the
rejected list stays in memory, and Save() re-validates the whole
config:

SetTrackListColumns = invalid track-list columns: unknown track-list column ID: "titleArtist"
in-memory columns after failure = [{trackName} {artistName} {album} {trackLength} {titleArtist}]
later, unrelated SetThemeAccentColor = could not save config: invalid config: ... unknown track-list column ID: "titleArtist"
later, plain Save = invalid config: ... unknown track-list column ID: "titleArtist"

One tick of the phantom row and no setting can be saved for the rest
of the session, silently. That second defect is reachable from any
invalid input rather than from this row alone, so it is being filed
separately rather than folded into this diff.

Approach here: the configurator stops offering a column the backend
cannot accept — ALL_COLUMN_IDS (Object.keys(COLUMN_DEFS)) becomes a
configurable set — and a test over the table pins both halves: no two
offered columns may share a label, and every offered id must be one Go
lists.

Picking this up on `fix/197-duplicate-column-label`. **The open question in the Direction is settled by the code, and the answer is the "honest" one — it is forced rather than preferred.** `titleArtist` is not a column the backend will accept. `backend/tracklist/config.go`'s `AllColumnIDs` does not list it, so `isValidColumnID("titleArtist")` is false and `Config.Validate()` returns `unknown track-list column ID: "titleArtist"`. Measured: ``` validate = unknown track-list column ID: "titleArtist" albumArt valid = true titleArtist valid = false ``` The Settings row is therefore not "at best inert": ticking it sends a column set the backend rejects, `config-page` swallows the rejection into `console.error('Failed to update columns:', err)`, and the tick reverts on the next render with nothing said. So there is no `configLabel` decision to make — the cheap answer would give a distinct name to a control that cannot work. **And it is worse than a control that does nothing.** `SetTrackListColumns` assigns `c.TrackList.Columns = columns` *before* validating, so the rejected list stays in memory, and `Save()` re-validates the whole config: ``` SetTrackListColumns = invalid track-list columns: unknown track-list column ID: "titleArtist" in-memory columns after failure = [{trackName} {artistName} {album} {trackLength} {titleArtist}] later, unrelated SetThemeAccentColor = could not save config: invalid config: ... unknown track-list column ID: "titleArtist" later, plain Save = invalid config: ... unknown track-list column ID: "titleArtist" ``` One tick of the phantom row and **no setting can be saved for the rest of the session**, silently. That second defect is reachable from any invalid input rather than from this row alone, so it is being filed separately rather than folded into this diff. **Approach here:** the configurator stops offering a column the backend cannot accept — `ALL_COLUMN_IDS` (`Object.keys(COLUMN_DEFS)`) becomes a configurable set — and a test over the table pins both halves: no two offered columns may share a label, and every offered id must be one Go lists.
logan commented 2026-08-30 09:02:08 +00:00
Author
Collaborator
Copy Link
Copy Source

PR: #232 — CI green on
both jobs (check 22713, e2e 22715), first attempt.

The configurator now lists CONFIGURABLE_COLUMN_IDS rather than
Object.keys(COLUMN_DEFS), so there is one row called "Track Name" and
no configLabel to invent — the second row could never have been
ticked. frontend/test/components/settings-column-list.test.ts is the
table sweep this issue asked for, and it reads Go's AllColumnIDs
out of the source rather than writing it down a third time; planted
against the old shape, all four assertions fail with the defect's own
numbers.

The second half of what was found here is #231 — SetTrackListColumns
assigns before it validates, so one tick of the phantom row left the
in-memory config invalid and no setting saved for the rest of the
session. Filed rather than folded in: it is reachable from any invalid
input, not from this row.

Leaving Status/In Progress on and not merging.

PR: https://git.ljones.me/yonlu/yellowjacket/pulls/232 — CI green on both jobs (`check` 22713, `e2e` 22715), first attempt. The configurator now lists `CONFIGURABLE_COLUMN_IDS` rather than `Object.keys(COLUMN_DEFS)`, so there is one row called "Track Name" and no `configLabel` to invent — the second row could never have been ticked. `frontend/test/components/settings-column-list.test.ts` is the table sweep this issue asked for, and it reads Go's `AllColumnIDs` **out of the source** rather than writing it down a third time; planted against the old shape, all four assertions fail with the defect's own numbers. The second half of what was found here is **#231** — `SetTrackListColumns` assigns before it validates, so one tick of the phantom row left the in-memory config invalid and *no setting saved for the rest of the session*. Filed rather than folded in: it is reachable from any invalid input, not from this row. Leaving `Status/In Progress` on and not merging.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Library-UI Area/Settings Kind/Bug Platform/Android Platform/Desktop
Priority
Low
4
Reviewed
Confirmed
1
Status
In Progress
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#197