Skip to content

[WC-3464] Rich text Tiptapeditor#2270

Open
gjulivan wants to merge 5 commits into
mainfrom
rich-text/tiptapeditor
Open

[WC-3464] Rich text Tiptapeditor#2270
gjulivan wants to merge 5 commits into
mainfrom
rich-text/tiptapeditor

Conversation

@gjulivan

Copy link
Copy Markdown
Collaborator

Pull request type


Description

@gjulivan gjulivan force-pushed the rich-text/tiptapeditor branch from 584fe21 to 96df6fb Compare July 1, 2026 08:34
@github-actions

This comment has been minimized.

@gjulivan gjulivan force-pushed the rich-text/tiptapeditor branch from 96df6fb to 57a6695 Compare July 1, 2026 08:53
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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; loading state handling
src/RichText.xml Widget manifest; new properties
src/components/Editor.tsx TipTap editor setup, extension configuration
src/components/EditorWrapper.tsx Value binding, debounce, status bar wiring
src/components/EditorContext.tsx Code-view reducer, context provider
src/components/StatusBar.tsx New status bar component
src/components/toolbars/Toolbar.tsx Toolbar rendering, context
src/components/toolbars/ToolbarConfig.ts All button/group definitions
src/components/toolbars/components/ToolbarButton.tsx Button renderer
src/components/toolbars/components/LinkDialog.tsx Link insert/edit dialog
src/components/toolbars/components/ImageDialog.tsx Image insert dialog
src/components/toolbars/components/VideoDialog.tsx Video/embed insert dialog
src/components/toolbars/components/ConfirmDialog.tsx Code-view save confirm dialog
src/components/toolbars/hooks/useDropdown.ts Floating UI positioning hook
src/utils/embedCodeParser.ts Embed iframe parser with domain whitelist
src/RichText.editorConfig.ts Studio Pro design-time config
src/RichText.editorPreview.tsx Studio Pro preview component
src/__tests__/RichText.spec.tsx Main widget unit tests
src/__tests__/helpers.spec.ts New tests for indent normalisation helpers
src/__tests__/fonts.spec.ts New tests for font attributors
src/__tests__/customList.spec.ts New tests for custom list blot helpers
e2e/RichText.spec.js E2E tests (new class-mode tests added)
CHANGELOG.md Unreleased section updated

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


Findings

🔶 Medium — E2E tests use stale Quill selectors after migration to TipTap

File: e2e/RichText.spec.js lines 36, 51, 131, 144, 156
Problem: Several tests click .ql-toolbar button.ql-image, .ql-toolbar button.ql-view-code, .ql-toolbar button.ql-video, and query .ql-editor. The widget has been rewritten on TipTap; these Quill-specific CSS class selectors no longer exist. Every test that touches these selectors will fail at runtime against the new editor.
Fix: Update selectors to match the new TipTap DOM. Buttons are rendered as <button title="…"> inside .tiptap-toolbar; the editor content is in .tiptap-editor. Example replacements:

// old Quill selectors
await page.click(".mx-name-richText1 .ql-toolbar button.ql-image");
await page.locator(".mx-name-richText1 .ql-editor")

// new TipTap selectors
await page.click('.mx-name-richText1 .tiptap-toolbar button[title="Insert Image"]');
await page.locator(".mx-name-richText1 .tiptap-editor")

🔶 Medium — page.waitForLoadState("networkidle") used in E2E tests

File: e2e/RichText.spec.js lines 123, 130, 143
Problem: The e2e guidelines explicitly require replacing page.waitForLoadState("networkidle") with waitForMendixApp(page) or a web-first locator assertion. The new class-mode tests added in this PR use networkidle instead.
Fix:

// replace
await page.waitForLoadState("networkidle");

// with
await waitForMendixApp(page);
// or
await expect(page.locator(".mx-name-richText1")).toBeVisible();

🔶 Medium — useEffect uses dialogRef.current as a dependency

File: src/components/toolbars/components/ImageDialog.tsx line 157
Problem: dialogRef.current is a mutable ref value, not a reactive dependency. Using it in the useEffect dependency array is a React anti-pattern — it will not cause the effect to re-run when the ref populates, and ESLint's exhaustive-deps rule flags it. The current code works because the ref is always populated on first mount, but the eslint-disable comment confirms the issue was noticed and silently suppressed rather than fixed.
Fix: Remove dialogRef.current from the dep array and capture the node in a local variable inside the effect (the standard cleanup pattern):

useEffect(() => {
    const imgRef = dialogRef.current;
    if (!imgRef) return;
    imgRef.addEventListener("imageSelected", handleImageSelected as EventListener);
    return () => {
        imgRef.removeEventListener("imageSelected", handleImageSelected as EventListener);
    };
    // handleImageSelected reads local state so include it
}, [handleImageSelected]);

Also convert handleImageSelected to useCallback so the dep array stays stable.


🔶 Medium — XML property key OverflowY is not lowerCamelCase

File: src/RichText.xml line 127
Problem: Mendix XML property keys must use lowerCamelCase. OverflowY starts with an uppercase letter, violating the convention (it should be overflowY). This is a pre-existing issue, but the XML was modified in this PR, making it the right time to correct it.
Fix:

<!-- before -->
<property key="OverflowY" type="enumeration" defaultValue="auto">
    <caption>Vertical Overflow</caption>

<!-- after -->
<property key="overflowY" type="enumeration" defaultValue="auto">
    <caption>Vertical Overflow</caption>

The TypeScript prop type and all references to OverflowY in the codebase must be updated to match.


⚠️ Low — Unit tests are snapshot-only with no behavioural assertions

File: src/__tests__/RichText.spec.tsx
Note: All six test cases assert only toMatchSnapshot(). Since TipTap initialises asynchronously, the snapshots capture the pre-hydrated shell (essentially just a wrapper div). They give zero coverage of editor rendering, event wiring, status bar display, or readonly behaviour. Consider adding at least one behavioural assertion per test case (e.g. presence of .widget-rich-text, .rich-text-status-bar, or .form-control-static classes) as a minimum guard against regressions.


⚠️ Low — handleSubmit in VideoDialog calls onClose() before checking embed validity

File: src/components/toolbars/components/VideoDialog.tsx lines 150–164
Note: handleEmbedSubmit returns early when !parsed.valid, but control returns to handleSubmit which immediately calls onClose() on line 163. This means a bad embed code will silently close the dialog without inserting anything and without showing the validation error. Move onClose() into each branch after a successful insertion:

const handleSubmit = (e: FormEvent): void => {
    e.preventDefault();
    if (!editor) return;
    const w = parseInt(width, 10);
    const h = parseInt(height, 10);
    if (activeTab === "url") {
        handleUrlSubmit(w, h);
        if (!validationError) onClose();
    } else {
        const success = handleEmbedSubmit(w, h);
        if (success) onClose();
    }
};

⚠️ Low — any cast on imageAttrs bypasses type safety

File: src/components/toolbars/components/ImageDialog.tsx line 115
Note: const imageAttrs: any = { ... } suppresses TypeScript checking for the attributes passed to setImage. Consider typing this as Parameters<typeof editor.commands.setImage>[0] or a narrow { src: string; alt?: string; title?: string; dataEntity?: boolean; dataEntityId?: string } to keep the type checked.


Positives

  • embedCodeParser.ts has solid defense-in-depth: it blocks javascript: URLs, data: URLs, non-HTTP protocols, and enforces a domain whitelist — exactly the right approach for third-party embeds.
  • EditorContext uses a proper useReducer for code-view state instead of multiple useState calls, making state transitions explicit and easy to test.
  • New unit tests (helpers.spec.ts, fonts.spec.ts, customList.spec.ts) are well-structured with describe/it blocks and cover both happy-path and edge cases.
  • The useDropdown hook correctly cleans up the mousedown global listener in the effect return function, avoiding the event-leak anti-pattern.
  • EditorWrapper guards the <Editor> render behind stringAttribute.status === "available", properly preventing premature rendering before Mendix data is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant