mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 03:09:57 +00:00
v3 library: multi-chart work grouping, complete (engine + API + card + drawer + toggle/split/filter-law) — P5a–P5e (#702)
* v3 library: multi-chart work grouping engine + work-charts API — P5a/P5b
Charts of the same song (same normalized artist+title) now GROUP under a
computed work_key, with a materialized representative filter so the grid
can collapse them without breaking keyset paging:
- work_key = normalize(artist+title) (diacritics/punct/case folded,
leading "The" folded on artist); resolves the effective artist via the
artist_alias table when present (feature-detected, no hard dep).
- Sparse, never-purged-on-rescan tables: chart_group_pref(work_key,
preferred_filename) + chart_group_split(filename, split_key); purged
only by the explicit per-song delete.
- Materialized work_display(filename, work_key, effective_work_key,
is_group_representative, group_size) read-model: lazy rebuild via a
dirty flag set on put/delete; set_chart_preferred does an incremental
re-flip (no full rebuild). Auto-pick representative = most
arrangements -> most plays -> newest -> filename; a user pref wins and
degrades to auto-pick if its file disappears.
- group=1 on query_page/query_stats = one extra representative
predicate applied identically to page + total + sort_letters, so the
keyset cursor (sort_value, filename) stays a valid total order and
counts works, not charts. Grouped rows carry chart_count + work_key.
- Charts API: GET /api/work/{work_key}/charts (members + which is the
keeper, your pick vs auto), PUT/DELETE .../preferred, and
POST /api/chart/{filename}/split + /unsplit (under /api/chart so the
DELETE /api/song catch-all can't shadow them).
Tests: 15 grouping-engine + 7 charts-API tests, including grouped
keyset pagination (no skip/dupe across pages).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN
* v3 library: grouped grid card + persistent "N charts" chip — P5c
Flip the v3 grid to the grouped library (group=1 on /api/library and the
rail's /api/library/stats fetch): one card per song, showing the
representative (preferred/auto-pick) chart. group rides page, total and
sort_letters identically so the A-Z rail's cumulative-seek math and the
virtualized sizer stay consistent, counting works not charts; the keyset
cursor chains with group on every page.
- New groupingActive() helper, default ON per the design; the persisted
per-view toggle (P5e) lands there. Only the local provider implements
group=; smart collections and remote providers ignore it and stay
flat, so it is safe to send unconditionally.
- chartsChipHtml(): a "flag N charts" chip rendered ONLY when
chart_count >= 2 - single-chart cards emit byte-identical markup.
First in the fixed-height chip row + shrink-0 so it never clips and
card height is unchanged.
- Chip click = feature-detected window.__fbOpenChartsDrawer (the Charts
drawer arrives in P5d); until then a no-op. Plain-click / play / the
arrangement chips are untouched and play the representative.
- The library-home repertoire meter's stats fetch deliberately stays
ungrouped: its mastered numerator counts chart filenames, so a works
denominator could exceed 100% - reconciling that is P5e's
mastery-anchor work. The tree view stays flat (query_artists has no
grouping; its opener is wired in P5d).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN
* v3 library: Charts drawer + openers — P5d
The single deep-management surface for a work's charts (design UX-2/3):
a body-appended slide-in drawer (filter-drawer idiom) listing every
chart of the work as a radiogroup — the checked row is the keeper the
grid card plays.
- Rows show format / tuning / arrangements / year / your accuracy (or
"not played") plus the pack filename, usually the only human-readable
distinguisher between duplicate charts. Keeper is labeled
"Preferred (auto)" vs "Preferred - your pick".
- Row click (or Enter/Space) = one-tap Set-preferred; "Reset to auto
pick" appears when the keeper is an explicit pick. Writes go through
the work-charts API and the drawer re-renders from the response; the
grid re-fetches in place since the representative may have flipped.
- Per-row Play (plays that exact chart) and Add-to-playlist (the picker
is z-[200], layering over the z-50 drawer).
- a11y: Tab focus-trap, Escape closes, ArrowUp/Down move focus between
rows (focus only - arrow-select would fire a preferred write per
keystroke), focus restored to the opener on close.
- Openers: the "N charts" chip opens the drawer directly; the card's
overflow menu gains "Charts (N)..." and "Play version >" (expands
inline; picking one plays it as a one-off - the keeper/headline does
not move). Tree rows ride the ungrouped artists endpoint, so the menu
resolves their work lazily via the new GET /api/chart/{fn}/work
({work_key, chart_count}) and slots a "Charts (N)..." entry in when
versions exist. A window.__fbOpenChartsDrawer global lets other views
open the drawer. Right-click is deferred: the open native card
context-menu PR should host that entry once both merge.
- tailwind.min.css rebuilt: carries the new utility classes from this
and the previous commit (the grouped-card chip tint was missing).
Split keys contain '#', so clients MUST URL-encode work_key in paths
(the v3 client does; a test documents the round-trip). 4 new endpoint
tests; 26/26 grouping+charts tests green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN
* v3 library: group toggle, split UI, the filter law + mastery-anchor rules — P5e
Completes the multi-chart grouping slice (design 7.1):
- Filter law under group=1: work-identity (artist/album/search) and
practice-state (favorites/mastery/tags/difficulty) predicates stay on
the representative row, while CHART-INTRINSIC predicates (format/
arrangements/stems/lyrics/tuning) now match if ANY member of the work
does - a song you own in Drop D is no longer hidden because your
preferred chart is E Standard. Intrinsic clauses moved to an
alias-aware builder and re-applied as a member EXISTS; identical in
query_page and query_stats so counts and the A-Z rail stay in
lockstep. A pure predicate - keyset paging is untouched (tested).
- Display-chart switch: when the representative itself doesn't match,
the row carries a display_chart override (the matching member). The
row stays the representative's - swapping rows wholesale would break
the (sort_value, filename) cursor - and the card renders/plays the
member while the accuracy badge and heart stay anchored on the
preferred chart.
- Mastery sort aggregates MAX across the group ("a song surfaces on any
chart you've touched"); OFFSET-paged, so cursor-safe. The
Recently-Added aggregate is deliberately deferred: mtime IS a keyset
sort, so its aggregate would need materializing into work_display.
- History-sticky auto-pick: most-played -> most-complete -> newest.
A newer/"more complete" import can't silently take the pick from the
chart your reps accrued on, and a one-off try of an alternate can't
out-rank a practiced incumbent; all-unplayed groups still pick by
completeness.
- Persisted "One card per song" toggle in the filter drawer (default
ON; OFF = one card per chart). A view mode: never counted in the
filter badge, never saved into collection rules, local provider only.
- Split escape hatch: "Split out" per drawer row gives a chart its own
card; the split card's overflow menu offers "Rejoin other versions"
(rows and the chart-work lookup now carry is_split).
- Mastery-anchor heads-up: after set-preferred the drawer shows a
one-line ambient note that practice history stays with each chart
(no toast - hearing-safe).
10 new filter-law tests; 38/38 grouping tests green. tailwind.min.css
rebuilt for the new utility classes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN
* fix(v3): work-grouping — escape Charts-drawer meta (XSS), keep non-Latin titles distinct, guard mid-rebuild reads (PR #702 review)
- XSS: esc() the composed `meta` string in _chartRowHtml (arrangement/tuning
names come from untrusted feedpak metadata) before innerHTML; acc stays HTML.
- Non-Latin titles: _norm_token falls back to raw lowercased whitespace-collapsed
text when the NFKD+strip fold yields "" (CJK/Cyrillic/Greek/Arabic), so distinct
non-Latin titles keep distinct _work_key values instead of collapsing into one
bogus work. Latin names still hit the folded branch — behavior unchanged.
- Mid-rebuild reads: wrap the grouped representative SELECT in query_page and
query_stats under self._lock (nullcontext when ungrouped, so lazy reads stay
lock-free) so a reader can't observe work_display between rebuild_work_display's
DELETE and INSERT/commit. _ensure_work_display stays OUTSIDE the lock — it
self-locks the rebuild and self._lock is non-reentrant — so only the SELECT is
guarded (rebuild fully completes before the guarded SELECT runs).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
byrongamatos
parent
a47accd894
commit
e9d95ad190
@@ -0,0 +1,200 @@
|
||||
"""Tests for the multi-chart grouping engine (P5a): several charts (feedpak rows)
|
||||
of the same WORK collapse to one representative card via a materialized
|
||||
work_display read-model + a `group=1` `WHERE is_group_representative=1` predicate.
|
||||
Counting works (not charts), auto-pick, chart_group_pref, split, and keyset
|
||||
paging under grouping all stay correct."""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def server(tmp_path, monkeypatch, isolate_logging):
|
||||
monkeypatch.setenv("CONFIG_DIR", str(tmp_path))
|
||||
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
||||
sys.modules.pop("server", None)
|
||||
srv = importlib.import_module("server")
|
||||
try:
|
||||
yield srv
|
||||
finally:
|
||||
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
||||
if conn is not None:
|
||||
conn.close()
|
||||
sys.modules.pop("server", None)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def client(server):
|
||||
return TestClient(server.app)
|
||||
|
||||
|
||||
def _put(server, fn, title, artist, arrangements=1, mtime=0.0):
|
||||
arr = [{"name": "Lead", "index": i} for i in range(arrangements)]
|
||||
server.meta_db.put(fn, mtime, 0, {"title": title, "artist": artist, "arrangements": arr})
|
||||
|
||||
|
||||
def _grouped(client, **params):
|
||||
params["group"] = 1
|
||||
return client.get("/api/library", params=params).json()
|
||||
|
||||
|
||||
# ── Baseline: ungrouped shows every chart ────────────────────────────────────
|
||||
|
||||
def test_ungrouped_shows_all_charts(client, server):
|
||||
_put(server, "a.archive", "Song", "Artist")
|
||||
_put(server, "b.archive", "Song", "Artist")
|
||||
body = client.get("/api/library").json()
|
||||
assert body["total"] == 2
|
||||
assert {s["filename"] for s in body["songs"]} == {"a.archive", "b.archive"}
|
||||
# ungrouped rows carry no chart_count (only the grouped path attaches it)
|
||||
assert "chart_count" not in body["songs"][0]
|
||||
|
||||
|
||||
# ── Grouping collapses a work to one representative ───────────────────────────
|
||||
|
||||
def test_grouping_collapses_to_one_representative(client, server):
|
||||
_put(server, "a.archive", "Song", "Artist", arrangements=1)
|
||||
_put(server, "b.archive", "Song", "Artist", arrangements=2)
|
||||
body = _grouped(client)
|
||||
assert body["total"] == 1
|
||||
assert len(body["songs"]) == 1
|
||||
rep = body["songs"][0]
|
||||
assert rep["filename"] == "b.archive" # most arrangements wins auto-pick
|
||||
assert rep["chart_count"] == 2
|
||||
|
||||
|
||||
def test_autopick_plays_tiebreak(client, server):
|
||||
_put(server, "a.archive", "S", "A", arrangements=1)
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
server.meta_db.record_session("a.archive", 0, score=100, accuracy=0.5) # a has a play
|
||||
assert _grouped(client)["songs"][0]["filename"] == "a.archive"
|
||||
|
||||
|
||||
def test_autopick_filename_tiebreak(client, server):
|
||||
_put(server, "b.archive", "S", "A")
|
||||
_put(server, "a.archive", "S", "A")
|
||||
# equal arrangements + plays + mtime → lowest filename
|
||||
assert _grouped(client)["songs"][0]["filename"] == "a.archive"
|
||||
|
||||
|
||||
# ── chart_group_pref override ────────────────────────────────────────────────
|
||||
|
||||
def test_preferred_overrides_autopick(client, server):
|
||||
_put(server, "a.archive", "S", "A", arrangements=2) # would auto-win
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
wk = server.meta_db.work_key_for("b.archive")
|
||||
server.meta_db.set_chart_preferred(wk, "b.archive")
|
||||
assert _grouped(client)["songs"][0]["filename"] == "b.archive"
|
||||
|
||||
|
||||
def test_set_preferred_incremental_reflip(client, server):
|
||||
_put(server, "a.archive", "S", "A", arrangements=2)
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
assert _grouped(client)["songs"][0]["filename"] == "a.archive" # builds read-model
|
||||
wk = server.meta_db.work_key_for("a.archive")
|
||||
server.meta_db.set_chart_preferred(wk, "b.archive") # incremental re-flip
|
||||
body = _grouped(client)
|
||||
assert body["songs"][0]["filename"] == "b.archive"
|
||||
assert body["songs"][0]["chart_count"] == 2 # group_size unchanged
|
||||
|
||||
|
||||
def test_clear_preferred_returns_to_autopick(client, server):
|
||||
_put(server, "a.archive", "S", "A", arrangements=2)
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
wk = server.meta_db.work_key_for("a.archive")
|
||||
server.meta_db.set_chart_preferred(wk, "b.archive")
|
||||
assert _grouped(client)["songs"][0]["filename"] == "b.archive"
|
||||
server.meta_db.clear_chart_preferred(wk)
|
||||
assert _grouped(client)["songs"][0]["filename"] == "a.archive" # auto again
|
||||
|
||||
|
||||
# ── Split / un-split ─────────────────────────────────────────────────────────
|
||||
|
||||
def test_split_makes_singleton(client, server):
|
||||
_put(server, "a.archive", "S", "A")
|
||||
_put(server, "b.archive", "S", "A")
|
||||
assert _grouped(client)["total"] == 1
|
||||
server.meta_db.split_chart("b.archive")
|
||||
body = _grouped(client)
|
||||
assert body["total"] == 2 # two works now
|
||||
assert {s["filename"] for s in body["songs"]} == {"a.archive", "b.archive"}
|
||||
assert all(s["chart_count"] == 1 for s in body["songs"])
|
||||
|
||||
|
||||
def test_unsplit_rejoins(client, server):
|
||||
_put(server, "a.archive", "S", "A")
|
||||
_put(server, "b.archive", "S", "A")
|
||||
server.meta_db.split_chart("b.archive")
|
||||
assert _grouped(client)["total"] == 2
|
||||
server.meta_db.unsplit_chart("b.archive")
|
||||
assert _grouped(client)["total"] == 1
|
||||
|
||||
|
||||
# ── Stats + A–Z count works, not charts ──────────────────────────────────────
|
||||
|
||||
def test_stats_counts_works_not_charts(client, server):
|
||||
_put(server, "a.archive", "Song", "Artist")
|
||||
_put(server, "b.archive", "Song", "Artist") # same work as a
|
||||
_put(server, "c.archive", "Other", "Artist")
|
||||
assert client.get("/api/library/stats").json()["total_songs"] == 3
|
||||
assert client.get("/api/library/stats", params={"group": 1}).json()["total_songs"] == 2
|
||||
|
||||
|
||||
def test_sort_letters_counts_works(client, server):
|
||||
_put(server, "a.archive", "Song", "Alpha")
|
||||
_put(server, "b.archive", "Song", "Alpha") # same work
|
||||
_put(server, "c.archive", "Track", "Beta")
|
||||
sl = client.get("/api/library/stats",
|
||||
params={"group": 1, "sort_letters": 1, "sort": "artist"}).json()["sort_letters"]
|
||||
assert sl.get("A") == 1 and sl.get("B") == 1
|
||||
|
||||
|
||||
# ── Keyset paging under grouping (the critical integration) ───────────────────
|
||||
|
||||
def test_grouped_keyset_pagination(client, server):
|
||||
for i in range(5):
|
||||
_put(server, f"w{i}_a.archive", f"Song {i}", "Artist")
|
||||
_put(server, f"w{i}_b.archive", f"Song {i}", "Artist") # 2 charts per work
|
||||
body = client.get("/api/library", params={"group": 1, "size": 3}).json()
|
||||
assert body["total"] == 5 and len(body["songs"]) == 3
|
||||
cur = body["next_cursor"]
|
||||
assert cur
|
||||
body2 = client.get("/api/library", params={"group": 1, "size": 3, "after": cur}).json()
|
||||
assert len(body2["songs"]) == 2
|
||||
p1 = {s["filename"] for s in body["songs"]}
|
||||
p2 = {s["filename"] for s in body2["songs"]}
|
||||
assert not (p1 & p2) # no skip / dupe across pages
|
||||
|
||||
|
||||
# ── Freshness: adding a chart regroups; overrides survive reindex ─────────────
|
||||
|
||||
def test_new_chart_updates_group_size(client, server):
|
||||
_put(server, "a.archive", "S", "A")
|
||||
assert _grouped(client)["songs"][0]["chart_count"] == 1
|
||||
_put(server, "b.archive", "S", "A") # dirties read-model
|
||||
body = _grouped(client)
|
||||
assert body["total"] == 1 and body["songs"][0]["chart_count"] == 2
|
||||
|
||||
|
||||
def test_preferred_survives_reindex(client, server):
|
||||
_put(server, "a.archive", "S", "A", arrangements=2)
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
wk = server.meta_db.work_key_for("b.archive")
|
||||
server.meta_db.set_chart_preferred(wk, "b.archive")
|
||||
assert _grouped(client)["songs"][0]["filename"] == "b.archive"
|
||||
# A rescan re-indexes both charts (INSERT OR REPLACE INTO songs).
|
||||
_put(server, "a.archive", "S", "A", arrangements=2)
|
||||
_put(server, "b.archive", "S", "A", arrangements=1)
|
||||
assert _grouped(client)["songs"][0]["filename"] == "b.archive" # pref survived
|
||||
|
||||
|
||||
# ── work_key normalization folds trivial differences ─────────────────────────
|
||||
|
||||
def test_work_key_folds_the_and_punctuation(server):
|
||||
wk = server.meta_db._work_key
|
||||
assert wk("The Beatles", "Hey Jude") == wk("beatles", "hey jude!")
|
||||
assert wk("AC/DC", "T.N.T.") == wk("acdc", "tnt")
|
||||
assert wk("Metallica", "One") != wk("Metallica", "Two")
|
||||
Reference in New Issue
Block a user