Skip to content

Comments

Prevent 6.3 from 'fixing' audio IDs in 6.4 custom covers (BL-15902)#7704

Open
JohnThomson wants to merge 1 commit intoVersion6.3from
patchForCustomCover
Open

Prevent 6.3 from 'fixing' audio IDs in 6.4 custom covers (BL-15902)#7704
JohnThomson wants to merge 1 commit intoVersion6.3from
patchForCustomCover

Conversation

@JohnThomson
Copy link
Contributor

@JohnThomson JohnThomson commented Feb 20, 2026


Open with Devin

This change is Reviewable

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment on lines 1284 to 1285
var duplicateAudioIdsFixed = 0;
foreach (var audioElement in nodes)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 duplicateAudioIdsFixed is never incremented, so SynchronizeDataItemsThroughoutDOM() is dead code

In FixDuplicateAudioIdsInDataDiv, the local variable duplicateAudioIdsFixed is initialized to 0 at line 1284 and checked at line 1296, but it is never incremented inside the loop when a duplicate is actually found and fixed (lines 1289–1293). As a result, the condition if (duplicateAudioIdsFixed > 0) is always false, and _bookData.SynchronizeDataItemsThroughoutDOM() is never called.

Root Cause and Impact

The loop at src/BloomExe/Book/Book.cs:1285-1293 calls FixDuplicateAudioId when a duplicate is detected, but never does ++duplicateAudioIdsFixed. Compare with the analogous method FixMissingAudioIdsInDataDiv at src/BloomExe/Book/Book.cs:1343-1360, which correctly checks nodeList.Count > 0 before calling SynchronizeDataItemsThroughoutDOM().

Without the synchronization call, duplicate audio IDs that are fixed in the data div are not propagated to the rest of the book's DOM, leaving the book pages out of sync with the data div.

This is a pre-existing bug but is in the method directly modified by this PR.

(Refers to lines 1284-1297)

Open in Devin Review

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

Comment on lines +1276 to +1282
// We expect the customCover to have copies of other stuff in the data div.
.Where(x =>
!(
x is SafeXmlElement e
&& e.ParentWithAttributeValue("data-book", "customCover") != null
)
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 customCover string literal only appears in C# code — verify it matches what 6.4 actually emits

The filter added in this PR checks for data-book="customCover" via e.ParentWithAttributeValue("data-book", "customCover"). However, searching the entire repository for customCover only returns these two lines in Book.cs. There are no HTML templates, test fixtures, or other references that confirm this is the exact attribute value used by version 6.4's custom cover feature. The commit c3f8b0378 ("Adds the cover layout control and allows a 'custom' cover (BL-15902)") is in the git log but its contents aren't available locally. It would be worth verifying that customCover is the exact data-book value used in the 6.4 branch.

Open in Devin Review

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant