Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
23f3d4b3b0 |
@@ -82,6 +82,100 @@ 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,6 +2562,19 @@ 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
|
||||||
@@ -2594,7 +2607,8 @@ 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 = ? AND `+p.column+` IS NOT NULL
|
WHERE entity_type = ?
|
||||||
|
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),
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -160,52 +160,29 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
|
|||||||
user-select: none;
|
user-select: none;
|
||||||
}
|
}
|
||||||
|
|
||||||
/*
|
|
||||||
* The hover play button is a *hover* affordance, so it is
|
|
||||||
* gated on the device having hover rather than on width. A
|
|
||||||
* touch long-press synthesises a hover state in the WebView,
|
|
||||||
* so on a phone it flashed into view during the 500ms hold
|
|
||||||
* that utils/long-press.ts is measuring for a context menu —
|
|
||||||
* a control appearing because you were reaching for a
|
|
||||||
* different one. A phone user taps the album and plays from
|
|
||||||
* the detail view, so there is nothing to replace it with.
|
|
||||||
*
|
|
||||||
* display:none outside the query rather than opacity:0 on
|
|
||||||
* its own: an opacity-0 button still takes taps and is
|
|
||||||
* still in the accessibility tree, so the invisible control
|
|
||||||
* would keep the hit area it was never meant to have on
|
|
||||||
* touch. Everything else stays inside, so the desktop
|
|
||||||
* animation is unchanged.
|
|
||||||
*/
|
|
||||||
.play {
|
.play {
|
||||||
display: none;
|
position: absolute;
|
||||||
|
right: 8px;
|
||||||
|
bottom: 8px;
|
||||||
|
width: 38px;
|
||||||
|
height: 38px;
|
||||||
|
border: none;
|
||||||
|
border-radius: 50%;
|
||||||
|
background: var(--yj-accent, #ffd43b);
|
||||||
|
color: var(--yj-accent-fg, #000);
|
||||||
|
display: flex;
|
||||||
|
align-items: center;
|
||||||
|
justify-content: center;
|
||||||
|
cursor: pointer;
|
||||||
|
opacity: 0;
|
||||||
|
transform: translateY(6px);
|
||||||
|
transition: opacity 0.12s ease, transform 0.12s ease;
|
||||||
}
|
}
|
||||||
|
|
||||||
@media (hover: hover) and (pointer: fine) {
|
.card:hover .play,
|
||||||
.play {
|
.card:focus-within .play {
|
||||||
position: absolute;
|
opacity: 1;
|
||||||
right: 8px;
|
transform: translateY(0);
|
||||||
bottom: 8px;
|
|
||||||
width: 38px;
|
|
||||||
height: 38px;
|
|
||||||
border: none;
|
|
||||||
border-radius: 50%;
|
|
||||||
background: var(--yj-accent, #ffd43b);
|
|
||||||
color: var(--yj-accent-fg, #000);
|
|
||||||
display: flex;
|
|
||||||
align-items: center;
|
|
||||||
justify-content: center;
|
|
||||||
cursor: pointer;
|
|
||||||
opacity: 0;
|
|
||||||
transform: translateY(6px);
|
|
||||||
transition: opacity 0.12s ease, transform 0.12s ease;
|
|
||||||
}
|
|
||||||
|
|
||||||
.card:hover .play,
|
|
||||||
.card:focus-within .play {
|
|
||||||
opacity: 1;
|
|
||||||
transform: translateY(0);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
.name {
|
.name {
|
||||||
|
|||||||
@@ -1,85 +0,0 @@
|
|||||||
/**
|
|
||||||
* A hover affordance is gated on the device having hover.
|
|
||||||
*
|
|
||||||
* The home page's cover cards reveal a play button on :hover. A touch
|
|
||||||
* long-press synthesises a hover state in the WebView, so on a phone
|
|
||||||
* that button flashed into view during the 500ms hold that
|
|
||||||
* utils/long-press.ts is measuring for a context menu — a control
|
|
||||||
* appearing because the user was reaching for a different one.
|
|
||||||
*
|
|
||||||
* This is asserted against the *parsed stylesheet* rather than by
|
|
||||||
* emulating a touch device, and that is a limitation worth stating
|
|
||||||
* rather than hiding. CDP's Emulation.setEmulatedMedia does not reach
|
|
||||||
* this tier's iframe — matchMedia still answers `hover: hover` after it
|
|
||||||
* is set — so there is no way here to render the component as a phone
|
|
||||||
* would and read the computed style. What can be checked is the shape
|
|
||||||
* the browser actually built from the css`` literal: that the reveal
|
|
||||||
* lives inside a hover media query and that the default is display:none.
|
|
||||||
*
|
|
||||||
* Which is the regression worth catching anyway. The failure mode is
|
|
||||||
* someone hoisting the rule back out of the query for a one-line tidy —
|
|
||||||
* a change nothing renders differently on a desktop, so every other
|
|
||||||
* assertion in this repo passes and the phone silently regresses.
|
|
||||||
*/
|
|
||||||
import { describe, expect, it } from 'vitest';
|
|
||||||
|
|
||||||
import '@components/home-view/home-view';
|
|
||||||
import { fixture } from '@test/support/render';
|
|
||||||
|
|
||||||
/** Every rule in the element's own adopted stylesheets, flattened. */
|
|
||||||
function rulesOf(host: Element): { text: string; condition: string | null }[] {
|
|
||||||
const sheets = host.shadowRoot?.adoptedStyleSheets ?? [];
|
|
||||||
const out: { text: string; condition: string | null }[] = [];
|
|
||||||
|
|
||||||
for (const sheet of sheets) {
|
|
||||||
for (const rule of Array.from(sheet.cssRules)) {
|
|
||||||
if (rule instanceof CSSMediaRule) {
|
|
||||||
for (const inner of Array.from(rule.cssRules)) {
|
|
||||||
out.push({ text: inner.cssText, condition: rule.conditionText });
|
|
||||||
}
|
|
||||||
|
|
||||||
continue;
|
|
||||||
}
|
|
||||||
|
|
||||||
out.push({ text: rule.cssText, condition: null });
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return out;
|
|
||||||
}
|
|
||||||
|
|
||||||
describe('the home card play button', () => {
|
|
||||||
it('reveals itself only where the device has hover', async () => {
|
|
||||||
const el = await fixture('home-view', {});
|
|
||||||
const rules = rulesOf(el);
|
|
||||||
|
|
||||||
// The sweep is worth nothing if it read no rules at all — the same
|
|
||||||
// first assertion icon-language.test.ts makes for the same reason.
|
|
||||||
expect(rules.length).toBeGreaterThan(0);
|
|
||||||
|
|
||||||
const reveals = rules.filter(
|
|
||||||
(r) => r.text.includes('.play') && /opacity:\s*1/.test(r.text),
|
|
||||||
);
|
|
||||||
|
|
||||||
expect(reveals.length).toBeGreaterThan(0);
|
|
||||||
|
|
||||||
for (const rule of reveals) {
|
|
||||||
expect(rule.condition).toMatch(/hover:\s*hover/);
|
|
||||||
expect(rule.condition).toMatch(/pointer:\s*fine/);
|
|
||||||
}
|
|
||||||
});
|
|
||||||
|
|
||||||
it('is display:none rather than transparent where it is absent', async () => {
|
|
||||||
const el = await fixture('home-view', {});
|
|
||||||
|
|
||||||
// opacity:0 alone would leave a button that still takes taps and is
|
|
||||||
// still in the accessibility tree, so a phone would keep the hit
|
|
||||||
// area for a control it can never see.
|
|
||||||
const unconditional = rulesOf(el).filter(
|
|
||||||
(r) => r.condition === null && r.text.startsWith('.play'),
|
|
||||||
);
|
|
||||||
|
|
||||||
expect(unconditional.length).toBeGreaterThan(0);
|
|
||||||
expect(unconditional.some((r) => /display:\s*none/.test(r.text))).toBe(true);
|
|
||||||
});
|
|
||||||
});
|
|
||||||
Reference in New Issue
Block a user