feat(editor): track regions — import a part into an existing project as a placed, movable region - #344
Conversation
…ion foundation) PlaceRegionCmd drops freshly-added track content onto the timeline as a BOUNDED, selected, draggable block: it slides the content so its first onset lands on the snapped startBeat (a musical move — beats preserved on a varying grid, mirroring MoveRegionCmd's remap with the constant-tempo fast path) and writes an explicit-length region covering it. Even a bar-1 placement stays bounded — never the implicit default — so the block is a distinct addressable object from birth. DeleteRegionCmd removes exactly the notes/hits a region's end-exclusive window owns plus the region entry, leaving neighbours untouched. Both round-trip: rollback restores a verbatim content snapshot (the beatOf∘timeOf round trip is not bit-reversible), the track's raw `regions` key (including deleting one that was never there), and the prior selection. Node-runnable with no DOM, like the move command. _nextRegionIdPure allocates the next free region:N, counting the implicit default region:1 even when unpersisted so a placed region can never collide with it. This is the command layer for "Add Track from File → placed region" (TRACK-REGIONS-DESIGN.md PR 3); the import front door wires in next. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds region placement and deletion commands for notation and drum tracks, placement-mode handling for imported drums, selection updates, exact rollback behavior, Tracks-view keyboard deletion, and multi-drum targeting coverage. ChangesTrack Region Workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…t the active tab A song can hold several drum parts now (#339): each type:"drums" arrangement owns its .drumTab and S.drumTab is only the ACTIVE grid target. The region commands and the parts-view drag arming predate that (#334) — kind:'drums' always operated on S.drumTab, and a materialized extra part ('drums-2'…) armed as 'notation' over its empty shell notes[], so dragging its block moved nothing (or, for the primary while another part was active, moved the WRONG part's hits). Resolution now mirrors the lane silhouette: arrIdx >= 0 names the part whose track carries the region → its own .drumTab; arrIdx < 0 stays the legacy unmaterialized S.drumTab. MoveRegionCmd delegates to the shared _contentList, and parts-view arms kind:'drums' for any drum-arrangement row. Tests drive a NON-active part and pin that the active tab never moves, plus the legacy no-arrIdx fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/region_place_delete.test.mjs (1)
92-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Good coverage of placement, bounded-region-at-bar-1, zero-sustain-tail guard, varying-grid musical preservation, and drag interop with
MoveRegionCmd.Note: every scenario seeds the target list with only the to-be-placed content — there's no test where
_contentListalready contains previously-placed items (e.g., a second drum import onto an already-populatedS.drumTab.hits). That gap is tied to thePlaceRegionCmd.exec()full-list concern raised insrc/region-commands.js(Lines 235-267); once that's addressed, this scenario would be worth covering.🤖 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 `@tests/region_place_delete.test.mjs` around lines 92 - 172, Add coverage for placing content when the target list already contains previously placed items, especially a second drum placement using PlaceRegionCmd.exec() with pre-populated S.drumTab.hits. Verify existing items are preserved while new hits are shifted and the bounded region is created correctly, guarding the full-list handling concern in PlaceRegionCmd.
🤖 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/region-commands.js`:
- Around line 273-285: Make region creation atomic in the command method
containing the _findTrack(this.trackId) block: validate the track before
shifting content or mutating selection, and return without changes when it is
missing. When choosing this._placedId, detect collisions between an explicit
regionId and existing track region IDs and fall back to
_nextRegionIdPure(track.regions); ensure the selected ID always corresponds to
the newly inserted region.
- Around line 313-346: Add an early guard at the start of DeleteRegionCmd.exec()
that returns without modifying state when this.region is missing or lacks a
valid string id. Keep the existing deletion and selection behavior unchanged for
valid region objects, and avoid relying on the constructor’s region || {}
fallback for validation.
- Around line 235-267: Update PlaceRegionCmd.exec so it scopes placement to only
the newly imported items, rather than shifting every item returned by
_contentList. Track or identify the imported region’s items before calculating
minBeat, sustains, and remapping, while preserving existing hits or notation
content and using _regionContainsBeatPure or the established region-membership
mechanism where appropriate. Ensure bounds and sorting apply only to the new
placement.
---
Nitpick comments:
In `@tests/region_place_delete.test.mjs`:
- Around line 92-172: Add coverage for placing content when the target list
already contains previously placed items, especially a second drum placement
using PlaceRegionCmd.exec() with pre-populated S.drumTab.hits. Verify existing
items are preserved while new hits are shifted and the bounded region is created
correctly, guarding the full-list handling concern in PlaceRegionCmd.
🪄 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: 075f2f2c-3202-4e6c-80c2-f1dcb12d44d4
📒 Files selected for processing (3)
src/region-commands.jssrc/region.jstests/region_place_delete.test.mjs
… Track from File) The R3b payoff: importing a drum part into an existing project now surfaces it in the Tracks view as a SELECTED region, ready to drag. The Add-Drums dialog gains an inline "Place at" choice — Keep source timing (default: the file's own timing, no content motion, no persisted window), Bar 1, or Playhead — the latter two sliding the whole part there as one undoable PlaceRegionCmd (bar-snapped, beat-preserving). placeImportedPartAsRegion (track-session.js) is the orchestrator: it re-normalizes the session so the fresh part's row exists (the import just created the part — its row is synthesized here), resolves the track from the part's arr:<idx> target, then places or just selects. The import flow (arrangement.js) reaches it through the host table — track-session imports arrangement, so the call inverts through the usual seam. _placeAtStartBeatPure (region.js) resolves the dialog choice to a startBeat, converter-free like the other region pures. Create mode keeps replace semantics: the Place-at row hides, the choice is forced to 'keep', and with no materialized arrangement the hook no-ops gracefully. Tests drive the real orchestrator: row synthesis + placement + selection + undo restore, keep-writes-no-window, playhead snapping, and the pure's resolution table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
…dow, undoable) Del/Backspace in the Tracks view deletes the selected region as one DeleteRegionCmd: the notes/hits its end-exclusive window owns plus the window entry, neighbours untouched. parts-view owns the resolution (same kind/arrIdx logic as the drag arming, so a drum part's region always deletes ITS OWN hits, never the active grid's) and input.js's Delete ladder asks it first through the host table — transcription rows only; audio regions stay select-only until the audio-region PR. Falls through cleanly outside the Tracks view / with nothing selected. Tests drive the real handler: delete + undo round-trip, the gate table, and the non-active-drum-part case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Runtime verification caught the Del key doing nothing in the Tracks view: onKeyDown's partsViewMode gate returns for every key except the parts toggle, so the region-delete branch further down the Delete ladder was unreachable on the only surface it serves. The gate itself now offers Delete/Backspace to host.partsViewRegionDelete() before ignoring the key — a region op on this surface, not a note-edit on the hidden chart — and the dead ladder branch is folded into it. Verified end-to-end via Playwright against the real host (import a drums MIDI → Place at Bar 1 → region:2 lands selected → drag commits "Moved region:2 later" → Delete commits "Deleted region region:2 and its notes" → undo restores). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
|
Runtime-verified end-to-end against the real host (isolated testbed on :8002, headless Playwright, AC/DC — Back In Black + a synthesized channel-10 drums MIDI):
The verify caught one real wiring bug the node suite couldn't see: 🤖 Generated with Claude Code |
Keep-both CHANGELOG; comment reconciliation with #343 (create-mode N-drums): create mode is no longer replace-semantics, so the Place-at gating comments now state the real rationale — placement there is deferred until the create build path proves it round-trips regions[]; the import still lands selected at source timing. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/parts-view.js (1)
466-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect, well-scoped delete handler — but the kind-resolution check is now duplicated 3x in this file.
The
(arrIdx >= 0 && isDrumArrangement(S.arrangements[arrIdx])) || <targetId> === 'drums'pattern now appears here (480-481), in the drag-arming code (422-423), and in the pre-existing click handler (~391). Worth extracting once to keep the three call sites from drifting.♻️ Proposed refactor
+function _isDrumRow(arrIdx, targetId) { + return (arrIdx >= 0 && isDrumArrangement(S.arrangements[arrIdx])) || targetId === 'drums'; +} + export function _partsViewRegionDelete() { if (!S.partsViewMode || !S.selectedTrackId || !S.selectedRegionId || !S.history) return false; const row = _unifiedRows().find(r => r.id === S.selectedTrackId); if (!row || row.type !== 'transcription') return false; const region = _trackRegionsResolvePure(row.regions).find(r => r.id === S.selectedRegionId); if (!region) return false; const arrIdx = _arrIndexForTarget(row.targetId); - const kind = (arrIdx >= 0 && isDrumArrangement(S.arrangements[arrIdx])) || row.targetId === 'drums' - ? 'drums' : 'notation'; + const kind = _isDrumRow(arrIdx, row.targetId) ? 'drums' : 'notation'; S.history.exec(new DeleteRegionCmd({ kind, arrIdx, trackId: row.id, region })); ...🤖 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/parts-view.js` around lines 466 - 487, Extract the repeated arrangement-kind resolution logic into a shared helper in parts-view.js, then reuse it in _partsViewRegionDelete, the drag-arming path, and the existing click handler. Preserve the current drums-versus-notation behavior, including the row targetId fallback when arrIdx is invalid.
🤖 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/track-session.js`:
- Around line 787-793: Update placeImportedPartAsRegion to allow arrIdx === -1
for the legacy drums target, while continuing to reject other invalid indices.
For negative indices, resolve the target by mixKey === 'drums'; retain the
existing arr: lookup for nonnegative indices, and add coverage for the
drums-only import path.
---
Nitpick comments:
In `@src/parts-view.js`:
- Around line 466-487: Extract the repeated arrangement-kind resolution logic
into a shared helper in parts-view.js, then reuse it in _partsViewRegionDelete,
the drag-arming path, and the existing click handler. Preserve the current
drums-versus-notation behavior, including the row targetId fallback when arrIdx
is invalid.
🪄 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: 5b4bbdbf-fc4c-4370-8808-7c6b2b2b3138
📒 Files selected for processing (11)
CHANGELOG.mdscreen.htmlsrc/arrangement.jssrc/host.jssrc/input.jssrc/main.jssrc/parts-view.jssrc/region-commands.jssrc/region.jssrc/track-session.jstests/region_place_delete.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/region-commands.js
1 participant
What
The payoff slice of the track-regions arc (
TRACK-REGIONS-DESIGN.mdPR 3, "THE DRIVER"): importing a drum part into an existing project now lands it in the Tracks view as a selected, movable region — with a "Place at" choice (Keep source timing / Bar 1 / Playhead) — instead of only merging invisibly into the model. Plus the delete surface and a multi-drum correctness fix the flow depends on.Builds directly on the merged stack: #332 (region model) → #333 (strip + selection) → #334 (drag-move) → #339 (N drum parts).
The commits (review bottom-up)
57e5c38— command layer.PlaceRegionCmd+DeleteRegionCmd(src/region-commands.js),_nextRegionIdPure(src/region.js).startBeat— a musical move (beats preserved on a varying grid; constant-tempo fast path bit-exact, mirroringMoveRegionCmd) — and writes a bounded region covering it (explicitlenBeateven at bar 1, never the implicit default). Lands selected.regionskey (including deleting a key that was never there), and selection.7f42183— multi-drum seam fix. A song holds several drum parts now (feat(editor): a song can hold several drum parts #339), but the region commands and the drag arming predated that:kind:'drums'always acted on the activeS.drumTab, and an extra part's row (drums-2…) armed as'notation'over its empty shellnotes[]— dragging its block moved nothing (or the wrong part's hits). Resolution now mirrors the lane silhouette:arrIdxnames the part → its own.drumTab;arrIdx < 0= the legacy unmaterialized tab.6891d0a— the import front door. The Add-Drums dialog gains "Place at" — Keep source timing (default), Bar 1, Playhead — andeditorDoAddDrumslands the fresh part through a newplaceImportedPartAsRegionorchestrator (src/track-session.js, reached via the host table since track-session imports arrangement): re-normalize so the fresh part's row exists → resolve its track → place (undoable) or select. Create mode keeps replace semantics (row hidden, choice forced to keep, hook no-ops).3e1960c— Delete key. Del/Backspace in the Tracks view deletes the selected region block (content + window, one undoable step); same resolution as the arming; audio regions stay select-only until the audio-region PR.One deliberate divergence from the design doc
The design sketched "placed at bar 1 (default)". This modal also serves full-song drum charts whose source timing is already correct — defaulting to bar 1 would yank any chart with intro silence to the start. So the shipped default is Keep source timing (today's behaviour, zero surprise; the part still lands selected + draggable via its default region), with Bar 1 / Playhead one click away. Easy to flip if you want the design's letter.
Tests
tests/region_place_delete.test.mjs— 23 suites, all failing onmain:regions[]and selectionarrIdxfallback pinnednpm test: 304/306 — the 2 failures (mixer_meter_teardown,song_fit) pre-exist on main (verified by stashing and re-running).npm run lint: 0 errors (3 pre-existing warnings in untouched files).🤖 Generated with Claude Code
https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Summary by CodeRabbit