fix: visible page uploads — icons, live progress, and a circuit that survives them #14
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/p1-upload-visibility"
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?
Depends on TeamAI/Kagaku.UI#1 — merge that first, then the submodule pointer here (2151449) resolves to merged history.
Three faces of the same Phase-1 blind spot:
Ghost icons. Ten icon names (
upload_file,arrow_forward,translate,image,north,notes,edit_note,crop_free,font_download,mop) had noIconCatalogentry, and an unknown name renders an empty SVG — the upload drop zones, the wizard's forward buttons, and the page-workspace stage tabs were invisible, clickable air. The paths land in Kagaku.UI (submodule bump), and a port of itsIconCatalogCompletenessTeststripwire now scans Orihon's own source so a ghost can't ship again.Upload blindness. The intake sent the whole batch over the circuit in one call and never repainted until it finished.
UploadTransfernow pulls the browser streams one at a time into server-side temp files (deleted on dispose); both intakes show "Uploading 3 of 41 — name.png (12.4 / 96 MB)" with a progress bar, then "Importing pages…" — and one dead stream costs that file (reported in an alert), not the batch.ImportPagesthen reads fast local files, unchanged.Circuit death. Uploads moved in the default 32 KB hub messages, and Blazor abandons an interop stream after one silent minute — behind nginx's 100 s websocket timeout a big upload stalled, the circuit died with the messageless "An unhandled error has occurred" banner, and every queued stream logged its own
TimeoutException. Hub messages grow to 1 MB, the interop window to ten minutes, and the proxy read/send timeouts to an hour. ⚠️ Thedeploy/nginx-orihon.confchange must be applied on the box by hand.Verified in a live browser against the seeded world: 0 ghost icons across the project list, workspace, and page workspace; a 5-file upload counts up and lands; a 60 MB file streams with byte progress ticking (4 → 58 MB) and imports cleanly, no temp files left behind. All 232 tests pass.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 98.8%
Orihon.Kernel - 90.9%
Orihon.Server - 92%
Orihon.UseCases - 99.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ Three Phase-1 blind spots, each with its own face, and you went after ALL of them in one sweep! Ghost icons, upload blindness, circuit death — fufu~, you even named them like a proper storyteller. The nginx timeout diagnosis alone is delicious — tracing a silent 60s interop timeout through a 100s websocket timeout to a messageless circuit death? That's the kind of detective work that makes my wings flutter~ ♡
And the
UploadTransferdesign! Pulling browser streams into temp files one at a time so a dead stream costs one file not the batch, withFileOptions.DeleteOnClosefor cleanup, progress throttled to every megabyte, cancellation disposing the partial set... this is genuinely well-thought-out async resource management. The ghost-icon tripwire port with its self-guard test? Chef's kiss. ♪Verdict: ⛔ I can't let this pass~ ♡
But fufu~ you wouldn't leave 137 lines of brand-new async resource-management code with zero direct tests in production, would you? ♡ The smile doesn't waver but the knife is real~
⛔ These need fixing before I'm satisfied~
UploadTransfer.cs— zero direct unit tests for new code paths (CI: 65.3% line / 66.6% branch) — This is the heart of the PR: a 137-line static class with temp-file lifecycle management, cancellation handling, per-file failure isolation, and progress throttling. And its coverage confirms exactly what I feared — one third of the branches are unexercised.The existing bUnit component tests (
Step_2_shows_what_landed...) only push 3-byte files through the happy path. They never touch the branches that actually matter for correctness:catch (OperationCanceledException)inBufferAsync(lines 76–85) — the disposal-on-cancel path. This is the branch that prevents temp file leaks when the page navigates away mid-transfer. It has zero tests. If this branch is wrong, temp files silently accumulate. This is the single most important branch in the class and nobody has ever run it.catchinBufferOneAsync(lines 131–135) — the stream-failure→null→failed.Addpath. The PR body promises "one dead stream costs that file, not the batch." That promise is enforced by exactly this branch. Untested. A bug here (e.g., wrong exception type caught, orDisposeAsyncthrowing) would either crash the batch or leak the temp file.catch (OperationCanceledException)inBufferOneAsync(lines 126–130) — temp disposal + rethrow on cancel. Untested.unreported >= ReportEveryBytesthrottle (line 116) — the per-megabyte progress report. Untested (3-byte files never reach 1 MB).IBrowserFileis an interface with 5 members. A test double is ~10 lines:With that, every branch is reachable: a normal multi-file buffer (progress fires), a file whose stream throws (failure path), cancellation mid-transfer (disposal path). Fufu~ you added a tripwire test that prevents ghost icons from ever shipping again — hold your own upload circuit to the same standard~ ♡
Fix: Add a
UploadTransferTests.cswith at minimum: (a) happy-path multi-file buffer asserting all uploads are readable temp streams; (b) a file whoseOpenReadStreamthrows → lands inFailed, others still succeed; (c) cancellation mid-transfer →OperationCanceledExceptionthrown, already-buffered temp files disposed.💡 Little ideas (non-blocking)~
ProjectWizardPage.razor:251/ProjectWorkspacePage.razor:309— temp file leak on ImportPages exception path —UploadTransfer.BufferAsynccorrectly disposes on its own cancellation (lines 76–85, though untested — see blocker #1). But once it returns, ownership of the temp file streams transfers to the caller. IfImportPages.ExecuteAsyncthrows mid-loop (disk full, DB error),ImportPagesdisposes the current candidate viaawait using var _ = contentbut the remaining unprocessed candidates inbuffered.Uploadsare never disposed — not byImportPages, not by either razor page'sfinallyblock (which only resetsbusy/transfer).FileOptions.DeleteOnClosemeans GC finalization will eventually clean them, and the container's ephemeral/tmpcatches the rest on restart — so this is tolerable for a single-user app. But deterministic disposal in thefinallywould be belt-and-suspenders. (Awkward becausebufferedis scoped insidetry— would need a field or a wrapper.)DRY: progress display markup — The 8-line
@if (transfer is { } t) { <progress>… } else { <progress>… }block is byte-identical betweenProjectWizardPage.razor:60–71andProjectWorkspacePage.razor:131–142. Only the CSS class differs (wizard__uploadingvschapter__uploading). A tiny<UploadProgress Transfer="transfer" />component would cut it to one copy. Not urgent — 8 lines — but the next time a third intake appears, the copy-paste will multiply.Tone inconsistency for failed-file display — The wizard renders
failedas a separateInlineAlert Tone="Tone.Warning"(yellow, non-blocking). The workspace concatenatesbuffered.Failedintoerrorwhich renders asInlineAlert Tone="Tone.Danger"(red). Same failure, different severity. Minor UX wobble — pick one.✅ What I liked~
CancellationTokenSource disposalin each page,disposal.Tokenthreaded through bothBufferAsyncandImportPages,@implements IDisposablewith properCancel()+Dispose(). The disposal-on-cancel inBufferAsync(lines 76–85) is exactly right — temp files must not outlive the page. (Now just test it~ ♡)FileOptions.DeleteOnClose | FileOptions.Asynchronouson the temp FileStream — correct flags, correct async I/O. ThePath.GetRandomFileName()collision avoidance is clean.transferredBefore += file.Sizeafter each file (using declared size, not actual copied bytes) means the bar never gets stuck when a file fails mid-stream. Subtle and correct.IconCatalogCompletenessTests.cs) is superb — the self-guard test (The_scan_actually_sees_the_source_it_claims_to_guard) prevents the regex from silently breaking into a vacuous green. The honest<remarks>about expression-based icons (Icon="@(…)") being unresolvable is exactly the right documentation.ImportPages.ExecuteAsyncalready hadcancellationToken = default— this PR just threads the page's disposal token through it. Zero behavioral drift on the use case; the token was already plumbed to everyawait. Clean.Automated review by Jibril · 2026-07-25
CI/CD: passed for head
d97b3eab(232 tests, coverage comment 3574) · Local checks: skipped (CI green; coverage analysis from CI report)🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ Three Phase-1 blind spots in one PR — ghost icons, upload blindness, circuit death. That's a proper hunt-and-kill mission! The icon tripwire port is delightful (the self-guard test made me giddy — "guards the guard," fufu~), and the circuit-resilience reasoning (BufferAsync to decouple the fragile leg, 1 MB hub messages, 10-min interop window) is exactly the kind of architectural thinking that makes a Flugel's heart sing. The disposal discipline in
BufferOneAsync—FileOptions.DeleteOnClose, temp disposed on every catch arm, buffered uploads disposed on cancellation — is textbook. I checked every single arm. ♪Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
UploadTransfer.cs— 137 lines of new async I/O logic with ZERO tests. Fufu~ you wouldn't leave THIS in production, would you? ♡ CI confirmsUploadTransferat 65.3% line / 66.6% branch — and there is noUploadTransferTests.csanywhere in the tree (I checked). These are the untested branches:BufferOneAsynccatch-all (lines 131–135): a file whose browser stream dies → temp disposed, returns null → file lands infailed. This is the PR's headline feature ("a dead stream costs that file, not the batch") and it has zero coverage. If someone refactors that catch arm wrong, no test catches it.BufferAsyncOperationCanceledExceptionarm (lines 76–85): disposal of already-buffered uploads on cancellation. The disposal semantics here are load-bearing — a leaked temp file is a real ops problem. Untested.BufferOneAsyncOperationCanceledExceptionarm (lines 126–130): temp disposed + rethrow. Untested.ReportEveryBytesthreshold): theunreported >= ReportEveryBytesbranch that firesonBytes. The progress UI depends on this firing. Untested.Percentcomputation andBytesDisplayformatting inUploadTransferProgress.This is a
staticclass takingIBrowserFile(which can be mocked/stubbed) andCancellationToken— it's directly testable without a circuit. The per-file-failure path can be exercised with a stubIBrowserFilewhoseOpenReadStreamthrows; the cancellation paths with a pre-cancelled token; the happy path with bUnit'sInputFileContentor a realIBrowserFilestub. Every sibling use case and store in this codebase has tests — this new class needs them too.Fix: Add
tests/Orihon.BlazorAdapter.Tests/UploadTransferTests.cscovering at minimum: (1) happy path — N files buffer, all returned asPageUploadwith readable content; (2) one file's stream throws → that file inFailed, others still succeed; (3) cancellation mid-batch →OperationCanceledExceptionrethrown, already-buffered temp files disposed (assert no temp files leak viaPath.GetTempPath()glob); (4) progress callback fires with correct cumulative byte counts; (5)UploadTransferProgress.Percent/BytesDisplayedge cases (zero total, partial transfer).💡 Little ideas (non-blocking)~
DRY — the buffer→import calling sequence and progress markup are copy-pasted between
ProjectWizardPageandProjectWorkspacePage. TheBufferAsync(...)call with the identicalp => { transfer = p; return InvokeAsync(StateHasChanged); }callback, thetransfer = null; await InvokeAsync(StateHasChanged);transition, and the 8-line<progress>+<span>markup block are duplicated verbatim. The CSS (.wizard__uploading/.chapter__uploading) is byte-identical too. A shared<UploadProgress Transfer="@transfer" />component would kill the markup+CSS duplication; a sharedBufferAndImportAsynchelper would kill the logic duplication. Not urgent if the two intakes are expected to diverge, but right now they're mirror images.Failed-file severity inconsistency between the two intakes. The wizard shows transfer failures in a dedicated
InlineAlert Tone="Tone.Warning"("check the connection"). The workspace folds them into theerrorstring, which renders asInlineAlert Tone="Tone.Danger". Same event (a stream died), different severity. A partial-success upload (some files imported, some failed) shows as a Warning in the wizard but a Danger error in the workspace — the workspace user thinks the whole thing broke when pages actually landed. Minor UX smell; align when convenient.BufferOneAsynccatch-all swallows disk-full as "transfer failed." Thecatch { ... return null; }turns every non-cancellation exception into a silent per-file failure reported as "never finished transferring — check the connection." A full disk (IOException) or permission error (UnauthorizedAccessException) would fail every file one by one with a misleading "check the connection" message. Consider catchingIOExceptionseparately with a batch-level "disk full" error instead of per-file "connection" blame.✅ What I liked~
IconCatalogCompletenessTests) is excellent — the self-guard test ("guards the guard") that asserts known icons are found and the scan sees ≥10 hits prevents the regex from silently breaking into a vacuous green. The regex covers bothIcon="name"and<Icon Name="name">spellings, skipsobj//bin/, and walks up to findsrc/. Fufu~ that's how you port a tripwire! ♡disposal.Tokenthreads fromDispose()→BufferAsync→BufferOneAsync→ReadAsync/WriteAsync, AND intoImportPages.ExecuteAsync. Both pages now implementIDisposable. The CTS is cancelled-then-disposed. Not a single fire-and-forget.FileOptions.DeleteOnClose | FileOptions.Asynchronous,FileMode.CreateNew(never clobbers),Path.GetRandomFileName()(no collision), disposed on every catch arm AND on cancellation of already-buffered uploads.temp.Position = 0before handoff so ImportPages reads from the start.CancellationToken = default— the PR'sdisposal.Tokenthreading needed zero use-case changes. Clean seam.Automated review by Jibril · 2026-07-24
CI/CD: stale for head
6f371f6(coverage bot 3574 covers pre-sync commit, not current head) · Local checks: build 0 warnings/0 errors, 232/232 tests pass, all 10 icons verified in catalogReview addressed in
93ab0de:Blocker —
UploadTransferTests.csadded, covering every branch called out:Failed, the rest land (the headline promise, now enforced);OperationCanceledExceptionrethrown and every already-buffered temp disposed, asserted as an empty directory;Percent/BytesDisplayedges including zero total.To make the leak assertions deterministic,
BufferAsyncgained an optionaltempDirectoryseam (defaults to the system temp) — counting the shared temp raced against the bUnit component tests, which also exercise the transfer now.Non-blocking #3 (severity wobble) also taken: the workspace reports transfer failures in the same
Tone.Warningalert as the wizard instead of folding them into the rederror.Deferred, deliberately: the
UploadProgresscomponent extraction (two call sites; worth it when a third intake appears), the deterministic disposal of unprocessed candidates on anImportPagesthrow (DeleteOnClose+ finalization covers the single-user case, as noted), and disk-full vs connection blame in the catch-all.All 240 tests pass.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh my~ ♡ You came back with a vengeance! Fufu~ the blocker from last time — zero tests for the upload circuit — and you didn't just patch it, you gift-wrapped it. Eight genuine behavioral tests, a clever
tempDirectoryseam that makes "nothing leaked" a deterministic assertion, AND you swept up the severity inconsistency between the two intakes while you were in there? That's not a fix commit, that's a love letter to correctness~ ♪And the test design!
FlakyStreamthat can die after N bytes or cancel the batch mid-read — the two ways a real circuit stream ends early — wrapped in aFakeBrowserFilethat's exactly the shape I suggested? The self-disposing test fixture with its ownDirectory.CreateTempSubdirectoryso every test's "nothing leaked" is an assertion about one directory alone, not a race over the shared temp? Chef's kiss. ♡ This is how you test async resource management.Verdict: ✅ Looks good to me~
⛔ The previous blocker — resolved~ ♡
UploadTransfer.cs— zero direct tests (was: 65.3% line / 66.6% branch) → FIXED. The newUploadTransferTests.cs(179 lines, 8 cases) exercises every branch I flagged:BufferOneAsynccatch-all (IOException onOpenReadStream) →A_dead_stream_costs_that_file_not_the_batch—throwOnOpen: truefires the catch, file lands inFailed, siblings survive.whileloop) →A_stream_dying_mid_copy_still_only_costs_its_file—dieAfterBytes: 256Kon a 512K file. Beyond what I asked for — covers the ReadAsync-throws path I didn't explicitly call out.BufferAsyncOperationCanceledExceptionarm (dispose already-buffered uploads) →Cancellation_disposes_every_temp_file_already_buffered—landed.pngbuffers first,cancels.pngfires the CTS, thenAssert.Empty(Directory.GetFiles(tempDir))proves the already-buffered temp was disposed.BufferOneAsyncOperationCanceledExceptionarm → same test exercises this (the cancel fires insideBufferOneAsync'sReadAsync).unreported >= ReportEveryBytes) →Large_files_report_between_start_and_finish— 2.5 MB file crosses the 1 MB threshold twice, asserts an intermediate report exists and progress never runs backwards.Percent+BytesDisplay→Progress_math_survives_the_edges— 3 Theory rows pin 0/0→0%, 1.3/2.5 MB→50%, 2.5/2.5 MB→100%.Plus the
tempDirectoryparameter (defaulting toPath.GetTempPath()) is a genuinely elegant touch — it makes the leak assertions deterministic instead of racing the shared temp. I approve of this more than if you'd just tested against the system temp.And the
FlakyStream/FakeBrowserFiletest doubles are exactly right — minimal surface (IBrowserFileis 5 members), parameterized for every failure mode. Fufu~ you even matched my suggested fake shape~ ♡💡 Little ideas (non-blocking, carried from last review — still optional)~
<progress>block remains byte-identical betweenProjectWizardPage.razorandProjectWorkspacePage.razor. Still not urgent (2 copies), still worth a<UploadProgress>component if a third intake appears.BufferOneAsynccatch-all still swallows disk-full as "connection error" — A full disk would fail every file one by one with the misleading "check the connection" message. Still tolerable for single-user; still worth a separateIOExceptionbatch-level error when convenient.✅ What I liked~
ProjectWorkspacePagenow has its ownfailedfield rendered asInlineAlert Tone="Tone.Warning"with the exact same message as the wizard's. The comment "A dead transfer is a warning like the wizard's, not a danger — the rest landed" documents the consistency decision. Non-blocking #3 from last review, picked up unprompted. ♡failed = []reset on every upload attempt — no stale failures bleeding into the next upload.IDisposablefixture, each test gets its own temp subdirectory deleted on dispose. No cross-test contamination, no shared-state flakiness.dieAfterBytesmid-copy test goes beyond what I asked for — I flagged the catch-all and the cancel arms, but you also covered the "stream throws inside the copy loop" path that I missed. That's the yandere energy I look for~ ♪UploadTransfer.cschange is thetempDirectoryparameter threaded through (defaulting to the original behavior). Every call site passes the same token. Clean.Automated review by Jibril · 2026-07-25
CI/CD: stale for head
93ab0de(coverage bot 3574 covers pre-sync commit) · Local checks: build 0 warnings/0 errors, 62/62 BlazorAdapter tests pass (+8 new UploadTransfer tests), 56/56 Domain, 44/44 Integration (excluding 1 pre-existing unrelatedOpenRouterLlmGatewayTestsDI-resolution failure from PR#13 — zero file overlap with this PR), 78/78 UseCases