Page MenuHomePhabricator

[web] introduce component to ping identity
ClosedPublic

Authored by varun on Jun 7 2024, 2:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 23, 9:03 AM
Unknown Object (File)
Sat, Dec 21, 5:57 PM
Unknown Object (File)
Thu, Dec 19, 7:04 AM
Unknown Object (File)
Thu, Dec 19, 4:12 AM
Unknown Object (File)
Thu, Dec 19, 2:33 AM
Unknown Object (File)
Thu, Dec 19, 1:17 AM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Subscribers

Details

Summary

we want to introduce this component now so that we can check in the next web release if the production identity service is reachable. before this change, ashoat's keyserver was the only device that called identity. going forward, all web clients should be connecting to identity as well.

Test Plan

saw "Identity ping successful" log in browser console

Diff Detail

Repository
rCOMM Comm
Branch
webping
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Jun 7 2024, 2:51 PM
ashoat requested changes to this revision.Jun 9 2024, 8:40 AM

Can you link to the Linear task? Wondering if we're planning to do this on native as well

web/components/identity-ping.react.js
8

I would move this definition down closer to where it's used

24

I don't love that this gets printed in production for normal users. Could we check useIsCurrentUserStaff here?

30–36

This is a good technique for making sure we only call ping once per page load. But if we take my advice above, I think that also would mean that we wouldn't print it unless the user is logged in at the time the page is loaded.

Techniques to get around that include:

  1. Resetting hasRun when the currentUserID changes
  2. Keeping a separate hasRun for each currentUserID
  3. Only rendering the component if the user is logged in
  4. Not worrying as much about how often ping is re-run

Option 4 might be easiest here, but it would be good to check and make sure it doesn't lead to endless log spam.

web/grpc/identity-service-client-wrapper.js
671–674

Minor, but you can avoid async/await syntax here, and also make this a little shorter

This revision now requires changes to proceed.Jun 9 2024, 8:40 AM

https://linear.app/comm/issue/ENG-8138/add-a-ping-call-to-web

I should've clarified that the purpose of this diff is to make sure that identity can be reached via gRPC-web in production

tested that there isn't any log spam after removing hasRun

This revision is now accepted and ready to land.Jun 10 2024, 1:10 PM

Please address the CI issue before landing

web/grpc/identity-service-client-wrapper.js
671–674

this actually doesn't work because this.unauthClient.ping(new Empty()) returns Promise<Empty>

i'm going to revert this change

undo change to ping method

This revision was landed with ongoing or failed builds.Jun 10 2024, 1:51 PM
This revision was automatically updated to reflect the committed changes.