[native] Change ChatContextProvider position
ClosedPublic
Actions

Authored by • patryk on Aug 17 2023, 6:23 AM.

Details

Reviewers

tomek
inka

Commits

rCOMMc8fa18ba6cc5: [native] Change ChatContextProvider position

Summary

This diff changes position of ChatContextProvider. It is necessary, because we use useNavigation hook in markdown-chat-mention.react.js and when we render dummy nodes (which are needed for our virtualised list) we don't have access to NavigationContainer.

Depends on D8846.

Test Plan

Tested if ChatContextProvider is used in:

MessageSearchProvider
RegistrationContextProvider
SQLiteDataHandler
ConnectedStatusBar
ReduxPersistGate
gated:
- LifecycleHandler
- DisconnectedBarVisibilityHandler
- DimensionsUpdater
- ConnectivityUpdater
- ThemeHandler
- OrientationHandler
PersistedStateGate
Socket

and it is not. Also checked locations where we use ChatContext:

native/chat/message-list-container.react.js
native/chat/message-list-types.js
native/chat/message-results-sceen.react.js
native/chat/multimedia/robotext/text-message.react.js
native/chat/sidebar-navigation.js
native/search/message-search.react.js
native/tooltip/tooltip.react.js

All of these files are "inside" RootNavigator. Only chatMessageItemKey from native/chat/utils.js is used in chat-item-height-measurer.react.js but this file is used in chat-context-provider.react.js only.

Diff Detail

Repository

rCOMM Comm

Branch

patryk/chat-mention-native

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• patryk created this revision.Aug 17 2023, 6:23 AM

• patryk held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 17 2023, 6:23 AM

Harbormaster failed remote builds in B21833: Diff 30005!Aug 17 2023, 6:34 AM

• patryk edited the summary of this revision. (Show Details)Aug 18 2023, 7:23 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added reviewers: tomek, inka.

• patryk mentioned this in D8874: [native] Memorize ChatContextProvider.Aug 19 2023, 11:03 AM

• patryk edited the summary of this revision. (Show Details)Aug 19 2023, 3:28 PM

• patryk added a parent revision: D8846: [native] Use chat mention candidates in markdown rules.

• patryk added a child revision: D8874: [native] Memorize ChatContextProvider.

Please note, that this change will not affect our native app performance. ChatContextProvider will additionally be wrapped in React.memo here: D8874.

• patryk published this revision for review.Aug 20 2023, 11:13 PM

Can you explain why we need to wrap the Provider in React.memo all of the sudden?
You have some CI errors, please address them.
Can you also add to the test plan checking if methods from ChatContextProvider are not being used in the components that are now not under it? like SQLiteDataHandler, ConnectedStatusBar, whatever is in gated (line 284) etc. Changing the structure like this seems very dangerous, so I'd rather you test it thoroughly.

This revision now requires changes to proceed.Aug 28 2023, 3:53 AM

Can you explain why we need to wrap the Provider in React.memo all of the sudden?

It was discussed during my 1:1 with Ashoat: he suggested that since I want to move this provider, it would also be good to wrap this in React.memo.

You have some CI errors, please address them.

Probably connected with previous diffs where I forgot to add one argument: will run them again when I update test plan.

Can you also add to the test plan checking if methods from ChatContextProvider are not being used in the components that are now not under it? like SQLiteDataHandler, ConnectedStatusBar, whatever is in gated (line 284) etc. Changing the structure like this seems very dangerous, so I'd rather you test it thoroughly.

During my 1:1 with Ashoat, we also discussed potential issues that might arise. While we didn't identify any problems, it's still a good idea to add more information to the test plan.

Rebase

Harbormaster completed remote builds in B22246: Diff 30543.Aug 30 2023, 5:19 AM

• patryk edited the test plan for this revision. (Show Details)Aug 31 2023, 6:52 AM

inka accepted this revision.Aug 31 2023, 8:12 AM

This revision is now accepted and ready to land.Aug 31 2023, 8:12 AM

This looks strange. We were rendering ChatContextProvider quite high in the tree, a couple of levels above navigation. Now, in order to make it accessible in other places, we're placing it within navigation. So it seems we're reducing the number of places where it is accessible. Am I missing something?

This revision now requires changes to proceed.Sep 12 2023, 1:56 AM

I think the reason is that ChatContextProvider renders dummy nodes for height measurement, and now those dummy nodes need navigation state since they might be rendering a link to a chat

tomek accepted this revision.Sep 13 2023, 2:39 AM

This revision is now accepted and ready to land.Sep 13 2023, 2:39 AM