fix #604: load config before the first MoQ derive on cold load#614
Conversation
On a cold load of the Stream view, listDynamicSamples() can resolve before loadConfig() populates configServerUrl. The first derive of a gateway_path-only pipeline then resolves with an empty configServerUrl, so resolveServerUrl() returns undefined, setServerUrl is skipped, and the server URL field stays blank until the user manually edits the client section. useMoqYamlSync now re-resolves the server URL from the last derived settings when configServerUrl transitions from empty to set, so the field populates without manual interaction. Refs #604 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
========================================
Coverage 84.97% 84.97%
========================================
Files 247 248 +1
Lines 74605 74624 +19
Branches 2378 2508 +130
========================================
+ Hits 63393 63411 +18
- Misses 11207 11208 +1
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.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…shot Address review on #604 fix: - When configServerUrl loads while a YAML edit is mid-debounce, re-resolve from the in-flight YAML instead of the stale lastSettingsRef, avoiding a transient wrong server URL. - Make the re-resolve fire exactly once on first config load so a later reset of configServerUrl can't clobber a manually edited server URL. Refs #604 Signed-off-by: streamkit-devin <devin@streamkit.dev>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolve the cold-load blank server URL (#604) at its source: the Stream view now starts the config load alongside the sample load and awaits it before the first derive, so resolveServerUrl always sees a populated configServerUrl. This removes the reactive re-resolve effect and its coordinating refs (lastSettingsRef/prevConfigServerUrlRef/ configLoadHandledRef) from useMoqYamlSync. Signed-off-by: streamkit-devin <devin@streamkit.dev>
staging-devin-ai-integration
Bot
changed the title
| useEffect(() => { | ||
| loadAndApplySamples(viewState, deriveMoqFromYaml); | ||
| const configReady = configLoaded ? Promise.resolve() : loadConfig(); | ||
| void loadAndApplySamples(viewState, deriveMoqFromYaml, configReady); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
📝 Info: Behavioral change: loadConfig no longer retries on re-render
The old code called loadConfig in a separate useEffect with [configLoaded, loadConfig] dependencies, meaning it would re-fire on any re-render where configLoaded was still false. The new code at StreamView.tsx:655 calls loadConfig() only once on mount (inside a []-deps effect). In practice this is equivalent because loadConfig always sets configLoaded: true (even on error, per ui/src/stores/streamStore.ts:259), so the old effect would never re-fire either. The new approach is actually cleaner since it avoids the theoretical risk of concurrent loadConfig calls if a re-render happened before the first call completed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Move the cold-load config sequencing into loadAndApplySamples so all of the ordering logic is exercised by the unit test, lifting patch coverage. The component effect reverts to a single call into the function. Refs #604 Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const { configLoaded, loadConfig } = useStreamStore.getState(); | ||
| const configReady = configLoaded ? Promise.resolve() : loadConfig(); |
There was a problem hiding this comment.
📝 Info: No deduplication guard on concurrent loadConfig calls across views
The loadConfig function in ui/src/stores/streamStore.ts:240-264 has no deduplication logic (no in-flight promise caching). If a user navigates quickly between StreamView and MonitorView, both loadAndApplySamples (ui/src/views/streamSamples.ts:32) and useMonitorPreview (ui/src/hooks/useMonitorPreview.ts:246) could call loadConfig() concurrently, resulting in duplicate fetchConfig() network requests. This is pre-existing and harmless (the second call overwrites with the same values), but could be improved with a simple promise-caching pattern if config fetches become expensive.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…Samples Apply selection and derive synchronously after config is awaited so a user template switch during config load is not clobbered by the auto-select, and align the test mock's selectedTemplateId with the production empty-string initial value. Adds a clobber regression test. Refs #604 Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const { configLoaded, loadConfig } = useStreamStore.getState(); | ||
| try { | ||
| viewState.setSamplesLoading(true); | ||
| viewState.setSamplesError(null); | ||
| const samples = await listDynamicSamples(); | ||
| const orderedSamples = orderSamplePipelinesSystemFirst(samples); | ||
| const samplesPromise = listDynamicSamples(); | ||
| if (!configLoaded) await loadConfig(); |
There was a problem hiding this comment.
📝 Info: Config loading is now exclusively triggered by sample loading in StreamView
The old code had a standalone useEffect that called loadConfig() independently of sample loading. The new code only triggers loadConfig() inside loadAndApplySamples (ui/src/views/streamSamples.ts:32). This means config loading in the StreamView context is now tightly coupled to sample loading — if loadAndApplySamples were ever skipped or short-circuited before the Promise.all, config would never load for this view. Currently this isn't an issue since the function always reaches the Promise.all, but it's a coupling worth being aware of. The useMonitorPreview hook (ui/src/hooks/useMonitorPreview.ts:245-246) independently calls loadConfig, so the Monitor view is unaffected.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Move the cold-load sample/config sequencing out of StreamView into a standalone streamSamples module so its unit test imports only the helper instead of the full view (restores codecov/project/ui, which dropped when the test pulled the heavy StreamView tree into v8 coverage). Fetch config and samples with Promise.all so a sample-fetch rejection during the config await no longer surfaces as an unhandled rejection, while keeping config loaded before the first MoQ derive (#604). Drop the clobber test: it mutated a plain object, which does not reflect React state updates, and the scenario it claimed to guard cannot occur (the template dropdown is empty until after the config await). Add an error-path test covering setSamplesError/setSamplesLoading instead. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| const [, samples] = await Promise.all([ | ||
| configLoaded ? Promise.resolve() : loadConfig(), | ||
| listDynamicSamples(), | ||
| ]); | ||
| const orderedSamples = orderSamplePipelinesSystemFirst(samples); | ||
| viewState.setSamples(orderedSamples); | ||
|
|
||
| if (orderedSamples.length > 0 && !viewState.selectedTemplateId) { | ||
| const first = orderedSamples[0]; | ||
| viewState.setSelectedTemplateId(first.id); | ||
| viewState.setPipelineYaml(first.yaml); | ||
| deriveMoqFromYaml(first.yaml); |
There was a problem hiding this comment.
📝 Info: Config failure is silently absorbed by Promise.all — by design
Since loadConfig() (ui/src/stores/streamStore.ts:240-264) has an internal try/catch and never rejects, a config fetch failure won't cause the Promise.all at ui/src/views/streamSamples.ts:31-34 to reject. Instead, configLoaded is set to true with an empty configServerUrl and an errorMessage in the store. The deriveMoqFromYaml call at line 42 will then resolve the server URL against an empty configServerUrl, resulting in no URL being set (resolveServerUrl at ui/src/utils/moqPeerSettings.ts:158 returns undefined when configServerUrl is empty). The user sees the store's error message and can manually enter a URL. This is correct behavior but worth understanding — a config failure doesn't block sample display.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
listDynamicSamples()can resolve before the separate asyncloadConfig()populatesconfigServerUrl, so the first derive of agateway_path-only pipeline runs withconfigServerUrl='',resolveServerUrl()returnsundefined, andsetServerUrlis skipped.awaits it before the first derive, soresolveServerUrlalways sees a populatedconfigServerUrl. Config and samples still fetch in parallel — only the derive waits.lastSettingsRef/prevConfigServerUrlRef/configLoadHandledRef) fromuseMoqYamlSync, removing the synchronous-shadow-of-async-state pattern flagged in review.Review & Validation
await configReadyonly gates the derive, not the sample fetch), so the sample list isn't slowed by config latency.loadConfig()never rejects (it setsconfigLoaded+ an error message), soconfigReadyalways resolves and the derive proceeds (URL simply stays empty for manual entry).just lintandjust test-uipass. NewStreamView.loadSamples.test.tsreproduces the cold-load ordering (sample resolves first) and asserts the derive is deferred until config is ready.Notes
Supersedes the previous re-resolve-effect approach in this PR after review feedback: rather than patching the late
configServerUrlreactively in the hook, the load order is fixed inStreamViewso the first derive is always correct.loadAndApplySamplesis now exported for unit testing the ordering guarantee.Link to Devin session: https://staging.itsdev.in/sessions/cbcff0155010454caacdde19106f1b20
Requested by: @streamer45
Devin Review
76d1979