Skip to content
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

[HOLD for payment 2024-07-24] [$500] Chat - Bad performance when opening, closing or zooming big image #38180

Closed
1 of 6 tasks
lanitochka17 opened this issue Mar 12, 2024 · 154 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 12, 2024

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: 1.4.51-0
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://expensify.testrail.io/index.php?/tests/view/4418462
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: the user must be logged in

  1. Go to any chat
  2. Send the attached GIF
  3. Send the attached image
  4. Click on the image preview to open it
  5. Verify the opening process is not smooth
  6. Click on the image to zoom in
  7. Verify is not instantly zoomed in, the behavior is slowly
  8. Try to move the zoom view, verify is not smooth
  9. Close the image
  10. Verify the closing process is not smooth

Expected Result:

There should be no performance issues when opening, zooming or closing big images

Actual Result:

The behavior of the app gets slower when opening, zooming or closing the big image

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

Add any screenshot/video evidence

Bug6411486_1710273467163.bandicam_2024-03-12_15-48-26-231.mp4

Bug6411486_1710273547556!Big_image (1)

Bug6411486_1710276381171!typing-fast-typing

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0184b3ab8ca5055856
  • Upwork Job ID: 1777877652768452608
  • Last Price Increase: 2024-06-27
  • Automatic offers:
    • wildan-m | Contributor | 102956097
Issue OwnerCurrent Issue Owner: @sobitneupane
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 12, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@lanitochka17
Copy link
Author

@mallenexpensify 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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp
CC @quinthar

@mallenexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added the Overdue label Mar 15, 2024
Copy link

melvin-bot bot commented Mar 18, 2024

@mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

@mallenexpensify
Copy link
Contributor

Adding to #vip-vsb since it's a bug that doesn't involve money.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 20, 2024
@mallenexpensify
Copy link
Contributor

Bumping to weekly while we await a status update

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2024
@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Mar 27, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 5, 2024
@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Apr 10, 2024
@melvin-bot melvin-bot bot changed the title Chat - Bad performance when opening, closing or zooming big image [$250] Chat - Bad performance when opening, closing or zooming big image Apr 10, 2024
Copy link

melvin-bot bot commented Apr 10, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 10, 2024
Copy link

melvin-bot bot commented Apr 10, 2024

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

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Apr 10, 2024

I think/hope this can be External. @sobitneupane , let me know if you disagree.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Overdue Weekly KSv2 labels Apr 10, 2024
@sobitneupane
Copy link
Contributor

@mallenexpensify I do agree. I think this can be handled externally.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 10, 2024

Proposal

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

We can see some performance issues with images, like when using high resolution images.

What is the root cause of that problem?

Images which have too high resolution show bad performance since the render time increases and tasks such as zoom also become heavy.

For example, the below image is 9001x9000.

Example image

https://private-user-images.githubusercontent.com/78819774/312232438-66b91727-3f02-456a-bc34-f2f0f80622bb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTMyNTkwNjEsIm5iZiI6MTcxMzI1ODc2MSwicGF0aCI6Ii83ODgxOTc3NC8zMTIyMzI0MzgtNjZiOTE3MjctM2YwMi00NTZhLWJjMzQtZjJmMGY4MDYyMmJiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MTYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDE2VDA5MTI0MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTBhNjFkNTVmNTkwMGU0NzY5ZDFkYjMzYWNkYzY3YjI4YTNlYWUxYTZmMmU1MWU0ZmZkZDczMzY3MzhjY2QxMjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Dv5Ay334kJNvPjzEruP8UgHyMdv47lpZGhdrPXZLUBM

In the validation logic, we are not checking this.

const validateAndDisplayFileToUpload = useCallback(
(data: FileObject) => {
if (!data || !isDirectoryCheck(data)) {
return;
}
let fileObject = data;
if ('getAsFile' in data && typeof data.getAsFile === 'function') {
fileObject = data.getAsFile();
}
if (!fileObject) {
return;
}
if (!isValidFile(fileObject)) {
return;
}
if (fileObject instanceof File) {
/**
* Cleaning file name, done here so that it covers all cases:
* upload, drag and drop, copy-paste
*/
let updatedFile = fileObject;
const cleanName = FileUtils.cleanFileName(updatedFile.name);
if (updatedFile.name !== cleanName) {
updatedFile = new File([updatedFile], cleanName, {type: updatedFile.type});
}
const inputSource = URL.createObjectURL(updatedFile);
const inputModalType = getModalType(inputSource, updatedFile);
setIsModalOpen(true);
setSourceState(inputSource);
setFile(updatedFile);
setModalType(inputModalType);
} else if (fileObject.uri) {
const inputModalType = getModalType(fileObject.uri, fileObject);
setIsModalOpen(true);
setSourceState(fileObject.uri);
setFile(fileObject);
setModalType(inputModalType);
}
},
[isValidFile, getModalType, isDirectoryCheck],
);

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

Add the below logic after we get updatedFile object in validateAndDisplayFileToUpload inside AttachmentModal. This would help us in displayed a preview with low size, while uploading the original image to the backend.

const img = new Image();
img.src = URL.createObjectURL(updatedFile);

await new Promise((resolve) => {
    img.onload = resolve;
});

let inputSource = URL.createObjectURL(updatedFile);

// If any dimension is above 4096, create a compressed version for display
if (img.width > 4096 || img.height > 4096) {
    const canvas = document.createElement('canvas');
    canvas.width = img.width > 4096 ? 4096 : img.width;
    canvas.height = img.height > 4096 ? 4096 : img.height;

    const ctx = canvas.getContext('2d');
    ctx.drawImage(img, 0, 0, canvas.width, canvas.height);

    inputSource = canvas.toDataURL();
}

Above code can be polished.

Note that we are only changing the inputSource and not the updatedFile. So, only the preview in the modal gets changed.

Then, we can also show a message saying that the preview is of low quality than expected, and original image can be seen by downloading (UX to be decided by design team).

{isImageCompressed && (
    <View style={[styles.flexRow, styles.alignItemsCenter, styles.mt2, styles.mb2]}>
        <Icon src={Info} additionalStyles={[styles.mr2]} fill={theme.placeholderText} />
        <Text style={styles.textNormal} color={theme.placeholderText}>{'This image has been resized for previewing. '}
            <TextLink
                onPress={downloadAttachment}
            >Download</TextLink>
            {' for full resolution.'}</Text>
    </View>
)}

UI above can be polished. Also, we'll need to ensure that the preview fits in the modal.

isImageCompressed is state which will be set to true only if compression happens.

What alternative solution did you explore?

Old solution

We can check if the image being uploaded has a high resolution. If yes, then we'll compress it before uploading.

Add the below code after checking if it's a valid file:

if (!isValidFile(fileObject)) {
return;
}

Pseudo code (we can polish it):

const cleanAndOpenUploadModal = (fileObject) => {
    if (fileObject instanceof File) {
        /**
         * Cleaning file name, done here so that it covers all cases:
         * upload, drag and drop, copy-paste
         */
        let updatedFile = fileObject;
        const cleanName = FileUtils.cleanFileName(updatedFile.name);
        if (updatedFile.name !== cleanName) {
            updatedFile = new File([updatedFile], cleanName, {type: updatedFile.type});
        }
        const inputSource = URL.createObjectURL(updatedFile);
        const inputModalType = getModalType(inputSource, updatedFile);
        setIsModalOpen(true);
        setSourceState(inputSource);
        setFile(updatedFile);
        setModalType(inputModalType);
    } else if (newImage.uri) {
        const inputModalType = getModalType(fileObject.uri, fileObject);
        setIsModalOpen(true);
        setSourceState(fileObject.uri);
        setFile(fileObject);
        setModalType(inputModalType);
    }
}


if (Str.isImage(fileObject.name)) {
    getImageResolution(fileObject).then(
        ({
             height,
             width
         }) => height <= CONST.AVATAR_MAX_HEIGHT_PX && width <= CONST.AVATAR_MAX_WIDTH_PX,
    ).then((check) => {
        if (!check) {
            cropOrRotateImage(fileObject.uri, [], {compress: 0.5})
                .then((newImage) => {
                    // Continue with new image
                    cleanAndOpenUploadModal(newImage)

                })

        } else {
            // Continue with old image
            cleanAndOpenUploadModal(fileObject)
        }
    })
} else {
    // Continue with the file
    cleanAndOpenUploadModal(fileObject)
}

Above logic compresses the image by 50% only if it exceeds 4096 in height/width. This would decrease the image quality, but that should be fine because the image in such cases would mostly already be high quality.

Note that I have tested the heavy image with the above compression logic, and it is working well.

Screen.Recording.2024-04-16.at.4.40.10.PM.mov

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 4, 2024
@wildan-m
Copy link
Contributor

wildan-m commented Jul 4, 2024

@sobitneupane The PR is ready for your review. The translation in slack is not confirmed yet.

https://expensify.slack.com/archives/C01GTK53T8Q/p1719953316322399

@Julesssss
Copy link
Contributor

Julesssss commented Jul 8, 2024

PR still lin review, I'm bumping it today. Translation was approved.

@Julesssss
Copy link
Contributor

PR merged, awaiting deployment 🎉

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 17, 2024
@melvin-bot melvin-bot bot changed the title [$500] Chat - Bad performance when opening, closing or zooming big image [HOLD for payment 2024-07-24] [$500] Chat - Bad performance when opening, closing or zooming big image Jul 17, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 17, 2024
Copy link

melvin-bot bot commented Jul 17, 2024

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

Copy link

melvin-bot bot commented Jul 17, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 17, 2024

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:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
  • [@sobitneupane] 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:
  • [@sobitneupane] 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:
  • [@sobitneupane] Determine if we should create a regression test for this bug.
  • [@sobitneupane] 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:

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jul 23, 2024
@mallenexpensify
Copy link
Contributor

@sobitneupane plz complete the BZ checklist above.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 24, 2024
@mallenexpensify
Copy link
Contributor

Contributor: @wildan-m paid $500 via Upwork
Contributor+: @sobitneupane owed $500 via NewDot

@sobitneupane plz complete the BZ checklist above.

@sobitneupane
Copy link
Contributor

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:

Rather than a bug, it is a performance issue.

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@sobitneupane
Copy link
Contributor

sobitneupane commented Jul 29, 2024

Regression Test Proposal

  1. Go to any chat and upload an image with dimension less than 7000*7000 px.
  2. Verify that the original image is displayed and can be zoomed.
  3. Select Send button.
  4. Open the newly uploaded image and verify that the original image is displayed and can be zoomed.
  5. Go to any chat and upload an image with dimension more than 7000*7000 px.
  6. Verify that the attachment modal opens up with filename and info text at the bottom of the modal.
  7. Select Send button.
  8. Verify that the image is displayed and can be zoomed and info text is displayed at the bottom of the image.
  9. Download the image and verify that the dimensions of the downloaded image match those of the original uploaded image.

Do we agree 👍 or 👎

@Julesssss
Copy link
Contributor

@sobitneupane I think that is good, but lets add a case for downloading the image.

@sobitneupane
Copy link
Contributor

@Julesssss Updated.

@JmillsExpensify
Copy link

$500 approved for @sobitneupane

@mallenexpensify
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests