[WC-3464] Rich text Tiptapeditor

[gjulivan]

Pull request type

---

Description

[yordan-st]

Would have a look at the high and medium flagged CC review comments

[github-actions]

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

---

What was reviewed

File	Change
src/RichText.tsx	Top-level widget component
src/components/EditorWrapper.tsx	Wraps editor + status bar, handles save debounce
src/components/Editor.tsx	TipTap editor + extensions initialization
src/components/EditorContext.tsx	Context + code-view reducer
src/components/LinkBubbleMenu.tsx	Contextual link bubble menu (new)
src/components/HighlightedCodeEditor.tsx	highlight.js code view (replaces CodeMirror)
src/components/toolbars/Toolbar.tsx	Main toolbar composition
src/components/toolbars/components/HelpButton.tsx	Help button (new)
src/components/toolbars/components/HelpDialog.tsx	Keyboard shortcuts modal (new)
src/components/toolbars/components/ImageDialog.tsx	Refactored image dialog (tab visibility fix)
src/components/toolbars/components/LinkDialog.tsx	Link insert/edit dialog (new)
src/components/toolbars/components/ColorPicker.tsx	Color picker with clear button (new)
src/components/toolbars/components/ConfirmDialog.tsx	Code-save confirm dialog
src/components/toolbars/components/ToolbarButton.tsx	Base toolbar button
src/components/toolbars/components/ToolbarDefaultButton.tsx	Shared button primitive
src/components/DynamicTableStyles.tsx	Dynamic background-color style injection
src/extensions/TableHeaderBackgroundColor.ts	Header-cell styling (new)
src/extensions/tableCellStyling.ts	Shared cell-styling utilities (new)
src/utils/helpers.ts	CSS/URL injection guards
src/utils/i18n/context.tsx	i18n provider (new)
src/utils/i18n/resolveLocale.ts	Locale detection (new)
src/RichText.xml	Widget manifest
typings/RichTextProps.d.ts	Generated typings
src/RichText.editorConfig.ts	Studio Pro properties editor
src/**/__tests__/*.spec.*	Unit tests
e2e/RichText.spec.js	E2E tests
CHANGELOG.md	Changelog

Skipped (out of scope): dist/, pnpm-lock.yaml, openspec/ docs

---

Findings

🔶 Medium — E2E snapshot baselines missing for new tests

File: e2e/RichText.spec.js lines 37-38, 51-52
Problem: The spec calls toHaveScreenshot("insertImageDialog.png"), toHaveScreenshot("viewCodeDialog.png"), and toHaveScreenshot("classModeViewCodeDialog.png") but none of those PNG baselines exist under e2e/RichText.spec.js-snapshots/. When a CI runner executes these tests for the first time without committed baselines, Playwright will fail (no reference image to compare against). Only 3 of the 6 baseline files referenced in the three new test bodies are committed.
Fix: Run pnpm run test:e2e --update-snapshots locally, then commit the generated PNGs for insertImageDialog-chromium-linux.png, viewCodeDialog-chromium-linux.png, and classModeViewCodeDialog-chromium-linux.png alongside the code change.

---

🔶 Medium — useEffect in ImageDialog captures a stale ref

File: src/components/toolbars/components/ImageDialog.tsx lines 154–165
Problem: The effect uses dialogRef.current as the variable to add/remove the event listener, but the dep array is [dialogRef.current]. dialogRef.current is a mutable value, not a stable identity, so React does not rerun the effect when it changes. Worse, handleImageSelected is not listed in the dep array so it closes over stale state. The // eslint-disable-next-line react-hooks/exhaustive-deps comment suppresses the lint error rather than fixing the problem.
Fix:

useEffect(() => {
    const imgRef = dialogRef.current;
    if (!imgRef) return;
    imgRef.addEventListener("imageSelected", handleImageSelected as EventListener);
    return () => {
        imgRef.removeEventListener("imageSelected", handleImageSelected as EventListener);
    };
}, [handleImageSelected]); // wrap handleImageSelected in useCallback in the component

Remove the eslint-disable comment once the deps are correct.

---

🔶 Medium — ConfirmDialog has no ARIA role/label — inaccessible to screen readers

File: src/components/toolbars/components/ConfirmDialog.tsx lines 27–42
Problem: The confirm dialog root div has no role="dialog", no aria-modal, and no aria-labelledby. A screen reader user asking "what is this?" gets no semantic information. By contrast, HelpDialog is correctly annotated.
Fix:

<div
  ref={dialogRef}
  className="toolbar-dialog confirm-dialog"
  role="dialog"
  aria-modal="true"
  aria-labelledby="confirm-dialog-title"
  tabIndex={-1}
>
  {title && <h3 id="confirm-dialog-title">{title}</h3>}
  {/* also move initial focus here the same way HelpDialog does */}

---

🔶 Medium — page.waitForTimeout(500) in E2E test (hardcoded sleep)

File: e2e/RichText.spec.js line 211
Problem: The test calls await page.waitForTimeout(500) after pressing Tab to blur the editor. This is a brittle time-based wait that will fail on slow CI machines and is explicitly forbidden by the E2E guidelines.
Fix: Replace the sleep with a web-first assertion that proves the save path has run. For example, wait for the hidden Mendix attribute storage to have updated, or assert that the editor is no longer focused:

// Option A: wait for the editor placeholder to appear (proves it's truly empty)
await expect(page.locator(".mx-name-richText1 .tiptap p.is-editor-empty")).toBeVisible();

// Option B: assert the text content is already observed to be empty (Playwright auto-retries)
await expect(page.locator(".mx-name-richText1 .tiptap")).toHaveText("");

---

⚠️ Low — actionRef memoized on the whole props object — re-creates every render

File: src/components/Editor.tsx lines 161–172
Problem: actionRef is created via useMemo(() => ({current: {...}}), [props]). Because props is a new object reference every render, useMemo provides no benefit here. The intent is a stable ref that avoids stale closures in TipTap callbacks — but the current implementation defeats that purpose. A real ref (useRef) is the idiomatic solution.
Fix:

const actionRef = useRef({
    onChange: props.onChange,
    onChangeType: props.onChangeType,
    onFocus: props.onFocus,
    onBlur: props.onBlur,
    onLoad: props.onLoad,
});
useEffect(() => {
    actionRef.current = {
        onChange: props.onChange,
        onChangeType: props.onChangeType,
        onFocus: props.onFocus,
        onBlur: props.onBlur,
        onLoad: props.onLoad,
    };
});

---

⚠️ Low — isSafeCssTextAlign is case-sensitive but its allowlist only contains lowercase

File: src/utils/helpers.ts lines 47, 66–68
Problem: CSS_TEXT_ALIGN_ALLOWLIST contains lowercase values ("left", "center", …), but isSafeCssTextAlign calls value.trim().toLowerCase() correctly — so this is actually fine. However, isSafeCssBorderStyle is also lowercase-correct. The inconsistency is that isSafeCssColor does not lowercase the value before checking the unsafe regex, which means URL(evil) (uppercase URL) bypasses the url\( check. The CSS_COLOR_UNSAFE regex has no i flag but the allowlist uses /i.
Fix: Add the i flag to CSS_COLOR_UNSAFE to cover uppercase variants:

const CSS_COLOR_UNSAFE = /[{}<>;@]|url\(|expression\(|\/\*|\\/i; // already has /i ✓

Wait — reviewing again, the regex does end with /i. This is actually fine. The note here is just for awareness; no change needed.

---

⚠️ Low — any type cast in ToolbarButton.tsx command dispatch

File: src/components/toolbars/components/ToolbarButton.tsx lines 54, 58, 62, 67, 70, 73, 79
Problem: Every command invocation casts editor.chain().focus() to any to bypass TypeScript. This entirely disables type checking for the command name and arguments, so typos silently become runtime errors.
Note: This pattern existed before this PR and is a pre-existing tech debt. If a refactor is out of scope, consider at minimum narrowing the cast to Record<string, (...args: any[]) => any> to surface at least structural errors.

---

Positives

• Security-first approach: The new isSafeCssColor, isSafeCssSize, isSafeCssBorderStyle, isSafeLinkUrl, and isSafeCssFontFamily guards are thorough and well-tested. URL validation correctly strips control characters before scheme detection — a subtle but important protection against java\tscript: bypasses.
• Stale-closure avoidance in editor callbacks: TipTap event handlers (onUpdate, onFocus, onBlur, onCreate) correctly reference actionRef.current rather than closing over live props, preventing stale-action bugs.
• extensions array memoized: Wrapping the extensions list in useMemo(() => [...], [styleDataFormat]) avoids re-registering extensions on every render — the direct fix for the Tiptap duplicate-extension warnings mentioned in the changelog.
• i18n provider design: TranslationProvider resolves the locale once with useMemo, and useT gracefully falls back to English when no provider is mounted — both unit-test friendly and robust in production.
• Comprehensive helpers.ts test suite: helpers.spec.ts covers every guard function including adversarial injection payloads, and both the CSS.supports path and the jsdom-fallback path are tested independently.
• CHANGELOG is detailed and user-facing: The entries describe observable behavior changes (including the breaking <p></p> → "" normalization) without leaking implementation details.