From ab014c26a285dc28f27971365d1bf0ddac277117 Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Wed, 13 Mar 2024 08:53:07 -0400 Subject: [PATCH] Stop constant reloading of BrowserViews for servers that are not available (#2980) --- src/main/server/serverAPI.test.js | 23 ++--------- src/main/server/serverAPI.ts | 5 +-- src/main/server/serverInfo.test.js | 54 +++++++++++++++++++++++++ src/main/server/serverInfo.ts | 13 ++++-- src/main/views/MattermostBrowserView.ts | 22 +++++++--- 5 files changed, 85 insertions(+), 32 deletions(-) create mode 100644 src/main/server/serverInfo.test.js diff --git a/src/main/server/serverAPI.test.js b/src/main/server/serverAPI.test.js index 6a4d1cda..4d755891 100644 --- a/src/main/server/serverAPI.test.js +++ b/src/main/server/serverAPI.test.js @@ -8,7 +8,6 @@ import {net, session} from 'electron'; import {getServerAPI} from './serverAPI'; const validURL = 'http://server-1.com/api/endpoint'; -const badDataURL = 'http://server-1.com/api/bad/endpoint'; const testData = { name: 'some data', value: 'some more data', @@ -21,13 +20,13 @@ jest.mock('electron', () => ({ requestCallback({ on: jest.fn().mockImplementation((event, responseCallback) => { if (event === 'data') { - responseCallback(url === badDataURL ? '98&H09986t&(*6BV789RhN^t97rb6Ev^*e5v89 re5bg^&' : JSON.stringify(testData)); + responseCallback(JSON.stringify(testData)); } if (event === 'end') { responseCallback(); } }), - statusCode: (url === validURL || url === badDataURL) ? 200 : 404, + statusCode: url === validURL ? 200 : 404, }); }), end: jest.fn(), @@ -50,7 +49,7 @@ describe('main/server/serverAPI', () => { false, successFn, ); - expect(successFn).toHaveBeenCalledWith(testData); + expect(successFn).toHaveBeenCalledWith(JSON.stringify(testData)); }); it('should call onError when bad status code received', async () => { @@ -67,20 +66,6 @@ describe('main/server/serverAPI', () => { expect(errorFn).toHaveBeenCalled(); }); - it('should call onError when data parsing fails', async () => { - const successFn = jest.fn(); - const errorFn = jest.fn(); - await getServerAPI( - badDataURL, - false, - successFn, - null, - errorFn, - ); - expect(successFn).not.toHaveBeenCalled(); - expect(errorFn).toHaveBeenCalled(); - }); - it('should call onError when response encounters an error', async () => { net.request.mockImplementation(({url}) => ({ on: jest.fn().mockImplementation((_, requestCallback) => { @@ -90,7 +75,7 @@ describe('main/server/serverAPI', () => { responseCallback(); } }), - statusCode: (url === validURL || url === badDataURL) ? 200 : 404, + statusCode: url === validURL ? 200 : 404, }); }), end: jest.fn(), diff --git a/src/main/server/serverAPI.ts b/src/main/server/serverAPI.ts index 3471e6df..278312d6 100644 --- a/src/main/server/serverAPI.ts +++ b/src/main/server/serverAPI.ts @@ -8,7 +8,7 @@ import {Logger} from 'common/log'; const log = new Logger('serverAPI'); -export async function getServerAPI(url: URL, isAuthenticated: boolean, onSuccess?: (data: T) => void, onAbort?: () => void, onError?: (error: Error) => void) { +export async function getServerAPI(url: URL, isAuthenticated: boolean, onSuccess?: (raw: string) => void, onAbort?: () => void, onError?: (error: Error) => void) { if (isAuthenticated) { const cookies = await session.defaultSession.cookies.get({}); if (!cookies) { @@ -47,8 +47,7 @@ export async function getServerAPI(url: URL, isAuthenticated: boolean, onSucc }); response.on('end', () => { try { - const data = JSON.parse(raw) as T; - onSuccess(data); + onSuccess(raw); } catch (e) { const error = `Error parsing server data from ${url.toString()}`; log.error(error); diff --git a/src/main/server/serverInfo.test.js b/src/main/server/serverInfo.test.js new file mode 100644 index 00000000..d935c833 --- /dev/null +++ b/src/main/server/serverInfo.test.js @@ -0,0 +1,54 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {getServerAPI} from './serverAPI'; +import {ServerInfo} from './serverInfo'; + +jest.mock('./serverAPI', () => ({ + getServerAPI: jest.fn(), +})); + +describe('main/server/serverInfo', () => { + describe('getRemoteInfo', () => { + const serverInfo = new ServerInfo({url: 'http://someurl.com'}); + const testData = {some: 'test'}; + const testString = JSON.stringify(testData); + const callback = jest.fn(); + + beforeEach(() => { + getServerAPI.mockImplementation((url, auth, success) => { + success(testString); + }); + }); + + afterEach(() => { + callback.mockClear(); + }); + + it('should return success callback when data is successfully parsed', async () => { + await serverInfo.getRemoteInfo(callback, '/some/url'); + expect(callback).toHaveBeenCalledWith(testData); + }); + + it('should reject when URL is missing', async () => { + await expect(serverInfo.getRemoteInfo(callback)).rejects.toThrow(); + expect(callback).not.toHaveBeenCalled(); + }); + + it('should reject when JSON does not parse', async () => { + getServerAPI.mockImplementation((url, auth, success) => { + success('T^&V*RT^&*%BDF*H^(YTNB*&F&^TB(FC'); + }); + await expect(serverInfo.getRemoteInfo(callback)).rejects.toThrow(); + expect(callback).not.toHaveBeenCalled(); + }); + + it('should reject when request fails', async () => { + getServerAPI.mockImplementation((url, auth, success, abort, fail) => { + fail(); + }); + await expect(serverInfo.getRemoteInfo(callback)).rejects.toThrow(); + expect(callback).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/main/server/serverInfo.ts b/src/main/server/serverInfo.ts index 3bfc5db5..191209fd 100644 --- a/src/main/server/serverInfo.ts +++ b/src/main/server/serverInfo.ts @@ -44,12 +44,17 @@ export class ServerInfo { return Promise.reject(new Error('Malformed URL')); } return new Promise((resolve, reject) => { - getServerAPI( + getServerAPI( url, false, - (data: T) => { - callback(data); - resolve(); + (raw: string) => { + try { + const data = JSON.parse(raw) as T; + callback(data); + resolve(); + } catch (e) { + reject(e); + } }, () => reject(new Error('Aborted')), (error: Error) => reject(error)); diff --git a/src/main/views/MattermostBrowserView.ts b/src/main/views/MattermostBrowserView.ts index 6d4ac57c..0c8adf97 100644 --- a/src/main/views/MattermostBrowserView.ts +++ b/src/main/views/MattermostBrowserView.ts @@ -24,6 +24,7 @@ import ServerManager from 'common/servers/serverManager'; import {RELOAD_INTERVAL, MAX_SERVER_RETRIES, SECOND, MAX_LOADING_SCREEN_SECONDS} from 'common/utils/constants'; import {isInternalURL, parseURL} from 'common/utils/url'; import type {MattermostView} from 'common/views/View'; +import {getServerAPI} from 'main/server/serverAPI'; import MainWindow from 'main/windows/mainWindow'; import WebContentsEventManager from './webContentEvents'; @@ -235,10 +236,10 @@ export class MattermostBrowserView extends EventEmitter { } }; - reload = () => { + reload = (loadURL?: URL | string) => { this.resetLoadingStatus(); AppState.updateExpired(this.id, false); - this.load(); + this.load(loadURL); }; getBounds = () => { @@ -452,10 +453,19 @@ export class MattermostBrowserView extends EventEmitter { if (!this.browserView || !this.browserView.webContents) { return; } - const loading = this.browserView.webContents.loadURL(loadURL, {userAgent: composeUserAgent()}); - loading.then(this.loadSuccess(loadURL)).catch(() => { - this.retryLoad = setTimeout(this.retryInBackground(loadURL), RELOAD_INTERVAL); - }); + const parsedURL = parseURL(loadURL); + if (!parsedURL) { + return; + } + getServerAPI( + parsedURL, + false, + () => this.reload(loadURL), + () => {}, + (error: Error) => { + this.log.debug(`Cannot reach server: ${error}`); + this.retryLoad = setTimeout(this.retryInBackground(loadURL), RELOAD_INTERVAL); + }); }; };