feat: initial tutorial-packs plugin (intro-bends + reading-the-highway) by byrongamatos · Pull Request #1 · slopsmith/slopsmith-plugin-tutorials

byrongamatos · 2026-05-27T22:27:35Z

What this adds

An interactive video-based tutorial system for Slopsmith. The plugin lives at `/api/plugins/tutorials/` and adds a top-level Tutorials nav entry that exposes three modes:

Browse — pack grid with progress bars and technique tags
Lesson — video player (YouTube embed or uploaded webm/mp4) + "Start exercise" button that launches the paired sloppak via `window.playSong`, plus a manual run-record form that awards XP through the minigames profile
Author — full pack/lesson editor with video upload, sloppak picker, per-lesson thumbnail upload, pass/mastery thresholds, and XP fields

Two starter content packs ship in `builtin/`:

intro-bends — 3 lessons on half-step bends, full-step bends, and bend+vibrato
reading-the-highway — 10 lessons covering every notation type the highway renders (notes, chords, bends, slides, HO/PO, harmonics, palm mute, dead notes, tremolo, taps, combined)

Why `plugin.json` has no `nav` block

The plugin deliberately omits the `nav` block. The standard `nav` entry would add Tutorials as a dropdown item inside the plugins menu. Instead, `screen.js:injectTopLevelNav()` inserts a sibling link next to Library / Favorites / Upload / Settings — the user-facing design decision being that tutorials are a first-class nav destination, not buried in a plugin submenu. The id guards on each injection (`tut-nav-top-link`, `tut-nav-top-link-mobile`) prevent duplicates on `loadPlugins()` re-runs.

Format conventions

Sloppak schema — beat fields use `{"time":..., "measure":...}` (not `{"t":..., "bar":...}` — we hit this schema mismatch already during development)
RS string-index convention — `s=0` is low E, `s=5` is high E, per `slopsmith/lib/gp2rs.py:459` ("Convert GP string number (1=high) to RS string index (0=low)"). The generate scripts and sloppak note objects follow this exactly
Atomic file writes — `pack.json` and `progress.json` use temp+fsync+rename per constitution §VII; video/cover/thumb uploads stream to a `.part` temp file, then `os.replace` when complete
Builtin pack sloppak resolution — exercise sloppak paths in builtin `pack.json`s are DLC-relative (`tutorials-builtin//.sloppak`). `setup()` calls `_seed_builtin_packs(plugin_dir, dlc_dir)` which copies those sloppaks into `<DLC_DIR>/tutorials-builtin//` so `window.playSong` can resolve them via the core `/ws/highway` route without any user action

Upload caps

Asset	Extension	MIME check	Size cap
Video	mp4, webm	yes	100 MB
Cover / thumb	png, jpg, webp	yes	4 MB
Manifest (PUT)	—	JSON	256 KB

Generators are reproducible

The `sloppaks/`, `cover.png`, and `thumbs/` in each builtin pack are committed so consumers get working content out-of-the-box. To regenerate:

cd builtin/intro-bends && python3 generate.py
cd builtin/reading-the-highway && python3 generate.py

Requires `fluidsynth` + `FluidR3_GM.sf2` (both present in the Slopsmith Docker image).

Codex preflight findings addressed

Builtin sloppaks not resolvable by playSong — seeder now copies to `<DLC_DIR>/tutorials-builtin/`; pack.json paths updated to DLC-relative form
`||0.7` coerces explicit-zero thresholds — replaced with `_numDefault`/`_intDefault` helpers
TDZ crash on plugin reload — hot-reload guard moved below `const` declarations
`song:ended` duplicate subscription — guarded via `slopsmithTutorials.__alive` check, subscribed once only
`pendingRun` left set when playSong unavailable — early-return before setting

Adds slopsmith-plugin-tutorials: an interactive video-based tutorial system with Browse, Lesson, and Author modes. Fixes applied from Codex preflight: - Builtin pack sloppaks are now seeded into <DLC_DIR>/tutorials-builtin/ so playSong can resolve them via the core highway WS - pack.json exercise references updated to use DLC-relative paths - readLessonEditor uses NaN-safe _numDefault/_intDefault helpers instead of || so an explicitly authored 0 threshold is preserved - Hot-reload guard moved below const declarations to avoid TDZ crash - song:ended subscription guarded against duplicate on plugin reload - pendingRun no longer set when playSong is unavailable - Stale screen.html routing comment corrected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T22:30:43Z

@coderabbitai review

Copilot

Pull request overview

This PR introduces the initial Tutorials plugin for Slopsmith, adding a video-and-exercise tutorial system with backend pack management, frontend browse/authoring UI, and bundled starter packs.

Changes:

Adds plugin manifest, screen UI, SPA behavior, and FastAPI routes for packs, uploads, progress, and run recording.
Ships builtin tutorial pack manifests, generators, covers/thumbnails, and sloppak assets.
Adds README and agent guidance documenting architecture and usage.

Reviewed changes

Copilot reviewed 11 out of 39 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
`plugin.json`	Declares the Tutorials plugin entry points and server file settings.
`screen.html`	Adds the plugin root layout and scoped inline styles.
`screen.js`	Implements browse, lesson, authoring, uploads, playback launch, and run submission UI.
`routes.py`	Adds backend pack CRUD, media serving/upload, builtin seeding, sloppak copy, and progress APIs.
`README.md`	Documents the plugin concept, API, generation flow, and architecture notes.
`CLAUDE.md`	Adds repository-specific architecture invariants and out-of-scope notes.
`builtin/intro-bends/README.md`	Documents the Intro to Bends starter pack asset checklist.
`builtin/intro-bends/pack.json`	Defines the 3-lesson Intro to Bends builtin pack.
`builtin/intro-bends/generate.py`	Adds generator for Intro to Bends sloppaks, cover, and thumbnails.
`builtin/reading-the-highway/pack.json`	Defines the 10-lesson Reading the Highway builtin pack.
`builtin/reading-the-highway/generate.py`	Adds generator for Reading the Highway sloppaks, cover, and thumbnails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- routes.py: add type/range validation for pass/mastery thresholds in _validate_manifest so record_run never hits AttributeError on a bad PUT - screen.js: guard file-video branch with `video.src && typeof video.src === 'string'` so empty src falls through to "No video attached" state instead of rendering a broken <video> element - builtin/*/pack.json: clear video.src to "" (videos not yet recorded) and update sloppak refs to DLC-relative paths for playSong resolution - builtin/reading-the-highway/generate.py: replace hash() with hashlib.sha256-based seed for reproducible generation; fix manifest template to write DLC-relative sloppak paths and empty video.src - builtin/intro-bends/generate.py: correct string-index docstring (s=0 is LOW E, opposite of GP numbering) - builtin/intro-bends/README.md: update content checklist (sloppaks / cover / thumbs now marked committed; video files remain pending) - README.md: correct /runs API description; remove stale XML artifact - CLAUDE.md: correct XP architecture (frontend posts both independently; no server-side relay) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T22:51:17Z

Addressing the two intentionally-deferred Copilot findings:

screen.js:122 — stale song:ended closure on reload
Not fixing in v1. The __alive guard at the IIFE bottom already prevents a duplicate subscription on reload. The stale-closure concern (handler still references the old state object after a settings-reload cycle) is real but only occurs in a dev-mode reload scenario, not normal playback. Will revisit when the IIFE gets a proper teardown/remount lifecycle.

screen.js:812 — lesson.xp fields unused by submitRun
By design. The xp fields are v1 schema placeholders. XP is currently hardcoded on the minigames side by game_id. Per-lesson XP customisation (letting Authors set a different XP reward) is planned for a later iteration once the minigames plugin exposes a custom_xp field on its /runs endpoint. The Author form persists them now so authored packs don't need a migration later.

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 6 comments.

- screen.js: add renderToken guard to prevent stale async renders; renderPackDetail and renderLesson bail if state.renderToken changed while awaiting /packs fetch (navigation-away race) - screen.js: add role/tabindex/onkeydown to pack card (article) and lesson row (div) so keyboard-only users can open them with Enter/Space - screen.js: rename "Locked" lesson state label to "Not started" — lessons are not actually gated so "Locked" was misleading Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T22:57:51Z

Round 2 deferred findings — intentional non-fixes:

screen.js:1093 — stale onSongEnded closure after hot-reload
Same v1 decision as the round-1 finding on screen.js:122. The __alive guard prevents a second subscription from being added on reload, so there is no duplicate. The stale-closure issue (handler closed over old state) is real but only observable during settings-reload cycles in a dev session, not during normal playback. Will address properly when the IIFE gets teardown/remount lifecycle support.

screen.js:461 — lesson.xp Author fields unused in run submission
Same as the earlier finding on screen.js:812. This is a v1 schema placeholder: the Author UI saves XP values into the manifest so pack authors can declare intent, and a later minigames endpoint update will consume a custom_xp field. Wiring it through today would require a schema bump on the minigames side outside this PR's scope.

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 3 comments.

- routes.py: tighten null-threshold validation in _validate_manifest; use `in` to detect explicit JSON null (vs missing key), and also reject bool values (bool is a subclass of int in Python) - screen.js: strip derived fields (cover_url, lesson.thumb_url) from manifest before PUT so response-only URLs are never persisted to pack.json and cannot become stale after the underlying file is removed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T23:04:33Z

Round 3 deferred finding:

screen.js:1107 — stale onSongEnded closure on reload (third occurrence)
Same v1 decision as the earlier findings on screen.js:122 and screen.js:1093. The __alive guard ensures the subscription fires exactly once across all IIFE executions. The stale-closure concern only materialises during dev-mode hot-reload, not normal playback. Filed for the teardown/remount milestone.

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 10 comments.

- screen.js: guard onSongEnded against stale pendingRun — clear and return if activePackId/activeLessonId no longer match when song ends - screen.js: clamp packProgressPct to 100% to prevent over-count from stale progress entries on removed/recreated packs - screen.js: strip cover_url/thumb_url derived fields before PUT (done in round 3); no new changes here — companion for the HTML fix below - screen.html: replace role="tablist" + aria-selected on mode buttons with aria-label="Tutorials mode" + aria-pressed; update matching CSS selector from [aria-selected="true"] to [aria-pressed="true"] so the active-mode highlight is not lost - builtin/*/generate.py: copy sloppaks to tutorials-builtin/<pack>/ subdirectory instead of DLC root, matching the DLC-relative paths in the committed manifests (fixes immediate playability after regen) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tics The Author form previously said "arrangement id (optional)" which implied string IDs like "lead" would work. Core's playSong only accepts integer arrangement indices; string IDs are silently ignored. Update the placeholder to "arrangement index 0, 1, 2… (optional)". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T23:18:03Z

Round 4 deferred findings:

screen.js:1118 — stale onSongEnded closure on reload (fourth occurrence)
Same v1 decision as all prior rounds. Deferred to teardown/remount milestone.

screen.js:952 and screen.js:967 — cover upload/remove calls refreshAndRender() and wipes unsaved Author edits
Acknowledged as a real UX limitation. In v1, all Author state lives in the form DOM and is not persisted until the user presses Save. Any operation that triggers a full re-render (including cover upload/remove) rebuilds the form from the last-saved manifest, discarding unsaved edits. Fixing this properly would require either: (a) extracting form state into state before re-rendering, or (b) using partial DOM updates for cover/thumb operations that don't touch the lesson list. Deferring to a follow-up PR — the current behavior is a UX inconvenience, not a data-loss bug (the last Save is always intact).

screen.js:475 — lesson.xp fields unused in minigames payload
Same v1 schema-placeholder decision as every prior round.

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 8 comments.

- routes.py: add range validation for accuracy thresholds (must be in [0, 1]) and mastery.speed (must be positive) in _validate_manifest - screen.js: add title attr to YouTube iframe for screen-reader users - screen.js: add aria-label="Remove tag <value>" to tag-chip remove buttons so assistive tech can identify which tag is being removed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-27T23:26:19Z

Round 5 deferred findings:

screen.js:1129 — stale onSongEnded closure on reload (fifth occurrence)
Same v1 decision as all prior rounds. Deferred to teardown/remount milestone.

screen.js:847 and screen.js:475 — lesson.xp fields unused in minigames payload
Same v1 schema-placeholder decision as every prior round.

screen.js:962 and screen.js:977 — cover upload/remove triggers full re-render
Same v1 limitation acknowledged in round 4. Deferred.

screen.js:489 — minigames XP awarded before local progress recorded
The ordering (minigames first) is intentional: it gives the user the XP reward feedback even if the local write encounters a transient error. XP deduplication across retries is the minigames plugin's responsibility via idempotent game_id semantics. A failure in the local /runs write after a successful minigames write is logged and shown to the user; they can retry the form submit which will re-post both. Double-awarding XP requires both a local-write failure AND a user retry — low probability and recoverable via minigames history.

- screen.js: fix arrangement parsing to use String(ex.arrangement ?? '') before trim() so numeric JSON values (e.g. exercise.arrangement: 1) don't throw, and arrangement 0 isn't dropped by the || coercion; also use /^\d+$/ regex to reject partial-int strings like "1abc" - README.md: expand generator requirements section to list all needed tools: fluidsynth + soundfont, ffmpeg, mido, Pillow, PyYAML Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 3 comments.

… in record_run - Move the stable song:ended wrapper registration out of init() and into the IIFE bottom section (after the singleton is created), so the wrapper is always registered on first load regardless of readyState. On hot-reload the _onSongEnded pointer is updated at the top of the hot-reload branch before init() is called, rather than inside init() where the old guard `if (window.slopsmithTutorials)` could be missed. - In record_run, use explicit `is not None` checks instead of dict.get() defaults for pass/mastery thresholds. dict.get(key, default) returns None when the key is present with a JSON null value, which would cause TypeError in the accuracy comparisons. The new pattern falls back to the safe default (0.7 / 0.9 / 1.0) only when the value is genuinely None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 3 comments.

…e response Introduce refreshPacksOnly() that updates state.packs without calling render(), so the Browse-side pack card picks up a fresh cover_url on the next navigation without destroying any unsaved Author form state (lesson list, thresholds, titles) currently in the form. Cover upload and cover remove now both await refreshPacksOnly() instead of refreshAndRender(), preserving all unsaved Author edits. The local refreshPreview() call still fires immediately to update the preview element in place. Also: the cover delete handler now checks dr.ok and throws on non-2xx responses instead of silently updating the UI on a failed delete. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 2 comments.

The copy_sloppak endpoint previously allowed any file extension from the DLC directory to be copied into a pack's sloppaks/ dir and served via the GET /sloppaks/{filename} route. Only .sloppak files should be copyable. Add a suffix check that returns 400 for any other extension. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 5 comments.

…rrangement ?? fix - delete_cover and delete_lesson_thumb now return 404 when the pack directory does not exist, matching the behavior of upload and other mutating endpoints. Previously glob on a missing directory silently returned an empty list, making the response indistinguishable from a successful delete of an already-absent file. - Thumbnail delete handler now checks dr.ok and throws on non-2xx responses, matching the cover delete fix from the prior commit. - arrangement input initial value: use `??` instead of `||` so a numeric 0 arrangement index is preserved in the Author form. The `||` operator treats 0 as falsy and blanks the field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 39 changed files in this pull request and generated 2 comments.

- Wrap window.playSong() in try/catch; clear state.pendingRun and alert on synchronous exception so a playSong error does not leave the run permanently pending. - Raise speed input max from 1.5 → 2.0 to match RunRecord.speed upper bound; lessons with mastery.speed thresholds above 1.5 were previously unreachable via the form. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

byrongamatos · 2026-05-28T00:32:15Z

@coderabbitai review

Copilot AI review requested due to automatic review settings May 27, 2026 22:27

Copilot started reviewing on behalf of byrongamatos May 27, 2026 22:27 View session