feat(career): curated genre drills — per-instrument, achievably cleared - #943
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesCareer passports now load curated Virtuoso drills from configuration, evaluate requirements per instrument, preserve drill progress through gained-only merges, expose drill labels, and render labeled drill requirements in the client UI. Tests cover song-only completion, drill-gated completion, and stale progress snapshots. Career passport drill flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerClient
participant CareerPassportsAPI
participant DrillProgressState
PlayerClient->>CareerPassportsAPI: Relay local byNode drill snapshot
CareerPassportsAPI->>DrillProgressState: Merge gained drill progress
DrillProgressState-->>CareerPassportsAPI: Persisted snapshot
CareerPassportsAPI-->>PlayerClient: Passport requirements and drill labels
PlayerClient->>PlayerClient: Render cleared and pending drills
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/career/routes.py (1)
303-346: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnfiltered, never-shrinking byNode merge can eventually brick future drill-state updates.
_merge_drill_nodespersists every node id relayed inbyNode, not just the handful actually referenced by curatedvirtuoso_nodesinpassports.json, and the merge never shrinks (gained-only by design). If the relayedvirtuoso.progress.byNodesnapshot reflects virtuoso's whole node catalog (likely much larger than the 5 curated drills here) rather than just career-relevant nodes, the persisted snapshot grows monotonically over the life of an install. Once it nearsDRILL_SNAPSHOT_MAX_BYTES, every subsequent/drill-statePOST — including ones carrying real progress on a still-required drill — gets rejected with 413 forever, since there's no pruning path.Consider filtering both the incoming and persisted
byNodedown to the node ids actually referenced by any genre'svirtuoso_nodesbefore merging/persisting, so storage stays bounded by curated content rather than virtuoso's full catalog.♻️ Proposed fix to bound persisted byNode to curated node ids
+def _known_drill_nodes(): + cfg = _state["passports_content"] + known = set() + for genre_cfg in (cfg.get("genres") or {}).values(): + nodes = (genre_cfg or {}).get("virtuoso_nodes") or [] + if isinstance(nodes, dict): + for lst in nodes.values(): + known.update(n for n in (lst or []) if isinstance(n, str)) + else: + known.update(n for n in nodes if isinstance(n, str)) + return known + def _merge_drill_nodes(old, new): out = dict(old) - for node_id, incoming in new.items(): + known = _known_drill_nodes() + for node_id, incoming in new.items(): + if node_id not in known: + continue if not isinstance(incoming, dict): continueTo confirm the risk profile, it would help to know the typical size of virtuoso's full
byNodecatalog relative toDRILL_SNAPSHOT_MAX_BYTES.Also applies to: 576-585
🤖 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 `@plugins/career/routes.py` around lines 303 - 346, Update the drill-state persistence flow around _merge_drill_nodes to restrict both the incoming and existing byNode maps to node IDs referenced by any genre’s virtuoso_nodes configuration. Merge and persist only this curated subset, while preserving the gained-only behavior for retained nodes so real progress on required drills continues to be accepted and storage remains bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/career/routes.py`:
- Around line 303-346: Update the drill-state persistence flow around
_merge_drill_nodes to restrict both the incoming and existing byNode maps to
node IDs referenced by any genre’s virtuoso_nodes configuration. Merge and
persist only this curated subset, while preserving the gained-only behavior for
retained nodes so real progress on required drills continues to be accepted and
storage remains bounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a289c328-2f8c-4ee1-8bc3-be105f84ce53
📒 Files selected for processing (5)
CHANGELOG.mdplugins/career/passports.jsonplugins/career/routes.pyplugins/career/screen.jstests/plugins/career/test_passports.py
Career v2, WS3. Bronze in blues/rock/metal/funk/jazz now also asks for
the genre's signature Virtuoso drill, data-driven in passports.json:
blues_shuffle, rock_power_backbeat, melodic_metal_gallop,
sixteenth_pocket, vl_shells — with career-side display labels the
passport page renders instead of raw node ids.
- virtuoso_nodes becomes {instrument: [node_ids]} so a keys passport
never demands a guitar drill; a flat list keeps meaning guitar
(virtuoso's content is guitar-first).
- _node_cleared also accepts keysCleared (a top-tier clean pass in one
key — virtuoso's FIRST gained-only artifact). The depth rungs
additionally require a maxed speed tier, too high a bar for Bronze.
- Genres without a curated entry stay songs-only.
Note: pre-release behavior change — v1 passports aren't in any shipped
build, so no earned badge can demote in the wild.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a5cfe77 to
1d014a5
Compare
Career v2, workstream 3. Bronze in blues/rock/metal/funk/jazz now also asks for the genre's signature Virtuoso drill — data-driven in
passports.json(blues_shuffle, rock_power_backbeat, melodic_metal_gallop, sixteenth_pocket, vl_shells) with career-side display labels.virtuoso_nodes: {instrument: [ids]}— a keys passport never demands a guitar drill; a flat list keeps meaning guitar (back-compat).keysCleared(a top-tier clean pass in one key — virtuoso's first gained-only artifact) now counts, alongside depth flips and mastery. The depth rungs additionally require a maxed speed tier — too high a bar for Bronze.Testing
Shipped-blues gating + keysCleared clearing, per-instrument resolution (keys earns songs-only), gained-only merge vs stale snapshots, flat-list back-compat, 413 guard preserved. 26 career tests green. Codex preflight: clean (round 5 — rounds 1–4 caught the localStorage bootstrap gap, the stale-relay clobber, the size-guard bypass, and the wrong invite copy).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes