Page MenuHomePhabricator

[desktop] Unregister did-fail-load event after startup
ClosedPublic

Authored by michal on Nov 24 2023, 7:39 AM.
Tags
None
Referenced Files
F3550678: D9967.diff
Thu, Dec 26, 6:20 PM
Unknown Object (File)
Wed, Dec 25, 1:42 PM
Unknown Object (File)
Mon, Dec 16, 10:16 AM
Unknown Object (File)
Mon, Dec 16, 5:04 AM
Unknown Object (File)
Thu, Nov 28, 6:13 PM
Unknown Object (File)
Nov 25 2024, 5:28 AM
Unknown Object (File)
Nov 25 2024, 4:26 AM
Unknown Object (File)
Nov 16 2024, 8:06 PM
Subscribers
None

Details

Summary

ENG-5606

We currently register an event listener for the did-fail-load event. It's used for app startup, which works like this:

  1. we display a window with some simple loader
  2. we create a hidden main window that tries to load web.comm.app
  3. If it fails and we get did-fail-load we try to load it again after 1s
  4. After we are succesful we get did-fisish-load event in which we call show() which displays the hidden window and puts it in front of other windows

The ENG-5606 issue happens when the auto-reload code fails because of network error. For some reason it triggers the did-fail-load event and we do steps (3) and (4). So after we regain network connection we call show() and Comm window steals focus from the user.

This diffs introduces a fix for that - we unregister the did-fail-load event handler after a succesfull web page load as it's only needed for startup.

Test Plan

Wait for auto-update with network turned off. Without this diff the web app reloads (to a black screen as there is no internet), and after network connection is regained the window steals focused. After this diff the app neither reloads nor steals focus.
Additionaly the logs showed electron: Failed to load URL: https://verify.walletconnect.com/03f32dbe53350f23cffea2ddc28152ba with error: ERR_INTERNET_DISCONNECTED error so I tried if eth login worked, and I was able to display the QR code for eth login.

Diff Detail

Repository
rCOMM Comm
Branch
michal/eng-5606-unregister-fail
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I think the declaration should be moved back – please re-request review if you disagree

desktop/src/main.js
243 ↗(On Diff #33587)

Why was this declaration moved down? I think it's better to make the declaration before using the variable. I'm actually not sure how JavaScript works in this case: technically you can always do something like blahBlahVariable = true even if it's never been declared, so it's not clear to me if line 229 is referring to the local scope variable on line 243, or a never-declared global variable

This revision is now accepted and ready to land.Nov 26 2023, 8:34 AM

Fix position of the variable, it was a mistake - I extracted the handler function but didn't move the variable declaration with it.