feat: look at what the detector found #192
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/reader-region-overlay"
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 reader's detected-region overlay — story 1.5 of
docs/TRANSLATION_PLAN.md, and the last code of
phase 1. A toggle in the reader's top bar outlines what a detector found on
the page, colour-coded by kind, off by default.
The state is the answer, not the list — on this side too.
notAnalysedand
analysedboth arrive with no regions, so the overlay captions the pagerather than leaving an empty one ambiguous: Not analysed yet against
Nothing found on this page, and a third for the page the detector could not
read. A page that does have outlines is not captioned — they are on the page,
and a caption would cover the thing the overlay exists to show.
The outlines land where the bitmap did, by construction. The reader draws
pages with
BoxFit.contain, which letterboxes, so a region at x = 0.5 ishalfway across the page, not the window.
containedImageRectasks Flutter'sown
applyBoxFitthe same question theImageasked, rather than re-derivingthe maths where it could drift.
Everything the overlay decides is a pure function —
containedImageRect,regionPaths,regionColour,regionOverlayMessage— so what it draws andwhat it says are asserted in tests instead of eyeballed in a screenshot.
The toggle is a client preference (ADR 0023), persisted by load-modify-write.
The epic loads what is on disk, changes the one value and saves it back.
Rebuilding a
StoredSettingsfrom the settings slice instead would meanlisting every field at a second site, and a field forgotten there silently
clears the server token or the API key. The Settings form has no control for
this one, so the save epic carries the current value through from state —
without that, saving any setting would quietly turn the overlay off on disk.
That is pinned by its own test.
Requests are made only for the pages on screen, and only once per page: a
variant runs to hundreds of pages, and an entry that is loading, loaded or
failed all count as asked, so one failing page cannot become a request per
rebuild.
Also here:
scripts/verify.shnow checks thatvendor/openrouter_dartis atthe commit
mainpins, not merely that it is non-empty. A submodule left onanother commit fails
flutter analyzeon a type the app uses everywhere,which reads as broken app code rather than as a stale checkout — and
pub getagainst it rewrites
app/pubspec.lockon the way. It cost an hour to diagnoseonce; the guard names the two commands that fix it.
Verification
scripts/verify.shgreen (== verify OK (all) ==), run again after mergingmain— backend 1,503 passed / 5 skipped, app 1,522 tests.reader_page_test.dart— off by default and silent by default, switching it on asks about the page already on screen, N regions draw N paths, switching it off draws none, a page already asked about is not asked about againregion_overlay_test.dart— placement, path count, the repaint predicate, and that a tap goes straight through to the page underneathregion_overlay_test.dart— the three empty answers (nothing looked / looked and found nothing / could not be read) and a failed request all read differentlyreader_state_test.dart(in flight, arrived, refused; closing forgets),region_overlay_epics_test.dartthroughcreateStore(so registration is pinned too)secure_storage_settings_test.dart,stored_settings_test.dart— the round trip, the pre-upgrade store, and that changing the overlay keeps every other settingserver_api_repository_test.dart,server_models_test.dart— the route, the envelope, unknown states and unknown kindsMutation-probed: 28 mutations, all killed. Four survived the first pass and
each one was a real finding:
isLoadingarm, both ofwhich every input already reached another way. Deleted rather than pinned,
and the deletions re-probed to confirm the remaining guards are live.
placement test survived having its x and y transposed. Replaced with a shape
asymmetric in both axes, every edge of its bounds asserted.
toggled the overlay on twice, which does not run the request hook a second
time. It now switches off and on again, which does.
The merge with #190
#190 landed on the same settings surface while this was in flight. Every
conflict was "both added" and was resolved by keeping both — but one thing
that was not a conflict would have been silent damage:
StoredSettings.withRegionOverlaycarries every field explicitly, so main'stwo new ones (
agentRouting,agentReasoningEffort) had to be added to it,or pressing the overlay toggle would have reset the assistant's routing and
reasoning effort. The test guarding that method enumerates the fields too, so
it needed the same edit; without it the test would have passed over the bug it
exists to catch.
docs/BACKLOG.md's phase-1 entry was refreshed for the samereason — it still said the reader overlay was what remained.
Follow-ups
One new entry in docs/BACKLOG.md: the
verify.shsubmodule-pin check has no test — the condition was checked by hand, there is
no shell harness in this repo, and one guard does not justify inventing one.
Phase 1 has no code left. What remains is not a PR: point a deployment at
a model, switch the sweep on, let it finish, and page through the library with
the overlay on. Nobody has yet looked at what this detector actually draws on
this library, and everything phase 2 would build rests on that opinion.
🤖 Generated with Claude Code
Flutter Coverage
Total: 86.5% (11587 of 13395)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.9%
DoujinManager.Infrastructure - 96.8%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
9CDB080D11D98B3E0D3ADB24949B4D7D90DD87AD1CEB5845F__Entry_0
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 67.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A PR whose body argues the state is the answer, not the list — and then proves it in the model, on the wire, in the reducer, and in the words on screen, each with its own test. Mutation-probed 28 ways, and the four survivors were real findings. I was having such a good time~
And then I probed two things your 28 mutations didn't. Fufu~ ♡ you wouldn't leave THESE in production, would you?
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
region_overlay.dart:62↔:182–189— a skipped region recolours every outline after it.regionPathsskips an outline with fewer thanminimumOutlinePoints(continueat:62), but the painter strokespaths[i]coloured byregions[i].kind(:188). Those two lists are index-aligned only while nothing is skipped — and you yourself built two ways to skip: the short-outline guard, andpointCount's odd-length truncation (an outline of 5 coordinates truncates to 2 points → skipped). One corrupt row and every region after it wears the wrong colour — a text outline drawn in balloon-yellow, silently, on the exact screen that exists to judge which kind is which. Mechanically reproduced at head05da2d1: a[short balloon, text triangle]list draws the text triangle in0xFFFFD600(balloon) instead of0xFF00E5FF(text). Your test 'an outline too short to enclose anything is skipped' asserts the path count but never the path↔colour pairing — all 192 tests in this PR's changed files stay green while the painter paints the wrong thing.Fix: keep the path and its kind together — have
regionPathsreturn the pairs (List<(Path, ImageRegionKind)>, or filter to drawable regions first and zip), and have the painter iterate those. Add the regression test: a skipped region followed by a normal one, assert the survivor's colour is its own.reader_page.dart:1002/region_overlay.dart:204— the caption is a dead tap zone.RegionOverlaywraps itself inIgnorePointerand your own test 'never takes a tap away from the page under it' states the principle in words — butRegionOverlayNotedoesn't, and_fittedPagepins it to the page's bottom-left, exactly where a reader taps to turn back. Mechanically reproduced: aGestureDetectorunderneath aRegionOverlayNote, tap the note → the tap never arrives. Every captioned page — and mid-sweep that is most of a library (notAnalysed, failed, nothing-found) — has a patch that eats page-turns, in the feature whose sibling explicitly refuses to.Fix: wrap
RegionOverlayNoteinIgnorePointer(it is words to read, not a control), and extend the tap-through test to cover the note as well as the outlines.Not a defect, but know before you merge: main moved under you — #190 conflicts in
secure_storage.dart,stored_settings.dart,epics.dart,settings_reducer.dart,app_state.freezed.dartand the storage test (mergeable: false, verified withgit merge-tree). A rebase is due.💡 Little ideas (non-blocking)~
epics.dart_loadImageRegionsEpic— a response that lands afterClearReaderActionre-adds its entry, so "closing forgets" quietly un-forgets. Benign (the data is per-content and real), but if you want close to mean it, the epic can drop answers for images the fresh reader state no longer knows.region_overlay.dart:113—'Regions unavailable: $error'interpolatese.toString(), so the caption reads "Regions unavailable: Exception: connection refused". The class name isn't words the reader shows; trim to the message.✅ What I liked~
containedImageRectaskingapplyBoxFitthe same question theImageasked, instead of re-deriving letterbox maths that could drift. Correct instinct~withRegionOverlayas a narrow explicit copy, with the token/API-key erasure hazard written down and caught bystored_settings_testif a field goes missing.Automated review by Jibril · 2026-09-21
CI/CD: inconclusive for head
05da2d1—CI / buildgreen,CI / test+Flutter CIstill queued · Local checks:flutter analyze0 issues; changed-file suite 192/192 pass; 2/2 review probes red🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Story 1.5, the very last code of phase 1 — the thing the whole detection story has been building toward! A toggle that outlines what the detector found, so a human eye can finally judge it. I read every line of this diff twice, you know~ The three-state-is-the-answer design (notAnalysed vs analysed-empty vs failed), the
applyBoxFitreuse instead of re-deriving the letterbox maths, the load-modify-write persistence that refuses to rebuildStoredSettingsat a second site — this is careful, deliberate work and I gushed a little~ ♡But then I found a silly little bug hiding in the painting code, and I probed it. Mechanically. It's real.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/lib/presentation/pages/reader/region_overlay.dart:182-190—RegionOverlayPainter.paintcolours paths by the wrong region's kind when any region is skipped.regionPaths()skips regions whose outline is shorter thanminimumOutlinePoints(line 62), so the path list and the region list are no longer index-aligned — butpaintstrokespaths[i]withregionColour(regions[i].kind). The moment one corrupt payload lands in a list, every region after it is drawn in its neighbour's colour: a text outline rendered balloon-amber, a panel rendered text-cyan. For an overlay whose entire purpose is "tell the kinds apart at a glance," a colour-lying overlay is worse than no overlay~I probe-tested this:
[short(panel), text, balloon]through the pristine head draws path 0 in0xFF7C4DFF— the panel purple of the region that was skipped — where it must be text cyan0xFF00E5FF. Your own test suite never catches it becauseregionPaths' skip test asserts onlyhasLength(1)(path count, never colour), and the painter tests use same-colour or count-only expectations. This is exactly the "fixture too symmetric" class you hunted in your own mutation probes — the transposition twin of the one you fixed~Fix: carry the pair together — build
(path, colour)records in one pass (for (final r in regions) if (r.pointCount >= minimumOutlinePoints) ...), or haveregionPaths/paint iterate a filtered list. I scratch-verified a pairs-based fix turns my probe green while your full suite stays green.Please also pin it: a painter test drawing
[short, text, balloon]and asserting both stroke colours (..path(color: …)is a named arg in this Flutter) — count-only assertions cannot see this bug class.Everything else, I checked and re-checked:
GET /api/images/{id}/regionsroute,statestrings (notAnalysed/analysed/failed), intkind, flat outline — all matchDetectionDtos.cs+DetectionEndpoints.csat this base.minimumOutlinePoints = 3matchesRegionGeometry.MinimumPoints = 3. ♪ImageRegionsEntry, entries surviving failures (no re-ask loop),ClearReaderActionwipe, save-epic carryingshowRegionOverlayfrom state — all pinned throughcreateStore, including the "saving settings doesn't turn the overlay off" test. Lovely~withRegionOverlaynarrow-copy with the whole-class assertion; pre-upgrade store,'yes'non-word, explicit-'false'write — all covered._recordProgress/_precacheAroundat all five sites; spread anchoring via_anchorFor/_lastVisiblePageForis correct for deep links and second halves.2a831e0):flutter analyze= No issues found; 116/116 + reader_page 78/78 green. The one deliberate silent catch (on Objectin_persistRegionOverlayEpic) has documented rationale and sibling precedent in the offline-save epics — accepted.✅ What I liked~
applyBoxFitso the outlines land where the bitmap lands by construction — asking Flutter the same question instead of re-deriving the maths. Elegant~Fix the colour pairing, pin both stroke colours, and this merges. I'll be watching~ ♡
Automated review by Jibril · 2026-09-21 (CEST)
CI/CD: absent for head SHA
2a831e0· Local checks: analyze 0 issues, 194/194 relevant tests green; blocker probe-verified red-on-head/green-on-fix7ca9c0343e72aa427402🔮 fufu~ Jibril reviewed your code!
Back so soon~? I re-derived the whole increment at
72aa427on a clean clone — this is the round where the board gets wiped, because the fix commit is exactly the three bullets from my last visit and nothing else.+213/-36across 5 files, zero scope creep, and every fix carries the pin I asked for. You even renamed the fixture helper'skindtorequiredso a symmetric fixture can never quietly reintroduce the transposition class — that's the instinct I've been nagging everyone about all phase 1~ ♡Verdict: ⛔ I can't let this pass~ ♡ …almost. One thread left loose.
⛔ These need fixing before I'm satisfied~
epics.dart:750-754—_readableFailure's else-arm is dead on the tree. The only Exception the epic can catch flows from stubs/IO that stringify with the"Exception: "prefix, sotext.startsWith(prefix) ? … : texttakes its true branch in every test that exists — andserver_api_repository_test.dartpinsException('connection refused').toString()straight intoerror. When a subclass arrives (SocketException('connection refused')→"SocketException: connection refused"— or an ApiException, sinceserver_api_repository.getImageRegionsrethrows asApiException(...)already stringified by.toString()and does not carry the prefix), the function silently produces an unstripped class name — and worse, when a future repo change starts throwing an exception whosetoString()no longer carries the prefix, every test asserting'connection refused'goes red on a production change that didn't touch this function. The untested branch is the exact shape that was a blocker in Tsumugi PR#8 round 3 (MergeSegmentWithPreviousjoin ternary 50%), and it's load-bearing in both directions: it's the only guard against a class name leaking into the caption and the only thing keeping future throw-site refactors from tripping it blind. The fix is two lines: a direct unit test withArgumentError('connection refused')(or similar non-"Exception: "-prefixed type) asserting the caption reads without the class name — the function is a pureString-in/String-out helper begging for its own pin.💡 Little ideas (non-blocking)~
epics.dart:713-714—_loadReaderEpicstill feedsReaderErrorAction(e.toString())raw ("Reader unavailable: Exception: …"), and sibling_loadImageRegionsEpicnow strips. Sibling-consistency polish for a follow-up — not this PR's scope, and I won't hold phase 1's last PR hostage to a caption nobody reads mid-error~✅ What I loved~
typedef RegionOutline = ({Path path, ImageRegionKind kind})— path and colour made inseparable at the type level, so the bug class is now unrepresentable rather than merely patched. The doc comment narrating the two skip-sources (short-outline guard + odd-coordinate truncation inpointCount) is exactly the record-keeping this repo's BACKLOG culture demands.a skipped region does not hand its colour to the next one), the order pin (every outline keeps its own kind, in order), and the canvas-levelpaints ..path(color: text) ..path(color: balloon)— count-only expectations explicitly called out as "what let the bug through the first time." That's a test author writing for the next reader.its caption never takes a tap away either) taps the note's own center — the exact point page-back taps land — not just somewhere nearby.ImageRegionsLoadedActionafterClearReaderActionis the kind of comment that kills a future reviewer's false-positive before it's typed.Automated review by Jibril · 2026-09-21
CI/CD: pending for head
72aa427(build ✓, Flutter analyze-and-test + test queued) · Local checks: analyze clean · full app suite 1527/1527 · mutation probes 3/3 red-on-own-test (painter index-drift, note IgnorePointer removed,_readableFailurereverted)Increment verified:
2a831e0→72aa427, +213/-36, 5 files. Clone /workspace/dm-pr192/repo pristine at72aa427(post-probe md5 match).Fixed at
9685013. Thank you for the third pass — and my apologies for pushing the last two rounds without replying here; the reasoning belongs in the thread, not only in commit messages.The blocker:
_readableFailure's else-armYou were right that the arm was untested, and writing the pin you asked for found something worse: the other arm was broken.
Generalising the strip, my first pattern was
^[A-Z][A-Za-z0-9_]*(Exception|Error):— which silently matches nothing for a bareException:, because[A-Z]consumes theEthe alternation needs and there is no way to backtrack into it. The regression test went red on the first run. Without it I would have shipped a strip that no longer stripped the one case it was originally written for. The capital is now asserted with a lookahead rather than consumed:One correction to the finding, because it changed the fix. I traced what actually reaches the epic before choosing a shape.
_ErrorMappingInterceptorwraps everyDioExceptioninApiException, andApiException.toString() => _message— the api client's own short sentence, with no type prefix at all. So the else-arm is not a fallback for an exotic case; it is the production path, and the strip exists for what escapes that layer — aFormatExceptionout of a malformed payload being the realistic one.ApiExceptionnever had a class name to leak.That is why the pattern is narrow and anchored rather than "everything up to the first colon": those real messages carry colons.
Server unreachable at nas.local:8080 — it said: no route to host.has two.Three pins, one per arm:
a subclass gets its type name trimmed tooFormatException: the date is not a date→the date is not a datea message that was already human survives wholea message that quotes an exception keeps the quote^anchor:The server said: FormatException: …keeps the quoteThat third one exists because the anchor survived my first probe round — nothing in the fixtures had a type name mid-string, so
^could be deleted with everything green. It cannot now.Probed: 6 mutations on the changed code, all red — the literal-
Exception:pattern, no strip at all, a blunt^.*?:, the anchor removed, plus the painter index-drift and the note'sIgnorePointerre-checked from the previous round.The non-blocking idea
_loadReaderEpicnot stripping while_loadImageRegionsEpicdoes — agreed, and deferred todocs/BACKLOG.mdrather than done here, with the note that the difference is mostly invisible in practice for the reason above: both epics usually carry anApiException, which has nothing to strip.One thing I did not take, from round one
Dropping a regions answer that lands after
ClearReaderAction, so "closing forgets" is literally true. I left it and wrote the reasoning into the reducer: nothing renders that slice with no reader open, and the nextLoadReaderActionreplaces it wholesale, so the entry cannot outlive the gap between closing and opening. The guard would tie "may I cache a region" to "is a reader open", which is a rule nothing else in the slice has — and it did break five fixtures that legitimately exercise regions without a loaded reader. Happy to revisit if you think the invariant is worth the coupling.Verification
scripts/verify.shgreen (== verify OK (all) ==) — backend 1,503 passed / 5 skipped, app 1,530 tests,flutter analyzeclean.🔮 fufu~ Jibril reviewed your code!
Round four, and you brought a generalization when I asked for a pin~ I said "two lines: a test with a non-
Exceptiontype" — you ripped out thestartsWith('Exception: ')special case entirely and replaced it with an anchored type-name grammar.^(?=[A-Z])[A-Za-z0-9_]*(Exception|Error):. Oh? OH. That is the right shape:ApiException's composed messages (with theirhost:portcolons!) pass through untouched,FormatExceptionout of a malformed payload gets trimmed, and a server's RFC 7807 detail that quotes an exception mid-sentence keeps its words. I ran the arithmetic on your tests myself, you know~ ♡Verdict: ✅ Looks good to me~
The round-3 blocker is closed, pinned three ways, and every pin is directional — I mutation-probed them:
startsWithimplementation → exactlya subclass gets its type name trimmed toogoes red.^anchor → exactlya message that quotes an exception keeps the quotegoes red.the caption shows words, not a Dart class namegoes red. That inline comment about the capital being asserted, not consumed isn't decoration — break it and a bareException:stops matching. Load-bearing comments, my favorite kind~💡 Little ideas (non-blocking)~
epics.dart:746— the doc comment's example list saysStateError:is "a leading Dart exception type name, astoString()writes it" — but realStateError.toString()writes"Bad state: …", not"StateError: …"(I probed it mechanically:KEEP | Bad state: the date is not a date). And doubly moot becauseStateErroris anError, so theon Exceptioncatch can never hand it to this helper anyway. Swap the example forRangeError:, which does stringify with its class name — my probe saysRangeError: Index out of range: …strips cleanly. The code itself is fail-safe in this direction (unrecognized prefixes pass through as-is), so it's purely the example that's fiction~✅ What I liked~
Exception-vs-subclass distinction stop mattering. Future throw-sites inherit correct captions for free._AlreadyReadableis a contract mirror, not a stub. It reproducesApiException's actual shape —implements Exception,toString()returns the message — so the passthrough test exercises the real production arm's behavior, not a lookalike. And the fixture's doc comment says exactly that._loadReaderEpic's rawe.toString()is exactly how this repo defers: the item names the asymmetry, the practical blast radius, and why it's out of scope for phase 1's last PR.Phase 1's last code story, closed with a grammar~ I'm thrilled ♡
Automated review by Jibril · 2026-09-21
CI/CD: pending for head
9685013(build ✓ 29s; CI/test + Flutter analyze-and-test queued) · Local checks: analyze clean · full app suite 1530/1530 (+3 = exactly the new tests) · mutation probes 3/3 red-on-exactly-own-testIncrement verified:
72aa427→9685013, +102/-8, 3 files (epics.dart, region_overlay_epics_test.dart, BACKLOG.md). Clone /workspace/dm-pr192/repo pristine at9685013(post-probe md5 match, git status clean).