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

[$500] Account Settings – Banners in About/ Preferences/ Security are without animation in Offline #38172

Closed
1 of 6 tasks
lanitochka17 opened this issue Mar 12, 2024 · 17 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 12, 2024

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: 1.4.51-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): applausetester+jp_e_category@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in
  3. Turn off internet on device
  4. Navigate to Account Settings
  5. Open About/ Preferences/ Security
  6. Go back and repeat steps 4-5 if need

Expected Result:

Banners in About/ Preferences/ Security are with animation in Offline

Actual Result:

Banners in About/ Preferences/ Security are without animation in Offline

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

Bug6411447_1710272130796.Banner.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0199c79b8079cceda2
  • Upwork Job ID: 1767649903440666624
  • Last Price Increase: 2024-03-12
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Mar 12, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 12, 2024

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@lanitochka17
Copy link
Author

Production:

bandicam.2024-03-12.21-49-01-425.mp4

@lanitochka17
Copy link
Author

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

@marcochavezf
Copy link
Contributor

This is also happening in production when the connection is disabled from the Network tab in the dev tools:

Screen.Recording.2024-03-12.at.2.24.30.p.m.mov

I don't think it's a blocker, but I'm going to make it external to look for a solution to make the animations work offline consistently.

@marcochavezf marcochavezf added Daily KSv2 Improvement Item broken or needs improvement. Hourly KSv2 External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Mar 12, 2024
@melvin-bot melvin-bot bot changed the title Account Settings – Banners in About/ Preferences/ Security are without animation in Offline [$500] Account Settings – Banners in About/ Preferences/ Security are without animation in Offline Mar 12, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

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

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

melvin-bot bot commented Mar 12, 2024

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

@abzokhattab
Copy link
Contributor

abzokhattab commented Mar 12, 2024

Proposal

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

Account Settings – Banners in About/ Preferences/ Security are without animation in Offline

What is the root cause of that problem?

when going offline and opening the settings page all lotties inside the setting page rerender which cause it to fail and render an empty view as specified here:

function Lottie({source, webStyle, ...props}: Props, ref: ForwardedRef<LottieView>) {
const styles = useThemeStyles();
const [isError, setIsError] = React.useState(false);
useNetwork({onReconnect: () => setIsError(false)});
const aspectRatioStyle = styles.aspectRatioLottie(source);
// If the image fails to load, we'll just render an empty view
if (isError) {
return <View style={aspectRatioStyle} />;
}
return (
<LottieView
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
source={source.file}
ref={ref}
style={[aspectRatioStyle, props.style]}
webStyle={{...aspectRatioStyle, ...webStyle}}
onAnimationFailure={() => setIsError(true)}
/>
);

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

wrap the Lottie component with memo that would rerender only if the source changes similar to what we already are doing in the images, this way it will not rerender when going offline also this will optimize the lottie component since it's used in multiple locations in the app so best would be to prevent rerender if the source is the same:

to achieve that we need to change the component to:

const Lottie = forwardRef<LottieView, Props>(({source, webStyle, ...props}, ref) => {
    const styles = useThemeStyles();
    const [isError, setIsError] = React.useState(false);

    useNetwork({v: () => setIsError(false)});

    const aspectRatioStyle = styles.aspectRatioLottie(source);

    // If the image fails to load, we'll just render an empty view
    if (isError) {
        return <View style={aspectRatioStyle} />;
    }

    return (
        <LottieView
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...props}
            source={source.file}
            ref={ref}
            style={[aspectRatioStyle, props.style]}
            webStyle={{...aspectRatioStyle, ...webStyle}}
            onAnimationFailure={() => setIsError(true)}
        />
    );
});

const MemoizedLottie = memo(
    Lottie,
    (prevProps, nextProps) =>
        // This checks if the source prop hasn't changed to avoid re-rendering
        prevProps.source === nextProps.source,
);

export default MemoizedLottie;

@brandonhenry
Copy link
Contributor

brandonhenry commented Mar 12, 2024

Proposal

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

The problem we are trying to solve is that the banners in the Account Settings (About, Preferences, Security) are not animating when the app is offline. The Lottie animations fail to render and display an empty view instead.

What is the root cause of that problem?

The root cause of the problem is that when the app goes offline and the settings page is opened, all Lottie animations inside the settings page re-render. This re-rendering causes the animations to fail and render an empty view because the Lottie component tries to load the animation data again while offline.

function Lottie({source, webStyle, ...props}: Props, ref: ForwardedRef<LottieView>) {
const styles = useThemeStyles();
const [isError, setIsError] = React.useState(false);
useNetwork({onReconnect: () => setIsError(false)});
const aspectRatioStyle = styles.aspectRatioLottie(source);
// If the image fails to load, we'll just render an empty view
if (isError) {
return <View style={aspectRatioStyle} />;
}
return (
<LottieView
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
source={source.file}
ref={ref}
style={[aspectRatioStyle, props.style]}
webStyle={{...aspectRatioStyle, ...webStyle}}
onAnimationFailure={() => setIsError(true)}
/>
);

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

To solve this problem, we should modify the Lottie component to cache the loaded animation data. Here's how we can achieve that:

  1. Add a ref called animationRef to store a reference to the LottieView component.
  2. Implement a handleAnimationLoaded function that is called when the animation is loaded successfully. Inside this function, use the getAnimationDataAsync method to get the animation data and then call the cacheAnimationData method to cache the animation data.
  3. Update the ref prop on the LottieView component to properly assign the ref.

By caching the animation data, even if the component re-renders when going offline, it can use the cached data instead of trying to load it again, avoiding the error state and allowing the animations to work consistently offline.

function Lottie({source, webStyle, ...props}: Props, ref: ForwardedRef<LottieView>) {
    const animationRef = useRef<LottieView>(null);

    // Cache the animation when it's loaded successfully
    const handleAnimationLoaded = () => {
        if (animationRef.current) {
            animationRef.current.getAnimationDataAsync().then((animationData) => {
                animationRef.current?.cacheAnimationData(animationData);
            });
        }
    };

    // ...

    return (
        <LottieView
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...props}
            source={source.file}
            ref={(ref as any)}
            style={[aspectRatioStyle, props.style]}
            webStyle={{...aspectRatioStyle, ...webStyle}}
            onAnimationLoaded={handleAnimationLoaded}
            onAnimationFailure={() => setIsError(true)}
        />
    );
}
Screen.Recording.2024-03-12.at.4.31.35.PM.mov

What alternative solutions did you explore? (Optional)

We could wrap the Lottie component with memo and only re-rendering if the source prop changes. However, this approach may not be sufficient in solving the issue completely, as it doesn't address the case when the app goes offline and the animation data needs to be loaded again.

@kaushiktd
Copy link
Contributor

Hey man, @brandonhenry I want to learn more about your proposal if you don't mind. I tried to check the proposal but it seems I am unable to run it in my system. Maybe I am missing something. If it is cool with you, can you please tell where to do the changes in the code exactly?

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

fedirjh commented Mar 15, 2024

cc @lanitochka17 Can you please retest this bug on staging? I couldn’t reproduce it. check this video :

Video
CleanShot.2024-03-15.at.13.21.28.mp4

Thanks everyone for the proposals; however, I don't think we need any fix for this issue. We have already solved it in the past by implementing a preloading mechanism for the Lottie animations. This mechanism caches the animation files. Check this PR

I believe the current issue could be caused by either a problem with Cloudflare or the user disabling their browser cache. It's important to mention that preloading does not work in a dev environment.

cc @marcochavezf Could you please retest and consider closing the issue?

@melvin-bot melvin-bot bot removed the Overdue label Mar 15, 2024
@lanitochka17
Copy link
Author

@fedirjh Hello Issue is still reproducible on the latest build v1.4.53

bandicam.2024-03-15.16-17-31-756.mp4

@fedirjh
Copy link
Contributor

fedirjh commented Mar 15, 2024

@lanitochka17 Thank you for testing, Could you please verify if you are disabling the cache in the network tab ?

Screenshot 2024-03-15 at 5 08 16 PM

@brandonhenry
Copy link
Contributor

brandonhenry commented Mar 15, 2024

@kaushiktd I made the changes directly to the Lottie file. I can push up a draft PR with the changes if that helps? will see if this is still reproducible from @lanitochka17

@lanitochka17
Copy link
Author

Issue is still reproducible on the latest build 1.4.53-1

bandicam.2024-03-15.20-12-46-620.mp4
bandicam.2024-03-15.19-51-22-637.mp4

@melvin-bot melvin-bot bot added the Overdue label Mar 18, 2024
@kaushiktd
Copy link
Contributor

I have attempted to cache the path and re-render from the cached path when offline. However, this approach is not working because @dotlottie/react-player utilizes several JavaScript files to render in the web interface. When users go offline, the JavaScript functionality fails due to CORS (Cross-Origin Resource Sharing) policy restrictions. Consequently, the lottie animation fails to render even from cache.

When in offline, the loadFromUrl() function from the chunk-RRTNX2SD.js file is called from here.(App-main/node_modules/@dotlottie/react-player/dist/chunk-RRTNX2SD.js)
This function requires online connectivity to fetch the bufferArray from the path in order to load the animation.

As a potential solution for the web interface, when users go offline, you could render a fallback, such as a GIF image resembling the lottie animation, or simply leave it plain with a background color.

@marcochavezf
Copy link
Contributor

Thanks @fedirjh for the analysis and review of the proposals. After thinking a bit more about this issue, I agree that there's no consistent way to reproduce it on different devices. Also, given our current priorities, I'm going to lean towards closing it and re-opening it later if we think it's still important to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.
Projects
None yet
Development

No branches or pull requests

6 participants