[Gekidou] Add performance and code improvements around post_list #6113

larkox · 2022-03-31T15:56:45Z

Summary

Add several performance and code improvements.

Wait for comments for more details.

Release Note

None

enahum · 2022-03-31T16:00:46Z

app/utils/role/index.ts

-    if (myChannel) {
-        rolesArray.push(...myChannel.roles.split(' '));
-    }
+export function observePermissionForChannel(channel: ChannelModel, user: UserModel, permission: string, defaultValue: boolean) {


this is nice, but I think we should move it to the queries folder and not in utils

I was thinking the same. Will do.

enahum

do we need to add strictMode to <AnimatedFlatList> and other FlatLists?

enahum · 2022-03-31T16:02:51Z

app/components/post_list/index.tsx

-            } else if (offsetY === 0 && y !== 0) {
-                setOffsetY(y);
-            }
+            setEnableRefreshControl(y === 0);


larkox

Comments on the changes

larkox · 2022-03-31T15:58:32Z

app/components/post_draft/uploads/index.tsx

@@ -80,7 +80,7 @@ function Uploads({
    const style = getStyleSheet(theme);

    const errorHeight = useSharedValue(ERROR_HEIGHT_MIN);
-    const containerHeight = useSharedValue(CONTAINER_HEIGHT_MAX);
+    const containerHeight = useSharedValue(files.length ? CONTAINER_HEIGHT_MAX : CONTAINER_HEIGHT_MIN);


Not sure what changed, but this was not being updated on the effect on the first render. Now it is initialized to the correct value.

The result was that when opening the channel, the height was as if a file was uploaded.

how did you find out about this ?

I don't know exactly the details. I was moving things around, and suddenly (I don't realize what the change must be) I started seeing the input box higher than usual. I realized it was the space of the uploads (it was not part of the textbox and was not reactive to clicks).
Changing the initial value (which makes sense by the way) fixed it.

larkox · 2022-03-31T15:59:23Z

app/components/post_list/index.tsx

@@ -99,7 +99,7 @@ const PostList = ({
    const onScrollEndIndexListener = useRef<onScrollEndIndexListenerEvent>();
    const onViewableItemsChangedListener = useRef<ViewableItemsChangedListenerEvent>();
    const scrolledToHighlighted = useRef(false);
-    const [offsetY, setOffsetY] = useState(0);
+    const [enableRefreshControl, setEnableRefreshControl] = useState(false);


No need for a number, a boolean is enough for what we use it, and makes it easier to work with.

larkox · 2022-03-31T15:59:56Z

app/components/post_list/index.tsx

-            } else if (offsetY === 0 && y !== 0) {
-                setOffsetY(y);
-            }
+            setEnableRefreshControl(y === 0);


Depending on the offset was forcing us to recreate the onScroll callback. This allow us to avoid that, reducing the number of re-renders.

larkox · 2022-03-31T16:01:05Z

app/components/post_list/post/index.ts

-    let threadCreatedByCurrentUser = false;
-    let rootPost: PostModel | undefined;
-    const myPosts = await queryPostsBetween(postsInThread.database, postsInThread.earliest, postsInThread.latest, null, currentUser.id, '', post.rootId || post.id).fetch();
+function observeShouldHighlightReplyBar(currentUser: UserModel, post: PostModel, postsInThread: PostsInThreadModel) {


We are removing most of the from$ by using observers instead of fetches. This should be more reactive and robust.

larkox · 2022-03-31T16:05:05Z

app/screens/post_options/index.ts

-        permissions.push(Permissions.EDIT_OTHERS_POSTS);
-    }
-
-    cep = permissions.every((permission) => hasPermissionForChannel(channel, user, permission, false));


I feel this was not working at all (it was always returning true, since every element had a promise, which is distinct from null, therefore true). I tried to fix it.

larkox · 2022-03-31T16:07:27Z

patches/react-native+0.67.4.patch

+           prevCellKey={prevCellKey}
+          onCellLayout={this._onCellLayout}
+           onUpdateSeparators={this._onUpdateSeparators}
+-          onLayout={e => this._onCellLayout(e, key, ii)}


From facebook/react-native@19cf702
A new onLayout method was being passed on every render.

larkox · 2022-03-31T16:10:49Z

patches/react-native+0.67.4.patch

+ };
+
+-class CellRenderer extends React.Component<
+class CellRenderer extends React.PureComponent<


We use PureComponent to avoid re-renders. This has some creative convergence with facebook/react-native#31327

larkox · 2022-03-31T16:11:47Z

patches/react-native+0.67.4.patch

+-        leadingItem: props.item,
+-      },
+-    };
+    if (prevState.separatorProps.leadingItem !== props.item) {


Since separatorProps is an object, we only want to update it if it actually changed.

larkox · 2022-03-31T16:13:32Z

patches/react-native+0.67.4.patch

+       ...restProps
+     } = this.props;
+
+    const renderer = strictMode ? this._memoizedRenderer : this._renderer;


Applying facebook/react-native@c231d5e

larkox · 2022-03-31T16:14:29Z

patches/react-native-reanimated+2.5.0.patch

+         }
+       }
+-      return style;
+      if (changed) {


Avoid sending new styles if there are no changes, to avoid re-renders.

larkox · 2022-03-31T16:16:06Z

As far as I know, strictMode is not necessary to avoid the re-renders we were experiencing. Not sure if it will have any other upside down the road, but right now is not necessary.

larkox · 2022-03-31T17:09:05Z

app/components/channel_list/index.test.tsx

@@ -40,7 +44,12 @@ describe('components/channel_list', () => {
        expect(wrapper.toJSON()).toBeTruthy();
    });

-    it('should render team error', () => {
+    it('should render team error', async () => {


I think something was broken in the channel_list header index that was preventing the header from being rendered in the failure cases, that got fixed with the changes on the way we handle permissions.

These changes are to adapt to the test now showing the header, and showing it correctly.

enahum

@larkox let's merge #6109 and then merge gekidou on this branch so that we can build it for testing

mattermod · 2022-03-31T17:25:56Z

Building app in separate branch.

mattermod · 2022-03-31T17:30:16Z

Successfully building:
https://circleci.com/gh/mattermost/mattermost-mobile/21400
https://circleci.com/gh/mattermost/mattermost-mobile/21399

mattermod · 2022-03-31T19:43:35Z

Building app in separate branch.

mattermod · 2022-03-31T19:47:56Z

Successfully building:
https://circleci.com/gh/mattermost/mattermost-mobile/21406
https://circleci.com/gh/mattermost/mattermost-mobile/21405

mattermod · 2022-03-31T20:17:56Z

Artifact links:
https://output.circle-artifacts.com/output/job/dc7aba89-9740-4ef7-8b55-ff56eb42f9c3/artifacts/0/tmp/artifacts/Mattermost_Beta.ipa
https://output.circle-artifacts.com/output/job/73d04d40-7121-4f77-9d7c-f470893fd087/artifacts/0/tmp/artifacts/Mattermost_Beta.apk

mattermod · 2022-03-31T21:05:47Z

Building app in separate branch.

mattermod · 2022-03-31T21:09:48Z

Successfully building:
https://circleci.com/gh/mattermost/mattermost-mobile/21412
https://circleci.com/gh/mattermost/mattermost-mobile/21411

mattermod · 2022-03-31T21:34:49Z

Artifact links:
https://output.circle-artifacts.com/output/job/c91dbd10-8431-4c07-9aa7-3cf4dead9077/artifacts/0/tmp/artifacts/Mattermost_Beta.apk
https://output.circle-artifacts.com/output/job/30a6a95b-c0e6-4b79-a7ef-2aad39ab0164/artifacts/0/tmp/artifacts/Mattermost_Beta.ipa

larkox added 2 commits March 31, 2022 17:46

Add performance and code improvements around post_list

1665a9c

Merge branch 'gekidou' into PostListImprovements

6a64721

larkox added the 2: Dev Review Requires review by a core commiter label Mar 31, 2022

larkox requested review from enahum and avinashlng1080 March 31, 2022 15:56

mm-cloud-bot added the release-note-none label Mar 31, 2022

enahum reviewed Mar 31, 2022

View reviewed changes

enahum requested changes Mar 31, 2022

View reviewed changes

larkox commented Mar 31, 2022

View reviewed changes

Fix test

777727b

larkox commented Mar 31, 2022

View reviewed changes

Move observers from utils to queries

97ca4ea

larkox requested a review from enahum March 31, 2022 17:15

enahum approved these changes Mar 31, 2022

View reviewed changes

Merge branch 'gekidou' into PostListImprovements

189281e

larkox added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

remove Flipper on iOS to fix CI build

77bd202

enahum added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

Fix observePermissionForChannel for DM/GM

0d975bf

enahum added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2022

avinashlng1080 approved these changes Apr 1, 2022

View reviewed changes

avinashlng1080 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Apr 1, 2022

avinashlng1080 assigned larkox Apr 1, 2022

enahum merged commit d1322e8 into mattermost:gekidou Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gekidou] Add performance and code improvements around post_list #6113

[Gekidou] Add performance and code improvements around post_list #6113

larkox commented Mar 31, 2022

enahum Mar 31, 2022

larkox Mar 31, 2022

enahum left a comment

enahum Mar 31, 2022

larkox left a comment

larkox Mar 31, 2022

larkox Mar 31, 2022

avinashlng1080 Apr 1, 2022

larkox Apr 1, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox Mar 31, 2022

larkox commented Mar 31, 2022

larkox Mar 31, 2022

enahum left a comment

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

[Gekidou] Add performance and code improvements around post_list #6113

[Gekidou] Add performance and code improvements around post_list #6113

Conversation

larkox commented Mar 31, 2022

Summary

Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox commented Mar 31, 2022

Choose a reason for hiding this comment

enahum left a comment

Choose a reason for hiding this comment

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022

mattermod commented Mar 31, 2022