Skip to content

Conversation

@mike182uk
Copy link
Member

ref https://linear.app/ghost/issue/BER-3013

Replaced per-account domainIsBlocked calls with a single getBlockedDomains query that returns all blocked domains upfront as a Set for O(1) lookups

ref https://linear.app/ghost/issue/BER-3013

Replaced per-account `domainIsBlocked` calls with a single `getBlockedDomains`
query that returns all blocked domains upfront as a `Set` for `O(1)` lookups
@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

This pull request refactors the domain blocking mechanism in the moderation service from per-entity boolean checks to a batch retrieval approach. The changes replace the domainIsBlocked method with a new getBlockedDomains method that returns a Set of lowercase blocked domain strings. The account follows view is updated to pre-fetch blocked domains once per request and reuse the set across all follows processing paths instead of checking each domain individually. The integration tests are rewritten to validate the new set-based retrieval behavior, including case normalization and hostname matching.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • src/moderation/moderation.service.ts: Review the replacement of domainIsBlocked(targetAccountId: number, domain: URL): Promise<boolean> with getBlockedDomains(accountId: number): Promise<Set<string>> to verify the query logic, lowercase normalization, and that the Set return type is correctly constructed.
  • src/http/api/views/account.follows.view.ts: Verify that the pre-fetched blockedDomains set is correctly initialized and threaded through all follows processing paths (processFollowsList, account-lookup flows). Check hostname extraction from apIdUrl and the domainBlockedByMe derivation logic.
  • src/moderation/moderation.service.integration.test.ts: Confirm test scenarios cover edge cases including subdomain handling, case-insensitive lookups, account-scoping, and empty result handling.
  • Error handling: Review the improved error logging in processFollowsList that continues processing instead of silent failure.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing an N+1 query performance issue in AccountFollowsView's domain block checks, which is the core objective of the PR.
Description check ✅ Passed The description is related to the changeset and explains the solution: replacing per-account calls with a single query returning blocked domains as a Set for O(1) lookups.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mike-ber-3013-account-follows-view-domain-block-checks

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5663f91 and ae1e28b.

📒 Files selected for processing (3)
  • src/http/api/views/account.follows.view.ts (3 hunks)
  • src/moderation/moderation.service.integration.test.ts (1 hunks)
  • src/moderation/moderation.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

All unit & integration test files should have the prefix .test.ts

Files:

  • src/moderation/moderation.service.integration.test.ts
**/*.integration.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

Integration test files should use the file extension .integration.test.ts to indicate the test type

Files:

  • src/moderation/moderation.service.integration.test.ts
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,tsx}: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash lookups instead - see ADR-0009
Always use the Result helper functions (isError, getError, getValue) with Result types instead of destructuring directly
Dependency injection parameter names must match registration names in Awilix CLASSIC injection mode
Routes should be defined using decorators (@APIRoute, @RequireRoles) rather than direct registration
Prefer class-based architecture with dependency injection over function factories for handlers
Repositories should not be used directly; they should be used through services
Controllers should be lean and delegate to services where appropriate
Views can talk directly to the database if necessary but should not be responsible for any business logic
Prefer immutable entities that generate domain events over mutable entities with dirty flags
Prefer error objects with context over string literal errors in Result types
Avoid mutable entities with dirty flags; use immutable entities with domain events instead

Files:

  • src/moderation/moderation.service.integration.test.ts
  • src/moderation/moderation.service.ts
  • src/http/api/views/account.follows.view.ts
🧠 Learnings (2)
📓 Common learnings
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-11-25T10:54:15.904Z
Learnt from: CR
Repo: TryGhost/ActivityPub PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T10:54:15.904Z
Learning: Applies to src/**/*.{ts,tsx} : Avoid mutable entities with dirty flags; use immutable entities with domain events instead

Applied to files:

  • src/http/api/views/account.follows.view.ts
🧬 Code graph analysis (1)
src/http/api/views/account.follows.view.ts (2)
src/http/api/types.ts (1)
  • MinimalAccountDTO (6-43)
src/account/utils.ts (1)
  • getAccountHandle (78-80)
⏰ 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). (3)
  • GitHub Check: build / Build Docker Images (linux/amd64, amd64)
  • GitHub Check: build / Lint
  • GitHub Check: build / Check yarn.lock
🔇 Additional comments (6)
src/moderation/moderation.service.ts (1)

127-133: LGTM! Clean implementation of batch domain retrieval.

The new getBlockedDomains method efficiently replaces N+1 per-entity checks with a single query. The lowercase normalization aligns correctly with URL.hostname behavior (which is always lowercase per the URL standard), ensuring consistent O(1) lookups.

src/http/api/views/account.follows.view.ts (3)

121-139: LGTM! Efficient batch retrieval replaces per-item checks.

The blocked domains are fetched once and reused across all accounts in the loop. Good use of apIdUrl to avoid duplicate URL parsing for both handle computation and domain-block checks.


361-386: LGTM! Correct use of hostname for domain-block lookups.

The distinction between host (for handles, includes port) and hostname (for domain blocks, excludes port) is correctly applied. The blocked domains are fetched once and reused for all items in the list.


405-418: LGTM! Remote actor fallback path correctly uses pre-fetched blocked domains.

The domain check uses item.hostname (the original AP ID URL), which is consistent with blocking based on the original source URL. The handle display using followsActor.id is appropriate since it reflects the canonical actor identity.

src/moderation/moderation.service.integration.test.ts (2)

320-348: LGTM! Comprehensive test coverage for the new getBlockedDomains method.

The tests properly validate Set-based retrieval including multiple domains, empty results, case normalization, account isolation, and subdomain handling.


362-386: Good test for URL.hostname compatibility.

The test at lines 382-385 correctly verifies that the caller pattern (using URL.hostname which is always lowercase) works with the Set lookups, ensuring the case normalization in the service aligns with real-world usage.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

return domainBlock === undefined;
}

async domainIsBlocked(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed domainIsBlocked as its not used anywhere anymore

@mike182uk mike182uk requested review from rmgpinto and sagzy December 18, 2025 08:55
Copy link
Contributor

@sagzy sagzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix @mike182uk!

CI seems to be failing for a random reason, triggered it again

@mike182uk mike182uk merged commit 8817595 into main Jan 6, 2026
29 of 31 checks passed
@mike182uk mike182uk deleted the mike-ber-3013-account-follows-view-domain-block-checks branch January 6, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants