Page MenuHomePhabricator

[native] Add MessageSearchContent
ClosedPublic

Authored by inka on May 10 2023, 3:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 19, 11:56 PM
Unknown Object (File)
Sat, Jun 15, 12:41 PM
Unknown Object (File)
Fri, Jun 14, 2:05 PM
Unknown Object (File)
Thu, Jun 13, 6:16 PM
Unknown Object (File)
Wed, Jun 12, 6:06 PM
Unknown Object (File)
Wed, Jun 12, 10:08 AM
Unknown Object (File)
Wed, Jun 12, 5:14 AM
Unknown Object (File)
Wed, Jun 12, 12:45 AM
Subscribers

Details

Summary

Issue: https://linear.app/comm/issue/ENG-3163/create-the-search-screen-for-message-searching
Add a component that will be displayed in the search screen when there is a query typed into the search bar. The logic is pretty similar to pinned messages : D7673
Some navigation types had to be updated to allow using MessageResult in this component.

Test Plan

Tested with the next diff - displayed it the search screen and checked that the messages are fetched and displayed correctly. Checked that the "End of results" and "No results..." messages display correctly. Checked the case when
the first fetch returns all results as well as the case when the user needs to scroll to fetch more messages a couple of times.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.May 10 2023, 4:06 AM
inka planned changes to this revision.May 11 2023, 2:46 AM
native/search/search-content.react.js
189 ↗(On Diff #26329)

Is it possible to factor out the shared logic between this function and getOldestNonLocalMessageID?

Address https://linear.app/comm/issue/ENG-3882/reaction-not-showing-in-search-screen - now all search results are being passed to messageListData, so that if a user reacts to a message in the search screen, the
reaction gets shown

native/search/search-content.react.js
189 ↗(On Diff #26329)

Is it worth it? It's a simple function, and making the logic shared would require making this code more complex since the types used by these functions are different and they check some property that the other type doesn't have (itemType), or doesn't make sense to check in the other case (id.startsWith(localIDPrefix)). The only thing they share is looping backwards

Rename Statement to SearchFooter

native/search/search-content.react.js
189 ↗(On Diff #26329)

Okay, fair point

ashoat requested changes to this revision.May 13 2023, 11:56 AM
ashoat added inline comments.
native/search/search-content.react.js
60 ↗(On Diff #26419)

Doing this in an effect feels weird to me, as opposed to doing it in the callback where we fetch the results from the server. I have two concerns:

  1. I worry that we might end up calling appendSearchResults for the same search results twice accidentally, for instance if threadInfo changes.
  2. The "data flow" seems less clear... we have messages coming from the search results (presumably from a callback there), and then it triggers this other effect in a different file.

Is there a way we can make all of this happen in a callback? For instance, what if useSearchMessages was passed a callback, and useSearchMessages would trigger that callback when it received new search results?

80 ↗(On Diff #26419)

Nit

101 ↗(On Diff #26419)

Why do we only add the loader if it's present in chatMessageInfos? I don't think there's any relation between the loader in chatMessageInfos (which is present if startReached === false in Redux) and the loader being used here (which is present if the search results have not been exhausted)

This revision now requires changes to proceed.May 13 2023, 11:56 AM

Address review on D7774 and D7775 - fix when loader is added, remove calling appendSearchResults in an Effect, and do it in a callback passed to useSearchMessages instead, remove
MessageSearchContent and instead add what is was supposed to display to MessageSearch.

native/search/message-search.react.js
180 ↗(On Diff #26732)

Why is this newline character here? This is really bad for responsiveness... you seem to be assuming the width of the device. I think you want to apply padding instead?

ashoat requested changes to this revision.May 22 2023, 7:22 AM

Passing back to your queue, in part for feedback above, but mostly for the changes I requested in D7771

This revision now requires changes to proceed.May 22 2023, 7:22 AM

Remove newline
I amended D7772 with a padding

Fix spinner not showing at the begining

ashoat added inline comments.
native/search/message-search.react.js
42–44 ↗(On Diff #26856)

Can you get rid of the wrapper function and just pass clearQuery in directly? The wrapper function doesn't appear to be doing anything

This revision is now accepted and ready to land.May 23 2023, 6:56 AM