Review 5204

#5204 — fix(richtext): close all 8 RichTextEditor audit BLOCKs

AKnassa · open · view on GitHub

Verdict: REQUEST CHANGES (posted)

HEAD REVIEWED: 8f470c9ed65060ab087dab292fb6ccd73c48eeb2 — the head was re-fetched before posting and had not moved.

Epic coordination. The original stable RichTextArea proposal #981 remains closed. The later #4678 decision created the narrower, canary-only @astryxdesign/richtext package. This review treats #5204 as hardening that canary surface, not as reopening the stable package/API proposal; the link was recorded on #981.

---

PR

#5204 fix(richtext): close all 8 RichTextEditor audit BLOCKs by AKnassa (bucket: contributor)

HEAD REVIEWED

8f470c9ed65060ab087dab292fb6ccd73c48eeb2

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12

LANE

LANE: full WHY: 31 files and 3,021 changed lines; public core API, behavior, Effect, theming, accessibility, and intended visual changes all promote this out of fast review.

PROBLEM

WHY 1: The audited editor left visible/AT strings untranslated, a read-only textbox unnamed and unreachable, disabled/read-only semantics identical, theme icon overrides ineffective, and RTL arrows wrong. WHY 2: Keyboard and screen-reader users could not reliably reach or understand persisted content, while builders could not theme the toolbar through the supported path. WHY 3: A rich-text system has to preserve authored content and its controls across input modality, locale, direction, and theme; otherwise people cannot safely read or edit the document. USER-FACING PROBLEM: People reading or editing rich text through keyboard, assistive technology, another locale, RTL, or a custom theme get an inaccessible or misleading editor. PROBLEM SEVERITY: broken task — the audited read-only path could not be reached by keyboard and exposed no accessible name.

VERDICT: clear

SOLUTION

The editor now translates its own language, separates read-only from disabled behavior, synchronizes Lexical when those props change, and routes toolbar visuals through theme and direction owners. It also widens core icon theming, changes the shared Selector and isRenderable, adds a render error boundary, and extends lint/tests so these contracts stay checked. Those extra owners make this a bundle of independent changes rather than one audit fix.

SOLUTION (15 decisions · ~754 runtime-line churn of 3,021 changed lines)

1. Extend strict/type-aware Astryx lint coverage to packages/richtext — root cause.
2. Route 31 toolbar/editor strings through the catalog — audit i18n problem.
3. Add RichTextView.label and split read-only/disabled semantics — audit a11y problem.
4. Synchronize Lexical editability after prop changes — state/ARIA consistency.
5. Move Selector trigger padding and add 44px coarse toolbar targets — target-size finding.
6. Widen defineTheme({icons}) to namespaced extension keys — toolbar theme problem.
7. Resolve toolbar icons through the active theme and mirror undo/redo — theme/RTL problems.
8. Tokenize toolbar glyph and editor heading typography — audit token debt.
9. Add a richtext themeProps doc/source guard — lint-coverage root cause.
10. Add BaseProps/ref/style merging on toolbar/view — audit public-contract problem.
11. Catch unusable serialized editor states and recover on value change — [NO STATED PROBLEM].
12. Make character counts grapheme-safe and merge computed/consumer ARIA — [NO STATED PROBLEM].
13. Change global isRenderable array behavior and icon own-property lookup — [NO STATED PROBLEM].
14. Publicly export test-only __resetDevWarnings — [NO STATED PROBLEM].
15. Rename linkOpensInNewTab and convert refs/defaults/casts — audit FIXes, independently landable.

BURDEN: high — 15 decisions, 31 files, 754 runtime-line churn, shared core behavior/API, one new Effect, four changesets, and 345 targeted tests. BURDEN MATCH: disproportionate — the eight serious audit findings justify substantial work, but unrelated core utilities and public test surface make settled fixes wait on independent decisions.

VERDICT: BLOCKS — independent fixes and unrecorded core changes need a concrete split.

ARCHITECTURE

OWNER: RichTextEditor/RichTextView own editor semantics; IconRegistry/defineTheme own icon extension; Selector owns its trigger; isRenderable owns slot presence. TIER 1: Field/input anatomy, useTranslator, IconRegistry, themeProps, and React/Lexical synchronization are reused. TIER 2: shared size tokens and semantic icon extension apply. SEAMS: toolbar slot, theme object/name lookup, RichTextView value/fallback transition, Selector InputGroup, RTL, and coarse pointer. BEHAVIOR UNIT: inline — the one new [editor, editable] Effect is a narrow one-way synchronization with the external Lexical editor; deterministic presence logic remains a pure utility.

seam	driven result
RichTextEditor toolbar slot	theme icon, RTL wrapper, and 44px coarse controls resolved in Chromium
RichTextView value update	valid → unusable value rendered the fallback in the review probe
Selector default trigger	200×20 before → 200×30 after; pixels remained byte-identical
theme inheritance/name lookup	targeted registry tests passed

The individual owner placements are coherent. Their number is evidence for splitting, not for inventing a new cross-system owner.

VERDICT: note — sound owners, bundled across four independently reviewable systems.

IMPACT

Richtext is private/canary-only, so its rename has no stable installed consumer; its editor fixes reach canary users. The core Selector, isRenderable, icon types, and /utils barrel reach released core builders: a legal unlabeled RichTextView remains unnamed, an array of only empty React children still draws guarded wrappers, and a test-only reset becomes public API.

VERDICT: BLOCKS — two user-visible holes remain and one unsupported public surface is added.

API

<RichTextView value={json} /> // still compiles; renders an unnamed role="textbox"
defineTheme({icons: {'richtext:bold': <MyBold />}})

	change	public?	class	doc'd?	verdict
+	RichTextView.label?: string	private/canary package	required input labels across Field components	guidance added; JSDoc example still omits it	finding — legal call remains unnamed
+	ExtendedIconRegistry and widened defineTheme.icons	core barrels	existing getExtendedIcon namespaced-key contract	theme doc + template + changeset	ok; split for focused review
~	RichTextEditorToolbarProps extends BaseProps, ref?	private/canary package	every component BaseProps/ref contract	source/tests	ok
~	linkOpensInNewTab? → hasNewTabLinks?	private/canary package	boolean naming	docs/tests	canary-only break; split
+	__resetDevWarnings	released @astryxdesign/core/utils	none — test-only implementation control	missing body rationale and changeset	finding
~	isRenderable(ReactNode) now special-cases arrays	released @astryxdesign/core/utils	global slot-presence helper	changeset + test	finding — nonempty all-empty arrays still return true

OSSIFICATION: label belongs to an established required-label class, so make it required before this canary API ships. Namespaced theme icons extend an already-landed getExtendedIcon class. The test reset has no user class and would cost a deprecation cycle to remove; it should not enter the public barrel. isRenderable is already public, so its new array promise must cover the reachable array class it names.

VERDICT: BLOCKS — optional label, incomplete array semantics, and a test-only core export.

THEMING

No theme target is added or removed: the existing astryx-rich-text-editor target now documents size and status. Toolbar glyphs resolve through the active theme, heading/glyph typography uses role tokens, and namespaced icon typing extends the existing registry concept. The pixel-changing heading and read-only treatments require visual inspection before approval.

VERDICT: note — source/theming contracts pass; rendered treatment was not visually inspectable in this session.

BREAKING

BEHAVIOR: yes — read-only/disabled, Selector hit area, empty-array slot guards, invalid-state fallback, character counting, and icon lookup change; the empty-child array class remains broken. API: yes for private/canary richtext (linkOpensInNewTab rename); released core additions are additive, including the unwanted test reset. VISUAL: yes — read-only opacity, 44px coarse controls, RTL glyph direction, and h3 typography change; the default Selector screenshot is byte-identical despite a 20→30px button box. THEME: no existing target/token is removed; namespaced icon overrides become reachable through defineTheme.

VERDICT: BLOCKS — the new isRenderable behavior does not cover its reachable class.

PERFORMANCE & RESOURCES

EFFECTS: + useEffect [editor, editable] at RichTextEditor.tsx:836 — one-way synchronization to Lexical's external editor state.

Effect + deps	external system	why render/handler cannot do it	measured render cost	lifetime + cleanup	focused test
useEffect [editor, editable]	Lexical editor	props can change without a user event; initialConfig.editable is mount-only	N=1/4/10: one React update commit in every arm; 1/4/10 setEditable calls; no extra React commit	no listener/layout read; one call per mounted editor per editability change	local profiler probe + PR toggle tests

RENDER: flat one React commit at N=1/4/10; the Effect adds linear Lexical calls but no state-driven React pass. Repo stories render at most one editor together; N=10 was the stress bound. LISTENERS/OBSERVERS: none added by the Effect. LAYOUT: no layout/style read or listener; setEditable updates Lexical's external editable state. BUNDLE: no dependency change.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the PR intentionally changes opacity, target geometry, type size, and RTL glyph paint while claiming the Selector pixels remain unchanged.

Four matched parent/head pairs were captured in Chromium on port 6524 with passing receipts: neutral/light, 900×500@1, exact story/state, zero page errors, same sensors except Build. The client has the shown artifacts PR #5204 — Selector before/after, Read-only before/after, RTL toolbar before/after, and Touch toolbar before/after.

pair	computed delta	geometry/state receipt
Selector default	0 / 450,000 pixels	button 200×20 → 200×30
seeded read-only	3,421 / 450,000 pixels	opacity .5 → 1; tabIndex -1 → 0
RTL toolbar	669 / 450,000 pixels	undo/redo wrapper transform none → scaleX(-1)
coarse toolbar	8,451 / 450,000 pixels	eight controls 28×28 → 44×44

The image reader returned “model protocol cannot carry media,” and private attachment upload was denied; therefore the reviewer could not perform the required by-eye judgement or embed the pixels in this markdown block.

VERDICT: not looked at — receipts and pixel deltas are banked, but the frames were not visible to the reviewer.

A11Y & I18N

Exact-head pr-a11y and pr-rtl ran green; the baseline shrinks by 13 and 345 targeted PR tests pass. Chromium proved read-only enters the tab order with aria-readonly, full opacity, and contenteditable=false; disabled stays skipped with aria-disabled, opacity .5; RTL glyphs flip once; coarse toolbar controls measure 44×44.

A public call <RichTextView value={value} /> still compiles and the review test fails toHaveAccessibleName(); the PR's own test expects the textbox to remain unnamed after only warning. Current rubric 1.12 also reclassifies the original toolbar target: before this PR it measured 68×20 fine / 77×20 coarse, while the nearest target began 71px / 75px from its center — well outside the 12px radius — so WCAG 2.5.8's spacing exception passes; 44px coarse expansion remains a FIX-level house improvement, not one of eight BLOCKs.

VERDICT: BLOCKS — the A1 accessible-name defect remains reachable through the legal API and its JSDoc example.

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	BLOCKS — independent/unrecorded decisions need a split
ARCHITECTURE	note — sound owners, four systems
IMPACT	BLOCKS — reachable holes + public test surface
API	BLOCKS — optional label, incomplete utility, test export
THEMING	note — rendered treatment uninspected
BREAKING	BLOCKS — incomplete array behavior
PERFORMANCE	clear
VISUAL	not looked at — image transport prevented inspection
A11Y & I18N	BLOCKS — unnamed textbox remains legal

PRIOR REVIEW: gonzoblasco called the close-out strong and highlighted the state split and lint root cause; this extends rather than contradicts that review by exercising legal public calls and the current rubric. GOAL: partly met — six live audit BLOCK classes are fixed; A1 remains, and the former A10 block now passes rubric 1.12's spacing exception. DISPOSITION: unnamed legal RichTextView → blocks now; all-empty nonempty array → blocks now; public test reset → blocks now; 15-decision bundle/stale A10 claim → split before review; missing by-eye visual inspection → blocks any later approval, not this request-changes verdict; en.json merge conflict → merge main. ADVICE: proven remedy — require the established label contract and update the JSDoc call site; recurse array children in isRenderable until one is renderable; keep warning-reset control out of the public barrel; split into richtext a11y/i18n, icon theming, Selector sizing, and core utility changes, starting with the first. AUTHOR CAN PROCEED: yes — each block has outcome-based acceptance criteria and no unresolved owner/API/design choice is being handed to the contributor. WORST OUTCOME: “a public call <RichTextView value={value} /> still compiles and the review test fails toHaveAccessibleName()” → request changes.

JUDGEMENT NEEDED: none — existing accessibility, utility, and public-surface contracts decide these findings.

request changes

1. RichTextView.label is optional and the JSDoc still omits it → a builder follows a legal documented call and ships an unnamed textbox · RichTextView.tsx:66,158

2. isRenderable treats every nonempty array as visible content → a conditional mapped slot containing only null/false still draws an empty divider or wrapper · isRenderable.ts:33

3. Fifteen decisions include a public test reset and an obsolete A10 premise → core builders inherit unsupported API while the six ready editor fixes stay blocked behind unrelated review · utils/index.ts:106, Selector.test.tsx:745

REVIEW

Thanks — the lint root cause and read-only/disabled split are strong. This still bundles about fifteen independent decisions; __resetDevWarnings is one test-only public export with no product case.

Two correctness holes remain: <RichTextView value={json} /> still compiles to an unnamed textbox, and isRenderable([null, false, '']) returns true although React renders nothing. This also needs a merge with main.

Could you stack this as (1) required label + read-only/disabled, (2) i18n, (3) icon theming, (4) Selector sizing, and (5) one core utility per PR, with lint coverage last once the package is clean? Start with 1. If you'd rather talk it through with someone, we're in Discord.

INLINE (0–3, only when genuinely line-anchored)

• packages/richtext/src/RichTextView.tsx:66 — This needs to be required; the optional path still ships an unnamed textbox.
• packages/core/src/utils/isRenderable.ts:33 — A nonempty array can still contain only empty React children.
• packages/core/src/utils/index.ts:106 — This test-only reset should stay out of the public package barrel.

EVIDENCE I DID NOT SPEND

• Exact-head CI ran 18 checks; all executable jobs passed and only review-required remains pending.
• Targeted changed suites passed: 8 files, 345 tests; the two independent review probes fail exactly on the unnamed textbox and all-empty array.
• The head conflicts with current main only in packages/core/locales/en.json; current main also has five later Selector commits.

TIME

TIME total 30m setup 6m dedicated worktree + fast install + one build + Storybook 6524 (warm main reused: yes) reading 9m full brief/critic, PR body, 4,107-line diff, current rubric/conventions, prior review/main drift measuring 10m 345 tests, 6 review probes, 4 parent/head frame pairs, 4 Chromium arms; 2 re-measures after sensor/class checks writing 5m three presentations + two critic passes waste 5m $KIT was unset; first Storybook start lacked a log; three sensor retries; one intercepted browser click; two blocked image-upload routes

WHAT I COULD NOT VERIFY

• By-eye pixel judgement: captures and receipts exist, but this session's image renderer returned no pixels and private upload was denied.
• Safari/Firefox behavior: managed-Mac policy supports Chromium only.