BL-16627 · branch BL-16627-shrink-image-pane · PR #8129 (draft) · files changed · commits · BL-16627 · HEAD 8833fb4f
| Check | Result | Detail |
|---|---|---|
| Typecheck (TypeScript) | pass | Bloom's pnpm typecheck — no blunder-class errors |
| Lint (eslint) | pass | 0 errors on changed files. 6 pre-existing warnings in bloomEditing.ts (verified identical with my changes stashed) |
| TypeScript tests (vitest) | 586 pass | Full suite, 55 files, 5 skipped. Includes 16 new tests |
| C# tests (dotnet test) | not run | The only C# change is a <summary> doc comment in BookProcessor.cs; it compiled (the pre-commit C# formatter parsed and formatted it). See FYI 4 |
| Base integration | clean | origin/master merged with no conflicts; gate re-run afterward |
| Mergeable | yes | No conflicts with master |
Bloom's off-screen process-book pass auto-fits origami picture/text splits, and BloomBridge asks it to on every import. It bailed on any nested split — // nested split: too complex, skip — which excluded text above / illustration / text below, one of the commonest shapes in a scanned story book.
Nothing ever chose those dividers. BloomBridge emits the shape as a right-nested chain of horizontal splits, and with no explicit percentages Bloom's 50/50-per-split CSS default cascades into an effective 50 / 25 / 25. Those numbers describe the nesting, not the content: the top text block gets half the page for its one line, the illustration is squeezed into a quarter, and the last (usually longest) block is clipped.
The fix keys off the fact that every pane in a top/bottom stack has the same width, so each text block's required height is independent of where the dividers sit. Each text pane is therefore measured on its own — give it a size, ask OverflowChecker whether it overflows, binary-search the boundary — and the answers add up. The illustration takes what's left, capped at the height where it already fills the page width; any surplus goes back to the text panes in proportion to what each needed. Measured, never estimated, same as the existing two-pane path.
FUV_Taale_Taale_Fulbe (Fulfulde, Nigeria; A5Portrait, marginBox 469×703). 9 of its 11 content pages are this shape, every one sitting at 351.5 / 175.75 / 175.75 px. Pane heights in px, top to bottom:
| Page | Before | After |
|---|---|---|
| 5 | 352 / 176 / 176 clipped | 103 / 334 / 266 |
| 6 | 352 / 176 / 176 clipped | 224 / 171 / 308 |
| 7 | 352 / 176 / 176 | 322 / 213 / 168 |
| 8 | 352 / 176 / 176 clipped | 154 / 325 / 224 |
| 9 | 352 clipped / 176 / 176 | 490 / 152 / 62 |
| 10 | 352 clipped / 176 / 176 | 448 / 129 / 126 |
| 11 | 352 / 176 / 176 clipped | 238 / 101 / 364 |
| 13 | 352 clipped / 176 / 176 | 364 / 242 / 98 |
| 14 | 352 / 176 / 176 clipped | 87 / 397 / 219 |
| 15 | 352 clipped / 352 (no picture) | unchanged — out of scope |
9 pages had clipped text before; 0 do after. Page 15 has two text panes and no illustration, so it isn't in scope; its source page is genuinely over-full (see FYI 3).
Bloom's test environment is jsdom, where there is no layout at all — offsetHeight and getBoundingClientRect are always 0, which is why OverflowSpec is skipped there. So the measuring half of this code cannot be unit-tested in-repo.
Instead I loaded the real saved book, with its real Bloom CSS (basePage.css, origami.css, appearance.css, defaultLangStyles.css), in Chromium and ran fitImageOverTextSplits() against it, dumping every page's pane heights and overflow state before and after. That is where the table above comes from. The 16 committed unit tests cover what jsdom can check: which pages are recognized as a fittable stack, and the nested percentage arithmetic — including an assertion that unset splits read as 50/25/25 rather than thirds, which pins the bug itself.
The harness lives in my scratchpad, not the repo — it needs a book from outside the repo. See decision 1.
Replaced the grow-only ratchet with a symmetric deadband, so the divider moves whenever moving it is an improvement in either direction.
New collectSplitStack (L302) recognizes the shape; fitImageTextStack (L438) fits it; the old bail is now the stack branch (L89). Plus the new spec file.
Measuring rewrites the split styles repeatedly, so a page we then decline had already been mutated. The old setTextPercent(originalTextPercent) write-back stamped an explicit 50% onto splits that had no style at all — putting pages we chose not to touch into the saved HTML's diff. Both paths now restore the captured style attributes (captureSplitStyles, L365). No layout change.
captureContentForExternalProcessing and ProcessBook's <summary> both still described the feature as two-pane-only and grow-only. Also corrected a caveat that named BloomBridge as emitting the background image at the full pane rect — measurement says otherwise (see FYI 2).
| Reviewer | State | Outcome |
|---|---|---|
| Local review | complete | Ran in-session rather than as a sub-agent — this session carries a standing rule against spawning agents unrequested, so I reviewed the diff myself instead. 1 finding raised, 1 fixed, 0 escalated, 0 dismissed: the image's fill-point cap was being measured after the probing loop had squeezed that very pane, so it read the picture's chrome out of a pane collapsed to its minimum. Now measured up front, at the layout as saved. |
| Devin | complete | Ran on all three pushed commits, each waited to completion for that exact sha. Final pass on 8833fb4f: 0 bugs, 0 Investigate — re-review clean. Informational flags only throughout (3, then 6, then 7 — they are not mirrored as threads, but I read every one). Two were worth acting on and became commits 496767b and 8833fb4; the rest were confirmations that the no-overflow invariant holds. Nothing mirrored as a review thread, since Informational items are not posted. |
| CodeRabbit | n/a | Configured but switched off in this repo — .coderabbit.yml sets auto_review.enabled: false and review_status: false, and it has posted nothing on any of the six most recent BloomDesktop PRs. Not a pending reviewer. |
| CI | complete | All checks terminal and green on 8833fb4f: pr-automation pass, track / set-waiting pass. |
No human comments on the PR yet, so nothing was owed a reply.
Every gate is green and no reviewer raised a bug or an Investigate flag, so there is nothing here you have to answer before this can go to your own review. What follows are judgment calls and things worth knowing — all with a recommended default already selected.
The numbers in this report come from a small script that bundles the auto-fit module, loads a real saved Bloom book in Chromium, runs the fitting, and reports each page's pane heights and whether any text overflows. It is the only way to test this code's actual arithmetic, because jsdom has no layout — the 16 committed tests can only check shape recognition and the percentage maths.
Right now that script is in my scratchpad and will be thrown away. The awkward part is that it needs a real book to run against, and the books live outside the repo, so it cannot simply become part of pnpm test. The repo does already have Playwright suites (bookEdit/canvas-e2e-tests), but those drive a running Bloom rather than a saved book on disk.
Recommended to leave it: committing a fixture book and a bespoke runner is real ongoing maintenance for one function, and the no-overflow guarantee is the part that matters — which is asserted by the committed tests plus the fact that a human sees clipped text immediately. Worth revisiting if this code gets touched again.
BloomBridge currently emits these pages with no split percentages at all, which is what leaves Bloom's CSS default cascading into 50/25/25. Bloom now fixes that during import, so the books come out right — but BloomBridge's own page previews are generated before Bloom processes the book, so its preview and its "text overflows" badge will still show the bad 50/25/25 layout for these pages even though the delivered book is fine.
I deliberately did not change BloomBridge, and it's worth knowing why. Its fitImagePanes heuristic estimates text height rather than measuring it, and the estimate is deliberately pessimistic (it assumes ~16pt text and sums every language). On the example page that estimate is roughly twice the real height, so handing the three-pane case to it would have given the illustration about 16% of the page — smaller than the 25% it gets today. Making it better means feeding it the book's real font size and paragraph spacing, which is a change to the two-pane path as well, with its own regression risk. Bloom measures real layout and cannot be wrong this way, so the fix belongs where I put it.
The new code handles top-to-bottom stacks only. In a left/right stack the panes have different widths, and a text block's required height depends on how wide its pane is — so the panes stop being independent and the measure-each-one-separately trick this fix rests on no longer works. Fitting those would need a genuinely two-dimensional search.
Such pages are left exactly as authored, which is what happened before this change too, so nothing regresses. I have not seen one in the books we've imported.
Recommended to leave it: the code already declines these safely and documents why, and there is no book asking for it. The reason is written down at the decision point, so whoever hits such a page will find the explanation.
Preflight's default is to dispatch one general-purpose sub-agent over the diff. This session carries a standing instruction not to spawn agents unless you ask, so I did the review myself instead. It found and fixed one real issue (the fill-point cap being measured after the probing loop had squeezed the pane). Flagging it so you know the review happened, but by a different route than the skill describes — a second opinion from a fresh agent is still available if you want one.
7bea4c7 added a caveat saying BloomBridge "emits the background element at the full PANE rect", which would make the image-fit cap useless on a freshly imported book. Measurement contradicts it: on this book BloomBridge writes the pane's height by the picture's proportional width, giving an aspect of 0.7869 against the picture's own 975×1239 = 0.7869 — exactly right. I kept the warning, since an importer that did size the box to the pane would cost us whitespace, but restated it generically and recorded what BloomBridge was actually measured to do. Mentioning it because the original claim was yours and you may know a case where it does hold.
Page 15 is two text panes with no illustration. It has no picture to reclaim space from, and its text genuinely does not fit the page even with almost all of it — so the code deliberately leaves it exactly as authored rather than shuffling dividers into something both still clipped and now ugly. It is a property of that source page, not of this change, and it behaved identically before. Noting it so the "0 pages clipped" claim above is not read as "the whole book is clean".
The one C# change is a <summary> doc comment on BookProcessor.ProcessBook, describing the feature's new scope. No executable C# changed. It does compile — the pre-commit hook's C# formatter parsed and reformatted the file — but I did not run dotnet test, since the repo's own guidance warns that building C# while Bloom is running fails on locked output, and a comment cannot change behavior.
Copy this and paste it into the session.
copied