diff --git a/webviews/components/comment.tsx b/webviews/components/comment.tsx index 6390c4b571..1d4c9bf3fd 100644 --- a/webviews/components/comment.tsx +++ b/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 =
; const containsSuggestion = ((body || bodyHTML)?.indexOf('```diff') ?? -1) > -1; @@ -514,12 +514,18 @@ export function AddComment({ const form = useRef(); const textareaRef = useRef(); - 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 = e => { e.preventDefault(); @@ -583,7 +589,7 @@ export function AddComment({ id={COMMENT_TEXTAREA_ID} name="body" ref={textareaRef as React.MutableRefObject} - onInput={({ target }) => updatePR({ pendingCommentText: (target as HTMLTextAreaElement).value })} + onChange={event => updatePR({ pendingCommentText: event.currentTarget.value })} onKeyDown={onKeyDown} onPaste={onPasteUploadFiles(uploadPastedFilesIntoPendingComment)} value={pendingCommentText} diff --git a/webviews/components/pullRequestStack.tsx b/webviews/components/pullRequestStack.tsx index 84c77501ff..34570b3eba 100644 --- a/webviews/components/pullRequestStack.tsx +++ b/webviews/components/pullRequestStack.tsx @@ -50,6 +50,11 @@ export const StackSection = ({ pr }: { pr: PullRequest }) => { const [error, setError] = React.useState(); const [updating, setUpdating] = React.useState(false); const [updateError, setUpdateError] = React.useState(); + 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); + } } }; diff --git a/webviews/components/sidebar.tsx b/webviews/components/sidebar.tsx index e776a2e4d0..ef773445f1 100644 --- a/webviews/components/sidebar.tsx +++ b/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 }[] }) => ( {users.slice(0, 10).map((u, i) => ( - @@ -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', diff --git a/webviews/components/test/comment.test.tsx b/webviews/components/test/comment.test.tsx new file mode 100644 index 0000000000..0b410bfe7d --- /dev/null +++ b/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( + + ); + 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) => + + ; + 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(); + } + }); +}); diff --git a/webviews/components/test/pullRequestStack.test.tsx b/webviews/components/test/pullRequestStack.test.tsx new file mode 100644 index 0000000000..974c458074 --- /dev/null +++ b/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((_, reject) => { rejectPending = reject; }); + const request = sandbox.stub(host, 'postMessage').returns(pending); + const errors = sandbox.stub(console, 'error'); + const out = render( + + ); + try { + fireEvent.click(out.getByText(action)); + assert.strictEqual(request.callCount, 1); + out.unmount(); + rejectPending(new Error('Host disposed')); + await new Promise(resolve => setTimeout(resolve, 0)); + assert.deepStrictEqual(errors.args, []); + } finally { + out.unmount(); + context.dispose(); + sandbox.restore(); + } + }); + } +}); diff --git a/webviews/components/test/sidebar.test.tsx b/webviews/components/test/sidebar.test.tsx new file mode 100644 index 0000000000..94a9f39e7c --- /dev/null +++ b/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( + + ); + assert.strictEqual(out.container.querySelectorAll('.stacked-avatar').length, 2); + assert(!error.args.some(args => String(args[0]).includes('key'))); + } finally { + context.dispose(); + sandbox.restore(); + } + }); +});