From 2ec2fecbdd9d43ae7238f560959bb416c795e0a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C5=ABdolfs=20O=C5=A1i=C5=86=C5=A1?= Date: Tue, 10 Nov 2026 14:56:23 +0100 Subject: [PATCH] desktop: Save embeds when editing a review summary The summary editor allowed attaching files but never sent them, so the link ended up in the summary while the file never reached the COB. Verdict changes and task toggles also sent no embeds, which would drop them again once a summary has some. --- .../bindings/cob/patch/Review.ts | 2 + .../src/domain/patch/models/patch.rs | 2 + desktop/src/components/ReviewPage.svelte | 12 +- desktop/src/components/Revision.svelte | 8 +- desktop/tests/e2e/repo/editingEmbeds.spec.ts | 124 ++++++++++++++++++ desktop/tests/unit/support/cobs.ts | 1 + 6 files changed, 144 insertions(+), 5 deletions(-) diff --git a/desktop/crates/radicle-types/bindings/cob/patch/Review.ts b/desktop/crates/radicle-types/bindings/cob/patch/Review.ts index 53bcaf083..c2de3d933 100644 --- a/desktop/crates/radicle-types/bindings/cob/patch/Review.ts +++ b/desktop/crates/radicle-types/bindings/cob/patch/Review.ts @@ -2,6 +2,7 @@ import type { Author } from "../Author"; import type { CodeLocation } from "../thread/CodeLocation"; import type { Comment } from "../thread/Comment"; +import type { Embed } from "../thread/Embed"; import type { Verdict } from "./Verdict"; export type Review = { @@ -12,4 +13,5 @@ export type Review = { comments: Array>; timestamp: number; labels: Array; + embeds: Array; }; diff --git a/desktop/crates/radicle-types/src/domain/patch/models/patch.rs b/desktop/crates/radicle-types/src/domain/patch/models/patch.rs index 25b8382b2..f73bab8c3 100644 --- a/desktop/crates/radicle-types/src/domain/patch/models/patch.rs +++ b/desktop/crates/radicle-types/src/domain/patch/models/patch.rs @@ -325,6 +325,7 @@ pub struct Review { timestamp: cob::common::Timestamp, #[ts(as = "Vec")] labels: Vec, + embeds: Vec, } impl Review { @@ -346,6 +347,7 @@ impl Review { }) .collect::>(), timestamp: review.timestamp(), + embeds: review.embeds().iter().cloned().map(Into::into).collect(), } } } diff --git a/desktop/src/components/ReviewPage.svelte b/desktop/src/components/ReviewPage.svelte index 26487e393..66a2172b2 100644 --- a/desktop/src/components/ReviewPage.svelte +++ b/desktop/src/components/ReviewPage.svelte @@ -29,6 +29,7 @@ fullFileLoader, gitStatusEntries, } from "@app/lib/diffText"; + import { embedMap } from "@app/lib/embeds"; import { nodeRunning } from "@app/lib/events"; import { cachedGetDiff, @@ -180,7 +181,7 @@ } } - async function saveSummary(newSummary: string) { + async function saveSummary(newSummary: string, embeds: Embed[]) { try { savingSummary = true; await invoke("edit_patch", { @@ -192,6 +193,7 @@ summary: newSummary, verdict: review.verdict, labels: review.labels, + embeds, }, opts: { announce: $nodeRunning && $announce }, }); @@ -241,6 +243,7 @@ summary: review.summary ?? "", verdict, labels: review.labels, + embeds: review.embeds, }, opts: { announce: $nodeRunning && $announce }, }); @@ -957,11 +960,12 @@ { - await saveSummary(comment); + submit={async ({ comment, embeds }) => { + await saveSummary(comment, Array.from(embeds.values())); }} close={() => (editingSummary = false)} /> @@ -974,7 +978,7 @@ content={review.summary ?? ""} toggleTaskItem={isOwnPublishedReview ? async content => { - await saveSummary(content); + await saveSummary(content, review.embeds); } : undefined} /> {:else} diff --git a/desktop/src/components/Revision.svelte b/desktop/src/components/Revision.svelte index 94356b35d..11d96bcec 100644 --- a/desktop/src/components/Revision.svelte +++ b/desktop/src/components/Revision.svelte @@ -606,6 +606,7 @@ reviewId: string, summary: string, review: { verdict?: Verdict; labels?: string[] }, + embeds: Embed[], ) { try { await invoke("edit_patch", { @@ -617,6 +618,7 @@ summary, verdict: review.verdict, labels: review.labels ?? [], + embeds, }, opts: { announce: $nodeRunning && $announce }, }); @@ -1426,6 +1428,10 @@ r => r.id === opId, )} {@const hasReviewComments = (reviewRecord?.comments?.length ?? 0) > 0} + {@const reviewEmbeds = + [revision, ...revisions] + .flatMap(r => r.reviews ?? []) + .find(r => r.id === opId)?.embeds ?? []} {@const reviewOp = data.op} { - await editReviewSummary(opId, summary, reviewOp); + await editReviewSummary(opId, summary, reviewOp, reviewEmbeds); } : undefined} onViewFullReview={hasReviewComments diff --git a/desktop/tests/e2e/repo/editingEmbeds.spec.ts b/desktop/tests/e2e/repo/editingEmbeds.spec.ts index aba9f4a1a..ce911dab3 100644 --- a/desktop/tests/e2e/repo/editingEmbeds.spec.ts +++ b/desktop/tests/e2e/repo/editingEmbeds.spec.ts @@ -3,6 +3,9 @@ import type { Revision } from "@bindings/cob/patch/Revision"; import type { Locator, Page } from "@playwright/test"; import type { RadiclePeer } from "@tests/support/peerManager.js"; +import * as Fs from "node:fs/promises"; +import * as Path from "node:path"; + import { api, saveEmbed } from "@tests/support/api.js"; import { createProject } from "@tests/support/collaboration.js"; import { expect, test, waitForCommand } from "@tests/support/fixtures.js"; @@ -57,6 +60,43 @@ async function patchWithEmbed(page: Page, peer: RadiclePeer) { return { rid, patchId, embed, link }; } +async function reviewWithEmbed(page: Page, peer: RadiclePeer) { + const { rid, patchId } = await createProject(peer); + const { embed, link } = await megaphone(page, peer, rid); + const reviewId = (await api(page, peer, "create_patch_review", { + args: { + rid, + revision: patchId, + verdict: "accept", + summary: "Looks fine", + labels: [], + // Only reviews with comments get a page of their own. + comments: [{ body: "Loud enough", location: null }], + }, + })) as string; + await api(page, peer, "edit_patch", { + rid, + cobId: patchId, + action: { + type: "review.edit", + review: reviewId, + summary: `Looks fine\n\n${link}`, + verdict: "accept", + labels: [], + embeds: [embed], + }, + opts, + }); + await page.goto(`/repos/${rid}/patches/${patchId}?review=${reviewId}`); + await expect(page.getByText("Looks fine")).toBeVisible(); + + const storedEmbeds = async () => + (await storedRevision(page, peer, rid, patchId)).reviews?.find( + review => review.id === reviewId, + )?.embeds; + return { embed, link, storedEmbeds }; +} + async function saveEdit(page: Page, command: string, body: string) { await page .getByRole("textbox", { name: "textarea-comment" }) @@ -190,3 +230,87 @@ test("editing a patch description on the changes tab keeps its embeds", async ({ expect(description.at(-1)?.body).toContain("so make it loud"); expect(description.at(-1)?.embeds).toEqual([embed]); }); + +test("editing a review summary keeps its embeds", async ({ page, peer }) => { + const { embed, link, storedEmbeds } = await reviewWithEmbed(page, peer); + + await page.getByText("Looks fine").hover(); + await page.getByTitle("Edit summary").click(); + await saveEdit(page, "edit_patch", `Too loud\n\n${link}`); + + await expect(page.getByText("Too loud")).toBeVisible(); + expect(await storedEmbeds()).toEqual([embed]); +}); + +test("ticking a task in a review of an older revision keeps its embeds", async ({ + page, + peer, +}) => { + const { rid, repoFolder, patchId } = await createProject(peer); + const { embed, link } = await megaphone(page, peer, rid); + const reviewId = (await api(page, peer, "create_patch_review", { + args: { + rid, + revision: patchId, + verdict: "accept", + summary: "Before merging", + labels: [], + comments: [], + }, + })) as string; + await api(page, peer, "edit_patch", { + rid, + cobId: patchId, + action: { + type: "review.edit", + review: reviewId, + summary: `Before merging:\n\n- [ ] Rename the file\n\n${link}`, + verdict: "accept", + labels: [], + embeds: [embed], + }, + opts, + }); + await Fs.writeFile( + Path.join(repoFolder, "notes.txt"), + "one\ntwo\nTHREE!\nfour\nfive\n", + ); + await peer.git(["commit", "-am", "Shout louder"], { cwd: repoFolder }); + await peer.git( + [ + "push", + "-o", + "patch.message=Shout louder", + "rad", + `HEAD:refs/heads/patches/${patchId}`, + ], + { cwd: repoFolder }, + ); + + await page.goto(`/repos/${rid}/patches/${patchId}`); + const rename = page.getByRole("checkbox", { name: "Rename the file" }); + await waitForCommand(page, "edit_patch", () => rename.click()); + await expect(rename).toBeChecked(); + + const revisions = (await api(page, peer, "revisions_by_patch", { + rid, + id: patchId, + })) as Revision[]; + const review = revisions + .flatMap(revision => revision.reviews ?? []) + .find(review => review.id === reviewId); + expect(review?.summary).toContain("- [x] Rename the file"); + expect(review?.embeds).toEqual([embed]); +}); + +test("changing a review's verdict keeps its embeds", async ({ page, peer }) => { + const { embed, storedEmbeds } = await reviewWithEmbed(page, peer); + + await page.getByRole("button", { name: /Accepted/ }).click(); + await waitForCommand(page, "edit_patch", () => + page.getByRole("button", { name: /Reject$/ }).click(), + ); + + await expect(page.getByRole("button", { name: /Rejected/ })).toBeVisible(); + expect(await storedEmbeds()).toEqual([embed]); +}); diff --git a/desktop/tests/unit/support/cobs.ts b/desktop/tests/unit/support/cobs.ts index d5114f486..8306610bb 100644 --- a/desktop/tests/unit/support/cobs.ts +++ b/desktop/tests/unit/support/cobs.ts @@ -56,6 +56,7 @@ export function review(props: Partial = {}): Review { comments: [], timestamp: 0, labels: [], + embeds: [], ...props, }; }