Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
35c0d83819 |
No files matched your search
@@ -82,100 +82,6 @@ func TestPruneStaleLocalCrossReferences(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestPruneClearsInLibraryWithNoLocalID covers the fixed point: a row
|
|
||||||
// carrying in_library with a NULL local_*_id. The upsert's conflict
|
|
||||||
// clause is `in_library = MAX(in_library, excluded.in_library)`, so it
|
|
||||||
// can only ever raise the flag, and this pass used to be gated on the id
|
|
||||||
// being present — which meant nothing in the app could clear such a row,
|
|
||||||
// ever. It is asserted for all three entity types because the gate was
|
|
||||||
// written once and used three times, so a fix applied to one is a fix
|
|
||||||
// that looks complete.
|
|
||||||
//
|
|
||||||
// The rows are seeded with raw SQL rather than through seedIndexResult
|
|
||||||
// deliberately: upsertBatch writes a zero LocalArtistID as literal 0,
|
|
||||||
// not NULL, and 0 satisfies `IS NOT NULL` — so the old gate already
|
|
||||||
// caught that shape and a fixture built through the upsert cannot
|
|
||||||
// reproduce this at all. NULL is what the artifact importer and any
|
|
||||||
// older writer leave behind, the column being nullable with no default.
|
|
||||||
func TestPruneClearsInLibraryWithNoLocalID(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
db := database.NewTestDB(t)
|
|
||||||
si := NewSearchIndex(db, nil, nil, slog.Default())
|
|
||||||
|
|
||||||
// A genuinely owned artist, to prove the wider gate does not simply
|
|
||||||
// clear everything it now looks at.
|
|
||||||
database.InsertTestTrack(t, db, database.TestTrack{
|
|
||||||
FilePath: "/music/owned.mp3",
|
|
||||||
Artist: "Owned",
|
|
||||||
})
|
|
||||||
|
|
||||||
artist, err := db.Queries.GetArtistByName(t.Context(), "Owned")
|
|
||||||
if err != nil {
|
|
||||||
t.Fatalf("read seeded artist: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
seedIndexResult(t, db, SearchIndexResult{
|
|
||||||
EntityType: EntityArtist,
|
|
||||||
MBID: testMBID("owned"),
|
|
||||||
Title: "Owned",
|
|
||||||
ArtistName: "Owned",
|
|
||||||
ArtistMBID: testMBID("owned"),
|
|
||||||
InLibrary: true,
|
|
||||||
LocalArtistID: artist.ID,
|
|
||||||
})
|
|
||||||
|
|
||||||
orphans := []struct {
|
|
||||||
name string
|
|
||||||
entityType string
|
|
||||||
mbid string
|
|
||||||
}{
|
|
||||||
{"artist", EntityArtist, "orphan-artist"},
|
|
||||||
{"release group", EntityReleaseGroup, "orphan-release-group"},
|
|
||||||
{"recording", EntityRecording, "orphan-recording"},
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, o := range orphans {
|
|
||||||
if _, err := db.ExecContext(
|
|
||||||
`INSERT INTO explore_index
|
|
||||||
(entity_type, mbid, title, artist_name, artist_mbid,
|
|
||||||
in_library,
|
|
||||||
local_artist_id, local_release_group_id, local_recording_id)
|
|
||||||
VALUES (?, ?, ?, ?, ?, 1, ?, ?, ?)`,
|
|
||||||
dbEntityType(o.entityType), dbMBID(testMBID(o.mbid)), o.name, o.name,
|
|
||||||
dbMBID(testMBID(o.mbid)),
|
|
||||||
nil, nil, nil,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed %s orphan: %v", o.name, err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
si.pruneStaleLocalCrossReferences()
|
|
||||||
|
|
||||||
inLibrary := func(t *testing.T, mbid string) int {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
var flag int
|
|
||||||
if err := db.QueryRowWriter(
|
|
||||||
"SELECT in_library FROM explore_index WHERE mbid = ?", dbMBID(mbid),
|
|
||||||
).Scan(&flag); err != nil {
|
|
||||||
t.Fatalf("read in_library for %q: %v", mbid, err)
|
|
||||||
}
|
|
||||||
|
|
||||||
return flag
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, o := range orphans {
|
|
||||||
if got := inLibrary(t, testMBID(o.mbid)); got != 0 {
|
|
||||||
t.Errorf("%s with a NULL local id: in_library = %d, want 0", o.name, got)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
if got := inLibrary(t, testMBID("owned")); got != 1 {
|
|
||||||
t.Errorf("owned artist: in_library = %d, want 1 (it still has a file)", got)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount verifies the
|
// TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount verifies the
|
||||||
// backfill queue prioritizes artists by how many tracks the user actually
|
// backfill queue prioritizes artists by how many tracks the user actually
|
||||||
// owns, not by how many duplicate-mbid artist rows happen to exist (the
|
// owns, not by how many duplicate-mbid artist rows happen to exist (the
|
||||||
|
|||||||
@@ -2562,19 +2562,6 @@ func (si *SearchIndex) PopulateLocalCrossReferences() {
|
|||||||
// The row itself is left in place (it may still be part of the shipped
|
// The row itself is left in place (it may still be part of the shipped
|
||||||
// catalog, just no longer owned) — only the "this is mine" bookkeeping
|
// catalog, just no longer owned) — only the "this is mine" bookkeeping
|
||||||
// is cleared.
|
// is cleared.
|
||||||
//
|
|
||||||
// It is gated on the flag *or* the id, not on the id alone. Gated on
|
|
||||||
// the id, `in_library = 1 AND local_*_id IS NULL` is a fixed point: the
|
|
||||||
// upsert can only ever raise the flag and this pass skipped such a row
|
|
||||||
// by construction, so nothing in the app could clear it — a row claiming
|
|
||||||
// to be owned, permanently, with no local row to check the claim
|
|
||||||
// against. Nothing in the tree writes that shape today
|
|
||||||
// (collectLibraryEntities sets both together), which is exactly why it
|
|
||||||
// is worth closing now: the exposure is a database written by an older
|
|
||||||
// version, and the next writer that sets the flag without an id, which
|
|
||||||
// nothing structurally prevents. A NULL id fails the existence test on
|
|
||||||
// its own, so the wider gate needs no second clause to say what "not
|
|
||||||
// owned" means.
|
|
||||||
func (si *SearchIndex) pruneStaleLocalCrossReferences() {
|
func (si *SearchIndex) pruneStaleLocalCrossReferences() {
|
||||||
type prune struct {
|
type prune struct {
|
||||||
entityType string
|
entityType string
|
||||||
@@ -2607,8 +2594,7 @@ func (si *SearchIndex) pruneStaleLocalCrossReferences() {
|
|||||||
result, err := si.db.ExecContext(
|
result, err := si.db.ExecContext(
|
||||||
`UPDATE explore_index
|
`UPDATE explore_index
|
||||||
SET in_library = 0, `+p.column+` = NULL
|
SET in_library = 0, `+p.column+` = NULL
|
||||||
WHERE entity_type = ?
|
WHERE entity_type = ? AND `+p.column+` IS NOT NULL
|
||||||
AND (`+p.column+` IS NOT NULL OR in_library = 1)
|
|
||||||
AND NOT EXISTS (`+p.exists+`)`,
|
AND NOT EXISTS (`+p.exists+`)`,
|
||||||
dbEntityType(p.entityType),
|
dbEntityType(p.entityType),
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,12 +1,6 @@
|
|||||||
import { test, expect } from '../support/fixtures.js';
|
import { test, expect } from '../support/fixtures.js';
|
||||||
import type { Page } from '@playwright/test';
|
import type { Page } from '@playwright/test';
|
||||||
|
|
||||||
/**
|
|
||||||
* How far the scroll test scrolls. One constant, because the guard and
|
|
||||||
* the assertion have to agree about it — they did not, which is #133.
|
|
||||||
*/
|
|
||||||
const SCROLL_TARGET = 80;
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Plan 007 phase 5: expanding an album shows its tracks.
|
* Plan 007 phase 5: expanding an album shows its tracks.
|
||||||
*
|
*
|
||||||
@@ -110,27 +104,20 @@ test.describe('the album dropdown', () => {
|
|||||||
await app.setViewportSize({ width: 900, height: 600 });
|
await app.setViewportSize({ width: 900, height: 600 });
|
||||||
|
|
||||||
try {
|
try {
|
||||||
// Wait for the range the assertion below actually needs, not for
|
await expect.poll(() => scrollRange(app)).toMatchObject({
|
||||||
// "scrollable at all" (#133). The guard used to be
|
scrollable: true,
|
||||||
// `scrollHeight > clientHeight + 40` while the next line asks to
|
overflowY: 'auto',
|
||||||
// reach 80, so any range in 41-79 satisfied it and could not
|
});
|
||||||
// satisfy the assertion — and the grid passes through exactly
|
|
||||||
// that while it settles, because it recomputes its columns after
|
|
||||||
// the resize rather than during it. The settled range here is
|
|
||||||
// 330, so this waits rather than weakening anything.
|
|
||||||
await expect
|
|
||||||
.poll(() => scrollRange(app))
|
|
||||||
.toMatchObject({ room: true, overflowY: 'auto' });
|
|
||||||
|
|
||||||
await app.evaluate((target) => {
|
await app.evaluate(() => {
|
||||||
const sc = document
|
const sc = document
|
||||||
.querySelector('cover-grid')
|
.querySelector('cover-grid')
|
||||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||||
|
|
||||||
if (sc) sc.scrollTop = target;
|
if (sc) sc.scrollTop = 80;
|
||||||
}, SCROLL_TARGET);
|
});
|
||||||
|
|
||||||
expect(await scrollTop(app)).toBe(SCROLL_TARGET);
|
expect(await scrollTop(app)).toBe(80);
|
||||||
|
|
||||||
// And the dropdown it opens is on screen, wherever the manager
|
// And the dropdown it opens is on screen, wherever the manager
|
||||||
// decides that leaves the scroll. It is *not* "the position is
|
// decides that leaves the scroll. It is *not* "the position is
|
||||||
@@ -263,19 +250,16 @@ async function closeDropdown(app: Page): Promise<void> {
|
|||||||
|
|
||||||
/** Whether the grid can scroll at all, which decides if a probe can move. */
|
/** Whether the grid can scroll at all, which decides if a probe can move. */
|
||||||
async function scrollRange(app: Page) {
|
async function scrollRange(app: Page) {
|
||||||
return app.evaluate((target) => {
|
return app.evaluate(() => {
|
||||||
const sc = document
|
const sc = document
|
||||||
.querySelector('cover-grid')
|
.querySelector('cover-grid')
|
||||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||||
|
|
||||||
return {
|
return {
|
||||||
// `room` is the precondition of the assertion that follows it:
|
scrollable: !!sc && sc.scrollHeight > sc.clientHeight + 40,
|
||||||
// enough range to actually reach the target. A threshold below
|
|
||||||
// what the caller depends on is not a guard.
|
|
||||||
room: !!sc && sc.scrollHeight - sc.clientHeight >= target,
|
|
||||||
overflowY: sc ? getComputedStyle(sc).overflowY : '',
|
overflowY: sc ? getComputedStyle(sc).overflowY : '',
|
||||||
};
|
};
|
||||||
}, SCROLL_TARGET);
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
async function scrollTop(app: Page): Promise<number> {
|
async function scrollTop(app: Page): Promise<number> {
|
||||||
|
|||||||
Reference in new issue
Block a user