diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.test.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.test.ts new file mode 100644 index 0000000000..c50db3be52 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.test.ts @@ -0,0 +1,19 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { shareUrlFromDeepLink } from "./reviewShareLink.js"; + +test("a share deep link keeps its origin and revision", () => { + const t = "a".repeat(43), + rev = "5b0e9a52-4a43-4f7e-9d8e-3c1f0e7b2a11"; + assert.equal( + shareUrlFromDeepLink(`/${t}`, `origin=${encodeURIComponent("https://stg.dev.fast")}&rev=${rev}`), + `https://stg.dev.fast/s/${t}?rev=${rev}`, + ); + assert.equal(shareUrlFromDeepLink(`/${t}`, ""), `https://app.dev.fast/s/${t}`); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.ts b/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.ts new file mode 100644 index 0000000000..b78484f6f6 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.ts @@ -0,0 +1,12 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +export function shareUrlFromDeepLink(path: string, query: string): string { + const params = new URLSearchParams(query); + const url = new URL(`/s${path}`, params.get("origin") ?? "https://app.dev.fast"); + const rev = params.get("rev"); + if (rev) url.searchParams.set("rev", rev); + return url.href; +} diff --git a/apps/review-desktop/code-oss/src/vs/review/contrib/sharing/reviewSharing.contribution.ts b/apps/review-desktop/code-oss/src/vs/review/contrib/sharing/reviewSharing.contribution.ts index 5befc07b61..300d5a560d 100644 --- a/apps/review-desktop/code-oss/src/vs/review/contrib/sharing/reviewSharing.contribution.ts +++ b/apps/review-desktop/code-oss/src/vs/review/contrib/sharing/reviewSharing.contribution.ts @@ -17,6 +17,7 @@ import { WorkbenchPhase, } from "../../../workbench/common/contributions.js"; import { ReviewApiClient } from "../../common/reviewProtocol.js"; +import { shareUrlFromDeepLink } from "../../common/reviewShareLink.js"; import { IReviewCanvasEditorTabsService } from "../../services/reviewCanvasEditorTabsService.js"; import { IReviewDesktopConnectionService } from "../../services/reviewDesktopConnectionService.js"; @@ -81,8 +82,7 @@ class SharedReviewUrlHandler extends Disposable implements IURLHandler { async handleURL(uri: URI): Promise { if (uri.authority !== "share") return false; try { - const origin = new URLSearchParams(uri.query).get("origin") ?? "https://app.dev.fast"; - await openShare({ url: new URL(`/s${uri.path}`, origin).href }, this.session, this.tabs, this.notifications, this.progress); + await openShare({ url: shareUrlFromDeepLink(uri.path, uri.query) }, this.session, this.tabs, this.notifications, this.progress); } catch { this.notifications.error("Invalid Whiteboard share link."); } diff --git a/apps/review-desktop/scripts/e2e/fixtures/share-service.mjs b/apps/review-desktop/scripts/e2e/fixtures/share-service.mjs new file mode 100644 index 0000000000..936ddc63bc --- /dev/null +++ b/apps/review-desktop/scripts/e2e/fixtures/share-service.mjs @@ -0,0 +1,147 @@ +/** A share service for journeys: one link per board, a revision per newer version. */ +import { randomBytes, randomUUID } from "node:crypto"; +import { createServer } from "node:http"; +import { createServer as createSecureServer } from "node:https"; + +export async function startShareService({ tls } = {}) { + const posts = []; + // Board id to { shareId, capability, revisions: [{ id, version, size, createdAt, bundle }] }, newest revision first. + const links = new Map(); + + const server = tls + ? createSecureServer( + tls, + (request, response) => void handle(request, response), + ) + : createServer((request, response) => void handle(request, response)); + + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + server.unref(); + + const origin = `${tls ? "https" : "http"}://localhost:${server.address().port}`; + + const linkUrl = (link, rev) => + `${origin}/s/${link.capability}${rev ? `?rev=${rev}` : ""}`; + + function publish(reviewId, bundle) { + let link = links.get(reviewId); + + if (!link) { + link = { + shareId: randomUUID(), + capability: randomBytes(32).toString("base64url"), + revisions: [], + }; + links.set(reviewId, link); + } + + if (!link.revisions.length || bundle.version > link.revisions[0].version) + link.revisions.unshift({ + id: randomUUID(), + version: bundle.version, + size: Buffer.byteLength(JSON.stringify(bundle)), + createdAt: Date.now(), + bundle, + }); + + return { shareId: link.shareId, url: linkUrl(link) }; + } + + const byShareId = (id) => + [...links.values()].find((link) => link.shareId === id); + + const byCapability = (cap) => + [...links.values()].find((link) => link.capability === cap); + + async function handle(request, response) { + const url = new URL(request.url, origin); + + const send = (status, body) => { + response.writeHead(status, { "content-type": "application/json" }); + response.end(JSON.stringify(body ?? { error: "not found" })); + }; + + let body = ""; + + for await (const chunk of request) body += chunk; + + const parts = url.pathname.split("/").slice(1); + const route = `${request.method} /${parts.slice(0, 3).join("/")}`; + + if (route === "GET /api/auth/token") return send(404); + + if (url.pathname === "/api/v2/shares" && request.method === "GET") + return send(200, []); + + if (url.pathname === "/api/v2/shares" && request.method === "POST") { + const input = JSON.parse(body); + + posts.push(input); + + return send(200, publish(input.bundle.reviewId, input.bundle)); + } + + if (route === "GET /api/v2/shares" && parts[4] === "revisions") { + const link = byShareId(parts[3]); + + return link + ? send(200, { + latest: link.revisions[0].id, + items: link.revisions.map(({ bundle: _, ...revision }) => ({ + ...revision, + url: linkUrl(link, revision.id), + })), + }) + : send(404); + } + + if (route === "DELETE /api/v2/shares") { + const link = byShareId(parts[3]); + + if (!link) return send(404); + + for (const [key, value] of links) if (value === link) links.delete(key); + + return send(200, { shareId: link.shareId, revoked: true }); + } + + if (route === "GET /api/v2/shared") { + const link = byCapability(parts[3]); + const rev = url.searchParams.get("rev"); + + const revision = rev + ? link?.revisions.find((candidate) => candidate.id === rev) + : link?.revisions[0]; + + if (!revision) return send(404); + + if (parts[4] === "bundle") return send(200, revision.bundle); + + return send(200, { + sender: { login: "e2e" }, + sharedAt: revision.createdAt, + size: revision.size, + revision: { + id: revision.id, + version: revision.version, + createdAt: revision.createdAt, + }, + latest: link.revisions[0].id, + }); + } + + send(404); + } + + return { + origin, + posts, + links, + publish, + stop: () => + new Promise((resolve) => { + server.closeAllConnections(); + server.close(() => resolve()); + }), + }; +} diff --git a/apps/review-desktop/scripts/e2e/journeys/share-link.mjs b/apps/review-desktop/scripts/e2e/journeys/share-link.mjs new file mode 100644 index 0000000000..64ec90d1d2 --- /dev/null +++ b/apps/review-desktop/scripts/e2e/journeys/share-link.mjs @@ -0,0 +1,96 @@ +/** A board shared, edited and shared again keeps one link, with a revision per version. */ +import assert from "node:assert/strict"; +import { writeFile } from "node:fs/promises"; +import path from "node:path"; + +import { startShareService } from "../fixtures/share-service.mjs"; +import { createReview, sleep } from "../harness.mjs"; + +export const name = "share-link"; + +export const phase = 1; + +export const options = { + async beforeLaunch(ctx) { + ctx.service = await startShareService(); + await writeFile( + path.join(ctx.home, "auth.json"), + JSON.stringify({ + origin: ctx.service.origin, + token: "e2e", + login: "e2e", + savedAt: new Date().toISOString(), + }), + ); + }, +}; + +export async function run(ctx) { + const { service } = ctx; + + try { + const { reviewId, canvas } = await createReview(ctx, { + title: "Shared link", + blocks: [{ type: "markdown", markdown: "First" }], + }); + + const page = canvas.page(); + const button = page.getByRole("button", { name: "Share review" }); + const input = page.getByLabel("Share link"); + + await button.click(); + await input.waitFor(); + + const u1 = await input.inputValue(); + + assert.equal(service.posts.length, 1); + await page + .getByText("This link always opens the latest share of this board.") + .waitFor(); + ctx.check("the first share uploads once and shows its link"); + await page.keyboard.press("Escape"); + await input.waitFor({ state: "detached" }); + + await ctx.apiOk("/reviews-api/commands", "POST", { + operation: { + type: "edit", + reviewId, + edit: { + type: "insert", + content: { type: "markdown", markdown: "Second" }, + }, + }, + }); + await canvas.getByText("Second", { exact: true }).waitFor(); + await button.click(); + await ctx.until( + async () => + service.posts.length === 2 && + (await input.inputValue().catch(() => "")) === u1, + "the reshared link", + ); + ctx.check("an edited board uploads again behind the same link"); + + const { shareId } = service.links.get(reviewId); + + const revisions = await ( + await fetch(`${service.origin}/api/v2/shares/${shareId}/revisions`) + ).json(); + + assert.equal(revisions.items.length, 2); + assert.ok(revisions.items[0].version > revisions.items[1].version); + assert.equal(revisions.latest, revisions.items[0].id); + ctx.check("the link holds two revisions, newest first"); + + await page.keyboard.press("Escape"); + await input.waitFor({ state: "detached" }); + await button.click(); + await input.waitFor(); + await sleep(2000); + assert.equal(service.posts.length, 2); + assert.equal(await input.inputValue(), u1); + ctx.check("reopening an unchanged board uploads nothing"); + } finally { + await service.stop(); + } +} diff --git a/apps/review-desktop/scripts/e2e/journeys/shared-review.mjs b/apps/review-desktop/scripts/e2e/journeys/shared-review.mjs index a4ece94a83..e6fcf9a842 100644 --- a/apps/review-desktop/scripts/e2e/journeys/shared-review.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/shared-review.mjs @@ -1,12 +1,13 @@ -/** A share's copy renders from its own pinned checkout after the sender's repository is gone. */ +/** A share link opens as a copy that renders from its own pinned checkout, refreshes in place while unedited, and copies beside an edited one. */ import assert from "node:assert/strict"; import { execFile } from "node:child_process"; -import { readFile } from "node:fs/promises"; +import { readFile, rename, writeFile } from "node:fs/promises"; import path from "node:path"; import { promisify } from "node:util"; import { assertNoBlockedReviewRequests } from "../../review-network-policy.mjs"; -import { sourcePackage } from "../harness.mjs"; +import { startShareService } from "../fixtures/share-service.mjs"; +import { openHome, sourcePackage } from "../harness.mjs"; export const name = "shared-review"; @@ -22,21 +23,105 @@ export const options = { async beforeLaunch(ctx) { const root = path.join(ctx.root, "share"); - await exec( - "pnpm", - ["exec", "tsx", "scripts/seed-share-fixture.ts", root, ctx.home], - { cwd: sourcePackage, env: ctx.env }, + const read = async (file) => + JSON.parse(await readFile(path.join(root, file), "utf8")); + + await exec("pnpm", ["exec", "tsx", "scripts/seed-share-fixture.ts", root], { + cwd: sourcePackage, + env: ctx.env, + }); + + const key = path.join(ctx.root, "key.pem"); + const cert = path.join(ctx.root, "cert.pem"); + + await exec("openssl", [ + "req", + "-x509", + "-newkey", + "rsa:2048", + "-nodes", + "-subj", + "/CN=localhost", + "-addext", + "subjectAltName=DNS:localhost", + "-days", + "1", + "-keyout", + key, + "-out", + cert, + ]); + ctx.env.NODE_EXTRA_CA_CERTS = cert; + + ctx.share = await read("fixture.json"); + ctx.bundles = [await read("bundle-1.json"), await read("bundle-2.json")]; + + // The bundle names its GitHub remote; the recipient fetches the sender's checkout instead. + await writeFile( + path.join(ctx.home, ".gitconfig"), + `[url "${ctx.share.repo}"]\n\tinsteadOf = https://github.com/fixture/review.git\n`, ); - ctx.share = JSON.parse( - await readFile(path.join(root, "fixture.json"), "utf8"), + + ctx.service = await startShareService({ + tls: { + key: await readFile(key, "utf8"), + cert: await readFile(cert, "utf8"), + }, + }); + ctx.share.url = ctx.service.publish(ctx.share.reviewId, ctx.bundles[0]).url; + await writeFile( + path.join(ctx.home, "auth.json"), + JSON.stringify({ + origin: ctx.service.origin, + token: "e2e", + login: "e2e", + savedAt: new Date().toISOString(), + }), ); }, }; export async function run(ctx) { - const { share } = ctx; + const { share, service, bundles } = ctx; + + try { + await journey(ctx, share, service, bundles); + } finally { + await service.stop(); + } +} + +async function journey(ctx, share, service, bundles) { + const revisions = () => service.links.get(share.reviewId).revisions; + + const receive = async () => { + const started = await ctx.api("/reviews-api/sharing/import", "POST", { + url: share.url, + }); + + assert.equal(started.status, 202, JSON.stringify(started.value)); + + const { reviewId } = await ctx.until(async () => { + const status = await ctx.apiOk( + `/reviews-api/sharing/import/${started.value.reviewId}`, + ); + + assert.notEqual(status.stage, "error", status.error); + + return status.stage === "ready" && status; + }, "the share import"); + + return reviewId; + }; + + const board = (reviewId) => ctx.apiOk(`/reviews-api/${reviewId}?full=true`); + + const a = await receive(); + const first = await board(a); - await ctx.apiOk(`/reviews-api/${share.reviewId}/open`, "POST", {}); + assert.equal(first.shared.revisionId, revisions()[0].id); + + await ctx.apiOk(`/reviews-api/${a}/open`, "POST", {}); const page = await ctx.apiCanvasFor(HEADING); @@ -47,6 +132,11 @@ export async function run(ctx) { await canvas.getByText("The answer is 42.").first().waitFor(); ctx.check("the shared document and trace quote render"); + await rename( + share.repo, + path.join(path.dirname(share.repo), "sender-unavailable"), + ); + // Native inline widgets expose their rendered code through Monaco's view lines. const code = canvas .locator(".view-lines") @@ -57,21 +147,76 @@ export async function run(ctx) { await code.waitFor(); const source = await ctx.apiOk( - `/reviews-api/${share.reviewId}/file?side=head&file=${encodeURIComponent(share.sourceFile)}`, + `/reviews-api/${a}/file?side=head&file=${encodeURIComponent(share.sourceFile)}`, ); assert.ok( source.text.includes(share.sourceText), `the retained checkout served ${JSON.stringify(source.text)}`, ); - - const summary = await ctx.apiOk(`/reviews-api/${share.reviewId}?full=true`); - - assert.equal(summary.version, share.version); ctx.check( "pinned source renders from the recipient checkout with the sender's repository gone", ); + service.publish(share.reviewId, bundles[1]); + assert.equal(await receive(), a); + + const refreshed = await board(a); + + assert.equal(refreshed.version, first.version + 1); + assert.equal(refreshed.shared.revisionId, revisions()[0].id); + await canvas.getByText(share.note).first().waitFor(); + ctx.check("an unedited copy refreshes in place and its open tab updates"); + + await openHome(ctx); + + const rows = ctx.page + .locator("main.review-home [data-home-row]") + .filter({ hasText: first.title }); + + const copies = (count) => + ctx.until( + async () => (await rows.count()) === count, + `${count} copies on Home`, + ); + + await copies(1); + ctx.check("Home lists one copy after the refresh"); + + await ctx.apiOk("/reviews-api/commands", "POST", { + operation: { + type: "edit", + reviewId: a, + edit: { + type: "insert", + content: { type: "markdown", markdown: "A reader's own note." }, + }, + }, + }); + + const edited = await board(a); + + // Revision 3 is revision 2 again at the sender's next version. + const version = bundles[1].version + 1; + + service.publish(share.reviewId, { + ...bundles[1], + version, + snapshot: { ...bundles[1].snapshot, version }, + }); + + const b = await receive(); + + assert.notEqual(b, a); + assert.equal((await board(b)).shared.revisionId, revisions()[0].id); + assert.equal((await board(a)).version, edited.version); + await copies(2); + ctx.check("a newer revision copies beside an edited copy, leaving it as is"); + + await service.stop(); + assert.equal(await receive(), b); + ctx.check("offline, the link opens its newest copy"); + // A received share renders from what is on disk: nothing leaves the machine. assertNoBlockedReviewRequests(ctx.requestUrls); assert.deepEqual( diff --git a/packages/review-share-protocol/package.json b/packages/review-share-protocol/package.json index ea15c605fc..eaa50f4c07 100644 --- a/packages/review-share-protocol/package.json +++ b/packages/review-share-protocol/package.json @@ -1,6 +1,6 @@ { "name": "@dev.fast/review-share-protocol", - "version": "0.4.1", + "version": "0.5.0", "description": "Portable immutable Review sharing envelope.", "license": "MIT", "repository": { diff --git a/packages/review-share-protocol/src/index.test.ts b/packages/review-share-protocol/src/index.test.ts index 7222737cec..1d268e01c3 100644 --- a/packages/review-share-protocol/src/index.test.ts +++ b/packages/review-share-protocol/src/index.test.ts @@ -29,6 +29,33 @@ it("puts the capability in the path and rejects credentials, fragments or other ); }); +it("carries a revision as ?rev= and rejects any other query", () => { + const token = "a".repeat(43), + rev = "5b0e9a52-4a43-4f7e-9d8e-3c1f0e7b2a11"; + + expect(shareLink("https://app.dev.fast", token, rev)).toBe( + `https://app.dev.fast/s/${token}?rev=${rev}`, + ); + expect(parseShareLink(`https://app.dev.fast/s/${token}?rev=${rev}`)).toEqual({ + origin: "https://app.dev.fast", + capability: token, + revision: rev, + }); + expect( + parseShareLink(`https://app.dev.fast/s/${token}?app=preview&rev=${rev}`) + .revision, + ).toBe(rev); + expect(() => + parseShareLink(`https://app.dev.fast/s/${token}?rev=nope`), + ).toThrow("Invalid UUID"); + expect(() => parseShareLink(`https://app.dev.fast/s/${token}?x=1`)).toThrow( + "HTTPS", + ); + expect(parseShareLink(`https://app.dev.fast/s/${token}`)).not.toHaveProperty( + "revision", + ); +}); + it("rejects traversal paths and platform-specific separators", () => { for (const file of [ "../secret", diff --git a/packages/review-share-protocol/src/index.ts b/packages/review-share-protocol/src/index.ts index 9f31a0dd6c..9f1dd072eb 100644 --- a/packages/review-share-protocol/src/index.ts +++ b/packages/review-share-protocol/src/index.ts @@ -139,11 +139,61 @@ export const createShareSchema = z.object({ bundle: shareBundleSchema, }); -export function shareLink(origin: string, capability: string): string { - return new URL(`/s/${capabilitySchema.parse(capability)}`, origin).href; +export const shareCreatedSchema = z.strictObject({ + shareId: shareIdSchema, + url: z.url(), +}); + +const shareSizeSchema = z.number().int().positive().max(MAX_SHARE_BYTES); + +export const shareRevisionRefSchema = z.object({ + id: shareIdSchema, + version: z.number().int().nonnegative(), + createdAt: z.number(), +}); + +export const shareRevisionSchema = shareRevisionRefSchema.extend({ + size: shareSizeSchema, + url: z.url(), +}); + +export const shareRevisionsSchema = z.object({ + latest: shareIdSchema, + items: z.array(shareRevisionSchema), +}); + +export type ShareRevisions = z.infer; + +export const sharedMetaSchema = z.object({ + sender: z.strictObject({ login: z.string().min(1).max(256) }), + sharedAt: z.number(), + size: shareSizeSchema, + revision: shareRevisionRefSchema, + latest: shareIdSchema, +}); + +export type SharedMeta = z.infer; + +export function shareLink( + origin: string, + capability: string, + revision?: string, +): string { + const url = new URL(`/s/${capabilitySchema.parse(capability)}`, origin); + + if (revision !== undefined) + url.searchParams.set("rev", shareIdSchema.parse(revision)); + + return url.href; } -export function parseShareLink(value: string) { +export type ShareLink = { + origin: string; + capability: string; + revision?: string; +}; + +export function parseShareLink(value: string): ShareLink { const url = new URL(value); if ( @@ -151,14 +201,20 @@ export function parseShareLink(value: string) { url.username || url.password || url.hash || - [...url.searchParams.keys()].some((key) => key !== "app") + [...url.searchParams.keys()].some((key) => key !== "app" && key !== "rev") ) throw new Error("Use an HTTPS Whiteboard share link."); - return { + const link: ShareLink = { origin: url.origin, capability: capabilitySchema.parse( /^\/s\/([^/]+)$/.exec(url.pathname)?.[1], ), }; + + const revision = url.searchParams.get("rev"); + + if (revision !== null) link.revision = shareIdSchema.parse(revision); + + return link; } diff --git a/packages/review/app/src/canvas-query.tsx b/packages/review/app/src/canvas-query.tsx index 5e6517ebd1..88b6aeda1d 100644 --- a/packages/review/app/src/canvas-query.tsx +++ b/packages/review/app/src/canvas-query.tsx @@ -34,8 +34,7 @@ export function createCanvasQueryClient(): QueryClient { /** Desktop shares one client across boards, so board keys name the board. */ export const canvasQueryKeys = { sharingAccount: () => ["sharing", "account"] as const, - shareLink: (board: string, version: number | undefined) => - ["sharing", "link", board, version] as const, + shareLink: (board: string) => ["sharing", "link", board] as const, traceList: ( board: string, version: number | undefined, diff --git a/packages/review/app/src/share-control.test.tsx b/packages/review/app/src/share-control.test.tsx index af1dca61dd..f8814709c1 100644 --- a/packages/review/app/src/share-control.test.tsx +++ b/packages/review/app/src/share-control.test.tsx @@ -93,7 +93,9 @@ function mount(options: { ); return Response.json({ + shareId: "6f1c2b0e-8e0a-4b8e-9a51-0d6f1e2c3b4a", url: "https://app.dev.fast/s/capability", + version: JSON.parse(String(init!.body)).version, }); }, ); diff --git a/packages/review/app/src/share-control.tsx b/packages/review/app/src/share-control.tsx index f506da262d..dbf6b150e8 100644 --- a/packages/review/app/src/share-control.tsx +++ b/packages/review/app/src/share-control.tsx @@ -51,6 +51,12 @@ interface ShareTarget { requestId: string; } +interface SharedLink { + shareId: string; + url: string; + version: number; +} + const actionError = (error: Error | null) => error && (error instanceof ReviewApiError && error.status < 500 @@ -88,12 +94,11 @@ export function ShareControl({ const label = "Share review"; const tooltip = useTooltip(label); - // Links created this canvas, so reopening a shared version skips the upload. - const link = useQuery({ - queryKey: canvasQueryKeys.shareLink( - context?.reviewId ?? "", - target?.version, - ), + // The board's one link; a new version uploads behind the same URL. + const linkKey = canvasQueryKeys.shareLink(context?.reviewId ?? ""); + + const link = useQuery({ + queryKey: linkKey, queryFn: skipToken, staleTime: Infinity, gcTime: Infinity, @@ -108,15 +113,15 @@ export function ShareControl({ const publish = useMutation({ mutationFn: ({ version, requestId }: ShareTarget) => - context!.client.post<{ url: string }>("/sharing/publish", { + context!.client.post("/sharing/publish", { reviewId: context!.reviewId, version, requestId, }), - onSuccess: ({ url }, { version }) => - queryClient.setQueryData( - canvasQueryKeys.shareLink(context!.reviewId, version), - url, + // An older version's upload can land after a newer one's. + onSuccess: (shared) => + queryClient.setQueryData(linkKey, (cached) => + cached && cached.version > shared.version ? cached : shared, ), onError: (error) => { // The host forgot a stale login; show sign-in and upload again after it. @@ -156,10 +161,14 @@ export function ShareControl({ }, })); + const current = + link && target && link.version >= target.version ? link : undefined; + // Upload once per opening; a failure waits for Retry or a new sign-in. useEffect(() => { - if (open && signedIn && target && publish.isIdle && !link) upload(target); - }, [open, signedIn, target, publish.isIdle, link]); + if (open && signedIn && target && publish.isIdle && !current) + upload(target); + }, [open, signedIn, target, publish.isIdle, current]); if (!context) return null; @@ -209,17 +218,20 @@ export function ShareControl({

)} {signedIn ? ( - link ? ( + current ? ( <> event.target.select()} /> - ) : error ? ( diff --git a/packages/review/scripts/seed-share-fixture.ts b/packages/review/scripts/seed-share-fixture.ts index 208f0571d4..f78a02bc30 100644 --- a/packages/review/scripts/seed-share-fixture.ts +++ b/packages/review/scripts/seed-share-fixture.ts @@ -1,56 +1,47 @@ -/** Seeds a share's copy into `` for the shared-review e2e journey; the sender's repository is renamed away afterwards. */ -import { randomBytes } from "node:crypto"; -import { mkdir, rename, writeFile } from "node:fs/promises"; +/** Writes two revisions of one share into `` for the shared-review journey. */ +import { writeFile } from "node:fs/promises"; import path from "node:path"; -import { openLocalReviewStore } from "@review/review-api/local-data.js"; import { exportShare } from "@review/sharing/export.js"; -import { importShare, shareKey } from "@review/sharing/import.js"; -import { fetchPinnedRepository } from "@review/sharing/repository.js"; import { createShareFixture } from "../test/fixtures/share/create.js"; -const [root, home] = process.argv.slice(2).map((arg) => path.resolve(arg)); +const root = process.argv[2] && path.resolve(process.argv[2]); -if (!root || !home) - throw new Error("Usage: tsx scripts/seed-share-fixture.ts "); - -await mkdir(home, { recursive: true }); +if (!root) throw new Error("Usage: tsx scripts/seed-share-fixture.ts "); const fixture = await createShareFixture(root); -const bundle = await exportShare(fixture); - -const recipient = openLocalReviewStore(path.join(home, "review-api.db")); +const note = "The second revision adds this note."; -const reviewId = await importShare( - { - ...recipient, - fetchRepository: (target, _url, ...rest) => - fetchPinnedRepository(target, fixture.repo, ...rest), - }, - shareKey("https://app.dev.fast", randomBytes(32).toString("base64url")), - bundle, - { login: "fixture-sender", sharedAt: Date.now() }, +await writeFile( + path.join(root, "bundle-1.json"), + JSON.stringify(await exportShare(fixture)), ); -const { version } = recipient.store.read(reviewId); - -await recipient.data.close(); +await fixture.store.execute({ + operation: { + type: "edit", + reviewId: fixture.reviewId, + edit: { type: "insert", content: { type: "markdown", markdown: note } }, + }, +}); -recipient.store.close(); +await writeFile( + path.join(root, "bundle-2.json"), + JSON.stringify(await exportShare(fixture)), +); await fixture.data.close(); fixture.store.close(); -await rename(fixture.repo, path.join(root, "sender-unavailable")); - await writeFile( path.join(root, "fixture.json"), JSON.stringify({ - reviewId, - version, + reviewId: fixture.reviewId, + repo: fixture.repo, + note, sourceFile: fixture.sourceFile, sourceText: fixture.sourceText, }), diff --git a/packages/review/src/review-api/copied-board.test.ts b/packages/review/src/review-api/copied-board.test.ts index b9058a5369..9445c72928 100644 --- a/packages/review/src/review-api/copied-board.test.ts +++ b/packages/review/src/review-api/copied-board.test.ts @@ -91,7 +91,8 @@ it("keeps a copied board's blocks, lenses and provenance, reading in its own rep seed.document[2], ]); expect(snapshot.lenses).toEqual(seed.lenses); - expect(snapshot.shared).toEqual(seed.shared); + expect(snapshot.shared).toEqual({ ...seed.shared, importedVersion: 0 }); + expect(snapshot.version).toBe(0); expect(snapshot.origin?.branch).toBe("feature"); const { targetId } = await store.execute({ @@ -105,23 +106,33 @@ it("keeps a copied board's blocks, lenses and provenance, reading in its own rep expect(targetId).toBe("block-13"); }); -it("finds a copy by its link until it is deleted", async () => { - const { reviewId } = await copy(); +it("lists a link's copies newest first, Trash included, until each is deleted", async () => { + const { reviewId: first } = await copy(); + const { reviewId: second } = await copy(); + const shared = { ...seed.shared, importedVersion: 0 }; - expect(store.copyOf("link")).toBe(reviewId); - expect(store.copyOf("other")).toBeUndefined(); + expect(store.copiesOf("link")).toEqual([ + { reviewId: second, version: 0, shared }, + { reviewId: first, version: 0, shared }, + ]); + expect(store.copiesOf("other")).toEqual([]); await store.execute( - { operation: { type: "lifecycle", state: "trash", reviewId } }, + { operation: { type: "lifecycle", state: "trash", reviewId: first } }, undefined, { kind: "person" }, ); - expect(store.copyOf("link")).toBe(reviewId); + expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([ + second, + first, + ]); - await store.execute({ operation: { type: "delete", reviewId } }, undefined, { - kind: "person", - }); - expect(store.copyOf("link")).toBeUndefined(); + await store.execute( + { operation: { type: "delete", reviewId: first } }, + undefined, + { kind: "person" }, + ); + expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([second]); }); it("still checks the images of a board made from scratch", async () => { diff --git a/packages/review/src/review-api/http.ts b/packages/review/src/review-api/http.ts index 72e723a034..fecc481e99 100644 --- a/packages/review/src/review-api/http.ts +++ b/packages/review/src/review-api/http.ts @@ -2197,7 +2197,7 @@ export function createReviewApi( ...(threadId && { threadId }), }; - // A copy's kept commits go when it is deleted for good. + // A link's kept commits go when its last copy is deleted for good. const copy = input.operation.type === "delete" && store.has(input.operation.reviewId) ? store.read(input.operation.reviewId) @@ -2207,7 +2207,16 @@ export function createReviewApi( const key = copy?.shared?.key; const kept = copy && boardRepositoryId(copy); - if (result.deleted && key && kept) + if ( + result.deleted && + key && + kept && + !store + .copiesOf(key) + .some( + ({ reviewId }) => boardRepositoryId(store.read(reviewId)) === kept, + ) + ) await Promise.resolve() .then(() => forgetSharedCommits(store.repositoryPath(kept), key)) .catch((error) => diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 47ae2596d5..e91c9096c2 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -362,6 +362,11 @@ export interface SharedProvenance { sharedAt?: number; cloneUrl?: string; key?: string; + revisionId?: string; + /** The version import wrote; anything later is the receiver's edit. */ + importedVersion?: number; + /** Opened from a link naming a revision: never refreshed. */ + pinned?: boolean; } /** Content a new board starts with, as another board left it. */ @@ -1110,16 +1115,24 @@ export class ReviewStore { return run; } - /** The board copied from a share link, until it is deleted. */ - copyOf(key: string): string | undefined { - const row = this.db + /** The boards copied from a share link, newest first, until each is deleted. */ + copiesOf( + key: string, + ): { reviewId: string; version: number; shared: SharedProvenance }[] { + return this.db .prepare( - `SELECT reviews.id FROM reviews JOIN versions ON versions.review_id=reviews.id AND versions.version=reviews.version - WHERE CASE WHEN json_valid(versions.snapshot) THEN json_extract(versions.snapshot,'$.shared.key') END=?`, + `SELECT reviews.id, reviews.version, json_extract(versions.snapshot,'$.shared') AS shared + FROM reviews JOIN versions ON versions.review_id=reviews.id AND versions.version=reviews.version + WHERE CASE WHEN json_valid(versions.snapshot) THEN json_extract(versions.snapshot,'$.shared.key') END=? + ORDER BY reviews.rowid DESC`, ) - .get(key); - - return row && String(row.id); + .all(key) + .map((row) => ({ + reviewId: String(row.id), + version: Number(row.version), + // SAFETY: the key matched this row's seed. + shared: JSON.parse(String(row.shared)) as SharedProvenance, + })); } /** Checkouts whose remote is cloneUrl, most recently used first. */ checkoutsOf(cloneUrl: string): string[] { @@ -1600,6 +1613,50 @@ export class ReviewStore { author, ).then((source) => this.applyPreparedCommand(source, initial, author)); + return this.track(prepared); + } + + /** Replaces a share copy's content under the same board id; 409 once it moved past `baseVersion`. */ + refreshCopy( + reviewId: string, + baseVersion: number, + // oxlint-disable-next-line anti-slop/no-unknown-parameters -- Command boundary: commandSchema.parse below rejects malformed input before mutation. + create: unknown, + seed: Seed, + ): Promise { + if (this.closing) + return Promise.reject(new Error("Review store is closing.")); + const command = commandSchema.parse(create); + const op = command.operation; + + if ( + op.type !== "create" || + op.pullRequestUrl || + op.parentId || + op.dependsOn || + op.projectId + ) + throw new ReviewInputError( + "A refresh takes a create command with a title and target alone.", + ); + const person: Author = { kind: "person" }; + + return this.track( + this.prepareCommand( + command, + undefined, + Promise.resolve(undefined), + person, + ).then((source) => + this.applyPreparedCommand(source, seed, person, { + reviewId, + baseVersion, + }), + ), + ); + } + + private track(prepared: Promise) { this.preparing.add(prepared); void prepared .finally(() => this.preparing.delete(prepared)) @@ -1690,6 +1747,7 @@ export class ReviewStore { prepared: PreparedCommand, initial: Seed | undefined, author: Author, + into?: { reviewId: string; baseVersion: number }, ) { const { op, @@ -1703,6 +1761,7 @@ export class ReviewStore { return this.enqueue(async () => { if ( + !into && op.type === "create" && (op.pullRequestUrl ? op.reuseExisting !== false @@ -1774,9 +1833,19 @@ export class ReviewStore { fromTrash: true, }); - const id = op.type === "create" ? randomUUID() : op.reviewId; + const id = + into?.reviewId ?? (op.type === "create" ? randomUUID() : op.reviewId); - const previous = op.type === "create" ? undefined : this.read(id); + const previous = + op.type === "create" && !into ? undefined : this.read(id); + + if ( + into && + (!previous?.shared?.key || previous.shared.key !== initial?.shared?.key) + ) + throw new ReviewInputError( + "Only a copy of the same share link refreshes in place.", + ); let snapshot: Snapshot = op.type === "create" @@ -1994,9 +2063,11 @@ export class ReviewStore { restore: op.type === "restore", repin: op.type === "set_target", copied: initial !== undefined, + inPlace: into !== undefined, + baseVersion: into?.baseVersion, projectLive: op.type !== "restore" && !resolvedTarget, result: { - ...(op.type === "create" && { created: true }), + ...(op.type === "create" && !into && { created: true }), targetId: applied?.targetId ?? lensTarget?.targetId, ...(applied && { type: applied.type }), ...(lensTarget && { type: lensTarget.type }), @@ -2039,6 +2110,8 @@ export class ReviewStore { repin?: boolean; /** A board made from another's content, whose images stayed behind. */ copied?: boolean; + /** A share copy refreshed in place, in Trash or not. */ + inPlace?: boolean; projectLive: boolean; /** The ids a person's save gave new blocks, by client key. */ personIds?: Record; @@ -2104,6 +2177,9 @@ export class ReviewStore { })); warnings = [...(change.warnings ?? []), ...warnings]; + if (change.copied && snapshot.shared) + snapshot.shared.importedVersion = snapshot.version; + const result: Result = { ...change.result, reviewId: id, @@ -2156,7 +2232,8 @@ export class ReviewStore { previous ? () => { // A board in Trash is read-only, whoever writes. - this.assertWritable(id); + if (change.inPlace) this.assertExists(id); + else this.assertWritable(id); if (change.baseVersion !== undefined) assertBaseVersion(change.baseVersion, this.currentVersion(id)); diff --git a/packages/review/src/sharing/client.test.ts b/packages/review/src/sharing/client.test.ts index 1d857f502e..d951cb6688 100644 --- a/packages/review/src/sharing/client.test.ts +++ b/packages/review/src/sharing/client.test.ts @@ -5,6 +5,7 @@ import path from "node:path"; import { MAX_SHARE_BYTES, + type SharedMeta, createShareSchema, } from "@dev.fast/review-share-protocol"; import { ReviewInputError } from "@dev.fast/whiteboard-document"; @@ -96,45 +97,62 @@ it("surfaces a failed create and refuses an oversized share before any request", expect(send).not.toHaveBeenCalled(); }); +function sharedService(meta: SharedMeta, body: () => BodyInit) { + const requests: { path: string; headers: Headers }[] = []; + + const send: typeof fetch = async (input, init) => { + const url = new URL(String(input)); + requests.push({ + path: url.pathname + url.search, + headers: new Headers(init?.headers), + }); + + return url.pathname.endsWith("/bundle") + ? new Response(body()) + : Response.json(meta); + }; + + return { + requests, + client: new ShareClient("https://app.dev.fast", "secret", send), + }; +} + it("downloads a share with the link's capability within the size limit", async () => { const shared = await bundle(); const capability = "c".repeat(43); - const requests: { path: string; headers: Headers }[] = []; + const R = randomUUID(); + const L = randomUUID(); + + const meta = { + sender: { login: "sender" }, + sharedAt: 1, + size: 1, + revision: { id: R, version: 3, createdAt: 1 }, + latest: L, + }; - const client = (body: () => BodyInit) => - new ShareClient("https://app.dev.fast", "secret", async (input, init) => { - const url = new URL(String(input)); - requests.push({ - path: url.pathname, - headers: new Headers(init?.headers), - }); - - return url.pathname.endsWith("/bundle") - ? new Response(body()) - : Response.json({ - title: shared.title, - sender: { login: "sender" }, - sharedAt: 1, - size: 1, - }); - }); + const { requests, client } = sharedService(meta, () => + JSON.stringify(shared), + ); - expect( - await client(() => JSON.stringify(shared)).download(capability), - ).toEqual({ + expect(await client.download(capability)).toEqual({ bundle: shared, attribution: { login: "sender", sharedAt: 1 }, + revision: { id: R, version: 3, createdAt: 1 }, + latest: L, }); expect(requests.map(({ path }) => path)).toEqual([ `/api/v2/shared/${capability}`, - `/api/v2/shared/${capability}/bundle`, + `/api/v2/shared/${capability}/bundle?rev=${R}`, ]); for (const { headers } of requests) expect(headers.get("authorization")).toBeNull(); await expect( - client( + sharedService( + meta, () => new ReadableStream({ start(controller) { @@ -142,10 +160,31 @@ it("downloads a share with the link's capability within the size limit", async ( controller.close(); }, }), - ).download(capability), + ).client.download(capability), ).rejects.toThrow("limit"); }); +it("asks for a named revision", async () => { + const shared = await bundle(); + const capability = "c".repeat(43); + const R = randomUUID(); + + const meta = { + sender: { login: "sender" }, + sharedAt: 1, + size: 1, + revision: { id: R, version: 2, createdAt: 1 }, + latest: randomUUID(), + }; + + const { requests, client } = sharedService(meta, () => + JSON.stringify(shared), + ); + + await client.download(capability, R); + expect(requests[0]?.path).toBe(`/api/v2/shared/${capability}?rev=${R}`); +}); + it("stops a streamed response as soon as it exceeds the declared size", async () => { let cancelled = false; diff --git a/packages/review/src/sharing/client.ts b/packages/review/src/sharing/client.ts index 43a31065a1..e23d62a9d7 100644 --- a/packages/review/src/sharing/client.ts +++ b/packages/review/src/sharing/client.ts @@ -4,8 +4,14 @@ import { type JsonValue, parseJsonText } from "@dev.fast/json"; import { MAX_SHARE_BYTES, type ShareBundle, + type ShareRevisions, + type SharedMeta, capabilitySchema, shareBundleSchema, + shareCreatedSchema, + shareIdSchema, + shareRevisionsSchema, + sharedMetaSchema, } from "@dev.fast/review-share-protocol"; import { StreamLimitError, @@ -37,14 +43,6 @@ const jwtPayloadSchema = z.object({ exp: z.number() }); const bearers = new Map>(); -const linkSchema = z.strictObject({ shareId: z.uuid(), url: z.url() }); - -const receivedSchema = z.object({ - sender: z.strictObject({ login: z.string().min(1).max(256) }), - sharedAt: z.number(), - size: z.number().int().positive().max(MAX_SHARE_BYTES), -}); - /** Size is checked while reading, even if Content-Length is absent or false. */ export async function readBoundedBytes( response: Response, @@ -206,7 +204,7 @@ export class ShareClient { } async create(bundle: ShareBundle, requestId: string = randomUUID()) { - return linkSchema.parse( + return shareCreatedSchema.parse( await this.api( "/api/v2/shares", "POST", @@ -222,7 +220,7 @@ export class ShareClient { } async recoverLink(shareId: string) { - return linkSchema.parse( + return shareCreatedSchema.parse( await this.api(`/api/v2/shares/${z.uuid().parse(shareId)}/link`), ); } @@ -231,16 +229,35 @@ export class ShareClient { return this.api(`/api/v2/shares/${z.uuid().parse(shareId)}`, "DELETE"); } - /** The bundle and who shared it, read with the link's capability. */ - async download(capability: string) { - const route = `/api/v2/shared/${capabilitySchema.parse(capability)}`; + async revisions(shareId: string): Promise { + return shareRevisionsSchema.parse( + await this.api( + `/api/v2/shares/${shareIdSchema.parse(shareId)}/revisions`, + ), + ); + } + + /** Who shared a link's revision, and which revision is its latest. */ + async shared(capability: string, revision?: string): Promise { + const query = + revision === undefined ? "" : `?rev=${shareIdSchema.parse(revision)}`; - const received = receivedSchema.parse( - await this.api(route, "GET", undefined, true), + return sharedMetaSchema.parse( + await this.api( + `/api/v2/shared/${capabilitySchema.parse(capability)}${query}`, + "GET", + undefined, + true, + ), ); + } + + /** The bundle of the revision the meta names, and who shared it. */ + async download(capability: string, revision?: string) { + const meta = await this.shared(capability, revision); const bytes = await this.request( - `${route}/bundle`, + `/api/v2/shared/${capability}/bundle?rev=${meta.revision.id}`, "GET", undefined, true, @@ -251,10 +268,9 @@ export class ShareClient { bundle: shareBundleSchema.parse( parseJsonText(new TextDecoder().decode(bytes)), ), - attribution: { - login: received.sender.login, - sharedAt: received.sharedAt, - }, + attribution: { login: meta.sender.login, sharedAt: meta.sharedAt }, + revision: meta.revision, + latest: meta.latest, }; } } diff --git a/packages/review/src/sharing/host.test.ts b/packages/review/src/sharing/host.test.ts index 4aeb8bae51..c42eccb4a8 100644 --- a/packages/review/src/sharing/host.test.ts +++ b/packages/review/src/sharing/host.test.ts @@ -9,6 +9,7 @@ import { type runStoreLogin, writeStoreAuth, } from "@dev.fast/trace-core"; +import type { SharedProvenance } from "@review/review-api/store.js"; import { Hono } from "hono"; import { afterEach, expect, it, vi } from "vitest"; @@ -16,9 +17,27 @@ import { createShareFixture } from "../../test/fixtures/share/create.js"; import { ShareClient } from "./client.js"; import { exportShare } from "./export.js"; import { mountSharingHost } from "./host.js"; +import { shareKey } from "./import.js"; import { fetchPinnedRepository } from "./repository.js"; -afterEach(() => vi.unstubAllEnvs()); +afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); +}); + +const R = randomUUID(); + +const capability = "a".repeat(43); + +const url = `https://app.dev.fast/s/${capability}`; + +const meta = { + sender: { login: "sender" }, + sharedAt: 1, + size: 1, + revision: { id: R, version: 1, createdAt: 1 }, + latest: R, +}; it("exposes the device URL and code while login is pending and allows retry after failure", async () => { const root = await mkdtemp(path.join(tmpdir(), "share-login-")); @@ -100,6 +119,8 @@ it("reports download and checkout preparation, and opens the same copy when the }), ); + vi.spyOn(ShareClient.prototype, "shared").mockResolvedValue(meta); + let fetched!: () => void; const gate = new Promise((resolve) => { @@ -113,7 +134,6 @@ it("reports download and checkout preparation, and opens the same copy when the await fetchPinnedRepository(target, fixture.repo, ...rest); }, }); - const url = `https://app.dev.fast/s/${"a".repeat(43)}`; const start = () => api.request("/sharing/import", { @@ -132,8 +152,14 @@ it("reports download and checkout preparation, and opens the same copy when the expect(await status()).toMatchObject({ stage: "downloading" }); await start(); - expect(downloadSpy).toHaveBeenCalledTimes(1); - download({ bundle, attribution: { login: "sender", sharedAt: 1 } }); + await vi.waitFor(() => expect(downloadSpy).toHaveBeenCalledTimes(1)); + + download({ + bundle, + attribution: { login: "sender", sharedAt: 1 }, + revision: meta.revision, + latest: R, + }); await vi.waitFor(async () => expect(await status()).toMatchObject({ stage: "fetching" }), ); @@ -151,17 +177,131 @@ it("reports download and checkout preparation, and opens the same copy when the expect(fixture.store.read(copy).shared).toMatchObject({ login: "sender" }); expect((await start()).status).toBe(202); - expect(await status()).toMatchObject({ stage: "ready", reviewId: copy }); + await vi.waitFor(async () => + expect(await status()).toMatchObject({ stage: "ready", reviewId: copy }), + ); expect(downloadSpy).toHaveBeenCalledTimes(1); } finally { fetched(); - downloadSpy.mockRestore(); await fixture.data.close(); fixture.store.close(); await rm(root, { recursive: true, force: true }); } }); +async function importer() { + const root = await mkdtemp(path.join(tmpdir(), "share-import-")); + vi.stubEnv("DEV_REVIEW_HOME", root); + const fixture = await createShareFixture(root); + const bundle = await exportShare(fixture); + const api = new Hono(); + + mountSharingHost(api, fixture.store, fixture.data, { + fetchRepository: (target, _url, ...rest) => + fetchPinnedRepository(target, fixture.repo, ...rest), + }); + + const start = async (link = url) => + ( + await api.request("/sharing/import", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: link }), + }) + ).json(); + + const status = async (id: string) => + (await api.request(`/sharing/import/${id}`)).json(); + + const close = async () => { + await fixture.data.close(); + fixture.store.close(); + await rm(root, { recursive: true, force: true }); + }; + + return { bundle, start, status, close, store: fixture.store }; +} + +it("opens the newest copy when the share service cannot answer, else reports it unavailable", async () => { + const { bundle, start, status, close } = await importer(); + const sharedSpy = vi.spyOn(ShareClient.prototype, "shared"); + + vi.spyOn(ShareClient.prototype, "download").mockResolvedValue({ + bundle, + attribution: { login: "sender", sharedAt: 1 }, + revision: meta.revision, + latest: R, + }); + + try { + sharedSpy.mockRejectedValue(new Error("Share request failed (404).")); + + const { reviewId: id } = await start(); + + await vi.waitFor(async () => + expect(await status(id)).toMatchObject({ stage: "error" }), + ); + + sharedSpy.mockResolvedValue(meta); + await start(); + await vi.waitFor( + async () => expect(await status(id)).toMatchObject({ stage: "ready" }), + { timeout: 10000 }, + ); + + const { reviewId: copy } = await status(id); + + sharedSpy.mockRejectedValue(new Error("Share request failed (404).")); + await start(); + await vi.waitFor(async () => + expect(await status(id)).toMatchObject({ + stage: "ready", + reviewId: copy, + }), + ); + } finally { + await close(); + } +}); + +it("opens the copy a link prefers when the share service cannot answer", async () => { + const { start, status, close, store } = await importer(); + const key = shareKey("https://app.dev.fast", capability); + const [older, newer] = [randomUUID(), randomUUID()]; + + vi.spyOn(ShareClient.prototype, "shared").mockRejectedValue( + new Error("Share request failed (404)."), + ); + + const copy = async (shared: SharedProvenance) => + ( + await store.execute( + { operation: { type: "create", title: "Copy" } }, + { document: [], shared: { key, ...shared } }, + { kind: "person" }, + ) + ).reviewId; + + const opens = async (link: string, reviewId: string) => { + const { reviewId: id } = await start(link); + + await vi.waitFor(async () => + expect(await status(id)).toMatchObject({ stage: "ready", reviewId }), + ); + }; + + try { + const live = await copy({ revisionId: older }); + const pinned = await copy({ revisionId: newer, pinned: true }); + + await opens(url, live); + await opens(`${url}?rev=${older}`, live); + await opens(`${url}?rev=${randomUUID()}`, pinned); + } finally { + await close(); + } +}); + it("reports a revoke through onRevoked", async () => { const root = await mkdtemp(path.join(tmpdir(), "share-revoke-")); vi.stubEnv("DEV_REVIEW_HOME", root); diff --git a/packages/review/src/sharing/host.ts b/packages/review/src/sharing/host.ts index 39c8e53968..48eec35377 100644 --- a/packages/review/src/sharing/host.ts +++ b/packages/review/src/sharing/host.ts @@ -4,7 +4,7 @@ import path from "node:path"; import { Writable } from "node:stream"; import { - type ShareBundle, + type SharedMeta, parseShareLink, shareBundleSchema, shareIdSchema, @@ -33,9 +33,11 @@ import { whiteboardFileText, } from "./file.js"; import { - type ShareAttribution, + type ShareImport, importShare, needsRepository, + planShareCopy, + receiveShare, shareKey, } from "./import.js"; import { @@ -183,9 +185,20 @@ export function mountSharingHost( const imports = new Map(); - // One import at a time, so two links of one repository clone it once. + // One download at a time, so two links of one repository clone it once. let importing = Promise.resolve(); + const serially = (task: () => Promise) => { + const run = importing.then(task); + + importing = run.then( + () => {}, + () => {}, + ); + + return run; + }; + app.get("/sharing/import/:id", (context) => { const id = context.req.param("id"); @@ -201,54 +214,40 @@ export function mountSharingHost( }); }); + const shareInput = (onStage: ShareImport["onStage"]): ShareImport => ({ + store, + data, + fetchRepository: options.fetchRepository, + cloneRepository: options.cloneRepository, + onStage, + }); + const startImport = ( id: string, initial: "downloading" | "reading", - load: () => Promise<{ - bundle: ShareBundle; - attribution?: ShareAttribution; - }>, + receive: (onStage: ShareImport["onStage"]) => Promise, ) => { const known = imports.get(id)?.stage; - // A failed import retries, and a deleted copy imports again. + // A finished import asks again: its link may serve a newer revision. if (!known || known === "ready" || known === "error") { - const copy = store.copyOf(id); - - const status: ImportStatus = copy - ? { stage: "ready", reviewId: copy } - : { stage: initial }; + const status: ImportStatus = { stage: initial }; imports.set(id, status); - - if (!copy) - importing = importing - .then(async () => { - const { bundle, attribution } = await load(); - - status.reviewId = await importShare( - { - store, - data, - fetchRepository: options.fetchRepository, - cloneRepository: options.cloneRepository, - onStage: (stage) => { - status.stage = stage; - }, - }, - id, - bundle, - attribution, - ); - status.stage = "ready"; - }) - .catch((error) => { - status.stage = "error"; - status.error = - error instanceof ReviewInputError - ? error.message - : "This share is unavailable, revoked, or needs a newer Whiteboard version."; - }); + receive((stage) => { + status.stage = stage; + }) + .then((reviewId) => { + status.reviewId = reviewId; + status.stage = "ready"; + }) + .catch((error) => { + status.stage = "error"; + status.error = + error instanceof ReviewInputError + ? error.message + : "This share is unavailable, revoked, or needs a newer Whiteboard version."; + }); } const { reviewId: _reviewId, ...status } = imports.get(id)!; @@ -272,10 +271,25 @@ export function mountSharingHost( ); }); + const key = whiteboardFileKey(text); + const copy = () => store.copiesOf(key)[0]?.reviewId; + return context.json( - startImport(whiteboardFileKey(text), "reading", async () => ({ - bundle: parseWhiteboardFile(text), - })), + startImport( + key, + "reading", + async (onStage) => + copy() ?? + serially( + async () => + copy() ?? + importShare( + shareInput(onStage), + key, + parseWhiteboardFile(text), + ), + ), + ), 202, ); } @@ -292,11 +306,49 @@ export function mountSharingHost( 400, ); + const { origin, capability, revision } = parsed; + const key = shareKey(origin, capability); + const client = new ShareClient(origin, undefined, options.fetch); + return context.json( startImport( - shareKey(parsed.origin, parsed.capability), + revision ? `${key}-${revision}` : key, "downloading", - () => new ShareClient(parsed.origin).download(parsed.capability), + async (onStage) => { + let meta: SharedMeta; + + try { + meta = await client.shared(capability, revision); + } catch (error) { + // Offline, the copy the link prefers still opens. + const copies = store.copiesOf(key); + + const copy = + copies.find(({ shared }) => + revision ? shared.revisionId === revision : !shared.pinned, + ) ?? copies[0]; + + if (!copy) throw error; + + return copy.reviewId; + } + + const pinned = revision !== undefined; + const plan = planShareCopy(store.copiesOf(key), meta, pinned); + + // receiveShare plans again: the queue may have made a copy since. + return plan.kind === "open" + ? plan.reviewId + : serially(() => + receiveShare( + shareInput(onStage), + key, + meta, + () => client.download(capability, meta.revision.id), + pinned, + ), + ); + }, ), 202, ); @@ -502,7 +554,9 @@ export function mountSharingPublisher( "Set DEV_REVIEW_SHARE_TOKEN for CI, or run whiteboard login first.", 409, ); - await new ShareClient(account.origin, account.token).revoke(shareId); + await new ShareClient(account.origin, account.token, options.fetch).revoke( + shareId, + ); options.onRevoked?.({ shareId }); return context.json({ shareId, revoked: true }); diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index 6a08b75070..3e0502110d 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -13,6 +13,8 @@ import { createShareFixture } from "../../test/fixtures/share/create.js"; import { exportShare } from "./export.js"; import { importShare, + planShareCopy, + receiveShare, shareKey, sharePresentationSchema, sharedSnapshotSchema, @@ -30,6 +32,13 @@ afterEach(async () => { const key = () => shareKey("https://app.dev.fast", randomBytes(32).toString("base64url")); +const [R1old, R1, R2, R3] = ["r0", "r1", "r2", "r3"]; + +const served = (id: string, latest: string) => ({ + revision: { id, version: 1, createdAt: 1 }, + latest, +}); + /** A sender's share, with a block pinned to commits of its own, and a * recipient with an empty store. */ async function share() { @@ -91,7 +100,7 @@ it("copies a share into a board the recipient edits like their own", async () => { ...recipient, fetchRepository }, link, bundle, - { login: "ada", sharedAt: 1 }, + { login: "ada", sharedAt: 1, revisionId: R1 }, ); await rename(sender.repo, `${sender.repo}-gone`); @@ -110,6 +119,8 @@ it("copies a share into a board the recipient edits like their own", async () => sharedAt: 1, cloneUrl: sender.repository.cloneUrl, key: link, + revisionId: R1, + importedVersion: 0, }); expect(repositoryId).not.toBe(pins.repositoryId); expect(JSON.stringify(copy.document)).not.toContain(pins.repositoryId); @@ -159,40 +170,243 @@ it("copies a share into a board the recipient edits like their own", async () => expect(recipient.store.read(reviewId).title).toBe("Mine now"); }); -it("opens the copy a link already made, even from Trash", async () => { +it("matches a legacy copy by its share time and refreshes it only while unedited", () => { + const legacy = (version: number, sharedAt?: number) => [ + { reviewId: "l", version, shared: { key: "k", sharedAt } }, + ]; + + expect(planShareCopy(legacy(0), served(R2, R2))).toEqual({ + kind: "refresh", + reviewId: "l", + version: 0, + }); + expect(planShareCopy(legacy(1), served(R2, R2))).toEqual({ kind: "import" }); + expect(planShareCopy(legacy(1, 1), served(R1old, R2))).toEqual({ + kind: "open", + reviewId: "l", + }); +}); + +it("reopens, refreshes in place from Trash, and copies beside an edited board", async () => { const { bundle, recipient, fetchRepository } = await share(); + const input = { ...recipient, fetchRepository }; const link = key(); - const reviewId = await importShare( - { ...recipient, fetchRepository }, - link, + const download = vi.fn[3]>(async () => ({ bundle, + attribution: { login: "ada", sharedAt: 1 }, + })); + + const a = await receiveShare(input, link, served(R1, R1), download); + + expect(await receiveShare(input, link, served(R1, R1), download)).toBe(a); + expect(download).toHaveBeenCalledTimes(1); + + await recipient.store.execute( + { operation: { type: "lifecycle", state: "trash", reviewId: a } }, + undefined, + { kind: "person" }, ); + expect(await receiveShare(input, link, served(R2, R2), download)).toBe(a); + expect(recipient.store.read(a)).toMatchObject({ + version: 1, + trashedAt: expect.any(String), + shared: { revisionId: R2, importedVersion: 1 }, + }); + await recipient.store.execute( - { operation: { type: "lifecycle", state: "trash", reviewId } }, + { operation: { type: "lifecycle", state: "active", reviewId: a } }, + undefined, + { kind: "person" }, + ); + await recipient.store.execute( + { + operation: { + type: "edit", + reviewId: a, + edit: { + type: "insert", + content: { type: "markdown", markdown: "Mine" }, + }, + }, + }, undefined, { kind: "person" }, ); - expect( - await importShare({ ...recipient, fetchRepository }, link, bundle), - ).toBe(reviewId); + const b = await receiveShare(input, link, served(R3, R3), download); + + expect(b).not.toBe(a); + expect(recipient.store.copiesOf(link).map((x) => x.reviewId)).toEqual([b, a]); + + const other = await receiveShare(input, key(), served(R1, R1), download); + + expect(recipient.store.read(other).pins!.repositoryId).toBe( + recipient.store.read(a).pins!.repositoryId, + ); expect(fetchRepository).toHaveBeenCalledTimes(1); - expect(recipient.store.list().map((board) => board.reviewId)).toEqual([ - reviewId, - ]); +}); - const other = await importShare( - { ...recipient, fetchRepository }, - key(), +it("keeps a revision link's copy pinned, apart from the live copy's refreshes", async () => { + const { bundle, recipient, fetchRepository } = await share(); + const input = { ...recipient, fetchRepository }; + const link = key(); + + const download = async () => ({ bundle, + attribution: { login: "ada", sharedAt: 1 }, + }); + + const a = await receiveShare(input, link, served(R1, R1), download); + + const pinned = await receiveShare( + input, + link, + served(R2, R2), + download, + true, ); - expect(other).not.toBe(reviewId); - expect(recipient.store.read(other).pins!.repositoryId).toBe( - recipient.store.read(reviewId).pins!.repositoryId, + expect(pinned).not.toBe(a); + expect(recipient.store.read(pinned).shared).toMatchObject({ + revisionId: R2, + pinned: true, + }); + expect(recipient.store.read(a).shared).toMatchObject({ revisionId: R1 }); + + expect(await receiveShare(input, link, served(R3, R3), download)).toBe(a); + expect(recipient.store.read(a).shared).toMatchObject({ revisionId: R3 }); + expect(recipient.store.read(pinned).shared).toMatchObject({ + revisionId: R2, + }); +}); + +it("copies beside a board edited or deleted while its refresh downloads", async () => { + const { bundle, recipient, fetchRepository } = await share(); + const input = { ...recipient, fetchRepository }; + const app = createReviewApi(recipient.store, recipient.data); + const link = key(); + let during = async () => {}; + + const download = async () => { + await during(); + + return { bundle, attribution: { login: "ada", sharedAt: 1 } }; + }; + + const a = await receiveShare(input, link, served(R1, R1), download); + + during = async () => { + await recipient.store.execute( + { + operation: { + type: "edit", + reviewId: a, + edit: { + type: "insert", + content: { type: "markdown", markdown: "Mine" }, + }, + }, + }, + undefined, + { kind: "person" }, + ); + }; + + const edited = recipient.store.read(a); + const b = await receiveShare(input, link, served(R2, R2), download); + const after = recipient.store.read(a); + + expect(b).not.toBe(a); + expect(after.version).toBe(edited.version + 1); + expect(after.shared).toMatchObject({ revisionId: R1 }); + expect(JSON.stringify(after.document)).toContain("Mine"); + + during = async () => { + await recipient.store.execute( + { operation: { type: "lifecycle", state: "trash", reviewId: b } }, + undefined, + { kind: "person" }, + ); + + const response = await app.request("/commands", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ operation: { type: "delete", reviewId: b } }), + }); + + expect(response.status).toBe(200); + }; + + const c = await receiveShare(input, link, served(R3, R3), download); + + expect([a, b]).not.toContain(c); + expect(recipient.store.read(c).shared).toMatchObject({ revisionId: R3 }); + expect(recipient.store.copiesOf(link).map((x) => x.reviewId)).toEqual([c, a]); +}); + +it("keeps a share's commits until its link's last copy is deleted", async () => { + const { bundle, recipient, fetchRepository, pins } = await share(); + const input = { ...recipient, fetchRepository }; + const link = key(); + + const download = async () => ({ + bundle, + attribution: { login: "ada", sharedAt: 1 }, + }); + + const a = await receiveShare(input, link, served(R1, R1), download); + + await recipient.store.execute( + { + operation: { + type: "edit", + reviewId: a, + edit: { + type: "insert", + content: { type: "markdown", markdown: "Mine" }, + }, + }, + }, + undefined, + { kind: "person" }, ); + + const b = await receiveShare(input, link, served(R2, R2), download); + + const root = recipient.store.repositoryPath( + recipient.store.read(a).pins!.repositoryId, + ); + + const git = (...args: string[]) => + execFileSync("git", args, { cwd: root, encoding: "utf8" }).trim(); + + const app = createReviewApi(recipient.store, recipient.data); + + const remove = async (reviewId: string) => { + await recipient.store.execute( + { operation: { type: "lifecycle", state: "trash", reviewId } }, + undefined, + { kind: "person" }, + ); + + const response = await app.request("/commands", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ operation: { type: "delete", reviewId } }), + }); + + expect(response.status).toBe(200); + }; + + await remove(a); + expect(git("rev-parse", `refs/review/shared/${link}/${pins.head}`)).toBe( + pins.head, + ); + + await remove(b); + expect(git("for-each-ref", `refs/review/shared/${link}`)).toBe(""); }); it("fetches the shared commits into the recipient's own checkout", async () => { diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 57aa24aea1..81ae50bb86 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -21,7 +21,10 @@ import { import { textIncludesQuote } from "@review/evidence.js"; import type { LocalReviewData } from "@review/review-api/local-data.js"; import { pullRequestUrl } from "@review/review-api/origin.js"; -import type { ReviewStore } from "@review/review-api/store.js"; +import type { + ReviewStore, + SharedProvenance, +} from "@review/review-api/store.js"; import { traceSchema } from "@review/review-api/trace-schema.js"; import { devReviewHome, @@ -235,18 +238,99 @@ export interface ShareImport { onStage?: (stage: "fetching" | "validating") => void; } -/** Imports a share as an ordinary board, once per link: the link's copy - * comes back while it exists, even from Trash. */ +export const FRESH_IMPORT_VERSION = 0; + +/** The revision a link serves and its share's latest. */ +export interface ServedRevision { + revision: { id: string; version: number; createdAt: number }; + latest: string; +} + +export type ShareCopyPlan = + | { kind: "open"; reviewId: string } + | { kind: "refresh"; reviewId: string; version: number } + | { kind: "import" }; + +/** Which copy a served revision opens or refreshes in place; otherwise a new copy. */ +export function planShareCopy( + copies: ReturnType, + served: ServedRevision, + pinned = false, +): ShareCopyPlan { + const same = copies.find(({ shared }) => + shared.revisionId === undefined + ? shared.sharedAt === served.revision.createdAt + : shared.revisionId === served.revision.id, + ); + + if (same) return { kind: "open", reviewId: same.reviewId }; + + const unedited = + !pinned && + served.revision.id === served.latest && + copies.find( + (copy) => + !copy.shared.pinned && + copy.version === (copy.shared.importedVersion ?? FRESH_IMPORT_VERSION), + ); + + return unedited + ? { + kind: "refresh", + reviewId: unedited.reviewId, + version: unedited.version, + } + : { kind: "import" }; +} + +/** Opens, refreshes or copies a link's board by the revision it serves. */ +export async function receiveShare( + input: ShareImport, + key: string, + served: ServedRevision, + download: () => Promise<{ + bundle: ShareBundle; + attribution: ShareAttribution; + }>, + pinned = false, +): Promise { + const plan = planShareCopy(input.store.copiesOf(key), served, pinned); + + if (plan.kind === "open") return plan.reviewId; + const { bundle, attribution } = await download(); + + const provenance = { + ...attribution, + revisionId: served.revision.id, + ...(pinned && { pinned }), + }; + + if (plan.kind === "import") + return importShare(input, key, bundle, provenance); + + try { + return await importShare(input, key, bundle, provenance, plan); + } catch (error) { + // The board was edited or deleted since the plan read it. + if ( + error instanceof ReviewInputError && + (error.status === 409 || error.status === 404) + ) + return importShare(input, key, bundle, provenance); + throw error; + } +} + +/** Imports a share as an ordinary board, or into a copy of its link. */ export async function importShare( input: ShareImport, key: string, received: ShareBundle, - attribution?: ShareAttribution, + provenance?: ShareAttribution & + Pick, + into?: { reviewId: string; version: number }, ): Promise { const { store, data } = input; - const existing = store.copyOf(key); - - if (existing) return existing; const { bundle, snapshot, maps } = validateShareBundle(received); const cloneUrl = bundle.repository?.cloneUrl; let repositoryId: string | undefined; @@ -319,27 +403,29 @@ export async function importShare( for (const [id, copy] of ids) document = document.replaceAll(id, copy); - const { reviewId } = await store.execute( - { - operation: { - type: "create", - ...(snapshot.title && { title: snapshot.title }), - ...(pins - ? { target: { kind: "commits", ...pins } } - : repositoryId && { - target: { kind: "project", repositoryId }, - }), - }, - }, - { - document: JSON.parse(document), - lenses: snapshot.lenses, - origin: snapshot.origin, - shared: { ...attribution, cloneUrl, key }, - copiedFrom: snapshot.reviewId, + const create = { + operation: { + type: "create", + ...(snapshot.title && { title: snapshot.title }), + ...(pins + ? { target: { kind: "commits", ...pins } } + : repositoryId && { + target: { kind: "project", repositoryId }, + }), }, - { kind: "person" }, - ); + }; + + const seed = { + document: JSON.parse(document), + lenses: snapshot.lenses, + origin: snapshot.origin, + shared: { ...provenance, cloneUrl, key }, + copiedFrom: snapshot.reviewId, + }; + + const { reviewId } = into + ? await store.refreshCopy(into.reviewId, into.version, create, seed) + : await store.execute(create, seed, { kind: "person" }); return reviewId; }