Skip to content

Refactor/activity abc parser#7511

Draft
vanshika2720 wants to merge 1 commit into
sugarlabs:masterfrom
vanshika2720:refactor/activity-abc-parser
Draft

Refactor/activity abc parser#7511
vanshika2720 wants to merge 1 commit into
sugarlabs:masterfrom
vanshika2720:refactor/activity-abc-parser

Conversation

@vanshika2720

@vanshika2720 vanshika2720 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

refactor: extract ABC parser into js/activity/abc-parser.js

Description

This PR extracts the ABC notation parser from js/activity.js into a dedicated module:

js/activity/abc-parser.js

The goal is to improve maintainability and reduce the size and complexity of the Activity implementation by isolating the ABC import subsystem into its own file.

Motivation

The ABC parser previously lived inline inside activity.js, which is one of the largest files in the codebase.

The parser is a self-contained subsystem responsible for:

  • ABC note parsing
  • Pitch conversion
  • Duration conversion
  • Block generation
  • Repeat handling

Moving this logic into a dedicated module follows the same modularization approach already used by other Activity subsystems and makes future maintenance easier.

Changes

New module

Created:

js/activity/abc-parser.js

The module now contains:

  • _adjustPitch()
  • _abcToStandardValue()
  • _createPitchBlocks()
  • _searchIndexForMusicBlock()
  • setupActivityAbcParser()

setupActivityAbcParser() attaches parseABC() to the Activity instance during initialization.

Activity integration

Removed the inline ABC parser implementation from:

js/activity.js

and replaced it with:

setupActivityAbcParser(this);

Loader updates

Updated:

js/loader.js

to:

  • register the activity/abc-parser RequireJS path
  • load the parser module before Activity initialization

Test updates

Updated:

js/__tests__/activity_startup_recovery.test.js

with a no-op setupActivityAbcParser() implementation required by the VM sandbox used in the test.

Added:

js/activity/__tests__/abc-parser.test.js

to provide coverage for:

  • single-voice parsing
  • multi-voice parsing
  • repeat handling
  • triplets
  • key signature accidentals

Defensive improvements

While extracting the parser, a small number of defensive checks were added around:

  • empty or incomplete voice structures
  • repeat block boundary handling

These guards prevent runtime failures caused by malformed or incomplete ABC data while preserving existing behavior for valid inputs.

Preserved behavior

The following remain unchanged:

  • Public API (this.parseABC(tune))
  • ABC import workflow
  • Generated Music Blocks structure for valid inputs
  • User-facing functionality
  • Existing Activity initialization flow

Result

This refactor improves separation of concerns by moving the ABC parser into its own module while maintaining compatibility with the existing ABC import pipeline and adding test coverage around parser behavior.
-[x] Documentation
-[x] Tests

@github-actions github-actions Bot added documentation Updates to docs, comments, or README size/XXL XXL: 1000+ lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Jun 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.36% | Branches: 40.05% | Functions: 53.02% | Lines: 48.74%
Master Coverage: Statements: 48.36% | Branches: 40.07% | Functions: 53.05% | Lines: 48.75%

@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

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.3% | Functions: 53.24% | Lines: 48.92%
Master Coverage: Statements: 48.36% | Branches: 40.06% | Functions: 53.04% | Lines: 48.75%

@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from 61710d9 to b1a4c6c Compare June 12, 2026 18:07
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.31% | Functions: 53.25% | Lines: 48.93%
Master Coverage: Statements: 48.36% | Branches: 40.06% | Functions: 53.04% | Lines: 48.75%

@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from b1a4c6c to eb6ae96 Compare June 12, 2026 18:26
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.3% | Functions: 53.25% | Lines: 48.92%
Master Coverage: Statements: 48.36% | Branches: 40.06% | Functions: 53.04% | Lines: 48.75%

@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from eb6ae96 to 7f52e67 Compare June 12, 2026 18:43
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.3% | Functions: 53.25% | Lines: 48.92%
Master Coverage: Statements: 48.36% | Branches: 40.06% | Functions: 53.04% | Lines: 48.75%

@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from 7f52e67 to db01350 Compare June 13, 2026 11:03
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.29% | Functions: 53.25% | Lines: 48.92%
Master Coverage: Statements: 49.01% | Branches: 40.47% | Functions: 53.17% | Lines: 49.42%

@github-actions github-actions Bot added the tests Adds or updates test coverage label Jun 13, 2026
@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from db01350 to b23d6ca Compare June 13, 2026 12:49
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.3% | Functions: 53.25% | Lines: 48.92%
Master Coverage: Statements: 49.12% | Branches: 40.56% | Functions: 53.32% | Lines: 49.54%

Move _adjustPitch, _abcToStandardValue, _createPitchBlocks,
_searchIndexForMusicBlock, and parseABC out of activity.js and into
a dedicated js/activity/abc-parser.js module.

The new module follows the same AMD + guarded module.exports pattern
as js/activity/recorder.js. setupActivityAbcParser(activityInstance)
attaches this.parseABC to the activity instance, preserving the
existing import workflow and public API without any UX changes.

loader.js is updated with a path alias and a dep entry for
activity/abc-parser so it is loaded before activity/activity.

The activity_startup_recovery test sandbox is updated with a no-op
setupActivityAbcParser so the vm slice of activity.js continues to
evaluate without a ReferenceError.

Additionally, add a unit test suite under js/activity/__tests__/abc-parser.test.js
covering key scenarios to guarantee exact behavioral preservation.

Verified: npm run lint (0 new errors), prettier --check (pass),
npm test (5449/5449 pass).
@vanshika2720 vanshika2720 force-pushed the refactor/activity-abc-parser branch from b23d6ca to 3670c86 Compare June 13, 2026 18:23
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

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

Coverage: Statements: 48.53% | Branches: 40.31% | Functions: 53.25% | Lines: 48.92%
Master Coverage: Statements: 49.12% | Branches: 40.56% | Functions: 53.32% | Lines: 49.54%

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 documentation Updates to docs, comments, or README size/XXL XXL: 1000+ lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

1 participant