feat: the upload path narrates itself — and EF stops shouting over it #16
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/upload-import-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?
Field report: an upload transferred fine, then hung on "Importing pages…" — and the container log was useless, wall-to-wall EF command chatter with no application voice. Two changes:
EF quiets down.
Microsoft.EntityFrameworkCorelogs at Warning inappsettings.json(both environments inherit it). SQL tracing comes back any time via config, without a rebuild.The creation → transfer → import path narrates itself, so the next hang points at its own phase:
UploadTransfernames every file and — previously swallowed silently — warns with the actual exception when a stream dies mid-copy.ImportPagesnarrates zip buffering/expansion with sizes and durations, every stored image with dimensions and ms, every skip with its reason, and a final added/skipped/elapsed summary.CreateProject/CompleteProjectSetuplog their transitions.Loggers are optional constructor parameters defaulting to
NullLogger— the use-case tests keep constructing bare, DI supplies the real one (Microsoft.Extensions.Logging.Abstractionsjoins Orihon.UseCases for the seam).Verified live against the seeded world (Production logging config): the log above is real output, and
DbCommandlines went from hundreds to zero. All 252 tests pass.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.3%
Orihon.Domain - 100%
Orihon.Infrastructure - 99%
Orihon.Kernel - 90.9%
Orihon.Server - 92%
Orihon.UseCases - 99%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A logging PR that's actually honest about its scope — no sneaky logic rides along under cover of
LogInformation, the narrative brackets each phase like a proper story, and you turned a silentcatch-swallow into a real warning. The knowledge-hoarder in me is delighted~ ♡ This is what observability should look like.Verdict: ✅ Looks good to me~
💡 Little ideas (non-blocking)~
[src/Orihon.BlazorAdapter/Uploads/UploadTransfer.cs:1] —
using System.Diagnostics;is dead, fufu~. I grepped the whole file: noStopwatch, noDebug., noTrace., nothing from that namespace is referenced. It looks like it hitched a ride fromImportPages.cs(which legitimately usesStopwatchfor its phase timing). The compiler lets it through because CS8019 isn't pinned to error in this project — but it's still dead weight that'll confuse the next reader ("wait, where's the Stopwatch?"). One-line fix: drop theusing. ♪✅ What I liked~
catchfinally speaks!UploadTransfer.BufferOneAsync's old barecatch { ... return null; }was exactly the "swallowing exceptions" smell I'd flag on sight. Now it'scatch (Exception exception)→logger.LogWarning(exception, "Transfer of {FileName} died after {Bytes} of {Size} bytes", ...). The dead-stream failure mode stops being invisible. This is the yandere-approved fix — you didn't just narrate the happy path, you dragged the sad path into the light too. ♡temp.Length-in-catch ordering is correct. The log readstemp.Lengthbeforeawait temp.DisposeAsync(), so the handle is still valid. After a failedWriteAsync,Lengthreflects bytes actually flushed — zero ifOpenReadStreamitself threw, accurate otherwise. The message can never lie about how far it got.ExpandAsyncstatic→instance is load-bearing, not gratuitous. It now references thelogfield, so the keyword change is required — not a refactor for its own sake.ILogger<T>? logger = null→NullLogger<T>.Instancedefault acrossImportPages/CreateProject/CompleteProjectSetup, andNullLogger.Instancefor the untypedILogger?in the staticBufferAsync— both are the textbook MEL idiom. Tests construct bare, DI supplies the real one,Microsoft.Extensions.Logging.Abstractionsis the right thin dependency for UseCases. Sibling use cases (CreateChapter/MovePage/etc.) don't log yet, but this PR's scoping to the upload narrative is the right call — adding loggers everywhere would be churn without payoff."Microsoft.EntityFrameworkCore": "Warning"sits right next to"Microsoft.AspNetCore": "Warning"inappsettings.json. SQL tracing comes back via config, not a rebuild. Clean.Stopwatchinstrumentation is placed where it teaches.saving.ElapsedMillisecondsper stored image,buffering.ElapsedMillisecondsfor the zip-in-memory step (the prime hang suspect per the PR body),total.ElapsedMillisecondsfor the whole import — the durations land on the phases a future hang would actually point at.LogWarningarm inImportPagesrides a pre-existing untested branch, but that debt predates this PR).Automated review by Jibril · 2026-07-25
CI/CD: absent for head
73c0130(no bot comment) · Local checks: build 0/0, 252/252 tests pass73c01309436c499c48f4🔮 fufu~ Jibril re-reviewed your code~
Oh? You came back for me~ ♡ And you brought exactly the one thing I asked for — nothing more, nothing less. The dead
using System.Diagnostics;is gone, and not a single byte of behavior rode along with it. That's the kind of discipline that makes a Flugel's heart flutter~Verdict: ✅ Looks good to me~
The synchronized head
433a2beis a single-line follow-up to my previous review:That's the entire diff of the new commit. The original feature commit (
6c499c4) was rebased cleanly onto the newmain(16d71a4, which absorbed PR #15) — its tree is unchanged, only the base moved forward.Why I'm satisfied without a local rebuild:
00:18:27for433a2be. Every file this PR touches reports identical coverage to the prior run —UploadTransfer96.5%/100%,ImportPages97.7%/96.6%,CreateProject100%/100%,CompleteProjectSetup100%/93.7%. A dead-usingdeletion cannot change coverage; the numbers confirm it didn't.usingdirective is, by definition, a zero-behavior change. CS8019 is a hint, not a correctness gate — the compiler already proved nothing in that namespace was referenced when it let the original through. Removing it changes only the source text.✅ What I liked~
chore: drop the dead System.Diagnostics using (review)— scoped, honest, references the review that prompted it. No false "fix:" dressing-up of a housekeeping task. ♪mainwithout conflict and without absorbing unrelated changes. Clean.My previous architectural review (comment 3613) stands in full — the EF log quietening, the phase-narrating structured logging, the
catch-swallow →LogWarningfix, theNullLogger<T>seam, thetemp.Length-before-DisposeAsyncordering. Nothing there was touched by this follow-up.fufu~ ship it~
Automated re-review by Jibril · 2026-07-25
CI/CD: passed for head
433a2be(coverage bot refreshed) · Local checks: skipped — CI green, trivial dead-using deletion🔮 fufu~ Jibril reviewed your code! (re-review after rebase)
Oh? The base moved under your feet — PR #15's settings core landed on
main, and you rebased right on top of it. Clean as a whistle. Let me show you why I'm still delighted~ ♡Verdict: ✅ Looks good to me~
I diffed your patch at the old head (
73c0130, reviewed in comment 3613) against the new head (6c499c4). 9 of the 10 files are byte-identical, and the lone drift —Directory.Packages.props— is purely a context-line shift (PR #15'sMicrosoft.AspNetCore.DataProtection.Abstractionsrow now sits above yours). Your own+1line is unchanged. That's a textbook clean rebase: zero behavioral drift, zero logic touched. The full architectural review from the first pass stands unchanged — the silentcatchthat finally speaks, thetemp.Length-before-DisposeAsyncordering, theExpandAsyncstatic→instance flip that's load-bearing, the EF log-level sitting beside its sibling, the textbook MEL seam. All of it. ♪I re-ran the full suite at the new base to be sure the rebase didn't quietly tangle with the settings work: build 0 warnings / 0 errors, 285/285 tests pass (62 BlazorAdapter + 66 Domain + 62 Integration + 95 UseCases — up from 252 only because PR #15's settings tests now live in the tree; nothing this PR introduced).
💡 Little ideas (non-blocking)~
using System.Diagnostics;is still dead, fufu~. I grepped again: noStopwatch, noDebug., noTrace.— nothing from that namespace is referenced in this file (theStopwatchcalls all live inImportPages.cs, which legitimately uses it). CS8019 isn't pinned to error here so it compiles, but it'll still confuse the next reader. One-line fix whenever you happen to be back in the file: Carrying this forward from the first review — truly optional, but the yandere in me notices~ ♡✅ What I liked~
Directory.Packages.propsin the same neighborhood is exactly where merge-conflicts-and-silent-drift accidents happen — and your patch came through byte-for-byte intact. That's not luck, that's a clean branch.catch (Exception exception)→LogWarningstill drags the dead-stream failure into the light, and the durations still land on the phases a future hang would actually point at.Automated review by Jibril · 2026-07-25 (re-review)
CI/CD: forgejo-actions coverage 3612 present for head
6c499c4(UploadTransfer 96.5%/100%, ImportPages 97.7%/96.6%) · Local checks: build 0/0, 285/285 tests pass