diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts index 3f014a568..e9b793e45 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts @@ -1450,6 +1450,7 @@ export class ReviewCanvasEditorPane extends EditorPane { | "onDidChangeDiffLayout" | "setupTooltip" | "notify" + | "confirm" | "ready" | "reportDiagnostic" | "reportSidebar" @@ -1472,6 +1473,7 @@ export class ReviewCanvasEditorPane extends EditorPane { setDiffLayout: (scope, layout) => this.diffViews.layout(scope).set(layout), onDidChangeDiffLayout: (scope, listener) => this.diffViews.layout(scope).onDidChange(listener), notify: (notification) => showReviewNotification(this.notificationService, notification), + confirm: async (request) => (await this.dialogService.confirm({ type: 'warning', ...request })).confirmed, setupTooltip: this.setupTooltip, ready: () => { if (generation !== this.loadGeneration || !this.targetDocument) return; diff --git a/apps/review-desktop/scripts/e2e/journeys/share-link.mjs b/apps/review-desktop/scripts/e2e/journeys/share-link.mjs index 64ec90d1d..f35662338 100644 --- a/apps/review-desktop/scripts/e2e/journeys/share-link.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/share-link.mjs @@ -1,4 +1,4 @@ -/** A board shared, edited and shared again keeps one link, with a revision per version. */ +/** A board shared, edited and shared again keeps one link, with a revision per version, until it is revoked. */ import assert from "node:assert/strict"; import { writeFile } from "node:fs/promises"; import path from "node:path"; @@ -11,6 +11,7 @@ export const name = "share-link"; export const phase = 1; export const options = { + settings: { "window.dialogStyle": "custom" }, async beforeLaunch(ctx) { ctx.service = await startShareService(); await writeFile( @@ -90,6 +91,56 @@ export async function run(ctx) { assert.equal(service.posts.length, 2); assert.equal(await input.inputValue(), u1); ctx.check("reopening an unchanged board uploads nothing"); + + const rows = page.locator("[data-share-revision]"); + + await ctx.until(async () => (await rows.count()) === 2, "two history rows"); + + const [latest, older] = await rows.allTextContents(); + + assert.match( + latest, + new RegExp(` · v${revisions.items[0].version} · .* · Latest$`), + ); + assert.match(older, new RegExp(` · v${revisions.items[1].version} · `)); + assert.doesNotMatch(older, /Latest/); + ctx.check("the popover lists both revisions, the newest marked Latest"); + + const revokeButton = page.getByRole("button", { name: "Revoke link" }); + + const dialog = ctx.page + .getByRole("dialog") + .filter({ hasText: "Revoke this share link?" }); + + const notice = page.getByText( + "Link revoked. Sharing this board again creates a new link.", + ); + + await revokeButton.click(); + await dialog.getByRole("button", { name: "Cancel", exact: true }).click(); + await dialog.waitFor({ state: "hidden" }); + await input.waitFor(); + assert.equal(service.links.get(reviewId)?.shareId, shareId); + ctx.check("cancelling the revoke keeps the link and the popover"); + + await revokeButton.click(); + await dialog.getByRole("button", { name: "Revoke", exact: true }).click(); + await notice.waitFor(); + assert.equal(service.links.has(reviewId), false); + ctx.check("revoking deletes the link at the service"); + + await page.keyboard.press("Escape"); + await notice.waitFor({ state: "detached" }); + await button.click(); + await ctx.until( + async () => + service.posts.length === 3 && + Boolean(await input.inputValue().catch(() => "")), + "the new link", + ); + assert.notEqual(await input.inputValue(), u1); + assert.notEqual(service.links.get(reviewId).shareId, shareId); + ctx.check("sharing a revoked board again uploads a new link"); } finally { await service.stop(); } diff --git a/packages/review-protocol/src/contracts.ts b/packages/review-protocol/src/contracts.ts index 2c75e63a5..c07355642 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -404,6 +404,12 @@ export interface ReviewCanvasBridge { listener: (show: boolean) => void, ): ReviewDisposable; notify?(notification: ReviewNotification): ReviewDisposable; + /** Asks in the host's modal dialog; resolves true when the primary button is chosen. */ + confirm?(request: { + message: string; + detail?: string; + primaryButton: string; + }): Promise; setupTooltip?( target: HTMLElement, text: string, diff --git a/packages/review/app/src/canvas-query.tsx b/packages/review/app/src/canvas-query.tsx index 88b6aeda1..172964a35 100644 --- a/packages/review/app/src/canvas-query.tsx +++ b/packages/review/app/src/canvas-query.tsx @@ -35,6 +35,7 @@ export function createCanvasQueryClient(): QueryClient { export const canvasQueryKeys = { sharingAccount: () => ["sharing", "account"] as const, shareLink: (board: string) => ["sharing", "link", board] as const, + shareHistory: (shareId: string) => ["sharing", "history", shareId] as const, traceList: ( board: string, version: number | undefined, diff --git a/packages/review/app/src/format.test.ts b/packages/review/app/src/format.test.ts index ed42d8828..0e2fd5223 100644 --- a/packages/review/app/src/format.test.ts +++ b/packages/review/app/src/format.test.ts @@ -1,6 +1,6 @@ import { expect, it } from "vitest"; -import { formatRelativeTime } from "./format"; +import { formatBytes, formatRelativeTime } from "./format"; const now = Date.parse("2026-10-15T12:00:00Z"); @@ -27,3 +27,9 @@ it("writes hours and days in short for a narrow row", () => { "2d ago", ); }); + +it("writes a size in B, KB or MB", () => { + expect(formatBytes(512)).toBe("512 B"); + expect(formatBytes(2048)).toBe("2 KB"); + expect(formatBytes(1_572_864)).toBe("1.5 MB"); +}); diff --git a/packages/review/app/src/format.ts b/packages/review/app/src/format.ts index 4046d1432..a63e4636c 100644 --- a/packages/review/app/src/format.ts +++ b/packages/review/app/src/format.ts @@ -65,3 +65,13 @@ export function formatDuration(durationMs: number): string { return rest ? `${hours}h ${rest}m` : `${hours}h`; } + +/** A byte count to one decimal: "512 B", "2 KB", "1.5 MB". */ +export function formatBytes(bytes: number): string { + if (bytes < 1024) return `${bytes} B`; + const kilobytes = bytes / 1024; + + if (kilobytes < 1024) return `${Number(kilobytes.toFixed(1))} KB`; + + return `${Number((kilobytes / 1024).toFixed(1))} MB`; +} diff --git a/packages/review/app/src/share-control.test.tsx b/packages/review/app/src/share-control.test.tsx index b285cc6c3..aec293ac1 100644 --- a/packages/review/app/src/share-control.test.tsx +++ b/packages/review/app/src/share-control.test.tsx @@ -11,6 +11,16 @@ import { ShareControl, SharingContext } from "./share-control"; let dispose: (() => void) | undefined; +const shareId = "6f1c2b0e-8e0a-4b8e-9a51-0d6f1e2c3b4a"; + +const revision5 = "0b8d6c3e-3a9f-4c1e-8f2a-5d7e9b1c4a60"; + +const revision4 = "1c9e7d4f-4b0a-4d2f-9a3b-6e8f0c2d5b71"; + +const u5 = `https://app.dev.fast/s/capability?rev=${revision5}`; + +const u4 = `https://app.dev.fast/s/capability?rev=${revision4}`; + afterEach(async () => { await act(async () => dispose?.()); document.body.replaceChildren(); @@ -28,12 +38,15 @@ function mount(options: { holdPublish?: Promise; holdVersion?: { version: number; until: Promise }; sender?: string; + confirm?: () => Promise; + historyStatus?: number; }) { vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); const state = { ...options }; const posts: Array<{ path: string; body: unknown }> = []; const telemetry: string[] = []; let accountReads = 0; + let historyReads = 0; const session = testReviewSession( {}, @@ -44,6 +57,7 @@ function mount(options: { return Response.json({ ok: true }); }, + confirm: options.confirm, }, ); @@ -75,6 +89,43 @@ function mount(options: { }); } + if (path.includes("/history/")) { + historyReads += 1; + + if (state.historyStatus === 401) state.signedIn = false; + + if (state.historyStatus) + return Response.json( + { error: "History is unavailable." }, + { status: state.historyStatus }, + ); + + return Response.json({ + latest: revision5, + items: [ + { + id: revision5, + version: 5, + size: 2048, + createdAt: Date.now(), + url: u5, + }, + { + id: revision4, + version: 4, + size: 1024, + createdAt: Date.now() - 2 * 3_600_000, + url: u4, + }, + ], + }); + } + + if (path.endsWith("/open")) return Response.json({ opened: true }); + + if (path.endsWith("/revoke")) + return Response.json({ shareId, revoked: true }); + if (state.holdPublish) await state.holdPublish; if ( @@ -101,7 +152,7 @@ function mount(options: { ); return Response.json({ - shareId: "6f1c2b0e-8e0a-4b8e-9a51-0d6f1e2c3b4a", + shareId, url: "https://app.dev.fast/s/capability", version: JSON.parse(String(init!.body)).version, }); @@ -163,6 +214,7 @@ function mount(options: { click, settle, accountReads: () => accountReads, + historyReads: () => historyReads, }; } @@ -410,6 +462,152 @@ it("shows a newer version's link when the popover opens on an older version", as expect(container.querySelector("input")?.value).toContain("/s/capability"); }); +it("lists the link's revisions newest first, marks the latest and opens one in the browser", async () => { + const harness = mount({ signedIn: true }); + const { container } = harness; + + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + expect( + [...container.querySelectorAll("[role=dialog] [data-share-revision]")].map( + (row) => row.textContent, + ), + ).toEqual(["just now · v5 · 2 KB · Latest", "2 hours ago · v4 · 1 KB"]); + await harness.click("2 hours ago · v4 · 1 KB"); + expect(harness.posts.at(-1)).toEqual({ + path: "/reviews-api/sharing/open", + body: { url: u4 }, + }); + expect(container.textContent).not.toContain("Revoke link"); + await harness.click("Share review"); + await harness.render(6); + await harness.click("Share review"); + await harness.settle(); + expect(publishes(harness)).toHaveLength(2); + expect(harness.historyReads()).toBe(2); +}); + +it("revokes the link after the host confirms, and the next opening shares a new one", async () => { + let answer = false; + const confirm = vi.fn<() => Promise>(async () => answer); + const harness = mount({ signedIn: true, confirm }); + const { container } = harness; + + const posts = (suffix: string) => + harness.posts.filter((post) => post.path.endsWith(suffix)); + + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + await harness.click("Revoke link"); + expect(confirm).toHaveBeenCalledTimes(1); + expect(posts("/revoke")).toHaveLength(0); + answer = true; + await harness.click("Revoke link"); + await harness.settle(); + expect(posts("/revoke").map((post) => post.body)).toEqual([{ shareId }]); + expect(container.textContent).toContain( + "Link revoked. Sharing this board again creates a new link.", + ); + expect(container.querySelector("input")).toBeNull(); + expect(publishes(harness)).toHaveLength(1); + await harness.click("Share review"); + await harness.click("Share review"); + await harness.settle(); + expect(publishes(harness)).toHaveLength(2); + expect(container.querySelector("input")?.value).toContain("/s/capability"); + expect(container.textContent).not.toContain("Link revoked"); +}); + +it("shows a history failure under the link and keeps it usable", async () => { + const harness = mount({ signedIn: true, historyStatus: 500 }); + const { container } = harness; + + await harness.render(5); + await harness.click("Share review"); + await vi.waitFor(async () => { + await harness.settle(); + expect(container.textContent).toContain("Could not load share history."); + }); + expect(container.querySelector("[role=alert]")).toBeNull(); + expect(container.querySelector("input")?.value).toContain("/s/capability"); + const dialog = container.querySelector("[role=dialog]")!; + expect( + [...dialog.children].findIndex((child) => + child.textContent?.includes("Could not load share history."), + ), + ).toBeGreaterThan( + [...dialog.children].findIndex((child) => child.tagName === "INPUT"), + ); + expect( + [...dialog.querySelectorAll("button")].map((button) => button.textContent), + ).toContain("Copy link"); +}); + +it("shows sign-in when the history read finds the login expired", async () => { + const harness = mount({ signedIn: true, historyStatus: 401 }); + const { container } = harness; + + await harness.render(5); + await harness.settle(); + const reads = harness.accountReads(); + await harness.click("Share review"); + await vi.waitFor(async () => { + await harness.settle(); + expect(harness.historyReads()).toBe(1); + expect(harness.accountReads()).toBeGreaterThan(reads); + expect( + [...container.querySelectorAll("[role=dialog] button")].map( + (button) => button.textContent, + ), + ).toContain("Sign in to share"); + }); +}); + +it("opens one confirmation while the first is unanswered", async () => { + let answer: (confirmed: boolean) => void = () => {}; + + const confirm = vi.fn<() => Promise>( + () => new Promise((resolve) => (answer = resolve)), + ); + + const harness = mount({ signedIn: true, confirm }); + + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + await harness.click("Revoke link"); + await harness.click("Revoke link"); + expect(confirm).toHaveBeenCalledTimes(1); + await act(async () => answer(false)); + await harness.click("Revoke link"); + expect(confirm).toHaveBeenCalledTimes(2); +}); + +it("shares again from the revoked state without closing", async () => { + const harness = mount({ signedIn: true, confirm: async () => true }); + const { container } = harness; + + await harness.render(5); + await harness.click("Share review"); + await harness.settle(); + await harness.click("Revoke link"); + await harness.settle(); + expect(container.textContent).toContain("Link revoked"); + await harness.click("Share again"); + await harness.settle(); + + const [first, second] = publishes(harness).map( + (post) => (post.body as { requestId: string }).requestId, + ); + + expect(second).toBeDefined(); + expect(second).not.toBe(first); + expect(container.querySelector("input")?.value).toContain("/s/capability"); + expect(container.textContent).not.toContain("Link revoked"); +}); + 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 dbf6b150e..f19f5d5ca 100644 --- a/packages/review/app/src/share-control.tsx +++ b/packages/review/app/src/share-control.tsx @@ -1,12 +1,15 @@ import { layer, space } from "@canvas/scale.stylex"; -import { Button, IconButton } from "@canvas/ui/button"; +import { Button, IconButton, buttonStyles } from "@canvas/ui/button"; import { useCopy } from "@canvas/ui/copy"; +import { focusStyles } from "@canvas/ui/focus"; import { iconStyles } from "@canvas/ui/icon-styles"; import { ShareIcon } from "@canvas/ui/icons"; +import { listRowStyles } from "@canvas/ui/list-row"; import { Popover } from "@canvas/ui/popover"; import { textStyles } from "@canvas/ui/text"; import { TextField } from "@canvas/ui/text-field"; import { useTooltip } from "@canvas/ui/tooltip"; +import type { ShareRevisions } from "@dev.fast/review-share-protocol"; import { type ReviewApiClient, ReviewApiError, @@ -29,6 +32,7 @@ import { } from "react"; import { canvasQueryKeys } from "./canvas-query"; +import { formatBytes, formatRelativeTime } from "./format"; import { useOptionalReviewSession } from "./host/review-session"; import { topbarActionsMarker } from "./markers.stylex"; import { useSharingAccount, useSignIn } from "./sharing-account"; @@ -63,6 +67,9 @@ const actionError = (error: Error | null) => ? error.message : "Could not complete this action. Please retry."); +// Keeps focus in the popover when the control that had it goes away. +const focusOnMount = (element: HTMLElement | null) => element?.focus(); + /** Whether this review can be shared, so a menu may offer Share. */ export function useCanShare(): boolean { return useContext(SharingContext) !== null; @@ -83,6 +90,7 @@ export function ShareControl({ const [open, setOpen] = useState(false); const [target, setTarget] = useState(); const [copyError, setCopyError] = useState(); + const [revoked, setRevoked] = useState(false); const control = useRef(null); const accountKey = canvasQueryKeys.sharingAccount(); @@ -119,10 +127,14 @@ export function ShareControl({ requestId, }), // An older version's upload can land after a newer one's. - onSuccess: (shared) => + onSuccess: (shared) => { queryClient.setQueryData(linkKey, (cached) => cached && cached.version > shared.version ? cached : shared, - ), + ); + void queryClient.invalidateQueries({ + queryKey: canvasQueryKeys.shareHistory(shared.shareId), + }); + }, onError: (error) => { // The host forgot a stale login; show sign-in and upload again after it. if (error instanceof ReviewApiError && error.status === 401) @@ -130,22 +142,63 @@ export function ShareControl({ }, }); + const openRevision = useMutation({ + mutationFn: (url: string) => + context!.client.post<{ opened: true }>("/sharing/open", { url }), + }); + + const revoke = useMutation({ + mutationFn: (shareId: string) => + context!.client.post("/sharing/revoke", { shareId }), + onSuccess: (_, shareId) => { + queryClient.removeQueries({ queryKey: linkKey }); + queryClient.removeQueries({ + queryKey: canvasQueryKeys.shareHistory(shareId), + }); + setRevoked(true); + }, + }); + + const confirm = session?.bridge.confirm; + const confirming = useRef(false); + + const revokeLink = async (shareId: string) => { + if (!confirm || confirming.current || revoke.isPending) return; + confirming.current = true; + + const confirmed = await confirm({ + message: "Revoke this share link?", + detail: + "Anyone with the link loses access to every version shared behind it.", + primaryButton: "Revoke", + }).finally(() => (confirming.current = false)); + + // The host's dialog can dismiss the popover; bring it back. + setOpen(true); + + if (confirmed) revoke.mutate(shareId); + }; + const upload = (next: ShareTarget) => { login.reset(); publish.mutate(next); }; + const shareAgain = () => { + setTarget({ version: context!.version, requestId: crypto.randomUUID() }); + publish.reset(); + setRevoked(false); + }; + const toggle = () => { if (!context) return; // An upload of this version in flight keeps its request. if (!open && !(publish.isPending && target?.version === context.version)) { - setTarget({ - version: context.version, - requestId: crypto.randomUUID(), - }); - publish.reset(); + shareAgain(); login.reset(); + openRevision.reset(); + revoke.reset(); setCopyError(undefined); } @@ -164,15 +217,41 @@ export function ShareControl({ const current = link && target && link.version >= target.version ? link : undefined; + const historyQuery = useQuery({ + queryKey: canvasQueryKeys.shareHistory(current?.shareId ?? ""), + queryFn: + open && current + ? ({ signal }) => + context!.client.read( + `/sharing/history/${current.shareId}`, + signal, + ) + : skipToken, + }); + + const history = historyQuery.data; + + useEffect(() => { + if ( + historyQuery.error instanceof ReviewApiError && + historyQuery.error.status === 401 + ) + void queryClient.invalidateQueries({ queryKey: accountKey }); + }, [historyQuery.error]); + // Upload once per opening; a failure waits for Retry or a new sign-in. useEffect(() => { - if (open && signedIn && target && publish.isIdle && !current) + if (open && signedIn && target && publish.isIdle && !current && !revoked) upload(target); - }, [open, signedIn, target, publish.isIdle, current]); + }, [open, signedIn, target, publish.isIdle, current, revoked]); if (!context) return null; - const error = copyError ?? actionError(publish.error ?? login.error); + const error = + copyError ?? + actionError( + publish.error ?? login.error ?? openRevision.error ?? revoke.error, + ); const accountError = accountQuery.isError ? "Could not read sign-in status." @@ -218,7 +297,21 @@ export function ShareControl({

)} {signedIn ? ( - current ? ( + revoked ? ( + <> +

+ Link revoked. Sharing this board again creates a new link. +

+ + + ) : current ? ( <> copy(current.url)}> {copied === current.url ? "Copied" : "Copy link"} + {history && history.items.length > 0 && ( +
    + {history.items.map((revision) => ( +
  • + +
    • + ))} +
+ )} + {historyQuery.isError && ( +

+ Could not load share history. +

+ )} + {confirm && ( + + )} ) : error ? (