fix(core): surface registry configuration errors - #3077
Conversation
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://codex-1350-registry-config-errors.try.emdashcms.com, https://codex-1350-registry-config-errors-emdash-playground.emdash-cms.workers.dev (commit 72970d3)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
🦋 Changeset detectedLatest commit: 72970d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
This is the right change for #1350 and fits EmDash's architecture well. By validating registry config while Astro evaluates the integration and by returning a safe, value-free diagnostic from the runtime manifest, it turns a generic admin 500 into a field-level banner that tells the operator what to fix.
I checked the diff against the implementation, the manifest route, the admin shell and router, the registry browse/detail components, and the relevant tests. The core retry/tests look good: malformed URLs, insecure non-local aggregators, and invalid minimumReleaseAge are all surfaced as RegistryConfigurationError diagnostics, and unexpected non-configuration failures are still rethrown rather than swallowed.
The main gap is that policy.minimumReleaseAgeExclude is not validated as an array before .map() is called, so a plausible misconfiguration (e.g., passing a bare string instead of an array) still crashes with a generic TypeError instead of a field-specific REGISTRY_MINIMUM_RELEASE_AGE_EXCLUDE_INVALID diagnostic. I also have a small suggestion to make the admin banner's switch default less misleading if new registry fields are added later.
Overall this is close to clean once the array-shape handling (and a matching test) is added.
Findings
-
[needs fixing]
packages/core/src/registry/config.ts:365-368The code assumes
config.policy.minimumReleaseAgeExcludeis an array and calls.map()on it without anArray.isArray()guard. If a user passes a bare string, number, or other non-array value, the normalization throws a genericTypeError: config.policy.minimumReleaseAgeExclude.map is not a functioninstead of the field-specificREGISTRY_MINIMUM_RELEASE_AGE_EXCLUDE_INVALIDdiagnostic the PR is introducing. That contradicts the goal of surfacing every malformedexperimental.registryfield safely.Validate the shape first and throw a registry configuration error, then add a unit test that asserts
resolveManifestRegistryConfigreturns the excluded-field code for a non-array value.if (config.policy?.minimumReleaseAgeExclude !== undefined) { if (!Array.isArray(config.policy.minimumReleaseAgeExclude)) { throw new RegistryConfigurationError( "REGISTRY_MINIMUM_RELEASE_AGE_EXCLUDE_INVALID", "experimental.registry.policy.minimumReleaseAgeExclude", "must be an array of DIDs or <did>/<slug> entries", ); } // Normalize at load time so callers (browser and server) can do // plain string compares without each one re-implementing the // case-folding rule. const list = config.policy.minimumReleaseAgeExclude.map((entry) => { -
[suggestion]
packages/admin/src/components/RegistryConfigurationBanner.tsx:19The
defaultbranch assumes every unknown field isexperimental.registry.aggregatorUrl. The union is currently closed to three literals so this won't mislead today, but if a new diagnostic field is added later the default will silently point the user to the wrong config key. Make theaggregatorUrlcase explicit and use a generic fallback, or rely on TypeScript exhaustiveness so adding a new field forces an update here.switch (error.field) { case "experimental.registry.aggregatorUrl": description = t`Check experimental.registry.aggregatorUrl in astro.config.mjs, then restart EmDash.`; break; case "experimental.registry.policy.minimumReleaseAge": description = t`Check experimental.registry.policy.minimumReleaseAge in astro.config.mjs, then restart EmDash.`; break; case "experimental.registry.policy.minimumReleaseAgeExclude": description = t`Check experimental.registry.policy.minimumReleaseAgeExclude in astro.config.mjs, then restart EmDash.`; break; }
There was a problem hiding this comment.
This is the right fix for #1350 and fits EmDash’s architecture well. By validating experimental.registry while Astro evaluates the integration and by having the runtime manifest route return a safe, value-free diagnostic, the change turns a generic admin 500 into a field-level banner that tells the operator exactly what to correct.
The re-review confirms the two blockers from the previous pass are resolved:
config.policy.minimumReleaseAgeExcludeis now guarded withArray.isArray()before.map(), and non-string entries also get theREGISTRY_MINIMUM_RELEASE_AGE_EXCLUDE_INVALIDdiagnostic.RegistryConfigurationBannernow handles all three known fields explicitly and falls back to a genericexperimental.registrymessage for future/unknown fields, avoiding pointing users at the wrong config key.
I checked the diff against registry/config.ts, the runtime manifest builder, the Astro integration, EmDashManifest/AdminManifest, the admin router/Shell, the new banner component, and the unit/browser tests. The approach is sound: recognized misconfigurations surface with a stable code + field name that leaks no configured values or credentials; unexpected failures are still rethrown rather than swallowed; no new queries are added; all admin UI strings are Lingui-wrapped; the changeset is accurate and proportional for a patch.
No further blockers or nits remain—LGTM.
What does this PR do?
Invalid plugin registry settings now produce field-specific configuration errors while Astro loads the site. If a recognized invalid registry setting reaches runtime manifest generation, the admin remains available and shows which
experimental.registryfield to correct; unexpected manifest failures still return an error.The diagnostic contains only a stable code and field name, so configured values and credentials are not exposed. The change adds no logged-out queries.
Part of #1350.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output