Parallelized mention account lookups in PostService#1622
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe implementation of getMentionedAccounts in src/post/post.service.ts was changed to collect FedifyMention tags into a list and resolve referenced accounts in parallel via Promise.allSettled. Per-tag resolution failures are caught and logged at debug level and skipped; only fulfilled results are converted to Mention objects that retain the tag-provided name and href. Logging for mention lookup failures was lowered from info to debug. An integration test (src/post/post.service.integration.test.ts) was added asserting getByApId returns a post containing only successfully resolved mentions when some lookups fail. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
rmgpinto
force-pushed
the
parallelize-mention-lookups
branch
from
4282632 to
32a656f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/post/post.service.ts (1)
138-150: Consider logging failed account lookups for debugging consistency.The
getMentionsFromContentmethod (lines 286-299) logs when account lookups fail, but this parallelized version silently discards failures. Adding logging would maintain consistency and help debug issues with mention resolution.♻️ Suggested addition for logging
const results = await Promise.allSettled( mentionTags.map(async (tag) => { const accountResult = await this.accountService.ensureByApId( tag.href, ); - if (isError(accountResult)) return null; + if (isError(accountResult)) { + this.logger.info( + `Failed to lookup account for mention: ${tag.name}, error: ${getError(accountResult)}`, + ); + return null; + } return { name: tag.name, href: tag.href, account: getValue(accountResult), }; }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/post/post.service.ts` around lines 138 - 150, The parallel mention resolution silently drops failures; update the Promise.allSettled handling in getMentionsFromContent's parallel path (the mentionTags -> accountService.ensureByApId flow) to log failed lookups: after awaiting Promise.allSettled(results), iterate the settled results and for any entry that is "rejected" or a fulfilled result where isError(accountResult) would have returned null, call the service logger (e.g., this.logger.warn or this.logger.error) with the tag.href and the rejection reason or error details so failures are visible for debugging while keeping the existing behavior of returning the filtered successes via getValue.
🤖 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/post/post.service.ts`:
- Around line 138-150: The parallel mention resolution silently drops failures;
update the Promise.allSettled handling in getMentionsFromContent's parallel path
(the mentionTags -> accountService.ensureByApId flow) to log failed lookups:
after awaiting Promise.allSettled(results), iterate the settled results and for
any entry that is "rejected" or a fulfilled result where isError(accountResult)
would have returned null, call the service logger (e.g., this.logger.warn or
this.logger.error) with the tag.href and the rejection reason or error details
so failures are visible for debugging while keeping the existing behavior of
returning the filtered successes via getValue.
src/post/post.service.ts
Outdated
| ); | ||
|
|
||
| return mentions; | ||
| return results |
There was a problem hiding this comment.
This felt a little verbose, I wonder if we could simplify to just:
const mentions = await Promise.all(
mentionTags.map(async ({href, name}): Promise<Mention | null> => {
try {
const accountResult = await this.accountService.ensureByApId(href);
if (isError(accountResult)){
return null;
}
return {name, href, account: getValue(accountResult)};
} catch (err) {
this.logger.debug(
'Failed to look up account for mention {href}',
{ href: href.toString(), error: err },
);
return null;
}
}),
);
return mentions.filter((m): m is Mention => m !== null);This would also let us log a failure instead of silently dropping (useful for debugging?)
There was a problem hiding this comment.
Also could be good to add a test for the best-effort behaviour change (if a lookup fails, others are still returned)
There was a problem hiding this comment.
I'm assuming order doesn't matter? (this could potentially return results out of the order they were passed in)
rmgpinto
force-pushed
the
parallelize-mention-lookups
branch
from
32a656f to
947f889
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/post/post.service.ts (1)
144-147: Consider usingdebuglevel for failed mention lookups.Failed lookups for unknown external accounts may be common and expected behavior. Using
infolevel could create unnecessary log noise in production. Consider downgrading todebuglevel since these logs are primarily useful for troubleshooting.💡 Suggested change
if (isError(accountResult)) { - this.logger.info( + this.logger.debug( `Failed to lookup account for mention: ${tag.name}, error: ${getError(accountResult)}`, ); return null; }And similarly at line 155:
} catch (err) { - this.logger.info( + this.logger.debug( `Failed to lookup account for mention: ${tag.name}, error: ${err}`, ); return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/post/post.service.ts` around lines 144 - 147, The log for failed external account lookup in post.service.ts (the this.logger.info call that logs `Failed to lookup account for mention: ${tag.name}, error: ${getError(accountResult)}`) should be lowered to this.logger.debug because failed mention lookups are expected; update both occurrences (the info at the lookup failure and the similar one at the later mention handling around line 155) to use debug so you retain the same message and getError(accountResult) details but avoid noisy production info logs.
🤖 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/post/post.service.ts`:
- Around line 144-147: The log for failed external account lookup in
post.service.ts (the this.logger.info call that logs `Failed to lookup account
for mention: ${tag.name}, error: ${getError(accountResult)}`) should be lowered
to this.logger.debug because failed mention lookups are expected; update both
occurrences (the info at the lookup failure and the similar one at the later
mention handling around line 155) to use debug so you retain the same message
and getError(accountResult) details but avoid noisy production info logs.
952f0a9 to
825899c
Compare
|
@mike182uk I've made the changes you suggested, can you re-review? Thanks |
src/post/post.service.ts
Outdated
| this.logger.debug( | ||
| `Failed to lookup account for mention: ${tag.name}, error: ${getError(accountResult)}`, | ||
| ); |
There was a problem hiding this comment.
We could probably just rethrow this error and handle everything in the catch?
throw getError(accountResult)
rmgpinto
force-pushed
the
parallelize-mention-lookups
branch
from
825899c to
740c8dc
Compare
2 participants
ref https://linear.app/ghost/issue/BER-3241