feat(editor): add 1/3T and 1/6T coarse triplet snap divisions - #70
Merged
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 (4)
📝 WalkthroughWalkthroughAdds triplet-based snap resolutions to the editor’s snap grid, updates the default snap index and dropdown options, and refreshes tests and changelog text to match the new labels and mappings. ChangesTriplet Snap Options
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)screen.jsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
screen.html (1)
93-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOption order/selection correctly mirrors
SNAP_OPTIONS.
1/4sits at index 3 withselected, matching the newsnapIdx: 3default, and the full ordering matchesscreen.js's array exactly.Since
editorSetSnap(this.selectedIndex)couples this static list toSNAP_OPTIONSby position, any future edit to one without the other silently desyncs the label shown from the value applied. Consider generating these<option>elements fromSNAP_OPTIONSat load time (e.g., populate the<select>in JS using the same array used forSNAP_VALUES) to remove this manual lockstep requirement, which the PR description itself flags as a maintenance burden.♻️ Sketch: derive options from SNAP_OPTIONS instead of hardcoding
- <select id="editor-snap" onchange="editorSetSnap(this.selectedIndex)" class="bg-dark-700 border border-gray-700 rounded px-1.5 py-0.5 text-xs text-gray-300 outline-none"> - <option>1/1</option> - <option>1/2</option> - <option>1/3T</option> - <option selected>1/4</option> - <option>1/6T</option> - <option>1/8</option> - <option>1/12T</option> - <option>1/16</option> - <option>1/24T</option> - <option>1/32</option> - <option>1/48T</option> - <option>1/64</option> - <option>1/96T</option> - </select> + <select id="editor-snap" onchange="editorSetSnap(this.selectedIndex)" class="bg-dark-700 border border-gray-700 rounded px-1.5 py-0.5 text-xs text-gray-300 outline-none"></select>// after SNAP_OPTIONS/S are defined const el = document.getElementById('editor-snap'); el.innerHTML = SNAP_OPTIONS.map((o, i) => `<option${i === S.snapIdx ? ' selected' : ''}>${o.label}</option>`).join('');🤖 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 `@screen.html` around lines 93 - 103, The snap dropdown in the static HTML is manually duplicating the same ordering as SNAP_OPTIONS, which can silently drift from the value applied by editorSetSnap(this.selectedIndex). Update the editor snap select so it is populated from the shared SNAP_OPTIONS data in screen.js rather than hardcoding the <option> list, and keep the default selection aligned with the current snapIdx logic. This should be done in the code that initializes the editor snap control and uses SNAP_VALUES/SNAP_OPTIONS so there is a single source of truth for both label and selection.
🤖 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 `@CHANGELOG.md`:
- Around line 10-19: The new changelog item is creating a second `### Added`
section inside the same Unreleased block, so update the `CHANGELOG.md` entry by
moving this bullet into the existing `### Added` section instead of introducing
a new heading. Keep the content of the new triplet snap note, but merge it with
the current Unreleased `### Added` list so there is only one `### Added` heading
per version block.
---
Nitpick comments:
In `@screen.html`:
- Around line 93-103: The snap dropdown in the static HTML is manually
duplicating the same ordering as SNAP_OPTIONS, which can silently drift from the
value applied by editorSetSnap(this.selectedIndex). Update the editor snap
select so it is populated from the shared SNAP_OPTIONS data in screen.js rather
than hardcoding the <option> list, and keep the default selection aligned with
the current snapIdx logic. This should be done in the code that initializes the
editor snap control and uses SNAP_VALUES/SNAP_OPTIONS so there is a single
source of truth for both label and selection.
🪄 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: fb5e6599-f3c5-46a3-91d7-d43e48e8904b
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/snap_options.test.js
Comment on lines
+10
to
+19
| ### Added | ||
| - **Coarse triplet snap divisions — `1/3T` and `1/6T`.** The snap grid now offers | ||
| quarter-note (`1/3T`) and eighth-note (`1/6T`) triplet resolutions alongside the | ||
| existing binary and fine-triplet options, so triplet passages can be snapped | ||
| without dropping all the way to `1/12T`. Triplet-family divisions are now labelled | ||
| with a `T` suffix (`1/3T`, `1/6T`, `1/12T`, `1/24T`, `1/48T`, `1/96T`) to set them | ||
| apart from the binary grid; the default snap stays `1/4`. (Ports the one division | ||
| set #62 had over the merged snap options, without adopting its `{step}` engine.) | ||
| Tests: `tests/snap_options.test.js`. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New entry creates a duplicate ### Added heading in the same Unreleased section.
This inserts a fresh ### Added block at the top of [Unreleased], but another ### Added section already exists further down in the same block (around line 64). Two separate ### Added headings under one version reduces changelog scannability; consider merging this bullet into the existing ### Added section instead.
🤖 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 `@CHANGELOG.md` around lines 10 - 19, The new changelog item is creating a
second `### Added` section inside the same Unreleased block, so update the
`CHANGELOG.md` entry by moving this bullet into the existing `### Added` section
instead of introducing a new heading. Keep the content of the new triplet snap
note, but merge it with the current Unreleased `### Added` list so there is only
one `### Added` heading per version block.
The snap grid jumped from 1/4 straight to 1/8 with no quarter- or eighth-note triplet resolution, so triplet passages could only snap at 1/12T or finer. Add 1/3T (value 1/3) and 1/6T (value 1/6) in value order, and label the triplet-family divisions (3/6/12/24/48/96) with a T suffix so the grid reads clearly against the binary options. The snap <select> options are static in screen.html and index-mapped via editorSetSnap(selectedIndex), so screen.html, SNAP_OPTIONS, and the snapIdx default are kept in lockstep; 1/4 shifts from index 2 to 3, so the S.snapIdx default and snap_options.test.js move with it. snapIdx is not persisted, so no migration is needed. Ports the one division set #62 (feat/snap-grid-on-profile-stack) had over the merged #57/#58 snap options, without adopting its {step} engine. Tests: snap_options.test.js updated (+1/3, +1/6 subdivisions, new label list, default index); full editor JS suite green (24 files). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
byrongamatos
force-pushed
the
feat/snap-coarse-triplets
branch
from
July 5, 2026 19:01
bbeb067 to
1683398
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds two coarse triplet snap resolutions to the arrangement editor's snap grid — 1/3T (quarter-note triplet) and 1/6T (eighth-note triplet) — placed in value order (between 1/2→1/4 and 1/4→1/8). Triplet-family divisions now carry a
Tsuffix (1/3T, 1/6T, 1/12T, 1/24T, 1/48T, 1/96T) so they read distinctly from the binary grid. Default snap stays 1/4.Why
The grid jumped from 1/4 straight to 1/8, so triplet passages could only be snapped at 1/12T or finer. These two coarse triplets are the one division set #62 (
feat/snap-grid-on-profile-stack) had over the merged #57/#58 snap options — ported onto the merged{value, subdivisions}model without adopting #62's{step}engine. (#62 was closed in favor of the merged options; this is the agreed follow-up.)Implementation notes
The snap
<select>options are static inscreen.htmland index-mapped viaeditorSetSnap(this.selectedIndex)— not JS-built — soscreen.html,SNAP_OPTIONS(screen.js), and thesnapIdxdefault move in lockstep. 1/4 shifts from index 2 → 3, so theS.snapIdxdefault (and thesnap_options.test.jsdefault-index assertion) move with it.snapIdxis not persisted, so no migration is required.Tests
tests/snap_options.test.jsupdated — default index, new label list, and added1/3→3/1/6→6subdivision coverage. Full editor JS suite green (24 files,node tests/*.test.js).🤖 Generated with Claude Code
https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
Summary by CodeRabbit
1/3Tand1/6T) and expanded the list ofT-suffixed divisions.1/4.T-suffixed labels.