Introduced UrlServiceFacade and migrated callers#27763
Merged
Merged
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:
WalkthroughThis PR migrates Ghost from an ID-based URL service API to a resource-object facade. It adds UrlServiceFacade (exposed as urlService.facade), a toPlain helper, updates boot/bridge/app/router wiring, converts frontend helpers/controllers and server services to use facade.getUrlForResource/resolveUrl/ownsResource, adapts stats enrichment to async resolveUrl, updates API serializers/mappers to pass router type, and revises many unit tests to stub/assert the new facade API. Possibly related PRs
🚥 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 |
allouis
force-pushed
the
hkg-1738-test-posts-stats-url
branch
from
5f9db53 to
95388f2
Compare
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
6baedce to
274fd22
Compare
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
274fd22 to
f3a6dce
Compare
![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: 4
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/indexnow.js (1)
87-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider guarding against a falsy URL before making the IndexNow request.
If
getUrlForResourcereturnsundefined(URL service not yet warm),URL.searchParams.set('url', url)silently coerces the value to the string"undefined", producing an unnecessary outbound request that will always 422. A cheap guard avoids the noise:🛡️ Proposed guard
const url = urlService.facade.getUrlForResource({...post, type: 'posts'}, {absolute: true}); + if (!url) { + logging.warn('IndexNow: Could not resolve URL for post, skipping ping'); + return; + } // Get the API key (auto-generated on boot by settings service)This behaviour also existed with the legacy
getUrlByResourceId, so this is not a regression — but the facade migration is a good moment to add the guard.🤖 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/indexnow.js` around lines 87 - 101, Guard against a falsy URL returned by urlService.facade.getUrlForResource before building the IndexNow request: after calling urlService.facade.getUrlForResource({...post, type: 'posts'}, {absolute: true}) and storing it in the url variable, check if url is falsy (undefined/null/empty) and log or return early to avoid calling indexNowUrl.searchParams.set('url', url) with an invalid value; update the surrounding logic in indexnow.js (where getUrlForResource, url, siteUrl and indexNowUrl are used) to skip sending the IndexNow request when url is not present.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/url/url-service-facade.test.js (1)
55-68: ⚡ Quick win
resolveUrlflatten test doesn't cover thedata.typeoverride case.The PR explicitly states "routing-level type wins over any DB type field in resolved resources." In production, the eager
UrlServicereturns resources as{config: {type: 'posts'}, data: {type: 'post', id: ...}}—datacarries a DB-level singulartype. The current test uses adataobject without atypefield, so both{type: config.type, ...data}(wrong) and{...data, type: config.type}(correct) would pass.Add a case that asserts
config.typewins over a conflictingdata.type:✅ Suggested additional test case
it('flattens the legacy {config, data} envelope into a Resource', async function () { urlService.getResource.returns({ config: {type: 'posts'}, data: {id: 'abc', slug: 'hello-world', title: 'Hello'} }); const result = await facade.resolveUrl('/hello-world/'); assert.deepEqual(result, { type: 'posts', id: 'abc', slug: 'hello-world', title: 'Hello' }); }); + + it('routing-level config.type wins over DB-level data.type', async function () { + urlService.getResource.returns({ + config: {type: 'posts'}, + data: {id: 'abc', slug: 'hello-world', type: 'post'} // DB stores singular 'post' + }); + + const result = await facade.resolveUrl('/hello-world/'); + + assert.equal(result.type, 'posts'); // config.type must win + });🤖 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/server/services/url/url-service-facade.test.js` around lines 55 - 68, The test for facade.resolveUrl must assert that the routing-level config.type overrides any conflicting data.type: update the unit test that stubs urlService.getResource (in ghost/core/test/unit/server/services/url/url-service-facade.test.js) to return data containing a type (e.g. data: {type: 'post', id: 'abc', slug: 'hello-world', title: 'Hello'}) and a config.type of 'posts', then call facade.resolveUrl('/hello-world/') and assert the resulting Resource has type === config.type ('posts') while preserving other data fields; this ensures resolveUrl uses config.type over data.type when flattening the {config, data} envelope.ghost/core/core/server/services/url/url-service-facade.ts (1)
125-132: 💤 Low value
getResourceByIdsilently bypasses the lazy backend.When
lazyUrlServiceis active,getResourceByIdstill delegates to the eagerurlService. BecauseLazyUrlServiceBackendhas no corresponding method, there is no compile-time reminder to implement it before removing the eager service. Adding an optionalgetResourceByIdtoLazyUrlServiceBackendwould make the gap visible:♻️ Proposed addition to interface
export interface LazyUrlServiceBackend { getUrlForResource(resource: Resource, options?: UrlOptions): string; ownsResource(routerId: string, resource: Resource): boolean; resolveUrl(path: string): Promise<Resource | null>; hasFinished(): boolean; onRouterAddedType(...args: unknown[]): unknown; onRouterUpdated(...args: unknown[]): unknown; reset(): void; + getResourceById?(resourceId: string): LegacyResourceEnvelope | null; }Then in
getResourceById:getResourceById(resourceId: string): LegacyResourceEnvelope | null { + if (this.lazyUrlService?.getResourceById) { + return this.lazyUrlService.getResourceById(resourceId); + } return this.urlService.getResourceById(resourceId); }🤖 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/url-service-facade.ts` around lines 125 - 132, getResourceById currently always calls the eager urlService and thus silently bypasses the lazy backend; add an optional getResourceById(resourceId: string): LegacyResourceEnvelope | null to the LazyUrlServiceBackend interface and update the URL service facade's getResourceById to first call lazyUrlService.getResourceById if present and return its result, otherwise fall back to urlService.getResourceById so callers keep receiving the LegacyResourceEnvelope | null contract.
🤖 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/server/api/endpoints/utils/serializers/output/utils/url.js`:
- Around line 6-15: The router type is derived from possibly filtered attrs
(const type = attrs.type === 'page' ? 'pages' : 'posts') which breaks when
?fields=url strips attrs.type; instead obtain the resource type from the
unfiltered context or an explicit parameter passed into forPost (do not rely on
attrs.type). Update the forPost call/site to accept a resourceType (or use the
original unfiltered attrs variable) and then call
urlService.facade.getUrlForResource({...attrs, id, type: resourceType === 'page'
? 'pages' : 'posts'}, {absolute: true}) so pages always resolve against the
pages router rather than falling back to posts.
In `@ghost/core/core/server/services/stats/content-stats-service.js`:
- Around line 248-250: The current check using
this.urlService.facade.hasFinished() leaves urlExists false when the facade
isn't ready, breaking the intended "service not ready" clickable fallback;
update the branch in the loop handling url resolution so that when
this.urlService.facade.hasFinished exists but returns false you explicitly set
urlExists = true and skip calling
this.urlService.facade.resolveUrl(item.pathname); otherwise keep the existing
behavior of awaiting resolveUrl and setting urlExists = !!resource. This touches
the code around this.urlService.facade.hasFinished, resolveUrl, and the
urlExists variable.
In `@ghost/core/core/server/services/url/url-service-facade.ts`:
- Around line 111-123: The resolveUrl method casts a merged object to Resource
without verifying resource.data.id is present and a string; update either the
LegacyResourceEnvelope type to require id (e.g. make data: Record<string,
unknown> & {id: string}) or add a runtime guard inside resolveUrl: after const
resource = this.urlService.getResource(urlPath) validate typeof
resource.data?.id === 'string' (and return null or handle error if not) before
returning Object.assign({}, resource.data, {type: resource.config.type}) as
Resource so callers can safely rely on result.id; reference resolveUrl,
urlService.getResource, LegacyResourceEnvelope and resource.data when making the
change.
- Line 72: The file exports UrlServiceFacade twice in incompatible ways: it
defines a named TypeScript export (export class UrlServiceFacade) but also does
module.exports = UrlServiceFacade, which overwrites the exports object and
breaks named/TS imports; replace the CommonJS assignment with the
TS/Node-compatible pattern by removing module.exports = UrlServiceFacade and
changing the export to use the export = UrlServiceFacade form (keeping existing
interfaces/type declarations as-is) so both require(...) and import {
UrlServiceFacade } work correctly; ensure no other module.exports/exports
assignments remain for UrlServiceFacade.
---
Outside diff comments:
In `@ghost/core/core/server/services/indexnow.js`:
- Around line 87-101: Guard against a falsy URL returned by
urlService.facade.getUrlForResource before building the IndexNow request: after
calling urlService.facade.getUrlForResource({...post, type: 'posts'}, {absolute:
true}) and storing it in the url variable, check if url is falsy
(undefined/null/empty) and log or return early to avoid calling
indexNowUrl.searchParams.set('url', url) with an invalid value; update the
surrounding logic in indexnow.js (where getUrlForResource, url, siteUrl and
indexNowUrl are used) to skip sending the IndexNow request when url is not
present.
---
Nitpick comments:
In `@ghost/core/core/server/services/url/url-service-facade.ts`:
- Around line 125-132: getResourceById currently always calls the eager
urlService and thus silently bypasses the lazy backend; add an optional
getResourceById(resourceId: string): LegacyResourceEnvelope | null to the
LazyUrlServiceBackend interface and update the URL service facade's
getResourceById to first call lazyUrlService.getResourceById if present and
return its result, otherwise fall back to urlService.getResourceById so callers
keep receiving the LegacyResourceEnvelope | null contract.
In `@ghost/core/test/unit/server/services/url/url-service-facade.test.js`:
- Around line 55-68: The test for facade.resolveUrl must assert that the
routing-level config.type overrides any conflicting data.type: update the unit
test that stubs urlService.getResource (in
ghost/core/test/unit/server/services/url/url-service-facade.test.js) to return
data containing a type (e.g. data: {type: 'post', id: 'abc', slug:
'hello-world', title: 'Hello'}) and a config.type of 'posts', then call
facade.resolveUrl('/hello-world/') and assert the resulting Resource has type
=== config.type ('posts') while preserving other data fields; this ensures
resolveUrl uses config.type over data.type when flattening the {config, data}
envelope.
🪄 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: bc39c425-0eda-45b7-b7c9-c76d749adaab
📒 Files selected for processing (41)
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/previews.jsghost/core/core/frontend/services/routing/router-manager.jsghost/core/core/frontend/services/rss/generate-feed.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/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/index.jsghost/core/core/server/services/url/url-service-facade.tsghost/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/rss/generate-feed.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/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/url-service-facade.test.js
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
f3a6dce to
6347435
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/unit/frontend/helpers/tags.test.js (1)
104-105: ⚡ Quick winTighten stub expectations to include resource type and options.
These stubs currently only assert
id, so they won’t catch regressions in{type: 'tags'}or{withSubdirectory: true}passed by the helper.Proposed assertion tightening
- urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: tags[0].id})).returns('tag url 1'); + urlServiceGetUrlForResourceStub + .withArgs( + sinon.match({id: tags[0].id, type: 'tags'}), + sinon.match({withSubdirectory: true}) + ) + .returns('tag url 1');🤖 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 the resource type and subdirectory option along with id: update the withArgs call(s) for urlServiceGetUrlForResourceStub so the sinon.match includes {id: tags[0].id, type: 'tags', withSubdirectory: true} (and similarly for tags[1]) so the test will fail if the helper stops passing type: 'tags' or withSubdirectory: true to urlService.getUrlForResource.ghost/core/test/unit/server/services/mentions/resource-service.test.js (1)
6-25: ⚡ Quick winAdd a test case for the
pagesresource type.The
pagesbranch (resource-service.js lines 40–45) is exercised by neither the existing post test nor the tag/null tests. Apagesinput that resolves to{ type: 'pages', id: '...' }should return{ type: 'page', id: <ObjectID> }, but there's currently no test pinning that.✅ Suggested addition to `buildUrlServiceWithStubbedFacade`
resolveUrl.withArgs('/tag-resource').resolves({ type: 'tags', id: '63ce473f992390b739b00b02' }); + +resolveUrl.withArgs('/page-resource').resolves({ + type: 'pages', + id: '63ce473f992390b739b00b03' +});And a new test:
it('Correctly converts page resources', async function () { const urlUtils = new UrlUtils({ getSiteUrl() { return 'https://site.com/blah/'; }, getSubdir() { return '/blah'; }, getAdminUrl() { return 'https://admin.com'; } }); const {urlService, resolveUrl} = buildUrlServiceWithStubbedFacade(); const resourceService = new ResourceService({urlUtils, urlService}); const result = await resourceService.getByURL( new URL('https://site.com/blah/page-resource') ); sinon.assert.calledWithExactly(resolveUrl, '/page-resource'); assert.equal(result.type, 'page'); assert.equal(result.id.toHexString(), '63ce473f992390b739b00b03'); });🤖 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/server/services/mentions/resource-service.test.js` around lines 6 - 25, The tests don't cover the "pages" branch in resource-service.js; update buildUrlServiceWithStubbedFacade to stub resolveUrl.withArgs('/page-resource') to resolve {type: 'pages', id: '63ce473f992390b739b00b03'}, then add a unit test that constructs UrlUtils, creates ResourceService with the stubbed urlService, calls resourceService.getByURL(new URL('https://site.com/blah/page-resource')), asserts resolveUrl was called with '/page-resource', and asserts the returned result.type === 'page' and result.id.toHexString() === '63ce473f992390b739b00b03' to verify pages are converted to singular 'page' and ID is parsed.
🤖 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/server/services/member-attribution/url-translator.js`:
- Around line 102-103: getTypeAndIdFromPath currently awaits
this.urlService.facade.resolveUrl(path) which can reject during routing rebuilds
and should not bubble up; wrap the resolveUrl call in a try/catch inside
getTypeAndIdFromPath (or the surrounding getResourceDetails flow) and on a
non-ready/rebuild rejection return nothing (undefined/null) so the existing URL
fallback ({type: 'url', id: null, url: path}) can run; ensure you only swallow
the specific readiness/rebuilding error from urlService.facade.resolveUrl and
rethrow other unexpected errors.
---
Nitpick comments:
In `@ghost/core/test/unit/frontend/helpers/tags.test.js`:
- Around line 104-105: Tighten the urlServiceGetUrlForResourceStub expectations
to verify the resource type and subdirectory option along with id: update the
withArgs call(s) for urlServiceGetUrlForResourceStub so the sinon.match includes
{id: tags[0].id, type: 'tags', withSubdirectory: true} (and similarly for
tags[1]) so the test will fail if the helper stops passing type: 'tags' or
withSubdirectory: true to urlService.getUrlForResource.
In `@ghost/core/test/unit/server/services/mentions/resource-service.test.js`:
- Around line 6-25: The tests don't cover the "pages" branch in
resource-service.js; update buildUrlServiceWithStubbedFacade to stub
resolveUrl.withArgs('/page-resource') to resolve {type: 'pages', id:
'63ce473f992390b739b00b03'}, then add a unit test that constructs UrlUtils,
creates ResourceService with the stubbed urlService, calls
resourceService.getByURL(new URL('https://site.com/blah/page-resource')),
asserts resolveUrl was called with '/page-resource', and asserts the returned
result.type === 'page' and result.id.toHexString() ===
'63ce473f992390b739b00b03' to verify pages are converted to singular 'page' and
ID is parsed.
🪄 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: 799205a4-8369-4261-b328-ab04a03ea8d3
📒 Files selected for processing (41)
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/previews.jsghost/core/core/frontend/services/routing/router-manager.jsghost/core/core/frontend/services/rss/generate-feed.jsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.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/indexnow.jsghost/core/core/server/services/member-attribution/url-translator.jsghost/core/core/server/services/mentions/resource-service.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/index.jsghost/core/core/server/services/url/url-service-facade.tsghost/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/rss/generate-feed.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/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/url-service-facade.test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/core/core/frontend/meta/author-url.js
- ghost/core/core/server/services/slack.js
- ghost/core/core/frontend/meta/url.js
- ghost/core/core/server/lib/common/to-plain.js
- ghost/core/test/unit/frontend/helpers/authors.test.js
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/frontend/services/routing/controllers/previews.js
- ghost/core/core/boot.js
- ghost/core/test/unit/frontend/services/rss/generate-feed.test.js
- ghost/core/core/frontend/services/routing/controllers/email-post.js
- ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js
- ghost/core/core/server/services/url/url-service-facade.ts
- ghost/core/test/unit/server/services/url/url-service-facade.test.js
- ghost/core/core/server/services/stats/posts-stats-service.js
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
6347435 to
02cfbf8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/frontend/services/routing/router-manager.js (1)
204-210: ⚡ Quick winJSDoc facade contract is missing a used method.
Line 34 calls
urlService.getResourceById(...), butURLServiceFacadetypedef doesn’t declaregetResourceById. Please add it so editor/type hints stay accurate and contract drift is visible earlier.🤖 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/router-manager.js` around lines 204 - 210, The JSDoc typedef URLServiceFacade is missing the getResourceById method that is actually used; update the URLServiceFacade typedef to include a getResourceById property (Function) so the facade contract matches usage of urlService.getResourceById and editor/type hints are correct for the router-manager.js module.
🤖 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.
Nitpick comments:
In `@ghost/core/core/frontend/services/routing/router-manager.js`:
- Around line 204-210: The JSDoc typedef URLServiceFacade is missing the
getResourceById method that is actually used; update the URLServiceFacade
typedef to include a getResourceById property (Function) so the facade contract
matches usage of urlService.getResourceById and editor/type hints are correct
for the router-manager.js module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21fc9725-ebe2-4125-800a-b3237cb4cb05
📒 Files selected for processing (37)
ghost/core/core/app.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/previews.jsghost/core/core/frontend/services/routing/router-manager.jsghost/core/core/frontend/services/rss/generate-feed.jsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.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/indexnow.jsghost/core/core/server/services/member-attribution/url-translator.jsghost/core/core/server/services/mentions/resource-service.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/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/rss/generate-feed.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/slack.test.jsghost/core/test/unit/server/services/stats/content.test.jsghost/core/test/unit/server/services/stats/posts.test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/core/core/frontend/services/routing/controllers/email-post.js
- ghost/core/core/server/lib/common/to-plain.js
- ghost/core/core/frontend/meta/author-url.js
- ghost/core/core/server/services/indexnow.js
- ghost/core/core/frontend/meta/url.js
🚧 Files skipped from review as they are similar to previous changes (10)
- ghost/core/test/unit/server/services/indexnow.test.js
- ghost/core/core/frontend/services/rss/generate-feed.js
- ghost/core/test/unit/frontend/helpers/authors.test.js
- ghost/core/core/server/services/stats/posts-stats-service.js
- ghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.js
- ghost/core/test/unit/server/services/slack.test.js
- ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js
- ghost/core/test/unit/server/services/stats/content.test.js
- ghost/core/core/server/services/stats/content-stats-service.js
- ghost/core/core/server/services/member-attribution/url-translator.js
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
02cfbf8 to
02627b4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ghost/core/core/server/services/stats/content-stats-service.js (1)
241-271: ⚡ Quick winAvoid resolving the same pathname twice per item.
In the no-
post_uuidpath,resolveUrl(item.pathname)is executed forurl_existsand then again viagetResourceTitle(item.pathname). Reusing the first resolved resource removes duplicate work and avoids inconsistent results when resolver state changes mid-loop.♻️ Proposed refactor
- // Check URL existence using the URL service - let urlExists = false; + // Check URL existence using the URL service + let urlExists = false; + let resolvedResource = null; 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 + resolvedResource = await this.urlService.facade.resolveUrl(item.pathname); + urlExists = !!resolvedResource; // Convert to boolean } // If URL service isn't ready, we default to true (clickable) } catch (error) { // If there's an error checking the URL service, default to true urlExists = true; } } @@ - const resourceInfo = await this.getResourceTitle(item.pathname); + const resourceInfo = resolvedResource + ? { + title: resolvedResource.title || resolvedResource.name, + resourceType: ROUTER_TYPE_TO_SINGULAR[resolvedResource.type] + } + : await this.getResourceTitle(item.pathname);🤖 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 241 - 271, The mapping in enrichedData does duplicate URL resolution: resolveUrl(item.pathname) is called to set urlExists and then getResourceTitle(item.pathname) calls resolution again; change the logic in the Promise.all map so you store the resolved resource (from this.urlService.facade.resolveUrl) in a local variable when checking URL existence and pass/reuse that resource into getResourceTitle (or inline the title extraction) instead of calling getResourceTitle with the same pathname again; update references to urlExists and the title/post_id/post_type population to use the cached resource (functions: urlService.facade.resolveUrl, getResourceTitle, enrichedData mapping) to avoid double resolution and inconsistent results.ghost/core/test/unit/frontend/services/rss/generate-feed.test.js (1)
94-95: ⚡ Quick winStrengthen stub expectations to include resource type.
These stubs currently only validate
id; addingtype: 'post'improves regression protection for the resource-based URL API contract.Proposed test assertion refinement
- routerManagerGetUrlForResourceStub.withArgs(sinon.match({id: post.id}), {absolute: true}).returns('http://my-ghost-blog.com/' + post.slug + '/'); + routerManagerGetUrlForResourceStub.withArgs(sinon.match({id: post.id, type: 'post'}), {absolute: true}).returns('http://my-ghost-blog.com/' + post.slug + '/');Also applies to: 192-193
🤖 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/services/rss/generate-feed.test.js` around lines 94 - 95, The stub for routerManagerGetUrlForResourceStub currently matches only on id and should be tightened to also require type: 'post' using sinon.match to avoid regressions in the resource-based URL contract; update the withArgs(...) calls that use sinon.match({id: post.id}) (including the other occurrence later in the file) to sinon.match({id: post.id, type: 'post'}) so the stub only matches when the resource type is a post and adjust any related test expectations accordingly.ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (1)
50-51: ⚡ Quick winPrefer property-based matcher over object-identity matcher in ownership stubs.
Matching by full object instance makes this test fragile; matching by stable fields (for example
id) keeps intent while tolerating harmless object reshaping.Proposed matcher hardening
- ownsResourceStub.withArgs('identifier', posts[0]).returns(true); + ownsResourceStub.withArgs('identifier', sinon.match({id: posts[0].id})).returns(true); ... - ownsResourceStub.withArgs('identifier', posts[0]).returns(false); - ownsResourceStub.withArgs('identifier', posts[1]).returns(true); - ownsResourceStub.withArgs('identifier', posts[2]).returns(false); - ownsResourceStub.withArgs('identifier', posts[3]).returns(false); + ownsResourceStub.withArgs('identifier', sinon.match({id: posts[0].id})).returns(false); + ownsResourceStub.withArgs('identifier', sinon.match({id: posts[1].id})).returns(true); + ownsResourceStub.withArgs('identifier', sinon.match({id: posts[2].id})).returns(false); + ownsResourceStub.withArgs('identifier', sinon.match({id: posts[3].id})).returns(false);Also applies to: 209-212
🤖 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/services/routing/controllers/collection.test.js` around lines 50 - 51, The test currently stubs ownership with a full-object identity matcher (ownsResourceStub.withArgs('identifier', posts[0])), which is fragile; change those uses to match on a stable property instead (e.g., use ownsResourceStub.withArgs('identifier', a sinon property matcher that checks id equals posts[0].id)) so the stub matches by post id rather than object identity; apply the same change for the other occurrences mentioned (the block around the ownsResourceStub calls at lines ~209-212).ghost/core/core/frontend/services/routing/router-manager.js (1)
204-210: ⚡ Quick winKeep
URLServiceFacadetypedef aligned with the exposed facade contract.The typedef is now missing methods that are part of the facade usage/contract (
getResourceByIdis used in this class, andhasFinishedis part of the PR objective), which weakens editor/type guidance.Proposed JSDoc update
/** * `@typedef` {Object} URLServiceFacade * `@property` {Function} getUrlForResource * `@property` {Function} ownsResource * `@property` {Function} resolveUrl + * `@property` {Function} getResourceById + * `@property` {Function} hasFinished * `@property` {Function} onRouterAddedType * `@property` {Function} onRouterUpdated */🤖 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/router-manager.js` around lines 204 - 210, The URLServiceFacade typedef is missing methods required by the facade contract—add the missing entries so editors/types reflect actual usage: include getResourceById and hasFinished in the JSDoc typedef for URLServiceFacade (alongside existing getUrlForResource, ownsResource, resolveUrl, onRouterAddedType, onRouterUpdated) so consumers like router-manager.js can rely on accurate types; make sure the names and signatures match how router-manager.js calls getResourceById and checks hasFinished.
🤖 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.
Nitpick comments:
In `@ghost/core/core/frontend/services/routing/router-manager.js`:
- Around line 204-210: The URLServiceFacade typedef is missing methods required
by the facade contract—add the missing entries so editors/types reflect actual
usage: include getResourceById and hasFinished in the JSDoc typedef for
URLServiceFacade (alongside existing getUrlForResource, ownsResource,
resolveUrl, onRouterAddedType, onRouterUpdated) so consumers like
router-manager.js can rely on accurate types; make sure the names and signatures
match how router-manager.js calls getResourceById and checks hasFinished.
In `@ghost/core/core/server/services/stats/content-stats-service.js`:
- Around line 241-271: The mapping in enrichedData does duplicate URL
resolution: resolveUrl(item.pathname) is called to set urlExists and then
getResourceTitle(item.pathname) calls resolution again; change the logic in the
Promise.all map so you store the resolved resource (from
this.urlService.facade.resolveUrl) in a local variable when checking URL
existence and pass/reuse that resource into getResourceTitle (or inline the
title extraction) instead of calling getResourceTitle with the same pathname
again; update references to urlExists and the title/post_id/post_type population
to use the cached resource (functions: urlService.facade.resolveUrl,
getResourceTitle, enrichedData mapping) to avoid double resolution and
inconsistent results.
In
`@ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js`:
- Around line 50-51: The test currently stubs ownership with a full-object
identity matcher (ownsResourceStub.withArgs('identifier', posts[0])), which is
fragile; change those uses to match on a stable property instead (e.g., use
ownsResourceStub.withArgs('identifier', a sinon property matcher that checks id
equals posts[0].id)) so the stub matches by post id rather than object identity;
apply the same change for the other occurrences mentioned (the block around the
ownsResourceStub calls at lines ~209-212).
In `@ghost/core/test/unit/frontend/services/rss/generate-feed.test.js`:
- Around line 94-95: The stub for routerManagerGetUrlForResourceStub currently
matches only on id and should be tightened to also require type: 'post' using
sinon.match to avoid regressions in the resource-based URL contract; update the
withArgs(...) calls that use sinon.match({id: post.id}) (including the other
occurrence later in the file) to sinon.match({id: post.id, type: 'post'}) so the
stub only matches when the resource type is a post and adjust any related test
expectations accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3184410c-8a18-4d90-8107-200fc822e2c0
📒 Files selected for processing (38)
ghost/core/core/app.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/previews.jsghost/core/core/frontend/services/routing/router-manager.jsghost/core/core/frontend/services/rss/generate-feed.jsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.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/indexnow.jsghost/core/core/server/services/member-attribution/url-translator.jsghost/core/core/server/services/mentions/resource-service.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/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/rss/generate-feed.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/slack.test.jsghost/core/test/unit/server/services/stats/content.test.jsghost/core/test/unit/server/services/stats/posts.test.jsghost/core/test/utils/url-service-utils.js
✅ Files skipped from review due to trivial changes (7)
- ghost/core/core/app.js
- ghost/core/core/server/services/indexnow.js
- ghost/core/core/frontend/helpers/tags.js
- ghost/core/test/unit/server/lib/common/to-plain.test.js
- ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.js
- ghost/core/core/frontend/services/rss/generate-feed.js
- ghost/core/test/unit/server/services/indexnow.test.js
🚧 Files skipped from review as they are similar to previous changes (12)
- ghost/core/core/server/services/mentions/resource-service.js
- ghost/core/test/unit/frontend/meta/author-url.test.js
- ghost/core/test/unit/frontend/helpers/authors.test.js
- ghost/core/core/server/services/audience-feedback/audience-feedback-service.js
- ghost/core/test/unit/server/services/slack.test.js
- ghost/core/core/frontend/meta/url.js
- ghost/core/test/unit/server/services/stats/posts.test.js
- ghost/core/test/unit/frontend/meta/url.test.js
- ghost/core/core/frontend/meta/author-url.js
- ghost/core/test/unit/server/services/stats/content.test.js
- ghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.js
- ghost/core/test/unit/server/services/comments/comments-service-emails.test.js
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
02627b4 to
76a28e4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/member-attribution/url-translator.js (1)
102-103:⚠️ Potential issue | 🟠 MajorKeep attribution lookup best-effort while routing rebuilds.
facade.resolveUrl(path)can reject while the URL service is not ready. Right now that abortsgetResourceDetails()instead of falling back to the existing{type: 'url', id: null, url: path}shape. Catch the readiness error here and return nothing so only unexpected failures bubble out.🤖 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 102 - 103, getTypeAndIdFromPath currently awaits this.urlService.facade.resolveUrl(path) and lets any rejection abort getResourceDetails; wrap the resolveUrl call in a try/catch inside getTypeAndIdFromPath (the method in url-translator.js), and on known readiness errors or when the URL service is not ready return nothing (e.g., return undefined) so the caller can fall back to the {type: 'url', id: null, url: path} behavior; rethrow only unexpected errors so those still bubble up.
🧹 Nitpick comments (3)
ghost/core/test/unit/frontend/helpers/authors.test.js (1)
116-117: ⚡ Quick winAssert the typed facade contract in these stubs.
Matching only
{id}makes these tests pass even if the helper omitstype: 'authors'or passes the DB type by mistake. Since this PR’s behavior hinges on the resource shape, it would be better to match the full resource and options here.Suggested tightening
-urlServiceGetUrlForResourceStub.withArgs(sinon.match({id: authors[0].id})).returns('author url 1'); +urlServiceGetUrlForResourceStub.withArgs( + sinon.match({id: authors[0].id, type: 'authors'}), + sinon.match.object +).returns('author url 1');Also applies to: 131-131, 145-145, 159-159, 174-175, 190-190, 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 stubs for urlServiceGetUrlForResourceStub are too loose—currently they match only {id} so tests pass even if the resource type is wrong; tighten the sinon.match expectations to assert the full resource shape (e.g., include type: 'authors' and any expected attributes) and also assert any expected options passed to urlService.getUrlForResource where applicable (use sinon.match({type: 'authors', id: authors[0].id, ...}) and/or sinon.match.has('optionsKey') in the same stub calls referenced by urlServiceGetUrlForResourceStub to ensure the typed facade contract is enforced).ghost/core/test/unit/frontend/services/rss/generate-feed.test.js (1)
94-95: ⚡ Quick winStrengthen matcher to assert post resource type, not only id.
At Line 94 and Line 192, matching only
{id}can let a wrongtypeslip through unnoticed. Pinningtype: 'posts'makes these tests catch contract regressions earlier.Suggested test hardening
- routerManagerGetUrlForResourceStub.withArgs(sinon.match({id: post.id}), {absolute: true}).returns('http://my-ghost-blog.com/' + post.slug + '/'); + routerManagerGetUrlForResourceStub.withArgs( + sinon.match({id: post.id, type: 'posts'}), + {absolute: true} + ).returns('http://my-ghost-blog.com/' + post.slug + '/');Also applies to: 192-193
🤖 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/services/rss/generate-feed.test.js` around lines 94 - 95, Update the test stubs that call routerManagerGetUrlForResourceStub to match the full post resource shape instead of only id: when stubbing/asserting use sinon.match({id: post.id, type: 'posts'}) (the two occurrences around routerManagerGetUrlForResourceStub at lines referencing post.id/slug) so the matcher requires type: 'posts' along with id; this ensures routerManagerGetUrlForResourceStub receives the correct resource type and prevents false positives from mismatched resource types.ghost/core/core/frontend/meta/url.js (1)
20-24: ⚡ Quick winMake the draft
/404/probe explicit to avoid option-default drift.Line 20 probes with implicit defaults, while Line 24 uses explicit options. Use explicit probe options too, and match
.../404/robustly to avoid subtle regressions.Suggested hardening
- if (data.status !== 'published' && urlService.facade.getUrlForResource(postResource) === '/404/') { + const draftProbeUrl = urlService.facade.getUrlForResource(postResource, {absolute: false, withSubdirectory: true}); + if (data.status !== 'published' && typeof draftProbeUrl === 'string' && draftProbeUrl.endsWith('/404/')) { return urlUtils.urlFor({relativeUrl: urlUtils.urlJoin('/p', data.uuid, '/')}, null, absolute); }🤖 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 20 - 24, The probe at getUrlForResource uses implicit defaults; change the conditional to call urlService.facade.getUrlForResource(postResource, {absolute: absolute, withSubdirectory: true}) (same options used in the return) and compare its result to a canonical 404 URL generated via urlUtils.urlFor({relativeUrl: '/404/'}, null, absolute) (or otherwise ensure trailing-slash exact match) so the draft `/404/` check uses explicit options and a robust comparison; update the code references to urlService.facade.getUrlForResource, urlUtils.urlFor, urlUtils.urlJoin and data.status accordingly.
🤖 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/server/services/comments/comments-service-emails.js`:
- Around line 28-30: The post object passed into getPostUrl must have its
permalink relations hydrated so :primary_tag/:primary_author work; update the
call sites that fetch posts before calling getPostUrl (including
notifyParentCommentAuthor and the other two callers referenced) to fetch the
post with withRelated: ['tags','authors'] or otherwise ensure the post object
includes tags and authors, then pass that enriched post into getPostUrl(post,
commentId) so urlService.facade.getUrlForResource can build correct permalinks.
---
Duplicate comments:
In `@ghost/core/core/server/services/member-attribution/url-translator.js`:
- Around line 102-103: getTypeAndIdFromPath currently awaits
this.urlService.facade.resolveUrl(path) and lets any rejection abort
getResourceDetails; wrap the resolveUrl call in a try/catch inside
getTypeAndIdFromPath (the method in url-translator.js), and on known readiness
errors or when the URL service is not ready return nothing (e.g., return
undefined) so the caller can fall back to the {type: 'url', id: null, url: path}
behavior; rethrow only unexpected errors so those still bubble up.
---
Nitpick comments:
In `@ghost/core/core/frontend/meta/url.js`:
- Around line 20-24: The probe at getUrlForResource uses implicit defaults;
change the conditional to call urlService.facade.getUrlForResource(postResource,
{absolute: absolute, withSubdirectory: true}) (same options used in the return)
and compare its result to a canonical 404 URL generated via
urlUtils.urlFor({relativeUrl: '/404/'}, null, absolute) (or otherwise ensure
trailing-slash exact match) so the draft `/404/` check uses explicit options and
a robust comparison; update the code references to
urlService.facade.getUrlForResource, urlUtils.urlFor, urlUtils.urlJoin and
data.status accordingly.
In `@ghost/core/test/unit/frontend/helpers/authors.test.js`:
- Around line 116-117: The stubs for urlServiceGetUrlForResourceStub are too
loose—currently they match only {id} so tests pass even if the resource type is
wrong; tighten the sinon.match expectations to assert the full resource shape
(e.g., include type: 'authors' and any expected attributes) and also assert any
expected options passed to urlService.getUrlForResource where applicable (use
sinon.match({type: 'authors', id: authors[0].id, ...}) and/or
sinon.match.has('optionsKey') in the same stub calls referenced by
urlServiceGetUrlForResourceStub to ensure the typed facade contract is
enforced).
In `@ghost/core/test/unit/frontend/services/rss/generate-feed.test.js`:
- Around line 94-95: Update the test stubs that call
routerManagerGetUrlForResourceStub to match the full post resource shape instead
of only id: when stubbing/asserting use sinon.match({id: post.id, type:
'posts'}) (the two occurrences around routerManagerGetUrlForResourceStub at
lines referencing post.id/slug) so the matcher requires type: 'posts' along with
id; this ensures routerManagerGetUrlForResourceStub receives the correct
resource type and prevents false positives from mismatched resource types.
🪄 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: 563e48b7-1a56-4d34-b878-e59fbdfea5d9
📒 Files selected for processing (45)
ghost/core/core/app.jsghost/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/server/api/endpoints/utils/serializers/output/mappers/posts.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/indexnow.jsghost/core/core/server/services/member-attribution/url-translator.jsghost/core/core/server/services/mentions/resource-service.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/index.jsghost/core/core/server/services/url/url-service-facade.tsghost/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/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/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/url-service-facade.test.jsghost/core/test/utils/url-service-utils.js
💤 Files with no reviewable changes (2)
- ghost/core/core/frontend/services/routing/controllers/entry.js
- ghost/core/test/unit/frontend/services/routing/controllers/entry.test.js
✅ Files skipped from review due to trivial changes (13)
- ghost/core/core/frontend/helpers/tags.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/test/unit/server/lib/common/to-plain.test.js
- ghost/core/core/server/services/url/index.js
- ghost/core/core/frontend/services/rss/generate-feed.js
- ghost/core/core/frontend/services/routing/controllers/email-post.js
- ghost/core/core/server/lib/common/to-plain.js
- ghost/core/core/frontend/meta/author-url.js
- ghost/core/test/unit/server/services/url/url-service-facade.test.js
- ghost/core/test/unit/frontend/helpers/tags.test.js
- ghost/core/core/server/services/stats/posts-stats-service.js
- ghost/core/test/unit/server/services/slack.test.js
- ghost/core/test/unit/server/services/stats/content.test.js
🚧 Files skipped from review as they are similar to previous changes (11)
- ghost/core/core/frontend/helpers/authors.js
- ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.js
- ghost/core/core/server/services/audience-feedback/audience-feedback-service.js
- ghost/core/core/app.js
- ghost/core/core/server/services/mentions/resource-service.js
- ghost/core/core/boot.js
- ghost/core/test/unit/server/services/indexnow.test.js
- ghost/core/test/unit/server/services/mentions/resource-service.test.js
- ghost/core/test/unit/server/services/audience-feedback/audience-feedback-service.test.js
- ghost/core/core/server/services/stats/content-stats-service.js
- ghost/core/core/server/services/url/url-service-facade.ts
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
76a28e4 to
6f06040
Compare
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.
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.
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
6f06040 to
98ea5c3
Compare
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/ 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/ `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.
allouis
force-pushed
the
hkg-1738-refactor-facade
branch
from
98ea5c3 to
db53496
Compare
4 tasks
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
Introduces
UrlServiceFacade— a resource-based abstraction in front of the eagerUrlService— and migrates every in-tree caller across. Behaviour-preserving; no flag, no functional change. Eager URL service is unchanged underneath.Stacked on #27762 (the posts-stats test PR replacing #27712).
ref https://linear.app/ghost/issue/HKG-1738/
Why
This is the preparatory refactor for an opt-in lazy URL routing experiment (#27714, stacked on this PR). The existing eager
UrlServiceprecomputes a full resource → URL map at boot through an 8-stage pipeline; we want to be able to swap in an on-demand backend without touching individual callers, but the call sites today are inconsistent — some usegetUrlByResourceId(id), others usegetResource(path)with a different envelope shape.This PR introduces a thin resource-based facade in front of the eager service and migrates every in-tree caller across, so a single backend boundary exists when the lazy implementation lands.
It also resolves an inconsistency in the resolved-resource shape: the routing-level type (
'posts'/'pages'/'tags'/'authors') now wins over any DB type field on the underlying record.What it does
core/server/services/url/url-service-facade.ts(new TS file). ExposesgetUrlForResource(resource, opts),ownsResource(routerId, resource),resolveUrl(path),getResourceById(id),hasFinished(), and lifecycle pass-throughs.urlService.getUrlByResourceId(id)andurlService.getResource(path)to the new facade.audience-feedbackandcommentsURL helpers from id-only to taking a post object, so the helpers carry the full record into the URL service. Adds a smalltoPlain(modelOrObj)helper for callers that may have either a Bookshelf model or a plain hash.?fields=urlContent API requests: the API output URL serializer now passesidexplicitly into the resource (theattrsspread alone wouldn't have it afterfields=urlstrips everything buturl, and the eager id-fallback would return/404/for every record).How to review
Please review commit-by-commit (the GitHub Commits tab). The full diff is a sweep across ~10 files, but each commit is small and isolated:
toPlain()helper.UrlServiceFacade(TS) — facade class only, no callers yet.output/utils/url.js. Includes the?fields=urlregression fix and a regression test for it. Also handles the post/page dispatch fix from PR review.{{tags}},{{authors}},meta/url,meta/author-url.getResource(path)→facade.resolveUrl(path).url-translator.getTypeAndIdFromPathandcontent-stats-service.getResourceTitlebecome async; the ripple stops at their existing async callers.Test plan
/, RSS, sitemap, comment notification email