Various QoL fixes for Desktop App (#2999)

* Some ESLint fixes

* Add login/logout signal to API, clear mentions on logout and flush cookies on login/logout

* Fix issue where local and HTTP-only servers would not validate correctly

* Reduce noise of renderer logging, adjust a few local renderer logs to be louder when needed

* Fallback to beginning of hostname for servers that don't change the site name

* Fix Save Image crash

* Update the name for insecure servers too

* Fix test

* Fix lint

* Reduce repetition

.eslintrc.json

@@ -48,9 +48,9 @@
                         "position": "after"
                     },
                     {
-                        "pattern": "(app|common|main|renderer){,/**}",
+                        "pattern": "@(app|common|main|renderer){,/**}",
-                        "group": "external",
+                        "group": "internal",
-                        "position": "after"
+                        "position": "before"
                     },
                     {
                         "pattern": "types{,/**}",

api-types/index.ts

@@ -26,6 +26,8 @@ export type DesktopAPI = {
         idleTime: number,
         isSystemEvent: boolean,
     ) => void) => () => void;
+    onLogin: () => void;
+    onLogout: () => void;
 
     // Unreads/mentions/notifications
     sendNotification: (title: string, body: string, channelId: string, teamId: string, url: string, silent: boolean, soundName: string) => Promise<{result: string; reason?: string; data?: string}>;

api-types/lib/index.d.ts

@@ -20,6 +20,8 @@ export type DesktopAPI = {
     reactAppInitialized: () => void;
     setSessionExpired: (isExpired: boolean) => void;
     onUserActivityUpdate: (listener: (userIsActive: boolean, idleTime: number, isSystemEvent: boolean) => void) => () => void;
+    onLogin: () => void;
+    onLogout: () => void;
     sendNotification: (title: string, body: string, channelId: string, teamId: string, url: string, silent: boolean, soundName: string) => Promise<{
         result: string;
         reason?: string;

api-types/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@mattermost/desktop-api",
-  "version": "5.8.0-3",
+  "version": "5.8.0-4",
   "description": "Shared types for the Desktop App API provided to the Web App",
   "keywords": [
     "mattermost"
@@ -28,6 +28,7 @@
   "scripts": {
     "build": "tsc --build --verbose",
     "run": "tsc --watch --preserveWatchOutput",
-    "clean": "rm -rf tsconfig.tsbuildinfo ./lib"
+    "clean": "rm -rf tsconfig.tsbuildinfo ./lib",
+    "prepublish": "npm run build && rm -rf tsconfig.tsbuildinfo"
   }
 }

e2e/utils/analyze-flaky-test.js

@@ -35,6 +35,7 @@ function analyzeFlakyTests() {
         return {commentBody, newFailedTests};
     } catch (error) {
         console.error('Error analyzing failures:', error);
+        return {};
     }
 }
 

package-lock.json

@@ -43,7 +43,7 @@
         "@babel/preset-typescript": "7.23.3",
         "@electron/fuses": "1.6.0",
         "@electron/rebuild": "3.6.0",
-        "@mattermost/desktop-api": "*",
+        "@mattermost/desktop-api": "file:api-types",
         "@mattermost/eslint-plugin": "1.1.0-0",
         "@types/auto-launch": "5.0.5",
         "@types/jest": "29.4.1",
@@ -100,6 +100,20 @@
         "node": ">=18.0.0"
       }
     },
+    "api-types": {
+      "name": "@mattermost/desktop-api",
+      "version": "5.8.0-4",
+      "dev": true,
+      "license": "MIT",
+      "peerDependencies": {
+        "typescript": "^4.3.0 || ^5.0.0"
+      },
+      "peerDependenciesMeta": {
+        "typescript": {
+          "optional": true
+        }
+      }
+    },
     "node_modules/@aashutoshrathi/word-wrap": {
       "version": "1.2.6",
       "resolved": "https://registry.npmjs.org/@aashutoshrathi/word-wrap/-/word-wrap-1.2.6.tgz",
@@ -5038,18 +5052,8 @@
       "integrity": "sha512-vQThJ4SAynnS2u94lQtZ9xANsStpVh8uTpsJascHJOWcavLuL2aDmMLgvg9EAx8Z1qRmTdP6hF5+IU5+9E9+Jg=="
     },
     "node_modules/@mattermost/desktop-api": {
-      "version": "5.8.0-1",
+      "resolved": "api-types",
-      "resolved": "https://registry.npmjs.org/@mattermost/desktop-api/-/desktop-api-5.8.0-1.tgz",
+      "link": true
-      "integrity": "sha512-uS/vBY9ehaMolR2j7EAQSSZJl0TO5VHxDFraxy75QHpq9hD504lLlNKIjGUNgPeZSMWde+ah29xX2lBq12gt7A==",
-      "dev": true,
-      "peerDependencies": {
-        "typescript": "5.3.3"
-      },
-      "peerDependenciesMeta": {
-        "typescript": {
-          "optional": true
-        }
-      }
     },
     "node_modules/@mattermost/eslint-plugin": {
       "version": "1.1.0-0",
@@ -9913,17 +9917,14 @@
       }
     },
     "node_modules/electron-dl": {
-      "version": "3.5.2",
+      "version": "3.5.0",
-      "resolved": "https://registry.npmjs.org/electron-dl/-/electron-dl-3.5.2.tgz",
+      "resolved": "https://registry.npmjs.org/electron-dl/-/electron-dl-3.5.0.tgz",
-      "integrity": "sha512-i104cl+u8yJ0lhpRAtUWfeGuWuL1PL6TBiw2gLf0MMIBjfgE485Ags2mcySx4uWU9P9uj/vsD3jd7X+w1lzZxw==",
+      "integrity": "sha512-Oj+VSuScVx8hEKM2HEvTQswTX6G3MLh7UoAz/oZuvKyNDfudNi1zY6PK/UnFoK1nCl9DF6k+3PFwElKbtZlDig==",
       "dependencies": {
         "ext-name": "^5.0.0",
         "pupa": "^2.0.1",
         "unused-filename": "^2.1.0"
       },
-      "engines": {
-        "node": ">=12"
-      },
       "funding": {
         "url": "https://github.com/sponsors/sindresorhus"
       }

package.json

@@ -135,7 +135,7 @@
     "@babel/preset-typescript": "7.23.3",
     "@electron/fuses": "1.6.0",
     "@electron/rebuild": "3.6.0",
-    "@mattermost/desktop-api": "*",
+    "@mattermost/desktop-api": "file:api-types",
     "@mattermost/eslint-plugin": "1.1.0-0",
     "@types/auto-launch": "5.0.5",
     "@types/jest": "29.4.1",

src/app/serverViewState.test.js

@@ -417,6 +417,26 @@ describe('app/serverViewState', () => {
             expect(result.validatedURL).toBe('http://server.com/');
         });
 
+        it('should be able to recognize localhost with a port and add the appropriate prefix', async () => {
+            ServerInfo.mockImplementation(({url}) => ({
+                fetchConfigData: jest.fn().mockImplementation(() => {
+                    if (url.startsWith('https:')) {
+                        return undefined;
+                    }
+
+                    return {
+                        serverVersion: '7.8.0',
+                        siteName: 'Mattermost',
+                        siteURL: url,
+                    };
+                }),
+            }));
+
+            const result = await serverViewState.handleServerURLValidation({}, 'localhost:8065');
+            expect(result.status).toBe(URLValidationStatus.Insecure);
+            expect(result.validatedURL).toBe('http://localhost:8065/');
+        });
+
         it('should show a warning when the ping request times out', async () => {
             ServerInfo.mockImplementation(() => ({
                 fetchConfigData: jest.fn().mockImplementation(() => {

src/app/serverViewState.ts

@@ -233,7 +233,7 @@ export class ServerViewState {
         if (!isValidURL(url)) {
             // If it already includes the protocol, tell them it's invalid
             if (isValidURI(url)) {
-                httpUrl = url.replace(/^(.+):/, 'https:');
+                httpUrl = url.replace(/^((.+):\/\/)?/, 'https://');
             } else {
                 // Otherwise add HTTPS for them
                 httpUrl = `https://${url}`;
@@ -260,11 +260,13 @@ export class ServerViewState {
 
         // Try and get remote info from the most secure URL, otherwise use the insecure one
         let remoteURL = secureURL;
+        const insecureURL = parseURL(secureURL.toString().replace(/^https:/, 'http:'));
         let remoteInfo = await this.testRemoteServer(secureURL);
-        if (!remoteInfo) {
+        if (!remoteInfo && insecureURL) {
-            if (secureURL.toString() !== parsedURL.toString()) {
+            // Try to fall back to HTTP
-                remoteURL = parsedURL;
+            remoteInfo = await this.testRemoteServer(insecureURL);
-                remoteInfo = await this.testRemoteServer(parsedURL);
+            if (remoteInfo) {
+                remoteURL = insecureURL;
             }
         }
 
@@ -274,9 +276,11 @@ export class ServerViewState {
             return {status: URLValidationStatus.NotMattermost, validatedURL: parsedURL.toString()};
         }
 
+        const remoteServerName = remoteInfo.siteName === 'Mattermost' ? remoteURL.host.split('.')[0] : remoteInfo.siteName;
+
         // If we were only able to connect via HTTP, warn the user that the connection is not secure
         if (remoteURL.protocol === 'http:') {
-            return {status: URLValidationStatus.Insecure, serverVersion: remoteInfo.serverVersion, validatedURL: remoteURL.toString()};
+            return {status: URLValidationStatus.Insecure, serverVersion: remoteInfo.serverVersion, serverName: remoteServerName, validatedURL: remoteURL.toString()};
         }
 
         // If the URL doesn't match the Site URL, set the URL to the correct one
@@ -292,15 +296,15 @@ export class ServerViewState {
                 // If we can't reach the remote Site URL, there's probably a configuration issue
                 const remoteSiteURLInfo = await this.testRemoteServer(parsedSiteURL);
                 if (!remoteSiteURLInfo) {
-                    return {status: URLValidationStatus.URLNotMatched, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteURL.toString()};
+                    return {status: URLValidationStatus.URLNotMatched, serverVersion: remoteInfo.serverVersion, serverName: remoteServerName, validatedURL: remoteURL.toString()};
                 }
             }
 
             // Otherwise fix it for them and return
-            return {status: URLValidationStatus.URLUpdated, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteInfo.siteURL};
+            return {status: URLValidationStatus.URLUpdated, serverVersion: remoteInfo.serverVersion, serverName: remoteServerName, validatedURL: remoteInfo.siteURL};
         }
 
-        return {status: URLValidationStatus.OK, serverVersion: remoteInfo.serverVersion, serverName: remoteInfo.siteName, validatedURL: remoteURL.toString()};
+        return {status: URLValidationStatus.OK, serverVersion: remoteInfo.serverVersion, serverName: remoteServerName, validatedURL: remoteURL.toString()};
     };
 
     private handleCloseView = (event: IpcMainEvent, viewId: string) => {

src/common/appState.ts

@@ -49,6 +49,8 @@ export class AppState extends EventEmitter {
         this.expired.delete(viewId);
         this.mentions.delete(viewId);
         this.unreads.delete(viewId);
+
+        this.emitStatusForView(viewId);
     };
 
     emitStatus = () => {

src/main/app/initialize.ts

@@ -87,9 +87,9 @@ import {
     shouldShowTrayIcon,
     updateSpellCheckerLocales,
     wasUpdated,
-    initCookieManager,
     migrateMacAppStore,
     updateServerInfos,
+    flushCookiesStore,
 } from './utils';
 import {
     handleClose,
@@ -200,6 +200,12 @@ function initializeAppEventListeners() {
     app.on('child-process-gone', handleChildProcessGone);
     app.on('login', AuthManager.handleAppLogin);
     app.on('will-finish-launching', handleAppWillFinishLaunching);
+
+    // Somehow cookies are not immediately saved to disk.
+    // So manually flush cookie store to disk on closing the app.
+    // https://github.com/electron/electron/issues/8416
+    // TODO: We can remove this once every server supported will flush on login/logout
+    app.on('before-quit', flushCookiesStore);
 }
 
 function initializeBeforeAppReady() {
@@ -347,7 +353,6 @@ async function initializeAfterAppReady() {
             catch((err) => log.error('An error occurred: ', err));
     }
 
-    initCookieManager(defaultSession);
     MainWindow.show();
 
     let deeplinkingURL;

src/main/app/utils.ts

@@ -4,7 +4,7 @@
 import fs from 'fs';
 import path from 'path';
 
-import type {BrowserWindow, Rectangle, Session} from 'electron';
+import type {BrowserWindow, Rectangle} from 'electron';
 import {app, Menu, session, dialog, nativeImage, screen} from 'electron';
 import isDev from 'electron-is-dev';
 
@@ -166,26 +166,13 @@ export function resizeScreen(browserWindow: BrowserWindow) {
     handle();
 }
 
-export function flushCookiesStore(session: Session) {
+export function flushCookiesStore() {
     log.debug('flushCookiesStore');
-    session.cookies.flushStore().catch((err) => {
+    session.defaultSession.cookies.flushStore().catch((err) => {
         log.error(`There was a problem flushing cookies:\n${err}`);
     });
 }
 
-export function initCookieManager(session: Session) {
-    // Somehow cookies are not immediately saved to disk.
-    // So manually flush cookie store to disk on closing the app.
-    // https://github.com/electron/electron/issues/8416
-    app.on('before-quit', () => {
-        flushCookiesStore(session);
-    });
-
-    app.on('browser-window-blur', () => {
-        flushCookiesStore(session);
-    });
-}
-
 export function migrateMacAppStore() {
     const migrationPrefs = new JsonFileManager<MigrationInfo>(migrationInfoPath);
     const oldPath = path.join(app.getPath('userData'), '../../../../../../../Library/Application Support/Mattermost');

src/main/preload/externalAPI.ts

@@ -70,6 +70,9 @@ const desktopAPI: DesktopAPI = {
     setSessionExpired: (isExpired) => ipcRenderer.send(SESSION_EXPIRED, isExpired),
     onUserActivityUpdate: (listener) => createListener(USER_ACTIVITY_UPDATE, listener),
 
+    onLogin: () => ipcRenderer.send(APP_LOGGED_IN),
+    onLogout: () => ipcRenderer.send(APP_LOGGED_OUT),
+
     // Unreads/mentions/notifications
     sendNotification: (title, body, channelId, teamId, url, silent, soundName) =>
         ipcRenderer.invoke(NOTIFY_MENTION, title, body, channelId, teamId, url, silent, soundName),
@@ -178,11 +181,11 @@ setInterval(() => {
 
 const onLoad = () => {
     if (document.getElementById('root') === null) {
-        console.log('The guest is not assumed as mattermost-webapp');
+        console.warn('The guest is not assumed as mattermost-webapp');
         return;
     }
     watchReactAppUntilInitialized(() => {
-        console.log('Legacy preload initialized');
+        console.warn('Legacy preload initialized');
         ipcRenderer.send(REACT_APP_INITIALIZED);
         ipcRenderer.invoke(REQUEST_BROWSER_HISTORY_STATUS).then(sendHistoryButtonReturn);
     });

src/main/views/viewManager.test.js

@@ -56,6 +56,10 @@ jest.mock('common/utils/url', () => ({
     equalUrlsIgnoringSubpath: jest.fn(),
 }));
 
+jest.mock('main/app/utils', () => ({
+    flushCookiesStore: jest.fn(),
+}));
+
 jest.mock('main/i18nManager', () => ({
     localizeMessage: jest.fn(),
 }));

src/main/views/viewManager.ts

@@ -43,6 +43,7 @@ import {getFormattedPathName, parseURL} from 'common/utils/url';
 import Utils from 'common/utils/util';
 import type {MattermostView} from 'common/views/View';
 import {TAB_MESSAGING} from 'common/views/View';
+import {flushCookiesStore} from 'main/app/utils';
 import {localizeMessage} from 'main/i18nManager';
 import MainWindow from 'main/windows/mainWindow';
 
@@ -471,11 +472,24 @@ export class ViewManager {
     };
 
     private handleAppLoggedIn = (event: IpcMainEvent) => {
-        this.getViewByWebContentsId(event.sender.id)?.onLogin(true);
+        log.debug('handleAppLoggedIn', event.sender.id);
+        const view = this.getViewByWebContentsId(event.sender.id);
+        if (!view) {
+            return;
+        }
+        view.onLogin(true);
+        flushCookiesStore();
     };
 
     private handleAppLoggedOut = (event: IpcMainEvent) => {
-        this.getViewByWebContentsId(event.sender.id)?.onLogin(false);
+        log.debug('handleAppLoggedOut', event.sender.id);
+        const view = this.getViewByWebContentsId(event.sender.id);
+        if (!view) {
+            return;
+        }
+        view.onLogin(false);
+        AppState.clear(view.id);
+        flushCookiesStore();
     };
 
     private handleBrowserHistoryPush = (e: IpcMainEvent, pathName: string) => {

src/main/views/webContentEvents.test.js

@@ -245,8 +245,7 @@ describe('main/views/webContentsEvents', () => {
         const logObject = {
             error: jest.fn(),
             warn: jest.fn(),
-            info: jest.fn(),
+            debug: jest.fn(),
-            verbose: jest.fn(),
             withPrefix: jest.fn().mockReturnThis(),
         };
         webContentsEventManager.log = jest.fn().mockReturnValue(logObject);
@@ -258,10 +257,10 @@ describe('main/views/webContentsEvents', () => {
 
         it('should respect logging levels', () => {
             consoleMessage({}, 0, 'test0', 0, '');
-            expect(logObject.verbose).toHaveBeenCalledWith('test0');
+            expect(logObject.debug).toHaveBeenCalledWith('test0');
 
             consoleMessage({}, 1, 'test1', 0, '');
-            expect(logObject.info).toHaveBeenCalledWith('test1');
+            expect(logObject.debug).toHaveBeenCalledWith('test1');
 
             consoleMessage({}, 2, 'test2', 0, '');
             expect(logObject.warn).toHaveBeenCalledWith('test2');
@@ -273,11 +272,11 @@ describe('main/views/webContentsEvents', () => {
         it('should only add line numbers for debug and silly', () => {
             getLevel.mockReturnValue('debug');
             consoleMessage({}, 0, 'test1', 42, 'meaning_of_life.js');
-            expect(logObject.verbose).toHaveBeenCalledWith('test1', '(meaning_of_life.js:42)');
+            expect(logObject.debug).toHaveBeenCalledWith('test1', '(meaning_of_life.js:42)');
 
-            getLevel.mockReturnValue('info');
+            getLevel.mockReturnValue('warn');
             consoleMessage({}, 0, 'test2', 42, 'meaning_of_life.js');
-            expect(logObject.verbose).not.toHaveBeenCalledWith('test2', '(meaning_of_life.js:42)');
+            expect(logObject.warn).not.toHaveBeenCalledWith('test2', '(meaning_of_life.js:42)');
         });
     });
 });

src/main/views/webContentEvents.ts

@@ -4,7 +4,7 @@
 import path from 'path';
 
 import type {WebContents, Event} from 'electron';
-import {BrowserWindow, session, shell} from 'electron';
+import {BrowserWindow, shell} from 'electron';
 
 import Config from 'common/config';
 import {Logger, getLevel} from 'common/log';
@@ -116,7 +116,7 @@ export class WebContentsEventManager {
                 return;
             }
             if (this.customLogins[webContentsId]?.inProgress) {
-                flushCookiesStore(session.defaultSession);
+                flushCookiesStore();
                 return;
             }
 
@@ -298,7 +298,7 @@ export class WebContentsEventManager {
 
     private generateHandleConsoleMessage = (webContentsId: number) => (_: Event, level: number, message: string, line: number, sourceId: string) => {
         const wcLog = this.log(webContentsId).withPrefix('renderer');
-        let logFn = wcLog.verbose;
+        let logFn = wcLog.debug;
         switch (level) {
         case ConsoleMessageLevel.Error:
             logFn = wcLog.error;
@@ -306,9 +306,6 @@ export class WebContentsEventManager {
         case ConsoleMessageLevel.Warning:
             logFn = wcLog.warn;
             break;
-        case ConsoleMessageLevel.Info:
-            logFn = wcLog.info;
-            break;
         }
 
         // Only include line entries if we're debugging

src/renderer/components/MainPage.tsx

@@ -162,7 +162,7 @@ class MainPage extends React.PureComponent<Props, State> {
 
         // set page on retry
         window.desktop.onLoadRetry((viewId, retry, err, loadUrl) => {
-            console.log(`${viewId}: failed to load ${err}, but retrying`);
+            console.error(`${viewId}: failed to load ${err}, but retrying`);
             const statusValue = {
                 status: Status.RETRY,
                 extra: {
@@ -179,7 +179,7 @@ class MainPage extends React.PureComponent<Props, State> {
         });
 
         window.desktop.onLoadFailed((viewId, err, loadUrl) => {
-            console.log(`${viewId}: failed to load ${err}`);
+            console.error(`${viewId}: failed to load ${err}`);
             const statusValue = {
                 status: Status.FAILED,
                 extra: {

src/renderer/index.tsx

@@ -56,7 +56,7 @@ class Root extends React.PureComponent<Record<string, never>, State> {
             const configRequest = await window.desktop.getConfiguration() as CombinedConfig;
             return configRequest;
         } catch (err: any) {
-            console.log(`there was an error with the config: ${err}`);
+            console.error(`there was an error with the config: ${err}`);
             if (exitOnError) {
                 window.desktop.quit(`unable to load configuration: ${err}`, err.stack);
             }

webpack.config.base.js

@@ -7,7 +7,7 @@ const path = require('path');
 
 const webpack = require('webpack');
 
-const VERSION = childProcess.execSync('git rev-parse --short HEAD').toString();
+const VERSION = childProcess.execSync('git rev-parse --short HEAD', {cwd: __dirname}).toString();
 const isProduction = process.env.NODE_ENV === 'production';
 const isTest = process.env.NODE_ENV === 'test';
 const isRelease = process.env.GITHUB_WORKFLOW && process.env.GITHUB_WORKFLOW.startsWith('release');