Repository: electron/fiddle (ts)

Issue:
"Save" dialog goes behind main window when creating new folder
To repro:

1. Start a new fiddle
2. Cmd+S
3. Click "New Folder"
4. Click either "Cancel" or "Create"

Expected behavior:
The new folder is created and selected in the save dialog, and the save dialog remains focused

Actual behavior:
The new folder is created and selected, but the save dialog disappears behind the main window. <kbd>Cmd+`</kbd> brings it back.

Pull request:
refactor: make save dialog synchronous
Fixes #551 by avoiding the offending API.

Swaps out `openDialog` for `openDialogSync` for saving local Fiddles.


Commits:
* cc84422 contains the code itself.
* c897f79 updates the tests so they pass.
* 212cd08 removes `<any>` types from `jest.Mock` in the tests.


Patch:
diff --git a/src/main/files.ts b/src/main/files.ts
index eb92a24c..60cdb548 100644
--- a/src/main/files.ts
+++ b/src/main/files.ts
@@ -43,13 +43,13 @@ export async function showOpenDialog() {
  */
 export async function showSaveDialog(event?: IpcEvents, as?: string) {
   // We want to save to a folder, so we'll use an open dialog here
-  const { filePaths } = await dialog.showOpenDialog({
+  const filePaths = dialog.showOpenDialogSync({
     buttonLabel: 'Save here',
     properties: ['openDirectory', 'createDirectory'],
     title: `Save Fiddle${as ? ` as ${as}` : ''}`,
   });
 
-  if (!filePaths || filePaths.length < 1) {
+  if (!Array.isArray(filePaths) || filePaths.length === 0) {
     return;
   }
 
diff --git a/yarn.lock b/yarn.lock
index a33eeb1f..be798123 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -7766,6 +7766,13 @@ lru-cache@^4.0.1:
     pseudomap "^1.0.2"
     yallist "^2.1.2"
 
+lru-cache@^6.0.0:
+  version "6.0.0"
+  resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-6.0.0.tgz#6d6fe6570ebd96aaf90fcad1dafa3b2566db3a94"
+  integrity sha512-Jo6dJ04CmSjuznwJSS3pUeWmd/H0ffTlkXXgwZi+eq1UCmqQwCh+eLsYOYCwY991i2Fah4h1BEMCx4qThGbsiA==
+  dependencies:
+    yallist "^4.0.0"
+
 lru_map@^0.3.3:
   version "0.3.3"
   resolved "https://registry.yarnpkg.com/lru_map/-/lru_map-0.3.3.tgz#b5c8351b9464cbd750335a79650a0ec0e56118dd"
@@ -8315,9 +8322,9 @@ node-modules-regexp@^1.0.0:
   integrity sha1-jZ2+KJZKSsVxLpExZCEHxx6Q7EA=
 
 node-notifier@^8.0.0:
-  version "8.0.0"
-  resolved "https://registry.yarnpkg.com/node-notifier/-/node-notifier-8.0.0.tgz#a7eee2d51da6d0f7ff5094bc7108c911240c1620"
-  integrity sha512-46z7DUmcjoYdaWyXouuFNNfUo6eFa94t23c53c+lG/9Cvauk4a98rAUp9672X5dxGdQmLpPzTxzu8f/OeEPaFA==
+  version "8.0.1"
+  resolved "https://registry.yarnpkg.com/node-notifier/-/node-notifier-8.0.1.tgz#f86e89bbc925f2b068784b31f382afdc6ca56be1"
+  integrity sha512-BvEXF+UmsnAfYfoapKM9nGxnP+Wn7P91YfXmrKnfcYCx6VBeoN5Ez5Ogck6I8Bi5k4RlpqRYaw75pAwzX9OphA==
   dependencies:
     growly "^1.3.0"
     is-wsl "^2.2.0"
@@ -10687,9 +10694,11 @@ semver@7.0.0:
   integrity sha512-+GB6zVA9LWh6zovYQLALHwv5rb2PHGlJi3lfiqIHxR0uuwCgefcOJc59v9fv1w8GbStwxuuqqAjI9NMAOOgq1A==
 
 semver@7.x, semver@^7.1.1, semver@^7.1.3, semver@^7.2.1, semver@^7.3.2:
-  version "7.3.2"
-  resolved "https://registry.yarnpkg.com/semver/-/semver-7.3.2.tgz#604962b052b81ed0786aae84389ffba70ffd3938"
-  integrity sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==
+  version "7.3.4"
+  resolved "https://registry.yarnpkg.com/semver/-/semver-7.3.4.tgz#27aaa7d2e4ca76452f98d3add093a72c943edc97"
+  integrity sha512-tCfb2WLjqFAtXn4KEdxIhalnRtoKFN7nAwj0B3ZXCbQloV2tq5eDbcTmT68JJD3nRJq24/XgxtQKFIpQdtvmVw==
+  dependencies:
+    lru-cache "^6.0.0"
 
 semver@^6.0.0, semver@^6.1.0, semver@^6.1.2, semver@^6.3.0:
   version "6.3.0"
@@ -12241,9 +12250,9 @@ uuid@^3.3.2:
   integrity sha512-HjSDRw6gZE5JMggctHBcjVak08+KEVhSIiDzFnT9S9aegmp85S/bReBVTb4QTFaRNptJ9kuYaNhnbNEOkbKb/A==
 
 uuid@^8.3.0:
-  version "8.3.1"
-  resolved "https://registry.yarnpkg.com/uuid/-/uuid-8.3.1.tgz#2ba2e6ca000da60fce5a196954ab241131e05a31"
-  integrity sha512-FOmRr+FmWEIG8uhZv6C2bTgEVXsHk08kE7mPlrBbEe+c3r9pjceVPgupIfNIhc4yx55H69OXANrUaSuu9eInKg==
+  version "8.3.2"
+  resolved "https://registry.yarnpkg.com/uuid/-/uuid-8.3.2.tgz#80d5b5ced271bb9af6c445f21a1a04c606cefbe2"
+  integrity sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==
 
 v8-compile-cache@^2.0.0, v8-compile-cache@^2.0.3, v8-compile-cache@^2.1.1:
   version "2.1.1"


Test patch:
diff --git a/tests/main/files-spec.ts b/tests/main/files-spec.ts
index 3fcb5a72..f06ebcb1 100644
--- a/tests/main/files-spec.ts
+++ b/tests/main/files-spec.ts
@@ -27,10 +27,10 @@ const mockTarget = {
 
 describe('files', () => {
   beforeEach(() => {
-    (dialog.showOpenDialog as jest.Mock<any>).mockResolvedValue({
+    (dialog.showOpenDialog as jest.Mock).mockResolvedValue({
       filePaths: ['my/fake/path'],
     });
-    (getOrCreateMainWindow as jest.Mock<any>).mockReturnValue(mockTarget);
+    (getOrCreateMainWindow as jest.Mock).mockReturnValue(mockTarget);
 
     ipcMainManager.readyWebContents.add(mockTarget.webContents as any);
   });
@@ -44,7 +44,7 @@ describe('files', () => {
       ]);
 
       ipcMainManager.emit(IpcEvents.FS_SAVE_FIDDLE_DIALOG);
-      expect(dialog.showOpenDialog).toHaveBeenCalled();
+      expect(dialog.showOpenDialogSync).toHaveBeenCalled();
     });
   });
 
@@ -52,7 +52,7 @@ describe('files', () => {
     it('tries to open an "open" dialog', async () => {
       await showOpenDialog();
 
-      const call = (dialog.showOpenDialog as jest.Mock<any>).mock.calls[0];
+      const call = (dialog.showOpenDialog as jest.Mock).mock.calls[0];
 
       expect(dialog.showOpenDialog).toHaveBeenCalled();
       expect(call[0]).toEqual({
@@ -62,7 +62,7 @@ describe('files', () => {
     });
 
     it('notifies the main window of the event', async () => {
-      (getOrCreateMainWindow as jest.Mock<any>).mockReturnValue(mockTarget);
+      (getOrCreateMainWindow as jest.Mock).mockReturnValue(mockTarget);
 
       await showOpenDialog();
 
@@ -71,12 +71,12 @@ describe('files', () => {
   });
 
   describe('showSaveDialog', () => {
-    it('tries to open an "open" dialog to be used as a save dialog', () => {
-      showSaveDialog();
+    it('tries to open an "open" dialog to be used as a save dialog', async () => {
+      await showSaveDialog();
 
-      const call = (dialog.showOpenDialog as jest.Mock<any>).mock.calls[0];
+      const call = (dialog.showOpenDialogSync as jest.Mock).mock.calls[0];
 
-      expect(dialog.showOpenDialog).toHaveBeenCalled();
+      expect(dialog.showOpenDialogSync).toHaveBeenCalled();
       expect(call[0]).toEqual({
         buttonLabel: 'Save here',
         properties: ['openDirectory', 'createDirectory'],
@@ -84,12 +84,12 @@ describe('files', () => {
       });
     });
 
-    it('tries to open an "open" dialog to be used as a save as dialog', () => {
-      showSaveDialog(IpcEvents.FS_SAVE_FIDDLE, 'hello');
+    it('tries to open an "open" dialog to be used as a save as dialog', async () => {
+      await showSaveDialog(IpcEvents.FS_SAVE_FIDDLE, 'hello');
 
-      const call = (dialog.showOpenDialog as jest.Mock<any>).mock.calls[0];
+      const call = (dialog.showOpenDialogSync as jest.Mock).mock.calls[0];
 
-      expect(dialog.showOpenDialog).toHaveBeenCalled();
+      expect(dialog.showOpenDialogSync).toHaveBeenCalled();
       expect(call[0]).toEqual({
         buttonLabel: 'Save here',
         properties: ['openDirectory', 'createDirectory'],
@@ -97,24 +97,16 @@ describe('files', () => {
       });
     });
 
-    it('handles not getting a path returned', async (done) => {
-      (dialog.showOpenDialog as jest.Mock<any>).mockResolvedValueOnce({
-        filePaths: [],
-      });
-
+    it('handles not getting a path returned', async () => {
+      (dialog.showOpenDialogSync as jest.Mock).mockReturnValueOnce([]);
       await showSaveDialog();
-
-      process.nextTick(() => {
-        expect(fs.existsSync).toHaveBeenCalledTimes(0);
-        done();
-      });
+      expect(fs.existsSync).toHaveBeenCalledTimes(0);
     });
 
     it('ensures that the target is empty on save', async () => {
-      (dialog.showMessageBox as jest.Mock<any>).mockImplementation(
-        async () => true,
-      );
-      (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
+      (dialog.showOpenDialogSync as jest.Mock).mockReturnValue(['path']);
+      (dialog.showMessageBox as jest.Mock).mockResolvedValue(true);
+      (fs.existsSync as jest.Mock).mockReturnValue(true);
       ipcMainManager.readyWebContents.add(mockTarget.webContents as any);
 
       await showSaveDialog();
@@ -124,11 +116,10 @@ describe('files', () => {
     });
 
     it('does not overwrite files without consent', async () => {
-      (dialog.showMessageBox as jest.Mock<any>).mockImplementation(
-        async () => false,
-      );
-      (getOrCreateMainWindow as jest.Mock<any>).mockReturnValue(mockTarget);
-      (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
+      (dialog.showOpenDialogSync as jest.Mock).mockReturnValue(['path']);
+      (dialog.showMessageBox as jest.Mock).mockResolvedValue(false);
+      (getOrCreateMainWindow as jest.Mock).mockReturnValue(mockTarget);
+      (fs.existsSync as jest.Mock).mockReturnValue(true);
 
       await showSaveDialog();
 
@@ -137,11 +128,12 @@ describe('files', () => {
     });
 
     it('does not overwrite files if an error happens', async () => {
-      (dialog.showMessageBox as jest.Mock<any>).mockImplementation(async () => {
+      (dialog.showOpenDialogSync as jest.Mock).mockReturnValue(['path']);
+      (dialog.showMessageBox as jest.Mock).mockImplementation(async () => {
         throw new Error('Nope');
       });
-      (getOrCreateMainWindow as jest.Mock<any>).mockReturnValue(mockTarget);
-      (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
+      (getOrCreateMainWindow as jest.Mock).mockReturnValue(mockTarget);
+      (fs.existsSync as jest.Mock).mockReturnValue(true);
 
       let errored = false;
 
diff --git a/tests/main/menu-spec.ts b/tests/main/menu-spec.ts
index bfcc5521..886558e2 100644
--- a/tests/main/menu-spec.ts
+++ b/tests/main/menu-spec.ts
@@ -17,11 +17,7 @@ jest.mock('../../src/main/ipc');
 describe('menu', () => {
   beforeEach(() => {
     electron.app.name = 'Electron Fiddle';
-    // need to keep this deprecated API for electron-default-menu mock as well
-    (electron.app.getName as any).mockReturnValue('Electron Fiddle');
-    (electron.dialog.showOpenDialog as any).mockReturnValue(
-      Promise.resolve({}),
-    );
+    (electron.dialog.showOpenDialog as jest.Mock).mockResolvedValue({});
   });
 
   afterEach(() => {
@@ -307,7 +303,7 @@ describe('menu', () => {
 
       it('saves a Fiddle as', () => {
         file.submenu[6].click();
-        expect(electron.dialog.showOpenDialog).toHaveBeenCalled();
+        expect(electron.dialog.showOpenDialogSync).toHaveBeenCalled();
       });
 
       it('saves a Fiddle as a gist', () => {
@@ -319,7 +315,7 @@ describe('menu', () => {
 
       it('saves a Fiddle as a forge project', () => {
         file.submenu[9].click();
-        expect(electron.dialog.showOpenDialog).toHaveBeenCalled();
+        expect(electron.dialog.showOpenDialogSync).toHaveBeenCalled();
       });
 
       it('saves a Fiddle with blocked accelerator', () => {
@@ -333,7 +329,7 @@ describe('menu', () => {
       it('saves as a Fiddle with blocked accelerator', () => {
         setupMenu([BlockableAccelerator.saveAs]);
         file.submenu[6].click();
-        expect(electron.dialog.showOpenDialog).toHaveBeenCalled();
+        expect(electron.dialog.showOpenDialogSync).toHaveBeenCalled();
       });
     });
   });
diff --git a/tests/mocks/electron.ts b/tests/mocks/electron.ts
index 14db7a97..48930108 100644
--- a/tests/mocks/electron.ts
+++ b/tests/mocks/electron.ts
@@ -167,8 +167,9 @@ const electronMock = {
     start: jest.fn(),
   },
   dialog: {
-    showOpenDialog: jest.fn(() => Promise.resolve({})),
-    showMessageBox: jest.fn(() => Promise.resolve({})),
+    showOpenDialog: jest.fn().mockResolvedValue({}),
+    showOpenDialogSync: jest.fn().mockReturnValue(['path']),
+    showMessageBox: jest.fn().mockResolvedValue({}),
   },
   ipcMain: new MockIPC(),
   ipcRenderer: new MockIPC(),