Page MenuHomePhabricator
Differential D11291

[native][web] Expose Secondary device registration RPC to JS
ClosedPublic
Actions

Authored by bartek on Mar 11 2024, 6:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 4, 3:59 PM
Unknown Object (File)
Thu, Apr 3, 11:35 PM
Unknown Object (File)
Tue, Mar 18, 10:27 AM
Unknown Object (File)
Feb 27 2025, 10:08 PM
Unknown Object (File)
Feb 27 2025, 7:26 PM
Unknown Object (File)
Feb 27 2025, 7:26 PM
Unknown Object (File)
Feb 27 2025, 7:26 PM
Unknown Object (File)
Feb 27 2025, 7:26 PM
View All 93 Files
Subscribers
ashoat
tomek

Details

Reviewers
kamil
michal
Commits
rCOMM8962994db2af: [native][web] Expose Secondary device registration RPC to JS
Summary

Exposed this RPC via Identity context

Depends on D11290

Test Plan

This is tested later in further diffs. Now just ensured it's callable on both native and web

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Mar 11 2024, 6:16 AM
bartek held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 11 2024, 6:16 AM
bartek added a child revision: D11292: [lib] Expose isAuthorized property in Tunnelbroker context.Mar 11 2024, 6:17 AM
Harbormaster completed remote builds in B27421: Diff 37969.Mar 11 2024, 6:31 AM
bartek updated this revision to Diff 37982.Mar 11 2024, 6:47 AM

Rebase

bartek edited the summary of this revision. (Show Details)Mar 11 2024, 7:03 AM
bartek edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B27434: Diff 37982.Mar 11 2024, 7:23 AM
bartek published this revision for review.Mar 11 2024, 7:27 AM
kamil accepted this revision.Mar 12 2024, 9:16 AM

Looks good but when landing you could comment in ENG-7198 to use olmAPI here - or if the olmAPI is ready at the time of landing, use it here.

native/identity-service/identity-service-context-provider.react.js
483

could you at least add a comment explaining why username is an empty string here?

web/grpc/identity-service-client-wrapper.js
433
This revision is now accepted and ready to land.Mar 12 2024, 9:16 AM
ashoat added inline comments.Mar 12 2024, 3:12 PM
native/identity-service/identity-service-context-provider.react.js
481

Nit: why assign to token instead of just letting it be accessToken?

483

Is there a task to address the username being an empty string?

web/grpc/identity-service-client-wrapper.js
422

Not sure why this newline was removed

bartek added inline comments.Mar 14 2024, 6:11 AM
native/identity-service/identity-service-context-provider.react.js
481

eslint complains about name shadowing. It's also done for other methods in this file

483

It's https://linear.app/comm/issue/ENG-7215/retrieve-username-when-doing-qr-code-auth

bartek updated this revision to Diff 38062.Mar 14 2024, 6:12 AM

Address comments

bartek added inline comments.Mar 14 2024, 6:14 AM
lib/types/identity-service-types.js
155 ↗(On Diff #38062)

unintended, will fix before landing

Harbormaster completed remote builds in B27505: Diff 38062.Mar 14 2024, 6:26 AM
Closed by commit rCOMM8962994db2af: [native][web] Expose Secondary device registration RPC to JS (authored by bartek). · Explain WhyMar 19 2024, 4:18 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM8962994db2af: [native][web] Expose Secondary device registration RPC to JS.

Revision Contents
Changeset List

PathSize
lib/
types/
14 lines
native/
identity-service/
45 lines
web/
grpc/
35 lines

Diff 37982

View Options

lib/types/identity-service-types.js

Loading...
View Options

native/identity-service/identity-service-context-provider.react.js

Loading...
View Options

web/grpc/identity-service-client-wrapper.js

Loading...