feat(editor): tempo-zone confirm bar + bounded per-zone refine (P2-3 completion) - #259
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds an interactive tempo-zone proposal workflow with boundary and tempo editing, bounded per-zone grid refinement, a single-tempo fallback, confirm-bar controls, and Tempo Map integration. Tests cover segment operations, refinement behavior, overlay structure, and stretch clamping. ChangesTempo-zone confirmation workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Tempo Map editor
participant Zones as tempo-zones.js
participant Tempo as tempo.js
participant Grid as TempoGridCmd
Editor->>Zones: adjust tempo-zone proposal
Zones->>Tempo: confirm refined zones
Tempo->>Grid: commit one refined grid
Grid-->>Editor: redraw canvas and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tempo-segment.js (1)
408-429: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMerging an unmapped zone with a mapped one silently produces a mapped result.
_segmentMergePureonly keepskind: 'unmapped'when both neighbours are unmapped (Line 419-420); merging an unmapped zone with a mapped one always yields'ramp'or'constant', using the unmapped side'sbpmStart/bpmEndin the endpoint math. Since an unmapped zone is user intent for "no barlines here," a merge silently overrides that intent and seeds a grid over what was explicitly marked unmapped. This is called out as deliberate in the comment above (Line 408-411), but the behavior isn't covered bytests/tempo_zones_confirm.test.mjs's merge test (which only merges two mapped zones).Worth confirming this is the desired UX (vs., e.g., keeping
'unmapped'when either side is unmapped, or requiring an explicit re-map first), and adding a test case for the mixed unmapped/mapped merge either 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 `@src/tempo-segment.js` around lines 408 - 429, Update _segmentMergePure so a merge involving either an unmapped or mapped segment preserves the unmapped intent according to the desired UX; do not derive a mapped ramp or constant from an unmapped endpoint. Add a test covering both mixed merge orders in the existing tempo-zone merge tests, and ensure the two-unmapped behavior remains unchanged.src/mouse.js (1)
351-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a hover-cursor cue for the zone-boundary handle.
segment-boundarygets full drag support here, but the unchanged tempo-map hover-cursor block (around Line 290-295) only checks_tempoSyncAtXfor theew-resizecursor — hovering a zone-boundary handle gives no visual affordance, unlike sync-point poles.♻️ Proposed addition to the tempo-map hover branch (outside this hunk, ~line 290-295)
if (S.tempoMapMode) { + if (_zonesStripHit(y)) { + if (canvas) canvas.style.cursor = _zonesBoundaryHitAt(x) >= 0 ? 'ew-resize' : 'pointer'; + return; + } const hit = _tempoSyncAtX(x, y); if (hit !== S.tempoHover) { S.tempoHover = hit; host.draw(); } if (canvas) canvas.style.cursor = hit >= 0 ? 'ew-resize' : ''; return; }🤖 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/mouse.js` around lines 351 - 357, Update the tempo-map hover-cursor logic near the existing _tempoSyncAtX check to also detect segment-boundary handles and apply the ew-resize cursor. Keep the current tempo-sync hover behavior unchanged and ensure the cue is shown before drag state begins.
🤖 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 `@src/tempo.js`:
- Around line 2468-2479: Update _tempoMapOnDragMove so segment-boundary
interactions use the same ≥3px movement threshold as tempo-sync before setting
dg.moved or calling _zonesDragBoundary. Preserve the existing no-op behavior for
sub-threshold pointer jitter and only reshape the proposal after the drag
threshold is exceeded.
---
Nitpick comments:
In `@src/mouse.js`:
- Around line 351-357: Update the tempo-map hover-cursor logic near the existing
_tempoSyncAtX check to also detect segment-boundary handles and apply the
ew-resize cursor. Keep the current tempo-sync hover behavior unchanged and
ensure the cue is shown before drag state begins.
In `@src/tempo-segment.js`:
- Around line 408-429: Update _segmentMergePure so a merge involving either an
unmapped or mapped segment preserves the unmapped intent according to the
desired UX; do not derive a mapped ramp or constant from an unmapped endpoint.
Add a test covering both mixed merge orders in the existing tempo-zone merge
tests, and ensure the two-unmapped behavior remains unchanged.
🪄 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: 3037762a-3cf2-4b37-b77c-ac7fa37779a9
📒 Files selected for processing (12)
CHANGELOG.mdscreen.htmlsrc/input.jssrc/main.jssrc/menu-bar.jssrc/mouse.jssrc/tempo-segment.jssrc/tempo-suggest.jssrc/tempo-zones.jssrc/tempo.jstests/screen_markup.test.mjstests/tempo_zones_confirm.test.mjs
435730b to
da07925
Compare
|
Applied — the 3px click-vs-drag threshold now guards Also landed from my own pass (da07925):
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
P2-3 completion: Scan paints adjustable zone bands (drag boundary / split / merge / kind / BPM, all pre-commit, Esc-cleared) with a docked confirm bar; Confirm & refine seeds the grid then runs _suggestFitPure bounded per zone with the new opts.stretchClamp as the segment-tempo prior — ONE TempoGridCmd. Single-tempo escape hatch included. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Review findings on the tempo-zone confirm bar:
1. _segmentBoundaryDragPure clamped into a possibly-EMPTY window. The
feasible join is [a.tStart + minLen, b.tEnd - minLen]; when the PAIR
is shorter than 2x minLen that range is empty, the Math.max wins, and
the function hands back a right segment BELOW minLen — silently
breaking the one invariant its contract states ("both neighbours keep
at least minLen"). The old guard only checked containment in the pair,
which an empty-window result still satisfies. Refuse instead.
Not reachable from Scan today (the detector floors zones at
minSegSec = 6, and split/merge both preserve >= minLen), but this is
an exported pure and the guard is one line.
2. editorZonesSingleTempo blamed the zones ("No mapped zone to take a
tempo from") for what is actually a missing onset analysis. Diagnose
the two apart, matching editorConfirmTempoZones.
3. _segmentRefineGridPure's doc claimed `refined` counts onset-corroborated
downbeats. It counts every non-locked proposal — an interior held-note
bar marches on prediction and still counts. Say what it counts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit: segment-boundary reshaped the proposal on the FIRST mousemove while tempo-sync and tempo-group both require >=3px first. Valid, and it bites harder than it looks: the handle is narrow (+/-6px) and the reshape is pre-commit, so pointer jitter on an intended click silently moved the boundary with no undo to recover it — Ctrl+Z only reaches committed grids. All three drag kinds now share the threshold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
46f5d73 to
b4c4dc0
Compare
What
The remaining half of segment-first tempo mapping (#252/#253 shipped the engine + rough Apply; their follow-up notes both point here):
Tempo/Grid ▸ Scan for tempo zones…enters Tempo Map, paints the detected zones as colored bands (teal steady / violet ramp / grey unmapped, BPM labels, "?" on low-confidence zones) on the timeline, and docks a confirm bar.src/tempo-segment.js) and clamped: drag a boundary handle (both neighbours keep a minimum span; stale phase seeds drop for re-seed), Split at the playhead (a ramp interpolates its tempo at the cut), Merge → (the join is described by its own endpoints — a real rit never flattens into a lie), cycle Kind (steady/ramp/unmapped), type BPM (degenerate values refused — a 0 BPM would hang the seeder). Proposal state is module-scoped, editGen- AND session-keyed, Esc-cleared, dismissed on mode exit, mutually exclusive with Suggest ghosts._suggestFitPurebounded inside each zone (opts.toIdx) with the newopts.stretchClampholding the drift tracker to ±12% of the zone tempo — the segment BPM becomes a real prior, the structural cure for the runaway march. Lands as ONE undoable TempoGridCmd (notes keep their seconds and ride).src/tempo-zones.js(state + chrome; apply verbs stay in tempo.js and arrive as init hooks, so no import cycle).editorApplyTempoZones(the feat(editor): apply a segment-first rough map (P2-3 Apply) #253 menu row) is unchanged as the quick path.Tests
tests/tempo_zones_confirm.test.mjs(9): every adjust verb pinned (clamps, ramp interpolation at a split, honest-endpoint merge, kind round-trip, BPM validation, weighted-median collapse); stretchClamp pinned on a miss-path march (a locked bar protects the miss from the trailing drop, so the clamped vs unclamped predictions are directly observable: 4.14 vs 4.20); bounded refine pinned on a two-zone fixture (anchors hold, each zone snaps to its own pulse, unmapped refines nothing). Behavioral cases fail on main.screen_markup.test.mjsextended to pin the new bar's nesting.Full suite green, lint 0 errors (3 pre-existing ratchet warnings),
routes.pyuntouched.Live-verified
On a tester's real project (Insomnium — Where the Last Wave Broke, 584 beats / 1114 notes, stems-only audio): Scan → 5 zones detected + flagged LOW CONFIDENCE (double-kick metal — exactly the honesty the "?" marks are for), bands + bar render, split→merge round-trips, Confirm & refine: "Built the grid from 5 tempo zones and snapped 76 barlines to the recording" as exactly one undo step, Ctrl+Z restores the old grid bit-exact, zero page errors.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit