feat: sync version download statuses between windows

[erikian]

This allows the download status and progress to be shared between windows:

2023-07-06_23-50-57.mp4

I also tried to make the BroadcastChannel logic extensible so we can use it to sync other data in the future.

[erikian]

Looks like the Node.js version of BroadcastChannel and Jest don't get along on CircleCI (though it works fine on my machine), the tests seem to run but Jest hangs after they're finished (maybe it actually creates a worker thread that keeps running?) Anyway, I mocked it out and now it works.

[dsanders11]

Looks like the Node.js version of BroadcastChannel and Jest don't get along on CircleCI (though it works fine on my machine), the tests seem to run but Jest hangs after they're finished (maybe it actually creates a worker thread that keeps running?) Anyway, I mocked it out and now it works.

Mocking is the correct thing to do here, rather than trying to use BroadcastChannel from node:worker_threads - we don't want to trigger any real behavior during the tests. 👍

[dsanders11]

I think the typings are cleaner if you use a discriminated union on the possible message types - removes the need to do a type cast.

Also shouldn't be adding anything to ambient.d.ts for this, can all be accomplished with imports.

diff --git a/src/ambient.d.ts b/src/ambient.d.ts
index 18c01d1f..a9315109 100644
--- a/src/ambient.d.ts
+++ b/src/ambient.d.ts
@@ -2,7 +2,6 @@ import { ReleaseInfo } from '@electron/fiddle-core';
 import * as MonacoType from 'monaco-editor';
 
 import {
-  AppStateBroadcastMessage,
   BisectRequest,
   BlockableAccelerator,
   EditorValues,
@@ -126,8 +125,4 @@ declare global {
       themePath: string;
     };
   }
-
-  interface AppStateBroadcastChannel extends BroadcastChannel {
-    postMessage<T = unknown>(params: AppStateBroadcastMessage<T>): void;
-  }
 }
diff --git a/src/interfaces.ts b/src/interfaces.ts
index 6252c926..87933298 100644
--- a/src/interfaces.ts
+++ b/src/interfaces.ts
@@ -221,11 +221,17 @@ export enum WindowSpecificSetting {
 
 export type WindowSpecificSettingKey = keyof typeof WindowSpecificSetting;
 
-export interface AppStateBroadcastMessage<T = unknown> {
-  type: AppStateBroadcastMessageType;
-  payload: T;
+export interface AppStateBroadcastChannel extends BroadcastChannel {
+  postMessage(params: AppStateBroadcastMessage): void;
 }
 
+// Use a discriminated union of types (there's only one here at the moment) so
+// that the payload can easily be typed correctly for each message type
+export type AppStateBroadcastMessage = {
+  type: AppStateBroadcastMessageType.syncVersion;
+  payload: RunnableVersion;
+};
+
 export enum AppStateBroadcastMessageType {
   syncVersion = 'syncVersion',
 }
diff --git a/src/renderer/state.ts b/src/renderer/state.ts
index a02bd1e5..0d1f95ca 100644
--- a/src/renderer/state.ts
+++ b/src/renderer/state.ts
@@ -16,6 +16,7 @@ import {
 } from 'mobx';
 
 import {
+  AppStateBroadcastChannel,
   AppStateBroadcastMessage,
   AppStateBroadcastMessageType,
   BlockableAccelerator,
@@ -215,7 +216,7 @@ export class AppState {
 
   // Notifies other windows that this version has changed so they can update their state to reflect that.
   private updateVersionDownloadStatus(version: RunnableVersion) {
-    this.broadcastChannel.postMessage<RunnableVersion>({
+    this.broadcastChannel.postMessage({
       type: AppStateBroadcastMessageType.syncVersion,
       payload: { ...version },
     });
@@ -445,7 +446,7 @@ export class AppState {
 
         switch (type) {
           case AppStateBroadcastMessageType.syncVersion: {
-            this.addNewVersions([payload as RunnableVersion]);
+            this.addNewVersions([payload]);
 
             break;
           }

[erikian]

Done. To be extra thorough, we'd still need to type cast if we want the general case of multiple types to work with the current Typescript version — but since we only have one type right now, it's probably simpler to leave it as is and just add the type casts if we happen to add any other types before #1389 lands.

[dsanders11]

Let's get some test coverage on this before we merge.

There's also a couple of corner cases we should think about more carefully:

• We should probably handle the case where one window updates the Electron versions (can happen manually under settings, or by loading a gist with a new version specified)
  • Maybe we make the broadcast message more generic, make it syncVersions (plural) - takes an array
  • Make a private AppState.addNewVersionsImpl and move the existing code there
  • Then broadcast the message in AppState.addNewVersions with the full list of versions - the broadcast message listener would use the private AppState.addNewVersionsImpl to avoid triggering a new broadcast
• What happens to the UI if one window removes a version another window is using? Might need to make a separate issue for this, we should likely prevent removing a version which any window is using - although figuring out the best way to share that state might be tricky (may need to use the main process to coordinate that)
  • I've opened Prevent Removing Other Window Current Versions #1394 with some more thoughts on this

[dsanders11]

I extended test coverage to test the broadcasting side as well (not just the receiving) and also made AppState.broadcastChannel private, as I think that makes the most sense. You can pick up the changes from this commit if they look good to you: dsanders11@21ac67a

[erikian]

Thanks for the test case, looks solid!

[dsanders11]

@erikian, since #1404 landed, I've pushed c79fdf9 to also broadcast isDownloadingAll so that windows stay in sync with that as well - otherwise one window would see versions downloading without the the downloading all state reflecting it.

If c79fdf9 looks good to you we can go ahead and merge this. 👍

[erikian]

LGTM!