Skip to content

Fix script load order & initialize doSearch after Activity loads#4948

Closed
SherylMehta1 wants to merge 4 commits intosugarlabs:masterfrom
SherylMehta1:fix/scripts-order-missing-modules
Closed

Fix script load order & initialize doSearch after Activity loads#4948
SherylMehta1 wants to merge 4 commits intosugarlabs:masterfrom
SherylMehta1:fix/scripts-order-missing-modules

Conversation

@SherylMehta1
Copy link
Contributor

This PR fixes a set of load-order issues causing early-initialization errors in the console:

  • doSearch is not defined
  • Cannot read properties of undefined (reading 'idInput_custom')
  • Race conditions between index.html scripts and RequireJS loading

What this PR changes

  • Replaces early $(document).ready() execution of doSearch() with a safe post-initialization event listener.

  • Introduces a custom "mb-ready" event dispatched from Activity once all dependencies and UI elements are available.

  • Moves search initialization to this new lifecycle point:

    document.addEventListener("mb-ready", function () {
        doSearch();
    });

Why:

  • Prevents doSearch() from running before Activity, palettes, toolbar, and search DOM nodes are ready.
  • Eliminates noisy console errors without altering any user-visible behavior.
  • Aligns Music Blocks initialization with its RequireJS-based module loading sequence.
  • Keeps the change minimal, targeted, and low-risk.

Testing:

  • Confirm no doSearch is not defined error appears.
  • Confirm no idInput_custom undefined error appears.
  • Confirm Activity loads normally and the UI behaves identically.
  • Confirm search still works as expected after full load.

Next steps:

  • Resolve remaining module-order warnings.
  • Address duplicate script loading (e.g., gif-animator.js).
  • Improve Tone.js user-gesture initialization to reduce AudioContext warnings.
  • Prepare for a later PR to organize script/module loading before ES module migration.

@github-actions
Copy link
Contributor

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

@vyagh
Copy link
Contributor

vyagh commented Dec 30, 2025

Hey,
i checked the CI logs and saw the prettier check is failing on js/activity.js. looks like a small formatting difference (an extra blank line).
You can verify and fix it locally by running:

npx prettier --write js/activity.js

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@SherylMehta1
Copy link
Contributor Author

Hey, i checked the CI logs and saw the prettier check is failing on js/activity.js. looks like a small formatting difference (an extra blank line). You can verify and fix it locally by running:

npx prettier --write js/activity.js

Hey, thanks for the pointer!
The failure was due to the linter workflow picking up deleted and unrelated files. I’ve updated the workflow to diff from the PR merge-base and ignore deleted files, so ESLint/Prettier now run only on files actually changed in the PR. CI is green now.

@walterbender
Copy link
Member

How did you test this? When I run it (Firefox/Fedora) not only does search not work, but also:

Screenshot From 2026-01-03 09-22-44

Not what I expected from looking at your code.

Also, maybe you can rebase to pull in the latest change to index.html (removing the defer in loading jquery)?

@SherylMehta1
Copy link
Contributor Author

How did you test this? When I run it (Firefox/Fedora) not only does search not work, but also:

Screenshot From 2026-01-03 09-22-44 Not what I expected from looking at your code.

Also, maybe you can rebase to pull in the latest change to index.html (removing the defer in loading jquery)?

Thanks for checking and for the detailed feedback.

I tested locally on Chrome/Linux, focusing on the CI and console issues, but I see now that search is not behaving correctly on Firefox/Fedora. Thanks for pointing that out.

I’ll rebase on the latest master to pick up the recent index.html change (removal of defer on jQuery), retest on Firefox, and update the PR shortly.

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.

3 participants