Details

Reviewers

varun
bartek
kamil
ashoat

Commits

rCOMM591f7251b7eb: [lib] Check for previous auth message and websocket connection state in…

Summary

This closes the identity search websocket connection if there is a null or changed auth message and the socket is still active.

Depends on D10938

Test Plan

flow and ran on web

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

will created this revision.Feb 6 2024, 8:37 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 6 2024, 8:37 PM

will retitled this revision from [lib] Check for previous auth message and websocket connection state in identity search context provider to [lib] [4/n] Check for previous auth message and websocket connection state in identity search context provider.Feb 6 2024, 8:41 PM

will edited the summary of this revision. (Show Details)

will added a parent revision: D10938: [lib] Basic implementation of identity search context provider without message sending.

Harbormaster completed remote builds in B26588: Diff 36736.Feb 6 2024, 8:54 PM

will requested review of this revision.Feb 6 2024, 8:54 PM

will planned changes to this revision.Feb 7 2024, 7:32 AM

will retitled this revision from [lib] [4/n] Check for previous auth message and websocket connection state in identity search context provider to [lib] [6/n] Check for previous auth message and websocket connection state in identity search context provider.Feb 7 2024, 7:39 AM

will retitled this revision from [lib] [6/n] Check for previous auth message and websocket connection state in identity search context provider to [lib] Check for previous auth message and websocket connection state in identity search context provider.Feb 8 2024, 6:14 PM

rebase

Harbormaster failed remote builds in B26666: Diff 36876!Feb 8 2024, 7:05 PM

will added a child revision: D11000: [lib] add prefix query support to context provider.Feb 8 2024, 7:24 PM

ashoat added inline comments.Feb 8 2024, 7:35 PM

lib/identity-search/identity-search-context.js
42–43 ↗	(On Diff #36876)	These two lines need to be moved to the effect. The code as-written is broken, as the component could render multiple times before the effect is run, which could result in `authMessageChanged` being `false` in the second run and the effect being skipped, even if `authMessageChanged` is `true` in the first run

ashoat added inline comments.Feb 8 2024, 7:37 PM

lib/identity-search/identity-search-context.js
60–62 ↗	(On Diff #36876)	This looks broken as well. If you need the effect to rerun whenever this changes, you have an issue, because React will not rerender a component (and consequently will not rerun an effect) when a ref changes. You should use state instead of a ref in that case. If you don't need the effect to rerun whenever this changes, then these lines should be in the effect. This lets you avoid including `isSocketActive` in the effect's dep list, which could result in the effect being rerun unnecessarily.

will planned changes to this revision.Feb 8 2024, 8:31 PM

feedback and memoization of authMessage

Harbormaster failed remote builds in B26730: Diff 36960!Feb 10 2024, 7:37 AM

Can you rebase on master to fix the build failures?

lib/identity-search/identity-search-context.js
42–46 ↗	(On Diff #36960)	This code is definitely not idiomatic, as you can probably tell by the use of `eslint-disable`. It looks like you're trying to ignore updates to `authMessage` when none of its fields actually changed. This points to a larger issue here. Whoever is rendering `IdentitySearchProvider` is causing trouble by regenerating `authMessage` when none of its fields have changed. That component is probably where the `React.useMemo` belongs, but I can't tell because it doesn't appear like you render `IdentitySearchProvider` anywhere in your stack. It would make it a lot easier to review your code if you weren't introducing a component without introducing any usages of that component. In general, you should always look to introduce a usage for a new piece of code at the same time that you introduce that code.

This revision now requires changes to proceed.Feb 12 2024, 1:55 PM

address feedback and revert memoization

Harbormaster completed remote builds in B26778: Diff 37018.Feb 13 2024, 8:59 AM

ashoat accepted this revision.Feb 13 2024, 1:29 PM

ashoat added inline comments.

lib/identity-search/identity-search-context.js
66 ↗	(On Diff #37018)	If `authMessageChanged` and `authMessage` is truthy, how does the socket get restarted? I'm guessing the `close` call below triggers `onClose`, which flips the value of `connected` and triggers this effect again?

This revision is now accepted and ready to land.Feb 13 2024, 1:29 PM

bartek accepted this revision.Feb 13 2024, 2:01 PM

bartek added inline comments.

lib/identity-search/identity-search-context.js
66 ↗	(On Diff #37018)	Yes, exactly

rebase with identity search prefix

Harbormaster completed remote builds in B27047: Diff 37416.Feb 21 2024, 9:49 PM

Closed by commit rCOMM591f7251b7eb: [lib] Check for previous auth message and websocket connection state in…. · Explain WhyFeb 27 2024, 4:49 AM

This revision was automatically updated to reflect the committed changes.

will added a commit: rCOMM591f7251b7eb: [lib] Check for previous auth message and websocket connection state in….

[lib] Check for previous auth message and websocket connection state in identity search context provider
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37637

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

[lib] Check for previous auth message and websocket connection state in identity search context providerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37637

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

[lib] Check for previous auth message and websocket connection state in identity search context provider
ClosedPublic
Actions

Revision Contents
Changeset List