refactor: optimize hashtag handling and media creation in post transa…#186
refactor: optimize hashtag handling and media creation in post transa…#186YousefAref72 merged 1 commit intodevfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the post creation transaction to optimize hashtag handling and improve database operation efficiency. The key changes focus on reducing the number of database queries during hashtag upserts and batching media/mention creation operations.
Key Changes:
- Optimized hashtag handling by using findMany + createMany instead of individual upserts
- Batched media and mention creation operations using Promise.all
- Added transaction timeout configuration (maxWait: 5000ms, timeout: 10000ms)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const existingHashtags = await tx.hashtag.findMany({ | ||
| where: { tag: { in: hashtags } }, | ||
| select: { id: true, tag: true }, | ||
| }); | ||
|
|
||
| const existingTags = new Set(existingHashtags.map((h) => h.tag)); | ||
| const newTags = hashtags.filter((tag) => !existingTags.has(tag)); | ||
|
|
||
| if (newTags.length > 0) { | ||
| await tx.hashtag.createMany({ | ||
| data: newTags.map((tag) => ({ tag })), | ||
| skipDuplicates: true, | ||
| }); | ||
|
|
||
| const newHashtags = await tx.hashtag.findMany({ | ||
| where: { tag: { in: newTags } }, | ||
| select: { id: true, tag: true }, | ||
| }); | ||
|
|
||
| hashtagRecords = [...existingHashtags, ...newHashtags]; | ||
| } else { | ||
| hashtagRecords = existingHashtags; | ||
| } |
There was a problem hiding this comment.
There's a potential race condition in the hashtag creation logic. Between the createMany operation with skipDuplicates and the subsequent findMany query, another transaction could create the same hashtags, resulting in incomplete hashtagRecords. This could lead to missing hashtag associations on the post. Consider fetching all hashtags (both existing and new) in a single query after the createMany operation, or use a different approach like upserting each hashtag individually within the transaction to ensure atomicity.
| const existingHashtags = await tx.hashtag.findMany({ | |
| where: { tag: { in: hashtags } }, | |
| select: { id: true, tag: true }, | |
| }); | |
| const existingTags = new Set(existingHashtags.map((h) => h.tag)); | |
| const newTags = hashtags.filter((tag) => !existingTags.has(tag)); | |
| if (newTags.length > 0) { | |
| await tx.hashtag.createMany({ | |
| data: newTags.map((tag) => ({ tag })), | |
| skipDuplicates: true, | |
| }); | |
| const newHashtags = await tx.hashtag.findMany({ | |
| where: { tag: { in: newTags } }, | |
| select: { id: true, tag: true }, | |
| }); | |
| hashtagRecords = [...existingHashtags, ...newHashtags]; | |
| } else { | |
| hashtagRecords = existingHashtags; | |
| } | |
| // Try to create any missing hashtags, but always fetch all after | |
| await tx.hashtag.createMany({ | |
| data: hashtags.map((tag) => ({ tag })), | |
| skipDuplicates: true, | |
| }); | |
| hashtagRecords = await tx.hashtag.findMany({ | |
| where: { tag: { in: hashtags } }, | |
| select: { id: true, tag: true }, | |
| }); |
|
|
||
| let parentPostAuthorId: number | undefined = undefined; | ||
|
|
||
| if (enrichedPost.originalPostData && 'postId' in enrichedPost.originalPostData) { |
There was a problem hiding this comment.
The parentPostAuthorId extraction logic may fail if the enrichedPost contains an originalPostData that represents a deleted post. When a parent post is deleted, the enrichIfQuoteOrReply method sets originalPostData to isDeleted: true which doesn't have a userId property. This would leave parentPostAuthorId undefined even when createPostDto.parentId exists, potentially preventing notifications from being sent for valid replies/quotes. Add a check to ensure originalPostData has a userId property before accessing it.
| if (enrichedPost.originalPostData && 'postId' in enrichedPost.originalPostData) { | |
| if ( | |
| enrichedPost.originalPostData && | |
| 'postId' in enrichedPost.originalPostData && | |
| 'userId' in enrichedPost.originalPostData | |
| ) { |
| const { data: [fullPost] } = await this.findPosts({ | ||
| where: { is_deleted: false, id: post.id }, | ||
| userId, | ||
| page: 1, | ||
| limit: 1, | ||
| }); | ||
| const [enrichedPost] = await this.enrichIfQuoteOrReply([fullPost], userId); |
There was a problem hiding this comment.
Moving the fullPost retrieval and enrichment (findPosts + enrichIfQuoteOrReply) outside the transaction and before notifications introduces a performance regression. This adds two additional database queries that execute sequentially before any notifications can be sent. The original implementation only performed a simple findUnique query inside the transaction when parentId existed. Consider either: 1) keeping the simple parent post user_id lookup inside the transaction as before, or 2) fetching the parent post author in parallel with the fullPost enrichment to avoid blocking notification emission.
| select: { | ||
| id: true, | ||
| user_id: true, | ||
| content: true, | ||
| type: true, | ||
| created_at: true, | ||
| parent_id: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The hashtags property is conditionally spread into the data object only when hashtagRecords has items. However, the select clause for the post.create operation does not include hashtags anymore (previously it used include: { hashtags: true }). This means the returned post object will never have hashtag information, which could break downstream code that expects it. If hashtag data is not needed in the transaction return value, ensure that all calling code is updated accordingly.
…ction