Save the best possible img from custom layout to standard (BL-16086)#7806
Save the best possible img from custom layout to standard (BL-16086)#7806JohnThomson wants to merge 1 commit intomasterfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| coverImgElt = GetCoverImageElt(outsideFrontCover); | ||
| if (coverImgElt == null) | ||
| return null; | ||
|
|
||
| return GetImagePath(coverImgElt); | ||
| } |
There was a problem hiding this comment.
🚩 Behavioral change: candidate selection no longer checks file existence on disk
The old GetCoverImagePathAndElt used GetImagePath() for candidate selection, which resolved paths against StoragePageFolder and checked RobustFile.Exists(). The new GetCoverImageElt uses GetImageSourceForCoverSelection() (Book.cs:5304-5311) which only reads DOM attributes. This means if the designated cover image has a valid-looking src but the file doesn't exist on disk, GetCoverImageElt will select that element, and then GetImagePath at Book.cs:5235 returns null — whereas the old code would have skipped it and tried the next candidate in the foreach loop. This affects all callers: CollectionApi.cs:710, BookMetadataApi.cs:56, BookCompressor.cs:48, PublishApi.cs:289, BookThumbNailer.cs:237, and Book.cs:5360. The scenario (designated image source is non-empty/non-placeholder but file missing, AND another valid candidate exists on the page) is very unlikely in practice, and DOM-based selection is more appropriate for the new BookData use case. Still, worth noting as a semantic change.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 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/BloomExe/Book/Book.cs">
<violation number="1" location="src/BloomExe/Book/Book.cs:5235">
P1: Behavioral regression: `GetCoverImageElt` selects elements based on whether the HTML source attribute is non-empty, but does not verify the referenced file exists on disk. The old inline code used `GetImagePath()` which checked file existence, so it would skip candidates with broken file references and fall through to other valid candidates. Now, if the best candidate's file is missing, `GetCoverImagePathAndElt` returns null instead of trying the next candidate.
Consider either (a) having `GetCoverImageElt` accept a file-existence predicate to skip broken references, or (b) adding a fallback loop in `GetCoverImagePathAndElt` that retries with the next-best candidate when `GetImagePath` returns null.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (coverImgElt == null) | ||
| return null; | ||
|
|
||
| return GetImagePath(coverImgElt); |
There was a problem hiding this comment.
P1: Behavioral regression: GetCoverImageElt selects elements based on whether the HTML source attribute is non-empty, but does not verify the referenced file exists on disk. The old inline code used GetImagePath() which checked file existence, so it would skip candidates with broken file references and fall through to other valid candidates. Now, if the best candidate's file is missing, GetCoverImagePathAndElt returns null instead of trying the next candidate.
Consider either (a) having GetCoverImageElt accept a file-existence predicate to skip broken references, or (b) adding a fallback loop in GetCoverImagePathAndElt that retries with the next-best candidate when GetImagePath returns null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomExe/Book/Book.cs, line 5235:
<comment>Behavioral regression: `GetCoverImageElt` selects elements based on whether the HTML source attribute is non-empty, but does not verify the referenced file exists on disk. The old inline code used `GetImagePath()` which checked file existence, so it would skip candidates with broken file references and fall through to other valid candidates. Now, if the best candidate's file is missing, `GetCoverImagePathAndElt` returns null instead of trying the next candidate.
Consider either (a) having `GetCoverImageElt` accept a file-existence predicate to skip broken references, or (b) adding a fallback loop in `GetCoverImagePathAndElt` that retries with the next-best candidate when `GetImagePath` returns null.</comment>
<file context>
@@ -5228,6 +5228,18 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
+ if (coverImgElt == null)
+ return null;
+
+ return GetImagePath(coverImgElt);
+ }
+
</file context>
1 participant
This change is