Skip to content

fix: only pop instrumentNames if setTimbre pushed it#5932

Open
Ady0333 wants to merge 1 commit intosugarlabs:masterfrom
Ady0333:fix/settimbre-instrument-names-pop
Open

fix: only pop instrumentNames if setTimbre pushed it#5932
Ady0333 wants to merge 1 commit intosugarlabs:masterfrom
Ady0333:fix/settimbre-instrument-names-pop

Conversation

@Ady0333
Copy link
Contributor

@Ady0333 Ady0333 commented Feb 27, 2026

Summary

This PR fixes a stack imbalance in the Set Timbre block where nested blocks using the same instrument could remove the wrong entry from the instrumentNames stack, causing subsequent notes to play with an incorrect timbre.


What changed

Added a small guard in setTimbre() to track whether the current block instance actually pushed a value to instrumentNames. The cleanup listener now pops the stack only when that push occurred.


Why this change was needed

The push to instrumentNames was guarded by !includes(synth) to prevent duplicate entries. However, the listener always executed pop().

When identical Set Timbre blocks were nested, the inner block skipped the push but still executed the pop in its listener. This removed the outer block's entry, leaving the stack empty while still inside the outer scope.

As a result:

  • last(instrumentNames) returned undefined
  • Notes after the inner block played with the default synth instead of the intended timbre
  • inSetTimbre was also cleared prematurely, disabling timbre effects for the remaining notes inside the outer block

Scope

  • Only affects setTimbre() in js/turtleactions/ToneActions.js
  • No changes to note scheduling, synth loading, or block execution flow
  • Existing behavior for non-nested timbre blocks remains unchanged

Verification

  • Tested nested Set Timbre blocks using the same instrument
  • Confirmed outer timbre remains active after the inner block exits
  • Notes play with the correct instrument throughout execution
  • All tests pass with no regressions

Fix


if (!tur.singer.instrumentNames.includes(synth)) {
tur.singer.instrumentNames.push(synth);
pushed = true;
}

const __listener = () => {
tur.inSetTimbre = false;
if (pushed) {
tur.singer.instrumentNames.pop();
}
};

Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
@Ady0333
Copy link
Contributor Author

Ady0333 commented Feb 27, 2026

@walterbender and @vanshika2720 and @omsuneri , Please review this pr.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant