Add manual lyrics editor: tap-to-time and waveform drag-editing

[carochacs]

What

Both existing lyrics plugins (lyrics_sync, lyrics_karaoke) are generate-only: paste text → Whisper forced-align → save, with no way to fix the ~10% alignment gets wrong, or author lyrics for a song with no vocals stem at all.

Adds a manual hand-editing surface to this plugin, whose manifest already describes it as "Author and time-align song lyrics":

• Tap-to-time — play the song, tap Space per syllable; each tap stamps that syllable's start time and closes the previous syllable's duration
• Waveform drag-editing — syllable blocks render as draggable/resizable blocks over a client-decoded waveform (WebAudio, decoded once per stem load — not on the render loop); drag to move, drag an edge to resize, double-click to edit text, snap to neighbours (Alt to bypass)
• Load & correct — "Open Editor" loads a song's existing lyrics.json via a new GET /lyrics endpoint; "Edit Manually" on an alignment preview seeds the editor from a fresh align result before it's saved
• Text seeding with manual /-syllable splits, undo/redo, A–B loop, playback speed control, stem picker
• Dirty-state tracking: reloading the editor, navigating away, or switching songs while there are unsaved edits prompts for confirmation instead of silently discarding them

Saves go through a new POST /save-lyrics, validated server-side (finite numeric t/d, drop non-positive durations, round, sort) — stricter than the sloppak loader's own read-side filter, so nothing the editor writes can be silently dropped on the next load. An empty list is a valid save (clears the track); saving a session where every syllable is still untimed confirms first, since that would otherwise clear the track's existing lyrics silently.

Also fixes

The existing align-based /save wrote lyrics.json into the extraction cache for zip-form .sloppak files instead of the archive itself, so edits were silently lost whenever the cache was invalidated. Both save paths now funnel through one _persist_lyrics helper (ported from lyrics_karaoke's zip-repack pattern) that rewrites directory-form packs in place and re-zips zip-form packs with a one-time .bak.

Also hardens _find_vocals_stem against a manifest-controlled path escaping the pack directory, matching the containment check already applied to request-supplied filenames (mirrors the existing _safe_dlc_path fix from #5/#6, one layer further in).

Testing

• tests/test_helpers.py (17 tests, committed) — path-traversal rejection for both request-supplied and manifest-supplied paths, manifest IO, atomic write, _rezip_sloppak (including the one-time .bak and that a second call doesn't clobber it), and — the load-bearing case — _persist_lyrics for both directory-form and zip-form sloppaks, asserting the write lands inside the actual .zip archive rather than only the extraction cache. That's a direct regression test for the persistence bug this PR fixes. Also covers the empty-list clear case and LRC formatting.
• Scope note: _resolve_sloppak/_find_vocals_stem/the setup()-registered routes aren't covered by the committed suite — they import core's sloppak module, which isn't available in this plugin repo's isolated CI (only supplied by the feedBack host at plugin-load time). Matches the existing scope boundary lyrics_karaoke's own test suite already draws for the same reason.
• Regression-checked the existing POST /save (align path) manually against a local core checkout, unchanged behavior.
• node --check screen.js, cross-checked every onclick/onchange handler in screen.html resolves to a defined JS function.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c1d83f95-7161-48b8-bac0-3a948fbad8a8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

Important

Two flows can silently write or discard the wrong data, and are worth addressing before merge: an alignment result isn't bound to the song it was computed for, so "Edit Manually"/"Save to Song" can persist the previous song's alignment into the currently selected one; and re-opening the editor (or leaving the screen) discards unsaved edits with no warning. Both are detailed inline, alongside two minor notes.

Reviewed changes

• Backend persistence rework — new _persist_lyrics / _rezip_sloppak / _atomic_write_json; both /save and the new /save-lyrics funnel through it, so zip-form .sloppak edits now land in the distributable archive (with a one-time .bak) instead of the extraction cache.
• New endpoints + manifest hardening — GET /lyrics (existing lyrics, stem URLs, duration) and POST /save-lyrics (validated; empty list clears the track); _safe_source_path refuses manifest-declared paths that escape the pack directory, and _find_vocals_stem now routes through it.
• Manual editor (screen.html/screen.js) — tap-to-time, waveform drag/resize editing, seed-from-text, A–B loop, playback speed, undo/redo, inspector; waveform peaks decoded once per stem via WebAudio and pre-rendered offscreen.
• Docs / manifest — README and plugin.json updates (feedBack rename, version 1.1.0, screen id plugin-lyrics_sync).

ℹ️ The tests described in the PR body aren't in the diff

The "Testing" section claims a round-trip persistence test through core's own sloppak.load_song (including after wiping the extraction cache), a full TestClient exercise of GET /lyrics and POST /save-lyrics (with the empty-list clear case), path-traversal rejection tests, and node --check. None of it is committed — the diff touches five files and no test file, and the repo has no tests/ directory. CI already self-detects tests/test_*.py / test/test_*.py (org-wide reusable ci.yml), so the claimed tests would run for free if added. The zip-persistence fix in particular repairs a class of silent data-loss bug that shipped before; a committed regression test for the re-zip + cache-wipe path would be high-value.

ℹ️ Nitpicks

• _lsEditorStartLoop keeps a rAF loop redrawing every frame for as long as the editor is open, even when idle; gating the redraw on actual state changes (audio position, selection, drag) would avoid constant full-canvas redraws on low-end hardware.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[carochacs]

Thanks for the thorough review — all four points addressed, pushed in 6e5636e and 7ea33fe:

Stale alignment result across song switches. lsSelectSong/lsClearSong now clear _lsAlignmentResult (and hide the preview) on every song change, so "Edit Manually"/"Save to Song" can no longer act on a previous song's alignment result.

Unsaved edits discarded silently. Added dirty tracking — a (generation, historyIndex) pair rather than a plain boolean, so undoing back to exactly the loaded/saved snapshot correctly reports clean again, and so a fresh alignment-seed (which reuses history index 0, same as a fresh disk-load) isn't mistaken for the clean state. lsOpenEditor's reopen-while-open path, lsCloseEditor, and the showScreen navigation hook all confirm before discarding. lsCloseEditor() now returns whether it actually closed; lsSelectSong/lsClearSong check that and abort the song switch if the user kept the editor open — otherwise they'd move on to the new song while the editor stayed open pointing at the old one.

Untimed-only save silently clearing the track. lsEditorSave now confirms before sending a payload that would be empty while _lsEditorSyllables isn't (all-untimed case).

Docstring accuracy + missing tests. Reworded ls_save_lyrics's docstring — you're right that it's stricter than the loader, not equivalent (the loader only checks type, no finiteness/d<=0/rounding). And added tests/test_helpers.py — this repo had no tests/ directory before, so nothing I ran locally was actually committed. It follows lyrics_karaoke's existing convention (pure/file-IO helpers only, since _resolve_sloppak/_find_vocals_stem need core's sloppak module which isn't importable in this repo's isolated CI). The load-bearing case is _persist_lyrics for both directory- and zip-form sloppaks, asserting the write lands inside the actual .zip — a direct regression test for the persistence bug this PR fixes. 17 tests, all passing locally.

Left the rAF-redraw-while-idle nitpick as-is per your own framing of it as deferable.

---

Generated by Claude Code

[pullfrog]

Important

The discard-prompt fix has one remaining hole: declining the prompt from "Edit Manually" still seeds the editor and wipes the unsaved edits it was meant to protect (details inline). Everything else in this delta addresses the prior review cleanly.

Reviewed changes

• Dirty-state tracking for the editor — new (generation, saved history index) scheme; re-opening or closing the editor now prompts on unsaved edits, song-switch and navigation abort when the user declines, and a successful save marks the editor clean.
• Stale-alignment guard — lsSelectSong/lsClearSong now clear the alignment result and hide the preview, so "Edit Manually"/"Save to Song" can't persist the previous song's alignment into the newly selected one.
• All-untimed save guard — saving a list where every syllable is untimed now confirms before posting the track-clearing empty payload.
• Committed test suite — new tests/test_helpers.py with 17 passing tests: path containment, manifest IO, atomic write, re-zip (incl. the one-time backup and the zip-persistence regression), _persist_lyrics for dir- and zip-form packs, the empty-list clear, and LRC formatting.
• Doc/manifest fixes — /save-lyrics docstring now correctly attributes validation strictness vs. core's loader; plugin version bumped to 1.1.1.

ℹ️ Nitpicks

• The PR body's "Testing" section still claims a TestClient exercise of GET /lyrics / POST /save-lyrics, a round-trip through core's sloppak.load_song, and node --check screen.js — none of those are in the committed suite. The committed tests are solid and the module docstring honestly scopes out the core-dependent routes, so this is just a body/commit mismatch worth trimming.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[carochacs]

Both real, and both fixed in 9bfd1d9:

lsEditInEditor seeding past a declined discard prompt. lsOpenEditor now returns true/false (actually reloaded vs. bailed — no song selected, or the user kept unsaved edits open), and lsEditInEditor's .then() checks it before touching _lsEditorSyllables.

History-cap eviction breaking the dirty check. You're right that keying "clean" to a history index doesn't survive the 50-entry cap shifting the saved snapshot out. Replaced the whole (generation, index) scheme with a content comparison instead — _lsEditorSavedSnapshot is a JSON snapshot of the syllables array taken at the last load/save, and _lsEditorIsDirty() just compares the live array against it by value. That sidesteps the ring-buffer bookkeeping entirely rather than trying to pin the saved index against eviction.

Also updated the PR body's Testing section — you were right that it still described tests (TestClient, sloppak.load_song round-trip) that aren't in the committed suite; trimmed it to describe what's actually there and why the core-dependent routes are out of scope for this repo's isolated CI.

---

Generated by Claude Code

[pullfrog]

✅ No new issues found.

Reviewed changes

• lsEditInEditor seed gating — lsOpenEditor now returns true only when it actually (re)loaded, and lsEditInEditor skips its alignment seed on false, closing the cancel-bypass where declining the discard prompt still wiped the unsaved edits it was meant to protect.
• Content-based dirty tracking — the (generation, history-index) scheme is replaced by comparing a serialized snapshot of _lsEditorSyllables against the live array by value, so the 50-entry undo cap can no longer evict the saved snapshot and make undo-to-start report clean while holding different content.
• PR body accuracy — the Testing section now correctly separates committed tests from manual checks, resolving the body/commit mismatch nitpick from the prior review.

Both open threads from the prior review are addressed and resolved. Verified locally: the 17 committed tests pass and node --check screen.js is clean.

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}