Support multiple player profiles with switching by carochacs · Pull Request #3 · get-flashbacks/feedBack

carochacs · 2026-07-31T21:19:29Z

What

Adds "who's playing" style multi-profile support so more than one person can share a feedBack install without mixing progress. Player identity, XP/level/streak, per-song practice stats, favorites, and career progression (paths/challenges/quests/wallet/shop) — plus the achievements/Feats plugin's own local store — are now scoped per-profile instead of one implicit device-wide profile.

New profiles table replacing the old CHECK(id=1) singleton profile row; a device-local active_profile pointer (no auth system exists to hang a session off, so switching is a full-page reload rather than per-request).
New endpoints: GET /api/profiles (list), POST /api/profiles (create), POST /api/profiles/{id}/activate (switch), DELETE /api/profiles/{id} (delete a profile and everything scoped to it — refuses to delete the active or the only remaining profile).
Existing endpoints (/api/profile, /api/stats, /api/progression, /api/shop, …) are unchanged in shape — they now implicitly operate on whichever profile is active, so no caller elsewhere in the app (including the career plugin, which reads meta_db directly) needed to change.
Achievements plugin's own sqlite tables (unlocks, counters, comp_ledger) get the same profile_id treatment.
Frontend: a "Switch profile" button on the Profile screen opens a picker to switch or create profiles; switching does a full page reload.
Upgrading installs migrate their existing single profile's data to profile 1 automatically (idempotent schema migration, verified against a hand-built pre-migration DB), with zero behavior change until a second profile is added.

Known v1 scope boundary, called out explicitly rather than silently dropped: playlists/collections and saved practice loops remain device-wide (not yet per-profile) — a documented follow-up.

feedpak surface

This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout) — this only touches the app's own SQLite metadata store (web_library.db, achievements.db), not the sloppak/feedpak chart format.

Checklist

CHANGELOG.md [Unreleased] updated (user-visible changes)
Tests added/updated for new behaviour (profile CRUD, cross-profile isolation for identity/XP/stats/wallet/shop/achievements, upgrade-migration smoke test)
Commits are DCO signed off (git commit -s)

Validation

Full test suite: 2714 passed, 4 skipped (unrelated missing optional deps), 0 failed.

Generated by Claude Code

carochacs

Reviewed the full diff (metadata_db.py schema/migration, lib/routers/profile.py, plugins/achievements/routes.py, static/v3/profile.js, tests).

No TODOs/NotImplementedError; profile scoping is consistently applied across favorites, XP, streaks, progression, wallet/shop via idempotent migration helpers that read live schema rather than hardcoding columns.
delete_profile correctly refuses to delete the active or last remaining profile, with lock-guarded cleanup across all associated tables.
Follows repo conventions: flat imports, threading.Lock around writes, parameterized SQL throughout, vanilla JS, no new frontend frameworks.
No auth-bypass or path-traversal concerns (this is explicitly a non-auth "who's playing" picker, per the PR). display_name is escaped client-side before insertion into innerHTML — no stored-XSS surface. All SQL uses parameterized placeholders.
Minor non-blocking note: activate_profile's existence check + insert/update inside _lock isn't strictly atomic against a concurrent delete_profile of the same id — low risk given the single-device usage model, and no unsafe SQL either way.

No blocking issues found — clear to merge once out of draft. (Could not submit as an "Approve" — GitHub blocks self-approval since this PR and the reviewing account share the same author.)

Generated by Claude Code

coderabbitai · 2026-08-10T11:43:38Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fe4e2373-43b3-48c1-b647-eb51cfb4dc61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pullfrog · 2026-08-10T11:44:05Z

origin/main is ec1157a, and 03e1c1d is reported as not an ancestor of it locally — yet it's a main-merged PR. The local clone is likely shallow, which distorts rev-list. CI (full-depth checkout) sees the true range as just {5d9dcc5, 566e75d} — only 5d9dcc5 is genuinely flagged. Let me confirm shalowness before rewriting.

Task list (1/6 completed)

Checkout PR branch and fetch check suite logs
Verify the failure was introduced by this PR (not pre-existing)
Diagnose the CI failure
Fix the issue and verify locally with the exact CI command
Commit and push changes to the existing PR
Report progress with diagnosis and fix summary

^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

deepsource-io · 2026-08-10T11:44:26Z

DeepSource Code Review

We reviewed changes in bf80de3...5c267f1 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade Focus Area: Security	Security Reliability Complexity Hygiene

Feedback

Nullable state leaking through the call graph

The type issues all connect back to None flowing where an int, dict, or tuple is expected, and then being indexed or unpacked (get_collection, tunings, plugins, song).
It’s one underlying question: which things are truly optional vs guaranteed? Tightening that contract once would simplify the whole chain.

Database boundary assumptions

Both the SQL injection warnings and the next()-without-guard issues live around metadata DB access and query handling.
Worth thinking of the DB layer as untrusted in both directions: sanitize what you send, and treat iteration / lookups as potentially empty or failing.

Code Review Summary

Analyzer	Updated (UTC)	Details
Docker	Aug 16, 2026 3:12p.m.	Review ↗
JavaScript	Aug 16, 2026 3:12p.m.	Review ↗
Python	Aug 16, 2026 3:12p.m.	Review ↗
Shell	Aug 16, 2026 3:12p.m.	Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

pullfrog · 2026-08-15T04:38:45Z

Fixed the failed core-commit-labeling check on PR #3 and pushed the corrected history.

Diagnosis. The only failing job (core-commit-labeling, from .github/workflows/fork-audit.yml) flagged commit 5d9dcc5 (feat(profile): support multiple player profiles with switching): it touches core paths (lib/metadata_db.py, lib/routers/profile.py, static/v3/profile.js) but its subject lacks the required core:/hook:/sync:/fix: prefix (docs/fork-maintenance.md Rule 2). No code or test failure — the PR's own content is fine.

Fix. A follow-up commit cannot satisfy the check (it iterates every commit in BASE..HEAD), so the offending commit's subject had to be rewritten in place. I rebuilt the branch's history on the shallow clone:

Relabeled 5d9dcc5's subject to core(profile): support multiple player profiles with switching (via amend, preserving author, date, body, and tree 6549b54d).
Recreated the 566e75d merge commit with the same tree (081536bd) and parents, preserving its message.
Recreated the tip 0e70e76 with its tree (00e3525a) unchanged.
Force-pushed to claude/feedback-profile-swap-mul3fs (now at 7349362).

Verification. Each rewritten commit's tree hash matches the original, so the PR's content is byte-identical. The merge remains a tree no-op vs its first parent (exempt from the check), the tip touches only plugins/achievements/routes.py (excluded), and the relabeled subject matches the workflow's regex — the check now passes. Re-running CI on the new head should go green.

Task list (7/7 completed)

Checkout PR Support multiple player profiles with switching #3 branch
Fetch check suite logs to diagnose failure
Verify failure was introduced by this PR
Diagnose and fix the issue
Verify fix by re-running CI command locally
Commit and push changes to PR Support multiple player profiles with switching #3
Report progress

^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

Player identity, XP/level/streak, per-song practice stats, favorites, career progression (paths/challenges/quests/wallet/shop), and achievements/Feats were all keyed to an implicit single device-wide profile (several tables even had a CHECK(id=1) constraint baked in). Households sharing one feedBack instance had no way to keep separate progress, so this adds real multi-profile support: a `profiles` table, a device-local active-profile pointer, and profile_id scoping threaded through every one of those subsystems (including the achievements plugin's own sqlite tables). Existing installs migrate their single profile's data to profile 1 automatically, with zero behavior change until a second profile is added. New endpoints: GET/POST /api/profiles, POST /api/profiles/{id}/activate, DELETE /api/profiles/{id}. Existing endpoints are unchanged in shape — they now implicitly read/write whichever profile is active. A "Switch profile" button on the Profile screen opens a picker to switch or add profiles; switching reloads the page since there's no session/auth system to scope a switch to more narrowly. Playlists/collections and saved practice loops remain device-wide for now (documented as a known v1 gap, not silently dropped).

…le-swap-mul3fs # Conflicts: # CHANGELOG.md

…hievements plugin

pullfrog

Important

The new 0e70e76 commit (sha1→sha256 in the achievements plugin's _chart_key) breaks an existing test and silently resets persisted per-chart play counters on upgrade. tests/plugins/achievements/test_routes.py:82 pins _chart_key to the sha1 digest and now fails (verified locally — this makes the PR's pytest check red), and because chart_plays:* keys are persisted in the counters table, swapping the hash orphans every previously accumulated Encore play count on an upgrading install — with no real security gain, since the digest is truncated to 16 hex chars and the input isn't attacker-controlled. Details inline on plugins/achievements/routes.py:311.

Reviewed changes

0e70e76 — Swapped the achievements plugin's per-chart counter key hash from sha1 to sha256 (docstring + _chart_key return line in plugins/achievements/routes.py), with no accompanying test update or counter migration.
566e75d — Merged origin/main into the branch; the range-diff contains no PR-specific code from this merge.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

…ash to sha1 - create_profile()/activate_profile() now read the row back under the same lock instead of after releasing it, closing the (narrow) window where a concurrent delete_profile() could remove the row first and raise an unhandled StopIteration. - _chart_key() reverts sha256 back to sha1: the digest is truncated to 16 hex chars either way, so the truncation defines the collision space regardless of algorithm, and the input isn't attacker-controlled. The sha256 swap broke test_chart_key_is_stable_not_builtin_hash (still pinned to sha1) and would have silently reset every upgrading install's persisted Encore Feat counters.

deepsource-io · 2026-08-16T15:14:48Z

+            # Read back under the same lock — a concurrent delete_profile()
+            # landing between commit() and an unlocked list_profiles() call
+            # could otherwise remove the just-created row before we read it.
+            return next(p for p in self.list_profiles() if p["id"] == pid)


Call to `next()` should be wrapped in `try-except`

Calls to next() should be inside try-except block.

deepsource-io · 2026-08-16T15:14:48Z

+            self.conn.commit()
+            # Same race as create_profile() — read back before releasing the
+            # lock so a concurrent delete_profile() can't remove the row first.
+            return next(p for p in self.list_profiles() if p["id"] == profile_id)


Call to `next()` should be wrapped in `try-except`

Calls to next() should be inside try-except block.

deepsource-io · 2026-08-16T15:14:48Z

+        cols = ("profile_id",) + self._STATS_COLS
        with self._lock:
            rows = [dict(zip(cols, r)) for r in self.conn.execute(
                "SELECT " + ", ".join(cols) + " FROM song_stats").fetchall()]
