fix(editor): resnap works with Snap off and snaps BOTH edges to the subdivision guidelines

[ChrisBeWithYou]

What

From a tester report this morning ("I miss a way to snap the selected notes to the grid. Like after setting the tempo map and importing from gp-file"): the feature exists (Edit ▸ Resnap selection / its shortcut) — but snapTime honoured the live Snap toggle, so with Snap off the explicit quantize silently moved nothing, and it printed no status either way. A silent no-op is indistinguishable from a missing feature.

• snapTime gains an opt-in force arg: explicit quantize verbs bypass the ON/OFF toggle while the selected snap subdivision (and Onset mode) still applies — the toggle governs live interactive placement only. Default unchanged, so every interactive caller keeps honouring the toggle bit-exactly.
• Both edges snap to the guidelines (the piano-roll grid model — the design call from this session): note starts quantise to the nearest guideline at the current subdivision, and a sustained note's end edge follows via the existing ResizeSustainGroupCmd. A sustained note never collapses onto its start (new snapGuidelineAfter supplies the one-subdivision minimum), and a zero-length chip is never inflated — length is authored intent.
• One undoable step (a small composite over the move + resize halves, rollback in reverse), and the status line always reports: "Snapped 842 of 1114 notes to the grid (both edges)" or "Selection already on the grid."

Tests

tests/resnap_explicit.test.mjs (8): the toggle-honouring identity is pinned unchanged; force lands on the grid with Snap off (fails on main — the arg is ignored there); force/no-force agree with Snap on; Onset-mode fallback; snapGuidelineAfter strictly-after semantics; both-edges quantise; the collapse guard; the chip guard. Full suite 147 green, lint 0 errors, routes.py untouched.

Live-verified

Real library pak, Snap toggled OFF, all 1,114 notes selected, resnap fired by its shortcut: 842 notes touched (698 starts moved, 581 end edges re-lengthed), zero chips inflated, exactly one undo entry, Ctrl+Z restores every time and sustain bit-exact. Zero page errors.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q

Summary by CodeRabbit

• New Features

  • “Resnap selection to grid” now explicitly quantizes both note starts and ends, even when Snap is turned off.
  • Sustained notes retain valid durations, while zero-length notes remain unchanged.
  • Resnap actions report their results and can be undone as a single operation.

• Bug Fixes

  • Corrected resnap behavior for short, sustained, and already-aligned notes.
  • Read-only editing now displays an appropriate lock notice.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b60b1e0-7813-47e5-9f2c-80a69dfab3b2

📥 Commits

Reviewing files that changed from the base of the PR and between b140a97 and 2f3b2db.

📒 Files selected for processing (4)

• CHANGELOG.md
• src/input.js
• src/loop.js
• tests/resnap_explicit.test.mjs

---

📝 Walkthrough

Walkthrough

Resnap selection now explicitly quantizes selected note starts and sustained ends to grid guidelines, including when Snap is off. The operation preserves short and zero-length notes, applies one undoable compound change, reports its result, and adds deterministic tests for the updated snapping behavior.

Changes

Resnap selection

Layer / File(s)	Summary
Forced snapping primitives src/loop.js	snapTime accepts forced quantization, and snapGuidelineAfter returns the next applicable grid guideline.
Two-edge resnap flow src/input.js, CHANGELOG.md	Resnap calculates both edges, preserves valid sustain lengths, applies movement and resizing through one undoable command, reports status, and documents the behavior.
Resnap behavior validation tests/resnap_explicit.test.mjs	Tests cover forced snapping, guideline selection, sustained and zero-length notes, and unavailable-grid handling.

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

Sequence Diagram(s)

sequenceDiagram
  participant Editor as _editorResnapSelection
  participant Edges as _resnapEdgesPure
  participant Grid as snapTime and snapGuidelineAfter
  participant History as Compound undo command
  Editor->>Edges: resnap selected note edges
  Edges->>Grid: force-quantize starts and sustained ends
  Grid-->>Edges: snapped times and sustain lengths
  Edges-->>Editor: changed note values
  Editor->>History: apply moves and conditional sustain resizes

Loading

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#144: Modifies the shared snapTime path for detected-onset snapping.
• got-feedBack/feedBack-plugin-editor#197: Modifies snapTime quantization for swing handling.
• got-feedBack/feedBack-plugin-editor#236: Modifies the onset-detection source used by snapTime.

Suggested reviewers: byrongamatos

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: explicit resnap now works with Snap off and snaps both note edges to grid guidelines.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 fix/editor-resnap-explicit

---

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

[coderabbitai]

🧹 Nitpick comments (2)

src/loop.js (1)

420-426: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct, well-tested; minor duplication with snapTime.

snapGuidelineAfter re-derives sv/subs the same way snapTime does (lines 402/409). Not a bug, but if the two ever need different subdivision rules this duplication could quietly drift.

♻️ Optional: share the sv/subs derivation

+function _effectiveSubs(force) {
+    const sv = _editorEffectiveSnapValuePure(S.snapEnabled || force, SNAP_VALUES[S.snapIdx]);
+    return sv ? _editorSnapSubdivisionsPure(sv) : 0;
+}
+
 export function snapGuidelineAfter(t, force = false) {
-    const sv = _editorEffectiveSnapValuePure(S.snapEnabled || force, SNAP_VALUES[S.snapIdx]);
-    if (!sv || S.beats.length < 2) return t;
-    const subs = _editorSnapSubdivisionsPure(sv);
+    const subs = _effectiveSubs(force);
+    if (!subs || S.beats.length < 2) return t;
     const q = Math.floor(beatOf(S.beats, t) * subs + 1e-6) + 1;
     return timeOf(S.beats, q / subs);
 }

🤖 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 420 - 426, Optionally extract the shared snap-value
and subdivision derivation from snapTime and snapGuidelineAfter into a reusable
helper, then use it in both functions. Preserve each function’s existing
snapping behavior and early-return conditions.

src/input.js (1)

503-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid wiring; matches the tested _resnapEdgesPure contract and PR's undo-grouping objective.

One gap: _editorResnapSelection itself (single-undo-entry grouping, the "already on the grid" branch, touched-count message) is only covered by manual/live verification per the PR description — the automated suite only exercises the pure helpers it calls. Given this function is the one place the "one undoable step" guarantee is assembled, a small integration test (export it, stub S.history/notes()/host) would catch a future regression that the pure-function tests can't see.

🤖 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/input.js` around lines 503 - 543, Add an integration test for
_editorResnapSelection that stubs notes(), host, and S.history, then verifies
the already-on-grid path and that changed selections execute exactly one grouped
undoable command with the expected status/update behavior. Export
_editorResnapSelection as needed for test access, while preserving the existing
pure-helper tests and production behavior.

🤖 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/input.js`:
- Around line 503-543: Add an integration test for _editorResnapSelection that
stubs notes(), host, and S.history, then verifies the already-on-grid path and
that changed selections execute exactly one grouped undoable command with the
expected status/update behavior. Export _editorResnapSelection as needed for
test access, while preserving the existing pure-helper tests and production
behavior.

In `@src/loop.js`:
- Around line 420-426: Optionally extract the shared snap-value and subdivision
derivation from snapTime and snapGuidelineAfter into a reusable helper, then use
it in both functions. Preserve each function’s existing snapping behavior and
early-return conditions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 967063ff-8d2e-4ed0-829b-65042945b5d2

📥 Commits

Reviewing files that changed from the base of the PR and between a866705 and b140a97.

📒 Files selected for processing (4)

• CHANGELOG.md
• src/input.js
• src/loop.js
• tests/resnap_explicit.test.mjs