Fix sign language tool for draggable videos (BL-16131) by JohnThomson · Pull Request #7836 · BloomBooks/BloomDesktop

JohnThomson · 2026-04-13T21:56:00Z

Fixing a few things here:

copying a draggable video to its target could put the bloom-selected class on the target. Having bloom-selected on a target could also happen during page open (because it was likely the first bloom-videoContainer in DOM order) or just because the page was saved that way. Several things work together to be sure this class is never put (or left) in a target.
trying to make the sign language (or any) tool selected was simulating a click on the check box for making it active (available). If it already was, this would hide it, and activate some other tool. I extracted the code for turning it on and selecting it, and call that explicitly.
seems we were counting on page-load code to reset the state of the tool after recording. That doesn't always happen, so I added code to reset some recording-in-process state, and make sure we see the new video.

This change is

Fixing a few things here: - copying a draggable video to its target could put the bloom-selected class on the target. Having bloom-selected on a target could also happen during page open (because it was likely the first bloom-videoContainer in DOM order) or just because the page was saved that way. Several things work together to be sure this class is never put (or left) in a target. - trying to make the sign language (or any) tool selected was simulating a click on the check box for making it active (available). If it already was, this would hide it, and activate some other tool. I extracted the code for turning it on and selecting it, and call that explicitly. - seems we were counting on page-load code to reset the state of the tool after recording. That doesn't always happen, so I added code to reset some recording-in-process state, and make sure we see the new video.

cubic-dev-ai

1 issue found across 9 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/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx">

<violation number="1" location="src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx:1895">
P2: The cleanup selector misses targets that themselves have `bloom-selected`, so the bug can persist after tab changes.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

cubic-dev-ai · 2026-04-13T22:00:44Z

 }

+function removeBloomSelectedFromTargets(page: HTMLElement): void {
+    page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => {


P2: The cleanup selector misses targets that themselves have bloom-selected, so the bug can persist after tab changes.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx, line 1895: <comment>The cleanup selector misses targets that themselves have `bloom-selected`, so the bug can persist after tab changes.</comment> <file context> @@ -1891,6 +1891,18 @@ export function getActiveDragActivityTab(): number { } +function removeBloomSelectedFromTargets(page: HTMLElement): void { + page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => { + el.classList.remove("bloom-selected"); + }); </file context>

Suggested change

page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => {

page.querySelectorAll("[data-target-of].bloom-selected, [data-target-of] .bloom-selected").forEach((el) => {

Not worried. Pretty sure .bloom-selected is never added to elements that actually have the data-target-of attribute. They are normally put on bloom-videoContainer elements, which are always a child of a target (or bloom-canvas-element)

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

JohnThomson

@JohnThomson made 1 comment.
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion.

JohnThomson · 2026-04-13T22:03:51Z

 }

+function removeBloomSelectedFromTargets(page: HTMLElement): void {
+    page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => {


Not worried. Pretty sure .bloom-selected is never added to elements that actually have the data-target-of attribute. They are normally put on bloom-videoContainer elements, which are always a child of a target (or bloom-canvas-element)

StephenMcConnel

There's a "new" Devin comment around line 870 of signLanguageTool.tsx that seems relevant. Another "new" comment on line 312 of bloomVideo.ts that might or might not be relevant. Everything else looks okay as far as I can tell.

@StephenMcConnel reviewed 9 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on JohnThomson).

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 648 at r1 (raw file):

                    ToolBox.addToolToString(toolId),
                    true,
                );

getting rid of checkBox.click() is probably long overdue. :-) 🥇

StephenMcConnel

@StephenMcConnel resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on JohnThomson).

JohnThomson

@JohnThomson made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).

devin-ai-integration

Devin Review found 3 new potential issues.

View 6 additional findings in Devin Review.

devin-ai-integration · 2026-04-14T17:22:40Z

        updateVideoInContainer(container, url);
    }

+    public leaveCurrentVideoContext(onComplete?: () => void) {


🟡 Missing comment on new public method leaveCurrentVideoContext

AGENTS.md mandates: "All public methods should have a comment." The new leaveCurrentVideoContext method is public but has no comment explaining its purpose or behavior.

Suggested change

public leaveCurrentVideoContext(onComplete?: () => void) {

// Reset the video recording state so the tool is ready for a new video context.

// Cleans up event listeners, timers, and overlays left over from any active recording session.

public leaveCurrentVideoContext(onComplete?: () => void) {

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

devin-ai-integration · 2026-04-14T17:22:41Z

+    return `${baseUrl}${search ? `?${search}` : ""}${hash}`;
+}
+
+export function stripTransientVideoTimestampParam(url: string): string {


🟡 Missing comment on exported function stripTransientVideoTimestampParam

AGENTS.md mandates: "All public methods should have a comment." The exported function stripTransientVideoTimestampParam has no comment.

Suggested change

export function stripTransientVideoTimestampParam(url: string): string {

// Remove the transient cache-busting query parameter from a video URL, preserving any other params and fragment.

export function stripTransientVideoTimestampParam(url: string): string {

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

devin-ai-integration · 2026-04-14T17:22:42Z

+    return `${baseUrl}${search ? `?${search}` : ""}${hash}`;
+}
+
+export function removeTransientVideoTimestampParams(root: ParentNode): void {


🟡 Missing comment on exported function removeTransientVideoTimestampParams

AGENTS.md mandates: "All public methods should have a comment." The exported function removeTransientVideoTimestampParams has no comment.

Suggested change

export function removeTransientVideoTimestampParams(root: ParentNode): void {

// Strip transient cache-busting query parameters from all video and video-source elements under the given root.

export function removeTransientVideoTimestampParams(root: ParentNode): void {

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

JohnThomson

I think I've dealt with all Devin's comments except where it is being picky about commenting public methods.

@JohnThomson made 1 comment.
Reviewable status: 7 of 11 files reviewed, 4 unresolved discussions (waiting on StephenMcConnel).

StephenMcConnel

@StephenMcConnel reviewed 4 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on JohnThomson).

StephenMcConnel

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on JohnThomson).

cubic-dev-ai Bot reviewed Apr 13, 2026

View reviewed changes

JohnThomson commented Apr 13, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

StephenMcConnel requested changes Apr 13, 2026

View reviewed changes

StephenMcConnel reviewed Apr 13, 2026

View reviewed changes

Post-review fixes

f09d9ed

JohnThomson commented Apr 14, 2026

View reviewed changes

Comment thread src/BloomBrowserUI/bookEdit/js/bloomVideo.ts Outdated

Comment thread src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx Outdated

devin-ai-integration Bot reviewed Apr 14, 2026

View reviewed changes

JohnThomson commented Apr 14, 2026

View reviewed changes

andrew-polk changed the base branch from master to Version6.4a April 14, 2026 20:59

StephenMcConnel approved these changes Apr 14, 2026

View reviewed changes

	page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => {
	page.querySelectorAll("[data-target-of].bloom-selected, [data-target-of] .bloom-selected").forEach((el) => {

	export function stripTransientVideoTimestampParam(url: string): string {
	// Remove the transient cache-busting query parameter from a video URL, preserving any other params and fragment.
	export function stripTransientVideoTimestampParam(url: string): string {

	export function removeTransientVideoTimestampParams(root: ParentNode): void {
	// Strip transient cache-busting query parameters from all video and video-source elements under the given root.
	export function removeTransientVideoTimestampParams(root: ParentNode): void {

Uh oh!

Conversation

JohnThomson commented Apr 13, 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 Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnThomson Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

JohnThomson Apr 13, 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!

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!

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

StephenMcConnel left a comment

Choose a reason for hiding this comment

Uh oh!

StephenMcConnel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JohnThomson commented Apr 13, 2026 •

edited by hatton

Loading

cubic-dev-ai Bot Apr 13, 2026 •

edited

Loading