refactor(editor): move the loop region and scroll viewport to src/loop.js (R2, step 28)

[byrongamatos]

Twenty-ninth module. src/main.js 7,714 → 7,183 — 66% below where it started.

528 lines: the A/B loop strip and its drag / nudge / keyboard handling, bar-range selection, the scroll-bounds math, and snapTime — the one place a raw time becomes a snapped one (grid, or the nearest audio onset). Every module that places something on the timeline snaps through host.snapTime, which resolves here.

Wiring

Three main.js symbols arrive as host hooks: the seek, the snap-step query, and the Loop-in-3D button refresh. The loop-region + scroll functions and snapTime were already host hooks pointing at main.js; they resolve to the loop.js exports now.

loop.js imports the A/B functions from audio.js; audio.js reaches loop’s _selectedLoopRegion / _setLoopRegionEnabled through host, not import — so no cycle. Verified: 29 modules, NO_CYCLES.

The recurring hook trap, watched for

updateLoopIn3DBtn was already a host hook from a prior step. My scripted add made a second one — caught it, removed the duplicate in both host.js and the setHostHooks call.

The three genuinely-new hooks landed correctly this time — but I had Codex confirm all three are present and point at real functions before trusting it, because the same scripted-edit-anchor-miss silently no-op’d in #176 and #179. It has bitten twice; it now gets an explicit check every time.

Also drops a dead _guideTimerSync import left in main.js when #179’s _abDisarm consolidation removed its last caller.

Tests

9 loop/onset/group suites retarget their source read to src/loop.js and strip the export keyword before eval. loop_ab / loop_nudge_live gained host stubs for the new hooks; loop_undo_mode reads both loop.js and main.js (its @pure:pending-view block stayed behind). Several CJS → .mjs.

Harness

verify_loop.py drives editorSetLoopSnapMode end to end (persists the pref + reports through setStatus) and asserts the window.* re-attach. Comment out the re-attach and it throws where 90 unit tests pass. snapTime is covered by verify_drum (a drum hit snaps through the same hook).

Verification

node --test 90/90 · pytest 248/248 · npm run lint 0 errors (6 warnings) · Codex clean · all 21 headless harnesses pass.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added end-to-end loop-strip editing with bar, grid, and free snapping modes, including drag, nudge, and keyboard edge adjustment.
  • Added optional onset-based snapping for loop timing (with grid subdivision fallback).
  • Added loop controls to toggle loop region and set the preferred snap mode, plus editor seek/snap-step hooks for external control.
• Bug Fixes
  • Improved loop movement math, including accurate scroll bounds clamping and more consistent group spacing during whole-selection drags.
• Tests
  • Updated loop-related tests to use the new loop implementation source.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b1cb5cea-49a0-4109-a35c-522655ba8f13

📥 Commits

Reviewing files that changed from the base of the PR and between 4f1b329 and 1c6a21e.

📒 Files selected for processing (4)

• tests/loop_ab.test.js
• tests/loop_region.test.mjs
• tests/loop_snap_modes.test.mjs
• tests/onset_snap.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/loop_snap_modes.test.mjs

---

📝 Walkthrough

Walkthrough

Loop-region geometry, snapping, strip interaction, playback controls, and viewport helpers move into src/loop.js. Host seek/snap hooks and loop globals are wired through src/main.js, while loop-focused tests are retargeted to the new module.

Changes

Loop editor behavior

Layer / File(s)	Summary
Loop geometry and snapping src/loop.js	Adds viewport bounds, bar/grid/free loop-region calculations, onset/grid snapping, and group-drag delta handling.
Loop strip interaction and state src/loop.js	Adds strip rendering, mode persistence, keyboard and mouse interaction, beat synchronization, and loop enablement controls.
Host and module wiring src/host.js, src/main.js, CHANGELOG.md	Adds seek and snap-step host hooks, exposes loop controls globally, removes relocated helpers, and updates the changelog.
Loop behavior test extraction tests/*loop*, tests/group_move_snap.test.js, tests/onset_snap.test.js	Retargets source extraction to src/loop.js and adapts exported-function and host-stub handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Editor
  participant LoopStrip
  participant LoopModule
  participant Host
  Editor->>LoopStrip: interact with loop strip
  LoopStrip->>LoopModule: create or adjust loop region
  LoopModule->>Host: read snap step or seek editor
  Host-->>LoopModule: provide hook result
  LoopModule-->>LoopStrip: render updated region and controls

Loading

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#92: Implements related loop snap-mode and region-selection logic.
• got-feedBack/feedBack-plugin-editor#137: Changes the shared loop beat-coordinate synchronization machinery.
• got-feedBack/feedBack-plugin-editor#168: Modifies the shared host-hook wiring used by this change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.21% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main refactor: moving loop-region and scroll viewport logic into src/loop.js.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/r2-step28-next

---

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

[Copilot]

Pull request overview

Refactors the editor by extracting the loop-region / bar selection / loop strip UI handling, scroll viewport bounds math, and snapTime into a new src/loop.js module, reducing src/main.js size and keeping the module graph acyclic via host hooks.

Changes:

• Adds src/loop.js and rewires src/main.js to import/export loop-region + viewport functionality from it (including snapTime), exposing required wiring through host hooks.
• Extends src/host.js with new default hooks (editorSeekToTime, editorSnapStepSeconds) used by loop.js.
• Retargets loop/onset-related tests to slice pures from src/loop.js and updates eval-stripping to handle export, plus updates the changelog entry.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/onset_snap.test.js	Retargets snapTime extraction source from main.js to loop.js.
tests/loop_undo_mode.test.mjs	Splits sourcing between loop.js (loop helpers) and main.js (@pure:pending-view).
tests/loop_snap_modes.test.mjs	Retargets @pure:loop-region slice to loop.js and strips export before eval.
tests/loop_region.test.mjs	Retargets @pure:loop-region slice to loop.js and strips export before eval.
tests/loop_nudge.test.mjs	Retargets loop nudge-related slices to loop.js and strips export before eval.
tests/loop_nudge_live.test.mjs	Updates the injected wiring to match new host-based dependencies and export-tolerant extraction.
tests/loop_beats.test.mjs	Retargets loop-beats helper extraction from main.js to loop.js.
tests/loop_ab.test.js	Updates _setLoopRegionEnabled sourcing and extraction to come from loop.js.
tests/group_move_snap.test.js	Retargets _groupTimeDeltaPure slice from main.js to loop.js and strips export.
src/main.js	Removes inlined loop/viewport/snapTime code and wires new host hooks + window handlers to loop.js exports.
src/loop.js	New module containing loop-region UI logic, scroll bounds helpers, and snapTime (including onset snapping).
src/host.js	Adds default no-op host hooks for seek + snap-step queried by loop.js.
CHANGELOG.md	Documents the refactor/module move and the main.js size reduction.

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/loop.js (1)

168-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Viewport-duration formula duplicated instead of reusing _editorViewportDuration().

((canvas.width / DPR) - LABEL_W) / S.zoom is recomputed inline in _loopStripTimeFromClientX (Line 172) and again in _renderLoopStrip (Line 222), duplicating the same math that _editorViewportDuration() (Lines 35-38) already encapsulates. If the geometry formula ever changes, these two call sites will silently drift out of sync with the canonical implementation.

♻️ Proposed consolidation

 function _loopStripTimeFromClientX(clientX) {
     const b = _loopStripTrackBounds();
     if (!b || !canvas) return 0;
     const ratio = Math.max(0, Math.min(1, (clientX - b.left) / b.width));
-    const viewDur = Math.max(0, ((canvas.width / DPR) - LABEL_W) / S.zoom);
+    const viewDur = Math.max(0, _editorViewportDuration());
     return S.scrollX + ratio * viewDur;
 }

-    const viewDur = Math.max(0, ((canvas.width / DPR) - LABEL_W) / S.zoom);
+    const viewDur = Math.max(0, _editorViewportDuration());
     const left = ((S.barSel.startTime - S.scrollX) / Math.max(0.0001, viewDur)) * 100;

Also applies to: 196-238

🤖 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/loop.js` around lines 168 - 174, Replace the duplicated viewport-duration
calculations in _loopStripTimeFromClientX and _renderLoopStrip with calls to the
canonical _editorViewportDuration() helper, preserving the existing clamping and
behavior while ensuring both loop-strip paths use the shared geometry formula.

🤖 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 `@tests/loop_region.test.mjs`:
- Line 34: Update the failure diagnostic near the `@pure`:loop-region block check
to reference src/loop.js instead of the stale src/main.js path, matching the
source file read by the test harness.

In `@tests/loop_snap_modes.test.mjs`:
- Line 47: The extraction failure diagnostic still references the outdated
src/main.js path; update the console.error message in the test harness to report
src/loop.js, matching the module actually read by the test.

---

Nitpick comments:
In `@src/loop.js`:
- Around line 168-174: Replace the duplicated viewport-duration calculations in
_loopStripTimeFromClientX and _renderLoopStrip with calls to the canonical
_editorViewportDuration() helper, preserving the existing clamping and behavior
while ensuring both loop-strip paths use the shared geometry formula.

🪄 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: 50fdb966-2b60-4098-8e4d-695bbd7ab80a

📥 Commits

Reviewing files that changed from the base of the PR and between f94de68 and 4f1b329.

📒 Files selected for processing (13)

• CHANGELOG.md
• src/host.js
• src/loop.js
• src/main.js
• tests/group_move_snap.test.js
• tests/loop_ab.test.js
• tests/loop_beats.test.mjs
• tests/loop_nudge.test.mjs
• tests/loop_nudge_live.test.mjs
• tests/loop_region.test.mjs
• tests/loop_snap_modes.test.mjs
• tests/loop_undo_mode.test.mjs
• tests/onset_snap.test.js

[byrongamatos]

All six fixed — same class: after retargeting the source reads to src/loop.js (and src/audio.js for the @pure:loop-ab / onset-snap blocks), the comments and block-missing failure messages still named src/main.js. Each now points at the file the sliced symbol actually lives in, verified against its location.