Converted Decodable Reader Setup Dialog to React - #8132
Conversation
70feaa3 to
76e069f
Compare
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
Claude Opus 5 (during /preflight on Andrew's machine) - Devin consultation log
Devin reviewed this PR at 3e0c2978. Its passes raised 9 bugs and 22 flags; most were either already fixed on the branch or have been fixed during this preflight (see the two commits above). What follows are the findings that are still open and need a human decision, one thread each.
No other review bot ran: this repo's .coderabbit.yml sets auto_review.enabled: false, and there is no Greptile review on this PR.
Fixed during preflight: typed sample words saved with line breaks (Devin's only severe finding - the words silently stopped counting as decodable); the hard-coded English hint under the sight-words box; the missing accessible names on the three text boxes; the dropped doc comment on the word-list cleaner; fail-fast when the toolbox bundle is missing; the props type aliases. Plus two things Devin caught in my own first preflight commit: sample words losing the legacy de-duplication, and save() comparing cleaned values against uncleaned ones.
Reported from a stale commit, already fixed before preflight: stage removal not deleting its word-list file, the stacked ternaries, the legacy 3-argument save path, and the dead LESS / reactHost block. I verified each against the current code rather than trusting the line numbers.
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
Claude Opus 5 (during /preflight, round 2) — Devin consultation log
Devin re-reviewed at 91864be and raised no new bugs against this commit. Every bug it still lists carries the SHA of a superseded commit; I checked each against the current code.
The developer's decisions from the first round are now implemented and each of those threads has a reply recording the outcome. One thread is deliberately left open for discussion (the persisted-settings shape), and Devin's later passes surfaced three further things worth a decision — one thread each below.
Round 2 also fixed one bug in my own round-1 work, which Devin caught: getSynphonyAlwaysMatchSymbols filtered on typeof symbol === "string" and so silently dropped an array-valued AlwaysMatch — the very symptom the function was added to remove. Fixed in 91864be with four tests; the array case fails against the previous implementation.
Gate at 91864be: typecheck pass, lint 0 errors, 612 TypeScript tests pass (up from 574 at the start of preflight — 38 added across this run), CI green. The C# suite was run earlier this preflight (2981 passed; the one failure needs Reading App Builder's build tools, which are not installed on this machine); nothing since has touched C# or the XLF files.
|
Claude Opus 5 (during Devin re-reviewed at Round-3 decisions, all now implemented:
Two things worth flagging for someone other than this PR. 1. Master's own leveled-reader tests were failing, and this branch had to unbreak them to go green. They fail the same way on
Someone from BL-16585 should sanity-check that second one — it assumes the em dash is the intended display, which is what the component does today. If blank was intended, the component is what needs changing, not the spec. 2. Master's shared One thread is still open, on the sample-text extension filter, pending discussion with the developer. |
8a7f5c6 to
a849a77
Compare
|
Claude Opus 5 (during Rebased onto master and force-pushed ( Master's test fix landed, and mine dropped out. The shared Two further bugs, found by Devin on the rebased branch and fixed:
Both verified in a running Bloom by counting the actual One item remains open — the thread on the sample-text extension filter. Worth noting what that finding is and is not: the extension list and its case-sensitivity are identical to the pre-PR dialog, so neither is a regression. What genuinely changed is that the old dialog listed unreadable files with an explanation ("Cannot read this format" / "File needs .TXT extension") plus a help link, and showed a message for an empty folder; the new one filters them out silently. Those strings still exist and are translated, but are now referenced only from the legacy pug. Awaiting the developer's call. |
6e3e779 to
133fb11
Compare
|
Claude Opus 5 (during Devin's final pass at Assessed as not worth a change, for two reasons:
Recording it here rather than silently dropping it. Happy to convert them if the team reads the rule more broadly than I have. Everything else from Devin is fixed; this was the only item left, and CI plus the 624-test front-end suite are green. |
Follow-on work to Josiah's conversion of the dialog to React: the review pass
over it, the fixes that came out of that, and the fixes that came out of Devin's
passes over those.
Data corruption and correctness:
- Typed sample words were saved verbatim, newlines and all, and the stored
moreWords string is later re-split on plain spaces, so words typed one per
line became a single run-together word and silently stopped counting as
decodable. Saving now normalizes moreWords the way the legacy dialog did, in a
new unit-testable prepareSettingsForSave() helper, and de-duplicates the list
(via cleanSampleWordList) so repeats no longer inflate word frequency.
- save() compared normalized values against the raw values it had loaded, so any
stored setting with a comma or doubled space looked changed even when the user
edited nothing, forcing a needless reload of every sample text file. The
baseline is normalized the same way before comparing.
- prepareSettingsForSave now drops stages the user put nothing into and
renumbers the survivors, which is what the legacy dialog did.
- Deleted letters kept counting in allowed-words mode: the full language-data
rebuild was the only thing that ever removed a grapheme, and it had been
skipped for allowed-words collections. Split out ResetLanguageDataGraphemes so
the alphabet is always rebuilt while the loaded sample words are preserved.
- Removed the dialog's private copy of Synphony's word splitting; it now calls
LanguageData.getGpcForm directly, so a typed word is segmented by exactly the
same code as a word from a sample text.
- Special language symbols were read from the workspace frame's always-empty
copy of the Synphony data, dropping typed words that contained one from the
matching-words preview. They now come from the toolbox frame via the new
getSynphonyAlwaysMatchSymbols, which handles AlwaysMatch holding either a
single symbol or an array (Synphony concats it, so both shapes are legal).
- getFileExtension split the whole path on ".", so a file with no extension
under a folder whose name contains a dot was mislabelled "Cannot read this
format" instead of "File needs .TXT extension". It now looks only at the last
path segment.
Dialog lifecycle:
- showDecodableReaderSetupDialog locked the edit view before rendering the
dialog, and only the dialog posts the unlock -- so a render failure left the
edit view locked with no way back except restarting Bloom. That was reachable
by pressing Set Up Stages before the reader settings had loaded. The lock now
happens in a mount effect, alongside the unlock.
- close() is idempotent via a ref, and the dialog's close handle is published
from an effect rather than during render, so a second close request can no
longer re-post setModalState = false and unlock the edit view underneath
whatever is relying on it.
User-visible restorations and additions:
- Unreadable files in the Sample Texts folder were being silently filtered out.
They are listed again with the already-translated explanations ("Cannot read
this format", "File needs .TXT extension") and the export/convert help link,
and extension matching is case-insensitive everywhere, via the single
kReadableSampleTextFileExtensions list in readerToolsModel.
- New ReaderSetup.NoSampleTextsYet message for an empty folder; the old
dialog's #please-add-texts label had no l10n key and never displayed anything.
- The Help button opens the page for the tab you are on again, rather than
always Decodable Stages.
- "Separate words with spaces." is localized instead of hard-coded English, and
the three shared text boxes take a localized ariaLabel so they have an
accessible name.
Tests, comments and cleanups:
- New specs for decodableStagesUtils (prepareSettingsForSave,
cleanSpaceDelimitedList, cloneReaderSettings, hasOnlyKnownGraphemes) and for
getSynphonyAlwaysMatchSymbols and getFileExtension.
- Fail fast if the toolbox bundle is missing instead of silently skipping
long-press support; inlined props type aliases per AGENTS.md; drive the tabs
with MUI's onChange.
- Comments recording the accepted limitations (stale grapheme segmentation and
the word-frequency inflation on repeated saves in allowed-words mode, the
stage-letter pruning on remount, the two matching-words helpers that really do
ask Synphony the same question) and the three spots that would trip up whoever
converts the Leveled Reader setup dialog next.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
942c43a to
2f319d8
Compare
3 participants
This pr is for converting the decodable reader's setup dialog to React; the leveled reader's dialog still needs to be converted.
Devin review
This change is