fix(UI): prevent startup spinner stall when external model resolution hangs#860
fix(UI): prevent startup spinner stall when external model resolution hangs#860divyansh-cyber wants to merge 1 commit intoaccordproject:mainfrom
Conversation
…ion stall Adds timeout guards to startup initialization and external model resolution, and limits external resolution to models that actually declare imports. This prevents the app from remaining on loading spinner indefinitely under degraded network conditions. Signed-off-by: Divyansh Rai <140232173+divyansh-cyber@users.noreply.github.com>
✅ Deploy Preview for ap-template-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a startup reliability issue in the Template Playground where the app could remain stuck on the initial loading spinner if external Concerto model resolution stalls, ensuring the editor UI becomes reachable even in degraded/offline conditions.
Changes:
- Added a timeout wrapper around
ModelManager.updateExternalModels()and skipped external resolution when the CTO model has noimportstatements. - Added an app initialization timeout guard so the startup spinner cannot block indefinitely.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/store/store.ts |
Adds timeout + conditional external model resolution during rebuild to prevent rebuild from hanging on remote imports. |
src/App.tsx |
Adds initialization timeout guard around init() / loadFromLink() to ensure loading spinner always exits. |
Comments suppressed due to low confidence (1)
src/App.tsx:79
- When initialization times out, the error is only logged to the console and
loadingis set to false. That leaves users with a blank/partially initialized editor (e.g., empty preview) without any visible indication of what happened. Consider surfacing the timeout failure in the UI (e.g., set storeerror+ open the problem panel, or show an AntDmessage.error) so degraded/offline startup has a clear user-facing failure path.
} catch (error) {
console.error("Initialization error:", error);
} finally {
setLoading(false);
}
| await withTimeout( | ||
| loadFromLink(compressedData), | ||
| APP_INIT_TIMEOUT_MS, | ||
| `Initialization timed out after ${APP_INIT_TIMEOUT_MS}ms while loading shared data.` | ||
| ); | ||
| if (window.location.pathname !== "/") { | ||
| navigate("/", { replace: true }); | ||
| } | ||
| } else { | ||
| await init(); | ||
| await withTimeout( | ||
| init(), | ||
| APP_INIT_TIMEOUT_MS, | ||
| `Initialization timed out after ${APP_INIT_TIMEOUT_MS}ms.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new initialization timeout behavior is user-facing (spinner should stop, and a failure path should be shown). There are existing component/store unit tests in the repo, but no coverage here to ensure the spinner reliably exits and the app renders the shell when init()/loadFromLink() hangs. Please add a unit test (Vitest + fake timers) that forces a never-resolving init promise and asserts loading state transitions / error surfacing.
| const EXTERNAL_MODEL_RESOLUTION_TIMEOUT_MS = 10000; | ||
|
|
||
| function withTimeout<T>(promise: Promise<T>, ms: number, message: string): Promise<T> { | ||
| return new Promise<T>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error(message)), ms); | ||
| promise | ||
| .then((value) => { | ||
| clearTimeout(timer); | ||
| resolve(value); | ||
| }) | ||
| .catch((error: unknown) => { | ||
| clearTimeout(timer); | ||
| reject(error); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function hasExternalImports(model: string): boolean { | ||
| return /^\s*import\s+/m.test(model); | ||
| } | ||
|
|
||
| async function rebuild(template: string, model: string, dataString: string): Promise<string> { | ||
| // Validate inputs before expensive operations | ||
| // This fails fast on invalid JSON or CTO syntax without running network calls | ||
| await validateBeforeRebuild(template, model, dataString); | ||
|
|
||
| const modelManager = new ModelManager({ strict: true }); | ||
| modelManager.addCTOModel(model, undefined, true); | ||
| await modelManager.updateExternalModels(); | ||
| const engine = new TemplateMarkInterpreter(modelManager, {}); | ||
| if (hasExternalImports(model)) { | ||
| await withTimeout( | ||
| modelManager.updateExternalModels(), | ||
| EXTERNAL_MODEL_RESOLUTION_TIMEOUT_MS, | ||
| `External model resolution timed out after ${EXTERNAL_MODEL_RESOLUTION_TIMEOUT_MS}ms. Check connectivity or remove remote imports.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The external model resolution timeout and the new hasExternalImports(model) gate are important behavior changes in the rebuild pipeline, but there’s no unit coverage to prevent regressions. Please add store-level tests that (1) verify updateExternalModels() is not called when the model has no imports, and (2) verify a hanging updateExternalModels() causes rebuild to fail within the timeout and sets the store error / shows the problem panel.
| try { | ||
| await Promise.race([promise, timeoutPromise]); | ||
| } finally { | ||
| if (timeoutHandle) { |
There was a problem hiding this comment.
withTimeout clears the timer only when timeoutHandle is truthy. In browsers (and especially in some test environments), setTimeout can return 0, which would skip the cleanup and leave the timer running. Use an explicit timeoutHandle !== undefined check (or initialize to null) so the timeout is always cleared when set.
| if (timeoutHandle) { | |
| if (timeoutHandle !== undefined) { |
|
This PR is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Fixes: #859
This PR fixes a startup reliability issue where the app could remain on the loading spinner for a long time (or indefinitely) when external model resolution stalls.
Problem:
Initialization awaited rebuild, and rebuild always awaited external model resolution with no timeout/fallback path. In degraded/offline environments this could block app startup and delay access to the editor UI.
What changed:
Expected behavior after this PR:
Scope:
Author Checklist