Removed unnecessary database queries in dispatchers #1500
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds account key-pair retrieval: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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 |
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
2 times, most recently
from
694e215 to
67a200d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
3 times, most recently
from
dfeac87 to
f5bb0c6
Compare
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
2 times, most recently
from
c65b17f to
16ddfdf
Compare
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
from
16ddfdf to
af965d8
Compare
…p diff smaller on this one
![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
🤖 Fix all issues with AI agents
In `@src/dispatchers.ts`:
- Around line 599-602: The code in createAnnounceHandler fetches
ctx.data.hostSite into hostSite and throws if missing, but hostSite is never
used; remove the unused retrieval and validation by deleting the const hostSite
= ctx.data.hostSite; and the if (!hostSite) { throw new Error(`Site not found
for host: ${ctx.host`); } lines from createAnnounceHandler so the function no
longer performs this redundant check.
♻️ Duplicate comments (1)
src/app.ts (1)
256-267: Consider logging when host data loading fails.When
loadDataForHostreturns an error, the code silently continues without settinghostSiteorhostAccount. This could make debugging difficult if dispatchers later fail due to missing host data.Proposed logging addition
if (!isError(result)) { const { site, account } = getValue(result); ctx.data.hostSite = site; ctx.data.hostAccount = account; + } else { + ctx.data.logger.warn( + 'Failed to load host data for {host}: {error}', + { host: ctx.host, error: getError(result) }, + ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/events/pubsub-http.tssrc/helpers/fedify.tssrc/http/api/follow.controller.tssrc/http/api/like.controller.tssrc/http/api/post.controller.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/http/api/like.controller.ts
- src/dispatchers.unit.test.ts
- src/http/api/post.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/http/api/follow.controller.tssrc/events/pubsub-http.tssrc/helpers/fedify.tssrc/app.tssrc/dispatchers.ts
🧠 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: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders, so additional SQL normalization is not needed for Sentry fingerprinting.
📚 Learning: 2025-08-21T13:32:58.582Z
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1223
File: src/http/api/bluesky.controller.ts:0-0
Timestamp: 2025-08-21T13:32:58.582Z
Learning: In the Ghost ActivityPub codebase, `ctx.get('account')` in controller methods should always return an account because the middleware guarantees this. Guards for missing account are unnecessary in this system.
Applied to files:
src/http/api/follow.controller.ts
📚 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/activity-handlers/**/*.{ts,tsx} : Legacy code in `dispatchers.ts` should not be used as a reference; new handlers should follow the class-based pattern in `/activity-handlers/` - see ADR-0006
Applied to files:
src/dispatchers.ts
📚 Learning: 2025-06-16T07:48:37.253Z
Learnt from: allouis
Repo: TryGhost/ActivityPub PR: 851
File: src/http/api/views/reply.chain.view.ts:89-94
Timestamp: 2025-06-16T07:48:37.253Z
Learning: In the ActivityPub codebase, the team prefers to let the code throw errors for invalid data (like malformed URLs) rather than adding defensive handling, as it indicates the system is in an undefined state that should be addressed at the source.
Applied to files:
src/dispatchers.ts
📚 Learning: 2025-07-31T16:35:35.813Z
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders, so additional SQL normalization is not needed for Sentry fingerprinting.
Applied to files:
src/dispatchers.ts
🧬 Code graph analysis (1)
src/app.ts (4)
src/site/site.service.ts (1)
Site(9-14)src/http/host-data-context-loader.ts (1)
HostDataContextLoader(9-96)src/core/result.ts (2)
isError(22-24)getValue(26-28)src/activitypub/fedify-context.factory.ts (1)
FedifyContextFactory(5-25)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (14)
src/app.ts (3)
172-177: LGTM!The
ContextDatatype extension is well-designed. MakinghostSiteandhostAccountnullable allows for contexts where host data isn't available (like queue processing), while enabling the optimization in request contexts.
778-795: LGTM!This middleware correctly propagates the host data already loaded by
createHostDataContextMiddlewareinto the Fedify context. The ordering ensuressiteandaccountare available on the Hono context before this middleware runs.
908-923: LGTM!The federation wiring correctly passes
hostSiteandhostAccountfrom the Hono context into the FedifyContextData, completing the data flow for federation requests.src/http/api/follow.controller.ts (2)
33-38: LGTM!The Fedify context creation correctly includes
hostSiteandhostAccount, consistent with the broader refactoring to pass host data through context rather than querying the database in dispatchers.
152-157: LGTM!Consistent with the changes in
handleFollow.src/helpers/fedify.ts (1)
5-19: LGTM!Simplifying to pass
ctxDatadirectly is cleaner since theContextDatatype now includes all required fields (globaldb,logger,hostSite,hostAccount).src/events/pubsub-http.ts (1)
56-68: LGTM!The explicit null values for
hostSiteandhostAccountwith the explanatory comment are a good approach. This documents that host data could be loaded here if needed in the future, while avoiding unnecessary DB queries for current use cases.src/dispatchers.ts (7)
44-83: LGTM!The
actorDispatchercorrectly retrieveshostAccountfrom context data and properly handles nullable URL fields with conditional checks, avoiding the previousnew URL('')crash risk.
85-134: LGTM!The
keypairDispatchercorrectly uses!hostAccountto handle both null and undefined cases. The intentional DB query for key pairs (viaaccountService.getKeyPair) aligns with the PR objective of not exposing private keys on the Account entity. The exhaustive error handling is well-implemented.
214-230: LGTM!The
handleAnnouncedCreatefunction correctly retrieveshostSitefrom context data and uses it for the follower check. Throwing an error whenhostSiteis missing is appropriate since the function cannot proceed without it.
784-851: LGTM!Both
createFollowersDispatcherandcreateFollowingDispatchercorrectly retrieve host data from context and usehostAccount.idfor their respective queries. The consistent error handling pattern is appropriate.
853-889: LGTM!Both counters consistently retrieve host data from context and use
hostAccount.idfor counting operations.
895-944: LGTM!The
createOutboxDispatcheris well-refactored: the factory now only requirespostService, with host data derived from context. BothhostAccount.idand the fullhostAccountobject are correctly used for their respective purposes.
946-960: LGTM!Consistent with the other counter implementations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/step_definitions/index.js (1)
99-109: Fix the duplicateawaitkeyword.Line 103 has
await await Promise.all([...])which contains a redundantawait. While this doesn't cause a runtime error (the second await just awaits the resolved value), it's unnecessary and likely a typo.🐛 Proposed fix
Before(async function reset() { await resetWiremock(); await purgeSubscriptions(); await resetDatabase(); - await await Promise.all([ + await Promise.all([ setupSelfSite(), fetchActivityPub('https://alice.test/.ghost/activitypub/v1/site'), fetchActivityPub('https://bob.test/.ghost/activitypub/v1/site'), fetchActivityPub('https://charlie.test/.ghost/activitypub/v1/site'), ]); });
🧹 Nitpick comments (1)
features/support/pubsub.js (1)
6-10: Consider filtering out undefined subscription names.If any of the environment variables are not set, the
SUBSCRIPTIONSarray will containundefinedvalues. While this is handled by the catch block, filtering them out explicitly would be cleaner and avoid unnecessary API calls.♻️ Suggested improvement
const SUBSCRIPTIONS = [ process.env.MQ_PUBSUB_SUBSCRIPTION_NAME, process.env.MQ_PUBSUB_RETRY_SUBSCRIPTION_NAME, process.env.MQ_PUBSUB_GHOST_SUBSCRIPTION_NAME, -]; +].filter(Boolean);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.ymlfeatures/step_definitions/index.jsfeatures/support/pubsub.js
🧰 Additional context used
📓 Path-based instructions (1)
features/step_definitions/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Step definitions should be grouped together by the high-level feature they are testing, not necessarily in 1-to-1 mapping with feature files
Files:
features/step_definitions/index.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders, so additional SQL normalization is not needed for Sentry fingerprinting.
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: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders (?), so additional SQL normalization is not needed for Sentry fingerprinting. The actual parameter values are stored separately in the bindings array.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1223
File: src/http/api/bluesky.controller.ts:0-0
Timestamp: 2025-08-21T13:32:58.582Z
Learning: In the Ghost ActivityPub codebase, `ctx.get('account')` in controller methods should always return an account because the middleware guarantees this. Guards for missing account are unnecessary in this system.
📚 Learning: 2025-06-10T10:07:58.527Z
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 811
File: src/app.ts:249-250
Timestamp: 2025-06-10T10:07:58.527Z
Learning: The MQ_PUBSUB_GHOST_TOPIC_NAME and MQ_PUBSUB_GHOST_SUBSCRIPTION_NAME environment variables were temporarily removed from validation checks in src/app.ts to allow the app to boot, but they are planned to be re-introduced later. The docker-compose.yml entries for these variables should be preserved for future use.
Applied to files:
features/support/pubsub.jsdocker-compose.yml
📚 Learning: 2025-06-11T12:57:36.707Z
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 826
File: src/app.ts:228-234
Timestamp: 2025-06-11T12:57:36.707Z
Learning: The ActivityPub project prefers to use default placeholder values for missing environment variables (e.g., MQ_PUBSUB_*), relying on runtime errors rather than explicit fail-fast checks during application startup.
Applied to files:
docker-compose.yml
🧬 Code graph analysis (1)
features/step_definitions/index.js (1)
features/support/pubsub.js (1)
purgeSubscriptions(32-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: Cursor Bugbot
🔇 Additional comments (4)
docker-compose.yml (1)
210-214: LGTM!The environment variables are correctly configured and consistent with the
pubsub-testingservice definitions (lines 256-265). ThePUBSUB_EMULATOR_HOSTformat is appropriate for the@google-cloud/pubsubclient's emulator mode.features/support/pubsub.js (1)
32-46: LGTM!The seek-to-future-timestamp pattern is a valid approach for purging Pub/Sub messages in test environments. The parallel execution and error suppression are appropriate for a test utility where subscriptions may not always exist.
features/step_definitions/index.js (2)
1-7: LGTM!The imports for
AfterandpurgeSubscriptionsare correctly added to support the new test lifecycle hooks.Also applies to: 17-17
111-113: LGTM!The
Afterhook correctly purges subscriptions after each test scenario, ensuring test isolation by cleaning up any messages generated during the test. This complements theBeforehook purge for robust message queue cleanup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
from
b869c28 to
16f03da
Compare
sagzy
force-pushed
the
remove-account-site-queries-in-dispatchers
branch
from
16f03da to
b6f52ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app.ts`:
- Around line 257-288: The result error branch currently compares string
literals from getError(result); change the Result error shape to discriminated
error objects (e.g., { type: 'site-not-found'; host: string } | { type:
'account-not-found'; host: string } | { type: 'multiple-users-for-site'; host:
string }) returned by hostDataLoader.loadDataForHost, then update the error
handling to switch on error.type (use getError(result) as the error object) and
reference the contextual fields (e.g., error.host) when calling
ctx.data.logger.error; also ensure exhaustiveCheck(error) still receives the
error object type so the compiler can verify exhaustiveness and leave the
success branch using getValue(result) to set ctx.data.hostSite and
ctx.data.hostAccount unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/step_definitions/index.jssrc/app.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- features/step_definitions/index.js
🧰 Additional context used
📓 Path-based instructions (1)
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/app.ts
🧠 Learnings (1)
📓 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: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders, so additional SQL normalization is not needed for Sentry fingerprinting.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1136
File: src/instrumentation.ts:36-48
Timestamp: 2025-07-31T16:35:35.813Z
Learning: In the TryGhost/ActivityPub repository, the SQL queries captured by extractQueryInfoFromError from Knex's query-error event are already normalized with placeholders (?), so additional SQL normalization is not needed for Sentry fingerprinting. The actual parameter values are stored separately in the bindings array.
Learnt from: mike182uk
Repo: TryGhost/ActivityPub PR: 1223
File: src/http/api/bluesky.controller.ts:0-0
Timestamp: 2025-08-21T13:32:58.582Z
Learning: In the Ghost ActivityPub codebase, `ctx.get('account')` in controller methods should always return an account because the middleware guarantees this. Guards for missing account are unnecessary in this system.
🧬 Code graph analysis (1)
src/app.ts (4)
src/site/site.service.ts (1)
Site(9-14)src/configuration/container.ts (1)
container(3-6)src/http/host-data-context-loader.ts (1)
HostDataContextLoader(9-96)src/core/result.ts (4)
isError(22-24)getError(30-32)exhaustiveCheck(52-54)getValue(26-28)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (5)
src/app.ts (5)
53-53: LGTM!The Result helper imports align with the coding guidelines requiring use of
isError,getError,getValue, andexhaustiveCheckfor handling Result types.
172-177: LGTM!The
ContextDatatype extension with nullablehostSiteandhostAccountfields is well-designed, allowing flexibility for contexts where host data isn't available (e.g., during queue startup).
211-218: LGTM!Passing
hostSite: nullandhostAccount: nullduring queue startup is correct since there's no specific host context available at this point.
799-816: LGTM!This middleware correctly propagates host data from the Hono context into the Fedify context. The placement after
createHostDataContextMiddlewareensures thatsiteandaccountare available in the Hono context variables.
929-944: LGTM!The federation context mapping correctly includes
hostSiteandhostAccount, ensuring consistency with theContextDatatype and the middleware implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ctx.data.logger = globalLogging; | ||
| } | ||
|
|
||
| const hostDataLoader = container.resolve<HostDataContextLoader>( |
There was a problem hiding this comment.
@mike182uk calling out one change since last time you looked here:
I've removed if (!ctx.data.hostSite || !ctx.data.hostAccount) {...} wrapper here for two reasons:
ensureCorrectContextis only called for dispatchers that don't use the request context, so we are not making a duplicate database query- Cucumber test was failing, with ctx.site / ctx.account set to another account than the one expected. I first thought that it might be a test setup issue, but I couldn't validate that there wasn't a bug locally as we have a single site. If we end up caching the wrong site data, that's not great - so I preferred to play it safe and have the database query run here systematically
|
Had a chat with @mike182uk - we both don't feel super confident about this changeset as it touches the Fedify Context. We're going to trade a bit of performance gains for more safety and make the change in the dispatcher code directly. Closing this PR and will open another one |
ref https://linear.app/ghost/issue/BER-3012
ensureCorrectContextto make sure the host data is available to all dispatchers codekeypairDispatcher, for which we still do an additional database query. The reason is that we don't want to expose the public/private key pair to the Account entity, as there is a risk we might inadvertently expose the private key