Replaced cdnUrl config with script-origin asset base derivation#26555
Open
Replaced cdnUrl config with script-origin asset base derivation#26555
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughIntroduces a new utility 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Feb 24, 2026
rob-ghost
force-pushed
the
feat/ghost-cdn-url-meta-tag
branch
from
306170c to
e5a2df9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin/index.html (1)
17-30: Guard decode/parse to avoid hard failure on malformed meta content.If the env meta tag is ever malformed (e.g., partial rewrite),
decodeURIComponent/JSON.parsewill throw and can stop the script. A small guard keeps the boot resilient.🔧 Suggested defensive guard
(function() { var c = document.querySelector('meta[name="ghost-cdn-url"]'); if (c && c.content) { var m = document.querySelector('meta[name="ghost-admin/config/environment"]'); if (m) { - var d = JSON.parse(decodeURIComponent(m.content)); - d.cdnUrl = c.content; - m.content = encodeURIComponent(JSON.stringify(d)); + try { + var d = JSON.parse(decodeURIComponent(m.content)); + d.cdnUrl = c.content; + m.content = encodeURIComponent(JSON.stringify(d)); + } catch (e) { + // no-op: keep original content if malformed + } } } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/index.html` around lines 17 - 30, Guard the decode/parse of the env meta content to avoid hard failures: in the inline IIFE that queries meta elements (variables c and m) and reads m.content, validate m.content is present and wrap the decodeURIComponent + JSON.parse steps in a try/catch (or conditional check) so malformed or empty content is skipped; only set d.cdnUrl and update m.content if parsing succeeds, and log or silently ignore parse errors to keep the boot resilient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin/index.html`:
- Around line 17-30: Guard the decode/parse of the env meta content to avoid
hard failures: in the inline IIFE that queries meta elements (variables c and m)
and reads m.content, validate m.content is present and wrap the
decodeURIComponent + JSON.parse steps in a try/catch (or conditional check) so
malformed or empty content is skipped; only set d.cdnUrl and update m.content if
parsing succeeds, and log or silently ignore parse errors to keep the boot
resilient.
rob-ghost
force-pushed
the
feat/ghost-cdn-url-meta-tag
branch
from
e5a2df9 to
542240d
Compare
rob-ghost
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/services/lazy-loader.js`:
- Around line 1-4: The init() method in lazy-loader.js references
config.environment but the config import was removed causing a ReferenceError at
runtime; restore the import of the app environment config (import config from
the app's environment module—the same module previously removed) at the top of
the file so that the config identifier used in init() is defined. Ensure the
imported name is exactly config and no other symbol is renamed so init() can
read config.environment without changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/admin/vite-ember-assets.tsghost/admin/app/components/admin-x/admin-x-component.jsghost/admin/app/helpers/parse-history-event.jsghost/admin/app/models/user.jsghost/admin/app/services/lazy-loader.jsghost/admin/app/utils/asset-base.jsghost/admin/app/utils/fetch-koenig-lexical.jsghost/admin/config/environment.js
💤 Files with no reviewable changes (1)
- ghost/admin/config/environment.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin/vite-ember-assets.ts
rob-ghost
force-pushed
the
feat/ghost-cdn-url-meta-tag
branch
from
542240d to
78029fa
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/admin/app/helpers/parse-history-event.js (1)
14-14: Normalize base before appendingassetsfor safer URL construction.Line 14 assumes
assetBase()always ends with/. Normalizing here avoids malformed paths if fallback formatting ever changes.💡 Proposed hardening
- const assetRoot = `${assetBase()}assets`; + const assetRoot = `${assetBase().replace(/\/?$/, '/')}assets`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/helpers/parse-history-event.js` at line 14, The current line builds assetRoot by blindly concatenating assetBase() and "assets", which breaks if assetBase() lacks a trailing slash; update the construction so you first normalize the base returned by assetBase() to ensure it ends with a single '/' (e.g., check assetBase() with endsWith('/') and append '/' if missing or strip duplicate slashes) and then append "assets", replacing the use of assetBase() directly in the template literal that defines assetRoot so assetRoot always becomes a valid URL/string regardless of assetBase() formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/admin/app/helpers/parse-history-event.js`:
- Line 14: The current line builds assetRoot by blindly concatenating
assetBase() and "assets", which breaks if assetBase() lacks a trailing slash;
update the construction so you first normalize the base returned by assetBase()
to ensure it ends with a single '/' (e.g., check assetBase() with endsWith('/')
and append '/' if missing or strip duplicate slashes) and then append "assets",
replacing the use of assetBase() directly in the template literal that defines
assetRoot so assetRoot always becomes a valid URL/string regardless of
assetBase() formatting.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/admin/vite-ember-assets.tsghost/admin/app/components/admin-x/admin-x-component.jsghost/admin/app/helpers/parse-history-event.jsghost/admin/app/models/user.jsghost/admin/app/services/lazy-loader.jsghost/admin/app/utils/asset-base.jsghost/admin/app/utils/fetch-koenig-lexical.jsghost/admin/config/environment.js
💤 Files with no reviewable changes (1)
- ghost/admin/config/environment.js
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/admin/app/services/lazy-loader.js
- ghost/admin/app/models/user.js
- ghost/admin/app/utils/fetch-koenig-lexical.js
- ghost/admin/app/utils/asset-base.js
- ghost/admin/app/components/admin-x/admin-x-component.js
Member
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
Overall implementation looks reasonable. My main concerns would be around how this will interact with the existing Ghost Moya flows that try to do path replace and our release flows outside of the new CD setup.
It should probably be fine, but it would be nice to double check that before we merge this and it goes out into production for everyone.
rob-ghost
force-pushed
the
feat/ghost-cdn-url-meta-tag
branch
from
78029fa to
6b1f371
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/admin/tests/unit/utils/asset-base-test.js (1)
53-80: Consider one more cache-busting variant test (?v=/#...).This is optional, but adding one URL-with-query/hash case would harden regression coverage for rewritten script URLs.
Optional test addition
describe('script detection', function () { + it('extracts base when script URL contains query/hash', function () { + const doc = docWithScript('http://localhost:2368/ghost/assets/ghost-abc123.js?v=1#entry'); + const result = resolveAssetBase(doc); + + expect(result).to.equal('http://localhost:2368/ghost/'); + }); + it('extracts base from a non-fingerprinted script (ghost.js)', function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/unit/utils/asset-base-test.js` around lines 53 - 80, Add a test in asset-base-test.js to cover cache-busting URL variants by calling resolveAssetBase with a document created via docWithScript that includes a script src containing a query string and/or hash (e.g., '.../ghost-abc123.js?v=1#hash') and asserting the returned base still produces a valid absolute URL usable with new URL() (same pattern as the existing "script-derived result works with new URL()" test); reference resolveAssetBase and docWithScript to locate where to add the new it(...) case and mirror the expectations used for koenigUrl.href or the new URL() no-throw assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/admin/tests/unit/utils/asset-base-test.js`:
- Around line 53-80: Add a test in asset-base-test.js to cover cache-busting URL
variants by calling resolveAssetBase with a document created via docWithScript
that includes a script src containing a query string and/or hash (e.g.,
'.../ghost-abc123.js?v=1#hash') and asserting the returned base still produces a
valid absolute URL usable with new URL() (same pattern as the existing
"script-derived result works with new URL()" test); reference resolveAssetBase
and docWithScript to locate where to add the new it(...) case and mirror the
expectations used for koenigUrl.href or the new URL() no-throw assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/admin/vite-ember-assets.tsghost/admin/app/components/admin-x/admin-x-component.jsghost/admin/app/helpers/parse-history-event.jsghost/admin/app/models/user.jsghost/admin/app/services/lazy-loader.jsghost/admin/app/utils/asset-base.jsghost/admin/app/utils/fetch-koenig-lexical.jsghost/admin/config/environment.jsghost/admin/tests/unit/utils/asset-base-test.js
💤 Files with no reviewable changes (1)
- ghost/admin/config/environment.js
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/admin/app/utils/fetch-koenig-lexical.js
- ghost/admin/app/helpers/parse-history-event.js
- ghost/admin/app/components/admin-x/admin-x-component.js
- ghost/admin/app/services/lazy-loader.js
- ghost/admin/app/utils/asset-base.js
- apps/admin/vite-ember-assets.ts
ref https://linear.app/ghost/issue/ENG-1326/ Ember's dynamic asset loading (lazy-loader, admin-x components, Koenig editor, user avatars) previously relied on a build-time `cdnUrl` config to construct absolute CDN URLs. This required piping GHOST_CDN_URL through environment.js into an encoded meta tag. Instead, derive the asset base at runtime from the Ember script's own URL — the same principle ES modules use for relative imports. If the script loaded from a CDN (via index.html sed rewrite), dynamic loads inherit the CDN origin. If local, they inherit the local path. New `assetBase()` utility replaces 6 duplicated cdnUrl conditionals. Also fixes the vite-ember-assets build to read from ghost/admin/dist.
rob-ghost
force-pushed
the
feat/ghost-cdn-url-meta-tag
branch
from
6b1f371 to
dd70141
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/ENG-1326
Summary
cdnUrlconfig with a runtimeassetBase()utility that derives the CDN base from where the Ember scripts were loaded — the same principle ES modules use for relative importsghost-cdn-urlmeta tag and bridge script fromindex.html(no longer needed)vite-ember-assetsplugin reading from the combined output directory instead of the Ember dist, which caused././assets/path prefixes to accumulate on repeated buildsBackground
Ghost(Pro) currently bakes absolute CDN URLs into the admin build at CI time via
GHOST_CDN_URL, then string-replaces them per environment at deploy time. The newcd.ymlflow (Ghost-Moya #135, merged) replaces this with a simpler approach: build once with relative paths, then rewrite onlyindex.htmlat deploy time withsed.Vite's ES modules handle this automatically —
import.meta.urlreflects where the module was loaded from, so dynamic imports resolve to CDN if the entry script was loaded from CDN. Ember uses classic AMD scripts with no module-relative resolution, so it previously needed acdnUrlconfig value to construct absolute URLs for lazy-loaded assets.Rather than shimming
cdnUrlthrough a meta tag, this PR makes Ember work the same way as Vite: derive the asset base from where the scripts were loaded.Details
assetBase()utility (ghost/admin/app/utils/asset-base.js): Finds the Ember<script>element in the DOM, reads itssrc(always resolved to an absolute URL by the browser), and extracts the base path up to/assets/. If the script loaded from CDN (becausesedrewroteindex.html), the result is the CDN base. If local, it's the local admin root. The result is memoized. Falls back toghostPaths().adminRootif no matching script is found.Replaced 6 duplicated conditionals:
lazy-loader.js,parse-history-event.js,admin-x-component.js,user.js, andfetch-koenig-lexical.jsall hadconfig.cdnUrl ? ... : ghostPaths()...patterns. Each is now a singleassetBase()call with unused imports removed.Removed
cdnUrlfromconfig/environment.js: No longer needed at runtime.GHOST_CDN_URLstill works at build time forember-cli-build.js(fingerprint.prepend,publicAssetURL) — self-hosters who set it get the same behaviour as before.././ fix: The
vite-ember-assetsplugin was reading Ember asset paths fromghost/core/core/built/admin/index.html— the combined output directory that gets overwritten by the plugin's owncloseBundlehook. On each build, paths that already had./got another./prepended. Now reads fromghost/admin/dist/index.html(the Ember build's own output), producing clean./assets/paths.