feat(editor): transport control bar + dual-domain LCD (workspace-shell B2) - #195
Merged
Merged
Conversation
…l B2) One always-present bar directly above the timeline (charrette $2.4/D-C3): go-to-start, rewind-a-bar, stop, play/pause, forward-a-bar, record, an LCD, and mirror groups for the existing toolbar commands (Parts/Mix/Follow, Click/Clap/A/B/Snap). No master-mute — mute/solo are per-track ($2.6). The LCD ports Virtuoso's recessed-panel skin GRAMMAR as .editor-lcd-* and rebuilds the commit wiring against the editor's audio-anchored tempo map: - Position (bars:beats:ticks) and Time (m:ss.mmm) shown together, both computed through beatOf/timeOf, with a toggle for which is primary. Editing either seeks (Enter applies, Escape reverts). - Tempo is an editable input only in FREE (no-audio) mode; with a recording the grid is fitted to the audio, so the cell becomes a derived readout wearing the AUDIO badge and BPM editing stays with Tempo Map/Sync. The free-mode input commits through window.editorSetBPM — the same path as the toolbar control (flatten-confirm on variable maps included). - Key writes through to the Key controls; Meter is a readout (meter edits live in Tempo Map mode); Sel counts the selection; a mode badge names the semantics (fitted-to-audio vs free). - No Count-in cell: the editor has no count-in feature yet, and a cell must write through to a REAL control — it arrives with that feature. Keydown inside an LCD field stops propagation so the canvas shortcut layer never sees typing (space can't toggle play mid-edit). The LCD refreshes on the transport tick (updateTimeDisplay) and selection refreshes (updateStatus), never from draw(); every DOM write is skip-if-unchanged. Customize Control Bar (down-arrow button or right-click, Logic idiom) shows/hides groups and cells, persisted as an editor pref. The one document-level listener (menu click-away) rides the teardown registry, so re-injection cannot stack copies. tests/transport_lcd.test.mjs (12): BBT format/parse as inverses on a DRIFTING grid (the tempo-map pillar), tick rollover, degenerate-grid degradation, meter across a mid-song signature change, the BPM-semantics decision, the keystroke policy, and pref corruption tolerance. Suite 91/91, ESLint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
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 (3)
📝 WalkthroughWalkthroughAdds a persistent transport control bar above the timeline with tempo-map-driven Position/Time LCD readouts, editable fields, customization persistence, transport controls, optimized refreshes, styling, editor wiring, and pure-helper tests. ChangesTransport bar and LCD
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant TransportBar as transport-bar.js
participant TempoMap as Tempo map
participant LCD as Position/Time LCD
Editor->>TransportBar: Initialize transport bar
Editor->>TransportBar: Refresh on time/status update
TransportBar->>TempoMap: Convert cursor time to LCD values
TempoMap-->>TransportBar: Position, meter, and tempo
TransportBar->>LCD: Update readouts and mirror states
LCD->>TransportBar: Commit edited field
TransportBar->>Editor: Apply seek or editor setting
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/transport-bar.js`:
- Around line 303-310: Update the Record button construction in the transport
bar so `tbtn()` receives or combines the `editor-transport-rec` class with its
default `editor-transport-btn` class, rather than appending a second class
attribute. Ensure the generated Record button contains both classes and
preserves its existing label and behavior.
- Around line 59-64: Update _lcdClockPure so millisecond rounding carries into
seconds and minutes instead of being reduced with % 1000; round the total
milliseconds first, then derive minutes, seconds, and milliseconds from that
normalized value while preserving the existing invalid/negative input handling
and output format.
- Around line 84-127: Update _lcdBBTPure and _lcdParseBBTPure to enforce valid,
round-trippable bar:beat:tick values: carry rounded ticks and beat overflow
across measure boundaries using the next labeled downbeat, reject beat numbers
beyond the current measure’s span during parsing, and explicitly handle pickup
positions before the first labeled downbeat so they never produce zero or
negative beats. Preserve non-dense imported measure lookup rather than deriving
bar numbers arithmetically.
🪄 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: d64580ad-af59-412a-888b-90e6ef6bd03b
📒 Files selected for processing (6)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/main.jssrc/transport-bar.jstests/transport_lcd.test.mjs
- _lcdClockPure: total-ms math so rounded milliseconds carry into the seconds/minutes fields (59.9996 reads 1:00.000, not 0:59.000). - _lcdBBTPure: quantize the beat COORDINATE to the tick grid first, so the rounding carry crosses beat AND bar boundaries (a hair before bar 2 reads 2:1:000, never 1:5:000); pre-grid coordinates clamp at beat 1. - The Record button emitted two class attributes (the second is ignored by HTML parsing), losing its .editor-transport-rec styling — now one. Both carries pinned by new tests. Suite 91/91. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…uild wireBar() ran on every buildBar() and re-added three container-delegated listeners (contextmenu/change/reset-click) to the persistent #editor-transport-bar element, so they accumulated across rebuilds. Because the change handler itself calls buildBar(), k Customize toggles fanned out into k listeners each firing k full rebuilds. Move those three to a wireBarContainer() wired once from initTransportBar() on the persistent element; hoist the shared menuToggle to module scope. Child-level per-build listeners stay in wireBar() (they die with the replaced innerHTML). Behavior unchanged; 91 test files green, lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 12, 2026
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
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
The workspace-shell's transport bar (charrette §2.4 / D-C3): one always-present row directly above the timeline —
[Parts Mix Follow] │ ⏮ ⏪ ■ ▶/⏸ ⏩ ● │ LCD │ [Click Clap A/B Snap] ▾The LCD ports Virtuoso's recessed-panel skin grammar (
.editor-lcd-*) with the commit wiring rebuilt against the editor's audio-anchored tempo map:beatOf/timeOf— the beat-primary pillar made literal — with a▸toggle for which is primary. Editing either seeks; Enter applies, Escape reverts.editorSetBPMpath as the toolbar control (flatten-confirm included); with a recording present the grid is fitted to the audio, so the cell becomes a derived readout wearing the AUDIO badge and BPM editing stays with Tempo Map/Sync. A mode badge (AUDIO/FREE) names the semantics.▾or right-click, Logic idiom): show/hide the utility groups and each LCD cell; persisted as an editor pref, never in the pack.Every button delegates to the existing command surface (a re-presentation, not a re-plumb) and mirror state re-syncs from the source of truth on each tick, so the old toolbar and this bar coexist without drifting until B4/B5 re-home the toolbar.
Deliberate scope cuts
Discipline
updateTimeDisplay) + selection refreshes (updateStatus), neverdraw(); every DOM write is skip-if-unchanged.Tests
tests/transport_lcd.test.mjs(12, real-import): BBT format/parse as inverses on a drifting grid (a constant-BPM conversion visibly fails the fixture), tick rollover, degenerate-grid degradation to time-only, meter across a mid-song signature change, the BPM-semantics decision, the Enter/Escape keystroke policy, and pref corruption tolerance. Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.Screenshots (module rendered with real state, both modes + customize menu) shared with Christian during review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit