Page MenuHomePhabricator

[native] Replace `componentDidUpdate` with `useEffect`s in `ConnectedChatThreadList`
ClosedPublic

Authored by atul on Sep 13 2023, 10:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 4, 6:10 AM
Unknown Object (File)
Wed, Sep 11, 6:24 PM
Unknown Object (File)
Wed, Sep 11, 6:24 PM
Unknown Object (File)
Wed, Sep 11, 6:24 PM
Unknown Object (File)
Wed, Sep 11, 6:23 PM
Unknown Object (File)
Wed, Sep 11, 6:17 PM
Unknown Object (File)
Aug 26 2024, 8:28 AM
Unknown Object (File)
Aug 11 2024, 2:03 AM
Subscribers
None

Details

Summary

Replace componentDidUpdate with three useEffects that should fire in the same conditions.

This diff is just one step in the process of converting ChatThreadList into a functional component. When this work is done, we will be able to avoid a lot of re-rendering and hopefully improve performance quite a bit.


Depends on D9189

Test Plan

ChatThreadList + search experience continue to work as expected.

Specifically, scrollToTop behavior was working as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Sep 13 2023, 11:07 AM
atul added 1 blocking reviewer(s): ashoat.
atul added inline comments.
native/chat/chat-thread-list.react.js
520–528 ↗(On Diff #31080)

This should be equivalent to the following:

ca06f1.png (1×1 px, 270 KB)

On mount: The initial value of searchCancelButtonOpen is 0, so the searchCancelButtonOpen.setValue(0) in the else case should be a noop.

This effect will also fire on subsequent changes to isActiveOrActivating.

My understanding is that searchCancelButtonOpen is referentially equal throughout, so it shouldn't cause the useEffect to fire.

530–535 ↗(On Diff #31080)

This should be equivalent to the following:

f28e55.png (1×1 px, 256 KB)

On mount: The initial value of scrollPos.current is 0 so the condition should evaluate to false and nothing should happen.

This effect will also fire on subsequent changes to searchText which is equivalent to previous behavior.

536–544 ↗(On Diff #31080)

This should be equivalent to the following:

fa1071.png (1×1 px, 256 KB)

On mount: The initial value of searchStatus is "inactive" so the condition should evaluate to false and nothing should happen.

This effect will fire when searchStatus changes. Specifically, when it changes and the new value is "activating". The previous logic checked that the previous searchStatus was "inactive", but I think this is fine as it's only possible for searchStatus to change to "activating" "from" "inactive". It's NOT possible to go from "active" to "activating".

ashoat added inline comments.
native/chat/chat-thread-list.react.js
536–544 ↗(On Diff #31080)

Seems a bit brittle, but okay. Only request is to extract searchStatus === 'activating' outside the effect so we can skip running the effect when eg. searchStatus changes from active to inactive. Not a big deal but slight perf improvement

This revision is now accepted and ready to land.Sep 13 2023, 1:42 PM
This revision was landed with ongoing or failed builds.Sep 13 2023, 2:56 PM
This revision was automatically updated to reflect the committed changes.