Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Apr 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1665a9c
Add performance and code improvements around post_list
larkox Mar 31, 2022
6a64721
Merge branch 'gekidou' into PostListImprovements
larkox Mar 31, 2022
777727b
Fix test
larkox Mar 31, 2022
97ca4ea
Move observers from utils to queries
larkox Mar 31, 2022
189281e
Merge branch 'gekidou' into PostListImprovements
larkox Mar 31, 2022
77bd202
remove Flipper on iOS to fix CI build
enahum Mar 31, 2022
0d975bf
Fix observePermissionForChannel for DM/GM
enahum Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/components/autocomplete/at_mention/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@

import {withDatabase} from '@nozbe/watermelondb/DatabaseProvider';
import withObservables from '@nozbe/with-observables';
import {of as of$, from as from$, combineLatest, Observable} from 'rxjs';
import {of as of$, combineLatest, Observable} from 'rxjs';
import {switchMap} from 'rxjs/operators';

import {Permissions} from '@constants';
import {observeChannel} from '@queries/servers/channel';
import {observeLicense} from '@queries/servers/system';
import {observeCurrentUser} from '@queries/servers/user';
import {hasPermissionForChannel} from '@utils/role';
import {observePermissionForChannel} from '@utils/role';

import AtMention from './at_mention';

Expand All @@ -28,9 +28,9 @@ const enhanced = withObservables([], ({database, channelId}: WithDatabaseArgs &
let useGroupMentions: Observable<boolean>;
if (channelId) {
const currentChannel = observeChannel(database, channelId);
useChannelMentions = combineLatest([currentUser, currentChannel]).pipe(switchMap(([u, c]) => (u && c ? from$(hasPermissionForChannel(c, u, Permissions.USE_CHANNEL_MENTIONS, false)) : of$(false))));
useChannelMentions = combineLatest([currentUser, currentChannel]).pipe(switchMap(([u, c]) => (u && c ? observePermissionForChannel(c, u, Permissions.USE_CHANNEL_MENTIONS, false) : of$(false))));
useGroupMentions = combineLatest([currentUser, currentChannel, hasLicense]).pipe(
switchMap(([u, c, lcs]) => (lcs && u && c ? from$(hasPermissionForChannel(c, u, Permissions.USE_GROUP_MENTIONS, false)) : of$(false))),
switchMap(([u, c, lcs]) => (lcs && u && c ? observePermissionForChannel(c, u, Permissions.USE_GROUP_MENTIONS, false) : of$(false))),
);
} else {
useChannelMentions = of$(false);
Expand Down
162 changes: 162 additions & 0 deletions app/components/channel_list/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,138 @@ exports[`components/channel_list should render channels error 1`] = `
}
}
>
<View
animatedStyle={
Object {
"value": Object {
"marginLeft": 0,
},
}
}
collapsable={false}
style={
Object {
"marginLeft": 0,
}
}
>
<View
style={
Object {
"alignItems": "center",
"flexDirection": "row",
"justifyContent": "space-between",
}
}
>
<View
accessible={true}
collapsable={false}
focusable={false}
onClick={[Function]}
onResponderGrant={[Function]}
onResponderMove={[Function]}
onResponderRelease={[Function]}
onResponderTerminate={[Function]}
onResponderTerminationRequest={[Function]}
onStartShouldSetResponder={[Function]}
style={
Object {
"opacity": 1,
}
}
>
<View
style={
Object {
"alignItems": "center",
"flexDirection": "row",
"justifyContent": "space-between",
}
}
>
<Text
style={
Object {
"color": "#ffffff",
"fontFamily": "Metropolis-SemiBold",
"fontSize": 28,
"fontWeight": "600",
"lineHeight": 36,
}
}
testID="channel_list_header.team_display_name"
>
Test Team!
</Text>
<View
style={
Object {
"marginLeft": 4,
}
}
>
<Icon
name="chevron-down"
style={
Object {
"color": "rgba(255,255,255,0.8)",
"fontSize": 24,
}
}
/>
</View>
</View>
</View>
<View
accessible={true}
collapsable={false}
focusable={true}
onClick={[Function]}
onResponderGrant={[Function]}
onResponderMove={[Function]}
onResponderRelease={[Function]}
onResponderTerminate={[Function]}
onResponderTerminationRequest={[Function]}
onStartShouldSetResponder={[Function]}
style={
Object {
"alignItems": "center",
"backgroundColor": "rgba(255,255,255,0.08)",
"borderRadius": 14,
"height": 28,
"justifyContent": "center",
"opacity": 1,
"width": 28,
}
}
>
<Icon
name="plus"
style={
Object {
"color": "rgba(255,255,255,0.8)",
"fontSize": 18,
}
}
/>
</View>
</View>
<Text
style={
Object {
"color": "rgba(255,255,255,0.64)",
"fontFamily": "OpenSans-SemiBold",
"fontSize": 11,
"fontWeight": "600",
"lineHeight": 16,
}
}
testID="channel_list_header.server_display_name"
>

</Text>
</View>
<View
style={
Object {
Expand Down Expand Up @@ -191,6 +323,36 @@ exports[`components/channel_list should render team error 1`] = `
}
}
>
<View
animatedStyle={
Object {
"value": Object {
"marginLeft": 0,
},
}
}
collapsable={false}
style={
Object {
"marginLeft": 0,
}
}
>
<Text
style={
Object {
"color": "rgba(255,255,255,0.64)",
"fontFamily": "OpenSans-SemiBold",
"fontSize": 11,
"fontWeight": "600",
"lineHeight": 16,
}
}
testID="channel_list_header.server_display_name"
>

</Text>
</View>
<View
style={
Object {
Expand Down
10 changes: 5 additions & 5 deletions app/components/channel_list/header/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

import {withDatabase} from '@nozbe/watermelondb/DatabaseProvider';
import withObservables from '@nozbe/with-observables';
import {combineLatest, of as of$, from as from$} from 'rxjs';
import {combineLatest, of as of$} from 'rxjs';
import {switchMap} from 'rxjs/operators';

import {Permissions} from '@constants';
import {observeCurrentTeam} from '@queries/servers/team';
import {observeCurrentUser} from '@queries/servers/user';
import {hasPermissionForTeam} from '@utils/role';
import {observePermissionForTeam} from '@utils/role';

import ChannelListHeader from './header';

Expand All @@ -21,15 +21,15 @@ const enhanced = withObservables([], ({database}: WithDatabaseArgs) => {
const currentUser = observeCurrentUser(database);

const canJoinChannels = combineLatest([currentUser, team]).pipe(
switchMap(([u, t]) => (t && u ? from$(hasPermissionForTeam(t, u, Permissions.JOIN_PUBLIC_CHANNELS, true)) : of$(false))),
switchMap(([u, t]) => (t && u ? observePermissionForTeam(t, u, Permissions.JOIN_PUBLIC_CHANNELS, true) : of$(false))),
);

const canCreatePublicChannels = combineLatest([currentUser, team]).pipe(
switchMap(([u, t]) => (t && u ? from$(hasPermissionForTeam(t, u, Permissions.CREATE_PUBLIC_CHANNEL, true)) : of$(false))),
switchMap(([u, t]) => (t && u ? observePermissionForTeam(t, u, Permissions.CREATE_PUBLIC_CHANNEL, true) : of$(false))),
);

const canCreatePrivateChannels = combineLatest([currentUser, team]).pipe(
switchMap(([u, t]) => (t && u ? from$(hasPermissionForTeam(t, u, Permissions.CREATE_PRIVATE_CHANNEL, false)) : of$(false))),
switchMap(([u, t]) => (t && u ? observePermissionForTeam(t, u, Permissions.CREATE_PRIVATE_CHANNEL, false) : of$(false))),
);

const canCreateChannels = combineLatest([canCreatePublicChannels, canCreatePrivateChannels]).pipe(
Expand Down
17 changes: 16 additions & 1 deletion app/components/channel_list/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import Database from '@nozbe/watermelondb/Database';
import React from 'react';
import {SafeAreaProvider} from 'react-native-safe-area-context';

import {SYSTEM_IDENTIFIERS} from '@constants/database';
import ServerDataOperator from '@database/operator/server_data_operator';
import {getTeamById} from '@queries/servers/team';
import {renderWithEverything} from '@test/intl-test-helper';
import TestHelper from '@test/test_helper';
Expand All @@ -13,9 +15,11 @@ import ChannelsList from './';

describe('components/channel_list', () => {
let database: Database;
let operator: ServerDataOperator;
beforeAll(async () => {
const server = await TestHelper.setupServerDatabase();
database = server.database;
operator = server.operator;

const team = await getTeamById(database, TestHelper.basicTeam!.id);
await database.write(async () => {
Expand All @@ -40,7 +44,12 @@ describe('components/channel_list', () => {
expect(wrapper.toJSON()).toBeTruthy();
});

it('should render team error', () => {
it('should render team error', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await operator.handleSystem({
systems: [{id: SYSTEM_IDENTIFIERS.CURRENT_TEAM_ID, value: ''}],
prepareRecordsOnly: false,
});

const wrapper = renderWithEverything(
<SafeAreaProvider>
<ChannelsList
Expand All @@ -52,7 +61,13 @@ describe('components/channel_list', () => {
</SafeAreaProvider>,
{database},
);

expect(wrapper.toJSON()).toMatchSnapshot();

await operator.handleSystem({
systems: [{id: SYSTEM_IDENTIFIERS.CURRENT_TEAM_ID, value: TestHelper.basicTeam!.id}],
prepareRecordsOnly: false,
});
});

it('should render channels error', () => {
Expand Down
6 changes: 3 additions & 3 deletions app/components/post_draft/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
import {withDatabase} from '@nozbe/watermelondb/DatabaseProvider';
import withObservables from '@nozbe/with-observables';
import React from 'react';
import {combineLatest, of as of$, from as from$} from 'rxjs';
import {combineLatest, of as of$} from 'rxjs';
import {switchMap} from 'rxjs/operators';

import {General, Permissions} from '@constants';
import {observeChannel} from '@queries/servers/channel';
import {queryDraft} from '@queries/servers/drafts';
import {observeConfigBooleanValue, observeCurrentChannelId} from '@queries/servers/system';
import {observeCurrentUser, observeUser} from '@queries/servers/user';
import {hasPermissionForChannel} from '@utils/role';
import {observePermissionForChannel} from '@utils/role';
import {isSystemAdmin, getUserIdFromChannelName} from '@utils/user';

import PostDraft from './post_draft';
Expand Down Expand Up @@ -50,7 +50,7 @@ const enhanced = withObservables([], (ownProps: WithDatabaseArgs & OwnProps) =>
switchMap((id) => observeChannel(database, id!)),
);

const canPost = combineLatest([channel, currentUser]).pipe(switchMap(([c, u]) => (c && u ? from$(hasPermissionForChannel(c, u, Permissions.CREATE_POST, false)) : of$(false))));
const canPost = combineLatest([channel, currentUser]).pipe(switchMap(([c, u]) => (c && u ? observePermissionForChannel(c, u, Permissions.CREATE_POST, false) : of$(false))));
const channelIsArchived = channel.pipe(switchMap((c) => (ownProps.channelIsArchived ? of$(true) : of$(c?.deleteAt !== 0))));

const experimentalTownSquareIsReadOnly = observeConfigBooleanValue(database, 'ExperimentalTownSquareIsReadOnly');
Expand Down
6 changes: 3 additions & 3 deletions app/components/post_draft/send_handler/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import {withDatabase} from '@nozbe/watermelondb/DatabaseProvider';
import withObservables from '@nozbe/with-observables';
import {combineLatest, of as of$, from as from$} from 'rxjs';
import {combineLatest, of as of$} from 'rxjs';
import {switchMap} from 'rxjs/operators';

import {General, Permissions} from '@constants';
Expand All @@ -12,7 +12,7 @@ import {observeChannel, observeCurrentChannel} from '@queries/servers/channel';
import {queryAllCustomEmojis} from '@queries/servers/custom_emoji';
import {observeConfig, observeCurrentUserId} from '@queries/servers/system';
import {observeUser} from '@queries/servers/user';
import {hasPermissionForChannel} from '@utils/role';
import {observePermissionForChannel} from '@utils/role';

import SendHandler from './send_handler';

Expand Down Expand Up @@ -59,7 +59,7 @@ const enhanced = withObservables([], (ownProps: WithDatabaseArgs & OwnProps) =>
return of$(true);
}

return u ? from$(hasPermissionForChannel(c, u, Permissions.USE_CHANNEL_MENTIONS, false)) : of$(false);
return u ? observePermissionForChannel(c, u, Permissions.USE_CHANNEL_MENTIONS, false) : of$(false);
}),
);

Expand Down
6 changes: 3 additions & 3 deletions app/components/post_draft/uploads/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {makeStyleSheetFromTheme} from '@utils/theme';
import UploadItem from './upload_item';

const CONTAINER_HEIGHT_MAX = 67;
const CONATINER_HEIGHT_MIN = 0;
const CONTAINER_HEIGHT_MIN = 0;
const ERROR_HEIGHT_MAX = 20;
const ERROR_HEIGHT_MIN = 0;

Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you find out about this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const filesForGallery = useRef(files.filter((f) => !f.failed && !DraftUploadManager.isUploading(f.clientId!)));

const errorAnimatedStyle = useAnimatedStyle(() => {
Expand Down Expand Up @@ -116,7 +116,7 @@ function Uploads({
containerHeight.value = CONTAINER_HEIGHT_MAX;
return;
}
containerHeight.value = CONATINER_HEIGHT_MIN;
containerHeight.value = CONTAINER_HEIGHT_MIN;
}, [files.length > 0]);

const openGallery = useCallback((file: FileInfo) => {
Expand Down
6 changes: 3 additions & 3 deletions app/components/post_list/combined_user_activity/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
import {withDatabase} from '@nozbe/watermelondb/DatabaseProvider';
import withObservables from '@nozbe/with-observables';
import React from 'react';
import {combineLatest, from as from$, of as of$} from 'rxjs';
import {combineLatest, of as of$} from 'rxjs';
import {map, switchMap} from 'rxjs/operators';

import {Permissions} from '@constants';
import {queryPostsById} from '@queries/servers/post';
import {observeCurrentUserId} from '@queries/servers/system';
import {observeUser, queryUsersByIdsOrUsernames} from '@queries/servers/user';
import {generateCombinedPost, getPostIdsForCombinedUserActivityPost} from '@utils/post_list';
import {hasPermissionForPost} from '@utils/role';
import {observePermissionForPost} from '@utils/role';

import CombinedUserActivity from './combined_user_activity';

Expand All @@ -28,7 +28,7 @@ const withCombinedPosts = withObservables(['postId'], ({database, postId}: WithD
const posts = queryPostsById(database, postIds).observe();
const post = posts.pipe(map((ps) => generateCombinedPost(postId, ps)));
const canDelete = combineLatest([posts, currentUser]).pipe(
switchMap(([ps, u]) => (u ? from$(hasPermissionForPost(ps[0], u, Permissions.DELETE_OTHERS_POSTS, false)) : of$(false))),
switchMap(([ps, u]) => (u ? observePermissionForPost(ps[0], u, Permissions.DELETE_OTHERS_POSTS, false) : of$(false))),
);

const usernamesById = post.pipe(
Expand Down
Loading