feat(career): first-launch milestone + download-size disclosure#1024
feat(career): first-launch milestone + download-size disclosure#1024mhglover wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe career plugin now distinguishes published venue packs by committed size, exposes nullable pack sizes, gates downloads accordingly, displays known sizes, and integrates two career achievements with boot and gig lifecycle events. ChangesCareer runtime and achievements
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CareerScreen
participant AchievementsQueue
participant AchievementsAPI
participant LocalStorage
CareerScreen->>AchievementsQueue: Queue registerAll and career_started unlock
AchievementsQueue->>AchievementsAPI: Drain queued achievement operations
CareerScreen->>LocalStorage: Read feedBack-career-welcomed
CareerScreen->>AchievementsAPI: Unlock career_started once
CareerScreen->>AchievementsAPI: Unlock first_venue_gig after gig completion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
19d6ca4 to
6d30bef
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/career/tests/passports.test.js (1)
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated code.
The
pending2.forEachline is duplicated three times.♻️ Proposed fix
const pending2 = w2.__feedBackAchievementsPending || []; const unlocked2 = []; pending2.forEach((fn) => fn({ register() {}, registerAll() {}, unlock: (id) => unlocked2.push(id) })); - pending2.forEach((fn) => fn({ register() {}, registerAll() {}, unlock: (id) => unlocked2.push(id) })); - pending2.forEach((fn) => fn({ register() {}, registerAll() {}, unlock: (id) => unlocked2.push(id) })); assert.deepEqual(unlocked2, []); });🤖 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/tests/passports.test.js` around lines 269 - 273, Remove the repeated pending-achievement execution setup in the test by extracting the shared pending2 iteration and unlock collection into a reusable local helper, then use that helper for each duplicated assertion while preserving the existing expected unlocked IDs.tools/content_packs.py (1)
128-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-hashing the pack a second time.
build_pack()'s return value ({"sha256", "bytes"}) is discarded, thenmanifest_entry()re-reads the whole zip (out_zip.read_bytes()) and recomputes its sha256 from scratch just to get the same numbers. For a ~300MB+ venue pack (e.g.arenaat ~335MB pervenues.json), that's a second full read+hash pass per pack, in every CI build/publish run.♻️ Proposed fix
for src in args.src: pid = _pack_id(src) zip_path = out_dir / pack_asset(pid, args.version) - build_pack(src, zip_path) + info = build_pack(src, zip_path) if args.publish: publish(pid, args.version, zip_path) url = pack_url(pid, args.version) else: url = (out_dir.resolve() / zip_path.name).as_uri() - manifest[pid] = manifest_entry(zip_path, url) + manifest[pid] = {"url": url, **info}🤖 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 `@tools/content_packs.py` around lines 128 - 138, Reuse the metadata returned by build_pack() when constructing each manifest entry instead of discarding it. Update manifest_entry() and its call in the surrounding pack loop to accept the build result’s sha256 and bytes, avoiding any second read or hash of the generated zip while preserving the existing URL and manifest format.
🤖 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 @.github/workflows/content-packs.yml:
- Around line 42-54: Update the “Build & publish packs” step to pass the
workflow inputs through the step’s env block rather than interpolating
github.event.inputs.venues or github.event.inputs.version directly into run.
Read those environment variables inside the shell script, preserving the
existing directory construction and content_packs.py invocation while preventing
input values from being parsed as shell syntax.
- Around line 70-81: Scope the create-pull-request step around “Open
manifest-bump PR” to commit only the manifest file, excluding the untracked
_packs/ zip artifacts produced by the publish step. Configure the action’s file
selection input while preserving the existing branch, commit message, title, and
body.
---
Nitpick comments:
In `@plugins/career/tests/passports.test.js`:
- Around line 269-273: Remove the repeated pending-achievement execution setup
in the test by extracting the shared pending2 iteration and unlock collection
into a reusable local helper, then use that helper for each duplicated assertion
while preserving the existing expected unlocked IDs.
In `@tools/content_packs.py`:
- Around line 128-138: Reuse the metadata returned by build_pack() when
constructing each manifest entry instead of discarding it. Update
manifest_entry() and its call in the surrounding pack loop to accept the build
result’s sha256 and bytes, avoiding any second read or hash of the generated zip
while preserving the existing URL and manifest format.
🪄 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: a6cf1cae-5b04-4281-bd4f-870f576554f3
📒 Files selected for processing (7)
.github/workflows/content-packs.ymlplugins/career/routes.pyplugins/career/screen.jsplugins/career/tests/passports.test.jsplugins/career/venues.jsontests/plugins/career/test_routes.pytools/content_packs.py
| - name: Open manifest-bump PR | ||
| uses: peter-evans/create-pull-request@v6 | ||
| with: | ||
| commit-message: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | ||
| title: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | ||
| body: | | ||
| Automated by the content-packs workflow after publishing | ||
| `${{ github.event.inputs.venues }}` v${{ github.event.inputs.version }} | ||
| to their `venue-<id>-v${{ github.event.inputs.version }}` releases. | ||
| Bumps `venues.json` pack url/sha256/bytes to match the uploaded zips. | ||
| branch: content-packs/manifest-bump | ||
| delete-branch: true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== workflow snippet ==\n'
sed -n '1,140p' .github/workflows/content-packs.yml
printf '\n== tool outline ==\n'
ast-grep outline tools/content_packs.py --view expanded || true
printf '\n== tool search for _packs and local/publish ==\n'
rg -n "_packs|--local|--publish|create-pull-request|add-paths|venues.json" tools/content_packs.py .github/workflows/content-packs.ymlRepository: got-feedBack/feedBack
Length of output: 5197
🌐 Web query:
peter-evans/create-pull-request v6 add-paths default commit all modified untracked files workspace
💡 Result:
In peter-evans/create-pull-request v6, the add-paths input determines which files are included in the pull request commit [1][2]. By default, if no paths are specified, the action automatically adds all new (untracked) and modified (tracked) files present in the repository workspace [1][3][4]. When using the add-paths input, you can provide a comma or newline-separated list of file paths using git pathspec syntax [1][2]. Importantly, if you specify paths, any file changes in the workspace that do not match the provided paths are automatically stashed and restored after the action completes, ensuring only the intended files are committed to the pull request [1][2].
Citations:
- 1: https://github.com/peter-evans/create-pull-request/blob/v6.0.0/README.md
- 2: https://github.com/peter-evans/create-pull-request/tree/v6.0.0
- 3: https://github.com/peter-evans/create-pull-request/blob/v6.0.0/action.yml
- 4: https://github.com/peter-evans/create-pull-request
🌐 Web query:
peter-evans/create-pull-request add-paths default all modified untracked files documentation
💡 Result:
The peter-evans/create-pull-request GitHub Action automatically detects and commits all new (untracked) and modified (tracked) files by default [1][2][3]. Regarding the add-paths input: 1. Default Behavior: If the add-paths input is omitted (left blank), the action automatically includes all modified and untracked files [1][2]. 2. Using add-paths: When you specify paths using the add-paths input, only the files that match the provided pathspec are committed [1][2]. 3. Handling Excluded Files: If you use add-paths to select specific files, any other changes in your workspace (tracked or untracked) that do not match your specified paths will be stashed and restored after the action finishes, ensuring they are not included in the pull request [1][2][4]. In summary, you do not need to explicitly configure anything to include all modified and untracked files; simply omitting the add-paths parameter achieves this by default [1][2].
Citations:
- 1: https://github.com/peter-evans/create-pull-request
- 2: https://github.com/marketplace/actions/create-pull-request
- 3: https://peterevans.dev/posts/github-actions-how-to-create-pull-requests-automatically/
- 4: https://github.com/peter-evans/create-pull-request/blob/main/docs/updating.md
Scope the manifest-bump PR to the manifest file. peter-evans/create-pull-request@v6 will otherwise include the untracked _packs/ zip artifacts from the publish step, since it commits all modified and new files by default.
Suggested change
- name: Open manifest-bump PR
uses: peter-evans/create-pull-request@v6
with:
commit-message: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})"
title: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})"
body: |
Automated by the content-packs workflow after publishing
`${{ github.event.inputs.venues }}` v${{ github.event.inputs.version }}
to their `venue-<id>-v${{ github.event.inputs.version }}` releases.
Bumps `venues.json` pack url/sha256/bytes to match the uploaded zips.
+ add-paths: plugins/career/venues.json
branch: content-packs/manifest-bump
delete-branch: true📝 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.
| - name: Open manifest-bump PR | |
| uses: peter-evans/create-pull-request@v6 | |
| with: | |
| commit-message: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | |
| title: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | |
| body: | | |
| Automated by the content-packs workflow after publishing | |
| `${{ github.event.inputs.venues }}` v${{ github.event.inputs.version }} | |
| to their `venue-<id>-v${{ github.event.inputs.version }}` releases. | |
| Bumps `venues.json` pack url/sha256/bytes to match the uploaded zips. | |
| branch: content-packs/manifest-bump | |
| delete-branch: true | |
| - name: Open manifest-bump PR | |
| uses: peter-evans/create-pull-request@v6 | |
| with: | |
| commit-message: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | |
| title: "career: refresh venue pack manifest (v${{ github.event.inputs.version }})" | |
| body: | | |
| Automated by the content-packs workflow after publishing | |
| `${{ github.event.inputs.venues }}` v${{ github.event.inputs.version }} | |
| to their `venue-<id>-v${{ github.event.inputs.version }}` releases. | |
| Bumps `venues.json` pack url/sha256/bytes to match the uploaded zips. | |
| add-paths: plugins/career/venues.json | |
| branch: content-packs/manifest-bump | |
| delete-branch: 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 @.github/workflows/content-packs.yml around lines 70 - 81, Scope the
create-pull-request step around “Open manifest-bump PR” to commit only the
manifest file, excluding the untracked _packs/ zip artifacts produced by the
publish step. Configure the action’s file selection input while preserving the
existing branch, commit message, title, and body.
Wire the opt-in content model into the career UI: - Download-size disclosure: the "Download venue pack" button now states the pack size (e.g. "Download venue pack (~335 MB)"), read live from the manifest (`pack.bytes`, exposed as `pack_bytes` in /state) so the number can't drift from the real asset. Absent/zero size → no suffix. - Career achievements: career is the first source plugin to use the achievements cross-plugin API. Registers career_started + first_venue_gig and unlocks them on first visit / first logged gig (competency only, idempotent). No new event bus; uses the documented pending-queue contract. - No progress loss on upgrade: verified `unlocked` is computed from stars (meta.db), independent of pack presence — a previously-unlocked venue whose media is no longer bundled shows unlocked + downloadable, never locked. Tests: upgrade/no-progress-loss (arena unlocked with media unbundled → downloadable → restored); formatBytes MB/GB; achievement wiring drains the pending queue and fires career_started once. All green. Stacked on the venue-packs PR (feedBack#1023). Part of #122. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Harris Glover <matthew@harrisglover.com>
…ian learning Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Harris Glover <matthew@harrisglover.com>
6d30bef to
cba2aad
Compare
Rebased onto merged main (was stacked on #1023/#1024, whose venue work is now in main) so it no longer carries a stale content_packs.py that would revert 1023's build_pack fixes. - build_vst_pack: slice a fat .vst3 tree to one platform (keep its binary dir + shared bundle files, drop the two foreign platform dirs and src/ build trees). Pins create_system=3 like build_pack — without it the same tree hashes differently on a Windows runner (native .vst3 are built there), breaking the precomputable-hash guarantee exactly where it matters. - Publish wiring: 'python tools/content_packs.py <vst-root> --vst --version N --publish' builds+uploads vst-<plat>-vN releases for mac/win/linux and emits a platform-keyed {url,sha256,bytes} manifest — the shape rig_builder's data/vst_packs.json consumes. publish() refactored onto a shared _publish_release helper (venue behaviour unchanged). - Tests: slice keeps target+shared/drops foreign, per-platform binary, reproducibility, unknown-platform reject, and a simulated-win32 guard that fails if the create_system pin is dropped. selfcheck covers the VST path. Original build_vst_pack by Matthew Harris Glover; reworked for the create_system fix, publish wiring, and rebase. Signed-off-by: byrongamatos <xasiklas@gmail.com> Co-authored-by: byrongamatos <xasiklas@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Draft — stacked on feedBack#1023 (venue packs). Review/merge that first; this branch includes its commits until then. Part of the nightly-slimming effort (feedBack-desktop#122).
Wires the opt-in content model into the career UI.
What's here
pack.bytes, exposed aspack_bytesin/state) so the number can't drift from the real asset. Absent/zero → no suffix (e.g. club until its release is published).career_started+first_venue_gigand unlocks them on first visit / first logged gig (competency only, idempotent, documented pending-queue contract — no new event bus).unlockedis computed from stars (meta.db), independent of pack presence, so a previously-unlocked venue whose media is no longer bundled shows unlocked + downloadable, never locked/broken.Tests (all green)
_download_packrestores it.formatBytesMB/GB rounding + empty-when-unknown.boot()drains the pending queue → registers both, unlockscareer_startedonce (not re-fired when already welcomed).Notes
career_startedmilestone (the achievements plugin surfaces its own toast).Design intent (achievements)
These achievements reward real musician learning, not game busywork. The opt-in content model exists so the player grows into the game, and the achievements mark milestones on that real-world path:
The aim: playing the game rewards you for learning the craft. Contributors adding achievements here or in sibling plugins should keep to that spirit — celebrate a skill or decision a working musician would recognize. (Same note is in the code above
CAREER_ACHIEVEMENTS.)Summary by CodeRabbit
New Features
Bug Fixes
Tests