fix(ui-components): defer object URL revoke in panel downloads - #1031
Merged
EdwardMoyse merged 1 commit intoSep 16, 2026
Merged
Conversation
`URL.createObjectURL` hands the browser a handle to the blob, and the anchor click that follows starts the download asynchronously. Revoking the URL in the same synchronous task can therefore tear the blob down before the browser has taken its own reference, cancelling the download. `saveFile` already defers its revoke to the next tick for exactly this reason, with a comment saying so. Three UI panels hand-roll the same download instead of calling it and revoke synchronously: - kinematics panel "Export TSV" - masterclass panel "Export results" - session pill ".phnxreplay" download The user clicks export and, depending on blob size and browser, either gets the file or silently gets nothing. Larger exports fail more often, since a bigger blob gives the browser a wider window in which to lose the race. PR HSF#992 fixed `saveFile` and stated these call sites "all revoke correctly". They do revoke, which is why they were passed over, but they revoke in the wrong task - so the leak was fixed there while this race stayed live here. Defer the revoke with `setTimeout` in all three, matching `saveFile`. The kinematics and masterclass anchors are also hidden and removed after the click, as `saveFile` does, so an unstyled anchor never lands in the layout. Added tests asserting the URL is not revoked synchronously and is revoked after the tick. The kinematics and masterclass "not revoked synchronously" cases fail before this change; the session-pill suite is new. Fixes HSF#1030 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🚀 Preview deployed: http://phoenix-pr-1031.surge.sh Built from 6f909a3. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1030
Problem
a.click()does not perform a download, it schedules one. The browser fetches theblob:URL afterwards, on its own timeline. Revoking the URL on the next line destroysthe handle immediately, so whether the export works is a race between the browser
starting its fetch and JavaScript tearing the blob down.
Three panels hand-roll the download and revoke synchronously:
kinematics-panel-overlaymasterclass-panel-overlaysession-pill.phnxreplaydownloadThe user clicks export and silently gets nothing — no file, no error, no console
message. Larger blobs lose the race more often, so long masterclass sessions and big
track collections are hit hardest.
Why this shape of fix
The repo already solved this once.
saveFiledefers its revoke to the next tick andcarries a comment explaining why (
helpers/file.ts#L18-L23). These three panels nevercall
saveFile; they reimplement it and reintroduce the bug it fixed.So the fix is to match the established pattern rather than invent one — defer the
revoke with
setTimeout, exactly assaveFiledoes. The URL is still revoked, sonothing leaks; it just happens once the browser holds its own reference.
The kinematics and masterclass anchors are additionally hidden (
display: none) and.remove()d after the click, again mirroringsaveFile, so an unstyled anchor neverlands in the layout.
session-pillalready appended and removed its anchor, so onlyits revoke changed.
I deliberately kept this to the revoke race and did not refactor the three call sites
onto
saveFileitself —saveFiletakes astringand these pass aBlob(session-pill's comes straight from
SessionManager), so unifying them is an APIchange that does not belong in a bug fix.
Relationship to #992
#992 fixed
saveFilefor this exact race and stated these call sites "all revokecorrectly". They do revoke, which is why they were passed over, but they revoke in
the wrong task. This is the other half of that fix, not a new regression.
Tests
Added coverage for all three components, asserting the user-visible contract: the URL
is not revoked synchronously, and is revoked after the tick.
kinematics-panel-overlay.component.test.ts(new)session-pill.component.test.ts(new)masterclass-panel-overlay.component.test.ts(newexportblock appended)Each suite also covers that no anchor is left in the document, and the masterclass and
session-pill suites cover the empty/no-blob early return.
Failing before this change: the "does not revoke the object URL synchronously"
case in the kinematics and masterclass suites. Verified by running the kinematics test
against unmodified code:
The
session-pillsuite is new, so it has no before-state.Components are instantiated via
Object.create(Cls.prototype)with the few fields theexport method touches, matching the existing
#923tests in the masterclass file andavoiding TestBed for methods that need no DI.
Verification
phoenix-ngsuite: 64 suites / 230 tests passed, 0 failures.eslintclean on all three component directories.prettier --checkwarns on these files, but it warns identically on a clean tree —the checkout is CRLF and prettier normalises to LF. Not caused by this PR, and left
alone to keep the diff to the bug.