FIX: distinguish catalog-unavailable from unregistered in Initializers page - #2464
FIX: distinguish catalog-unavailable from unregistered in Initializers page#2464fei (feiiiiii5) wants to merge 4 commits into
Conversation
| initializer={resolveRegisteredInitializer( | ||
| item.initializer_name, | ||
| registeredInitializers, | ||
| catalogUnavailable, |
There was a problem hiding this comment.
When the catalog request fails, this placeholder has no parameter schema, but Edit remains enabled. Clicking Save then replaces existing parameters, such as { mode: 'strict' }, with null; please disable editing until the catalog reloads or preserve the existing parameters while the schema is unavailable.
There was a problem hiding this comment.
Addressed in ddf816c (took the disable-until-reload option): Edit is disabled while catalogAvailable is false, the card states "Initializer catalog is unavailable; editing is disabled until it reloads." inline, and Apply/Remove stay enabled since they only operate on stored parameters — so a catalog outage can no longer null out existing parameters like {"mode": "strict"}. Covered by two new tests: Edit disabled + inline explanation while unavailable, and Apply/Remove remaining usable.
There was a problem hiding this comment.
|
You may need to push your fix commit :-) |
|
You were right — the commit referenced above never made it to the remote. Apologies for that. It is now pushed as 5796799 (re-done on top of the branch): Edit is disabled while |
| <BaselineInitializers | ||
| items={settings.baseline} | ||
| registeredInitializers={registeredInitializers} | ||
| catalogUnavailable={catalogUnavailable} |
There was a problem hiding this comment.
Could we pass this unavailable state to AvailableInitializersDialog as well, or disable its trigger when the catalog request fails? On an initial failure, the dialog receives an empty array and says "No registered initializers were found," which still presents a temporary outage as a definitive empty catalog.
There was a problem hiding this comment.
Addressed in d291d77: AvailableInitializersDialog now receives catalogUnavailable and reports that the catalog cannot be shown until it loads, instead of "No registered initializers were found". The trigger stays enabled since the dialog is a read-only reference; the empty-registry message is unchanged for a successful fetch that registers nothing.
|
Done in d291d77 — took the first option: |
|
Follow-up: 20a5d7c adopts the status-union design suggested in review and removes the synthetic placeholder entries. (The suggestion arrived on #2469 — a duplicate PR I opened by mistake, now closed in favor of this one. Apologies for the noise.)
Full Initializers suite (78 tests), |
| setRegisteredInitializers(registeredResult.value.items) | ||
| setCatalogStatus('loaded') | ||
| } else { | ||
| setCatalogStatus('error') |
There was a problem hiding this comment.
If a refresh fails after a successful load, registeredInitializers keeps the old entries while catalogStatus becomes error. That leaves stale metadata visible and the Add controls enabled even though catalog-dependent UI should be unavailable; could we clear the array here or make consumers ignore it unless the status is loaded, with a regression test for success followed by refresh failure?
There was a problem hiding this comment.
Addressed in 749e65e (merged main as acf7bb0 so this sits on current main).
Took the first option — the failure branch now drops the catalog instead of keeping the last good list:
} else {
// Drop the previous catalog: entries left behind would keep rendering stale
// descriptions and env vars, and would keep Add enabled off the first stale name.
setRegisteredInitializers([])
setCatalogStatus('error')Both symptoms you named follow from that one change, with no new gating logic:
- Stale metadata: every consumer resolves entries through
findRegisteredInitializer, so an empty list makes the baseline/additional rows fall back toinitializerFallbackDescription('error')→ "Catalog metadata temporarily unavailable.", and theRequired env vars:line stops rendering (it is inside{initializer && ...}). - Add controls: the select's existing
registeredInitializers.length === 0guard fires, andinitializerNameresolves to'', sodisabled={creating || !initializerName}disables the button. A staleselectedInitializerNamecannot keep it enabled — the section unmounts whileloadingis true, so that local state resets on remount.
Regression test: should drop the loaded catalog and disable Add when a refresh fails after a successful load (loads successfully and asserts the real description + env vars + enabled Add first, then fails the second listRegistered call). Verified it fails on the pre-fix code — the stale "Registers targets." description is still rendered — and passes with the fix.
Initializers folder: 79/79 passing (was 78 before this test), tsc --noEmit clean, eslint --max-warnings 0 clean, all re-run after the merge commit.
|
Friendly ping — the last review round is fully addressed in |
Port of 20a5d7c from the pre-microsoft#2525 structure: the lookup module now exports CatalogStatus ('loading' | 'loaded' | 'error') and findRegisteredInitializer, which returns undefined on a miss instead of synthesizing a placeholder. Only a settled, successful catalog load may claim 'no longer registered'; any other status falls back to 'Catalog metadata temporarily unavailable.' via initializerFallbackDescription.
…s page Port of 2dc177d onto the post-microsoft#2525 structure. When the initializer catalog request fails, ConfiguredInitializers previously synthesized a placeholder claiming each configured initializer was 'no longer registered' — a false statement about a startup sequence the user cannot verify. The row now falls back to 'Catalog metadata temporarily unavailable.', omits the env-var line (it is catalog-derived and unknown), and only a settled, successful load may claim 'no longer registered.'
Port of d291d77: an empty list in the dialog previously read as 'No registered initializers were found.' even when the catalog request had failed — a false statement about the registry. With catalogStatus the dialog distinguishes a genuinely empty registry from an outage.
Port of 749e65e: a failed refresh previously kept the stale catalog in state, so rows kept rendering stale descriptions and env vars, and the available-initializers dialog stayed enabled off the stale list — right after the user was shown a refresh error. The rejected path now clears the catalog and flips catalogStatus to 'error'.
26c01a9 to
8a1861f
Compare
|
Rebased onto current
|
2 participants
Description
Fixes #2442
When
/api/initializers/settingssucceeds but the registered-initializer catalog request (GET /api/initializers) fails transiently, the Initializers page preserved configured baseline settings but described valid entries asInitializer is no longer registered.A temporary metadata availability failure was therefore presented as a definitive registration problem.Changes:
initializerLookup.ts: exportsCatalogStatus = 'loading' | 'loaded' | 'error';findRegisteredInitializerresolves a settings entry to its catalog entry orundefined(no synthetic placeholder objects);initializerFallbackDescriptionreportsInitializer is no longer registered.only when the catalog loaded successfully —loadinganderrorboth renderCatalog metadata temporarily unavailable.Initializers.tsx: trackscatalogStatusderived from thePromise.allSettledresult instead of a boolean flagBaselineInitializers.tsx/AdditionalInitializers.tsx: rows render env vars and parameter summaries only for real catalog entries; a catalog error shows an inline "editing disabled" note and disables Edit so an outage cannot null out saved parameters via a schema-less editorAvailableInitializersDialog.tsx: an empty list during a catalog error reports the catalog as unavailable instead ofNo registered initializers were foundBehavior note: Edit now requires a real catalog entry, so a genuinely unregistered name can no longer open the schema-less editor either.
Tests and Documentation
initializerLookup.test.ts: unit tests for the new lookup and status-based fallback copyAdditionalInitializers.test.tsx: Edit stays disabled while the catalog is unavailable; new case asserting Edit stays disabled for a name missing from a loaded catalog (no synthetic env vars / parameter summary rendered)BaselineInitializers.test.tsx/AvailableInitializersDialog.test.tsx: updated for the status-based propstsc --noEmit, andeslintpassThis contribution was developed with LLM assistance following repo conventions.