Enhancements to custom pages (BL-15948) by JohnThomson · Pull Request #7766 · BloomBooks/BloomDesktop

JohnThomson · 2026-03-24T15:55:43Z

Add format dialog for read-only fields on custom pages
Remove cover-is-image book setting and migrate to custom layout
Keep image selected after "become background" command
Make hint bubbles work on custom pages

This change is

devin-ai-integration

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

src/BloomExe/Book/BookStorage.cs

devin-ai-integration · 2026-03-24T16:05:00Z

src/BloomExe/Book/BookStorage.cs

+                    if (
+                        outsideFrontCover.HasClass("bloom-customLayout")
+                        && !string.IsNullOrEmpty(
+                            outsideFrontCover.GetAttribute("data-custom-layout-id")
+                        )
+                    )
+                    {
+                        var onePageDom = new HtmlDom("<html><head></head><body></body></html>");
+                        onePageDom.Body.AppendChild(
+                            onePageDom.RawDom.ImportNode(outsideFrontCover, true)
+                        );
+                        bookData.SuckInDataFromEditedDom(onePageDom, BookInfo);
+                    }


🚩 MigrateToLevel14 can dereference null bookData when coverIsImage was present with data-custom-layout-id

At BookStorage.cs:4610, bookData.SuckInDataFromEditedDom(onePageDom, BookInfo) is called but bookData is a nullable parameter (default null). If the outsideFrontCover has both bloom-customLayout class (just added on line 4595) AND a data-custom-layout-id attribute, and bookData is null, this would throw a NullReferenceException. In practice, the call from Book.cs:1872 always passes _bookData, but the method signature allows null. The condition on line 4600 (outsideFrontCover.HasClass("bloom-customLayout")) is always true since line 4595 just added that class, so the only real gate is data-custom-layout-id. For a standard cover-is-image page, the xmatter template might or might not set this attribute. If it does and a caller passes null bookData, this crashes.

Was this helpful? React with 👍 or 👎 to provide feedback.

JohnThomson

@JohnThomson made 2 comments and resolved 2 discussions.
Reviewable status: 0 of 15 files reviewed, all discussions resolved.

src/BloomExe/Book/BookStorage.cs

JohnThomson · 2026-03-24T16:24:25Z

src/BloomExe/Book/BookStorage.cs

+                    if (
+                        outsideFrontCover.HasClass("bloom-customLayout")
+                        && !string.IsNullOrEmpty(
+                            outsideFrontCover.GetAttribute("data-custom-layout-id")
+                        )
+                    )
+                    {
+                        var onePageDom = new HtmlDom("<html><head></head><body></body></html>");
+                        onePageDom.Body.AppendChild(
+                            onePageDom.RawDom.ImportNode(outsideFrontCover, true)
+                        );
+                        bookData.SuckInDataFromEditedDom(onePageDom, BookInfo);
+                    }


StephenMcConnel

@StephenMcConnel reviewed 15 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on JohnThomson).

JohnThomson

@JohnThomson made 1 comment and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on JohnThomson).

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx

devin-ai-integration

Devin Review found 2 new potential issues.

View 7 additional findings in Devin Review.

devin-ai-integration · 2026-03-25T18:18:35Z

src/BloomExe/Book/BookStorage.cs

+                    outsideFrontCover.AddClass("bloom-customLayout");
+
+                    BookInfo.AppearanceSettings.PendingChangeRequiresXmatterUpdate = true;


🔴 Level 14 migration adds bloom-customLayout without data-custom-layout-id, causing it to be lost on next xmatter update

The MigrateToLevel14CoverIsImageToCustomLayout method at src/BloomExe/Book/BookStorage.cs:4598 adds bloom-customLayout class to the outside front cover but never sets data-custom-layout-id. It then sets PendingChangeRequiresXmatterUpdate = true at line 4600, which signals that the xmatter should be re-injected. When BringXmatterHtmlUpToDate runs (either triggered by the pending flag or on the next book update), the preservation code at src/BloomExe/Book/Book.cs:2548-2582 only preserves bloom-customLayout for pages that have BOTH bloom-customLayout AND data-custom-layout-id. Since the migration never sets data-custom-layout-id, the bloom-customLayout class will be stripped and the restored margin box content will be replaced with the standard xmatter template, resulting in loss of the user's custom cover content.

Xmatter preservation code that requires data-custom-layout-id

At Book.cs:2548-2582, before xmatter removal, customLayoutIds is collected from pages with both bloom-customLayout AND data-custom-layout-id. After re-injection, bloom-customLayout is only re-added to pages whose data-custom-layout-id is in that set. Without the attribute, the custom layout is lost.

Prompt for agents

In src/BloomExe/Book/BookStorage.cs, in the MigrateToLevel14CoverIsImageToCustomLayout method, after adding bloom-customLayout class at line 4598, also set data-custom-layout-id on the outsideFrontCover element. A reasonable value would be something like "customOutsideFrontCover" or derived from the page's data-xmatter-page attribute. This is required because BringXmatterHtmlUpToDate in src/BloomExe/Book/Book.cs (lines 2548-2582) only preserves bloom-customLayout for pages that also have data-custom-layout-id. Without it, the bloom-customLayout class and restored content will be lost on the next xmatter re-injection. Also reconsider whether PendingChangeRequiresXmatterUpdate should be set at all - if it triggers another BringXmatterHtmlUpToDate, the migration's work will be undone. The capture/restore mechanism only runs once (in CaptureInitialStateForMigration/RestoreStuffBeforeMigration), so a second xmatter injection would replace the restored content.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-03-25T18:18:36Z

src/BloomExe/Book/BookStorage.cs

+                    outsideFrontCover.RemoveClass("cover-is-image");
+                    outsideFrontCover.AddClass("bloom-customLayout");
+
+                    BookInfo.AppearanceSettings.PendingChangeRequiresXmatterUpdate = true;


🚩 PendingChangeRequiresXmatterUpdate may cause unintended second xmatter injection

At src/BloomExe/Book/BookStorage.cs:4600, PendingChangeRequiresXmatterUpdate is set to true. This flag was historically used when the coverIsImage setting changed in the UI to signal that xmatter needed re-injection. In the migration context, xmatter has already been re-injected (in EnsureUpToDateMemoryUnprotected), and the capture/restore mechanism has already run. If this flag causes another BringXmatterHtmlUpToDate call during the save path, the restored content and bloom-customLayout class would both be lost. It's worth investigating whether this flag is still consumed anywhere and whether setting it here is intentional or vestigial from the old coverIsImage flow.

Was this helpful? React with 👍 or 👎 to provide feedback.

JohnThomson added 4 commits March 24, 2026 10:52

Add format dialog for read-only custom fields (BL-15948)

3ee8245

Remove cover-is-image and migrate to custom cover

a24bef7

Select the thing that just became the background

d20c090

Make hint bubbles work on custom pages

515d2ed

JohnThomson changed the title ~~Enhancements to custom pages~~ Enhancements to custom pages (BL-15948) Mar 24, 2026

devin-ai-integration bot reviewed Mar 24, 2026

View reviewed changes

JohnThomson commented Mar 24, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

StephenMcConnel approved these changes Mar 24, 2026

View reviewed changes

post-review fixes

23693b9

JohnThomson force-pushed the customPageImprovements branch from 73bef05 to 23693b9 Compare March 25, 2026 18:10

JohnThomson commented Mar 25, 2026

View reviewed changes

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx Show resolved Hide resolved

devin-ai-integration bot reviewed Mar 25, 2026

View reviewed changes

		outsideFrontCover.AddClass("bloom-customLayout");

		BookInfo.AppearanceSettings.PendingChangeRequiresXmatterUpdate = true;

Uh oh!

Conversation

JohnThomson commented Mar 24, 2026 • edited by hatton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JohnThomson Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

StephenMcConnel left a comment

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JohnThomson commented Mar 24, 2026 •

edited by hatton

Loading