fix(android): Unable to update properties for view tag#5216

Closed

ngocle2497 wants to merge 3 commits intosoftware-mansion:mainfrom

ngocle2497:fix/android_update_props

ngocle2497 commented Oct 11, 2023

Summary

Platform: Android

I catch this issue when use FlashList with large data and have animated update on each item.
One element can be update/replace/recycle, so view tag can be update/remove too
So, before update props, we check current view tag exist in UIManager or not.

Fix #4505
Fix #4231
Fix #3860

Test plan

Before:

before.mp4

After:

after.mp4


          fix(android): Unable to update properties for view tag

857e55b

Contributor

efstathiosntonas commented Oct 11, 2023 •

edited

Loading

Hi @MasonLe2497, Log needs to be imported:

NodesManager.java:356: error: cannot find symbol Log.d("Reanimated-updateProps", "Skip update props cause viewTag not exist or have been removed!");

Member

tomekzaw commented Oct 11, 2023

@MasonLe2497 Thanks for submitting the PR! I agree we should do something about this error. Instead of checking if the view exists before updating props, maybe we could wrap mUIImplementation.synchronouslyUpdateViewOnUIThread and mUIManager.updateView calls into try/catch blocks? This way we can also avoid the error in case when the view exists when calling updateProps on the UI thread but no longer exists when calling mUIManager.updateView on the native modules thread.

Author

ngocle2497 commented Oct 11, 2023

@MasonLe2497 Thanks for submitting the PR! I agree we should do something about this error. Instead of checking if the view exists before updating props, maybe we could wrap mUIImplementation.synchronouslyUpdateViewOnUIThread and mUIManager.updateView calls into try/catch blocks? This way we can also avoid the error in case when the view exists when calling updateProps on the UI thread but no longer exists when calling mUIManager.updateView on the native modules thread.

Ok. I'll try tomorrow.(from UTC +7:00)


          add missing import Log

5d7b34c

Author

ngocle2497 commented Oct 12, 2023

@tomekzaw i tried to wrap try/catch to mUIImplementation.synchronouslyUpdateViewOnUIThread, but i still catch the issue.
I think synchronouslyUpdateViewOnUIThread run on different thread, so we cant catch nothing here

Screen.Recording.2023-10-12.at.09.19.45.mov

Author

ngocle2497 commented Oct 12, 2023

Hi @MasonLe2497, Log needs to be imported:

NodesManager.java:356: error: cannot find symbol Log.d("Reanimated-updateProps", "Skip update props cause viewTag not exist or have been removed!");

Thanks, I just update my PR

Latropos added Important and removed Important labels

ranaavneet commented Nov 17, 2023

Hey @tomekzaw, is there anything else we can do about this?

thomazcapra commented Nov 28, 2023

👀

damianbeles commented Jan 26, 2024

@tomekzaw any update?

tomekzaw requested changes

View reviewed changes

android/src/main/java/com/swmansion/reanimated/NodesManager.java

Comment on lines +347 to +349

+                  // We need to check current view exist in UIManager or not
+                  // Sometime, view can be replace, recycle, ... with wrong way (like FlashList)
+                  // So before update, We need to check view tag exist in UIManager

Member

tomekzaw Jan 26, 2024

Let's rephrase this comment

android/src/main/java/com/swmansion/reanimated/NodesManager.java

Comment on lines +352 to +354

+                      if(view == null) {
+                        return;
+                      }

Member

tomekzaw Jan 26, 2024

Suggested change

      
                    if(view == null) {
          
                      return;
          
                    }
          
                    if (view == null) {
          
                      return;
          
                    }

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+                      if(view == null) {
+                        return;
+                      }
+                  } catch (Exception ex) {

Member

tomekzaw Jan 26, 2024

Suggested change

      
                } catch (Exception ex) {
          
                } catch (Exception e) {

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+                        return;
+                      }
+                  } catch (Exception ex) {
+                      Log.d("Reanimated-updateProps", "Skip update props cause viewTag not exist or have been removed!");

Member

tomekzaw Jan 26, 2024

Let's remove this warning so that we don't pollute the logs.

Suggested change

      
                    Log.d("Reanimated-updateProps", "Skip update props cause viewTag not exist or have been removed!");

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+                      }
+                  } catch (Exception ex) {
+                      Log.d("Reanimated-updateProps", "Skip update props cause viewTag not exist or have been removed!");
+                      return;

Member

tomekzaw Jan 26, 2024

Can you please describe in which cases the exception will be thrown?

damianbeles commented Jan 29, 2024

@ngocle2497 do you plan to review the change requests? We really need this in prod. If not, let me know and I'll do it.

This comment was marked as resolved.

Sign in to view

joe-sam mentioned this pull request

[3.7.0] Data type not recognized by value unpacker. at valueUnpacker (WorkletRuntime::WorkletRuntime:1:1477) #5660

Closed


          Merge branch 'main' into fix/android_update_props

39e4d96

thomas-rx mentioned this pull request

Prevent crash on non-existent view updates in Android #5767

Merged

Contributor

thomas-rx commented Mar 7, 2024

@damianbeles @ngocle2497 I opened ticket #5767 to address that important error.

github-merge-queue bot pushed a commit that referenced this pull request


           Prevent crash on non-existent view updates in Android (#5767)

4cdfe93

<!-- Thanks for submitting a pull request! We appreciate you spending
the time to work on these changes. Please follow the template so that
the reviewers can easily understand what the code changes affect. -->

## Summary

I'm opening this PR to address a critical issue previously tackled by PR
#5216, which has stalled due to the original author's inactivity.

This fix is crucial for resolving significant bugs affecting Android
view property updates (referenced in issues #4505, #4231, #3860) that
impact application stability and performance.

Typical error : 
```unknown:NativeViewHierarchyManager: Unable to update properties for view tag 943
03-07 19:54:33.543 19722 19722 E unknown:NativeViewHierarchyManager: com.facebook.react.uimanager.IllegalViewOperationException: ViewManager for tag 943 could not be found.
```

## Test plan

<!-- Provide a minimal but complete code snippet that can be used to
test out this change along with instructions how to run it and a
description of the expected behavior. -->

### Before patching

https://github.com/software-mansion/react-native-reanimated/assets/40337934/a7cd7f8c-c0a3-4a84-98f7-4853886799f0

### After patching

https://github.com/software-mansion/react-native-reanimated/assets/40337934/c4dfe495-fed9-48cf-a227-e7768cec26cc

---------

Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>

Member

piaskowyk commented May 13, 2024

Thank you for your pull request and the time you've dedicated to it! I appreciate that 🙌. These changes have been implemented in this PR, so I'll close this one.

piaskowyk closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment