mirror of
https://github.com/runbear-io/beardrive.git
synced 2026-08-25 08:08:08 +02:00
fix(webapp): restore asks before it syncs to every device (BEA-129) (#145)
Clicking `restore` in History posted straight to the hub — no dialog — while the two other web actions that leave the browser (revoke a share link, remove a file) both confirm first. The gated action was the reversible one and the effectively-irreversible one wasn't. The dialog's second line is case-aware, because the two cases really do differ: swapping a live file's content is walk-backable from History, but bringing a deleted file back is not — the run card's "undo — remove file" is the only delete control in the whole web UI, and a restore produces no run card. So that branch says so instead of promising an undo the UI can't keep. The flag needs no new computation: headBlob's "" already marks a path whose newest op is a delete, so HistoryView passes `recreates` down beside `restoreSha` (both feed shapes — standalone rows and rows inside a run card). A row can't decide this itself: an older EDIT row of a deleted path re-creates the file just as much as the DELETED row does, and only the feed knows that. Confirm styling is non-danger on purpose: restore adds content, it takes none away, so it doesn't wear Remove's red. Building the missing undo for the ADDED row a restore creates is deliberately out of scope (Snow scoped this to "add confirmation").
This commit is contained in:
Snow Lee (Sungwon)
@@ -611,8 +611,16 @@ test("a file the run created can be undone, and comes back", async ({ page }) =>
|
||||
const gone = page.locator('.history > .hentry.delete:has-text("runbook.md")').first();
|
||||
await expect(gone).toBeVisible();
|
||||
|
||||
// ...and the delete row puts it back, bytes and all.
|
||||
// ...and the delete row puts it back, bytes and all — after asking, and
|
||||
// saying the one true thing about this branch: History can't take it away
|
||||
// again (BEA-129).
|
||||
await gone.locator(".hrestore-btn").click();
|
||||
const rmodal = page.locator(".modal");
|
||||
await expect(rmodal).toContainText("Restore this version of runbook.md?");
|
||||
await expect(rmodal).toContainText("isn't available from History yet");
|
||||
// Restore adds content, so it is not dressed as a destructive action.
|
||||
await expect(rmodal.locator(".danger-btn")).toHaveCount(0);
|
||||
await rmodal.getByRole("button", { name: "Restore" }).click();
|
||||
await expectToast(page, /Restored runbook\.md/);
|
||||
await page.goto(`/${pid}/runbook.md`);
|
||||
await expect(page.locator("#content")).toContainText("Created during the agent run");
|
||||
@@ -631,7 +639,26 @@ test("restoring an old version brings its content back", async ({ page }) => {
|
||||
await page.goto(`/${pid}/history/${path}`);
|
||||
const older = page.locator(".hentry.add"); // the first version
|
||||
await expect(older).toBeVisible();
|
||||
|
||||
// It reaches every device, so it asks first — and Cancel writes nothing:
|
||||
// no request, no `restoring…`, no new row (BEA-129).
|
||||
await older.locator(".hrestore-btn").click();
|
||||
const modal = page.locator(".modal");
|
||||
await expect(modal).toContainText("Restore this version of restore-me.md?");
|
||||
// The file still exists, so this restore IS walk-backable — the copy says so.
|
||||
await expect(modal).toContainText("You can restore any other version afterwards.");
|
||||
await page.keyboard.press("Escape"); // Esc dismisses it too
|
||||
await expect(modal).toHaveCount(0);
|
||||
await older.locator(".hrestore-btn").click();
|
||||
await modal.getByRole("button", { name: "Cancel" }).click();
|
||||
await expect(modal).toHaveCount(0);
|
||||
await expect(page.locator(".history .hentry")).toHaveCount(2);
|
||||
await expect(page.locator(".hrestore-btn").first()).toHaveText(/restore$/);
|
||||
// Asking is still not navigating: the row stayed put through the dialog.
|
||||
await expect(page).toHaveURL(`/${pid}/history/${path}`);
|
||||
|
||||
await older.locator(".hrestore-btn").click();
|
||||
await modal.getByRole("button", { name: "Restore" }).click();
|
||||
await expectToast(page, /Restored restore-me\.md/);
|
||||
// The restore is itself a change, and the file serves the old bytes again.
|
||||
await expect(page.locator(".history .hentry")).toHaveCount(3);
|
||||
@@ -672,6 +699,7 @@ test("the current version offers no restore", async ({ page }) => {
|
||||
const own = page.locator(".history .hentry");
|
||||
await expect(own.first().locator(".hrestore-btn")).toHaveCount(0);
|
||||
await own.last().locator(".hrestore-btn").click(); // the oldest row: the first version
|
||||
await page.locator(".modal").getByRole("button", { name: "Restore" }).click();
|
||||
await expectToast(page, /Restored current-version\.md/);
|
||||
await expect(own).toHaveCount(3);
|
||||
await expect(own.first().locator(".hrestore-btn")).toHaveCount(0);
|
||||
|
||||
@@ -268,11 +268,28 @@ export default function Browser(props: {
|
||||
/* ---- restore ----
|
||||
Putting an old version back is a write, so a read-only member sees no
|
||||
Restore button rather than one that 403s. The restore itself is a new
|
||||
change: the tree, the file, and the history feed all move. */
|
||||
change: the tree, the file, and the history feed all move — so it asks
|
||||
first, like every other action here that leaves the browser (BEA-129).
|
||||
The second line is case-aware because the two cases really differ:
|
||||
swapping a live file's content is walk-backable from History, bringing a
|
||||
deleted file back is not — the run card's "undo — remove file" is the
|
||||
only delete control in the web UI, and a restore produces no run card.
|
||||
Non-danger styling on purpose: restore adds content, it takes none away. */
|
||||
const [restoring, setRestoring] = useState("");
|
||||
const canRestore = hub && !!project && atLeast(project?.perm, "write");
|
||||
const onRestore = useCallback(
|
||||
async (p: string, sha: string) => {
|
||||
async (p: string, sha: string, recreates: boolean) => {
|
||||
if (
|
||||
!(await modalConfirm(
|
||||
"Restore this version of " + p + "?",
|
||||
"It syncs to every device as a new change. " +
|
||||
(recreates
|
||||
? "The file comes back on every device. Removing it again isn't available from History yet."
|
||||
: "You can restore any other version afterwards."),
|
||||
"Restore",
|
||||
))
|
||||
)
|
||||
return;
|
||||
setRestoring(p + sha);
|
||||
try {
|
||||
await postJSON(apiBase + "restore", { path: p, sha });
|
||||
|
||||
@@ -18,7 +18,10 @@ const KIND_LABEL: Record<string, string> = { add: "added", edit: "edited", delet
|
||||
// isn't a hub project), in which case no Restore button is drawn at all —
|
||||
// better than one that 403s.
|
||||
export type RestoreAction = {
|
||||
onRestore: (path: string, sha: string) => void;
|
||||
// `recreates` = this restore brings a currently-deleted file back, which
|
||||
// nothing in the browser can walk back — so the confirm says so instead of
|
||||
// promising an undo (BEA-129). Only the feed knows it; see `recreates` below.
|
||||
onRestore: (path: string, sha: string, recreates: boolean) => void;
|
||||
busy?: string; // path+sha currently in flight
|
||||
};
|
||||
|
||||
@@ -57,6 +60,7 @@ export function HistoryRow({
|
||||
restore,
|
||||
remove,
|
||||
restoreSha,
|
||||
recreates,
|
||||
inRun,
|
||||
read,
|
||||
}: {
|
||||
@@ -78,6 +82,10 @@ export function HistoryRow({
|
||||
// The version this row puts back: its own bytes, or — for a delete — the
|
||||
// content it removed. The view computes it, since it needs the whole feed.
|
||||
restoreSha?: string;
|
||||
// Whether this restore puts a currently-deleted file back. A row only sees
|
||||
// itself — an older EDIT row of a deleted path re-creates the file just as
|
||||
// much as the DELETED row does — so the view decides it.
|
||||
recreates?: boolean;
|
||||
// Inside a run card, where "this run created the file" is a statement we
|
||||
// can actually make.
|
||||
inRun?: boolean;
|
||||
@@ -153,7 +161,7 @@ export function HistoryRow({
|
||||
title={"Put this version of " + e.path + " back as a new change"}
|
||||
onClick={(ev) => {
|
||||
ev.stopPropagation();
|
||||
restore!.onRestore(e.path, restoreSha!);
|
||||
restore!.onRestore(e.path, restoreSha!, !!recreates);
|
||||
}}
|
||||
onKeyDown={(ev) => ev.stopPropagation()}
|
||||
>
|
||||
|
||||
@@ -119,6 +119,10 @@ export function HistoryView(props: {
|
||||
const sha = entries[i].kind === "delete" ? prevBlob(i) : entries[i].blob;
|
||||
return sha && sha === headBlob.get(entries[i].path) ? undefined : sha;
|
||||
};
|
||||
// "" is the marker headBlob already uses for a path whose newest op is a
|
||||
// delete — so this restore brings the file back, which nothing in the
|
||||
// browser can walk back afterwards. The confirm copy says so (BEA-129).
|
||||
const recreates = (i: number) => headBlob.get(entries[i].path) === "";
|
||||
return (
|
||||
<div className="history">
|
||||
{bar}
|
||||
@@ -143,6 +147,7 @@ export function HistoryView(props: {
|
||||
apiBase={apiBase}
|
||||
prevBlob={prevBlob}
|
||||
restoreSha={restoreSha}
|
||||
recreates={recreates}
|
||||
restore={restore}
|
||||
remove={remove}
|
||||
/>
|
||||
@@ -160,6 +165,7 @@ export function HistoryView(props: {
|
||||
diff={{ apiBase, prev: prevBlob(item.i) }}
|
||||
restore={restore}
|
||||
restoreSha={restoreSha(item.i)}
|
||||
recreates={recreates(item.i)}
|
||||
/* no `remove`: an add outside a run card isn't attributable to a
|
||||
run, so the undo has nothing to claim (follow-up issue). */
|
||||
/>
|
||||
@@ -188,6 +194,7 @@ function RunGroup({
|
||||
apiBase,
|
||||
prevBlob,
|
||||
restoreSha,
|
||||
recreates,
|
||||
restore,
|
||||
remove,
|
||||
}: {
|
||||
@@ -196,6 +203,7 @@ function RunGroup({
|
||||
apiBase: string;
|
||||
prevBlob: (i: number) => string | undefined;
|
||||
restoreSha: (i: number) => string | undefined;
|
||||
recreates: (i: number) => boolean;
|
||||
restore?: RestoreAction;
|
||||
remove?: RemoveAction;
|
||||
}) {
|
||||
@@ -267,6 +275,7 @@ function RunGroup({
|
||||
restore={restore}
|
||||
remove={remove}
|
||||
restoreSha={restoreSha(run.idx[k])}
|
||||
recreates={recreates(run.idx[k])}
|
||||
inRun
|
||||
read={readPaths.has(e.path)}
|
||||
/>
|
||||
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -5,7 +5,7 @@
|
||||
<meta name="viewport" content="width=device-width, initial-scale=1">
|
||||
<title>BearDrive</title>
|
||||
<link rel="icon" href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32' fill='%23f5a623'><rect x='4' y='4' width='5.6' height='24'/><rect x='11.2' y='4' width='14.4' height='11.2'/><rect x='11.2' y='16.8' width='16.8' height='11.2'/></svg>">
|
||||
<script type="module" crossorigin src="/assets/index-D7MxmRut.js"></script>
|
||||
<script type="module" crossorigin src="/assets/index-o0X8ZIDt.js"></script>
|
||||
<link rel="modulepreload" crossorigin href="/assets/_commonjsHelpers-CqkleIqs.js">
|
||||
<link rel="modulepreload" crossorigin href="/assets/mermaid-CP2pUOT9.js">
|
||||
<link rel="stylesheet" crossorigin href="/assets/index-Bhy4rJG7.css">
|
||||
|
||||
Reference in New Issue
Block a user