[Draft] Removed GHOST_CDN_URL from Ghost(Pro) admin CI build#26521
[Draft] Removed GHOST_CDN_URL from Ghost(Pro) admin CI build#26521
Conversation
WalkthroughThe CI workflow configuration in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
1772-1783: Validation only runs indeploy_admin(main-only); regressions won't be caught on PRs.The
deploy_adminjob is gated byis_main == 'true', so a PR that accidentally re-introduces absolute CDN URLs would only fail CI after merging tomain. Consider running the sametest -f/grepassertions in a PR-accessible job (e.g., add aValidate admin build outputstep tojob_browser-testswhich already builds the Vite admin at line 383, or add a lightweight dedicated step to a job that runs on every push).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1772 - 1783, The Validate admin build output step currently only runs in the deploy_admin job gated by is_main == 'true', so duplicate the exact validation commands (the test -f and the two grep checks that assert relative asset refs and no https:// absolute URLs) into a job that runs for PRs—e.g., add the same "Validate admin build output" step right after the Vite admin build step in job_browser-tests (which already builds the admin), or create a lightweight cross-PR job that runs on pushes/PRs; keep the step name "Validate admin build output" and reuse the same shell commands so regressions are caught before merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 1781: The grep check only rejects explicit http/https URLs using the
pattern '(src|href)="https?://' and misses protocol-relative URLs like
src="//cdn.example.com/..."; update the regex used in the grep command (the
string containing '(src|href)="https?://') to also match protocol-relative
URLs—for example by allowing an optional "http(s):" prefix before the '//'
(i.e., change to something that matches '(src|href)="(https?:)?//') so both
absolute and protocol-relative external links are caught.
In `@apps/admin/vite.config.ts`:
- Around line 32-34: The dev server base path returned by getSubdir() currently
produces "/ghost" without a trailing slash which breaks Vite 7 HMR/URL
resolution; update the return value in the code that computes the dev server
base (the expression that currently returns `${getSubdir()}/ghost`) to ensure it
ends with a trailing slash (e.g. `${getSubdir()}/ghost/`) so the Vite base path
always includes the final "/" for proper HMR and nested-route resolution.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1772-1783: The Validate admin build output step currently only
runs in the deploy_admin job gated by is_main == 'true', so duplicate the exact
validation commands (the test -f and the two grep checks that assert relative
asset refs and no https:// absolute URLs) into a job that runs for PRs—e.g., add
the same "Validate admin build output" step right after the Vite admin build
step in job_browser-tests (which already builds the admin), or create a
lightweight cross-PR job that runs on pushes/PRs; keep the step name "Validate
admin build output" and reuse the same shell commands so regressions are caught
before merge.
.github/workflows/ci.yml
Outdated
| grep -qE '(src|href)="\./' apps/admin/dist/index.html | ||
|
|
||
| # No absolute URLs baked in | ||
| ! grep -qE '(src|href)="https?://' apps/admin/dist/index.html |
There was a problem hiding this comment.
Validation gap: protocol-relative URLs (//cdn.example.com/…) are not caught.
The negative assertion only rejects https?://. A protocol-relative src="//cdn.example.com/…" would pass both checks undetected. Expanding the pattern closes this gap:
🛡️ Proposed fix
- ! grep -qE '(src|href)="https?://' apps/admin/dist/index.html
+ ! grep -qE '(src|href)="(https?:)?//' apps/admin/dist/index.html📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ! grep -qE '(src|href)="https?://' apps/admin/dist/index.html | |
| ! grep -qE '(src|href)="(https?:)?//' apps/admin/dist/index.html |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 1781, The grep check only rejects explicit
http/https URLs using the pattern '(src|href)="https?://' and misses
protocol-relative URLs like src="//cdn.example.com/..."; update the regex used
in the grep command (the string containing '(src|href)="https?://') to also
match protocol-relative URLs—for example by allowing an optional "http(s):"
prefix before the '//' (i.e., change to something that matches
'(src|href)="(https?:)?//') so both absolute and protocol-relative external
links are caught.
apps/admin/vite.config.ts
Outdated
| // Dev server only: tells Vite HMR/module loading to use /ghost/ (or /blog/ghost/) | ||
| // as the base path, matching where Ghost serves admin in development. | ||
| return `${getSubdir()}/ghost`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the vite.config.ts file to see the actual code
fd vite.config.ts --type f | head -5Repository: TryGhost/Ghost
Length of output: 189
🏁 Script executed:
# Find and examine the vite.config.ts file in apps/admin
find apps/admin -name vite.config.ts -type fRepository: TryGhost/Ghost
Length of output: 83
🏁 Script executed:
# Let's also look for any vite.config files in the repo
fd vite.config -type fRepository: TryGhost/Ghost
Length of output: 228
🌐 Web query:
Vite 7 base path configuration trailing slash requirement dev server
💡 Result:
In Vite 7, if you set a non-relative base, it’s expected to be a directory-style public path (i.e. it effectively behaves like a URL prefix), so you should include the trailing slash:
// vite.config.ts
export default {
base: '/app/', // ✅
// base: '/app', // 🚫 (can lead to wrong URL resolution)
}- The Vite 7 docs list
/foo/(with trailing slash) as the canonical “absolute URL pathname” form forbase. [1] - The build docs/examples also use a trailing slash in
vite build --base=/my/public/path/. [2] - Vite has historically normalized/enforced leading/trailing slashes for non-URL
basevalues in config (see discussion around the enforcement). [3]
Dev server behavior (why the trailing slash matters)
On the dev server, paths without a trailing slash can be treated as “file-like” URLs, which changes how relative URLs resolve. A common symptom (especially for nested routes / MPA) is that visiting /nested serves the wrong HTML, while /nested/ works as expected—an inconsistency that has been reported and discussed in Vite’s tracker. [4]
Practical guidance
- Use
base: '/app/'. - Ensure your reverse proxy / backend redirects
/app→/app/(and similarly for any “directory” routes) if you want both to work consistently. - If you truly need a base that doesn’t behave like a directory prefix, consider whether you actually want a relative base (
base: './'orbase: '') for embedded deployments instead. [1]
Sources:
[1] Vite 7 config: base option (valid values/examples) (vite.dev)
[2] Vite 7 build guide: Public Base Path (vite build --base=.../) (vite.dev)
[3] Vite issue discussing enforced leading/trailing slashes for base (github.com)
[4] Vite issue: dev server trailing-slash behavior for nested URLs (github.com)
Citations:
- 1: https://vite.dev/config/shared-options.html
- 2: https://vite.dev/guide/build
- 3: Avoid adding leading/trailing slash to base vitejs/vite#8202
- 4: Inconsistent URL trailing slash behavior between dev and preview servers vitejs/vite#6596
🏁 Script executed:
# Now let's examine the actual vite.config.ts file in apps/admin
cat -n apps/admin/vite.config.tsRepository: TryGhost/Ghost
Length of output: 2762
🏁 Script executed:
# Also find and check the getSubdir() function
rg "getSubdir" apps/admin/vite.config.ts -A 5Repository: TryGhost/Ghost
Length of output: 327
🏁 Script executed:
# Search for getSubdir definition
rg "function getSubdir|const getSubdir|export.*getSubdir" --type tsRepository: TryGhost/Ghost
Length of output: 121
Add trailing slash to dev server base path for proper HMR resolution.
The comment on lines 32–33 documents /ghost/ (with trailing slash) but the code returns /ghost without one. Vite 7 requires the base path to end with / for correct dev server HMR and URL resolution; without it, nested routes can fail to resolve properly.
Fix
- return `${getSubdir()}/ghost`;
+ return `${getSubdir()}/ghost/`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Dev server only: tells Vite HMR/module loading to use /ghost/ (or /blog/ghost/) | |
| // as the base path, matching where Ghost serves admin in development. | |
| return `${getSubdir()}/ghost`; | |
| // Dev server only: tells Vite HMR/module loading to use /ghost/ (or /blog/ghost/) | |
| // as the base path, matching where Ghost serves admin in development. | |
| return `${getSubdir()}/ghost/`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/vite.config.ts` around lines 32 - 34, The dev server base path
returned by getSubdir() currently produces "/ghost" without a trailing slash
which breaks Vite 7 HMR/URL resolution; update the return value in the code that
computes the dev server base (the expression that currently returns
`${getSubdir()}/ghost`) to ensure it ends with a trailing slash (e.g.
`${getSubdir()}/ghost/`) so the Vite base path always includes the final "/" for
proper HMR and nested-route resolution.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26521 +/- ##
==========================================
- Coverage 73.12% 73.12% -0.01%
==========================================
Files 1528 1528
Lines 120068 120135 +67
Branches 14476 14495 +19
==========================================
+ Hits 87802 87844 +42
- Misses 31264 31270 +6
- Partials 1002 1021 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rob-ghost
force-pushed
the
chore/remove-ghost-cdn-url
branch
2 times, most recently
from
1eb1147 to
3981ff2
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 1514: The current grep check "! grep -qE '(src|href)=\"https?://'
apps/admin/dist/index.html" misses protocol-relative URLs like
//cdn.example.com; update the regex used in the CI step (the grep -qE pattern)
to also detect protocol-relative URLs by matching either "https?://" or "//"
after src|href, e.g. change the pattern to something that includes (https?:)?//
or explicitly alternates "//" so the grep command flags both absolute and
protocol-relative URLs.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/admin/vite.config.ts`:
- Around line 32-34: The comment and returned base are inconsistent and Vite
requires the base to end with a trailing slash; update the return value that
currently uses `${getSubdir()}/ghost` to include a trailing slash (e.g.,
`${getSubdir()}/ghost/`) in the function that builds the dev server base in
vite.config.ts, and update the adjacent comment if needed to match the returned
value; reference getSubdir() and the dev-server base return expression.
|
@rob-ghost @JohnONolan This is a breaking change, and should not be released before v7, if we care semantic versioning. I believe there are many self-hosted users of this undocumented feature, as many other undocumented (or not well documented) things in Ghost. Please give extra attention when you make breaking changes on an open-source project. This is also not something that will give you any extra hassle. Just don't use env variable on Ghost(Pro). Of course I have no clue about BlackPearl or the relative admin build proposal, but still, being able to serve static assets from a separate domain is a good thing to have. And I can't see any reason to drop this feature. |
Stopped setting GHOST_CDN_URL in the deploy_admin CI job so Ghost(Pro) admin builds use relative paths by default. The GHOST_CDN_URL config option is preserved — anyone who sets it (self-hosters, contributors) gets the same behaviour as before.
rob-ghost
force-pushed
the
chore/remove-ghost-cdn-url
branch
from
3981ff2 to
7bab130
Compare
|
@muratcorlu thank you for the feedback and sorry for any alarm. This PR is actually a part of a proposal draft that hasn't been reviewed or accepted and we definitely won't be making any breaking changes as a part of that. I've changed this to a draft PR and also reverted the removal of the build option to be absolutely clear that we're not intending to do that anytime soon. Sorry for the confusion caused, we'll look into how to make it more clear when PRs are a part of a proposal / suggestion and when they're intended for merge. |
rob-ghost
changed the title
|
Hello @rob-ghost. Thanks for the reply. I'm glad to hear that you re-consider this change. And yes, we really need to have more clear and open communication about development of Ghost. I understand you have Ghost(Pro) related tasks, but since we all (other hosting providers and self-hosters) use the same code-base, it's important for us to be able to track what is going on. Thanks for giving attention to that topic. |
|
@muratcorlu you are welcome to share feedback and we're happy to receive it, but please take it down a notch. |
|
Hello @JohnONolan . If I wrote in a disturbing tone, I apologize. Since English is not my native language, I may sometimes have difficulty adjusting the tone. This may result in excessive directness. I will be more careful. |
|
Superseded by #26556 — same intent (remove See the full set of PRs:
|
3 participants
Context
Ghost(Pro) is moving to relative-path admin builds so a single artifact works across staging and production. The
GHOST_CDN_URLconfig option is fully preserved for anyone who uses it — this PR only stops setting it in Ghost(Pro)'s CI build.This PR
Removes the
GHOST_CDN_URLenv var from thedeploy_adminCI job. Without it set, Vite defaults tobase: './'(relative paths) and Ember defaults to relative asset URLs. The existing code paths that readGHOST_CDN_URLare untouched — self-hosters and contributors who set the env var get the same behaviour as before.Depends on BlackPearl's asset proxy being deployed first so that relative paths in
index.htmlresolve correctly via the CDN.