Skip to content

Fix various tasks when SL tool not initialized (BL-16068)#7790

Open
JohnThomson wants to merge 1 commit intomasterfrom
missingSLTools
Open

Fix various tasks when SL tool not initialized (BL-16068)#7790
JohnThomson wants to merge 1 commit intomasterfrom
missingSLTools

Conversation

@JohnThomson
Copy link
Copy Markdown
Contributor

@JohnThomson JohnThomson commented Mar 30, 2026


Open with Devin

This change is Reviewable

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown

@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 1 new potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +1074 to +1076
function isToolInitialized(tool: ITool): boolean {
return !!getToolElement(tool);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 isToolInitialized checks toolbox DOM, but detachFromPage operates on page content

isToolInitialized at line 1074 checks whether the tool's <h3> header exists in the #toolbox DOM. However, detachFromPage() for several tools (e.g., SignLanguageTool.detachFromPage removes click listeners from video containers on the page, GameTool.detachFromPage calls undoPrepareActivity(page)) operates on the page iframe content, not the toolbox DOM. If a tool was properly activated and then its h3 was removed (e.g., via adjustToolListForPage for requiresToolId tools), detachFromPage() would be skipped even though page-level cleanup is still needed. I traced the page-navigation flow and found that removeToolboxMarkup() calls detachFromPage() BEFORE the h3 is removed, so cleanup happens at the right time in the normal flow. But if there's any path where adjustToolListForPage runs without a prior removeToolboxMarkup(), this could result in missed cleanup.

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