Page MenuHomePhabricator

[desktop] Base windows config
ClosedPublic

Authored by michal on Dec 16 2022, 7:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 3 2024, 2:15 PM
Unknown Object (File)
Apr 3 2024, 1:53 PM
Unknown Object (File)
Mar 27 2024, 10:49 PM
Unknown Object (File)
Mar 7 2024, 6:53 AM
Unknown Object (File)
Mar 7 2024, 4:03 AM
Unknown Object (File)
Mar 5 2024, 7:26 PM
Unknown Object (File)
Mar 5 2024, 7:25 PM
Unknown Object (File)
Mar 5 2024, 7:25 PM
Subscribers

Details

Summary

This is the basic config for building an electron windows app.
It uses the Squirrel framework, which is the recommended installer by electron. It doesn't require administrator privileges, is faster and enables auto updating.

Squirrel works roughly like this:

  1. During packaging it creates an .exe that contains our app, Update.exe and installation logic
  2. When a user runs this executable, it unpacks everything
  3. It then runs our app with --squirrel-install flag. When we detect it we should do whatever setup we need (in our case, create shortcuts) and quit
  4. Then it runs our app with --squirrel-firstrun and we should run the app normally

It behaves similarly when updating/ uninstalling the app.

This diff doesn't handle:

  • easy building of the app on windows - this will be put in a later diff, the workaround for now is included in the Test Plan (I would prefer to get code review already started)
  • code signing (we don't have the certs yet)
Test Plan

Can only be built on a Windows machine. You first have to remove the desktop from workspaces in the root package.json. Then just call yarn install and yarn make to make an installer.

Checked:

  • installing, uninstalling, trying to install when the app was already installed
  • check if the shortcuts on the desktop and in the windows menu were created/ removed correctly
  • check if the app worked correctly, double clicking the top-bar, window buttons etc.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

desktop/forge.config.cjs
97–103 ↗(On Diff #19434)

I took description from the app store.

iconUrl is displayed in the Windows Settings > Installed Apps and it must be an URL (I've added a new file in keyserver/icons).

loadingGif is displayed while Squirrel handles installing the app (if takes too long). For now, I've made it a static image that looks like the splash screen so there's a smooth transition from installing to loading the web app.

desktop/src/handle-squirrel-event.js
8 ↗(On Diff #19434)

This file is adapted from readme of windows-installer package and code in the electron-squirrel-startup.

electron-squirrel-startup hasn't had an update in years, the code is very minimal (the whole package is basically this file) and we would probably want to modify it anyway. For example electron-squirrel-startup doesn't handle someone deleting the shortcut - when the app would update it would recreate it. I consider it out of scope for now but it would be nice to handle it correctly.

17 ↗(On Diff #19434)

This Update.exe file is provided by squirrel. It mostly handles updating the app but also has flags for creating and deleting shortcuts that we are using.

desktop/src/main.js
90–94 ↗(On Diff #19434)

Options for the Windows menu buttons:

image.png (1×1 px, 372 KB)

239–241 ↗(On Diff #19434)

Windows doesn't have native badge support. This task tracks fixing this: https://linear.app/comm/issue/ENG-2414

tomek requested changes to this revision.Dec 19 2022, 3:32 AM
tomek added inline comments.
desktop/forge.config.cjs
101 ↗(On Diff #19434)

Shouldn't we store it on some CDN? It seems like on landing we're using cloudfront.

desktop/src/main.js
231 ↗(On Diff #19434)

Can we early exit?

231–233 ↗(On Diff #19434)

It seems like there are multiple ways to exit handleSquirrelEvent with false. Are we sure that all of them should be handled here?

This revision now requires changes to proceed.Dec 19 2022, 3:32 AM

Rewrote the Squirrel startup logic a bit.

desktop/forge.config.cjs
101 ↗(On Diff #19434)

Good point, that would probably be better.

michal added inline comments.
desktop/forge.config.cjs
101 ↗(On Diff #19434)

I made a task for it. I'm going to update the URL in code when it's done.

tomek added a reviewer: ashoat.
tomek added inline comments.
desktop/src/handle-squirrel-event.js
17–19 ↗(On Diff #19543)
21 ↗(On Diff #19543)

Are we sure that this is the only argument we will receive?

36–37 ↗(On Diff #19543)

This can be removed and we will handle it by default return false at the end of this file

desktop/src/main.js
260–266 ↗(On Diff #19543)

Can we simplify these ifs? We should try to reduce the indentation and we can probably avoid having repeated run

Kind of reviewing blind since I don't have a Windows machine to test this out on, but looks reasonable.

I think getting this on CI will be more impactful than most other workflows since IIRC no one is using Windows as their main machine and so if things break it might take a bit for us to realize and respond.

In D5882#178870, @atul wrote:

I think getting this on CI will be more impactful than most other workflows since IIRC no one is using Windows as their main machine and so if things break it might take a bit for us to realize and respond.

Specifically the GitHub Actions CI that runs when a diff is landed. They have prebuilt Windows machines that should make this easy. I don't think it'd be worth it at all to get this working with our Buildkite CI.

Agree with both @tomek and @atul's feedback. @tomek's can probably be addressed in this diff, whereas @atul's probably needs a follow-up Linear task – please create before landing!

This revision is now accepted and ready to land.Dec 20 2022, 1:58 PM

Simplified code.
Task for the CI build: ENG-2532

desktop/src/handle-squirrel-event.js
21 ↗(On Diff #19543)

Yes, according to the squirrel documentation. For some of them there will be a second argument that's an app version, but we can ignore that for now.

Rebased. Changed link to the icon hosted on aws.

Can we land this now? Want to make sure we have a Windows installer hosted in the cloud ASAP