Uh oh!
There was an error while loading. Please reload this page.
feat(gamification): XP, levels, achievements catalog, and leaderboards - #505
Conversation
Warning Review limit reached
Next review available in:29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis pull request adds gamification across the backend and frontend. It introduces XP, levels, streaks, achievements, leaderboards, activity views, friendships, icon uploads, admin controls, migration recovery, and cross-platform local test tooling. ChangesGamification platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 8304bd6 | Commit Preview URL Branch Preview URL | Aug 12 2026, 12:59 AM |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Turns the half-built achievements feature into a full growth system: an append-only XP ledger, levels mapped to the eleven growth stages, three leaderboards, an activity dashboard, and an admin wiki in the existing admin console tab. Resolves three contradictions in the source design: STAGE_MIN vs stageFor() thresholds (STAGE_MIN wins, stored in growth_stages), the XP curve (the mock's per-user numbers imply levels getting cheaper -- the stage sheet is coherent and is adopted instead, 29,800 XP to L50), and streak freezes (no mechanic exists anywhere; cut from v1). Also records that Sapling has no friends model today, so the friends leaderboard scope and its two achievements need one built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… them Jack's call: the final catalog is exactly the design's 30, so the five existing badges with no design equivalent (documents_5, documents_25, quizzes_10, flashcards_50, post_count_50) are deleted rather than kept as retired rows. user_achievements.achievement_id cascades, so any earned rows for those five go with them -- recorded explicitly in the spec. Drops the is_retired column with them; it existed only to preserve those five, and the wiki's existing delete action already covers removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oards
18 tasks, TDD throughout, each ending in a commit.
Revises the spec for the work-in-progress catalog Jack asked for: achievements
gain a `status` ('draft' | 'live'). The design's 30 come in live; the 10
already seeded flip to draft and become the wiki's work-in-progress list.
Nothing is deleted or remapped, so no earned row is cascaded away and the
prod users holding first_login / documents_5 / documents_25 keep them.
Drafts are invisible to users, never trigger-evaluated, and excluded from
"N of M" badge counts.
Two gaps found while checking the plan against the code:
- nothing ever advanced users.streak_count (initialised to 0, only read), so
the hero streak tile and four streak achievements would read zero forever.
Task 6b adds services/streak_service.py as its sole writer.
- `school` is not a column on user_profiles after the 0024 identity split, so
the school leaderboard resolves peers via academics.school_peer_user_ids,
the same fail-closed path GET /api/social/students uses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The stage bands sum to 30,000, not 29,800 — an arithmetic slip in the prose. The growth_stages table, which is the authoritative source for level maths and what migration 0043 seeds, was always correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
growth_stages is the single source of truth for level maths — the hero card, the leaderboard's stage label and the `level` achievement trigger all read the curve through here. Cached with lru_cache plus a clear_growth_cache() hook wired into the autouse _clear_lru_caches fixture, per #98. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_band_for_level scanned _bands() with an early break, which is only correct if bands are ascending by min_level. sort_order and min_level are independent columns (0043_gamification.sql ties neither to the other) that merely happen to agree in the current seed; a future reorder of sort_order without a matching min_level change would silently return the wrong band past the divergence. - _bands() now sorts explicitly by min_level before computing spans. - Add a regression test with sort_order/min_level disagreeing, which fails on the old code (band lookup returns the wrong stage). - Reword stages() docstring: the copy is per-row shallow (dict(r)), not deep — sufficient because growth_stages rows are flat. - Comment the level_for_xp break: only the terminal band is expected to have a non-positive per-level cost; a misconfigured non-terminal band would strand the user at that level with no error, not loop.
Single chokepoint for every XP grant: writes to the append-only xp_events ledger keyed on a deterministic idempotency_key, then refreshes users.total_xp/level. A 409 from the unique index means already-paid and is a clean no-op that leaves users untouched. award_xp_safe wraps this for request paths that must not fail on XP bookkeeping.
Wires services.xp_service.award_xp_safe into the four routes that earn XP, each keyed on the id of the row just persisted (attempt/document/ note/session id) so retries are idempotent no-ops rather than double payouts. documents.py awards inside the shared _persist_document helper so the streaming /upload and /upload/sync twin each pay out exactly once per logical upload. Pre-existing check_achievements calls in quiz.py/learn.py (an older achievement system) were left as-is, no duplicate imports added.
The prior test_xp_wiring.py only exercised services.xp_service directly; the 190 route tests pass regardless of a typo'd rule_key or a swapped source_id because the autouse hermetic Supabase client makes any unmocked services.xp_service.table call a silent no-op. Add one route test per earning path (quiz submit, document upload/sync, note create, session end) that patches services.xp_service.table with an enabled rule and asserts the exact xp_events.insert payload against an independently-known earning id, plus one negative test (the pending- session early return) proving a failed/unpersisted action awards nothing. Test-only; no route code changed.
Adds the 13 new trigger types for the 30-badge catalog seeded by migration
0044 (flashcards_reviewed, concepts_mastered, courses_with_mastery,
graph_nodes_count, friends_count, level, session_minutes,
session_before_hour, session_after_midnight, xp_in_day, goal_streak,
owned_room_members, rooms_active, room_replies).
check_achievements now resolves and status-checks the achievement row
BEFORE granting, so 'draft' badges (10 pre-existing ones left in that
state by 0044) can never be awarded to a user. On a live grant it now
pays the badge's xp_reward via xp_service.award_xp_safe.
Return shape changes from list[str] slugs to list[dict] ({"slug",
"name", "xp"}) — verified by grep that every existing call site
(admin.py, auth.py, documents.py, flashcards.py, learn.py, quiz.py,
social.py) discards or merely forwards the list without treating
elements as strings, so no caller changes were required.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>…course_grade_a trigger Review follow-up on the achievement-trigger work: - _goal_streak now clamps the stored daily_goal_xp to at least 1. The column has no CHECK enforcing positivity (0043), and int(... or 50) only guards falsy values, not negatives — a stored goal of 0 or less made `0 >= goal` trivially true forever, so the backwards day-walk never terminated (confirmed via RED: an unclamped -1 goal blows through the loop until date underflows with OverflowError). - Adds the course_grade_a trigger (migration 0044 seeds a 'grade-a' badge on it that was previously unearnable — _get_user_stat had no branch for it and silently fell through to 0). Counts the user's enrollments whose current computed letter grade is an A variant (A, A-, or A+ under a custom scale), reusing gradebook_service.current_grade / letter_for rather than reimplementing the grade math, with points_possible/points_earned decrypted the same way routes/gradebook.py's _load_assignments does. Also pins the session_before_hour "hours before 24" inversion with an explicit unit test per review request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rom users users.streak_count was initialised to 0 and only ever read; nothing advanced it, so streak-based achievements (on-fire, marathon, wildfire) and the hero card streak tile read zero forever. services/streak_service.py::touch_streak is now the sole writer, called via touch_streak_safe from routes/learn.py's end_session beside the session_completed XP award (same commit point, same pending-session early return, same-day idempotency mirroring the xp_events key). touch_streak is idempotent within a UTC calendar day, increments on a consecutive day, resets to 1 after a gap, and longest_streak only ratchets up. Also scopes routes/profile.py's get_achievements to status=live so migration 0044's draft badges stop appearing in user-facing reads; user_achievements rows are untouched, so a badge earned before being drafted keeps its row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of task 6b found that services/graph_service.py::update_streak already advanced streak_count from apply_graph_update on every mastery change (reached from learn, notes, documents, quiz and agent tool code) — using local date.today() and never writing longest_streak. Racing against streak_service.touch_streak's UTC calendar day on the same two columns let streak_count flap between a UTC-day and local-day value depending on which path wrote last, and left longest_streak lagging the true peak. graph_service.apply_graph_update's mastery-change branch now delegates to streak_service.touch_streak_safe instead of a duplicate implementation, so there is exactly one definition of "a study day." No import cycle: streak_service only imports db.connection, confirmed by direct import. Adds test coverage in test_graph_service.py: a dedicated assertion that a mastery change advances streak_count/longest_streak via streak_service (UTC semantics), and a reconciliation test (TestStreakReconciliation) with a stateful users-table fake proving a mastery-driven touch followed by a session-end touch on the same UTC day increments exactly once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds friendships/friend_requests-backed endpoints under /api/social/friends
(request, accept, decline, remove, list, requests). Accept writes symmetric
friendship rows and fires check_achievements for both users. Fixes a route-
ordering bug present in the task brief's code block itself: GET
/friends/{user_id} was written before GET /friends/requests, which would
have made /friends/requests unreachable (FastAPI matches "requests" as a
user_id) despite the brief's own prose asserting the opposite order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The friends endpoints (request/accept/decline/remove/list/list-requests)
trusted caller-supplied user_id with no session check, unlike every other
endpoint in social.py. Concretely: anyone could accept/decline someone
else's friend request, tear down another user's friendships, read any
user's friends list, or send a request "as" another user by setting
from_user_id.
Add require_self(user_id, request) to all six endpoints, matching the
file's existing convention (e.g. create_room, join_room). GET
/friends/{user_id} is guarded self-only for now — a friends list may
become shareable later, but the safe default until that's a deliberate
product decision is owner-only. The to_user_id != user_id 403 check in
_load_request stays: it answers "was this request addressed to you",
which is still meaningful once identity is authenticated.
Adds TestAuthGuard: one test per endpoint asserting require_self is
called with the right user_id (patching routes.social.require_self on
top of conftest's autouse no-op stub), plus one proving a guard
rejection actually propagates as 403 rather than being swallowed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>send_friend_request only 409d when an existing (from_user_id, to_user_id)
row was 'pending'. A 'declined' row, or an 'accepted' row that survived
remove_friend (it only deletes friendships rows, not the historical
friend_requests row), fell through to insert() with the same pair and hit
the UNIQUE(from_user_id, to_user_id) constraint from migration 0043 —
db/connection.py's insert() calls raise_for_status() unconditionally, so
the conflict surfaced as an unhandled 500. User-visible path: A asks B,
B declines, they patch things up later, A asks again -> 500. Same for
unfriend-then-refriend.
Fix: when a non-pending row already exists for the pair, UPDATE it back
to pending (clear responded_at, refresh created_at) instead of inserting
a duplicate, returning the same {"request": {...}} shape either way.
pending -> 409 is unchanged.
Confirmed the accepted/declined row remove_friend leaves behind does not
leak into GET /friends/requests: both incoming/outgoing selects already
filter status=eq.pending explicitly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Adds GET /api/gamification/{me,leaderboard,activity}: the read side of the
XP/achievements feature, deriving hero-card progress, weekly leaderboard
ranking (everyone/friends/school scopes, private-profile suppression), and
7-day/8-week activity charts from the xp_events ledger. Mounted in main.py.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>_events_since fetched the full weekly/8-week xp_events window in a single select() with no limit. supabase/config.toml caps PostgREST at max_rows=1000, and PostgREST signals that cut with 206 Partial Content (a 2xx), so raise_for_status() never catches it -- past ~1000 platform-wide weekly events, /leaderboard totals and rank order would silently go wrong for an unpredictable subset. Page to completion via select_with_count with a deterministic created_at,id order (required for offset paging to be correct), and add a regression test that fails against a single-page read. Also covers the previously-untested scope=school leaderboard path, and leaves a comment at each make_etag() call noting daily_goal_xp isn't yet an etag input (inert today; nothing writes that column). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ation
Adds POST /api/admin/achievements/{id}/icon: admins upload a base64
PNG/WebP/SVG, storage_service.validate_icon parses width/height from
the file header (never a client-supplied field) and rejects anything
off-spec, malformed, or truncated with a clean 400. Widens the
update_achievement allowlist to cover icon_url/xp_reward/sort_order/status
added by migration 0043.… WebP coverage _svg_is_square previously accepted the first viewBox= match anywhere in the first 4KB, so a square decoy in a leading comment or on a nested <symbol>/<pattern>/inner <svg> could slip a non-square root icon past server-side validation. It now skips leading whitespace/XML decl/DOCTYPE/ comments, requires the next tag to be the root <svg>, and only reads viewBox from that tag's own attributes — failing closed (reject) on anything else, same as the PNG/WebP header parsers. Also adds a real RIFF/WEBP container builder and coverage for all three _webp_dimensions variants (VP8X/VP8 /VP8L), a wrong-dimension case, and a truncated-header case, which had no tests before. Documents why the shared len(data) < 30 guard is deliberately the max across variants rather than per-variant.
Task 6 stopped the automatic trigger checker from granting non-live badges, but the manual POST /achievements/grant admin path still bypassed that check. Look up the achievement's status before the user_achievements insert: 404 if it doesn't exist, 409 naming the slug and telling the admin to publish it if the status isn't 'live'. Preserves the existing already-earned skip and check_achievements call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add GET /xp-rules and PATCH /xp-rules/{key} so admins can retune
per-action XP amounts and disable rules without a deploy;
services/xp_service.py reads xp_rules at award time. PATCH validates
amount >= 0 and rejects an empty body, both 400. UpdateXpRuleBody
lives in the models/ package alongside the other admin body models.
Note: the routes/admin.py route bodies for this landed in the
previous commit (ba6552a) because `git commit <pathspec>` snapshots
the working-tree content of matched paths, and admin.py had both
this and the grant-gate change pending at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>test_404_if_achievement_missing and test_409_if_achievement_is_draft previously asserted only the HTTP status/detail, so a handler that inserted into user_achievements before 409ing would have passed both. Assert table() was never invoked with "user_achievements" in either case, following the t.return_value.update.assert_not_called() convention in test_cannot_unapprove_self. Verified RED by temporarily moving the insert above the gate in routes/admin.py, confirming both tests fail on the new assertion (not the status code), then reverting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a Friends panel to Social.tsx (friends list with Remove behind useConfirm, Incoming requests with Accept/Decline and a count chip, Outgoing pending list) and an add-friend action to ProfileView.tsx that checks the viewer's own friends/requests to render Add friend, Request sent, or a Friends indicator, sending via sendFriendRequest and handling 409 via the existing toast. Every mutation refetches from the server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds frontend/e2e/gamification.spec.ts (earning XP moves the hero card's total-XP readout, the leaderboard, and the activity tab's week total) plus an awardXp helper in support/db.ts that ports xp_service.py's ledger-insert + total_xp/level cache refresh (including the growth_stages level curve) directly against Postgres, since there's no XP-grant API endpoint. Adds growth_stages/xp_rules to both TRUNCATE_DENYLISTs (db.ts and the pytest integration conftest) — they're migration-seeded reference tables the rich seed never re-inserts, so without the denylist entry every per-test reset would empty the level curve and rules the whole gamification UI depends on. Also fixes the testid convention drift Task 16 left behind: Social.tsx's bare friend-* ids are renamed to social-friend-* (one prefix per surface, per docs/frontend-testids.md), and the profile surface (ProfileView.tsx) plus the new achievements surface (Achievements.tsx + HeroCard/ LeaderboardTab/ActivityTab) are registered in that doc and added to the eslint testid-enforcement file list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re_self
/api/gamification/me, /leaderboard and /activity took user_id as a query
parameter and called no guard. The module never imported auth_guard and
main.py mounts the router without dependencies=, and there is no global auth
middleware — so with no cookie at all, `leaderboard?scope=everyone`
enumerated every user with app-decrypted display names, levels, XP and
streaks; `/me?user_id=<victim>` returned their stats; and
`leaderboard?user_id=<victim>&scope=friends` returned the victim's entire
friends list.
require_self(user_id, request) is now the first statement of all three
handlers, matching routes/social.py and routes/profile.py.
profile.py::get_achievements had the same shape and no guard (pre-existing,
not introduced by this branch) — guarded too. Its payload is per-user
(earned state plus progress counters computed for user_id) and the frontend
only ever calls it for the signed-in user.
Also closes two payload bugs on that endpoint. The select omitted xp_reward,
icon_url, sort_order and status, all of which the frontend Achievement type
declares required; the response is cast and never validated, so tsc passed
while BadgeGrid/BadgeModal rendered "+undefined XP" and iconUrl={undefined},
making every admin-uploaded icon invisible to users. And the showcase embed
in _get_featured_achievements had no status filter (it needs achievements!inner
before a filter can reach the embedded table), so badges the wiki unpublishes
stayed pinned on the showcase while being absent from the grid and the
"N of M" count. export_data stays unfiltered on purpose — a user's own data
export should include everything they earned.
conftest stubs require_self to a no-op for every test, so the guard tests
re-patch it per-module and assert both the call and that a rejection
propagates, the way test_friends_routes.py::TestAuthGuard does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>users.total_xp is a cache of the append-only xp_events ledger, but award_xp derived it as prev_total + value despite the docstring claiming it refreshes from the ledger. That is a read-modify-write with no lock: two concurrent awards read the same prev_total and the second UPDATE discards the first, permanently, with nothing anywhere to reconcile it. It is user-visible, not just theoretical: /activity and the xp_in_day / goal_streak achievement triggers sum xp_events directly, while /me and the leaderboard read the cache — so a user sees an activity chart totalling more XP than their hero card claims. _ledger_total() now sums the user's whole ledger through the same select_with_count pagination pattern routes/gamification.py uses, with the same _XP_EVENTS_PAGE=1000 cap and the same created_at,id stable sort. Both matter: PostgREST signals a truncated page with 206 Partial Content, a 2xx, so raise_for_status never fires and a single unbounded select would have silently reset a heavy user's total to the first page's sum. Recomputing also makes the cache self-healing — whatever it drifted to, the next award lands it back on the ledger sum. Not using a PostgREST server-side aggregate (select=amount.sum()): it would be cheaper but depends on db-aggregates-enabled, and if that is off PostgREST 400s, award_xp raises, award_xp_safe swallows it and XP silently stops being cached. Test fixture consequence: xp_events stubs that were bare MagicMocks now need a real select_with_count, so both shared helpers model an in-memory ledger. test_xp_wiring's mattered most — a bare MagicMock would make the tuple unpack raise *after* the insert those tests assert on, silently retiring their coverage of the tail of award_xp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_load_request checked to_user_id but not status, and the friendships insert had no conflict handling against its PRIMARY KEY (user_id, friend_id). Two routes in, neither needing an adversary: 1. a plain double-click or retry on an already-accepted request; 2. mutual requests — send_friend_request only checks the exact (from, to) pair, never the reverse, so A->B and B->A both go pending; B accepts A->B, then A clicks Accept on B->A. Either way: duplicate key -> unhandled 500. And because the row stayed pending, it 500d on every subsequent retry, permanently. Accept now short-circuits when the request is not pending or the users are already friends. It still resolves the request row in that case — a bare early return would leave a stale pending row surfacing as an actionable incoming request forever. The friendships write is also an upsert on the primary key rather than an insert. The status check alone cannot close the race: a real double-click fires both requests before either has updated the status, so both read pending and only a conflict-tolerant write is actually safe. send_friend_request is deliberately left alone — mutual pending requests stay legal, now that accepting one makes the other a no-op. Auto-accepting a reverse request would silently create a friendship from a click that only meant "send a request". Also wires the room-side achievement dispatch this file owns (room_replies, rooms_active on post; owned_room_members on join/create) — see the following commit. owned_room_members is fired for the room's created_by, not the joiner: Grovekeeper is "build a room five people join", the owner's stat. The pre-existing happy-path accept test relied on an unstubbed friendships.select MagicMock reading as truthy; it now stubs it explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check_achievements only evaluates triggers whose trigger_type equals the event_type it is handed. Task 6 taught _get_user_stat thirteen new trigger types, but nothing in the product ever fired them — so 18 of the 30 badges 0044 makes live could never be earned. Worse than merely unearnable: profile.py's progress bars call _get_user_stat read-only, so a user watched a bar fill to 100/100 next to a badge that stayed locked forever. New call sites, all post-commit side effects that cannot fail the action that earned them: graph_service.apply_graph_update concepts_mastered, graph_nodes_count, courses_with_mastery flashcards.rate_card flashcards_reviewed learn.end_session session_minutes, session_before_hour, session_after_midnight, goal_streak gradebook create/update assignment course_grade_a social (previous commit) room_replies, rooms_active, owned_room_members xp_service.award_xp (earlier commit) level (on level-up), xp_in_day The graph dispatch sits at the end of apply_graph_update rather than beside touch_streak_safe inside `if mastery_changes:` — graph_nodes_count grows when nodes are created, which happens on updates that change no mastery at all. course_grade_a did have a clean hook after all: a letter grade is derived and never stored, so there is no "grade recorded" row, but every write that can move the computed percent goes through create_assignment or update_assignment_route. No hook was invented. Recursion: check_achievements pays xp_reward through award_xp_safe, which re-enters award_xp, so a level-up badge that pays enough XP to level you up again would recurse without bound. award_xp's dispatch sits behind a threading.local re-entrancy flag cleared in a finally — thread-local because the app is threaded and one request's dispatch must not suppress another's, and the finally because a dispatch that raises must still release the flag or every later award on that thread silently stops dispatching. Depth is always 1. The cost is that a payout which itself causes a level-up is not noticed until the user's next award: bounded and self-correcting, versus a loop. check_achievements now drops already-earned triggers before evaluating the stat. Several of these stats are expensive (course_grade_a walks every enrollment's assignments; xp_in_day and goal_streak scan the whole xp_events ledger) and they now run on request paths — flashcard ratings, room posts, grade writes. Once a badge is earned its stat cannot change the outcome. The tests assert the CALL SITE EXISTS — they drive the real route/service and assert check_achievements was called with the expected event type — rather than that _get_user_stat returns the right number, which is the gap that let this survive 18 reviews. test_every_live_trigger_type_has_a_dispatch_site is the durable backstop: it diffs 0044's trigger types against every check_achievements literal in routes/ and services/, so a new trigger type added without a call site now fails the suite. test_the_migration_parses guards the guard, since a regex matching nothing would make it vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
grant_achievement inserted the user_achievements row directly and awarded nothing, while check_achievements pays xp_reward through award_xp_safe on the earned path. mentor, comeback, secret and methuselah are manual-grant-only, so they paid 0 instead of 980 XP and two users with identical badge sets ended up with different totals. Uses the same rule_key and source_id as the earned path, so the shared xp_events idempotency key makes a re-grant (or an earned-then-granted badge) a clean no-op rather than a double payout. award_xp_safe, so XP cannot fail the grant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lifespan created the bucket with allowed_mime_types=ALLOWED_CONTENT_TYPES (jpeg/png/webp/gif), but ICON_CONTENT_TYPES also carries image/svg+xml. Supabase enforces the bucket's MIME list even for service-role writes, so validate_icon accepted an SVG and the PUT then 400d — the admin saw "502 Icon upload failed (Supabase 400)". That contradicts the recorded product decision that SVG stays supported. The bootstrap list is now the union. OPERATOR ACTION, not fixable in code: ensure_bucket_exists treats a 409 as success and deliberately does not overwrite settings, so this only takes effect on buckets this code creates. Staging and prod buckets predate the change and need a one-off bucket update to add image/svg+xml before SVG icon uploads work there. Recorded as an OPERATOR NOTE in main.py's lifespan block so it is discoverable from the code. The existing lifespan test asserted the MIME list equalled ALLOWED_CONTENT_TYPES — that assertion is what pinned the bug in place, so it now asserts the union and that image/svg+xml is present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0044 rebuilds triggers with a DELETE scoped to status = 'live', on the assumption that the live catalog is exactly the ten 0007 seeds it demotes one statement earlier — it is not scoped to those slugs. POST /api/admin/achievements has shipped for a while and creates rows defaulting to status = 'live', so any achievement an admin created through the wiki lost its triggers and became a live, visible, permanently unearnable badge. 0044 is already applied, so it is not edited. 0045 is deliberately DETECTION AND DOCUMENTATION ONLY and mutates nothing: - A data-only repair is not expressible. The triggers were DELETEd; achievement_triggers has no history, tombstone or audit trail, so the (trigger_type, threshold) pairs an admin configured are gone. There is nothing to reconstruct them from, and guessing in SQL is worse. - Auto-demoting affected rows to 'draft' WAS expressible and was rejected: grant_achievement 409s on a draft, so it would silently break an admin who created a badge specifically to hand out manually. So it RAISEs a WARNING naming any live achievement with zero triggers, and is a no-op on any catalog that only ever held the seeded slugs — i.e. every environment where no admin used the wiki before 0044 ran. The header documents the hazard, why it is unrepairable, why demotion was rejected, the operator pre-flight (SELECT slug, status FROM achievements) and the manual wiki repair. No user_achievements rows were touched, by 0044 or by 0045 — nobody lost a badge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_daily_totals did an unbounded select("amount,created_at") over the whole
ledger. PostgREST caps a response at max_rows = 1000 (supabase/config.toml)
and signals it with 206 Partial Content, which is a 2xx, so db/connection.py's
raise_for_status never fires and the truncation is silent.
That was latent until the last wave made award_xp dispatch xp_in_day on every
award. Past ~1000 lifetime events, golden-hour (xp_in_day >= 500) was computed
over an arbitrary truncated prefix and became unearnable, and perfect-week
(goal_streak >= 7) could spuriously reset.
Pages via select_with_count with a deterministic created_at.asc,id.asc order,
matching xp_service._ledger_total (same call path) rather than inventing a
third idiom. Deliberately unwindowed: _best_day_xp is an all-time max, and a
window would cap goal_streak at the window length, breaking any
admin-configured threshold above it. Cost is bounded by _ledger_total, which
already pages the same ledger on every award.
Test: TestDailyTotalsPaging stubs a full page followed by a short page and
asserts both are accumulated, so it fails if only the first page is taken.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>POST /api/social/public-rooms/{room_id}/join fired no check_achievements at
all, while the invite-code join_room fires two. Effect: room-leader
(Grovekeeper, "create a study room five people join") never advanced for an
owner whose members arrived through the invite-less #405 public join, so the
badge was earnable or not depending on which join button the fifth member
pressed.
Mirrors join_room exactly: rooms_joined for the joiner, owned_room_members for
rooms.created_by (the owner, NOT the joining user). The handler's select only
returned id,is_public, so created_by is now selected too. Dispatch is fired
unconditionally rather than only on a fresh membership, matching join_room, and
wrapped so a broken dispatch cannot fail the join.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>0045 states the triggers 0044 deleted "are simply gone - there is nothing in the database to reconstruct them from", and tells the operator to re-add each badge's trigger in the wiki from memory. That is false. Every admin-created trigger was written through routes/admin.py's trigger endpoints, all three of which log to admin_audit_log (0010) with trigger.create / trigger.update / trigger.delete and a payload carrying achievement_id, trigger_type and trigger_threshold. The exact values were on disk the whole time. 0044 and 0045 are already applied and are not edited. 0046 supersedes 0045's guidance: it corrects the record, embeds the reconstruction query for an operator who wants to preview, and performs the repair. It REPAIRS rather than only documents because the values are exact rather than inferred, and the guards make it unable to do harm: it only writes to an achievement that is status='live' AND has ZERO achievement_triggers rows, so it can never overwrite, duplicate or contradict a configured trigger or touch a draft. Rows are restored under their ORIGINAL trigger id with ON CONFLICT (id) DO NOTHING, so it is idempotent twice over and admin_audit_log.target_id keeps resolving. A trigger an admin deleted through the wiki has a trigger.delete tombstone and is deliberately NOT resurrected. Documentation-only was rejected because 0045 already tried that and the advice never reached anyone. Which is the second half of the finding: 0045's only output is a server-side RAISE WARNING and db/migrate.py registered no psycopg notice handler, so the message was discarded before an operator could read it. run() now attaches print_notice, routing NOTICE to stdout and WARNING/ERROR to stderr. Only 0027 and 0045 RAISE anything today and both are applied everywhere, so no existing environment's output changes - it just stops discarding future ones. Verified against the real local schema in a rolled-back transaction: a wiped trigger is restored with its latest PATCHed threshold under its original id, a deliberately-deleted one is not resurrected, an achievement that already has a trigger is untouched, and a second run is a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stent id _count_rows selected "id". room_members is PRIMARY KEY (room_id, user_id) (0001_baseline_schema.sql) and friendships is PRIMARY KEY (user_id, friend_id) (0043_gamification.sql) — neither table has an id column. PostgREST answers a projection over a missing column with 400 (42703), which db/connection.py raises, so rooms_joined, owned_room_members and friends_count did not return a wrong count, they threw. study-circle, room-leader (Grovekeeper), first-friend and popular were unearnable. That also made a55f5b5 inert: the public-room join now dispatches owned_room_members, but the stat it dispatches raised before it could count. Selects user_id instead, which is present on all eight tables _count_rows is called with, including the one call that filters by room_id. Second half: accept_friend_request called check_achievements unguarded, unlike every other dispatch site in social.py (:64, :144, :185). The friendship rows and the request status are committed before it, so the 42703 surfaced as a 500 on a friend-accept that had in fact succeeded. Wrapped to match its siblings. Every existing test in test_achievement_service.py replaces `table` with a bare MagicMock, so the requested column was never checked against anything — which is how this shipped. TestCountRowsSelectsAnExistingColumn gives the fake the real column sets from the migrations and raises PostgREST's 42703 on an unknown projection. Verified RED: all three fail with "column room_members.id does not exist" against the previous line. Found by the scoped re-review of 1230653..90abe49. Pre-existing, not introduced by that range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three Linux-only assumptions made `make e2e-up` unrunnable under Git Bash, so the E2E lane had never been executed on this machine. 1. DOCKER_HOST. Every one of the four scripts exported unix:///run/user/$(id -u)/podman/podman.sock whenever the podman BINARY was present. On Windows podman is present but runs in a VM the CLI reaches through its own default connection, and that socket path does not exist — so the export pointed the Supabase CLI at nothing and every command failed with "Cannot connect to the Docker daemon". Verified: `supabase status` fails with the export and succeeds without it. Replaced by set_docker_host_for_podman in local-common.sh, which additionally requires the socket to exist (-S). A pre-set DOCKER_HOST still always wins and is never recomputed, which .github/workflows/e2e.yml depends on: ubuntu-latest ships podman alongside Docker and pre-sets DOCKER_HOST to the real Docker socket. 2. venv layout. The preflight tested `-x backend/venv/bin/python` and the migrate/seed steps shelled out to `venv/bin/python`. A Windows venv puts the interpreter in venv/Scripts/python.exe, so e2e-up died in preflight. $VENV_PY is now resolved once in local-common.sh, accepts both layouts, and an explicit $VENV_PY still wins. 3. setsid. Both servers launched under setsid so the recorded PID leads a process group that e2e-down kills as a unit. Git Bash has no setsid. e2e-up now degrades to a plain background job with a printed note, and e2e-down gains a taskkill /T process-tree fallback — reached only if the PID survives the POSIX group/plain kills, so Linux and CI never touch it. Without that fallback, killing the `npm run start:test` wrapper would orphan next-server still holding :3000 and the next e2e-up would fail its port preflight. local-up.sh and local-db-reset.sh carried the same DOCKER_HOST line and are fixed identically; they already picked up $VENV_PY through migrate_reload_seed. scripts/explore.sh (Chapter 2) still hardcodes setsid and venv/bin/python and is NOT covered here. Verified on Windows: all five scripts pass bash -n, and sourcing local-common resolves VENV_PY to backend/venv/Scripts/python.exe, CONTAINER_CMD to podman, and leaves DOCKER_HOST unset. The Linux path is unchanged by construction: the socket exists there, so set_docker_host_for_podman exports exactly what the old line did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g it
Found by running the E2E lane for the first time: gamification.spec.ts failed
with the hero card still reading "0 XP total" after XP was earned and the page
reloaded. The DB and the API were both correct — users.total_xp was 30 and
GET /api/gamification/me returned total_xp 30 when asked. The browser simply
never asked.
The three gamification routes shipped with http_cache.CACHE_CONTROL,
"private, max-age=30, stale-while-revalidate=60". Inside that 30s freshness
window Chromium answers from its own cache without a request, so the ETag —
which was already correct and did include total_xp — never got the chance to
invalidate anything. The backend log shows it exactly: the reload refetched
/api/profile/{id}/achievements but issued no /api/gamification/me at all, and
the next one landed 37 seconds later.
max-age=30 is right for data the user does not immediately cause to change.
It is wrong for a live counter: sub-30s feedback is the entire point of the XP
surface, and a user who just finished a quiz and opened /achievements would
see the old number.
conditional() and cached_json() take an optional cache_control (default
unchanged, so every other #99 route keeps its 30s window), and /me,
/leaderboard and /activity pass REVALIDATE_CACHE_CONTROL = "private, no-cache".
no-cache does not disable caching — it requires revalidation before reuse, so
an unchanged read is still a cheap 304 and only the no-ask window goes away.
The 304 path takes the same directive as the 200 deliberately: a 304 refreshes
the stored response's headers, so returning the default there would re-grant a
fresh 30s no-ask window on the next revalidation and reintroduce the bug one
request later. TestLiveCountersRevalidate asserts both halves.
Verified against the live stack: the journey that caught it now passes, and the
full lane is 38/38 with the oracles reporting 0 findings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>…ult ports Second half of the Windows port (f36228c), all found by actually booting the lane rather than reading the scripts. 1. db/migrate.py read migration files with the platform default codec, which is cp1252 on Windows, and died with UnicodeDecodeError 0x90 partway through the chain. This is the same one-character fix as e6311f9 on fix/staging-deploy-env-hardening, applied identically so the pending merge between the two branches stays trivial. 2. Ports. This dev box excludes TCP 54288-54788 wholesale (six WinNAT/Hyper-V reservations), which swallows the API (54321), DB (54322) and Studio (54323) ports. Binding one fails with "An attempt was made to access a socket in a way forbidden by its access permissions" while every container reports healthy — the containers are fine, the host just cannot reach them. Verified directly: 54321/54322 refuse an explicit TcpListener bind, 55421/55422 accept. $SUPABASE_DB_PORT / $SUPABASE_API_PORT now drive the DB URL and the PostgREST health poll, defaulting to the documented values so Linux and CI are unaffected. Shifting supabase/config.toml's ports to match is a local, uncommitted change — the exclusion is specific to this machine. 3. $FRONTEND_PORT is overridable and passed through to `next start`, so the lane can boot while another worktree's `next dev` holds :3000. Nothing else hardcodes the frontend's own port: build:test only bakes BACKEND_URL, and the browser reaches the API same-origin through Next. The Playwright harness follows via the E2E_FRONTEND_URL that support/stack.ts already supported. 4. build:test hardcoded NEXT_PUBLIC_SUPABASE_URL, which as a command-prefix assignment beats the environment and cannot be overridden. Now ${NEXT_PUBLIC_SUPABASE_URL:-http://127.0.0.1:54321} — same default, same POSIX-shell requirement the script already had. Not changed, worth knowing for anyone repeating this on Windows: npm scripts need a POSIX shell for those prefix assignments (export npm_config_script_shell=<git-bash>), and the Playwright re-seed fixture needs E2E_SEED_PYTHON pointed at venv/Scripts/python.exe — support/db.ts already had that override, so no change was required there. Result: the lane boots and is 38/38 green with the oracles at 0 findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e2e-down left next-server running on a clean teardown, orphaning the port. Without setsid, $! is the `npm` wrapper, and npm exits as soon as next-server is spawned. By teardown the recorded PID is already dead, so stop_tracked's `kill -0` guard skips the whole branch — including the taskkill /T fallback added in f36228c, which only runs when the PID is still alive. next-server survives, keeps the port, and the next boot cannot bind it. Observed once: `e2e-down.sh` reported success having stopped only uvicorn, and :3001 stayed held until the tree was killed by hand. By the time the health check passes, the process listening on that port is unambiguously the one we just started, so record it instead of the wrapper. Guarded to the no-setsid path — under setsid the recorded pid leads the process group and is already correct — and to netstat being present. Deliberately does NOT sweep the port by owner at teardown: that would kill whatever holds the port, which on this machine is how another worktree's `next dev` on :3000 would get taken out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was written before the NNNN_ -> YYYYMMDDHHMMSS_ cutover and carried 0043-0046. main froze the legacy set at 48 files and guards the count in tests/test_migration_naming.py, so rebasing onto main took the tree to 52 and failed that guard. The four are renamed to their authoring timestamps, which preserves the original apply order. Safe to rename here specifically because the branch is unmerged: schema_migrations keys on basename, and the auto-migrate job (#506) only runs on merge to main, so these have never been recorded in a shared ledger under the old names. Local and E2E databases need a reset. The files cross-reference each other by bare number in ~23 places, including operator-facing RAISE WARNING strings. Rather than rewrite that prose, each file gets a header note mapping it back to its old name so those references still resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase fallout, and the kind a clean merge hides. main added test_migrate_work_mem.py with a _FakeConn modelling only what its own run() touched (cursor, commit). This branch's run() also calls attach_notice_handler(conn), so the double raised AttributeError before reaching the ledger and both maintenance_work_mem tests failed for a reason unrelated to what they assert. The production code is right — run() takes a psycopg.Connection, which always has add_notice_handler. The double was incomplete, so it grows the method rather than the runner growing a getattr guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
53ca811 to
ef710d5CompareDarkest-Teddy
commented
Aug 11, 2026
Rebased onto main, migrations renamed — and a verification gap to know about before mergingHeads up: this branch was force-pushed ( What changedRebased all 50 commits onto
The four migration files cross-reference each other by bare number in ~23 places, including operator-facing Verification
The gapThe Chapter 1 Playwright lane and the oracles are unverified. Not because they failed on this branch's merits, but because the local E2E lane doesn't run to completion on a Windows dev box — filed as #532 with seven distinct blockers. The decisive one is Playwright workers dying with Concretely: in the one run that got far enough, Recommendation: before merging, have someone run the lane on Linux (or wait for CI's |
Darkest-Teddy
commented
Aug 12, 2026
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/screens/Achievements.tsx (1)
159-166: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep showcase state consistent with the persisted order.
persistFeaturedreports a failed request but leaves the optimistic local order in place. The screen then shows a showcase that the profile does not persist.
frontend/src/components/screens/Achievements.tsx#L159-L166: propagate request failure to the caller, or return a success result.frontend/src/components/screens/Achievements.tsx#L168-L179: restore the previous featured IDs when the toggle request fails.frontend/src/components/screens/Achievements.tsx#L181-L192: restore the previous featured IDs when the reorder request fails.🤖 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 `@frontend/src/components/screens/Achievements.tsx` around lines 159 - 166, Update persistFeatured to propagate setFeaturedAchievements failures (or return an explicit success result) after showing the error. In the toggle handler at frontend/src/components/screens/Achievements.tsx lines 168-179 and reorder handler at lines 181-192, retain each operation’s previous featured IDs and restore them when persistence fails.backend/routes/learn.py (1)
980-1001: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
end_sessiondoes not verify that the session belongs to the caller.The pending-session branch on Line 968 compares
pending["user_id"]withbody.user_idand raises403. The materialized path does not make the equivalent comparison. It loads the row bysession_idalone, writesended_at, and now awardssession_completedXP tobody.user_id.
require_selfonly proves thatbody.user_idis the caller. It does not prove that the caller ownssession_id. A caller who knows or guesses another user's session id can end that session and collect the XP, and can repeat it with a different session id for a fresh idempotency key each time. The award makes the existing gap exploitable for gain.Compare the loaded row's
user_idbefore the update.🔒 Proposed fix
if not session_rows: raise HTTPException(status_code=404, detail="Session not found") session = session_rows[0] + if body.user_id and session.get("user_id") != body.user_id:+ raise HTTPException(status_code=403, detail="Session user mismatch") table("sessions").update(🤖 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 `@backend/routes/learn.py` around lines 980 - 1001, Update the materialized-session path in end_session to compare the loaded session row’s user_id with body.user_id before updating or awarding XP. Raise the same 403 ownership error used by the pending-session branch when they differ, and preserve the existing 404 handling for missing sessions.
🟡 Minor comments (12)
scripts/lib/local-common.sh-140-144 (1)
140-144: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail when PostgREST does not become ready.
After all 30 attempts fail, this function continues to seed data.
scripts/local-up.shandscripts/local-db-reset.shthen can report success although the REST endpoint is unavailable. Exit after the retry loop when$codeis not200.Proposed fix
for _ in $(seq 1 30); do code="$(curl -s -o /dev/null -w '%{http_code}' \ "http://127.0.0.1:${SUPABASE_API_PORT:-54321}/rest/v1/terms?select=id&limit=1" \ -H "apikey: $KEY" -H "Authorization: Bearer $KEY")" [ "$code" = "200" ] && { echo " ready"; break; } sleep 1 done + if [ "$code" != "200" ]; then+ echo "✗ PostgREST did not become ready (last HTTP $code)"+ exit 1+ fi🤖 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 `@scripts/lib/local-common.sh` around lines 140 - 144, Update the PostgREST readiness retry loop to track whether any attempt received HTTP 200 and, after all 30 attempts, exit with failure when the final status is not 200. Preserve the existing ready message, break behavior, and successful continuation into data seeding.frontend/src/components/screens/admin/AchievementWiki.tsx-554-554 (1)
554-554: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the
×buttons for assistive technology.The trigger delete button at line 554 and the cosmetic unlink button at line 591 expose only the character
×. A screen reader announces "times", which does not identify the target or the action. Add anaria-labeland atitle.🛠️ Proposed fix
- <button className="btn btn--sm btn--ghost" onClick={() => deleteTriggerInline(t.id)}>×</button>+ <button+ className="btn btn--sm btn--ghost"+ aria-label={`Delete trigger ${t.trigger_type}`}+ title="Delete trigger"+ onClick={() => deleteTriggerInline(t.id)}+ >×</button><button className="btn btn--sm btn--ghost" + aria-label={`Unlink cosmetic ${c ? c.name : cid}`}+ title="Unlink cosmetic" onClick={() => unlinkCosmetic(cid)} disabled={cosmeticBusyId === cid} >Also applies to: 589-596
🤖 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 `@frontend/src/components/screens/admin/AchievementWiki.tsx` at line 554, Add aria-label and title attributes to the trigger delete button invoking deleteTriggerInline and the cosmetic unlink button in the corresponding trigger-list markup, using clear labels that identify the affected item and action instead of exposing only “×”.frontend/src/components/screens/Social.tsx-326-328 (1)
326-328: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the loading state when
userIdis absent.
refreshreturns at line 327 before thetry/finally, sosetLoading(false)never runs whileuserIdis null. The panel then renders "Loading…" permanently for a viewer whose id never resolves.Socialitself gates its own load onuserReady, butFriendsPanelreadsuserIdonly.🛠️ Proposed fix
const refresh = React.useCallback(async () => { - if (!userId) return;+ if (!userId) { setLoading(false); return; } try {Also applies to: 377-384
🤖 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 `@frontend/src/components/screens/Social.tsx` around lines 326 - 328, Update the refresh callback in Social to clear the loading state before returning when userId is absent, ensuring FriendsPanel does not remain stuck on “Loading…”. Apply the same handling to the additional userId-dependent path around the second referenced section, while preserving the existing try/finally behavior for valid user IDs.frontend/src/components/screens/admin/AchievementWiki.tsx-722-728 (1)
722-728: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn empty amount field commits
0.
Number("")returns0, and0passesNumber.isFinite. If an admin clears the amount box and blurs it,commitAmountsendsPATCH { amount: 0 }and disables the XP payout for that rule without any warning. Treat a blank draft as "no change".🛠️ Proposed fix
const commitAmount = async (key: string) => { const raw = drafts[key]; if (raw === undefined) return; const amount = Number(raw); const rule = rules.find(r => r.key === key); setDrafts(prev => { const next = { ...prev }; delete next[key]; return next; }); - if (!rule || !Number.isFinite(amount) || amount === rule.amount) return;+ if (raw.trim() === "") return;+ if (!rule || !Number.isFinite(amount) || amount === rule.amount) return;🤖 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 `@frontend/src/components/screens/admin/AchievementWiki.tsx` around lines 722 - 728, Update commitAmount to treat a blank or whitespace-only drafts[key] value as no change before converting it with Number. Preserve the existing draft cleanup behavior, but return without issuing the PATCH when the amount field is empty.frontend/src/components/ProfileView.tsx-34-60 (1)
34-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
checkStatusagainst a stale response.
checkStatuscommitssetStatuswithout a cancellation check. IfprofileUserIdchanges while the two fetches are in flight, the older response resolves last in some orderings and sets a status that belongs to the previous profile. The viewer then sees "Friends" or "Add friend" for the wrong person until the next remount.The sibling components in this PR already handle this.
frontend/src/components/screens/achievements/ActivityTab.tsxuses acancelledflag, andfrontend/src/components/screens/achievements/LeaderboardTab.tsxuses a request-id ref. Apply the same pattern here.🛠️ Proposed fix using a request-id ref
export function AddFriendAction({ profileUserId }: { profileUserId: string }) { const { userId: viewerId } = useUser(); const toast = useToast(); const [status, setStatus] = React.useState<FriendStatus>("loading"); + const requestRef = React.useRef(0); const checkStatus = React.useCallback(async () => { if (!viewerId || viewerId === profileUserId) return; + const requestId = ++requestRef.current;+ const commit = (next: FriendStatus) => {+ if (requestRef.current === requestId) setStatus(next);+ }; try { const [friendsRes, requestsRes] = await Promise.all([ fetchFriends(viewerId), fetchFriendRequests(viewerId), ]); if (friendsRes.friends.some((f) => f.user_id === profileUserId)) { - setStatus("friends");+ commit("friends"); return; } if (requestsRes.outgoing.some((r) => r.to_user_id === profileUserId)) { - setStatus("pending");+ commit("pending"); return; } - setStatus("eligible");+ commit("eligible"); } catch { // Couldn't confirm status. Default to eligible rather than hiding the // action entirely — a stale "Add friend" that 409s is recoverable via // the toast in send(), unlike a button that silently never shows up. - setStatus("eligible");+ commit("eligible"); } }, [viewerId, profileUserId]);Also reset the visible state when the profile changes, so the previous profile's status does not persist during the refetch:
React.useEffect(() => { + setStatus("loading"); checkStatus(); }, [checkStatus]);🤖 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 `@frontend/src/components/ProfileView.tsx` around lines 34 - 60, Update ProfileView’s checkStatus flow to ignore results from outdated requests by using a request-id ref or cancellation flag, checking it before every setStatus call in both success and error paths. Reset the visible status when profileUserId changes so the previous profile’s status is not shown while fetching the new one, while preserving the existing status precedence.frontend/src/components/screens/Achievements.tsx-279-279 (1)
279-279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender uploaded artwork in showcase cards.
Use
BadgeArtwithiconUrl={ua.achievement.icon_url}here. The grid and modal use that field, but the showcase only rendersachievement.icon. Uploaded achievement icons therefore never appear in featured cards.🤖 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 `@frontend/src/components/screens/Achievements.tsx` at line 279, Update the showcase card artwork rendering in the achievements component to use BadgeArt with iconUrl={ua.achievement.icon_url} instead of rendering ua.achievement.icon or the fallback star, matching the grid and modal behavior.docs/superpowers/specs/2026-07-31-gamification-xp-achievements-design.md-74-83 (1)
74-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language label to both fenced code blocks.
Markdownlint reports MD040 because the fences at Line 74 and Line 196 have no language. Use
textif the blocks are illustrative schemas and signatures.Also applies to: 196-206
🤖 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 `@docs/superpowers/specs/2026-07-31-gamification-xp-achievements-design.md` around lines 74 - 83, Add the text language label to both unlabeled fenced code blocks in the schema/signature sections, including the blocks near the visible table definition and the corresponding block around line 196, while leaving their contents unchanged.Source: Linters/SAST tools
frontend/src/app/globals.css-297-300 (1)
297-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve each leaf rotation during
sap-burst.The keyframes animate
transform, so they override each span's inline rotation while the animation runs. The burst leaves will share the same translation direction instead of retaining their assigned angles.Compose the rotation into the animated transform through a CSS custom property, or animate a nested child element.
🤖 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 `@frontend/src/app/globals.css` around lines 297 - 300, Update the sap-burst animation and its leaf elements so each span’s assigned rotation is preserved while scale and vertical translation animate. Use a CSS custom property in the animated transform or animate a nested child, ensuring the existing per-leaf angle remains effective throughout the keyframes.backend/routes/admin.py-241-252 (1)
241-252: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTwo admin endpoints report success for a target row that does not exist. Both handlers act first and update by filter afterwards. A PostgREST
updatethat matches zero rows is not an error, so a wrong id or key returns a200success body and writes a misleading audit entry.grant_achievementin the same file now resolves its target first and raises404; apply the same pattern.
backend/routes/admin.py#L241-L252: select the achievement byachievement_idand raise404before decoding and uploading the icon, so no orphaned object is written to storage.backend/routes/admin.py#L435-L449: select thexp_rulesrow bykeyand raise404before buildingupdates, so an unknown key does not return{"updated": True}.🤖 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 `@backend/routes/admin.py` around lines 241 - 252, The admin handlers must verify target rows before performing side effects. In backend/routes/admin.py lines 241-252, update upload_icon to select the achievement by achievement_id and raise 404 before decoding or calling upload_achievement_icon; in backend/routes/admin.py lines 435-449, update the xp_rules handler to select the row by key and raise 404 before constructing updates. Ensure both existing success paths remain unchanged for valid targets.backend/tests/test_admin_routes.py-138-138 (1)
138-138: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the multi-line
withformatting on these two lines.Lines 138 and 167 place four context managers on one physical line, separated by runs of about 14 spaces. The syntax is valid, but every other
within this file uses backslash continuation (lines 110-112, 186, 200, 220, 247-249, 620-623). The long space runs look like line continuations that were lost during an automated edit, and the lines are long enough to trip a line-length rule.♻️ Proposed fix for line 138
- with _mock_admin(), patch("routes.admin.table", side_effect=by_name), patch("routes.admin.check_achievements", return_value=[]), patch("routes.admin.award_xp_safe") as award:+ with _mock_admin(), \+ patch("routes.admin.table", side_effect=by_name), \+ patch("routes.admin.check_achievements", return_value=[]), \+ patch("routes.admin.award_xp_safe") as award:Apply the same change at line 167.
Also applies to: 167-167
🤖 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 `@backend/tests/test_admin_routes.py` at line 138, Restore the multi-line backslash continuation formatting for the four context managers in the with statements near _mock_admin(), patch("routes.admin.table"), patch("routes.admin.check_achievements"), and patch("routes.admin.award_xp_safe"). Apply the same formatting to both affected with statements, matching the surrounding conventions and avoiding overlong lines.backend/tests/test_achievement_dispatch.py-151-182 (1)
151-182: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
graph_nodesstub configuresinsert, butapply_graph_updatecallsupsert.
backend/services/graph_service.pyline 683 writes new nodes withtable("graph_nodes").upsert(...), notinsert. Settinghandles["graph_nodes"].insert.return_valueat lines 155-158 and 173-175 has no effect on the code under test.upsertreturns a bareMagicMock, soisinstance(returned, list)isFalseandcanonical_idsilently falls back to the generated UUID.Both tests still pass, because they assert only on dispatched event types. The stub is misleading and does not exercise the canonical-id read-back.
♻️ Proposed fix
handles["graph_nodes"].select.return_value = [] - handles["graph_nodes"].insert.return_value = [+ handles["graph_nodes"].upsert.return_value = [ {"id": "n1", "concept_name": "Gradient Descent", "mastery_score": 0.1, "course_id": "c1"} ]Apply the same change at lines 173-175.
🤖 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 `@backend/tests/test_achievement_dispatch.py` around lines 151 - 182, Update both test methods, test_fires_the_graph_trigger_types and test_a_broken_dispatch_does_not_fail_the_graph_write, to configure handles["graph_nodes"].upsert.return_value instead of insert.return_value. Preserve the existing returned node payloads so apply_graph_update exercises canonical-id read-back through its upsert path.backend/tests/test_xp_wiring.py-308-330 (1)
308-330: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the
PENDING_SESSIONSentry after the test.Line 317 writes into
routes.learn.PENDING_SESSIONS, a module-global dict, and the test never removes the key. If the early-return branch does not pop the entry,"sess-xp-pending"stays in the dict for the rest of the pytest session. Any later test that posts to/api/learn/end-sessionwith that id, or that asserts on the size or contents ofPENDING_SESSIONS, then depends on test execution order.Discard the key in a
finallyblock so the test cleans up whether or not the route pops it.🛡️ Proposed fix
import routes.learn as learn learn.PENDING_SESSIONS["sess-xp-pending"] = { "user_id": "user-xp-pending", "mode": "socratic", "topic": "T", "offering_id": None, "assistant_reply": "hi", "graph_update": {}, } - with patch("routes.learn.award_xp_safe") as award_mock:- r = client.post("/api/learn/end-session", json={- "session_id": "sess-xp-pending", "user_id": "user-xp-pending",- })+ try:+ with patch("routes.learn.award_xp_safe") as award_mock:+ r = client.post("/api/learn/end-session", json={+ "session_id": "sess-xp-pending", "user_id": "user-xp-pending",+ })+ finally:+ learn.PENDING_SESSIONS.pop("sess-xp-pending", None) assert r.status_code == 200 award_mock.assert_not_called()🤖 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 `@backend/tests/test_xp_wiring.py` around lines 308 - 330, Update test_pending_session_early_return_does_not_award to remove the "sess-xp-pending" entry from routes.learn.PENDING_SESSIONS in a finally block surrounding the request and assertions, ensuring cleanup occurs whether or not the route removes it.
🧹 Nitpick comments (19)
frontend/src/components/screens/admin/AchievementWiki.test.tsx (2)
176-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore globals and spies in
afterEach, not at the end of the test body.
vi.unstubAllGlobals()at lines 200 and 229 andclickSpy.mockRestore()at line 228 run only when the test reaches the end. If an assertion fails earlier, thecreateImageBitmapstub and the patchedHTMLInputElement.prototype.clickleak into the following tests in this file, which turns one failure into several. ThereadIcondescribe at lines 253-259 already uses thebeforeEach/afterEachform. Apply the same form here.♻️ Proposed cleanup
describe("AchievementWiki — icon upload gating (Finding 3)", () => { + afterEach(() => {+ vi.unstubAllGlobals();+ vi.restoreAllMocks();+ });+ function makeFile(name = "icon.png", type = "image/png", bytes = 10) {Then remove the trailing
vi.unstubAllGlobals()andclickSpy.mockRestore()calls from the three test bodies.🤖 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 `@frontend/src/components/screens/admin/AchievementWiki.test.tsx` around lines 176 - 230, Add shared beforeEach/afterEach cleanup for the upload tests, restoring stubbed globals with vi.unstubAllGlobals() and resetting the HTMLInputElement.prototype.click spy after every test. Apply the established cleanup pattern used by the readIcon describe block, then remove the trailing vi.unstubAllGlobals() and clickSpy.mockRestore() calls from the affected test bodies.
94-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the trigger editor and the XP-rule amount field.
This suite covers the save closure, cosmetic linking, and icon upload. Two edited surfaces in
AchievementWiki.tsxhave no test, and both carry defects raised in this review:
- The trigger inputs at lines 541-556 write on every keystroke.
commitAmountat lines 722-728 treats an empty field as0.Add a test that types into a trigger field and asserts the number of
adminUpdateTriggercalls. Add a test that clears an XP-rule amount, blurs, and asserts thatadminUpdateXpRuleis not called.Also applies to: 171-251
🤖 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 `@frontend/src/components/screens/admin/AchievementWiki.test.tsx` around lines 94 - 133, Add coverage in the AchievementWiki test suite for both edited surfaces: type into a trigger input and assert the expected adminUpdateTrigger call count, then clear an XP-rule amount, blur the field, and assert adminUpdateXpRule is not called. Reuse the existing setup and expansion helpers, and target the trigger editor and XP-rule amount field specifically.frontend/src/lib/api.ts (1)
1698-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the inline response shapes into
types.ts.
fetchFriendRequestsdeclares the incoming/outgoing request objects inline.frontend/src/components/screens/Social.tsxlines 41-42 redeclare the same two shapes asIncomingFriendRequestandOutgoingFriendRequest.adminListXpRulesdeclares the XP-rule shape inline, andfrontend/src/components/screens/admin/AchievementWiki.tsxline 700 redeclares it asinterface XpRule. Every other gamification contract in this PR (Friend,LeaderboardRow,GamificationMe,ActivityData) lives infrontend/src/lib/types.ts. ExportFriendRequestIncoming,FriendRequestOutgoing, andXpRulefromtypes.tsand reference them in both places, so a backend field change fails in one location instead of drifting silently.Also applies to: 1726-1728
🤖 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 `@frontend/src/lib/api.ts` around lines 1698 - 1702, Move the inline incoming and outgoing friend-request response shapes from fetchFriendRequests into exported FriendRequestIncoming and FriendRequestOutgoing types in types.ts, then reuse those types in fetchFriendRequests and Social.tsx. Likewise extract the inline adminListXpRules shape into exported XpRule in types.ts and replace the duplicate AchievementWiki.tsx interface and API response annotation with it.backend/routes/notes.py (1)
200-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the idempotency claim in the comment.
The comment states that keying on the new note id makes a client retry "an idempotent no-op". That is not what happens. A retried
POSTcallscreate_noteagain, produces a differentnote["id"], and therefore a different idempotency key, so the second request pays XP again. The key only deduplicates a repeated award for the same note id, and this route awards once per created note.The award placement is correct. Only the stated reason is wrong. Describe the key as protection against a double award for the same note.
🤖 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 `@backend/routes/notes.py` around lines 200 - 207, Correct the comment above award_xp_safe to remove the claim that a client retry is an idempotent no-op. State that source_id=note["id"] prevents duplicate awards for the same newly-created note, while each retried POST creates a separate note and is awarded independently; leave the award placement and implementation unchanged.backend/routes/gradebook.py (1)
417-417: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDispatch only when the write can change the computed grade.
_check_grade_achievementsruns_course_grade_a_count, which walks every enrollment and issues three queries per enrollment, then decrypts every assignment. Until Top Marks is earned, that cost is paid synchronously on every assignment create and every assignment patch.A patch that carries only
title,due_date, orassignment_typecannot move the percent, so the recompute is pure waste on that path. Gate the update-path call on the fields that feed the grade math.♻️ Proposed refactor for the update path
table("assignments").update( patch_data, filters={"id": f"eq.{assignment_id}"}, ) - _check_grade_achievements(body.user_id)+ # Only a points change can move the computed percent.+ if {"points_possible", "points_earned", "category_id"} & set(patch_data):+ _check_grade_achievements(body.user_id) return {"updated": True}Also applies to: 446-446
🤖 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 `@backend/routes/gradebook.py` at line 417, Gate the `_check_grade_achievements` calls in the assignment update paths (including the call near line 446) so they run only when the request changes fields used by grade calculation. Skip recomputation for patches containing only `title`, `due_date`, or `assignment_type`, while preserving the existing behavior for grade-affecting fields.backend/services/growth.py (1)
38-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the derived bands.
_bands()rebuilds the full band list on every call._band_for_levelcalls it once per lookup, andlevel_for_xpcallsxp_for_levelonce per level, so a singlexp_into_level(total_xp)call rebuilds the bands up to ~2×max_leveltimes.get_leaderboardinbackend/routes/gamification.pyalso callsstage_for_levelper ranked row.Add a second
lru_cachelayer thatclear_growth_cachealso clears. This keeps the guideline requirement that every mutator clears the cache through a single hook.As per coding guidelines: "Use `functools.lru_cache` only for deterministic per-process reads. Cached mutable values must be deep-copied, arguments must be hashable, and mutable data must have a `clear_*_cache()` hook called by every mutator."♻️ Proposed refactor
+@lru_cache(maxsize=1)+def _bands_cached() -> tuple:+ return tuple(_build_bands())++ def clear_growth_cache() -> None: """`#98`: every growth_stages mutator must call this.""" _stages_cached.cache_clear() + _bands_cached.cache_clear()-def _bands() -> list[dict]:+def _bands() -> list[dict]:+ return [dict(b) for b in _bands_cached()]+++def _build_bands() -> list[dict]:🤖 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 `@backend/services/growth.py` around lines 38 - 59, Cache the derived result of _bands() with a second functools.lru_cache layer, preserving its deterministic no-argument lookup. Update clear_growth_cache to clear this bands cache alongside the existing caches, and ensure every growth-data mutator continues to invoke that single cache-clearing hook; do not expose cached mutable values without the required defensive copying.Source: Coding guidelines
backend/routes/learn.py (1)
1060-1079: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPost-commit achievement dispatch discards every failure without a signal. Each site correctly refuses to fail the write that earned the achievement, but each one also drops the exception with
except Exception: pass.services/xp_service.py::_dispatch_xp_achievementshandles the identical boundary withlogger.exception. A broken dispatch therefore makes a badge permanently unearnable with no log line, which is the exact class of defect the_count_rowsdocstring records.
backend/routes/learn.py#L1060-L1079: replacepasswithlogger.exception, and wrap each of the sixcheck_achievementscalls separately so one failure does not skip the remaining dispatches.backend/routes/flashcards.py#L356-L365: replacepasswithlogger.exceptionfor theflashcards_revieweddispatch.backend/routes/gradebook.py#L363-L379: replacepasswithlogger.exceptionin_check_grade_achievements.🤖 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 `@backend/routes/learn.py` around lines 1060 - 1079, Preserve successful writes while making achievement-dispatch failures observable. In backend/routes/learn.py lines 1060-1079, update the post-session achievement dispatch around check_achievements to use logger.exception and isolate each of the six calls so one failure does not prevent later dispatches; in backend/routes/flashcards.py lines 356-365, replace the swallowed exception for the flashcards_reviewed dispatch with logger.exception; and in backend/routes/gradebook.py lines 363-379, replace the pass in _check_grade_achievements with logger.exception.backend/routes/social.py (2)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the local re-imports of
check_achievements.Line 16 already imports
check_achievementsat module scope. The four localfrom services.achievement_service import check_achievementsstatements inside thetryblocks shadow it with the same object. Remove them and call the module-level symbol, as the new friends code at line 760 already does.Also applies to: 136-145, 175-186, 513-519
🤖 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 `@backend/routes/social.py` around lines 61 - 65, Remove the redundant local from-services.achievement_service imports inside the four try blocks around the achievement checks, including the blocks near the owned-room-members, related achievement, and line-513 flows. Reuse the module-level check_achievements import already defined at module scope, preserving the existing calls and exception handling.
671-702: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTwo concurrent identical friend requests can hit the unique constraint.
The
existingread and the insert are separate statements. A double-click sends two requests; both read no row and both insert.UNIQUE(from_user_id, to_user_id)then fails the loser with a 500. This is the same check-then-act shape the join paths already fixed withupsert. Use the same pattern here.♻️ Proposed change
- result = table("friend_requests").insert({- "from_user_id": body.from_user_id,- "to_user_id": body.to_user_id,- "status": "pending",- })+ result = table("friend_requests").upsert(+ {+ "from_user_id": body.from_user_id,+ "to_user_id": body.to_user_id,+ "status": "pending",+ },+ on_conflict="from_user_id,to_user_id",+ )🤖 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 `@backend/routes/social.py` around lines 671 - 702, Replace the separate existing-row check and conditional insert/update in the friend-request handler with the established upsert pattern, using the same conflict key for from_user_id and to_user_id. Preserve the pending-request 409 behavior and ensure retries reactivate existing non-pending rows without exposing a unique-constraint failure.backend/tests/test_streak_service.py (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the missing-user branch and for
touch_streak_safe.Two branches of
services/streak_service.pyhave no test:
touch_streakreturns0and writes nothing when theusersselect returns no rows. A request path reaches this after an account is deleted.touch_streak_safeswallows the exception and returnsNone.graph_service.apply_graph_updateandroutes/learn.py::end_sessionboth depend on that contract, so a regression there would turn a logged warning into a failed study session.🧪 Proposed additional tests
def test_first_ever_activity_starts_at_one(self): t = _user(None, 0) with patch("services.streak_service.table", return_value=t): from services.streak_service import touch_streak assert touch_streak("u1") == 1 ++ def test_missing_user_returns_zero_and_writes_nothing(self):+ t = MagicMock()+ t.select.return_value = []+ with patch("services.streak_service.table", return_value=t):+ from services.streak_service import touch_streak+ assert touch_streak("u1") == 0+ t.update.assert_not_called()+++class TestTouchStreakSafe:+ def test_a_db_failure_is_swallowed(self):+ """Callers on request paths rely on this never raising."""+ with patch("services.streak_service.touch_streak",+ side_effect=RuntimeError("db down")):+ from services.streak_service import touch_streak_safe+ assert touch_streak_safe("u1") is None🤖 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 `@backend/tests/test_streak_service.py` around lines 28 - 32, Add tests in the streak service test module covering the missing-user branch of touch_streak: mock the users lookup to return no rows, assert it returns 0, and verify no write occurs. Also test touch_streak_safe by forcing touch_streak to raise, then assert the exception is swallowed and the result is None.backend/tests/test_achievement_icon_upload.py (1)
102-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the degenerate-viewBox cases.
_svg_is_squarehas two uncovered fail-closed branches: a zero or negative width (viewBox="0 0 0 0", guarded bywidth > 0) and a non-numeric viewBox (viewBox="a b c d", guarded by theValueErrorhandler). Both currently reject, so this is coverage rather than a defect.💚 Suggested additional cases
def test_rejects_an_svg_with_no_viewbox(self): from services.storage_service import validate_icon with pytest.raises(HTTPException): validate_icon(b"<svg></svg>", "image/svg+xml") ++ `@pytest.mark.parametrize`("view_box", [b"0 0 0 0", b"a b c d", b"0 0 64"])+ def test_rejects_a_degenerate_viewbox(self, view_box):+ from services.storage_service import validate_icon+ with pytest.raises(HTTPException):+ validate_icon(b'<svg viewBox="' + view_box + b'"></svg>',+ "image/svg+xml")🤖 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 `@backend/tests/test_achievement_icon_upload.py` around lines 102 - 114, Add tests alongside test_rejects_an_svg_with_no_viewbox covering SVGs with a zero or negative viewBox width and a non-numeric viewBox, and assert each raises HTTPException through validate_icon with the SVG content type.backend/tests/test_achievement_dispatch.py (2)
378-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment describes two exclusions, but the set holds one.
The comment names
account_age_daysandmanual_admin_grant._NOT_EVENT_DISPATCHEDcontains onlymanual_admin_grant. Eitheraccount_age_daysis genuinely absent from the migration (in which case the sentence belongs elsewhere, since the set never needs to exclude it) or it belongs in the set.Align the comment with the set so a future reader does not add
account_age_daysback on the assumption it was dropped by mistake.🤖 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 `@backend/tests/test_achievement_dispatch.py` around lines 378 - 381, Align the comment above _NOT_EVENT_DISPATCHED with the set’s sole entry, manual_admin_grant, and remove the account_age_days exclusion rationale from that comment so future readers do not infer it belongs in the set.
373-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the backstop resilient to a migration rename and to nested call sites.
Two robustness points:
_MIGRATIONhardcodes20260731194102_achievement_catalog.sql. The PR already renamed these four migrations once, from numeric prefixes to timestamp prefixes. After another rename,read_textraisesFileNotFoundErrorand the wholeTestEveryTriggerTypeIsDispatchedclass errors with an opaque message rather than a clear "catalog migration not found".
glob("*.py")does not descend into subdirectories. If acheck_achievementscall site moves toroutes/<subdir>/, the backstop reports it as missing. That fails in the safe direction, but the failure message would point at the wrong cause.♻️ Proposed fix
-_MIGRATION = (- Path(__file__).resolve().parent.parent- / "db" / "migrations" / "20260731194102_achievement_catalog.sql"-)+_MIGRATIONS_DIR = Path(__file__).resolve().parent.parent / "db" / "migrations"+_CATALOG = sorted(_MIGRATIONS_DIR.glob("*_achievement_catalog.sql"))+assert _CATALOG, (+ f"no *_achievement_catalog.sql under {_MIGRATIONS_DIR} — the backstop "+ "cannot read the live trigger types"+)+_MIGRATION = _CATALOG[-1]for sub in ("routes", "services"): - for path in (root / sub).glob("*.py"):+ for path in (root / sub).rglob("*.py"):🤖 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 `@backend/tests/test_achievement_dispatch.py` around lines 373 - 399, Make the dispatch backstop resilient by updating _trigger_types_in_migration to locate the achievement catalog migration without relying on one hardcoded filename, and emit a clear “catalog migration not found” failure when no matching migration exists. Update _dispatched_event_types to recursively scan Python files under routes and services (including nested subdirectories), while preserving the existing check_achievements literal extraction.backend/services/graph_service.py (1)
806-822: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIsolate each dispatch and skip the dispatch when the update changed nothing.
Two concerns in this block:
The three
check_achievementscalls share onetry. Ifgraph_nodes_countraises,concepts_masteredandcourses_with_masterynever run for that update. Per-call isolation keeps one failing stat from suppressing the other two.The block runs on every
apply_graph_updatecall, including calls that create no node and change no mastery.apply_graph_updateis on the chat request path and is called on every turn, so an emptygraph_updatestill pays threecheck_achievementsround trips (each does aachievement_triggersselect plus auser_achievementsselect before it can short-circuit). None of these three stats can change when no node was created and no mastery changed.♻️ Proposed fix: guard on actual change and isolate per call
- try:- from services.achievement_service import check_achievements- check_achievements(user_id, "graph_nodes_count", {})- check_achievements(user_id, "concepts_mastered", {})- check_achievements(user_id, "courses_with_mastery", {})- except Exception:- logger.exception("achievement dispatch failed after graph update user=%s", user_id)+ if inserted_in_batch or mastery_changes:+ from services.achievement_service import check_achievements+ for event_type in ("graph_nodes_count", "concepts_mastered",+ "courses_with_mastery"):+ try:+ check_achievements(user_id, event_type, {})+ except Exception:+ logger.exception(+ "achievement dispatch failed after graph update "+ "user=%s event=%s", user_id, event_type,+ )Note:
backend/tests/test_achievement_dispatch.py::test_fires_the_graph_trigger_typescreates a node, so the guard keeps that test green.🤖 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 `@backend/services/graph_service.py` around lines 806 - 822, Update the post-commit achievement dispatch in apply_graph_update to run only when the graph update created a node or changed mastery. Isolate each check_achievements call in its own exception handler so a failure for graph_nodes_count does not prevent concepts_mastered or courses_with_mastery from running, while preserving per-call error logging and non-propagation.backend/tests/test_xp_wiring.py (1)
48-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the table stub fail readably for an unexpected table name.
Line 76 returns
lambda name: handles[name].award_xptouches onlyxp_rules,xp_events, anduserstoday. If a future change adds a fourth table read throughservices.xp_service.table, every test in this file fails with a bareKeyErrorthat names the table but explains nothing about why the stub is incomplete.♻️ Proposed fix
handles["users"].select.return_value = [{"total_xp": 0, "level": 1}] handles["users"].update.return_value = [] - return (lambda name: handles[name]), handles++ def _table(name):+ if name not in handles:+ raise AssertionError(+ f"award_xp read an unstubbed table {name!r}; add it to "+ "_xp_tables so these route tests keep asserting the payload"+ )+ return handles[name]++ return _table, handles🤖 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 `@backend/tests/test_xp_wiring.py` around lines 48 - 76, Update the table accessor returned by _xp_tables so an unknown table name raises a clear, purpose-specific error identifying the unsupported name and indicating that the test stub must be updated, while preserving normal lookups for xp_rules, xp_events, and users.backend/tests/test_xp_rules_routes.py (2)
11-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo new admin gamification endpoint has negative-path authorization coverage. Every test in both new files patches
routes.admin.require_adminaway, so the guard on these endpoints is asserted nowhere. The PR objectives list "authentication guards for gamification and friends endpoints" as a review finding that was fixed; without a rejection test, a later refactor can drop the dependency and every test stays green.
backend/tests/test_xp_rules_routes.py#L11-L19: add a test thatGET /api/admin/xp-rulesandPATCH /api/admin/xp-rules/{key}reject a non-admin caller. The PATCH endpoint sets the payout amount for every future XP award, so an unguarded route lets any caller inflate XP product-wide.backend/tests/test_achievement_icon_upload.py#L149-L168: add a test thatPOST /api/admin/achievements/{id}/iconrejects a non-admin caller. The endpoint writes to shared public storage and mutatesachievements.icon_url.Use the failure shape
require_adminraises in the existing_mock_adminhelper inbackend/tests/test_admin_routes.py, and drive the endpoint without patchingrequire_admin.🤖 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 `@backend/tests/test_xp_rules_routes.py` around lines 11 - 19, The admin gamification tests lack negative authorization coverage. In backend/tests/test_xp_rules_routes.py:11-19, add tests for GET /api/admin/xp-rules and PATCH /api/admin/xp-rules/{key} that invoke the routes without patching routes.admin.require_admin and assert the rejection shape used by _mock_admin in backend/tests/test_admin_routes.py; in backend/tests/test_achievement_icon_upload.py:149-168, add equivalent coverage for POST /api/admin/achievements/{id}/icon. Keep existing authorized tests unchanged.
34-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that a rejected update writes nothing.
Both rejection tests check only the status code. A route that writes the row and then validates would still return 400 and still pass.
backend/tests/test_admin_routes.pylines 230-233 pins exactly this property for the draft-achievement gate; apply the same standard here.💚 Suggested change
def test_rejects_a_negative_amount(self): - with patch("routes.admin.require_admin"), patch("routes.admin.table"):+ with patch("routes.admin.require_admin"), \+ patch("routes.admin.log_admin_action") as audit, \+ patch("routes.admin.table") as t: r = client.patch("/api/admin/xp-rules/quiz_completed", json={"amount": -5}) assert r.status_code == 400 + # The 400 must come from a gate, not from a write that was rolled back.+ t.return_value.update.assert_not_called()+ audit.assert_not_called()Apply the same assertions to
test_rejects_an_empty_body.🤖 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 `@backend/tests/test_xp_rules_routes.py` around lines 34 - 42, Update test_rejects_a_negative_amount and test_rejects_an_empty_body to assert the mocked routes.admin.table is not called after each rejected PATCH request, matching the draft-achievement gate test’s write-prevention assertion while preserving the existing 400 status checks.backend/tests/test_admin_routes.py (2)
99-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
by_nametable factory.The same
by_nameclosure appears six times in this file: lines 99-108, 126-136, 156-165, 176-184, 236-245, and 609-618. Only theachievementsrow and theuser_achievementsselect result vary.♻️ Proposed shared helper
def_grant_tables(achievement: dict|None, *, already_earned: bool=False): """`routes.admin.table` stub for the grant endpoint."""defby_name(name): m=MagicMock() ifname=="achievements": m.select.return_value= [achievement] ifachievementelse [] elifname=="user_achievements": m.select.return_value= [{"achievement_id": "a1"}] ifalready_earnedelse [] m.insert.return_value= [{}] returnmreturnby_nameEach test then becomes:
- def by_name(name):- m = MagicMock()- if name == "achievements":- m.select.return_value = [- {"id": "a1", "slug": "live_badge", "status": "live"}- ]- elif name == "user_achievements":- m.select.return_value = []- m.insert.return_value = [{}]- return m-+ by_name = _grant_tables({"id": "a1", "slug": "live_badge", "status": "live"})🤖 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 `@backend/tests/test_admin_routes.py` around lines 99 - 118, Extract the duplicated by_name table factory into a shared _grant_tables helper in the test module, parameterized by the optional achievement row and already-earned state. Update all six grant-related tests to patch routes.admin.table with this helper, preserving each test’s varying achievement and user_achievements select results and insert behavior.
235-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test_grants_when_achievement_is_liveduplicatestest_grants_achievement_to_user.Lines 235-255 and lines 98-118 build the same
by_namestub, apply the same three patches, post the same body, and assert the same status code andgrantedflag. The two tests carry identical signal.Either delete one, or differentiate this one so it earns its name — for example, assert that
check_achievementsis invoked for the granted user, or thatuser_achievements.insertreceived the expectedachievement_id.🤖 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 `@backend/tests/test_admin_routes.py` around lines 235 - 255, The test_grants_when_achievement_is_live duplicates test_grants_achievement_to_user and adds no distinct coverage. Remove the duplicate test, or differentiate it by asserting a live-achievement-specific interaction such as check_achievements being called for the granted user or user_achievements.insert receiving achievement_id, while preserving the existing grant response assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d944c11e-2b5e-4b12-bd4c-8f40a43c4652
⛔ Files ignored due to path filters (11)
frontend/public/growth/bare.svgis excluded by!**/*.svgfrontend/public/growth/bloom.svgis excluded by!**/*.svgfrontend/public/growth/branch.svgis excluded by!**/*.svgfrontend/public/growth/fruit.svgis excluded by!**/*.svgfrontend/public/growth/old.svgis excluded by!**/*.svgfrontend/public/growth/sapling.svgis excluded by!**/*.svgfrontend/public/growth/seed.svgis excluded by!**/*.svgfrontend/public/growth/seedling.svgis excluded by!**/*.svgfrontend/public/growth/soil.svgis excluded by!**/*.svgfrontend/public/growth/sprout.svgis excluded by!**/*.svgfrontend/public/growth/young.svgis excluded by!**/*.svg
📒 Files selected for processing (78)
backend/db/migrate.pybackend/db/migrations/20260731193214_gamification.sqlbackend/db/migrations/20260731194102_achievement_catalog.sqlbackend/db/migrations/20260801022421_restore_admin_created_triggers.sqlbackend/db/migrations/20260801070026_recover_admin_triggers_from_audit_log.sqlbackend/main.pybackend/models/__init__.pybackend/routes/admin.pybackend/routes/documents.pybackend/routes/flashcards.pybackend/routes/gamification.pybackend/routes/gradebook.pybackend/routes/learn.pybackend/routes/notes.pybackend/routes/profile.pybackend/routes/quiz.pybackend/routes/social.pybackend/services/achievement_service.pybackend/services/graph_service.pybackend/services/growth.pybackend/services/http_cache.pybackend/services/storage_service.pybackend/services/streak_service.pybackend/services/xp_service.pybackend/tests/conftest.pybackend/tests/integration/conftest.pybackend/tests/test_achievement_dispatch.pybackend/tests/test_achievement_icon_upload.pybackend/tests/test_achievement_service.pybackend/tests/test_admin_routes.pybackend/tests/test_auth_first_login_achievement.pybackend/tests/test_friends_routes.pybackend/tests/test_gamification_routes.pybackend/tests/test_graph_service.pybackend/tests/test_growth.pybackend/tests/test_migrate.pybackend/tests/test_migrate_work_mem.pybackend/tests/test_profile_routes.pybackend/tests/test_storage_service.pybackend/tests/test_streak_service.pybackend/tests/test_xp_rules_routes.pybackend/tests/test_xp_service.pybackend/tests/test_xp_wiring.pydocs/frontend-testids.mddocs/superpowers/plans/2026-07-31-gamification-xp-achievements.mddocs/superpowers/specs/2026-07-31-gamification-xp-achievements-design.mdfrontend/e2e/gamification.spec.tsfrontend/e2e/support/db.tsfrontend/eslint-suppressions.jsonfrontend/eslint.config.mjsfrontend/package.jsonfrontend/src/app/globals.cssfrontend/src/components/ProfileView.friends.test.tsxfrontend/src/components/ProfileView.tsxfrontend/src/components/growth/BadgeArt.test.tsxfrontend/src/components/growth/BadgeArt.tsxfrontend/src/components/growth/levels.test.tsfrontend/src/components/growth/levels.tsfrontend/src/components/screens/Achievements.tsxfrontend/src/components/screens/Admin.tsxfrontend/src/components/screens/Social.friends.test.tsxfrontend/src/components/screens/Social.tsxfrontend/src/components/screens/achievements/ActivityTab.buckets.test.tsfrontend/src/components/screens/achievements/ActivityTab.tsxfrontend/src/components/screens/achievements/BadgeGrid.tsxfrontend/src/components/screens/achievements/BadgeModal.tsxfrontend/src/components/screens/achievements/HeroCard.tsxfrontend/src/components/screens/achievements/LeaderboardTab.podium.test.tsfrontend/src/components/screens/achievements/LeaderboardTab.tsxfrontend/src/components/screens/admin/AchievementWiki.test.tsxfrontend/src/components/screens/admin/AchievementWiki.tsxfrontend/src/lib/api.tsfrontend/src/lib/types.tsscripts/e2e-down.shscripts/e2e-up.shscripts/lib/local-common.shscripts/local-db-reset.shscripts/local-up.sh
Uh oh!
There was an error while loading. Please reload this page.
| now = datetime.now(timezone.utc) | ||
| start = _week_start(now) | ||
| events = _events_since(None, start) | ||
| weekly: dict[str, int] = {} | ||
| for e in events: | ||
| uid = e.get("user_id") | ||
| if uid: | ||
| weekly[uid] = weekly.get(uid, 0) + int(e.get("amount") or 0) | ||
| allowed = _scope_ids(user_id, scope) | ||
| if allowed is not None: | ||
| weekly = {k: v for k, v in weekly.items() if k in allowed} | ||
| # The viewer always sees their own row, even at zero and even when private. | ||
| weekly.setdefault(user_id, 0) | ||
| hidden = _private_ids() - {user_id} if scope in ("everyone", "school") else set() | ||
| ids = [k for k in weekly if k not in hidden] | ||
| etag = make_etag(user_id, scope, len(ids), sum(weekly.get(i, 0) for i in ids)) | ||
| not_mod = conditional(request, etag, REVALIDATE_CACHE_CONTROL) | ||
| if not_mod: | ||
| return not_mod | ||
| users = table("users").select( | ||
| "id,level,total_xp,streak_count", filters={"id": f"in.({','.join(ids)})"} | ||
| ) if ids else [] | ||
| names = get_display_names(ids) if ids else {} | ||
| by_id = {u["id"]: u for u in users or []} | ||
| ranked = sorted(ids, key=lambda i: (-weekly.get(i, 0), i)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Description: Inspect the connection layer's filter transport and look for existing in.() chunking helpers.set -euo pipefail
fd -t f 'connection.py' backend/db --exec cat -n {}
# Find other in.() filter builders to see whether chunking is an established pattern.
rg -n --type=py -C3 'in\.\(' backend | head -80Repository: SaplingLearn/Sapling
Length of output: 9436
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo'--- gamification structure ---'
ast-grep outline backend/routes/gamification.py
echo'--- relevant route and helpers ---'
cat -n backend/routes/gamification.py | sed -n '1,240p'echo'--- xp_events schema and access paths ---'
rg -n --type py --type sql -C3 'xp_events|_events_since|weekly|leaderboard|make_etag|REVALIDATE_CACHE_CONTROL' backend supabase migrations 2>/dev/null | head -240Repository: SaplingLearn/Sapling
Length of output: 30516
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo'--- leaderboard consumers and tests ---'
rg -n --type py --type ts --type tsx -C3 'leaderboard|scope=|week_xp|rows.*rank|\"you\"' backend frontend 2>/dev/null | head -240
echo'--- display-name query shape ---'
ast-grep outline backend/services/profiles.py
rg -n -C5 'def get_display_names|in\.\(' backend/services/profiles.py
echo'--- UUID filter transport and URL-size threshold ---'
python3 - <<'PY'from urllib.parse import urlencodetry: import httpxexcept ImportError: httpx = Nonefor n in (100, 150, 200, 250, 300): ids = [f"00000000-0000-0000-0000-{i:012d}" for i in range(n)] params = {"select": "id,level,total_xp,streak_count", "id": f"in.({','.join(ids)})"} if httpx: url = str(httpx.Request("GET", "https://example.test/rest/v1/users", params=params).url) else: url = "https://example.test/rest/v1/users?" + urlencode(params) print(n, len(url), url[-80:])PYRepository: SaplingLearn/Sapling
Length of output: 198
🏁 Script executed:
#!/bin/bashset -u
echo'--- repository paths relevant to leaderboard consumers ---'
fd -t f | rg '(^|/)(frontend|web|app|tests?)(/|$)|gamification|leaderboard'| head -200
echo'--- display-name query shape ---'if [ -f backend/services/profiles.py ];then
ast-grep outline backend/services/profiles.py
rg -n -C5 'def get_display_names|in\.\(' backend/services/profiles.py ||truefiecho'--- UUID filter transport and URL-size threshold ---'
python3 - <<'PY'from urllib.parse import urlencodetry: import httpxexcept ImportError: httpx = Nonefor n in (100, 150, 200, 250, 300): ids = [f"00000000-0000-0000-0000-{i:012d}" for i in range(n)] params = { "select": "id,level,total_xp,streak_count", "id": f"in.({','.join(ids)})", } if httpx: url = str(httpx.Request( "GET", "https://example.test/rest/v1/users", params=params ).url) else: url = "https://example.test/rest/v1/users?" + urlencode(params) print(f"{n} ids: {len(url)} bytes")PYRepository: SaplingLearn/Sapling
Length of output: 12982
🏁 Script executed:
#!/bin/bashset -u
echo'--- gamification tests ---'
cat -n backend/tests/test_gamification_routes.py | sed -n '1,320p'echo'--- profile resolver implementation ---'
cat -n backend/services/profiles.py | sed -n '1,70p'echo'--- user id definition and leaderboard-related schema ---'
rg -n --type sql -C4 'CREATE TABLE.*users|CREATE TABLE.*user_profiles|id[[:space:]]+UUID|id[[:space:]]+TEXT' backend/db/migrations | head -160
echo'--- encoded request sample ---'
python3 - <<'PY'import httpxids = [f"00000000-0000-0000-0000-{i:012d}" for i in range(3)]params = { "select": "id,level,total_xp,streak_count", "id": f"in.({','.join(ids)})",}request = httpx.Request("GET", "https://example.test/rest/v1/users", params=params)print(request.url)PYRepository: SaplingLearn/Sapling
Length of output: 35396
🏁 Script executed:
#!/bin/bashset -u
echo'--- ETag implementation and tests ---'
cat -n backend/services/http_cache.py | sed -n '45,75p'
rg -n -C4 'make_etag|If-None-Match|304' backend/tests/test_http_cache.py backend/tests/test_gamification_routes.py ||trueecho'--- deterministic ETag-input collision ---'
python3 - <<'PY'import hashlibdef etag(*parts): raw = "\x1f".join(str(p) for p in parts).encode() return '"' + hashlib.sha256(raw).hexdigest()[:24] + '"'# The route uses (viewer, scope, number of visible ids, sum of visible XP).before = etag("u1", "everyone", 2, 300)after = etag("u1", "everyone", 2, 300)print("etag_equal:", before == after)print("before_week_xp:", {"u1": 100, "u2": 200})print("after_week_xp:", {"u1": 150, "u2": 150})PYRepository: SaplingLearn/Sapling
Length of output: 12101
Aggregate weekly XP and bound the ranked-user lookups
_events_since(None, start) pages all weekly xp_events before applying friends or school filters, so every request and 304 scans platform-wide activity. The ETag also collides when the visible user count and XP sum remain unchanged while rankings change. Aggregate with the scope filter in the database and derive the ETag from a change key. Bound or paginate the result, and chunk the users and get_display_namesin.(...) lookups; 200 UUID-like IDs already produce an approximately 7.9 KB query URL.
🤖 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 `@backend/routes/gamification.py` around lines 167 - 198, Update the weekly
leaderboard flow around _events_since, make_etag, and the ranked user lookups to
aggregate XP in the database using the requested scope filter instead of
scanning all platform events, and derive the ETag from a reliable aggregation
change key. Bound or paginate ranked results, and chunk IDs for both the users
table query and get_display_names so large UUID lists do not create oversized
requests while preserving scope, privacy, and self-row behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Split this one — the ETag half is fixed in 8304bd6, the aggregation half is not.
Fixed: the ETag collision is real. (len(ids), sum(...)) is preserved by any reshuffle that keeps the same people and the same total, so first and second place could swap while every viewer kept getting a 304 for the stale order. It now keys on the ranked (id, xp) pairs, with a regression test that reshuffles 100/200 to 150/150 and asserts the ETag changes.
Not doing here: aggregating weekly XP in the database. PostgREST cannot GROUP BY without an RPC, and this repo requires all Supabase access to go through db/connection.py::table() (CLAUDE.md), so it needs a migration plus a new sanctioned access path. Same for bounding/paginating the board — the UI renders every row, so truncating to a top-N is a product decision, not a refactor. Both are real at scale and worth a follow-up issue; neither is a merge blocker for a pre-launch student app.
Note the scan is somewhat self-limiting today: these routes are private, no-cache, so the work happens per revalidation, but the ledger is only a week deep and paged.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo'--- leaderboard ETag and ranked rows ---'
rg -n -A45 -B20 'def get_leaderboard|make_etag\(' backend/routes/gamification.py
echo'--- ETag regression coverage ---'
rg -n -A16 -B10 'reshuff|ETag|etag|150|200|100' backend/tests/test_gamification_routes.py
echo'--- claimed commit summary, if available locally ---'
git show --no-patch --oneline 8304bd6 2>&1||trueLength of output: 26672
@Darkest-Teddy, verified the ETag fix.
get_leaderboard now derives its ETag from the ordered ranked (user_id, week_xp) pairs. The regression test covers the 100/200 to 150/150 reshuffle and confirms that the ETag changes.
The platform-wide weekly-event scan and unbounded UUID lookups remain valid scalability concerns. They are deferred from this PR. The current private, no-cache response policy requires revalidation, but it does not reduce the database work per request.
Would you like me to open a follow-up issue for database-side leaderboard aggregation and bounded or chunked ranked-user reads?
You are interacting with an AI system.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…view findings Review pass over #505, plus the CodeRabbit and static-analysis findings. Correctness - achievement_service: `session_before_hour` encoded "earlier is better" as `24 - ended.hour` behind a `hour < 12` guard, so against early-bird's threshold of 7 every session ending 00:00-11:59 UTC scored 13-24 and cleared it — "Finish a study session before 7am" was granted for finishing at 11am, and user_achievements is append-only. It is now an explicit LOWER_IS_BETTER stat reporting the earliest finish hour, so the stored threshold stays the literal hour in the badge text and keeps meaning that when an admin retunes it from the wiki. profile._progress_for skips these: a countdown to a wall-clock hour has no progress bar, and the `min(stat, target)` clamp rendered the unearned badge as 100% complete. - social.accept_friend_request: `status != "pending" or _are_friends(...)` lumped `declined` in with `accepted`, so accepting a declined request skipped the friendships upsert, still stamped the row `accepted` and returned success — a request recorded as accepted with no friendship behind it. Only an existing friendship is idempotent now; anything else already resolved is a 409. - admin.grant_achievement: a granted badge's linked cosmetics never unlocked. check_achievements("manual_admin_grant") cannot do it — the stat is a hard-coded 0 and every manual_admin_grant trigger's threshold is 1, so the skip fires on all of them — which left mentor, comeback, secret and methuselah, all manual-grant-only, unable to unlock theirs at all. Extracted achievement_service.grant_linked_cosmetics and shared it with the earned path. - gamification.leaderboard: the ETag keyed on (row count, total XP), which any reshuffle preserves (u1:100/u2:200 -> u1:150/u2:150), so first and second place could swap while every viewer kept getting a 304 for the stale order. Keyed on the ranked (id, xp) pairs instead. Observability - The five `except Exception: pass` around achievement dispatch now log via logger.exception, matching xp_service/graph_service. They must not fail the action that earned them, but swallowing silently is exactly how the `friendships.id` 400 this PR describes stayed invisible. Tests - test_storage_service: both assertions in TestBucketBootstrapCoversIcons were tautologies (`ICON <= (ALLOWED | ICON)`; `ICON - (ALLOWED | ICON)`), true no matter what main.py passes, so they would still pass if the bootstrap dropped every icon type. Replaced with one test asserting against the list the lifespan actually hands ensure_bucket_exists. Frontend - AchievementWiki: the trigger editor PATCHed on every keystroke and each reload replaced the input's value, so an out-of-order response rewrote the field mid-typing. Moved to the draft/onBlur pattern XpRulesPanel already uses, plus an in-flight guard so a double-click on Add can't create two triggers. - BadgeModal: role="dialog", aria-modal, focus moved into the panel, Tab trapped inside it, focus restored on close. - Achievements: the showcase could only be reordered by pointer drag; added move-earlier/move-later buttons. Not addressed, deliberately: CodeRabbit also asked the leaderboard to aggregate weekly XP in the database. PostgREST cannot GROUP BY without an RPC, and CLAUDE.md requires all Supabase access to go through db/connection.py::table(), so that needs a migration and a new access path — a scale follow-up rather than a merge blocker. Its one Critical finding (a "duplicate TABS declaration" in Achievements.tsx) was a false positive: TABS is declared once and tsc is clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Darkest-Teddy
commented
Aug 12, 2026
Review pass: 4 correctness bugs fixed, CodeRabbit's 9 findings triagedReviewed the branch and landed The code is unusually well documented — most of what reads as suspicious turns out to be deliberate and explained in a comment. Four things were genuinely wrong. CorrectnessEarly Bird was granted for lunchtime sessions. Accepting a declined friend request faked success. Admin-granted badges never unlocked their cosmetics. Leaderboard ETag collided on reshuffles. It keyed on ObservabilityThe five Tests
Frontend
CodeRabbit triage7 of 9 implemented, replied inline on each thread. Two exceptions:
Follow-ups worth an issue (not fixed here)
|
Uh oh!
There was an error while loading. Please reload this page.
Builds out the growth system from the
Achievements.dc.htmlClaude Design project: an XP ledger, levels mapped to the eleven Sapling growth stages, three leaderboards, an activity dashboard, and an editable achievement wiki in the existing admin console tab.Design spec:
docs/superpowers/specs/2026-07-31-gamification-xp-achievements-design.mdStatus
Draft — spec landed, implementation in progress. Commits will land incrementally.
Scope
xp_eventsappend-only ledger with idempotency keys, plus admin-editablexp_rulesgrowth_stagesas the single source of truth for level maths (29,800 XP to L50)profile_visibilityiconcolumnfriendships+friend_requests) — Sapling had none, and the design's friends scope needs oneAdmin.tsx's existingachievementstab: inline edit of description/icon/rarity/XP, plus the XP-rules panelNotes
The source design had three internal contradictions; the spec records how each was resolved (stage thresholds, the XP curve, and streak freezes — the last cut from v1 as it had no mechanic behind it anywhere).
🤖 Generated with Claude Code
Summary by CodeRabbit