feat(editor): View ▸ Canvas appearance — customizable grid & canvas, DAW-style - #300
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 (10)
📝 WalkthroughWalkthroughChangesThe editor now provides persisted canvas appearance controls for grid strength, brightness, color intensity, and hue. A View menu entry and modal expose live updates and reset behavior, while structural canvas rendering uses the adjusted palette across editor, drum, piano-roll, and parts views. Canvas appearance customization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EditorMenu
participant AppearanceModal
participant CanvasAppearance
participant CanvasRenderer
EditorMenu->>AppearanceModal: open Canvas Appearance
AppearanceModal->>CanvasAppearance: update slider setting
CanvasAppearance->>CanvasRenderer: provide adjusted CP palette
CanvasRenderer->>CanvasRenderer: redraw structural canvas colors
AppearanceModal->>CanvasAppearance: reset settings
CanvasAppearance->>CanvasRenderer: redraw defaults
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🤖 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 16-22: Update the CHANGELOG entry’s semantic-color exclusion list
to remove “lane,” while retaining the exclusions for string, drum piece, note,
selection, and playhead colors. Keep the structural palette description and
implementation references unchanged.
In `@screen.html`:
- Around line 723-764: The editor-canvas-appearance-modal needs dialog
accessibility and focus management. Add role="dialog", aria-modal="true", and an
accessible name; associate each visible label with its corresponding slider, and
give the close button an aria-label. Update editorShowCanvasAppearance and
editorHideCanvasAppearance to move focus into the modal when opened and restore
it to the triggering control when closed, preserving the existing visibility
behavior.
In `@src/canvas-appearance.js`:
- Around line 36-42: Update _appearanceFieldPure to reject null and blank-string
values before calling Number(value), returning the field’s default for those
inputs. Preserve the existing finite-number validation and range clamping for
valid numeric values.
In `@src/drum.js`:
- Around line 308-311: Update the drum beat rendering loop near the lane
separator to replace hard-coded RGBA colors with CP-based palette lookups, using
CP('gridMeasure') for measure lines and CP('gridBeat') for regular beat lines.
Preserve the existing line classification and ensure grid strength, brightness,
intensity, and hue settings affect both drum grid types.
🪄 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: e1bb0bf1-2442-457a-92e3-a05e481850e8
📒 Files selected for processing (9)
CHANGELOG.mddocs/USER-GUIDE.mdscreen.htmlsrc/canvas-appearance.jssrc/draw.jssrc/drum.jssrc/menu-bar.jssrc/parts-view.jstests/canvas_appearance.test.mjs
Comment on lines
+723
to
+764
| <div id="editor-canvas-appearance-modal" class="hidden absolute inset-0 z-50 flex items-center justify-center" style="background:rgba(0,0,0,0.7)"> | ||
| <div class="bg-dark-700 rounded-xl border border-gray-700 w-full max-w-sm flex flex-col"> | ||
| <div class="flex items-center justify-between px-4 py-3 border-b border-gray-700"> | ||
| <span class="text-sm font-medium">Canvas Appearance</span> | ||
| <button onclick="editorHideCanvasAppearance()" class="text-gray-500 hover:text-white">×</button> | ||
| </div> | ||
| <div class="p-4 space-y-4"> | ||
| <div> | ||
| <div class="flex justify-between items-baseline mb-1"> | ||
| <label class="text-xs font-medium text-gray-400">Grid lines</label> | ||
| <span id="editor-canvas-app-grid-val" class="text-xs text-gray-500">100%</span> | ||
| </div> | ||
| <input type="range" id="editor-canvas-app-grid" min="25" max="400" step="5" value="100" | ||
| class="w-full" oninput="editorCanvasAppearanceInput('grid', this.value)" | ||
| title="Strength of grid lines and lane separators — raise it if beat lines are hard to see"> | ||
| </div> | ||
| <div> | ||
| <div class="flex justify-between items-baseline mb-1"> | ||
| <label class="text-xs font-medium text-gray-400">Brightness</label> | ||
| <span id="editor-canvas-app-brightness-val" class="text-xs text-gray-500">100%</span> | ||
| </div> | ||
| <input type="range" id="editor-canvas-app-brightness" min="50" max="250" step="5" value="100" | ||
| class="w-full" oninput="editorCanvasAppearanceInput('brightness', this.value)" | ||
| title="Overall canvas background brightness"> | ||
| </div> | ||
| <div> | ||
| <div class="flex justify-between items-baseline mb-1"> | ||
| <label class="text-xs font-medium text-gray-400">Color intensity</label> | ||
| <span id="editor-canvas-app-intensity-val" class="text-xs text-gray-500">100%</span> | ||
| </div> | ||
| <input type="range" id="editor-canvas-app-intensity" min="0" max="200" step="5" value="100" | ||
| class="w-full" oninput="editorCanvasAppearanceInput('intensity', this.value)" | ||
| title="Color saturation of the canvas palette (0 = grayscale)"> | ||
| </div> | ||
| <div> | ||
| <div class="flex justify-between items-baseline mb-1"> | ||
| <label class="text-xs font-medium text-gray-400">Hue</label> | ||
| <span id="editor-canvas-app-hue-val" class="text-xs text-gray-500">0°</span> | ||
| </div> | ||
| <input type="range" id="editor-canvas-app-hue" min="-180" max="180" step="5" value="0" | ||
| class="w-full" oninput="editorCanvasAppearanceInput('hue', this.value)" | ||
| title="Hue rotation of the canvas palette (structural colors only — string, note and drum-piece colors keep their meaning)"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find the modal handlers.
git ls-files | rg '(^|/)screen\.html$|(^|/)(.*\.(js|ts|html))$' >/tmp/files.txt
printf '\n-- screen.html locations --\n'
rg -n "editor-canvas-appearance-modal|editorHideCanvasAppearance|editorCanvasAppearanceInput|Canvas Appearance" screen.html || true
printf '\n-- nearby lines in screen.html --\n'
sed -n '700,790p' screen.html
printf '\n-- search for related JS handlers in repo --\n'
rg -n "function editorHideCanvasAppearance|editorHideCanvasAppearance\s*\(|editorShowCanvasAppearance|editorCanvasAppearanceInput\s*\(" . || trueRepository: got-feedBack/feedBack-plugin-editor
Length of output: 7813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- canvas appearance handlers --\n'
sed -n '180,280p' src/canvas-appearance.js
printf '\n-- search for focus handling and ARIA in modal-related code --\n'
rg -n "aria-|role=|focus\(|activeElement|restore|inert|trap|dialog" src screen.html || true
printf '\n-- inspect any label associations in screen.html --\n'
rg -n "editor-canvas-app-(grid|brightness|intensity|hue)|for=\"editor-canvas-app" screen.html || trueRepository: got-feedBack/feedBack-plugin-editor
Length of output: 41151
Make the Canvas Appearance modal a proper dialog. It still lacks role="dialog"/aria-modal="true", an accessible name, and focus handling on open/close. Associate the four visible labels with their sliders, give the × button an aria-label, and move focus into the dialog on open and back to the trigger on close.
🤖 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 723 - 764, The editor-canvas-appearance-modal needs
dialog accessibility and focus management. Add role="dialog", aria-modal="true",
and an accessible name; associate each visible label with its corresponding
slider, and give the close button an aria-label. Update
editorShowCanvasAppearance and editorHideCanvasAppearance to move focus into the
modal when opened and restore it to the triggering control when closed,
preserving the existing visibility behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/drum_grid_appearance.test.mjs (1)
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover measure-grid scaling in the slider test.
The test changes the appearance setting but asserts only
gridBeat. A regression that leaves measure lines unchanged would still pass, despite both beat and measure grids being part of this feature.Proposed assertion
const beatDefault = CP('gridBeat'); + const measureDefault = CP('gridMeasure'); setCanvasAppearance('grid', 400); const beatBright = CP('gridBeat'); + const measureBright = CP('gridMeasure'); assert.notStrictEqual(beatBright, beatDefault); // sanity: slider changed the color + assert.notStrictEqual(measureBright, measureDefault); const strokes = drive(); assert.ok(strokes.includes(beatBright), `raising Grid lines must brighten the drum beat grid to ${beatBright}, got ${JSON.stringify(strokes)}`); + assert.ok(strokes.includes(measureBright), + `raising Grid lines must brighten the measure grid to ${measureBright}, got ${JSON.stringify(strokes)}`);🤖 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/drum_grid_appearance.test.mjs` around lines 51 - 59, Extend the test “the Grid-lines slider reaches the drum beat grid” to also capture the default and updated measure-grid color via CP('gridMeasure'), then assert the driven strokes include the updated measure color. Keep the existing gridBeat assertions and slider behavior unchanged.
🤖 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 `@tests/drum_grid_appearance.test.mjs`:
- Around line 51-59: Extend the test “the Grid-lines slider reaches the drum
beat grid” to also capture the default and updated measure-grid color via
CP('gridMeasure'), then assert the driven strokes include the updated measure
color. Keep the existing gridBeat assertions and slider behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae654f26-f162-4a58-a685-13dd3898ef66
📒 Files selected for processing (2)
src/drum.jstests/drum_grid_appearance.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/drum.js
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
byrongamatos
force-pushed
the
feat/canvas-appearance
branch
from
July 16, 2026 18:48
b28f4cd to
f7c7899
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
Community fix + feature in one (from #song-editor-feedback):
The fix: default beat lines were
#16162cat 0.5 px on a#0c0c1cbackground — invisible on many panels. Defaults are now#20203eat 1 px (beats) /#32325c(measures): visible out of the box.The feature: View ▸ Canvas appearance… — modeled on Ableton Live 12's Theme ▸ Customization tab (the org's DAW design reference), four live sliders:
Deliberate boundary: only the canvas's structural palette restyles. String/lane colors, drum piece colors, note, selection, and playhead colors carry meaning and never change. The dialog says so.
How
New
src/canvas-appearance.js: a named structural palette (CP(name)) + a pure hex→HSL→hex adjustment pipeline (@pureblock). Adjusted colors cache per settings generation — steady-state draw cost is one map lookup (no per-frame color math). Settings are a global editor preference (editorCanvasAppearancein localStorage, likeeditorTheme— the chrome theme deliberately leaves the canvas alone; this is its canvas counterpart), junk-tolerant and clamped. Module is window-guarded so it imports cleanly under node.Adopters in this PR:
draw.js(lanes, piano rows, beat/measure grid, label gutters),drum.js(lane grid),parts-view.js(lanes, headers, downbeat ticks). Remaining surfaces (ruler, tempo lane, annotation lanes, waveform bg) can adoptCPincrementally.Docs: USER-GUIDE gets a "Canvas appearance" section under The Workspace.
Tests
tests/canvas_appearance.test.mjs— 12 new: hex↔HSL round-trip, sanitization/clamping (junk → per-field default), adjustment semantics (brightness scaling, grid strength reaching only grid-bucket entries, desaturation, hue rotation ±4°, never-clips-to-white guard), CP cache invalidation on change, and a pin that the default beat line is genuinely brighter than the old#16162cwith the measure > beat visual hierarchy intact.Gates: JS suite green (
song_fit's failure reproduces on pristine main — environmental), lint 0 errors / 3 baseline warnings,routes.pyuntouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit