[HOLD for payment 2024-10-24] [$100] Attachment - Chat blinks when txt attachment is uploaded

[IuliiaHerets]

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: 9.0.30-7
Reproducible in staging?: Y
Reproducible in production?: Y
Issue was found when executing this PR: #47778
Issue reported by: Applause Internal Team

Action Performed:

1. Log in with a Gmail account
2. Open any 1:1 chat
3. Send a txt attachment

Expected Result:

It shouldn't blink.

Actual Result:

Chat blinks when txt attachment is uploaded.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6594960_1725596733523.OIMT0425.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833566073242365431
• Upwork Job ID: 1833566073242365431
• Last Price Increase: 2024-09-27
• Automatic offers:
• nkdengineer | Contributor | 104162747

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

Triggered auto assignment to @isabelastisser (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.

[IuliiaHerets]

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[isabelastisser]

review by EOD

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

Chat blinks when txt attachment is uploaded.

What is the root cause of that problem?

We get the download data via withOnyx. It's an old HOC and can affect some weird lifecycle component bugs. As in this case, BaseAnchorForAttachmentsOnly disappears and appears again after the API is complete.

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Lines 81 to 84 in 762bffc

	export default withOnyx<BaseAnchorForAttachmentsOnlyProps, BaseAnchorForAttachmentsOnlyOnyxProps>({
	download: {
	key: ({source}) => {
	const sourceID = (source?.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1];

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

We should remove withOnyx in this component and use useOnyx to get the download data

const [download] = useOnyx(`${ONYXKEYS.COLLECTION.DOWNLOAD}${sourceID}`);

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Lines 81 to 84 in 762bffc

	export default withOnyx<BaseAnchorForAttachmentsOnlyProps, BaseAnchorForAttachmentsOnlyOnyxProps>({
	download: {
	key: ({source}) => {
	const sourceID = (source?.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1];

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2024-09-11.at.12.34.21.mov

[isabelastisser]

@getusha, can you please review the proposal above? thanks!

[melvin-bot]

@isabelastisser, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

[isabelastisser]

Hey @getusha, any updates?

[getusha]

@nkdengineer what about this line?

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 84 in 0c618ac

const sourceID = (source?.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1];

seems like we are not directly using the value

[nkdengineer]

@getusha What do you mean? When we remove withOnyx, we also remove this line and we already get the sourceID in the component.

[getusha]

oh yeah you're right

[getusha]

@nkdengineer could you test this on latest main? seems like uploading txt file isn't working on android & ios mweb

[nkdengineer]

@getusha I still able to upload the txt file for Android app and web on the latest main.

Screen.Recording.2024-09-16.at.16.45.31.mov

Screen.Recording.2024-09-16.at.16.44.31.mov

[melvin-bot]

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

[isabelastisser]

@getusha, can you please follow up on the comment above and provide an update? Thanks!

[melvin-bot]

@isabelastisser, @MonilBhavsar, @getusha, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[nkdengineer]

@getusha this PR is ready for review

[isabelastisser]

@getusha, can you please provide an update? Thanks!

[getusha]

Reviewing the PR, will complete it in short.

[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 9.0.49-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Chat blinks when txt attachment is uploaded #49908

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

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

• @getusha requires payment through NewDot Manual Requests
• @nkdengineer 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:

• [@getusha] The PR that introduced the bug has been identified. Link to the PR:
• [@getusha] 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:
• [@getusha] 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:
• [@getusha] Determine if we should create a regression test for this bug.
• [@getusha] 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.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[isabelastisser]

@getusha, do we need a regression test? Please complete the list above, thanks!

[isabelastisser]

Payment summary:

@getusha requires payment through NewDot Manual Requests - PENDING $100 for C+ review
@nkdengineer paid in Upwork (https://www.upwork.com/nx/wm/offer/104162747) $100 Contributor role

[isabelastisser]

Bump @getusha to complete the checklist. Thanks!

[getusha]

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:

• The PR that introduced the bug has been identified. Link to the PR: N/a, it was an issue with withOnyx
• 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: N/a
• 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: N/a
• Determine if we should create a regression test for this bug. No, Doesn't affect any important user flow.
• 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. N/A

[isabelastisser]

Thanks, all set!