feat(magazine): undo and redo buttons (20-step history) in the sticky review bar

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

CHANGELOG.md

@@ -43,6 +43,8 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi
 - **"Edge cuts text" needs the box's own lines.** Ink beside a left/right edge only counts as cut-off letters when it sits on the rows where the box itself has text and is intermittent (letters and gaps); a solid vertical line (a screenshot frame, a chart border, a photo edge) or ink between the box's lines does not, and a thin horizontal rule above or below is not a cut line. On the measured issue clipped edges fell from 82 to 48 and pages needing a decision from 53 to 42.
 - **Unboxed ink in or beside a picture, chart or advert is no longer reported as missed text** (labels of a chart, text set into a photo, an advert's own wording).
 
+- **Undo and Redo buttons in the sticky review bar** (right after *Contents*), also on Ctrl+Z and Ctrl+Shift+Z / Ctrl+Y (not while typing in a text field, where the browser undoes the typing). The last 20 recorded changes are kept (compressed, in the magazine's `undo/` folder) and each button's tooltip names the change it would undo or redo. More changes are now recorded: approving/rejecting or editing an article, moving or resizing a region, drawing a region, re-reading a page, setting or releasing a page's reading order, *Fix reading order* and *Clean up boxes*, on top of region edits, linking, auto-fix and spelling corrections. A new change ends the redo history. As before, an undo (or redo) is refused if something else changed the magazine in between, so it can never roll back work it does not know about (`GET /history`, `POST /undo`, `POST /redo`).
+
 ### Changed
 - **The Contents button is the first item of the sticky review bar**, so it stays in reach while scrolling a long page.
 - **The page thumbnail is the Review button.** In the thumbnail grid, clicking the (big) page image opens the page in review; the separate *Review* / *Review this page* buttons are gone. A page shared by two or more articles keeps one labelled *Review* button per article, so it stays clear which is meant.

Manual.md

@@ -295,6 +295,13 @@ unreviewed boundaries may include a neighbouring article or an advert.
 
 **Colours in the article list:** a **blue** row with a *not reviewed* pill is an article nobody has approved or rejected yet; a green edge means approved.
 
+### Undo and Redo
+
+**Purpose:** take back a change (or put it back) without leaving review.
+**Steps:** use **Undo** / **Redo** in the sticky bar (next to *Contents*), or Ctrl+Z / Ctrl+Shift+Z (Ctrl+Y). The button tooltip names the change, e.g. "Undo: Deleted 2 regions". The last 20 recorded changes are kept per magazine: region edits and moves, drawing a region, approving/rejecting an article, page linking, auto-fix, reading-order changes, box clean-up and spelling corrections.
+**Errors:** a button stays disabled when there is nothing to undo/redo, or when something else has changed the magazine since (for example a whole-magazine run), because undoing then would roll that work back too. A new change ends the redo history.
+**Privacy:** the saved states live in the magazine's own `undo/` folder; nothing is sent anywhere.
+
 ### Less manual work: what happens automatically
 
 At import, and whenever you press **Auto-fix all pages**: boxes are fitted, inset pull quotes found, wrapped paragraphs cut, continuation pages **linked into the article running from the page before**, missed text added, suspicious words re-read and (with a language model) provable misspellings fixed. The report then lists only the pages that need a *decision about their boxes*, with **Start with page N**; pages that only have words to check are collapsed and optional. Articles that gained pages go back to *detected*, so re-approve them (the batch tools in *Select* do many at once). A backup of the layout is kept.

routes/magazine.py

@@ -610,7 +610,11 @@ async def magazine_update_article(doc_id: str, article_id: str, request: Request
             raise HTTPException(400, "pages must be a list of integers")
         target["pages"] = sorted(set(pages))
 
+    label = {"approved": "Approved the article", "rejected": "Rejected the article",
+             "detected": "Marked the article for review"}.get(str(body.get("status")), "Edited the article")
+    token = _snapshot_for_undo(d, label)
     mag.save_articles(d, articles)
+    _seal_undo(d, token)
     return target
 
 
@@ -940,10 +944,12 @@ def _reocr_page_sync(doc_id: str, job_id: str, page: int,
         articles = mag.load_articles(d)
         mag.remap_reviewed_page(articles, layout, updated, page)
         _add_split_paragraphs_to_articles(articles, cleanup)
+        token = _snapshot_for_undo(d, f"Re-read page {page}")
         mag.save_layout(d, updated)
         mag.save_articles(d, articles)
+        _seal_undo(d, token)
         background_job_update(job_id, stage="done", done=True,
-                              detail=f"Page {page} re-read", result={"page": page})
+                              detail=f"Page {page} re-read", result={"page": page, "undo": token})
     except Exception as exc:                       # noqa: BLE001 - surfaced to UI
         background_job_update(job_id, stage="failed", done=True, error=str(exc)[:1000])
 
@@ -1395,13 +1401,67 @@ def _add_split_paragraphs_to_articles(articles: list[dict], report: dict) -> boo
     return changed
 
 
+# ── Undo / redo ───────────────────────────────────────────────────────────────
+# Every undoable change first stores the layout and articles as they were (gzip, one file each in
+# <doc>/undo/) and pushes an entry on the undo stack; undoing stores the state it replaces on the
+# redo stack. Only the latest entry can be undone, and only while the saved state is still exactly
+# what that change left behind -- a later change that recorded nothing would otherwise be rolled
+# back silently along with it. A new change clears the redo stack.
+
+_UNDO_DEPTH = 20
+
+
+def _undo_dir(d: Path) -> Path:
+    path = d / "undo"
+    path.mkdir(exist_ok=True)
+    return path
+
+
+def _history(d: Path) -> dict[str, list[dict[str, Any]]]:
+    try:
+        data = json.loads((d / "undo" / "history.json").read_text("utf-8"))
+        return {"undo": list(data.get("undo") or []), "redo": list(data.get("redo") or [])}
+    except (OSError, ValueError, AttributeError):
+        return {"undo": [], "redo": []}
+
+
+def _save_history(d: Path, history: dict[str, list[dict[str, Any]]]) -> None:
+    (_undo_dir(d) / "history.json").write_text(json.dumps(history, ensure_ascii=False), "utf-8")
+
+
+def _write_snapshot(d: Path, token: str, label: str, layout: dict[str, Any], articles: list[dict[str, Any]]) -> None:
+    import gzip
+    payload = json.dumps({"label": label, "layout": layout, "articles": articles}, ensure_ascii=False)
+    with gzip.open(_undo_dir(d) / f"{token}.json.gz", "wt", encoding="utf-8", compresslevel=3) as fh:
+        fh.write(payload)
+
+
+def _read_snapshot(d: Path, token: str) -> dict[str, Any]:
+    import gzip
+    with gzip.open(d / "undo" / f"{token}.json.gz", "rt", encoding="utf-8") as fh:
+        return json.loads(fh.read())
+
+
+def _drop_snapshot(d: Path, token: str) -> None:
+    (d / "undo" / f"{token}.json.gz").unlink(missing_ok=True)
+
+
 def _snapshot_for_undo(d: Path, label: str) -> str:
     """Keep the layout and articles as they are now, so the change about to happen can be undone.
-    One level: the next snapshot replaces this one. Returns the token the client undoes with.
+    Returns the token the client undoes with. The caller seals it with _seal_undo once the change
-    The caller seals it with _seal_undo once the change is saved."""
+    is saved. A new change ends the redo history."""
+    if d is None or not Path(d).is_dir():
+        return ""                                    # nothing on disk to protect (e.g. a stubbed folder)
     token = uuid.uuid4().hex
-    snapshot = {"token": token, "label": label, "layout": mag.load_layout(d), "articles": mag.load_articles(d)}
+    _write_snapshot(d, token, label, mag.load_layout(d), mag.load_articles(d))
-    (d / "undo.json").write_text(json.dumps(snapshot, ensure_ascii=False), "utf-8")
+    history = _history(d)
+    for entry in history["redo"]:
+        _drop_snapshot(d, entry["token"])
+    history["redo"] = []
+    history["undo"].append({"token": token, "label": label, "after": None})
+    while len(history["undo"]) > _UNDO_DEPTH:
+        _drop_snapshot(d, history["undo"].pop(0)["token"])
+    _save_history(d, history)
     return token
 
 
@@ -1415,35 +1475,92 @@ def _state_digest(d: Path) -> str:
 
 
 def _seal_undo(d: Path | None, token: str | None) -> None:
-    """Record what the state looks like right after the undoable change. Undo is refused once the
+    """Record what the state looks like right after the undoable change (see above)."""
-    state differs from that -- any later change (drawing a region, fixing the reading order,
+    if d is None or not token:
-    approving) would otherwise be rolled back silently with it."""
-    path = d / "undo.json" if d is not None and token else None
-    if path is None or not path.is_file():
         return
-    snapshot = json.loads(path.read_text("utf-8"))
+    history = _history(d)
-    if snapshot.get("token") == token:
+    for entry in history["undo"]:
-        snapshot["after"] = _state_digest(d)
+        if entry["token"] == token:
-        path.write_text(json.dumps(snapshot, ensure_ascii=False), "utf-8")
+            entry["after"] = _state_digest(d)
+            _save_history(d, history)
+            return
+
+
+def _history_state(d: Path) -> dict[str, Any]:
+    history = _history(d)
+    digest = _state_digest(d)
+    top_undo = history["undo"][-1] if history["undo"] else None
+    top_redo = history["redo"][-1] if history["redo"] else None
+    can_undo = bool(top_undo and top_undo.get("after") == digest)
+    can_redo = bool(top_redo and top_redo.get("expects") == digest)
+    return {"can_undo": can_undo, "can_redo": can_redo,
+            "undo_label": top_undo["label"] if can_undo else "", "redo_label": top_redo["label"] if can_redo else "",
+            "undo_depth": len(history["undo"]), "redo_depth": len(history["redo"])}
+
+
+@router.get("/api/magazine/{doc_id}/history")
+async def magazine_history(doc_id: str):
+    """What Undo and Redo would do right now (label of the change, or nothing)."""
+    d, _ = _require_magazine(doc_id)
+    return _history_state(d)
 
 
 @router.post("/api/magazine/{doc_id}/undo")
 async def magazine_undo(doc_id: str, request: Request):
-    """Undo the last region change (delete, type change, merge, split, bulk). Body: {token}.
+    """Undo the latest recorded change. Body: {token?} -- when given it must be the latest change's,
-    Only the latest change can be undone, so a stale token never rolls back a newer one."""
+    so a stale button never rolls back a newer one."""
     d, _ = _require_magazine(doc_id)
-    body = await request.json()
+    try:
+        body = await request.json()
+    except ValueError:
+        body = {}
     token = body.get("token") if isinstance(body, dict) else None
-    path = d / "undo.json"
+    history = _history(d)
-    if not isinstance(token, str) or not path.is_file():
+    if not history["undo"]:
         raise HTTPException(409, "Nothing to undo")
-    snapshot = json.loads(path.read_text("utf-8"))
+    top = history["undo"][-1]
-    if snapshot.get("token") != token or snapshot.get("after") != _state_digest(d):
+    if isinstance(token, str) and token and token != top["token"]:
+        raise HTTPException(409, "Only the latest change can be undone")
+    if top.get("after") != _state_digest(d):
         raise HTTPException(409, "Something else has changed since -- this can no longer be undone")
+    try:
+        snapshot = _read_snapshot(d, top["token"])
+    except (OSError, ValueError):
+        raise HTTPException(409, "The saved copy of that state is gone")
+    redo_token = uuid.uuid4().hex
+    _write_snapshot(d, redo_token, top["label"], mag.load_layout(d), mag.load_articles(d))
     mag.save_layout(d, snapshot["layout"])
     mag.save_articles(d, snapshot["articles"])
-    path.unlink(missing_ok=True)
+    history["undo"].pop()
-    return {"ok": True, "label": snapshot.get("label", "")}
+    _drop_snapshot(d, top["token"])
+    history["redo"].append({"token": redo_token, "label": top["label"], "expects": _state_digest(d)})
+    _save_history(d, history)
+    return {"ok": True, "label": top.get("label", ""), **_history_state(d)}
+
+
+@router.post("/api/magazine/{doc_id}/redo")
+async def magazine_redo(doc_id: str):
+    """Put back the change that was just undone."""
+    d, _ = _require_magazine(doc_id)
+    history = _history(d)
+    if not history["redo"]:
+        raise HTTPException(409, "Nothing to redo")
+    top = history["redo"][-1]
+    if top.get("expects") != _state_digest(d):
+        raise HTTPException(409, "Something else has changed since -- this can no longer be redone")
+    try:
+        snapshot = _read_snapshot(d, top["token"])
+    except (OSError, ValueError):
+        raise HTTPException(409, "The saved copy of that state is gone")
+    undo_token = uuid.uuid4().hex
+    _write_snapshot(d, undo_token, top["label"], mag.load_layout(d), mag.load_articles(d))
+    mag.save_layout(d, snapshot["layout"])
+    mag.save_articles(d, snapshot["articles"])
+    history["redo"].pop()
+    _drop_snapshot(d, top["token"])
+    history["undo"].append({"token": undo_token, "label": top["label"], "after": _state_digest(d)})
+    _save_history(d, history)
+    return {"ok": True, "label": top.get("label", ""), **_history_state(d)}
 
 
 @router.post("/api/magazine/{doc_id}/pages/{page}/regions/bulk")
@@ -1584,11 +1701,13 @@ async def magazine_clean_up_boxes(doc_id: str):
     # Last, so the boxes are fitted after the rules above have settled which ones remain.
     layout, snapped = await asyncio.to_thread(mag.snap_text_boxes, d, layout)
     report["snapped"] = snapped
+    token = _snapshot_for_undo(d, "Cleaned up the boxes")
     mag.save_layout(d, layout)
     articles = mag.load_articles(d)
     if _add_split_paragraphs_to_articles(articles, report):
         mag.save_articles(d, articles)
-    return {"ok": True, "backup": backup.name, "counts": {k: len(v) for k, v in report.items()}, "report": report}
+    _seal_undo(d, token)
+    return {"ok": True, "undo": token, "backup": backup.name, "counts": {k: len(v) for k, v in report.items()}, "report": report}
 
 
 def _set_page_advertisement(doc_id: str, page: int, value: bool) -> dict[str, Any]:
@@ -1635,8 +1754,10 @@ async def magazine_set_page_order(doc_id: str, page: int, request: Request):
         layout = mag.set_page_order(layout, page, order)
     except ValueError as exc:
         raise HTTPException(400, str(exc))
+    token = _snapshot_for_undo(d, f"Set the reading order of page {page}")
     mag.save_layout(d, layout)
-    return {"ok": True, "locked": True}
+    _seal_undo(d, token)
+    return {"ok": True, "locked": True, "undo": token}
 
 
 @router.delete("/api/magazine/{doc_id}/pages/{page}/reading-order")
@@ -1650,8 +1771,10 @@ async def magazine_clear_page_order(doc_id: str, page: int):
         layout = mag.clear_page_order(layout, page)
     except ValueError as exc:
         raise HTTPException(400, str(exc))
+    token = _snapshot_for_undo(d, f"Released the reading order of page {page}")
     mag.save_layout(d, layout)
-    return {"ok": True, "locked": False}
+    _seal_undo(d, token)
+    return {"ok": True, "locked": False, "undo": token}
 
 
 @router.post("/api/magazine/{doc_id}/reading-order")
@@ -1666,9 +1789,11 @@ async def magazine_recompute_reading_order(doc_id: str):
     if not layout:
         raise HTTPException(404, "No layout yet -- run ingestion first")
     layout, changed = await asyncio.to_thread(mag.recompute_reading_order, layout)
+    token = _snapshot_for_undo(d, "Fixed the reading order")
     mag.save_layout(d, layout)
+    _seal_undo(d, token)
     locked = sum(1 for p in layout.get("pages", []) if p.get("order_locked"))
-    return {"ok": True, "pages_changed": changed, "pages_locked": locked}
+    return {"ok": True, "pages_changed": changed, "pages_locked": locked, "undo": token}
 
 
 @router.post("/api/magazine/{doc_id}/figures/refine")
@@ -1713,9 +1838,11 @@ def _add_region_sync(doc_id: str, job_id: str, page: int, bbox: list[float], kin
         updated = mag.add_manual_region(d, layout, page, bbox, kind, text,
                                         vision_ocr_url, vision_ocr_model,
                                         vision_ocr_fallback_url, vision_ocr_fallback_model)
+        token = _snapshot_for_undo(d, "Drew a region")
         mag.save_layout(d, updated)
+        _seal_undo(d, token)
         background_job_update(job_id, stage="done", done=True, detail="Region added",
-                              result={"page": page, "kind": kind})
+                              result={"page": page, "kind": kind, "undo": token})
     except Exception as exc:                       # noqa: BLE001 - surfaced to UI
         background_job_update(job_id, stage="failed", done=True, error=str(exc)[:1000])
 
@@ -1812,7 +1939,8 @@ async def magazine_update_region(doc_id: str, page: int, block_id: str, request:
     except ValueError as exc:
         raise HTTPException(400, str(exc))
     # Taken once the change is known to be valid, from the files on disk (still the old state).
-    token = _snapshot_for_undo(d, "Changed a region") if (new_type is not None or new_text is not None) and d is not None else None
+    token = (_snapshot_for_undo(d, "Moved or resized a region" if new_type is None and new_text is None else "Changed a region")
+             if (new_type is not None or new_text is not None or bbox is not None or reocr) and d is not None else None)
     mag.save_layout(d, layout)
     if region_id != block_id:
         _move_article_membership(d, block_id, region_id, to_image=new_type == "image")

static/js/magazine.js

@@ -46,6 +46,8 @@ const state = {
   audit: null,           // live audit of the reviewed page ({page, data})
   flow: null,            // how the open article's text continues between regions
   flowOn: true,          // show the flow arrows/chips in review
+  history: null,         // {can_undo, can_redo, undo_label, redo_label} from the server
+  historyBusy: false,
   acronyms: null,        // the open article's glossary state
   queue: [],             // article ids for "play all" (empty = not queued)
   queueIndex: -1,
@@ -1614,6 +1616,13 @@ function renderReview() {
       // Back to the contents lives in the sticky bar, so it stays in reach while scrolling a long page.
       '<button type="button" class="btn-sm" id="mag-review-contents-btn" onclick="magBackToContents()" title="Back to the contents">' +
       '<span class="mdi mdi-arrow-left" aria-hidden="true"></span> Contents</button>' +
+      '<div class="mag-review-pagenav-group" role="group" aria-label="Undo and redo">' +
+      '<button type="button" class="btn-sm" id="mag-undo-btn" onclick="magHistoryStep(\'undo\')" aria-keyshortcuts="Control+Z" ' +
+        (state.history?.can_undo ? '' : 'disabled ') + 'title="' + esc(state.history?.can_undo ? 'Undo: ' + state.history.undo_label + ' (Ctrl+Z)' : 'Nothing to undo') + '">' +
+        '<span class="mdi mdi-undo" aria-hidden="true"></span> Undo</button>' +
+      '<button type="button" class="btn-sm" id="mag-redo-btn" onclick="magHistoryStep(\'redo\')" aria-keyshortcuts="Control+Shift+Z Control+Y" ' +
+        (state.history?.can_redo ? '' : 'disabled ') + 'title="' + esc(state.history?.can_redo ? 'Redo: ' + state.history.redo_label + ' (Ctrl+Shift+Z)' : 'Nothing to redo') + '">' +
+        '<span class="mdi mdi-redo" aria-hidden="true"></span> Redo</button></div>' +
       // One page stepper for the whole magazine, not a separate one bounded
       // to just this article's own pages — the two almost always agreed and
       // having both doubled the toolbar's width for no real benefit.
@@ -1746,6 +1755,7 @@ function renderReview() {
   magSpellLoadPage();
   magAuditLoad();
   magFlowLoad();
+  magHistoryRefresh();
 }
 
 // A Headline or Author region belonging to this article is exactly what its
@@ -2145,6 +2155,58 @@ async function magBulk(action, type) {
   await magReloadReview();
 }
 
+// ── Undo / redo (server keeps the last 20 recorded changes) ───────────────────
+function magHistoryPaint() {
+  const h = state.history || {};
+  const undo = $('mag-undo-btn'), redo = $('mag-redo-btn');
+  if (undo) {
+    undo.disabled = !h.can_undo;
+    undo.title = h.can_undo ? 'Undo: ' + h.undo_label + ' (Ctrl+Z)' : 'Nothing to undo';
+  }
+  if (redo) {
+    redo.disabled = !h.can_redo;
+    redo.title = h.can_redo ? 'Redo: ' + h.redo_label + ' (Ctrl+Shift+Z)' : 'Nothing to redo';
+  }
+}
+
+async function magHistoryRefresh() {
+  if (!state.doc) return;
+  try { state.history = await api('/api/magazine/' + state.doc.id + '/history'); } catch (_) { state.history = null; }
+  magHistoryPaint();
+}
+
+async function magHistoryStep(kind) {
+  if (state.historyBusy || !state.doc) return;
+  if (kind === 'undo' ? !state.history?.can_undo : !state.history?.can_redo) return;
+  state.historyBusy = true;
+  try {
+    const r = await api('/api/magazine/' + state.doc.id + '/' + kind, {
+      method: 'POST', headers: { 'Content-Type': 'application/json' }, body: '{}',
+    });
+    state.history = r;
+    state.undo = null;                              // the old "Undo" bar named a change that is now gone
+    const articleId = state.article?.id;
+    await magRefreshArticles();
+    state.article = (state.articles || []).find((a) => a.id === articleId) || state.article;
+    _magReviewChecked = null;
+    await magReloadReviewNow((kind === 'undo' ? 'Undone: ' : 'Redone: ') + r.label);
+  } catch (e) {
+    say('Could not ' + kind + ': ' + e.message, 'err');
+    await magHistoryRefresh();
+  } finally {
+    state.historyBusy = false;
+  }
+}
+
+document.addEventListener('keydown', (event) => {
+  if (state.view !== 'review' || !(event.ctrlKey || event.metaKey) || event.altKey) return;
+  const target = event.target;
+  if (target && (target.isContentEditable || /^(INPUT|TEXTAREA|SELECT)$/.test(target.tagName))) return;   // native undo of typing
+  const key = String(event.key).toLowerCase();
+  if (key === 'z' && !event.shiftKey) { event.preventDefault(); magHistoryStep('undo'); }
+  else if ((key === 'z' && event.shiftKey) || key === 'y') { event.preventDefault(); magHistoryStep('redo'); }
+});
+
 function magUndoBarHtml() {
   if (!state.undo) return '';
   return '<span class="mdi mdi-undo-variant" aria-hidden="true"></span> ' + esc(state.undo.label) + ' ' +
@@ -5011,6 +5073,7 @@ root.magDrivingCycleSpeed = magDrivingCycleSpeed;
 root.magDrivingToggleText = magDrivingToggleText;
 root.magDrivingToggleImages = magDrivingToggleImages;
 root.magDrivingPrevious = magDrivingPrevious;
+root.magHistoryStep = magHistoryStep;
 root.magFlowToggle = magFlowToggle;
 root.magLinkPage = magLinkPage;
 root.magAutofixPage = magAutofixPage;

tests/magazine.spec.js

@@ -90,6 +90,8 @@ async function mockBackend(page, { articleStatus = 'detected', hasAudio = false,
     json: { page: 3, uncovered: [], uncovered_ratio: 0, suspect_blocks: [], overlaps: [], clipped: [], measured: true, spelling: 0, advertisement: false },
   }));
   await page.route(`**/api/magazine/${DOC_ID}/spelling**`, route => route.fulfill({ json: { issues: [], dictionary: {} } }));
+  await page.route(`**/api/magazine/${DOC_ID}/history`, route => route.fulfill({
+    json: { can_undo: false, can_redo: false, undo_label: '', redo_label: '', undo_depth: 0, redo_depth: 0 } }));
   await page.route(`**/api/magazine/${DOC_ID}/articles/article-001/flow`, route => route.fulfill({
     json: { links: [], first: null, last: null } }));
   await page.route(`**/api/magazine/${DOC_ID}/pages/*`, route => route.fulfill({
@@ -464,6 +466,78 @@ test.describe('Magazine pronunciation and spelling checks', () => {
   });
 });
 
+test.describe('Magazine: undo and redo', () => {
+  async function openReview(page) {
+    await page.goto('/', { waitUntil: 'domcontentloaded' });
+    await page.waitForSelector('#app-skeleton', { state: 'detached', timeout: 15_000 });
+    await navTo(page, 's-magazine');
+    await page.locator('#mag-library-list .mag-card').first().click();
+    await page.locator('#mag-contents-view-toggle').click();
+    await page.locator('.mag-card-wrap').filter({ hasText: 'Page 3' }).getByRole('button', { name: /^Review/ }).click();
+    await page.waitForSelector('#mag-undo-btn');
+  }
+
+  // A tiny stand-in for the server's history: one change that can be undone, then redone.
+  async function mockHistory(page, calls) {
+    let state = { can_undo: true, can_redo: false, undo_label: 'Deleted 2 regions', redo_label: '', undo_depth: 1, redo_depth: 0 };
+    await page.route(`**/api/magazine/${DOC_ID}/history`, route => route.fulfill({ json: state }));
+    await page.route(`**/api/magazine/${DOC_ID}/undo`, (route) => {
+      calls.push('undo');
+      state = { can_undo: false, can_redo: true, undo_label: '', redo_label: 'Deleted 2 regions', undo_depth: 0, redo_depth: 1 };
+      return route.fulfill({ json: { ok: true, label: 'Deleted 2 regions', ...state } });
+    });
+    await page.route(`**/api/magazine/${DOC_ID}/redo`, (route) => {
+      calls.push('redo');
+      state = { can_undo: true, can_redo: false, undo_label: 'Deleted 2 regions', redo_label: '', undo_depth: 1, redo_depth: 0 };
+      return route.fulfill({ json: { ok: true, label: 'Deleted 2 regions', ...state } });
+    });
+  }
+
+  test('Undo and Redo sit right after Contents in the sticky bar and are disabled with nothing to do', async ({ page }) => {
+    await mockBackend(page);
+    await openReview(page);
+    const bar = page.locator('#mag-review .mag-review-pagenav');
+    await expect(bar.locator('> *').nth(0)).toHaveId('mag-review-contents-btn');
+    await expect(bar.locator('> *').nth(1).getByRole('button')).toHaveText([/Undo/, /Redo/]);
+    await expect(page.locator('#mag-undo-btn')).toBeDisabled();
+    await expect(page.locator('#mag-redo-btn')).toBeDisabled();
+    await expect(page.locator('#mag-undo-btn')).toHaveAttribute('title', 'Nothing to undo');
+  });
+
+  test('Undo, then Redo, call the server and swap which button is enabled', async ({ page }) => {
+    await mockBackend(page);
+    const calls = [];
+    await mockHistory(page, calls);
+    await openReview(page);
+    await expect(page.locator('#mag-undo-btn')).toBeEnabled();
+    await expect(page.locator('#mag-undo-btn')).toHaveAttribute('title', /Undo: Deleted 2 regions/);
+    await page.locator('#mag-undo-btn').click();
+    await expect.poll(() => calls).toEqual(['undo']);
+    await expect(page.locator('#mag-redo-btn')).toBeEnabled();
+    await expect(page.locator('#mag-undo-btn')).toBeDisabled();
+    await page.locator('#mag-redo-btn').click();
+    await expect.poll(() => calls).toEqual(['undo', 'redo']);
+    await expect(page.locator('#mag-undo-btn')).toBeEnabled();
+  });
+
+  test('Ctrl+Z and Ctrl+Shift+Z work in review but leave typing in a text field alone', async ({ page }) => {
+    await mockBackend(page);
+    const calls = [];
+    await mockHistory(page, calls);
+    await openReview(page);
+    await page.locator('#mag-rev-title').click();
+    await page.keyboard.press('Control+z');
+    await page.waitForTimeout(300);
+    expect(calls).toEqual([]);                                   // native undo of typing, not a magazine undo
+    await page.locator('#mag-review .mag-review-page').click({ position: { x: 5, y: 5 } });
+    await page.keyboard.press('Control+z');
+    await expect.poll(() => calls).toEqual(['undo']);
+    await expect(page.locator('#mag-redo-btn')).toBeEnabled();     // the undo has finished reloading
+    await page.keyboard.press('Control+Shift+z');
+    await expect.poll(() => calls).toEqual(['undo', 'redo']);
+  });
+});
+
 test('the Contents button is the first item of the sticky review bar and goes back', async ({ page }) => {
   await mockBackend(page);
   await page.goto('/', { waitUntil: 'domcontentloaded' });

tests/test_magazine_routes.py

@@ -13,7 +13,7 @@ os.environ.setdefault("CONFIG_DIR", _CONFIG)
 
 from fastapi import HTTPException
 
-from routes.magazine import _article_id, _doc_dir, _parse_script
+from routes.magazine import _article_id, _doc_dir, _history, _parse_script
 
 
 async def _run_sync(fn, *args, **kwargs):
@@ -1232,7 +1232,7 @@ class BulkUndoFoldTests(unittest.TestCase):
                     self.run_route(magazine_bulk_regions, 1, body=body)
                 self.assertEqual(err.exception.status_code, 400)
         self.assertEqual(len(mag.load_layout(self.dir)["blocks"]), 4)
-        self.assertFalse((self.dir / "undo.json").exists())
+        self.assertEqual(_history(self.dir)["undo"], [])
 
 
 class UndoSafetyAndExtraFieldTests(BulkUndoFoldTests):
@@ -1251,14 +1251,83 @@ class UndoSafetyAndExtraFieldTests(BulkUndoFoldTests):
         self.assertEqual(err.exception.status_code, 409)
         self.assertEqual(mag.load_layout(self.dir)["blocks"][0]["text"], "edited later")
 
-    def test_an_unsealed_old_snapshot_is_refused(self):
+    def test_an_unsealed_snapshot_is_refused(self):
-        import json
+        from routes.magazine import magazine_undo, _snapshot_for_undo
-        from routes.magazine import magazine_undo
+        _snapshot_for_undo(self.dir, "x")               # taken but never sealed: its aftermath is unknown
+        with self.assertRaises(HTTPException) as err:
+            self.run_route(magazine_undo, body={})
+        self.assertEqual(err.exception.status_code, 409)
+
+    def _ids(self):
         from core import magazine as mag
-        (self.dir / "undo.json").write_text(json.dumps({"token": "t", "label": "x", "layout": mag.load_layout(self.dir),
+        return sorted(b["id"] for b in mag.load_layout(self.dir)["blocks"])
-                                                        "articles": mag.load_articles(self.dir)}), "utf-8")
+
-        with self.assertRaises(HTTPException):
+    def test_undo_then_redo_puts_the_change_back_and_the_history_says_what_is_possible(self):
-            self.run_route(magazine_undo, body={"token": "t"})
+        from routes.magazine import magazine_bulk_regions, magazine_history, magazine_redo, magazine_undo
+        before = self._ids()
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg1"], "action": "delete"})
+        after = self._ids()
+        state = self.run_route(magazine_history)
+        self.assertEqual((state["can_undo"], state["can_redo"]), (True, False))
+        self.assertIn("Deleted", state["undo_label"])
+        undone = self.run_route(magazine_undo, body={})               # no token: the latest change
+        self.assertEqual((self._ids(), undone["can_undo"], undone["can_redo"]), (before, False, True))
+        redone = self.run_route(magazine_redo)
+        self.assertEqual((self._ids(), redone["can_undo"], redone["can_redo"]), (after, True, False))
+        self.run_route(magazine_undo, body={})
+        self.assertEqual(self._ids(), before)                          # and it can be undone again
+
+    def test_several_changes_are_undone_one_by_one_and_redone_in_order(self):
+        from routes.magazine import magazine_bulk_regions, magazine_redo, magazine_undo
+        start = self._ids()
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg1"], "action": "delete"})
+        one = self._ids()
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg2"], "action": "delete"})
+        two = self._ids()
+        self.run_route(magazine_undo, body={}); self.assertEqual(self._ids(), one)
+        self.run_route(magazine_undo, body={}); self.assertEqual(self._ids(), start)
+        self.run_route(magazine_redo); self.assertEqual(self._ids(), one)
+        self.run_route(magazine_redo); self.assertEqual(self._ids(), two)
+
+    def test_a_new_change_ends_the_redo_history(self):
+        from routes.magazine import magazine_bulk_regions, magazine_history, magazine_redo, magazine_undo
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg1"], "action": "delete"})
+        self.run_route(magazine_undo, body={})
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg2"], "action": "delete"})
+        self.assertFalse(self.run_route(magazine_history)["can_redo"])
+        with self.assertRaises(HTTPException) as err:
+            self.run_route(magazine_redo)
+        self.assertEqual(err.exception.status_code, 409)
+
+    def test_redo_is_refused_once_anything_else_changed(self):
+        from core import magazine as mag
+        from routes.magazine import magazine_bulk_regions, magazine_history, magazine_redo, magazine_undo
+        self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg1"], "action": "delete"})
+        self.run_route(magazine_undo, body={})
+        layout = mag.load_layout(self.dir)
+        layout["blocks"][0]["text"] = "edited later"
+        mag.save_layout(self.dir, layout)
+        self.assertFalse(self.run_route(magazine_history)["can_redo"])
+        with self.assertRaises(HTTPException) as err:
+            self.run_route(magazine_redo)
+        self.assertEqual(err.exception.status_code, 409)
+        self.assertEqual(mag.load_layout(self.dir)["blocks"][0]["text"], "edited later")
+
+    def test_only_the_latest_twenty_changes_are_kept(self):
+        from routes.magazine import _UNDO_DEPTH, _history, _seal_undo, _snapshot_for_undo
+        for i in range(_UNDO_DEPTH + 5):
+            _seal_undo(self.dir, _snapshot_for_undo(self.dir, f"change {i}"))
+        kept = _history(self.dir)["undo"]
+        self.assertEqual((len(kept), kept[0]["label"], kept[-1]["label"]), (_UNDO_DEPTH, "change 5", f"change {_UNDO_DEPTH + 4}"))
+        self.assertEqual(len(list((self.dir / "undo").glob("*.json.gz"))), _UNDO_DEPTH)
+
+    def test_nothing_to_undo_or_redo_is_a_plain_409(self):
+        from routes.magazine import magazine_history, magazine_redo, magazine_undo
+        for fn, kw in ((magazine_undo, {"body": {}}), (magazine_redo, {})):
+            with self.assertRaises(HTTPException) as err:
+                self.run_route(fn, **kw)
+            self.assertEqual(err.exception.status_code, 409)
+        self.assertEqual(self.run_route(magazine_history)["can_undo"], False)
 
     def test_drawing_a_region_on_a_layout_with_a_split_author_line_works(self):
         """"Could not add region: Block.__init__() got an unexpected keyword argument 'original_text'"."""