fix: address code review issues for embeds implementation

- Fix ?? to || in metadataFetcher.ts to handle empty strings from Cheerio
- Fix stale embeds on message edit: delete old embeds synchronously before
  broadcast, then resolve new ones async (all 4 edit paths: REST+WS, msg+DM)
- Revert unrelated MessageList.tsx scroll threshold change (5000 not 150)
- Remove duplicate embed indexes from migrateAddIndexes (kept standalone ones)
This commit is contained in:
Jannis Braun
2026-03-20 23:58:06 +01:00
parent dcdf10c5de
commit ff2decded5
6 changed files with 30 additions and 23 deletions
-3
View File
@@ -465,9 +465,6 @@ function migrateAddIndexes(db: Database.Database): void {
// Categories // Categories
'CREATE INDEX IF NOT EXISTS idx_channel_categories_space_id ON channel_categories(space_id)', 'CREATE INDEX IF NOT EXISTS idx_channel_categories_space_id ON channel_categories(space_id)',
// Embeds
'CREATE INDEX IF NOT EXISTS idx_embeds_message_id ON embeds(message_id)',
'CREATE INDEX IF NOT EXISTS idx_embeds_dm_message_id ON embeds(dm_message_id)',
]; ];
db.exec(indexes.join(';\n')); db.exec(indexes.join(';\n'));
+5 -2
View File
@@ -1007,6 +1007,9 @@ export async function dmRoutes(app: FastifyInstance): Promise<void> {
.where(eq(schema.dmMessages.id, id)) .where(eq(schema.dmMessages.id, id))
.run(); .run();
// Delete old embeds synchronously so the broadcast reflects the edit
db.delete(schema.embeds).where(eq(schema.embeds.dmMessageId, id)).run();
const updated = getDmMessageWithUser(id); const updated = getDmMessageWithUser(id);
if (!updated) { if (!updated) {
return reply.code(500).send({ error: 'Failed to update message', statusCode: 500 }); return reply.code(500).send({ error: 'Failed to update message', statusCode: 500 });
@@ -1025,9 +1028,9 @@ export async function dmRoutes(app: FastifyInstance): Promise<void> {
}); });
} }
// Re-resolve embeds asynchronously after responding // Resolve new embeds asynchronously (old ones already deleted above)
setImmediate(() => { setImmediate(() => {
reResolveEmbeds(id, content.trim(), msg.dmChannelId, true, null).catch(() => {}); resolveEmbeds(id, content.trim(), msg.dmChannelId, true, null).catch(() => {});
}); });
return reply.code(200).send(updated); return reply.code(200).send(updated);
+8 -7
View File
@@ -410,20 +410,21 @@ export async function messageRoutes(app: FastifyInstance): Promise<void> {
.where(eq(schema.attachments.messageId, id)) .where(eq(schema.attachments.messageId, id))
.all(); .all();
// Hydrate reactions, embeds, and reply-to // Delete old embeds synchronously so the broadcast reflects the edit
db.delete(schema.embeds).where(eq(schema.embeds.messageId, id)).run();
// Hydrate reactions and reply-to (embeds are empty after deletion)
const reactionsMap = fetchReactionsForMessages([id]); const reactionsMap = fetchReactionsForMessages([id]);
const reactions = reactionsMap.get(id) ?? []; const reactions = reactionsMap.get(id) ?? [];
const embedMap = fetchEmbedsForMessages([id]);
const embedRows = embedMap.get(id) ?? [];
let replyTo: MessageWithUser | null = null; let replyTo: MessageWithUser | null = null;
if (updatedMessage.replyToId) { if (updatedMessage.replyToId) {
const replyToMap = fetchReplyToMessages([updatedMessage]); const replyToMap = fetchReplyToMessages([updatedMessage]);
replyTo = replyToMap.get(updatedMessage.replyToId) ?? null; replyTo = replyToMap.get(updatedMessage.replyToId) ?? null;
} }
const messageWithUser = buildMessageWithUser(updatedMessage, user, attachmentRows, reactions, replyTo, embedRows); const messageWithUser = buildMessageWithUser(updatedMessage, user, attachmentRows, reactions, replyTo, []);
// Broadcast edit // Broadcast edit (with empty embeds — new ones arrive via embeds_resolved)
const spaceId = getChannelSpaceId(message.channelId); const spaceId = getChannelSpaceId(message.channelId);
if (spaceId) { if (spaceId) {
connectionManager.sendToSpace(spaceId, { connectionManager.sendToSpace(spaceId, {
@@ -431,9 +432,9 @@ export async function messageRoutes(app: FastifyInstance): Promise<void> {
message: messageWithUser, message: messageWithUser,
}); });
// Re-resolve embeds asynchronously after responding // Resolve new embeds asynchronously (old ones already deleted above)
setImmediate(() => { setImmediate(() => {
reResolveEmbeds(id, content.trim(), message.channelId, false, spaceId).catch(() => {}); resolveEmbeds(id, content.trim(), message.channelId, false, spaceId).catch(() => {});
}); });
} }
+5 -5
View File
@@ -93,13 +93,13 @@ export async function fetchUrlMetadata(url: string): Promise<UrlMetadata | null>
const $ = cheerio.load(html); const $ = cheerio.load(html);
const metadata: UrlMetadata = { const metadata: UrlMetadata = {
title: $('meta[property="og:title"]').attr('content') ?? $('title').text() ?? null, title: $('meta[property="og:title"]').attr('content') || $('title').text() || null,
description: description:
$('meta[property="og:description"]').attr('content') ?? $('meta[property="og:description"]').attr('content') ||
$('meta[name="description"]').attr('content') ?? $('meta[name="description"]').attr('content') ||
null, null,
image: $('meta[property="og:image"]').attr('content') ?? null, image: $('meta[property="og:image"]').attr('content') || null,
siteName: $('meta[property="og:site_name"]').attr('content') ?? null, siteName: $('meta[property="og:site_name"]').attr('content') || null,
url, url,
}; };
+10 -4
View File
@@ -313,6 +313,9 @@ function handleMessageEdit(event: Record<string, unknown>, userId: string): void
const spaceId = getChannelSpaceId(message.channelId); const spaceId = getChannelSpaceId(message.channelId);
if (!spaceId) return; if (!spaceId) return;
// Delete old embeds synchronously so the broadcast reflects the edit
db.delete(schema.embeds).where(eq(schema.embeds.messageId, messageId)).run();
const updatedMessage = getMessageWithUser(messageId); const updatedMessage = getMessageWithUser(messageId);
if (updatedMessage) { if (updatedMessage) {
connectionManager.sendToChannel(spaceId, message.channelId, { connectionManager.sendToChannel(spaceId, message.channelId, {
@@ -320,9 +323,9 @@ function handleMessageEdit(event: Record<string, unknown>, userId: string): void
message: updatedMessage, message: updatedMessage,
}); });
// Re-resolve embeds asynchronously // Resolve new embeds asynchronously (old ones already deleted above)
setImmediate(() => { setImmediate(() => {
reResolveEmbeds(messageId, content.trim(), message.channelId, false, spaceId).catch(() => {}); resolveEmbeds(messageId, content.trim(), message.channelId, false, spaceId).catch(() => {});
}); });
} }
} }
@@ -862,6 +865,9 @@ function handleDmMessageEdit(event: Record<string, unknown>, userId: string): vo
.where(eq(schema.dmMessages.id, messageId)) .where(eq(schema.dmMessages.id, messageId))
.run(); .run();
// Delete old embeds synchronously so the broadcast reflects the edit
db.delete(schema.embeds).where(eq(schema.embeds.dmMessageId, messageId)).run();
const updated = getDmMessageWithUser(messageId); const updated = getDmMessageWithUser(messageId);
if (!updated) return; if (!updated) return;
@@ -877,9 +883,9 @@ function handleDmMessageEdit(event: Record<string, unknown>, userId: string): vo
}); });
} }
// Re-resolve embeds asynchronously // Resolve new embeds asynchronously (old ones already deleted above)
setImmediate(() => { setImmediate(() => {
reResolveEmbeds(messageId, content.trim(), msg.dmChannelId, true, null).catch(() => {}); resolveEmbeds(messageId, content.trim(), msg.dmChannelId, true, null).catch(() => {});
}); });
} }
@@ -134,7 +134,7 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
if (el) { if (el) {
el.scrollIntoView({ block: 'start' }); el.scrollIntoView({ block: 'start' });
const dist = container.scrollHeight - container.scrollTop - container.clientHeight; const dist = container.scrollHeight - container.scrollTop - container.clientHeight;
const near = dist < 150; const near = dist < 5000;
setIsNearBottom(near); setIsNearBottom(near);
isNearBottomRef.current = near; isNearBottomRef.current = near;
return; return;
@@ -218,7 +218,7 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
// Check if near bottom // Check if near bottom
const distanceFromBottom = container.scrollHeight - container.scrollTop - container.clientHeight; const distanceFromBottom = container.scrollHeight - container.scrollTop - container.clientHeight;
const nearBottom = distanceFromBottom < 150; const nearBottom = distanceFromBottom < 5000;
setIsNearBottom(nearBottom); setIsNearBottom(nearBottom);
isNearBottomRef.current = nearBottom; isNearBottomRef.current = nearBottom;