From 09d9cd477de3de582737ac0672c4991b9ca7ed7c Mon Sep 17 00:00:00 2001 From: automated-signal <37887102+automated-signal@users.noreply.github.com> Date: Wed, 16 Mar 2022 10:03:12 -0700 Subject: [PATCH] Optimize profile avatar uploads and sync urls Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> --- ts/components/AvatarPreview.tsx | 17 ++---- ts/components/ProfileEditor.stories.tsx | 4 +- ts/components/ProfileEditor.tsx | 53 ++++++++++++++----- ts/components/ProfileEditorModal.tsx | 3 +- ts/models/conversations.ts | 6 +++ ts/services/storageRecordOps.ts | 13 ++--- ts/services/writeProfile.ts | 53 ++++++++++++------- ts/state/ducks/conversations.ts | 7 +-- ts/state/smart/ProfileEditorModal.ts | 4 +- .../util/encryptProfileData_test.ts | 35 ++++++++++-- ts/textsecure/WebAPI.ts | 1 + ts/types/Avatar.ts | 5 ++ ts/types/Storage.d.ts | 2 +- ts/util/encryptProfileData.ts | 12 +++-- ts/util/lint/exceptions.json | 9 +--- 15 files changed, 147 insertions(+), 77 deletions(-) diff --git a/ts/components/AvatarPreview.tsx b/ts/components/AvatarPreview.tsx index 059835569..24e099b8f 100644 --- a/ts/components/AvatarPreview.tsx +++ b/ts/components/AvatarPreview.tsx @@ -2,7 +2,7 @@ // SPDX-License-Identifier: AGPL-3.0-only import type { CSSProperties } from 'react'; -import React, { useEffect, useRef, useState } from 'react'; +import React, { useEffect, useState } from 'react'; import { noop } from 'lodash'; import * as log from '../logging/log'; @@ -46,10 +46,6 @@ export const AvatarPreview = ({ onClick, style = {}, }: PropsType): JSX.Element => { - const startingAvatarPathRef = useRef( - avatarValue ? undefined : avatarPath - ); - const [avatarPreview, setAvatarPreview] = useState(); // Loads the initial avatarPath if one is provided, but only if we're in editable mode. @@ -60,8 +56,7 @@ export const AvatarPreview = ({ return; } - const startingAvatarPath = startingAvatarPathRef.current; - if (!startingAvatarPath) { + if (!avatarPath) { return noop; } @@ -69,14 +64,12 @@ export const AvatarPreview = ({ (async () => { try { - const buffer = await imagePathToBytes(startingAvatarPath); + const buffer = await imagePathToBytes(avatarPath); if (shouldCancel) { return; } setAvatarPreview(buffer); - if (onAvatarLoaded) { - onAvatarLoaded(buffer); - } + onAvatarLoaded?.(buffer); } catch (err) { if (shouldCancel) { return; @@ -92,7 +85,7 @@ export const AvatarPreview = ({ return () => { shouldCancel = true; }; - }, [onAvatarLoaded, isEditable]); + }, [avatarPath, onAvatarLoaded, isEditable]); // Ensures that when avatarValue changes we generate new URLs useEffect(() => { diff --git a/ts/components/ProfileEditor.stories.tsx b/ts/components/ProfileEditor.stories.tsx index bbcd3a8b1..17476a3e5 100644 --- a/ts/components/ProfileEditor.stories.tsx +++ b/ts/components/ProfileEditor.stories.tsx @@ -25,7 +25,7 @@ const stories = storiesOf('Components/ProfileEditor', module); const createProps = (overrideProps: Partial = {}): PropsType => ({ aboutEmoji: overrideProps.aboutEmoji, aboutText: text('about', overrideProps.aboutText || ''), - avatarPath: overrideProps.avatarPath, + profileAvatarPath: overrideProps.profileAvatarPath, clearUsernameSave: action('clearUsernameSave'), conversationId: '123', color: overrideProps.color || getRandomColor(), @@ -64,7 +64,7 @@ stories.add('Full Set', () => { {...createProps({ aboutEmoji: '🙏', aboutText: 'Live. Laugh. Love', - avatarPath: '/fixtures/kitten-3-64-64.jpg', + profileAvatarPath: '/fixtures/kitten-3-64-64.jpg', onSetSkinTone: setSkinTone, familyName: getLastName(), skinTone, diff --git a/ts/components/ProfileEditor.tsx b/ts/components/ProfileEditor.tsx index 49becbda0..0c12c3b12 100644 --- a/ts/components/ProfileEditor.tsx +++ b/ts/components/ProfileEditor.tsx @@ -9,6 +9,7 @@ import type { AvatarColorType } from '../types/Colors'; import { AvatarColors } from '../types/Colors'; import type { AvatarDataType, + AvatarUpdateType, DeleteAvatarFromDiskActionType, ReplaceAvatarActionType, SaveAvatarToDiskActionType, @@ -58,14 +59,14 @@ type PropsExternalType = { onEditStateChanged: (editState: EditState) => unknown; onProfileChanged: ( profileData: ProfileDataType, - avatarBuffer?: Uint8Array + avatar: AvatarUpdateType ) => unknown; }; export type PropsDataType = { aboutEmoji?: string; aboutText?: string; - avatarPath?: string; + profileAvatarPath?: string; color?: AvatarColorType; conversationId: string; familyName?: string; @@ -211,7 +212,7 @@ function mapSaveStateToEditState({ export const ProfileEditor = ({ aboutEmoji, aboutText, - avatarPath, + profileAvatarPath, clearUsernameSave, color, conversationId, @@ -254,9 +255,16 @@ export const ProfileEditor = ({ UsernameEditState.Editing ); + const [startingAvatarPath, setStartingAvatarPath] = + useState(profileAvatarPath); + + const [oldAvatarBuffer, setOldAvatarBuffer] = useState< + Uint8Array | undefined + >(undefined); const [avatarBuffer, setAvatarBuffer] = useState( undefined ); + const [isLoadingAvatar, setIsLoadingAvatar] = useState(true); const [stagedProfile, setStagedProfile] = useState({ aboutEmoji, aboutText, @@ -285,6 +293,9 @@ export const ProfileEditor = ({ // To make AvatarEditor re-render less often const handleAvatarChanged = useCallback( (avatar: Uint8Array | undefined) => { + // Do not display stale avatar from disk anymore. + setStartingAvatarPath(undefined); + setAvatarBuffer(avatar); setEditState(EditState.None); onProfileChanged( @@ -295,10 +306,11 @@ export const ProfileEditor = ({ ? trim(stagedProfile.familyName) : undefined, }, - avatar + { oldAvatar: oldAvatarBuffer, newAvatar: avatar } ); + setOldAvatarBuffer(avatar); }, - [onProfileChanged, stagedProfile] + [onProfileChanged, stagedProfile, oldAvatarBuffer] ); const getFullNameText = () => { @@ -405,9 +417,14 @@ export const ProfileEditor = ({ }; // To make AvatarEditor re-render less often - const handleAvatarLoaded = useCallback(avatar => { - setAvatarBuffer(avatar); - }, []); + const handleAvatarLoaded = useCallback( + avatar => { + setAvatarBuffer(avatar); + setOldAvatarBuffer(avatar); + setIsLoadingAvatar(false); + }, + [setAvatarBuffer, setOldAvatarBuffer, setIsLoadingAvatar] + ); let content: JSX.Element; @@ -415,7 +432,7 @@ export const ProfileEditor = ({ content = ( @@ -513,8 +534,9 @@ export const ProfileEditor = ({ ); } else if (editState === EditState.Bio) { const shouldDisableSave = - stagedProfile.aboutText === fullBio.aboutText && - stagedProfile.aboutEmoji === fullBio.aboutEmoji; + isLoadingAvatar || + (stagedProfile.aboutText === fullBio.aboutText && + stagedProfile.aboutEmoji === fullBio.aboutEmoji); content = ( <> @@ -613,7 +635,10 @@ export const ProfileEditor = ({ aboutText: stagedProfile.aboutText, }); - onProfileChanged(stagedProfile, avatarBuffer); + onProfileChanged(stagedProfile, { + oldAvatar: oldAvatarBuffer, + newAvatar: avatarBuffer, + }); handleBack(); }} > @@ -689,7 +714,7 @@ export const ProfileEditor = ({ <> unknown; toggleProfileEditor: () => unknown; toggleProfileEditorHasError: () => unknown; diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index c382f9336..8d79cec48 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -1812,6 +1812,7 @@ export class ConversationModel extends window.Backbone avatarPath: this.getAbsoluteAvatarPath(), avatarHash: this.getAvatarHash(), unblurredAvatarPath: this.getAbsoluteUnblurredAvatarPath(), + profileAvatarPath: this.getAbsoluteProfileAvatarPath(), color, conversationColor: this.getConversationColor(), customColor, @@ -4947,6 +4948,11 @@ export class ConversationModel extends window.Backbone return avatarPath ? getAbsoluteAttachmentPath(avatarPath) : undefined; } + getAbsoluteProfileAvatarPath(): string | undefined { + const avatarPath = this.get('profileAvatar')?.path; + return avatarPath ? getAbsoluteAttachmentPath(avatarPath) : undefined; + } + getAbsoluteUnblurredAvatarPath(): string | undefined { const unblurredAvatarPath = this.get('unblurredAvatarPath'); return unblurredAvatarPath diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index 73197a5a3..8c3d057c8 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -178,7 +178,10 @@ export function toAccountRecord( if (conversation.get('profileFamilyName')) { accountRecord.familyName = conversation.get('profileFamilyName') || ''; } - accountRecord.avatarUrl = window.storage.get('avatarUrl') || ''; + const avatarUrl = window.storage.get('avatarUrl'); + if (avatarUrl !== undefined) { + accountRecord.avatarUrl = avatarUrl; + } accountRecord.noteToSelfArchived = Boolean(conversation.get('isArchived')); accountRecord.noteToSelfMarkedUnread = Boolean( conversation.get('markedUnread') @@ -895,7 +898,6 @@ export async function mergeAccountRecord( ): Promise { let details = new Array(); const { - avatarUrl, linkPreviews, notDiscoverableByPhoneNumber, noteToSelfArchived, @@ -1146,10 +1148,9 @@ export async function mergeAccountRecord( { viaStorageServiceSync: true } ); - if (avatarUrl) { - await conversation.setProfileAvatar(avatarUrl, profileKey); - window.storage.put('avatarUrl', avatarUrl); - } + const avatarUrl = dropNull(accountRecord.avatarUrl); + await conversation.setProfileAvatar(avatarUrl, profileKey); + window.storage.put('avatarUrl', avatarUrl); } const { hasConflict, details: extraDetails } = doesRecordHavePendingChanges( diff --git a/ts/services/writeProfile.ts b/ts/services/writeProfile.ts index 5f85b96f4..debce1dd4 100644 --- a/ts/services/writeProfile.ts +++ b/ts/services/writeProfile.ts @@ -11,10 +11,11 @@ import { getProfile } from '../util/getProfile'; import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue'; import { strictAssert } from '../util/assert'; import { isWhitespace } from '../util/whitespaceStringUtil'; +import type { AvatarUpdateType } from '../types/Avatar'; export async function writeProfile( conversation: ConversationType, - avatarBuffer?: Uint8Array + avatar: AvatarUpdateType ): Promise { // Before we write anything we request the user's profile so that we can // have an up-to-date paymentAddress to be able to include it when we write @@ -41,7 +42,7 @@ export async function writeProfile( const [profileData, encryptedAvatarData] = await encryptProfileData( conversation, - avatarBuffer + avatar ); const avatarRequestHeaders = await window.textsecure.messaging.putProfile( profileData @@ -50,37 +51,49 @@ export async function writeProfile( // Upload the avatar if provided // delete existing files on disk if avatar has been removed // update the account's avatar path and hash if it's a new avatar - let profileAvatar: - | { - hash: string; - path: string; - } - | undefined; - if (avatarRequestHeaders && encryptedAvatarData && avatarBuffer) { - await window.textsecure.messaging.uploadAvatar( + const { newAvatar } = avatar; + let maybeProfileAvatarUpdate: { + profileAvatar?: + | { + hash: string; + path: string; + } + | undefined; + } = {}; + if (profileData.sameAvatar) { + log.info('writeProfile: not updating avatar'); + } else if (avatarRequestHeaders && encryptedAvatarData && newAvatar) { + log.info('writeProfile: uploading new avatar'); + const avatarUrl = await window.textsecure.messaging.uploadAvatar( avatarRequestHeaders, encryptedAvatarData ); - const hash = await computeHash(avatarBuffer); + const hash = await computeHash(newAvatar); if (hash !== avatarHash) { + log.info('writeProfile: removing old avatar and saving the new one'); const [path] = await Promise.all([ - window.Signal.Migrations.writeNewAttachmentData(avatarBuffer), + window.Signal.Migrations.writeNewAttachmentData(newAvatar), avatarPath ? window.Signal.Migrations.deleteAttachmentData(avatarPath) : undefined, ]); - profileAvatar = { - hash, - path, + maybeProfileAvatarUpdate = { + profileAvatar: { hash, path }, }; } - } else if (avatarPath) { - await window.Signal.Migrations.deleteAttachmentData(avatarPath); - } - const profileAvatarData = profileAvatar ? { profileAvatar } : {}; + await window.storage.put('avatarUrl', avatarUrl); + } else if (avatarPath) { + log.info('writeProfile: removing avatar'); + await Promise.all([ + window.Signal.Migrations.deleteAttachmentData(avatarPath), + window.storage.put('avatarUrl', undefined), + ]); + + maybeProfileAvatarUpdate = { profileAvatar: undefined }; + } // Update backbone, update DB, run storage service upload model.set({ @@ -88,7 +101,7 @@ export async function writeProfile( aboutEmoji, profileName: firstName, profileFamilyName: familyName, - ...profileAvatarData, + ...maybeProfileAvatarUpdate, }); dataInterface.updateConversation(model.attributes); diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 50833c753..6b1025a21 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -63,7 +63,7 @@ import { getMe, getUsernameSaveState, } from '../selectors/conversations'; -import type { AvatarDataType } from '../../types/Avatar'; +import type { AvatarDataType, AvatarUpdateType } from '../../types/Avatar'; import { getDefaultAvatars } from '../../types/Avatar'; import { getAvatarData } from '../../util/getAvatarData'; import { isSameAvatarData } from '../../util/isSameAvatarData'; @@ -121,6 +121,7 @@ export type ConversationType = { avatars?: Array; avatarPath?: string; avatarHash?: string; + profileAvatarPath?: string; unblurredAvatarPath?: string; areWeAdmin?: boolean; areWePending?: boolean; @@ -1094,7 +1095,7 @@ function saveUsername({ function myProfileChanged( profileData: ProfileDataType, - avatarBuffer?: Uint8Array + avatar: AvatarUpdateType ): ThunkAction< void, RootStateType, @@ -1110,7 +1111,7 @@ function myProfileChanged( ...conversation, ...profileData, }, - avatarBuffer + avatar ); // writeProfile above updates the backbone model which in turn updates diff --git a/ts/state/smart/ProfileEditorModal.ts b/ts/state/smart/ProfileEditorModal.ts index fb995e82d..c68a1fdd4 100644 --- a/ts/state/smart/ProfileEditorModal.ts +++ b/ts/state/smart/ProfileEditorModal.ts @@ -17,7 +17,7 @@ function mapStateToProps( ): Omit & ProfileEditorModalPropsType { const { - avatarPath, + profileAvatarPath, avatars: userAvatarData = [], aboutText, aboutEmoji, @@ -34,7 +34,7 @@ function mapStateToProps( return { aboutEmoji, aboutText, - avatarPath, + profileAvatarPath, color, conversationId, familyName, diff --git a/ts/test-electron/util/encryptProfileData_test.ts b/ts/test-electron/util/encryptProfileData_test.ts index 95f894949..61f5d5204 100644 --- a/ts/test-electron/util/encryptProfileData_test.ts +++ b/ts/test-electron/util/encryptProfileData_test.ts @@ -10,13 +10,17 @@ import { decryptProfileName, decryptProfile, } from '../../Crypto'; +import type { ConversationType } from '../../state/ducks/conversations'; import { UUID } from '../../types/UUID'; import { encryptProfileData } from '../../util/encryptProfileData'; describe('encryptProfileData', () => { - it('encrypts and decrypts properly', async () => { - const keyBuffer = getRandomBytes(32); - const conversation = { + let keyBuffer: Uint8Array; + let conversation: ConversationType; + + beforeEach(() => { + keyBuffer = getRandomBytes(32); + conversation = { aboutEmoji: '🐢', aboutText: 'I like turtles', familyName: 'Kid', @@ -33,8 +37,13 @@ describe('encryptProfileData', () => { title: '', type: 'direct' as const, }; + }); - const [encrypted] = await encryptProfileData(conversation); + it('encrypts and decrypts properly', async () => { + const [encrypted] = await encryptProfileData(conversation, { + oldAvatar: undefined, + newAvatar: undefined, + }); assert.isDefined(encrypted.version); assert.isDefined(encrypted.name); @@ -83,4 +92,22 @@ describe('encryptProfileData', () => { assert.isDefined(encrypted.aboutEmoji); } }); + + it('sets sameAvatar to true when avatars are the same', async () => { + const [encrypted] = await encryptProfileData(conversation, { + oldAvatar: new Uint8Array([1, 2, 3]), + newAvatar: new Uint8Array([1, 2, 3]), + }); + + assert.isTrue(encrypted.sameAvatar); + }); + + it('sets sameAvatar to false when avatars are different', async () => { + const [encrypted] = await encryptProfileData(conversation, { + oldAvatar: new Uint8Array([1, 2, 3]), + newAvatar: new Uint8Array([4, 5, 6, 7]), + }); + + assert.isFalse(encrypted.sameAvatar); + }); }); diff --git a/ts/textsecure/WebAPI.ts b/ts/textsecure/WebAPI.ts index 3f7753c01..f25b0c91a 100644 --- a/ts/textsecure/WebAPI.ts +++ b/ts/textsecure/WebAPI.ts @@ -701,6 +701,7 @@ export type ProfileRequestDataType = { about: string | null; aboutEmoji: string | null; avatar: boolean; + sameAvatar: boolean; commitment: string; name: string; paymentAddress: string | null; diff --git a/ts/types/Avatar.ts b/ts/types/Avatar.ts index eadd7da9f..d4c4151b5 100644 --- a/ts/types/Avatar.ts +++ b/ts/types/Avatar.ts @@ -65,6 +65,11 @@ export type SaveAvatarToDiskActionType = ( conversationId?: string ) => unknown; +export type AvatarUpdateType = Readonly<{ + oldAvatar: Uint8Array | undefined; + newAvatar: Uint8Array | undefined; +}>; + const groupIconColors = [ 'A180', 'A120', diff --git a/ts/types/Storage.d.ts b/ts/types/Storage.d.ts index ad4e9f1fc..76914b334 100644 --- a/ts/types/Storage.d.ts +++ b/ts/types/Storage.d.ts @@ -107,7 +107,7 @@ export type StorageAccessType = { typingIndicators: boolean; sealedSenderIndicators: boolean; storageFetchComplete: boolean; - avatarUrl: string; + avatarUrl: string | undefined; manifestVersion: number; storageCredentials: StorageServiceCredentials; 'storage-service-error-records': Array; diff --git a/ts/util/encryptProfileData.ts b/ts/util/encryptProfileData.ts index 35932baa6..16b8d3584 100644 --- a/ts/util/encryptProfileData.ts +++ b/ts/util/encryptProfileData.ts @@ -10,11 +10,12 @@ import { encryptProfile, encryptProfileItemWithPadding, } from '../Crypto'; +import type { AvatarUpdateType } from '../types/Avatar'; import { deriveProfileKeyCommitment, deriveProfileKeyVersion } from './zkgroup'; export async function encryptProfileData( conversation: ConversationType, - avatarBuffer?: Uint8Array + { oldAvatar, newAvatar }: AvatarUpdateType ): Promise<[ProfileRequestDataType, Uint8Array | undefined]> { const { aboutEmoji, @@ -55,10 +56,12 @@ export async function encryptProfileData( ) : null; - const encryptedAvatarData = avatarBuffer - ? encryptProfile(avatarBuffer, keyBuffer) + const encryptedAvatarData = newAvatar + ? encryptProfile(newAvatar, keyBuffer) : undefined; + const sameAvatar = Bytes.areEqual(oldAvatar, newAvatar); + const profileData = { version: deriveProfileKeyVersion(profileKey, uuid), name: Bytes.toBase64(bytesName), @@ -66,7 +69,8 @@ export async function encryptProfileData( aboutEmoji: bytesAboutEmoji ? Bytes.toBase64(bytesAboutEmoji) : null, badgeIds: (badges || []).map(({ id }) => id), paymentAddress: window.storage.get('paymentAddress') || null, - avatar: Boolean(avatarBuffer), + avatar: Boolean(newAvatar), + sameAvatar, commitment: deriveProfileKeyCommitment(profileKey, uuid), }; diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index 54f1e055c..48ce40876 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -7226,13 +7226,6 @@ "reasonCategory": "falseMatch", "updated": "2021-05-05T23:11:22.692Z" }, - { - "rule": "React-useRef", - "path": "ts/components/AvatarPreview.tsx", - "line": " const startingAvatarPathRef = useRef(", - "reasonCategory": "usageTrusted", - "updated": "2021-08-03T21:17:38.615Z" - }, { "rule": "React-useRef", "path": "ts/components/AvatarTextEditor.tsx", @@ -8079,4 +8072,4 @@ "reasonCategory": "usageTrusted", "updated": "2021-09-17T21:02:59.414Z" } -] \ No newline at end of file +]