Repair the tags on files already recorded: search, a detail pane, and every field the recorder writes

CHANGELOG.md

@@ -84,14 +84,39 @@ phase plan these entries follow.
   an automatic match is wrong often enough that the review step is the feature rather than
   friction in front of it. Files that already carry a title, artist and album are left alone and
   cost no request; a single row can still be looked up deliberately when its tags are complete
-  and wrong. Each file is one compact row showing what would be written, with the fields opening
-  underneath only when asked — a library is a hundred files of which three are wrong, and a row
-  marked **Will change** is how the three are found without opening the other ninety-seven. A
-  **Filter** box narrows the list by title, artist, album or file name for libraries too long to
-  scroll — it changes what is drawn and nothing else, so Save still writes every ticked row it is
-  hiding. Beneath the editable fields, **Also from the match** shows the year, genre and artwork a
-  lookup found: Save writes all three and they appeared nowhere, so a row could announce that it
-  would change while every field on screen matched the file exactly.
+  and wrong. The page is a list beside an editor: each file is one compact row saying what would
+  be written, and picking a row fills a pane on the right with its fields. A library is a hundred
+  files of which three are wrong, and a row marked **Will change** is how the three are found
+  without reading the other ninety-seven. A **Filter** box narrows the list by title, artist,
+  album or file name for libraries too long to scroll — it changes what is drawn and nothing else,
+  so Save still writes every ticked row it is hiding. Every tag Offstream writes while recording
+  can be edited here: title, artist, album artist, album, genre, year, track number, tracks on
+  album, disc and copyright. Three of them were editable at first and the rest were not shown at
+  all, which left a row free to announce that it would change while every field on screen matched
+  the file exactly — and left a tag the recorder writes but the page could not reach as a tag
+  nobody could ever fix. The fields sit in two columns rather than one, because ten stacked
+  fields are 650 DIP inside a 347 DIP pane and seven of them would have opened below the fold.
+  And when the match itself is wrong, **Not the right track?** searches
+  Spotify for whatever you type and lists what it finds, year included, so a remaster can be told
+  from the original; it is pinned to the foot of the pane, so the results arrive where the user is
+  already looking rather than below a fold. Until this there was no way to correct a bad match
+  except to type all three fields by hand and give up the year, genre and artwork that come with a
+  real one — re-fetching could not do it, because the automatic lookup builds its query from the
+  file's own fields and then refuses any result whose artist disagrees with them, which is
+  precisely the case where the file is what is wrong. The list scrolls by pixel rather than by
+  row, because a wheel notch moved three rows and a row here is three lines of text — most of a
+  screen at a time. Picking a row fills its search box from what the row says at that moment, so a
+  search asks for the track as it has been corrected rather than as it was scanned. And a
+  lookup that has no genre for a track no longer offers to remove the one the file has: every
+  provider assigns genre and year unconditionally, which is right where they were written, tagging
+  a recording that starts with nothing, and wrong here, where the track starts as the file's own
+  tags. The same holds for every other tag a lookup can leave empty, and for clearing a box by
+  hand — Save writes no blank over a value, so an emptied field stopped claiming a change it was
+  never going to make. Saving also keeps an artist tag that holds more than one name. ID3v2.3
+  separates artists with a slash, so a file recorded as `AC/DC` is stored as the two values `AC`
+  and `DC`; the page has one artist box, it is filled from the first of them, and writing that
+  box back narrowed the tag to `AC` — the repair page destroying the tag it was opened to
+  repair. An artist nobody has typed over is now written back exactly as it was found.
 - **Looking a track up now falls back from Spotify to Last.fm instead of stopping at whichever
   one the Settings page names.** That setting answers "who tags a recording as it is made", where
   one source keeps a library consistent. Repairing files already on disk is the opposite problem —

CLAUDE.md

@@ -54,6 +54,45 @@ These are load-bearing; violating them breaks the app in ways that are not obvio
 - **Never enable aggressive trimming.** WPF trims poorly and settings/localisation are reflection-driven. Publish self-contained and untrimmed.
 - **`Offstream.Core` must not reference WPF or `System.Windows`.** The predecessor passed the form itself into its watcher and recorder; Offstream uses events or `IProgress<T>` instead.
 - **All conversion goes through ffmpeg.** No NAudio.Lame, no bundled LAME DLLs. This governs *conversion*, and only that. **Retagging a file that already exists goes through TagLib#** (decided 2026-08-29, `Metadata/Library/TagLibTagStore.cs`), because ffmpeg cannot edit a tag in place — the equivalent is remuxing the whole file to change one string, which rewrites audio that had nothing wrong with it. So "ffmpeg writes every textual tag" stays true of the recording pipeline and is false of the Metadata page. Two rules follow from having a second writer: it writes tags and cover art in **one** TagLib# session rather than calling `CoverArtWriter` after itself, which would save the file twice for one edit; and it pins `Id3v2.Tag.DefaultVersion = 3`, because TagLib# defaults to v2.4 while this project ships v2.3 — a retag that silently upgraded the tag would make tags vanish from Windows Explorer and Media Player on files that displayed correctly before Offstream touched them.
+- **A lookup on the Metadata page adds tags; it never takes one away** (decided 2026-08-30). Every
+  provider assigns genre and year unconditionally — right where they were written, tagging a
+  recording that starts with nothing and where the provider is the only source there is, and wrong
+  on this page, where the track starts as the file's own tags. Spotify returns an empty genre list
+  for most of its catalogue since late 2024 and Last.fm returns none for an artist nobody has
+  tagged, so without this a lookup blanks a genre the user curated. Nothing ever reaches the file —
+  `TagLibTagStore` writes a genre only when there is one — but the row reports a change Save will
+  not make, and the editor spells it out as an offer to erase. The rule is written once, in
+  `LibraryLookup.Snapshot` / `KeepWhatWasThere`, and covers every field a lookup can leave empty
+  — not just genre and year. It has **two** call sites and both are load-bearing.
+  `MetadataViewModel.FetchOneAsync` is the one every automatic lookup passes through, so the rule
+  holds there for whichever provider in the chain answers and for any provider added later — it
+  was fixed once inside the Spotify path alone and the Last.fm fallback still had the hole.
+  `SpotifyCatalogEnricher` calls it separately because the manual-match path (**Use this**, via
+  `SpotifyMatchSearch.ApplyAsync`) reaches the enricher without going through `FetchOneAsync`.
+  Deleting either call restores the bug on one of the two paths. Add a new field to the helper,
+  never to a call site — the original bug survived its first fix precisely because the rule
+  existed twice.
+- **The Metadata page edits exactly the tags the recorder writes, and a name is not a
+  comma-separated list** (decided 2026-08-31). The editable set is title, artist, album artist,
+  album, genre, year, track, tracks-on-album, disc and copyright, which is
+  `FFmpegArguments.MetadataArguments` — a tag Offstream writes while recording and cannot repair
+  afterwards is a tag nobody can fix at all. Composer, comment and BPM are in neither list, and
+  leaving them out is what keeps this from becoming a general-purpose tag editor. Two rules for
+  the fields that hold several values. **Genres split on commas; artist and album artist do
+  not** — `Earth, Wind & Fire` is one band, and splitting it is a worse corruption than the
+  multi-value tag the box was added to expose, so those boxes keep the array they were filled
+  from while the text still matches it and take the whole line as one value when it does not.
+  And **an artist tag can hold more than one name behind a single box**: ID3v2.3 separates
+  artists with a slash, so a file recorded as `AC/DC` reads back as the two values `AC` and `DC`.
+  `TagLibTagStore.Write` writes `track.Performers` back verbatim when its first entry still
+  equals `track.Artist` (`KeepsEveryPerformer`), because that first entry is where the box was
+  filled from. Writing `[track.Artist]` unconditionally is what it used to do, and it narrowed
+  the tag on the page whose whole purpose is repairing tags.
+- **`LibraryTrack.HasChanges` means "saving would alter the file", not "these differ"**
+  (decided 2026-08-31). `TagLibTagStore.Write` never writes a blank over a value, so a plain
+  inequality made an emptied box light the **Will change** badge and then save nothing. Any field
+  added to the editor has to go through `Replaces`, which asks whether there is a new value at
+  all before asking whether it differs.
 - **Use `ProcessStartInfo.ArgumentList`, never a command string.** Track metadata comes from Spotify window titles and is untrusted. The argv array prevents argument injection structurally; the old app needed hand-written `CommandLineToArgvW` escaping because .NET Framework lacked `ArgumentList`.
 
 ## Spotify Web API rules
@@ -63,7 +102,7 @@ Set by the user on 2026-08-13 and binding like the constraints above. Most were
 - **Endpoints, parameters and field names come from the [OpenAPI schema](https://developer.spotify.com/reference/web-api/open-api-schema.yaml).** Never guessed. In practice `SpotifyAPI.Web` is the typed SDK and nothing here hand-rolls HTTP; check the schema before adding a call, not after.
 - **[Authorization Code with PKCE](https://developer.spotify.com/documentation/web-api/tutorials/code-pkce-flow) for user data.** Client Credentials only for public non-user data. **Implicit Grant is banned** — it is deprecated. A desktop app is a public client, so there is no Client Secret to protect and none may be introduced.
 - **Redirect URIs are HTTPS**, except `http://127.0.0.1` for local development. Never bare `localhost` (Spotify rejects it), never wildcards. `SpotifyAuthOptions.DefaultRedirectUri` is the one in use and a test pins its shape.
-- **Request the minimum [scopes](https://developer.spotify.com/documentation/web-api/concepts/scopes) the shipped features need, never a scope for a feature that might arrive later.** Offstream makes five calls, and only two of them cost a scope: `/me/player/currently-playing`, which needs `user-read-currently-playing`; `/me`, which puts the signed-in account's name on the Settings page and needs `user-read-private`; and `/albums/{id}`, `/artists/{id}` and `/search`, which are public catalogue data and need no user scope. Two scopes, and `SpotifyAuthOptionsTests` fails the build if the list grows. **The count is not the thing to guard — the scope list is.** This line said "exactly three calls" while the code already made four, because `/artists/{id}` was added for genre and nobody updated the prose; `/search` joined them for the Metadata page on 2026-08-29 and added no scope either. Adding a call to an endpoint that needs no user scope is ordinary work. Adding one that needs a scope is the thing that has to be argued for. `user-read-email` would identify an account unambiguously and is deliberately **not** requested: Spotify removed the `email` field in its late-2024 cull, so the permission now covers data the endpoint no longer returns. `SpotifyAuthOptions.DefaultScopes` carries the full reasoning and is the place to change it. A scope requested ahead of its feature is a permission the user grants for nothing, on a consent screen where the spare lines look identical to the load-bearing one.
+- **Request the minimum [scopes](https://developer.spotify.com/documentation/web-api/concepts/scopes) the shipped features need, never a scope for a feature that might arrive later.** Offstream makes six calls, and only two of them cost a scope: `/me/player/currently-playing`, which needs `user-read-currently-playing`; `/me`, which puts the signed-in account's name on the Settings page and needs `user-read-private`; and `/albums/{id}`, `/artists/{id}`, `/search` and `/tracks/{id}`, which are public catalogue data and need no user scope. Two scopes, and `SpotifyAuthOptionsTests` fails the build if the list grows. **The count is not the thing to guard — the scope list is.** This line said "exactly three calls" while the code already made four, because `/artists/{id}` was added for genre and nobody updated the prose; `/search` joined them for the Metadata page on 2026-08-29, and `/tracks/{id}` on 2026-08-30 for the manual match picker, neither adding a scope. Adding a call to an endpoint that needs no user scope is ordinary work. Adding one that needs a scope is the thing that has to be argued for. `user-read-email` would identify an account unambiguously and is deliberately **not** requested: Spotify removed the `email` field in its late-2024 cull, so the permission now covers data the endpoint no longer returns. `SpotifyAuthOptions.DefaultScopes` carries the full reasoning and is the place to change it. A scope requested ahead of its feature is a permission the user grants for nothing, on a consent screen where the spare lines look identical to the load-bearing one.
 - **Tokens are stored protected, refreshed, and abandoned when dead.** DPAPI covers the refresh token; the access token never reaches disk. Spotify rotates the refresh token on every renewal, so the replacement must be persisted or a long-running install silently stops working. A **401** means the refresh token itself is gone — clear it and put the user back through sign-in rather than retrying it forever. Only 401: treating a rate limit or an outage that way would sign the user out over a transient fault.
 - **On 429, honour `Retry-After` exactly; back off exponentially only where there is no such instruction.** Guessing shorter is what gets an application throttled harder. Never retry in a tight loop. `SpotifyRetryHandler` is the implementation — note that `SpotifyClientConfig.CreateDefault()` attaches **no** retry handler, so a client built without one treats rate limiting as fatal.
 - **Log throttling at `Warning`.** The Record page's activity log shows Information and above, so anything quieter is invisible to everyone who has not gone looking. Transient 5xx stays at `Information` — it usually clears, and promoting it makes the Problems filter too noisy to read.
@@ -94,4 +133,6 @@ Beyond that: ffmpeg argv golden tests, encode-integration tests asserted with ff
 - MVVM via CommunityToolkit.Mvvm source generators; no code-behind logic beyond wiring.
 - Inline validation (`INotifyDataErrorInfo`), not modal dialogs.
 - **A setting's description is a tooltip, not a line under its label** (decided 2026-08-29). The Advanced page has no `ScrollViewer` and the shell's `MinHeight` promises no setting is ever out of reach, so a line of prose per row is height the page does not have — it has been clipped off the bottom three times. Put the sentence in `ToolTip` on the whole row so hovering anywhere finds it, and in `AutomationProperties.HelpText` on the control, because a tooltip is invisible to a screen reader. The text still lives in `Strings.resx` + `.fr.resx` and is still translated. Labels stay inline.
+- **The Metadata page is a list beside an editor, and the editor does not go back inside the row** (decided 2026-08-31). Measured at the shell's minimum window, an expanded row was **539 DIP inside a 347 DIP list viewport** — the editor was 1.55× the height of the container it opened in, so opening one row buried the library and pushed the search results off the bottom. Trimming cannot close that: the page owns only ~130 DIP of the chrome above the list, and spending all of it still leaves a deficit. Two consequences are load-bearing. The list's **selected row is the editor's input**, so it carries a quiet highlight — a tinted background and a narrow accent edge from a replaced `ListBoxItem` template, never the themed container's saturated fill; `MetadataViewModel.ApplyFilter` must capture `SelectedTrack` and restore it after refilling `VisibleTracks`, or the editor empties on every keystroke in the filter box. And the pane has **two regions, not one scroller**: the fields scroll, while the search box and its results stay pinned to its foot, because results that arrive below a fold announce themselves only by shortening a scroll bar. Commands bound from the pane use `RelativeSource AncestorType=UserControl` — there is no `ListBox` ancestor out there.
+- **Before building an expander, check that the thing it opens is smaller than the space it opens into.** That is a measurement available before the layout is written, and it is the check that two rebuilds of the panel above skipped. When it fails, the answer is master–detail, not tighter spacing.
 - User-facing strings live in `Offstream.App/Resources/Strings.resx` (+ `.fr.resx`) with an en/fr key-parity test. Resource **keys are re-keyed for Offstream**; do not carry the predecessor's key names across.