diff --git a/backend/autotag/recommend.go b/backend/autotag/recommend.go index 98201b1..5dd38cc 100644 --- a/backend/autotag/recommend.go +++ b/backend/autotag/recommend.go @@ -17,6 +17,34 @@ const ( RecommendationStrong Recommendation = "strong" ) +// ConfidentTier is the tier at which this package considers a match +// good enough to act on without being asked to look. +// +// It exists as a name rather than as `== RecommendationStrong` at +// each call site because two features read it and they must not +// disagree about what "high confidence" means: the album page tells +// the user unprompted that the autotagger has a match (#28), and +// strict auto-accept will rewrite the files without asking (#90). +// A page that says "we are sure" about something the auto-accept +// pass would decline is the app contradicting itself. +// +// What the two do *not* share is everything else. Surfacing a match +// is a suggestion with a confirm dialog behind it; auto-accept is an +// irreversible on-disk rewrite, and #90 gates it on further +// conditions this tier cannot express — exact track count, every +// title matching, lengths within a couple of seconds, no cover +// replacement, no MBID conflict. So this is the floor both stand on, +// not the whole of either test. +const ConfidentTier = RecommendationStrong + +// Confident reports whether a tier clears ConfidentTier. +// +// A comparison rather than an equality, so adding a tier above +// "strong" later does not silently stop qualifying. +func Confident(r Recommendation) bool { + return recommendationRank(r) >= recommendationRank(ConfidentTier) +} + const ( // Absolute score tiers. strongScoreThresh = 0.90 diff --git a/backend/autotag/recommend_test.go b/backend/autotag/recommend_test.go index 73f2e94..94ab007 100644 --- a/backend/autotag/recommend_test.go +++ b/backend/autotag/recommend_test.go @@ -168,3 +168,34 @@ func TestRecommend_LocalCandidatesWithoutRGMBIDCompareByTitle(t *testing.T) { t.Errorf("different-title rival: Recommend = %q, want medium", got) } } + +// The tier both features stand on is one name, checked here rather +// than assumed at two call sites. +// +// #28 renders "we have a match for this album" on the album page and +// #90 will rewrite files without asking; a page that claims confidence +// the auto-accept pass would decline is the app contradicting itself. +// What they do not share is everything else — auto-accept adds gates +// this tier cannot express — so this pins the floor, not the whole of +// either test. +func TestConfidentIsTheOneSharedFloor(t *testing.T) { + t.Parallel() + + if ConfidentTier != RecommendationStrong { + t.Errorf("ConfidentTier = %q, want strong", ConfidentTier) + } + + for _, tc := range []struct { + rec Recommendation + want bool + }{ + {RecommendationNone, false}, + {RecommendationLow, false}, + {RecommendationMedium, false}, + {RecommendationStrong, true}, + } { + if got := Confident(tc.rec); got != tc.want { + t.Errorf("Confident(%q) = %v, want %v", tc.rec, got, tc.want) + } + } +} diff --git a/backend/autotagservice/albummatch.go b/backend/autotagservice/albummatch.go new file mode 100644 index 0000000..f60eab1 --- /dev/null +++ b/backend/autotagservice/albummatch.go @@ -0,0 +1,145 @@ +package autotagservice + +import ( + "database/sql" + "fmt" + + "yellowjacket/backend/autotag" +) + +// AlbumMatchView is "the autotagger already has a confident match for +// the album you are looking at". +// +// It is deliberately not a score. The album page renders a suggestion, +// and a suggestion has to be actionable: which release, what it is +// called, and whether acting on it here would do the whole album or +// only part of it. +type AlbumMatchView struct { + // GroupKey is the tagging group the actions operate on. + GroupKey string `json:"groupKey"` + + // Recommendation is the tier, as a string, for a caller that + // wants to render the strength rather than trust the filter. + Recommendation string `json:"recommendation"` + + // Score is the top candidate's raw score, 0..1. + Score float64 `json:"score"` + + // ReleaseMBID is the release Apply would write. + ReleaseMBID string `json:"releaseMbid"` + + // Title and ArtistCredit name that release, so the banner can say + // what it is offering rather than "a match". + Title string `json:"title"` + ArtistCredit string `json:"artistCredit"` + + // TrackCount is the group's local track count. + TrackCount int64 `json:"trackCount"` + + // GroupCount is how many tagging groups this album spans. + // + // More than one means a multi-disc album (one group per disc), and + // it is the reason this is a field rather than an implementation + // detail: applying "the album" from a single button would retag + // one disc of three and leave the folder holding a mix of old and + // new tags. The caller offers review instead. + GroupCount int `json:"groupCount"` +} + +// MatchForAlbum answers "does the autotagger have something confident +// to say about this album", for the album detail page. +// +// Three things about it are load-bearing. +// +// **It costs no MusicBrainz request.** Everything it needs is already +// on disk: `tagging_items` carries the top score and release from the +// background prefetch, and `tagging_candidates` durably holds the +// scored list. The rate limiters here are shared with every page the +// user can open, so a lookup that fires on page load must not join +// that queue — which also means this returns nothing for a folder +// nobody has scored yet, rather than scoring it now. That is the +// right trade: the prefetch will get to it, and a page that silently +// spends a minute of somebody's MusicBrainz budget to draw a banner +// is worse than a page that says nothing. +// +// **The tier is computed, not read.** `tagging_items.score` is the raw +// number and `Recommend` is what turns it into a claim — capping it +// for an ambiguous runner-up, an incomplete alignment or a folder too +// small to corroborate itself. Filtering on the raw score would +// promise confidence the scorer had explicitly withheld. +// +// **Nothing is said about an album the user has already answered +// for.** Only a `pending` group qualifies: `confirmed` covers both a +// finished apply and an explicit "leave as is", and `skipped` is the +// user saying not now. Re-offering either is nagging, and "leave as +// is" would be actively wrong to argue with. +func (s *Service) MatchForAlbum(albumID int64) (*AlbumMatchView, error) { + if albumID <= 0 { + return nil, nil //nolint:nilnil // "no album" is not an error. + } + + rows, err := s.db.Queries.GetTaggingItemsForAlbum( + s.ctx, sql.NullInt64{Int64: albumID, Valid: true}, + ) + if err != nil { + return nil, fmt.Errorf("tagging items for album: %w", err) + } + + pending := rows[:0:0] + + for _, row := range rows { + if row.Status == "pending" { + pending = append(pending, row) + } + } + + if len(pending) == 0 { + return nil, nil //nolint:nilnil // nothing to say is not an error. + } + + // Rows arrive best-score-first, so the first pending one is the + // group worth describing. On a multi-disc album that is one disc + // of several and GroupCount says so. + best := pending[0] + + cands := s.lookupCachedCandidates(best.GroupKey) + if len(cands) == 0 { + return nil, nil //nolint:nilnil // not scored yet; see the doc comment. + } + + locals, err := s.scorer.LocalTracksForGroup(s.ctx, best.GroupKey) + if err != nil { + return nil, fmt.Errorf("local tracks for group: %w", err) + } + + group := autotag.Group{ + AlbumName: best.AlbumName, + AlbumArtist: best.AlbumArtist, + Tracks: locals, + Synthetic: best.Synthetic != 0, + } + + rec := autotag.Recommend(group, cands) + if !autotag.Confident(rec) { + return nil, nil //nolint:nilnil // not confident enough to interrupt. + } + + top := cands[0] + + // The release the banner names must be the release Apply would + // write. Apply with an empty MBID takes the top cached candidate, + // which is what this reads — but it is passed explicitly anyway, + // so a rescore between the page rendering and the user clicking + // cannot swap the album out from under a button they have already + // read. + return &AlbumMatchView{ + GroupKey: best.GroupKey, + Recommendation: string(rec), + Score: top.Score, + ReleaseMBID: top.ReleaseMBID, + Title: top.Title, + ArtistCredit: top.ArtistCredit, + TrackCount: best.TrackCount, + GroupCount: len(pending), + }, nil +} diff --git a/backend/autotagservice/albummatch_test.go b/backend/autotagservice/albummatch_test.go new file mode 100644 index 0000000..db6580e --- /dev/null +++ b/backend/autotagservice/albummatch_test.go @@ -0,0 +1,320 @@ +package autotagservice + +import ( + "encoding/json" + "testing" + + "yellowjacket/backend/autotag" + "yellowjacket/backend/database" +) + +// seedAlbumGroup writes one album's files, its tagging item and the +// durable candidate blob the prefetch would have left behind. +// +// The candidate list is what a real one looks like in the two ways +// that decide the tier: a per-track alignment for every local track, +// and a runner-up far enough away not to count as ambiguity. +func seedAlbumGroup( + t *testing.T, + db *database.DB, + groupKey string, + tracks int, + status string, + score float64, +) int64 { + t.Helper() + + for i := 1; i <= tracks; i++ { + database.InsertTestTrack(t, db, database.TestTrack{ + FilePath: filePathFor(groupKey, i), + Title: titleFor(i), + Artist: "Tideline", + Album: "Glass Harbour", + AlbumArtist: "Tideline", + TrackNumber: int64(i), + LengthMs: 200000, + LibraryID: 0, + GroupKey: groupKey, + }) + } + + if _, err := db.ExecContext(` + INSERT INTO tagging_items + (group_key, library_id, track_count, album_name, album_artist, + disc_number, status, score, best_match_release_mbid) + VALUES (?, 0, ?, 'Glass Harbour', 'Tideline', 0, ?, ?, 'rel-1') + `, groupKey, tracks, status, score); err != nil { + t.Fatalf("insert tagging item: %v", err) + } + + var albumID int64 + if err := db.QueryRowWriter( + `SELECT album_id FROM audio_files WHERE group_key = ? LIMIT 1`, groupKey, + ).Scan(&albumID); err != nil { + t.Fatalf("read album id: %v", err) + } + + return albumID +} + +func filePathFor(groupKey string, n int) string { + return "/music/" + groupKey + "/0" + string(rune('0'+n)) + ".mp3" +} + +func titleFor(n int) string { + return "Track " + string(rune('0'+n)) +} + +// storeCandidates writes the durable blob GetCandidates would have +// cached, with `top` as the winning score. +func storeCandidates( + t *testing.T, db *database.DB, groupKey string, tracks int, top float64, +) { + t.Helper() + + aligns := make([]autotag.TrackAlignment, 0, tracks) + for i := range tracks { + aligns = append(aligns, autotag.TrackAlignment{ + Status: autotag.AlignmentMatched, + LocalIndex: i, + }) + } + + cands := []autotag.Candidate{ + { + ReleaseMBID: "rel-1", + ReleaseGroupMBID: "rg-1", + Title: "Glass Harbour", + ArtistCredit: "Tideline", + TrackCount: tracks, + Alignments: aligns, + Score: top, + }, + { + ReleaseMBID: "rel-2", + ReleaseGroupMBID: "rg-2", + Title: "Something Else", + ArtistCredit: "Another Band", + TrackCount: tracks, + Score: 0.40, + }, + } + + blob, err := json.Marshal(cands) + if err != nil { + t.Fatalf("marshal candidates: %v", err) + } + + if _, err := db.ExecContext( + `INSERT INTO tagging_candidates (group_key, candidates) VALUES (?, ?)`, + groupKey, string(blob), + ); err != nil { + t.Fatalf("insert candidates: %v", err) + } +} + +// A confident match is what the album page exists to surface. +func TestMatchForAlbumSurfacesAConfidentMatch(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + albumID := seedAlbumGroup(t, db, "grp-1", 8, "pending", 0.95) + storeCandidates(t, db, "grp-1", 8, 0.95) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got == nil { + t.Fatal("no match returned for a strong candidate") + } + + if got.Recommendation != string(autotag.RecommendationStrong) { + t.Errorf("recommendation = %q, want strong", got.Recommendation) + } + + // The release named is the release Apply would write — the page + // must not offer one album and tag another. + if got.ReleaseMBID != "rel-1" || got.Title != "Glass Harbour" { + t.Errorf("named %q/%q, want rel-1/Glass Harbour", got.ReleaseMBID, got.Title) + } + + if got.GroupCount != 1 { + t.Errorf("groupCount = %d, want 1", got.GroupCount) + } +} + +// The tier is computed from the candidates, not read off the raw +// score — a high number the scorer would have capped must not reach +// the page as confidence it withheld. +func TestMatchForAlbumDoesNotTrustTheStoredScore(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + // Two tracks: below the evidence floor, so `Recommend` caps this + // at medium however well it scores. + albumID := seedAlbumGroup(t, db, "grp-2", 2, "pending", 0.99) + storeCandidates(t, db, "grp-2", 2, 0.99) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got != nil { + t.Errorf("surfaced %+v for a two-track folder, want nothing", got) + } +} + +// A weak match is not worth interrupting for. +func TestMatchForAlbumStaysQuietBelowTheTier(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + albumID := seedAlbumGroup(t, db, "grp-3", 8, "pending", 0.60) + storeCandidates(t, db, "grp-3", 8, 0.60) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got != nil { + t.Errorf("surfaced %+v for a 0.60 match, want nothing", got) + } +} + +// An album the user has already answered for is not re-offered. +// +// `confirmed` covers both a finished apply and an explicit "leave as +// is", and arguing with the second would be actively wrong. +func TestMatchForAlbumRespectsAnAnswerAlreadyGiven(t *testing.T) { + t.Parallel() + + for _, status := range []string{"confirmed", "skipped", "matched"} { + t.Run(status, func(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + albumID := seedAlbumGroup(t, db, "grp-"+status, 8, status, 0.95) + storeCandidates(t, db, "grp-"+status, 8, 0.95) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got != nil { + t.Errorf("surfaced %+v for a %s group, want nothing", got, status) + } + }) + } +} + +// A folder nobody has scored yet says nothing, rather than scoring it +// now: the MusicBrainz limiter is shared with every page the user can +// open, and this runs on page load. +func TestMatchForAlbumMakesNoNetworkCallForAnUnscoredFolder(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + // No storeCandidates: the prefetch has not reached this folder. + albumID := seedAlbumGroup(t, db, "grp-4", 8, "pending", 0.95) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got != nil { + t.Errorf("surfaced %+v with no cached candidates, want nothing", got) + } +} + +// A multi-disc album is several groups, and the count is what stops +// the page offering one button that would retag one disc of two. +func TestMatchForAlbumCountsEveryGroupOfTheAlbum(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + albumID := seedAlbumGroup(t, db, "grp-d1", 8, "pending", 0.95) + storeCandidates(t, db, "grp-d1", 8, 0.95) + + // Disc two: same album row, its own folder and tagging group. + for i := 1; i <= 6; i++ { + database.InsertTestTrack(t, db, database.TestTrack{ + FilePath: filePathFor("grp-d2", i), + Title: titleFor(i), + Artist: "Tideline", + Album: "Glass Harbour", + AlbumArtist: "Tideline", + TrackNumber: int64(i), + DiscNumber: 2, + LengthMs: 200000, + LibraryID: 0, + GroupKey: "grp-d2", + }) + } + + if _, err := db.ExecContext(` + INSERT INTO tagging_items + (group_key, library_id, track_count, album_name, album_artist, + disc_number, status, score) + VALUES ('grp-d2', 0, 6, 'Glass Harbour', 'Tideline', 2, 'pending', 0.93) + `); err != nil { + t.Fatalf("insert disc two: %v", err) + } + + storeCandidates(t, db, "grp-d2", 6, 0.93) + + got, err := svc.MatchForAlbum(albumID) + if err != nil { + t.Fatalf("MatchForAlbum: %v", err) + } + + if got == nil { + t.Fatal("no match returned") + } + + if got.GroupCount != 2 { + t.Errorf("groupCount = %d, want 2", got.GroupCount) + } + + // Best-first: the 0.95 disc is the one described. + if got.GroupKey != "grp-d1" { + t.Errorf("described %q, want the higher-scoring grp-d1", got.GroupKey) + } +} + +// An album with no local files at all — a pure catalog page — is not +// a question this can answer. +func TestMatchForAlbumSaysNothingWithoutAnAlbum(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + svc := newTestService(t, db) + + for _, id := range []int64{0, -1, 4242} { + got, err := svc.MatchForAlbum(id) + if err != nil { + t.Fatalf("MatchForAlbum(%d): %v", id, err) + } + + if got != nil { + t.Errorf("MatchForAlbum(%d) = %+v, want nil", id, got) + } + } +} diff --git a/backend/database/sql/queries/tagging_items.sql b/backend/database/sql/queries/tagging_items.sql index 9a6a2fa..1094f62 100644 --- a/backend/database/sql/queries/tagging_items.sql +++ b/backend/database/sql/queries/tagging_items.sql @@ -342,3 +342,35 @@ WHERE ti.status = 'pending' ) ORDER BY ti.group_key LIMIT 1; + +-- name: GetTaggingItemsForAlbum :many +-- Every tagging group holding a file of this album. +-- +-- The join is `audio_files.group_key`, not a key derived from the +-- album's folder path: a group carved out of a mixed-bag folder by +-- SplitMixedFolder is keyed on its tags rather than on a directory, +-- so a path-derived key finds nothing for exactly the messiest +-- libraries this is meant to help. +-- +-- Usually one row. A multi-disc album is one group per disc, which +-- the caller has to know about rather than average over -- applying +-- to "the album" would silently retag one disc of three. +SELECT + ti.group_key, + ti.status, + ti.score, + ti.best_match_release_mbid, + ti.track_count, + ti.album_name, + ti.album_artist, + ti.synthetic +FROM tagging_items ti +WHERE ti.group_key IN ( + SELECT DISTINCT af.group_key + FROM audio_files af + WHERE af.album_id = sqlc.arg(album_id) AND af.group_key != '' + ) + AND ti.cleared_at IS NULL +-- Best first, with an unscored group last rather than first: NULL +-- sorts low in SQLite and DESC would put it at the top. +ORDER BY ti.score IS NULL, ti.score DESC, ti.group_key; diff --git a/backend/database/sql/sqlcgen/tagging_items.sql.go b/backend/database/sql/sqlcgen/tagging_items.sql.go index da8425f..f3842fd 100644 --- a/backend/database/sql/sqlcgen/tagging_items.sql.go +++ b/backend/database/sql/sqlcgen/tagging_items.sql.go @@ -231,6 +231,82 @@ func (q *Queries) GetTaggingItem(ctx context.Context, groupKey string) (TaggingI return i, err } +const getTaggingItemsForAlbum = `-- name: GetTaggingItemsForAlbum :many +SELECT + ti.group_key, + ti.status, + ti.score, + ti.best_match_release_mbid, + ti.track_count, + ti.album_name, + ti.album_artist, + ti.synthetic +FROM tagging_items ti +WHERE ti.group_key IN ( + SELECT DISTINCT af.group_key + FROM audio_files af + WHERE af.album_id = ?1 AND af.group_key != '' + ) + AND ti.cleared_at IS NULL +ORDER BY ti.score IS NULL, ti.score DESC, ti.group_key +` + +type GetTaggingItemsForAlbumRow struct { + GroupKey string + Status string + Score sql.NullFloat64 + BestMatchReleaseMbid sql.NullString + TrackCount int64 + AlbumName string + AlbumArtist string + Synthetic int64 +} + +// Every tagging group holding a file of this album. +// +// The join is `audio_files.group_key`, not a key derived from the +// album's folder path: a group carved out of a mixed-bag folder by +// SplitMixedFolder is keyed on its tags rather than on a directory, +// so a path-derived key finds nothing for exactly the messiest +// libraries this is meant to help. +// +// Usually one row. A multi-disc album is one group per disc, which +// the caller has to know about rather than average over -- applying +// to "the album" would silently retag one disc of three. +// Best first, with an unscored group last rather than first: NULL +// sorts low in SQLite and DESC would put it at the top. +func (q *Queries) GetTaggingItemsForAlbum(ctx context.Context, albumID sql.NullInt64) ([]GetTaggingItemsForAlbumRow, error) { + rows, err := q.db.QueryContext(ctx, getTaggingItemsForAlbum, albumID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetTaggingItemsForAlbumRow + for rows.Next() { + var i GetTaggingItemsForAlbumRow + if err := rows.Scan( + &i.GroupKey, + &i.Status, + &i.Score, + &i.BestMatchReleaseMbid, + &i.TrackCount, + &i.AlbumName, + &i.AlbumArtist, + &i.Synthetic, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listAudioFilesInTaggingGroup = `-- name: ListAudioFilesInTaggingGroup :many SELECT af.id, diff --git a/frontend/bindings/yellowjacket/backend/autotagservice/index.ts b/frontend/bindings/yellowjacket/backend/autotagservice/index.ts index 2d8d648..f4a466e 100644 --- a/frontend/bindings/yellowjacket/backend/autotagservice/index.ts +++ b/frontend/bindings/yellowjacket/backend/autotagservice/index.ts @@ -7,6 +7,7 @@ export { }; export type { + AlbumMatchView, AlignmentView, ApplyResultView, CandidateView, diff --git a/frontend/bindings/yellowjacket/backend/autotagservice/models.ts b/frontend/bindings/yellowjacket/backend/autotagservice/models.ts index 2c4ae9d..48bce0f 100644 --- a/frontend/bindings/yellowjacket/backend/autotagservice/models.ts +++ b/frontend/bindings/yellowjacket/backend/autotagservice/models.ts @@ -1,6 +1,61 @@ // Cynhyrchwyd y ffeil hon yn awtomatig. PEIDIWCH Â MODIWL // This file is automatically generated. DO NOT EDIT +/** + * AlbumMatchView is "the autotagger already has a confident match for + * the album you are looking at". + * + * It is deliberately not a score. The album page renders a suggestion, + * and a suggestion has to be actionable: which release, what it is + * called, and whether acting on it here would do the whole album or + * only part of it. + */ +export interface AlbumMatchView { + /** + * GroupKey is the tagging group the actions operate on. + */ + "groupKey": string; + + /** + * Recommendation is the tier, as a string, for a caller that + * wants to render the strength rather than trust the filter. + */ + "recommendation": string; + + /** + * Score is the top candidate's raw score, 0..1. + */ + "score": number; + + /** + * ReleaseMBID is the release Apply would write. + */ + "releaseMbid": string; + + /** + * Title and ArtistCredit name that release, so the banner can say + * what it is offering rather than "a match". + */ + "title": string; + "artistCredit": string; + + /** + * TrackCount is the group's local track count. + */ + "trackCount": number; + + /** + * GroupCount is how many tagging groups this album spans. + * + * More than one means a multi-disc album (one group per disc), and + * it is the reason this is a field rather than an implementation + * detail: applying "the album" from a single button would retag + * one disc of three and leave the folder holding a mix of old and + * new tags. The caller offers review instead. + */ + "groupCount": number; +} + /** * AlignmentView mirrors autotag.TrackAlignment. LocalIndex of -1 * means "candidate has this track, folder doesn't" (status=missing). diff --git a/frontend/bindings/yellowjacket/backend/autotagservice/service.ts b/frontend/bindings/yellowjacket/backend/autotagservice/service.ts index d43281b..773e110 100644 --- a/frontend/bindings/yellowjacket/backend/autotagservice/service.ts +++ b/frontend/bindings/yellowjacket/backend/autotagservice/service.ts @@ -160,6 +160,39 @@ export function ListPendingFolders(libraryID: number): $CancellablePromise<$mode return $Call.ByID(617511590, libraryID); } +/** + * MatchForAlbum answers "does the autotagger have something confident + * to say about this album", for the album detail page. + * + * Three things about it are load-bearing. + * + * **It costs no MusicBrainz request.** Everything it needs is already + * on disk: `tagging_items` carries the top score and release from the + * background prefetch, and `tagging_candidates` durably holds the + * scored list. The rate limiters here are shared with every page the + * user can open, so a lookup that fires on page load must not join + * that queue — which also means this returns nothing for a folder + * nobody has scored yet, rather than scoring it now. That is the + * right trade: the prefetch will get to it, and a page that silently + * spends a minute of somebody's MusicBrainz budget to draw a banner + * is worse than a page that says nothing. + * + * **The tier is computed, not read.** `tagging_items.score` is the raw + * number and `Recommend` is what turns it into a claim — capping it + * for an ambiguous runner-up, an incomplete alignment or a folder too + * small to corroborate itself. Filtering on the raw score would + * promise confidence the scorer had explicitly withheld. + * + * **Nothing is said about an album the user has already answered + * for.** Only a `pending` group qualifies: `confirmed` covers both a + * finished apply and an explicit "leave as is", and `skipped` is the + * user saying not now. Re-offering either is nagging, and "leave as + * is" would be actively wrong to argue with. + */ +export function MatchForAlbum(albumID: number): $CancellablePromise<$models.AlbumMatchView | null> { + return $Call.ByID(514173221, albumID); +} + /** * RetagGroup flips a group back to 'pending' so the user can * re-review after an apply or skip. Drops the durably-cached diff --git a/frontend/index.ts b/frontend/index.ts index 2431050..fee2280 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -308,6 +308,18 @@ async function handleNavigate( deactivateView(currentViewEl); } target.classList.remove('view-hidden'); + + // A primary view is cached, so there is no construction to + // hand a payload to the way a detail view gets one below. The + // one navigation that carries something is the album page's + // "Review in Autotag", which has to land on *that* album: the + // request goes on as an attribute and `autotag-view` consumes + // it (removes it) once acted on, or every later visit would + // reopen a folder the user finished with long ago. + if (view === 'autotag' && typeof detail.groupKey === 'string') { + target.setAttribute('group-key', detail.groupKey); + } + // A freshly created view was appended hidden, so it did not // self-activate on connection; a cached one was deactivated on // the way out. Either way this is the call that starts it. diff --git a/frontend/src/components/autotag-view/autotag-view.ts b/frontend/src/components/autotag-view/autotag-view.ts index 405bfd3..6d4f6e6 100644 --- a/frontend/src/components/autotag-view/autotag-view.ts +++ b/frontend/src/components/autotag-view/autotag-view.ts @@ -1315,13 +1315,40 @@ export class AutotagView extends ViewLifecycleMixin(LitElement) { // the page only needs the folder list, which is local and may // have moved while the page was away. if (this.queueStarted) { - void this.loadFolders(); + void this.loadFolders().then(() => this.openRequestedFolder()); } else { this.queueStarted = true; - void this.startQueue(); + void this.startQueue().then(() => this.openRequestedFolder()); } } + /** + * Open the folder somebody navigated here to look at. + * + * The album page's "Review in Autotag" has to land on *that* + * album. The queue is sorted by score so the intended folder is + * often near the top, but "often" is a link that sometimes opens + * the wrong album, which is worse than no link. + * + * It is an attribute rather than a property because this is a + * **cached primary view**: `index.ts` creates it once and reuses + * it, so there is no construction to pass a value to. Which is + * also why the request is *consumed* — the attribute is removed + * once acted on, or every later visit to Autotag would reopen an + * album the user finished with three navigations ago. + */ + private openRequestedFolder(): void { + const requested = this.getAttribute('group-key'); + + if (!requested) return; + + this.removeAttribute('group-key'); + + if (this.current?.groupKey === requested) return; + + void this.selectFolder(requested); + } + protected override onViewDeactivate(): void { this.unsubscribeLibraryStore?.(); this.unsubscribeLibraryStore = undefined; diff --git a/frontend/src/components/confirm-dialog/confirm-dialog.ts b/frontend/src/components/confirm-dialog/confirm-dialog.ts index 220f5c8..6c138d5 100644 --- a/frontend/src/components/confirm-dialog/confirm-dialog.ts +++ b/frontend/src/components/confirm-dialog/confirm-dialog.ts @@ -81,23 +81,55 @@ export class ConfirmDialog extends LitElement { `, ]; + /** + * Which question is on screen. + * + * This is a singleton reused for every confirmation in the app, + * and `wa-dialog` reports its close *asynchronously* — `open = + * false` starts an animation and `wa-hide` arrives after it. So a + * hide belonging to a question that has already been answered can + * land after the *next* question has opened, and cancel it: the + * user is asked something, the dialog vanishes on its own, and the + * call site is told they said no. + * + * The counter is what tells one question from the next. Every + * close bumps it, and the `wa-hide` handler carries the id its + * template was rendered with. + */ + private askSeq = 0; + /** Ask. Resolves true if the user went ahead. */ ask(request: ConfirmRequest): Promise { this.close(false); + + const id = ++this.askSeq; + this.request = request; return new Promise((resolve) => { this.settle = resolve; void this.updateComplete.then(() => { - if (this.dialog) this.dialog.open = true; + // A third question could have arrived while this one + // was waiting for its own render. + if (this.askSeq === id && this.dialog) this.dialog.open = true; }); }); } - private close(ok: boolean): void { + /** + * Settle the current question, if `id` still names it. + * + * The button handlers pass nothing and always mean the question on + * screen; only `wa-hide` carries an id, because only `wa-hide` can + * arrive late. + */ + private close(ok: boolean, id = this.askSeq): void { + if (id !== this.askSeq) return; + const settle = this.settle; this.settle = null; + this.askSeq++; if (this.dialog) this.dialog.open = false; this.request = null; @@ -118,11 +150,15 @@ export class ConfirmDialog extends LitElement { if (!request) return nothing; + // Captured at render time, so the handler answers the question + // it was drawn for and not whichever one is up when it fires. + const id = this.askSeq; + return html` this.close(false)} + @wa-hide=${() => this.close(false, id)} >

${request.message}

${request.impact diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index acd5605..5106123 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -30,12 +30,16 @@ import { Events } from '../../events'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '../library-status-indicator/library-status-indicator.js'; import { libraryStatusFor } from '@utils/library-status'; +import { ICON_AUTOTAG } from '@utils/icon-language'; import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js'; import '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; import '@awesome.me/webawesome/dist/components/button/button.js'; import '../download-picker/download-picker'; import { downloadStore } from '../../store/download-store'; +import { MatchForAlbum, ApplyAsync } from '@go/autotagservice/service.js'; +import type * as autotagservice from '@go/autotagservice/models.js'; +import { confirmAction } from '../confirm-dialog/confirm-dialog'; import { queueStore } from '../../store/queue-store'; import type { QueueSource } from '../../store/queue-store'; import { notificationStore } from '../../store/notification-store'; @@ -175,6 +179,19 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { @property({ type: Number, attribute: 'local-album-id' }) localAlbumId = 0; + /** + * A confident autotag match for this album, or null when there is + * none worth mentioning. + * + * The tier behind "confident" is `autotag.ConfidentTier`, decided + * in the backend so this page and strict auto-accept cannot + * disagree about what it means (#28, #90). + */ + @state() private autotagMatch: autotagservice.AlbumMatchView | null = null; + + /** True while an apply started from this page is in flight. */ + @state() private applyingTags = false; + /* ── Internal state ── */ @state() private releaseGroup: MBReleaseGroup | null = null; @@ -545,6 +562,57 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { flex-shrink: 0; } + /* ── The autotag suggestion ── */ + .autotag-match { + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 8px 12px; + margin-bottom: 12px; + padding: 10px 14px; + border: 1px solid + var(--yj-border-subtle, rgba(255, 255, 255, 0.08)); + border-radius: 8px; + background: var(--yj-surface-1, rgba(255, 255, 255, 0.04)); + } + + .autotag-match > wa-icon { + flex-shrink: 0; + font-size: var(--yj-icon-sm); + color: var(--yj-text-secondary, #b3b3b3); + } + + .autotag-match-text { + margin: 0; + flex: 1; + /* The suggestion sits in a flex row beside its buttons, + * and a grid/flex item's implicit minimum is its + * content — without this a long release title pushes + * the actions off the end at phone width. */ + min-width: 0; + font-size: var(--yj-text-sm); + color: var(--yj-text-secondary, #b3b3b3); + } + + .autotag-match-text strong { + color: var(--yj-text-primary, #fff); + font-weight: 600; + } + + .autotag-match-note { + display: block; + margin-top: 2px; + font-size: var(--yj-text-xs); + color: var(--yj-text-tertiary, #888); + } + + .autotag-match-actions { + display: flex; + align-items: center; + gap: 8px; + flex-shrink: 0; + } + /* ── Other versions (a disclosure, below the tracklist) ── */ .versions { margin-top: 24px; @@ -1062,6 +1130,14 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { this.localTracks = []; this.filePaths = new Map(); this.askedFor = new Set(); + this.autotagMatch = null; + + // Not awaited: the banner is a bonus and the page must not + // wait on it. It is also the *most* useful on an untagged + // album, which is exactly the page that has least else to + // show, so it is asked for on both branches below rather than + // only the catalog one. + void this.loadAutotagMatch(); // Local-only album (no MBID) — populate entirely from library. if (!mbid && this.localAlbumId) { @@ -1254,6 +1330,31 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { return this.completeness; } + /** + * Ask whether the autotagger already has a confident match here. + * + * It answers from what the background prefetch has already scored + * and makes no MusicBrainz request, so this is safe on page load — + * see `MatchForAlbum`. A folder nobody has reached yet answers + * `null`, which is the same as "nothing to say": the banner is a + * bonus, so a failure is a missing suggestion rather than an error + * the user can act on, and it stays in the console. + */ + private async loadAutotagMatch(): Promise { + if (this.localAlbumId <= 0) { + this.autotagMatch = null; + + return; + } + + try { + this.autotagMatch = await MatchForAlbum(this.localAlbumId); + } catch (err) { + console.error('[explore-album] autotag match lookup failed', err); + this.autotagMatch = null; + } + } + /** * Fetch and set `localTracks` directly by local album id — the * definite source of truth, used when nothing else has already @@ -2415,6 +2516,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { entity-type="album" @catalog-retry=${this.retryCatalog} > + ${this.renderAutotagMatch()} ${this.renderChosenVersion()} ${this.renderTracklistScope()} ${this.renderTracklist()} @@ -3069,6 +3171,155 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { /* ── Version Selector (R025, R026, R027) ── */ + /** + * "MusicBrainz has a match for this album." + * + * The complaint this answers is that the user had to notice the + * metadata was missing, then go and hunt the album down on the + * Autotag page — so the point is to say it *here*, while they are + * looking at the thing, with something to do about it. + * + * Three things about it are load-bearing. + * + * **Applying is offered only when it would do the whole album.** A + * tagging group is a folder, so a multi-disc album is several, and + * one button that applied to the best-scoring one would leave the + * album holding a mix of old and new tags — the exact case the + * app's Blocking notification level exists for. `groupCount` is + * the test, and the answer there is review, not apply. + * + * **The confirm is not a formality.** This rewrites tags on disk + * and cannot be undone, so it goes through `confirmAction()` with + * an impact line that says so in those words. + * + * **The banner does not claim a percentage.** The backend has a + * score and deliberately does not put it in the sentence: 0.95 + * reads as a probability and is not one. What the user needs is + * which release it is, which is what the release title and artist + * are for. + */ + private renderAutotagMatch() { + const match = this.autotagMatch; + + if (!match) return nothing; + + const wholeAlbum = match.groupCount === 1; + + return html` +
+ +

+ MusicBrainz has a match for this album: + ${match.title} + ${match.artistCredit ? html` by ${match.artistCredit}` : nothing}. + ${wholeAlbum + ? nothing + : html`It is filed as ${match.groupCount} folders here, so + tagging it is a review rather than one + step.`} +

+
+ ${wholeAlbum + ? html`Apply tags` + : nothing} + Review in Autotag +
+
+ `; + } + + /** Hand the group over to the Autotag page and go there. */ + private onReviewAutotagMatch = () => { + const match = this.autotagMatch; + + if (!match) return; + + this.dispatchEvent( + new CustomEvent('navigate', { + bubbles: true, + composed: true, + detail: { view: 'autotag', groupKey: match.groupKey }, + }), + ); + }; + + /** + * Apply the match, after asking. + * + * `ApplyAsync` is the registered-job path, so the work is visible + * in the jobs indicator and cancellable there like every other + * long-running operation — this page does not grow a second + * progress surface for it. What it does own is the *acknowledgement* + * that the request was accepted, because the button is here. + * + * The page is not refreshed on completion either: rewriting tags + * emits `TrackMetadataChanged`, which `library-store` answers by + * discarding every cached collection, and this page reloads from + * that like everything else. + */ + private onApplyAutotagMatch = async () => { + const match = this.autotagMatch; + + if (!match || this.applyingTags) return; + + const ok = await confirmAction({ + title: `Tag this album as “${match.title}”?`, + message: + `The ${match.trackCount} files of this album are rewritten to` + + ` match the MusicBrainz release${ + match.artistCredit ? ` by ${match.artistCredit}` : '' + }.`, + impact: + 'This edits the tags in the files on disk and cannot be' + + ' undone. Nothing is moved or deleted.', + confirmLabel: 'Apply tags', + }); + + if (!ok) return; + + this.applyingTags = true; + + try { + await ApplyAsync(match.groupKey, match.releaseMbid); + + // The suggestion has been acted on, so it stops being a + // suggestion immediately rather than sitting there inviting + // a second click while the job runs. + this.autotagMatch = null; + + notificationStore.transient({ + text: 'Tagging this album — progress is in the jobs indicator.', + }); + } catch (err) { + console.error('[explore-album] autotag apply failed', err); + + // Persistent rather than transient: the user asked for + // something that did not happen, and retrying is meaningful. + notificationStore.persistent({ + text: describeError( + err, + 'Those tags could not be applied.', + ), + tone: 'error', + }); + } finally { + this.applyingTags = false; + } + }; + /** * Which pressing is on screen — said only when the user chose it. * diff --git a/frontend/src/components/sidebar/app-sidebar.ts b/frontend/src/components/sidebar/app-sidebar.ts index 797cd1d..1c3c56f 100644 --- a/frontend/src/components/sidebar/app-sidebar.ts +++ b/frontend/src/components/sidebar/app-sidebar.ts @@ -6,6 +6,7 @@ import { designTokens } from '../../styles/tokens.css'; import type { DragActiveDetail } from '@utils/drag-controller'; import { ICON_PLAYLIST, + ICON_AUTOTAG, ICON_REQUESTED, } from '@utils/icon-language'; @@ -208,7 +209,7 @@ export class AppSidebar extends LitElement { { id: 'tracks', label: 'Tracks', icon: 'music' }, { id: 'explore', label: 'Explore', icon: 'globe' }, { id: 'downloads', label: 'Downloads', icon: ICON_REQUESTED }, - { id: 'autotag', label: 'Autotag', icon: 'tag' }, + { id: 'autotag', label: 'Autotag', icon: ICON_AUTOTAG }, { id: 'jobs', label: 'Jobs', icon: 'list-check' }, { id: 'settings', label: 'Settings', icon: 'gear' }, ]; diff --git a/frontend/src/utils/icon-language.ts b/frontend/src/utils/icon-language.ts index 28ffc94..e18a646 100644 --- a/frontend/src/utils/icon-language.ts +++ b/frontend/src/utils/icon-language.ts @@ -80,6 +80,17 @@ export const ICON_REQUESTED = 'solid/bookmark'; */ export const ICON_IN_LIBRARY = 'check'; +/** + * The autotagger, and a match it is offering. + * + * The same icon as the Autotag destination in the sidebar, on the rule + * `ICON_PLAYLIST` was chosen by: an icon names the noun it acts on, so + * a suggestion on the album page wears the mark of the page it would + * send you to. Governed from the moment there were two call sites, + * which is when a name stops being a detail of one component. + */ +export const ICON_AUTOTAG = 'tag'; + /** * Something is being fetched right now. * diff --git a/frontend/test/components/album-autotag-match.test.ts b/frontend/test/components/album-autotag-match.test.ts new file mode 100644 index 0000000..efe6705 --- /dev/null +++ b/frontend/test/components/album-autotag-match.test.ts @@ -0,0 +1,286 @@ +/** + * Being told about a match while looking at the album. + * + * The complaint (#28) is that the user had to notice their metadata + * was missing and then go and hunt the album down on the Autotag page. + * So the suggestion is drawn here, with something to do about it — and + * the something rewrites files on disk, which is what most of this + * file is about. + * + * The confidence tier behind "MusicBrainz has a match" is decided in + * the backend (`autotag.ConfidentTier`) so this page and strict + * auto-accept cannot disagree about what it means; what is pinned here + * is only what the page does with the answer. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; +import { page } from 'vitest/browser'; + +import '@components/explore-album-details/explore-album-details'; +import { stub, stubFailure, flush, resetHarness, calls } from '@test/support/harness'; +import { notificationStore } from '@store/notification-store'; +import '@components/notifications/notification-host'; +import { fixture, shadow, shadowAll } from '@test/support/render'; + +const MATCH = 'autotagservice.Service.MatchForAlbum'; +const APPLY = 'autotagservice.Service.ApplyAsync'; + +function match(over: Record = {}) { + return { + groupKey: 'grp-1', + recommendation: 'strong', + score: 0.95, + releaseMbid: 'rel-1', + title: 'Glass Harbour', + artistCredit: 'Tideline', + trackCount: 10, + groupCount: 1, + ...over, + }; +} + +async function albumPage(): Promise { + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + localAlbumId: 7, + }); + + await flush(); + await el.updateComplete; + + return el; +} + +/** The confirm dialog attaches itself to the document on first use. */ +function confirmHost(): (LitElement & { shadowRoot: ShadowRoot | null }) | null { + return document.querySelector('confirm-dialog'); +} + +/** Press one of the dialog's own buttons, the way a person would. */ +async function pressConfirm(testid: string): Promise { + const host = confirmHost(); + + if (!host) throw new Error('confirm-dialog did not mount itself'); + + await host.updateComplete; + host.shadowRoot + ?.querySelector(`[data-testid="${testid}"]`) + ?.click(); + await host.updateComplete; + await flush(); +} + +/** Click one of the banner's buttons by its label. */ +async function pressBanner(el: LitElement, label: string): Promise { + shadowAll(el, '.autotag-match-actions wa-button') + .find((b) => b.textContent?.includes(label)) + ?.click(); + await flush(); +} + +beforeEach(() => { + resetHarness(); + notificationStore.clear(); + stub('explore.Service.BrowseReleases', []); + stub('explore.Service.LookupReleaseGroup', null); + stub('explore.Service.GetThumbnail', ''); + stub('library.Library.GetAlbumTracks', []); + stub('library.Library.GetAlbumCompleteness', { + owned: 0, + expected: 0, + known: false, + complete: false, + }); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + stub('library.Library.GetFilePathsByRecordingMBIDs', {}); + stub(MATCH, null); +}); + +describe('the autotag suggestion', () => { + it('says nothing when the backend has nothing confident', async () => { + const el = await albumPage(); + + expect(shadow(el, '.autotag-match')).toBeNull(); + }); + + /** + * A pure catalog page has no files to retag, so the question is not + * asked at all — this runs on every album open and a call that + * cannot have an answer is a call not worth making. + */ + it('is not even asked about an album with no local files', async () => { + stub(MATCH, match()); + + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + releaseGroupMBID: 'rg-1', + }); + + await flush(); + await el.updateComplete; + + expect(calls(MATCH)).toHaveLength(0); + expect(shadow(el, '.autotag-match')).toBeNull(); + }); + + /** + * The banner names the release rather than quoting a number: 0.95 + * reads as a probability and is not one, and which release it is, is + * the thing the user can actually judge. + */ + it('names the release it is offering', async () => { + stub(MATCH, match()); + + const el = await albumPage(); + const text = shadow(el, '.autotag-match')?.textContent ?? ''; + + expect(text).toContain('MusicBrainz has a match'); + expect(text).toContain('Glass Harbour'); + expect(text).toContain('Tideline'); + expect(text).not.toContain('95'); + }); + + it('offers both an apply and a review', async () => { + stub(MATCH, match()); + + await albumPage(); + + await expect + .element(page.getByRole('button', { name: 'Apply tags' })) + .toBeInTheDocument(); + await expect + .element(page.getByRole('button', { name: 'Review in Autotag' })) + .toBeInTheDocument(); + }); + + /** + * A tagging group is a folder, so a multi-disc album is several. One + * button that applied to the best-scoring one would leave the album + * holding a mix of old and new tags — which is the case the app's + * Blocking notification level exists for, and is worth not creating. + */ + it('will not apply to an album filed as several folders', async () => { + stub(MATCH, match({ groupCount: 2 })); + + const el = await albumPage(); + const labels = shadowAll(el, '.autotag-match-actions wa-button').map( + (b) => b.textContent?.trim(), + ); + + expect(labels).toEqual(['Review in Autotag']); + expect(shadow(el, '.autotag-match')?.textContent).toContain('2 folders'); + }); + + it('navigates to Autotag carrying the group key', async () => { + stub(MATCH, match()); + + const el = await albumPage(); + const seen: CustomEvent[] = []; + + el.addEventListener('navigate', (e) => seen.push(e as CustomEvent)); + + await pressBanner(el, 'Review'); + + expect(seen).toHaveLength(1); + expect(seen[0]?.detail).toMatchObject({ + view: 'autotag', + groupKey: 'grp-1', + }); + }); +}); + +describe('applying from the album page', () => { + /** + * This rewrites tags in files on disk and cannot be undone, so it + * asks first — and cancelling has to be a true no-op, not a + * confirmation that fires the call anyway. + */ + it('asks before it writes, and cancelling writes nothing', async () => { + stub(MATCH, match()); + stub(APPLY, null); + + const el = await albumPage(); + + await pressBanner(el, 'Apply'); + + expect(confirmHost()).not.toBeNull(); + expect(calls(APPLY)).toHaveLength(0); + + await pressConfirm('confirm-cancel'); + await el.updateComplete; + + expect(calls(APPLY)).toHaveLength(0); + expect(shadow(el, '.autotag-match')).not.toBeNull(); + }); + + /** + * The impact line has to say the thing that cannot be taken back, in + * those words — "cannot be undone" — and that nothing is deleted, + * because "rewrites your files" reads worse than it is. + */ + it('says what cannot be undone', async () => { + stub(MATCH, match()); + + const el = await albumPage(); + + await pressBanner(el, 'Apply'); + + const text = confirmHost()?.shadowRoot?.textContent ?? ''; + + expect(text).toContain('cannot be'); + expect(text).toContain('undone'); + expect(text.toLowerCase()).toContain('nothing is moved or deleted'); + }); + + /** + * `ApplyAsync` is the registered-job path, so progress belongs to + * the jobs indicator and this page does not grow a second one. What + * it owes the user is an acknowledgement, because the button is + * here — and the suggestion has to stop inviting a second click. + */ + it('hands the work to the job registry and stands down', async () => { + stub(MATCH, match()); + stub(APPLY, null); + + const el = await albumPage(); + + await pressBanner(el, 'Apply'); + await pressConfirm('confirm-accept'); + await el.updateComplete; + + expect(calls(APPLY)).toHaveLength(1); + // The release is passed explicitly: a rescore between the page + // rendering and the click must not swap the album out from under + // a button the user has already read. + expect(calls(APPLY)[0]?.args).toEqual(['grp-1', 'rel-1']); + expect(shadow(el, '.autotag-match')).toBeNull(); + }); + + /** + * A failure is Persistent, not Transient: the user asked for + * something that did not happen and retrying is meaningful, which is + * the notification store's own rule for choosing the level. + */ + it('keeps a failure on screen', async () => { + stub(MATCH, match()); + stubFailure(APPLY, 'the tag writer refused'); + + const el = await albumPage(); + + await pressBanner(el, 'Apply'); + await pressConfirm('confirm-accept'); + await el.updateComplete; + + // Read it the way a person would: the app's one notification + // surface, rendered. + const host = await fixture('notification-host'); + + await host.updateComplete; + + const shown = shadowAll(host, '[data-testid="notification"]').map( + (n) => n.textContent ?? '', + ); + + expect(shown.join(' ')).toContain('could not be'); + }); +}); diff --git a/frontend/test/components/autotag-requested-folder.test.ts b/frontend/test/components/autotag-requested-folder.test.ts new file mode 100644 index 0000000..5aa7af5 --- /dev/null +++ b/frontend/test/components/autotag-requested-folder.test.ts @@ -0,0 +1,114 @@ +/** + * "Review in Autotag" has to land on *that* album. + * + * The album page can now say the autotagger has a match for what you + * are looking at (#28), and the review link is only worth having if it + * opens the same album. The queue is sorted by score, so the intended + * folder is often near the top — but "often" is a link that sometimes + * opens a different album, which is worse than no link. + * + * Autotag is a **cached primary view**: `index.ts` creates it once and + * reuses it, so there is no construction to pass a value to. The + * request arrives as an attribute, which is why the interesting part + * is that it is *consumed* — an attribute left on a cached element + * would reopen the same folder on every later visit to the page. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/autotag-view/autotag-view'; +import { flush, resetHarness, stub } from '@test/support/harness'; +import { fixture } from '@test/support/render'; + +const FOLDERS = 'autotagservice.Service.ListPendingFolders'; +const CANDIDATES = 'autotagservice.Service.GetCandidates'; + +function folder(groupKey: string, album: string) { + return { + groupKey, + libraryId: 0, + libraryName: 'Test', + folderSubPath: album, + trackCount: 10, + albumName: album, + albumArtist: 'Tideline', + discNumber: 0, + status: 'pending', + score: groupKey === 'grp-top' ? 0.99 : 0.5, + bestMatchReleaseMbid: 'rel-1', + synthetic: false, + likelyMixedBag: false, + }; +} + +/** Mount the view and run its activation, as navigation would. */ +async function autotag(groupKey?: string): Promise { + const el = await fixture('autotag-view'); + + if (groupKey !== undefined) el.setAttribute('group-key', groupKey); + + (el as unknown as { onViewActivate: () => void }).onViewActivate(); + + await flush(); + await el.updateComplete; + await flush(); + await el.updateComplete; + + return el; +} + +/** The folder the view has selected. */ +function selected(el: LitElement): string | undefined { + return (el as unknown as { current?: { groupKey: string } }).current + ?.groupKey; +} + +beforeEach(() => { + resetHarness(); + stub('autotagservice.Service.StartAutotagQueue', null); + stub('autotagservice.Service.GetLocalCoverArt', ''); + stub('autotagservice.Service.AckLibraryWarning', null); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + stub(FOLDERS, [folder('grp-top', 'Loudest Match'), folder('grp-asked', 'Glass Harbour')]); + stub(CANDIDATES, { + groupKey: 'grp-asked', + recommendation: 'strong', + localTracks: [], + candidates: [], + synthetic: false, + mixedBag: false, + }); +}); + +describe('arriving at Autotag from an album page', () => { + it('opens the folder that was asked for, not the top of the queue', async () => { + const el = await autotag('grp-asked'); + + expect(selected(el)).toBe('grp-asked'); + }); + + it('still lands on the best pending folder when nothing was asked', async () => { + const el = await autotag(); + + expect(selected(el)).toBe('grp-top'); + }); + + /** + * The view is cached and never unmounts, so an attribute left behind + * is a standing instruction: every later visit to Autotag would + * reopen an album the user finished with three navigations ago. + */ + it('consumes the request rather than remembering it', async () => { + const el = await autotag('grp-asked'); + + expect(el.hasAttribute('group-key')).toBe(false); + + // A second visit, with no new request: whatever the user had + // selected stays selected. + (el as unknown as { onViewActivate: () => void }).onViewActivate(); + await flush(); + await el.updateComplete; + + expect(selected(el)).toBe('grp-asked'); + }); +}); diff --git a/frontend/test/components/confirm-dialog.test.ts b/frontend/test/components/confirm-dialog.test.ts index ac3683c..9ab447d 100644 --- a/frontend/test/components/confirm-dialog.test.ts +++ b/frontend/test/components/confirm-dialog.test.ts @@ -80,3 +80,42 @@ describe('confirmAction', () => { await expect(Promise.all([first, second])).resolves.toEqual([false, true]); }); }); + +/** + * A late `wa-hide` must not answer the next question. + * + * This is one singleton for every confirmation in the app, and + * `wa-dialog` reports its close *asynchronously* — `open = false` + * starts an animation and `wa-hide` arrives after it. So a hide + * belonging to a question already answered can land after the next one + * has opened: the user is asked something, the dialog vanishes on its + * own, and the call site is told they said no. + * + * Found by writing two `confirmAction()` tests in one file — the + * second could not be accepted at all, because the first one's hide + * had cancelled it before the click landed. In the app it needs two + * confirmations close together, which "apply these tags" now makes + * reachable. + */ +describe('two questions in a row', () => { + it('does not let the first one answer the second', async () => { + const first = confirmAction({ title: 'First?', message: 'One.' }); + + await press('confirm-cancel'); + await expect(first).resolves.toBe(false); + + const second = confirmAction({ title: 'Second?', message: 'Two.' }); + + // Whatever the first dialog's hide animation is still doing, the + // second question is on screen and unanswered. + await new Promise((r) => setTimeout(r, 0)); + + // The title is a `label` on `wa-dialog` and lands in *its* shadow + // root; the message is the part this component renders. + expect(host().shadowRoot?.textContent ?? '').toContain('Two.'); + + await press('confirm-accept'); + + await expect(second).resolves.toBe(true); + }); +}); diff --git a/frontend/test/components/icon-language.test.ts b/frontend/test/components/icon-language.test.ts index 1c14fe5..cf8367f 100644 --- a/frontend/test/components/icon-language.test.ts +++ b/frontend/test/components/icon-language.test.ts @@ -43,6 +43,7 @@ const GOVERNED = [ 'solid/bookmark', 'regular/bookmark', 'bars-staggered', + 'tag', ]; /** The one file allowed to say them, plus its own test. */