Repository navigation
feat: paragraph/heading blocks with shared rich-text toolbar - #286
ViewFromTheBox merged 2 commits into
Conversation
Adds the first two production block types and a shared Bold/Italic/Link toolbar. Both blocks use a minimal Tiptap editor (document + paragraph or heading + text + hard-break + bold/italic/link) with a block-level keymap that dispatches Enter and Backspace back to the Zustand store. - Split (Enter) and merge (Backspace at start) are committed via replaceBlocks so each operation is a single undo step. - Backspace-merge places the cursor at the merge boundary via a pending cursor sidechannel, not at the end of the combined content. - Icons are served via @fortawesome/react-fontawesome with an Icon wrapper; the toolbar and block drag handle both consume it. - Adds registerCoreBlocks() and wires it into the editor boot so the initial document contains a heading + paragraph. Closes #272
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds paragraph and heading block implementations with Tiptap editors, split/merge behavior, a shared rich-text toolbar (bold/italic/link and heading level switch), supporting utilities (editor registry, split content, TipTap hook), Font Awesome integration, component wiring, tests, and store initialization updates. Changes
Sequence DiagramssequenceDiagram
participant User
participant BlockEditor as Block TipTap Editor
participant SplitHandler as handleBlockEnter
participant Store as EditorStore
participant Registry as BlockEditorRegistry
User->>BlockEditor: Press Enter
BlockEditor->>SplitHandler: invoke splitAtCursor(clientId, editor)
SplitHandler->>BlockEditor: serialize leftHtml / rightHtml
SplitHandler->>Store: update left block attributes (emitUpdate: false)
SplitHandler->>Store: createClientId() -> newId
SplitHandler->>Store: replaceBlocks(insert new paragraph with newId)
SplitHandler->>Registry: setPendingCursor(newId, 'start')
SplitHandler->>BlockEditor: return true (handled)
Store->>Registry: notify subscribers -> render new block/editor
sequenceDiagram
participant User
participant Store as EditorStore
participant Toolbar as RichTextToolbar
participant Registry as BlockEditorRegistry
participant Tiptap as Block TipTap Editor
User->>Store: Select block
Store->>Toolbar: selection changed
Toolbar->>Registry: getBlockEditor(selectedClientId)
Registry->>Toolbar: return Tiptap
User->>Toolbar: Click Bold button
Toolbar->>Tiptap: chain().focus().toggleBold().run()
Tiptap->>Toolbar: onUpdate -> getHTML()
Toolbar->>Store: updateBlockAttributes(selectedClientId, html)
Store->>BlockEditor: re-render with updated attributes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
resources/js/visual-editor/editor/store/ids.ts (1)
9-15: Consider normalizing client ID format across environments.Line 10 returns a raw UUID while Line 15 returns a
ve-...ID. Keeping one stable shape (for example always prefixing withve-) makes logs/test selectors/debugging more consistent.Suggested tweak
export function createClientId(): string { if (typeof globalThis.crypto?.randomUUID === 'function') { - return globalThis.crypto.randomUUID(); + return `ve-${globalThis.crypto.randomUUID()}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/store/ids.ts` around lines 9 - 15, The ID generator currently returns a raw UUID when globalThis.crypto.randomUUID exists but returns a prefixed fallback (`ve-${Date.now().toString(36)}-${counter.toString(36)}`) otherwise; normalize to a single shape by always returning IDs with the "ve-" prefix. In the branch that uses globalThis.crypto.randomUUID, wrap the UUID with the same "ve-" prefix (so the branch using globalThis.crypto.randomUUID returns `ve-<uuid>`), and keep the existing fallback that uses counter and Date so both paths produce IDs with the `ve-` prefix; update references to globalThis.crypto.randomUUID, counter, and the template literal to implement this change.resources/js/visual-editor/editor/components/Icon.tsx (1)
13-20: Consider conditionally passingtitleto avoid empty title attributes.When
titleis an empty string, it will still be passed toFontAwesomeIcon, potentially rendering an emptytitleattribute in the DOM. For cleaner output:✨ Suggested refinement
return ( <FontAwesomeIcon icon={icon} className={className} - title={title} + title={hasTitle ? title : undefined} role={hasTitle ? 'img' : undefined} aria-hidden={hasTitle ? undefined : true} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/components/Icon.tsx` around lines 13 - 20, The Icon component is currently passing title to FontAwesomeIcon even when title is an empty string; change the JSX so the title prop is only provided when non-empty (use the existing hasTitle check) — e.g., pass title={hasTitle ? title : undefined} or conditionally spread the title prop in the <FontAwesomeIcon> element to avoid rendering an empty title attribute.resources/js/visual-editor/editor/blocks/__tests__/heading.test.tsx (1)
67-80: Unnecessaryasynckeyword on test function.The test function is marked
asyncbut doesn't await anything meaningful—act()here is synchronous. This is harmless but inconsistent with the non-async test at line 58.✨ Minor cleanup
- it('updates the rendered heading level when attributes.level changes', async () => { + it('updates the rendered heading level when attributes.level changes', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/blocks/__tests__/heading.test.tsx` around lines 67 - 80, The test function labeled "updates the rendered heading level when attributes.level changes" is unnecessarily declared async; remove the async keyword from the it(...) callback so the test matches the synchronous behavior (no awaits) used elsewhere. Locate the test block that calls makeHeading('h1', '<h2>Title</h2>', 2), createEditorStore([block]), renderBlock(store, block), act(() => store.getState().updateBlockAttributes('h1', { level: 4 })); and getBlockEditor('h1'), and change its declaration to a plain synchronous function.resources/js/visual-editor/editor/components/__tests__/RichTextToolbar.test.tsx (1)
105-126: Missing null check for editor before assertion.The test at line 115 uses
editor!directly without a preceding null check, unlike the pattern at lines 90-91 which assertsexpect(editor).not.toBeNull()first.Suggested fix for consistency
const editor = getBlockEditor('p1'); + expect(editor).not.toBeNull(); await act(async () => { editor!.commands.selectAll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/components/__tests__/RichTextToolbar.test.tsx` around lines 105 - 126, The test "toggles italic on the active editor" uses getBlockEditor('p1') and then dereferences editor! without asserting it's not null; add a null-check assertion (e.g., expect(editor).not.toBeNull()) immediately after obtaining editor to match the pattern used earlier, then proceed to call editor.commands.selectAll() and expect(editor.isActive('italic')).toBe(true) so the test fails cleanly if getBlockEditor returns null.resources/js/visual-editor/editor/blocks/shared/splitContent.ts (1)
15-40: Potential type safety issue with DOMSerializer output.
DOMSerializer.serializeNode()can return either anHTMLElementor aTextnode depending on the node type. The cast toHTMLElementon lines 33-34 could fail if an unexpected node type is serialized, though for paragraph/heading nodes this should always produce an element.Defensive type check
const leftDom = serializer.serializeNode(leftNode) as HTMLElement; const rightDom = serializer.serializeNode(rightNode) as HTMLElement; + if (!(leftDom instanceof HTMLElement) || !(rightDom instanceof HTMLElement)) { + return { leftHtml: '', rightHtml: '' }; + } + return { leftHtml: leftDom.outerHTML, rightHtml: rightDom.outerHTML, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/blocks/shared/splitContent.ts` around lines 15 - 40, The call to serializer.serializeNode in splitAtCursor may return a Text node, but the code unconditionally casts to HTMLElement (leftDom/rightDom); update splitAtCursor to defensively handle non-HTMLElement results from serializer.serializeNode(serialized) by checking if the returned value is an instance of HTMLElement and, if not, wrapping the Text node (or other Node) into a container element (e.g., createElement('div') and append the node or use node.textContent) before reading outerHTML; ensure both leftDom and rightDom are processed this way so leftHtml/rightHtml are always strings.resources/js/visual-editor/editor/components/RichTextToolbar.tsx (1)
153-200: Consider restoring editor focus when closing popover via Escape.When the user presses Escape to close the link popover, the editor focus is not explicitly restored. This could leave focus in a limbo state since the input will be unmounted.
Suggested enhancement
} else if (event.key === 'Escape') { event.preventDefault(); + editor.commands.focus(); setOpen(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/components/RichTextToolbar.tsx` around lines 153 - 200, The popover close paths (Escape handler in onKeyDown and the setOpen(false) branches in applyLink/removeLink) don't explicitly restore editor focus, which can leave focus in limbo when the input is unmounted; update the Escape branch in onKeyDown to call editor.chain().focus().run() before setOpen(false), and likewise ensure removeLink and the "unset" branch in applyLink call editor.chain().focus().run() (or keep the existing focus chain calls) so the editor regains focus whenever the link popover is closed; locate these changes in the LinkControl component (functions onKeyDown, applyLink, removeLink).resources/js/visual-editor/editor/blocks/heading/edit.tsx (1)
46-64: Stale closure:editorreferenced before assignment.The
onEnterandonBackspaceAtStartcallbacks referenceeditor, buteditoris the return value ofuseBlockTiptap. At the time these callbacks are defined,editorcaptures the value from the previous render (initiallynull). The null checks work, but the logic relies on the refs pattern insideuseBlockTiptapto get the current callbacks.This works correctly because
useBlockTiptapstores callbacks in refs and reads from.currentat invocation time. However, the pattern is subtle and theif (!editor)checks are checking a potentially stale value.A cleaner pattern would be to accept the editor as a parameter in the callbacks or structure them differently, but given the refs pattern in
useBlockTiptaphandles this correctly, this is a minor readability concern rather than a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/blocks/heading/edit.tsx` around lines 46 - 64, The callbacks close over a potentially stale editor variable; instead accept the current editor instance passed by useBlockTiptap (or read it from a ref) and forward it to the handlers: change the onEnter and onBackspaceAtStart props to signatures like (editorInstance) => handleBlockEnter(store, clientId, editorInstance) and (editorInstance) => handleBlockBackspace(store, clientId, editorInstance), remove the stale null-checks against the outer editor variable, and ensure handleBlockEnter and handleBlockBackspace are called with the supplied editor instance; see useBlockTiptap, onEnter, onBackspaceAtStart, handleBlockEnter, and handleBlockBackspace.resources/js/visual-editor/editor/blocks/shared/blockSplitMerge.ts (1)
121-135: Regex-based HTML manipulation may be fragile with complex attributes.The
extractInnerHtmlandwrapInSameTagfunctions use regex to parse HTML. While this works for the simple paragraph/heading tags currently in use, it could break with:
- Tags containing
>in attribute values:<p data-x="a>b">- Multi-line attributes or content
For Phase 1 scope with controlled output from TipTap, this is acceptable, but consider using DOM parsing for robustness in future phases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/blocks/shared/blockSplitMerge.ts` around lines 121 - 135, The regex used in extractInnerHtml and wrapInSameTag is fragile for tags with attributes containing ">" or multiline attributes; replace the regex approach by using DOM parsing: in extractInnerHtml(parseOuter) use DOMParser to parse outerHtml and return the document.body.firstElementChild?.innerHTML || outerHtml, and in wrapInSameTag(templateHtml, innerHtml) parse templateHtml with DOMParser, grab the first element (including its tagName and attributes), create a new element with that tagName, copy attributes from the parsed template element, set its innerHTML = innerHtml, and serialize/return the outerHTML; if parsing fails, fall back to the current behavior (return innerHtml or outerHtml) to preserve safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/js/visual-editor/editor/components/editor.css`:
- Around line 193-202: The .ve-sr-only rule uses the deprecated clip property;
update the .ve-sr-only CSS declaration to remove or replace clip: rect(0, 0, 0,
0); and use the modern equivalent clip-path: inset(50%); (keep the rest of the
properties intact) so the element remains visually hidden but accessible for
screen readers; locate the .ve-sr-only selector in editor.css to make this
change.
---
Nitpick comments:
In `@resources/js/visual-editor/editor/blocks/__tests__/heading.test.tsx`:
- Around line 67-80: The test function labeled "updates the rendered heading
level when attributes.level changes" is unnecessarily declared async; remove the
async keyword from the it(...) callback so the test matches the synchronous
behavior (no awaits) used elsewhere. Locate the test block that calls
makeHeading('h1', '<h2>Title</h2>', 2), createEditorStore([block]),
renderBlock(store, block), act(() =>
store.getState().updateBlockAttributes('h1', { level: 4 })); and
getBlockEditor('h1'), and change its declaration to a plain synchronous
function.
In `@resources/js/visual-editor/editor/blocks/heading/edit.tsx`:
- Around line 46-64: The callbacks close over a potentially stale editor
variable; instead accept the current editor instance passed by useBlockTiptap
(or read it from a ref) and forward it to the handlers: change the onEnter and
onBackspaceAtStart props to signatures like (editorInstance) =>
handleBlockEnter(store, clientId, editorInstance) and (editorInstance) =>
handleBlockBackspace(store, clientId, editorInstance), remove the stale
null-checks against the outer editor variable, and ensure handleBlockEnter and
handleBlockBackspace are called with the supplied editor instance; see
useBlockTiptap, onEnter, onBackspaceAtStart, handleBlockEnter, and
handleBlockBackspace.
In `@resources/js/visual-editor/editor/blocks/shared/blockSplitMerge.ts`:
- Around line 121-135: The regex used in extractInnerHtml and wrapInSameTag is
fragile for tags with attributes containing ">" or multiline attributes; replace
the regex approach by using DOM parsing: in extractInnerHtml(parseOuter) use
DOMParser to parse outerHtml and return the
document.body.firstElementChild?.innerHTML || outerHtml, and in
wrapInSameTag(templateHtml, innerHtml) parse templateHtml with DOMParser, grab
the first element (including its tagName and attributes), create a new element
with that tagName, copy attributes from the parsed template element, set its
innerHTML = innerHtml, and serialize/return the outerHTML; if parsing fails,
fall back to the current behavior (return innerHtml or outerHtml) to preserve
safety.
In `@resources/js/visual-editor/editor/blocks/shared/splitContent.ts`:
- Around line 15-40: The call to serializer.serializeNode in splitAtCursor may
return a Text node, but the code unconditionally casts to HTMLElement
(leftDom/rightDom); update splitAtCursor to defensively handle non-HTMLElement
results from serializer.serializeNode(serialized) by checking if the returned
value is an instance of HTMLElement and, if not, wrapping the Text node (or
other Node) into a container element (e.g., createElement('div') and append the
node or use node.textContent) before reading outerHTML; ensure both leftDom and
rightDom are processed this way so leftHtml/rightHtml are always strings.
In
`@resources/js/visual-editor/editor/components/__tests__/RichTextToolbar.test.tsx`:
- Around line 105-126: The test "toggles italic on the active editor" uses
getBlockEditor('p1') and then dereferences editor! without asserting it's not
null; add a null-check assertion (e.g., expect(editor).not.toBeNull())
immediately after obtaining editor to match the pattern used earlier, then
proceed to call editor.commands.selectAll() and
expect(editor.isActive('italic')).toBe(true) so the test fails cleanly if
getBlockEditor returns null.
In `@resources/js/visual-editor/editor/components/Icon.tsx`:
- Around line 13-20: The Icon component is currently passing title to
FontAwesomeIcon even when title is an empty string; change the JSX so the title
prop is only provided when non-empty (use the existing hasTitle check) — e.g.,
pass title={hasTitle ? title : undefined} or conditionally spread the title prop
in the <FontAwesomeIcon> element to avoid rendering an empty title attribute.
In `@resources/js/visual-editor/editor/components/RichTextToolbar.tsx`:
- Around line 153-200: The popover close paths (Escape handler in onKeyDown and
the setOpen(false) branches in applyLink/removeLink) don't explicitly restore
editor focus, which can leave focus in limbo when the input is unmounted; update
the Escape branch in onKeyDown to call editor.chain().focus().run() before
setOpen(false), and likewise ensure removeLink and the "unset" branch in
applyLink call editor.chain().focus().run() (or keep the existing focus chain
calls) so the editor regains focus whenever the link popover is closed; locate
these changes in the LinkControl component (functions onKeyDown, applyLink,
removeLink).
In `@resources/js/visual-editor/editor/store/ids.ts`:
- Around line 9-15: The ID generator currently returns a raw UUID when
globalThis.crypto.randomUUID exists but returns a prefixed fallback
(`ve-${Date.now().toString(36)}-${counter.toString(36)}`) otherwise; normalize
to a single shape by always returning IDs with the "ve-" prefix. In the branch
that uses globalThis.crypto.randomUUID, wrap the UUID with the same "ve-" prefix
(so the branch using globalThis.crypto.randomUUID returns `ve-<uuid>`), and keep
the existing fallback that uses counter and Date so both paths produce IDs with
the `ve-` prefix; update references to globalThis.crypto.randomUUID, counter,
and the template literal to implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44f488a1-0a97-4912-8e8f-e753cf600556
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (23)
package.jsonresources/js/visual-editor/editor/blocks/__tests__/heading.test.tsxresources/js/visual-editor/editor/blocks/__tests__/paragraph.test.tsxresources/js/visual-editor/editor/blocks/heading/edit.tsxresources/js/visual-editor/editor/blocks/heading/index.tsresources/js/visual-editor/editor/blocks/index.tsresources/js/visual-editor/editor/blocks/paragraph/edit.tsxresources/js/visual-editor/editor/blocks/paragraph/index.tsresources/js/visual-editor/editor/blocks/shared/blockEditorRegistry.tsresources/js/visual-editor/editor/blocks/shared/blockSplitMerge.tsresources/js/visual-editor/editor/blocks/shared/index.tsresources/js/visual-editor/editor/blocks/shared/splitContent.tsresources/js/visual-editor/editor/blocks/shared/useBlockTiptap.tsresources/js/visual-editor/editor/components/BlockWrapper.tsxresources/js/visual-editor/editor/components/EditorShell.tsxresources/js/visual-editor/editor/components/Icon.tsxresources/js/visual-editor/editor/components/RichTextToolbar.tsxresources/js/visual-editor/editor/components/__tests__/RichTextToolbar.test.tsxresources/js/visual-editor/editor/components/editor.cssresources/js/visual-editor/editor/components/index.tsresources/js/visual-editor/editor/main.tsxresources/js/visual-editor/editor/store/ids.tsresources/js/visual-editor/editor/store/index.ts
`clip: rect(...)` has been deprecated in favor of `clip-path: inset(50%)` for the visually-hidden pattern. Swap it so the rule keeps working in modern browsers without deprecation warnings.
Description
Implements Phase 1.10: ships the first two production block types (paragraph and heading) along with a shared rich-text toolbar that serves both. Builds on the Tiptap strategy decided in #267 (extension picks + paragraph-node approach) and the Zustand store from #265/#266.
Closes: #272
Type of Change
Related Issue
Issue: #272
Motivation and Context
Phase 1 needs at least paragraph and heading blocks before the editor can hold real content. The shared Tiptap factory and rich-text toolbar built here are the foundations every subsequent text-oriented block (list, quote, etc.) will extend.
Changes Made
useBlockTiptap) that wires document + paragraph/heading + text + hard-break + bold/italic/link extensions with a block-level keydown handler and an external-content sync effect.editor/blocks/paragraph/with Enter-splits-into-new-block and Backspace-at-start-merges-into-previous behavior, both committed through a singlereplaceBlockscall so undo reverses the whole operation in one step.editor/blocks/heading/with levels H1–H6 stored as alevelattribute, level-switcher integration, and the same split/merge keymap.editor/components/RichTextToolbar.tsxwith Bold / Italic / Link (popover with URL input + apply/remove) plus a heading-level dropdown slot that appears only for selected headings. Subscribes to the active editor via auseSyncExternalStorehook over ablockEditorRegistry.blockEditorRegistryso the merge handler can tell the previous block's edit component to place the caret at the merge boundary (rather than at the end of the merged content).@fortawesome/react-fontawesome) for all in-editor icons, including the toolbar buttons and the block drag handle.registerCoreBlocks()entry point wired intomain.tsx; the editor now boots with a heading + paragraph instead of placeholder blocks.createClientId()helper ineditor/store/ids.tsfor fresh block IDs.How Has This Been Tested?
Testing Environment:
npm run dev:editor1.0.0-spikeTests Performed:
Accessibility Tests Run
role=\"toolbar\"witharia-label; buttons usearia-label+aria-pressed; link popover usesrole=\"dialog\";Iconwrapper setsaria-hiddenunless atitleis provided, in which case it exposesrole=\"img\")Details:
Toolbar buttons all have explicit
aria-labeltext. The heading level select is wrapped in a<label>with a screen-reader-only span for context. TheIconcomponent exposes icons to assistive tech only when atitleis passed, otherwise it stays decorative.Tests Added
Test details:
21 new Vitest specs across three files:
editor/blocks/__tests__/paragraph.test.tsx— registration, initial content, onUpdate → store, Enter-at-end split, mid-cursor split, Backspace-at-start merge, cursor-at-merge-boundary, single-entry undo for split and for merge.editor/blocks/__tests__/heading.test.tsx— registration,normalizeHeadingLevelguard, initial render, level-attribute → node updates, onUpdate → store.editor/components/__tests__/RichTextToolbar.test.tsx— hidden when no selection, bold/italic toggles apply to the active editor, link popover creates + removes links, heading-level switcher dispatchesupdateBlockAttributes.Full suite: 187 tests passing.
Documentation
Documentation details:
Short JSDoc blocks on the public helpers (
useBlockTiptap,handleBlockEnter,handleBlockBackspace,splitAtCursor,Icon). No wiki or README changes yet; those land with the Phase 1 closeout.Pre-Submission Checklist
Summary by CodeRabbit
New Features
Behavior
Tests
Style