[HOLD for payment 2024-10-14] [$250] mWeb - Attachment - Opened offline attachment directed to conversation page on online

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.25 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Open a chat
3. Go offline
4. Upload a image
5. Open the image
6. Go online
7. Note User directed to conversation page

Expected Result:

Upload and open attachment in offline, then going online user must stay in same attachment page

Actual Result:

Upload and open attachment in offline, then going online user directed to conversation page

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/083f0cf4-a3a8-4dad-864d-7bb3184eda93

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e366edd029d4e402
• Upwork Job ID: 1829648417535865218
• Last Price Increase: 2024-09-20
• Automatic offers:
• wildan-m | Contributor | 104146531

Issue Owner

Current Issue Owner: @mallenexpensify

[s77rt]

@wildan-m The new field will make will increase the flow complexity and it requires maintenance. We are still looking for a more straightforward solution.

[wildan-m]

Proposal Updated

• Add alternative solution using caching
• Branch link provided for more clarity

[s77rt]

@wildan-m Thanks for the update. I don't think adding in-memory cache is the best option here. Maybe we can still use the report action id and add a sequence id to identify the correct attachment?

This will also require us to fix the empty report action ids that we get for additional attachments

[wildan-m]

Proposal updated

• add alternative solution using sequence id (Alternative 2)

Maybe we can still use the report action id and add a sequence id to identify the correct attachment?

@s77rt that's possible, but we need to also check if the reportActionIsEdited to avoid false positive result.

[s77rt]

@wildan-m Ah this won't work well. We still need a way to identify attachments. The sequence id could change on edit.

[wildan-m]

@s77rt when edit, optimistic attachment is not currently possible, that's way we need to check isEdited. Ideally attachment should have it's own unique id, but I think that's beyond the scope of this issue

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[s77rt]

@wildan-m I get it. Adding a id for each attachment would work but it may be an overkill. Maybe we can take another approach and rethink on which cases to dismiss the modal.

[wildan-m]

Maybe we can take another approach and rethink on which cases to dismiss the modal.

@s77rt even if we can avoid the modal from dismissed, the targetAttachments will be updated, Not found page will appear as the localurl updated.

In my opinion, using an in-memory cache solution is the most practical approach. We can clear the cache each time we open attachmentCarousel to prevent any performance regression.

[s77rt]

@wildan-m Indeed the attachments state should be updated correctly. The cache approach still seems a little over-engineered (too much work compared to the bug impact). Yet I don't see a clear solution so far. Let's keep exploring for now

[wildan-m]

Proposal Updated

• Add alternative 3 solution - dismiss when there is no attachment with the same pageIndex

@s77rt What are your thoughts?

[s77rt]

@wildan-m I don't see how we can use the index to tell if the previous image is still available. This check targetAttachments[prevInitialPageRef.current] can always be true if the deleted attachment was not last.

[wildan-m]

@s77rt yes, as described in the initial solution description, but the optional section of solution can do approximate match using report action id, file name and uploading status. we can't do exact match here

[wildan-m]

Proposal Updated

• clarify optional check of alternative 3 solution by using isAttachmentSimilar function and provide branch

cc: @s77rt

[s77rt]

@wildan-m I don't think using the "similar" approach is worth it. It's not a complete solution since we'd have some left edge cases.

[wildan-m]

@s77rt Without a unique ID for the attachment, do you believe an exact match is feasible?

[s77rt]

@wildan-m I doubt matching optimistic attachment is possible. I'm hoping to find some other different approaches. If we don't get much going, I will raise this to Slack to see how we can deal with it (and to whether ignoring some edge cases is okay in that case)

[bernhardoj]

Proposal

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

Opening an optimistic attachment and then the upload complete will close the carousel.

What is the root cause of that problem?

When the attachment isn't found in the report action attachments list (by comparing the source), then the carousel will be dismissed. https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/components/Attachments/AttachmentCarousel/index.tsx#L91-L95

This happens because the optimistic attachment source is a local file, but when the file is uploaded, the report action contains the remote source.

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

We need to have a unique ID for each attachment which we can use as the comparison, instead of only relying on the source.

What can we use as the ID? The report action ID. To cover multiple attachments in a single action, we can append a 0-index numbering to it, e.g., 1231512_0, 1251512_1, ...

The idea is to pass the generated ID as the route param, then, when extracting each attachment object (extractAttachments), also generate the ID. When comparing, we can use those ID as the fallback when comparing the source fails.

here are the steps:

1. In getReportActionMessageFragments, modify the HTML of each fragment to include the ID. https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/libs/ReportActionsUtils.ts#L1338-L1355

function addIDToAttachmentHtml(item: Message, reportActionID: string): Message {
    let index = 0;
    return {
        ...item,
        html: item.html?.replace(/(<img.*?)(\/>)|(<video.*?)(>)/g, (_, g1, g2, g3, g4) => {
            return `${g1 || g3} data-id="${reportActionID}_${index++}"${g2 || g4}`;
        }),
    }
}

function getReportActionMessageFragments(action: ReportAction): Message[] {
    ...
    if (Array.isArray(actionMessage)) {
        return actionMessage.filter((item): item is Message => !!item).map((item) => addIDToAttachmentHtml(item, action.reportActionID));
    }
    return actionMessage ? [addIDToAttachmentHtml(actionMessage, action.reportActionID)] : [];
}

2. Since the HTML now contains the ID (data-id), we can access it and pass it to the route param. We need to update the ROUTE first to accept the id param. Do the same for VideoRenderer. https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L93-L94

const route = ROUTES.ATTACHMENTS.getRoute(reportID ?? '-1', type, source, htmlAttribs['data-id'], accountID);

3. Next, generate the ID in extractAttachments. We already have the ID, but we only replace the first occurrences of /> and we use the reportActionID as the ID. https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/components/Attachments/AttachmentCarousel/extractAttachments.ts#L97-L106

   let index = 0;
   const html = ReportActionsUtils.getReportActionHtml(action).replace(/(<img.*?)(\/>)|(<video.*?)(>)/g, (_, g1, g2, g3, g4) => {
   return `${g1 || g3} data-flagged="${hasBeenFlagged}" data-id="${action.reportActionID}_${index++}"${g2 || g4}`;
   });

I notice that we haven't pass the ID to the video attachment object, https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/components/Attachments/AttachmentCarousel/extractAttachments.ts#L33-L48

so, we need to pass it too.

reportActionID: attribs['data-id'],

Btw, I think we need to rename it from reportActionID to just id or attachmentID.

4. Now that the ID is ready, the left thing is to pass the ID route param down to AttachmentCarousel and do the comparison. Pass it from ReportAttachments > AttachmentModal > AttachmentCarousel.

Then, compare the ID here https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/components/Attachments/AttachmentCarousel/index.tsx#L72

const compareImage = useCallback((attachment: Attachment) => attachment.source === source || attachment.reportActionID === id, [source]);

5. Last, we need to pass the ID here every time the attachment changes. https://github.com/Expensify/App/blob/b1720c7a420a5f5e5482f40b8a6a31459c909d18/src/pages/home/report/ReportAttachments.tsx#L34-L40

const routeToNavigate = ROUTES.ATTACHMENTS.getRoute(reportID, type, String(attachment.source), String(attachment.reportActionID), Number(accountID));

[melvin-bot[bot]]

@s77rt @kevinksullivan @mallenexpensify this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[s77rt]

@bernhardoj Thanks for the proposal. The RCA is correct. Regarding the id, the index cannot be used as id because it could change if any attachment is removed or added in between

[wildan-m]

Curious with @Expensify/design opinion: @shawnborton @dubielzyk-expensify @dannymcclain

Context We have this modal dismissal issue when uploading an attachment while offline. The uploaded attachment modal preview will be dismissed when the network back online since the source url will be different:

Before	After
blob:https://...... (local blob url)	https://staging.new..... (server url)

The current challenges are -- we can't exactly match the previous local attachment and uploaded attachment with the following reasons:

• File name is not unique
• Report action ID can contain multiple attachments and some attachment report action id can be empty

Adding a unique attachment id would be considered as overkill (https://github.com/Expensify/App/issues/48173#issuecomment-2364207534) since it might require backend change and not quite simple to implement especially when editing the images for multiple attachments case.

I want to know your opinion about my alternative 3 proposal https://github.com/Expensify/App/issues/48173#issuecomment-2354920968 (excluding optional logic), since it will change current behavior. With this solution, we don't need to compare the attachment to handle the dismissal, but it will change the dismissal behavior as explained in the following table:

Before	After
Dismiss if the exact image is deleted	Change the image to the image with the same index after deletion; otherwise, dismiss the modal

[bernhardoj]

Regarding the id, the index cannot be used as id because it could change if any attachment is removed or added in between

Do you found an issue with that approach?

[wildan-m]

I checked Slack's behavior in this situation and found that it is similar to my proposed solution. The only difference is that Slack will revert to the initial index if there are no other attachments with the same index. We can follow that difference if needed.

I test it using private group, use personal self chat will not work, seems personal self chat not live synchronized to active devices.

https://github.com/user-attachments/assets/e3a42442-52a1-4684-bacc-e6d4d9b41249

[s77rt]

@bernhardoj

Do you found an issue with that approach?

Not really but the idea of using a non-unique attribute as the id does not feel right

[dannymcclain]

I don't think I have any issues with the behavior that you're showing here in the After column. Both the before and after behavior feel pretty normal to me. So I think I would defer to engineers to decide what makes the most sense from a technical perspective.

[s77rt]

@wildan-m The new behaviour seems to work well and does fix the issue. Let's go with alternative solution (3) without the isAttachmentSimilar and without prevInitialPageRef (it's same as attachments.findIndex(compareImage))

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @wildan-m 🎉 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 📖

[wildan-m]

@s77rt The PR is ready https://github.com/Expensify/App/pull/49832 Thanks!

[mallenexpensify]

Note to self... PR hit staging on Oct 4th (posting as a reminder in case production deploy automation fails, it should be fixed now though)

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/49832

If no regressions arise, payment will be issued on 2024-10-14. :confetti_ball:

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

• @wildan-m requires payment automatic offer (Contributor)
• @s77rt requires payment through NewDot Manual Requests

[melvin-bot[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.
• [ ] [@mallenexpensify] 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: https://github.com/Expensify/App/pull/20798 and https://github.com/Expensify/App/pull/44889
• The offending PR has been commented on: https://github.com/Expensify/App/pull/20798#discussion_r1797016934
• A discussion in #expensify-bugs has been started: Not needed. Unique bug
• Determine if we should create a regression test for this bug: Not yet, we still have another related issue to fix then we can add the regression test.

[mallenexpensify]

Contributor: @wildan-m paid $250 via Upwork Contributor+: @s77rt due $250 via NewDot

I posted in the other issue to state we need a regression test there https://github.com/Expensify/App/issues/50296#issuecomment-2412230228

Thx!

[JmillsExpensify]

$250 approved for @s77rt