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

CC: @rushatgabhane @mananjadhav @hungvu193 seen this before?

@trjExpensify haven't seen before but i can reproduce this janky-ness

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

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

Let's send it external then!

Proposal

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

There is a slight flashing/janky animation of the collapse component

What is the root cause of that problem?

The bug is in react-native-collapsible

There's a condition mismatch between https://github.com/oblador/react-native-collapsible/blob/2bd7f8d50c2f3d876119bc65da8e7681c38563ad/Collapsible.js#L188 and https://github.com/oblador/react-native-collapsible/blob/2bd7f8d50c2f3d876119bc65da8e7681c38563ad/Collapsible.js#L194.

The hasKnownHeight is used to render the parent view with fixed height or not. So when hasKnownHeight is false initially when collapsed becomes false, there will be no style here. At the same time, the measuring doesn't start yet, so the measuring here is false, so the content becomes rendered.

Right after that the measuring becomes true and the content becomes hidden, then after measuring completed, the content becomes visible again.

So the root cause is that although the hasKnownHeight is false (the parent view is not ready to be shown yet), we're still showing the content (because measuring didn't start).

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

Make the condition between https://github.com/oblador/react-native-collapsible/blob/2bd7f8d50c2f3d876119bc65da8e7681c38563ad/Collapsible.js#L188 and https://github.com/oblador/react-native-collapsible/blob/2bd7f8d50c2f3d876119bc65da8e7681c38563ad/Collapsible.js#L194 consistent.

If hasKnownHeight is false (the parent view is not ready to be shown yet), the content should also not show.

This condition can be updated to:

if (!hasKnownHeight) {

The !hasKnownHeight condition is inclusive of the existing (measuring) condition because if measuring is true, hasKnownHeight will be false and !hasKnownHeight will also be true. So this will not cause any problem.

To test this locally, please modify the same condition in node_modules/react-native-collapsible/Collapsible.js

What alternative solutions did you explore? (Optional)

Update the logic here https://github.com/oblador/react-native-collapsible/blob/2bd7f8d50c2f3d876119bc65da8e7681c38563ad/Collapsible.js#L189-L192 so if hasKnownHeight is false, the parent view will have invisible style

const style = hasKnownHeight ? {
      overflow: 'hidden',
      height: height,
    } : {
      overflow: 'hidden',
      height: 0,
    };

Or just

const style = {
      overflow: 'hidden',
      height: height,
    };

As when hasKnownHeight is false, the height will also be 0

Proposal

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

The problem is that when a user navigates to "Other integrations" in the accounting section of the workspace, the animation is not smooth. Instead, there is a slight flashing animation.

What is the root cause of that problem?

The root cause of the janky animation could be due to:

1. Inefficient Rendering: Inefficient rendering or re-rendering of components causing the animation to lag.
2. Heavy Components: Heavy components or operations being loaded synchronously, which blocks the main thread and causes animation to stutter.
3. CSS/Style Issues: Improper use of CSS or style properties that interfere with smooth animations.

What changes do you think we should make in order to solve the problem?
To address this issue, we need to:

1. Optimize Rendering:
   • Ensure that components are rendered efficiently, possibly using React.memo or PureComponent to avoid unnecessary re-renders.
2. Asynchronous Loading:
   • Load heavy components or data asynchronously to prevent blocking the main thread during animations.
3. CSS/Style Improvements:
   • Review and optimize CSS or style properties used for animations to ensure smooth transitions.

Example of changes:

1. Optimize Rendering:

   • Use React.memo or PureComponent to prevent unnecessary re-renders.

   const OtherIntegrations = React.memo(() => {
       return (
           <View>
               {/* Other integration components */}
           </View>
       );
   });

2. Asynchronous Loading:

   • Load heavy components or data asynchronously.

   useEffect(() => {
       const loadHeavyComponents = async () => {
           await loadData();
       };
   
       loadHeavyComponents();
   }, []);

3. CSS/Style Improvements:

   • Ensure CSS animations are optimized and using properties that can be hardware accelerated, like transform and opacity.

   .integration-animation {
       transition: transform 0.3s ease-in-out, opacity 0.3s ease-in-out;
   }

Alternative solutions ?

1. Performance Profiling: Use performance profiling tools to identify and address specific bottlenecks causing the animation to jank.
2. Virtualization: Implement virtualization for lists or heavy components to load only visible items, reducing the rendering load.
3. Animation Libraries: Utilize animation libraries like Lottie or React Native Reanimated to handle complex animations more efficiently.

Please note that the above explanation outlines only the technical approach I plan to take. If my proposal gets accepted, I will submit the finalized solution, as mentioned.

Thanks!

@Pujan92 can you take a look at the proposals here?

As @truph01's mentioned in their proposal the bug seems to be in react-native-collapsible. A similar issue oblador/react-native-collapsible#473 is raised in the same repo.

I agree with @truph01's RCA where due to those conditions the content gets rendered before the parent View style has been assigned. I like their alternative solution to provide overflow: 'hidden' in each cases for the parent to avoid overflowing the content. I believe hasKnownHeight part can be omitted.
For this we need to raise a PR in react-native-collapsible and I think the reviewer will help there in case hasKnownHeight is required.

🎀👀🎀 C+ reviewed

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

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

@neil-marcellini - can you take a quick look at this one?

As @truph01's mentioned in their proposal the bug seems to be in react-native-collapsible. A similar issue oblador/react-native-collapsible#473 is raised in the same repo.

I agree with @truph01's RCA where due to those conditions the content gets rendered before the parent View style has been assigned. I like their alternative solution to provide overflow: 'hidden' in each cases for the parent to avoid overflowing the content. I believe hasKnownHeight part can be omitted. For this we need to raise a PR in react-native-collapsible and I think the reviewer will help there in case hasKnownHeight is required.

🎀👀🎀 C+ reviewed

Sounds good, hiring @truph01

📣 @Pujan92 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

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

@Pujan92 @neil-marcellini With this issue, should I create a pull request (PR) to fix the library directly or create a patch package?

@Pujan92 Do you have any updates here?

I got a response on 25th July -- I am, but on another business trip 🙈 Would it help if I pushed a branch where I reproduce the blinking?

I have requested them to share the branch once they push, but seems they are quite busy so let's wait.

@Pujan92 Did the author respond?

Any update @Pujan92?

@Pujan92 If the author is not available to review the PR, we can consider creating a patch. What do you think?

Yes, we can consider that option as PR review in lib is taking a long. WDYT @neil-marcellini?

What do you think about this idea @neil-marcellini ?

Still waiting for a response from @neil-marcellini.

Yeah let's do a patch since it's been a while and the maintainer is unresponsive, please go ahead and create a PR.

Lib PR gets merged, @truph01 We can raise a PR with a version bump.

@truph01 Seems our changes are causing an issue oblador/react-native-collapsible#479 where the content isn't rendered if the initial value for the expanded is true, Let's try to find and fix it.

@Pujan92 I am investigating the RCA

Update: I am trying to find the RCA. Also, I tried applying other solutions mentioned in here but still encountered different problems.

PR was deployed to production on 31/8/2024, maybe Melvin-bot does not work in this case.

Ok, thanks! Just clarifying, this PR fixed the bug right? And then we take this other one off hold?

Payment summary:

• Contributor: @truph01 $250 (paid in Upwork)
• Reviewer: @Pujan92 $250 (please request in NewDot)

I just want to point out: To fix this issue, we need to submit a PR to the react-native-collapsible library. Once that PR is merged, our Expensify app work correctly. However, other users who use the react-native-collapsible library encountered a new issue when upgrading the lib's version to 1.6.2 and react-native's version to 0.74.5, and we are still investigating the root cause along with other users.

$250 approved for @Pujan92