[HOLD for payment 2024-06-06] [$250] Attachment - Blank preview when opened URL of deleted file

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.74-4
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://github.com/Expensify/Expensify/issues/313988
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition: use two Gmail accounts or 1 Gmail and 1 Expensifail,

1. From user A send an attachment to user B.
2. From user B open the attachment in preview, and copy the URL.
3. From user A delete the attachment and wait the user B attachment preview is dismissed.
4. User B: Open the copied URL.

Expected Result:

The app displays 'Hmm its not here' page.

Actual Result:

The app displays blank page.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6486224_1716210045654.attachment_preview_URL.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01aefa25ba0c7aa996
• Upwork Job ID: 1793751045977239552
• Last Price Increase: 2024-05-23
• Automatic offers:
• s77rt | Reviewer | 102487826
• bernhardoj | Contributor | 102487828

Issue Owner

Current Issue Owner: @slafortune / @MitchExpensify

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[dragnoir]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Blank preview when opened URL of deleted file

What is the root cause of that problem?

The AttachmentModal component, needs the prop shouldShowNotFoundPage to display the "Hmm... it's not here".
But in ReportAttachments we are not sending any details about when shouldShowNotFoundPage will be true!

In ReportAttachments we get the image source from the route

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/pages/home/report/ReportAttachments.tsx#L19C6-L19C71

and then we send this source to AttachmentModal. Any source wrong or correct will be send and displayed. If it's wrong, there will be just w white screen.

What changes do you think we should make in order to solve the problem?

We need to check if the source is a valid file and exists, we can use a combination of URL validation and an HTTP request to verify its existence. Here's how you can achieve this in your React Native web app:

1. Validate the URL format: Ensure the source string is a valid URL or file path.
2. Check the file existence: Use a fetch request to check if the file exists at the given URL.

Here's a code snippet that demonstrates these solution:

function ReportAttachments({route}: ReportAttachmentsProps) {
    const reportID = route.params.reportID;
    const report = ReportUtils.getReport(reportID);
    const [source, setSource] = useState<string | number | null>(null);
    const [isValidSource, setIsValidSource] = useState(false);

    useEffect(() => {
        // eslint-disable-next-line @lwc/lwc/no-async-await
        const validateAndCheckSource = async () => {
            const sourceToCheck = Number(route.params.source) || route.params.source;
            console.log('source: ', sourceToCheck);

            if (typeof sourceToCheck === 'string') {
                try {
                    const response = await fetch(sourceToCheck, {method: 'HEAD'});
                    if (response.ok) {
                        setSource(sourceToCheck);
                        setIsValidSource(true);
                    } else {
                        console.error('File does not exist at the given source.');
                        setIsValidSource(false);
                    }
                } catch (error) {
                    console.error('Error fetching the source:', error);
                    setIsValidSource(false);
                }
            } else {
                // Assuming native image source is valid
                setSource(sourceToCheck);
                setIsValidSource(true);
            }
        };

        validateAndCheckSource();
    }, [route.params.source]);

    const onCarouselAttachmentChange = useCallback(
        (attachment: Attachment) => {
            const routeToNavigate = ROUTES.REPORT_ATTACHMENTS.getRoute(reportID, String(attachment.source));
            Navigation.navigate(routeToNavigate);
        },
        [reportID],
    );

    return (
        <AttachmentModal
            allowDownload
            defaultOpen
            report={report}
            source={source}
            shouldShowNotFoundPage={!isValidSource}
            onModalHide={() => {
                Navigation.dismissModal();
                // This enables Composer refocus when the attachments modal is closed by the browser navigation
                ComposerFocusManager.setReadyToFocus();
            }}
            onCarouselAttachmentChange={onCarouselAttachmentChange}
        />
    );
}

ReportAttachments.displayName = 'ReportAttachments';

export default ReportAttachments;

Explanation:

1. State Initialization:

   • source holds the actual source to be used.
   • isValidSource tracks whether the source is valid and exists.

2. Effect Hook:

   • useEffect is used to validate and check the existence of the source when the component mounts or the source changes.
   • It uses a fetch request with the HEAD method to check if the file exists at the given URL without downloading the file.

3. Conditional Rendering:

   • If the source is invalid or the file doesn't exist, then true for shouldShowNotFoundPage
   • If the source is valid, it proceeds with rendering the AttachmentModal.

This approach ensures that your app checks the validity and existence of the file before attempting to use it.

POC Video:

20240521_180905.mp4

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The attachment carosel shows blank when opening a deleted attachment.

What is the root cause of that problem?

This issue only happens when we do it on a chat that doesn't have any attachment. The not found will show if the page state is -1.

App/src/components/Attachments/AttachmentCarousel/index.tsx

Lines 167 to 168 in e8ae3c5

	{page === -1 ? (
	<BlockingView

The initial value of page is 0,

App/src/components/Attachments/AttachmentCarousel/index.tsx

Line 42 in e8ae3c5

const [page, setPage] = useState(0);

and we initialize it inside this useEffect.

App/src/components/Attachments/AttachmentCarousel/index.tsx

Lines 49 to 64 in e8ae3c5

	useEffect(() => {
	const parentReportAction = report.parentReportActionID && parentReportActions ? parentReportActions[report.parentReportActionID] : undefined;
	const attachmentsFromReport = extractAttachmentsFromReport(parentReportAction, reportActions ?? undefined);
	if (isEqual(attachments, attachmentsFromReport)) {
	return;
	}
	const initialPage = attachmentsFromReport.findIndex(compareImage);
	// Dismiss the modal when deleting an attachment during its display in preview.
	if (initialPage === -1 && attachments.find(compareImage)) {
	Navigation.dismissModal();
	} else {
	setPage(initialPage);
	setAttachments(attachmentsFromReport);

initialPage will be -1 if the attachment isn't found in the chat.

However, it won't reach that code because attachments and attachmentsFromReport are equal, both are empty because the chat/report doesn't have any attachments, so the page is still 0 and the not found page won't ever show.

What changes do you think we should make in order to solve the problem?

If both attachments and attachmentsFromReport are empty, then we set the page to -1.

One way to do it is to check the array inside this if

App/src/components/Attachments/AttachmentCarousel/index.tsx

Lines 53 to 55 in e8ae3c5

	if (isEqual(attachments, attachmentsFromReport)) {
	return;
	}

if (attachments.length === 0) {
    setPage(-1);
}

If the attachments length is 0, then attachmentsFromReport must be 0 too.

we can check whether the page is already -1 or not before setting the page state too

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01aefa25ba0c7aa996

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[slafortune]

I'll be on vacation until June 4th so adding another BZ, I'll take this back on the 4th.

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[s77rt]

@dragnoir Thanks for the proposal. Your RCA is not correct. In AttachmentModal if we are viewing a report's attachment we will use AttachmentCarousel in which we have a another not found logic that should cover this case.

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct. The solution looks good to me 👍.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

@bondydaa, @slafortune, @s77rt, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[s77rt]

Waiting on engineer review #42438 (comment)

[melvin-bot]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @s77rt

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.77-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix blank preview when opened URL of deleted file #42680

If no regressions arise, payment will be issued on 2024-06-06. 🎊

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@slafortune / @MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: Video player #30829
• The offending PR has been commented on: Video player #30829 (comment)
• A discussion in #expensify-bugs has been started: Not needed (unique bug)
• Determine if we should create a regression test for this bug: No (edge case)

[slafortune]

Thanks @MitchExpensify I'll take this back now - vacation is O V E R

[slafortune]

@s77rt requires Paid ✅
@bernhardoj Paid ✅