From 9f588b6d616ff13a214e929399c6e468c55089bb Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 23 Oct 2026 12:43:11 -0400 Subject: [PATCH] docs: the history phrases the id sweep missed, restated as present-tense facts or traps (#6277) A wider pass for "previous", "before the fix", "we shipped" and the like. The chi double-subtree, uppercase Vorbis key and stopPropagation comments now describe the trap they guard; commit hashes, CI run numbers and a deleted file name are gone; uuidString's comment says what it returns. Co-Authored-By: Claude Opus 5.5 --- internal/auth/clientip_test.go | 4 ++-- internal/coverart/enricher.go | 2 +- internal/library/genre_test.go | 2 +- internal/library/mbids_test.go | 7 +++---- internal/library/song_link_test.go | 4 ++-- internal/library/vorbisgenre_test.go | 2 +- internal/playlists/scheduler.go | 5 ++--- internal/server/server_test.go | 16 +++++++--------- internal/tracks/service.go | 4 ++-- .../lib/components/DiscoverResultCard.test.ts | 15 ++++++--------- web/src/lib/components/SuggestionFeed.test.ts | 8 ++++---- web/src/lib/utils/shuffle.ts | 2 +- 12 files changed, 32 insertions(+), 39 deletions(-) diff --git a/internal/auth/clientip_test.go b/internal/auth/clientip_test.go index f13f52f3..b462e7e4 100644 --- a/internal/auth/clientip_test.go +++ b/internal/auth/clientip_test.go @@ -34,8 +34,8 @@ func TestClientIP(t *testing.T) { }, { // The common case: one TLS-terminating proxy. Note RemoteAddr is - // PUBLIC here — a proxy on its own host — which the previous - // private-range heuristic got wrong. + // PUBLIC here — a proxy on its own host — which a private-range + // heuristic would get wrong. name: "one proxy on a public address yields the client", hops: 1, remoteAddr: "203.0.113.200:40000", diff --git a/internal/coverart/enricher.go b/internal/coverart/enricher.go index 34f6d9f9..bcce89c1 100644 --- a/internal/coverart/enricher.go +++ b/internal/coverart/enricher.go @@ -362,7 +362,7 @@ func (e *Enricher) RetryAlbum(ctx context.Context, albumID pgtype.UUID) error { return e.EnrichAlbum(ctx, albumID) } -// uuidString carries over from the previous enricher. +// uuidString renders a UUID as text for logs, or "" when it is not set. func uuidString(u pgtype.UUID) string { if !u.Valid { return "" diff --git a/internal/library/genre_test.go b/internal/library/genre_test.go index a3055058..66576036 100644 --- a/internal/library/genre_test.go +++ b/internal/library/genre_test.go @@ -334,7 +334,7 @@ func TestExtractGenres_EndToEnd(t *testing.T) { if err != nil { t.Fatalf("tag.ReadFrom: %v", err) } - // Confirm the upstream behaviour this fix exists for is still present — + // Confirm the upstream behaviour this reader works around is still present — // if dhowden ever fixes it, this test tells us the workaround can go. if welded := meta.Genre(); welded != "Alternative RockRock" { t.Logf("note: dhowden/tag no longer welds multi-values (got %q)", welded) diff --git a/internal/library/mbids_test.go b/internal/library/mbids_test.go index 510ec05a..7f3471d4 100644 --- a/internal/library/mbids_test.go +++ b/internal/library/mbids_test.go @@ -97,10 +97,9 @@ func TestExtractMBIDs_MissingTagsReturnEmpty(t *testing.T) { } func TestExtractMBIDs_VorbisUppercaseKeysMiss(t *testing.T) { - // Regression guard: real dhowden never produces uppercase Vorbis - // keys. If a future change starts looking up "MUSICBRAINZ_ALBUMID" - // directly, this test must keep returning empty so we don't - // silently re-introduce the v0 bug where every album skipped. + // Real dhowden never produces uppercase Vorbis keys. Looking up + // "MUSICBRAINZ_ALBUMID" directly would find nothing, and every album + // would be skipped; this test keeps that lookup returning empty. m := stubMeta{ format: tag.VORBIS, raw: map[string]interface{}{ diff --git a/internal/library/song_link_test.go b/internal/library/song_link_test.go index 1c75cbe2..f42581ca 100644 --- a/internal/library/song_link_test.go +++ b/internal/library/song_link_test.go @@ -168,8 +168,8 @@ func TestCrossReleaseLinkNeedsAutoResolve_Integration(t *testing.T) { // A merge keeps the removed copy's link to the song on another release, // whichever copy's id names the song. That copy has no song_id of its own, -// and before the fix its link was lost whenever the random ids made it the -// key (the flaky failure in run 8891). +// and its link must survive whichever way the random ids fall, including when +// they make it the key. func TestMergeInheritsSongLink_Integration(t *testing.T) { for _, removeAlbumCut := range []bool{true, false} { pool := newPool(t) diff --git a/internal/library/vorbisgenre_test.go b/internal/library/vorbisgenre_test.go index 1349123f..9277c92c 100644 --- a/internal/library/vorbisgenre_test.go +++ b/internal/library/vorbisgenre_test.go @@ -170,7 +170,7 @@ func TestExtractGenres_FLACRepeatedFields(t *testing.T) { if err != nil { t.Fatalf("tag.ReadFrom: %v", err) } - // Confirm the upstream behaviour this fix exists for is still present. + // Confirm the upstream behaviour this reader works around is still present. if got := meta.Genre(); got != "Instrumental Hip Hop" { t.Logf("note: dhowden/tag no longer keeps only the last GENRE (got %q)", got) } diff --git a/internal/playlists/scheduler.go b/internal/playlists/scheduler.go index fe65c3a5..ee23f7e1 100644 --- a/internal/playlists/scheduler.go +++ b/internal/playlists/scheduler.go @@ -221,9 +221,8 @@ func (s *Scheduler) reconcile(ctx context.Context) { } // runStartupCatchUp fires BuildSystemPlaylists once for every active -// user at startup, regardless of scheduled time. Matches the -// behavior of the previous system_cron.go startup runOnce so users -// whose scheduled fire was missed during downtime get caught up. +// user at startup, regardless of scheduled time, so users whose +// scheduled fire was missed during downtime get caught up. func (s *Scheduler) runStartupCatchUp(ctx context.Context, users []dbq.ListActiveUsersWithTimezonesRow) { for _, u := range users { s.rebuildUserDaily(ctx, u.ID, time.Now()) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 7a82d9dc..41900cad 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -150,13 +150,12 @@ func TestRouter_RestPathNotSwallowedBySPA(t *testing.T) { } } -// TestRouter_AdminSubtreeNotShadowed is a regression test for a real bug we -// shipped in 06a1fe1 and didn't catch until M6a: r.Route("/api/admin", ...) -// in server.go was creating a second chi subtree at the same prefix as the -// nested admin Route inside api.Mount. chi.Walk shows both subtrees as -// registered, but runtime routing dispatch only matches one — every admin -// endpoint registered by api.Mount (summary, quarantine, etc.) -// silently 404'd for authenticated callers in production. +// TestRouter_AdminSubtreeNotShadowed guards the trap of a second +// r.Route("/api/admin", ...) in server.go: it creates a chi subtree at the +// same prefix as the nested admin Route inside api.Mount. chi.Walk shows both +// subtrees as registered, but runtime routing dispatch only matches one, so +// every admin endpoint registered by api.Mount (summary, quarantine, etc.) +// would silently 404 for authenticated callers. // // chi.Walk enumeration would NOT have caught this (both branches are in // the tree). The only reliable check is to make a real authenticated @@ -214,8 +213,7 @@ func TestRouter_AdminSubtreeNotShadowed(t *testing.T) { // Each path is registered by a different package: admin/summary from // api.Mount and admin/scan from server.Router. If chi runtime dispatch - // only routes one /api/admin subtree, one of these returns 404 — that - // IS the bug we shipped in 06a1fe1. + // only routes one /api/admin subtree, one of these returns 404. cases := []struct { method string path string diff --git a/internal/tracks/service.go b/internal/tracks/service.go index f01e9ce7..63b76aa0 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -3,8 +3,8 @@ // part goes through library.DeleteTrackFile — the one path that deletes a // track file. // -// The web track-kebab entry that called this was removed in f7278f24; the -// endpoint was kept deliberately for a safer admin surface to rebind. +// No client calls the endpoint today; it is kept for an admin surface to +// rebind. package tracks import ( diff --git a/web/src/lib/components/DiscoverResultCard.test.ts b/web/src/lib/components/DiscoverResultCard.test.ts index 254aa59d..e22ad06f 100644 --- a/web/src/lib/components/DiscoverResultCard.test.ts +++ b/web/src/lib/components/DiscoverResultCard.test.ts @@ -70,15 +70,12 @@ describe('DiscoverResultCard', () => { expect(onRequest).not.toHaveBeenCalled(); }); - // NOTE: previous versions of this file asserted .badge-row { min-height: 22px - // } and .actions { margin-top: auto } via getComputedStyle, which only worked - // because the component shipped inline style="" attributes — a layering - // violation. Stylesheet introspection (document.styleSheets) was tried as a - // replacement, but vitest's @testing-library/svelte renderer does not inject - // the component's scoped