Add lightweight cached NodeInfo metrics#1819
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces NodeInfoService: a KV-cached service that aggregates lastActivityAt, localPosts, and localComments from outboxes, likes, and follows via a single SQL query and stores results with a 30-minute TTL. The nodeInfoDispatcher is converted to a DI factory that resolves site/account per request and uses NodeInfoService to populate NodeInfo responses with usage and metadata. Like/Post controllers now dispatch ActivityPub sends in parallel with Promise.all and log warnings on send failures. Unit and integration tests were added/updated; post repository removeLikes now returns an object with removedLikesCount. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
JohnONolan
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
from
96be7c5 to
c826986
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/configuration/registrations.ts (1)
1-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix import ordering to satisfy lint.
This file currently fails the import organization rule from CI (
assist/source/organizeImports). Please reorder imports to unblock the lint check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/configuration/registrations.ts` around lines 1 - 25, Reorder the import statements to satisfy the project's organizeImports rule: group external module imports first (e.g., createFederation, KvStore, RedisKvStore, PubSub, getLogger, Awilix helpers, Redis, Knex) sorted alphabetically within that group, then place local project imports (those starting with '`@/`') afterwards, also alphabetized (e.g., KnexAccountRepository, AccountService, CreateHandler, DeleteHandler, FollowHandler, UpdateHandler, FedifyContextFactory, FediverseBridge, FollowersService, NodeInfoEventService, NodeInfoService); keep type-only imports with their modules and preserve named import formatting (asValue/asFunction/asClass/aliasTo) so the linter recognizes the correct order.
🧹 Nitpick comments (2)
src/dispatchers.unit.test.ts (1)
485-538: ⚡ Quick winAdd a guard-path test for missing site/account context.
The new dispatcher has an explicit throw path when context is incomplete, but this block currently validates only the success path. Add a focused test for the error branch to prevent regressions.
Suggested test addition
describe('nodeInfoDispatcher', () => { it('returns the node info', async () => { // existing test... }); + + it('throws when site/account context is missing', async () => { + const nodeInfoService = { + getNodeInfo: vi.fn(), + }; + const dispatcher = nodeInfoDispatcher( + nodeInfoService as unknown as NodeInfoService, + ); + + await expect( + dispatcher({ + data: { + logger: { error: vi.fn() }, + }, + } as unknown as FedifyRequestContext), + ).rejects.toThrow('NodeInfo requested without site context'); + expect(nodeInfoService.getNodeInfo).not.toHaveBeenCalled(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dispatchers.unit.test.ts` around lines 485 - 538, Add a unit test that exercises the dispatcher’s error/guard path when the request context is missing site or account: call nodeInfoDispatcher (constructed with a mocked NodeInfoService where getNodeInfo should not be called) with a FedifyRequestContext missing site and/or account, assert that the dispatcher throws the expected error, and verify the logger.error was invoked and getNodeInfo was not called; this ensures nodeInfoDispatcher’s guard path is covered and prevents regressions.src/app.ts (1)
213-218: ⚡ Quick winUse
interfaceforContextDataobject shape.Please switch
ContextDatafrom a type alias to an interface to match the TypeScript guideline.As per coding guidelines, `**/*.{ts,tsx}`: Prefer `interface` for defining object shapes in TypeScript.Suggested change
-export type ContextData = { +export interface ContextData { globaldb: KvStore; logger: Logger; site?: Site; account?: Account; -}; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.ts` around lines 213 - 218, Change the ContextData type alias to an interface so the object shape follows the repo guideline: replace the export type ContextData = { ... } with export interface ContextData { globaldb: KvStore; logger: Logger; site?: Site; account?: Account; } ensuring the same property names and optional modifiers are preserved; update any imports/usages if your editor flags them, but no runtime behavior should change for references to ContextData.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/activitypub/nodeinfo.service.ts`:
- Around line 265-266: isCachedNodeInfo() currently only verifies that
nodeInfo.software.homepage and nodeInfo.software.repository are strings, but
deserializeNodeInfo() constructs new URL(...) and will throw on malformed cached
values; update isCachedNodeInfo() to perform stricter URL validation (e.g.,
attempt to construct new URL(...) or use a robust URL validation routine) for
nodeInfo.software.homepage and nodeInfo.software.repository (and the analogous
checks around lines 276–283) so that deserializeNodeInfo() never receives
malformed URL strings from cache and won’t throw during deserialization.
In `@src/http/api/like.controller.unit.test.ts`:
- Around line 3-8: The import block in like.controller.unit.test.ts is failing
the import organizer; reorder and/or run your editor's "organize imports" so the
specifiers from '`@fedify/fedify`' are consistently grouped and alphabetized
(e.g., ensure "type" imports and value imports are ordered/preserved) —
specifically adjust the import that currently lists Federation, Like, Object as
APObject, and Undo so they follow the project's import-sort rules (run
assist/source/organizeImports or sort to: APObject, Federation, Like, Undo or
use your IDE organizer) to unblock the lint job.
In `@src/http/api/post.controller.unit.test.ts`:
- Around line 3-8: Reorder and clean up the import block so it meets the
project's import-organization rule: combine and alphabetize imported symbols,
use type-only imports for type aliases, and remove any unused imports;
specifically update the import that currently brings in Announce, Federation,
Object as APObject, and Undo from '`@fedify/fedify`' so that type-only items
(e.g., Federation, Object/APObject) are imported with the TypeScript "type"
modifier as appropriate, the identifiers are sorted consistently (alphabetical),
and any unused names are removed to satisfy assist/source/organizeImports.
---
Outside diff comments:
In `@src/configuration/registrations.ts`:
- Around line 1-25: Reorder the import statements to satisfy the project's
organizeImports rule: group external module imports first (e.g.,
createFederation, KvStore, RedisKvStore, PubSub, getLogger, Awilix helpers,
Redis, Knex) sorted alphabetically within that group, then place local project
imports (those starting with '`@/`') afterwards, also alphabetized (e.g.,
KnexAccountRepository, AccountService, CreateHandler, DeleteHandler,
FollowHandler, UpdateHandler, FedifyContextFactory, FediverseBridge,
FollowersService, NodeInfoEventService, NodeInfoService); keep type-only imports
with their modules and preserve named import formatting
(asValue/asFunction/asClass/aliasTo) so the linter recognizes the correct order.
---
Nitpick comments:
In `@src/app.ts`:
- Around line 213-218: Change the ContextData type alias to an interface so the
object shape follows the repo guideline: replace the export type ContextData = {
... } with export interface ContextData { globaldb: KvStore; logger: Logger;
site?: Site; account?: Account; } ensuring the same property names and optional
modifiers are preserved; update any imports/usages if your editor flags them,
but no runtime behavior should change for references to ContextData.
In `@src/dispatchers.unit.test.ts`:
- Around line 485-538: Add a unit test that exercises the dispatcher’s
error/guard path when the request context is missing site or account: call
nodeInfoDispatcher (constructed with a mocked NodeInfoService where getNodeInfo
should not be called) with a FedifyRequestContext missing site and/or account,
assert that the dispatcher throws the expected error, and verify the
logger.error was invoked and getNodeInfo was not called; this ensures
nodeInfoDispatcher’s guard path is covered and prevents regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebc2de76-e67e-4ef7-ad55-2bf298048bb9
📒 Files selected for processing (19)
src/activitypub/nodeinfo-event.service.tssrc/activitypub/nodeinfo-event.service.unit.test.tssrc/activitypub/nodeinfo.service.integration.test.tssrc/activitypub/nodeinfo.service.tssrc/activitypub/nodeinfo.service.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/http/api/follow.controller.tssrc/http/api/follow.controller.unit.test.tssrc/http/api/like.controller.tssrc/http/api/like.controller.unit.test.tssrc/http/api/post.controller.tssrc/http/api/post.controller.unit.test.tssrc/post/post-unliked.event.tssrc/post/post-unliked.event.unit.test.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
JohnONolan
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
4 times, most recently
from
6117078 to
05e7ced
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/api/post.controller.ts (1)
772-788:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMarking activity before federation success can produce false positives.
markAccountActiveruns even whenapCtx.sendActivity(...)later fails, because both sends are not awaited. This violates the “successful wire-only path” intent and can overcount active users.Suggested fix
- if (attributionActor) { - apCtx.sendActivity( + if (attributionActor) { + await apCtx.sendActivity( { username: account.username }, attributionActor, undo, { preferSharedInbox: true, }, ); } - apCtx.sendActivity({ username: account.username }, 'followers', undo, { + await apCtx.sendActivity({ username: account.username }, 'followers', undo, { preferSharedInbox: true, }); if (shouldMarkActivity) { await this.markAccountActive(ctx, account.id); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/api/post.controller.ts` around lines 772 - 788, The code marks an account active (markAccountActive) before the federation sends (apCtx.sendActivity) have completed, which can produce false positives; change the flow so both sendActivity calls are awaited and only call await this.markAccountActive(ctx, account.id) after those awaits succeed (use await for each sendActivity or Promise.all on the two sendActivity promises), and ensure errors from sendActivity are caught and handled (do not call markAccountActive if sendActivity throws/fails).
🧹 Nitpick comments (1)
src/activitypub/nodeinfo.service.ts (1)
10-53: ⚡ Quick winPrefer
interfacefor these object-shape declarations.These object-shape aliases should use
interfaceto match project conventions.♻️ Proposed refactor
-type NodeInfoCounts = { +interface NodeInfoCounts { localPosts: number; localComments: number; -}; +} -type CachedNodeInfo = { +interface CachedNodeInfo { software: { name: 'ghost'; version: { major: number; minor: number; patch: number }; homepage: string; repository: string; @@ metadata: { nodeName: string; nodeDescription?: string; nodeIcon?: string; nodeBanner?: string; private: false; postFormats: ['text/html']; }; -}; +} -type NodeInfo = Omit<CachedNodeInfo, 'software'> & { +interface NodeInfo extends Omit<CachedNodeInfo, 'software'> { software: Omit<CachedNodeInfo['software'], 'homepage' | 'repository'> & { homepage: URL; repository: URL; }; -}; +}As per coding guidelines,
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/activitypub/nodeinfo.service.ts` around lines 10 - 53, Replace the object-shape type aliases with interfaces to follow project conventions: change the type declarations for NodeInfoCounts, CachedNodeInfo, and NodeInfo into interface declarations while preserving the exact property shapes and unions (including nested software, protocols, services, usage, metadata and the Omit-based transformation for NodeInfo); ensure NodeInfo still represents the same structure by rewriting the Omit<CachedNodeInfo,'software'> pattern to use interface inheritance or declaration merging so software in NodeInfo becomes an interface with homepage and repository typed as URL while keeping homepage/repository removed from the base CachedNodeInfo software shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/api/like.controller.ts`:
- Around line 157-163: The code currently calls apCtx.sendActivity({...},
'followers', like, ...) fire-and-forget and then unconditionally calls
markAccountActive when shouldMarkActivity is true; change both occurrences (the
one around apCtx.sendActivity/markAccountActive and the one later at the other
location) to await the sendActivity call and only call
this.markAccountActive(ctx, account.id) if sendActivity resolves successfully;
wrap the await apCtx.sendActivity(...) in a try/catch so send failures are
caught (log or handle the error) and do not trigger markAccountActive when the
send rejects.
---
Outside diff comments:
In `@src/http/api/post.controller.ts`:
- Around line 772-788: The code marks an account active (markAccountActive)
before the federation sends (apCtx.sendActivity) have completed, which can
produce false positives; change the flow so both sendActivity calls are awaited
and only call await this.markAccountActive(ctx, account.id) after those awaits
succeed (use await for each sendActivity or Promise.all on the two sendActivity
promises), and ensure errors from sendActivity are caught and handled (do not
call markAccountActive if sendActivity throws/fails).
---
Nitpick comments:
In `@src/activitypub/nodeinfo.service.ts`:
- Around line 10-53: Replace the object-shape type aliases with interfaces to
follow project conventions: change the type declarations for NodeInfoCounts,
CachedNodeInfo, and NodeInfo into interface declarations while preserving the
exact property shapes and unions (including nested software, protocols,
services, usage, metadata and the Omit-based transformation for NodeInfo);
ensure NodeInfo still represents the same structure by rewriting the
Omit<CachedNodeInfo,'software'> pattern to use interface inheritance or
declaration merging so software in NodeInfo becomes an interface with homepage
and repository typed as URL while keeping homepage/repository removed from the
base CachedNodeInfo software shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b5dce98-f097-4267-8978-4fc08aa0271f
📒 Files selected for processing (19)
src/activitypub/nodeinfo-event.service.tssrc/activitypub/nodeinfo-event.service.unit.test.tssrc/activitypub/nodeinfo.service.integration.test.tssrc/activitypub/nodeinfo.service.tssrc/activitypub/nodeinfo.service.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/http/api/follow.controller.tssrc/http/api/follow.controller.unit.test.tssrc/http/api/like.controller.tssrc/http/api/like.controller.unit.test.tssrc/http/api/post.controller.tssrc/http/api/post.controller.unit.test.tssrc/post/post-unliked.event.tssrc/post/post-unliked.event.unit.test.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
JohnONolan
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
2 times, most recently
from
eeb1009 to
a7f1869
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/post/post-unliked.event.ts`:
- Around line 32-41: The current PostUnlikedEvent.fromJSON validates only typeof
for postId and accountId allowing NaN/Infinity; update the checks in fromJSON to
ensure both data.postId and data.accountId are finite, safe integers (e.g., use
Number.isFinite and Number.isSafeInteger) and throw descriptive errors if they
fail, before calling new PostUnlikedEvent(postId, accountId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03b52e98-fd79-4aa7-820d-11a727b4a872
📒 Files selected for processing (19)
src/activitypub/nodeinfo-event.service.tssrc/activitypub/nodeinfo-event.service.unit.test.tssrc/activitypub/nodeinfo.service.integration.test.tssrc/activitypub/nodeinfo.service.tssrc/activitypub/nodeinfo.service.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/http/api/follow.controller.tssrc/http/api/follow.controller.unit.test.tssrc/http/api/like.controller.tssrc/http/api/like.controller.unit.test.tssrc/http/api/post.controller.tssrc/http/api/post.controller.unit.test.tssrc/post/post-unliked.event.tssrc/post/post-unliked.event.unit.test.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
JohnONolan
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
5 times, most recently
from
adb7d66 to
bdaa025
Compare
JohnONolan
added a commit
that referenced
this pull request
Jun 1, 2026
Replace write-path NodeInfo activity tracking with a cached stats query so KV remains a cache instead of durable state. Resolve host data inside the NodeInfo dispatcher and remove dispatcher-specific site/account fields from shared Fedify context.
JohnONolan
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
from
bdaa025 to
8d493ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/activitypub/nodeinfo.service.ts (2)
52-71: ⚡ Quick winRemove the unnecessary “index may be missing” risk
FORCE INDEX (idx_outboxes_account_id_outbox_type_published_at_desc)is present inmigrate/migrations/000055_recreate-outboxes-table.up.sql(KEY idx_outboxes_account_id_outbox_type_published_at_desc (account_id, outbox_type, published_at DESC)), so this query won’t currently fail with MySQL error 1176 due to a missing index. TheFORCE INDEXname is still brittle though—keep that index name stable (or update this query together with migrations).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/activitypub/nodeinfo.service.ts` around lines 52 - 71, The SQL in the this.db.raw call in nodeinfo.service.ts includes brittle FORCE INDEX (idx_outboxes_account_id_outbox_type_published_at_desc) hints; remove all FORCE INDEX (...) occurrences inside that raw query (both the subselects for published_at and the COUNT(*) subqueries for local_posts/local_comments) so the optimizer can pick the index and the code won't break if the index name changes—leave the rest of the query unchanged and keep the surrounding this.db.raw(...) call and the alias last_activity_at/local_posts/local_comments intact.
51-94: 🏗️ Heavy liftMove the NodeInfo stats SQL out of
NodeInfoServiceinto a repository/view
src/activitypub/nodeinfo.service.ts(queryStats) callsthis.db.raw(...)directly to computelast_activity_at,local_posts, andlocal_comments. Move this aggregation into a dedicated read-only view or repository method soNodeInfoServicestays an abstraction layer (orchestrate + serialize/cache) while views handle complex DB reads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/activitypub/nodeinfo.service.ts` around lines 51 - 94, queryStats in NodeInfoService directly runs raw SQL to compute last_activity_at, local_posts, and local_comments; extract this DB logic into a dedicated repository or read-only view. Create a new NodeInfoRepository (or DB view) with a method like getStats(accountId: number) that encapsulates the raw SQL (or SELECT from the new view) and returns NodeInfoStatsRow, then update NodeInfoService.queryStats to call NodeInfoRepository.getStats(accountId) and keep the existing parsing via parseLastActivityAt and conversion to Number for localPosts/localComments; reference the symbols queryStats, NodeInfoService, parseLastActivityAt, OutboxType and the NodeInfoStatsRow shape when moving the SQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dispatchers.ts`:
- Around line 1341-1384: The nodeInfoDispatcher function factory should be
refactored into a class-based dispatcher: create a NodeInfoDispatcher class that
takes hostDataContextLoader: HostDataContextLoader and nodeInfoService:
NodeInfoService in its constructor and exposes an async dispatch(ctx:
FedifyRequestContext) method that contains the existing logic (use
hostDataContextLoader.loadDataForHost, isError/getError/getValue, and
nodeInfoService.getData). Replace the plain throw new Error('NodeInfo requested
without site context') with returning or propagating a Result-style error object
(include context like host and the original error via getError(hostData)) per
your Result/error convention instead of throwing, and update any call sites that
consumed nodeInfoDispatcher to instantiate NodeInfoDispatcher and call dispatch.
---
Nitpick comments:
In `@src/activitypub/nodeinfo.service.ts`:
- Around line 52-71: The SQL in the this.db.raw call in nodeinfo.service.ts
includes brittle FORCE INDEX
(idx_outboxes_account_id_outbox_type_published_at_desc) hints; remove all FORCE
INDEX (...) occurrences inside that raw query (both the subselects for
published_at and the COUNT(*) subqueries for local_posts/local_comments) so the
optimizer can pick the index and the code won't break if the index name
changes—leave the rest of the query unchanged and keep the surrounding
this.db.raw(...) call and the alias last_activity_at/local_posts/local_comments
intact.
- Around line 51-94: queryStats in NodeInfoService directly runs raw SQL to
compute last_activity_at, local_posts, and local_comments; extract this DB logic
into a dedicated repository or read-only view. Create a new NodeInfoRepository
(or DB view) with a method like getStats(accountId: number) that encapsulates
the raw SQL (or SELECT from the new view) and returns NodeInfoStatsRow, then
update NodeInfoService.queryStats to call NodeInfoRepository.getStats(accountId)
and keep the existing parsing via parseLastActivityAt and conversion to Number
for localPosts/localComments; reference the symbols queryStats, NodeInfoService,
parseLastActivityAt, OutboxType and the NodeInfoStatsRow shape when moving the
SQL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b81a41ef-f6df-42cc-a01d-9cbaf5f22d1b
📒 Files selected for processing (13)
src/activitypub/nodeinfo.service.integration.test.tssrc/activitypub/nodeinfo.service.tssrc/activitypub/nodeinfo.service.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/http/api/follow.controller.unit.test.tssrc/http/api/like.controller.tssrc/http/api/like.controller.unit.test.tssrc/http/api/post.controller.tssrc/http/api/post.controller.unit.test.tssrc/post/post.repository.knex.ts
✅ Files skipped from review due to trivial changes (1)
- src/http/api/follow.controller.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activitypub/nodeinfo.service.integration.test.ts
Comment on lines
+1341
to
+1384
| export const nodeInfoDispatcher = | ||
| ( | ||
| hostDataContextLoader: HostDataContextLoader, | ||
| nodeInfoService: NodeInfoService, | ||
| ) => | ||
| async (ctx: FedifyRequestContext) => { | ||
| const hostData = await hostDataContextLoader.loadDataForHost(ctx.host); | ||
|
|
||
| if (isError(hostData)) { | ||
| ctx.data.logger.error('NodeInfo: failed to resolve host', { | ||
| host: ctx.host, | ||
| error: getError(hostData), | ||
| }); | ||
| throw new Error('NodeInfo requested without site context'); | ||
| } | ||
|
|
||
| const { site, account } = getValue(hostData); | ||
| const data = await nodeInfoService.getData(site, account); | ||
|
|
||
| return { | ||
| software: { | ||
| name: 'ghost' as const, | ||
| version: { major: 0, minor: 1, patch: 0 }, | ||
| homepage: new URL('https://ghost.org/'), | ||
| repository: new URL('https://github.com/TryGhost/Ghost'), | ||
| }, | ||
| protocols: ['activitypub'] as Protocol[], | ||
| services: { | ||
| inbound: [], | ||
| outbound: [], | ||
| }, | ||
| openRegistrations: false, | ||
| usage: { | ||
| users: { | ||
| total: 1, | ||
| activeMonth: isActiveWithin(data.lastActivityAt, 30), | ||
| activeHalfyear: isActiveWithin(data.lastActivityAt, 180), | ||
| }, | ||
| localPosts: data.localPosts, | ||
| localComments: data.localComments, | ||
| }, | ||
| localPosts: 0, | ||
| localComments: 0, | ||
| }, | ||
| metadata: getNodeInfoMetadata(account), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Refactor to class-based architecture.
The nodeInfoDispatcher uses a function factory pattern, which violates the class-based architecture guideline. Consider refactoring to a class with a dispatch method that accepts FedifyRequestContext and returns the NodeInfo response. The class constructor should accept hostDataContextLoader and nodeInfoService as DI parameters.
Additionally, line 1354 throws a plain Error instead of returning a Result type with context, which also violates the guideline for error handling.
As per coding guidelines: "Use class-based architecture with dependency injection instead of function factories" and "Use error objects with context in Result types instead of string literal errors".
Example class-based refactor
export class NodeInfoDispatcher {
constructor(
private readonly hostDataContextLoader: HostDataContextLoader,
private readonly nodeInfoService: NodeInfoService,
) {}
async dispatch(ctx: FedifyRequestContext) {
const hostData = await this.hostDataContextLoader.loadDataForHost(ctx.host);
if (isError(hostData)) {
ctx.data.logger.error('NodeInfo: failed to resolve host', {
host: ctx.host,
error: getError(hostData),
});
throw new Error('NodeInfo requested without site context');
}
const { site, account } = getValue(hostData);
const data = await this.nodeInfoService.getData(site, account);
return {
software: {
name: 'ghost' as const,
version: { major: 0, minor: 1, patch: 0 },
homepage: new URL('https://ghost.org/'),
repository: new URL('https://github.com/TryGhost/Ghost'),
},
protocols: ['activitypub'] as Protocol[],
services: {
inbound: [],
outbound: [],
},
openRegistrations: false,
usage: {
users: {
total: 1,
activeMonth: isActiveWithin(data.lastActivityAt, 30),
activeHalfyear: isActiveWithin(data.lastActivityAt, 180),
},
localPosts: data.localPosts,
localComments: data.localComments,
},
metadata: getNodeInfoMetadata(account),
};
}
}Then update registrations.ts:
-container.register(
- 'nodeInfoDispatcher',
- asFunction(nodeInfoDispatcher).singleton(),
-);
+container.register(
+ 'nodeInfoDispatcher',
+ asClass(NodeInfoDispatcher).singleton(),
+);And update app.ts:
-const dispatcher =
- container.resolve<ReturnType<typeof nodeInfoDispatcher>>(
- 'nodeInfoDispatcher',
- );
-return dispatcher(ctx);
+const dispatcher =
+ container.resolve<NodeInfoDispatcher>('nodeInfoDispatcher');
+return dispatcher.dispatch(ctx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dispatchers.ts` around lines 1341 - 1384, The nodeInfoDispatcher function
factory should be refactored into a class-based dispatcher: create a
NodeInfoDispatcher class that takes hostDataContextLoader: HostDataContextLoader
and nodeInfoService: NodeInfoService in its constructor and exposes an async
dispatch(ctx: FedifyRequestContext) method that contains the existing logic (use
hostDataContextLoader.loadDataForHost, isError/getError/getValue, and
nodeInfoService.getData). Replace the plain throw new Error('NodeInfo requested
without site context') with returning or propagating a Result-style error object
(include context like host and the original error via getError(hostData)) per
your Result/error convention instead of throwing, and update any call sites that
consumed nodeInfoDispatcher to instantiate NodeInfoDispatcher and call dispatch.
JohnONolan
added a commit
that referenced
this pull request
Jun 1, 2026
Treat NodeInfo cache reads and writes as best-effort because the KV entry is cache-only, and document the retained-row activity approximation. Broaden tests so every retained activity source can drive lastActivityAt and active-user windows cover exact boundaries. Simplify unlike persistence by removing an unused pre-select from removeLikes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activitypub/nodeinfo.service.ts (1)
137-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard cached timestamp deserialization against invalid date strings.
deserialize()can returnInvalid Datefor malformed cachedlastActivityAtvalues; that leaks a non-null but unusable date into callers. Parse throughparseLastActivityAt()here so malformed cache data degrades tonull.Proposed fix
private deserialize(data: CachedNodeInfoData): NodeInfoData { return { - lastActivityAt: - data.lastActivityAt === null - ? null - : new Date(data.lastActivityAt), + lastActivityAt: this.parseLastActivityAt(data.lastActivityAt), localPosts: data.localPosts, localComments: data.localComments, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/activitypub/nodeinfo.service.ts` around lines 137 - 143, deserialize() currently constructs lastActivityAt with new Date(data.lastActivityAt) which can produce an "Invalid Date" for malformed cache strings; change it to call the existing parseLastActivityAt(data.lastActivityAt) so invalid or malformed values degrade to null. Update the deserialize function (referencing deserialize, CachedNodeInfoData, NodeInfoData, and parseLastActivityAt) to pass data.lastActivityAt through parseLastActivityAt and return that result for lastActivityAt instead of instantiating Date directly.
🧹 Nitpick comments (1)
src/post/post.repository.knex.ts (1)
684-695: ⚡ Quick winUse a named
interfacefor theremoveLikesreturn shape.Please replace the inline object type with an interface to align with project TS conventions.
Suggested change
+interface RemoveLikesResult { + removedLikesCount: number; +} + private async removeLikes( post: Post, accountIds: number[], transaction: Knex.Transaction, - ): Promise<{ removedLikesCount: number }> { + ): Promise<RemoveLikesResult> { const removedLikesCount = await transaction('likes') .where({ post_id: post.id, }) .whereIn('account_id', accountIds) .del(); return { removedLikesCount, }; }As per coding guidelines, "
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/post/post.repository.knex.ts` around lines 684 - 695, The return type for the function that deletes likes currently uses an inline object type Promise<{ removedLikesCount: number }>; define a named interface (e.g., RemovedLikesResult) with removedLikesCount: number and replace the inline type with Promise<RemovedLikesResult> in the function signature (locate the function around the transaction('likes') deletion block, e.g., removeLikes or the method containing removedLikesCount) and export the interface if other modules need it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/activitypub/nodeinfo.service.ts`:
- Around line 137-143: deserialize() currently constructs lastActivityAt with
new Date(data.lastActivityAt) which can produce an "Invalid Date" for malformed
cache strings; change it to call the existing
parseLastActivityAt(data.lastActivityAt) so invalid or malformed values degrade
to null. Update the deserialize function (referencing deserialize,
CachedNodeInfoData, NodeInfoData, and parseLastActivityAt) to pass
data.lastActivityAt through parseLastActivityAt and return that result for
lastActivityAt instead of instantiating Date directly.
---
Nitpick comments:
In `@src/post/post.repository.knex.ts`:
- Around line 684-695: The return type for the function that deletes likes
currently uses an inline object type Promise<{ removedLikesCount: number }>;
define a named interface (e.g., RemovedLikesResult) with removedLikesCount:
number and replace the inline type with Promise<RemovedLikesResult> in the
function signature (locate the function around the transaction('likes') deletion
block, e.g., removeLikes or the method containing removedLikesCount) and export
the interface if other modules need it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 133d5d2e-19af-41bb-9dd7-77cb313ab7f8
📒 Files selected for processing (5)
src/activitypub/nodeinfo.service.integration.test.tssrc/activitypub/nodeinfo.service.tssrc/activitypub/nodeinfo.service.unit.test.tssrc/dispatchers.unit.test.tssrc/post/post.repository.knex.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dispatchers.unit.test.ts
mike182uk
pushed a commit
that referenced
this pull request
Jun 8, 2026
Replace write-path NodeInfo activity tracking with a cached stats query so KV remains a cache instead of durable state. Resolve host data inside the NodeInfo dispatcher and remove dispatcher-specific site/account fields from shared Fedify context.
mike182uk
pushed a commit
that referenced
this pull request
Jun 8, 2026
Treat NodeInfo cache reads and writes as best-effort because the KV entry is cache-only, and document the retained-row activity approximation. Broaden tests so every retained activity source can drive lastActivityAt and active-user windows cover exact boundaries. Simplify unlike persistence by removing an unused pre-select from removeLikes.
mike182uk
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
from
205291c to
be74d87
Compare
Add a NodeInfo service that uses KvStore-backed response and count caches so normal NodeInfo reads stay cheap while localPosts and localComments come from indexed outbox counts on cache misses. Track active user windows with a per-site lastActivityAt marker updated from local outgoing activity, including wire-only ActivityPub actions that do not emit repository events. Keep users.total fixed at 1 for Ghost's single-user instance model. Tests cover active windows, cache hit/miss behavior, outbox count computation, event wiring, controller activity marking, and unlike event emission. Full yarn-based verification could not be run in this shell because yarn is unavailable on PATH.
Replace write-path NodeInfo activity tracking with a cached stats query so KV remains a cache instead of durable state. Resolve host data inside the NodeInfo dispatcher and remove dispatcher-specific site/account fields from shared Fedify context.
Treat NodeInfo cache reads and writes as best-effort because the KV entry is cache-only, and document the retained-row activity approximation. Broaden tests so every retained activity source can drive lastActivityAt and active-user windows cover exact boundaries. Simplify unlike persistence by removing an unused pre-select from removeLikes.
mike182uk
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
from
be74d87 to
2a10f11
Compare
mike182uk
force-pushed
the
codex/lightweight-nodeinfo-metrics
branch
from
2a10f11 to
5cef157
Compare
sagzy
added a commit
that referenced
this pull request
Jun 8, 2026
Resolves conflicts from main's "Add lightweight cached NodeInfo metrics" (#1819), which refactored the inline nodeInfoDispatcher in dispatchers.ts into a class-based NodeInfoDispatcher + NodeInfoService. Took main's side for both conflicts (the old standalone function and its test were removed) and dropped the now-unused Protocol import from dispatchers.ts. Migrated main's new NodeInfo files to the Fedify v2 layout this branch upgrades to (2.2.5): - nodeinfo.dispatcher.ts: NodeInfo software.version is a string in v2, not { major, minor, patch }. - nodeinfo.service.ts: use the ambient Temporal namespace instead of importing from @js-temporal/polyfill, so KvStoreSetOptions.ttl type-checks (matches commit 9315b65). - nodeinfo.service.unit.test.ts: KvStore.list() is required in v2, so the FakeKvStore now implements it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Adds a lightweight
NodeInfoServicefor dynamic NodeInfo metrics without adding tables or write-path counter invalidation.usage.users.totalfixed at1nodeinfo:lastActivityAt:{siteId}marker inKvStorelocalPostsandlocalCommentsfrom indexedoutboxescounts on cache miss, then caches the count object for 7 daysActivity tracking
Local outgoing activity now updates the active marker through repository/domain events where those already exist, and through controller-level marking only for successful wire-only paths that do not emit persistence events.
Covered actions include post/reply lifecycle activity, like/unlike, repost/derepost, follow/unfollow, and remote follow initiation.
Testing
Added unit and integration coverage for:
PostUnlikedEventserialization and repository emissionValidation
git diff --checkpassesyarn test:singleoryarn test:typeslocally becauseyarnis not available on PATH in this shell