feat(editor): Tempo Map legibility S-pack — HUD legend, unmapped tail, barline copy - #220
Conversation
…, barline copy Charrette UX P6. Four legibility fixes to Tempo Map mode: - HUD legend: a colour key (mapped / selected / locked / suggested / unmapped) at the right end of the tempo HUD strip, using the EXACT pole colours _tempoMapDraw paints. Module-const data + a shared diagonal-hatch helper (no per-frame allocation); drawn only when it clears the guidance text so the two never collide on a narrow canvas. - Unmapped tail: the recording past the last confirmed downbeat carries no fitted tempo, so it now gets the doc-mandated hatched wash + "Unmapped" label in the grid, drawn under the notes/poles. - Lock copy correction: the old "global tempo re-fits will hold this beat" read as if locking were needed to KEEP an edit. New copy (shared LOCK_TOOLTIP + _lockStatusTextPure) makes clear manual edits always persist and a lock only defends a barline against automatic re-fits (Fit tempo / Suggest / Modulate). Applied to the right-click item's tooltip and the S-key status. - Vocabulary: retire user-facing "sync point" for "barline" (inspector hints, delete titles, lock/status messages); "sync point" stays internal/export only. Tests: tempo_map_guidance gains cases pinning the lock copy (accurate, no "sync point") and the inspector "barline" strings — all fail on main. HUD-text pins unchanged (already "barline"). Legend/tail verified live (pixel-sampled swatch colours; Unmapped label at the song end). npm test 114 green, lint 0 errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTempo Map rendering gains a color legend and hatched unmapped-tail visualization. Inspector, deletion, tap-tempo, and lock messaging now use “barline” terminology, with exported lock-copy helpers and tests covering the revised UI text. ChangesTempo Map legibility and copy
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tempo.js (1)
59-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new canvas branches with render tests.
The supplied tests validate copy helpers but do not exercise hatch rendering, legend collision/omission, or unmapped-tail clipping. Add canvas-mock cases for narrow and wide HUDs plus partially off-screen tails to protect these visual behaviors.
Also applies to: 242-269
🤖 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 `@src/tempo.js` around lines 59 - 115, Add canvas-mock render tests covering _tempoHatchRect clipping for partially off-screen unmapped tails, and _tempoDrawLegend behavior on both wide canvases and narrow canvases where the legend must be omitted due to guidance overlap. Assert the relevant canvas operations and state changes, while preserving existing copy-helper test coverage.
🤖 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 `@src/tempo.js`:
- Around line 59-115: Add canvas-mock render tests covering _tempoHatchRect
clipping for partially off-screen unmapped tails, and _tempoDrawLegend behavior
on both wide canvases and narrow canvases where the legend must be omitted due
to guidance overlap. Assert the relevant canvas operations and state changes,
while preserving existing copy-helper test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46561be2-acd7-4376-93d9-546e992484c1
📒 Files selected for processing (3)
CHANGELOG.mdsrc/tempo.jstests/tempo_map_guidance.test.mjs
The P6 copy sweep retired user-facing 'sync point' wording, but the tap-tempo stale-selection status still said 'sync-point selection changed'. Say 'barline' like every other status message, and add a source-scan regression test asserting no string literal in tempo.js carries the separated lowercase form (identifiers and comments stay internal vocabulary by design). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Charrette arch 5a / rhythm G5 / trans P6-prep (design slice 2a). Sparse labeled chips on the ruler show where the tempo and meter change through the song. ZERO storage — markers are a PURE function of S.beats (the executable truth, never a second source): _tempoMarkersPure derives tempo-change markers where a measure's BPM leaves the current run beyond 0.01 (the same constant _tempoHasMultipleMeasureBpmsPure uses) and meter markers where the numerator / den changes, with a bar-1 baseline of each and no spurious marker on a trailing partial bar. Memoized on editGen (+ the S.beats ref). Painted in the ruler band via timeToX only (the D-T1 invariant); tempo chips (cyan) and meter chips (violet) stack when they share a barline. Marker fields are never smuggled into S.beats entries. Filed feedpak-spec#51 for the AUTHORED markers the grid can't express (tempo ramps, meter groupings, fermata holds, lock/provenance) — slice 2b, range-provenance and community maps gate on it; not built here. tests/tempo_markers.test.mjs (7): baseline-only on a constant map, tempo-change detection, within-tolerance run behaviour, mid-song meter change + trailing- partial suppression, denominator change, degenerate inputs, and purity (no mutation of the input beats). Verified live: the ruler shows per-bar BPM chips tracking AC/DC's imported tempo drift + a 4/4 meter chip, no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only merged ruler #209/#211). Keep-both seam with #220 on tempo.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Charrette UX P5 / arch 1 (first half). Select and delete multiple Tempo Map barlines at once, without touching the single-focus model (S.tempoSel — which inspector / tap / lock / modulate / suggest all key on). - New S.tempoSelMulti: Set<downbeatIdx> (the S.drumSel pattern). Shift+click a pole extends the contiguous downbeat range; a drag on empty grid arms a tempo-marquee (the drum editor's deferred-3px `moved` idiom) that box-selects downbeats in its swept X range (plain replaces, Shift unions); Ctrl+A in mode selects every downbeat. The set is index-based, so it is CLEARED — never remapped — in TempoGridCmd exec/rollback and on Tempo Map exit. - Render: a light amber wash across the selected range, and selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass. - Bulk delete = ONE TempoGridCmd: _tempoDeleteBarlinesPure demotes every selected INTERIOR downbeat (never the first/last — the existing guard, generalized to a set) + one renumber. Reachable via Del and right-click "Delete N barlines". - Escape clears the selection, layered UNDER #215's suggest-dismiss (ghosts own Escape while showing). tests/tempo_multiselect.test.mjs (6): marquee hit math, the delete transform (demote+renumber, first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection. Verified live: Ctrl+A → "105 barlines selected" (amber render), Escape clears, a marquee box-selects, Ctrl+A+Del → "Deleted 103 barlines" (first/last kept), no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only on merged #215); keep-both seams with #220 (_tempoMapDraw) and #218/#225 on the shared tempo.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…5a) (#226) Charrette UX P5 / arch 1 (first half). Select and delete multiple Tempo Map barlines at once, without touching the single-focus model (S.tempoSel — which inspector / tap / lock / modulate / suggest all key on). - New S.tempoSelMulti: Set<downbeatIdx> (the S.drumSel pattern). Shift+click a pole extends the contiguous downbeat range; a drag on empty grid arms a tempo-marquee (the drum editor's deferred-3px `moved` idiom) that box-selects downbeats in its swept X range (plain replaces, Shift unions); Ctrl+A in mode selects every downbeat. The set is index-based, so it is CLEARED — never remapped — in TempoGridCmd exec/rollback and on Tempo Map exit. - Render: a light amber wash across the selected range, and selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass. - Bulk delete = ONE TempoGridCmd: _tempoDeleteBarlinesPure demotes every selected INTERIOR downbeat (never the first/last — the existing guard, generalized to a set) + one renumber. Reachable via Del and right-click "Delete N barlines". - Escape clears the selection, layered UNDER #215's suggest-dismiss (ghosts own Escape while showing). tests/tempo_multiselect.test.mjs (6): marquee hit math, the delete transform (demote+renumber, first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection. Verified live: Ctrl+A → "105 barlines selected" (amber render), Escape clears, a marquee box-selects, Ctrl+A+Del → "Deleted 103 barlines" (first/last kept), no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only on merged #215); keep-both seams with #220 (_tempoMapDraw) and #218/#225 on the shared tempo.js. Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…10) (#227) Charrette arch 5a / rhythm G5 / trans P6-prep (design slice 2a). Sparse labeled chips on the ruler show where the tempo and meter change through the song. ZERO storage — markers are a PURE function of S.beats (the executable truth, never a second source): _tempoMarkersPure derives tempo-change markers where a measure's BPM leaves the current run beyond 0.01 (the same constant _tempoHasMultipleMeasureBpmsPure uses) and meter markers where the numerator / den changes, with a bar-1 baseline of each and no spurious marker on a trailing partial bar. Memoized on editGen (+ the S.beats ref). Painted in the ruler band via timeToX only (the D-T1 invariant); tempo chips (cyan) and meter chips (violet) stack when they share a barline. Marker fields are never smuggled into S.beats entries. Filed feedpak-spec#51 for the AUTHORED markers the grid can't express (tempo ramps, meter groupings, fermata holds, lock/provenance) — slice 2b, range-provenance and community maps gate on it; not built here. tests/tempo_markers.test.mjs (7): baseline-only on a constant map, tempo-change detection, within-tolerance run behaviour, mid-song meter change + trailing- partial suppression, denominator change, degenerate inputs, and purity (no mutation of the input beats). Verified live: the ruler shows per-bar BPM chips tracking AC/DC's imported tempo drift + a 4/4 meter chip, no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only merged ruler #209/#211). Keep-both seam with #220 on tempo.js. Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
Charrette UX P6 — four legibility fixes to Tempo Map mode (the "S-pack"). No new commands.
1. HUD legend
A colour key at the right end of the tempo HUD strip — mapped · selected · locked · suggested · unmapped — using the exact pole colours
_tempoMapDrawpaints (slate / amber / emerald / dashed-amber ghost / hatch). Module-const data + a shared diagonal-hatch helper, so it rides the existing draw pass with no per-frame allocation. It only draws when it clears the guidance text to its left, so the two never collide on a narrow canvas (the legend simply drops out).2. Unmapped tail
The recording past the last confirmed downbeat carries no fitted tempo (design doc: that tail is Unmapped). It now gets the doc-mandated hatched wash + "Unmapped" label in the grid, drawn under the notes/poles so they stay legible.
3. Lock copy correction
The old status — "global tempo re-fits will hold this beat" — read as if you had to lock a barline to keep an edit. You don't; manual edits always persist. New shared copy (
LOCK_TOOLTIP+_lockStatusTextPure):Applied to the right-click item's tooltip and the S-key status line.
4. "sync point" → "barline"
User-facing "sync point" wording retired for "barline" (inspector hints, delete titles, lock/status messages). "sync point" stays only as internal/export vocabulary (code comments, the GoPlayAlong importer's own format term).
Tests & verification
tests/tempo_map_guidance.test.mjsgains cases pinning the corrected lock copy (accurate, contains no "sync point") and the inspector "barline" strings — all fail on main. The existing HUD-text pins are unchanged (that pure already said "barline").npm test114 green,npm run lint0 errors (3 pre-existing warnings = main's ratchet).Second PR of the tempo-mapping track (
TEMPO-MAPPING-PR-PROMPTS.md, PR 2). Independent of PR 1 (#218); the pair (2,3) can land in either order.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements