[HOLD for payment 2022-06-16] [HOLD for payment 2022-06-15] [$500] Uploading an image causes the thumbnail to resize 3 times

[mvtglobally]

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

---

Action Performed:

1. Open any chat
2. Upload a larger file to share

Expected Result:

Thumbnail should load once uploaded, or if it's a large file it should display a spinner until it's uploaded.

Actual Result:

If you share an image, the uploading thumbnail resizes 3 times. Then it finalizes the upload.

Workaround:

Just wait for the file to upload but it's a jarring experience.

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.52-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

If you click + > add attachment in a DM, and then select this attachment:

You'll notice it changes to this momentarily:

And then this:

Upwork job URL: https://www.upwork.com/jobs/~01ca59e5811fecf4d5 Issue reported by: @cead22 Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1649352167230159

View all open jobs on GitHub

[chiragsalian]

Yeah that looks fine for staging. Not sure why its not working for you. Either way, the code should be on production tomorrow morning but i'll let you know too.

[chiragsalian]

@allroundexperts The code for having the dimensions as data attributes is now on production 🙂

[allroundexperts]

Proposal

The thumbnail image that first appears on upload can be made the exact same size by using the RenderHTML method same as how it's done with the uploaded image. This leads to the disappearance of the first 2 resizes.

The following changes from:

https://github.com/Expensify/App/blob/ec828a22f031f646b19af819a27c70512a881712/src/pages/home/report/ReportActionItemFragment.js#L73-L98

To:

            if (props.isAttachment && props.loading) {
                return (
                    Str.isImage(props.attachmentInfo.name)
                        ? (
                            <RenderHTML html={`<comment><img src="${props.attachmentInfo.source}"/></comment>`} />
                        ) : (
                            <View style={[styles.chatItemAttachmentPlaceholder]}>
                                <ActivityIndicator
                                    size="large"
                                    color={themeColors.textSupporting}
                                    style={[styles.flex1]}
                                />
                            </View>
                        )
                );
            }

The third resize occurs when we have the uploaded image url but we are loading it (Large images take some time to load). While an image is loading, we don't have its size. As such, the thumbnail was of a default (200 x 200) size. To get the size of image before it loads, we can append the image size as html attributes from the backend. These attributes can then be used to make sure that thumbnail that appears while image is loading is of same size as the original image.

The code changes from:

https://github.com/Expensify/App/blob/ec828a22f031f646b19af819a27c70512a881712/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js#L33-L67

To:

    const isAttachment = Boolean(htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]);
    const originalFileName = htmlAttribs['data-name'];
    let previewSource = htmlAttribs.src;
    let source = isAttachment
        ? htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]
        : htmlAttribs.src;

    // Update the image URL so the images can be accessed depending on the config environment
    previewSource = previewSource.replace(
        Config.EXPENSIFY.EXPENSIFY_URL,
        Config.EXPENSIFY.URL_API_ROOT,
    );
    source = source.replace(
        Config.EXPENSIFY.EXPENSIFY_URL,
        Config.EXPENSIFY.URL_API_ROOT,
    );

    const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
    const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

    return (
        <AttachmentModal
            sourceURL={source}
            isAuthTokenRequired={isAttachment}
            originalFileName={originalFileName}
        >
            {({show}) => (
                <PressableWithoutFocus
                    style={styles.noOutline}
                    onPress={show}
                >
                    <ThumbnailImage
                        previewSourceURL={previewSource}
                        style={styles.webViewStyles.tagStyles.img}
                        isAuthTokenRequired={isAttachment}
                        imageWidth={imageWidth}
                        imageHeight={imageHeight}
                    />
                </PressableWithoutFocus>
            )}
        </AttachmentModal>

The ThumbnailImage component is also refactored so that it can handle the new imageHeight & imageWidth prop.

https://github.com/Expensify/App/blob/ec828a22f031f646b19af819a27c70512a881712/src/components/ThumbnailImage.js#L11-L60

To:

const propTypes = {
    /** Source URL for the preview image */
    previewSourceURL: PropTypes.string.isRequired,

    /** Any additional styles to apply */
    // eslint-disable-next-line react/forbid-prop-types
    style: PropTypes.any,

    /** Do the urls require an authToken? */
    isAuthTokenRequired: PropTypes.bool.isRequired,

    /** Image size */
    imageWidth: PropTypes.number,
    imageHeight: PropTypes.number,

    ...windowDimensionsPropTypes,
};

const defaultProps = {
    style: {},
    imageWidth: 200,
    imageHeight: 200,
};

class ThumbnailImage extends PureComponent {
    constructor(props) {
        super(props);

        this.updateImageSize = this.updateImageSize.bind(this);
        const {width, height} = this.calculateThumbnailImageSize({width: props.imageWidth, height: props.imageHeight});
        this.state = {
            thumbnailWidth: width,
            thumbnailHeight: height,
        };
    }

    calculateThumbnailImageSize({width, height}) {
        if (!width || !height) { return {}; }

        // Width of the thumbnail works better as a constant than it does
        // a percentage of the screen width since it is relative to each screen
        // Note: Clamp minimum width 40px to support touch device
        let thumbnailScreenWidth = lodashClamp(width, 40, 250);
        const imageHeight = height / (width / thumbnailScreenWidth);
        let thumbnailScreenHeight = lodashClamp(imageHeight, 40, this.props.windowHeight * 0.40);
        const aspectRatio = height / width;

        // If thumbnail height is greater than its width, then the image is portrait otherwise landscape.
        // For portrait images, we need to adjust the width of the image to keep the aspect ratio and vice-versa.
        if (thumbnailScreenHeight > thumbnailScreenWidth) {
            thumbnailScreenWidth = Math.round(thumbnailScreenHeight * (1 / aspectRatio));
        } else {
            thumbnailScreenHeight = Math.round(thumbnailScreenWidth * aspectRatio);
        }
        return {width: thumbnailScreenWidth, height: thumbnailScreenHeight};
    }

    updateImageSize({width, height}) {
        const {width: thumbnailScreenWidth, height: thumbnailScreenHeight} = this.calculateThumbnailImageSize({width, height});
        this.setState({thumbnailWidth: thumbnailScreenWidth, thumbnailHeight: Math.max(40, thumbnailScreenHeight)});
    }

Here is how the final result looks:

https://user-images.githubusercontent.com/30054992/167023275-f91c0d23-80bb-4415-9023-d07677fe6544.mp4

[Santhosh-Sellavel]

@allroundexperts Always include the code snippet instead of images, it would help us to review it faster.

Can you share the updated proposal with code snippets? Thanks!

[allroundexperts]

@Santhosh-Sellavel Updated.

[Santhosh-Sellavel]

The issue still exists for the image which was sent earlier, not for the image update recently check the video.

https://user-images.githubusercontent.com/85645967/167208448-309bea2a-97c1-4bd9-8453-23ba82f2cdf3.mov

[allroundexperts]

Issue still exists for few images.

issue.mov

@Santhosh-Sellavel These seem to be previously uploaded images. This will happen for them unless they're updated with the new attribute.

[Santhosh-Sellavel]

Yeah, I was updating the same in the comment!

[allroundexperts]

@Santhosh-Sellavel I don't think we can do anything about the previously uploaded images. @chiragsalian Maybe you can run some sort of cron that updates the already existing images?

[Santhosh-Sellavel]

Yeah, @chiragsalian can show some clarity here!

[Santhosh-Sellavel]

@allroundexperts what do you mean by this, is it necessary defaultProps.imageSize width or height here?

this.state = {
            thumbnailWidth: width || defaultProps.imageSize.width,
            thumbnailHeight: height || defaultProps.imageSize.height,
        };

[allroundexperts]

It's checking if width and height was supplied in the image attributes. If not, we're setting a default width and height for thumbnail exactly how it was previously. We're doing defaultProps to allow for previously uploaded images which won't have the width and height as attribute. @Santhosh-Sellavel

[Santhosh-Sellavel]

Maybe check out this documentation https://reactjs.org/docs/typechecking-with-proptypes.html. Know more about propTypes & defaultProps!

[allroundexperts]

@Santhosh-Sellavel What do you mean? Sorry, I did not get you.

[allroundexperts]

@Santhosh-Sellavel I get it now. Will update proposal.

[allroundexperts]

@Santhosh-Sellavel Updated.

[Santhosh-Sellavel]

Mostly looks good, waiting for @chiragsalian response for this https://github.com/Expensify/App/issues/8590#issuecomment-1119967584

[michaelhaxhiu]

Not overdue, awaiting input from @chiragsalian :)

[chiragsalian]

Just reading this now. Yeah i don't think its valuable to update all past images on newDot. I think it's okay to have this feature only working moving forward with new images.

[Santhosh-Sellavel]

Thanks, @chiragsalian.

@allroundexperts proposal looks good to me!

A couple of suggestions,

1. There is some unnecessary object construction & destructuring. So the following code

   this.calculateThumbnailImageSize({width: props.imageWidth, height: props.imageHeight});
   ...
   calculateThumbnailImageSize({width, height}) {
   ...
   }

   refactered to this

   
   this.calculateThumbnailImageSize(props.imageWidth, props.imageHeight);
   ...

calculateThumbnailImageSize(width, height) { ... }

2.  `allowDownload` prop is deleted in the proposal, make sure to add it back!
<img width="547" alt="Screenshot 2022-05-10 at 5 21 19 AM" src="https://user-images.githubusercontent.com/85645967/167516519-33415543-4ebb-41ad-8a56-69530f2ccbc4.png">

[chiragsalian]

Awesome, proposal looks good to me too. Feel free to work on the PR @allroundexperts.

[melvin-bot[bot]]

📣 @allroundexperts You have been assigned to this job by @chiragsalian! Please apply to this job in Upwork 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 📖

[michaelhaxhiu]

@allroundexperts can you please apply for the job on upwork?

[allroundexperts]

@michaelhaxhiu Applied.

[michaelhaxhiu]

Hired, feel free to start!

[allroundexperts]

@michaelhaxhiu I can't seem to run the app on iOS. I am on a m1 macbook but the instructions to run it on a m1 device requires me to login. Can you help? SO

[michaelhaxhiu]

@allroundexperts let's post in #expensify-open-source for peer input!

[michaelhaxhiu]

Next step is for @Santhosh-Sellavel to review the PR

[Santhosh-Sellavel]

Already on it.

[chiragsalian]

Hey guys I had a question, so from the PR we got the behavior like so,

https://user-images.githubusercontent.com/85645967/169619263-7ff7211a-df92-4145-aaa5-a65a865cc671.mov

where,

1. First the user uploads and image we have a temporary thumbnail and dimensions so we show it.
2. The server uploads the image, generates a cloud link and returns it to the user. Now the user updates the image url to the cloud url so we see a loading gif.
3. Once it finishes downloading the user see the images.

The thumbnails from step 1 and 3 are the same and i was wondering if the loading gif is a bad user experience or not. In the issue we discussed removing the loading gif but that seems pretty tricky atm. We could show the first image thumbnail with an opacity to indicate its not uploaded yet but i was think maybe this is unnecessary because of the offline first approach as there might be another indicator for this.

Anyway i was wondering what others think. Does the video above look fine or would people prefer to see some changes?

[cead22]

The thumbnails from step 1 and 3 are the same and i was wondering if the loading gif is a bad user experience or not

I think the video already shows an improvement from what we already have, but I do think showing the spinner is not ideal. Do either of these two options seem feasible and worth exploring?

• One idea would be to display preview, upload the image, once that's done we create an invisible image element, and once it finishes loading (img.onload) we can swap the preview with the invisible image
• Another is to display the preview, upload the image, once that's done we create an image element that's underneath the preview, and once it finishes loading (img.onload) we can remove the preview to reveal the image underneath

[chiragsalian]

I think they are worth exploring. What do you feel @allroundexperts? Additionally for one of your comments in the PR,

Doing this change for just a single user (other users would still see loader) is not worth the time in my opinion.

Well for other users its completely understandable they would see a loader and maybe there is some room for improvement here but for now i think its worth focusing on the improvement for the single user experience of uploading the image.

[allroundexperts]

These were discussed in the beginning as well but given how code is structured it won’t be easy to do these. Also, this would only work for the uploader of the image. The other users in the chat would still have to see a loader.

On Tue, 24 May 2022 at 10:46 PM, Carlos Alvarez @.***> wrote:

The thumbnails from step 1 and 3 are the same and i was wondering if the loading gif is a bad user experience or not

I think the video already shows an improvement from what we already have, but I do think showing the spinner is not ideal. Do either of these two options seem feasible and worth exploring?

• One idea would be to display preview, upload the image, once that's done we create an invisible image element, and once it finishes loading (img.onload) we can swap the preview with the invisible image
• Another is to display the preview, upload the image, once that's done we create an image element that's underneath the preview, and once it finishes loading (img.onload) we can remove the preview to reveal the image underneath

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/8590#issuecomment-1136256236, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHFJUUCISGJIS3JC7SVP4CDVLUIW3ANCNFSM5TCA4OKQ . You are receiving this because you were mentioned.Message ID: @.***>

[michaelhaxhiu]

and maybe there is some room for improvement here but for now i think its worth focusing on the improvement for the single user experience of uploading the image.

Another option is to expand the scope of this job to include the addition, and increase the pay to compensate for that time/work. I think we can also make a separate job.

But either way, it sounds like we want to address the multi-user case right?

[chiragsalian]

Sure yes any future work can be a separate job. We can open up a follow up issue to discuss improvements if others feel strongly too. I dont think its about addressing multi-user case. I imagine a user receiving the image will see a black loading gif and thats understandable since they need to download the image. I feel like the use case of the person uploading the image can have a smoother experience by not showing the blank white loading gif. We'll have to look at the offline first doc to see how report comments are synced and then get designs input on best way attachments should be synced once the user comes online. If every image gets a white loading gif when they come online i think it would be pretty ugly. So yes, lets create a follow up issue to discuss.

[Santhosh-Sellavel]

@chiragsalian Status on PR: Just a few minor changes pending on PR on @chiragsalian final review. Mostly my job is done, here. Moving forward If there is anything major that needs C+ attention, feel free to un-assign me and add a new C+ here, thanks!

cc: @michaelhaxhiu

[michaelhaxhiu]

Started a discussion in #expensify-open-source here about further improvements we can make to the flow of image upload (let's not block this job on the discussion though).

[chiragsalian]

@allroundexperts, unfortunately we had to revert your PR due to this regression issue found by QA. Can you recreate your PR and address that issue as well.

[allroundexperts]

I'll check that.

On Wed, Jun 8, 2022 at 12:50 AM Chirag Chandrakant Salian < @.***> wrote:

@allroundexperts https://github.com/allroundexperts, unfortunately we had to revert https://github.com/Expensify/App/pull/9343 your PR due to this regression issue https://github.com/Expensify/App/issues/9334 found by QA. Can you recreate your PR and address that issue as well.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/8590#issuecomment-1149098137, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHFJUUEYZ56YKCZCFV3OMPDVN6RZDANCNFSM5TCA4OKQ . You are receiving this because you were mentioned.Message ID: @.***>

[allroundexperts]

@chiragsalian Created a new PR: https://github.com/Expensify/App/pull/9346

[chiragsalian]

Nice, thank you for the quick fix. Shall review shortly.

[chiragsalian]

The new PR was created, reviewed and merged 🙂

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.1.73-2 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/8928

If no regressions arise, payment will be issued on 2022-06-15. :confetti_ball:

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.1.74-2 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/9346

If no regressions arise, payment will be issued on 2022-06-16. :confetti_ball:

[michaelhaxhiu]

Ok so payment is due today. I'm seeing a regression here and so the payout structure would be:

• $500 for @allroundexperts
• $500 for @Santhosh-Sellavel
• $brownie points for @cead22 the bug reporter

[michaelhaxhiu]

Awaiting confirmation from Chirag for this, as I'm alittle unsure about this regression 👆

[chiragsalian]

I don't feel a penalty for this regression is required for C+ because the PR was testing thumbnail loading and not really the attachment modal. So QA finding a regression with attachment modal would have been very hard to catch in the normal PR and hence i don't feel there should be a penalty for the C+. (P.S i also tested the PR on different devices and would not have thought to test the attachment modal the way QA did)

[michaelhaxhiu]

Got it, so it seems you are saying this was out of scope of C+ responsibility.

Did we make the necessary changes to our process to ensure we don't miss this testing step again?

[chiragsalian]

I feel like the issue caught by QA was out of the scope of the C+ responsibility since its quite the edge case.

Also technically even QA tested and cleared the PR without realizing any issue with it and later found the regression issue. Most likely from their checklist of complete app testing steps.

Did we make the necessary changes to our process to ensure we don't miss this testing step again?

I'm not sure what can be done differently other than expecting our C+ to do the same regression testing as QA but this list is huge and not worth it for the C+ to go over so im unsure. Maybe we can recommend people to test a bit more beyond the PR's expectation. Not sure but thats a discussion for slack.

[michaelhaxhiu]

Got it! Thanks for the context.

In that case, it seems like the feedback is for QA so they don't miss it next time. Sometimes our regressions are QAs fault, I think that's going to happen from time to time!