MM-59973: fix the emoji count 0 issue when archive and unarchive chan…

[Rajat-Dabade]

Summary

The PR fixes the bug count to be 0 for every reaction in the centre channel when archive and unarchive channels.

Ticket Link

https://mattermost.atlassian.net/browse/MM-59973

Device Information

IOS, Andriod

Screenshots/ Screen Recording

Before:

Screen.Recording.2024-08-13.at.2.03.15.PM.mov

After:
https://github.com/user-attachments/assets/5f07acb0-6d65-4228-9966-378e905ff725

Release Note

Fixed the bug for Post in archived channel shows emoji reactions with count `0` instead of the actual count

[enahum]

can you explain the change? why the memo is not working while the ref does?

more importantly, why does the count goes to 0 when archived in the original code?

basically what I see happening is that you are now calculating the count on every render, this would be "fine" if we can measure the overall impact on doing so.

as far as I can tell, when archiving, something is changing causing the reaction count to not match the memo and then returns 0 while unarchiving does not run the memo function again, and I want to understand the reason why that is happening and not only just changing it to a ref, which in turn it is not even needed, you could just remove the ref and calculate the value directly

[larkox]

Looking at the code, the original useMemo was not doing anything at all, since numberStringToDigitArray was being created by Array.from, which would generate a new array on each render.

The proposal you are making is just sticking to the original value. I wonder if adding or removing a reaction will properly show it in your solution.

Have you checked the value of animateToNumber is reaching this component correctly? My gut feeling says that the problem is in a higher level component not sending the correct value, but 0/5.

[Rajat-Dabade]

The issue that I think for the original implementation using useMemo was that re-calculation of animations when numberStringToDigitsArray changes (as it is a dependency for useMemo), which in turn could result in incorrect initialisation when numberheight was 0. This may cause all digits to reset to 0 because animationHeight was calculated as 0.

Switching to useRef ensures that the animations array is initialised only once and can persist across renders. I agree that we could calculate the values directly without useRef and this approach would involve ensuring that height is correctly set before calculations. Do you have any thoughts on this @enahum, should we go for direct calculation 0/5 on this?

@larkox First I thought the same, but when I checked the value for animateToNumber the value was correct reaching to this component. It's not an issue with the higher-level component.

[larkox]

Thanks for the explanation @Rajat-Dabade . I have to admit this component is complex, and I struggle to grasp everything that is happening. I see some "weird things" but not sure if they are intended or not.

For starters, what I already said, the useMemo is doing nothing. Then we have an effect that start the animations, but the dependency array seems to be all over the place. Changes in the animations themselves does not run it, and it depends on fontStyle that apparently does not have anything to do with it.

Also, what I don't get from your explanation, is how having a numberHeight of 0 will set the numbers to 0. I would expect some visual artifact, but not the numbers changing to 0. Again, I may be misunderstanding part of the component.

Also, numberStringToDigitsArray should be changing on every render, so not sure why the error comes only when the channel gets archived / unarchived.

[Rajat-Dabade]

@larkox, Thanks for the feedback. So the animationHeight for each digit is calculated as -1 ( numberHeight ( prevNumberArr[index]. As numberHeight is 0 initially, this results in animationHeight being 0, regardless of the animateToNumber value.

What I think is each digit's Animated.Value is initialized with animationHeight. If animationHeight is 0, this means that all the digits are placed at the top position in the list of numbers (0-9) that are being animated. Since all the digits are positioned at the top, it looks like every number has turned into 0, even though they haven’t. Any thoughts on this?

[yasserfaraazkhan]

The count did not check after archiveing a channel on mobile.
Tested on iOS and Android device.

[rahimrahman]

Unrelated to the problem here, but I'm looking at this component and it is kinda concerning at how many Views this component will be generating. If the number is 999, it creates 3 x 2 (Animated.View & parent view) x 10 (number views) which is 60. If it goes up to 1000, the views will be 80.

That many views to make 1 animated number is quite a lot. If you have 10 emojis, 80 x 10 = 800. If you have 100 emojis (I hope nobody ever have 100 emojis attached to a post), it'll be 8000.

Can I make recommendation to have community re-write this?

[rahimrahman]

I think there could be a few cleanup in this component.

I've expressed a major concern with this component, but probably out of scope for this PR/ticket.

[rahimrahman]

nit: should useRef be added into import on line 4 instead of using via namespace?

Could also clean up the rest of the react hook functions (useEffect, useState, etc)

[rahimrahman]

isn't numberHeight always 0 until setButtonLayout is being called? So this if block will return true thus animations will be an array with all the elements having the value of new Animated.Value(0), I think?

If this is the case, could just make it

  useRef(numberStringToDigitsArray.fill(new Animated.Value(0));

I believe the useEffect on line 54 is what updates the value depending on the number of the digit.

[rahimrahman]

@larkox is correct, the useMemo will be triggered every time the component re-renders since the dependency is an object (array). If you switch that to a string animateToNumberString, that would solved the problem without having to switch to useRef.

But the useMemo has a few dependencies missing: numberHeight, and prevNumberersArr. Since prevNumberersArr is an array, we need to use prevNumberString.

    // if wanting to keep useMemo()
    const animations = useMemo(() => {
        return Array.from(animateToNumberString, Number).map((__, index) => {
            const prevNumberArr = Array.from(prevNumberString, Number);

            if (typeof prevNumberArr[index] !== 'number') {
                return new Animated.Value(0);
            }

            const animationHeight = -1 * (numberHeight * prevNumberArr[index]);

            return new Animated.Value(animationHeight);
        });
    }, [animateToNumberString, numberHeight, prevNumberString]);

I think. xD

[Rajat-Dabade]

Unrelated to the problem here, but I'm looking at this component and it is kinda concerning at how many Views this component will be generating. If the number is 999, it creates 3 x 2 (Animated.View & parent view) x 10 (number views) which is 60. If it goes up to 1000, the views will be 80.
That many views to make 1 animated number is quite a lot. If you have 10 emojis, 80 x 10 = 800. If you have 100 emojis (I hope nobody ever have 100 emojis attached to a post), it'll be 8000.
Can I make recommendation to have community re-write this?

@rahimrahman I see, If this is the case then I guess rewriting the component will make more sense than updating it.

[rahimrahman]

@Rajat-Dabade In my opinion, the fix is something that is needed soon-ish, whereas the re-write is my recommendation, and might be added in the roadmap in the future, or might not be done at all. The way it is working now is a concern of mine, because it might impact performance, but we will need more proof.

[enahum]

I agree with @rahimrahman about the re-write, but not really sure if that is needed as today is working with this code and does not seem to be a major problem.

other than the comments that already were made, this lgtm

[larkox]

Just a minor improvement from my side.

[rahimrahman]

@Rajat-Dabade I use AnimatedNumber component as a topic of my presentation a few weeks ago, about how many Views this component is generating and the potential impact on performance. The demo was done using a new Expo app.

I couldn't use your changes because it was crashing, and I didn't have the time to figure out why (at that time) and I completely forgot about it after that, but seeing this PR discussion started up again, it reminded me that there was an issue and glad you were able to fix crash.

But, since I had that Expo app created already and your recent changes fixed the crash, I ran a comparison between the original and your changes today. See the two videos:

original.mp4

after-fix.mp4

You can definitely see the lag on after-fix video. While the demo in these videos are not a real use case of what we have for this component, I think it's indicative of a potential performance impact. If you choose to continue with useRef, perhaps you could figure out why it's causing a lag in this stress test environment.

Since useMemo() is the faster alternative, I played around with it and found out the fix is really simple, which is adding all the missing dependencies in the useEffect.

    React.useEffect(() => {
        animations.forEach((animation, index) => {
           ...
        });
    }, [animationDuration, animations, easing, numberHeight, numberStringToDigitsArray]); // <=== as far as I can tell, adding missing dependencies here fixes the original problem

But another problem with the dependencies above, is easing, numberStringToDigitsArray and animations are of type function and array (non-primitive). So, it's possible that the useEffect will get triggered by any of the above even if the array content or function didn't change.

Another issue is the use of useMemo and useEffect is redundant. I combined the two into:

 const animations = useMemo(() => {
    const _numberStringToDigitsArray = Array.from(
      animateToNumberString,
      Number
    );
    const prevNumberString = String(Math.abs(prevNumber));
    const prevNumberersArr = Array.from(prevNumberString, Number);

    return _numberStringToDigitsArray.map((__, index) => {
      const animationHeight = -1 * (numberHeight * prevNumberersArr[index]);

      const animation =
        typeof prevNumberersArr[index] === "number"
          ? new Animated.Value(animationHeight)
          : new Animated.Value(0);

      Animated.timing(animation, {
        toValue: -1 * (numberHeight * _numberStringToDigitsArray[index]),
        duration: animationDuration || 1400,
        useNativeDriver: true,
        easing: Easing.elastic(easing || 1.2),
      }).start();

      return animation;
    });
  }, [
    animationDuration,
    easing,
    numberHeight,
    animateToNumberString,
    prevNumber,
  ]);

This is just a quick refactor, and I think further improvement can be made. As an example, moving from 20 => 21, the first digit 2 doesn't have to be re-calculated, since that didn't change, so no need to "animate" it (add if block to not add Animated.timing().start().

Also, notice that the dependencies are all primitive (so it can do a shallow comparison). I moved all of the conversion into array inside the useMemo block. I also change the props easing to a number instead of a function. I didn't see easing being passed in any of the AnimatedNumber component usage, so this change should be safe (but please check to make sure).

I know my respond here is pretty lengthy, but I hope it make sense.

Also, while it's not yet a common practice here at MM (even though we just talked about this last week during dev meeting), I encourage creating a proper component unit test before any refactoring, to ensure the changes doesn't break any other functionalities of the AnimatedNumber component.

[Rajat-Dabade]

@rahimrahman, Thanks for the detailed feedback and for highlighting the potential performance issue with the number of Views generated and unnecessary re-animations. I appreciate the insights from your demo!.

I've implemented your suggested changes, including:

• Refactoring to use useMemo for creating animations, completely removing the useEffect as the useEffect block would trigger after each render, recalculating and re-animating the Animated.Value instances regardless of whether the animation needed to change.
• Handling the condition where digits remain the same between the previous and current numbers, to avoid animating unchanged digits.

By introducing these changes, it should now only animate the digits that have actually changed. This reduces the number of unnecessary animations, and I expect it to help with performance, especially in edge cases where digits remain constant during number transitions.

I've also updated the easing prop to accept a number since it isn’t passed as a function in any usage scenarios.

Could you please take another look and let me know if there is anything I have left?

It's a good learning 🚀 .

Thank you.

[rahimrahman]

1. Could this cause issues when prevNumber = 99 and animateToNumber = 100? In this case, numberStringToDigitsArray = [1, 0, 0], but prevNumberersArr = [9, 9]. On the first digit, prevNumberersArr[0] !== 1, so the condition will fail. Could this behavior cause issues during a rapid animation test (e.g., moving +1 every 100ms)?

2. (NIT) This comment seems redundant since the logic is already clear from the code itself

             if (prevNumberersArr[index] === digit) {

Following the DRY principle, you could remove it. However, I think a comment explaining the next line

              // skipping any animation by just returning the current "position"
              return new Animated.Value(-1 * (numberHeight * digit));

...could add value.

[Rajat-Dabade]

Yes, this condition could indeed cause an issue when transitioning from 99 to 100. The problem arises because we skip the animation when the digits are the same. For example:

• prevNumberersArr = [9, 9]
• numberStringToDigitsArray = [1, 0, 0]

When comparing the first digit (1 vs 9), the condition prevNumberersArr[0] === digit will fail, which is expected, and the animation will proceed for the first digit. However, for the second and third digits, the condition prevNumberersArr[1] === digit will compare 9 (previous) to 0 (current), which is not equal, so those digits will still be animated, which is correct.

The issue is not necessarily in this block itself, but the logic could behave unexpectedly if we are rapidly changing values (e.g., every 100ms). Skipping digits could lead to an incorrect visual transition, especially when the change is rapid, such as going from 99 to 100.

To ensure the conditions for rapid changes are avoided we can add an extra condition for checking whether the length of the prevNumberersArr and numberStringToDigitsArray are equal or not which will ensure that the animation isn't skipped due to rapid changes.

[rahimrahman]

prevNumber is not a dependency of this useMemo but prevNumberString is. I know they are both related, but for the sake of not triggering "exhaustive-deps" warning (it is currently disabled), I recommend using the correct dependency.

[Rajat-Dabade]

Updated the dependency.

[rahimrahman]

Q: would it be safer to return

    return numberStringToDigitsArray.fill(new Animated.Value(0));

just to protect from future regression?

Could transform.translateY: undefined

                              translateY: animations[index],

cause any issue if index is out of bounds due to animations being empty.

[Rajat-Dabade]

Yes, but I think numberStringToDigitsArray is a number array and cannot be filled with Value type. Maybe map can work here to return the new array filled with Animated.Value(0).

[yasserfaraazkhan]

thank you Rajat

[Rajat-Dabade]

/update-branch