diff --git a/packages/app/e2e/browser/composer-diff-stat.spec.ts b/packages/app/e2e/browser/composer-diff-stat.spec.ts index 455c58180a..00705faccd 100644 --- a/packages/app/e2e/browser/composer-diff-stat.spec.ts +++ b/packages/app/e2e/browser/composer-diff-stat.spec.ts @@ -1,7 +1,14 @@ +import type { Locator } from "@playwright/test"; import { writeFile } from "node:fs/promises"; import path from "node:path"; import { expect, test, type Page } from "../support/fixtures"; import { openAgentRoute, seedMockAgentWorkspace } from "../support/helpers/mock-agent"; +import { gateNextAgentMessage } from "../support/helpers/agent-message-gate"; +import { + fillComposerDraft, + expectComposerDraft, + attachFileFromMenu, +} from "../support/helpers/composer"; import { ensureExplorerSidebar, openFilesPanel } from "../support/helpers/workspace-tabs"; const APP_SETTINGS_KEY = "@paseo:app-settings"; @@ -26,14 +33,19 @@ async function openComposerDiff(page: Page) { await composerChangesPill(page).click(); } -async function seedChangedAgent(repoPrefix: string) { +async function seedChangedAgent( + repoPrefix: string, + featureValues?: Record, + files: Array<{ path: string; content: string }> = [], +) { const workspace = await seedMockAgentWorkspace({ repoPrefix, + featureValues, title: "Composer diff stat", repo: { withRemote: true, // Exclude the local remote before the daemon starts observing this checkout. - files: [{ path: ".gitignore", content: "/remote.git/\n" }], + files: [{ path: ".gitignore", content: "/remote.git/\n" }, ...files], }, }); try { @@ -156,3 +168,691 @@ test("composer diff stat reveals Changes, then opens the diff in the focused pan await workspace.cleanup(); } }); + +async function tapFirstAddedDiffLine( + page: Page, + surface: Locator = page.getByTestId("working-diff-panel"), +) { + const body = surface.getByTestId("diff-file-0-body"); + await expect(body).toBeVisible({ timeout: 30000 }); + const bounds = await body.boundingBox(); + if (!bounds) throw new Error("Expanded diff body has no bounds"); + const fontSize = await surface + .getByTestId("git-diff-canvas") + .evaluate((element) => Number.parseFloat(getComputedStyle(element).fontSize)); + await page.touchscreen.tap(bounds.x + 120, bounds.y + Math.round(fontSize * 1.5) * 2.5); +} + +async function openReviewDiff(page: Page, workspace: Awaited>) { + await page.setViewportSize({ width: 1400, height: 900 }); + await openAgentRoute(page, { workspaceId: workspace.workspaceId, agentId: workspace.agentId }); + await expect(composerChangesPill(page)).toBeVisible({ timeout: 30000 }); + if (await page.getByTestId("workspace-tab-working_diff").count()) { + await selectDiffTab(page); + } else { + await revealComposerChangesInExplorer(page); + await openComposerDiff(page); + } + await expect(page.getByTestId("diff-file-0-body")).toBeVisible(); +} + +async function saveReviewComment(page: Page, body: string) { + await page.getByRole("textbox", { name: "Review comment" }).fill(body); + await page.getByRole("button", { name: "Save review comment", exact: true }).click(); + await expect(page.getByRole("button", { name: "Bottom sheet backdrop" })).toHaveCount(0); +} + +async function cancelReviewComment(page: Page) { + await page.getByRole("button", { name: "Cancel review comment", exact: true }).click(); + await expect(page.getByRole("button", { name: "Bottom sheet backdrop" })).toHaveCount(0); +} + +test.describe("review comments", () => { + test.use({ hasTouch: true }); + + test("compact review comments open a sheet and preserve saved comments through save, edit, cancel and reload", async ({ + page, + }) => { + const workspace = await seedChangedAgent("compact-review-comment-"); + try { + await test.step("Open a compact sheet with reachable input and actions", async () => { + await openReviewDiff(page, workspace); + await showCompactLayout(page); + await tapFirstAddedDiffLine(page); + await expectCompactCommentSheet(page); + }); + await test.step("Save, discard an edit, then save an updated comment", async () => { + await saveReviewComment(page, " Please simplify this. "); + await expectSavedReview(page, "Please simplify this."); + await editSavedReview(page, "Please simplify this."); + await fillReviewComment(page, "Discard this edit"); + await cancelReviewComment(page); + await expectSavedReview(page, "Please simplify this."); + await editSavedReview(page, "Please simplify this."); + await saveReviewComment(page, "Updated comment"); + await expectSavedReview(page, "Updated comment"); + }); + await test.step("Discard a new comment and retain the saved one after reload", async () => { + await tapFirstAddedDiffLine(page); + await fillReviewComment(page, "Discard new comment"); + await cancelReviewComment(page); + await expectReviewAbsent(page, "Discard new comment"); + await reloadReview(page); + await expectSavedReview(page, "Updated comment"); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("wide review comments retain inline editing", async ({ page }) => { + const workspace = await seedChangedAgent("wide-review-comment-"); + try { + await test.step("Save and cancel an inline edit on a wide screen", async () => { + await openReviewDiff(page, workspace); + await tapFirstAddedDiffLine(page); + await expectInlineCommentEditor(page); + await saveReviewComment(page, "Wide comment"); + await expectSavedReview(page, "Wide comment"); + await editSavedReview(page, "Wide comment"); + await expectInlineCommentEditor(page); + await cancelReviewComment(page); + await expectSavedReview(page, "Wide comment"); + }); + await test.step("The existing composer still sends the review", async () => { + await sendReviewThroughComposer(page, workspace.agentId); + await selectDiffTab(page); + await expectReviewAbsent(page, "Wide comment"); + }); + } finally { + await workspace.cleanup(); + } + }); +}); + +async function sendReviewThroughComposer(page: Page, agentId: string) { + await page.getByTestId(`workspace-tab-agent_${agentId}`).click(); + await expect(page.getByTestId("composer-review-attachment-pill")).toBeVisible(); + await page.getByRole("textbox", { name: "Message agent..." }).fill("Address this review"); + await page.getByRole("button", { name: "Send message", exact: true }).click(); + await expect(page.getByText("Address this review", { exact: true }).first()).toBeVisible(); + await expect(page.getByTestId("composer-review-attachment-pill")).toHaveCount(0); +} + +async function addSavedReview( + page: Page, + body: string, + surface: Locator = page.getByTestId("working-diff-panel"), +) { + await tapFirstAddedDiffLine(page, surface); + await saveReviewComment(page, body); + await expectSavedReview(page, body, surface); +} + +async function expectSavedReview( + page: Page, + body: string, + surface: Locator = page.getByTestId("working-diff-panel"), +) { + await expect(surface.getByText(body, { exact: true })).toBeVisible(); +} + +async function editSavedReview(page: Page, expectedBody: string) { + await page + .getByTestId("working-diff-panel") + .getByRole("button", { name: "Edit review comment", exact: true }) + .click(); + await expect(page.getByRole("textbox", { name: "Review comment" })).toHaveValue(expectedBody); +} + +async function sendFeedback(page: Page, count: number) { + await page.getByRole("button", { name: `Send feedback (${count})`, exact: true }).click(); +} + +async function expectFeedbackSent(page: Page, recipient: string) { + await expect(page.getByText(`Feedback sent to ${recipient}`, { exact: true })).toBeVisible(); + await expect(page.getByTestId("working-diff-panel")).toBeVisible(); +} + +test.describe("send review feedback", () => { + test.use({ hasTouch: true }); + + test("compact feedback sends directly to the only workspace agent and preserves its composer draft", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-feedback-single-"); + try { + await test.step("Save a review without changing the chat draft", async () => { + await openReviewDiff(page, workspace); + await selectAgentTab(page, workspace.agentId); + await fillComposerDraft(page, "Keep this thought"); + await attachFileFromMenu(page, { + name: "unrelated.txt", + mimeType: "text/plain", + buffer: Buffer.from("Unrelated draft attachment"), + }); + await selectDiffTab(page); + await showCompactLayout(page); + await addSavedReview(page, "Please simplify this."); + }); + await test.step("Send directly and stay on the diff", async () => { + await sendFeedback(page, 1); + await expectFeedbackSent(page, "Composer diff stat"); + await expectReviewAbsent(page, "Please simplify this."); + await captureFeedback(page, "single"); + }); + await test.step("Return to the untouched composer draft", async () => { + await showWideLayout(page); + await selectAgentTab(page, workspace.agentId); + await expectComposerDraft(page, "Keep this thought"); + await expect(page.getByText("unrelated.txt", { exact: true })).toBeVisible(); + await expect(page.getByTestId("composer-review-attachment-pill")).toHaveCount(0); + }); + } finally { + await workspace.cleanup(); + } + }); + test("feedback pending ownership follows the draft across Diff and compact Changes", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-shared-pending-"); + const gate = await gateNextAgentMessage(page); + try { + await test.step("Send feedback from the Diff tab and hold its real request", async () => { + await openReviewDiff(page, workspace); + await showCompactLayout(page); + await addSavedReview(page, "One draft across surfaces"); + await sendFeedback(page, 1); + await gate.waitForRequest(); + }); + await test.step("The compact Changes surface observes the same pending operation", async () => { + await revealCompactChanges(page); + await expect( + compactReviewSurface(page).getByRole("button", { + name: "Sending feedback (1)", + exact: true, + }), + ).toBeDisabled(); + expect(gate.requestCount()).toBe(1); + gate.accept(); + await expect( + compactReviewSurface(page).getByText("Feedback sent to Composer diff stat", { + exact: true, + }), + ).toBeVisible(); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("an unsaved review survives presentation changes and acknowledgement before Save", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-editor-ownership-"); + const gate = await gateNextAgentMessage(page); + try { + await test.step("Carry unsaved inline text into the compact sheet and back", async () => { + await openReviewDiff(page, workspace); + await tapFirstAddedDiffLine(page); + await fillReviewComment(page, "Body across presentations"); + await showCompactLayout(page); + await expectReviewEditorBody(page, "Body across presentations"); + await showWideLayout(page); + await expectReviewEditorBody(page, "Body across presentations"); + await saveReviewComment(page, "Body across presentations"); + }); + await test.step("Keep an open edit saveable after acknowledgement removes its original", async () => { + await sendFeedback(page, 1); + await gate.waitForRequest(); + await editSavedReview(page, "Body across presentations"); + await fillReviewComment(page, "Keep this unsaved edit"); + gate.accept(); + await expectFeedbackSent(page, "Composer diff stat"); + await expectReviewEditorBody(page, "Keep this unsaved edit"); + await showCompactLayout(page); + await expectReviewEditorBody(page, "Keep this unsaved edit"); + await saveReviewComment(page, "Keep this unsaved edit"); + await expectSavedReview(page, "Keep this unsaved edit"); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("saved feedback preserves navigation across multiple diff files", async ({ page }) => { + const workspace = await seedMultiFileReview(); + try { + await test.step("Save compact feedback and retain both floating actions", async () => { + await openCompactReviewExplorer(page, workspace); + await addSavedReview(page, "Keep file navigation available", compactReviewSurface(page)); + await expectReviewNavigationReachable(page); + }); + await test.step("Jump to another file without covering the feedback action", async () => { + await jumpToReviewFile(page, "z-last.ts"); + await scrollReviewToLastLine(page); + await expectReviewNavigationReachable(page); + await captureFeedback(page, "navigation-compact"); + }); + await test.step("Wide file navigation and feedback remain reachable", async () => { + await openReviewDiff(page, workspace); + await navigateWideReviewFile(page, "README.md"); + await expectFeedbackReachable(page); + await expectSavedReview(page, "Keep file navigation available"); + await captureFeedback(page, "navigation-wide"); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("feedback sends only matching comments and preserves orphaned comments", async ({ + page, + }) => { + const workspace = await seedMultiFileReview(); + try { + await test.step("A saved comment whose diff disappears has no sendable context", async () => { + await openReviewDiff(page, workspace); + await showCompactLayout(page); + await addSavedReview(page, "Retain this orphan"); + await setReadmeDiff(workspace, false); + await expectUnmatchedFeedback(page); + }); + await test.step("Send the matching subset with an accurate count", async () => { + await addSavedReview(page, "Send this matching comment"); + await sendFeedback(page, 1); + await expectFeedbackSent(page, "Composer diff stat"); + await expectReviewAbsent(page, "Send this matching comment"); + await expectUnmatchedFeedback(page); + }); + await test.step("Restoring the missing diff reveals the unsent comment", async () => { + await setReadmeDiff(workspace, true); + await expectSavedReview(page, "Retain this orphan"); + await expectFeedbackReachable(page); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("zero open agent tabs retain feedback with a disabled reason", async ({ page }) => { + const workspace = await seedChangedAgent("review-feedback-zero-"); + try { + await test.step("Close the agent tab while keeping the review", async () => { + await openReviewDiff(page, workspace); + await addSavedReview(page, "Keep until an agent is opened"); + await closeAgentTab(page, workspace.agentId); + }); + await test.step("The disabled action explains why in wide and compact layouts", async () => { + await expectFeedbackUnavailable(page); + await captureFeedback(page, "zero-wide"); + await showCompactLayout(page); + await expectFeedbackUnavailable(page); + await expectSavedReview(page, "Keep until an agent is opened"); + await captureFeedback(page, "zero-compact"); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("only open tabs in this workspace appear in compact and wide recipient menus", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-feedback-multiple-"); + const other = await seedChangedAgent("review-feedback-other-"); + try { + const second = await createReviewRecipient(workspace, "Second reviewer"); + const historical = await createReviewRecipient(workspace, "Closed historical agent"); + await test.step("Open two local tabs and a tab in another workspace", async () => { + await openReviewDiff(page, other); + await addSavedReview(page, "Other workspace review"); + await openReviewDiff(page, workspace); + await closeAgentTab(page, historical.id); + await openAgentRoute(page, { workspaceId: workspace.workspaceId, agentId: second.id }); + await selectDiffTab(page); + await addSavedReview(page, "Review for second agent"); + }); + await test.step("The wide menu includes only local open agents", async () => { + await captureWideFeedbackAppearance(page); + }); + await test.step("Escape returns keyboard focus to feedback so the chooser can reopen", async () => { + await openFeedbackWithKeyboard(page); + await dismissRecipientMenu(page); + await expectFeedbackFocused(page); + await reopenFeedbackWithKeyboard(page); + await dismissRecipientMenu(page); + await expectFeedbackFocused(page); + }); + await test.step("Choose the second agent in the compact menu and stay on the diff", async () => { + await showCompactLayout(page); + await captureFeedback(page, "cta-compact"); + await sendFeedback(page, 1); + await expectRecipientMenu(page); + await captureFeedback(page, "menu-compact"); + await chooseFeedbackRecipient(page, "Second reviewer"); + await expectFeedbackSent(page, "Second reviewer"); + await expectReviewAbsent(page, "Review for second agent"); + }); + await test.step("The other workspace's saved review remains", async () => { + await openReviewDiff(page, other); + await expectSavedReview(page, "Other workspace review"); + }); + } finally { + await workspace.cleanup(); + await other.cleanup(); + } + }); + + test("failed feedback remains editable and retries through the real provider", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-feedback-retry-", { mockPromptRejections: 1 }); + try { + await test.step("A provider rejection retains the saved review", async () => { + await openReviewDiff(page, workspace); + await showCompactLayout(page); + await addSavedReview(page, "Initial feedback"); + await sendFeedback(page, 1); + await expectFeedbackFailure(page); + await expectSavedReview(page, "Initial feedback"); + await captureFeedback(page, "failure"); + }); + await test.step("Edit the retained feedback and retry successfully", async () => { + await editSavedReview(page, "Initial feedback"); + await saveReviewComment(page, "Corrected feedback"); + await sendFeedback(page, 1); + await expectFeedbackSent(page, "Composer diff stat"); + await expectReviewAbsent(page, "Corrected feedback"); + }); + } finally { + await workspace.cleanup(); + } + }); + + test("pending feedback blocks repeat submission and preserves edits and additions", async ({ + page, + }) => { + const workspace = await seedChangedAgent("review-feedback-pending-"); + const gate = await gateNextAgentMessage(page); + try { + await test.step("Hold the real request while feedback is pending", async () => { + await openReviewDiff(page, workspace); + await showCompactLayout(page); + await addSavedReview(page, "Sent version"); + await sendFeedback(page, 1); + const request = await gate.waitForRequest(); + expect(request.agentId).toBe(workspace.agentId); + expect(request.text).toBe("Please address this code review."); + expect(request.attachments).toMatchObject([ + { + type: "review", + cwd: workspace.cwd, + comments: [ + { + filePath: "README.md", + lineNumber: 2, + body: "Sent version", + context: { targetLine: { content: "export const one = 1;", newLineNumber: 2 } }, + }, + ], + }, + ]); + await expectFeedbackPending(page); + }); + await test.step("Save an edit and another comment before acknowledgement", async () => { + await editSavedReview(page, "Sent version"); + await saveReviewComment(page, "Edited during send"); + await addSavedReview(page, "Added during send"); + await expectFeedbackPending(page); + expect(gate.requestCount()).toBe(1); + await captureFeedback(page, "pending"); + }); + await test.step("A real acknowledgement preserves both new versions", async () => { + gate.accept(); + await expectFeedbackSent(page, "Composer diff stat"); + await expectSavedReview(page, "Edited during send"); + await expectSavedReview(page, "Added during send"); + await expectReviewAbsent(page, "Sent version"); + }); + } finally { + await workspace.cleanup(); + } + }); +}); + +async function showCompactLayout(page: Page) { + await page.setViewportSize({ width: 390, height: 844 }); +} +async function showWideLayout(page: Page) { + await page.setViewportSize({ width: 1400, height: 900 }); +} +async function selectAgentTab(page: Page, agentId: string) { + await page.getByTestId(`workspace-tab-agent_${agentId}`).click({ position: { x: 12, y: 13 } }); +} +async function selectDiffTab(page: Page) { + await page.getByTestId("workspace-tab-working_diff").click({ position: { x: 12, y: 13 } }); +} +async function fillReviewComment(page: Page, body: string) { + await page.getByRole("textbox", { name: "Review comment" }).fill(body); +} +async function expectReviewAbsent(page: Page, body: string) { + await expect(page.getByTestId("working-diff-panel").getByText(body, { exact: true })).toHaveCount( + 0, + ); +} +async function reloadReview(page: Page) { + await page.reload(); + await expect(page.getByTestId("working-diff-panel")).toBeVisible({ timeout: 30000 }); +} +async function expectInlineCommentEditor(page: Page) { + await expect(page.getByTestId("inline-review-editor")).toBeVisible(); + await expect(page.getByTestId("review-comment-sheet")).toHaveCount(0); +} +async function expectCompactCommentSheet(page: Page) { + await expect(page.getByTestId("review-comment-sheet")).toBeVisible(); + await expect(page.getByText("README.md · +2", { exact: true })).toBeVisible(); + await expect(page.getByRole("textbox", { name: "Review comment" })).toBeInViewport({ ratio: 1 }); + for (const action of ["Save", "Cancel"]) + await expect( + page.getByRole("button", { name: `${action} review comment`, exact: true }), + ).toBeInViewport({ ratio: 1 }); + await expect(page.getByTestId("inline-review-editor")).toHaveCount(0); + await expect( + page.getByRole("button", { name: "Save review comment", exact: true }), + ).toBeDisabled(); +} +function feedbackButton(page: Page) { + return page.getByRole("button", { name: "Send feedback (1)", exact: true }); +} +async function openFeedbackWithKeyboard(page: Page) { + await feedbackButton(page).focus(); + await expectFeedbackFocused(page); + await reopenFeedbackWithKeyboard(page); +} +async function reopenFeedbackWithKeyboard(page: Page) { + await page.keyboard.press("Enter"); + await expectRecipientMenu(page); + const recipient = page.getByRole("menuitem", { name: "Composer diff stat", exact: true }); + await recipient.focus(); + await expect(recipient).toBeFocused(); +} +async function expectFeedbackFocused(page: Page) { + await expect(feedbackButton(page)).toBeFocused(); +} + +async function captureWideFeedbackAppearance(page: Page) { + for (const colorScheme of ["light", "dark"] as const) { + await page.emulateMedia({ colorScheme }); + await expectSavedReview(page, "Review for second agent"); + await captureFeedback(page, `cta-wide-${colorScheme}-closed`); + await sendFeedback(page, 1); + await expectRecipientMenu(page); + await captureFeedback(page, `cta-wide-${colorScheme}-open`); + await dismissRecipientMenu(page); + } + await page.emulateMedia({ colorScheme: "light" }); +} + +async function captureFeedback(page: Page, name: string) { + await page.screenshot({ path: `/tmp/phase2-feedback-${name}.png` }); +} + +async function closeAgentTab(page: Page, agentId: string) { + await page.getByTestId(`workspace-tab-agent_${agentId}`).click({ button: "right" }); + await page.getByTestId(`workspace-tab-context-agent_${agentId}-close`).click(); + await expect(page.getByTestId(`workspace-tab-agent_${agentId}`)).toHaveCount(0); +} +async function expectFeedbackUnavailable(page: Page) { + await expect(page.getByRole("button", { name: "Send feedback (1)", exact: true })).toBeDisabled(); + await expect( + page.getByText("Open an agent tab in this workspace to send feedback.", { exact: true }), + ).toBeVisible(); +} +async function createReviewRecipient( + workspace: Awaited>, + title: string, +) { + return workspace.client.createAgent({ + provider: "mock", + cwd: workspace.cwd, + workspaceId: workspace.workspaceId, + title, + modeId: "load-test", + model: "e2e-fast-stream", + }); +} +async function expectRecipientMenu(page: Page) { + for (const name of ["Composer diff stat", "Second reviewer"]) { + await expect(page.getByRole("menuitem", { name, exact: true })).toBeInViewport({ ratio: 1 }); + } + await expect(page.getByRole("menuitem")).toHaveCount(2); +} +async function dismissRecipientMenu(page: Page) { + await page.keyboard.press("Escape"); + await expect(page.getByRole("menuitem")).toHaveCount(0); +} +async function chooseFeedbackRecipient(page: Page, name: string) { + await page.getByRole("menuitem", { name, exact: true }).click(); +} +async function expectFeedbackFailure(page: Page) { + await expect( + page.getByText("Requested mock prompt rejection", { exact: true }).first(), + ).toBeVisible(); + await expect(page.getByRole("button", { name: "Send feedback (1)", exact: true })).toBeEnabled(); +} +async function expectFeedbackPending(page: Page, count = 1) { + await expect( + page.getByRole("button", { name: `Sending feedback (${count})`, exact: true }), + ).toBeDisabled(); +} + +async function seedMultiFileReview() { + const workspace = await seedChangedAgent("review-feedback-navigation-", undefined, [ + { path: "z-last.ts", content: "export const final = 0;\n" }, + ]); + await writeFile( + path.join(workspace.cwd, "z-last.ts"), + Array.from({ length: 120 }, (_, index) => `export const value${index} = ${index};`).join("\n") + + "\n", + ); + await workspace.client.checkoutRefresh(workspace.cwd); + return workspace; +} +async function expectFeedbackReachable(page: Page) { + await expect(page.getByRole("button", { name: "Send feedback (1)", exact: true })).toBeInViewport( + { ratio: 1 }, + ); +} +async function expectReviewNavigationReachable(page: Page) { + await expectFeedbackReachable(page); + const jump = page.getByRole("button", { name: "Jump to file", exact: true }); + await expect(jump).toBeInViewport({ ratio: 1 }); + const navigationBounds = await jump.boundingBox(); + const feedbackBounds = await page + .getByRole("button", { name: "Send feedback (1)", exact: true }) + .boundingBox(); + if (!navigationBounds || !feedbackBounds) throw new Error("Review actions have no bounds"); + expect(feedbackBounds.y + feedbackBounds.height).toBeLessThanOrEqual(navigationBounds.y); +} +async function jumpToReviewFile(page: Page, name: string) { + await page.getByRole("button", { name: "Jump to file", exact: true }).click(); + const sheet = page.getByTestId("changes-jump-to-file-sheet"); + await page + .getByTestId("changes-file-tree") + .filter({ visible: true }) + .getByText(name, { exact: true }) + .click(); + await expect(sheet).toHaveCount(0); + await expect(reviewFileHeader(compactReviewSurface(page), name)).toBeInViewport(); +} +async function navigateWideReviewFile(page: Page, name: string) { + const explorer = await ensureExplorerSidebar(page); + await explorer.getByTestId("changes-tree-panel").getByText(name, { exact: true }).click(); + await expect(reviewFileHeader(page.getByTestId("working-diff-panel"), name)).toBeInViewport(); +} + +async function setReadmeDiff( + workspace: Awaited>, + changed: boolean, +) { + await writeFile( + path.join(workspace.cwd, "README.md"), + changed ? "# Temp Repo\nexport const one = 1;\nexport const two = 2;\n" : "# Temp Repo\n", + ); + await workspace.client.checkoutRefresh(workspace.cwd); +} +async function expectUnmatchedFeedback(page: Page) { + await expect(page.getByRole("button", { name: "Send feedback (0)", exact: true })).toBeDisabled(); + await expect( + page.getByText("Saved comments no longer match this diff.", { exact: true }), + ).toBeVisible(); +} + +function compactReviewSurface(page: Page) { + return page.getByTestId("explorer-content-area").filter({ visible: true }); +} +async function openCompactReviewExplorer( + page: Page, + workspace: Awaited>, +) { + await showWideLayout(page); + await openAgentRoute(page, { workspaceId: workspace.workspaceId, agentId: workspace.agentId }); + await selectAgentTab(page, workspace.agentId); + await showCompactLayout(page); + await composerChangesPill(page).click(); + await expect(compactReviewSurface(page).getByTestId("diff-file-0-body")).toBeVisible(); +} + +function reviewFileHeader(surface: Locator, name: string) { + return surface.locator(`[data-diff-header-path="${name}"]`).getByTestId(/^diff-file-\d+$/); +} + +async function scrollReviewToLastLine(page: Page) { + const surface = compactReviewSurface(page); + const bounds = await surface.boundingBox(); + if (!bounds) throw new Error("Compact review has no bounds"); + await page.mouse.move(bounds.x + 120, bounds.y + 200); + await page.mouse.wheel(0, 10000); + await expect + .poll(async () => { + const body = await surface.getByTestId("diff-file-1-body").boundingBox(); + const feedback = await page + .getByRole("button", { name: "Send feedback (1)", exact: true }) + .boundingBox(); + if (!body || !feedback) return false; + return body.y + body.height <= feedback.y; + }) + .toBe(true); +} + +async function revealCompactChanges(page: Page) { + await page.getByTestId("workspace-explorer-toggle").first().click(); + await page.getByTestId("explorer-header").getByText("Changes", { exact: true }).click(); + await expect(compactReviewSurface(page).getByTestId("diff-file-0-body")).toBeVisible(); +} +async function expectReviewEditorBody(page: Page, body: string) { + await expect(page.getByRole("textbox", { name: "Review comment", exact: true })).toHaveValue( + body, + ); +} diff --git a/packages/app/e2e/support/helpers/agent-message-gate.ts b/packages/app/e2e/support/helpers/agent-message-gate.ts index 7444fee796..9f5735d5ac 100644 --- a/packages/app/e2e/support/helpers/agent-message-gate.ts +++ b/packages/app/e2e/support/helpers/agent-message-gate.ts @@ -6,6 +6,8 @@ type WebSocketMessage = string | Buffer; interface SendAgentMessageRequest { type: "send_agent_message_request"; requestId: string; + text: unknown; + attachments: unknown; agentId: string; /** Absent when the client sends into an idle agent. */ activeTurnBehavior?: string; @@ -29,6 +31,8 @@ function readSendRequest(message: WebSocketMessage): SendAgentMessageRequest | n return { type: "send_agent_message_request", requestId: request.requestId, + text: request.text, + attachments: request.attachments, agentId: request.agentId, activeTurnBehavior: typeof request.activeTurnBehavior === "string" ? request.activeTurnBehavior : undefined, @@ -74,6 +78,7 @@ export async function gateNextAgentMessage(page: Page) { return { waitForRequest, + requestCount: () => requests.length, accept(index = 0) { const heldMessage = heldMessages[index]; if (!serverSocket || !heldMessage) { diff --git a/packages/app/src/components/ui/button.tsx b/packages/app/src/components/ui/button.tsx index e160970223..707ce60648 100644 --- a/packages/app/src/components/ui/button.tsx +++ b/packages/app/src/components/ui/button.tsx @@ -174,6 +174,7 @@ export function Button({ ...props }: PropsWithChildren< Omit & { + ref?: React.Ref; variant?: ButtonVariant; size?: ButtonSize; leftIcon?: LeftIcon; diff --git a/packages/app/src/components/ui/floating-action-button.tsx b/packages/app/src/components/ui/floating-action-button.tsx index 33d72985fe..2b06e097f2 100644 --- a/packages/app/src/components/ui/floating-action-button.tsx +++ b/packages/app/src/components/ui/floating-action-button.tsx @@ -1,47 +1,67 @@ +import { forwardRef, useMemo } from "react"; +import { Text, type View } from "react-native"; import type { LucideIcon } from "lucide-react-native"; import { StyleSheet } from "react-native-unistyles"; import { PressHighlight } from "@/components/ui/press-highlight"; -import { ICON_SIZE, SPACING } from "@/styles/theme"; +import { ICON_SIZE } from "@/styles/theme"; -const FLOATING_ACTION_BUTTON_SIZE = 56; -const FLOATING_ACTION_BUTTON_INSET = SPACING[4]; - -/** Space a scrolling surface keeps below its content so the button traps nothing. */ -export const FLOATING_ACTION_BUTTON_CLEARANCE = - FLOATING_ACTION_BUTTON_SIZE + FLOATING_ACTION_BUTTON_INSET; +import { + FLOATING_ACTION_BUTTON_SIZE, + FLOATING_ACTION_BUTTON_INSET, +} from "./floating-action-layout"; +export { + FLOATING_ACTION_BUTTON_CLEARANCE, + floatingActionsClearance, +} from "./floating-action-layout"; export interface FloatingActionButtonProps { icon: LucideIcon; - /** Names the action; the button carries no label. */ + /** Names the action for assistive technology. */ accessibilityLabel: string; onPress: () => void; + label?: string; + disabled?: boolean; + bottomInset?: number; testID?: string; } /** - * The one floating action a scrolling surface can offer. It pins itself to the - * bottom-right of its nearest positioned ancestor, so the ancestor decides what - * the button may cover. + * Pins an action to the bottom-right of its nearest positioned ancestor. + * The surface reserves scroll clearance and uses bottomInset to separate stacked actions. */ -export function FloatingActionButton({ - icon: Icon, - accessibilityLabel, - onPress, - testID, -}: FloatingActionButtonProps) { - return ( - - - - ); -} +export const FloatingActionButton = forwardRef( + function FloatingActionButton( + { icon: Icon, accessibilityLabel, onPress, label, disabled = false, bottomInset = 0, testID }, + ref, + ) { + const accessibilityState = useMemo(() => ({ disabled }), [disabled]); + const buttonStyle = useMemo( + () => [ + styles.button, + label && styles.labeled, + disabled && styles.disabled, + { bottom: FLOATING_ACTION_BUTTON_INSET + bottomInset }, + ], + [bottomInset, disabled, label], + ); + return ( + + + {label ? {label} : null} + + ); + }, +); const styles = StyleSheet.create((theme) => ({ button: { @@ -56,6 +76,15 @@ const styles = StyleSheet.create((theme) => ({ backgroundColor: theme.colors.surface3, ...theme.shadow.md, }, + labeled: { + width: "auto", + minWidth: FLOATING_ACTION_BUTTON_SIZE, + paddingHorizontal: theme.spacing[4], + flexDirection: "row", + gap: theme.spacing[2], + }, + label: { color: theme.colors.foreground, fontSize: theme.fontSize.base }, + disabled: { opacity: theme.opacity[50] }, pressed: { backgroundColor: theme.colors.interactionHighlight, }, diff --git a/packages/app/src/components/ui/floating-action-layout.test.ts b/packages/app/src/components/ui/floating-action-layout.test.ts new file mode 100644 index 0000000000..2256f4ef9b --- /dev/null +++ b/packages/app/src/components/ui/floating-action-layout.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; +import { + floatingActionsClearance, + FLOATING_ACTION_BUTTON_CLEARANCE, +} from "./floating-action-layout"; + +describe("floating action scroll clearance", () => { + it("keeps the last diff line above Jump to file before and after feedback is added", () => { + const jump = FLOATING_ACTION_BUTTON_CLEARANCE; + const feedback = FLOATING_ACTION_BUTTON_CLEARANCE + 44; + expect(floatingActionsClearance([jump, 0], 34)).toBe(106); + expect(floatingActionsClearance([jump, feedback], 34)).toBe(222); + expect(floatingActionsClearance([0, feedback], 34)).toBe(150); + expect(floatingActionsClearance([jump, feedback], 0)).toBe(188); + expect(floatingActionsClearance([0, 0], 34)).toBe(0); + }); +}); diff --git a/packages/app/src/components/ui/floating-action-layout.ts b/packages/app/src/components/ui/floating-action-layout.ts new file mode 100644 index 0000000000..c54fd62dbe --- /dev/null +++ b/packages/app/src/components/ui/floating-action-layout.ts @@ -0,0 +1,15 @@ +import { SPACING } from "@/styles/theme"; + +export const FLOATING_ACTION_BUTTON_SIZE = 56; +export const FLOATING_ACTION_BUTTON_INSET = SPACING[4]; +export const FLOATING_ACTION_BUTTON_CLEARANCE = + FLOATING_ACTION_BUTTON_SIZE + FLOATING_ACTION_BUTTON_INSET; + +/** Reserve the stacked actions and the safe-area inset once for their scrolling surface. */ +export function floatingActionsClearance( + actionClearances: readonly number[], + bottomInset: number, +): number { + const clearance = actionClearances.reduce((total, action) => total + action, 0); + return clearance > 0 ? clearance + bottomInset : 0; +} diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts index 89ecce2b1e..60b386bd45 100644 --- a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts +++ b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts @@ -245,3 +245,12 @@ describe("bottom sheet visibility tracker", () => { expect(context.backPress.press()).toBe(false); }); }); + +it("does not cancel the owner's editor when the compact sheet leaves the tree on resize", () => { + const context = setup(); + openSheet(context); + context.tracker.attachController(null); + context.tracker.handleSheetDismiss(); + expect(context.closeCount()).toBe(0); + expect(context.backPress.press()).toBe(false); +}); diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts index 54ee6f58cd..dec2dd535c 100644 --- a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts +++ b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts @@ -121,6 +121,9 @@ export function createBottomSheetVisibilityTracker(opts: { } }, handleSheetDismiss() { + // Portal cleanup reports dismissal after the sheet leaves the tree. It is + // presentation teardown, so the owner keeps its desired state. + if (!controller) return; if (visible) { setPhase("dismissing"); notifyClose(); diff --git a/packages/app/src/git/diff-document/hit-testing.test.ts b/packages/app/src/git/diff-document/hit-testing.test.ts index a0ed9c1578..d728c2f2d7 100644 --- a/packages/app/src/git/diff-document/hit-testing.test.ts +++ b/packages/app/src/git/diff-document/hit-testing.test.ts @@ -315,6 +315,7 @@ function reviewActionsForFirstAddition(): NonNullable< commentsByTarget: new Map(), editor: { target, commentId: null, body: "" }, onStartComment() {}, + onChangeEditorBody() {}, onCancelEditor() {}, onSaveEditor() {}, onEditComment() {}, diff --git a/packages/app/src/git/diff-document/model.test.ts b/packages/app/src/git/diff-document/model.test.ts index c2b6d86c9c..ace965c1ce 100644 --- a/packages/app/src/git/diff-document/model.test.ts +++ b/packages/app/src/git/diff-document/model.test.ts @@ -389,6 +389,7 @@ describe("diff document model", () => { commentId: null, }, onStartComment() {}, + onChangeEditorBody() {}, onCancelEditor() {}, onSaveEditor() {}, onEditComment() {}, @@ -790,6 +791,7 @@ function reviewActionsWithEditor( commentsByTarget: new Map(), editor: target ? { target, body: "", commentId: null } : null, onStartComment() {}, + onChangeEditorBody() {}, onCancelEditor() {}, onSaveEditor() {}, onEditComment() {}, diff --git a/packages/app/src/git/diff-pane.tsx b/packages/app/src/git/diff-pane.tsx index 139387ccb1..6bafd3eee3 100644 --- a/packages/app/src/git/diff-pane.tsx +++ b/packages/app/src/git/diff-pane.tsx @@ -1,3 +1,5 @@ +import { useSafeAreaInsets } from "react-native-safe-area-context"; +import { floatingActionsClearance } from "@/components/ui/floating-action-button"; import { useState, useCallback, useMemo, type ReactElement, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import type { TFunction } from "i18next"; @@ -31,6 +33,7 @@ import { import { type ParsedDiffFile } from "@/git/use-diff-query"; import type { ChangesState } from "@/panels/changes/state"; import { defaultChangesState } from "@/panels/changes/state"; +import { ReviewCommentSheet, ReviewFeedbackAction } from "@/review"; import { DiffDocument, type WorkingDiffMode } from "@/git/diff-document"; import { ChangedFilesTree } from "@/git/changed-files-tree"; import { JUMP_TO_FILE_CLEARANCE, JumpToFile } from "@/git/jump-to-file"; @@ -1548,7 +1551,9 @@ export function ChangesSurface({ isDiffLoading, reviewActions, reviewAttachment, + reviewFeedback, } = useWorkingDiff({ + presentation, serverId, workspaceId: workspaceId ?? undefined, cwd, @@ -1729,6 +1734,7 @@ export function ChangesSurface({ ], ); + const { bottom: bottomInset } = useSafeAreaInsets(); const hasChanges = files.length > 0; const jumpToFileInset = jumpToFileClearance({ isCompact: isMobile, @@ -1778,27 +1784,41 @@ export function ChangesSurface({ }); const diffContent: ReactElement = ( - - + + + + - + ); const bodyContent = ( + + void; +}) { + if (enabled === false || !editor) return null; + return ( + + ); +} + +function OpenReviewCommentSheet({ + editor, + reviewActions, + onChangeBody, +}: { + editor: InlineReviewEditorState; + reviewActions: InlineReviewActions; + onChangeBody: (body: string) => void; +}) { + const { t } = useTranslation(); + const context = `${editor.target.filePath} · ${editor.target.side === "old" ? "−" : "+"}${editor.target.lineNumber}`; + const header = useMemo( + () => ({ + title: t("review.comment.label"), + subtitle: {context}, + }), + [context, t], + ); + const { onSaveEditor, onCancelEditor } = reviewActions; + const handleSave = useCallback(() => onSaveEditor(editor.body), [onSaveEditor, editor.body]); + const canSave = editor.body.trim().length > 0; + const footer = useMemo( + () => ( + + + + + ), + [canSave, handleSave, onCancelEditor, t], + ); + return ( + + + + ); +} + +const COMMENT_SNAP_POINTS = ["50%", "90%"]; + +const styles = StyleSheet.create((theme) => ({ + context: { color: theme.colors.foregroundMuted, fontSize: theme.fontSize.sm }, + input: { flex: 1, minHeight: 0, textAlignVertical: "top" }, + actions: { flex: 1, flexDirection: "row", justifyContent: "flex-end", gap: theme.spacing[2] }, +})); diff --git a/packages/app/src/review/editor/index.test.ts b/packages/app/src/review/editor/index.test.ts new file mode 100644 index 0000000000..6735adef34 --- /dev/null +++ b/packages/app/src/review/editor/index.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it } from "vitest"; +import { createReviewEditor, createReviewEditorScope, type ReviewCommentWriter } from "./index"; +import type { ReviewDraftComment } from "../state"; +import type { ReviewableDiffTarget } from "@/utils/diff-layout"; + +const target: ReviewableDiffTarget = { + key: "src/example.ts:new:2", + filePath: "src/example.ts", + side: "new", + lineNumber: 2, + hunkHeader: "@@ -1,2 +1,2 @@", + hunkIndex: 0, + lineIndex: 2, + oldLineNumber: null, + newLineNumber: 2, + lineType: "add", + content: "const value = next;", +}; + +function memoryWriter() { + const drafts = new Map(); + let sequence = 0; + const writer: ReviewCommentWriter = { + saveComment: ({ key, id, comment }) => { + const current = drafts.get(key) ?? []; + if (id && current.some((saved) => saved.id === id)) { + drafts.set( + key, + current.map((saved) => (saved.id === id ? { ...saved, ...comment } : saved)), + ); + } else { + const saved = { + ...comment, + id: String(++sequence), + createdAt: "2026-10-09T00:00:00Z", + updatedAt: "2026-10-09T00:00:00Z", + }; + drafts.set(key, [...current, saved]); + } + }, + deleteComment: ({ key, id }) => { + drafts.set( + key, + (drafts.get(key) ?? []).filter((comment) => comment.id !== id), + ); + }, + }; + return { writer, comments: (key: string) => drafts.get(key) ?? [] }; +} + +describe("review editor", () => { + it("saves trimmed comments, edits the same record, and cancels without altering saved comments", () => { + const storage = memoryWriter(); + const editor = createReviewEditor("first", storage.writer); + editor.start(target); + editor.setBody(" First comment "); + editor.save(editor.getState()!.body); + expect(editor.getState()).toBeNull(); + const saved = storage.comments("first")[0]!; + expect(saved).toMatchObject({ + filePath: target.filePath, + side: "new", + lineNumber: 2, + body: "First comment", + }); + editor.edit(target, saved); + editor.setBody("Discarded edit"); + editor.cancel(); + expect(storage.comments("first")).toEqual([saved]); + editor.edit(target, saved); + editor.save(" Updated comment "); + expect(storage.comments("first")).toEqual([{ ...saved, body: "Updated comment" }]); + editor.start(target); + editor.setBody("Discarded new comment"); + editor.cancel(); + expect(storage.comments("first")).toHaveLength(1); + }); + + it("keeps an open edit saveable after its submitted record is acknowledged", () => { + const storage = memoryWriter(); + const editor = createReviewEditor("first", storage.writer); + editor.start(target); + editor.save("Submitted body"); + const saved = storage.comments("first")[0]!; + editor.edit(target, saved); + editor.setBody("Typed before acknowledgement"); + storage.writer.deleteComment({ key: "first", id: saved.id }); + expect(editor.getState()!.body).toBe("Typed before acknowledgement"); + editor.save(editor.getState()!.body); + expect(storage.comments("first")).toMatchObject([ + { + body: "Typed before acknowledgement", + filePath: target.filePath, + lineNumber: target.lineNumber, + }, + ]); + }); + + it("rejects empty saves and repeated saves after the editor closes", () => { + const storage = memoryWriter(); + const editor = createReviewEditor("first", storage.writer); + editor.start(target); + editor.save(" \n "); + expect(editor.getState()).toEqual({ target, commentId: null, body: "" }); + expect(storage.comments("first")).toEqual([]); + editor.save("Saved once"); + editor.save("Duplicate"); + expect(storage.comments("first").map((comment) => comment.body)).toEqual(["Saved once"]); + }); + + it("isolates draft ownership and closes an editor when its saved comment is deleted", () => { + const storage = memoryWriter(); + const first = createReviewEditor("first", storage.writer); + const second = createReviewEditor("second", storage.writer); + first.start(target); + first.save("First workspace"); + const saved = storage.comments("first")[0]!; + first.edit(target, saved); + expect(second.getState()).toBeNull(); + second.start(target); + second.save("Second workspace"); + first.delete(saved.id); + expect(first.getState()).toBeNull(); + expect(storage.comments("first")).toEqual([]); + expect(storage.comments("second").map((comment) => comment.body)).toEqual(["Second workspace"]); + }); +}); + +it("retains an open editor through remounts without exposing it on another surface or draft", async () => { + const storage = memoryWriter(); + const scope = createReviewEditorScope(storage.writer); + const editor = scope.forDraft("first", "diff"); + const unsubscribe = editor.subscribe(() => {}); + editor.start(target); + editor.setBody("Unsaved wide body"); + unsubscribe(); + await Promise.resolve(); + const compact = scope.forDraft("first", "diff"); + expect(compact.getState()!.body).toBe("Unsaved wide body"); + expect(scope.forDraft("first", "combined").getState()).toBeNull(); + expect(scope.forDraft("second", "diff").getState()).toBeNull(); + compact.save(compact.getState()!.body); + expect(storage.comments("first")[0]!.body).toBe("Unsaved wide body"); + expect(scope.forDraft("first", "diff").getState()).toBeNull(); +}); diff --git a/packages/app/src/review/editor/index.ts b/packages/app/src/review/editor/index.ts new file mode 100644 index 0000000000..6a7741eceb --- /dev/null +++ b/packages/app/src/review/editor/index.ts @@ -0,0 +1,84 @@ +import type { ReviewableDiffTarget } from "@/utils/diff-layout"; +import type { InlineReviewEditorState } from "../geometry"; +import type { ReviewDraftComment } from "../state"; + +export interface ReviewCommentWriter { + saveComment: (input: { + key: string; + id: string | null; + comment: Pick; + }) => void; + deleteComment: (input: { key: string; id: string }) => void; +} + +/** One editor belongs to one review draft; replacing the draft creates a fresh editor. */ +export function createReviewEditor(key: string, writer: ReviewCommentWriter) { + return buildReviewEditor(key, writer, () => {}); +} + +function buildReviewEditor(key: string, writer: ReviewCommentWriter, onUnused: () => void) { + let editor: InlineReviewEditorState | null = null; + const listeners = new Set<() => void>(); + function publish(next: InlineReviewEditorState | null) { + editor = next; + listeners.forEach((listener) => listener()); + if (!editor && listeners.size === 0) onUnused(); + } + return { + getState: () => editor, + subscribe: (listener: () => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + queueMicrotask(() => { + if (!editor && listeners.size === 0) onUnused(); + }); + }; + }, + start: (target: ReviewableDiffTarget) => publish({ target, commentId: null, body: "" }), + edit: (target: ReviewableDiffTarget, comment: ReviewDraftComment) => + publish({ target, commentId: comment.id, body: comment.body }), + setBody: (body: string) => { + if (editor) publish({ ...editor, body }); + }, + cancel: () => publish(null), + save: (body: string) => { + const trimmedBody = body.trim(); + if (!editor || !trimmedBody) return; + writer.saveComment({ + key, + id: editor.commentId, + comment: { + filePath: editor.target.filePath, + side: editor.target.side, + lineNumber: editor.target.lineNumber, + body: trimmedBody, + }, + }); + publish(null); + }, + delete: (id: string) => { + writer.deleteComment({ key, id }); + if (editor?.commentId === id) publish(null); + }, + }; +} + +/** An open draft survives presentation remounts; each presentation owns its editor for that draft. */ +export function createReviewEditorScope(writer: ReviewCommentWriter) { + const editors = new Map>(); + return { + forDraft(key: string, presentation: string) { + const owner = JSON.stringify([key, presentation]); + let model = editors.get(owner); + if (!model) { + const created = buildReviewEditor(key, writer, () => { + if (editors.get(owner) === created) editors.delete(owner); + }); + model = created; + editors.set(owner, model); + } + return model; + }, + }; +} diff --git a/packages/app/src/review/feedback/index.test.ts b/packages/app/src/review/feedback/index.test.ts new file mode 100644 index 0000000000..b423855b75 --- /dev/null +++ b/packages/app/src/review/feedback/index.test.ts @@ -0,0 +1,272 @@ +import { describe, expect, it } from "vitest"; +import { + createReviewFeedback, + createReviewFeedbackScope, + type ReviewFeedbackPorts, + type ReviewFeedbackSnapshot, + type FeedbackAttachment, + type FeedbackRecipient, +} from "./index"; +import type { ReviewDraftComment } from "../state"; + +function makeComment(id: string, body = id): ReviewDraftComment { + return { + id, + body, + filePath: "sample.ts", + side: "new", + lineNumber: 2, + createdAt: "2026-10-09T00:00:00Z", + updatedAt: "2026-10-09T00:00:00Z", + }; +} +function makeRecipient(agentId: string): FeedbackRecipient { + return { + agentId, + tab: { + key: `agent_${agentId}`, + tabId: `agent_${agentId}`, + kind: "agent", + target: { kind: "agent", agentId }, + }, + }; +} +function makeAttachment(comments: readonly ReviewDraftComment[]): FeedbackAttachment | null { + if (!comments.length) return null; + const line = { + type: "add" as const, + content: "const value = 1", + oldLineNumber: null, + newLineNumber: 2, + }; + return { + kind: "review", + reviewDraftKey: "draft", + commentCount: comments.length, + attachment: { + type: "review", + mimeType: "application/paseo-review", + cwd: "/workspace", + mode: "uncommitted", + baseRef: null, + comments: comments.map((comment) => ({ + filePath: comment.filePath, + side: comment.side, + lineNumber: comment.lineNumber, + body: comment.body, + context: { hunkHeader: "@@ -1 +1 @@", targetLine: line, lines: [line] }, + })), + }, + }; +} +function setup(agentIds: string[], unavailableIds: string[] = [], create = createReviewFeedback) { + let comments = [makeComment("first")]; + let recipients = agentIds.map(makeRecipient); + let connected = true; + let rejectSend: ((error: Error) => void) | null = null; + let resolveSend: (() => void) | null = null; + const sends: Array<{ recipient: FeedbackRecipient; attachment: FeedbackAttachment }> = []; + const read = (): ReviewFeedbackSnapshot => ({ + comments, + recipients, + connected, + attachment: makeAttachment(comments.filter((comment) => !unavailableIds.includes(comment.id))), + }); + const ports: ReviewFeedbackPorts = { + read, + send: (recipient, attachment) => { + sends.push({ recipient, attachment }); + return new Promise((resolve, reject) => { + resolveSend = resolve; + rejectSend = reject; + }); + }, + clearSent: (sent) => { + comments = comments.filter( + (current) => + !sent.some( + (prior) => + prior.id === current.id && + prior.updatedAt === current.updatedAt && + prior.body === current.body, + ), + ); + }, + failedMessage: () => "Failed to send", + }; + const model = create(ports); + return { + model, + ports, + sends, + comments: () => comments, + setComments: (next: ReviewDraftComment[]) => { + comments = next; + model.refresh(); + }, + setRecipients: (ids: string[]) => { + recipients = ids.map(makeRecipient); + model.refresh(); + }, + disconnect: () => { + connected = false; + model.refresh(); + }, + succeed: () => resolveSend!(), + fail: () => rejectSend!(new Error("Host rejected feedback")), + }; +} + +describe("review feedback", () => { + it("counts only attached comments, freezes the sending count, and retains orphans", async () => { + const feedback = setup(["one"], ["orphan"]); + feedback.setComments([makeComment("first"), makeComment("orphan")]); + expect(feedback.model.getState()).toMatchObject({ commentCount: 2, sendableCommentCount: 1 }); + const pending = feedback.model.press(); + feedback.setComments([makeComment("first"), makeComment("orphan"), makeComment("added")]); + expect(feedback.model.getState()).toMatchObject({ + phase: { kind: "sending", commentCount: 1 }, + sendableCommentCount: 2, + }); + expect(feedback.sends[0]!.attachment.commentCount).toBe(1); + feedback.succeed(); + await pending; + expect(feedback.comments()).toEqual([makeComment("orphan"), makeComment("added")]); + }); + it("retains an entirely orphaned draft without claiming any comments are sendable", async () => { + const feedback = setup(["one"], ["first"]); + expect(feedback.model.getState()).toMatchObject({ + commentCount: 1, + sendableCommentCount: 0, + disabledReason: "no-context", + canSend: false, + }); + await feedback.model.press(); + expect(feedback.sends).toEqual([]); + expect(feedback.comments()).toEqual([makeComment("first")]); + }); + it("retains comments and explains why no workspace agent can receive them", async () => { + const feedback = setup([]); + await feedback.model.press(); + expect(feedback.model.getState()).toMatchObject({ + canSend: false, + disabledReason: "no-agents", + commentCount: 1, + }); + expect(feedback.sends).toEqual([]); + expect(feedback.comments()).toEqual([makeComment("first")]); + }); + it("sends once to the only recipient and clears only sent versions", async () => { + const feedback = setup(["one"]); + const pending = feedback.model.press(); + await feedback.model.press(); + await feedback.model.send("one"); + expect(feedback.sends).toEqual([ + { recipient: makeRecipient("one"), attachment: makeAttachment([makeComment("first")]) }, + ]); + feedback.setComments([makeComment("first", "Edited during send"), makeComment("added")]); + feedback.succeed(); + await pending; + expect(feedback.comments()).toEqual([ + makeComment("first", "Edited during send"), + makeComment("added"), + ]); + expect(feedback.model.getState().phase).toEqual({ + kind: "sent", + recipient: makeRecipient("one"), + }); + }); + it("clears unchanged sent comments while preserving additions", async () => { + const feedback = setup(["one"]); + const pending = feedback.model.press(); + feedback.setComments([makeComment("first"), makeComment("added")]); + feedback.succeed(); + await pending; + expect(feedback.comments()).toEqual([makeComment("added")]); + }); + it("asks for a recipient when several tabs exist and validates the selection again", async () => { + const feedback = setup(["one", "two"]); + await feedback.model.press(); + expect(feedback.model.getState().phase).toEqual({ kind: "choosing" }); + expect(feedback.sends).toEqual([]); + feedback.setRecipients(["one"]); + await feedback.model.send("two"); + expect(feedback.sends).toEqual([]); + const pending = feedback.model.send("one"); + feedback.succeed(); + await pending; + expect(feedback.comments()).toEqual([]); + }); + it("retains comments on failure and retries using the latest saved version", async () => { + const feedback = setup(["one"]); + const failed = feedback.model.press(); + feedback.fail(); + await failed; + expect(feedback.model.getState().phase).toEqual({ + kind: "failed", + message: "Host rejected feedback", + }); + expect(feedback.comments()).toEqual([makeComment("first")]); + feedback.setComments([makeComment("first", "Corrected before retry")]); + const retried = feedback.model.press(); + feedback.succeed(); + await retried; + expect(feedback.sends).toHaveLength(2); + expect(feedback.sends[1]!.attachment.attachment.comments[0]!.body).toBe( + "Corrected before retry", + ); + expect(feedback.comments()).toEqual([]); + }); + it("keeps workspace drafts independent and prevents disconnected sends", async () => { + const first = setup(["one"]); + const second = setup(["other"]); + first.disconnect(); + await first.model.press(); + expect(first.model.getState().disabledReason).toBe("disconnected"); + const pending = second.model.press(); + second.succeed(); + await pending; + expect(first.comments()).toEqual([makeComment("first")]); + expect(first.sends).toEqual([]); + expect(second.comments()).toEqual([]); + }); +}); + +describe("shared review submission ownership", () => { + it("blocks another surface and reconnects to the pending draft while other drafts remain usable", async () => { + const scope = createReviewFeedbackScope(); + const first = setup(["one"], [], (ports) => scope.connect("workspace-one", ports)); + const second = scope.connect("workspace-one", first.ports); + const other = setup(["other"], [], (ports) => scope.connect("workspace-other", ports)); + const unsubscribe = first.model.subscribe(() => {}); + const pending = first.model.press(); + unsubscribe(); + await Promise.resolve(); + const reconnected = scope.connect("workspace-one", first.ports); + expect(second.getState()).toMatchObject({ canSend: false, phase: { kind: "sending" } }); + await second.press(); + await reconnected.send("one"); + expect(first.sends).toHaveLength(1); + const otherPending = other.model.press(); + expect(other.sends).toHaveLength(1); + first.fail(); + await pending; + expect(second.getState().phase).toEqual({ kind: "failed", message: "Host rejected feedback" }); + const retry = second.press(); + first.succeed(); + await retry; + expect(first.sends).toHaveLength(2); + other.succeed(); + await otherPending; + }); + it("opens only the active surface's recipient chooser", async () => { + const scope = createReviewFeedbackScope(); + const first = setup(["one", "two"], [], (ports) => scope.connect("same", ports)); + const second = scope.connect("same", first.ports); + await first.model.press(); + expect(first.model.getState().phase).toEqual({ kind: "choosing" }); + expect(second.getState().phase).toEqual({ kind: "idle" }); + first.model.closeMenu(); + expect(first.model.getState().phase).toEqual({ kind: "idle" }); + }); +}); diff --git a/packages/app/src/review/feedback/index.ts b/packages/app/src/review/feedback/index.ts new file mode 100644 index 0000000000..69e0592fe8 --- /dev/null +++ b/packages/app/src/review/feedback/index.ts @@ -0,0 +1,177 @@ +import type { ComposerAttachment } from "@/attachments/types"; +import type { ReviewDraftComment } from "../state"; +import type { WorkspaceTabDescriptor } from "@/screens/workspace/workspace-tabs-types"; + +export interface FeedbackRecipient { + agentId: string; + tab: WorkspaceTabDescriptor; +} +export type FeedbackAttachment = Extract; +export interface ReviewFeedbackSnapshot { + comments: readonly ReviewDraftComment[]; + attachment: FeedbackAttachment | null; + recipients: readonly FeedbackRecipient[]; + connected: boolean; +} +export interface ReviewFeedbackPorts { + read: () => ReviewFeedbackSnapshot; + send: (recipient: FeedbackRecipient, attachment: FeedbackAttachment) => Promise; + clearSent: (comments: readonly ReviewDraftComment[]) => void; + failedMessage: () => string; +} +export type FeedbackPhase = + | { kind: "idle" } + | { kind: "choosing" } + | { kind: "sending"; recipient: FeedbackRecipient; commentCount: number } + | { kind: "failed"; message: string } + | { kind: "sent"; recipient: FeedbackRecipient }; +export interface ReviewFeedbackState { + phase: FeedbackPhase; + commentCount: number; + sendableCommentCount: number; + recipients: readonly FeedbackRecipient[]; + disabledReason: "no-agents" | "disconnected" | "no-context" | null; + canSend: boolean; +} + +export function createReviewFeedback(ports: ReviewFeedbackPorts) { + return createFeedbackView(ports, createFeedbackWorkflow()); +} + +/** Surfaces share submission ownership while keeping their chooser and live inputs local. */ +export function createReviewFeedbackScope() { + const workflows = new Map>(); + return { + connect(key: string, ports: ReviewFeedbackPorts) { + let workflow = workflows.get(key); + if (!workflow) { + const created = createFeedbackWorkflow(() => { + if (workflows.get(key) === created) workflows.delete(key); + }); + workflow = created; + workflows.set(key, workflow); + } + return createFeedbackView(ports, workflow); + }, + }; +} + +function createFeedbackWorkflow(onUnused: () => void = () => {}) { + let phase: FeedbackPhase = { kind: "idle" }; + const observers = new Set<() => void>(); + function releaseIfUnused() { + queueMicrotask(() => { + if (observers.size === 0 && phase.kind !== "sending") onUnused(); + }); + } + return { + getPhase: () => phase, + changePhase(next: FeedbackPhase) { + phase = next; + observers.forEach((observer) => observer()); + releaseIfUnused(); + }, + subscribe(observer: () => void) { + observers.add(observer); + return () => { + observers.delete(observer); + releaseIfUnused(); + }; + }, + }; +} + +function createFeedbackView( + ports: ReviewFeedbackPorts, + workflow: ReturnType, +) { + let choosing = false; + const choosingPhase: FeedbackPhase = { kind: "choosing" }; + const getPhase = () => (choosing ? choosingPhase : workflow.getPhase()); + let state = deriveState(ports.read(), getPhase()); + const listeners = new Set<() => void>(); + function publish() { + state = deriveState(ports.read(), getPhase()); + listeners.forEach((listener) => listener()); + } + async function send(agentId: string) { + const snapshot = ports.read(); + const current = deriveState(snapshot, workflow.getPhase()); + const recipient = snapshot.recipients.find((candidate) => candidate.agentId === agentId); + if (!current.canSend || !recipient || !snapshot.attachment) return; + const attachment = snapshot.attachment; + const sentComments = snapshot.comments.filter((comment) => + attachment.attachment.comments.some( + (sent) => + sent.filePath === comment.filePath && + sent.side === comment.side && + sent.lineNumber === comment.lineNumber && + sent.body === comment.body, + ), + ); + choosing = false; + workflow.changePhase({ kind: "sending", recipient, commentCount: attachment.commentCount }); + publish(); + try { + await ports.send(recipient, attachment); + ports.clearSent(sentComments); + workflow.changePhase({ kind: "sent", recipient }); + } catch (error) { + workflow.changePhase({ + kind: "failed", + message: error instanceof Error ? error.message : ports.failedMessage(), + }); + } + publish(); + } + return { + getState: () => { + if (state.phase !== getPhase()) state = deriveState(ports.read(), getPhase()); + return state; + }, + subscribe: (listener: () => void) => { + listeners.add(listener); + const unsubscribe = workflow.subscribe(() => { + choosing = false; + publish(); + }); + return () => { + listeners.delete(listener); + unsubscribe(); + }; + }, + refresh: publish, + press: async () => { + const snapshot = ports.read(); + if (!deriveState(snapshot, workflow.getPhase()).canSend) return; + if (snapshot.recipients.length === 1) { + await send(snapshot.recipients[0]!.agentId); + } else { + choosing = true; + publish(); + } + }, + closeMenu: () => { + if (choosing) { + choosing = false; + publish(); + } + }, + send, + }; +} + +function deriveState(snapshot: ReviewFeedbackSnapshot, phase: FeedbackPhase): ReviewFeedbackState { + let disabledReason: ReviewFeedbackState["disabledReason"] = null; + if (snapshot.recipients.length === 0) disabledReason = "no-agents"; + else if (!snapshot.connected) disabledReason = "disconnected"; + else if (!snapshot.attachment) disabledReason = "no-context"; + return { + phase, + commentCount: snapshot.comments.length, + sendableCommentCount: snapshot.attachment?.commentCount ?? 0, + recipients: snapshot.recipients, + disabledReason, + canSend: snapshot.comments.length > 0 && disabledReason === null && phase.kind !== "sending", + }; +} diff --git a/packages/app/src/review/feedback/surface.tsx b/packages/app/src/review/feedback/surface.tsx new file mode 100644 index 0000000000..7f87f46d88 --- /dev/null +++ b/packages/app/src/review/feedback/surface.tsx @@ -0,0 +1,284 @@ +import { useCallback, useMemo, type ReactNode } from "react"; +import { Text, View } from "react-native"; +import { Send } from "lucide-react-native"; +import { useTranslation } from "react-i18next"; +import { useSafeAreaInsets } from "react-native-safe-area-context"; +import { StyleSheet } from "react-native-unistyles"; +import { DropdownMenu, DropdownMenuContent, DropdownMenuItem } from "@/components/ui/dropdown-menu"; +import { useMenuContext } from "@/components/ui/menu"; +import { Button } from "@/components/ui/button"; +import { buttonControlHeight } from "@/components/ui/control-geometry"; +import { useIsCompactFormFactor } from "@/constants/layout"; +import { SPACING } from "@/styles/theme"; +import { FloatingActionButton } from "@/components/ui/floating-action-button"; +import { + WorkspaceTabPresentationResolver, + WorkspaceTabIcon, + type WorkspaceTabPresentation, +} from "@/screens/workspace/workspace-tab-presentation"; +import type { FeedbackRecipient, ReviewFeedbackState, createReviewFeedback } from "./index"; + +type FeedbackModel = ReturnType; +interface ReviewFeedbackActionProps { + bottomOffset: number; + model: FeedbackModel; + state: ReviewFeedbackState; + serverId: string; + workspaceId: string; +} + +export function ReviewFeedbackAction({ + enabled, + hasDiff, + ...props +}: Omit & { + workspaceId?: string | null; + enabled: boolean | undefined; + hasDiff: boolean; +}) { + if (enabled === false || !hasDiff) return null; + return ; +} + +function OpenReviewFeedbackAction({ + model, + state, + serverId, + workspaceId, + bottomOffset, +}: ReviewFeedbackActionProps) { + const { t } = useTranslation(); + const { bottom } = useSafeAreaInsets(); + const isCompact = useIsCompactFormFactor(); + const onOpenChange = useCallback( + (open: boolean) => { + if (!open) model.closeMenu(); + }, + [model], + ); + const noteStyle = useMemo( + () => [ + styles.note, + !isCompact && styles.centered, + { + bottom: + bottom + + bottomOffset + + (isCompact ? 88 : buttonControlHeight.sm + SPACING[4] + SPACING[2]), + }, + ], + [bottom, bottomOffset, isCompact], + ); + const sending = state.phase.kind === "sending"; + const label = t(sending ? "review.feedback.sending" : "review.feedback.send", { + count: state.phase.kind === "sending" ? state.phase.commentCount : state.sendableCommentCount, + }); + const unavailableNotice = + state.disabledReason && state.commentCount > 0 ? ( + {t(`review.feedback.${state.disabledReason}`)} + ) : null; + let notice: ReactNode = unavailableNotice; + if (state.phase.kind === "failed") + notice = {state.phase.message}; + else if (state.phase.kind === "sent") + notice = ( + <> + + {unavailableNotice} + + ); + return ( + <> + {state.commentCount > 0 ? ( + + + + {state.recipients.map((recipient) => ( + + ))} + + + ) : null} + {notice ? ( + + {notice} + + ) : null} + + ); +} + +function FeedbackTrigger({ + model, + label, + disabled, + bottomInset, +}: { + model: FeedbackModel; + label: string; + disabled: boolean; + bottomInset: number; +}) { + const { triggerRef } = useMenuContext("FeedbackTrigger"); + const isCompact = useIsCompactFormFactor(); + const press = useCallback(() => { + void model.press(); + }, [model]); + if (!isCompact) { + return ( + + + + ); + } + return ( + + ); +} + +function RecipientOption({ + recipient, + model, + serverId, + workspaceId, +}: { + recipient: FeedbackRecipient; + model: FeedbackModel; + serverId: string; + workspaceId: string; +}) { + const choose = useCallback(() => { + void model.send(recipient.agentId); + }, [model, recipient.agentId]); + const render = useCallback( + (presentation: WorkspaceTabPresentation) => ( + + ), + [choose], + ); + return ( + + {render} + + ); +} + +function RecipientRow({ + presentation, + onSelect, +}: { + presentation: WorkspaceTabPresentation; + onSelect: () => void; +}) { + const leading = useMemo( + () => , + [presentation], + ); + return ( + + {presentation.label} + + ); +} + +function SentNotice({ + recipient, + serverId, + workspaceId, +}: { + recipient: FeedbackRecipient; + serverId: string; + workspaceId: string; +}) { + const { t } = useTranslation(); + const render = useCallback( + (presentation: WorkspaceTabPresentation) => ( + + {t("review.feedback.sent", { recipient: presentation.label })} + + ), + [t], + ); + return ( + + {render} + + ); +} + +const styles = StyleSheet.create((theme) => ({ + wideAction: { position: "absolute", left: 0, right: 0, alignItems: "center" }, + centered: { alignItems: "center" }, + note: { + position: "absolute", + right: theme.spacing[4], + left: theme.spacing[4], + alignItems: "flex-end", + }, + text: { + color: theme.colors.foreground, + fontSize: theme.fontSize.sm, + backgroundColor: theme.colors.surface1, + padding: theme.spacing[2], + borderRadius: theme.borderRadius.md, + }, + error: { + color: theme.colors.destructive, + fontSize: theme.fontSize.sm, + backgroundColor: theme.colors.surface1, + padding: theme.spacing[2], + borderRadius: theme.borderRadius.md, + }, +})); diff --git a/packages/app/src/review/feedback/use-feedback.ts b/packages/app/src/review/feedback/use-feedback.ts new file mode 100644 index 0000000000..e2b151953e --- /dev/null +++ b/packages/app/src/review/feedback/use-feedback.ts @@ -0,0 +1,120 @@ +import { useEffect, useMemo, useRef, useSyncExternalStore } from "react"; +import { + useReviewDraftComments, + getReviewDraftComments, + clearSentReviewDraftComments, +} from "../store"; +import { useWorkspaceLayoutStore } from "@/stores/workspace-layout-store"; +import { buildWorkspaceTabPersistenceKey } from "@/workspace-tabs/model"; +import { deriveWorkspaceTabModel } from "@/screens/workspace/workspace-tab-model"; +import { useHostRuntimeClient, useHostRuntimeIsConnected } from "@/runtime/host-runtime"; +import { useAppSettings } from "@/hooks/use-settings"; +import { dispatchComposerAgentMessage } from "@/composer/actions"; +import { createMessageSubmissionWriter } from "@/composer/submission/writer"; +import { useSessionStore, selectAgentTurnPresentation } from "@/stores/session-store"; +import { FLOATING_ACTION_BUTTON_CLEARANCE } from "@/components/ui/floating-action-button"; +import { useIsCompactFormFactor } from "@/constants/layout"; +import { buttonControlHeight } from "@/components/ui/control-geometry"; +import { SPACING } from "@/styles/theme"; +import { i18n } from "@/i18n/i18next"; +import { + createReviewFeedbackScope, + type FeedbackAttachment, + type FeedbackRecipient, +} from "./index"; + +const feedbackScope = createReviewFeedbackScope(); + +export function useReviewFeedback(input: { + reviewDraftKey: string; + serverId: string; + workspaceId?: string; + attachment: FeedbackAttachment | null; +}) { + const { serverId, reviewDraftKey, workspaceId } = input; + const isCompact = useIsCompactFormFactor(); + const workspaceKey = workspaceId + ? buildWorkspaceTabPersistenceKey({ serverId, workspaceId }) + : null; + const client = useHostRuntimeClient(serverId); + const connected = useHostRuntimeIsConnected(serverId); + const { settings } = useAppSettings(); + const comments = useReviewDraftComments(reviewDraftKey); + const layout = useWorkspaceLayoutStore((store) => + workspaceKey ? store.layoutByWorkspace[workspaceKey] : undefined, + ); + // The adapter keeps live host and diff inputs available to commands without reconstructing the workflow. + const live = useRef({ + client, + connected, + attachment: input.attachment, + sendBehavior: settings.sendBehavior, + }); + live.current = { + client, + connected, + attachment: input.attachment, + sendBehavior: settings.sendBehavior, + }; + const model = useMemo( + () => + feedbackScope.connect(reviewDraftKey, { + read: () => ({ + comments: getReviewDraftComments(reviewDraftKey) ?? [], + attachment: live.current.attachment, + connected: live.current.connected, + recipients: readRecipients(workspaceKey), + }), + send: async (recipient, attachment) => { + const { client: targetClient, sendBehavior } = live.current; + if (!targetClient) throw new Error(i18n.t("workspace.terminal.hostDisconnected")); + const activeTurnBehavior = sendBehavior === "steer" ? "steer" : "interrupt"; + await dispatchComposerAgentMessage({ + client: targetClient, + agentId: recipient.agentId, + text: i18n.t("review.feedback.prompt"), + attachments: [attachment], + encodeImages: async () => [], + submission: createMessageSubmissionWriter(serverId), + activeTurnBehavior, + activeTurnId: + activeTurnBehavior === "steer" + ? (selectAgentTurnPresentation( + useSessionStore.getState().sessions[serverId], + recipient.agentId, + ).turnId ?? undefined) + : undefined, + }); + }, + clearSent: (sentComments) => + clearSentReviewDraftComments({ key: reviewDraftKey, comments: sentComments }), + failedMessage: () => i18n.t("review.feedback.failed"), + }), + [reviewDraftKey, serverId, workspaceKey], + ); + useEffect(() => { + model.refresh(); + }, [model, comments, layout, connected, input.attachment]); + const state = useSyncExternalStore(model.subscribe, model.getState, model.getState); + return { + model, + state, + clearance: + state.commentCount > 0 + ? (isCompact ? FLOATING_ACTION_BUTTON_CLEARANCE : buttonControlHeight.sm + SPACING[4]) + 44 + : 0, + }; +} + +function readRecipients(workspaceKey: string | null): FeedbackRecipient[] { + const tabs = workspaceKey + ? useWorkspaceLayoutStore.getState().getWorkspaceTabs(workspaceKey) + : []; + const descriptors = deriveWorkspaceTabModel({ tabs }).tabs; + const recipients: FeedbackRecipient[] = []; + for (const { descriptor } of descriptors) { + if (descriptor.target.kind === "agent") + recipients.push({ agentId: descriptor.target.agentId, tab: descriptor }); + } + return recipients; +} diff --git a/packages/app/src/review/geometry.ts b/packages/app/src/review/geometry.ts index 819c1d29b1..46bbf88ecc 100644 --- a/packages/app/src/review/geometry.ts +++ b/packages/app/src/review/geometry.ts @@ -19,6 +19,7 @@ export interface InlineReviewActions { onStartComment: (target: ReviewableDiffTarget) => void; onEditComment: (target: ReviewableDiffTarget, comment: ReviewDraftComment) => void; onCancelEditor: () => void; + onChangeEditorBody: (body: string) => void; onSaveEditor: (body: string) => void; onDeleteComment: (id: string) => void; } diff --git a/packages/app/src/review/index.ts b/packages/app/src/review/index.ts index 014447090b..308f955259 100644 --- a/packages/app/src/review/index.ts +++ b/packages/app/src/review/index.ts @@ -31,3 +31,8 @@ export { SMALL_ACTION_HIT_SLOP, useInlineReviewController, } from "./surface"; + +export { ReviewCommentSheet } from "./comment-sheet"; + +export { useReviewFeedback } from "./feedback/use-feedback"; +export { ReviewFeedbackAction } from "./feedback/surface"; diff --git a/packages/app/src/review/store.test.ts b/packages/app/src/review/store.test.ts index 344b28b61d..222a3f57f3 100644 --- a/packages/app/src/review/store.test.ts +++ b/packages/app/src/review/store.test.ts @@ -2,7 +2,16 @@ import { describe, expect, it } from "vitest"; import type { StateStorage } from "zustand/middleware"; import type { ParsedDiffFile } from "@/git/use-diff-query"; import { createValidatedPersistStorage } from "@/storage/validated-persist-storage"; -import { buildReviewAttachmentSnapshot, buildReviewDraftKey } from "./store"; +import { + buildReviewAttachmentSnapshot, + buildReviewDraftKey, + addReviewDraftComment, + saveReviewDraftComment, + clearSentReviewDraftComments, + getReviewDraftComments, + resetReviewDraftStore, + useReviewDraftStore, +} from "./store"; import { addCommentToState, clearReviewInState, @@ -304,3 +313,49 @@ describe("buildReviewAttachmentSnapshot", () => { }); }); }); + +describe("successful feedback cleanup", () => { + it("clears sent versions only, preserving edits, additions and other workspace drafts", () => { + resetReviewDraftStore(); + const first = addReviewDraftComment({ key: "first", comment: makeComment() }); + const edited = addReviewDraftComment({ + key: "first", + comment: makeComment({ id: "edited", body: "Before send" }), + }); + const other = addReviewDraftComment({ key: "other", comment: makeComment({ id: "other" }) }); + useReviewDraftStore.getState().updateComment({ + key: "first", + id: edited.id, + updates: { body: "After send" }, + updatedAt: edited.updatedAt, + }); + const added = addReviewDraftComment({ key: "first", comment: makeComment({ id: "added" }) }); + clearSentReviewDraftComments({ key: "first", comments: [first, edited] }); + expect(getReviewDraftComments("first")).toEqual([{ ...edited, body: "After send" }, added]); + expect(getReviewDraftComments("other")).toEqual([other]); + resetReviewDraftStore(); + }); +}); + +it("saves an acknowledged open edit as a new version and keeps subsequent edits on that record", () => { + resetReviewDraftStore(); + const submitted = addReviewDraftComment({ key: "first", comment: makeComment() }); + clearSentReviewDraftComments({ key: "first", comments: [submitted] }); + saveReviewDraftComment({ + key: "first", + id: submitted.id, + comment: { ...submitted, body: "Open edit after acknowledgement" }, + }); + const restored = getReviewDraftComments("first")![0]!; + expect(restored.body).toBe("Open edit after acknowledgement"); + expect(restored.id).not.toBe(submitted.id); + clearSentReviewDraftComments({ key: "first", comments: [submitted] }); + expect(getReviewDraftComments("first")).toEqual([restored]); + saveReviewDraftComment({ + key: "first", + id: restored.id, + comment: { ...restored, body: "Saved again" }, + }); + expect(getReviewDraftComments("first")).toMatchObject([{ id: restored.id, body: "Saved again" }]); + resetReviewDraftStore(); +}); diff --git a/packages/app/src/review/store.ts b/packages/app/src/review/store.ts index bac1659751..614572190a 100644 --- a/packages/app/src/review/store.ts +++ b/packages/app/src/review/store.ts @@ -320,3 +320,43 @@ export function useReviewAttachmentSnapshot(input: { [comments, input.key, input.cwd, input.mode, input.baseRef, input.diffFiles], ); } + +/** Acknowledgement removes only the versions included in that submission. */ +export function clearSentReviewDraftComments(input: { + key: string; + comments: readonly ReviewDraftComment[]; +}): void { + const sentById = new Map(input.comments.map((comment) => [comment.id, comment])); + useReviewDraftStore.setState((state) => { + const current = state.drafts[input.key]; + if (!current) return state; + const remaining = current.filter((comment) => { + const sent = sentById.get(comment.id); + return !sent || sent.updatedAt !== comment.updatedAt || sent.body !== comment.body; + }); + if (remaining.length === current.length) return state; + return { drafts: { ...state.drafts, [input.key]: remaining } }; + }); +} + +/** Acknowledgement can remove the original while the user still has an edit open. */ +export function saveReviewDraftComment(input: { + key: string; + id: string | null; + comment: Pick; +}): void { + const store = useReviewDraftStore.getState(); + if (input.id && store.drafts[input.key]?.some((comment) => comment.id === input.id)) { + store.updateComment({ key: input.key, id: input.id, updates: { body: input.comment.body } }); + } else { + store.addComment({ + key: input.key, + comment: { + filePath: input.comment.filePath, + side: input.comment.side, + lineNumber: input.comment.lineNumber, + body: input.comment.body, + }, + }); + } +} diff --git a/packages/app/src/review/surface.test.tsx b/packages/app/src/review/surface.test.tsx index e23f321d38..925ebe6b24 100644 --- a/packages/app/src/review/surface.test.tsx +++ b/packages/app/src/review/surface.test.tsx @@ -9,45 +9,24 @@ import { useReviewDraftStore, type ReviewDraftComment } from "./store"; import { buildReviewableDiffTargetKey, type ReviewableDiffTarget } from "@/utils/diff-layout"; import { getInlineReviewThreadState, - getInlineReviewThreadViewportStyle, getSplitInlineReviewThreadState, + type InlineReviewActions, +} from "./geometry"; +import { + getInlineReviewThreadViewportStyle, groupInlineReviewCommentsByTarget, InlineReviewEditor, InlineReviewGutterCell, InlineReviewThread, SMALL_ACTION_HIT_SLOP, useInlineReviewController, - type InlineReviewActions, -} from "./index"; +} from "./surface"; void testI18n; -const { theme, pressablePropsByLabel } = vi.hoisted(() => { +const { pressablePropsByLabel } = vi.hoisted(() => { Object.assign(globalThis, { __DEV__: false }); - return { - theme: { - spacing: { 1: 4, 2: 8, 3: 12 }, - borderWidth: { 1: 1 }, - borderRadius: { base: 4, md: 6, lg: 8, xl: 12, full: 999 }, - opacity: { 50: 0.5 }, - fontSize: { xs: 11, sm: 13 }, - fontWeight: { normal: "400", medium: "500" }, - lineHeight: { diff: 18 }, - colors: { - accent: "#0a84ff", - accentForeground: "#fff", - border: "#555", - destructive: "#ff453a", - foreground: "#fff", - foregroundMuted: "#aaa", - surface1: "#111", - surface2: "#222", - surface3: "#333", - palette: { white: "#fff" }, - }, - }, - pressablePropsByLabel: new Map>(), - }; + return { pressablePropsByLabel: new Map>() }; }); vi.mock("react-native", async (importOriginal) => { @@ -88,13 +67,16 @@ vi.mock("react-native", async (importOriginal) => { }; }); -vi.mock("react-native-unistyles", () => ({ - StyleSheet: { - create: (factory: unknown) => (typeof factory === "function" ? factory(theme) : factory), - }, - withUnistyles: (component: T) => component, - useUnistyles: () => ({ theme, rt: { breakpoint: "md" } }), -})); +vi.mock("react-native-unistyles", async () => { + const { darkTheme: theme } = await import("@/styles/theme"); + return { + StyleSheet: { + create: (factory: unknown) => (typeof factory === "function" ? factory(theme) : factory), + }, + withUnistyles: (component: T) => component, + useUnistyles: () => ({ theme, rt: { breakpoint: "md" } }), + }; +}); vi.mock("@/constants/platform", () => ({ getIsElectron: () => false, @@ -103,19 +85,6 @@ vi.mock("@/constants/platform", () => ({ isWeb: true, })); -vi.mock("lucide-react-native", () => { - const createIcon = (name: string) => (props: Record) => - React.createElement("span", { ...props, "data-icon": name }); - return { - Check: createIcon("Check"), - CircleDot: createIcon("CircleDot"), - Pencil: createIcon("Pencil"), - Plus: createIcon("Plus"), - Trash2: createIcon("Trash2"), - X: createIcon("X"), - }; -}); - function target(overrides: Partial = {}): ReviewableDiffTarget { return { filePath: "src/example.ts", @@ -147,6 +116,7 @@ function buildReviewActions(overrides: Partial = {}): Inlin onStartComment: vi.fn(), onEditComment: vi.fn(), onCancelEditor: vi.fn(), + onChangeEditorBody: () => {}, onSaveEditor: vi.fn(), onDeleteComment: vi.fn(), ...overrides, @@ -312,8 +282,9 @@ describe("git diff inline review helpers", () => { it("keeps the line number visible and only floats the plus for line hover", () => { const reviewTarget = target(); - const { container, queryByText, rerender } = render( + const { queryByTestId, queryByText, rerender } = render( { ); expect(queryByText("2")).toBeTruthy(); - expect(container.querySelector("[data-icon='Plus']")).toBeNull(); + expect(queryByTestId("add-comment-glyph")).toBeNull(); rerender( { ); expect(queryByText("2")).toBeTruthy(); - expect(container.querySelector("[data-icon='Plus']")).toBeNull(); + expect(queryByTestId("add-comment-glyph")).toBeNull(); rerender( { ); expect(queryByText("2")).toBeTruthy(); - expect(container.querySelector("[data-icon='Plus']")).toBeNull(); + expect(queryByTestId("add-comment-glyph")).toBeNull(); rerender( { ); expect(queryByText("2")).toBeTruthy(); - expect(container.querySelector("[data-icon='Plus']")).toBeTruthy(); + expect(queryByTestId("add-comment-glyph")).toBeTruthy(); }); }); @@ -384,7 +358,7 @@ describe("InlineReviewEditor", () => { const onCancel = vi.fn(); const onSave = vi.fn(); const { getByTestId } = render( - { const onCancel = vi.fn(); const onSave = vi.fn(); const { getByTestId } = render( - { removeEventListener: vi.fn(), }); const { getByTestId, queryByText } = render( - { expect(actions.onDeleteComment).toHaveBeenCalledWith("comment-1"); }); }); + +function ReviewEditorFixture({ + initialBody, + ...props +}: Omit, "body" | "onChangeBody"> & { + initialBody: string; +}) { + const [body, onChangeBody] = React.useState(initialBody); + return ; +} diff --git a/packages/app/src/review/surface.tsx b/packages/app/src/review/surface.tsx index 43b6af914a..7466052456 100644 --- a/packages/app/src/review/surface.tsx +++ b/packages/app/src/review/surface.tsx @@ -1,4 +1,11 @@ -import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, + useSyncExternalStore, +} from "react"; import type { ReactNode } from "react"; import { useTranslation } from "react-i18next"; import { Pencil, Plus, Trash2 } from "lucide-react-native"; @@ -17,10 +24,17 @@ import { EditingTextInput as TextInput, type EditingTextInputHandle, } from "@/components/ui/text-input"; +import { useIsCompactFormFactor } from "@/constants/layout"; +import { createReviewEditorScope } from "./editor"; import { isWeb } from "@/constants/platform"; import { inlineUnistylesStyle } from "@/styles/unistyles-inline-style"; import type { Theme } from "@/styles/theme"; -import { useReviewDraftComments, useReviewDraftStore, type ReviewDraftComment } from "./store"; +import { + useReviewDraftComments, + useReviewDraftStore, + saveReviewDraftComment, + type ReviewDraftComment, +} from "./store"; import { buildReviewableDiffTargetKey, type ReviewableDiffTarget } from "@/utils/diff-layout"; import { INLINE_REVIEW_EDITOR_HEIGHT, @@ -98,92 +112,42 @@ export function groupInlineReviewCommentsByTarget( return grouped; } -export function useInlineReviewController(input: { reviewDraftKey: string }): InlineReviewActions { +const reviewEditors = createReviewEditorScope({ + saveComment: saveReviewDraftComment, + deleteComment: (input) => useReviewDraftStore.getState().deleteComment(input), +}); + +export function useInlineReviewController(input: { + reviewDraftKey: string; + presentation?: string; +}): InlineReviewActions & { + sheetEditor: InlineReviewEditorState | null; + onChangeEditorBody: (body: string) => void; +} { + const isCompact = useIsCompactFormFactor(); const reviewComments = useReviewDraftComments(input.reviewDraftKey); const commentsByTarget = useMemo( () => groupInlineReviewCommentsByTarget(reviewComments), [reviewComments], ); - const [editor, setEditor] = useState(null); - const addComment = useReviewDraftStore((state) => state.addComment); - const updateComment = useReviewDraftStore((state) => state.updateComment); - const deleteComment = useReviewDraftStore((state) => state.deleteComment); - - useEffect(() => { - setEditor(null); - }, [input.reviewDraftKey]); - - const handleStartComment = useCallback((target: ReviewableDiffTarget) => { - setEditor({ target, commentId: null, body: "" }); - }, []); - - const handleEditComment = useCallback( - (target: ReviewableDiffTarget, comment: ReviewDraftComment) => { - setEditor({ target, commentId: comment.id, body: comment.body }); - }, - [], + const model = useMemo( + () => reviewEditors.forDraft(input.reviewDraftKey, input.presentation ?? "diff"), + [input.reviewDraftKey, input.presentation], ); - - const handleCancelEditor = useCallback(() => { - setEditor(null); - }, []); - - const handleSaveEditor = useCallback( - (body: string) => { - const trimmedBody = body.trim(); - if (!editor || trimmedBody.length === 0) { - return; - } - - if (editor.commentId) { - updateComment({ - key: input.reviewDraftKey, - id: editor.commentId, - updates: { body: trimmedBody }, - }); - } else { - addComment({ - key: input.reviewDraftKey, - comment: { - filePath: editor.target.filePath, - side: editor.target.side, - lineNumber: editor.target.lineNumber, - body: trimmedBody, - }, - }); - } - setEditor(null); - }, - [addComment, editor, input.reviewDraftKey, updateComment], - ); - - const handleDeleteComment = useCallback( - (id: string) => { - deleteComment({ key: input.reviewDraftKey, id }); - setEditor((current) => (current?.commentId === id ? null : current)); - }, - [deleteComment, input.reviewDraftKey], - ); - - return useMemo( + const editor = useSyncExternalStore(model.subscribe, model.getState, model.getState); + return useMemo( () => ({ commentsByTarget, - editor, - onStartComment: handleStartComment, - onEditComment: handleEditComment, - onCancelEditor: handleCancelEditor, - onSaveEditor: handleSaveEditor, - onDeleteComment: handleDeleteComment, + editor: isCompact ? null : editor, + sheetEditor: isCompact ? editor : null, + onChangeEditorBody: model.setBody, + onStartComment: model.start, + onEditComment: model.edit, + onCancelEditor: model.cancel, + onSaveEditor: model.save, + onDeleteComment: model.delete, }), - [ - commentsByTarget, - editor, - handleCancelEditor, - handleDeleteComment, - handleEditComment, - handleSaveEditor, - handleStartComment, - ], + [commentsByTarget, editor, isCompact, model], ); } @@ -321,7 +285,8 @@ export function InlineReviewThread({ const editorElement = editor ? ( void; onCancel: () => void; onSave: (body: string) => void; testID?: string; }) { const { t } = useTranslation(); const inputRef = useRef(null); - const [body, setBody] = useState(initialBody); const [isFocused, setIsFocused] = useState(false); const trimmedBody = body.trim(); const canSave = trimmedBody.length > 0; @@ -506,7 +472,7 @@ export function InlineReviewEditor({ placeholderTextColor={styles.placeholderColor.color} multiline initialValue={body} - onChangeText={setBody} + onChangeText={onChangeBody} onFocus={handleFocus} onBlur={handleBlur} style={inputStyle}