refactor(stores): lean setlist shape with reactive catalog overlay#96
Open
silverbucket wants to merge 3 commits into
Open
refactor(stores): lean setlist shape with reactive catalog overlay#96silverbucket wants to merge 3 commits into
silverbucket wants to merge 3 commits into
Conversation
Setlist entries now reference the catalog by id instead of cloning catalog fields. A `displayedSetlist` / `displayedSavedSetlists` derived runs scoreFixedOrder against the live catalog, so RS pulls and local edits propagate to every screen via Svelte reactivity — no manual sync helpers, no mirror blocks. Reorder/remove/add collapse to lean array splices since the derived handles scoring. Saved setlists migrate from the fat shape via rs-migrate v2; the localStorage current-set normalizes on read. loadSavedSetlist drops songs no longer in the catalog with a toast.
Setlist entries are now { songId, performance } — fat-shape songs
embedded in fixtures broke every test that opens the modal or checks
song count. Two fixes:
- roll-survives-sync: setlistIds reads s.songId instead of s.id
- saved: setlistFixture songs converted to lean shape; fixtureCatalogSongs /
tallCatalogSongs helpers seed the catalog so hydrateSetlist can resolve
them; anxiety assertion updated (score is now computed live, not stored)
…aved modal confirmOptimizeOrder() was reading generatedSetlist.songs (lean shape) for cover/instrumental counts and fixedSongIds. All three fields are undefined on lean entries, producing an all-undefined fixedSongIds array and zero-capped covers/instrumentals. Switch to displayedSetlist.songs which is already the hydrated form the generator expects. SavedScreen viewingSet stored a hydrated snapshot on open, so a catalog rename or RS sync after opening the modal would not be reflected until close/reopen. Replace the $state object with a $state id + $derived lookup against store.displayedSavedSetlists so the modal always shows live catalog data.
📝 WalkthroughWalkthroughThis PR refactors setlist persistence to separate lean (songId + performance) from displayed (scored + hydrated) formats. A new v2 migration converts persisted setlists; hydration derives display-ready setlists from the live song catalog. Components now consume displayedSetlist/displayedSavedSetlists, and all persistence workflows migrate through the new schema. ChangesLean Setlist State Architecture
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/saved.spec.ts (1)
462-462: ⚡ Quick winRemove legacy
songCountfrom the tall fixture payload.Line 462 still injects
songCountinto a v2 lean fixture. Keeping this legacy field in default E2E data weakens schema-contract coverage and can hide accidental dependencies on removed fields.Suggested diff
- return setlistFixture({ id: "tall", name: "The Marathon", songs, songCount }); + return setlistFixture({ id: "tall", name: "The Marathon", songs });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/saved.spec.ts` at line 462, The tall fixture creation is still passing the legacy songCount property into setlistFixture (the call with id "tall" and name "The Marathon"); remove the songCount field from that payload so the v2 lean fixture no longer includes the deprecated property (i.e., update the setlistFixture invocation for the "tall" fixture to omit songCount).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/stores/app.svelte.js`:
- Around line 1382-1383: When persisting setlists, don't save
generatedSetlist.songs verbatim; instead prune out IDs that no longer exist in
the current catalog before cloning and persisting. Update the two save sites
that call clone(generatedSetlist.songs) / write the setlist to first filter
generatedSetlist.songs against the active catalog (e.g., catalog or
catalog.songs / catalogById) so only catalog-valid song references are included,
then clone the filtered array and persist that result.
---
Nitpick comments:
In `@tests/e2e/saved.spec.ts`:
- Line 462: The tall fixture creation is still passing the legacy songCount
property into setlistFixture (the call with id "tall" and name "The Marathon");
remove the songCount field from that payload so the v2 lean fixture no longer
includes the deprecated property (i.e., update the setlistFixture invocation for
the "tall" fixture to omit songCount).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d8979ba-52d8-48b0-a6c7-43143b6f7a8f
📒 Files selected for processing (7)
src/lib/components/roll/RollScreen.sveltesrc/lib/components/saved/SavedScreen.sveltesrc/lib/migrations.jssrc/lib/stores/app.svelte.jssrc/lib/stores/app.test.jstests/e2e/roll-survives-sync.spec.tstests/e2e/saved.spec.ts
Comment on lines
+1382
to
1383
| songs: clone(generatedSetlist.songs), | ||
| }); |
There was a problem hiding this comment.
Persist only catalog-valid song references when saving setlists.
At Line 1382 and Line 1414, saving uses generatedSetlist.songs directly. That can re-persist deleted-song IDs (they’re filtered in display hydration, not always pruned from generatedSetlist), so saved docs keep stale refs and users repeatedly hit dropped-song warnings on load.
Suggested fix
async function saveCurrentSetlist() {
if (!generatedSetlist) return;
const currentSaved = savedSetlists || [];
+ const persistedSongs = (generatedSetlist.songs || [])
+ .filter((e) => songsById.has(e.songId))
+ .map((e) => ({ songId: e.songId, performance: e.performance || {} }));
// If this setlist was loaded from a saved entry, update that entry in
// place instead of creating a duplicate with a new id and name.
if (loadedSavedId) {
const existing = currentSaved.find((s) => s.id === loadedSavedId);
if (existing) {
await updateSavedSetlist(loadedSavedId, {
savedAt: nowIso(),
seed: generatedSetlist.seed,
minimumsRelaxed: !!generatedSetlist.minimumsRelaxed,
openerFilterRelaxed: !!generatedSetlist.openerFilterRelaxed,
closerFilterRelaxed: !!generatedSetlist.closerFilterRelaxed,
- songs: clone(generatedSetlist.songs),
+ songs: persistedSongs,
});
setlistSaved = true;
return;
}
// Saved entry no longer exists (deleted elsewhere) — fall through.
loadedSavedId = "";
@@
const entry = {
id: uid("set"),
name: randomName,
savedAt: nowIso(),
schemaVersion: 2,
seed: generatedSetlist.seed,
minimumsRelaxed: !!generatedSetlist.minimumsRelaxed,
openerFilterRelaxed: !!generatedSetlist.openerFilterRelaxed,
closerFilterRelaxed: !!generatedSetlist.closerFilterRelaxed,
- songs: clone(generatedSetlist.songs),
+ songs: persistedSongs,
};Also applies to: 1414-1415
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/stores/app.svelte.js` around lines 1382 - 1383, When persisting
setlists, don't save generatedSetlist.songs verbatim; instead prune out IDs that
no longer exist in the current catalog before cloning and persisting. Update the
two save sites that call clone(generatedSetlist.songs) / write the setlist to
first filter generatedSetlist.songs against the active catalog (e.g., catalog or
catalog.songs / catalogById) so only catalog-valid song references are included,
then clone the filtered array and persist that result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{ songId, performance }) instead of cloning catalog fields. Display goes throughdisplayedSetlist/displayedSavedSetlistsderived, which runsscoreFixedOrderonce against the live catalog — RS pulls and local edits flow into every screen via Svelte reactivity, no manual sync.syncSavedSongIntoSetlist,stripEnergy, and thesaveSongmirror block.scoreFixedOrderwas being called from four places (generator, reorder, remove, add); now it lives inside the derived only. Mutation helpers collapse to array splices.current-setnormalizes on read. Loading a saved setlist whose songs were deleted from the catalog drops them silently with a toast.Why
Original report: RS-driven song changes updated the Songs view but not the Roll screen. Initial fix was an imperative mirror (
syncCatalogIntoSetlists); on review that was patching the symptom — the live and saved setlists embedded deep clones of catalog fields, so reactivity couldn't propagate. This refactor makes the catalog the single source of truth and lets Svelte's reactive overlay do the work.Notable
Test plan
npm test— 291/291 pass (existing) + new migration tests.npm run lint— no new warnings.current-setlocalStorage entry → normalizer upgrades it on read; setlist renders.Summary by CodeRabbit
Refactor
Tests
Chores