Merged
Conversation
- Replace Post entity with primitive fields in PostDeletedEvent - Add toJSON/fromJSON for serialization support - Update all consumers (FediverseBridge, FeedUpdateService, NotificationEventService, GhostPostService) to use new getters - Change FeedService.removePostFromDiscoveryFeeds to accept postId - Change NotificationService.removePostNotifications to accept postId - Register PostDeletedEvent with EventSerializer in app.ts - Add comprehensive unit tests for serialization/deserialization - Update existing tests across all affected services Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
- Remove unused Post import from feed.service.ts - Fix formatting in feed.service.integration.test.ts - Handle nullable apFollowers in post repository event emission Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
WalkthroughPostDeletedEvent was converted to a SerializableEvent that carries primitive fields (postId, postApId, accountId, authorApId, authorApFollowers, authorUsername, authorIsInternal) and exposes getters, toJSON, and fromJSON. Call sites were updated to use these getters (e.g., getPostId, getPostApId, isAuthorInternal) instead of a Post object. FeedService and NotificationService now accept postId for deletion paths. The fediverse delete path was changed to validate event.isAuthorInternal(), fetch the account by event.getAccountId(), and build/send the Delete activity using account-derived actor/cc and event.getPostApId(). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
mike182uk
commented
Feb 26, 2026
| await deleteActivity.toJsonLd(), | ||
| ); | ||
|
|
||
| await this.sendActivityToFollowers(post.author, deleteActivity); |
Member
Author
There was a problem hiding this comment.
@cursoragent Why did you change this to use ctx.sendActivity directly?
| const post = event.getPost(); | ||
| const postId = event.getPostId(); | ||
|
|
||
| if (post.id !== null) { |
Member
Author
There was a problem hiding this comment.
@cursoragent Hm, why did we account for this originally if we don't need to now?
| return this.authorIsInternal; | ||
| } | ||
|
|
||
| static getName(): string { |
Member
Author
There was a problem hiding this comment.
@cursoragent Lets add non-static version whilst we are here
|
|
||
| // Remove the post from discovery feeds | ||
| await feedService.removePostFromDiscoveryFeeds(post); | ||
| await feedService.removePostFromDiscoveryFeeds(post.id as number); |
There was a problem hiding this comment.
Already fixed. The as number was needed because Post.id is typed as number | null, and the method now accepts number. But as number is a blunt type assertion — I've replaced it with post.id!, which is the more idiomatic non-null assertion for cases where we know the post has been saved and therefore has an id.
- Revert to using sendActivityToFollowers in FediverseBridge instead of inlining ctx.sendActivity directly - Add non-static getName() instance method to PostDeletedEvent - Replace 'as number' casts with non-null assertions in tests - Add test for instance getName() method Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
mike182uk
commented
Mar 2, 2026
src/activitypub/fediverse-bridge.ts
Outdated
|
|
||
| await this.sendActivityToFollowers(post.author, deleteActivity); | ||
| await this.sendActivityToFollowers( | ||
| { username: event.getAuthorUsername() } as Account, |
Member
Author
There was a problem hiding this comment.
@cursoragent Not keen on this, doesn't feel safe
mike182uk
commented
Mar 2, 2026
| post.apId.href, | ||
| post.author.id, | ||
| post.author.apId.href, | ||
| post.author.apFollowers?.href ?? '', |
Member
Author
There was a problem hiding this comment.
@cursoragent Does this default become problematic when used as a url? (new URL(''))
There was a problem hiding this comment.
Yes, new URL('') throws a TypeError, so the ?? '' fallback would blow up at runtime if it were ever reached. Since the previous commit added a proper account lookup via AccountService, I've now updated handlePostDeleted to use account.apId and account.apFollowers directly from the looked-up entity for the actor and cc fields on the Delete activity. The only event field still used for URL construction is postApId (since the post is gone). This sidesteps the empty string issue entirely, and the ?? '' in the repository is now harmless since no consumer wraps it in new URL() anymore.
Replace the partial object cast '{ username } as Account' with a
proper account lookup via AccountService.getAccountById. The author
account still exists after post deletion, so this is safe and avoids
passing an incomplete Account to sendActivityToFollowers.
Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
Since we now look up the account from AccountService, use account.apId
and account.apFollowers directly for the Delete activity instead of
reconstructing URLs from event strings. This avoids the problematic
empty string fallback (new URL('') throws) and is more correct — the
only event field needed is postApId since the post no longer exists.
Also replace post.id as number with post.id! for consistency.
Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
![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 (4)
src/notification/notification.service.ts (1)
411-415: Group the OR condition to make future scoping safer.Current behavior is fine, but wrapping the two post predicates avoids accidental broad deletes if more
whereclauses are later added.Safer query shape
async removePostNotifications(postId: number) { await this.db('notifications') - .where('post_id', postId) - .orWhere('in_reply_to_post_id', postId) + .where((qb) => { + qb.where('post_id', postId).orWhere( + 'in_reply_to_post_id', + postId, + ); + }) .delete(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notification/notification.service.ts` around lines 411 - 415, In removePostNotifications, the .where/.orWhere predicates should be grouped to avoid accidental broad deletes when additional where clauses are added later; wrap the two predicates in a single grouped where callback on this.db('notifications') (e.g., use a .where(function() { this.where('post_id', postId).orWhere('in_reply_to_post_id', postId) })) so the OR is scoped together before calling .delete(), keeping the awaited call and intent unchanged.src/feed/feed.service.ts (1)
594-597: Align the JSDoc param with the new method signature.The doc block still describes
@param post, but the method now acceptspostId.Suggested doc fix
- * `@param` post Post to remove from discovery feeds + * `@param` postId ID of the post to remove from discovery feeds */ async removePostFromDiscoveryFeeds(postId: number) { await this.db('discovery_feeds').where('post_id', postId).delete(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feed/feed.service.ts` around lines 594 - 597, Update the JSDoc for the removePostFromDiscoveryFeeds method to reflect the current signature: replace the incorrect "@param post" with "@param postId" and adjust the description to say "Post ID to remove from discovery feeds" (or similar), ensuring the param name matches the function parameter and any type annotation in the doc matches the method's usage.src/activitypub/fediverse-bridge.ts (1)
125-151: Add a defensive internal-account guard after account lookup.This path currently trusts
event.isAuthorInternal()only. Adding a check on the resolved account avoids dispatching deletes if payload/state drift occurs.Suggested change
- if (!account) { + if (!account || !account.isInternal) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activitypub/fediverse-bridge.ts` around lines 125 - 151, After resolving account via this.accountService.getAccountById (after the existing event.isAuthorInternal() check), add a defensive guard that verifies the resolved account is internal before proceeding (e.g., check account.isInternal or account.isAuthorInternal equivalent on the Account object) and return early if not; this prevents sending Delete activities for mismatched/external accounts when building the Delete (Delete, ctx.getObjectUri, sendActivityToFollowers) and storing it in ctx.data.globaldb.src/activitypub/fediverse-bridge.unit.test.ts (1)
92-114: Strengthen this test by asserting the emittedDeletepayload fields.Given the refactor switched actor/object sourcing, asserting those values would prevent silent regressions (not just verifying call existence).
Suggested assertion additions
- expect(sendActivity.mock.lastCall).toBeDefined(); + const sendActivityCall = sendActivity.mock.lastCall; + expect(sendActivityCall).toBeDefined(); + expect(sendActivityCall![0]).toMatchObject({ username: author.username }); + expect(sendActivityCall![1]).toBe('followers');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activitypub/fediverse-bridge.unit.test.ts` around lines 92 - 114, The test currently only asserts accountService.getAccountById was called; instead also capture/spy the code that emits the ActivityPub Delete (the handler invoked by events.emit for PostDeletedEvent) and assert the emitted Delete payload includes the correct fields: type === 'Delete', actor === author's apId ('https://author.com/user/123'), object === post URL ('https://author.com/post/123') and (optionally) to === followers inbox ('https://author.com/user/123/followers') so the refactor that changed actor/object sourcing is covered; keep the existing PostDeletedEvent creation and events.emit call, then add an assertion like expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({type: 'Delete', actor: 'https://author.com/user/123', object: 'https://author.com/post/123'})) while still asserting accountService.getAccountById was invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/post/post-deleted.event.ts`:
- Around line 67-108: The deserializer currently only checks that postApId,
authorApId, and authorApFollowers are strings but not valid URLs; update the
static deserialization logic (the code block that constructs PostDeletedEvent)
to validate those three fields as proper URLs by attempting new URL(value) for
each and throwing a descriptive Error if any are invalid; keep the existing type
checks, then perform URL validation before returning new
PostDeletedEvent(postId, postApId, accountId, authorApId, authorApFollowers,
authorUsername, authorIsInternal).
---
Nitpick comments:
In `@src/activitypub/fediverse-bridge.ts`:
- Around line 125-151: After resolving account via
this.accountService.getAccountById (after the existing event.isAuthorInternal()
check), add a defensive guard that verifies the resolved account is internal
before proceeding (e.g., check account.isInternal or account.isAuthorInternal
equivalent on the Account object) and return early if not; this prevents sending
Delete activities for mismatched/external accounts when building the Delete
(Delete, ctx.getObjectUri, sendActivityToFollowers) and storing it in
ctx.data.globaldb.
In `@src/activitypub/fediverse-bridge.unit.test.ts`:
- Around line 92-114: The test currently only asserts
accountService.getAccountById was called; instead also capture/spy the code that
emits the ActivityPub Delete (the handler invoked by events.emit for
PostDeletedEvent) and assert the emitted Delete payload includes the correct
fields: type === 'Delete', actor === author's apId
('https://author.com/user/123'), object === post URL
('https://author.com/post/123') and (optionally) to === followers inbox
('https://author.com/user/123/followers') so the refactor that changed
actor/object sourcing is covered; keep the existing PostDeletedEvent creation
and events.emit call, then add an assertion like
expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({type: 'Delete',
actor: 'https://author.com/user/123', object: 'https://author.com/post/123'}))
while still asserting accountService.getAccountById was invoked.
In `@src/feed/feed.service.ts`:
- Around line 594-597: Update the JSDoc for the removePostFromDiscoveryFeeds
method to reflect the current signature: replace the incorrect "@param post"
with "@param postId" and adjust the description to say "Post ID to remove from
discovery feeds" (or similar), ensuring the param name matches the function
parameter and any type annotation in the doc matches the method's usage.
In `@src/notification/notification.service.ts`:
- Around line 411-415: In removePostNotifications, the .where/.orWhere
predicates should be grouped to avoid accidental broad deletes when additional
where clauses are added later; wrap the two predicates in a single grouped where
callback on this.db('notifications') (e.g., use a .where(function() {
this.where('post_id', postId).orWhere('in_reply_to_post_id', postId) })) so the
OR is scoped together before calling .delete(), keeping the awaited call and
intent unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 168ee50d-73f8-4f7c-bcd1-87af61ad7d92
📒 Files selected for processing (16)
src/activitypub/fediverse-bridge.tssrc/activitypub/fediverse-bridge.unit.test.tssrc/app.tssrc/feed/feed-update.service.tssrc/feed/feed-update.service.unit.test.tssrc/feed/feed.service.integration.test.tssrc/feed/feed.service.tssrc/ghost/ghost-post.service.tssrc/notification/notification-event.service.tssrc/notification/notification-event.service.unit.test.tssrc/notification/notification.service.integration.test.tssrc/notification/notification.service.tssrc/post/post-deleted.event.tssrc/post/post-deleted.event.unit.test.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
Comment on lines
+67
to
+108
| if (!('postApId' in data) || typeof data.postApId !== 'string') { | ||
| throw new Error('postApId must be a string'); | ||
| } | ||
|
|
||
| if (!('accountId' in data) || typeof data.accountId !== 'number') { | ||
| throw new Error('accountId must be a number'); | ||
| } | ||
|
|
||
| if (!('authorApId' in data) || typeof data.authorApId !== 'string') { | ||
| throw new Error('authorApId must be a string'); | ||
| } | ||
|
|
||
| if ( | ||
| !('authorApFollowers' in data) || | ||
| typeof data.authorApFollowers !== 'string' | ||
| ) { | ||
| throw new Error('authorApFollowers must be a string'); | ||
| } | ||
|
|
||
| if ( | ||
| !('authorUsername' in data) || | ||
| typeof data.authorUsername !== 'string' | ||
| ) { | ||
| throw new Error('authorUsername must be a string'); | ||
| } | ||
|
|
||
| if ( | ||
| !('authorIsInternal' in data) || | ||
| typeof data.authorIsInternal !== 'boolean' | ||
| ) { | ||
| throw new Error('authorIsInternal must be a boolean'); | ||
| } | ||
|
|
||
| return new PostDeletedEvent( | ||
| data.postId, | ||
| data.postApId, | ||
| data.accountId, | ||
| data.authorApId, | ||
| data.authorApFollowers, | ||
| data.authorUsername, | ||
| data.authorIsInternal, | ||
| ); |
There was a problem hiding this comment.
Validate URL-shaped fields during deserialization to prevent downstream runtime failures.
postApId, authorApId, and authorApFollowers are currently checked as strings only. Invalid URL strings can slip through and break consumers later when they build activities.
Proposed fix
+ private static ensureValidUrl(value: string, field: string): string {
+ try {
+ return new URL(value).href;
+ } catch {
+ throw new Error(`${field} must be a valid URL`);
+ }
+ }
+
static fromJSON(data: Record<string, unknown>): PostDeletedEvent {
if (!('postId' in data) || typeof data.postId !== 'number') {
throw new Error('postId must be a number');
}
@@
if (
!('authorIsInternal' in data) ||
typeof data.authorIsInternal !== 'boolean'
) {
throw new Error('authorIsInternal must be a boolean');
}
+ const postApId = PostDeletedEvent.ensureValidUrl(
+ data.postApId,
+ 'postApId',
+ );
+ const authorApId = PostDeletedEvent.ensureValidUrl(
+ data.authorApId,
+ 'authorApId',
+ );
+ const authorApFollowers = PostDeletedEvent.ensureValidUrl(
+ data.authorApFollowers,
+ 'authorApFollowers',
+ );
+
return new PostDeletedEvent(
data.postId,
- data.postApId,
+ postApId,
data.accountId,
- data.authorApId,
- data.authorApFollowers,
+ authorApId,
+ authorApFollowers,
data.authorUsername,
data.authorIsInternal,
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/post/post-deleted.event.ts` around lines 67 - 108, The deserializer
currently only checks that postApId, authorApId, and authorApFollowers are
strings but not valid URLs; update the static deserialization logic (the code
block that constructs PostDeletedEvent) to validate those three fields as proper
URLs by attempting new URL(value) for each and throwing a descriptive Error if
any are invalid; keep the existing type checks, then perform URL validation
before returning new PostDeletedEvent(postId, postApId, accountId, authorApId,
authorApFollowers, authorUsername, authorIsInternal).
- Fix JSDoc param name in FeedService.removePostFromDiscoveryFeeds - Add defensive account.isInternal guard in FediverseBridge after account lookup for defense-in-depth - Strengthen delete test assertions to verify sendActivity payload (username and followers target) Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/activitypub/fediverse-bridge.unit.test.ts (2)
92-123: Strengthen delete-path assertions to validate activity payload, not just dispatch call shape.This test currently won’t catch regressions in
Delete.actor,Delete.object, orDelete.cc. Also, Line 92–97 mock account should includeapFollowersto mirror production usage.✅ Suggested test hardening
const author = { id: 123, username: 'index', apId: new URL('https://author.com/user/123'), + apFollowers: new URL('https://author.com/user/123/followers'), isInternal: true, } as AccountEntity; @@ expect(sendActivityCall![1]).toBe('followers'); + const deleteActivity = sendActivityCall![2]; + const deleteJsonLd = (await deleteActivity.toJsonLd()) as { + actor: string; + object: string; + cc: string | string[]; + }; + expect(deleteJsonLd.actor).toBe(author.apId.toString()); + expect(deleteJsonLd.object).toBe('https://author.com/post/123');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activitypub/fediverse-bridge.unit.test.ts` around lines 92 - 123, The test should mock the account with the apFollowers field and assert the actual Delete activity payload rather than only the dispatch call shape: update the mocked author object returned by accountService.getAccountById to include apFollowers (e.g., the followers URL used by PostDeletedEvent), then after emitting PostDeletedEvent and awaiting nextTick, inspect sendActivity.mock.lastCall[0] (the activity) and assert Delete.actor equals the author's apId, Delete.object equals the deleted post URL, and Delete.cc includes the author's apFollowers (and/or followers collection name 'followers') in addition to the existing assertions that sendActivity was called and context.data.globaldb.set was invoked.
131-145: Assert no account lookup for external-author delete events.Adding this expectation will lock in the intended early-return behavior and prevent accidental DB work in this path.
Suggested assertion
await nextTick(); + expect(accountService.getAccountById).not.toHaveBeenCalled(); expect(sendActivity.mock.lastCall).not.toBeDefined(); expect(context.data.globaldb.set).not.toHaveBeenCalledOnce();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activitypub/fediverse-bridge.unit.test.ts` around lines 131 - 145, Add an assertion to the PostDeletedEvent test to ensure external-author delete events do not perform account lookups or DB writes: after emitting the PostDeletedEvent (created via PostDeletedEvent(...)) and awaiting nextTick(), assert that sendActivity.mock was not called and that context.data.globaldb.set was not invoked (e.g., expect(sendActivity.mock.lastCall).not.toBeDefined() and expect(context.data.globaldb.set).not.toHaveBeenCalledOnce()). This locks in the early-return behavior for external-author deletes and prevents accidental DB work; update the test around events.emit(PostDeletedEvent.getName(), event) / nextTick() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/activitypub/fediverse-bridge.ts`:
- Around line 129-135: The current early return after calling
accountService.getAccountById(event.getAccountId()) (and the analogous check at
the other location) drops Delete deliveries when the DB lookup misses; change
the logic in fediverse-bridge: where you use accountService.getAccountById(...)
and check account?.isInternal, add a fallback to use the serialized author/actor
fields contained in the event (e.g., event.getAuthor(), event.getActor(), or the
serialized author properties on the event) when account is null or not internal
so Delete handling can proceed using the event’s embedded identity data; ensure
the same fallback is applied at the second occurrence mentioned in the comment.
- Line 141: The code constructs an Activity 'object' with new
URL(event.getPostApId()) which can throw on malformed persisted data; guard by
validating/parsing the string first: retrieve event.getPostApId(), ensure it's a
non-empty string, wrap new URL(...) in a try/catch (or use a safe URL validation
helper) and handle failures by logging the bad value and skipping/returning
early from the async handler instead of letting an exception propagate; update
the activity construction site (the line using new URL(...)) to use the safely
parsed URL value or a guarded fallback.
---
Nitpick comments:
In `@src/activitypub/fediverse-bridge.unit.test.ts`:
- Around line 92-123: The test should mock the account with the apFollowers
field and assert the actual Delete activity payload rather than only the
dispatch call shape: update the mocked author object returned by
accountService.getAccountById to include apFollowers (e.g., the followers URL
used by PostDeletedEvent), then after emitting PostDeletedEvent and awaiting
nextTick, inspect sendActivity.mock.lastCall[0] (the activity) and assert
Delete.actor equals the author's apId, Delete.object equals the deleted post
URL, and Delete.cc includes the author's apFollowers (and/or followers
collection name 'followers') in addition to the existing assertions that
sendActivity was called and context.data.globaldb.set was invoked.
- Around line 131-145: Add an assertion to the PostDeletedEvent test to ensure
external-author delete events do not perform account lookups or DB writes: after
emitting the PostDeletedEvent (created via PostDeletedEvent(...)) and awaiting
nextTick(), assert that sendActivity.mock was not called and that
context.data.globaldb.set was not invoked (e.g.,
expect(sendActivity.mock.lastCall).not.toBeDefined() and
expect(context.data.globaldb.set).not.toHaveBeenCalledOnce()). This locks in the
early-return behavior for external-author deletes and prevents accidental DB
work; update the test around events.emit(PostDeletedEvent.getName(), event) /
nextTick() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9df72483-efb9-4170-85e1-7bf5c001197b
📒 Files selected for processing (3)
src/activitypub/fediverse-bridge.tssrc/activitypub/fediverse-bridge.unit.test.tssrc/feed/feed.service.ts
Comment on lines
+129
to
135
| const account = await this.accountService.getAccountById( | ||
| event.getAccountId(), | ||
| ); | ||
|
|
||
| if (!account || !account.isInternal) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Delete federation is dropped when account lookup misses, despite serialized event having all required data.
Line 129–135 returns early if the account is not found. That reintroduces runtime DB dependency into a serialized event flow and can silently lose Delete delivery for valid persisted events.
💡 Suggested fix (fallback to serialized author fields)
- const account = await this.accountService.getAccountById(
- event.getAccountId(),
- );
-
- if (!account || !account.isInternal) {
- return;
- }
+ const account = await this.accountService.getAccountById(
+ event.getAccountId(),
+ );
+ if (account && !account.isInternal) {
+ return;
+ }
+
+ const actorApId = account?.apId ?? new URL(event.getAuthorApId());
+ const followersApId =
+ account?.apFollowers ?? new URL(event.getAuthorApFollowers());
+ const username = account?.username ?? event.getAuthorUsername();
const ctx = this.fedifyContextFactory.getFedifyContext();
const deleteActivity = new Delete({
id: ctx.getObjectUri(Delete, { id: uuidv4() }),
- actor: account.apId,
+ actor: actorApId,
object: new URL(event.getPostApId()),
to: PUBLIC_COLLECTION,
- cc: account.apFollowers,
+ cc: followersApId,
});
await ctx.data.globaldb.set(
[deleteActivity.id!.href],
await deleteActivity.toJsonLd(),
);
- await this.sendActivityToFollowers(account, deleteActivity);
+ await ctx.sendActivity(
+ { username },
+ 'followers',
+ deleteActivity,
+ { preferSharedInbox: true },
+ );Also applies to: 151-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/activitypub/fediverse-bridge.ts` around lines 129 - 135, The current
early return after calling accountService.getAccountById(event.getAccountId())
(and the analogous check at the other location) drops Delete deliveries when the
DB lookup misses; change the logic in fediverse-bridge: where you use
accountService.getAccountById(...) and check account?.isInternal, add a fallback
to use the serialized author/actor fields contained in the event (e.g.,
event.getAuthor(), event.getActor(), or the serialized author properties on the
event) when account is null or not internal so Delete handling can proceed using
the event’s embedded identity data; ensure the same fallback is applied at the
second occurrence mentioned in the comment.
| actor: post.author.apId, | ||
| object: post.apId, | ||
| actor: account.apId, | ||
| object: new URL(event.getPostApId()), |
There was a problem hiding this comment.
Guard URL parsing for serialized postApId.
Line 141 (new URL(event.getPostApId())) can throw and fail the async handler on malformed persisted data. Add explicit parsing guard before constructing the activity.
🛡️ Suggested guard
+ let postObjectId: URL;
+ try {
+ postObjectId = new URL(event.getPostApId());
+ } catch {
+ return;
+ }
+
const deleteActivity = new Delete({
id: ctx.getObjectUri(Delete, { id: uuidv4() }),
actor: account.apId,
- object: new URL(event.getPostApId()),
+ object: postObjectId,
to: PUBLIC_COLLECTION,
cc: account.apFollowers,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/activitypub/fediverse-bridge.ts` at line 141, The code constructs an
Activity 'object' with new URL(event.getPostApId()) which can throw on malformed
persisted data; guard by validating/parsing the string first: retrieve
event.getPostApId(), ensure it's a non-empty string, wrap new URL(...) in a
try/catch (or use a safe URL validation helper) and handle failures by logging
the bad value and skipping/returning early from the async handler instead of
letting an exception propagate; update the activity construction site (the line
using new URL(...)) to use the safely parsed URL value or a guarded fallback.
- Add apFollowers to mock account in delete test - Assert no account lookup occurs for external-author deletes Co-authored-by: Michael Barrett <mike182uk@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/activitypub/fediverse-bridge.unit.test.ts (1)
102-110: Optional: factorPostDeletedEventcreation into a small test helper.Both tests duplicate the same constructor payload except
authorIsInternal; a helper would reduce brittleness when event fields evolve again.♻️ Minimal refactor sketch
+const makePostDeletedEvent = (authorIsInternal: boolean) => + new PostDeletedEvent( + 456, + 'https://author.com/post/123', + 123, + 'https://author.com/user/123', + 'https://author.com/user/123/followers', + 'index', + authorIsInternal, + ); -const event = new PostDeletedEvent( - 456, - 'https://author.com/post/123', - 123, - 'https://author.com/user/123', - 'https://author.com/user/123/followers', - 'index', - true, -); +const event = makePostDeletedEvent(true); -const event = new PostDeletedEvent( - 456, - 'https://author.com/post/123', - 123, - 'https://author.com/user/123', - 'https://author.com/user/123/followers', - 'index', - false, -); +const event = makePostDeletedEvent(false);Also applies to: 132-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activitypub/fediverse-bridge.unit.test.ts` around lines 102 - 110, Tests duplicate the same PostDeletedEvent constructor payload; add a small helper (e.g., makePostDeletedEvent or buildPostDeletedEvent) in the test file to return a new PostDeletedEvent with the shared defaults and an optional parameter to override authorIsInternal so both test cases call the helper (one with default, one with authorIsInternal = false) instead of repeating the full constructor payload; update invocations at the two spots (the existing PostDeletedEvent calls) to use the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/activitypub/fediverse-bridge.unit.test.ts`:
- Around line 102-110: Tests duplicate the same PostDeletedEvent constructor
payload; add a small helper (e.g., makePostDeletedEvent or
buildPostDeletedEvent) in the test file to return a new PostDeletedEvent with
the shared defaults and an optional parameter to override authorIsInternal so
both test cases call the helper (one with default, one with authorIsInternal =
false) instead of repeating the full constructor payload; update invocations at
the two spots (the existing PostDeletedEvent calls) to use the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c04b5815-007e-48fb-a28e-7c9631457e56
📒 Files selected for processing (1)
src/activitypub/fediverse-bridge.unit.test.ts
mike182uk
deleted the
cursor/BER-3166-post-deleted-event-serialization-bd92
branch
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
2 participants
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.
Refactor
PostDeletedEventto implementSerializableEventand use primitive data to comply with ADR-0011 and ensure event data persistence after post deletion.The
PostDeletedEventpreviously contained the fullPostentity, which is problematic for deletion events as the post may no longer exist in the database when the event is processed. This change ensures all necessary data for federation and other consumers is explicitly stored as primitive, serializable fields within the event itself.Linear Issue: BER-3166