fix: guard against null popup window in PhraseMaker export

[rish106-hub]

Description

PhraseMaker._export() (js/widgets/phrasemaker.js) opens a new browser window/tab and renders the current Music Matrix into it as a formatted, downloadable HTML table. It does this with:

const exportWindow = window.open("");
const exportDocument = exportWindow.document;

window.open() returns null whenever the browser's popup blocker prevents the window from opening — this is standard, spec-defined behavior, not an edge case. When that happens, exportWindow.document throws immediately:

TypeError: Cannot read properties of null (reading 'document')

There was already an attempt at a guard for this:

if (exportDocument === undefined) {
    console.debug("Could not create export window");
    return;
}

but it's dead code: it checks the wrong variable (exportDocument, which is a derived property of the thing that's actually null) against the wrong falsy value (undefined instead of null), and the check runs one statement after the line that already threw. In practice the guard could never fire before the crash happened.

Why it matters: any user with popup blocking enabled (the browser default in most cases, and standard in locked-down school/classroom Chromebook profiles that Music Blocks is commonly deployed on) hits an uncaught exception the moment they try to export their Music Matrix, with no explanation and no way to recover short of reloading.

Related Issue

This PR fixes #7082

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

js/widgets/phrasemaker.js — _export():

• Moved the null-check to immediately after window.open(""), before any property is read off the result
• Check the actual returned value (if (!exportWindow)) instead of a property already derived from it
• Removed the old exportDocument === undefined check — it's unreachable/redundant once exportWindow is confirmed truthy (a real Window object always has a .document)
• Added a user-facing warning via this.activity.errorMsg(...) when the popup is blocked, so the user gets told why the export didn't happen instead of it failing silently (previously it only logged to console.debug, invisible to anyone but a developer with devtools open)
• The warning string is routed through this._(...) for i18n, matching every other user-facing string already in this widget (this._("Play"), this._("Export"), etc. — see line 84 where this._ is bound from the injected deps._)
• this.activity.errorMsg is the established notification pattern already used elsewhere in this same file (passed as a callback at lines 773/785) and across other widgets (reflection.js, legobricks.js) — no new utility introduced, just reusing what's already there

Before:

_export() {
    const exportWindow = window.open("");
    const exportDocument = exportWindow.document;
    if (exportDocument === undefined) {
        console.debug("Could not create export window");
        return;
    }
    // ...

After:

_export() {
    const exportWindow = window.open("");
    if (!exportWindow) {
        console.debug("Could not create export window");
        this.activity.errorMsg(
            this._("Could not open export window. Please allow pop-ups for this site.")
        );
        return;
    }
    const exportDocument = exportWindow.document;
    // ...

Nothing else in the 188-line method was touched — the table/row/cell generation and download-link logic downstream of the guard is unrelated to this bug and works correctly once the guard fires (or doesn't) at the right time.

js/widgets/__tests__/phrasemaker.test.js — new describe("_export", ...) block (this method had no prior test coverage):

• Blocked popup: stubs window.open to return null, asserts _export() does not throw and that activity.errorMsg is called exactly once with a message mentioning the export window
• Successful popup: stubs window.open to return a mock window/document, asserts _export() runs to completion (including building the table and closing the document) without ever calling errorMsg

Reproduction (before the fix)

1. Open Music Blocks in a browser with popup blocking enabled (default in most browsers for programmatic window.open calls not tied to a direct user gesture, and standard on locked-down classroom Chromebook profiles)
2. Open the PhraseMaker (Music Matrix) widget
3. Trigger the Music Matrix export action
4. Observe: uncaught TypeError in the console, export silently fails, no feedback to the user

After the fix, step 4 instead shows a visible warning message telling the user to allow pop-ups, and the app continues to function normally.

Testing Performed

• npx jest js/widgets/__tests__/phrasemaker.test.js — 71/71 pass (69 pre-existing + 2 new)
• npx jest (full repo suite) — 6077/6077 pass, zero regressions
• npx eslint js/widgets/phrasemaker.js js/widgets/__tests__/phrasemaker.test.js — clean, zero warnings/errors
• Manually traced the fixed code path against the issue's exact reproduction steps and confirmed the guard now fires before any dereference, matching the failure mode described in the issue

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 pnpm run lint and pnpm exec 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".

Additional Notes for Reviewers

Kept this surgical and single-topic per the contribution guidelines — only the guard clause and the new warning call changed, no drive-by refactoring of the rest of _export() or adjacent methods. window.open() isn't used anywhere else in the codebase, so there was no existing null-check convention to match; the errorMsg/this._() pattern chosen here is the one already used consistently throughout this file and the wider widget codebase.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.66%. Comparing base (d09973a) to head (8d476da).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7721      +/-   ##
==========================================
+ Coverage   54.60%   54.66%   +0.06%     
==========================================
  Files         172      172              
  Lines       57270    57271       +1     
==========================================
+ Hits        31272    31309      +37     
+ Misses      25998    25962      -36     

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

[walterbender]

A guard makes sense and also it is good to have a user-facing error message.
A few questions/comments:

1. Do we know for sure that the reason for the failure is an ad blocker? If not, maybe the language of the message is misleading. Perhaps suggest trying again with ad blocker disabled for this site
2. I'm not sure of the mechanism, but for me, when I use FF with ad blocking enabled, the export button does not even appear.
3. We are in string freeze so regardless of the decision re Change icons folderlinks for apache #1 above, we cannot merge this change until after the next release. Hopefully within the next few days.

[rish106-hub]

Thanks for the review @walterbender sir !

1. Agreed the wording shouldn't presume a specific tool , it's meant to cover popup-blocking generally (native browser setting or extension), not specifically "ad blocker." Happy to adjust the copy if you'd prefer different phrasing.

2. Sir I traced this and it looks like a separate bug: the Export button is gated behind if (!localStorage.beginnerMode) (phrasemaker.js:490), and under restricted-storage browser modes that localStorage read can throw, which would prevent the button from ever being created. That matches the systemic pattern already tracked in #7074.

Want me to file/reference that specifically, or fold a guard for this callsite into that issue's scope? Either way it's separate from this PR, which only handles the crash once the button is clicked and the popup gets blocked.

3. Understood on the freeze - happy to wait, just flag me when it's safe to rebase/merge.

[walterbender]

I misspoke when I said "ad blocker". I meant "pop-up blocker". But the point is the same -- it is a likely cause of the failure, but not the exclusive cause of the failure, so some rewording is needed.

Re #7074 that is a good point. We should prob. fix that sooner than later. There is an effort to migrate to indexDB, so maybe #7074 can be handled in that context: #7678

[rish106-hub]

Reworded @walterbender sir . it no longer states pop-up blocking as the definite cause, just flags it as a likely one and points the user to check settings. Pushed. Will sit ready for whenever the freeze lifts.