Fixed IndexNow pinging /404/ for posts with no web URL#28311
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds an early validation guard to the Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/unit/server/services/indexnow.test.js`:
- Around line 289-290: The test currently only asserts loggingStub was called
once and that loggingStub.args[0][0].system.event === 'indexnow.unresolved_url';
expand the assertion to validate the full structured warning payload by
asserting the presence and expected values/types of
loggingStub.args[0][0].system.event, loggingStub.args[0][0].context.post_id,
loggingStub.args[0][0].context.post_slug, and loggingStub.args[0][0].context.url
(use the same expected post id/slug/url values used to trigger the warning in
the test); update the assertions around loggingStub in indexnow.test.js to check
these fields in addition to the event to prevent regressions in the log
contract.
🪄 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: c6eb6f68-bd6f-4573-888f-96671336eb15
📒 Files selected for processing (2)
ghost/core/core/server/services/indexnow.jsghost/core/test/unit/server/services/indexnow.test.js
ref #28295 On publish, IndexNow resolves the post URL via getUrlForResource, which returns a /404/ URL when a published post is not owned by any route (e.g. an imported or members post outside the site's collections). The ping then submitted the /404/ URL instead of a post URL. Guard the ping: when the resolved URL is empty or a /404/ URL, skip it and log an indexnow.unresolved_url event. Behaviour is otherwise unchanged.
44652f9 to
e3368ae
Compare
|
Addressed CodeRabbit's suggestion: the |
What
IndexNow (behind the
indexnowlabs flag) pingsapi.indexnow.orgwhen a post is published or edited, resolving the post URL viaurlService.facade.getUrlForResource(...).That returns a
/404/URL when a published post is not owned by any route — e.g. an imported or members post that falls outside the site's collections, or a routing config with no catch-all. In that case the ping submittedhttps://<site>/404/instead of a post URL. This was seen in production on a site whose unrouted posts produced most of its IndexNow pings.Change
After resolving the URL, if it's empty or ends in
/404/, skip the ping and log anindexnow.unresolved_urlevent instead. Other behaviour is unchanged.Notes
indexnow.unresolved_urlevent makes it observable.getUrlForResourcereturns/404/as its not-found sentinel (seeurl-service.js), so matching it identifies an unrouted resource.Testing
Added a unit test asserting no ping and an
indexnow.unresolved_urlevent (withpost_id/post_slug/url) when the URL resolves to/404/; existing ping tests stub a resolved URL. 22 unit tests pass; lint clean.