Skip to content

fix: clean up plugin setup script tags#7750

Merged
walterbender merged 2 commits into
sugarlabs:masterfrom
rish106-hub:fix/7388-plugin-script-cleanup
Jul 7, 2026
Merged

fix: clean up plugin setup script tags#7750
walterbender merged 2 commits into
sugarlabs:masterfrom
rish106-hub:fix/7388-plugin-script-cleanup

Conversation

@rish106-hub

Copy link
Copy Markdown
Contributor

Description

This PR fixes a remaining plugin loader cleanup path where temporary setup
<script> elements are appended to document.head but not removed after they
finish loading or fail to load.

The main Blob-based plugin registry script already cleans itself up on the
current branch. However, setup scripts created later for plugin initialization
logic, such as BLOCKPLUGINS, GLOBALS, and ONLOAD, still remained in the DOM
after execution.

The first-principle issue is that these setup scripts are temporary transport
mechanisms for executing Blob-backed plugin setup code. Once a setup script has
loaded or failed, the script element is no longer needed. Keeping it in
document.head causes repeated plugin loads to accumulate stale DOM nodes during
long-running sessions.

Related Issue

This PR fixes #7388

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

  • Updated js/utils/utils.js so temporary plugin setup script elements are removed from document.head after successful load.
  • Updated the same setup script error path so failed setup script loads are also removed from document.head.
  • Kept the existing URL.revokeObjectURL(...) behavior intact so Blob URLs are still released.
  • Exported processPluginData through the existing guarded CommonJS export path so the plugin loader behavior can be tested in Jest without changing browser loading behavior.
  • Added focused Jest coverage in js/utils/__tests__/utils-plugin-loader.test.js for:
    • setup script cleanup after onload
    • setup script cleanup after onerror
    • Blob URL revocation in both paths

Testing Performed

  • npx jest js/utils/__tests__/utils-plugin-loader.test.js --runInBand --coverage=false
    • 2 tests passed
  • npm run lint
    • Passed with existing repository warnings
  • npx prettier --check js/utils/utils.js js/utils/__tests__/utils-plugin-loader.test.js
    • Passed
  • npm test
    • 179 test suites passed
    • 6077 tests passed

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.
  • 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" (required for auto-rebase; this only affects the PR branch, not your fork).

Additional Notes for Reviewers

This PR is intentionally limited to issue #7388. It does not change plugin
execution semantics; it only removes temporary setup script elements after their
load lifecycle has completed.

The browser-facing behavior remains the same: plugin setup code still runs from
Blob-backed script elements, Blob URLs are still revoked, and setup load errors
still resolve so later setup blocks can continue.

@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 Jul 6, 2026
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.70%. Comparing base (a138477) to head (c85c089).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7750      +/-   ##
==========================================
+ Coverage   56.60%   56.70%   +0.10%     
==========================================
  Files         172      172              
  Lines       57675    57679       +4     
==========================================
+ Hits        32645    32706      +61     
+ Misses      25030    24973      -57     

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

Remove temporary plugin setup script elements from document.head after load

or load failure so repeated plugin loads do not leave stale DOM nodes

behind.

Related to sugarlabs#7388
@rish106-hub rish106-hub force-pushed the fix/7388-plugin-script-cleanup branch from e94055f to a617346 Compare July 6, 2026 15:08
@walterbender

Copy link
Copy Markdown
Member

Question: did you run the code to ensure that plugins still load?

@rish106-hub

Copy link
Copy Markdown
Contributor Author

Confirmed locally that plugins still load. I browser-tested the real built-in maths plugin through loadBuiltInPluginFromXHR("maths"); the palette and plugin blocks loaded, arg handlers were registered as functions, the plugin registry was cleaned, and no Blob script tags were left behind.

I also strengthened the Jest coverage so the setup Blob script is actually executed before cleanup is asserted.

@walterbender walterbender merged commit cc0319d into sugarlabs:master Jul 7, 2026
13 checks passed
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/M Medium: 50-249 lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

Plugin loader keeps injected <script> tags in document.head after execution, causing unnecessary DOM and memory growth during long sessions

2 participants