fix(screencast): auto-save video when page closes without stop#41746
Open
Git-Raini wants to merge 2 commits into
Open
fix(screencast): auto-save video when page closes without stop#41746Git-Raini wants to merge 2 commits into
Git-Raini wants to merge 2 commits into
Conversation
When using page.screencast.start({ path }), if the page closes before
stop() is called, the video file was never saved. The server already
stops the recording gracefully, but the client never called
artifact.saveAs().
Listen for Events.Page.Close in the client Screencast constructor and
auto-save the artifact when the page closes while a recording is active.
Fixes microsoft#41608
Addresses review feedback: microsoft#41744 (comment) The auto-save triggered from the once(Events.Page.Close) listener was fire-and-forget, so page.close() could resolve before the video was actually written to disk - nothing guaranteed ordering, and a failed save was silently swallowed with no signal at all. Screencast now tracks the in-flight save as _pendingAutoSave, and Page.close() awaits it in a finally block. This is safe because the server dispatches the 'close' event (which triggers the auto-save) before it sends the response to the close() call itself, so the promise is always already assigned by the time page.close() reaches its finally block. Verified with 195 runs (13 tests x 15 repeats) of screencast.spec.ts, all passing, plus the full page-close.spec.ts suite (47 tests) with no regressions.
Contributor
Author
|
@microsoft-github-policy-service agree |
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
2 participants
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.
Closes #41608
This builds on @sarang-code2's #41744, which correctly diagnosed and fixed the original problem (video never saved if the page closes before
screencast.stop()is called). Full credit to them for that work - this PR carries their commit as-is, plus one addition:Original fix (unchanged from #41744): listen for
Events.Page.Closein the client-sideScreencastconstructor, and auto-save the artifact if a recording is active with apathoption.Added on top: in review on #41744, I flagged that the auto-save was fire-and-forget (
artifact.saveAs(savePath).catch(() => {}), never awaited) - sopage.close()could resolve before the video was actually written to disk, and a failed save produced no signal at all. This PR makesScreencasttrack the in-flight save (_pendingAutoSave) and hasPage.close()await it in afinallyblock. Verified safe by tracingPageDispatcher: the server dispatches the'close'event (which triggers the auto-save) before it sends the response to theclose()call, so the pending-save promise is always already assigned by the timeclose()reaches itsfinally.Full discussion: #41744 (comment) and sarang-code2#1 (opened against their branch first, before this one).
Test results - screencast.spec.ts, 195 runs (13 tests × 15 repeats), all passing
Test results - full page-close.spec.ts suite (18 tests), no regressions