Repository navigation
feat(admin): core update notice in the admin dashboard (WordPress-style version banner) - #1939
Conversation
🦋 Changeset detectedLatest commit: 521be94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 515 lines across 13 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sensible and matches the approved Discussion: a lightweight, deferred registry check with a purely informational admin banner is the right scope for core update awareness. The implementation generally follows EmDash conventions — admin-only route with requirePerm, after() for the background fetch, options-table caching, Lingui-wrapped strings, and RTL-safe layout classes.
I found two real issues that should be fixed before merge:
-
updateCheck: falsedoes nothing at runtime. The option is defined onEmDashConfigand documented, but it is not added toserializableConfig, which is what becomesvirtual:emdash/configand is exposed aslocals.emdash.config. The route therefore readsemdash.config.updateCheckasundefined, making the documented opt-out silently ignored. -
A malformed
checkedAtstring prevents the cache from ever refreshing.parseCacheonly verifies thatcheckedAtis a string, not that it parses to a valid date. If the stored value is garbage, the stale check becomesNaN >= CHECK_INTERVAL_MS(false), so no deferred refresh is scheduled and stale/corrupt data can persist indefinitely.
The unit tests cover the handler and the fetch logic well, but they exercise handleCoreUpdateStatus directly (with VERSION = "dev", so the deferred branch is never hit) and do not cover the integration-level serialization of updateCheck. Add a serialization-level check or route-level test for the opt-out once the config is wired through.
Findings
-
[needs fixing]
packages/core/src/astro/integration/index.ts:353updateCheckis added to theEmDashConfigtype and documented, but it is missing from theserializableConfigobject that is baked intovirtual:emdash/configand later attached aslocals.emdash.config. Because the route readsemdash.config.updateCheck !== false, the value is alwaysundefinedat runtime and the check is always treated as enabled — the documented opt-out is silently ignored.toolbar: resolvedConfig.toolbar, updateCheck: resolvedConfig.updateCheck, };
github-actions
Bot
added
review/needs-rereview
|
Both findings addressed in 79abc28:
|
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
There was a problem hiding this comment.
This is a clean re-review of the core update banner PR. The approach remains sound: it implements the admin-only, awareness-only banner from approved Discussion #1889 with a daily deferred npm-registry check, an options-table cache, a new updates:read permission, and runtime opt-out via updateCheck: false. It introduces no logged-out queries and fits EmDash's handler/route/API-envelope model.
The blocking item from the previous review — the missing .js extensions on the new admin component's internal ESM imports — is fixed. The other prior feedback has also been addressed in this revision:
CoreUpdateBannernow computes itslocalStoragekey lazily and guardslocationfor SSR/Node contexts.isNewerVersioncorrectly treats a stable release as newer than a prerelease of the same base version.- The module and component JSDocs no longer narrate rejected alternatives or PR rationale.
- The mysterious
ponytail:markers are gone.
I re-read the full changed sources and diff. Authorization on GET /_emdash/api/admin/core-update is wired through requirePerm("updates:read"), the endpoint is admin-only, and the registry fetch is deferred via after() with a 10 s timeout and swallowed failures. updateCheck is serialized through buildSerializableConfig, and all user-facing strings go through Lingui. Tests now cover the cache, malformed/unparsable cache handling, opt-out, and serializable-config propagation.
No new blockers or regressions. This is ready to merge.
c0d46d3 to
1120414
Compare
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a clean re-review of the core update banner PR. The approach remains the right one for approved Discussion #1889: an admin-only, awareness-only banner with a daily deferred npm-registry check, an options-table cache, a new updates:read permission, and runtime opt-out via updateCheck: false. It introduces no logged-out queries and follows EmDash’s handler/route/API-envelope model.
All blockers and suggestions from the previous review are addressed:
- The new admin component imports carry the
.jsextension. CoreUpdateBannercomputes its dismissal key lazily and guardslocation.isNewerVersioncorrectly treats a stable release as newer than a prerelease of the same base version.- The module and function JSDocs no longer narrate PR rationale or rejected alternatives.
I re-checked the changed sources and diff. Authorization on GET /_emdash/api/admin/core-update is wired through requirePerm("updates:read"), the registry fetch is deferred via after() with a 10 s timeout and swallowed failures, updateCheck is serialized through buildSerializableConfig, and all user-facing strings go through Lingui. The cache parsing correctly handles malformed entries and unparsable checkedAt timestamps.
One minor issue remains: a newly-added unit test for buildSerializableConfig is a config-pin test dropped into the wrong file (the image-remote-patterns test). It asserts config inputs back as outputs and the comment justifies an implementation detail rather than testing observable behavior. I’ve left a suggestion to remove it from that file and either rewrite it against behavior or move it to a dedicated test file.
No new blockers or regressions.
| describe("buildSerializableConfig", () => { | ||
| it("carries runtime-read options into virtual:emdash/config", () => { | ||
| // Options that routes/middleware read via `locals.emdash.config` are | ||
| // only visible at runtime if they survive serialization. An option | ||
| // added to EmDashConfig but not serialized is silently ignored. | ||
| const serialized = buildSerializableConfig({ | ||
| siteUrl: "https://example.com", | ||
| marketplace: "https://marketplace.example.com", | ||
| updateCheck: false, | ||
| }); | ||
|
|
||
| expect(serialized.siteUrl).toBe("https://example.com"); | ||
| expect(serialized.marketplace).toBe("https://marketplace.example.com"); | ||
| expect(serialized.updateCheck).toBe(false); | ||
| }); |
There was a problem hiding this comment.
[suggestion] The new buildSerializableConfig test is a config-pin test dropped into the wrong file. It takes a config literal, passes it through the serializer, and asserts the same values back, which mostly re-states the implementation. The comment explains why serialization matters, but that belongs in the source docs, not in a test that doesn’t exercise a user-observable regression. If someone accidentally removes a runtime option from buildSerializableConfig, an integration test of the actual route/middleware behavior would be far more durable.
Move this to a dedicated test file (e.g. serializable-config.test.ts) and rewrite it against an observable invariant, or delete it and rely on the handler/route tests. At minimum, don’t leave an unrelated serialization test in image-remote-patterns.test.ts.
| describe("buildSerializableConfig", () => { | |
| it("carries runtime-read options into virtual:emdash/config", () => { | |
| // Options that routes/middleware read via `locals.emdash.config` are | |
| // only visible at runtime if they survive serialization. An option | |
| // added to EmDashConfig but not serialized is silently ignored. | |
| const serialized = buildSerializableConfig({ | |
| siteUrl: "https://example.com", | |
| marketplace: "https://marketplace.example.com", | |
| updateCheck: false, | |
| }); | |
| expect(serialized.siteUrl).toBe("https://example.com"); | |
| expect(serialized.marketplace).toBe("https://marketplace.example.com"); | |
| expect(serialized.updateCheck).toBe(false); | |
| }); | |
| import { describe, expect, it } from "vitest"; | |
| import { | |
| buildImageRemotePatterns, | |
| resolveImageEndpoint, | |
| } from "../../../../src/astro/integration/index.js"; | |
| const s3 = (publicUrl?: string) => ({ entrypoint: "x", config: { publicUrl } }); | |
| const localStorage = { entrypoint: "x", config: { directory: "./uploads" } }; | |
| const MEDIA_PATH = "/_emdash/api/media/file/**"; | |
| describe("buildImageRemotePatterns", () => { | |
| it("authorizes the storage public URL host", () => { | |
| expect(buildImageRemotePatterns(s3("https://cdn.example.com"), undefined, "build")).toEqual([ | |
| { protocol: "https", hostname: "cdn.example.com" }, | |
| ]); | |
| }); | |
| it("scopes the pattern to the public URL path prefix when present", () => { | |
| expect( | |
| buildImageRemotePatterns(s3("https://cdn.example.com/assets"), undefined, "build"), | |
| ).toEqual([{ protocol: "https", hostname: "cdn.example.com", pathname: "/assets/**" }]); | |
| }); | |
| it("normalizes a trailing slash on the path prefix", () => { | |
| expect( | |
| buildImageRemotePatterns(s3("https://cdn.example.com/assets/"), undefined, "build"), | |
| ).toEqual([{ protocol: "https", hostname: "cdn.example.com", pathname: "/assets/**" }]); | |
| }); | |
| it("ignores a non-http(s) public URL", () => { | |
| expect(buildImageRemotePatterns(s3("ftp://files.example.com"), undefined, "build")).toEqual([]); | |
| }); | |
| it("ignores an unparseable public URL", () => { | |
| expect(buildImageRemotePatterns(s3("not a url"), undefined, "build")).toEqual([]); | |
| }); | |
| it("authorizes the site origin scoped to the media route when siteUrl is set", () => { | |
| expect(buildImageRemotePatterns(localStorage, "https://example.com", "build")).toEqual([ | |
| { hostname: "example.com", pathname: MEDIA_PATH }, | |
| ]); | |
| }); | |
| it("adds a host-agnostic media pattern only in dev", () => { | |
| expect(buildImageRemotePatterns(localStorage, undefined, "dev")).toEqual([ | |
| { pathname: MEDIA_PATH }, | |
| ]); | |
| expect(buildImageRemotePatterns(localStorage, undefined, "build")).toEqual([]); | |
| }); | |
| it("combines CDN, site-origin, and dev patterns", () => { | |
| expect( | |
| buildImageRemotePatterns(s3("https://cdn.example.com"), "https://example.com", "dev"), | |
| ).toEqual([ | |
| { protocol: "https", hostname: "cdn.example.com" }, | |
| { hostname: "example.com", pathname: MEDIA_PATH }, | |
| { pathname: MEDIA_PATH }, | |
| ]); | |
| }); | |
| }); | |
| describe("resolveImageEndpoint", () => { | |
| it("installs the Node endpoint on a stock/undefined endpoint", () => { | |
| expect( | |
| resolveImageEndpoint({ | |
| imagesDisabled: false, | |
| currentEntrypoint: undefined, | |
| isCloudflare: false, | |
| }), | |
| ).toEqual({ entrypoint: "emdash/image-endpoint" }); | |
| expect( | |
| resolveImageEndpoint({ | |
| imagesDisabled: false, | |
| currentEntrypoint: "astro/assets/endpoint/generic", | |
| isCloudflare: false, | |
| }), | |
| ).toEqual({ entrypoint: "emdash/image-endpoint" }); | |
| }); | |
| it("installs the Cloudflare endpoint under the Cloudflare adapter", () => { | |
| expect( | |
| resolveImageEndpoint({ | |
| imagesDisabled: false, | |
| currentEntrypoint: "@astrojs/cloudflare/image-transform-endpoint", | |
| isCloudflare: true, | |
| }), | |
| ).toEqual({ entrypoint: "@emdash-cms/cloudflare/image-endpoint" }); | |
| }); | |
| it("skips silently when images are disabled", () => { | |
| expect( | |
| resolveImageEndpoint({ | |
| imagesDisabled: true, | |
| currentEntrypoint: undefined, | |
| isCloudflare: true, | |
| }), | |
| ).toEqual({}); | |
| }); | |
| it("leaves a deliberate passthrough endpoint alone without warning", () => { | |
| expect( | |
| resolveImageEndpoint({ | |
| imagesDisabled: false, | |
| currentEntrypoint: "@astrojs/cloudflare/image-passthrough-endpoint", | |
| isCloudflare: true, | |
| }), | |
| ).toEqual({}); | |
| }); | |
| it("warns and skips when a custom endpoint is configured", () => { | |
| const result = resolveImageEndpoint({ | |
| imagesDisabled: false, | |
| currentEntrypoint: "./src/my-endpoint.ts", | |
| isCloudflare: false, | |
| }); | |
| expect(result.entrypoint).toBeUndefined(); | |
| expect(result.warn).toMatch(/custom image\.endpoint/); | |
| }); | |
| }); |
emdashbot
Bot
added
review/approved
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a clean re-review of the core update banner PR. The approach remains the right one for approved Discussion #1889: an admin-only, awareness-only banner with a daily deferred npm-registry check, an options-table cache, a new updates:read permission, and runtime opt-out via updateCheck: false. It introduces no logged-out queries and follows EmDash’s handler/route/API-envelope model.
All blockers from the previous review are addressed:
- Internal ESM imports use
.jsextensions. CoreUpdateBannercomputes its dismissal key lazily and guardslocation/localStorage.isNewerVersioncorrectly treats a stable release as newer than a prerelease of the same base version.- Module and component JSDocs are concise and no longer narrate PR rationale or rejected alternatives.
I re-checked the changed sources and diff. Authorization on GET /_emdash/api/admin/core-update is wired through requirePerm("updates:read"), the registry fetch is deferred via after() with a 10 s timeout and swallowed failures, updateCheck is serialized through buildSerializableConfig, and all user-facing strings go through Lingui. The cache parsing correctly handles malformed entries and unparsable checkedAt timestamps.
One minor type-contract gap remains: updateCheck is now part of the serialized config baked into virtual:emdash/config, but the public declaration for that virtual module does not list it. That can give downstream TypeScript consumers a false error when reading the option.
The previously flagged config-pin buildSerializableConfig test is now isolated in its own file, which satisfies the “at minimum, move it out of image-remote-patterns.test.ts” resolution from the last review, even though it still asserts a value back at itself.
Findings
-
[suggestion]
packages/core/src/virtual-modules.d.ts:18updateCheckis now serialized throughbuildSerializableConfig()and emitted by thevirtual:emdash/configmodule, but the publicVirtualConfigdeclaration does not list it. Downstream code that imports the virtual module without@ts-ignorewill get a TypeScript error when readingconfig.updateCheck, even though the value is present at runtime.Add the new option to the interface so the declaration matches the serialized shape:
toolbar?: "server" | "client" | false; updateCheck?: boolean;
There was a problem hiding this comment.
This is a clean re-review of the core update banner PR. The approach remains the right one for the maintainer-approved Discussion #1889: an admin-only, awareness-only banner with a daily deferred npm-registry check, an options-table cache, a new updates:read permission, and runtime opt-out via updateCheck: false.
I re-read the diff and the changed sources. All blockers from the previous review are resolved:
- Internal ESM imports use
.jsextensions (CoreUpdateBanner,Dashboard,core-update.ts, route aliases, etc.). CoreUpdateBannercomputes its dismissal key lazily and guardslocation(typeof location === "undefined"), so it is safe to import wherelocationis unavailable.isNewerVersionnow correctly treats a stable release as newer than a prerelease of the same base version.- Module and component JSDocs are concise and no longer narrate PR rationale or rejected alternatives.
updateCheckis serialized throughbuildSerializableConfig()and the publicvirtual:emdash/configdeclaration now listsupdateCheck?: boolean, so downstream TypeScript consumers receive the correct runtime shape.
The new endpoint follows EmDash conventions: GET /_emdash/api/admin/core-update is a thin route that checks updates:read via requirePerm, defers the registry fetch through after(), and returns the standard { success, data } envelope via unwrapResult(). The handler uses the options-table cache, treats malformed/unparsable cache entries as absent so they can be overwritten, and returns updateAvailable: false for the "dev" test/development version. All user-visible strings go through Lingui, and the Tailwind classes in the banner are direction-neutral.
The change introduces no logged-out queries; the only new request is an admin-only local options read, and the outbound npm fetch is deferred and opt-outable. The changeset names the affected packages, the new permission, and the opt-out config. No remaining issues to fix.
|
We'd like a 24h grace period before prompting for updates: only report |
Adds a daily, deferred npm-registry check for the latest published emdash version, cached in the options table, with a dismissible admin-only dashboard banner and an updateCheck config opt-out.
|
Thanks! Done in 521be94: One deviation from "cached alongside latest": the cache keeps the publish times of all stable, non-deprecated releases up to |
There was a problem hiding this comment.
This is a clean re-review of the core update banner PR. The approach remains the right one for the maintainer-approved Discussion #1889: an admin-only, awareness-only banner with a daily deferred npm-registry check, an options-table cache, a new updates:read permission, and runtime opt-out via updateCheck: false.
The Sep 27 update adds the requested minimumReleaseAge feature, and the implementation is solid:
- Config validation happens during integration setup by reusing
parseDurationSecondsfrom the registry policy code, so invalid duration strings fail at startup with a clear, source-attributed message. handleCoreUpdateStatusfalls back to the default 24-hour holdback whenupdateCheckis omitted ortrue, and accepts zero/integer seconds for users who want no holdback.newestReleasedcorrectly chooses the newest stable version that is older thanminimumReleaseAge, falling back to older releases when the absolute latest is too fresh.buildSerializableConfig()now carriesupdateCheck, so runtime routes can read it fromvirtual:emdash/config; the virtual-module declaration matches.- All prior bot-review blockers are still addressed: internal imports use
.jsextensions,CoreUpdateBannerderives its dismissal key lazily and guardslocation,isNewerVersiontreats a stable release as newer than a prerelease of the same base version, comments are concise and no longer narrate PR rationale, and the config serialization test moved to a dedicated file. - The endpoint is admin-only, the only new synchronous request is a local options read, and the outbound npm fetch is deferred with
after(), so logged-out routes are unaffected. - User-facing strings are Lingui-wrapped, Tailwind classes in the banner are direction-neutral, docs and the changeset cover the new option, and tests exercise the duration parsing/fallback path.
I read the diff, the changed sources, and the relevant siblings. No remaining issues to fix.
What does this PR do?
Implements the core update notice proposed (and maintainer-+1'd) in Discussion #1889: a WordPress-style "a new version is available" banner in the admin dashboard.
Coming from WordPress, the update notice is the single biggest "am I safe?" signal. EmDash already surfaces updates for marketplace plugins, but for the core there is nothing — a site owner who isn't the developer has no way to know they're running 0.22 while 0.24 is out with security fixes.
How it works:
VERSION). Published releases come from a plain GET to theemdashpackument athttps://registry.npmjs.org/emdash(~460 KB today;/emdash/latesthas no publish time) — checked at most once per day, also after a failed check, always deferred viaafter()so it never blocks a request. Only a small map of stable, non-deprecated release versions up tolatestand their publish times (~2 KB) is cached in the options table. The request carries no site data; sites without outbound internet simply never show the banner.minimumReleaseAge(default"24h"), the same way pnpm'sminimumReleaseAgepicks a version. Configure it withupdateCheck: { minimumReleaseAge: "7d" }(duration string or seconds, parsed likeregistry.policy.minimumReleaseAgeand validated at startup).GET /_emdash/api/admin/core-update(newupdates:readpermission, min role admin) serves the cached status — always a fast local read.updateCheck: falsein the EmDash config disables the check entirely (both options are documented in the configuration reference).Deliberately not in scope (as discussed): an "update now" button. An EmDash update is an npm bump + rebuild + redeploy, which the admin cannot and should not trigger. The value is the awareness.
Update (Sep 27): added the minimum release age requested in review, and rebased onto current
main(buildSerializableConfig()now also carriesobjectCacheEnabled).Rebase note (Sep 10): squashed and rebased onto current
main. Both findings from the July bot review remain addressed (updateCheckserialization viabuildSerializableConfig()with a covering test; unparsablecheckedAttreated as no cache).buildSerializableConfig()now carries themigrationskey upstream added since.Discussion: #1889
Closes #1889
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
(Banner strings are Lingui-wrapped; the German catalog entry is extracted on merge, hence the English banner text in a German UI here.)