Multistream Toggle 2 - #6167
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed correctness issues in mid-stream lifecycle handling (instance counting) and restream ingest setup error propagation need to be fixed before this is safe to ship.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes the “Live Output Editing” feature by wiring the Edit Stream window UI to per-display restream target management and adding recovery behavior for partial mid-stream target update failures in the streaming/restream services.
Changes:
- Refactors mid-stream updates in
StreamingServiceto support per-display target add/remove, failure recovery, and improved error propagation/details. - Updates
RestreamServiceto manage runtime targets per display/orientation and adds APIs to reconcile saved settings against live targets. - Reworks the Edit Stream window UI to expose destination/output toggles and a post-update cooldown flow, plus supporting view/state changes.
File summaries
| File | Description |
|---|---|
| app/services/streaming/streaming.ts | Live output editing update flow, new recovery paths, and adjusted streaming signal handling for per-display target changes. |
| app/services/streaming/streaming-view.ts | Feature gating + view logic updates for live output editing, dual output interactions, and common-field behavior. |
| app/services/restream.ts | Per-display ingest/target management, improved typed restream errors, and live-target reconciliation helpers. |
| app/components-react/windows/go-live/useGoLiveSettings.ts | Edit Stream state handling for immediate toggle persistence, restore-on-close, and post-failure resync. |
| app/components-react/windows/go-live/PlatformSettings.tsx | Renders Live Output Editing / Stream Shift cards and adjusts layout logic. |
| app/components-react/windows/go-live/GoLiveChecklist.tsx | Checklist rendering updated to reflect per-target start/stop/continue actions in Edit Stream mode. |
| app/components-react/windows/go-live/EditStreamWindow.tsx | New Edit Stream layout with destination switchers, checklist/settings switching, and cooldown timer UX. |
| app/components-react/windows/go-live/DestinationSwitchers.tsx | Adds update-mode disabling/alerts for certain toggles and supports externally disabled switchers. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The new reconciliation logic can leave saved Go Live settings incorrect after a failed attempt to stop targets, which risks misleading UI state and incorrect subsequent update decisions.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
app/services/streaming/streaming.ts:1129
syncTargetsToLiveonly queries/restores enabled platforms and destinations from the attempted settings update. If the user tried to stop a target and the stop failed, that target is disabled insettingsand won’t be included ingetLiveTargets, so it can’t be re-enabled to match what’s actually live (leaving saved settings incorrect after a failed stop).
app/components-react/windows/go-live/useGoLiveSettings.ts:795restoreTargets()re-implements the custom destination identity logic (${url}/${streamKey}) instead of usinggetDestinationId(), even though this module already usesgetDestinationId()elsewhere. Using the shared helper avoids drift if destination identity rules change and keeps matching consistent across the feature.
app/components-react/windows/go-live/EditStreamWindow.tsx:70
- The cooldown comment says “3-second countdown” but the logic sets a 10-second timer (
setTimer(10)) and the footer message also references 10 seconds. This is misleading for future maintenance.
useEffect(() => {
// 3-second countdown timer for cooldown after adding/removing targets
if (timer && timer > 0) {
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
BundleMonFiles updated (1)
Unchanged files (3)
Total files change +40.34KB +0.26% Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete correctness/i18n issues (including a TypeScript type mismatch in restoreTargets and non-localized user-facing restream error details) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app/services/streaming/streaming-view.ts:590
- The docstring for
getValidatedDisplaysays it readssavedLiveOutputEditing/ only applies when live output editing is enabled, but the implementation doesn’t reference that flag and is used unconditionally byshouldSetupDualOutput. This makes the comment misleading for future changes/debugging.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There are verified error-handling/message issues (silent per-display ingest failures via allSettled and duplicated RESTREAM error text) that should be corrected before merging.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
app/services/streaming/streaming.ts:1698
- When formatting RESTREAM errors, the code now iterates
StreamError.getModel()and appendstype,message,details, andplatformintomessages. Sincemessagesalready contains the caller-provided message anddetailsis separately rendered, this can produce noisy/duplicated user-facing error text (e.g., "Type: ...", "Message: ...", "Details: ..." repeating what was already shown).
Object.entries(model).forEach(([key, value]) => {
// Unset fields say nothing, and a function has no readable value
if (value == null || value === '' || typeof value === 'function') return;
const name = capitalize(key.replace(/([A-Z])/g, ' $1'));
// Never show the actual stream key and server url to the user for security purposes
if (['streamKey', 'serverUrl'].includes(key)) {
messages.push($t('Missing server url or stream key'));
} else {
messages.push(`${name}: ${value}`);
}
});
app/services/streaming/streaming-view.ts:592
- This JSDoc claims
getValidatedDisplay"ReadssavedLiveOutputEditing" but the method doesn't reference it; it only normalizesdisplayand checksdualOutputView.dualOutputMode. This is misleading documentation for future maintainers.
app/services/restream.ts:952
setupIngestusesPromise.allSettled(...)when writing per-display stream settings, which will silently ignore any rejectedfetchUserSettings/write and letbeforeGoLivecontinue with stale or incomplete settings. That can lead to confusing downstream failures (e.g.,createStreamingreading old values) without surfacing the original error.
// Await the settings for every display with `allSettled`. Otherwise `beforeGoLive` resolves before the
// stream settings have been written and `createStreaming` reads stale values.
await Promise.allSettled(
displays.map(async display => {
const mode = this.getMode(display);
const settings = await this.fetchUserSettings(mode);
this.setStreamSettingsForDisplay(display, settings.streamKey, ingest);
}),
);
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Wire Up Live Output Editing: Restream Targeting, Mid-Stream Update Recovery, and Edit Stream Window
This is PR 2 of 2, the follow-up to PR 6164 (
mw_multistream_toggle_final), and depends on it. PR 6164 added theLiveOutputEditingCard/StreamShiftCardcomponents, the granularRESTREAM_*error types, andgetDestinationId()/TDestinationIdas inert scaffolding — nothing in that PR rendered the cards, threw the new error types, or calledgetDestinationId. This PR is what actually consumes all three and makes Live Output Editing (editing stream destinations mid-stream from the Edit Stream window) functional.Summary
Three layers of the same feature land together because they're interdependent: the restream service needs to create/remove targets per-display instead of for the whole stream, the streaming service needs to recover cleanly when a mid-stream target update partially fails, and the Edit Stream window UI needs to actually expose per-target start/stop/continue controls instead of the old all-or-nothing settings form.
Changes
restream.ts— per-display target management.streaming.ts— mid-stream update recovery.streaming-view.ts— change for live output editing mode.useGoLiveSettings.ts— toggle state and recovery. Stream Shift and Live Output Editing are enforced as mutually exclusive in bothsetStreamShift()andprepopulate()(previously onlyisStreamShiftDisabledblocked one direction). NewrestoreTargets()reverts any target toggled in the Edit Stream window but never applied, called from the window'sdestroy()hook. NewsyncToLiveTargets()re-renders the switchers from the server-corrected saved settings after a failedupdateStream().getDestinationId()(from PR 6164) replaces ad hoc`${url}${streamKey}`concatenation everywhere destinations are matched against the active stream (isUpdatingTargets,isTargetLive,parseUpdateCustomDestinations).initialize()now callsRestreamService.actions.checkIsLive()on window open (Go Live window only) to detect an existing stream-shift session.Files changed:
app/components-react/windows/go-live/DestinationSwitchers.tsx,app/components-react/windows/go-live/EditStreamWindow.tsx,app/components-react/windows/go-live/GoLiveChecklist.tsx,app/components-react/windows/go-live/PlatformSettings.tsx,app/components-react/windows/go-live/useGoLiveSettings.ts,app/services/restream.ts,app/services/streaming/streaming-view.ts,app/services/streaming/streaming.tsPerformance Implications
Mostly neutral, with one deliberate behavior change and one net-new request path.
setupIngest()'s per-display loop switched from fire-and-forgetforEachtoawait Promise.allSettled(...)the old code letbeforeGoLiveresolve before stream settings were actually written. A failed mid-stream update now makes one extragetLiveTargetscall (fetches the restream target list) to reconcile local state. The Edit Stream window's new 10-second cooldown timer after an update is a deliberate UX throttle, not a network cost. No new polling was added —checkIsLive()is called once per Go Live window open, not on an interval.