Page MenuHomePhabricator
Differential D8351

[web/native] Send platform details on http communication
ClosedPublic
Actions

Authored by michal on Jun 28 2023, 3:18 AM.
Tags
None
Referenced Files
F2134414: D8351.id28377.diff
Fri, Jun 28, 6:39 AM
F2134267: D8351.diff
Fri, Jun 28, 6:01 AM
Unknown Object (File)
Tue, Jun 25, 9:29 PM
Unknown Object (File)
Sun, Jun 23, 8:53 PM
Unknown Object (File)
Sat, Jun 22, 8:44 AM
Unknown Object (File)
Sat, Jun 22, 8:32 AM
Unknown Object (File)
Sat, Jun 22, 8:32 AM
Unknown Object (File)
Sat, Jun 22, 8:32 AM
View All 54 Files
Subscribers
ashoat

Details

Reviewers
kamil
tomek
Commits
rCOMMba611328bb0b: [web/native] Send platform details on http communication
Summary

https://linear.app/comm/issue/ENG-3905/platform-details-arent-updated-on-http-communication
Using redux state introduced in the previous diff, send platform details when calling http endpoints as needed. Also updated the websocket to only send the platform details when needed and not on every initialization.

Depends on D8350

Test Plan

Disable the websocket, update version, trigger endpoint call, check if redux state is updated.
Enable the websocket, wait for initialization, check if redux state is updated.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

michal created this revision.Jun 28 2023, 3:18 AM
Herald added a subscriber: ashoat. · View Herald TranscriptJun 28 2023, 3:18 AM
Harbormaster completed remote builds in B20570: Diff 28205.Jun 28 2023, 3:35 AM
michal requested review of this revision.Jun 28 2023, 3:35 AM
michal added inline comments.Jun 28 2023, 4:14 AM
lib/utils/action-utils.js
213 ↗(On Diff #28205)

This is fine because this used in logging in the redux state will be updated anyway.

ashoat added inline comments.Jun 28 2023, 6:09 PM
keyserver/src/responders/handlers.js
58–61 ↗(On Diff #28205)

This is actually risky because if it throws an exception, nothing will catch it.

The issue is that if the promise rejects without being caught, Node.js treats this a bit like an exception being thrown and not caught.

You'll see an "unhandled promise rejection" warning in our current version of Node. But in more recent versions of Node, this would crash the whole app.

michal updated this revision to Diff 28276.Jun 29 2023, 10:48 AM

Await the setCookiePlatformDetails

Harbormaster failed remote builds in B20625: Diff 28276!Jun 29 2023, 11:02 AM
ashoat added inline comments.Jul 2 2023, 6:46 PM
keyserver/src/responders/handlers.js
58 ↗(On Diff #28276)

Does setCookiePlatformDetails really need to block policiesValidator? If not, can you find a way to execute these simulatneously?

You can also use handleAsyncPromise here if you don't want to block execution. Not sure what the intention is here

michal updated this revision to Diff 28352.Jul 3 2023, 7:17 AM

Set new platform details concurrently with policy validation. We need to block execution before running endpoint logic, because it might depend on the version

Harbormaster completed remote builds in B20690: Diff 28352.Jul 3 2023, 7:34 AM
kamil accepted this revision.Jul 4 2023, 2:45 AM
kamil added inline comments.
lib/socket/socket.react.js
111 ↗(On Diff #28352)

nit: this is more // Redux state rather than // keyserver olm sessions specific props

also, I think there is a possibility that lastCommunicatedPlatformDetails can be passed as undefined.

lib/utils/call-server-endpoint.js
110–114 ↗(On Diff #28352)

Are you sure this is correct? I think when there is no lastCommunicatedPlatformDetails we should send platform details

web/socket.react.js
58–60 ↗(On Diff #28352)

nit: I would prefer to see this selector a couple of lines higher, grouped with other selectors

This revision is now accepted and ready to land.Jul 4 2023, 2:45 AM
michal updated this revision to Diff 28377.Jul 4 2023, 3:12 AM

Address review

lib/utils/call-server-endpoint.js
110–114 ↗(On Diff #28352)

lastCommunicatedPlatformDetails is set to null only when calling it from fetchNewCookieFromNativeCredentials. In this case, we don't want to send platform details because they will be later invalidated anyway (fetchNewCookieFromNativeCredentials also calls login).

Harbormaster completed remote builds in B20708: Diff 28377.Jul 4 2023, 3:29 AM
Closed by commit rCOMMba611328bb0b: [web/native] Send platform details on http communication (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyJul 11 2023, 2:46 AM
This revision was automatically updated to reflect the committed changes.
Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMMba611328bb0b: [web/native] Send platform details on http communication.
tomek added inline comments.Aug 7 2023, 6:28 AM
lib/socket/socket.react.js
535

Revision Contents
Changeset List

PathSize
keyserver/
src/
responders/
25 lines
lib/
selectors/
5 lines
socket/
20 lines
utils/
8 lines
27 lines
native/
5 lines
web/
5 lines

Diff 28570

View Options

keyserver/src/responders/handlers.js

Loading...
View Options

lib/selectors/server-calls.js

Loading...
View Options

lib/socket/socket.react.js

Loading...
View Options

lib/utils/action-utils.js

Loading...
View Options

lib/utils/call-server-endpoint.js

Loading...
View Options

native/socket.react.js

Loading...
View Options

web/socket.react.js

Loading...