diff --git a/agents-docs/LESSONS.md b/agents-docs/LESSONS.md index 0a196fe..63c1fe3 100644 --- a/agents-docs/LESSONS.md +++ b/agents-docs/LESSONS.md @@ -25,6 +25,13 @@ Durable rules for AI agents working on this project. Read this file at session s ## Lessons +### Bind chat attachments to a pre-allocated message id, never by matching content [attachments] [chat] [mobile] + +- **Trigger:** caption-less media (videos/images sent with no text) grouped onto the message bubble above and left an empty message below on Android — `ChatMessagesComponent` dispatched `sendMessage` without an id, then a `setTimeout` re-discovered the message by `entry.content === content` (always `''` for attachment-only sends) and called `publishAttachments` on it. +- **Rule:** pre-allocate the message id in the component (`planChatMessageSend` in `chat-message-send.rules.ts`), dispatch it via `MessagesActions.sendMessage({ id, ... })` (effect uses `id ?? uuidv4()`), and bind attachments to that exact id with `publishAttachments(id, files)` — never re-find the message by content/timing. +- **Why:** empty content is shared by every attachment-only message, so content matching picks the newest match and races the async create-effect; on Android the create latency exceeds the old 100 ms timer, so the file binds to a stale sibling. The race is invisible on fast desktop browsers, so the deterministic regression proof is the unit test that asserts the dispatched action id equals the attachment-binding id, not an e2e timing game (see the "don't bump E2E timeouts for sync flakes" lesson). +- **Example:** `planChatMessageSend(...).attachmentBinding.messageId === plan.action.id` enforced in `chat-message-send.rules.spec.ts`; behavioral guard in `e2e/tests/chat/attachment-only-message-grouping.spec.ts` (proves the id flows component→effect→attachment by requiring each caption-less attachment to render in its own bubble). + ### Attachment file persistence must be platform-agnostic, not Electron-only [attachments] [persistence] [mobile] - **Trigger:** `AttachmentStorageService` talked only to `window.electronAPI`, so `canWriteFiles()` returned `false` on Android (Capacitor) and in the browser — no bytes were ever persisted there, and after restart/logout-login the uploader hit "Your original upload could not be found on this device" / "no peer with this file". diff --git a/e2e/pages/chat-messages.page.ts b/e2e/pages/chat-messages.page.ts index a48f744..1ff88a6 100644 --- a/e2e/pages/chat-messages.page.ts +++ b/e2e/pages/chat-messages.page.ts @@ -94,6 +94,25 @@ export class ChatMessagesPage { }, files); } + /** Sends the currently-attached files with no text caption (attachment-only message). */ + async sendPendingAttachments(): Promise { + await this.waitForReady(); + await expect(this.sendButton).toBeEnabled({ timeout: 10_000 }); + await this.sendButton.click(); + } + + /** The message bubble that contains the rendered image with the given alt text. */ + getMessageItemContainingImage(altText: string): Locator { + return this.messageItems.filter({ + has: this.page.locator(`img[alt="${altText}"]`) + }).last(); + } + + /** Resolves the stable data-message-id of the bubble holding the given image. */ + async getMessageIdContainingImage(altText: string): Promise { + return this.getMessageItemContainingImage(altText).getAttribute('data-message-id'); + } + async openGifPicker(): Promise { await this.waitForReady(); await this.gifButton.click(); diff --git a/e2e/tests/chat/attachment-only-message-grouping.spec.ts b/e2e/tests/chat/attachment-only-message-grouping.spec.ts new file mode 100644 index 0000000..ab23a3b --- /dev/null +++ b/e2e/tests/chat/attachment-only-message-grouping.spec.ts @@ -0,0 +1,125 @@ +import { + test, + expect, + type Client +} from '../../fixtures/multi-client'; +import { RegisterPage } from '../../pages/register.page'; +import { ServerSearchPage } from '../../pages/server-search.page'; +import { ChatMessagesPage, type ChatDropFilePayload } from '../../pages/chat-messages.page'; + +/** + * Regression coverage for: "Video attachment on android gets sent in the + * message bubble above with no preview image." + * + * Root cause was platform-agnostic: caption-less media was bound to a message + * re-discovered by matching `content` (always '' for attachment-only sends), + * which raced the async create-effect and grouped a second attachment onto the + * previous bubble - leaving an empty message behind. The fix pre-allocates the + * message id, dispatches it, and binds attachments to that exact id. This test + * proves each caption-less attachment lands in its own bubble and renders. + */ +test.describe('Attachment-only message grouping', () => { + test.describe.configure({ timeout: 180_000 }); + + test('each caption-less attachment keeps its own message bubble and preview', async ({ createClient }) => { + const scenario = await createSingleClientChatScenario(createClient); + const serverName = `Attachment Group ${uniqueName('srv')}`; + const introText = `Intro line ${uniqueName('intro')}`; + const firstImageName = `${uniqueName('first')}.svg`; + const secondImageName = `${uniqueName('second')}.svg`; + const firstImage = createSvgFilePayload(firstImageName); + const secondImage = createSvgFilePayload(secondImageName); + + await test.step('Create a server and open its room', async () => { + await scenario.search.createServer(serverName, { description: 'Attachment grouping regression server' }); + await expect(scenario.client.page).toHaveURL(/\/room\//, { timeout: 15_000 }); + await scenario.messages.waitForReady(); + }); + + await test.step('Send a normal text message first', async () => { + await scenario.messages.sendMessage(introText); + await expect(scenario.messages.getMessageItemByText(introText)).toBeVisible({ timeout: 20_000 }); + }); + + await test.step('Send two caption-less attachments back-to-back', async () => { + // Fire them rapidly (no render wait between) to mirror the reported + // rapid-upload repro and stress the message-create vs. attach ordering. + await scenario.messages.attachFiles([firstImage]); + await scenario.messages.sendPendingAttachments(); + await scenario.messages.attachFiles([secondImage]); + await scenario.messages.sendPendingAttachments(); + + await scenario.messages.expectMessageImageLoaded(firstImageName); + await scenario.messages.expectMessageImageLoaded(secondImageName); + }); + + await test.step('Each attachment lives in its own bubble (no grouping, no blank message)', async () => { + const firstMessageId = await scenario.messages.getMessageIdContainingImage(firstImageName); + const secondMessageId = await scenario.messages.getMessageIdContainingImage(secondImageName); + + expect(firstMessageId).toBeTruthy(); + expect(secondMessageId).toBeTruthy(); + // The bug grouped both onto one bubble; distinct ids prove they did not. + expect(firstMessageId).not.toBe(secondMessageId); + + // Exactly two bubbles carry an image, and neither carries both. + await expect(scenario.messages.messageItems.filter({ has: scenario.client.page.locator('img[alt$=".svg"]') })) + .toHaveCount(2, { timeout: 20_000 }); + + await expect(scenario.messages.getMessageItemContainingImage(firstImageName).locator('img[alt$=".svg"]')) + .toHaveCount(1); + + await expect(scenario.messages.getMessageItemContainingImage(secondImageName).locator('img[alt$=".svg"]')) + .toHaveCount(1); + }); + }); +}); + +interface SingleClientChatScenario { + client: Client; + messages: ChatMessagesPage; + search: ServerSearchPage; +} + +async function createSingleClientChatScenario(createClient: () => Promise): Promise { + const suffix = uniqueName('solo'); + const client = await createClient(); + const credentials = { + username: `solo_${suffix}`, + displayName: 'Solo', + password: 'TestPass123!' + }; + const registerPage = new RegisterPage(client.page); + + await registerPage.goto(); + await registerPage.register(credentials.username, credentials.displayName, credentials.password); + + await expect(client.page).toHaveURL(/\/dashboard/, { timeout: 15_000 }); + + return { + client, + messages: new ChatMessagesPage(client.page), + search: new ServerSearchPage(client.page) + }; +} + +function createSvgFilePayload(name: string): ChatDropFilePayload { + const markup = [ + '', + '', + '', + `${name}`, + '' + ].join(''); + + return { + name, + mimeType: 'image/svg+xml', + base64: Buffer.from(markup, 'utf8').toString('base64') + }; +} + +function uniqueName(prefix: string): string { + return `${prefix}-${Date.now()}-${Math.random().toString(36) + .slice(2, 8)}`; +} diff --git a/images/icon-new-rounded.png b/images/icon-new-rounded.png new file mode 100644 index 0000000..4da9e05 Binary files /dev/null and b/images/icon-new-rounded.png differ diff --git a/toju-app/src/app/domains/chat/README.md b/toju-app/src/app/domains/chat/README.md index b9da600..72a8344 100644 --- a/toju-app/src/app/domains/chat/README.md +++ b/toju-app/src/app/domains/chat/README.md @@ -97,6 +97,10 @@ sequenceDiagram User->>DC: broadcastMessage(delete-message) ``` +### Attachment binding (pre-allocated message id) + +`ChatMessagesComponent.handleMessageSubmitted` pre-allocates the message id via `planChatMessageSend` (`domain/rules/chat-message-send.rules.ts`), passes it to `MessagesActions.sendMessage({ id, ... })`, and binds any pending files to that **same** id with `AttachmentFacade.publishAttachments(id, ...)`. The send effect honours a provided `id` (falling back to a fresh UUID). Never re-discover the just-sent message by matching its `content` to attach files — caption-less media all share an empty content string, so a content match is ambiguous and races the async create-effect, grouping attachments onto a sibling bubble and leaving an empty message behind. + ## Message integrity Outgoing creates/edits/deletes also emit signed `message-revision` events and persist revision audit rows locally. Sync inventories include `revision` and `headHash`; merge prefers a verified higher revision over legacy timestamp comparison. See `agents-docs/features/message-integrity.md` and `MessageRevisionService`. diff --git a/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.spec.ts b/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.spec.ts new file mode 100644 index 0000000..83d2fd6 --- /dev/null +++ b/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.spec.ts @@ -0,0 +1,79 @@ +import { + describe, + expect, + it +} from 'vitest'; + +import { planChatMessageSend } from './chat-message-send.rules'; + +function makeFile(name: string): File { + return new File([ + new Uint8Array([ + 1, + 2, + 3 + ]) + ], name, { type: 'video/mp4' }); +} + +describe('planChatMessageSend', () => { + it('binds attachments to the exact id carried by the dispatched action', () => { + const video = makeFile('clip.mp4'); + const plan = planChatMessageSend({ + generateId: () => 'msg-123', + content: '', + pendingFiles: [video] + }); + + expect(plan.action.id).toBe('msg-123'); + expect(plan.attachmentBinding).not.toBeNull(); + // The whole point of the fix: same id for the action and the attachment binding. + expect(plan.attachmentBinding?.messageId).toBe(plan.action.id); + expect(plan.attachmentBinding?.files).toEqual([video]); + }); + + it('gives each caption-less media send a distinct id so attachments never group onto a sibling', () => { + const ids = ['msg-1', 'msg-2']; + + let call = 0; + + const generateId = () => ids[call++]; + const first = planChatMessageSend({ generateId, content: '', pendingFiles: [makeFile('first.mp4')] }); + const second = planChatMessageSend({ generateId, content: '', pendingFiles: [makeFile('second.mp4')] }); + + expect(first.action.id).toBe('msg-1'); + expect(second.action.id).toBe('msg-2'); + expect(first.action.id).not.toBe(second.action.id); + expect(first.attachmentBinding?.messageId).toBe('msg-1'); + expect(second.attachmentBinding?.messageId).toBe('msg-2'); + }); + + it('produces no attachment binding when there are no pending files', () => { + const plan = planChatMessageSend({ + generateId: () => 'msg-text-only', + content: 'hello', + pendingFiles: [], + replyToId: 'reply-1', + channelId: 'general' + }); + + expect(plan.attachmentBinding).toBeNull(); + expect(plan.action).toEqual({ + id: 'msg-text-only', + content: 'hello', + replyToId: 'reply-1', + channelId: 'general' + }); + }); + + it('normalizes a null channel id to undefined', () => { + const plan = planChatMessageSend({ + generateId: () => 'msg-x', + content: 'hi', + pendingFiles: [], + channelId: null + }); + + expect(plan.action.channelId).toBeUndefined(); + }); +}); diff --git a/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.ts b/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.ts new file mode 100644 index 0000000..4ec1d05 --- /dev/null +++ b/toju-app/src/app/domains/chat/domain/rules/chat-message-send.rules.ts @@ -0,0 +1,52 @@ +/** + * Pure planning for an outgoing chat message and its attachments. + * + * The single invariant this module guarantees is that the id carried by the + * dispatched `Send Message` action is the *same* id used to bind any pending + * attachments. Earlier code dispatched the message without an id and then + * tried to re-discover the just-created message by matching its `content`. + * For caption-less media (content === '') that match is ambiguous and races + * the asynchronous message-create effect, so an attachment could bind to a + * sibling message - grouping a video onto the bubble above and leaving an + * empty message behind (Android-visible, but a latent bug on every platform). + */ +export interface ChatMessageSendAction { + id: string; + content: string; + replyToId?: string; + channelId?: string; +} + +export interface ChatMessageAttachmentBinding { + messageId: string; + files: File[]; +} + +export interface ChatMessageSendPlan { + action: ChatMessageSendAction; + attachmentBinding: ChatMessageAttachmentBinding | null; +} + +export interface ChatMessageSendInput { + generateId: () => string; + content: string; + pendingFiles: File[]; + replyToId?: string; + channelId?: string | null; +} + +export function planChatMessageSend(input: ChatMessageSendInput): ChatMessageSendPlan { + const id = input.generateId(); + + return { + action: { + id, + content: input.content, + replyToId: input.replyToId, + channelId: input.channelId ?? undefined + }, + attachmentBinding: input.pendingFiles.length > 0 + ? { messageId: id, files: input.pendingFiles } + : null + }; +} diff --git a/toju-app/src/app/domains/chat/feature/chat-messages/chat-messages.component.ts b/toju-app/src/app/domains/chat/feature/chat-messages/chat-messages.component.ts index ebeb8f1..b405a4d 100644 --- a/toju-app/src/app/domains/chat/feature/chat-messages/chat-messages.component.ts +++ b/toju-app/src/app/domains/chat/feature/chat-messages/chat-messages.component.ts @@ -11,6 +11,7 @@ import { import { toObservable, toSignal } from '@angular/core/rxjs-interop'; import { switchMap } from 'rxjs/operators'; import { Store } from '@ngrx/store'; +import { v4 as uuidv4 } from 'uuid'; import { ElectronBridgeService } from '../../../../core/platform/electron/electron-bridge.service'; import { ViewportService } from '../../../../core/platform'; import { BottomSheetComponent } from '../../../../shared'; @@ -36,6 +37,7 @@ import { KlipyGifPickerComponent } from '../klipy-gif-picker/klipy-gif-picker.co import { ChatMessageListComponent } from './components/message-list/chat-message-list.component'; import { ChatMessageOverlaysComponent } from './components/message-overlays/chat-message-overlays.component'; import { stepLightboxIndex } from '../../domain/rules/chat-message-lightbox.rules'; +import { planChatMessageSend } from '../../domain/rules/chat-message-send.rules'; import { ChatLightboxState, ChatMessageComposerSubmitEvent, @@ -117,18 +119,20 @@ export class ChatMessagesComponent { handleMessageSubmitted(event: ChatMessageComposerSubmitEvent): void { this.messageList?.scrollToBottomAfterLocalSend(); - this.store.dispatch( - MessagesActions.sendMessage({ - content: event.content, - replyToId: this.replyTo()?.id, - channelId: this.activeChannelId() - }) - ); + const plan = planChatMessageSend({ + generateId: uuidv4, + content: event.content, + pendingFiles: event.pendingFiles, + replyToId: this.replyTo()?.id, + channelId: this.activeChannelId() + }); + + this.store.dispatch(MessagesActions.sendMessage(plan.action)); this.clearReply(); - if (event.pendingFiles.length > 0) { - setTimeout(() => this.attachFilesToLastOwnMessage(event.content, event.pendingFiles), 100); + if (plan.attachmentBinding) { + this.attachFilesToMessage(plan.attachmentBinding.messageId, plan.attachmentBinding.files); } } @@ -493,21 +497,14 @@ export class ChatMessagesComponent { }); } - private attachFilesToLastOwnMessage(content: string, pendingFiles: File[]): void { + private attachFilesToMessage(messageId: string, pendingFiles: File[]): void { const currentUserId = this.currentUser()?.id; + const roomId = this.currentRoom()?.id; - if (!currentUserId) - return; - - const message = [...this.channelMessages()] - .reverse() - .find((entry) => entry.senderId === currentUserId && entry.content === content && !entry.isDeleted); - - if (!message) { - setTimeout(() => this.attachFilesToLastOwnMessage(content, pendingFiles), 150); - return; + if (roomId) { + this.attachmentsSvc.rememberMessageRoom(messageId, roomId); } - this.attachmentsSvc.publishAttachments(message.id, pendingFiles, currentUserId || undefined); + this.attachmentsSvc.publishAttachments(messageId, pendingFiles, currentUserId || undefined); } } diff --git a/toju-app/src/app/store/messages/messages.actions.ts b/toju-app/src/app/store/messages/messages.actions.ts index e0b758d..9ba6af6 100644 --- a/toju-app/src/app/store/messages/messages.actions.ts +++ b/toju-app/src/app/store/messages/messages.actions.ts @@ -50,8 +50,15 @@ export const MessagesActions = createActionGroup({ }>(), 'Load Older Messages Failure': props<{ error: string }>(), - /** Sends a new chat message to the current room and broadcasts to peers. */ - 'Send Message': props<{ content: string; replyToId?: string; channelId?: string }>(), + /** + * Sends a new chat message to the current room and broadcasts to peers. + * + * An optional `id` lets the caller pre-allocate the message id so it can + * bind attachments to the exact message it created, instead of guessing + * by matching content after the fact (which mis-attaches caption-less + * media to a sibling message - see the attachment-only media bug). + */ + 'Send Message': props<{ id?: string; content: string; replyToId?: string; channelId?: string }>(), 'Send Message Success': props<{ message: Message }>(), 'Send Message Failure': props<{ error: string }>(), diff --git a/toju-app/src/app/store/messages/messages.effects.ts b/toju-app/src/app/store/messages/messages.effects.ts index 65f624a..a491fd4 100644 --- a/toju-app/src/app/store/messages/messages.effects.ts +++ b/toju-app/src/app/store/messages/messages.effects.ts @@ -230,7 +230,7 @@ export class MessagesEffects { this.store.select(selectCurrentRoom) ), mergeMap(([ - { content, replyToId, channelId }, + { id, content, replyToId, channelId }, currentUser, currentRoom ]) => { @@ -239,7 +239,7 @@ export class MessagesEffects { } const draftMessage: Message = { - id: uuidv4(), + id: id ?? uuidv4(), roomId: currentRoom.id, channelId: channelId || 'general', senderId: currentUser.id,