Skip to content

fix: halt doChorus, doTremolo, doDistortion when depth/distortion validation fails - #7979

Open
Abhishek-Sonje wants to merge 1 commit into
sugarlabs:masterfrom
Abhishek-Sonje:fix/7978-halt-invalid-chorus-tremolo-distortion
Open

fix: halt doChorus, doTremolo, doDistortion when depth/distortion validation fails#7979
Abhishek-Sonje wants to merge 1 commit into
sugarlabs:masterfrom
Abhishek-Sonje:fix/7978-halt-invalid-chorus-tremolo-distortion

Conversation

@Abhishek-Sonje

@Abhishek-Sonje Abhishek-Sonje commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

Singer.ToneActions.doChorus, doTremolo, and doDistortion validate their depth/distortion parameter, but when validation fails, each function only displays an error message and sets activity.logo.stopTurtle = true. Since execution does not stop immediately, the function continues and updates the turtle's singer state with the invalid value, and registers a playback listener regardless.

This change adds early returns after each validation check so that the functions exit immediately when invalid input is detected.

Related Issue

This PR fixes #7978

PR Category

  • Bug Fix — Fixes a bug or incorrect behavior
  • Feature — Adds new functionality
  • Performance — Improves performance (load time, memory, rendering, etc.)
  • Tests — Adds or updates test coverage
  • Documentation — Updates to docs, comments, or README
  • Chore / Refactor — Maintenance, cleanup, or refactoring with no behavior change
  • CI/CD — Changes to CI/CD workflows and automation

Changes Made

  • Added an early return after the invalid depth validation in Singer.ToneActions.doChorus.
  • Added an early return after the invalid depth validation in Singer.ToneActions.doTremolo.
  • Added an early return after the invalid distortion validation in Singer.ToneActions.doDistortion.
  • Strengthened the existing Jest tests for all three functions to assert that the relevant singer state arrays remain empt when validation fails
    (previously the tests only checked that errorMsg fired and stopTurtle was set, so they passed even with the fall-through bug in place).

Testing Performed

  • Ran: npx jest js/turtleactions/tests/ToneActions.test.js

  • Confirmed the new/updated assertions fail against the unpatched functions
    (proving they catch the leak), and pass after adding the returns.

  • Confirmed valid chorus/tremolo/distortion values still work as expected.

  • Confirmed invalid parameters display the existing error message and no
    longer modify turtle state or register a listener.

Checklist

  • I have tested these changes locally and they work as expected.
  • I have added/updated tests that prove the effectiveness of these changes.
  • I have updated the documentation to reflect these changes, if applicable. (Not applicable)
  • I have followed the project's coding style guidelines.
  • I have run npm run lint and npx prettier --check . with no errors.
  • I have addressed the code review feedback from the previous submission, if applicable.
  • I have enabled "Allow edits from maintainers".

Additional Notes for Reviewers

Same root cause as #7921, fixed for doVibrato in #7922 — this PR applies the identical fix to three sibling effect functions in the same file (js/turtleactions/ToneActions.js) discovered while auditing for similar stopTurtle = true-without-return patterns.

Copilot AI lite review requested due to automatic review settings August 6, 2026 10:53
@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior tests Adds or updates test coverage size/S Small: 10-49 lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Aug 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a runtime bug in the tone-effects action layer (Singer.ToneActions) where validation failures would still fall through and mutate turtle singer state / register listeners, despite setting activity.logo.stopTurtle = true.

Changes:

  • Added early return statements after invalid depth/distortion validation in doChorus, doTremolo, and doDistortion.
  • Strengthened Jest coverage to assert the relevant singer state arrays remain unchanged when validation fails.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
js/turtleactions/ToneActions.js Adds early returns to prevent state mutation/listener registration after validation errors in chorus/tremolo/distortion actions.
js/turtleactions/tests/ToneActions.test.js Extends regression tests to assert singer state arrays remain empty when validation fails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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/S Small: 10-49 lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

[Bug] doChorus, doTremolo, doDistortion continue execution after validation failure

2 participants