feat(editor): view dropdown — Tab, Notation, and drum-track parity - #316
Conversation
|
Warning Review limit reached
Next review available in: 31 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 (9)
📝 WalkthroughWalkthroughThe PR replaces the per-track String/Piano controls with a view dropdown, adds shared drum alphaTex generation, and extends the live Tab lens to render and select drum notation while preserving drum-editor state. ChangesTrack view lens and drum notation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ViewDropdown
participant ViewModeRouter
participant TabLens
participant AlphaTex
participant Renderer
User->>ViewDropdown: Choose a track view
ViewDropdown->>ViewModeRouter: Pass selected mode
ViewModeRouter->>TabLens: Enable or disable chart/drum lens
TabLens->>AlphaTex: Generate notes or drum-hit engraving
AlphaTex->>Renderer: Provide alphaTex and staff configuration
Renderer-->>User: Display live notation or Tab
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 |
|
Reworked per Christian's design call: the switcher is now a dropdown, not a pill toggle — String view · Piano roll · Tab · Notation · Notation + Tab (every staff profile an explicit option, so the selected value always reads back truthfully and no pref-preservation special-casing is needed). Re-verified in the host with Playwright: five options, Tab renders the engraving, Notation + Tab persists the 'both' pref, String exits the lens. |
ChrisBeWithYou
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/view_switch_lens.test.mjs (1)
58-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover selection preservation when changing staff profiles.
Seed
S.selbefore switching from Tab to Notation/Both and assert it remains unchanged. The current assertions miss the redundant lens re-entry regression.🤖 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/view_switch_lens.test.mjs` around lines 58 - 67, Update the test "'notation' and 'both' set the matching staff profile" to seed S.sel while in Tab mode before switching profiles, then assert the selection is unchanged after switching to both Notation and Both. Preserve the existing tabViewMode, tabViewStaff, and dropdown-value assertions while covering redundant lens re-entry.
🤖 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`:
- Line 42: Restore the changelog bullet boundary before the “In Tempo Map mode”
text so it becomes a separate entry rather than continuing the drum-notation
entry. Update the affected changelog text only, preserving both entries’
wording.
In `@src/key-view.js`:
- Around line 148-151: Remove the redundant editorToggleTabView(true) call from
the mode-switch branch in src/key-view.js lines 148-151, relying on
editorSetTabViewStaff() to enter the lens without clearing S.sel. Update
tests/view_switch_lens.test.mjs lines 58-67 to seed S.sel and assert that
switching from Tab to Notation and Both preserves the selection.
---
Nitpick comments:
In `@tests/view_switch_lens.test.mjs`:
- Around line 58-67: Update the test "'notation' and 'both' set the matching
staff profile" to seed S.sel while in Tab mode before switching profiles, then
assert the selection is unchanged after switching to both Notation and Both.
Preserve the existing tabViewMode, tabViewStaff, and dropdown-value assertions
while covering redundant lens re-entry.
🪄 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: 7f66cafb-aa2a-46f6-adfc-98ba384604e5
📒 Files selected for processing (8)
CHANGELOG.mddocs/USER-GUIDE.mdscreen.htmlsrc/alphatex.jssrc/key-view.jssrc/tab-view-live.jstests/alphatex.test.mjstests/view_switch_lens.test.mjs
| with no notation symbol are skipped and counted, never silently | ||
| dropped). Clicking an engraved beat selects its hits in the drum grid | ||
| and seeks. The drum-editor mode stays on underneath the lens, so | ||
| switching back to Drum grid restores it with all its state. In Tempo Map |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the Tempo Map bullet boundary.
The unrelated In Tempo Map mode... text currently continues the drum-notation entry.
Proposed fix
- switching back to Drum grid restores it with all its state. In Tempo Map
+ switching back to Drum grid restores it with all its state.
+- **Tempo Map issue navigation.** In Tempo Map📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switching back to Drum grid restores it with all its state. In Tempo Map | |
| switching back to Drum grid restores it with all its state. | |
| - **Tempo Map issue navigation.** In Tempo Map |
🤖 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` at line 42, Restore the changelog bullet boundary before the
“In Tempo Map mode” text so it becomes a separate entry rather than continuing
the drum-notation entry. Update the affected changelog text only, preserving
both entries’ wording.
|
Planned follow-up (Christian's call, 07-18): unify the drum layouts under this dropdown. This PR gives drum tracks Once both this and #317 are merged, a small follow-up commit lands on whichever merged second:
Net effect is drums on the same three-view vocabulary as fretted tracks: instrument geometry (String / Drum grid) → pitch-ordered (Piano roll / GM rows) → engraved (Tab-Notation / Notation). Deliberately not stacked as a third PR — this repo has been bitten by stacked-PR merge order before (#302 auto-closing), so it waits for a clean base. |
8d8dcee to
3ee0940
Compare
The live engraved score (tab-view-live.js) existed but was reachable
only through the view-cycle shortcut and the View menu — the top-right
switcher showed just String | Piano roll, and its active state ignored
the lens entirely (String stayed lit while the engraving was showing).
- The switcher becomes String · Piano roll · Tab · Notation. Tab opens
the engraved tablature; Notation the standard-notation staff — both
drive the existing lens (editorToggleTabView) with the matching staff
profile (editorSetTabViewStaff).
- A user whose Score-staff preference is "notation + tab" keeps it:
_tabStaffForClickPure preserves 'both' on either click, and
_viewSwitchActivePure lights Tab AND Notation together for it (the
view genuinely shows both).
- The active state is lens-aware; the roll-lock pill hides while the
lens owns the timeline; Tab/Notation hide on drum tracks (which have
no tab — same refusal the lens itself makes); keys stay piano-locked.
- Returning to String/Piano roll drops the lens before writing the
per-part view pref (mirrors the cycle's ordering).
- User Guide: the view list documents all four + the switcher.
Tests: tests/view_switch_lens.test.mjs — active-set derivation
(including 'both' lighting two buttons and junk-staff degradation),
'both'-preserving staff-for-click, lens entry via
editorSetViewMode('tab'/'notation'), String exit dropping the lens, and
keys/drums refusal. All fail on main. Existing tab_view_cycle suite
unchanged and green; full suite 210/210, lint 0 errors.
Runtime-verified through the host with Playwright: 4 buttons, Tab
renders the engraving, Notation switches staff + persists the pref,
String hides the mount.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
The pill toggle from the previous commit becomes a per-track view dropdown: String view · Piano roll · Tab · Notation · Notation + Tab. With a dropdown every staff profile is an explicit option, so the selected value is always a truthful readback of state (lens included) and the 'preserve the both pref on click' dance disappears — _tabStaffForClickPure is gone, _viewSwitchValuePure derives the single value. Drum tracks lose the engraved options (disabled + hidden) instead of buttons that refuse. Tests reworked to the value model; 'both' now pinned as an explicit choice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…down
Christian's parity call: drums should get Notation too. In the drum
editor the same dropdown now offers Drum grid / Notation, and the live
lens engraves the drum tab on a real percussion staff.
- src/alphatex.js: _alphaTexFromDrumHitsPure, the percussion flavor of
the generator. The bar walker (bucketing, gap durations, rest fill,
beatMap) is factored into a shared _alphaTexAssemblePure so drums and
fretted parts keep ONE quantization contract. DRUM_TEX_ARTICULATIONS
maps each piece to its alphaTab articulation id; `stack` has none, so
its hits are skipped and COUNTED, never silently dropped.
- Three things the browser taught us, each now pinned by a test:
* every drum beat is parenthesized — a bare `36.4` lexes as the FLOAT
36.4, not articulation 36 with a quarter duration (fretted notes
dodge this because `3.6.4` has two dots). This was a hard parse
error: the engraving came up blank.
* `\clef neutral` is BAR metadata and load-bearing — without it
alphaTab engraves the kit under a TREBLE clef.
* the percussion staff renders under StaveProfile.Default; forcing
Score (as fretted parts do) engraves it as a pitched staff.
- src/tab-view-live.js: a `_texSource` ('chart' | 'drums') selects the
generator, keys the render + api-rebuild, and routes click-to-select
into S.drumSel instead of S.sel. Entering the lens from the drum
editor keeps S.drumEditMode ON underneath (the lens paints over it),
so leaving Notation restores the grid with its state intact; the draw
pass drops the lens if the drum tab or mode goes away.
- src/key-view.js + screen.html: the dropdown shows the drum pair in
drum mode and the five standard views otherwise.
Tests: alphatex suite +5 (percussion header/clef/articulations, chord
grouping with same-piece dedupe, unmapped counting, clef only on bar 1,
and the fretted generator NEVER emitting a clef); view-switch suite +2
(drum value derivation, and drum-notation not leaving the drum editor).
All fail on main. Full suite 210/210, lint 0 errors.
Runtime-verified in the host: the dropdown offers Drum grid / Notation,
the engraving renders with a PERCUSSION clef, kick on the bottom space,
snare on the middle line, X noteheads for hats; switching back restores
the grid. Verified via alphaTab's parsed model too: isPercussion=true,
channel 9, articulations resolving to MIDI 36/38/42.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…tion With the GM roll (#317, merged into this branch) and the view dropdown both present, the drum piano roll was reachable only from the Rows button while fretted tracks advertised "Piano roll" in the dropdown — so a user checking the dropdown for "what layouts does this track have?" concluded drums had none. Christian's call: fold the unification in rather than defer it, since he controls the merge order. - The drum dropdown becomes Drum grid · Piano roll · Notation, where Piano roll selects GM-roll density. Full/Compact stay on the Rows button: they are densities of the same KIT-ordered grid, whereas the GM roll is a different axis (pitch). - Returning to Drum grid restores the density you came from (_drumKitDensityBack), so a Compact user isn't silently reset to Full by a trip through the roll. - Both controls route through ONE setter (_editorSetDrumDensity, which _editorToggleDrumDensity now delegates to), so the Rows button and the dropdown cannot drift apart — the dropdown reads _drumDensityMode(). - No import cycle: key-view.js -> drum.js is one-directional. Drums now match fretted tracks' vocabulary: instrument geometry (String view / Drum grid) -> pitch-ordered (Piano roll / GM rows) -> engraved (Tab-Notation / Notation). Tests: +4 in view_switch_lens (density-driven value derivation, Piano roll setting GM density with both controls agreeing, the Compact/Full round-trip restore, and the Rows cycle still wrapping through the same setter). Full suite 210/210, lint 0 errors. Runtime-verified in the host: the dropdown lists all three; Rows -> Compact then dropdown -> Piano roll flips the Rows button to GM roll; returning to Drum grid restores COMPACT (not Full); Notation engraves without disturbing the density; leaving it returns to the grid. Merge order: #317 first, then this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
8d8e90e to
431298f
Compare
1 participant
Why
Christian (07-18): "I need to also add the tab/notation view", then on review "it should be a dropdown, not a pill toggle" and "make sure drums have parity — drums should have notation available as well."
The live engraved score already shipped (
tab-view-live.js) but was effectively invisible: reachable only via the view-cycle shortcut or View ▸ Score staff, while the top-right switcher showed justString | Piano roll— and kept String lit even while the engraving owned the timeline. Drums had no engraved view at all, and (with #317) their piano roll was reachable only from the Rows button.What
A per-track view dropdown, replacing the two-button switcher:
Every staff profile is an explicit option, so the selected value is always a truthful readback of state — the lens included. No pref-preservation special-casing needed.
Drum parity, both halves:
S.drumSel) and seeks. The drum-editor mode stays on underneath the lens, so switching back restores the grid with all its state.Drum gridrestores the density you came from rather than resetting to Full, and both controls route through one setter so they can't drift._alphaTexFromDrumHitsPureis the percussion flavor of the generator; the bar walker (bucketing, gap durations, rest fill, beatMap) is factored into a shared_alphaTexAssemblePure, so drums and fretted parts keep one quantization contract. Pieces with no notation symbol (stack) are skipped and counted, never silently dropped.Three things only the browser caught
Each was a real defect found by driving the actual host; each is now pinned by a test:
36.4lexes as the float 36.4, not articulation 36 with a quarter duration — fretted notes dodge this because3.6.4has two dots. Hard alphaTex parse error: the engraving came up blank. Every drum beat is now parenthesized ((36).4).\clef neutralis bar metadata and load-bearing — without it alphaTab engraves the kit under a treble clef.StaveProfile.Default— forcingScore(what fretted parts use) engraves it as a pitched staff.Tests
view_switch_lens.test.mjs(new): value derivation incl.bothand the drum triple, lens entry/exit per option, drum-notation not leaving the drum editor, Piano roll ↔ GM density with both controls agreeing, the Compact→roll→Compact restore, the Rows cycle through the shared setter, keys/drums refusal.alphatex.test.mjs(+5): percussion header/clef/articulation ids, chord grouping with same-piece dedupe, unmapped counting, clef on bar 1 only, and the fretted generator never emitting a clef.All fail on main.
npm test210/210 · lint 0 errors.Runtime verification (real host + Playwright)
Fretted: dropdown renders the right option set, Tab and Notation + Tab render and persist the staff pref, String exits the lens. Drums: all three options listed; Rows→Compact then dropdown→Piano roll flips the Rows button to GM roll; returning to Drum grid restores Compact, not Full; Notation engraves with a percussion clef (kick bottom space, snare middle line, X noteheads for hats) without disturbing density. Cross-checked against alphaTab's parsed model:
isPercussion: true, channel 9, articulations resolving to MIDI 36/38/42.🤖 Generated with Claude Code
https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW