Improved the GitGraphView's message handling and logging (when sending messages from the back-end to the front-end).

Author: mhutchie

src/gitGraphView.ts

@@ -247,7 +247,8 @@ export class GitGraphView extends Disposable {
 			case 'compareCommits':
 				this.sendMessage({
 					command: 'compareCommits',
-					commitHash: msg.commitHash, compareWithHash: msg.compareWithHash,
+					commitHash: msg.commitHash,
+					compareWithHash: msg.compareWithHash,
 					...await this.dataSource.getCommitComparison(msg.repo, msg.fromHash, msg.toHash),
 					codeReview: msg.toHash !== UNCOMMITTED ? this.extensionState.getCodeReview(msg.repo, msg.fromHash + '-' + msg.toHash) : null,
 					refresh: msg.refresh
@@ -371,6 +372,15 @@ export class GitGraphView extends Disposable {
 					errors: errorInfos
 				});
 				break;
+			case 'endCodeReview':
+				this.extensionState.endCodeReview(msg.repo, msg.id);
+				break;
+			case 'exportRepoConfig':
+				this.sendMessage({
+					command: 'exportRepoConfig',
+					error: await this.repoManager.exportRepoConfig(msg.repo)
+				});
+				break;
 			case 'fetch':
 				this.sendMessage({
 					command: 'fetch',
@@ -386,15 +396,6 @@ export class GitGraphView extends Disposable {
 					error: await this.dataSource.fetchIntoLocalBranch(msg.repo, msg.remote, msg.remoteBranch, msg.localBranch, msg.force)
 				});
 				break;
-			case 'endCodeReview':
-				this.extensionState.endCodeReview(msg.repo, msg.id);
-				break;
-			case 'exportRepoConfig':
-				this.sendMessage({
-					command: 'exportRepoConfig',
-					error: await this.repoManager.exportRepoConfig(msg.repo)
-				});
-				break;
 			case 'loadCommits':
 				this.loadCommitsRefreshId = msg.refreshId;
 				this.sendMessage({
@@ -439,7 +440,8 @@ export class GitGraphView extends Disposable {
 				break;
 			case 'merge':
 				this.sendMessage({
-					command: 'merge', actionOn: msg.actionOn,
+					command: 'merge',
+					actionOn: msg.actionOn,
 					error: await this.dataSource.merge(msg.repo, msg.obj, msg.actionOn, msg.createNewCommit, msg.squash, msg.noCommit)
 				});
 				break;
@@ -512,7 +514,9 @@ export class GitGraphView extends Disposable {
 				break;
 			case 'rebase':
 				this.sendMessage({
-					command: 'rebase', actionOn: msg.actionOn, interactive: msg.interactive,
+					command: 'rebase',
+					actionOn: msg.actionOn,
+					interactive: msg.interactive,
 					error: await this.dataSource.rebase(msg.repo, msg.obj, msg.actionOn, msg.ignoreDate, msg.interactive)
 				});
 				break;
@@ -613,7 +617,20 @@ export class GitGraphView extends Disposable {
 	 * @param msg The message to be sent.
 	 */
 	private sendMessage(msg: ResponseMessage) {
-		this.panel.webview.postMessage(msg);
+		if (this.isDisposed()) {
+			this.logger.log('The Git Graph View has already been disposed, ignored sending "' + msg.command + '" message.');
+		} else {
+			this.panel.webview.postMessage(msg).then(
+				() => { },
+				() => {
+					if (this.isDisposed()) {
+						this.logger.log('The Git Graph View was disposed while sending "' + msg.command + '" message.');
+					} else {
+						this.logger.logError('Unable to send "' + msg.command + '" message to the Git Graph View.');
+					}
+				}
+			);
+		}
 	}
 
 	/**

src/utils/disposable.ts

@@ -2,12 +2,18 @@ import * as vscode from 'vscode';
 
 export class Disposable implements vscode.Disposable {
 	private disposables: vscode.Disposable[] = [];
+	private disposed: boolean = false;
 
 	/**
 	 * Disposes the resources used by the subclass.
 	 */
 	public dispose() {
-		this.disposables.forEach((disposable) => disposable.dispose());
+		this.disposed = true;
+		this.disposables.forEach((disposable) => {
+			try {
+				disposable.dispose();
+			} catch (_) { }
+		});
 		this.disposables = [];
 	}
 
@@ -24,6 +30,14 @@ export class Disposable implements vscode.Disposable {
 	protected registerDisposables(...disposables: vscode.Disposable[]) {
 		this.disposables.push(...disposables);
 	}
+
+	/**
+	 * Is the Disposable disposed.
+	 * @returns `TRUE` => Disposable has been disposed, `FALSE` => Disposable hasn't been disposed.
+	 */
+	protected isDisposed() {
+		return this.disposed;
+	}
 }
 
 export function toDisposable(fn: () => void): vscode.Disposable {

tests/disposable.test.ts

@@ -0,0 +1,102 @@
+import * as vscode from 'vscode';
+import { Disposable, toDisposable } from '../src/utils/disposable';
+
+class DisposableTest extends Disposable {
+	constructor() {
+		super();
+	}
+
+	public registerDisposable(disposable: vscode.Disposable) {
+		super.registerDisposable(disposable);
+	}
+
+	public registerDisposables(...disposables: vscode.Disposable[]) {
+		super.registerDisposables(...disposables);
+	}
+
+	public isDisposed() {
+		return super.isDisposed();
+	}
+}
+
+describe('Disposable', () => {
+	it('Should register a disposable', () => {
+		// Setup
+		const disposableTest = new DisposableTest();
+		const disposable = { dispose: jest.fn() };
+
+		// Run
+		disposableTest.registerDisposable(disposable);
+
+		// Assert
+		expect(disposableTest.isDisposed()).toBe(false);
+		expect(disposableTest['disposables']).toStrictEqual([disposable]);
+	});
+
+	it('Should register multiple disposables', () => {
+		// Setup
+		const disposableTest = new DisposableTest();
+		const disposable1 = { dispose: jest.fn() };
+		const disposable2 = { dispose: jest.fn() };
+
+		// Run
+		disposableTest.registerDisposables(disposable1, disposable2);
+
+		// Assert
+		expect(disposableTest.isDisposed()).toBe(false);
+		expect(disposableTest['disposables']).toStrictEqual([disposable1, disposable2]);
+	});
+
+	it('Should dispose all registered disposables', () => {
+		// Setup
+		const disposableTest = new DisposableTest();
+		const disposable1 = { dispose: jest.fn() };
+		const disposable2 = { dispose: jest.fn() };
+		disposableTest.registerDisposables(disposable1, disposable2);
+
+		// Run
+		disposableTest.dispose();
+
+		// Assert
+		expect(disposableTest.isDisposed()).toBe(true);
+		expect(disposableTest['disposables']).toStrictEqual([]);
+		expect(disposable1.dispose).toHaveBeenCalled();
+		expect(disposable2.dispose).toHaveBeenCalled();
+	});
+
+	it('Should dispose all registered disposables independently, catching any exceptions', () => {
+		// Setup
+		const disposableTest = new DisposableTest();
+		const disposable1 = { dispose: jest.fn() };
+		const disposable2 = {
+			dispose: jest.fn(() => {
+				throw new Error();
+			})
+		};
+		const disposable3 = { dispose: jest.fn() };
+		disposableTest.registerDisposables(disposable1, disposable2, disposable3);
+
+		// Run
+		disposableTest.dispose();
+
+		// Assert
+		expect(disposableTest.isDisposed()).toBe(true);
+		expect(disposableTest['disposables']).toStrictEqual([]);
+		expect(disposable1.dispose).toHaveBeenCalled();
+		expect(disposable2.dispose).toHaveBeenCalled();
+		expect(disposable3.dispose).toHaveBeenCalled();
+	});
+});
+
+describe('toDisposable', () => {
+	it('Should wrap a function with a disposable', () => {
+		// Setup
+		const fn = () => { };
+
+		// Run
+		const result = toDisposable(fn);
+
+		// Assert
+		expect(result).toStrictEqual({ dispose: fn });
+	});
+});

tests/gitGraphView.test.ts

@@ -33,6 +33,7 @@ describe('GitGraphView', () => {
 	let repoManager: RepoManager;
 
 	let spyOnLog: jest.SpyInstance;
+	let spyOnLogError: jest.SpyInstance;
 	let spyOnGetRepos: jest.SpyInstance;
 	let spyOnIsGitExecutableUnknown: jest.SpyInstance;
 
@@ -49,6 +50,7 @@ describe('GitGraphView', () => {
 		repoManager = new RepoManager(dataSource, extensionState, onDidChangeConfiguration.subscribe, logger);
 
 		spyOnLog = jest.spyOn(logger, 'log');
+		spyOnLogError = jest.spyOn(logger, 'logError');
 		spyOnGetRepos = jest.spyOn(repoManager, 'getRepos');
 		spyOnIsGitExecutableUnknown = jest.spyOn(dataSource, 'isGitExecutableUnknown');
 
@@ -3467,6 +3469,103 @@ describe('GitGraphView', () => {
 		});
 	});
 
+	describe('sendMessage', () => {
+		beforeEach(() => {
+			GitGraphView.createOrShow('/path/to/extension', dataSource, extensionState, avatarManager, repoManager, logger, null);
+			spyOnLog.mockReset();
+			spyOnLogError.mockReset();
+		});
+
+		it('Should send a message to the Webview', async () => {
+			// Setup
+			const mockedWebviewPanel = vscode.getMockedWebviewPanel(0);
+			jest.spyOn(utils, 'viewScm').mockResolvedValueOnce(null);
+			jest.spyOn(mockedWebviewPanel.panel.webview, 'postMessage').mockResolvedValueOnce(true);
+
+			// Run
+			mockedWebviewPanel.mocks.panel.webview.onDidReceiveMessage({
+				command: 'viewScm'
+			});
+
+			// Assert
+			await waitForExpect(() => {
+				expect(mockedWebviewPanel.panel.webview.postMessage).toHaveBeenCalledWith({
+					command: 'viewScm',
+					error: null
+				});
+				expect(spyOnLog).not.toHaveBeenCalled();
+				expect(spyOnLogError).not.toHaveBeenCalled();
+			});
+		});
+
+		it('Should log an error message when Webview.postMessage rejects, and the GitGraphView hasn\'t been disposed', async () => {
+			// Setup
+			const mockedWebviewPanel = vscode.getMockedWebviewPanel(0);
+			jest.spyOn(utils, 'viewScm').mockResolvedValueOnce(null);
+			jest.spyOn(mockedWebviewPanel.panel.webview, 'postMessage').mockRejectedValueOnce(null);
+
+			// Run
+			mockedWebviewPanel.mocks.panel.webview.onDidReceiveMessage({
+				command: 'viewScm'
+			});
+
+			// Assert
+			await waitForExpect(() => {
+				expect(mockedWebviewPanel.panel.webview.postMessage).toHaveBeenCalledWith({
+					command: 'viewScm',
+					error: null
+				});
+				expect(spyOnLog).not.toHaveBeenCalled();
+				expect(spyOnLogError).toHaveBeenCalledWith('Unable to send "viewScm" message to the Git Graph View.');
+			});
+		});
+
+		it('Should log an information message when Webview.postMessage rejects, and the GitGraphView has been disposed', async () => {
+			// Setup
+			const mockedWebviewPanel = vscode.getMockedWebviewPanel(0);
+			jest.spyOn(utils, 'viewScm').mockResolvedValueOnce(null);
+			jest.spyOn(mockedWebviewPanel.panel.webview, 'postMessage').mockImplementationOnce(() => {
+				GitGraphView.currentPanel!.dispose();
+				return Promise.reject();
+			});
+
+			// Run
+			mockedWebviewPanel.mocks.panel.webview.onDidReceiveMessage({
+				command: 'viewScm'
+			});
+
+			// Assert
+			await waitForExpect(() => {
+				expect(mockedWebviewPanel.panel.webview.postMessage).toHaveBeenCalledWith({
+					command: 'viewScm',
+					error: null
+				});
+				expect(spyOnLog).toHaveBeenCalledWith('The Git Graph View was disposed while sending "viewScm" message.');
+				expect(spyOnLogError).not.toHaveBeenCalled();
+			});
+		});
+
+		it('Shouldn\'t send a message to the Webview if it has been disposed', async () => {
+			// Setup
+			const mockedWebviewPanel = vscode.getMockedWebviewPanel(0);
+			jest.spyOn(utils, 'viewScm').mockResolvedValueOnce(null);
+			jest.spyOn(mockedWebviewPanel.panel.webview, 'postMessage').mockResolvedValueOnce(true);
+
+			// Run
+			GitGraphView.currentPanel!.dispose();
+			mockedWebviewPanel.mocks.panel.webview.onDidReceiveMessage({
+				command: 'viewScm'
+			});
+
+			// Assert
+			await waitForExpect(() => {
+				expect(mockedWebviewPanel.panel.webview.postMessage).not.toHaveBeenCalled();
+				expect(spyOnLog).toHaveBeenCalledWith('The Git Graph View has already been disposed, ignored sending "viewScm" message.');
+				expect(spyOnLogError).not.toHaveBeenCalled();
+			});
+		});
+	});
+
 	describe('getHtmlForWebview', () => {
 		beforeEach(() => {
 			jest.spyOn(utils, 'getNonce').mockReturnValueOnce('1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d');