Shrink large images, reporting wherever the caller already reports (BL-16646) by StephenMcConnel · Pull Request #8154 · BloomBooks/BloomDesktop

StephenMcConnel · 2026-08-05T20:38:20Z

MigrateToMediaLevel1ShrinkLargeImages shrinks images larger than 3840x2800 the first time an old book (pre-4.9, still at mediaMaintenanceLevel 0) is brought up to date, and showed its own "Updating Image Files" progress dialog while it worked. Several of the operations that bring a book up to date run on a background thread, and WinForms only allows a Form to be created on the UI thread — so on an old book with an oversized image, those paths could hang or leave a stray window.

The fix decides where to report based on whether the caller already has somewhere real to report, and whether we could legally put up a dialog at all:

The caller has a real progress — Do Updates of All Books, .bloomSource and spreadsheet import, the publish previews, and the single-book "Update Book" command. Its messages go there, so they land in the window the user is already watching and we add nothing of our own.
All the caller has is a NullProgress, and we are on the UI thread — most importantly clicking a book to select it. There is nowhere to report, and doing the work inline would freeze Bloom, so we put up our own dialog, which is legal here and which runs the work on a background worker so the app stays responsive.
We could not show a dialog even if we wanted to — off the UI thread (WinForms forbids it), headless (no window exists), or under test. The caller's progress is used, or a NullProgress if it passed none.

MigrateToMediaLevel1ShrinkLargeImages gained an optional IProgress parameter for this, passed at Book.cs:1110 and Book.cs:1898. Only tests rely on the default — both production call sites pass something non-null, since Book.EnsureUpToDate substitutes a NullProgress for a null argument.

How a failed shrink is handled

This took several passes to settle, so here is the whole of it in one place.

Originally a failure inside the dialog's background worker vanished — ProgressDialogBackground never reads RunWorkerCompletedEventArgs.Error — and we set mediaMaintenanceLevel to 1 anyway, permanently, since the level is never revisited. That book's images stayed oversized forever with nothing reported.

Now, in both branches:

The failure is caught and reported passively via NonFatalProblem.Report(ModalIf.None, PassiveIf.All, …): a toast, plus the whole exception to the log and Sentry. It is passive on purpose — the book still works, its pictures are merely left large, and we will try again — so it is not worth interrupting the user for.
The failure is also written to the caller's progress when it has one, so an operation already showing a progress box does not appear to have finished cleanly. This deliberately uses WriteWarning, not WriteError: ProgressDialogForeground pops a modal "There was a problem performing that operation" whenever its progress records an error, which would defeat the point of reporting passively.
The user-visible text is localized (ImageUtils.ShrinkingImagesFailed, in BloomLowPriority.xlf as a rarely-seen failure message). The book's folder path goes in the details that reach the log and Sentry, not in the sentence the user reads.
mediaMaintenanceLevel is bumped only on success, so the shrink is retried the next time that book is brought up to date. Retrying is safe: FixSizeAndTransparencyOfImagesInFolder only resizes a file whose current dimensions differ from the desired ones, so images shrunk on an earlier attempt are skipped rather than re-encoded.
The retry does not happen twice in one pass. Book.EnsureUpToDate calls this migration twice — once by way of EnsureUpToDateMemory and once directly afterwards — and the second call was a no-op only because the first had always bumped the level. A flag on the BookStorage remembers the failed attempt, so one pass makes one attempt.

The consequence worth a reviewer's eye: a bad image now costs that one book's shrink rather than the rest of the operation. Previously an exception from the direct branch abandoned the remaining twelve migrations and the Save, and for Do Updates of All Books silently ended the whole batch at the first bad image.

Also fixed along the way: Shell.GetShellOrOtherOpenForm() returns null in the headless CLI, and the InvokeRequired test would have thrown a NullReferenceException there — on exactly the old-book-with-a-huge-image case this ticket is about.

Supporting change: WebProgressAdapter now accepts the IWebSocketProgress interface rather than the concrete WebSocketProgress, so MakeDerivativeFromBloomSourceFile can forward the progress it is handed.

Notes for the reviewer

The branch has been through several designs; the history shows marshal-the-dialog, delete-the-dialog-entirely, and two shapes of hybrid. Some resolved Devin threads on this PR discuss code that is no longer here, and each is annotated to say so.

One consequence worth a look: when the caller has a real progress and happens to be on the UI thread, the shrink runs inline on that thread rather than on a background worker. Today the only such caller is "Update Book", which runs under ProgressDialogForeground and so already does the whole of BringBookUpToDate on the UI thread — its progress pumps Application.DoEvents on every message, so the dialog keeps painting and nothing new freezes. It does mean a future UI-thread caller passing a live progress that does not pump would block the UI for the whole shrink. The invariant is written down in a comment at the branch.

Known limitations, recorded rather than fixed here, and now tracked as BL-16663:

FixSizeAndTransparencyOfImagesInFolder has no per-file try/catch, so one unreadable image aborts its loop at the same point on every attempt and the images after it are never shrunk. The retry above therefore cannot get past such a file.
Its most likely failures do not throw at all — a missing or failing GraphicsMagick just returns false, and one of the two call sites ignores that — so those books are marked migrated with oversized images. The success flag above only catches failures that throw.

Both fixes change a method with other callers, so they need their own change and test pass.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16646

Devin review

This change is

MigrateToMediaLevel1ShrinkLargeImages shows an "Updating Image Files" ProgressDialogBackground when a book still has images larger than 3840x2800. Several of the operations that bring a book up to date run on a background thread -- Do Updates of All Books, the BloomPub/ePUB publish preview, .bloomSource and spreadsheet import, the bulk-upload CLI -- and WinForms does not allow a Form to be created off the UI thread. So on an old book that still has an oversized image, those paths could hang, throw, or leave a stray dialog. Marshal the dialog onto the UI thread via Shell.GetShellOrOtherOpenForm() and Invoke, the same pattern already used in AudioRecording, NonFatalProblem, and OneDriveUtils. When no form is open at all -- the headless CLI, which has no message loop -- do the work directly against a NullProgress instead of trying to show a modal dialog that nobody could see or dismiss. The dialog body moves unchanged into a new UpdateImagesWithProgressDialog helper. mediaMaintenanceLevel is still bumped in exactly the same place for every branch, and the Program.RunningUnitTests special case is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…(BL-16646) The new helper is the one piece of this code that must not be called off the UI thread, which is the whole point of the branch above it. Say so where someone editing the helper will see it, per our convention of commenting private methods. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-05T20:42:37Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-05 up to commit 462f97734351b1f447e1b934af86bc8a129a708d.

It found no bugs. It raised one thing worth investigating — whether the blocking Invoke could deadlock rather than just stack a dialog — which we checked path by path and resolved as not an issue; see that thread for the reasoning.

Two further items were informational and not mirrored as threads, but both bear on the design question raised in the PR description, so they are worth a reviewer's eye: that InvokeRequired reports false for a form whose window handle isn't created yet (a known rough edge of this idiom, shared with the existing call sites in AudioRecording and OneDriveUtils), and that the headless/bulk-upload path now shrinks images with a NullProgress, so a slow run produces no console diagnostics — something the card's IProgress alternative would have preserved.

CI (pr-automation) is green.

… (BL-16646) MigrateToMediaLevel1ShrinkLargeImages used to put up its own "Updating Image Files" ProgressDialogBackground. Several of the operations that bring a book up to date run on a background thread, and WinForms does not allow a Form to be created there, so on an old book that still had an image larger than 3840x2800 those paths could hang, throw, or leave a stray dialog. Rather than teach the dialog which thread it is on, remove it. The migration now takes the IProgress its caller already has and hands that to ImageUtils.FixSizeAndTransparencyOfImagesInFolder, which has always accepted one. With no Form there is no thread rule left to break, and this moves with the conversion to React instead of adding to what has to be unwound later. Each caller now gets the progress it already had: Do Updates of All Books and the .bloomSource import and publish-preview paths report into the progress dialog they had already opened, while headless callers such as the bulk-upload CLI pass NullProgress and stay silent rather than trying to show a modal that nobody could see or dismiss. The user still gets per-image feedback either way, because the resize itself writes the already-localized "Preparing image: {0}" status. The Program.RunningUnitTests special case goes away with the dialog. It only ever existed to dodge the modal, and both of its branches now collapse into the same single call. Also widen WebProgressAdapter to accept IWebSocketProgress rather than the concrete WebSocketProgress, so MakeDerivativeFromBloomSourceFile can forward the progress it is handed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…-16646) Removing the dialog left two comments describing behavior that no longer happens: the block comment in MigrateToMediaLevel1ShrinkLargeImages still said a progress dialog pops up when images must be shrunk, and the method's doc comment said nothing about the new IProgress parameter at all. Say what actually happens instead, and document the parameter -- including that headless callers pass NullProgress and so report nothing, and that it must not be null because ImageUtils.FixSizeAndTransparencyOfImagesInFolder dereferences it. (Book.EnsureUpToDate already substitutes a NullProgress for a null argument, which is what keeps every current caller safe.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-05T22:03:40Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-05 up to commit bb09b0d61735fc33b96f930b65ae7b79a9ad9358.

This was a re-review after the branch was reworked to delete the progress dialog rather than marshal it onto the UI thread. No bugs. Three things needed investigating and four were informational.

The one that matters is the loss of feedback on the NullProgress call sites. I verified it and it is real — on book selection the shrink now runs on the UI thread with nothing to report to, so Bloom freezes where the old dialog kept it responsive and showed a bar. That thread is deliberately left open: deciding where those call sites should get a progress channel is a product call, so it has gone to the developer.

Of the rest, the stale PR description was a fair catch that had already been fixed earlier in this run, and is resolved. Devin also re-raised the blocking-Invoke deadlock question against code the rework deleted; that is the existing thread from the previous run, already resolved and annotated to say the code is gone, so it was not posted again.

The four informational items were not mirrored: a missing null guard on the new IProgress parameter (verified unreachable — Book.EnsureUpToDate substitutes a NullProgress at Book.cs:1068, and every other caller passes one explicitly), migration messages now surfacing in the import dialog (intended), a doc comment still describing the removed dialog (fixed in this very commit, so Devin's copy is stale), and the InvokeRequired-before-handle-creation edge (moot, that code is deleted).

CI (pr-automation) is green.

…L-16646) Design 2 removed the "Updating Image Files" dialog entirely and reported through the caller's IProgress. That left the callers holding only a NullProgress with no feedback at all, most visibly book selection. This reinstates the dialog for the callers that have nothing better, while letting the ones that own a real browser progress (WebProgressAdapter) keep reporting into it, and keeps the direct call for unit tests. Also guard against a null shell. Shell.GetShellOrOtherOpenForm() returns null when no window is open at all -- the bulk-upload and hydrate CLI commands -- and the InvokeRequired test would have thrown a NullReferenceException there, on exactly the old-book-with-a-huge-image case this ticket is about. A null shell now takes the direct path with whatever progress the caller handed us. Note for review: which branch each caller lands in is keyed off shell.InvokeRequired, and that mapping needs a second look before this merges -- see the preflight report on the card. InvokeRequired is true when we are NOT on the UI thread, so as written the dialog is constructed on background threads and the synchronous path runs on the UI thread, which is the opposite of what either symptom needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-05T22:57:20Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-05 up to commit 7b14a6d5ab1b833ea75fa79c39505be9049da313.

This was a re-review after a third design landed — a hybrid that reinstates the "Updating Image Files" dialog for callers with no browser progress. Devin found one bug and raised five things to investigate.

The bug is the important one: the thread test that picks between the dialog and the direct call is inverted, so the dialog is created on background threads and the silent path runs on the UI thread — the opposite of what either symptom needs. I had reached the same conclusion before Devin filed it, and a local review agent verified the WinForms semantics with an actual probe rather than from memory; that thread carries the caller-by-caller breakdown and two knock-on consequences (a failed shrink still being recorded as migrated, and progress is WebProgressAdapter not reliably meaning "this caller already shows progress"). Left open — inverting the test changes what users see on several paths, so it is the developer's call.

Also left open, both tied to that same decision: the comments that still describe the previous design (I wrote two of them last run, when they were true), and the lambda parameter that shadows the new progress parameter so the caller's progress is dead on the dialog path.

The stale PR description was a fair catch and is fixed and resolved — it had fallen a design behind for the second time.

Two flags were not posted again because they duplicate existing threads. Devin's "silent shrink for NullProgress callers on the UI thread" is the same finding as the still-open freeze thread from the previous run, which has been updated with the current status rather than forked into a second thread. And the blocking-Invoke deadlock flag continues to be raised against code that no longer exists anywhere on the branch; that thread is already resolved and annotated to say so.

Five informational items were assessed and not mirrored. Two are worth a reviewer's eye and are carried into the report: that the WebProgressAdapter test is a fragile proxy for "has visible progress", and that InvokeRequired reads false for a form whose window handle is not yet created — which matters more now that InvokeRequired is what selects the branch. The others: the null-guard concern on the new IProgress parameter (verified unreachable — Book.cs:1068 substitutes a NullProgress), migration messages surfacing in the import dialog (intended), and a doc-comment observation superseded by the thread above.

CI (pr-automation) is green, and the full C# suite passes at this commit: 3022 passed, 0 failed, 13 skipped.

The thread test that chose between our own progress dialog and the caller's IProgress was inverted: InvokeRequired is true when we are NOT on the UI thread, so the dialog was being created on background threads -- the original bug -- while the silent inline path ran on the UI thread, where it froze Bloom with no feedback. Swap the two. Off the UI thread we hand our messages to the progress the caller already owns, since anything that got here from a background thread is running under some progress of its own; on the UI thread there is nothing to borrow and inline work would freeze Bloom, so we put up our own dialog, which does the work on a background worker. Book selection now gets that dialog instead of a silent freeze, and Do Updates of All Books reports into the progress dialog it already has open instead of stacking a second one on top. The shell.Invoke that briefly wrapped the dialog is gone with the swap rather than inverted along with it: the dialog branch is now only reached when InvokeRequired is false, so we are already on the shell's thread and there is nothing to marshal. Also stop recording a book as migrated when the shrink failed. ProgressDialogBackground never reads RunWorkerCompletedEventArgs.Error, so an exception thrown by the work vanished and we carried on to set mediaMaintenanceLevel to 1 -- permanently, because the level is never revisited, so that book's images would stay oversized forever with nothing reported. The work now runs inside a try/catch whose exception is rethrown on the calling thread with ExceptionDispatchInfo, which reports the failure and leaves the level alone, matching what the direct path has always done. The dialog body moves into ShrinkImagesBehindProgressDialog, whose lambda parameter is named dialogProgress rather than shadowing the method's own progress parameter -- the shadowing made it look as though the caller's progress was being honored on that path when it was not. Comments in BookStorage and BookProcessor that still described the interim design (no dialog at all) now describe this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-05T23:29:00Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-05 up to commit 47ab0d5dc7309f4a589ce85ad0569ecc3ab44586.

This is the re-review after the developer's decisions were implemented: the thread test was swapped, a failed shrink no longer records the book as migrated, the lambda parameter was renamed, and the comments were brought in line.

Devin's own output confirms the main fix landed — one of its informational items reads "The 'inverted thread test' described in the PR body is not present in this head commit." Its bug list still carries the wrong-thread bug and five Investigate flags from earlier commits on this branch; every one of those already has a thread here that was replied to and resolved this run, so they were not posted again:

the inverted thread test — fixed and resolved
the UI freeze on NullProgress callers — fixed and resolved; book selection is now the case that gets the dialog, and the dialog runs the work on a background worker, so it neither freezes nor goes silent
the stale comments and the shadowed lambda parameter — both fixed and resolved
the stale PR description — resolved earlier; the description has since been rewritten again for the settled design
the blocking-Invoke deadlock flag — still raised against code that is not on the branch; that thread remains resolved and annotated

One genuinely new flag, and a fair one: image-shrink failures now abort the book update instead of being swallowed. That is the intended consequence of the developer's decision not to mark a book migrated when the shrink failed, and the selection path already has a try/catch that enriches and rethrows into Bloom's error reporting — so it surfaces as a reported error naming the book rather than an unhandled crash. Replied with the reasoning and resolved.

One informational item was left as accepted rather than fixed, and is written up in the PR description as a known limitation: progress is WebProgressAdapter is a fragile proxy for "this caller already shows progress". In practice the callers it misclassifies are all on background threads and take the caller's-progress path regardless.

CI (pr-automation) is green. On the full C# suite, one run reported 9 failures and two subsequent runs at the identical tree were fully green (3022 passed, 0 failed, 13 skipped); the failing run's detail was lost to an output filter on my side, so those 9 are recorded as non-reproducible rather than diagnosed. This repo has a known history of intermittent C# failures.

Make the progress parameter optional, defaulting to null, and substitute a NullProgress when we use it directly. Only the unit tests actually rely on the default: both production call sites already pass something non-null, because Book.EnsureUpToDate substitutes a NullProgress for a null argument. This lets the tests read as what they are -- calls that do not care about progress. Then make the dialog decision match what the doc comment claims. Previously the choice was made purely on the thread, so a caller already on the UI thread got our "Updating Image Files" dialog even when it had a perfectly good visible progress of its own. The single-book "Update Book" command is exactly that case: it runs under ProgressDialogForeground, whose work runs on the UI thread via an Application.Idle handler, so it was getting a second progress dialog stacked inside the one already on screen, and the foreground dialog it had open was written to not at all during the shrink. That was a regression introduced two commits ago when the thread test was swapped -- the commit before it took the direct path here and reported into the existing dialog. So decide on whether there is anywhere real to report, not only on the thread. A NullProgress reports nowhere, so having one is equivalent to having none, and on the UI thread it still earns a dialog; anything else is somewhere real and gets used. Book selection, which is what BL-16646 is really about, is unaffected: it arrives with a NullProgress substituted by EnsureUpToDate, so it keeps its dialog and stays responsive. Exactly one caller changes behavior, and it is the one that was wrong. This also retires the "progress is WebProgressAdapter" test, which was a narrow proxy for this same question -- a WebProgressAdapter is simply one kind of real progress, so it is now covered by the general case rather than named specially. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-06T17:17:23Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-06 up to commit e8ccafa0a0a6faae13c933a7116ada1c0fa259eb.

No new bugs, and no new review threads were needed: Devin's bug and Investigate lists are entirely carried-over findings from earlier commits on this branch, and every one of them already has a thread here that was replied to and resolved. Its own informational items confirm as much — one notes that the "inverted thread test" described in an earlier PR body is not present in this head commit.

Two informational items were new and worth recording.

The one that matters. On the UI thread with a real progress, the shrink now runs inline rather than on a background worker. That is a direct consequence of this commit, which stopped choosing purely on the thread and started asking whether the caller has anywhere real to report. Devin audited the callers and found the only UI-thread caller supplying a non-null progress is CollectionModel.BringBookUpToDate ("Update Book"), which uses ProgressDialogForeground and already runs the whole of BringBookUpToDate on the UI thread — so nothing new freezes. A local review agent reached the same conclusion independently, adding that the MultiProgress there includes ApplicationDoEventsProgress, which pumps on every message, so the dialog keeps painting. Both flag the same forward-looking caveat: a future UI-thread caller passing a live progress that does not pump would block the UI for the whole shrink. Recorded in the PR description as a note for the reviewer rather than mirrored as a thread, since it is informational and not a defect today.

The PR description was stale again — it still described the progress is WebProgressAdapter test as a known limitation, and this commit retired that test in favour of the general "is there anywhere real to report?" question. Rewritten, along with the title. The existing description-mismatch thread stays resolved rather than being reopened for the same class of problem a third time.

Worth noting what this commit fixes, since it was a regression of my own making: the swap two commits ago moved the single-book "Update Book" command onto the dialog path, so it was stacking an "Updating Image Files" dialog inside the ProgressDialogForeground it already had on screen and writing nothing to the foreground dialog during the shrink. The commit before the swap did not do that. Deciding on "is there anywhere real to report" rather than on the thread alone puts that caller back to reporting into its own dialog, and leaves book selection — the case this ticket is actually about — on the dialog path, where it stays responsive.

CI (pr-automation) is green, and the full C# suite passes at this commit: 3034 passed, 0 failed, 13 skipped.

Using the caller's progress also means doing the shrinking synchronously on the caller's thread. That is fine for every caller today, but it puts an unwritten requirement on any future one that is on the UI thread: its progress has to pump messages, or Bloom freezes for the whole shrink. Write that down where someone adding such a caller will see it, along with why the one UI-thread caller we do have is safe -- ProgressDialogForeground runs all of BringBookUpToDate on the UI thread anyway, and its MultiProgress includes an ApplicationDoEventsProgress that pumps on every message. Comment only; no behavior change. Devin raised the consequence as an informational item and the developer chose to record the invariant rather than add machinery for a caller that does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…6646) When the shrink runs behind our own "Updating Image Files" dialog, a failure in the background worker was captured and rethrown on the calling thread. That did keep mediaMaintenanceLevel at 0, so the book was not falsely recorded as migrated -- but it also aborted the rest of bringing the book up to date (migrations 2-13 and the Save that follows), for a failure on a single image. Report the failure and let the rest of the update continue: the helper now returns false, and the caller only bumps mediaMaintenanceLevel when the work actually succeeded, so the shrink is retried the next time this book is brought up to date. Retrying is safe -- FixSizeAndTransparencyOfImagesInFolder only resizes a file whose current dimensions differ from the desired ones, so images shrunk on the earlier attempt are skipped rather than re-encoded. Log the whole exception via Logger.WriteError (the idiom already used five times in this file) rather than just its Message. The likeliest failure is a TagLib error on one particular image, where the message alone typically names neither the file nor the inner exception. Also drop the now-unused System.Runtime.ExceptionServices using, and correct the helper's remarks, which still described the rethrow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…6646) Book.EnsureUpToDate calls MigrateToMediaLevel1ShrinkLargeImages twice: once by way of EnsureUpToDateMemory, and once directly afterwards. That was harmless only because the first call always bumped mediaMaintenanceLevel, so the second returned at the level check. Now that a failed shrink deliberately leaves the level at 0, the second call would scan the folder and start the whole slow shrink over again, put up the "Updating Image Files" dialog a second time, and fail the same way -- the cause (a TagLib error on one image, say) is deterministic. So a book with one bad image would take twice the work and flash the dialog twice, with nothing explained to the user. Remember the failed attempt in a field on the BookStorage, so it happens once per pass. The level still stays at 0, so the shrink is still retried the next time that book is loaded and brought up to date; we just do not do it twice over in one pass. Found by Devin's review of the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-06T18:27:21Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-06 up to commit f81e1b815e1c7cdeaa0f93926b1c0843808e6217.

This was the re-review after the developer changed a failed shrink from rethrowing to logging and carrying on. Devin earned its keep this round: it found one real bug in that change and it is now fixed.

A failed shrink was retried immediately, in the same update pass — thread. Book.EnsureUpToDate calls this migration twice, and the second call was harmless only while the first always bumped mediaMaintenanceLevel. Once a failure left the level at 0, the second call redid the whole slow shrink and put the dialog up a second time before failing again. Fixed in f81e1b815; Devin's re-review marks it resolved.
The direct branch still throws where the dialog branch now logs — thread, left open for the developer. It is the one thing on this PR still awaiting a human decision.
The PR description no longer matched the code — thread. Correct; the description has been rewritten.
"Silent shrink for NullProgress callers on the UI thread" — thread. Not right about the current code; it has the branches the other way round. Explained and resolved.

Its remaining four Investigate flags and both listed bugs are carried-over findings from earlier commits on this branch — every one already has a thread here that was replied to and resolved, and Devin now marks both bugs resolved. Eleven further items were informational; none needed action.

The full C# suite is green at this commit: 3040 passed, 0 failed, 13 skipped.

Until now a failed shrink was reported only to the log, and only on the branch that puts up our own dialog; the direct branch let the exception propagate, which abandoned the remaining twelve migrations and the Save, and for "Do Updates of All Books" silently ended the whole batch at the first bad image. Both branches now catch the failure and report it the same way, so a bad image costs that one book's shrink rather than the rest of the operation. The report is a passive NonFatalProblem: the book still works, its pictures are merely left large, and mediaMaintenanceLevel is left at 0 so the shrink is retried the next time the book is brought up to date -- none of which is worth interrupting the user for. NonFatalProblem also logs the whole exception and reports to Sentry. Two details worth recording: The user-visible text is localized (ImageUtils.ShrinkingImagesFailed, in BloomLowPriority.xlf since it is a rarely-seen failure message) rather than an interpolated string, and it no longer leads with the book's folder path -- that belongs in the details that go to the log and Sentry, not in the sentence the user reads. This follows the sibling toast strings in that file. The failure is also written to the caller's progress when it has one, so an operation that is already showing the user a progress box does not appear to have finished cleanly. It deliberately uses WriteWarning, not WriteError: ProgressDialogForeground pops a modal "There was a problem performing that operation" whenever its progress records an error, which would defeat the point of reporting this passively. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-06T19:45:49Z

[Claude Opus 5 (1M context) during preflight] Consulted Devin on 2026-08-06 up to commit 4e0324264af2b388e00b6035511294392e0ae54b.

This was the re-review after the developer settled the open question from the last round: a failed shrink is now caught on both branches and reported passively, so a bad image costs that one book's shrink rather than the rest of the update.

One new flag, and it was correct — thread: the PR description still described the old behavior, where the direct branch threw. Devin also asked whether the semantic change was intended. It is, and it is the point rather than a side effect — before this, one unreadable image ended a whole Do-Updates-of-All-Books run with nothing shown. Description rewritten; thread replied to and resolved.
The previously open thread is now closed too — thread. It asked for exactly this fix, the developer chose it, and the outcome is recorded there.

Its other seven Investigate flags and both listed bugs are carried-over findings from earlier commits on this branch — each already has a thread here that was replied to and resolved, and Devin marks both bugs resolved. Twelve further items were informational; none needed action.

There are now no open review threads on this PR.

The full C# suite is green at this commit: 3031 passed, 0 failed, 13 skipped. That count is lower than the 3040 reported for the previous commit because master has since made dotnet test exclude the Integration and Nightly categories by default; it matches the figure in that commit's own message.

StephenMcConnel and others added 3 commits August 5, 2026 14:35

Merge remote-tracking branch 'origin/master' into BL-16646-HugeImageHang

462f977