Details

Reviewers

varun
bartek
ashoat

Commits

rCOMM700a9fff718c: [lib] add prefix query support to context provider

Summary

This implements prefix query messaging on the identity search context handler.
sendPrefixQuery add a promise to an array and then returns the promise. It then sends a websocket search query with an id.
When either a Success or Error search response type is received, the context handler will either resolve or reject promises using the id in the search response.

Depends on D10973

Test Plan

flow and console logged search results on web

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

will created this revision.Feb 8 2024, 11:23 AM

Herald added a subscriber: tomek. · View Herald TranscriptFeb 8 2024, 11:23 AM

will edited the summary of this revision. (Show Details)Feb 8 2024, 11:24 AM

will added a parent revision: D10973: [lib] Check for previous auth message and websocket connection state in identity search context provider.

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 8 2024, 11:26 AM

Harbormaster failed remote builds in B26669: Diff 36879!

latest version

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 8 2024, 11:37 AM

Harbormaster failed remote builds in B26670: Diff 36880!

will added a child revision: D11001: [lib] identity search function.Feb 8 2024, 11:40 AM

will requested review of this revision.Feb 8 2024, 11:45 AM

will added inline comments.Feb 8 2024, 11:54 AM

lib/identity-search/identity-search-context.js
192 ↗	(On Diff #36880)	Unsure if this should be an invariant
216 ↗	(On Diff #36880)	I used the preexisting clientRequestVisualTimeout constant which is what we currently use in inflight-requests.js. However, given that search results should be returned immediately, I was considering creating a shorter timeout duration so that we might fallback to keyserver search quicker

will retitled this revision from [lib] add prefix query support to context handler to [lib] add prefix query support to context provider.Feb 8 2024, 11:59 AM

will marked an inline comment as not done.Feb 8 2024, 12:03 PM

Good work on matching the requests / responses!

Did you consider using InflightRequests directly? I'm guessing that some parts of it are too tightly coupled to the keyserver WebSocket client code?

lib/identity-search/identity-search-context.js
159 ↗	(On Diff #36880)	Should we add a log or something? This seems like it would be an unexpected thing
192 ↗	(On Diff #36880)	I would "DeMorgan's Law" this RE the `invariant`, there's really no difference between an `invariant` and this code... it's a stylistic preference more than anything
211 ↗	(On Diff #36880)	Feels a bit weird that this line is inside this `new Promise` declaration. Can you move it out?
215–218 ↗	(On Diff #36880)	This function looks like it can be extracted to the top-level scope, which would avoid the need for it to be redefined on every invocation of `sendPrefixQuery`
217 ↗	(On Diff #36880)	Not clear why you're throwing an empty array. Can you throw a `new Error` instead?

This revision now requires changes to proceed.Feb 8 2024, 12:09 PM

review feedback

Harbormaster failed remote builds in B26731: Diff 36961!Feb 9 2024, 11:39 PM

Definitely looked onto using InflightRequests directly but seemed like a lot more work to decouple with keyserver code. It would require revamping a lot of my current solution which might not be worth the time required

Can you rebase on master to fix the CI issues?

This revision is now accepted and ready to land.Feb 12 2024, 6:02 AM

rebase for CI

will mentioned this in D10938: [lib] Basic implementation of identity search context provider without message sending.Feb 13 2024, 2:39 PM

Harbormaster completed remote builds in B26805: Diff 37048.Feb 13 2024, 2:54 PM

will mentioned this in D11060: [web] Change identity search function return type to GlobalUserInfo.Feb 14 2024, 1:07 PM

will added a child revision: D11058: [web] [lib] Modify useSearchUsers to include identity search and utilize identity search on web and native.Feb 14 2024, 10:08 PM

will removed a child revision: D11001: [lib] identity search function.Feb 25 2024, 2:01 PM

Closed by commit rCOMM700a9fff718c: [lib] add prefix query support to context provider (authored by will). · Explain WhyFeb 26 2024, 8:49 PM

This revision was automatically updated to reflect the committed changes.

will added a commit: rCOMM700a9fff718c: [lib] add prefix query support to context provider.