Page MenuHomePhabricator
Differential D10938

[lib] Basic implementation of identity search context provider without message sending
ClosedPublic
Actions

Authored by will on Feb 2 2024, 8:39 PM.
Tags
None
Referenced Files
F3486839: D10938.id36632.diff
Wed, Dec 18, 6:10 AM
F3486824: D10938.id36875.diff
Wed, Dec 18, 6:06 AM
F3486790: D10938.id36599.diff
Wed, Dec 18, 5:57 AM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
varun
bartek
atul
ashoat
Commits
rCOMMfe02aef634ed: [lib] Basic implementation of identity search context provider without message…
Summary

This implements a basic context provider which connects to the identity search service

Depends on D10928

Test Plan

flow and verified connection by writing some basic code on web utilizing context provider

Diff Detail

Repository
rCOMM Comm
Branch
wyilio/prefix-client
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

will created this revision.Feb 2 2024, 8:39 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 2 2024, 8:39 PM
will retitled this revision from [lib] Basic implementation of identity search context provider without message sending to [lib] [2/n] Basic implementation of identity search context provider without message sending.Feb 2 2024, 8:40 PM
will edited the summary of this revision. (Show Details)
will added a parent revision: D10928: [lib] Add types for identity search websocket server.
will updated this revision to Diff 36600.Feb 2 2024, 8:41 PM

Remove lib/identity-search/identity-search.js

Harbormaster completed remote builds in B26475: Diff 36599.Feb 2 2024, 8:58 PM
Harbormaster completed remote builds in B26476: Diff 36600.
will requested review of this revision.Feb 2 2024, 9:00 PM
ashoat added inline comments.Feb 3 2024, 9:28 AM
lib/identity-search/identity-search-context.js
64 ↗(On Diff #36600)

Was this accidentally left over?

68 ↗(On Diff #36600)

Should onclose be handled too?

105 ↗(On Diff #36600)

I guess these console.logs get replaced in later diffs?

will planned changes to this revision.Feb 5 2024, 7:42 AM
will retitled this revision from [lib] [2/n] Basic implementation of identity search context provider without message sending to [lib] [3/n] Basic implementation of identity search context provider without message sending.Feb 5 2024, 7:45 AM
will updated this revision to Diff 36632.Feb 5 2024, 10:57 AM

addressed feedback and rebased to use modified flow types

will added inline comments.Feb 5 2024, 11:00 AM
lib/identity-search/identity-search-context.js
68 ↗(On Diff #36600)

Yeah. Addressed in latest diff

105 ↗(On Diff #36600)

Besides for the auth token console log which I just removed, Tunnelbroker also keeps these console logs.

ashoat added a comment.Feb 5 2024, 11:00 AM

Can you explain which of these console.logs are going to get removed in a later diff, versus which ones are meant to stay long-term?

will added a comment.Feb 5 2024, 11:14 AM

All the console logs currently in the latest diff (36632) I plan to keep long-term.

Harbormaster completed remote builds in B26500: Diff 36632.Feb 5 2024, 11:17 AM
will planned changes to this revision.Feb 5 2024, 11:50 AM
ashoat added a comment.Feb 5 2024, 4:44 PM

Let's strip the logs for things we expect to happen

We should also consider the dev environment. If we expect the calls to fail in the dev failure, then we should find a way to silence the logs. Guessing it should work fine with staging though, right?

lib/identity-search/identity-search-context.js
69 ↗(On Diff #36632)

I don't think we should keep this

109 ↗(On Diff #36632)

I don't think we should keep this

will added a child revision: D10973: [lib] Check for previous auth message and websocket connection state in identity search context provider.Feb 6 2024, 12:41 PM
will retitled this revision from [lib] [3/n] Basic implementation of identity search context provider without message sending to [lib] [5/n] Basic implementation of identity search context provider without message sending.Feb 6 2024, 11:39 PM
will retitled this revision from [lib] [5/n] Basic implementation of identity search context provider without message sending to [lib] Basic implementation of identity search context provider without message sending.Feb 8 2024, 10:14 AM
will updated this revision to Diff 36875.Feb 8 2024, 11:01 AM

Remove expected logs

Harbormaster failed remote builds in B26665: Diff 36875!Feb 8 2024, 11:04 AM
will updated this revision to Diff 37017.Feb 13 2024, 12:31 AM

rebase

Harbormaster completed remote builds in B26777: Diff 37017.Feb 13 2024, 12:48 AM
will added a reviewer: ashoat.Feb 13 2024, 10:55 AM
ashoat accepted this revision.Feb 13 2024, 11:18 AM
ashoat added inline comments.
lib/identity-search/identity-search-context.js
110 ↗(On Diff #37017)

Please make sure all lines are no longer than 80 chars (except where forced by Prettier)

115 ↗(On Diff #37017)

You should add a space after the colon here

123 ↗(On Diff #37017)

Shouldn't this be identitySearchMessageToServerTypes?

150 ↗(On Diff #37017)

I assume you'll need to add a method later to enable a consumer of this context to send messages (ie. search queries) across the socket?

This revision is now accepted and ready to land.Feb 13 2024, 11:18 AM
ashoat added inline comments.Feb 13 2024, 11:19 AM
lib/identity-search/identity-search-context.js
115 ↗(On Diff #37017)

Actually not sure about this... how does console.log for native / web print two params? Does it already put some whitespace between them?

If no, please add a space in all of the relevant spots here. If yes, you can ignore this feedback

varun added inline comments.Feb 13 2024, 11:25 AM
lib/identity-search/identity-search-context.js
115 ↗(On Diff #37017)

console.log automatically adds whitespace when there are multiple params

will added inline comments.Feb 13 2024, 2:39 PM
lib/identity-search/identity-search-context.js
150 ↗(On Diff #37017)

Yep! This happens in https://phab.comm.dev/D11000 which was already reviewed

will added inline comments.Feb 13 2024, 2:41 PM
lib/identity-search/identity-search-context.js
123 ↗(On Diff #37017)

Yeah. Thanks for catching this

will updated this revision to Diff 37049.Feb 13 2024, 2:49 PM

address feedback. Shorten line size and use server heartbeat type

Harbormaster completed remote builds in B26806: Diff 37049.Feb 13 2024, 3:05 PM
will updated this revision to Diff 37415.Feb 21 2024, 1:19 PM

[lib] rebase with identity search prefix and add auth message hook

will requested review of this revision.Feb 21 2024, 1:20 PM
will added a reviewer: atul.
will added a comment.Feb 21 2024, 1:23 PM

Implementing the useGetIdentitySearchAuthMessage follows discussion from https://phab.comm.dev/D11059

will mentioned this in D11059: [native] Utilize identity search on native.Feb 21 2024, 1:28 PM
Harbormaster completed remote builds in B27046: Diff 37415.Feb 21 2024, 1:35 PM
ashoat resigned from this revision.Feb 21 2024, 4:32 PM

Already reviewed this one – looks like @will wants a review from @atul based on history

lib/utils/identity-search-utils.js
12 ↗(On Diff #37415)

You don't need the question mark here

atul accepted this revision.Feb 22 2024, 7:52 PM
atul added inline comments.
lib/identity-search/identity-search-context.js
75 ↗(On Diff #37415)

Do we need any sort of validation here on the client side? Guessing there's validation on the identity service

This revision is now accepted and ready to land.Feb 22 2024, 7:52 PM
will added inline comments.Feb 24 2024, 12:34 AM
lib/identity-search/identity-search-context.js
75 ↗(On Diff #37415)

Yeah. It's mainly validation on the identity service which then sends a response back whether or not the authMessage was valid or not.

will updated this revision to Diff 37554.Feb 24 2024, 12:36 AM

address feedback

Harbormaster failed remote builds in B27137: Diff 37554!Feb 24 2024, 12:47 AM
Harbormaster completed remote builds in B27137: Diff 37554.Feb 26 2024, 2:35 PM
Closed by commit rCOMMfe02aef634ed: [lib] Basic implementation of identity search context provider without message… (authored by will). · Explain WhyFeb 26 2024, 8:49 PM
This revision was automatically updated to reflect the committed changes.
will added a commit: rCOMMfe02aef634ed: [lib] Basic implementation of identity search context provider without message….

Revision Contents
Changeset List

PathSize
lib/
facts/
9 lines
identity-search/
165 lines
1 line
shared/
4 lines

Diff 36599

View Options

lib/facts/identity-search.js

Loading...
View Options

lib/identity-search/identity-search-context.js

Loading...
View Options

lib/identity-search/identity-search.js

Loading...
View Options

lib/shared/timeouts.js

Loading...