Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(connect): move remembered device away from local variable #9790

Merged
merged 5 commits into from
Feb 4, 2024

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Oct 28, 2023

Make "remember" device work when core is loaded in popup.

Description

In #9525 we are introducing loading connect core inside popup. This has 2 effects:

  • to support environments that can't inject iframe.
  • to support webusb in popup

Couple of changes are needed. First of them is how "remember" device works:

  • Previously, remembered device was stored in a local variable which was ok, because iframe was actually making it persist between 2 calls. This is not the case anymore. When core is loaded in popup, everything locally stored is gone after popup is closed. This means we need to move it to storage.
  • Previously, due to persistent nature ensured by iframe, when user disconnected device when popup was already closed, transport event was handled and local variable _preferedDevice was cleared. Without this behaviour we would get Device not found error, because connect would obviously try to target a device which is not connected at all. I had to change this behaviour in 2 ways:
    • do not rely on disconnect event
    • remove prefered device from store when device is not available in initDevice phase and show device selection instead.

Testing

  • it should be just rework of internal logic, now new features, no changes
  • mainly using webusb with remember on sldev should work as you would expect
  • maybe try please testing with more than 1 device

Related Issue

#9525, #9644

Screenshots:

Connecting device for the 1st time or if there is more than 1 device connected

image

@mroz22 mroz22 force-pushed the connect-remember-device branch 3 times, most recently from b6911d0 to 917fff9 Compare October 30, 2023 16:07
@mroz22 mroz22 force-pushed the connect-remember-device branch from 592bc55 to f258c9b Compare October 31, 2023 12:33
@mroz22 mroz22 force-pushed the connect-remember-device branch from f258c9b to d92dfca Compare November 8, 2023 10:12
Copy link

socket-security bot commented Nov 8, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@mroz22 mroz22 force-pushed the connect-remember-device branch from d92dfca to 58992de Compare November 9, 2023 16:04
@mroz22 mroz22 force-pushed the connect-remember-device branch 3 times, most recently from 7d072d5 to 00ecc1b Compare December 3, 2023 16:19
@mroz22 mroz22 force-pushed the connect-remember-device branch from 00ecc1b to fb34b4a Compare January 5, 2024 13:06
@mroz22 mroz22 force-pushed the connect-remember-device branch from fb34b4a to 6d05b57 Compare January 7, 2024 17:12
@mroz22 mroz22 force-pushed the connect-remember-device branch 6 times, most recently from 3d240d1 to 45c8af4 Compare January 12, 2024 16:56
@@ -210,7 +210,20 @@ const initDevice = async (method: AbstractMethod<any>) => {
const isWebUsb = _deviceList.transportType() === 'WebUsbTransport';
let device: Device | typeof undefined;
let showDeviceSelection = isWebUsb;
if (method.devicePath) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check removed line 297, its related

@mroz22 mroz22 marked this pull request as ready for review January 12, 2024 17:12
@mroz22
Copy link
Contributor Author

mroz22 commented Jan 12, 2024

cc @marekrjpolak because I am touching core (_preferredDevice), cc @karliatto because it relates to all kind of popup behaviours and is related to webextension

@mroz22 mroz22 assigned karliatto and unassigned Hannsek Jan 12, 2024
@mroz22 mroz22 requested a review from karliatto January 12, 2024 18:48
@mroz22 mroz22 assigned martykan and unassigned karliatto Jan 25, 2024
@mroz22
Copy link
Contributor Author

mroz22 commented Jan 30, 2024

if possible and we are sure it doesn't break anything, it would make sense to include this in 9.1.12 release. since properly worker @trezor/connect-webextension sort of depends on this imho. cc @karliatto

@martykan
Copy link
Member

/rebase

Copy link

@trezor-ci trezor-ci force-pushed the connect-remember-device branch from 45c8af4 to ec8c0d9 Compare January 30, 2024 16:11
@martykan
Copy link
Member

martykan commented Jan 30, 2024

@mroz22 For some reason remember device doesn't seem to get saved on the first try, but only the second time. Will debug more tomorrow

@martykan
Copy link
Member

martykan commented Jan 31, 2024

tmp: run all popup test should be included or probably not?

Update: removed it

@martykan martykan force-pushed the connect-remember-device branch from 46986ad to abb5b23 Compare January 31, 2024 13:24
@mroz22
Copy link
Contributor Author

mroz22 commented Jan 31, 2024

I am still not decided whether to push this into 9.1.12 or wait for 9.1.13. leaning slightly towards 9.1.13

@Hannsek
Copy link
Contributor

Hannsek commented Jan 31, 2024

If we don't need it in the next release and you are unsure, let's wait.

@mroz22
Copy link
Contributor Author

mroz22 commented Feb 4, 2024

/rebase

Copy link

github-actions bot commented Feb 4, 2024

@trezor-ci trezor-ci force-pushed the connect-remember-device branch from abb5b23 to efb95ee Compare February 4, 2024 09:39
@mroz22 mroz22 merged commit b587a65 into develop Feb 4, 2024
32 checks passed
@mroz22 mroz22 deleted the connect-remember-device branch February 4, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants