Skip to content

Converted Talking Book Tool to React (BL-16410)#8075

Open
josiahwall wants to merge 5 commits into
masterfrom
talkingBookToolReact
Open

Converted Talking Book Tool to React (BL-16410)#8075
josiahwall wants to merge 5 commits into
masterfrom
talkingBookToolReact

Conversation

@josiahwall

@josiahwall josiahwall commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Devin review


This change is Reviewable

JohnThomson and others added 4 commits July 9, 2026 10:52
A design/hand-off document (Steps 0-5) describing how to convert the last
non-React toolbox tool to React: move the AudioRecording engine to the page
iframe, turn the button-state machine and controls into a React component in
the toolbox, and stop audioRecording.ts from being duplicated into the page
bundle. Step 6 (retiring the legacy non-React toolbox framework) is documented
as a deferred follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR converts the Talking Book Tool from a pug/jQuery/DOM-class-based implementation to a React component (TalkingBookToolControls). The engine (audioRecording.ts) is refactored to drive UI via an observer pattern (registerStateListener / notifyStateChanged) instead of direct DOM mutations and delegated jQuery handlers.

  • Engine decoupled from DOM: setStatus now writes to uiState.buttons and calls notifyStateChanged() rather than toggling CSS classes on #audio-* elements; the #player element is created lazily when needed instead of requiring it in the HTML.
  • React UI layer added: TalkingBookToolControls.tsx renders buttons, the recording meter/device menu, and the advanced section from a single TalkingBookUiState snapshot; the canvas reference is passed to the engine via useMountEffect.
  • Legacy infrastructure removed: talkingBookToolboxTool.pug, audioRecording.less, SetupTalkingBookUIElements test helper, delegated jQuery handlers, and the ToolboxRoot legacy-HTML loading path are all deleted.

Important Files Changed

Filename Overview
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBookToolControls.tsx New React component rendering the full talking book UI; correctly uses useMountEffect for canvas setup; TalkingBookButton destructures props in violation of the repo style guide.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts Core engine refactored from DOM-manipulation to a uiState/observer pattern; delegated handlers removed in favor of public methods called directly by React; setStatus now mutates uiState and calls notifyStateChanged; getOrCreateAudioRecorder added for lazy initialization.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx New React adaptor replacing the old talkingBook.ts; delegates lifecycle events to the audioRecording engine cleanly; makeRootElement wires TalkingBookToolControls with getOrCreateAudioRecorder.
src/BloomBrowserUI/bookEdit/js/talkingBookChecksum.ts Extracted checksum helper; the replace call only strips the first '
src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts Removed the old pug-based HTML fetch path from beginAddTool; code is correct, but a stray //} comment left over from the refactoring remains.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts Tests updated to check uiState directly instead of DOM classes; DOM button setup removed since player is now created lazily; setupForListen call added to force player element creation before tests that need it.

Reviews (10): Last reviewed commit: "Converted Talking Book Tool to React (BL..." | Re-trigger Greptile

@josiahwall
josiahwall force-pushed the talkingBookToolReact branch 7 times, most recently from 12cc862 to 05064c7 Compare July 17, 2026 21:51
// However, we don't want the sentence markup to be updated because the checksums differ (since a character was deleted).
//
// Thus, our checksum function needs to ignore the vertical line character when computing the checksum.
const adjustedMessage = message.replace("|", "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Only the first | is stripped. String.prototype.replace with a plain string argument stops after the first match, so a sentence like "Hello | World | test" produces checksum "Hello World | test" — the second pipe is retained. After the user deletes all delimiter characters the actual text becomes "Hello World test", so the checksums never agree and the audio markup is unnecessarily regenerated every session. Use a global regex to remove all occurrences.

Suggested change
const adjustedMessage = message.replace("|", "");
const adjustedMessage = message.replace(/\|/g, "");

@josiahwall
josiahwall force-pushed the talkingBookToolReact branch 2 times, most recently from 24bcd41 to 19b45f5 Compare July 17, 2026 23:06
@josiahwall josiahwall changed the title Converted Talking Book Tool to React Converted Talking Book Tool to React (BL-16410) Jul 17, 2026
Comment on lines +1833 to +1836
if (this.uiState.shouldShowDeviceMenu) {
this.uiState.shouldShowDeviceMenu = false;
this.notifyStateChanged();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Device menu anchor lost on tool remount

shouldShowDeviceMenu is never reset to false in handleToolHiding() or setupForRecordingAsync(). If the user opens the device menu and then hides/reshows the tool, the React component remounts with menuAnchor reset to undefined (local useState), but uiState.shouldShowDeviceMenu is still true from the engine. The MUI Menu then renders with open={true} and anchorEl={undefined}, which causes it to appear at the wrong position (body top-left corner) rather than below the device icon.

Adding this.uiState.shouldShowDeviceMenu = false; inside handleToolHiding() would prevent the stale-open state from being carried over to the next mount.

@josiahwall
josiahwall force-pushed the talkingBookToolReact branch from 19b45f5 to 5191780 Compare July 17, 2026 23:38
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.

3 participants