Fixed lazy URL shadow-comparison errors from thin resources - #29126
Conversation
WalkthroughThis PR adds fallback redirect handling for audience feedback when a target post no longer exists: Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 4s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 48s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 10s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 25s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 6s | View ↗ |
nx run ghost:build:assets |
✅ Succeeded | 2s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 2s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-06 07:38:41 UTC
… field ref https://linear.app/ghost/issue/HKG-1822 A request like ?fields=id,title strips post attrs down to the requested columns, but forPost still computed the URL and only deleted it again at the end of the function. Under lazyRouting shadow comparison the stripped resource carries no status (or tags/authors), so the lazy backend rejected it as thin and every such request logged a LAZY_URL_COMPARE_ERROR. Skipping URL generation when url was not requested matches the guards forUser and forTag already have, and saves wasted work under the eager service too.
ref https://linear.app/ghost/issue/HKG-1822 The newsletter feedback button endpoint (/members/feedback/:postId/:score) built its redirect from a bare {id} resource. The eager service looks URLs up by id so this worked, but the lazy backend needs status (and tags/authors for filtered routes) and rejects the resource as thin, so under shadow comparison every feedback click logged a LAZY_URL_COMPARE_ERROR. The controller now loads the full post with its tags and authors before building the link, and falls back to the home-page link without touching the URL service when the post no longer exists.
ref https://linear.app/ghost/issue/HKG-1822 A users browse or read with ?fields=url strips the permalink columns (slug), so the lazy URL service generated /author/undefined/ while eager returned the real URL, showing up as a parity mismatch under shadow comparison. Staff users route through the authors router types, so this mirrors the forceUrlColumnsWhenLazy wiring the authors serializers already have.
f960747 to
ecb4ceb
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/url.js (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared "skip when url not requested" guard.
Per the comment, this exact guard (
frame.options.columns && !frame.options.columns.includes('url')) is duplicated inforUser/forTag. Extracting a small shared helper (e.g.shouldComputeUrl(frame)) would reduce triplication and centralize the LAZY_URL_THIN_RESOURCE avoidance logic for future maintenance.🤖 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/api/endpoints/utils/serializers/output/utils/url.js` around lines 15 - 23, Extract the duplicated “skip when url not requested” check from the URL serializer utilities into a shared helper such as shouldComputeUrl(frame) and use it in output/utils/url.js as well as the forUser and forTag paths. Keep the existing behavior that returns attrs early when frame.options.columns does not include url, but centralize the guard so the LAZY_URL_THIN_RESOURCE avoidance logic is maintained in one place and all URL computation entry points stay consistent.ghost/core/core/server/services/audience-feedback/audience-feedback-service.js (1)
57-60: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
postIdisn't URI-encoded in the hash, unlikeuuid/key.
uuidandkeyare wrapped inencodeURIComponent, butpostIdis interpolated raw.postIdcan originate fromreq.params.postId(buildFallbackLink) orpostData.id, so an unexpected value (e.g. containing/,?,&) would alter the fragment structure the Portal SPA parses. This is pre-existing behavior extended by this refactor, not user-facing/exploitable server-side (fragments never leave the browser), but encoding it would remove the inconsistency and any parsing surprises.🔒 Proposed fix
`#withFeedbackHash`(url, postId, score, uuid, key) { - url.hash = `#/feedback/${postId}/${score}/?uuid=${encodeURIComponent(uuid)}&key=${encodeURIComponent(key)}`; + url.hash = `#/feedback/${encodeURIComponent(postId)}/${score}/?uuid=${encodeURIComponent(uuid)}&key=${encodeURIComponent(key)}`; return url; }🤖 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/audience-feedback/audience-feedback-service.js` around lines 57 - 60, The `AudienceFeedbackService.#withFeedbackHash` method interpolates `postId` into the fragment without encoding it, unlike `uuid` and `key`. Update the hash construction in `#withFeedbackHash` so `postId` is passed through `encodeURIComponent` before being inserted, keeping the fragment structure consistent for values coming from `buildFallbackLink` or `postData.id` and avoiding parsing surprises in the Portal SPA.
🤖 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/server/api/endpoints/utils/serializers/output/utils/url.js`:
- Around line 15-23: Extract the duplicated “skip when url not requested” check
from the URL serializer utilities into a shared helper such as
shouldComputeUrl(frame) and use it in output/utils/url.js as well as the forUser
and forTag paths. Keep the existing behavior that returns attrs early when
frame.options.columns does not include url, but centralize the guard so the
LAZY_URL_THIN_RESOURCE avoidance logic is maintained in one place and all URL
computation entry points stay consistent.
In
`@ghost/core/core/server/services/audience-feedback/audience-feedback-service.js`:
- Around line 57-60: The `AudienceFeedbackService.#withFeedbackHash` method
interpolates `postId` into the fragment without encoding it, unlike `uuid` and
`key`. Update the hash construction in `#withFeedbackHash` so `postId` is passed
through `encodeURIComponent` before being inserted, keeping the fragment
structure consistent for values coming from `buildFallbackLink` or `postData.id`
and avoiding parsing surprises in the Portal SPA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0235ebf1-e8bf-43b9-8317-e240e3da86d1
📒 Files selected for processing (10)
ghost/core/core/server/api/endpoints/utils/serializers/input/users.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/url.jsghost/core/core/server/services/audience-feedback/audience-feedback-controller.jsghost/core/core/server/services/audience-feedback/audience-feedback-service.jsghost/core/core/server/services/audience-feedback/feedback-repository.jsghost/core/test/e2e-api/members/feedback.test.jsghost/core/test/unit/api/canary/utils/serializers/input/users.test.jsghost/core/test/unit/api/canary/utils/serializers/output/utils/url.test.jsghost/core/test/unit/server/services/audience-feedback/audience-feedback-controller.test.jsghost/core/test/unit/server/services/audience-feedback/audience-feedback-service.test.js

ref https://linear.app/ghost/issue/HKG-1822/monitor-shadow-comparison-and-address-mismatches
Problem
Production is logging ~25k
LAZY_URL_COMPARE_ERROR("Lazy URL service threw during comparison") events across 169 sites. The error details all have the shape:{"resourceType":"posts","resourceId":"...","baseFilter":"status:published+type:post","missing":["status"]}i.e. the lazy backend's
_assertBaseFieldsPresentrejecting a resource that reachesgetUrlForResourcewithout thestatuscolumn, wrapped by the compare-mode facade. Eager stays authoritative so there's no user impact, but the noise drowns out real parity signals.Two call sites were handing the URL service thin resources:
forPost— computedattrs.urlfor every post/page API response and only deleted it afterwards when?fields=/columnsexcludedurl. With narrowed fields the attrs carry nostatus(or tags/authors), so the lazy shadow threw on every such request.forceUrlColumnsWhenLazyonly covers the case whereurlis requested./members/feedback/:postId/:score) — built its redirect from a bare{id: postId}resource, so every newsletter feedback-button click threw in the lazy shadow. This explains the recurring per-post counts in the logs.A sweep of the remaining
getUrlForResource/ownsResource/resolveUrlcallers also turned up a staff-users gap:?fields=urlon users browse/read stripsslug, so lazy generates/author/undefined/— aLAZY_URL_PARITY_MISMATCHrather than a throw.Solution
forPostnow skips URL generation entirely whenurlwas not requested, matching the guardsforUser/forTagalready have (also saves wasted work under eager).withRelated: tags, authors) before building the link, and uses a newbuildFallbackLink(home page + feedback fragment, no URL service involved) when the post no longer exists — same destinationbuildLinkpicks when the URL service returns/404/.urlis requested, mirroring the authors serializers.All other
getUrlForResourcecallers (slack, indexnow, comments emails, member attribution, activity feed, RSS, previews, email-post, meta/url, theme helpers) already pass fully-loaded models.Testing
🤖 Generated with Claude Code