From debb19c267980c522c0033fb4df014adf2c71a1d Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:06:57 -0500 Subject: [PATCH 01/17] Carry a share revision in share links Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../review-share-protocol/src/index.test.ts | 27 ++++++++++++++++ packages/review-share-protocol/src/index.ts | 31 ++++++++++++++++--- 2 files changed, 53 insertions(+), 5 deletions(-) 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..29096e741a 100644 --- a/packages/review-share-protocol/src/index.ts +++ b/packages/review-share-protocol/src/index.ts @@ -139,11 +139,26 @@ 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 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 +166,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; } From daef1da90b59d91d33adc37dc499d005a7dabab6 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:07:20 -0500 Subject: [PATCH 02/17] Add share revision schemas and release review-share-protocol 0.5.0 Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review-share-protocol/package.json | 2 +- .../review-share-protocol/src/index.test.ts | 35 +++++++++++++++++++ packages/review-share-protocol/src/index.ts | 35 +++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) 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 1d268e01c3..cb51072c59 100644 --- a/packages/review-share-protocol/src/index.test.ts +++ b/packages/review-share-protocol/src/index.test.ts @@ -6,8 +6,11 @@ import { normalizeGitHubRemote, parseShareLink, shareBundleSchema, + shareCreatedSchema, shareLink, sharePathSchema, + shareRevisionsSchema, + sharedMetaSchema, } from "./index.js"; it("puts the capability in the path and rejects credentials, fragments or other paths", () => { @@ -56,6 +59,38 @@ it("carries a revision as ?rev= and rejects any other query", () => { ); }); +it("reads a link's revisions and the revision a capability serves", () => { + const id = "5b0e9a52-4a43-4f7e-9d8e-3c1f0e7b2a11", + url = `https://app.dev.fast/s/${"a".repeat(43)}?rev=${id}`; + + expect( + shareRevisionsSchema.parse({ + latest: id, + items: [{ id, version: 3, size: 10, createdAt: 1, url, extra: true }], + }).items[0], + ).toEqual({ id, version: 3, size: 10, createdAt: 1, url }); + expect( + sharedMetaSchema.safeParse({ + sender: { login: "ada" }, + sharedAt: 1, + size: 10, + }).success, + ).toBe(false); + expect( + sharedMetaSchema.parse({ + title: "T", + sender: { login: "ada" }, + sharedAt: 1, + size: 10, + revision: { id, version: 3, createdAt: 1 }, + latest: id, + }).revision.id, + ).toBe(id); + expect( + shareCreatedSchema.safeParse({ shareId: id, url, revision: id }).success, + ).toBe(false); +}); + 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 29096e741a..9f1dd072eb 100644 --- a/packages/review-share-protocol/src/index.ts +++ b/packages/review-share-protocol/src/index.ts @@ -139,6 +139,41 @@ export const createShareSchema = z.object({ bundle: shareBundleSchema, }); +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, From 24ec43c8928073aba2e2c9e9d6251e505acb1ba6 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:10:28 -0500 Subject: [PATCH 03/17] Read share revisions and download the revision the meta names Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/src/sharing/client.test.ts | 134 +++++++++++++++++---- packages/review/src/sharing/client.ts | 56 ++++++--- packages/review/src/sharing/host.test.ts | 9 +- 3 files changed, 154 insertions(+), 45 deletions(-) diff --git a/packages/review/src/sharing/client.test.ts b/packages/review/src/sharing/client.test.ts index 1d857f502e..15a884eb3b 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,63 @@ 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 = vi.fn(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, + send, + 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 +161,77 @@ 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 and refuses a malformed one before any request", 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}`); + + const refused = sharedService(meta, () => JSON.stringify(shared)); + + await expect(refused.client.shared(capability, "nope")).rejects.toThrow( + "UUID", + ); + expect(refused.send).not.toHaveBeenCalled(); +}); + +it("reads a link's revisions with the account's token", async () => { + const shareId = randomUUID(); + const R1 = randomUUID(); + const R2 = randomUUID(); + const u1 = `https://app.dev.fast/s/${"x".repeat(43)}?rev=${R1}`; + const u2 = `https://app.dev.fast/s/${"x".repeat(43)}?rev=${R2}`; + const requests: string[] = []; + + const revisions = { + latest: R2, + items: [ + { id: R2, version: 5, size: 20, createdAt: 2, url: u2 }, + { id: R1, version: 4, size: 10, createdAt: 1, url: u1 }, + ], + }; + + const client = new ShareClient( + "https://app.dev.fast", + "secret", + async (input, init) => { + const { pathname } = new URL(String(input)); + + if (pathname === "/api/auth/token") + return new Response(null, { status: 404 }); + requests.push( + `${init?.method ?? "GET"} ${pathname} ${new Headers(init?.headers).get("authorization")}`, + ); + + return Response.json(revisions); + }, + ); + + expect(await client.revisions(shareId)).toEqual(revisions); + expect(requests).toEqual([ + `GET /api/v2/shares/${shareId}/revisions Bearer secret`, + ]); + await expect(client.revisions("x")).rejects.toThrow("UUID"); +}); + 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..c4e287186f 100644 --- a/packages/review/src/sharing/host.test.ts +++ b/packages/review/src/sharing/host.test.ts @@ -133,7 +133,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 } }); + const shareId = randomUUID(); + + download({ + bundle, + attribution: { login: "sender", sharedAt: 1 }, + revision: { id: shareId, version: 0, createdAt: 1 }, + latest: shareId, + }); await vi.waitFor(async () => expect(await status()).toMatchObject({ stage: "fetching" }), ); From 69f91be27f6ea1810d6855e53ae6e029a5410c49 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:12:06 -0500 Subject: [PATCH 04/17] List every copy of a share link and record the version each import made Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../src/review-api/copied-board.test.ts | 46 ++++++++++++++----- packages/review/src/review-api/store.ts | 31 +++++++++---- packages/review/src/sharing/host.ts | 2 +- packages/review/src/sharing/import.test.ts | 1 + packages/review/src/sharing/import.ts | 2 +- 5 files changed, 61 insertions(+), 21 deletions(-) diff --git a/packages/review/src/review-api/copied-board.test.ts b/packages/review/src/review-api/copied-board.test.ts index b9058a5369..dabae836b6 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,46 @@ 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: "edit", + reviewId: first, + edit: { type: "insert", content: { type: "divider" } }, + }, + }); + expect(store.copiesOf("link")[1]).toEqual({ + reviewId: first, + version: 1, + shared, + }); 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/store.ts b/packages/review/src/review-api/store.ts index 47ae2596d5..827956a6ef 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -362,6 +362,10 @@ export interface SharedProvenance { sharedAt?: number; cloneUrl?: string; key?: string; + /** The link revision the board was made or last refreshed from. */ + revisionId?: string; + /** The version that import wrote; a later version is the receiver's edit. */ + importedVersion?: number; } /** Content a new board starts with, as another board left it. */ @@ -1110,16 +1114,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, so `shared` is the object a seed wrote. + shared: JSON.parse(String(row.shared)) as SharedProvenance, + })); } /** Checkouts whose remote is cloneUrl, most recently used first. */ checkoutsOf(cloneUrl: string): string[] { @@ -2104,6 +2116,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, diff --git a/packages/review/src/sharing/host.ts b/packages/review/src/sharing/host.ts index 39c8e53968..528f5353c4 100644 --- a/packages/review/src/sharing/host.ts +++ b/packages/review/src/sharing/host.ts @@ -213,7 +213,7 @@ export function mountSharingHost( // A failed import retries, and a deleted copy imports again. if (!known || known === "ready" || known === "error") { - const copy = store.copyOf(id); + const copy = store.copiesOf(id)[0]?.reviewId; const status: ImportStatus = copy ? { stage: "ready", reviewId: copy } diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index 6a08b75070..4db9a9f265 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -110,6 +110,7 @@ it("copies a share into a board the recipient edits like their own", async () => sharedAt: 1, cloneUrl: sender.repository.cloneUrl, key: link, + importedVersion: 0, }); expect(repositoryId).not.toBe(pins.repositoryId); expect(JSON.stringify(copy.document)).not.toContain(pins.repositoryId); diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 57aa24aea1..819f1af11d 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -244,7 +244,7 @@ export async function importShare( attribution?: ShareAttribution, ): Promise { const { store, data } = input; - const existing = store.copyOf(key); + const existing = store.copiesOf(key)[0]?.reviewId; if (existing) return existing; const { bundle, snapshot, maps } = validateShareBundle(received); From af84acdab8040ccb48c57e69d42b4a76076afdb7 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:14:11 -0500 Subject: [PATCH 05/17] Refresh a share copy in place Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../src/review-api/copied-board.test.ts | 48 ++++++++++++++ packages/review/src/review-api/store.ts | 62 +++++++++++++++++-- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/packages/review/src/review-api/copied-board.test.ts b/packages/review/src/review-api/copied-board.test.ts index dabae836b6..239193c405 100644 --- a/packages/review/src/review-api/copied-board.test.ts +++ b/packages/review/src/review-api/copied-board.test.ts @@ -148,6 +148,54 @@ it("lists a link's copies newest first, Trash included, until each is deleted", expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([second]); }); +it("refreshes a copy in place with a newer share, even in Trash, and refuses once it was edited", async () => { + const { reviewId } = await copy(); + + await store.execute( + { operation: { type: "lifecycle", state: "trash", reviewId } }, + undefined, + { kind: "person" }, + ); + + const next: Seed = { + ...structuredClone(seed), + document: [ + { id: "markdown-3", type: "markdown", markdown: "Why, revised" }, + ], + shared: { ...seed.shared, revisionId: "r2", sharedAt: 2 }, + }; + + const create = { + operation: { + type: "create", + title: "Copy v2", + target: { kind: "commits", ...pins }, + }, + }; + + const result = await store.refreshCopy(reviewId, 0, create, next); + + expect(result).toMatchObject({ reviewId, version: 1 }); + + const snapshot = store.read(reviewId); + + expect(snapshot).toMatchObject({ + title: "Copy v2", + trashedAt: expect.any(String), + shared: { + revisionId: "r2", + sharedAt: 2, + key: "link", + importedVersion: 1, + }, + }); + expect(snapshot.document).toEqual(next.document); + expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([reviewId]); + await expect( + store.refreshCopy(reviewId, 0, create, next), + ).rejects.toMatchObject({ status: 409 }); +}); + it("still checks the images of a board made from scratch", async () => { const { reviewId } = await store.execute({ operation: { diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 827956a6ef..e7c27ee046 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -1612,6 +1612,51 @@ export class ReviewStore { author, ).then((source) => this.applyPreparedCommand(source, initial, author)); + return this.track(prepared); + } + + /** Replaces a share copy's content with a newer revision of its link: same board id, Trash state kept. + * Refuses (409) once the board has 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)) @@ -1702,6 +1747,8 @@ export class ReviewStore { prepared: PreparedCommand, initial: Seed | undefined, author: Author, + /** A share copy whose content the create replaces in place. */ + into?: { reviewId: string; baseVersion: number }, ) { const { op, @@ -1715,6 +1762,7 @@ export class ReviewStore { return this.enqueue(async () => { if ( + !into && op.type === "create" && (op.pullRequestUrl ? op.reuseExisting !== false @@ -1786,9 +1834,11 @@ 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); let snapshot: Snapshot = op.type === "create" @@ -2006,9 +2056,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 }), @@ -2051,6 +2103,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; @@ -2171,7 +2225,7 @@ export class ReviewStore { previous ? () => { // A board in Trash is read-only, whoever writes. - this.assertWritable(id); + if (!change.inPlace) this.assertWritable(id); if (change.baseVersion !== undefined) assertBaseVersion(change.baseVersion, this.currentVersion(id)); From 1f66658db01b4867fdcbc82b88fe45049a7d1b02 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:19:43 -0500 Subject: [PATCH 06/17] Open, refresh or copy a shared board by the revision its link serves Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../src/review-api/copied-board.test.ts | 29 +++++ packages/review/src/review-api/store.ts | 11 +- packages/review/src/sharing/import.test.ts | 111 ++++++++++++---- packages/review/src/sharing/import.ts | 121 ++++++++++++++---- 4 files changed, 223 insertions(+), 49 deletions(-) diff --git a/packages/review/src/review-api/copied-board.test.ts b/packages/review/src/review-api/copied-board.test.ts index 239193c405..b5b7c63de3 100644 --- a/packages/review/src/review-api/copied-board.test.ts +++ b/packages/review/src/review-api/copied-board.test.ts @@ -196,6 +196,35 @@ it("refreshes a copy in place with a newer share, even in Trash, and refuses onc ).rejects.toMatchObject({ status: 409 }); }); +it("refreshes only a copy of the same link", async () => { + const { reviewId } = await copy(); + + const { reviewId: own } = await store.execute({ + operation: { + type: "create", + title: "Own", + target: { kind: "commits", ...pins }, + }, + }); + + const create = { + operation: { + type: "create", + title: "Copy v2", + target: { kind: "commits", ...pins }, + }, + }; + + const other = { ...seed, shared: { ...seed.shared, key: "other" } }; + + await expect(store.refreshCopy(own, 0, create, seed)).rejects.toMatchObject({ + status: 400, + }); + await expect( + store.refreshCopy(reviewId, 0, create, other), + ).rejects.toMatchObject({ status: 400 }); +}); + it("still checks the images of a board made from scratch", async () => { const { reviewId } = await store.execute({ operation: { diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index e7c27ee046..805ee32f60 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -1840,6 +1840,14 @@ export class ReviewStore { 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" ? createdSnapshot( @@ -2225,7 +2233,8 @@ export class ReviewStore { previous ? () => { // A board in Trash is read-only, whoever writes. - if (!change.inPlace) 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/import.test.ts b/packages/review/src/sharing/import.test.ts index 4db9a9f265..d0bba5c05c 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,7 @@ 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); @@ -160,40 +170,97 @@ 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("plans the three rules", () => { + const c = (over: object) => ({ + reviewId: "a", + version: 3, + shared: { key: "k", revisionId: R1, importedVersion: 3, ...over }, + }); + + expect( + planShareCopy([c({})], { + revision: { id: R1, version: 4, createdAt: 1 }, + latest: R2, + }), + ).toEqual({ kind: "open", reviewId: "a" }); + expect(planShareCopy([c({})], served(R2, R2))).toEqual({ + kind: "refresh", + reviewId: "a", + version: 3, + }); + expect(planShareCopy([{ ...c({}), version: 4 }], served(R2, R2))).toEqual({ + kind: "import", + }); + expect(planShareCopy([c({})], served(R1old, R2))).toEqual({ + kind: "import", + }); + expect( + planShareCopy( + [{ reviewId: "l", version: 0, shared: { key: "k" } }], + served(R2, R2), + ), + ).toEqual({ kind: "refresh", reviewId: "l", version: 0 }); + expect( + planShareCopy( + [{ reviewId: "l", version: 1, shared: { key: "k" } }], + served(R2, R2), + ), + ).toEqual({ kind: "import" }); +}); + +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(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 } }, + { operation: { type: "lifecycle", state: "trash", reviewId: a } }, undefined, { kind: "person" }, ); - expect( - await importShare({ ...recipient, fetchRepository }, link, bundle), - ).toBe(reviewId); - expect(fetchRepository).toHaveBeenCalledTimes(1); - expect(recipient.store.list().map((board) => board.reviewId)).toEqual([ - reviewId, - ]); + 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 }, + }); - const other = await importShare( - { ...recipient, fetchRepository }, - key(), - bundle, + await recipient.store.execute( + { operation: { type: "lifecycle", state: "active", reviewId: a } }, + undefined, + { kind: "person" }, ); - - expect(other).not.toBe(reviewId); - expect(recipient.store.read(other).pins!.repositoryId).toBe( - recipient.store.read(reviewId).pins!.repositoryId, + 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(R3, R3), download); + + expect(b).not.toBe(a); + expect(recipient.store.copiesOf(link).map((x) => x.reviewId)).toEqual([b, a]); + expect(fetchRepository).toHaveBeenCalledTimes(1); }); 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 819f1af11d..0d5ae0a3eb 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -235,18 +235,85 @@ 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" }; + +/** A copy made from the served revision opens; an unedited copy takes the + * latest revision in place; anything else gets a new copy. */ +export function planShareCopy( + copies: ReturnType, + served: ServedRevision, +): ShareCopyPlan { + const same = copies.find( + (copy) => copy.shared.revisionId === served.revision.id, + ); + + if (same) return { kind: "open", reviewId: same.reviewId }; + + const unedited = + served.revision.id === served.latest && + copies.find( + (copy) => + 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; + }>, +): Promise { + const plan = planShareCopy(input.store.copiesOf(key), served); + + if (plan.kind === "open") return plan.reviewId; + const { bundle, attribution } = await download(); + const provenance = { ...attribution, revisionId: served.revision.id }; + + 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 since the plan read it. + if (error instanceof ReviewInputError && error.status === 409) + 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 & { revisionId?: string }, + into?: { reviewId: string; version: number }, ): Promise { const { store, data } = input; - const existing = store.copiesOf(key)[0]?.reviewId; - - if (existing) return existing; const { bundle, snapshot, maps } = validateShareBundle(received); const cloneUrl = bundle.repository?.cloneUrl; let repositoryId: string | undefined; @@ -319,27 +386,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 }, - }), - }, + const create = { + 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, - }, - { 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; } From 10820ddad12731bc1238b569a45a2d14e6b55bab Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:20:27 -0500 Subject: [PATCH 07/17] Keep a share's commits while another copy of its link remains Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/src/review-api/http.ts | 4 +- packages/review/src/sharing/import.test.ts | 62 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/review/src/review-api/http.ts b/packages/review/src/review-api/http.ts index 72e723a034..a6227d0067 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,7 @@ 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).length) await Promise.resolve() .then(() => forgetSharedCommits(store.repositoryPath(kept), key)) .catch((error) => diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index d0bba5c05c..2fb21f6050 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -263,6 +263,68 @@ it("reopens, refreshes in place from Trash, and copies beside an edited board", expect(fetchRepository).toHaveBeenCalledTimes(1); }); +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 () => { const { root, sender, pins, bundle, recipient, fetchRepository } = await share(); From 5c1f8688853e31aed98836b09a6888e37a166b5c Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:23:50 -0500 Subject: [PATCH 08/17] Ask the share service which revision a link serves before opening its copy Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/src/sharing/host.test.ts | 141 +++++++++++++++++++-- packages/review/src/sharing/host.ts | 108 +++++++++------- packages/review/src/sharing/import.test.ts | 6 +- 3 files changed, 199 insertions(+), 56 deletions(-) diff --git a/packages/review/src/sharing/host.test.ts b/packages/review/src/sharing/host.test.ts index c4e287186f..8a21a0c46c 100644 --- a/packages/review/src/sharing/host.test.ts +++ b/packages/review/src/sharing/host.test.ts @@ -16,9 +16,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 +118,10 @@ it("reports download and checkout preparation, and opens the same copy when the }), ); + const sharedSpy = vi + .spyOn(ShareClient.prototype, "shared") + .mockResolvedValue(meta); + let fetched!: () => void; const gate = new Promise((resolve) => { @@ -113,7 +135,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,14 +153,13 @@ 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); - const shareId = randomUUID(); + await vi.waitFor(() => expect(downloadSpy).toHaveBeenCalledTimes(1)); download({ bundle, attribution: { login: "sender", sharedAt: 1 }, - revision: { id: shareId, version: 0, createdAt: 1 }, - latest: shareId, + revision: meta.revision, + latest: R, }); await vi.waitFor(async () => expect(await status()).toMatchObject({ stage: "fetching" }), @@ -158,17 +178,122 @@ 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(sharedSpy).toHaveBeenCalledTimes(2); expect(downloadSpy).toHaveBeenCalledTimes(1); + expect(downloadSpy).toHaveBeenCalledWith(capability, R); } 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 }; +} + +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", + error: + "This share is unavailable, revoked, or needs a newer Whiteboard version.", + }), + ); + + 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, + }), + ); + expect(sharedSpy).toHaveBeenCalledTimes(3); + } finally { + await close(); + } +}); + +it("keeps a revision link's import apart from the live link's", async () => { + const { start, status, close } = await importer(); + + const sharedSpy = vi + .spyOn(ShareClient.prototype, "shared") + .mockRejectedValue(new Error("Share request failed (404).")); + + try { + const { reviewId } = await start(`${url}?rev=${R}`); + + expect(reviewId).toBe( + `${shareKey("https://app.dev.fast", capability)}-${R}`, + ); + await vi.waitFor(async () => + expect(await status(reviewId)).toMatchObject({ stage: "error" }), + ); + expect(sharedSpy).toHaveBeenCalledWith(capability, R); + } 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 528f5353c4..81367c72dc 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,10 @@ import { whiteboardFileText, } from "./file.js"; import { - type ShareAttribution, + type ShareImport, importShare, needsRepository, + receiveShare, shareKey, } from "./import.js"; import { @@ -201,54 +202,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.copiesOf(id)[0]?.reviewId; - - 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."; + importing = importing + .then(async () => { + status.reviewId = await receive((stage) => { + status.stage = stage; }); + 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 +259,16 @@ export function mountSharingHost( ); }); + const key = whiteboardFileKey(text); + return context.json( - startImport(whiteboardFileKey(text), "reading", async () => ({ - bundle: parseWhiteboardFile(text), - })), + startImport( + key, + "reading", + async (onStage) => + store.copiesOf(key)[0]?.reviewId ?? + importShare(shareInput(onStage), key, parseWhiteboardFile(text)), + ), 202, ); } @@ -292,11 +285,32 @@ 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 link's newest copy still opens. + const copy = store.copiesOf(key)[0]?.reviewId; + + if (!copy) throw error; + + return copy; + } + + return receiveShare(shareInput(onStage), key, meta, () => + client.download(capability, meta.revision.id), + ); + }, ), 202, ); @@ -502,7 +516,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 2fb21f6050..754bf39b29 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -7,6 +7,7 @@ import path from "node:path"; import type { Block } from "@dev.fast/whiteboard-document"; import { createReviewApi } from "@review/review-api/http.js"; import { openLocalReviewStore } from "@review/review-api/local-data.js"; +import type { SharedProvenance } from "@review/review-api/store.js"; import { afterEach, expect, it, vi } from "vitest"; import { createShareFixture } from "../../test/fixtures/share/create.js"; @@ -171,7 +172,7 @@ it("copies a share into a board the recipient edits like their own", async () => }); it("plans the three rules", () => { - const c = (over: object) => ({ + const c = (over: Partial) => ({ reviewId: "a", version: 3, shared: { key: "k", revisionId: R1, importedVersion: 3, ...over }, @@ -213,7 +214,7 @@ it("reopens, refreshes in place from Trash, and copies beside an edited board", const input = { ...recipient, fetchRepository }; const link = key(); - const download = vi.fn(async () => ({ + const download = vi.fn[3]>(async () => ({ bundle, attribution: { login: "ada", sharedAt: 1 }, })); @@ -291,6 +292,7 @@ it("keeps a share's commits until its link's last copy is deleted", async () => ); const b = await receiveShare(input, link, served(R2, R2), download); + const root = recipient.store.repositoryPath( recipient.store.read(a).pins!.repositoryId, ); From d1cc209a83b530250fbc4059ad7ed22a13798ec1 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:28:10 -0500 Subject: [PATCH 09/17] Keep one share link per board in the share popover Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/app/src/canvas-query.tsx | 3 +- .../review/app/src/share-control.test.tsx | 14 +++++- packages/review/app/src/share-control.tsx | 43 +++++++++++-------- 3 files changed, 39 insertions(+), 21 deletions(-) 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..efaf729439 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, }); }, ); @@ -289,7 +291,7 @@ it("falls back to sign-in when the stored login has expired, then uploads after expect(container.textContent).not.toContain("expired"); }); -it("reuses the link for a version that was already shared and uploads again for a new one", async () => { +it("keeps one link per board: reuses it while the version holds and uploads a new version behind it", async () => { const harness = mount({ signedIn: true }); const { container } = harness; @@ -299,6 +301,9 @@ it("reuses the link for a version that was already shared and uploads again for await harness.settle(); expect(publishes(harness)).toHaveLength(1); expect(container.querySelector("input")?.value).toContain("/s/capability"); + expect(container.textContent).toContain( + "This link always opens the latest share of this board.", + ); await harness.click("Share review"); await harness.click("Share review"); expect(container.textContent).not.toContain("Uploading"); @@ -313,6 +318,13 @@ it("reuses the link for a version that was already shared and uploads again for expect.objectContaining({ version: 4 }), expect.objectContaining({ version: 5 }), ]); + expect(container.querySelector("input")?.value).toBe( + "https://app.dev.fast/s/capability", + ); + await harness.click("Share review"); + await harness.click("Share review"); + await harness.settle(); + expect(publishes(harness)).toHaveLength(2); }); it("keeps one upload when the popover is reopened while it is in flight", async () => { diff --git a/packages/review/app/src/share-control.tsx b/packages/review/app/src/share-control.tsx index f506da262d..f10e3ecb4a 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,16 +113,12 @@ 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, - ), + onSuccess: (shared) => queryClient.setQueryData(linkKey, shared), onError: (error) => { // The host forgot a stale login; show sign-in and upload again after it. if (error instanceof ReviewApiError && error.status === 401) @@ -156,10 +157,13 @@ export function ShareControl({ }, })); + const current = link && 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 +213,20 @@ export function ShareControl({

)} {signedIn ? ( - link ? ( + current ? ( <> event.target.select()} /> - ) : error ? ( From 4769b6d4cb9e55d89d3333f4d684b785cf655b34 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:28:41 -0500 Subject: [PATCH 10/17] Carry a share revision through the Desktop deep link Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../vs/review/common/reviewShareLink.test.ts | 19 +++++++++++++++++++ .../src/vs/review/common/reviewShareLink.ts | 12 ++++++++++++ .../sharing/reviewSharing.contribution.ts | 4 ++-- 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.test.ts create mode 100644 apps/review-desktop/code-oss/src/vs/review/common/reviewShareLink.ts 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."); } From ba248face3b65d8ea5ab4800e6c22c5184d41377 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:38:12 -0500 Subject: [PATCH 11/17] Open an existing share copy without waiting on other imports Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/src/review-api/http.ts | 11 ++++- packages/review/src/sharing/host.test.ts | 42 ++++++++++++++++++ packages/review/src/sharing/host.ts | 50 +++++++++++++++++----- packages/review/src/sharing/import.test.ts | 45 +++++++++++++++++++ 4 files changed, 136 insertions(+), 12 deletions(-) diff --git a/packages/review/src/review-api/http.ts b/packages/review/src/review-api/http.ts index a6227d0067..fecc481e99 100644 --- a/packages/review/src/review-api/http.ts +++ b/packages/review/src/review-api/http.ts @@ -2207,7 +2207,16 @@ export function createReviewApi( const key = copy?.shared?.key; const kept = copy && boardRepositoryId(copy); - if (result.deleted && key && kept && !store.copiesOf(key).length) + 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/sharing/host.test.ts b/packages/review/src/sharing/host.test.ts index 8a21a0c46c..512a885343 100644 --- a/packages/review/src/sharing/host.test.ts +++ b/packages/review/src/sharing/host.test.ts @@ -272,6 +272,48 @@ it("opens the newest copy when the share service cannot answer, else reports it } }); +it("opens an existing copy while another link's import is still downloading", async () => { + const { bundle, start, status, close } = await importer(); + const other = `https://app.dev.fast/s/${"b".repeat(43)}`; + + vi.spyOn(ShareClient.prototype, "shared").mockResolvedValue(meta); + vi.spyOn(ShareClient.prototype, "download").mockImplementation( + async (link) => { + if (link !== capability) await new Promise(() => {}); + + return { + bundle, + attribution: { login: "sender", sharedAt: 1 }, + revision: meta.revision, + latest: R, + }; + }, + ); + + try { + const { reviewId: id } = await start(); + + await vi.waitFor( + async () => expect(await status(id)).toMatchObject({ stage: "ready" }), + { timeout: 10000 }, + ); + + const { reviewId: copy } = await status(id); + const { reviewId: held } = await start(other); + + await start(); + await vi.waitFor(async () => + expect(await status(id)).toMatchObject({ + stage: "ready", + reviewId: copy, + }), + ); + expect(await status(held)).toMatchObject({ stage: "downloading" }); + } finally { + await close(); + } +}); + it("keeps a revision link's import apart from the live link's", async () => { const { start, status, close } = await importer(); diff --git a/packages/review/src/sharing/host.ts b/packages/review/src/sharing/host.ts index 81367c72dc..4ec697405d 100644 --- a/packages/review/src/sharing/host.ts +++ b/packages/review/src/sharing/host.ts @@ -36,6 +36,7 @@ import { type ShareImport, importShare, needsRepository, + planShareCopy, receiveShare, shareKey, } from "./import.js"; @@ -184,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"); @@ -222,11 +234,11 @@ export function mountSharingHost( const status: ImportStatus = { stage: initial }; imports.set(id, status); - importing = importing - .then(async () => { - status.reviewId = await receive((stage) => { - status.stage = stage; - }); + receive((stage) => { + status.stage = stage; + }) + .then((reviewId) => { + status.reviewId = reviewId; status.stage = "ready"; }) .catch((error) => { @@ -260,14 +272,23 @@ export function mountSharingHost( }); const key = whiteboardFileKey(text); + const copy = () => store.copiesOf(key)[0]?.reviewId; return context.json( startImport( key, "reading", async (onStage) => - store.copiesOf(key)[0]?.reviewId ?? - importShare(shareInput(onStage), key, parseWhiteboardFile(text)), + copy() ?? + serially( + async () => + copy() ?? + importShare( + shareInput(onStage), + key, + parseWhiteboardFile(text), + ), + ), ), 202, ); @@ -307,9 +328,16 @@ export function mountSharingHost( return copy; } - return receiveShare(shareInput(onStage), key, meta, () => - client.download(capability, meta.revision.id), - ); + const plan = planShareCopy(store.copiesOf(key), meta); + + // 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), + ), + ); }, ), 202, diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index 754bf39b29..3c39d67202 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -327,6 +327,51 @@ it("keeps a share's commits until its link's last copy is deleted", async () => expect(git("for-each-ref", `refs/review/shared/${link}`)).toBe(""); }); +it("drops a share's commits when its link's last copy in that repository is deleted", async () => { + const { bundle, recipient, fetchRepository } = await share(); + const link = key(); + + const a = await receiveShare( + { ...recipient, fetchRepository }, + link, + served(R1, R1), + async () => ({ bundle, attribution: { login: "ada", sharedAt: 1 } }), + ); + + await recipient.store.execute( + { operation: { type: "create", title: "Elsewhere" } }, + { document: [], shared: { login: "ada", key: link } }, + { kind: "person" }, + ); + + const root = recipient.store.repositoryPath( + recipient.store.read(a).pins!.repositoryId, + ); + + await recipient.store.execute( + { operation: { type: "lifecycle", state: "trash", reviewId: a } }, + undefined, + { kind: "person" }, + ); + + const response = await createReviewApi( + recipient.store, + recipient.data, + ).request("/commands", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ operation: { type: "delete", reviewId: a } }), + }); + + expect(response.status).toBe(200); + expect( + execFileSync("git", ["for-each-ref", `refs/review/shared/${link}`], { + cwd: root, + encoding: "utf8", + }).trim(), + ).toBe(""); +}); + it("fetches the shared commits into the recipient's own checkout", async () => { const { root, sender, pins, bundle, recipient, fetchRepository } = await share(); From 4391f7dfbe85bfcd6b19a1bbf0e71fac81c6870d Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 15:47:29 -0500 Subject: [PATCH 12/17] Never move a share link's cache backwards Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 12a7a292-c657-465f-b5f3-c638d3396f22 Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../review/app/src/share-control.test.tsx | 40 +++++++++++++++++++ packages/review/app/src/share-control.tsx | 6 ++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/packages/review/app/src/share-control.test.tsx b/packages/review/app/src/share-control.test.tsx index efaf729439..5a1fabfd6e 100644 --- a/packages/review/app/src/share-control.test.tsx +++ b/packages/review/app/src/share-control.test.tsx @@ -26,6 +26,7 @@ function mount(options: { publishFails?: boolean; publishExpired?: boolean; holdPublish?: Promise; + holdVersion?: { version: number; until: Promise }; sender?: string; }) { vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); @@ -76,6 +77,12 @@ function mount(options: { if (state.holdPublish) await state.holdPublish; + if ( + path.endsWith("/publish") && + JSON.parse(String(init!.body)).version === state.holdVersion?.version + ) + await state.holdVersion.until; + if (path.endsWith("/publish") && state.publishExpired) { state.signedIn = false; state.publishExpired = false; @@ -353,6 +360,39 @@ it("keeps one upload when the popover is reopened while it is in flight", async expect(publishes(harness)).toHaveLength(1); }); +it("keeps a newer version's link when an older upload lands after it", async () => { + let release: (() => void) | undefined; + + const harness = mount({ + signedIn: true, + holdVersion: { + version: 4, + until: new Promise((resolve) => { + release = resolve; + }), + }, + }); + + const { container } = harness; + + await harness.render(4); + await harness.click("Share review"); + await harness.settle(); + await harness.click("Share review"); + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + expect(container.querySelector("input")?.value).toContain("/s/capability"); + release?.(); + await harness.settle(); + expect(publishes(harness).map((post) => post.body)).toEqual([ + expect.objectContaining({ version: 4 }), + expect.objectContaining({ version: 5 }), + ]); + expect(container.textContent).not.toContain("Uploading"); + expect(container.querySelector("input")?.value).toContain("/s/capability"); +}); + it("polls only while sign-in is pending and stops when the control unmounts", async () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "setInterval", "clearInterval"], diff --git a/packages/review/app/src/share-control.tsx b/packages/review/app/src/share-control.tsx index f10e3ecb4a..b898e6d943 100644 --- a/packages/review/app/src/share-control.tsx +++ b/packages/review/app/src/share-control.tsx @@ -118,7 +118,11 @@ export function ShareControl({ version, requestId, }), - onSuccess: (shared) => queryClient.setQueryData(linkKey, shared), + // 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. if (error instanceof ReviewApiError && error.status === 401) From 440a9a241e60d242fcc25fb999eecda92e5a9273 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 16:02:35 -0500 Subject: [PATCH 13/17] Check a reshared board keeps its link end to end Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../scripts/e2e/fixtures/share-service.mjs | 147 ++++++++++++++++++ .../scripts/e2e/journeys/share-link.mjs | 96 ++++++++++++ 2 files changed, 243 insertions(+) create mode 100644 apps/review-desktop/scripts/e2e/fixtures/share-service.mjs create mode 100644 apps/review-desktop/scripts/e2e/journeys/share-link.mjs 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(); + } +} From d99a71519e9ea0c01acce6489ffd6ce823b464e3 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 16:02:36 -0500 Subject: [PATCH 14/17] Check a reopened share link refreshes its copy or copies beside an edited one Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../scripts/e2e/journeys/shared-review.mjs | 177 ++++++++++++++++-- packages/review/scripts/seed-share-fixture.ts | 53 +++--- 2 files changed, 183 insertions(+), 47 deletions(-) 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/scripts/seed-share-fixture.ts b/packages/review/scripts/seed-share-fixture.ts index 208f0571d4..30c81a085f 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 a share's two revisions as `bundle-1.json` and `bundle-2.json` into `` for the shared-review e2e 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, }), From 70a534ee9333f0e748f4707bec1b5d2aa2176d57 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 16:16:31 -0500 Subject: [PATCH 15/17] Keep legacy and pinned share copies apart from live refreshes Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- packages/review/src/review-api/store.ts | 2 + packages/review/src/sharing/host.test.ts | 41 ++++++- packages/review/src/sharing/host.ts | 22 +++- packages/review/src/sharing/import.test.ts | 123 +++++++++++++++++++++ packages/review/src/sharing/import.ts | 39 +++++-- 5 files changed, 210 insertions(+), 17 deletions(-) diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 805ee32f60..07ecbd92d4 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -366,6 +366,8 @@ export interface SharedProvenance { revisionId?: string; /** The version that import wrote; a later version 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. */ diff --git a/packages/review/src/sharing/host.test.ts b/packages/review/src/sharing/host.test.ts index 512a885343..a4a20ad3c5 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"; @@ -222,7 +223,7 @@ async function importer() { await rm(root, { recursive: true, force: true }); }; - return { bundle, start, status, close }; + return { bundle, start, status, close, store: fixture.store }; } it("opens the newest copy when the share service cannot answer, else reports it unavailable", async () => { @@ -336,6 +337,44 @@ it("keeps a revision link's import apart from the live link's", async () => { } }); +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 4ec697405d..48eec35377 100644 --- a/packages/review/src/sharing/host.ts +++ b/packages/review/src/sharing/host.ts @@ -320,22 +320,32 @@ export function mountSharingHost( try { meta = await client.shared(capability, revision); } catch (error) { - // Offline, the link's newest copy still opens. - const copy = store.copiesOf(key)[0]?.reviewId; + // 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; + return copy.reviewId; } - const plan = planShareCopy(store.copiesOf(key), meta); + 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), + receiveShare( + shareInput(onStage), + key, + meta, + () => client.download(capability, meta.revision.id), + pinned, ), ); }, diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index 3c39d67202..d7900b2653 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -207,6 +207,25 @@ it("plans the three rules", () => { served(R2, R2), ), ).toEqual({ kind: "import" }); + expect( + planShareCopy( + [{ reviewId: "l", version: 1, shared: { key: "k", sharedAt: 1 } }], + served(R1old, R2), + ), + ).toEqual({ kind: "open", reviewId: "l" }); + expect(planShareCopy([c({ sharedAt: 1 })], served(R1old, R2))).toEqual({ + kind: "import", + }); + expect(planShareCopy([c({ pinned: true })], served(R2, R2))).toEqual({ + kind: "import", + }); + expect(planShareCopy([c({ pinned: true })], served(R1, R2))).toEqual({ + kind: "open", + reviewId: "a", + }); + expect(planShareCopy([c({})], served(R2, R2), true)).toEqual({ + kind: "import", + }); }); it("reopens, refreshes in place from Trash, and copies beside an edited board", async () => { @@ -261,9 +280,113 @@ it("reopens, refreshes in place from Trash, and copies beside an edited board", 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); }); +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(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 }; diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 0d5ae0a3eb..1d2586248c 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, @@ -248,22 +251,28 @@ export type ShareCopyPlan = | { kind: "refresh"; reviewId: string; version: number } | { kind: "import" }; -/** A copy made from the served revision opens; an unedited copy takes the - * latest revision in place; anything else gets a new copy. */ +/** A copy made from the served revision opens; an unedited, unpinned copy + * takes the latest revision in place; anything else gets a new copy. A + * pinned link never refreshes. */ export function planShareCopy( copies: ReturnType, served: ServedRevision, + pinned = false, ): ShareCopyPlan { - const same = copies.find( - (copy) => copy.shared.revisionId === served.revision.id, + 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), ); @@ -285,12 +294,18 @@ export async function receiveShare( bundle: ShareBundle; attribution: ShareAttribution; }>, + pinned = false, ): Promise { - const plan = planShareCopy(input.store.copiesOf(key), served); + 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 }; + + const provenance = { + ...attribution, + revisionId: served.revision.id, + ...(pinned && { pinned }), + }; if (plan.kind === "import") return importShare(input, key, bundle, provenance); @@ -298,8 +313,11 @@ export async function receiveShare( try { return await importShare(input, key, bundle, provenance, plan); } catch (error) { - // The board was edited since the plan read it. - if (error instanceof ReviewInputError && error.status === 409) + // 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; } @@ -310,7 +328,8 @@ export async function importShare( input: ShareImport, key: string, received: ShareBundle, - provenance?: ShareAttribution & { revisionId?: string }, + provenance?: ShareAttribution & + Pick, into?: { reviewId: string; version: number }, ): Promise { const { store, data } = input; From d3136675de70fcb52019434bcfd9dd3424081ed9 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sun, 1 Nov 2026 16:16:32 -0500 Subject: [PATCH 16/17] Show a share link the cache already holds for a newer version Agent-Session: 64182dea-534e-45f1-8e28-c1e27a5d6bab Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: 196d7f0b-63a2-4186-84b0-5068df27bf9a Agent-Session: b47f56d8-24aa-4d4f-95b3-4217b7ec9e00 Agent-Session: f2f8f3d8-5a50-4c15-8ad8-9c9308d4a110 Agent-Session: 226d831c-5d71-4f4d-8cdb-7b3c4f30284d --- .../review/app/src/share-control.test.tsx | 19 ++++++++++++++++++- packages/review/app/src/share-control.tsx | 3 ++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/review/app/src/share-control.test.tsx b/packages/review/app/src/share-control.test.tsx index 5a1fabfd6e..b285cc6c3a 100644 --- a/packages/review/app/src/share-control.test.tsx +++ b/packages/review/app/src/share-control.test.tsx @@ -79,7 +79,8 @@ function mount(options: { if ( path.endsWith("/publish") && - JSON.parse(String(init!.body)).version === state.holdVersion?.version + state.holdVersion && + JSON.parse(String(init!.body)).version === state.holdVersion.version ) await state.holdVersion.until; @@ -393,6 +394,22 @@ it("keeps a newer version's link when an older upload lands after it", async () expect(container.querySelector("input")?.value).toContain("/s/capability"); }); +it("shows a newer version's link when the popover opens on an older version", async () => { + const harness = mount({ signedIn: true }); + const { container } = harness; + + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + await harness.click("Share review"); + await harness.render(4); + await harness.click("Share review"); + await harness.settle(); + expect(publishes(harness)).toHaveLength(1); + expect(container.textContent).not.toContain("Uploading"); + expect(container.querySelector("input")?.value).toContain("/s/capability"); +}); + it("polls only while sign-in is pending and stops when the control unmounts", async () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "setInterval", "clearInterval"], diff --git a/packages/review/app/src/share-control.tsx b/packages/review/app/src/share-control.tsx index b898e6d943..dbf6b150e8 100644 --- a/packages/review/app/src/share-control.tsx +++ b/packages/review/app/src/share-control.tsx @@ -161,7 +161,8 @@ export function ShareControl({ }, })); - const current = link && link.version === target?.version ? link : undefined; + 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(() => { From 90d26c08cdd1b950b86dddfaba8cdf5a5dd8b920 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Mon, 2 Nov 2026 14:44:59 -0500 Subject: [PATCH 17/17] Trim share link tests and comments Agent-Session: 61c94475-694a-4e46-8971-e0d703d3b344 Agent-Session: 2b04f44e-91c6-4568-8c57-c4a01b2828ad Agent-Session: d28b54c2-3ec8-4cf0-959a-e356abec0831 --- .../review-share-protocol/src/index.test.ts | 35 ------ .../review/app/src/share-control.test.tsx | 69 +----------- packages/review/scripts/seed-share-fixture.ts | 2 +- .../src/review-api/copied-board.test.ts | 90 --------------- packages/review/src/review-api/store.ts | 9 +- packages/review/src/sharing/client.test.ts | 53 +-------- packages/review/src/sharing/host.test.ts | 77 +------------ packages/review/src/sharing/import.test.ts | 106 ++---------------- packages/review/src/sharing/import.ts | 4 +- 9 files changed, 21 insertions(+), 424 deletions(-) diff --git a/packages/review-share-protocol/src/index.test.ts b/packages/review-share-protocol/src/index.test.ts index cb51072c59..1d268e01c3 100644 --- a/packages/review-share-protocol/src/index.test.ts +++ b/packages/review-share-protocol/src/index.test.ts @@ -6,11 +6,8 @@ import { normalizeGitHubRemote, parseShareLink, shareBundleSchema, - shareCreatedSchema, shareLink, sharePathSchema, - shareRevisionsSchema, - sharedMetaSchema, } from "./index.js"; it("puts the capability in the path and rejects credentials, fragments or other paths", () => { @@ -59,38 +56,6 @@ it("carries a revision as ?rev= and rejects any other query", () => { ); }); -it("reads a link's revisions and the revision a capability serves", () => { - const id = "5b0e9a52-4a43-4f7e-9d8e-3c1f0e7b2a11", - url = `https://app.dev.fast/s/${"a".repeat(43)}?rev=${id}`; - - expect( - shareRevisionsSchema.parse({ - latest: id, - items: [{ id, version: 3, size: 10, createdAt: 1, url, extra: true }], - }).items[0], - ).toEqual({ id, version: 3, size: 10, createdAt: 1, url }); - expect( - sharedMetaSchema.safeParse({ - sender: { login: "ada" }, - sharedAt: 1, - size: 10, - }).success, - ).toBe(false); - expect( - sharedMetaSchema.parse({ - title: "T", - sender: { login: "ada" }, - sharedAt: 1, - size: 10, - revision: { id, version: 3, createdAt: 1 }, - latest: id, - }).revision.id, - ).toBe(id); - expect( - shareCreatedSchema.safeParse({ shareId: id, url, revision: id }).success, - ).toBe(false); -}); - it("rejects traversal paths and platform-specific separators", () => { for (const file of [ "../secret", diff --git a/packages/review/app/src/share-control.test.tsx b/packages/review/app/src/share-control.test.tsx index b285cc6c3a..f8814709c1 100644 --- a/packages/review/app/src/share-control.test.tsx +++ b/packages/review/app/src/share-control.test.tsx @@ -26,7 +26,6 @@ function mount(options: { publishFails?: boolean; publishExpired?: boolean; holdPublish?: Promise; - holdVersion?: { version: number; until: Promise }; sender?: string; }) { vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); @@ -77,13 +76,6 @@ function mount(options: { if (state.holdPublish) await state.holdPublish; - if ( - path.endsWith("/publish") && - state.holdVersion && - JSON.parse(String(init!.body)).version === state.holdVersion.version - ) - await state.holdVersion.until; - if (path.endsWith("/publish") && state.publishExpired) { state.signedIn = false; state.publishExpired = false; @@ -299,7 +291,7 @@ it("falls back to sign-in when the stored login has expired, then uploads after expect(container.textContent).not.toContain("expired"); }); -it("keeps one link per board: reuses it while the version holds and uploads a new version behind it", async () => { +it("reuses the link for a version that was already shared and uploads again for a new one", async () => { const harness = mount({ signedIn: true }); const { container } = harness; @@ -309,9 +301,6 @@ it("keeps one link per board: reuses it while the version holds and uploads a ne await harness.settle(); expect(publishes(harness)).toHaveLength(1); expect(container.querySelector("input")?.value).toContain("/s/capability"); - expect(container.textContent).toContain( - "This link always opens the latest share of this board.", - ); await harness.click("Share review"); await harness.click("Share review"); expect(container.textContent).not.toContain("Uploading"); @@ -326,13 +315,6 @@ it("keeps one link per board: reuses it while the version holds and uploads a ne expect.objectContaining({ version: 4 }), expect.objectContaining({ version: 5 }), ]); - expect(container.querySelector("input")?.value).toBe( - "https://app.dev.fast/s/capability", - ); - await harness.click("Share review"); - await harness.click("Share review"); - await harness.settle(); - expect(publishes(harness)).toHaveLength(2); }); it("keeps one upload when the popover is reopened while it is in flight", async () => { @@ -361,55 +343,6 @@ it("keeps one upload when the popover is reopened while it is in flight", async expect(publishes(harness)).toHaveLength(1); }); -it("keeps a newer version's link when an older upload lands after it", async () => { - let release: (() => void) | undefined; - - const harness = mount({ - signedIn: true, - holdVersion: { - version: 4, - until: new Promise((resolve) => { - release = resolve; - }), - }, - }); - - const { container } = harness; - - await harness.render(4); - await harness.click("Share review"); - await harness.settle(); - await harness.click("Share review"); - await harness.render(5); - await harness.click("Share review"); - await harness.settle(); - expect(container.querySelector("input")?.value).toContain("/s/capability"); - release?.(); - await harness.settle(); - expect(publishes(harness).map((post) => post.body)).toEqual([ - expect.objectContaining({ version: 4 }), - expect.objectContaining({ version: 5 }), - ]); - expect(container.textContent).not.toContain("Uploading"); - expect(container.querySelector("input")?.value).toContain("/s/capability"); -}); - -it("shows a newer version's link when the popover opens on an older version", async () => { - const harness = mount({ signedIn: true }); - const { container } = harness; - - await harness.render(5); - await harness.click("Share review"); - await harness.settle(); - await harness.click("Share review"); - await harness.render(4); - await harness.click("Share review"); - await harness.settle(); - expect(publishes(harness)).toHaveLength(1); - expect(container.textContent).not.toContain("Uploading"); - expect(container.querySelector("input")?.value).toContain("/s/capability"); -}); - it("polls only while sign-in is pending and stops when the control unmounts", async () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "setInterval", "clearInterval"], diff --git a/packages/review/scripts/seed-share-fixture.ts b/packages/review/scripts/seed-share-fixture.ts index 30c81a085f..f78a02bc30 100644 --- a/packages/review/scripts/seed-share-fixture.ts +++ b/packages/review/scripts/seed-share-fixture.ts @@ -1,4 +1,4 @@ -/** Writes a share's two revisions as `bundle-1.json` and `bundle-2.json` into `` for the shared-review e2e journey. */ +/** Writes two revisions of one share into `` for the shared-review journey. */ import { writeFile } from "node:fs/promises"; import path from "node:path"; diff --git a/packages/review/src/review-api/copied-board.test.ts b/packages/review/src/review-api/copied-board.test.ts index b5b7c63de3..9445c72928 100644 --- a/packages/review/src/review-api/copied-board.test.ts +++ b/packages/review/src/review-api/copied-board.test.ts @@ -117,19 +117,6 @@ it("lists a link's copies newest first, Trash included, until each is deleted", ]); expect(store.copiesOf("other")).toEqual([]); - await store.execute({ - operation: { - type: "edit", - reviewId: first, - edit: { type: "insert", content: { type: "divider" } }, - }, - }); - expect(store.copiesOf("link")[1]).toEqual({ - reviewId: first, - version: 1, - shared, - }); - await store.execute( { operation: { type: "lifecycle", state: "trash", reviewId: first } }, undefined, @@ -148,83 +135,6 @@ it("lists a link's copies newest first, Trash included, until each is deleted", expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([second]); }); -it("refreshes a copy in place with a newer share, even in Trash, and refuses once it was edited", async () => { - const { reviewId } = await copy(); - - await store.execute( - { operation: { type: "lifecycle", state: "trash", reviewId } }, - undefined, - { kind: "person" }, - ); - - const next: Seed = { - ...structuredClone(seed), - document: [ - { id: "markdown-3", type: "markdown", markdown: "Why, revised" }, - ], - shared: { ...seed.shared, revisionId: "r2", sharedAt: 2 }, - }; - - const create = { - operation: { - type: "create", - title: "Copy v2", - target: { kind: "commits", ...pins }, - }, - }; - - const result = await store.refreshCopy(reviewId, 0, create, next); - - expect(result).toMatchObject({ reviewId, version: 1 }); - - const snapshot = store.read(reviewId); - - expect(snapshot).toMatchObject({ - title: "Copy v2", - trashedAt: expect.any(String), - shared: { - revisionId: "r2", - sharedAt: 2, - key: "link", - importedVersion: 1, - }, - }); - expect(snapshot.document).toEqual(next.document); - expect(store.copiesOf("link").map((c) => c.reviewId)).toEqual([reviewId]); - await expect( - store.refreshCopy(reviewId, 0, create, next), - ).rejects.toMatchObject({ status: 409 }); -}); - -it("refreshes only a copy of the same link", async () => { - const { reviewId } = await copy(); - - const { reviewId: own } = await store.execute({ - operation: { - type: "create", - title: "Own", - target: { kind: "commits", ...pins }, - }, - }); - - const create = { - operation: { - type: "create", - title: "Copy v2", - target: { kind: "commits", ...pins }, - }, - }; - - const other = { ...seed, shared: { ...seed.shared, key: "other" } }; - - await expect(store.refreshCopy(own, 0, create, seed)).rejects.toMatchObject({ - status: 400, - }); - await expect( - store.refreshCopy(reviewId, 0, create, other), - ).rejects.toMatchObject({ status: 400 }); -}); - it("still checks the images of a board made from scratch", async () => { const { reviewId } = await store.execute({ operation: { diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 07ecbd92d4..e91c9096c2 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -362,9 +362,8 @@ export interface SharedProvenance { sharedAt?: number; cloneUrl?: string; key?: string; - /** The link revision the board was made or last refreshed from. */ revisionId?: string; - /** The version that import wrote; a later version is the receiver's edit. */ + /** The version import wrote; anything later is the receiver's edit. */ importedVersion?: number; /** Opened from a link naming a revision: never refreshed. */ pinned?: boolean; @@ -1131,7 +1130,7 @@ export class ReviewStore { .map((row) => ({ reviewId: String(row.id), version: Number(row.version), - // SAFETY: the key matched, so `shared` is the object a seed wrote. + // SAFETY: the key matched this row's seed. shared: JSON.parse(String(row.shared)) as SharedProvenance, })); } @@ -1617,8 +1616,7 @@ export class ReviewStore { return this.track(prepared); } - /** Replaces a share copy's content with a newer revision of its link: same board id, Trash state kept. - * Refuses (409) once the board has moved past `baseVersion`. */ + /** Replaces a share copy's content under the same board id; 409 once it moved past `baseVersion`. */ refreshCopy( reviewId: string, baseVersion: number, @@ -1749,7 +1747,6 @@ export class ReviewStore { prepared: PreparedCommand, initial: Seed | undefined, author: Author, - /** A share copy whose content the create replaces in place. */ into?: { reviewId: string; baseVersion: number }, ) { const { diff --git a/packages/review/src/sharing/client.test.ts b/packages/review/src/sharing/client.test.ts index 15a884eb3b..d951cb6688 100644 --- a/packages/review/src/sharing/client.test.ts +++ b/packages/review/src/sharing/client.test.ts @@ -100,7 +100,7 @@ it("surfaces a failed create and refuses an oversized share before any request", function sharedService(meta: SharedMeta, body: () => BodyInit) { const requests: { path: string; headers: Headers }[] = []; - const send = vi.fn(async (input, init) => { + const send: typeof fetch = async (input, init) => { const url = new URL(String(input)); requests.push({ path: url.pathname + url.search, @@ -110,11 +110,10 @@ function sharedService(meta: SharedMeta, body: () => BodyInit) { return url.pathname.endsWith("/bundle") ? new Response(body()) : Response.json(meta); - }); + }; return { requests, - send, client: new ShareClient("https://app.dev.fast", "secret", send), }; } @@ -165,7 +164,7 @@ it("downloads a share with the link's capability within the size limit", async ( ).rejects.toThrow("limit"); }); -it("asks for a named revision and refuses a malformed one before any request", async () => { +it("asks for a named revision", async () => { const shared = await bundle(); const capability = "c".repeat(43); const R = randomUUID(); @@ -184,52 +183,6 @@ it("asks for a named revision and refuses a malformed one before any request", a await client.download(capability, R); expect(requests[0]?.path).toBe(`/api/v2/shared/${capability}?rev=${R}`); - - const refused = sharedService(meta, () => JSON.stringify(shared)); - - await expect(refused.client.shared(capability, "nope")).rejects.toThrow( - "UUID", - ); - expect(refused.send).not.toHaveBeenCalled(); -}); - -it("reads a link's revisions with the account's token", async () => { - const shareId = randomUUID(); - const R1 = randomUUID(); - const R2 = randomUUID(); - const u1 = `https://app.dev.fast/s/${"x".repeat(43)}?rev=${R1}`; - const u2 = `https://app.dev.fast/s/${"x".repeat(43)}?rev=${R2}`; - const requests: string[] = []; - - const revisions = { - latest: R2, - items: [ - { id: R2, version: 5, size: 20, createdAt: 2, url: u2 }, - { id: R1, version: 4, size: 10, createdAt: 1, url: u1 }, - ], - }; - - const client = new ShareClient( - "https://app.dev.fast", - "secret", - async (input, init) => { - const { pathname } = new URL(String(input)); - - if (pathname === "/api/auth/token") - return new Response(null, { status: 404 }); - requests.push( - `${init?.method ?? "GET"} ${pathname} ${new Headers(init?.headers).get("authorization")}`, - ); - - return Response.json(revisions); - }, - ); - - expect(await client.revisions(shareId)).toEqual(revisions); - expect(requests).toEqual([ - `GET /api/v2/shares/${shareId}/revisions Bearer secret`, - ]); - await expect(client.revisions("x")).rejects.toThrow("UUID"); }); it("stops a streamed response as soon as it exceeds the declared size", async () => { diff --git a/packages/review/src/sharing/host.test.ts b/packages/review/src/sharing/host.test.ts index a4a20ad3c5..c42eccb4a8 100644 --- a/packages/review/src/sharing/host.test.ts +++ b/packages/review/src/sharing/host.test.ts @@ -119,9 +119,7 @@ it("reports download and checkout preparation, and opens the same copy when the }), ); - const sharedSpy = vi - .spyOn(ShareClient.prototype, "shared") - .mockResolvedValue(meta); + vi.spyOn(ShareClient.prototype, "shared").mockResolvedValue(meta); let fetched!: () => void; @@ -182,9 +180,7 @@ it("reports download and checkout preparation, and opens the same copy when the await vi.waitFor(async () => expect(await status()).toMatchObject({ stage: "ready", reviewId: copy }), ); - expect(sharedSpy).toHaveBeenCalledTimes(2); expect(downloadSpy).toHaveBeenCalledTimes(1); - expect(downloadSpy).toHaveBeenCalledWith(capability, R); } finally { fetched(); await fixture.data.close(); @@ -243,11 +239,7 @@ it("opens the newest copy when the share service cannot answer, else reports it const { reviewId: id } = await start(); await vi.waitFor(async () => - expect(await status(id)).toMatchObject({ - stage: "error", - error: - "This share is unavailable, revoked, or needs a newer Whiteboard version.", - }), + expect(await status(id)).toMatchObject({ stage: "error" }), ); sharedSpy.mockResolvedValue(meta); @@ -267,71 +259,6 @@ it("opens the newest copy when the share service cannot answer, else reports it reviewId: copy, }), ); - expect(sharedSpy).toHaveBeenCalledTimes(3); - } finally { - await close(); - } -}); - -it("opens an existing copy while another link's import is still downloading", async () => { - const { bundle, start, status, close } = await importer(); - const other = `https://app.dev.fast/s/${"b".repeat(43)}`; - - vi.spyOn(ShareClient.prototype, "shared").mockResolvedValue(meta); - vi.spyOn(ShareClient.prototype, "download").mockImplementation( - async (link) => { - if (link !== capability) await new Promise(() => {}); - - return { - bundle, - attribution: { login: "sender", sharedAt: 1 }, - revision: meta.revision, - latest: R, - }; - }, - ); - - try { - const { reviewId: id } = await start(); - - await vi.waitFor( - async () => expect(await status(id)).toMatchObject({ stage: "ready" }), - { timeout: 10000 }, - ); - - const { reviewId: copy } = await status(id); - const { reviewId: held } = await start(other); - - await start(); - await vi.waitFor(async () => - expect(await status(id)).toMatchObject({ - stage: "ready", - reviewId: copy, - }), - ); - expect(await status(held)).toMatchObject({ stage: "downloading" }); - } finally { - await close(); - } -}); - -it("keeps a revision link's import apart from the live link's", async () => { - const { start, status, close } = await importer(); - - const sharedSpy = vi - .spyOn(ShareClient.prototype, "shared") - .mockRejectedValue(new Error("Share request failed (404).")); - - try { - const { reviewId } = await start(`${url}?rev=${R}`); - - expect(reviewId).toBe( - `${shareKey("https://app.dev.fast", capability)}-${R}`, - ); - await vi.waitFor(async () => - expect(await status(reviewId)).toMatchObject({ stage: "error" }), - ); - expect(sharedSpy).toHaveBeenCalledWith(capability, R); } finally { await close(); } diff --git a/packages/review/src/sharing/import.test.ts b/packages/review/src/sharing/import.test.ts index d7900b2653..3e0502110d 100644 --- a/packages/review/src/sharing/import.test.ts +++ b/packages/review/src/sharing/import.test.ts @@ -7,7 +7,6 @@ import path from "node:path"; import type { Block } from "@dev.fast/whiteboard-document"; import { createReviewApi } from "@review/review-api/http.js"; import { openLocalReviewStore } from "@review/review-api/local-data.js"; -import type { SharedProvenance } from "@review/review-api/store.js"; import { afterEach, expect, it, vi } from "vitest"; import { createShareFixture } from "../../test/fixtures/share/create.js"; @@ -171,60 +170,20 @@ it("copies a share into a board the recipient edits like their own", async () => expect(recipient.store.read(reviewId).title).toBe("Mine now"); }); -it("plans the three rules", () => { - const c = (over: Partial) => ({ - reviewId: "a", - version: 3, - shared: { key: "k", revisionId: R1, importedVersion: 3, ...over }, - }); +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([c({})], { - revision: { id: R1, version: 4, createdAt: 1 }, - latest: R2, - }), - ).toEqual({ kind: "open", reviewId: "a" }); - expect(planShareCopy([c({})], served(R2, R2))).toEqual({ + expect(planShareCopy(legacy(0), served(R2, R2))).toEqual({ kind: "refresh", - reviewId: "a", - version: 3, + reviewId: "l", + version: 0, }); - expect(planShareCopy([{ ...c({}), version: 4 }], served(R2, R2))).toEqual({ - kind: "import", - }); - expect(planShareCopy([c({})], served(R1old, R2))).toEqual({ - kind: "import", - }); - expect( - planShareCopy( - [{ reviewId: "l", version: 0, shared: { key: "k" } }], - served(R2, R2), - ), - ).toEqual({ kind: "refresh", reviewId: "l", version: 0 }); - expect( - planShareCopy( - [{ reviewId: "l", version: 1, shared: { key: "k" } }], - served(R2, R2), - ), - ).toEqual({ kind: "import" }); - expect( - planShareCopy( - [{ reviewId: "l", version: 1, shared: { key: "k", sharedAt: 1 } }], - served(R1old, R2), - ), - ).toEqual({ kind: "open", reviewId: "l" }); - expect(planShareCopy([c({ sharedAt: 1 })], served(R1old, R2))).toEqual({ - kind: "import", - }); - expect(planShareCopy([c({ pinned: true })], served(R2, R2))).toEqual({ - kind: "import", - }); - expect(planShareCopy([c({ pinned: true })], served(R1, R2))).toEqual({ + expect(planShareCopy(legacy(1), served(R2, R2))).toEqual({ kind: "import" }); + expect(planShareCopy(legacy(1, 1), served(R1old, R2))).toEqual({ kind: "open", - reviewId: "a", - }); - expect(planShareCopy([c({})], served(R2, R2), true)).toEqual({ - kind: "import", + reviewId: "l", }); }); @@ -450,51 +409,6 @@ it("keeps a share's commits until its link's last copy is deleted", async () => expect(git("for-each-ref", `refs/review/shared/${link}`)).toBe(""); }); -it("drops a share's commits when its link's last copy in that repository is deleted", async () => { - const { bundle, recipient, fetchRepository } = await share(); - const link = key(); - - const a = await receiveShare( - { ...recipient, fetchRepository }, - link, - served(R1, R1), - async () => ({ bundle, attribution: { login: "ada", sharedAt: 1 } }), - ); - - await recipient.store.execute( - { operation: { type: "create", title: "Elsewhere" } }, - { document: [], shared: { login: "ada", key: link } }, - { kind: "person" }, - ); - - const root = recipient.store.repositoryPath( - recipient.store.read(a).pins!.repositoryId, - ); - - await recipient.store.execute( - { operation: { type: "lifecycle", state: "trash", reviewId: a } }, - undefined, - { kind: "person" }, - ); - - const response = await createReviewApi( - recipient.store, - recipient.data, - ).request("/commands", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ operation: { type: "delete", reviewId: a } }), - }); - - expect(response.status).toBe(200); - expect( - execFileSync("git", ["for-each-ref", `refs/review/shared/${link}`], { - cwd: root, - encoding: "utf8", - }).trim(), - ).toBe(""); -}); - it("fetches the shared commits into the recipient's own checkout", async () => { const { root, sender, pins, bundle, recipient, fetchRepository } = await share(); diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 1d2586248c..81ae50bb86 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -251,9 +251,7 @@ export type ShareCopyPlan = | { kind: "refresh"; reviewId: string; version: number } | { kind: "import" }; -/** A copy made from the served revision opens; an unedited, unpinned copy - * takes the latest revision in place; anything else gets a new copy. A - * pinned link never refreshes. */ +/** Which copy a served revision opens or refreshes in place; otherwise a new copy. */ export function planShareCopy( copies: ReturnType, served: ServedRevision,