issue: https://linear.app/comm/issue/ENG-4163/add-results-and-position-to-message-search-context
We want to remember the results the user has fetched until they change/clear the query. We want to be able to close and reopen the modal, and see those results.
We need to remember endReached so that when the user scrolls to the bottom, and they had already fetched all messages, they don't query the server again.
query was changed to getQuery because:
If the user searches for something, closes the modal and reopens it, we want to show them the results they have previously fetched. We only want the client to call the server, if the user presses the "Search" button, or scrolls to the bottom.
When the "Search" button is pressed, we first have to set the query via setQuery function provided by this context. Only once the query is set, we want to call the server. In class react we could pass a callback to setState, but we cannot
do it in functional react. So to be able to set the query and call the server with its correct value, we will keep the query in a ref. This allows us to change the query without having to wait for the next rerender. So for the calling code to
always have the current query, we need to give it a getter.
This solution was disscussed with @tomek.
Details
Checked that the context state changes as expected when supplied functions are called
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/search/message-search-state-provider.react.js | ||
|---|---|---|
| 40–44 ↗ | (On Diff #28147) | These need to be refs since the query is a ref, so that when searchMessages is called, it sees the newest values |
| web/search/message-search-state-provider.react.js | ||
|---|---|---|
| 184 ↗ | (On Diff #28147) | We want to omit reaction and so on |
| web/search/message-search-state-provider.react.js | ||
|---|---|---|
| 101 ↗ | (On Diff #28147) | It appears that you use a mix of direct mutation (eg. the delete lines) and creating a new object. I think it'd be better to be consistent. If you want to make it read-only, we should update the types and get rid of the delete lines. If you want it to be mutable, then you can just set it directly here |
Do you think it would make sense to:
- merge searchMessages with setQuery?
- change query (if the above point is implemented) and endsReached to state? Because (to my understanding of React) currently if they change they won't trigger a rerender of the Modal component
| web/search/message-search-state-provider.react.js | ||
|---|---|---|
| 57 ↗ | (On Diff #28248) | I think this can be removed |
I cannot merge setQuery with searchMessages, because the user might:
- Type in 'a' and press Search
- Type in 'b' (the query is now 'ab')
- Scroll to the bottom
In this case we want to fetch the second page for 'a', not results for 'ab'
Because (to my understanding of React) currently if they change they won't trigger a rerender of the Modal component
If the query changes setQuery is called, which calls clearResults. So results change, so getResults changes, so the modal is rerendered