Convert Decodable Reader Tool to React

[josiahwall]

I've completed the "first draft" of the new decodable reader tool, and so I want to run my changes through the series of code reviews to see if I need to make any changes.

Devin review

---

This change is 

[greptile-apps]

Greptile Summary

This PR converts the Decodable Reader toolbox tool from an HTML/legacy implementation to a React component, registering a new DecodableReaderTool class backed by DecodableReaderToolControls and leaving the old class temporarily in place (renamed to "decodableReaderOld") for rollback safety.

• New DecodableReaderToolControls renders stage navigation, grapheme grids, word lists with sort controls, and the enable/disable switch — all as React state driven by the existing ReaderToolsModel via a new refreshFunc callback.
• readerToolsModel.ts gains three new sorted-list getters (getKnownGraphemesSorted, getStageSightWordsSorted, getAllowedWordsSorted) and the refreshFunc hook that lets updateControlContents push updates into the React component.

Important Files Changed

Filename	Overview
src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx	New React component for decodable reader controls; showTool is never updated on page navigation, leaving controls in the wrong visible/hidden state after switching pages.
src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx	New React-adaptor class wiring the component into the toolbox; newPageReady triggers updateControlContents but the resulting updateState callback omits setShowTool, which is the root cause of the page-navigation visibility bug.
src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts	Adds refreshFunc callback and three new sorted-list getters; getKnownGraphemesSorted mutates this.stageGraphemes in place instead of sorting a copy.
src/BloomBrowserUI/bookEdit/toolbox/readers/ReaderToolSwitch.tsx	Adds optional changeDisplayFunc prop called on toggle; uses correct functional updater form at the call site in the parent. Minor blank-line cleanup.
src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderToolboxTool.ts	Old tool's id() changed to "decodableReaderOld" and key methods commented out to prevent conflicts with the new React tool during the transition.
src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts	Swaps DecodableReaderToolboxTool registration for the new DecodableReaderTool; straightforward import swap.
src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx	Decodable reader entry in legacyToolSubPathByToolId commented out since the new tool renders via React rather than loading an HTML sub-path.
src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts	Corresponding decodableReaderTool HTML sub-path entry commented out; matches the ToolboxRoot.tsx change.
src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderToolboxTool.ts	Comment-only update: references to decodableReaderToolboxTool.ts updated to decodableReaderTool.tsx.

_{Reviews (4): Last reviewed commit: "Tweaked css to work with window resizing" | Re-trigger Greptile}

[greptile-apps]

showTool not synced when navigating to a new page

showTool is initialized once from isReaderToolEnabledOnCurrentPage(false), and the only place it is subsequently updated is through changeDisplayFunc — which only fires when the user manually toggles the switch. When the user navigates to a different page, newPageReady() calls model.updateControlContents() → refreshFunc() → updateState(), but updateState never calls setShowTool. The result is that the tool content remains in whatever visible/hidden state it was in for the previous page, regardless of whether the new page has the decodable reader enabled. For example, if the user navigates from an enabled page (showTool = true) to a disabled page, the stage navigation and word lists are still rendered when they should be hidden.

[andrew-polk]

Reader enabled state is per book, not page.

[greptile-apps]

You're right, thanks for the correction. If reader-enabled is a book-level setting, then showTool can only change when the user explicitly toggles it, which changeDisplayFunc already handles. My concern about page navigation causing stale state was based on a wrong assumption.

Shall I remember this as a rule: "In the decodable reader tool, reader-enabled state is scoped to the book, not individual pages, so showTool does not need to be refreshed on page navigation"?

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[andrew-polk]

Reader enabled state is per book, not page.

[andrew-polk]

I still need to review DecodableReaderToolControls.tsx.
But I'm posting what I have so far.

@andrew-polk partially reviewed 11 files and all commit messages, and made 9 comments.
Reviewable status: 8 of 12 files reviewed, 8 unresolved discussions (waiting on josiahwall).

---

src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx line 73 at r2 (raw file):

const legacyToolSubPathByToolId: Record<string, string> = {
    talkingBook: "talkingBook/talkingBookToolboxTool.html",
    //decodableReader: "readers/decodableReader/decodableReaderToolboxTool.html",

Let's remove, not comment.

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 1276 at r2 (raw file):

        talkingBookTool: "talkingBook/talkingBookToolboxTool.html",
        //decodableReaderTool:
        //"readers/decodableReader/decodableReaderToolboxTool.html",

Let's remove, not comment.

And I think we can now delete decodableReaderToolboxTool.html? And the pug file?

---

src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts line 445 at r2 (raw file):

        );

        stageParent.innerText = localizedFormattedText;

Let's get rid of as much of this html-setting cruft as we can.

Anything at all related to Leveled Reader can wait for the next PR.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts line 866 at r2 (raw file):

     * @returns a sorted array of letters
     */
    public getKnownGraphemesSorted(stageNumber: number): string[] {

Since stageNumber isn't used, let's remove it to reduce the possibility of future confusion.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts line 880 at r2 (raw file):

        letters.sort((a, b) => {
            return allLetters.indexOf(a) - allLetters.indexOf(b);
        });

Here, you're actually sorting this.stageGraphemes directly. This may be how the current one is working, and in practice, it might be harmless, but it would be better/safer to make a copy of this.stageGraphemes to sort and return.

So something like

    // copy so we don't mutate this.stageGraphemes
    const letters = [...this.stageGraphemes];

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx line 14 at r2 (raw file):

// of the methods that were implemented in decodableReaderToolboxTool.ts,
// so that everything would load properly and the markup would work as
// usual.

I'm not sure that referencing back to the old file is helpful for future maintainers.
Let's rewrite this comment from a here-forward perspective.
Simply, what is this class's current function?

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx line 54 at r2 (raw file):

            )["decodableReaderState"];
            // This wrapper function keeps Devin happy by ensuring that the promise gets resolved,
            // even in the very unlikely case that setStageNumber fails.

Let's not refer to devin here. The comment should just state why this is here.
Something like
// This wrapper guarantees that the promise gets resolved,
// even in the very unlikely case that setStageNumber fails.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderToolboxTool.ts line 1 at r2 (raw file):

/// <reference path="../../toolbox.ts" />

Let's delete this file since it is now obsolete.

[andrew-polk]

@andrew-polk reviewed 3 files.
Reviewable status: 11 of 12 files reviewed, 8 unresolved discussions (waiting on josiahwall).

[andrew-polk]

Over all, looking good!

@andrew-polk reviewed 1 file and made 8 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on josiahwall).

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 205 at r2 (raw file):

#b0dee4

For this and the other two instances, please use the const kBloomLightBlue

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 518 at r2 (raw file):

#1a1a1a

This is const kDarkestBackground.
Please use here and below.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 19 at r2 (raw file):

import { useL10n } from "../../../../react_components/l10nHooks";

const StageNav: FunctionComponent<{

Since you know you will reuse this for leveled reader tool, let's go ahead and extract it and make it general.
Let's move it its own file and try to come up with a general name.
ReaderToolNav?
This will be good practice for creating reusable components.

Also, please add a description of what a "StageNav"(ReaderToolNav) is.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 102 at r2 (raw file):

// of characters, since there are instances where strings such as
// ww can be bigger than iii
const DecodableGrid: FunctionComponent<{

In theory, this could be a useful component apart from Decodable.
Since there is currently no other known use case, we can leave it in this file, but let's try to generalize it a little.
Is there a better name like WordGrid?
Instead of forGraphemes, you could make it direction: "row" | "column" or byRow: boolean.

Update: after reading through the rest and realizing there is special logic for DataWords and isSightWord, that does push it a bit more toward DecodableGrid. Up to you. But I think I would still make the direction part general.

Peronally, I think displayList is clearer as simply words.

For the comment above the function, I think I would move the technical details about how it works inside the function.
I would leave the comment above the function for a higher-level statement of what it does.
Like, "Displays a grid of words with dynamically spaced columns based on the longest word"

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 144 at r2 (raw file):

                ref={measureRef}
                css={css`
                    position: absolute;

Why is this absolutely positioned?

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 419 at r2 (raw file):

            model.refreshFunc = undefined;
        };
    });

There is a comment in devin about using a ref here to prevent doing this on every render and possibly creating a race condition. Please evaluate that and see if it makes sense to use the ref.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/DecodableReaderToolControls.tsx line 448 at r2 (raw file):

    return (
        <div

I thought we had set this up with <ThemeProvider theme\={toolboxTheme}\>. Either way it isn't here now, but it should be.

I think that will give us the text-transform: none; we want on the button to launch the settings dialog (making it not all-caps).