This is the screen that should display the pinned messages / the search results.
Logic is similar to web: https://phab.comm.dev/D7380
Depends on D7672
Details
Details
Check to see the pinned messages render in the screen
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/chat/message-results-screen.react.js | ||
|---|---|---|
| 28 ↗ | (On Diff #25877) | The logic is very similar to web: https://phab.comm.dev/D7380 |
| 37–38 ↗ | (On Diff #25877) | I can probably use dispatchActionPromise() here to use a loading state |
| 119–144 ↗ | (On Diff #25877) | Same comment as here: https://phab.comm.dev/D7607#inline-49676 |
| native/chat/message.react.js | ||
| 33–39 ↗ | (On Diff #25877) | Here, as well as in text-message.react.js and robotext-message.react.js, I can probably extract this to one shared type, so we only have to update it in one place |
| native/chat/robotext-message.react.js | ||
| 31–38 ↗ | (On Diff #25877) | Can probably extract to one shared type (though this is for the flow errors I was running into by not including ChatNavigationProp<'MessageResultsScreen'> and NavigationRoute<'MessageResultsScreen'> here, but I can check if there's a better way to handle this without including it here |
| native/chat/text-message.react.js | ||
| 41–48 ↗ | (On Diff #25877) | Can probably extract to one shared type |
| native/chat/message-result.react.js | ||
|---|---|---|
| 50 ↗ | (On Diff #25877) | Accidental change |
| native/chat/robotext-message.react.js | ||
|---|---|---|
| 31–38 ↗ | (On Diff #25877) | Here's a comment of mine on one of @ginsu's diff that explains how to do this in a more idiomatic way. @ginsu can help with pointers if you need any! |
| native/chat/robotext-message.react.js | ||
|---|---|---|
| 31–38 ↗ | (On Diff #25877) | Thanks! I'll take a look and give it a go |
Comment Actions
Thanks! I'll take a look and give it a go
Is this ready for review at the moment? Sending back to your queue, feel free to re-request review if it is.
| native/chat/robotext-message.react.js | ||
|---|---|---|
| 31–38 ↗ | (On Diff #25877) | Looking at this again, I can see that we have some ChatNavigationProps and some AppNavigationProps... it'll be pretty messy to select the right one based on the route name, so probably not worth it. I think we can leave this as-is |
Comment Actions
It's ready for review now, talked about it in my sync with Ashoat and it'll be best to leave it as is at the moment
| native/chat/message-results-screen.react.js | ||
|---|---|---|
| 58–60 | I think there's a serious issue here – you don't appear to be fetching the pinned messages' related messages (eg. reactions, edits), and are instead relying on them coincidentally being in the Redux store. Is this correct? I thought you and @inka had been collaborating on this work, and @inka had introduced a utility on the keyserver side that you would be able to use for this. @inka sent this message to you on Thursday:  Have you followed up on that and implemented the functionality to fetch related messages using that utility? If yes, can you please explain why you need to access messageListData here? If no, you'll need to make those changes on both native and web in order to be able to land this project... | |
Comment Actions
Re-requesting review after D7736 and following up about whether we need messageListData or not:
I think so personally, since that seems like the place where the logic for constructing ChatMessageInfoItems is handled (getting the reactions, the thread created from the message, etc.). Without that, I feel like it'd involve a lot of copy-paste for doing all of that