[keyserver][lib][web][native] use local search for thick threads
ClosedPublic
Actions

Authored by inka on Jul 24 2024, 1:58 AM.

Details

Reviewers

tomek
kamil

Commits

rCOMMb4354b47dc17: [keyserver][lib][web][native] use local search for thick threads

Summary

issue: ENG-8653

Test Plan

Tested by commenting out isThreadThin check, and commenting out extractKeyserverIDFromIDOptional(messageInfo.threadID) check in getMessageSearchStoreOps. This allowed me to treat a thin thread as if it were thick, and execute local search on it.

Tested that the messages are fetched, ordered by timestamp descending. Tested that in thin threads (if the above hacks are removed) search works as before.

One thing that doesn’t work in thick threads is fetching next pages of results. This is because timestamps exceed cpp int, so SQLiteQueryExecutor::searchMessages doesn’t bind them to sql query correctly. Fixing this is tracked in ENG-8893

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Jul 24 2024, 1:58 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJul 24 2024, 1:58 AM

Harbormaster completed remote builds in B30627: Diff 42723.Jul 24 2024, 2:15 AM

inka requested review of this revision.Jul 24 2024, 2:15 AM

inka added 1 blocking reviewer(s): kamil.Jul 24 2024, 2:17 AM

inka added inline comments.

lib/actions/message-actions.js
496 ↗	(On Diff #42723)	@kamil do you remember why we decided to use string for `timestampCursor`? Is it because it is easier to handle when passing to cpp? Or did I use a string in tests, and you just followed that?

One thing that doesn’t work in thick threads is fetching next pages of results. This is because timestamps exceed cpp int, so SQLiteQueryExecutor::searchMessages doesn’t bind them to sql query correctly. Fixing this is tracked in ENG-8893

Next diff fixes this

tomek accepted this revision.Jul 24 2024, 5:44 AM

tomek added inline comments.

lib/actions/message-actions.js
477 ↗	(On Diff #42723)	Can we use `defaultNumberPerThread` from `lib/types/message-types.js`?
507–513 ↗	(On Diff #42723)	It's up to you, but this can be simplified
native/search/message-search.react.js
49–50 ↗	(On Diff #42723)	We could also use a single state variable with the whole message info
web/search/message-search-state-provider.react.js
42–47 ↗	(On Diff #42723)	Would it be simpler if we had a single state variable with the whole message info?

kamil accepted this revision.Jul 25 2024, 1:42 AM

kamil added inline comments.

lib/actions/message-actions.js
496 ↗	(On Diff #42723)	This on purpose for safety, in general, we have a convention to pass numbers as strings to `.wasm` or native code to avoid issues with encoding on a different platforms

This revision is now accepted and ready to land.Jul 25 2024, 1:42 AM

inka added inline comments.Jul 25 2024, 1:53 AM

lib/actions/message-actions.js
496 ↗	(On Diff #42723)	Thank you for explaining!

Address review

native/search/message-search.react.js
49–50 ↗	(On Diff #42723)	I don't like that. They are being set in different places and we want memos to depend on them separately. So I feel like they should be separate

Harbormaster completed remote builds in B30664: Diff 42767.Jul 25 2024, 3:46 AM

Closed by commit rCOMMb4354b47dc17: [keyserver][lib][web][native] use local search for thick threads (authored by inka). · Explain WhyJul 26 2024, 5:28 AM

This revision was automatically updated to reflect the committed changes.

inka added a commit: rCOMMb4354b47dc17: [keyserver][lib][web][native] use local search for thick threads.

ashoat added inline comments.Jul 31 2024, 7:11 PM

lib/actions/message-actions.js
498 ↗	(On Diff #42827)	Nit: seems like a capitalization typo, should this be `messageIDCursor`?
lib/types/message-types.js
697–698 ↗	(On Diff #42827)	Curious why we need both cursors

inka mentioned this in D12965: Fix capitalization.Aug 5 2024, 1:49 AM

inka added inline comments.Aug 5 2024, 2:02 AM

lib/actions/message-actions.js
498 ↗	(On Diff #42827)	Created D12965 to address
lib/types/message-types.js
697–698 ↗	(On Diff #42827)	(This was discussed here) On the keyserver message ids grow with time, and there are no two identical ids. Meaning a newer message will have a bigger id, and messages with the same timestamp have different ids so we can sort them deterministically. In DMs, message ids are random. So we cannot sort by them. So we sort by timestamp. But on the off chance that two messages will have the same timestamp, we need some way to deterministically decide which is "first". This way, if the page of results happens to end on a timestamp that is shared between messages, but not all messages with this timestamp were fit in that page, we know which messages to send in the next page of results. This lets us avoid repetition or missing some messages.