feat(core): exclude posts tagged #no-search-index from the index#52
Conversation
Add a configurable, tag-based content-policy exclusion (`collection.excludeTags`, default `['#no-search-index']`) alongside the existing internal-tag filtering and gated-content redaction. Posts carrying an excluded tag are skipped by indexAllPosts and removed by indexPost — so via the webhook handler, an edit that adds the tag de-indexes the post. Matching mirrors isInternalTag: by tag name or slug, case-insensitively, deriving the Ghost `hash-` slug from `#`-prefixed names. The default lives in core so every consumer (CLI, webhook handler) shares one canonical convention; an explicit `[]` disables it. Documented in the README + createDefaultConfig.
Reviewer's GuideAdds a configurable tag-based mechanism to exclude posts (by default those tagged #no-search-index) from the Typesense search index at both bulk and single-post indexing paths, wires the configuration through the shared config package, and documents the behavior in the README with accompanying unit tests. Sequence diagram for indexPost handling of #no-search-index exclusionsequenceDiagram
actor Ghost
participant WebhookHandler
participant GhostTypesenseManager
participant TypesenseCollection
Ghost ->> WebhookHandler: post.updated
WebhookHandler ->> GhostTypesenseManager: indexPost(post)
GhostTypesenseManager ->> GhostTypesenseManager: isExcludedByTag(post.data)
alt [post is excluded by tag]
GhostTypesenseManager ->> TypesenseCollection: documents().delete(filter_by: id:postId)
TypesenseCollection -->> GhostTypesenseManager: delete result
GhostTypesenseManager -->> WebhookHandler: return
else [post is not excluded]
GhostTypesenseManager ->> TypesenseCollection: documents().upsert(document)
TypesenseCollection -->> GhostTypesenseManager: upsert result
GhostTypesenseManager -->> WebhookHandler: return
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider precomputing the normalized
excludeTagsset once perGhostTypesenseManager(e.g. in the constructor) instead of rebuilding thewantedSeton everyisExcludedByTagcall to avoid repeated work during bulk indexing. - The tag normalization logic in
isExcludedByTag(lower-casing, derivinghash-slugs) closely mirrors the patterns aroundisInternalTag; it might be worth extracting a shared helper to avoid subtle drift between these tag-matching behaviours over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider precomputing the normalized `excludeTags` set once per `GhostTypesenseManager` (e.g. in the constructor) instead of rebuilding the `wanted` `Set` on every `isExcludedByTag` call to avoid repeated work during bulk indexing.
- The tag normalization logic in `isExcludedByTag` (lower-casing, deriving `hash-` slugs) closely mirrors the patterns around `isInternalTag`; it might be worth extracting a shared helper to avoid subtle drift between these tag-matching behaviours over time.
## Individual Comments
### Comment 1
<location path="packages/core/src/index.ts" line_range="20" />
<code_context>
+ * consumer of the package — CLI, webhook handler — shares one canonical
+ * convention without each having to opt in.
+ */
+const DEFAULT_EXCLUDE_TAGS = ['#no-search-index'];
+
export interface Post {
</code_context>
<issue_to_address>
**issue:** Default tag list is now duplicated in both core and config, which can drift over time.
This introduces a second source of truth for default exclusion tags: `DEFAULT_EXCLUDE_TAGS` here and `excludeTags: ['#no-search-index']` in `createDefaultConfig`. If only one is updated (e.g. when adding a new canonical tag), behavior will differ between callers using explicit config vs. defaults. Please centralize this (e.g. export a single constant and reuse it, or move both to a shared module) to keep the default exclusion behavior consistent.
</issue_to_address>
### Comment 2
<location path="packages/core/src/__tests__/index.test.ts" line_range="400-409" />
<code_context>
+describe('GhostTypesenseManager — tag-based exclusion (#no-search-index)', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding coverage for tag-based exclusion in the bulk indexing path (`indexAllPosts`), not just `indexPost`.
The tests for `isExcludedByTag` and `indexPost` cover the webhook/edit flow well. Since `isExcludedByTag` is also used by `indexAllPosts`, it would be valuable to add a test that bulk indexing skips `#no-search-index` (and custom `excludeTags`) posts—for example, by indexing a mix of excluded and non-excluded posts and asserting only the allowed ones reach Typesense.
Suggested implementation:
```typescript
describe('GhostTypesenseManager — tag-based exclusion (#no-search-index)', () => {
it('skips #no-search-index posts when bulk indexing via indexAllPosts', async () => {
const config: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: {
nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }],
apiKey: 'test-key'
},
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true }
],
defaultSortingField: 'id'
}
};
const manager = new GhostTypesenseManager(config);
// Mock the bulk source of posts used by indexAllPosts to return a mix of
// excluded and non-excluded posts.
// NOTE: adjust this to the actual client/property used by indexAllPosts.
(manager as any).ghostAdmin = {
posts: {
browse: jest.fn().mockResolvedValue([
{
id: 'included-post',
slug: 'included-post',
title: 'Included Post',
html: '<p>Included</p>',
tags: []
},
{
id: 'excluded-post',
slug: 'excluded-post',
title: 'Excluded Post',
html: '<p>Excluded</p>',
tags: [{ slug: 'no-search-index', name: '#no-search-index' }]
}
])
}
};
mockDocuments.upsert.mockClear();
mockDocuments.delete.mockClear();
await manager.indexAllPosts();
// Only the non-excluded post should be sent to Typesense
expect(mockDocuments.upsert).toHaveBeenCalledTimes(1);
expect(mockDocuments.upsert).toHaveBeenCalledWith(
expect.objectContaining({ id: 'included-post' })
);
expect(mockDocuments.upsert).not.toHaveBeenCalledWith(
expect.objectContaining({ id: 'excluded-post' })
);
expect(mockDocuments.delete).not.toHaveBeenCalled();
});
it('skips custom excludeTags when bulk indexing via indexAllPosts', async () => {
const config: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: {
nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }],
apiKey: 'test-key'
},
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true }
],
defaultSortingField: 'id'
},
excludeTags: ['#exclude-from-search']
};
const manager = new GhostTypesenseManager(config);
(manager as any).ghostAdmin = {
posts: {
browse: jest.fn().mockResolvedValue([
{
id: 'included-post',
slug: 'included-post',
title: 'Included Post',
html: '<p>Included</p>',
tags: []
},
{
id: 'custom-excluded-post',
slug: 'custom-excluded-post',
title: 'Custom Excluded Post',
html: '<p>Excluded</p>',
tags: [{ slug: 'exclude-from-search', name: '#exclude-from-search' }]
}
])
}
};
mockDocuments.upsert.mockClear();
mockDocuments.delete.mockClear();
await manager.indexAllPosts();
expect(mockDocuments.upsert).toHaveBeenCalledTimes(1);
expect(mockDocuments.upsert).toHaveBeenCalledWith(
expect.objectContaining({ id: 'included-post' })
);
expect(mockDocuments.upsert).not.toHaveBeenCalledWith(
expect.objectContaining({ id: 'custom-excluded-post' })
);
expect(mockDocuments.delete).not.toHaveBeenCalled();
});
const baseConfig: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: { nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }], apiKey: 'test-key' },
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true },
```
1. The tests above assume that `indexAllPosts` pulls posts via `(manager as any).ghostAdmin.posts.browse`. If the real implementation uses a different internal client (e.g. `ghostContentApi.posts.browse` or a different property name), adjust the mocked property accordingly so that `indexAllPosts` consumes the mocked posts.
2. Ensure `mockDocuments` is the same Jest mock used in the other tests (for `indexPost`). If its initialization lives in a shared `beforeEach`, no change is required; otherwise, you may need to expose/initialize it so these new tests can access and clear it.
3. If your `Config` type requires additional fields (e.g. search parameters, facet configs), mirror the minimal valid config from the existing tests (e.g. copy the `baseConfig` shape) to keep these new tests aligned with the rest of the suite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| describe('GhostTypesenseManager — tag-based exclusion (#no-search-index)', () => { | ||
| const baseConfig: Config = { | ||
| ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' }, | ||
| typesense: { nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }], apiKey: 'test-key' }, | ||
| collection: { | ||
| name: 'test-collection', | ||
| fields: [ | ||
| { name: 'id', type: 'string', optional: false }, | ||
| { name: 'title', type: 'string', optional: false }, | ||
| { name: 'slug', type: 'string', optional: false }, |
There was a problem hiding this comment.
suggestion (testing): Consider adding coverage for tag-based exclusion in the bulk indexing path (indexAllPosts), not just indexPost.
The tests for isExcludedByTag and indexPost cover the webhook/edit flow well. Since isExcludedByTag is also used by indexAllPosts, it would be valuable to add a test that bulk indexing skips #no-search-index (and custom excludeTags) posts—for example, by indexing a mix of excluded and non-excluded posts and asserting only the allowed ones reach Typesense.
Suggested implementation:
describe('GhostTypesenseManager — tag-based exclusion (#no-search-index)', () => {
it('skips #no-search-index posts when bulk indexing via indexAllPosts', async () => {
const config: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: {
nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }],
apiKey: 'test-key'
},
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true }
],
defaultSortingField: 'id'
}
};
const manager = new GhostTypesenseManager(config);
// Mock the bulk source of posts used by indexAllPosts to return a mix of
// excluded and non-excluded posts.
// NOTE: adjust this to the actual client/property used by indexAllPosts.
(manager as any).ghostAdmin = {
posts: {
browse: jest.fn().mockResolvedValue([
{
id: 'included-post',
slug: 'included-post',
title: 'Included Post',
html: '<p>Included</p>',
tags: []
},
{
id: 'excluded-post',
slug: 'excluded-post',
title: 'Excluded Post',
html: '<p>Excluded</p>',
tags: [{ slug: 'no-search-index', name: '#no-search-index' }]
}
])
}
};
mockDocuments.upsert.mockClear();
mockDocuments.delete.mockClear();
await manager.indexAllPosts();
// Only the non-excluded post should be sent to Typesense
expect(mockDocuments.upsert).toHaveBeenCalledTimes(1);
expect(mockDocuments.upsert).toHaveBeenCalledWith(
expect.objectContaining({ id: 'included-post' })
);
expect(mockDocuments.upsert).not.toHaveBeenCalledWith(
expect.objectContaining({ id: 'excluded-post' })
);
expect(mockDocuments.delete).not.toHaveBeenCalled();
});
it('skips custom excludeTags when bulk indexing via indexAllPosts', async () => {
const config: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: {
nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }],
apiKey: 'test-key'
},
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true }
],
defaultSortingField: 'id'
},
excludeTags: ['#exclude-from-search']
};
const manager = new GhostTypesenseManager(config);
(manager as any).ghostAdmin = {
posts: {
browse: jest.fn().mockResolvedValue([
{
id: 'included-post',
slug: 'included-post',
title: 'Included Post',
html: '<p>Included</p>',
tags: []
},
{
id: 'custom-excluded-post',
slug: 'custom-excluded-post',
title: 'Custom Excluded Post',
html: '<p>Excluded</p>',
tags: [{ slug: 'exclude-from-search', name: '#exclude-from-search' }]
}
])
}
};
mockDocuments.upsert.mockClear();
mockDocuments.delete.mockClear();
await manager.indexAllPosts();
expect(mockDocuments.upsert).toHaveBeenCalledTimes(1);
expect(mockDocuments.upsert).toHaveBeenCalledWith(
expect.objectContaining({ id: 'included-post' })
);
expect(mockDocuments.upsert).not.toHaveBeenCalledWith(
expect.objectContaining({ id: 'custom-excluded-post' })
);
expect(mockDocuments.delete).not.toHaveBeenCalled();
});
const baseConfig: Config = {
ghost: { url: 'https://test.com', key: 'test-key', version: 'v5.0' },
typesense: { nodes: [{ host: 'localhost', port: 8108, protocol: 'http' }], apiKey: 'test-key' },
collection: {
name: 'test-collection',
fields: [
{ name: 'id', type: 'string', optional: false },
{ name: 'title', type: 'string', optional: false },
{ name: 'slug', type: 'string', optional: false },
{ name: 'html', type: 'string', optional: true },- The tests above assume that
indexAllPostspulls posts via(manager as any).ghostAdmin.posts.browse. If the real implementation uses a different internal client (e.g.ghostContentApi.posts.browseor a different property name), adjust the mocked property accordingly so thatindexAllPostsconsumes the mocked posts. - Ensure
mockDocumentsis the same Jest mock used in the other tests (forindexPost). If its initialization lives in a sharedbeforeEach, no change is required; otherwise, you may need to expose/initialize it so these new tests can access and clear it. - If your
Configtype requires additional fields (e.g. search parameters, facet configs), mirror the minimal valid config from the existing tests (e.g. copy thebaseConfigshape) to keep these new tests aligned with the rest of the suite.
|
Warning Review limit reached
More reviews will be available in 3 minutes and 17 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds configurable tag-based exclusion for Ghost posts, defaults to ChangesTag-based post exclusion
Sequence Diagram(s)sequenceDiagram
participant GhostWebhook
participant GhostTypesenseManager
participant GhostAPI
participant Typesense
GhostWebhook->>GhostTypesenseManager: indexPost(postId)
GhostTypesenseManager->>GhostAPI: fetch post
GhostAPI-->>GhostTypesenseManager: post with tags
GhostTypesenseManager->>GhostTypesenseManager: isExcludedByTag(post)
alt post is excluded
GhostTypesenseManager->>Typesense: documents.delete(filter_by id: postId)
else post is not excluded
GhostTypesenseManager->>Typesense: documents.upsert(post)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/index.ts (1)
595-598: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently swallowing the de-index delete can leave excluded content searchable.
.catch(() => {})discards every error from the de-index delete. For a content-policy escape hatch, a transient Typesense failure here means a post the publisher explicitly marked non-searchable stays in the index with no log or signal. Consider at least logging the failure (mirroring the gated branch at Line 604 if you adjust both).🔍 Suggested logging
- await collection.documents().delete({ filter_by: `id:${postId}` }).catch(() => {}); + await collection.documents().delete({ filter_by: `id:${postId}` }) + .catch((err: any) => console.error(`Failed to de-index excluded post ${postId}: ${err?.message || err}`)); return;🤖 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 `@packages/core/src/index.ts` around lines 595 - 598, The delete in the `isExcludedByTag` branch of `collection.documents().delete` is swallowing all errors, which can leave excluded posts searchable with no signal. Update this path to handle the rejection explicitly in `indexPost`/the surrounding indexing flow: catch the failure, log it with the same context used in the gated error handling branch, and keep the early return so excluded content still bypasses indexing.packages/core/src/__tests__/index.test.ts (1)
400-470: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage of the webhook path; the bulk (
indexAllPosts) filter is untested.The new tests exercise
isExcludedByTagand theindexPostwebhook branch, butindexAllPosts' exclusion filter (Line 367) isn't covered — thebrowsemock returns no excluded post. Consider adding a bulk-path case asserting an excluded post is dropped frompostsToIndex.🤖 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 `@packages/core/src/__tests__/index.test.ts` around lines 400 - 470, The bulk indexing path in GhostTypesenseManager is missing coverage for tag-based exclusion, so add a test around indexAllPosts that uses the existing browse mock to return an excluded post and asserts it is filtered out of postsToIndex before indexing. Reuse the GhostTypesenseManager test setup and verify the exclusion behavior in the bulk path, not just isExcludedByTag and indexPost.
🤖 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 `@packages/core/src/__tests__/index.test.ts`:
- Around line 400-470: The bulk indexing path in GhostTypesenseManager is
missing coverage for tag-based exclusion, so add a test around indexAllPosts
that uses the existing browse mock to return an excluded post and asserts it is
filtered out of postsToIndex before indexing. Reuse the GhostTypesenseManager
test setup and verify the exclusion behavior in the bulk path, not just
isExcludedByTag and indexPost.
In `@packages/core/src/index.ts`:
- Around line 595-598: The delete in the `isExcludedByTag` branch of
`collection.documents().delete` is swallowing all errors, which can leave
excluded posts searchable with no signal. Update this path to handle the
rejection explicitly in `indexPost`/the surrounding indexing flow: catch the
failure, log it with the same context used in the gated error handling branch,
and keep the early return so excluded content still bypasses indexing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 704edc54-ee0e-4a71-a65f-340484e64c2d
📒 Files selected for processing (5)
README.mdpackages/config/src/__tests__/index.test.tspackages/config/src/index.tspackages/core/src/__tests__/index.test.tspackages/core/src/index.ts
- Centralize DEFAULT_EXCLUDE_TAGS in the config package (single source of truth shared by core and createDefaultConfig) so the default can't drift. - Precompute the normalized exclude-tag set once in the constructor instead of rebuilding it on every isExcludedByTag call during bulk indexing. - Log instead of silently swallowing failures when de-indexing an excluded or gated post, so a transient delete failure isn't invisible. - Add bulk-path coverage: indexAllPosts drops a #no-search-index post.
Publishes the #no-search-index tag exclusion (PR #52): posts tagged #no-search-index (configurable via collection.excludeTags) are kept out of the index, alongside internal-tag filtering and gated redaction. Bumps all packages to 2.0.8 with the CHANGELOG entry.
What
Adds a configurable, tag-based way to keep individual posts out of the search index — for landing pages, legal/policy pages, internal notes, etc. By default, any post tagged
#no-search-indexis excluded.This is the same category of index-time content policy that already lives in
packages/core—isInternalTag()(exclude internal tags) andbuildRedactedPost()(redact gated bodies) — so it sits right next to them, as one canonical, documented convention rather than something each consumer reinvents.How
collection.excludeTags(config) — optionalstring[]; the indexer defaults to['#no-search-index']when omitted, and an explicit[]disables exclusion.isExcludedByTag()(core) — mirrorsisInternalTag's defensive shape: matches a post's tags by name or slug, case-insensitively, and derives the Ghosthash-slug from#-prefixed names (so#no-search-indexalso matcheshash-no-search-indexeven if the tag's display name was changed).indexAllPostsfilters excluded posts out of the bulk sync.indexPostdeletes-and-returns when the tag is present — so through the webhook handler (which callsindexPost), an edit that adds the tag de-indexes the post. No webhook-handler change needed.Tests
excludeTagsundefined when omitted, accepts a custom list, accepts[], andcreateDefaultConfigsurfaces the convention.isExcludedByTag(default match by name/slug, custom list override,[]disables), plusindexPostintegration proving a#no-search-indexpost is deleted, not upserted (and indexed normally whenexcludeTags: []).Notes
apps/api/src/lib/typesense/*) and an ad-hoc thepulse script each reimplement indexing and have already drifted (the internal-tag leak came from one such fork). Mirroring this exclusion there — ideally converging thepulse onto this package's CLI — is the bigger win and a separate task.Summary by Sourcery
Introduce configurable tag-based exclusion of posts from the search index, defaulting to ignoring posts tagged #no-search-index across all core indexer entry points.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
#no-search-indexconvention.Documentation
#-prefixed tags behave.