Fix change field handling of narration (BL-16368) by JohnThomson · Pull Request #7939 · BloomBooks/BloomDesktop

JohnThomson · 2026-06-01T18:01:32Z

changing to None, or something that doesn't have text already: keeps the current text and any recording, but recording get a new ID and is no longer locked (or duplicated)
changing to something that has a value: also gets any existing recording.

This change is

cubic-dev-ai

2 issues 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/web/controllers/EditingViewApi.cs">

<violation number="1" location="src/BloomExe/web/controllers/EditingViewApi.cs:162">
P2: Do not interpolate request parameters directly into XPath; malformed or crafted values can break or alter the query.</violation>
</file>

<file name="src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts">

<violation number="1" location="src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts:6242">
P1: The new audio-independence logic skips `TextBox` recordings, so some kept recordings may retain shared ids instead of being duplicated with a new id.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

cubic-dev-ai · 2026-06-01T18:06:04Z

+            editable.classList.contains("audio-sentence") &&
+            editable.getAttribute("id")


P1: The new audio-independence logic skips TextBox recordings, so some kept recordings may retain shared ids instead of being duplicated with a new id.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts, line 6242: <comment>The new audio-independence logic skips `TextBox` recordings, so some kept recordings may retain shared ids instead of being duplicated with a new id.</comment> <file context> @@ -6225,6 +6228,36 @@ export class CanvasElementManager { + + const audioElements: Element[] = []; + if ( + editable.classList.contains("audio-sentence") && + editable.getAttribute("id") + ) { </file context>

Suggested change

editable.classList.contains("audio-sentence") &&

editable.getAttribute("id")

(editable.classList.contains("audio-sentence") ||

editable.getAttribute("data-audiorecordingmode") ===

"TextBox") &&

editable.getAttribute("id")

This looks like a hallucination. The new logic calls makeEditableAudioIndependent(), which includes the passed element in the list it passes to copySoundFileAndAttributes(). The latter does copy ids and various other attributes.

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

cubic-dev-ai · 2026-06-01T18:06:04Z

-            request.ReplyWithText(value);
+            var matchingDataDivElement =
+                View.Model.CurrentBook.RawDom.SelectSingleNode(
+                    $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"


P2: Do not interpolate request parameters directly into XPath; malformed or crafted values can break or alter the query.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomExe/web/controllers/EditingViewApi.cs, line 162: <comment>Do not interpolate request parameters directly into XPath; malformed or crafted values can break or alter the query.</comment> <file context> @@ -157,7 +157,32 @@ private void HandleGetDataBookValue(ApiRequest request) - request.ReplyWithText(value); + var matchingDataDivElement = + View.Model.CurrentBook.RawDom.SelectSingleNode( + $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']" + ) as SafeXmlElement; + </file context>

I think it's saying we should not put {dataBook} and {lang} into the xpath because someone might find a way to use fake values to inject something dangerous. But we do this all over, and I'm not sure what reasonable alternative there might be here. I don't think it's worth fixing.

Thanks for the context—understood this isn’t a change you want to make here.

JohnThomson

@JohnThomson made 2 comments.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions.

JohnThomson · 2026-06-01T19:31:07Z

+            editable.classList.contains("audio-sentence") &&
+            editable.getAttribute("id")


This looks like a hallucination. The new logic calls makeEditableAudioIndependent(), which includes the passed element in the list it passes to copySoundFileAndAttributes(). The latter does copy ids and various other attributes.

JohnThomson · 2026-06-01T19:31:08Z

-            request.ReplyWithText(value);
+            var matchingDataDivElement =
+                View.Model.CurrentBook.RawDom.SelectSingleNode(
+                    $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"


I think it's saying we should not put {dataBook} and {lang} into the xpath because someone might find a way to use fake values to inject something dangerous. But we do this all over, and I'm not sure what reasonable alternative there might be here. I don't think it's worth fixing.

andrew-polk

@andrew-polk reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on JohnThomson).

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx line 1039 at r1 (raw file):

                                    ? { content: result.data }
                                    : result.data;
                            const content = response.content;

Is this just defensive code the agent added for theoretical backward compatibility? Can't we just remove it since we control the data source?

src/BloomExe/web/controllers/EditingViewApi.cs line 183 at r1 (raw file):

                    hasBloomPostAudioSplitClass = matchingDataDivElement
                        ?.GetAttribute("class")
                        ?.Contains("bloom-postAudioSplit") == true,

Use SafeXmlElement.HasClass

andrew-polk

Please add the card number to the commit message.

@andrew-polk made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on JohnThomson).

- changing to None, or something that doesn't have text already: keeps the current text and any recording, but recording get a new ID and is no longer locked (or duplicated) - changing to something that has a value: also gets any existing recording.

JohnThomson

Sorry. Done.

@JohnThomson made 3 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on andrew-polk).

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx line 1039 at r1 (raw file):

Previously, andrew-polk wrote…

Is this just defensive code the agent added for theoretical backward compatibility? Can't we just remove it since we control the data source?

Done.

src/BloomExe/web/controllers/EditingViewApi.cs line 183 at r1 (raw file):

Previously, andrew-polk wrote…

Use SafeXmlElement.HasClass

Done.

greptile-apps · 2026-06-08T21:59:55Z

Greptile Summary

This PR fixes how switching a canvas field type's narration setting is handled: changing to "None" (or a type with no existing text) now gives the editable a fresh audio ID while preserving its current content, and changing to a field type that has a data-book value copies both the text and any associated audio recording identity.

getDataBookValue now returns a JSON object that includes audio attributes (id, recording mode, duration, end-times, MD5, classes) from the matching bloomDataDiv element, so the client can fully replicate the audio identity.
setEditableContentFromKnownDataBookValueIfAny becomes async and returns a boolean indicating whether the existing content was kept; callers use this to decide whether to call the new makeEditableAudioIndependent helper.
makeEditableAudioIndependent assigns a new XHTML-safe ID to every audio-sentence element in the editable and asynchronously copies the corresponding audio file, decoupling the recording from any shared source.

Important Files Changed

Filename	Overview
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx	Refactors field-type and None onClick handlers to use async/await inside wrapWithRequestPageContentDelay, adds applyEditableAudioIdentityFromDataBookValue to sync audio attributes from the data book response, and makes setEditableContentFromKnownDataBookValueIfAny return a boolean so callers can conditionally call makeEditableAudioIndependent. Logic looks correct and async DOM changes are properly guarded.
src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts	Promotes copyAnySoundFileAndAttributesForEditable to public and adds makeEditableAudioIndependent; the new method correctly captures the old id before overwriting it. Two C#-style XML doc comments were added that won't be processed by TypeScript tooling.
src/BloomExe/web/controllers/EditingViewApi.cs	Extends getDataBookValue to return a JSON object including audio identity attributes from the matching bloomDataDiv element. The XPath query interpolates lang and dataBook URL params without sanitisation, which is low-risk in this desktop context but fragile against malformed book attributes.

_{Reviews (1): Last reviewed commit: "Post-review improvements" | Re-trigger Greptile}

greptile-apps · 2026-06-08T21:59:59Z

-            request.ReplyWithText(value);
+            var matchingDataDivElement =
+                View.Model.CurrentBook.RawDom.SelectSingleNode(
+                    $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"


XPath injection risk from unsanitized params

dataBook and lang are inserted directly into the XPath string using string interpolation. A value containing a single quote (e.g. a lang attribute set to en' in a crafted book file) would malform the expression, causing a parse error or — in adversarial input — an injected predicate. Standard language tags never carry single quotes, but a corrupt or hand-crafted book could set unusual lang values.

Consider escaping or validating both parameters before interpolation, or use a parameterised XPath API if the underlying library supports one.

andrew-polk

@andrew-polk reviewed 2 files and all commit messages, made 1 comment, resolved 3 discussions, and dismissed @greptile-apps[bot] from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on JohnThomson).

andrew-polk · 2026-06-08T22:16:33Z

-            request.ReplyWithText(value);
+            var matchingDataDivElement =
+                View.Model.CurrentBook.RawDom.SelectSingleNode(
+                    $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"


JohnThomson · 2026-06-16T19:38:58Z

This PR is a draft because we don't actually want to merge it to master (and it has many conflicts). The conflict resolution resulted in a new PR. Keeping this alive because IF we want to cherry-pick to 6.4, which does not have the changes that caused all the conflicts, this is a better starting point.

cubic-dev-ai Bot reviewed Jun 1, 2026

View reviewed changes

JohnThomson commented Jun 1, 2026

View reviewed changes

andrew-polk requested changes Jun 5, 2026

View reviewed changes

andrew-polk reviewed Jun 5, 2026

View reviewed changes

JohnThomson added 2 commits June 8, 2026 15:31

Post-review improvements

9aff65d

JohnThomson force-pushed the keepAudioChangingField branch from 1e6fc12 to 9aff65d Compare June 8, 2026 21:55

JohnThomson commented Jun 8, 2026

View reviewed changes

greptile-apps Bot reviewed Jun 8, 2026

View reviewed changes

andrew-polk approved these changes Jun 8, 2026

View reviewed changes

andrew-polk assigned JohnThomson Jun 16, 2026

JohnThomson marked this pull request as draft June 16, 2026 19:37

JohnThomson changed the title ~~Fix change field handling of narration~~ Fix change field handling of narration (BL-16368) Jun 16, 2026

		editable.classList.contains("audio-sentence") &&
		editable.getAttribute("id")

Uh oh!

Conversation

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

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

JohnThomson Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk 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!

greptile-apps Bot commented Jun 8, 2026

Greptile Summary

Important Files Changed

Uh oh!

greptile-apps Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-polk Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohnThomson commented Jun 1, 2026 •

edited by hatton

Loading