Skip to content

fix: resolve off-by-one in ModeWidget._clear() keeping tonic active#7218

Open
rakshaak29 wants to merge 1 commit into
sugarlabs:masterfrom
rakshaak29:fix/modewidget-clear-tonic-bug
Open

fix: resolve off-by-one in ModeWidget._clear() keeping tonic active#7218
rakshaak29 wants to merge 1 commit into
sugarlabs:masterfrom
rakshaak29:fix/modewidget-clear-tonic-bug

Conversation

@rakshaak29

@rakshaak29 rakshaak29 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Description

This PR resolves a critical off-by-one bug in the Mode Widget where the _clear() method failed to deselect the root/tonic note (index 0).
Because _selectedNotes[0] was never cleared, a cascade of silent failures occurred whenever a user attempted to clear the widget to build a new mode from scratch:

  • Data Corruption: The _calculateMode() logic would evaluate the single note as [12], which _save() would blindly overwrite into localStorage.custommode, clobbering the user's legitimately saved custom modes.
  • Mislabeled Export: Since [12] does not match any valid entry in MUSICALMODES, _setModeName() would blank the mode label, causing the generated action and definemode blocks to always incorrectly default to the name "custom".
  • Broken Features: The Rotate animations (_rotateRight / _rotateLeft) immediately abort if _selectedNotes[0] is true, meaning rotation was permanently broken after clicking Clear.
  • UI/UX: The wheel retained a "ghost" selection, confusing users.

PR Category

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

  • js/widgets/modewidget.js: Updated the loop bounds in _clear() to initialize at i = 0 instead of i = 1, ensuring all 12 note positions are properly deselected.
  • js/widgets/__tests__/modewidget.test.js: Updated the should clear all notes except root test. The test was originally written to explicitly expect this broken behavior. It has been renamed to should clear all notes including root and updated to assert that _selectedNotes[0] evaluates to false.

Testing Performed

  • Unit Tests: Ran npm test. All 5,006 tests across 152 suites pass successfully.
  • Manual Verification: Verified that clicking the Clear button now fully clears the wheel, rotation animations function normally, and custom modes can be saved without data corruption.

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 45.18% | Branches: 36.53% | Functions: 50.56% | Lines: 45.62%

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:

planetInterface.test.js

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/XS Extra small: < 10 lines changed area/javascript Changes to JS source files area/tests Changes to test files labels May 6, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been open for more than 60 days without any activity. It will be closed in 3 days unless the stale label is removed or commented on.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.27%. Comparing base (e58db21) to head (1c78c3b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7218   +/-   ##
=======================================
  Coverage   59.27%   59.27%           
=======================================
  Files         176      176           
  Lines       57975    57975           
=======================================
  Hits        34365    34365           
  Misses      23610    23610           

☔ 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.

@rakshaak29 rakshaak29 force-pushed the fix/modewidget-clear-tonic-bug branch from fb2f52a to 1c78c3b Compare July 13, 2026 07:48
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/XS Extra small: < 10 lines changed Stale

Projects

Development

Successfully merging this pull request may close these issues.

2 participants