From db3dbc37de0746095f969cb52ccdc1bfea5e650a Mon Sep 17 00:00:00 2001 From: swithek <52840391+swithek@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:05:37 +0200 Subject: [PATCH] fix comments in diff mode --- e2e/AGENTS.md | 27 +- e2e/helpers/comments.ts | 122 ++- e2e/helpers/editor.ts | 164 ++++ e2e/helpers/review.ts | 18 +- e2e/tests/review-comments.test.ts | 919 ++++++++++++++++++ .../comments/CommentRenderer.nuxt.test.ts | 137 ++- .../editor/comments/CommentRenderer.vue | 7 + 7 files changed, 1390 insertions(+), 4 deletions(-) create mode 100644 e2e/tests/review-comments.test.ts diff --git a/e2e/AGENTS.md b/e2e/AGENTS.md index 2426d85a..814d9428 100644 --- a/e2e/AGENTS.md +++ b/e2e/AGENTS.md @@ -199,7 +199,15 @@ cheaper tier can: is rendered inside the paragraph, so a plain text read splices the other user's name into the content. - **The title and the body are two editors** (`titleEditor()`, - `contentEditor()`); Enter in the title moves into the body. + `contentEditor()`); Enter in the title moves into the body. With "show + changes" on, the body is a third, `diffEditor()`, and everything an + editor renders outside its ProseMirror element (handle, bubble menu, + comment popover) is reached through its pane, `contentPane()` or + `diffPane()`. +- **Block handles come through `openBlockMenu()`**, which hovers the block + until the handle shows: the handle stays away while the pointer is level + with any `drag-handle-ignore` element in the editor, such as a parameter + separator in the other column of a split block. ### Independence & concurrency @@ -228,10 +236,27 @@ cheaper tier can: - **`documentPersisted()` is the one permitted `waitForTimeout`**: hocuspocus stores two seconds after the last change and nothing reports it. +- **The editor's selection lags the browser's caret.** A click or an arrow + key moves the caret at once; tiptap learns of it from the queued + `selectionchange` event. A key tiptap handles itself (Enter, Backspace) + or a click handler that reads the selection, sent straight after, acts + on the old one. Place and extend selections through `placeCaret()` and + `selectText()`, and open a text comment through `openTextComment()`, + which all wait for the editor to catch up. Never `Home` or `End`: macOS + chromium binds them to the document, not the line. - Three waits are raised above the default (post-signup redirect, post-login redirect, cold document load), all cross-service chains. Any other raised timeout carries a comment saying what is slow. +### A test that fails after a code change + +- **Never edit a test just to make it pass again.** First decide whether + the code or the test is wrong. A failure caused by anything other than an + intended change is a bug the test caught; fix the code. +- Change a test only where the behaviour it asserts changed on purpose, + and change only those assertions. Name that intended change when + reporting the edit. + ### Failure artifacts `trace: "on-first-retry"`, `screenshot: "only-on-failure"`; `test-results/` diff --git a/e2e/helpers/comments.ts b/e2e/helpers/comments.ts index 9f5a67ec..f17e756c 100644 --- a/e2e/helpers/comments.ts +++ b/e2e/helpers/comments.ts @@ -1,5 +1,5 @@ import { expect, type Locator, type Page } from "@playwright/test" -import { contentEditor } from "./editor" +import { contentEditor, openBlockMenu, selectionSettled } from "./editor" import { t } from "./i18n" // commentPopover is the floating comment-thread panel. It is rendered @@ -9,6 +9,13 @@ export function commentPopover(page: Page): Locator { return page.locator(".content-editor div.z-popover") } +// paneCommentPopover is the same panel found through the pane it belongs +// to, for a test that drives the draft's diff view, where a second +// renderer owns the popover. +export function paneCommentPopover(pane: Locator): Locator { + return pane.locator("div.z-popover") +} + // commentComposer is the tiptap instance inside the popover that new // comment text is typed into. Saved comment bodies are read-only tiptap // instances in the same popover, so the editable one is the composer. @@ -46,3 +53,116 @@ export async function addComment(page: Page, text: string): Promise { ), ).toBeVisible() } + +// commentOnSelection files a comment on the text already selected in the +// pane's editor and returns the id the server gave it. The popover is +// left open. +export async function commentOnSelection( + page: Page, + pane: Locator, + text: string, +): Promise { + await pane + .locator("[data-visible]") + .getByRole("button", { name: t("editor.bubble-menu.comment-label") }) + .click() + await submitNewComment(page, pane, text) + + return savedCommentId(pane, "[data-comment-id]") +} + +// commentOnBlock files a comment on a whole block through its block +// handle and returns the id the server gave it. The popover is left open. +export async function commentOnBlock( + page: Page, + pane: Locator, + block: Locator, + text: string, +): Promise { + await openBlockMenu(page, pane, block) + await page + .getByRole("menuitem", { + name: t("editor.drag-handle.options.add-node-comment"), + }) + .click() + await submitNewComment(page, pane, text) + + // the commented node and the highlight drawn over it both carry the + // id, so the highlight alone is read + return savedCommentId(pane, ".node-comment-overlay[data-node-comment-id]") +} + +// openTextComment clicks a text comment's highlight to open its thread. +// The thread opens on click and reads the editor's selection, which the +// editor takes from the browser's caret only once the selectionchange +// event has fired. A human click is slow enough for that; a synthetic +// one is not, so the button is held down until the editor has caught up. +// A highlight over changed text is several spans, so the one to click +// is named by the text it covers. +export async function openTextComment( + page: Page, + pane: Locator, + id: string, + text: string, +): Promise { + const span = pane.locator(`[data-comment-id="${id}"]`, { hasText: text }) + + await span.hover() + await page.mouse.down() + await selectionSettled(span) + await page.mouse.up() +} + +// commentedText is what a text comment highlights in the pane's editor, +// read across every span the highlight is split into. A highlight over +// changed text is split wherever the diff marks change. +export function commentedText(pane: Locator, id: string): Promise { + return pane + .locator(`[data-comment-id="${id}"]`) + .evaluateAll((spans) => spans.map((span) => span.textContent).join("")) +} + +async function submitNewComment( + page: Page, + pane: Locator, + text: string, +): Promise { + await paneCommentPopover(pane) + .locator('.ProseMirror[contenteditable="true"]') + .click() + await page.keyboard.type(text) + await paneCommentPopover(pane) + .getByRole("button", { + name: t("editor.comment-thread.comment-button"), + exact: true, + }) + .click() +} + +// savedCommentId waits until the pending highlight has been traded for +// the server's id — the proof the comment was saved — and returns that +// id. A highlight can be several elements; they all carry the one id. +async function savedCommentId( + pane: Locator, + selector: string, +): Promise { + await expect(pane.locator('[data-comment-id^="pending-"]')).toHaveCount(0) + await expect(pane.locator('[data-node-comment-id^="pending-"]')).toHaveCount( + 0, + ) + + const ids = await pane + .locator(selector) + .evaluateAll((elements) => [ + ...new Set( + elements.map( + (el) => + el.getAttribute("data-comment-id") ?? + el.getAttribute("data-node-comment-id"), + ), + ), + ]) + expect(ids).toHaveLength(1) + + return ids[0] ?? "" +} diff --git a/e2e/helpers/editor.ts b/e2e/helpers/editor.ts index 79fadf48..4a239778 100644 --- a/e2e/helpers/editor.ts +++ b/e2e/helpers/editor.ts @@ -13,6 +13,170 @@ export function contentEditor(page: Page): Locator { return page.locator(".content-editor .ProseMirror") } +// diffEditor is the merged read-only editor a draft shows while "show +// changes" is on. It is the one editor that hides its caret, which is +// also what tells it apart from the comment composer mounted in the same +// pane. +export function diffEditor(page: Page): Locator { + return page.locator(".diff-editor .ProseMirror.caret-transparent") +} + +// the panes wrapping each editor together with its block handle, bubble +// menu and comment popover. Anything the editor owns but renders outside +// its ProseMirror element is found through the pane. +export function contentPane(page: Page): Locator { + return page.locator(".content-editor") +} + +export function diffPane(page: Page): Locator { + return page.locator(".diff-editor") +} + +// placeCaret clicks just inside the left or right edge of a block, which +// maps to the first or last position of its line. The block is a +// full-width element, so its edge sits well past its text, and the click +// is retried until focus sticks: a late re-render of a freshly switched +// branch can swallow the first one. Home and End are not used because on +// macOS chromium binds them to the document, not the line. +export async function placeCaret( + block: Locator, + edge: "start" | "end", +): Promise { + await expect(async () => { + const box = await block.boundingBox() + expect(box).not.toBeNull() + + await block.click({ + position: { + x: edge === "start" ? 1 : (box?.width ?? 2) - 1, + y: (box?.height ?? 2) / 2, + }, + }) + await expect( + block.locator("xpath=ancestor::*[contains(@class, 'ProseMirror')]"), + ).toBeFocused() + }).toPass() + await selectionSettled(block) +} + +// selectText selects one run of a block's text with the keyboard, walking +// to it from the block's start. A mouse drag would depend on where each +// glyph is drawn. +export async function selectText( + page: Page, + block: Locator, + text: string, +): Promise { + const content = await editorText(block) + const start = content.indexOf(text) + if (start === -1) { + throw new Error(`"${text}" is not in the block reading "${content}"`) + } + + await placeCaret(block, "start") + await pressTimes(page, "ArrowRight", start) + await pressTimes(page, "Shift+ArrowRight", text.length) + await selectionSettled(block) +} + +// the slice of tiptap's editor, reachable from its element, that +// selectionSettled reads +interface EditorHandle { + state: { selection: { from: number; to: number } } + view: { posAtDOM(node: Node, offset: number): number } +} + +// selectionSettled waits until the editor's own selection matches the +// browser's. A click or an arrow key moves the browser's caret at once, +// but the editor only learns of it from the selectionchange event, which +// queues behind the input that caused it. A key the editor handles +// itself, such as Enter or Backspace, sent before then acts on the old +// selection. +export async function selectionSettled(block: Locator): Promise { + await expect + .poll(() => + block.evaluate((el) => { + const root = el.closest( + ".ProseMirror", + ) + const editor = root?.editor + const selection = document.getSelection() + if (!editor || !selection?.anchorNode || !selection.focusNode) { + return false + } + + try { + const anchor = editor.view.posAtDOM( + selection.anchorNode, + selection.anchorOffset, + ) + const focus = editor.view.posAtDOM( + selection.focusNode, + selection.focusOffset, + ) + const { from, to } = editor.state.selection + + return ( + Math.min(anchor, focus) === from && Math.max(anchor, focus) === to + ) + } catch { + return false + } + }), + ) + .toBe(true) +} + +async function pressTimes( + page: Page, + key: string, + times: number, +): Promise { + for (let pressed = 0; pressed < times; pressed += 1) { + await page.keyboard.press(key) + } +} + +// the heights, as fractions of a block, at which openBlockMenu hovers it +const HOVER_HEIGHTS = [0.5, 0.25, 0.75, 0.1, 0.9] + +// openBlockMenu opens the block handle's menu for a block. The handle +// slides in beside whichever block the pointer is over, so the block is +// hovered first, at more than one height if need be: the handle stays +// away while the pointer is level with an element that opts out of it, +// such as a parameter separator in the other column of a split block. +// On an editable branch the handle carries two triggers, hooks and block +// actions, told apart by the icon each one shows. +export async function openBlockMenu( + page: Page, + pane: Locator, + block: Locator, +): Promise { + const trigger = pane.locator( + '.z-drag-handle [data-slot="dropdown-menu-trigger"]', + { has: page.locator('[class*="dots-line"]') }, + ) + + let attempt = 0 + await expect(async () => { + const box = await block.boundingBox() + expect(box).not.toBeNull() + + const height = HOVER_HEIGHTS[attempt % HOVER_HEIGHTS.length] ?? 0.5 + attempt += 1 + await block.hover({ + position: { x: (box?.width ?? 2) / 2, y: (box?.height ?? 2) * height }, + }) + await expect(trigger).toBeVisible({ timeout: 1_000 }) + }).toPass() + await trigger.click() + + const menu = page.getByRole("menu") + await expect(menu).toBeVisible() + + return menu +} + // readModeToggle is the navbar button switching the page between read and // edit mode. Its accessible name is the mode the page is in. export function readModeToggle(page: Page): Locator { diff --git a/e2e/helpers/review.ts b/e2e/helpers/review.ts index b37c641f..54c8d8e2 100644 --- a/e2e/helpers/review.ts +++ b/e2e/helpers/review.ts @@ -1,5 +1,5 @@ import { expect, type Locator, type Page } from "@playwright/test" -import { contentEditor, waitForEditor } from "./editor" +import { contentEditor, diffEditor, waitForEditor } from "./editor" import { t } from "./i18n" // the two branches a reviewable page carries. They are the branch names @@ -69,6 +69,22 @@ export async function switchToBranch( } } +// showChanges turns the draft's "show changes" switch on and waits for the +// merged diff editor it mounts over the draft. The diff needs the main +// branch synced as well, so on a fresh page the editor takes a moment. +export async function showChanges(page: Page): Promise { + await page.locator("#show-diff").click() + await expect(diffEditor(page)).toBeVisible({ timeout: 15_000 }) +} + +// hideChanges turns the switch off again and waits for the draft editor +// to be back in view. +export async function hideChanges(page: Page): Promise { + await page.locator("#show-diff").click() + await expect(diffEditor(page)).toHaveCount(0) + await expect(contentEditor(page)).toBeVisible() +} + // reviewActions is the split button carrying the draft's approve and // merge actions. It only renders while the draft branch is open. export function reviewActions(page: Page): Locator { diff --git a/e2e/tests/review-comments.test.ts b/e2e/tests/review-comments.test.ts new file mode 100644 index 00000000..98a5caf7 --- /dev/null +++ b/e2e/tests/review-comments.test.ts @@ -0,0 +1,919 @@ +import { + expect, + test, + type APIRequestContext, + type Locator, + type Page, +} from "@playwright/test" +import { + commentedText, + commentOnBlock, + commentOnSelection, + openTextComment, + paneCommentPopover, +} from "../helpers/comments" +import { joinAsSecondUser } from "../helpers/collaboration" +import { + contentEditor, + contentPane, + createDocument, + diffEditor, + diffPane, + documentPersisted, + editorText, + openBlockMenu, + openSlashMenu, + placeCaret, + selectText, + waitForEditor, +} from "../helpers/editor" +import { t } from "../helpers/i18n" +import { visit } from "../helpers/page" +import { + hideChanges, + makeReviewable, + showChanges, + switchToBranch, +} from "../helpers/review" +import { signUpWithWorkspace } from "../helpers/workspace" + +test.describe("comments on draft changes", () => { + // every test builds a page, makes it reviewable, edits the draft and + // waits for the edits to be stored before the first assertion. slow() + // triples the budget for the whole flow. + test.beforeEach(() => { + test.slow() + }) + + test("files a comment on an added paragraph", async ({ page, request }) => { + await draftWithParagraphChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + addedParagraph(page), + "Is this paragraph needed?", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("Is this paragraph needed?"), + ).toBeVisible() + await expect( + diffEditor(page).locator(`p[data-node-comment-id="${id}"]`), + ).toHaveText("Added paragraph") + }) + + test("shows the added paragraph's comment on the draft once changes are hidden", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedParagraph(page), + "Reads well", + ) + + await hideChanges(page) + + await expect( + contentEditor(page).locator(`p[data-node-comment-id="${id}"]`), + ).toHaveText("Added paragraph") + + await showChanges(page) + + await expect( + diffEditor(page).locator(`p[data-node-comment-id="${id}"]`), + ).toHaveText("Added paragraph") + }) + + test("keeps the added paragraph's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedParagraph(page), + "Still here later", + ) + await documentPersisted(page) + + await reopenDraftChanges(page, url) + + await diffEditor(page).locator(`p[data-node-comment-id="${id}"]`).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Still here later"), + ).toBeVisible() + }) + + test("files a comment on a removed paragraph", async ({ page, request }) => { + await draftWithParagraphChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + removedParagraph(page), + "Why was this dropped?", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("Why was this dropped?"), + ).toBeVisible() + await expect( + diffEditor(page).locator(`p[data-node-comment-id="${id}"]`), + ).toHaveText("Doomed paragraph") + }) + + test("keeps the removed paragraph's comment when changes are hidden and shown again", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + removedParagraph(page), + "Bring it back", + ) + + await hideChanges(page) + await showChanges(page) + + await diffEditor(page).locator(`p[data-node-comment-id="${id}"]`).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Bring it back"), + ).toBeVisible() + }) + + test("keeps the removed paragraph's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + removedParagraph(page), + "Gone but discussed", + ) + + await reopenDraftChanges(page, url) + + await diffEditor(page).locator(`p[data-node-comment-id="${id}"]`).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Gone but discussed"), + ).toBeVisible() + }) + + test("files a comment on added text", async ({ page, request }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "delta") + + const id = await commentOnSelection(page, diffPane(page), "Prefer gamma") + + await expect( + paneCommentPopover(diffPane(page)).getByText("Prefer gamma"), + ).toBeVisible() + await expect.poll(() => commentedText(diffPane(page), id)).toBe("delta") + }) + + test("shows the added text's comment on the draft once changes are hidden", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "delta") + const id = await commentOnSelection(page, diffPane(page), "Typo?") + + await hideChanges(page) + + await expect.poll(() => commentedText(contentPane(page), id)).toBe("delta") + + await showChanges(page) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("delta") + }) + + test("keeps the added text's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "delta") + const id = await commentOnSelection(page, diffPane(page), "Keep delta") + await documentPersisted(page) + + await reopenDraftChanges(page, url) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("delta") + await openTextComment(page, diffPane(page), id, "delta") + await expect( + paneCommentPopover(diffPane(page)).getByText("Keep delta"), + ).toBeVisible() + }) + + test("files a comment on removed text", async ({ page, request }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "beta") + + const id = await commentOnSelection(page, diffPane(page), "Beta was fine") + + await expect( + paneCommentPopover(diffPane(page)).getByText("Beta was fine"), + ).toBeVisible() + await expect.poll(() => commentedText(diffPane(page), id)).toBe("beta") + }) + + test("keeps the removed text's comment when changes are hidden and shown again", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "beta") + const id = await commentOnSelection(page, diffPane(page), "Restore beta") + + await hideChanges(page) + await showChanges(page) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("beta") + }) + + test("keeps the removed text's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "beta") + const id = await commentOnSelection(page, diffPane(page), "Beta again") + + await reopenDraftChanges(page, url) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("beta") + await openTextComment(page, diffPane(page), id, "beta") + await expect( + paneCommentPopover(diffPane(page)).getByText("Beta again"), + ).toBeVisible() + }) + + test("files a comment spanning kept, removed and added text", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "alpha betadelta gamma") + + const id = await commentOnSelection(page, diffPane(page), "Whole line") + + await expect + .poll(() => commentedText(diffPane(page), id)) + .toBe("alpha betadelta gamma") + }) + + test("keeps the kept and added part of a spanning comment on the draft", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "alpha betadelta gamma") + const id = await commentOnSelection(page, diffPane(page), "Whole line") + + await hideChanges(page) + + await expect + .poll(() => commentedText(contentPane(page), id)) + .toBe("alpha delta gamma") + + // the removed part is drawn back in once the changes are shown again + await showChanges(page) + + await expect + .poll(() => commentedText(diffPane(page), id)) + .toBe("alpha betadelta gamma") + }) + + test("keeps a spanning comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + await selectText(page, modifiedParagraph(page), "alpha betadelta gamma") + const id = await commentOnSelection(page, diffPane(page), "Whole line") + await documentPersisted(page) + + await reopenDraftChanges(page, url) + + await expect + .poll(() => commentedText(diffPane(page), id)) + .toBe("alpha betadelta gamma") + }) + + test("files a comment on text inside a removed paragraph", async ({ + page, + request, + }) => { + await draftWithParagraphChanges(page, request) + await selectText(page, removedParagraph(page), "Doomed") + + const id = await commentOnSelection(page, diffPane(page), "Harsh word") + + await expect( + paneCommentPopover(diffPane(page)).getByText("Harsh word"), + ).toBeVisible() + await expect.poll(() => commentedText(diffPane(page), id)).toBe("Doomed") + }) + + test("keeps a comment on text inside a removed paragraph across a reload", async ({ + page, + request, + }) => { + const url = await draftWithParagraphChanges(page, request) + await selectText(page, removedParagraph(page), "Doomed") + const id = await commentOnSelection(page, diffPane(page), "Harsh word") + + await reopenDraftChanges(page, url) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("Doomed") + await openTextComment(page, diffPane(page), id, "Doomed") + await expect( + paneCommentPopover(diffPane(page)).getByText("Harsh word"), + ).toBeVisible() + }) + + test("files a comment on a parameter added to a split block", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + addedParameter(page), + "Document the default", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("Document the default"), + ).toBeVisible() + await expect(commentedParameter(diffEditor(page), id)).toContainText( + "limit", + ) + }) + + test("shows the added parameter's comment on the draft once changes are hidden", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedParameter(page), + "Reads well", + ) + + await hideChanges(page) + + await expect(commentedParameter(contentEditor(page), id)).toContainText( + "limit", + ) + + await showChanges(page) + + await expect(commentedParameter(diffEditor(page), id)).toContainText( + "limit", + ) + }) + + test("keeps the added parameter's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedParameter(page), + "Still here later", + ) + await documentPersisted(page) + + await reopenDraftChanges(page, url) + + await commentedParameter(diffEditor(page), id).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Still here later"), + ).toBeVisible() + }) + + test("files a comment on a parameter removed from a split block", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + removedParameter(page), + "Callers still send this", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("Callers still send this"), + ).toBeVisible() + await expect(commentedParameter(diffEditor(page), id)).toContainText( + "Identifier", + ) + }) + + test("keeps the removed parameter's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + removedParameter(page), + "Gone but discussed", + ) + + await reopenDraftChanges(page, url) + + await commentedParameter(diffEditor(page), id).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Gone but discussed"), + ).toBeVisible() + }) + + test("files a comment on a code block added to a split block", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + addedCodeBlock(page), + "Add a usage example", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("Add a usage example"), + ).toBeVisible() + await expect(commentedCodeBlock(diffEditor(page), id)).toContainText( + "Gamma", + ) + }) + + test("shows the added code block's comment on the draft once changes are hidden", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedCodeBlock(page), + "Looks right", + ) + + await hideChanges(page) + + await expect(commentedCodeBlock(contentEditor(page), id)).toContainText( + "Gamma", + ) + + await showChanges(page) + + await expect(commentedCodeBlock(diffEditor(page), id)).toContainText( + "Gamma", + ) + }) + + test("keeps the added code block's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + addedCodeBlock(page), + "Still here later", + ) + await documentPersisted(page) + + await reopenDraftChanges(page, url) + + await commentedCodeBlock(diffEditor(page), id).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Still here later"), + ).toBeVisible() + }) + + test("files a comment on a code block removed from a split block", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + + const id = await commentOnBlock( + page, + diffPane(page), + removedCodeBlock(page), + "This one was useful", + ) + + await expect( + paneCommentPopover(diffPane(page)).getByText("This one was useful"), + ).toBeVisible() + await expect(commentedCodeBlock(diffEditor(page), id)).toContainText("Beta") + }) + + test("keeps the removed code block's comment across a reload", async ({ + page, + request, + }) => { + const url = await draftWithSplitChanges(page, request) + const id = await commentOnBlock( + page, + diffPane(page), + removedCodeBlock(page), + "Gone but discussed", + ) + + await reopenDraftChanges(page, url) + + await commentedCodeBlock(diffEditor(page), id).click() + await expect( + paneCommentPopover(diffPane(page)).getByText("Gone but discussed"), + ).toBeVisible() + }) + + test("files a comment on the changed description of a split block", async ({ + page, + request, + }) => { + await draftWithSplitChanges(page, request) + await selectText(page, modifiedParagraph(page), "thingsstuff") + + const id = await commentOnSelection(page, diffPane(page), "Pick one word") + + await expect + .poll(() => commentedText(diffPane(page), id)) + .toBe("thingsstuff") + + await hideChanges(page) + + await expect.poll(() => commentedText(contentPane(page), id)).toBe("stuff") + + await showChanges(page) + + await expect + .poll(() => commentedText(diffPane(page), id)) + .toBe("thingsstuff") + }) + + test("shows a comment on added text to a teammate viewing the changes", async ({ + page, + request, + browser, + }) => { + await draftWithParagraphChanges(page, request) + const other = await joinAsSecondUser(browser, page, request) + await switchToBranch(other.page, "draft") + await showChanges(other.page) + await selectText(page, modifiedParagraph(page), "delta") + + const id = await commentOnSelection(page, diffPane(page), "Seen by both") + + await expect + .poll(() => commentedText(diffPane(other.page), id)) + .toBe("delta") + await openTextComment(other.page, diffPane(other.page), id, "delta") + await expect( + paneCommentPopover(diffPane(other.page)).getByText("Seen by both"), + ).toBeVisible() + + await other.context.close() + }) + + test("shows a comment on a removed paragraph to a teammate viewing the changes", async ({ + page, + request, + browser, + }) => { + await draftWithParagraphChanges(page, request) + const other = await joinAsSecondUser(browser, page, request) + await switchToBranch(other.page, "draft") + await showChanges(other.page) + + const id = await commentOnBlock( + page, + diffPane(page), + removedParagraph(page), + "Missed by the draft", + ) + + const commented = diffEditor(other.page).locator( + `p[data-node-comment-id="${id}"]`, + ) + await expect(commented).toHaveText("Doomed paragraph") + await commented.click() + await expect( + paneCommentPopover(diffPane(other.page)).getByText("Missed by the draft"), + ).toBeVisible() + + await other.context.close() + }) + + test("shows a teammate's comment on removed text to the owner", async ({ + page, + request, + browser, + }) => { + await draftWithParagraphChanges(page, request) + const other = await joinAsSecondUser(browser, page, request) + await switchToBranch(other.page, "draft") + await showChanges(other.page) + await selectText(other.page, modifiedParagraph(other.page), "beta") + + const id = await commentOnSelection( + other.page, + diffPane(other.page), + "Keep beta please", + ) + + await expect.poll(() => commentedText(diffPane(page), id)).toBe("beta") + await openTextComment(page, diffPane(page), id, "beta") + await expect( + paneCommentPopover(diffPane(page)).getByText("Keep beta please"), + ).toBeVisible() + + await other.context.close() + }) +}) + +// the paragraphs a draft changes, as the diff shows them. The merged +// document marks each block with the change it carries. +function addedParagraph(page: Page): Locator { + return diffEditor(page).locator('p[data-diff-status="added"]') +} + +function removedParagraph(page: Page): Locator { + return diffEditor(page).locator('p[data-diff-status="removed"]') +} + +function modifiedParagraph(page: Page): Locator { + return diffEditor(page).locator('p[data-diff-status="modified"]') +} + +// draftWithParagraphChanges builds a page of three paragraphs, makes it +// reviewable and gives the draft one change of each kind: a paragraph +// added after the first, "beta" replaced by "delta" in the second and the +// third removed. It leaves the draft open with the changes shown and +// returns the page's url. +async function draftWithParagraphChanges( + page: Page, + request: APIRequestContext, +): Promise { + await signUpWithWorkspace(page, request) + await createDocument(page) + const url = page.url() + await contentEditor(page).click() + await page.keyboard.type("First paragraph") + await page.keyboard.press("Enter") + await page.keyboard.type("alpha beta gamma") + await page.keyboard.press("Enter") + await page.keyboard.type("Doomed paragraph") + await documentPersisted(page) + await makeReviewable(page) + await switchToBranch(page, "draft") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("Doomed paragraph") + + await placeCaret(draftText(page, "First paragraph"), "end") + await page.keyboard.press("Enter") + await page.keyboard.type("Added paragraph") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("Added paragraph") + + await selectText(page, draftText(page, "alpha beta gamma"), "beta") + await page.keyboard.type("delta") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("alpha delta gamma") + + // the text goes first and the empty paragraph joins the one before it, + // which removes the block without touching its neighbour + await selectText( + page, + draftText(page, "Doomed paragraph"), + "Doomed paragraph", + ) + await page.keyboard.press("Backspace") + await page.keyboard.press("Backspace") + await expect + .poll(() => editorText(contentEditor(page))) + .not.toContain("Doomed") + + await documentPersisted(page) + await showChanges(page) + await expect(addedParagraph(page)).toHaveText("Added paragraph") + await expect(removedParagraph(page)).toHaveText("Doomed paragraph") + await expect(modifiedParagraph(page)).toHaveText("alpha betadelta gamma") + + return url +} + +// reopenDraftChanges reloads the page, which lands on the main branch, +// and opens the draft with its changes shown again. +async function reopenDraftChanges(page: Page, url: string): Promise { + await visit(page, url) + await waitForEditor(page) + await switchToBranch(page, "draft") + await showChanges(page) +} + +// the parts of the split documentation block the draft changes, as the +// diff shows them. A parameter and a code block are found by the text +// they hold, since the change is marked on the block or on its children. +function addedParameter(page: Page): Locator { + return diffEditor(page).locator( + '[data-type="split-documentation-parameter-list-item"]', + { hasText: "Page size" }, + ) +} + +function removedParameter(page: Page): Locator { + return diffEditor(page).locator( + '[data-type="split-documentation-parameter-list-item"]', + { hasText: "Identifier" }, + ) +} + +function addedCodeBlock(page: Page): Locator { + return diffEditor(page).locator('[data-type="titled-code-block"]', { + hasText: "Gamma", + }) +} + +function removedCodeBlock(page: Page): Locator { + return diffEditor(page).locator('[data-type="titled-code-block"]', { + hasText: "Beta", + }) +} + +function commentedParameter(editor: Locator, id: string): Locator { + return editor.locator( + `[data-type="split-documentation-parameter-list-item"][data-node-comment-id="${id}"]`, + ) +} + +function commentedCodeBlock(editor: Locator, id: string): Locator { + return editor.locator( + `[data-type="titled-code-block"][data-node-comment-id="${id}"]`, + ) +} + +// draftWithSplitChanges builds a page holding one split documentation +// block — a description, two parameters and two code blocks — makes it +// reviewable and changes every part on the draft: a parameter and a code +// block are added, a parameter and a code block removed, and a word of +// the description replaced. It leaves the draft open with the changes +// shown and returns the page's url. +async function draftWithSplitChanges( + page: Page, + request: APIRequestContext, +): Promise { + await signUpWithWorkspace(page, request) + await createDocument(page) + const url = page.url() + await contentEditor(page).click() + const menu = await openSlashMenu(page) + await page.keyboard.type("Split") + await expect(menu.getByRole("button")).toHaveCount(1) + await page.keyboard.press("Enter") + + // the block opens with the caret in its heading, and its description + // is the line below + await page.keyboard.type("Topic") + await page.keyboard.press("ArrowDown") + await page.keyboard.type("Describe things") + await bottomAction( + page, + t("editor.split-documentation.left-side-bottom-action-button"), + ).click() + await page.keyboard.type("Options") + await page.keyboard.press("ArrowDown") + await typeParameter(page, "id", "string", "Identifier") + await page.keyboard.press("Enter") + await typeParameter(page, "name", "string", "Display name") + await bottomAction( + page, + t("editor.split-documentation.right-side-bottom-action-buttons.add-code"), + ).click() + await page.keyboard.type("Beta") + await expect.poll(() => editorText(contentEditor(page))).toContain("Beta") + await documentPersisted(page) + await makeReviewable(page) + await switchToBranch(page, "draft") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("Display name") + + // Enter at the end of a parameter's description starts a new parameter + await placeCaret(draftText(page, "Display name"), "end") + await page.keyboard.press("Enter") + await typeParameter(page, "limit", "int", "Page size") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("Page size") + + // Backspace at the start of a parameter's name removes the parameter + await placeCaret(draftText(page, "id"), "start") + await page.keyboard.press("Backspace") + await expect + .poll(() => editorText(contentEditor(page))) + .not.toContain("Identifier") + + await selectText(page, draftText(page, "Describe things"), "things") + await page.keyboard.type("stuff") + await expect + .poll(() => editorText(contentEditor(page))) + .toContain("Describe stuff") + + await bottomAction( + page, + t("editor.split-documentation.right-side-bottom-action-buttons.add-code"), + ).click() + await page.keyboard.type("Gamma") + await expect.poll(() => editorText(contentEditor(page))).toContain("Gamma") + + await openBlockMenu(page, contentPane(page), draftText(page, "Beta")) + await page + .getByRole("menuitem", { + name: t("editor.drag-handle.options.delete-block"), + }) + .click() + await expect.poll(() => editorText(contentEditor(page))).not.toContain("Beta") + + await documentPersisted(page) + await showChanges(page) + await expect( + diffEditor(page).locator('[data-diff-status="added"]', { + hasText: "Page size", + }), + ).not.toHaveCount(0) + await expect( + diffEditor(page).locator('[data-diff-status="removed"]', { + hasText: "Identifier", + }), + ).not.toHaveCount(0) + await expect( + diffEditor(page).locator('[data-diff-status="added"]', { + hasText: "Gamma", + }), + ).not.toHaveCount(0) + await expect( + diffEditor(page).locator('[data-diff-status="removed"]', { + hasText: "Beta", + }), + ).not.toHaveCount(0) + await expect(modifiedParagraph(page)).toHaveText("Describe thingsstuff") + + return url +} + +// the split block's bottom actions, which add a parameter list on the +// left and a code block on the right +function bottomAction(page: Page, name: string): Locator { + return contentPane(page).getByRole("button", { name: name }) +} + +// typeParameter fills the parameter the caret has just been placed in: +// its name, then its type to the right, then its description below +async function typeParameter( + page: Page, + name: string, + type: string, + description: string, +): Promise { + await page.keyboard.type(name) + await page.keyboard.press("ArrowRight") + await page.keyboard.type(type) + await page.keyboard.press("ArrowDown") + await page.keyboard.type(description) +} + +function draftText(page: Page, text: string): Locator { + return contentEditor(page).getByText(text, { exact: true }) +} diff --git a/web/app/components/editor/comments/CommentRenderer.nuxt.test.ts b/web/app/components/editor/comments/CommentRenderer.nuxt.test.ts index b09fcb29..ce9e7656 100644 --- a/web/app/components/editor/comments/CommentRenderer.nuxt.test.ts +++ b/web/app/components/editor/comments/CommentRenderer.nuxt.test.ts @@ -9,11 +9,12 @@ import { import { setResponseStatus } from "h3" import { afterEach, beforeEach, describe, it, vi } from "vitest" import CommentRenderer from "./CommentRenderer.vue" -import { CommentMark } from "./comment-mark" +import { CommentMark, findCommentMarkById } from "./comment-mark" import { NodeComment } from "./node-comment-extension" import { CommentExtensions } from "./utils" import UniqueID from "../tiptap-utils/unique-id" import { stubThemeColorContext } from "../test-helpers/theme" +import { DiffStatus, type PositionMap } from "../diff/position-map" import { clearQueryCache, disposeMockEndpoints, @@ -65,8 +66,10 @@ const DOCUMENT_ID = makeXid("doc") const BRANCH_ID = makeXid("branch") const ME = makeXid("usme") const COMMENT_ID = makeXid("cmt") +const BLOCK_UID = "block-1" let contentEditor: Editor | null = null +let diffEditor: Editor | null = null // the renderer anchors comments in a live document, so the suite drives a // real editor holding the same mark and node comment extensions the app @@ -115,6 +118,83 @@ function serverComment(overrides: Record = {}) { } } +// a diff editor over the same content. Both editors carry the block under +// one uid, the way the diff view pairs merged blocks with their source. +function diffDocOf(source: Editor): Editor { + const element = document.createElement("div") + document.body.appendChild(element) + + diffEditor = new TiptapEditor({ + element: element, + extensions: [ + ...CommentExtensions, + UniqueID.configure({ types: ["paragraph"], attributeName: "uid" }), + CommentMark, + NodeComment.configure({ types: ["paragraph"] }), + ], + content: source.getJSON(), + }) + + for (const editor of [source, diffEditor]) { + const block = editor.state.doc.firstChild + editor.view.dispatch( + editor.state.tr.setNodeMarkup(0, undefined, { + ...block?.attrs, + uid: BLOCK_UID, + }), + ) + } + + return diffEditor +} + +// the first block, reported as added on the branch +function addedBlockMap(editor: Editor): PositionMap { + return [ + { + source: "modified", + blockIndex: 0, + startPos: 0, + nodeSize: editor.state.doc.firstChild?.nodeSize ?? 0, + diffStatus: DiffStatus.Added, + uid: BLOCK_UID, + }, + ] +} + +function mountDiffRenderer(content: Editor, diff: Editor) { + return mountSuspended(CommentRenderer, { + props: { + contentEditor: content, + container: null, + diffContext: { + diffEditor: diff, + positionMap: addedBlockMap(diff), + suppressNextRecompute: () => undefined, + }, + }, + }) +} + +// types a draft into the open thread and saves it against the stubbed +// server, which answers with COMMENT_ID +async function saveDraft(wrapper: VueWrapper) { + mockEndpoint("POST", `/api/documents/${DOCUMENT_ID}/comments`, () => + serverComment(), + ) + // saving refetches the thread list + mockEndpoint("GET", `/api/documents/${DOCUMENT_ID}/comments`, () => [ + serverComment(), + ]) + commentEditor(wrapper).commands.setContent(commentBody("looks good")) + await flushPromises() + + await popoverButton( + wrapper, + t("editor.comment-thread.comment-button"), + ).trigger("click") +} + function mountRenderer(editor: Editor) { return mountSuspended(CommentRenderer, { props: { contentEditor: editor, container: null }, @@ -198,6 +278,14 @@ describe("", { concurrent: false }, () => { element?.remove() contentEditor = null } + + if (diffEditor) { + const element = diffEditor.view.dom.parentElement + + diffEditor.destroy() + element?.remove() + diffEditor = null + } }) it("stays out of the way until a comment is opened", async ({ expect }) => { @@ -700,4 +788,51 @@ describe("", { concurrent: false }, () => { expect(calls.length).toBeGreaterThan(0) }, WAIT_FOR_OPTIONS) }) + // the content editor keeps its own renderer mounted, hidden, next to + // the diff one + describe("when the diff view is on", () => { + beforeEach(() => { + useEditorStore().setReviewableDiffActive(true) + }) + + it("keeps a block comment on added content in the branch", async ({ + expect, + }) => { + const content = textDoc("hello world") + const diff = diffDocOf(content) + await mountRenderer(content) + const wrapper = await mountDiffRenderer(content, diff) + await api(wrapper).addNewComment(0) + await nextTick() + + await saveDraft(wrapper) + + await vi.waitFor(() => { + expect(diff.state.doc.firstChild?.attrs.nodeCommentId).toBe(COMMENT_ID) + }, WAIT_FOR_OPTIONS) + expect(content.state.doc.firstChild?.attrs.nodeCommentId).toBe(COMMENT_ID) + }) + + it("keeps a text comment on added content in the branch", async ({ + expect, + }) => { + const content = textDoc("hello world") + const diff = diffDocOf(content) + diff.commands.setTextSelection({ from: 1, to: 6 }) + await mountRenderer(content) + const wrapper = await mountDiffRenderer(content, diff) + await api(wrapper).addNewComment("text-selection") + await nextTick() + + await saveDraft(wrapper) + + await vi.waitFor(() => { + expect(findCommentMarkById(diff.state, COMMENT_ID)).not.toBeNull() + }, WAIT_FOR_OPTIONS) + expect(findCommentMarkById(content.state, COMMENT_ID)).toMatchObject({ + from: 1, + to: 6, + }) + }) + }) }) diff --git a/web/app/components/editor/comments/CommentRenderer.vue b/web/app/components/editor/comments/CommentRenderer.vue index e720021c..3f077de7 100644 --- a/web/app/components/editor/comments/CommentRenderer.vue +++ b/web/app/components/editor/comments/CommentRenderer.vue @@ -352,6 +352,13 @@ watch( ) watchDeep([fetchComments.state, fetchComments.isPending], () => { + // the content editor's renderer stays mounted, hidden, while the diff + // view is on. The diff renderer then owns the drafts in the content + // editor, and removing them here loses comments on added content. + if (!isDiffMode.value && editorStore.reviewableDiffActive) { + return + } + if (isDiffMode.value && props.diffContext?.diffEditor) { deletePendingCommentData( props.diffContext.diffEditor,