fix: contain the organizer's scrolling and make step 2 explicit #62
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/organizer-layout"
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?
Owner feedback on the merged page organizer (#55): scrolling the page dragged the preview along ("the selected image stays stuck" out of view), the preview was too small, the wizard should use the full width like the workspace pages — and resuming a draft auto-advanced past step 2, with no way back to organizing.
What's in
max-widthcolumn the shell scrolled as a whole, so the organizer's internaloverflow-ynever engaged — there was no bounded height above it. It now extends the shell's scrolling model one level down exactly likeProjectWorkspacePage(ADR 0010):.wizardis a full-width fill column (flex: 1 1 auto; min-height: 0), the upload card is fixed page chrome (the count text and Continue moved into a footer row inside it, so Continue is always visible), and the organizer sits below the card as the fill sibling — only its chapters column scrolls. The preview column no longer scrolls at all: the page image fits the panel whole viaobject-fit: contain(seeing the page is the point, not scrolling it).minmax(16rem, 22rem)→minmax(20rem, 28rem)(352px → 448px at full size)..wizard__narrowwrapper — a name field stretched across a wide monitor helps nobody. Thewizard--widemodifier is gone.images_uploadedis set automatically by the first import, so it cannot mean "done organizing" — a resumed wizard lands back on step 2, and only the explicit Continue (or a draft whose setup actually progressed,setup_done+) reaches step 3. ADR 0020's amendment gains the sentence. Within a session Back/Continue work as before.Tests — 553/553 green (76 + 213 + 93 + 171).
An_uploaded_draft_still_resumes_at_the_upload_stepreplaces the old resumes-at-setup fact: asserts step 2 with Continue enabled, then that Continue — and only Continue — reaches step 3.SetupChatTestsnow walk through the explicit Continue via a sharedRenderAtSetupStep/RenderWizardAtSetuphelper — the same path a user takes; their assertions are otherwise unchanged.Browser-verified (Playwright, fresh project + real 8-page zip, 1440×900 and a cramped 1200×550): the document never scrolls at either size (
scrollHeight == clientHeight); at the cramped size the chapters column scrolls internally (scrollTopmoves) while the preview panel stays fully in view with the selected image fixed in place; preview measures 448px; the wizard spans the viewport; the preview image fits its panel with zero overflow; a reload after upload resumes at step 2, Continue reaches setup, Back returns.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.8%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 91.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A scrolling bug, the kind that hides in the gap between two flex boxes~ I do love a good layout chase. And reworking the resume contract so the auto-set
images_uploadedflag stops lying about "done organizing"? That is sharp, fufu~ ♡ The diagnosis of why the oldmax-widthcolumn defeated the organizer's internal scroll is exactly right — no bounded height above it, sooverflow-y: autowas a no-op.Verdict: ✅ Looks good to me~
This is a clean, well-reasoned fix. The CSS cascade is correct:
.wizard(flex column,flex: 1 1 auto,min-height: 0) →.wizard__organize(flex column,flex: 1 1 auto,min-height: 0) →.organizer(flex: 1, grid) → only the chapters column scrolls. The preview flipping toobject-fit: containso the page fits whole instead of scrolling its own panel is the right call — "seeing the page is the point, not scrolling it" is exactly the user-facing intent. And retiringwizard--widein favor of a uniform full-width shell with a.wizard__narrowwrapper for the reading steps is a genuine consistency win against the workspace/page siblings (ADR 0010).The resume-logic rewrite is the load-bearing behavioral change, and it's correct:
SetupState >= SetupState.SetupDone ? 3 : 2replaces the old== Named ? 2 : 3. With the enum contractNamed=1 < ImagesUploaded=2 < SetupDone=3 < Ready=4(and the file documenting "append, never renumber"), the>=is the honest expression of "only a draft that actually progressed setup resumes at step 3." AReadyproject (4) correctly falls through to 3 as well — though in practice the wizard effect bounces to the workspace before that matters. The ADR 0020 amendment sentence is precise and matches the code. I broke-tested the logic mentally both ways:ImagesUploaded(2) →2 < 3→ step 2 ✓;SetupDone(3) →3 >= 3→ step 3 ✓.✅ What I liked~
An_uploaded_draft_still_resumes_at_the_upload_stepdoesn't just assert the step — it assertsDoesNotContain("Project setup")(proves it's not on step 3) AND clicks Continue to prove only Continue reaches step 3. That's the contract pinned, not a tautology. fufu~ ♡RenderAtSetupStep/RenderWizardAtSetuphelpers are the right DRY move — every step-3-dependent test now walks the real user path instead of leaning on the old (now-wrong) auto-advance. The seed-with-page addition (SeedPage(draft, order: 0, withImage: true)) is necessary and correct: Continue isDisabledwhenPageCount == 0, so the click would no-op without it. Sharp catch.Another_wizards_stale_error_never_bleeds_into_this_oneassertion flip (Project setup→Upload the pages) correctly tracks the new resume target — easy to miss, not missed.images_uploadedis automatic), names the rule (explicit Continue orsetup_done+). No scope creep into re-architecting the state machine.💡 Little ideas (non-blocking)~
>= SetupState.SetupDone → step 3resume arm is unexercised by the wizard tests.grepacrosstests/showsSetupState.SetupDoneis never seeded in anyProjectWizardPage/SetupChattest — every wizard test seedsImagesUploadedand walks through Continue. TheSetupDone-resumes-at-3 path (e.g. a draft whose setup chat crashed mid-agent, reloaded) compiles and is logically sound, but no test actually drives aSetupDonedraft into the wizard and asserts step 3. Not blocking because (a) the>=comparison is trivially correct against a documented never-renumber enum, and (b) the broader "wizard bounces to workspace once Ready" effect would shadow it anyway — but if you ever want a tripwire, a one-lineSeedProject("...", SetupState.SetupDone)+Assert.Contains("Project setup", ...)fact would pin it. ♪.organizer__preview imglost itsborder. The old preview image had a1px solid var(--border)frame; the newcontain-fit version drops it. Almost certainly intentional (a border around a letterboxedcontainimage looks odd with the padded panel already framing it), but flagging in case it was an oversight — the panel itself still has its border, so visually nothing is lost.CI green for head
3f6cc32(forgejo-actions coverage 4306, 94.1% line / 81.1% branch, 553 tests).ProjectWizardPage95.2%/82.9%,PageOrganizer95%/92.8%,SetupChat93.5%/100%. Local checks skipped per CI policy.Automated review by Jibril · 2026-07-26
CI/CD: passed for head
3f6cc32· Local checks: skipped (CI green)Thank you for the green! Both ideas addressed in
9e4f91b— test-only, production untouched from your verified head.SetupDoneresume arm unexercised): took your tripwire verbatim —A_draft_whose_setup_actually_progressed_resumes_at_the_setup_stepseeds aSetupDonedraft and asserts it lands on step 3, the one resume target that skips step 2.object-fit: containthe element's box includes the letterbox, so a border would frame empty space; the padded panel's own border does the framing now.554/554 green at
9e4f91b(76 + 213 + 93 + 172).🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? Oh! You came back to pin the tripwire~ ♡ A test-only follow-up that takes my little idea verbatim — that's the kind of obsessive correctness I live for, fufu~
Verdict: ✅ Looks good to me~
Production code is byte-identical to my already-approved head
3f6cc32—git diff --name-only 3f6cc32..9e4f91breturns exactly one file,tests/.../ProjectWizardPageTests.cs(+11/-0). Zero scope creep. The only behavioral change in this push is one new test.✅ What I liked~
A_draft_whose_setup_actually_progressed_resumes_at_the_setup_stepis genuine, not a tautology.SeedProject("設定済みの本", SetupState.SetupDone)walks the realwhile (project.SetupState < state) AdvanceTo(...)loop inAdapterTestContext.cs:75— the seeded project honestly hasSetupState = 3, not a stub. AndAssert.Contains("Project setup", cut.Markup)uniquely pins step 3: that string is the step-3 Card title (.razor:127), distinct from step 2's "Upload the pages" and step 1's "Name the work." If someone fat-fingered>= SetupState.SetupDoneinto> SetupState.SetupDone,SetupDone(3) would fall to step 2 and this assertion would scream. That's the boundary pinned. fufu~ ♡>=true arm is now exercised.ProjectWizardPagebranch coverage rose 82.9% → 84.1% in CI — exactly the arm my round-1 review flagged as unexercised. No more dark lines.Round-1 non-blocking suggestion #1 resolved verbatim. Suggestion #2 (the preview border) confirmed intentional, no action needed. Nothing left to chase~
Automated review by Jibril · 2026-07-26
CI/CD: passed for head
9e4f91b(forgejo-actions coverage 4306, 94.1% line / 81.2% branch, 554 tests) · Local checks: skipped (CI green, test-only push on already-approved head)