Fix webview comment and stack action lifecycle

webviews/components/comment.tsx

@@ -425,6 +425,7 @@ export interface Embodied {
 }
 
 export const CommentBody = ({ comment, bodyHTML, body, canApplyPatch, allowEmpty, specialDisplayBodyPostfix }: Embodied) => {
+	const { applyPatch } = useContext(PullRequestContext);
 	if (!body && !bodyHTML) {
 		if (allowEmpty) {
 			return null;
@@ -436,7 +437,6 @@ export const CommentBody = ({ comment, bodyHTML, body, canApplyPatch, allowEmpty
 		);
 	}
 
-	const { applyPatch } = useContext(PullRequestContext);
 	const renderedBody = <div dangerouslySetInnerHTML={{ __html: bodyHTML ?? '' }} />;
 
 	const containsSuggestion = ((body || bodyHTML)?.indexOf('```diff') ?? -1) > -1;
@@ -514,12 +514,18 @@ export function AddComment({
 	const form = useRef<HTMLFormElement>();
 	const textareaRef = useRef<HTMLTextAreaElement>();
 
-	emitter.addListener('quoteReply', (message: string) => {
-		const quoted = message.replace(/\n/g, '\n> ');
-		updatePR({ pendingCommentText: `> ${quoted} \n\n` });
-		textareaRef.current?.scrollIntoView();
-		textareaRef.current?.focus();
-	});
+	useEffect(() => {
+		const quoteReply = (message: string) => {
+			const quoted = message.replace(/\n/g, '\n> ');
+			updatePR({ pendingCommentText: `> ${quoted} \n\n` });
+			textareaRef.current?.scrollIntoView();
+			textareaRef.current?.focus();
+		};
+		emitter.addListener('quoteReply', quoteReply);
+		return () => {
+			emitter.removeListener('quoteReply', quoteReply);
+		};
+	}, [updatePR]);
 
 	const closeButton: React.MouseEventHandler<HTMLButtonElement> = e => {
 		e.preventDefault();
@@ -583,7 +589,7 @@ export function AddComment({
 					id={COMMENT_TEXTAREA_ID}
 					name="body"
 					ref={textareaRef as React.MutableRefObject<HTMLTextAreaElement>}
-					onInput={({ target }) => updatePR({ pendingCommentText: (target as HTMLTextAreaElement).value })}
+					onChange={event => updatePR({ pendingCommentText: event.currentTarget.value })}
 					onKeyDown={onKeyDown}
 					onPaste={onPasteUploadFiles(uploadPastedFilesIntoPendingComment)}
 					value={pendingCommentText}

webviews/components/pullRequestStack.tsx

@@ -50,6 +50,11 @@ export const StackSection = ({ pr }: { pr: PullRequest }) => {
 	const [error, setError] = React.useState<string | undefined>();
 	const [updating, setUpdating] = React.useState(false);
 	const [updateError, setUpdateError] = React.useState<string | undefined>();
+	const mounted = React.useRef(true);
+	React.useLayoutEffect(() => {
+		mounted.current = true;
+		return () => { mounted.current = false; };
+	}, []);
 	const { stack } = pr;
 	if (!stack) {
 		return null;
@@ -65,9 +70,13 @@ export const StackSection = ({ pr }: { pr: PullRequest }) => {
 			setUpdateError(undefined);
 			await unstackAll();
 		} catch (unstackError) {
-			setError(`Unable to unstack pull requests: ${unstackError instanceof Error ? unstackError.message || unstackError.name : String(unstackError)}`);
+			if (mounted.current) {
+				setError(`Unable to unstack pull requests: ${unstackError instanceof Error ? unstackError.message || unstackError.name : String(unstackError)}`);
+			}
 		} finally {
-			setBusy(false);
+			if (mounted.current) {
+				setBusy(false);
+			}
 		}
 	};
 
@@ -78,9 +87,13 @@ export const StackSection = ({ pr }: { pr: PullRequest }) => {
 			setError(undefined);
 			await updateStack();
 		} catch (updateFailure) {
-			setUpdateError(`Unable to update the stack: ${updateFailure instanceof Error ? updateFailure.message || updateFailure.name : String(updateFailure)}`);
+			if (mounted.current) {
+				setUpdateError(`Unable to update the stack: ${updateFailure instanceof Error ? updateFailure.message || updateFailure.name : String(updateFailure)}`);
+			}
 		} finally {
-			setUpdating(false);
+			if (mounted.current) {
+				setUpdating(false);
+			}
 		}
 	};
 

webviews/components/sidebar.tsx

@@ -346,12 +346,12 @@ function CollapsedLabel(props: PullRequest) {
 		return () => window.removeEventListener('resize', checkViewportWidth);
 	}, []);
 
-	const AvatarStack = ({ users }: { users: { avatarUrl: string; name: string }[] }) => (
+	const AvatarStack = ({ users }: { users: { id: string; avatarUrl: string; name: string }[] }) => (
 		<span className="avatar-stack" style={{
 			width: `${Math.min(users.length, 10) * 10 + 10}px`
 		}}>
 			{users.slice(0, 10).map((u, i) => (
-				<span className='stacked-avatar' style={{
+				<span key={u.id} className='stacked-avatar' style={{
 					left: `${i * 10}px`,
 				}}>
 					<Avatar for={u} />
@@ -430,7 +430,7 @@ function CollapsedLabel(props: PullRequest) {
 	// Collect non-empty sections in order, with custom rendering
 	const sections: { label: string; value: React.ReactNode; count: number }[] = [];
 
-	const reviewersWithAvatar = reviewers?.filter((r): r is ReviewState & { reviewer: { avatarUrl: string } } => !!r.reviewer.avatarUrl).map(r => ({ avatarUrl: r.reviewer.avatarUrl, name: reviewerLabel(r.reviewer) }));
+	const reviewersWithAvatar = reviewers?.filter((r): r is ReviewState & { reviewer: { avatarUrl: string } } => !!r.reviewer.avatarUrl).map(r => ({ id: reviewerId(r.reviewer), avatarUrl: r.reviewer.avatarUrl, name: reviewerLabel(r.reviewer) }));
 	if (!isIssue && reviewersWithAvatar && reviewersWithAvatar.length) {
 		sections.push({
 			label: 'Reviewers',
@@ -439,7 +439,7 @@ function CollapsedLabel(props: PullRequest) {
 		});
 	}
 
-	const assigneesWithAvatar = assignees?.filter((a): a is IAccount & { avatarUrl: string; login: string } => !!a.avatarUrl).map(a => ({ avatarUrl: a.avatarUrl, name: reviewerLabel(a) }));
+	const assigneesWithAvatar = assignees?.filter((a): a is IAccount & { avatarUrl: string; login: string } => !!a.avatarUrl).map(a => ({ id: a.id, avatarUrl: a.avatarUrl, name: reviewerLabel(a) }));
 	if (assigneesWithAvatar && assigneesWithAvatar.length) {
 		sections.push({
 			label: 'Assignees',

webviews/components/test/comment.test.tsx

@@ -0,0 +1,75 @@
+/*---------------------------------------------------------------------------------------------
+ *  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 { act, cleanup, fireEvent, render, wait } from 'react-testing-library';
+import { createSandbox } from 'sinon';
+import { createTestHost } from '../../../src/test/webviews/testHost';
+import PullRequestContext, { PRContext } from '../../common/context';
+import emitter from '../../common/events';
+import { vscodeTransport as vscode } from '../../common/host';
+import { PullRequestBuilder } from '../../editorWebview/test/builder/pullRequest';
+import { AddComment } from '../comment';
+
+describe('Comment composer quote replies', () => {
+	afterEach(() => {
+		cleanup();
+		vscode.setState(undefined);
+	});
+
+	it('updates a controlled draft through the React change event without warnings', () => {
+		const sandbox = createSandbox();
+		const context = new PRContext(createTestHost(new PullRequestBuilder().build()));
+		context.updatePR({ pendingCommentText: 'Existing draft' });
+		const errors = sandbox.stub(console, 'error');
+		const out = render(<PullRequestContext.Provider value={context}>
+			<AddComment {...context.pr!} />
+		</PullRequestContext.Provider>);
+		try {
+			const textarea = out.container.querySelector('textarea')!;
+			assert.strictEqual(textarea.value, 'Existing draft');
+			fireEvent.change(textarea, { target: { value: 'Updated draft' } });
+			assert.strictEqual(context.pr!.pendingCommentText, 'Updated draft');
+			assert.deepStrictEqual(errors.args, []);
+		} finally {
+			out.unmount();
+			context.dispose();
+			sandbox.restore();
+		}
+	});
+
+	it('registers once across rerenders and removes its listener on unmount', async () => {
+		const sandbox = createSandbox();
+		const pr = new PullRequestBuilder().build();
+		const context = new PRContext(createTestHost(pr));
+		const update = sandbox.spy(context, 'updatePR');
+		const previousListeners = emitter.listenerCount('quoteReply');
+		const view = (busy: boolean) => <PullRequestContext.Provider value={context}>
+			<AddComment {...pr} busy={busy} />
+		</PullRequestContext.Provider>;
+		const out = render(view(false));
+		try {
+			await wait(() => assert.strictEqual(emitter.listenerCount('quoteReply'), previousListeners + 1));
+			out.rerender(view(true));
+			out.rerender(view(false));
+			assert.strictEqual(emitter.listenerCount('quoteReply'), previousListeners + 1);
+			const textarea = out.container.querySelector('textarea')!;
+			textarea.scrollIntoView = sandbox.spy();
+			act(() => { emitter.emit('quoteReply', 'First line\nSecond line'); });
+			assert.strictEqual(update.callCount, 1);
+			assert.strictEqual(context.pr!.pendingCommentText, '> First line\n> Second line \n\n');
+			assert.strictEqual(document.activeElement, textarea);
+			out.unmount();
+			await wait(() => assert.strictEqual(emitter.listenerCount('quoteReply'), previousListeners));
+			emitter.emit('quoteReply', 'After unmount');
+			assert.strictEqual(update.callCount, 1);
+		} finally {
+			out.unmount();
+			context.dispose();
+			sandbox.restore();
+		}
+	});
+});

webviews/components/test/pullRequestStack.test.tsx

@@ -0,0 +1,54 @@
+/*---------------------------------------------------------------------------------------------
+ *  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, fireEvent, render } from 'react-testing-library';
+import { createSandbox } from 'sinon';
+import { GithubItemStateEnum, PullRequestMergeability } from '../../../src/github/interface';
+import { createTestHost } from '../../../src/test/webviews/testHost';
+import PullRequestContext, { PRContext } from '../../common/context';
+import { PullRequestBuilder } from '../../editorWebview/test/builder/pullRequest';
+import { StackSection } from '../pullRequestStack';
+
+describe('Stack action disposal', () => {
+	afterEach(cleanup);
+
+	for (const action of ['Update stack', 'Unstack all']) {
+		it(`does not update an unmounted component when ${action} rejects`, async () => {
+			const sandbox = createSandbox();
+			const pr = new PullRequestBuilder().stack({
+				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();
+			pr.hasWritePermission = true;
+			pr.canUpdateStack = true;
+			const host = createTestHost(pr);
+			const context = new PRContext(host);
+			let rejectPending!: (error: Error) => void;
+			const pending = new Promise<never>((_, reject) => { rejectPending = reject; });
+			const request = sandbox.stub(host, 'postMessage').returns(pending);
+			const errors = sandbox.stub(console, 'error');
+			const out = render(<PullRequestContext.Provider value={context}>
+				<StackSection pr={pr} />
+			</PullRequestContext.Provider>);
+			try {
+				fireEvent.click(out.getByText(action));
+				assert.strictEqual(request.callCount, 1);
+				out.unmount();
+				rejectPending(new Error('Host disposed'));
+				await new Promise<void>(resolve => setTimeout(resolve, 0));
+				assert.deepStrictEqual(errors.args, []);
+			} finally {
+				out.unmount();
+				context.dispose();
+				sandbox.restore();
+			}
+		});
+	}
+});

webviews/components/test/sidebar.test.tsx

@@ -0,0 +1,36 @@
+/*---------------------------------------------------------------------------------------------
+ *  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 } from 'react-testing-library';
+import { createSandbox } from 'sinon';
+import { createTestHost } from '../../../src/test/webviews/testHost';
+import PullRequestContext, { PRContext } from '../../common/context';
+import { AccountBuilder } from '../../editorWebview/test/builder/account';
+import { PullRequestBuilder } from '../../editorWebview/test/builder/pullRequest';
+import { CollapsibleSidebar } from '../sidebar';
+
+describe('Collapsed sidebar avatars', () => {
+	afterEach(cleanup);
+
+	it('renders accounts with shared names and avatars without duplicate or missing keys', () => {
+		const sandbox = createSandbox();
+		const error = sandbox.spy(console, 'error');
+		const assignees = ['first', 'second'].map(id => new AccountBuilder().id(id).build());
+		const pr = new PullRequestBuilder().assignees(assignees).build();
+		const context = new PRContext(createTestHost(pr));
+		try {
+			const out = render(<PullRequestContext.Provider value={context}>
+				<CollapsibleSidebar {...pr} />
+			</PullRequestContext.Provider>);
+			assert.strictEqual(out.container.querySelectorAll('.stacked-avatar').length, 2);
+			assert(!error.args.some(args => String(args[0]).includes('key')));
+		} finally {
+			context.dispose();
+			sandbox.restore();
+		}
+	});
+});