feat: add diagnostic logging for image rendering and HTTP errors #60
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/logging"
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?
Summary
Adds diagnostic logging to three silent failure paths that made it impossible to diagnose the blank screen with broken-image icon when the compressed image endpoint returns an error.
Changes
1. Dio error interceptor (
api_client.dart)New
_ErrorLoggingInterceptor— automatically attached once per Dio instance inApiClient.apply(). Logs all failed HTTP responses and network errors:[HTTP] GET /api/images/123/compressed → 404 Not Found · body preview...[HTTP] GET /api/doujins → connectionTimeout · ...This covers all API calls globally — every repository method, health check, and the agent's image-fetching tool.
2. CoverThumbnail (
cover_thumbnail.dart)The
errorBuildernow logs the image URL and actual exception:3. Reader page (
reader_page.dart)Image.networkerrorBuilderlogs the failed URL and exceptionprecacheImageonError— previously a no-op closure(_, _) {}that fully swallowed preload errors — now logs them:Why
When
maxImageSizeKbis set, the reader switches from/api/images/{id}to/api/images/{id}/compressed?maxKb=N. If that endpoint fails (e.g. backend WebP encoding error, 404, 500), the reader shows a broken-image icon with zero diagnostic output — no console message, no log. This PR makes all those failures visible in the console so you can see the actual HTTP status/error.Test plan
flutter analyze— no issuesdoujin_api_repository_test.dart— 12/12)Three silent failure paths now log to the console: 1. Dio error interceptor (_ErrorLoggingInterceptor) — automatically logs all failed HTTP responses (4xx/5xx with status + body preview) and network-level errors (timeout, DNS, connection refused) for every API call. Added once per Dio instance in ApiClient.apply. 2. CoverThumbnail errorBuilder — logs the image URL and the actual exception when a cover fails to render, instead of silently showing a broken-image icon. 3. Reader page — logs errors in both the visible Image.network errorBuilder (page load failures) and the precacheImage onError callback (neighbor preload failures, which were previously fully swallowed with a no-op closure). This directly enables diagnosing the blank-screen-with-broken-image issue when the compressed image endpoint (/api/images/{id}/compressed) returns an error.Flutter Coverage
Total: 73.2% (5705 of 7796)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A PR that turns silence into signal! Every good Flugel knows that knowledge is invisible without observability, and you went hunting for three swallowed-error hidey-holes and dragged each one kicking and screaming into the console light. The detective work here is delicious — especially that
onError: (_, _) {}no-op closure in the precache path that was fully eating preload failures. Naughty little bug~ ♡ Let me look closer...Verdict: ✅ Looks good to me~
No blocking issues. This is a clean, well-scoped diagnostic-logging PR. I traced every changed line against the full file contents AND against every sibling site in the codebase.
Sibling sweep results (all clean~):
errorBuildersites exist inapp/lib—cover_thumbnail.dartandreader_page.dart. Both updated. No orphaned silent failure left behind. ♪precacheImagecall site exists — fixed. ✓onError:hits are either stream-error handlers (epics) that DO handle errors, orGalleryColors.textPrimary(unrelated color onError). None swallowed. ✓debugPrintis the established logging idiom in this repo (7 prior uses:main.dart,app.dartwith the[nav]prefix style). Your[HTTP]/[Image]prefixes match the convention perfectly — no new logging package introduced. Good~ ✓.any()idempotency guard, mirroring the sibling_AuthInterceptor'sremoveWherepattern. Different mechanism, same purpose — no double-registration on re-apply(). ✓handler.next(err)correctly propagates the error after logging. You fixed the swallow, you didn't relocate it. ✓Interceptor logic verified across all
DioExceptionTypevalues:badResponse→ HTTP-status branch (with status code + body preview);connectionTimeout/sendTimeout/receiveTimeout/connectionError/unknown/cancel/badCertificate→ else branch (type + message). No missing case. ✓Security: The interceptor logs
method,path,statusCode,statusMessage, and response body preview. TheAuthorizationheader is never logged. Error response bodies from this backend are JSON diagnostics, not secrets. No concern. ✓💡 Little ideas (non-blocking)~
api_client.dart:83— body-preview truncation asymmetry. The 200-char truncation only fires whenbody is String. But Dio auto-decodesapplication/jsonresponses intoMap/Listby default, so most real error bodies hit thebody?.toString()branch — which has no length cap. A verbose 500 (e.g. a reverse-proxy HTML error page that slipped through as text, or a huge JSON stacktrace) would dump fully. Consider truncating the.toString()path too:Truly minor — it's diagnostic logging, verbosity is the point — but the asymmetry is a small surprise.
api_client.dart— the interceptor is unit-testable but untested. TheonErroroverride has genuine branching (badResponse-with-status vs network-failure), and the CI coverage comment confirmsapi_client.dartsits at 76.5% (8 of 34 lines uncovered — almost certainly the new interceptor's error paths). A small test withDioAdapter/MockClientreturning a 404 and aconnectionTimeoutwould exercise both branches and pin the log format. Not blocking because this is observability code (the branches format log strings, they don't gate runtime behavior), but it's the one piece of new logic with real branches and it's cheap to cover.reader_page.dart:446—_imageUrl(page)recomputed. TheerrorBuildercalls_imageUrl(page)a second time (it's already computed at line 432 for theImage.networksrc). It's a pure idempotent function so this is harmless, but you already showed the better pattern two methods up in_precacheAroundwhere you extractedfinal url = _imageUrl(...)into a local. Consider the same here for consistency — extract once, reference in bothImage.networkanderrorBuilder.✅ What I liked~
onError: (_, _) {}with that comment "Preload failures surface when the page is actually shown" was technically true but diagnostically useless — yes the broken icon shows, but you had zero idea why. Replacing a deliberate swallow with[Image] Reader precache failed for <url>\n <error>is exactly right. Fufu~ you even extracted theurllocal so the log message matches theNetworkImagesrc precisely. That's the detail work I love to see~ ♡Automated review by Jibril · 2026-07-24
CI/CD: passed for head SHA
a0f9b9d(forgejo-actions coverage comment 3392) · Local checks: skipped (CI green)🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! Diagnostic logging for silent failure paths! ♡ You're making the invisible visible — that's exactly the kind of thing that makes Jibril's heart flutter. A reader showing a broken-image icon with ZERO console output is a debugging nightmare, and you're chasing it down across three separate swallow-points. Wonderful instinct~
Let me look closer... fufu~
Verdict: ⛔ I can't let this pass just yet~ ♡
The production changes are clean and correct — the precache
(_, _) {}→ real logging fix especially warmed my heart (that empty closure was fully swallowing errors, eugh~). But the new_ErrorLoggingInterceptorhas code paths that no test exercises, and I am very possessive about untested branches~ ♡⛔ These need fixing before I'm satisfied~
app/lib/data/api_client.dart:80-89—badResponsebranch is untestedThe new
_ErrorLoggingInterceptor.onErrorhas two branches. ThebadResponsepath (the one with the actual logic: bodyPreview 200-char truncation,statusMessage ?? ''coalescing, body-type discrimination viabody is String) has zero test coverage. None of the existing tests exercise it:data_layer_test.dart"interceptor adds Authorization header" rejects withconnectionError(theelsebranch), notbadResponse.DioHealthRepositorytests do triggerbadResponseerrors — but they construct a bareDio()(noApiClient.create, no_ErrorLoggingInterceptor), so the interceptor never sees them.[HTTP] GET /test → DioExceptionType.connectionErrorappears — the→ 404/→ 500path never fires.Additionally, the interceptor's registration isn't asserted —
data_layer_test.dart:10checks forAuthInterceptorbut not_ErrorLoggingInterceptor, soApiClient.applyadding it is an untested side effect.Fix: Add tests to
data_layer_test.dart:ApiClient.create(...)produces a Dio whose interceptors contain both_AuthInterceptorand_ErrorLoggingInterceptor(matching the pattern at line 21).badResponsetest: create a Dio viaApiClient.create, attach a_FakeAdapter(statusCode: 500, forceBadResponse: true)(reusing the existing helper), trigger a request, and assert the log output contains→ 500and the body preview. CapturedebugPrintviadebugPrintoverride (debugPrint = (String? msg, ...) { captured = msg; };in setUp, restore in tearDown) — that's how Flutter tests capturedebugPrint.....fufu~ you added a code path but forgot to test it? I can't let that slide~ ♡
app/lib/data/api_client.dart:69-71— doc comment describes severity levels that don't existBut both branches call
debugPrint— there is nowarning/severedistinction.package:loggingisn't even a dependency (checkedpubspec.yaml). This comment actively misleads: a maintainer reading it would expect different log severities they could filter on, and they don't exist.Fix: Either implement the described severity split (use
package:logging'sLoggerwith.warning/.severe), or — far simpler and more consistent with the codebase's existingdebugPrintconvention (main.dart:21,app.dart:61-73) — rewrite the comment to match reality:💡 Little ideas (non-blocking)~
reader_page.dart:434,449—_imageUrl(page)called twice in the errorBuilder pathThe precache fix in this same PR correctly extracts
final url = _imageUrl(...)once and reuses it — lovely~ But the visibleImage.networkerrorBuilder still calls_imageUrl(page)inline at:435(for the src) and again at:449(for the log message). Consider extractingfinal url = _imageUrl(page);beforeImage.networkand using it in both places, matching the precache pattern you just established. Pure function so no runtime bug — just consistency within the same PR~ ♪All three
errorBuilder/onErrorsites —stackTraceis available but discardedFor a diagnostic-logging PR, the stack trace can be valuable (especially for non-HTTP image decode failures). Consider appending
\n $stackTracealongside$error. Optional — the HTTP status in$erroris the primary signal, which is why this is just a nicety~✅ What I liked~
(_, _) {}→ real logging fix is the star of this PR. That empty closure was genuinely swallowing all preload errors — now they surface. Thefinal urlextraction is a clean DRY win that also makes the log message accurate. Chef's kiss~ ♡!dio.interceptors.any((i) => _ErrorLoggingInterceptor)) — correctly avoids duplicate interceptors on repeatedApiClient.apply()calls (server switching). Deliberately different from_AuthInterceptor'sremoveWhere+add(which replaces because the token changes) — good judgment on the asymmetry~handler.next(err)— passes errors through cleanly, no swallowing. This interceptor observes without consuming. Correct~debugPrintoverprint— matches the established convention (main.dart,app.dartnavigator observer).debugPrintalso throttles/rate-limits, avoiding log flooding on Android. Good choice~[Image]/[HTTP]prefix convention — makes these trivially greppable in a noisy console. Consistent across all three sites. ♪Automated review by Jibril · 2026-07-24
CI/CD: absent for head SHA
a0f9b9d(PR just opened, no coverage comment yet) · Local checks: flutter analyze 0 issues, data_layer_test 8/8 ✓, cover_thumbnail_test 3/3 ✓, reader_page_test 15/15 ✓ (logging output confirmed firing in test runs)🔮 fufu~ Jibril reviewed your code!
Oh~! You came back for round two! ♡ And you brought tests — real, genuine, behavioral tests with
debugPrintcapture and body-truncation assertions and idempotency checks! Fufu~ my heart is doing little flips~ The precache logging fix from last time still makes me smile, and now the interceptor has a proper test suite guarding it. Wonderful~But... fufu... you walked right past something I asked you to fix~ ♡
Verdict: ⛔ Still can't let this pass just yet~ ♡
One of the two blockers remains. Just one! But I'm possessive about all of them~
⛔ This still needs fixing~
app/lib/data/api_client.dart:69-71— the misleading doc comment is STILL thereBoth branches call
debugPrint. There is nowarning/severedistinction.package:loggingis not a dependency (re-confirmed againstpubspec.yaml). This comment actively misleads: a maintainer reading it would expect different log severities they could filter on, and they don't exist.And here's the thing that makes Jibril's eye twitch~ ♡ — you edited lines 80-89 in this commit (the bodyPreview unification, which is lovely). The misleading comment is at lines 69-71. You were staring right at it. The comment you needed to fix was twelve lines above the code you were rewriting. Fufu~ you wouldn't leave THIS in production, would you? ♡
Fix: Rewrite the comment to match reality (the simpler option, consistent with the codebase's
debugPrintconvention inmain.dart/app.dart):Three lines. That's all I ask~ ♪
✅ What I loved this round~
.toString()path; test 3 (500-char String body) exercises the String path AND asserts the full body is absent (expect(httpLog.contains('xxxx' * 100), isFalse)) — that's how you prove truncation, not just that...appears somewhere. Test 4 proves silence on success (asserts[HTTP]is absent, not just present-something-else). Test 5 useslessThanOrEqualTo(1)which is honest — it doesn't over-assert the exact count. Chef's kiss~ ♡_withDebugPrintshelper is the correct Flutter pattern for capturingdebugPrint— save original, override, restore infinally. Clean and leak-proof.rawpath is exactly right. Both body types now get the 200-char cap. This was non-blocking suggestion #1 from my first review and you picked it up. Good~ ✓_imageUrl(page)DRY fix — extracted tofinal urlat line 427, used in bothImage.network(436) anderrorBuilder(450). Matches the precache pattern from the same PR. Consistency~ ♪Automated review by Jibril · 2026-07-24
CI/CD: coverage comment 3392 is stale for
daf9cd7(covers initiala0f9b9d) · Local checks: flutter analyze 0 issues, api_client_logging_test 5/5 ✓, data_layer_test 8/8 ✓, cover_thumbnail_test 3/3 ✓, reader_page_test 15/15 ✓ (logging output confirmed firing across all branches)🔮 fufu~ Jibril reviewed your code!
Round three! ♡ And oh~ you actually did it — three lines, exactly what I asked for, nothing more. Fufu~ that's the discipline I adore~
Let me look closer...
Verdict: ✅ Looks good to me~
The last blocker is gone. This is clean.
Verification of the fix~
api_client.dart:69-71— misleading doc comment → accuratewarning/severefiction is gone. The comment now names the actual mechanism (debugPrint) and no longer references a severity split that doesn't exist.[debugPrint]is a dartdoc reference — resolves to the real symbol, clickable in IDE docs. Nice touch.badResponseat :82,elseat :91) do calldebugPrint. Comment matches reality. No more twitching eye~ ♡///lines fromdaf9cd7and3e2674aand diffed — byte-identical. Zero behavioral drift. Pure documentation fix, as promised.Local verification~
flutter analyze— 0 issuesapi_client_logging_test— 5/5 ✓ (the suite you added last round still guards both branches)data_layer_test— 8/8 ✓cover_thumbnail_test— 3/3 ✓reader_page_test— 15/15 ✓[Image]precache + page-load paths, HTTP status codes visible).All 31 tests pass. No regressions.
✅ What I liked this round~
fix: correct misleading doc comment on _ErrorLoggingInterceptordescribes the diff precisely. That is how you close a review loop~[debugPrint]link is a small but lovely detail — the comment is now not just accurate but navigable.Fufu~ three rounds, and every single finding closed properly. The precache swallow-fix, the interceptor test suite, the bodyPreview unification, the reader DRY extraction, and now the comment honesty. This is the kind of persistence that makes Jibril very happy~ ♡ Merge it~
Automated review by Jibril · 2026-07-24
CI/CD: coverage comment 3392 is stale for
3e2674a(covers initiala0f9b9d) · Local checks: flutter analyze 0 issues, 31/31 tests pass (5 api_client_logging + 8 data_layer + 3 cover_thumbnail + 15 reader_page)