Repository navigation
feat(webapp): report WebM recording failures after start through onError - #2181
irvingouj@Devolutions (irvingoujAtDevolution) merged 3 commits into
Conversation
Once start() resolves, the WebM recorder had no way to tell its caller that recording stopped. A push that failed or closed mid-recording, or a MediaRecorder error, ended the capture silently, so the session went on unrecorded even when recording was required. WebMRecorderOptions gets an optional onError hook. It's called with 'ConnectionToTheRecordingServerLost' when the push WebSocket fails or closes, and 'MediaRecorderFailed' when the browser's recorder fails. Failures before start() resolves still reject the start promise. Issue: HB-9197 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
🟡 Changes recommended
A teardown-flushed chunk can incorrectly resolve startup after an early socket or recorder failure, triggering onError instead of rejecting start().
1 open issue
What changed in this PR
Adds an optional error callback to WebMRecorder so consumers can respond when recording fails after startup, including ending sessions where recording is required.
Changes:
- Reports post-start connection and recorder failures through
onError. - Distinguishes runtime recorder failures from startup errors.
- Catches exceptions thrown by the consumer’s error callback.
| File | Description |
|---|---|
webapp/packages/web-recorder/src/webm-recorder.ts |
Adds the onError option and post-start failure notifications. |
Review effort: Balanced
…ailure MediaRecorder.stop() fires a last dataavailable before stop. When the push or the recorder failed before the first chunk, that flushed chunk resolved start() and the failure then went to onError, instead of rejecting start(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Krista House (kristahouse)
left a comment
There was a problem hiding this comment.
The main paths look right: a push failure after start calls onError once (an error followed by a close is caught by the isCleaningUp guard), stop() still closes with 4011 without calling it, failures before start still reject, and calling start() again from inside onError is safe because fireError runs after cleanupResources(). The second commit fixes Copilot's comment about the flushed chunk.
One medium finding before merge, see the inline comments: a recorder that stops on its own (for example when its tracks end) is treated as a user stop(). It closes with 4011 and never calls onError, so the gap this PR closes stays open in that case. The fix touches two places, because the chunk flushed on the way out can resolve start() first.
Optional / follow-ups:
onError?: (error: string)could take aWebMRecorderErrorstring union exported fromindex.ts, likeWebMRecorderTelemetryEvent. That gives compile-time checking of the literals; runtime constants would need anas constobject.failedAfterStartinfers "after start" fromrejectStart === null;this._isRecordingread before cleanup gives the same answer and says what it means.AsciiCastV2Recorderhas the same gap (settledoes nothing once start has settled). That's a separate PR, if required recording also covers terminal sessions.- The fake WebSocket/MediaRecorder script from the description would be worth committing. The most useful cases: a spontaneous stop before and after the first chunk, and the final chunk arriving before
stop.
MediaRecorder stops by itself when all its tracks end. That stop came in like a stop() call: the push closed with 4011 and onError never ran. Every stop we ask for sets isCleaningUp first, so a stop without it is now a failure: 'MediaRecorderFailed' after start, a rejected start() before. A chunk now confirms the start only while the recorder is recording. The spec makes the recorder inactive before the chunk flushed by any stop or error, so none of those can resolve start(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Good catch on the spontaneous stop. Fixed in f4ca78c with both of your changes: a stop without Reran the fake script with tracks ending before and after the first chunk, errors in spec order, and Not in this PR:
|
Krista House (kristahouse)
left a comment
There was a problem hiding this comment.
LGTM! Thank you :)
a3264f3
into
master
Bumps `@devolutions/web-recorder` to 0.3.0 so the change from #2181 gets published. `WebMRecorderOptions` gains an optional `onError`, called when a WebM recording fails after `start()` resolved. Hub's switch to the package (Devolutions/RDM#20749) needs it to end a session whose recording is required. Issue: HB-9197 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Once
start()resolves,WebMRecordercan't tell the caller anything anymore. If the push drops or closes mid-recording, or MediaRecorder errors, capture just stops and the session keeps going unrecorded, even when recording is required.This adds an optional
onErrortoWebMRecorderOptions. It's called with'ConnectionToTheRecordingServerLost'when the push WebSocket fails or closes, or'MediaRecorderFailed'when the browser's recorder fails or stops on its own (all tracks ended). Failures beforestart()resolves still reject the start promise like before.Hub needs this to end a session whose recording is required (HB-9197). The version bump comes in a separate PR.
Tested with a small script that fakes WebSocket and MediaRecorder: clean close, WebSocket error and MediaRecorder error after the first chunk each call
onErroronce.stop()still closes with 4011 and doesn't call it. Failures before the first chunk still rejectstart(), even when MediaRecorder flushes a last chunk on its way out. Tracks ending on their own count as a failure the same way, before and after the first chunk.Issue: HB-9197
🤖 Generated with Claude Code