Skip to content

feat: refactor GlideBlock to resolve double-execution and timing issues (Fix #6917)#6918

Open
gcharpe1604 wants to merge 5 commits into
sugarlabs:masterfrom
gcharpe1604:feat/glide-block-refactor
Open

feat: refactor GlideBlock to resolve double-execution and timing issues (Fix #6917)#6918
gcharpe1604 wants to merge 5 commits into
sugarlabs:masterfrom
gcharpe1604:feat/glide-block-refactor

Conversation

@gcharpe1604

Copy link
Copy Markdown
Contributor

Description

This PR refactors the GlideBlock (glissando) implementation to remove the dependency on Singer.noteCounter. This resolves the "double-execution" bug where blocks inside the Glide clamp were run twice, causing unintended side-effects on variables and state. It also implements a buffering mechanism in the Singer engine to ensure perfectly synchronized glissando playback.

Related Issue

This PR fixes #6917

PR Category

  • Bug Fix — Fixes a bug or incorrect behavior (Double-execution of blocks)
  • Feature — Adds new functionality
  • Performance — Improves performance
  • Tests — Adds or updates test coverage

Changes Made

  • Removed Singer.noteCounter from GlideBlock.flow in js/blocks/OrnamentBlocks.js.
  • Implemented a glideBuffer in js/turtle-singer.js to collect notes during the synchronous execution of the glissando clamp.
  • Added Singer.playGlideBuffer to handle synchronized audio triggering with precise Tone.now() offsets.
  • Refactored Singer.processNote to unify synth trigger logic across instruments and voices.
  • Updated js/blocks/__tests__/OrnamentBlocks.test.js to verify the new buffering and signal-based playback behavior.

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

Copy link
Copy Markdown
Contributor

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

Coverage: Statements: 44.81% | Branches: 36.3% | Functions: 49.9% | Lines: 45.25%

@gcharpe1604 gcharpe1604 force-pushed the feat/glide-block-refactor branch from 8fca8f7 to a4949bb Compare April 26, 2026 08:08
@github-actions

Copy link
Copy Markdown
Contributor

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

Coverage: Statements: 44.82% | Branches: 36.33% | Functions: 49.89% | Lines: 45.25%

@github-actions

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

@github-actions github-actions Bot added needs-rebase size/XXL XXL: 1000+ lines changed area/css Changes to CSS/SASS style files area/docs Changes to documentation area/ci-cd Changes to CI/CD workflows area/lib Changes to library files area/core Changes to core app entry files and removed needs-rebase size/M Medium: 50-249 lines changed labels May 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Coverage: Statements: 47.56% | Branches: 38.82% | Functions: 52.43% | Lines: 47.97%

Note: These failures may be introduced by this PR or may already exist in the master branch.
Tip: Update your branch with the latest master and rerun tests.
If the same failures are present on master, they are likely not introduced by this PR.

Failed Tests:

LocalCard.test.js

@gcharpe1604 gcharpe1604 force-pushed the feat/glide-block-refactor branch from 1252889 to fb2c856 Compare July 12, 2026 07:00
@github-actions github-actions Bot added size/M Medium: 50-249 lines changed and removed size/XXL XXL: 1000+ lines changed area/css Changes to CSS/SASS style files area/docs Changes to documentation labels Jul 12, 2026
@github-actions github-actions Bot removed area/ci-cd Changes to CI/CD workflows area/lib Changes to library files area/core Changes to core app entry files labels Jul 12, 2026
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.26087% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.30%. Comparing base (a76b8bb) to head (4c5015b).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
js/turtle-singer.js 61.53% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6918      +/-   ##
==========================================
+ Coverage   59.28%   59.30%   +0.02%     
==========================================
  Files         176      176              
  Lines       57967    57995      +28     
==========================================
+ Hits        34365    34395      +30     
+ Misses      23602    23600       -2     

☔ 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 size/L Large: 250-499 lines changed and removed size/M Medium: 50-249 lines changed labels Jul 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

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/L Large: 250-499 lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

Double-execution side-effects and timing inaccuracy in GlideBlock (Glissando)

1 participant