Skip to content

fix(webapp): restore is not offered on the version that is already current (BEA-57) - #110

Merged
ssowonny merged 1 commit into
mainfrom
bea-57-ph-scan-bug-restore-is-offered-on-the-version-that-is
Aug 3, 2026
Merged

fix(webapp): restore is not offered on the version that is already current (BEA-57)#110
ssowonny merged 1 commit into
mainfrom
bea-57-ph-scan-bug-restore-is-offered-on-the-version-that-is

Conversation

@ssowonny

@ssowonny ssowonny commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

TL;DR

  • Clicking restore on a file's newest version used to write a +0 −0 change to everyone's history — that button is gone now, since it's already the file's content.
  • The API refuses the same call with a 409, so no client can manufacture the empty row either.
  • Deleted files still restore, and restoring a genuinely older version is untouched.
  • The rule is content equality, not "row 0" — an older row hand-reverted to the current bytes loses the button too, deliberately.
  • Known gap: +0 −0 rows already written stay. Journals are append-only.

Fixes BEA-57.

Screenshots

Per-file history for guide.md — the top row is the file's current content:

before after
before after

The project-wide feed, where the same rule applies per path (and reaches inside run cards). restore survives exactly where it can do something: the DELETED scratch.md row, and every older version:

before after
before after

What was wrong

restoreSha(i) returned entries[i].blob for any non-delete row, and the newest entry for a path is its current content. HistoryRow's restorable never compared against the file's head. So the top row of every version list offered a restore that was a guaranteed no-op write — journaled under a real account, on a real device, replicated to every teammate, in the audit trail BearDrive's history story depends on. handleRestore accepted the same call from any client.

What changed

Server (internal/webapp/restore.go) — one guard, positioned deliberately:

decode → cleanUploadPath → sha regex → loadOps
     → 404 no such version of that path      (unchanged)
     → 409 already the current content       ◄── new
     → CheckWrite → Commit → RecordUsage

after the 404 so an unknown sha still 404s, and before CheckWrite so a refused restore records no quota. Head comes from journal.Replay, which is how the CLI already answers this question and which sorts internally — loadOps returns ops unsorted, so hand-rolling "last op wins" over that slice would have been wrong.

Client (HistoryView.tsx) — entries are strictly newest-first, so one walk gives each path's head; a newest delete maps to "nothing is current", which is what keeps deleted files restorable. restoreSha returns undefined for bytes that already are the head. No change in HistoryRow.tsx: restorable already requires !!restoreSha, so the button, its title and its busy state disappear together.

Matching the server's check exactly means the UI can never show a button that errors — which is why the rule is content equality rather than row index.

Not doing: a confirm dialog on restore. Its absence is deliberate (HistoryRow.tsx:154 — removal reaches every device destructively, a restore is additive). The defect was never "restore should ask", it was "restore should not be offered where it cannot do anything".

Deviation from the reviewed plan

One thing the plan didn't anticipate: an existing e2e assertion was pinning the bug.

browse.spec.ts asserted the run card's notes/readme.md edit row still offers a restore — but that seeded op is the head of that path, so under the new rule it correctly loses the button. The assertion (and the now-stale seed comment in e2e_serve_test.go calling that file "restorable") were updated to state the new rule, and the test still proves what it was really about: the run-created row's undo is a removal, and the edit row gets neither control. This also gives the rule coverage inside run cards, which the plan's e2e step didn't reach.

Tests

  • TestRestoreNoOpCurrentVersion — restoring the current version 409s with every journal byte-identical; the older version still 200s; and once it is current, restoring it 409s too, proving the guard is narrow.
  • TestRestoreAfterDelete gained a case: replay drops a deleted path, so restoring it goes through — and once the file is back, the same call is the no-op.
  • e2e "the current version offers no restore" — the seeded file's top row has no control and the row below does (read-only, so the rest of the suite still finds guide.md saying "Second version"); the API 409s the head sha and nothing is written; then, on its own file, a real restore leaves the new top row without a control and the hand-reverted first version without one either.
  • go test ./..., go vet ./..., and all 108 Playwright specs pass; internal/webapp/static rebuilt and committed (check-dist.sh: fresh).

Build session

cd $(git worktree list | grep bea-57 | awk '{print $1}') && claude --resume 18788a8c-9f04-428e-9d01-e1b66dec2dae

(only works on the machine that ran the build)

…rrent (BEA-57)

The newest row for a path IS the file's current content, so its `restore`
button could only ever journal a +0 −0 change — attributed to a real person,
on a real device, replicated to every teammate, in the audit trail the whole
history story depends on. It was also the single most tempting row to click.

One rule, enforced at both ends. handleRestore now 409s when the requested
sha is already the path's head (journal.Replay, the way the CLI already
answers this question), placed after the "no such version of that path" 404
and before CheckWrite so an unknown sha still 404s and a refused restore
records no quota. HistoryView computes each path's head from the loaded
window — entries are strictly newest-first, so a path's first occurrence
decides — and restoreSha returns undefined for bytes that already are the
head, which removes the button, its title and its busy state together.

The rule is content equality, not row index: an older row hand-reverted to
the current bytes is just as much of a no-op, and matching what the server
checks means the UI can never show a button that errors.

A newest DELETE leaves the path out of the replay, so a deleted file still
restores — that is a real change. Confirm-on-restore stays out, deliberately
(HistoryRow.tsx:154): the defect was never "restore should ask", it was
"restore should not be offered where it cannot do anything".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ssowonny
ssowonny requested a review from thefron August 3, 2026 01:15
@ssowonny
ssowonny merged commit d7772a2 into main Aug 3, 2026
2 checks passed
@ssowonny
ssowonny deleted the bea-57-ph-scan-bug-restore-is-offered-on-the-version-that-is branch August 3, 2026 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant