[HKG-1738] Experiment: lazy URL routing service#27701
Closed
allouis wants to merge 14 commits into
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces lazy URL service initialization to Ghost, adding a facade pattern and on-demand URL resolution pathway. The changes include: a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/stats/content-stats-service.js (1)
242-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
urlExistsdefaults tofalsewhen URL service isn't ready, but comment says it should default totrue.Line 243 initializes
urlExists = false, and the comment on line 252 says "default to true (clickable)" when the service isn't ready. However, ifhasFinished()returnsfalse, the code skips the block andurlExistsremainsfalse.🐛 Proposed fix to align behavior with comment
// Check URL existence using the URL service - let urlExists = false; + let urlExists = true; // Default to true (clickable) if URL service check is skipped if (this.urlService && item.pathname) { try { // Check if URL service is ready if (this.urlService.facade.hasFinished && this.urlService.facade.hasFinished()) { const resource = await this.urlService.facade.resolveUrl(item.pathname); urlExists = !!resource; // Convert to boolean } - // If URL service isn't ready, we default to true (clickable) + // If URL service isn't ready, urlExists stays true (clickable) } catch (error) { // If there's an error checking the URL service, default to true urlExists = true; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/stats/content-stats-service.js` around lines 242 - 257, The code currently initializes urlExists = false but the intended default when the URL service isn't ready is true; change the logic so urlExists defaults to true (e.g., initialize urlExists = true) and only overwrite it when this.urlService exists and this.urlService.facade.hasFinished() returns true by calling await this.urlService.facade.resolveUrl(item.pathname) and setting urlExists = !!resource; keep the catch branch setting urlExists = true to preserve the safe default and reference the symbols urlExists, this.urlService, item.pathname, urlService.facade.hasFinished, and resolveUrl to locate where to change the initialization and assignment.
🧹 Nitpick comments (3)
ghost/core/test/unit/frontend/helpers/authors.test.js (1)
116-117: ⚡ Quick winStrengthen stub matching for the facade contract.
These stubs only match
{id}, so tests still pass if the helper stops passingtype: 'authors'or{withSubdirectory: true}. Match both args to lock the migration contract.Suggested tightening pattern
-urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: authors[0].id})).returns('author url 1'); +urlServiceGetUrlForResourceStub + .withArgs( + sinon.match({id: authors[0].id, type: 'authors'}), + sinon.match({withSubdirectory: true}) + ) + .returns('author url 1');Also applies to: 131-132, 145-146, 159-160, 174-175, 190-191, 205-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/frontend/helpers/authors.test.js` around lines 116 - 117, The urlServiceGetUrlForResourceStub stubs only match {id} which lets the helper change its resource shape; update each withArgs call (e.g., urlServiceGetUrlForResourceStub.withArgs(...)) to match both the resource and options arguments explicitly—use sinon.match to assert {id: authors[i].id, type: 'authors'} for the first arg and sinon.match({withSubdirectory: true}) (or the actual options shape used in the helper) for the second arg so the stub enforces the facade contract; apply the same tightening to the other occurrences around the referenced lines.ghost/core/test/unit/frontend/helpers/tags.test.js (1)
104-105: ⚡ Quick winTighten facade stub matching to include resource type.
These stubs currently validate only
id, so a wrong resource type could pass unnoticed. Match{id, type: 'tags'}to keep the migration contract enforced.Suggested test hardening
- urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: tags[0].id})).returns('tag url 1'); - urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: tags[1].id})).returns('tag url 2'); + urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: tags[0].id, type: 'tags'})).returns('tag url 1'); + urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: tags[1].id, type: 'tags'})).returns('tag url 2');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/frontend/helpers/tags.test.js` around lines 104 - 105, Tighten the urlServiceGetUrlForResourceStub expectations to verify both id and resource type so wrong resource types don't match; update the calls that currently use sinon.match({id: tags[0].id}) and sinon.match({id: tags[1].id}) to match an object including type: 'tags' (e.g., sinon.match({id: tags[0].id, type: 'tags'})) so the stub for urlServiceGetUrlForResourceStub only responds for tag resources.ghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.js (1)
29-45: ⚡ Quick winAssert the assigned post URL value, not just property presence.
Object.hasOwn(post, 'url')still passes ifforPost()writesundefinedor the wrong string, so this test no longer pins the facade result the way the tag/user cases do.Suggested fix
- assert(Object.hasOwn(post, 'url')); + assert.equal(post.url, 'getUrlForResource');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.js` around lines 29 - 45, The test currently only checks that forPost assigned a 'url' property but not its value; update the test around the forPost call to stub getUrlForResourceStub to return a known URL (e.g., 'the-url') and then assert that post.url === 'the-url' (in addition to existing checks on resource and options). This uses the existing getUrlForResourceStub and forPost identifiers so the test pins the facade result instead of only verifying the property presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/frontend/meta/url.js`:
- Around line 19-24: The code forces every resource to type 'posts' by creating
postResource = {...data, type: 'posts'}, causing page URLs to be resolved via
the post resolver and fall back to /404/; change this to derive the facade type
from data.type (e.g., use resourceType = data.type === 'page' ? 'pages' :
data.type || 'posts') and build the resource object with that type before
calling urlService.facade.getUrlForResource in both the unpublished-check branch
and the final return so pages preserve their proper resource type.
In `@ghost/core/core/frontend/services/routing/controllers/email-post.js`:
- Line 51: The email-posts serializer currently uses mappers.posts() to build
emailPost but never sets the resource type, causing lazy-url-service's
_routerTypeOf() to return null and getUrlForResource() to produce /404/; fix by
assigning the model's type to the serialized object (add emailPost.type =
model.get('type') immediately after the mappers.posts(...) call in the
email-posts serializer), following the same pattern used in previews.js where
frame.response.previews[0].type is set.
In `@ghost/core/core/server/services/comments/comments-service-emails.js`:
- Line 53: The post passed into getPostUrl (used at postUrl:
this.getPostUrl(post, comment.get('id')) and the other occurrences around lines
115 and 163) may lack relations required for permalink templates; before calling
getPostUrl, ensure the Post model is hydrated with both 'tags' and 'authors'
relations (e.g., fetch or load post.withRelated(['tags','authors']) or await
post.load({relation: 'tags'}); await post.load({relation: 'authors'}) depending
on your data API) so :primary_tag and :primary_author permalink tokens resolve
correctly.
In `@ghost/core/core/server/services/member-attribution/url-translator.js`:
- Around line 162-165: The query in models.Post.findOne currently uses a
hardcoded filter 'type:[post,page]' which allows mismatched id/type pairs to
resolve; update the findOne call inside getResourceDetails (or the function
performing this lookup) to constrain the filter to the caller's requested type
(use the incoming requested type variable, e.g. resource.type or requestedType)
instead of the static [post,page] list so that the query returns null for
mismatched types; ensure the filter passed to models.Post.findOne reflects only
that requested type and keep require:false and the withRelated/tags/authors
options unchanged.
In `@ghost/core/core/server/services/slack.js`:
- Around line 57-60: The code eagerly calls
urlService.facade.getUrlForResource({...post, type: 'posts'}) inside the
hasPostProperties branch which triggers a post URL lookup even for
non-webhook/page early-return cases; change this by short-circuiting non-webhook
or page/default-slug paths before calling getUrlForResource, or alternatively
use the existing post.type to set the resource type (e.g. respect post.type when
building the object passed to getUrlForResource) so you only resolve the URL for
true post resources; update the logic around hasPostProperties, message, and
title to avoid invoking urlService.facade.getUrlForResource until after the
page/default-slug guards.
In `@ghost/core/core/server/services/url/BENCHMARK.md`:
- Line 15: The benchmark table row "Sitemap generation" references an invalid
curl flag `--time`; update that cell to use a valid timing command such as
prefixing with `time curl` or using `curl -w '%{time_total}\n'` so the
documentation shows a correct way to measure request duration for
`/sitemap.xml`, `/sitemap-posts.xml`, etc.; edit the table entry in BENCHMARK.md
replacing `curl --time` with one of these valid options.
---
Outside diff comments:
In `@ghost/core/core/server/services/stats/content-stats-service.js`:
- Around line 242-257: The code currently initializes urlExists = false but the
intended default when the URL service isn't ready is true; change the logic so
urlExists defaults to true (e.g., initialize urlExists = true) and only
overwrite it when this.urlService exists and
this.urlService.facade.hasFinished() returns true by calling await
this.urlService.facade.resolveUrl(item.pathname) and setting urlExists =
!!resource; keep the catch branch setting urlExists = true to preserve the safe
default and reference the symbols urlExists, this.urlService, item.pathname,
urlService.facade.hasFinished, and resolveUrl to locate where to change the
initialization and assignment.
---
Nitpick comments:
In `@ghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.js`:
- Around line 29-45: The test currently only checks that forPost assigned a
'url' property but not its value; update the test around the forPost call to
stub getUrlForResourceStub to return a known URL (e.g., 'the-url') and then
assert that post.url === 'the-url' (in addition to existing checks on resource
and options). This uses the existing getUrlForResourceStub and forPost
identifiers so the test pins the facade result instead of only verifying the
property presence.
In `@ghost/core/test/unit/frontend/helpers/authors.test.js`:
- Around line 116-117: The urlServiceGetUrlForResourceStub stubs only match {id}
which lets the helper change its resource shape; update each withArgs call
(e.g., urlServiceGetUrlForResourceStub.withArgs(...)) to match both the resource
and options arguments explicitly—use sinon.match to assert {id: authors[i].id,
type: 'authors'} for the first arg and sinon.match({withSubdirectory: true}) (or
the actual options shape used in the helper) for the second arg so the stub
enforces the facade contract; apply the same tightening to the other occurrences
around the referenced lines.
In `@ghost/core/test/unit/frontend/helpers/tags.test.js`:
- Around line 104-105: Tighten the urlServiceGetUrlForResourceStub expectations
to verify both id and resource type so wrong resource types don't match; update
the calls that currently use sinon.match({id: tags[0].id}) and sinon.match({id:
tags[1].id}) to match an object including type: 'tags' (e.g., sinon.match({id:
tags[0].id, type: 'tags'})) so the stub for urlServiceGetUrlForResourceStub only
responds for tag resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26dfd48b-be6c-463c-98ff-62ee7195f92d
📒 Files selected for processing (52)
ghost/core/core/boot.jsghost/core/core/bridge.jsghost/core/core/frontend/helpers/authors.jsghost/core/core/frontend/helpers/tags.jsghost/core/core/frontend/meta/author-url.jsghost/core/core/frontend/meta/url.jsghost/core/core/frontend/services/routing/controllers/collection.jsghost/core/core/frontend/services/routing/controllers/email-post.jsghost/core/core/frontend/services/routing/controllers/entry.jsghost/core/core/frontend/services/routing/controllers/previews.jsghost/core/core/frontend/services/routing/router-manager.jsghost/core/core/frontend/services/rss/generate-feed.jsghost/core/core/frontend/services/sitemap/handler.jsghost/core/core/frontend/services/sitemap/site-map-manager.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/url.jsghost/core/core/server/lib/common/to-plain.jsghost/core/core/server/services/audience-feedback/audience-feedback-service.jsghost/core/core/server/services/comments/comments-service-emails.jsghost/core/core/server/services/email-service/email-renderer.jsghost/core/core/server/services/indexnow.jsghost/core/core/server/services/member-attribution/url-translator.jsghost/core/core/server/services/mentions/resource-service.jsghost/core/core/server/services/route-settings/route-settings.jsghost/core/core/server/services/slack.jsghost/core/core/server/services/stats/content-stats-service.jsghost/core/core/server/services/stats/posts-stats-service.jsghost/core/core/server/services/url/BENCHMARK.mdghost/core/core/server/services/url/index.jsghost/core/core/server/services/url/lazy-url-service.jsghost/core/core/server/services/url/url-service-facade.jsghost/core/core/shared/config/defaults.jsonghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.jsghost/core/test/unit/frontend/helpers/authors.test.jsghost/core/test/unit/frontend/helpers/tags.test.jsghost/core/test/unit/frontend/meta/author-url.test.jsghost/core/test/unit/frontend/meta/url.test.jsghost/core/test/unit/frontend/services/routing/controllers/collection.test.jsghost/core/test/unit/frontend/services/routing/controllers/entry.test.jsghost/core/test/unit/frontend/services/rss/generate-feed.test.jsghost/core/test/unit/frontend/services/sitemap/manager.test.jsghost/core/test/unit/server/lib/common/to-plain.test.jsghost/core/test/unit/server/services/audience-feedback/audience-feedback-service.test.jsghost/core/test/unit/server/services/comments/comments-service-emails.test.jsghost/core/test/unit/server/services/indexnow.test.jsghost/core/test/unit/server/services/member-attribution/url-translator.test.jsghost/core/test/unit/server/services/mentions/resource-service.test.jsghost/core/test/unit/server/services/route-settings/route-settings.test.jsghost/core/test/unit/server/services/slack.test.jsghost/core/test/unit/server/services/stats/content.test.jsghost/core/test/unit/server/services/stats/posts.test.jsghost/core/test/unit/server/services/url/lazy-url-service.test.jsghost/core/test/unit/server/services/url/url-service-facade.test.js
Comment on lines
+19
to
+24
| const postResource = {...data, type: 'posts'}; | ||
| if (data.status !== 'published' && urlService.facade.getUrlForResource(postResource) === '/404/') { | ||
| return urlUtils.urlFor({relativeUrl: urlUtils.urlJoin('/p', data.uuid, '/')}, null, absolute); | ||
| } | ||
|
|
||
| return urlService.getUrlByResourceId(data.id, {absolute: absolute, withSubdirectory: true}); | ||
| return urlService.facade.getUrlForResource(postResource, {absolute: absolute, withSubdirectory: true}); |
Contributor
There was a problem hiding this comment.
Preserve page resources instead of forcing everything to 'posts'.
checks.isPost(data) is the shared branch for post/page URLs, but Line 19 overwrites every resource to {type: 'posts'}. With lazy routing enabled, page {{url}} lookups now go through the post resolver and can fall back to /404/ or the wrong permalink. Derive the facade type from data.type before both calls.
Suggested fix
- const postResource = {...data, type: 'posts'};
+ const postResource = {
+ ...data,
+ type: data.type === 'page' ? 'pages' : 'posts'
+ };📝 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.
Suggested change
| const postResource = {...data, type: 'posts'}; | |
| if (data.status !== 'published' && urlService.facade.getUrlForResource(postResource) === '/404/') { | |
| return urlUtils.urlFor({relativeUrl: urlUtils.urlJoin('/p', data.uuid, '/')}, null, absolute); | |
| } | |
| return urlService.getUrlByResourceId(data.id, {absolute: absolute, withSubdirectory: true}); | |
| return urlService.facade.getUrlForResource(postResource, {absolute: absolute, withSubdirectory: true}); | |
| const postResource = { | |
| ...data, | |
| type: data.type === 'page' ? 'pages' : 'posts' | |
| }; | |
| if (data.status !== 'published' && urlService.facade.getUrlForResource(postResource) === '/404/') { | |
| return urlUtils.urlFor({relativeUrl: urlUtils.urlJoin('/p', data.uuid, '/')}, null, absolute); | |
| } | |
| return urlService.facade.getUrlForResource(postResource, {absolute: absolute, withSubdirectory: true}); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/frontend/meta/url.js` around lines 19 - 24, The code forces
every resource to type 'posts' by creating postResource = {...data, type:
'posts'}, causing page URLs to be resolved via the post resolver and fall back
to /404/; change this to derive the facade type from data.type (e.g., use
resourceType = data.type === 'page' ? 'pages' : data.type || 'posts') and build
the resource object with that type before calling
urlService.facade.getUrlForResource in both the unpublished-check branch and the
final return so pages preserve their proper resource type.
|
|
||
| if (post.status === 'published') { | ||
| return urlUtils.redirect301(res, routerManager.getUrlByResourceId(post.id, {withSubdirectory: true})); | ||
| return urlUtils.redirect301(res, routerManager.getUrlForResource(post, {withSubdirectory: true})); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify router manager URL contract + relevant callsites (read-only)
set -euo pipefail
echo "== getUrlForResource definitions and wrappers =="
rg -n -C3 "getUrlForResource\\s*\\(" --type=js ghost/core/core/frontend/services ghost/core/core/server/services
echo
echo "== Email-post controller callsite =="
rg -n -C3 "emailPostController|redirect301\\(|getUrlForResource\\(" --type=js ghost/core/core/frontend/services/routing/controllers/email-post.js
echo
echo "== Post URL callsites that explicitly set type:'posts' (for comparison) =="
rg -n -C2 "type:\\s*['\\\"]posts['\\\"]" --type=js ghost/core/core/frontend ghost/core/core/serverRepository: TryGhost/Ghost
Length of output: 11405
🏁 Script executed:
#!/bin/bash
# Examine post loading in email-post controller + post model structure
set -euo pipefail
echo "== Full email-post controller to see post loading =="
head -60 ghost/core/core/frontend/services/routing/controllers/email-post.js
echo
echo "== Check if post model/API response includes type property =="
rg -n "type.*:.*['\"]posts['\"]|type.*=.*['\"]posts['\"]" --type=js ghost/core/core/server/models -A2 -B2 | head -50
echo
echo "== Previews controller for comparison =="
head -60 ghost/core/core/frontend/services/routing/controllers/previews.jsRepository: TryGhost/Ghost
Length of output: 2122
🏁 Script executed:
#!/bin/bash
# Check router options and post model/API response structure
set -euo pipefail
echo "== Router configuration for email posts =="
rg -n "email|emailPost" --type=js ghost/core/core/frontend/services/routing -B3 -A3 | head -80
echo
echo "== Post model structure / serializer =="
fd -e js "post" ghost/core/core/server/models | head -10
echo
echo "== Check Ghost API response format =="
rg -n "type.*:" --type=ts --type=js ghost/core/core/server/api ghost/core/core/server/services/proxy -B2 -A2 | head -100Repository: TryGhost/Ghost
Length of output: 14946
🏁 Script executed:
#!/bin/bash
# Check posts serializer output and lazy-url-service _routerTypeOf implementation
set -euo pipefail
echo "== Posts output serializer to see if type is included =="
rg -n "output.*posts|type.*:" --type=js ghost/core/core/server/api/endpoints/utils/serializers/output/posts.js | head -50
echo
echo "== Lazy URL service _routerTypeOf implementation =="
rg -n "_routerTypeOf" --type=js ghost/core/core/server/services/url/lazy-url-service.js -A15
echo
echo "== Check common posts serialization fields =="
cat ghost/core/core/server/api/endpoints/utils/serializers/output/posts.js | head -100Repository: TryGhost/Ghost
Length of output: 5282
🏁 Script executed:
#!/bin/bash
# Check posts mapper output + TYPE_TO_ROUTER_TYPE mapping
set -euo pipefail
echo "== Posts mapper to see what fields are serialized =="
fd -e js mappers.js ghost/core/core/server/api/endpoints/utils/serializers/output/
cat ghost/core/core/server/api/endpoints/utils/serializers/output/mappers.js | head -150
echo
echo "== TYPE_TO_ROUTER_TYPE mapping in lazy-url-service =="
rg -n "TYPE_TO_ROUTER_TYPE" --type=js ghost/core/core/server/services/url/lazy-url-service.js -B5 -A10Repository: TryGhost/Ghost
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Find mappers file and check TYPE_TO_ROUTER_TYPE
set -euo pipefail
echo "== Find mappers file =="
find ghost/core/core/server/api/endpoints/utils/serializers -name "*mapper*" -type f
echo
echo "== Posts serializer output mapping =="
rg -n "mappers\.posts" --type=js ghost/core/core/server/api/endpoints/utils/serializers/output/ -B5 -A10
echo
echo "== TYPE_TO_ROUTER_TYPE mapping =="
rg -n "TYPE_TO_ROUTER_TYPE\s*=" --type=js ghost/core/core/server/services/url/ -A15Repository: TryGhost/Ghost
Length of output: 8766
🏁 Script executed:
#!/bin/bash
# Check if email-posts serializer explicitly adds type field
set -euo pipefail
echo "== Full email-posts serializer =="
cat ghost/core/core/server/api/endpoints/utils/serializers/output/email-posts.js
echo
echo "== Full previews serializer for comparison =="
cat ghost/core/core/server/api/endpoints/utils/serializers/output/previews.js
echo
echo "== Check if mappers.posts includes type =="
rg -n "mappers\s*=" --type=js ghost/core/core/server/api/endpoints/utils/serializers/output/ -A5 | head -20Repository: TryGhost/Ghost
Length of output: 4786
Add missing type field to email-posts serializer output.
At line 51, post is passed to getUrlForResource() without an explicit type property. The email-posts serializer (output/email-posts.js) calls mappers.posts() but—unlike previews.js which explicitly sets frame.response.previews[0].type = model.get('type')—never assigns the type field. When post.type is undefined, _routerTypeOf() in lazy-url-service returns null, causing getUrlForResource() to return /404/ instead of the post URL. Published email-only posts will incorrectly redirect to 404.
Add emailPost.type = model.get('type'); in ghost/core/core/server/api/endpoints/utils/serializers/output/email-posts.js after the mappers.posts() call, matching the pattern in previews.js.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/frontend/services/routing/controllers/email-post.js` at line
51, The email-posts serializer currently uses mappers.posts() to build emailPost
but never sets the resource type, causing lazy-url-service's _routerTypeOf() to
return null and getUrlForResource() to produce /404/; fix by assigning the
model's type to the serialized object (add emailPost.type = model.get('type')
immediately after the mappers.posts(...) call in the email-posts serializer),
following the same pattern used in previews.js where
frame.response.previews[0].type is set.
| siteDomain: this.siteDomain, | ||
| postTitle: post.get('title'), | ||
| postUrl: this.getPostUrl(post.get('id'), comment.get('id')), | ||
| postUrl: this.getPostUrl(post, comment.get('id')), |
Contributor
There was a problem hiding this comment.
Load tags and authors before resolving these email URLs.
These call sites now hand the facade whatever was loaded on the post model, but the surrounding queries still fetch either no relations or only authors. In lazy routing, permalink templates that depend on :primary_tag/:primary_author can therefore produce broken links here. Please hydrate both relations before passing the post into getPostUrl().
Also applies to: 115-115, 163-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/services/comments/comments-service-emails.js` at line
53, The post passed into getPostUrl (used at postUrl: this.getPostUrl(post,
comment.get('id')) and the other occurrences around lines 115 and 163) may lack
relations required for permalink templates; before calling getPostUrl, ensure
the Post model is hydrated with both 'tags' and 'authors' relations (e.g., fetch
or load post.withRelated(['tags','authors']) or await post.load({relation:
'tags'}); await post.load({relation: 'authors'}) depending on your data API) so
:primary_tag and :primary_author permalink tokens resolve correctly.
Comment on lines
+162
to
+165
| const post = await this.models.Post.findOne({id}, { | ||
| require: false, | ||
| filter: 'type:[post,page]+status:[published,sent]', | ||
| withRelated: ['tags', 'authors'] |
Contributor
There was a problem hiding this comment.
Filter post/page lookups by the requested type.
Using type:[post,page] here means a stale or malformed {id, type} pair can still resolve a row from posts, and getResourceDetails() will then report the caller’s requested type with the wrong URL instead of returning null. This should fail closed by constraining the query to the requested type.
Suggested fix
- const post = await this.models.Post.findOne({id}, {
- require: false,
- filter: 'type:[post,page]+status:[published,sent]',
- withRelated: ['tags', 'authors']
- });
+ const post = await this.models.Post.findOne({id}, {
+ require: false,
+ filter: `type:[${type}]+status:[published,sent]`,
+ withRelated: ['tags', 'authors']
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/services/member-attribution/url-translator.js` around
lines 162 - 165, The query in models.Post.findOne currently uses a hardcoded
filter 'type:[post,page]' which allows mismatched id/type pairs to resolve;
update the findOne call inside getResourceDetails (or the function performing
this lookup) to constrain the filter to the caller's requested type (use the
incoming requested type variable, e.g. resource.type or requestedType) instead
of the static [post,page] list so that the query returns null for mismatched
types; ensure the filter passed to models.Post.findOne reflects only that
requested type and keep require:false and the withRelated/tags/authors options
unchanged.
Comment on lines
57
to
60
| // If this is a post, we want to send the link of the post | ||
| if (hasPostProperties(post)) { | ||
| message = urlService.getUrlByResourceId(post.id, {absolute: true}); | ||
| message = urlService.facade.getUrlForResource({...post, type: 'posts'}, {absolute: true}); | ||
| title = post.title ? post.title : null; |
Contributor
There was a problem hiding this comment.
Short-circuit non-webhook posts before resolving a post URL.
Line 59 eagerly resolves {...post, type: 'posts'} before the later page/default-slug guards. With lazy routing on, page publishes now hit the post resolver with the wrong resource type on a path whose result is thrown away. Move the early returns above the lookup, or derive the resource type from post.type here.
The page/welcome tests in this PR will need to stop asserting that the lookup happens on those early-return paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/services/slack.js` around lines 57 - 60, The code
eagerly calls urlService.facade.getUrlForResource({...post, type: 'posts'})
inside the hasPostProperties branch which triggers a post URL lookup even for
non-webhook/page early-return cases; change this by short-circuiting non-webhook
or page/default-slug paths before calling getUrlForResource, or alternatively
use the existing post.type to set the resource type (e.g. respect post.type when
building the object passed to getUrlForResource) so you only resolve the URL for
true post resources; update the logic around hasPostProperties, message, and
title to avoid invoking urlService.facade.getUrlForResource until after the
page/default-slug guards.
| | Browse endpoint response time | `autocannon` | `/ghost/api/content/posts/?limit=15` | | ||
| | Single post read | `autocannon` | `/ghost/api/content/posts/slug/<known-slug>/` | | ||
| | Page render | `autocannon` | `/<slug>/` (frontend) | | ||
| | Sitemap generation | one-shot `curl --time` | `/sitemap.xml`, `/sitemap-posts.xml`, etc. | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify available curl timing-related options/help text
curl --help all | rg -n -- '--time|-w|--write-out'Repository: TryGhost/Ghost
Length of output: 327
Replace invalid curl --time with a valid timing command.
The benchmark table references curl --time, which is not a valid curl option. Use time curl or curl -w '%{time_total}\n' to measure execution time.
Suggested doc fix
-| Sitemap generation | one-shot `curl --time` | `/sitemap.xml`, `/sitemap-posts.xml`, etc. |
+| Sitemap generation | one-shot `time curl` (or `curl -w '%{time_total}\n'`) | `/sitemap.xml`, `/sitemap-posts.xml`, etc. |📝 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.
Suggested change
| | Sitemap generation | one-shot `curl --time` | `/sitemap.xml`, `/sitemap-posts.xml`, etc. | | |
| | Sitemap generation | one-shot `time curl` (or `curl -w '%{time_total}\n'`) | `/sitemap.xml`, `/sitemap-posts.xml`, etc. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/services/url/BENCHMARK.md` at line 15, The benchmark
table row "Sitemap generation" references an invalid curl flag `--time`; update
that cell to use a valid timing command such as prefixing with `time curl` or
using `curl -w '%{time_total}\n'` so the documentation shows a correct way to
measure request duration for `/sitemap.xml`, `/sitemap-posts.xml`, etc.; edit
the table entry in BENCHMARK.md replacing `curl --time` with one of these valid
options.
allouis
force-pushed
the
hkg-1738-lazy-routes
branch
from
fc5e079 to
a56f44f
Compare
Collaborator
Author
|
Force-pushed: rebased onto latest main + fixed a regression in the API URL serializer where |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 25432072994 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
allouis
force-pushed
the
hkg-1738-lazy-routes
branch
from
a56f44f to
6e33a8a
Compare
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 25433247952 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
allouis
force-pushed
the
hkg-1738-lazy-routes
branch
from
6e33a8a to
8e21dfa
Compare
ref https://linear.app/ghost/issue/HKG-1761/ Lands on top of main: characterises the existing helper-passes-id behaviour through the public notify* methods so that HKG-1761's switch from id-only to passing the post object is caught if it ever regresses. Independently mergeable on top of main without the rest of the lazy-routing stack.
ref https://linear.app/ghost/issue/HKG-1761/ The lazy URL service evaluates permalink templates against the resource itself (slug, published_at, primary_tag, ...). The old contract, `getUrlByResourceId(post.id)`, only carries an id; passing the full post object is the prerequisite for letting the lazy backend reach those fields without an extra DB lookup. Eager-mode behaviour is unchanged. Adds a tiny `toPlain(modelOrObj)` helper so callers can hand in either a Bookshelf model instance or an already-serialised hash and the URL helpers get a plain object regardless. Without this, `{...model}` would silently drop prototype-defined fields like `id` on Bookshelf models.
allouis
force-pushed
the
hkg-1738-lazy-routes
branch
from
8e21dfa to
5c614c2
Compare
ref https://linear.app/ghost/issue/HKG-1763/ The facade gives every caller a stable, resource-based interface (`getUrlForResource(resource, options)`, `ownsResource(routerId, resource)`, `resolveUrl(path)`, `getResourceById(id)`) so we can later swap the eager precomputing UrlService for an on-demand backend without touching every caller. This commit only introduces the facade; subsequent commits move callers across, and HKG-1771 wires in the lazy backend behind a config flag. Eager behaviour is unchanged.
ref https://linear.app/ghost/issue/HKG-1764/ RouterManager now holds a reference to the facade rather than the raw eager UrlService, and the routing controllers (entry, channel, collection, static, taxonomy, previews, email-post) plus rss/generate-feed go through `routerManager.getUrlForResource(resource, ...)` / `ownsResource(routerId, resource)`. This is the first batch of caller migrations from the id-based `getUrlByResourceId` API to the resource-based facade — required so the lazy backend can later read permalink-template fields off the resource without hitting the DB twice.
ref https://linear.app/ghost/issue/HKG-1765/ `output/utils/url.js`'s `forPost`/`forUser`/`forTag` helpers now call `urlService.facade.getUrlForResource({...attrs, id, type}, ...)` instead of `urlService.getUrlByResourceId(id, ...)`. The explicit `id` is critical: Content API requests like `?fields=url` strip every attribute except url, so a plain `{...attrs, type}` would send an id-less resource and the eager facade's id-based fallback would return `/404/` for every record. Unit tests pin both the standard call and the stripped-attrs case so the regression class is caught at the serializer boundary.
ref https://linear.app/ghost/issue/HKG-1766/ `{{tags}}` / `{{authors}}` helpers and the `meta/url` and `meta/author-url` generators now go through `urlService.facade.getUrlForResource(resource, ...)` rather than `urlService.getUrlByResourceId(id, ...)`. Theme rendering hands us the full resource object, so spreading it into the facade is a direct fit and matches the contract the lazy backend will rely on. Drops three router-manager pass-throughs (`owns`, `getUrlByResourceId`, the no-longer-used routerManager-level `getResourceById` was kept on the facade for the entry controller's resource-type check) along with their last callers.
ref https://linear.app/ghost/issue/HKG-1767/ Lands on top of main: characterises the existing IndexNow ping URL via the public `indexnowListener` so the HKG-1767 migration of the URL helper lookup is caught if it regresses. Independently mergeable on top of main without the rest of the lazy-routing stack.
ref https://linear.app/ghost/issue/HKG-1767/ Last batch of `getUrlByResourceId(id)` callers in the backend: the slack notifier, the IndexNow notifier, the comments-emails service, and the audience-feedback service now call `urlService.facade.getUrlForResource` with a full resource. Each call site already has the model in hand so the spread is direct. After this commit no in-tree caller invokes `urlService.getUrlByResourceId` on the eager service; the legacy method only survives behind the facade as the eager fall-through implementation of `getUrlForResource`.
ref https://linear.app/ghost/issue/HKG-1768/ Lands on top of main: pins the case where `getResource(path)` returns undefined so `_enrichWithTitles` records `url_exists === false`. The existing test only exercised the truthy branch, leaving HKG-1768's switch from `getResource` → `facade.resolveUrl` (which becomes async) without guard against a regression on the falsy half.
ref https://linear.app/ghost/issue/HKG-1768/ `url-translator.getTypeAndIdFromPath`, `posts-stats-service`, `content-stats-service`, and `mentions/resource-service` now consume the flat resource shape returned by `urlService.facade.resolveUrl(path)` instead of the legacy `{config: {type}, data: {...}}` envelope from `urlService.getResource(path)`. The translator and content-stats helper become async to match resolveUrl's contract; their direct callers are already in async contexts so the ripple stops there. The facade also resolves an inconsistency: the routing-level type ('posts'/'pages'/'tags'/'authors') wins over any DB type field on the underlying resource data, so the flat resource is unambiguous.
ref https://linear.app/ghost/issue/HKG-1769/ The eager UrlService fires `url.added` / `url.removed` / `URLResourceUpdatedEvent` events that the sitemap subscribes to. The upcoming lazy backend doesn't precompute URLs and therefore cannot fire those events, so the sitemap would stay empty in lazy mode. When `config.lazyRouting` is on, `SiteMapManager` skips the URL service event subscriptions and lazily populates from the database on first XML request. Per-type browse options mirror the eager URL service's resource queries (`services/url/config.js`): posts/pages by `status:published+type:`, tags/authors by `visibility:public`, plus `withRelated: ['tags', 'authors']` for posts so `:primary_tag`/`:primary_author` permalinks resolve. A generation token + identity-checked promise handle protects the populate from `routers.reset` races: a stale settle never marks the manager populated, never clobbers a successor populate's handle. Static route + collection index URLs continue to come from the `router.created` event, which fires from RouterManager regardless of mode.
ref https://linear.app/ghost/issue/HKG-1770/ Pure on-demand UrlService implementation that backs the facade in lazy mode. `getUrlForResource` iterates collection routers in priority order, evaluates the NQL filter against the resource (using the same EXPANSIONS table and `page:true|false` transformer as UrlGenerator), and applies the permalink template via `url-utils.replacePermalink`. No match → `/404/`. `ownsResource` is the single-router NQL match. `resolveUrl` pattern-matches a URL path against each router's permalink, calls an injected `findResource` hook to look the record up by slug, and re-verifies the NQL filter for posts. Pure callers don't need findResource. `hasFinished` returns true. The class is not yet wired into the lifecycle; HKG-1771 picks the implementation off the lazyRouting flag.
ref https://linear.app/ghost/issue/HKG-1771/ Final wire-up: when `config.lazyRouting` is true (default false), `services/url/index.js` instantiates a LazyUrlService with a model-backed `findResource` hook and attaches it to the facade alongside the eager service. The facade's `isLazy()` helper lets the lifecycle code branch. Lifecycle in lazy mode: - `bridge.reloadFrontend()` skips `urlService.queue.start` (nothing to precompute). - `boot.js` skips `urlService.init()` and the `urlService.shutdown` cleanup task. - `route-settings.setFromFilePath()` drops generators via `facade.reset()` and skips the `hasFinished()` readiness poll, since the lazy service is always finished. Flag-off behaviour is unchanged.
ref https://linear.app/ghost/issue/HKG-1772/ Documents what to measure (browse / single-read / page-render latency, RSS, sitemap, boot time), the dataset (a 250K-post production clone with default and complex `routes.yaml`), the autocannon-based procedure, and the working-hypothesis thresholds that distinguish "ship as-is" from "add memoization (Approach 2)". Execution and results capture happen against real infrastructure and are tracked in the Linear project document, not in this repo.
allouis
force-pushed
the
hkg-1738-lazy-routes
branch
from
5c614c2 to
0b65556
Compare
Collaborator
Author
|
Superseded by stacked PRs:
The three test PRs are independently mergeable on The |
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
1 participant
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.
Summary
Experimental opt-in lazy URL routing service, gated behind
config.lazyRouting(defaultfalse). When the flag is on, the existing eagerUrlService(which preloads every post/page/tag/author at boot) is replaced by a pure on-demandLazyUrlServicethat resolves URLs from the DB the first time each one is asked for. The flag-off path is the existing eager service unchanged.This is a multi-issue stack (HKG-1738). 15 commits, each independently reviewable. Three of them are pure-tests-on-top-of-main commits inserted before the impl they exercise (TDD discipline + independently mergeable).
Stack
UrlServiceFacadetest: pin IndexNow ping URL output)test: pin posts-stats urlExists false branch)lazyRoutingis onLazyUrlServiceclassLazyUrlServiceinto the boot/reset lifecycle behind the flagWhat this changes for flag-off users (default)
Behaviour-preserving migrations only. Backend services and routing controllers now talk to a thin facade (
urlService.facade) that delegates to the existing eagerUrlService. No semantic change for any flag-off path; no callers were removed. Two refactor-only patches add atoPlain()helper for Bookshelf-or-plain inputs and tighten the entry-controller type check.What flips on with
config.lazyRouting: true(opt-in)UrlServiceFacaderoutes throughLazyUrlServiceinstead of the eager service. EachgetUrlForResource/resolveUrlwalks the registered routers' permalink templates against the resource (orfindResource()for path → resource) on demand.SiteMapManagerpopulates itself from the DB on first request (per type, paginated) instead of waiting forurl.added/url.removedevents. Mirrors the eager URL service's resource queries: posts/pages bystatus:published+type:, tags/authors byvisibility:public. Posts are loaded withwithRelated: ['tags', 'authors']so:primary_tag/:primary_authorpermalinks resolve.bridge.start()skipsurlService.queue.start(no more 8-stage URL boot pipeline).route-settings.setFromFilePathcallsfacade.reset()and skips the readiness poll.Production safety
false; flag-off path is unchanged. Verified by 4 review passes.models.TagPublicandmodels.Author(the existingshouldHavePosts-gated models) so the sitemap doesn't leak staff-only User accounts. Tags/authors also filtered byvisibility:publicto mirror the eager service._populateFromDatabaseguards a re-entry race: ifrouters.resetfires mid-populate, the in-flight result is invalidated via a generation token, and a stale settle is prevented from clobbering a successor populate's handle.Test plan
cd ghost/core && pnpm lint— 0 errors, 2 pre-existing warningscd ghost/core && pnpm test:single test/unit/server/services— 3168 passingcd ghost/core && pnpm test:single test/unit/frontend— 1769 passingto-plain.test.js(5),lazy-url-service.test.js(26),url-service-facade.test.js(15), sitemapmanager.test.js(15),route-settings.test.js,url-translator.test.js(29)config.lazyRouting: true, hit/, RSS, sitemap, member-attribution URL)Reviewer notes
Stacked PR. Read the commits in order — each migration is small and isolated. The two tests-on-top-of-main commits are intentional: they characterise pre-existing behaviour through the public methods that the migrations touch, so they're independently mergeable on top of
mainif the rest of the stack ever changes shape.Six rounds of multi-agent review (clean-code / production-safety / bug+security / test-quality) prior to opening this PR. CodeRabbit will get the next pass.