Skip to content

Fix attachment modal for broken images #61459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9 changes: 7 additions & 2 deletions src/components/AttachmentModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import viewRef from '@src/types/utils/viewRef';
import AttachmentCarousel from './Attachments/AttachmentCarousel';
import AttachmentCarouselPagerContext from './Attachments/AttachmentCarousel/Pager/AttachmentCarouselPagerContext';
import AttachmentView from './Attachments/AttachmentView';
import useAttachmentErrors from './Attachments/AttachmentView/useAttachmentErrors';
import type {Attachment} from './Attachments/types';
import BlockingView from './BlockingViews/BlockingView';
import Button from './Button';
Expand Down Expand Up @@ -213,6 +214,7 @@ function AttachmentModal({
const transactionID = (isMoneyRequestAction(parentReportAction) && getOriginalMessage(parentReportAction)?.IOUTransactionID) || CONST.DEFAULT_NUMBER_ID;
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {canBeMissing: true});
const [currentAttachmentLink, setCurrentAttachmentLink] = useState(attachmentLink);
const {setAttachmentError, isErrorInAttachment, clearAttachmentErrors} = useAttachmentErrors();

const [file, setFile] = useState<FileObject | undefined>(
originalFileName
Expand Down Expand Up @@ -490,7 +492,7 @@ function AttachmentModal({
const headerTitleNew = headerTitle ?? translate(isReceiptAttachment ? 'common.receipt' : 'common.attachment');
const shouldShowThreeDotsButton = isReceiptAttachment && isModalOpen && threeDotsMenuItems.length !== 0;
let shouldShowDownloadButton = false;
if (!isEmptyObject(report) || type === CONST.ATTACHMENT_TYPE.SEARCH) {
if ((!isEmptyObject(report) || type === CONST.ATTACHMENT_TYPE.SEARCH) && !isErrorInAttachment(sourceState)) {
shouldShowDownloadButton = allowDownload && isDownloadButtonReadyToBeShown && !shouldShowNotFoundPage && !isReceiptAttachment && !isOffline && !isLocalSource;
}
const context = useMemo(
Expand All @@ -503,8 +505,9 @@ function AttachmentModal({
onTap: () => {},
onScaleChanged: () => {},
onSwipeDown: closeModal,
onAttachmentError: setAttachmentError,
}),
[closeModal, nope, sourceForAttachmentView],
[closeModal, setAttachmentError, nope, sourceForAttachmentView],
);

const submitRef = useRef<View | HTMLElement>(null);
Expand All @@ -524,6 +527,7 @@ function AttachmentModal({
onModalHide();
}
setShouldLoadAttachment(false);
clearAttachmentErrors();
if (isPDFLoadError.current) {
setIsAttachmentInvalid(true);
setAttachmentInvalidReasonTitle('attachmentPicker.attachmentError');
Expand Down Expand Up @@ -603,6 +607,7 @@ function AttachmentModal({
source={source}
setDownloadButtonVisibility={setDownloadButtonVisibility}
attachmentLink={currentAttachmentLink}
onAttachmentError={setAttachmentError}
/>
) : (
!!sourceForAttachmentView &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,12 @@ type AttachmentCarouselPagerContextValue = {

/** Function to call after a swipe down event */
onSwipeDown: () => void;

/** Callback for attachment errors */
onAttachmentError?: (source: AttachmentSource, state?: boolean) => void;
};

const AttachmentCarouselPagerContext = createContext<AttachmentCarouselPagerContextValue | null>(null);

export default AttachmentCarouselPagerContext;
export type {AttachmentCarouselPagerContextValue};
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

const WrappedPagerView = createNativeWrapper(PagerView) as React.ForwardRefExoticComponent<
PagerViewProps &
NativeViewGestureHandlerProps &

Check failure on line 20 in src/components/Attachments/AttachmentCarousel/Pager/index.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

'NativeViewGestureHandlerProps' is deprecated. NativeViewGestureHandler will be removed in the future version of Gesture Handler. Use `Gesture.Native()` instead
React.RefAttributes<React.Component<PagerViewProps>> & {
useNext: boolean;
}
Expand Down Expand Up @@ -55,10 +55,13 @@

/** The reportID related to the attachment */
reportID?: string;

/** Callback for attachment errors */
onAttachmentError?: (source: AttachmentSource) => void;
};

function AttachmentCarouselPager(
{items, activeAttachmentID, initialPage, setShouldShowArrows, onPageSelected, onClose, reportID}: AttachmentCarouselPagerProps,
{items, activeAttachmentID, initialPage, setShouldShowArrows, onPageSelected, onClose, reportID, onAttachmentError}: AttachmentCarouselPagerProps,
ref: ForwardedRef<AttachmentCarouselPagerHandle>,
) {
const {handleTap, handleScaleChange, isScrollEnabled} = useCarouselContextEvents(setShouldShowArrows);
Expand Down Expand Up @@ -100,8 +103,9 @@
onTap: handleTap,
onSwipeDown: onClose,
onScaleChanged: handleScaleChange,
onAttachmentError,
}),
[pagerItems, activePageIndex, isPagerScrolling, isScrollEnabled, handleTap, onClose, handleScaleChange],
[pagerItems, activePageIndex, isPagerScrolling, isScrollEnabled, handleTap, onClose, handleScaleChange, onAttachmentError],
);

const animatedProps = useAnimatedProps(() => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ import AttachmentCarouselPager from './Pager';
import type {AttachmentCarouselProps} from './types';
import useCarouselArrows from './useCarouselArrows';

function AttachmentCarousel({report, source, attachmentID, onNavigate, setDownloadButtonVisibility, onClose, type, accountID}: AttachmentCarouselProps) {
function AttachmentCarousel({report, source, attachmentID, onNavigate, setDownloadButtonVisibility, onClose, type, accountID, onAttachmentError}: AttachmentCarouselProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const pagerRef = useRef<AttachmentCarouselPagerHandle>(null);
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false});
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false});
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false, canBeMissing: true});
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false, canBeMissing: true});
const [page, setPage] = useState<number>();
const [attachments, setAttachments] = useState<Attachment[]>([]);
const {shouldShowArrows, setShouldShowArrows, autoHideArrows, cancelAutoHideArrows} = useCarouselArrows();
Expand Down Expand Up @@ -144,6 +144,7 @@ function AttachmentCarousel({report, source, attachmentID, onNavigate, setDownlo
<AttachmentCarouselPager
items={attachments}
initialPage={page}
onAttachmentError={onAttachmentError}
activeAttachmentID={activeAttachmentID}
setShouldShowArrows={setShouldShowArrows}
onPageSelected={({nativeEvent: {position: newPage}}) => updatePage(newPage)}
Expand Down
9 changes: 5 additions & 4 deletions src/components/Attachments/AttachmentCarousel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function DeviceAwareGestureDetector({canUseTouchScreen, gesture, children}: Devi
return canUseTouchScreen ? <GestureDetector gesture={gesture}>{children}</GestureDetector> : children;
}

function AttachmentCarousel({report, attachmentID, source, onNavigate, setDownloadButtonVisibility, type, accountID, onClose, attachmentLink}: AttachmentCarouselProps) {
function AttachmentCarousel({report, attachmentID, source, onNavigate, setDownloadButtonVisibility, type, accountID, onClose, attachmentLink, onAttachmentError}: AttachmentCarouselProps) {
const theme = useTheme();
const {translate} = useLocalize();
const {windowWidth} = useWindowDimensions();
Expand All @@ -61,8 +61,8 @@ function AttachmentCarousel({report, attachmentID, source, onNavigate, setDownlo
const scrollRef = useAnimatedRef<Animated.FlatList<ListRenderItemInfo<Attachment>>>();
const isPagerScrolling = useSharedValue(false);
const pagerRef = useRef<GestureType>(null);
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false});
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false});
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false, canBeMissing: true});
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false, canBeMissing: true});
const canUseTouchScreen = canUseTouchScreenUtil();

const modalStyles = styles.centeredModalStyles(shouldUseNarrowLayout, true);
Expand Down Expand Up @@ -226,8 +226,9 @@ function AttachmentCarousel({report, attachmentID, source, onNavigate, setDownlo
onTap: handleTap,
onScaleChanged: handleScaleChange,
onSwipeDown: onClose,
onAttachmentError,
}),
[source, isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange, onClose],
[onAttachmentError, source, isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange, onClose],
);

/** Defines how a single attachment should be rendered */
Expand Down
3 changes: 3 additions & 0 deletions src/components/Attachments/AttachmentCarousel/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ type AttachmentCarouselProps = {
onClose: () => void;

attachmentLink?: string;

/** Callback for attachment errors */
onAttachmentError?: (source: AttachmentSource, state?: boolean) => void;
};

export type {AttachmentCarouselProps, UpdatePageProps};
14 changes: 12 additions & 2 deletions src/components/Attachments/AttachmentView/index.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {Str} from 'expensify-common';
import React, {memo, useEffect, useState} from 'react';
import React, {memo, useContext, useEffect, useState} from 'react';
import type {GestureResponderEvent, ImageURISource, StyleProp, ViewStyle} from 'react-native';
import {View} from 'react-native';
import AttachmentCarouselPagerContext from '@components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPagerContext';
import type {Attachment, AttachmentSource} from '@components/Attachments/types';
import DistanceEReceipt from '@components/DistanceEReceipt';
import EReceipt from '@components/EReceipt';
Expand Down Expand Up @@ -119,10 +120,12 @@ function AttachmentView({
isUploading = false,
reportID,
}: AttachmentViewProps) {
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {canBeMissing: true});
const {translate} = useLocalize();
const {updateCurrentURLAndReportID} = usePlaybackContext();

const attachmentCarouselPagerContext = useContext(AttachmentCarouselPagerContext);
const {onAttachmentError} = attachmentCarouselPagerContext ?? {};
const theme = useTheme();
const {safeAreaPaddingBottomStyle} = useSafeAreaPaddings();
const styles = useThemeStyles();
Expand Down Expand Up @@ -151,6 +154,12 @@ function AttachmentView({
});
}, [file]);

useEffect(() => {
const isImageSource = typeof source !== 'function' && !!checkIsFileImage(source, file?.name);
const isErrorInImage = imageError && (typeof fallbackSource === 'number' || typeof fallbackSource === 'function');
onAttachmentError?.(source, isErrorInImage && isImageSource);
}, [fallbackSource, file?.name, imageError, onAttachmentError, source]);

// Handles case where source is a component (ex: SVG) or a number
// Number may represent a SVG or an image
if (typeof source === 'function' || (maybeIcon && typeof source === 'number')) {
Expand Down Expand Up @@ -294,6 +303,7 @@ function AttachmentView({
if (isOffline) {
return;
}

setImageError(true);
}}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import {useCallback, useState} from 'react';
import type {AttachmentSource} from '@components/Attachments/types';

function convertSourceToString(source: AttachmentSource) {
if (typeof source === 'string' || typeof source === 'number') {
return source.toString();
}
if (source instanceof Array) {
return source.map((src) => src.uri).join(', ');
}
if ('uri' in source) {
return source.uri ?? '';
}

return '';
}

/**
* A custom React hook that provides functionalities to manage attachment errors.
* - `setAttachmentError(key)`: Sets or unsets an error for a given key.
* - `clearAttachmentErrors()`: Clears all errors.
* - `isErrorInAttachment(key)`: Checks if there is an error associated with a specific key.
* Errors are indexed by a serialized key - for example url or source object.
*/
function useAttachmentErrors() {
const [attachmentErrors, setAttachmentErrors] = useState<Record<string, boolean>>({});

const setAttachmentError = useCallback((key: AttachmentSource, state = true) => {
const url = convertSourceToString(key);
if (!url) {
return;
}
setAttachmentErrors((prevState) => ({
...prevState,
[url]: state,
}));
}, []);

const clearAttachmentErrors = useCallback(() => {
setAttachmentErrors({});
}, []);

const isErrorInAttachment = useCallback((key: AttachmentSource) => attachmentErrors?.[convertSourceToString(key)], [attachmentErrors]);

return {setAttachmentError, clearAttachmentErrors, isErrorInAttachment};
}

export default useAttachmentErrors;
Loading