feat(career): career plugin — stars, venue tiers, pack downloads (career mode 2/3)#907
Conversation
…k downloads (career mode PR2) Bundled plugin: per-song stars from best_accuracy (60/75/85% → 1/2/3★), cumulative stars unlock bar → club → arena (data-driven venues.json). Venue packs (UE-rendered crowd loops) download on demand to CONFIG_DIR/plugin_uploads/career/ on a background thread with sha256 + zip-slip validation, served via FileResponse. Career screen (promoted sidebar entry) shows progress and pushes the active venue's manifest into the crowd video layer (v3VenueCrowd, PR1) — degrades cleanly when either side is absent. Pack URLs land in venues.json in PR3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…or installed venues Codex preflight: nulling _appliedManifestVenue on delete skipped pushCrowdManifest's setManifest(null) cleanup, leaving the crowd layer on a deleted pack; and the 'playing here' badge showed for an override venue whose pack was removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex preflight: a manifest fetch resolving after a newer refresh (pack deleted, venue switched) could re-apply a stale pack over the user's newer selection — fetches now carry a generation token and bail when superseded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex preflight: scans hide rather than delete stats of removed songs, so stars now apply the same existing-song filter other stats surfaces use (filename IN (SELECT filename FROM songs)). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR introduces a bundled Career plugin with configurable venue tiers, star-based progression, downloadable venue packs, a browser UI, crowd manifest integration, sidebar navigation, and backend and frontend test coverage. ChangesCareer plugin
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant CareerScreen
participant CareerAPI
participant VenuePackStorage
participant VenueCrowd
Player->>CareerScreen: Open Career screen
CareerScreen->>CareerAPI: Fetch career state
CareerAPI-->>CareerScreen: Return stars and venue states
CareerScreen->>CareerAPI: Request unlocked venue pack
CareerAPI->>VenuePackStorage: Download and install pack
VenuePackStorage-->>CareerAPI: Report installation status
CareerAPI-->>CareerScreen: Return updated state
CareerScreen->>VenueCrowd: Set selected venue manifest
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
plugins/career/routes.py (3)
114-115: 🔒 Security & Privacy | 🔵 TrivialVerify the configured pack URL trust boundary.
pack["url"]comes from bundledvenues.json, not directly fromvenue_id, so the current endpoint is not itself request-controlled SSRF. Before release assets populate these fields, confirm the metadata is immutable/trusted and enforce HTTPS or an allowed-host policy before callingurlopen.🤖 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 114 - 115, Validate the configured URL in the download flow before constructing the request or calling urlopen: ensure pack["url"] is sourced from immutable trusted metadata and enforce HTTPS or the project’s allowed-host policy. Update the logic around the visible urllib.request.Request/urlopen calls while preserving the existing download behavior for approved URLs.Source: Linters/SAST tools
163-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the supplied plugin logger unconditionally.
context.get("log") or _state["log"]falls back to a module logger for this non-CLI plugin, bypassing centralized host logging when the context logger is absent or falsey. Usecontext["log"]according to the setup contract.As per path instructions, “Use
context["log"]for all backend plugin output and never useprint(); CLI entry points may configure a standard-library logging fallback.”🤖 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` at line 163, Update the logger assignment in the plugin setup flow to use context["log"] unconditionally instead of context.get("log") or _state["log"]. Preserve the setup contract that backend plugin output uses the supplied context logger and does not fall back to the module logger.Source: Path instructions
42-49: 🩺 Stability & Availability | 🔵 TrivialVerify download coordination across server workers.
_state,_lock, and the daemon threads are process-local. With multiple FastAPI/Uvicorn workers, two requests can both seeidleand download or remove the same venue concurrently, bypassing the409guard. Confirm single-process deployment or move coordination to a shared lock/state store.Also applies to: 200-208
🤖 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 42 - 49, Update the career download coordination around the process-local _state, _lock, and daemon-thread workflow to use coordination shared across FastAPI/Uvicorn workers, including shared status and locking for download and removal operations so concurrent requests cannot bypass the 409 guard. If the implementation intentionally requires single-process deployment instead, enforce and document that deployment constraint rather than relying on process-local state.tests/plugins/career/test_routes.py (1)
127-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that a checksum failure preserves the installed pack.
The comment promises “nothing installed over the good pack,” but the test only checks the error status. A regression that deletes or replaces the existing installation could still pass. Snapshot the known-good manifest (or at least assert the pack remains installed) after the corrupt download.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugins/career/test_routes.py` around lines 127 - 131, Extend the checksum-failure test around career_routes._download_pack to capture the existing good pack state before the corrupt download, then assert that the installed pack and its manifest remain unchanged afterward. Keep the existing error-status and sha256-message assertions, and compare against the pre-download snapshot rather than only checking installation presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/career/routes.py`:
- Around line 109-112: Move staging initialization in the worker flow around
mkdtemp into the existing try block, starting with staging = None before it.
Ensure allocation failures are caught by the worker’s exception handling and
update the venue’s progress state from running to a failed/completed state so
polling and deletion do not remain blocked.
- Around line 94-102: Update the validation around REQUIRED_LOOPS and stingers
to use a media-only filename rule that rejects .json and accepts only supported
video files; continue checking that each referenced file exists in pack_dir.
Keep PACK_FILENAME_RE available for manifest.json validation, but do not use it
for loop or stinger entries.
- Around line 117-124: Protect all progress mutations in the download worker
with the existing _lock, including bytes_total, bytes_done, status, and error
updates. Add or reuse a lock-protected helper for updating progress fields, then
use it in the loop around resp.read, digest/write progress updates, and the
status/error paths referenced near lines 146-150 so get_state() never observes a
partial update.
- Around line 115-124: The download loop around urllib.request.urlopen must
enforce a maximum archive size while reading, rather than only recording
Content-Length or pack["bytes"] in progress. Add extraction quotas in the ZIP
handling flow as well, covering compressed size, total uncompressed size, file
count, and each file’s uncompressed size; reject the asset before writing beyond
any limit or filling CONFIG_DIR, while preserving existing digest, progress, and
verification behavior.
- Around line 143-145: Update the venue install and DELETE flows around
final_dir and extract_dir to use a per-venue removing state, preventing
concurrent installs while removal is in progress. Install into a new versioned
directory and atomically swap it into place only after extraction completes;
remove the old version afterward, preserving the previous pack if the swap
fails. Ensure DELETE removes the files before clearing the download state so a
concurrent POST cannot install content that the deletion then removes.
In `@plugins/career/screen.html`:
- Line 6: Update the instructional paragraph near the career screen markup to
derive all star accuracy percentages from state.star_accuracy_thresholds instead
of hard-coding 60%, 75%, and 85%. Preserve the existing explanatory text while
interpolating the configured thresholds in the correct star-order.
In `@plugins/career/screen.js`:
- Around line 96-100: Update venueCardHTML() to avoid calling
localStorage.getItem() directly when calculating isActive; reuse the existing
safe storage accessor used by the other storage paths, while preserving the
current override-key and venue-id comparison.
- Around line 133-137: Update the refresh error-handling path near schedulePoll
so transient fetch failures still schedule another poll when the previous state
contains a venue with download status "running". Reuse schedulePoll with the
last known state from the catch path, ensuring the existing timer behavior and
normal successful-refresh flow remain unchanged.
- Around line 180-188: Update the download and delete fetch chains in the career
screen action handler to validate response.ok before calling refresh, surface
non-2xx responses, and catch network failures with the existing visible error
mechanism. Keep refresh limited to successful actions and apply the same
handling to both dlBtn and delBtn branches.
- Around line 162-173: Update refresh() to use a state-request generation
counter like _manifestReqGen: increment it for each invocation, capture the
current generation before await fetchState(), and discard the response if a
newer refresh has started. Only apply _state, announceUnlocks, render,
schedulePoll, and pushCrowdManifest for the latest generation, while preserving
the existing retry behavior for fetch failures.
In `@tests/plugins/career/conftest.py`:
- Around line 5-13: Update the Career route loading in the test configuration to
avoid sys.path mutation, sys.modules.pop, and bare import routes; load the
Career routes module under a unique, plugin-specific module name while
preserving the career_routes reference used by the tests.
---
Nitpick comments:
In `@plugins/career/routes.py`:
- Around line 114-115: Validate the configured URL in the download flow before
constructing the request or calling urlopen: ensure pack["url"] is sourced from
immutable trusted metadata and enforce HTTPS or the project’s allowed-host
policy. Update the logic around the visible urllib.request.Request/urlopen calls
while preserving the existing download behavior for approved URLs.
- Line 163: Update the logger assignment in the plugin setup flow to use
context["log"] unconditionally instead of context.get("log") or _state["log"].
Preserve the setup contract that backend plugin output uses the supplied context
logger and does not fall back to the module logger.
- Around line 42-49: Update the career download coordination around the
process-local _state, _lock, and daemon-thread workflow to use coordination
shared across FastAPI/Uvicorn workers, including shared status and locking for
download and removal operations so concurrent requests cannot bypass the 409
guard. If the implementation intentionally requires single-process deployment
instead, enforce and document that deployment constraint rather than relying on
process-local state.
In `@tests/plugins/career/test_routes.py`:
- Around line 127-131: Extend the checksum-failure test around
career_routes._download_pack to capture the existing good pack state before the
corrupt download, then assert that the installed pack and its manifest remain
unchanged afterward. Keep the existing error-status and sha256-message
assertions, and compare against the pre-download snapshot rather than only
checking installation presence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8aacb978-aad2-4c6f-afcc-ad0e23b883b0
📒 Files selected for processing (12)
.gitignoreplugins/career/assets/career.cssplugins/career/plugin.jsonplugins/career/routes.pyplugins/career/screen.htmlplugins/career/screen.jsplugins/career/venues.jsonstatic/v3/shell.jstests/js/career_plugin.test.jstests/plugins/career/__init__.pytests/plugins/career/conftest.pytests/plugins/career/test_routes.py
| for state in REQUIRED_LOOPS: | ||
| name = loops.get(state) | ||
| if not name or not PACK_FILENAME_RE.match(name): | ||
| raise ValueError(f"manifest is missing the '{state}' loop") | ||
| if not (pack_dir / name).is_file(): | ||
| raise ValueError(f"loop file '{name}' missing from pack") | ||
| for name in (manifest.get("stingers") or {}).values(): | ||
| if name and (not PACK_FILENAME_RE.match(name) or not (pack_dir / name).is_file()): | ||
| raise ValueError(f"stinger file '{name}' invalid or missing") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject non-video files for loop and stinger entries.
PACK_FILENAME_RE allows .json for every manifest value, so a malformed pack can point loops.bored at manifest.json and still install. The frontend then treats JSON as video. Use a media-only filename rule for loop and stinger values, reserving .json for manifest.json.
🤖 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 94 - 102, Update the validation around
REQUIRED_LOOPS and stingers to use a media-only filename rule that rejects .json
and accepts only supported video files; continue checking that each referenced
file exists in pack_dir. Keep PACK_FILENAME_RE available for manifest.json
validation, but do not use it for loop or stinger entries.
| staging = Path(tempfile.mkdtemp(prefix=f"career-{venue_id}-", | ||
| dir=str(_state["venues_dir"]))) | ||
| zip_path = staging / "pack.zip" | ||
| try: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep staging allocation inside the guarded worker path.
mkdtemp() runs before try; if the config directory is unavailable or disk-full, the thread exits with the download permanently marked running. The UI will poll forever and DELETE will remain 409. Initialize staging = None, create it inside try, and ensure failures update the progress state.
🤖 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 109 - 112, Move staging initialization
in the worker flow around mkdtemp into the existing try block, starting with
staging = None before it. Ensure allocation failures are caught by the worker’s
exception handling and update the venue’s progress state from running to a
failed/completed state so polling and deletion do not remain blocked.
| with urllib.request.urlopen(req, timeout=60) as resp, open(zip_path, "wb") as out: | ||
| total = int(resp.headers.get("Content-Length") or pack.get("bytes") or 0) | ||
| progress["bytes_total"] = total | ||
| while True: | ||
| chunk = resp.read(DOWNLOAD_CHUNK) | ||
| if not chunk: | ||
| break | ||
| digest.update(chunk) | ||
| out.write(chunk) | ||
| progress["bytes_done"] += len(chunk) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound both download and extracted-pack sizes.
Content-Length and pack["bytes"] only update progress; the stream accepts unlimited bytes, and ZIP extraction has no compressed, uncompressed, file-count, or per-file limits. A bad or compromised release asset can fill CONFIG_DIR before verification completes. Enforce maximum download and extraction quotas.
Also applies to: 130-139
🧰 Tools
🪛 Ruff (0.15.20)
[error] 115-115: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 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 115 - 124, The download loop around
urllib.request.urlopen must enforce a maximum archive size while reading, rather
than only recording Content-Length or pack["bytes"] in progress. Add extraction
quotas in the ZIP handling flow as well, covering compressed size, total
uncompressed size, file count, and each file’s uncompressed size; reject the
asset before writing beyond any limit or filling CONFIG_DIR, while preserving
existing digest, progress, and verification behavior.
| progress["bytes_total"] = total | ||
| while True: | ||
| chunk = resp.read(DOWNLOAD_CHUNK) | ||
| if not chunk: | ||
| break | ||
| digest.update(chunk) | ||
| out.write(chunk) | ||
| progress["bytes_done"] += len(chunk) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Lock progress mutations as well as reads.
get_state() snapshots progress under _lock, but the worker mutates bytes_total, bytes_done, status, and error without it. Readers can observe inconsistent progress or an error status before its message is written. Update these fields through a lock-protected helper.
Also applies to: 146-150
🤖 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 117 - 124, Protect all progress
mutations in the download worker with the existing _lock, including bytes_total,
bytes_done, status, and error updates. Add or reuse a lock-protected helper for
updating progress fields, then use it in the loop around resp.read, digest/write
progress updates, and the status/error paths referenced near lines 146-150 so
get_state() never observes a partial update.
| if final_dir.exists(): | ||
| shutil.rmtree(final_dir) | ||
| extract_dir.rename(final_dir) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make install and removal atomic per venue.
rmtree(final_dir) followed by rename exposes a missing/partial pack and can lose the old pack if the rename fails. Separately, DELETE removes the download state before deleting files, allowing a concurrent POST to install a new pack that the old DELETE then removes. Use a per-venue removing state and an atomic versioned-directory swap.
Also applies to: 215-220
🤖 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 143 - 145, Update the venue install
and DELETE flows around final_dir and extract_dir to use a per-venue removing
state, preventing concurrent installs while removal is in progress. Install into
a new versioned directory and atomically swap it into place only after
extraction completes; remove the old version afterward, preserving the previous
pack if the swap fails. Ensure DELETE removes the files before clearing the
download state so a concurrent POST cannot install content that the deletion
then removes.
| function schedulePoll(state) { | ||
| clearTimeout(_pollTimer); | ||
| if (state.venues.some((v) => (v.download || {}).status === 'running')) { | ||
| _pollTimer = setTimeout(refresh, POLL_MS); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep polling alive after transient fetch failures.
When a scheduled refresh fails, the catch returns before schedulePoll(state), so no new timer is installed even though the backend download may still be running. The UI then remains stale until another event. Schedule a retry while the previous state indicates an active download.
Also applies to: 162-168
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 135-135: Avoid using the initial state variable in setState
Context: setTimeout(refresh, POLL_MS)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[error] 135-135: React's useState should not be directly called
Context: setTimeout(refresh, POLL_MS)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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/screen.js` around lines 133 - 137, Update the refresh
error-handling path near schedulePoll so transient fetch failures still schedule
another poll when the previous state contains a venue with download status
"running". Reuse schedulePoll with the last known state from the catch path,
ensuring the existing timer behavior and normal successful-refresh flow remain
unchanged.
| async function refresh() { | ||
| let state; | ||
| try { | ||
| state = await fetchState(); | ||
| } catch (_) { | ||
| return; // server restarting; next trigger retries | ||
| } | ||
| _state = state; | ||
| announceUnlocks(state); | ||
| render(state); | ||
| schedulePoll(state); | ||
| pushCrowdManifest(state); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Discard stale state responses.
refresh() can run concurrently from polling, stats:recorded, and click handlers. Without a request generation or abort check, an older response can overwrite newer install/download/unlock state and schedule polling from stale data. Add a state-request generation similar to _manifestReqGen.
🤖 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/screen.js` around lines 162 - 173, Update refresh() to use a
state-request generation counter like _manifestReqGen: increment it for each
invocation, capture the current generation before await fetchState(), and
discard the response if a newer refresh has started. Only apply _state,
announceUnlocks, render, schedulePoll, and pushCrowdManifest for the latest
generation, while preserving the existing retry behavior for fetch failures.
| if (dlBtn) { | ||
| fetch(`${API}/packs/${dlBtn.dataset.careerDownload}/download`, { method: 'POST' }) | ||
| .then(refresh); | ||
| } else if (delBtn) { | ||
| // Do NOT null _appliedManifestVenue here: pushCrowdManifest() | ||
| // clears/replaces the crowd manifest precisely by seeing that the | ||
| // applied venue is no longer among the installed ones. | ||
| fetch(`${API}/packs/${delBtn.dataset.careerDelete}`, { method: 'DELETE' }) | ||
| .then(refresh); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle non-2xx action responses visibly.
Download and delete call .then(refresh) regardless of res.ok, and network failures are unhandled. A 403, 404, or 409 therefore appears as a silent no-op. Check the response status, catch failures, and surface the error before refreshing.
🤖 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/screen.js` around lines 180 - 188, Update the download and
delete fetch chains in the career screen action handler to validate response.ok
before calling refresh, surface non-2xx responses, and catch network failures
with the existing visible error mechanism. Keep refresh limited to successful
actions and apply the same handling to both dlBtn and delBtn branches.
| sys.path.insert(0, str(Path(__file__).parent.parent.parent.parent / 'plugins' / 'career')) | ||
|
|
||
| import pytest | ||
| from fastapi import FastAPI | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| # Drop a sibling 'routes' cached by another plugin's tests (bare-name collision). | ||
| sys.modules.pop('routes', None) | ||
| import routes as career_routes |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*(from|import)\s+routes\b' testsRepository: got-feedBack/feedBack
Length of output: 689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant test fixture files first
ast-grep outline tests/plugins/career/conftest.py --view expanded || true
ast-grep outline tests/plugins/tuner/conftest.py --view expanded || true
ast-grep outline tests/plugins/achievements/conftest.py --view expanded || true
# Show the relevant line ranges with line numbers
for f in tests/plugins/career/conftest.py tests/plugins/tuner/conftest.py tests/plugins/achievements/conftest.py tests/plugins/career/test_routes.py tests/plugins/achievements/test_routes.py tests/plugins/achievements/test_datamin.py tests/plugins/tuner/test_config.py; do
echo "===== $f ====="
wc -l "$f"
cat -n "$f" | sed -n '1,120p'
doneRepository: got-feedBack/feedBack
Length of output: 28775
Avoid bare routes imports here
sys.path.insert(0, ...) plus import routes makes the module name process-global, so other test modules that do import routes can resolve to the wrong plugin during collection. Load Career under a unique module name instead of mutating sys.path/sys.modules this way.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/plugins/career/conftest.py` around lines 5 - 13, Update the Career
route loading in the test configuration to avoid sys.path mutation,
sys.modules.pop, and bare import routes; load the Career routes module under a
unique, plugin-specific module name while preserving the career_routes reference
used by the tests.
Byron's progression tuning: club at 50★, arena at 150★. /state now returns star_detail rows (title/artist joined from the library, stars, best accuracy, next-star threshold) sorted closest-to-next-star first, and the career screen renders a collection panel: tier summary plus a per-song list with a 'N% to next star' practice hint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atch guards - 'Play here' now also defaults the visualization to Venue (remembering the prior viz); active venues show 'Leave venue' which restores it and sets the '__none__' override so no installed venue silently reapplies. - Pack manifests may ship an intro block (flyover video + ambience mp3); files validate like loops/stingers, .mp3 added to the serving whitelist. - Codex preflight: whitelist regexes use fullmatch (trailing-newline names could validate but 500 on serving). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex preflight: nulling _appliedManifestVenue before refresh skipped the setManifest(null) cleanup branch, leaving the crowd playing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/career/screen.js`:
- Around line 88-95: Update the venue rendering logic around the active
predicate and the existing isActive check to reuse a single guarded localStorage
lookup, following the protection already used in pushCrowdManifest. Ensure
storage exceptions fall back safely without aborting render(), and remove the
duplicated unguarded VENUE_OVERRIDE_KEY access.
- Around line 246-257: Update the venue-unselect branch handling the
PREV_VIZ_KEY so that when no previous visualization is stored, it explicitly
restores the default non-venue visualization instead of leaving the current
'venue' selection active. Continue restoring the stored value through
localStorage and window.setViz when prev exists, then call refresh() as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d97beb2-b979-439b-9707-1ae9bce1ed90
📒 Files selected for processing (8)
plugins/career/assets/career.cssplugins/career/routes.pyplugins/career/screen.htmlplugins/career/screen.jsplugins/career/venues.jsonstatic/tailwind.min.csstests/plugins/career/conftest.pytests/plugins/career/test_routes.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/plugins/career/conftest.py
- plugins/career/routes.py
- tests/plugins/career/test_routes.py
| const active = localStorage.getItem(VENUE_OVERRIDE_KEY) === v.id; | ||
| const main = active | ||
| ? `<button data-career-unselect="1" class="career-btn career-btn-ghost">Leave venue</button>` | ||
| : `<button data-career-play="${esc(v.id)}" class="career-btn career-btn-primary">Play here</button>`; | ||
| action = `<div class="flex items-center gap-2"> | ||
| ${main} | ||
| <button data-career-delete="${esc(v.id)}" class="career-btn career-btn-ghost">Remove pack</button> | ||
| </div>`; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded localStorage.getItem reintroduced (and duplicated).
Line 88 calls localStorage.getItem(VENUE_OVERRIDE_KEY) directly, unlike the guarded lookup added in pushCrowdManifest (lines 51-56). This is the same failure mode previously flagged for this file: in blocked/private/sandboxed storage contexts this throws and render() fails entirely. The existing isActive check at line 106-107 has the same unguarded call and computes the identical predicate — worth consolidating into a single guarded lookup.
🩹 Proposed fix
function venueCardHTML(v, state) {
const locked = !v.unlocked;
const dl = v.download || { status: 'idle' };
const pct = dl.bytes_total > 0
? Math.round((dl.bytes_done / dl.bytes_total) * 100) : 0;
+ let override = null;
+ try { override = localStorage.getItem(VENUE_OVERRIDE_KEY); } catch (_) { /* ok */ }
let action = '';
if (locked) {
...
} else if (v.installed) {
- const active = localStorage.getItem(VENUE_OVERRIDE_KEY) === v.id;
+ const active = override === v.id;
...
- const isActive = !locked && v.installed &&
- localStorage.getItem(VENUE_OVERRIDE_KEY) === v.id;
+ const isActive = !locked && v.installed && override === v.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 `@plugins/career/screen.js` around lines 88 - 95, Update the venue rendering
logic around the active predicate and the existing isActive check to reuse a
single guarded localStorage lookup, following the protection already used in
pushCrowdManifest. Ensure storage exceptions fall back safely without aborting
render(), and remove the duplicated unguarded VENUE_OVERRIDE_KEY access.
| } else if (e.target.closest('[data-career-unselect]')) { | ||
| try { | ||
| localStorage.setItem(VENUE_OVERRIDE_KEY, NO_VENUE); | ||
| const prev = localStorage.getItem(PREV_VIZ_KEY); | ||
| if (prev) { | ||
| localStorage.setItem('vizSelection', prev); | ||
| if (typeof window.setViz === 'function') window.setViz(prev); | ||
| } | ||
| } catch (_) { /* ok */ } | ||
| // keep _appliedManifestVenue: pushCrowdManifest clears the crowd | ||
| // manifest precisely by seeing it is still set with no venue left | ||
| refresh(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"Leave venue" can leave the UI stuck in venue viz.
If PREV_VIZ_KEY was never written (e.g. first-ever "Play here" click with no prior explicit vizSelection), prev is falsy here and neither vizSelection nor window.setViz is restored — the visualization stays on 'venue' indefinitely with no venue selected.
🩹 Proposed fix
try {
localStorage.setItem(VENUE_OVERRIDE_KEY, NO_VENUE);
- const prev = localStorage.getItem(PREV_VIZ_KEY);
- if (prev) {
- localStorage.setItem('vizSelection', prev);
- if (typeof window.setViz === 'function') window.setViz(prev);
- }
+ const prev = localStorage.getItem(PREV_VIZ_KEY) || 'grid';
+ localStorage.setItem('vizSelection', prev);
+ if (typeof window.setViz === 'function') window.setViz(prev);
} catch (_) { /* ok */ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (e.target.closest('[data-career-unselect]')) { | |
| try { | |
| localStorage.setItem(VENUE_OVERRIDE_KEY, NO_VENUE); | |
| const prev = localStorage.getItem(PREV_VIZ_KEY); | |
| if (prev) { | |
| localStorage.setItem('vizSelection', prev); | |
| if (typeof window.setViz === 'function') window.setViz(prev); | |
| } | |
| } catch (_) { /* ok */ } | |
| // keep _appliedManifestVenue: pushCrowdManifest clears the crowd | |
| // manifest precisely by seeing it is still set with no venue left | |
| refresh(); | |
| } else if (e.target.closest('[data-career-unselect]')) { | |
| try { | |
| localStorage.setItem(VENUE_OVERRIDE_KEY, NO_VENUE); | |
| const prev = localStorage.getItem(PREV_VIZ_KEY) || 'grid'; | |
| localStorage.setItem('vizSelection', prev); | |
| if (typeof window.setViz === 'function') window.setViz(prev); | |
| } catch (_) { /* ok */ } | |
| // keep _appliedManifestVenue: pushCrowdManifest clears the crowd | |
| // manifest precisely by seeing it is still set with no venue left | |
| refresh(); |
🤖 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/screen.js` around lines 246 - 257, Update the venue-unselect
branch handling the PREV_VIZ_KEY so that when no previous visualization is
stored, it explicitly restores the default non-venue visualization instead of
leaving the current 'venue' selection active. Continue restoring the stored
value through localStorage and window.setViz when prev exists, then call
refresh() as before.
1 participant
Summary
Second slice of the career-mode epic (follows #905, lands independently — the two share no files and each degrades without the other):
plugins/career/(new bundled plugin):song_statstable → 60/75/85 % = 1/2/3 ★ (thresholds data-driven invenues.json), with the existing-song filter so orphaned stats don't count.CONFIG_DIR/plugin_uploads/career/venues/<id>/with sha256 verification, zip-slip-guarded extraction, manifest validation, and atomic swap-in; served back viaFileResponsewith the highway_3d no-cache/nosniff recipe.venues.jsonshipspack: nulluntil PR 3 publishes release assets.PROMOTED_PLUGINS): star total, next-unlock progress bar, per-venue cards with download/remove/play-here, unlock toasts viafbNotify.v3VenueCrowd.setManifest, from feat(venue): reactive crowd video layer behind the 3D highway (career mode 1/3) #905), guarded so either side being absent is a clean no-op.Testing
tests/plugins/career/(11 pytest): star math, threshold unlocks, orphaned-stats exclusion, 403/404/409 paths, traversal guards, and the download worker end-to-end (real zip via file://, sha256 verify, corrupt-hash rejection).tests/js/career_plugin.test.js: manifest/venues.json shape, shell promotion wiring, crowd-manifest push degradation.career.cssfor classes outside the core glob).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests