Skip to content

fix: route portamento notes through setNote path, not fast-path triggerRelease#7758

Open
santhosh-7777 wants to merge 1 commit into
sugarlabs:masterfrom
santhosh-7777:fix/7023-glide-portamento-fastpath
Open

fix: route portamento notes through setNote path, not fast-path triggerRelease#7758
santhosh-7777 wants to merge 1 commit into
sugarlabs:masterfrom
santhosh-7777:fix/7023-glide-portamento-fastpath

Conversation

@santhosh-7777

@santhosh-7777 santhosh-7777 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR Category

  • Bug Fix
  • Feature
  • Performance
  • Tests
  • Documentation

Problem

Playing a glide block over multiple notes did not slide the pitch across the whole phrase. Instead, the first note played normally and the next notes played as separate, disconnected notes breaking issue.

fixes #7023.

Root Cause

Inside _performNotes() in js/utils/synthutils.js, there is a "fast path" added for performance that skips the normal effects setup for simple notes.

  • The check that decides which notes need the full setup (_needsGraphRewire, ~line 1864) only looks at vibrato, distortion, tremolo, phaser, chorus, and neighbor effects.
  • It never checks for portamento (glide).
  • Because of this, every glide note takes the fast path, which always calls triggerAttackRelease and completely ignores the setNote flag.
  • The correct glide logic already existed in the slow path (~lines 2043–2062), where setNote is checked and synth.setNote(notes) is called to continue the same note and bend the pitch , but glide notes never reached that code.

Fix

Added doPortamento && setNote to the _needsGraphRewire check, so glide notes go through the slow path where setNote is handled correctly.

Testing

  1. Open Music Blocks, drag a glide block onto the canvas.
  2. Put two note blocks inside it with different pitches (e.g. sol 4 then la 4).
  3. Run the project.
  4. Before the fix: two separate notes play, no slide.
  5. After the fix: pitch slides smoothly from the first note into the second, no gap.
  6. Added 3 Jest tests in js/utils/__tests__/synthutils.test.js (_performNotes glide/portamento setNote routing):
    • checks setNote is called (not triggerAttackRelease) when glide + setNote are both true
    • checks triggerAttackRelease still runs when setNote is false (no regression)
    • checks normal, non-glide notes still work the same as before
  7. Ran the full synthutils.test.js suite locally , all 118 tests pass, nothing broke.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.83%. Comparing base (338362b) to head (9b9e98c).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7758      +/-   ##
==========================================
+ Coverage   56.80%   56.83%   +0.02%     
==========================================
  Files         172      172              
  Lines       57683    57683              
==========================================
+ Hits        32767    32784      +17     
+ Misses      24916    24899      -17     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Jul 8, 2026
@walterbender

Copy link
Copy Markdown
Member

@ssz2605 what do you think? LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

[Bug]: GlideBlock glissando ends before the full note group

2 participants