BL-16450 API for conversion

[hatton]

This change is 

Devin review

[greptile-apps]

Greptile Summary

This PR implements the BloomBridge book-conversion API: a suite of external/* HTTP endpoints that let an external tool drive Bloom to process, import, and inspect books without a human at the keyboard. The centerpiece is external/process-book, which loads each page of a book into a throwaway off-screen WebView2, runs the same load-time DOM fix-ups the live editor would apply (image sizing, canvas-element layout, auto-fit origami splits), and saves the result — all while the UI thread keeps pumping so the collection tab stays live behind an opaque busy overlay.

• New BookProcessor + shared-environment batch: each page gets a fresh WebView2 control but shares one CoreWebView2Environment, cutting per-page overhead from ~315 ms to ~100 ms (~30 s → ~18 s for a 22-page book).
• Async/poll design for process-book: the endpoint replies immediately with a jobId and runs heavy work via BeginInvoke; the client polls external/process-book-status to avoid hanging on a dropped keep-alive socket.
• New autoFitImageOverTextSplits.ts: binary-searches the smallest text-pane percent that avoids overflow (using real browser layout via OverflowChecker) and caps image growth at the natural image-fill point.

Important Files Changed

Filename	Overview
src/BloomExe/web/controllers/ExternalApi.cs	Major new file introducing process-book, process-book-status, update-book, select-book, add-book, and collection-languages endpoints; async/poll design is well-thought-out with re-entrancy guards and lock-protected job state, but has a stale "blocks" comment and a narrow window where _processBookInProgress can get permanently stuck.
src/BloomExe/Book/BookProcessor.cs	New class driving the off-screen per-page WebView2 fix-up; shared-environment batch, foreground-restoration, CKEditor stripping, and the three-step JS handshake are all well-implemented and well-documented.
src/BloomBrowserUI/bookEdit/js/bloomEditing.ts	New captureContentForExternalProcessing export and extractAndStripPageContentForSave refactor; kMaxWaitTimeMs doubled to 4000ms for both live and off-screen paths, increase is documented and intentional.
src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts	New binary-search image/text split auto-fitter; correctly restricts to simple two-pane pages with no overlays, uses real browser layout for overflow detection, and caps image growth at the natural image-fill point.
src/BloomExe/WebView2Browser.cs	Adds shared CoreWebView2Environment batch and exposes IsReadyToNavigate; thread-safety assertion on the UI-thread-only statics is appropriate.
src/BloomBrowserUI/bookEdit/editablePage.ts	Exports captureContentForExternalProcessing and adds the __bloomEditablePageReady / __bloomExternalPageContent Window globals and handshake documentation; flag is correctly set after bootstrap() in $(document).ready.
src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx	Simple modal overlay driven by externalProcessing websocket show/hide events; correctly starts closed so a socket reconnect resets to the hidden state if the React app reinitializes.
src/BloomExe/Edit/EditingModel.cs	Adds ReloadCurrentBookDiscardingEdits, GetEditedPageDomFromBrowserContent, and _reloadFromDiskOnLeavingEditTab flag; the on-leave-tab disk-reload path correctly handles both SavePending and non-pending states.
src/BloomExe/web/controllers/CommonApi.cs	Adds editableCollectionFolder/collectionName to /bloom/info and null-guards currentBook?.BookData in bubble-language collection to prevent NRE when off-screen process-book loads a page without a selected book.

_{Reviews (19): Last reviewed commit: "Raise off-screen capture delay budget fr..." | Re-trigger Greptile}

[cubic-dev-ai]

7 issues found across 15 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx">

<violation number="1" location="src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx:29">
P2: User-visible fallback text is hardcoded and bypasses localization.</violation>
</file>

<file name="src/BloomExe/Book/BookProcessor.cs">

<violation number="1" location="src/BloomExe/Book/BookProcessor.cs:214">
P1: Browser readiness polling has no timeout, so a stuck WebView2 init can hang process-book forever.</violation>
</file>

<file name="src/BloomExe/WebView2Browser.cs">

<violation number="1" location="src/BloomExe/WebView2Browser.cs:403">
P2: Shared WebView2 environment creation is race-prone: concurrent async initializations can create multiple environments in one batch.</violation>
</file>

<file name="src/BloomExe/web/controllers/ExternalApi.cs">

<violation number="1" location="src/BloomExe/web/controllers/ExternalApi.cs:559">
P2: `external/updateBook` returns success even when the target book id is not found, causing false-positive "Added/Updated" notifications and hiding real failures from callers.</violation>
</file>

<file name="src/BloomExe/ProjectContext.cs">

<violation number="1" location="src/BloomExe/ProjectContext.cs:389">
P2: The new static collection-settings assignment is never cleared, so `common/instanceInfo` can return stale collection info after a project is closed/reloaded.</violation>
</file>

<file name="src/BloomExe/CollectionTab/CollectionModel.cs">

<violation number="1" location="src/BloomExe/CollectionTab/CollectionModel.cs:259">
P2: Import-by-id replacement continues even when recycle fails, which can create duplicate books with the same id and break later id-based targeting.</violation>
</file>

<file name="src/BloomExe/Edit/EditingModel.cs">

<violation number="1" location="src/BloomExe/Edit/EditingModel.cs:402">
P1: `OnTabAboutToChange` now reloads from disk in the SaveThen fallback path, which can run during `SavePending` and race with an in-flight save.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

3 issues found across 5 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx">

<violation number="1" location="src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx:29">
P2: User-visible fallback text is hardcoded and bypasses localization.</violation>
</file>

<file name="src/BloomExe/web/controllers/ExternalApi.cs">

<violation number="1" location="src/BloomExe/web/controllers/ExternalApi.cs:109">
P2: Renaming external API routes without keeping legacy aliases is a breaking API change for existing callers of `external/updateBook` and `external/selectBook`.</violation>

<violation number="2" location="src/BloomExe/web/controllers/ExternalApi.cs:559">
P2: `external/updateBook` returns success even when the target book id is not found, causing false-positive "Added/Updated" notifications and hiding real failures from callers.</violation>
</file>

<file name="src/BloomExe/CollectionTab/CollectionModel.cs">

<violation number="1" location="src/BloomExe/CollectionTab/CollectionModel.cs:259">
P2: Import-by-id replacement continues even when recycle fails, which can create duplicate books with the same id and break later id-based targeting.</violation>
</file>

<file name="src/BloomExe/ProjectContext.cs">

<violation number="1" location="src/BloomExe/ProjectContext.cs:389">
P2: The new static collection-settings assignment is never cleared, so `common/instanceInfo` can return stale collection info after a project is closed/reloaded.</violation>
</file>

<file name="src/BloomExe/Edit/EditingModel.cs">

<violation number="1" location="src/BloomExe/Edit/EditingModel.cs:402">
P1: `OnTabAboutToChange` now reloads from disk in the SaveThen fallback path, which can run during `SavePending` and race with an in-flight save.</violation>
</file>

<file name="src/BloomExe/ToPalaso/ProcessExtra.cs">

<violation number="1" location="src/BloomExe/ToPalaso/ProcessExtra.cs:28">
P3: Duplicate P/Invoke declaration for `GetForegroundWindow` — now that `ProcessExtra.cs` exposes a public `GetForegroundWindow()`, the identical private declaration in `ProblemReportApi.cs` (line 1063) should be removed and its callers switched to `ProcessExtra.GetForegroundWindow()` to keep a single canonical definition of the native import.</violation>
</file>

<file name="src/BloomExe/Book/BookProcessor.cs">

<violation number="1" location="src/BloomExe/Book/BookProcessor.cs:187">
P2: Foreground restore can wrongly override user focus changes because it always forces the startup window whenever foreground differs.</violation>
</file>

<sub>Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic</sub>

[hatton]

@greptileai please review

[hatton]

I chose not to act on any of the ideas in Devin up to this point.

src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts:191
Scale inconsistency in computeImageFitTopPercent may produce suboptimal (but safe) image caps
In computeImageFitTopPercent, scale (from EditableDivUtils.getPageScale()) is used to adjust extraHeight at line 199 ((topComponent.offsetHeight - topCanvas.offsetHeight) / scale) but NOT to adjust imageHeight at line 193 (width / aspectRatio) or splitPaneHeight at line 189. Since offsetWidth/offsetHeight are CSS layout dimensions unaffected by CSS transforms, all values are in the same coordinate space, and dividing only extraHeight by scale introduces an asymmetry. However, the comment says this "mirrors the horizontal-split aspect math in split-pane.ts getImagePercent()" so it may be intentionally matching existing behavior. In the off-screen browser context (where this code primarily runs), getPageScale() likely returns 1 (no scaling applied), making this moot in practice. Even when scale ≠ 1, the effect is only that the image-width cap is slightly too generous or conservative — the binary search + cushion still prevents text overflow, so this is cosmetic, not functional.

---

src/BloomExe/Book/BookProcessor.cs:36
BringBookUpToDate may write to disk before per-page processing, weakening all-or-nothing claim
The BookProcessor.ProcessBook doc comment at BookProcessor.cs:39 says "All-or-nothing: a failure on any page... throws, the save at the end is skipped, and nothing is persisted." However, book.BringBookUpToDate(new NullProgress()) at line 75 runs before the per-page loop and may write structural changes (xmatter migrations, CSS link additions) to disk. If a subsequent page fails and throws, those structural changes persist even though the per-page Save is skipped. For the path-based flow (HandleProcessBookByPath), the book is typically in a staging folder so this is harmless; for the id-based flow the book IS in the collection. The practical impact is minimal (structural updates are idempotent), but the documented guarantee is slightly overstated.

---

src/BloomExe/web/controllers/ExternalApi.cs:339
HandleProcessBookByPath could hit a stale FolderPath if BringBookUpToDate renames the selected book's folder
In ExternalApi.HandleProcessBookByPath (ExternalApi.cs:339-342), processingSelectedBook is computed by comparing the selected book's FolderPath with the supplied folderPath. BookProcessor.ProcessBook subsequently calls BringBookUpToDate and Save, both of which can rename the book folder to match the title. If the folder is renamed, the selected book's FolderPath (a separate Book instance) still points to the old location. Then ReloadCurrentBookDiscardingEdits calls CurrentBook.ReloadFromDisk(null) on the selected book, which would try to read from the old (now-renamed) path. In practice this edge case requires (a) the external tool to target the selected book's own folder AND (b) the processing to change the title/folder name. The code documents that the normal path targets a staging folder, not the selected book, and HandleProcessBook already requires the Collection tab. Still, this path is theoretically reachable.

---

src/BloomExe/web/controllers/ExternalApi.cs:660
Tab change from ReloadCurrentBookDiscardingEdits may be asynchronous while HandleUpdateBook continues
In HandleUpdateBook (src/BloomExe/web/controllers/ExternalApi.cs:545-556), after calling _editingModel.ReloadCurrentBookDiscardingEdits() (which triggers ChangeTab if the Edit tab is visible), the code immediately continues with _collectionModel.ReloadEditableCollection(). However, ChangeTab raises _selectedTabAboutToChangeEvent which calls SaveThen, and if the state machine needs to wait for browser content before completing the tab switch, the PostponedWork (which actually completes the tab change and sets _tabSelection.ActiveTab) runs later. This means ReloadEditableCollection runs while the tab change is still in flight. Since both operations run on the UI thread and the async parts only trigger when the message loop is pumped (which doesn't happen until after the handler returns), there shouldn't be data corruption, but the ordering is subtle and could cause issues if any of the reload logic checks _tabSelection.ActiveTab.

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts:1468
doWhenWorkspaceBundleLoaded polls indefinitely in off-screen context
In the off-screen process-book context, bootstrap() at bloomEditing.ts:591 calls doWhenWorkspaceBundleLoaded(callback). This function (workspaceFrames.ts:62-75) checks for window.workspaceBundle and retries via setTimeout every 10ms if not found. In the off-screen context there is no workspace frame, so workspaceBundle is never set and the timer fires indefinitely for the lifetime of each browser. Each off-screen browser is disposed after processing its page (a few seconds), so the accumulated timers are short-lived and the callbacks are trivially cheap (check a property, re-schedule). Not a practical problem, but worth being aware of if off-screen processing ever becomes longer-lived.

---

src/BloomExe/Collection/BookCollection.cs:455
BookMetaData.FromFolder called during collection rescan has potential side effects
The new stale-id check in BookCollection.AddBookInfo at line 457 calls BookMetaData.FromFolder(folderPath) during what is conceptually a read-only collection scan. As the comment notes, this method is not a pure read — it can restore from backup (deleting/moving files) on corrupt metadata. The surrounding catch block at line 448 handles thrown exceptions, and the ?? reusableSelectionInfo.Id fallback handles a null return. The side effects are confined to corrupt-metadata edge cases and match what new BookInfo(...) would do anyway, but this is worth knowing for anyone debugging metadata file changes during collection reloads.

---

src/BloomExe/Collection/BookCollection.cs:455
BookCollection id-mismatch check reads meta.json twice for the selection book
When the book folder matches the current selection AND the SaveContext matches, the new code additionally calls BookMetaData.FromFolder(folderPath) to verify the on-disk id still matches. This is an extra disk read that happens on every collection scan for the selected book. BookMetaData.FromFolder reads and parses the entire meta.json. For normal collection scans (not triggered by external tools), this adds a small I/O cost. The comment acknowledges this is intentional ('preserving the previous reuse behavior' when meta.json is unreadable). In practice, meta.json files are small and cached by the OS, so the performance impact should be negligible.

---

src/BloomExe/Book/BookStorage.cs:1850
BookStorage.SetBookName: Debug.Fail removed, changing debug-build behavior
In BookStorage.cs:1850-1861, the catch block for failed folder renames was changed from Debug.Fail(...) (which crashes debug builds) to just Logger.WriteEvent(...). The comment explains this was intentional: the old Debug.Fail turned a handled, non-fatal condition (folder rename collision) into a hard crash in debug builds when external/process-book triggers a rename to an already-existing same-id folder. Release builds already survived. This is a deliberate behavioral change in debug builds — the condition is now silently logged rather than crashing. Developers relying on Debug.Fail to catch unexpected renames during manual testing will no longer see the assertion.

---

src/BloomExe/Edit/EditingModel.cs:349
UpdateBookDomFromBrowserPageContent not refactored to use GetEditedPageDomFromBrowserContent
The new GetEditedPageDomFromBrowserContent (src/BloomExe/Edit/EditingModel.cs:349-361) duplicates the parsing logic of the existing UpdateBookDomFromBrowserPageContent(string) at src/BloomExe/Edit/EditingModel.cs:274-289 (split on <SPLIT-DATA>, extract body and CSS, call GetCleanCurrentPageFromBodyAndCss). The original method was not refactored to delegate to the new one, so there are now two places where the <SPLIT-DATA> delimiter and parsing logic are duplicated. The PR comment on extractAndStripPageContentForSave explicitly says the delimiter 'can't drift' on the JS side, but on the C# parsing side the duplication means a future change could be applied to one method and not the other. Not a bug today, but a maintenance concern.

---

src/BloomExe/Edit/EditingModel.cs:396
DoPostSaveAction merges stale content before ReloadFromDisk in the main discard path
When _reloadFromDiskOnLeavingEditTab is true and the state machine takes the main path (state is Editing → SavePending), the browser returns page content, and DoPostSaveAction calls _updateBookWithPageContents to merge it into the in-memory DOM before the postSaveAction lambda runs ReloadFromDisk. This means the stale editor content is briefly merged then immediately overwritten. Because _saveBook() is skipped (_saveActionHandlesSaveBook is true via skipSaveToDisk), nothing is written to disk, so the merge is functionally harmless — but it does execute UpdateDomFromEditedPage with side effects (metadata extraction, title updates) that are then discarded. The DiscardInFlightSave() mechanism in the fallback path (doIfNotInRightStateToSave) avoids this wasted work, but the main path doesn't use it. Not a correctness bug since ReloadFromDisk overwrites everything synchronously before control returns, but worth noting for future maintainers.

---

src/BloomExe/Edit/EditingModel.cs:436
In-flight save completion after tab switch navigates to a page on a hidden browser
In the OnTabAboutToChange fallback path (EditingModel.cs:436-446), when reloadFromDiskInsteadOfSaving is true and a save is already in flight (state is SavePending), DiscardInFlightSave() is called. When the browser eventually returns the page content, DoPostSaveAction skips the merge (discard=true) but still runs the original postSaveAction, which may return a page ID. ToNavigating(pageId) is then called, but since the Edit tab is no longer visible, StartNavigationToEditPage checks if (Visible) and skips the actual navigation. The state machine ends up in Navigating state; when the user returns to the Edit tab, OnBecomeVisible calls GoToPage which triggers ToNavigating again — the Navigating→Navigating transition is handled correctly (either ignored or restarted). Traced through thoroughly; no bug, but the interaction is complex enough to warrant awareness.

---

src/BloomExe/web/controllers/ExternalApi.cs:232
Nested try/catch/finally in HandleProcessBook requires careful reading but is correct
The HandleProcessBook method at src/BloomExe/web/controllers/ExternalApi.cs:232-296 has a non-trivial nested try structure: an outer try/catch/finally (for _processBookInProgress guard + fallback error response) wrapping an inner try/finally (for the 'hide' overlay). The overlay 'show' is sent just before the inner try, so any path that enters the inner try will execute the inner finally and send 'hide'. Early returns (JSON parse failure, tab check) happen before the overlay is shown, so they correctly skip both the inner try and the inner finally. The return; statements inside the inner try (after HandleProcessBookByPath/HandleProcessBookById) correctly trigger both finally blocks. This is correct but complex enough that a reader might wonder whether the overlay could leak; it cannot.

---

src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx:27
ExternalBusyOverlay remains visible if websocket hide event is lost
The ExternalBusyOverlay is shown/hidden via websocket events (externalProcessing show/hide). If the 'hide' event is lost (e.g., websocket disconnects during processing), the overlay would stay permanently visible, blocking all user interaction. The C# side sends 'hide' in a finally block (ExternalApi.cs:293), so it should always fire, but the websocket delivery is not guaranteed. A timeout-based fallback or a way for the user to dismiss the overlay could make this more robust. In practice, since everything runs on the UI thread and the websocket is localhost, delivery loss is extremely unlikely.

---

src/BloomExe/ProjectContext.cs:384
CommonApi.CurrentCollectionSettings is a mutable static set during project initialization
In ProjectContext.cs:384-389, CommonApi.CurrentCollectionSettings is set to the project-scoped CollectionSettings. Since CommonApi is application-scoped (created before any project opens and reused across project opens), this static property could theoretically hold stale settings if a project is closed and another opened in the same Bloom session. However, examining the codebase, ProjectContext is recreated for each collection open, so this static is always refreshed. The pattern matches how CommonApi.WorkspaceView is already used.

---

src/BloomExe/WebView2Browser.cs:280
Shared WebView2 environment batch relies on UI-thread-only invariant with Debug.Assert only
The _useSharedEnvironment / _sharedEnvironment statics in WebView2Browser are explicitly unsynchronized and rely on the invariant that all access is on the UI thread. This is asserted only via Debug.Assert in AssertSharedEnvironmentStaticsAreUiThreadOnly() (lines 300-307), which is stripped in release builds. If a future change inadvertently drives a batch off the UI thread, the statics could corrupt silently in release. The current design is correct for the single caller (BookProcessor), but the fragility is worth noting for future maintenance.

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts:1326
buildSavePageContentString() now mutates the DOM from two distinct call sites
The refactored buildSavePageContentString() calls removeEditingDebris() and getBodyContentForSavePage(), both of which mutate the live DOM (remove classes, blur elements, turn off canvas editing, do CKEditor cleanup). In the live editor path (requestPageContentInternal), the page is re-navigated by the state machine after saving, so mutations are restored. In the off-screen path (captureContentForExternalProcessing), each page is in a fresh disposable browser, so mutations are harmless. However, the function does NOT restore the DOM to its pre-call state — it's a destructive read. If a future caller invokes buildSavePageContentString() in a context where the page continues to be live afterward, the DOM will be left in a stripped state. The function's name ('build...String') doesn't convey that it's destructive.

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts:1451
captureContentForExternalProcessing relies on setTimeout callbacks being pumped by C# polling
In bloomEditing.ts:1416-1423, if activeDelays.length > 0, the function uses setTimeout(waitForDelaysThenFinish, 50) to poll. This works because the C# caller (BookProcessor.WaitForJavascriptResult) calls Application.DoEvents() between polls, which pumps the WebView2 message loop and allows JavaScript timers to fire. In the off-screen context, activeDelays should always be empty (no interactive editing operations are occurring), so the synchronous path (finish() called immediately) should be the normal case. The async path is a safety net for any unexpected delays added during page setup.

---

src/BloomExe/Book/BookProcessor.cs:44
DoEvents pumping during ProcessBook allows re-entrant API calls
BookProcessor (and HandleProcessBook's DoEvents loop at src/BloomExe/web/controllers/ExternalApi.cs:241-245) pump the Windows message loop while occupying the UI thread. Since handleOnUiThread: true dispatches API calls via message posts, a second external/process-book request arriving during processing could be dispatched re-entrantly. The shared statics _useSharedEnvironment/_sharedEnvironment in WebView2Browser.cs:277-278 are not designed for concurrent batches (as noted in comments). In practice, the external caller (BloomBridge) sends requests sequentially, so this is unlikely but worth being aware of if the API is ever called concurrently.

---

src/BloomExe/Book/BookProcessor.cs:243
BookProcessor init loop has no timeout guard
In BookProcessor.ProcessOnePage (src/BloomExe/Book/BookProcessor.cs:214-218), the loop while (!browser.IsReadyToNavigate) { Application.DoEvents(); Thread.Sleep(5); } has no timeout. If WebView2 initialization fails silently (e.g., browser is disposed before completing init), this loop could hang indefinitely. In practice this is unlikely because: (1) WebView2 initialization failure calls Environment.Exit(1), and (2) BookProcessor controls the browser lifecycle and doesn't dispose it until after processing. Still, a defensive timeout matching kReadyTimeoutMs would make the code more robust.

---

src/BloomExe/Edit/EditingModel.cs:300
GetCleanCurrentPageFromBodyAndCss changed from private instance to internal static
In EditingModel.cs:300, GetCleanCurrentPageFromBodyAndCss was changed from private to internal static. I verified it accesses no instance state — it only processes its parameters via static/pure calls (XmlHtmlConverter, BookStorage.CheckForEmptyMarginBoxOnPage, SaveCustomizedCssRules). The visibility change is safe and necessary for GetEditedPageDomFromBrowserContent (the new public static method used by BookProcessor). SaveCustomizedCssRules was similarly made static and also has no instance dependencies.

---

src/BloomExe/web/controllers/ExternalApi.cs:342
HandleProcessBookById reports failure if post-processing throws even though the book was successfully processed
In HandleProcessBookById, after BookProcessor.ProcessBook(book) succeeds (the book is saved to disk), there's follow-up work: ReloadCurrentBookDiscardingEdits, ReloadEditableCollection, UpdateThumbnailAsync. If any of these throw, the exception propagates to the outer catch in HandleProcessBook (ExternalApi.cs:261-264), which calls request.Failed(). The external caller is told the operation failed, but the book has actually been fully processed and saved. A retry would be harmless (ProcessBook is idempotent) but this could cause confusion. Considered flagging as a bug but decided the error reporting is acceptable — the operation (process + refresh UI) did partially fail.

---

src/BloomExe/web/controllers/ExternalApi.cs:548
HandleUpdateBook does not guard against active tab unlike process-book and add-book
HandleUpdateBook does NOT check _tabSelection.ActiveTab before proceeding, unlike HandleProcessBook (which requires Collection tab) and HandleAddBook (which also requires Collection tab). This is intentional: update-book is designed to work even when the Edit tab is active, by calling ReloadCurrentBookDiscardingEdits() which safely kicks the user back to the Collection tab if they're mid-edit. The design difference is justified by the different semantics: process-book rewrites the book (so it must not fight the editor), while update-book reacts to an already-written book and just needs to refresh the in-memory state.

[hatton]

✅ Resolved — Devin now considers this fixed as of 30299f0f (the master merge into this branch). The merge adopted ExtractPageSizeName(nameLower, …) plus a case-insensitive NormalizeLegacyPageSizeName, so mixed-case inputs like EBook16x9Landscape now map to the Device16x9 family and the test passes. — _{marked by Claude Opus 4.8}

---

[Devin] Bug: Legacy ebook layout name normalization fails for mixed-case input, causing a test to fail

SizeAndOrientation.cs:80

A case-sensitivity gap in the page-size-name extraction means inputs like "EBook16x9Landscape" are NOT normalized to the Device16x9 family (NormalizeLegacyPageSizeName at SizeAndOrientation.cs:84), so the new test case asserting this mapping would fail.

Impact: A hypothetical book from a future Bloom version whose class uses "EBook" (capital B) instead of "Ebook" would not be recognized as a Device16x9 layout.

Root cause: Replace("ebook", "Ebook") is dead code after ToUpperFirstLetter()

[hatton]

[Devin] Investigate: Scale inconsistency in computeImageFitTopPercent may produce suboptimal (but safe) image caps

autoFitImageOverTextSplits.ts:284-315

In computeImageFitTopPercent, scale (from EditableDivUtils.getPageScale()) is used to adjust extraHeight at line 199 ((topComponent.offsetHeight - topCanvas.offsetHeight) / scale) but NOT to adjust imageHeight at line 193 (width / aspectRatio) or splitPaneHeight at line 189. Since offsetWidth/offsetHeight are CSS layout dimensions unaffected by CSS transforms, all values are in the same coordinate space, and dividing only extraHeight by scale introduces an asymmetry. However, the comment says this "mirrors the horizontal-split aspect math in split-pane.ts getImagePercent()" so it may be intentionally matching existing behavior. In the off-screen browser context (where this code primarily runs), getPageScale() likely returns 1 (no scaling applied), making this moot in practice. Even when scale ≠ 1, the effect is only that the image-width cap is slightly too generous or conservative — the binary search + cushion still prevents text overflow, so this is cosmetic, not functional.

[hatton]

[Devin] Investigate: BringBookUpToDate may write to disk before per-page processing, weakening all-or-nothing claim

BookProcessor.cs:36-52

BringBookUpToDate(new NullProgress()) at line 75 runs before the per-page loop and may write structural changes (xmatter migrations, CSS link additions) to disk. If a subsequent page fails and throws, those structural changes persist even though the per-page Save is skipped. For the path-based flow (HandleProcessBookByPath), the book is typically in a staging folder so this is harmless; for the id-based flow the book IS in the collection. The practical impact is minimal (structural updates are idempotent), but the documented guarantee is slightly overstated.

[hatton]

[Devin] Investigate: HandleProcessBookByPath could hit a stale FolderPath if BringBookUpToDate renames the selected book's folder

ExternalApi.cs:505-566

HandleProcessBookByPath (ExternalApi.cs:339-342), processingSelectedBook is computed by comparing the selected book's FolderPath with the supplied folderPath. BookProcessor.ProcessBook subsequently calls BringBookUpToDate and Save, both of which can rename the book folder to match the title. If the folder is renamed, the selected book's FolderPath (a separate Book instance) still points to the old location. Then ReloadCurrentBookDiscardingEdits calls CurrentBook.ReloadFromDisk(null) on the selected book, which would try to read from the old (now-renamed) path. In practice this edge case requires (a) the external tool to target the selected book's own folder AND (b) the processing to change the title/folder name. The code documents that the normal path targets a staging folder, not the selected book, and HandleProcessBook already requires the Collection tab. Still, this path is theoretically reachable.

[hatton]

[Devin] Investigate: Tab change from ReloadCurrentBookDiscardingEdits may be asynchronous while HandleUpdateBook continues

ExternalApi.cs:849-865

In HandleUpdateBook (ExternalApi.cs:545-556), after calling _editingModel.ReloadCurrentBookDiscardingEdits() (which triggers ChangeTab if the Edit tab is visible), the code immediately continues with _collectionModel.ReloadEditableCollection(). However, ChangeTab raises _selectedTabAboutToChangeEvent which calls SaveThen, and if the state machine needs to wait for browser content before completing the tab switch, the PostponedWork (which actually completes the tab change and sets _tabSelection.ActiveTab) runs later. This means ReloadEditableCollection runs while the tab change is still in flight. Since both operations run on the UI thread and the async parts only trigger when the message loop is pumped (which doesn't happen until after the handler returns), there shouldn't be data corruption, but the ordering is subtle and could cause issues if any of the reload logic checks _tabSelection.ActiveTab.

[hatton]

[Devin] Investigate: Device16x9 layouts get renamed to 'Ebook' in the user-facing display name

Layout.cs:169-176

The DisplayName property in Layout.cs:161-176 now maps "device16x9 portrait" to "Ebook 9x16 Portrait" and "device16x9 landscape" to "Ebook 16x9 Landscape". This is a user-visible behavioral change: existing books with Device16x9 layouts will now show different display names in the layout chooser. The localization IDs (LayoutChoices.Device16x9Portrait, LayoutChoices.Device16x9Landscape) remain the same, so existing translations map to the new English text. This may surprise translators or users who had become familiar with the old names.

[hatton]

[Devin] Investigate: Replace("ebook", "Ebook") in ExtractPageSizeName is dead code

SizeAndOrientation.cs:76

The .Replace("ebook", "Ebook") added at SizeAndOrientation.cs:76 can never match at runtime. The line immediately before it calls .ToUpperFirstLetter(), which capitalizes the initial 'e' to 'E', making the substring "Ebook..." — at which point the all-lowercase needle "ebook" is no longer present. This means the Replace is a no-op for every possible input.

For the normalization to work, it relies entirely on the subsequent NormalizeLegacyPageSizeName doing an exact match against "Ebook16x9" / "Ebook9x16". This happens to work for all-lowercase inputs (e.g. from HTML classes) because ToUpperFirstLetter produces exactly "Ebook...", but the dead Replace gives a false sense that mixed-case inputs like "EBook..." are handled.

[hatton]

[Claude Opus 4.8] Disposition of the Devin findings, for the human reviewer.

Fixed in ed7b31f50:

• Bug — case-sensitive ebook 16x9 normalization (SizeAndOrientation.cs): NormalizeLegacyPageSizeName compared against Ebook9x16/Ebook16x9 case-sensitively while ExtractPageSizeName only uppercases the first letter, so EBook16x9Landscape (capital B) was not mapped to Device16x9 and the committed test FromString_Ebook16x9Aliases_NormalizeToDevice16x9 failed. Now compares case-insensitively; all 15 SizeAndOrientationTests pass. Devin's re-review now reports 0 bugs.
• Flag — dead code (SizeAndOrientation.cs:76): removed Replace("ebook", "Ebook"). It was provably dead — "ebook" only ever appears at position 0, already capitalized by ToUpperFirstLetter (unlike "letter"/"weaver", which appear mid-string and were kept).

Remaining 5 Investigate flags — reviewed, no change (intentional / FYI):

• Scale inconsistency (autoFitImageOverTextSplits): intentional — mirrors split-pane.ts getImagePercent() so the fit cap agrees with the actual re-fit; off-screen getPageScale() is 1; the binary-search cushion prevents overflow regardless. Cosmetic only.
• BringBookUpToDate writes before per-page loop: the all-or-nothing doc comment is slightly broad; structural updates are idempotent, and the BloomBridge path operates on a staging folder. Doc nuance only.
• Stale FolderPath on rename (ExternalApi): requires targeting the selected book's own folder and a title/folder rename; the documented BloomBridge flow targets a staging folder, so it isn't reached.
• Tab-change async (ExternalApi): traced through — everything runs on the UI thread and the async tab-switch work only runs once the handler returns; no data corruption.
• Device16x9 → "Ebook" display name (Layout.cs:169): intentional — this is the StoryWeaver eBook feature (Device16x9 is presented to users as "Ebook 16x9 / 9x16").

[hatton]

[Claude Sonnet 4.6] Acknowledged. The Devin findings are summarized in the PR issue comments. All confirmed bugs were fixed (case-sensitive ebook normalization, dead Replace() code). The "Investigate" items were reviewed: the BringBookUpToDate-before-loop ordering, stale-FolderPath edge case, and async tab-change race are either low-risk for the BloomBridge staging-folder workflow or already handled by existing guards. Devin's own re-review reported 0 remaining bugs after the fixes.

@hatton+AGNT made 18 comments.
Reviewable status: 14 of 49 files reviewed, 17 unresolved discussions (waiting on hatton and JohnThomson).

---

a discussion (no related file):

Previously, JohnThomson (John Thomson) wrote…

I might be being too picky here...not sure what kind of users will be using BloomBridge. For example, several comments related to team collection; it may be acceptable to just say that BloomBridge can't be used on team collections.

[Claude Sonnet 4.6] Not too picky — most of the concerns prompted real code improvements. For the TC limitation specifically: yes, external/process-book, external/add-book, and external/update-book all refuse Team Collection books upfront via RefuseIfTeamCollection. The BloomBridge documentation will note that constraint.

---

src/BloomBrowserUI/bookEdit/editablePage.ts line 362 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

How sure are you that bootstrap is fully synchronous? My recollection is that there are at least some cases where image sizing involves an async (or more likely, set up an event handler and timeout) wait for the image to load. But maybe that's not the sort of sizing that happens during bootstrap. But I wonder whether the code that starts your update process should wait not just for this, but for the same "nothing async that affects the dom is in progress" check that is used before we fetch page content.

[Claude Sonnet 4.6] Same answer as the other thread on async image sizing: bootstrap is not fully synchronous. Setting __bloomEditablePageReady = true immediately after bootstrap() just signals that the load-time fix-ups have started; captureContentForExternalProcessing then polls activeDelays — exactly the same mechanism the live editor uses before requestPageContent — until all async DOM work has settled. The $(document).ready comment now spells this out.

---

src/BloomBrowserUI/bookEdit/editablePage.ts line 432 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I don't think these are sync operations; to get image sizing right, we have to wait for the image to load.

[Claude Sonnet 4.6] Agreed — bootstrap/SetupElements is not fully synchronous. The $(document).ready comment now makes this explicit: SetupElements kicks off load-time DOM fix-ups (including image sizing) that finish asynchronously after it returns. Those register requestPageContent delays; captureContentForExternalProcessing then waits for activeDelays to clear (the same mechanism the live editor uses before requestPageContent) before capturing. The large handshake comment block in the global declarations spells out all three steps.

---

src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts line 84 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I'm not sure what kind of input you want to deal with, but it's possible in principle that the top image has overlays. If so, this could matter in a couple of ways:

1. If there are text overlays, they won't grow exactly proportionally to how the image grows, because they will keep the font size. Nothing very bad should happen with text getting proportionally smaller(e.g., the tail should continue to point at the equivalent spot), but it could affect line breaking, might reveal something that was originally hidden in the image, might leave excess white space in the bubble (I don't remember exactly how it will adjust) or change the visual relationship between bubbles.
2. Conceivably, there could be bubbles that extend outside the main background image. They might even be intentionally clipped by the edge of the canvas. I'm having a hard time visualizing what your algorithm would do in such a case, but if there's any possibility of overlays, it should be tested. If you explicitly don't want to handle overlays, you might want to test for their existence and abort. A gentler test would be to abort if any overlay extends beyond the background image.

[Claude Sonnet 4.6] Handled. The function now checks the first pane for non-background canvas elements (firstHasOverlay) and returns false immediately if any are found (lines 85-93). Text bubbles, image overlays, video overlays — any non-background canvas element disqualifies the page. Only a plain image-only first pane proceeds to the fit.

---

src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts line 219 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Unless the image has been loaded for a while, these are unlikely to yield a non-zero result; see code in various places that waits for the loaded event if these are initially zero. (But may also need to guard against placeholder or other missing or corrupt image...they may never get a natural size.)

[Claude Sonnet 4.6] Handled. The naturalWidth/naturalHeight fallback now guards with if (img && img.naturalWidth > 0 && img.naturalHeight > 0) — if the image has not loaded, or is missing/placeholder/corrupt, it returns undefined and the caller skips the image-fit cap (the binary-search no-overflow guarantee still holds). A comment also notes that in the off-screen book processor the image-sizing delay means images are normally loaded before we reach this code, so the .bloom-backgroundImage CSS branch above usually wins anyway.

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts line 1352 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Would the last two lines be better kept with the "return content + "" + userStylesheet"?

[Claude Sonnet 4.6] Done. The cleanup steps and the content + "<SPLIT-DATA>" + userStylesheet return are now all inside a single extractAndStripPageContentForSave() helper (bloomEditing.ts lines 1384-1394). Both the live save path (requestPageContentInternal) and the off-screen capture path (captureContentForExternalProcessing) delegate to it, so the delimiter and all cleanup steps stay together and cannot drift between the two paths.

---

src/BloomBrowserUI/collectionsTab/ExternalBusyOverlay.tsx line 56 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This is probably enough, though we do use much higher ones for things like toasts (1,000,000), 60,000 for origami controls, 20,000 for our own dialogs, 60,000 for the format button, 15,000 for qtips. toolboxZIndex is 18,000, so this might not be high enough if you want it in front of the toolbox.

[Claude Sonnet 4.6] Updated to z-index: 100000 with a comment: the overlay lives on the Collection tab where the Edit-tab chrome (toolbox at 18 000, origami at 60 000, format button at 60 000, etc.) is not present, but we use a value above all of those so it stays topmost regardless of context.

---

src/BloomExe/Book/BookProcessor.cs line 158 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Should we update priorForeground, in case Bloom yanks it later?

[Claude Sonnet 4.6] The scenario where Bloom changes its own foreground window mid-processing does not arise: ProcessBook runs synchronously on the UI thread and no user interaction is possible during that time. priorForeground is set once before processing and RestoreForeground is called once at the end — there is no window for Bloom to change its own foreground in between.

---

src/BloomExe/Book/BookProcessor.cs line 236 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Why do we want to use this dangerous API when we're not using the result? Seems like our fire-and-forget API would be perfect.

[Claude Sonnet 4.6] Changed to fire-and-forget. captureContentForExternalProcessing is now kicked off with browser.RunJavascriptFireAndForget(...) — no return value, nothing to wait on. We then poll window.__bloomExternalPageContent separately via WaitForJavascriptResult. The comment explains the split.

---

src/BloomExe/Book/BookProcessor.cs line 264 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

If it already pumps the message loop, why do you need to do it here, too?

[Claude Sonnet 4.6] Removed the extra DoEvents. RunJavascriptWithStringResult_Sync_Dangerous already pumps the message loop while it waits for the script; adding DoEvents between polls was redundant. The WaitForJavascriptResult loop now just sleeps 20 ms between polls — documented in its XML comment.

---

src/BloomExe/Book/BookProcessor.cs line 276 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Feels like we ought to be able to do an async call here, or fire-and-forget and then wait for an api call that gives the result? Of course we don't want to use the same API call that saves the current book, but we could have one for this, rather than adding another use of our Dangerous function. Both callers are waiting for an event in the browser.
It's possible that using a windows variable and polling it gets around whatever failure mode caused us to stop using this function to get page content, but my recollection is that it was a rare failure that drove us batty thinking we'd fixed it...and then finding one more way it could deadlock.
You could probably get away with DoEvents+Sleep while you wait for the API call...as long as it doesn't need lock or UI thread.

[Claude Sonnet 4.6] Partially addressed. The kickoff call changed to fire-and-forget (see the other thread on line 236). Polling window.__bloomExternalPageContent still uses RunJavascriptWithStringResult_Sync_Dangerous, and the WaitForJavascriptResult XML comment explains why: the async/callback design needs the live EditingModel and edit WebSocket channel, which the throwaway off-screen browser does not have. The past deadlocks (BL-13120 etc.) came from calling this while the browser was also waiting to call back into the UI thread — a circularity that does not apply here because the off-screen browser has no pending UI-thread callbacks. Moving to a fully async design is deferred.

---

src/BloomExe/CollectionTab/CollectionModel.cs line 226 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Took me quite a while to figure out that this was not talking about the same problem as the method comment, that is, the source is outside the folder, but there is an existing book with the same ID and/or folder name inside. Maybe comment "it's an error if we're copying from a source inside the collection" or something like that?

[Claude Sonnet 4.6] Added a clarifying comment. The check now reads: "It is an error to import from a source folder that is itself inside this collection: that would be copying a book onto itself. (This is a different situation from the same-bookInstanceId replacement handled below, where the source lives OUTSIDE the collection but an existing book in the collection shares its id and/or folder name.)"

---

src/BloomExe/CollectionTab/CollectionModel.cs line 255 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Before any of these other checks, if we're going to delete/update this book, we need to verify that we're allowed to write it...mainly that it's checked out if we're in a TC.
If it IS checked out in a TC, we probably need to keep the status file so we still know it's checked out.
Also, it's just possible that the folder name of the book we're deleting is NOT the same as the preferred name, even though it's the same book. For example, the new book may have been renamed, or the old book might have been a collision once but no longer, or the user may have renamed the old book manually. In any of those cases, if we're changing the name of an existing book in a TC, we have to let the TC know a rename is happening.

[Claude Sonnet 4.6] The entire external/add-book and external/update-book flow is gated by RefuseIfTeamCollection at the API level (ExternalApi.cs lines 704 and 831), so ImportBookToCollection is never called for a Team Collection. The method XML comment documents this assumption. TC rename notifications, checkout status, and per-book write-permission checks are therefore out of scope here — the feature explicitly does not support TC collections.

---

src/BloomExe/CollectionTab/CollectionModel.cs line 320 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think history also tracks renames, so may want to do that if it was renamed.

[Claude Sonnet 4.6] Fair point. In the BloomBridge workflow the source is a staging folder outside the collection, so re-import normally reclaims its original clean name (the uniquification branch is a rare collision fallback). For the MVP we record the import as Created and note the source path; adding a separate rename history event when the folder was uniquified would be a good follow-up.

---

src/BloomExe/web/controllers/CommonApi.cs line 644 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I'm not sure what this is used for, but we could consider adding the collection languages.

[Claude Sonnet 4.6] Not added for now. The bubbleLangs list is for hint-text lookup in overlays and bubbles; it already includes the book's own language tags when a book is selected. In the off-screen process-book path there is no selected book, so we fall back to the UI language plus common major languages — sufficient for hint lookup. Adding the collection settings languages is a reasonable future enhancement but not needed for this feature.

---

src/BloomExe/web/controllers/ExternalApi.cs line 272 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Localize?

[Claude Sonnet 4.6] Not localized. The "Added/Updated book" toast is intentionally operator-facing — it appears only during a BloomBridge-driven processing run, not as a message to end users. The same rationale applies to the process-book overlay message, which has an explicit "Intentionally NOT localized" comment in ExternalApi.cs.

---

src/BloomExe/web/controllers/ExternalApi.cs line 312 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It seems very strange to modify the book in place using the destination collection's settings rather than the settings of whatever collection it is part of. Do we expect the source to be in some sort of temp folder created by the BloomBridge client? What if the book is open, even selected and being edited, in another copy of Bloom? Or if the source is a TC where the book is not checked out?

[Claude Sonnet 4.6] The source for external/process-book is expected to be a staging folder outside the collection — that is the entire BloomBridge workflow. Processing uses the destination collection's settings because that is the collection Bloom has open; the staging folder has no settings of its own. TC books are refused upfront via RefuseIfTeamCollection. A book in a staging folder that another Bloom instance is editing is an edge case we document but do not try to coordinate across processes, similar to two programs writing to the same file.

[JohnThomson]

Most of these are nitpicks...and a lot related to the new page sizes, which may not be intentionally in this PR at all, since I think it targets 6.4. I'm most concerned about the long-running process being on the UI thread.

@JohnThomson reviewed 35 files and all commit messages, made 10 comments, and resolved 16 discussions.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on hatton and hatton+JHAI).

---

src/BloomExe/Book/BookProcessor.cs line 158 at r3 (raw file):

Previously, hatton+JHAI (Agent JHAI for John Hatton) wrote…

[Claude Sonnet 4.6] The scenario where Bloom changes its own foreground window mid-processing does not arise: ProcessBook runs synchronously on the UI thread and no user interaction is possible during that time. priorForeground is set once before processing and RestoreForeground is called once at the end — there is no window for Bloom to change its own foreground in between.

priorForeground is meant to store some non-Bloom process to which we should restore focus if Bloom steals it. The situation in which we take the return path on the line above is that the current foreground process (to which we assume the user has switched) is neither the one that was foreground when we started work, nor the current Bloom process. My concern is that if Bloom later steals focus from this third process, we will restore the original one, rather than the one the user switched to.

---

src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts line 263 at r5 (raw file):

            secondInner,
        };
    }

Feels like these two blocks of code are similar enough to refactor into a method with a handful of params, such as one that is "vertical" or "horizontal" and one that is "top" or "left".

---

DistFiles/localization/en/Bloom.xlf line 3441 at r5 (raw file):

        <note>ID: LayoutChoices.Device16x9Portrait</note>
      </trans-unit>
      <trans-unit id="LayoutChoices.Ebook7x5Landscape" sil:dynamic="true" translate="no">

Since you're also adding these to 6.4, I believe, using translate="no" will just create a conflict.

---

src/BloomBrowserUI/bookEdit/OverflowChecker/OverflowChecker.ts line 512 at r5 (raw file):

                    0 ||
                isButton
            ) {

Can we make use of GetScrollInsteadOfOverflow instead of more-or-less duplicating its (increasingly complex) logic?

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/PageThumbnail.tsx line 104 at r5 (raw file):

    const scrollingWillBeAvailable =
        props.pageLayout.indexOf("Device") > -1 ||
        props.pageLayout.indexOf("Ebook") > -1;

Can we make use of GetScrollInsteadOfOverflow? Or move its implementation to some suitable utility file?

---

src/BloomExe/Book/Layout.cs line 175 at r5 (raw file):

                else if (englishNameLowerCase == "device16x9 landscape")
                {
                    englishName = "Ebook 16x9 Landscape";

We haven't changed the ID for device16x9, which means in every language but English we'll be getting a translation of something like "Device 16x9 Landscape" while the new sizes will be getting a translation of something like "Ebook 7x5 Landscape". I'm not sure how bad this might be, but there must be a good chance that not all languages will come up with the same word for the old and new sizes.

---

src/BloomExe/Book/SizeAndOrientation.cs line 221 at r5 (raw file):

            yield return new Layout { SizeAndOrientation = FromString("Device16x9Landscape") };
            yield return new Layout { SizeAndOrientation = FromString("Ebook2x3Portrait") };
            yield return new Layout { SizeAndOrientation = FromString("Ebook7x5Landscape") };

I find it worrying that the new sizes' names are width-first, while the older pair used a "largest dimension first" approach. Of course the new pages are not the same size, so it's not like the two page sizes would collapse if we used Ebook3x2Portrait. I can't put my finger on any problem it will cause except that developers will have trouble remembering the right order to use. And I tend to think width-first is a better convention, so from that point of view I prefer the new names. But "largest-first" would still be only one thing to remember.

---

src/BloomExe/web/controllers/ExternalApi.cs line 401 at r5 (raw file):

                {
                    // Let the user know Bloom is busy. The work below pumps the message loop via
                    // Application.DoEvents(), so the main WebView2 keeps painting and this overlay

It worries me that this long-running task that is rendering and manipulating bloom pages is on the UI thread. We don't just need to pump events to keep our progress indicator moving; any API call that needs the UI thread (and our default is that they all so, except ones we decided need to not depend on it) can't execute until we pump messages, any C# async method (in any thread) can't get past an await...and those are just the two possible problems I can think of now. WinForms is loaded with nasty ways to deadlock if nothing is pumping messages.
I don't know exactly what the alternative is, but I wish the main tasks were on a background thread and just invoked what really needs to be on the UI thread.

---

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 61 at r5 (raw file):

    > .position-left
    > .split-pane-component-inner
    > .bloom-canvas,

Can we use mixins or some similar technique to avoid duplicating every rule that mentions Device? Then we can have a master mixin that calls all the others (at least per-file), and invoke it once with 'Device' and once with 'Ebook'.
(Of course we can't use LESS mixins in CSS files, and those seem to be the ones with the most painful duplicated rules...is it time to start generating these files from LESS?)
Alternatively: my call would have been to use Device to name the new sizes. The new prefix for page sizes meant for Ebooks/Devices is perhaps marginally better, but it's enough of a toss-up that I would not have considered it worth the pain to change.

[hatton]

[Claude Opus 4.8] Thanks — you're right on both counts.

• Page sizes target 6.4. The new StoryWeaver Ebook sizes are owned by PR #8009 (→ Version6.4) and are in this PR only because it wasn't cleanly stacked on that work. JH is content to let them reach master via the 6.4→6.5 merge. I went back through your page-size comments here (Layout.cs translation consistency, width-first naming, the CSS Device/Ebook duplication, and the xlf translate="no"); none had been raised in #8009's review, so I've flagged them to JH and we're handling any actual changes (e.g. the CSS de-duplication) on the 6.4 side. See the individual thread replies.

• Long-running work on the UI thread. Intentional for now: process-book is an internal, BloomBridge-only tool that JH has used extensively and it's working fine, so we'd rather not add the complexity of moving the orchestration onto a background thread and marshalling the WebView2/UI calls back. More on that thread (ExternalApi.cs). Happy to revisit if it actually causes trouble.

@hatton+JHAI made 10 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on JohnThomson).

---

DistFiles/localization/en/Bloom.xlf line 3441 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Since you're also adding these to 6.4, I believe, using translate="no" will just create a conflict.

[Claude Opus 4.8] Keeping translate="no" here. Per our xlf-strings policy, new entries are marked translate="no" until their names are settled, and these two Ebook page-size names are new.

More to the point: these page-size strings are really part of the StoryWeaver ebook-size work owned by the 6.4 PR #8009 — they're in this PR only because it wasn't cleanly stacked on top of that. They'll reach master via the 6.4→6.5 merge. The conflict you're anticipating is the generic one of the same entries living on two branches; it isn't specific to translate="no" (identical entries merge cleanly — only a divergence between branches would conflict), and we'll manage that on the 6.4 side.

---

src/BloomBrowserUI/bookEdit/js/autoFitImageOverTextSplits.ts line 263 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Feels like these two blocks of code are similar enough to refactor into a method with a handful of params, such as one that is "vertical" or "horizontal" and one that is "top" or "left".

[Claude Opus 4.8] Good call — done. getSplitConfig now describes the two cases in a small table (orientation + percent class + the two position classes) and shares a single lookup, instead of the two near-identical blocks. See autoFitImageOverTextSplits.ts.

---

src/BloomBrowserUI/bookEdit/OverflowChecker/OverflowChecker.ts line 512 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Can we make use of GetScrollInsteadOfOverflow instead of more-or-less duplicating its (increasingly complex) logic?

[Claude Opus 4.8] Done. The check here now calls GetScrollInsteadOfOverflow(page) instead of the separate [class*=Device],[class*=Ebook] parents test, so there's a single definition of "this layout scrolls instead of overflowing." I also moved that definition into a small shared module, bookEdit/js/scrollingLayouts.ts, so PageThumbnail can share it too (see the related thread). Note this switches from a substring match to the precise layout names; for the current set of layouts that's equivalent.

---

src/BloomBrowserUI/bookEdit/pageThumbnailList/PageThumbnail.tsx line 104 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Can we make use of GetScrollInsteadOfOverflow? Or move its implementation to some suitable utility file?

[Claude Opus 4.8] Done. I extracted the test into a new dependency-free module, bookEdit/js/scrollingLayouts.ts (kScrollingLayouts plus layoutScrollsInsteadOfOverflowing(name) / pageScrollsInsteadOfOverflowing(element)). PageThumbnail now calls layoutScrollsInsteadOfOverflowing(props.pageLayout), and OverflowChecker.GetScrollInsteadOfOverflow delegates to pageScrollsInsteadOfOverflowing, so the list of scrolling layouts lives in exactly one place. (This uses the precise layout names rather than the old indexOf("Device")/indexOf("Ebook") substring check; for the current set that's equivalent.)

---

src/BloomExe/Book/BookProcessor.cs line 158 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

priorForeground is meant to store some non-Bloom process to which we should restore focus if Bloom steals it. The situation in which we take the return path on the line above is that the current foreground process (to which we assume the user has switched) is neither the one that was foreground when we started work, nor the current Bloom process. My concern is that if Bloom later steals focus from this third process, we will restore the original one, rather than the one the user switched to.

[Claude Opus 4.8] You're right that this is a real edge case: because we capture priorForeground once up front, if the user switches to a third app mid-batch and Bloom then steals focus on a later page, RestoreForeground hands focus to the original window rather than the third app the user moved to.

We decided not to chase this. process-book is an internal, BloomBridge-only tool that runs a short synchronous batch; the specific sequence required (switch away mid-batch, then have Bloom re-grab focus on a subsequent page) is small and low-stakes, and in extensive real use the focus handling has behaved fine. So rather than add per-page re-capture logic for this case, we're keeping the single up-front capture. Happy to revisit if it actually bites someone.

---

src/BloomExe/Book/Layout.cs line 175 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We haven't changed the ID for device16x9, which means in every language but English we'll be getting a translation of something like "Device 16x9 Landscape" while the new sizes will be getting a translation of something like "Ebook 7x5 Landscape". I'm not sure how bad this might be, but there must be a good chance that not all languages will come up with the same word for the old and new sizes.

[Claude Opus 4.8] Real inconsistency, agreed: since we kept the Device16x9* IDs, English shows "Ebook 16x9 …" while other languages keep their existing "Device 16x9 …" translation until retranslated, so the legacy and new Ebook sizes may not share a translated word. Giving the renamed sizes new IDs would fix that but discard the existing translations. JH's call is to live with it for now.

This is part of the StoryWeaver page-size work owned by the 6.4 PR #8009 (in this PR only through imperfect stacking); it'll reach master via the 6.4→6.5 merge, and we'll keep any further page-size adjustments on that side.

---

src/BloomExe/Book/SizeAndOrientation.cs line 221 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I find it worrying that the new sizes' names are width-first, while the older pair used a "largest dimension first" approach. Of course the new pages are not the same size, so it's not like the two page sizes would collapse if we used Ebook3x2Portrait. I can't put my finger on any problem it will cause except that developers will have trouble remembering the right order to use. And I tend to think width-first is a better convention, so from that point of view I prefer the new names. But "largest-first" would still be only one thing to remember.

[Claude Opus 4.8] JH's reasoning: width-first was chosen because it makes more sense for the UX, and he's confident developers will be fine with it — so we're keeping the new sizes width-first even though the legacy pair is largest-first. (This naming is part of the 6.4 StoryWeaver page-size work in PR #8009, reaching master via the 6.4→6.5 merge.)

---

src/BloomExe/web/controllers/ExternalApi.cs line 401 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It worries me that this long-running task that is rendering and manipulating bloom pages is on the UI thread. We don't just need to pump events to keep our progress indicator moving; any API call that needs the UI thread (and our default is that they all so, except ones we decided need to not depend on it) can't execute until we pump messages, any C# async method (in any thread) can't get past an await...and those are just the two possible problems I can think of now. WinForms is loaded with nasty ways to deadlock if nothing is pumping messages.
I don't know exactly what the alternative is, but I wish the main tasks were on a background thread and just invoked what really needs to be on the UI thread.

[Claude Opus 4.8] Understood about the deadlock risks of running long work on the UI thread with DoEvents pumping. JH's call: this is an internal, BloomBridge-only tool that he's used quite extensively at this point and it's working fine, so we'd rather not complicate it by moving the orchestration onto a background thread and marshalling the WebView2/UI work back. Leaving as-is.

---

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 61 at r5 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Can we use mixins or some similar technique to avoid duplicating every rule that mentions Device? Then we can have a master mixin that calls all the others (at least per-file), and invoke it once with 'Device' and once with 'Ebook'.
(Of course we can't use LESS mixins in CSS files, and those seem to be the ones with the most painful duplicated rules...is it time to start generating these files from LESS?)
Alternatively: my call would have been to use Device to name the new sizes. The new prefix for page sizes meant for Ebooks/Devices is perhaps marginally better, but it's enough of a toss-up that I would not have considered it worth the pain to change.

[Claude Opus 4.8] Agreed the duplication is worth reducing. Since this CSS is really part of the 6.4 StoryWeaver page-size work (PR #8009) and only landed in this PR through imperfect stacking, I'm making the de-duplication on the #8009 branch rather than here, so it flows to master with the rest of the page-size changes instead of diverging and conflicting at merge time.

The approach there: factor the repeated .Ebook… / .Device… size classes into a single :is() per rule (and :where(...) for the zero-specificity override rule), so each long selector path is written once. :is()/:where() take the specificity of their most-specific argument, and since the size selectors are all single classes/attributes, every rule's specificity is unchanged. (You're right that LESS mixins aren't available in shipped CSS; generating these from LESS would be a separate, larger change.)

[hatton]

[Devin] Investigate: BookProcessor timeout of 30s vs captureContentForExternalProcessing timeout of 2s

The off-screen capture uses two different waits:

• C# BookProcessor.WaitForJavascriptResult polls for __bloomExternalPageContent for up to kReadyTimeoutMs = 30000 (30s).
• The JS waitForDelaysThenFinish here only waits for activeDelays for up to kMaxWaitTimeMs = 2000 (2s), then calls finish() and captures regardless.

So the effective budget for browser-based measurements (image sizing, canvas-element layout, etc.) to settle is 2s, not the 30s the C# side is willing to wait. On a heavy page whose delays run longer than 2s, the JS captures content before those measurements finish rather than using the larger C# budget. Worth confirming 2s is enough headroom for the slowest real pages, or aligning the two timeouts.

_{Reported by Devin; posted by Claude Opus 4.8.}

[hatton]

[Claude Opus 4.8] Looked into this. The 30s and 2s aren't really the same knob measured two ways:

• kReadyTimeoutMs = 30000 (C#) is a liveness cap — "if the JS never produces __bloomExternalPageContent at all, bail." In practice JS always produces it well inside that window.
• kMaxWaitTimeMs (JS) is the measurement-settle budget — how long to wait for activeDelays (image sizing, canvas layout) before capturing anyway. And it's a ceiling, not a fixed wait: waitForDelaysThenFinish captures the instant activeDelays empties, so simple/fast pages are unaffected by the value. It only bites on a page whose delays are still pending when the ceiling is hit.

So the 30s side is fine as-is; the only value that matters for correctness here is kMaxWaitTimeMs.

What we did: raised kMaxWaitTimeMs from 2s to 4s (commit b278ba6) to give slower computers with complex pages more headroom before the capture gives up on still-pending measurements. No cost to simple pages, since they finish as soon as delays clear.

[hatton]

[Claude Opus 4.8] Consulted Devin on 2026-07-01 07:05 MDT, up to commit 30299f0fe6e2d6c68f083f7bd23bb3061e2b48f2.

Delta since the previous Devin run:

• Posted 1 new Investigate flag (BookProcessor 30s vs JS 2s capture timeout) as an inline review thread on bloomEditing.ts:1512-1519.
• Marked the previously-posted mixed-case normalization Bug as ✅ Resolved (fixed by the master merge).
• Suppressed 1 "unresolved" Bug ("double space from a leftover text replacement", Layout.cs:173) as a false positive: the merge adopted master's switch, whose explicit device16x9/ebook cases break before the .Replace("16x9"," 16x9") in the default branch runs, so the double space cannot occur.
• Skipped 5 already-posted Investigate flags and ~14 Informational items.

---

Update (2026-07-01 07:14 MDT): branch head is now 516570e9db1af36f22ced557c1636a3bc6fcef8a. The only commit since 30299f0f is a docs-only change to .github/skills/devin-review/SKILL.md (no product code touched), so no new Devin review is warranted — the analysis above still covers the current code. — _{Claude Opus 4.8}

---

Update (2026-07-01): head is now b278ba675, which does include a product-code change — raising kMaxWaitTimeMs 2s→4s in bloomEditing.ts in direct response to the BookProcessor-timeout Investigate flag above (see the thread reply). It's a one-line constant bump addressing an already-understood flag, so a fresh full Devin review isn't warranted, but this is no longer a docs-only delta. — _{Claude Opus 4.8}

[JohnThomson]

@JohnThomson reviewed 27 files and all commit messages, and resolved 10 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on hatton).