From d8708e4e73bcc58d9f18ca4a535e4c9bc55bc489 Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Mon, 25 Apr 2022 13:25:50 -0400 Subject: [PATCH] Ensure that we resolve attachments before displaying them --- stylesheets/components/StoryImage.scss | 2 +- ts/components/StoryImage.tsx | 18 +- ts/components/StoryViewer.tsx | 5 + ts/components/TextAttachment.stories.tsx | 15 +- ts/components/TextAttachment.tsx | 40 +++-- ts/state/ducks/stories.ts | 77 +++++++- ts/test-electron/state/ducks/stories_test.ts | 178 +++++++++++++++++++ ts/types/Attachment.ts | 2 +- ts/util/getStoryDuration.ts | 13 +- 9 files changed, 319 insertions(+), 31 deletions(-) create mode 100644 ts/test-electron/state/ducks/stories_test.ts diff --git a/stylesheets/components/StoryImage.scss b/stylesheets/components/StoryImage.scss index a0a43b75d703..c135a43ff361 100644 --- a/stylesheets/components/StoryImage.scss +++ b/stylesheets/components/StoryImage.scss @@ -22,7 +22,7 @@ width: 100%; } - &__spinner-container { + &__overlay-container { align-items: center; display: flex; height: 100%; diff --git a/ts/components/StoryImage.tsx b/ts/components/StoryImage.tsx index ffc370f2b636..2797cf233bbb 100644 --- a/ts/components/StoryImage.tsx +++ b/ts/components/StoryImage.tsx @@ -40,7 +40,9 @@ export const StoryImage = ({ storyId, }: PropsType): JSX.Element | null => { const shouldDownloadAttachment = - !isDownloaded(attachment) && !isDownloading(attachment); + !isDownloaded(attachment) && + !isDownloading(attachment) && + !hasNotResolved(attachment); useEffect(() => { if (shouldDownloadAttachment) { @@ -61,7 +63,11 @@ export const StoryImage = ({ let storyElement: JSX.Element; if (attachment.textAttachment) { storyElement = ( - + ); } else if (isNotReadyToShow) { storyElement = ( @@ -98,10 +104,10 @@ export const StoryImage = ({ ); } - let spinner: JSX.Element | undefined; + let overlay: JSX.Element | undefined; if (isPending) { - spinner = ( -
+ overlay = ( +
@@ -117,7 +123,7 @@ export const StoryImage = ({ )} > {storyElement} - {spinner} + {overlay}
); }; diff --git a/ts/components/StoryViewer.tsx b/ts/components/StoryViewer.tsx index be95a03b3813..54bc898de7f5 100644 --- a/ts/components/StoryViewer.tsx +++ b/ts/components/StoryViewer.tsx @@ -198,6 +198,11 @@ export const StoryViewer = ({ // We need to be careful about this effect refreshing, it should only run // every time a story changes or its duration changes. useEffect(() => { + if (!storyDuration) { + spring.stop(); + return; + } + spring.start({ config: { duration: storyDuration, diff --git a/ts/components/TextAttachment.stories.tsx b/ts/components/TextAttachment.stories.tsx index 675d7f64fd9b..650d94851845 100644 --- a/ts/components/TextAttachment.stories.tsx +++ b/ts/components/TextAttachment.stories.tsx @@ -164,7 +164,20 @@ story.add('Link preview', () => ( preview: { url: 'https://www.signal.org/workworkwork', title: 'Signal >> Careers', - // TODO add image + }, + }} + /> +)); + +story.add('Link preview (thumbnail)', () => ( + > Careers', }, }} /> diff --git a/ts/components/TextAttachment.tsx b/ts/components/TextAttachment.tsx index 6a10a1dac930..f30f23e7fd74 100644 --- a/ts/components/TextAttachment.tsx +++ b/ts/components/TextAttachment.tsx @@ -40,6 +40,7 @@ enum TextSize { export type PropsType = { i18n: LocalizerType; + isThumbnail?: boolean; textAttachment: TextAttachmentType; }; @@ -85,6 +86,7 @@ function getFont( export const TextAttachment = ({ i18n, + isThumbnail, textAttachment, }: PropsType): JSX.Element | null => { const linkPreview = useRef(null); @@ -149,25 +151,27 @@ export const TextAttachment = ({ )} {textAttachment.preview && ( <> - {linkPreviewOffsetTop && textAttachment.preview.url && ( - -
-
{i18n('TextAttachment__preview__link')}
-
-
- - )} +
+ + )}
{ +): ThunkAction< + void, + RootStateType, + unknown, + NoopActionType | ResolveAttachmentUrlActionType +> { return async dispatch => { const story = await getMessageById(storyId); @@ -226,6 +245,25 @@ function queueStoryDownload( } if (isDownloaded(attachment)) { + if (!attachment.path) { + return; + } + + // This function also resolves the attachment's URL in case we've already + // downloaded the attachment but haven't pointed its path to an absolute + // location on disk. + if (hasNotResolved(attachment)) { + dispatch({ + type: RESOLVE_ATTACHMENT_URL, + payload: { + messageId: storyId, + attachmentUrl: window.Signal.Migrations.getAbsoluteAttachmentPath( + attachment.path + ), + }, + }); + } + return; } @@ -500,5 +538,40 @@ export function reducer( }; } + if (action.type === RESOLVE_ATTACHMENT_URL) { + const { messageId, attachmentUrl } = action.payload; + + const storyIndex = state.stories.findIndex( + existingStory => existingStory.messageId === messageId + ); + + if (storyIndex < 0) { + return state; + } + + const story = state.stories[storyIndex]; + + if (!story.attachment) { + return state; + } + + const storyWithResolvedAttachment = { + ...story, + attachment: { + ...story.attachment, + url: attachmentUrl, + }, + }; + + return { + ...state, + stories: replaceIndex( + state.stories, + storyIndex, + storyWithResolvedAttachment + ), + }; + } + return state; } diff --git a/ts/test-electron/state/ducks/stories_test.ts b/ts/test-electron/state/ducks/stories_test.ts new file mode 100644 index 000000000000..a769bf6dec1f --- /dev/null +++ b/ts/test-electron/state/ducks/stories_test.ts @@ -0,0 +1,178 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import * as sinon from 'sinon'; +import path from 'path'; +import { assert } from 'chai'; +import { v4 as uuid } from 'uuid'; + +import type { StoriesStateType } from '../../../state/ducks/stories'; +import type { MessageAttributesType } from '../../../model-types.d'; +import { IMAGE_JPEG } from '../../../types/MIME'; +import { + actions, + getEmptyState, + reducer, + RESOLVE_ATTACHMENT_URL, +} from '../../../state/ducks/stories'; +import { noopAction } from '../../../state/ducks/noop'; +import { reducer as rootReducer } from '../../../state/reducer'; + +describe('both/state/ducks/stories', () => { + const getEmptyRootState = () => ({ + ...rootReducer(undefined, noopAction()), + stories: getEmptyState(), + }); + + function getStoryMessage(id: string): MessageAttributesType { + const now = Date.now(); + + return { + conversationId: uuid(), + id, + received_at: now, + sent_at: now, + timestamp: now, + type: 'story', + }; + } + + describe('queueStoryDownload', () => { + const { queueStoryDownload } = actions; + + it('no attachment, no dispatch', async function test() { + const storyId = uuid(); + const messageAttributes = getStoryMessage(storyId); + + window.MessageController.register(storyId, messageAttributes); + + const dispatch = sinon.spy(); + await queueStoryDownload(storyId)(dispatch, getEmptyRootState, null); + + sinon.assert.notCalled(dispatch); + }); + + it('downloading, no dispatch', async function test() { + const storyId = uuid(); + const messageAttributes = { + ...getStoryMessage(storyId), + attachments: [ + { + contentType: IMAGE_JPEG, + downloadJobId: uuid(), + pending: true, + size: 0, + }, + ], + }; + + window.MessageController.register(storyId, messageAttributes); + + const dispatch = sinon.spy(); + await queueStoryDownload(storyId)(dispatch, getEmptyRootState, null); + + sinon.assert.notCalled(dispatch); + }); + + it('downloaded, no dispatch', async function test() { + const storyId = uuid(); + const messageAttributes = { + ...getStoryMessage(storyId), + attachments: [ + { + contentType: IMAGE_JPEG, + path: 'image.jpg', + url: '/path/to/image.jpg', + size: 0, + }, + ], + }; + + window.MessageController.register(storyId, messageAttributes); + + const dispatch = sinon.spy(); + await queueStoryDownload(storyId)(dispatch, getEmptyRootState, null); + + sinon.assert.notCalled(dispatch); + }); + + it('downloaded, but unresolved, we should resolve the path', async function test() { + const storyId = uuid(); + const attachment = { + contentType: IMAGE_JPEG, + path: 'image.jpg', + size: 0, + }; + const messageAttributes = { + ...getStoryMessage(storyId), + attachments: [attachment], + }; + + window.MessageController.register(storyId, messageAttributes); + + const dispatch = sinon.spy(); + await queueStoryDownload(storyId)(dispatch, getEmptyRootState, null); + + const action = dispatch.getCall(0).args[0]; + + sinon.assert.calledWith(dispatch, { + type: RESOLVE_ATTACHMENT_URL, + payload: { + messageId: storyId, + attachmentUrl: action.payload.attachmentUrl, + }, + }); + assert.equal( + attachment.path, + path.basename(action.payload.attachmentUrl) + ); + + const stateWithStory: StoriesStateType = { + ...getEmptyRootState().stories, + stories: [ + { + ...messageAttributes, + messageId: storyId, + attachment, + }, + ], + }; + + const nextState = reducer(stateWithStory, action); + assert.isDefined(nextState.stories); + assert.equal( + nextState.stories[0].attachment?.url, + action.payload.attachmentUrl + ); + + const state = getEmptyRootState().stories; + + const sameState = reducer(state, action); + assert.isDefined(sameState.stories); + assert.equal(sameState, state); + }); + + it('not downloaded, queued for download', async function test() { + const storyId = uuid(); + const messageAttributes = { + ...getStoryMessage(storyId), + attachments: [ + { + contentType: IMAGE_JPEG, + size: 0, + }, + ], + }; + + window.MessageController.register(storyId, messageAttributes); + + const dispatch = sinon.spy(); + await queueStoryDownload(storyId)(dispatch, getEmptyRootState, null); + + sinon.assert.calledWith(dispatch, { + type: 'NOOP', + payload: null, + }); + }); + }); +}); diff --git a/ts/types/Attachment.ts b/ts/types/Attachment.ts index 8a5d001ee315..f3bf88edc2b6 100644 --- a/ts/types/Attachment.ts +++ b/ts/types/Attachment.ts @@ -730,7 +730,7 @@ export function isDownloaded(attachment?: AttachmentType): boolean { } export function hasNotResolved(attachment?: AttachmentType): boolean { - return Boolean(attachment && !attachment.url); + return Boolean(attachment && !attachment.url && !attachment.textAttachment); } export function isDownloading(attachment?: AttachmentType): boolean { diff --git a/ts/util/getStoryDuration.ts b/ts/util/getStoryDuration.ts index 43c9b6b807cf..7d6fda9b0f2b 100644 --- a/ts/util/getStoryDuration.ts +++ b/ts/util/getStoryDuration.ts @@ -2,7 +2,12 @@ // SPDX-License-Identifier: AGPL-3.0-only import type { AttachmentType } from '../types/Attachment'; -import { isGIF, isVideo } from '../types/Attachment'; +import { + hasNotResolved, + isDownloaded, + isGIF, + isVideo, +} from '../types/Attachment'; import { count } from './grapheme'; import { SECOND } from './durations'; @@ -12,7 +17,11 @@ const MIN_TEXT_DURATION = 3 * SECOND; export async function getStoryDuration( attachment: AttachmentType -): Promise { +): Promise { + if (!isDownloaded(attachment) || hasNotResolved(attachment)) { + return; + } + if (isGIF([attachment]) || isVideo([attachment])) { const videoEl = document.createElement('video'); if (!attachment.url) {