From b2319b43d68d06581efbaa30756c3b9fa77d32a1 Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Tue, 15 Aug 2023 19:24:19 -0400 Subject: [PATCH] Tracks send state for edited messages --- ts/CI.ts | 28 ++- ts/components/CompositionArea.tsx | 2 + ts/messageModifiers/MessageReceipts.ts | 97 ++++++--- ts/model-types.d.ts | 1 + ts/sql/channels.ts | 4 +- ts/test-mock/messaging/edit_test.ts | 261 ++++++++++++++++++++++++- ts/util/handleEditMessage.ts | 3 + ts/util/sendEditedMessage.ts | 50 ++--- 8 files changed, 391 insertions(+), 55 deletions(-) diff --git a/ts/CI.ts b/ts/CI.ts index 1209d277af0e..0a30ad737128 100644 --- a/ts/CI.ts +++ b/ts/CI.ts @@ -3,15 +3,21 @@ import { ipcRenderer } from 'electron'; -import { explodePromise } from './util/explodePromise'; -import { SECOND } from './util/durations'; -import * as log from './logging/log'; import type { IPCResponse as ChallengeResponseType } from './challenge'; +import type { MessageAttributesType } from './model-types.d'; +import * as log from './logging/log'; +import { explodePromise } from './util/explodePromise'; +import { ipcInvoke } from './sql/channels'; +import { SECOND } from './util/durations'; type ResolveType = (data: unknown) => void; export type CIType = { deviceName: string; + getConversationId: (address: string | null) => string | null; + getMessagesBySentAt( + sentAt: number + ): Promise>; handleEvent: (event: string, data: unknown) => unknown; setProvisioningURL: (url: string) => unknown; solveChallenge: (response: ChallengeResponseType) => unknown; @@ -108,8 +114,24 @@ export function getCI(deviceName: string): CIType { window.Signal.challengeHandler?.onResponse(response); } + async function getMessagesBySentAt(sentAt: number) { + const messages = await ipcInvoke>( + 'getMessagesBySentAt', + [sentAt] + ); + return messages.map( + m => window.MessageController.register(m.id, m).attributes + ); + } + + function getConversationId(address: string | null): string | null { + return window.ConversationController.getConversationId(address); + } + return { deviceName, + getConversationId, + getMessagesBySentAt, handleEvent, setProvisioningURL, solveChallenge, diff --git a/ts/components/CompositionArea.tsx b/ts/components/CompositionArea.tsx index 1c505fc6d76a..4127002a8ad4 100644 --- a/ts/components/CompositionArea.tsx +++ b/ts/components/CompositionArea.tsx @@ -434,12 +434,14 @@ export function CompositionArea({ useEffect(() => { if (inputApiRef.current) { inputApiRef.current.focus(); + setHasFocus(true); } }, []); // Focus input whenever explicitly requested useEffect(() => { if (focusCounter !== previousFocusCounter && inputApiRef.current) { inputApiRef.current.focus(); + setHasFocus(true); } }, [inputApiRef, focusCounter, previousFocusCounter]); diff --git a/ts/messageModifiers/MessageReceipts.ts b/ts/messageModifiers/MessageReceipts.ts index c09cfa824086..af2e0e5b17c6 100644 --- a/ts/messageModifiers/MessageReceipts.ts +++ b/ts/messageModifiers/MessageReceipts.ts @@ -8,6 +8,7 @@ import { Collection, Model } from 'backbone'; import type { MessageModel } from '../models/messages'; import type { MessageAttributesType } from '../model-types.d'; +import type { SendStateByConversationId } from '../messages/MessageSendState'; import { isOutgoing, isStory } from '../state/selectors/message'; import { getOwn } from '../util/getOwn'; import { missingCaseError } from '../util/missingCaseError'; @@ -183,26 +184,14 @@ export class MessageReceipts extends Collection { }); } - private async updateMessageSendState( - receipt: MessageReceiptModel, - message: MessageModel - ): Promise { - const messageSentAt = receipt.get('messageSentAt'); - - if (shouldDropReceipt(receipt, message)) { - log.info( - `MessageReceipts: Dropping a receipt ${receipt.get('type')} ` + - `for message ${messageSentAt}` - ); - return; - } - + private getNewSendStateByConversationId( + oldSendStateByConversationId: SendStateByConversationId, + receipt: MessageReceiptModel + ): SendStateByConversationId { const receiptTimestamp = receipt.get('receiptTimestamp'); const sourceConversationId = receipt.get('sourceConversationId'); const type = receipt.get('type'); - const oldSendStateByConversationId = - message.get('sendStateByConversationId') || {}; const oldSendState = getOwn( oldSendStateByConversationId, sourceConversationId @@ -228,14 +217,73 @@ export class MessageReceipts extends Collection { updatedAt: receiptTimestamp, }); - // The send state may not change. For example, this can happen if we get a read - // receipt before a delivery receipt. - if (!isEqual(oldSendState, newSendState)) { - message.set('sendStateByConversationId', { - ...oldSendStateByConversationId, - [sourceConversationId]: newSendState, - }); + return { + ...oldSendStateByConversationId, + [sourceConversationId]: newSendState, + }; + } + private async updateMessageSendState( + receipt: MessageReceiptModel, + message: MessageModel + ): Promise { + const messageSentAt = receipt.get('messageSentAt'); + + if (shouldDropReceipt(receipt, message)) { + log.info( + `MessageReceipts: Dropping a receipt ${receipt.get('type')} ` + + `for message ${messageSentAt}` + ); + return; + } + + let hasChanges = false; + + const editHistory = message.get('editHistory') ?? []; + const newEditHistory = editHistory?.map(edit => { + if (messageSentAt !== edit.timestamp) { + return edit; + } + + const oldSendStateByConversationId = edit.sendStateByConversationId ?? {}; + const newSendStateByConversationId = this.getNewSendStateByConversationId( + oldSendStateByConversationId, + receipt + ); + + return { + ...edit, + sendStateByConversationId: newSendStateByConversationId, + }; + }); + if (!isEqual(newEditHistory, editHistory)) { + message.set('editHistory', newEditHistory); + hasChanges = true; + } + + const editMessageTimestamp = message.get('editMessageTimestamp'); + if ( + messageSentAt === message.get('timestamp') || + messageSentAt === editMessageTimestamp + ) { + const oldSendStateByConversationId = + message.get('sendStateByConversationId') ?? {}; + const newSendStateByConversationId = this.getNewSendStateByConversationId( + oldSendStateByConversationId, + receipt + ); + + // The send state may not change. For example, this can happen if we get a read + // receipt before a delivery receipt. + if ( + !isEqual(oldSendStateByConversationId, newSendStateByConversationId) + ) { + message.set('sendStateByConversationId', newSendStateByConversationId); + hasChanges = true; + } + } + + if (hasChanges) { queueUpdateMessage(message.attributes); // notify frontend listeners @@ -250,6 +298,9 @@ export class MessageReceipts extends Collection { } } + const sourceConversationId = receipt.get('sourceConversationId'); + const type = receipt.get('type'); + if ( (type === MessageReceiptType.Delivery && wasDeliveredWithSealedSender(sourceConversationId, message) && diff --git a/ts/model-types.d.ts b/ts/model-types.d.ts index 5db343d955a8..908593450952 100644 --- a/ts/model-types.d.ts +++ b/ts/model-types.d.ts @@ -131,6 +131,7 @@ export type EditHistoryType = { bodyRanges?: ReadonlyArray; preview?: Array; quote?: QuotedMessageType; + sendStateByConversationId?: SendStateByConversationId; timestamp: number; }; diff --git a/ts/sql/channels.ts b/ts/sql/channels.ts index 1d25df125c0c..37950afa92b9 100644 --- a/ts/sql/channels.ts +++ b/ts/sql/channels.ts @@ -11,10 +11,10 @@ let activeJobCount = 0; let resolveShutdown: (() => void) | undefined; let shutdownPromise: Promise | null = null; -export async function ipcInvoke( +export async function ipcInvoke( name: string, args: ReadonlyArray -): Promise { +): Promise { const fnName = String(name); if (shutdownPromise && name !== 'close') { diff --git a/ts/test-mock/messaging/edit_test.ts b/ts/test-mock/messaging/edit_test.ts index ae78c4b39d3a..9dd193b6bda1 100644 --- a/ts/test-mock/messaging/edit_test.ts +++ b/ts/test-mock/messaging/edit_test.ts @@ -5,12 +5,14 @@ import { Proto } from '@signalapp/mock-server'; import { assert } from 'chai'; import createDebug from 'debug'; import Long from 'long'; -import { strictAssert } from '../../util/assert'; -import * as durations from '../../util/durations'; import type { App } from '../playwright'; +import * as durations from '../../util/durations'; import { Bootstrap } from '../bootstrap'; import { ReceiptType } from '../../types/Receipt'; +import { SendStatus } from '../../messages/MessageSendState'; +import { sleep } from '../../util/sleep'; +import { strictAssert } from '../../util/assert'; export const debug = createDebug('mock:test:edit'); @@ -74,7 +76,7 @@ describe('editing', function needsName() { await bootstrap.teardown(); }); - it('handles outgoing edited messages phone to desktop', async () => { + it('handles incoming edited messages phone to desktop', async () => { const { phone, desktop } = bootstrap; const window = await app.getWindow(); @@ -403,4 +405,257 @@ describe('editing', function needsName() { const messages = window.locator('.module-message__text'); assert.strictEqual(await messages.count(), 1, 'message count'); }); + + it('tracks message send state for edits', async () => { + const { contacts, desktop } = bootstrap; + + const [friend] = contacts; + const contact = friend.toContact(); + + const page = await app.getWindow(); + + debug('message sent friend -> desktop'); + await friend.sendText(desktop, 'hello', { + timestamp: bootstrap.getTimestamp(), + }); + + debug('opening conversation'); + const leftPane = page.locator('#LeftPane'); + await leftPane + .locator('.module-conversation-list__item--contact-or-conversation') + .first() + .click(); + await page.locator('.module-conversation-hero').waitFor(); + + // Sending the original message + // getting a read receipt + // testing the message's send state + const originalText = 'v1'; + debug('finding composition input and clicking it'); + { + const input = await app.waitForEnabledComposer(); + + debug('sending message desktop -> friend'); + await input.type(originalText); + await input.press('Enter'); + } + + debug("waiting for message on friend's device"); + const { dataMessage: originalMessage } = await friend.waitForMessage(); + assert.strictEqual( + originalMessage.body, + originalText, + 'original message is correct' + ); + strictAssert(originalMessage.timestamp, 'timestamp missing'); + + const originalMessageTimestamp = Number(originalMessage.timestamp); + debug('original message', { timestamp: originalMessageTimestamp }); + + { + const readReceiptTimestamp = bootstrap.getTimestamp(); + debug('sending read receipt friend -> desktop', { + target: originalMessageTimestamp, + timestamp: readReceiptTimestamp, + }); + const readReceiptSendOptions = { + timestamp: readReceiptTimestamp, + }; + await friend.sendRaw( + desktop, + { + receiptMessage: { + type: 1, + timestamp: [originalMessage.timestamp], + }, + }, + readReceiptSendOptions + ); + } + + debug("getting friend's conversationId"); + const conversationId = await page.evaluate( + serviceId => window.SignalCI?.getConversationId(serviceId), + contact.uuid + ); + debug(`got friend's conversationId: ${conversationId}`); + strictAssert(conversationId, 'conversationId exists'); + + debug("testing message's send state (original)"); + { + debug('getting message from app (original)'); + const messages = await page.evaluate( + timestamp => window.SignalCI?.getMessagesBySentAt(timestamp), + originalMessageTimestamp + ); + strictAssert(messages, 'messages does not exist'); + + debug('verifying message send state (original)'); + const [message] = messages; + strictAssert( + message.sendStateByConversationId, + 'sendStateByConversationId' + ); + assert.strictEqual( + message.sendStateByConversationId[conversationId].status, + SendStatus.Read, + 'send state is read for main message' + ); + assert.isUndefined(message.editHistory, 'no edit history, yet'); + } + + // Sending a v2 edited message targetting the original + // this one goes unread + // but we'll still test the send state + const editMessageV2Text = 'v2'; + debug('finding composition input and clicking it v2'); + { + const input = await app.waitForEnabledComposer(); + + debug('sending edit message v2 desktop -> friend'); + await sleep(50); + await input.press('ArrowUp'); + await input.press('Backspace'); + await input.press('Backspace'); + await input.type(editMessageV2Text); + await input.press('Enter'); + } + + debug("waiting for message on friend's device (original)"); + const { editMessage: editMessageV2 } = await friend.waitForEditMessage(); + assert.strictEqual(editMessageV2.dataMessage?.body, editMessageV2Text); + debug('v2 message', { + timestamp: Number(editMessageV2.dataMessage?.timestamp), + }); + + // Sending a v3 edited message targetting v2 + // v3 will be read after we receive v4 + const editMessageV3Text = 'v3'; + debug('finding composition input and clicking it v3'); + { + const input = await app.waitForEnabledComposer(); + + debug('sending edit message v3 desktop -> friend'); + await sleep(50); + await input.press('ArrowUp'); + await input.press('Backspace'); + await input.press('Backspace'); + await input.type(editMessageV3Text); + await input.press('Enter'); + } + + debug("waiting for message on friend's device (v3)"); + const { editMessage: editMessageV3 } = await friend.waitForEditMessage(); + assert.strictEqual(editMessageV3.dataMessage?.body, editMessageV3Text); + strictAssert(editMessageV3.dataMessage?.timestamp, 'timestamp missing'); + + const editMessageV3Timestamp = Number(editMessageV3.dataMessage.timestamp); + debug('v3 message', { timestamp: editMessageV3Timestamp }); + + // Sending a v4 edited message targetting v3 + // getting a read receipt for v3 + // testing send state of the full message + const editMessageV4Text = 'v4'; + debug('finding composition input and clicking it v4'); + { + const input = await app.waitForEnabledComposer(); + + debug('sending edit message v4 desktop -> friend'); + await sleep(50); + await input.press('ArrowUp'); + await input.press('Backspace'); + await input.press('Backspace'); + await input.type(editMessageV4Text); + await input.press('Enter'); + } + + debug("waiting for message on friend's device (v4)"); + const { editMessage: editMessageV4 } = await friend.waitForEditMessage(); + assert.strictEqual(editMessageV4.dataMessage?.body, editMessageV4Text); + strictAssert(editMessageV4.dataMessage?.timestamp, 'timestamp missing'); + + const editMessageV4Timestamp = Number(editMessageV4.dataMessage.timestamp); + debug('v4 message', { timestamp: editMessageV4Timestamp }); + + { + const readReceiptTimestamp = bootstrap.getTimestamp(); + debug('sending read receipt for edit v3 friend -> desktop', { + target: editMessageV3Timestamp, + timestamp: readReceiptTimestamp, + }); + const readReceiptSendOptions = { + timestamp: readReceiptTimestamp, + }; + await friend.sendRaw( + desktop, + { + receiptMessage: { + type: 1, + timestamp: [editMessageV3.dataMessage.timestamp], + }, + }, + readReceiptSendOptions + ); + } + + debug("testing v4's send state"); + { + debug('getting edited message from app (v4)'); + const messages = await page.evaluate( + timestamp => window.SignalCI?.getMessagesBySentAt(timestamp), + originalMessageTimestamp + ); + strictAssert(messages, 'messages does not exist'); + + debug('verifying edited message send state (v4)'); + const [message] = messages; + + strictAssert( + message.sendStateByConversationId, + 'sendStateByConversationId' + ); + assert.strictEqual( + message.sendStateByConversationId[conversationId].status, + SendStatus.Sent, + 'original message send state is sent (v4)' + ); + + strictAssert(message.editHistory, 'edit history exists'); + const [v4, v3, v2, v1] = message.editHistory; + + strictAssert(v1.sendStateByConversationId, 'v1 has send state'); + assert.strictEqual( + v1.sendStateByConversationId[conversationId].status, + SendStatus.Read, + 'send state for first message is read' + ); + + strictAssert(v2.sendStateByConversationId, 'v2 has send state'); + assert.strictEqual( + v2.sendStateByConversationId[conversationId].status, + SendStatus.Pending, + 'send state for v2 message is pending' + ); + + strictAssert(v3.sendStateByConversationId, 'v3 has send state'); + assert.strictEqual( + v3.sendStateByConversationId[conversationId].status, + SendStatus.Read, + 'send state for v3 message is read' + ); + + strictAssert(v4.sendStateByConversationId, 'v4 has send state'); + assert.strictEqual( + v4.sendStateByConversationId[conversationId].status, + SendStatus.Pending, + 'send state for v4 message is pending' + ); + + assert.strictEqual( + v4.body, + message.body, + 'body is same for v4 and main message' + ); + } + }); }); diff --git a/ts/util/handleEditMessage.ts b/ts/util/handleEditMessage.ts index 5d5fd3ba2dd3..c7d20a72858f 100644 --- a/ts/util/handleEditMessage.ts +++ b/ts/util/handleEditMessage.ts @@ -94,6 +94,7 @@ export async function handleEditMessage( bodyRanges: mainMessage.bodyRanges, preview: mainMessage.preview, quote: mainMessage.quote, + sendStateByConversationId: { ...mainMessage.sendStateByConversationId }, timestamp: mainMessage.timestamp, }, ]; @@ -224,6 +225,8 @@ export async function handleEditMessage( body: upgradedEditedMessageData.body, bodyRanges: upgradedEditedMessageData.bodyRanges, preview: nextEditedMessagePreview, + sendStateByConversationId: + upgradedEditedMessageData.sendStateByConversationId, timestamp: upgradedEditedMessageData.timestamp, quote: nextEditedMessageQuote, }; diff --git a/ts/util/sendEditedMessage.ts b/ts/util/sendEditedMessage.ts index 9a691a60227e..4430275f054e 100644 --- a/ts/util/sendEditedMessage.ts +++ b/ts/util/sendEditedMessage.ts @@ -95,30 +95,6 @@ export async function sendEditedMessage( conversation.clearTypingTimers(); - const ourConversation = - window.ConversationController.getOurConversationOrThrow(); - const fromId = ourConversation.id; - - const recipientMaybeConversations = map( - conversation.getRecipients(), - identifier => window.ConversationController.get(identifier) - ); - const recipientConversations = filter(recipientMaybeConversations, isNotNil); - const recipientConversationIds = concat( - map(recipientConversations, c => c.id), - [fromId] - ); - const sendStateByConversationId = zipObject( - recipientConversationIds, - repeat({ - status: SendStatus.Pending, - updatedAt: timestamp, - }) - ); - - // Resetting send state for the target message - targetMessage.set({ sendStateByConversationId }); - // Can't send both preview and attachments const attachments = preview && preview.length ? [] : targetMessage.get('attachments') || []; @@ -165,6 +141,28 @@ export async function sendEditedMessage( } } + const ourConversation = + window.ConversationController.getOurConversationOrThrow(); + const fromId = ourConversation.id; + + // Create the send state for later use + const recipientMaybeConversations = map( + conversation.getRecipients(), + identifier => window.ConversationController.get(identifier) + ); + const recipientConversations = filter(recipientMaybeConversations, isNotNil); + const recipientConversationIds = concat( + map(recipientConversations, c => c.id), + [fromId] + ); + const sendStateByConversationId = zipObject( + recipientConversationIds, + repeat({ + status: SendStatus.Pending, + updatedAt: timestamp, + }) + ); + // An ephemeral message that we just use to handle the edit const tmpMessage: MessageAttributesType = { attachments: attachments?.map((attachment, index) => @@ -188,6 +186,7 @@ export async function sendEditedMessage( quote, received_at: incrementMessageCounter(), received_at_ms: timestamp, + sendStateByConversationId, sent_at: timestamp, timestamp, type: 'outgoing', @@ -202,6 +201,9 @@ export async function sendEditedMessage( message: tmpMessage, }); + // Reset send state prior to send + targetMessage.set({ sendStateByConversationId }); + // Inserting the send into a job and saving it to the message await timeAndLogIfTooLong( SEND_REPORT_THRESHOLD_MS,