diff --git a/src/test/webviews/testHost.ts b/src/test/webviews/testHost.ts new file mode 100644 index 0000000000..2d0648aa79 --- /dev/null +++ b/src/test/webviews/testHost.ts @@ -0,0 +1,25 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { afterEach } from 'mocha'; +import { createWebviewHost, WebviewHost, WebviewTransport } from '../../../webviews/common/host'; + +const hosts = new Set(); + +export function createTestHost(state?: unknown, transport?: WebviewTransport): WebviewHost { + const host = createWebviewHost(transport); + if (state !== undefined) { + host.setState(state); + } + hosts.add(host); + return host; +} + +afterEach(() => { + for (const host of hosts) { + host.dispose(); + } + hosts.clear(); +}); diff --git a/webviews/activityBarView/app.tsx b/webviews/activityBarView/app.tsx index 9d40442273..1d9a3827bb 100644 --- a/webviews/activityBarView/app.tsx +++ b/webviews/activityBarView/app.tsx @@ -7,10 +7,19 @@ import React, { useContext, useEffect, useState } from 'react'; import { render } from 'react-dom'; import { Overview } from './overview'; import { PullRequest } from '../../src/github/views'; -import PullRequestContext from '../common/context'; +import PullRequestContext, { PRContext } from '../common/context'; +import { createWebviewHost } from '../common/host'; +import { rethrowUnlessDisposed } from '../common/message'; export function main() { - render({pr => }, document.getElementById('app')); + const host = createWebviewHost(); + const context = new PRContext(host); + render( + + {pr => } + , + document.getElementById('app') + ); } export function Root({ children }) { @@ -19,8 +28,11 @@ export function Root({ children }) { useEffect(() => { ctx.onchange = setPR; setPR(ctx.pr); + return () => { + ctx.onchange = null; + }; }, []); - ctx.postMessage({ command: 'ready' }); - ctx.postMessage({ command: 'pr.debug', args: 'initialized ' + (pr ? 'with PR' : 'without PR') }); + ctx.postMessage({ command: 'ready' }).catch(rethrowUnlessDisposed); + ctx.postMessage({ command: 'pr.debug', args: 'initialized ' + (pr ? 'with PR' : 'without PR') }).catch(rethrowUnlessDisposed); return pr ? children(pr) :
Loading...
; } diff --git a/webviews/common/cache.ts b/webviews/common/cache.ts deleted file mode 100644 index 1950c0e266..0000000000 --- a/webviews/common/cache.ts +++ /dev/null @@ -1,29 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import { vscode } from './message'; -import { PullRequest } from '../../src/github/views'; - -export function getState(): PullRequest { - return vscode.getState(); -} - -export function setState(pullRequest: PullRequest | undefined): void { - const oldPullRequest = getState(); - - if (oldPullRequest && oldPullRequest.number && oldPullRequest.number === pullRequest?.number) { - pullRequest.pendingCommentText = oldPullRequest.pendingCommentText; - pullRequest.pendingReviewSummaryText = oldPullRequest.pendingReviewSummaryText; - } - - if (pullRequest) { - vscode.setState(pullRequest); - } -} - -export function updateState(data: Partial | undefined): void { - const pullRequest = vscode.getState(); - vscode.setState(Object.assign(pullRequest, data)); -} diff --git a/webviews/common/context.tsx b/webviews/common/context.tsx index dd145c2305..41e35701da 100644 --- a/webviews/common/context.tsx +++ b/webviews/common/context.tsx @@ -4,9 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import { createContext } from 'react'; -import { getState, setState, updateState } from './cache'; import { COMMENT_TEXTAREA_ID } from './constants'; -import { getMessageHandler, MessageHandler } from './message'; +import { WebviewHost } from './host'; import { CloseResult, DescriptionResult, OpenCommitChangesArgs, OpenLocalFileArgs } from '../../common/views'; import { IComment } from '../../src/common/comment'; import { EventType, ReviewEvent, SessionLinkInfo, TimelineEvent } from '../../src/common/timelineEvent'; @@ -32,17 +31,21 @@ function bytesToBase64(bytes: Uint8Array): string { const MAX_UPLOAD_SIZE_BYTES = 25 * 1024 * 1024; export class PRContext { + public pr: PullRequest | undefined; + public onchange: ((ctx: PullRequest | undefined) => void) | null = null; public preview: OverviewItemPreview | undefined; public onPreviewChange: ((preview: OverviewItemPreview | undefined) => void) | null = null; + private readonly _unsubscribe: () => void; - constructor( - public pr: PullRequest | undefined = getState(), - public onchange: ((ctx: PullRequest | undefined) => void) | null = null, - private _handler: MessageHandler | null = null, - ) { - if (!_handler) { - this._handler = getMessageHandler(this.handleMessage); - } + constructor(private readonly _host: WebviewHost) { + this.pr = _host.getState(); + this._unsubscribe = _host.onCommand(this.handleMessage); + } + + public dispose(): void { + this._unsubscribe(); + this.onchange = null; + this.onPreviewChange = null; } public setTitle = async (title: string) => { @@ -194,7 +197,7 @@ export class PRContext { this.postMessage({ command: 'pr.cancel-generate-description' }); public updateDraft = (id: number, body: string) => { - const pullRequest = getState(); + const pullRequest = this._host.getState() ?? this.pr; const pendingCommentDrafts = pullRequest.pendingCommentDrafts || Object.create(null); if (body === pendingCommentDrafts[id]) { return; @@ -550,7 +553,14 @@ export class PRContext { this.preview = undefined; this.onPreviewChange?.(undefined); this.pr = pr; - setState(this.pr); + const oldPullRequest = this._host.getState(); + if (oldPullRequest?.number && oldPullRequest.number === pr?.number) { + pr.pendingCommentText = oldPullRequest.pendingCommentText; + pr.pendingReviewSummaryText = oldPullRequest.pendingReviewSummaryText; + } + if (pr) { + this._host.setState(pr); + } if (this.onchange) { this.onchange(this.pr); } @@ -558,7 +568,8 @@ export class PRContext { }; updatePR = (pr: Partial | undefined) => { - updateState(pr); + const current = this._host.getState() ?? this.pr; + this._host.setState(Object.assign(current ?? {}, pr)); this.pr = this.pr ? { ...this.pr, ...pr } : pr as PullRequest; if (this.onchange) { this.onchange(this.pr); @@ -566,8 +577,8 @@ export class PRContext { return this; }; - postMessage(message: any) { - return (this._handler?.postMessage(message) ?? Promise.resolve(undefined)); + async postMessage(message: any): Promise { + return this._host.postMessage(message); } handleMessage = (message: any) => { @@ -627,8 +638,8 @@ export class PRContext { } }; - public static instance = new PRContext(); } -const PullRequestContext = createContext(PRContext.instance); +// Every render entry point must supply a provider. +const PullRequestContext = createContext(undefined!); export default PullRequestContext; diff --git a/webviews/common/createContextNew.ts b/webviews/common/createContextNew.ts index 7dfb26a9a0..f339ba7ef2 100644 --- a/webviews/common/createContextNew.ts +++ b/webviews/common/createContextNew.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { createContext } from 'react'; -import { getMessageHandler, MessageHandler, vscode } from './message'; +import { WebviewHost } from './host'; import { RemoteInfo } from '../../common/types'; import { CancelCreatePullRequestNew, ChooseBaseRemoteAndBranchResult, ChooseCompareRemoteAndBranchResult, ChooseRemoteAndBranchArgs, CreateParamsNew, CreatePullRequestNew, ScrollPosition, StackCandidate, TitleAndDescriptionArgs, TitleAndDescriptionResult } from '../../common/views'; import { compareIgnoreCase } from '../../src/common/utils'; @@ -39,6 +39,10 @@ const defaultCreateParams: CreateParamsNew = { usingTemplate: false }; +function createDefaultParams(): CreateParamsNew { + return { ...defaultCreateParams, labels: [], assignees: [], reviewers: [] }; +} + function remoteChanged(next: RemoteInfo | undefined, current: RemoteInfo | undefined): boolean { return next !== undefined && (next.owner !== current?.owner || next.repositoryName !== current?.repositoryName); } @@ -53,17 +57,19 @@ function stackCandidateChanged(next: Partial, current: StackCan export class CreatePRContextNew { public createParams: CreateParamsNew; + public onchange: ((ctx: CreateParamsNew) => void) | null = null; private _titleStack: string[] = []; private _descriptionStack: string[] = []; + private readonly _unsubscribe: () => void; - constructor( - public onchange: ((ctx: CreateParamsNew) => void) | null = null, - private _handler: MessageHandler | null = null, - ) { - this.createParams = vscode.getState() ?? defaultCreateParams; - if (!_handler) { - this._handler = getMessageHandler(this.handleMessage); - } + constructor(private readonly _host: WebviewHost) { + this.createParams = _host.getState() ?? createDefaultParams(); + this._unsubscribe = _host.onCommand(this.handleMessage); + } + + public dispose(): void { + this._unsubscribe(); + this.onchange = null; } get isCreatable(): boolean { @@ -116,7 +122,7 @@ export class CreatePRContextNew { // dialog, or the message did not get a response) preserve the user's // in-progress title/description. if (result?.cancelled === true) { - vscode.setState(defaultCreateParams); + this._host.setState(createDefaultParams()); } }; @@ -133,8 +139,8 @@ export class CreatePRContextNew { } public updateState = (params: Partial, reset: boolean = false): void => { - this.createParams = reset ? { ...defaultCreateParams, ...params } : { ...this.createParams, ...params }; - vscode.setState(this.createParams); + this.createParams = reset ? { ...createDefaultParams(), ...params } : { ...this.createParams, ...params }; + this._host.setState(this.createParams); if (this.onchange) { this.onchange(this.createParams); } @@ -324,14 +330,14 @@ export class CreatePRContextNew { // awaiting would discard the user's title/description if the create fails // (for example when the branch push fails and needs `--force`), causing // the description to fall back to the template on the next webview load. - vscode.setState(defaultCreateParams); + this._host.setState(createDefaultParams()); } catch (e) { this.updateState({ createError: (typeof e === 'string') ? e : (e.message ? e.message : 'An unknown error occurred.') }); } }; postMessage = async (message: any): Promise => { - return this._handler?.postMessage(message); + return this._host.postMessage(message); }; handleMessage = async (message: { command: string, params?: Partial, scrollPosition?: ScrollPosition }): Promise => { @@ -396,7 +402,7 @@ export class CreatePRContextNew { case 'reset': if (!message.params) { - this.updateState(defaultCreateParams, true); + this.updateState(createDefaultParams(), true); return; } message.params.creating = message.params.creating ?? false; @@ -453,8 +459,8 @@ export class CreatePRContextNew { } }; - public static instance = new CreatePRContextNew(); } -const PullRequestContextNew = createContext(CreatePRContextNew.instance); +// Every render entry point must supply a provider. +const PullRequestContextNew = createContext(undefined!); export default PullRequestContextNew; diff --git a/webviews/common/host.ts b/webviews/common/host.ts new file mode 100644 index 0000000000..65a0ab35b4 --- /dev/null +++ b/webviews/common/host.ts @@ -0,0 +1,61 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { MessageHandler } from './message'; + +export interface WebviewHost extends WebviewState { + postMessage(message: CommandMessage): Promise; + onCommand(listener: (command: unknown) => void): () => void; + dispose(): void; +} + +export function createWebviewHost(transport: WebviewTransport = vscodeTransport): WebviewHost { + return new MessageHandler(transport); +} + +export interface CommandMessage { + command: string; + args?: unknown; +} + +export interface RequestMessage extends CommandMessage { + req: string; +} + +export interface ReplyMessage { + seq?: string; + err?: string; + res?: unknown; +} + +export interface WebviewState { + getState(): T | undefined; + setState(state: T): void; +} + +export interface WebviewTransport extends WebviewState { + postMessage(message: RequestMessage): void; + onMessage(listener: (message: ReplyMessage) => void): () => void; +} + +declare function acquireVsCodeApi(): Omit; + +let api: Omit | undefined; + +function getApi(): Omit { + api ??= acquireVsCodeApi(); + return api; +} + +export const vscodeTransport: WebviewTransport = { + getState: () => getApi().getState(), + setState: (state: T) => getApi().setState(state), + postMessage: message => getApi().postMessage(message), + onMessage: listener => { + const handler = (event: MessageEvent) => listener(event.data); + window.addEventListener('message', handler); + return () => window.removeEventListener('message', handler); + }, +}; diff --git a/webviews/common/message.ts b/webviews/common/message.ts index 2833174da2..96cd511007 100644 --- a/webviews/common/message.ts +++ b/webviews/common/message.ts @@ -3,46 +3,57 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -interface IRequestMessage { - req: string; - command: string; - args: T; -} +import type { CommandMessage, ReplyMessage, WebviewHost, WebviewTransport } from './host'; + +let lastSentReq = 0; -interface IReplyMessage { - seq: string; - err: string; - // eslint-disable-next-line rulesdir/no-any-except-union-method-signature - res: any; +export class MessageHandlerDisposedError extends Error { + constructor() { + super('The webview message handler has been disposed.'); + } } -// eslint-disable-next-line rulesdir/no-any-except-union-method-signature -declare let acquireVsCodeApi: any; -export const vscode = acquireVsCodeApi(); +export function rethrowUnlessDisposed(error: unknown): void { + if (!(error instanceof MessageHandlerDisposedError)) { + throw error; + } +} -export class MessageHandler { - private _commandHandler: ((message: any) => void) | null; - private lastSentReq: number; +export class MessageHandler implements WebviewHost { + private readonly _commandListeners = new Set<(command: unknown) => void>(); private readonly _pendingReplies = new Map void; reject: (reason: unknown) => void }>(); - constructor(commandHandler: any) { - this._commandHandler = commandHandler; - this.lastSentReq = 0; - window.addEventListener('message', this.handleMessage.bind(this) as (this: Window, ev: MessageEvent) => any); + private readonly _unsubscribe: () => void; + private _disposed = false; + + constructor(private readonly _transport: WebviewTransport) { + this._unsubscribe = _transport.onMessage(this._handleMessage); + } + + public getState(): T | undefined { + return this._transport.getState(); + } + + public setState(state: T): void { + this._transport.setState(state); } - public registerCommandHandler(commandHandler: (message: any) => void) { - this._commandHandler = commandHandler; + public onCommand(listener: (command: unknown) => void): () => void { + if (this._disposed) { + throw new MessageHandlerDisposedError(); + } + this._commandListeners.add(listener); + return () => this._commandListeners.delete(listener); } - public async postMessage(message: any): Promise { - const req = String(++this.lastSentReq); - return new Promise((resolve, reject) => { + public async postMessage(message: CommandMessage): Promise { + if (this._disposed) { + throw new MessageHandlerDisposedError(); + } + const req = String(++lastSentReq); + return new Promise((resolve, reject) => { this._pendingReplies.set(req, { resolve, reject }); try { - message = Object.assign(message, { - req: req, - }); - vscode.postMessage(message as IRequestMessage); + this._transport.postMessage({ ...message, req }); } catch (error) { this._pendingReplies.delete(req); reject(error); @@ -50,11 +61,21 @@ export class MessageHandler { }); } - // handle message should resolve promises - private handleMessage(event: any) { - const message: IReplyMessage = event.data; // The json data that the extension sent + public dispose(): void { + if (this._disposed) { + return; + } + this._disposed = true; + this._unsubscribe(); + this._commandListeners.clear(); + for (const reply of this._pendingReplies.values()) { + reply.reject(new MessageHandlerDisposedError()); + } + this._pendingReplies.clear(); + } + + private _handleMessage = (message: ReplyMessage) => { if (message.seq) { - // this is a reply const pendingReply = this._pendingReplies.get(message.seq); if (pendingReply) { this._pendingReplies.delete(message.seq); @@ -67,12 +88,8 @@ export class MessageHandler { return; } - if (this._commandHandler) { - this._commandHandler(message.res); + for (const listener of this._commandListeners) { + listener(message.res); } - } -} - -export function getMessageHandler(handler: ((message: any) => void) | null) { - return new MessageHandler(handler); + }; } diff --git a/webviews/common/test/host.test.ts b/webviews/common/test/host.test.ts new file mode 100644 index 0000000000..8cb4ee4804 --- /dev/null +++ b/webviews/common/test/host.test.ts @@ -0,0 +1,359 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import * as React from 'react'; +import { unmountComponentAtNode } from 'react-dom'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { render, wait } from 'react-testing-library'; +import { createSandbox } from 'sinon'; +import { CreateParamsNew } from '../../../common/views'; +import { PullRequest } from '../../../src/github/views'; +import { createTestHost } from '../../../src/test/webviews/testHost'; +import { PullRequestBuilder } from '../../editorWebview/test/builder/pullRequest'; +import { ReplyMessage, RequestMessage, vscodeTransport, WebviewTransport } from '../host'; + +function importContextsWithoutVsCodeApi() { + const sandbox = createSandbox(); + sandbox.stub(globalThis as typeof globalThis & { acquireVsCodeApi(): unknown }, 'acquireVsCodeApi') + .throws(new Error('Importing a context must not acquire the VS Code API')); + const listeners = sandbox.spy(window, 'addEventListener'); + try { + const pr = require('../context') as typeof import('../context'); + const create = require('../createContextNew') as typeof import('../createContextNew'); + const message = require('../message') as typeof import('../message'); + assert.strictEqual(listeners.callCount, 0); + return { pr, create, message }; + } finally { + sandbox.restore(); + } +} + +const imported = importContextsWithoutVsCodeApi(); +const { PRContext, default: PullRequestContext } = imported.pr; +const { CreatePRContextNew, default: PullRequestContextNew } = imported.create; +const { MessageHandlerDisposedError, rethrowUnlessDisposed } = imported.message; + +class TestTransport implements WebviewTransport { + public state: unknown; + public readonly messages: RequestMessage[] = []; + public readonly listeners = new Set<(message: ReplyMessage) => void>(); + + getState(): T | undefined { + return this.state as T | undefined; + } + + setState(state: T): void { + this.state = state; + } + + postMessage(message: RequestMessage): void { + this.messages.push(message); + } + + onMessage(listener: (message: ReplyMessage) => void): () => void { + this.listeners.add(listener); + return () => this.listeners.delete(listener); + } + + reply(index: number, res: unknown): void { + this.emit({ seq: this.messages[index].req, res }); + } + + emit(message: ReplyMessage): void { + for (const listener of this.listeners) { + listener(message); + } + } +} + +describe('Webview host isolation', () => { + it('imports messaging and both contexts without acquiring the VS Code API', () => { + assert(imported.pr.PRContext); + assert(imported.create.CreatePRContextNew); + assert(imported.message.MessageHandler); + }); + + it('returns the exact supplied contexts, including nested providers, without a production host', () => { + const sandbox = createSandbox(); + sandbox.stub(globalThis as typeof globalThis & { acquireVsCodeApi(): unknown }, 'acquireVsCodeApi') + .throws(new Error('Unexpected VS Code API access')); + const first = new PRContext(createTestHost(new PullRequestBuilder().build(), new TestTransport())); + const second = new PRContext(createTestHost(new PullRequestBuilder().build(), new TestTransport())); + const create = new CreatePRContextNew(createTestHost(undefined, new TestTransport())); + const observed: InstanceType[] = []; + const PRConsumer = () => { + observed.push(React.useContext(PullRequestContext)); + return null; + }; + const CreateConsumer = () => { + assert.strictEqual(React.useContext(PullRequestContextNew), create); + return null; + }; + try { + renderToStaticMarkup(React.createElement(PullRequestContext.Provider, { value: first }, + React.createElement(PRConsumer), + React.createElement(PullRequestContext.Provider, { value: second }, React.createElement(PRConsumer)), + React.createElement(PRConsumer), + React.createElement(PullRequestContextNew.Provider, { value: create }, React.createElement(CreateConsumer)), + )); + assert.strictEqual(observed.length, 3); + assert.strictEqual(observed[0], first); + assert.strictEqual(observed[1], second); + assert.strictEqual(observed[2], first); + } finally { + first.dispose(); + second.dispose(); + create.dispose(); + sandbox.restore(); + } + }); + + it('creates independent contexts in all production entry points', () => { + const { main: editor } = require('../../editorWebview/app') as typeof import('../../editorWebview/app'); + const { main: activity } = require('../../activityBarView/app') as typeof import('../../activityBarView/app'); + const { main: create } = require('../../createPullRequestViewNew/app') as typeof import('../../createPullRequestViewNew/app'); + const sandbox = createSandbox(); + sandbox.stub(globalThis as typeof globalThis & { acquireVsCodeApi(): unknown }, 'acquireVsCodeApi') + .throws(new Error('Unexpected VS Code API access')); + const app = document.createElement('div'); + app.id = 'app'; + document.body.appendChild(app); + try { + for (const main of [editor, activity, create]) { + const host = new TestTransport(); + const state = sandbox.stub(vscodeTransport, 'getState').returns(undefined); + const persist = sandbox.stub(vscodeTransport, 'setState').callsFake(value => host.setState(value)); + const send = sandbox.stub(vscodeTransport, 'postMessage').callsFake(message => { + host.postMessage(message); + host.reply(host.messages.length - 1, undefined); + }); + const listen = sandbox.stub(vscodeTransport, 'onMessage').callsFake(listener => host.onMessage(listener)); + try { + for (let mount = 1; mount <= 2; mount++) { + main(); + assert(app.childElementCount > 0); + assert.strictEqual(listen.callCount, mount); + assert.strictEqual(host.messages.filter(message => message.command === 'ready').length, mount); + unmountComponentAtNode(app); + assert.strictEqual(host.listeners.size, mount); + } + } finally { + unmountComponentAtNode(app); + host.listeners.clear(); + state.restore(); + persist.restore(); + send.restore(); + listen.restore(); + } + } + } finally { + app.remove(); + sandbox.restore(); + } + }); + + it('provides contexts without acquiring a VS Code API or installing global listeners', () => { + const sandbox = createSandbox(); + const acquire = sandbox.stub(globalThis as typeof globalThis & { acquireVsCodeApi(): unknown }, 'acquireVsCodeApi') + .throws(new Error('A fixture must not acquire the VS Code API')); + const prTransport = new TestTransport(); + const createTransport = new TestTransport(); + const prHost = createTestHost(new PullRequestBuilder().build(), prTransport); + const createHost = createTestHost(undefined, createTransport); + let prContext: InstanceType | undefined; + let createContext: InstanceType | undefined; + try { + prContext = new PRContext(prHost); + createContext = new CreatePRContextNew(createHost); + assert.strictEqual(acquire.callCount, 0); + assert.strictEqual(prTransport.listeners.size, 1); + assert.strictEqual(createTransport.listeners.size, 1); + assert(PullRequestContext); + assert(PullRequestContextNew); + } finally { + prContext?.dispose(); + createContext?.dispose(); + sandbox.restore(); + } + assert.strictEqual(prTransport.listeners.size, 1); + assert.strictEqual(createTransport.listeners.size, 1); + prHost.dispose(); + createHost.dispose(); + assert.strictEqual(prTransport.listeners.size, 0); + assert.strictEqual(createTransport.listeners.size, 0); + }); + + it('routes simultaneous replies per handler, without mutating requests or treating replies as commands', async () => { + const host = new TestTransport(); + const commands: unknown[] = []; + const first = createTestHost(undefined, host); + const second = createTestHost(undefined, host); + first.onCommand(message => commands.push(message)); + second.onCommand(message => commands.push(message)); + const message = { command: 'pr.get-stack' }; + const firstRequest = first.postMessage(message); + const secondRequest = second.postMessage(message); + assert.deepStrictEqual(message, { command: 'pr.get-stack' }); + assert.notStrictEqual(host.messages[0].req, host.messages[1].req); + host.reply(1, 'second'); + host.reply(0, 'first'); + assert.strictEqual(await firstRequest, 'first'); + assert.strictEqual(await secondRequest, 'second'); + host.reply(0, 'duplicate'); + assert.deepStrictEqual(commands, []); + first.dispose(); + second.dispose(); + }); + + it('unsubscribes contexts without disposing their shared host or cancelling its requests', async () => { + const transport = new TestTransport(); + const pr = new PullRequestBuilder().build(); + const host = createTestHost(pr, transport); + const first = new PRContext(host); + const second = new PRContext(host); + const pending = first.postMessage({ command: 'pr.get-stack' }); + first.dispose(); + first.dispose(); + assert.strictEqual(transport.listeners.size, 1); + transport.reply(0, 'reply after context disposal'); + assert.strictEqual(await pending, 'reply after context disposal'); + transport.emit({ res: { command: 'pr.initialize', pullrequest: { ...pr, title: 'Still subscribed' } } }); + assert.strictEqual(first.pr?.title, pr.title); + assert.strictEqual(second.pr?.title, 'Still subscribed'); + second.dispose(); + assert.strictEqual(transport.listeners.size, 1); + host.dispose(); + assert.strictEqual(transport.listeners.size, 0); + }); + + it('rejects errors and outstanding requests and removes listeners on idempotent disposal', async () => { + const host = new TestTransport(); + const handler = createTestHost(undefined, host); + const rejected = handler.postMessage({ command: 'pr.update-stack' }); + const assertion = assert.rejects(rejected, reason => reason === 'Update failed'); + host.emit({ seq: host.messages[0].req, err: 'Update failed' }); + await assertion; + const pending = handler.postMessage({ command: 'pr.get-stack' }); + const pendingAssertion = assert.rejects(pending, MessageHandlerDisposedError); + handler.dispose(); + handler.dispose(); + await pendingAssertion; + assert.strictEqual(host.listeners.size, 0); + await assert.rejects(handler.postMessage({ command: 'pr.get-stack' }), MessageHandlerDisposedError); + assert.strictEqual(host.messages.length, 2); + }); + + it('propagates synchronous host failures rather than leaving requests pending', async () => { + const host = new TestTransport(); + const failure = new Error('Unexpected command'); + host.postMessage = () => { throw failure; }; + const handler = createTestHost(undefined, host); + await assert.rejects(handler.postMessage({ command: 'unexpected' }), error => error === failure); + handler.dispose(); + }); + + it('ignores only disposal errors for lifecycle notifications', async () => { + const host = new TestTransport(); + const handler = createTestHost(undefined, host); + const ready = handler.postMessage({ command: 'ready' }).catch(rethrowUnlessDisposed); + handler.dispose(); + await ready; + const failure = new Error('Unexpected host error'); + await assert.rejects(Promise.reject(failure).catch(rethrowUnlessDisposed), error => error === failure); + }); + + it('persists PR changes and incoming commands only in the owning context', () => { + const firstHost = new TestTransport(); + const secondHost = new TestTransport(); + const pr = new PullRequestBuilder().build(); + const first = new PRContext(createTestHost({ ...pr }, firstHost)); + const second = new PRContext(createTestHost({ ...pr }, secondHost)); + first.updatePR({ title: 'First title' }); + secondHost.emit({ res: { command: 'pr.update', pullrequest: { title: 'Second title' } } }); + assert.strictEqual(first.pr?.title, 'First title'); + assert.strictEqual(firstHost.getState()?.title, 'First title'); + assert.strictEqual(second.pr?.title, 'Second title'); + assert.strictEqual(secondHost.getState()?.title, 'Second title'); + firstHost.setState({ ...pr, pendingCommentText: 'Draft', pendingReviewSummaryText: 'Review draft' }); + first.setPR({ ...pr }); + assert.strictEqual(first.pr?.pendingCommentText, 'Draft'); + assert.strictEqual(first.pr?.pendingReviewSummaryText, 'Review draft'); + first.dispose(); + firstHost.emit({ res: { command: 'pr.update', pullrequest: { title: 'Late title' } } }); + assert.strictEqual(first.pr?.title, pr.title); + second.dispose(); + }); + + it('restores independent create state and keeps default arrays local to each instance', () => { + const firstHost = new TestTransport(); + const secondHost = new TestTransport(); + const first = new CreatePRContextNew(createTestHost(undefined, firstHost)); + const second = new CreatePRContextNew(createTestHost(undefined, secondHost)); + assert.notStrictEqual(first.createParams.labels, second.createParams.labels); + assert.notStrictEqual(first.createParams.assignees, second.createParams.assignees); + assert.notStrictEqual(first.createParams.reviewers, second.createParams.reviewers); + first.updateState({ pendingTitle: 'First draft' }); + second.updateState({ pendingTitle: 'Second draft' }); + const restored = new CreatePRContextNew(createTestHost(undefined, firstHost)); + assert.strictEqual(restored.createParams.pendingTitle, 'First draft'); + assert.strictEqual(secondHost.getState()?.pendingTitle, 'Second draft'); + first.dispose(); + second.dispose(); + restored.dispose(); + }); + + it('preserves create persistence on failure and clears only the owning state on success', async () => { + const firstHost = new TestTransport(); + const secondHost = new TestTransport(); + const first = new CreatePRContextNew(createTestHost(undefined, firstHost)); + const second = new CreatePRContextNew(createTestHost(undefined, secondHost)); + first.updateState({ + pendingTitle: 'First draft', + baseRemote: { owner: 'owner', repositoryName: 'repo' }, + baseBranch: 'main', + compareRemote: { owner: 'owner', repositoryName: 'repo' }, + compareBranch: 'feature' + }); + second.updateState({ pendingTitle: 'Second draft' }); + const failed = first.submit(); + firstHost.emit({ seq: firstHost.messages[0].req, err: 'Create failed' }); + await failed; + assert.strictEqual(firstHost.getState()?.pendingTitle, 'First draft'); + assert.strictEqual(first.createParams.createError, 'Create failed'); + const succeeded = first.submit(); + firstHost.reply(1, undefined); + await succeeded; + assert.strictEqual(firstHost.getState()?.pendingTitle, undefined); + assert.strictEqual(secondHost.getState()?.pendingTitle, 'Second draft'); + first.dispose(); + second.dispose(); + }); + + it('cleans up Root callbacks without disposing the supplied context', async () => { + const { Root } = require('../../editorWebview/app') as typeof import('../../editorWebview/app'); + const transport = new TestTransport(); + const host = createTestHost(new PullRequestBuilder().build(), transport); + const context = new PRContext(host); + const root = render(React.createElement(PullRequestContext.Provider, { value: context }, + React.createElement(Root, { children: () => React.createElement('div') }))); + try { + await wait(() => assert(context.onchange)); + root.unmount(); + await wait(() => { + assert.strictEqual(context.onchange, null); + assert.strictEqual(context.onPreviewChange, null); + assert.strictEqual(transport.listeners.size, 1); + }); + context.dispose(); + assert.strictEqual(transport.listeners.size, 1); + host.dispose(); + assert.strictEqual(transport.listeners.size, 0); + } finally { + root.unmount(); + context.dispose(); + } + }); +}); diff --git a/webviews/common/test/message.test.ts b/webviews/common/test/message.test.ts index 5b1dd29568..fe166a0184 100644 --- a/webviews/common/test/message.test.ts +++ b/webviews/common/test/message.test.ts @@ -5,11 +5,19 @@ import assert from 'assert'; import { createSandbox } from 'sinon'; -import { MessageHandler, vscode } from '../message'; +import { vscodeTransport as vscode } from '../host'; +import { MessageHandler } from '../message'; describe('MessageHandler pending replies', () => { const sandbox = createSandbox(); - afterEach(() => sandbox.restore()); + let handler: MessageHandler; + beforeEach(() => { + handler = new MessageHandler(vscode); + }); + afterEach(() => { + handler.dispose(); + sandbox.restore(); + }); it('releases successful and rejected replies while keeping outstanding requests', async () => { const requests: string[] = []; @@ -17,7 +25,7 @@ describe('MessageHandler pending replies', () => { requests.push(message.req); }); const commands = sandbox.spy(); - const handler = new MessageHandler(commands); + handler.onCommand(commands); const first = handler.postMessage({ command: 'first' }); const second = handler.postMessage({ command: 'second' }); const secondAssertion = assert.rejects(second, error => error === 'Request failed'); @@ -40,7 +48,6 @@ describe('MessageHandler pending replies', () => { it('releases callbacks when sending throws synchronously', async () => { const failure = new Error('Transport failed'); sandbox.stub(vscode, 'postMessage').throws(failure); - const handler = new MessageHandler(null); await assert.rejects(handler.postMessage({ command: 'failure' }), error => error === failure); assert.strictEqual(handler['_pendingReplies'].size, 0); }); diff --git a/webviews/components/test/timestamp.test.tsx b/webviews/components/test/timestamp.test.tsx new file mode 100644 index 0000000000..3654afa47d --- /dev/null +++ b/webviews/components/test/timestamp.test.tsx @@ -0,0 +1,68 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import * as React from 'react'; +import { cleanup, render, wait } from 'react-testing-library'; +import { createSandbox, SinonSandbox } from 'sinon'; +import { dateFromNow } from '../../../src/common/utils'; +import { Timestamp, TimestampFormatContext } from '../timestamp'; + +describe('Timestamp', () => { + let sandbox: SinonSandbox; + + beforeEach(() => { + sandbox = createSandbox(); + }); + + afterEach(() => { + cleanup(); + sandbox.restore(); + }); + + it('uses an instance-local formatter without scheduling clock updates', async () => { + const interval = sandbox.spy(window, 'setInterval'); + const format = { + relative: (date: Date | string) => `fixed ${new Date(date).toISOString()}`, + title: (date: Date | string) => new Date(date).toISOString(), + }; + const view = (date: string) => + + ; + const out = render(view('2025-01-15T10:00:00Z')); + const link = out.container.querySelector('a')!; + assert.strictEqual(link.textContent, 'fixed 2025-01-15T10:00:00.000Z'); + assert.strictEqual(link.title, '2025-01-15T10:00:00.000Z'); + assert.strictEqual(link.getAttribute('href'), '#event'); + out.rerender(view('2025-01-16T10:00:00Z')); + await wait(() => assert.strictEqual(link.textContent, 'fixed 2025-01-16T10:00:00.000Z')); + assert.strictEqual(interval.callCount, 0); + }); + + it('preserves the default formatter and disposes live updates', async () => { + const date = new Date(); + const interval = sandbox.spy(window, 'setInterval'); + const clear = sandbox.spy(globalThis, 'clearInterval'); + const out = render(); + assert.strictEqual(out.container.textContent, dateFromNow(date)); + assert.strictEqual(out.container.querySelector('.timestamp')!.getAttribute('title'), date.toLocaleString()); + await wait(() => assert.strictEqual(interval.callCount, 1)); + out.unmount(); + await wait(() => assert(clear.calledWith(interval.firstCall.returnValue))); + }); + + it('does not leak one instance formatter into another', () => { + const date = '2025-01-15T10:00:00Z'; + const out = render(<> + 'fixed', title: () => 'UTC' }}> + + + + ); + const timestamps = out.container.querySelectorAll('.timestamp'); + assert.strictEqual(timestamps[0].textContent, 'fixed'); + assert.strictEqual(timestamps[1].textContent, dateFromNow(date)); + }); +}); diff --git a/webviews/components/timestamp.tsx b/webviews/components/timestamp.tsx index 25b15cd05c..00bf47e3fe 100644 --- a/webviews/components/timestamp.tsx +++ b/webviews/components/timestamp.tsx @@ -3,15 +3,27 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import React, { useEffect, useState } from 'react'; +import React, { createContext, useContext, useEffect, useState } from 'react'; import { dateFromNow } from '../../src/common/utils'; +export interface TimestampFormat { + relative(date: Date | string): string; + title(date: Date | string): string; +} + +export const TimestampFormatContext = createContext(undefined); + export const Timestamp = ({ date, href }: { date: Date | string; href?: string }) => { - const [timeString, setTimeString] = useState(dateFromNow(date)); - const title = typeof date === 'string' ? new Date(date).toLocaleString() : date.toLocaleString(); + const format = useContext(TimestampFormatContext); + const [timeString, setTimeString] = useState(() => format ? format.relative(date) : dateFromNow(date)); + const title = format ? format.title(date) : (typeof date === 'string' ? new Date(date).toLocaleString() : date.toLocaleString()); useEffect(() => { + if (format) { + setTimeString(format.relative(date)); + return; + } // Update the time string immediately setTimeString(dateFromNow(date)); @@ -86,7 +98,7 @@ export const Timestamp = ({ date, href }: { date: Date | string; href?: string } } document.removeEventListener('visibilitychange', handleVisibilityChange); }; - }, [date]); + }, [date, format]); return href ? ( diff --git a/webviews/createPullRequestViewNew/app.tsx b/webviews/createPullRequestViewNew/app.tsx index 51cc13daf3..5fdd0911ee 100644 --- a/webviews/createPullRequestViewNew/app.tsx +++ b/webviews/createPullRequestViewNew/app.tsx @@ -9,9 +9,11 @@ import { RemoteInfo } from '../../common/types'; import { CreateParamsNew, StackCandidate } from '../../common/views'; import { isITeam, MergeMethod } from '../../src/github/interface'; import { ChangeTemplateReply } from '../../src/github/views'; -import PullRequestContextNew from '../common/createContextNew'; +import PullRequestContextNew, { CreatePRContextNew } from '../common/createContextNew'; import { ErrorBoundary } from '../common/errorBoundary'; +import { createWebviewHost } from '../common/host'; import { LabelCreate } from '../common/label'; +import { rethrowUnlessDisposed } from '../common/message'; import { ContextDropdown } from '../components/contextDropdown'; import { accountIcon, feedbackIcon, gitCompareIcon, layersIcon, milestoneIcon, notebookTemplate, prMergeIcon, projectIcon, settingsIcon, sparkleIcon, stopCircleIcon, tagIcon } from '../components/icon'; import { Avatar } from '../components/user'; @@ -88,7 +90,10 @@ export const ChooseRemoteAndBranch = ({ onClick, defaultRemote, defaultBranch, i }; export function main() { + const host = createWebviewHost(); + const context = new CreatePRContextNew(host); render( + {(params: CreateParamsNew) => { const ctx = useContext(PullRequestContextNew); @@ -418,7 +423,8 @@ export function main() { ; }} - , + + , document.getElementById('app'), ); } @@ -431,7 +437,10 @@ export function Root({ children }: RootProps): JSX.Element { useEffect(() => { ctx.onchange = setPR; setPR(ctx.createParams); + return () => { + ctx.onchange = null; + }; }, []); - ctx.postMessage({ command: 'ready' }); + ctx.postMessage({ command: 'ready' }).catch(rethrowUnlessDisposed); return <>{children(pr)}; } diff --git a/webviews/createPullRequestViewNew/test/app.test.tsx b/webviews/createPullRequestViewNew/test/app.test.tsx index 5a61966574..04e972e779 100644 --- a/webviews/createPullRequestViewNew/test/app.test.tsx +++ b/webviews/createPullRequestViewNew/test/app.test.tsx @@ -9,8 +9,9 @@ import { unmountComponentAtNode } from 'react-dom'; import { act, cleanup, fireEvent, render } from 'react-testing-library'; import { createSandbox, SinonSandbox } from 'sinon'; import { StackCandidate } from '../../../common/views'; +import { createTestHost } from '../../../src/test/webviews/testHost'; import { CreatePRContextNew } from '../../common/createContextNew'; -import { MessageHandler, vscode } from '../../common/message'; +import { vscodeTransport as vscode } from '../../common/host'; import { main, makeCreateMenuContext, StackOption } from '../app'; describe('Create pull request stack', function () { @@ -31,7 +32,6 @@ describe('Create pull request stack', function () { unmountComponentAtNode(app); app.remove(); } - CreatePRContextNew.instance.onchange = null; vscode.setState(previousState); sinon.restore(); }); @@ -63,7 +63,7 @@ describe('Create pull request stack', function () { }); it('hides auto-merge menu choices only when adding to the stack', function () { - const context = new CreatePRContextNew(); + const context = new CreatePRContextNew(createTestHost()); const params = { ...context.createParams, allowAutoMerge: true, @@ -81,7 +81,7 @@ describe('Create pull request stack', function () { const app = document.createElement('div'); app.id = 'app'; document.body.appendChild(app); - const context = CreatePRContextNew.instance; + const context = new CreatePRContextNew(createTestHost()); context.updateState({ defaultBaseRemote: { owner: 'owner', repositoryName: 'repo' }, defaultBaseBranch: 'D3', @@ -97,6 +97,7 @@ describe('Create pull request stack', function () { autoMergeMethod: 'squash', mergeMethodsAvailability: { merge: true, squash: true, rebase: true }, }, true); + context.dispose(); act(() => { main(); }); const checkbox = app.querySelector('.stack-option input'); @@ -118,9 +119,9 @@ describe('Create pull request stack', function () { }); it('submits a stack request without auto-merge even when it was previously selected', async function () { - const handler = new MessageHandler(null); + const handler = createTestHost(); const postMessage = sinon.stub(handler, 'postMessage').resolves({}); - const context = new CreatePRContextNew(null, handler); + const context = new CreatePRContextNew(handler); context.updateState({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, baseBranch: 'D3', @@ -136,20 +137,25 @@ describe('Create pull request stack', function () { await context.submit(); assert(postMessage.calledOnce); - assert.strictEqual(postMessage.firstCall.args[0].args.addToStack, true); - assert.strictEqual(postMessage.firstCall.args[0].args.stackParentPullRequest, 795); - assert.strictEqual(postMessage.firstCall.args[0].args.stackNumber, 12); - assert.strictEqual(postMessage.firstCall.args[0].args.autoMerge, false); + assert(postMessage.calledWithMatch({ + command: 'pr.create', + args: { + addToStack: true, + stackParentPullRequest: 795, + stackNumber: 12, + autoMerge: false, + }, + })); }); it('clears a checked stack option when the base branch changes', async function () { - const handler = new MessageHandler(null); + const handler = createTestHost(); sinon.stub(handler, 'postMessage').resolves({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, baseBranch: 'D2', stackCandidate: { parentPullRequestNumber: 794, size: 1, url: 'https://github.com/owner/repo/pull/794' }, }); - const context = new CreatePRContextNew(null, handler); + const context = new CreatePRContextNew(handler); context.updateState({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, baseBranch: 'D3', @@ -174,7 +180,7 @@ describe('Create pull request stack', function () { { stackCandidate: { ...candidate, stackNumber: 13 } }, ]; for (const params of cases) { - const context = new CreatePRContextNew(); + const context = new CreatePRContextNew(createTestHost()); context.updateState({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, compareRemote: { owner: 'owner', repositoryName: 'repo' }, @@ -187,7 +193,7 @@ describe('Create pull request stack', function () { assert.strictEqual(context.createParams.addToStack, false); } - const context = new CreatePRContextNew(); + const context = new CreatePRContextNew(createTestHost()); context.updateState({ stackCandidate: candidate, addToStack: true }); await context.handleMessage({ command: 'pr.initialize', params: { pendingTitle: 'Fourth change' } }); assert.strictEqual(context.createParams.addToStack, true); @@ -196,9 +202,9 @@ describe('Create pull request stack', function () { }); it('keeps the selected branch and stack option when changing compare branches fails', async function () { - const handler = new MessageHandler(null); + const handler = createTestHost(); sinon.stub(handler, 'postMessage').rejects(new Error('Branch does not exist locally.')); - const context = new CreatePRContextNew(null, handler); + const context = new CreatePRContextNew(handler); context.updateState({ compareRemote: { owner: 'owner', repositoryName: 'repo' }, compareBranch: 'D4', @@ -214,14 +220,14 @@ describe('Create pull request stack', function () { }); it('clears a stale branch warning after a successful selection of the same base branch', async function () { - const handler = new MessageHandler(null); + const handler = createTestHost(); sinon.stub(handler, 'postMessage').resolves({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, baseBranch: 'D3', stackCandidate: candidate, warning: undefined, }); - const context = new CreatePRContextNew(null, handler); + const context = new CreatePRContextNew(handler); context.updateState({ baseRemote: { owner: 'owner', repositoryName: 'repo' }, baseBranch: 'D3', @@ -235,14 +241,14 @@ describe('Create pull request stack', function () { }); it('replaces a stale compare-branch warning with the current server warning', async function () { - const handler = new MessageHandler(null); + const handler = createTestHost(); sinon.stub(handler, 'postMessage').resolves({ compareRemote: { owner: 'owner', repositoryName: 'repo' }, compareBranch: 'D4', stackCandidate: candidate, warning: 'A pull request already exists for this branch.', }); - const context = new CreatePRContextNew(null, handler); + const context = new CreatePRContextNew(handler); context.updateState({ compareRemote: { owner: 'owner', repositoryName: 'repo' }, compareBranch: 'D4', diff --git a/webviews/editorWebview/app.tsx b/webviews/editorWebview/app.tsx index ae45fa8fed..c82a58cf61 100644 --- a/webviews/editorWebview/app.tsx +++ b/webviews/editorWebview/app.tsx @@ -10,10 +10,19 @@ import { Overview, OverviewPreview } from './overview'; import { extractCodeReferenceLinkMetadata } from '../../src/common/utils'; import { OverviewItemPreview, PullRequest } from '../../src/github/views'; import { COMMENT_TEXTAREA_ID } from '../common/constants'; -import PullRequestContext from '../common/context'; +import PullRequestContext, { PRContext } from '../common/context'; +import { createWebviewHost } from '../common/host'; +import { rethrowUnlessDisposed } from '../common/message'; export function main() { - render({pr => }, document.getElementById('app')); + const host = createWebviewHost(); + const context = new PRContext(host); + render( + + {pr => } + , + document.getElementById('app') + ); } export function Root({ children }) { @@ -74,18 +83,25 @@ export function Root({ children }) { return () => document.removeEventListener('click', handleLinkClick, true); }, [ctx]); - window.onscroll = debounce(() => { - ctx.postMessage({ - command: 'scroll', - args: { - scrollPosition: { - x: window.scrollX, - y: window.scrollY + useEffect(() => { + const onScroll = debounce(() => { + ctx.postMessage({ + command: 'scroll', + args: { + scrollPosition: { + x: window.scrollX, + y: window.scrollY + } } - } - }); - }, 200); - ctx.postMessage({ command: 'ready' }); - ctx.postMessage({ command: 'pr.debug', args: 'initialized ' + (pr ? 'with PR' : 'without PR') }); + }).catch(rethrowUnlessDisposed); + }, 200); + window.addEventListener('scroll', onScroll); + return () => { + window.removeEventListener('scroll', onScroll); + onScroll.clear(); + }; + }, [ctx]); + ctx.postMessage({ command: 'ready' }).catch(rethrowUnlessDisposed); + ctx.postMessage({ command: 'pr.debug', args: 'initialized ' + (pr ? 'with PR' : 'without PR') }).catch(rethrowUnlessDisposed); return pr ? children(pr) : preview ? :
Loading...
; } diff --git a/webviews/editorWebview/test/app.test.tsx b/webviews/editorWebview/test/app.test.tsx index 8499c5ce39..9247776746 100644 --- a/webviews/editorWebview/test/app.test.tsx +++ b/webviews/editorWebview/test/app.test.tsx @@ -12,8 +12,9 @@ import { AccountBuilder } from './builder/account'; import { PullRequestBuilder } from './builder/pullRequest'; import { GithubItemStateEnum } from '../../../src/github/interface'; import { IssuePreview, PullRequestPreview } from '../../../src/github/views'; +import { createTestHost } from '../../../src/test/webviews/testHost'; import { PRContext, default as PullRequestContext } from '../../common/context'; -import { vscode } from '../../common/message'; +import { vscodeTransport as vscode } from '../../common/host'; import { Root } from '../app'; import { Overview, OverviewPreview } from '../overview'; @@ -30,7 +31,7 @@ describe('Root', function () { }); it('displays "loading" while the PR is loading', function () { - const context = new PRContext(); + const context = new PRContext(createTestHost()); const children = sinon.stub(); assert(!context.pr); @@ -46,7 +47,7 @@ describe('Root', function () { }); it('renders preview HTML without exposing actions or persisting an incomplete PR', async function () { - const context = new PRContext(); + const context = new PRContext(createTestHost()); const persist = sinon.stub(vscode, 'setState'); const postMessage = sinon.stub(context, 'postMessage').resolves(); const children = sinon.stub().returns(
Complete overview
); @@ -98,7 +99,7 @@ describe('Root', function () { }); it('renders an issue preview without pull request fields', async function () { - const context = new PRContext(); + const context = new PRContext(createTestHost()); const out = render( {() =>
Complete overview
} @@ -121,7 +122,7 @@ describe('Root', function () { }); it('renders cached issue preview text when HTML is unavailable without treating it as markup', async function () { - const context = new PRContext(); + const context = new PRContext(createTestHost()); const persist = sinon.stub(vscode, 'setState'); const children = sinon.stub(); const out = render( @@ -148,7 +149,7 @@ describe('Root', function () { it('uses the final title, subtitle and description markup in the preview', function () { const pr = new PullRequestBuilder().canEdit(false).isAuthor(false).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const out = render( @@ -169,15 +170,24 @@ describe('Root', function () { it('uses a collapsed metadata placeholder in a narrow preview', function () { const media = window.matchMedia('(max-width: 768px)'); sinon.stub(window, 'matchMedia').returns({ ...media, matches: true, addEventListener() { }, removeEventListener() { } }); - const out = render(); - assert(out.container.querySelector('.collapsible-sidebar')); - assert.strictEqual(out.container.querySelector('#sidebar'), null); - assert.strictEqual(out.container.querySelector('[role="button"]'), null); + const pr = new PullRequestBuilder().build(); + const context = new PRContext(createTestHost(pr)); + const out = render( + + ); + try { + assert(out.container.querySelector('.collapsible-sidebar')); + assert.strictEqual(out.container.querySelector('#sidebar'), null); + assert.strictEqual(out.container.querySelector('[role="button"]'), null); + } finally { + out.unmount(); + context.dispose(); + } }); it('renders its child prop with a pull request from the context', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const children = sinon.stub().returns(
); render( @@ -193,7 +203,7 @@ describe('Root', function () { const pr = new PullRequestBuilder().build(); pr.isCurrentlyCheckedOut = true; pr.doneCheckoutBranch = 'main'; - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); const checkout = sinon.spy(context, 'checkout'); const out = render( diff --git a/webviews/editorWebview/test/merge.test.tsx b/webviews/editorWebview/test/merge.test.tsx index 282bd0d840..bbc6e56b27 100644 --- a/webviews/editorWebview/test/merge.test.tsx +++ b/webviews/editorWebview/test/merge.test.tsx @@ -10,6 +10,7 @@ import { createSandbox, SinonFakeTimers, SinonSandbox } from 'sinon'; import { PullRequestBuilder } from './builder/pullRequest'; import { PullRequestMergeability } from '../../../src/github/interface'; +import { createTestHost } from '../../../src/test/webviews/testHost'; import { PRContext, default as PullRequestContext } from '../../common/context'; import { MergeStatusAndActions } from '../../components/merge'; @@ -32,7 +33,7 @@ describe('Merge status and actions', function () { .mergeable(PullRequestMergeability.Unknown) .hasWritePermission(false) .build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const checkMergeability = sinon.stub(context, 'checkMergeability').resolves({ mergeability: PullRequestMergeability.Mergeable, }); diff --git a/webviews/editorWebview/test/overview.test.tsx b/webviews/editorWebview/test/overview.test.tsx index 7937b3644c..7c583d1f6c 100644 --- a/webviews/editorWebview/test/overview.test.tsx +++ b/webviews/editorWebview/test/overview.test.tsx @@ -12,6 +12,7 @@ import { createSandbox, SinonSandbox } from 'sinon'; import { PullRequestBuilder } from './builder/pullRequest'; import { CheckState, GithubItemStateEnum, PullRequestCheckStatus, PullRequestMergeability } from '../../../src/github/interface'; +import { createTestHost } from '../../../src/test/webviews/testHost'; import { Root as ActivityBarRoot } from '../../activityBarView/app'; import { Overview as ActivityBarOverview } from '../../activityBarView/overview'; import { PRContext, default as PullRequestContext } from '../../common/context'; @@ -32,7 +33,7 @@ describe('Overview', function () { it('renders the PR header with title', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const out = render( @@ -46,7 +47,7 @@ describe('Overview', function () { it('opens PR number links on GitHub', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const openOnGitHub = sinon.stub(context, 'openOnGitHub'); const out = render( @@ -90,7 +91,7 @@ describe('Overview', function () { isRequired: index % 2 === 0, })); const pr = new PullRequestBuilder().status(status => status.state(CheckState.Failure).statuses(statuses)).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const viewCheckLogs = sinon.stub(context, 'viewCheckLogs').resolves(); const out = render( @@ -147,7 +148,7 @@ describe('Overview', function () { try { for (const Component of [Overview, ActivityBarOverview]) { const out = render( - + , ); @@ -167,7 +168,7 @@ describe('Overview', function () { it('opens a PR number link exactly once', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const openOnGitHub = sinon.stub(context, 'openOnGitHub'); // Stands in for the webview host, which opens any anchor with an href that a click @@ -215,7 +216,7 @@ describe('Overview', function () { { position: 3, number: 795, title: 'Third Change', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: true, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); const mergeStack = sinon.stub(context, 'mergeStack').resolves({ status: 'merged', state: GithubItemStateEnum.Merged }); const openOnGitHub = sinon.stub(context, 'openOnGitHub'); @@ -284,7 +285,7 @@ describe('Overview', function () { ], }).build(); const out = render( - + , ); @@ -324,7 +325,7 @@ describe('Overview', function () { })), }).build(); const out = render( - + , ); @@ -354,7 +355,7 @@ describe('Overview', function () { it('does not show a stack badge or section for an unstacked pull request', function () { const pr = new PullRequestBuilder().build(); const out = render( - + , ); @@ -377,7 +378,7 @@ describe('Overview', function () { state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable, })), }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); const out = render( @@ -423,7 +424,7 @@ describe('Overview', function () { ], }; const pr = new PullRequestBuilder().number(795).stack(stack).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const unstackAll = sinon.stub(context, 'unstackAll').resolves({ cancelled: false, remainingPullRequests: [794] }); const out = render( @@ -442,13 +443,13 @@ describe('Overview', function () { assert(section.hasAttribute('open')); out.rerender( - + , ); assert.strictEqual(out.queryByText('Unstack all'), null); out.rerender( - + , ); @@ -462,7 +463,7 @@ describe('Overview', function () { { position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); sinon.stub(context, 'unstackAll').rejects(new Error('Stack is locked')); const out = render( @@ -485,7 +486,7 @@ describe('Overview', function () { }], }).build(); const out = render( - + , ); @@ -505,7 +506,7 @@ describe('Overview', function () { ], }; const pr = new PullRequestBuilder().stack(stack).canUpdateStack(true).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [1233, 1234] }); const out = render( @@ -525,7 +526,7 @@ describe('Overview', function () { const entry = { position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }; const stack = { position: 1, size: 1, base: 'main', pullRequests: [entry] }; const pr = new PullRequestBuilder().stack(stack).canUpdateStack(true).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const out = render(); assert(out.getByText('Update stack')); for (const change of [ @@ -544,7 +545,7 @@ describe('Overview', function () { position: 1, size: 1, base: 'main', pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); sinon.stub(context, 'updateStack').rejects(new Error('Branch changed on GitHub')); const out = render(); fireEvent.click(out.getByText('Update stack')); @@ -562,7 +563,7 @@ describe('Overview', function () { state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Behind, }], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const failure = new Error('Previous action failed'); const update = sinon.stub(context, 'updateStack'); const unstack = sinon.stub(context, 'unstackAll'); @@ -610,7 +611,7 @@ describe('Overview', function () { ], }).build(); const out = render( - + , ); @@ -628,7 +629,7 @@ describe('Overview', function () { it('keeps the original Delete Branch placement outside stacks', function () { const pr = new PullRequestBuilder().state(GithubItemStateEnum.Closed).build(); const out = render( - + , ); @@ -649,7 +650,7 @@ describe('Overview', function () { ], }).build(); const out = render( - + , ); @@ -673,7 +674,7 @@ describe('Overview', function () { }; for (const mergeable of [PullRequestMergeability.Behind, PullRequestMergeability.Mergeable, PullRequestMergeability.NotMergeable]) { const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).mergeable(mergeable).stack(stack).build(); - const out = render(); + const out = render(); assert(out.container.querySelector('#pull-request-stack')); if (mergeable === PullRequestMergeability.Behind) { assert(out.getByText('This branch is out-of-date with the base branch.')); @@ -693,7 +694,7 @@ describe('Overview', function () { }; const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).canUpdateStack(true) .mergeable(PullRequestMergeability.Behind).stack(stack).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [794] }); const updateBranch = sinon.stub(context, 'updateBranch'); const out = render(); @@ -721,7 +722,7 @@ describe('Overview', function () { }; const pr = new PullRequestBuilder().number(794).canUpdateBranch(true).canUpdateStack(true) .mergeable(PullRequestMergeability.Behind).stack(stack).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [794, 795] }); const out = render(); @@ -741,7 +742,7 @@ describe('Overview', function () { { position: 3, number: 795, title: 'Third', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); - const out = render(); + const out = render(); assert.strictEqual(out.queryByText('Update stack'), null); }); @@ -751,7 +752,7 @@ describe('Overview', function () { position: 1, size: 1, base: 'main', pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.NotMergeable }], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const updateStack = sinon.stub(context, 'updateStack').resolves({ updatedPullRequests: [1234] }); const updateBranch = sinon.stub(context, 'updateBranch'); const out = render(); @@ -767,7 +768,7 @@ describe('Overview', function () { it('retains merge-commit updates for unstacked PRs and conflict resolution for stacked PRs', function () { for (const mergeable of [PullRequestMergeability.Behind, PullRequestMergeability.Mergeable]) { const pr = new PullRequestBuilder().canUpdateBranch(true).mergeable(mergeable).build(); - const out = render(); + const out = render(); assert(out.getByText(/Update with merge commit/i)); out.unmount(); } @@ -775,7 +776,7 @@ describe('Overview', function () { position: 1, size: 1, base: 'main', pullRequests: [{ position: 1, number: 1234, title: 'First', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Conflict }], }).build(); - const out = render(); + const out = render(); assert(out.getByText('Resolve conflicts')); assert.strictEqual(out.queryByText(/Update with merge commit/i), null); }); @@ -783,7 +784,7 @@ describe('Overview', function () { it('does not offer merge-commit updates before stack membership is known or when loading fails', function () { for (const change of [{ stackLoaded: false }, { stackLoadError: true }]) { const pr = { ...new PullRequestBuilder().canUpdateBranch(true).mergeable(PullRequestMergeability.Behind).build(), ...change }; - const out = render(); + const out = render(); assert.strictEqual(out.queryByText(/Update with merge commit/i), null); out.unmount(); } @@ -816,7 +817,7 @@ describe('Overview', function () { ]; for (const pr of blocked) { const out = render( - + , ); @@ -839,7 +840,7 @@ describe('Overview', function () { }; const pr = new PullRequestBuilder().number(794).stack(stack).build(); const out = render( - + , ); @@ -856,7 +857,7 @@ describe('Overview', function () { ], }).build(); const out = render( - + , ); @@ -876,7 +877,7 @@ describe('Overview', function () { ], }).build(); const out = render( - + , ); @@ -894,7 +895,7 @@ describe('Overview', function () { { position: 1, number: 1234, title: 'First Change', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); const mergeStack = sinon.stub(context, 'mergeStack').resolves({ status: 'pending' }); const out = render( @@ -922,7 +923,7 @@ describe('Overview', function () { { position: 1, number: 1234, title: 'First Change', head: 'D1', url: 'https://example.com/1234', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const mergeStack = sinon.stub(context, 'mergeStack').rejects(new Error('Required checks failed')); const out = render( @@ -942,7 +943,7 @@ describe('Overview', function () { it('does not offer a legacy merge while stack membership is loading or failed', function () { const pr = new PullRequestBuilder().stackLoaded(false).build(); const out = render( - + , ); @@ -951,7 +952,7 @@ describe('Overview', function () { const failed = { ...pr, stackLoaded: true, stackLoadError: true }; out.rerender( - + , ); @@ -961,7 +962,7 @@ describe('Overview', function () { it('shows view changes in both headers', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const viewChanges = sinon.stub(context, 'viewChanges'); const out = render( @@ -981,7 +982,7 @@ describe('Overview', function () { it('does not show view changes in either header for issues', function () { const pr = new PullRequestBuilder().isIssue(true).build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const out = render( @@ -994,7 +995,7 @@ describe('Overview', function () { it('applies sticky class when scrolled', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); const out = render( @@ -1016,7 +1017,7 @@ describe('Overview', function () { it('applies deferred pull request updates', function () { const pr = new PullRequestBuilder().build(); - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); context.handleMessage({ diff --git a/webviews/editorWebview/test/reviewSummary.test.tsx b/webviews/editorWebview/test/reviewSummary.test.tsx index 4e4e4e6a93..be2c8316d5 100644 --- a/webviews/editorWebview/test/reviewSummary.test.tsx +++ b/webviews/editorWebview/test/reviewSummary.test.tsx @@ -6,11 +6,13 @@ import { default as assert } from 'assert'; import * as React from 'react'; import { act, cleanup, fireEvent, render } from 'react-testing-library'; +import { createSandbox } from 'sinon'; import { PullRequestBuilder } from './builder/pullRequest'; import { EventType, ReviewEvent } from '../../../src/common/timelineEvent'; +import { createTestHost } from '../../../src/test/webviews/testHost'; import { PRContext, default as PullRequestContext } from '../../common/context'; -import { MessageHandler, vscode } from '../../common/message'; +import { vscodeTransport as vscode } from '../../common/host'; import { Timeline } from '../../components/timeline'; const ReviewSummaryView = ({ context }: { context: PRContext }) => { @@ -55,7 +57,7 @@ describe('Review summary', function () { }; pr.events = [pendingReview]; - const context = new PRContext(pr); + const context = new PRContext(createTestHost(pr)); context.setPR(pr); const view = render(); const summary = view.getByPlaceholderText('Leave a review summary comment') as HTMLTextAreaElement; @@ -74,7 +76,7 @@ describe('Review summary', function () { }); view.unmount(); - const restoredContext = new PRContext(); + const restoredContext = new PRContext(createTestHost()); const restoredView = render(); const restoredSummary = restoredView.getByPlaceholderText('Leave a review summary comment') as HTMLTextAreaElement; @@ -109,13 +111,17 @@ describe('Review summary', function () { state: 'PENDING', }; pr.events = [pendingReview]; - const context = new PRContext(pr, null, { - postMessage: async () => ({ deletedReviewId: pendingReview.id }), - } as unknown as MessageHandler); - context.setPR(pr); - - await context.deleteReview(); - - assert.strictEqual(context.pr?.pendingReviewSummaryText, ''); + const host = createTestHost(pr); + const sandbox = createSandbox(); + sandbox.stub(host, 'postMessage').resolves({ deletedReviewId: pendingReview.id }); + const context = new PRContext(host); + try { + context.setPR(pr); + await context.deleteReview(); + assert.strictEqual(context.pr?.pendingReviewSummaryText, ''); + } finally { + context.dispose(); + sandbox.restore(); + } }); });