🐛 Fixed private mentions being treated as public#1429
Conversation
WalkthroughAdds a PUBLIC_COLLECTION import and a pre-processing step to the Create activity handler that computes recipients from Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Areas to pay attention to:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/support/notifications.js (1)
42-42: Inconsistent error messages in retry handlers.The error message at line 42 omits the retry count, while the similar error at line 79 includes
(${MAX_RETRIES})and additional context. Consider keeping the messages consistent for better debugging, or document why this difference is intentional.- `Max retries reached when waiting on item in notifications`, + `Max retries reached (${MAX_RETRIES}) when waiting on item in notifications`,Note: If the simplified message is required for test assertions (as seen in mention_steps.js line 124), consider using a more flexible matching approach in tests rather than exact string comparison.
src/activity-handlers/create.handler.ts (1)
24-32: Approve the public collection check with a defensive coding suggestion.The logic correctly filters out non-public activities by checking for
PUBLIC_COLLECTION.hrefin the recipients. This aligns with the ActivityPub specification and addresses the PR's objective.Consider adding defensive checks in case
toIdsorccIdsare undefined:- const recipients = [...create.toIds, ...create.ccIds].map( + const recipients = [...(create.toIds ?? []), ...(create.ccIds ?? [])].map( (id) => id.href, );This prevents potential runtime errors if Fedify's Create object has undefined arrays in edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/handle-mention.feature(1 hunks)features/step_definitions/activitypub_steps.js(0 hunks)features/step_definitions/mention_steps.js(1 hunks)features/support/notifications.js(1 hunks)src/activity-handlers/create.handler.ts(2 hunks)
💤 Files with no reviewable changes (1)
- features/step_definitions/activitypub_steps.js
🧰 Additional context used
📓 Path-based instructions (4)
features/**/*.feature
📄 CodeRabbit inference engine (AGENTS.md)
e2e tests should reside in the features directory
Files:
features/handle-mention.feature
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activity-handlers/create.handler.ts
src/activity-handlers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement new handlers using the class-based pattern under /activity-handlers
Files:
src/activity-handlers/create.handler.ts
features/step_definitions/**/*_steps.js
📄 CodeRabbit inference engine (AGENTS.md)
Group Cucumber step definitions by high-level feature and name files with the _steps.js suffix under features/step_definitions
Files:
features/step_definitions/mention_steps.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
Applied to files:
src/activity-handlers/create.handler.ts
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to features/step_definitions/**/*_steps.js : Group Cucumber step definitions by high-level feature and name files with the _steps.js suffix under features/step_definitions
Applied to files:
features/step_definitions/mention_steps.js
🧬 Code graph analysis (1)
features/step_definitions/mention_steps.js (3)
features/support/fixtures.js (2)
createObject(64-105)createActivity(107-242)features/support/request.js (1)
fetchActivityPub(9-36)features/support/notifications.js (3)
found(32-34)found(71-71)waitForItemInNotifications(3-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (3)
features/handle-mention.feature (1)
1-11: LGTM! Clear test coverage for public vs private mentions.The feature file now explicitly distinguishes between public and private mentions with appropriate assertions. The new private mention scenario correctly validates that private mentions do not generate notifications, aligning with the PR's objective to fix private mentions being treated as public.
features/step_definitions/mention_steps.js (2)
9-47: LGTM! Public mention step is well-structured.The step correctly:
- Validates actor existence
- Creates a public Note with Mention tags
- Uses default public delivery (to='as:Public')
- Stores the mention ID and objects for later assertions
Good error handling for missing actors.
49-94: LGTM! Private mention step correctly implements private delivery.The step properly creates a private mention by:
- Setting
object.toandactivity.toto only the local actor ID- Setting
ccto empty array- This ensures the mention is not public and won't be processed by the handler's public check
sagzy
force-pushed
the
bug/private-mentions
branch
from
8e83f76 to
0cd7aec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
features/step_definitions/mention_steps.js (1)
96-105: Fix the notification field matcher to check the correct field for ActivityPub URLs.The helper
waitForItemInNotificationschecksnotification.post?.idwhen given a string input, butpost.idis a numeric database identifier. Sincethis.mentionIdis an ActivityPub URL (object.id), the comparison will always fail. Update the matcher to checknotification.post?.urlinstead.
🧹 Nitpick comments (1)
features/step_definitions/mention_steps.js (1)
107-123: Consider a dedicated helper for negative assertions to improve test performance.The current pattern verifies absence by catching the retry timeout error after ~5 seconds. While functional, this approach is slow and couples the test to the error message string. Consider adding a dedicated
expectNotInNotificationshelper that returns a boolean after a shorter timeout, making negative assertions faster and less fragile.Example helper in
features/support/notifications.js:export async function expectNotInNotifications(input, maxWaitMs = 2000) { const startTime = Date.now(); while (Date.now() - startTime < maxWaitMs) { const response = await fetchActivityPub( 'https://self.test/.ghost/activitypub/v1/notifications', { headers: { Accept: 'application/ld+json' } } ); const json = await response.json(); const matcher = typeof input === 'string' ? (n) => n.post?.id === input : input; if (json.notifications.find(matcher)) { return false; } await new Promise((resolve) => setTimeout(resolve, 400)); } return true; }Then update the step:
Then('the mention is not in our notifications', async function () { if (!this.mentionId) { throw new Error( 'You need to call a step which creates a mention before this', ); } - try { - await waitForItemInNotifications(this.mentionId); - assert.fail(`Expected mention ${this.mentionId} to not be in notifications`); - } catch (error) { - assert.equal( - error.message, - `Max retries reached when waiting on item in notifications`, - ); - } + const notFound = await expectNotInNotifications(this.mentionId); + assert(notFound, `Expected mention ${this.mentionId} to not be in notifications`); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/handle-mention.feature(1 hunks)features/step_definitions/activitypub_steps.js(0 hunks)features/step_definitions/mention_steps.js(1 hunks)features/support/notifications.js(1 hunks)src/activity-handlers/create.handler.ts(2 hunks)
💤 Files with no reviewable changes (1)
- features/step_definitions/activitypub_steps.js
🚧 Files skipped from review as they are similar to previous changes (2)
- features/handle-mention.feature
- features/support/notifications.js
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activity-handlers/create.handler.ts
src/activity-handlers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement new handlers using the class-based pattern under /activity-handlers
Files:
src/activity-handlers/create.handler.ts
features/step_definitions/**/*_steps.js
📄 CodeRabbit inference engine (AGENTS.md)
Group Cucumber step definitions by high-level feature and name files with the _steps.js suffix under features/step_definitions
Files:
features/step_definitions/mention_steps.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 606
File: src/activitypub/fediverse-bridge.unit.test.ts:158-167
Timestamp: 2025-05-01T13:08:36.516Z
Learning: In the ActivityPub FediverseBridge test suite, expect assertions on sendActivity calls should verify the `username` property, not `handle`, as username is the correct parameter name.
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
Applied to files:
src/activity-handlers/create.handler.ts
🧬 Code graph analysis (1)
features/step_definitions/mention_steps.js (3)
features/support/fixtures.js (2)
createObject(64-105)createActivity(107-242)features/support/request.js (1)
fetchActivityPub(9-36)features/support/notifications.js (3)
found(32-34)found(71-71)waitForItemInNotifications(3-54)
🔇 Additional comments (3)
src/activity-handlers/create.handler.ts (1)
1-1: LGTM! Public content filtering correctly implemented.The addition of PUBLIC_COLLECTION-based filtering correctly prevents private mentions and DMs from being processed as public content. The logic appropriately checks both
toandccrecipients and exits early before any post storage occurs.Also applies to: 24-32
features/step_definitions/mention_steps.js (2)
9-47: LGTM! Public mention step correctly structured.The step definition properly constructs a public mention using the existing fixture helpers, stores the necessary context for assertions, and posts to the inbox. The default public addressing aligns with the test scenario.
49-94: LGTM! Private mention step correctly implements private addressing.The step properly overrides both object and activity recipients to create a private mention. Setting
toto the local actor and clearingccensures the Create activity will not include PUBLIC_COLLECTION and will be filtered by the handler.
ref https://linear.app/ghost/issue/BER-2987 - we currently don't support private mentions or DMs, and they were wrongly been treated as public content - added an extra check when handling Create activity, to ignore non-public content
sagzy
force-pushed
the
bug/private-mentions
branch
from
0cd7aec to
0629e08
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
features/step_definitions/mention_steps.js (2)
32-33: Consider explicit public addressing for clarity.The step relies on
createObjectdefaults to setto='as:Public'. While documented in the comment, making this explicit would improve robustness and clarity.Consider passing public addressing explicitly:
- // Create a public Note with the mention (uses defaults: to='as:Public') - const object = await createObject('Note', actor, `Hello ${mention}`, tags); + // Create a public Note with the mention + const object = await createObject('Note', actor, `Hello ${mention}`, tags); + object.to = 'as:Public';Alternatively, if
createObjectsupports it, passtoas a parameter to avoid mutation.
72-81: Post-creation mutation works but consider cleaner approach.The private mention implementation mutates
toandccproperties after object/activity creation. While functional, this pattern could be fragile if the creation functions add additional addressing logic in the future.If
createObjectandcreateActivitysupport custom addressing parameters, consider passing them during creation rather than mutating afterward. If not supported, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/handle-mention.feature(1 hunks)features/step_definitions/activitypub_steps.js(0 hunks)features/step_definitions/mention_steps.js(1 hunks)features/support/notifications.js(1 hunks)src/activity-handlers/create.handler.ts(2 hunks)
💤 Files with no reviewable changes (1)
- features/step_definitions/activitypub_steps.js
🚧 Files skipped from review as they are similar to previous changes (1)
- features/handle-mention.feature
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activity-handlers/create.handler.ts
src/activity-handlers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement new handlers using the class-based pattern under /activity-handlers
Files:
src/activity-handlers/create.handler.ts
features/step_definitions/**/*_steps.js
📄 CodeRabbit inference engine (AGENTS.md)
Group Cucumber step definitions by high-level feature and name files with the _steps.js suffix under features/step_definitions
Files:
features/step_definitions/mention_steps.js
🧠 Learnings (5)
📓 Common learnings
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1021
File: src/post/post.entity.ts:327-329
Timestamp: 2025-07-10T08:39:14.900Z
Learning: In the TryGhost/ActivityPub repository, when parsing data from Ghost CMS (such as post excerpts) using branded types with validation (like PostSummary.parse()), allouis prefers to let the parsing throw errors if Ghost sends incompatible data rather than adding defensive handling like truncation. This approach helps detect incompatibilities between the ActivityPub system and Ghost CMS early, ensuring issues are addressed at the source rather than silently ignored.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 606
File: src/activitypub/fediverse-bridge.unit.test.ts:158-167
Timestamp: 2025-05-01T13:08:36.516Z
Learning: In the ActivityPub FediverseBridge test suite, expect assertions on sendActivity calls should verify the `username` property, not `handle`, as username is the correct parameter name.
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
Applied to files:
src/activity-handlers/create.handler.ts
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to features/step_definitions/**/*_steps.js : Group Cucumber step definitions by high-level feature and name files with the _steps.js suffix under features/step_definitions
Applied to files:
features/step_definitions/mention_steps.js
📚 Learning: 2025-07-09T06:47:50.064Z
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Applied to files:
features/step_definitions/mention_steps.js
📚 Learning: 2025-05-27T09:09:22.110Z
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Applied to files:
features/step_definitions/mention_steps.js
🧬 Code graph analysis (1)
features/step_definitions/mention_steps.js (3)
features/support/fixtures.js (2)
createObject(64-105)createActivity(107-242)features/support/request.js (1)
fetchActivityPub(9-36)features/support/notifications.js (3)
found(32-34)found(71-71)waitForItemInNotifications(3-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (5)
features/support/notifications.js (1)
42-42: Error message simplified for test assertions.The dynamic MAX_RETRIES value was removed from the error message to enable exact string matching in tests (see mention_steps.js line 122).
src/activity-handlers/create.handler.ts (2)
1-1: LGTM!Correct import of PUBLIC_COLLECTION for public/private content detection.
24-32: Code is correct—verification confirms spread operator safety.Fedify's
toIdsandccIdsare non-functional (multi) properties that hold arrays of recipient IRIs, eliminating any TypeError risk from the spread operator at lines 24–26. The public/private detection logic correctly addresses the PR objective by filtering non-public content before post retrieval.features/step_definitions/mention_steps.js (2)
96-105: LGTM!Clean positive assertion with proper error handling for missing context.
107-125: Negative assertion correctly implemented.The retry-based negative check is consistent with the positive assertion (both check
notification.post?.id) and properly validates the error message fromwaitForItemInNotifications.
| Then('the mention is not in our notifications', async function () { | ||
| if (!this.mentionId) { | ||
| throw new Error( | ||
| 'You need to call a step which creates a mention before this', | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| await waitForItemInNotifications(this.mentionId); | ||
| assert.fail( | ||
| `Expected mention ${this.mentionId} to not be in notifications`, | ||
| ); | ||
| } catch (error) { | ||
| assert.equal( | ||
| error.message, | ||
| `Max retries reached when waiting on item in notifications`, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I'm not sure about this - this step is always going to take ~7.5s to run (waiting for the max timeout)
I get why we need to do this, but it doesn't feel great. Would we be better just keeping the happy path at the the e2e level, and then testing the non-happy paths at the unit (or integration) level?
Also, semantically, not receiving a private mention is not really a feature, more an implementation detail (because we don't support private mention) so we probably don't need to document it via cucumber?
What do you think?
There was a problem hiding this comment.
Yeah that's fair! I wanted to have a e2e test here to reproduce the actual bug that was reported - a private mention that is treated as public content - but agree the negative e2e test is slow.
Let me see if I move this test to unit / integratio
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/activity-handlers/create.handler.unit.test.ts (2)
94-110: Add test coverage for PUBLIC_COLLECTION in ccIds.The test correctly verifies early exit when
PUBLIC_COLLECTIONis not intoIds. However, the handler checks bothtoIdsandccIdswhen determining if a Create activity is public. Consider adding a complementary test to verify that the handler accepts a Create activity whenPUBLIC_COLLECTIONappears inccIds.Add this test case:
it('should process Create activity when PUBLIC_COLLECTION is in ccIds', async () => { const mockAccount = await createTestExternalAccount(1, { username: 'testuser', name: 'Test User', bio: null, url: null, avatarUrl: null, bannerImageUrl: null, customFields: null, apId: new URL('https://example.com/users/testuser'), apFollowers: null, apInbox: new URL('https://example.com/users/testuser/inbox'), }); const postApId = new URL('https://example.com/post/123'); const mockPost = new Post( 1, 'post-uuid', mockAccount, PostType.Article, Audience.Public, PostTitle.parse('Test Post'), PostSummary.parse('Test excerpt'), null, 'Test content', postApId, null, new Date(), { ghostAuthors: [] }, 0, 0, 0, null, null, null, [], postApId, ); vi.mocked(mockPostService.getByApId).mockResolvedValue(ok(mockPost)); const mockCreateJson = { '@context': 'https://www.w3.org/ns/activitystreams', type: 'Create', id: 'https://example.com/create/123', }; const mockCreate = { id: new URL('https://example.com/create/123'), objectId: new URL('https://example.com/post/123'), toIds: [new URL('https://example.com/users/specific-user')], ccIds: [PUBLIC_COLLECTION], // PUBLIC_COLLECTION in cc instead of to toJsonLd: vi.fn().mockResolvedValue(mockCreateJson), } as unknown as Create; await handler.handle(mockContext, mockCreate); expect(mockPostService.getByApId).toHaveBeenCalled(); expect(mockGlobalDb.set).toHaveBeenCalled(); });
112-179: Consider adding tests for PostService error paths.The handler has specific error handling for
upstream-error,not-a-post, andmissing-authorcases fromPostService.getByApId. Adding tests for these error paths would improve coverage and ensure the handler logs appropriately without crashing or storing incomplete data.Example test for error case:
it('should exit gracefully when PostService returns upstream-error', async () => { const mockCreate = { id: new URL('https://example.com/create/123'), objectId: new URL('https://example.com/post/123'), toIds: [PUBLIC_COLLECTION], ccIds: [], toJsonLd: vi.fn(), } as unknown as Create; vi.mocked(mockPostService.getByApId).mockResolvedValue( error('upstream-error'), ); await handler.handle(mockContext, mockCreate); expect(mockLogger.info).toHaveBeenCalledWith( 'Upstream error fetching post for create handling', expect.any(Object), ); expect(mockGlobalDb.set).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/handle-mention.feature(1 hunks)features/step_definitions/mention_steps.js(1 hunks)src/activity-handlers/create.handler.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- features/step_definitions/mention_steps.js
- features/handle-mention.feature
🧰 Additional context used
📓 Path-based instructions (4)
**/*.unit.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use the .unit.test.ts file extension for unit tests
Files:
src/activity-handlers/create.handler.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
All unit and integration test files should use the .test.ts suffix
Files:
src/activity-handlers/create.handler.unit.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activity-handlers/create.handler.unit.test.ts
src/activity-handlers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement new handlers using the class-based pattern under /activity-handlers
Files:
src/activity-handlers/create.handler.unit.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
Applied to files:
src/activity-handlers/create.handler.unit.test.ts
🧬 Code graph analysis (1)
src/activity-handlers/create.handler.unit.test.ts (5)
src/activity-handlers/create.handler.ts (1)
CreateHandler(7-72)src/post/post.service.ts (1)
PostService(68-550)src/app.ts (1)
ContextData(169-172)src/test/account-entity-test-helpers.ts (1)
createTestExternalAccount(92-118)src/core/result.ts (1)
ok(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (4)
src/activity-handlers/create.handler.unit.test.ts (4)
1-16: LGTM!Imports are clean and appropriate for the test suite. All necessary dependencies are included.
18-55: LGTM!Test setup is clean with proper mock initialization in
beforeEach. The mock structure correctly implements the expected interfaces.
58-92: LGTM!Early exit tests for missing
idandobjectIdare well-structured and correctly verify that no downstream operations occur.
112-179: ****The original review comment incorrectly identified the issue. The Post constructor has two distinct URL fields:
url(parameter 10) andapId(parameter 21). The test correctly passespostApIdto both fields because the post's URL is its ActivityPub ID—this is intentional design, not a copy-paste error. Parameter 9 iscontent(a string) and parameter 18 isthreadRoot(a number), neither of which is duplicated or problematic.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/activity-handlers/create.handler.unit.test.ts (2)
94-110: LGTM! This test validates the core PR objective.This test correctly verifies that private/unlisted Create activities (not addressed to PUBLIC_COLLECTION) are ignored, directly addressing the bug fix for private mentions being treated as public.
Consider adding a test case where PUBLIC_COLLECTION appears in
ccIdsrather thantoIdsto verify the handler correctly checks both arrays:it('should process Create activity when PUBLIC_COLLECTION is in ccIds', async () => { // Setup similar to valid test but with: // toIds: [new URL('https://example.com/users/specific-user')], // ccIds: [PUBLIC_COLLECTION], // Should still be treated as public });
112-179: LGTM! Comprehensive test for the success path.This test thoroughly validates that a valid public Create activity is processed correctly, including post retrieval and storage.
Consider adding test cases for error paths from
postService.getByApIdto ensure the handler gracefully handles failures:it('should handle upstream-error from postService', async () => { vi.mocked(mockPostService.getByApId).mockResolvedValue( error('upstream-error') ); const mockCreate = { id: new URL('https://example.com/create/123'), objectId: new URL('https://example.com/post/123'), toIds: [PUBLIC_COLLECTION], ccIds: [], toJsonLd: vi.fn(), } as unknown as Create; await handler.handle(mockContext, mockCreate); expect(mockLogger.info).toHaveBeenCalledWith( 'Upstream error fetching post for create handling', expect.any(Object) ); expect(mockGlobalDb.set).not.toHaveBeenCalled(); }); // Similar tests for 'not-a-post' and 'missing-author' errorsNote: You'll need to import
errorfrom@/core/resultalongside the existingokimport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/activity-handlers/create.handler.unit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.unit.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use the .unit.test.ts file extension for unit tests
Files:
src/activity-handlers/create.handler.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
All unit and integration test files should use the .test.ts suffix
Files:
src/activity-handlers/create.handler.unit.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activity-handlers/create.handler.unit.test.ts
src/activity-handlers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement new handlers using the class-based pattern under /activity-handlers
Files:
src/activity-handlers/create.handler.unit.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.064Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 606
File: src/activitypub/fediverse-bridge.unit.test.ts:158-167
Timestamp: 2025-05-01T13:08:36.516Z
Learning: In the ActivityPub FediverseBridge test suite, expect assertions on sendActivity calls should verify the `username` property, not `handle`, as username is the correct parameter name.
📚 Learning: 2025-09-22T13:57:08.390Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:57:08.390Z
Learning: Applies to src/activity-handlers/**/*.ts : Implement new handlers using the class-based pattern under /activity-handlers
Applied to files:
src/activity-handlers/create.handler.unit.test.ts
🧬 Code graph analysis (1)
src/activity-handlers/create.handler.unit.test.ts (6)
src/activity-handlers/create.handler.ts (1)
CreateHandler(7-72)src/post/post.service.ts (1)
PostService(68-550)src/app.ts (1)
ContextData(169-172)src/test/account-entity-test-helpers.ts (1)
createTestExternalAccount(92-118)src/post/post.entity.ts (1)
Post(143-687)src/core/result.ts (1)
ok(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (3)
src/activity-handlers/create.handler.unit.test.ts (3)
1-55: LGTM! Well-structured test setup.The test setup is clean with appropriate mocks for all dependencies. The beforeEach hook properly initializes fresh mocks for each test case, preventing test interdependencies.
58-74: LGTM! Correctly validates the missing id guard.This test properly verifies that the handler exits early when the Create activity lacks an id, preventing any downstream operations.
76-92: LGTM! Correctly validates the missing objectId guard.This test properly verifies that the handler exits early when the Create activity lacks an objectId, which is essential for preventing null pointer issues in downstream processing.
2 participants
ref https://linear.app/ghost/issue/BER-2987