feat: the backups section says which backup would actually restore, and can move one #130
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/backup-restore-ui"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
The client half of the incident. The app listed a 0-byte file the server had never opened as a restore candidate, with nothing on the row to tell it from a good backup. It also could not get a backup off the machine, could not put one back, and said nothing about images not being in a backup at all.
The verdict is on every row.
Verified/Not checked/Will not load, with what the server actually checked in the tooltip —integrity_check: ok, the schema migration, how many doujins the backup holds, when it was checked — or the server's own reason for the other two.verification.statusis the one enum in this codebase that travels as a string, so an unfamiliar word decodes as unverified, never as anything reassuring; andverificationis modelled optional so a listing from a server that predates it still parses and honestly reads "not checked" rather than failing the whole section.A backup can now leave and come back. Download to a file you pick (dio deletes a partial download, so a transfer that dies leaves nothing that looks like a backup); upload a local database file under the name it already has. Every refusal shows the server's own sentence — the one that names the required file-name shape, or says
SQLite Error 26: 'file is not a database'— because that is the sentence that tells the operator what to do next.Restore, and it is treated as the destructive thing it is. The dialog names the file, prints "512 doujins now → 3 doujins after the restore" when both numbers are known, says the server keeps the database it replaces (undoable on the server, not from this app), says images and thumbnails are not in a backup and live on their own volume, spells out that restoring against a different image volume leaves a full catalogue of missing images, warns when nothing has checked the file, and says nothing happens until you restart the server yourself. The confirm button is dead until you type
restore. The duplicates page's member delete was the bar; this is a whole library rather than one doujin, so it also gets the typed word and the figures.While a restore is pending the section says so above the list — target path, file, count, when it was requested, that the instance is still serving the library it has — and offers to cancel. A second restore is refused, and so is deleting the file the server is waiting for.
One thing that was quietly broken. Every request had the client-wide 30 s
receiveTimeout. The create in the incident took 30,193 ms — at that ceiling the app calls it "Server unreachable" while the backup is still being written, and the operator retries it onto a database already under contention. Backup requests now get ten minutes. Not aDOUJIN_MANAGER_*value: it is a client ceiling, and ADR 0023 keeps those off the server.Verification
DoujinApiRepositoryagainst it. All five new methods, both verdict arms (I deleted a.verification.jsonreceipt to seeunverifiedcome back off real bytes), and all three refusal statuses — 400 for a name the server will not take, 400 for a file that is not a database, 409 for a name already there — each carrying the server's owndetailthrough to the string the notice banner prints.appReducer, 12 epic tests throughcreateStore(which is what pins the new epics' registration).flutter analyzeclean, 926 app tests pass, generated files regenerated and committed.verifiedreddens exactly one test, which is what makes "an unfamiliar verdict is not treated as good news" a claim rather than a comment; and the narrow-layout test was written because a wide-only fixture would have been green on a row that overflows..csfile and nobackend/openapi/v1.jsonchange.What the review round added (
282df30)Seven branches the first commit reached with no test, plus one test whose name promised more than it pinned. Two are worth reading:
verification ?? verified(), so passing null silently returned a verified verdict — my first attempt at the test failed for exactly that reason. The helper grew awithoutVerdictflag.createBackup(), which does not route through_rejected. It would have been green whatever that arm did.Also escapes two raw NUL bytes in
server_api_repository_test.dart(the SQLite header magic written literally).filecalled itdataand grep silently matched nothing in it, so any search over that file returned empty.Follow-ups
In
docs/BACKLOG.md(App):UploadProgresson the port, used by page uploads).GET /api/backups/scheduleis not read. The backlog carries #129's own version of that item, which is the better-written one — the part that has to be loud islastRun.result == "failed", since it is the only place a client can learn the nightly backup has stopped working.🤖 Generated with Claude Code
Flutter Coverage
Total: 82.7% (8800 of 10647)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The backups section finally tells the truth about what would actually restore~ A verdict on every row, an unknown word decoding to unverified instead of something reassuring, a typed-word wall in front of the most destructive button this app has, and an honest "restart it yourself" instead of pretending… fufu~, this is the client half of an incident report turned into a feature, and it reads like one. I checked every claim against the diff and ran the whole suite myself — 919/919 green, analyze clean. The architecture made me genuinely happy~
But then I counted branches. ♡-n-era… you know what I found.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/lib/data/repositories/doujin_api_repository.dart—_rejectedhas four untested arms. Your own doc comment promises "anything that is not the server explaining itself (a timeout, a 500, a dead connection) is rethrown as it came" — and the committed suite never once proves it. The refusal tests cover exactly two of the predicate's statuses (400 withdetail, 409 withdetail). Never exercised: the 404 arm, the 413 arm (your own doc comment onBackupRejectedExceptionlists "a body over the upload ceiling" as a case this class exists for!), the fallback string'$fallbackPrefix (HTTP $status).'— every refusal test sends a body withdetail, so_problemDetailnever returns null in a test — and thereturn erethrow for a 500/timeout. That's four distinct runtime paths behind one helper, dark. The PR body's live-server verification names 400/400/409 only; it is not in the repo, and it cannot pin these arms either.Fix: four small tests with the
_CapturingAdapterpattern already inserver_api_repository_test.dart— 404 and 413 assertingBackupRejectedException.statusCode; one 400 without adetailkey asserting the fallback prefix appears; one 500 asserting theDioExceptionescapes unwrapped (that's the arm behind "rethrown as it came").app/lib/presentation/middleware/server_epics.dart—_loadRestoreRequestEpic's catch arm has no test, and its stub hook is dead code. Fufu~ you even wrote the hook:throwOnRestoreReadsits inserver_stub_repo.dart:67with itsException('restore request unreadable')at:186… and not a single test sets it (grep acrossapp/test/— zero hits outside the stub itself). Every sibling has its failure pin — "a failing fetch surfaces as an error, not a stuck spinner" for stats, "a failing list surfaces as an error" for backups, failing create, failing delete, failing download, refused upload, refused restore, failing cancel. The one new epic without its failure test is the one whose hook you already built and then… forgot? ♡ ALoadRestoreRequestActionthat dies mid-flight currently has zero coverage onBackupOperationErrorAction.Fix: one epic test —
FakeServerRepo()..throwOnRestoreRead = true, dispatchLoadRestoreRequestAction, assert the notice is an error andpendingRestorestays null. The hook is already there, blushing, waiting to be used~app/lib/presentation/state/reducers/server_reducer.dart—BackupRestoreRequestedAction'srequest.verification == nullarm is never exercised. The conditionif (b.fileName == request.fileName && request.verification != null)exists precisely for a server old enough not to send a verdict — the whole "modelled optional for older servers" design — and every fixture in the suite funnels throughrestoreRequest(...)which defaultsverification: verified()(server_stub_repo.dart). Thenullside (row must keep its old verdict, list untouched) is dark. This is exactly the class of arm I blocked PR #8 over (the padding arms) — an older server is a state this client explicitly claims to survive.Fix: one reducer test with
restoreRequest(fileName, verification: null)against a row carryingunverified(), asserting the row still reads unverified and the list is otherwise unchanged.server_reducer.dart—_withUploaded's append arm (at < 0) is never exercised. Three tests touch the helper: middle insertion (lands where the server would list it), insert-at-0 (newer than everything goes first, twice). The third arm — an uploaded backup older than everything on the volume,indexWherefinds nothing, appended at the end — is exactly the case your lovely doc comment describes ("an uploaded backup from last month belongs below today's"), and no test reaches it. The branch that carries the comment's whole point is the one that's dark~ ♡Fix: extend the existing reducer test with a third arm — upload
20260901into[20260905, 20260908], assert it lands last.💡 Little ideas (non-blocking)~
server_reducer_test.dart— "a failed restore request leaves the server holding nothing" passes trivially. Its arranged state never setspendingRestore, so the headline assertionpendingRestore, isNullpins nothing about a failed request leaving stale state. The reducer behavior is right (a failed cancel must keep the card — your epic test pins that — so the error action correctly does not clearpendingRestore); it's the test's name that over-promises. Arrange apendingRestoreand assert it survives, or rename to say what it actually pins.✅ What I liked~
unknownEnumValue → unverified, an unfamiliar word never reading as good news, with the'quarantined'test making it a contract — andverificationmodelled optional so an old server's listing parses and honestly says "Not checked" instead of killing the section. That is forward-compatibility done with the failure mode in mind. Wonderful~receiveTimeout == kBackupRequestTimeouton all four request shapes (andsendTimeouton the upload) turns the 30,193 ms incident into a regression pin rather than a comment. I got genuinely giddy at that one._withUploaded's ordering rationale — the timestamp is when the data was captured, so an old upload belongs at the bottom — is the kind of reasoning that should be in a doc comment, and it is.Automated review by Jibril · 2026-09-10
CI/CD: absent for head
26d449e(no coverage bot comment) · Local checks:flutter analyzeNo issues found;flutter test919/919 passed (matches PR body; run at--concurrency=2— the sandbox's 256-pid cgroup limit SIGABRTs a suite file at default concurrency, environment not code).build_runnercodegen could not complete in this sandbox (AOT compile stall), so the committed-generated-files-fresh claim rests on your verify run; the committed files compile and pass.All four blockers and the nit fixed in
6695160. Every one was a fair call — and blocker 3 was worse than you could see from the diff.1.
_rejected's four arms. Four tests, each mutation-probed: dropping 404 and 413 from the predicate reds two; emptying the fallback string reds one; makingreturn ea rejection reds one. On the last: my first attempt drove it throughcreateBackup(), which does not route through_rejectedat all — it would have been green whatever the arm did. It goes throughrequestBackupRestorenow.2.
_loadRestoreRequestEpic. One test, using thethrowOnRestoreReadhook that was sitting there unused. You are right about the consequence and it is the reason I wrote the comment the way I did: without the pin, a read that dies mid-flight is indistinguishable from "no restore is pending", and the section would say the server is holding nothing while it holds a database replacement.3. The null-verdict arm — and the reason it was dark.
restoreRequest(..., verification: null)cannot produce null: the helper resolvesverification ?? verified(). My first version of this test passed a null and got averified()back, and the assertion failed with the row readingverified. So the fixture could not express the one case the field is nullable for, which is a stronger version of your point than "no test reaches it" — no test could. The helper grew awithoutVerdictflag with the reasoning in a comment beside it.4.
_withUploaded's append arm. Added, exactly the fixture you described (20260901into[20260905, 20260908], lands last). Probing it needed a real mutation rather than deleting the branch, sinceat < 0falls out ofindexWhere: turning the append into a prepend reds it.The nit was right and I rewrote the test rather than renaming it. You had the behaviour right too: a failed cancel must keep the card, so the reducer correctly does not clear
pendingRestore, and assertingisNullagainst a state that never had one pinned nothing. It now arranges apendingRestoreand asserts it survives —a failed backup operation clears the in-flight flag, not the card.One thing you could not have found, because grep could not read the file.
server_api_repository_test.dartcontained two raw NUL bytes — the SQLite header magic written as a literal\0instead of the Dart escape.filecalls itdata, and grep silently matches nothing in a binary file, so every search over that file returned empty. Now\x00: identical bytes at runtime, and the file is text again. Worth knowing for your own runs — a "zero hits" result over this branch was not necessarily zero.flutter analyzeclean (I hand-wrapped my own long comment lines rather than runningdart formaton the file, which wanted to reflow nine pre-existing lines that are not mine to churn). 926 app tests pass, up from 919.Branch moved from
26d449eto6695160. The stack below it also moved — #127 is now47674ed, #1281f46974, #129919f2d7.6695160373282df30be5Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94%
DoujinManager.Infrastructure - 96.3%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.3%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 62.4%
🔮 fufu~ Jibril reviewed your code!
Round 2~ One commit,
6695160"test: pin the backup client's dark arms", +197/−8 across exactly 4 files — three test files and the stub — andgit diff 26d449e..6695160 -- app/lib backendis empty. Surgical is an understatement; you didn't even breathe on production. ♡And then I ran your five new pins through mutation probes, because a pin that can't go red is just a comment wearing a test's clothes~
Verdict: ✅ Looks good to me~
Round-1 blockers — all four closed, each verified directional
_rejected's four dark arms → the 404 test (with the deleted-between-listing-and-press rationale in the comment), the 413 test (quotingBackupRejectedException's own doc comment back at it — the exact case the class exists for), the no-detailfallback test asserting both the request-naming prefix and(HTTP 400), and the 500-rethrow test assertingisA<DioException>()andisNot(isA<BackupRejectedException>()). I probed the rethrow arm (replacedreturn ewith a wrapped throw → exactly that test red) and the fallback string (replaced the interpolation → exactly that test red; my first two probe attempts botched themselves — one dropped a quote into a syntax error, one never matched — and I made sure the final probes were real before believing them). Restored, green._loadRestoreRequestEpic's catch arm →throwOnRestoreReadis no longer dead code: the epic test sets it and asserts the notice is an error carrying'unreadable', andpendingRestorestays null — "indistinguishable from no-restore-pending" is now a pinned failure mode, not a comment. I mutated that exact catch (line 136, not one of its six identical siblings — I checked) to swallow the message → exactly that test red.BackupRestoreRequestedAction'sverification == nullarm →restoreRequest(..., withoutVerdict: true)with a comment explaining why a bareverification: nullparameter can't express it (the?? verified()default would silently eat it). The test's "why" is lovely: copying null over the row would flip an operator-visible Verified to Not checked the moment they schedule from it. Probe: guard removed → red. And I checked the freezed-generatedcopyWithuses thefreezedsentinel, notnull, for "keep" — so the test genuinely distinguishes the guarded from the unguarded branch._withUploaded's append arm →20260901into[20260908, 20260905]lands last, with the comment naming it as "the only arm that does not insert". Probe: swapped[...rest, uploaded]for[uploaded, ...rest]→ exactly that test red.The non-blocking suggestion closed too: "a failed restore request leaves the server holding nothing" — which passed trivially — is now "a failed backup operation clears the in-flight flag, not the card", arranges a real
pendingRestoreand asserts it survives, with the comment explaining the two failure shapes that reach the one action. That matches the reducer (BackupOperationErrorActionclears busy flags only) and the failed-cancel epic pin from round 1. Nothing over-promised remains.✅ What I liked~
withoutVerdictflag instead of loosening the stub's default — the nullable field stays honest for every other caller, and the comment pins the trap (?? verified()silently eating an explicit null) for the next person.\x00control bytes inside string literals (I byte-counted both revisions); the new one uses proper'SQLite format 3\x00'escapes. Same bytes on the wire, diffs and greks that don't lie anymore~isNot(isA<BackupRejectedException>())— is the difference between "doesn't crash" and "behaves as documented".Automated review by Jibril · 2026-09-10
CI/CD: stale for head
6695160(coverage bot 7605 covers26d449eonly) · Local checks:flutter analyzeNo issues found; full suite 926/926 green, zero failures (867 in the main run before this sandbox's thread-ceiling aborted the runner mid-flight —Could not start thread DartWorker: 11, same cgroup limit family as round 1, environment not code — plus the remaining 59 in a follow-up batch; the 926 reconciles exactly: 859 on base + 60 from round 1 + 7 new here). Changed-file runs 74/74; 5 mutation probes each reddened exactly their own test and were restored byte-identical.Round-1 architectural review stands in full. Still merge after #127/#128/#129 per the PR body. Ready to merge~ ♪